Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
MIME-Version: 1.0
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>
In-Reply-To: <13d5dee2-1da4-2ad0-91f8-a53b43adba2b@quicinc.com>
From:   Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date:   Mon, 23 Oct 2023 11:03:46 +0300
Message-ID: <CAA8EJprb=804S=GfxPw_7AxoHRO1XzC+wGPCsvNGrRpJgj+y6A@mail.gmail.com>
Subject: Re: [PATCH 07/11] iio: adc: Add support for QCOM PMIC5 Gen3 ADC
To:     Jishnu Prakash <quic_jprakash@quicinc.com>
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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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:

> >> +
> >> +    ret = adc5_get_fw_data(adc);
> >> +    if (ret < 0) {
> >> +            dev_err(adc->dev, "adc get dt data failed, ret=%d\n", ret);
> >> +            return ret;
> >> +    }
> >> +
> >> +    for (i = 0; i < adc->num_sdams; i++) {
> >> +            ret = devm_request_irq(dev, adc->base[i].irq, adc5_gen3_isr,
> >> +                                    0, adc->base[i].irq_name, adc);
> >> +            if (ret < 0) {
> >> +                    dev_err(adc->dev, "Getting IRQ %d failed, ret=%d\n", adc->base[i].irq, ret);
> >> +                    return ret;
> >> +            }
> >> +    }
> >> +
> >> +    ret = adc_tm_register_tzd(adc);
> >> +    if (ret < 0)
> >> +            return ret;
> >> +
> >> +    if (adc->n_tm_channels)
> >> +            INIT_WORK(&adc->tm_handler_work, tm_handler_work);
> >> +
> >> +    indio_dev->name = pdev->name;
> >> +    indio_dev->modes = INDIO_DIRECT_MODE;
> >> +    indio_dev->info = &adc5_gen3_info;
> >> +    indio_dev->channels = adc->iio_chans;
> >> +    indio_dev->num_channels = adc->nchannels;
> >> +
> >> +    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]


-- 
With best wishes
Dmitry