[PATCH -next v3] iio: frequency: adf4350: Use device managed functions and fix power down issue.

[PATCH -next v3] iio: frequency: adf4350: Use device managed functions and fix power down issue.

[Jinjie Ruan]

The devm_clk_get_enabled() helper:
    - calls devm_clk_get()
    - calls clk_prepare_enable() and registers what is needed in order to
      call clk_disable_unprepare() when needed, as a managed resource.

Also replace devm_regulator_get() and regulator_enable() with
devm_regulator_get_enable() helper and remove regulator_disable().

Replace iio_device_register() with devm_iio_device_register() and remove
iio_device_unregister().

And st->reg is not used anymore, so remove it.

As Jonathan pointed out, couple of things that are wrong:

1) The device is powered down 'before' we unregister it with the
   subsystem and as such userspace interfaces are still exposed which
   probably won't do the right thing if the chip is powered down.

2) This isn't done in the error paths in probe.

To solve this problem, register a new callback adf4350_power_down()
with devm_add_action_or_reset(), to enable software power down in both
error and device detach path. So the remove function can be removed.

Remove spi_set_drvdata() from the probe function, since spi_get_drvdata()
is not used anymore.

Fixes: e31166f0fd48 ("iio: frequency: New driver for Analog Devices ADF4350/ADF4351 Wideband Synthesizers")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Suggested-by: Jonathan Cameron <jic23@kernel.org>
---
v3:
- Add a device managed hook to enable software power down.
- Remove the remove function.
- Remove the spi_set_drvdata() call in probe function.
- Add the fix tag.
- Update the commit message and title.
v2:
- Also use devm_regulator_get_enable() and devm_iio_device_register().
- Update the commit message and title.
---
 drivers/iio/frequency/adf4350.c | 77 +++++++++++----------------------
 1 file changed, 26 insertions(+), 51 deletions(-)

