[PATCH v4 00/17] media: imx: Create media links in bound notifiers

[PATCH v4 00/17] media: imx: Create media links in bound notifiers

[Steve Longerbeam]

Move media link creation into the notifier bound callbacks in the
minimum set of subdevices required by imx (imx5/6/7 CSI,
imx6/7 MIPI CSI-2, and video mux).

History:

v4:
- Removed the endpoint parsing callback APIs from video-mux and imx drivers
  as suggested by Sakari, replacing with endpoint parsing local to the
  drivers and the use of v4l2_async_notifier_add_fwnode_remote_subdev().
  As a result convenience function v4l2_async_register_fwnode_subdev()
  is no longer used and is reverted.

v3:
- The changes to the default behaviour in media_entity_get_fwnode_pad(),
  and the fixes to current media drivers that call it inconsistently, have
  been put-off to another time. Instead this version implements the
  get_fwnode_pad operation where required in the imx and video-mux
  subdevices to make media link creation work correctly. The
  improvements to media_entity_get_fwnode_pad() can wait to another
  patch series.

v2:
- rename/move the notifier-to-state inlines in imx7-mipi-csis.c and
  imx7-media-csi.c, suggested by Rui Silva.
- rewrite imx_media_create_links() to only add the missing media links
  from the imx6 MIPI CSI-2 receiver.


Steve Longerbeam (17):
  media: entity: Pass entity to get_fwnode_pad operation
  media: video-mux: Parse information from firmware without using
    callbacks
  media: imx: Parse information from firmware without using callbacks
  Revert "media: v4l2-fwnode: Add a convenience function for registering
    subdevs with notifiers"
  media: imx: csi: Implement get_fwnode_pad op
  media: imx: mipi csi-2: Implement get_fwnode_pad op
  media: video-mux: Implement get_fwnode_pad op
  media: imx: Add imx_media_create_fwnode_pad_link()
  media: video-mux: Create media links in bound notifier
  media: imx: mipi csi-2: Create media links in bound notifier
  media: imx7: mipi csis: Create media links in bound notifier
  media: imx7: csi: Create media links in bound notifier
  media: imx: csi: Create media links in bound notifier
  media: imx: csi: Lookup upstream endpoint with
    imx_media_get_pad_fwnode
  media: imx: Create missing links from CSI-2 receiver
  media: imx: silence a couple debug messages
  media: imx: TODO: Remove media link creation todos

 drivers/media/mc/mc-entity.c                  |   2 +-
 drivers/media/platform/video-mux.c            | 185 ++++++++++++++++--
 drivers/media/v4l2-core/v4l2-fwnode.c         |  62 ------
 drivers/staging/media/imx/TODO                |  29 ---
 drivers/staging/media/imx/imx-media-csi.c     | 136 ++++++++-----
 .../staging/media/imx/imx-media-dev-common.c  |  50 ++---
 drivers/staging/media/imx/imx-media-dev.c     |   2 +-
 .../staging/media/imx/imx-media-internal-sd.c |   6 +-
 drivers/staging/media/imx/imx-media-of.c      | 114 -----------
 drivers/staging/media/imx/imx-media-utils.c   | 124 ++++++++++++
 drivers/staging/media/imx/imx-media.h         |   9 +-
 drivers/staging/media/imx/imx6-mipi-csi2.c    | 119 +++++++++--
 drivers/staging/media/imx/imx7-media-csi.c    | 100 +++++++---
 drivers/staging/media/imx/imx7-mipi-csis.c    | 105 +++++++---
 include/media/media-entity.h                  |   3 +-
 include/media/v4l2-fwnode.h                   |  38 ----
 16 files changed, 654 insertions(+), 430 deletions(-)

-- 
2.17.1

[PATCH v4 01/17] media: entity: Pass entity to get_fwnode_pad operation

[Steve Longerbeam]

Add a missing pointer to the entity in the media_entity operation
get_fwnode_pad. There are no implementers of this op yet, but a future
entity that does so will almost certainly need a reference to itself
to carry out the work.

