Rework V4L2 fwnode parsing; add defaults and avoid iteration

PATCH Superseded Sakari Ailus 1 participant 9 messages Jun 28, 2019

Subsystems (8)

Media (V4L/DVB) Arm Qualcomm mailing list Arm/Zynq architecture Qualcomm media platform +4 more subsystems

lore

Share & tools

lore

Sakari Ailus

author maintainer

2019-06-28 10:33 UTC #0 lore raw

v3 cover Rework V4L2 fwnode parsing; add defaults and avoid iteration

Hi folks,

This patchset reworks V4L2 fwnode endpoint parsing. It enables the use of endpoint configuration defaults that is available sensors and other drivers that only use a single endpoint. Well, the functionality was available already but no driver used it likely because of two reasons:
lack of any examples in a non-trivial problem area as well as lack of a needed functionality to obtain through non-iterative means in the fwnode graph API. Also the fwnode framework did not provide the most convenient APIs to perform this for drivers.

Conversion from the iterative API is done for the omap3isp and ipu3-cio2 drivers. A downside here is that this adds code: what used to be done in the framework in a one-size-fits-all fashion is now the responsibility of the driver. The benefits (default settings and simplicity of the implementation from driver's point of view) are not really achievable without some of that.

I'll send a pull request on these soonish as they've been out for review for a long time with trivial changes in this version only.

since v2:

- Postpone fwnode matching by endpoints instead of device nodes.

since v1:

- Fix a typo in ipu3-cio2 driver --- it did not compile.

- Remove unused ret variable.

- Rework the code regarding ret variable in the 2nd patch. That code did not compile until the 3rd patch fixed it.

since RFC v1:

- Add another patch to change fwnode refcounting for v4l2_async_notifier_add_fwnode_subdev

- Add a patch to fix OF node refcounting and use / put order for davinci-vpif

- Don't take endpoint reference in v4l2_async_register_subdev; that's not intended

- Fix kerneldoc documentation for
v4l2_async_notifier_add_fwnode_remote_subdev

- Fix endpoint refcounting in the patch changing fwnode parsing for the omap3isp driver

- Fixed a compiler error in rcar_drif.c --- thanks, Niklas!

Sakari Ailus (8):
davinci-vpif: Don't dereference endpoint after putting it, fix
refcounting
v4l2-async: Get fwnode reference when putting it to the notifier's
list
v4l2-async: Add v4l2_async_notifier_add_fwnode_remote_subdev
omap3isp: Rework OF endpoint parsing
v4l2-async: Safely clean up an uninitialised notifier
ipu3-cio2: Clean up notifier's subdev list if parsing endpoints fails
ipu3-cio2: Proceed with notifier init even if there are no subdevs
ipu3-cio2: Parse information from firmware without using callbacks

 drivers/media/pci/intel/ipu3/ipu3-cio2.c      |  96 ++++----
 drivers/media/platform/am437x/am437x-vpfe.c   |   5 +-
 drivers/media/platform/davinci/vpif_capture.c |  18 +-
 drivers/media/platform/omap3isp/isp.c         | 331 +++++++++++++++-----------
 drivers/media/platform/qcom/camss/camss.c     |   2 +-
 drivers/media/platform/xilinx/xilinx-vipp.c   |   2 +-
 drivers/media/v4l2-core/v4l2-async.c          |  28 ++-
 drivers/media/v4l2-core/v4l2-fwnode.c         |  23 +-
 include/media/v4l2-async.h                    |  30 ++-
 9 files changed, 322 insertions(+), 213 deletions(-)

Signature · 1 line

2.11.0

Sakari Ailus

author maintainer

2019-06-28 10:33 UTC #1 lore raw

PATCH v3 1/8 davinci-vpif: Don't dereference endpoint after putting it, fix refcounting

The davinci-vpif driver dereferences its local endpoints after releasing the reference to them.

The driver also puts its endpoints explicitly while the of_graph_get_next_endpoint() does that, too, leading to obtaining a reference once and releasing it twice.

Both are fixed by this patch.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/platform/davinci/vpif_capture.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/media/platform/davinci/vpif_capture.c +2 -3

index f0f7ef638c56..394202c28b1a 100644
@@ -1554,7 +1554,6 @@ vpif_capture_get_pdata(struct platform_device *pdev)
 		if (!rem) {
 			dev_dbg(&pdev->dev, "Remote device at %pOF not found\n",
 				endpoint);
-			of_node_put(endpoint);
 			goto done;
 		}

@@ -1566,7 +1565,6 @@ vpif_capture_get_pdata(struct platform_device *pdev)
 					    GFP_KERNEL);
 		if (!chan->inputs) {
 			of_node_put(rem);
-			of_node_put(endpoint);
 			goto err_cleanup;
 		}

@@ -1577,7 +1575,6 @@ vpif_capture_get_pdata(struct platform_device *pdev)
 
 		err = v4l2_fwnode_endpoint_parse(of_fwnode_handle(endpoint),
 						 &bus_cfg);
