[PATCH v3 1/3] ACPI: PMIC: Use sizeof() instead of hard coded value

[PATCH v3 1/3] ACPI: PMIC: Use sizeof() instead of hard coded value

[Andy Shevchenko]

It's better to use sizeof() of a given buffer than spreading
a hard coded value.

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_chtdc_ti.c | 2 +-
 drivers/acpi/pmic/intel_pmic_xpower.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

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);
 
-- 
2.35.1

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

[Andy Shevchenko]

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;
+	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,
-- 
2.35.1

[PATCH v3 3/3] ACPI: PMIC: Convert pr_*() to dev_*() printing macros

[Andy Shevchenko]

Since we have a device pointer in the regmap, use it for
error messages.

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_chtwc.c  | 5 +++--
 drivers/acpi/pmic/intel_pmic_xpower.c | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/pmic/intel_pmic_chtwc.c b/drivers/acpi/pmic/intel_pmic_chtwc.c
index f2c42f4c79ca..25aa3e33b09a 100644
--- a/drivers/acpi/pmic/intel_pmic_chtwc.c
+++ b/drivers/acpi/pmic/intel_pmic_chtwc.c
@@ -236,11 +236,12 @@ static int intel_cht_wc_exec_mipi_pmic_seq_element(struct regmap *regmap,
 						   u32 reg_address,
 						   u32 value, u32 mask)
 {
+	struct device *dev = regmap_get_device(regmap);
 	u32 address;
 
 	if (i2c_client_address > 0xff || reg_address > 0xff) {
-		pr_warn("%s warning addresses too big client 0x%x reg 0x%x\n",
-			__func__, i2c_client_address, reg_address);
+		dev_warn(dev, "warning addresses too big client 0x%x reg 0x%x\n",
+			 i2c_client_address, reg_address);
 		return -ERANGE;
 	}
 
diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c b/drivers/acpi/pmic/intel_pmic_xpower.c
index 3c7380ec8203..a6dc8dd0d191 100644
--- a/drivers/acpi/pmic/intel_pmic_xpower.c
+++ b/drivers/acpi/pmic/intel_pmic_xpower.c
@@ -278,11 +278,12 @@ static int intel_xpower_exec_mipi_pmic_seq_element(struct regmap *regmap,
 						   u16 i2c_address, u32 reg_address,
 						   u32 value, u32 mask)
 {
+	struct device *dev = regmap_get_device(regmap);
 	int ret;
 
 	if (i2c_address != 0x34) {
-		pr_err("%s: Unexpected i2c-addr: 0x%02x (reg-addr 0x%x value 0x%x mask 0x%x)\n",
-		       __func__, i2c_address, reg_address, value, mask);
+		dev_err(dev, "Unexpected i2c-addr: 0x%02x (reg-addr 0x%x value 0x%x mask 0x%x)\n",
+			i2c_address, reg_address, value, mask);
 		return -ENXIO;
 	}
 
-- 
2.35.1

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

[Hans de Goede]

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.

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,

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

[Andy Shevchenko]

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)

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

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

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

[Andy Shevchenko]

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).

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

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