| Message ID | 20181205124721.26624-7-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Regulator ena_gpiod fixups | expand |
On Wed, Dec 05, 2018 at 01:47:12PM +0100, Linus Walleij wrote: > Use the gpiod_get() rather than the devm_* version so that the > regulator core can handle the lifecycle of these descriptors. > > Fixes: e7d2be696faa ("regulator: max8973: Pass descriptor instead of GPIO number") > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > ChangeLog v2->v3: > - Resending. > ChangeLog v1->v2: > - Drop the gpiod_put() on the errorpath after devm_regulator_register() > as this will be handled by the regulator core. > - Fix the second case of devm_gpiod_get() > - Put a comment in the code so maintainers knows not to > use managed resources (devm*) > --- > @@ -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. Very sorry it took me so long to review v2 (been one of those wonderful weeks at this end) and then I managed to perfectly time reviewing it to the exact second you sent v3. I think apart from this the series looks good to me though. Thanks, Charles
On Wed, Dec 5, 2018 at 2:43 PM Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > On Wed, Dec 05, 2018 at 01:47:12PM +0100, Linus Walleij wrote: > > Use the gpiod_get() rather than the devm_* version so that the > > regulator core can handle the lifecycle of these descriptors. > > > > Fixes: e7d2be696faa ("regulator: max8973: Pass descriptor instead of GPIO number") > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > > --- > > ChangeLog v2->v3: > > - Resending. > > ChangeLog v1->v2: > > - Drop the gpiod_put() on the errorpath after devm_regulator_register() > > as this will be handled by the regulator core. > > - Fix the second case of devm_gpiod_get() > > - Put a comment in the code so maintainers knows not to > > use managed resources (devm*) > > --- > > @@ -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? Yours, Linus Walleij
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@opensource.cirrus.com> 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. 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. Thanks, Charles
On Wed, Dec 5, 2018 at 4:33 PM Charles Keepax <ckeepax@opensource.cirrus.com> 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@opensource.cirrus.com> 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. Thanks a lot! Linus Walleij
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@opensource.cirrus.com> 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@opensource.cirrus.com> 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
On Thu, Dec 6, 2018 at 11:34 AM Charles Keepax <ckeepax@opensource.cirrus.com> 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 > > <ckeepax@opensource.cirrus.com> 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@opensource.cirrus.com> 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
On Thu, Dec 06, 2018 at 12:47:46PM +0100, Linus Walleij wrote: > On Thu, Dec 6, 2018 at 11:34 AM Charles Keepax > <ckeepax@opensource.cirrus.com> 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 > > > <ckeepax@opensource.cirrus.com> 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@opensource.cirrus.com> wrote: > > > > > > On Wed, Dec 05, 2018 at 01:47:12PM +0100, Linus Walleij wrote: > > 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. > That's ok no objections from my if you prefer it that way. Thanks, Charles
diff --git a/drivers/regulator/max8973-regulator.c b/drivers/regulator/max8973-regulator.c index e7a58b509032..ef8f4789a517 100644 --- a/drivers/regulator/max8973-regulator.c +++ b/drivers/regulator/max8973-regulator.c @@ -632,7 +632,7 @@ static int max8973_probe(struct i2c_client *client, struct max8973_chip *max; bool pdata_from_dt = false; unsigned int chip_id; - struct gpio_desc *gpiod; + struct gpio_desc *gpiod = NULL; enum gpiod_flags gflags; int ret; @@ -759,9 +759,13 @@ static int max8973_probe(struct i2c_client *client, else gflags = GPIOD_OUT_LOW; gflags |= GPIOD_FLAGS_BIT_NONEXCLUSIVE; - gpiod = devm_gpiod_get_optional(&client->dev, - "maxim,enable", - gflags); + /* + * Do not use devm* here: the regulator core takes over the + * lifecycle management of the GPIO descriptor. + */ + gpiod = gpiod_get_optional(&client->dev, + "maxim,enable", + gflags); if (IS_ERR(gpiod)) return PTR_ERR(gpiod); if (gpiod) { @@ -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); if (IS_ERR(gpiod)) return PTR_ERR(gpiod); if (gpiod) @@ -798,6 +805,8 @@ static int max8973_probe(struct i2c_client *client, ret = max8973_init_dcdc(max, pdata); if (ret < 0) { + if (gpiod) + gpiod_put(gpiod); dev_err(max->dev, "Max8973 Init failed, err = %d\n", ret); return ret; }
Use the gpiod_get() rather than the devm_* version so that the regulator core can handle the lifecycle of these descriptors. Fixes: e7d2be696faa ("regulator: max8973: Pass descriptor instead of GPIO number") Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- ChangeLog v2->v3: - Resending. ChangeLog v1->v2: - Drop the gpiod_put() on the errorpath after devm_regulator_register() as this will be handled by the regulator core. - Fix the second case of devm_gpiod_get() - Put a comment in the code so maintainers knows not to use managed resources (devm*) --- drivers/regulator/max8973-regulator.c | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) -- 2.19.2