Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Message-ID: <60a058ba-0bd3-9a5e-5021-4f80b9df65a8@quicinc.com>
Date:   Thu, 16 Nov 2023 08:54:04 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.1
From:   Jishnu Prakash <quic_jprakash@quicinc.com>
Subject: Re: [PATCH 07/11] iio: adc: Add support for QCOM PMIC5 Gen3 ADC
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>,
        =?UTF-8?Q?Leonard_G=c3=b6hrs?= <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>
References: <20230708072835.3035398-1-quic_jprakash@quicinc.com>
 <20230708072835.3035398-8-quic_jprakash@quicinc.com>
 <20230708165955.08c1159c@jic23-huawei>
 <13d5dee2-1da4-2ad0-91f8-a53b43adba2b@quicinc.com>
 <CAA8EJprb=804S=GfxPw_7AxoHRO1XzC+wGPCsvNGrRpJgj+y6A@mail.gmail.com>
Content-Language: en-US
In-Reply-To: <CAA8EJprb=804S=GfxPw_7AxoHRO1XzC+wGPCsvNGrRpJgj+y6A@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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


>