| Message ID | 20220808204740.307667-11-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [01/13] iio: adc: ad7124: Benefit from devm_clk_get_enabled() to simplify | expand |
On Mon, 8 Aug 2022 22:47:38 +0200 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. > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Looks fine to me, but there is a subtle reordering + it does even more non parsing stuff in a function called _parse. Anyhow, would like Antoniu or someone else from ADI to take a quick look if possible before I pick this one up. > --- > drivers/iio/frequency/admv1013.c | 15 +-------------- > 1 file changed, 1 insertion(+), 14 deletions(-) > > diff --git a/drivers/iio/frequency/admv1013.c b/drivers/iio/frequency/admv1013.c > index b0e1f6571afb..1346d77df77f 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,7 +554,7 @@ 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"); > + 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"); > @@ -601,14 +596,6 @@ 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->nb.notifier_call = admv1013_freq_change; > ret = devm_clk_notifier_register(&spi->dev, st->clkin, &st->nb); > if (ret)
On Sat, Aug 13, 2022 at 7:26 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Mon, 8 Aug 2022 22:47:38 +0200 > 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. > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Looks fine to me, but there is a subtle reordering + it does even more > non parsing stuff in a function called _parse. > > Anyhow, would like Antoniu or someone else from ADI to take a quick look if > possible before I pick this one up. ... > > - st->clkin = devm_clk_get(&spi->dev, "lo_in"); > > + 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"); So it seems better to drop above and... > > - 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; ...put a call here. This will make parse() look more parse.
On Sun, 14 Aug 2022 22:04:35 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sat, Aug 13, 2022 at 7:26 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Mon, 8 Aug 2022 22:47:38 +0200 > > 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. > > > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Looks fine to me, but there is a subtle reordering + it does even more > > non parsing stuff in a function called _parse. > > > > Anyhow, would like Antoniu or someone else from ADI to take a quick look if > > possible before I pick this one up. > > ... > > > > - st->clkin = devm_clk_get(&spi->dev, "lo_in"); > > > + 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"); > > So it seems better to drop above and... > > > > - 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; > > ...put a call here. This will make parse() look more parse. > Agreed. I was thinking we actually did something (like get the rate) inbetween but there isn't anything like that. Whilst it's sort of true that a devm_clk_get_enabled() is part of the firmware parsing, that's also true of the regulators that are already outside that function. Hence the rearrangement Andy suggests makes sense to me. Thanks, Jonathan
diff --git a/drivers/iio/frequency/admv1013.c b/drivers/iio/frequency/admv1013.c index b0e1f6571afb..1346d77df77f 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,7 +554,7 @@ 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"); + 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"); @@ -601,14 +596,6 @@ 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->nb.notifier_call = admv1013_freq_change; ret = devm_clk_notifier_register(&spi->dev, st->clkin, &st->nb); if (ret)