Re: [PATCH v3] media: sun6i-mipi-csi2: Use V4L2 subdev active state

Hi Sakari,

On Thu, Jul 9, 2026 at 8:04 AM arash golgol <[email protected]> wrote:
>
> Hi,
>
> On Thu, May 7, 2026 at 3:30 PM Paul Kocialkowski <[email protected]> wrote:
> >
> > Hi Arash,
> >
> > Le Sat 14 Feb 26, 08:39, Arash Golgol a écrit :
> > > Use the V4L2 subdev active state API to store the active format.
> > > This simplifies the driver not only by dropping the bridge mbus_format
> > > field, but it also allows dropping the bridge lock, replaced with
> > > the state lock.
> > >
> > > The sun6i-mipi-csi2 hardware does not perform any format conversion.
> > > Enforce identical formats on the sink and source pads in the set_fmt()
> > > and init_state() callbacks.
> > >
> > > Signed-off-by: Arash Golgol <[email protected]>
> > > Reviewed-by: Laurent Pinchart <[email protected]>
> >
> > Looks very good. Feel free to let me know if you'd like to do the same
> > with A83T (I could test it on my side), otherwise I will take a look at
> > it later. The driver has essentially exactly the same structure.
> >
> > Reviewed-by: Paul Kocialkowski <[email protected]>
> > Tested-by: Paul Kocialkowski <[email protected]>
>
> This is a gentle ping for this patch.
> As far as I understand, the patch has received the required reviews and testing.
> If there are no remaining issues, could it be considered for merging?

This patch currently has no delegate in patchwork (you were the
delegate on an earlier version).
Link: https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/

I appreciate your feedback.

