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

[Guenter Roeck <linux@roeck-us.net>]

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