Re: [PATCH 1/3] media: i2c: ov9282: Convert to CCI register access helpers

[Tarang Raval]

Hi Xiaolei,

> Use the new common CCI register access helpers to replace the private
> register access helpers in the ov9282 driver. This simplifies the driver
> by reducing the amount of code.
>
> Signed-off-by: Xiaolei Wang <xiaolei.wang@xxxxxxxxxxxxx>

...

>  /**
>   * ov9282_update_controls() - Update control ranges based on streaming mode
>   * @ov9282: pointer to ov9282 device
> @@ -639,15 +536,15 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
>         dev_dbg(ov9282->dev, "Set exp %u (~%u us), analog gain %u",
>                 exposure, exposure_us, gain);
>
> -       ret = ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 1);
> +       ret = cci_write(ov9282->regmap, OV9282_REG_HOLD, 0x01, NULL);
>         if (ret)
>                 return ret;
>
> -       ret = ov9282_write_reg(ov9282, OV9282_REG_EXPOSURE, 3, exposure << 4);
> +       ret = cci_write(ov9282->regmap, OV9282_REG_EXPOSURE, exposure << 4, NULL);
>         if (ret)
>                 goto error_release_group_hold;
>
> -       ret = ov9282_write_reg(ov9282, OV9282_REG_AGAIN, 1, gain);
> +       ret = cci_write(ov9282->regmap, OV9282_REG_AGAIN, gain, NULL);
>         if (ret)
>                 goto error_release_group_hold;
>
> @@ -656,62 +553,11 @@ static int ov9282_update_exp_gain(struct ov9282 *ov9282, u32 exposure, u32 gain)
>                                        OV9282_STROBE_FRAME_SPAN_DEFAULT);
>
>  error_release_group_hold:
> -       ov9282_write_reg(ov9282, OV9282_REG_HOLD, 1, 0);
> +       cci_write(ov9282->regmap, OV9282_REG_HOLD, 0, NULL);

If all the operations above succeed but releasing the group hold fails,
you still return success, which is wrong.

This case is rare, but it’s better to fix the error path.

I think we should do something like this:

error_release_group_hold:
      int ret_hold = cci_write(ov9282->regmap, OV9282_REG_HOLD, 0, NULL);

      return ret ? ret : ret_hold;

>
>         return ret;
>  }

Other than this issue, the patch looks good to me.

Reviewed-by: Tarang Raval <tarang.raval@xxxxxxxxxxxxxxxxx>

Best Regards,
Tarang