linux-iio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] iio: adc: ad7124: Fixes and devm_ for all of probe
@ 2021-05-08 18:23 Jonathan Cameron
  2021-05-08 18:23 ` [PATCH 1/3] iio: adc: ad7124: Fix missbalanced regulator enable / disable on error Jonathan Cameron
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Jonathan Cameron @ 2021-05-08 18:23 UTC (permalink / raw)
  To: linux-iio
  Cc: Lars-Peter Clausen, alexandru.tachici, Alexandru Ardelean,
	Jonathan Cameron

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

I noticed an issue around regulator error handling and managed to
hit another with my hacked together test setup.  Hence let's fix
those two issues first then we might as well follow up by converting
the last few bits of this driver to use device managed functions so
we can simplify the error handling and cleanup.

Testing conducted with QEMU hacking and insertion of errors at relevant
locations in the code.

Jonathan Cameron (3):
  iio: adc: ad7124: Fix missbalanced regulator enable / disable on
    error.
  iio: adc: ad7124: Fix potential overflow due to non sequential channel
    numbers
  iio: adc: ad7124: Use devm_ managed calls for all of probe() + drop
    remove()

 drivers/iio/adc/ad7124.c | 89 ++++++++++++++++++++--------------------
 1 file changed, 44 insertions(+), 45 deletions(-)

-- 
2.31.1


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

* [PATCH 1/3] iio: adc: ad7124: Fix missbalanced regulator enable / disable on error.
  2021-05-08 18:23 [PATCH 0/3] iio: adc: ad7124: Fixes and devm_ for all of probe Jonathan Cameron
@ 2021-05-08 18:23 ` Jonathan Cameron
  2021-05-09  7:20   ` Alexandru Ardelean
  2021-05-08 18:23 ` [PATCH 2/3] iio: adc: ad7124: Fix potential overflow due to non sequential channel numbers Jonathan Cameron
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Jonathan Cameron @ 2021-05-08 18:23 UTC (permalink / raw)
  To: linux-iio
  Cc: Lars-Peter Clausen, alexandru.tachici, Alexandru Ardelean,
	Jonathan Cameron

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

If the devm_regulator_get() call succeeded but not the regulator_enable()
then regulator_disable() would be called on a regulator that was not
enabled.

Fix this by moving regulator enabling / disabling over to
devm_ management via devm_add_action_or_reset.

Fixes: b3af341bbd96 ("iio: adc: Add ad7124 support")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Alexandru Ardelean <ardeleanalex@gmail.com>
---
 drivers/iio/adc/ad7124.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 766c73333604..c0d0870a29ff 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -707,6 +707,11 @@ static int ad7124_setup(struct ad7124_state *st)
 	return ret;
 }
 
+static void ad7124_reg_disable(void *r)
+{
+	regulator_disable(r);
+}
+
 static int ad7124_probe(struct spi_device *spi)
 {
 	const struct ad7124_chip_info *info;
@@ -752,17 +757,20 @@ static int ad7124_probe(struct spi_device *spi)
 		ret = regulator_enable(st->vref[i]);
 		if (ret)
 			return ret;
+
+		ret = devm_add_action_or_reset(&spi->dev, ad7124_reg_disable,
+					       st->vref[i]);
+		if (ret)
+			return ret;
 	}
 
 	st->mclk = devm_clk_get(&spi->dev, "mclk");
-	if (IS_ERR(st->mclk)) {
-		ret = PTR_ERR(st->mclk);
-		goto error_regulator_disable;
-	}
+	if (IS_ERR(st->mclk))
+		return PTR_ERR(st->mclk);
 
 	ret = clk_prepare_enable(st->mclk);
 	if (ret < 0)
-		goto error_regulator_disable;
+		return ret;
 
 	ret = ad7124_soft_reset(st);
 	if (ret < 0)
@@ -792,11 +800,6 @@ static int ad7124_probe(struct spi_device *spi)
 	ad_sd_cleanup_buffer_and_trigger(indio_dev);
 error_clk_disable_unprepare:
 	clk_disable_unprepare(st->mclk);
-error_regulator_disable:
-	for (i = ARRAY_SIZE(st->vref) - 1; i >= 0; i--) {
-		if (!IS_ERR_OR_NULL(st->vref[i]))
-			regulator_disable(st->vref[i]);
-	}
 
 	return ret;
 }
