On Thu, 7 May 2020 12:50:16 +0300
Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> On Sat, May 02, 2020 at 07:25:42PM +0100, Jonathan Cameron wrote:
> > On Tue, 28 Apr 2020 12:31:28 +0300
> > Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> wrote:
> > > +static void ad5933_cleanup(void *data)
> > > +{
> > > + struct ad5933_state *st = data;
> > > +
> > > + clk_disable_unprepare(st->mclk);
> > > + regulator_disable(st->reg);
> >
> > Please do two separate callbacks so that these can be handled
> > in the correct places. I.e. you do something then immediately
> > register the handler to undo it.
> >
> > Currently you can end up disabling a clock you haven't enabled
> > (which I am fairly sure will give you an error message).
>
> Yeah. It does.
>
> It feels like we should just make a devm_ version of regulator_enable().
> Or potentially this is more complicated than it seems, but in that case
> probably adding devm_add_action_or_reset() is more complicated than it
> seems as well.
>
> regards,
> dan carpenter
It has been a while since that was last proposed. At the time the
counter argument was that you should almost always be doing some form
of PM and hence the regulator shouldn't have the same lifetime as the
driver. Reality is that a lot of simple drivers either don't do
PM or have elected to not turn the regulator off so as to retain state
etc.
Mark what do you think?
Jonathan
