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:
+ 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);
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 = {
--
2.43.0On 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
On 12/29/25 21:43, Laurent Pinchart 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.
>
> 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>
Hi Laurent,
Thank you for the review and the suggestions.
I'll address both points in v3:
1. Rename the label from 'err_rpm_put' to 'done' since it's used for
both error and normal exit paths.
2. Extract the duplicated stream stop code into a separate
ov5647_stream_stop() function to eliminate code duplication between
ov5647_disable_streams() and ov5647_power_on().
3. Remove the unnecessary ret = 0 initialization once the function
is extracted.
Will send the updated patch shortly.
Best regards,
Xiaolei
>
>> 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
© 2016 - 2026 Red Hat, Inc.