[PATCH 0/2] hmwon: (sy7636a) fix regulator handling

[PATCH 0/2] hmwon: (sy7636a) fix regulator handling

[Andreas Kemnade]

The temperature sensor in these devices in only usable if its
regulators are turned on.
To have that properly working fix a race during probe which leads to
dummy regulator assigned instead. That fix looks questionable,
therefore mark it as RFC. Having the regulators enabled increases
power consumption a lot, so rather turn them on only when needed.
The usual usage pattern would be to enable regulators, read temperature to
determine how to configure waveforms for the epaper display,
initiate the refresh and turning the regulators off.

Andreas Kemnade (2):
  hwmon: (sy7636a) fix races during probe of mfd subdevices
  hwmon: (sy7636a) enable regulator only if needed

 drivers/hwmon/sy7636a-hwmon.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

-- 
2.47.3

[PATCH RFC 1/2] hwmon: (sy7636a) fix races during probe of mfd subdevices

[Andreas Kemnade]

If regulator subdevice is not ready early enough, devm_regulator_get will
get a dummy regulator device, not the real one needed from the regulator
subdevice, so defer probe in such case.
devm_regulator_get_optional returns an error in that case.

That fixes things, but looks odd, therefore RFC.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/hwmon/sy7636a-hwmon.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
index ed110884786b..e83d0e50c735 100644
--- a/drivers/hwmon/sy7636a-hwmon.c
+++ b/drivers/hwmon/sy7636a-hwmon.c
@@ -73,9 +73,9 @@ static int sy7636a_sensor_probe(struct platform_device *pdev)
 	if (!regmap)
 		return -EPROBE_DEFER;
 
-	regulator = devm_regulator_get(&pdev->dev, "vcom");
-	if (IS_ERR(regulator))
-		return PTR_ERR(regulator);
+	regulator = devm_regulator_get_optional(&pdev->dev, "vcom");
+	if (IS_ERR_OR_NULL(regulator))
+		return -EPROBE_DEFER;
 
 	err = regulator_enable(regulator);
 	if (err)
-- 
2.47.3

Re: [PATCH RFC 1/2] hwmon: (sy7636a) fix races during probe of mfd subdevices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]

On Sat, Sep 20, 2025 at 01:43:10PM +0200, Andreas Kemnade wrote:
> If regulator subdevice is not ready early enough, devm_regulator_get will
> get a dummy regulator device, not the real one needed from the regulator
> subdevice, so defer probe in such case.
> devm_regulator_get_optional returns an error in that case.
> 
> That fixes things, but looks odd, therefore RFC.

No, this is buggy and broken.  You should only use _optional for
supplies that are optional, the clue is in the name.  You need to fix
whatever is causing the device to be instantiated to ensure that the
regulators for the device are described before it tries to instnatiate
the device.  Normally this is all part of a unified firmware
description.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC 1/2] hwmon: (sy7636a) fix races during probe of mfd subdevices

[Andreas Kemnade]

On Sat, 20 Sep 2025 20:58:07 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Sat, Sep 20, 2025 at 01:43:10PM +0200, Andreas Kemnade wrote:
> > If regulator subdevice is not ready early enough, devm_regulator_get will
> > get a dummy regulator device, not the real one needed from the regulator
> > subdevice, so defer probe in such case.
> > devm_regulator_get_optional returns an error in that case.
> > 
> > That fixes things, but looks odd, therefore RFC.  
> 
> No, this is buggy and broken.  You should only use _optional for
> supplies that are optional, the clue is in the name.  You need to fix
> whatever is causing the device to be instantiated to ensure that the
> regulators for the device are described before it tries to instnatiate
> the device.  Normally this is all part of a unified firmware
> description.

Just for learning, yes, it is an abuse of the _optional for non-optional
things, so a dirty hack which should not go in, therefore RFC. But what
happens more than having the hwmon device endlessly deferred at worst?

The wanted regulator is the one defined in sy7636a-regulator.c. So it
is all an issue internal to the sy7636a.
  
Both subdevices are instantiated via drivers/simple-mfd-i2c.c.
I see several other solutions:
a) call device_is_bound() on every other children of dev->parent, if not
bound defer.
b) do not care about the regulator api at all, just check whether
   the corresponding bit is set before reading temperature, return
   -ENODATA if not, some mutex is probably needed.
c) do not care about the regulator api at all, just set the
   corresponding bit (together with some mutex locking and counting).
d) copy the of_node pointer from the parent, add a regulator phandle property
   to the node pointing to the regulator in the node itself.
   That sounds like your idea but is against the current dt binding for
   this device and afaik it is uncommon to have mfd-internal things wired
   up this way

e) something clean, simple I miss

Regards,
Andreas

Re: [PATCH RFC 1/2] hwmon: (sy7636a) fix races during probe of mfd subdevices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2168 bytes --]

On Sat, Sep 20, 2025 at 11:33:07PM +0200, Andreas Kemnade wrote:

> Just for learning, yes, it is an abuse of the _optional for non-optional
> things, so a dirty hack which should not go in, therefore RFC. But what
> happens more than having the hwmon device endlessly deferred at worst?

There's also the fact that this API is so frequently abused for bad and
broken reasons that I regularly audit users and try to fix them, I'd
rather not see any new users that don't have a really strong reason to
use it.

> The wanted regulator is the one defined in sy7636a-regulator.c. So it
> is all an issue internal to the sy7636a.

> Both subdevices are instantiated via drivers/simple-mfd-i2c.c.
> I see several other solutions:
> a) call device_is_bound() on every other children of dev->parent, if not
> bound defer.
> b) do not care about the regulator api at all, just check whether
>    the corresponding bit is set before reading temperature, return
>    -ENODATA if not, some mutex is probably needed.
> c) do not care about the regulator api at all, just set the
>    corresponding bit (together with some mutex locking and counting).

I assume this is using the regulator API because someone might use an
external regulator in a system design for some reason (better quality,
power efficiency or a shared reference between multiple devices I
guess?), or because the supply might also be used by external devices?

> d) copy the of_node pointer from the parent, add a regulator phandle property
>    to the node pointing to the regulator in the node itself.
>    That sounds like your idea but is against the current dt binding for
>    this device and afaik it is uncommon to have mfd-internal things wired
>    up this way
> 
> e) something clean, simple I miss

The idea is that the relationship between the devices should be
registered before the devices, that's how the regulator knows to defer.
We used to have an API for doing this for board files which might fit
here, but it got removed since nobody wants board files any more.  If
you're allocating the devices dynamically that's annoying to implement
though...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC 1/2] hwmon: (sy7636a) fix races during probe of mfd subdevices

[Andreas Kemnade]

On Sat, 20 Sep 2025 23:18:59 +0100
Mark Brown <broonie@kernel.org> wrote:

> > The wanted regulator is the one defined in sy7636a-regulator.c. So it
> > is all an issue internal to the sy7636a.  
> 
> > Both subdevices are instantiated via drivers/simple-mfd-i2c.c.
> > I see several other solutions:
> > a) call device_is_bound() on every other children of dev->parent, if not
> > bound defer.
> > b) do not care about the regulator api at all, just check whether
> >    the corresponding bit is set before reading temperature, return
> >    -ENODATA if not, some mutex is probably needed.
> > c) do not care about the regulator api at all, just set the
> >    corresponding bit (together with some mutex locking and counting).  
> 
> I assume this is using the regulator API because someone might use an
> external regulator in a system design for some reason (better quality,
> power efficiency or a shared reference between multiple devices I
> guess?), or because the supply might also be used by external devices?

