[PATCH 00/11] rcar-vin: Add r8a779a0 support

[PATCH 00/11] rcar-vin: Add r8a779a0 support

[Niklas Söderlund]

Hello,

This series adds support for V3U (r8a779a0) to the R-Car VIN driver. The 
V3U SoC is different from other Renesas SoCs as a new IP block (the ISP 
channel selector) is added between the CSI-2 receiver and the VIN 
modules. This new ISP IP deals with CSI-2 channel filtering based on 
VC/DT which in turn makes the VIN drivers much simpler with regards to 
the media graph. But it also means the rcar-vin driver needs to support 
and generate both the generic Gen3 MC-graph and the specific V3U 
MC-graph.

The rcar-vin driver intertwines the VIN group concept and the usage of 
the media graph into a single implementation. This needs to be broken 
apart before the new V3U support can be added. The first 01/11 - 10/11 
patches deals with this separation and its fallout. Fortunately patch 
11/11 after all that preparation work is quiet simple and straight 
forward when adding the V3U support.

There is a large patch (10/11) in the series, reviewers please fear not 
it only moves blocks of code around verbatim.

While working on this series it have become even more apparent to me 
that the VIN group concept probably should be replaced with something 
like the Media Device Allocator API once it learns how to work with DT.  
This series separation of the two VIN concepts is a good first step for 
me to hope to find time to dig into that.

Updates to the DT bindings have been posted in a separate patch.

Niklas Söderlund (11):
  rcar-vin: Refactor controls creation for video device
  rcar-vin: Fix error paths for rvin_mc_init()
  rcar-vin: Improve async notifier cleanup paths
  rcar-vin: Improve reuse of parallel notifier
  rcar-vin: Rename array storing subdevice information
  rcar-vin: Move group async notifier
  rcar-vin: Extend group notifier DT parser to work with any port
  rcar-vin: Create a callback to setup media links
  rcar-vin: Specify media device ops at group creation time
  rcar-vin: Move and rename CSI-2 link notifications
  rcar-vin: Add r8a779a0 support

 drivers/media/platform/rcar-vin/rcar-core.c | 973 +++++++++++---------
 drivers/media/platform/rcar-vin/rcar-dma.c  |  20 +-
 drivers/media/platform/rcar-vin/rcar-vin.h  |  24 +-
 3 files changed, 581 insertions(+), 436 deletions(-)

-- 
2.31.1

[PATCH 01/11] rcar-vin: Refactor controls creation for video device

[Niklas Söderlund]

The controls for the video device are created in different code paths
depending on if the driver is using the media graph centric model (Gen3)
or the device centric model (Gen2 and earlier). This have lead to code
duplication that can be consolidated.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 82 +++++++++++----------
 1 file changed, 45 insertions(+), 37 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index cb3025992817d625..c798dc9409e4cdcd 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -405,6 +405,45 @@ static const struct v4l2_ctrl_ops rvin_ctrl_ops = {
 	.s_ctrl = rvin_s_ctrl,
 };
 
+static void rvin_free_controls(struct rvin_dev *vin)
+{
+	v4l2_ctrl_handler_free(&vin->ctrl_handler);
+	vin->vdev.ctrl_handler = NULL;
+}
+
+static int rvin_create_controls(struct rvin_dev *vin, struct v4l2_subdev *subdev)
+{
+	int ret;
+
+	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
+	if (ret < 0)
+		return ret;
+
+	/* The VIN directly deals with alpha component. */
+	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
+			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
+
+	if (vin->ctrl_handler.error) {
+		ret = vin->ctrl_handler.error;
+		rvin_free_controls(vin);
+		return ret;
+	}
+
+	/* For the non-MC mode add controls from the subdevice. */
+	if (subdev) {
+		ret = v4l2_ctrl_add_handler(&vin->ctrl_handler,
+					    subdev->ctrl_handler, NULL, true);
+		if (ret < 0) {
+			rvin_free_controls(vin);
+			return ret;
+		}
+	}
+
+	vin->vdev.ctrl_handler = &vin->ctrl_handler;
+
+	return 0;
+}
+
 /* -----------------------------------------------------------------------------
  * Async notifier
  */
@@ -490,28 +529,10 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
 		return ret;
 
 	/* Add the controls */
-	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
+	ret = rvin_create_controls(vin, subdev);
 	if (ret < 0)
 		return ret;
 
-	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
-			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
-
-	if (vin->ctrl_handler.error) {
-		ret = vin->ctrl_handler.error;
-		v4l2_ctrl_handler_free(&vin->ctrl_handler);
-		return ret;
-	}
-
-	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler,
-				    NULL, true);
-	if (ret < 0) {
-		v4l2_ctrl_handler_free(&vin->ctrl_handler);
-		return ret;
-	}
-
-	vin->vdev.ctrl_handler = &vin->ctrl_handler;
-
 	vin->parallel.subdev = subdev;
 
 	return 0;
@@ -522,10 +543,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
 	rvin_v4l2_unregister(vin);
 	vin->parallel.subdev = NULL;
 
-	if (!vin->info->use_mc) {
-		v4l2_ctrl_handler_free(&vin->ctrl_handler);
-		vin->vdev.ctrl_handler = NULL;
-	}
+	if (!vin->info->use_mc)
+		rvin_free_controls(vin);
 }
 
 static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
@@ -935,21 +954,10 @@ static int rvin_mc_init(struct rvin_dev *vin)
 	if (ret)
 		rvin_group_put(vin);
 
-	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1);
+	ret = rvin_create_controls(vin, NULL);
 	if (ret < 0)
 		return ret;
 
-	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
-			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
-
-	if (vin->ctrl_handler.error) {
-		ret = vin->ctrl_handler.error;
-		v4l2_ctrl_handler_free(&vin->ctrl_handler);
-		return ret;
-	}
-
-	vin->vdev.ctrl_handler = &vin->ctrl_handler;
-
 	return ret;
 }
 
@@ -1446,7 +1454,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
 	return 0;
 
 error_group_unregister:
-	v4l2_ctrl_handler_free(&vin->ctrl_handler);
+	rvin_free_controls(vin);
 
 	if (vin->info->use_mc) {
 		mutex_lock(&vin->group->lock);
@@ -1481,7 +1489,7 @@ static int rcar_vin_remove(struct platform_device *pdev)
 		rvin_group_put(vin);
 	}
 
-	v4l2_ctrl_handler_free(&vin->ctrl_handler);
+	rvin_free_controls(vin);
 
 	rvin_dma_unregister(vin);
 
-- 
2.31.1

[PATCH 02/11] rcar-vin: Fix error paths for rvin_mc_init()

[Niklas Söderlund]

The error paths of rvin_mc_init() do not clean up properly, fix this.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index c798dc9409e4cdcd..d4932f7b42647ee1 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -946,17 +946,23 @@ static int rvin_mc_init(struct rvin_dev *vin)
 	if (ret)
 		return ret;
 
+	ret = rvin_create_controls(vin, NULL);
+	if (ret < 0)
+		return ret;
+
 	ret = rvin_group_get(vin);
 	if (ret)
-		return ret;
+		goto err_controls;
 
 	ret = rvin_mc_parse_of_graph(vin);
 	if (ret)
-		rvin_group_put(vin);
+		goto err_group;
 
-	ret = rvin_create_controls(vin, NULL);
-	if (ret < 0)
-		return ret;
+	return 0;
+err_group:
+	rvin_group_put(vin);
+err_controls:
+	rvin_free_controls(vin);
 
 	return ret;
 }
-- 
2.31.1

[PATCH 03/11] rcar-vin: Improve async notifier cleanup paths

[Niklas Söderlund]

The cleanup code for the async notifiers can be refactored to own
functions to reduce code duplication and improve readability. While at
it rename the CSI-2 initialization function _csi2_ instead of _mc_ to
match.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 51 ++++++++++++---------
 1 file changed, 30 insertions(+), 21 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index d4932f7b42647ee1..da23d55aa72b7f0d 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -383,6 +383,16 @@ static void rvin_group_put(struct rvin_dev *vin)
 	kref_put(&group->refcount, rvin_group_release);
 }
 
+static void rvin_group_notifier_cleanup(struct rvin_dev *vin)
+{
+	mutex_lock(&vin->group->lock);
+	if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) {
+		v4l2_async_notifier_unregister(&vin->group->notifier);
+		v4l2_async_notifier_cleanup(&vin->group->notifier);
+	}
+	mutex_unlock(&vin->group->lock);
+}
+
 /* -----------------------------------------------------------------------------
  * Controls
  */
@@ -676,6 +686,12 @@ static int rvin_parallel_parse_of(struct rvin_dev *vin)
 	return ret;
 }
 
+static void rvin_parallel_cleanup(struct rvin_dev *vin)
+{
+	v4l2_async_notifier_unregister(&vin->notifier);
+	v4l2_async_notifier_cleanup(&vin->notifier);
+}
+
 static int rvin_parallel_init(struct rvin_dev *vin)
 {
 	int ret;
@@ -937,7 +953,16 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
 	return 0;
 }
 
-static int rvin_mc_init(struct rvin_dev *vin)
+static void rvin_csi2_cleanup(struct rvin_dev *vin)
+{
+	if (!vin->info->use_mc)
+		return;
+
+	rvin_group_notifier_cleanup(vin);
+	rvin_group_put(vin);
+}
+
+static int rvin_csi2_init(struct rvin_dev *vin)
 {
 	int ret;
 
@@ -1445,7 +1470,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, vin);
 
 	if (vin->info->use_mc) {
-		ret = rvin_mc_init(vin);
+		ret = rvin_csi2_init(vin);
 		if (ret)
 			goto error_dma_unregister;
 	}
@@ -1458,20 +1483,9 @@ static int rcar_vin_probe(struct platform_device *pdev)
 	pm_runtime_enable(&pdev->dev);
 
 	return 0;
-
 error_group_unregister:
 	rvin_free_controls(vin);
-
-	if (vin->info->use_mc) {
-		mutex_lock(&vin->group->lock);
-		if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) {
-			v4l2_async_notifier_unregister(&vin->group->notifier);
-			v4l2_async_notifier_cleanup(&vin->group->notifier);
-		}
-		mutex_unlock(&vin->group->lock);
-		rvin_group_put(vin);
-	}
-
+	rvin_csi2_cleanup(vin);
 error_dma_unregister:
 	rvin_dma_unregister(vin);
 
@@ -1486,14 +1500,9 @@ static int rcar_vin_remove(struct platform_device *pdev)
 
 	rvin_v4l2_unregister(vin);
 
-	v4l2_async_notifier_unregister(&vin->notifier);
-	v4l2_async_notifier_cleanup(&vin->notifier);
+	rvin_parallel_cleanup(vin);
 
-	if (vin->info->use_mc) {
-		v4l2_async_notifier_unregister(&vin->group->notifier);
-		v4l2_async_notifier_cleanup(&vin->group->notifier);
-		rvin_group_put(vin);
-	}
+	rvin_csi2_cleanup(vin);
 
 	rvin_free_controls(vin);
 
-- 
2.31.1

[PATCH 04/11] rcar-vin: Improve reuse of parallel notifier

[Niklas Söderlund]

In preparation for adding a new media graph layout move the code reuse
of the parallel notifier setup from probe directly to the current media
graph initialization function. This is needed as there will be no
parallel interface in the new graph layout.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 48 ++++++++++-----------
 1 file changed, 22 insertions(+), 26 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index da23d55aa72b7f0d..81574bf33116ad59 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -702,9 +702,8 @@ static int rvin_parallel_init(struct rvin_dev *vin)
 	if (ret)
 		return ret;
 
-	/* If using mc, it's fine not to have any input registered. */
 	if (!vin->parallel.asd)
-		return vin->info->use_mc ? 0 : -ENODEV;
+		return -ENODEV;
 
 	vin_dbg(vin, "Found parallel subdevice %pOF\n",
 		to_of_node(vin->parallel.asd->match.fwnode));
@@ -955,11 +954,9 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
 
 static void rvin_csi2_cleanup(struct rvin_dev *vin)
 {
-	if (!vin->info->use_mc)
-		return;
-
 	rvin_group_notifier_cleanup(vin);
 	rvin_group_put(vin);
+	rvin_free_controls(vin);
 }
 
 static int rvin_csi2_init(struct rvin_dev *vin)
@@ -979,11 +976,18 @@ static int rvin_csi2_init(struct rvin_dev *vin)
 	if (ret)
 		goto err_controls;
 
-	ret = rvin_mc_parse_of_graph(vin);
-	if (ret)
+	/* It's OK to not have a parallel subdevice. */
+	ret = rvin_parallel_init(vin);
+	if (ret && ret != -ENODEV)
 		goto err_group;
 
+	ret = rvin_mc_parse_of_graph(vin);
+	if (ret)
+		goto err_parallel;
+
 	return 0;
+err_parallel:
+	rvin_parallel_cleanup(vin);
 err_group:
 	rvin_group_put(vin);
 err_controls:
@@ -1469,27 +1473,20 @@ static int rcar_vin_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, vin);
 
-	if (vin->info->use_mc) {
+	if (vin->info->use_mc)
 		ret = rvin_csi2_init(vin);
-		if (ret)
-			goto error_dma_unregister;
-	}
+	else
+		ret = rvin_parallel_init(vin);
 
-	ret = rvin_parallel_init(vin);
-	if (ret)
-		goto error_group_unregister;
+	if (ret) {
+		rvin_dma_unregister(vin);
+		return ret;
+	}
 
 	pm_suspend_ignore_children(&pdev->dev, true);
 	pm_runtime_enable(&pdev->dev);
 
 	return 0;
-error_group_unregister:
-	rvin_free_controls(vin);
-	rvin_csi2_cleanup(vin);
-error_dma_unregister:
-	rvin_dma_unregister(vin);
-
-	return ret;
 }
 
 static int rcar_vin_remove(struct platform_device *pdev)
@@ -1500,11 +1497,10 @@ static int rcar_vin_remove(struct platform_device *pdev)
 
 	rvin_v4l2_unregister(vin);
 
-	rvin_parallel_cleanup(vin);
-
-	rvin_csi2_cleanup(vin);
-
-	rvin_free_controls(vin);
+	if (vin->info->use_mc)
+		rvin_csi2_cleanup(vin);
+	else
+		rvin_parallel_cleanup(vin);
 
 	rvin_dma_unregister(vin);
 
-- 
2.31.1

[PATCH 05/11] rcar-vin: Rename array storing subdevice information

[Niklas Söderlund]

The VIN group have always been connected to CSI-2 receivers and this
have spilled over to the naming of the array storing the subdevice
information. In preparation for connecting other types of subdevices
rename the array to remotes.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 32 ++++++++++-----------
 drivers/media/platform/rcar-vin/rcar-vin.h  |  8 ++++--
 2 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 81574bf33116ad59..cc980cad805c022c 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -49,16 +49,16 @@
  */
 
 /* group lock should be held when calling this function. */
-static int rvin_group_entity_to_csi_id(struct rvin_group *group,
-				       struct media_entity *entity)
+static int rvin_group_entity_to_remote_id(struct rvin_group *group,
+					  struct media_entity *entity)
 {
 	struct v4l2_subdev *sd;
 	unsigned int i;
 
 	sd = media_entity_to_v4l2_subdev(entity);
 
-	for (i = 0; i < RVIN_CSI_MAX; i++)
-		if (group->csi[i].subdev == sd)
+	for (i = 0; i < RVIN_REMOTES_MAX; i++)
+		if (group->remotes[i].subdev == sd)
 			return i;
 
 	return -ENODEV;
@@ -163,14 +163,14 @@ static int rvin_group_link_notify(struct media_link *link, u32 flags,
 		if (!csi_pad)
 			continue;
 
-		csi_id = rvin_group_entity_to_csi_id(group, csi_pad->entity);
+		csi_id = rvin_group_entity_to_remote_id(group, csi_pad->entity);
 		channel = rvin_group_csi_pad_to_channel(csi_pad->index);
 
 		mask &= rvin_group_get_mask(group->vin[i], csi_id, channel);
 	}
 
 	/* Add the new link to the existing mask and check if it works. */
-	csi_id = rvin_group_entity_to_csi_id(group, link->source->entity);
+	csi_id = rvin_group_entity_to_remote_id(group, link->source->entity);
 
 	if (csi_id == -ENODEV) {
 		struct v4l2_subdev *sd;
@@ -766,10 +766,10 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
 			continue;
 
 		/* Check that CSI-2 is part of the group. */
-		if (!vin->group->csi[route->csi].subdev)
+		if (!vin->group->remotes[route->csi].subdev)
 			continue;
 
-		source = &vin->group->csi[route->csi].subdev->entity;
+		source = &vin->group->remotes[route->csi].subdev->entity;
 		source_idx = rvin_group_csi_channel_to_pad(route->channel);
 		source_pad = &source->pads[source_idx];
 
@@ -806,10 +806,10 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
 	mutex_lock(&vin->group->lock);
 
 	for (i = 0; i < RVIN_CSI_MAX; i++) {
-		if (vin->group->csi[i].asd != asd)
+		if (vin->group->remotes[i].asd != asd)
 			continue;
-		vin->group->csi[i].subdev = NULL;
-		vin_dbg(vin, "Unbind CSI-2 %s from slot %u\n", subdev->name, i);
+		vin->group->remotes[i].subdev = NULL;
+		vin_dbg(vin, "Unbind %s from slot %u\n", subdev->name, i);
 		break;
 	}
 
@@ -828,10 +828,10 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
 	mutex_lock(&vin->group->lock);
 
 	for (i = 0; i < RVIN_CSI_MAX; i++) {
-		if (vin->group->csi[i].asd != asd)
+		if (vin->group->remotes[i].asd != asd)
 			continue;
-		vin->group->csi[i].subdev = subdev;
-		vin_dbg(vin, "Bound CSI-2 %s to slot %u\n", subdev->name, i);
+		vin->group->remotes[i].subdev = subdev;
+		vin_dbg(vin, "Bound %s to slot %u\n", subdev->name, i);
 		break;
 	}
 
@@ -883,7 +883,7 @@ static int rvin_mc_parse_of(struct rvin_dev *vin, unsigned int id)
 		goto out;
 	}
 
-	vin->group->csi[vep.base.id].asd = asd;
+	vin->group->remotes[vep.base.id].asd = asd;
 
 	vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
 		to_of_node(fwnode), vep.base.id);
@@ -928,7 +928,7 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
 			continue;
 
 		for (id = 0; id < RVIN_CSI_MAX; id++) {
-			if (vin->group->csi[id].asd)
+			if (vin->group->remotes[id].asd)
 				continue;
 
 			ret = rvin_mc_parse_of(vin->group->vin[i], id);
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index b263ead4db2bfb08..39207aaf39ef9391 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -48,6 +48,8 @@ enum rvin_csi_id {
 	RVIN_CSI_MAX,
 };
 
+#define RVIN_REMOTES_MAX RVIN_CSI_MAX
+
 /**
  * enum rvin_dma_state - DMA states
  * @STOPPED:   No operation in progress
@@ -267,8 +269,8 @@ struct rvin_dev {
  * @count:		number of enabled VIN instances found in DT
  * @notifier:		group notifier for CSI-2 async subdevices
  * @vin:		VIN instances which are part of the group
- * @csi:		array of pairs of fwnode and subdev pointers
- *			to all CSI-2 subdevices.
+ * @remotes:		array of pairs of fwnode and subdev pointers
+ *			to all remote subdevices.
  */
 struct rvin_group {
 	struct kref refcount;
@@ -283,7 +285,7 @@ struct rvin_group {
 	struct {
 		struct v4l2_async_subdev *asd;
 		struct v4l2_subdev *subdev;
-	} csi[RVIN_CSI_MAX];
+	} remotes[RVIN_REMOTES_MAX];
 };
 
 int rvin_dma_register(struct rvin_dev *vin, int irq);
-- 
2.31.1

[PATCH 06/11] rcar-vin: Move group async notifier

[Niklas Söderlund]

The VIN group notifier code is intertwined with the media graph layout
code for R-Car CSI-2 subdevices, this makes it hard to extend the group
to also support the R-Car ISP channel selector.

Before breaking the two concepts apart and extending it move the group
code to its final location. There is no functional change and all
functions are moved verbatim.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 460 ++++++++++----------
 1 file changed, 230 insertions(+), 230 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index cc980cad805c022c..d951f739b3a9a034 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -383,6 +383,176 @@ static void rvin_group_put(struct rvin_dev *vin)
 	kref_put(&group->refcount, rvin_group_release);
 }
 
