Re: [PATCH v3 2/3] ACPI: PMIC: Replace open coded be16_to_cpu()

From: Hans de Goede
Date: Wed Aug 31 2022 - 14:25:18 EST

Next message: Paul E. McKenney: "[PATCH nolibc 0/18] nolibc updates for v6.1"
Previous message: Paul E. McKenney: "[PATCH rcu 4/4] srcu: Make Tiny SRCU use full-sized grace-period counters"
In reply to: Andy Shevchenko: "[PATCH v3 2/3] ACPI: PMIC: Replace open coded be16_to_cpu()"
Next in thread: Andy Shevchenko: "Re: [PATCH v3 2/3] ACPI: PMIC: Replace open coded be16_to_cpu()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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.

Regards,

Hans

> + reg_val = be16_to_cpu(buf);
>
> - reg_val = temp_l | WHISKEY_COVE_ADC_HIGH_BIT(temp_h);
> - cursrc = WHISKEY_COVE_ADC_CURSRC(temp_h);
> - rlsb = rlsb_array[cursrc];
> - adc_val = reg_val * rlsb / 1000;
> + rlsb = rlsb_array[reg_val >> 12];
> + adc_val = (reg_val & PMIC_REG_MASK) * rlsb / 1000;
>
> return adc_val;
> }
> @@ -325,7 +321,9 @@ intel_bxtwc_pmic_update_aux(struct regmap *regmap, int reg, int raw)
> {
> u32 bsr_num;
> u16 resi_val, count = 0, thrsh = 0;
> - u8 alrt_h, alrt_l, cursel = 0;
> + u16 mask = PMIC_REG_MASK;
> + __be16 buf;
> + u8 cursel;
>
> bsr_num = raw;
> bsr_num /= (1 << 5);
> @@ -336,15 +334,11 @@ intel_bxtwc_pmic_update_aux(struct regmap *regmap, int reg, int raw)
> thrsh = raw / (1 << (4 + cursel));
>
> resi_val = (cursel << 9) | thrsh;
> - alrt_h = (resi_val >> 8) & WHISKEY_COVE_ALRT_HIGH_BIT_MASK;
> - if (regmap_update_bits(regmap,
> - reg - 1,
> - WHISKEY_COVE_ALRT_HIGH_BIT_MASK,
> - alrt_h))
> - return -EIO;
>
> - alrt_l = (u8)resi_val;
> - return regmap_write(regmap, reg, alrt_l);
> + if (regmap_bulk_read(regmap, reg - 1, &buf, sizeof(buf)))
> + return -EIO;
> + buf = cpu_to_be16((be16_to_cpu(buf) & ~mask) | (resi_val & mask));
> + return regmap_bulk_write(regmap, reg - 1, &buf, sizeof(buf));
> }
>
> static int
> diff --git a/drivers/acpi/pmic/intel_pmic_bytcrc.c b/drivers/acpi/pmic/intel_pmic_bytcrc.c
> index 9ea79f210965..ce647bc46978 100644
> --- a/drivers/acpi/pmic/intel_pmic_bytcrc.c
> +++ b/drivers/acpi/pmic/intel_pmic_bytcrc.c
> @@ -6,6 +6,8 @@
> */
>
> #include <linux/acpi.h>
> +#include <linux/bits.h>
> +#include <linux/byteorder/generic.h>
> #include <linux/init.h>
> #include <linux/mfd/intel_soc_pmic.h>
> #include <linux/platform_device.h>
> @@ -14,6 +16,8 @@
>
> #define PWR_SOURCE_SELECT BIT(1)
>
> +#define PMIC_REG_MASK GENMASK(9, 0)
> +
> #define PMIC_A0LOCK_REG 0xc5
>
> static struct pmic_table power_table[] = {
> @@ -219,23 +223,27 @@ static int intel_crc_pmic_update_power(struct regmap *regmap, int reg,
>
> static int intel_crc_pmic_get_raw_temp(struct regmap *regmap, int reg)
> {
> - int temp_l, temp_h;
> + __be16 buf;
>
> /*
> * Raw temperature value is 10bits: 8bits in reg
> * and 2bits in reg-1: bit0,1
> */
> - if (regmap_read(regmap, reg, &temp_l) ||
> - regmap_read(regmap, reg - 1, &temp_h))
> + if (regmap_bulk_read(regmap, reg - 1, &buf, sizeof(buf)))
> return -EIO;
>
> - return temp_l | (temp_h & 0x3) << 8;
> + return be16_to_cpu(buf) & PMIC_REG_MASK;
> }
>
> static int intel_crc_pmic_update_aux(struct regmap *regmap, int reg, int raw)
> {
> - return regmap_write(regmap, reg, raw) ||
> - regmap_update_bits(regmap, reg - 1, 0x3, raw >> 8) ? -EIO : 0;
> + u16 mask = PMIC_REG_MASK;
> + __be16 buf;
> +
> + if (regmap_bulk_read(regmap, reg - 1, &buf, sizeof(buf)))
> + return -EIO;
> + buf = cpu_to_be16((be16_to_cpu(buf) & ~mask) | (raw & mask));
> + return regmap_bulk_write(regmap, reg - 1, &buf, sizeof(buf));
> }
>
> static int intel_crc_pmic_get_policy(struct regmap *regmap,
> diff --git a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
> index 6c2a6da430ed..1e80969c4d89 100644
> --- a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
> +++ b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
> @@ -8,12 +8,16 @@
> */
>
> #include <linux/acpi.h>
> +#include <linux/bits.h>
> +#include <linux/byteorder/generic.h>
> #include <linux/init.h>
> #include <linux/mfd/intel_soc_pmic.h>
> #include <linux/platform_device.h>
> #include "intel_pmic.h"
>
> /* registers stored in 16bit BE (high:low, total 10bit) */
> +#define PMIC_REG_MASK GENMASK(9, 0)
> +
> #define CHTDC_TI_VBAT 0x54
> #define CHTDC_TI_DIETEMP 0x56
> #define CHTDC_TI_BPTHERM 0x58
> @@ -73,7 +77,7 @@ static int chtdc_ti_pmic_get_power(struct regmap *regmap, int reg, int bit,
> if (regmap_read(regmap, reg, &data))
> return -EIO;
>
> - *value = data & 1;
> + *value = data & BIT(0);
> return 0;
> }
>
> @@ -85,13 +89,12 @@ static int chtdc_ti_pmic_update_power(struct regmap *regmap, int reg, int bit,
>
> static int chtdc_ti_pmic_get_raw_temp(struct regmap *regmap, int reg)
> {
> - u8 buf[2];
> + __be16 buf;
>
> - if (regmap_bulk_read(regmap, reg, buf, sizeof(buf)))
> + if (regmap_bulk_read(regmap, reg, &buf, sizeof(buf)))
> return -EIO;
>
> - /* stored in big-endian */
> - return ((buf[0] & 0x03) << 8) | buf[1];
> + return be16_to_cpu(buf) & PMIC_REG_MASK;
> }
>
> static const struct intel_pmic_opregion_data chtdc_ti_pmic_opregion_data = {
> diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
> index 33c5e85294cd..3c7380ec8203 100644
> --- a/drivers/acpi/pmic/intel_pmic_xpower.c
> +++ b/drivers/acpi/pmic/intel_pmic_xpower.c
> @@ -6,11 +6,15 @@
> */
>
> #include <linux/acpi.h>
> +#include <linux/bits.h>
> +#include <linux/byteorder/generic.h>
> #include <linux/init.h>
> #include <linux/mfd/axp20x.h>
> #include <linux/regmap.h>
> #include <linux/platform_device.h>
> +
> #include <asm/iosf_mbi.h>
> +
> #include "intel_pmic.h"
>
> #define XPOWER_GPADC_LOW 0x5b
> @@ -218,7 +222,7 @@ static int intel_xpower_pmic_update_power(struct regmap *regmap, int reg,
> static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap, int reg)
> {
> int ret, adc_ts_pin_ctrl;
> - u8 buf[2];
> + __be16 buf;
>
> /*
> * The current-source used for the battery temp-sensor (TS) is shared
> @@ -255,9 +259,9 @@ 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, sizeof(buf));
> + ret = regmap_bulk_read(regmap, AXP288_GP_ADC_H, &buf, sizeof(buf));
> if (ret == 0)
> - ret = (buf[0] << 4) + ((buf[1] >> 4) & 0x0f);
> + ret = be16_to_cpu(buf) >> 4;
>
> if (adc_ts_pin_ctrl & AXP288_ADC_TS_CURRENT_ON_OFF_MASK) {
> regmap_update_bits(regmap, AXP288_ADC_TS_PIN_CTRL,

Next message: Paul E. McKenney: "[PATCH nolibc 0/18] nolibc updates for v6.1"
Previous message: Paul E. McKenney: "[PATCH rcu 4/4] srcu: Make Tiny SRCU use full-sized grace-period counters"
In reply to: Andy Shevchenko: "[PATCH v3 2/3] ACPI: PMIC: Replace open coded be16_to_cpu()"
Next in thread: Andy Shevchenko: "Re: [PATCH v3 2/3] ACPI: PMIC: Replace open coded be16_to_cpu()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]