On Sun, 2020-05-31 at 15:54 +0100, Jonathan Cameron wrote: > [External] > > On Fri, 29 May 2020 15:45:33 +0200 > Johan Hovold <johan@xxxxxxxxxx> wrote: > > > [ Trimming CC to something more reasonable... ] > > > > On Fri, May 29, 2020 at 11:08:38AM +0000, Ardelean, Alexandru wrote: > > > On Fri, 2020-05-29 at 12:16 +0200, Johan Hovold wrote: > > > > On Fri, May 22, 2020 at 11:22:07AM +0300, Alexandru Ardelean > > > > wrote: > > > > > This assignment is the more peculiar of the bunch as it assigns > > > > > the parent > > > > > of the platform-device's device (i.e. pdev->dev.parent) as the > > > > > IIO device's > > > > > parent. > > > > > > > > > > It's unclear whether this is intentional or not. > > > > > Hence it is in it's own patch. > > > > > > > > Yeah, we have a few mfd drivers whose child drivers registers their > > > > class devices directly under the parent mfd device rather than the > > > > corresponding child platform device. > > > > > > > > Since it's done consistently I think you need to update them all if > > > > you > > > > really want to change this. > > > > > > > > And it may not be worth it since at least in theory someone could > > > > now be > > > > relying on this topology. > > > > > > Thanks for the feedback. > > > I guess, it could make sense to do here: > > > devm_iio_device_alloc(pdev->dev.parent, ...) > > > > > > Currently it's: > > > devm_iio_device_alloc(&pdev->dev, ...) > > > > > > That would make it slightly more consistent. i.e. the life-time of > > > the object would be attached to the parent of the platform device, > > > versus the platform-device. > > > > Not really. If you unbind the iio driver, the iio device gets > > deregistered (as it should be) and there's no need to keep it around > > any > > more. > > > > You'd essentially just leak resources every time you rebind the driver > > (e.g. during development). > > > > And in fact, you could also introduce a use-after-free depending on if > > the parent mfd driver use devres to deregister its children. > > > > > Currently, as it is, the allocation [of the IIO device] is tied the > > > platform-device, and the IIO registration to the parent (of the > > > platform-device). > > > > Not quite; the iio device still gets deregistered when the platform > > device is unbound. > > > > > I'm not super-familiar with the internals here, but does this sound a > > > bit wrong? > > > > It's not a common pattern but not necessarily wrong per se. > > > > > Is there a chance where the IIO device could be de-allocated, while > > > registered? > > > > No, the device-managed iio device object is freed when the platform > > device is unbound and specifically after the iio device has been > > deregistered. > > I had a feeling we might have a few cases like this hiding in IIO. > > For these I'd just leave the parent being manually set. > It doesn't do any harm and the facility existing is useful for messing > around with topology. > > We may however want to wrap it up in a utility function on the > basis that we may want to change the visibility and location > of the IIO device down the line. > > iio_device_set_parent() perhaps with docs to specify that it must > be called between allocation and registration + is meant to allow > cases where we want a different parent than the device used for > managed allocations etc. > Works for me. If no objections, I'll include in the next re-spin. > Jonathan > > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> > > > > > --- > > > > > drivers/iio/light/lm3533-als.c | 1 - > > > > > 1 file changed, 1 deletion(-) > > > > > > > > > > diff --git a/drivers/iio/light/lm3533-als.c > > > > > b/drivers/iio/light/lm3533-als.c > > > > > index bc196c212881..0f380ec8d30c 100644 > > > > > --- a/drivers/iio/light/lm3533-als.c > > > > > +++ b/drivers/iio/light/lm3533-als.c > > > > > @@ -852,7 +852,6 @@ static int lm3533_als_probe(struct > > > > > platform_device > > > > > *pdev) > > > > > indio_dev->channels = lm3533_als_channels; > > > > > indio_dev->num_channels = ARRAY_SIZE(lm3533_als_channels); > > > > > indio_dev->name = dev_name(&pdev->dev); > > > > > - indio_dev->dev.parent = pdev->dev.parent; > > > > > indio_dev->modes = INDIO_DIRECT_MODE; > > > > > > > > > > als = iio_priv(indio_dev); > > > > Johan