So just what is behind enabling the regulator:
a bit controlling two boost convertes, two ldos behind tham,
and the name-giving vcom regulator.
Enabling this bit is also required to have the temperature register
working, althogh none of the regulators is used for measurenig the 
temperature.
All these regulator are designed for powering EPDs. So the
regulator API is needed. The only question is whether it might be bypassed
for internal usage (which of course needs some locking, etc.)

Regards,
Andreas

Re: [PATCH RFC 1/2] hwmon: (sy7636a) fix races during probe of mfd subdevices

[Andreas Kemnade]

On Sat, 20 Sep 2025 23:18:59 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Sat, Sep 20, 2025 at 11:33:07PM +0200, Andreas Kemnade wrote:
> 
> > Just for learning, yes, it is an abuse of the _optional for non-optional
> > things, so a dirty hack which should not go in, therefore RFC. But what
> > happens more than having the hwmon device endlessly deferred at worst?  
> 
> There's also the fact that this API is so frequently abused for bad and
> broken reasons that I regularly audit users and try to fix them, I'd
> rather not see any new users that don't have a really strong reason to
> use it.
> 
> > The wanted regulator is the one defined in sy7636a-regulator.c. So it
> > is all an issue internal to the sy7636a.  
> 
> > Both subdevices are instantiated via drivers/simple-mfd-i2c.c.
> > I see several other solutions:
> > a) call device_is_bound() on every other children of dev->parent, if not
> > bound defer.
> > b) do not care about the regulator api at all, just check whether
> >    the corresponding bit is set before reading temperature, return
> >    -ENODATA if not, some mutex is probably needed.
> > c) do not care about the regulator api at all, just set the
> >    corresponding bit (together with some mutex locking and counting).  
> 
> I assume this is using the regulator API because someone might use an
> external regulator in a system design for some reason (better quality,
> power efficiency or a shared reference between multiple devices I
> guess?), or because the supply might also be used by external devices?
> 
> > d) copy the of_node pointer from the parent, add a regulator phandle property
> >    to the node pointing to the regulator in the node itself.
> >    That sounds like your idea but is against the current dt binding for
> >    this device and afaik it is uncommon to have mfd-internal things wired
> >    up this way
> > 
> > e) something clean, simple I miss  
> 
> The idea is that the relationship between the devices should be
> registered before the devices, that's how the regulator knows to defer.
> We used to have an API for doing this for board files which might fit
> here, but it got removed since nobody wants board files any more.  If
> you're allocating the devices dynamically that's annoying to implement
> though...

looking a bit around:
max5970-regulator.c has hwmon integrated and no extra device. That would
simplify things. Although it does not report temperature. Some
touchscreens have temperature via hwmon, some others have temperature
via iio, directly in one device without mfd. Maybe that is also
the better way here?

Regards,
Andreas

Re: [PATCH RFC 1/2] hwmon: (sy7636a) fix races during probe of mfd subdevices

[Guenter Roeck]

On 9/24/25 00:00, Andreas Kemnade wrote:
> On Sat, 20 Sep 2025 23:18:59 +0100
> Mark Brown <broonie@kernel.org> wrote:
> 
>> On Sat, Sep 20, 2025 at 11:33:07PM +0200, Andreas Kemnade wrote:
>>
>>> Just for learning, yes, it is an abuse of the _optional for non-optional
>>> things, so a dirty hack which should not go in, therefore RFC. But what
>>> happens more than having the hwmon device endlessly deferred at worst?
>>
>> There's also the fact that this API is so frequently abused for bad and
>> broken reasons that I regularly audit users and try to fix them, I'd
>> rather not see any new users that don't have a really strong reason to
>> use it.
>>
>>> The wanted regulator is the one defined in sy7636a-regulator.c. So it
>>> is all an issue internal to the sy7636a.
>>
>>> Both subdevices are instantiated via drivers/simple-mfd-i2c.c.
>>> I see several other solutions:
>>> a) call device_is_bound() on every other children of dev->parent, if not
>>> bound defer.
>>> b) do not care about the regulator api at all, just check whether
>>>     the corresponding bit is set before reading temperature, return
>>>     -ENODATA if not, some mutex is probably needed.
>>> c) do not care about the regulator api at all, just set the
>>>     corresponding bit (together with some mutex locking and counting).
>>
>> I assume this is using the regulator API because someone might use an
>> external regulator in a system design for some reason (better quality,
>> power efficiency or a shared reference between multiple devices I
>> guess?), or because the supply might also be used by external devices?
>>
>>> d) copy the of_node pointer from the parent, add a regulator phandle property
>>>     to the node pointing to the regulator in the node itself.
>>>     That sounds like your idea but is against the current dt binding for
>>>     this device and afaik it is uncommon to have mfd-internal things wired
>>>     up this way
>>>
>>> e) something clean, simple I miss
>>
>> The idea is that the relationship between the devices should be
>> registered before the devices, that's how the regulator knows to defer.
>> We used to have an API for doing this for board files which might fit
>> here, but it got removed since nobody wants board files any more.  If
>> you're allocating the devices dynamically that's annoying to implement
>> though...
> 
> looking a bit around:
> max5970-regulator.c has hwmon integrated and no extra device. That would
> simplify things. Although it does not report temperature. Some
> touchscreens have temperature via hwmon, some others have temperature
> via iio, directly in one device without mfd. Maybe that is also
> the better way here?
> 

Touchscreens reporting temperature via iio is in general the wrong thing to do.
Touchscreens report the temperature for monitoring reasons, after all.
But then, sure, if you insist. I am getting tired of arguing.

FWIW, the proper implementation would probably have been to implement
the hwmon device as auxiliary device.

Guenter

Re: [PATCH RFC 1/2] hwmon: (sy7636a) fix races during probe of mfd subdevices

[Andreas Kemnade]

