Re: [PATCH v2 2/3] media: i2c: ov5647: Switch to using the sub-device state lock

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

On 12/30/25 02:55, Sakari Ailus 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,
>
> On Mon, Dec 29, 2025 at 10:30:17AM +0800, Xiaolei Wang wrote:
>> Switch to using the sub-device state lock and properly call
>> v4l2_subdev_init_finalize() / v4l2_subdev_cleanup() on probe() /
>> remove().
>>
>> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
>> ---
>>   drivers/media/i2c/ov5647.c | 40 +++++++++++++-------------------------
>>   1 file changed, 14 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
>> index fd69f1616794..f0ca8cc14794 100644
>> --- a/drivers/media/i2c/ov5647.c
>> +++ b/drivers/media/i2c/ov5647.c
>> @@ -91,7 +91,6 @@ struct ov5647 {
>>        struct v4l2_subdev              sd;
>>        struct regmap                   *regmap;
>>        struct media_pad                pad;
>> -     struct mutex                    lock;
>>        struct clk                      *xclk;
>>        struct gpio_desc                *pwdn;
>>        bool                            clock_ncont;
>> @@ -652,7 +651,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)
>>        }
>>
>>        /* Apply customized values from user when stream starts. */
>> -     ret =  __v4l2_ctrl_handler_setup(sd->ctrl_handler);
>> +     ret =  v4l2_ctrl_handler_setup(sd->ctrl_handler);
>>        if (ret)
>>                return ret;
>>
>> @@ -807,15 +806,12 @@ __ov5647_get_pad_crop(struct ov5647 *ov5647,
>>   static int ov5647_s_stream(struct v4l2_subdev *sd, int enable)
>>   {
>>        struct i2c_client *client = v4l2_get_subdevdata(sd);
>> -     struct ov5647 *sensor = to_sensor(sd);
>>        int ret;
>>
>> -     mutex_lock(&sensor->lock);
> Note that you shouldn't remove mutex_lock() here quite yet -- s_stream()
> callback won't involve sub-device state and thus the caller won't take the
> state lock either. In other words, the end result is fine after the third
> patch so you should explicitly lock the active state and remove that in the
> third patch (see e.g. v4l2_subdev_lock_and_get_active_state() in
> drivers/media/i2c/imx290.c).
Hi Hans,

Thank you for the detailed review and suggestions.

You're absolutely right about the approach. Using regmap_multi_reg_write()
with struct reg_sequence would indeed be cleaner and result in a much
smaller, more reviewable diff.

I'll revise the patch to:
- Use struct reg_sequence instead of struct cci_reg_sequence
- Call regmap_multi_reg_write() instead of cci_multi_reg_write()
- Keep the existing array initializer values unchanged

This will maintain the same functionality while making the conversion
more straightforward and consistent with other driver conversions.

I'll send v3 shortly.
>
>> -
>>        if (enable) {
>>                ret = pm_runtime_resume_and_get(&client->dev);
>>                if (ret < 0)
>> -                     goto error_unlock;
>> +                     return ret;
>>
>>                ret = ov5647_stream_on(sd);
>>                if (ret < 0) {
>> @@ -831,14 +827,10 @@ static int ov5647_s_stream(struct v4l2_subdev *sd, int enable)
>>                pm_runtime_put(&client->dev);
>>        }
>>
>> -     mutex_unlock(&sensor->lock);
>> -
>>        return 0;
>>
>>   error_pm:
>>        pm_runtime_put(&client->dev);
>> -error_unlock:
>> -     mutex_unlock(&sensor->lock);
>>
>>        return ret;
>>   }
> --
> Kind regards,
>
> Sakari Ailus