On Thu, Dec 6, 2018 at 11:34 AM Charles Keepax <[email protected]> wrote: > On Thu, Dec 06, 2018 at 09:58:30AM +0100, Linus Walleij wrote: > > On Wed, Dec 5, 2018 at 4:33 PM Charles Keepax > > <[email protected]> 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 > > > > <[email protected]> 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.
Indeed I just think it will be confusing when people read the code. It's better consistency if its just devm_* and the we unhinge the one we pass to the regulator core IMO. Yours, Linus Walleij