-		of_node_put(endpoint);
 		if (err) {
 			dev_err(&pdev->dev, "Could not parse the endpoint\n");
 			of_node_put(rem);

@@ -1608,6 +1605,7 @@ vpif_capture_get_pdata(struct platform_device *pdev)
 	}
 
 done:
+	of_node_put(endpoint);
 	pdata->asd_sizes[0] = i;
 	pdata->subdev_count = i;
 	pdata->card_name = "DA850/OMAP-L138 Video Capture";

@@ -1615,6 +1613,7 @@ vpif_capture_get_pdata(struct platform_device *pdev)
 	return pdata;
 
 err_cleanup:
+	of_node_put(endpoint);
 	v4l2_async_notifier_cleanup(&vpif_obj.notifier);
 
 	return NULL;

Signature · 1 line

2.11.0

Sakari Ailus

author maintainer

2019-06-28 10:33 UTC #2 lore raw

PATCH v3 2/8 v4l2-async: Get fwnode reference when putting it to the notifier's list

In reply to Sakari Ailus

The v4l2_async_notifier_add_fwnode_subdev() did not take a reference of the added fwnode, relying on the caller to handle that instead, in essence putting the fwnode to be added if there was an error.

As the reference is eventually released during the notifier cleanup, this is not intuitive nor logical. Improve this by always getting a reference when the function succeeds, and the caller releasing the reference when it does not *itself* need it anymore.

Luckily, perhaps, there were just a handful of callers using the function.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/platform/am437x/am437x-vpfe.c   |  5 ++---
 drivers/media/platform/davinci/vpif_capture.c | 13 ++++++-------
 drivers/media/platform/qcom/camss/camss.c     |  2 +-
 drivers/media/platform/xilinx/xilinx-vipp.c   |  2 +-
 drivers/media/v4l2-core/v4l2-async.c          |  3 ++-
 drivers/media/v4l2-core/v4l2-fwnode.c         | 23 ++++++-----------------
 include/media/v4l2-async.h                    |  5 +++--
 7 files changed, 21 insertions(+), 32 deletions(-)

drivers/media/platform/am437x/am437x-vpfe.c +2 -3

index fe7b937eb5f2..f6220e3e1f22 100644
@@ -2505,10 +2505,9 @@ vpfe_get_pdata(struct vpfe_device *vpfe)
 		pdata->asd[i] = v4l2_async_notifier_add_fwnode_subdev(
 			&vpfe->notifier, of_fwnode_handle(rem),
 			sizeof(struct v4l2_async_subdev));
-		if (IS_ERR(pdata->asd[i])) {
-			of_node_put(rem);
+		of_node_put(rem);
+		if (IS_ERR(pdata->asd[i]))
 			goto cleanup;
-		}
 	}
 
 	of_node_put(endpoint);

drivers/media/platform/davinci/vpif_capture.c +6 -7

index 394202c28b1a..091682305f37 100644
@@ -1511,6 +1511,7 @@ static struct vpif_capture_config *
 vpif_capture_get_pdata(struct platform_device *pdev)
 {
 	struct device_node *endpoint = NULL;
+	struct device_node *rem = NULL;
 	struct vpif_capture_config *pdata;
 	struct vpif_subdev_info *sdinfo;
 	struct vpif_capture_chan_config *chan;

@@ -1541,7 +1542,6 @@ vpif_capture_get_pdata(struct platform_device *pdev)
 
 	for (i = 0; i < VPIF_CAPTURE_NUM_CHANNELS; i++) {
 		struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
-		struct device_node *rem;
 		unsigned int flags;
 		int err;

@@ -1563,10 +1563,8 @@ vpif_capture_get_pdata(struct platform_device *pdev)
 					    VPIF_CAPTURE_NUM_CHANNELS,
 					    sizeof(*chan->inputs),
 					    GFP_KERNEL);
-		if (!chan->inputs) {
-			of_node_put(rem);
+		if (!chan->inputs)
 			goto err_cleanup;
-		}
 
 		chan->input_count++;
 		chan->inputs[i].input.type = V4L2_INPUT_TYPE_CAMERA;

@@ -1598,10 +1596,10 @@ vpif_capture_get_pdata(struct platform_device *pdev)
 		pdata->asd[i] = v4l2_async_notifier_add_fwnode_subdev(
 			&vpif_obj.notifier, of_fwnode_handle(rem),
 			sizeof(struct v4l2_async_subdev));
-		if (IS_ERR(pdata->asd[i])) {
-			of_node_put(rem);
+		if (IS_ERR(pdata->asd[i]))
 			goto err_cleanup;
-		}
+
+		of_node_put(rem);
 	}
 
 done:

@@ -1613,6 +1611,7 @@ vpif_capture_get_pdata(struct platform_device *pdev)
 	return pdata;
 
 err_cleanup:
+	of_node_put(rem);
 	of_node_put(endpoint);
 	v4l2_async_notifier_cleanup(&vpif_obj.notifier);

