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