* [PATCH v2 0/9] Rework V4L2 fwnode parsing; add defaults and avoid iteration
@ 2019-06-06 13:02 Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 1/9] davinci-vpif: Don't dereference endpoint after putting it, fix refcounting Sakari Ailus
` (8 more replies)
0 siblings, 9 replies; 21+ messages in thread
From: Sakari Ailus @ 2019-06-06 13:02 UTC (permalink / raw)
To: linux-media; +Cc: hverkuil, laurent.pinchart, jacopo, niklas.soderlund
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.
Also baked in the set is matching devices with endpoints by endpoint node
rather than the device's node. This allows finding out more information
than just the device bound (i.e. the port --- or endpoint --- through
which it was bound). Compatibility support is provided for existing
drivers by setting the fwnode to be matched based on the available
endpoints.
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 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 (9):
davinci-vpif: Don't dereference endpoint after putting it, fix
refcounting
v4l2-async: Use endpoint node, not device node, for fwnode match
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 | 7 +-
drivers/media/platform/atmel/atmel-isc.c | 2 +-
drivers/media/platform/atmel/atmel-isi.c | 2 +-
drivers/media/platform/cadence/cdns-csi2rx.c | 2 +-
drivers/media/platform/davinci/vpif_capture.c | 37 +--
drivers/media/platform/exynos4-is/media-dev.c | 14 +-
drivers/media/platform/omap3isp/isp.c | 331 +++++++++++++++-----------
drivers/media/platform/pxa_camera.c | 2 +-
drivers/media/platform/qcom/camss/camss.c | 10 +-
drivers/media/platform/rcar_drif.c | 2 +-
drivers/media/platform/renesas-ceu.c | 2 +-
drivers/media/platform/stm32/stm32-dcmi.c | 2 +-
drivers/media/platform/ti-vpe/cal.c | 2 +-
drivers/media/platform/xilinx/xilinx-vipp.c | 15 +-
drivers/media/v4l2-core/v4l2-async.c | 37 ++-
drivers/media/v4l2-core/v4l2-fwnode.c | 26 +-
drivers/staging/media/soc_camera/soc_camera.c | 14 +-
include/media/v4l2-async.h | 30 ++-
19 files changed, 386 insertions(+), 247 deletions(-)
--
2.11.0
Sakari Ailus (9):
davinci-vpif: Don't dereference endpoint after putting it, fix
refcounting
v4l2-async: Use endpoint node, not device node, for fwnode match
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 | 7 +-
drivers/media/platform/atmel/atmel-isc.c | 2 +-
drivers/media/platform/atmel/atmel-isi.c | 2 +-
drivers/media/platform/cadence/cdns-csi2rx.c | 2 +-
drivers/media/platform/davinci/vpif_capture.c | 37 +--
drivers/media/platform/exynos4-is/media-dev.c | 14 +-
drivers/media/platform/omap3isp/isp.c | 331 +++++++++++++++-----------
drivers/media/platform/pxa_camera.c | 2 +-
drivers/media/platform/qcom/camss/camss.c | 10 +-
drivers/media/platform/rcar_drif.c | 2 +-
drivers/media/platform/renesas-ceu.c | 2 +-
drivers/media/platform/stm32/stm32-dcmi.c | 2 +-
drivers/media/platform/ti-vpe/cal.c | 2 +-
drivers/media/platform/xilinx/xilinx-vipp.c | 15 +-
drivers/media/v4l2-core/v4l2-async.c | 37 ++-
drivers/media/v4l2-core/v4l2-fwnode.c | 27 +--
drivers/staging/media/soc_camera/soc_camera.c | 14 +-
include/media/v4l2-async.h | 30 ++-
19 files changed, 386 insertions(+), 248 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 1/9] davinci-vpif: Don't dereference endpoint after putting it, fix refcounting
2019-06-06 13:02 [PATCH v2 0/9] Rework V4L2 fwnode parsing; add defaults and avoid iteration Sakari Ailus
@ 2019-06-06 13:02 ` Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 2/9] v4l2-async: Use endpoint node, not device node, for fwnode match Sakari Ailus
` (7 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Sakari Ailus @ 2019-06-06 13:02 UTC (permalink / raw)
To: linux-media; +Cc: hverkuil, laurent.pinchart, jacopo, niklas.soderlund
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 <sakari.ailus@linux.intel.com>
---
drivers/media/platform/davinci/vpif_capture.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
index b5aacb0fb96b..72bdb3c10962 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -1555,7 +1555,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;
}
@@ -1567,7 +1566,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;
}
@@ -1578,7 +1576,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);
@@ -1609,6 +1606,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";
@@ -1616,6 +1614,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;
--
2.11.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 2/9] v4l2-async: Use endpoint node, not device node, for fwnode match
2019-06-06 13:02 [PATCH v2 0/9] Rework V4L2 fwnode parsing; add defaults and avoid iteration Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 1/9] davinci-vpif: Don't dereference endpoint after putting it, fix refcounting Sakari Ailus
@ 2019-06-06 13:02 ` Sakari Ailus
2019-06-14 15:45 ` Jacopo Mondi
2019-06-14 21:21 ` Niklas Söderlund
2019-06-06 13:02 ` [PATCH v2 3/9] v4l2-async: Get fwnode reference when putting it to the notifier's list Sakari Ailus
` (6 subsequent siblings)
8 siblings, 2 replies; 21+ messages in thread
From: Sakari Ailus @ 2019-06-06 13:02 UTC (permalink / raw)
To: linux-media; +Cc: hverkuil, laurent.pinchart, jacopo, niklas.soderlund
V4L2 async framework can use both device's fwnode and endpoints's fwnode
for matching the async sub-device with the sub-device. In order to proceed
moving towards endpoint matching assign the endpoint to the async
sub-device.
As most async sub-device drivers (and the related hardware) only supports
a single endpoint, use the first endpoint found. This works for all
current drivers --- we only ever supported a single async sub-device per
device to begin with.
For async devices that have no endpoints, continue to use the fwnode
related to the device. This includes e.g. lens devices.
Depends-on: ("pxa-camera: Match with device node, not the port node")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/platform/am437x/am437x-vpfe.c | 2 +-
drivers/media/platform/atmel/atmel-isc.c | 2 +-
drivers/media/platform/atmel/atmel-isi.c | 2 +-
drivers/media/platform/cadence/cdns-csi2rx.c | 2 +-
drivers/media/platform/davinci/vpif_capture.c | 20 +++++++++++++++-----
drivers/media/platform/exynos4-is/media-dev.c | 14 ++++++++++----
drivers/media/platform/pxa_camera.c | 2 +-
drivers/media/platform/qcom/camss/camss.c | 10 +++++-----
drivers/media/platform/rcar_drif.c | 2 +-
drivers/media/platform/renesas-ceu.c | 2 +-
drivers/media/platform/stm32/stm32-dcmi.c | 2 +-
drivers/media/platform/ti-vpe/cal.c | 2 +-
drivers/media/platform/xilinx/xilinx-vipp.c | 13 ++++++++++---
drivers/media/v4l2-core/v4l2-async.c | 9 +++++++--
drivers/media/v4l2-core/v4l2-fwnode.c | 8 +++-----
drivers/staging/media/soc_camera/soc_camera.c | 14 ++++++++------
16 files changed, 67 insertions(+), 39 deletions(-)
diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index fe7b937eb5f2..db263c0ce48e 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -2495,7 +2495,7 @@ vpfe_get_pdata(struct vpfe_device *vpfe)
if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
sdinfo->vpfe_param.vdpol = 1;
- rem = of_graph_get_remote_port_parent(endpoint);
+ rem = of_graph_get_remote_endpoint(endpoint);
if (!rem) {
dev_err(dev, "Remote device at %pOF not found\n",
endpoint);
diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
index da3b441e7961..fad10e6d5ecf 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -2352,7 +2352,7 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
if (!epn)
return 0;
- rem = of_graph_get_remote_port_parent(epn);
+ rem = of_graph_get_remote_endpoint(epn);
if (!rem) {
dev_notice(dev, "Remote device at %pOF not found\n",
epn);
diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
index 08b8d5583080..e4e74454e016 100644
--- a/drivers/media/platform/atmel/atmel-isi.c
+++ b/drivers/media/platform/atmel/atmel-isi.c
@@ -1110,7 +1110,7 @@ static int isi_graph_parse(struct atmel_isi *isi, struct device_node *node)
if (!ep)
return -EINVAL;
- remote = of_graph_get_remote_port_parent(ep);
+ remote = of_graph_get_remote_endpoint(ep);
of_node_put(ep);
if (!remote)
return -EINVAL;
diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
index 31ace114eda1..2da34b93e8f4 100644
--- a/drivers/media/platform/cadence/cdns-csi2rx.c
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -395,7 +395,7 @@ static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
return -EINVAL;
}
- csi2rx->asd.match.fwnode = fwnode_graph_get_remote_port_parent(fwh);
+ csi2rx->asd.match.fwnode = fwnode_graph_get_remote_endpoint(fwh);
csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
of_node_put(ep);
diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
index 72bdb3c10962..8fdea45ae090 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -1542,7 +1542,7 @@ 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;
+ struct device_node *rem, *rem_ep;
unsigned int flags;
int err;
@@ -1551,13 +1551,22 @@ vpif_capture_get_pdata(struct platform_device *pdev)
if (!endpoint)
break;
- rem = of_graph_get_remote_port_parent(endpoint);
- if (!rem) {
- dev_dbg(&pdev->dev, "Remote device at %pOF not found\n",
+ rem_ep = of_graph_get_remote_endpoint(endpoint);
+ if (!rem_ep) {
+ dev_dbg(&pdev->dev, "Remote for endpoint %pOF not found\n",
endpoint);
goto done;
}
+ rem = of_graph_get_port_parent(rem_ep);
+ if (!rem) {
+ dev_dbg(&pdev->dev, "Remote endpoint at %pOF not found\n",
+ rem_ep);
+ of_node_put(rem_ep);
+ goto done;
+ }
+ of_node_put(rem_ep);
+
sdinfo = &pdata->subdev_info[i];
chan = &pdata->chan_config[i];
chan->inputs = devm_kcalloc(&pdev->dev,
@@ -1597,12 +1606,13 @@ vpif_capture_get_pdata(struct platform_device *pdev)
sdinfo->name = rem->full_name;
pdata->asd[i] = v4l2_async_notifier_add_fwnode_subdev(
- &vpif_obj.notifier, of_fwnode_handle(rem),
+ &vpif_obj.notifier, of_fwnode_handle(rem_ep),
sizeof(struct v4l2_async_subdev));
if (IS_ERR(pdata->asd[i])) {
of_node_put(rem);
goto err_cleanup;
}
+
}
done:
diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index d1d5041cdae5..0cbc2076b94d 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -411,7 +411,7 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
pd->mux_id = (endpoint.base.port - 1) & 0x1;
- rem = of_graph_get_remote_port_parent(ep);
+ rem = of_graph_get_remote_endpoint(ep);
of_node_put(ep);
if (rem == NULL) {
v4l2_info(&fmd->v4l2_dev, "Remote device at %pOF not found\n",
@@ -1376,11 +1376,17 @@ static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
int i;
/* Find platform data for this sensor subdev */
- for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++)
- if (fmd->sensor[i].asd.match.fwnode ==
- of_fwnode_handle(subdev->dev->of_node))
+ for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++) {
+ struct fwnode_handle *fwnode =
+ fwnode_graph_get_remote_port_parent(fmd->sensor[i].asd.
+ match.fwnode);
+
+ if (fwnode == of_fwnode_handle(subdev->dev->of_node))
si = &fmd->sensor[i];
+ fwnode_handle_put(fwnode);
+ }
+
if (si == NULL)
return -EINVAL;
diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
index a7b2b89d6155..0aeba52aee13 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -2350,7 +2350,7 @@ static int pxa_camera_pdata_from_dt(struct device *dev,
pcdev->platform_flags |= PXA_CAMERA_PCLK_EN;
asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
- remote = of_graph_get_remote_port_parent(np);
+ remote = of_graph_get_remote_endpoint(np);
if (remote)
asd->match.fwnode = of_fwnode_handle(remote);
else
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 63da18773d24..a979f210f441 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -466,7 +466,7 @@ static int camss_of_parse_ports(struct camss *camss)
{
struct device *dev = camss->dev;
struct device_node *node = NULL;
- struct device_node *remote = NULL;
+ struct fwnode_handle *remote = NULL;
int ret, num_subdevs = 0;
for_each_endpoint_of_node(dev->of_node, node) {
@@ -476,7 +476,8 @@ static int camss_of_parse_ports(struct camss *camss)
if (!of_device_is_available(node))
continue;
- remote = of_graph_get_remote_port_parent(node);
+ remote = fwnode_graph_get_remote_endpoint(
+ of_fwnode_handle(node));
if (!remote) {
dev_err(dev, "Cannot get remote parent\n");
ret = -EINVAL;
@@ -484,11 +485,10 @@ static int camss_of_parse_ports(struct camss *camss)
}
asd = v4l2_async_notifier_add_fwnode_subdev(
- &camss->notifier, of_fwnode_handle(remote),
- sizeof(*csd));
+ &camss->notifier, remote, sizeof(*csd));
if (IS_ERR(asd)) {
ret = PTR_ERR(asd);
- of_node_put(remote);
+ fwnode_handle_put(remote);
goto err_cleanup;
}
diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
index 608e5217ccd5..258e14c290e8 100644
--- a/drivers/media/platform/rcar_drif.c
+++ b/drivers/media/platform/rcar_drif.c
@@ -1222,7 +1222,7 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr)
if (!ep)
return 0;
- fwnode = fwnode_graph_get_remote_port_parent(ep);
+ fwnode = fwnode_graph_get_remote_endpoint(ep);
if (!fwnode) {
dev_warn(sdr->dev, "bad remote port parent\n");
fwnode_handle_put(ep);
diff --git a/drivers/media/platform/renesas-ceu.c b/drivers/media/platform/renesas-ceu.c
index 57d0c0f9fa4b..1e625d560258 100644
--- a/drivers/media/platform/renesas-ceu.c
+++ b/drivers/media/platform/renesas-ceu.c
@@ -1581,7 +1581,7 @@ static int ceu_parse_dt(struct ceu_device *ceudev)
ceu_sd = &ceudev->subdevs[i];
INIT_LIST_HEAD(&ceu_sd->asd.list);
- remote = of_graph_get_remote_port_parent(ep);
+ remote = of_graph_get_remote_endpoint(ep);
ceu_sd->mbus_flags = fw_ep.bus.parallel.flags;
ceu_sd->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
ceu_sd->asd.match.fwnode = of_fwnode_handle(remote);
diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
index d855e9c09c08..3c91fe84e0d5 100644
--- a/drivers/media/platform/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/stm32/stm32-dcmi.c
@@ -1602,7 +1602,7 @@ static int dcmi_graph_parse(struct stm32_dcmi *dcmi, struct device_node *node)
if (!ep)
return -EINVAL;
- remote = of_graph_get_remote_port_parent(ep);
+ remote = of_graph_get_remote_endpoint(ep);
of_node_put(ep);
if (!remote)
return -EINVAL;
diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index 8d075683e448..d7995e2f4c54 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -1693,7 +1693,7 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst)
goto cleanup_exit;
}
- sensor_node = of_graph_get_remote_port_parent(ep_node);
+ sensor_node = of_graph_get_remote_endpoint(ep_node);
if (!sensor_node) {
ctx_dbg(3, ctx, "can't get remote parent\n");
goto cleanup_exit;
diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
index edce0402155d..41df417153bd 100644
--- a/drivers/media/platform/xilinx/xilinx-vipp.c
+++ b/drivers/media/platform/xilinx/xilinx-vipp.c
@@ -14,6 +14,7 @@
#include <linux/of.h>
#include <linux/of_graph.h>
#include <linux/platform_device.h>
+#include <linux/property.h>
#include <linux/slab.h>
#include <media/v4l2-async.h>
@@ -81,6 +82,8 @@ static int xvip_graph_build_one(struct xvip_composite_device *xdev,
dev_dbg(xdev->dev, "creating links for entity %s\n", local->name);
while (1) {
+ struct fwnode_handle *fwnode;
+
/* Get the next endpoint and parse its link. */
ep = fwnode_graph_get_next_endpoint(entity->asd.match.fwnode,
ep);
@@ -116,11 +119,13 @@ static int xvip_graph_build_one(struct xvip_composite_device *xdev,
continue;
}
+ fwnode = fwnode_graph_get_port_parent(link.remote_node);
+ fwnode_handle_put(fwnode);
+
/* Skip DMA engines, they will be processed separately. */
- if (link.remote_node == of_fwnode_handle(xdev->dev->of_node)) {
+ if (fwnode == dev_fwnode(xdev->dev)) {
dev_dbg(xdev->dev, "skipping DMA port %p:%u\n",
link.local_node, link.local_port);
- v4l2_fwnode_put_link(&link);
continue;
}
@@ -374,9 +379,11 @@ static int xvip_graph_parse_one(struct xvip_composite_device *xdev,
}
fwnode_handle_put(ep);
+ fwnode = fwnode_graph_get_port_parent(remote);
+ fwnode_handle_put(fwnode);
/* Skip entities that we have already processed. */
- if (remote == of_fwnode_handle(xdev->dev->of_node) ||
+ if (fwnode == dev_fwnode(xdev->dev) ||
xvip_graph_find_entity(xdev, remote)) {
fwnode_handle_put(remote);
continue;
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 15b0c44a76e7..304969ff3191 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -670,8 +670,13 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
* (struct v4l2_subdev.dev), and async sub-device does not
* exist independently of the device at any point of time.
*/
- if (!sd->fwnode && sd->dev)
- sd->fwnode = dev_fwnode(sd->dev);
+ if (!sd->fwnode && sd->dev) {
+ sd->fwnode = fwnode_graph_get_next_endpoint(
+ dev_fwnode(sd->dev), NULL);
+ fwnode_handle_put(sd->fwnode);
+ if (!sd->fwnode)
+ sd->fwnode = dev_fwnode(sd->dev);
+ }
mutex_lock(&list_lock);
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index dea8917fd912..810b6584b522 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -617,7 +617,7 @@ v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
asd->match.fwnode =
- fwnode_graph_get_remote_port_parent(endpoint);
+ fwnode_graph_get_remote_endpoint(endpoint);
if (!asd->match.fwnode) {
dev_dbg(dev, "no remote endpoint found\n");
ret = -ENOTCONN;
@@ -1051,7 +1051,6 @@ v4l2_fwnode_reference_parse_int_props(struct device *dev,
{
struct fwnode_handle *fwnode;
unsigned int index;
- int ret;
const char *prop = p->name;
const char * const *props = p->props;
unsigned int nprops = p->nprops;
@@ -1087,9 +1086,8 @@ v4l2_fwnode_reference_parse_int_props(struct device *dev,
asd = v4l2_async_notifier_add_fwnode_subdev(notifier, fwnode,
sizeof(*asd));
if (IS_ERR(asd)) {
- ret = PTR_ERR(asd);
/* not an error if asd already exists */
- if (ret == -EEXIST) {
+ if (PTR_ERR(asd) == -EEXIST) {
fwnode_handle_put(fwnode);
continue;
}
@@ -1102,7 +1100,7 @@ v4l2_fwnode_reference_parse_int_props(struct device *dev,
error:
fwnode_handle_put(fwnode);
- return ret;
+ return PTR_ERR(fwnode);
}
int v4l2_async_notifier_parse_fwnode_sensor_common(struct device *dev,
diff --git a/drivers/staging/media/soc_camera/soc_camera.c b/drivers/staging/media/soc_camera/soc_camera.c
index a6232dcd59bc..f0768b469fc2 100644
--- a/drivers/staging/media/soc_camera/soc_camera.c
+++ b/drivers/staging/media/soc_camera/soc_camera.c
@@ -1514,6 +1514,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
struct soc_camera_async_client *sasc;
struct soc_of_info *info;
struct i2c_client *client;
+ struct device_node *np;
char clk_name[V4L2_CLK_NAME_SIZE];
int ret;
@@ -1548,23 +1549,23 @@ static int soc_of_bind(struct soc_camera_host *ici,
v4l2_async_notifier_init(&sasc->notifier);
ret = v4l2_async_notifier_add_subdev(&sasc->notifier, info->subdev);
- if (ret) {
- of_node_put(remote);
+ if (ret)
goto eaddasd;
- }
sasc->notifier.ops = &soc_camera_async_ops;
icd->sasc = sasc;
icd->parent = ici->v4l2_dev.dev;
+ np = of_graph_get_port_parent(remote);
+ of_node_put(remote);
- client = of_find_i2c_device_by_node(remote);
+ client = of_find_i2c_device_by_node(np);
if (client)
v4l2_clk_name_i2c(clk_name, sizeof(clk_name),
client->adapter->nr, client->addr);
else
- v4l2_clk_name_of(clk_name, sizeof(clk_name), remote);
+ v4l2_clk_name_of(clk_name, sizeof(clk_name), np);
icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
if (IS_ERR(icd->clk)) {
@@ -1587,6 +1588,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
eallocpdev:
devm_kfree(ici->v4l2_dev.dev, info);
dev_err(ici->v4l2_dev.dev, "group probe failed: %d\n", ret);
+ of_node_put(np);
return ret;
}
@@ -1603,7 +1605,7 @@ static void scan_of_host(struct soc_camera_host *ici)
if (!epn)
break;
- rem = of_graph_get_remote_port_parent(epn);
+ rem = of_graph_get_remote_endpoint(epn);
if (!rem) {
dev_notice(dev, "no remote for %pOF\n", epn);
continue;
--
2.11.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 3/9] v4l2-async: Get fwnode reference when putting it to the notifier's list
2019-06-06 13:02 [PATCH v2 0/9] Rework V4L2 fwnode parsing; add defaults and avoid iteration Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 1/9] davinci-vpif: Don't dereference endpoint after putting it, fix refcounting Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 2/9] v4l2-async: Use endpoint node, not device node, for fwnode match Sakari Ailus
@ 2019-06-06 13:02 ` Sakari Ailus
2019-06-14 16:01 ` Jacopo Mondi
2019-06-06 13:02 ` [PATCH v2 4/9] v4l2-async: Add v4l2_async_notifier_add_fwnode_remote_subdev Sakari Ailus
` (5 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Sakari Ailus @ 2019-06-06 13:02 UTC (permalink / raw)
To: linux-media; +Cc: hverkuil, laurent.pinchart, jacopo, niklas.soderlund
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 <sakari.ailus@linux.intel.com>
---
drivers/media/platform/am437x/am437x-vpfe.c | 5 ++---
drivers/media/platform/davinci/vpif_capture.c | 16 ++++++++--------
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, 23 insertions(+), 33 deletions(-)
diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index db263c0ce48e..ccdbd9227955 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -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);
diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
index 8fdea45ae090..2a0c3f3fb443 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -1512,6 +1512,7 @@ static struct vpif_capture_config *
vpif_capture_get_pdata(struct platform_device *pdev)
{
struct device_node *endpoint = NULL;
+ struct device_node *rem = NULL, *rem_ep = NULL;
struct vpif_capture_config *pdata;
struct vpif_subdev_info *sdinfo;
struct vpif_capture_chan_config *chan;
@@ -1542,7 +1543,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, *rem_ep;
unsigned int flags;
int err;
@@ -1565,7 +1565,6 @@ vpif_capture_get_pdata(struct platform_device *pdev)
of_node_put(rem_ep);
goto done;
}
- of_node_put(rem_ep);
sdinfo = &pdata->subdev_info[i];
chan = &pdata->chan_config[i];
@@ -1573,10 +1572,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;
@@ -1588,6 +1585,7 @@ vpif_capture_get_pdata(struct platform_device *pdev)
if (err) {
dev_err(&pdev->dev, "Could not parse the endpoint\n");
of_node_put(rem);
+ of_node_put(rem_ep);
goto done;
}
@@ -1608,11 +1606,11 @@ vpif_capture_get_pdata(struct platform_device *pdev)
pdata->asd[i] = v4l2_async_notifier_add_fwnode_subdev(
&vpif_obj.notifier, of_fwnode_handle(rem_ep),
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);
+ of_node_put(rem_ep);
}
done:
@@ -1624,6 +1622,8 @@ vpif_capture_get_pdata(struct platform_device *pdev)
return pdata;
err_cleanup:
+ of_node_put(rem);
+ of_node_put(rem_ep);
of_node_put(endpoint);
v4l2_async_notifier_cleanup(&vpif_obj.notifier);
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index a979f210f441..4ab4a47d34f3 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -486,9 +486,9 @@ static int camss_of_parse_ports(struct camss *camss)
asd = v4l2_async_notifier_add_fwnode_subdev(
&camss->notifier, remote, sizeof(*csd));
+ fwnode_handle_put(remote);
if (IS_ERR(asd)) {
ret = PTR_ERR(asd);
- fwnode_handle_put(remote);
goto err_cleanup;
}
diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
index 41df417153bd..d788f4870a23 100644
--- a/drivers/media/platform/xilinx/xilinx-vipp.c
+++ b/drivers/media/platform/xilinx/xilinx-vipp.c
@@ -392,9 +392,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;
}
}
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 304969ff3191..dc4e83b4f6ba 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -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);
}
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 810b6584b522..b48725824580 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -780,23 +780,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;
}
/*
@@ -1085,22 +1079,17 @@ 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)) {
/* not an error if asd already exists */
- if (PTR_ERR(asd) == -EEXIST) {
- fwnode_handle_put(fwnode);
+ if (PTR_ERR(asd) == -EEXIST)
continue;
- }
- goto error;
+ return PTR_ERR(asd);
}
}
return PTR_ERR(fwnode) == -ENOENT ? 0 : PTR_ERR(fwnode);
-
-error:
- fwnode_handle_put(fwnode);
- return PTR_ERR(fwnode);
}
int v4l2_async_notifier_parse_fwnode_sensor_common(struct device *dev,
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index 1497bda66c3b..b9141ffa188a 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -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,
--
2.11.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 4/9] v4l2-async: Add v4l2_async_notifier_add_fwnode_remote_subdev
2019-06-06 13:02 [PATCH v2 0/9] Rework V4L2 fwnode parsing; add defaults and avoid iteration Sakari Ailus
` (2 preceding siblings ...)
2019-06-06 13:02 ` [PATCH v2 3/9] v4l2-async: Get fwnode reference when putting it to the notifier's list Sakari Ailus
@ 2019-06-06 13:02 ` Sakari Ailus
2019-06-14 16:14 ` Jacopo Mondi
2019-06-06 13:02 ` [PATCH v2 5/9] omap3isp: Rework OF endpoint parsing Sakari Ailus
` (4 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Sakari Ailus @ 2019-06-06 13:02 UTC (permalink / raw)
To: linux-media; +Cc: hverkuil, laurent.pinchart, jacopo, niklas.soderlund
v4l2_async_notifier_add_fwnode_remote_subdev is a convenience function for
parsing information on V4L2 fwnode subdevs.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/v4l2-core/v4l2-async.c | 23 +++++++++++++++++++++++
include/media/v4l2-async.h | 25 +++++++++++++++++++++++++
2 files changed, 48 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index dc4e83b4f6ba..2ea8afbcbf8f 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -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_ep;
+ int ret;
+
+ remote_ep = fwnode_graph_get_remote_endpoint(endpoint);
+ if (!remote_ep)
+ return -ENOTCONN;
+
+ asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
+ asd->match.fwnode = remote_ep;
+
+ ret = v4l2_async_notifier_add_subdev(notif, asd);
+ if (ret)
+ fwnode_handle_put(remote_ep);
+
+ 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,
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index b9141ffa188a..55ce3c1672a4 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -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.
*
--
2.11.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 5/9] omap3isp: Rework OF endpoint parsing
2019-06-06 13:02 [PATCH v2 0/9] Rework V4L2 fwnode parsing; add defaults and avoid iteration Sakari Ailus
` (3 preceding siblings ...)
2019-06-06 13:02 ` [PATCH v2 4/9] v4l2-async: Add v4l2_async_notifier_add_fwnode_remote_subdev Sakari Ailus
@ 2019-06-06 13:02 ` Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 6/9] v4l2-async: Safely clean up an uninitialised notifier Sakari Ailus
` (3 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Sakari Ailus @ 2019-06-06 13:02 UTC (permalink / raw)
To: linux-media; +Cc: hverkuil, laurent.pinchart, jacopo, niklas.soderlund
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 <sakari.ailus@linux.intel.com>
---
drivers/media/platform/omap3isp/isp.c | 331 ++++++++++++++++++++--------------
1 file changed, 197 insertions(+), 134 deletions(-)
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 008933beebe0..5db4e3c8cd6a 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -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));
--
2.11.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 6/9] v4l2-async: Safely clean up an uninitialised notifier
2019-06-06 13:02 [PATCH v2 0/9] Rework V4L2 fwnode parsing; add defaults and avoid iteration Sakari Ailus
` (4 preceding siblings ...)
2019-06-06 13:02 ` [PATCH v2 5/9] omap3isp: Rework OF endpoint parsing Sakari Ailus
@ 2019-06-06 13:02 ` Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 7/9] ipu3-cio2: Clean up notifier's subdev list if parsing endpoints fails Sakari Ailus
` (2 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Sakari Ailus @ 2019-06-06 13:02 UTC (permalink / raw)
To: linux-media; +Cc: hverkuil, laurent.pinchart, jacopo, niklas.soderlund
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 <sakari.ailus@linux.intel.com>
---
drivers/media/v4l2-core/v4l2-async.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 2ea8afbcbf8f..9107776b4550 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -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, ¬ifier->asd_list, asd_list) {
--
2.11.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 7/9] ipu3-cio2: Clean up notifier's subdev list if parsing endpoints fails
2019-06-06 13:02 [PATCH v2 0/9] Rework V4L2 fwnode parsing; add defaults and avoid iteration Sakari Ailus
` (5 preceding siblings ...)
2019-06-06 13:02 ` [PATCH v2 6/9] v4l2-async: Safely clean up an uninitialised notifier Sakari Ailus
@ 2019-06-06 13:02 ` Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 8/9] ipu3-cio2: Proceed with notifier init even if there are no subdevs Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 9/9] ipu3-cio2: Parse information from firmware without using callbacks Sakari Ailus
8 siblings, 0 replies; 21+ messages in thread
From: Sakari Ailus @ 2019-06-06 13:02 UTC (permalink / raw)
To: linux-media; +Cc: hverkuil, laurent.pinchart, jacopo, niklas.soderlund
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 <sakari.ailus@linux.intel.com>
---
drivers/media/pci/intel/ipu3/ipu3-cio2.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index c1d133e17e4b..373970f812f4 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -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;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 8/9] ipu3-cio2: Proceed with notifier init even if there are no subdevs
2019-06-06 13:02 [PATCH v2 0/9] Rework V4L2 fwnode parsing; add defaults and avoid iteration Sakari Ailus
` (6 preceding siblings ...)
2019-06-06 13:02 ` [PATCH v2 7/9] ipu3-cio2: Clean up notifier's subdev list if parsing endpoints fails Sakari Ailus
@ 2019-06-06 13:02 ` Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 9/9] ipu3-cio2: Parse information from firmware without using callbacks Sakari Ailus
8 siblings, 0 replies; 21+ messages in thread
From: Sakari Ailus @ 2019-06-06 13:02 UTC (permalink / raw)
To: linux-media; +Cc: hverkuil, laurent.pinchart, jacopo, niklas.soderlund
The notifier may be registered even if there are no subdevs. Do that to
simplify the code.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
drivers/media/pci/intel/ipu3/ipu3-cio2.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 373970f812f4..690d3bd08ddd 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -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,
--
2.11.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 9/9] ipu3-cio2: Parse information from firmware without using callbacks
2019-06-06 13:02 [PATCH v2 0/9] Rework V4L2 fwnode parsing; add defaults and avoid iteration Sakari Ailus
` (7 preceding siblings ...)
2019-06-06 13:02 ` [PATCH v2 8/9] ipu3-cio2: Proceed with notifier init even if there are no subdevs Sakari Ailus
@ 2019-06-06 13:02 ` Sakari Ailus
2019-06-14 16:31 ` Jacopo Mondi
8 siblings, 1 reply; 21+ messages in thread
From: Sakari Ailus @ 2019-06-06 13:02 UTC (permalink / raw)
To: linux-media; +Cc: hverkuil, laurent.pinchart, jacopo, niklas.soderlund
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 <sakari.ailus@linux.intel.com>
---
drivers/media/pci/intel/ipu3/ipu3-cio2.c | 92 +++++++++++++++++---------------
1 file changed, 49 insertions(+), 43 deletions(-)
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 690d3bd08ddd..40e8b8617f55 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -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);
--
2.11.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/9] v4l2-async: Use endpoint node, not device node, for fwnode match
2019-06-06 13:02 ` [PATCH v2 2/9] v4l2-async: Use endpoint node, not device node, for fwnode match Sakari Ailus
@ 2019-06-14 15:45 ` Jacopo Mondi
2019-06-14 21:21 ` Niklas Söderlund
1 sibling, 0 replies; 21+ messages in thread
From: Jacopo Mondi @ 2019-06-14 15:45 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, hverkuil, laurent.pinchart, niklas.soderlund
[-- Attachment #1: Type: text/plain, Size: 18243 bytes --]
Hi Sakari,
I'll soon test on my platforms as well. Here you have a few comments
from just reading the patch.
On Thu, Jun 06, 2019 at 04:02:18PM +0300, Sakari Ailus wrote:
> V4L2 async framework can use both device's fwnode and endpoints's fwnode
> for matching the async sub-device with the sub-device. In order to proceed
> moving towards endpoint matching assign the endpoint to the async
> sub-device.
>
> As most async sub-device drivers (and the related hardware) only supports
> a single endpoint, use the first endpoint found. This works for all
> current drivers --- we only ever supported a single async sub-device per
> device to begin with.
>
> For async devices that have no endpoints, continue to use the fwnode
> related to the device. This includes e.g. lens devices.
>
> Depends-on: ("pxa-camera: Match with device node, not the port node")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/platform/am437x/am437x-vpfe.c | 2 +-
> drivers/media/platform/atmel/atmel-isc.c | 2 +-
> drivers/media/platform/atmel/atmel-isi.c | 2 +-
> drivers/media/platform/cadence/cdns-csi2rx.c | 2 +-
> drivers/media/platform/davinci/vpif_capture.c | 20 +++++++++++++++-----
> drivers/media/platform/exynos4-is/media-dev.c | 14 ++++++++++----
> drivers/media/platform/pxa_camera.c | 2 +-
> drivers/media/platform/qcom/camss/camss.c | 10 +++++-----
> drivers/media/platform/rcar_drif.c | 2 +-
> drivers/media/platform/renesas-ceu.c | 2 +-
> drivers/media/platform/stm32/stm32-dcmi.c | 2 +-
> drivers/media/platform/ti-vpe/cal.c | 2 +-
> drivers/media/platform/xilinx/xilinx-vipp.c | 13 ++++++++++---
> drivers/media/v4l2-core/v4l2-async.c | 9 +++++++--
> drivers/media/v4l2-core/v4l2-fwnode.c | 8 +++-----
> drivers/staging/media/soc_camera/soc_camera.c | 14 ++++++++------
> 16 files changed, 67 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> index fe7b937eb5f2..db263c0ce48e 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> @@ -2495,7 +2495,7 @@ vpfe_get_pdata(struct vpfe_device *vpfe)
> if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> sdinfo->vpfe_param.vdpol = 1;
>
> - rem = of_graph_get_remote_port_parent(endpoint);
> + rem = of_graph_get_remote_endpoint(endpoint);
> if (!rem) {
> dev_err(dev, "Remote device at %pOF not found\n",
> endpoint);
> diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
> index da3b441e7961..fad10e6d5ecf 100644
> --- a/drivers/media/platform/atmel/atmel-isc.c
> +++ b/drivers/media/platform/atmel/atmel-isc.c
> @@ -2352,7 +2352,7 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
> if (!epn)
> return 0;
>
> - rem = of_graph_get_remote_port_parent(epn);
> + rem = of_graph_get_remote_endpoint(epn);
> if (!rem) {
> dev_notice(dev, "Remote device at %pOF not found\n",
> epn);
> diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
> index 08b8d5583080..e4e74454e016 100644
> --- a/drivers/media/platform/atmel/atmel-isi.c
> +++ b/drivers/media/platform/atmel/atmel-isi.c
> @@ -1110,7 +1110,7 @@ static int isi_graph_parse(struct atmel_isi *isi, struct device_node *node)
> if (!ep)
> return -EINVAL;
>
> - remote = of_graph_get_remote_port_parent(ep);
> + remote = of_graph_get_remote_endpoint(ep);
> of_node_put(ep);
> if (!remote)
> return -EINVAL;
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> index 31ace114eda1..2da34b93e8f4 100644
> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -395,7 +395,7 @@ static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
> return -EINVAL;
> }
>
> - csi2rx->asd.match.fwnode = fwnode_graph_get_remote_port_parent(fwh);
> + csi2rx->asd.match.fwnode = fwnode_graph_get_remote_endpoint(fwh);
> csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> of_node_put(ep);
>
> diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> index 72bdb3c10962..8fdea45ae090 100644
> --- a/drivers/media/platform/davinci/vpif_capture.c
> +++ b/drivers/media/platform/davinci/vpif_capture.c
> @@ -1542,7 +1542,7 @@ 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;
> + struct device_node *rem, *rem_ep;
Since you're changing this, can you move them inside the for loop or
does it get complex to properly clean up in the error path?
> unsigned int flags;
> int err;
>
> @@ -1551,13 +1551,22 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> if (!endpoint)
> break;
>
> - rem = of_graph_get_remote_port_parent(endpoint);
> - if (!rem) {
> - dev_dbg(&pdev->dev, "Remote device at %pOF not found\n",
> + rem_ep = of_graph_get_remote_endpoint(endpoint);
> + if (!rem_ep) {
> + dev_dbg(&pdev->dev, "Remote for endpoint %pOF not found\n",
> endpoint);
> goto done;
> }
>
> + rem = of_graph_get_port_parent(rem_ep);
rem is only used to initialize sdinfo->name. Could you just
of_full_name(of_graph_get_port_parent(rem_ep))
and drop rem completely? (in that case I would s/rem_ep/rem)
> + if (!rem) {
> + dev_dbg(&pdev->dev, "Remote endpoint at %pOF not found\n",
> + rem_ep);
> + of_node_put(rem_ep);
> + goto done;
> + }
> + of_node_put(rem_ep);
> +
> sdinfo = &pdata->subdev_info[i];
> chan = &pdata->chan_config[i];
> chan->inputs = devm_kcalloc(&pdev->dev,
> @@ -1597,12 +1606,13 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> sdinfo->name = rem->full_name;
>
> pdata->asd[i] = v4l2_async_notifier_add_fwnode_subdev(
> - &vpif_obj.notifier, of_fwnode_handle(rem),
> + &vpif_obj.notifier, of_fwnode_handle(rem_ep),
> sizeof(struct v4l2_async_subdev));
> if (IS_ERR(pdata->asd[i])) {
> of_node_put(rem);
> goto err_cleanup;
> }
> +
Ups
> }
>
> done:
> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> index d1d5041cdae5..0cbc2076b94d 100644
> --- a/drivers/media/platform/exynos4-is/media-dev.c
> +++ b/drivers/media/platform/exynos4-is/media-dev.c
> @@ -411,7 +411,7 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
>
> pd->mux_id = (endpoint.base.port - 1) & 0x1;
>
> - rem = of_graph_get_remote_port_parent(ep);
> + rem = of_graph_get_remote_endpoint(ep);
> of_node_put(ep);
> if (rem == NULL) {
> v4l2_info(&fmd->v4l2_dev, "Remote device at %pOF not found\n",
> @@ -1376,11 +1376,17 @@ static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
> int i;
>
> /* Find platform data for this sensor subdev */
> - for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++)
> - if (fmd->sensor[i].asd.match.fwnode ==
> - of_fwnode_handle(subdev->dev->of_node))
> + for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++) {
> + struct fwnode_handle *fwnode =
> + fwnode_graph_get_remote_port_parent(fmd->sensor[i].asd.
> + match.fwnode);
> +
> + if (fwnode == of_fwnode_handle(subdev->dev->of_node))
> si = &fmd->sensor[i];
>
> + fwnode_handle_put(fwnode);
> + }
> +
> if (si == NULL)
> return -EINVAL;
>
> diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
> index a7b2b89d6155..0aeba52aee13 100644
> --- a/drivers/media/platform/pxa_camera.c
> +++ b/drivers/media/platform/pxa_camera.c
> @@ -2350,7 +2350,7 @@ static int pxa_camera_pdata_from_dt(struct device *dev,
> pcdev->platform_flags |= PXA_CAMERA_PCLK_EN;
>
> asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> - remote = of_graph_get_remote_port_parent(np);
> + remote = of_graph_get_remote_endpoint(np);
> if (remote)
> asd->match.fwnode = of_fwnode_handle(remote);
> else
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 63da18773d24..a979f210f441 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -466,7 +466,7 @@ static int camss_of_parse_ports(struct camss *camss)
> {
> struct device *dev = camss->dev;
> struct device_node *node = NULL;
> - struct device_node *remote = NULL;
> + struct fwnode_handle *remote = NULL;
> int ret, num_subdevs = 0;
>
> for_each_endpoint_of_node(dev->of_node, node) {
> @@ -476,7 +476,8 @@ static int camss_of_parse_ports(struct camss *camss)
> if (!of_device_is_available(node))
> continue;
>
> - remote = of_graph_get_remote_port_parent(node);
> + remote = fwnode_graph_get_remote_endpoint(
> + of_fwnode_handle(node));
> if (!remote) {
> dev_err(dev, "Cannot get remote parent\n");
Could you change the error message as well?
> ret = -EINVAL;
> @@ -484,11 +485,10 @@ static int camss_of_parse_ports(struct camss *camss)
> }
>
> asd = v4l2_async_notifier_add_fwnode_subdev(
> - &camss->notifier, of_fwnode_handle(remote),
> - sizeof(*csd));
> + &camss->notifier, remote, sizeof(*csd));
> if (IS_ERR(asd)) {
> ret = PTR_ERR(asd);
> - of_node_put(remote);
> + fwnode_handle_put(remote);
> goto err_cleanup;
> }
>
> diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
> index 608e5217ccd5..258e14c290e8 100644
> --- a/drivers/media/platform/rcar_drif.c
> +++ b/drivers/media/platform/rcar_drif.c
> @@ -1222,7 +1222,7 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr)
> if (!ep)
> return 0;
>
> - fwnode = fwnode_graph_get_remote_port_parent(ep);
> + fwnode = fwnode_graph_get_remote_endpoint(ep);
> if (!fwnode) {
> dev_warn(sdr->dev, "bad remote port parent\n");
Here too. I suspect there might be others.
> fwnode_handle_put(ep);
> diff --git a/drivers/media/platform/renesas-ceu.c b/drivers/media/platform/renesas-ceu.c
> index 57d0c0f9fa4b..1e625d560258 100644
> --- a/drivers/media/platform/renesas-ceu.c
> +++ b/drivers/media/platform/renesas-ceu.c
> @@ -1581,7 +1581,7 @@ static int ceu_parse_dt(struct ceu_device *ceudev)
> ceu_sd = &ceudev->subdevs[i];
> INIT_LIST_HEAD(&ceu_sd->asd.list);
>
> - remote = of_graph_get_remote_port_parent(ep);
> + remote = of_graph_get_remote_endpoint(ep);
> ceu_sd->mbus_flags = fw_ep.bus.parallel.flags;
> ceu_sd->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> ceu_sd->asd.match.fwnode = of_fwnode_handle(remote);
> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> index d855e9c09c08..3c91fe84e0d5 100644
> --- a/drivers/media/platform/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> @@ -1602,7 +1602,7 @@ static int dcmi_graph_parse(struct stm32_dcmi *dcmi, struct device_node *node)
> if (!ep)
> return -EINVAL;
>
> - remote = of_graph_get_remote_port_parent(ep);
> + remote = of_graph_get_remote_endpoint(ep);
> of_node_put(ep);
> if (!remote)
> return -EINVAL;
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 8d075683e448..d7995e2f4c54 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -1693,7 +1693,7 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst)
> goto cleanup_exit;
> }
>
> - sensor_node = of_graph_get_remote_port_parent(ep_node);
> + sensor_node = of_graph_get_remote_endpoint(ep_node);
> if (!sensor_node) {
> ctx_dbg(3, ctx, "can't get remote parent\n");
> goto cleanup_exit;
> diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
> index edce0402155d..41df417153bd 100644
> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> @@ -14,6 +14,7 @@
> #include <linux/of.h>
> #include <linux/of_graph.h>
> #include <linux/platform_device.h>
> +#include <linux/property.h>
> #include <linux/slab.h>
>
> #include <media/v4l2-async.h>
> @@ -81,6 +82,8 @@ static int xvip_graph_build_one(struct xvip_composite_device *xdev,
> dev_dbg(xdev->dev, "creating links for entity %s\n", local->name);
>
> while (1) {
> + struct fwnode_handle *fwnode;
> +
> /* Get the next endpoint and parse its link. */
> ep = fwnode_graph_get_next_endpoint(entity->asd.match.fwnode,
> ep);
> @@ -116,11 +119,13 @@ static int xvip_graph_build_one(struct xvip_composite_device *xdev,
> continue;
> }
>
> + fwnode = fwnode_graph_get_port_parent(link.remote_node);
> + fwnode_handle_put(fwnode);
> +
> /* Skip DMA engines, they will be processed separately. */
> - if (link.remote_node == of_fwnode_handle(xdev->dev->of_node)) {
> + if (fwnode == dev_fwnode(xdev->dev)) {
> dev_dbg(xdev->dev, "skipping DMA port %p:%u\n",
> link.local_node, link.local_port);
> - v4l2_fwnode_put_link(&link);
> continue;
> }
>
> @@ -374,9 +379,11 @@ static int xvip_graph_parse_one(struct xvip_composite_device *xdev,
> }
>
> fwnode_handle_put(ep);
> + fwnode = fwnode_graph_get_port_parent(remote);
> + fwnode_handle_put(fwnode);
>
> /* Skip entities that we have already processed. */
> - if (remote == of_fwnode_handle(xdev->dev->of_node) ||
> + if (fwnode == dev_fwnode(xdev->dev) ||
> xvip_graph_find_entity(xdev, remote)) {
> fwnode_handle_put(remote);
> continue;
In this function I read
ep = fwnode_graph_get_next_endpoint(fwnode, ep);
remote = fwnode_graph_get_remote_port_parent(ep);
asd = v4l2_async_notifier_add_fwnode_subdev(
&xdev->notifier, remote,
sizeof(struct xvip_graph_entity));
shouldn't remote be set to fwnode_graph_get_remote_endpoint() ?
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 15b0c44a76e7..304969ff3191 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -670,8 +670,13 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> * (struct v4l2_subdev.dev), and async sub-device does not
> * exist independently of the device at any point of time.
> */
> - if (!sd->fwnode && sd->dev)
> - sd->fwnode = dev_fwnode(sd->dev);
> + if (!sd->fwnode && sd->dev) {
> + sd->fwnode = fwnode_graph_get_next_endpoint(
> + dev_fwnode(sd->dev), NULL);
> + fwnode_handle_put(sd->fwnode);
> + if (!sd->fwnode)
> + sd->fwnode = dev_fwnode(sd->dev);
> + }
>
> mutex_lock(&list_lock);
>
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index dea8917fd912..810b6584b522 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -617,7 +617,7 @@ v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
>
> asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> asd->match.fwnode =
> - fwnode_graph_get_remote_port_parent(endpoint);
> + fwnode_graph_get_remote_endpoint(endpoint);
> if (!asd->match.fwnode) {
> dev_dbg(dev, "no remote endpoint found\n");
> ret = -ENOTCONN;
> @@ -1051,7 +1051,6 @@ v4l2_fwnode_reference_parse_int_props(struct device *dev,
> {
> struct fwnode_handle *fwnode;
> unsigned int index;
> - int ret;
> const char *prop = p->name;
> const char * const *props = p->props;
> unsigned int nprops = p->nprops;
> @@ -1087,9 +1086,8 @@ v4l2_fwnode_reference_parse_int_props(struct device *dev,
> asd = v4l2_async_notifier_add_fwnode_subdev(notifier, fwnode,
> sizeof(*asd));
> if (IS_ERR(asd)) {
> - ret = PTR_ERR(asd);
> /* not an error if asd already exists */
> - if (ret == -EEXIST) {
> + if (PTR_ERR(asd) == -EEXIST) {
This seems unrelated. Is it just to save declaring ret?
> fwnode_handle_put(fwnode);
> continue;
> }
> @@ -1102,7 +1100,7 @@ v4l2_fwnode_reference_parse_int_props(struct device *dev,
>
> error:
> fwnode_handle_put(fwnode);
> - return ret;
> + return PTR_ERR(fwnode);
> }
>
> int v4l2_async_notifier_parse_fwnode_sensor_common(struct device *dev,
> diff --git a/drivers/staging/media/soc_camera/soc_camera.c b/drivers/staging/media/soc_camera/soc_camera.c
> index a6232dcd59bc..f0768b469fc2 100644
> --- a/drivers/staging/media/soc_camera/soc_camera.c
> +++ b/drivers/staging/media/soc_camera/soc_camera.c
> @@ -1514,6 +1514,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
> struct soc_camera_async_client *sasc;
> struct soc_of_info *info;
> struct i2c_client *client;
> + struct device_node *np;
> char clk_name[V4L2_CLK_NAME_SIZE];
> int ret;
>
> @@ -1548,23 +1549,23 @@ static int soc_of_bind(struct soc_camera_host *ici,
> v4l2_async_notifier_init(&sasc->notifier);
>
> ret = v4l2_async_notifier_add_subdev(&sasc->notifier, info->subdev);
> - if (ret) {
> - of_node_put(remote);
> + if (ret)
> goto eaddasd;
> - }
>
> sasc->notifier.ops = &soc_camera_async_ops;
>
> icd->sasc = sasc;
> icd->parent = ici->v4l2_dev.dev;
> + np = of_graph_get_port_parent(remote);
> + of_node_put(remote);
>
> - client = of_find_i2c_device_by_node(remote);
> + client = of_find_i2c_device_by_node(np);
>
> if (client)
> v4l2_clk_name_i2c(clk_name, sizeof(clk_name),
> client->adapter->nr, client->addr);
> else
> - v4l2_clk_name_of(clk_name, sizeof(clk_name), remote);
> + v4l2_clk_name_of(clk_name, sizeof(clk_name), np);
>
> icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
> if (IS_ERR(icd->clk)) {
> @@ -1587,6 +1588,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
> eallocpdev:
> devm_kfree(ici->v4l2_dev.dev, info);
> dev_err(ici->v4l2_dev.dev, "group probe failed: %d\n", ret);
> + of_node_put(np);
>
> return ret;
> }
> @@ -1603,7 +1605,7 @@ static void scan_of_host(struct soc_camera_host *ici)
> if (!epn)
> break;
>
> - rem = of_graph_get_remote_port_parent(epn);
> + rem = of_graph_get_remote_endpoint(epn);
> if (!rem) {
> dev_notice(dev, "no remote for %pOF\n", epn);
> continue;
> --
> 2.11.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/9] v4l2-async: Get fwnode reference when putting it to the notifier's list
2019-06-06 13:02 ` [PATCH v2 3/9] v4l2-async: Get fwnode reference when putting it to the notifier's list Sakari Ailus
@ 2019-06-14 16:01 ` Jacopo Mondi
2019-06-28 11:35 ` Sakari Ailus
0 siblings, 1 reply; 21+ messages in thread
From: Jacopo Mondi @ 2019-06-14 16:01 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, hverkuil, laurent.pinchart, niklas.soderlund
[-- Attachment #1: Type: text/plain, Size: 8641 bytes --]
Hi Sakari,
On Thu, Jun 06, 2019 at 04:02:19PM +0300, Sakari Ailus wrote:
> 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 <sakari.ailus@linux.intel.com>
> ---
> drivers/media/platform/am437x/am437x-vpfe.c | 5 ++---
> drivers/media/platform/davinci/vpif_capture.c | 16 ++++++++--------
> 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, 23 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> index db263c0ce48e..ccdbd9227955 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> @@ -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);
> diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> index 8fdea45ae090..2a0c3f3fb443 100644
> --- a/drivers/media/platform/davinci/vpif_capture.c
> +++ b/drivers/media/platform/davinci/vpif_capture.c
> @@ -1512,6 +1512,7 @@ static struct vpif_capture_config *
> vpif_capture_get_pdata(struct platform_device *pdev)
> {
> struct device_node *endpoint = NULL;
> + struct device_node *rem = NULL, *rem_ep = NULL;
> struct vpif_capture_config *pdata;
> struct vpif_subdev_info *sdinfo;
> struct vpif_capture_chan_config *chan;
> @@ -1542,7 +1543,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, *rem_ep;
> unsigned int flags;
> int err;
>
> @@ -1565,7 +1565,6 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> of_node_put(rem_ep);
> goto done;
> }
> - of_node_put(rem_ep);
>
> sdinfo = &pdata->subdev_info[i];
> chan = &pdata->chan_config[i];
> @@ -1573,10 +1572,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;
> @@ -1588,6 +1585,7 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> if (err) {
> dev_err(&pdev->dev, "Could not parse the endpoint\n");
> of_node_put(rem);
> + of_node_put(rem_ep);
> goto done;
> }
>
> @@ -1608,11 +1606,11 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> pdata->asd[i] = v4l2_async_notifier_add_fwnode_subdev(
> &vpif_obj.notifier, of_fwnode_handle(rem_ep),
> 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);
> + of_node_put(rem_ep);
> }
>
> done:
> @@ -1624,6 +1622,8 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> return pdata;
>
> err_cleanup:
> + of_node_put(rem);
> + of_node_put(rem_ep);
> of_node_put(endpoint);
> v4l2_async_notifier_cleanup(&vpif_obj.notifier);
>
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index a979f210f441..4ab4a47d34f3 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -486,9 +486,9 @@ static int camss_of_parse_ports(struct camss *camss)
>
> asd = v4l2_async_notifier_add_fwnode_subdev(
> &camss->notifier, remote, sizeof(*csd));
> + fwnode_handle_put(remote);
> if (IS_ERR(asd)) {
> ret = PTR_ERR(asd);
> - fwnode_handle_put(remote);
> goto err_cleanup;
> }
>
> diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
> index 41df417153bd..d788f4870a23 100644
> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> @@ -392,9 +392,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;
> }
> }
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 304969ff3191..dc4e83b4f6ba 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -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);
> }
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 810b6584b522..b48725824580 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -780,23 +780,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;
> }
>
> /*
> @@ -1085,22 +1079,17 @@ 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)) {
> /* not an error if asd already exists */
> - if (PTR_ERR(asd) == -EEXIST) {
> - fwnode_handle_put(fwnode);
> + if (PTR_ERR(asd) == -EEXIST)
> continue;
> - }
>
> - goto error;
> + return PTR_ERR(asd);
> }
> }
>
> return PTR_ERR(fwnode) == -ENOENT ? 0 : PTR_ERR(fwnode);
> -
> -error:
> - fwnode_handle_put(fwnode);
> - return PTR_ERR(fwnode);
> }
>
> int v4l2_async_notifier_parse_fwnode_sensor_common(struct device *dev,
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 1497bda66c3b..b9141ffa188a 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -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.
I would add that, as a consequence, the caller need to put their local
references as soon as they don't need them anymore (possibly just
after calling this function).
That apart
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Thanks
j
> */
> struct v4l2_async_subdev *
> v4l2_async_notifier_add_fwnode_subdev(struct v4l2_async_notifier *notifier,
> --
> 2.11.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 4/9] v4l2-async: Add v4l2_async_notifier_add_fwnode_remote_subdev
2019-06-06 13:02 ` [PATCH v2 4/9] v4l2-async: Add v4l2_async_notifier_add_fwnode_remote_subdev Sakari Ailus
@ 2019-06-14 16:14 ` Jacopo Mondi
2019-06-28 11:47 ` Sakari Ailus
0 siblings, 1 reply; 21+ messages in thread
From: Jacopo Mondi @ 2019-06-14 16:14 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, hverkuil, laurent.pinchart, niklas.soderlund
[-- Attachment #1: Type: text/plain, Size: 3901 bytes --]
Hi Sakari,
On Thu, Jun 06, 2019 at 04:02:20PM +0300, Sakari Ailus wrote:
> v4l2_async_notifier_add_fwnode_remote_subdev is a convenience function for
> parsing information on V4L2 fwnode subdevs.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/v4l2-core/v4l2-async.c | 23 +++++++++++++++++++++++
> include/media/v4l2-async.h | 25 +++++++++++++++++++++++++
> 2 files changed, 48 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index dc4e83b4f6ba..2ea8afbcbf8f 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -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_ep;
> + int ret;
> +
> + remote_ep = fwnode_graph_get_remote_endpoint(endpoint);
> + if (!remote_ep)
> + return -ENOTCONN;
> +
> + asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> + asd->match.fwnode = remote_ep;
> +
> + ret = v4l2_async_notifier_add_subdev(notif, asd);
> + if (ret)
> + fwnode_handle_put(remote_ep);
> +
> + 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,
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index b9141ffa188a..55ce3c1672a4 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -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
Not sure I get what you mean here, maybe you're missing a "to" between
the "local endpoint" and "the remote sub-device" ?
> + * @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
and adds
> + * 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.
This makes v4l2_async_notifier_add_fwnode_subdev behave differently from
v4l2_async_notifier_add_subdev and the here introduced
v4l2_async_notifier_add_remote_subdev in the sense that it's the only
one that allocates the asd for the caller. I'm not sure I see the
advantage of having many functions for doing similar things with a
slightly different interface. What is the reason add_fwnode-subdev
allocates the asd for the caller?
Thanks
j
> + */
> +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.
> *
> --
> 2.11.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 9/9] ipu3-cio2: Parse information from firmware without using callbacks
2019-06-06 13:02 ` [PATCH v2 9/9] ipu3-cio2: Parse information from firmware without using callbacks Sakari Ailus
@ 2019-06-14 16:31 ` Jacopo Mondi
2019-06-28 11:57 ` Sakari Ailus
0 siblings, 1 reply; 21+ messages in thread
From: Jacopo Mondi @ 2019-06-14 16:31 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, hverkuil, laurent.pinchart, niklas.soderlund
[-- Attachment #1: Type: text/plain, Size: 5764 bytes --]
Hi Sakari,
On Thu, Jun 06, 2019 at 04:02:25PM +0300, Sakari Ailus wrote:
> 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 <sakari.ailus@linux.intel.com>
> ---
> drivers/media/pci/intel/ipu3/ipu3-cio2.c | 92 +++++++++++++++++---------------
> 1 file changed, 49 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 690d3bd08ddd..40e8b8617f55 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -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(
How would you feel trying to remove this function completely? There
are only 2 users mainline (rcar-vin and sunxi) and this is 'yet
another way to parse an endpoint and add it to a notifier async list'.
I would say, stabilizing on the use of one of the three
v4l2_async_notifier_add_ versions could be desirable...
> - &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);
Won't the notifier cleanup put this device node for us?
This apart, for the patch itself:
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Thanks
j
> + 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);
> --
> 2.11.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/9] v4l2-async: Use endpoint node, not device node, for fwnode match
2019-06-06 13:02 ` [PATCH v2 2/9] v4l2-async: Use endpoint node, not device node, for fwnode match Sakari Ailus
2019-06-14 15:45 ` Jacopo Mondi
@ 2019-06-14 21:21 ` Niklas Söderlund
2019-06-21 9:19 ` Jacopo Mondi
2019-06-28 8:29 ` Sakari Ailus
1 sibling, 2 replies; 21+ messages in thread
From: Niklas Söderlund @ 2019-06-14 21:21 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, hverkuil, laurent.pinchart, jacopo
Hi Sakari,
Thanks for your work and sorry that I missed this and replied to v1 the
other day. I have tested v2 now and unfortunately it still breaks
rcar-vin, see bellow :-(
On 2019-06-06 16:02:18 +0300, Sakari Ailus wrote:
> V4L2 async framework can use both device's fwnode and endpoints's fwnode
> for matching the async sub-device with the sub-device. In order to proceed
> moving towards endpoint matching assign the endpoint to the async
> sub-device.
>
> As most async sub-device drivers (and the related hardware) only supports
> a single endpoint, use the first endpoint found. This works for all
> current drivers --- we only ever supported a single async sub-device per
> device to begin with.
Unfortunately this breaks for rcar-vin and adv7604 on Koelsch :-(
In DT we have the node:
i2chdmi: i2c-13 {
hdmi-in@4c {
compatible = "adi,adv7612";
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
adv7612_in: endpoint {
remote-endpoint = <&hdmi_con_in>;
};
};
port@2 {
reg = <2>;
adv7612_out: endpoint {
remote-endpoint = <&vin0ep2>;
};
};
};
};
};
- The rcar-vin in rvin_parallel_init() parses the DT using the
v4l2_async_notifier_parse_fwnode_endpoints_by_port() helper and
registers '/i2c-13/hdmi-in@4c/ports/port@2/endpoint' with the async
framework.
- The adv7604 register itself with async using
'/i2c-13/hdmi-in@4c/ports/port@0/endpoint'.
The result is that matching breaks and the two devices never find each
other. I'm not sure how to solve this :-( Maybe the subdevices could
register itself multiple times with the async framework, once for each
port described?
That would open up for the same subdev to be bound multiple times,
possibly to different consumers. I think that is a direction that would
be useful but I fear there might be some work involved in allowing that
from a subdev point of view.
>
> For async devices that have no endpoints, continue to use the fwnode
> related to the device. This includes e.g. lens devices.
>
> Depends-on: ("pxa-camera: Match with device node, not the port node")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/platform/am437x/am437x-vpfe.c | 2 +-
> drivers/media/platform/atmel/atmel-isc.c | 2 +-
> drivers/media/platform/atmel/atmel-isi.c | 2 +-
> drivers/media/platform/cadence/cdns-csi2rx.c | 2 +-
> drivers/media/platform/davinci/vpif_capture.c | 20 +++++++++++++++-----
> drivers/media/platform/exynos4-is/media-dev.c | 14 ++++++++++----
> drivers/media/platform/pxa_camera.c | 2 +-
> drivers/media/platform/qcom/camss/camss.c | 10 +++++-----
> drivers/media/platform/rcar_drif.c | 2 +-
> drivers/media/platform/renesas-ceu.c | 2 +-
> drivers/media/platform/stm32/stm32-dcmi.c | 2 +-
> drivers/media/platform/ti-vpe/cal.c | 2 +-
> drivers/media/platform/xilinx/xilinx-vipp.c | 13 ++++++++++---
> drivers/media/v4l2-core/v4l2-async.c | 9 +++++++--
> drivers/media/v4l2-core/v4l2-fwnode.c | 8 +++-----
> drivers/staging/media/soc_camera/soc_camera.c | 14 ++++++++------
> 16 files changed, 67 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> index fe7b937eb5f2..db263c0ce48e 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> @@ -2495,7 +2495,7 @@ vpfe_get_pdata(struct vpfe_device *vpfe)
> if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> sdinfo->vpfe_param.vdpol = 1;
>
> - rem = of_graph_get_remote_port_parent(endpoint);
> + rem = of_graph_get_remote_endpoint(endpoint);
> if (!rem) {
> dev_err(dev, "Remote device at %pOF not found\n",
> endpoint);
> diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
> index da3b441e7961..fad10e6d5ecf 100644
> --- a/drivers/media/platform/atmel/atmel-isc.c
> +++ b/drivers/media/platform/atmel/atmel-isc.c
> @@ -2352,7 +2352,7 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
> if (!epn)
> return 0;
>
> - rem = of_graph_get_remote_port_parent(epn);
> + rem = of_graph_get_remote_endpoint(epn);
> if (!rem) {
> dev_notice(dev, "Remote device at %pOF not found\n",
> epn);
> diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
> index 08b8d5583080..e4e74454e016 100644
> --- a/drivers/media/platform/atmel/atmel-isi.c
> +++ b/drivers/media/platform/atmel/atmel-isi.c
> @@ -1110,7 +1110,7 @@ static int isi_graph_parse(struct atmel_isi *isi, struct device_node *node)
> if (!ep)
> return -EINVAL;
>
> - remote = of_graph_get_remote_port_parent(ep);
> + remote = of_graph_get_remote_endpoint(ep);
> of_node_put(ep);
> if (!remote)
> return -EINVAL;
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> index 31ace114eda1..2da34b93e8f4 100644
> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -395,7 +395,7 @@ static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
> return -EINVAL;
> }
>
> - csi2rx->asd.match.fwnode = fwnode_graph_get_remote_port_parent(fwh);
> + csi2rx->asd.match.fwnode = fwnode_graph_get_remote_endpoint(fwh);
> csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> of_node_put(ep);
>
> diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> index 72bdb3c10962..8fdea45ae090 100644
> --- a/drivers/media/platform/davinci/vpif_capture.c
> +++ b/drivers/media/platform/davinci/vpif_capture.c
> @@ -1542,7 +1542,7 @@ 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;
> + struct device_node *rem, *rem_ep;
> unsigned int flags;
> int err;
>
> @@ -1551,13 +1551,22 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> if (!endpoint)
> break;
>
> - rem = of_graph_get_remote_port_parent(endpoint);
> - if (!rem) {
> - dev_dbg(&pdev->dev, "Remote device at %pOF not found\n",
> + rem_ep = of_graph_get_remote_endpoint(endpoint);
> + if (!rem_ep) {
> + dev_dbg(&pdev->dev, "Remote for endpoint %pOF not found\n",
> endpoint);
> goto done;
> }
>
> + rem = of_graph_get_port_parent(rem_ep);
> + if (!rem) {
> + dev_dbg(&pdev->dev, "Remote endpoint at %pOF not found\n",
> + rem_ep);
> + of_node_put(rem_ep);
> + goto done;
> + }
> + of_node_put(rem_ep);
> +
> sdinfo = &pdata->subdev_info[i];
> chan = &pdata->chan_config[i];
> chan->inputs = devm_kcalloc(&pdev->dev,
> @@ -1597,12 +1606,13 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> sdinfo->name = rem->full_name;
>
> pdata->asd[i] = v4l2_async_notifier_add_fwnode_subdev(
> - &vpif_obj.notifier, of_fwnode_handle(rem),
> + &vpif_obj.notifier, of_fwnode_handle(rem_ep),
> sizeof(struct v4l2_async_subdev));
> if (IS_ERR(pdata->asd[i])) {
> of_node_put(rem);
> goto err_cleanup;
> }
> +
> }
>
> done:
> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> index d1d5041cdae5..0cbc2076b94d 100644
> --- a/drivers/media/platform/exynos4-is/media-dev.c
> +++ b/drivers/media/platform/exynos4-is/media-dev.c
> @@ -411,7 +411,7 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
>
> pd->mux_id = (endpoint.base.port - 1) & 0x1;
>
> - rem = of_graph_get_remote_port_parent(ep);
> + rem = of_graph_get_remote_endpoint(ep);
> of_node_put(ep);
> if (rem == NULL) {
> v4l2_info(&fmd->v4l2_dev, "Remote device at %pOF not found\n",
> @@ -1376,11 +1376,17 @@ static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
> int i;
>
> /* Find platform data for this sensor subdev */
> - for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++)
> - if (fmd->sensor[i].asd.match.fwnode ==
> - of_fwnode_handle(subdev->dev->of_node))
> + for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++) {
> + struct fwnode_handle *fwnode =
> + fwnode_graph_get_remote_port_parent(fmd->sensor[i].asd.
> + match.fwnode);
> +
> + if (fwnode == of_fwnode_handle(subdev->dev->of_node))
> si = &fmd->sensor[i];
>
> + fwnode_handle_put(fwnode);
> + }
> +
> if (si == NULL)
> return -EINVAL;
>
> diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
> index a7b2b89d6155..0aeba52aee13 100644
> --- a/drivers/media/platform/pxa_camera.c
> +++ b/drivers/media/platform/pxa_camera.c
> @@ -2350,7 +2350,7 @@ static int pxa_camera_pdata_from_dt(struct device *dev,
> pcdev->platform_flags |= PXA_CAMERA_PCLK_EN;
>
> asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> - remote = of_graph_get_remote_port_parent(np);
> + remote = of_graph_get_remote_endpoint(np);
> if (remote)
> asd->match.fwnode = of_fwnode_handle(remote);
> else
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 63da18773d24..a979f210f441 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -466,7 +466,7 @@ static int camss_of_parse_ports(struct camss *camss)
> {
> struct device *dev = camss->dev;
> struct device_node *node = NULL;
> - struct device_node *remote = NULL;
> + struct fwnode_handle *remote = NULL;
> int ret, num_subdevs = 0;
>
> for_each_endpoint_of_node(dev->of_node, node) {
> @@ -476,7 +476,8 @@ static int camss_of_parse_ports(struct camss *camss)
> if (!of_device_is_available(node))
> continue;
>
> - remote = of_graph_get_remote_port_parent(node);
> + remote = fwnode_graph_get_remote_endpoint(
> + of_fwnode_handle(node));
> if (!remote) {
> dev_err(dev, "Cannot get remote parent\n");
> ret = -EINVAL;
> @@ -484,11 +485,10 @@ static int camss_of_parse_ports(struct camss *camss)
> }
>
> asd = v4l2_async_notifier_add_fwnode_subdev(
> - &camss->notifier, of_fwnode_handle(remote),
> - sizeof(*csd));
> + &camss->notifier, remote, sizeof(*csd));
> if (IS_ERR(asd)) {
> ret = PTR_ERR(asd);
> - of_node_put(remote);
> + fwnode_handle_put(remote);
> goto err_cleanup;
> }
>
> diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
> index 608e5217ccd5..258e14c290e8 100644
> --- a/drivers/media/platform/rcar_drif.c
> +++ b/drivers/media/platform/rcar_drif.c
> @@ -1222,7 +1222,7 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr)
> if (!ep)
> return 0;
>
> - fwnode = fwnode_graph_get_remote_port_parent(ep);
> + fwnode = fwnode_graph_get_remote_endpoint(ep);
> if (!fwnode) {
> dev_warn(sdr->dev, "bad remote port parent\n");
> fwnode_handle_put(ep);
> diff --git a/drivers/media/platform/renesas-ceu.c b/drivers/media/platform/renesas-ceu.c
> index 57d0c0f9fa4b..1e625d560258 100644
> --- a/drivers/media/platform/renesas-ceu.c
> +++ b/drivers/media/platform/renesas-ceu.c
> @@ -1581,7 +1581,7 @@ static int ceu_parse_dt(struct ceu_device *ceudev)
> ceu_sd = &ceudev->subdevs[i];
> INIT_LIST_HEAD(&ceu_sd->asd.list);
>
> - remote = of_graph_get_remote_port_parent(ep);
> + remote = of_graph_get_remote_endpoint(ep);
> ceu_sd->mbus_flags = fw_ep.bus.parallel.flags;
> ceu_sd->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> ceu_sd->asd.match.fwnode = of_fwnode_handle(remote);
> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> index d855e9c09c08..3c91fe84e0d5 100644
> --- a/drivers/media/platform/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> @@ -1602,7 +1602,7 @@ static int dcmi_graph_parse(struct stm32_dcmi *dcmi, struct device_node *node)
> if (!ep)
> return -EINVAL;
>
> - remote = of_graph_get_remote_port_parent(ep);
> + remote = of_graph_get_remote_endpoint(ep);
> of_node_put(ep);
> if (!remote)
> return -EINVAL;
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index 8d075683e448..d7995e2f4c54 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -1693,7 +1693,7 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst)
> goto cleanup_exit;
> }
>
> - sensor_node = of_graph_get_remote_port_parent(ep_node);
> + sensor_node = of_graph_get_remote_endpoint(ep_node);
> if (!sensor_node) {
> ctx_dbg(3, ctx, "can't get remote parent\n");
> goto cleanup_exit;
> diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
> index edce0402155d..41df417153bd 100644
> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> @@ -14,6 +14,7 @@
> #include <linux/of.h>
> #include <linux/of_graph.h>
> #include <linux/platform_device.h>
> +#include <linux/property.h>
> #include <linux/slab.h>
>
> #include <media/v4l2-async.h>
> @@ -81,6 +82,8 @@ static int xvip_graph_build_one(struct xvip_composite_device *xdev,
> dev_dbg(xdev->dev, "creating links for entity %s\n", local->name);
>
> while (1) {
> + struct fwnode_handle *fwnode;
> +
> /* Get the next endpoint and parse its link. */
> ep = fwnode_graph_get_next_endpoint(entity->asd.match.fwnode,
> ep);
> @@ -116,11 +119,13 @@ static int xvip_graph_build_one(struct xvip_composite_device *xdev,
> continue;
> }
>
> + fwnode = fwnode_graph_get_port_parent(link.remote_node);
> + fwnode_handle_put(fwnode);
> +
> /* Skip DMA engines, they will be processed separately. */
> - if (link.remote_node == of_fwnode_handle(xdev->dev->of_node)) {
> + if (fwnode == dev_fwnode(xdev->dev)) {
> dev_dbg(xdev->dev, "skipping DMA port %p:%u\n",
> link.local_node, link.local_port);
> - v4l2_fwnode_put_link(&link);
> continue;
> }
>
> @@ -374,9 +379,11 @@ static int xvip_graph_parse_one(struct xvip_composite_device *xdev,
> }
>
> fwnode_handle_put(ep);
> + fwnode = fwnode_graph_get_port_parent(remote);
> + fwnode_handle_put(fwnode);
>
> /* Skip entities that we have already processed. */
> - if (remote == of_fwnode_handle(xdev->dev->of_node) ||
> + if (fwnode == dev_fwnode(xdev->dev) ||
> xvip_graph_find_entity(xdev, remote)) {
> fwnode_handle_put(remote);
> continue;
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 15b0c44a76e7..304969ff3191 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -670,8 +670,13 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> * (struct v4l2_subdev.dev), and async sub-device does not
> * exist independently of the device at any point of time.
> */
> - if (!sd->fwnode && sd->dev)
> - sd->fwnode = dev_fwnode(sd->dev);
> + if (!sd->fwnode && sd->dev) {
> + sd->fwnode = fwnode_graph_get_next_endpoint(
> + dev_fwnode(sd->dev), NULL);
> + fwnode_handle_put(sd->fwnode);
> + if (!sd->fwnode)
> + sd->fwnode = dev_fwnode(sd->dev);
> + }
>
> mutex_lock(&list_lock);
>
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index dea8917fd912..810b6584b522 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -617,7 +617,7 @@ v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
>
> asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> asd->match.fwnode =
> - fwnode_graph_get_remote_port_parent(endpoint);
> + fwnode_graph_get_remote_endpoint(endpoint);
> if (!asd->match.fwnode) {
> dev_dbg(dev, "no remote endpoint found\n");
> ret = -ENOTCONN;
> @@ -1051,7 +1051,6 @@ v4l2_fwnode_reference_parse_int_props(struct device *dev,
> {
> struct fwnode_handle *fwnode;
> unsigned int index;
> - int ret;
> const char *prop = p->name;
> const char * const *props = p->props;
> unsigned int nprops = p->nprops;
> @@ -1087,9 +1086,8 @@ v4l2_fwnode_reference_parse_int_props(struct device *dev,
> asd = v4l2_async_notifier_add_fwnode_subdev(notifier, fwnode,
> sizeof(*asd));
> if (IS_ERR(asd)) {
> - ret = PTR_ERR(asd);
> /* not an error if asd already exists */
> - if (ret == -EEXIST) {
> + if (PTR_ERR(asd) == -EEXIST) {
> fwnode_handle_put(fwnode);
> continue;
> }
> @@ -1102,7 +1100,7 @@ v4l2_fwnode_reference_parse_int_props(struct device *dev,
>
> error:
> fwnode_handle_put(fwnode);
> - return ret;
> + return PTR_ERR(fwnode);
> }
>
> int v4l2_async_notifier_parse_fwnode_sensor_common(struct device *dev,
> diff --git a/drivers/staging/media/soc_camera/soc_camera.c b/drivers/staging/media/soc_camera/soc_camera.c
> index a6232dcd59bc..f0768b469fc2 100644
> --- a/drivers/staging/media/soc_camera/soc_camera.c
> +++ b/drivers/staging/media/soc_camera/soc_camera.c
> @@ -1514,6 +1514,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
> struct soc_camera_async_client *sasc;
> struct soc_of_info *info;
> struct i2c_client *client;
> + struct device_node *np;
> char clk_name[V4L2_CLK_NAME_SIZE];
> int ret;
>
> @@ -1548,23 +1549,23 @@ static int soc_of_bind(struct soc_camera_host *ici,
> v4l2_async_notifier_init(&sasc->notifier);
>
> ret = v4l2_async_notifier_add_subdev(&sasc->notifier, info->subdev);
> - if (ret) {
> - of_node_put(remote);
> + if (ret)
> goto eaddasd;
> - }
>
> sasc->notifier.ops = &soc_camera_async_ops;
>
> icd->sasc = sasc;
> icd->parent = ici->v4l2_dev.dev;
> + np = of_graph_get_port_parent(remote);
> + of_node_put(remote);
>
> - client = of_find_i2c_device_by_node(remote);
> + client = of_find_i2c_device_by_node(np);
>
> if (client)
> v4l2_clk_name_i2c(clk_name, sizeof(clk_name),
> client->adapter->nr, client->addr);
> else
> - v4l2_clk_name_of(clk_name, sizeof(clk_name), remote);
> + v4l2_clk_name_of(clk_name, sizeof(clk_name), np);
>
> icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
> if (IS_ERR(icd->clk)) {
> @@ -1587,6 +1588,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
> eallocpdev:
> devm_kfree(ici->v4l2_dev.dev, info);
> dev_err(ici->v4l2_dev.dev, "group probe failed: %d\n", ret);
> + of_node_put(np);
>
> return ret;
> }
> @@ -1603,7 +1605,7 @@ static void scan_of_host(struct soc_camera_host *ici)
> if (!epn)
> break;
>
> - rem = of_graph_get_remote_port_parent(epn);
> + rem = of_graph_get_remote_endpoint(epn);
> if (!rem) {
> dev_notice(dev, "no remote for %pOF\n", epn);
> continue;
> --
> 2.11.0
>
--
Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/9] v4l2-async: Use endpoint node, not device node, for fwnode match
2019-06-14 21:21 ` Niklas Söderlund
@ 2019-06-21 9:19 ` Jacopo Mondi
2019-06-21 11:43 ` Niklas Söderlund
2019-06-28 8:29 ` Sakari Ailus
1 sibling, 1 reply; 21+ messages in thread
From: Jacopo Mondi @ 2019-06-21 9:19 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Sakari Ailus, linux-media, hverkuil, laurent.pinchart
[-- Attachment #1: Type: text/plain, Size: 20901 bytes --]
Hi Niklas,
On Fri, Jun 14, 2019 at 11:21:05PM +0200, Niklas Söderlund wrote:
> Hi Sakari,
>
> Thanks for your work and sorry that I missed this and replied to v1 the
> other day. I have tested v2 now and unfortunately it still breaks
> rcar-vin, see bellow :-(
>
> On 2019-06-06 16:02:18 +0300, Sakari Ailus wrote:
> > V4L2 async framework can use both device's fwnode and endpoints's fwnode
> > for matching the async sub-device with the sub-device. In order to proceed
> > moving towards endpoint matching assign the endpoint to the async
> > sub-device.
> >
> > As most async sub-device drivers (and the related hardware) only supports
> > a single endpoint, use the first endpoint found. This works for all
> > current drivers --- we only ever supported a single async sub-device per
> > device to begin with.
>
> Unfortunately this breaks for rcar-vin and adv7604 on Koelsch :-(
>
> In DT we have the node:
>
> i2chdmi: i2c-13 {
> hdmi-in@4c {
> compatible = "adi,adv7612";
>
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
>
> port@0 {
> reg = <0>;
> adv7612_in: endpoint {
> remote-endpoint = <&hdmi_con_in>;
> };
> };
>
> port@2 {
> reg = <2>;
> adv7612_out: endpoint {
> remote-endpoint = <&vin0ep2>;
> };
> };
> };
> };
> };
>
> - The rcar-vin in rvin_parallel_init() parses the DT using the
> v4l2_async_notifier_parse_fwnode_endpoints_by_port() helper and
> registers '/i2c-13/hdmi-in@4c/ports/port@2/endpoint' with the async
> framework.
>
> - The adv7604 register itself with async using
> '/i2c-13/hdmi-in@4c/ports/port@0/endpoint'.
>
> The result is that matching breaks and the two devices never find each
> other. I'm not sure how to solve this :-( Maybe the subdevices could
> register itself multiple times with the async framework, once for each
> port described?
Isn't this an issue on adv7604 side? With Sakari's patches the fwnode
of the first available endpoint is used for matching, and in case of
adv7604 is the one that connects to the HDMI connector.
Shouldn't the adv7604 driver register its async subdev by using the
fwnode handle of its only source port instead?
That said, I wonder how many of these similar situation would go
unoticed without someone doing the testing, as this is a run-time
failure hard to detect at development time.
>
> That would open up for the same subdev to be bound multiple times,
> possibly to different consumers. I think that is a direction that would
> be useful but I fear there might be some work involved in allowing that
> from a subdev point of view.
>
> >
> > For async devices that have no endpoints, continue to use the fwnode
> > related to the device. This includes e.g. lens devices.
> >
> > Depends-on: ("pxa-camera: Match with device node, not the port node")
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/platform/am437x/am437x-vpfe.c | 2 +-
> > drivers/media/platform/atmel/atmel-isc.c | 2 +-
> > drivers/media/platform/atmel/atmel-isi.c | 2 +-
> > drivers/media/platform/cadence/cdns-csi2rx.c | 2 +-
> > drivers/media/platform/davinci/vpif_capture.c | 20 +++++++++++++++-----
> > drivers/media/platform/exynos4-is/media-dev.c | 14 ++++++++++----
> > drivers/media/platform/pxa_camera.c | 2 +-
> > drivers/media/platform/qcom/camss/camss.c | 10 +++++-----
> > drivers/media/platform/rcar_drif.c | 2 +-
> > drivers/media/platform/renesas-ceu.c | 2 +-
> > drivers/media/platform/stm32/stm32-dcmi.c | 2 +-
> > drivers/media/platform/ti-vpe/cal.c | 2 +-
> > drivers/media/platform/xilinx/xilinx-vipp.c | 13 ++++++++++---
> > drivers/media/v4l2-core/v4l2-async.c | 9 +++++++--
> > drivers/media/v4l2-core/v4l2-fwnode.c | 8 +++-----
> > drivers/staging/media/soc_camera/soc_camera.c | 14 ++++++++------
> > 16 files changed, 67 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> > index fe7b937eb5f2..db263c0ce48e 100644
> > --- a/drivers/media/platform/am437x/am437x-vpfe.c
> > +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> > @@ -2495,7 +2495,7 @@ vpfe_get_pdata(struct vpfe_device *vpfe)
> > if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> > sdinfo->vpfe_param.vdpol = 1;
> >
> > - rem = of_graph_get_remote_port_parent(endpoint);
> > + rem = of_graph_get_remote_endpoint(endpoint);
> > if (!rem) {
> > dev_err(dev, "Remote device at %pOF not found\n",
> > endpoint);
> > diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
> > index da3b441e7961..fad10e6d5ecf 100644
> > --- a/drivers/media/platform/atmel/atmel-isc.c
> > +++ b/drivers/media/platform/atmel/atmel-isc.c
> > @@ -2352,7 +2352,7 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
> > if (!epn)
> > return 0;
> >
> > - rem = of_graph_get_remote_port_parent(epn);
> > + rem = of_graph_get_remote_endpoint(epn);
> > if (!rem) {
> > dev_notice(dev, "Remote device at %pOF not found\n",
> > epn);
> > diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
> > index 08b8d5583080..e4e74454e016 100644
> > --- a/drivers/media/platform/atmel/atmel-isi.c
> > +++ b/drivers/media/platform/atmel/atmel-isi.c
> > @@ -1110,7 +1110,7 @@ static int isi_graph_parse(struct atmel_isi *isi, struct device_node *node)
> > if (!ep)
> > return -EINVAL;
> >
> > - remote = of_graph_get_remote_port_parent(ep);
> > + remote = of_graph_get_remote_endpoint(ep);
> > of_node_put(ep);
> > if (!remote)
> > return -EINVAL;
> > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> > index 31ace114eda1..2da34b93e8f4 100644
> > --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> > @@ -395,7 +395,7 @@ static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
> > return -EINVAL;
> > }
> >
> > - csi2rx->asd.match.fwnode = fwnode_graph_get_remote_port_parent(fwh);
> > + csi2rx->asd.match.fwnode = fwnode_graph_get_remote_endpoint(fwh);
> > csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > of_node_put(ep);
> >
> > diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> > index 72bdb3c10962..8fdea45ae090 100644
> > --- a/drivers/media/platform/davinci/vpif_capture.c
> > +++ b/drivers/media/platform/davinci/vpif_capture.c
> > @@ -1542,7 +1542,7 @@ 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;
> > + struct device_node *rem, *rem_ep;
> > unsigned int flags;
> > int err;
> >
> > @@ -1551,13 +1551,22 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> > if (!endpoint)
> > break;
> >
> > - rem = of_graph_get_remote_port_parent(endpoint);
> > - if (!rem) {
> > - dev_dbg(&pdev->dev, "Remote device at %pOF not found\n",
> > + rem_ep = of_graph_get_remote_endpoint(endpoint);
> > + if (!rem_ep) {
> > + dev_dbg(&pdev->dev, "Remote for endpoint %pOF not found\n",
> > endpoint);
> > goto done;
> > }
> >
> > + rem = of_graph_get_port_parent(rem_ep);
> > + if (!rem) {
> > + dev_dbg(&pdev->dev, "Remote endpoint at %pOF not found\n",
> > + rem_ep);
> > + of_node_put(rem_ep);
> > + goto done;
> > + }
> > + of_node_put(rem_ep);
> > +
> > sdinfo = &pdata->subdev_info[i];
> > chan = &pdata->chan_config[i];
> > chan->inputs = devm_kcalloc(&pdev->dev,
> > @@ -1597,12 +1606,13 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> > sdinfo->name = rem->full_name;
> >
> > pdata->asd[i] = v4l2_async_notifier_add_fwnode_subdev(
> > - &vpif_obj.notifier, of_fwnode_handle(rem),
> > + &vpif_obj.notifier, of_fwnode_handle(rem_ep),
> > sizeof(struct v4l2_async_subdev));
> > if (IS_ERR(pdata->asd[i])) {
> > of_node_put(rem);
> > goto err_cleanup;
> > }
> > +
> > }
> >
> > done:
> > diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> > index d1d5041cdae5..0cbc2076b94d 100644
> > --- a/drivers/media/platform/exynos4-is/media-dev.c
> > +++ b/drivers/media/platform/exynos4-is/media-dev.c
> > @@ -411,7 +411,7 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
> >
> > pd->mux_id = (endpoint.base.port - 1) & 0x1;
> >
> > - rem = of_graph_get_remote_port_parent(ep);
> > + rem = of_graph_get_remote_endpoint(ep);
> > of_node_put(ep);
> > if (rem == NULL) {
> > v4l2_info(&fmd->v4l2_dev, "Remote device at %pOF not found\n",
> > @@ -1376,11 +1376,17 @@ static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
> > int i;
> >
> > /* Find platform data for this sensor subdev */
> > - for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++)
> > - if (fmd->sensor[i].asd.match.fwnode ==
> > - of_fwnode_handle(subdev->dev->of_node))
> > + for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++) {
> > + struct fwnode_handle *fwnode =
> > + fwnode_graph_get_remote_port_parent(fmd->sensor[i].asd.
> > + match.fwnode);
> > +
> > + if (fwnode == of_fwnode_handle(subdev->dev->of_node))
> > si = &fmd->sensor[i];
> >
> > + fwnode_handle_put(fwnode);
> > + }
> > +
> > if (si == NULL)
> > return -EINVAL;
> >
> > diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
> > index a7b2b89d6155..0aeba52aee13 100644
> > --- a/drivers/media/platform/pxa_camera.c
> > +++ b/drivers/media/platform/pxa_camera.c
> > @@ -2350,7 +2350,7 @@ static int pxa_camera_pdata_from_dt(struct device *dev,
> > pcdev->platform_flags |= PXA_CAMERA_PCLK_EN;
> >
> > asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> > - remote = of_graph_get_remote_port_parent(np);
> > + remote = of_graph_get_remote_endpoint(np);
> > if (remote)
> > asd->match.fwnode = of_fwnode_handle(remote);
> > else
> > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> > index 63da18773d24..a979f210f441 100644
> > --- a/drivers/media/platform/qcom/camss/camss.c
> > +++ b/drivers/media/platform/qcom/camss/camss.c
> > @@ -466,7 +466,7 @@ static int camss_of_parse_ports(struct camss *camss)
> > {
> > struct device *dev = camss->dev;
> > struct device_node *node = NULL;
> > - struct device_node *remote = NULL;
> > + struct fwnode_handle *remote = NULL;
> > int ret, num_subdevs = 0;
> >
> > for_each_endpoint_of_node(dev->of_node, node) {
> > @@ -476,7 +476,8 @@ static int camss_of_parse_ports(struct camss *camss)
> > if (!of_device_is_available(node))
> > continue;
> >
> > - remote = of_graph_get_remote_port_parent(node);
> > + remote = fwnode_graph_get_remote_endpoint(
> > + of_fwnode_handle(node));
> > if (!remote) {
> > dev_err(dev, "Cannot get remote parent\n");
> > ret = -EINVAL;
> > @@ -484,11 +485,10 @@ static int camss_of_parse_ports(struct camss *camss)
> > }
> >
> > asd = v4l2_async_notifier_add_fwnode_subdev(
> > - &camss->notifier, of_fwnode_handle(remote),
> > - sizeof(*csd));
> > + &camss->notifier, remote, sizeof(*csd));
> > if (IS_ERR(asd)) {
> > ret = PTR_ERR(asd);
> > - of_node_put(remote);
> > + fwnode_handle_put(remote);
> > goto err_cleanup;
> > }
> >
> > diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
> > index 608e5217ccd5..258e14c290e8 100644
> > --- a/drivers/media/platform/rcar_drif.c
> > +++ b/drivers/media/platform/rcar_drif.c
> > @@ -1222,7 +1222,7 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr)
> > if (!ep)
> > return 0;
> >
> > - fwnode = fwnode_graph_get_remote_port_parent(ep);
> > + fwnode = fwnode_graph_get_remote_endpoint(ep);
> > if (!fwnode) {
> > dev_warn(sdr->dev, "bad remote port parent\n");
> > fwnode_handle_put(ep);
> > diff --git a/drivers/media/platform/renesas-ceu.c b/drivers/media/platform/renesas-ceu.c
> > index 57d0c0f9fa4b..1e625d560258 100644
> > --- a/drivers/media/platform/renesas-ceu.c
> > +++ b/drivers/media/platform/renesas-ceu.c
> > @@ -1581,7 +1581,7 @@ static int ceu_parse_dt(struct ceu_device *ceudev)
> > ceu_sd = &ceudev->subdevs[i];
> > INIT_LIST_HEAD(&ceu_sd->asd.list);
> >
> > - remote = of_graph_get_remote_port_parent(ep);
> > + remote = of_graph_get_remote_endpoint(ep);
> > ceu_sd->mbus_flags = fw_ep.bus.parallel.flags;
> > ceu_sd->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > ceu_sd->asd.match.fwnode = of_fwnode_handle(remote);
> > diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> > index d855e9c09c08..3c91fe84e0d5 100644
> > --- a/drivers/media/platform/stm32/stm32-dcmi.c
> > +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> > @@ -1602,7 +1602,7 @@ static int dcmi_graph_parse(struct stm32_dcmi *dcmi, struct device_node *node)
> > if (!ep)
> > return -EINVAL;
> >
> > - remote = of_graph_get_remote_port_parent(ep);
> > + remote = of_graph_get_remote_endpoint(ep);
> > of_node_put(ep);
> > if (!remote)
> > return -EINVAL;
> > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> > index 8d075683e448..d7995e2f4c54 100644
> > --- a/drivers/media/platform/ti-vpe/cal.c
> > +++ b/drivers/media/platform/ti-vpe/cal.c
> > @@ -1693,7 +1693,7 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst)
> > goto cleanup_exit;
> > }
> >
> > - sensor_node = of_graph_get_remote_port_parent(ep_node);
> > + sensor_node = of_graph_get_remote_endpoint(ep_node);
> > if (!sensor_node) {
> > ctx_dbg(3, ctx, "can't get remote parent\n");
> > goto cleanup_exit;
> > diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
> > index edce0402155d..41df417153bd 100644
> > --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> > +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> > @@ -14,6 +14,7 @@
> > #include <linux/of.h>
> > #include <linux/of_graph.h>
> > #include <linux/platform_device.h>
> > +#include <linux/property.h>
> > #include <linux/slab.h>
> >
> > #include <media/v4l2-async.h>
> > @@ -81,6 +82,8 @@ static int xvip_graph_build_one(struct xvip_composite_device *xdev,
> > dev_dbg(xdev->dev, "creating links for entity %s\n", local->name);
> >
> > while (1) {
> > + struct fwnode_handle *fwnode;
> > +
> > /* Get the next endpoint and parse its link. */
> > ep = fwnode_graph_get_next_endpoint(entity->asd.match.fwnode,
> > ep);
> > @@ -116,11 +119,13 @@ static int xvip_graph_build_one(struct xvip_composite_device *xdev,
> > continue;
> > }
> >
> > + fwnode = fwnode_graph_get_port_parent(link.remote_node);
> > + fwnode_handle_put(fwnode);
> > +
> > /* Skip DMA engines, they will be processed separately. */
> > - if (link.remote_node == of_fwnode_handle(xdev->dev->of_node)) {
> > + if (fwnode == dev_fwnode(xdev->dev)) {
> > dev_dbg(xdev->dev, "skipping DMA port %p:%u\n",
> > link.local_node, link.local_port);
> > - v4l2_fwnode_put_link(&link);
> > continue;
> > }
> >
> > @@ -374,9 +379,11 @@ static int xvip_graph_parse_one(struct xvip_composite_device *xdev,
> > }
> >
> > fwnode_handle_put(ep);
> > + fwnode = fwnode_graph_get_port_parent(remote);
> > + fwnode_handle_put(fwnode);
> >
> > /* Skip entities that we have already processed. */
> > - if (remote == of_fwnode_handle(xdev->dev->of_node) ||
> > + if (fwnode == dev_fwnode(xdev->dev) ||
> > xvip_graph_find_entity(xdev, remote)) {
> > fwnode_handle_put(remote);
> > continue;
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 15b0c44a76e7..304969ff3191 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -670,8 +670,13 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> > * (struct v4l2_subdev.dev), and async sub-device does not
> > * exist independently of the device at any point of time.
> > */
> > - if (!sd->fwnode && sd->dev)
> > - sd->fwnode = dev_fwnode(sd->dev);
> > + if (!sd->fwnode && sd->dev) {
> > + sd->fwnode = fwnode_graph_get_next_endpoint(
> > + dev_fwnode(sd->dev), NULL);
> > + fwnode_handle_put(sd->fwnode);
> > + if (!sd->fwnode)
> > + sd->fwnode = dev_fwnode(sd->dev);
> > + }
> >
> > mutex_lock(&list_lock);
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index dea8917fd912..810b6584b522 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -617,7 +617,7 @@ v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
> >
> > asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> > asd->match.fwnode =
> > - fwnode_graph_get_remote_port_parent(endpoint);
> > + fwnode_graph_get_remote_endpoint(endpoint);
> > if (!asd->match.fwnode) {
> > dev_dbg(dev, "no remote endpoint found\n");
> > ret = -ENOTCONN;
> > @@ -1051,7 +1051,6 @@ v4l2_fwnode_reference_parse_int_props(struct device *dev,
> > {
> > struct fwnode_handle *fwnode;
> > unsigned int index;
> > - int ret;
> > const char *prop = p->name;
> > const char * const *props = p->props;
> > unsigned int nprops = p->nprops;
> > @@ -1087,9 +1086,8 @@ v4l2_fwnode_reference_parse_int_props(struct device *dev,
> > asd = v4l2_async_notifier_add_fwnode_subdev(notifier, fwnode,
> > sizeof(*asd));
> > if (IS_ERR(asd)) {
> > - ret = PTR_ERR(asd);
> > /* not an error if asd already exists */
> > - if (ret == -EEXIST) {
> > + if (PTR_ERR(asd) == -EEXIST) {
> > fwnode_handle_put(fwnode);
> > continue;
> > }
> > @@ -1102,7 +1100,7 @@ v4l2_fwnode_reference_parse_int_props(struct device *dev,
> >
> > error:
> > fwnode_handle_put(fwnode);
> > - return ret;
> > + return PTR_ERR(fwnode);
> > }
> >
> > int v4l2_async_notifier_parse_fwnode_sensor_common(struct device *dev,
> > diff --git a/drivers/staging/media/soc_camera/soc_camera.c b/drivers/staging/media/soc_camera/soc_camera.c
> > index a6232dcd59bc..f0768b469fc2 100644
> > --- a/drivers/staging/media/soc_camera/soc_camera.c
> > +++ b/drivers/staging/media/soc_camera/soc_camera.c
> > @@ -1514,6 +1514,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
> > struct soc_camera_async_client *sasc;
> > struct soc_of_info *info;
> > struct i2c_client *client;
> > + struct device_node *np;
> > char clk_name[V4L2_CLK_NAME_SIZE];
> > int ret;
> >
> > @@ -1548,23 +1549,23 @@ static int soc_of_bind(struct soc_camera_host *ici,
> > v4l2_async_notifier_init(&sasc->notifier);
> >
> > ret = v4l2_async_notifier_add_subdev(&sasc->notifier, info->subdev);
> > - if (ret) {
> > - of_node_put(remote);
> > + if (ret)
> > goto eaddasd;
> > - }
> >
> > sasc->notifier.ops = &soc_camera_async_ops;
> >
> > icd->sasc = sasc;
> > icd->parent = ici->v4l2_dev.dev;
> > + np = of_graph_get_port_parent(remote);
> > + of_node_put(remote);
> >
> > - client = of_find_i2c_device_by_node(remote);
> > + client = of_find_i2c_device_by_node(np);
> >
> > if (client)
> > v4l2_clk_name_i2c(clk_name, sizeof(clk_name),
> > client->adapter->nr, client->addr);
> > else
> > - v4l2_clk_name_of(clk_name, sizeof(clk_name), remote);
> > + v4l2_clk_name_of(clk_name, sizeof(clk_name), np);
> >
> > icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
> > if (IS_ERR(icd->clk)) {
> > @@ -1587,6 +1588,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
> > eallocpdev:
> > devm_kfree(ici->v4l2_dev.dev, info);
> > dev_err(ici->v4l2_dev.dev, "group probe failed: %d\n", ret);
> > + of_node_put(np);
> >
> > return ret;
> > }
> > @@ -1603,7 +1605,7 @@ static void scan_of_host(struct soc_camera_host *ici)
> > if (!epn)
> > break;
> >
> > - rem = of_graph_get_remote_port_parent(epn);
> > + rem = of_graph_get_remote_endpoint(epn);
> > if (!rem) {
> > dev_notice(dev, "no remote for %pOF\n", epn);
> > continue;
> > --
> > 2.11.0
> >
>
> --
> Regards,
> Niklas Söderlund
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/9] v4l2-async: Use endpoint node, not device node, for fwnode match
2019-06-21 9:19 ` Jacopo Mondi
@ 2019-06-21 11:43 ` Niklas Söderlund
0 siblings, 0 replies; 21+ messages in thread
From: Niklas Söderlund @ 2019-06-21 11:43 UTC (permalink / raw)
To: Jacopo Mondi; +Cc: Sakari Ailus, linux-media, hverkuil, laurent.pinchart
Hi Jacopo,
On 2019-06-21 11:19:27 +0200, Jacopo Mondi wrote:
> Hi Niklas,
>
> On Fri, Jun 14, 2019 at 11:21:05PM +0200, Niklas Söderlund wrote:
> > Hi Sakari,
> >
> > Thanks for your work and sorry that I missed this and replied to v1 the
> > other day. I have tested v2 now and unfortunately it still breaks
> > rcar-vin, see bellow :-(
> >
> > On 2019-06-06 16:02:18 +0300, Sakari Ailus wrote:
> > > V4L2 async framework can use both device's fwnode and endpoints's fwnode
> > > for matching the async sub-device with the sub-device. In order to proceed
> > > moving towards endpoint matching assign the endpoint to the async
> > > sub-device.
> > >
> > > As most async sub-device drivers (and the related hardware) only supports
> > > a single endpoint, use the first endpoint found. This works for all
> > > current drivers --- we only ever supported a single async sub-device per
> > > device to begin with.
> >
> > Unfortunately this breaks for rcar-vin and adv7604 on Koelsch :-(
> >
> > In DT we have the node:
> >
> > i2chdmi: i2c-13 {
> > hdmi-in@4c {
> > compatible = "adi,adv7612";
> >
> > ports {
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > port@0 {
> > reg = <0>;
> > adv7612_in: endpoint {
> > remote-endpoint = <&hdmi_con_in>;
> > };
> > };
> >
> > port@2 {
> > reg = <2>;
> > adv7612_out: endpoint {
> > remote-endpoint = <&vin0ep2>;
> > };
> > };
> > };
> > };
> > };
> >
> > - The rcar-vin in rvin_parallel_init() parses the DT using the
> > v4l2_async_notifier_parse_fwnode_endpoints_by_port() helper and
> > registers '/i2c-13/hdmi-in@4c/ports/port@2/endpoint' with the async
> > framework.
> >
> > - The adv7604 register itself with async using
> > '/i2c-13/hdmi-in@4c/ports/port@0/endpoint'.
> >
> > The result is that matching breaks and the two devices never find each
> > other. I'm not sure how to solve this :-( Maybe the subdevices could
> > register itself multiple times with the async framework, once for each
> > port described?
>
> Isn't this an issue on adv7604 side? With Sakari's patches the fwnode
> of the first available endpoint is used for matching, and in case of
> adv7604 is the one that connects to the HDMI connector.
>
> Shouldn't the adv7604 driver register its async subdev by using the
> fwnode handle of its only source port instead?
It should ;-)
What I tried to express above was that I think one direction to this
would be solved in a nice way would be to have all subdevices register
themself using all endpoints described AND allow being bound multiple
multiple times, once for each endpoint. This would however require some
work, as the subdevices of today can only be bound once.
In the short term changing the adv7604 to register using the endpoint
which 'represents' its source pad would solve this. But what about
adv7482 which have more then one endpoint describing a source? Today
this is hacked together with custom matching logic in rcar-csi2 which is
not super nice.
>
> That said, I wonder how many of these similar situation would go
> unoticed without someone doing the testing, as this is a run-time
> failure hard to detect at development time.
This is also a good point.
>
> >
> > That would open up for the same subdev to be bound multiple times,
> > possibly to different consumers. I think that is a direction that would
> > be useful but I fear there might be some work involved in allowing that
> > from a subdev point of view.
> >
> > >
> > > For async devices that have no endpoints, continue to use the fwnode
> > > related to the device. This includes e.g. lens devices.
> > >
> > > Depends-on: ("pxa-camera: Match with device node, not the port node")
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > > drivers/media/platform/am437x/am437x-vpfe.c | 2 +-
> > > drivers/media/platform/atmel/atmel-isc.c | 2 +-
> > > drivers/media/platform/atmel/atmel-isi.c | 2 +-
> > > drivers/media/platform/cadence/cdns-csi2rx.c | 2 +-
> > > drivers/media/platform/davinci/vpif_capture.c | 20 +++++++++++++++-----
> > > drivers/media/platform/exynos4-is/media-dev.c | 14 ++++++++++----
> > > drivers/media/platform/pxa_camera.c | 2 +-
> > > drivers/media/platform/qcom/camss/camss.c | 10 +++++-----
> > > drivers/media/platform/rcar_drif.c | 2 +-
> > > drivers/media/platform/renesas-ceu.c | 2 +-
> > > drivers/media/platform/stm32/stm32-dcmi.c | 2 +-
> > > drivers/media/platform/ti-vpe/cal.c | 2 +-
> > > drivers/media/platform/xilinx/xilinx-vipp.c | 13 ++++++++++---
> > > drivers/media/v4l2-core/v4l2-async.c | 9 +++++++--
> > > drivers/media/v4l2-core/v4l2-fwnode.c | 8 +++-----
> > > drivers/staging/media/soc_camera/soc_camera.c | 14 ++++++++------
> > > 16 files changed, 67 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> > > index fe7b937eb5f2..db263c0ce48e 100644
> > > --- a/drivers/media/platform/am437x/am437x-vpfe.c
> > > +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> > > @@ -2495,7 +2495,7 @@ vpfe_get_pdata(struct vpfe_device *vpfe)
> > > if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> > > sdinfo->vpfe_param.vdpol = 1;
> > >
> > > - rem = of_graph_get_remote_port_parent(endpoint);
> > > + rem = of_graph_get_remote_endpoint(endpoint);
> > > if (!rem) {
> > > dev_err(dev, "Remote device at %pOF not found\n",
> > > endpoint);
> > > diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
> > > index da3b441e7961..fad10e6d5ecf 100644
> > > --- a/drivers/media/platform/atmel/atmel-isc.c
> > > +++ b/drivers/media/platform/atmel/atmel-isc.c
> > > @@ -2352,7 +2352,7 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
> > > if (!epn)
> > > return 0;
> > >
> > > - rem = of_graph_get_remote_port_parent(epn);
> > > + rem = of_graph_get_remote_endpoint(epn);
> > > if (!rem) {
> > > dev_notice(dev, "Remote device at %pOF not found\n",
> > > epn);
> > > diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
> > > index 08b8d5583080..e4e74454e016 100644
> > > --- a/drivers/media/platform/atmel/atmel-isi.c
> > > +++ b/drivers/media/platform/atmel/atmel-isi.c
> > > @@ -1110,7 +1110,7 @@ static int isi_graph_parse(struct atmel_isi *isi, struct device_node *node)
> > > if (!ep)
> > > return -EINVAL;
> > >
> > > - remote = of_graph_get_remote_port_parent(ep);
> > > + remote = of_graph_get_remote_endpoint(ep);
> > > of_node_put(ep);
> > > if (!remote)
> > > return -EINVAL;
> > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> > > index 31ace114eda1..2da34b93e8f4 100644
> > > --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> > > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> > > @@ -395,7 +395,7 @@ static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
> > > return -EINVAL;
> > > }
> > >
> > > - csi2rx->asd.match.fwnode = fwnode_graph_get_remote_port_parent(fwh);
> > > + csi2rx->asd.match.fwnode = fwnode_graph_get_remote_endpoint(fwh);
> > > csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > > of_node_put(ep);
> > >
> > > diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> > > index 72bdb3c10962..8fdea45ae090 100644
> > > --- a/drivers/media/platform/davinci/vpif_capture.c
> > > +++ b/drivers/media/platform/davinci/vpif_capture.c
> > > @@ -1542,7 +1542,7 @@ 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;
> > > + struct device_node *rem, *rem_ep;
> > > unsigned int flags;
> > > int err;
> > >
> > > @@ -1551,13 +1551,22 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> > > if (!endpoint)
> > > break;
> > >
> > > - rem = of_graph_get_remote_port_parent(endpoint);
> > > - if (!rem) {
> > > - dev_dbg(&pdev->dev, "Remote device at %pOF not found\n",
> > > + rem_ep = of_graph_get_remote_endpoint(endpoint);
> > > + if (!rem_ep) {
> > > + dev_dbg(&pdev->dev, "Remote for endpoint %pOF not found\n",
> > > endpoint);
> > > goto done;
> > > }
> > >
> > > + rem = of_graph_get_port_parent(rem_ep);
> > > + if (!rem) {
> > > + dev_dbg(&pdev->dev, "Remote endpoint at %pOF not found\n",
> > > + rem_ep);
> > > + of_node_put(rem_ep);
> > > + goto done;
> > > + }
> > > + of_node_put(rem_ep);
> > > +
> > > sdinfo = &pdata->subdev_info[i];
> > > chan = &pdata->chan_config[i];
> > > chan->inputs = devm_kcalloc(&pdev->dev,
> > > @@ -1597,12 +1606,13 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> > > sdinfo->name = rem->full_name;
> > >
> > > pdata->asd[i] = v4l2_async_notifier_add_fwnode_subdev(
> > > - &vpif_obj.notifier, of_fwnode_handle(rem),
> > > + &vpif_obj.notifier, of_fwnode_handle(rem_ep),
> > > sizeof(struct v4l2_async_subdev));
> > > if (IS_ERR(pdata->asd[i])) {
> > > of_node_put(rem);
> > > goto err_cleanup;
> > > }
> > > +
> > > }
> > >
> > > done:
> > > diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> > > index d1d5041cdae5..0cbc2076b94d 100644
> > > --- a/drivers/media/platform/exynos4-is/media-dev.c
> > > +++ b/drivers/media/platform/exynos4-is/media-dev.c
> > > @@ -411,7 +411,7 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
> > >
> > > pd->mux_id = (endpoint.base.port - 1) & 0x1;
> > >
> > > - rem = of_graph_get_remote_port_parent(ep);
> > > + rem = of_graph_get_remote_endpoint(ep);
> > > of_node_put(ep);
> > > if (rem == NULL) {
> > > v4l2_info(&fmd->v4l2_dev, "Remote device at %pOF not found\n",
> > > @@ -1376,11 +1376,17 @@ static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
> > > int i;
> > >
> > > /* Find platform data for this sensor subdev */
> > > - for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++)
> > > - if (fmd->sensor[i].asd.match.fwnode ==
> > > - of_fwnode_handle(subdev->dev->of_node))
> > > + for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++) {
> > > + struct fwnode_handle *fwnode =
> > > + fwnode_graph_get_remote_port_parent(fmd->sensor[i].asd.
> > > + match.fwnode);
> > > +
> > > + if (fwnode == of_fwnode_handle(subdev->dev->of_node))
> > > si = &fmd->sensor[i];
> > >
> > > + fwnode_handle_put(fwnode);
> > > + }
> > > +
> > > if (si == NULL)
> > > return -EINVAL;
> > >
> > > diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
> > > index a7b2b89d6155..0aeba52aee13 100644
> > > --- a/drivers/media/platform/pxa_camera.c
> > > +++ b/drivers/media/platform/pxa_camera.c
> > > @@ -2350,7 +2350,7 @@ static int pxa_camera_pdata_from_dt(struct device *dev,
> > > pcdev->platform_flags |= PXA_CAMERA_PCLK_EN;
> > >
> > > asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> > > - remote = of_graph_get_remote_port_parent(np);
> > > + remote = of_graph_get_remote_endpoint(np);
> > > if (remote)
> > > asd->match.fwnode = of_fwnode_handle(remote);
> > > else
> > > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> > > index 63da18773d24..a979f210f441 100644
> > > --- a/drivers/media/platform/qcom/camss/camss.c
> > > +++ b/drivers/media/platform/qcom/camss/camss.c
> > > @@ -466,7 +466,7 @@ static int camss_of_parse_ports(struct camss *camss)
> > > {
> > > struct device *dev = camss->dev;
> > > struct device_node *node = NULL;
> > > - struct device_node *remote = NULL;
> > > + struct fwnode_handle *remote = NULL;
> > > int ret, num_subdevs = 0;
> > >
> > > for_each_endpoint_of_node(dev->of_node, node) {
> > > @@ -476,7 +476,8 @@ static int camss_of_parse_ports(struct camss *camss)
> > > if (!of_device_is_available(node))
> > > continue;
> > >
> > > - remote = of_graph_get_remote_port_parent(node);
> > > + remote = fwnode_graph_get_remote_endpoint(
> > > + of_fwnode_handle(node));
> > > if (!remote) {
> > > dev_err(dev, "Cannot get remote parent\n");
> > > ret = -EINVAL;
> > > @@ -484,11 +485,10 @@ static int camss_of_parse_ports(struct camss *camss)
> > > }
> > >
> > > asd = v4l2_async_notifier_add_fwnode_subdev(
> > > - &camss->notifier, of_fwnode_handle(remote),
> > > - sizeof(*csd));
> > > + &camss->notifier, remote, sizeof(*csd));
> > > if (IS_ERR(asd)) {
> > > ret = PTR_ERR(asd);
> > > - of_node_put(remote);
> > > + fwnode_handle_put(remote);
> > > goto err_cleanup;
> > > }
> > >
> > > diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
> > > index 608e5217ccd5..258e14c290e8 100644
> > > --- a/drivers/media/platform/rcar_drif.c
> > > +++ b/drivers/media/platform/rcar_drif.c
> > > @@ -1222,7 +1222,7 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr)
> > > if (!ep)
> > > return 0;
> > >
> > > - fwnode = fwnode_graph_get_remote_port_parent(ep);
> > > + fwnode = fwnode_graph_get_remote_endpoint(ep);
> > > if (!fwnode) {
> > > dev_warn(sdr->dev, "bad remote port parent\n");
> > > fwnode_handle_put(ep);
> > > diff --git a/drivers/media/platform/renesas-ceu.c b/drivers/media/platform/renesas-ceu.c
> > > index 57d0c0f9fa4b..1e625d560258 100644
> > > --- a/drivers/media/platform/renesas-ceu.c
> > > +++ b/drivers/media/platform/renesas-ceu.c
> > > @@ -1581,7 +1581,7 @@ static int ceu_parse_dt(struct ceu_device *ceudev)
> > > ceu_sd = &ceudev->subdevs[i];
> > > INIT_LIST_HEAD(&ceu_sd->asd.list);
> > >
> > > - remote = of_graph_get_remote_port_parent(ep);
> > > + remote = of_graph_get_remote_endpoint(ep);
> > > ceu_sd->mbus_flags = fw_ep.bus.parallel.flags;
> > > ceu_sd->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > > ceu_sd->asd.match.fwnode = of_fwnode_handle(remote);
> > > diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> > > index d855e9c09c08..3c91fe84e0d5 100644
> > > --- a/drivers/media/platform/stm32/stm32-dcmi.c
> > > +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> > > @@ -1602,7 +1602,7 @@ static int dcmi_graph_parse(struct stm32_dcmi *dcmi, struct device_node *node)
> > > if (!ep)
> > > return -EINVAL;
> > >
> > > - remote = of_graph_get_remote_port_parent(ep);
> > > + remote = of_graph_get_remote_endpoint(ep);
> > > of_node_put(ep);
> > > if (!remote)
> > > return -EINVAL;
> > > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> > > index 8d075683e448..d7995e2f4c54 100644
> > > --- a/drivers/media/platform/ti-vpe/cal.c
> > > +++ b/drivers/media/platform/ti-vpe/cal.c
> > > @@ -1693,7 +1693,7 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst)
> > > goto cleanup_exit;
> > > }
> > >
> > > - sensor_node = of_graph_get_remote_port_parent(ep_node);
> > > + sensor_node = of_graph_get_remote_endpoint(ep_node);
> > > if (!sensor_node) {
> > > ctx_dbg(3, ctx, "can't get remote parent\n");
> > > goto cleanup_exit;
> > > diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
> > > index edce0402155d..41df417153bd 100644
> > > --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> > > +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> > > @@ -14,6 +14,7 @@
> > > #include <linux/of.h>
> > > #include <linux/of_graph.h>
> > > #include <linux/platform_device.h>
> > > +#include <linux/property.h>
> > > #include <linux/slab.h>
> > >
> > > #include <media/v4l2-async.h>
> > > @@ -81,6 +82,8 @@ static int xvip_graph_build_one(struct xvip_composite_device *xdev,
> > > dev_dbg(xdev->dev, "creating links for entity %s\n", local->name);
> > >
> > > while (1) {
> > > + struct fwnode_handle *fwnode;
> > > +
> > > /* Get the next endpoint and parse its link. */
> > > ep = fwnode_graph_get_next_endpoint(entity->asd.match.fwnode,
> > > ep);
> > > @@ -116,11 +119,13 @@ static int xvip_graph_build_one(struct xvip_composite_device *xdev,
> > > continue;
> > > }
> > >
> > > + fwnode = fwnode_graph_get_port_parent(link.remote_node);
> > > + fwnode_handle_put(fwnode);
> > > +
> > > /* Skip DMA engines, they will be processed separately. */
> > > - if (link.remote_node == of_fwnode_handle(xdev->dev->of_node)) {
> > > + if (fwnode == dev_fwnode(xdev->dev)) {
> > > dev_dbg(xdev->dev, "skipping DMA port %p:%u\n",
> > > link.local_node, link.local_port);
> > > - v4l2_fwnode_put_link(&link);
> > > continue;
> > > }
> > >
> > > @@ -374,9 +379,11 @@ static int xvip_graph_parse_one(struct xvip_composite_device *xdev,
> > > }
> > >
> > > fwnode_handle_put(ep);
> > > + fwnode = fwnode_graph_get_port_parent(remote);
> > > + fwnode_handle_put(fwnode);
> > >
> > > /* Skip entities that we have already processed. */
> > > - if (remote == of_fwnode_handle(xdev->dev->of_node) ||
> > > + if (fwnode == dev_fwnode(xdev->dev) ||
> > > xvip_graph_find_entity(xdev, remote)) {
> > > fwnode_handle_put(remote);
> > > continue;
> > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > > index 15b0c44a76e7..304969ff3191 100644
> > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > @@ -670,8 +670,13 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> > > * (struct v4l2_subdev.dev), and async sub-device does not
> > > * exist independently of the device at any point of time.
> > > */
> > > - if (!sd->fwnode && sd->dev)
> > > - sd->fwnode = dev_fwnode(sd->dev);
> > > + if (!sd->fwnode && sd->dev) {
> > > + sd->fwnode = fwnode_graph_get_next_endpoint(
> > > + dev_fwnode(sd->dev), NULL);
> > > + fwnode_handle_put(sd->fwnode);
> > > + if (!sd->fwnode)
> > > + sd->fwnode = dev_fwnode(sd->dev);
> > > + }
> > >
> > > mutex_lock(&list_lock);
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > index dea8917fd912..810b6584b522 100644
> > > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > @@ -617,7 +617,7 @@ v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
> > >
> > > asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> > > asd->match.fwnode =
> > > - fwnode_graph_get_remote_port_parent(endpoint);
> > > + fwnode_graph_get_remote_endpoint(endpoint);
> > > if (!asd->match.fwnode) {
> > > dev_dbg(dev, "no remote endpoint found\n");
> > > ret = -ENOTCONN;
> > > @@ -1051,7 +1051,6 @@ v4l2_fwnode_reference_parse_int_props(struct device *dev,
> > > {
> > > struct fwnode_handle *fwnode;
> > > unsigned int index;
> > > - int ret;
> > > const char *prop = p->name;
> > > const char * const *props = p->props;
> > > unsigned int nprops = p->nprops;
> > > @@ -1087,9 +1086,8 @@ v4l2_fwnode_reference_parse_int_props(struct device *dev,
> > > asd = v4l2_async_notifier_add_fwnode_subdev(notifier, fwnode,
> > > sizeof(*asd));
> > > if (IS_ERR(asd)) {
> > > - ret = PTR_ERR(asd);
> > > /* not an error if asd already exists */
> > > - if (ret == -EEXIST) {
> > > + if (PTR_ERR(asd) == -EEXIST) {
> > > fwnode_handle_put(fwnode);
> > > continue;
> > > }
> > > @@ -1102,7 +1100,7 @@ v4l2_fwnode_reference_parse_int_props(struct device *dev,
> > >
> > > error:
> > > fwnode_handle_put(fwnode);
> > > - return ret;
> > > + return PTR_ERR(fwnode);
> > > }
> > >
> > > int v4l2_async_notifier_parse_fwnode_sensor_common(struct device *dev,
> > > diff --git a/drivers/staging/media/soc_camera/soc_camera.c b/drivers/staging/media/soc_camera/soc_camera.c
> > > index a6232dcd59bc..f0768b469fc2 100644
> > > --- a/drivers/staging/media/soc_camera/soc_camera.c
> > > +++ b/drivers/staging/media/soc_camera/soc_camera.c
> > > @@ -1514,6 +1514,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
> > > struct soc_camera_async_client *sasc;
> > > struct soc_of_info *info;
> > > struct i2c_client *client;
> > > + struct device_node *np;
> > > char clk_name[V4L2_CLK_NAME_SIZE];
> > > int ret;
> > >
> > > @@ -1548,23 +1549,23 @@ static int soc_of_bind(struct soc_camera_host *ici,
> > > v4l2_async_notifier_init(&sasc->notifier);
> > >
> > > ret = v4l2_async_notifier_add_subdev(&sasc->notifier, info->subdev);
> > > - if (ret) {
> > > - of_node_put(remote);
> > > + if (ret)
> > > goto eaddasd;
> > > - }
> > >
> > > sasc->notifier.ops = &soc_camera_async_ops;
> > >
> > > icd->sasc = sasc;
> > > icd->parent = ici->v4l2_dev.dev;
> > > + np = of_graph_get_port_parent(remote);
> > > + of_node_put(remote);
> > >
> > > - client = of_find_i2c_device_by_node(remote);
> > > + client = of_find_i2c_device_by_node(np);
> > >
> > > if (client)
> > > v4l2_clk_name_i2c(clk_name, sizeof(clk_name),
> > > client->adapter->nr, client->addr);
> > > else
> > > - v4l2_clk_name_of(clk_name, sizeof(clk_name), remote);
> > > + v4l2_clk_name_of(clk_name, sizeof(clk_name), np);
> > >
> > > icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
> > > if (IS_ERR(icd->clk)) {
> > > @@ -1587,6 +1588,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
> > > eallocpdev:
> > > devm_kfree(ici->v4l2_dev.dev, info);
> > > dev_err(ici->v4l2_dev.dev, "group probe failed: %d\n", ret);
> > > + of_node_put(np);
> > >
> > > return ret;
> > > }
> > > @@ -1603,7 +1605,7 @@ static void scan_of_host(struct soc_camera_host *ici)
> > > if (!epn)
> > > break;
> > >
> > > - rem = of_graph_get_remote_port_parent(epn);
> > > + rem = of_graph_get_remote_endpoint(epn);
> > > if (!rem) {
> > > dev_notice(dev, "no remote for %pOF\n", epn);
> > > continue;
> > > --
> > > 2.11.0
> > >
> >
> > --
> > Regards,
> > Niklas Söderlund
--
Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/9] v4l2-async: Use endpoint node, not device node, for fwnode match
2019-06-14 21:21 ` Niklas Söderlund
2019-06-21 9:19 ` Jacopo Mondi
@ 2019-06-28 8:29 ` Sakari Ailus
1 sibling, 0 replies; 21+ messages in thread
From: Sakari Ailus @ 2019-06-28 8:29 UTC (permalink / raw)
To: Niklas Söderlund; +Cc: linux-media, hverkuil, laurent.pinchart, jacopo
Hi Niklas,
On Fri, Jun 14, 2019 at 11:21:05PM +0200, Niklas Söderlund wrote:
> Hi Sakari,
>
> Thanks for your work and sorry that I missed this and replied to v1 the
> other day. I have tested v2 now and unfortunately it still breaks
> rcar-vin, see bellow :-(
Thank you for testing the set.
>
> On 2019-06-06 16:02:18 +0300, Sakari Ailus wrote:
> > V4L2 async framework can use both device's fwnode and endpoints's fwnode
> > for matching the async sub-device with the sub-device. In order to proceed
> > moving towards endpoint matching assign the endpoint to the async
> > sub-device.
> >
> > As most async sub-device drivers (and the related hardware) only supports
> > a single endpoint, use the first endpoint found. This works for all
> > current drivers --- we only ever supported a single async sub-device per
> > device to begin with.
>
> Unfortunately this breaks for rcar-vin and adv7604 on Koelsch :-(
>
> In DT we have the node:
>
> i2chdmi: i2c-13 {
> hdmi-in@4c {
> compatible = "adi,adv7612";
>
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
>
> port@0 {
> reg = <0>;
> adv7612_in: endpoint {
> remote-endpoint = <&hdmi_con_in>;
> };
> };
>
> port@2 {
> reg = <2>;
> adv7612_out: endpoint {
> remote-endpoint = <&vin0ep2>;
> };
> };
> };
> };
> };
>
> - The rcar-vin in rvin_parallel_init() parses the DT using the
> v4l2_async_notifier_parse_fwnode_endpoints_by_port() helper and
> registers '/i2c-13/hdmi-in@4c/ports/port@2/endpoint' with the async
> framework.
>
> - The adv7604 register itself with async using
> '/i2c-13/hdmi-in@4c/ports/port@0/endpoint'.
>
> The result is that matching breaks and the two devices never find each
> other. I'm not sure how to solve this :-( Maybe the subdevices could
> register itself multiple times with the async framework, once for each
> port described?
That would result in the complete callback not being called. Albeit we
really should get rid of that in the long run: it's bad in many other ways,
too.
This could be addressed by registering the async subdev with the endpoint
in port 2 instead, because the other ports are presumably used for
registering an async notifier. This raises an obvious question that I
didn't really think about when writing the set: how do you know whether you
should register an async sub-device and when not? Perhaps each endpoint
should be registered as a possible connection to an external device
instead. Async sub-devices right now aren't exactly that.
The sub-notifier mainly exists because the main notifier may not be
available at the time it's needed.
I guess we need to transform (or replace) the async sub-devices into (or
with) more generic connection handlers to external endpoints. One would be
registered by the driver for each endpoint, and this needs to be symmetric
independently of whether the driver is currently dealing with a notifier or
just an async sub-device.
This would be important for the purpose of properly supporting devices that
can be used in capture as well as output pipelines; the driver won't know
that anyway so currently it'd have no way of knowing which fwnode should be
registered for the async sub-device.
There might be a way to get rid of the sub-notifiers, too; by deferring
probe if the notifier isn't found when it's needed. We've been perhaps a
tad too fixated to the V4L2 async objects (async sub-devices and notifiers)
that were after all written for quite a bit more restricted use case.
But for now, I'll see if I can rebase the fwnode parsing patches before the
one that moves the matching to take place by endpoints.
>
> That would open up for the same subdev to be bound multiple times,
> possibly to different consumers. I think that is a direction that would
> be useful but I fear there might be some work involved in allowing that
> from a subdev point of view.
I'm sure there would be. :-)
>
> >
> > For async devices that have no endpoints, continue to use the fwnode
> > related to the device. This includes e.g. lens devices.
> >
> > Depends-on: ("pxa-camera: Match with device node, not the port node")
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/platform/am437x/am437x-vpfe.c | 2 +-
> > drivers/media/platform/atmel/atmel-isc.c | 2 +-
> > drivers/media/platform/atmel/atmel-isi.c | 2 +-
> > drivers/media/platform/cadence/cdns-csi2rx.c | 2 +-
> > drivers/media/platform/davinci/vpif_capture.c | 20 +++++++++++++++-----
> > drivers/media/platform/exynos4-is/media-dev.c | 14 ++++++++++----
> > drivers/media/platform/pxa_camera.c | 2 +-
> > drivers/media/platform/qcom/camss/camss.c | 10 +++++-----
> > drivers/media/platform/rcar_drif.c | 2 +-
> > drivers/media/platform/renesas-ceu.c | 2 +-
> > drivers/media/platform/stm32/stm32-dcmi.c | 2 +-
> > drivers/media/platform/ti-vpe/cal.c | 2 +-
> > drivers/media/platform/xilinx/xilinx-vipp.c | 13 ++++++++++---
> > drivers/media/v4l2-core/v4l2-async.c | 9 +++++++--
> > drivers/media/v4l2-core/v4l2-fwnode.c | 8 +++-----
> > drivers/staging/media/soc_camera/soc_camera.c | 14 ++++++++------
> > 16 files changed, 67 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> > index fe7b937eb5f2..db263c0ce48e 100644
> > --- a/drivers/media/platform/am437x/am437x-vpfe.c
> > +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> > @@ -2495,7 +2495,7 @@ vpfe_get_pdata(struct vpfe_device *vpfe)
> > if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> > sdinfo->vpfe_param.vdpol = 1;
> >
> > - rem = of_graph_get_remote_port_parent(endpoint);
> > + rem = of_graph_get_remote_endpoint(endpoint);
> > if (!rem) {
> > dev_err(dev, "Remote device at %pOF not found\n",
> > endpoint);
> > diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
> > index da3b441e7961..fad10e6d5ecf 100644
> > --- a/drivers/media/platform/atmel/atmel-isc.c
> > +++ b/drivers/media/platform/atmel/atmel-isc.c
> > @@ -2352,7 +2352,7 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
> > if (!epn)
> > return 0;
> >
> > - rem = of_graph_get_remote_port_parent(epn);
> > + rem = of_graph_get_remote_endpoint(epn);
> > if (!rem) {
> > dev_notice(dev, "Remote device at %pOF not found\n",
> > epn);
> > diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
> > index 08b8d5583080..e4e74454e016 100644
> > --- a/drivers/media/platform/atmel/atmel-isi.c
> > +++ b/drivers/media/platform/atmel/atmel-isi.c
> > @@ -1110,7 +1110,7 @@ static int isi_graph_parse(struct atmel_isi *isi, struct device_node *node)
> > if (!ep)
> > return -EINVAL;
> >
> > - remote = of_graph_get_remote_port_parent(ep);
> > + remote = of_graph_get_remote_endpoint(ep);
> > of_node_put(ep);
> > if (!remote)
> > return -EINVAL;
> > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> > index 31ace114eda1..2da34b93e8f4 100644
> > --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> > @@ -395,7 +395,7 @@ static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
> > return -EINVAL;
> > }
> >
> > - csi2rx->asd.match.fwnode = fwnode_graph_get_remote_port_parent(fwh);
> > + csi2rx->asd.match.fwnode = fwnode_graph_get_remote_endpoint(fwh);
> > csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > of_node_put(ep);
> >
> > diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> > index 72bdb3c10962..8fdea45ae090 100644
> > --- a/drivers/media/platform/davinci/vpif_capture.c
> > +++ b/drivers/media/platform/davinci/vpif_capture.c
> > @@ -1542,7 +1542,7 @@ 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;
> > + struct device_node *rem, *rem_ep;
> > unsigned int flags;
> > int err;
> >
> > @@ -1551,13 +1551,22 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> > if (!endpoint)
> > break;
> >
> > - rem = of_graph_get_remote_port_parent(endpoint);
> > - if (!rem) {
> > - dev_dbg(&pdev->dev, "Remote device at %pOF not found\n",
> > + rem_ep = of_graph_get_remote_endpoint(endpoint);
> > + if (!rem_ep) {
> > + dev_dbg(&pdev->dev, "Remote for endpoint %pOF not found\n",
> > endpoint);
> > goto done;
> > }
> >
> > + rem = of_graph_get_port_parent(rem_ep);
> > + if (!rem) {
> > + dev_dbg(&pdev->dev, "Remote endpoint at %pOF not found\n",
> > + rem_ep);
> > + of_node_put(rem_ep);
> > + goto done;
> > + }
> > + of_node_put(rem_ep);
> > +
> > sdinfo = &pdata->subdev_info[i];
> > chan = &pdata->chan_config[i];
> > chan->inputs = devm_kcalloc(&pdev->dev,
> > @@ -1597,12 +1606,13 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> > sdinfo->name = rem->full_name;
> >
> > pdata->asd[i] = v4l2_async_notifier_add_fwnode_subdev(
> > - &vpif_obj.notifier, of_fwnode_handle(rem),
> > + &vpif_obj.notifier, of_fwnode_handle(rem_ep),
> > sizeof(struct v4l2_async_subdev));
> > if (IS_ERR(pdata->asd[i])) {
> > of_node_put(rem);
> > goto err_cleanup;
> > }
> > +
> > }
> >
> > done:
> > diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> > index d1d5041cdae5..0cbc2076b94d 100644
> > --- a/drivers/media/platform/exynos4-is/media-dev.c
> > +++ b/drivers/media/platform/exynos4-is/media-dev.c
> > @@ -411,7 +411,7 @@ static int fimc_md_parse_port_node(struct fimc_md *fmd,
> >
> > pd->mux_id = (endpoint.base.port - 1) & 0x1;
> >
> > - rem = of_graph_get_remote_port_parent(ep);
> > + rem = of_graph_get_remote_endpoint(ep);
> > of_node_put(ep);
> > if (rem == NULL) {
> > v4l2_info(&fmd->v4l2_dev, "Remote device at %pOF not found\n",
> > @@ -1376,11 +1376,17 @@ static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
> > int i;
> >
> > /* Find platform data for this sensor subdev */
> > - for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++)
> > - if (fmd->sensor[i].asd.match.fwnode ==
> > - of_fwnode_handle(subdev->dev->of_node))
> > + for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++) {
> > + struct fwnode_handle *fwnode =
> > + fwnode_graph_get_remote_port_parent(fmd->sensor[i].asd.
> > + match.fwnode);
> > +
> > + if (fwnode == of_fwnode_handle(subdev->dev->of_node))
> > si = &fmd->sensor[i];
> >
> > + fwnode_handle_put(fwnode);
> > + }
> > +
> > if (si == NULL)
> > return -EINVAL;
> >
> > diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
> > index a7b2b89d6155..0aeba52aee13 100644
> > --- a/drivers/media/platform/pxa_camera.c
> > +++ b/drivers/media/platform/pxa_camera.c
> > @@ -2350,7 +2350,7 @@ static int pxa_camera_pdata_from_dt(struct device *dev,
> > pcdev->platform_flags |= PXA_CAMERA_PCLK_EN;
> >
> > asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> > - remote = of_graph_get_remote_port_parent(np);
> > + remote = of_graph_get_remote_endpoint(np);
> > if (remote)
> > asd->match.fwnode = of_fwnode_handle(remote);
> > else
> > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> > index 63da18773d24..a979f210f441 100644
> > --- a/drivers/media/platform/qcom/camss/camss.c
> > +++ b/drivers/media/platform/qcom/camss/camss.c
> > @@ -466,7 +466,7 @@ static int camss_of_parse_ports(struct camss *camss)
> > {
> > struct device *dev = camss->dev;
> > struct device_node *node = NULL;
> > - struct device_node *remote = NULL;
> > + struct fwnode_handle *remote = NULL;
> > int ret, num_subdevs = 0;
> >
> > for_each_endpoint_of_node(dev->of_node, node) {
> > @@ -476,7 +476,8 @@ static int camss_of_parse_ports(struct camss *camss)
> > if (!of_device_is_available(node))
> > continue;
> >
> > - remote = of_graph_get_remote_port_parent(node);
> > + remote = fwnode_graph_get_remote_endpoint(
> > + of_fwnode_handle(node));
> > if (!remote) {
> > dev_err(dev, "Cannot get remote parent\n");
> > ret = -EINVAL;
> > @@ -484,11 +485,10 @@ static int camss_of_parse_ports(struct camss *camss)
> > }
> >
> > asd = v4l2_async_notifier_add_fwnode_subdev(
> > - &camss->notifier, of_fwnode_handle(remote),
> > - sizeof(*csd));
> > + &camss->notifier, remote, sizeof(*csd));
> > if (IS_ERR(asd)) {
> > ret = PTR_ERR(asd);
> > - of_node_put(remote);
> > + fwnode_handle_put(remote);
> > goto err_cleanup;
> > }
> >
> > diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
> > index 608e5217ccd5..258e14c290e8 100644
> > --- a/drivers/media/platform/rcar_drif.c
> > +++ b/drivers/media/platform/rcar_drif.c
> > @@ -1222,7 +1222,7 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr)
> > if (!ep)
> > return 0;
> >
> > - fwnode = fwnode_graph_get_remote_port_parent(ep);
> > + fwnode = fwnode_graph_get_remote_endpoint(ep);
> > if (!fwnode) {
> > dev_warn(sdr->dev, "bad remote port parent\n");
> > fwnode_handle_put(ep);
> > diff --git a/drivers/media/platform/renesas-ceu.c b/drivers/media/platform/renesas-ceu.c
> > index 57d0c0f9fa4b..1e625d560258 100644
> > --- a/drivers/media/platform/renesas-ceu.c
> > +++ b/drivers/media/platform/renesas-ceu.c
> > @@ -1581,7 +1581,7 @@ static int ceu_parse_dt(struct ceu_device *ceudev)
> > ceu_sd = &ceudev->subdevs[i];
> > INIT_LIST_HEAD(&ceu_sd->asd.list);
> >
> > - remote = of_graph_get_remote_port_parent(ep);
> > + remote = of_graph_get_remote_endpoint(ep);
> > ceu_sd->mbus_flags = fw_ep.bus.parallel.flags;
> > ceu_sd->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > ceu_sd->asd.match.fwnode = of_fwnode_handle(remote);
> > diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> > index d855e9c09c08..3c91fe84e0d5 100644
> > --- a/drivers/media/platform/stm32/stm32-dcmi.c
> > +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> > @@ -1602,7 +1602,7 @@ static int dcmi_graph_parse(struct stm32_dcmi *dcmi, struct device_node *node)
> > if (!ep)
> > return -EINVAL;
> >
> > - remote = of_graph_get_remote_port_parent(ep);
> > + remote = of_graph_get_remote_endpoint(ep);
> > of_node_put(ep);
> > if (!remote)
> > return -EINVAL;
> > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> > index 8d075683e448..d7995e2f4c54 100644
> > --- a/drivers/media/platform/ti-vpe/cal.c
> > +++ b/drivers/media/platform/ti-vpe/cal.c
> > @@ -1693,7 +1693,7 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst)
> > goto cleanup_exit;
> > }
> >
> > - sensor_node = of_graph_get_remote_port_parent(ep_node);
> > + sensor_node = of_graph_get_remote_endpoint(ep_node);
> > if (!sensor_node) {
> > ctx_dbg(3, ctx, "can't get remote parent\n");
> > goto cleanup_exit;
> > diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
> > index edce0402155d..41df417153bd 100644
> > --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> > +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> > @@ -14,6 +14,7 @@
> > #include <linux/of.h>
> > #include <linux/of_graph.h>
> > #include <linux/platform_device.h>
> > +#include <linux/property.h>
> > #include <linux/slab.h>
> >
> > #include <media/v4l2-async.h>
> > @@ -81,6 +82,8 @@ static int xvip_graph_build_one(struct xvip_composite_device *xdev,
> > dev_dbg(xdev->dev, "creating links for entity %s\n", local->name);
> >
> > while (1) {
> > + struct fwnode_handle *fwnode;
> > +
> > /* Get the next endpoint and parse its link. */
> > ep = fwnode_graph_get_next_endpoint(entity->asd.match.fwnode,
> > ep);
> > @@ -116,11 +119,13 @@ static int xvip_graph_build_one(struct xvip_composite_device *xdev,
> > continue;
> > }
> >
> > + fwnode = fwnode_graph_get_port_parent(link.remote_node);
> > + fwnode_handle_put(fwnode);
> > +
> > /* Skip DMA engines, they will be processed separately. */
> > - if (link.remote_node == of_fwnode_handle(xdev->dev->of_node)) {
> > + if (fwnode == dev_fwnode(xdev->dev)) {
> > dev_dbg(xdev->dev, "skipping DMA port %p:%u\n",
> > link.local_node, link.local_port);
> > - v4l2_fwnode_put_link(&link);
> > continue;
> > }
> >
> > @@ -374,9 +379,11 @@ static int xvip_graph_parse_one(struct xvip_composite_device *xdev,
> > }
> >
> > fwnode_handle_put(ep);
> > + fwnode = fwnode_graph_get_port_parent(remote);
> > + fwnode_handle_put(fwnode);
> >
> > /* Skip entities that we have already processed. */
> > - if (remote == of_fwnode_handle(xdev->dev->of_node) ||
> > + if (fwnode == dev_fwnode(xdev->dev) ||
> > xvip_graph_find_entity(xdev, remote)) {
> > fwnode_handle_put(remote);
> > continue;
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 15b0c44a76e7..304969ff3191 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -670,8 +670,13 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> > * (struct v4l2_subdev.dev), and async sub-device does not
> > * exist independently of the device at any point of time.
> > */
> > - if (!sd->fwnode && sd->dev)
> > - sd->fwnode = dev_fwnode(sd->dev);
> > + if (!sd->fwnode && sd->dev) {
> > + sd->fwnode = fwnode_graph_get_next_endpoint(
> > + dev_fwnode(sd->dev), NULL);
> > + fwnode_handle_put(sd->fwnode);
> > + if (!sd->fwnode)
> > + sd->fwnode = dev_fwnode(sd->dev);
> > + }
> >
> > mutex_lock(&list_lock);
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index dea8917fd912..810b6584b522 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -617,7 +617,7 @@ v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
> >
> > asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> > asd->match.fwnode =
> > - fwnode_graph_get_remote_port_parent(endpoint);
> > + fwnode_graph_get_remote_endpoint(endpoint);
> > if (!asd->match.fwnode) {
> > dev_dbg(dev, "no remote endpoint found\n");
> > ret = -ENOTCONN;
> > @@ -1051,7 +1051,6 @@ v4l2_fwnode_reference_parse_int_props(struct device *dev,
> > {
> > struct fwnode_handle *fwnode;
> > unsigned int index;
> > - int ret;
> > const char *prop = p->name;
> > const char * const *props = p->props;
> > unsigned int nprops = p->nprops;
> > @@ -1087,9 +1086,8 @@ v4l2_fwnode_reference_parse_int_props(struct device *dev,
> > asd = v4l2_async_notifier_add_fwnode_subdev(notifier, fwnode,
> > sizeof(*asd));
> > if (IS_ERR(asd)) {
> > - ret = PTR_ERR(asd);
> > /* not an error if asd already exists */
> > - if (ret == -EEXIST) {
> > + if (PTR_ERR(asd) == -EEXIST) {
> > fwnode_handle_put(fwnode);
> > continue;
> > }
> > @@ -1102,7 +1100,7 @@ v4l2_fwnode_reference_parse_int_props(struct device *dev,
> >
> > error:
> > fwnode_handle_put(fwnode);
> > - return ret;
> > + return PTR_ERR(fwnode);
> > }
> >
> > int v4l2_async_notifier_parse_fwnode_sensor_common(struct device *dev,
> > diff --git a/drivers/staging/media/soc_camera/soc_camera.c b/drivers/staging/media/soc_camera/soc_camera.c
> > index a6232dcd59bc..f0768b469fc2 100644
> > --- a/drivers/staging/media/soc_camera/soc_camera.c
> > +++ b/drivers/staging/media/soc_camera/soc_camera.c
> > @@ -1514,6 +1514,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
> > struct soc_camera_async_client *sasc;
> > struct soc_of_info *info;
> > struct i2c_client *client;
> > + struct device_node *np;
> > char clk_name[V4L2_CLK_NAME_SIZE];
> > int ret;
> >
> > @@ -1548,23 +1549,23 @@ static int soc_of_bind(struct soc_camera_host *ici,
> > v4l2_async_notifier_init(&sasc->notifier);
> >
> > ret = v4l2_async_notifier_add_subdev(&sasc->notifier, info->subdev);
> > - if (ret) {
> > - of_node_put(remote);
> > + if (ret)
> > goto eaddasd;
> > - }
> >
> > sasc->notifier.ops = &soc_camera_async_ops;
> >
> > icd->sasc = sasc;
> > icd->parent = ici->v4l2_dev.dev;
> > + np = of_graph_get_port_parent(remote);
> > + of_node_put(remote);
> >
> > - client = of_find_i2c_device_by_node(remote);
> > + client = of_find_i2c_device_by_node(np);
> >
> > if (client)
> > v4l2_clk_name_i2c(clk_name, sizeof(clk_name),
> > client->adapter->nr, client->addr);
> > else
> > - v4l2_clk_name_of(clk_name, sizeof(clk_name), remote);
> > + v4l2_clk_name_of(clk_name, sizeof(clk_name), np);
> >
> > icd->clk = v4l2_clk_register(&soc_camera_clk_ops, clk_name, icd);
> > if (IS_ERR(icd->clk)) {
> > @@ -1587,6 +1588,7 @@ static int soc_of_bind(struct soc_camera_host *ici,
> > eallocpdev:
> > devm_kfree(ici->v4l2_dev.dev, info);
> > dev_err(ici->v4l2_dev.dev, "group probe failed: %d\n", ret);
> > + of_node_put(np);
> >
> > return ret;
> > }
> > @@ -1603,7 +1605,7 @@ static void scan_of_host(struct soc_camera_host *ici)
> > if (!epn)
> > break;
> >
> > - rem = of_graph_get_remote_port_parent(epn);
> > + rem = of_graph_get_remote_endpoint(epn);
> > if (!rem) {
> > dev_notice(dev, "no remote for %pOF\n", epn);
> > continue;
> > --
> > 2.11.0
> >
>
> --
> Regards,
> Niklas Söderlund
--
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/9] v4l2-async: Get fwnode reference when putting it to the notifier's list
2019-06-14 16:01 ` Jacopo Mondi
@ 2019-06-28 11:35 ` Sakari Ailus
0 siblings, 0 replies; 21+ messages in thread
From: Sakari Ailus @ 2019-06-28 11:35 UTC (permalink / raw)
To: Jacopo Mondi; +Cc: linux-media, hverkuil, laurent.pinchart, niklas.soderlund
Hi Jacopo,
Thanks for the review!
On Fri, Jun 14, 2019 at 06:01:41PM +0200, Jacopo Mondi wrote:
> Hi Sakari,
>
> On Thu, Jun 06, 2019 at 04:02:19PM +0300, Sakari Ailus wrote:
> > 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 <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/platform/am437x/am437x-vpfe.c | 5 ++---
> > drivers/media/platform/davinci/vpif_capture.c | 16 ++++++++--------
> > 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, 23 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> > index db263c0ce48e..ccdbd9227955 100644
> > --- a/drivers/media/platform/am437x/am437x-vpfe.c
> > +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> > @@ -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);
> > diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> > index 8fdea45ae090..2a0c3f3fb443 100644
> > --- a/drivers/media/platform/davinci/vpif_capture.c
> > +++ b/drivers/media/platform/davinci/vpif_capture.c
> > @@ -1512,6 +1512,7 @@ static struct vpif_capture_config *
> > vpif_capture_get_pdata(struct platform_device *pdev)
> > {
> > struct device_node *endpoint = NULL;
> > + struct device_node *rem = NULL, *rem_ep = NULL;
> > struct vpif_capture_config *pdata;
> > struct vpif_subdev_info *sdinfo;
> > struct vpif_capture_chan_config *chan;
> > @@ -1542,7 +1543,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, *rem_ep;
> > unsigned int flags;
> > int err;
> >
> > @@ -1565,7 +1565,6 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> > of_node_put(rem_ep);
> > goto done;
> > }
> > - of_node_put(rem_ep);
> >
> > sdinfo = &pdata->subdev_info[i];
> > chan = &pdata->chan_config[i];
> > @@ -1573,10 +1572,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;
> > @@ -1588,6 +1585,7 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> > if (err) {
> > dev_err(&pdev->dev, "Could not parse the endpoint\n");
> > of_node_put(rem);
> > + of_node_put(rem_ep);
> > goto done;
> > }
> >
> > @@ -1608,11 +1606,11 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> > pdata->asd[i] = v4l2_async_notifier_add_fwnode_subdev(
> > &vpif_obj.notifier, of_fwnode_handle(rem_ep),
> > 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);
> > + of_node_put(rem_ep);
> > }
> >
> > done:
> > @@ -1624,6 +1622,8 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> > return pdata;
> >
> > err_cleanup:
> > + of_node_put(rem);
> > + of_node_put(rem_ep);
> > of_node_put(endpoint);
> > v4l2_async_notifier_cleanup(&vpif_obj.notifier);
> >
> > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> > index a979f210f441..4ab4a47d34f3 100644
> > --- a/drivers/media/platform/qcom/camss/camss.c
> > +++ b/drivers/media/platform/qcom/camss/camss.c
> > @@ -486,9 +486,9 @@ static int camss_of_parse_ports(struct camss *camss)
> >
> > asd = v4l2_async_notifier_add_fwnode_subdev(
> > &camss->notifier, remote, sizeof(*csd));
> > + fwnode_handle_put(remote);
> > if (IS_ERR(asd)) {
> > ret = PTR_ERR(asd);
> > - fwnode_handle_put(remote);
> > goto err_cleanup;
> > }
> >
> > diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
> > index 41df417153bd..d788f4870a23 100644
> > --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> > +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> > @@ -392,9 +392,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;
> > }
> > }
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 304969ff3191..dc4e83b4f6ba 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -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);
> > }
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index 810b6584b522..b48725824580 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -780,23 +780,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;
> > }
> >
> > /*
> > @@ -1085,22 +1079,17 @@ 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)) {
> > /* not an error if asd already exists */
> > - if (PTR_ERR(asd) == -EEXIST) {
> > - fwnode_handle_put(fwnode);
> > + if (PTR_ERR(asd) == -EEXIST)
> > continue;
> > - }
> >
> > - goto error;
> > + return PTR_ERR(asd);
> > }
> > }
> >
> > return PTR_ERR(fwnode) == -ENOENT ? 0 : PTR_ERR(fwnode);
> > -
> > -error:
> > - fwnode_handle_put(fwnode);
> > - return PTR_ERR(fwnode);
> > }
> >
> > int v4l2_async_notifier_parse_fwnode_sensor_common(struct device *dev,
> > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > index 1497bda66c3b..b9141ffa188a 100644
> > --- a/include/media/v4l2-async.h
> > +++ b/include/media/v4l2-async.h
> > @@ -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.
>
> I would add that, as a consequence, the caller need to put their local
> references as soon as they don't need them anymore (possibly just
> after calling this function).
I'd say that's redundant, as it's in line how refcounting generally works:
when you no longer need a reference, you put it.
>
> That apart
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Thanks. :-)
--
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 4/9] v4l2-async: Add v4l2_async_notifier_add_fwnode_remote_subdev
2019-06-14 16:14 ` Jacopo Mondi
@ 2019-06-28 11:47 ` Sakari Ailus
0 siblings, 0 replies; 21+ messages in thread
From: Sakari Ailus @ 2019-06-28 11:47 UTC (permalink / raw)
To: Jacopo Mondi; +Cc: linux-media, hverkuil, laurent.pinchart, niklas.soderlund
Hi Jacopo,
On Fri, Jun 14, 2019 at 06:14:16PM +0200, Jacopo Mondi wrote:
> Hi Sakari,
>
> On Thu, Jun 06, 2019 at 04:02:20PM +0300, Sakari Ailus wrote:
> > v4l2_async_notifier_add_fwnode_remote_subdev is a convenience function for
> > parsing information on V4L2 fwnode subdevs.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/v4l2-core/v4l2-async.c | 23 +++++++++++++++++++++++
> > include/media/v4l2-async.h | 25 +++++++++++++++++++++++++
> > 2 files changed, 48 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index dc4e83b4f6ba..2ea8afbcbf8f 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -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_ep;
> > + int ret;
> > +
> > + remote_ep = fwnode_graph_get_remote_endpoint(endpoint);
> > + if (!remote_ep)
> > + return -ENOTCONN;
> > +
> > + asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> > + asd->match.fwnode = remote_ep;
> > +
> > + ret = v4l2_async_notifier_add_subdev(notif, asd);
> > + if (ret)
> > + fwnode_handle_put(remote_ep);
> > +
> > + 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,
> > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > index b9141ffa188a..55ce3c1672a4 100644
> > --- a/include/media/v4l2-async.h
> > +++ b/include/media/v4l2-async.h
> > @@ -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
>
> Not sure I get what you mean here, maybe you're missing a "to" between
> the "local endpoint" and "the remote sub-device" ?
This was meant to read "local endpoint pointing to 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
>
> and adds
Fixed.
>
> > + * 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.
>
> This makes v4l2_async_notifier_add_fwnode_subdev behave differently from
> v4l2_async_notifier_add_subdev and the here introduced
> v4l2_async_notifier_add_remote_subdev in the sense that it's the only
> one that allocates the asd for the caller. I'm not sure I see the
> advantage of having many functions for doing similar things with a
> slightly different interface. What is the reason add_fwnode-subdev
> allocates the asd for the caller?
Having to do less work in drivers that can be moved elsewhere. The old
function is used by around 20 users so I think it'll be around for a while.
The fwnode API is getting big and I think after this set there should be
more work done in converting the old drivers to use newer (and better)
interfaces.
--
Regards,
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 9/9] ipu3-cio2: Parse information from firmware without using callbacks
2019-06-14 16:31 ` Jacopo Mondi
@ 2019-06-28 11:57 ` Sakari Ailus
0 siblings, 0 replies; 21+ messages in thread
From: Sakari Ailus @ 2019-06-28 11:57 UTC (permalink / raw)
To: Jacopo Mondi; +Cc: linux-media, hverkuil, laurent.pinchart, niklas.soderlund
Hi Jacopo,
On Fri, Jun 14, 2019 at 06:31:42PM +0200, Jacopo Mondi wrote:
> Hi Sakari,
>
> On Thu, Jun 06, 2019 at 04:02:25PM +0300, Sakari Ailus wrote:
> > 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 <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 92 +++++++++++++++++---------------
> > 1 file changed, 49 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > index 690d3bd08ddd..40e8b8617f55 100644
> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > @@ -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(
>
> How would you feel trying to remove this function completely? There
> are only 2 users mainline (rcar-vin and sunxi) and this is 'yet
> another way to parse an endpoint and add it to a notifier async list'.
>
> I would say, stabilizing on the use of one of the three
> v4l2_async_notifier_add_ versions could be desirable...
I agree. The reason I didn't do it in this patchset is that I have no
hardware to test. I'd like to get in what I can test first.
>
> > - &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);
>
> Won't the notifier cleanup put this device node for us?
There was an error so no. But we already did so before checking for the
error. Good catch! ;)
>
> This apart, for the patch itself:
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> Thanks
> j
>
> > + 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);
> > --
> > 2.11.0
> >
--
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2019-06-28 11:57 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-06 13:02 [PATCH v2 0/9] Rework V4L2 fwnode parsing; add defaults and avoid iteration Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 1/9] davinci-vpif: Don't dereference endpoint after putting it, fix refcounting Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 2/9] v4l2-async: Use endpoint node, not device node, for fwnode match Sakari Ailus
2019-06-14 15:45 ` Jacopo Mondi
2019-06-14 21:21 ` Niklas Söderlund
2019-06-21 9:19 ` Jacopo Mondi
2019-06-21 11:43 ` Niklas Söderlund
2019-06-28 8:29 ` Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 3/9] v4l2-async: Get fwnode reference when putting it to the notifier's list Sakari Ailus
2019-06-14 16:01 ` Jacopo Mondi
2019-06-28 11:35 ` Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 4/9] v4l2-async: Add v4l2_async_notifier_add_fwnode_remote_subdev Sakari Ailus
2019-06-14 16:14 ` Jacopo Mondi
2019-06-28 11:47 ` Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 5/9] omap3isp: Rework OF endpoint parsing Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 6/9] v4l2-async: Safely clean up an uninitialised notifier Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 7/9] ipu3-cio2: Clean up notifier's subdev list if parsing endpoints fails Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 8/9] ipu3-cio2: Proceed with notifier init even if there are no subdevs Sakari Ailus
2019-06-06 13:02 ` [PATCH v2 9/9] ipu3-cio2: Parse information from firmware without using callbacks Sakari Ailus
2019-06-14 16:31 ` Jacopo Mondi
2019-06-28 11:57 ` Sakari Ailus
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).