[PATCH v4 0/9] rcar-vin: Add support for parallel input on Gen3

[PATCH v4 0/9] rcar-vin: Add support for parallel input on Gen3

[Jacopo Mondi]

Hello,
   this series adds support for parallel video input to the Gen3 version of
rcar-vin driver.

Compared to v3, this new iteration closes all comments from Niklas and Sergei.

As the meat of the patch series hasn't changed much, please refer to v3 cover
letter for more details.

The most notable change is the great simplification of the parallel
input notifiers registration in [5/9], as now the media controller is
initialized before parallel inputs are parsed. As the media device is now
properly initialized, parallel input notifiers can be registered as soon as new
input devices are discovered, without having to wait for the group notifier
complete callback to be called. Thanks Niklas for pointing that out.

Testing:
Tested image capture on both Draak and Salvator-X M3-W with a fake parallel
input device added.

Test branch for Salvator-X available at
git://jmondi.org d3/media-master/salvator-x-dts_csi2/m3w-add_parallel_to_dts+driver-v4
For Draak at:
git://jmondi.org d3/media-master/salvator-x-dts_csi2/d3-vin-driver-v4+enable-HDMI-in-dts
(sorry for the horrid branch names :)

Niklas, I know you have a V3M with expansion that has both CSI-2 and parallel
input. Could you give this one a spin please?

No changelog reported here, except from the one reported above.

Most of the other changes are minors, the most notable ones are reported in
each patch commit message.

Thanks
   j

Jacopo Mondi (9):
  media: rcar-vin: Rename 'digital' to 'parallel'
  media: rcar-vin: Remove two empty lines
  media: rcar-vin: Create a group notifier
  media: rcar-vin: Cache the mbus configuration flags
  media: rcar-vin: Parse parallel input on Gen3
  media: rcar-vin: Link parallel input media entities
  media: rcar-vin: Handle parallel subdev in link_notify
  media: rcar-vin: Rename _rcar_info to rcar_info
  media: rcar-vin: Add support for R-Car R8A77995 SoC

 drivers/media/platform/rcar-vin/rcar-core.c | 258 ++++++++++++++++++----------
 drivers/media/platform/rcar-vin/rcar-dma.c  |  36 ++--
 drivers/media/platform/rcar-vin/rcar-v4l2.c |  12 +-
 drivers/media/platform/rcar-vin/rcar-vin.h  |  29 ++--
 4 files changed, 215 insertions(+), 120 deletions(-)

--
2.7.4

[PATCH v4 1/9] media: rcar-vin: Rename 'digital' to 'parallel'

[Jacopo Mondi]

As the term 'digital' is used all over the rcar-vin code in place of
'parallel', rename all the occurrencies.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 72 ++++++++++++++---------------
 drivers/media/platform/rcar-vin/rcar-dma.c  |  4 +-
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 12 ++---
 drivers/media/platform/rcar-vin/rcar-vin.h  |  6 +--
 4 files changed, 47 insertions(+), 47 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index d3072e1..16d3e01 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -376,12 +376,12 @@ static int rvin_find_pad(struct v4l2_subdev *sd, int direction)
 }
 
 /* -----------------------------------------------------------------------------
- * Digital async notifier
+ * Parallel async notifier
  */
 
 /* The vin lock should be held when calling the subdevice attach and detach */
