Re: [PATCH 07/11] iio: adc: Add support for QCOM PMIC5 Gen3 ADC

[Jishnu Prakash]

Hi Dmitry,

On 10/23/2023 1:33 PM, Dmitry Baryshkov wrote:
> On Mon, 23 Oct 2023 at 09:15, Jishnu Prakash<quic_jprakash@xxxxxxxxxxx>  wrote:
> > Hi Jonathan,
> >
> > On 7/8/2023 9:29 PM, Jonathan Cameron wrote:
> > > On Sat, 8 Jul 2023 12:58:31 +0530
> > > Jishnu Prakash<quic_jprakash@xxxxxxxxxxx>  wrote:
> +
> > > > +    return devm_iio_device_register(dev, indio_dev);
> > > > +}
> > > > +
> > > > +static int adc5_gen3_exit(struct platform_device *pdev)
> > > > +{
> > > As you are mixing devm manged cleanup and the explicit sort the
> > > result is that you remove the userspace interfaces 'after' you run
> > > everything in here. I'm thinking disabling the channels at least
> > > isn't a good idea in that case.
> > >
> > > If you want to use devm (which is good) then you need to work out how
> > > to register additional callbacks during probe to tear down everything in
> > > the right order (typically the reverse of what happens in probe)
> > > devm_add_action_or_reset() is the way to add those extra callbacks.
> > >
> > > If not, just don't use devm for at least those bits that will end up
> > > running out of order (such as iio_device_register()) and manually call their
> > > cleanup routines instead.
> > I checked some other examples in the iio/adc/ folder, I think I see what
> > you mean here. It looks like drivers with a remove callback always use
> > iio_device_register and iio_device_unregister instead of the devm_*
> > variant, due to the issue with sysfs removal as you said.
> >
> > I'll update the probe and remove functions similarly, to do explicit
> > cleanups as required, avoiding devm_ usage for places where it should be
> > avoided.
> I think you got the message all wrong. There is nothing bad with using
> devm_. As a matter of fact it is a preferred form in most of the
> cases. However you have to be careful to tear down your device in the
> correct order. And as Jonathan pointed
> out, you might add necessary hooks manually by calling
> devm_add_action_or_reset().
>
> [skipped the rest]

Thanks for your comment. I checked the code again and I think we can do 
the teardown with a devm_add_action() call and drop the remove API 
entirely in favor of using devm_* APIs , I'll update this in the next 
patchset.

Thanks,

Jishnu


>