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

[Jonathan Cameron <jic23@kernel.org>]

On Fri,  3 Jun 2022 12:59:59 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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@metafoo.de>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
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. 


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