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