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

[Hans de Goede <hdegoede@redhat.com>]

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