[PATCH 0/5] v4l2-fwnode: Remove v4l2_async_notifier_parse_fwnode_endpoints_by_port() helper

[PATCH 0/5] v4l2-fwnode: Remove v4l2_async_notifier_parse_fwnode_endpoints_by_port() helper

[Niklas Söderlund]

Hello,

This series aims to remove a V4L2 helper that provides a too simple 
implementation, v4l2_async_notifier_parse_fwnode_endpoints_by_port().  
Instead drivers shall implement what the helper does directly to get 
greater control of the process. There is only one user left in tree of 
this interface, R-Car VIN.

This series starts therefor to rework the R-Car VIN driver to not depend 
on the helper. And in the process a small fwnode imbalance is fixed.  
After the last user of the helper is reworked the series removes the 
helper to prevent usage of it to resurface.

This series is based on-top of the latest media tree and is tested on 
Renesas R-Car M3-N and Koelsch without any regressions or change in 
behavior detected.

Niklas Söderlund (5):
  rcar-vin: Only dynamically allocate v4l2_async_subdev
  rcar-vin: Rework parallel firmware parsing
  rcar-vin: Use v4l2_async_subdev instead of fwnode_handle to match
    subdevices
  rcar-vin: Rework CSI-2 firmware parsing
  v4l2-fwnode: Remove
    v4l2_async_notifier_parse_fwnode_endpoints_by_port()

 drivers/media/platform/rcar-vin/rcar-core.c | 156 ++++++++++++--------
 drivers/media/platform/rcar-vin/rcar-dma.c  |  16 +-
 drivers/media/platform/rcar-vin/rcar-v4l2.c |  12 +-
 drivers/media/platform/rcar-vin/rcar-vin.h  |   8 +-
 drivers/media/v4l2-core/v4l2-fwnode.c       |  14 --
 include/media/v4l2-fwnode.h                 |  53 -------
 6 files changed, 113 insertions(+), 146 deletions(-)

-- 
2.29.2

[PATCH 1/5] rcar-vin: Only dynamically allocate v4l2_async_subdev

[Niklas Söderlund]

In preparation of removing the usage of the old helper
v4l2_async_notifier_parse_fwnode_endpoints_by_port() do not dynamically
allocate the whole structure containing the parameters for the parallel
interface, instead only allocate the v4l2_async_subdev structure. There
is no functional change.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 45 ++++++++++-----------
 drivers/media/platform/rcar-vin/rcar-dma.c  | 16 ++++----
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 12 +++---
 drivers/media/platform/rcar-vin/rcar-vin.h  |  6 +--
 4 files changed, 39 insertions(+), 40 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 518813456194e8b3..07f250bfc0051135 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -185,8 +185,8 @@ static int rvin_group_link_notify(struct media_link *link, u32 flags,
 		 */
 		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) {
+			if (group->vin[i] &&
+			    group->vin[i]->parallel.subdev == sd) {
 				group->vin[i]->is_csi = false;
 				ret = 0;
 				goto out;
@@ -440,20 +440,20 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
 	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SOURCE);
 	if (ret < 0)
 		return ret;
-	vin->parallel->source_pad = ret;
+	vin->parallel.source_pad = ret;
 
 	ret = rvin_find_pad(subdev, MEDIA_PAD_FL_SINK);
-	vin->parallel->sink_pad = ret < 0 ? 0 : ret;
+	vin->parallel.sink_pad = ret < 0 ? 0 : ret;
 
 	if (vin->info->use_mc) {
-		vin->parallel->subdev = subdev;
+		vin->parallel.subdev = subdev;
 		return 0;
 	}
 
 	/* Find compatible subdevices mbus format */
 	vin->mbus_code = 0;
 	code.index = 0;
-	code.pad = vin->parallel->source_pad;
+	code.pad = vin->parallel.source_pad;
 	while (!vin->mbus_code &&
 	       !v4l2_subdev_call(subdev, pad, enum_mbus_code, NULL, &code)) {
 		code.index++;
@@ -512,7 +512,7 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
 
 	vin->vdev.ctrl_handler = &vin->ctrl_handler;
 
-	vin->parallel->subdev = subdev;
+	vin->parallel.subdev = subdev;
 
 	return 0;
 }
@@ -520,7 +520,7 @@ static int rvin_parallel_subdevice_attach(struct rvin_dev *vin,
 static void rvin_parallel_subdevice_detach(struct rvin_dev *vin)
 {
 	rvin_v4l2_unregister(vin);
-	vin->parallel->subdev = NULL;
+	vin->parallel.subdev = NULL;
 
 	if (!vin->info->use_mc) {
 		v4l2_ctrl_handler_free(&vin->ctrl_handler);
@@ -551,11 +551,11 @@ static int rvin_parallel_notify_complete(struct v4l2_async_notifier *notifier)
 		return 0;
 
 	/* If we're running with media-controller, link the subdevs. */
-	source = &vin->parallel->subdev->entity;
+	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);
+	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);
@@ -592,8 +592,8 @@ static int rvin_parallel_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->parallel->source_pad,
-		vin->parallel->sink_pad);
+		subdev->name, vin->parallel.source_pad,
+		vin->parallel.sink_pad);
 
 	return 0;
 }