diff --git a/drivers/iio/frequency/adf4350.c b/drivers/iio/frequency/adf4350.c
index 85e289700c3c..2c17110f3b61 100644
--- a/drivers/iio/frequency/adf4350.c
+++ b/drivers/iio/frequency/adf4350.c
@@ -33,7 +33,6 @@ enum {
 
 struct adf4350_state {
 	struct spi_device		*spi;
-	struct regulator		*reg;
 	struct gpio_desc		*lock_detect_gpiod;
 	struct adf4350_platform_data	*pdata;
 	struct clk			*clk;
@@ -469,6 +468,15 @@ static struct adf4350_platform_data *adf4350_parse_dt(struct device *dev)
 	return pdata;
 }
 
+static void adf4350_power_down(void *data)
+{
+	struct iio_dev *indio_dev = data;
+	struct adf4350_state *st = iio_priv(indio_dev);
+
+	st->regs[ADF4350_REG2] |= ADF4350_REG2_POWER_DOWN_EN;
+	adf4350_sync_config(st);
+}
+
 static int adf4350_probe(struct spi_device *spi)
 {
 	struct adf4350_platform_data *pdata;
@@ -491,31 +499,21 @@ static int adf4350_probe(struct spi_device *spi)
 	}
 
 	if (!pdata->clkin) {
-		clk = devm_clk_get(&spi->dev, "clkin");
+		clk = devm_clk_get_enabled(&spi->dev, "clkin");
 		if (IS_ERR(clk))
-			return -EPROBE_DEFER;
-
-		ret = clk_prepare_enable(clk);
-		if (ret < 0)
-			return ret;
+			return PTR_ERR(clk);
 	}
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
-	if (indio_dev == NULL) {
-		ret =  -ENOMEM;
-		goto error_disable_clk;
-	}
+	if (indio_dev == NULL)
+		return -ENOMEM;
 
 	st = iio_priv(indio_dev);
 
-	st->reg = devm_regulator_get(&spi->dev, "vcc");
-	if (!IS_ERR(st->reg)) {
-		ret = regulator_enable(st->reg);
-		if (ret)
-			goto error_disable_clk;
-	}
+	ret = devm_regulator_get_enable(&spi->dev, "vcc");
+	if (ret)
+		return ret;
 
-	spi_set_drvdata(spi, indio_dev);
 	st->spi = spi;
 	st->pdata = pdata;
 
@@ -544,47 +542,25 @@ static int adf4350_probe(struct spi_device *spi)
 
 	st->lock_detect_gpiod = devm_gpiod_get_optional(&spi->dev, NULL,
 							GPIOD_IN);
-	if (IS_ERR(st->lock_detect_gpiod)) {
-		ret = PTR_ERR(st->lock_detect_gpiod);
-		goto error_disable_reg;
-	}
+	if (IS_ERR(st->lock_detect_gpiod))
+		return PTR_ERR(st->lock_detect_gpiod);
 
 	if (pdata->power_up_frequency) {
 		ret = adf4350_set_freq(st, pdata->power_up_frequency);
 		if (ret)
-			goto error_disable_reg;
+			return ret;
 	}
 
-	ret = iio_device_register(indio_dev);
+	ret = devm_add_action_or_reset(&spi->dev, adf4350_power_down, indio_dev);
 	if (ret)
-		goto error_disable_reg;
+		return dev_err_probe(&spi->dev, ret,
+				     "Failed to add action to managed power down\n");
 
-	return 0;
-
-error_disable_reg:
-	if (!IS_ERR(st->reg))
-		regulator_disable(st->reg);
-error_disable_clk:
-	clk_disable_unprepare(clk);
-
-	return ret;
-}
-
-static void adf4350_remove(struct spi_device *spi)
-{
-	struct iio_dev *indio_dev = spi_get_drvdata(spi);
-	struct adf4350_state *st = iio_priv(indio_dev);
-	struct regulator *reg = st->reg;
-
-	st->regs[ADF4350_REG2] |= ADF4350_REG2_POWER_DOWN_EN;
-	adf4350_sync_config(st);
-
-	iio_device_unregister(indio_dev);
-
-	clk_disable_unprepare(st->clk);
+	ret = devm_iio_device_register(&spi->dev, indio_dev);
+	if (ret)
+		return ret;
 
-	if (!IS_ERR(reg))
-		regulator_disable(reg);
+	return 0;
 }
 
 static const struct of_device_id adf4350_of_match[] = {
@@ -607,7 +583,6 @@ static struct spi_driver adf4350_driver = {
 		.of_match_table = adf4350_of_match,
 	},
 	.probe		= adf4350_probe,
-	.remove		= adf4350_remove,
 	.id_table	= adf4350_id,
 };
 module_spi_driver(adf4350_driver);
-- 
2.34.1

Re: [PATCH -next v3] iio: frequency: adf4350: Use device managed functions and fix power down issue.

[Jonathan Cameron]

On Mon, 28 Aug 2023 14:27:16 +0800
Jinjie Ruan <ruanjinjie@huawei.com> wrote:

> The devm_clk_get_enabled() helper:
>     - calls devm_clk_get()
>     - calls clk_prepare_enable() and registers what is needed in order to
>       call clk_disable_unprepare() when needed, as a managed resource.
> 
> Also replace devm_regulator_get() and regulator_enable() with
> devm_regulator_get_enable() helper and remove regulator_disable().
> 
> Replace iio_device_register() with devm_iio_device_register() and remove
> iio_device_unregister().
> 
> And st->reg is not used anymore, so remove it.
> 
> As Jonathan pointed out, couple of things that are wrong:
> 
> 1) The device is powered down 'before' we unregister it with the
>    subsystem and as such userspace interfaces are still exposed which
>    probably won't do the right thing if the chip is powered down.
> 
> 2) This isn't done in the error paths in probe.
> 
> To solve this problem, register a new callback adf4350_power_down()
> with devm_add_action_or_reset(), to enable software power down in both
> error and device detach path. So the remove function can be removed.
> 
> Remove spi_set_drvdata() from the probe function, since spi_get_drvdata()
> is not used anymore.
> 
> Fixes: e31166f0fd48 ("iio: frequency: New driver for Analog Devices ADF4350/ADF4351 Wideband Synthesizers")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
Applied to the togreg branch of iio.git but only pushed out as testing for now
as I will be rebasing on rc1.  As such these won't appear in linux-next until
after the merge window has closed.

Thanks,

Jonathan