+static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
+{
+	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
+	const struct rvin_group_route *route;
+	unsigned int i;
+	int ret;
+
+	ret = media_device_register(&vin->group->mdev);
+	if (ret)
+		return ret;
+
+	ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);
+	if (ret) {
+		vin_err(vin, "Failed to register subdev nodes\n");
+		return ret;
+	}
+
+	/* Register all video nodes for the group. */
+	for (i = 0; i < RCAR_VIN_NUM; i++) {
+		if (vin->group->vin[i] &&
+		    !video_is_registered(&vin->group->vin[i]->vdev)) {
+			ret = rvin_v4l2_register(vin->group->vin[i]);
+			if (ret)
+				return ret;
+		}
+	}
+
+	/* Create all media device links between VINs and CSI-2's. */
+	mutex_lock(&vin->group->lock);
+	for (route = vin->info->routes; route->mask; route++) {
+		struct media_pad *source_pad, *sink_pad;
+		struct media_entity *source, *sink;
+		unsigned int source_idx;
+
+		/* Check that VIN is part of the group. */
+		if (!vin->group->vin[route->vin])
+			continue;
+
+		/* Check that VIN' master is part of the group. */
+		if (!vin->group->vin[rvin_group_id_to_master(route->vin)])
+			continue;
+
+		/* Check that CSI-2 is part of the group. */
+		if (!vin->group->remotes[route->csi].subdev)
+			continue;
+
+		source = &vin->group->remotes[route->csi].subdev->entity;
+		source_idx = rvin_group_csi_channel_to_pad(route->channel);
+		source_pad = &source->pads[source_idx];
+
+		sink = &vin->group->vin[route->vin]->vdev.entity;
+		sink_pad = &sink->pads[0];
+
+		/* Skip if link already exists. */
+		if (media_entity_find_link(source_pad, sink_pad))
+			continue;
+
+		ret = media_create_pad_link(source, source_idx, sink, 0, 0);
+		if (ret) {
+			vin_err(vin, "Error adding link from %s to %s\n",
+				source->name, sink->name);
+			break;
+		}
+	}
+	mutex_unlock(&vin->group->lock);
+
+	return ret;
+}
+
+static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
+				     struct v4l2_subdev *subdev,
+				     struct v4l2_async_subdev *asd)
+{
+	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
+	unsigned int i;
+
+	for (i = 0; i < RCAR_VIN_NUM; i++)
+		if (vin->group->vin[i])
+			rvin_v4l2_unregister(vin->group->vin[i]);
+
+	mutex_lock(&vin->group->lock);
+
+	for (i = 0; i < RVIN_CSI_MAX; i++) {
+		if (vin->group->remotes[i].asd != asd)
+			continue;
+		vin->group->remotes[i].subdev = NULL;
+		vin_dbg(vin, "Unbind %s from slot %u\n", subdev->name, i);
+		break;
+	}
+
+	mutex_unlock(&vin->group->lock);
+
+	media_device_unregister(&vin->group->mdev);
+}
+
+static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
+				   struct v4l2_subdev *subdev,
+				   struct v4l2_async_subdev *asd)
+{
+	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
+	unsigned int i;
+
+	mutex_lock(&vin->group->lock);
+
+	for (i = 0; i < RVIN_CSI_MAX; i++) {
+		if (vin->group->remotes[i].asd != asd)
+			continue;
+		vin->group->remotes[i].subdev = subdev;
+		vin_dbg(vin, "Bound %s to slot %u\n", subdev->name, i);
+		break;
+	}
+
+	mutex_unlock(&vin->group->lock);
+
+	return 0;
+}
+
+static const struct v4l2_async_notifier_operations rvin_group_notify_ops = {
+	.bound = rvin_group_notify_bound,
+	.unbind = rvin_group_notify_unbind,
+	.complete = rvin_group_notify_complete,
+};
+
+static int rvin_mc_parse_of(struct rvin_dev *vin, unsigned int id)
+{
+	struct fwnode_handle *ep, *fwnode;
+	struct v4l2_fwnode_endpoint vep = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY,
+	};
+	struct v4l2_async_subdev *asd;
+	int ret;
+
+	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 1, id, 0);
+	if (!ep)
+		return 0;
+
+	fwnode = fwnode_graph_get_remote_endpoint(ep);
+	ret = v4l2_fwnode_endpoint_parse(ep, &vep);
+	fwnode_handle_put(ep);
+	if (ret) {
+		vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (!of_device_is_available(to_of_node(fwnode))) {
+		vin_dbg(vin, "OF device %pOF disabled, ignoring\n",
+			to_of_node(fwnode));
+		ret = -ENOTCONN;
+		goto out;
+	}
+
+	asd = v4l2_async_notifier_add_fwnode_subdev(&vin->group->notifier,
+						    fwnode,
+						    struct v4l2_async_subdev);
+	if (IS_ERR(asd)) {
+		ret = PTR_ERR(asd);
+		goto out;
+	}
+
+	vin->group->remotes[vep.base.id].asd = asd;
+
+	vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
+		to_of_node(fwnode), vep.base.id);
+out:
+	fwnode_handle_put(fwnode);
+
+	return ret;
+}
+
 static void rvin_group_notifier_cleanup(struct rvin_dev *vin)
 {
 	mutex_lock(&vin->group->lock);
@@ -393,6 +563,65 @@ static void rvin_group_notifier_cleanup(struct rvin_dev *vin)
 	mutex_unlock(&vin->group->lock);
 }
 
+static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
+{
+	unsigned int count = 0, vin_mask = 0;
+	unsigned int i, id;
+	int ret;
+
+	mutex_lock(&vin->group->lock);
+
+	/* If not all VIN's are registered don't register the notifier. */
+	for (i = 0; i < RCAR_VIN_NUM; i++) {
+		if (vin->group->vin[i]) {
+			count++;
+			vin_mask |= BIT(i);
+		}
+	}
+
+	if (vin->group->count != count) {
+		mutex_unlock(&vin->group->lock);
+		return 0;
+	}
+
+	mutex_unlock(&vin->group->lock);
+
+	v4l2_async_notifier_init(&vin->group->notifier);
+
+	/*
+	 * Have all VIN's look for CSI-2 subdevices. Some subdevices will
+	 * overlap but the parser function can handle it, so each subdevice
+	 * will only be registered once with the group notifier.
+	 */
+	for (i = 0; i < RCAR_VIN_NUM; i++) {
+		if (!(vin_mask & BIT(i)))
+			continue;
+
+		for (id = 0; id < RVIN_CSI_MAX; id++) {
+			if (vin->group->remotes[id].asd)
+				continue;
+
+			ret = rvin_mc_parse_of(vin->group->vin[i], id);
+			if (ret)
+				return ret;
+		}
+	}
+
+	if (list_empty(&vin->group->notifier.asd_list))
+		return 0;
+
+	vin->group->notifier.ops = &rvin_group_notify_ops;
+	ret = v4l2_async_notifier_register(&vin->v4l2_dev,
+					   &vin->group->notifier);
+	if (ret < 0) {
+		vin_err(vin, "Notifier registration failed\n");
+		v4l2_async_notifier_cleanup(&vin->group->notifier);
+		return ret;
+	}
+
+	return 0;
+}
+
 /* -----------------------------------------------------------------------------
  * Controls
  */
@@ -720,238 +949,9 @@ static int rvin_parallel_init(struct rvin_dev *vin)
 }
 
 /* -----------------------------------------------------------------------------
- * Group async notifier
+ * CSI-2
  */
 
-static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
-{
-	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
-	const struct rvin_group_route *route;
-	unsigned int i;
-	int ret;
-
-	ret = media_device_register(&vin->group->mdev);
-	if (ret)
-		return ret;
-
-	ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);
-	if (ret) {
-		vin_err(vin, "Failed to register subdev nodes\n");
-		return ret;
-	}
-
-	/* Register all video nodes for the group. */
-	for (i = 0; i < RCAR_VIN_NUM; i++) {
-		if (vin->group->vin[i] &&
-		    !video_is_registered(&vin->group->vin[i]->vdev)) {
-			ret = rvin_v4l2_register(vin->group->vin[i]);
-			if (ret)
-				return ret;
-		}
-	}
-
-	/* Create all media device links between VINs and CSI-2's. */
-	mutex_lock(&vin->group->lock);
-	for (route = vin->info->routes; route->mask; route++) {
-		struct media_pad *source_pad, *sink_pad;
-		struct media_entity *source, *sink;
-		unsigned int source_idx;
-
-		/* Check that VIN is part of the group. */
-		if (!vin->group->vin[route->vin])
-			continue;
-
-		/* Check that VIN' master is part of the group. */
-		if (!vin->group->vin[rvin_group_id_to_master(route->vin)])
-			continue;
-
-		/* Check that CSI-2 is part of the group. */
-		if (!vin->group->remotes[route->csi].subdev)
-			continue;
-
-		source = &vin->group->remotes[route->csi].subdev->entity;
-		source_idx = rvin_group_csi_channel_to_pad(route->channel);
-		source_pad = &source->pads[source_idx];
-
-		sink = &vin->group->vin[route->vin]->vdev.entity;
-		sink_pad = &sink->pads[0];
-
-		/* Skip if link already exists. */
-		if (media_entity_find_link(source_pad, sink_pad))
-			continue;
-
-		ret = media_create_pad_link(source, source_idx, sink, 0, 0);
-		if (ret) {
-			vin_err(vin, "Error adding link from %s to %s\n",
-				source->name, sink->name);
-			break;
-		}
-	}
-	mutex_unlock(&vin->group->lock);
-
-	return ret;
-}
-
-static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
-				     struct v4l2_subdev *subdev,
-				     struct v4l2_async_subdev *asd)
-{
-	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
-	unsigned int i;
-
-	for (i = 0; i < RCAR_VIN_NUM; i++)
-		if (vin->group->vin[i])
-			rvin_v4l2_unregister(vin->group->vin[i]);
-
-	mutex_lock(&vin->group->lock);
-
-	for (i = 0; i < RVIN_CSI_MAX; i++) {
-		if (vin->group->remotes[i].asd != asd)
-			continue;
-		vin->group->remotes[i].subdev = NULL;
-		vin_dbg(vin, "Unbind %s from slot %u\n", subdev->name, i);
-		break;
-	}
-
-	mutex_unlock(&vin->group->lock);
-
-	media_device_unregister(&vin->group->mdev);
-}
-
-static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
-				   struct v4l2_subdev *subdev,
-				   struct v4l2_async_subdev *asd)
-{
-	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
-	unsigned int i;
-
-	mutex_lock(&vin->group->lock);
-
-	for (i = 0; i < RVIN_CSI_MAX; i++) {
-		if (vin->group->remotes[i].asd != asd)
-			continue;
-		vin->group->remotes[i].subdev = subdev;
-		vin_dbg(vin, "Bound %s to slot %u\n", subdev->name, i);
-		break;
-	}
-
-	mutex_unlock(&vin->group->lock);
-
-	return 0;
-}
-
-static const struct v4l2_async_notifier_operations rvin_group_notify_ops = {
-	.bound = rvin_group_notify_bound,
-	.unbind = rvin_group_notify_unbind,
-	.complete = rvin_group_notify_complete,
-};
-
-static int rvin_mc_parse_of(struct rvin_dev *vin, unsigned int id)
-{
-	struct fwnode_handle *ep, *fwnode;
-	struct v4l2_fwnode_endpoint vep = {
-		.bus_type = V4L2_MBUS_CSI2_DPHY,
-	};
-	struct v4l2_async_subdev *asd;
-	int ret;
-
-	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 1, id, 0);
-	if (!ep)
-		return 0;
-
-	fwnode = fwnode_graph_get_remote_endpoint(ep);
-	ret = v4l2_fwnode_endpoint_parse(ep, &vep);
-	fwnode_handle_put(ep);
-	if (ret) {
-		vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
-		ret = -EINVAL;
-		goto out;
-	}
-
-	if (!of_device_is_available(to_of_node(fwnode))) {
-		vin_dbg(vin, "OF device %pOF disabled, ignoring\n",
-			to_of_node(fwnode));
-		ret = -ENOTCONN;
-		goto out;
-	}
-
-	asd = v4l2_async_notifier_add_fwnode_subdev(&vin->group->notifier,
-						    fwnode,
-						    struct v4l2_async_subdev);
-	if (IS_ERR(asd)) {
-		ret = PTR_ERR(asd);
-		goto out;
-	}
-
-	vin->group->remotes[vep.base.id].asd = asd;
-
-	vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
-		to_of_node(fwnode), vep.base.id);
-out:
-	fwnode_handle_put(fwnode);
-
-	return ret;
-}
-
-static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
-{
-	unsigned int count = 0, vin_mask = 0;
-	unsigned int i, id;
-	int ret;
-
-	mutex_lock(&vin->group->lock);
-
-	/* If not all VIN's are registered don't register the notifier. */
-	for (i = 0; i < RCAR_VIN_NUM; i++) {
-		if (vin->group->vin[i]) {
-			count++;
-			vin_mask |= BIT(i);
-		}
-	}
-
-	if (vin->group->count != count) {
-		mutex_unlock(&vin->group->lock);
-		return 0;
-	}
-
-	mutex_unlock(&vin->group->lock);
-
-	v4l2_async_notifier_init(&vin->group->notifier);
-
-	/*
-	 * Have all VIN's look for CSI-2 subdevices. Some subdevices will
-	 * overlap but the parser function can handle it, so each subdevice
-	 * will only be registered once with the group notifier.
-	 */
-	for (i = 0; i < RCAR_VIN_NUM; i++) {
-		if (!(vin_mask & BIT(i)))
-			continue;
-
-		for (id = 0; id < RVIN_CSI_MAX; id++) {
-			if (vin->group->remotes[id].asd)
-				continue;
-
-			ret = rvin_mc_parse_of(vin->group->vin[i], id);
-			if (ret)
-				return ret;
-		}
-	}
-
-	if (list_empty(&vin->group->notifier.asd_list))
-		return 0;
-
-	vin->group->notifier.ops = &rvin_group_notify_ops;
-	ret = v4l2_async_notifier_register(&vin->v4l2_dev,
-					   &vin->group->notifier);
-	if (ret < 0) {
-		vin_err(vin, "Notifier registration failed\n");
-		v4l2_async_notifier_cleanup(&vin->group->notifier);
-		return ret;
-	}
-
-	return 0;
-}
-
 static void rvin_csi2_cleanup(struct rvin_dev *vin)
 {
 	rvin_group_notifier_cleanup(vin);
-- 
2.31.1

[PATCH 07/11] rcar-vin: Extend group notifier DT parser to work with any port

[Niklas Söderlund]

The R-Car VIN group notifier will be extend to support a new group of
subdevices, the R-Car ISP channel selector in addition to the existing
R-Car CSI-2 receiver subdevices.

The existing DT parsing code can be reused if the port and max number of
endpoints are provided as parameters instead of being hard-coded. While
at it align the group notifier parser function names with the rest of
the driver.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index d951f739b3a9a034..2628637084ae2aa9 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -506,7 +506,8 @@ static const struct v4l2_async_notifier_operations rvin_group_notify_ops = {
 	.complete = rvin_group_notify_complete,
 };
 
-static int rvin_mc_parse_of(struct rvin_dev *vin, unsigned int id)
+static int rvin_group_parse_of(struct rvin_dev *vin, unsigned int port,
+			       unsigned int id)
 {
 	struct fwnode_handle *ep, *fwnode;
 	struct v4l2_fwnode_endpoint vep = {
@@ -515,7 +516,7 @@ static int rvin_mc_parse_of(struct rvin_dev *vin, unsigned int id)
 	struct v4l2_async_subdev *asd;
 	int ret;
 
-	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 1, id, 0);
+	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), port, id, 0);
 	if (!ep)
 		return 0;
 
@@ -563,7 +564,8 @@ static void rvin_group_notifier_cleanup(struct rvin_dev *vin)
 	mutex_unlock(&vin->group->lock);
 }
 
-static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
+static int rvin_group_notifier_init(struct rvin_dev *vin, unsigned int port,
+				    unsigned int max_id)
 {
 	unsigned int count = 0, vin_mask = 0;
 	unsigned int i, id;
@@ -589,19 +591,18 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
 	v4l2_async_notifier_init(&vin->group->notifier);
 
 	/*
-	 * Have all VIN's look for CSI-2 subdevices. Some subdevices will
-	 * overlap but the parser function can handle it, so each subdevice
-	 * will only be registered once with the group notifier.
+	 * Some subdevices may overlap but the parser function can handle it and
+	 * each subdevice will only be registered once with the group notifier.
 	 */
 	for (i = 0; i < RCAR_VIN_NUM; i++) {
 		if (!(vin_mask & BIT(i)))
 			continue;
 
-		for (id = 0; id < RVIN_CSI_MAX; id++) {
+		for (id = 0; id < max_id; id++) {
 			if (vin->group->remotes[id].asd)
 				continue;
 
-			ret = rvin_mc_parse_of(vin->group->vin[i], id);
+			ret = rvin_group_parse_of(vin->group->vin[i], port, id);
 			if (ret)
 				return ret;
 		}
@@ -981,7 +982,7 @@ static int rvin_csi2_init(struct rvin_dev *vin)
 	if (ret && ret != -ENODEV)
 		goto err_group;
 
-	ret = rvin_mc_parse_of_graph(vin);
+	ret = rvin_group_notifier_init(vin, 1, RVIN_CSI_MAX);
 	if (ret)
 		goto err_parallel;
 
-- 
2.31.1

[PATCH 08/11] rcar-vin: Create a callback to setup media links

[Niklas Söderlund]

New IP versions will have different media graphs and require a different
link setup. Breakout the specific link setup to a callback that are
associated with the group.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 97 ++++++++++++---------
 drivers/media/platform/rcar-vin/rcar-vin.h  |  3 +
 2 files changed, 57 insertions(+), 43 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 2628637084ae2aa9..d03ecd443b01c2b3 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -247,7 +247,8 @@ static void rvin_group_cleanup(struct rvin_group *group)
 	mutex_destroy(&group->lock);
 }
 
-static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
+static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin,
+			   void (*link_setup)(struct rvin_dev *))
 {
 	struct media_device *mdev = &group->mdev;
 	const struct of_device_id *match;
@@ -263,6 +264,8 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
 
 	vin_dbg(vin, "found %u enabled VIN's in DT", group->count);
 
+	group->link_setup = link_setup;
+
 	mdev->dev = vin->dev;
 	mdev->ops = &rvin_media_ops;
 
@@ -295,7 +298,8 @@ static void rvin_group_release(struct kref *kref)
 	mutex_unlock(&rvin_group_lock);
 }
 
-static int rvin_group_get(struct rvin_dev *vin)
+static int rvin_group_get(struct rvin_dev *vin,
+			  void (*link_setup)(struct rvin_dev *))
 {
 	struct rvin_group *group;
 	u32 id;
@@ -327,7 +331,7 @@ static int rvin_group_get(struct rvin_dev *vin)
 			goto err_group;
 		}
 
-		ret = rvin_group_init(group, vin);
+		ret = rvin_group_init(group, vin, link_setup);
 		if (ret) {
 			kfree(group);
 			vin_err(vin, "Failed to initialize group\n");
@@ -386,7 +390,6 @@ static void rvin_group_put(struct rvin_dev *vin)
 static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
 {
 	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
-	const struct rvin_group_route *route;
 	unsigned int i;
 	int ret;
 
@@ -410,44 +413,7 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
 		}
 	}
 
-	/* Create all media device links between VINs and CSI-2's. */
-	mutex_lock(&vin->group->lock);
-	for (route = vin->info->routes; route->mask; route++) {
-		struct media_pad *source_pad, *sink_pad;
-		struct media_entity *source, *sink;
-		unsigned int source_idx;
-
-		/* Check that VIN is part of the group. */
-		if (!vin->group->vin[route->vin])
-			continue;
-
-		/* Check that VIN' master is part of the group. */
-		if (!vin->group->vin[rvin_group_id_to_master(route->vin)])
-			continue;
-
-		/* Check that CSI-2 is part of the group. */
-		if (!vin->group->remotes[route->csi].subdev)
-			continue;
-
-		source = &vin->group->remotes[route->csi].subdev->entity;
-		source_idx = rvin_group_csi_channel_to_pad(route->channel);
-		source_pad = &source->pads[source_idx];
-
-		sink = &vin->group->vin[route->vin]->vdev.entity;
-		sink_pad = &sink->pads[0];
-
-		/* Skip if link already exists. */
-		if (media_entity_find_link(source_pad, sink_pad))
-			continue;
-
-		ret = media_create_pad_link(source, source_idx, sink, 0, 0);
-		if (ret) {
-			vin_err(vin, "Error adding link from %s to %s\n",
-				source->name, sink->name);
-			break;
-		}
-	}
-	mutex_unlock(&vin->group->lock);
+	vin->group->link_setup(vin);
 
 	return ret;
 }
@@ -953,6 +919,51 @@ static int rvin_parallel_init(struct rvin_dev *vin)
  * CSI-2
  */
 