drivers/media/platform/qcom/camss/camss.c +1 -1

index 63da18773d24..3fdc9f964a3c 100644
@@ -486,9 +486,9 @@ static int camss_of_parse_ports(struct camss *camss)
 		asd = v4l2_async_notifier_add_fwnode_subdev(
 			&camss->notifier, of_fwnode_handle(remote),
 			sizeof(*csd));
+		of_node_put(remote);
 		if (IS_ERR(asd)) {
 			ret = PTR_ERR(asd);
-			of_node_put(remote);
 			goto err_cleanup;
 		}

drivers/media/platform/xilinx/xilinx-vipp.c +1 -1

index edce0402155d..cc2856efea59 100644
@@ -385,9 +385,9 @@ static int xvip_graph_parse_one(struct xvip_composite_device *xdev,
 		asd = v4l2_async_notifier_add_fwnode_subdev(
 			&xdev->notifier, remote,
 			sizeof(struct xvip_graph_entity));
+		fwnode_handle_put(remote);
 		if (IS_ERR(asd)) {
 			ret = PTR_ERR(asd);
-			fwnode_handle_put(remote);
 			goto err_notifier_cleanup;
 		}
 	}

drivers/media/v4l2-core/v4l2-async.c +2 -1

index 15b0c44a76e7..27d7ed3d5177 100644
@@ -596,10 +596,11 @@ v4l2_async_notifier_add_fwnode_subdev(struct v4l2_async_notifier *notifier,
 		return ERR_PTR(-ENOMEM);
 
 	asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
-	asd->match.fwnode = fwnode;
+	asd->match.fwnode = fwnode_handle_get(fwnode);
 
 	ret = v4l2_async_notifier_add_subdev(notifier, asd);
 	if (ret) {
+		fwnode_handle_put(fwnode);
 		kfree(asd);
 		return ERR_PTR(ret);
 	}

drivers/media/v4l2-core/v4l2-fwnode.c +6 ~~-17~~

index 7e740d332a54..1647f72fb665 100644
@@ -777,23 +777,17 @@ static int v4l2_fwnode_reference_parse(struct device *dev,
 		asd = v4l2_async_notifier_add_fwnode_subdev(notifier,
 							    args.fwnode,
 							    sizeof(*asd));
+		fwnode_handle_put(args.fwnode);
 		if (IS_ERR(asd)) {
-			ret = PTR_ERR(asd);
 			/* not an error if asd already exists */
-			if (ret == -EEXIST) {
-				fwnode_handle_put(args.fwnode);
+			if (PTR_ERR(asd) == -EEXIST)
 				continue;
-			}
 
-			goto error;
+			return PTR_ERR(asd);
 		}
 	}
 
 	return 0;
-
-error:
-	fwnode_handle_put(args.fwnode);
-	return ret;
 }
 
 /*

@@ -1083,23 +1077,18 @@ v4l2_fwnode_reference_parse_int_props(struct device *dev,
 
 		asd = v4l2_async_notifier_add_fwnode_subdev(notifier, fwnode,
 							    sizeof(*asd));
+		fwnode_handle_put(fwnode);
 		if (IS_ERR(asd)) {
 			ret = PTR_ERR(asd);
 			/* not an error if asd already exists */
-			if (ret == -EEXIST) {
-				fwnode_handle_put(fwnode);
+			if (ret == -EEXIST)
 				continue;
-			}
 
-			goto error;
+			return PTR_ERR(asd);
 		}
 	}
 
 	return !fwnode || PTR_ERR(fwnode) == -ENOENT ? 0 : PTR_ERR(fwnode);