On Wed, 24 Sep 2025 00:17:48 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 9/24/25 00:00, Andreas Kemnade wrote:
> > On Sat, 20 Sep 2025 23:18:59 +0100
> > Mark Brown <broonie@kernel.org> wrote:
> >   
> >> On Sat, Sep 20, 2025 at 11:33:07PM +0200, Andreas Kemnade wrote:
> >>  
> >>> Just for learning, yes, it is an abuse of the _optional for non-optional
> >>> things, so a dirty hack which should not go in, therefore RFC. But what
> >>> happens more than having the hwmon device endlessly deferred at worst?  
> >>
> >> There's also the fact that this API is so frequently abused for bad and
> >> broken reasons that I regularly audit users and try to fix them, I'd
> >> rather not see any new users that don't have a really strong reason to
> >> use it.
> >>  
> >>> The wanted regulator is the one defined in sy7636a-regulator.c. So it
> >>> is all an issue internal to the sy7636a.  
> >>  
> >>> Both subdevices are instantiated via drivers/simple-mfd-i2c.c.
> >>> I see several other solutions:
> >>> a) call device_is_bound() on every other children of dev->parent, if not
> >>> bound defer.
> >>> b) do not care about the regulator api at all, just check whether
> >>>     the corresponding bit is set before reading temperature, return
> >>>     -ENODATA if not, some mutex is probably needed.
> >>> c) do not care about the regulator api at all, just set the
> >>>     corresponding bit (together with some mutex locking and counting).  
> >>
> >> I assume this is using the regulator API because someone might use an
> >> external regulator in a system design for some reason (better quality,
> >> power efficiency or a shared reference between multiple devices I
> >> guess?), or because the supply might also be used by external devices?
> >>  
> >>> d) copy the of_node pointer from the parent, add a regulator phandle property
> >>>     to the node pointing to the regulator in the node itself.
> >>>     That sounds like your idea but is against the current dt binding for
> >>>     this device and afaik it is uncommon to have mfd-internal things wired
> >>>     up this way
> >>>
> >>> e) something clean, simple I miss  
> >>
> >> The idea is that the relationship between the devices should be
> >> registered before the devices, that's how the regulator knows to defer.
> >> We used to have an API for doing this for board files which might fit
> >> here, but it got removed since nobody wants board files any more.  If
> >> you're allocating the devices dynamically that's annoying to implement
> >> though...  
> > 
> > looking a bit around:
> > max5970-regulator.c has hwmon integrated and no extra device. That would
> > simplify things. Although it does not report temperature. Some
> > touchscreens have temperature via hwmon, some others have temperature
> > via iio, directly in one device without mfd. Maybe that is also
> > the better way here?
> >   
> 
> Touchscreens reporting temperature via iio is in general the wrong thing to do.
> Touchscreens report the temperature for monitoring reasons, after all.
> But then, sure, if you insist. I am getting tired of arguing.
> 
I apparently did not make clear what my question refers to. It was more about separate
hwmon device + mfd vs. integrating everything into the regulator driver.

But since you brought up the topic hwmon vs. iio for temperature. I do not have
a strong opinion here as long as I can somehow live with it. Nothing I want to
fight for. One sensor I use for measuring room temperature is hwmon, another
one is iio. So it is all not that consistent.

But what is the hwmon equivalent for
devm_fwnode_iio_channel_get_by_name() + iio_read_channel_processed()?

I wonder whether I really need a thermal zone. It adds stuff not needed here,
trip points and polling. 

Documentation/hwmon/sy7636a-hwmon.rst seems to be wrong. It is not
SoC-on-die temperature, but temperature from an external NTC. And
that is typically used to tune the EPD refresh to the temperature.
I have a sy7636a connected to a broken display causing excessive current
consumption and heat generation. I looked at it with an IR camera.

Regards,
Andreas

Re: [PATCH RFC 1/2] hwmon: (sy7636a) fix races during probe of mfd subdevices

[Guenter Roeck]

On 9/24/25 10:53, Andreas Kemnade wrote:
> On Wed, 24 Sep 2025 00:17:48 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On 9/24/25 00:00, Andreas Kemnade wrote:
>>> On Sat, 20 Sep 2025 23:18:59 +0100
>>> Mark Brown <broonie@kernel.org> wrote:
>>>    
>>>> On Sat, Sep 20, 2025 at 11:33:07PM +0200, Andreas Kemnade wrote:
>>>>   
>>>>> Just for learning, yes, it is an abuse of the _optional for non-optional
>>>>> things, so a dirty hack which should not go in, therefore RFC. But what
>>>>> happens more than having the hwmon device endlessly deferred at worst?
>>>>
>>>> There's also the fact that this API is so frequently abused for bad and
>>>> broken reasons that I regularly audit users and try to fix them, I'd
>>>> rather not see any new users that don't have a really strong reason to
>>>> use it.
>>>>   
>>>>> The wanted regulator is the one defined in sy7636a-regulator.c. So it
>>>>> is all an issue internal to the sy7636a.
>>>>   
>>>>> Both subdevices are instantiated via drivers/simple-mfd-i2c.c.
>>>>> I see several other solutions:
>>>>> a) call device_is_bound() on every other children of dev->parent, if not
>>>>> bound defer.
>>>>> b) do not care about the regulator api at all, just check whether
>>>>>      the corresponding bit is set before reading temperature, return
>>>>>      -ENODATA if not, some mutex is probably needed.
>>>>> c) do not care about the regulator api at all, just set the
>>>>>      corresponding bit (together with some mutex locking and counting).
>>>>
>>>> I assume this is using the regulator API because someone might use an
>>>> external regulator in a system design for some reason (better quality,
>>>> power efficiency or a shared reference between multiple devices I
>>>> guess?), or because the supply might also be used by external devices?
>>>>   
>>>>> d) copy the of_node pointer from the parent, add a regulator phandle property
>>>>>      to the node pointing to the regulator in the node itself.
>>>>>      That sounds like your idea but is against the current dt binding for
>>>>>      this device and afaik it is uncommon to have mfd-internal things wired
>>>>>      up this way
>>>>>
>>>>> e) something clean, simple I miss
>>>>
>>>> The idea is that the relationship between the devices should be
>>>> registered before the devices, that's how the regulator knows to defer.
>>>> We used to have an API for doing this for board files which might fit
>>>> here, but it got removed since nobody wants board files any more.  If
>>>> you're allocating the devices dynamically that's annoying to implement
>>>> though...
>>>
>>> looking a bit around:
>>> max5970-regulator.c has hwmon integrated and no extra device. That would
>>> simplify things. Although it does not report temperature. Some
>>> touchscreens have temperature via hwmon, some others have temperature
>>> via iio, directly in one device without mfd. Maybe that is also
>>> the better way here?
>>>    
>>
>> Touchscreens reporting temperature via iio is in general the wrong thing to do.
>> Touchscreens report the temperature for monitoring reasons, after all.
>> But then, sure, if you insist. I am getting tired of arguing.
>>
> I apparently did not make clear what my question refers to. It was more about separate
> hwmon device + mfd vs. integrating everything into the regulator driver.
> 

What I keep failing to understand is why people keep avoiding the potential of
implementing auxiliary device drivers, since that would be the perfect solution
and match the intended use case for auxiliary devices.

> But since you brought up the topic hwmon vs. iio for temperature. I do not have
> a strong opinion here as long as I can somehow live with it. Nothing I want to
> fight for. One sensor I use for measuring room temperature is hwmon, another
> one is iio. So it is all not that consistent.
> 

That doesn't mean what exists is consistent or even makes sense. Some driver support
for chips intended for reporting the environment or chip temperature are pushed into iio.
I have no idea why that is the case. Yes, that results in odd situations like yours,
but there is nothing I can do about it. I can only guess that _someone_ is pushing for
submitting drivers into IIO instead of hwmon, but that is just a wild guess. You would
have to ask the driver authors and/or IIO maintainers for reasons. I am copying the IIO
mailing list for feedback.

Also, again, I am tired of arguing, so I typically don't even comment anymore (if I even
notice) unless explicitly asked.

> But what is the hwmon equivalent for
> devm_fwnode_iio_channel_get_by_name() + iio_read_channel_processed()?
> 

Assuming you refer to the exported functions for in-kernel use, so far no one has
expressed a need for it. The best solution would probably be a hwmon->iio bridge,
or equivalent functions could be implemented and exported.