+static void rvin_csi2_setup_links(struct rvin_dev *vin)
+{
+	const struct rvin_group_route *route;
+	int ret;
+
+	/* Create all media device links between VINs and CSI-2's. */
+	mutex_lock(&vin->group->lock);
+	for (route = vin->info->routes; route->mask; route++) {
+		struct media_pad *source_pad, *sink_pad;
+		struct media_entity *source, *sink;
+		unsigned int source_idx;
+
+		/* Check that VIN is part of the group. */
+		if (!vin->group->vin[route->vin])
+			continue;
+
+		/* Check that VIN' master is part of the group. */
+		if (!vin->group->vin[rvin_group_id_to_master(route->vin)])
+			continue;
+
+		/* Check that CSI-2 is part of the group. */
+		if (!vin->group->remotes[route->csi].subdev)
+			continue;
+
+		source = &vin->group->remotes[route->csi].subdev->entity;
+		source_idx = rvin_group_csi_channel_to_pad(route->channel);
+		source_pad = &source->pads[source_idx];
+
+		sink = &vin->group->vin[route->vin]->vdev.entity;
+		sink_pad = &sink->pads[0];
+
+		/* Skip if link already exists. */
+		if (media_entity_find_link(source_pad, sink_pad))
+			continue;
+
+		ret = media_create_pad_link(source, source_idx, sink, 0, 0);
+		if (ret) {
+			vin_err(vin, "Error adding link from %s to %s\n",
+				source->name, sink->name);
+			break;
+		}
+	}
+	mutex_unlock(&vin->group->lock);
+}
+
 static void rvin_csi2_cleanup(struct rvin_dev *vin)
 {
 	rvin_group_notifier_cleanup(vin);
@@ -973,7 +984,7 @@ static int rvin_csi2_init(struct rvin_dev *vin)
 	if (ret < 0)
 		return ret;
 
-	ret = rvin_group_get(vin);
+	ret = rvin_group_get(vin, rvin_csi2_setup_links);
 	if (ret)
 		goto err_controls;
 
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 39207aaf39ef9391..f2c32d5ceb7f83d9 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -269,6 +269,7 @@ struct rvin_dev {
  * @count:		number of enabled VIN instances found in DT
  * @notifier:		group notifier for CSI-2 async subdevices
  * @vin:		VIN instances which are part of the group
+ * @link_setup:		Callback to create all links for the media graph
  * @remotes:		array of pairs of fwnode and subdev pointers
  *			to all remote subdevices.
  */
@@ -282,6 +283,8 @@ struct rvin_group {
 	struct v4l2_async_notifier notifier;
 	struct rvin_dev *vin[RCAR_VIN_NUM];
 
+	void (*link_setup)(struct rvin_dev *vin);
+
 	struct {
 		struct v4l2_async_subdev *asd;
 		struct v4l2_subdev *subdev;
-- 
2.31.1

[PATCH 09/11] rcar-vin: Specify media device ops at group creation time

[Niklas Söderlund]

The media device operations structure will be different deepening on
what media graph layout is used. Instead of hard-coding the R-Car CSI-2
layout allow the operations to be passed as an argument.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index d03ecd443b01c2b3..28bf9e8f19a1a27b 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -248,7 +248,8 @@ static void rvin_group_cleanup(struct rvin_group *group)
 }
 
 static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin,
-			   void (*link_setup)(struct rvin_dev *))
+			   void (*link_setup)(struct rvin_dev *),
+			   const struct media_device_ops *ops)
 {
 	struct media_device *mdev = &group->mdev;
 	const struct of_device_id *match;
@@ -267,7 +268,7 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin,
 	group->link_setup = link_setup;
 
 	mdev->dev = vin->dev;
-	mdev->ops = &rvin_media_ops;
+	mdev->ops = ops;
 
 	match = of_match_node(vin->dev->driver->of_match_table,
 			      vin->dev->of_node);
@@ -299,7 +300,8 @@ static void rvin_group_release(struct kref *kref)
 }
 
 static int rvin_group_get(struct rvin_dev *vin,
-			  void (*link_setup)(struct rvin_dev *))
+			  void (*link_setup)(struct rvin_dev *),
+			  const struct media_device_ops *ops)
 {
 	struct rvin_group *group;
 	u32 id;
@@ -331,7 +333,7 @@ static int rvin_group_get(struct rvin_dev *vin,
 			goto err_group;
 		}
 
-		ret = rvin_group_init(group, vin, link_setup);
+		ret = rvin_group_init(group, vin, link_setup, ops);
 		if (ret) {
 			kfree(group);
 			vin_err(vin, "Failed to initialize group\n");
@@ -984,7 +986,7 @@ static int rvin_csi2_init(struct rvin_dev *vin)
 	if (ret < 0)
 		return ret;
 
-	ret = rvin_group_get(vin, rvin_csi2_setup_links);
+	ret = rvin_group_get(vin, rvin_csi2_setup_links, &rvin_media_ops);
 	if (ret)
 		goto err_controls;
 
-- 
2.31.1

[PATCH 10/11] rcar-vin: Move and rename CSI-2 link notifications

[Niklas Söderlund]

The CSI-2 link notifications are no longer the only option for the VIN
group. Change the symbol prefix to rvin_csi2_ for all CSI-2 specific
code and move the link notification code to the correct section to not
mix it with the soon to be added R-Car ISP channel selector notification
helpers.

There is no functional change and apart from the symbol prefix change
all functions are moved verbatim.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 360 ++++++++++----------
 1 file changed, 178 insertions(+), 182 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 28bf9e8f19a1a27b..763be02f507e6f3f 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -44,187 +44,6 @@
 
 #define v4l2_dev_to_vin(d)	container_of(d, struct rvin_dev, v4l2_dev)
 
-/* -----------------------------------------------------------------------------
- * Media Controller link notification
- */
-
-/* group lock should be held when calling this function. */
-static int rvin_group_entity_to_remote_id(struct rvin_group *group,
-					  struct media_entity *entity)
-{
-	struct v4l2_subdev *sd;
-	unsigned int i;
-
-	sd = media_entity_to_v4l2_subdev(entity);
-
-	for (i = 0; i < RVIN_REMOTES_MAX; i++)
-		if (group->remotes[i].subdev == sd)
-			return i;
-
-	return -ENODEV;
-}
-
-static unsigned int rvin_group_get_mask(struct rvin_dev *vin,
-					enum rvin_csi_id csi_id,
-					unsigned char channel)
-{
-	const struct rvin_group_route *route;
-	unsigned int mask = 0;
-
-	for (route = vin->info->routes; route->mask; route++) {
-		if (route->vin == vin->id &&
-		    route->csi == csi_id &&
-		    route->channel == channel) {
-			vin_dbg(vin,
-				"Adding route: vin: %d csi: %d channel: %d\n",
-				route->vin, route->csi, route->channel);
-			mask |= route->mask;
-		}
-	}
-
-	return mask;
-}
-
-/*
- * Link setup for the links between a VIN and a CSI-2 receiver is a bit
- * complex. The reason for this is that the register controlling routing
- * is not present in each VIN instance. There are special VINs which
- * control routing for themselves and other VINs. There are not many
- * different possible links combinations that can be enabled at the same
- * time, therefor all already enabled links which are controlled by a
- * master VIN need to be taken into account when making the decision
- * if a new link can be enabled or not.
- *
- * 1. Find out which VIN the link the user tries to enable is connected to.
- * 2. Lookup which master VIN controls the links for this VIN.
- * 3. Start with a bitmask with all bits set.
- * 4. For each previously enabled link from the master VIN bitwise AND its
- *    route mask (see documentation for mask in struct rvin_group_route)
- *    with the bitmask.
- * 5. Bitwise AND the mask for the link the user tries to enable to the bitmask.
- * 6. If the bitmask is not empty at this point the new link can be enabled
- *    while keeping all previous links enabled. Update the CHSEL value of the
- *    master VIN and inform the user that the link could be enabled.
- *
- * Please note that no link can be enabled if any VIN in the group is
- * currently open.
- */
-static int rvin_group_link_notify(struct media_link *link, u32 flags,
-				  unsigned int notification)
-{
-	struct rvin_group *group = container_of(link->graph_obj.mdev,
-						struct rvin_group, mdev);
-	unsigned int master_id, channel, mask_new, i;
-	unsigned int mask = ~0;
-	struct media_entity *entity;
-	struct video_device *vdev;
-	struct media_pad *csi_pad;
-	struct rvin_dev *vin = NULL;
-	int csi_id, ret;
-
-	ret = v4l2_pipeline_link_notify(link, flags, notification);
-	if (ret)
-		return ret;
-
-	/* Only care about link enablement for VIN nodes. */
-	if (!(flags & MEDIA_LNK_FL_ENABLED) ||
-	    !is_media_entity_v4l2_video_device(link->sink->entity))
-		return 0;
-
-	/*
-	 * Don't allow link changes if any entity in the graph is
-	 * streaming, modifying the CHSEL register fields can disrupt
-	 * running streams.
-	 */
-	media_device_for_each_entity(entity, &group->mdev)
-		if (entity->stream_count)
-			return -EBUSY;
-
-	mutex_lock(&group->lock);
-
-	/* Find the master VIN that controls the routes. */
-	vdev = media_entity_to_video_device(link->sink->entity);
-	vin = container_of(vdev, struct rvin_dev, vdev);
-	master_id = rvin_group_id_to_master(vin->id);
-
-	if (WARN_ON(!group->vin[master_id])) {
-		ret = -ENODEV;
-		goto out;
-	}
-
-	/* Build a mask for already enabled links. */
-	for (i = master_id; i < master_id + 4; i++) {
-		if (!group->vin[i])
-			continue;
-
-		/* Get remote CSI-2, if any. */
-		csi_pad = media_entity_remote_pad(
-				&group->vin[i]->vdev.entity.pads[0]);
-		if (!csi_pad)
-			continue;
-
-		csi_id = rvin_group_entity_to_remote_id(group, csi_pad->entity);
-		channel = rvin_group_csi_pad_to_channel(csi_pad->index);
-
-		mask &= rvin_group_get_mask(group->vin[i], csi_id, channel);
-	}
-
-	/* Add the new link to the existing mask and check if it works. */
-	csi_id = rvin_group_entity_to_remote_id(group, link->source->entity);
-
-	if (csi_id == -ENODEV) {
-		struct v4l2_subdev *sd;
-
-		/*
-		 * Make sure the source entity subdevice is registered as
-		 * a parallel input of one of the enabled VINs if it is not
-		 * one of the CSI-2 subdevices.
-		 *
-		 * No hardware configuration required for parallel inputs,
-		 * we can return here.
-		 */
-		sd = media_entity_to_v4l2_subdev(link->source->entity);
-		for (i = 0; i < RCAR_VIN_NUM; i++) {
-			if (group->vin[i] &&
-			    group->vin[i]->parallel.subdev == sd) {
-				group->vin[i]->is_csi = false;
-				ret = 0;
-				goto out;
-			}
-		}
-
-		vin_err(vin, "Subdevice %s not registered to any VIN\n",
-			link->source->entity->name);
-		ret = -ENODEV;
-		goto out;
-	}
-
-	channel = rvin_group_csi_pad_to_channel(link->source->index);
-	mask_new = mask & rvin_group_get_mask(vin, csi_id, channel);
-	vin_dbg(vin, "Try link change mask: 0x%x new: 0x%x\n", mask, mask_new);
-
-	if (!mask_new) {
-		ret = -EMLINK;
-		goto out;
-	}
-
-	/* New valid CHSEL found, set the new value. */
-	ret = rvin_set_channel_routing(group->vin[master_id], __ffs(mask_new));
-	if (ret)
-		goto out;
-
-	vin->is_csi = true;
-
-out:
-	mutex_unlock(&group->lock);
-
-	return ret;
-}
-
-static const struct media_device_ops rvin_media_ops = {
-	.link_notify = rvin_group_link_notify,
-};
-
 /* -----------------------------------------------------------------------------
  * Gen3 CSI2 Group Allocator
  */
@@ -389,6 +208,22 @@ static void rvin_group_put(struct rvin_dev *vin)
 	kref_put(&group->refcount, rvin_group_release);
 }
 
+/* group lock should be held when calling this function. */
+static int rvin_group_entity_to_remote_id(struct rvin_group *group,
+					  struct media_entity *entity)
+{
+	struct v4l2_subdev *sd;
+	unsigned int i;
+
+	sd = media_entity_to_v4l2_subdev(entity);
+
+	for (i = 0; i < RVIN_REMOTES_MAX; i++)
+		if (group->remotes[i].subdev == sd)
+			return i;
+
+	return -ENODEV;
+}
+
 static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
 {
 	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
@@ -921,6 +756,167 @@ static int rvin_parallel_init(struct rvin_dev *vin)
  * CSI-2
  */
 
+static unsigned int rvin_csi2_get_mask(struct rvin_dev *vin,
+				       enum rvin_csi_id csi_id,
+				       unsigned char channel)
+{
+	const struct rvin_group_route *route;
+	unsigned int mask = 0;
+
+	for (route = vin->info->routes; route->mask; route++) {
+		if (route->vin == vin->id &&
+		    route->csi == csi_id &&
+		    route->channel == channel) {
+			vin_dbg(vin,
+				"Adding route: vin: %d csi: %d channel: %d\n",
+				route->vin, route->csi, route->channel);
+			mask |= route->mask;
+		}
+	}
+
+	return mask;
+}
+
+/*
+ * Link setup for the links between a VIN and a CSI-2 receiver is a bit
+ * complex. The reason for this is that the register controlling routing
+ * is not present in each VIN instance. There are special VINs which
+ * control routing for themselves and other VINs. There are not many
+ * different possible links combinations that can be enabled at the same
+ * time, therefor all already enabled links which are controlled by a
+ * master VIN need to be taken into account when making the decision
+ * if a new link can be enabled or not.
+ *
+ * 1. Find out which VIN the link the user tries to enable is connected to.
+ * 2. Lookup which master VIN controls the links for this VIN.
+ * 3. Start with a bitmask with all bits set.
+ * 4. For each previously enabled link from the master VIN bitwise AND its
+ *    route mask (see documentation for mask in struct rvin_group_route)
+ *    with the bitmask.
+ * 5. Bitwise AND the mask for the link the user tries to enable to the bitmask.
+ * 6. If the bitmask is not empty at this point the new link can be enabled
+ *    while keeping all previous links enabled. Update the CHSEL value of the
+ *    master VIN and inform the user that the link could be enabled.
+ *
+ * Please note that no link can be enabled if any VIN in the group is
+ * currently open.
+ */
+static int rvin_csi2_link_notify(struct media_link *link, u32 flags,
+				 unsigned int notification)
+{
+	struct rvin_group *group = container_of(link->graph_obj.mdev,
+						struct rvin_group, mdev);
+	unsigned int master_id, channel, mask_new, i;
+	unsigned int mask = ~0;
+	struct media_entity *entity;
+	struct video_device *vdev;
+	struct media_pad *csi_pad;
+	struct rvin_dev *vin = NULL;
+	int csi_id, ret;
+
+	ret = v4l2_pipeline_link_notify(link, flags, notification);
+	if (ret)
+		return ret;
+
+	/* Only care about link enablement for VIN nodes. */
+	if (!(flags & MEDIA_LNK_FL_ENABLED) ||
+	    !is_media_entity_v4l2_video_device(link->sink->entity))
+		return 0;
+
+	/*
+	 * Don't allow link changes if any entity in the graph is
+	 * streaming, modifying the CHSEL register fields can disrupt
+	 * running streams.
+	 */
+	media_device_for_each_entity(entity, &group->mdev)
+		if (entity->stream_count)
+			return -EBUSY;
+
+	mutex_lock(&group->lock);
+
+	/* Find the master VIN that controls the routes. */
+	vdev = media_entity_to_video_device(link->sink->entity);
+	vin = container_of(vdev, struct rvin_dev, vdev);
+	master_id = rvin_group_id_to_master(vin->id);
+
+	if (WARN_ON(!group->vin[master_id])) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	/* Build a mask for already enabled links. */
+	for (i = master_id; i < master_id + 4; i++) {
+		if (!group->vin[i])
+			continue;
+
+		/* Get remote CSI-2, if any. */
+		csi_pad = media_entity_remote_pad(
+				&group->vin[i]->vdev.entity.pads[0]);
+		if (!csi_pad)
+			continue;
+
+		csi_id = rvin_group_entity_to_remote_id(group, csi_pad->entity);
+		channel = rvin_group_csi_pad_to_channel(csi_pad->index);
+
+		mask &= rvin_csi2_get_mask(group->vin[i], csi_id, channel);
+	}
+
+	/* Add the new link to the existing mask and check if it works. */
+	csi_id = rvin_group_entity_to_remote_id(group, link->source->entity);
+
+	if (csi_id == -ENODEV) {
+		struct v4l2_subdev *sd;
+
+		/*
+		 * Make sure the source entity subdevice is registered as
+		 * a parallel input of one of the enabled VINs if it is not
+		 * one of the CSI-2 subdevices.
+		 *
+		 * No hardware configuration required for parallel inputs,
+		 * we can return here.
+		 */
+		sd = media_entity_to_v4l2_subdev(link->source->entity);
+		for (i = 0; i < RCAR_VIN_NUM; i++) {
+			if (group->vin[i] &&
+			    group->vin[i]->parallel.subdev == sd) {
+				group->vin[i]->is_csi = false;
+				ret = 0;
+				goto out;
+			}
+		}
+
+		vin_err(vin, "Subdevice %s not registered to any VIN\n",
+			link->source->entity->name);
+		ret = -ENODEV;
+		goto out;
+	}
+
+	channel = rvin_group_csi_pad_to_channel(link->source->index);
+	mask_new = mask & rvin_csi2_get_mask(vin, csi_id, channel);
+	vin_dbg(vin, "Try link change mask: 0x%x new: 0x%x\n", mask, mask_new);
+
+	if (!mask_new) {
+		ret = -EMLINK;
+		goto out;
+	}
+
+	/* New valid CHSEL found, set the new value. */
+	ret = rvin_set_channel_routing(group->vin[master_id], __ffs(mask_new));
+	if (ret)
+		goto out;
+
+	vin->is_csi = true;
+
+out:
+	mutex_unlock(&group->lock);
+
+	return ret;
+}
+
+static const struct media_device_ops rvin_csi2_media_ops = {
+	.link_notify = rvin_csi2_link_notify,
+};
+
 static void rvin_csi2_setup_links(struct rvin_dev *vin)
 {
 	const struct rvin_group_route *route;
@@ -986,7 +982,7 @@ static int rvin_csi2_init(struct rvin_dev *vin)
 	if (ret < 0)
 		return ret;
 
-	ret = rvin_group_get(vin, rvin_csi2_setup_links, &rvin_media_ops);
+	ret = rvin_group_get(vin, rvin_csi2_setup_links, &rvin_csi2_media_ops);
 	if (ret)
 		goto err_controls;
 
-- 
2.31.1

[PATCH 11/11] rcar-vin: Add r8a779a0 support

[Niklas Söderlund]

Add support for the R-Car V3U (r8a779a0) to the driver. The V3U have the
VIN modules connected to a ISP instead of directly to the R-Car CSI-2
receivers. The reason being that the ISP performs channel selection
based on CSI-2 VC/DT pairs and routes the video data to different VIN
modules. Other SoC versions this filtering is done by the VIN modules
themself.

While the media graph very different from other SoCs the only difference
in operating the VIN modules is that the VC/DT filtering should be
skipped as that is performed by the ISP.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 102 +++++++++++++++++++-
 drivers/media/platform/rcar-vin/rcar-dma.c  |  20 ++--
 drivers/media/platform/rcar-vin/rcar-vin.h  |  15 ++-
 3 files changed, 124 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 763be02f507e6f3f..98326e80d2ed23d2 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -1006,6 +1006,87 @@ static int rvin_csi2_init(struct rvin_dev *vin)
 	return ret;
 }
 
+/* -----------------------------------------------------------------------------
+ * ISP
+ */
+
+static void rvin_isp_setup_links(struct rvin_dev *vin)
+{
+	unsigned int i;
+
+	/* Create all media device links between VINs and ISP's. */
+	mutex_lock(&vin->group->lock);
+	for (i = 0; i < RCAR_VIN_NUM; i++) {
+		struct media_pad *source_pad, *sink_pad;
+		struct media_entity *source, *sink;
+		unsigned int source_slot = i / 8;
+		unsigned int source_idx = i % 8 + 1;
+
+		if (!vin->group->vin[i])
+			continue;
+
+		/* Check that ISP is part of the group. */
+		if (!vin->group->remotes[source_slot].subdev)
+			continue;
+
+		source = &vin->group->remotes[source_slot].subdev->entity;
+		source_pad = &source->pads[source_idx];
+
+		sink = &vin->group->vin[i]->vdev.entity;
+		sink_pad = &sink->pads[0];
+
+		/* Skip if link already exists. */
+		if (media_entity_find_link(source_pad, sink_pad))
+			continue;
+
+		if (media_create_pad_link(source, source_idx, sink, 0,
+					  MEDIA_LNK_FL_ENABLED |
+					  MEDIA_LNK_FL_IMMUTABLE)) {
+			vin_err(vin, "Error adding link from %s to %s\n",
+				source->name, sink->name);
+			break;
+		}
+	}
+	mutex_unlock(&vin->group->lock);
+}
+
+static void rvin_isp_cleanup(struct rvin_dev *vin)
+{
+	rvin_group_notifier_cleanup(vin);
+	rvin_group_put(vin);
+	rvin_free_controls(vin);
+}
+
+static int rvin_isp_init(struct rvin_dev *vin)
+{
+	int ret;
+
+	vin->pad.flags = MEDIA_PAD_FL_SINK;
+	ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
+	if (ret)
+		return ret;
+
+	ret = rvin_create_controls(vin, NULL);
+	if (ret < 0)
+		return ret;
+
+	ret = rvin_group_get(vin, rvin_isp_setup_links, NULL);
+	if (ret)
+		goto err_controls;
+
+	ret = rvin_group_notifier_init(vin, 2, RVIN_ISP_MAX);
+	if (ret)
+		goto err_group;
+
+	return 0;
+err_group:
+	rvin_group_put(vin);
+err_controls:
+	rvin_free_controls(vin);
+
+	return ret;
+}
+
 /* -----------------------------------------------------------------------------
  * Suspend / Resume
  */
@@ -1378,6 +1459,15 @@ static const struct rvin_info rcar_info_r8a77995 = {
 	.routes = rcar_info_r8a77995_routes,
 };
 
+static const struct rvin_info rcar_info_r8a779a0 = {
+	.model = RCAR_GEN3,
+	.use_mc = true,
+	.use_isp = true,
+	.nv12 = true,
+	.max_width = 4096,
+	.max_height = 4096,
+};
+
 static const struct of_device_id rvin_of_id_table[] = {
 	{
 		.compatible = "renesas,vin-r8a774a1",
@@ -1435,6 +1525,10 @@ static const struct of_device_id rvin_of_id_table[] = {
 		.compatible = "renesas,vin-r8a77995",
 		.data = &rcar_info_r8a77995,
 	},
+	{
+		.compatible = "renesas,vin-r8a779a0",
+		.data = &rcar_info_r8a779a0,
+	},
 	{ /* Sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, rvin_of_id_table);
@@ -1483,7 +1577,9 @@ static int rcar_vin_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, vin);
 
-	if (vin->info->use_mc)
+	if (vin->info->use_isp)
+		ret = rvin_isp_init(vin);
+	else if (vin->info->use_mc)
 		ret = rvin_csi2_init(vin);
 	else
 		ret = rvin_parallel_init(vin);
@@ -1507,7 +1603,9 @@ static int rcar_vin_remove(struct platform_device *pdev)
 
 	rvin_v4l2_unregister(vin);
 
-	if (vin->info->use_mc)
+	if (vin->info->use_isp)
+		rvin_isp_cleanup(vin);
+	else if (vin->info->use_mc)
 		rvin_csi2_cleanup(vin);
 	else
 		rvin_parallel_cleanup(vin);
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index f30dafbdf61ca15f..24b3a51e524fcc72 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -783,16 +783,18 @@ static int rvin_setup(struct rvin_dev *vin)
 	/* Always update on field change */
 	vnmc |= VNMC_VUP;
 
-	/* If input and output use the same colorspace, use bypass mode */
-	if (input_is_yuv == output_is_yuv)
-		vnmc |= VNMC_BPS;
+	if (!vin->info->use_isp) {
+		/* If input and output use the same colorspace, use bypass mode */
+		if (input_is_yuv == output_is_yuv)
+			vnmc |= VNMC_BPS;
 
-	if (vin->info->model == RCAR_GEN3) {
-		/* Select between CSI-2 and parallel input */
-		if (vin->is_csi)
-			vnmc &= ~VNMC_DPINE;
-		else
-			vnmc |= VNMC_DPINE;
+		if (vin->info->model == RCAR_GEN3) {
+			/* Select between CSI-2 and parallel input */
+			if (vin->is_csi)
+				vnmc &= ~VNMC_DPINE;
+			else
+				vnmc |= VNMC_DPINE;
+		}
 	}
 
 	/* Progressive or interlaced mode */
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index f2c32d5ceb7f83d9..88728ebe470ecafd 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -29,7 +29,7 @@
 #define HW_BUFFER_MASK 0x7f
 
 /* Max number on VIN instances that can be in a system */
-#define RCAR_VIN_NUM 8
+#define RCAR_VIN_NUM 32
 
 struct rvin_group;
 
@@ -48,7 +48,17 @@ enum rvin_csi_id {
 	RVIN_CSI_MAX,
 };
 
-#define RVIN_REMOTES_MAX RVIN_CSI_MAX
+enum rvin_isp_id {
+	RVIN_ISP0,
+	RVIN_ISP1,
+	RVIN_ISP2,
+	RVIN_ISP4,
+	RVIN_ISP_MAX,
+};
+
+#define RVIN_REMOTES_MAX \
+	(((unsigned int)RVIN_CSI_MAX) > ((unsigned int)RVIN_ISP_MAX) ? \
+	 RVIN_CSI_MAX : RVIN_ISP_MAX)
 
 /**
  * enum rvin_dma_state - DMA states
@@ -158,6 +168,7 @@ struct rvin_group_route {
 struct rvin_info {
 	enum model_id model;
 	bool use_mc;
+	bool use_isp;
 	bool nv12;
 
 	unsigned int max_width;
-- 
2.31.1

Re: [PATCH 01/11] rcar-vin: Refactor controls creation for video device

[Jacopo Mondi]

Hi Niklas,

On Tue, Apr 13, 2021 at 08:02:43PM +0200, Niklas Söderlund wrote:
> The controls for the video device are created in different code paths
> depending on if the driver is using the media graph centric model (Gen3)
> or the device centric model (Gen2 and earlier). This have lead to code
> duplication that can be consolidated.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 82 +++++++++++----------
>  1 file changed, 45 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index cb3025992817d625..c798dc9409e4cdcd 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -405,6 +405,45 @@ static const struct v4l2_ctrl_ops rvin_ctrl_ops = {
>  	.s_ctrl = rvin_s_ctrl,
>  };
>
> +static void rvin_free_controls(struct rvin_dev *vin)
> +{
> +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> +	vin->vdev.ctrl_handler = NULL;
> +}
> +
> +static int rvin_create_controls(struct rvin_dev *vin, struct v4l2_subdev *subdev)
> +{
> +	int ret;
> +
> +	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* The VIN directly deals with alpha component. */
> +	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> +			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> +
> +	if (vin->ctrl_handler.error) {
> +		ret = vin->ctrl_handler.error;
> +		rvin_free_controls(vin);
> +		return ret;
> +	}
> +
> +	/* For the non-MC mode add controls from the subdevice. */
> +	if (subdev) {
> +		ret = v4l2_ctrl_add_handler(&vin->ctrl_handler,
> +					    subdev->ctrl_handler, NULL, true);
> +		if (ret < 0) {
> +			rvin_free_controls(vin);
> +			return ret;
> +		}
> +	}
> +
> +	vin->vdev.ctrl_handler = &vin->ctrl_handler;
> +
> +	return 0;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Async notifier
>   */
> @@ -490,28 +529,10 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
>  		return ret;
>
>  	/* Add the controls */
> -	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> +	ret = rvin_create_controls(vin, subdev);
>  	if (ret < 0)
>  		return ret;
>
> -	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> -			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> -
> -	if (vin->ctrl_handler.error) {
> -		ret = vin->ctrl_handler.error;
> -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> -		return ret;
> -	}
> -
> -	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler,
> -				    NULL, true);
> -	if (ret < 0) {
> -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> -		return ret;
> -	}
> -
> -	vin->vdev.ctrl_handler = &vin->ctrl_handler;
> -
>  	vin->parallel.subdev = subdev;
>
>  	return 0;
> @@ -522,10 +543,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
>  	rvin_v4l2_unregister(vin);
>  	vin->parallel.subdev = NULL;
>
> -	if (!vin->info->use_mc) {
> -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> -		vin->vdev.ctrl_handler = NULL;
> -	}
> +	if (!vin->info->use_mc)

I know it was there already, but give that rvin_parallel_notify_unbind()
is only registered for parallel, can this happen ?

Apart this small nit:
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
  j

> +		rvin_free_controls(vin);
>  }
>
>  static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
> @@ -935,21 +954,10 @@ static int rvin_mc_init(struct rvin_dev *vin)
>  	if (ret)
>  		rvin_group_put(vin);
>
> -	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1);
> +	ret = rvin_create_controls(vin, NULL);
>  	if (ret < 0)
>  		return ret;
>
> -	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> -			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> -
> -	if (vin->ctrl_handler.error) {
> -		ret = vin->ctrl_handler.error;
> -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> -		return ret;
> -	}
> -
> -	vin->vdev.ctrl_handler = &vin->ctrl_handler;
> -
>  	return ret;
>  }
>
> @@ -1446,7 +1454,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
>  	return 0;
>
>  error_group_unregister:
> -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> +	rvin_free_controls(vin);
>
>  	if (vin->info->use_mc) {
>  		mutex_lock(&vin->group->lock);
> @@ -1481,7 +1489,7 @@ static int rcar_vin_remove(struct platform_device *pdev)
>  		rvin_group_put(vin);
>  	}
>
> -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> +	rvin_free_controls(vin);
>
>  	rvin_dma_unregister(vin);
>
> --
> 2.31.1
>

Re: [PATCH 01/11] rcar-vin: Refactor controls creation for video device

[Jacopo Mondi]

Hi again,

On Tue, Apr 13, 2021 at 08:02:43PM +0200, Niklas Söderlund wrote:
> The controls for the video device are created in different code paths
> depending on if the driver is using the media graph centric model (Gen3)
> or the device centric model (Gen2 and earlier). This have lead to code
> duplication that can be consolidated.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 82 +++++++++++----------
>  1 file changed, 45 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index cb3025992817d625..c798dc9409e4cdcd 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -405,6 +405,45 @@ static const struct v4l2_ctrl_ops rvin_ctrl_ops = {
>  	.s_ctrl = rvin_s_ctrl,
>  };
>
> +static void rvin_free_controls(struct rvin_dev *vin)
> +{
> +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> +	vin->vdev.ctrl_handler = NULL;
> +}
> +
> +static int rvin_create_controls(struct rvin_dev *vin, struct v4l2_subdev *subdev)
> +{
> +	int ret;
> +
> +	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);

Not a big deal, but 16 because we have to reserve space for the
eventual subdevice controls ?

> +	if (ret < 0)
> +		return ret;
> +
> +	/* The VIN directly deals with alpha component. */
> +	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> +			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> +
> +	if (vin->ctrl_handler.error) {
> +		ret = vin->ctrl_handler.error;
> +		rvin_free_controls(vin);
> +		return ret;
> +	}
> +
> +	/* For the non-MC mode add controls from the subdevice. */
> +	if (subdev) {
> +		ret = v4l2_ctrl_add_handler(&vin->ctrl_handler,
> +					    subdev->ctrl_handler, NULL, true);
> +		if (ret < 0) {
> +			rvin_free_controls(vin);
> +			return ret;
> +		}
> +	}
> +
> +	vin->vdev.ctrl_handler = &vin->ctrl_handler;
> +
> +	return 0;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Async notifier
>   */
> @@ -490,28 +529,10 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
>  		return ret;
>
>  	/* Add the controls */
> -	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> +	ret = rvin_create_controls(vin, subdev);
>  	if (ret < 0)
>  		return ret;
>
> -	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> -			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> -
> -	if (vin->ctrl_handler.error) {
> -		ret = vin->ctrl_handler.error;
> -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> -		return ret;
> -	}
> -
> -	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler,
> -				    NULL, true);
> -	if (ret < 0) {
> -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> -		return ret;
> -	}
> -
> -	vin->vdev.ctrl_handler = &vin->ctrl_handler;
> -
>  	vin->parallel.subdev = subdev;
>
>  	return 0;
> @@ -522,10 +543,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
>  	rvin_v4l2_unregister(vin);
>  	vin->parallel.subdev = NULL;
>
> -	if (!vin->info->use_mc) {
> -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> -		vin->vdev.ctrl_handler = NULL;
> -	}
> +	if (!vin->info->use_mc)
> +		rvin_free_controls(vin);
>  }
>
>  static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
> @@ -935,21 +954,10 @@ static int rvin_mc_init(struct rvin_dev *vin)
>  	if (ret)
>  		rvin_group_put(vin);
>
> -	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1);
> +	ret = rvin_create_controls(vin, NULL);
>  	if (ret < 0)
>  		return ret;
>
> -	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> -			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> -
> -	if (vin->ctrl_handler.error) {
> -		ret = vin->ctrl_handler.error;
> -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> -		return ret;
> -	}
> -
> -	vin->vdev.ctrl_handler = &vin->ctrl_handler;
> -
>  	return ret;
>  }
>
> @@ -1446,7 +1454,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
>  	return 0;
>
>  error_group_unregister:
> -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> +	rvin_free_controls(vin);
>
>  	if (vin->info->use_mc) {
>  		mutex_lock(&vin->group->lock);
> @@ -1481,7 +1489,7 @@ static int rcar_vin_remove(struct platform_device *pdev)
>  		rvin_group_put(vin);
>  	}
>
> -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> +	rvin_free_controls(vin);
>
>  	rvin_dma_unregister(vin);
>
> --
> 2.31.1
>

