Re: [PATCH 06/15 v3] regulator: max8973: Let core handle GPIO descriptor

[Charles Keepax]

On Thu, Dec 06, 2018 at 09:58:30AM +0100, Linus Walleij wrote:
> On Wed, Dec 5, 2018 at 4:33 PM Charles Keepax
> <ckeepax@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > On Wed, Dec 05, 2018 at 03:42:06PM +0100, Linus Walleij wrote:
> > > On Wed, Dec 5, 2018 at 2:43 PM Charles Keepax
> > > <ckeepax@xxxxxxxxxxxxxxxxxxxxx> wrote:
> > > > On Wed, Dec 05, 2018 at 01:47:12PM +0100, Linus Walleij wrote:
> > > > > @@ -775,10 +779,13 @@ static int max8973_probe(struct i2c_client *client,
> > > > >               /*
> > > > >                * We do not let the core switch this regulator on/off,
> > > > >                * we just leave it on.
> > > > > +              *
> > > > > +              * Do not use devm* here: the regulator core takes over the
> > > > > +              * lifecycle management of the GPIO descriptor.
> > > > >                */
> > > > > -             gpiod = devm_gpiod_get_optional(&client->dev,
> > > > > -                                             "maxim,enable",
> > > > > -                                             GPIOD_OUT_HIGH);
> > > > > +             gpiod = gpiod_get_optional(&client->dev,
> > > > > +                                        "maxim,enable",
> > > > > +                                        GPIOD_OUT_HIGH);
> > > >
> > > > My comment on v2 still stands here, the GPIO is not passed to
> > > > the regulator core on this path.
> > >
> > > Patch 01 should take care of that, did you check it?
> >
> > Yeah, patch 1 makes the regulator core consistently handle GPIOs
> > that are passed into it.
> 
> Sorry. I confused this patch for the max77686 patch. That
> one was fixed with patch 01...
> 
> > However, on the MAX77621 path in this
> > switch statement the GPIO is never passed into the regulator
> > core, so the core never takes ownership of it, so the driver still
> > needs to manage the lifetime.
> >
> > It should be a pretty easy fix though as commented on v2, again
> > apologies for the slow review.
> 
> OK I switch it to devm_ as you suggested, as we implemented
> gpiod_unhinge it's a piece of cake nowadays.
> 

You shouldn't really need to use unhinge, you can just use devm
on the path that doesn't pass it to the core and not on the
one that does. You just need to update the error case below it to
use config->ena_gpiod rather than gpiod.

Thanks,
Charles