> I wonder whether I really need a thermal zone. It adds stuff not needed here,
> trip points and polling.
> 

The driver _registers_ a thermal zone, but that doesn't mean it has to be configured.
If one is configured (typically via devicetree), it is obviously needed.

> Documentation/hwmon/sy7636a-hwmon.rst seems to be wrong. It is not
> SoC-on-die temperature, but temperature from an external NTC. And
> that is typically used to tune the EPD refresh to the temperature.

Please feel free to submit a patch correcting the documentation.

Guenter

Re: [PATCH RFC 1/2] hwmon: (sy7636a) fix races during probe of mfd subdevices

[Jonathan Cameron]

On Wed, 24 Sep 2025 12:16:14 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 9/24/25 10:53, Andreas Kemnade wrote:
> > On Wed, 24 Sep 2025 00:17:48 -0700
> > Guenter Roeck <linux@roeck-us.net> wrote:
> >   
> >> On 9/24/25 00:00, Andreas Kemnade wrote:  
> >>> On Sat, 20 Sep 2025 23:18:59 +0100
> >>> Mark Brown <broonie@kernel.org> wrote:
> >>>      
> >>>> On Sat, Sep 20, 2025 at 11:33:07PM +0200, Andreas Kemnade wrote:
> >>>>     
> >>>>> Just for learning, yes, it is an abuse of the _optional for non-optional
> >>>>> things, so a dirty hack which should not go in, therefore RFC. But what
> >>>>> happens more than having the hwmon device endlessly deferred at worst?  
> >>>>
> >>>> There's also the fact that this API is so frequently abused for bad and
> >>>> broken reasons that I regularly audit users and try to fix them, I'd
> >>>> rather not see any new users that don't have a really strong reason to
> >>>> use it.
> >>>>     
> >>>>> The wanted regulator is the one defined in sy7636a-regulator.c. So it
> >>>>> is all an issue internal to the sy7636a.  
> >>>>     
> >>>>> Both subdevices are instantiated via drivers/simple-mfd-i2c.c.
> >>>>> I see several other solutions:
> >>>>> a) call device_is_bound() on every other children of dev->parent, if not
> >>>>> bound defer.
> >>>>> b) do not care about the regulator api at all, just check whether
> >>>>>      the corresponding bit is set before reading temperature, return
> >>>>>      -ENODATA if not, some mutex is probably needed.
> >>>>> c) do not care about the regulator api at all, just set the
> >>>>>      corresponding bit (together with some mutex locking and counting).  
> >>>>
> >>>> I assume this is using the regulator API because someone might use an
> >>>> external regulator in a system design for some reason (better quality,
> >>>> power efficiency or a shared reference between multiple devices I
> >>>> guess?), or because the supply might also be used by external devices?
> >>>>     
> >>>>> d) copy the of_node pointer from the parent, add a regulator phandle property
> >>>>>      to the node pointing to the regulator in the node itself.
> >>>>>      That sounds like your idea but is against the current dt binding for
> >>>>>      this device and afaik it is uncommon to have mfd-internal things wired
> >>>>>      up this way
> >>>>>
> >>>>> e) something clean, simple I miss  
> >>>>
> >>>> The idea is that the relationship between the devices should be
> >>>> registered before the devices, that's how the regulator knows to defer.
> >>>> We used to have an API for doing this for board files which might fit
> >>>> here, but it got removed since nobody wants board files any more.  If
> >>>> you're allocating the devices dynamically that's annoying to implement
> >>>> though...  
> >>>
> >>> looking a bit around:
> >>> max5970-regulator.c has hwmon integrated and no extra device. That would
> >>> simplify things. Although it does not report temperature. Some
> >>> touchscreens have temperature via hwmon, some others have temperature
> >>> via iio, directly in one device without mfd. Maybe that is also
> >>> the better way here?
> >>>      
> >>
> >> Touchscreens reporting temperature via iio is in general the wrong thing to do.
> >> Touchscreens report the temperature for monitoring reasons, after all.
> >> But then, sure, if you insist. I am getting tired of arguing.
> >>  
> > I apparently did not make clear what my question refers to. It was more about separate
> > hwmon device + mfd vs. integrating everything into the regulator driver.
> >   
> 
> What I keep failing to understand is why people keep avoiding the potential of
> implementing auxiliary device drivers, since that would be the perfect solution
> and match the intended use case for auxiliary devices.
> 
> > But since you brought up the topic hwmon vs. iio for temperature. I do not have
> > a strong opinion here as long as I can somehow live with it. Nothing I want to
> > fight for. One sensor I use for measuring room temperature is hwmon, another
> > one is iio. So it is all not that consistent.
> >   
> 
> That doesn't mean what exists is consistent or even makes sense. Some driver support
> for chips intended for reporting the environment or chip temperature are pushed into iio.

There might be some that have slipped through but when it's monitoring specific chip
as opposed to part of of a SoC ADC where some channels are for monitoring and others
very much not (e.g. the touchscreen ADC channels), I do direct people to provide
reasons and +CC hwmon. There are a few odd corner cases where we had a driver for
a mems device or similar that doesn't fit in hwmon come along and there is a
variant of that silicon that only has the temp part enabled.

Without specific part numbers I don't know why it happened here.

We get a few drivers every year where I think it's just familiarity with IIO that
drove the initial submission and for those we push them to submit a hwmon driver instead
and I +CC the hwmon list on that suggestion.  Guenter frequently eyeballs the driver
and points that another driver can be trivially modified to support it.

Mind you I may well miss some :(


> I have no idea why that is the case. Yes, that results in odd situations like yours,
> but there is nothing I can do about it. I can only guess that _someone_ is pushing for
> submitting drivers into IIO instead of hwmon, but that is just a wild guess. You would
> have to ask the driver authors and/or IIO maintainers for reasons. I am copying the IIO
> mailing list for feedback.
> 
> Also, again, I am tired of arguing, so I typically don't even comment anymore (if I even
> notice) unless explicitly asked.
> 
> > But what is the hwmon equivalent for
> > devm_fwnode_iio_channel_get_by_name() + iio_read_channel_processed()?
> >   
> 
> Assuming you refer to the exported functions for in-kernel use, so far no one has
> expressed a need for it. The best solution would probably be a hwmon->iio bridge,
> or equivalent functions could be implemented and exported.
> 
> > I wonder whether I really need a thermal zone. It adds stuff not needed here,
> > trip points and polling.
> >   
> 
> The driver _registers_ a thermal zone, but that doesn't mean it has to be configured.
> If one is configured (typically via devicetree), it is obviously needed.
> 
> > Documentation/hwmon/sy7636a-hwmon.rst seems to be wrong. It is not
> > SoC-on-die temperature, but temperature from an external NTC. And
> > that is typically used to tune the EPD refresh to the temperature.  
> 
> Please feel free to submit a patch correcting the documentation.
> 
> Guenter
> 

Re: [PATCH RFC 1/2] hwmon: (sy7636a) fix races during probe of mfd subdevices

[Andreas Kemnade]

