On Sun, 14 Aug 2022 22:04:35 +0300 Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > On Sat, Aug 13, 2022 at 7:26 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > > > > On Mon, 8 Aug 2022 22:47:38 +0200 > > Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> 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@xxxxxxxxx> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> > > 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