Fixes: ae45cd5efc120 ("[media] media: entity: Add get_fwnode_pad entity
operation")
Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
 drivers/media/mc/mc-entity.c | 2 +-
 include/media/media-entity.h | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index 211279c5fd77..12b45e669bcc 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -386,7 +386,7 @@ int media_entity_get_fwnode_pad(struct media_entity *entity,
 	if (ret)
 		return ret;
 
-	ret = entity->ops->get_fwnode_pad(&endpoint);
+	ret = entity->ops->get_fwnode_pad(entity, &endpoint);
 	if (ret < 0)
 		return ret;
 
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 8cb2c504a05c..cde80ad029b7 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -212,7 +212,8 @@ struct media_pad {
  *    mutex held.
  */
 struct media_entity_operations {
-	int (*get_fwnode_pad)(struct fwnode_endpoint *endpoint);
+	int (*get_fwnode_pad)(struct media_entity *entity,
+			      struct fwnode_endpoint *endpoint);
 	int (*link_setup)(struct media_entity *entity,
 			  const struct media_pad *local,
 			  const struct media_pad *remote, u32 flags);
-- 
2.17.1

Re: [PATCH v4 01/17] media: entity: Pass entity to get_fwnode_pad operation

[Laurent Pinchart]

Hi Steve,

Thank you for the patch.

On Tue, Mar 03, 2020 at 03:42:40PM -0800, Steve Longerbeam wrote:
> Add a missing pointer to the entity in the media_entity operation
> get_fwnode_pad. There are no implementers of this op yet, but a future
> entity that does so will almost certainly need a reference to itself
> to carry out the work.
> 
> Fixes: ae45cd5efc120 ("[media] media: entity: Add get_fwnode_pad entity
> operation")
> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/mc/mc-entity.c | 2 +-
>  include/media/media-entity.h | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index 211279c5fd77..12b45e669bcc 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -386,7 +386,7 @@ int media_entity_get_fwnode_pad(struct media_entity *entity,
>  	if (ret)
>  		return ret;
>  
> -	ret = entity->ops->get_fwnode_pad(&endpoint);
> +	ret = entity->ops->get_fwnode_pad(entity, &endpoint);
>  	if (ret < 0)
>  		return ret;
>  
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 8cb2c504a05c..cde80ad029b7 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -212,7 +212,8 @@ struct media_pad {
>   *    mutex held.
>   */
>  struct media_entity_operations {
> -	int (*get_fwnode_pad)(struct fwnode_endpoint *endpoint);
> +	int (*get_fwnode_pad)(struct media_entity *entity,
> +			      struct fwnode_endpoint *endpoint);
>  	int (*link_setup)(struct media_entity *entity,
>  			  const struct media_pad *local,
>  			  const struct media_pad *remote, u32 flags);

-- 
Regards,

Laurent Pinchart

[PATCH v4 02/17] media: video-mux: Parse information from firmware without using callbacks

[Steve Longerbeam]

Instead of using the convenience function
v4l2_async_register_fwnode_subdev(), parse the video-mux input endpoints
and set up the async sub-devices without using callbacks. The video-mux
knows which ports it must parse (the input ports) and how to handle
unconnected remotes, so it makes the code simpler to transfer control
of endpoint parsing to the driver.

Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
 drivers/media/platform/video-mux.c | 70 ++++++++++++++++++++----------
 1 file changed, 46 insertions(+), 24 deletions(-)

diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
index ddd0e338f9e4..7b6c96a29aa5 100644
--- a/drivers/media/platform/video-mux.c
+++ b/drivers/media/platform/video-mux.c
@@ -21,6 +21,7 @@
 
 struct video_mux {
 	struct v4l2_subdev subdev;
+	struct v4l2_async_notifier notifier;
 	struct media_pad *pads;
 	struct v4l2_mbus_framefmt *format_mbus;
 	struct mux_control *mux;
@@ -330,36 +331,49 @@ static const struct v4l2_subdev_ops video_mux_subdev_ops = {
 	.video = &video_mux_subdev_video_ops,
 };
 
-static int video_mux_parse_endpoint(struct device *dev,
-				    struct v4l2_fwnode_endpoint *vep,
-				    struct v4l2_async_subdev *asd)
-{
-	/*
-	 * it's not an error if remote is missing on a video-mux
-	 * input port, return -ENOTCONN to skip this endpoint with
-	 * no error.
-	 */
-	return fwnode_device_is_available(asd->match.fwnode) ? 0 : -ENOTCONN;
-}
-
 static int video_mux_async_register(struct video_mux *vmux,
 				    unsigned int num_input_pads)
 {
-	unsigned int i, *ports;
+	unsigned int i;
 	int ret;
 
-	ports = kcalloc(num_input_pads, sizeof(*ports), GFP_KERNEL);
-	if (!ports)
-		return -ENOMEM;
-	for (i = 0; i < num_input_pads; i++)
-		ports[i] = i;
+	v4l2_async_notifier_init(&vmux->notifier);
 
-	ret = v4l2_async_register_fwnode_subdev(
-		&vmux->subdev, sizeof(struct v4l2_async_subdev),
-		ports, num_input_pads, video_mux_parse_endpoint);
+	for (i = 0; i < num_input_pads; i++) {
+		struct v4l2_async_subdev *asd;
+		struct fwnode_handle *ep;
 
-	kfree(ports);
-	return ret;
+		ep = fwnode_graph_get_endpoint_by_id(
+			dev_fwnode(vmux->subdev.dev), i, 0,
+			FWNODE_GRAPH_ENDPOINT_NEXT);
+		if (!ep)
+			continue;
+
+		asd = kzalloc(sizeof(*asd), GFP_KERNEL);
+		if (!asd) {
+			fwnode_handle_put(ep);
+			return -ENOMEM;
+		}
+
+		ret = v4l2_async_notifier_add_fwnode_remote_subdev(
+			&vmux->notifier, ep, asd);
+
+		fwnode_handle_put(ep);
+
+		if (ret) {
+			kfree(asd);
+			/* OK if asd already exists */
+			if (ret != -EEXIST)
+				return ret;
+		}
+	}
+
+	ret = v4l2_async_subdev_notifier_register(&vmux->subdev,
+						  &vmux->notifier);
+	if (ret)
+		return ret;
+
+	return v4l2_async_register_subdev(&vmux->subdev);
 }
 
 static int video_mux_probe(struct platform_device *pdev)
@@ -434,7 +448,13 @@ static int video_mux_probe(struct platform_device *pdev)
 
 	vmux->subdev.entity.ops = &video_mux_ops;
 
-	return video_mux_async_register(vmux, num_pads - 1);
+	ret = video_mux_async_register(vmux, num_pads - 1);
+	if (ret) {
+		v4l2_async_notifier_unregister(&vmux->notifier);
+		v4l2_async_notifier_cleanup(&vmux->notifier);
+	}
+
+	return ret;
 }
 
 static int video_mux_remove(struct platform_device *pdev)
@@ -442,6 +462,8 @@ static int video_mux_remove(struct platform_device *pdev)
 	struct video_mux *vmux = platform_get_drvdata(pdev);
 	struct v4l2_subdev *sd = &vmux->subdev;
 
+	v4l2_async_notifier_unregister(&vmux->notifier);
+	v4l2_async_notifier_cleanup(&vmux->notifier);
 	v4l2_async_unregister_subdev(sd);
 	media_entity_cleanup(&sd->entity);
 
-- 
2.17.1

[PATCH v4 03/17] media: imx: Parse information from firmware without using callbacks

[Steve Longerbeam]

Instead of using the convenience functions
v4l2_async_notifier_parse_fwnode_endpoints*() or
v4l2_async_register_fwnode_subdev(), parse the input endpoints
and set up the async sub-devices without using callbacks. The drivers
know which ports it must parse and how to handle unconnected remotes,
so it makes the code simpler to transfer control of endpoint parsing
to the driver.

Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
 drivers/staging/media/imx/imx-media-csi.c  | 78 +++++++++----------
 drivers/staging/media/imx/imx6-mipi-csi2.c | 70 ++++++++++++-----
 drivers/staging/media/imx/imx7-media-csi.c | 53 ++++++++++---
 drivers/staging/media/imx/imx7-mipi-csis.c | 91 +++++++++++++++-------
 4 files changed, 192 insertions(+), 100 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index b60ed4f22f6d..f409fca88dcf 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -58,6 +58,8 @@ struct csi_priv {
 	struct ipu_soc *ipu;
 	struct v4l2_subdev sd;
 	struct media_pad pad[CSI_NUM_PADS];
+	struct v4l2_async_notifier notifier;
+
 	/* the video device at IDMAC output pad */
 	struct imx_media_video_dev *vdev;
 	struct imx_media_fim *fim;
@@ -1864,59 +1866,49 @@ static const struct v4l2_subdev_internal_ops csi_internal_ops = {
 	.unregistered = csi_unregistered,
 };
 
-static int imx_csi_parse_endpoint(struct device *dev,
-				  struct v4l2_fwnode_endpoint *vep,
-				  struct v4l2_async_subdev *asd)
-{
-	return fwnode_device_is_available(asd->match.fwnode) ? 0 : -ENOTCONN;
-}
-
 static int imx_csi_async_register(struct csi_priv *priv)
 {
-	struct v4l2_async_notifier *notifier;
-	struct fwnode_handle *fwnode;
+	struct v4l2_async_subdev *asd = NULL;
+	struct fwnode_handle *ep;
 	unsigned int port;
 	int ret;
 
-	notifier = kzalloc(sizeof(*notifier), GFP_KERNEL);
-	if (!notifier)
-		return -ENOMEM;
-
-	v4l2_async_notifier_init(notifier);
-
-	fwnode = dev_fwnode(priv->dev);
+	v4l2_async_notifier_init(&priv->notifier);
 
 	/* get this CSI's port id */
-	ret = fwnode_property_read_u32(fwnode, "reg", &port);
-	if (ret < 0)
-		goto out_free;
-
-	ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
-		priv->dev->parent, notifier, sizeof(struct v4l2_async_subdev),
-		port, imx_csi_parse_endpoint);
+	ret = fwnode_property_read_u32(dev_fwnode(priv->dev), "reg", &port);
 	if (ret < 0)
-		goto out_cleanup;
+		return ret;
 
-	ret = v4l2_async_subdev_notifier_register(&priv->sd, notifier);
-	if (ret < 0)
-		goto out_cleanup;
+	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(priv->dev->parent),
+					     port, 0,
+					     FWNODE_GRAPH_ENDPOINT_NEXT);
+	if (ep) {
+		asd = kzalloc(sizeof(*asd), GFP_KERNEL);
+		if (!asd) {
+			fwnode_handle_put(ep);
+			return -ENOMEM;
+		}
 
-	ret = v4l2_async_register_subdev(&priv->sd);
-	if (ret < 0)
-		goto out_unregister;
+		ret = v4l2_async_notifier_add_fwnode_remote_subdev(
+			&priv->notifier, ep, asd);
 
-	priv->sd.subdev_notifier = notifier;
+		fwnode_handle_put(ep);
 
-	return 0;
+		if (ret) {
+			kfree(asd);
+			/* OK if asd already exists */
+			if (ret != -EEXIST)
+				return ret;
+		}
+	}
 
-out_unregister:
-	v4l2_async_notifier_unregister(notifier);
-out_cleanup:
-	v4l2_async_notifier_cleanup(notifier);
-out_free:
-	kfree(notifier);
+	ret = v4l2_async_subdev_notifier_register(&priv->sd,
+						  &priv->notifier);
+	if (ret)
+		return ret;
 
-	return ret;
+	return v4l2_async_register_subdev(&priv->sd);
 }
 
 static int imx_csi_probe(struct platform_device *pdev)
@@ -1996,9 +1988,13 @@ static int imx_csi_probe(struct platform_device *pdev)
 
 	ret = imx_csi_async_register(priv);
 	if (ret)
-		goto free;
+		goto cleanup;
 
 	return 0;
+
+cleanup:
+	v4l2_async_notifier_unregister(&priv->notifier);
+	v4l2_async_notifier_cleanup(&priv->notifier);
 free:
 	v4l2_ctrl_handler_free(&priv->ctrl_hdlr);
 	mutex_destroy(&priv->lock);
@@ -2012,6 +2008,8 @@ static int imx_csi_remove(struct platform_device *pdev)
 
 	v4l2_ctrl_handler_free(&priv->ctrl_hdlr);
 	mutex_destroy(&priv->lock);
+	v4l2_async_notifier_unregister(&priv->notifier);
+	v4l2_async_notifier_cleanup(&priv->notifier);
 	v4l2_async_unregister_subdev(sd);
 	media_entity_cleanup(&sd->entity);
 
diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
index cd3dd6e33ef0..fdd763587e6c 100644
--- a/drivers/staging/media/imx/imx6-mipi-csi2.c
+++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
@@ -35,6 +35,7 @@
 struct csi2_dev {
 	struct device          *dev;
 	struct v4l2_subdev      sd;
+	struct v4l2_async_notifier notifier;
 	struct media_pad       pad[CSI2_NUM_PADS];
 	struct clk             *dphy_clk;
 	struct clk             *pllref_clk;
@@ -530,34 +531,59 @@ static const struct v4l2_subdev_internal_ops csi2_internal_ops = {
 	.registered = csi2_registered,
 };
 
-static int csi2_parse_endpoint(struct device *dev,
-			       struct v4l2_fwnode_endpoint *vep,
-			       struct v4l2_async_subdev *asd)
+static int csi2_async_register(struct csi2_dev *csi2)
 {
-	struct v4l2_subdev *sd = dev_get_drvdata(dev);
-	struct csi2_dev *csi2 = sd_to_dev(sd);
+	struct v4l2_fwnode_endpoint vep = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY,
+	};
+	struct v4l2_async_subdev *asd = NULL;
+	struct fwnode_handle *ep;
+	int ret;
 
-	if (!fwnode_device_is_available(asd->match.fwnode)) {
-		v4l2_err(&csi2->sd, "remote is not available\n");
-		return -EINVAL;
-	}
+	v4l2_async_notifier_init(&csi2->notifier);
 
-	if (vep->bus_type != V4L2_MBUS_CSI2_DPHY) {
-		v4l2_err(&csi2->sd, "invalid bus type, must be MIPI CSI2\n");
-		return -EINVAL;
-	}
+	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(csi2->dev), 0, 0,
+					     FWNODE_GRAPH_ENDPOINT_NEXT);
+	if (!ep)
+		return -ENOTCONN;
+
+	ret = v4l2_fwnode_endpoint_parse(ep, &vep);
+	if (ret)
+		goto err_parse;
 
-	csi2->bus = vep->bus.mipi_csi2;
+	csi2->bus = vep.bus.mipi_csi2;
 
 	dev_dbg(csi2->dev, "data lanes: %d\n", csi2->bus.num_data_lanes);
 	dev_dbg(csi2->dev, "flags: 0x%08x\n", csi2->bus.flags);
 
-	return 0;
+	asd = kzalloc(sizeof(*asd), GFP_KERNEL);
+	if (!asd) {
+		ret = -ENOMEM;
+		goto err_parse;
+	}
+
+	ret = v4l2_async_notifier_add_fwnode_remote_subdev(
+		&csi2->notifier, ep, asd);
+	if (ret)
+		goto err_parse;
+
+	fwnode_handle_put(ep);
+
+	ret = v4l2_async_subdev_notifier_register(&csi2->sd,
+						  &csi2->notifier);
+	if (ret)
+		return ret;
+
+	return v4l2_async_register_subdev(&csi2->sd);
+
+err_parse:
+	fwnode_handle_put(ep);
+	kfree(asd);
+	return ret;
 }
 
 static int csi2_probe(struct platform_device *pdev)
 {
-	unsigned int sink_port = 0;
 	struct csi2_dev *csi2;
 	struct resource *res;
 	int i, ret;
@@ -636,15 +662,15 @@ static int csi2_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, &csi2->sd);
 
-	ret = v4l2_async_register_fwnode_subdev(
-		&csi2->sd, sizeof(struct v4l2_async_subdev),
-		&sink_port, 1, csi2_parse_endpoint);
+	ret = csi2_async_register(csi2);
 	if (ret)
-		goto dphy_off;
+		goto clean_notifier;
 
 	return 0;
 
-dphy_off:
+clean_notifier:
+	v4l2_async_notifier_unregister(&csi2->notifier);
+	v4l2_async_notifier_cleanup(&csi2->notifier);
 	clk_disable_unprepare(csi2->dphy_clk);
 pllref_off:
 	clk_disable_unprepare(csi2->pllref_clk);
@@ -658,6 +684,8 @@ static int csi2_remove(struct platform_device *pdev)
 	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
 	struct csi2_dev *csi2 = sd_to_dev(sd);
 
+	v4l2_async_notifier_unregister(&csi2->notifier);
+	v4l2_async_notifier_cleanup(&csi2->notifier);
 	v4l2_async_unregister_subdev(sd);
 	clk_disable_unprepare(csi2->dphy_clk);
 	clk_disable_unprepare(csi2->pllref_clk);
diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index db30e2c70f2f..776eb42ae5c8 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -155,6 +155,7 @@
 struct imx7_csi {
 	struct device *dev;
 	struct v4l2_subdev sd;
+	struct v4l2_async_notifier notifier;
 	struct imx_media_video_dev *vdev;
 	struct imx_media_dev *imxmd;
 	struct media_pad pad[IMX7_CSI_PADS_NUM];
@@ -1179,11 +1180,41 @@ static const struct v4l2_subdev_internal_ops imx7_csi_internal_ops = {
 	.unregistered	= imx7_csi_unregistered,
 };
 
-static int imx7_csi_parse_endpoint(struct device *dev,
-				   struct v4l2_fwnode_endpoint *vep,
-				   struct v4l2_async_subdev *asd)
+static int imx7_csi_async_register(struct imx7_csi *csi)
 {
-	return fwnode_device_is_available(asd->match.fwnode) ? 0 : -EINVAL;
+	struct v4l2_async_subdev *asd = NULL;
+	struct fwnode_handle *ep;
+	int ret;
+
+	v4l2_async_notifier_init(&csi->notifier);
+
+	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(csi->dev), 0, 0,
+					     FWNODE_GRAPH_ENDPOINT_NEXT);
+	if (ep) {
+		asd = kzalloc(sizeof(*asd), GFP_KERNEL);
+		if (!asd) {
+			fwnode_handle_put(ep);
+			return -ENOMEM;
+		}
+
+		ret = v4l2_async_notifier_add_fwnode_remote_subdev(
+			&csi->notifier, ep, asd);
+
+		fwnode_handle_put(ep);
+
+		if (ret) {
+			kfree(asd);
+			/* OK if asd already exists */
+			if (ret != -EEXIST)
+				return ret;
+		}
+	}
+
+	ret = v4l2_async_subdev_notifier_register(&csi->sd, &csi->notifier);
+	if (ret)
+		return ret;
+
+	return v4l2_async_register_subdev(&csi->sd);
 }
 
 static int imx7_csi_probe(struct platform_device *pdev)
@@ -1266,19 +1297,21 @@ static int imx7_csi_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto free;
 
-	ret = v4l2_async_register_fwnode_subdev(&csi->sd,
-						sizeof(struct v4l2_async_subdev),
-						NULL, 0,
-						imx7_csi_parse_endpoint);
+	ret = imx7_csi_async_register(csi);
 	if (ret)
-		goto free;
+		goto subdev_notifier_cleanup;
 
 	return 0;
 
+subdev_notifier_cleanup:
+	v4l2_async_notifier_unregister(&csi->notifier);
+	v4l2_async_notifier_cleanup(&csi->notifier);
+
 free:
 	v4l2_ctrl_handler_free(&csi->ctrl_hdlr);
 
 cleanup:
+	v4l2_async_notifier_unregister(&imxmd->notifier);
 	v4l2_async_notifier_cleanup(&imxmd->notifier);
 	v4l2_device_unregister(&imxmd->v4l2_dev);
 	media_device_unregister(&imxmd->md);
@@ -1303,6 +1336,8 @@ static int imx7_csi_remove(struct platform_device *pdev)
 	v4l2_device_unregister(&imxmd->v4l2_dev);
 	media_device_cleanup(&imxmd->md);
 
+	v4l2_async_notifier_unregister(&csi->notifier);
+	v4l2_async_notifier_cleanup(&csi->notifier);
 	v4l2_async_unregister_subdev(sd);
 	v4l2_ctrl_handler_free(&csi->ctrl_hdlr);
 
diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 383abecb3bec..7e872d8f51e0 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -223,6 +223,7 @@ struct csi_state {
 	struct device *dev;
 	struct media_pad pads[CSIS_PADS_NUM];
 	struct v4l2_subdev mipi_sd;
+	struct v4l2_async_notifier notifier;
 	struct v4l2_subdev *src_sd;
 
 	u8 index;
@@ -827,33 +828,11 @@ static int mipi_csis_parse_dt(struct platform_device *pdev,
 
 static int mipi_csis_pm_resume(struct device *dev, bool runtime);
 
-static int mipi_csis_parse_endpoint(struct device *dev,
-				    struct v4l2_fwnode_endpoint *ep,
-				    struct v4l2_async_subdev *asd)
-{
-	struct v4l2_subdev *mipi_sd = dev_get_drvdata(dev);
-	struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
-
-	if (ep->bus_type != V4L2_MBUS_CSI2_DPHY) {
-		dev_err(dev, "invalid bus type, must be MIPI CSI2\n");
-		return -EINVAL;
-	}
-
-	state->bus = ep->bus.mipi_csi2;
-
-	dev_dbg(state->dev, "data lanes: %d\n", state->bus.num_data_lanes);
-	dev_dbg(state->dev, "flags: 0x%08x\n", state->bus.flags);
-
-	return 0;
-}
-
 static int mipi_csis_subdev_init(struct v4l2_subdev *mipi_sd,
 				 struct platform_device *pdev,
 				 const struct v4l2_subdev_ops *ops)
 {
 	struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
-	unsigned int sink_port = 0;
-	int ret;
 
 	v4l2_subdev_init(mipi_sd, ops);
 	mipi_sd->owner = THIS_MODULE;
@@ -878,17 +857,58 @@ static int mipi_csis_subdev_init(struct v4l2_subdev *mipi_sd,
 
 	state->pads[CSIS_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
 	state->pads[CSIS_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
-	ret = media_entity_pads_init(&mipi_sd->entity, CSIS_PADS_NUM,
-				     state->pads);
+	return media_entity_pads_init(&mipi_sd->entity, CSIS_PADS_NUM,
+				      state->pads);
+}
+
+static int mipi_csis_async_register(struct csi_state *state)
+{
+	struct v4l2_fwnode_endpoint vep = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY,
+	};
+	struct v4l2_async_subdev *asd = NULL;
+	struct fwnode_handle *ep;
+	int ret;
+
+	v4l2_async_notifier_init(&state->notifier);
+
+	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(state->dev), 0, 0,
+					     FWNODE_GRAPH_ENDPOINT_NEXT);
+	if (!ep)
+		return -ENOTCONN;
+
+	ret = v4l2_fwnode_endpoint_parse(ep, &vep);
+	if (ret)
+		goto err_parse;
+
+	state->bus = vep.bus.mipi_csi2;
+
+	dev_dbg(state->dev, "data lanes: %d\n", state->bus.num_data_lanes);
+	dev_dbg(state->dev, "flags: 0x%08x\n", state->bus.flags);
+
+	asd = kzalloc(sizeof(*asd), GFP_KERNEL);
+	if (!asd) {
+		ret = -ENOMEM;
+		goto err_parse;
+	}
+
+	ret = v4l2_async_notifier_add_fwnode_remote_subdev(
+		&state->notifier, ep, asd);
+	if (ret)
+		goto err_parse;
+
+	fwnode_handle_put(ep);
+
+	ret = v4l2_async_subdev_notifier_register(&state->mipi_sd,
+						  &state->notifier);
 	if (ret)
 		return ret;
 
-	ret = v4l2_async_register_fwnode_subdev(mipi_sd,
-						sizeof(struct v4l2_async_subdev),
-						&sink_port, 1,
-						mipi_csis_parse_endpoint);
-	if (ret < 0)
-		dev_err(&pdev->dev, "async fwnode register failed: %d\n", ret);
+	return v4l2_async_register_subdev(&state->mipi_sd);
+
+err_parse:
+	fwnode_handle_put(ep);
+	kfree(asd);
 
 	return ret;
 }
@@ -995,6 +1015,12 @@ static int mipi_csis_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto disable_clock;
 
+	ret = mipi_csis_async_register(state);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "async register failed: %d\n", ret);
+		goto cleanup;
+	}
+
 	memcpy(state->events, mipi_csis_events, sizeof(state->events));
 
 	mipi_csis_debugfs_init(state);
@@ -1013,7 +1039,10 @@ static int mipi_csis_probe(struct platform_device *pdev)
 
 unregister_all:
 	mipi_csis_debugfs_exit(state);
+cleanup:
 	media_entity_cleanup(&state->mipi_sd.entity);
+	v4l2_async_notifier_unregister(&state->notifier);
+	v4l2_async_notifier_cleanup(&state->notifier);
 	v4l2_async_unregister_subdev(&state->mipi_sd);
 disable_clock:
 	mipi_csis_clk_disable(state);
@@ -1101,6 +1130,8 @@ static int mipi_csis_remove(struct platform_device *pdev)
 	struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
 
 	mipi_csis_debugfs_exit(state);
+	v4l2_async_notifier_unregister(&state->notifier);
+	v4l2_async_notifier_cleanup(&state->notifier);
 	v4l2_async_unregister_subdev(&state->mipi_sd);
 
 	pm_runtime_disable(&pdev->dev);
-- 
2.17.1

[PATCH v4 04/17] Revert "media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers"

[Steve Longerbeam]

The users of v4l2_async_register_fwnode_subdev() have switched to
parsing their endpoints and setting up async sub-device lists in their
notifiers locally, without using the endpoint parsing callbacks. There
are no more users of v4l2_async_register_fwnode_subdev() so this
convenience function can be removed.

This reverts commit 1634f0eded87d1f150e823fa56cd782ea0775eb2.

Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
 drivers/media/v4l2-core/v4l2-fwnode.c | 62 ---------------------------
 include/media/v4l2-fwnode.h           | 38 ----------------
 2 files changed, 100 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 6ece4320e1d2..1dc18940ac0e 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -1163,68 +1163,6 @@ int v4l2_async_register_subdev_sensor_common(struct v4l2_subdev *sd)
 }
 EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor_common);
 
-int v4l2_async_register_fwnode_subdev(struct v4l2_subdev *sd,
-				      size_t asd_struct_size,
-				      unsigned int *ports,
-				      unsigned int num_ports,
-				      parse_endpoint_func parse_endpoint)
-{
-	struct v4l2_async_notifier *notifier;
-	struct device *dev = sd->dev;
-	struct fwnode_handle *fwnode;
-	int ret;
-
-	if (WARN_ON(!dev))
-		return -ENODEV;
-
-	fwnode = dev_fwnode(dev);
-	if (!fwnode_device_is_available(fwnode))
-		return -ENODEV;
-
-	notifier = kzalloc(sizeof(*notifier), GFP_KERNEL);
-	if (!notifier)
-		return -ENOMEM;
-
-	v4l2_async_notifier_init(notifier);
-
-	if (!ports) {
-		ret = v4l2_async_notifier_parse_fwnode_endpoints(dev, notifier,
-								 asd_struct_size,
-								 parse_endpoint);
-		if (ret < 0)
-			goto out_cleanup;
-	} else {
-		unsigned int i;
-
-		for (i = 0; i < num_ports; i++) {
-			ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(dev, notifier, asd_struct_size, ports[i], parse_endpoint);
-			if (ret < 0)
-				goto out_cleanup;
-		}
-	}
-
-	ret = v4l2_async_subdev_notifier_register(sd, notifier);
-	if (ret < 0)
-		goto out_cleanup;
-
-	ret = v4l2_async_register_subdev(sd);
-	if (ret < 0)
-		goto out_unregister;
-
-	sd->subdev_notifier = notifier;
-
-	return 0;
-
-out_unregister:
-	v4l2_async_notifier_unregister(notifier);
-out_cleanup:
-	v4l2_async_notifier_cleanup(notifier);
-	kfree(notifier);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(v4l2_async_register_fwnode_subdev);
-
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
 MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index f6a7bcd13197..caa854d2a867 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -20,7 +20,6 @@
 #include <linux/types.h>
 
 #include <media/v4l2-mediabus.h>
-#include <media/v4l2-subdev.h>
 
 struct fwnode_handle;
 struct v4l2_async_notifier;
@@ -369,41 +368,4 @@ v4l2_async_notifier_parse_fwnode_endpoints_by_port(struct device *dev,
 int v4l2_async_notifier_parse_fwnode_sensor_common(struct device *dev,
 						   struct v4l2_async_notifier *notifier);
 
-/**
- * v4l2_async_register_fwnode_subdev - registers a sub-device to the
- *					asynchronous sub-device framework
- *					and parses fwnode endpoints
- *
- * @sd: pointer to struct &v4l2_subdev
- * @asd_struct_size: size of the driver's async sub-device struct, including
- *		     sizeof(struct v4l2_async_subdev). The &struct
- *		     v4l2_async_subdev shall be the first member of
- *		     the driver's async sub-device struct, i.e. both
- *		     begin at the same memory address.
- * @ports: array of port id's to parse for fwnode endpoints. If NULL, will
- *	   parse all ports owned by the sub-device.
- * @num_ports: number of ports in @ports array. Ignored if @ports is NULL.
- * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
- *		    endpoint. Optional.
- *
- * This function is just like v4l2_async_register_subdev() with the
- * exception that calling it will also allocate a notifier for the
- * sub-device, parse the sub-device's firmware node endpoints using
- * v4l2_async_notifier_parse_fwnode_endpoints() or
- * v4l2_async_notifier_parse_fwnode_endpoints_by_port(), and
- * registers the sub-device notifier. The sub-device is similarly
- * unregistered by calling v4l2_async_unregister_subdev().
- *
- * While registered, the subdev module is marked as in-use.
- *
- * An error is returned if the module is no longer loaded on any attempts
- * to register it.
- */
-int
-v4l2_async_register_fwnode_subdev(struct v4l2_subdev *sd,
-				  size_t asd_struct_size,
-				  unsigned int *ports,
-				  unsigned int num_ports,
-				  parse_endpoint_func parse_endpoint);
-
 #endif /* _V4L2_FWNODE_H */
-- 
2.17.1

[PATCH v4 05/17] media: imx: csi: Implement get_fwnode_pad op

[Steve Longerbeam]

The CSI does not have a 1:1 relationship between fwnode port numbers and
pad indexes. In fact the CSI fwnode device is itself a port which is the
sink, containing only a single fwnode endpoint. Implement media_entity
operation get_fwnode_pad to first verify the given endpoint is the CSI's
sink endpoint, and if so return the CSI sink pad index.

Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
 drivers/staging/media/imx/imx-media-csi.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index f409fca88dcf..35f2512ed2a9 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1827,9 +1827,32 @@ static void csi_unregistered(struct v4l2_subdev *sd)
 		ipu_csi_put(priv->csi);
 }
 
+/*
+ * The CSI has only one fwnode endpoint, at the sink pad. Verify the
+ * endpoint belongs to us, and return CSI_SINK_PAD.
+ */
+static int csi_get_fwnode_pad(struct media_entity *entity,
+			      struct fwnode_endpoint *endpoint)
+{
+	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
+	struct csi_priv *priv = v4l2_get_subdevdata(sd);
+	struct fwnode_handle *csi_port = dev_fwnode(priv->dev);
+	struct fwnode_handle *csi_ep;
+	int ret;
+
+	csi_ep = fwnode_get_next_child_node(csi_port, NULL);
+
+	ret = endpoint->local_fwnode == csi_ep ? CSI_SINK_PAD : -ENXIO;
+
+	fwnode_handle_put(csi_ep);
+
+	return ret;
+}
+
 static const struct media_entity_operations csi_entity_ops = {
 	.link_setup = csi_link_setup,
 	.link_validate = v4l2_subdev_link_validate,
+	.get_fwnode_pad = csi_get_fwnode_pad,
 };
 
 static const struct v4l2_subdev_core_ops csi_core_ops = {
-- 
2.17.1

Re: [PATCH v4 05/17] media: imx: csi: Implement get_fwnode_pad op

[Laurent Pinchart]

Hi Steve,

Thank you for the patch.

On Tue, Mar 03, 2020 at 03:42:44PM -0800, Steve Longerbeam wrote:
> The CSI does not have a 1:1 relationship between fwnode port numbers and
> pad indexes. In fact the CSI fwnode device is itself a port which is the
> sink, containing only a single fwnode endpoint. Implement media_entity
> operation get_fwnode_pad to first verify the given endpoint is the CSI's
> sink endpoint, and if so return the CSI sink pad index.
> 
> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/staging/media/imx/imx-media-csi.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index f409fca88dcf..35f2512ed2a9 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -1827,9 +1827,32 @@ static void csi_unregistered(struct v4l2_subdev *sd)
>  		ipu_csi_put(priv->csi);
>  }
>  
> +/*
> + * The CSI has only one fwnode endpoint, at the sink pad. Verify the
> + * endpoint belongs to us, and return CSI_SINK_PAD.
> + */
> +static int csi_get_fwnode_pad(struct media_entity *entity,
> +			      struct fwnode_endpoint *endpoint)
> +{
> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> +	struct csi_priv *priv = v4l2_get_subdevdata(sd);
> +	struct fwnode_handle *csi_port = dev_fwnode(priv->dev);
> +	struct fwnode_handle *csi_ep;
> +	int ret;
> +
> +	csi_ep = fwnode_get_next_child_node(csi_port, NULL);
> +
> +	ret = endpoint->local_fwnode == csi_ep ? CSI_SINK_PAD : -ENXIO;
> +
> +	fwnode_handle_put(csi_ep);
> +
> +	return ret;
> +}
> +
>  static const struct media_entity_operations csi_entity_ops = {
>  	.link_setup = csi_link_setup,
>  	.link_validate = v4l2_subdev_link_validate,
> +	.get_fwnode_pad = csi_get_fwnode_pad,
>  };
>  
>  static const struct v4l2_subdev_core_ops csi_core_ops = {

-- 
Regards,

Laurent Pinchart

[PATCH v4 06/17] media: imx: mipi csi-2: Implement get_fwnode_pad op

[Steve Longerbeam]

Implement get_fwnode_pad operation. If the endpoint is owned by the MIPI
CSI-2 receiver, return the endpoint's port number. The MIPI CSI-2 receiver
maps port numbers and pad indexes 1:1.

Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
 drivers/staging/media/imx/imx6-mipi-csi2.c | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
index fdd763587e6c..8500207e5ea9 100644
--- a/drivers/staging/media/imx/imx6-mipi-csi2.c
+++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
@@ -507,9 +507,37 @@ static int csi2_registered(struct v4l2_subdev *sd)
 				      640, 480, 0, V4L2_FIELD_NONE, NULL);
 }
 
+static int csi2_get_fwnode_pad(struct media_entity *entity,
+			       struct fwnode_endpoint *endpoint)
+{
+	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
+	struct csi2_dev *csi2 = sd_to_dev(sd);
+	struct fwnode_handle *csi2_ep;
+
+	/*
+	 * If the endpoint is one of ours, return the endpoint's port
+	 * number. This device maps port numbers and pad indexes 1:1.
+	 */
+	fwnode_graph_for_each_endpoint(dev_fwnode(csi2->dev), csi2_ep) {
+		if (endpoint->local_fwnode == csi2_ep) {
+			struct fwnode_endpoint fwep;
+			int ret;
+
+			ret = fwnode_graph_parse_endpoint(csi2_ep, &fwep);
+
+			fwnode_handle_put(csi2_ep);
+
+			return ret ? ret : fwep.port;
+		}
+	}
+
+	return -ENXIO;
+}
+
 static const struct media_entity_operations csi2_entity_ops = {
 	.link_setup = csi2_link_setup,
 	.link_validate = v4l2_subdev_link_validate,
+	.get_fwnode_pad = csi2_get_fwnode_pad,
 };
 
 static const struct v4l2_subdev_video_ops csi2_video_ops = {
-- 
2.17.1

Re: [PATCH v4 06/17] media: imx: mipi csi-2: Implement get_fwnode_pad op

[Laurent Pinchart]

Hi Steve,

Thank you for the patch.

On Tue, Mar 03, 2020 at 03:42:45PM -0800, Steve Longerbeam wrote:
> Implement get_fwnode_pad operation. If the endpoint is owned by the MIPI
> CSI-2 receiver, return the endpoint's port number. The MIPI CSI-2 receiver
> maps port numbers and pad indexes 1:1.
> 
> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> ---
>  drivers/staging/media/imx/imx6-mipi-csi2.c | 28 ++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
> index fdd763587e6c..8500207e5ea9 100644
> --- a/drivers/staging/media/imx/imx6-mipi-csi2.c
> +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
> @@ -507,9 +507,37 @@ static int csi2_registered(struct v4l2_subdev *sd)
>  				      640, 480, 0, V4L2_FIELD_NONE, NULL);
>  }
>  
> +static int csi2_get_fwnode_pad(struct media_entity *entity,
> +			       struct fwnode_endpoint *endpoint)
> +{
> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> +	struct csi2_dev *csi2 = sd_to_dev(sd);
> +	struct fwnode_handle *csi2_ep;
> +
> +	/*
> +	 * If the endpoint is one of ours, return the endpoint's port
> +	 * number. This device maps port numbers and pad indexes 1:1.
> +	 */
> +	fwnode_graph_for_each_endpoint(dev_fwnode(csi2->dev), csi2_ep) {
> +		if (endpoint->local_fwnode == csi2_ep) {
> +			struct fwnode_endpoint fwep;
> +			int ret;
> +
> +			ret = fwnode_graph_parse_endpoint(csi2_ep, &fwep);
> +
> +			fwnode_handle_put(csi2_ep);
> +
> +			return ret ? ret : fwep.port;
> +		}
> +	}
> +
> +	return -ENXIO;
> +}

As the 1:1 mapping is the common case, would it make sense to modify
media_entity_get_fwnode_pad() accordingly when .get_fwnode_pad is not
set ? The current behaviour is to return the first pad that matches the
requested direction, which could be preserved as a second-level fallback
if the 1:1 mapping doesn't give the right direction (but I'm not sure
there's a use case for that, the 1:1 mapping seems to be all we need if
there's no specific .get_fwnode_pad implementation).

> +
>  static const struct media_entity_operations csi2_entity_ops = {
>  	.link_setup = csi2_link_setup,
>  	.link_validate = v4l2_subdev_link_validate,
> +	.get_fwnode_pad = csi2_get_fwnode_pad,
>  };
>  
>  static const struct v4l2_subdev_video_ops csi2_video_ops = {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 06/17] media: imx: mipi csi-2: Implement get_fwnode_pad op

[Sakari Ailus]

Hi Laurent,

On Wed, Apr 15, 2020 at 02:07:29AM +0300, Laurent Pinchart wrote:
> Hi Steve,
> 
> Thank you for the patch.
> 
> On Tue, Mar 03, 2020 at 03:42:45PM -0800, Steve Longerbeam wrote:
> > Implement get_fwnode_pad operation. If the endpoint is owned by the MIPI
> > CSI-2 receiver, return the endpoint's port number. The MIPI CSI-2 receiver
> > maps port numbers and pad indexes 1:1.
> > 
> > Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> > ---
> >  drivers/staging/media/imx/imx6-mipi-csi2.c | 28 ++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
> > index fdd763587e6c..8500207e5ea9 100644
> > --- a/drivers/staging/media/imx/imx6-mipi-csi2.c
> > +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
> > @@ -507,9 +507,37 @@ static int csi2_registered(struct v4l2_subdev *sd)
> >  				      640, 480, 0, V4L2_FIELD_NONE, NULL);
> >  }
> >  
> > +static int csi2_get_fwnode_pad(struct media_entity *entity,
> > +			       struct fwnode_endpoint *endpoint)
> > +{
> > +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> > +	struct csi2_dev *csi2 = sd_to_dev(sd);
> > +	struct fwnode_handle *csi2_ep;
> > +
> > +	/*
> > +	 * If the endpoint is one of ours, return the endpoint's port
> > +	 * number. This device maps port numbers and pad indexes 1:1.
> > +	 */
> > +	fwnode_graph_for_each_endpoint(dev_fwnode(csi2->dev), csi2_ep) {
> > +		if (endpoint->local_fwnode == csi2_ep) {
> > +			struct fwnode_endpoint fwep;
> > +			int ret;
> > +
> > +			ret = fwnode_graph_parse_endpoint(csi2_ep, &fwep);
> > +
> > +			fwnode_handle_put(csi2_ep);
> > +
> > +			return ret ? ret : fwep.port;
> > +		}
> > +	}
> > +
> > +	return -ENXIO;
> > +}
> 
> As the 1:1 mapping is the common case, would it make sense to modify
> media_entity_get_fwnode_pad() accordingly when .get_fwnode_pad is not
> set ? The current behaviour is to return the first pad that matches the

I also think this would make sense.

> requested direction, which could be preserved as a second-level fallback
> if the 1:1 mapping doesn't give the right direction (but I'm not sure
> there's a use case for that, the 1:1 mapping seems to be all we need if
> there's no specific .get_fwnode_pad implementation).

I believe at least the smiapp driver breaks if you do that, so the current
behaviour should be retained (secondary to the 1:1 mapping).

-- 
Regards,

Sakari Ailus

Re: [PATCH v4 06/17] media: imx: mipi csi-2: Implement get_fwnode_pad op

[Laurent Pinchart]

Hi Sakari,

On Wed, Apr 15, 2020 at 02:20:36AM +0300, Sakari Ailus wrote:
> On Wed, Apr 15, 2020 at 02:07:29AM +0300, Laurent Pinchart wrote:
> > On Tue, Mar 03, 2020 at 03:42:45PM -0800, Steve Longerbeam wrote:
> > > Implement get_fwnode_pad operation. If the endpoint is owned by the MIPI
> > > CSI-2 receiver, return the endpoint's port number. The MIPI CSI-2 receiver
> > > maps port numbers and pad indexes 1:1.
> > > 
> > > Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> > > ---
> > >  drivers/staging/media/imx/imx6-mipi-csi2.c | 28 ++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > > 
> > > diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
> > > index fdd763587e6c..8500207e5ea9 100644
> > > --- a/drivers/staging/media/imx/imx6-mipi-csi2.c
> > > +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
> > > @@ -507,9 +507,37 @@ static int csi2_registered(struct v4l2_subdev *sd)
> > >  				      640, 480, 0, V4L2_FIELD_NONE, NULL);
> > >  }
> > >  
> > > +static int csi2_get_fwnode_pad(struct media_entity *entity,
> > > +			       struct fwnode_endpoint *endpoint)
> > > +{
> > > +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> > > +	struct csi2_dev *csi2 = sd_to_dev(sd);
> > > +	struct fwnode_handle *csi2_ep;
> > > +
> > > +	/*
> > > +	 * If the endpoint is one of ours, return the endpoint's port
> > > +	 * number. This device maps port numbers and pad indexes 1:1.
> > > +	 */
> > > +	fwnode_graph_for_each_endpoint(dev_fwnode(csi2->dev), csi2_ep) {
> > > +		if (endpoint->local_fwnode == csi2_ep) {
> > > +			struct fwnode_endpoint fwep;
> > > +			int ret;
> > > +
> > > +			ret = fwnode_graph_parse_endpoint(csi2_ep, &fwep);
> > > +
> > > +			fwnode_handle_put(csi2_ep);
> > > +
> > > +			return ret ? ret : fwep.port;
> > > +		}
> > > +	}
> > > +
> > > +	return -ENXIO;
> > > +}
> > 
> > As the 1:1 mapping is the common case, would it make sense to modify
> > media_entity_get_fwnode_pad() accordingly when .get_fwnode_pad is not
> > set ? The current behaviour is to return the first pad that matches the
> 
> I also think this would make sense.
> 
> > requested direction, which could be preserved as a second-level fallback
> > if the 1:1 mapping doesn't give the right direction (but I'm not sure
> > there's a use case for that, the 1:1 mapping seems to be all we need if
> > there's no specific .get_fwnode_pad implementation).
> 
> I believe at least the smiapp driver breaks if you do that, so the current
> behaviour should be retained (secondary to the 1:1 mapping).

