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:23:07 +0100
Jonathan Cameron <jic23@xxxxxxxxxx> wrote:

> 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.

Andy, put a cover letter on these larger series - if nothing else it gives
somewhere convenient for people to give tags for the whole series, or 
maintainer to say what they are doing with it.

Anyhow, I'm fine with the series, but will leave it on list for a while
longer, particularly to get patch 6 some eyes + testing.

Currently I plan to drop the fixes tag from this first patch, but I'm prepared
to be convinced it's a bug fix rather than a consistency cleanup.

Jonathan

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