All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] iio: adc: lpc18xx_adc: Convert probe to device managed version
@ 2021-10-08 18:29 ` André Gustavo Nakagomi Lopez
  0 siblings, 0 replies; 4+ messages in thread
From: André Gustavo Nakagomi Lopez @ 2021-10-08 18:29 UTC (permalink / raw)
  To: jic23, lars, vz; +Cc: linux-iio, linux-arm-kernel, linux-kernel, andregnl

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@usp.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,
+				       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);
+	if (ret)
+		return ret;
 
-	return 0;
+	ret = devm_iio_device_register(&pdev->dev, indio_dev);
+	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


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH] iio: adc: lpc18xx_adc: Convert probe to device managed version
@ 2021-10-08 18:29 ` André Gustavo Nakagomi Lopez
  0 siblings, 0 replies; 4+ messages in thread
From: André Gustavo Nakagomi Lopez @ 2021-10-08 18:29 UTC (permalink / raw)
  To: jic23, lars, vz; +Cc: linux-iio, linux-arm-kernel, linux-kernel, andregnl

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@usp.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,
+				       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);
+	if (ret)
+		return ret;
 
-	return 0;
+	ret = devm_iio_device_register(&pdev->dev, indio_dev);
+	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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] iio: adc: lpc18xx_adc: Convert probe to device managed version
  2021-10-08 18:29 ` André Gustavo Nakagomi Lopez
@ 2021-10-11  8:09   ` Alexandru Ardelean
  -1 siblings, 0 replies; 4+ messages in thread
From: Alexandru Ardelean @ 2021-10-11  8:09 UTC (permalink / raw)
  To: André Gustavo Nakagomi Lopez
  Cc: Jonathan Cameron, Lars-Peter Clausen, vz, linux-iio,
	linux-arm-kernel, LKML

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] iio: adc: lpc18xx_adc: Convert probe to device managed version
@ 2021-10-11  8:09   ` Alexandru Ardelean
  0 siblings, 0 replies; 4+ messages in thread
From: Alexandru Ardelean @ 2021-10-11  8:09 UTC (permalink / raw)
  To: André Gustavo Nakagomi Lopez
  Cc: Jonathan Cameron, Lars-Peter Clausen, vz, linux-iio,
	linux-arm-kernel, LKML

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-10-11  8:12 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-08 18:29 [PATCH] iio: adc: lpc18xx_adc: Convert probe to device managed version André Gustavo Nakagomi Lopez
2021-10-08 18:29 ` André Gustavo Nakagomi Lopez
2021-10-11  8:09 ` Alexandru Ardelean
2021-10-11  8:09   ` Alexandru Ardelean

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.