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@xxxxxxxxxxxxxxx>
> > Reviewed-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > ---
> > 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)
--
With Best Regards,
Andy Shevchenko
