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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox