[PATCH v3 0/3] Unified fwnode endpoint parser

[PATCH v3 0/3] Unified fwnode endpoint parser

[Sakari Ailus]

Hi folks,

We have a large influx of new, unmerged, drivers that are now parsing
fwnode endpoints and each one of them is doing this a little bit
differently. The needs are still exactly the same for the graph data
structure is device independent. This is still a non-trivial task and the
majority of the driver implementations are buggy, just buggy in different
ways.

Facilitate parsing endpoints by adding a convenience function for parsing
the endpoints, and make the omap3isp driver use it as an example.

I plan to include the first patch to a pull request soonish, the second
could go in with the first user.

since v2:

- Rebase on CCP2 support patches.

- Prepend a patch cleaning up omap3isp driver a little.

since v1:

- The first patch has been merged (it was a bugfix).

- In anticipation that the parsing can take place over several iterations,
  take the existing number of async sub-devices into account when
  re-allocating an array of async sub-devices.

- Rework the first patch to better anticipate parsing single endpoint at a
  time by a driver.

- Add a second patch that adds a function for parsing endpoints one at a
  time based on port and endpoint numbers.

Sakari Ailus (3):
  omap3isp: Drop redundant isp->subdevs field and ISP_MAX_SUBDEVS
  v4l: fwnode: Support generic parsing of graph endpoints in a device
  v4l: fwnode: Support generic parsing of graph endpoints in a single
    port

 drivers/media/platform/omap3isp/isp.c | 116 +++++++---------------
 drivers/media/platform/omap3isp/isp.h |   3 -
 drivers/media/v4l2-core/v4l2-fwnode.c | 176 ++++++++++++++++++++++++++++++++++
 include/media/v4l2-async.h            |   4 +-
 include/media/v4l2-fwnode.h           |  16 ++++
 5 files changed, 231 insertions(+), 84 deletions(-)

-- 
2.11.0

[PATCH v3 1/3] omap3isp: Drop redundant isp->subdevs field and ISP_MAX_SUBDEVS

[Sakari Ailus]

struct omap3isp.subdevs field and ISP_MAX_SUBDEVS macro are both unused.
Remove them.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/isp.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
index e528df6efc09..848cd96b67ca 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -220,9 +220,6 @@ struct isp_device {
 
 	unsigned int sbl_resources;
 	unsigned int subclk_resources;
-
-#define ISP_MAX_SUBDEVS		8
-	struct v4l2_subdev *subdevs[ISP_MAX_SUBDEVS];
 };
 
 struct isp_async_subdev {
-- 
2.11.0

[PATCH v3 2/3] v4l: fwnode: Support generic parsing of graph endpoints in a device

[Sakari Ailus]

The current practice is that drivers iterate over their endpoints and
parse each endpoint separately. This is very similar in a number of
drivers, implement a generic function for the job. Driver specific matters
can be taken into account in the driver specific callback.

Convert the omap3isp as an example.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/isp.c | 116 ++++++++++---------------------
 drivers/media/v4l2-core/v4l2-fwnode.c | 125 ++++++++++++++++++++++++++++++++++
 include/media/v4l2-async.h            |   4 +-
 include/media/v4l2-fwnode.h           |   9 +++
 4 files changed, 173 insertions(+), 81 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index c3014c82d64d..3ed213b82eaa 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2011,44 +2011,41 @@ enum isp_of_phy {
 	ISP_OF_PHY_CSIPHY2,
 };
 
-static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
-			    struct isp_async_subdev *isd)
+static int isp_fwnode_parse(struct device *dev,
+			    struct v4l2_fwnode_endpoint *vep,
+			    struct v4l2_async_subdev *asd)
 {
+	struct isp_async_subdev *isd =
+		container_of(asd, struct isp_async_subdev, asd);
 	struct isp_bus_cfg *buscfg = &isd->bus;
-	struct v4l2_fwnode_endpoint vep;
-	unsigned int i;
-	int ret;
 	bool csi1 = false;
-
-	ret = v4l2_fwnode_endpoint_parse(fwnode, &vep);
-	if (ret)
-		return ret;
+	unsigned int i;
 
 	dev_dbg(dev, "parsing endpoint %s, interface %u\n",
-		to_of_node(fwnode)->full_name, vep.base.port);
+		to_of_node(vep->base.local_fwnode)->full_name, vep->base.port);
 