> ---
> v3:
> - Add a device managed hook to enable software power down.
> - Remove the remove function.
> - Remove the spi_set_drvdata() call in probe function.
> - Add the fix tag.
> - Update the commit message and title.
> v2:
> - Also use devm_regulator_get_enable() and devm_iio_device_register().
> - Update the commit message and title.
> ---
>  drivers/iio/frequency/adf4350.c | 77 +++++++++++----------------------
>  1 file changed, 26 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/iio/frequency/adf4350.c b/drivers/iio/frequency/adf4350.c
> index 85e289700c3c..2c17110f3b61 100644
> --- a/drivers/iio/frequency/adf4350.c
> +++ b/drivers/iio/frequency/adf4350.c
> @@ -33,7 +33,6 @@ enum {
>  
>  struct adf4350_state {
>  	struct spi_device		*spi;
> -	struct regulator		*reg;
>  	struct gpio_desc		*lock_detect_gpiod;
>  	struct adf4350_platform_data	*pdata;
>  	struct clk			*clk;
> @@ -469,6 +468,15 @@ static struct adf4350_platform_data *adf4350_parse_dt(struct device *dev)
>  	return pdata;
>  }
>  
> +static void adf4350_power_down(void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +	struct adf4350_state *st = iio_priv(indio_dev);
> +
> +	st->regs[ADF4350_REG2] |= ADF4350_REG2_POWER_DOWN_EN;
> +	adf4350_sync_config(st);
> +}
> +
>  static int adf4350_probe(struct spi_device *spi)
>  {
>  	struct adf4350_platform_data *pdata;
> @@ -491,31 +499,21 @@ static int adf4350_probe(struct spi_device *spi)
>  	}
>  
>  	if (!pdata->clkin) {
> -		clk = devm_clk_get(&spi->dev, "clkin");
> +		clk = devm_clk_get_enabled(&spi->dev, "clkin");
>  		if (IS_ERR(clk))
> -			return -EPROBE_DEFER;
> -
> -		ret = clk_prepare_enable(clk);
> -		if (ret < 0)
> -			return ret;
> +			return PTR_ERR(clk);
>  	}
>  
>  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> -	if (indio_dev == NULL) {
> -		ret =  -ENOMEM;
> -		goto error_disable_clk;
> -	}
> +	if (indio_dev == NULL)
> +		return -ENOMEM;
>  
>  	st = iio_priv(indio_dev);
>  
> -	st->reg = devm_regulator_get(&spi->dev, "vcc");
> -	if (!IS_ERR(st->reg)) {
> -		ret = regulator_enable(st->reg);
> -		if (ret)
> -			goto error_disable_clk;
> -	}
> +	ret = devm_regulator_get_enable(&spi->dev, "vcc");
> +	if (ret)
> +		return ret;
>  
> -	spi_set_drvdata(spi, indio_dev);
>  	st->spi = spi;
>  	st->pdata = pdata;
>  
> @@ -544,47 +542,25 @@ static int adf4350_probe(struct spi_device *spi)
>  
>  	st->lock_detect_gpiod = devm_gpiod_get_optional(&spi->dev, NULL,
>  							GPIOD_IN);
> -	if (IS_ERR(st->lock_detect_gpiod)) {
> -		ret = PTR_ERR(st->lock_detect_gpiod);
> -		goto error_disable_reg;
> -	}
> +	if (IS_ERR(st->lock_detect_gpiod))
> +		return PTR_ERR(st->lock_detect_gpiod);
>  
>  	if (pdata->power_up_frequency) {
>  		ret = adf4350_set_freq(st, pdata->power_up_frequency);
>  		if (ret)
> -			goto error_disable_reg;
> +			return ret;
>  	}
>  
> -	ret = iio_device_register(indio_dev);
> +	ret = devm_add_action_or_reset(&spi->dev, adf4350_power_down, indio_dev);
>  	if (ret)
> -		goto error_disable_reg;
> +		return dev_err_probe(&spi->dev, ret,
> +				     "Failed to add action to managed power down\n");
>  
> -	return 0;
> -
> -error_disable_reg:
> -	if (!IS_ERR(st->reg))
> -		regulator_disable(st->reg);
> -error_disable_clk:
> -	clk_disable_unprepare(clk);
> -
> -	return ret;
> -}
> -
> -static void adf4350_remove(struct spi_device *spi)
> -{
> -	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> -	struct adf4350_state *st = iio_priv(indio_dev);
> -	struct regulator *reg = st->reg;
> -
> -	st->regs[ADF4350_REG2] |= ADF4350_REG2_POWER_DOWN_EN;
> -	adf4350_sync_config(st);
> -
> -	iio_device_unregister(indio_dev);
> -
> -	clk_disable_unprepare(st->clk);
> +	ret = devm_iio_device_register(&spi->dev, indio_dev);
> +	if (ret)
> +		return ret;
>  
> -	if (!IS_ERR(reg))
> -		regulator_disable(reg);
> +	return 0;
>  }
>  
>  static const struct of_device_id adf4350_of_match[] = {
> @@ -607,7 +583,6 @@ static struct spi_driver adf4350_driver = {
>  		.of_match_table = adf4350_of_match,
>  	},
>  	.probe		= adf4350_probe,
> -	.remove		= adf4350_remove,
>  	.id_table	= adf4350_id,
>  };
>  module_spi_driver(adf4350_driver);