Re: [PATCH 02/11] rcar-vin: Fix error paths for rvin_mc_init()

[Jacopo Mondi]

Hi Niklas,

On Tue, Apr 13, 2021 at 08:02:44PM +0200, Niklas Söderlund wrote:
> The error paths of rvin_mc_init() do not clean up properly, fix this.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index c798dc9409e4cdcd..d4932f7b42647ee1 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -946,17 +946,23 @@ static int rvin_mc_init(struct rvin_dev *vin)
>  	if (ret)
>  		return ret;
>
> +	ret = rvin_create_controls(vin, NULL);
> +	if (ret < 0)
> +		return ret;
> +
>  	ret = rvin_group_get(vin);
>  	if (ret)
> -		return ret;
> +		goto err_controls;
>
>  	ret = rvin_mc_parse_of_graph(vin);
>  	if (ret)
> -		rvin_group_put(vin);
> +		goto err_group;
>
> -	ret = rvin_create_controls(vin, NULL);
> -	if (ret < 0)
> -		return ret;
> +	return 0;

Empty line maybe ?

Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
  j
> +err_group:
> +	rvin_group_put(vin);
> +err_controls:
> +	rvin_free_controls(vin);
>
>  	return ret;
>  }
> --
> 2.31.1
>

Re: [PATCH 01/11] rcar-vin: Refactor controls creation for video device

[Niklas Söderlund]

Hi Jacopo,

On 2021-07-06 18:04:01 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Tue, Apr 13, 2021 at 08:02:43PM +0200, Niklas Söderlund wrote:
> > The controls for the video device are created in different code paths
> > depending on if the driver is using the media graph centric model (Gen3)
> > or the device centric model (Gen2 and earlier). This have lead to code
> > duplication that can be consolidated.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 82 +++++++++++----------
> >  1 file changed, 45 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index cb3025992817d625..c798dc9409e4cdcd 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -405,6 +405,45 @@ static const struct v4l2_ctrl_ops rvin_ctrl_ops = {
> >  	.s_ctrl = rvin_s_ctrl,
> >  };
> >
> > +static void rvin_free_controls(struct rvin_dev *vin)
> > +{
> > +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > +	vin->vdev.ctrl_handler = NULL;
> > +}
> > +
> > +static int rvin_create_controls(struct rvin_dev *vin, struct v4l2_subdev *subdev)
> > +{
> > +	int ret;
> > +
> > +	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* The VIN directly deals with alpha component. */
> > +	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> > +			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> > +
> > +	if (vin->ctrl_handler.error) {
> > +		ret = vin->ctrl_handler.error;
> > +		rvin_free_controls(vin);
> > +		return ret;
> > +	}
> > +
> > +	/* For the non-MC mode add controls from the subdevice. */
> > +	if (subdev) {
> > +		ret = v4l2_ctrl_add_handler(&vin->ctrl_handler,
> > +					    subdev->ctrl_handler, NULL, true);
> > +		if (ret < 0) {
> > +			rvin_free_controls(vin);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	vin->vdev.ctrl_handler = &vin->ctrl_handler;
> > +
> > +	return 0;
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Async notifier
> >   */
> > @@ -490,28 +529,10 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> >  		return ret;
> >
> >  	/* Add the controls */
> > -	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> > +	ret = rvin_create_controls(vin, subdev);
> >  	if (ret < 0)
> >  		return ret;
> >
> > -	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> > -			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> > -
> > -	if (vin->ctrl_handler.error) {
> > -		ret = vin->ctrl_handler.error;
> > -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > -		return ret;
> > -	}
> > -
> > -	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler,
> > -				    NULL, true);
> > -	if (ret < 0) {
> > -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > -		return ret;
> > -	}
> > -
> > -	vin->vdev.ctrl_handler = &vin->ctrl_handler;
> > -
> >  	vin->parallel.subdev = subdev;
> >
> >  	return 0;
> > @@ -522,10 +543,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
> >  	rvin_v4l2_unregister(vin);
> >  	vin->parallel.subdev = NULL;
> >
> > -	if (!vin->info->use_mc) {
> > -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > -		vin->vdev.ctrl_handler = NULL;
> > -	}
> > +	if (!vin->info->use_mc)
> 
> I know it was there already, but give that rvin_parallel_notify_unbind()
> is only registered for parallel, can this happen ?

Yes, on Gen2 where we don't use a media-graph.

> 
> Apart this small nit:
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> Thanks
>   j
> 
> > +		rvin_free_controls(vin);
> >  }
> >
> >  static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
> > @@ -935,21 +954,10 @@ static int rvin_mc_init(struct rvin_dev *vin)
> >  	if (ret)
> >  		rvin_group_put(vin);
> >
> > -	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1);
> > +	ret = rvin_create_controls(vin, NULL);
> >  	if (ret < 0)
> >  		return ret;
> >
> > -	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> > -			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> > -
> > -	if (vin->ctrl_handler.error) {
> > -		ret = vin->ctrl_handler.error;
> > -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > -		return ret;
> > -	}
> > -
> > -	vin->vdev.ctrl_handler = &vin->ctrl_handler;
> > -
> >  	return ret;
> >  }
> >
> > @@ -1446,7 +1454,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
> >  	return 0;
> >
> >  error_group_unregister:
> > -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > +	rvin_free_controls(vin);
> >
> >  	if (vin->info->use_mc) {
> >  		mutex_lock(&vin->group->lock);
> > @@ -1481,7 +1489,7 @@ static int rcar_vin_remove(struct platform_device *pdev)
> >  		rvin_group_put(vin);
> >  	}
> >
> > -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > +	rvin_free_controls(vin);
> >
> >  	rvin_dma_unregister(vin);
> >
> > --
> > 2.31.1
> >

-- 
Regards,
Niklas Söderlund

Re: [PATCH 01/11] rcar-vin: Refactor controls creation for video device

[Niklas Söderlund]

Hello :-)

On 2021-07-06 18:08:42 +0200, Jacopo Mondi wrote:
> Hi again,
> 
> On Tue, Apr 13, 2021 at 08:02:43PM +0200, Niklas Söderlund wrote:
> > The controls for the video device are created in different code paths
> > depending on if the driver is using the media graph centric model (Gen3)
> > or the device centric model (Gen2 and earlier). This have lead to code
> > duplication that can be consolidated.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 82 +++++++++++----------
> >  1 file changed, 45 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index cb3025992817d625..c798dc9409e4cdcd 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -405,6 +405,45 @@ static const struct v4l2_ctrl_ops rvin_ctrl_ops = {
> >  	.s_ctrl = rvin_s_ctrl,
> >  };
> >
> > +static void rvin_free_controls(struct rvin_dev *vin)
> > +{
> > +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > +	vin->vdev.ctrl_handler = NULL;
> > +}
> > +
> > +static int rvin_create_controls(struct rvin_dev *vin, struct v4l2_subdev *subdev)
> > +{
> > +	int ret;
> > +
> > +	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> 
> Not a big deal, but 16 because we have to reserve space for the
> eventual subdevice controls ?

Yes, again for Gen2 where the controls of the subdevice are exposed on 
the video device. IIRC the number 16 comes from the soc-camera ancestor 
for this driver.

> 
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* The VIN directly deals with alpha component. */
> > +	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> > +			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> > +
> > +	if (vin->ctrl_handler.error) {
> > +		ret = vin->ctrl_handler.error;
> > +		rvin_free_controls(vin);
> > +		return ret;
> > +	}
> > +
> > +	/* For the non-MC mode add controls from the subdevice. */
> > +	if (subdev) {
> > +		ret = v4l2_ctrl_add_handler(&vin->ctrl_handler,
> > +					    subdev->ctrl_handler, NULL, true);
> > +		if (ret < 0) {
> > +			rvin_free_controls(vin);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	vin->vdev.ctrl_handler = &vin->ctrl_handler;
> > +
> > +	return 0;
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Async notifier
> >   */
> > @@ -490,28 +529,10 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> >  		return ret;
> >
> >  	/* Add the controls */
> > -	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> > +	ret = rvin_create_controls(vin, subdev);
> >  	if (ret < 0)
> >  		return ret;
> >
> > -	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> > -			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> > -
> > -	if (vin->ctrl_handler.error) {
> > -		ret = vin->ctrl_handler.error;
> > -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > -		return ret;
> > -	}
> > -
> > -	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler,
> > -				    NULL, true);
> > -	if (ret < 0) {
> > -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > -		return ret;
> > -	}
> > -
> > -	vin->vdev.ctrl_handler = &vin->ctrl_handler;
> > -
> >  	vin->parallel.subdev = subdev;
> >
> >  	return 0;
> > @@ -522,10 +543,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
> >  	rvin_v4l2_unregister(vin);
> >  	vin->parallel.subdev = NULL;
> >
> > -	if (!vin->info->use_mc) {
> > -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > -		vin->vdev.ctrl_handler = NULL;
> > -	}
> > +	if (!vin->info->use_mc)
> > +		rvin_free_controls(vin);
> >  }
> >
> >  static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
> > @@ -935,21 +954,10 @@ static int rvin_mc_init(struct rvin_dev *vin)
> >  	if (ret)
> >  		rvin_group_put(vin);
> >
> > -	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1);
> > +	ret = rvin_create_controls(vin, NULL);
> >  	if (ret < 0)
> >  		return ret;
> >
> > -	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> > -			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> > -
> > -	if (vin->ctrl_handler.error) {
> > -		ret = vin->ctrl_handler.error;
> > -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > -		return ret;
> > -	}
> > -
> > -	vin->vdev.ctrl_handler = &vin->ctrl_handler;
> > -
> >  	return ret;
> >  }
> >
> > @@ -1446,7 +1454,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
> >  	return 0;
> >
> >  error_group_unregister:
> > -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > +	rvin_free_controls(vin);
> >
> >  	if (vin->info->use_mc) {
> >  		mutex_lock(&vin->group->lock);
> > @@ -1481,7 +1489,7 @@ static int rcar_vin_remove(struct platform_device *pdev)
> >  		rvin_group_put(vin);
> >  	}
> >
> > -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > +	rvin_free_controls(vin);
> >
> >  	rvin_dma_unregister(vin);
> >
> > --
> > 2.31.1
> >

-- 
Regards,
Niklas Söderlund

Re: [PATCH 03/11] rcar-vin: Improve async notifier cleanup paths

[Jacopo Mondi]

Hi Niklas

