| Message ID | 20230313185333.2776785-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] iio: frequency: admv1013: Benefit from devm_clk_get_enabled() to simplify | expand |
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);
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);
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(-)