@@ -609,28 +609,27 @@ static int rvin_parallel_parse_v4l2(struct device *dev,
 				    struct v4l2_async_subdev *asd)
 {
 	struct rvin_dev *vin = dev_get_drvdata(dev);
-	struct rvin_parallel_entity *rvpe =
-		container_of(asd, struct rvin_parallel_entity, asd);
 
 	if (vep->base.port || vep->base.id)
 		return -ENOTCONN;
 
-	vin->parallel = rvpe;
-	vin->parallel->mbus_type = vep->bus_type;
+	vin->parallel.mbus_type = vep->bus_type;
 
-	switch (vin->parallel->mbus_type) {
+	switch (vin->parallel.mbus_type) {
 	case V4L2_MBUS_PARALLEL:
 	case V4L2_MBUS_BT656:
 		vin_dbg(vin, "Found %s media bus\n",
-			vin->parallel->mbus_type == V4L2_MBUS_PARALLEL ?
+			vin->parallel.mbus_type == V4L2_MBUS_PARALLEL ?
 			"PARALLEL" : "BT656");
-		vin->parallel->bus = vep->bus.parallel;
+		vin->parallel.bus = vep->bus.parallel;
 		break;
 	default:
 		vin_err(vin, "Unknown media bus type\n");
 		return -EINVAL;
 	}
 
+	vin->parallel.asd = asd;
+
 	return 0;
 }
 
@@ -641,17 +640,17 @@ static int rvin_parallel_init(struct rvin_dev *vin)
 	v4l2_async_notifier_init(&vin->notifier);
 
 	ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
-		vin->dev, &vin->notifier, sizeof(struct rvin_parallel_entity),
+		vin->dev, &vin->notifier, sizeof(*vin->parallel.asd),
 		0, rvin_parallel_parse_v4l2);
 	if (ret)
 		return ret;
 
 	/* If using mc, it's fine not to have any input registered. */
-	if (!vin->parallel)
+	if (!vin->parallel.asd)
 		return vin->info->use_mc ? 0 : -ENODEV;
 
 	vin_dbg(vin, "Found parallel subdevice %pOF\n",
-		to_of_node(vin->parallel->asd.match.fwnode));
+		to_of_node(vin->parallel.asd->match.fwnode));
 
 	vin->notifier.ops = &rvin_parallel_notify_ops;
 	ret = v4l2_async_notifier_register(&vin->v4l2_dev, &vin->notifier);
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 5a5f0e5007478c8d..95b7828ee35231c6 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -672,7 +672,7 @@ static int rvin_setup(struct rvin_dev *vin)
 	case MEDIA_BUS_FMT_UYVY8_2X8:
 		/* BT.656 8bit YCbCr422 or BT.601 8bit YCbCr422 */
 		if (!vin->is_csi &&
-		    vin->parallel->mbus_type == V4L2_MBUS_BT656)
+		    vin->parallel.mbus_type == V4L2_MBUS_BT656)
 			vnmc |= VNMC_INF_YUV8_BT656;
 		else
 			vnmc |= VNMC_INF_YUV8_BT601;
@@ -685,7 +685,7 @@ static int rvin_setup(struct rvin_dev *vin)
 	case MEDIA_BUS_FMT_UYVY10_2X10:
 		/* BT.656 10bit YCbCr422 or BT.601 10bit YCbCr422 */
 		if (!vin->is_csi &&
-		    vin->parallel->mbus_type == V4L2_MBUS_BT656)
+		    vin->parallel.mbus_type == V4L2_MBUS_BT656)
 			vnmc |= VNMC_INF_YUV10_BT656;
 		else
 			vnmc |= VNMC_INF_YUV10_BT601;
