Date:   Tue, 7 Dec 2021 14:24:52 +0200
From:   Sakari Ailus <sakari.ailus@iki.fi>
To:     Joe Perches <joe@perches.com>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        Paul Kocialkowski <paul.kocialkowski@bootlin.com>,
        linux-media <linux-media@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] media: i2c: ov8865: Neaten unnecessary indentation
Message-ID: <Ya9SlGo5HZpOXTmZ@valkosipuli.retiisi.eu>
References: <c6189daaac183ddf51da1444c597d8577c1ac416.camel@perches.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c6189daaac183ddf51da1444c597d8577c1ac416.camel@perches.com>
Precedence: bulk

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