| Message ID | 20220831135749.78743-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/3] ACPI: PMIC: Use sizeof() instead of hard coded value | expand |
On Wed, Aug 31, 2022 at 08:19:24PM +0200, Hans de Goede wrote: > Hi, > > On 8/31/22 15:57, Andy Shevchenko wrote: > > It's easier to understand the nature of a data type when > > it's written explicitly. With that, replace open coded > > endianess conversion. > > > > As a side effect it fixes the returned value of > > intel_crc_pmic_update_aux() since ACPI PMIC core code > > expects negative or zero and never uses positive one. > > > > While at it, use macros from bits.h to reduce a room for mistake. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > --- > > v3: added tag (Mika) > > drivers/acpi/pmic/intel_pmic_bxtwc.c | 50 +++++++++++-------------- > > drivers/acpi/pmic/intel_pmic_bytcrc.c | 20 +++++++--- > > drivers/acpi/pmic/intel_pmic_chtdc_ti.c | 13 ++++--- > > drivers/acpi/pmic/intel_pmic_xpower.c | 10 +++-- > > 4 files changed, 51 insertions(+), 42 deletions(-) > > > > diff --git a/drivers/acpi/pmic/intel_pmic_bxtwc.c b/drivers/acpi/pmic/intel_pmic_bxtwc.c > > index e247615189fa..90a2e62a37e4 100644 > > --- a/drivers/acpi/pmic/intel_pmic_bxtwc.c > > +++ b/drivers/acpi/pmic/intel_pmic_bxtwc.c > > @@ -7,19 +7,20 @@ > > > > #include <linux/init.h> > > #include <linux/acpi.h> > > +#include <linux/bits.h> > > +#include <linux/byteorder/generic.h> > > #include <linux/mfd/intel_soc_pmic.h> > > #include <linux/regmap.h> > > #include <linux/platform_device.h> > > #include "intel_pmic.h" > > > > -#define WHISKEY_COVE_ALRT_HIGH_BIT_MASK 0x0F > > -#define WHISKEY_COVE_ADC_HIGH_BIT(x) (((x & 0x0F) << 8)) > > -#define WHISKEY_COVE_ADC_CURSRC(x) (((x & 0xF0) >> 4)) > > -#define VR_MODE_DISABLED 0 > > -#define VR_MODE_AUTO BIT(0) > > -#define VR_MODE_NORMAL BIT(1) > > -#define VR_MODE_SWITCH BIT(2) > > -#define VR_MODE_ECO (BIT(0)|BIT(1)) > > +#define PMIC_REG_MASK GENMASK(11, 0) > > + > > +#define VR_MODE_DISABLED (0 << 0) > > +#define VR_MODE_AUTO (1 << 0) > > +#define VR_MODE_NORMAL (2 << 0) > > +#define VR_MODE_ECO (3 << 0) > > +#define VR_MODE_SWITCH (4 << 0) > > #define VSWITCH2_OUTPUT BIT(5) > > #define VSWITCH1_OUTPUT BIT(4) > > #define VUSBPHY_CHARGE BIT(1) > > @@ -297,25 +298,20 @@ static int intel_bxtwc_pmic_update_power(struct regmap *regmap, int reg, > > > > static int intel_bxtwc_pmic_get_raw_temp(struct regmap *regmap, int reg) > > { > > - unsigned int val, adc_val, reg_val; > > - u8 temp_l, temp_h, cursrc; > > unsigned long rlsb; > > static const unsigned long rlsb_array[] = { > > 0, 260420, 130210, 65100, 32550, 16280, > > 8140, 4070, 2030, 0, 260420, 130210 }; > > + unsigned int adc_val, reg_val; > > + __be16 buf; > > > > - if (regmap_read(regmap, reg, &val)) > > + if (regmap_bulk_read(regmap, reg - 1, &buf, sizeof(buf))) > > return -EIO; > > - temp_l = (u8) val; > > > > - if (regmap_read(regmap, (reg - 1), &val)) > > - return -EIO; > > - temp_h = (u8) val; > > Hmm, you are changing the order of the register > reads here. The old code is doing: > > read(reg); > read(reg -1); > > Where as the new code is doing: > > read(reg -1); > read(reg); > > The order matters since typically upon reading the > low byte, the high bits will get latched so that > the next read of the high bytes uses the bits > from the same x-bits value as the low 8 bits. > > This avoids things like: > > 1. Entire register value (all bits) 0x0ff > 2. Read reg with low 8 bits, read 0x0ff > 3. Entire register value becomes 0x100 > 4. Read reg with high bits > 5. Combined value now reads as 0x1ff > > I have no idea if the bxtwc PMIC latches > the bits, but giving the lack of documentation > it would IMHO be better to not change the reading order. Interestingly documentation suggests otherwise, e.g.: THRMZN0H_REG Battery Thermal Zone 0 Limit Register High Offset 044H Description Z0HYS Temperature hysteresis value for TCOLD threshold Z0CURHYS Battery ChargerTemperature Zone Current hysteresis for TCOLD (MSBs) 3 bits of the battery charger temperature zone current hysteresis for zones 0/1. TCOLD_H Battery ChargerTemperature Zone Threshold for TCOLD (MSBs) Upper 1 bit of the battery charger temperature zone threshold for zones 0/1. Writes to THRMZN0H (and all thermal zone registers) are not committed until THRMZN0L (lower byte) is written to. Write Before: THRMZN0L_REG.TCOLD_L (Note the last description)
Hi, On 8/31/22 21:20, Andy Shevchenko wrote: > On Wed, Aug 31, 2022 at 08:19:24PM +0200, Hans de Goede wrote: >> Hi, >> >> On 8/31/22 15:57, Andy Shevchenko wrote: >>> It's easier to understand the nature of a data type when >>> it's written explicitly. With that, replace open coded >>> endianess conversion. >>> >>> As a side effect it fixes the returned value of >>> intel_crc_pmic_update_aux() since ACPI PMIC core code >>> expects negative or zero and never uses positive one. >>> >>> While at it, use macros from bits.h to reduce a room for mistake. >>> >>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >>> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> >>> --- >>> v3: added tag (Mika) >>> drivers/acpi/pmic/intel_pmic_bxtwc.c | 50 +++++++++++-------------- >>> drivers/acpi/pmic/intel_pmic_bytcrc.c | 20 +++++++--- >>> drivers/acpi/pmic/intel_pmic_chtdc_ti.c | 13 ++++--- >>> drivers/acpi/pmic/intel_pmic_xpower.c | 10 +++-- >>> 4 files changed, 51 insertions(+), 42 deletions(-) >>> >>> diff --git a/drivers/acpi/pmic/intel_pmic_bxtwc.c b/drivers/acpi/pmic/intel_pmic_bxtwc.c >>> index e247615189fa..90a2e62a37e4 100644 >>> --- a/drivers/acpi/pmic/intel_pmic_bxtwc.c >>> +++ b/drivers/acpi/pmic/intel_pmic_bxtwc.c >>> @@ -7,19 +7,20 @@ >>> >>> #include <linux/init.h> >>> #include <linux/acpi.h> >>> +#include <linux/bits.h> >>> +#include <linux/byteorder/generic.h> >>> #include <linux/mfd/intel_soc_pmic.h> >>> #include <linux/regmap.h> >>> #include <linux/platform_device.h> >>> #include "intel_pmic.h" >>> >>> -#define WHISKEY_COVE_ALRT_HIGH_BIT_MASK 0x0F >>> -#define WHISKEY_COVE_ADC_HIGH_BIT(x) (((x & 0x0F) << 8)) >>> -#define WHISKEY_COVE_ADC_CURSRC(x) (((x & 0xF0) >> 4)) >>> -#define VR_MODE_DISABLED 0 >>> -#define VR_MODE_AUTO BIT(0) >>> -#define VR_MODE_NORMAL BIT(1) >>> -#define VR_MODE_SWITCH BIT(2) >>> -#define VR_MODE_ECO (BIT(0)|BIT(1)) >>> +#define PMIC_REG_MASK GENMASK(11, 0) >>> + >>> +#define VR_MODE_DISABLED (0 << 0) >>> +#define VR_MODE_AUTO (1 << 0) >>> +#define VR_MODE_NORMAL (2 << 0) >>> +#define VR_MODE_ECO (3 << 0) >>> +#define VR_MODE_SWITCH (4 << 0) >>> #define VSWITCH2_OUTPUT BIT(5) >>> #define VSWITCH1_OUTPUT BIT(4) >>> #define VUSBPHY_CHARGE BIT(1) >>> @@ -297,25 +298,20 @@ static int intel_bxtwc_pmic_update_power(struct regmap *regmap, int reg, >>> >>> static int intel_bxtwc_pmic_get_raw_temp(struct regmap *regmap, int reg) >>> { >>> - unsigned int val, adc_val, reg_val; >>> - u8 temp_l, temp_h, cursrc; >>> unsigned long rlsb; >>> static const unsigned long rlsb_array[] = { >>> 0, 260420, 130210, 65100, 32550, 16280, >>> 8140, 4070, 2030, 0, 260420, 130210 }; >>> + unsigned int adc_val, reg_val; >>> + __be16 buf; >>> >>> - if (regmap_read(regmap, reg, &val)) >>> + if (regmap_bulk_read(regmap, reg - 1, &buf, sizeof(buf))) >>> return -EIO; >>> - temp_l = (u8) val; >>> >>> - if (regmap_read(regmap, (reg - 1), &val)) >>> - return -EIO; >>> - temp_h = (u8) val; >> >> Hmm, you are changing the order of the register >> reads here. The old code is doing: >> >> read(reg); >> read(reg -1); >> >> Where as the new code is doing: >> >> read(reg -1); >> read(reg); >> >> The order matters since typically upon reading the >> low byte, the high bits will get latched so that >> the next read of the high bytes uses the bits >> from the same x-bits value as the low 8 bits. >> >> This avoids things like: >> >> 1. Entire register value (all bits) 0x0ff >> 2. Read reg with low 8 bits, read 0x0ff >> 3. Entire register value becomes 0x100 >> 4. Read reg with high bits >> 5. Combined value now reads as 0x1ff >> >> I have no idea if the bxtwc PMIC latches >> the bits, but giving the lack of documentation >> it would IMHO be better to not change the reading order. > > Interestingly documentation suggests otherwise, e.g.: > > THRMZN0H_REG > Battery Thermal Zone 0 Limit Register High > Offset 044H > > Description > > Z0HYS Temperature hysteresis value for TCOLD threshold > > Z0CURHYS Battery ChargerTemperature Zone Current hysteresis for TCOLD (MSBs) > 3 bits of the battery charger temperature zone current hysteresis for zones 0/1. > > TCOLD_H Battery ChargerTemperature Zone Threshold for TCOLD (MSBs) > Upper 1 bit of the battery charger temperature zone threshold for zones 0/1. > Writes to THRMZN0H (and all thermal zone registers) are not committed until > THRMZN0L (lower byte) is written to. > Write Before: THRMZN0L_REG.TCOLD_L > > (Note the last description) I see, but that is about writes and the write path was already first doing a read + write of reg - 1, followed by writing reg 1. So for the write path this patch does not introduce any functional changes. But what about the read path, is read latching the same or does it need the inverse order of writes? Note I think it is likely the read order for proper latching is likely also first high then low, but it would be good to check. If that is indeed the case then this would actually be a bugfix, not just a cleanup. Also you have only checked for 1 of the 4 PMICs you are making changes to in this patch? The commit message suggests this code change does not cause any functional changes, but as discussed it actually does make changes, so this should be in the commit message. Talking about making changes to 4 PMICs unlike patch 1 and 3 the changes in this one are not trivial so IMHO this should be split into 1 patch per PMIC. This has 3 advantages: 1. It makes reviewing easier, during my initial review I stopped at the intel_bxtwc_pmic changes not even realizing more was coming... 2. This makes properly describing the actual functional changes in the commit message a lot easier, otherwise the commit msg is going to become somewhat messy. 3. This will also make reverting things easier if something does break (even if it is just for testing if these changes are the cause of the breakage). ### So I've been taking a closer look at these changes and here are some more remarks: intel_crc_pmic_get_raw_temp() you are again changing the order in which the 2 (low/high) registers are read. This needs to be checked and mentioned in the commit message. intel_crc_pmic_update_aux() unlike the intel_pmic_bxtwc.c equivalent in this case your changes do switch the write-order, assuming the same write order as in bxtwc should be used this would actually be another bugfix. For intel_pmic_chtdc_ti.c this does seems to be purely a refactor. For intel_pmic_xpower.c the original code actually seems to be wrong, the datasheet says: REG 5AH: GPADC pin input ADC data, highest 8bit Bit 7-0 GPADC pin input ADC data, highest 8bit REG 5BH: GPADC pin input ADC data, lowest 4bit Bit 7-4 Reserved Bit 3-0 GPADC pin input ADC data, lowest 4bit So it looks like instead of your patch we actually need the following fix: --- a/drivers/acpi/pmic/intel_pmic_xpower.c +++ b/drivers/acpi/pmic/intel_pmic_xpower.c @@ -257,7 +257,7 @@ static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg) ret = regmap_bulk_read(regmap, AXP288_GP_ADC_H, buf, 2); if (ret == 0) - ret = (buf[0] << 4) + ((buf[1] >> 4) & 0x0f); + ret = (buf[0] << 4) | (buf[1] & 0x0f); if (adc_ts_pin_ctrl & AXP288_ADC_TS_CURRENT_ON_OFF_MASK) { regmap_update_bits(regmap, AXP288_ADC_TS_PIN_CTRL, I will try to make some time to check this on actual hw to see if the code or the doc is right soon-ish Regards, Hans
On Thu, Sep 01, 2022 at 11:02:11AM +0200, Hans de Goede wrote: > On 8/31/22 21:20, Andy Shevchenko wrote: > > On Wed, Aug 31, 2022 at 08:19:24PM +0200, Hans de Goede wrote: > >> On 8/31/22 15:57, Andy Shevchenko wrote: ... > >>> - if (regmap_read(regmap, (reg - 1), &val)) > >>> - return -EIO; > >>> - temp_h = (u8) val; > >> > >> Hmm, you are changing the order of the register > >> reads here. The old code is doing: > >> > >> read(reg); > >> read(reg -1); > >> > >> Where as the new code is doing: > >> > >> read(reg -1); > >> read(reg); > >> > >> The order matters since typically upon reading the > >> low byte, the high bits will get latched so that > >> the next read of the high bytes uses the bits > >> from the same x-bits value as the low 8 bits. > >> > >> This avoids things like: > >> > >> 1. Entire register value (all bits) 0x0ff > >> 2. Read reg with low 8 bits, read 0x0ff > >> 3. Entire register value becomes 0x100 > >> 4. Read reg with high bits > >> 5. Combined value now reads as 0x1ff > >> > >> I have no idea if the bxtwc PMIC latches > >> the bits, but giving the lack of documentation > >> it would IMHO be better to not change the reading order. > > > > Interestingly documentation suggests otherwise, e.g.: > > > > THRMZN0H_REG > > Battery Thermal Zone 0 Limit Register High > > Offset 044H > > > > Description > > > > Z0HYS Temperature hysteresis value for TCOLD threshold > > > > Z0CURHYS Battery ChargerTemperature Zone Current hysteresis for TCOLD (MSBs) > > 3 bits of the battery charger temperature zone current hysteresis for zones 0/1. > > > > TCOLD_H Battery ChargerTemperature Zone Threshold for TCOLD (MSBs) > > Upper 1 bit of the battery charger temperature zone threshold for zones 0/1. > > Writes to THRMZN0H (and all thermal zone registers) are not committed until > > THRMZN0L (lower byte) is written to. > > Write Before: THRMZN0L_REG.TCOLD_L > > > > (Note the last description) > > I see, but that is about writes and the write path was already > first doing a read + write of reg - 1, followed by writing > reg 1. So for the write path this patch does not introduce > any functional changes. But what about the read path, is read > latching the same or does it need the inverse order of writes? > > Note I think it is likely the read order for proper latching > is likely also first high then low, but it would be good to check. > If that is indeed the case then this would actually be a bugfix, > not just a cleanup. > > Also you have only checked for 1 of the 4 PMICs you are making > changes to in this patch? > > The commit message suggests this code change does not cause any > functional changes, but as discussed it actually does make changes, > so this should be in the commit message. > > Talking about making changes to 4 PMICs unlike patch 1 and 3 the changes > in this one are not trivial so IMHO this should be split into 1 patch > per PMIC. This has 3 advantages: > > 1. It makes reviewing easier, during my initial review I stopped > at the intel_bxtwc_pmic changes not even realizing more was coming... > > 2. This makes properly describing the actual functional changes > in the commit message a lot easier, otherwise the commit msg > is going to become somewhat messy. > > 3. This will also make reverting things easier if something does > break (even if it is just for testing if these changes are the cause > of the breakage). > > ### > > So I've been taking a closer look at these changes and here are some > more remarks: > > intel_crc_pmic_get_raw_temp() you are again changing the order > in which the 2 (low/high) registers are read. This needs to be > checked and mentioned in the commit message. > > intel_crc_pmic_update_aux() unlike the intel_pmic_bxtwc.c > equivalent in this case your changes do switch the write-order, > assuming the same write order as in bxtwc should be used > this would actually be another bugfix. > > For intel_pmic_chtdc_ti.c this does seems to be purely a refactor. > > For intel_pmic_xpower.c the original code actually seems > to be wrong, the datasheet says: > > REG 5AH: GPADC pin input ADC data, highest 8bit > Bit 7-0 GPADC pin input ADC data, highest 8bit > > REG 5BH: GPADC pin input ADC data, lowest 4bit > Bit 7-4 Reserved > Bit 3-0 GPADC pin input ADC data, lowest 4bit > So it looks like instead of your patch we actually need Not instead, but probably as a prerequisite fix. > the following fix: > > --- a/drivers/acpi/pmic/intel_pmic_xpower.c > +++ b/drivers/acpi/pmic/intel_pmic_xpower.c > @@ -257,7 +257,7 @@ static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg) > > ret = regmap_bulk_read(regmap, AXP288_GP_ADC_H, buf, 2); > if (ret == 0) > - ret = (buf[0] << 4) + ((buf[1] >> 4) & 0x0f); > + ret = (buf[0] << 4) | (buf[1] & 0x0f); > > if (adc_ts_pin_ctrl & AXP288_ADC_TS_CURRENT_ON_OFF_MASK) { > regmap_update_bits(regmap, AXP288_ADC_TS_PIN_CTRL, > > I will try to make some time to check this on actual hw to see if > the code or the doc is right soon-ish Thanks for your review and explanations. I will split pure cleanups and resend with Mika's tag, and will see what I can do about the rest (considering availability of the documentation and it's fullness).
Hi, On 9/2/22 12:00, Andy Shevchenko wrote: > On Thu, Sep 01, 2022 at 11:02:11AM +0200, Hans de Goede wrote: >> On 8/31/22 21:20, Andy Shevchenko wrote: >>> On Wed, Aug 31, 2022 at 08:19:24PM +0200, Hans de Goede wrote: >>>> On 8/31/22 15:57, Andy Shevchenko wrote: > > ... > >>>>> - if (regmap_read(regmap, (reg - 1), &val)) >>>>> - return -EIO; >>>>> - temp_h = (u8) val; >>>> >>>> Hmm, you are changing the order of the register >>>> reads here. The old code is doing: >>>> >>>> read(reg); >>>> read(reg -1); >>>> >>>> Where as the new code is doing: >>>> >>>> read(reg -1); >>>> read(reg); >>>> >>>> The order matters since typically upon reading the >>>> low byte, the high bits will get latched so that >>>> the next read of the high bytes uses the bits >>>> from the same x-bits value as the low 8 bits. >>>> >>>> This avoids things like: >>>> >>>> 1. Entire register value (all bits) 0x0ff >>>> 2. Read reg with low 8 bits, read 0x0ff >>>> 3. Entire register value becomes 0x100 >>>> 4. Read reg with high bits >>>> 5. Combined value now reads as 0x1ff >>>> >>>> I have no idea if the bxtwc PMIC latches >>>> the bits, but giving the lack of documentation >>>> it would IMHO be better to not change the reading order. >>> >>> Interestingly documentation suggests otherwise, e.g.: >>> >>> THRMZN0H_REG >>> Battery Thermal Zone 0 Limit Register High >>> Offset 044H >>> >>> Description >>> >>> Z0HYS Temperature hysteresis value for TCOLD threshold >>> >>> Z0CURHYS Battery ChargerTemperature Zone Current hysteresis for TCOLD (MSBs) >>> 3 bits of the battery charger temperature zone current hysteresis for zones 0/1. >>> >>> TCOLD_H Battery ChargerTemperature Zone Threshold for TCOLD (MSBs) >>> Upper 1 bit of the battery charger temperature zone threshold for zones 0/1. >>> Writes to THRMZN0H (and all thermal zone registers) are not committed until >>> THRMZN0L (lower byte) is written to. >>> Write Before: THRMZN0L_REG.TCOLD_L >>> >>> (Note the last description) >> >> I see, but that is about writes and the write path was already >> first doing a read + write of reg - 1, followed by writing >> reg 1. So for the write path this patch does not introduce >> any functional changes. But what about the read path, is read >> latching the same or does it need the inverse order of writes? >> >> Note I think it is likely the read order for proper latching >> is likely also first high then low, but it would be good to check. >> If that is indeed the case then this would actually be a bugfix, >> not just a cleanup. >> >> Also you have only checked for 1 of the 4 PMICs you are making >> changes to in this patch? >> >> The commit message suggests this code change does not cause any >> functional changes, but as discussed it actually does make changes, >> so this should be in the commit message. >> >> Talking about making changes to 4 PMICs unlike patch 1 and 3 the changes >> in this one are not trivial so IMHO this should be split into 1 patch >> per PMIC. This has 3 advantages: >> >> 1. It makes reviewing easier, during my initial review I stopped >> at the intel_bxtwc_pmic changes not even realizing more was coming... >> >> 2. This makes properly describing the actual functional changes >> in the commit message a lot easier, otherwise the commit msg >> is going to become somewhat messy. >> >> 3. This will also make reverting things easier if something does >> break (even if it is just for testing if these changes are the cause >> of the breakage). >> >> ### >> >> So I've been taking a closer look at these changes and here are some >> more remarks: >> >> intel_crc_pmic_get_raw_temp() you are again changing the order >> in which the 2 (low/high) registers are read. This needs to be >> checked and mentioned in the commit message. >> >> intel_crc_pmic_update_aux() unlike the intel_pmic_bxtwc.c >> equivalent in this case your changes do switch the write-order, >> assuming the same write order as in bxtwc should be used >> this would actually be another bugfix. >> >> For intel_pmic_chtdc_ti.c this does seems to be purely a refactor. >> >> For intel_pmic_xpower.c the original code actually seems >> to be wrong, the datasheet says: >> >> REG 5AH: GPADC pin input ADC data, highest 8bit >> Bit 7-0 GPADC pin input ADC data, highest 8bit >> >> REG 5BH: GPADC pin input ADC data, lowest 4bit >> Bit 7-4 Reserved >> Bit 3-0 GPADC pin input ADC data, lowest 4bit > >> So it looks like instead of your patch we actually need > > Not instead, but probably as a prerequisite fix. Since there is a hole in the bits: high-byte low-byte bit 11 10 9 8 7 6 5 4 r r r r 3 2 1 0 r = reserved I don't think we can use be16_to_cpu here at all. > >> the following fix: >> >> --- a/drivers/acpi/pmic/intel_pmic_xpower.c >> +++ b/drivers/acpi/pmic/intel_pmic_xpower.c >> @@ -257,7 +257,7 @@ static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg) >> >> ret = regmap_bulk_read(regmap, AXP288_GP_ADC_H, buf, 2); >> if (ret == 0) >> - ret = (buf[0] << 4) + ((buf[1] >> 4) & 0x0f); >> + ret = (buf[0] << 4) | (buf[1] & 0x0f); >> >> if (adc_ts_pin_ctrl & AXP288_ADC_TS_CURRENT_ON_OFF_MASK) { >> regmap_update_bits(regmap, AXP288_ADC_TS_PIN_CTRL, >> >> I will try to make some time to check this on actual hw to see if >> the code or the doc is right soon-ish > > Thanks for your review and explanations. I will split pure cleanups and resend > with Mika's tag, and will see what I can do about the rest (considering > availability of the documentation and it's fullness). Thanks. Regards, Hans
diff --git a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c index 418eec523025..6c2a6da430ed 100644 --- a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c +++ b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c @@ -87,7 +87,7 @@ static int chtdc_ti_pmic_get_raw_temp(struct regmap *regmap, int reg) { u8 buf[2]; - if (regmap_bulk_read(regmap, reg, buf, 2)) + if (regmap_bulk_read(regmap, reg, buf, sizeof(buf))) return -EIO; /* stored in big-endian */ diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c index 61bbe4c24d87..33c5e85294cd 100644 --- a/drivers/acpi/pmic/intel_pmic_xpower.c +++ b/drivers/acpi/pmic/intel_pmic_xpower.c @@ -255,7 +255,7 @@ static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg) if (ret) return ret; - ret = regmap_bulk_read(regmap, AXP288_GP_ADC_H, buf, 2); + ret = regmap_bulk_read(regmap, AXP288_GP_ADC_H, buf, sizeof(buf)); if (ret == 0) ret = (buf[0] << 4) + ((buf[1] >> 4) & 0x0f);