diff mbox series

[v2,2/3] ACPI: PMIC: Replace open coded be16_to_cpu()

Message ID 20220830171155.42962-2-andriy.shevchenko@linux.intel.com (mailing list archive)
State Superseded, archived
Headers show
Series [v2,1/3] ACPI: PMIC: Use sizeof() instead of hard coded value | expand

Commit Message

Andy Shevchenko Aug. 30, 2022, 5:11 p.m. UTC 
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>
---
v2: fixed inclusion (Mika), updated other drivers
 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(-)

Comments

Mika Westerberg Aug. 31, 2022, 5:43 a.m. UTC | #1 
On Tue, Aug 30, 2022 at 08:11:54PM +0300, Andy Shevchenko wrote:
> -#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)

IMHO this one is worse than what it was.

Anyway, that's just a nitpick. The other parts look good,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Andy Shevchenko Aug. 31, 2022, 9:34 a.m. UTC | #2 
On Wed, Aug 31, 2022 at 08:43:54AM +0300, Mika Westerberg wrote:
> On Tue, Aug 30, 2022 at 08:11:54PM +0300, Andy Shevchenko wrote:
> > -#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)
> 
> IMHO this one is worse than what it was.

I'm not sure why. Here is obvious wrong use of BIT() macro against
plain numbers. I can split it into a separate change with an explanation
of why it's better. But I think it doesn't worth the churn.

> Anyway, that's just a nitpick. The other parts look good,
> 
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Thanks!
Hans de Goede Aug. 31, 2022, 9:37 a.m. UTC | #3 
Hi,

On 8/31/22 11:34, Andy Shevchenko wrote:
> On Wed, Aug 31, 2022 at 08:43:54AM +0300, Mika Westerberg wrote:
>> On Tue, Aug 30, 2022 at 08:11:54PM +0300, Andy Shevchenko wrote:
>>> -#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)
>>
>> IMHO this one is worse than what it was.
> 
> I'm not sure why. Here is obvious wrong use of BIT() macro against
> plain numbers. I can split it into a separate change with an explanation
> of why it's better. But I think it doesn't worth the churn.

FWIW I'm with Andy here, the VR_MODE_ECO clearly is trying
to just say 3, so this is just a plain enum for values 0-4 and
as such should not use the BIT macros.

Regards,

Hans


>> Anyway, that's just a nitpick. The other parts look good,
>>
>> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> Thanks!
>
Mika Westerberg Aug. 31, 2022, 9:48 a.m. UTC | #4 
On Wed, Aug 31, 2022 at 11:37:21AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 8/31/22 11:34, Andy Shevchenko wrote:
> > On Wed, Aug 31, 2022 at 08:43:54AM +0300, Mika Westerberg wrote:
> >> On Tue, Aug 30, 2022 at 08:11:54PM +0300, Andy Shevchenko wrote:
> >>> -#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)
> >>
> >> IMHO this one is worse than what it was.
> > 
> > I'm not sure why. Here is obvious wrong use of BIT() macro against
> > plain numbers. I can split it into a separate change with an explanation
> > of why it's better. But I think it doesn't worth the churn.
> 
> FWIW I'm with Andy here, the VR_MODE_ECO clearly is trying
> to just say 3, so this is just a plain enum for values 0-4 and
> as such should not use the BIT macros.

Yeah, enum would look better but the << 0 just makes me confused ;-)
David Laight Aug. 31, 2022, 10:06 a.m. UTC | #5 
From: Mika Westerberg
> Sent: 31 August 2022 10:49
> 
> On Wed, Aug 31, 2022 at 11:37:21AM +0200, Hans de Goede wrote:
> > Hi,
> >
> > On 8/31/22 11:34, Andy Shevchenko wrote:
> > > On Wed, Aug 31, 2022 at 08:43:54AM +0300, Mika Westerberg wrote:
> > >> On Tue, Aug 30, 2022 at 08:11:54PM +0300, Andy Shevchenko wrote:
> > >>> -#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)
> > >>
> > >> IMHO this one is worse than what it was.
> > >
> > > I'm not sure why. Here is obvious wrong use of BIT() macro against
> > > plain numbers. I can split it into a separate change with an explanation
> > > of why it's better. But I think it doesn't worth the churn.
> >
> > FWIW I'm with Andy here, the VR_MODE_ECO clearly is trying
> > to just say 3, so this is just a plain enum for values 0-4 and
> > as such should not use the BIT macros.
> 
> Yeah, enum would look better but the << 0 just makes me confused ;-)

No idea what that code is doing.
The values are all used to initialise a .bit structure member.
So maybe BIT() is right.
The _ECO value isn't used at all.

Deeper analysis may be needed.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Andy Shevchenko Aug. 31, 2022, 10:27 a.m. UTC | #6 
On Wed, Aug 31, 2022 at 10:06:09AM +0000, David Laight wrote:
> From: Mika Westerberg
> > Sent: 31 August 2022 10:49
> > On Wed, Aug 31, 2022 at 11:37:21AM +0200, Hans de Goede wrote:
> > > On 8/31/22 11:34, Andy Shevchenko wrote:
> > > > On Wed, Aug 31, 2022 at 08:43:54AM +0300, Mika Westerberg wrote:
> > > >> On Tue, Aug 30, 2022 at 08:11:54PM +0300, Andy Shevchenko wrote:
> > > >>> -#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)
> > > >>
> > > >> IMHO this one is worse than what it was.
> > > >
> > > > I'm not sure why. Here is obvious wrong use of BIT() macro against
> > > > plain numbers. I can split it into a separate change with an explanation
> > > > of why it's better. But I think it doesn't worth the churn.
> > >
> > > FWIW I'm with Andy here, the VR_MODE_ECO clearly is trying
> > > to just say 3, so this is just a plain enum for values 0-4 and
> > > as such should not use the BIT macros.
> > 
> > Yeah, enum would look better but the << 0 just makes me confused ;-)
> 
> No idea what that code is doing.
> The values are all used to initialise a .bit structure member.
> So maybe BIT() is right.
> The _ECO value isn't used at all.
> 
> Deeper analysis may be needed.

So, can you do that since you already started?
diff mbox series

Patch

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,