@@ -805,17 +808,11 @@ static int ad7124_remove(struct spi_device *spi)
 {
 	struct iio_dev *indio_dev = spi_get_drvdata(spi);
 	struct ad7124_state *st = iio_priv(indio_dev);
-	int i;
 
 	iio_device_unregister(indio_dev);
 	ad_sd_cleanup_buffer_and_trigger(indio_dev);
 	clk_disable_unprepare(st->mclk);
 
-	for (i = ARRAY_SIZE(st->vref) - 1; i >= 0; i--) {
-		if (!IS_ERR_OR_NULL(st->vref[i]))
-			regulator_disable(st->vref[i]);
-	}
-
 	return 0;
 }
 
-- 
2.31.1


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

* [PATCH 2/3] iio: adc: ad7124: Fix potential overflow due to non sequential channel numbers
  2021-05-08 18:23 [PATCH 0/3] iio: adc: ad7124: Fixes and devm_ for all of probe Jonathan Cameron
  2021-05-08 18:23 ` [PATCH 1/3] iio: adc: ad7124: Fix missbalanced regulator enable / disable on error Jonathan Cameron
@ 2021-05-08 18:23 ` Jonathan Cameron
  2021-05-09  7:22   ` Alexandru Ardelean
  2021-05-08 18:23 ` [PATCH 3/3] iio: adc: ad7124: Use devm_ managed calls for all of probe() + drop remove() Jonathan Cameron
  2021-05-09  7:31 ` [PATCH 0/3] iio: adc: ad7124: Fixes and devm_ for all of probe Alexandru Ardelean
  3 siblings, 1 reply; 9+ messages in thread
From: Jonathan Cameron @ 2021-05-08 18:23 UTC (permalink / raw)
  To: linux-iio
  Cc: Lars-Peter Clausen, alexandru.tachici, Alexandru Ardelean,
	Jonathan Cameron

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Channel numbering must start at 0 and then not have any holes, or
it is possible to overflow the available storage.  Note this bug was
introduced as part of a fix to ensure we didn't rely on the ordering
of child nodes.  So we need to support arbitrary ordering but they all
need to be there somewhere.

Note I hit this when using qemu to test the rest of this series.
Arguably this isn't the best fix, but it is probably the most minimal
option for backporting etc.

Fixes: d7857e4ee1ba6 ("iio: adc: ad7124: Fix DT channel configuration")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ad7124.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index c0d0870a29ff..9c2401c5848e 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -616,6 +616,13 @@ static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
 		if (ret)
 			goto err;
 
+		if (channel >= indio_dev->num_channels) {
+			dev_err(indio_dev->dev.parent,
+				"Channel index >= number of channels\n");
+			ret = -EINVAL;
+			goto err;
+		}
+
 		ret = of_property_read_u32_array(child, "diff-channels",
 						 ain, 2);
 		if (ret)
-- 
2.31.1


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

* [PATCH 3/3] iio: adc: ad7124: Use devm_ managed calls for all of probe() + drop remove()
  2021-05-08 18:23 [PATCH 0/3] iio: adc: ad7124: Fixes and devm_ for all of probe Jonathan Cameron
  2021-05-08 18:23 ` [PATCH 1/3] iio: adc: ad7124: Fix missbalanced regulator enable / disable on error Jonathan Cameron
  2021-05-08 18:23 ` [PATCH 2/3] iio: adc: ad7124: Fix potential overflow due to non sequential channel numbers Jonathan Cameron
@ 2021-05-08 18:23 ` Jonathan Cameron
  2021-05-09  7:30   ` Alexandru Ardelean
  2021-05-09  7:31 ` [PATCH 0/3] iio: adc: ad7124: Fixes and devm_ for all of probe Alexandru Ardelean
  3 siblings, 1 reply; 9+ messages in thread
From: Jonathan Cameron @ 2021-05-08 18:23 UTC (permalink / raw)
  To: linux-iio
  Cc: Lars-Peter Clausen, alexandru.tachici, Alexandru Ardelean,
	Jonathan Cameron

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

As not many steps were not already devm_ managed, use
devm_add_action_or_reset() to handle the rest.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Cc: Alexandru Ardelean <ardeleanalex@gmail.com>
---
 drivers/iio/adc/ad7124.c | 53 ++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 9c2401c5848e..deb166d2b645 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -719,6 +719,16 @@ static void ad7124_reg_disable(void *r)
 	regulator_disable(r);
 }
 