Re: [PATCH -next v3] iio: frequency: adf4350: Use device managed functions and fix power down issue.

[Jonathan Cameron]

On Mon, 28 Aug 2023 14:27:16 +0800
Jinjie Ruan <ruanjinjie@huawei.com> wrote:

> The devm_clk_get_enabled() helper:
>     - calls devm_clk_get()
>     - calls clk_prepare_enable() and registers what is needed in order to
>       call clk_disable_unprepare() when needed, as a managed resource.
> 
> Also replace devm_regulator_get() and regulator_enable() with
> devm_regulator_get_enable() helper and remove regulator_disable().
> 
> Replace iio_device_register() with devm_iio_device_register() and remove
> iio_device_unregister().
> 
> And st->reg is not used anymore, so remove it.
> 
> As Jonathan pointed out, couple of things that are wrong:
> 
> 1) The device is powered down 'before' we unregister it with the
>    subsystem and as such userspace interfaces are still exposed which
>    probably won't do the right thing if the chip is powered down.
> 
> 2) This isn't done in the error paths in probe.
> 
> To solve this problem, register a new callback adf4350_power_down()
> with devm_add_action_or_reset(), to enable software power down in both
> error and device detach path. So the remove function can be removed.
> 
> Remove spi_set_drvdata() from the probe function, since spi_get_drvdata()
> is not used anymore.
> 
> Fixes: e31166f0fd48 ("iio: frequency: New driver for Analog Devices ADF4350/ADF4351 Wideband Synthesizers")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> ---
> v3:
> - Add a device managed hook to enable software power down.
> - Remove the remove function.
> - Remove the spi_set_drvdata() call in probe function.
> - Add the fix tag.
> - Update the commit message and title.
> v2:
> - Also use devm_regulator_get_enable() and devm_iio_device_register().
> - Update the commit message and title.
> ---
>  drivers/iio/frequency/adf4350.c | 77 +++++++++++----------------------
>  1 file changed, 26 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/iio/frequency/adf4350.c b/drivers/iio/frequency/adf4350.c
> index 85e289700c3c..2c17110f3b61 100644
> --- a/drivers/iio/frequency/adf4350.c
> +++ b/drivers/iio/frequency/adf4350.c
> @@ -33,7 +33,6 @@ enum {
>  
>  struct adf4350_state {
>  	struct spi_device		*spi;
> -	struct regulator		*reg;
>  	struct gpio_desc		*lock_detect_gpiod;
>  	struct adf4350_platform_data	*pdata;
>  	struct clk			*clk;
> @@ -469,6 +468,15 @@ static struct adf4350_platform_data *adf4350_parse_dt(struct device *dev)
>  	return pdata;
>  }
>  
> +static void adf4350_power_down(void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +	struct adf4350_state *st = iio_priv(indio_dev);
> +
> +	st->regs[ADF4350_REG2] |= ADF4350_REG2_POWER_DOWN_EN;
> +	adf4350_sync_config(st);
> +}
> +
>  static int adf4350_probe(struct spi_device *spi)
>  {
>  	struct adf4350_platform_data *pdata;
> @@ -491,31 +499,21 @@ static int adf4350_probe(struct spi_device *spi)
>  	}
>  
>  	if (!pdata->clkin) {
> -		clk = devm_clk_get(&spi->dev, "clkin");
> +		clk = devm_clk_get_enabled(&spi->dev, "clkin");
>  		if (IS_ERR(clk))
> -			return -EPROBE_DEFER;
> -
> -		ret = clk_prepare_enable(clk);
> -		if (ret < 0)
> -			return ret;
> +			return PTR_ERR(clk);
>  	}
>  
>  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> -	if (indio_dev == NULL) {
> -		ret =  -ENOMEM;
> -		goto error_disable_clk;
> -	}
> +	if (indio_dev == NULL)
> +		return -ENOMEM;
>  
>  	st = iio_priv(indio_dev);
>  
> -	st->reg = devm_regulator_get(&spi->dev, "vcc");
> -	if (!IS_ERR(st->reg)) {
> -		ret = regulator_enable(st->reg);
> -		if (ret)
> -			goto error_disable_clk;
> -	}
> +	ret = devm_regulator_get_enable(&spi->dev, "vcc");
> +	if (ret)
> +		return ret;
>  
> -	spi_set_drvdata(spi, indio_dev);
>  	st->spi = spi;
>  	st->pdata = pdata;
>  
> @@ -544,47 +542,25 @@ static int adf4350_probe(struct spi_device *spi)
>  
>  	st->lock_detect_gpiod = devm_gpiod_get_optional(&spi->dev, NULL,
>  							GPIOD_IN);
> -	if (IS_ERR(st->lock_detect_gpiod)) {
> -		ret = PTR_ERR(st->lock_detect_gpiod);
> -		goto error_disable_reg;
> -	}
> +	if (IS_ERR(st->lock_detect_gpiod))
> +		return PTR_ERR(st->lock_detect_gpiod);
>  
>  	if (pdata->power_up_frequency) {
>  		ret = adf4350_set_freq(st, pdata->power_up_frequency);
>  		if (ret)
> -			goto error_disable_reg;
> +			return ret;
>  	}
>  
> -	ret = iio_device_register(indio_dev);
> +	ret = devm_add_action_or_reset(&spi->dev, adf4350_power_down, indio_dev);
>  	if (ret)
> -		goto error_disable_reg;
> +		return dev_err_probe(&spi->dev, ret,
> +				     "Failed to add action to managed power down\n");
>  
> -	return 0;
> -
> -error_disable_reg:
> -	if (!IS_ERR(st->reg))
> -		regulator_disable(st->reg);
> -error_disable_clk:
> -	clk_disable_unprepare(clk);
> -
> -	return ret;
> -}
> -
> -static void adf4350_remove(struct spi_device *spi)
> -{
> -	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> -	struct adf4350_state *st = iio_priv(indio_dev);
> -	struct regulator *reg = st->reg;
> -
> -	st->regs[ADF4350_REG2] |= ADF4350_REG2_POWER_DOWN_EN;
> -	adf4350_sync_config(st);
> -
> -	iio_device_unregister(indio_dev);
> -
> -	clk_disable_unprepare(st->clk);
> +	ret = devm_iio_device_register(&spi->dev, indio_dev);
> +	if (ret)
> +		return ret;
>  
> -	if (!IS_ERR(reg))
> -		regulator_disable(reg);
> +	return 0;
Tiny tweak, which I'll make whilst applying.

	return devm_iio_device_register() is the same
and saves us a few lines.

One of the random static analysis scripts people run picks up on this
one so if I left it we would probably get a patch in a few weeks making the
change.  Easier to do it now :)

Jonathan

>  }
>  
>  static const struct of_device_id adf4350_of_match[] = {
> @@ -607,7 +583,6 @@ static struct spi_driver adf4350_driver = {
>  		.of_match_table = adf4350_of_match,
>  	},
>  	.probe		= adf4350_probe,
> -	.remove		= adf4350_remove,
>  	.id_table	= adf4350_id,
>  };
>  module_spi_driver(adf4350_driver);