Message-ID: <073c2dce-bd03-9dfe-539f-203b6fb7eeaf@redhat.com>
Date:   Thu, 1 Sep 2022 11:02:11 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Len Brown <lenb@...nel.org>, Andy Shevchenko <andy@...nel.org>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>
Subject: Re: [PATCH v3 2/3] ACPI: PMIC: Replace open coded be16_to_cpu()

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@...ux.intel.com>
>>> Reviewed-by: Mika Westerberg <mika.westerberg@...ux.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

Powered by blists - more mailing lists