@@ -710,21 +710,21 @@ static int rvin_setup(struct rvin_dev *vin)
 
 	if (!vin->is_csi) {
 		/* Hsync Signal Polarity Select */
-		if (!(vin->parallel->bus.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
+		if (!(vin->parallel.bus.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW))
 			dmr2 |= VNDMR2_HPS;
 
 		/* Vsync Signal Polarity Select */
-		if (!(vin->parallel->bus.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
+		if (!(vin->parallel.bus.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW))
 			dmr2 |= VNDMR2_VPS;
 
 		/* Data Enable Polarity Select */
-		if (vin->parallel->bus.flags & V4L2_MBUS_DATA_ENABLE_LOW)
+		if (vin->parallel.bus.flags & V4L2_MBUS_DATA_ENABLE_LOW)
 			dmr2 |= VNDMR2_CES;
 
 		switch (vin->mbus_code) {
 		case MEDIA_BUS_FMT_UYVY8_2X8:
-			if (vin->parallel->bus.bus_width == 8 &&
-			    vin->parallel->bus.data_shift == 8)
+			if (vin->parallel.bus.bus_width == 8 &&
+			    vin->parallel.bus.data_shift == 8)
 				dmr2 |= VNDMR2_YDS;
 			break;
 		default:
@@ -1203,7 +1203,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->parallel->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 3e7a3ae2a6b97045..e6ea2b7991b8dcb3 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -205,7 +205,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
 {
 	struct v4l2_subdev_format fmt = {
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
-		.pad = vin->parallel->source_pad,
+		.pad = vin->parallel.source_pad,
 	};
 	int ret;
 
@@ -246,7 +246,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->parallel->source_pad,
+		.pad = vin->parallel.source_pad,
 	};
 	enum v4l2_field field;
 	u32 width, height;
@@ -632,7 +632,7 @@ static int rvin_enum_dv_timings(struct file *file, void *priv_fh,
 	if (timings->pad)
 		return -EINVAL;
 
-	timings->pad = vin->parallel->sink_pad;
+	timings->pad = vin->parallel.sink_pad;
 
 	ret = v4l2_subdev_call(sd, pad, enum_dv_timings, timings);
 
@@ -684,7 +684,7 @@ static int rvin_dv_timings_cap(struct file *file, void *priv_fh,
 	if (cap->pad)
 		return -EINVAL;
 
-	cap->pad = vin->parallel->sink_pad;
+	cap->pad = vin->parallel.sink_pad;
 
 	ret = v4l2_subdev_call(sd, pad, dv_timings_cap, cap);
 
@@ -702,7 +702,7 @@ static int rvin_g_edid(struct file *file, void *fh, struct v4l2_edid *edid)
 	if (edid->pad)
 		return -EINVAL;
 
-	edid->pad = vin->parallel->sink_pad;
+	edid->pad = vin->parallel.sink_pad;
 
 	ret = v4l2_subdev_call(sd, pad, get_edid, edid);
 
@@ -720,7 +720,7 @@ static int rvin_s_edid(struct file *file, void *fh, struct v4l2_edid *edid)
 	if (edid->pad)
 		return -EINVAL;
 
-	edid->pad = vin->parallel->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 4539bd53d9d41e9c..34e3979acf931b67 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -101,7 +101,7 @@ struct rvin_video_format {
  *
  */
 struct rvin_parallel_entity {
-	struct v4l2_async_subdev asd;
+	struct v4l2_async_subdev *asd;
 	struct v4l2_subdev *subdev;
 
 	enum v4l2_mbus_type mbus_type;
@@ -213,7 +213,7 @@ struct rvin_dev {
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct v4l2_async_notifier notifier;
 
-	struct rvin_parallel_entity *parallel;
+	struct rvin_parallel_entity parallel;
 
 	struct rvin_group *group;
 	unsigned int id;
@@ -248,7 +248,7 @@ struct rvin_dev {
 	unsigned int alpha;
 };
 
-#define vin_to_source(vin)		((vin)->parallel->subdev)
+#define vin_to_source(vin)		((vin)->parallel.subdev)
 
 /* Debug */
 #define vin_dbg(d, fmt, arg...)		dev_dbg(d->dev, fmt, ##arg)
-- 
2.29.2

[PATCH 2/5] rcar-vin: Rework parallel firmware parsing

[Niklas Söderlund]

Rework the parallel firmware parsing code to not use the soon to be
removed v4l2_async_notifier_parse_fwnode_endpoints_by_port() helper. The
change only aims to prepare for the removing of the old helper and there
are no functional change.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 07f250bfc0051135..396ff5531359f3f4 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -604,32 +604,56 @@ static const struct v4l2_async_notifier_operations rvin_parallel_notify_ops = {
 	.complete = rvin_parallel_notify_complete,
 };
 
-static int rvin_parallel_parse_v4l2(struct device *dev,
-				    struct v4l2_fwnode_endpoint *vep,
-				    struct v4l2_async_subdev *asd)
+static int rvin_parallel_parse_of(struct rvin_dev *vin)
 {
-	struct rvin_dev *vin = dev_get_drvdata(dev);
+	struct fwnode_handle *ep, *fwnode;
+	struct v4l2_fwnode_endpoint vep = {
+		.bus_type = V4L2_MBUS_UNKNOWN,
+	};
+	struct v4l2_async_subdev *asd;
+	int ret;
 
-	if (vep->base.port || vep->base.id)
-		return -ENOTCONN;
+	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);
+	if (!ep)
+		return 0;
 
-	vin->parallel.mbus_type = vep->bus_type;
+	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;
+	}
 
-	switch (vin->parallel.mbus_type) {
+	switch (vep.bus_type) {
 	case V4L2_MBUS_PARALLEL:
 	case V4L2_MBUS_BT656:
 		vin_dbg(vin, "Found %s media bus\n",
-			vin->parallel.mbus_type == V4L2_MBUS_PARALLEL ?
+			vep.bus_type == V4L2_MBUS_PARALLEL ?
 			"PARALLEL" : "BT656");
-		vin->parallel.bus = vep->bus.parallel;
+		vin->parallel.mbus_type = vep.bus_type;
+		vin->parallel.bus = vep.bus.parallel;
 		break;
 	default:
 		vin_err(vin, "Unknown media bus type\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out;
+	}
+
+	asd = v4l2_async_notifier_add_fwnode_subdev(&vin->notifier, fwnode,
+						    sizeof(*asd));
+	if (IS_ERR(asd)) {
+		ret = PTR_ERR(asd);
+		goto out;
 	}
 
 	vin->parallel.asd = asd;
 
+	vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));
+out:
+	fwnode_handle_put(fwnode);
+
 	return 0;
 }
 
@@ -639,9 +663,7 @@ static int rvin_parallel_init(struct rvin_dev *vin)
 
 	v4l2_async_notifier_init(&vin->notifier);
 
-	ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
-		vin->dev, &vin->notifier, sizeof(*vin->parallel.asd),
-		0, rvin_parallel_parse_v4l2);
+	ret = rvin_parallel_parse_of(vin);
 	if (ret)
 		return ret;
 
-- 
2.29.2

Re: [PATCH 2/5] rcar-vin: Rework parallel firmware parsing

[Jacopo Mondi]

Hi Niklas,

On Wed, Nov 25, 2020 at 05:44:47PM +0100, Niklas Söderlund wrote:
> Rework the parallel firmware parsing code to not use the soon to be
> removed v4l2_async_notifier_parse_fwnode_endpoints_by_port() helper. The
> change only aims to prepare for the removing of the old helper and there
> are no functional change.

nit: 'changes' as you use 'are'

>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 50 +++++++++++++++------
>  1 file changed, 36 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 07f250bfc0051135..396ff5531359f3f4 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -604,32 +604,56 @@ static const struct v4l2_async_notifier_operations rvin_parallel_notify_ops = {
>  	.complete = rvin_parallel_notify_complete,
>  };
>
> -static int rvin_parallel_parse_v4l2(struct device *dev,
> -				    struct v4l2_fwnode_endpoint *vep,
> -				    struct v4l2_async_subdev *asd)
> +static int rvin_parallel_parse_of(struct rvin_dev *vin)
>  {
> -	struct rvin_dev *vin = dev_get_drvdata(dev);
> +	struct fwnode_handle *ep, *fwnode;
> +	struct v4l2_fwnode_endpoint vep = {
> +		.bus_type = V4L2_MBUS_UNKNOWN,
> +	};

Afaict bus autodiscovery is kind of deprectated. Unfortunately we
don't enforce the "bus-type" property presence in DTS, so it's very hard
to identify mis-configurations and set defaults (which we know and
should set). I think this calls for a slight rework of this part with
an associated bindings update.

We assume DTS are correct, so this is not pressing, but currently we
don't have any validation in place.

For later, anyway.

> +	struct v4l2_async_subdev *asd;
> +	int ret;
>
> -	if (vep->base.port || vep->base.id)
> -		return -ENOTCONN;
> +	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 0, 0, 0);

get_next_endpoint() would do the same afaict, but being explicit is
probably better

> +	if (!ep)
> +		return 0;
>
> -	vin->parallel.mbus_type = vep->bus_type;
> +	fwnode = fwnode_graph_get_remote_endpoint(ep);

We're matching a parallel subdevice, which usually registers its async
subdev on the device node, not on endpoints.

In facts v4l2_async_notifier_fwnode_parse_endpoint() which is in the
call path of the v4l2_async_notifier_parse_fwnode_endpoints_by_port()
you are removing does:

	asd->match.fwnode =
		fwnode_graph_get_remote_port_parent(endpoint);

We now have match_fwnode() that adjusts endpoints to be matched
against the remote's parent but this still feels like a workaround as
most subdevs in mainline (all but adv748x?) still match on device
node.

I wonder how many system would actually break if we change
        v4l2_async_register_subdev[_sensor_common]()
to use the first available endpoint as match target.

> +	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;
> +	}
>
> -	switch (vin->parallel.mbus_type) {
> +	switch (vep.bus_type) {
>  	case V4L2_MBUS_PARALLEL:
>  	case V4L2_MBUS_BT656:
>  		vin_dbg(vin, "Found %s media bus\n",
> -			vin->parallel.mbus_type == V4L2_MBUS_PARALLEL ?
> +			vep.bus_type == V4L2_MBUS_PARALLEL ?
>  			"PARALLEL" : "BT656");
> -		vin->parallel.bus = vep->bus.parallel;
> +		vin->parallel.mbus_type = vep.bus_type;
> +		vin->parallel.bus = vep.bus.parallel;
>  		break;
>  	default:
>  		vin_err(vin, "Unknown media bus type\n");
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	asd = v4l2_async_notifier_add_fwnode_subdev(&vin->notifier, fwnode,
> +						    sizeof(*asd));
> +	if (IS_ERR(asd)) {
> +		ret = PTR_ERR(asd);
> +		goto out;
>  	}
>
>  	vin->parallel.asd = asd;
>
> +	vin_dbg(vin, "Add parallel OF device %pOF\n", to_of_node(fwnode));

You could omit OF as it is implied :)

> +out:
> +	fwnode_handle_put(fwnode);
> +
>  	return 0;
>  }
>
> @@ -639,9 +663,7 @@ static int rvin_parallel_init(struct rvin_dev *vin)
>
>  	v4l2_async_notifier_init(&vin->notifier);
>
> -	ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> -		vin->dev, &vin->notifier, sizeof(*vin->parallel.asd),
> -		0, rvin_parallel_parse_v4l2);
> +	ret = rvin_parallel_parse_of(vin);

Patch is very nice, I like this direction
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
   j

>  	if (ret)
>  		return ret;
>
> --
> 2.29.2
>

[PATCH 3/5] rcar-vin: Use v4l2_async_subdev instead of fwnode_handle to match subdevices

[Niklas Söderlund]

In preparation of removing the usage of the old helper
v4l2_async_notifier_parse_fwnode_endpoints_by_port() use the
v4l2_async_subdev instead of fwnode_handle to match subdevices.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 396ff5531359f3f4..830ab0865967310b 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -772,7 +772,7 @@ 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].fwnode != asd->match.fwnode)
+		if (vin->group->csi[i].asd != asd)
 			continue;
 		vin->group->csi[i].subdev = NULL;
 		vin_dbg(vin, "Unbind CSI-2 %s from slot %u\n", subdev->name, i);
@@ -794,7 +794,7 @@ 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].fwnode != asd->match.fwnode)
+		if (vin->group->csi[i].asd != asd)
 			continue;
 		vin->group->csi[i].subdev = subdev;
 		vin_dbg(vin, "Bound CSI-2 %s to slot %u\n", subdev->name, i);
@@ -830,14 +830,14 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
 
 	mutex_lock(&vin->group->lock);
 
-	if (vin->group->csi[vep->base.id].fwnode) {
+	if (vin->group->csi[vep->base.id].asd) {
 		vin_dbg(vin, "OF device %pOF already handled\n",
 			to_of_node(asd->match.fwnode));
 		ret = -ENOTCONN;
 		goto out;
 	}
 
-	vin->group->csi[vep->base.id].fwnode = asd->match.fwnode;
+	vin->group->csi[vep->base.id].asd = asd;
 
 	vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
 		to_of_node(asd->match.fwnode), vep->base.id);
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 34e3979acf931b67..0ee9d402f5aceaf5 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -280,7 +280,7 @@ struct rvin_group {
 	struct rvin_dev *vin[RCAR_VIN_NUM];
 
 	struct {
-		struct fwnode_handle *fwnode;
+		struct v4l2_async_subdev *asd;
 		struct v4l2_subdev *subdev;
 	} csi[RVIN_CSI_MAX];
 };
-- 
2.29.2

[PATCH 4/5] rcar-vin: Rework CSI-2 firmware parsing

[Niklas Söderlund]

Rework the CSI-2 firmware parsing code to not use the soon to be
removed v4l2_async_notifier_parse_fwnode_endpoints_by_port() helper. The
change only aims to prepare for the removing of the old helper and there
are no functional change.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 830ab0865967310b..98bff765b02e67d9 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -812,37 +812,48 @@ static const struct v4l2_async_notifier_operations rvin_group_notify_ops = {
 	.complete = rvin_group_notify_complete,
 };
 
-static int rvin_mc_parse_of_endpoint(struct device *dev,
-				     struct v4l2_fwnode_endpoint *vep,
-				     struct v4l2_async_subdev *asd)
+static int rvin_mc_parse_of(struct rvin_dev *vin, unsigned int id)
 {
-	struct rvin_dev *vin = dev_get_drvdata(dev);
-	int ret = 0;
+	struct fwnode_handle *ep, *fwnode;
+	struct v4l2_fwnode_endpoint vep = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY,
+	};
+	struct v4l2_async_subdev *asd;
+	int ret;
 
-	if (vep->base.port != 1 || vep->base.id >= RVIN_CSI_MAX)
-		return -EINVAL;
+	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 1, id, 0);
+	if (!ep)
+		return 0;
 
-	if (!of_device_is_available(to_of_node(asd->match.fwnode))) {
+	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(asd->match.fwnode));
-		return -ENOTCONN;
-	}
-
-	mutex_lock(&vin->group->lock);
-
-	if (vin->group->csi[vep->base.id].asd) {
-		vin_dbg(vin, "OF device %pOF already handled\n",
-			to_of_node(asd->match.fwnode));
+			to_of_node(fwnode));
 		ret = -ENOTCONN;
 		goto out;
 	}
 
