Re: [PATCH 11/13] iio: frequency: admv1013: Benefit from devm_clk_get_enabled() to simplify

[Jonathan Cameron <jic23@kernel.org>]

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