Shouldn't the smiapp driver get a .get_fwnode_pad() implementation then
? It could be argued that the smiapp use case is also quite common, and
would deserve handling in the core, as a second fallback. It makes me
wonder if we should really try to have a core fallback in the first
place then, we could instead create helpers for the common cases that
would be used by drivers as their .get_fwnode_pad() implementation,
forcing each driver to think about which version matches its needs best.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 06/17] media: imx: mipi csi-2: Implement get_fwnode_pad op

[Sakari Ailus]

Hi Laurent,

On Wed, Apr 15, 2020 at 02:27:03AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wed, Apr 15, 2020 at 02:20:36AM +0300, Sakari Ailus wrote:
> > On Wed, Apr 15, 2020 at 02:07:29AM +0300, Laurent Pinchart wrote:
> > > On Tue, Mar 03, 2020 at 03:42:45PM -0800, Steve Longerbeam wrote:
> > > > Implement get_fwnode_pad operation. If the endpoint is owned by the MIPI
> > > > CSI-2 receiver, return the endpoint's port number. The MIPI CSI-2 receiver
> > > > maps port numbers and pad indexes 1:1.
> > > > 
> > > > Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> > > > ---
> > > >  drivers/staging/media/imx/imx6-mipi-csi2.c | 28 ++++++++++++++++++++++
> > > >  1 file changed, 28 insertions(+)
> > > > 
> > > > diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
> > > > index fdd763587e6c..8500207e5ea9 100644
> > > > --- a/drivers/staging/media/imx/imx6-mipi-csi2.c
> > > > +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
> > > > @@ -507,9 +507,37 @@ static int csi2_registered(struct v4l2_subdev *sd)
> > > >  				      640, 480, 0, V4L2_FIELD_NONE, NULL);
> > > >  }
> > > >  
> > > > +static int csi2_get_fwnode_pad(struct media_entity *entity,
> > > > +			       struct fwnode_endpoint *endpoint)
> > > > +{
> > > > +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> > > > +	struct csi2_dev *csi2 = sd_to_dev(sd);
> > > > +	struct fwnode_handle *csi2_ep;
> > > > +
> > > > +	/*
> > > > +	 * If the endpoint is one of ours, return the endpoint's port
> > > > +	 * number. This device maps port numbers and pad indexes 1:1.
> > > > +	 */
> > > > +	fwnode_graph_for_each_endpoint(dev_fwnode(csi2->dev), csi2_ep) {
> > > > +		if (endpoint->local_fwnode == csi2_ep) {
> > > > +			struct fwnode_endpoint fwep;
> > > > +			int ret;
> > > > +
> > > > +			ret = fwnode_graph_parse_endpoint(csi2_ep, &fwep);
> > > > +
> > > > +			fwnode_handle_put(csi2_ep);
> > > > +
> > > > +			return ret ? ret : fwep.port;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	return -ENXIO;
> > > > +}
> > > 
> > > As the 1:1 mapping is the common case, would it make sense to modify
> > > media_entity_get_fwnode_pad() accordingly when .get_fwnode_pad is not
> > > set ? The current behaviour is to return the first pad that matches the
> > 
> > I also think this would make sense.
> > 
> > > requested direction, which could be preserved as a second-level fallback
> > > if the 1:1 mapping doesn't give the right direction (but I'm not sure
> > > there's a use case for that, the 1:1 mapping seems to be all we need if
> > > there's no specific .get_fwnode_pad implementation).
> > 
> > I believe at least the smiapp driver breaks if you do that, so the current
> > behaviour should be retained (secondary to the 1:1 mapping).
> 
> Shouldn't the smiapp driver get a .get_fwnode_pad() implementation then
> ? It could be argued that the smiapp use case is also quite common, and
> would deserve handling in the core, as a second fallback. It makes me
> wonder if we should really try to have a core fallback in the first
> place then, we could instead create helpers for the common cases that
> would be used by drivers as their .get_fwnode_pad() implementation,
> forcing each driver to think about which version matches its needs best.

I guess this could be a little cleaner with explicit helpers set by drives.
I'm not against that, but at the same time I don't think this is an area
the cleaning of which would significantly help something.

-- 
Regards,

Sakari Ailus

Re: [PATCH v4 06/17] media: imx: mipi csi-2: Implement get_fwnode_pad op

[Steve Longerbeam]

Hi Sakari, Laurent,

On 4/14/20 4:20 PM, Sakari Ailus wrote:
> Hi Laurent,
>
> On Wed, Apr 15, 2020 at 02:07:29AM +0300, Laurent Pinchart wrote:
>> Hi Steve,
>>
>> Thank you for the patch.
>>
>> On Tue, Mar 03, 2020 at 03:42:45PM -0800, Steve Longerbeam wrote:
>>> Implement get_fwnode_pad operation. If the endpoint is owned by the MIPI
>>> CSI-2 receiver, return the endpoint's port number. The MIPI CSI-2 receiver
>>> maps port numbers and pad indexes 1:1.
>>>
>>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
>>> ---
>>>   drivers/staging/media/imx/imx6-mipi-csi2.c | 28 ++++++++++++++++++++++
>>>   1 file changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
>>> index fdd763587e6c..8500207e5ea9 100644
>>> --- a/drivers/staging/media/imx/imx6-mipi-csi2.c
>>> +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
>>> @@ -507,9 +507,37 @@ static int csi2_registered(struct v4l2_subdev *sd)
>>>   				      640, 480, 0, V4L2_FIELD_NONE, NULL);
>>>   }
>>>   
>>> +static int csi2_get_fwnode_pad(struct media_entity *entity,
>>> +			       struct fwnode_endpoint *endpoint)
>>> +{
>>> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
>>> +	struct csi2_dev *csi2 = sd_to_dev(sd);
>>> +	struct fwnode_handle *csi2_ep;
>>> +
>>> +	/*
>>> +	 * If the endpoint is one of ours, return the endpoint's port
>>> +	 * number. This device maps port numbers and pad indexes 1:1.
>>> +	 */
>>> +	fwnode_graph_for_each_endpoint(dev_fwnode(csi2->dev), csi2_ep) {
>>> +		if (endpoint->local_fwnode == csi2_ep) {
>>> +			struct fwnode_endpoint fwep;
>>> +			int ret;
>>> +
>>> +			ret = fwnode_graph_parse_endpoint(csi2_ep, &fwep);
>>> +
>>> +			fwnode_handle_put(csi2_ep);
>>> +
>>> +			return ret ? ret : fwep.port;
>>> +		}
>>> +	}
>>> +
>>> +	return -ENXIO;
>>> +}
>> As the 1:1 mapping is the common case, would it make sense to modify
>> media_entity_get_fwnode_pad() accordingly when .get_fwnode_pad is not
>> set ? The current behaviour is to return the first pad that matches the
> I also think this would make sense.

What do you think about https://patchwork.linuxtv.org/patch/60312/ ? I'm 
planning to resurrect it for v5.

Steve

>> requested direction, which could be preserved as a second-level fallback
>> if the 1:1 mapping doesn't give the right direction (but I'm not sure
>> there's a use case for that, the 1:1 mapping seems to be all we need if
>> there's no specific .get_fwnode_pad implementation).
> I believe at least the smiapp driver breaks if you do that, so the current
> behaviour should be retained (secondary to the 1:1 mapping).
>

Re: [PATCH v4 06/17] media: imx: mipi csi-2: Implement get_fwnode_pad op

[Laurent Pinchart]

Hi Steve,

On Tue, Apr 14, 2020 at 04:50:55PM -0700, Steve Longerbeam wrote:
> On 4/14/20 4:20 PM, Sakari Ailus wrote:
> > On Wed, Apr 15, 2020 at 02:07:29AM +0300, Laurent Pinchart wrote:
> >> On Tue, Mar 03, 2020 at 03:42:45PM -0800, Steve Longerbeam wrote:
> >>> Implement get_fwnode_pad operation. If the endpoint is owned by the MIPI
> >>> CSI-2 receiver, return the endpoint's port number. The MIPI CSI-2 receiver
> >>> maps port numbers and pad indexes 1:1.
> >>>
> >>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> >>> ---
> >>>   drivers/staging/media/imx/imx6-mipi-csi2.c | 28 ++++++++++++++++++++++
> >>>   1 file changed, 28 insertions(+)
> >>>
> >>> diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
> >>> index fdd763587e6c..8500207e5ea9 100644
> >>> --- a/drivers/staging/media/imx/imx6-mipi-csi2.c
> >>> +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
> >>> @@ -507,9 +507,37 @@ static int csi2_registered(struct v4l2_subdev *sd)
> >>>   				      640, 480, 0, V4L2_FIELD_NONE, NULL);
> >>>   }
> >>>   
> >>> +static int csi2_get_fwnode_pad(struct media_entity *entity,
> >>> +			       struct fwnode_endpoint *endpoint)
> >>> +{
> >>> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> >>> +	struct csi2_dev *csi2 = sd_to_dev(sd);
> >>> +	struct fwnode_handle *csi2_ep;
> >>> +
> >>> +	/*
> >>> +	 * If the endpoint is one of ours, return the endpoint's port
> >>> +	 * number. This device maps port numbers and pad indexes 1:1.
> >>> +	 */
> >>> +	fwnode_graph_for_each_endpoint(dev_fwnode(csi2->dev), csi2_ep) {
> >>> +		if (endpoint->local_fwnode == csi2_ep) {
> >>> +			struct fwnode_endpoint fwep;
> >>> +			int ret;
> >>> +
> >>> +			ret = fwnode_graph_parse_endpoint(csi2_ep, &fwep);
> >>> +
> >>> +			fwnode_handle_put(csi2_ep);
> >>> +
> >>> +			return ret ? ret : fwep.port;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	return -ENXIO;
> >>> +}
> >>
> >> As the 1:1 mapping is the common case, would it make sense to modify
> >> media_entity_get_fwnode_pad() accordingly when .get_fwnode_pad is not
> >> set ? The current behaviour is to return the first pad that matches the
> >
> > I also think this would make sense.
> 
> What do you think about https://patchwork.linuxtv.org/patch/60312/ ? I'm 
> planning to resurrect it for v5.

The approach looks good to me.

> >> requested direction, which could be preserved as a second-level fallback
> >> if the 1:1 mapping doesn't give the right direction (but I'm not sure
> >> there's a use case for that, the 1:1 mapping seems to be all we need if
> >> there's no specific .get_fwnode_pad implementation).
> >
> > I believe at least the smiapp driver breaks if you do that, so the current
> > behaviour should be retained (secondary to the 1:1 mapping).

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 06/17] media: imx: mipi csi-2: Implement get_fwnode_pad op

[Steve Longerbeam]

Hi Laurent,

On 4/14/20 4:07 PM, Laurent Pinchart wrote:
> Hi Steve,
>
> Thank you for the patch.
>
> On Tue, Mar 03, 2020 at 03:42:45PM -0800, Steve Longerbeam wrote:
>> Implement get_fwnode_pad operation. If the endpoint is owned by the MIPI
>> CSI-2 receiver, return the endpoint's port number. The MIPI CSI-2 receiver
>> maps port numbers and pad indexes 1:1.
>>
>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
>> ---
>>   drivers/staging/media/imx/imx6-mipi-csi2.c | 28 ++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
>> index fdd763587e6c..8500207e5ea9 100644
>> --- a/drivers/staging/media/imx/imx6-mipi-csi2.c
>> +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
>> @@ -507,9 +507,37 @@ static int csi2_registered(struct v4l2_subdev *sd)
>>   				      640, 480, 0, V4L2_FIELD_NONE, NULL);
>>   }
>>   
>> +static int csi2_get_fwnode_pad(struct media_entity *entity,
>> +			       struct fwnode_endpoint *endpoint)
>> +{
>> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
>> +	struct csi2_dev *csi2 = sd_to_dev(sd);
>> +	struct fwnode_handle *csi2_ep;
>> +
>> +	/*
>> +	 * If the endpoint is one of ours, return the endpoint's port
>> +	 * number. This device maps port numbers and pad indexes 1:1.
>> +	 */
>> +	fwnode_graph_for_each_endpoint(dev_fwnode(csi2->dev), csi2_ep) {
>> +		if (endpoint->local_fwnode == csi2_ep) {
>> +			struct fwnode_endpoint fwep;
>> +			int ret;
>> +
>> +			ret = fwnode_graph_parse_endpoint(csi2_ep, &fwep);
>> +
>> +			fwnode_handle_put(csi2_ep);
>> +
>> +			return ret ? ret : fwep.port;
>> +		}
>> +	}
>> +
>> +	return -ENXIO;
>> +}
> As the 1:1 mapping is the common case, would it make sense to modify
> media_entity_get_fwnode_pad() accordingly when .get_fwnode_pad is not
> set ?

Yes! In fact that was a previous revision of this patchset (v2). But I 
dropped that idea in v3 and v4 because it didn't seem to be getting any 
traction.

I guess I'll try again. I'll bring back [1] in v5.

Steve


[1] https://patchwork.linuxtv.org/patch/60312/