On Sat, 27 Sep 2025 19:39:45 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed, 24 Sep 2025 12:16:14 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > On 9/24/25 10:53, Andreas Kemnade wrote:  
> > > On Wed, 24 Sep 2025 00:17:48 -0700
> > > Guenter Roeck <linux@roeck-us.net> wrote:
> > >     
> > >> On 9/24/25 00:00, Andreas Kemnade wrote:    
> > >>> On Sat, 20 Sep 2025 23:18:59 +0100
> > >>> Mark Brown <broonie@kernel.org> wrote:
> > >>>        
> > >>>> On Sat, Sep 20, 2025 at 11:33:07PM +0200, Andreas Kemnade wrote:
> > >>>>       
> > >>>>> Just for learning, yes, it is an abuse of the _optional for non-optional
> > >>>>> things, so a dirty hack which should not go in, therefore RFC. But what
> > >>>>> happens more than having the hwmon device endlessly deferred at worst?    
> > >>>>
> > >>>> There's also the fact that this API is so frequently abused for bad and
> > >>>> broken reasons that I regularly audit users and try to fix them, I'd
> > >>>> rather not see any new users that don't have a really strong reason to
> > >>>> use it.
> > >>>>       
> > >>>>> The wanted regulator is the one defined in sy7636a-regulator.c. So it
> > >>>>> is all an issue internal to the sy7636a.    
> > >>>>       
> > >>>>> Both subdevices are instantiated via drivers/simple-mfd-i2c.c.
> > >>>>> I see several other solutions:
> > >>>>> a) call device_is_bound() on every other children of dev->parent, if not
> > >>>>> bound defer.
> > >>>>> b) do not care about the regulator api at all, just check whether
> > >>>>>      the corresponding bit is set before reading temperature, return
> > >>>>>      -ENODATA if not, some mutex is probably needed.
> > >>>>> c) do not care about the regulator api at all, just set the
> > >>>>>      corresponding bit (together with some mutex locking and counting).    
> > >>>>
> > >>>> I assume this is using the regulator API because someone might use an
> > >>>> external regulator in a system design for some reason (better quality,
> > >>>> power efficiency or a shared reference between multiple devices I
> > >>>> guess?), or because the supply might also be used by external devices?
> > >>>>       
> > >>>>> d) copy the of_node pointer from the parent, add a regulator phandle property
> > >>>>>      to the node pointing to the regulator in the node itself.
> > >>>>>      That sounds like your idea but is against the current dt binding for
> > >>>>>      this device and afaik it is uncommon to have mfd-internal things wired
> > >>>>>      up this way
> > >>>>>
> > >>>>> e) something clean, simple I miss    
> > >>>>
> > >>>> The idea is that the relationship between the devices should be
> > >>>> registered before the devices, that's how the regulator knows to defer.
> > >>>> We used to have an API for doing this for board files which might fit
> > >>>> here, but it got removed since nobody wants board files any more.  If
> > >>>> you're allocating the devices dynamically that's annoying to implement
> > >>>> though...    
> > >>>
> > >>> looking a bit around:
> > >>> max5970-regulator.c has hwmon integrated and no extra device. That would
> > >>> simplify things. Although it does not report temperature. Some
> > >>> touchscreens have temperature via hwmon, some others have temperature
> > >>> via iio, directly in one device without mfd. Maybe that is also
> > >>> the better way here?
> > >>>        
> > >>
> > >> Touchscreens reporting temperature via iio is in general the wrong thing to do.
> > >> Touchscreens report the temperature for monitoring reasons, after all.
> > >> But then, sure, if you insist. I am getting tired of arguing.
> > >>    
> > > I apparently did not make clear what my question refers to. It was more about separate
> > > hwmon device + mfd vs. integrating everything into the regulator driver.
> > >     
> > 
> > What I keep failing to understand is why people keep avoiding the potential of
> > implementing auxiliary device drivers, since that would be the perfect solution
> > and match the intended use case for auxiliary devices.
> >   
> > > But since you brought up the topic hwmon vs. iio for temperature. I do not have
> > > a strong opinion here as long as I can somehow live with it. Nothing I want to
> > > fight for. One sensor I use for measuring room temperature is hwmon, another
> > > one is iio. So it is all not that consistent.
> > >     
> > 
> > That doesn't mean what exists is consistent or even makes sense. Some driver support
> > for chips intended for reporting the environment or chip temperature are pushed into iio.  
> 
> There might be some that have slipped through but when it's monitoring specific chip
> as opposed to part of of a SoC ADC where some channels are for monitoring and others
> very much not (e.g. the touchscreen ADC channels), I do direct people to provide
> reasons and +CC hwmon. There are a few odd corner cases where we had a driver for
> a mems device or similar that doesn't fit in hwmon come along and there is a
> variant of that silicon that only has the temp part enabled.
> 
> Without specific part numbers I don't know why it happened here.
> 
HIH6130 (humidity + temperature via hwmon) vs bme280
(humidity + temperature + pressure via iio).

So maybe the very basic thing: What does "monitor" mean in this context?
Checking the state of the hardware (what temperature) to take
safety/emergency measures or to tune things to work optimally at the given
conditions? 

But when monitoring is about the health of the system... well in
a drone e.g. probably everything which can be iio can be considered part of the
system health and like the temperature, things can be controlled by some actors.

Well as long as I do not need anything specific to one or the other api, I can
live with things. I am a bit curious about the background.

Regards,
Andreas

In-kernel hwmon read: (was: Re: [PATCH RFC 1/2] hwmon: (sy7636a) fix races during probe of mfd subdevices)

[Andreas Kemnade]

On Wed, 24 Sep 2025 12:16:14 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> > But what is the hwmon equivalent for
> > devm_fwnode_iio_channel_get_by_name() + iio_read_channel_processed()?
> >   
> 
> Assuming you refer to the exported functions for in-kernel use, so far no one has
> expressed a need for it. The best solution would probably be a hwmon->iio bridge,
> or equivalent functions could be implemented and exported.

So first analyzing the need of such an interface. I think
there is a need for such interface. The need just gets masked by some hacks
to be able to divert to other interfaces.

There is out-of-tree waiting to be upstreamed:
- rockchip_ebc driver: Display on PineNote, uses iio_read_chanel_processed()
                       in combination with an out-of-tree iio temperature
		       driver
- mxc_epdc_drm driver: Display on Kobo/Tolino ebook readers, uses
                       thermal_zone_get_temp() now.

In tree, there is:
- drivers/gpu/drm/tiny/repaper.c
  in combination with
  Documentation/devicetree/bindings/display/repaper.txt

  Code excerpt:
        if (!device_property_read_string(dev, "pervasive,thermal-zone",
                                         &thermal_zone)) {
                epd->thermal = thermal_zone_get_zone_by_name(thermal_zone);

  and thermal_zone_get_temp() to tune refreshes according to panel temperature.



The example in the binding is:
        display_temp: lm75@48 {
                compatible = "lm75b";
                reg = <0x48>;
                #thermal-sensor-cells = <0>;
        };

        thermal-zones {
                display {
                        polling-delay-passive = <0>;
                        polling-delay = <0>;
                        thermal-sensors = <&display_temp>;
                };
        };

[...]
                    pervasive,thermal-zone = "display";


I would prefer to be able to use e.g. pervasive,temperature-sensor = <&display_temp>;
or maybe <&display_temp 0> if there are multiple sensors in the same chip.
so that dtc will tell me when there is a typo and avoid the thermal
zone layer

So what are the options:
a) provide similar logic like iio/inkern.c for hwmon usage.
b) automatically add iio channels during hwmon registration.

