Message-ID: <e6c8f022-ebfc-459c-806b-f75618f65916@roeck-us.net>
Date: Wed, 24 Sep 2025 12:16:14 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Andreas Kemnade <andreas@...nade.info>
Cc: Mark Brown <broonie@...nel.org>, jdelvare@...e.com, lgirdwood@...il.com,
 linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
 Alistair Francis <alistair@...stair23.me>,
 "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
 Jonathan Cameron <jic23@...nel.org>
Subject: Re: [PATCH RFC 1/2] hwmon: (sy7636a) fix races during probe of mfd
 subdevices

On 9/24/25 10:53, Andreas Kemnade wrote:
> On Wed, 24 Sep 2025 00:17:48 -0700
> Guenter Roeck <linux@...ck-us.net> wrote:
> 
>> On 9/24/25 00:00, Andreas Kemnade wrote:
>>> On Sat, 20 Sep 2025 23:18:59 +0100
>>> Mark Brown <broonie@...nel.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

Powered by blists - more mailing lists