+static void ad7124_clk_disable(void *c)
+{
+	clk_disable_unprepare(c);
+}
+
+static void ad7124_buffer_cleanup(void *idev)
+{
+	ad_sd_cleanup_buffer_and_trigger(idev);
+}
+
 static int ad7124_probe(struct spi_device *spi)
 {
 	const struct ad7124_chip_info *info;
@@ -740,8 +750,6 @@ static int ad7124_probe(struct spi_device *spi)
 
 	ad_sd_init(&st->sd, indio_dev, spi, &ad7124_sigma_delta_info);
 
-	spi_set_drvdata(spi, indio_dev);
-
 	indio_dev->name = st->chip_info->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &ad7124_info;
@@ -779,48 +787,36 @@ static int ad7124_probe(struct spi_device *spi)
 	if (ret < 0)
 		return ret;
 
+	ret = devm_add_action_or_reset(&spi->dev, ad7124_clk_disable, st->mclk);
+	if (ret)
+		return ret;
+
 	ret = ad7124_soft_reset(st);
 	if (ret < 0)
-		goto error_clk_disable_unprepare;
+		return ret;
 
 	ret = ad7124_check_chip_id(st);
 	if (ret)
-		goto error_clk_disable_unprepare;
+		return ret;
 
 	ret = ad7124_setup(st);
 	if (ret < 0)
-		goto error_clk_disable_unprepare;
+		return ret;
 
 	ret = ad_sd_setup_buffer_and_trigger(indio_dev);
 	if (ret < 0)
-		goto error_clk_disable_unprepare;
-
-	ret = iio_device_register(indio_dev);
-	if (ret < 0) {
-		dev_err(&spi->dev, "Failed to register iio device\n");
-		goto error_remove_trigger;
-	}
+		return ret;
 
-	return 0;
+	ret = devm_add_action_or_reset(&spi->dev, ad7124_buffer_cleanup, indio_dev);
+	if (ret)
+		return ret;
 
-error_remove_trigger:
-	ad_sd_cleanup_buffer_and_trigger(indio_dev);
-error_clk_disable_unprepare:
-	clk_disable_unprepare(st->mclk);
+	ret = devm_iio_device_register(&spi->dev, indio_dev);
+	if (ret)
+		dev_err(&spi->dev, "Failed to register iio device\n");
 
 	return ret;
-}
 
-static int ad7124_remove(struct spi_device *spi)
-{
-	struct iio_dev *indio_dev = spi_get_drvdata(spi);
-	struct ad7124_state *st = iio_priv(indio_dev);
-
-	iio_device_unregister(indio_dev);
-	ad_sd_cleanup_buffer_and_trigger(indio_dev);
-	clk_disable_unprepare(st->mclk);
-
-	return 0;
 }
 
 static const struct of_device_id ad7124_of_match[] = {
@@ -838,7 +834,6 @@ static struct spi_driver ad71124_driver = {
 		.of_match_table = ad7124_of_match,
 	},
 	.probe = ad7124_probe,
-	.remove	= ad7124_remove,
 };
 module_spi_driver(ad71124_driver);
 
-- 
2.31.1


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

* Re: [PATCH 1/3] iio: adc: ad7124: Fix missbalanced regulator enable / disable on error.
  2021-05-08 18:23 ` [PATCH 1/3] iio: adc: ad7124: Fix missbalanced regulator enable / disable on error Jonathan Cameron
@ 2021-05-09  7:20   ` Alexandru Ardelean
  0 siblings, 0 replies; 9+ messages in thread
From: Alexandru Ardelean @ 2021-05-09  7:20 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-iio, Lars-Peter Clausen, alexandru.tachici, Jonathan Cameron

On Sat, May 8, 2021 at 9:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> If the devm_regulator_get() call succeeded but not the regulator_enable()
> then regulator_disable() would be called on a regulator that was not
> enabled.
>
> Fix this by moving regulator enabling / disabling over to
> devm_ management via devm_add_action_or_reset.
>

Definitely looks unbalanced.
This was probably written in the days when I wasn't familiar with devm_ much.
I probably would have complained about this.

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>