looking at the iio_hwmon bridge we already have, it depends on
iio/inkern.c so for a  hwmon_iio bridge we need a) or b) anyways,
so I think a separate bridge device/driver has no advantages.
And having a devicetree node for the bridge would be bad because
we are then describing linux implementation details instead of
the actual hardware in the devicetree.

Regards,
Andreas

Re: In-kernel hwmon read: (was: Re: [PATCH RFC 1/2] hwmon: (sy7636a) fix races during probe of mfd subdevices)

[Jonathan Cameron]

On Thu, 22 Jan 2026 15:23:06 +0100
Andreas Kemnade <andreas@kemnade.info> wrote:

> On Wed, 24 Sep 2025 12:16:14 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > > But what is the hwmon equivalent for
> > > devm_fwnode_iio_channel_get_by_name() + iio_read_channel_processed()?
> > >     
> > 
> > Assuming you refer to the exported functions for in-kernel use, so far no one has
> > expressed a need for it. The best solution would probably be a hwmon->iio bridge,
> > or equivalent functions could be implemented and exported.  
> 
> So first analyzing the need of such an interface. I think
> there is a need for such interface. The need just gets masked by some hacks
> to be able to divert to other interfaces.
> 
> There is out-of-tree waiting to be upstreamed:
> - rockchip_ebc driver: Display on PineNote, uses iio_read_chanel_processed()
>                        in combination with an out-of-tree iio temperature
> 		       driver
> - mxc_epdc_drm driver: Display on Kobo/Tolino ebook readers, uses
>                        thermal_zone_get_temp() now.
> 
> In tree, there is:
> - drivers/gpu/drm/tiny/repaper.c
>   in combination with
>   Documentation/devicetree/bindings/display/repaper.txt
> 
>   Code excerpt:
>         if (!device_property_read_string(dev, "pervasive,thermal-zone",
>                                          &thermal_zone)) {
>                 epd->thermal = thermal_zone_get_zone_by_name(thermal_zone);
> 
>   and thermal_zone_get_temp() to tune refreshes according to panel temperature.
> 
> 
> 
> The example in the binding is:
>         display_temp: lm75@48 {
>                 compatible = "lm75b";
>                 reg = <0x48>;
>                 #thermal-sensor-cells = <0>;
>         };
> 
>         thermal-zones {
>                 display {
>                         polling-delay-passive = <0>;
>                         polling-delay = <0>;
>                         thermal-sensors = <&display_temp>;
>                 };
>         };
> 
> [...]
>                     pervasive,thermal-zone = "display";
> 
> 
> I would prefer to be able to use e.g. pervasive,temperature-sensor = <&display_temp>;
> or maybe <&display_temp 0> if there are multiple sensors in the same chip.
> so that dtc will tell me when there is a typo and avoid the thermal
> zone layer
> 
> So what are the options:
> a) provide similar logic like iio/inkern.c for hwmon usage.
> b) automatically add iio channels during hwmon registration.
> 
> looking at the iio_hwmon bridge we already have, it depends on
> iio/inkern.c so for a  hwmon_iio bridge we need a) or b) anyways,
> so I think a separate bridge device/driver has no advantages.
> And having a devicetree node for the bridge would be bad because
> we are then describing linux implementation details instead of
> the actual hardware in the devicetree.

That's always been a little controversial for the iio-hwmon
but I had quite a few discussions of the years of how to describe
this and no one ever came up with anything that worked other than
maybe pushing it to configfs and making it a usespace script problem.

Jonathan

> 
> Regards,
> Andreas
> 

Re: In-kernel hwmon read:

[Guenter Roeck]

On 1/22/26 06:23, Andreas Kemnade wrote:
> On Wed, 24 Sep 2025 12:16:14 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>>> But what is the hwmon equivalent for
>>> devm_fwnode_iio_channel_get_by_name() + iio_read_channel_processed()?
>>>    
>>
>> Assuming you refer to the exported functions for in-kernel use, so far no one has
>> expressed a need for it. The best solution would probably be a hwmon->iio bridge,
>> or equivalent functions could be implemented and exported.
> 
> So first analyzing the need of such an interface. I think
> there is a need for such interface. The need just gets masked by some hacks
> to be able to divert to other interfaces.
> 
> There is out-of-tree waiting to be upstreamed:
> - rockchip_ebc driver: Display on PineNote, uses iio_read_chanel_processed()
>                         in combination with an out-of-tree iio temperature
> 		       driver
> - mxc_epdc_drm driver: Display on Kobo/Tolino ebook readers, uses
>                         thermal_zone_get_temp() now.
> 
> In tree, there is:
> - drivers/gpu/drm/tiny/repaper.c
>    in combination with
>    Documentation/devicetree/bindings/display/repaper.txt
> 
>    Code excerpt:
>          if (!device_property_read_string(dev, "pervasive,thermal-zone",
>                                           &thermal_zone)) {
>                  epd->thermal = thermal_zone_get_zone_by_name(thermal_zone);
> 
>    and thermal_zone_get_temp() to tune refreshes according to panel temperature.
> 
> 
> 
> The example in the binding is:
>          display_temp: lm75@48 {
>                  compatible = "lm75b";
>                  reg = <0x48>;
>                  #thermal-sensor-cells = <0>;
>          };
> 
>          thermal-zones {
>                  display {
>                          polling-delay-passive = <0>;
>                          polling-delay = <0>;
>                          thermal-sensors = <&display_temp>;
>                  };
>          };
> 
> [...]
>                      pervasive,thermal-zone = "display";
> 
> 
> I would prefer to be able to use e.g. pervasive,temperature-sensor = <&display_temp>;
> or maybe <&display_temp 0> if there are multiple sensors in the same chip.
> so that dtc will tell me when there is a typo and avoid the thermal
> zone layer
> 
> So what are the options:
> a) provide similar logic like iio/inkern.c for hwmon usage.
> b) automatically add iio channels during hwmon registration.
> 

b) _is_ the hwmon->iio bridge. Add HWMON_C_REGISTER_IIO,
and register as appropriate with iio if it is set. Then add
HWMON_C_REGISTER_IIO to all drivers needing it.

Not sure though how that would look like in devicetree for
in-kernel use.

Guenter

> looking at the iio_hwmon bridge we already have, it depends on
> iio/inkern.c so for a  hwmon_iio bridge we need a) or b) anyways,
> so I think a separate bridge device/driver has no advantages.
> And having a devicetree node for the bridge would be bad because
> we are then describing linux implementation details instead of
> the actual hardware in the devicetree.
> 
> Regards,
> Andreas
> 

Re: [PATCH RFC 1/2] hwmon: (sy7636a) fix races during probe of mfd subdevices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 274 bytes --]

On Wed, Sep 24, 2025 at 09:00:23AM +0200, Andreas Kemnade wrote:

> max5970-regulator.c has hwmon integrated and no extra device. That would
> simplify things. Although it does not report temperature. Some

If that's the only reason why it's a MFD I'm fine with doing that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH RFC 1/2] hwmon: (sy7636a) fix races during probe of mfd subdevices

[Andreas Kemnade]

Hi,

