[PATCH v2] iio: frequency: admv1013: Benefit from devm_clk_get_enabled() to simplify

[PATCH v2] iio: frequency: admv1013: Benefit from devm_clk_get_enabled() to simplify

[Uwe Kleine-König]

Make use of devm_clk_get_enabled() to replace some code that effectively
open codes this new function.

To retain ordering move the request to a place that is executed later.
This way the time of enable keeps the same.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

(implicit) v1 of this patch was part of a bigger series some time
ago[1].

In v1 the call to devm_clk_get_enabled() was where devm_clk_get used to
be. Jonathan had the valid concern that this changes ordering which
might introduce subtle regressions. Andy suggested to move the call to
the later place.

Best regards
Uwe

[1] https://lore.kernel.org/linux-iio/20220808204740.307667-11-u.kleine-koenig@pengutronix.de

 drivers/iio/frequency/admv1013.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/frequency/admv1013.c b/drivers/iio/frequency/admv1013.c
index ed8167271358..9bf8337806fc 100644
--- a/drivers/iio/frequency/admv1013.c
+++ b/drivers/iio/frequency/admv1013.c
@@ -490,11 +490,6 @@ static int admv1013_init(struct admv1013_state *st)
 					  st->input_mode);
 }
 
-static void admv1013_clk_disable(void *data)
-{
-	clk_disable_unprepare(data);
-}
-
 static void admv1013_reg_disable(void *data)
 {
 	regulator_disable(data);
@@ -559,11 +554,6 @@ static int admv1013_properties_parse(struct admv1013_state *st)
 		return dev_err_probe(&spi->dev, PTR_ERR(st->reg),
 				     "failed to get the common-mode voltage\n");
 
-	st->clkin = devm_clk_get(&spi->dev, "lo_in");
-	if (IS_ERR(st->clkin))
-		return dev_err_probe(&spi->dev, PTR_ERR(st->clkin),
-				     "failed to get the LO input clock\n");
-
 	return 0;
 }
 
@@ -601,13 +591,10 @@ static int admv1013_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	ret = clk_prepare_enable(st->clkin);
-	if (ret)
-		return ret;
-
-	ret = devm_add_action_or_reset(&spi->dev, admv1013_clk_disable, st->clkin);
-	if (ret)
-		return ret;
+	st->clkin = devm_clk_get_enabled(&spi->dev, "lo_in");
+	if (IS_ERR(st->clkin))
+		return dev_err_probe(&spi->dev, PTR_ERR(st->clkin),
+				     "failed to get the LO input clock\n");
 
 	st->nb.notifier_call = admv1013_freq_change;
 	ret = devm_clk_notifier_register(&spi->dev, st->clkin, &st->nb);
-- 
2.39.1

Re: [PATCH v2] iio: frequency: admv1013: Benefit from devm_clk_get_enabled() to simplify

[Jonathan Cameron]

On Mon, 13 Mar 2023 19:53:33 +0100
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> Make use of devm_clk_get_enabled() to replace some code that effectively
> open codes this new function.
> 
> To retain ordering move the request to a place that is executed later.
> This way the time of enable keeps the same.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Looks good to me.  If we were reviewing this driver for the first time
I'd be tempted to also pull the regulator get out of the
properties parsing function on basis that neither that nor this clk
retrieval are as simple as parsing the firmware.

Probably not worth making that shuffle now though - people can cope
with the small dissonance :) 

Applied to the togreg branch of iio.git and pushed out as testing for
0-day to take a poke at it.  Still time for others to comment though
as I will rebase it if any comments coming before I push it out as togreg
(probably end of next week)

Thanks,

Jonathan

> ---
> Hello,
> 
> (implicit) v1 of this patch was part of a bigger series some time
> ago[1].
> 
> In v1 the call to devm_clk_get_enabled() was where devm_clk_get used to
> be. Jonathan had the valid concern that this changes ordering which
> might introduce subtle regressions. Andy suggested to move the call to
> the later place.
> 
> Best regards
> Uwe
> 
> [1] https://lore.kernel.org/linux-iio/20220808204740.307667-11-u.kleine-koenig@pengutronix.de
> 
>  drivers/iio/frequency/admv1013.c | 21 ++++-----------------
>  1 file changed, 4 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/frequency/admv1013.c b/drivers/iio/frequency/admv1013.c
> index ed8167271358..9bf8337806fc 100644
> --- a/drivers/iio/frequency/admv1013.c
> +++ b/drivers/iio/frequency/admv1013.c
> @@ -490,11 +490,6 @@ static int admv1013_init(struct admv1013_state *st)
>  					  st->input_mode);
>  }
>  
> -static void admv1013_clk_disable(void *data)
> -{
> -	clk_disable_unprepare(data);
> -}
> -
>  static void admv1013_reg_disable(void *data)
>  {
>  	regulator_disable(data);
> @@ -559,11 +554,6 @@ static int admv1013_properties_parse(struct admv1013_state *st)
>  		return dev_err_probe(&spi->dev, PTR_ERR(st->reg),
>  				     "failed to get the common-mode voltage\n");
>  
> -	st->clkin = devm_clk_get(&spi->dev, "lo_in");
> -	if (IS_ERR(st->clkin))
> -		return dev_err_probe(&spi->dev, PTR_ERR(st->clkin),
> -				     "failed to get the LO input clock\n");
> -
>  	return 0;
>  }
>  
> @@ -601,13 +591,10 @@ static int admv1013_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
>  
> -	ret = clk_prepare_enable(st->clkin);
> -	if (ret)
> -		return ret;
> -
> -	ret = devm_add_action_or_reset(&spi->dev, admv1013_clk_disable, st->clkin);
> -	if (ret)
> -		return ret;
> +	st->clkin = devm_clk_get_enabled(&spi->dev, "lo_in");
> +	if (IS_ERR(st->clkin))
> +		return dev_err_probe(&spi->dev, PTR_ERR(st->clkin),
> +				     "failed to get the LO input clock\n");
>  
>  	st->nb.notifier_call = admv1013_freq_change;
>  	ret = devm_clk_notifier_register(&spi->dev, st->clkin, &st->nb);