| Message ID | 20200409121202.11130-4-kieran.bingham+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | max9286 v8 - modifications | expand |
Hi Kieran, slightly unrelated on this patch but On Thu, Apr 09, 2020 at 01:11:52PM +0100, Kieran Bingham wrote: > - Fix up cleanup path from GPIO PowerDown registration > --- > drivers/media/i2c/max9286.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index 0a43137b8112..cc99740b34c5 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client) > > priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable", > GPIOD_OUT_HIGH); > - if (IS_ERR(priv->gpiod_pwdn)) > - return PTR_ERR(priv->gpiod_pwdn); > + if (IS_ERR(priv->gpiod_pwdn)) { > + ret = PTR_ERR(priv->gpiod_pwdn); > + goto err_cleanup_dt; > + } > > gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn"); > gpiod_set_value_cansleep(priv->gpiod_pwdn, 1); As we get_optional(), shouldn't this be protected by an if (priv->gpiod_pwdn) ? Another point (sorry, I'm looking at gpio handling only now) we have ret = max9286_gpio(priv); if (ret) return ret; That's not really a descriptive function name.. Could this be max9286_register_gpiochip() ? One last point and then I'll stop. We currently do probe() { parse_dt() pwdn = devm_get_gpio_optional() if (err) goto err_cleanup_dt() set(pwdn, 1); register_gpiochip(); //uses devm goto err_cleanup_dt() get_regulator() goto err_cleanup_dt() ret = init() if (ret) goto err_regulator(); return 0 err_regulator: regulator_put() mux_close() i2c_ack_disable() err_cleanup_dt: cleanup_dt() } With patch 5 of this series this becomes probe() { parse_dt() pwdn = devm_get_gpio_optional() if (err) goto err_cleanup_dt() set(pwdn, 1); register_gpiochip(); //uses devm goto err_cleanup_dt() devm_get_regulator() goto err_cleanup_dt() ret = init() if (ret) goto err_regulator(); return 0 err_regulator: mux_close() i2c_ack_disable() err_cleanup_dt: cleanup_dt() } as the i2c_mux is already closed at the end of init() (or never open if we fail earlier) and i2c_ack can be disabled at the end of max9286_setup() and in the error path there (as there are no more i2c writes after that function returns), I think we could simplify all of thise even more to: probe() { pwdn = devm_get_gpio_optional() if (err) return; set(pwdn, 1); register_gpiochip(); //uses devm return; devm_get_regulator() return; parse_dt() ret = init() if (ret) goto cleanup_dt(); return 0 err_cleanup_dt: cleanup_dt() } This could be done after 5/5 in this series if you want to keep fixups separate for another review round. What do you think ? Thanks j > @@ -1193,7 +1195,7 @@ static int max9286_probe(struct i2c_client *client) > PTR_ERR(priv->regulator)); > ret = PTR_ERR(priv->regulator); > priv->regulator = NULL; > - goto err_free; > + goto err_cleanup_dt; > } > > /* > @@ -1230,7 +1232,7 @@ static int max9286_probe(struct i2c_client *client) > regulator_put(priv->regulator); > max9286_i2c_mux_close(priv); > max9286_configure_i2c(priv, false); > -err_free: > +err_cleanup_dt: > max9286_cleanup_dt(priv); > > return ret; > @@ -1248,10 +1250,10 @@ static int max9286_remove(struct i2c_client *client) > regulator_disable(priv->regulator); > regulator_put(priv->regulator); > > - max9286_cleanup_dt(priv); > - > gpiod_set_value_cansleep(priv->gpiod_pwdn, 0); > > + max9286_cleanup_dt(priv); > + > return 0; > } > > -- > 2.20.1 >
Hi Jacopo, On Thu, Apr 9, 2020 at 7:07 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > On Thu, Apr 09, 2020 at 01:11:52PM +0100, Kieran Bingham wrote: > > - Fix up cleanup path from GPIO PowerDown registration > > --- > > drivers/media/i2c/max9286.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > > index 0a43137b8112..cc99740b34c5 100644 > > --- a/drivers/media/i2c/max9286.c > > +++ b/drivers/media/i2c/max9286.c > > @@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client) > > > > priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable", > > GPIOD_OUT_HIGH); > > - if (IS_ERR(priv->gpiod_pwdn)) > > - return PTR_ERR(priv->gpiod_pwdn); > > + if (IS_ERR(priv->gpiod_pwdn)) { > > + ret = PTR_ERR(priv->gpiod_pwdn); > > + goto err_cleanup_dt; > > + } > > > > gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn"); > > gpiod_set_value_cansleep(priv->gpiod_pwdn, 1); > > As we get_optional(), shouldn't this be protected by an > if (priv->gpiod_pwdn) > > ? When passed a NULL desc, validate_desc() just returns 0, without printing an error message, so both functions become no-ops. Gr{oetje,eeting}s, Geert
Hi Jacopo, On 09/04/2020 17:22, Jacopo Mondi wrote: > Hi Kieran, > slightly unrelated on this patch but Everything's related :-) All comments welcome! > > On Thu, Apr 09, 2020 at 01:11:52PM +0100, Kieran Bingham wrote: >> - Fix up cleanup path from GPIO PowerDown registration >> --- >> drivers/media/i2c/max9286.c | 14 ++++++++------ >> 1 file changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c >> index 0a43137b8112..cc99740b34c5 100644 >> --- a/drivers/media/i2c/max9286.c >> +++ b/drivers/media/i2c/max9286.c >> @@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client) >> >> priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable", >> GPIOD_OUT_HIGH); >> - if (IS_ERR(priv->gpiod_pwdn)) >> - return PTR_ERR(priv->gpiod_pwdn); >> + if (IS_ERR(priv->gpiod_pwdn)) { >> + ret = PTR_ERR(priv->gpiod_pwdn); >> + goto err_cleanup_dt; >> + } >> >> gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn"); >> gpiod_set_value_cansleep(priv->gpiod_pwdn, 1); > > As we get_optional(), shouldn't this be protected by an > if (priv->gpiod_pwdn) > > ? > Err - yes! That's odd - I was sure I had tested this without a gpio specifier, and I thought those functions were null-safe, and were a no-op if there was no desc. Clearly some wire got crossed in my head - because I see no such null-checks now! I've added a new squash patch on top to correct for this (including checking priv->gpiod_pwdn on all uses). > Another point (sorry, I'm looking at gpio handling only now) we have > ret = max9286_gpio(priv); > if (ret) > return ret; > > That's not really a descriptive function name.. Could this be > max9286_register_gpiochip() Yup, I'm happy enough to rename that. Patch added on top. > ? > > One last point and then I'll stop. > > We currently do > > probe() { > parse_dt() > > pwdn = devm_get_gpio_optional() > if (err) > goto err_cleanup_dt() > set(pwdn, 1); > > register_gpiochip(); //uses devm > goto err_cleanup_dt() > > get_regulator() > goto err_cleanup_dt() > > ret = init() > if (ret) > goto err_regulator(); > > return 0 > > err_regulator: > regulator_put() > mux_close() > i2c_ack_disable() > err_cleanup_dt: > cleanup_dt() > > } > > With patch 5 of this series this becomes > > probe() { > parse_dt() > > pwdn = devm_get_gpio_optional() > if (err) > goto err_cleanup_dt() > set(pwdn, 1); > > register_gpiochip(); //uses devm > goto err_cleanup_dt() > > devm_get_regulator() > goto err_cleanup_dt() > > ret = init() > if (ret) > goto err_regulator(); > > return 0 > > err_regulator: > mux_close() > i2c_ack_disable() > err_cleanup_dt: > cleanup_dt() > } > > as the i2c_mux is already closed at the end of init() (or never open > if we fail earlier) and i2c_ack can be disabled at the end of > max9286_setup() and in the error path there (as there are no more i2c > writes after that function returns), I think we could simplify all of > thise even more to: > > probe() { > pwdn = devm_get_gpio_optional() > if (err) > return; > > set(pwdn, 1); > > register_gpiochip(); //uses devm > return; > > devm_get_regulator() > return; > > parse_dt() > > ret = init() > if (ret) > goto cleanup_dt(); > > return 0 > > err_cleanup_dt: > cleanup_dt() > } > > This could be done after 5/5 in this series if you want to keep fixups > separate for another review round. > Yes, indeed - it would make sense where possible to have the devm_ handled constructions before the manually managed ones. I'll add a patch on top. > What do you think ? > > Thanks > j > > > >> @@ -1193,7 +1195,7 @@ static int max9286_probe(struct i2c_client *client) >> PTR_ERR(priv->regulator)); >> ret = PTR_ERR(priv->regulator); >> priv->regulator = NULL; >> - goto err_free; >> + goto err_cleanup_dt; >> } >> >> /* >> @@ -1230,7 +1232,7 @@ static int max9286_probe(struct i2c_client *client) >> regulator_put(priv->regulator); >> max9286_i2c_mux_close(priv); >> max9286_configure_i2c(priv, false); >> -err_free: >> +err_cleanup_dt: >> max9286_cleanup_dt(priv); >> >> return ret; >> @@ -1248,10 +1250,10 @@ static int max9286_remove(struct i2c_client *client) >> regulator_disable(priv->regulator); >> regulator_put(priv->regulator); >> >> - max9286_cleanup_dt(priv); >> - >> gpiod_set_value_cansleep(priv->gpiod_pwdn, 0); >> >> + max9286_cleanup_dt(priv); >> + >> return 0; >> } >> >> -- >> 2.20.1 >>
On 10/04/2020 08:34, Geert Uytterhoeven wrote: > Hi Jacopo, > > On Thu, Apr 9, 2020 at 7:07 PM Jacopo Mondi <jacopo@jmondi.org> wrote: >> On Thu, Apr 09, 2020 at 01:11:52PM +0100, Kieran Bingham wrote: >>> - Fix up cleanup path from GPIO PowerDown registration >>> --- >>> drivers/media/i2c/max9286.c | 14 ++++++++------ >>> 1 file changed, 8 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c >>> index 0a43137b8112..cc99740b34c5 100644 >>> --- a/drivers/media/i2c/max9286.c >>> +++ b/drivers/media/i2c/max9286.c >>> @@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client) >>> >>> priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable", >>> GPIOD_OUT_HIGH); >>> - if (IS_ERR(priv->gpiod_pwdn)) >>> - return PTR_ERR(priv->gpiod_pwdn); >>> + if (IS_ERR(priv->gpiod_pwdn)) { >>> + ret = PTR_ERR(priv->gpiod_pwdn); >>> + goto err_cleanup_dt; >>> + } >>> >>> gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn"); >>> gpiod_set_value_cansleep(priv->gpiod_pwdn, 1); >> >> As we get_optional(), shouldn't this be protected by an >> if (priv->gpiod_pwdn) >> >> ? > > When passed a NULL desc, validate_desc() just returns 0, without printing > an error message, so both functions become no-ops. Aha! I knew I'd tested that code path :-) Thanks for highlighting that, I missed it just 10 minutes ago when I rechecked because: VALIDATE_DESC(desc); has a *HIDDEN RETURN* dammit: if (__valid <= 0) \ return __valid; \ } while (0) Honestly - I thought we didn't do that in the kernel for exactly this reason. Grrrrrrrrrr ... oh and grrrrr again ! They could have at least named it: VALIDATE_DESC_OR_RETURN(desc) > > Gr{oetje,eeting}s, > > Geert >
On 10/04/2020 08:38, Kieran Bingham wrote: > Hi Jacopo, >>> >>> priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable", >>> GPIOD_OUT_HIGH); >>> - if (IS_ERR(priv->gpiod_pwdn)) >>> - return PTR_ERR(priv->gpiod_pwdn); >>> + if (IS_ERR(priv->gpiod_pwdn)) { >>> + ret = PTR_ERR(priv->gpiod_pwdn); >>> + goto err_cleanup_dt; >>> + } >>> >>> gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn"); >>> gpiod_set_value_cansleep(priv->gpiod_pwdn, 1); >> >> As we get_optional(), shouldn't this be protected by an >> if (priv->gpiod_pwdn) >> >> ? >> > > Err - yes! That's odd - I was sure I had tested this without a gpio > specifier, and I thought those functions were null-safe, and were a > no-op if there was no desc. Clearly some wire got crossed in my head - > because I see no such null-checks now! Ahem, ok - so as highlighted by Geert, - these *are* NULL safe... > > > I've added a new squash patch on top to correct for this (including > checking priv->gpiod_pwdn on all uses). now dropped :-)
Hi Geert, On Fri, Apr 10, 2020 at 09:34:28AM +0200, Geert Uytterhoeven wrote: > Hi Jacopo, > > On Thu, Apr 9, 2020 at 7:07 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > On Thu, Apr 09, 2020 at 01:11:52PM +0100, Kieran Bingham wrote: > > > - Fix up cleanup path from GPIO PowerDown registration > > > --- > > > drivers/media/i2c/max9286.c | 14 ++++++++------ > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > > > index 0a43137b8112..cc99740b34c5 100644 > > > --- a/drivers/media/i2c/max9286.c > > > +++ b/drivers/media/i2c/max9286.c > > > @@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client) > > > > > > priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable", > > > GPIOD_OUT_HIGH); > > > - if (IS_ERR(priv->gpiod_pwdn)) > > > - return PTR_ERR(priv->gpiod_pwdn); > > > + if (IS_ERR(priv->gpiod_pwdn)) { > > > + ret = PTR_ERR(priv->gpiod_pwdn); > > > + goto err_cleanup_dt; > > > + } > > > > > > gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn"); > > > gpiod_set_value_cansleep(priv->gpiod_pwdn, 1); > > > > As we get_optional(), shouldn't this be protected by an > > if (priv->gpiod_pwdn) > > > > ? > > When passed a NULL desc, validate_desc() just returns 0, without printing > an error message, so both functions become no-ops. > Ah indeed, sorry I have overlooked this! Thanks for pointing this out and sorry Kieran for the noise! > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
HI Kieran, On Thu, Apr 09, 2020 at 06:22:12PM +0200, Jacopo Mondi wrote: > Hi Kieran, > slightly unrelated on this patch but > > On Thu, Apr 09, 2020 at 01:11:52PM +0100, Kieran Bingham wrote: > > - Fix up cleanup path from GPIO PowerDown registration > > --- > > drivers/media/i2c/max9286.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > > index 0a43137b8112..cc99740b34c5 100644 > > --- a/drivers/media/i2c/max9286.c > > +++ b/drivers/media/i2c/max9286.c > > @@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client) > > > > priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable", > > GPIOD_OUT_HIGH); > > - if (IS_ERR(priv->gpiod_pwdn)) > > - return PTR_ERR(priv->gpiod_pwdn); > > + if (IS_ERR(priv->gpiod_pwdn)) { > > + ret = PTR_ERR(priv->gpiod_pwdn); > > + goto err_cleanup_dt; > > + } > > > > gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn"); > > gpiod_set_value_cansleep(priv->gpiod_pwdn, 1); > > [snip] > as the i2c_mux is already closed at the end of init() (or never open > if we fail earlier) and i2c_ack can be disabled at the end of > max9286_setup() and in the error path there (as there are no more i2c > writes after that function returns), I think we could simplify all of Knowing you're working on a new squash! series, I've just noticed I've said somthing not correct here. > i2c_ack can be disabled at the end of > max9286_setup() and in the error path there (as there are no more i2c > writes after that function returns) That's not true, i2c auto ack should be disabled after registering the i2c adapters for the remote ends. i2c_add_adapter probes the remote ends, and so -a lot- of i2c writes take place. Sorry for the confusion, I think i2c auto ack could be disabled at the end of _init() not _setup(). Thanks j > probe() { > pwdn = devm_get_gpio_optional() > if (err) > return; > > set(pwdn, 1); > > register_gpiochip(); //uses devm > return; > > devm_get_regulator() > return; > > parse_dt() > > ret = init() > if (ret) > goto cleanup_dt(); > > return 0 > > err_cleanup_dt: > cleanup_dt() > } > > This could be done after 5/5 in this series if you want to keep fixups > separate for another review round. > > What do you think ? > > Thanks > j > > > > > @@ -1193,7 +1195,7 @@ static int max9286_probe(struct i2c_client *client) > > PTR_ERR(priv->regulator)); > > ret = PTR_ERR(priv->regulator); > > priv->regulator = NULL; > > - goto err_free; > > + goto err_cleanup_dt; > > } > > > > /* > > @@ -1230,7 +1232,7 @@ static int max9286_probe(struct i2c_client *client) > > regulator_put(priv->regulator); > > max9286_i2c_mux_close(priv); > > max9286_configure_i2c(priv, false); > > -err_free: > > +err_cleanup_dt: > > max9286_cleanup_dt(priv); > > > > return ret; > > @@ -1248,10 +1250,10 @@ static int max9286_remove(struct i2c_client *client) > > regulator_disable(priv->regulator); > > regulator_put(priv->regulator); > > > > - max9286_cleanup_dt(priv); > > - > > gpiod_set_value_cansleep(priv->gpiod_pwdn, 0); > > > > + max9286_cleanup_dt(priv); > > + > > return 0; > > } > > > > -- > > 2.20.1 > >
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c index 0a43137b8112..cc99740b34c5 100644 --- a/drivers/media/i2c/max9286.c +++ b/drivers/media/i2c/max9286.c @@ -1171,8 +1171,10 @@ static int max9286_probe(struct i2c_client *client) priv->gpiod_pwdn = devm_gpiod_get_optional(&client->dev, "enable", GPIOD_OUT_HIGH); - if (IS_ERR(priv->gpiod_pwdn)) - return PTR_ERR(priv->gpiod_pwdn); + if (IS_ERR(priv->gpiod_pwdn)) { + ret = PTR_ERR(priv->gpiod_pwdn); + goto err_cleanup_dt; + } gpiod_set_consumer_name(priv->gpiod_pwdn, "max9286-pwdn"); gpiod_set_value_cansleep(priv->gpiod_pwdn, 1); @@ -1193,7 +1195,7 @@ static int max9286_probe(struct i2c_client *client) PTR_ERR(priv->regulator)); ret = PTR_ERR(priv->regulator); priv->regulator = NULL; - goto err_free; + goto err_cleanup_dt; } /* @@ -1230,7 +1232,7 @@ static int max9286_probe(struct i2c_client *client) regulator_put(priv->regulator); max9286_i2c_mux_close(priv); max9286_configure_i2c(priv, false); -err_free: +err_cleanup_dt: max9286_cleanup_dt(priv); return ret; @@ -1248,10 +1250,10 @@ static int max9286_remove(struct i2c_client *client) regulator_disable(priv->regulator); regulator_put(priv->regulator); - max9286_cleanup_dt(priv); - gpiod_set_value_cansleep(priv->gpiod_pwdn, 0); + max9286_cleanup_dt(priv); + return 0; }