On Fri, Oct 8, 2021 at 9:31 PM 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.
>
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@xxxxxx>
> ---
> 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
>