On Wed, 24 Sep 2025 10:44:37 +0200
Mark Brown <broonie@kernel.org> wrote:

> On Wed, Sep 24, 2025 at 09:00:23AM +0200, Andreas Kemnade wrote:
> 
> > max5970-regulator.c has hwmon integrated and no extra device. That would
> > simplify things. Although it does not report temperature. Some  
> 
> If that's the only reason why it's a MFD I'm fine with doing that.

So lets summarize.
Regulator + Temperature without hierarchy = regulator driver with hwmon integrated or
                                            mfd                                  
                                          + regulator driver
                                          + hwmon driver

Regulator + Temperature with some kind of hierarchy =  regulator driver with hwmon as
                                                       aux device or
                                                       regulator driver with hwmon integrated



I am just thinking about upstreaming one of the other epd pmic drivers
I carry around with full power management in mind at the beginning first.
The difference is that temperature part in these chips is more separated,
so the other design patterns are more suitable. So it is not one bit
controlling everything. I am thinking about chips like the JD9930,
TPS65185, or the FP9928. So I get a good picture how things could work. 

Regards,
Andreas

[PATCH 2/2] hwmon: (sy7636a) enable regulator only if needed

[Andreas Kemnade]

Avoid having all the regulators in the SY7636A enabled all the time
to significantly reduce current consumption. In pratical scenarios,
the regulators are only needed when a refresh is done on the epaper
display powered by the SY7636A. This is can save around 10mA which
is much for this kind of devices.
Also fixes the asymmetrical single enable call.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/hwmon/sy7636a-hwmon.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
index e83d0e50c735..0fda69bea3b4 100644
--- a/drivers/hwmon/sy7636a-hwmon.c
+++ b/drivers/hwmon/sy7636a-hwmon.c
@@ -18,14 +18,26 @@
 
 #include <linux/mfd/sy7636a.h>
 
+struct sy7636a_hwmon_data {
+	struct regmap *regmap;
+	struct regulator *regulator;
+};
+
+
 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);
+	struct sy7636a_hwmon_data *drvdata = dev_get_drvdata(dev);
 	int ret, reg_val;
 
