On 6/2/22 13:42, Andy Shevchenko wrote:
return -ENOMEM;
>
> @@ -690,7 +687,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(idev, &priv->clk_gate.hw);
You are not changing anything here. But we shouldn't be devm'ing on the
IIO device. It will get freed eventually, but only when the last
reference to the iio device has been dropped, which might be long after
the platform device has been removed. devm'ing should happen on the
platform_device's device. Might be worth fixing.
> if (WARN_ON(IS_ERR(priv->adc_clk)))
> return PTR_ERR(priv->adc_clk);
>
> @@ -706,8 +703,7 @@ static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
> size_t read_len;
>
On Thu, Jun 2, 2022 at 2:08 PM Lars-Peter Clausen <[email protected]> wrote:
> On 6/2/22 13:42, Andy Shevchenko wrote:
...
> > - priv->adc_clk = devm_clk_register(&indio_dev->dev, &priv->clk_gate.hw);
> > + priv->adc_clk = devm_clk_register(idev, &priv->clk_gate.hw);
>
> You are not changing anything here.
The scope of patch is supposed not to change the current behaviour :-)
> But we shouldn't be devm'ing on the
> IIO device. It will get freed eventually, but only when the last
> reference to the iio device has been dropped, which might be long after
> the platform device has been removed. devm'ing should happen on the
> platform_device's device. Might be worth fixing.
Thanks for confirming my suspicions (as I mentioned to Martin, using
an IIO device there feels wrong).
I will add another patch to v3.
--
With Best Regards,
Andy Shevchenko
On Thu, 2 Jun 2022 15:45:27 +0200
Andy Shevchenko <[email protected]> wrote:
> On Thu, Jun 2, 2022 at 2:08 PM Lars-Peter Clausen <[email protected]> wrote:
> > On 6/2/22 13:42, Andy Shevchenko wrote:
>
> ...
>
> > > - priv->adc_clk = devm_clk_register(&indio_dev->dev, &priv->clk_gate.hw);
> > > + priv->adc_clk = devm_clk_register(idev, &priv->clk_gate.hw);
> >
> > You are not changing anything here.
>
> The scope of patch is supposed not to change the current behaviour :-)
>
> > But we shouldn't be devm'ing on the
> > IIO device. It will get freed eventually, but only when the last
> > reference to the iio device has been dropped, which might be long after
> > the platform device has been removed. devm'ing should happen on the
> > platform_device's device. Might be worth fixing.
>
> Thanks for confirming my suspicions (as I mentioned to Martin, using
> an IIO device there feels wrong).
> I will add another patch to v3.
>
I thought that the iio_dev ends up holding a reference to the platform
device dev so the parent would only be released if the child had already been,
but I may well be wrong on that.
Either way I don't mind it being tidied up.
Jonathan