On Fri, 3 Jun 2022 17:06:12 +0100
Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> On Fri, 3 Jun 2022 12:59:59 +0300
> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> > It feels wrong and actually inconsistent to attach managed resources
> > to the IIO device object, which is child of the physical device object.
> > The rest of the ->probe() calls do that against physical device.
> >
> > Resolve this by reassigning managed resources to the physical device object.
> >
> > Fixes: 3adbf3427330 ("iio: adc: add a driver for the SAR ADC found in Amlogic Meson SoCs")
> > Suggested-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Hi Andy,
>
> This has come up a few times in the past (and we elected to not clean it up
> at the time, though it wasn't a decision to never do so!)
>
> It would definitely be wrong if we had another driver binding against
> the resulting created device (funnily enough I reported a bug on a driver
> doing just that earlier this week), but in this case it's harmless because the
> the tear down will occur with a put_device() ultimately calling device_release()
> and devres_release_all()
>
> https://elixir.bootlin.com/linux/latest/source/drivers/base/core.c#L2211
>
> Has a comment that covers this case (more or less).
> "
> * Some platform devices are driven without driver attached
> * and managed resources may have been acquired. Make sure
> * all resources are released.
> "
>
> Now, I definitely agree with your statement that it's a bit inconsistent to
> do this, just not the fixes tag.
>
> One other suggestion below.
>
>
> > ---
> > v3: new fix-patch
> > drivers/iio/adc/meson_saradc.c | 12 +++++-------
> > 1 file changed, 5 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> > index 62cc6fb0ef85..4fe6b997cd03 100644
> > --- a/drivers/iio/adc/meson_saradc.c
> > +++ b/drivers/iio/adc/meson_saradc.c
> > @@ -650,11 +650,11 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev,
> > void __iomem *base)
> > {
> > struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
> > + struct device *dev = indio_dev->dev.parent;
>
> I'd slightly prefer the device was passed in explicitly to this function rather
> than using the parent assignment which feels a little fragile.
Meh, ignore this. I see from one of the later patches, the driver is already
making the assumption this is set in other calls, so we aren't making anything
worse with this change.
Jonathan
>
>
> > struct clk_init_data init;
> > const char *clk_parents[1];
> >
> > - init.name = devm_kasprintf(&indio_dev->dev, GFP_KERNEL, "%s#adc_div",
> > - dev_name(indio_dev->dev.parent));
> > + init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#adc_div", dev_name(dev));
> > if (!init.name)
> > return -ENOMEM;
> >
> > @@ -670,13 +670,11 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev,
> > priv->clk_div.hw.init = &init;
> > priv->clk_div.flags = 0;
> >
> > - priv->adc_div_clk = devm_clk_register(&indio_dev->dev,
> > - &priv->clk_div.hw);
> > + priv->adc_div_clk = devm_clk_register(dev, &priv->clk_div.hw);
> > if (WARN_ON(IS_ERR(priv->adc_div_clk)))
> > return PTR_ERR(priv->adc_div_clk);
> >
> > - init.name = devm_kasprintf(&indio_dev->dev, GFP_KERNEL, "%s#adc_en",
> > - dev_name(indio_dev->dev.parent));
> > + init.name = devm_kasprintf(dev, GFP_KERNEL, "%s#adc_en", dev_name(dev));
> > if (!init.name)
> > return -ENOMEM;
> >
> > @@ -690,7 +688,7 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev,
> > priv->clk_gate.bit_idx = __ffs(MESON_SAR_ADC_REG3_CLK_EN);
> > priv->clk_gate.hw.init = &init;
> >
> > - priv->adc_clk = devm_clk_register(&indio_dev->dev, &priv->clk_gate.hw);
> > + priv->adc_clk = devm_clk_register(dev, &priv->clk_gate.hw);
> > if (WARN_ON(IS_ERR(priv->adc_clk)))
> > return PTR_ERR(priv->adc_clk);
> >
>