-	switch (vep.base.port) {
+	switch (vep->base.port) {
 	case ISP_OF_PHY_PARALLEL:
 		buscfg->interface = ISP_INTERFACE_PARALLEL;
 		buscfg->bus.parallel.data_lane_shift =
-			vep.bus.parallel.data_shift;
+			vep->bus.parallel.data_shift;
 		buscfg->bus.parallel.clk_pol =
-			!!(vep.bus.parallel.flags
+			!!(vep->bus.parallel.flags
 			   & V4L2_MBUS_PCLK_SAMPLE_FALLING);
 		buscfg->bus.parallel.hs_pol =
-			!!(vep.bus.parallel.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW);
+			!!(vep->bus.parallel.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW);
 		buscfg->bus.parallel.vs_pol =
-			!!(vep.bus.parallel.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW);
+			!!(vep->bus.parallel.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW);
 		buscfg->bus.parallel.fld_pol =
-			!!(vep.bus.parallel.flags & V4L2_MBUS_FIELD_EVEN_LOW);
+			!!(vep->bus.parallel.flags & V4L2_MBUS_FIELD_EVEN_LOW);
 		buscfg->bus.parallel.data_pol =
-			!!(vep.bus.parallel.flags & V4L2_MBUS_DATA_ACTIVE_LOW);
-		buscfg->bus.parallel.bt656 = vep.bus_type == V4L2_MBUS_BT656;
+			!!(vep->bus.parallel.flags & V4L2_MBUS_DATA_ACTIVE_LOW);
+		buscfg->bus.parallel.bt656 = vep->bus_type == V4L2_MBUS_BT656;
 		break;
 
 	case ISP_OF_PHY_CSIPHY1:
 	case ISP_OF_PHY_CSIPHY2:
-		switch (vep.bus_type) {
+		switch (vep->bus_type) {
 		case V4L2_MBUS_CCP2:
 		case V4L2_MBUS_CSI1:
 			dev_dbg(dev, "CSI-1/CCP-2 configuration\n");
@@ -2060,11 +2057,11 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
 			break;
 		default:
 			dev_err(dev, "unsupported bus type %u\n",
-				vep.bus_type);
+				vep->bus_type);
 			return -EINVAL;
 		}
 
-		switch (vep.base.port) {
+		switch (vep->base.port) {
 		case ISP_OF_PHY_CSIPHY1:
 			if (csi1)
 				buscfg->interface = ISP_INTERFACE_CCP2B_PHY1;
@@ -2080,47 +2077,47 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
 		}
 		if (csi1) {
 			buscfg->bus.ccp2.lanecfg.clk.pos =
-				vep.bus.mipi_csi1.clock_lane;
+				vep->bus.mipi_csi1.clock_lane;
 			buscfg->bus.ccp2.lanecfg.clk.pol =
-				vep.bus.mipi_csi1.lane_polarity[0];
+				vep->bus.mipi_csi1.lane_polarity[0];
 			dev_dbg(dev, "clock lane polarity %u, pos %u\n",
 				buscfg->bus.ccp2.lanecfg.clk.pol,
 				buscfg->bus.ccp2.lanecfg.clk.pos);
 
 			buscfg->bus.ccp2.lanecfg.data[0].pos =
-				vep.bus.mipi_csi1.data_lane;
+				vep->bus.mipi_csi1.data_lane;
 			buscfg->bus.ccp2.lanecfg.data[0].pol =
-				vep.bus.mipi_csi1.lane_polarity[1];
+				vep->bus.mipi_csi1.lane_polarity[1];
 
-			dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
+			dev_dbg(dev, "data lane polarity %u, pos %u\n",
 				buscfg->bus.ccp2.lanecfg.data[0].pol,
 				buscfg->bus.ccp2.lanecfg.data[0].pos);
 
 			buscfg->bus.ccp2.strobe_clk_pol =
-				vep.bus.mipi_csi1.clock_inv;
-			buscfg->bus.ccp2.phy_layer = vep.bus.mipi_csi1.strobe;
+				vep->bus.mipi_csi1.clock_inv;
+			buscfg->bus.ccp2.phy_layer = vep->bus.mipi_csi1.strobe;
 			buscfg->bus.ccp2.ccp2_mode =
-				vep.bus_type == V4L2_MBUS_CCP2;
+				vep->bus_type == V4L2_MBUS_CCP2;
 			buscfg->bus.ccp2.vp_clk_pol = 1;
 
 			buscfg->bus.ccp2.crc = 1;
 		} else {
 			buscfg->bus.csi2.lanecfg.clk.pos =
-				vep.bus.mipi_csi2.clock_lane;
+				vep->bus.mipi_csi2.clock_lane;
 			buscfg->bus.csi2.lanecfg.clk.pol =
-				vep.bus.mipi_csi2.lane_polarities[0];
+				vep->bus.mipi_csi2.lane_polarities[0];
 			dev_dbg(dev, "clock lane polarity %u, pos %u\n",
 				buscfg->bus.csi2.lanecfg.clk.pol,
 				buscfg->bus.csi2.lanecfg.clk.pos);
 
 			buscfg->bus.csi2.num_data_lanes =
-				vep.bus.mipi_csi2.num_data_lanes;
+				vep->bus.mipi_csi2.num_data_lanes;
 
 			for (i = 0; i < buscfg->bus.csi2.num_data_lanes; i++) {
 				buscfg->bus.csi2.lanecfg.data[i].pos =
-					vep.bus.mipi_csi2.data_lanes[i];
+					vep->bus.mipi_csi2.data_lanes[i];
 				buscfg->bus.csi2.lanecfg.data[i].pol =
-					vep.bus.mipi_csi2.lane_polarities[i + 1];
+					vep->bus.mipi_csi2.lane_polarities[i + 1];
 				dev_dbg(dev,
 					"data lane %u polarity %u, pos %u\n", i,
 					buscfg->bus.csi2.lanecfg.data[i].pol,
@@ -2137,57 +2134,14 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
 
 	default:
 		dev_warn(dev, "%s: invalid interface %u\n",
-			 to_of_node(fwnode)->full_name, vep.base.port);
+			 to_of_node(vep->base.local_fwnode)->full_name,
+			 vep->base.port);
 		return -EINVAL;
 	}
 
 	return 0;
 }
 
-static int isp_fwnodes_parse(struct device *dev,
-			     struct v4l2_async_notifier *notifier)
-{
-	struct fwnode_handle *fwnode = NULL;
-
-	notifier->subdevs = devm_kcalloc(
-		dev, ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL);
-	if (!notifier->subdevs)
-		return -ENOMEM;
-
-	while (notifier->num_subdevs < ISP_MAX_SUBDEVS &&
-	       (fwnode = fwnode_graph_get_next_endpoint(
-			of_fwnode_handle(dev->of_node), fwnode))) {
-		struct isp_async_subdev *isd;
-
-		isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
-		if (!isd)
-			goto error;
-
-		if (isp_fwnode_parse(dev, fwnode, isd)) {
-			devm_kfree(dev, isd);
-			continue;
-		}
-
-		notifier->subdevs[notifier->num_subdevs] = &isd->asd;
-
-		isd->asd.match.fwnode.fwnode =
-			fwnode_graph_get_remote_port_parent(fwnode);
-		if (!isd->asd.match.fwnode.fwnode) {
-			dev_warn(dev, "bad remote port parent\n");
-			goto error;
-		}
-
-		isd->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
-		notifier->num_subdevs++;
-	}
-
-	return notifier->num_subdevs;
-
-error:
-	fwnode_handle_put(fwnode);
-	return -EINVAL;
-}
-
 static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 {
 	struct isp_device *isp = container_of(async, struct isp_device,
@@ -2256,7 +2210,9 @@ static int isp_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = isp_fwnodes_parse(&pdev->dev, &isp->notifier);
+	ret = v4l2_fwnode_endpoints_parse(
+		&pdev->dev, &isp->notifier, sizeof(struct isp_async_subdev),
+		isp_fwnode_parse);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 5cd2687310fe..cb0fc4b4e3bf 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -26,6 +26,7 @@
 #include <linux/string.h>
 #include <linux/types.h>
 
+#include <media/v4l2-async.h>
 #include <media/v4l2-fwnode.h>
 
 enum v4l2_fwnode_bus_type {
@@ -383,6 +384,130 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
 }
 EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
 
+static int notifier_realloc(struct device *dev,
+			    struct v4l2_async_notifier *notifier,
+			    unsigned int max_subdevs)
+{
+	struct v4l2_async_subdev **subdevs;
+	unsigned int i;
+
+	if (max_subdevs + notifier->num_subdevs <= notifier->max_subdevs)
+		return 0;
+
+	subdevs = devm_kcalloc(
+		dev, max_subdevs + notifier->num_subdevs,
+		sizeof(*notifier->subdevs), GFP_KERNEL);
+	if (!subdevs)
+		return -ENOMEM;
+
+	if (notifier->subdevs) {
+		for (i = 0; i < notifier->num_subdevs; i++)
+			subdevs[i] = notifier->subdevs[i];
+
+		devm_kfree(dev, notifier->subdevs);
+	}
+
+	notifier->subdevs = subdevs;
+	notifier->max_subdevs = max_subdevs + notifier->num_subdevs;
+
+	return 0;
+}
+
+static int __v4l2_fwnode_endpoint_parse(
+	struct device *dev, struct v4l2_async_notifier *notifier,
+	struct fwnode_handle *endpoint, struct v4l2_async_subdev *asd,
+	int (*parse_single)(struct device *dev,
+			    struct v4l2_fwnode_endpoint *vep,
+			    struct v4l2_async_subdev *asd))
+{
+	struct v4l2_fwnode_endpoint *vep;
+	int ret;
+
+	/* Ignore endpoints the parsing of which failed. */
+	vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
+	if (IS_ERR(vep))
+		return 0;
+
+	notifier->subdevs[notifier->num_subdevs] = asd;
+
+	ret = parse_single(dev, vep, asd);
+	v4l2_fwnode_endpoint_free(vep);
+	if (ret)
+		return ret;
+
+	asd->match.fwnode.fwnode =
+		fwnode_graph_get_remote_port_parent(endpoint);
+	if (!asd->match.fwnode.fwnode) {
+		dev_warn(dev, "bad remote port parent\n");
+		return -EINVAL;
+	}
+
+	asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
+	notifier->num_subdevs++;
+
+	return 0;
+}
+
+/**
+ * v4l2_fwnode_endpoint_parse - Parse V4L2 fwnode endpoints in a device node
+ * @dev: local struct device
+ * @notifier: async notifier related to @dev
+ * @asd_struct_size: size of the driver's async sub-device struct, including
+ *		     sizeof(struct v4l2_async_subdev)
+ * @parse_single: driver's callback function called on each V4L2 fwnode endpoint
+ *
+ * Parse all V4L2 fwnode endpoints related to the device.
+ *
+ * Note that this function is intended for drivers to replace the existing
+ * implementation that loops over all ports and endpoints. It is NOT INTENDED TO
+ * BE USED BY NEW DRIVERS.
+ */
+int v4l2_fwnode_endpoints_parse(
+	struct device *dev, struct v4l2_async_notifier *notifier,
+	size_t asd_struct_size,
+	int (*parse_single)(struct device *dev,
+			    struct v4l2_fwnode_endpoint *vep,
+			    struct v4l2_async_subdev *asd))
+{
+	struct fwnode_handle *fwnode = NULL;
+	unsigned int max_subdevs = notifier->max_subdevs;
+	int ret;
+
+	if (asd_struct_size < sizeof(struct v4l2_async_subdev))
+		return -EINVAL;
+
+	while ((fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev),
+							fwnode)))
+		max_subdevs++;
+
+	ret = notifier_realloc(dev, notifier, max_subdevs);
+	if (ret)
+		return ret;
+
+	for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
+				     dev_fwnode(dev), fwnode)) &&
+		     !WARN_ON(notifier->num_subdevs >= notifier->max_subdevs);
+		) {
+		struct v4l2_async_subdev *asd;
+
+		asd = devm_kzalloc(dev, asd_struct_size, GFP_KERNEL);
+		if (!asd) {
+			ret = -ENOMEM;
+			goto error;
+		}
+
+		ret = __v4l2_fwnode_endpoint_parse(dev, notifier, fwnode, asd,
+						   parse_single);
+		if (ret < 0)
+			goto error;
+	}
+
+error:
+	fwnode_handle_put(fwnode);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoints_parse);
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
 MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index c69d8c8a66d0..067f3687774b 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -78,7 +78,8 @@ struct v4l2_async_subdev {
 /**
  * struct v4l2_async_notifier - v4l2_device notifier data
  *
- * @num_subdevs: number of subdevices
+ * @num_subdevs: number of subdevices used in subdevs array
+ * @max_subdevs: number of subdevices allocated in subdevs array
  * @subdevs:	array of pointers to subdevice descriptors
  * @v4l2_dev:	pointer to struct v4l2_device
  * @waiting:	list of struct v4l2_async_subdev, waiting for their drivers
@@ -90,6 +91,7 @@ struct v4l2_async_subdev {
  */
 struct v4l2_async_notifier {
 	unsigned int num_subdevs;
+	unsigned int max_subdevs;
 	struct v4l2_async_subdev **subdevs;
 	struct v4l2_device *v4l2_dev;
 	struct list_head waiting;
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index cb34dcb0bb65..c75a768d4ef7 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -25,6 +25,8 @@
 #include <media/v4l2-mediabus.h>
 
 struct fwnode_handle;
+struct v4l2_async_notifier;
+struct v4l2_async_subdev;
 
 #define MAX_DATA_LANES	4
 
@@ -122,4 +124,11 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
 			   struct v4l2_fwnode_link *link);
 void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
 
+int v4l2_fwnode_endpoints_parse(
+	struct device *dev, struct v4l2_async_notifier *notifier,
+	size_t asd_struct_size,
+	int (*parse_single)(struct device *dev,
+			    struct v4l2_fwnode_endpoint *vep,
+			    struct v4l2_async_subdev *asd));
+
 #endif /* _V4L2_FWNODE_H */
-- 
2.11.0

[PATCH v3 3/3] v4l: fwnode: Support generic parsing of graph endpoints in a single port

[Sakari Ailus]

This is the preferred way to parse the endpoints.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-fwnode.c | 51 +++++++++++++++++++++++++++++++++++
 include/media/v4l2-fwnode.h           |  7 +++++
 2 files changed, 58 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index cb0fc4b4e3bf..961bcdf22d9a 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -508,6 +508,57 @@ int v4l2_fwnode_endpoints_parse(
 }
 EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoints_parse);
 
+/**
+ * v4l2_fwnode_endpoint_parse - Parse V4L2 fwnode endpoints in a port node
+ * @dev: local struct device
+ * @notifier: async notifier related to @dev
+ * @port: port number
+ * @endpoint: endpoint number
+ * @asd_struct_size: size of the driver's async sub-device struct, including
+ *		     sizeof(struct v4l2_async_subdev)
+ * @parse_single: driver's callback function called on each V4L2 fwnode endpoint
+ *
+ * Parse all V4L2 fwnode endpoints related to a given port. This is
+ * the preferred interface over v4l2_fwnode_endpoints_parse() and
+ * should be used by new drivers.
+ */
+int v4l2_fwnode_endpoint_parse_port(
+	struct device *dev, struct v4l2_async_notifier *notifier,
+	unsigned int port, unsigned int endpoint, size_t asd_struct_size,
+	int (*parse_single)(struct device *dev,
+			    struct v4l2_fwnode_endpoint *vep,
+			    struct v4l2_async_subdev *asd))
+{
+	struct fwnode_handle *fwnode;
+	struct v4l2_async_subdev *asd;
+	int ret;
+
+	fwnode = fwnode_graph_get_remote_node(dev_fwnode(dev), port, endpoint);
+	if (!fwnode)
+		return -ENOENT;
+
+	asd = devm_kzalloc(dev, asd_struct_size, GFP_KERNEL);
+	if (!asd)
+		return -ENOMEM;
+
+	ret = notifier_realloc(dev, notifier, notifier->num_subdevs + 1);
+	if (ret)
+		goto out_free;
+
+	ret = __v4l2_fwnode_endpoint_parse(dev, notifier, fwnode, asd,
+					   parse_single);
+	if (ret)
+		goto out_free;
+
+	return 0;
+
+out_free:
+	devm_kfree(dev, asd);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoint_parse_port);
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
 MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index c75a768d4ef7..5adf28e7b070 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -131,4 +131,11 @@ int v4l2_fwnode_endpoints_parse(
 			    struct v4l2_fwnode_endpoint *vep,
 			    struct v4l2_async_subdev *asd));
 
