[PATCH v3] media: sun8i-a83t-mipi-csi2: Use V4L2 subdev active state

[PATCH v3] media: sun8i-a83t-mipi-csi2: Use V4L2 subdev active state

[Arash Golgol]

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.

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

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

[Paul Kocialkowski]

[-- 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 --]

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

[arash golgol]

Hi,

On Mon, May 18, 2026 at 12:48 AM Paul Kocialkowski <paulk@sys-base.io> wrote:
>
> 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>

I'd like to gently follow up on this patch.

> 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.

-- 
Regards,
Arash Golgol