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

[Guenter Roeck]

On 11/2/26 00:09, Pablo Mazzini wrote:
> Thanks for looking at it.
>
> Reading the temperature races with the rails being switched on and
> off, as Andreas pointed out on v1.

This should be explained as comment, with source (I don't see that
in the datasheet).

> PG only goes high once all rails are up, after the first measurement.
> It is also in register 0x07, next to the thermistor readout, so one
> short transfer reads both.

That is not really an argument. With the current implementation,
static state with EN=1 and PG=0 will report no temperatures even though
temperatures may be available.

Guenter

> Pablo
>
> On Mon, Nov 2, 2026 at 3:35 AM 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.
> >
> > 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.
> >
> > 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.
> >
> > Thanks,
> > Guenter
> >    > +   *temp = (s8)buf[1] * 1000;
> > >        return 0;
> > >    }
> > > @@ -72,12 +79,8 @@ static int sy7636a_sensor_probe(struct platform_device *pdev)
> > >        if (!regmap)
> > >                return -EPROBE_DEFER;
> > >
> > > -     err = devm_regulator_get_enable(&pdev->dev, "vcom");
> > > -     if (err)
> > > -             return err;
> > > -
> > >        hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
> > > -                                                      "sy7636a_temperature", regmap,
> > > +                                                      "sy7636a", regmap,
> > >                                                         &sy7636a_chip_info, NULL);
> > >
> > >        if (IS_ERR(hwmon_dev)) {