Re: [PATCH v2 30/66] media: sun6i-csi: Add bridge v4l2 subdev with port management

[Sakari Ailus]

Hi Paul,

On Sat, Feb 05, 2022 at 07:53:53PM +0100, Paul Kocialkowski wrote:
> Introduce a bridge v4l2 subdev to prepare for separation between the
> processing part (bridge) and the dma engine, which is required to
> properly support ths isp workflow later on.
> 
> Currently the bridge just manages fwnode mapping to media pads,
> using an async notifier (which was previously in the main code).
> The s_stream video op just forwards to the connected v4l2 subdev
> (sensor or MIPI CSI-2 bridge).
> 
> The video capture device is now registered after the bridge and
> attaches to it with a media link.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> ---
>  .../media/platform/sunxi/sun6i-csi/Makefile   |   2 +-
>  .../platform/sunxi/sun6i-csi/sun6i_csi.c      | 156 +-----
>  .../platform/sunxi/sun6i-csi/sun6i_csi.h      |  12 +-
>  .../sunxi/sun6i-csi/sun6i_csi_bridge.c        | 473 ++++++++++++++++++
>  .../sunxi/sun6i-csi/sun6i_csi_bridge.h        |  44 ++
>  .../platform/sunxi/sun6i-csi/sun6i_video.c    |  19 +
>  6 files changed, 571 insertions(+), 135 deletions(-)
>  create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c
>  create mode 100644 drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.h
> 
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/Makefile b/drivers/media/platform/sunxi/sun6i-csi/Makefile
> index e7e315347804..7a699580a641 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/Makefile
> +++ b/drivers/media/platform/sunxi/sun6i-csi/Makefile
> @@ -1,4 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -sun6i-csi-y += sun6i_video.o sun6i_csi.o
> +sun6i-csi-y += sun6i_video.o sun6i_csi.o sun6i_csi_bridge.o
>  
>  obj-$(CONFIG_VIDEO_SUN6I_CSI) += sun6i-csi.o
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> index c8fe31cc38b5..a1847ae3e88e 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> @@ -34,16 +34,17 @@
>  bool sun6i_csi_is_format_supported(struct sun6i_csi_device *csi_dev,
>  				   u32 pixformat, u32 mbus_code)
>  {
> -	struct sun6i_csi_v4l2 *v4l2 = &csi_dev->v4l2;
> +	struct v4l2_fwnode_endpoint *endpoint =
> +		&csi_dev->bridge.source->endpoint;
>  
>  	/*
>  	 * Some video receivers have the ability to be compatible with
>  	 * 8bit and 16bit bus width.
>  	 * Identify the media bus format from device tree.
>  	 */
> -	if ((v4l2->v4l2_ep.bus_type == V4L2_MBUS_PARALLEL
> -	     || v4l2->v4l2_ep.bus_type == V4L2_MBUS_BT656)
> -	     && v4l2->v4l2_ep.bus.parallel.bus_width == 16) {
> +	if ((endpoint->bus_type == V4L2_MBUS_PARALLEL
> +	     || endpoint->bus_type == V4L2_MBUS_BT656)
> +	     && endpoint->bus.parallel.bus_width == 16) {
>  		switch (pixformat) {
>  		case V4L2_PIX_FMT_NV12_16L16:
>  		case V4L2_PIX_FMT_NV12:
> @@ -328,7 +329,8 @@ static enum csi_input_seq get_csi_input_seq(struct sun6i_csi_device *csi_dev,
>  
>  static void sun6i_csi_setup_bus(struct sun6i_csi_device *csi_dev)
>  {
> -	struct v4l2_fwnode_endpoint *endpoint = &csi_dev->v4l2.v4l2_ep;
> +	struct v4l2_fwnode_endpoint *endpoint =
> +		&csi_dev->bridge.source->endpoint;
>  	struct sun6i_csi_config *config = &csi_dev->config;
>  	unsigned char bus_width;
>  	u32 flags;
> @@ -583,95 +585,11 @@ static const struct media_device_ops sun6i_csi_media_ops = {
>  
>  /* V4L2 */
>  
> -static int sun6i_csi_link_entity(struct sun6i_csi_device *csi_dev,
> -				 struct media_entity *entity,
> -				 struct fwnode_handle *fwnode)
> +int sun6i_csi_v4l2_complete(struct sun6i_csi_device *csi_dev)
>  {
> -	struct media_entity *sink;
> -	struct media_pad *sink_pad;
> -	int src_pad_index;
> -	int ret;
> -
> -	ret = media_entity_get_fwnode_pad(entity, fwnode, MEDIA_PAD_FL_SOURCE);
> -	if (ret < 0) {
> -		dev_err(csi_dev->dev,
> -			"%s: no source pad in external entity %s\n", __func__,
> -			entity->name);
> -		return -EINVAL;
> -	}
> -
> -	src_pad_index = ret;
> -
> -	sink = &csi_dev->video.video_dev.entity;
> -	sink_pad = &csi_dev->video.pad;
> -
> -	dev_dbg(csi_dev->dev, "creating %s:%u -> %s:%u link\n",
> -		entity->name, src_pad_index, sink->name, sink_pad->index);
> -	ret = media_create_pad_link(entity, src_pad_index, sink,
> -				    sink_pad->index,
> -				    MEDIA_LNK_FL_ENABLED |
> -				    MEDIA_LNK_FL_IMMUTABLE);
> -	if (ret < 0) {
> -		dev_err(csi_dev->dev, "failed to create %s:%u -> %s:%u link\n",
> -			entity->name, src_pad_index,
> -			sink->name, sink_pad->index);
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
> -static int sun6i_subdev_notify_complete(struct v4l2_async_notifier *notifier)
> -{
> -	struct sun6i_csi_device *csi_dev =
> -		container_of(notifier, struct sun6i_csi_device,
> -			     v4l2.notifier);
> -	struct sun6i_csi_v4l2 *v4l2 = &csi_dev->v4l2;
> -	struct v4l2_device *v4l2_dev = &v4l2->v4l2_dev;
> -	struct v4l2_subdev *sd;
> -	int ret;
> -
> -	dev_dbg(csi_dev->dev, "notify complete, all subdevs registered\n");
> -
> -	sd = list_first_entry(&v4l2_dev->subdevs, struct v4l2_subdev, list);
> -	if (!sd)
> -		return -EINVAL;
> -
> -	ret = sun6i_csi_link_entity(csi_dev, &sd->entity, sd->fwnode);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = v4l2_device_register_subdev_nodes(v4l2_dev);
> -	if (ret < 0)
> -		return ret;
> -
> -	return 0;
> -}
> -
> -static const struct v4l2_async_notifier_operations sun6i_csi_async_ops = {
> -	.complete = sun6i_subdev_notify_complete,
> -};
> -
> -static int sun6i_csi_fwnode_parse(struct device *dev,
> -				  struct v4l2_fwnode_endpoint *vep,
> -				  struct v4l2_async_subdev *asd)
> -{
> -	struct sun6i_csi_device *csi_dev = dev_get_drvdata(dev);
> +	struct v4l2_device *v4l2_dev = &csi_dev->v4l2.v4l2_dev;
>  
> -	if (vep->base.port || vep->base.id) {
> -		dev_warn(dev, "Only support a single port with one endpoint\n");
> -		return -ENOTCONN;
> -	}
> -
> -	switch (vep->bus_type) {
> -	case V4L2_MBUS_PARALLEL:
> -	case V4L2_MBUS_BT656:
> -		csi_dev->v4l2.v4l2_ep = *vep;
> -		return 0;
> -	default:
> -		dev_err(dev, "Unsupported media bus type\n");
> -		return -ENOTCONN;
> -	}
> +	return v4l2_device_register_subdev_nodes(v4l2_dev);
>  }
>  
>  static int sun6i_csi_v4l2_setup(struct sun6i_csi_device *csi_dev)
> @@ -679,7 +597,6 @@ static int sun6i_csi_v4l2_setup(struct sun6i_csi_device *csi_dev)
>  	struct sun6i_csi_v4l2 *v4l2 = &csi_dev->v4l2;
>  	struct media_device *media_dev = &v4l2->media_dev;
>  	struct v4l2_device *v4l2_dev = &v4l2->v4l2_dev;
> -	struct v4l2_async_notifier *notifier = &v4l2->notifier;
>  	struct device *dev = csi_dev->dev;
>  	int ret;
>  
> @@ -720,42 +637,8 @@ static int sun6i_csi_v4l2_setup(struct sun6i_csi_device *csi_dev)
>  		goto error_v4l2_ctrl;
>  	}
>  
> -	/* Video */
> -
> -	ret = sun6i_video_setup(csi_dev);
> -	if (ret)
> -		goto error_v4l2_device;
> -
> -	/* V4L2 Async */
> -
> -	v4l2_async_nf_init(notifier);
> -	notifier->ops = &sun6i_csi_async_ops;
> -
> -	ret = v4l2_async_nf_parse_fwnode_endpoints(dev, notifier,
> -						   sizeof(struct
> -							  v4l2_async_subdev),
> -						   sun6i_csi_fwnode_parse);
> -	if (ret)
> -		goto error_video;
> -
> -	ret = v4l2_async_nf_register(v4l2_dev, notifier);
> -	if (ret) {
> -		dev_err(dev, "failed to register v4l2 async notifier: %d\n",
> -			ret);
> -		goto error_v4l2_async_notifier;
> -	}
> -
>  	return 0;
>  
> -error_v4l2_async_notifier:
> -	v4l2_async_nf_cleanup(notifier);
> -
> -error_video:
> -	sun6i_video_cleanup(csi_dev);
> -
> -error_v4l2_device:
> -	v4l2_device_unregister(&v4l2->v4l2_dev);
> -
>  error_v4l2_ctrl:
>  	v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
>  
> @@ -771,9 +654,6 @@ static void sun6i_csi_v4l2_cleanup(struct sun6i_csi_device *csi_dev)
>  	struct sun6i_csi_v4l2 *v4l2 = &csi_dev->v4l2;
>  
>  	media_device_unregister(&v4l2->media_dev);
> -	v4l2_async_nf_unregister(&v4l2->notifier);
> -	v4l2_async_nf_cleanup(&v4l2->notifier);
> -	sun6i_video_cleanup(csi_dev);
>  	v4l2_device_unregister(&v4l2->v4l2_dev);
>  	v4l2_ctrl_handler_free(&v4l2->ctrl_handler);
>  	media_device_cleanup(&v4l2->media_dev);
> @@ -995,8 +875,22 @@ static int sun6i_csi_probe(struct platform_device *platform_dev)
>  	if (ret)
>  		goto error_resources;
>  
> +	ret = sun6i_csi_bridge_setup(csi_dev);
> +	if (ret)
> +		goto error_v4l2;
> +
> +	ret = sun6i_video_setup(csi_dev);
> +	if (ret)
> +		goto error_bridge;
> +
>  	return 0;
>  
> +error_bridge:
> +	sun6i_csi_bridge_cleanup(csi_dev);
> +
> +error_v4l2:
> +	sun6i_csi_v4l2_cleanup(csi_dev);
> +
>  error_resources:
>  	sun6i_csi_resources_cleanup(csi_dev);
>  
> @@ -1007,6 +901,8 @@ static int sun6i_csi_remove(struct platform_device *pdev)
>  {
>  	struct sun6i_csi_device *csi_dev = platform_get_drvdata(pdev);
>  
> +	sun6i_video_cleanup(csi_dev);
> +	sun6i_csi_bridge_cleanup(csi_dev);
>  	sun6i_csi_v4l2_cleanup(csi_dev);
>  	sun6i_csi_resources_cleanup(csi_dev);
>  
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h
> index 4dd83e57bafa..576c7f10289e 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h
> @@ -13,11 +13,16 @@
>  #include <media/v4l2-fwnode.h>
>  #include <media/videobuf2-v4l2.h>
>  
> +#include "sun6i_csi_bridge.h"
>  #include "sun6i_video.h"
>  
>  #define SUN6I_CSI_NAME		"sun6i-csi"
>  #define SUN6I_CSI_DESCRIPTION	"Allwinner A31 CSI Device"
>  
> +enum sun6i_csi_port {
> +	SUN6I_CSI_PORT_PARALLEL		= 0,
> +};
> +
>  struct sun6i_csi_buffer {
>  	struct vb2_v4l2_buffer		v4l2_buffer;
>  	struct list_head		list;
> @@ -46,10 +51,6 @@ struct sun6i_csi_v4l2 {
>  	struct v4l2_device		v4l2_dev;
>  	struct v4l2_ctrl_handler	ctrl_handler;
>  	struct media_device		media_dev;
> -
> -	struct v4l2_async_notifier	notifier;
> -	/* video port settings */
> -	struct v4l2_fwnode_endpoint	v4l2_ep;
>  };
>  
>  struct sun6i_csi_device {
> @@ -57,6 +58,7 @@ struct sun6i_csi_device {
>  
>  	struct sun6i_csi_config		config;
>  	struct sun6i_csi_v4l2		v4l2;
> +	struct sun6i_csi_bridge		bridge;
>  	struct sun6i_video		video;
>  
>  	struct regmap			*regmap;
> @@ -156,4 +158,6 @@ static inline int sun6i_csi_get_bpp(unsigned int pixformat)
>  	return 0;
>  }
>  
> +int sun6i_csi_v4l2_complete(struct sun6i_csi_device *csi_dev);
> +
>  #endif /* __SUN6I_CSI_H__ */
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c
> new file mode 100644
> index 000000000000..74706d883359
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c
> @@ -0,0 +1,473 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2021-2022 Bootlin
> + * Author: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> + */
> +
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +
> +#include "sun6i_csi.h"
> +#include "sun6i_csi_bridge.h"
> +
> +/* Format */
> +
> +static const u32 sun6i_csi_bridge_mbus_codes[] = {
> +	/* Bayer */
> +	MEDIA_BUS_FMT_SBGGR8_1X8,
> +	MEDIA_BUS_FMT_SGBRG8_1X8,
> +	MEDIA_BUS_FMT_SGRBG8_1X8,
> +	MEDIA_BUS_FMT_SRGGB8_1X8,
> +	MEDIA_BUS_FMT_SBGGR10_1X10,
> +	MEDIA_BUS_FMT_SGBRG10_1X10,
> +	MEDIA_BUS_FMT_SGRBG10_1X10,
> +	MEDIA_BUS_FMT_SRGGB10_1X10,
> +	MEDIA_BUS_FMT_SBGGR12_1X12,
> +	MEDIA_BUS_FMT_SGBRG12_1X12,
> +	MEDIA_BUS_FMT_SGRBG12_1X12,
> +	MEDIA_BUS_FMT_SRGGB12_1X12,
> +	/* RGB */
> +	MEDIA_BUS_FMT_RGB565_2X8_LE,
> +	MEDIA_BUS_FMT_RGB565_2X8_BE,
> +	/* YUV422 */
> +	MEDIA_BUS_FMT_YUYV8_2X8,
> +	MEDIA_BUS_FMT_UYVY8_2X8,
> +	MEDIA_BUS_FMT_YVYU8_2X8,
> +	MEDIA_BUS_FMT_UYVY8_2X8,
> +	MEDIA_BUS_FMT_VYUY8_2X8,
> +	MEDIA_BUS_FMT_YUYV8_1X16,
> +	MEDIA_BUS_FMT_UYVY8_1X16,
> +	MEDIA_BUS_FMT_YVYU8_1X16,
> +	MEDIA_BUS_FMT_UYVY8_1X16,
> +	MEDIA_BUS_FMT_VYUY8_1X16,
> +	/* Compressed */
> +	MEDIA_BUS_FMT_JPEG_1X8,
> +};
> +
> +static bool sun6i_csi_bridge_mbus_code_check(u32 mbus_code)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(sun6i_csi_bridge_mbus_codes); i++)
> +		if (sun6i_csi_bridge_mbus_codes[i] == mbus_code)
> +			return true;
> +
> +	return false;
> +}
> +
> +/* V4L2 Subdev */
> +
> +static int sun6i_csi_bridge_s_stream(struct v4l2_subdev *subdev, int on)
> +{
> +	struct sun6i_csi_device *csi_dev = v4l2_get_subdevdata(subdev);
> +	struct v4l2_subdev *source_subdev;
> +	/* Initialize to 0 to use both in disable label (ret != 0) and off. */
> +	int ret = 0;
> +
> +	/* Source */
> +
> +	if (!csi_dev->bridge.source)
> +		return -ENODEV;
> +
> +	source_subdev = csi_dev->bridge.source->subdev;
> +
> +	if (!on) {
> +		v4l2_subdev_call(source_subdev, video, s_stream, 0);
> +		goto disable;
> +	}
> +
> +	ret = v4l2_subdev_call(source_subdev, video, s_stream, 1);
> +	if (ret && ret != -ENOIOCTLCMD)
> +		goto disable;
> +
> +	return 0;
> +
> +disable:
> +	csi_dev->bridge.source = NULL;
> +
> +	return ret;
> +}
> +
> +static const struct v4l2_subdev_video_ops sun6i_csi_bridge_video_ops = {
> +	.s_stream	= sun6i_csi_bridge_s_stream,
> +};
> +
> +static void
> +sun6i_csi_bridge_mbus_format_prepare(struct v4l2_mbus_framefmt *mbus_format)
> +{
> +	if (!sun6i_csi_bridge_mbus_code_check(mbus_format->code))
> +		mbus_format->code = sun6i_csi_bridge_mbus_codes[0];
> +
> +	mbus_format->field = V4L2_FIELD_NONE;
> +	mbus_format->colorspace = V4L2_COLORSPACE_RAW;
> +	mbus_format->quantization = V4L2_QUANTIZATION_DEFAULT;
> +	mbus_format->xfer_func = V4L2_XFER_FUNC_DEFAULT;
> +}
> +
> +static int sun6i_csi_bridge_init_cfg(struct v4l2_subdev *subdev,
> +				     struct v4l2_subdev_state *state)
> +{
> +	unsigned int pad = SUN6I_CSI_BRIDGE_PAD_SINK;
> +	struct v4l2_mbus_framefmt *mbus_format =
> +		v4l2_subdev_get_try_format(subdev, state, pad);
> +
> +	mbus_format->code = sun6i_csi_bridge_mbus_codes[0];
> +	mbus_format->width = 1280;
> +	mbus_format->height = 720;
> +
> +	sun6i_csi_bridge_mbus_format_prepare(mbus_format);
> +
> +	return 0;
> +}
> +
> +static int
> +sun6i_csi_bridge_enum_mbus_code(struct v4l2_subdev *subdev,
> +				struct v4l2_subdev_state *state,
> +				struct v4l2_subdev_mbus_code_enum *code_enum)
> +{
> +	if (code_enum->index >= ARRAY_SIZE(sun6i_csi_bridge_mbus_codes))
> +		return -EINVAL;
> +
> +	code_enum->code = sun6i_csi_bridge_mbus_codes[code_enum->index];
> +
> +	return 0;
> +}
> +
> +static int sun6i_csi_bridge_get_fmt(struct v4l2_subdev *subdev,
> +				    struct v4l2_subdev_state *state,
> +				    struct v4l2_subdev_format *format)
> +{
> +	struct sun6i_csi_device *csi_dev = v4l2_get_subdevdata(subdev);
> +	struct v4l2_mbus_framefmt *mbus_format = &format->format;
> +
> +	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> +		*mbus_format = *v4l2_subdev_get_try_format(subdev, state,
> +							   format->pad);
> +	else
> +		*mbus_format = csi_dev->bridge.mbus_format;
> +
> +	return 0;
> +}
> +
> +static int sun6i_csi_bridge_set_fmt(struct v4l2_subdev *subdev,
> +				    struct v4l2_subdev_state *state,
> +				    struct v4l2_subdev_format *format)
> +{
> +	struct sun6i_csi_device *csi_dev = v4l2_get_subdevdata(subdev);
> +	struct v4l2_mbus_framefmt *mbus_format = &format->format;
> +
> +	sun6i_csi_bridge_mbus_format_prepare(mbus_format);
> +
> +	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
> +		*v4l2_subdev_get_try_format(subdev, state, format->pad) =
> +			*mbus_format;
> +	else
> +		csi_dev->bridge.mbus_format = *mbus_format;

Note that the driver is responsible for serialising access to its data,
i.e. you have to acquire the mutex here.

> +
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_pad_ops sun6i_csi_bridge_pad_ops = {
> +	.init_cfg	= sun6i_csi_bridge_init_cfg,
> +	.enum_mbus_code	= sun6i_csi_bridge_enum_mbus_code,
> +	.get_fmt	= sun6i_csi_bridge_get_fmt,
> +	.set_fmt	= sun6i_csi_bridge_set_fmt,
> +};
> +
> +const struct v4l2_subdev_ops sun6i_csi_bridge_subdev_ops = {
> +	.video	= &sun6i_csi_bridge_video_ops,
> +	.pad	= &sun6i_csi_bridge_pad_ops,
> +};
> +
> +/* Media Entity */
> +
> +static int sun6i_csi_bridge_link_validate(struct media_link *link)
> +{
> +	struct v4l2_subdev *subdev =
> +		media_entity_to_v4l2_subdev(link->sink->entity);
> +	struct sun6i_csi_device *csi_dev = v4l2_get_subdevdata(subdev);
> +	struct sun6i_csi_bridge *bridge = &csi_dev->bridge;
> +	struct device *dev = csi_dev->dev;
> +	struct v4l2_subdev *source_subdev =
> +		media_entity_to_v4l2_subdev(link->source->entity);
> +	int ret;
> +
> +	/* Only support one enabled source at a time. */
> +	if (bridge->source) {
> +		dev_err(dev, "multiple sources are connected to the bridge\n");
> +		return -EBUSY;
> +	}
> +
> +	ret = v4l2_subdev_link_validate(link);
> +	if (ret)
> +		return ret;
> +
> +	if (source_subdev == bridge->source_parallel.subdev)
> +		bridge->source = &bridge->source_parallel;
> +	else

Useless use of else.

> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static const struct media_entity_operations sun6i_csi_bridge_entity_ops = {
> +	.link_validate	= sun6i_csi_bridge_link_validate,
> +};
> +
> +/* V4L2 Async */
> +
> +static int sun6i_csi_bridge_link(struct sun6i_csi_device *csi_dev,
> +				 int sink_pad_index,
> +				 struct v4l2_subdev *remote_subdev,
> +				 bool enabled)
> +{
> +	struct device *dev = csi_dev->dev;
> +	struct v4l2_subdev *subdev = &csi_dev->bridge.subdev;
> +	struct media_entity *sink_entity = &subdev->entity;
> +	struct media_entity *source_entity = &remote_subdev->entity;
> +	int source_pad_index;
> +	int ret;
> +
> +	/* Get the first remote source pad. */
> +	ret = media_entity_get_fwnode_pad(source_entity, remote_subdev->fwnode,
> +					  MEDIA_PAD_FL_SOURCE);
> +	if (ret < 0) {
> +		dev_err(dev, "missing source pad in external entity %s\n",
> +			source_entity->name);
> +		return -EINVAL;
> +	}
> +
> +	source_pad_index = ret;
> +
> +	dev_dbg(dev, "creating %s:%u -> %s:%u link\n", source_entity->name,
> +		source_pad_index, sink_entity->name, sink_pad_index);
> +
> +	ret = media_create_pad_link(source_entity, source_pad_index,
> +				    sink_entity, sink_pad_index,
> +				    enabled ? MEDIA_LNK_FL_ENABLED : 0);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to create %s:%u -> %s:%u link\n",
> +			source_entity->name, source_pad_index,
> +			sink_entity->name, sink_pad_index);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +sun6i_csi_bridge_notifier_bound(struct v4l2_async_notifier *notifier,
> +				struct v4l2_subdev *remote_subdev,
> +				struct v4l2_async_subdev *async_subdev)
> +{
> +	struct sun6i_csi_device *csi_dev =
> +		container_of(notifier, struct sun6i_csi_device,
> +			     bridge.notifier);
> +	struct sun6i_csi_bridge *bridge = &csi_dev->bridge;
> +	struct sun6i_csi_bridge_source *source = NULL;
> +	struct fwnode_handle *fwnode = dev_fwnode(csi_dev->dev);
> +	struct fwnode_handle *handle = NULL;
> +	bool enabled;
> +	int ret;
> +
> +	while ((handle = fwnode_graph_get_next_endpoint(fwnode, handle))) {

I'd instead store the information you need here in struct sun6i_csi_bridge.
You could remove the loop here.

> +		struct fwnode_endpoint endpoint = { 0 };
> +		struct fwnode_handle *remote_fwnode;
> +
> +		remote_fwnode = fwnode_graph_get_remote_port_parent(handle);
> +		if (!remote_fwnode)
> +			continue;
> +
> +		if (remote_fwnode != remote_subdev->fwnode)
> +			goto next;
> +
> +		ret = fwnode_graph_parse_endpoint(handle, &endpoint);
> +		if (ret < 0)
> +			goto next;
> +
> +		switch (endpoint.port) {
> +		case SUN6I_CSI_PORT_PARALLEL:
> +			source = &bridge->source_parallel;
> +			enabled = true;
> +			break;
> +		default:
> +			break;
> +		}
> +
> +next:
> +		fwnode_handle_put(remote_fwnode);
> +	}
> +
> +	if (!source)
> +		return -EINVAL;
> +
> +	source->subdev = remote_subdev;
> +
> +	return sun6i_csi_bridge_link(csi_dev, SUN6I_CSI_BRIDGE_PAD_SINK,
> +				     remote_subdev, enabled);
> +}
> +
> +static int
> +sun6i_csi_bridge_notifier_complete(struct v4l2_async_notifier *notifier)
> +{
> +	struct sun6i_csi_device *csi_dev =
> +		container_of(notifier, struct sun6i_csi_device,
> +			     bridge.notifier);
> +
> +	return sun6i_csi_v4l2_complete(csi_dev);

You could call v4l2_device_register_subdev_nodes() here.

> +}
> +
> +static const struct v4l2_async_notifier_operations
> +sun6i_csi_bridge_notifier_ops = {
> +	.bound		= sun6i_csi_bridge_notifier_bound,
> +	.complete	= sun6i_csi_bridge_notifier_complete,
> +};
> +
> +/* Bridge */
> +
> +static int sun6i_csi_bridge_source_setup(struct sun6i_csi_device *csi_dev,
> +					 struct sun6i_csi_bridge_source *source,
> +					 u32 port,
> +					 enum v4l2_mbus_type *bus_types)
> +{
> +	struct device *dev = csi_dev->dev;
> +	struct v4l2_async_notifier *notifier = &csi_dev->bridge.notifier;
> +	struct v4l2_fwnode_endpoint *endpoint = &source->endpoint;
> +	struct v4l2_async_subdev *async_subdev;
> +	struct fwnode_handle *handle;
> +	int ret;
> +
> +	handle = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), port, 0, 0);
> +	if (!handle)
> +		return -ENODEV;
> +
> +	ret = v4l2_fwnode_endpoint_parse(handle, endpoint);
> +	if (ret)
> +		goto complete;
> +
> +	if (bus_types) {
> +		bool valid = false;
> +		unsigned int i;
> +
> +		for (i = 0; bus_types[i] != V4L2_MBUS_INVALID; i++) {
> +			if (endpoint->bus_type == bus_types[i]) {
> +				valid = true;
> +				break;
> +			}
> +		}
> +
> +		if (!valid) {
> +			dev_err(dev, "unsupported bus type for port %d\n",
> +				port);
> +			ret = -EINVAL;
> +			goto complete;
> +		}
> +	}
> +
> +	async_subdev = v4l2_async_nf_add_fwnode_remote(notifier, handle,
> +		struct v4l2_async_subdev);
> +	if (IS_ERR(async_subdev)) {
> +		ret = PTR_ERR(async_subdev);
> +		goto complete;
> +	}
> +
> +	source->expected = true;
> +
> +complete:
> +	fwnode_handle_put(handle);
> +
> +	return ret;
> +}
> +
> +int sun6i_csi_bridge_setup(struct sun6i_csi_device *csi_dev)
> +{
> +	struct device *dev = csi_dev->dev;
> +	struct sun6i_csi_bridge *bridge = &csi_dev->bridge;
> +	struct v4l2_device *v4l2_dev = &csi_dev->v4l2.v4l2_dev;
> +	struct v4l2_subdev *subdev = &bridge->subdev;
> +	struct v4l2_async_notifier *notifier = &bridge->notifier;
> +	struct media_pad *pads = bridge->pads;
> +	enum v4l2_mbus_type parallel_mbus_types[] = {
> +		V4L2_MBUS_PARALLEL,
> +		V4L2_MBUS_BT656,
> +		V4L2_MBUS_INVALID
> +	};
> +	int ret;
> +
> +	/* V4L2 Subdev */
> +
> +	v4l2_subdev_init(subdev, &sun6i_csi_bridge_subdev_ops);
> +	strscpy(subdev->name, SUN6I_CSI_BRIDGE_NAME, sizeof(subdev->name));
> +	subdev->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	subdev->owner = THIS_MODULE;
> +	subdev->dev = dev;
> +
> +	v4l2_set_subdevdata(subdev, csi_dev);
> +
> +	/* Media Entity */
> +
> +	subdev->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> +	subdev->entity.ops = &sun6i_csi_bridge_entity_ops;
> +
> +	/* Media Pads */
> +
> +	pads[SUN6I_CSI_BRIDGE_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> +	pads[SUN6I_CSI_BRIDGE_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE |
> +						  MEDIA_PAD_FL_MUST_CONNECT;
> +
> +	ret = media_entity_pads_init(&subdev->entity,
> +				     SUN6I_CSI_BRIDGE_PAD_COUNT, pads);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* V4L2 Subdev */
> +
> +	ret = v4l2_device_register_subdev(v4l2_dev, subdev);
> +	if (ret) {
> +		dev_err(dev, "failed to register v4l2 subdev: %d\n", ret);
> +		goto error_media_entity;
> +	}
> +
> +	/* V4L2 Async */
> +
> +	v4l2_async_nf_init(notifier);
> +	notifier->ops = &sun6i_csi_bridge_notifier_ops;
> +
> +	sun6i_csi_bridge_source_setup(csi_dev, &bridge->source_parallel,
> +				      SUN6I_CSI_PORT_PARALLEL,
> +				      parallel_mbus_types);
> +
> +	ret = v4l2_async_nf_register(v4l2_dev, notifier);
> +	if (ret) {
> +		dev_err(dev, "failed to register v4l2 async notifier: %d\n",
> +			ret);
> +		goto error_v4l2_async_notifier;
> +	}
> +
> +	return 0;
> +
> +error_v4l2_async_notifier:
> +	v4l2_async_nf_cleanup(notifier);
> +
> +	v4l2_device_unregister_subdev(subdev);
> +
> +error_media_entity:
> +	media_entity_cleanup(&subdev->entity);
> +
> +	return ret;
> +}
> +
> +void sun6i_csi_bridge_cleanup(struct sun6i_csi_device *csi_dev)
> +{
> +	struct v4l2_subdev *subdev = &csi_dev->bridge.subdev;
> +	struct v4l2_async_notifier *notifier = &csi_dev->bridge.notifier;
> +
> +	v4l2_async_nf_unregister(notifier);
> +	v4l2_async_nf_cleanup(notifier);
> +
> +	v4l2_device_unregister_subdev(subdev);
> +
> +	media_entity_cleanup(&subdev->entity);
> +}
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.h b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.h
> new file mode 100644
> index 000000000000..2ee7878102b6
> --- /dev/null
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright 2021 Bootlin

2022?

> + * Author: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> + */
> +
> +#ifndef _SUN6I_CSI_BRIDGE_H_
> +#define _SUN6I_CSI_BRIDGE_H_
> +
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +
> +#define SUN6I_CSI_BRIDGE_NAME	"sun6i-csi-bridge"
> +
> +enum sun6i_csi_bridge_pad {
> +	SUN6I_CSI_BRIDGE_PAD_SINK	= 0,
> +	SUN6I_CSI_BRIDGE_PAD_SOURCE	= 1,
> +	SUN6I_CSI_BRIDGE_PAD_COUNT	= 2,
> +};
> +
> +struct sun6i_csi_device;
> +
> +struct sun6i_csi_bridge_source {
> +	struct v4l2_subdev		*subdev;
> +	struct v4l2_fwnode_endpoint	endpoint;
> +	bool				expected;
> +};
> +
> +struct sun6i_csi_bridge {
> +	struct v4l2_subdev		subdev;
> +	struct v4l2_async_notifier	notifier;
> +	struct media_pad		pads[2];
> +	struct v4l2_mbus_framefmt	mbus_format;
> +
> +	struct sun6i_csi_bridge_source	source_parallel;
> +	struct sun6i_csi_bridge_source	*source;
> +};
> +
> +/* Bridge */
> +
> +int sun6i_csi_bridge_setup(struct sun6i_csi_device *csi_dev);
> +void sun6i_csi_bridge_cleanup(struct sun6i_csi_device *csi_dev);
> +
> +#endif
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
> index d32ff6b81f8a..fa5bf3697ace 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
> @@ -632,6 +632,7 @@ int sun6i_video_setup(struct sun6i_csi_device *csi_dev)
>  {
>  	struct sun6i_video *video = &csi_dev->video;
>  	struct v4l2_device *v4l2_dev = &csi_dev->v4l2.v4l2_dev;
> +	struct v4l2_subdev *bridge_subdev = &csi_dev->bridge.subdev;
>  	struct video_device *video_dev = &video->video_dev;
>  	struct vb2_queue *queue = &video->queue;
>  	struct media_pad *pad = &video->pad;
> @@ -715,8 +716,26 @@ int sun6i_video_setup(struct sun6i_csi_device *csi_dev)
>  	v4l2_info(v4l2_dev, "device %s registered as %s\n", video_dev->name,
>  		  video_device_node_name(video_dev));
>  
> +	/* Media Pad Link */
> +
> +	ret = media_create_pad_link(&bridge_subdev->entity,
> +				    SUN6I_CSI_BRIDGE_PAD_SOURCE,
> +				    &video_dev->entity, 0,
> +				    MEDIA_LNK_FL_ENABLED |
> +				    MEDIA_LNK_FL_IMMUTABLE);
> +	if (ret < 0) {
> +		v4l2_err(v4l2_dev, "failed to create %s:%u -> %s:%u link\n",
> +			 bridge_subdev->entity.name,
> +			 SUN6I_CSI_BRIDGE_PAD_SOURCE,
> +			 video_dev->entity.name, 0);
> +		goto error_video_device;
> +	}
> +
>  	return 0;
>  
> +error_video_device:
> +	vb2_video_unregister_device(video_dev);
> +
>  error_media_entity:
>  	media_entity_cleanup(&video_dev->entity);
>  

-- 
Sakari Ailus