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

[Andreas Kemnade <andreas@kemnade.info>]

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).

Regards,
Andreas