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@xxxxxxxxxxx>
> ---
> 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
