* [PATCH] media: i2c: ov8865: Neaten unnecessary indentation
@ 2021-12-02 9:06 Joe Perches
2021-12-07 12:24 ` Sakari Ailus
0 siblings, 1 reply; 5+ messages in thread
From: Joe Perches @ 2021-12-02 9:06 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Paul Kocialkowski, linux-media, LKML
Jumping to the start of a labeled else block isn't typical.
Unindent the code by reversing the test and using a goto instead.
Signed-off-by: Joe Perches <joe@perches.com>
---
drivers/media/i2c/ov8865.c | 81 +++++++++++++++++++++++-----------------------
1 file changed, 41 insertions(+), 40 deletions(-)
diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index ebdb20d3fe9d8..7ef83a10f586f 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -2396,56 +2396,57 @@ static int ov8865_sensor_init(struct ov8865_sensor *sensor)
static int ov8865_sensor_power(struct ov8865_sensor *sensor, bool on)
{
- /* Keep initialized to zero for disable label. */
- int ret = 0;
+ int ret;
- if (on) {
- gpiod_set_value_cansleep(sensor->reset, 1);
- gpiod_set_value_cansleep(sensor->powerdown, 1);
+ if (!on) {
+ ret = 0;
+ goto disable;
+ }
- ret = regulator_enable(sensor->dovdd);
- if (ret) {
- dev_err(sensor->dev,
- "failed to enable DOVDD regulator\n");
- goto disable;
- }
+ gpiod_set_value_cansleep(sensor->reset, 1);
+ gpiod_set_value_cansleep(sensor->powerdown, 1);
- ret = regulator_enable(sensor->avdd);
- if (ret) {
- dev_err(sensor->dev,
- "failed to enable AVDD regulator\n");
- goto disable;
- }
+ ret = regulator_enable(sensor->dovdd);
+ if (ret) {
+ dev_err(sensor->dev, "failed to enable DOVDD regulator\n");
+ goto disable;
+ }
- ret = regulator_enable(sensor->dvdd);
- if (ret) {
- dev_err(sensor->dev,
- "failed to enable DVDD regulator\n");
- goto disable;
- }
+ ret = regulator_enable(sensor->avdd);
+ if (ret) {
+ dev_err(sensor->dev, "failed to enable AVDD regulator\n");
+ goto disable;
+ }
- ret = clk_prepare_enable(sensor->extclk);
- if (ret) {
- dev_err(sensor->dev, "failed to enable EXTCLK clock\n");
- goto disable;
- }
+ ret = regulator_enable(sensor->dvdd);
+ if (ret) {
+ dev_err(sensor->dev, "failed to enable DVDD regulator\n");
+ goto disable;
+ }
+
+ ret = clk_prepare_enable(sensor->extclk);
+ if (ret) {
+ dev_err(sensor->dev, "failed to enable EXTCLK clock\n");
+ goto disable;
+ }
- gpiod_set_value_cansleep(sensor->reset, 0);
- gpiod_set_value_cansleep(sensor->powerdown, 0);
+ gpiod_set_value_cansleep(sensor->reset, 0);
+ gpiod_set_value_cansleep(sensor->powerdown, 0);
+
+ /* Time to enter streaming mode according to power timings. */
+ usleep_range(10000, 12000);
+
+ return 0;
- /* Time to enter streaming mode according to power timings. */
- usleep_range(10000, 12000);
- } else {
disable:
- gpiod_set_value_cansleep(sensor->powerdown, 1);
- gpiod_set_value_cansleep(sensor->reset, 1);
+ gpiod_set_value_cansleep(sensor->powerdown, 1);
+ gpiod_set_value_cansleep(sensor->reset, 1);
- clk_disable_unprepare(sensor->extclk);
+ clk_disable_unprepare(sensor->extclk);
- regulator_disable(sensor->dvdd);
- regulator_disable(sensor->avdd);
- regulator_disable(sensor->dovdd);
- }
+ regulator_disable(sensor->dvdd);
+ regulator_disable(sensor->avdd);
+ regulator_disable(sensor->dovdd);
return ret;
}
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] media: i2c: ov8865: Neaten unnecessary indentation
2021-12-02 9:06 [PATCH] media: i2c: ov8865: Neaten unnecessary indentation Joe Perches
@ 2021-12-07 12:24 ` Sakari Ailus
2021-12-07 14:47 ` Joe Perches
0 siblings, 1 reply; 5+ messages in thread
From: Sakari Ailus @ 2021-12-07 12:24 UTC (permalink / raw)
To: Joe Perches; +Cc: Mauro Carvalho Chehab, Paul Kocialkowski, linux-media, LKML
Hi Joe (and Paul),
On Thu, Dec 02, 2021 at 01:06:01AM -0800, Joe Perches wrote:
> Jumping to the start of a labeled else block isn't typical.
>
> Unindent the code by reversing the test and using a goto instead.
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
> drivers/media/i2c/ov8865.c | 81 +++++++++++++++++++++++-----------------------
> 1 file changed, 41 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
> index ebdb20d3fe9d8..7ef83a10f586f 100644
> --- a/drivers/media/i2c/ov8865.c
> +++ b/drivers/media/i2c/ov8865.c
> @@ -2396,56 +2396,57 @@ static int ov8865_sensor_init(struct ov8865_sensor *sensor)
>
> static int ov8865_sensor_power(struct ov8865_sensor *sensor, bool on)
> {
> - /* Keep initialized to zero for disable label. */
> - int ret = 0;
> + int ret;
>
> - if (on) {
> - gpiod_set_value_cansleep(sensor->reset, 1);
> - gpiod_set_value_cansleep(sensor->powerdown, 1);
> + if (!on) {
> + ret = 0;
> + goto disable;
> + }
>
> - ret = regulator_enable(sensor->dovdd);
> - if (ret) {
> - dev_err(sensor->dev,
> - "failed to enable DOVDD regulator\n");
> - goto disable;
I guess this patch is fine as such but there seems to be a problem in error
handling here: all regulators are disabled if there's a problem enabling
one of them.
Would it be possible to fix this as well?
> - }
> + gpiod_set_value_cansleep(sensor->reset, 1);
> + gpiod_set_value_cansleep(sensor->powerdown, 1);
>
> - ret = regulator_enable(sensor->avdd);
> - if (ret) {
> - dev_err(sensor->dev,
> - "failed to enable AVDD regulator\n");
> - goto disable;
> - }
> + ret = regulator_enable(sensor->dovdd);
> + if (ret) {
> + dev_err(sensor->dev, "failed to enable DOVDD regulator\n");
> + goto disable;
> + }
>
> - ret = regulator_enable(sensor->dvdd);
> - if (ret) {
> - dev_err(sensor->dev,
> - "failed to enable DVDD regulator\n");
> - goto disable;
> - }
> + ret = regulator_enable(sensor->avdd);
> + if (ret) {
> + dev_err(sensor->dev, "failed to enable AVDD regulator\n");
> + goto disable;
> + }
>
> - ret = clk_prepare_enable(sensor->extclk);
> - if (ret) {
> - dev_err(sensor->dev, "failed to enable EXTCLK clock\n");
> - goto disable;
> - }
> + ret = regulator_enable(sensor->dvdd);
> + if (ret) {
> + dev_err(sensor->dev, "failed to enable DVDD regulator\n");
> + goto disable;
> + }
> +
> + ret = clk_prepare_enable(sensor->extclk);
> + if (ret) {
> + dev_err(sensor->dev, "failed to enable EXTCLK clock\n");
> + goto disable;
> + }
>
> - gpiod_set_value_cansleep(sensor->reset, 0);
> - gpiod_set_value_cansleep(sensor->powerdown, 0);
> + gpiod_set_value_cansleep(sensor->reset, 0);
> + gpiod_set_value_cansleep(sensor->powerdown, 0);
> +
> + /* Time to enter streaming mode according to power timings. */
> + usleep_range(10000, 12000);
> +
> + return 0;
>
> - /* Time to enter streaming mode according to power timings. */
> - usleep_range(10000, 12000);
> - } else {
> disable:
> - gpiod_set_value_cansleep(sensor->powerdown, 1);
> - gpiod_set_value_cansleep(sensor->reset, 1);
> + gpiod_set_value_cansleep(sensor->powerdown, 1);
> + gpiod_set_value_cansleep(sensor->reset, 1);
>
> - clk_disable_unprepare(sensor->extclk);
> + clk_disable_unprepare(sensor->extclk);
>
> - regulator_disable(sensor->dvdd);
> - regulator_disable(sensor->avdd);
> - regulator_disable(sensor->dovdd);
> - }
> + regulator_disable(sensor->dvdd);
> + regulator_disable(sensor->avdd);
> + regulator_disable(sensor->dovdd);
>
> return ret;
> }
>
>
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: i2c: ov8865: Neaten unnecessary indentation
2021-12-07 12:24 ` Sakari Ailus
@ 2021-12-07 14:47 ` Joe Perches
2021-12-15 8:01 ` Sakari Ailus
0 siblings, 1 reply; 5+ messages in thread
From: Joe Perches @ 2021-12-07 14:47 UTC (permalink / raw)
To: Sakari Ailus; +Cc: Mauro Carvalho Chehab, Paul Kocialkowski, linux-media, LKML
On Tue, 2021-12-07 at 14:24 +0200, Sakari Ailus wrote:
> Hi Joe (and Paul),
> I guess this patch is fine as such but there seems to be a problem in error
> handling here: all regulators are disabled if there's a problem enabling
> one of them.
>
> Would it be possible to fix this as well?
I've no hardware to test, so I've no idea if that's the right thing to do.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: i2c: ov8865: Neaten unnecessary indentation
2021-12-07 14:47 ` Joe Perches
@ 2021-12-15 8:01 ` Sakari Ailus
2021-12-15 8:07 ` Joe Perches
0 siblings, 1 reply; 5+ messages in thread
From: Sakari Ailus @ 2021-12-15 8:01 UTC (permalink / raw)
To: Joe Perches; +Cc: Mauro Carvalho Chehab, Paul Kocialkowski, linux-media, LKML
Hi Joe
On Tue, Dec 07, 2021 at 06:47:45AM -0800, Joe Perches wrote:
> On Tue, 2021-12-07 at 14:24 +0200, Sakari Ailus wrote:
> > Hi Joe (and Paul),
>
> > I guess this patch is fine as such but there seems to be a problem in error
> > handling here: all regulators are disabled if there's a problem enabling
> > one of them.
> >
> > Would it be possible to fix this as well?
>
> I've no hardware to test, so I've no idea if that's the right thing to do.
I don't have the hardware either.
But I can tell that you shouldn't disable a regulator you haven't enabled
to begin with. Bugs (fixes of which probably should go to stable trees)
need to be fixed before reworking the code.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: i2c: ov8865: Neaten unnecessary indentation
2021-12-15 8:01 ` Sakari Ailus
@ 2021-12-15 8:07 ` Joe Perches
0 siblings, 0 replies; 5+ messages in thread
From: Joe Perches @ 2021-12-15 8:07 UTC (permalink / raw)
To: Sakari Ailus; +Cc: Mauro Carvalho Chehab, Paul Kocialkowski, linux-media, LKML
On Wed, 2021-12-15 at 10:01 +0200, Sakari Ailus wrote:
> Hi Joe
>
> On Tue, Dec 07, 2021 at 06:47:45AM -0800, Joe Perches wrote:
> > On Tue, 2021-12-07 at 14:24 +0200, Sakari Ailus wrote:
> > > Hi Joe (and Paul),
> >
> > > I guess this patch is fine as such but there seems to be a problem in error
> > > handling here: all regulators are disabled if there's a problem enabling
> > > one of them.
> > >
> > > Would it be possible to fix this as well?
> >
> > I've no hardware to test, so I've no idea if that's the right thing to do.
>
> I don't have the hardware either.
>
> But I can tell that you shouldn't disable a regulator you haven't enabled
> to begin with. Bugs (fixes of which probably should go to stable trees)
> need to be fixed before reworking the code.
I'm just fixing the ugly code.
You are welcome to fix what you believe to be logical defects.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-12-15 8:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-02 9:06 [PATCH] media: i2c: ov8865: Neaten unnecessary indentation Joe Perches
2021-12-07 12:24 ` Sakari Ailus
2021-12-07 14:47 ` Joe Perches
2021-12-15 8:01 ` Sakari Ailus
2021-12-15 8:07 ` Joe Perches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).