+int v4l2_fwnode_endpoint_parse_port(
+	struct device *dev, struct v4l2_async_notifier *notifier,
+	unsigned int port, unsigned int endpoint, size_t asd_struct_size,
+	int (*parse_single)(struct device *dev,
+			    struct v4l2_fwnode_endpoint *vep,
+			    struct v4l2_async_subdev *asd));
+
 #endif /* _V4L2_FWNODE_H */
-- 
2.11.0

Re: [PATCH v3 0/3] Unified fwnode endpoint parser

[Sakari Ailus]

On Fri, Aug 18, 2017 at 02:23:14PM +0300, Sakari Ailus wrote:
> Hi folks,
> 
> We have a large influx of new, unmerged, drivers that are now parsing
> fwnode endpoints and each one of them is doing this a little bit
> differently. The needs are still exactly the same for the graph data
> structure is device independent. This is still a non-trivial task and the
> majority of the driver implementations are buggy, just buggy in different
> ways.
> 
> Facilitate parsing endpoints by adding a convenience function for parsing
> the endpoints, and make the omap3isp driver use it as an example.
> 
> I plan to include the first patch to a pull request soonish, the second
> could go in with the first user.

And now that a new patch has been added in front of the set, this means
that 1 and 2 could IMO go in soonish whereas the third would go in later.

> 
> since v2:
> 
> - Rebase on CCP2 support patches.
> 
> - Prepend a patch cleaning up omap3isp driver a little.
> 
> since v1:
> 
> - The first patch has been merged (it was a bugfix).
> 
> - In anticipation that the parsing can take place over several iterations,
>   take the existing number of async sub-devices into account when
>   re-allocating an array of async sub-devices.
> 
> - Rework the first patch to better anticipate parsing single endpoint at a
>   time by a driver.
> 
> - Add a second patch that adds a function for parsing endpoints one at a
>   time based on port and endpoint numbers.
> 
> Sakari Ailus (3):
>   omap3isp: Drop redundant isp->subdevs field and ISP_MAX_SUBDEVS
>   v4l: fwnode: Support generic parsing of graph endpoints in a device
>   v4l: fwnode: Support generic parsing of graph endpoints in a single
>     port
> 
>  drivers/media/platform/omap3isp/isp.c | 116 +++++++---------------
>  drivers/media/platform/omap3isp/isp.h |   3 -
>  drivers/media/v4l2-core/v4l2-fwnode.c | 176 ++++++++++++++++++++++++++++++++++
>  include/media/v4l2-async.h            |   4 +-
>  include/media/v4l2-fwnode.h           |  16 ++++
>  5 files changed, 231 insertions(+), 84 deletions(-)
> 
> -- 
> 2.11.0
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH v3 1/3] omap3isp: Drop redundant isp->subdevs field and ISP_MAX_SUBDEVS

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Friday, 18 August 2017 14:23:15 EEST Sakari Ailus wrote:
> struct omap3isp.subdevs field and ISP_MAX_SUBDEVS macro are both unused.
> Remove them.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

The field and macro are still used, you only remove them in patch 2/3. You can 
squash 1/3 and 2/3 together.

> ---
>  drivers/media/platform/omap3isp/isp.h | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.h
> b/drivers/media/platform/omap3isp/isp.h index e528df6efc09..848cd96b67ca
> 100644
> --- a/drivers/media/platform/omap3isp/isp.h
> +++ b/drivers/media/platform/omap3isp/isp.h
> @@ -220,9 +220,6 @@ struct isp_device {
> 
>  	unsigned int sbl_resources;
>  	unsigned int subclk_resources;
> -
> -#define ISP_MAX_SUBDEVS		8
> -	struct v4l2_subdev *subdevs[ISP_MAX_SUBDEVS];
>  };
> 
>  struct isp_async_subdev {


-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 1/3] omap3isp: Drop redundant isp->subdevs field and ISP_MAX_SUBDEVS

[Sakari Ailus]

On Tue, Aug 22, 2017 at 03:30:22PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday, 18 August 2017 14:23:15 EEST Sakari Ailus wrote:
> > struct omap3isp.subdevs field and ISP_MAX_SUBDEVS macro are both unused.
> > Remove them.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> The field and macro are still used, you only remove them in patch 2/3. You can 
> squash 1/3 and 2/3 together.

Oh, I missed this indeed. I'll squash the patches.

> 
> > ---
> >  drivers/media/platform/omap3isp/isp.h | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/media/platform/omap3isp/isp.h
> > b/drivers/media/platform/omap3isp/isp.h index e528df6efc09..848cd96b67ca
> > 100644
> > --- a/drivers/media/platform/omap3isp/isp.h
> > +++ b/drivers/media/platform/omap3isp/isp.h
> > @@ -220,9 +220,6 @@ struct isp_device {
> > 
> >  	unsigned int sbl_resources;
> >  	unsigned int subclk_resources;
> > -
> > -#define ISP_MAX_SUBDEVS		8
> > -	struct v4l2_subdev *subdevs[ISP_MAX_SUBDEVS];
> >  };
> > 
> >  struct isp_async_subdev {
> 
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH v3 2/3] v4l: fwnode: Support generic parsing of graph endpoints in a device

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Friday, 18 August 2017 14:23:16 EEST Sakari Ailus wrote:
> The current practice is that drivers iterate over their endpoints and
> parse each endpoint separately. This is very similar in a number of
> drivers, implement a generic function for the job. Driver specific matters
> can be taken into account in the driver specific callback.
> 
> Convert the omap3isp as an example.

It would be nice to convert at least two drivers to show that the code can 
indeed be shared between multiple drivers. Even better, you could convert all 
drivers.
 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/platform/omap3isp/isp.c | 116 ++++++++++---------------------
> drivers/media/v4l2-core/v4l2-fwnode.c | 125 ++++++++++++++++++++++++++++++++
> include/media/v4l2-async.h            |   4 +-
> include/media/v4l2-fwnode.h           |   9 +++
> 4 files changed, 173 insertions(+), 81 deletions(-)

[snip]

> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> b/drivers/media/v4l2-core/v4l2-fwnode.c index 5cd2687310fe..cb0fc4b4e3bf
> 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -26,6 +26,7 @@
>  #include <linux/string.h>
>  #include <linux/types.h>
> 
> +#include <media/v4l2-async.h>
>  #include <media/v4l2-fwnode.h>
> 
>  enum v4l2_fwnode_bus_type {
> @@ -383,6 +384,130 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link
> *link) }
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> 
> +static int notifier_realloc(struct device *dev,
> +			    struct v4l2_async_notifier *notifier,
> +			    unsigned int max_subdevs)

It looks like you interpret the variable as an increment. You shouldn't call 
it max_subdevs in that case. I would however keep the name and pass the total 
number of subdevs instead of an increment, to mimic the realloc API.

> +{
> +	struct v4l2_async_subdev **subdevs;
> +	unsigned int i;
> +
> +	if (max_subdevs + notifier->num_subdevs <= notifier->max_subdevs)
> +		return 0;
> +
> +	subdevs = devm_kcalloc(
> +		dev, max_subdevs + notifier->num_subdevs,
> +		sizeof(*notifier->subdevs), GFP_KERNEL);

We know that we'll have to move away from devm_* allocation to fix object 
lifetime management, so we could as well start now.

> +	if (!subdevs)
> +		return -ENOMEM;
> +
> +	if (notifier->subdevs) {
> +		for (i = 0; i < notifier->num_subdevs; i++)
> +			subdevs[i] = notifier->subdevs[i];

Is there a reason to use a loop here instead of a memcpy() covering the whole 
array ?

> +		devm_kfree(dev, notifier->subdevs);
> +	}
> +
> +	notifier->subdevs = subdevs;
> +	notifier->max_subdevs = max_subdevs + notifier->num_subdevs;
> +
> +	return 0;
> +}
> +
> +static int __v4l2_fwnode_endpoint_parse(
> +	struct device *dev, struct v4l2_async_notifier *notifier,
> +	struct fwnode_handle *endpoint, struct v4l2_async_subdev *asd,
> +	int (*parse_single)(struct device *dev,
> +			    struct v4l2_fwnode_endpoint *vep,
> +			    struct v4l2_async_subdev *asd))
> +{
> +	struct v4l2_fwnode_endpoint *vep;
> +	int ret;
> +
> +	/* Ignore endpoints the parsing of which failed. */

Silently ignoring invalid DT sounds bad, I'd rather catch errors and return 
with an error code to make sure that DT gets fixed.

> +	vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
> +	if (IS_ERR(vep))
> +		return 0;
> +
> +	notifier->subdevs[notifier->num_subdevs] = asd;
> +
> +	ret = parse_single(dev, vep, asd);
> +	v4l2_fwnode_endpoint_free(vep);
> +	if (ret)
> +		return ret;
> +
> +	asd->match.fwnode.fwnode =
> +		fwnode_graph_get_remote_port_parent(endpoint);
> +	if (!asd->match.fwnode.fwnode) {
> +		dev_warn(dev, "bad remote port parent\n");
> +		return -EINVAL;
> +	}
> +
> +	asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> +	notifier->num_subdevs++;
> +
> +	return 0;
> +}
> +
> +/**
> + * v4l2_fwnode_endpoint_parse - Parse V4L2 fwnode endpoints in a device
> node

This doesn't match the function name.

> + * @dev: local struct device

Based on the documentation only and without a priori knowledge of the API, 
local struct device is very vague.

> + * @notifier: async notifier related to @dev

Ditto. You need more documentation, especially given that this is the first 
function in the core that fills a notifier from DT. You might also want to 
reflect that fact in the function name.

> + * @asd_struct_size: size of the driver's async sub-device struct,
> including
> + *		     sizeof(struct v4l2_async_subdev)
> + * @parse_single: driver's callback function called on each V4L2 fwnode
> endpoint

The parse_single return values should be documented.

> + * Parse all V4L2 fwnode endpoints related to the device.
> + *
> + * Note that this function is intended for drivers to replace the existing
> + * implementation that loops over all ports and endpoints. It is NOT
> INTENDED TO
> + * BE USED BY NEW DRIVERS.

You should document what the preferred way is. And I'd much rather convert 
drivers to the preferred way instead of adding a helper function that is 
already deprecated.

> + */
> +int v4l2_fwnode_endpoints_parse(

v4l2_fwnode_parse_endpoints() would sound more natural.

> +	struct device *dev, struct v4l2_async_notifier *notifier,
> +	size_t asd_struct_size,
> +	int (*parse_single)(struct device *dev,
> +			    struct v4l2_fwnode_endpoint *vep,
> +			    struct v4l2_async_subdev *asd))
> +{
> +	struct fwnode_handle *fwnode = NULL;
> +	unsigned int max_subdevs = notifier->max_subdevs;
> +	int ret;
> +
> +	if (asd_struct_size < sizeof(struct v4l2_async_subdev))
> +		return -EINVAL;
> +
> +	while ((fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev),
> +							fwnode)))
> +		max_subdevs++;
> +
> +	ret = notifier_realloc(dev, notifier, max_subdevs);
> +	if (ret)
> +		return ret;
> +
> +	for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> +				     dev_fwnode(dev), fwnode)) &&
> +		     !WARN_ON(notifier->num_subdevs >= notifier->max_subdevs);

It's nice to warn that the kernel will crash, but it would be even nicer to 
prevent the crash by returning an error instead of continuing parsing 
endpoints :-)

> +		) {
> +		struct v4l2_async_subdev *asd;
> +
> +		asd = devm_kzalloc(dev, asd_struct_size, GFP_KERNEL);
> +		if (!asd) {
> +			ret = -ENOMEM;
> +			goto error;
> +		}
> +
> +		ret = __v4l2_fwnode_endpoint_parse(dev, notifier, fwnode, asd,
> +						   parse_single);
> +		if (ret < 0)
> +			goto error;
> +	}
> +
> +error:
> +	fwnode_handle_put(fwnode);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoints_parse);
> +
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
>  MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index c69d8c8a66d0..067f3687774b 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -78,7 +78,8 @@ struct v4l2_async_subdev {
>  /**
>   * struct v4l2_async_notifier - v4l2_device notifier data
>   *
> - * @num_subdevs: number of subdevices
> + * @num_subdevs: number of subdevices used in subdevs array
> + * @max_subdevs: number of subdevices allocated in subdevs array
>   * @subdevs:	array of pointers to subdevice descriptors
>   * @v4l2_dev:	pointer to struct v4l2_device
>   * @waiting:	list of struct v4l2_async_subdev, waiting for their drivers
> @@ -90,6 +91,7 @@ struct v4l2_async_subdev {
>   */
>  struct v4l2_async_notifier {
>  	unsigned int num_subdevs;
> +	unsigned int max_subdevs;
>  	struct v4l2_async_subdev **subdevs;
>  	struct v4l2_device *v4l2_dev;
>  	struct list_head waiting;
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index cb34dcb0bb65..c75a768d4ef7 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -25,6 +25,8 @@
>  #include <media/v4l2-mediabus.h>
> 
>  struct fwnode_handle;
> +struct v4l2_async_notifier;
> +struct v4l2_async_subdev;
> 
>  #define MAX_DATA_LANES	4
> 
> @@ -122,4 +124,11 @@ int v4l2_fwnode_parse_link(struct fwnode_handle
> *fwnode, struct v4l2_fwnode_link *link);
>  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> 
> +int v4l2_fwnode_endpoints_parse(
> +	struct device *dev, struct v4l2_async_notifier *notifier,
> +	size_t asd_struct_size,
> +	int (*parse_single)(struct device *dev,
> +			    struct v4l2_fwnode_endpoint *vep,
> +			    struct v4l2_async_subdev *asd));
> +
>  #endif /* _V4L2_FWNODE_H */


-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 3/3] v4l: fwnode: Support generic parsing of graph endpoints in a single port

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Friday, 18 August 2017 14:23:17 EEST Sakari Ailus wrote:
> This is the preferred way to parse the endpoints.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 51 ++++++++++++++++++++++++++++++++
>  include/media/v4l2-fwnode.h           |  7 +++++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> b/drivers/media/v4l2-core/v4l2-fwnode.c index cb0fc4b4e3bf..961bcdf22d9a
> 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -508,6 +508,57 @@ int v4l2_fwnode_endpoints_parse(
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoints_parse);
> 
> +/**
> + * v4l2_fwnode_endpoint_parse - Parse V4L2 fwnode endpoints in a port node

This doesn't match the function name.

> + * @dev: local struct device
> + * @notifier: async notifier related to @dev
> + * @port: port number
> + * @endpoint: endpoint number
> + * @asd_struct_size: size of the driver's async sub-device struct,
> including
> + *		     sizeof(struct v4l2_async_subdev)
> + * @parse_single: driver's callback function called on each V4L2 fwnode
> endpoint
> + *
> + * Parse all V4L2 fwnode endpoints related to a given port.

It doesn't, it parses a single endpoint only.

As for patch 2/3, more detailed documentation is needed.

> This is
> + * the preferred interface over v4l2_fwnode_endpoints_parse() and
> + * should be used by new drivers.

I think converting one driver as an example would make it clearer how this 
function is supposed to be used.

> + */
> +int v4l2_fwnode_endpoint_parse_port(
> +	struct device *dev, struct v4l2_async_notifier *notifier,
> +	unsigned int port, unsigned int endpoint, size_t asd_struct_size,
> +	int (*parse_single)(struct device *dev,
> +			    struct v4l2_fwnode_endpoint *vep,
> +			    struct v4l2_async_subdev *asd))
> +{
> +	struct fwnode_handle *fwnode;
> +	struct v4l2_async_subdev *asd;
> +	int ret;
> +
> +	fwnode = fwnode_graph_get_remote_node(dev_fwnode(dev), port, endpoint);
> +	if (!fwnode)
> +		return -ENOENT;
> +
> +	asd = devm_kzalloc(dev, asd_struct_size, GFP_KERNEL);
> +	if (!asd)
> +		return -ENOMEM;
> +
> +	ret = notifier_realloc(dev, notifier, notifier->num_subdevs + 1);
> +	if (ret)
> +		goto out_free;
> +
> +	ret = __v4l2_fwnode_endpoint_parse(dev, notifier, fwnode, asd,
> +					   parse_single);
> +	if (ret)
> +		goto out_free;
> +
> +	return 0;
> +
> +out_free:
> +	devm_kfree(dev, asd);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoint_parse_port);
> +
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
>  MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index c75a768d4ef7..5adf28e7b070 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -131,4 +131,11 @@ int v4l2_fwnode_endpoints_parse(
>  			    struct v4l2_fwnode_endpoint *vep,
>  			    struct v4l2_async_subdev *asd));
> 
> +int v4l2_fwnode_endpoint_parse_port(
> +	struct device *dev, struct v4l2_async_notifier *notifier,
> +	unsigned int port, unsigned int endpoint, size_t asd_struct_size,
> +	int (*parse_single)(struct device *dev,
> +			    struct v4l2_fwnode_endpoint *vep,
> +			    struct v4l2_async_subdev *asd));
> +
>  #endif /* _V4L2_FWNODE_H */


