From: Paul Kocialkowski <paulk@sys-base.io>
To: Arash Golgol <arash.golgol@gmail.com>
Cc: linux-media@vger.kernel.org, mchehab@kernel.org, wens@kernel.org,
jernej.skrabec@gmail.com, samuel@sholland.org,
laurent.pinchart@ideasonboard.com, linux-sunxi@lists.linux.dev
Subject: Re: [PATCH v3] media: sun8i-a83t-mipi-csi2: Use V4L2 subdev active state
Date: Sun, 17 May 2026 23:18:03 +0200 [thread overview]
Message-ID: <agowixYIRIB7D3J3@collins> (raw)
In-Reply-To: <20260515173101.8978-1-arash.golgol@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 10665 bytes --]
Hi Arash,
Le Fri 15 May 26, 21:01, 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 sun8i-a83t-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.
Looks good to me and works well on the hardware!
Thanks again for your work.
Reviewed-by: Paul Kocialkowski <paulk@sys-base.io>
Tested-by: Paul Kocialkowski <paulk@sys-base.io>
All the best,
Paul
> Signed-off-by: Arash Golgol <arash.golgol@gmail.com>
> ---
> Changes in v3:
> - Fix active state lock leak on runtime PM error path
>
> Changes in v2:
> - Initialize active state before calling v4l2_subdev_state_get_format()
> - Fix line wrapping reported by checkpatch
> - Link to media-ci report: https://linux-media.pages.freedesktop.org/-/users/patchwork/-/jobs/99865145/artifacts/report.htm
>
> .../sun8i_a83t_mipi_csi2.c | 113 +++++++++---------
> .../sun8i_a83t_mipi_csi2.h | 2 -
> 2 files changed, 56 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c b/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c
> index dbc51daa4fe3..2b7635f3952d 100644
> --- a/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c
> +++ b/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.c
> @@ -144,12 +144,12 @@ sun8i_a83t_mipi_csi2_disable(struct sun8i_a83t_mipi_csi2_device *csi2_dev)
> }
>
> static void
> -sun8i_a83t_mipi_csi2_configure(struct sun8i_a83t_mipi_csi2_device *csi2_dev)
> +sun8i_a83t_mipi_csi2_configure(struct sun8i_a83t_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 sun8i_a83t_mipi_csi2_format *format;
> struct device *dev = csi2_dev->dev;
> u32 version = 0;
> @@ -205,7 +205,8 @@ static int sun8i_a83t_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 sun8i_a83t_mipi_csi2_format *format;
> struct phy *dphy = csi2_dev->dphy;
> struct device *dev = csi2_dev->dev;
> @@ -215,8 +216,12 @@ static int sun8i_a83t_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);
> @@ -228,7 +233,7 @@ static int sun8i_a83t_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 */
>
> @@ -254,6 +259,9 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
> goto error_pm;
> }
>
> + mbus_format =
> + v4l2_subdev_state_get_format(state,
> + SUN8I_A83T_MIPI_CSI2_PAD_SINK);
> format = sun8i_a83t_mipi_csi2_format_find(mbus_format->code);
> if (WARN_ON(!format)) {
> ret = -ENODEV;
> @@ -292,7 +300,7 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
>
> /* Controller */
>
> - sun8i_a83t_mipi_csi2_configure(csi2_dev);
> + sun8i_a83t_mipi_csi2_configure(csi2_dev, mbus_format);
> sun8i_a83t_mipi_csi2_enable(csi2_dev);
>
> /* D-PHY */
> @@ -309,7 +317,8 @@ static int sun8i_a83t_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);
> @@ -318,6 +327,8 @@ static int sun8i_a83t_mipi_csi2_s_stream(struct v4l2_subdev *subdev, int on)
> error_pm:
> pm_runtime_put(dev);
>
> +unlock:
> + v4l2_subdev_unlock_state(state);
> return ret;
> }
>
> @@ -341,22 +352,24 @@ sun8i_a83t_mipi_csi2_mbus_format_prepare(struct v4l2_mbus_framefmt *mbus_format)
> static int sun8i_a83t_mipi_csi2_init_state(struct v4l2_subdev *subdev,
> struct v4l2_subdev_state *state)
> {
> - struct sun8i_a83t_mipi_csi2_device *csi2_dev =
> - v4l2_get_subdevdata(subdev);
> - unsigned int pad = SUN8I_A83T_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 = sun8i_a83t_mipi_csi2_formats[0].mbus_code;
> - mbus_format->width = 640;
> - mbus_format->height = 480;
> + mbus_format = v4l2_subdev_state_get_format(state, pad);
> +
> + mbus_format->code = sun8i_a83t_mipi_csi2_formats[0].mbus_code;
> + mbus_format->width = 640;
> + mbus_format->height = 480;
>
> - sun8i_a83t_mipi_csi2_mbus_format_prepare(mbus_format);
> + sun8i_a83t_mipi_csi2_mbus_format_prepare(mbus_format);
> + }
>
> - mutex_unlock(lock);
>
> return 0;
> }
> @@ -375,55 +388,33 @@ sun8i_a83t_mipi_csi2_enum_mbus_code(struct v4l2_subdev *subdev,
> return 0;
> }
>
> -static int sun8i_a83t_mipi_csi2_get_fmt(struct v4l2_subdev *subdev,
> - struct v4l2_subdev_state *state,
> - struct v4l2_subdev_format *format)
> -{
> - struct sun8i_a83t_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 sun8i_a83t_mipi_csi2_set_fmt(struct v4l2_subdev *subdev,
> struct v4l2_subdev_state *state,
> struct v4l2_subdev_format *format)
> {
> - struct sun8i_a83t_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 != SUN8I_A83T_MIPI_CSI2_PAD_SINK)
> + return v4l2_subdev_get_fmt(subdev, state, format);
>
> - sun8i_a83t_mipi_csi2_mbus_format_prepare(mbus_format);
> + sun8i_a83t_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,
> + SUN8I_A83T_MIPI_CSI2_PAD_SOURCE);
> + *fmt = format->format;
>
> return 0;
> }
>
> static const struct v4l2_subdev_pad_ops sun8i_a83t_mipi_csi2_pad_ops = {
> .enum_mbus_code = sun8i_a83t_mipi_csi2_enum_mbus_code,
> - .get_fmt = sun8i_a83t_mipi_csi2_get_fmt,
> + .get_fmt = v4l2_subdev_get_fmt,
> .set_fmt = sun8i_a83t_mipi_csi2_set_fmt,
> };
>
> @@ -540,8 +531,6 @@ sun8i_a83t_mipi_csi2_bridge_setup(struct sun8i_a83t_mipi_csi2_device *csi2_dev)
> bool notifier_registered = false;
> int ret;
>
> - mutex_init(&bridge->lock);
> -
> /* V4L2 Subdev */
>
> v4l2_subdev_init(subdev, &sun8i_a83t_mipi_csi2_subdev_ops);
> @@ -570,6 +559,12 @@ sun8i_a83t_mipi_csi2_bridge_setup(struct sun8i_a83t_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);
> @@ -603,6 +598,9 @@ sun8i_a83t_mipi_csi2_bridge_setup(struct sun8i_a83t_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;
> @@ -617,6 +615,7 @@ sun8i_a83t_mipi_csi2_bridge_cleanup(struct sun8i_a83t_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/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.h b/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.h
> index f1e64c53434c..819527bcd64d 100644
> --- a/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.h
> +++ b/drivers/media/platform/sunxi/sun8i-a83t-mipi-csi2/sun8i_a83t_mipi_csi2.h
> @@ -33,8 +33,6 @@ struct sun8i_a83t_mipi_csi2_bridge {
> struct media_pad pads[SUN8I_A83T_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
>
--
Paul Kocialkowski,
Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/
Expert in multimedia, graphics and embedded hardware support with Linux.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
prev parent reply other threads:[~2026-05-17 21:18 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-15 17:31 [PATCH v3] media: sun8i-a83t-mipi-csi2: Use V4L2 subdev active state Arash Golgol
2026-05-17 21:18 ` Paul Kocialkowski [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=agowixYIRIB7D3J3@collins \
--to=paulk@sys-base.io \
--cc=arash.golgol@gmail.com \
--cc=jernej.skrabec@gmail.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=linux-sunxi@lists.linux.dev \
--cc=mchehab@kernel.org \
--cc=samuel@sholland.org \
--cc=wens@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.