> > All the best,
> >
> > Paul
> >
> > > ---
> > > Changes in v3:
> > >     - link to v2: https://patchwork.kernel.org/project/linux-media/patch/[email protected]/
> > >     - Keep error path jumping to error_v4l2_notifier_cleanup on
> > >     bridge setup failure
> > >
> > > Changes in v2:
> > >     - link to v1: https://patchwork.kernel.org/project/linux-media/patch/[email protected]/
> > >     - Simplify control flow by dropping the else at end of s_stream()
> > >     - Call v4l2_subdev_cleanup() on bridge setup failure before
> > >     notifier registration
> > >
> > >  .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c   | 107 +++++++++---------
> > >  .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h   |   2 -
> > >  2 files changed, 53 insertions(+), 56 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c
> > > index b06cb73015cd..682bdd82098c 100644
> > > --- a/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c
> > > +++ b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c
> > > @@ -95,12 +95,12 @@ static void sun6i_mipi_csi2_disable(struct sun6i_mipi_csi2_device *csi2_dev)
> > >                          SUN6I_MIPI_CSI2_CTL_EN, 0);
> > >  }
> > >
> > > -static void sun6i_mipi_csi2_configure(struct sun6i_mipi_csi2_device *csi2_dev)
> > > +static void sun6i_mipi_csi2_configure(struct sun6i_mipi_csi2_device *csi2_dev,
> > > +                                    const struct v4l2_mbus_framefmt *mbus_format)
> > >  {
> > >       struct regmap *regmap = csi2_dev->regmap;
> > >       unsigned int lanes_count =
> > >               csi2_dev->bridge.endpoint.bus.mipi_csi2.num_data_lanes;
> > > -     struct v4l2_mbus_framefmt *mbus_format = &csi2_dev->bridge.mbus_format;
> > >       const struct sun6i_mipi_csi2_format *format;
> > >       struct device *dev = csi2_dev->dev;
> > >       u32 version = 0;
> > > @@ -173,7 +173,8 @@ static int sun6i_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
> > >       struct v4l2_subdev *source_subdev = csi2_dev->bridge.source_subdev;
> > >       union phy_configure_opts dphy_opts = { 0 };
> > >       struct phy_configure_opts_mipi_dphy *dphy_cfg = &dphy_opts.mipi_dphy;
> > > -     struct v4l2_mbus_framefmt *mbus_format = &csi2_dev->bridge.mbus_format;
> > > +     struct v4l2_subdev_state *state;
> > > +     const struct v4l2_mbus_framefmt *mbus_format;
> > >       const struct sun6i_mipi_csi2_format *format;
> > >       struct phy *dphy = csi2_dev->dphy;
> > >       struct device *dev = csi2_dev->dev;
> > > @@ -183,8 +184,12 @@ static int sun6i_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
> > >       unsigned long pixel_rate;
> > >       int ret;
> > >
> > > -     if (!source_subdev)
> > > -             return -ENODEV;
> > > +     state = v4l2_subdev_lock_and_get_active_state(subdev);
> > > +
> > > +     if (!source_subdev) {
> > > +             ret = -ENODEV;
> > > +             goto unlock;
> > > +     }
> > >
> > >       if (!on) {
> > >               v4l2_subdev_call(source_subdev, video, s_stream, 0);
> > > @@ -196,7 +201,7 @@ static int sun6i_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
> > >
> > >       ret = pm_runtime_resume_and_get(dev);
> > >       if (ret < 0)
> > > -             return ret;
> > > +             goto unlock;
> > >
> > >       /* Sensor Pixel Rate */
> > >
> > > @@ -222,6 +227,8 @@ static int sun6i_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
> > >               goto error_pm;
> > >       }
> > >
> > > +     mbus_format = v4l2_subdev_state_get_format(state,
> > > +                                                SUN6I_MIPI_CSI2_PAD_SINK);
> > >       format = sun6i_mipi_csi2_format_find(mbus_format->code);
> > >       if (WARN_ON(!format)) {
> > >               ret = -ENODEV;
> > > @@ -260,7 +267,7 @@ static int sun6i_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
> > >
> > >       /* Controller */
> > >
> > > -     sun6i_mipi_csi2_configure(csi2_dev);
> > > +     sun6i_mipi_csi2_configure(csi2_dev, mbus_format);
> > >       sun6i_mipi_csi2_enable(csi2_dev);
> > >
> > >       /* D-PHY */
> > > @@ -277,7 +284,8 @@ static int sun6i_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
> > >       if (ret && ret != -ENOIOCTLCMD)
> > >               goto disable;
> > >
> > > -     return 0;
> > > +     ret = 0;
> > > +     goto unlock;
> > >
> > >  disable:
> > >       phy_power_off(dphy);
> > > @@ -286,6 +294,8 @@ static int sun6i_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
> > >  error_pm:
> > >       pm_runtime_put(dev);
> > >
> > > +unlock:
> > > +     v4l2_subdev_unlock_state(state);
> > >       return ret;
> > >  }
> > >
> > > @@ -308,21 +318,23 @@ sun6i_mipi_csi2_mbus_format_prepare(struct v4l2_mbus_framefmt *mbus_format)
> > >  static int sun6i_mipi_csi2_init_state(struct v4l2_subdev *subdev,
> > >                                     struct v4l2_subdev_state *state)
> > >  {
> > > -     struct sun6i_mipi_csi2_device *csi2_dev = v4l2_get_subdevdata(subdev);
> > > -     unsigned int pad = SUN6I_MIPI_CSI2_PAD_SINK;
> > > -     struct v4l2_mbus_framefmt *mbus_format =
> > > -             v4l2_subdev_state_get_format(state, pad);
> > > -     struct mutex *lock = &csi2_dev->bridge.lock;
> > > +     unsigned int pad;
> > >
> > > -     mutex_lock(lock);
> > > +     /*
> > > +      * This subdev does not perform format conversion,
> > > +      * initialize both pads identically.
> > > +      */
> > > +     for (pad = 0; pad < subdev->entity.num_pads; pad++) {
> > > +             struct v4l2_mbus_framefmt *mbus_format;
> > >
> > > -     mbus_format->code = sun6i_mipi_csi2_formats[0].mbus_code;
> > > -     mbus_format->width = 640;
> > > -     mbus_format->height = 480;
> > > +             mbus_format = v4l2_subdev_state_get_format(state, pad);
> > >
> > > -     sun6i_mipi_csi2_mbus_format_prepare(mbus_format);
> > > +             mbus_format->code = sun6i_mipi_csi2_formats[0].mbus_code;
> > > +             mbus_format->width = 640;
> > > +             mbus_format->height = 480;
> > >
> > > -     mutex_unlock(lock);
> > > +             sun6i_mipi_csi2_mbus_format_prepare(mbus_format);
> > > +     }
> > >
> > >       return 0;
> > >  }
> > > @@ -340,53 +352,32 @@ sun6i_mipi_csi2_enum_mbus_code(struct v4l2_subdev *subdev,
> > >       return 0;
> > >  }
> > >
> > > -static int sun6i_mipi_csi2_get_fmt(struct v4l2_subdev *subdev,
> > > -                                struct v4l2_subdev_state *state,
> > > -                                struct v4l2_subdev_format *format)
> > > -{
> > > -     struct sun6i_mipi_csi2_device *csi2_dev = v4l2_get_subdevdata(subdev);
> > > -     struct v4l2_mbus_framefmt *mbus_format = &format->format;
> > > -     struct mutex *lock = &csi2_dev->bridge.lock;
> > > -
> > > -     mutex_lock(lock);
> > > -
> > > -     if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> > > -             *mbus_format = *v4l2_subdev_state_get_format(state,
> > > -                                                          format->pad);
> > > -     else
> > > -             *mbus_format = csi2_dev->bridge.mbus_format;
> > > -
> > > -     mutex_unlock(lock);
> > > -
> > > -     return 0;
> > > -}
> > > -
> > >  static int sun6i_mipi_csi2_set_fmt(struct v4l2_subdev *subdev,
> > >                                  struct v4l2_subdev_state *state,
> > >                                  struct v4l2_subdev_format *format)
> > >  {
> > > -     struct sun6i_mipi_csi2_device *csi2_dev = v4l2_get_subdevdata(subdev);
> > > -     struct v4l2_mbus_framefmt *mbus_format = &format->format;
> > > -     struct mutex *lock = &csi2_dev->bridge.lock;
> > > +     struct v4l2_mbus_framefmt *fmt;
> > >
> > > -     mutex_lock(lock);
> > > +     /* The format on the source pad always matches the sink pad. */
> > > +     if (format->pad != SUN6I_MIPI_CSI2_PAD_SINK)
> > > +             return v4l2_subdev_get_fmt(subdev, state, format);
> > >
> > > -     sun6i_mipi_csi2_mbus_format_prepare(mbus_format);
> > > +     sun6i_mipi_csi2_mbus_format_prepare(&format->format);
> > >
> > > -     if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> > > -             *v4l2_subdev_state_get_format(state, format->pad) =
> > > -                     *mbus_format;
> > > -     else
> > > -             csi2_dev->bridge.mbus_format = *mbus_format;
> > > +     /* Set the format on the sink pad. */
> > > +     fmt = v4l2_subdev_state_get_format(state, format->pad);
> > > +     *fmt = format->format;
> > >
> > > -     mutex_unlock(lock);
> > > +     /* Propagate the format to the source pad. */
> > > +     fmt = v4l2_subdev_state_get_format(state, SUN6I_MIPI_CSI2_PAD_SOURCE);
> > > +     *fmt = format->format;
> > >
> > >       return 0;
> > >  }
> > >
> > >  static const struct v4l2_subdev_pad_ops sun6i_mipi_csi2_pad_ops = {
> > >       .enum_mbus_code = sun6i_mipi_csi2_enum_mbus_code,
> > > -     .get_fmt        = sun6i_mipi_csi2_get_fmt,
> > > +     .get_fmt        = v4l2_subdev_get_fmt,
> > >       .set_fmt        = sun6i_mipi_csi2_set_fmt,
> > >  };
> > >
> > > @@ -502,8 +493,6 @@ static int sun6i_mipi_csi2_bridge_setup(struct sun6i_mipi_csi2_device *csi2_dev)
> > >       bool notifier_registered = false;
> > >       int ret;
> > >
> > > -     mutex_init(&bridge->lock);
> > > -
> > >       /* V4L2 Subdev */
> > >
> > >       v4l2_subdev_init(subdev, &sun6i_mipi_csi2_subdev_ops);
> > > @@ -532,6 +521,12 @@ static int sun6i_mipi_csi2_bridge_setup(struct sun6i_mipi_csi2_device *csi2_dev)
> > >       if (ret)
> > >               return ret;
> > >
> > > +     /* V4L2 Subdev finalize */
> > > +
> > > +     ret = v4l2_subdev_init_finalize(subdev);
> > > +     if (ret < 0)
> > > +             goto error_media_entity_cleanup;
> > > +
> > >       /* V4L2 Async */
> > >
> > >       v4l2_async_subdev_nf_init(notifier, subdev);
> > > @@ -565,6 +560,9 @@ static int sun6i_mipi_csi2_bridge_setup(struct sun6i_mipi_csi2_device *csi2_dev)
> > >  error_v4l2_notifier_cleanup:
> > >       v4l2_async_nf_cleanup(notifier);
> > >
> > > +     v4l2_subdev_cleanup(subdev);
> > > +
> > > +error_media_entity_cleanup:
> > >       media_entity_cleanup(&subdev->entity);
> > >
> > >       return ret;
> > > @@ -579,6 +577,7 @@ sun6i_mipi_csi2_bridge_cleanup(struct sun6i_mipi_csi2_device *csi2_dev)
> > >       v4l2_async_unregister_subdev(subdev);
> > >       v4l2_async_nf_unregister(notifier);
> > >       v4l2_async_nf_cleanup(notifier);
> > > +     v4l2_subdev_cleanup(subdev);
> > >       media_entity_cleanup(&subdev->entity);
> > >  }
> > >
> > > diff --git a/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h
> > > index 24b15e34b5e8..d72dfbd6a993 100644
> > > --- a/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h
> > > +++ b/drivers/media/platform/sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.h
> > > @@ -32,8 +32,6 @@ struct sun6i_mipi_csi2_bridge {
> > >       struct media_pad                pads[SUN6I_MIPI_CSI2_PAD_COUNT];
> > >       struct v4l2_fwnode_endpoint     endpoint;
> > >       struct v4l2_async_notifier      notifier;
> > > -     struct v4l2_mbus_framefmt       mbus_format;
> > > -     struct mutex                    lock; /* Mbus format lock. */
> > >
> > >       struct v4l2_subdev              *source_subdev;
> > >  };
> > > --
> > > 2.34.1

-- 
Regards,
Arash Golgol