On Tue, Apr 13, 2021 at 08:02:45PM +0200, Niklas Söderlund wrote:
> The cleanup code for the async notifiers can be refactored to own
> functions to reduce code duplication and improve readability. While at
> it rename the CSI-2 initialization function _csi2_ instead of _mc_ to
> match.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 51 ++++++++++++---------
>  1 file changed, 30 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index d4932f7b42647ee1..da23d55aa72b7f0d 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -383,6 +383,16 @@ static void rvin_group_put(struct rvin_dev *vin)
>  	kref_put(&group->refcount, rvin_group_release);
>  }
>
> +static void rvin_group_notifier_cleanup(struct rvin_dev *vin)
> +{
> +	mutex_lock(&vin->group->lock);
> +	if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) {
> +		v4l2_async_notifier_unregister(&vin->group->notifier);
> +		v4l2_async_notifier_cleanup(&vin->group->notifier);
> +	}
> +	mutex_unlock(&vin->group->lock);
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Controls
>   */
> @@ -676,6 +686,12 @@ static int rvin_parallel_parse_of(struct rvin_dev *vin)
>  	return ret;
>  }
>
> +static void rvin_parallel_cleanup(struct rvin_dev *vin)
> +{
> +	v4l2_async_notifier_unregister(&vin->notifier);
> +	v4l2_async_notifier_cleanup(&vin->notifier);
> +}
> +
>  static int rvin_parallel_init(struct rvin_dev *vin)
>  {
>  	int ret;
> @@ -937,7 +953,16 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  	return 0;
>  }
>
> -static int rvin_mc_init(struct rvin_dev *vin)
> +static void rvin_csi2_cleanup(struct rvin_dev *vin)
> +{
> +	if (!vin->info->use_mc)
> +		return;
> +
> +	rvin_group_notifier_cleanup(vin);
> +	rvin_group_put(vin);
> +}
> +
> +static int rvin_csi2_init(struct rvin_dev *vin)
>  {
>  	int ret;
>
> @@ -1445,7 +1470,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, vin);
>
>  	if (vin->info->use_mc) {
> -		ret = rvin_mc_init(vin);
> +		ret = rvin_csi2_init(vin);
>  		if (ret)
>  			goto error_dma_unregister;
>  	}
> @@ -1458,20 +1483,9 @@ static int rcar_vin_probe(struct platform_device *pdev)
>  	pm_runtime_enable(&pdev->dev);
>
>  	return 0;
> -

Intentional ?

Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
   j

>  error_group_unregister:
>  	rvin_free_controls(vin);
> -
> -	if (vin->info->use_mc) {
> -		mutex_lock(&vin->group->lock);
> -		if (&vin->v4l2_dev == vin->group->notifier.v4l2_dev) {
> -			v4l2_async_notifier_unregister(&vin->group->notifier);
> -			v4l2_async_notifier_cleanup(&vin->group->notifier);
> -		}
> -		mutex_unlock(&vin->group->lock);
> -		rvin_group_put(vin);
> -	}
> -
> +	rvin_csi2_cleanup(vin);
>  error_dma_unregister:
>  	rvin_dma_unregister(vin);
>
> @@ -1486,14 +1500,9 @@ static int rcar_vin_remove(struct platform_device *pdev)
>
>  	rvin_v4l2_unregister(vin);
>
> -	v4l2_async_notifier_unregister(&vin->notifier);
> -	v4l2_async_notifier_cleanup(&vin->notifier);
> +	rvin_parallel_cleanup(vin);
>
> -	if (vin->info->use_mc) {
> -		v4l2_async_notifier_unregister(&vin->group->notifier);
> -		v4l2_async_notifier_cleanup(&vin->group->notifier);
> -		rvin_group_put(vin);
> -	}
> +	rvin_csi2_cleanup(vin);
>
>  	rvin_free_controls(vin);
>
> --
> 2.31.1
>

Re: [PATCH 04/11] rcar-vin: Improve reuse of parallel notifier

[Jacopo Mondi]

Hi Niklas,

On Tue, Apr 13, 2021 at 08:02:46PM +0200, Niklas Söderlund wrote:
> In preparation for adding a new media graph layout move the code reuse
> of the parallel notifier setup from probe directly to the current media
> graph initialization function. This is needed as there will be no
> parallel interface in the new graph layout.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 48 ++++++++++-----------
>  1 file changed, 22 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index da23d55aa72b7f0d..81574bf33116ad59 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -702,9 +702,8 @@ static int rvin_parallel_init(struct rvin_dev *vin)
>  	if (ret)
>  		return ret;
>
> -	/* If using mc, it's fine not to have any input registered. */
>  	if (!vin->parallel.asd)
> -		return vin->info->use_mc ? 0 : -ENODEV;
> +		return -ENODEV;

Nit: isn't it better to keep the error handling here ?

>
>  	vin_dbg(vin, "Found parallel subdevice %pOF\n",
>  		to_of_node(vin->parallel.asd->match.fwnode));
> @@ -955,11 +954,9 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>
>  static void rvin_csi2_cleanup(struct rvin_dev *vin)
>  {
> -	if (!vin->info->use_mc)
> -		return;
> -
>  	rvin_group_notifier_cleanup(vin);
>  	rvin_group_put(vin);
> +	rvin_free_controls(vin);
>  }
>
>  static int rvin_csi2_init(struct rvin_dev *vin)
> @@ -979,11 +976,18 @@ static int rvin_csi2_init(struct rvin_dev *vin)
>  	if (ret)
>  		goto err_controls;
>
> -	ret = rvin_mc_parse_of_graph(vin);
> -	if (ret)
> +	/* It's OK to not have a parallel subdevice. */
> +	ret = rvin_parallel_init(vin);
> +	if (ret && ret != -ENODEV)
>  		goto err_group;
>
> +	ret = rvin_mc_parse_of_graph(vin);
> +	if (ret)
> +		goto err_parallel;
> +
>  	return 0;
> +err_parallel:
> +	rvin_parallel_cleanup(vin);
>  err_group:
>  	rvin_group_put(vin);
>  err_controls:
> @@ -1469,27 +1473,20 @@ static int rcar_vin_probe(struct platform_device *pdev)
>
>  	platform_set_drvdata(pdev, vin);
>
> -	if (vin->info->use_mc) {
> +	if (vin->info->use_mc)
>  		ret = rvin_csi2_init(vin);
> -		if (ret)
> -			goto error_dma_unregister;
> -	}
> +	else
> +		ret = rvin_parallel_init(vin);
>
> -	ret = rvin_parallel_init(vin);
> -	if (ret)
> -		goto error_group_unregister;
> +	if (ret) {
> +		rvin_dma_unregister(vin);
> +		return ret;
> +	}
>
>  	pm_suspend_ignore_children(&pdev->dev, true);
>  	pm_runtime_enable(&pdev->dev);
>
>  	return 0;
> -error_group_unregister:
> -	rvin_free_controls(vin);
> -	rvin_csi2_cleanup(vin);
> -error_dma_unregister:
> -	rvin_dma_unregister(vin);
> -
> -	return ret;

This looks much much better and seems correct to me!

>  }
>
>  static int rcar_vin_remove(struct platform_device *pdev)
> @@ -1500,11 +1497,10 @@ static int rcar_vin_remove(struct platform_device *pdev)
>
>  	rvin_v4l2_unregister(vin);
>
> -	rvin_parallel_cleanup(vin);
> -
> -	rvin_csi2_cleanup(vin);
> -
> -	rvin_free_controls(vin);
> +	if (vin->info->use_mc)
> +		rvin_csi2_cleanup(vin);
> +	else
> +		rvin_parallel_cleanup(vin);

In the case use_mc == true but a parallel input was registered as well
this won't clean up the parallel notifier it seems.

Does it hurt to clean it up unconditionally ?

Thanks
  j
>
>  	rvin_dma_unregister(vin);
>
> --
> 2.31.1
>

Re: [PATCH 05/11] rcar-vin: Rename array storing subdevice information

[Jacopo Mondi]

Hi Niklas,

On Tue, Apr 13, 2021 at 08:02:47PM +0200, Niklas Söderlund wrote:
> The VIN group have always been connected to CSI-2 receivers and this
> have spilled over to the naming of the array storing the subdevice
> information. In preparation for connecting other types of subdevices
> rename the array to remotes.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Seems a rather good idea!

Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
  j
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 32 ++++++++++-----------
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  8 ++++--
>  2 files changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 81574bf33116ad59..cc980cad805c022c 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -49,16 +49,16 @@
>   */
>
>  /* group lock should be held when calling this function. */
> -static int rvin_group_entity_to_csi_id(struct rvin_group *group,
> -				       struct media_entity *entity)
> +static int rvin_group_entity_to_remote_id(struct rvin_group *group,
> +					  struct media_entity *entity)
>  {
>  	struct v4l2_subdev *sd;
>  	unsigned int i;
>
>  	sd = media_entity_to_v4l2_subdev(entity);
>
> -	for (i = 0; i < RVIN_CSI_MAX; i++)
> -		if (group->csi[i].subdev == sd)
> +	for (i = 0; i < RVIN_REMOTES_MAX; i++)
> +		if (group->remotes[i].subdev == sd)
>  			return i;
>
>  	return -ENODEV;
> @@ -163,14 +163,14 @@ static int rvin_group_link_notify(struct media_link *link, u32 flags,
>  		if (!csi_pad)
>  			continue;
>
> -		csi_id = rvin_group_entity_to_csi_id(group, csi_pad->entity);
> +		csi_id = rvin_group_entity_to_remote_id(group, csi_pad->entity);
>  		channel = rvin_group_csi_pad_to_channel(csi_pad->index);
>
>  		mask &= rvin_group_get_mask(group->vin[i], csi_id, channel);
>  	}
>
>  	/* Add the new link to the existing mask and check if it works. */
> -	csi_id = rvin_group_entity_to_csi_id(group, link->source->entity);
> +	csi_id = rvin_group_entity_to_remote_id(group, link->source->entity);
>
>  	if (csi_id == -ENODEV) {
>  		struct v4l2_subdev *sd;
> @@ -766,10 +766,10 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
>  			continue;
>
>  		/* Check that CSI-2 is part of the group. */
> -		if (!vin->group->csi[route->csi].subdev)
> +		if (!vin->group->remotes[route->csi].subdev)
>  			continue;
>
> -		source = &vin->group->csi[route->csi].subdev->entity;
> +		source = &vin->group->remotes[route->csi].subdev->entity;
>  		source_idx = rvin_group_csi_channel_to_pad(route->channel);
>  		source_pad = &source->pads[source_idx];
>
> @@ -806,10 +806,10 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
>  	mutex_lock(&vin->group->lock);
>
>  	for (i = 0; i < RVIN_CSI_MAX; i++) {
> -		if (vin->group->csi[i].asd != asd)
> +		if (vin->group->remotes[i].asd != asd)
>  			continue;
> -		vin->group->csi[i].subdev = NULL;
> -		vin_dbg(vin, "Unbind CSI-2 %s from slot %u\n", subdev->name, i);
> +		vin->group->remotes[i].subdev = NULL;
> +		vin_dbg(vin, "Unbind %s from slot %u\n", subdev->name, i);
>  		break;
>  	}
>
> @@ -828,10 +828,10 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
>  	mutex_lock(&vin->group->lock);
>
>  	for (i = 0; i < RVIN_CSI_MAX; i++) {
> -		if (vin->group->csi[i].asd != asd)
> +		if (vin->group->remotes[i].asd != asd)
>  			continue;
> -		vin->group->csi[i].subdev = subdev;
> -		vin_dbg(vin, "Bound CSI-2 %s to slot %u\n", subdev->name, i);
> +		vin->group->remotes[i].subdev = subdev;
> +		vin_dbg(vin, "Bound %s to slot %u\n", subdev->name, i);
>  		break;
>  	}
>
> @@ -883,7 +883,7 @@ static int rvin_mc_parse_of(struct rvin_dev *vin, unsigned int id)
>  		goto out;
>  	}
>
> -	vin->group->csi[vep.base.id].asd = asd;
> +	vin->group->remotes[vep.base.id].asd = asd;
>
>  	vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
>  		to_of_node(fwnode), vep.base.id);
> @@ -928,7 +928,7 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  			continue;
>
>  		for (id = 0; id < RVIN_CSI_MAX; id++) {
> -			if (vin->group->csi[id].asd)
> +			if (vin->group->remotes[id].asd)
>  				continue;
>
>  			ret = rvin_mc_parse_of(vin->group->vin[i], id);
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index b263ead4db2bfb08..39207aaf39ef9391 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -48,6 +48,8 @@ enum rvin_csi_id {
>  	RVIN_CSI_MAX,
>  };
>
> +#define RVIN_REMOTES_MAX RVIN_CSI_MAX
> +
>  /**
>   * enum rvin_dma_state - DMA states
>   * @STOPPED:   No operation in progress
> @@ -267,8 +269,8 @@ struct rvin_dev {
>   * @count:		number of enabled VIN instances found in DT
>   * @notifier:		group notifier for CSI-2 async subdevices
>   * @vin:		VIN instances which are part of the group
> - * @csi:		array of pairs of fwnode and subdev pointers
> - *			to all CSI-2 subdevices.
> + * @remotes:		array of pairs of fwnode and subdev pointers
> + *			to all remote subdevices.
>   */
>  struct rvin_group {
>  	struct kref refcount;
> @@ -283,7 +285,7 @@ struct rvin_group {
>  	struct {
>  		struct v4l2_async_subdev *asd;
>  		struct v4l2_subdev *subdev;
> -	} csi[RVIN_CSI_MAX];
> +	} remotes[RVIN_REMOTES_MAX];
>  };
>
>  int rvin_dma_register(struct rvin_dev *vin, int irq);
> --
> 2.31.1
>

Re: [PATCH 01/11] rcar-vin: Refactor controls creation for video device

[Jacopo Mondi]

Hi Niklas,

On Tue, Jul 06, 2021 at 06:16:04PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> On 2021-07-06 18:04:01 +0200, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Tue, Apr 13, 2021 at 08:02:43PM +0200, Niklas Söderlund wrote:
> > > The controls for the video device are created in different code paths
> > > depending on if the driver is using the media graph centric model (Gen3)
> > > or the device centric model (Gen2 and earlier). This have lead to code
> > > duplication that can be consolidated.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > >  drivers/media/platform/rcar-vin/rcar-core.c | 82 +++++++++++----------
> > >  1 file changed, 45 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > > index cb3025992817d625..c798dc9409e4cdcd 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > @@ -405,6 +405,45 @@ static const struct v4l2_ctrl_ops rvin_ctrl_ops = {
> > >  	.s_ctrl = rvin_s_ctrl,
> > >  };
> > >
> > > +static void rvin_free_controls(struct rvin_dev *vin)
> > > +{
> > > +	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > > +	vin->vdev.ctrl_handler = NULL;
> > > +}
> > > +
> > > +static int rvin_create_controls(struct rvin_dev *vin, struct v4l2_subdev *subdev)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	/* The VIN directly deals with alpha component. */
> > > +	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> > > +			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> > > +
> > > +	if (vin->ctrl_handler.error) {
> > > +		ret = vin->ctrl_handler.error;
> > > +		rvin_free_controls(vin);
> > > +		return ret;
> > > +	}
> > > +
> > > +	/* For the non-MC mode add controls from the subdevice. */
> > > +	if (subdev) {
> > > +		ret = v4l2_ctrl_add_handler(&vin->ctrl_handler,
> > > +					    subdev->ctrl_handler, NULL, true);
> > > +		if (ret < 0) {
> > > +			rvin_free_controls(vin);
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > > +	vin->vdev.ctrl_handler = &vin->ctrl_handler;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  /* -----------------------------------------------------------------------------
> > >   * Async notifier
> > >   */
> > > @@ -490,28 +529,10 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> > >  		return ret;
> > >
> > >  	/* Add the controls */
> > > -	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 16);
> > > +	ret = rvin_create_controls(vin, subdev);
> > >  	if (ret < 0)
> > >  		return ret;
> > >
> > > -	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> > > -			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> > > -
> > > -	if (vin->ctrl_handler.error) {
> > > -		ret = vin->ctrl_handler.error;
> > > -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > > -		return ret;
> > > -	}
> > > -
> > > -	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, subdev->ctrl_handler,
> > > -				    NULL, true);
> > > -	if (ret < 0) {
> > > -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > > -		return ret;
> > > -	}
> > > -
> > > -	vin->vdev.ctrl_handler = &vin->ctrl_handler;
> > > -
> > >  	vin->parallel.subdev = subdev;
> > >
> > >  	return 0;
> > > @@ -522,10 +543,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
> > >  	rvin_v4l2_unregister(vin);
> > >  	vin->parallel.subdev = NULL;
> > >
> > > -	if (!vin->info->use_mc) {
> > > -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > > -		vin->vdev.ctrl_handler = NULL;
> > > -	}
> > > +	if (!vin->info->use_mc)
> >
> > I know it was there already, but give that rvin_parallel_notify_unbind()
> > is only registered for parallel, can this happen ?
>
> Yes, on Gen2 where we don't use a media-graph.
>

Ah correct, for gen3 the controls are freed elsewhere, right!

Thanks for the clarification

> >
> > Apart this small nit:
> > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >
> > Thanks
> >   j
> >
> > > +		rvin_free_controls(vin);
> > >  }
> > >
> > >  static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
> > > @@ -935,21 +954,10 @@ static int rvin_mc_init(struct rvin_dev *vin)
> > >  	if (ret)
> > >  		rvin_group_put(vin);
> > >
> > > -	ret = v4l2_ctrl_handler_init(&vin->ctrl_handler, 1);
> > > +	ret = rvin_create_controls(vin, NULL);
> > >  	if (ret < 0)
> > >  		return ret;
> > >
> > > -	v4l2_ctrl_new_std(&vin->ctrl_handler, &rvin_ctrl_ops,
> > > -			  V4L2_CID_ALPHA_COMPONENT, 0, 255, 1, 255);
> > > -
> > > -	if (vin->ctrl_handler.error) {
> > > -		ret = vin->ctrl_handler.error;
> > > -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > > -		return ret;
> > > -	}
> > > -
> > > -	vin->vdev.ctrl_handler = &vin->ctrl_handler;
> > > -
> > >  	return ret;
> > >  }
> > >
> > > @@ -1446,7 +1454,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
> > >  	return 0;
> > >
> > >  error_group_unregister:
> > > -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > > +	rvin_free_controls(vin);
> > >
> > >  	if (vin->info->use_mc) {
> > >  		mutex_lock(&vin->group->lock);
> > > @@ -1481,7 +1489,7 @@ static int rcar_vin_remove(struct platform_device *pdev)
> > >  		rvin_group_put(vin);
> > >  	}
> > >
> > > -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > > +	rvin_free_controls(vin);
> > >
> > >  	rvin_dma_unregister(vin);
> > >
> > > --
> > > 2.31.1
> > >
>
> --
> Regards,
> Niklas Söderlund

Re: [PATCH 06/11] rcar-vin: Move group async notifier

[Jacopo Mondi]

Hi Niklas,

On Tue, Apr 13, 2021 at 08:02:48PM +0200, Niklas Söderlund wrote:
> The VIN group notifier code is intertwined with the media graph layout
> code for R-Car CSI-2 subdevices, this makes it hard to extend the group
> to also support the R-Car ISP channel selector.
>
> Before breaking the two concepts apart and extending it move the group
> code to its final location. There is no functional change and all
> functions are moved verbatim.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Seems there are no functional changes indeed!

Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
  j

> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 460 ++++++++++----------
>  1 file changed, 230 insertions(+), 230 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index cc980cad805c022c..d951f739b3a9a034 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -383,6 +383,176 @@ static void rvin_group_put(struct rvin_dev *vin)
>  	kref_put(&group->refcount, rvin_group_release);
>  }
>
> +static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
> +{
> +	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> +	const struct rvin_group_route *route;
> +	unsigned int i;
> +	int ret;
> +
> +	ret = media_device_register(&vin->group->mdev);
> +	if (ret)
> +		return ret;
> +
> +	ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);
> +	if (ret) {
> +		vin_err(vin, "Failed to register subdev nodes\n");
> +		return ret;
> +	}
> +
> +	/* Register all video nodes for the group. */
> +	for (i = 0; i < RCAR_VIN_NUM; i++) {
> +		if (vin->group->vin[i] &&
> +		    !video_is_registered(&vin->group->vin[i]->vdev)) {
> +			ret = rvin_v4l2_register(vin->group->vin[i]);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	/* Create all media device links between VINs and CSI-2's. */
> +	mutex_lock(&vin->group->lock);
> +	for (route = vin->info->routes; route->mask; route++) {
> +		struct media_pad *source_pad, *sink_pad;
> +		struct media_entity *source, *sink;
> +		unsigned int source_idx;
> +
> +		/* Check that VIN is part of the group. */
> +		if (!vin->group->vin[route->vin])
> +			continue;
> +
> +		/* Check that VIN' master is part of the group. */
> +		if (!vin->group->vin[rvin_group_id_to_master(route->vin)])
> +			continue;
> +
> +		/* Check that CSI-2 is part of the group. */
> +		if (!vin->group->remotes[route->csi].subdev)
> +			continue;
> +
> +		source = &vin->group->remotes[route->csi].subdev->entity;
> +		source_idx = rvin_group_csi_channel_to_pad(route->channel);
> +		source_pad = &source->pads[source_idx];
> +
> +		sink = &vin->group->vin[route->vin]->vdev.entity;
> +		sink_pad = &sink->pads[0];
> +
> +		/* Skip if link already exists. */
> +		if (media_entity_find_link(source_pad, sink_pad))
> +			continue;
> +
> +		ret = media_create_pad_link(source, source_idx, sink, 0, 0);
> +		if (ret) {
> +			vin_err(vin, "Error adding link from %s to %s\n",
> +				source->name, sink->name);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&vin->group->lock);
> +
> +	return ret;
> +}
> +
> +static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
> +				     struct v4l2_subdev *subdev,
> +				     struct v4l2_async_subdev *asd)
> +{
> +	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> +	unsigned int i;
> +
> +	for (i = 0; i < RCAR_VIN_NUM; i++)
> +		if (vin->group->vin[i])
> +			rvin_v4l2_unregister(vin->group->vin[i]);
> +
> +	mutex_lock(&vin->group->lock);
> +
> +	for (i = 0; i < RVIN_CSI_MAX; i++) {
> +		if (vin->group->remotes[i].asd != asd)
> +			continue;
> +		vin->group->remotes[i].subdev = NULL;
> +		vin_dbg(vin, "Unbind %s from slot %u\n", subdev->name, i);
> +		break;
> +	}
> +
> +	mutex_unlock(&vin->group->lock);
> +
> +	media_device_unregister(&vin->group->mdev);
> +}
> +
> +static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
> +				   struct v4l2_subdev *subdev,
> +				   struct v4l2_async_subdev *asd)
> +{
> +	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> +	unsigned int i;
> +
> +	mutex_lock(&vin->group->lock);
> +
> +	for (i = 0; i < RVIN_CSI_MAX; i++) {
> +		if (vin->group->remotes[i].asd != asd)
> +			continue;
> +		vin->group->remotes[i].subdev = subdev;
> +		vin_dbg(vin, "Bound %s to slot %u\n", subdev->name, i);
> +		break;
> +	}
> +
> +	mutex_unlock(&vin->group->lock);
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_async_notifier_operations rvin_group_notify_ops = {
> +	.bound = rvin_group_notify_bound,
> +	.unbind = rvin_group_notify_unbind,
> +	.complete = rvin_group_notify_complete,
> +};
> +
> +static int rvin_mc_parse_of(struct rvin_dev *vin, unsigned int id)
> +{
> +	struct fwnode_handle *ep, *fwnode;
> +	struct v4l2_fwnode_endpoint vep = {
> +		.bus_type = V4L2_MBUS_CSI2_DPHY,
> +	};
> +	struct v4l2_async_subdev *asd;
> +	int ret;
> +
> +	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 1, id, 0);
> +	if (!ep)
> +		return 0;
> +
> +	fwnode = fwnode_graph_get_remote_endpoint(ep);
> +	ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> +	fwnode_handle_put(ep);
> +	if (ret) {
> +		vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (!of_device_is_available(to_of_node(fwnode))) {
> +		vin_dbg(vin, "OF device %pOF disabled, ignoring\n",
> +			to_of_node(fwnode));
> +		ret = -ENOTCONN;
> +		goto out;
> +	}
> +
> +	asd = v4l2_async_notifier_add_fwnode_subdev(&vin->group->notifier,
> +						    fwnode,
> +						    struct v4l2_async_subdev);
> +	if (IS_ERR(asd)) {
> +		ret = PTR_ERR(asd);
> +		goto out;
> +	}
> +
> +	vin->group->remotes[vep.base.id].asd = asd;
> +
> +	vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
> +		to_of_node(fwnode), vep.base.id);
> +out:
> +	fwnode_handle_put(fwnode);
> +
> +	return ret;
> +}
> +
>  static void rvin_group_notifier_cleanup(struct rvin_dev *vin)
>  {
>  	mutex_lock(&vin->group->lock);
> @@ -393,6 +563,65 @@ static void rvin_group_notifier_cleanup(struct rvin_dev *vin)
>  	mutex_unlock(&vin->group->lock);
>  }
>
> +static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
> +{
> +	unsigned int count = 0, vin_mask = 0;
> +	unsigned int i, id;
> +	int ret;
> +
> +	mutex_lock(&vin->group->lock);
> +
> +	/* If not all VIN's are registered don't register the notifier. */
> +	for (i = 0; i < RCAR_VIN_NUM; i++) {
> +		if (vin->group->vin[i]) {
> +			count++;
> +			vin_mask |= BIT(i);
> +		}
> +	}
> +
> +	if (vin->group->count != count) {
> +		mutex_unlock(&vin->group->lock);
> +		return 0;
> +	}
> +
> +	mutex_unlock(&vin->group->lock);
> +
> +	v4l2_async_notifier_init(&vin->group->notifier);
> +
> +	/*
> +	 * Have all VIN's look for CSI-2 subdevices. Some subdevices will
> +	 * overlap but the parser function can handle it, so each subdevice
> +	 * will only be registered once with the group notifier.
> +	 */
> +	for (i = 0; i < RCAR_VIN_NUM; i++) {
> +		if (!(vin_mask & BIT(i)))
> +			continue;
> +
> +		for (id = 0; id < RVIN_CSI_MAX; id++) {
> +			if (vin->group->remotes[id].asd)
> +				continue;
> +
> +			ret = rvin_mc_parse_of(vin->group->vin[i], id);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	if (list_empty(&vin->group->notifier.asd_list))
> +		return 0;
> +
> +	vin->group->notifier.ops = &rvin_group_notify_ops;
> +	ret = v4l2_async_notifier_register(&vin->v4l2_dev,
> +					   &vin->group->notifier);
> +	if (ret < 0) {
> +		vin_err(vin, "Notifier registration failed\n");
> +		v4l2_async_notifier_cleanup(&vin->group->notifier);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Controls
>   */
> @@ -720,238 +949,9 @@ static int rvin_parallel_init(struct rvin_dev *vin)
>  }
>
>  /* -----------------------------------------------------------------------------
> - * Group async notifier
> + * CSI-2
>   */
>
> -static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
> -{
> -	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> -	const struct rvin_group_route *route;
> -	unsigned int i;
> -	int ret;
> -
> -	ret = media_device_register(&vin->group->mdev);
> -	if (ret)
> -		return ret;
> -
> -	ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);
> -	if (ret) {
> -		vin_err(vin, "Failed to register subdev nodes\n");
> -		return ret;
> -	}
> -
> -	/* Register all video nodes for the group. */
> -	for (i = 0; i < RCAR_VIN_NUM; i++) {
> -		if (vin->group->vin[i] &&
> -		    !video_is_registered(&vin->group->vin[i]->vdev)) {
> -			ret = rvin_v4l2_register(vin->group->vin[i]);
> -			if (ret)
> -				return ret;
> -		}
> -	}
> -
> -	/* Create all media device links between VINs and CSI-2's. */
> -	mutex_lock(&vin->group->lock);
> -	for (route = vin->info->routes; route->mask; route++) {
> -		struct media_pad *source_pad, *sink_pad;
> -		struct media_entity *source, *sink;
> -		unsigned int source_idx;
> -
> -		/* Check that VIN is part of the group. */
> -		if (!vin->group->vin[route->vin])
> -			continue;
> -
> -		/* Check that VIN' master is part of the group. */
> -		if (!vin->group->vin[rvin_group_id_to_master(route->vin)])
> -			continue;
> -
> -		/* Check that CSI-2 is part of the group. */
> -		if (!vin->group->remotes[route->csi].subdev)
> -			continue;
> -
> -		source = &vin->group->remotes[route->csi].subdev->entity;
> -		source_idx = rvin_group_csi_channel_to_pad(route->channel);
> -		source_pad = &source->pads[source_idx];
> -
> -		sink = &vin->group->vin[route->vin]->vdev.entity;
> -		sink_pad = &sink->pads[0];
> -
> -		/* Skip if link already exists. */
> -		if (media_entity_find_link(source_pad, sink_pad))
> -			continue;
> -
> -		ret = media_create_pad_link(source, source_idx, sink, 0, 0);
> -		if (ret) {
> -			vin_err(vin, "Error adding link from %s to %s\n",
> -				source->name, sink->name);
> -			break;
> -		}
> -	}
> -	mutex_unlock(&vin->group->lock);
> -
> -	return ret;
> -}
> -
> -static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
> -				     struct v4l2_subdev *subdev,
> -				     struct v4l2_async_subdev *asd)
> -{
> -	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> -	unsigned int i;
> -
> -	for (i = 0; i < RCAR_VIN_NUM; i++)
> -		if (vin->group->vin[i])
> -			rvin_v4l2_unregister(vin->group->vin[i]);
> -
> -	mutex_lock(&vin->group->lock);
> -
> -	for (i = 0; i < RVIN_CSI_MAX; i++) {
> -		if (vin->group->remotes[i].asd != asd)
> -			continue;
> -		vin->group->remotes[i].subdev = NULL;
> -		vin_dbg(vin, "Unbind %s from slot %u\n", subdev->name, i);
> -		break;
> -	}
> -
> -	mutex_unlock(&vin->group->lock);
> -
> -	media_device_unregister(&vin->group->mdev);
> -}
> -
> -static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
> -				   struct v4l2_subdev *subdev,
> -				   struct v4l2_async_subdev *asd)
> -{
> -	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> -	unsigned int i;
> -
> -	mutex_lock(&vin->group->lock);
> -
> -	for (i = 0; i < RVIN_CSI_MAX; i++) {
> -		if (vin->group->remotes[i].asd != asd)
> -			continue;
> -		vin->group->remotes[i].subdev = subdev;
> -		vin_dbg(vin, "Bound %s to slot %u\n", subdev->name, i);
> -		break;
> -	}
> -
> -	mutex_unlock(&vin->group->lock);
> -
> -	return 0;
> -}
> -
> -static const struct v4l2_async_notifier_operations rvin_group_notify_ops = {
> -	.bound = rvin_group_notify_bound,
> -	.unbind = rvin_group_notify_unbind,
> -	.complete = rvin_group_notify_complete,
> -};
> -
> -static int rvin_mc_parse_of(struct rvin_dev *vin, unsigned int id)
> -{
> -	struct fwnode_handle *ep, *fwnode;
> -	struct v4l2_fwnode_endpoint vep = {
> -		.bus_type = V4L2_MBUS_CSI2_DPHY,
> -	};
> -	struct v4l2_async_subdev *asd;
> -	int ret;
> -
> -	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 1, id, 0);
> -	if (!ep)
> -		return 0;
> -
> -	fwnode = fwnode_graph_get_remote_endpoint(ep);
> -	ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> -	fwnode_handle_put(ep);
> -	if (ret) {
> -		vin_err(vin, "Failed to parse %pOF\n", to_of_node(fwnode));
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -
> -	if (!of_device_is_available(to_of_node(fwnode))) {
> -		vin_dbg(vin, "OF device %pOF disabled, ignoring\n",
> -			to_of_node(fwnode));
> -		ret = -ENOTCONN;
> -		goto out;
> -	}
> -
> -	asd = v4l2_async_notifier_add_fwnode_subdev(&vin->group->notifier,
> -						    fwnode,
> -						    struct v4l2_async_subdev);
> -	if (IS_ERR(asd)) {
> -		ret = PTR_ERR(asd);
> -		goto out;
> -	}
> -
> -	vin->group->remotes[vep.base.id].asd = asd;
> -
> -	vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
> -		to_of_node(fwnode), vep.base.id);
> -out:
> -	fwnode_handle_put(fwnode);
> -
> -	return ret;
> -}
> -
> -static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
> -{
> -	unsigned int count = 0, vin_mask = 0;
> -	unsigned int i, id;
> -	int ret;
> -
> -	mutex_lock(&vin->group->lock);
> -
> -	/* If not all VIN's are registered don't register the notifier. */
> -	for (i = 0; i < RCAR_VIN_NUM; i++) {
> -		if (vin->group->vin[i]) {
> -			count++;
> -			vin_mask |= BIT(i);
> -		}
> -	}
> -
> -	if (vin->group->count != count) {
> -		mutex_unlock(&vin->group->lock);
> -		return 0;
> -	}
> -
> -	mutex_unlock(&vin->group->lock);
> -
> -	v4l2_async_notifier_init(&vin->group->notifier);
> -
> -	/*
> -	 * Have all VIN's look for CSI-2 subdevices. Some subdevices will
> -	 * overlap but the parser function can handle it, so each subdevice
> -	 * will only be registered once with the group notifier.
> -	 */
> -	for (i = 0; i < RCAR_VIN_NUM; i++) {
> -		if (!(vin_mask & BIT(i)))
> -			continue;
> -
> -		for (id = 0; id < RVIN_CSI_MAX; id++) {
> -			if (vin->group->remotes[id].asd)
> -				continue;
> -
> -			ret = rvin_mc_parse_of(vin->group->vin[i], id);
> -			if (ret)
> -				return ret;
> -		}
> -	}
> -
> -	if (list_empty(&vin->group->notifier.asd_list))
> -		return 0;
> -
> -	vin->group->notifier.ops = &rvin_group_notify_ops;
> -	ret = v4l2_async_notifier_register(&vin->v4l2_dev,
> -					   &vin->group->notifier);
> -	if (ret < 0) {
> -		vin_err(vin, "Notifier registration failed\n");
> -		v4l2_async_notifier_cleanup(&vin->group->notifier);
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  static void rvin_csi2_cleanup(struct rvin_dev *vin)
>  {
>  	rvin_group_notifier_cleanup(vin);
> --
> 2.31.1
>

Re: [PATCH 04/11] rcar-vin: Improve reuse of parallel notifier

[Niklas Söderlund]

Hi Jacopo,

Thanks for your feedback.

On 2021-07-06 18:51:41 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Tue, Apr 13, 2021 at 08:02:46PM +0200, Niklas Söderlund wrote:
> > In preparation for adding a new media graph layout move the code reuse
> > of the parallel notifier setup from probe directly to the current media
> > graph initialization function. This is needed as there will be no
> > parallel interface in the new graph layout.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 48 ++++++++++-----------
> >  1 file changed, 22 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index da23d55aa72b7f0d..81574bf33116ad59 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -702,9 +702,8 @@ static int rvin_parallel_init(struct rvin_dev *vin)
> >  	if (ret)
> >  		return ret;
> >
> > -	/* If using mc, it's fine not to have any input registered. */
> >  	if (!vin->parallel.asd)
> > -		return vin->info->use_mc ? 0 : -ENODEV;
> > +		return -ENODEV;
> 
> Nit: isn't it better to keep the error handling here ?

I'm trying to reduce the number of use_mc checks and if needed only keep 
it in code paths where it's relevant. I like moving it to the _csi2_ 
functions. But I agree _csi2_ is a bad prefix, it should likely be 
renamed somewhen in the future as it now really means _gen3_ minus v3u 
:-)

> 
> >
> >  	vin_dbg(vin, "Found parallel subdevice %pOF\n",
> >  		to_of_node(vin->parallel.asd->match.fwnode));
> > @@ -955,11 +954,9 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
> >
> >  static void rvin_csi2_cleanup(struct rvin_dev *vin)
> >  {
> > -	if (!vin->info->use_mc)
> > -		return;
> > -
> >  	rvin_group_notifier_cleanup(vin);
> >  	rvin_group_put(vin);
> > +	rvin_free_controls(vin);
> >  }
> >
> >  static int rvin_csi2_init(struct rvin_dev *vin)
> > @@ -979,11 +976,18 @@ static int rvin_csi2_init(struct rvin_dev *vin)
> >  	if (ret)
> >  		goto err_controls;
> >
> > -	ret = rvin_mc_parse_of_graph(vin);
> > -	if (ret)
> > +	/* It's OK to not have a parallel subdevice. */
> > +	ret = rvin_parallel_init(vin);
> > +	if (ret && ret != -ENODEV)
> >  		goto err_group;
> >
> > +	ret = rvin_mc_parse_of_graph(vin);
> > +	if (ret)
> > +		goto err_parallel;
> > +
> >  	return 0;
> > +err_parallel:
> > +	rvin_parallel_cleanup(vin);
> >  err_group:
> >  	rvin_group_put(vin);
> >  err_controls:
> > @@ -1469,27 +1473,20 @@ static int rcar_vin_probe(struct platform_device *pdev)
> >
> >  	platform_set_drvdata(pdev, vin);
> >
> > -	if (vin->info->use_mc) {
> > +	if (vin->info->use_mc)
> >  		ret = rvin_csi2_init(vin);
> > -		if (ret)
> > -			goto error_dma_unregister;
> > -	}
> > +	else
> > +		ret = rvin_parallel_init(vin);
> >
> > -	ret = rvin_parallel_init(vin);
> > -	if (ret)
> > -		goto error_group_unregister;
> > +	if (ret) {
> > +		rvin_dma_unregister(vin);
> > +		return ret;
> > +	}
> >
> >  	pm_suspend_ignore_children(&pdev->dev, true);
> >  	pm_runtime_enable(&pdev->dev);
> >
> >  	return 0;
> > -error_group_unregister:
> > -	rvin_free_controls(vin);
> > -	rvin_csi2_cleanup(vin);
> > -error_dma_unregister:
> > -	rvin_dma_unregister(vin);
> > -
> > -	return ret;
> 
> This looks much much better and seems correct to me!
> 
> >  }
> >
> >  static int rcar_vin_remove(struct platform_device *pdev)
> > @@ -1500,11 +1497,10 @@ static int rcar_vin_remove(struct platform_device *pdev)
> >
> >  	rvin_v4l2_unregister(vin);
> >
> > -	rvin_parallel_cleanup(vin);
> > -
> > -	rvin_csi2_cleanup(vin);
> > -
> > -	rvin_free_controls(vin);
> > +	if (vin->info->use_mc)
> > +		rvin_csi2_cleanup(vin);
> > +	else
> > +		rvin_parallel_cleanup(vin);
> 
> In the case use_mc == true but a parallel input was registered as well
> this won't clean up the parallel notifier it seems.

Good catch! rvin_parallel_cleanup() should be called from 
rvin_csi2_cleanup() as rvin_parallel_init() is called form 
rvin_csi2_init(). I will fix.

> 
> Does it hurt to clean it up unconditionally ?
> 
> Thanks
>   j
> >
> >  	rvin_dma_unregister(vin);
> >
> > --
> > 2.31.1
> >

-- 
Regards,
Niklas Söderlund

Re: [PATCH 07/11] rcar-vin: Extend group notifier DT parser to work with any port

[Jacopo Mondi]

Hi Niklas,

On Tue, Apr 13, 2021 at 08:02:49PM +0200, Niklas Söderlund wrote:
> The R-Car VIN group notifier will be extend to support a new group of
> subdevices, the R-Car ISP channel selector in addition to the existing
> R-Car CSI-2 receiver subdevices.
>
> The existing DT parsing code can be reused if the port and max number of
> endpoints are provided as parameters instead of being hard-coded. While
> at it align the group notifier parser function names with the rest of
> the driver.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index d951f739b3a9a034..2628637084ae2aa9 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -506,7 +506,8 @@ static const struct v4l2_async_notifier_operations rvin_group_notify_ops = {
>  	.complete = rvin_group_notify_complete,
>  };
>
> -static int rvin_mc_parse_of(struct rvin_dev *vin, unsigned int id)
> +static int rvin_group_parse_of(struct rvin_dev *vin, unsigned int port,
> +			       unsigned int id)
>  {
>  	struct fwnode_handle *ep, *fwnode;
>  	struct v4l2_fwnode_endpoint vep = {
> @@ -515,7 +516,7 @@ static int rvin_mc_parse_of(struct rvin_dev *vin, unsigned int id)
>  	struct v4l2_async_subdev *asd;
>  	int ret;
>
> -	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 1, id, 0);
> +	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), port, id, 0);
>  	if (!ep)
>  		return 0;
>
> @@ -563,7 +564,8 @@ static void rvin_group_notifier_cleanup(struct rvin_dev *vin)
>  	mutex_unlock(&vin->group->lock);
>  }
>
> -static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
> +static int rvin_group_notifier_init(struct rvin_dev *vin, unsigned int port,
> +				    unsigned int max_id)
>  {
>  	unsigned int count = 0, vin_mask = 0;
>  	unsigned int i, id;
> @@ -589,19 +591,18 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  	v4l2_async_notifier_init(&vin->group->notifier);
>
>  	/*
> -	 * Have all VIN's look for CSI-2 subdevices. Some subdevices will
> -	 * overlap but the parser function can handle it, so each subdevice
> -	 * will only be registered once with the group notifier.
> +	 * Some subdevices may overlap but the parser function can handle it and
> +	 * each subdevice will only be registered once with the group notifier.
>  	 */
>  	for (i = 0; i < RCAR_VIN_NUM; i++) {
>  		if (!(vin_mask & BIT(i)))
>  			continue;
>
> -		for (id = 0; id < RVIN_CSI_MAX; id++) {
> +		for (id = 0; id < max_id; id++) {
>  			if (vin->group->remotes[id].asd)
>  				continue;
>
> -			ret = rvin_mc_parse_of(vin->group->vin[i], id);
> +			ret = rvin_group_parse_of(vin->group->vin[i], port, id);
>  			if (ret)
>  				return ret;
>  		}
> @@ -981,7 +982,7 @@ static int rvin_csi2_init(struct rvin_dev *vin)
>  	if (ret && ret != -ENODEV)
>  		goto err_group;
>
> -	ret = rvin_mc_parse_of_graph(vin);
> +	ret = rvin_group_notifier_init(vin, 1, RVIN_CSI_MAX);

You know, I would define RVIN_CSI2_ENDPOINT_NUM or similar.
Small nit apart, it's good to be able to reuse the same routines!

Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
   j

>  	if (ret)
>  		goto err_parallel;
>
> --
> 2.31.1
>

Re: [PATCH 08/11] rcar-vin: Create a callback to setup media links

[Jacopo Mondi]

Hi Niklas,

On Tue, Apr 13, 2021 at 08:02:50PM +0200, Niklas Söderlund wrote:
> New IP versions will have different media graphs and require a different
> link setup. Breakout the specific link setup to a callback that are
> associated with the group.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 97 ++++++++++++---------
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  3 +
>  2 files changed, 57 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 2628637084ae2aa9..d03ecd443b01c2b3 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -247,7 +247,8 @@ static void rvin_group_cleanup(struct rvin_group *group)
>  	mutex_destroy(&group->lock);
>  }
>
> -static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
> +static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin,
> +			   void (*link_setup)(struct rvin_dev *))
>  {
>  	struct media_device *mdev = &group->mdev;
>  	const struct of_device_id *match;
> @@ -263,6 +264,8 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
>
>  	vin_dbg(vin, "found %u enabled VIN's in DT", group->count);
>
> +	group->link_setup = link_setup;
> +
>  	mdev->dev = vin->dev;
>  	mdev->ops = &rvin_media_ops;
>
> @@ -295,7 +298,8 @@ static void rvin_group_release(struct kref *kref)
>  	mutex_unlock(&rvin_group_lock);
>  }
>
> -static int rvin_group_get(struct rvin_dev *vin)
> +static int rvin_group_get(struct rvin_dev *vin,
> +			  void (*link_setup)(struct rvin_dev *))
>  {
>  	struct rvin_group *group;
>  	u32 id;
> @@ -327,7 +331,7 @@ static int rvin_group_get(struct rvin_dev *vin)
>  			goto err_group;
>  		}
>
> -		ret = rvin_group_init(group, vin);
> +		ret = rvin_group_init(group, vin, link_setup);
>  		if (ret) {
>  			kfree(group);
>  			vin_err(vin, "Failed to initialize group\n");
> @@ -386,7 +390,6 @@ static void rvin_group_put(struct rvin_dev *vin)
>  static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
>  {
>  	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> -	const struct rvin_group_route *route;
>  	unsigned int i;
>  	int ret;
>
> @@ -410,44 +413,7 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
>  		}
>  	}
>
> -	/* Create all media device links between VINs and CSI-2's. */
> -	mutex_lock(&vin->group->lock);
> -	for (route = vin->info->routes; route->mask; route++) {
> -		struct media_pad *source_pad, *sink_pad;
> -		struct media_entity *source, *sink;
> -		unsigned int source_idx;
> -
> -		/* Check that VIN is part of the group. */
> -		if (!vin->group->vin[route->vin])
> -			continue;
> -
> -		/* Check that VIN' master is part of the group. */
> -		if (!vin->group->vin[rvin_group_id_to_master(route->vin)])
> -			continue;
> -
> -		/* Check that CSI-2 is part of the group. */
> -		if (!vin->group->remotes[route->csi].subdev)
> -			continue;
> -
> -		source = &vin->group->remotes[route->csi].subdev->entity;
> -		source_idx = rvin_group_csi_channel_to_pad(route->channel);
> -		source_pad = &source->pads[source_idx];
> -
> -		sink = &vin->group->vin[route->vin]->vdev.entity;
> -		sink_pad = &sink->pads[0];
> -
> -		/* Skip if link already exists. */
> -		if (media_entity_find_link(source_pad, sink_pad))
> -			continue;
> -
> -		ret = media_create_pad_link(source, source_idx, sink, 0, 0);
> -		if (ret) {
> -			vin_err(vin, "Error adding link from %s to %s\n",
> -				source->name, sink->name);
> -			break;
> -		}
> -	}
> -	mutex_unlock(&vin->group->lock);
> +	vin->group->link_setup(vin);

Compared to the previuous version, we've lost the ability to return
erros on failures in link creation. Is it intentional ?

Thanks
   j

>
>  	return ret;
>  }
> @@ -953,6 +919,51 @@ static int rvin_parallel_init(struct rvin_dev *vin)
>   * CSI-2
>   */
>
> +static void rvin_csi2_setup_links(struct rvin_dev *vin)
> +{
> +	const struct rvin_group_route *route;
> +	int ret;
> +
> +	/* Create all media device links between VINs and CSI-2's. */
> +	mutex_lock(&vin->group->lock);
> +	for (route = vin->info->routes; route->mask; route++) {
> +		struct media_pad *source_pad, *sink_pad;
> +		struct media_entity *source, *sink;
> +		unsigned int source_idx;
> +
> +		/* Check that VIN is part of the group. */
> +		if (!vin->group->vin[route->vin])
> +			continue;
> +
> +		/* Check that VIN' master is part of the group. */
> +		if (!vin->group->vin[rvin_group_id_to_master(route->vin)])
> +			continue;
> +
> +		/* Check that CSI-2 is part of the group. */
> +		if (!vin->group->remotes[route->csi].subdev)
> +			continue;
> +
> +		source = &vin->group->remotes[route->csi].subdev->entity;
> +		source_idx = rvin_group_csi_channel_to_pad(route->channel);
> +		source_pad = &source->pads[source_idx];
> +
> +		sink = &vin->group->vin[route->vin]->vdev.entity;
> +		sink_pad = &sink->pads[0];
> +
> +		/* Skip if link already exists. */
> +		if (media_entity_find_link(source_pad, sink_pad))
> +			continue;
> +
> +		ret = media_create_pad_link(source, source_idx, sink, 0, 0);
> +		if (ret) {
> +			vin_err(vin, "Error adding link from %s to %s\n",
> +				source->name, sink->name);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&vin->group->lock);
> +}
> +
>  static void rvin_csi2_cleanup(struct rvin_dev *vin)
>  {
>  	rvin_group_notifier_cleanup(vin);
> @@ -973,7 +984,7 @@ static int rvin_csi2_init(struct rvin_dev *vin)
>  	if (ret < 0)
>  		return ret;
>
> -	ret = rvin_group_get(vin);
> +	ret = rvin_group_get(vin, rvin_csi2_setup_links);
>  	if (ret)
>  		goto err_controls;
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 39207aaf39ef9391..f2c32d5ceb7f83d9 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -269,6 +269,7 @@ struct rvin_dev {
>   * @count:		number of enabled VIN instances found in DT
>   * @notifier:		group notifier for CSI-2 async subdevices
>   * @vin:		VIN instances which are part of the group
> + * @link_setup:		Callback to create all links for the media graph
>   * @remotes:		array of pairs of fwnode and subdev pointers
>   *			to all remote subdevices.
>   */
> @@ -282,6 +283,8 @@ struct rvin_group {
>  	struct v4l2_async_notifier notifier;
>  	struct rvin_dev *vin[RCAR_VIN_NUM];
>
> +	void (*link_setup)(struct rvin_dev *vin);
> +
>  	struct {
>  		struct v4l2_async_subdev *asd;
>  		struct v4l2_subdev *subdev;
> --
> 2.31.1
>

Re: [PATCH 09/11] rcar-vin: Specify media device ops at group creation time

[Jacopo Mondi]

Hi Niklas,

On Tue, Apr 13, 2021 at 08:02:51PM +0200, Niklas Söderlund wrote:
> The media device operations structure will be different deepening on

s/deepening/depending

> what media graph layout is used. Instead of hard-coding the R-Car CSI-2
> layout allow the operations to be passed as an argument.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index d03ecd443b01c2b3..28bf9e8f19a1a27b 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -248,7 +248,8 @@ static void rvin_group_cleanup(struct rvin_group *group)
>  }
>
>  static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin,
> -			   void (*link_setup)(struct rvin_dev *))
> +			   void (*link_setup)(struct rvin_dev *),
> +			   const struct media_device_ops *ops)
>  {
>  	struct media_device *mdev = &group->mdev;
>  	const struct of_device_id *match;
> @@ -267,7 +268,7 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin,
>  	group->link_setup = link_setup;
>
>  	mdev->dev = vin->dev;
> -	mdev->ops = &rvin_media_ops;
> +	mdev->ops = ops;
>
>  	match = of_match_node(vin->dev->driver->of_match_table,
>  			      vin->dev->of_node);
> @@ -299,7 +300,8 @@ static void rvin_group_release(struct kref *kref)
>  }
>
>  static int rvin_group_get(struct rvin_dev *vin,
> -			  void (*link_setup)(struct rvin_dev *))
> +			  void (*link_setup)(struct rvin_dev *),
> +			  const struct media_device_ops *ops)
>  {
>  	struct rvin_group *group;
>  	u32 id;
> @@ -331,7 +333,7 @@ static int rvin_group_get(struct rvin_dev *vin,
>  			goto err_group;
>  		}
>
> -		ret = rvin_group_init(group, vin, link_setup);
> +		ret = rvin_group_init(group, vin, link_setup, ops);
>  		if (ret) {
>  			kfree(group);
>  			vin_err(vin, "Failed to initialize group\n");
> @@ -984,7 +986,7 @@ static int rvin_csi2_init(struct rvin_dev *vin)
>  	if (ret < 0)
>  		return ret;
>
> -	ret = rvin_group_get(vin, rvin_csi2_setup_links);
> +	ret = rvin_group_get(vin, rvin_csi2_setup_links, &rvin_media_ops);

I would continue the effort of using the csi2_ prefix for CSI-2
related fields. Just a nit, the change looks good

Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
  j

>  	if (ret)
>  		goto err_controls;
>
> --
> 2.31.1
>

Re: [PATCH 10/11] rcar-vin: Move and rename CSI-2 link notifications

[Jacopo Mondi]

Hi Niklas,

On Tue, Apr 13, 2021 at 08:02:52PM +0200, Niklas Söderlund wrote:
> The CSI-2 link notifications are no longer the only option for the VIN
> group. Change the symbol prefix to rvin_csi2_ for all CSI-2 specific

Ah, here you go :)

> code and move the link notification code to the correct section to not

s/to not/not to/

> mix it with the soon to be added R-Car ISP channel selector notification
> helpers.
>
> There is no functional change and apart from the symbol prefix change
> all functions are moved verbatim.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 360 ++++++++++----------
>  1 file changed, 178 insertions(+), 182 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 28bf9e8f19a1a27b..763be02f507e6f3f 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -44,187 +44,6 @@
>
>  #define v4l2_dev_to_vin(d)	container_of(d, struct rvin_dev, v4l2_dev)
>
> -/* -----------------------------------------------------------------------------
> - * Media Controller link notification
> - */
> -
> -/* group lock should be held when calling this function. */
> -static int rvin_group_entity_to_remote_id(struct rvin_group *group,
> -					  struct media_entity *entity)
> -{
> -	struct v4l2_subdev *sd;
> -	unsigned int i;
> -
> -	sd = media_entity_to_v4l2_subdev(entity);
> -
> -	for (i = 0; i < RVIN_REMOTES_MAX; i++)
> -		if (group->remotes[i].subdev == sd)
> -			return i;
> -
> -	return -ENODEV;
> -}
> -
> -static unsigned int rvin_group_get_mask(struct rvin_dev *vin,
> -					enum rvin_csi_id csi_id,
> -					unsigned char channel)
> -{
> -	const struct rvin_group_route *route;
> -	unsigned int mask = 0;
> -
> -	for (route = vin->info->routes; route->mask; route++) {
> -		if (route->vin == vin->id &&
> -		    route->csi == csi_id &&
> -		    route->channel == channel) {
> -			vin_dbg(vin,
> -				"Adding route: vin: %d csi: %d channel: %d\n",
> -				route->vin, route->csi, route->channel);
> -			mask |= route->mask;
> -		}
> -	}
> -
> -	return mask;
> -}
> -
> -/*
> - * Link setup for the links between a VIN and a CSI-2 receiver is a bit
> - * complex. The reason for this is that the register controlling routing
> - * is not present in each VIN instance. There are special VINs which
> - * control routing for themselves and other VINs. There are not many
> - * different possible links combinations that can be enabled at the same
> - * time, therefor all already enabled links which are controlled by a
> - * master VIN need to be taken into account when making the decision
> - * if a new link can be enabled or not.
> - *
> - * 1. Find out which VIN the link the user tries to enable is connected to.
> - * 2. Lookup which master VIN controls the links for this VIN.
> - * 3. Start with a bitmask with all bits set.
> - * 4. For each previously enabled link from the master VIN bitwise AND its
> - *    route mask (see documentation for mask in struct rvin_group_route)
> - *    with the bitmask.
> - * 5. Bitwise AND the mask for the link the user tries to enable to the bitmask.
> - * 6. If the bitmask is not empty at this point the new link can be enabled
> - *    while keeping all previous links enabled. Update the CHSEL value of the
> - *    master VIN and inform the user that the link could be enabled.
> - *
> - * Please note that no link can be enabled if any VIN in the group is
> - * currently open.
> - */
> -static int rvin_group_link_notify(struct media_link *link, u32 flags,
> -				  unsigned int notification)
> -{
> -	struct rvin_group *group = container_of(link->graph_obj.mdev,
> -						struct rvin_group, mdev);
> -	unsigned int master_id, channel, mask_new, i;
> -	unsigned int mask = ~0;
> -	struct media_entity *entity;
> -	struct video_device *vdev;
> -	struct media_pad *csi_pad;
> -	struct rvin_dev *vin = NULL;
> -	int csi_id, ret;
> -
> -	ret = v4l2_pipeline_link_notify(link, flags, notification);
> -	if (ret)
> -		return ret;
> -
> -	/* Only care about link enablement for VIN nodes. */
> -	if (!(flags & MEDIA_LNK_FL_ENABLED) ||
> -	    !is_media_entity_v4l2_video_device(link->sink->entity))
> -		return 0;
> -
> -	/*
> -	 * Don't allow link changes if any entity in the graph is
> -	 * streaming, modifying the CHSEL register fields can disrupt
> -	 * running streams.
> -	 */
> -	media_device_for_each_entity(entity, &group->mdev)
> -		if (entity->stream_count)
> -			return -EBUSY;
> -
> -	mutex_lock(&group->lock);
> -
> -	/* Find the master VIN that controls the routes. */
> -	vdev = media_entity_to_video_device(link->sink->entity);
> -	vin = container_of(vdev, struct rvin_dev, vdev);
> -	master_id = rvin_group_id_to_master(vin->id);
> -
> -	if (WARN_ON(!group->vin[master_id])) {
> -		ret = -ENODEV;
> -		goto out;
> -	}
> -
> -	/* Build a mask for already enabled links. */
> -	for (i = master_id; i < master_id + 4; i++) {
> -		if (!group->vin[i])
> -			continue;
> -
> -		/* Get remote CSI-2, if any. */
> -		csi_pad = media_entity_remote_pad(
> -				&group->vin[i]->vdev.entity.pads[0]);
> -		if (!csi_pad)
> -			continue;
> -
> -		csi_id = rvin_group_entity_to_remote_id(group, csi_pad->entity);
> -		channel = rvin_group_csi_pad_to_channel(csi_pad->index);
> -
> -		mask &= rvin_group_get_mask(group->vin[i], csi_id, channel);
> -	}
> -
> -	/* Add the new link to the existing mask and check if it works. */
> -	csi_id = rvin_group_entity_to_remote_id(group, link->source->entity);
> -
> -	if (csi_id == -ENODEV) {
> -		struct v4l2_subdev *sd;
> -
> -		/*
> -		 * Make sure the source entity subdevice is registered as
> -		 * a parallel input of one of the enabled VINs if it is not
> -		 * one of the CSI-2 subdevices.
> -		 *
> -		 * No hardware configuration required for parallel inputs,
> -		 * we can return here.
> -		 */
> -		sd = media_entity_to_v4l2_subdev(link->source->entity);
> -		for (i = 0; i < RCAR_VIN_NUM; i++) {
> -			if (group->vin[i] &&
> -			    group->vin[i]->parallel.subdev == sd) {
> -				group->vin[i]->is_csi = false;
> -				ret = 0;
> -				goto out;
> -			}
> -		}
> -
> -		vin_err(vin, "Subdevice %s not registered to any VIN\n",
> -			link->source->entity->name);
> -		ret = -ENODEV;
> -		goto out;
> -	}
> -
> -	channel = rvin_group_csi_pad_to_channel(link->source->index);
> -	mask_new = mask & rvin_group_get_mask(vin, csi_id, channel);
> -	vin_dbg(vin, "Try link change mask: 0x%x new: 0x%x\n", mask, mask_new);
> -
> -	if (!mask_new) {
> -		ret = -EMLINK;
> -		goto out;
> -	}
> -
> -	/* New valid CHSEL found, set the new value. */
> -	ret = rvin_set_channel_routing(group->vin[master_id], __ffs(mask_new));
> -	if (ret)
> -		goto out;
> -
> -	vin->is_csi = true;
> -
> -out:
> -	mutex_unlock(&group->lock);
> -
> -	return ret;
> -}
> -
> -static const struct media_device_ops rvin_media_ops = {
> -	.link_notify = rvin_group_link_notify,
> -};
> -
>  /* -----------------------------------------------------------------------------
>   * Gen3 CSI2 Group Allocator

This is not only CSI2 anymore, right ? :)

>   */
> @@ -389,6 +208,22 @@ static void rvin_group_put(struct rvin_dev *vin)
>  	kref_put(&group->refcount, rvin_group_release);
>  }
>
> +/* group lock should be held when calling this function. */
> +static int rvin_group_entity_to_remote_id(struct rvin_group *group,
> +					  struct media_entity *entity)
> +{
> +	struct v4l2_subdev *sd;
> +	unsigned int i;
> +
> +	sd = media_entity_to_v4l2_subdev(entity);
> +
> +	for (i = 0; i < RVIN_REMOTES_MAX; i++)
> +		if (group->remotes[i].subdev == sd)
> +			return i;
> +
> +	return -ENODEV;
> +}
> +
>  static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
>  {
>  	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> @@ -921,6 +756,167 @@ static int rvin_parallel_init(struct rvin_dev *vin)
>   * CSI-2
>   */
>
> +static unsigned int rvin_csi2_get_mask(struct rvin_dev *vin,
> +				       enum rvin_csi_id csi_id,
> +				       unsigned char channel)
> +{
> +	const struct rvin_group_route *route;
> +	unsigned int mask = 0;
> +
> +	for (route = vin->info->routes; route->mask; route++) {
> +		if (route->vin == vin->id &&
> +		    route->csi == csi_id &&
> +		    route->channel == channel) {
> +			vin_dbg(vin,
> +				"Adding route: vin: %d csi: %d channel: %d\n",
> +				route->vin, route->csi, route->channel);
> +			mask |= route->mask;
> +		}
> +	}
> +
> +	return mask;
> +}
> +
> +/*
> + * Link setup for the links between a VIN and a CSI-2 receiver is a bit
> + * complex. The reason for this is that the register controlling routing
> + * is not present in each VIN instance. There are special VINs which
> + * control routing for themselves and other VINs. There are not many
> + * different possible links combinations that can be enabled at the same
> + * time, therefor all already enabled links which are controlled by a
> + * master VIN need to be taken into account when making the decision
> + * if a new link can be enabled or not.
> + *
> + * 1. Find out which VIN the link the user tries to enable is connected to.
> + * 2. Lookup which master VIN controls the links for this VIN.
> + * 3. Start with a bitmask with all bits set.
> + * 4. For each previously enabled link from the master VIN bitwise AND its
> + *    route mask (see documentation for mask in struct rvin_group_route)
> + *    with the bitmask.
> + * 5. Bitwise AND the mask for the link the user tries to enable to the bitmask.
> + * 6. If the bitmask is not empty at this point the new link can be enabled
> + *    while keeping all previous links enabled. Update the CHSEL value of the
> + *    master VIN and inform the user that the link could be enabled.
> + *
> + * Please note that no link can be enabled if any VIN in the group is
> + * currently open.
> + */
> +static int rvin_csi2_link_notify(struct media_link *link, u32 flags,
> +				 unsigned int notification)
> +{
> +	struct rvin_group *group = container_of(link->graph_obj.mdev,
> +						struct rvin_group, mdev);
> +	unsigned int master_id, channel, mask_new, i;
> +	unsigned int mask = ~0;
> +	struct media_entity *entity;
> +	struct video_device *vdev;
> +	struct media_pad *csi_pad;
> +	struct rvin_dev *vin = NULL;
> +	int csi_id, ret;
> +
> +	ret = v4l2_pipeline_link_notify(link, flags, notification);
> +	if (ret)
> +		return ret;
> +
> +	/* Only care about link enablement for VIN nodes. */
> +	if (!(flags & MEDIA_LNK_FL_ENABLED) ||
> +	    !is_media_entity_v4l2_video_device(link->sink->entity))
> +		return 0;
> +
> +	/*
> +	 * Don't allow link changes if any entity in the graph is
> +	 * streaming, modifying the CHSEL register fields can disrupt
> +	 * running streams.
> +	 */
> +	media_device_for_each_entity(entity, &group->mdev)
> +		if (entity->stream_count)
> +			return -EBUSY;
> +
> +	mutex_lock(&group->lock);
> +
> +	/* Find the master VIN that controls the routes. */
> +	vdev = media_entity_to_video_device(link->sink->entity);
> +	vin = container_of(vdev, struct rvin_dev, vdev);
> +	master_id = rvin_group_id_to_master(vin->id);
> +
> +	if (WARN_ON(!group->vin[master_id])) {
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	/* Build a mask for already enabled links. */
> +	for (i = master_id; i < master_id + 4; i++) {
> +		if (!group->vin[i])
> +			continue;
> +
> +		/* Get remote CSI-2, if any. */
> +		csi_pad = media_entity_remote_pad(
> +				&group->vin[i]->vdev.entity.pads[0]);
> +		if (!csi_pad)
> +			continue;
> +
> +		csi_id = rvin_group_entity_to_remote_id(group, csi_pad->entity);
> +		channel = rvin_group_csi_pad_to_channel(csi_pad->index);
> +
> +		mask &= rvin_csi2_get_mask(group->vin[i], csi_id, channel);
> +	}
> +
> +	/* Add the new link to the existing mask and check if it works. */
> +	csi_id = rvin_group_entity_to_remote_id(group, link->source->entity);
> +
> +	if (csi_id == -ENODEV) {
> +		struct v4l2_subdev *sd;
> +
> +		/*
> +		 * Make sure the source entity subdevice is registered as
> +		 * a parallel input of one of the enabled VINs if it is not
> +		 * one of the CSI-2 subdevices.
> +		 *
> +		 * No hardware configuration required for parallel inputs,
> +		 * we can return here.
> +		 */
> +		sd = media_entity_to_v4l2_subdev(link->source->entity);
> +		for (i = 0; i < RCAR_VIN_NUM; i++) {
> +			if (group->vin[i] &&
> +			    group->vin[i]->parallel.subdev == sd) {
> +				group->vin[i]->is_csi = false;
> +				ret = 0;
> +				goto out;
> +			}
> +		}
> +
> +		vin_err(vin, "Subdevice %s not registered to any VIN\n",
> +			link->source->entity->name);
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	channel = rvin_group_csi_pad_to_channel(link->source->index);
> +	mask_new = mask & rvin_csi2_get_mask(vin, csi_id, channel);
> +	vin_dbg(vin, "Try link change mask: 0x%x new: 0x%x\n", mask, mask_new);
> +
> +	if (!mask_new) {
> +		ret = -EMLINK;
> +		goto out;
> +	}
> +
> +	/* New valid CHSEL found, set the new value. */
> +	ret = rvin_set_channel_routing(group->vin[master_id], __ffs(mask_new));
> +	if (ret)
> +		goto out;
> +
> +	vin->is_csi = true;
> +
> +out:
> +	mutex_unlock(&group->lock);
> +
> +	return ret;
> +}
> +
> +static const struct media_device_ops rvin_csi2_media_ops = {
> +	.link_notify = rvin_csi2_link_notify,
> +};
> +
>  static void rvin_csi2_setup_links(struct rvin_dev *vin)
>  {
>  	const struct rvin_group_route *route;
> @@ -986,7 +982,7 @@ static int rvin_csi2_init(struct rvin_dev *vin)
>  	if (ret < 0)
>  		return ret;
>
> -	ret = rvin_group_get(vin, rvin_csi2_setup_links, &rvin_media_ops);
> +	ret = rvin_group_get(vin, rvin_csi2_setup_links, &rvin_csi2_media_ops);

The code seems to be only moved indeed!
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
   j

>  	if (ret)
>  		goto err_controls;
>
> --
> 2.31.1
>

Re: [PATCH 11/11] rcar-vin: Add r8a779a0 support

[Jacopo Mondi]

Hi Niklas,

On Tue, Apr 13, 2021 at 08:02:53PM +0200, Niklas Söderlund wrote:
> Add support for the R-Car V3U (r8a779a0) to the driver. The V3U have the

has

> VIN modules connected to a ISP instead of directly to the R-Car CSI-2
> receivers. The reason being that the ISP performs channel selection
> based on CSI-2 VC/DT pairs and routes the video data to different VIN
> modules. Other SoC versions this filtering is done by the VIN modules

"In other SoC versions". Or "For"...

> themself.
>
> While the media graph very different from other SoCs the only difference

the media graph is

> in operating the VIN modules is that the VC/DT filtering should be
> skipped as that is performed by the ISP.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 102 +++++++++++++++++++-
>  drivers/media/platform/rcar-vin/rcar-dma.c  |  20 ++--
>  drivers/media/platform/rcar-vin/rcar-vin.h  |  15 ++-
>  3 files changed, 124 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 763be02f507e6f3f..98326e80d2ed23d2 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -1006,6 +1006,87 @@ static int rvin_csi2_init(struct rvin_dev *vin)
>  	return ret;
>  }
>
> +/* -----------------------------------------------------------------------------
> + * ISP
> + */
> +
> +static void rvin_isp_setup_links(struct rvin_dev *vin)
> +{
> +	unsigned int i;
> +
> +	/* Create all media device links between VINs and ISP's. */
> +	mutex_lock(&vin->group->lock);
> +	for (i = 0; i < RCAR_VIN_NUM; i++) {
> +		struct media_pad *source_pad, *sink_pad;
> +		struct media_entity *source, *sink;
> +		unsigned int source_slot = i / 8;
> +		unsigned int source_idx = i % 8 + 1;
> +
> +		if (!vin->group->vin[i])
> +			continue;
> +
> +		/* Check that ISP is part of the group. */
> +		if (!vin->group->remotes[source_slot].subdev)
> +			continue;
> +
> +		source = &vin->group->remotes[source_slot].subdev->entity;
> +		source_pad = &source->pads[source_idx];
> +
> +		sink = &vin->group->vin[i]->vdev.entity;
> +		sink_pad = &sink->pads[0];
> +
> +		/* Skip if link already exists. */
> +		if (media_entity_find_link(source_pad, sink_pad))
> +			continue;
> +
> +		if (media_create_pad_link(source, source_idx, sink, 0,
> +					  MEDIA_LNK_FL_ENABLED |
> +					  MEDIA_LNK_FL_IMMUTABLE)) {
> +			vin_err(vin, "Error adding link from %s to %s\n",
> +				source->name, sink->name);

Same question about the CSI-2 version, is is ok not reporting errors ?

> +			break;
> +		}
> +	}
> +	mutex_unlock(&vin->group->lock);
> +}
> +
> +static void rvin_isp_cleanup(struct rvin_dev *vin)
> +{
> +	rvin_group_notifier_cleanup(vin);
> +	rvin_group_put(vin);
> +	rvin_free_controls(vin);
> +}
> +
> +static int rvin_isp_init(struct rvin_dev *vin)
> +{
> +	int ret;
> +
> +	vin->pad.flags = MEDIA_PAD_FL_SINK;
> +	ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
> +	if (ret)
> +		return ret;
> +
> +	ret = rvin_create_controls(vin, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = rvin_group_get(vin, rvin_isp_setup_links, NULL);
> +	if (ret)
> +		goto err_controls;
> +
> +	ret = rvin_group_notifier_init(vin, 2, RVIN_ISP_MAX);
> +	if (ret)
> +		goto err_group;
> +
> +	return 0;
> +err_group:
> +	rvin_group_put(vin);
> +err_controls:
> +	rvin_free_controls(vin);
> +
> +	return ret;
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Suspend / Resume
>   */
> @@ -1378,6 +1459,15 @@ static const struct rvin_info rcar_info_r8a77995 = {
>  	.routes = rcar_info_r8a77995_routes,
>  };
>
> +static const struct rvin_info rcar_info_r8a779a0 = {
> +	.model = RCAR_GEN3,
> +	.use_mc = true,
> +	.use_isp = true,
> +	.nv12 = true,
> +	.max_width = 4096,
> +	.max_height = 4096,
> +};
> +
>  static const struct of_device_id rvin_of_id_table[] = {
>  	{
>  		.compatible = "renesas,vin-r8a774a1",
> @@ -1435,6 +1525,10 @@ static const struct of_device_id rvin_of_id_table[] = {
>  		.compatible = "renesas,vin-r8a77995",
>  		.data = &rcar_info_r8a77995,
>  	},
> +	{
> +		.compatible = "renesas,vin-r8a779a0",
> +		.data = &rcar_info_r8a779a0,
> +	},
>  	{ /* Sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, rvin_of_id_table);
> @@ -1483,7 +1577,9 @@ static int rcar_vin_probe(struct platform_device *pdev)
>
>  	platform_set_drvdata(pdev, vin);
>
> -	if (vin->info->use_mc)
> +	if (vin->info->use_isp)
> +		ret = rvin_isp_init(vin);
> +	else if (vin->info->use_mc)
>  		ret = rvin_csi2_init(vin);
>  	else
>  		ret = rvin_parallel_init(vin);
> @@ -1507,7 +1603,9 @@ static int rcar_vin_remove(struct platform_device *pdev)
>
>  	rvin_v4l2_unregister(vin);
>
> -	if (vin->info->use_mc)
> +	if (vin->info->use_isp)
> +		rvin_isp_cleanup(vin);
> +	else if (vin->info->use_mc)
>  		rvin_csi2_cleanup(vin);
>  	else
>  		rvin_parallel_cleanup(vin);
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index f30dafbdf61ca15f..24b3a51e524fcc72 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -783,16 +783,18 @@ static int rvin_setup(struct rvin_dev *vin)
>  	/* Always update on field change */
>  	vnmc |= VNMC_VUP;
>
> -	/* If input and output use the same colorspace, use bypass mode */
> -	if (input_is_yuv == output_is_yuv)
> -		vnmc |= VNMC_BPS;
> +	if (!vin->info->use_isp) {
> +		/* If input and output use the same colorspace, use bypass mode */
> +		if (input_is_yuv == output_is_yuv)
> +			vnmc |= VNMC_BPS;
>
> -	if (vin->info->model == RCAR_GEN3) {
> -		/* Select between CSI-2 and parallel input */
> -		if (vin->is_csi)
> -			vnmc &= ~VNMC_DPINE;
> -		else
> -			vnmc |= VNMC_DPINE;
> +		if (vin->info->model == RCAR_GEN3) {
> +			/* Select between CSI-2 and parallel input */
> +			if (vin->is_csi)
> +				vnmc &= ~VNMC_DPINE;
> +			else
> +				vnmc |= VNMC_DPINE;
> +		}
>  	}
>
>  	/* Progressive or interlaced mode */
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index f2c32d5ceb7f83d9..88728ebe470ecafd 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -29,7 +29,7 @@
>  #define HW_BUFFER_MASK 0x7f
>
>  /* Max number on VIN instances that can be in a system */
> -#define RCAR_VIN_NUM 8
> +#define RCAR_VIN_NUM 32
>
>  struct rvin_group;
>
> @@ -48,7 +48,17 @@ enum rvin_csi_id {
>  	RVIN_CSI_MAX,
>  };
>
> -#define RVIN_REMOTES_MAX RVIN_CSI_MAX
> +enum rvin_isp_id {
> +	RVIN_ISP0,
> +	RVIN_ISP1,
> +	RVIN_ISP2,
> +	RVIN_ISP4,
> +	RVIN_ISP_MAX,
> +};

Be aware this seems not to be used

> +
> +#define RVIN_REMOTES_MAX \
> +	(((unsigned int)RVIN_CSI_MAX) > ((unsigned int)RVIN_ISP_MAX) ? \
> +	 RVIN_CSI_MAX : RVIN_ISP_MAX)

And I assume this is here in order to prepare for the introduction of
new CSI-2 interfaces or new ISPs

The way how easy it looks plumbing ISP support is means you did a
great job paving the way for that to happen in the previous patches!
Really good job!

Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
   j

>
>  /**
>   * enum rvin_dma_state - DMA states
> @@ -158,6 +168,7 @@ struct rvin_group_route {
>  struct rvin_info {
>  	enum model_id model;
>  	bool use_mc;
> +	bool use_isp;
>  	bool nv12;
>
>  	unsigned int max_width;
> --
> 2.31.1
>

Re: [PATCH 01/11] rcar-vin: Refactor controls creation for video device

[Sakari Ailus]

Hi Niklas, Jacopo,

On Tue, Jul 06, 2021 at 06:58:03PM +0200, Jacopo Mondi wrote:
> > > > @@ -522,10 +543,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
> > > >  	rvin_v4l2_unregister(vin);
> > > >  	vin->parallel.subdev = NULL;
> > > >
> > > > -	if (!vin->info->use_mc) {
> > > > -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > > > -		vin->vdev.ctrl_handler = NULL;
> > > > -	}
> > > > +	if (!vin->info->use_mc)
> > >
> > > I know it was there already, but give that rvin_parallel_notify_unbind()
> > > is only registered for parallel, can this happen ?
> >
> > Yes, on Gen2 where we don't use a media-graph.
> >
> 
> Ah correct, for gen3 the controls are freed elsewhere, right!
> 
> Thanks for the clarification

I already had the set in my tree but I can throw it out if you'd prefer to
send v2 instead. At least I noticed only minor matters in the comments.

-- 
Regards,

Sakari Ailus

Re: [PATCH 01/11] rcar-vin: Refactor controls creation for video device

[Niklas Söderlund]

Hi Sakari,

On 2021-07-08 16:40:40 +0300, Sakari Ailus wrote:
> Hi Niklas, Jacopo,
> 
> On Tue, Jul 06, 2021 at 06:58:03PM +0200, Jacopo Mondi wrote:
> > > > > @@ -522,10 +543,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
> > > > >  	rvin_v4l2_unregister(vin);
> > > > >  	vin->parallel.subdev = NULL;
> > > > >
> > > > > -	if (!vin->info->use_mc) {
> > > > > -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > > > > -		vin->vdev.ctrl_handler = NULL;
> > > > > -	}
> > > > > +	if (!vin->info->use_mc)
> > > >
> > > > I know it was there already, but give that rvin_parallel_notify_unbind()
> > > > is only registered for parallel, can this happen ?
> > >
> > > Yes, on Gen2 where we don't use a media-graph.
> > >
> > 
> > Ah correct, for gen3 the controls are freed elsewhere, right!
> > 
> > Thanks for the clarification
> 
> I already had the set in my tree but I can throw it out if you'd prefer to
> send v2 instead. At least I noticed only minor matters in the comments.

There is one small issue in a cleanup path in 4/11 that should be fixed 
and it's always good to get the small things fixed. If I have a v2 out 
before end of day tomorrow could you refresh what you have in your tree?

> 
> -- 
> Regards,
> 
> Sakari Ailus

-- 
Regards,
Niklas Söderlund

Re: [PATCH 01/11] rcar-vin: Refactor controls creation for video device

[Sakari Ailus]

On Thu, Jul 08, 2021 at 04:05:43PM +0200, Niklas Söderlund wrote:
> Hi Sakari,
> 
> On 2021-07-08 16:40:40 +0300, Sakari Ailus wrote:
> > Hi Niklas, Jacopo,
> > 
> > On Tue, Jul 06, 2021 at 06:58:03PM +0200, Jacopo Mondi wrote:
> > > > > > @@ -522,10 +543,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
> > > > > >  	rvin_v4l2_unregister(vin);
> > > > > >  	vin->parallel.subdev = NULL;
> > > > > >
> > > > > > -	if (!vin->info->use_mc) {
> > > > > > -		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > > > > > -		vin->vdev.ctrl_handler = NULL;
> > > > > > -	}
> > > > > > +	if (!vin->info->use_mc)
> > > > >
> > > > > I know it was there already, but give that rvin_parallel_notify_unbind()
> > > > > is only registered for parallel, can this happen ?
> > > >
> > > > Yes, on Gen2 where we don't use a media-graph.
> > > >
> > > 
> > > Ah correct, for gen3 the controls are freed elsewhere, right!
> > > 
> > > Thanks for the clarification
> > 
> > I already had the set in my tree but I can throw it out if you'd prefer to
> > send v2 instead. At least I noticed only minor matters in the comments.
> 
> There is one small issue in a cleanup path in 4/11 that should be fixed 
> and it's always good to get the small things fixed. If I have a v2 out 
> before end of day tomorrow could you refresh what you have in your tree?

Certainly. There's no urgency --- we don't have even rc1 yet.

-- 
Sakari Ailus

Re: [PATCH 08/11] rcar-vin: Create a callback to setup media links

[Niklas Söderlund]

Hi Jacopo,

Thanks for your review effort.

On 2021-07-07 12:33:55 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Tue, Apr 13, 2021 at 08:02:50PM +0200, Niklas Söderlund wrote:
> > New IP versions will have different media graphs and require a different
> > link setup. Breakout the specific link setup to a callback that are
> > associated with the group.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 97 ++++++++++++---------
> >  drivers/media/platform/rcar-vin/rcar-vin.h  |  3 +
> >  2 files changed, 57 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index 2628637084ae2aa9..d03ecd443b01c2b3 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -247,7 +247,8 @@ static void rvin_group_cleanup(struct rvin_group *group)
> >  	mutex_destroy(&group->lock);
> >  }
> >
> > -static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
> > +static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin,
> > +			   void (*link_setup)(struct rvin_dev *))
> >  {
> >  	struct media_device *mdev = &group->mdev;
> >  	const struct of_device_id *match;
> > @@ -263,6 +264,8 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin)
> >
> >  	vin_dbg(vin, "found %u enabled VIN's in DT", group->count);
> >
> > +	group->link_setup = link_setup;
> > +
> >  	mdev->dev = vin->dev;
> >  	mdev->ops = &rvin_media_ops;
> >
> > @@ -295,7 +298,8 @@ static void rvin_group_release(struct kref *kref)
> >  	mutex_unlock(&rvin_group_lock);
> >  }
> >
> > -static int rvin_group_get(struct rvin_dev *vin)
> > +static int rvin_group_get(struct rvin_dev *vin,
> > +			  void (*link_setup)(struct rvin_dev *))
> >  {
> >  	struct rvin_group *group;
> >  	u32 id;
> > @@ -327,7 +331,7 @@ static int rvin_group_get(struct rvin_dev *vin)
> >  			goto err_group;
> >  		}
> >
> > -		ret = rvin_group_init(group, vin);
> > +		ret = rvin_group_init(group, vin, link_setup);
> >  		if (ret) {
> >  			kfree(group);
> >  			vin_err(vin, "Failed to initialize group\n");
> > @@ -386,7 +390,6 @@ static void rvin_group_put(struct rvin_dev *vin)
> >  static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
> >  {
> >  	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
> > -	const struct rvin_group_route *route;
> >  	unsigned int i;
> >  	int ret;
> >
> > @@ -410,44 +413,7 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
> >  		}
> >  	}
> >
> > -	/* Create all media device links between VINs and CSI-2's. */
> > -	mutex_lock(&vin->group->lock);
> > -	for (route = vin->info->routes; route->mask; route++) {
> > -		struct media_pad *source_pad, *sink_pad;
> > -		struct media_entity *source, *sink;
> > -		unsigned int source_idx;
> > -
> > -		/* Check that VIN is part of the group. */
> > -		if (!vin->group->vin[route->vin])
> > -			continue;
> > -
> > -		/* Check that VIN' master is part of the group. */
> > -		if (!vin->group->vin[rvin_group_id_to_master(route->vin)])
> > -			continue;
> > -
> > -		/* Check that CSI-2 is part of the group. */
> > -		if (!vin->group->remotes[route->csi].subdev)
> > -			continue;
> > -
> > -		source = &vin->group->remotes[route->csi].subdev->entity;
> > -		source_idx = rvin_group_csi_channel_to_pad(route->channel);
> > -		source_pad = &source->pads[source_idx];
> > -
> > -		sink = &vin->group->vin[route->vin]->vdev.entity;
> > -		sink_pad = &sink->pads[0];
> > -
> > -		/* Skip if link already exists. */
> > -		if (media_entity_find_link(source_pad, sink_pad))
> > -			continue;
> > -
> > -		ret = media_create_pad_link(source, source_idx, sink, 0, 0);
> > -		if (ret) {
> > -			vin_err(vin, "Error adding link from %s to %s\n",
> > -				source->name, sink->name);
> > -			break;
> > -		}
> > -	}
> > -	mutex_unlock(&vin->group->lock);
> > +	vin->group->link_setup(vin);
> 
> Compared to the previuous version, we've lost the ability to return
> erros on failures in link creation. Is it intentional ?

This is a good point, I will fix it for v2 (same for the comment in the 
ISP case you point out in 11/11).

> 
> Thanks
>    j
> 
> >
> >  	return ret;
> >  }
> > @@ -953,6 +919,51 @@ static int rvin_parallel_init(struct rvin_dev *vin)
> >   * CSI-2
> >   */
> >
> > +static void rvin_csi2_setup_links(struct rvin_dev *vin)
> > +{
> > +	const struct rvin_group_route *route;
> > +	int ret;
> > +
> > +	/* Create all media device links between VINs and CSI-2's. */
> > +	mutex_lock(&vin->group->lock);
> > +	for (route = vin->info->routes; route->mask; route++) {
> > +		struct media_pad *source_pad, *sink_pad;
> > +		struct media_entity *source, *sink;
> > +		unsigned int source_idx;
> > +
> > +		/* Check that VIN is part of the group. */
> > +		if (!vin->group->vin[route->vin])
> > +			continue;
> > +
> > +		/* Check that VIN' master is part of the group. */
> > +		if (!vin->group->vin[rvin_group_id_to_master(route->vin)])
> > +			continue;
> > +
> > +		/* Check that CSI-2 is part of the group. */
> > +		if (!vin->group->remotes[route->csi].subdev)
> > +			continue;
> > +
> > +		source = &vin->group->remotes[route->csi].subdev->entity;
> > +		source_idx = rvin_group_csi_channel_to_pad(route->channel);
> > +		source_pad = &source->pads[source_idx];
> > +
> > +		sink = &vin->group->vin[route->vin]->vdev.entity;
> > +		sink_pad = &sink->pads[0];
> > +
> > +		/* Skip if link already exists. */
> > +		if (media_entity_find_link(source_pad, sink_pad))
> > +			continue;
> > +
> > +		ret = media_create_pad_link(source, source_idx, sink, 0, 0);
> > +		if (ret) {
> > +			vin_err(vin, "Error adding link from %s to %s\n",
> > +				source->name, sink->name);
> > +			break;
> > +		}
> > +	}
> > +	mutex_unlock(&vin->group->lock);
> > +}
> > +
> >  static void rvin_csi2_cleanup(struct rvin_dev *vin)
> >  {
> >  	rvin_group_notifier_cleanup(vin);
> > @@ -973,7 +984,7 @@ static int rvin_csi2_init(struct rvin_dev *vin)
> >  	if (ret < 0)
> >  		return ret;
> >
> > -	ret = rvin_group_get(vin);
> > +	ret = rvin_group_get(vin, rvin_csi2_setup_links);
> >  	if (ret)
> >  		goto err_controls;
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > index 39207aaf39ef9391..f2c32d5ceb7f83d9 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > @@ -269,6 +269,7 @@ struct rvin_dev {
> >   * @count:		number of enabled VIN instances found in DT
> >   * @notifier:		group notifier for CSI-2 async subdevices
> >   * @vin:		VIN instances which are part of the group
> > + * @link_setup:		Callback to create all links for the media graph
> >   * @remotes:		array of pairs of fwnode and subdev pointers
> >   *			to all remote subdevices.
> >   */
> > @@ -282,6 +283,8 @@ struct rvin_group {
> >  	struct v4l2_async_notifier notifier;
> >  	struct rvin_dev *vin[RCAR_VIN_NUM];
> >
> > +	void (*link_setup)(struct rvin_dev *vin);
> > +
> >  	struct {
> >  		struct v4l2_async_subdev *asd;
> >  		struct v4l2_subdev *subdev;
> > --
> > 2.31.1
> >

-- 
Regards,
Niklas Söderlund