linux-kernel - Re: [PATCH] iio: adc: lpc18xx_adc: Convert probe to device managed version

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CA+U=Dsrovvtd=iG=pYm53eM0GiOaoMgWPPan2rCLbpiP0PVjOQ@mail.gmail.com>
Date:   Mon, 11 Oct 2021 11:09:30 +0300
From:   Alexandru Ardelean <ardeleanalex@...il.com>
To:     André Gustavo Nakagomi Lopez <andregnl@....br>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>, vz@...ia.com,
        linux-iio <linux-iio@...r.kernel.org>,
        linux-arm-kernel@...ts.infradead.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] iio: adc: lpc18xx_adc: Convert probe to device managed version

On Fri, Oct 8, 2021 at 9:31 PM André Gustavo Nakagomi Lopez
<andregnl@....br> 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.
>

Hey,

Patch looks good overall.

A few notes:
1. you can now remove the "platform_set_drvdata(....)" call. The
private data is no longer needed.

> Signed-off-by: André Gustavo Nakagomi Lopez <andregnl@....br>
> ---
> I was not able to test the patch due to the fact I do not have the necessary hardware.
>  drivers/iio/adc/lpc18xx_adc.c | 60 +++++++++++++++++++----------------
>  1 file changed, 33 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/iio/adc/lpc18xx_adc.c b/drivers/iio/adc/lpc18xx_adc.c
> index 3566990ae87d..7b6ba5e4a003 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_writel(void *data)
> +{
> +       struct lpc18xx_adc *adc = data;
> +
> +       writel(0, adc->base + LPC18XX_ADC_CR);
> +}
> +
> +static void lpc18xx_clk_disable_unprepare(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;
> @@ -163,46 +180,36 @@ 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;
>         }
>
> +       ret = devm_add_action_or_reset(&pdev->dev, lpc18xx_clk_disable_unprepare,

You can leave this as-is.but   "lpc18xx_clk_disable" also works.
No strong opinion

> +                                      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;
> -       }
> +       ret = devm_add_action_or_reset(&pdev->dev, lpc18xx_writel, adc);

"lpc18xx_writel" is a little vague. Maybe "lpc18xx_clear_cr_reg()"
since this clears the control register.

> +       if (ret)
> +               return ret;
>
> -       return 0;
> +       ret = devm_iio_device_register(&pdev->dev, indio_dev);

You can directly do:
return devm_iio_device_register(&pdev->dev, indio_dev);

The kernel should print probe failure.

> +       if (ret)
> +               dev_err(&pdev->dev, "unable to register device\n");
>
> -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);
> -
> -       return 0;
> -}
> -
>  static const struct of_device_id lpc18xx_adc_match[] = {
>         { .compatible = "nxp,lpc1850-adc" },
>         { /* sentinel */ }
> @@ -211,7 +218,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,
> --
> 2.33.0
>