-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 2/3] v4l: fwnode: Support generic parsing of graph endpoints in a device

[Sakari Ailus]

Hi Laurent,

Thanks for the critique.

On Tue, Aug 22, 2017 at 03:52:33PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Friday, 18 August 2017 14:23:16 EEST Sakari Ailus wrote:
> > The current practice is that drivers iterate over their endpoints and
> > parse each endpoint separately. This is very similar in a number of
> > drivers, implement a generic function for the job. Driver specific matters
> > can be taken into account in the driver specific callback.
> > 
> > Convert the omap3isp as an example.
> 
> It would be nice to convert at least two drivers to show that the code can 
> indeed be shared between multiple drivers. Even better, you could convert all 
> drivers.

That's the intent in the long run. There's still no definite need to do
this in a single, big, hot patchset.

>  
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/platform/omap3isp/isp.c | 116 ++++++++++---------------------
> > drivers/media/v4l2-core/v4l2-fwnode.c | 125 ++++++++++++++++++++++++++++++++
> > include/media/v4l2-async.h            |   4 +-
> > include/media/v4l2-fwnode.h           |   9 +++
> > 4 files changed, 173 insertions(+), 81 deletions(-)
> 
> [snip]
> 
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> > b/drivers/media/v4l2-core/v4l2-fwnode.c index 5cd2687310fe..cb0fc4b4e3bf
> > 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/string.h>
> >  #include <linux/types.h>
> > 
> > +#include <media/v4l2-async.h>
> >  #include <media/v4l2-fwnode.h>
> > 
> >  enum v4l2_fwnode_bus_type {
> > @@ -383,6 +384,130 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link
> > *link) }
> >  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> > 
> > +static int notifier_realloc(struct device *dev,
> > +			    struct v4l2_async_notifier *notifier,
> > +			    unsigned int max_subdevs)
> 
> It looks like you interpret the variable as an increment. You shouldn't call 
> it max_subdevs in that case. I would however keep the name and pass the total 
> number of subdevs instead of an increment, to mimic the realloc API.

Works for me.

> 
> > +{
> > +	struct v4l2_async_subdev **subdevs;
> > +	unsigned int i;
> > +
> > +	if (max_subdevs + notifier->num_subdevs <= notifier->max_subdevs)
> > +		return 0;
> > +
> > +	subdevs = devm_kcalloc(
> > +		dev, max_subdevs + notifier->num_subdevs,
> > +		sizeof(*notifier->subdevs), GFP_KERNEL);
> 
> We know that we'll have to move away from devm_* allocation to fix object 
> lifetime management, so we could as well start now.

The memory is in practice allocated using devm_() interface in existing
drivers. The fact that it's in a single location makes it much easier
getting rid of it.

I'd rather get rid of memory allocation here in the first place, to be
replaced by a linked list. But first the user of notifier->subdevs in
drivers need to go. The framework interface doesn't need to change as a
result.

> 
> > +	if (!subdevs)
> > +		return -ENOMEM;
> > +
> > +	if (notifier->subdevs) {
> > +		for (i = 0; i < notifier->num_subdevs; i++)
> > +			subdevs[i] = notifier->subdevs[i];
> 
> Is there a reason to use a loop here instead of a memcpy() covering the whole 
> array ?

You could do that, yes, although I'd think this looks nicer. Performance is
hardly a concern here.

> 
> > +		devm_kfree(dev, notifier->subdevs);
> > +	}
> > +
> > +	notifier->subdevs = subdevs;
> > +	notifier->max_subdevs = max_subdevs + notifier->num_subdevs;
> > +
> > +	return 0;
> > +}
> > +
> > +static int __v4l2_fwnode_endpoint_parse(
> > +	struct device *dev, struct v4l2_async_notifier *notifier,
> > +	struct fwnode_handle *endpoint, struct v4l2_async_subdev *asd,
> > +	int (*parse_single)(struct device *dev,
> > +			    struct v4l2_fwnode_endpoint *vep,
> > +			    struct v4l2_async_subdev *asd))
> > +{
> > +	struct v4l2_fwnode_endpoint *vep;
> > +	int ret;
> > +
> > +	/* Ignore endpoints the parsing of which failed. */
> 
> Silently ignoring invalid DT sounds bad, I'd rather catch errors and return 
> with an error code to make sure that DT gets fixed.

That would mean that if a single node is bad, none of the correct ones can
be used either. I'm not sure everyone would be happy about it.

> 
> > +	vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
> > +	if (IS_ERR(vep))
> > +		return 0;
> > +
> > +	notifier->subdevs[notifier->num_subdevs] = asd;
> > +
> > +	ret = parse_single(dev, vep, asd);
> > +	v4l2_fwnode_endpoint_free(vep);
> > +	if (ret)
> > +		return ret;
> > +
> > +	asd->match.fwnode.fwnode =
> > +		fwnode_graph_get_remote_port_parent(endpoint);
> > +	if (!asd->match.fwnode.fwnode) {
> > +		dev_warn(dev, "bad remote port parent\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> > +	notifier->num_subdevs++;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * v4l2_fwnode_endpoint_parse - Parse V4L2 fwnode endpoints in a device
> > node
> 
> This doesn't match the function name.

Will fix.

> 
> > + * @dev: local struct device
> 
> Based on the documentation only and without a priori knowledge of the API, 
> local struct device is very vague.

What would you use then?

Write a fairytale about it? :-)

> 
> > + * @notifier: async notifier related to @dev
> 
> Ditto. You need more documentation, especially given that this is the first 
> function in the core that fills a notifier from DT. You might also want to 
> reflect that fact in the function name.

I can add more documentation.

> 
> > + * @asd_struct_size: size of the driver's async sub-device struct,
> > including
> > + *		     sizeof(struct v4l2_async_subdev)
> > + * @parse_single: driver's callback function called on each V4L2 fwnode
> > endpoint
> 
> The parse_single return values should be documented.

Agreed.

> 
> > + * Parse all V4L2 fwnode endpoints related to the device.
> > + *
> > + * Note that this function is intended for drivers to replace the existing
> > + * implementation that loops over all ports and endpoints. It is NOT
> > INTENDED TO
> > + * BE USED BY NEW DRIVERS.
> 
> You should document what the preferred way is. And I'd much rather convert 
> drivers to the preferred way instead of adding a helper function that is 
> already deprecated.

The preferred way is not a part of this patch but the second one. This is
intended for moving the existing copies of the same code away from drivers.

The preferred way would be to explicitly check ports and endpoints in them
for connections. I'm not sure if the existing DT documentation is enough to
cover this for it does not generally document endpoint numbering.

> 
> > + */
> > +int v4l2_fwnode_endpoints_parse(
> 
> v4l2_fwnode_parse_endpoints() would sound more natural.

We'll need to think more about naming this. v4l2_fwnode_endpoint_parse()
will parse a single endpoint and is entirely unaware of the notifier.

How about v4l2_async_notifier_parse_endpoints()? It's a big lengthy though.

> 
> > +	struct device *dev, struct v4l2_async_notifier *notifier,
> > +	size_t asd_struct_size,
> > +	int (*parse_single)(struct device *dev,
> > +			    struct v4l2_fwnode_endpoint *vep,
> > +			    struct v4l2_async_subdev *asd))
> > +{
> > +	struct fwnode_handle *fwnode = NULL;
> > +	unsigned int max_subdevs = notifier->max_subdevs;
> > +	int ret;
> > +
> > +	if (asd_struct_size < sizeof(struct v4l2_async_subdev))
> > +		return -EINVAL;
> > +
> > +	while ((fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev),
> > +							fwnode)))
> > +		max_subdevs++;
> > +
> > +	ret = notifier_realloc(dev, notifier, max_subdevs);
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> > +				     dev_fwnode(dev), fwnode)) &&
> > +		     !WARN_ON(notifier->num_subdevs >= notifier->max_subdevs);
> 
> It's nice to warn that the kernel will crash, but it would be even nicer to 
> prevent the crash by returning an error instead of continuing parsing 
> endpoints :-)

