Re: [PATCH v2 5/7] hwmon: sy7636a: read the temperature only while the rails are on

[Guenter Roeck <linux@roeck-us.net>]

On 11/2/26 02:25, Andreas Kemnade wrote:
> On Sun, 1 Nov 2026 19:35:16 -0800
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On 11/1/26 14:31, Pablo Mazzini wrote:
>>> The driver enables VCOM at probe, which keeps the panel's high voltage
>>> rails on for good, and gets a dummy regulator if it probes before the
>>> regulator exists. Read the temperature only while the rails are on, when
>>> the chip samples it, and return -ENODATA otherwise. Read power good with
>>> it in one transfer, so the value can not be from before the first sample
>>> or after the rails went off. Register it under the chip name, as the
>>> TPS65185 and FP9931 drivers do.
>>>
>>> Signed-off-by: Pablo Mazzini <pmazzini@gmail.com>
>>> ---
>>>    drivers/hwmon/sy7636a-hwmon.c | 25 ++++++++++++++-----------
>>>    1 file changed, 14 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
>>> index f01f368d68e5..475aff997c2d 100644
>>> --- a/drivers/hwmon/sy7636a-hwmon.c
>>> +++ b/drivers/hwmon/sy7636a-hwmon.c
>>> @@ -14,7 +14,6 @@
>>>    #include <linux/module.h>
>>>    #include <linux/platform_device.h>
>>>    #include <linux/regmap.h>
>>> -#include <linux/regulator/machine.h
>>>    
>>>    #include <linux/mfd/sy7636a.h>
>>>    
>>> @@ -22,14 +21,22 @@ static int sy7636a_read(struct device *dev, enum hwmon_sensor_types type,
>>>    			u32 attr, int channel, long *temp)
>>>    {
>>>    	struct regmap *regmap = dev_get_drvdata(dev);
>>> -	int ret, reg_val;
>>> -
>>> -	ret = regmap_read(regmap,
>>> -			  SY7636A_REG_TERMISTOR_READOUT, &reg_val);
>>> +	u8 buf[2];
>>> +	int ret;
>>> +
>>> +	/*
>>> +	 * Read power good and the thermistor in one transfer: the thermistor is
>>> +	 * only sampled while the rails are on, and power good comes after the
>>> +	 * first sample
>>> +	 */
>>> +	ret = regmap_bulk_read(regmap, SY7636A_REG_FAULT_FLAG, buf, sizeof(buf));
>>>    	if (ret)
>>>    		return ret;
>>>    
>>> -	*temp = (s8)reg_val * 1000;
>>> +	if (!(buf[0] & SY7636A_FAULT_FLAG_PG))
>>> +		return -ENODATA;
>>> +
>>
>> I had a look at the datasheet. It does not say anything about the
>> temperature not being measured when PG is not set.
>>
> That is what I read from there, too. But the PG bit will only
> be set if the On/Off bit is also set.
> 
>> The datasheet does suggest that temperatures are only measured
>> if bit 7 of the operation mode register is set, but that is
>> different from the "power good" bit in the fault register.
>>
> I can confirm from tests that temperature reads zero if regulator
> was not yet powered on.
> 
> I read the datasheet in that way so things happen it that order:
> 
> 1. on bit is set
> 2. ramping up voltages start / temperature sampling starts.
> 3. after some time (sampling time + conversion time (ADC_SPL+ADC_DEL)?),
>     the first sample is ready and the temperature can be read.
> 4. after some more time voltage lines stabilize
> 5. after 1.5 ms PG is asserted
> 
>> So why the association with the PG bit and not with the On/Off bit
>> of the operation mode register ? That warrants some more explanation.
> 
> That should definitively be commented in the code. I have pointed out
> races earlier. This has of course the drawback that it limits the time
> frame in which reads are possible a bit. I doubt it is worth dealing with
> synchronisation code to use that on/off bit (although posssible).
> 
As mentioned in the other mail, downside is that the temperature will not
be reported if EN=1 and PG=0 (for whatever reason), even though
the temperature may be available. At the very least the reason for
accepting this limitation will need to be explained in the driver.

Guenter