From: Jacopo Mondi <jacopo@jmondi.org>
To: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc: linux-renesas-soc@vger.kernel.org,
"Laurent Pinchart" <laurent.pinchart@ideasonboard.com>,
"Niklas Söderlund" <niklas.soderlund@ragnatech.se>,
"Hyun Kwon" <hyunk@xilinx.com>,
"Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org>
Subject: Re: [PATCH v8 03/13] squash! max9286: Fix cleanup path from GPIO powerdown
Date: Thu, 9 Apr 2020 18:22:07 +0200 [thread overview]
Message-ID: <20200409162207.xcj2ro4ecji5yte4@uno.localdomain> (raw)
In-Reply-To: <20200409121202.11130-4-kieran.bingham+renesas@ideasonboard.com>
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
>
next prev parent reply other threads:[~2020-04-09 16:19 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-09 12:11 [PATCH v8 00/13] max9286 v8 - modifications Kieran Bingham
2020-04-09 12:11 ` [PATCH v8 01/13] squash! max9286: Update the bound_sources mask on unbind Kieran Bingham
2020-04-09 15:20 ` Jacopo Mondi
2020-04-09 12:11 ` [PATCH v8 02/13] squash! max9286: convert probe kzalloc Kieran Bingham
2020-04-09 16:33 ` Laurent Pinchart
2020-04-10 8:20 ` Kieran Bingham
2020-04-10 11:15 ` Laurent Pinchart
2020-04-23 7:38 ` Sakari Ailus
2020-04-23 9:32 ` Laurent Pinchart
2020-04-23 10:55 ` Kieran Bingham
2020-04-23 23:25 ` Sakari Ailus
2020-04-09 17:08 ` [PATCH] squash! i2c: max9286: Put of node on error Jacopo Mondi
2020-04-09 20:20 ` Jacopo Mondi
2020-04-10 7:20 ` Kieran Bingham
2020-04-09 12:11 ` [PATCH v8 03/13] squash! max9286: Fix cleanup path from GPIO powerdown Kieran Bingham
2020-04-09 16:22 ` Jacopo Mondi [this message]
2020-04-10 7:34 ` Geert Uytterhoeven
2020-04-10 7:41 ` Kieran Bingham
2020-04-10 7:52 ` Jacopo Mondi
2020-04-10 7:38 ` Kieran Bingham
2020-04-10 7:42 ` Kieran Bingham
2020-04-11 12:33 ` Jacopo Mondi
2020-04-09 12:11 ` [PATCH v8 04/13] squash! max9286: cleanup GPIO device registration fail path Kieran Bingham
2020-04-09 12:11 ` [PATCH v8 05/13] squash! max9286: Convert to use devm_regulator_get() Kieran Bingham
2020-04-09 12:15 ` Kieran Bingham
2020-04-09 12:11 ` [PATCH v8 06/13] squash! max9286: Fit max9286_parse_dt print on one line Kieran Bingham
2020-04-09 12:11 ` [PATCH v8 07/13] squash! max9286: Move multi-device workarounds out of upstream Kieran Bingham
2020-04-09 12:11 ` [PATCH v8 08/13] squash! max9286: Remove I2C mod-table Kieran Bingham
2020-04-09 12:11 ` [PATCH v8 09/13] sqaush! max9286: Lock format changes Kieran Bingham
2020-04-09 12:11 ` [PATCH v8 10/13] squash! max9286: Implement Pixelrate control Kieran Bingham
2020-04-09 16:29 ` Jacopo Mondi
2020-04-10 7:51 ` Kieran Bingham
2020-04-10 12:35 ` Jacopo Mondi
2020-04-14 10:50 ` Jacopo Mondi
2020-04-14 21:10 ` Laurent Pinchart
2020-04-15 9:13 ` Jacopo Mondi
2020-04-15 15:28 ` Laurent Pinchart
2020-04-09 12:12 ` [PATCH v8 11/13] squash! max9286: Disable overlap window Kieran Bingham
2020-04-09 16:32 ` Jacopo Mondi
2020-04-10 7:14 ` Kieran Bingham
2020-04-10 7:48 ` Jacopo Mondi
2020-04-09 12:12 ` [PATCH v8 12/13] sqaush! max9286: Describe pad index usage Kieran Bingham
2020-04-09 12:12 ` [PATCH v8 13/13] sqaush! max9286: Remove poc_enabled workaround Kieran Bingham
2020-04-09 16:33 ` Jacopo Mondi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200409162207.xcj2ro4ecji5yte4@uno.localdomain \
--to=jacopo@jmondi.org \
--cc=hyunk@xilinx.com \
--cc=kieran.bingham+renesas@ideasonboard.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=manivannan.sadhasivam@linaro.org \
--cc=niklas.soderlund@ragnatech.se \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.