lkml.org 
[lkml]   [2022]   [Jun]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 1/6] iio: adc: meson_saradc: Don't attach managed resource to IIO device object
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);
>

\
 
 \ /
  Last update: 2022-06-03 17:57    [W:0.115 / U:0.532 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site