>   The current behaviour is to return the first pad that matches the
> requested direction, which could be preserved as a second-level fallback
> if the 1:1 mapping doesn't give the right direction (but I'm not sure
> there's a use case for that, the 1:1 mapping seems to be all we need if
> there's no specific .get_fwnode_pad implementation).
>
>> +
>>   static const struct media_entity_operations csi2_entity_ops = {
>>   	.link_setup = csi2_link_setup,
>>   	.link_validate = v4l2_subdev_link_validate,
>> +	.get_fwnode_pad = csi2_get_fwnode_pad,
>>   };
>>   
>>   static const struct v4l2_subdev_video_ops csi2_video_ops = {

[PATCH v4 07/17] media: video-mux: Implement get_fwnode_pad op

[Steve Longerbeam]

Implement get_fwnode_pad operation. If the endpoint is owned by the video
mux, return the endpoint's port number. The video mux maps fwnode port
numbers and pad indexes 1:1.

Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
 drivers/media/platform/video-mux.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
index 7b6c96a29aa5..f446ada82176 100644
--- a/drivers/media/platform/video-mux.c
+++ b/drivers/media/platform/video-mux.c
@@ -94,9 +94,38 @@ static int video_mux_link_setup(struct media_entity *entity,
 	return ret;
 }
 
+static int video_mux_get_fwnode_pad(struct media_entity *entity,
+				    struct fwnode_endpoint *endpoint)
+{
+	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
+	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
+	struct fwnode_handle *vmux_fwnode = dev_fwnode(vmux->subdev.dev);
+	struct fwnode_handle *vmux_ep;
+
+	/*
+	 * If the endpoint is one of ours, return the endpoint's port
+	 * number. This device maps port numbers and pad indexes 1:1.
+	 */
+	fwnode_graph_for_each_endpoint(vmux_fwnode, vmux_ep) {
+		if (endpoint->local_fwnode == vmux_ep) {
+			struct fwnode_endpoint fwep;
+			int ret;
+
+			ret = fwnode_graph_parse_endpoint(vmux_ep, &fwep);
+
+			fwnode_handle_put(vmux_ep);
+
+			return ret ? ret : fwep.port;
+		}
+	}
+
+	return -ENXIO;
+}
+
 static const struct media_entity_operations video_mux_ops = {
 	.link_setup = video_mux_link_setup,
 	.link_validate = v4l2_subdev_link_validate,
+	.get_fwnode_pad = video_mux_get_fwnode_pad,
 };
 
 static int video_mux_s_stream(struct v4l2_subdev *sd, int enable)
-- 
2.17.1

Re: [PATCH v4 07/17] media: video-mux: Implement get_fwnode_pad op

[Laurent Pinchart]

Hi Steve,

Thank you for the patch.

On Tue, Mar 03, 2020 at 03:42:46PM -0800, Steve Longerbeam wrote:
> Implement get_fwnode_pad operation. If the endpoint is owned by the video
> mux, return the endpoint's port number. The video mux maps fwnode port
> numbers and pad indexes 1:1.

If you follow my suggestion from 06/17, this patch could be dropped.

> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> ---
>  drivers/media/platform/video-mux.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
> index 7b6c96a29aa5..f446ada82176 100644
> --- a/drivers/media/platform/video-mux.c
> +++ b/drivers/media/platform/video-mux.c
> @@ -94,9 +94,38 @@ static int video_mux_link_setup(struct media_entity *entity,
>  	return ret;
>  }
>  
> +static int video_mux_get_fwnode_pad(struct media_entity *entity,
> +				    struct fwnode_endpoint *endpoint)
> +{
> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> +	struct fwnode_handle *vmux_fwnode = dev_fwnode(vmux->subdev.dev);
> +	struct fwnode_handle *vmux_ep;
> +
> +	/*
> +	 * If the endpoint is one of ours, return the endpoint's port
> +	 * number. This device maps port numbers and pad indexes 1:1.
> +	 */
> +	fwnode_graph_for_each_endpoint(vmux_fwnode, vmux_ep) {
> +		if (endpoint->local_fwnode == vmux_ep) {
> +			struct fwnode_endpoint fwep;
> +			int ret;
> +
> +			ret = fwnode_graph_parse_endpoint(vmux_ep, &fwep);
> +
> +			fwnode_handle_put(vmux_ep);
> +
> +			return ret ? ret : fwep.port;
> +		}
> +	}
> +
> +	return -ENXIO;
> +}
> +
>  static const struct media_entity_operations video_mux_ops = {
>  	.link_setup = video_mux_link_setup,
>  	.link_validate = v4l2_subdev_link_validate,
> +	.get_fwnode_pad = video_mux_get_fwnode_pad,
>  };
>  
>  static int video_mux_s_stream(struct v4l2_subdev *sd, int enable)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 07/17] media: video-mux: Implement get_fwnode_pad op

[Steve Longerbeam]

On 4/14/20 4:08 PM, Laurent Pinchart wrote:
> Hi Steve,
>
> Thank you for the patch.
>
> On Tue, Mar 03, 2020 at 03:42:46PM -0800, Steve Longerbeam wrote:
>> Implement get_fwnode_pad operation. If the endpoint is owned by the video
>> mux, return the endpoint's port number. The video mux maps fwnode port
>> numbers and pad indexes 1:1.
> If you follow my suggestion from 06/17, this patch could be dropped.

Agreed.

Steve

>
>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
>> ---
>>   drivers/media/platform/video-mux.c | 29 +++++++++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
>> index 7b6c96a29aa5..f446ada82176 100644
>> --- a/drivers/media/platform/video-mux.c
>> +++ b/drivers/media/platform/video-mux.c
>> @@ -94,9 +94,38 @@ static int video_mux_link_setup(struct media_entity *entity,
>>   	return ret;
>>   }
>>   
>> +static int video_mux_get_fwnode_pad(struct media_entity *entity,
>> +				    struct fwnode_endpoint *endpoint)
>> +{
>> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
>> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> +	struct fwnode_handle *vmux_fwnode = dev_fwnode(vmux->subdev.dev);
>> +	struct fwnode_handle *vmux_ep;
>> +
>> +	/*
>> +	 * If the endpoint is one of ours, return the endpoint's port
>> +	 * number. This device maps port numbers and pad indexes 1:1.
>> +	 */
>> +	fwnode_graph_for_each_endpoint(vmux_fwnode, vmux_ep) {
>> +		if (endpoint->local_fwnode == vmux_ep) {
>> +			struct fwnode_endpoint fwep;
>> +			int ret;
>> +
>> +			ret = fwnode_graph_parse_endpoint(vmux_ep, &fwep);
>> +
>> +			fwnode_handle_put(vmux_ep);
>> +
>> +			return ret ? ret : fwep.port;
>> +		}
>> +	}
>> +
>> +	return -ENXIO;
>> +}
>> +
>>   static const struct media_entity_operations video_mux_ops = {
>>   	.link_setup = video_mux_link_setup,
>>   	.link_validate = v4l2_subdev_link_validate,
>> +	.get_fwnode_pad = video_mux_get_fwnode_pad,
>>   };
>>   
>>   static int video_mux_s_stream(struct v4l2_subdev *sd, int enable)

[PATCH v4 08/17] media: imx: Add imx_media_create_fwnode_pad_link()

[Steve Longerbeam]

Add functions to create media links between a source and sink subdevice
based on fwnode endpoint connections between them.

Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
 drivers/staging/media/imx/imx-media-utils.c | 91 +++++++++++++++++++++
 drivers/staging/media/imx/imx-media.h       |  4 +
 2 files changed, 95 insertions(+)

diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 0788a1874557..87152bd9af22 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -729,6 +729,97 @@ void imx_media_grp_id_to_sd_name(char *sd_name, int sz, u32 grp_id, int ipu_id)
 }
 EXPORT_SYMBOL_GPL(imx_media_grp_id_to_sd_name);
 