> Fixes: b3af341bbd96 ("iio: adc: Add ad7124 support")
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Alexandru Ardelean <ardeleanalex@gmail.com>
> ---
>  drivers/iio/adc/ad7124.c | 29 +++++++++++++----------------
>  1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 766c73333604..c0d0870a29ff 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -707,6 +707,11 @@ static int ad7124_setup(struct ad7124_state *st)
>         return ret;
>  }
>
> +static void ad7124_reg_disable(void *r)
> +{
> +       regulator_disable(r);
> +}
> +
>  static int ad7124_probe(struct spi_device *spi)
>  {
>         const struct ad7124_chip_info *info;
> @@ -752,17 +757,20 @@ static int ad7124_probe(struct spi_device *spi)
>                 ret = regulator_enable(st->vref[i]);
>                 if (ret)
>                         return ret;
> +
> +               ret = devm_add_action_or_reset(&spi->dev, ad7124_reg_disable,
> +                                              st->vref[i]);
> +               if (ret)
> +                       return ret;
>         }
>
>         st->mclk = devm_clk_get(&spi->dev, "mclk");
> -       if (IS_ERR(st->mclk)) {
> -               ret = PTR_ERR(st->mclk);
> -               goto error_regulator_disable;
> -       }
> +       if (IS_ERR(st->mclk))
> +               return PTR_ERR(st->mclk);
>
>         ret = clk_prepare_enable(st->mclk);
>         if (ret < 0)
> -               goto error_regulator_disable;
> +               return ret;
>
>         ret = ad7124_soft_reset(st);
>         if (ret < 0)
> @@ -792,11 +800,6 @@ static int ad7124_probe(struct spi_device *spi)
>         ad_sd_cleanup_buffer_and_trigger(indio_dev);
>  error_clk_disable_unprepare:
>         clk_disable_unprepare(st->mclk);
> -error_regulator_disable:
> -       for (i = ARRAY_SIZE(st->vref) - 1; i >= 0; i--) {
> -               if (!IS_ERR_OR_NULL(st->vref[i]))
> -                       regulator_disable(st->vref[i]);
> -       }
>
>         return ret;
>  }
> @@ -805,17 +808,11 @@ static int ad7124_remove(struct spi_device *spi)
>  {
>         struct iio_dev *indio_dev = spi_get_drvdata(spi);
>         struct ad7124_state *st = iio_priv(indio_dev);
> -       int i;
>
>         iio_device_unregister(indio_dev);
>         ad_sd_cleanup_buffer_and_trigger(indio_dev);
>         clk_disable_unprepare(st->mclk);
>
> -       for (i = ARRAY_SIZE(st->vref) - 1; i >= 0; i--) {
> -               if (!IS_ERR_OR_NULL(st->vref[i]))
> -                       regulator_disable(st->vref[i]);
> -       }
> -
>         return 0;
>  }
>
> --
> 2.31.1
>

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

* Re: [PATCH 2/3] iio: adc: ad7124: Fix potential overflow due to non sequential channel numbers
  2021-05-08 18:23 ` [PATCH 2/3] iio: adc: ad7124: Fix potential overflow due to non sequential channel numbers Jonathan Cameron
@ 2021-05-09  7:22   ` Alexandru Ardelean
  0 siblings, 0 replies; 9+ messages in thread
From: Alexandru Ardelean @ 2021-05-09  7:22 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-iio, Lars-Peter Clausen, alexandru.tachici, Jonathan Cameron

On Sat, May 8, 2021 at 9:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Channel numbering must start at 0 and then not have any holes, or
> it is possible to overflow the available storage.  Note this bug was
> introduced as part of a fix to ensure we didn't rely on the ordering
> of child nodes.  So we need to support arbitrary ordering but they all
> need to be there somewhere.
>
> Note I hit this when using qemu to test the rest of this series.
> Arguably this isn't the best fix, but it is probably the most minimal
> option for backporting etc.
>

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>

> Fixes: d7857e4ee1ba6 ("iio: adc: ad7124: Fix DT channel configuration")
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/iio/adc/ad7124.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index c0d0870a29ff..9c2401c5848e 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -616,6 +616,13 @@ static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
>                 if (ret)
>                         goto err;
>
> +               if (channel >= indio_dev->num_channels) {
> +                       dev_err(indio_dev->dev.parent,
> +                               "Channel index >= number of channels\n");
> +                       ret = -EINVAL;
> +                       goto err;
> +               }
> +
>                 ret = of_property_read_u32_array(child, "diff-channels",
>                                                  ain, 2);
>                 if (ret)
> --
> 2.31.1
>

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

* Re: [PATCH 3/3] iio: adc: ad7124: Use devm_ managed calls for all of probe() + drop remove()
  2021-05-08 18:23 ` [PATCH 3/3] iio: adc: ad7124: Use devm_ managed calls for all of probe() + drop remove() Jonathan Cameron
@ 2021-05-09  7:30   ` Alexandru Ardelean
  0 siblings, 0 replies; 9+ messages in thread