-static int rvin_digital_subdevice_attach(struct rvin_dev *vin,
-					 struct v4l2_subdev *subdev)
+static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
+					  struct v4l2_subdev *subdev)
 {
 	struct v4l2_subdev_mbus_code_enum code = {
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
@@ -392,15 +392,15 @@ static int rvin_digital_subdevice_attach(struct rvin_dev *vin,
 	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
 	if (ret < 0)
 		return ret;
-	vin->digital->source_pad = ret;
+	vin->parallel->source_pad = ret;
 
 	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
-	vin->digital->sink_pad = ret < 0 ? 0 : ret;
+	vin->parallel->sink_pad = ret < 0 ? 0 : ret;
 
 	/* Find compatible subdevices mbus format */
 	vin->mbus_code = 0;
 	code.index = 0;
-	code.pad = vin->digital->source_pad;
+	code.pad = vin->parallel->source_pad;
 	while (!vin->mbus_code &&
 	       !v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
 		code.index++;
@@ -450,21 +450,21 @@ static int rvin_digital_subdevice_attach(struct rvin_dev *vin,
 
 	vin->vdev.ctrl_handler = &vin->ctrl_handler;
 
-	vin->digital->subdev = subdev;
+	vin->parallel->subdev = subdev;
 
 	return 0;
 }
 
-static void rvin_digital_subdevice_detach(struct rvin_dev *vin)
+static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
 {
 	rvin_v4l2_unregister(vin);
 	v4l2_ctrl_handler_free(&vin->ctrl_handler);
 
 	vin->vdev.ctrl_handler = NULL;
-	vin->digital->subdev = NULL;
+	vin->parallel->subdev = NULL;
 }
 
-static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
+static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
 {
 	struct rvin_dev *vin = notifier_to_vin(notifier);
 	int ret;
@@ -478,28 +478,28 @@ static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
 	return rvin_v4l2_register(vin);
 }
 
-static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
-				       struct v4l2_subdev *subdev,
-				       struct v4l2_async_subdev *asd)
+static void rvin_parallel_notify_unbind(struct v4l2_async_notifier *notifier,
+					struct v4l2_subdev *subdev,
+					struct v4l2_async_subdev *asd)
 {
 	struct rvin_dev *vin = notifier_to_vin(notifier);
 
-	vin_dbg(vin, "unbind digital subdev %s\n", subdev->name);
+	vin_dbg(vin, "unbind parallel subdev %s\n", subdev->name);
 
 	mutex_lock(&vin->lock);
-	rvin_digital_subdevice_detach(vin);
+	rvin_parallel_subdevice_detach(vin);
 	mutex_unlock(&vin->lock);
 }
 
-static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
-				     struct v4l2_subdev *subdev,
-				     struct v4l2_async_subdev *asd)
+static int rvin_parallel_notify_bound(struct v4l2_async_notifier *notifier,
+				      struct v4l2_subdev *subdev,
+				      struct v4l2_async_subdev *asd)
 {
 	struct rvin_dev *vin = notifier_to_vin(notifier);
 	int ret;
 
 	mutex_lock(&vin->lock);
-	ret = rvin_digital_subdevice_attach(vin, subdev);
+	ret = rvin_parallel_subdevice_attach(vin, subdev);
 	mutex_unlock(&vin->lock);
 	if (ret)
 		return ret;
@@ -507,21 +507,21 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
 	v4l2_set_subdev_hostdata(subdev, vin);
 
 	vin_dbg(vin, "bound subdev %s source pad: %u sink pad: %u\n",
-		subdev->name, vin->digital->source_pad,
-		vin->digital->sink_pad);
+		subdev->name, vin->parallel->source_pad,
+		vin->parallel->sink_pad);
 
 	return 0;
 }
 
-static const struct v4l2_async_notifier_operations rvin_digital_notify_ops = {
-	.bound = rvin_digital_notify_bound,
-	.unbind = rvin_digital_notify_unbind,
-	.complete = rvin_digital_notify_complete,
+static const struct v4l2_async_notifier_operations rvin_parallel_notify_ops = {
+	.bound = rvin_parallel_notify_bound,
+	.unbind = rvin_parallel_notify_unbind,
+	.complete = rvin_parallel_notify_complete,
 };
 
-static int rvin_digital_parse_v4l2(struct device *dev,
-				   struct v4l2_fwnode_endpoint *vep,
-				   struct v4l2_async_subdev *asd)
+static int rvin_parallel_parse_v4l2(struct device *dev,
+				    struct v4l2_fwnode_endpoint *vep,
+				    struct v4l2_async_subdev *asd)
 {
 	struct rvin_dev *vin = dev_get_drvdata(dev);
 	struct rvin_graph_entity *rvge =
@@ -546,28 +546,28 @@ static int rvin_digital_parse_v4l2(struct device *dev,
 		return -EINVAL;
 	}
 
-	vin->digital = rvge;
+	vin->parallel = rvge;
 
 	return 0;
 }
 
-static int rvin_digital_graph_init(struct rvin_dev *vin)
+static int rvin_parallel_graph_init(struct rvin_dev *vin)
 {
 	int ret;
 
 	ret = v4l2_async_notifier_parse_fwnode_endpoints(
 		vin->dev, &vin->notifier,
-		sizeof(struct rvin_graph_entity), rvin_digital_parse_v4l2);
+		sizeof(struct rvin_graph_entity), rvin_parallel_parse_v4l2);
 	if (ret)
 		return ret;
 
-	if (!vin->digital)
+	if (!vin->parallel)
 		return -ENODEV;
 
-	vin_dbg(vin, "Found digital subdevice %pOF\n",
-		to_of_node(vin->digital->asd.match.fwnode));
+	vin_dbg(vin, "Found parallel subdevice %pOF\n",
+		to_of_node(vin->parallel->asd.match.fwnode));
 
-	vin->notifier.ops = &rvin_digital_notify_ops;
+	vin->notifier.ops = &rvin_parallel_notify_ops;
 	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
 	if (ret < 0) {
 		vin_err(vin, "Notifier registration failed\n");
@@ -1088,7 +1088,7 @@ static int rcar_vin_probe(struct platform_device *pdev)
 	if (vin->info->use_mc)
 		ret = rvin_mc_init(vin);
 	else
-		ret = rvin_digital_graph_init(vin);
+		ret = rvin_parallel_graph_init(vin);
 	if (ret < 0)
 		goto error;
 
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index ac07f99..f1c3585 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -733,7 +733,7 @@ static int rvin_setup(struct rvin_dev *vin)
 		vnmc |= VNMC_BPS;
 
 	if (vin->info->model == RCAR_GEN3) {
-		/* Select between CSI-2 and Digital input */
+		/* Select between CSI-2 and parallel input */
 		if (vin->mbus_cfg.type == V4L2_MBUS_CSI2)
 			vnmc &= ~VNMC_DPINE;
 		else
@@ -1074,7 +1074,7 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
 
 	/* No media controller used, simply pass operation to subdevice. */
 	if (!vin->info->use_mc) {
-		ret = v4l2_subdev_call(vin->digital->subdev, video, s_stream,
+		ret = v4l2_subdev_call(vin->parallel->subdev, video, s_stream,
 				       on);
 
 		return ret == -ENOIOCTLCMD ? 0 : ret;
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index e78fba8..87a718b 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -144,7 +144,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
 {
 	struct v4l2_subdev_format fmt = {
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
-		.pad = vin->digital->source_pad,
+		.pad = vin->parallel->source_pad,
 	};
 	int ret;
 
@@ -175,7 +175,7 @@ static int rvin_try_format(struct rvin_dev *vin, u32 which,
 	struct v4l2_subdev_pad_config *pad_cfg;
 	struct v4l2_subdev_format format = {
 		.which = which,
-		.pad = vin->digital->source_pad,
+		.pad = vin->parallel->source_pad,
 	};
 	enum v4l2_field field;
 	u32 width, height;
@@ -517,7 +517,7 @@ static int rvin_enum_dv_timings(struct file *file, void *priv_fh,
 	if (timings->pad)
 		return -EINVAL;
 
-	timings->pad = vin->digital->sink_pad;
+	timings->pad = vin->parallel->sink_pad;
 
 	ret = v4l2_subdev_call(sd, pad, enum_dv_timings, timings);
 
@@ -569,7 +569,7 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
 	if (cap->pad)
 		return -EINVAL;
 
-	cap->pad = vin->digital->sink_pad;
+	cap->pad = vin->parallel->sink_pad;
 
 	ret = v4l2_subdev_call(sd, pad, dv_timings_cap, cap);
 
@@ -587,7 +587,7 @@ static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid *edid)
 	if (edid->pad)
 		return -EINVAL;
 
-	edid->pad = vin->digital->sink_pad;
+	edid->pad = vin->parallel->sink_pad;
 
 	ret = v4l2_subdev_call(sd, pad, get_edid, edid);
 
@@ -605,7 +605,7 @@ static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid *edid)
 	if (edid->pad)
 		return -EINVAL;
 
-	edid->pad = vin->digital->sink_pad;
+	edid->pad = vin->parallel->sink_pad;
 
 	ret = v4l2_subdev_call(sd, pad, set_edid, edid);
 
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index c2aef78..755ac3c 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -146,7 +146,7 @@ struct rvin_info {
  * @v4l2_dev:		V4L2 device
  * @ctrl_handler:	V4L2 control handler
  * @notifier:		V4L2 asynchronous subdevs notifier
- * @digital:		entity in the DT for local digital subdevice
+ * @parallel:		entity in the DT for local parallel subdevice
  *
  * @group:		Gen3 CSI group
  * @id:			Gen3 group id for this VIN
@@ -182,7 +182,7 @@ struct rvin_dev {
 	struct v4l2_device v4l2_dev;
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct v4l2_async_notifier notifier;
-	struct rvin_graph_entity *digital;
+	struct rvin_graph_entity *parallel;
 
 	struct rvin_group *group;
 	unsigned int id;
@@ -209,7 +209,7 @@ struct rvin_dev {
 	v4l2_std_id std;
 };
 
-#define vin_to_source(vin)		((vin)->digital->subdev)
+#define vin_to_source(vin)		((vin)->parallel->subdev)
 
 /* Debug */
 #define vin_dbg(d, fmt, arg...)		dev_dbg(d->dev, fmt, ##arg)
-- 
2.7.4

[PATCH v4 2/9] media: rcar-vin: Remove two empty lines

[Jacopo Mondi]

Remove un-necessary empty lines.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 16d3e01..bcf02de 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -707,11 +707,9 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
 		return -EINVAL;
 
 	if (!of_device_is_available(to_of_node(asd->match.fwnode))) {
-
 		vin_dbg(vin, "OF device %pOF disabled, ignoring\n",
 			to_of_node(asd->match.fwnode));
 		return -ENOTCONN;
-
 	}
 
 	if (vin->group->csi[vep->base.id].fwnode) {
-- 
2.7.4

[PATCH v4 3/9] media: rcar-vin: Create a group notifier

[Jacopo Mondi]

As CSI-2 subdevices are shared between several VIN instances, a shared
notifier to collect the CSI-2 async subdevices is required. So far, the
rcar-vin driver used the notifier of the last VIN instance to probe but
with the forth-coming introduction of parallel input subdevices support
in mc-compliant code path, each VIN may register its own notifier if any
parallel subdevice is connected there.

To avoid registering a notifier twice (once for parallel subdev and one
for the CSI-2 subdevs) create a group notifier, shared by all the VIN
instances.

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

---
v3 -> v4:
- Unregister and cleanup group notifier when un-registering the VIN
  instance whose v4l2_dev the notifier is associated to.
---
 drivers/media/platform/rcar-vin/rcar-core.c | 38 ++++++++++++++---------------
 drivers/media/platform/rcar-vin/rcar-vin.h  |  5 ++--
 2 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index bcf02de..d3aadf3 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -46,6 +46,8 @@
  */
 #define rvin_group_id_to_master(vin) ((vin) < 4 ? 0 : 4)
 
+#define v4l2_dev_to_vin(d)	container_of(d, struct rvin_dev, v4l2_dev)
+
 /* -----------------------------------------------------------------------------
  * Media Controller link notification
  */
@@ -583,7 +585,7 @@ static int rvin_parallel_graph_init(struct rvin_dev *vin)
 
 static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
 {
-	struct rvin_dev *vin = notifier_to_vin(notifier);
+	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
 	const struct rvin_group_route *route;
 	unsigned int i;
 	int ret;
@@ -649,7 +651,7 @@ static void rvin_group_notify_unbind(struct v4l2_async_notifier *notifier,
 				     struct v4l2_subdev *subdev,
 				     struct v4l2_async_subdev *asd)
 {
-	struct rvin_dev *vin = notifier_to_vin(notifier);
+	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
 	unsigned int i;
 
 	for (i = 0; i < RCAR_VIN_NUM; i++)
@@ -673,7 +675,7 @@ static int rvin_group_notify_bound(struct v4l2_async_notifier *notifier,
 				   struct v4l2_subdev *subdev,
 				   struct v4l2_async_subdev *asd)
 {
-	struct rvin_dev *vin = notifier_to_vin(notifier);
+	struct rvin_dev *vin = v4l2_dev_to_vin(notifier->v4l2_dev);
 	unsigned int i;
 
 	mutex_lock(&vin->group->lock);
@@ -734,12 +736,6 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
 
 	mutex_lock(&vin->group->lock);
 
-	/* If there already is a notifier something has gone wrong, bail out. */
-	if (WARN_ON(vin->group->notifier)) {
-		mutex_unlock(&vin->group->lock);
-		return -EINVAL;
-	}
-
 	/* 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])
@@ -751,19 +747,16 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
 	}
 
 	/*
-	 * Have all VIN's look for subdevices. Some subdevices will overlap
-	 * but the parser function can handle it, so each subdevice will
-	 * only be registered once with the 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.
 	 */
-
-	vin->group->notifier = &vin->notifier;
-
 	for (i = 0; i < RCAR_VIN_NUM; i++) {
 		if (!vin->group->vin[i])
 			continue;
 
 		ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
-				vin->group->vin[i]->dev, vin->group->notifier,
+				vin->group->vin[i]->dev, &vin->group->notifier,
 				sizeof(struct v4l2_async_subdev), 1,
 				rvin_mc_parse_of_endpoint);
 		if (ret) {
@@ -774,9 +767,12 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
 
 	mutex_unlock(&vin->group->lock);
 
-	vin->group->notifier->ops = &rvin_group_notify_ops;
+	if (!vin->group->notifier.num_subdevs)
+		return 0;
 
-	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
+	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");
 		return ret;
@@ -1114,8 +1110,10 @@ static int rcar_vin_remove(struct platform_device *pdev)
 
 	if (vin->info->use_mc) {
 		mutex_lock(&vin->group->lock);
-		if (vin->group->notifier == &vin->notifier)
-			vin->group->notifier = NULL;
+		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);
 	} else {
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 755ac3c..ebb480f7 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -225,8 +225,7 @@ struct rvin_dev {
  *
  * @lock:		protects the count, notifier, vin and csi members
  * @count:		number of enabled VIN instances found in DT
- * @notifier:		pointer to the notifier of a VIN which handles the
- *			groups async sub-devices.
+ * @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.
@@ -238,7 +237,7 @@ struct rvin_group {
 
 	struct mutex lock;
 	unsigned int count;
-	struct v4l2_async_notifier *notifier;
+	struct v4l2_async_notifier notifier;
 	struct rvin_dev *vin[RCAR_VIN_NUM];
 
 	struct {
-- 
2.7.4

[PATCH v4 4/9] media: rcar-vin: Cache the mbus configuration flags

[Jacopo Mondi]

Media bus configuration flags and media bus type were so far a property
of each VIN instance, as the subdevice they were connected to was
immutable during the whole system life time.

With the forth-coming introduction of parallel input devices support,
a VIN instance can have the subdevice it is connected to switched at
runtime, from a CSI-2 subdevice to a parallel one and viceversa, through
the modification of links between media entities in the media controller
graph. To avoid discarding the per-subdevice configuration flags retrieved by
v4l2_fwnode parsing facilities, cache them in the 'rvin_graph_entity'
member of each VIN instance, opportunely renamed to 'rvin_parallel_entity'.

Also modify the register configuration function to take mbus flags into
account when running on a bus type that supports them.

The media bus type currently in use will be updated in a follow-up patch
to the link state change notification function.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 21 ++++++++-----------
 drivers/media/platform/rcar-vin/rcar-dma.c  | 32 +++++++++++++++++++----------
 drivers/media/platform/rcar-vin/rcar-vin.h  | 22 ++++++++++++++------
 3 files changed, 45 insertions(+), 30 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index d3aadf3..a799684 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -526,30 +526,29 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
 				    struct v4l2_async_subdev *asd)
 {
 	struct rvin_dev *vin = dev_get_drvdata(dev);
-	struct rvin_graph_entity *rvge =
-		container_of(asd, struct rvin_graph_entity, asd);
+	struct rvin_parallel_entity *rvpe =
+		container_of(asd, struct rvin_parallel_entity, asd);
 
 	if (vep->base.port || vep->base.id)
 		return -ENOTCONN;
 
-	vin->mbus_cfg.type = vep->bus_type;
+	vin->parallel = rvpe;
+	vin->parallel->mbus_type = vep->bus_type;
 
-	switch (vin->mbus_cfg.type) {
+	switch (vin->parallel->mbus_type) {
 	case V4L2_MBUS_PARALLEL:
 		vin_dbg(vin, "Found PARALLEL media bus\n");
-		vin->mbus_cfg.flags = vep->bus.parallel.flags;
+		vin->parallel->mbus_flags = vep->bus.parallel.flags;
 		break;
 	case V4L2_MBUS_BT656:
 		vin_dbg(vin, "Found BT656 media bus\n");
-		vin->mbus_cfg.flags = 0;
+		vin->parallel->mbus_flags = 0;
 		break;
 	default:
 		vin_err(vin, "Unknown media bus type\n");
 		return -EINVAL;
 	}
 
-	vin->parallel = rvge;
-
 	return 0;
 }
 
@@ -559,7 +558,7 @@ static int rvin_parallel_graph_init(struct rvin_dev *vin)
 
 	ret = v4l2_async_notifier_parse_fwnode_endpoints(
 		vin->dev, &vin->notifier,
-		sizeof(struct rvin_graph_entity), rvin_parallel_parse_v4l2);
+		sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2);
 	if (ret)
 		return ret;
 
@@ -785,10 +784,6 @@ static int rvin_mc_init(struct rvin_dev *vin)
 {
 	int ret;
 
-	/* All our sources are CSI-2 */
-	vin->mbus_cfg.type = V4L2_MBUS_CSI2;
-	vin->mbus_cfg.flags = 0;
-
 	vin->pad.flags = MEDIA_PAD_FL_SINK;
 	ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
 	if (ret)
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index f1c3585..d2b7002 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -659,8 +659,12 @@ static int rvin_setup(struct rvin_dev *vin)
 		break;
 	case MEDIA_BUS_FMT_UYVY8_2X8:
 		/* BT.656 8bit YCbCr422 or BT.601 8bit YCbCr422 */
-		vnmc |= vin->mbus_cfg.type == V4L2_MBUS_BT656 ?
-			VNMC_INF_YUV8_BT656 : VNMC_INF_YUV8_BT601;
+		if (!vin->is_csi &&
+		    vin->parallel->mbus_type == V4L2_MBUS_BT656)
+			vnmc |= VNMC_INF_YUV8_BT656;
+		else
+			vnmc |= VNMC_INF_YUV8_BT601;
+
 		input_is_yuv = true;
 		break;
 	case MEDIA_BUS_FMT_RGB888_1X24:
@@ -668,8 +672,12 @@ static int rvin_setup(struct rvin_dev *vin)
 		break;
 	case MEDIA_BUS_FMT_UYVY10_2X10:
 		/* BT.656 10bit YCbCr422 or BT.601 10bit YCbCr422 */
-		vnmc |= vin->mbus_cfg.type == V4L2_MBUS_BT656 ?
-			VNMC_INF_YUV10_BT656 : VNMC_INF_YUV10_BT601;
+		if (!vin->is_csi &&
+		    vin->parallel->mbus_type == V4L2_MBUS_BT656)
+			vnmc |= VNMC_INF_YUV10_BT656;
+		else
+			vnmc |= VNMC_INF_YUV10_BT601;
+
 		input_is_yuv = true;
 		break;
 	default:
@@ -682,13 +690,15 @@ static int rvin_setup(struct rvin_dev *vin)
 	else
 		dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
 
-	/* Hsync Signal Polarity Select */
-	if (!(vin->mbus_cfg.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
-		dmr2 |= VNDMR2_HPS;
+	if (!vin->is_csi) {
+		/* Hsync Signal Polarity Select */
+		if (!(vin->parallel->mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
+			dmr2 |= VNDMR2_HPS;
 
-	/* Vsync Signal Polarity Select */
-	if (!(vin->mbus_cfg.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
-		dmr2 |= VNDMR2_VPS;
+		/* Vsync Signal Polarity Select */
+		if (!(vin->parallel->mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
+			dmr2 |= VNDMR2_VPS;
+	}
 
 	/*
 	 * Output format
@@ -734,7 +744,7 @@ static int rvin_setup(struct rvin_dev *vin)
 
 	if (vin->info->model == RCAR_GEN3) {
 		/* Select between CSI-2 and parallel input */
-		if (vin->mbus_cfg.type == V4L2_MBUS_CSI2)
+		if (vin->is_csi)
 			vnmc &= ~VNMC_DPINE;
 		else
 			vnmc |= VNMC_DPINE;
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index ebb480f7..8bc3704 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -73,16 +73,22 @@ struct rvin_video_format {
 };
 
 /**
- * struct rvin_graph_entity - Video endpoint from async framework
+ * struct rvin_parallel_entity - Parallel video input endpoint descriptor
  * @asd:	sub-device descriptor for async framework
  * @subdev:	subdevice matched using async framework
+ * @mbus_type:	media bus type
+ * @mbus_flags:	media bus configuration flags
  * @source_pad:	source pad of remote subdevice
  * @sink_pad:	sink pad of remote subdevice
+ *
  */
-struct rvin_graph_entity {
+struct rvin_parallel_entity {
 	struct v4l2_async_subdev asd;
 	struct v4l2_subdev *subdev;
 
+	enum v4l2_mbus_type mbus_type;
+	unsigned int mbus_flags;
+
 	unsigned int source_pad;
 	unsigned int sink_pad;
 };
@@ -146,7 +152,8 @@ struct rvin_info {
  * @v4l2_dev:		V4L2 device
  * @ctrl_handler:	V4L2 control handler
  * @notifier:		V4L2 asynchronous subdevs notifier
- * @parallel:		entity in the DT for local parallel subdevice
+ *
+ * @parallel:		parallel input subdevice descriptor
  *
  * @group:		Gen3 CSI group
  * @id:			Gen3 group id for this VIN
@@ -164,7 +171,8 @@ struct rvin_info {
  * @sequence:		V4L2 buffers sequence number
  * @state:		keeps track of operation state
  *
- * @mbus_cfg:		media bus configuration from DT
+ * @is_csi:		flag to mark the VIN as using a CSI-2 subdevice
+ *
  * @mbus_code:		media bus format code
  * @format:		active V4L2 pixel format
  *
@@ -182,7 +190,8 @@ struct rvin_dev {
 	struct v4l2_device v4l2_dev;
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct v4l2_async_notifier notifier;
-	struct rvin_graph_entity *parallel;
+
+	struct rvin_parallel_entity *parallel;
 
 	struct rvin_group *group;
 	unsigned int id;
@@ -199,7 +208,8 @@ struct rvin_dev {
 	unsigned int sequence;
 	enum rvin_dma_state state;
 
-	struct v4l2_mbus_config mbus_cfg;
+	bool is_csi;
+
 	u32 mbus_code;
 	struct v4l2_pix_format format;
 
-- 
2.7.4

[PATCH v4 5/9] media: rcar-vin: Parse parallel input on Gen3

[Jacopo Mondi]

The rcar-vin driver so far had a mutually exclusive code path for
handling parallel and CSI-2 video input subdevices, with only the CSI-2
use case supporting media-controller. As we add support for parallel
inputs to Gen3 media-controller compliant code path now parse both port@0
and port@1, handling the media-controller use case in the parallel
bound/unbind notifier operations.

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

---
v3 -> v4:
- Change the mc/parallel initialization order. Initialize mc first, then
  parallel
- As a consequence no need to delay parallel notifiers registration, the
  media controller is set up already when parallel input got parsed,
  this greatly simplify the group notifier complete callback.
---
 drivers/media/platform/rcar-vin/rcar-core.c | 56 ++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 21 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index a799684..29619c2 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -399,6 +399,11 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
 	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
 	vin->parallel->sink_pad = ret < 0 ? 0 : ret;
 
+	if (vin->info->use_mc) {
+		vin->parallel->subdev = subdev;
+		return 0;
+	}
+
 	/* Find compatible subdevices mbus format */
 	vin->mbus_code = 0;
 	code.index = 0;
@@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
 static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
 {
 	rvin_v4l2_unregister(vin);
-	v4l2_ctrl_handler_free(&vin->ctrl_handler);
-
-	vin->vdev.ctrl_handler = NULL;
 	vin->parallel->subdev = NULL;
+
+	if (!vin->info->use_mc) {
+		v4l2_ctrl_handler_free(&vin->ctrl_handler);
+		vin->vdev.ctrl_handler = NULL;
+	}
 }
 
 static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
@@ -552,18 +559,18 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
 	return 0;
 }
 
-static int rvin_parallel_graph_init(struct rvin_dev *vin)
+static int rvin_parallel_init(struct rvin_dev *vin)
 {
 	int ret;
 
-	ret = v4l2_async_notifier_parse_fwnode_endpoints(
-		vin->dev, &vin->notifier,
-		sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2);
+	ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
+		vin->dev, &vin->notifier, sizeof(struct rvin_parallel_entity),
+		0, rvin_parallel_parse_v4l2);
 	if (ret)
 		return ret;
 
 	if (!vin->parallel)
-		return -ENODEV;
+		return -ENOTCONN;
 
 	vin_dbg(vin, "Found parallel subdevice %pOF\n",
 		to_of_node(vin->parallel->asd.match.fwnode));
@@ -784,14 +791,8 @@ static int rvin_mc_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_group_get(vin);
-	if (ret)
-		return ret;
+	if (!vin->info->use_mc)
+		return 0;
 
 	ret = rvin_mc_parse_of_graph(vin);
 	if (ret)
@@ -1074,11 +1075,24 @@ static int rcar_vin_probe(struct platform_device *pdev)
 		return ret;
 
 	platform_set_drvdata(pdev, vin);
-	if (vin->info->use_mc)
-		ret = rvin_mc_init(vin);
-	else
-		ret = rvin_parallel_graph_init(vin);
-	if (ret < 0)
+
+	if (vin->info->use_mc) {
+		vin->pad.flags = MEDIA_PAD_FL_SINK;
+		ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
+		if (ret)
+			return ret;
+
+		ret = rvin_group_get(vin);
+		if (ret)
+			return ret;
+	}
+
+	ret = rvin_mc_init(vin);
+	if (ret)
+		return ret;
+
+	ret = rvin_parallel_init(vin);
+	if (ret < 0 && ret != -ENOTCONN)
 		goto error;
 
 	pm_suspend_ignore_children(&pdev->dev, true);
-- 
2.7.4

[PATCH v4 6/9] media: rcar-vin: Link parallel input media entities

[Jacopo Mondi]

When running with media-controller link the parallel input
media entities with the VIN entities at 'complete' callback time.

To create media links the v4l2_device should be registered first.
Check if the device is already registered, to avoid double registrations.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 29619c2..b69b375 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -476,6 +476,8 @@ static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
 static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
 {
 	struct rvin_dev *vin = notifier_to_vin(notifier);
+	struct media_entity *source;
+	struct media_entity *sink;
 	int ret;
 
 	ret = v4l2_device_register_subdev_nodes(&vin->v4l2_dev);
@@ -484,7 +486,26 @@ static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
 		return ret;
 	}
 
-	return rvin_v4l2_register(vin);
+	if (!video_is_registered(&vin->vdev)) {
+		ret = rvin_v4l2_register(vin);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (!vin->info->use_mc)
+		return 0;
+
+	/* If we're running with media-controller, link the subdevs. */
+	source = &vin->parallel->subdev->entity;
+	sink = &vin->vdev.entity;
+
+	ret = media_create_pad_link(source, vin->parallel->source_pad,
+				    sink, vin->parallel->sink_pad, 0);
+	if (ret)
+		vin_err(vin, "Error adding link from %s to %s: %d\n",
+			source->name, sink->name, ret);
+
+	return ret;
 }
 
 static void rvin_parallel_notify_unbind(struct v4l2_async_notifier *notifier,
@@ -604,7 +625,8 @@ static int rvin_group_notify_complete(struct v4l2_async_notifier *notifier)
 
 	/* Register all video nodes for the group. */
 	for (i = 0; i < RCAR_VIN_NUM; i++) {
-		if (vin->group->vin[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;
-- 
2.7.4

[PATCH v4 7/9] media: rcar-vin: Handle parallel subdev in link_notify

[Jacopo Mondi]

Handle parallel subdevices in link_notify callback. If the notified link
involves a parallel subdevice, do not change routing of the VIN-CSI-2
devices and mark the VIN instance as using a parallel input. If the
CSI-2 link setup succeeds instead, mark the VIN instance as using CSI-2.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 35 ++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index b69b375..8edf896 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -171,9 +171,37 @@ static int rvin_group_link_notify(struct media_link *link, u32 flags,
 
 	/* 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);
+
+	if (csi_id == -ENODEV) {
+		struct v4l2_subdev *sd;
+		unsigned int i;
+
+		/*
+		 * 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 &&
+			    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) {
@@ -183,6 +211,11 @@ static int rvin_group_link_notify(struct media_link *link, u32 flags,
 
 	/* 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);
 
-- 
2.7.4

[PATCH v4 8/9] media: rcar-vin: Rename _rcar_info to rcar_info

[Jacopo Mondi]

Remove leading underscore to align all rcar_group_route structure
declarations.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 8edf896..30f6ea7 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -1019,7 +1019,7 @@ static const struct rvin_info rcar_info_r8a7796 = {
 	.routes = rcar_info_r8a7796_routes,
 };
 
-static const struct rvin_group_route _rcar_info_r8a77970_routes[] = {
+static const struct rvin_group_route rcar_info_r8a77970_routes[] = {
 	{ .csi = RVIN_CSI40, .channel = 0, .vin = 0, .mask = BIT(0) | BIT(3) },
 	{ .csi = RVIN_CSI40, .channel = 0, .vin = 1, .mask = BIT(2) },
 	{ .csi = RVIN_CSI40, .channel = 1, .vin = 1, .mask = BIT(3) },
@@ -1035,7 +1035,7 @@ static const struct rvin_info rcar_info_r8a77970 = {
 	.use_mc = true,
 	.max_width = 4096,
 	.max_height = 4096,
-	.routes = _rcar_info_r8a77970_routes,
+	.routes = rcar_info_r8a77970_routes,
 };
 
 static const struct of_device_id rvin_of_id_table[] = {
-- 
2.7.4

[PATCH v4 9/9] media: rcar-vin: Add support for R-Car R8A77995 SoC

[Jacopo Mondi]

Add R-Car R8A77995 SoC to the rcar-vin supported ones.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 30f6ea7..223310f 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -1038,6 +1038,18 @@ static const struct rvin_info rcar_info_r8a77970 = {
 	.routes = rcar_info_r8a77970_routes,
 };
 
+static const struct rvin_group_route rcar_info_r8a77995_routes[] = {
+	{ /* Sentinel */ }
+};
+
+static const struct rvin_info rcar_info_r8a77995 = {
+	.model = RCAR_GEN3,
+	.use_mc = true,
+	.max_width = 4096,
+	.max_height = 4096,
+	.routes = rcar_info_r8a77995_routes,
+};
+
 static const struct of_device_id rvin_of_id_table[] = {
 	{
 		.compatible = "renesas,vin-r8a7778",
@@ -1079,6 +1091,10 @@ static const struct of_device_id rvin_of_id_table[] = {
 		.compatible = "renesas,vin-r8a77970",
 		.data = &rcar_info_r8a77970,
 	},
+	{
+		.compatible = "renesas,vin-r8a77995",
+		.data = &rcar_info_r8a77995,
+	},
 	{ /* Sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, rvin_of_id_table);
-- 
2.7.4

Re: [PATCH v4 5/9] media: rcar-vin: Parse parallel input on Gen3

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

I really like what you did with this patch in v4.

On 2018-05-25 00:02:15 +0200, Jacopo Mondi wrote:
> The rcar-vin driver so far had a mutually exclusive code path for
> handling parallel and CSI-2 video input subdevices, with only the CSI-2
> use case supporting media-controller. As we add support for parallel
> inputs to Gen3 media-controller compliant code path now parse both port@0
> and port@1, handling the media-controller use case in the parallel
> bound/unbind notifier operations.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> ---
> v3 -> v4:
> - Change the mc/parallel initialization order. Initialize mc first, then
>   parallel
> - As a consequence no need to delay parallel notifiers registration, the
>   media controller is set up already when parallel input got parsed,
>   this greatly simplify the group notifier complete callback.
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 56 ++++++++++++++++++-----------
>  1 file changed, 35 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index a799684..29619c2 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -399,6 +399,11 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
>  	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
>  	vin->parallel->sink_pad = ret < 0 ? 0 : ret;
>  
> +	if (vin->info->use_mc) {
> +		vin->parallel->subdev = subdev;
> +		return 0;
> +	}
> +
>  	/* Find compatible subdevices mbus format */
>  	vin->mbus_code = 0;
>  	code.index = 0;
> @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
>  static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
>  {
>  	rvin_v4l2_unregister(vin);
> -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> -
> -	vin->vdev.ctrl_handler = NULL;
>  	vin->parallel->subdev = NULL;
> +
> +	if (!vin->info->use_mc) {
> +		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> +		vin->vdev.ctrl_handler = NULL;
> +	}
>  }
>  
>  static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
> @@ -552,18 +559,18 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
>  	return 0;
>  }
>  
> -static int rvin_parallel_graph_init(struct rvin_dev *vin)
> +static int rvin_parallel_init(struct rvin_dev *vin)
>  {
>  	int ret;
>  
> -	ret = v4l2_async_notifier_parse_fwnode_endpoints(
> -		vin->dev, &vin->notifier,
> -		sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2);
> +	ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> +		vin->dev, &vin->notifier, sizeof(struct rvin_parallel_entity),
> +		0, rvin_parallel_parse_v4l2);
>  	if (ret)
>  		return ret;
>  
>  	if (!vin->parallel)
> -		return -ENODEV;
> +		return -ENOTCONN;

I think you still should return -ENODEV here if !vin->info->use_mc to 
preserve Gen2 which runs without media controller behavior. How about:

    return vin->info->use_mc ? -ENOTCONN : -ENODEV;

>  
>  	vin_dbg(vin, "Found parallel subdevice %pOF\n",
>  		to_of_node(vin->parallel->asd.match.fwnode));
> @@ -784,14 +791,8 @@ static int rvin_mc_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_group_get(vin);
> -	if (ret)
> -		return ret;
> +	if (!vin->info->use_mc)
> +		return 0;
>  
>  	ret = rvin_mc_parse_of_graph(vin);
>  	if (ret)
> @@ -1074,11 +1075,24 @@ static int rcar_vin_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	platform_set_drvdata(pdev, vin);
> -	if (vin->info->use_mc)
> -		ret = rvin_mc_init(vin);
> -	else
> -		ret = rvin_parallel_graph_init(vin);
> -	if (ret < 0)
> +
> +	if (vin->info->use_mc) {
> +		vin->pad.flags = MEDIA_PAD_FL_SINK;
> +		ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
> +		if (ret)
> +			return ret;
> +
> +		ret = rvin_group_get(vin);
> +		if (ret)
> +			return ret;
> +	}

I don't see why you need to move the media pad creation out of 
rvin_mc_init(). With the reorder of the rvin_mc_init() 
rvin_parallel_init() I would keep this in rvin_mc_init().

> +
> +	ret = rvin_mc_init(vin);
> +	if (ret)
> +		return ret;
> +
> +	ret = rvin_parallel_init(vin);
> +	if (ret < 0 && ret != -ENOTCONN)
>  		goto error;
>  
>  	pm_suspend_ignore_children(&pdev->dev, true);
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v4 5/9] media: rcar-vin: Parse parallel input on Gen3

[Niklas Söderlund]

Hi Jacopo,

Thanks for your work.

I really like what you did with this patch in v4.

On 2018-05-25 00:02:15 +0200, Jacopo Mondi wrote:
> The rcar-vin driver so far had a mutually exclusive code path for
> handling parallel and CSI-2 video input subdevices, with only the CSI-2
> use case supporting media-controller. As we add support for parallel
> inputs to Gen3 media-controller compliant code path now parse both port@0
> and port@1, handling the media-controller use case in the parallel
> bound/unbind notifier operations.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> ---
> v3 -> v4:
> - Change the mc/parallel initialization order. Initialize mc first, then
>   parallel
> - As a consequence no need to delay parallel notifiers registration, the
>   media controller is set up already when parallel input got parsed,
>   this greatly simplify the group notifier complete callback.
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 56 ++++++++++++++++++-----------
>  1 file changed, 35 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index a799684..29619c2 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -399,6 +399,11 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
>  	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
>  	vin->parallel->sink_pad = ret < 0 ? 0 : ret;
>  
> +	if (vin->info->use_mc) {
> +		vin->parallel->subdev = subdev;
> +		return 0;
> +	}
> +
>  	/* Find compatible subdevices mbus format */
>  	vin->mbus_code = 0;
>  	code.index = 0;
> @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
>  static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
>  {
>  	rvin_v4l2_unregister(vin);
> -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> -
> -	vin->vdev.ctrl_handler = NULL;
>  	vin->parallel->subdev = NULL;
> +
> +	if (!vin->info->use_mc) {
> +		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> +		vin->vdev.ctrl_handler = NULL;
> +	}
>  }
>  
>  static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
> @@ -552,18 +559,18 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
>  	return 0;
>  }
>  
> -static int rvin_parallel_graph_init(struct rvin_dev *vin)
> +static int rvin_parallel_init(struct rvin_dev *vin)
>  {
>  	int ret;
>  
> -	ret = v4l2_async_notifier_parse_fwnode_endpoints(
> -		vin->dev, &vin->notifier,
> -		sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2);
> +	ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> +		vin->dev, &vin->notifier, sizeof(struct rvin_parallel_entity),
> +		0, rvin_parallel_parse_v4l2);
>  	if (ret)
>  		return ret;
>  
>  	if (!vin->parallel)
> -		return -ENODEV;
> +		return -ENOTCONN;

I think you still should return -ENODEV here if !vin->info->use_mc to 
preserve Gen2 which runs without media controller behavior. How about:

    return vin->info->use_mc ? -ENOTCONN : -ENODEV;

>  
>  	vin_dbg(vin, "Found parallel subdevice %pOF\n",
>  		to_of_node(vin->parallel->asd.match.fwnode));
> @@ -784,14 +791,8 @@ static int rvin_mc_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_group_get(vin);
> -	if (ret)
> -		return ret;
> +	if (!vin->info->use_mc)
> +		return 0;
>  
>  	ret = rvin_mc_parse_of_graph(vin);
>  	if (ret)
> @@ -1074,11 +1075,24 @@ static int rcar_vin_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	platform_set_drvdata(pdev, vin);
> -	if (vin->info->use_mc)
> -		ret = rvin_mc_init(vin);
> -	else
> -		ret = rvin_parallel_graph_init(vin);
> -	if (ret < 0)
> +
> +	if (vin->info->use_mc) {
> +		vin->pad.flags = MEDIA_PAD_FL_SINK;
> +		ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
> +		if (ret)
> +			return ret;
> +
> +		ret = rvin_group_get(vin);
> +		if (ret)
> +			return ret;
> +	}

I don't see why you need to move the media pad creation out of 
rvin_mc_init(). With the reorder of the rvin_mc_init() 
rvin_parallel_init() I would keep this in rvin_mc_init().

> +
> +	ret = rvin_mc_init(vin);
> +	if (ret)
> +		return ret;
> +
> +	ret = rvin_parallel_init(vin);
> +	if (ret < 0 && ret != -ENOTCONN)
>  		goto error;
>  
>  	pm_suspend_ignore_children(&pdev->dev, true);
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH v4 5/9] media: rcar-vin: Parse parallel input on Gen3

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 5509 bytes --]

Hi Niklas,

On Fri, May 25, 2018 at 12:29:44AM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> I really like what you did with this patch in v4.

Thanks for review and suggestions, what's there comes mostly from your
comments and guidance.

>
> On 2018-05-25 00:02:15 +0200, Jacopo Mondi wrote:
> > The rcar-vin driver so far had a mutually exclusive code path for
> > handling parallel and CSI-2 video input subdevices, with only the CSI-2
> > use case supporting media-controller. As we add support for parallel
> > inputs to Gen3 media-controller compliant code path now parse both port@0
> > and port@1, handling the media-controller use case in the parallel
> > bound/unbind notifier operations.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >
> > ---
> > v3 -> v4:
> > - Change the mc/parallel initialization order. Initialize mc first, then
> >   parallel
> > - As a consequence no need to delay parallel notifiers registration, the
> >   media controller is set up already when parallel input got parsed,
> >   this greatly simplify the group notifier complete callback.
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 56 ++++++++++++++++++-----------
> >  1 file changed, 35 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index a799684..29619c2 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -399,6 +399,11 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> >  	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
> >  	vin->parallel->sink_pad = ret < 0 ? 0 : ret;
> >
> > +	if (vin->info->use_mc) {
> > +		vin->parallel->subdev = subdev;
> > +		return 0;
> > +	}
> > +
> >  	/* Find compatible subdevices mbus format */
> >  	vin->mbus_code = 0;
> >  	code.index = 0;
> > @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> >  static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
> >  {
> >  	rvin_v4l2_unregister(vin);
> > -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > -
> > -	vin->vdev.ctrl_handler = NULL;
> >  	vin->parallel->subdev = NULL;
> > +
> > +	if (!vin->info->use_mc) {
> > +		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > +		vin->vdev.ctrl_handler = NULL;
> > +	}
> >  }
> >
> >  static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
> > @@ -552,18 +559,18 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> >  	return 0;
> >  }
> >
> > -static int rvin_parallel_graph_init(struct rvin_dev *vin)
> > +static int rvin_parallel_init(struct rvin_dev *vin)
> >  {
> >  	int ret;
> >
> > -	ret = v4l2_async_notifier_parse_fwnode_endpoints(
> > -		vin->dev, &vin->notifier,
> > -		sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2);
> > +	ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> > +		vin->dev, &vin->notifier, sizeof(struct rvin_parallel_entity),
> > +		0, rvin_parallel_parse_v4l2);
> >  	if (ret)
> >  		return ret;
> >
> >  	if (!vin->parallel)
> > -		return -ENODEV;
> > +		return -ENOTCONN;
>
> I think you still should return -ENODEV here if !vin->info->use_mc to
> preserve Gen2 which runs without media controller behavior. How about:
>
>     return vin->info->use_mc ? -ENOTCONN : -ENODEV;

Right, I wish I had some gen2 board to test. I wonder if that's not
better handled in probe though... I'll see how it looks like.
>
> >
> >  	vin_dbg(vin, "Found parallel subdevice %pOF\n",
> >  		to_of_node(vin->parallel->asd.match.fwnode));
> > @@ -784,14 +791,8 @@ static int rvin_mc_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_group_get(vin);
> > -	if (ret)
> > -		return ret;
> > +	if (!vin->info->use_mc)
> > +		return 0;
> >
> >  	ret = rvin_mc_parse_of_graph(vin);
> >  	if (ret)
> > @@ -1074,11 +1075,24 @@ static int rcar_vin_probe(struct platform_device *pdev)
> >  		return ret;
> >
> >  	platform_set_drvdata(pdev, vin);
> > -	if (vin->info->use_mc)
> > -		ret = rvin_mc_init(vin);
> > -	else
> > -		ret = rvin_parallel_graph_init(vin);
> > -	if (ret < 0)
> > +
> > +	if (vin->info->use_mc) {
> > +		vin->pad.flags = MEDIA_PAD_FL_SINK;
> > +		ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = rvin_group_get(vin);
> > +		if (ret)
> > +			return ret;
> > +	}
>
> I don't see why you need to move the media pad creation out of
> rvin_mc_init(). With the reorder of the rvin_mc_init()
> rvin_parallel_init() I would keep this in rvin_mc_init().
>

Just because I've been lazy re-structuring that part of code I broke
up in previous versions. I'll move that part back to mc_init() and
possibly also remove the

+	if (!vin->info->use_mc)
+		return 0;

in that function and handle this in probe().

Thanks
   j


> > +
> > +	ret = rvin_mc_init(vin);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = rvin_parallel_init(vin);
> > +	if (ret < 0 && ret != -ENOTCONN)
> >  		goto error;
> >
> >  	pm_suspend_ignore_children(&pdev->dev, true);
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas Söderlund

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 5/9] media: rcar-vin: Parse parallel input on Gen3

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 7265 bytes --]

Hi Niklas,
   I might have another question before sending v5.

On Fri, May 25, 2018 at 12:29:44AM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> I really like what you did with this patch in v4.
>
> On 2018-05-25 00:02:15 +0200, Jacopo Mondi wrote:
> > The rcar-vin driver so far had a mutually exclusive code path for
> > handling parallel and CSI-2 video input subdevices, with only the CSI-2
> > use case supporting media-controller. As we add support for parallel
> > inputs to Gen3 media-controller compliant code path now parse both port@0
> > and port@1, handling the media-controller use case in the parallel
> > bound/unbind notifier operations.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >
> > ---
> > v3 -> v4:
> > - Change the mc/parallel initialization order. Initialize mc first, then
> >   parallel
> > - As a consequence no need to delay parallel notifiers registration, the
> >   media controller is set up already when parallel input got parsed,
> >   this greatly simplify the group notifier complete callback.
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 56 ++++++++++++++++++-----------
> >  1 file changed, 35 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index a799684..29619c2 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -399,6 +399,11 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> >  	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
> >  	vin->parallel->sink_pad = ret < 0 ? 0 : ret;
> >
> > +	if (vin->info->use_mc) {
> > +		vin->parallel->subdev = subdev;
> > +		return 0;
> > +	}
> > +
> >  	/* Find compatible subdevices mbus format */
> >  	vin->mbus_code = 0;
> >  	code.index = 0;
> > @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> >  static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
> >  {
> >  	rvin_v4l2_unregister(vin);
> > -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > -
> > -	vin->vdev.ctrl_handler = NULL;
> >  	vin->parallel->subdev = NULL;
> > +
> > +	if (!vin->info->use_mc) {
> > +		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > +		vin->vdev.ctrl_handler = NULL;
> > +	}
> >  }
> >
> >  static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
> > @@ -552,18 +559,18 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> >  	return 0;
> >  }
> >
> > -static int rvin_parallel_graph_init(struct rvin_dev *vin)
> > +static int rvin_parallel_init(struct rvin_dev *vin)
> >  {
> >  	int ret;
> >
> > -	ret = v4l2_async_notifier_parse_fwnode_endpoints(
> > -		vin->dev, &vin->notifier,
> > -		sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2);
> > +	ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> > +		vin->dev, &vin->notifier, sizeof(struct rvin_parallel_entity),
> > +		0, rvin_parallel_parse_v4l2);
> >  	if (ret)
> >  		return ret;
> >
> >  	if (!vin->parallel)
> > -		return -ENODEV;
> > +		return -ENOTCONN;
>
> I think you still should return -ENODEV here if !vin->info->use_mc to
> preserve Gen2 which runs without media controller behavior. How about:
>
>     return vin->info->use_mc ? -ENOTCONN : -ENODEV;
>
> >
> >  	vin_dbg(vin, "Found parallel subdevice %pOF\n",
> >  		to_of_node(vin->parallel->asd.match.fwnode));
> > @@ -784,14 +791,8 @@ static int rvin_mc_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_group_get(vin);
> > -	if (ret)
> > -		return ret;
> > +	if (!vin->info->use_mc)
> > +		return 0;
> >
> >  	ret = rvin_mc_parse_of_graph(vin);
> >  	if (ret)
> > @@ -1074,11 +1075,24 @@ static int rcar_vin_probe(struct platform_device *pdev)
> >  		return ret;
> >
> >  	platform_set_drvdata(pdev, vin);
> > -	if (vin->info->use_mc)
> > -		ret = rvin_mc_init(vin);
> > -	else
> > -		ret = rvin_parallel_graph_init(vin);
> > -	if (ret < 0)
> > +
> > +	if (vin->info->use_mc) {
> > +		vin->pad.flags = MEDIA_PAD_FL_SINK;
> > +		ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = rvin_group_get(vin);
> > +		if (ret)
> > +			return ret;
> > +	}
>
> I don't see why you need to move the media pad creation out of
> rvin_mc_init(). With the reorder of the rvin_mc_init()
> rvin_parallel_init() I would keep this in rvin_mc_init().
>
> > +
> > +	ret = rvin_mc_init(vin);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = rvin_parallel_init(vin);
> > +	if (ret < 0 && ret != -ENOTCONN)
> >  		goto error;
> >
> >  	pm_suspend_ignore_children(&pdev->dev, true);

I've been looking at the error path handling now that the code looks
like this in the forthcoming v5:

	if (vin->info->use_mc) {
		ret = rvin_mc_init(vin);
		if (ret)
			goto error_dma_unregister;
	}

	ret = rvin_parallel_init(vin);
	if (ret)
		goto error_group_unregister;


        ...


error_group_unreg:
	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);
	}

error_dma_unreg:
	rvin_dma_unregister(vin);

	return ret;

Question is, do you think the group notifier should be unregistered
and cleaned up if the parallel input initialization of the VIN
instance whose v4l2_dev is used to register the group notifier fails?

I feel like it should, as the VIN instance whose v4l2_dev is used is
always the last probing one, so there should be no issues with other
VINs registering after it and finding themselves without a valid
notifier.

I felt like it was better anticipating this to you instead of adding
this part out of the blue in v5.

Also, I think in both parallel input and mc notifier registration, the
notifier should be cleaned up to release the parsed async subdevices
memory allocated by
v4l2_async_notifier_parse_fwnode_endpoints_by_port() if the
sub-sequent v4l2_async_notifier_register() fails.

That would be:

@@ -781,18 +782,29 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
                                           &vin->group->notifier);
        if (ret < 0) {
                vin_err(vin, "Notifier registration failed\n");
-               return ret;
+               goto error_clean_up_notifier;
        }

        return 0;
+
+error_clean_up_notifier:
+       v4l2_async_notifier_cleanup(&vin->group->notifier);
+
+       return ret;
 }

in both mc and parallel initialization functions.

With your ack I can send two patches on top of the currently merged VIN
version, and rebase my series on top eventually.

Sorry for the long email.

Thanks
   j


> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas Söderlund

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 5/9] media: rcar-vin: Parse parallel input on Gen3

[Niklas Söderlund]

Hi Jacopo,

On 2018-05-25 13:50:08 +0200, Jacopo Mondi wrote:
> Hi Niklas,
>    I might have another question before sending v5.
> 
> On Fri, May 25, 2018 at 12:29:44AM +0200, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your work.
> >
> > I really like what you did with this patch in v4.
> >
> > On 2018-05-25 00:02:15 +0200, Jacopo Mondi wrote:
> > > The rcar-vin driver so far had a mutually exclusive code path for
> > > handling parallel and CSI-2 video input subdevices, with only the CSI-2
> > > use case supporting media-controller. As we add support for parallel
> > > inputs to Gen3 media-controller compliant code path now parse both port@0
> > > and port@1, handling the media-controller use case in the parallel
> > > bound/unbind notifier operations.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > >
> > > ---
> > > v3 -> v4:
> > > - Change the mc/parallel initialization order. Initialize mc first, then
> > >   parallel
> > > - As a consequence no need to delay parallel notifiers registration, the
> > >   media controller is set up already when parallel input got parsed,
> > >   this greatly simplify the group notifier complete callback.
> > > ---
> > >  drivers/media/platform/rcar-vin/rcar-core.c | 56 ++++++++++++++++++-----------
> > >  1 file changed, 35 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > > index a799684..29619c2 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > @@ -399,6 +399,11 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> > >  	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
> > >  	vin->parallel->sink_pad = ret < 0 ? 0 : ret;
> > >
> > > +	if (vin->info->use_mc) {
> > > +		vin->parallel->subdev = subdev;
> > > +		return 0;
> > > +	}
> > > +
> > >  	/* Find compatible subdevices mbus format */
> > >  	vin->mbus_code = 0;
> > >  	code.index = 0;
> > > @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> > >  static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
> > >  {
> > >  	rvin_v4l2_unregister(vin);
> > > -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > > -
> > > -	vin->vdev.ctrl_handler = NULL;
> > >  	vin->parallel->subdev = NULL;
> > > +
> > > +	if (!vin->info->use_mc) {
> > > +		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > > +		vin->vdev.ctrl_handler = NULL;
> > > +	}
> > >  }
> > >
> > >  static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
> > > @@ -552,18 +559,18 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> > >  	return 0;
> > >  }
> > >
> > > -static int rvin_parallel_graph_init(struct rvin_dev *vin)
> > > +static int rvin_parallel_init(struct rvin_dev *vin)
> > >  {
> > >  	int ret;
> > >
> > > -	ret = v4l2_async_notifier_parse_fwnode_endpoints(
> > > -		vin->dev, &vin->notifier,
> > > -		sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2);
> > > +	ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> > > +		vin->dev, &vin->notifier, sizeof(struct rvin_parallel_entity),
> > > +		0, rvin_parallel_parse_v4l2);
> > >  	if (ret)
> > >  		return ret;
> > >
> > >  	if (!vin->parallel)
> > > -		return -ENODEV;
> > > +		return -ENOTCONN;
> >
> > I think you still should return -ENODEV here if !vin->info->use_mc to
> > preserve Gen2 which runs without media controller behavior. How about:
> >
> >     return vin->info->use_mc ? -ENOTCONN : -ENODEV;
> >
> > >
> > >  	vin_dbg(vin, "Found parallel subdevice %pOF\n",
> > >  		to_of_node(vin->parallel->asd.match.fwnode));
> > > @@ -784,14 +791,8 @@ static int rvin_mc_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_group_get(vin);
> > > -	if (ret)
> > > -		return ret;
> > > +	if (!vin->info->use_mc)
> > > +		return 0;
> > >
> > >  	ret = rvin_mc_parse_of_graph(vin);
> > >  	if (ret)
> > > @@ -1074,11 +1075,24 @@ static int rcar_vin_probe(struct platform_device *pdev)
> > >  		return ret;
> > >
> > >  	platform_set_drvdata(pdev, vin);
> > > -	if (vin->info->use_mc)
> > > -		ret = rvin_mc_init(vin);
> > > -	else
> > > -		ret = rvin_parallel_graph_init(vin);
> > > -	if (ret < 0)
> > > +
> > > +	if (vin->info->use_mc) {
> > > +		vin->pad.flags = MEDIA_PAD_FL_SINK;
> > > +		ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		ret = rvin_group_get(vin);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> >
> > I don't see why you need to move the media pad creation out of
> > rvin_mc_init(). With the reorder of the rvin_mc_init()
> > rvin_parallel_init() I would keep this in rvin_mc_init().
> >
> > > +
> > > +	ret = rvin_mc_init(vin);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = rvin_parallel_init(vin);
> > > +	if (ret < 0 && ret != -ENOTCONN)
> > >  		goto error;
> > >
> > >  	pm_suspend_ignore_children(&pdev->dev, true);
> 
> I've been looking at the error path handling now that the code looks
> like this in the forthcoming v5:
> 
> 	if (vin->info->use_mc) {
> 		ret = rvin_mc_init(vin);
> 		if (ret)
> 			goto error_dma_unregister;
> 	}
> 
> 	ret = rvin_parallel_init(vin);
> 	if (ret)
> 		goto error_group_unregister;
> 
> 
>         ...
> 
> 
> error_group_unreg:
> 	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);
> 	}
> 
> error_dma_unreg:
> 	rvin_dma_unregister(vin);
> 
> 	return ret;
> 
> Question is, do you think the group notifier should be unregistered
> and cleaned up if the parallel input initialization of the VIN
> instance whose v4l2_dev is used to register the group notifier fails?
> 
> I feel like it should, as the VIN instance whose v4l2_dev is used is
> always the last probing one, so there should be no issues with other
> VINs registering after it and finding themselves without a valid
> notifier.

I agree with you. If the parallel initialization fails everything done 
by that particular VIN probe should be undone. So if it have registered 
the group notifier it should unregister it.

> 
> I felt like it was better anticipating this to you instead of adding
> this part out of the blue in v5.
> 
> Also, I think in both parallel input and mc notifier registration, the
> notifier should be cleaned up to release the parsed async subdevices
> memory allocated by
> v4l2_async_notifier_parse_fwnode_endpoints_by_port() if the
> sub-sequent v4l2_async_notifier_register() fails.

I agree with you here as well. That this memory should be cleaned up, 
nice catch.

> 
> That would be:
> 
> @@ -781,18 +782,29 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>                                            &vin->group->notifier);
>         if (ret < 0) {
>                 vin_err(vin, "Notifier registration failed\n");
> -               return ret;
> +               goto error_clean_up_notifier;
>         }
> 
>         return 0;
> +
> +error_clean_up_notifier:
> +       v4l2_async_notifier_cleanup(&vin->group->notifier);
> +
> +       return ret;
>  }
> 
> in both mc and parallel initialization functions.
> 
> With your ack I can send two patches on top of the currently merged VIN
> version, and rebase my series on top eventually.
> 
> Sorry for the long email.
> 
> Thanks
>    j
> 
> 
> > > --
> > > 2.7.4
> > >
> >
> > --
> > Regards,
> > Niklas Söderlund



-- 
Regards,
Niklas Söderlund

Re: [PATCH v4 5/9] media: rcar-vin: Parse parallel input on Gen3

[Niklas Söderlund]

Hi Jacopo,

On 2018-05-25 13:50:08 +0200, Jacopo Mondi wrote:
> Hi Niklas,
>    I might have another question before sending v5.
> 
> On Fri, May 25, 2018 at 12:29:44AM +0200, Niklas S�derlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your work.
> >
> > I really like what you did with this patch in v4.
> >
> > On 2018-05-25 00:02:15 +0200, Jacopo Mondi wrote:
> > > The rcar-vin driver so far had a mutually exclusive code path for
> > > handling parallel and CSI-2 video input subdevices, with only the CSI-2
> > > use case supporting media-controller. As we add support for parallel
> > > inputs to Gen3 media-controller compliant code path now parse both port@0
> > > and port@1, handling the media-controller use case in the parallel
> > > bound/unbind notifier operations.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > >
> > > ---
> > > v3 -> v4:
> > > - Change the mc/parallel initialization order. Initialize mc first, then
> > >   parallel
> > > - As a consequence no need to delay parallel notifiers registration, the
> > >   media controller is set up already when parallel input got parsed,
> > >   this greatly simplify the group notifier complete callback.
> > > ---
> > >  drivers/media/platform/rcar-vin/rcar-core.c | 56 ++++++++++++++++++-----------
> > >  1 file changed, 35 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > > index a799684..29619c2 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > @@ -399,6 +399,11 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> > >  	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
> > >  	vin->parallel->sink_pad = ret < 0 ? 0 : ret;
> > >
> > > +	if (vin->info->use_mc) {
> > > +		vin->parallel->subdev = subdev;
> > > +		return 0;
> > > +	}
> > > +
> > >  	/* Find compatible subdevices mbus format */
> > >  	vin->mbus_code = 0;
> > >  	code.index = 0;
> > > @@ -460,10 +465,12 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
> > >  static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
> > >  {
> > >  	rvin_v4l2_unregister(vin);
> > > -	v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > > -
> > > -	vin->vdev.ctrl_handler = NULL;
> > >  	vin->parallel->subdev = NULL;
> > > +
> > > +	if (!vin->info->use_mc) {
> > > +		v4l2_ctrl_handler_free(&vin->ctrl_handler);
> > > +		vin->vdev.ctrl_handler = NULL;
> > > +	}
> > >  }
> > >
> > >  static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
> > > @@ -552,18 +559,18 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
> > >  	return 0;
> > >  }
> > >
> > > -static int rvin_parallel_graph_init(struct rvin_dev *vin)
> > > +static int rvin_parallel_init(struct rvin_dev *vin)
> > >  {
> > >  	int ret;
> > >
> > > -	ret = v4l2_async_notifier_parse_fwnode_endpoints(
> > > -		vin->dev, &vin->notifier,
> > > -		sizeof(struct rvin_parallel_entity), rvin_parallel_parse_v4l2);
> > > +	ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> > > +		vin->dev, &vin->notifier, sizeof(struct rvin_parallel_entity),
> > > +		0, rvin_parallel_parse_v4l2);
> > >  	if (ret)
> > >  		return ret;
> > >
> > >  	if (!vin->parallel)
> > > -		return -ENODEV;
> > > +		return -ENOTCONN;
> >
> > I think you still should return -ENODEV here if !vin->info->use_mc to
> > preserve Gen2 which runs without media controller behavior. How about:
> >
> >     return vin->info->use_mc ? -ENOTCONN : -ENODEV;
> >
> > >
> > >  	vin_dbg(vin, "Found parallel subdevice %pOF\n",
> > >  		to_of_node(vin->parallel->asd.match.fwnode));
> > > @@ -784,14 +791,8 @@ static int rvin_mc_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_group_get(vin);
> > > -	if (ret)
> > > -		return ret;
> > > +	if (!vin->info->use_mc)
> > > +		return 0;
> > >
> > >  	ret = rvin_mc_parse_of_graph(vin);
> > >  	if (ret)
> > > @@ -1074,11 +1075,24 @@ static int rcar_vin_probe(struct platform_device *pdev)
> > >  		return ret;
> > >
> > >  	platform_set_drvdata(pdev, vin);
> > > -	if (vin->info->use_mc)
> > > -		ret = rvin_mc_init(vin);
> > > -	else
> > > -		ret = rvin_parallel_graph_init(vin);
> > > -	if (ret < 0)
> > > +
> > > +	if (vin->info->use_mc) {
> > > +		vin->pad.flags = MEDIA_PAD_FL_SINK;
> > > +		ret = media_entity_pads_init(&vin->vdev.entity, 1, &vin->pad);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		ret = rvin_group_get(vin);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> >
> > I don't see why you need to move the media pad creation out of
> > rvin_mc_init(). With the reorder of the rvin_mc_init()
> > rvin_parallel_init() I would keep this in rvin_mc_init().
> >
> > > +
> > > +	ret = rvin_mc_init(vin);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = rvin_parallel_init(vin);
> > > +	if (ret < 0 && ret != -ENOTCONN)
> > >  		goto error;
> > >
> > >  	pm_suspend_ignore_children(&pdev->dev, true);
> 
> I've been looking at the error path handling now that the code looks
> like this in the forthcoming v5:
> 
> 	if (vin->info->use_mc) {
> 		ret = rvin_mc_init(vin);
> 		if (ret)
> 			goto error_dma_unregister;
> 	}
> 
> 	ret = rvin_parallel_init(vin);
> 	if (ret)
> 		goto error_group_unregister;
> 
> 
>         ...
> 
> 
> error_group_unreg:
> 	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);
> 	}
> 
> error_dma_unreg:
> 	rvin_dma_unregister(vin);
> 
> 	return ret;
> 
> Question is, do you think the group notifier should be unregistered
> and cleaned up if the parallel input initialization of the VIN
> instance whose v4l2_dev is used to register the group notifier fails?
> 
> I feel like it should, as the VIN instance whose v4l2_dev is used is
> always the last probing one, so there should be no issues with other
> VINs registering after it and finding themselves without a valid
> notifier.

I agree with you. If the parallel initialization fails everything done 
by that particular VIN probe should be undone. So if it have registered 
the group notifier it should unregister it.

> 
> I felt like it was better anticipating this to you instead of adding
> this part out of the blue in v5.
> 
> Also, I think in both parallel input and mc notifier registration, the
> notifier should be cleaned up to release the parsed async subdevices
> memory allocated by
> v4l2_async_notifier_parse_fwnode_endpoints_by_port() if the
> sub-sequent v4l2_async_notifier_register() fails.

I agree with you here as well. That this memory should be cleaned up, 
nice catch.

> 
> That would be:
> 
> @@ -781,18 +782,29 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>                                            &vin->group->notifier);
>         if (ret < 0) {
>                 vin_err(vin, "Notifier registration failed\n");
> -               return ret;
> +               goto error_clean_up_notifier;
>         }
> 
>         return 0;
> +
> +error_clean_up_notifier:
> +       v4l2_async_notifier_cleanup(&vin->group->notifier);
> +
> +       return ret;
>  }
> 
> in both mc and parallel initialization functions.
> 
> With your ack I can send two patches on top of the currently merged VIN
> version, and rebase my series on top eventually.
> 
> Sorry for the long email.
> 
> Thanks
>    j
> 
> 
> > > --
> > > 2.7.4
> > >
> >
> > --
> > Regards,
> > Niklas S�derlund



-- 
Regards,
Niklas S�derlund