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