On Sun, Oct 17, 2021 at 12:35:33PM +0100, Jonathan Cameron wrote:
> On Fri, 15 Oct 2021 15:47:12 -0300
> André Gustavo Nakagomi Lopez <andregnl@xxxxxx> wrote:
>
> > The remove function and the goto sections are not necessary if devm
> > functions are used.
> >
> > Convert device register to devm version. Add hook functions to release
> > device resources, and use them inside probe with devm_add_action,
> > which will release resources on driver detach.
> >
> > To maintain the order of which device resources were released/reseted,
> > register the hook functions as soon as resources are obtained/initialized.
> > Since devres actions are called on driver detach, the remove
> > function and the error-handling goto sections are no longer necessary.
> >
> > Signed-off-by: André Gustavo Nakagomi Lopez <andregnl@xxxxxx>
>
> Hi André
>
> Looks good to me, but I did notice an unrelated issue whilst looking at this driver.
> Comment on that inline. Follow up patch welcome!
>
> Applied to the togreg branch of iio.git and pushed out as testing to let 0-day
> see if it can find anything we missed.
>
> Thanks,
>
> Jonathan
>
>
>
> > ---
> > Change log V1 -> V2:
> > -Renamed lpc18xx_clk_disable_unprepare function
> > -Removed platform_set_drvdata(pdev, indio_dev)
> > -Renamed lpc18xx_writel to lpc18xx_clear_cr_reg
> > -devm_iio_device_register now returns directly in the end of probe function
> > drivers/iio/adc/lpc18xx_adc.c | 61 ++++++++++++++++++-----------------
> > 1 file changed, 31 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/iio/adc/lpc18xx_adc.c b/drivers/iio/adc/lpc18xx_adc.c
> > index 3566990ae87d..7d50107427ac 100644
> > --- a/drivers/iio/adc/lpc18xx_adc.c
> > +++ b/drivers/iio/adc/lpc18xx_adc.c
> > @@ -115,6 +115,23 @@ static const struct iio_info lpc18xx_adc_info = {
> > .read_raw = lpc18xx_adc_read_raw,
> > };
> >
> > +static void lpc18xx_clear_cr_reg(void *data)
> > +{
> > + struct lpc18xx_adc *adc = data;
> > +
> > + writel(0, adc->base + LPC18XX_ADC_CR);
> > +}
> > +
> > +static void lpc18xx_clk_disable(void *clk)
> > +{
> > + clk_disable_unprepare(clk);
> > +}
> > +
> > +static void lpc18xx_regulator_disable(void *vref)
> > +{
> > + regulator_disable(vref);
> > +}
> > +
> > static int lpc18xx_adc_probe(struct platform_device *pdev)
> > {
> > struct iio_dev *indio_dev;
> > @@ -127,7 +144,6 @@ static int lpc18xx_adc_probe(struct platform_device *pdev)
> > if (!indio_dev)
> > return -ENOMEM;
> >
> > - platform_set_drvdata(pdev, indio_dev);
> > adc = iio_priv(indio_dev);
> > adc->dev = &pdev->dev;
> > mutex_init(&adc->lock);
> > @@ -163,44 +179,30 @@ static int lpc18xx_adc_probe(struct platform_device *pdev)
> > return ret;
> > }
> >
> > + ret = devm_add_action_or_reset(&pdev->dev, lpc18xx_regulator_disable, adc->vref);
> > + if (ret)
> > + return ret;
> > +
> > ret = clk_prepare_enable(adc->clk);
> > if (ret) {
> > dev_err(&pdev->dev, "unable to enable clock\n");
> > - goto dis_reg;
> > + return ret;
>
> Unrelated but clk_get_rate() as called before the clk_prepare_enable() is not guaranteed
> to work. So a good follow up patch would reorder that to be after the clk is turned on.
>
>
> > }
> >
> > + ret = devm_add_action_or_reset(&pdev->dev, lpc18xx_clk_disable,
> > + adc->clk);
> > + if (ret)
> > + return ret;
> > +
> > adc->cr_reg = (clkdiv << LPC18XX_ADC_CR_CLKDIV_SHIFT) |
> > LPC18XX_ADC_CR_PDN;
> > writel(adc->cr_reg, adc->base + LPC18XX_ADC_CR);
> >
> > - ret = iio_device_register(indio_dev);
> > - if (ret) {
> > - dev_err(&pdev->dev, "unable to register device\n");
> > - goto dis_clk;
> > - }
> > -
> > - return 0;
> > -
> > -dis_clk:
> > - writel(0, adc->base + LPC18XX_ADC_CR);
> > - clk_disable_unprepare(adc->clk);
> > -dis_reg:
> > - regulator_disable(adc->vref);
> > - return ret;
> > -}
> > -
> > -static int lpc18xx_adc_remove(struct platform_device *pdev)
> > -{
> > - struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > - struct lpc18xx_adc *adc = iio_priv(indio_dev);
> > -
> > - iio_device_unregister(indio_dev);
> > -
> > - writel(0, adc->base + LPC18XX_ADC_CR);
> > - clk_disable_unprepare(adc->clk);
> > - regulator_disable(adc->vref);
> > + ret = devm_add_action_or_reset(&pdev->dev, lpc18xx_clear_cr_reg, adc);
> > + if (ret)
> > + return ret;
> >
> > - return 0;
> > + return devm_iio_device_register(&pdev->dev, indio_dev);
> > }
> >
> > static const struct of_device_id lpc18xx_adc_match[] = {
> > @@ -211,7 +213,6 @@ MODULE_DEVICE_TABLE(of, lpc18xx_adc_match);
> >
> > static struct platform_driver lpc18xx_adc_driver = {
> > .probe = lpc18xx_adc_probe,
> > - .remove = lpc18xx_adc_remove,
> > .driver = {
> > .name = "lpc18xx-adc",
> > .of_match_table = lpc18xx_adc_match,
>
Ok. Thank you for the review, I will work on the change.