-	vin->group->csi[vep->base.id].asd = asd;
+	asd = v4l2_async_notifier_add_fwnode_subdev(&vin->group->notifier,
+						    fwnode, sizeof(*asd));
+	if (IS_ERR(asd)) {
+		ret = PTR_ERR(asd);
+		goto out;
+	}
+
+	vin->group->csi[vep.base.id].asd = asd;
 
 	vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
-		to_of_node(asd->match.fwnode), vep->base.id);
+		to_of_node(fwnode), vep.base.id);
 out:
-	mutex_unlock(&vin->group->lock);
+	fwnode_handle_put(fwnode);
 
 	return ret;
 }
@@ -850,7 +861,7 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
 static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
 {
 	unsigned int count = 0, vin_mask = 0;
-	unsigned int i;
+	unsigned int i, id;
 	int ret;
 
 	mutex_lock(&vin->group->lock);
@@ -881,12 +892,14 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
 		if (!(vin_mask & BIT(i)))
 			continue;
 
-		ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
-				vin->group->vin[i]->dev, &vin->group->notifier,
-				sizeof(struct v4l2_async_subdev), 1,
-				rvin_mc_parse_of_endpoint);
-		if (ret)
-			return ret;
+		for (id = 0; id < RVIN_CSI_MAX; id++) {
+			if (vin->group->csi[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))
-- 
2.29.2

Re: [PATCH 4/5] rcar-vin: Rework CSI-2 firmware parsing

[Sakari Ailus]

Hejssan Niklas,

Tack för detta hop av lappar! De är verkligen jättebehagliga!

On Wed, Nov 25, 2020 at 05:44:49PM +0100, Niklas Söderlund wrote:
> Rework the CSI-2 firmware parsing code to not use the soon to be
> removed v4l2_async_notifier_parse_fwnode_endpoints_by_port() helper. The
> change only aims to prepare for the removing of the old helper and there
> are no functional change.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 67 ++++++++++++---------
>  1 file changed, 40 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 830ab0865967310b..98bff765b02e67d9 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -812,37 +812,48 @@ static const struct v4l2_async_notifier_operations rvin_group_notify_ops = {
>  	.complete = rvin_group_notify_complete,
>  };
>  
> -static int rvin_mc_parse_of_endpoint(struct device *dev,
> -				     struct v4l2_fwnode_endpoint *vep,
> -				     struct v4l2_async_subdev *asd)
> +static int rvin_mc_parse_of(struct rvin_dev *vin, unsigned int id)
>  {
> -	struct rvin_dev *vin = dev_get_drvdata(dev);
> -	int ret = 0;
> +	struct fwnode_handle *ep, *fwnode;
> +	struct v4l2_fwnode_endpoint vep = {
> +		.bus_type = V4L2_MBUS_CSI2_DPHY,
> +	};

This would fit on a single line.

> +	struct v4l2_async_subdev *asd;
> +	int ret;
>  
> -	if (vep->base.port != 1 || vep->base.id >= RVIN_CSI_MAX)
> -		return -EINVAL;
> +	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(vin->dev), 1, id, 0);

You could use the FWNODE_GRAPH_ENDPOINT_NEXT flag to get the next endpoint
instead of specifying its number. Whichever works better...

> +	if (!ep)
> +		return 0;
>  
> -	if (!of_device_is_available(to_of_node(asd->match.fwnode))) {
> +	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))) {

fwnode is an endpoint here, not the device node.

But there's no need for this check either, as
fwnode_graph_get_endpoint_by_id(), by default, only returns endpoints that
are connected to available device's endpoints. So you can remove this
check.

>  		vin_dbg(vin, "OF device %pOF disabled, ignoring\n",
> -			to_of_node(asd->match.fwnode));
> -		return -ENOTCONN;
> -	}
> -
> -	mutex_lock(&vin->group->lock);
> -
> -	if (vin->group->csi[vep->base.id].asd) {
> -		vin_dbg(vin, "OF device %pOF already handled\n",
> -			to_of_node(asd->match.fwnode));
> +			to_of_node(fwnode));
>  		ret = -ENOTCONN;
>  		goto out;
>  	}
>  
> -	vin->group->csi[vep->base.id].asd = asd;
> +	asd = v4l2_async_notifier_add_fwnode_subdev(&vin->group->notifier,
> +						    fwnode, sizeof(*asd));
> +	if (IS_ERR(asd)) {
> +		ret = PTR_ERR(asd);
> +		goto out;
> +	}
> +
> +	vin->group->csi[vep.base.id].asd = asd;
>  
>  	vin_dbg(vin, "Add group OF device %pOF to slot %u\n",
> -		to_of_node(asd->match.fwnode), vep->base.id);
> +		to_of_node(fwnode), vep.base.id);
>  out:
> -	mutex_unlock(&vin->group->lock);
> +	fwnode_handle_put(fwnode);
>  
>  	return ret;
>  }
> @@ -850,7 +861,7 @@ static int rvin_mc_parse_of_endpoint(struct device *dev,
>  static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  {
>  	unsigned int count = 0, vin_mask = 0;
> -	unsigned int i;
> +	unsigned int i, id;
>  	int ret;
>  
>  	mutex_lock(&vin->group->lock);
> @@ -881,12 +892,14 @@ static int rvin_mc_parse_of_graph(struct rvin_dev *vin)
>  		if (!(vin_mask & BIT(i)))
>  			continue;
>  
> -		ret = v4l2_async_notifier_parse_fwnode_endpoints_by_port(
> -				vin->group->vin[i]->dev, &vin->group->notifier,
> -				sizeof(struct v4l2_async_subdev), 1,
> -				rvin_mc_parse_of_endpoint);
> -		if (ret)
> -			return ret;
> +		for (id = 0; id < RVIN_CSI_MAX; id++) {
> +			if (vin->group->csi[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))

-- 
Vänliga hälsningar,

Sakari Ailus

[PATCH 5/5] v4l2-fwnode: Remove v4l2_async_notifier_parse_fwnode_endpoints_by_port()

[Niklas Söderlund]

There are no users left of this helper and as it implements an
undesirable and too simple behaviour that should instead be implemented
directly by drivers remove it to prevent future uses of it.

Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/v4l2-core/v4l2-fwnode.c | 14 -------
 include/media/v4l2-fwnode.h           | 53 ---------------------------
 2 files changed, 67 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 44dd04b05e2970ab..5353e37eb950e813 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -911,20 +911,6 @@ v4l2_async_notifier_parse_fwnode_endpoints(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);
 
-int
-v4l2_async_notifier_parse_fwnode_endpoints_by_port(struct device *dev,
-						   struct v4l2_async_notifier *notifier,
-						   size_t asd_struct_size,
-						   unsigned int port,
-						   parse_endpoint_func parse_endpoint)
-{
-	return __v4l2_async_notifier_parse_fwnode_ep(dev, notifier,
-						     asd_struct_size,
-						     port, true,
-						     parse_endpoint);
-}
-EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints_by_port);
-
 /*
  * v4l2_fwnode_reference_parse - parse references for async sub-devices
  * @dev: the device node the properties of which are parsed for references
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index 4e1f6e1d847ec864..4365430eea6f3802 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -484,59 +484,6 @@ v4l2_async_notifier_parse_fwnode_endpoints(struct device *dev,
 					   size_t asd_struct_size,
 					   parse_endpoint_func parse_endpoint);
 
-/**
- * v4l2_async_notifier_parse_fwnode_endpoints_by_port - Parse V4L2 fwnode
- *							endpoints of a port in a
- *							device node
- * @dev: the device the endpoints of which are to be parsed
- * @notifier: notifier for @dev
- * @asd_struct_size: size of the driver's async sub-device struct, including
- *		     sizeof(struct v4l2_async_subdev). The &struct
- *		     v4l2_async_subdev shall be the first member of
- *		     the driver's async sub-device struct, i.e. both
- *		     begin at the same memory address.
- * @port: port number where endpoints are to be parsed
- * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
- *		    endpoint. Optional.
- *
- * This function is just like v4l2_async_notifier_parse_fwnode_endpoints() with
- * the exception that it only parses endpoints in a given port. This is useful
- * on devices that have both sinks and sources: the async sub-devices connected
- * to sources have already been configured by another driver (on capture
- * devices). In this case the driver must know which ports to parse.
- *
- * Parse the fwnode endpoints of the @dev device on a given @port and populate
- * the async sub-devices list of the notifier. The @parse_endpoint callback
- * function is called for each endpoint with the corresponding async sub-device
- * pointer to let the caller initialize the driver-specific part of the async
- * sub-device structure.
- *
- * The notifier memory shall be zeroed before this function is called on the
- * notifier the first time.
- *
- * This function may not be called on a registered notifier and may be called on
- * a notifier only once per port.
- *
- * The &struct v4l2_fwnode_endpoint passed to the callback function
- * @parse_endpoint is released once the function is finished. If there is a need
- * to retain that configuration, the user needs to allocate memory for it.
- *
- * Any notifier populated using this function must be released with a call to
- * v4l2_async_notifier_cleanup() after it has been unregistered and the async
- * sub-devices are no longer in use, even if the function returned an error.
- *
- * Return: %0 on success, including when no async sub-devices are found
- *	   %-ENOMEM if memory allocation failed
- *	   %-EINVAL if graph or endpoint parsing failed
- *	   Other error codes as returned by @parse_endpoint
- */
-int
-v4l2_async_notifier_parse_fwnode_endpoints_by_port(struct device *dev,
-						   struct v4l2_async_notifier *notifier,
-						   size_t asd_struct_size,
-						   unsigned int port,
-						   parse_endpoint_func parse_endpoint);
-
 /**
  * v4l2_async_notifier_parse_fwnode_sensor_common - parse common references on
  *					       sensors for async sub-devices
-- 
2.29.2

Re: [PATCH 5/5] v4l2-fwnode: Remove v4l2_async_notifier_parse_fwnode_endpoints_by_port()

[Sakari Ailus]

Hejssan,

Tack för dessa lappar!

On Wed, Nov 25, 2020 at 05:44:50PM +0100, Niklas Söderlund wrote:
> There are no users left of this helper and as it implements an
> undesirable and too simple behaviour that should instead be implemented
> directly by drivers remove it to prevent future uses of it.
> 
> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 14 -------
>  include/media/v4l2-fwnode.h           | 53 ---------------------------
>  2 files changed, 67 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 44dd04b05e2970ab..5353e37eb950e813 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -911,20 +911,6 @@ v4l2_async_notifier_parse_fwnode_endpoints(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints);
>  
> -int
> -v4l2_async_notifier_parse_fwnode_endpoints_by_port(struct device *dev,
> -						   struct v4l2_async_notifier *notifier,
> -						   size_t asd_struct_size,
> -						   unsigned int port,
> -						   parse_endpoint_func parse_endpoint)
> -{
> -	return __v4l2_async_notifier_parse_fwnode_ep(dev, notifier,
> -						     asd_struct_size,
> -						     port, true,
> -						     parse_endpoint);
> -}
> -EXPORT_SYMBOL_GPL(v4l2_async_notifier_parse_fwnode_endpoints_by_port);

Could you also merge __v4l2_async_notifier_parse_fwnode_ep with
v4l2_async_notifier_parse_fwnode_endpoints as well? I'd do it in a separate
patch though.

> -
>  /*
>   * v4l2_fwnode_reference_parse - parse references for async sub-devices
>   * @dev: the device node the properties of which are parsed for references
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index 4e1f6e1d847ec864..4365430eea6f3802 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -484,59 +484,6 @@ v4l2_async_notifier_parse_fwnode_endpoints(struct device *dev,
>  					   size_t asd_struct_size,
>  					   parse_endpoint_func parse_endpoint);
>  
> -/**
> - * v4l2_async_notifier_parse_fwnode_endpoints_by_port - Parse V4L2 fwnode
> - *							endpoints of a port in a
> - *							device node
> - * @dev: the device the endpoints of which are to be parsed
> - * @notifier: notifier for @dev
> - * @asd_struct_size: size of the driver's async sub-device struct, including
> - *		     sizeof(struct v4l2_async_subdev). The &struct
> - *		     v4l2_async_subdev shall be the first member of
> - *		     the driver's async sub-device struct, i.e. both
> - *		     begin at the same memory address.
> - * @port: port number where endpoints are to be parsed
> - * @parse_endpoint: Driver's callback function called on each V4L2 fwnode
> - *		    endpoint. Optional.
> - *
> - * This function is just like v4l2_async_notifier_parse_fwnode_endpoints() with
> - * the exception that it only parses endpoints in a given port. This is useful
> - * on devices that have both sinks and sources: the async sub-devices connected
> - * to sources have already been configured by another driver (on capture
> - * devices). In this case the driver must know which ports to parse.
> - *
> - * Parse the fwnode endpoints of the @dev device on a given @port and populate
> - * the async sub-devices list of the notifier. The @parse_endpoint callback
> - * function is called for each endpoint with the corresponding async sub-device
> - * pointer to let the caller initialize the driver-specific part of the async
> - * sub-device structure.
> - *
> - * The notifier memory shall be zeroed before this function is called on the
> - * notifier the first time.
> - *
> - * This function may not be called on a registered notifier and may be called on
> - * a notifier only once per port.
> - *
> - * The &struct v4l2_fwnode_endpoint passed to the callback function
> - * @parse_endpoint is released once the function is finished. If there is a need
> - * to retain that configuration, the user needs to allocate memory for it.
> - *
> - * Any notifier populated using this function must be released with a call to
> - * v4l2_async_notifier_cleanup() after it has been unregistered and the async
> - * sub-devices are no longer in use, even if the function returned an error.
> - *
> - * Return: %0 on success, including when no async sub-devices are found
> - *	   %-ENOMEM if memory allocation failed
> - *	   %-EINVAL if graph or endpoint parsing failed
> - *	   Other error codes as returned by @parse_endpoint
> - */
> -int
> -v4l2_async_notifier_parse_fwnode_endpoints_by_port(struct device *dev,
> -						   struct v4l2_async_notifier *notifier,
> -						   size_t asd_struct_size,
> -						   unsigned int port,
> -						   parse_endpoint_func parse_endpoint);
> -
>  /**
>   * v4l2_async_notifier_parse_fwnode_sensor_common - parse common references on
>   *					       sensors for async sub-devices
> -- 
> 2.29.2
> 

-- 
Sakari Ailus

Re: [PATCH 0/5] v4l2-fwnode: Remove v4l2_async_notifier_parse_fwnode_endpoints_by_port() helper

[Jacopo Mondi]

Hi Niklas, Sakari,

On Wed, Nov 25, 2020 at 05:44:45PM +0100, Niklas Söderlund wrote:
> Hello,
>
> This series aims to remove a V4L2 helper that provides a too simple
> implementation, v4l2_async_notifier_parse_fwnode_endpoints_by_port().
> Instead drivers shall implement what the helper does directly to get
> greater control of the process. There is only one user left in tree of
> this interface, R-Car VIN.

What is the plan going forward ?
removing v4l2_async_notifier_parse_fwnode_endpoints_by_port() here
then remove v4l2_async_notifier_parse_fwnode_endpoints() as it has a
single user in mainline too ?

Are we standardizing all platform drivers to use
v4l2_async_notifier_add_fwnode_subdev() and perform fwnode.match
initialization by themselves or is there a plan to replace
v4l2_async_notifier_parse_fwnode_endpoints*() with something else ?

Thanks
  j

>
> This series starts therefor to rework the R-Car VIN driver to not depend
> on the helper. And in the process a small fwnode imbalance is fixed.
> After the last user of the helper is reworked the series removes the
> helper to prevent usage of it to resurface.
>
> This series is based on-top of the latest media tree and is tested on
> Renesas R-Car M3-N and Koelsch without any regressions or change in
> behavior detected.
>
> Niklas Söderlund (5):
>   rcar-vin: Only dynamically allocate v4l2_async_subdev
>   rcar-vin: Rework parallel firmware parsing
>   rcar-vin: Use v4l2_async_subdev instead of fwnode_handle to match
>     subdevices
>   rcar-vin: Rework CSI-2 firmware parsing
>   v4l2-fwnode: Remove
>     v4l2_async_notifier_parse_fwnode_endpoints_by_port()
>
>  drivers/media/platform/rcar-vin/rcar-core.c | 156 ++++++++++++--------
>  drivers/media/platform/rcar-vin/rcar-dma.c  |  16 +-
>  drivers/media/platform/rcar-vin/rcar-v4l2.c |  12 +-
>  drivers/media/platform/rcar-vin/rcar-vin.h  |   8 +-
>  drivers/media/v4l2-core/v4l2-fwnode.c       |  14 --
>  include/media/v4l2-fwnode.h                 |  53 -------
>  6 files changed, 113 insertions(+), 146 deletions(-)
>
> --
> 2.29.2
>

Re: [PATCH 0/5] v4l2-fwnode: Remove v4l2_async_notifier_parse_fwnode_endpoints_by_port() helper

[Sakari Ailus]

Hi Jacopo,

On Thu, Nov 26, 2020 at 11:12:51AM +0100, Jacopo Mondi wrote:
> Hi Niklas, Sakari,
> 
> On Wed, Nov 25, 2020 at 05:44:45PM +0100, Niklas Söderlund wrote:
> > Hello,
> >
> > This series aims to remove a V4L2 helper that provides a too simple
> > implementation, v4l2_async_notifier_parse_fwnode_endpoints_by_port().
> > Instead drivers shall implement what the helper does directly to get
> > greater control of the process. There is only one user left in tree of
> > this interface, R-Car VIN.
> 
> What is the plan going forward ?
> removing v4l2_async_notifier_parse_fwnode_endpoints_by_port() here
> then remove v4l2_async_notifier_parse_fwnode_endpoints() as it has a
> single user in mainline too ?
> 
> Are we standardizing all platform drivers to use
> v4l2_async_notifier_add_fwnode_subdev() and perform fwnode.match
> initialization by themselves or is there a plan to replace

Yes, please.

> v4l2_async_notifier_parse_fwnode_endpoints*() with something else ?

That's always been somewhat clunky and required special cases. The other
option, i.e. what this patchset does, is straightforward as well as allows
setting defaults in drivers, and admittedly, comes with a little bit of
extra code in drivers in areas that are driver specific. The old functions
such as v4l2_async_notifier_parse_fwnode_endpoints() just pretended they
were not...

-- 
Regards,

Sakari Ailus

Re: [PATCH 0/5] v4l2-fwnode: Remove v4l2_async_notifier_parse_fwnode_endpoints_by_port() helper

[Jacopo Mondi]

Hi Sakari,

On Thu, Nov 26, 2020 at 12:22:05PM +0200, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Thu, Nov 26, 2020 at 11:12:51AM +0100, Jacopo Mondi wrote:
> > Hi Niklas, Sakari,
> >
> > On Wed, Nov 25, 2020 at 05:44:45PM +0100, Niklas Söderlund wrote:
> > > Hello,
> > >
> > > This series aims to remove a V4L2 helper that provides a too simple
> > > implementation, v4l2_async_notifier_parse_fwnode_endpoints_by_port().
> > > Instead drivers shall implement what the helper does directly to get
> > > greater control of the process. There is only one user left in tree of
> > > this interface, R-Car VIN.
> >
> > What is the plan going forward ?
> > removing v4l2_async_notifier_parse_fwnode_endpoints_by_port() here
> > then remove v4l2_async_notifier_parse_fwnode_endpoints() as it has a
> > single user in mainline too ?
> >
> > Are we standardizing all platform drivers to use
> > v4l2_async_notifier_add_fwnode_subdev() and perform fwnode.match
> > initialization by themselves or is there a plan to replace
>
> Yes, please.
>
> > v4l2_async_notifier_parse_fwnode_endpoints*() with something else ?
>
> That's always been somewhat clunky and required special cases. The other
> option, i.e. what this patchset does, is straightforward as well as allows
> setting defaults in drivers, and admittedly, comes with a little bit of
> extra code in drivers in areas that are driver specific. The old functions
> such as v4l2_async_notifier_parse_fwnode_endpoints() just pretended they
> were not...

Agreed in full :)
(At the expense of a little extra code in drivers)

>
> --
> Regards,
>
> Sakari Ailus