* [PATCH v2 1/3] ACPI: PMIC: Use sizeof() instead of hard coded value
@ 2022-08-30 17:11 Andy Shevchenko
2022-08-30 17:11 ` [PATCH v2 2/3] ACPI: PMIC: Replace open coded be16_to_cpu() Andy Shevchenko
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Andy Shevchenko @ 2022-08-30 17:11 UTC (permalink / raw)
To: Andy Shevchenko, Hans de Goede, linux-acpi, linux-kernel
Cc: Rafael J. Wysocki, Len Brown, Andy Shevchenko, Mika Westerberg
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>
---
v2: updated another driver as well (due to this no tag added)
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] ACPI: PMIC: Replace open coded be16_to_cpu()
2022-08-30 17:11 [PATCH v2 1/3] ACPI: PMIC: Use sizeof() instead of hard coded value Andy Shevchenko
@ 2022-08-30 17:11 ` Andy Shevchenko
2022-08-31 5:43 ` Mika Westerberg
2022-08-30 17:11 ` [PATCH v2 3/3] ACPI: PMIC: Convert pr_*() to dev_*() printing macros Andy Shevchenko
2022-08-31 5:37 ` [PATCH v2 1/3] ACPI: PMIC: Use sizeof() instead of hard coded value Mika Westerberg
2 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2022-08-30 17:11 UTC (permalink / raw)
To: Andy Shevchenko, Hans de Goede, linux-acpi, linux-kernel
Cc: Rafael J. Wysocki, Len Brown, Andy Shevchenko, Mika Westerberg
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(-)
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] ACPI: PMIC: Convert pr_*() to dev_*() printing macros
2022-08-30 17:11 [PATCH v2 1/3] ACPI: PMIC: Use sizeof() instead of hard coded value Andy Shevchenko
2022-08-30 17:11 ` [PATCH v2 2/3] ACPI: PMIC: Replace open coded be16_to_cpu() Andy Shevchenko
@ 2022-08-30 17:11 ` Andy Shevchenko
2022-08-31 5:44 ` Mika Westerberg
2022-08-31 5:37 ` [PATCH v2 1/3] ACPI: PMIC: Use sizeof() instead of hard coded value Mika Westerberg
2 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2022-08-30 17:11 UTC (permalink / raw)
To: Andy Shevchenko, Hans de Goede, linux-acpi, linux-kernel
Cc: Rafael J. Wysocki, Len Brown, Andy Shevchenko, Mika Westerberg
Since we have a device pointer in the regmap, use it for
error messages.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: new patch
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] ACPI: PMIC: Use sizeof() instead of hard coded value
2022-08-30 17:11 [PATCH v2 1/3] ACPI: PMIC: Use sizeof() instead of hard coded value Andy Shevchenko
2022-08-30 17:11 ` [PATCH v2 2/3] ACPI: PMIC: Replace open coded be16_to_cpu() Andy Shevchenko
2022-08-30 17:11 ` [PATCH v2 3/3] ACPI: PMIC: Convert pr_*() to dev_*() printing macros Andy Shevchenko
@ 2022-08-31 5:37 ` Mika Westerberg
2 siblings, 0 replies; 11+ messages in thread
From: Mika Westerberg @ 2022-08-31 5:37 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Hans de Goede, linux-acpi, linux-kernel, Rafael J. Wysocki,
Len Brown, Andy Shevchenko
On Tue, Aug 30, 2022 at 08:11:53PM +0300, Andy Shevchenko wrote:
> 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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] ACPI: PMIC: Replace open coded be16_to_cpu()
2022-08-30 17:11 ` [PATCH v2 2/3] ACPI: PMIC: Replace open coded be16_to_cpu() Andy Shevchenko
@ 2022-08-31 5:43 ` Mika Westerberg
2022-08-31 9:34 ` Andy Shevchenko
0 siblings, 1 reply; 11+ messages in thread
From: Mika Westerberg @ 2022-08-31 5:43 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Hans de Goede, linux-acpi, linux-kernel, Rafael J. Wysocki,
Len Brown, Andy Shevchenko
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] ACPI: PMIC: Convert pr_*() to dev_*() printing macros
2022-08-30 17:11 ` [PATCH v2 3/3] ACPI: PMIC: Convert pr_*() to dev_*() printing macros Andy Shevchenko
@ 2022-08-31 5:44 ` Mika Westerberg
0 siblings, 0 replies; 11+ messages in thread
From: Mika Westerberg @ 2022-08-31 5:44 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Hans de Goede, linux-acpi, linux-kernel, Rafael J. Wysocki,
Len Brown, Andy Shevchenko
On Tue, Aug 30, 2022 at 08:11:55PM +0300, Andy Shevchenko wrote:
> 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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] ACPI: PMIC: Replace open coded be16_to_cpu()
2022-08-31 5:43 ` Mika Westerberg
@ 2022-08-31 9:34 ` Andy Shevchenko
2022-08-31 9:37 ` Hans de Goede
0 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2022-08-31 9:34 UTC (permalink / raw)
To: Mika Westerberg
Cc: Hans de Goede, linux-acpi, linux-kernel, Rafael J. Wysocki,
Len Brown, Andy Shevchenko
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!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] ACPI: PMIC: Replace open coded be16_to_cpu()
2022-08-31 9:34 ` Andy Shevchenko
@ 2022-08-31 9:37 ` Hans de Goede
2022-08-31 9:48 ` Mika Westerberg
0 siblings, 1 reply; 11+ messages in thread
From: Hans de Goede @ 2022-08-31 9:37 UTC (permalink / raw)
To: Andy Shevchenko, Mika Westerberg
Cc: linux-acpi, linux-kernel, Rafael J. Wysocki, Len Brown, Andy Shevchenko
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!
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] ACPI: PMIC: Replace open coded be16_to_cpu()
2022-08-31 9:37 ` Hans de Goede
@ 2022-08-31 9:48 ` Mika Westerberg
2022-08-31 10:06 ` David Laight
0 siblings, 1 reply; 11+ messages in thread
From: Mika Westerberg @ 2022-08-31 9:48 UTC (permalink / raw)
To: Hans de Goede
Cc: Andy Shevchenko, linux-acpi, linux-kernel, Rafael J. Wysocki,
Len Brown, Andy Shevchenko
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 ;-)
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v2 2/3] ACPI: PMIC: Replace open coded be16_to_cpu()
2022-08-31 9:48 ` Mika Westerberg
@ 2022-08-31 10:06 ` David Laight
2022-08-31 10:27 ` Andy Shevchenko
0 siblings, 1 reply; 11+ messages in thread
From: David Laight @ 2022-08-31 10:06 UTC (permalink / raw)
To: 'Mika Westerberg', Hans de Goede
Cc: Andy Shevchenko, linux-acpi, linux-kernel, Rafael J. Wysocki,
Len Brown, Andy Shevchenko
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)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] ACPI: PMIC: Replace open coded be16_to_cpu()
2022-08-31 10:06 ` David Laight
@ 2022-08-31 10:27 ` Andy Shevchenko
0 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2022-08-31 10:27 UTC (permalink / raw)
To: David Laight
Cc: 'Mika Westerberg',
Hans de Goede, linux-acpi, linux-kernel, Rafael J. Wysocki,
Len Brown, Andy Shevchenko
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?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-08-31 10:28 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-30 17:11 [PATCH v2 1/3] ACPI: PMIC: Use sizeof() instead of hard coded value Andy Shevchenko
2022-08-30 17:11 ` [PATCH v2 2/3] ACPI: PMIC: Replace open coded be16_to_cpu() Andy Shevchenko
2022-08-31 5:43 ` Mika Westerberg
2022-08-31 9:34 ` Andy Shevchenko
2022-08-31 9:37 ` Hans de Goede
2022-08-31 9:48 ` Mika Westerberg
2022-08-31 10:06 ` David Laight
2022-08-31 10:27 ` Andy Shevchenko
2022-08-30 17:11 ` [PATCH v2 3/3] ACPI: PMIC: Convert pr_*() to dev_*() printing macros Andy Shevchenko
2022-08-31 5:44 ` Mika Westerberg
2022-08-31 5:37 ` [PATCH v2 1/3] ACPI: PMIC: Use sizeof() instead of hard coded value Mika Westerberg
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.