Re: [PATCH v2 2/5] iio: adc: add helpers for parsing ADC nodes

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 11/02/2025 21:07, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 10:52:51 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 

...

>>>> +int iio_adc_device_get_channels(struct device *dev, int *channels,
>>>> +				int max_channels)
>>>> +{
>>>> +	struct fwnode_handle *fwnode, *child;
>>>> +	int num_chan = 0, ret;
>>>> +
>>>> +	fwnode = dev_fwnode(dev);
>>>> +	if (!fwnode) {
>>>
>>> As before, I'd relax this until we need to do it. We may never do so.
>>
>> The BD79124 does not require this as I dropped the MFD, so this is
>> ultimately your call :) I, however, would like to humbly suggest adding
>> it now ;) I have changed some APIs in the regulator subsystem and in the
>> clk subsystem to support cases where the device-tree node is in the
>> parent (usual MFD device-case), and it has been somewhat scary... (What
>> if somewhere in some of the existing device-trees the parent happens to
>> have interesting properties - but it actually is not correct node? This
>> becomes a potential source of regression when adding support to an
>> existing API).
>>
>> Furthermore, when the node is unconditionally taken from the given
>> device-pointer, it is tempting for the caller to just pass the parent
>> device as argument...
>>
>>    - If you have done this - please raise your hand. /me raises.
>>    - If you have only later realized it can cause life-time issues when
>>      devm is used - please raise your hand. /me raises.
>>    - If you have tried to kick your own behind when fixing the issues -
>>      please, raise your hand. /me raises. (and if you succeeded - congraz,
>>      you aren't as old and clumsy as I am).
> 
> Maybe. I'd be happier if there was a single user included
> with a patch set doing this.  I'm not sure this applies to
> any of the SoC ADCs which are MFD hosted but maybe it does.
> 

I did a quick "grep powered audit" of the ADC drivers out of the 
curiosity. It seems to me that most of the platform drivers under ADC do 
have the of_match table populated. I suppose they have own node for the 
ADC stuff with ADC specific compatibles, so they're safe from this problem.

(I think we should still also consider cases where the ADC block does 
not have own compatible/node. This sure is just an opinion but I think 
it is kind of artificial to have own node for ADC block when it is just 
a part of a smallish device. Also, having multiple compatibles for one 
device feels "quirky" to me).

The exception to a rule seems to be the 'sun4i-gpadc-iio.c'

And, if I am not misreading the code, the thermal zone registration may 
be causing some problems. It seems to me the data of the thermal zone is 
allocated by the devm_iio_device_alloc() - and bound to the lifetime of 
the platform device.

The thermal zone in MFD branch is bound to the life time of the parent 
(MFD) device.

(in MFD case):
	info->sensor_device = pdev->dev.parent;
...

devm_thermal_of_zone_register(info->sensor_device, ...

I believe that releasing the IIO device will free the thermal zone data 
- but the thermal zone stays there until MFD is released. I haven't 
checked when the thermal zone uses the data though - but I'd be a bit 
wary of this design. Furthermore, removing and re-registering the IIO 
driver may cause some collision with the thermal zone which has stayed 
there. (Again, I didn't check the thermal zone implementation so I'm not 
sure this really is a problem).

In any case, it seems to me most of the current IIO ADC MFD devices have 
dt node "bound" to the platform device and don't use the parent node.

Yours,
   -- Matti