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

From: Jishnu Prakash <quic_jprakash@quicinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: "Jonathan Cameron" <jic23@kernel.org>,
	agross@kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linus.walleij@linaro.org,
	Jonathan.Cameron@huawei.com, sboyd@kernel.org,
	quic_subbaram@quicinc.com, quic_collinsd@quicinc.com,
	quic_kamalw@quicinc.com, marijn.suijten@somainline.org,
	andriy.shevchenko@linux.intel.com,
	krzysztof.kozlowski@linaro.org,
	"Lars-Peter Clausen" <lars@metafoo.de>,
	"Bjorn Andersson" <andersson@kernel.org>,
	"Konrad Dybcio" <konrad.dybcio@linaro.org>,
	"Arnd Bergmann" <arnd@arndb.de>,
	"Cosmin Tanislav" <demonsingur@gmail.com>,
	"Mike Looijmans" <mike.looijmans@topic.nl>,
	"Ramona Bolboaca" <ramona.bolboaca@analog.com>,
	"ChiYuan Huang" <cy_huang@richtek.com>,
	"Ibrahim Tilki" <Ibrahim.Tilki@analog.com>,
	"William Breathitt Gray" <william.gray@linaro.org>,
	"Lee Jones" <lee@kernel.org>,
	"Leonard Göhrs" <l.goehrs@pengutronix.de>,
	"Haibo Chen" <haibo.chen@nxp.com>,
	linux-iio@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-arm-msm-owner@vger.kernel.org
Subject: Re: [PATCH 07/11] iio: adc: Add support for QCOM PMIC5 Gen3 ADC
Date: Thu, 16 Nov 2023 08:54:04 +0530	[thread overview]
Message-ID: <60a058ba-0bd3-9a5e-5021-4f80b9df65a8@quicinc.com> (raw)
In-Reply-To: <CAA8EJprb=804S=GfxPw_7AxoHRO1XzC+wGPCsvNGrRpJgj+y6A@mail.gmail.com>

Hi Dmitry,

On 10/23/2023 1:33 PM, Dmitry Baryshkov wrote:
> On Mon, 23 Oct 2023 at 09:15, Jishnu Prakash<quic_jprakash@quicinc.com>  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@quicinc.com>  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

>