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

[Pablo Mazzini]

OK, I will explain it in a comment in v3, including the limitation.

On Mon, Nov 2, 2026 at 2:43 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On 11/2/26 02:25, Andreas Kemnade wrote:
> > On Sun, 1 Nov 2026 19:35:16 -0800
> > Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxxxx>
> >>> ---
> >>>    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
>