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

[xiaolei wang]

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@xxxxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxxxx>
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