-
-error:
-	fwnode_handle_put(fwnode);
-	return ret;
 }
 
 int v4l2_async_notifier_parse_fwnode_sensor_common(struct device *dev,

include/media/v4l2-async.h +3 -2

index 1497bda66c3b..b9141ffa188a 100644
@@ -175,8 +175,9 @@ int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
  *		     the driver's async sub-device struct, i.e. both
  *		     begin at the same memory address.
  *
- * Allocate a fwnode-matched asd of size asd_struct_size, and add it
- * to the notifiers @asd_list.
+ * Allocate a fwnode-matched asd of size asd_struct_size, and add it to the
+ * notifiers @asd_list. The function also gets a reference of the fwnode which
+ * is released later at notifier cleanup time.
  */
 struct v4l2_async_subdev *
 v4l2_async_notifier_add_fwnode_subdev(struct v4l2_async_notifier *notifier,

Signature · 1 line

2.11.0

Sakari Ailus

author maintainer

2019-06-28 10:33 UTC #3 lore raw

PATCH v3 3/8 v4l2-async: Add v4l2_async_notifier_add_fwnode_remote_subdev

In reply to Sakari Ailus

v4l2_async_notifier_add_fwnode_remote_subdev is a convenience function for parsing information on V4L2 fwnode subdevs.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/v4l2-core/v4l2-async.c | 23 +++++++++++++++++++++++
 include/media/v4l2-async.h           | 25 +++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

drivers/media/v4l2-core/v4l2-async.c +23 -0

index 27d7ed3d5177..ac686125354a 100644
@@ -609,6 +609,29 @@ v4l2_async_notifier_add_fwnode_subdev(struct v4l2_async_notifier *notifier,
 }
 EXPORT_SYMBOL_GPL(v4l2_async_notifier_add_fwnode_subdev);
 
+int
+v4l2_async_notifier_add_fwnode_remote_subdev(struct v4l2_async_notifier *notif,
+					     struct fwnode_handle *endpoint,
+					     struct v4l2_async_subdev *asd)
+{
+	struct fwnode_handle *remote;
+	int ret;
+
+	remote = fwnode_graph_get_remote_port_parent(endpoint);
+	if (!remote)
+		return -ENOTCONN;
+
+	asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
+	asd->match.fwnode = remote;
+
+	ret = v4l2_async_notifier_add_subdev(notif, asd);
+	if (ret)
+		fwnode_handle_put(remote);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_async_notifier_add_fwnode_remote_subdev);
+
 struct v4l2_async_subdev *
 v4l2_async_notifier_add_i2c_subdev(struct v4l2_async_notifier *notifier,
 				   int adapter_id, unsigned short address,

include/media/v4l2-async.h +25 -0

index b9141ffa188a..55ce3c1672a4 100644
@@ -185,6 +185,31 @@ v4l2_async_notifier_add_fwnode_subdev(struct v4l2_async_notifier *notifier,
 				      unsigned int asd_struct_size);
 
 /**
+ * v4l2_async_notifier_add_fwnode_remote_subdev - Allocate and add a fwnode
+ *						  remote async subdev to the
+ *						  notifier's master asd_list.
+ *
+ * @notif: pointer to &struct v4l2_async_notifier
+ * @endpoint: local endpoint the remote sub-device to be matched
+ * @asd: Async sub-device struct allocated by the caller. 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.
+ *
+ * Gets the remote endpoint of a given local endpoint, set it up for fwnode
+ * matching and add the async sub-device to the notifier's @asd_list. The
+ * function also gets a reference of the fwnode which is released later at
+ * notifier cleanup time.
+ *
+ * This is just like @v4l2_async_notifier_add_fwnode_subdev, but with the
+ * exception that the fwnode refers to a local endpoint, not the remote one, and
+ * the function relies on the caller to allocate the async sub-device struct.
+ */
+int
+v4l2_async_notifier_add_fwnode_remote_subdev(struct v4l2_async_notifier *notif,
+					     struct fwnode_handle *endpoint,
+					     struct v4l2_async_subdev *asd);
+
+/**
  * v4l2_async_notifier_add_i2c_subdev - Allocate and add an i2c async
  *				subdev to the notifier's master asd_list.
  *

Signature · 1 line

2.11.0

Sakari Ailus

author maintainer

2019-06-28 10:34 UTC #4 lore raw

PATCH v3 4/8 omap3isp: Rework OF endpoint parsing

In reply to Sakari Ailus

Rework OF endpoint parsing for the omap3isp driver. This does add some lines of code. The benefits are still clear:

- the great complication related to callbacks in endpoint parsing is gone; instead endpoints are obtained port by port and

- endpoints may now have a default bus configuration which was not possible while using callbacks. This driver does not benefit from that feature, but as the omap3isp is one of the exemplary drivers, this works as an example for driver developers.

Depends-on: ("device property: Add fwnode_graph_get_endpoint_by_id")

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/platform/omap3isp/isp.c | 331 ++++++++++++++++++++--------------
 1 file changed, 197 insertions(+), 134 deletions(-)

drivers/media/platform/omap3isp/isp.c +197 ~~-134~~

index 008933beebe0..5db4e3c8cd6a 100644
@@ -2017,136 +2017,6 @@ enum isp_of_phy {
 	ISP_OF_PHY_CSIPHY2,
 };
 
-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;
-	bool csi1 = false;
-	unsigned int i;
-
-	dev_dbg(dev, "parsing endpoint %pOF, interface %u\n",
-		to_of_node(vep->base.local_fwnode), 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;
-		buscfg->bus.parallel.clk_pol =
-			!!(vep->bus.parallel.flags
-			   & V4L2_MBUS_PCLK_SAMPLE_FALLING);
-		buscfg->bus.parallel.hs_pol =
-			!!(vep->bus.parallel.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW);
-		buscfg->bus.parallel.vs_pol =
-			!!(vep->bus.parallel.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW);
-		buscfg->bus.parallel.fld_pol =
-			!!(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;
-		break;
-
-	case ISP_OF_PHY_CSIPHY1:
-	case ISP_OF_PHY_CSIPHY2:
-		switch (vep->bus_type) {
-		case V4L2_MBUS_CCP2:
-		case V4L2_MBUS_CSI1:
-			dev_dbg(dev, "CSI-1/CCP-2 configuration\n");
-			csi1 = true;
-			break;
-		case V4L2_MBUS_CSI2_DPHY:
-			dev_dbg(dev, "CSI-2 configuration\n");
-			csi1 = false;
-			break;
-		default:
-			dev_err(dev, "unsupported bus type %u\n",
-				vep->bus_type);
-			return -EINVAL;
-		}
-
-		switch (vep->base.port) {
-		case ISP_OF_PHY_CSIPHY1:
-			if (csi1)
-				buscfg->interface = ISP_INTERFACE_CCP2B_PHY1;
-			else
-				buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
-			break;
-		case ISP_OF_PHY_CSIPHY2:
-			if (csi1)
-				buscfg->interface = ISP_INTERFACE_CCP2B_PHY2;
-			else
-				buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
-			break;
-		}
-		if (csi1) {
-			buscfg->bus.ccp2.lanecfg.clk.pos =
-				vep->bus.mipi_csi1.clock_lane;
-			buscfg->bus.ccp2.lanecfg.clk.pol =
-				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;
-			buscfg->bus.ccp2.lanecfg.data[0].pol =
-				vep->bus.mipi_csi1.lane_polarity[1];
-
-			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;
-			buscfg->bus.ccp2.ccp2_mode =
-				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;
-			buscfg->bus.csi2.lanecfg.clk.pol =
-				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;
-
-			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];
-				buscfg->bus.csi2.lanecfg.data[i].pol =
-					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,
-					buscfg->bus.csi2.lanecfg.data[i].pos);
-			}
-			/*
-			 * FIXME: now we assume the CRC is always there.
-			 * Implement a way to obtain this information from the
-			 * sensor. Frame descriptors, perhaps?
-			 */
-			buscfg->bus.csi2.crc = 1;
-		}
-		break;
-
-	default:
-		dev_warn(dev, "%pOF: invalid interface %u\n",
-			 to_of_node(vep->base.local_fwnode), vep->base.port);
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 {
 	struct isp_device *isp = container_of(async, struct isp_device,

@@ -2176,6 +2046,201 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 	return media_device_register(&isp->media_dev);
 }
 
+static void isp_parse_of_parallel_endpoint(struct device *dev,
+					   struct v4l2_fwnode_endpoint *vep,
+					   struct isp_bus_cfg *buscfg)
+{
+	buscfg->interface = ISP_INTERFACE_PARALLEL;
+	buscfg->bus.parallel.data_lane_shift = vep->bus.parallel.data_shift;
+	buscfg->bus.parallel.clk_pol =
+		!!(vep->bus.parallel.flags & V4L2_MBUS_PCLK_SAMPLE_FALLING);
+	buscfg->bus.parallel.hs_pol =
+		!!(vep->bus.parallel.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW);
+	buscfg->bus.parallel.vs_pol =
+		!!(vep->bus.parallel.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW);
+	buscfg->bus.parallel.fld_pol =
+		!!(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;
+}
+
+static void isp_parse_of_csi2_endpoint(struct device *dev,
+				       struct v4l2_fwnode_endpoint *vep,
+				       struct isp_bus_cfg *buscfg)
+{
+	unsigned int i;
+
+	buscfg->bus.csi2.lanecfg.clk.pos = vep->bus.mipi_csi2.clock_lane;
+	buscfg->bus.csi2.lanecfg.clk.pol =
+		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;
+
+	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];
+		buscfg->bus.csi2.lanecfg.data[i].pol =
+			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,
+			buscfg->bus.csi2.lanecfg.data[i].pos);
+	}
+	/*
+	 * FIXME: now we assume the CRC is always there. Implement a way to
+	 * obtain this information from the sensor. Frame descriptors, perhaps?
+	 */
+	buscfg->bus.csi2.crc = 1;
+}
+
+static void isp_parse_of_csi1_endpoint(struct device *dev,
+				       struct v4l2_fwnode_endpoint *vep,
+				       struct isp_bus_cfg *buscfg)
+{
+	buscfg->bus.ccp2.lanecfg.clk.pos = vep->bus.mipi_csi1.clock_lane;
+	buscfg->bus.ccp2.lanecfg.clk.pol = 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;
+	buscfg->bus.ccp2.lanecfg.data[0].pol =
+		vep->bus.mipi_csi1.lane_polarity[1];
+
+	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;
+	buscfg->bus.ccp2.ccp2_mode = vep->bus_type == V4L2_MBUS_CCP2;
+	buscfg->bus.ccp2.vp_clk_pol = 1;
+
+	buscfg->bus.ccp2.crc = 1;
+}
+
+static int isp_alloc_isd(struct isp_async_subdev **isd,
+			 struct isp_bus_cfg **buscfg)
+{
+	struct isp_async_subdev *__isd;
+
+	__isd = kzalloc(sizeof(*__isd), GFP_KERNEL);
+	if (!__isd)
+		return -ENOMEM;
+
+	*isd = __isd;
+	*buscfg = &__isd->bus;
+
+	return 0;
+}
+
+static struct {
+	u32 phy;
+	u32 csi2_if;
+	u32 csi1_if;
+} isp_bus_interfaces[2] = {
+	{ ISP_OF_PHY_CSIPHY1,
+	  ISP_INTERFACE_CSI2C_PHY1, ISP_INTERFACE_CCP2B_PHY1 },
+	{ ISP_OF_PHY_CSIPHY2,
+	  ISP_INTERFACE_CSI2A_PHY2, ISP_INTERFACE_CCP2B_PHY2 },
+};
+
+static int isp_parse_of_endpoints(struct isp_device *isp)
+{
+	struct fwnode_handle *ep;
+	struct isp_async_subdev *isd = NULL;
+	struct isp_bus_cfg *buscfg;
+	unsigned int i;
+
+	ep = fwnode_graph_get_endpoint_by_id(
+		dev_fwnode(isp->dev), ISP_OF_PHY_PARALLEL, 0,
+		FWNODE_GRAPH_ENDPOINT_NEXT);
+
+	if (ep) {
+		struct v4l2_fwnode_endpoint vep = {
+			.bus_type = V4L2_MBUS_PARALLEL
+		};
+		int ret;
+
+		dev_dbg(isp->dev, "parsing parallel interface\n");
+
+		ret = v4l2_fwnode_endpoint_parse(ep, &vep);
+
+		if (!ret) {
+			ret = isp_alloc_isd(&isd, &buscfg);
+			if (ret)
+				return ret;
+		}
+
+		if (!ret) {
+			isp_parse_of_parallel_endpoint(isp->dev, &vep, buscfg);
+			ret = v4l2_async_notifier_add_fwnode_remote_subdev(
+				&isp->notifier, ep, &isd->asd);
+		}
+
+		fwnode_handle_put(ep);
+		if (ret)
+			kfree(isd);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(isp_bus_interfaces); i++) {
+		struct v4l2_fwnode_endpoint vep = {
+			.bus_type = V4L2_MBUS_CSI2_DPHY
+		};
+		int ret;
+
+		ep = fwnode_graph_get_endpoint_by_id(
+			dev_fwnode(isp->dev), isp_bus_interfaces[i].phy, 0,
+			FWNODE_GRAPH_ENDPOINT_NEXT);
+
+		if (!ep)
+			continue;
+
+		dev_dbg(isp->dev, "parsing serial interface %u, node %pOF\n", i,
+			to_of_node(ep));
+
+		ret = isp_alloc_isd(&isd, &buscfg);
+		if (ret)
+			return ret;
+
+		ret = v4l2_fwnode_endpoint_parse(ep, &vep);
+		if (!ret) {
+			buscfg->interface = isp_bus_interfaces[i].csi2_if;
+			isp_parse_of_csi2_endpoint(isp->dev, &vep, buscfg);
+		} else if (ret == -ENXIO) {
+			vep = (struct v4l2_fwnode_endpoint)
+				{ .bus_type = V4L2_MBUS_CSI1 };
+			ret = v4l2_fwnode_endpoint_parse(ep, &vep);
+
+			if (ret == -ENXIO) {
+				vep = (struct v4l2_fwnode_endpoint)
+					{ .bus_type = V4L2_MBUS_CCP2 };
+				ret = v4l2_fwnode_endpoint_parse(ep, &vep);
+			}
+			if (!ret) {
+				buscfg->interface =
+					isp_bus_interfaces[i].csi1_if;
+				isp_parse_of_csi1_endpoint(isp->dev, &vep,
+							   buscfg);
+			}
+		}
+
+		if (!ret)
+			ret = v4l2_async_notifier_add_fwnode_remote_subdev(
+				&isp->notifier, ep, &isd->asd);
+
+		fwnode_handle_put(ep);
+		if (ret)
+			kfree(isd);
+	}
+
+	return 0;
+}
+
 static const struct v4l2_async_notifier_operations isp_subdev_notifier_ops = {
 	.complete = isp_subdev_notifier_complete,
 };

@@ -2226,14 +2291,12 @@ static int isp_probe(struct platform_device *pdev)
 	mutex_init(&isp->isp_mutex);
 	spin_lock_init(&isp->stat_lock);
 	v4l2_async_notifier_init(&isp->notifier);
+	isp->dev = &pdev->dev;
 
-	ret = v4l2_async_notifier_parse_fwnode_endpoints(
-		&pdev->dev, &isp->notifier, sizeof(struct isp_async_subdev),
-		isp_fwnode_parse);
+	ret = isp_parse_of_endpoints(isp);
 	if (ret < 0)
 		goto error;
 
-	isp->dev = &pdev->dev;
 	isp->ref_count = 0;
 
 	ret = dma_coerce_mask_and_coherent(isp->dev, DMA_BIT_MASK(32));

Signature · 1 line

2.11.0

Sakari Ailus

author maintainer

2019-06-28 10:34 UTC #5 lore raw

PATCH v3 5/8 v4l2-async: Safely clean up an uninitialised notifier

In reply to Sakari Ailus

Make the V4L2 async framework a bit more robust by allowing to clean up an uninitialised notifier. Otherwise the result would be a (close to) NULL pointer dereference.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/v4l2-core/v4l2-async.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/media/v4l2-core/v4l2-async.c +1 -1

index ac686125354a..e861e69c7096 100644
@@ -537,7 +537,7 @@ static void __v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
 {
 	struct v4l2_async_subdev *asd, *tmp;
 
-	if (!notifier)
+	if (!notifier || !notifier->asd_list.next)
 		return;
 
 	list_for_each_entry_safe(asd, tmp, &notifier->asd_list, asd_list) {

Signature · 1 line

2.11.0

Sakari Ailus

author maintainer

2019-06-28 10:34 UTC #6 lore raw

PATCH v3 6/8 ipu3-cio2: Clean up notifier's subdev list if parsing endpoints fails

In reply to Sakari Ailus

The notifier must be cleaned up whenever parsing endpoints fails. Do that to avoid a memory leak in that case.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/media/pci/intel/ipu3/ipu3-cio2.c +6 -2

index c1d133e17e4b..373970f812f4 100644
@@ -1504,7 +1504,7 @@ static int cio2_notifier_init(struct cio2_device *cio2)
 		sizeof(struct sensor_async_subdev),
 		cio2_fwnode_parse);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	if (list_empty(&cio2->notifier.asd_list))
 		return -ENODEV;	/* no endpoint */

@@ -1514,9 +1514,13 @@ static int cio2_notifier_init(struct cio2_device *cio2)
 	if (ret) {
 		dev_err(&cio2->pci_dev->dev,
 			"failed to register async notifier : %d\n", ret);
-		v4l2_async_notifier_cleanup(&cio2->notifier);
+		goto out;
 	}
 
+out:
+	if (ret)
+		v4l2_async_notifier_cleanup(&cio2->notifier);
+
 	return ret;
 }

Signature · 1 line

2.11.0

Sakari Ailus

author maintainer

2019-06-28 10:34 UTC #7 lore raw

PATCH v3 7/8 ipu3-cio2: Proceed with notifier init even if there are no subdevs

In reply to Sakari Ailus

The notifier may be registered even if there are no subdevs. Do that to simplify the code.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

drivers/media/pci/intel/ipu3/ipu3-cio2.c +5 -5

index 373970f812f4..690d3bd08ddd 100644
@@ -1506,9 +1506,10 @@ static int cio2_notifier_init(struct cio2_device *cio2)
 	if (ret < 0)
 		goto out;
 
-	if (list_empty(&cio2->notifier.asd_list))
-		return -ENODEV;	/* no endpoint */
-
+	/*
+	 * Proceed even without sensors connected to allow the device to
+	 * suspend.
+	 */
 	cio2->notifier.ops = &cio2_async_ops;
 	ret = v4l2_async_notifier_register(&cio2->v4l2_dev, &cio2->notifier);
 	if (ret) {

@@ -1815,8 +1816,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 
 	/* Register notifier for subdevices we care */
 	r = cio2_notifier_init(cio2);
-	/* Proceed without sensors connected to allow the device to suspend. */
-	if (r && r != -ENODEV)
+	if (r)
 		goto fail_cio2_queue_exit;
 
 	r = devm_request_irq(&pci_dev->dev, pci_dev->irq, cio2_irq,

Signature · 1 line

2.11.0

Sakari Ailus

author maintainer

2019-06-28 10:34 UTC #8 lore raw

PATCH v3 8/8 ipu3-cio2: Parse information from firmware without using callbacks

In reply to Sakari Ailus

Instead of using the convenience function v4l2_async_notifier_parse_fwnode_endpoints(), parse the endpoints and set up the async sub-devices without using callbacks. While this adds a little bit of code, it makes parsing the endpoints quite a bit more simple and gives more control to the driver over the process. The parsing assumes D-PHY instead of letting the V4L2 fwnode framework guess it.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 92 +++++++++++++++++---------------
 1 file changed, 49 insertions(+), 43 deletions(-)

drivers/media/pci/intel/ipu3/ipu3-cio2.c +49 ~~-43~~

index 690d3bd08ddd..40e8b8617f55 100644
@@ -1475,36 +1475,51 @@ static const struct v4l2_async_notifier_operations cio2_async_ops = {
 	.complete = cio2_notifier_complete,
 };
 
-static int cio2_fwnode_parse(struct device *dev,
-			     struct v4l2_fwnode_endpoint *vep,
-			     struct v4l2_async_subdev *asd)
+static int cio2_parse_firmware(struct cio2_device *cio2)
 {
-	struct sensor_async_subdev *s_asd =
-			container_of(asd, struct sensor_async_subdev, asd);
+	unsigned int i;
+	int ret;
 
-	if (vep->bus_type != V4L2_MBUS_CSI2_DPHY) {
-		dev_err(dev, "Only CSI2 bus type is currently supported\n");
-		return -EINVAL;
-	}
+	for (i = 0; i < CIO2_NUM_PORTS; i++) {
+		struct v4l2_fwnode_endpoint vep = {
+			.bus_type = V4L2_MBUS_CSI2_DPHY
+		};
+		struct sensor_async_subdev *s_asd = NULL;
+		struct fwnode_handle *ep;
 
-	s_asd->csi2.port = vep->base.port;
-	s_asd->csi2.lanes = vep->bus.mipi_csi2.num_data_lanes;
+		ep = fwnode_graph_get_endpoint_by_id(
+			dev_fwnode(&cio2->pci_dev->dev), i, 0,
+			FWNODE_GRAPH_ENDPOINT_NEXT);
 
-	return 0;
-}
+		if (!ep)
+			continue;
 
-static int cio2_notifier_init(struct cio2_device *cio2)
-{
-	int ret;
+		ret = v4l2_fwnode_endpoint_parse(ep, &vep);
+		if (ret)
+			goto err_parse;
 
-	v4l2_async_notifier_init(&cio2->notifier);
+		s_asd = kzalloc(sizeof(*s_asd), GFP_KERNEL);
+		if (!s_asd) {
+			ret = -ENOMEM;
+			goto err_parse;
+		}
 
-	ret = v4l2_async_notifier_parse_fwnode_endpoints(
-		&cio2->pci_dev->dev, &cio2->notifier,
-		sizeof(struct sensor_async_subdev),
-		cio2_fwnode_parse);
-	if (ret < 0)
-		goto out;
+		s_asd->csi2.port = vep.base.port;
+		s_asd->csi2.lanes = vep.bus.mipi_csi2.num_data_lanes;
+
+		ret = v4l2_async_notifier_add_fwnode_remote_subdev(
+			&cio2->notifier, ep, &s_asd->asd);
+		fwnode_handle_put(ep);
+		if (ret)
+			goto err_parse;
+
+		continue;
+
+err_parse:
+		fwnode_handle_put(ep);
+		kfree(s_asd);
+		return ret;
+	}
 
 	/*
 	 * Proceed even without sensors connected to allow the device to

@@ -1512,25 +1527,13 @@ static int cio2_notifier_init(struct cio2_device *cio2)
 	 */
 	cio2->notifier.ops = &cio2_async_ops;
 	ret = v4l2_async_notifier_register(&cio2->v4l2_dev, &cio2->notifier);
-	if (ret) {
+	if (ret)
 		dev_err(&cio2->pci_dev->dev,
 			"failed to register async notifier : %d\n", ret);
-		goto out;
-	}
-
-out:
-	if (ret)
-		v4l2_async_notifier_cleanup(&cio2->notifier);
 
 	return ret;
 }
 
-static void cio2_notifier_exit(struct cio2_device *cio2)
-{
-	v4l2_async_notifier_unregister(&cio2->notifier);
-	v4l2_async_notifier_cleanup(&cio2->notifier);
-}
-
 /**************** Queue initialization ****************/
 static const struct media_entity_operations cio2_media_ops = {
 	.link_validate = v4l2_subdev_link_validate,

@@ -1814,16 +1817,18 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 	if (r)
 		goto fail_v4l2_device_unregister;
 
+	v4l2_async_notifier_init(&cio2->notifier);
+
 	/* Register notifier for subdevices we care */
-	r = cio2_notifier_init(cio2);
+	r = cio2_parse_firmware(cio2);
 	if (r)
-		goto fail_cio2_queue_exit;
+		goto fail_clean_notifier;
 
 	r = devm_request_irq(&pci_dev->dev, pci_dev->irq, cio2_irq,
 			     IRQF_SHARED, CIO2_NAME, cio2);
 	if (r) {
 		dev_err(&pci_dev->dev, "failed to request IRQ (%d)\n", r);
-		goto fail;
+		goto fail_clean_notifier;
 	}
 
 	pm_runtime_put_noidle(&pci_dev->dev);

@@ -1831,9 +1836,9 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 
 	return 0;
 
-fail:
-	cio2_notifier_exit(cio2);
-fail_cio2_queue_exit:
+fail_clean_notifier:
+	v4l2_async_notifier_unregister(&cio2->notifier);
+	v4l2_async_notifier_cleanup(&cio2->notifier);
 	cio2_queues_exit(cio2);
 fail_v4l2_device_unregister:
 	v4l2_device_unregister(&cio2->v4l2_dev);

@@ -1852,7 +1857,8 @@ static void cio2_pci_remove(struct pci_dev *pci_dev)
 	struct cio2_device *cio2 = pci_get_drvdata(pci_dev);
 
 	media_device_unregister(&cio2->media_dev);
-	cio2_notifier_exit(cio2);
+	v4l2_async_notifier_unregister(&cio2->notifier);
+	v4l2_async_notifier_cleanup(&cio2->notifier);
 	cio2_queues_exit(cio2);
 	cio2_fbpt_exit_dummy(cio2);
 	v4l2_device_unregister(&cio2->v4l2_dev);

Signature · 1 line

2.11.0