Re: [PATCH v9 08/19] media: staging: starfive: Move to enabel-disable streams in starfive drivers

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

On Wed, Jan 14, 2026 at 06:21:21PM +0530, Jai Luthra wrote:
> Hi Rishikesh,
> 
> Thanks for the patch.

We should actually drop the driver. Starfive has confirmed they don't
plan to develop it further, so it shouldn't stay in staging.

> > Subject: [PATCH v9 08/19] media: staging: starfive: Move to enabel-disable streams in starfive drivers
> 
> s/enabel/enable
> 
> Quoting Rishikesh Donadkar (2025-12-30 14:02:09)
> > The enable_streams() API in v4l2 supports passing a bitmask to enable
> > each pad/stream combination individually on any media subdev. Use this
> > API instead of  s_stream() API in the starfive drivers
> 
> nit: I think the description can be explicit that this driver does not
> support "multiple streams" (at least right now), but just switching to the
> new API while ignoring the passed streams mask.
> 
> > 
> > Signed-off-by: Rishikesh Donadkar <r-donadkar@ti.com>
> > ---
> >  .../staging/media/starfive/camss/stf-isp.c    | 43 ++++++++++++-------
> >  .../staging/media/starfive/camss/stf-video.c  |  4 +-
> >  2 files changed, 30 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/staging/media/starfive/camss/stf-isp.c b/drivers/staging/media/starfive/camss/stf-isp.c
> > index df7a903fbb1b0..4930ffb0e07a6 100644
> > --- a/drivers/staging/media/starfive/camss/stf-isp.c
> > +++ b/drivers/staging/media/starfive/camss/stf-isp.c
> > @@ -55,27 +55,43 @@ int stf_isp_init(struct stfcamss *stfcamss)
> >         return 0;
> >  }
> >  
> > -static int isp_set_stream(struct v4l2_subdev *sd, int enable)
> > +static int isp_sd_enable_stream(struct v4l2_subdev *sd,
> > +                               struct v4l2_subdev_state *state,
> > +                               u32 pad, u64 streams_mask)
> >  {
> >         struct stf_isp_dev *isp_dev = v4l2_get_subdevdata(sd);
> >         struct v4l2_subdev_state *sd_state;
> >         struct v4l2_mbus_framefmt *fmt;
> >         struct v4l2_rect *crop;
> > +       int ret;
> >  
> > -       sd_state = v4l2_subdev_lock_and_get_active_state(sd);
> > +       sd_state = v4l2_subdev_get_locked_active_state(sd);
> >         fmt = v4l2_subdev_state_get_format(sd_state, STF_ISP_PAD_SINK);
> >         crop = v4l2_subdev_state_get_crop(sd_state, STF_ISP_PAD_SRC);
> >  
> > -       if (enable) {
> > -               stf_isp_reset(isp_dev);
> > -               stf_isp_init_cfg(isp_dev);
> > -               stf_isp_settings(isp_dev, crop, fmt->code);
> > -               stf_isp_stream_set(isp_dev);
> > -       }
> > +       stf_isp_reset(isp_dev);
> > +       stf_isp_init_cfg(isp_dev);
> > +       stf_isp_settings(isp_dev, crop, fmt->code);
> > +       stf_isp_stream_set(isp_dev);
> > +
> > +       ret = v4l2_subdev_enable_streams(isp_dev->source_subdev, 1, BIT(0));
> 
> Given you have a streams_mask argument in this function now, it might be
> cleaner to use it here (and let stf-video populate it with BIT(0)).
> 
> > +       if (ret)
> > +               return ret;
> > +
> > +       return 0;
> > +}
> >  
> > -       v4l2_subdev_call(isp_dev->source_subdev, video, s_stream, enable);
> > +static int isp_sd_disable_stream(struct v4l2_subdev *sd,
> > +                                struct v4l2_subdev_state *state,
> > +                                u32 pad, u64 streams_mask)
> > +{
> > +       struct stf_isp_dev *isp_dev = v4l2_get_subdevdata(sd);
> > +       int ret;
> > +
> > +       ret = v4l2_subdev_disable_streams(isp_dev->source_subdev, 1, BIT(0));
> 
> Same here.
> 
> > +       if (ret)
> > +               return ret;
> >  
> > -       v4l2_subdev_unlock_state(sd_state);
> >         return 0;
> >  }
> >  
> > @@ -300,20 +316,17 @@ static int isp_init_formats(struct v4l2_subdev *sd,
> >         return isp_set_format(sd, sd_state, &format);
> >  }
> >  
> > -static const struct v4l2_subdev_video_ops isp_video_ops = {
> > -       .s_stream = isp_set_stream,
> > -};
> > -
> >  static const struct v4l2_subdev_pad_ops isp_pad_ops = {
> >         .enum_mbus_code = isp_enum_mbus_code,
> >         .get_fmt = v4l2_subdev_get_fmt,
> >         .set_fmt = isp_set_format,
> >         .get_selection = isp_get_selection,
> >         .set_selection = isp_set_selection,
> > +       .enable_streams = isp_sd_enable_stream,
> > +       .disable_streams = isp_sd_disable_stream,
> >  };
> >  
> >  static const struct v4l2_subdev_ops isp_v4l2_ops = {
> > -       .video = &isp_video_ops,
> >         .pad = &isp_pad_ops,
> >  };
> >  
> > diff --git a/drivers/staging/media/starfive/camss/stf-video.c b/drivers/staging/media/starfive/camss/stf-video.c
> > index a0420eb6a0aa0..2db29bf8bdef8 100644
> > --- a/drivers/staging/media/starfive/camss/stf-video.c
> > +++ b/drivers/staging/media/starfive/camss/stf-video.c
> > @@ -287,7 +287,7 @@ static int video_start_streaming(struct vb2_queue *q, unsigned int count)
> >  
> >         video->ops->start_streaming(video);
> >  
> > -       ret = v4l2_subdev_call(video->source_subdev, video, s_stream, true);
> > +       ret = v4l2_subdev_enable_streams(video->source_subdev, 1, BIT(0));
> 
> Now that I think of it, it was not necessary to implement enable / disable
> API for the ISP subdev driver given v4l2_subdev_*_streams falls back on
> s_stream. But it's anyway good to move drivers, so I guess it's alright.
> 
> >         if (ret) {
> >                 dev_err(video->stfcamss->dev, "stream on failed\n");
> >                 goto err_pm_put;
> > @@ -311,7 +311,7 @@ static void video_stop_streaming(struct vb2_queue *q)
> >  
> >         video->ops->stop_streaming(video);
> >  
> > -       v4l2_subdev_call(video->source_subdev, video, s_stream, false);
> > +       v4l2_subdev_disable_streams(video->source_subdev, 1, BIT(0));
> >  
> >         pm_runtime_put(video->stfcamss->dev);
> >  

-- 
Regards,

Laurent Pinchart