-	ret = regmap_read(regmap,
+	ret = regulator_enable(drvdata->regulator);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(drvdata->regmap,
 			  SY7636A_REG_TERMISTOR_READOUT, &reg_val);
+	regulator_disable(drvdata->regulator);
+
 	if (ret)
 		return ret;
 
@@ -66,23 +78,24 @@ static const struct hwmon_chip_info sy7636a_chip_info = {
 static int sy7636a_sensor_probe(struct platform_device *pdev)
 {
 	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
-	struct regulator *regulator;
+	struct sy7636a_hwmon_data *drvdata;
 	struct device *hwmon_dev;
 	int err;
 
 	if (!regmap)
 		return -EPROBE_DEFER;
 
-	regulator = devm_regulator_get_optional(&pdev->dev, "vcom");
-	if (IS_ERR_OR_NULL(regulator))
-		return -EPROBE_DEFER;
+	drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
 
-	err = regulator_enable(regulator);
-	if (err)
-		return err;
+	drvdata->regmap = regmap;
+	drvdata->regulator = devm_regulator_get_optional(&pdev->dev, "vcom");
+	if (IS_ERR_OR_NULL(drvdata->regulator))
+		return -EPROBE_DEFER;
 
 	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
-							 "sy7636a_temperature", regmap,
+							 "sy7636a_temperature", drvdata,
 							 &sy7636a_chip_info, NULL);
 
 	if (IS_ERR(hwmon_dev)) {
@@ -102,5 +115,6 @@ static struct platform_driver sy7636a_sensor_driver = {
 };
 module_platform_driver(sy7636a_sensor_driver);
 
+MODULE_ALIAS("platform:sy7636a-temperature");
 MODULE_DESCRIPTION("SY7636A sensor driver");
 MODULE_LICENSE("GPL");
-- 
2.47.3

Re: [PATCH 2/2] hwmon: (sy7636a) enable regulator only if needed

[Guenter Roeck]

On Sat, Sep 20, 2025 at 01:43:11PM +0200, Andreas Kemnade wrote:
> Avoid having all the regulators in the SY7636A enabled all the time
> to significantly reduce current consumption. In pratical scenarios,
> the regulators are only needed when a refresh is done on the epaper
> display powered by the SY7636A. This is can save around 10mA which
> is much for this kind of devices.
> Also fixes the asymmetrical single enable call.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  drivers/hwmon/sy7636a-hwmon.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
> index e83d0e50c735..0fda69bea3b4 100644
> --- a/drivers/hwmon/sy7636a-hwmon.c
> +++ b/drivers/hwmon/sy7636a-hwmon.c
> @@ -18,14 +18,26 @@
>  
>  #include <linux/mfd/sy7636a.h>
>  
> +struct sy7636a_hwmon_data {
> +	struct regmap *regmap;
> +	struct regulator *regulator;
> +};
> +
> +
>  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);
> +	struct sy7636a_hwmon_data *drvdata = dev_get_drvdata(dev);
>  	int ret, reg_val;
>  
> -	ret = regmap_read(regmap,
> +	ret = regulator_enable(drvdata->regulator);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(drvdata->regmap,
>  			  SY7636A_REG_TERMISTOR_READOUT, &reg_val);

Does that really work without delay ? Usually it takes some time for a chip
to return useful data after its power supply has been enabled.

Guenter

> +	regulator_disable(drvdata->regulator);
> +
>  	if (ret)
>  		return ret;
>  
> @@ -66,23 +78,24 @@ static const struct hwmon_chip_info sy7636a_chip_info = {
>  static int sy7636a_sensor_probe(struct platform_device *pdev)
>  {
>  	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
> -	struct regulator *regulator;
> +	struct sy7636a_hwmon_data *drvdata;
>  	struct device *hwmon_dev;
>  	int err;
>  
>  	if (!regmap)
>  		return -EPROBE_DEFER;
>  
> -	regulator = devm_regulator_get_optional(&pdev->dev, "vcom");
> -	if (IS_ERR_OR_NULL(regulator))
> -		return -EPROBE_DEFER;
> +	drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
>  
> -	err = regulator_enable(regulator);
> -	if (err)
> -		return err;
> +	drvdata->regmap = regmap;
> +	drvdata->regulator = devm_regulator_get_optional(&pdev->dev, "vcom");
> +	if (IS_ERR_OR_NULL(drvdata->regulator))
> +		return -EPROBE_DEFER;
>  
>  	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
> -							 "sy7636a_temperature", regmap,
> +							 "sy7636a_temperature", drvdata,
>  							 &sy7636a_chip_info, NULL);
>  
>  	if (IS_ERR(hwmon_dev)) {
> @@ -102,5 +115,6 @@ static struct platform_driver sy7636a_sensor_driver = {
>  };
>  module_platform_driver(sy7636a_sensor_driver);
>  
> +MODULE_ALIAS("platform:sy7636a-temperature");
>  MODULE_DESCRIPTION("SY7636A sensor driver");
>  MODULE_LICENSE("GPL");

Re: [PATCH 2/2] hwmon: (sy7636a) enable regulator only if needed

[Andreas Kemnade]

On Wed, 24 Sep 2025 09:06:15 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On Sat, Sep 20, 2025 at 01:43:11PM +0200, Andreas Kemnade wrote:
> > Avoid having all the regulators in the SY7636A enabled all the time
> > to significantly reduce current consumption. In pratical scenarios,
> > the regulators are only needed when a refresh is done on the epaper
> > display powered by the SY7636A. This is can save around 10mA which
> > is much for this kind of devices.
> > Also fixes the asymmetrical single enable call.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> >  drivers/hwmon/sy7636a-hwmon.c | 34 ++++++++++++++++++++++++----------
> >  1 file changed, 24 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
> > index e83d0e50c735..0fda69bea3b4 100644
> > --- a/drivers/hwmon/sy7636a-hwmon.c
> > +++ b/drivers/hwmon/sy7636a-hwmon.c
> > @@ -18,14 +18,26 @@
> >  
> >  #include <linux/mfd/sy7636a.h>
> >  
> > +struct sy7636a_hwmon_data {
> > +	struct regmap *regmap;
> > +	struct regulator *regulator;
> > +};
> > +
> > +
> >  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);
> > +	struct sy7636a_hwmon_data *drvdata = dev_get_drvdata(dev);
> >  	int ret, reg_val;
> >  
> > -	ret = regmap_read(regmap,
> > +	ret = regulator_enable(drvdata->regulator);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_read(drvdata->regmap,
> >  			  SY7636A_REG_TERMISTOR_READOUT, &reg_val);  
> 
> Does that really work without delay ? Usually it takes some time for a chip
> to return useful data after its power supply has been enabled. calls
> 
Hmm, enabling the onoff bit behind the regulator is just needed to turn
ADC on here. But there is also the power good wait which should usually be enough.
Usually... But there is no guarantee. So yes, better wait for one adc aquisition.

Regards,
Andreas

Re: [PATCH 2/2] hwmon: (sy7636a) enable regulator only if needed

[Guenter Roeck]

On 9/24/25 13:40, Andreas Kemnade wrote:
> On Wed, 24 Sep 2025 09:06:15 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On Sat, Sep 20, 2025 at 01:43:11PM +0200, Andreas Kemnade wrote:
>>> Avoid having all the regulators in the SY7636A enabled all the time
>>> to significantly reduce current consumption. In pratical scenarios,
>>> the regulators are only needed when a refresh is done on the epaper
>>> display powered by the SY7636A. This is can save around 10mA which
>>> is much for this kind of devices.
>>> Also fixes the asymmetrical single enable call.
>>>
>>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
>>> ---
>>>   drivers/hwmon/sy7636a-hwmon.c | 34 ++++++++++++++++++++++++----------
>>>   1 file changed, 24 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
>>> index e83d0e50c735..0fda69bea3b4 100644
>>> --- a/drivers/hwmon/sy7636a-hwmon.c
>>> +++ b/drivers/hwmon/sy7636a-hwmon.c
>>> @@ -18,14 +18,26 @@
>>>   
>>>   #include <linux/mfd/sy7636a.h>
>>>   
>>> +struct sy7636a_hwmon_data {
>>> +	struct regmap *regmap;
>>> +	struct regulator *regulator;
>>> +};
>>> +
>>> +
>>>   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);
>>> +	struct sy7636a_hwmon_data *drvdata = dev_get_drvdata(dev);
>>>   	int ret, reg_val;
>>>   
>>> -	ret = regmap_read(regmap,
>>> +	ret = regulator_enable(drvdata->regulator);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = regmap_read(drvdata->regmap,
>>>   			  SY7636A_REG_TERMISTOR_READOUT, &reg_val);
>>
>> Does that really work without delay ? Usually it takes some time for a chip
>> to return useful data after its power supply has been enabled. calls
>>
> Hmm, enabling the onoff bit behind the regulator is just needed to turn
> ADC on here. But there is also the power good wait which should usually be enough.
> Usually... But there is no guarantee. So yes, better wait for one adc aquisition.
> 

Would that be the power good timeout (50ms) ? The conversion time on top if that
is not much, but the power good wait is a long time. That strongly suggests that
this should be made optional. Not everyone might like the additional delay for
each temperature reading.

Guenter

Re: [PATCH 2/2] hwmon: (sy7636a) enable regulator only if needed

[Andreas Kemnade]

On Wed, 24 Sep 2025 14:06:57 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On 9/24/25 13:40, Andreas Kemnade wrote:
> > On Wed, 24 Sep 2025 09:06:15 -0700
> > Guenter Roeck <linux@roeck-us.net> wrote:
> >   
> >> On Sat, Sep 20, 2025 at 01:43:11PM +0200, Andreas Kemnade wrote:  
> >>> Avoid having all the regulators in the SY7636A enabled all the time
> >>> to significantly reduce current consumption. In pratical scenarios,
> >>> the regulators are only needed when a refresh is done on the epaper
> >>> display powered by the SY7636A. This is can save around 10mA which
> >>> is much for this kind of devices.
> >>> Also fixes the asymmetrical single enable call.
> >>>
> >>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> >>> ---
> >>>   drivers/hwmon/sy7636a-hwmon.c | 34 ++++++++++++++++++++++++----------
> >>>   1 file changed, 24 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
> >>> index e83d0e50c735..0fda69bea3b4 100644
> >>> --- a/drivers/hwmon/sy7636a-hwmon.c
> >>> +++ b/drivers/hwmon/sy7636a-hwmon.c
> >>> @@ -18,14 +18,26 @@
> >>>   
> >>>   #include <linux/mfd/sy7636a.h>
> >>>   
> >>> +struct sy7636a_hwmon_data {
> >>> +	struct regmap *regmap;
> >>> +	struct regulator *regulator;
> >>> +};
> >>> +
> >>> +
> >>>   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);
> >>> +	struct sy7636a_hwmon_data *drvdata = dev_get_drvdata(dev);
> >>>   	int ret, reg_val;
> >>>   
> >>> -	ret = regmap_read(regmap,
> >>> +	ret = regulator_enable(drvdata->regulator);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	ret = regmap_read(drvdata->regmap,
> >>>   			  SY7636A_REG_TERMISTOR_READOUT, &reg_val);  
> >>
> >> Does that really work without delay ? Usually it takes some time for a chip
> >> to return useful data after its power supply has been enabled. calls
> >>  
> > Hmm, enabling the onoff bit behind the regulator is just needed to turn
> > ADC on here. But there is also the power good wait which should usually be enough.
> > Usually... But there is no guarantee. So yes, better wait for one adc aquisition.
> >   
> 
> Would that be the power good timeout (50ms) ? The conversion time on top if that
> is not much, but the power good wait is a long time. That strongly suggests that
> this should be made optional. Not everyone might like the additional delay for
> each temperature reading.
> 
Yes, that is the mechanism. Well, the regulator can be marked as always
enabled via regulator-aways-on flag in devicetree so that is optional, that
would have the side effect of having temperature conversion always
working, no delay needed. The power good stuff is about the stepup
regulators, so I think (will verify that in experiments) waiting for the
conversation time (around 0.5 ms) after the on bit is set is probably enough.

And I have seen, the power good stuff is not really implemented yet,
had another incarnation of this driver in my head while writing this,
where regulator_enable waits for power good.

Regards,
Andreas