I'm not quite sure what do you mean. If the number of sub-devices reaches
what's allocated for them in the array, this will stop with a warning.

> 
> > +		) {
> > +		struct v4l2_async_subdev *asd;
> > +
> > +		asd = devm_kzalloc(dev, asd_struct_size, GFP_KERNEL);
> > +		if (!asd) {
> > +			ret = -ENOMEM;
> > +			goto error;
> > +		}
> > +
> > +		ret = __v4l2_fwnode_endpoint_parse(dev, notifier, fwnode, asd,
> > +						   parse_single);
> > +		if (ret < 0)
> > +			goto error;
> > +	}
> > +
> > +error:
> > +	fwnode_handle_put(fwnode);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoints_parse);
> > +
> >  MODULE_LICENSE("GPL");
> >  MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
> >  MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
> > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > index c69d8c8a66d0..067f3687774b 100644
> > --- a/include/media/v4l2-async.h
> > +++ b/include/media/v4l2-async.h
> > @@ -78,7 +78,8 @@ struct v4l2_async_subdev {
> >  /**
> >   * struct v4l2_async_notifier - v4l2_device notifier data
> >   *
> > - * @num_subdevs: number of subdevices
> > + * @num_subdevs: number of subdevices used in subdevs array
> > + * @max_subdevs: number of subdevices allocated in subdevs array
> >   * @subdevs:	array of pointers to subdevice descriptors
> >   * @v4l2_dev:	pointer to struct v4l2_device
> >   * @waiting:	list of struct v4l2_async_subdev, waiting for their drivers
> > @@ -90,6 +91,7 @@ struct v4l2_async_subdev {
> >   */
> >  struct v4l2_async_notifier {
> >  	unsigned int num_subdevs;
> > +	unsigned int max_subdevs;
> >  	struct v4l2_async_subdev **subdevs;
> >  	struct v4l2_device *v4l2_dev;
> >  	struct list_head waiting;
> > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > index cb34dcb0bb65..c75a768d4ef7 100644
> > --- a/include/media/v4l2-fwnode.h
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -25,6 +25,8 @@
> >  #include <media/v4l2-mediabus.h>
> > 
> >  struct fwnode_handle;
> > +struct v4l2_async_notifier;
> > +struct v4l2_async_subdev;
> > 
> >  #define MAX_DATA_LANES	4
> > 
> > @@ -122,4 +124,11 @@ int v4l2_fwnode_parse_link(struct fwnode_handle
> > *fwnode, struct v4l2_fwnode_link *link);
> >  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> > 
> > +int v4l2_fwnode_endpoints_parse(
> > +	struct device *dev, struct v4l2_async_notifier *notifier,
> > +	size_t asd_struct_size,
> > +	int (*parse_single)(struct device *dev,
> > +			    struct v4l2_fwnode_endpoint *vep,
> > +			    struct v4l2_async_subdev *asd));
> > +
> >  #endif /* _V4L2_FWNODE_H */
> 
> 

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH v3 2/3] v4l: fwnode: Support generic parsing of graph endpoints in a device

[Laurent Pinchart]

Hi Sakari,

On Wednesday, 23 August 2017 12:01:24 EEST Sakari Ailus wrote:
> On Tue, Aug 22, 2017 at 03:52:33PM +0300, Laurent Pinchart wrote:
> > On Friday, 18 August 2017 14:23:16 EEST Sakari Ailus wrote:
> >> The current practice is that drivers iterate over their endpoints and
> >> parse each endpoint separately. This is very similar in a number of
> >> drivers, implement a generic function for the job. Driver specific
> >> matters can be taken into account in the driver specific callback.
> >> 
> >> Convert the omap3isp as an example.
> > 
> > It would be nice to convert at least two drivers to show that the code can
> > indeed be shared between multiple drivers. Even better, you could convert
> > all drivers.
> 
> That's the intent in the long run. There's still no definite need to do
> this in a single, big, hot patchset.

You don't need to convert them all in a single patch, but I'd still prefer 
converting them all in a single patch series (and I'd split this patch in two 
to make backporting easier). Otherwise we'll likely end up with multiple 
competing implementations.

> >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> ---
> >> drivers/media/platform/omap3isp/isp.c | 116 ++++++++++------------------
> >> drivers/media/v4l2-core/v4l2-fwnode.c | 125 +++++++++++++++++++++++++
> >> include/media/v4l2-async.h            |   4 +-
> >> include/media/v4l2-fwnode.h           |   9 +++
> >> 4 files changed, 173 insertions(+), 81 deletions(-)
> > 
> > [snip]
> > 
> >> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> >> b/drivers/media/v4l2-core/v4l2-fwnode.c index 5cd2687310fe..cb0fc4b4e3bf
> >> 100644
> >> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> >> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> >> @@ -26,6 +26,7 @@
> >>  #include <linux/string.h>
> >>  #include <linux/types.h>
> >> 
> >> +#include <media/v4l2-async.h>
> >>  #include <media/v4l2-fwnode.h>
> >>  
> >>  enum v4l2_fwnode_bus_type {
> >> @@ -383,6 +384,130 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link
> >> *link) }
> >> 
> >>  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> >> 
> >> +static int notifier_realloc(struct device *dev,
> >> +			    struct v4l2_async_notifier *notifier,
> >> +			    unsigned int max_subdevs)
> > 
> > It looks like you interpret the variable as an increment. You shouldn't
> > call it max_subdevs in that case. I would however keep the name and pass
> > the total number of subdevs instead of an increment, to mimic the realloc
> > API.
> 
> Works for me.
> 
> >> +{
> >> +	struct v4l2_async_subdev **subdevs;
> >> +	unsigned int i;
> >> +
> >> +	if (max_subdevs + notifier->num_subdevs <= notifier->max_subdevs)
> >> +		return 0;
> >> +
> >> +	subdevs = devm_kcalloc(
> >> +		dev, max_subdevs + notifier->num_subdevs,
> >> +		sizeof(*notifier->subdevs), GFP_KERNEL);
> > 
> > We know that we'll have to move away from devm_* allocation to fix object
> > lifetime management, so we could as well start now.
> 
> The memory is in practice allocated using devm_() interface in existing
> drivers.

Yes, and that's bad :-)

> The fact that it's in a single location makes it much easier getting rid of
> it.

Great, so let's get rid of it :-)

> I'd rather get rid of memory allocation here in the first place, to be
> replaced by a linked list. But first the user of notifier->subdevs in
> drivers need to go. The framework interface doesn't need to change as a
> result.

How are you going to allocate and free the linked list entries ?

> >> +	if (!subdevs)
> >> +		return -ENOMEM;
> >> +
> >> +	if (notifier->subdevs) {
> >> +		for (i = 0; i < notifier->num_subdevs; i++)
> >> +			subdevs[i] = notifier->subdevs[i];
> > 
> > Is there a reason to use a loop here instead of a memcpy() covering the
> > whole array ?
> 
> You could do that, yes, although I'd think this looks nicer. Performance is
> hardly a concern here.

It's certainly not a performance issue, I would find the code easier to read.

> >> +		devm_kfree(dev, notifier->subdevs);
> >> +	}
> >> +
> >> +	notifier->subdevs = subdevs;
> >> +	notifier->max_subdevs = max_subdevs + notifier->num_subdevs;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int __v4l2_fwnode_endpoint_parse(
> >> +	struct device *dev, struct v4l2_async_notifier *notifier,
> >> +	struct fwnode_handle *endpoint, struct v4l2_async_subdev *asd,
> >> +	int (*parse_single)(struct device *dev,
> >> +			    struct v4l2_fwnode_endpoint *vep,
> >> +			    struct v4l2_async_subdev *asd))
> >> +{
> >> +	struct v4l2_fwnode_endpoint *vep;
> >> +	int ret;
> >> +
> >> +	/* Ignore endpoints the parsing of which failed. */
> > 
> > Silently ignoring invalid DT sounds bad, I'd rather catch errors and
> > return with an error code to make sure that DT gets fixed.
> 
> That would mean that if a single node is bad, none of the correct ones can
> be used either. I'm not sure everyone would be happy about it.

We should be tolerant towards hardware failures and make as much of the device 
usable as possible, but be very pedantic when parsing DT to catch errors as 
early as possible.

> >> +	vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
> >> +	if (IS_ERR(vep))
> >> +		return 0;
> >> +
> >> +	notifier->subdevs[notifier->num_subdevs] = asd;
> >> +
> >> +	ret = parse_single(dev, vep, asd);
> >> +	v4l2_fwnode_endpoint_free(vep);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	asd->match.fwnode.fwnode =
> >> +		fwnode_graph_get_remote_port_parent(endpoint);
> >> +	if (!asd->match.fwnode.fwnode) {
> >> +		dev_warn(dev, "bad remote port parent\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> >> +	notifier->num_subdevs++;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +/**
> >> + * v4l2_fwnode_endpoint_parse - Parse V4L2 fwnode endpoints in a device
> >> node
> > 
> > This doesn't match the function name.
> 
> Will fix.
> 
> >> + * @dev: local struct device
> > 
> > Based on the documentation only and without a priori knowledge of the API,
> > local struct device is very vague.
> 
> What would you use then?
> 
> Write a fairytale about it? :-)

Just explain which struct device is expected, and how it relates to the 
purpose of the function and its other parameters. If you find that hard to 
explain, imagine how confused someone trying to use the API without any a 
priori knowledge of the subsystem will feel.

> >> + * @notifier: async notifier related to @dev
> > 
> > Ditto. You need more documentation, especially given that this is the
> > first function in the core that fills a notifier from DT. You might also
> > want to reflect that fact in the function name.
> 
> I can add more documentation.
> 
> >> + * @asd_struct_size: size of the driver's async sub-device struct,
> >> including
> >> + *		     sizeof(struct v4l2_async_subdev)
> >> + * @parse_single: driver's callback function called on each V4L2 fwnode
> >> endpoint
> > 
> > The parse_single return values should be documented.
> 
> Agreed.
> 
> >> + * Parse all V4L2 fwnode endpoints related to the device.
> >> + *
> >> + * Note that this function is intended for drivers to replace the
> >> existing
> >> + * implementation that loops over all ports and endpoints. It is NOT
> >> INTENDED TO
> >> + * BE USED BY NEW DRIVERS.
> > 
> > You should document what the preferred way is. And I'd much rather convert
> > drivers to the preferred way instead of adding a helper function that is
> > already deprecated.
> 
> The preferred way is not a part of this patch but the second one. This is
> intended for moving the existing copies of the same code away from drivers.

You remove the code from a single driver here. How many drivers do we need to 
convert ?

> The preferred way would be to explicitly check ports and endpoints in them
> for connections. I'm not sure if the existing DT documentation is enough to
> cover this for it does not generally document endpoint numbering.
> 
> >> + */
> >> +int v4l2_fwnode_endpoints_parse(
> > 
> > v4l2_fwnode_parse_endpoints() would sound more natural.
> 
> We'll need to think more about naming this. v4l2_fwnode_endpoint_parse()
> will parse a single endpoint and is entirely unaware of the notifier.
> 
> How about v4l2_async_notifier_parse_endpoints()? It's a big lengthy though.

And it's missing fwnode in the name :-)

> >> +	struct device *dev, struct v4l2_async_notifier *notifier,
> >> +	size_t asd_struct_size,
> >> +	int (*parse_single)(struct device *dev,
> >> +			    struct v4l2_fwnode_endpoint *vep,
> >> +			    struct v4l2_async_subdev *asd))
> >> +{
> >> +	struct fwnode_handle *fwnode = NULL;
> >> +	unsigned int max_subdevs = notifier->max_subdevs;
> >> +	int ret;
> >> +
> >> +	if (asd_struct_size < sizeof(struct v4l2_async_subdev))
> >> +		return -EINVAL;
> >> +
> >> +	while ((fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev),
> >> +							fwnode)))
> >> +		max_subdevs++;
> >> +
> >> +	ret = notifier_realloc(dev, notifier, max_subdevs);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> >> +				     dev_fwnode(dev), fwnode)) &&
> >> +		     !WARN_ON(notifier->num_subdevs >= notifier->max_subdevs);
> > 
> > It's nice to warn that the kernel will crash, but it would be even nicer
> > to prevent the crash by returning an error instead of continuing parsing
> > endpoints :-)
> 
> I'm not quite sure what do you mean. If the number of sub-devices reaches
> what's allocated for them in the array, this will stop with a warning.