+/*
+ * Look for and create a single fwnode link that connects a source
+ * subdevice to a sink pad.
+ */
+int imx_media_create_fwnode_pad_link(struct v4l2_subdev *src_sd,
+				     struct media_pad *sink)
+{
+	struct fwnode_handle *endpoint;
+
+	/* loop thru the source's fwnode endpoints */
+	fwnode_graph_for_each_endpoint(dev_fwnode(src_sd->dev), endpoint) {
+		struct fwnode_handle *remote_ep;
+		int src_idx, sink_idx, ret;
+		struct media_pad *src;
+
+		remote_ep = fwnode_graph_get_remote_endpoint(endpoint);
+		if (!remote_ep)
+			continue;
+
+		/*
+		 * ask the sink entity to verify that this fwnode link
+		 * actually does connect with the entity, and if so that
+		 * it connects to its requested sink pad.
+		 */
+		sink_idx = media_entity_get_fwnode_pad(sink->entity,
+						       remote_ep,
+						       MEDIA_PAD_FL_SINK);
+		fwnode_handle_put(remote_ep);
+
+		if (sink_idx < 0 || sink_idx != sink->index)
+			continue;
+
+		src_idx = media_entity_get_fwnode_pad(&src_sd->entity,
+						      endpoint,
+						      MEDIA_PAD_FL_SOURCE);
+		if (src_idx < 0)
+			continue;
+
+		/*
+		 * found the fwnode link that works, create the media
+		 * link for it.
+		 */
+
+		fwnode_handle_put(endpoint);
+
+		src = &src_sd->entity.pads[src_idx];
+
+		/* success if it already exists */
+		if (media_entity_find_link(src, sink))
+			return 0;
+
+		dev_dbg(src_sd->dev, "%s:%d -> %s:%d\n",
+			src_sd->entity.name, src_idx,
+			sink->entity->name, sink_idx);
+
+		ret = media_create_pad_link(&src_sd->entity, src_idx,
+					    sink->entity, sink_idx, 0);
+		if (ret)
+			dev_err(src_sd->dev,
+				"%s:%d -> %s:%d failed with %d\n",
+				src_sd->entity.name, src_idx,
+				sink->entity->name, sink_idx, ret);
+
+		return ret;
+	}
+
+	return -ENXIO;
+}
+EXPORT_SYMBOL_GPL(imx_media_create_fwnode_pad_link);
+
+int imx_media_create_fwnode_pad_links(struct v4l2_subdev *src_sd,
+				      struct v4l2_subdev *sink_sd)
+{
+	int i;
+
+	for (i = 0; i < sink_sd->entity.num_pads; i++) {
+		struct media_pad *pad = &sink_sd->entity.pads[i];
+		int ret;
+
+		if (!(pad->flags & MEDIA_PAD_FL_SINK))
+			continue;
+
+		ret = imx_media_create_fwnode_pad_link(src_sd, pad);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(imx_media_create_fwnode_pad_links);
+
 struct v4l2_subdev *
 imx_media_find_subdev_by_fwnode(struct imx_media_dev *imxmd,
 				struct fwnode_handle *fwnode)
diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
index 11861191324a..f90a65ba4ced 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -183,6 +183,10 @@ int imx_media_ipu_image_to_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
 				    struct ipu_image *image);
 void imx_media_grp_id_to_sd_name(char *sd_name, int sz,
 				 u32 grp_id, int ipu_id);
+int imx_media_create_fwnode_pad_link(struct v4l2_subdev *src_sd,
+				     struct media_pad *sink);
+int imx_media_create_fwnode_pad_links(struct v4l2_subdev *src_sd,
+				      struct v4l2_subdev *sink_sd);
 struct v4l2_subdev *
 imx_media_find_subdev_by_fwnode(struct imx_media_dev *imxmd,
 				struct fwnode_handle *fwnode);
-- 
2.17.1

Re: [PATCH v4 08/17] media: imx: Add imx_media_create_fwnode_pad_link()

[Laurent Pinchart]

Hi Steve,

Thank you for the patch.

On Tue, Mar 03, 2020 at 03:42:47PM -0800, Steve Longerbeam wrote:
> Add functions to create media links between a source and sink subdevice
> based on fwnode endpoint connections between them.
> 
> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> ---
>  drivers/staging/media/imx/imx-media-utils.c | 91 +++++++++++++++++++++
>  drivers/staging/media/imx/imx-media.h       |  4 +
>  2 files changed, 95 insertions(+)
> 
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index 0788a1874557..87152bd9af22 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -729,6 +729,97 @@ void imx_media_grp_id_to_sd_name(char *sd_name, int sz, u32 grp_id, int ipu_id)
>  }
>  EXPORT_SYMBOL_GPL(imx_media_grp_id_to_sd_name);
>  
> +/*
> + * Look for and create a single fwnode link that connects a source
> + * subdevice to a sink pad.
> + */
> +int imx_media_create_fwnode_pad_link(struct v4l2_subdev *src_sd,
> +				     struct media_pad *sink)
> +{
> +	struct fwnode_handle *endpoint;
> +
> +	/* loop thru the source's fwnode endpoints */
> +	fwnode_graph_for_each_endpoint(dev_fwnode(src_sd->dev), endpoint) {
> +		struct fwnode_handle *remote_ep;
> +		int src_idx, sink_idx, ret;
> +		struct media_pad *src;
> +
> +		remote_ep = fwnode_graph_get_remote_endpoint(endpoint);
> +		if (!remote_ep)
> +			continue;
> +
> +		/*
> +		 * ask the sink entity to verify that this fwnode link
> +		 * actually does connect with the entity, and if so that
> +		 * it connects to its requested sink pad.
> +		 */
> +		sink_idx = media_entity_get_fwnode_pad(sink->entity,
> +						       remote_ep,
> +						       MEDIA_PAD_FL_SINK);
> +		fwnode_handle_put(remote_ep);
> +
> +		if (sink_idx < 0 || sink_idx != sink->index)
> +			continue;
> +
> +		src_idx = media_entity_get_fwnode_pad(&src_sd->entity,
> +						      endpoint,
> +						      MEDIA_PAD_FL_SOURCE);
> +		if (src_idx < 0)
> +			continue;
> +
> +		/*
> +		 * found the fwnode link that works, create the media
> +		 * link for it.
> +		 */
> +
> +		fwnode_handle_put(endpoint);
> +
> +		src = &src_sd->entity.pads[src_idx];
> +
> +		/* success if it already exists */
> +		if (media_entity_find_link(src, sink))
> +			return 0;
> +
> +		dev_dbg(src_sd->dev, "%s:%d -> %s:%d\n",
> +			src_sd->entity.name, src_idx,
> +			sink->entity->name, sink_idx);
> +
> +		ret = media_create_pad_link(&src_sd->entity, src_idx,
> +					    sink->entity, sink_idx, 0);
> +		if (ret)
> +			dev_err(src_sd->dev,
> +				"%s:%d -> %s:%d failed with %d\n",
> +				src_sd->entity.name, src_idx,
> +				sink->entity->name, sink_idx, ret);
> +
> +		return ret;
> +	}
> +
> +	return -ENXIO;
> +}
> +EXPORT_SYMBOL_GPL(imx_media_create_fwnode_pad_link);
> +
> +int imx_media_create_fwnode_pad_links(struct v4l2_subdev *src_sd,
> +				      struct v4l2_subdev *sink_sd)
> +{
> +	int i;
> +
> +	for (i = 0; i < sink_sd->entity.num_pads; i++) {
> +		struct media_pad *pad = &sink_sd->entity.pads[i];
> +		int ret;
> +
> +		if (!(pad->flags & MEDIA_PAD_FL_SINK))
> +			continue;
> +
> +		ret = imx_media_create_fwnode_pad_link(src_sd, pad);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(imx_media_create_fwnode_pad_links);

As commented in 09/17, I think these should be core helpers.

> +
>  struct v4l2_subdev *
>  imx_media_find_subdev_by_fwnode(struct imx_media_dev *imxmd,
>  				struct fwnode_handle *fwnode)
> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> index 11861191324a..f90a65ba4ced 100644
> --- a/drivers/staging/media/imx/imx-media.h
> +++ b/drivers/staging/media/imx/imx-media.h
> @@ -183,6 +183,10 @@ int imx_media_ipu_image_to_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
>  				    struct ipu_image *image);
>  void imx_media_grp_id_to_sd_name(char *sd_name, int sz,
>  				 u32 grp_id, int ipu_id);
> +int imx_media_create_fwnode_pad_link(struct v4l2_subdev *src_sd,
> +				     struct media_pad *sink);
> +int imx_media_create_fwnode_pad_links(struct v4l2_subdev *src_sd,
> +				      struct v4l2_subdev *sink_sd);
>  struct v4l2_subdev *
>  imx_media_find_subdev_by_fwnode(struct imx_media_dev *imxmd,
>  				struct fwnode_handle *fwnode);

-- 
Regards,

Laurent Pinchart

[PATCH v4 09/17] media: video-mux: Create media links in bound notifier

[Steve Longerbeam]

Implement a notifier bound op to register media links from the remote
sub-device's source pad(s) to the video-mux sink pad(s).

Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
Changes in v3:
- this version does the work inline. The previous version called
  a media_create_fwnode_links() which is removed in v3.
---
 drivers/media/platform/video-mux.c | 92 ++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
index f446ada82176..3991b1ea671c 100644
--- a/drivers/media/platform/video-mux.c
+++ b/drivers/media/platform/video-mux.c
@@ -36,6 +36,12 @@ static const struct v4l2_mbus_framefmt video_mux_format_mbus_default = {
 	.field = V4L2_FIELD_NONE,
 };
 
+static inline struct video_mux *
+notifier_to_video_mux(struct v4l2_async_notifier *n)
+{
+	return container_of(n, struct video_mux, notifier);
+}
+
 static inline struct video_mux *v4l2_subdev_to_video_mux(struct v4l2_subdev *sd)
 {
 	return container_of(sd, struct video_mux, subdev);
@@ -360,6 +366,90 @@ static const struct v4l2_subdev_ops video_mux_subdev_ops = {
 	.video = &video_mux_subdev_video_ops,
 };
 
+static int video_mux_notify_bound(struct v4l2_async_notifier *notifier,
+				  struct v4l2_subdev *sd,
+				  struct v4l2_async_subdev *asd)
+{
+	struct video_mux *vmux = notifier_to_video_mux(notifier);
+	struct fwnode_handle *vmux_fwnode = dev_fwnode(vmux->subdev.dev);
+	struct fwnode_handle *sd_fwnode = dev_fwnode(sd->dev);
+	struct fwnode_handle *vmux_ep;
+
+	fwnode_graph_for_each_endpoint(vmux_fwnode, vmux_ep) {
+		struct fwnode_handle *remote_ep, *sd_ep;
+		struct media_pad *src_pad, *sink_pad;
+		struct fwnode_endpoint fwep;
+		int src_idx, sink_idx, ret;
+		bool remote_ep_belongs;
+
+		ret = fwnode_graph_parse_endpoint(vmux_ep, &fwep);
+		if (ret)
+			continue;
+
+		/* only create links to the vmux sink pads */
+		if (fwep.port >= vmux->subdev.entity.num_pads - 1)
+			continue;
+
+		sink_idx = fwep.port;
+		sink_pad = &vmux->subdev.entity.pads[sink_idx];
+
+		remote_ep = fwnode_graph_get_remote_endpoint(vmux_ep);
+		if (!remote_ep)
+			continue;
+
+		/*
+		 * verify that this remote endpoint is owned by the
+		 * sd, in case the sd does not check for that in its
+		 * .get_fwnode_pad operation or does not implement it.
+		 */
+		remote_ep_belongs = false;
+		fwnode_graph_for_each_endpoint(sd_fwnode, sd_ep) {
+			if (sd_ep == remote_ep) {
+				remote_ep_belongs = true;
+				fwnode_handle_put(sd_ep);
+				break;
+			}
+		}
+		if (!remote_ep_belongs)
+			continue;
+
+		src_idx = media_entity_get_fwnode_pad(&sd->entity, remote_ep,
+						      MEDIA_PAD_FL_SOURCE);
+		fwnode_handle_put(remote_ep);
+
+		if (src_idx < 0)
+			continue;
+
+		src_pad = &sd->entity.pads[src_idx];
+
+		/* skip this link if it already exists */
+		if (media_entity_find_link(src_pad, sink_pad))
+			continue;
+
+		ret = media_create_pad_link(&sd->entity, src_idx,
+					    &vmux->subdev.entity,
+					    sink_idx, 0);
+		if (ret) {
+			dev_err(vmux->subdev.dev,
+				"%s:%d -> %s:%d failed with %d\n",
+				sd->entity.name, src_idx,
+				vmux->subdev.entity.name, sink_idx, ret);
+			fwnode_handle_put(vmux_ep);
+			return ret;
+		}
+
+		dev_dbg(vmux->subdev.dev, "%s:%d -> %s:%d\n",
+			sd->entity.name, src_idx,
+			vmux->subdev.entity.name, sink_idx);
+	}
+
+	return 0;
+}
+
+static const struct v4l2_async_notifier_operations video_mux_notify_ops = {
+	.bound = video_mux_notify_bound,
+};
+
 static int video_mux_async_register(struct video_mux *vmux,
 				    unsigned int num_input_pads)
 {
@@ -397,6 +487,8 @@ static int video_mux_async_register(struct video_mux *vmux,
 		}
 	}
 
+	vmux->notifier.ops = &video_mux_notify_ops;
+
 	ret = v4l2_async_subdev_notifier_register(&vmux->subdev,
 						  &vmux->notifier);
 	if (ret)
-- 
2.17.1

Re: [PATCH v4 09/17] media: video-mux: Create media links in bound notifier

[Laurent Pinchart]

Hi Steve,

Thank you for the patch.

On Tue, Mar 03, 2020 at 03:42:48PM -0800, Steve Longerbeam wrote:
> Implement a notifier bound op to register media links from the remote
> sub-device's source pad(s) to the video-mux sink pad(s).
> 
> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> ---
> Changes in v3:
> - this version does the work inline. The previous version called
>   a media_create_fwnode_links() which is removed in v3.
> ---
>  drivers/media/platform/video-mux.c | 92 ++++++++++++++++++++++++++++++
>  1 file changed, 92 insertions(+)
> 
> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
> index f446ada82176..3991b1ea671c 100644
> --- a/drivers/media/platform/video-mux.c
> +++ b/drivers/media/platform/video-mux.c
> @@ -36,6 +36,12 @@ static const struct v4l2_mbus_framefmt video_mux_format_mbus_default = {
>  	.field = V4L2_FIELD_NONE,
>  };
>  
> +static inline struct video_mux *
> +notifier_to_video_mux(struct v4l2_async_notifier *n)
> +{
> +	return container_of(n, struct video_mux, notifier);
> +}
> +
>  static inline struct video_mux *v4l2_subdev_to_video_mux(struct v4l2_subdev *sd)
>  {
>  	return container_of(sd, struct video_mux, subdev);
> @@ -360,6 +366,90 @@ static const struct v4l2_subdev_ops video_mux_subdev_ops = {
>  	.video = &video_mux_subdev_video_ops,
>  };
>  
> +static int video_mux_notify_bound(struct v4l2_async_notifier *notifier,
> +				  struct v4l2_subdev *sd,
> +				  struct v4l2_async_subdev *asd)
> +{
> +	struct video_mux *vmux = notifier_to_video_mux(notifier);
> +	struct fwnode_handle *vmux_fwnode = dev_fwnode(vmux->subdev.dev);
> +	struct fwnode_handle *sd_fwnode = dev_fwnode(sd->dev);
> +	struct fwnode_handle *vmux_ep;

There doesn't seem to be anything in this function that is specific to
the video_mux driver. I think it would make sense to turn it into a
generic helper that creates links between two subdevs based on their
fwnode connections.

I also wonder if it wouldn't be more efficient to create links at
complete() time instead of bound() time, in which case the helper would
create all links for a given subdevice, not links between two specific
subdevices (or maybe all links for a given pad direction, to be able to
remove the existing link check below).

> +
> +	fwnode_graph_for_each_endpoint(vmux_fwnode, vmux_ep) {
> +		struct fwnode_handle *remote_ep, *sd_ep;
> +		struct media_pad *src_pad, *sink_pad;
> +		struct fwnode_endpoint fwep;
> +		int src_idx, sink_idx, ret;
> +		bool remote_ep_belongs;
> +
> +		ret = fwnode_graph_parse_endpoint(vmux_ep, &fwep);
> +		if (ret)
> +			continue;
> +
> +		/* only create links to the vmux sink pads */
> +		if (fwep.port >= vmux->subdev.entity.num_pads - 1)
> +			continue;
> +
> +		sink_idx = fwep.port;
> +		sink_pad = &vmux->subdev.entity.pads[sink_idx];
> +
> +		remote_ep = fwnode_graph_get_remote_endpoint(vmux_ep);
> +		if (!remote_ep)
> +			continue;
> +
> +		/*
> +		 * verify that this remote endpoint is owned by the
> +		 * sd, in case the sd does not check for that in its
> +		 * .get_fwnode_pad operation or does not implement it.
> +		 */
> +		remote_ep_belongs = false;
> +		fwnode_graph_for_each_endpoint(sd_fwnode, sd_ep) {
> +			if (sd_ep == remote_ep) {
> +				remote_ep_belongs = true;
> +				fwnode_handle_put(sd_ep);
> +				break;
> +			}
> +		}
> +		if (!remote_ep_belongs)
> +			continue;
> +
> +		src_idx = media_entity_get_fwnode_pad(&sd->entity, remote_ep,
> +						      MEDIA_PAD_FL_SOURCE);
> +		fwnode_handle_put(remote_ep);
> +
> +		if (src_idx < 0)
> +			continue;
> +
> +		src_pad = &sd->entity.pads[src_idx];
> +
> +		/* skip this link if it already exists */
> +		if (media_entity_find_link(src_pad, sink_pad))
> +			continue;

Have you encountered this in practice ? If we only create links for sink
pads this shouldn't happen.

> +
> +		ret = media_create_pad_link(&sd->entity, src_idx,
> +					    &vmux->subdev.entity,
> +					    sink_idx, 0);
> +		if (ret) {
> +			dev_err(vmux->subdev.dev,
> +				"%s:%d -> %s:%d failed with %d\n",
> +				sd->entity.name, src_idx,
> +				vmux->subdev.entity.name, sink_idx, ret);
> +			fwnode_handle_put(vmux_ep);
> +			return ret;
> +		}
> +
> +		dev_dbg(vmux->subdev.dev, "%s:%d -> %s:%d\n",
> +			sd->entity.name, src_idx,
> +			vmux->subdev.entity.name, sink_idx);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_async_notifier_operations video_mux_notify_ops = {
> +	.bound = video_mux_notify_bound,
> +};
> +
>  static int video_mux_async_register(struct video_mux *vmux,
>  				    unsigned int num_input_pads)
>  {
> @@ -397,6 +487,8 @@ static int video_mux_async_register(struct video_mux *vmux,
>  		}
>  	}
>  
> +	vmux->notifier.ops = &video_mux_notify_ops;
> +
>  	ret = v4l2_async_subdev_notifier_register(&vmux->subdev,
>  						  &vmux->notifier);
>  	if (ret)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 09/17] media: video-mux: Create media links in bound notifier

[Steve Longerbeam]

Hi Laurent,

On 4/14/20 4:16 PM, Laurent Pinchart wrote:
> Hi Steve,
>
> Thank you for the patch.
>
> On Tue, Mar 03, 2020 at 03:42:48PM -0800, Steve Longerbeam wrote:
>> Implement a notifier bound op to register media links from the remote
>> sub-device's source pad(s) to the video-mux sink pad(s).
>>
>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
>> ---
>> Changes in v3:
>> - this version does the work inline. The previous version called
>>    a media_create_fwnode_links() which is removed in v3.
>> ---
>>   drivers/media/platform/video-mux.c | 92 ++++++++++++++++++++++++++++++
>>   1 file changed, 92 insertions(+)
>>
>> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
>> index f446ada82176..3991b1ea671c 100644
>> --- a/drivers/media/platform/video-mux.c
>> +++ b/drivers/media/platform/video-mux.c
>> @@ -36,6 +36,12 @@ static const struct v4l2_mbus_framefmt video_mux_format_mbus_default = {
>>   	.field = V4L2_FIELD_NONE,
>>   };
>>   
>> +static inline struct video_mux *
>> +notifier_to_video_mux(struct v4l2_async_notifier *n)
>> +{
>> +	return container_of(n, struct video_mux, notifier);
>> +}
>> +
>>   static inline struct video_mux *v4l2_subdev_to_video_mux(struct v4l2_subdev *sd)
>>   {
>>   	return container_of(sd, struct video_mux, subdev);
>> @@ -360,6 +366,90 @@ static const struct v4l2_subdev_ops video_mux_subdev_ops = {
>>   	.video = &video_mux_subdev_video_ops,
>>   };
>>   
>> +static int video_mux_notify_bound(struct v4l2_async_notifier *notifier,
>> +				  struct v4l2_subdev *sd,
>> +				  struct v4l2_async_subdev *asd)
>> +{
>> +	struct video_mux *vmux = notifier_to_video_mux(notifier);
>> +	struct fwnode_handle *vmux_fwnode = dev_fwnode(vmux->subdev.dev);
>> +	struct fwnode_handle *sd_fwnode = dev_fwnode(sd->dev);
>> +	struct fwnode_handle *vmux_ep;
> There doesn't seem to be anything in this function that is specific to
> the video_mux driver. I think it would make sense to turn it into a
> generic helper that creates links between two subdevs based on their
> fwnode connections.

Agreed, in fact I wrote imx_media_create_fwnode_pad_links(src_sd, 
sink_sd) (patch 8 in this series), and it is completely generic, it 
could simply be renamed v4l2_create_fwnode_pad_links() and moved to core.

> I also wonder if it wouldn't be more efficient to create links at
> complete() time instead of bound() time, in which case the helper would
> create all links for a given subdevice, not links between two specific
> subdevices (or maybe all links for a given pad direction, to be able to
> remove the existing link check below).

It looks like that should be possible. The bound sub-devices at 
complete() time are available in the notifier->done list. I'll start 
looking at that for v5.

Steve


>
>> +
>> +	fwnode_graph_for_each_endpoint(vmux_fwnode, vmux_ep) {
>> +		struct fwnode_handle *remote_ep, *sd_ep;
>> +		struct media_pad *src_pad, *sink_pad;
>> +		struct fwnode_endpoint fwep;
>> +		int src_idx, sink_idx, ret;
>> +		bool remote_ep_belongs;
>> +
>> +		ret = fwnode_graph_parse_endpoint(vmux_ep, &fwep);
>> +		if (ret)
>> +			continue;
>> +
>> +		/* only create links to the vmux sink pads */
>> +		if (fwep.port >= vmux->subdev.entity.num_pads - 1)
>> +			continue;
>> +
>> +		sink_idx = fwep.port;
>> +		sink_pad = &vmux->subdev.entity.pads[sink_idx];
>> +
>> +		remote_ep = fwnode_graph_get_remote_endpoint(vmux_ep);
>> +		if (!remote_ep)
>> +			continue;
>> +
>> +		/*
>> +		 * verify that this remote endpoint is owned by the
>> +		 * sd, in case the sd does not check for that in its
>> +		 * .get_fwnode_pad operation or does not implement it.
>> +		 */
>> +		remote_ep_belongs = false;
>> +		fwnode_graph_for_each_endpoint(sd_fwnode, sd_ep) {
>> +			if (sd_ep == remote_ep) {
>> +				remote_ep_belongs = true;
>> +				fwnode_handle_put(sd_ep);
>> +				break;
>> +			}
>> +		}
>> +		if (!remote_ep_belongs)
>> +			continue;
>> +
>> +		src_idx = media_entity_get_fwnode_pad(&sd->entity, remote_ep,
>> +						      MEDIA_PAD_FL_SOURCE);
>> +		fwnode_handle_put(remote_ep);
>> +
>> +		if (src_idx < 0)
>> +			continue;
>> +
>> +		src_pad = &sd->entity.pads[src_idx];
>> +
>> +		/* skip this link if it already exists */
>> +		if (media_entity_find_link(src_pad, sink_pad))
>> +			continue;
> Have you encountered this in practice ? If we only create links for sink
> pads this shouldn't happen.
>
>> +
>> +		ret = media_create_pad_link(&sd->entity, src_idx,
>> +					    &vmux->subdev.entity,
>> +					    sink_idx, 0);
>> +		if (ret) {
>> +			dev_err(vmux->subdev.dev,
>> +				"%s:%d -> %s:%d failed with %d\n",
>> +				sd->entity.name, src_idx,
>> +				vmux->subdev.entity.name, sink_idx, ret);
>> +			fwnode_handle_put(vmux_ep);
>> +			return ret;
>> +		}
>> +
>> +		dev_dbg(vmux->subdev.dev, "%s:%d -> %s:%d\n",
>> +			sd->entity.name, src_idx,
>> +			vmux->subdev.entity.name, sink_idx);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct v4l2_async_notifier_operations video_mux_notify_ops = {
>> +	.bound = video_mux_notify_bound,
>> +};
>> +
>>   static int video_mux_async_register(struct video_mux *vmux,
>>   				    unsigned int num_input_pads)
>>   {
>> @@ -397,6 +487,8 @@ static int video_mux_async_register(struct video_mux *vmux,
>>   		}
>>   	}
>>   
>> +	vmux->notifier.ops = &video_mux_notify_ops;
>> +
>>   	ret = v4l2_async_subdev_notifier_register(&vmux->subdev,
>>   						  &vmux->notifier);
>>   	if (ret)

Re: [PATCH v4 09/17] media: video-mux: Create media links in bound notifier

[Steve Longerbeam]

Hi Laurent,

On 4/14/20 4:47 PM, Steve Longerbeam wrote:
> Hi Laurent,
>
> On 4/14/20 4:16 PM, Laurent Pinchart wrote:
>> Hi Steve,
>>
>> Thank you for the patch.
>>
>> On Tue, Mar 03, 2020 at 03:42:48PM -0800, Steve Longerbeam wrote:
>>> Implement a notifier bound op to register media links from the remote
>>> sub-device's source pad(s) to the video-mux sink pad(s).
>>>
>>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
>>> ---
>>> Changes in v3:
>>> - this version does the work inline. The previous version called
>>>    a media_create_fwnode_links() which is removed in v3.
>>> ---
>>>   drivers/media/platform/video-mux.c | 92 
>>> ++++++++++++++++++++++++++++++
>>>   1 file changed, 92 insertions(+)
>>>
>>> diff --git a/drivers/media/platform/video-mux.c 
>>> b/drivers/media/platform/video-mux.c
>>> index f446ada82176..3991b1ea671c 100644
>>> --- a/drivers/media/platform/video-mux.c
>>> +++ b/drivers/media/platform/video-mux.c
>>> @@ -36,6 +36,12 @@ static const struct v4l2_mbus_framefmt 
>>> video_mux_format_mbus_default = {
>>>       .field = V4L2_FIELD_NONE,
>>>   };
>>>   +static inline struct video_mux *
>>> +notifier_to_video_mux(struct v4l2_async_notifier *n)
>>> +{
>>> +    return container_of(n, struct video_mux, notifier);
>>> +}
>>> +
>>>   static inline struct video_mux *v4l2_subdev_to_video_mux(struct 
>>> v4l2_subdev *sd)
>>>   {
>>>       return container_of(sd, struct video_mux, subdev);
>>> @@ -360,6 +366,90 @@ static const struct v4l2_subdev_ops 
>>> video_mux_subdev_ops = {
>>>       .video = &video_mux_subdev_video_ops,
>>>   };
>>>   +static int video_mux_notify_bound(struct v4l2_async_notifier 
>>> *notifier,
>>> +                  struct v4l2_subdev *sd,
>>> +                  struct v4l2_async_subdev *asd)
>>> +{
>>> +    struct video_mux *vmux = notifier_to_video_mux(notifier);
>>> +    struct fwnode_handle *vmux_fwnode = dev_fwnode(vmux->subdev.dev);
>>> +    struct fwnode_handle *sd_fwnode = dev_fwnode(sd->dev);
>>> +    struct fwnode_handle *vmux_ep;
>> There doesn't seem to be anything in this function that is specific to
>> the video_mux driver. I think it would make sense to turn it into a
>> generic helper that creates links between two subdevs based on their
>> fwnode connections.
>
> Agreed, in fact I wrote imx_media_create_fwnode_pad_links(src_sd, 
> sink_sd) (patch 8 in this series), and it is completely generic, it 
> could simply be renamed v4l2_create_fwnode_pad_links() and moved to core.
>
>> I also wonder if it wouldn't be more efficient to create links at
>> complete() time instead of bound() time, in which case the helper would
>> create all links for a given subdevice, not links between two specific
>> subdevices (or maybe all links for a given pad direction, to be able to
>> remove the existing link check below).
>
> It looks like that should be possible. The bound sub-devices at 
> complete() time are available in the notifier->done list. I'll start 
> looking at that for v5.

Actually for sub-devices that won't work. The .complete() callback is 
only called on the root notifier, not on subdev notifiers. I don't think 
it's a big deal, it works fine to create links from one source subdev at 
a time in .bound(), and I don't see it as much of a efficiency hit.

Steve

>
>
>
>>
>>> +
>>> +    fwnode_graph_for_each_endpoint(vmux_fwnode, vmux_ep) {
>>> +        struct fwnode_handle *remote_ep, *sd_ep;
>>> +        struct media_pad *src_pad, *sink_pad;
>>> +        struct fwnode_endpoint fwep;
>>> +        int src_idx, sink_idx, ret;
>>> +        bool remote_ep_belongs;
>>> +
>>> +        ret = fwnode_graph_parse_endpoint(vmux_ep, &fwep);
>>> +        if (ret)
>>> +            continue;
>>> +
>>> +        /* only create links to the vmux sink pads */
>>> +        if (fwep.port >= vmux->subdev.entity.num_pads - 1)
>>> +            continue;
>>> +
>>> +        sink_idx = fwep.port;
>>> +        sink_pad = &vmux->subdev.entity.pads[sink_idx];
>>> +
>>> +        remote_ep = fwnode_graph_get_remote_endpoint(vmux_ep);
>>> +        if (!remote_ep)
>>> +            continue;
>>> +
>>> +        /*
>>> +         * verify that this remote endpoint is owned by the
>>> +         * sd, in case the sd does not check for that in its
>>> +         * .get_fwnode_pad operation or does not implement it.
>>> +         */
>>> +        remote_ep_belongs = false;
>>> +        fwnode_graph_for_each_endpoint(sd_fwnode, sd_ep) {
>>> +            if (sd_ep == remote_ep) {
>>> +                remote_ep_belongs = true;
>>> +                fwnode_handle_put(sd_ep);
>>> +                break;
>>> +            }
>>> +        }
>>> +        if (!remote_ep_belongs)
>>> +            continue;
>>> +
>>> +        src_idx = media_entity_get_fwnode_pad(&sd->entity, remote_ep,
>>> +                              MEDIA_PAD_FL_SOURCE);
>>> +        fwnode_handle_put(remote_ep);
>>> +
>>> +        if (src_idx < 0)
>>> +            continue;
>>> +
>>> +        src_pad = &sd->entity.pads[src_idx];
>>> +
>>> +        /* skip this link if it already exists */
>>> +        if (media_entity_find_link(src_pad, sink_pad))
>>> +            continue;
>> Have you encountered this in practice ? If we only create links for sink
>> pads this shouldn't happen.
>>
>>> +
>>> +        ret = media_create_pad_link(&sd->entity, src_idx,
>>> +                        &vmux->subdev.entity,
>>> +                        sink_idx, 0);
>>> +        if (ret) {
>>> +            dev_err(vmux->subdev.dev,
>>> +                "%s:%d -> %s:%d failed with %d\n",
>>> +                sd->entity.name, src_idx,
>>> +                vmux->subdev.entity.name, sink_idx, ret);
>>> +            fwnode_handle_put(vmux_ep);
>>> +            return ret;
>>> +        }
>>> +
>>> +        dev_dbg(vmux->subdev.dev, "%s:%d -> %s:%d\n",
>>> +            sd->entity.name, src_idx,
>>> +            vmux->subdev.entity.name, sink_idx);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct v4l2_async_notifier_operations 
>>> video_mux_notify_ops = {
>>> +    .bound = video_mux_notify_bound,
>>> +};
>>> +
>>>   static int video_mux_async_register(struct video_mux *vmux,
>>>                       unsigned int num_input_pads)
>>>   {
>>> @@ -397,6 +487,8 @@ static int video_mux_async_register(struct 
>>> video_mux *vmux,
>>>           }
>>>       }
>>>   +    vmux->notifier.ops = &video_mux_notify_ops;
>>> +
>>>       ret = v4l2_async_subdev_notifier_register(&vmux->subdev,
>>>                             &vmux->notifier);
>>>       if (ret)
>

[PATCH v4 10/17] media: imx: mipi csi-2: Create media links in bound notifier

[Steve Longerbeam]

Implement a notifier bound op to register media links from the remote
sub-device's source pad(s) to the mipi csi-2 receiver sink pad.

Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
Changes in v3:
- call a local imx-media utility imx_media_create_fwnode_pad_link()
  that creates a single link.
---
 drivers/staging/media/imx/imx6-mipi-csi2.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c
index 8500207e5ea9..26c0b579e6ae 100644
--- a/drivers/staging/media/imx/imx6-mipi-csi2.c
+++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
@@ -91,6 +91,11 @@ static inline struct csi2_dev *sd_to_dev(struct v4l2_subdev *sdev)
 	return container_of(sdev, struct csi2_dev, sd);
 }
 
+static inline struct csi2_dev *notifier_to_dev(struct v4l2_async_notifier *n)
+{
+	return container_of(n, struct csi2_dev, notifier);
+}
+
 /*
  * The required sequence of MIPI CSI-2 startup as specified in the i.MX6
  * reference manual is as follows:
@@ -559,6 +564,20 @@ static const struct v4l2_subdev_internal_ops csi2_internal_ops = {
 	.registered = csi2_registered,
 };
 
+static int csi2_notify_bound(struct v4l2_async_notifier *notifier,
+			     struct v4l2_subdev *sd,
+			     struct v4l2_async_subdev *asd)
+{
+	struct csi2_dev *csi2 = notifier_to_dev(notifier);
+	struct media_pad *sink = &csi2->sd.entity.pads[CSI2_SINK_PAD];
+
+	return imx_media_create_fwnode_pad_link(sd, sink);
+}
+
+static const struct v4l2_async_notifier_operations csi2_notify_ops = {
+	.bound = csi2_notify_bound,
+};
+
 static int csi2_async_register(struct csi2_dev *csi2)
 {
 	struct v4l2_fwnode_endpoint vep = {
@@ -597,6 +616,8 @@ static int csi2_async_register(struct csi2_dev *csi2)
 
 	fwnode_handle_put(ep);
 
+	csi2->notifier.ops = &csi2_notify_ops;
+
 	ret = v4l2_async_subdev_notifier_register(&csi2->sd,
 						  &csi2->notifier);
 	if (ret)
-- 
2.17.1

[PATCH v4 11/17] media: imx7: mipi csis: Create media links in bound notifier

[Steve Longerbeam]

Implement a notifier bound op to register media links from the remote
sub-device's source pad(s) to the mipi csi-2 receiver sink pad.

Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
Changes in v3:
- call a local imx-media utility imx_media_create_fwnode_pad_link().
Changes in v2:
- Move notifier_to_csis_state() next to mipi_sd_to_csis_state(), remove
  unnecessary inline, and rename to mipi_notifier_to_csis_state().
  Suggested by Rui Silva.
---
 drivers/staging/media/imx/imx7-mipi-csis.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 7e872d8f51e0..363545127047 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -317,6 +317,12 @@ static int mipi_csis_dump_regs(struct csi_state *state)
 	return 0;
 }
 
+static struct csi_state *
+mipi_notifier_to_csis_state(struct v4l2_async_notifier *n)
+{
+	return container_of(n, struct csi_state, notifier);
+}
+
 static struct csi_state *mipi_sd_to_csis_state(struct v4l2_subdev *sdev)
 {
 	return container_of(sdev, struct csi_state, mipi_sd);
@@ -828,6 +834,20 @@ static int mipi_csis_parse_dt(struct platform_device *pdev,
 
 static int mipi_csis_pm_resume(struct device *dev, bool runtime);
 
+static int mipi_csis_notify_bound(struct v4l2_async_notifier *notifier,
+				  struct v4l2_subdev *sd,
+				  struct v4l2_async_subdev *asd)
+{
+	struct csi_state *state = mipi_notifier_to_csis_state(notifier);
+	struct media_pad *sink = &state->mipi_sd.entity.pads[CSIS_PAD_SINK];
+
+	return imx_media_create_fwnode_pad_link(sd, sink);
+}
+
+static const struct v4l2_async_notifier_operations mipi_csis_notify_ops = {
+	.bound = mipi_csis_notify_bound,
+};
+
 static int mipi_csis_subdev_init(struct v4l2_subdev *mipi_sd,
 				 struct platform_device *pdev,
 				 const struct v4l2_subdev_ops *ops)
@@ -899,6 +919,8 @@ static int mipi_csis_async_register(struct csi_state *state)
 
 	fwnode_handle_put(ep);
 
+	state->notifier.ops = &mipi_csis_notify_ops;
+
 	ret = v4l2_async_subdev_notifier_register(&state->mipi_sd,
 						  &state->notifier);
 	if (ret)
-- 
2.17.1

[PATCH v4 12/17] media: imx7: csi: Create media links in bound notifier

[Steve Longerbeam]

Implement a notifier bound op to register media links from the remote
sub-device's source pad(s) to the CSI sink pad.

Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
Reviewed-by: Rui Miguel Silva <rmfrfs@gmail.com>
---
Changes in v3:
- call a local imx-media utility imx_media_create_fwnode_pad_link().
Changes in v2:
- Rename notifier_to_dev() to imx7_csi_notifier_to_dev() and remove
  unnecessary inline. Suggested by Rui Silva.
---
 drivers/staging/media/imx/imx7-media-csi.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index 776eb42ae5c8..88e21c13e420 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -196,6 +196,12 @@ struct imx7_csi {
 	struct completion last_eof_completion;
 };
 
+static struct imx7_csi *
+imx7_csi_notifier_to_dev(struct v4l2_async_notifier *n)
+{
+	return container_of(n, struct imx7_csi, notifier);
+}
+
 static u32 imx7_csi_reg_read(struct imx7_csi *csi, unsigned int offset)
 {
 	return readl(csi->regbase + offset);
@@ -1180,6 +1186,20 @@ static const struct v4l2_subdev_internal_ops imx7_csi_internal_ops = {
 	.unregistered	= imx7_csi_unregistered,
 };
 
+static int imx7_csi_notify_bound(struct v4l2_async_notifier *notifier,
+				 struct v4l2_subdev *sd,
+				 struct v4l2_async_subdev *asd)
+{
+	struct imx7_csi *csi = imx7_csi_notifier_to_dev(notifier);
+	struct media_pad *sink = &csi->sd.entity.pads[IMX7_CSI_PAD_SINK];
+
+	return imx_media_create_fwnode_pad_link(sd, sink);
+}
+
+static const struct v4l2_async_notifier_operations imx7_csi_notify_ops = {
+	.bound = imx7_csi_notify_bound,
+};
+
 static int imx7_csi_async_register(struct imx7_csi *csi)
 {
 	struct v4l2_async_subdev *asd = NULL;
@@ -1210,6 +1230,8 @@ static int imx7_csi_async_register(struct imx7_csi *csi)
 		}
 	}
 
+	csi->notifier.ops = &imx7_csi_notify_ops;
+
 	ret = v4l2_async_subdev_notifier_register(&csi->sd, &csi->notifier);
 	if (ret)
 		return ret;
-- 
2.17.1

[PATCH v4 13/17] media: imx: csi: Create media links in bound notifier

[Steve Longerbeam]

Implement a notifier bound op to register media links from the remote
sub-device's source pad(s) to the CSI sink pad.

Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
Changes in v3:
- call a local imx-media utility imx_media_create_fwnode_pad_link().
---
 drivers/staging/media/imx/imx-media-csi.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 35f2512ed2a9..59ab1cf72841 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -120,6 +120,11 @@ static inline struct csi_priv *sd_to_dev(struct v4l2_subdev *sdev)
 	return container_of(sdev, struct csi_priv, sd);
 }
 
+static inline struct csi_priv *notifier_to_dev(struct v4l2_async_notifier *n)
+{
+	return container_of(n, struct csi_priv, notifier);
+}
+
 static inline bool is_parallel_bus(struct v4l2_fwnode_endpoint *ep)
 {
 	return ep->bus_type != V4L2_MBUS_CSI2_DPHY;
@@ -1889,6 +1894,20 @@ static const struct v4l2_subdev_internal_ops csi_internal_ops = {
 	.unregistered = csi_unregistered,
 };
 
+static int imx_csi_notify_bound(struct v4l2_async_notifier *notifier,
+				struct v4l2_subdev *sd,
+				struct v4l2_async_subdev *asd)
+{
+	struct csi_priv *priv = notifier_to_dev(notifier);
+	struct media_pad *sink = &priv->sd.entity.pads[CSI_SINK_PAD];
+
+	return imx_media_create_fwnode_pad_link(sd, sink);
+}
+
+static const struct v4l2_async_notifier_operations csi_notify_ops = {
+	.bound = imx_csi_notify_bound,
+};
+
 static int imx_csi_async_register(struct csi_priv *priv)
 {
 	struct v4l2_async_subdev *asd = NULL;
@@ -1926,6 +1945,8 @@ static int imx_csi_async_register(struct csi_priv *priv)
 		}
 	}
 
+	priv->notifier.ops = &csi_notify_ops;
+
 	ret = v4l2_async_subdev_notifier_register(&priv->sd,
 						  &priv->notifier);
 	if (ret)
-- 
2.17.1

[PATCH v4 14/17] media: imx: csi: Lookup upstream endpoint with imx_media_get_pad_fwnode

[Steve Longerbeam]

Fix the 1:1 port-id:pad-index assumption for the upstream subdevice, by
searching the upstream subdevice's endpoints for one that maps to the
pad's index. This is carried out by a new reverse mapping function
imx_media_get_pad_fwnode().

Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
 drivers/staging/media/imx/imx-media-csi.c   | 22 ++++----------
 drivers/staging/media/imx/imx-media-utils.c | 33 +++++++++++++++++++++
 drivers/staging/media/imx/imx-media.h       |  1 +
 drivers/staging/media/imx/imx7-media-csi.c  | 25 +++++-----------
 4 files changed, 47 insertions(+), 34 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 59ab1cf72841..0388d73e916c 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -164,7 +164,7 @@ static inline bool requires_passthrough(struct v4l2_fwnode_endpoint *ep,
 static int csi_get_upstream_endpoint(struct csi_priv *priv,
 				     struct v4l2_fwnode_endpoint *ep)
 {
-	struct device_node *endpoint, *port;
+	struct fwnode_handle *endpoint;
 	struct media_entity *src;
 	struct v4l2_subdev *sd;
 	struct media_pad *pad;
@@ -203,23 +203,13 @@ static int csi_get_upstream_endpoint(struct csi_priv *priv,
 	if (!pad)
 		return -ENODEV;
 
-	sd = media_entity_to_v4l2_subdev(pad->entity);
+	endpoint = imx_media_get_pad_fwnode(pad);
+	if (IS_ERR(endpoint))
+		return PTR_ERR(endpoint);
 
-	/*
-	 * NOTE: this assumes an OF-graph port id is the same as a
-	 * media pad index.
-	 */
-	port = of_graph_get_port_by_id(sd->dev->of_node, pad->index);
-	if (!port)
-		return -ENODEV;
-
-	endpoint = of_get_next_child(port, NULL);
-	of_node_put(port);
-	if (!endpoint)
-		return -ENODEV;
+	v4l2_fwnode_endpoint_parse(endpoint, ep);
 
-	v4l2_fwnode_endpoint_parse(of_fwnode_handle(endpoint), ep);
-	of_node_put(endpoint);
+	fwnode_handle_put(endpoint);
 
 	return 0;
 }
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 87152bd9af22..61752c6b074d 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -1007,6 +1007,39 @@ imx_media_pipeline_video_device(struct media_entity *start_entity,
 }
 EXPORT_SYMBOL_GPL(imx_media_pipeline_video_device);
 
+/*
+ * Find a fwnode endpoint that maps to the given subdevice's pad.
+ * If there are multiple endpoints that map to the pad, only the
+ * first endpoint encountered is returned.
+ *
+ * On success the refcount of the returned fwnode endpoint is
+ * incremented.
+ */
+struct fwnode_handle *imx_media_get_pad_fwnode(struct media_pad *pad)
+{
+	struct fwnode_handle *endpoint;
+	struct v4l2_subdev *sd;
+
+	if (!is_media_entity_v4l2_subdev(pad->entity))
+		return ERR_PTR(-ENODEV);
+
+	sd = media_entity_to_v4l2_subdev(pad->entity);
+
+	fwnode_graph_for_each_endpoint(dev_fwnode(sd->dev), endpoint) {
+		int pad_idx = media_entity_get_fwnode_pad(&sd->entity,
+							  endpoint,
+							  pad->flags);
+		if (pad_idx < 0)
+			continue;
+
+		if (pad_idx == pad->index)
+			return endpoint;
+	}
+
+	return ERR_PTR(-ENODEV);
+}
+EXPORT_SYMBOL_GPL(imx_media_get_pad_fwnode);
+
 /*
  * Turn current pipeline streaming on/off starting from entity.
  */
diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
index f90a65ba4ced..5f23d852122f 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -205,6 +205,7 @@ imx_media_pipeline_subdev(struct media_entity *start_entity, u32 grp_id,
 struct video_device *
 imx_media_pipeline_video_device(struct media_entity *start_entity,
 				enum v4l2_buf_type buftype, bool upstream);
+struct fwnode_handle *imx_media_get_pad_fwnode(struct media_pad *pad);
 
 struct imx_media_dma_buf {
 	void          *virt;
diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index 88e21c13e420..9b31379611ac 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -439,9 +439,8 @@ static int imx7_csi_get_upstream_endpoint(struct imx7_csi *csi,
 					  struct v4l2_fwnode_endpoint *ep,
 					  bool skip_mux)
 {
-	struct device_node *endpoint, *port;
+	struct fwnode_handle *endpoint;
 	struct media_entity *src;
-	struct v4l2_subdev *sd;
 	struct media_pad *pad;
 
 	if (!csi->src_sd)
@@ -463,29 +462,19 @@ static int imx7_csi_get_upstream_endpoint(struct imx7_csi *csi,
 	if (!pad)
 		return -ENODEV;
 
-	sd = media_entity_to_v4l2_subdev(pad->entity);
-
 	/* To get bus type we may need to skip video mux */
 	if (skip_mux && src->function == MEDIA_ENT_F_VID_MUX) {
-		src = &sd->entity;
+		src = pad->entity;
 		goto skip_video_mux;
 	}
 
-	/*
-	 * NOTE: this assumes an OF-graph port id is the same as a
-	 * media pad index.
-	 */
-	port = of_graph_get_port_by_id(sd->dev->of_node, pad->index);
-	if (!port)
-		return -ENODEV;
+	endpoint = imx_media_get_pad_fwnode(pad);
+	if (IS_ERR(endpoint))
+		return PTR_ERR(endpoint);
 
-	endpoint = of_get_next_child(port, NULL);
-	of_node_put(port);
-	if (!endpoint)
-		return -ENODEV;
+	v4l2_fwnode_endpoint_parse(endpoint, ep);
 
-	v4l2_fwnode_endpoint_parse(of_fwnode_handle(endpoint), ep);
-	of_node_put(endpoint);
+	fwnode_handle_put(endpoint);
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH v4 14/17] media: imx: csi: Lookup upstream endpoint with imx_media_get_pad_fwnode

[Laurent Pinchart]

Hi Steve,

Thank you for the patch.

On Tue, Mar 03, 2020 at 03:42:53PM -0800, Steve Longerbeam wrote:
> Fix the 1:1 port-id:pad-index assumption for the upstream subdevice, by
> searching the upstream subdevice's endpoints for one that maps to the
> pad's index. This is carried out by a new reverse mapping function
> imx_media_get_pad_fwnode().

This gets pretty inefficient :-S I think it's fine for now, but I
believe there's room for additional refactoring to avoid the need to get
the upstream endpoint in the first place.

> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> ---
>  drivers/staging/media/imx/imx-media-csi.c   | 22 ++++----------
>  drivers/staging/media/imx/imx-media-utils.c | 33 +++++++++++++++++++++
>  drivers/staging/media/imx/imx-media.h       |  1 +
>  drivers/staging/media/imx/imx7-media-csi.c  | 25 +++++-----------
>  4 files changed, 47 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index 59ab1cf72841..0388d73e916c 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -164,7 +164,7 @@ static inline bool requires_passthrough(struct v4l2_fwnode_endpoint *ep,
>  static int csi_get_upstream_endpoint(struct csi_priv *priv,
>  				     struct v4l2_fwnode_endpoint *ep)
>  {
> -	struct device_node *endpoint, *port;
> +	struct fwnode_handle *endpoint;
>  	struct media_entity *src;
>  	struct v4l2_subdev *sd;
>  	struct media_pad *pad;
> @@ -203,23 +203,13 @@ static int csi_get_upstream_endpoint(struct csi_priv *priv,
>  	if (!pad)
>  		return -ENODEV;
>  
> -	sd = media_entity_to_v4l2_subdev(pad->entity);
> +	endpoint = imx_media_get_pad_fwnode(pad);
> +	if (IS_ERR(endpoint))
> +		return PTR_ERR(endpoint);
>  
> -	/*
> -	 * NOTE: this assumes an OF-graph port id is the same as a
> -	 * media pad index.
> -	 */
> -	port = of_graph_get_port_by_id(sd->dev->of_node, pad->index);
> -	if (!port)
> -		return -ENODEV;
> -
> -	endpoint = of_get_next_child(port, NULL);
> -	of_node_put(port);
> -	if (!endpoint)
> -		return -ENODEV;
> +	v4l2_fwnode_endpoint_parse(endpoint, ep);
>  
> -	v4l2_fwnode_endpoint_parse(of_fwnode_handle(endpoint), ep);
> -	of_node_put(endpoint);
> +	fwnode_handle_put(endpoint);
>  
>  	return 0;
>  }
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index 87152bd9af22..61752c6b074d 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -1007,6 +1007,39 @@ imx_media_pipeline_video_device(struct media_entity *start_entity,
>  }
>  EXPORT_SYMBOL_GPL(imx_media_pipeline_video_device);
>  
> +/*
> + * Find a fwnode endpoint that maps to the given subdevice's pad.
> + * If there are multiple endpoints that map to the pad, only the
> + * first endpoint encountered is returned.
> + *
> + * On success the refcount of the returned fwnode endpoint is
> + * incremented.
> + */
> +struct fwnode_handle *imx_media_get_pad_fwnode(struct media_pad *pad)
> +{
> +	struct fwnode_handle *endpoint;
> +	struct v4l2_subdev *sd;
> +
> +	if (!is_media_entity_v4l2_subdev(pad->entity))
> +		return ERR_PTR(-ENODEV);
> +
> +	sd = media_entity_to_v4l2_subdev(pad->entity);
> +
> +	fwnode_graph_for_each_endpoint(dev_fwnode(sd->dev), endpoint) {
> +		int pad_idx = media_entity_get_fwnode_pad(&sd->entity,
> +							  endpoint,
> +							  pad->flags);
> +		if (pad_idx < 0)
> +			continue;
> +
> +		if (pad_idx == pad->index)
> +			return endpoint;
> +	}
> +
> +	return ERR_PTR(-ENODEV);
> +}
> +EXPORT_SYMBOL_GPL(imx_media_get_pad_fwnode);
> +
>  /*
>   * Turn current pipeline streaming on/off starting from entity.
>   */
> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> index f90a65ba4ced..5f23d852122f 100644
> --- a/drivers/staging/media/imx/imx-media.h
> +++ b/drivers/staging/media/imx/imx-media.h
> @@ -205,6 +205,7 @@ imx_media_pipeline_subdev(struct media_entity *start_entity, u32 grp_id,
>  struct video_device *
>  imx_media_pipeline_video_device(struct media_entity *start_entity,
>  				enum v4l2_buf_type buftype, bool upstream);
> +struct fwnode_handle *imx_media_get_pad_fwnode(struct media_pad *pad);
>  
>  struct imx_media_dma_buf {
>  	void          *virt;
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index 88e21c13e420..9b31379611ac 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -439,9 +439,8 @@ static int imx7_csi_get_upstream_endpoint(struct imx7_csi *csi,
>  					  struct v4l2_fwnode_endpoint *ep,
>  					  bool skip_mux)
>  {
> -	struct device_node *endpoint, *port;
> +	struct fwnode_handle *endpoint;
>  	struct media_entity *src;
> -	struct v4l2_subdev *sd;
>  	struct media_pad *pad;
>  
>  	if (!csi->src_sd)
> @@ -463,29 +462,19 @@ static int imx7_csi_get_upstream_endpoint(struct imx7_csi *csi,
>  	if (!pad)
>  		return -ENODEV;
>  
> -	sd = media_entity_to_v4l2_subdev(pad->entity);
> -
>  	/* To get bus type we may need to skip video mux */
>  	if (skip_mux && src->function == MEDIA_ENT_F_VID_MUX) {
> -		src = &sd->entity;
> +		src = pad->entity;
>  		goto skip_video_mux;
>  	}
>  
> -	/*
> -	 * NOTE: this assumes an OF-graph port id is the same as a
> -	 * media pad index.
> -	 */
> -	port = of_graph_get_port_by_id(sd->dev->of_node, pad->index);
> -	if (!port)
> -		return -ENODEV;
> +	endpoint = imx_media_get_pad_fwnode(pad);
> +	if (IS_ERR(endpoint))
> +		return PTR_ERR(endpoint);
>  
> -	endpoint = of_get_next_child(port, NULL);
> -	of_node_put(port);
> -	if (!endpoint)
> -		return -ENODEV;
> +	v4l2_fwnode_endpoint_parse(endpoint, ep);
>  
> -	v4l2_fwnode_endpoint_parse(of_fwnode_handle(endpoint), ep);
> -	of_node_put(endpoint);
> +	fwnode_handle_put(endpoint);
>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

[PATCH v4 15/17] media: imx: Create missing links from CSI-2 receiver

[Steve Longerbeam]

The entities external to the i.MX6 IPU and i.MX7 now create the links
to their fwnode-endpoint connected entities in their notifier bound
callbacks. Which means imx_media_create_of_links() and
imx_media_create_csi_of_links() are no longer needed and are removed.

However there is still one case in which imx-media needs to create
fwnode-endpoint based links at probe completion. The v4l2-async framework
does not allow multiple subdevice notifiers to contain a duplicate
subdevice in their asd_list. Only the first subdev notifier that discovers
and adds that one subdevice to its asd_list will receive a bound callback
for it. Other subdevices that also have firmware endpoint connections to
this duplicate subdevice will not have it in their asd_list, and thus will
never receive a bound callback for it. In the case of imx-media, the one
duplicate subdevice in question is the i.MX6 MIPI CSI-2 receiver.

Until there is a solution to that problem, rewrite imx_media_create_links()
to add the missing links from the CSI-2 receiver to the CSIs and CSI muxes.
The function is renamed imx_media_create_csi2_links().

Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
Changes in v3:
- call a local imx-media utility imx_media_create_fwnode_pad_links().
Changes in v2:
- this is a rewrite of v1 "media: imx: Use media_create_fwnode_links for
  external links", which only adds the missing CSI-2 receiver links.
---
 .../staging/media/imx/imx-media-dev-common.c  |  46 +++----
 drivers/staging/media/imx/imx-media-of.c      | 114 ------------------
 drivers/staging/media/imx/imx-media.h         |   4 -
 3 files changed, 17 insertions(+), 147 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-dev-common.c b/drivers/staging/media/imx/imx-media-dev-common.c
index 66b505f7e8df..f7ad3cbbeec2 100644
--- a/drivers/staging/media/imx/imx-media-dev-common.c
+++ b/drivers/staging/media/imx/imx-media-dev-common.c
@@ -30,41 +30,31 @@ static int imx_media_subdev_bound(struct v4l2_async_notifier *notifier,
 }
 
 /*
- * Create the media links for all subdevs that registered.
+ * Create the missing media links from the CSI-2 receiver.
  * Called after all async subdevs have bound.
  */
-static int imx_media_create_links(struct v4l2_async_notifier *notifier)
+static void imx_media_create_csi2_links(struct imx_media_dev *imxmd)
 {
-	struct imx_media_dev *imxmd = notifier2dev(notifier);
-	struct v4l2_subdev *sd;
+	struct v4l2_subdev *sd, *csi2 = NULL;
 
 	list_for_each_entry(sd, &imxmd->v4l2_dev.subdevs, list) {
-		switch (sd->grp_id) {
-		case IMX_MEDIA_GRP_ID_IPU_VDIC:
-		case IMX_MEDIA_GRP_ID_IPU_IC_PRP:
-		case IMX_MEDIA_GRP_ID_IPU_IC_PRPENC:
-		case IMX_MEDIA_GRP_ID_IPU_IC_PRPVF:
-			/*
-			 * links have already been created for the
-			 * sync-registered subdevs.
-			 */
-			break;
-		case IMX_MEDIA_GRP_ID_IPU_CSI0:
-		case IMX_MEDIA_GRP_ID_IPU_CSI1:
-		case IMX_MEDIA_GRP_ID_CSI:
-			imx_media_create_csi_of_links(imxmd, sd);
-			break;
-		default:
-			/*
-			 * if this subdev has fwnode links, create media
-			 * links for them.
-			 */
-			imx_media_create_of_links(imxmd, sd);
+		if (sd->grp_id == IMX_MEDIA_GRP_ID_CSI2) {
+			csi2 = sd;
 			break;
 		}
 	}
+	if (!csi2)
+		return;
 
-	return 0;
+	list_for_each_entry(sd, &imxmd->v4l2_dev.subdevs, list) {
+		/* skip if not a CSI or a video mux */
+		if (!(sd->grp_id & IMX_MEDIA_GRP_ID_IPU_CSI) &&
+		    !(sd->grp_id & IMX_MEDIA_GRP_ID_CSI) &&
+		    sd->entity.function != MEDIA_ENT_F_VID_MUX)
+			continue;
+
+		imx_media_create_fwnode_pad_links(csi2, sd);
+	}
 }
 
 /*
@@ -196,9 +186,7 @@ int imx_media_probe_complete(struct v4l2_async_notifier *notifier)
 
 	mutex_lock(&imxmd->mutex);
 
-	ret = imx_media_create_links(notifier);
-	if (ret)
-		goto unlock;
+	imx_media_create_csi2_links(imxmd);
 
 	ret = imx_media_create_pad_vdev_lists(imxmd);
 	if (ret)
diff --git a/drivers/staging/media/imx/imx-media-of.c b/drivers/staging/media/imx/imx-media-of.c
index 2d3efd2a6dde..82e13e972e23 100644
--- a/drivers/staging/media/imx/imx-media-of.c
+++ b/drivers/staging/media/imx/imx-media-of.c
@@ -74,117 +74,3 @@ int imx_media_add_of_subdevs(struct imx_media_dev *imxmd,
 	return ret;
 }
 EXPORT_SYMBOL_GPL(imx_media_add_of_subdevs);
-
-/*
- * Create a single media link to/from sd using a fwnode link.
- *
- * NOTE: this function assumes an OF port node is equivalent to
- * a media pad (port id equal to media pad index), and that an
- * OF endpoint node is equivalent to a media link.
- */
-static int create_of_link(struct imx_media_dev *imxmd,
-			  struct v4l2_subdev *sd,
-			  struct v4l2_fwnode_link *link)
-{
-	struct v4l2_subdev *remote, *src, *sink;
-	int src_pad, sink_pad;
-
-	if (link->local_port >= sd->entity.num_pads)
-		return -EINVAL;
-
-	remote = imx_media_find_subdev_by_fwnode(imxmd, link->remote_node);
-	if (!remote)
-		return 0;
-
-	if (sd->entity.pads[link->local_port].flags & MEDIA_PAD_FL_SINK) {
-		src = remote;
-		src_pad = link->remote_port;
-		sink = sd;
-		sink_pad = link->local_port;
-	} else {
-		src = sd;
-		src_pad = link->local_port;
-		sink = remote;
-		sink_pad = link->remote_port;
-	}
-
-	/* make sure link doesn't already exist before creating */
-	if (media_entity_find_link(&src->entity.pads[src_pad],
-				   &sink->entity.pads[sink_pad]))
-		return 0;
-
-	v4l2_info(sd->v4l2_dev, "%s:%d -> %s:%d\n",
-		  src->name, src_pad, sink->name, sink_pad);
-
-	return media_create_pad_link(&src->entity, src_pad,
-				     &sink->entity, sink_pad, 0);
-}
-
-/*
- * Create media links to/from sd using its device-tree endpoints.
- */
-int imx_media_create_of_links(struct imx_media_dev *imxmd,
-			      struct v4l2_subdev *sd)
-{
-	struct v4l2_fwnode_link link;
-	struct device_node *ep;
-	int ret;
-
-	for_each_endpoint_of_node(sd->dev->of_node, ep) {
-		ret = v4l2_fwnode_parse_link(of_fwnode_handle(ep), &link);
-		if (ret)
-			continue;
-
-		ret = create_of_link(imxmd, sd, &link);
-		v4l2_fwnode_put_link(&link);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(imx_media_create_of_links);
-
-/*
- * Create media links to the given CSI subdevice's sink pads,
- * using its device-tree endpoints.
- */
-int imx_media_create_csi_of_links(struct imx_media_dev *imxmd,
-				  struct v4l2_subdev *csi)
-{
-	struct device_node *csi_np = csi->dev->of_node;
-	struct device_node *ep;
-
-	for_each_child_of_node(csi_np, ep) {
-		struct fwnode_handle *fwnode, *csi_ep;
-		struct v4l2_fwnode_link link;
-		int ret;
-
-		memset(&link, 0, sizeof(link));
-
-		link.local_node = of_fwnode_handle(csi_np);
-		link.local_port = CSI_SINK_PAD;
-
-		csi_ep = of_fwnode_handle(ep);
-
-		fwnode = fwnode_graph_get_remote_endpoint(csi_ep);
-		if (!fwnode)
-			continue;
-
-		fwnode = fwnode_get_parent(fwnode);
-		fwnode_property_read_u32(fwnode, "reg", &link.remote_port);
-		fwnode = fwnode_get_next_parent(fwnode);
-		if (is_of_node(fwnode) &&
-		    of_node_name_eq(to_of_node(fwnode), "ports"))
-			fwnode = fwnode_get_next_parent(fwnode);
-		link.remote_node = fwnode;
-
-		ret = create_of_link(imxmd, csi, &link);
-		fwnode_handle_put(link.remote_node);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(imx_media_create_csi_of_links);
diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
index 5f23d852122f..5271b84bea9a 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -248,10 +248,6 @@ void imx_media_unregister_ipu_internal_subdevs(struct imx_media_dev *imxmd);
 /* imx-media-of.c */
 int imx_media_add_of_subdevs(struct imx_media_dev *dev,
 			     struct device_node *np);
-int imx_media_create_of_links(struct imx_media_dev *imxmd,
-			      struct v4l2_subdev *sd);
-int imx_media_create_csi_of_links(struct imx_media_dev *imxmd,
-				  struct v4l2_subdev *csi);
 int imx_media_of_add_csi(struct imx_media_dev *imxmd,
 			 struct device_node *csi_np);
 
-- 
2.17.1

Re: [PATCH v4 15/17] media: imx: Create missing links from CSI-2 receiver

[Laurent Pinchart]

Hi Steve,

Thank you for the patch.

On Tue, Mar 03, 2020 at 03:42:54PM -0800, Steve Longerbeam wrote:
> The entities external to the i.MX6 IPU and i.MX7 now create the links
> to their fwnode-endpoint connected entities in their notifier bound
> callbacks. Which means imx_media_create_of_links() and
> imx_media_create_csi_of_links() are no longer needed and are removed.
> 
> However there is still one case in which imx-media needs to create
> fwnode-endpoint based links at probe completion. The v4l2-async framework
> does not allow multiple subdevice notifiers to contain a duplicate
> subdevice in their asd_list. Only the first subdev notifier that discovers
> and adds that one subdevice to its asd_list will receive a bound callback
> for it. Other subdevices that also have firmware endpoint connections to
> this duplicate subdevice will not have it in their asd_list, and thus will
> never receive a bound callback for it. In the case of imx-media, the one
> duplicate subdevice in question is the i.MX6 MIPI CSI-2 receiver.
> 
> Until there is a solution to that problem, rewrite imx_media_create_links()
> to add the missing links from the CSI-2 receiver to the CSIs and CSI muxes.
> The function is renamed imx_media_create_csi2_links().
> 
> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> ---
> Changes in v3:
> - call a local imx-media utility imx_media_create_fwnode_pad_links().
> Changes in v2:
> - this is a rewrite of v1 "media: imx: Use media_create_fwnode_links for
>   external links", which only adds the missing CSI-2 receiver links.
> ---
>  .../staging/media/imx/imx-media-dev-common.c  |  46 +++----
>  drivers/staging/media/imx/imx-media-of.c      | 114 ------------------
>  drivers/staging/media/imx/imx-media.h         |   4 -
>  3 files changed, 17 insertions(+), 147 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx-media-dev-common.c b/drivers/staging/media/imx/imx-media-dev-common.c
> index 66b505f7e8df..f7ad3cbbeec2 100644
> --- a/drivers/staging/media/imx/imx-media-dev-common.c
> +++ b/drivers/staging/media/imx/imx-media-dev-common.c
> @@ -30,41 +30,31 @@ static int imx_media_subdev_bound(struct v4l2_async_notifier *notifier,
>  }
>  
>  /*
> - * Create the media links for all subdevs that registered.
> + * Create the missing media links from the CSI-2 receiver.
>   * Called after all async subdevs have bound.
>   */
> -static int imx_media_create_links(struct v4l2_async_notifier *notifier)
> +static void imx_media_create_csi2_links(struct imx_media_dev *imxmd)
>  {
> -	struct imx_media_dev *imxmd = notifier2dev(notifier);
> -	struct v4l2_subdev *sd;
> +	struct v4l2_subdev *sd, *csi2 = NULL;
>  
>  	list_for_each_entry(sd, &imxmd->v4l2_dev.subdevs, list) {
> -		switch (sd->grp_id) {
> -		case IMX_MEDIA_GRP_ID_IPU_VDIC:
> -		case IMX_MEDIA_GRP_ID_IPU_IC_PRP:
> -		case IMX_MEDIA_GRP_ID_IPU_IC_PRPENC:
> -		case IMX_MEDIA_GRP_ID_IPU_IC_PRPVF:
> -			/*
> -			 * links have already been created for the
> -			 * sync-registered subdevs.
> -			 */
> -			break;
> -		case IMX_MEDIA_GRP_ID_IPU_CSI0:
> -		case IMX_MEDIA_GRP_ID_IPU_CSI1:
> -		case IMX_MEDIA_GRP_ID_CSI:
> -			imx_media_create_csi_of_links(imxmd, sd);
> -			break;
> -		default:
> -			/*
> -			 * if this subdev has fwnode links, create media
> -			 * links for them.
> -			 */
> -			imx_media_create_of_links(imxmd, sd);
> +		if (sd->grp_id == IMX_MEDIA_GRP_ID_CSI2) {
> +			csi2 = sd;
>  			break;
>  		}
>  	}
> +	if (!csi2)
> +		return;
>  
> -	return 0;
> +	list_for_each_entry(sd, &imxmd->v4l2_dev.subdevs, list) {
> +		/* skip if not a CSI or a video mux */
> +		if (!(sd->grp_id & IMX_MEDIA_GRP_ID_IPU_CSI) &&
> +		    !(sd->grp_id & IMX_MEDIA_GRP_ID_CSI) &&
> +		    sd->entity.function != MEDIA_ENT_F_VID_MUX)

This is a bit dangerous, as the external source connected to the CSI-2
receiver could be a video mux. How about assigning a group id to the
internal mux, as done in the "[PATCH 2/2] media: staging/imx: Don't
create links between external entities" patch that I've posted, and
checking the group id here ?

With that fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +			continue;
> +
> +		imx_media_create_fwnode_pad_links(csi2, sd);
> +	}
>  }
>  
>  /*
> @@ -196,9 +186,7 @@ int imx_media_probe_complete(struct v4l2_async_notifier *notifier)
>  
>  	mutex_lock(&imxmd->mutex);
>  
> -	ret = imx_media_create_links(notifier);
> -	if (ret)
> -		goto unlock;
> +	imx_media_create_csi2_links(imxmd);
>  
>  	ret = imx_media_create_pad_vdev_lists(imxmd);
>  	if (ret)
> diff --git a/drivers/staging/media/imx/imx-media-of.c b/drivers/staging/media/imx/imx-media-of.c
> index 2d3efd2a6dde..82e13e972e23 100644
> --- a/drivers/staging/media/imx/imx-media-of.c
> +++ b/drivers/staging/media/imx/imx-media-of.c
> @@ -74,117 +74,3 @@ int imx_media_add_of_subdevs(struct imx_media_dev *imxmd,
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(imx_media_add_of_subdevs);
> -
> -/*
> - * Create a single media link to/from sd using a fwnode link.
> - *
> - * NOTE: this function assumes an OF port node is equivalent to
> - * a media pad (port id equal to media pad index), and that an
> - * OF endpoint node is equivalent to a media link.
> - */
> -static int create_of_link(struct imx_media_dev *imxmd,
> -			  struct v4l2_subdev *sd,
> -			  struct v4l2_fwnode_link *link)
> -{
> -	struct v4l2_subdev *remote, *src, *sink;
> -	int src_pad, sink_pad;
> -
> -	if (link->local_port >= sd->entity.num_pads)
> -		return -EINVAL;
> -
> -	remote = imx_media_find_subdev_by_fwnode(imxmd, link->remote_node);
> -	if (!remote)
> -		return 0;
> -
> -	if (sd->entity.pads[link->local_port].flags & MEDIA_PAD_FL_SINK) {
> -		src = remote;
> -		src_pad = link->remote_port;
> -		sink = sd;
> -		sink_pad = link->local_port;
> -	} else {
> -		src = sd;
> -		src_pad = link->local_port;
> -		sink = remote;
> -		sink_pad = link->remote_port;
> -	}
> -
> -	/* make sure link doesn't already exist before creating */
> -	if (media_entity_find_link(&src->entity.pads[src_pad],
> -				   &sink->entity.pads[sink_pad]))
> -		return 0;
> -
> -	v4l2_info(sd->v4l2_dev, "%s:%d -> %s:%d\n",
> -		  src->name, src_pad, sink->name, sink_pad);
> -
> -	return media_create_pad_link(&src->entity, src_pad,
> -				     &sink->entity, sink_pad, 0);
> -}
> -
> -/*
> - * Create media links to/from sd using its device-tree endpoints.
> - */
> -int imx_media_create_of_links(struct imx_media_dev *imxmd,
> -			      struct v4l2_subdev *sd)
> -{
> -	struct v4l2_fwnode_link link;
> -	struct device_node *ep;
> -	int ret;
> -
> -	for_each_endpoint_of_node(sd->dev->of_node, ep) {
> -		ret = v4l2_fwnode_parse_link(of_fwnode_handle(ep), &link);
> -		if (ret)
> -			continue;
> -
> -		ret = create_of_link(imxmd, sd, &link);
> -		v4l2_fwnode_put_link(&link);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(imx_media_create_of_links);
> -
> -/*
> - * Create media links to the given CSI subdevice's sink pads,
> - * using its device-tree endpoints.
> - */
> -int imx_media_create_csi_of_links(struct imx_media_dev *imxmd,
> -				  struct v4l2_subdev *csi)
> -{
> -	struct device_node *csi_np = csi->dev->of_node;
> -	struct device_node *ep;
> -
> -	for_each_child_of_node(csi_np, ep) {
> -		struct fwnode_handle *fwnode, *csi_ep;
> -		struct v4l2_fwnode_link link;
> -		int ret;
> -
> -		memset(&link, 0, sizeof(link));
> -
> -		link.local_node = of_fwnode_handle(csi_np);
> -		link.local_port = CSI_SINK_PAD;
> -
> -		csi_ep = of_fwnode_handle(ep);
> -
> -		fwnode = fwnode_graph_get_remote_endpoint(csi_ep);
> -		if (!fwnode)
> -			continue;
> -
> -		fwnode = fwnode_get_parent(fwnode);
> -		fwnode_property_read_u32(fwnode, "reg", &link.remote_port);
> -		fwnode = fwnode_get_next_parent(fwnode);
> -		if (is_of_node(fwnode) &&
> -		    of_node_name_eq(to_of_node(fwnode), "ports"))
> -			fwnode = fwnode_get_next_parent(fwnode);
> -		link.remote_node = fwnode;
> -
> -		ret = create_of_link(imxmd, csi, &link);
> -		fwnode_handle_put(link.remote_node);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(imx_media_create_csi_of_links);
> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> index 5f23d852122f..5271b84bea9a 100644
> --- a/drivers/staging/media/imx/imx-media.h
> +++ b/drivers/staging/media/imx/imx-media.h
> @@ -248,10 +248,6 @@ void imx_media_unregister_ipu_internal_subdevs(struct imx_media_dev *imxmd);
>  /* imx-media-of.c */
>  int imx_media_add_of_subdevs(struct imx_media_dev *dev,
>  			     struct device_node *np);
> -int imx_media_create_of_links(struct imx_media_dev *imxmd,
> -			      struct v4l2_subdev *sd);
> -int imx_media_create_csi_of_links(struct imx_media_dev *imxmd,
> -				  struct v4l2_subdev *csi);
>  int imx_media_of_add_csi(struct imx_media_dev *imxmd,
>  			 struct device_node *csi_np);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 15/17] media: imx: Create missing links from CSI-2 receiver

[Steve Longerbeam]

On 4/14/20 4:32 PM, Laurent Pinchart wrote:
> Hi Steve,
>
> Thank you for the patch.
>
> On Tue, Mar 03, 2020 at 03:42:54PM -0800, Steve Longerbeam wrote:
>> The entities external to the i.MX6 IPU and i.MX7 now create the links
>> to their fwnode-endpoint connected entities in their notifier bound
>> callbacks. Which means imx_media_create_of_links() and
>> imx_media_create_csi_of_links() are no longer needed and are removed.
>>
>> However there is still one case in which imx-media needs to create
>> fwnode-endpoint based links at probe completion. The v4l2-async framework
>> does not allow multiple subdevice notifiers to contain a duplicate
>> subdevice in their asd_list. Only the first subdev notifier that discovers
>> and adds that one subdevice to its asd_list will receive a bound callback
>> for it. Other subdevices that also have firmware endpoint connections to
>> this duplicate subdevice will not have it in their asd_list, and thus will
>> never receive a bound callback for it. In the case of imx-media, the one
>> duplicate subdevice in question is the i.MX6 MIPI CSI-2 receiver.
>>
>> Until there is a solution to that problem, rewrite imx_media_create_links()
>> to add the missing links from the CSI-2 receiver to the CSIs and CSI muxes.
>> The function is renamed imx_media_create_csi2_links().
>>
>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
>> ---
>> Changes in v3:
>> - call a local imx-media utility imx_media_create_fwnode_pad_links().
>> Changes in v2:
>> - this is a rewrite of v1 "media: imx: Use media_create_fwnode_links for
>>    external links", which only adds the missing CSI-2 receiver links.
>> ---
>>   .../staging/media/imx/imx-media-dev-common.c  |  46 +++----
>>   drivers/staging/media/imx/imx-media-of.c      | 114 ------------------
>>   drivers/staging/media/imx/imx-media.h         |   4 -
>>   3 files changed, 17 insertions(+), 147 deletions(-)
>>
>> diff --git a/drivers/staging/media/imx/imx-media-dev-common.c b/drivers/staging/media/imx/imx-media-dev-common.c
>> index 66b505f7e8df..f7ad3cbbeec2 100644
>> --- a/drivers/staging/media/imx/imx-media-dev-common.c
>> +++ b/drivers/staging/media/imx/imx-media-dev-common.c
>> @@ -30,41 +30,31 @@ static int imx_media_subdev_bound(struct v4l2_async_notifier *notifier,
>>   }
>>   
>>   /*
>> - * Create the media links for all subdevs that registered.
>> + * Create the missing media links from the CSI-2 receiver.
>>    * Called after all async subdevs have bound.
>>    */
>> -static int imx_media_create_links(struct v4l2_async_notifier *notifier)
>> +static void imx_media_create_csi2_links(struct imx_media_dev *imxmd)
>>   {
>> -	struct imx_media_dev *imxmd = notifier2dev(notifier);
>> -	struct v4l2_subdev *sd;
>> +	struct v4l2_subdev *sd, *csi2 = NULL;
>>   
>>   	list_for_each_entry(sd, &imxmd->v4l2_dev.subdevs, list) {
>> -		switch (sd->grp_id) {
>> -		case IMX_MEDIA_GRP_ID_IPU_VDIC:
>> -		case IMX_MEDIA_GRP_ID_IPU_IC_PRP:
>> -		case IMX_MEDIA_GRP_ID_IPU_IC_PRPENC:
>> -		case IMX_MEDIA_GRP_ID_IPU_IC_PRPVF:
>> -			/*
>> -			 * links have already been created for the
>> -			 * sync-registered subdevs.
>> -			 */
>> -			break;
>> -		case IMX_MEDIA_GRP_ID_IPU_CSI0:
>> -		case IMX_MEDIA_GRP_ID_IPU_CSI1:
>> -		case IMX_MEDIA_GRP_ID_CSI:
>> -			imx_media_create_csi_of_links(imxmd, sd);
>> -			break;
>> -		default:
>> -			/*
>> -			 * if this subdev has fwnode links, create media
>> -			 * links for them.
>> -			 */
>> -			imx_media_create_of_links(imxmd, sd);
>> +		if (sd->grp_id == IMX_MEDIA_GRP_ID_CSI2) {
>> +			csi2 = sd;
>>   			break;
>>   		}
>>   	}
>> +	if (!csi2)
>> +		return;
>>   
>> -	return 0;
>> +	list_for_each_entry(sd, &imxmd->v4l2_dev.subdevs, list) {
>> +		/* skip if not a CSI or a video mux */
>> +		if (!(sd->grp_id & IMX_MEDIA_GRP_ID_IPU_CSI) &&
>> +		    !(sd->grp_id & IMX_MEDIA_GRP_ID_CSI) &&
>> +		    sd->entity.function != MEDIA_ENT_F_VID_MUX)
> This is a bit dangerous, as the external source connected to the CSI-2
> receiver could be a video mux.

Yes I agree it looks dangerous on the face of it, but if there is a 
video-mux as a source to the csi-2 receiver, the video-mux's 
.get_fwnode_pad() verifies that the passed fwnode endpoint is actually 
owned by the video-mux, and imx_media_create_fwnode_pad_link() is only 
looking for a sink pad... Well, maybe that wasn't clear but if you 
follow the code you'll see that a weird link won't be created in that case.

But I agree, to make this at least look less dangerous, I'll do what you 
have done in "[PATCH 2/2] media: staging/imx: Don't create links between 
external entities" and mark the video-mux in the ipu-csi bound callbacks 
as one of the imx6 internal video muxes via a imx grp_id.

Steve

>   How about assigning a group id to the
> internal mux, as done in the "[PATCH 2/2] media: staging/imx: Don't
> create links between external entities" patch that I've posted, and
> checking the group id here ?
>
> With that fixed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
>> +			continue;
>> +
>> +		imx_media_create_fwnode_pad_links(csi2, sd);
>> +	}
>>   }
>>   
>>   /*
>> @@ -196,9 +186,7 @@ int imx_media_probe_complete(struct v4l2_async_notifier *notifier)
>>   
>>   	mutex_lock(&imxmd->mutex);
>>   
>> -	ret = imx_media_create_links(notifier);
>> -	if (ret)
>> -		goto unlock;
>> +	imx_media_create_csi2_links(imxmd);
>>   
>>   	ret = imx_media_create_pad_vdev_lists(imxmd);
>>   	if (ret)
>> diff --git a/drivers/staging/media/imx/imx-media-of.c b/drivers/staging/media/imx/imx-media-of.c
>> index 2d3efd2a6dde..82e13e972e23 100644
>> --- a/drivers/staging/media/imx/imx-media-of.c
>> +++ b/drivers/staging/media/imx/imx-media-of.c
>> @@ -74,117 +74,3 @@ int imx_media_add_of_subdevs(struct imx_media_dev *imxmd,
>>   	return ret;
>>   }
>>   EXPORT_SYMBOL_GPL(imx_media_add_of_subdevs);
>> -
>> -/*
>> - * Create a single media link to/from sd using a fwnode link.
>> - *
>> - * NOTE: this function assumes an OF port node is equivalent to
>> - * a media pad (port id equal to media pad index), and that an
>> - * OF endpoint node is equivalent to a media link.
>> - */
>> -static int create_of_link(struct imx_media_dev *imxmd,
>> -			  struct v4l2_subdev *sd,
>> -			  struct v4l2_fwnode_link *link)
>> -{
>> -	struct v4l2_subdev *remote, *src, *sink;
>> -	int src_pad, sink_pad;
>> -
>> -	if (link->local_port >= sd->entity.num_pads)
>> -		return -EINVAL;
>> -
>> -	remote = imx_media_find_subdev_by_fwnode(imxmd, link->remote_node);
>> -	if (!remote)
>> -		return 0;
>> -
>> -	if (sd->entity.pads[link->local_port].flags & MEDIA_PAD_FL_SINK) {
>> -		src = remote;
>> -		src_pad = link->remote_port;
>> -		sink = sd;
>> -		sink_pad = link->local_port;
>> -	} else {
>> -		src = sd;
>> -		src_pad = link->local_port;
>> -		sink = remote;
>> -		sink_pad = link->remote_port;
>> -	}
>> -
>> -	/* make sure link doesn't already exist before creating */
>> -	if (media_entity_find_link(&src->entity.pads[src_pad],
>> -				   &sink->entity.pads[sink_pad]))
>> -		return 0;
>> -
>> -	v4l2_info(sd->v4l2_dev, "%s:%d -> %s:%d\n",
>> -		  src->name, src_pad, sink->name, sink_pad);
>> -
>> -	return media_create_pad_link(&src->entity, src_pad,
>> -				     &sink->entity, sink_pad, 0);
>> -}
>> -
>> -/*
>> - * Create media links to/from sd using its device-tree endpoints.
>> - */
>> -int imx_media_create_of_links(struct imx_media_dev *imxmd,
>> -			      struct v4l2_subdev *sd)
>> -{
>> -	struct v4l2_fwnode_link link;
>> -	struct device_node *ep;
>> -	int ret;
>> -
>> -	for_each_endpoint_of_node(sd->dev->of_node, ep) {
>> -		ret = v4l2_fwnode_parse_link(of_fwnode_handle(ep), &link);
>> -		if (ret)
>> -			continue;
>> -
>> -		ret = create_of_link(imxmd, sd, &link);
>> -		v4l2_fwnode_put_link(&link);
>> -		if (ret)
>> -			return ret;
>> -	}
>> -
>> -	return 0;
>> -}
>> -EXPORT_SYMBOL_GPL(imx_media_create_of_links);
>> -
>> -/*
>> - * Create media links to the given CSI subdevice's sink pads,
>> - * using its device-tree endpoints.
>> - */
>> -int imx_media_create_csi_of_links(struct imx_media_dev *imxmd,
>> -				  struct v4l2_subdev *csi)
>> -{
>> -	struct device_node *csi_np = csi->dev->of_node;
>> -	struct device_node *ep;
>> -
>> -	for_each_child_of_node(csi_np, ep) {
>> -		struct fwnode_handle *fwnode, *csi_ep;
>> -		struct v4l2_fwnode_link link;
>> -		int ret;
>> -
>> -		memset(&link, 0, sizeof(link));
>> -
>> -		link.local_node = of_fwnode_handle(csi_np);
>> -		link.local_port = CSI_SINK_PAD;
>> -
>> -		csi_ep = of_fwnode_handle(ep);
>> -
>> -		fwnode = fwnode_graph_get_remote_endpoint(csi_ep);
>> -		if (!fwnode)
>> -			continue;
>> -
>> -		fwnode = fwnode_get_parent(fwnode);
>> -		fwnode_property_read_u32(fwnode, "reg", &link.remote_port);
>> -		fwnode = fwnode_get_next_parent(fwnode);
>> -		if (is_of_node(fwnode) &&
>> -		    of_node_name_eq(to_of_node(fwnode), "ports"))
>> -			fwnode = fwnode_get_next_parent(fwnode);
>> -		link.remote_node = fwnode;
>> -
>> -		ret = create_of_link(imxmd, csi, &link);
>> -		fwnode_handle_put(link.remote_node);
>> -		if (ret)
>> -			return ret;
>> -	}
>> -
>> -	return 0;
>> -}
>> -EXPORT_SYMBOL_GPL(imx_media_create_csi_of_links);
>> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
>> index 5f23d852122f..5271b84bea9a 100644
>> --- a/drivers/staging/media/imx/imx-media.h
>> +++ b/drivers/staging/media/imx/imx-media.h
>> @@ -248,10 +248,6 @@ void imx_media_unregister_ipu_internal_subdevs(struct imx_media_dev *imxmd);
>>   /* imx-media-of.c */
>>   int imx_media_add_of_subdevs(struct imx_media_dev *dev,
>>   			     struct device_node *np);
>> -int imx_media_create_of_links(struct imx_media_dev *imxmd,
>> -			      struct v4l2_subdev *sd);
>> -int imx_media_create_csi_of_links(struct imx_media_dev *imxmd,
>> -				  struct v4l2_subdev *csi);
>>   int imx_media_of_add_csi(struct imx_media_dev *imxmd,
>>   			 struct device_node *csi_np);
>>   

[PATCH v4 16/17] media: imx: silence a couple debug messages

[Steve Longerbeam]

Convert to dev_dbg the "subdev bound" and IPU-internal media-link
creation messages.

Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
 drivers/staging/media/imx/imx-media-dev-common.c  | 4 +++-
 drivers/staging/media/imx/imx-media-dev.c         | 2 +-
 drivers/staging/media/imx/imx-media-internal-sd.c | 6 +++---
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-dev-common.c b/drivers/staging/media/imx/imx-media-dev-common.c
index f7ad3cbbeec2..b23bbfab388a 100644
--- a/drivers/staging/media/imx/imx-media-dev-common.c
+++ b/drivers/staging/media/imx/imx-media-dev-common.c
@@ -24,7 +24,9 @@ static int imx_media_subdev_bound(struct v4l2_async_notifier *notifier,
 				  struct v4l2_subdev *sd,
 				  struct v4l2_async_subdev *asd)
 {
-	v4l2_info(sd->v4l2_dev, "subdev %s bound\n", sd->name);
+	struct imx_media_dev *imxmd = notifier2dev(notifier);
+
+	dev_dbg(imxmd->md.dev, "subdev %s bound\n", sd->name);
 
 	return 0;
 }
diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
index 2c3c2adca683..6d2205461e56 100644
--- a/drivers/staging/media/imx/imx-media-dev.c
+++ b/drivers/staging/media/imx/imx-media-dev.c
@@ -32,7 +32,7 @@ static int imx_media_subdev_bound(struct v4l2_async_notifier *notifier,
 			return ret;
 	}
 
-	v4l2_info(&imxmd->v4l2_dev, "subdev %s bound\n", sd->name);
+	dev_dbg(imxmd->md.dev, "subdev %s bound\n", sd->name);
 
 	return 0;
 }
diff --git a/drivers/staging/media/imx/imx-media-internal-sd.c b/drivers/staging/media/imx/imx-media-internal-sd.c
index d4237e1a4241..da4109b2fd13 100644
--- a/drivers/staging/media/imx/imx-media-internal-sd.c
+++ b/drivers/staging/media/imx/imx-media-internal-sd.c
@@ -142,9 +142,9 @@ static int create_internal_link(struct imx_media_dev *imxmd,
 				   &sink->entity.pads[link->remote_pad]))
 		return 0;
 
-	v4l2_info(&imxmd->v4l2_dev, "%s:%d -> %s:%d\n",
-		  src->name, link->local_pad,
-		  sink->name, link->remote_pad);
+	dev_dbg(imxmd->md.dev, "%s:%d -> %s:%d\n",
+		src->name, link->local_pad,
+		sink->name, link->remote_pad);
 
 	ret = media_create_pad_link(&src->entity, link->local_pad,
 				    &sink->entity, link->remote_pad, 0);
-- 
2.17.1

Re: [PATCH v4 16/17] media: imx: silence a couple debug messages

[Laurent Pinchart]

Hi Steve,

Thank you for the patch.

On Tue, Mar 03, 2020 at 03:42:55PM -0800, Steve Longerbeam wrote:
> Convert to dev_dbg the "subdev bound" and IPU-internal media-link
> creation messages.
> 
> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/staging/media/imx/imx-media-dev-common.c  | 4 +++-
>  drivers/staging/media/imx/imx-media-dev.c         | 2 +-
>  drivers/staging/media/imx/imx-media-internal-sd.c | 6 +++---
>  3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx-media-dev-common.c b/drivers/staging/media/imx/imx-media-dev-common.c
> index f7ad3cbbeec2..b23bbfab388a 100644
> --- a/drivers/staging/media/imx/imx-media-dev-common.c
> +++ b/drivers/staging/media/imx/imx-media-dev-common.c
> @@ -24,7 +24,9 @@ static int imx_media_subdev_bound(struct v4l2_async_notifier *notifier,
>  				  struct v4l2_subdev *sd,
>  				  struct v4l2_async_subdev *asd)
>  {
> -	v4l2_info(sd->v4l2_dev, "subdev %s bound\n", sd->name);
> +	struct imx_media_dev *imxmd = notifier2dev(notifier);
> +
> +	dev_dbg(imxmd->md.dev, "subdev %s bound\n", sd->name);
>  
>  	return 0;
>  }
> diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
> index 2c3c2adca683..6d2205461e56 100644
> --- a/drivers/staging/media/imx/imx-media-dev.c
> +++ b/drivers/staging/media/imx/imx-media-dev.c
> @@ -32,7 +32,7 @@ static int imx_media_subdev_bound(struct v4l2_async_notifier *notifier,
>  			return ret;
>  	}
>  
> -	v4l2_info(&imxmd->v4l2_dev, "subdev %s bound\n", sd->name);
> +	dev_dbg(imxmd->md.dev, "subdev %s bound\n", sd->name);
>  
>  	return 0;
>  }
> diff --git a/drivers/staging/media/imx/imx-media-internal-sd.c b/drivers/staging/media/imx/imx-media-internal-sd.c
> index d4237e1a4241..da4109b2fd13 100644
> --- a/drivers/staging/media/imx/imx-media-internal-sd.c
> +++ b/drivers/staging/media/imx/imx-media-internal-sd.c
> @@ -142,9 +142,9 @@ static int create_internal_link(struct imx_media_dev *imxmd,
>  				   &sink->entity.pads[link->remote_pad]))
>  		return 0;
>  
> -	v4l2_info(&imxmd->v4l2_dev, "%s:%d -> %s:%d\n",
> -		  src->name, link->local_pad,
> -		  sink->name, link->remote_pad);
> +	dev_dbg(imxmd->md.dev, "%s:%d -> %s:%d\n",
> +		src->name, link->local_pad,
> +		sink->name, link->remote_pad);
>  
>  	ret = media_create_pad_link(&src->entity, link->local_pad,
>  				    &sink->entity, link->remote_pad, 0);

-- 
Regards,

Laurent Pinchart

[PATCH v4 17/17] media: imx: TODO: Remove media link creation todos

[Steve Longerbeam]

Remove the TODO items regarding media link creation, these issues are
resolved by moving media link creation to individual entity bound
callbacks and the implementation of the get_fwnode_pad operation.

Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
 drivers/staging/media/imx/TODO | 29 -----------------------------
 1 file changed, 29 deletions(-)

diff --git a/drivers/staging/media/imx/TODO b/drivers/staging/media/imx/TODO
index 6f29b5ca5324..a371cdedcdb0 100644
--- a/drivers/staging/media/imx/TODO
+++ b/drivers/staging/media/imx/TODO
@@ -17,35 +17,6 @@
   decided whether this feature is useful enough to make it generally
   available by exporting to v4l2-core.
 
-- After all async subdevices have been bound, v4l2_fwnode_parse_link()
-  is used to form the media links between the devices discovered in
-  the OF graph.
-
-  While this approach allows support for arbitrary OF graphs, there
-  are some assumptions for this to work:
-
-  1. If a port owned by a device in the graph has endpoint nodes, the
-     port is treated as a media pad.
-
-     This presents problems for devices that don't make this port = pad
-     assumption. Examples are SMIAPP compatible cameras which define only
-     a single output port node, but which define multiple pads owned
-     by multiple subdevices (pixel-array, binner, scaler). Or video
-     decoders (entity function MEDIA_ENT_F_ATV_DECODER), which also define
-     only a single output port node, but define multiple pads for video,
-     VBI, and audio out.
-
-     A workaround at present is to set the port reg properties to
-     correspond to the media pad index that the port represents. A
-     possible long-term solution is to implement a subdev API that
-     maps a port id to a media pad index.
-
-  2. Every endpoint of a port owned by a device in the graph is treated
-     as a media link.
-
-     Which means a port must not contain mixed-use endpoints, they
-     must all refer to media links between V4L2 subdevices.
-
 - i.MX7: all of the above, since it uses the imx media core
 
 - i.MX7: use Frame Interval Monitor
-- 
2.17.1

Re: [PATCH v4 00/17] media: imx: Create media links in bound notifiers

[Steve Longerbeam]

Hi Sakari,

When you have time can you review this series.

TIA!

Steve

On 3/3/20 3:42 PM, Steve Longerbeam wrote:
> Move media link creation into the notifier bound callbacks in the
> minimum set of subdevices required by imx (imx5/6/7 CSI,
> imx6/7 MIPI CSI-2, and video mux).
>
> History:
>
> v4:
> - Removed the endpoint parsing callback APIs from video-mux and imx drivers
>    as suggested by Sakari, replacing with endpoint parsing local to the
>    drivers and the use of v4l2_async_notifier_add_fwnode_remote_subdev().
>    As a result convenience function v4l2_async_register_fwnode_subdev()
>    is no longer used and is reverted.
>
> v3:
> - The changes to the default behaviour in media_entity_get_fwnode_pad(),
>    and the fixes to current media drivers that call it inconsistently, have
>    been put-off to another time. Instead this version implements the
>    get_fwnode_pad operation where required in the imx and video-mux
>    subdevices to make media link creation work correctly. The
>    improvements to media_entity_get_fwnode_pad() can wait to another
>    patch series.
>
> v2:
> - rename/move the notifier-to-state inlines in imx7-mipi-csis.c and
>    imx7-media-csi.c, suggested by Rui Silva.
> - rewrite imx_media_create_links() to only add the missing media links
>    from the imx6 MIPI CSI-2 receiver.
>
>
> Steve Longerbeam (17):
>    media: entity: Pass entity to get_fwnode_pad operation
>    media: video-mux: Parse information from firmware without using
>      callbacks
>    media: imx: Parse information from firmware without using callbacks
>    Revert "media: v4l2-fwnode: Add a convenience function for registering
>      subdevs with notifiers"
>    media: imx: csi: Implement get_fwnode_pad op
>    media: imx: mipi csi-2: Implement get_fwnode_pad op
>    media: video-mux: Implement get_fwnode_pad op
>    media: imx: Add imx_media_create_fwnode_pad_link()
>    media: video-mux: Create media links in bound notifier
>    media: imx: mipi csi-2: Create media links in bound notifier
>    media: imx7: mipi csis: Create media links in bound notifier
>    media: imx7: csi: Create media links in bound notifier
>    media: imx: csi: Create media links in bound notifier
>    media: imx: csi: Lookup upstream endpoint with
>      imx_media_get_pad_fwnode
>    media: imx: Create missing links from CSI-2 receiver
>    media: imx: silence a couple debug messages
>    media: imx: TODO: Remove media link creation todos
>
>   drivers/media/mc/mc-entity.c                  |   2 +-
>   drivers/media/platform/video-mux.c            | 185 ++++++++++++++++--
>   drivers/media/v4l2-core/v4l2-fwnode.c         |  62 ------
>   drivers/staging/media/imx/TODO                |  29 ---
>   drivers/staging/media/imx/imx-media-csi.c     | 136 ++++++++-----
>   .../staging/media/imx/imx-media-dev-common.c  |  50 ++---
>   drivers/staging/media/imx/imx-media-dev.c     |   2 +-
>   .../staging/media/imx/imx-media-internal-sd.c |   6 +-
>   drivers/staging/media/imx/imx-media-of.c      | 114 -----------
>   drivers/staging/media/imx/imx-media-utils.c   | 124 ++++++++++++
>   drivers/staging/media/imx/imx-media.h         |   9 +-
>   drivers/staging/media/imx/imx6-mipi-csi2.c    | 119 +++++++++--
>   drivers/staging/media/imx/imx7-media-csi.c    | 100 +++++++---
>   drivers/staging/media/imx/imx7-mipi-csis.c    | 105 +++++++---
>   include/media/media-entity.h                  |   3 +-
>   include/media/v4l2-fwnode.h                   |  38 ----
>   16 files changed, 654 insertions(+), 430 deletions(-)
>