| Message ID | c6189daaac183ddf51da1444c597d8577c1ac416.camel@perches.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: ov8865: Neaten unnecessary indentation | expand |
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; > } > >
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.
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.
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.
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; }
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(-)