From: Alexandru Ardelean @ 2021-05-09  7:30 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-iio, Lars-Peter Clausen, alexandru.tachici, Jonathan Cameron

On Sat, May 8, 2021 at 9:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> As not many steps were not already devm_ managed, use
> devm_add_action_or_reset() to handle the rest.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Cc: Alexandru Ardelean <ardeleanalex@gmail.com>

double Cc: tag here

One minor thought about maybe removing the log spam.
Other than that:

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>


> ---
>  drivers/iio/adc/ad7124.c | 53 ++++++++++++++++++----------------------
>  1 file changed, 24 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 9c2401c5848e..deb166d2b645 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -719,6 +719,16 @@ static void ad7124_reg_disable(void *r)
>         regulator_disable(r);
>  }
>
> +static void ad7124_clk_disable(void *c)
> +{
> +       clk_disable_unprepare(c);
> +}
> +
> +static void ad7124_buffer_cleanup(void *idev)
> +{
> +       ad_sd_cleanup_buffer_and_trigger(idev);

We never seem to have done a devm_ version for ad_sd_setup_buffer_and_trigger().


> +}
> +
>  static int ad7124_probe(struct spi_device *spi)
>  {
>         const struct ad7124_chip_info *info;
> @@ -740,8 +750,6 @@ static int ad7124_probe(struct spi_device *spi)
>
>         ad_sd_init(&st->sd, indio_dev, spi, &ad7124_sigma_delta_info);
>
> -       spi_set_drvdata(spi, indio_dev);
> -
>         indio_dev->name = st->chip_info->name;
>         indio_dev->modes = INDIO_DIRECT_MODE;
>         indio_dev->info = &ad7124_info;
> @@ -779,48 +787,36 @@ static int ad7124_probe(struct spi_device *spi)
>         if (ret < 0)
>                 return ret;
>
> +       ret = devm_add_action_or_reset(&spi->dev, ad7124_clk_disable, st->mclk);
> +       if (ret)
> +               return ret;
> +
>         ret = ad7124_soft_reset(st);
>         if (ret < 0)
> -               goto error_clk_disable_unprepare;
> +               return ret;
>
>         ret = ad7124_check_chip_id(st);
>         if (ret)
> -               goto error_clk_disable_unprepare;
> +               return ret;
>
>         ret = ad7124_setup(st);
>         if (ret < 0)
> -               goto error_clk_disable_unprepare;
> +               return ret;
>
>         ret = ad_sd_setup_buffer_and_trigger(indio_dev);
>         if (ret < 0)
> -               goto error_clk_disable_unprepare;
> -
> -       ret = iio_device_register(indio_dev);
> -       if (ret < 0) {
> -               dev_err(&spi->dev, "Failed to register iio device\n");
> -               goto error_remove_trigger;
> -       }
> +               return ret;
>
> -       return 0;
> +       ret = devm_add_action_or_reset(&spi->dev, ad7124_buffer_cleanup, indio_dev);
> +       if (ret)
> +               return ret;
>
> -error_remove_trigger:
> -       ad_sd_cleanup_buffer_and_trigger(indio_dev);
> -error_clk_disable_unprepare:
> -       clk_disable_unprepare(st->mclk);
> +       ret = devm_iio_device_register(&spi->dev, indio_dev);
> +       if (ret)
> +               dev_err(&spi->dev, "Failed to register iio device\n");

I guess Andy may come along and suggest that we remove this log spam
and just do a direct return.
Which would sound like a reasonable idea.

>
>         return ret;
> -}
>
> -static int ad7124_remove(struct spi_device *spi)
> -{
> -       struct iio_dev *indio_dev = spi_get_drvdata(spi);
> -       struct ad7124_state *st = iio_priv(indio_dev);
> -
> -       iio_device_unregister(indio_dev);
> -       ad_sd_cleanup_buffer_and_trigger(indio_dev);
> -       clk_disable_unprepare(st->mclk);
> -
> -       return 0;
>  }
>
>  static const struct of_device_id ad7124_of_match[] = {
> @@ -838,7 +834,6 @@ static struct spi_driver ad71124_driver = {
>                 .of_match_table = ad7124_of_match,
>         },
>         .probe = ad7124_probe,
> -       .remove = ad7124_remove,
>  };
>  module_spi_driver(ad71124_driver);
>
> --
> 2.31.1
>

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