Oops, my bad, I've misread the code. Your for loop is not very readable :-/ 
How about moving the num_subdevs test inside the loop with

		if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs))
			break;

That should make it more readable.

> >> +		) {
> >> +		struct v4l2_async_subdev *asd;
> >> +
> >> +		asd = devm_kzalloc(dev, asd_struct_size, GFP_KERNEL);
> >> +		if (!asd) {
> >> +			ret = -ENOMEM;
> >> +			goto error;
> >> +		}
> >> +
> >> +		ret = __v4l2_fwnode_endpoint_parse(dev, notifier, fwnode, asd,
> >> +						   parse_single);
> >> +		if (ret < 0)
> >> +			goto error;
> >> +	}
> >> +
> >> +error:
> >> +	fwnode_handle_put(fwnode);
> >> +	return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoints_parse);

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 2/3] v4l: fwnode: Support generic parsing of graph endpoints in a device

[Sakari Ailus]

Hi Laurent,

On Wed, Aug 23, 2017 at 03:59:35PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wednesday, 23 August 2017 12:01:24 EEST Sakari Ailus wrote:
> > On Tue, Aug 22, 2017 at 03:52:33PM +0300, Laurent Pinchart wrote:
> > > On Friday, 18 August 2017 14:23:16 EEST Sakari Ailus wrote:
> > >> The current practice is that drivers iterate over their endpoints and
> > >> parse each endpoint separately. This is very similar in a number of
> > >> drivers, implement a generic function for the job. Driver specific
> > >> matters can be taken into account in the driver specific callback.
> > >> 
> > >> Convert the omap3isp as an example.
> > > 
> > > It would be nice to convert at least two drivers to show that the code can
> > > indeed be shared between multiple drivers. Even better, you could convert
> > > all drivers.
> > 
> > That's the intent in the long run. There's still no definite need to do
> > this in a single, big, hot patchset.
> 
> You don't need to convert them all in a single patch, but I'd still prefer 
> converting them all in a single patch series (and I'd split this patch in two 
> to make backporting easier). Otherwise we'll likely end up with multiple 
> competing implementations.

I don't think that presumption is founded. The fact that there is a driver
doing something on its own does not prevent add a helper function to the
framework which could be used to remove driver code. We have, as an
example, the pipeline power management code in v4l2-mc.c. It was introduced
with the omap3isp driver and is now used by a number of drivers.

This is also related preparation for supporting associations that will be
needed for lens and flash devices.

> 
> > >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > >> ---
> > >> drivers/media/platform/omap3isp/isp.c | 116 ++++++++++------------------
> > >> drivers/media/v4l2-core/v4l2-fwnode.c | 125 +++++++++++++++++++++++++
> > >> include/media/v4l2-async.h            |   4 +-
> > >> include/media/v4l2-fwnode.h           |   9 +++
> > >> 4 files changed, 173 insertions(+), 81 deletions(-)
> > > 
> > > [snip]
> > > 
> > >> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> > >> b/drivers/media/v4l2-core/v4l2-fwnode.c index 5cd2687310fe..cb0fc4b4e3bf
> > >> 100644
> > >> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > >> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > >> @@ -26,6 +26,7 @@
> > >>  #include <linux/string.h>
> > >>  #include <linux/types.h>
> > >> 
> > >> +#include <media/v4l2-async.h>
> > >>  #include <media/v4l2-fwnode.h>
> > >>  
> > >>  enum v4l2_fwnode_bus_type {
> > >> @@ -383,6 +384,130 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link
> > >> *link) }
> > >> 
> > >>  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> > >> 
> > >> +static int notifier_realloc(struct device *dev,
> > >> +			    struct v4l2_async_notifier *notifier,
> > >> +			    unsigned int max_subdevs)
> > > 
> > > It looks like you interpret the variable as an increment. You shouldn't
> > > call it max_subdevs in that case. I would however keep the name and pass
> > > the total number of subdevs instead of an increment, to mimic the realloc
> > > API.
> > 
> > Works for me.
> > 
> > >> +{
> > >> +	struct v4l2_async_subdev **subdevs;
> > >> +	unsigned int i;
> > >> +
> > >> +	if (max_subdevs + notifier->num_subdevs <= notifier->max_subdevs)
> > >> +		return 0;
> > >> +
> > >> +	subdevs = devm_kcalloc(
> > >> +		dev, max_subdevs + notifier->num_subdevs,
> > >> +		sizeof(*notifier->subdevs), GFP_KERNEL);
> > > 
> > > We know that we'll have to move away from devm_* allocation to fix object
> > > lifetime management, so we could as well start now.
> > 
> > The memory is in practice allocated using devm_() interface in existing
> > drivers.
> 
> Yes, and that's bad :-)
> 
> > The fact that it's in a single location makes it much easier getting rid of
> > it.
> 
> Great, so let's get rid of it :-)
> 
> > I'd rather get rid of memory allocation here in the first place, to be
> > replaced by a linked list. But first the user of notifier->subdevs in
> > drivers need to go. The framework interface doesn't need to change as a
> > result.
> 
> How are you going to allocate and free the linked list entries ?

Thinking about it some more --- we can't actually use other objects (not
allocated here) to hold the list struct, so we'll keep the allocation.

