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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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).

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