* Re: [PATCH 0/3] iio: adc: ad7124: Fixes and devm_ for all of probe
  2021-05-08 18:23 [PATCH 0/3] iio: adc: ad7124: Fixes and devm_ for all of probe Jonathan Cameron
                   ` (2 preceding siblings ...)
  2021-05-08 18:23 ` [PATCH 3/3] iio: adc: ad7124: Use devm_ managed calls for all of probe() + drop remove() Jonathan Cameron
@ 2021-05-09  7:31 ` Alexandru Ardelean
  2021-05-09  9:35   ` Jonathan Cameron
  3 siblings, 1 reply; 9+ messages in thread
From: Alexandru Ardelean @ 2021-05-09  7:31 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: linux-iio, Lars-Peter Clausen, alexandru.tachici, Jonathan Cameron

On Sat, May 8, 2021 at 9:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> I noticed an issue around regulator error handling and managed to
> hit another with my hacked together test setup.  Hence let's fix
> those two issues first then we might as well follow up by converting
> the last few bits of this driver to use device managed functions so
> we can simplify the error handling and cleanup.
>
> Testing conducted with QEMU hacking and insertion of errors at relevant
> locations in the code.

I guess the devm_ cleanup didn't catch-on much with others.
It means I'll probably resume it :)

>
> Jonathan Cameron (3):
>   iio: adc: ad7124: Fix missbalanced regulator enable / disable on
>     error.
>   iio: adc: ad7124: Fix potential overflow due to non sequential channel
>     numbers
>   iio: adc: ad7124: Use devm_ managed calls for all of probe() + drop
>     remove()
>
>  drivers/iio/adc/ad7124.c | 89 ++++++++++++++++++++--------------------
>  1 file changed, 44 insertions(+), 45 deletions(-)
>
> --
> 2.31.1
>

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

* Re: [PATCH 0/3] iio: adc: ad7124: Fixes and devm_ for all of probe
  2021-05-09  7:31 ` [PATCH 0/3] iio: adc: ad7124: Fixes and devm_ for all of probe Alexandru Ardelean
@ 2021-05-09  9:35   ` Jonathan Cameron
  0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2021-05-09  9:35 UTC (permalink / raw)
  To: Alexandru Ardelean
  Cc: linux-iio, Lars-Peter Clausen, alexandru.tachici, Jonathan Cameron

On Sun, 9 May 2021 10:31:59 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Sat, May 8, 2021 at 9:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > I noticed an issue around regulator error handling and managed to
> > hit another with my hacked together test setup.  Hence let's fix
> > those two issues first then we might as well follow up by converting
> > the last few bits of this driver to use device managed functions so
> > we can simplify the error handling and cleanup.
> >
> > Testing conducted with QEMU hacking and insertion of errors at relevant
> > locations in the code.  
> 
> I guess the devm_ cleanup didn't catch-on much with others.
> It means I'll probably resume it :)

It's a fun one for when you have 20 mins left at the end of a day.

Though less fun when you then hit a random memory corruption bug and
loose an hour :(

Jonathan

> 
> >
> > Jonathan Cameron (3):
> >   iio: adc: ad7124: Fix missbalanced regulator enable / disable on
> >     error.
> >   iio: adc: ad7124: Fix potential overflow due to non sequential channel
> >     numbers
> >   iio: adc: ad7124: Use devm_ managed calls for all of probe() + drop
> >     remove()
> >
> >  drivers/iio/adc/ad7124.c | 89 ++++++++++++++++++++--------------------
> >  1 file changed, 44 insertions(+), 45 deletions(-)
> >
> > --
> > 2.31.1
> >  


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

end of thread, other threads:[~2021-05-09  9:34 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-08 18:23 [PATCH 0/3] iio: adc: ad7124: Fixes and devm_ for all of probe Jonathan Cameron
2021-05-08 18:23 ` [PATCH 1/3] iio: adc: ad7124: Fix missbalanced regulator enable / disable on error Jonathan Cameron
2021-05-09  7:20   ` Alexandru Ardelean
2021-05-08 18:23 ` [PATCH 2/3] iio: adc: ad7124: Fix potential overflow due to non sequential channel numbers Jonathan Cameron
2021-05-09  7:22   ` Alexandru Ardelean
2021-05-08 18:23 ` [PATCH 3/3] iio: adc: ad7124: Use devm_ managed calls for all of probe() + drop remove() Jonathan Cameron
2021-05-09  7:30   ` Alexandru Ardelean
2021-05-09  7:31 ` [PATCH 0/3] iio: adc: ad7124: Fixes and devm_ for all of probe Alexandru Ardelean
2021-05-09  9:35   ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).