Re: [PATCH v2 3/3] media: i2c: ov5647: switch to {enable,disable}_streams

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Mon, Dec 29, 2025 at 10:30:18AM +0800, Xiaolei Wang wrote:
> Switch from s_stream to enable_streams and disable_streams callbacks.
> 
> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> ---
>  drivers/media/i2c/ov5647.c | 69 ++++++++++++++++----------------------
>  1 file changed, 29 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index f0ca8cc14794..7f4541f46335 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -637,23 +637,29 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)
>  	return 0;
>  }
>  
> -static int ov5647_stream_on(struct v4l2_subdev *sd)
> +static int ov5647_enable_streams(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_state *state, u32 pad,
> +				 u64 streams_mask)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  	struct ov5647 *sensor = to_sensor(sd);
>  	u8 val = MIPI_CTRL00_BUS_IDLE;
>  	int ret = 0;
>  
> +	ret = pm_runtime_resume_and_get(&client->dev);
> +	if (ret < 0)
> +		return ret;
> +
>  	ret = ov5647_set_mode(sd);
>  	if (ret) {
>  		dev_err(&client->dev, "Failed to program sensor mode: %d\n", ret);
> -		return ret;
> +		goto err_rpm_put;
>  	}
>  
>  	/* 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;
> +		goto err_rpm_put;
>  
>  	if (sensor->clock_ncont)
>  		val |= MIPI_CTRL00_CLOCK_LANE_GATE |
> @@ -663,11 +669,18 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)
>  	cci_write(sensor->regmap, OV5647_REG_FRAME_OFF_NUMBER, 0x00, &ret);
>  	cci_write(sensor->regmap, OV5640_REG_PAD_OUT, 0x00, &ret);
>  
> +err_rpm_put:

I would name the label 'done' as it's also used in the normal exit path,
not only in case of errors.

> +	if (ret)
> +		pm_runtime_put(&client->dev);
> +
>  	return ret;
>  }
>  
> -static int ov5647_stream_off(struct v4l2_subdev *sd)
> +static int ov5647_disable_streams(struct v4l2_subdev *sd,
> +				  struct v4l2_subdev_state *state, u32 pad,
> +				  u64 streams_mask)
>  {
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  	struct ov5647 *sensor = to_sensor(sd);
>  	int ret = 0;
>  
> @@ -677,13 +690,15 @@ static int ov5647_stream_off(struct v4l2_subdev *sd)
>  	cci_write(sensor->regmap, OV5647_REG_FRAME_OFF_NUMBER, 0x0f, &ret);
>  	cci_write(sensor->regmap, OV5640_REG_PAD_OUT, 0x01, &ret);
>  
> +	pm_runtime_put(&client->dev);
> +
>  	return ret;
>  }
>  
>  static int ov5647_power_on(struct device *dev)
>  {
>  	struct ov5647 *sensor = dev_get_drvdata(dev);
> -	int ret;
> +	int ret = 0;
>  
>  	dev_dbg(dev, "OV5647 power on\n");
>  
> @@ -706,7 +721,11 @@ static int ov5647_power_on(struct device *dev)
>  	}
>  
>  	/* Stream off to coax lanes into LP-11 state. */
> -	ret = ov5647_stream_off(&sensor->sd);
> +	cci_write(sensor->regmap, OV5647_REG_MIPI_CTRL00,
> +		  MIPI_CTRL00_CLOCK_LANE_GATE | MIPI_CTRL00_BUS_IDLE |
> +		  MIPI_CTRL00_CLOCK_LANE_DISABLE, &ret);
> +	cci_write(sensor->regmap, OV5647_REG_FRAME_OFF_NUMBER, 0x0f, &ret);
> +	cci_write(sensor->regmap, OV5640_REG_PAD_OUT, 0x01, &ret);

Could you please factor this code out to a separate function (you can
name it ov5647_stream_stop() for instance) instead of duplicating it ?

With that (and the 0 initialization of ret above dropped as it won't be
needed anymore),

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	if (ret < 0) {
>  		dev_err(dev, "camera not available, check power\n");
>  		goto error_clk_disable;
> @@ -803,40 +822,8 @@ __ov5647_get_pad_crop(struct ov5647 *ov5647,
>  	return NULL;
>  }
>  
> -static int ov5647_s_stream(struct v4l2_subdev *sd, int enable)
> -{
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
> -	int ret;
> -
> -	if (enable) {
> -		ret = pm_runtime_resume_and_get(&client->dev);
> -		if (ret < 0)
> -			return ret;
> -
> -		ret = ov5647_stream_on(sd);
> -		if (ret < 0) {
> -			dev_err(&client->dev, "stream start failed: %d\n", ret);
> -			goto error_pm;
> -		}
> -	} else {
> -		ret = ov5647_stream_off(sd);
> -		if (ret < 0) {
> -			dev_err(&client->dev, "stream stop failed: %d\n", ret);
> -			goto error_pm;
> -		}
> -		pm_runtime_put(&client->dev);
> -	}
> -
> -	return 0;
> -
> -error_pm:
> -	pm_runtime_put(&client->dev);
> -
> -	return ret;
> -}
> -
>  static const struct v4l2_subdev_video_ops ov5647_subdev_video_ops = {
> -	.s_stream =		ov5647_s_stream,
> +	.s_stream = v4l2_subdev_s_stream_helper,
>  };
>  
>  static int ov5647_enum_mbus_code(struct v4l2_subdev *sd,
> @@ -979,6 +966,8 @@ static const struct v4l2_subdev_pad_ops ov5647_subdev_pad_ops = {
>  	.set_fmt		= ov5647_set_pad_fmt,
>  	.get_fmt		= ov5647_get_pad_fmt,
>  	.get_selection		= ov5647_get_selection,
> +	.enable_streams         = ov5647_enable_streams,
> +	.disable_streams        = ov5647_disable_streams,
>  };
>  
>  static const struct v4l2_subdev_ops ov5647_subdev_ops = {

-- 
Regards,

Laurent Pinchart