I think we'll need v4l2_async_notifier_release() as a result. I'll add that
to v2. This could be used to release resources related to a notifier which
is not yet registered.

> 
> > >> +	if (!subdevs)
> > >> +		return -ENOMEM;
> > >> +
> > >> +	if (notifier->subdevs) {
> > >> +		for (i = 0; i < notifier->num_subdevs; i++)
> > >> +			subdevs[i] = notifier->subdevs[i];
> > > 
> > > Is there a reason to use a loop here instead of a memcpy() covering the
> > > whole array ?
> > 
> > You could do that, yes, although I'd think this looks nicer. Performance is
> > hardly a concern here.
> 
> It's certainly not a performance issue, I would find the code easier to read.

Would you find

	memcpy(subdevs, notifier->subdevs, sizeof(*subdevs) * num_subdevs);

easier to read?

You also lose type checking as a result.

> 
> > >> +		devm_kfree(dev, notifier->subdevs);
> > >> +	}
> > >> +
> > >> +	notifier->subdevs = subdevs;
> > >> +	notifier->max_subdevs = max_subdevs + notifier->num_subdevs;
> > >> +
> > >> +	return 0;
> > >> +}
> > >> +
> > >> +static int __v4l2_fwnode_endpoint_parse(
> > >> +	struct device *dev, struct v4l2_async_notifier *notifier,
> > >> +	struct fwnode_handle *endpoint, struct v4l2_async_subdev *asd,
> > >> +	int (*parse_single)(struct device *dev,
> > >> +			    struct v4l2_fwnode_endpoint *vep,
> > >> +			    struct v4l2_async_subdev *asd))
> > >> +{
> > >> +	struct v4l2_fwnode_endpoint *vep;
> > >> +	int ret;
> > >> +
> > >> +	/* Ignore endpoints the parsing of which failed. */
> > > 
> > > Silently ignoring invalid DT sounds bad, I'd rather catch errors and
> > > return with an error code to make sure that DT gets fixed.
> > 
> > That would mean that if a single node is bad, none of the correct ones can
> > be used either. I'm not sure everyone would be happy about it.
> 
> We should be tolerant towards hardware failures and make as much of the device 
> usable as possible, but be very pedantic when parsing DT to catch errors as 
> early as possible.

How about shouting out loud about this, and continuing then?

> 
> > >> +	vep = v4l2_fwnode_endpoint_alloc_parse(endpoint);
> > >> +	if (IS_ERR(vep))
> > >> +		return 0;
> > >> +
> > >> +	notifier->subdevs[notifier->num_subdevs] = asd;
> > >> +
> > >> +	ret = parse_single(dev, vep, asd);
> > >> +	v4l2_fwnode_endpoint_free(vep);
> > >> +	if (ret)
> > >> +		return ret;
> > >> +
> > >> +	asd->match.fwnode.fwnode =
> > >> +		fwnode_graph_get_remote_port_parent(endpoint);
> > >> +	if (!asd->match.fwnode.fwnode) {
> > >> +		dev_warn(dev, "bad remote port parent\n");
> > >> +		return -EINVAL;
> > >> +	}
> > >> +
> > >> +	asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> > >> +	notifier->num_subdevs++;
> > >> +
> > >> +	return 0;
> > >> +}
> > >> +
> > >> +/**
> > >> + * v4l2_fwnode_endpoint_parse - Parse V4L2 fwnode endpoints in a device
> > >> node
> > > 
> > > This doesn't match the function name.
> > 
> > Will fix.
> > 
> > >> + * @dev: local struct device
> > > 
> > > Based on the documentation only and without a priori knowledge of the API,
> > > local struct device is very vague.
> > 
> > What would you use then?
> > 
> > Write a fairytale about it? :-)
> 
> Just explain which struct device is expected, and how it relates to the 
> purpose of the function and its other parameters. If you find that hard to 
> explain, imagine how confused someone trying to use the API without any a 
> priori knowledge of the subsystem will feel.

Having a good example is often more important than perfect documentation.
I'll add more documentation to the next version.

> 
> > >> + * @notifier: async notifier related to @dev
> > > 
> > > Ditto. You need more documentation, especially given that this is the
> > > first function in the core that fills a notifier from DT. You might also
> > > want to reflect that fact in the function name.
> > 
> > I can add more documentation.
> > 
> > >> + * @asd_struct_size: size of the driver's async sub-device struct,
> > >> including
> > >> + *		     sizeof(struct v4l2_async_subdev)
> > >> + * @parse_single: driver's callback function called on each V4L2 fwnode
> > >> endpoint
> > > 
> > > The parse_single return values should be documented.
> > 
> > Agreed.
> > 
> > >> + * Parse all V4L2 fwnode endpoints related to the device.
> > >> + *
> > >> + * Note that this function is intended for drivers to replace the
> > >> existing
> > >> + * implementation that loops over all ports and endpoints. It is NOT
> > >> INTENDED TO
> > >> + * BE USED BY NEW DRIVERS.
> > > 
> > > You should document what the preferred way is. And I'd much rather convert
> > > drivers to the preferred way instead of adding a helper function that is
> > > already deprecated.
> > 
> > The preferred way is not a part of this patch but the second one. This is
> > intended for moving the existing copies of the same code away from drivers.
> 
> You remove the code from a single driver here. How many drivers do we need to 
> convert ?

About 15, based on how many drivers use v4l2_async_notifier_register().

> 
> > The preferred way would be to explicitly check ports and endpoints in them
> > for connections. I'm not sure if the existing DT documentation is enough to
> > cover this for it does not generally document endpoint numbering.
> > 
> > >> + */
> > >> +int v4l2_fwnode_endpoints_parse(
> > > 
> > > v4l2_fwnode_parse_endpoints() would sound more natural.
> > 
> > We'll need to think more about naming this. v4l2_fwnode_endpoint_parse()
> > will parse a single endpoint and is entirely unaware of the notifier.
> > 
> > How about v4l2_async_notifier_parse_endpoints()? It's a big lengthy though.
> 
> And it's missing fwnode in the name :-)

Yes.

v4l2_async_notifier_parse_fwnode_endpoints()?

It'll still fit in practice. :-)

> 
> > >> +	struct device *dev, struct v4l2_async_notifier *notifier,
> > >> +	size_t asd_struct_size,
> > >> +	int (*parse_single)(struct device *dev,
> > >> +			    struct v4l2_fwnode_endpoint *vep,
> > >> +			    struct v4l2_async_subdev *asd))
> > >> +{
> > >> +	struct fwnode_handle *fwnode = NULL;
> > >> +	unsigned int max_subdevs = notifier->max_subdevs;
> > >> +	int ret;
> > >> +
> > >> +	if (asd_struct_size < sizeof(struct v4l2_async_subdev))
> > >> +		return -EINVAL;
> > >> +
> > >> +	while ((fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev),
> > >> +							fwnode)))
> > >> +		max_subdevs++;
> > >> +
> > >> +	ret = notifier_realloc(dev, notifier, max_subdevs);
> > >> +	if (ret)
> > >> +		return ret;
> > >> +
> > >> +	for (fwnode = NULL; (fwnode = fwnode_graph_get_next_endpoint(
> > >> +				     dev_fwnode(dev), fwnode)) &&
> > >> +		     !WARN_ON(notifier->num_subdevs >= notifier->max_subdevs);
> > > 
> > > It's nice to warn that the kernel will crash, but it would be even nicer
> > > to prevent the crash by returning an error instead of continuing parsing
> > > endpoints :-)
> > 
> > I'm not quite sure what do you mean. If the number of sub-devices reaches
> > what's allocated for them in the array, this will stop with a warning.
> 
> Oops, my bad, I've misread the code. Your for loop is not very readable :-/ 
> How about moving the num_subdevs test inside the loop with
> 
> 		if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs))
> 			break;
> 
> That should make it more readable.

I can move it inside the loop.

> 
> > >> +		) {
> > >> +		struct v4l2_async_subdev *asd;
> > >> +
> > >> +		asd = devm_kzalloc(dev, asd_struct_size, GFP_KERNEL);
> > >> +		if (!asd) {
> > >> +			ret = -ENOMEM;
> > >> +			goto error;
> > >> +		}
> > >> +
> > >> +		ret = __v4l2_fwnode_endpoint_parse(dev, notifier, fwnode, asd,
> > >> +						   parse_single);
> > >> +		if (ret < 0)
> > >> +			goto error;
> > >> +	}
> > >> +
> > >> +error:
> > >> +	fwnode_handle_put(fwnode);
> > >> +	return ret;
> > >> +}
> > >> +EXPORT_SYMBOL_GPL(v4l2_fwnode_endpoints_parse);
> 
> [snip]
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi