On 11/02/2025 21:07, Jonathan Cameron wrote:
On Tue, 11 Feb 2025 10:52:51 +0200
Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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
