Re: [PATCH v3 1/6] iio: adc: meson_saradc: Don't attach managed resource to IIO device object

[Jonathan Cameron]

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);
> >    
>