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

[xiaolei wang <xiaolei.wang@windriver.com>]

On 3/1/26 02:12, Tarang Raval wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> 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@windriver.com>
> ...
>
>>   /**
>>    * 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;
Hi Tarang,

Thank you for catching this!

  I'll fix it in v2 with:

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

Best Regards,
Xiaolei
>
>>          return ret;
>>   }
> Other than this issue, the patch looks good to me.
>
> Reviewed-by: Tarang Raval <tarang.raval@siliconsignals.io>
>
> Best Regards,
> Tarang