lkml.org 
[lkml]   [2021]   [Dec]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] media: i2c: ov8865: Neaten unnecessary indentation
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

\
 
 \ /
  Last update: 2021-12-07 13:25    [W:0.065 / U:0.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site