* [RFC 1/1] v4l: async: Use endpoint node, not device node, for fwnode match
@ 2017-12-04 21:03 Sakari Ailus
2017-12-06 10:43 ` Kieran Bingham
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Sakari Ailus @ 2017-12-04 21:03 UTC (permalink / raw)
To: linux-media; +Cc: 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.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Hi Niklas,
What do you think of this one? I've tested this on N9, both sensor and
flash devices work nicely there. No opportunistic checks for backwards
compatibility are needed.
The changes were surprisingly simple, there are only two drivers that
weren't entirely trivial to change (this part is truly weird in exynos4-is
and xilinx-vipp). Converting the two to use the common parsing functions
would be quite a bit more work and would be very nice to test. The changes
in this patch were still relatively simple.
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/davinci/vpif_capture.c | 2 +-
drivers/media/platform/exynos4-is/media-dev.c | 14 ++++++++++----
drivers/media/platform/pxa_camera.c | 2 +-
drivers/media/platform/qcom/camss-8x16/camss.c | 2 +-
drivers/media/platform/rcar_drif.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 | 16 +++++++++++++---
drivers/media/v4l2-core/v4l2-async.c | 8 ++++++--
drivers/media/v4l2-core/v4l2-fwnode.c | 2 +-
13 files changed, 39 insertions(+), 19 deletions(-)
diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
index 0997c640191d..892d9e935d25 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -2493,7 +2493,7 @@ vpfe_get_pdata(struct platform_device *pdev)
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(&pdev->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 13f1c1c797b0..c8bb60eeb629 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -2044,7 +2044,7 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
if (!epn)
break;
- 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 e900995143a3..eafdf91a4541 100644
--- a/drivers/media/platform/atmel/atmel-isi.c
+++ b/drivers/media/platform/atmel/atmel-isi.c
@@ -1119,7 +1119,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);
if (!remote) {
of_node_put(ep);
return -EINVAL;
diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
index fca4dc829f73..7c9c2b2bb710 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -1572,7 +1572,7 @@ vpif_capture_get_pdata(struct platform_device *pdev)
if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
chan->vpif_if.vd_pol = 1;
- rem = of_graph_get_remote_port_parent(endpoint);
+ rem = of_graph_get_remote_endpoint(endpoint);
if (!rem) {
dev_dbg(&pdev->dev, "Remote device at %pOF not found\n",
endpoint);
diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index 0ef583cfc424..ab5dfe6d7ac4 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",
@@ -1363,11 +1363,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.fwnode ==
- of_fwnode_handle(subdev->dev->of_node))
+ for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++) {
+ struct fwnode_handle *fwnode =
+ fwnode_graph_get_port_parent(
+ of_fwnode_handle(subdev->dev->of_node));
+
+ if (fmd->sensor[i].asd.match.fwnode.fwnode == fwnode)
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 4e0839829e6e..82aaafd113d4 100644
--- a/drivers/media/platform/pxa_camera.c
+++ b/drivers/media/platform/pxa_camera.c
@@ -2334,7 +2334,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.fwnode = of_fwnode_handle(remote);
of_node_put(remote);
diff --git a/drivers/media/platform/qcom/camss-8x16/camss.c b/drivers/media/platform/qcom/camss-8x16/camss.c
index 390a42c17b66..73cac6301756 100644
--- a/drivers/media/platform/qcom/camss-8x16/camss.c
+++ b/drivers/media/platform/qcom/camss-8x16/camss.c
@@ -332,7 +332,7 @@ static int camss_of_parse_ports(struct device *dev,
return ret;
}
- remote = of_graph_get_remote_port_parent(node);
+ remote = of_graph_get_remote_endpoint(node);
of_node_put(node);
if (!remote) {
diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
index 63c94f4028a7..f6e0a08d72f4 100644
--- a/drivers/media/platform/rcar_drif.c
+++ b/drivers/media/platform/rcar_drif.c
@@ -1228,7 +1228,7 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr)
return 0;
notifier->subdevs[notifier->num_subdevs] = &sdr->ep.asd;
- 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/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
index ac4c450a6c7d..18e0aa8af3b3 100644
--- a/drivers/media/platform/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/stm32/stm32-dcmi.c
@@ -1511,7 +1511,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);
if (!remote) {
of_node_put(ep);
return -EINVAL;
diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index a1748b84deea..f4af6c5a7c6c 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -1697,7 +1697,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 d881cf09876d..17d4ac0a908d 100644
--- a/drivers/media/platform/xilinx/xilinx-vipp.c
+++ b/drivers/media/platform/xilinx/xilinx-vipp.c
@@ -82,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. */
next = of_graph_get_next_endpoint(entity->node, ep);
if (next == NULL)
@@ -121,8 +123,11 @@ 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 == of_fwnode_handle(xdev->dev->of_node)) {
dev_dbg(xdev->dev, "skipping DMA port %pOF:%u\n",
to_of_node(link.local_node),
link.local_port);
@@ -367,20 +372,25 @@ static int xvip_graph_parse_one(struct xvip_composite_device *xdev,
dev_dbg(xdev->dev, "parsing node %pOF\n", node);
while (1) {
+ struct fwnode_handle *fwnode;
+
ep = of_graph_get_next_endpoint(node, ep);
if (ep == NULL)
break;
dev_dbg(xdev->dev, "handling endpoint %pOF\n", ep);
- remote = of_graph_get_remote_port_parent(ep);
+ remote = of_graph_get_remote_endpoint(ep);
if (remote == NULL) {
ret = -EINVAL;
break;
}
+ fwnode = fwnode_graph_get_port_parent(of_fwnode_handle(remote));
+ fwnode_handle_put(fwnode);
+
/* Skip entities that we have already processed. */
- if (remote == xdev->dev->of_node ||
+ if (fwnode == xdev->dev->of_node ||
xvip_graph_find_entity(xdev, remote)) {
of_node_put(remote);
continue;
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index e5acfab470a5..f53eff07e8b8 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -539,8 +539,12 @@ 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);
+ 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 fb72c7ac04d4..9c17a26d544c 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -360,7 +360,7 @@ static int v4l2_async_notifier_fwnode_parse_endpoint(
asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
asd->match.fwnode.fwnode =
- fwnode_graph_get_remote_port_parent(endpoint);
+ fwnode_graph_get_remote_endpoint(endpoint);
if (!asd->match.fwnode.fwnode) {
dev_warn(dev, "bad remote port parent\n");
ret = -EINVAL;
--
2.11.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [RFC 1/1] v4l: async: Use endpoint node, not device node, for fwnode match
2017-12-04 21:03 [RFC 1/1] v4l: async: Use endpoint node, not device node, for fwnode match Sakari Ailus
@ 2017-12-06 10:43 ` Kieran Bingham
2017-12-06 15:57 ` Niklas Söderlund
2017-12-07 14:29 ` jacopo mondi
2 siblings, 0 replies; 11+ messages in thread
From: Kieran Bingham @ 2017-12-06 10:43 UTC (permalink / raw)
To: Sakari Ailus, linux-media; +Cc: niklas.soderlund, kieran.bingham
Hi Sakari,
Thanks for the patch.
On 04/12/17 21:03, 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.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Excellent - this looks like some good progression towards full endpoint matching
which I think is essential for more complicated devices... (I would say that
though wouldn't I :D )
Spotted that the error messages hadn't been updated in this patch - but as this
is an RFC, then that's not entirely unexpected, though I've highlighted them anyway.
Regards
Kieran
> ---
> Hi Niklas,
>
> What do you think of this one? I've tested this on N9, both sensor and
> flash devices work nicely there. No opportunistic checks for backwards
> compatibility are needed.
>
> The changes were surprisingly simple, there are only two drivers that
> weren't entirely trivial to change (this part is truly weird in exynos4-is
> and xilinx-vipp). Converting the two to use the common parsing functions
> would be quite a bit more work and would be very nice to test. The changes
> in this patch were still relatively simple.
>
> 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/davinci/vpif_capture.c | 2 +-
> drivers/media/platform/exynos4-is/media-dev.c | 14 ++++++++++----
> drivers/media/platform/pxa_camera.c | 2 +-
> drivers/media/platform/qcom/camss-8x16/camss.c | 2 +-
> drivers/media/platform/rcar_drif.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 | 16 +++++++++++++---
> drivers/media/v4l2-core/v4l2-async.c | 8 ++++++--
> drivers/media/v4l2-core/v4l2-fwnode.c | 2 +-
> 13 files changed, 39 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> index 0997c640191d..892d9e935d25 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> @@ -2493,7 +2493,7 @@ vpfe_get_pdata(struct platform_device *pdev)
> 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(&pdev->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 13f1c1c797b0..c8bb60eeb629 100644
> --- a/drivers/media/platform/atmel/atmel-isc.c
> +++ b/drivers/media/platform/atmel/atmel-isc.c
> @@ -2044,7 +2044,7 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
> if (!epn)
> break;
>
> - 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 e900995143a3..eafdf91a4541 100644
> --- a/drivers/media/platform/atmel/atmel-isi.c
> +++ b/drivers/media/platform/atmel/atmel-isi.c
> @@ -1119,7 +1119,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);
> if (!remote) {
> of_node_put(ep);
> return -EINVAL;
> diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> index fca4dc829f73..7c9c2b2bb710 100644
> --- a/drivers/media/platform/davinci/vpif_capture.c
> +++ b/drivers/media/platform/davinci/vpif_capture.c
> @@ -1572,7 +1572,7 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> chan->vpif_if.vd_pol = 1;
>
> - rem = of_graph_get_remote_port_parent(endpoint);
> + rem = of_graph_get_remote_endpoint(endpoint);
> if (!rem) {
> dev_dbg(&pdev->dev, "Remote device at %pOF not found\n",
> endpoint);
> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> index 0ef583cfc424..ab5dfe6d7ac4 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",
> @@ -1363,11 +1363,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.fwnode ==
> - of_fwnode_handle(subdev->dev->of_node))
> + for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++) {
> + struct fwnode_handle *fwnode =
> + fwnode_graph_get_port_parent(
> + of_fwnode_handle(subdev->dev->of_node));
> +
> + if (fmd->sensor[i].asd.match.fwnode.fwnode == fwnode)
> 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 4e0839829e6e..82aaafd113d4 100644
> --- a/drivers/media/platform/pxa_camera.c
> +++ b/drivers/media/platform/pxa_camera.c
> @@ -2334,7 +2334,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.fwnode = of_fwnode_handle(remote);
> of_node_put(remote);
> diff --git a/drivers/media/platform/qcom/camss-8x16/camss.c b/drivers/media/platform/qcom/camss-8x16/camss.c
> index 390a42c17b66..73cac6301756 100644
> --- a/drivers/media/platform/qcom/camss-8x16/camss.c
> +++ b/drivers/media/platform/qcom/camss-8x16/camss.c
> @@ -332,7 +332,7 @@ static int camss_of_parse_ports(struct device *dev,
> return ret;
> }
>
> - remote = of_graph_get_remote_port_parent(node);
> + remote = of_graph_get_remote_endpoint(node);
> of_node_put(node);
>
> if (!remote) {
> diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
> index 63c94f4028a7..f6e0a08d72f4 100644
> --- a/drivers/media/platform/rcar_drif.c
> +++ b/drivers/media/platform/rcar_drif.c
> @@ -1228,7 +1228,7 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr)
> return 0;
>
> notifier->subdevs[notifier->num_subdevs] = &sdr->ep.asd;
> - 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");
s/port parent/endpoint/
> fwnode_handle_put(ep);
> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> index ac4c450a6c7d..18e0aa8af3b3 100644
> --- a/drivers/media/platform/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> @@ -1511,7 +1511,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);
> if (!remote) {
> of_node_put(ep);
> return -EINVAL;
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index a1748b84deea..f4af6c5a7c6c 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -1697,7 +1697,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 d881cf09876d..17d4ac0a908d 100644
> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> @@ -82,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. */
> next = of_graph_get_next_endpoint(entity->node, ep);
> if (next == NULL)
> @@ -121,8 +123,11 @@ 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 == of_fwnode_handle(xdev->dev->of_node)) {
> dev_dbg(xdev->dev, "skipping DMA port %pOF:%u\n",
> to_of_node(link.local_node),
> link.local_port);
> @@ -367,20 +372,25 @@ static int xvip_graph_parse_one(struct xvip_composite_device *xdev,
> dev_dbg(xdev->dev, "parsing node %pOF\n", node);
>
> while (1) {
> + struct fwnode_handle *fwnode;
> +
> ep = of_graph_get_next_endpoint(node, ep);
> if (ep == NULL)
> break;
>
> dev_dbg(xdev->dev, "handling endpoint %pOF\n", ep);
>
> - remote = of_graph_get_remote_port_parent(ep);
> + remote = of_graph_get_remote_endpoint(ep);
> if (remote == NULL) {
> ret = -EINVAL;
> break;
> }
>
> + fwnode = fwnode_graph_get_port_parent(of_fwnode_handle(remote));
> + fwnode_handle_put(fwnode);
> +
> /* Skip entities that we have already processed. */
> - if (remote == xdev->dev->of_node ||
> + if (fwnode == xdev->dev->of_node ||
> xvip_graph_find_entity(xdev, remote)) {
> of_node_put(remote);
> continue;
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index e5acfab470a5..f53eff07e8b8 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -539,8 +539,12 @@ 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);
> + 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 fb72c7ac04d4..9c17a26d544c 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -360,7 +360,7 @@ static int v4l2_async_notifier_fwnode_parse_endpoint(
>
> asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> asd->match.fwnode.fwnode =
> - fwnode_graph_get_remote_port_parent(endpoint);
> + fwnode_graph_get_remote_endpoint(endpoint);
> if (!asd->match.fwnode.fwnode) {
> dev_warn(dev, "bad remote port parent\n");
s/port parent/endpoint/
> ret = -EINVAL;
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC 1/1] v4l: async: Use endpoint node, not device node, for fwnode match
2017-12-04 21:03 [RFC 1/1] v4l: async: Use endpoint node, not device node, for fwnode match Sakari Ailus
2017-12-06 10:43 ` Kieran Bingham
@ 2017-12-06 15:57 ` Niklas Söderlund
2017-12-06 16:36 ` Kieran Bingham
2017-12-07 7:39 ` Sakari Ailus
2017-12-07 14:29 ` jacopo mondi
2 siblings, 2 replies; 11+ messages in thread
From: Niklas Söderlund @ 2017-12-06 15:57 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, jmondi, Kieran Bingham
CC Jacopo, Kieran
Hi Sakari,
Thanks for your patch.
On 2017-12-04 23:03:02 +0200, 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.
Endpoint matching I think is the way to go forward. It will solve a set
of problems that exists today. So I think this a good step in the right
direction.
>
> 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.
This assumption is not true, the adv748x exposes multiple subdevice from
a single device node in DT and registers them using different endpoints.
Now the only user of the adv748x driver I know of is the rcar-csi2
driver which is not yet upstream so this can be consider a special case.
Unfortunately this patch do break already existing configurations
upstream :-( For example the Koelsch board, from r8a7791-koelsch.dts:
hdmi-in {
compatible = "hdmi-connector";
type = "a";
port {
hdmi_con_in: endpoint {
remote-endpoint = <&adv7612_in>;
};
};
};
hdmi-in@4c {
compatible = "adi,adv7612";
reg = <0x4c>;
interrupt-parent = <&gpio4>;
interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
default-input = <0>;
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>;
};
};
};
};
&vin0 {
status = "okay";
pinctrl-0 = <&vin0_pins>;
pinctrl-names = "default";
port {
#address-cells = <1>;
#size-cells = <0>;
vin0ep2: endpoint {
remote-endpoint = <&adv7612_out>;
bus-width = <24>;
hsync-active = <0>;
vsync-active = <0>;
pclk-sample = <1>;
data-active = <1>;
};
};
};
Here the adv7612 driver would register a subdevice using the endpoint
'hdmi-in@4c/ports/port@0/endpoint' while the rcar-vin driver which uses
the async parsing helpers would register a notifier looking for
'hdmi-in@4c/ports/port@2/endpoint'.
Something like Kieran's '[PATCH v5] v4l2-async: Match parent devices'
would be needed in addition to this patch. I tried Kieran's patch
together with this and it did not solve the problem upstream. I did make
a hack on-top of Kieran's patch to add a comparison 'sd_parent ==
asd_parent' in match_fwnode() which got rcar-gen2 working again, but I'm
not sure if that will have other side effects.
>
> For async devices that have no endpoints, continue to use the fwnode
> related to the device. This includes e.g. lens devices.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Hi Niklas,
>
> What do you think of this one? I've tested this on N9, both sensor and
> flash devices work nicely there. No opportunistic checks for backwards
> compatibility are needed.
Over all I like it, endpoint matching I think is a good thing! If we can
sort out the issue above I be happy to use the new async parsing helpers
in rcar-csi2 driver.
>
> The changes were surprisingly simple, there are only two drivers that
> weren't entirely trivial to change (this part is truly weird in exynos4-is
> and xilinx-vipp). Converting the two to use the common parsing functions
> would be quite a bit more work and would be very nice to test. The changes
> in this patch were still relatively simple.
>
> 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/davinci/vpif_capture.c | 2 +-
> drivers/media/platform/exynos4-is/media-dev.c | 14 ++++++++++----
> drivers/media/platform/pxa_camera.c | 2 +-
> drivers/media/platform/qcom/camss-8x16/camss.c | 2 +-
> drivers/media/platform/rcar_drif.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 | 16 +++++++++++++---
> drivers/media/v4l2-core/v4l2-async.c | 8 ++++++--
> drivers/media/v4l2-core/v4l2-fwnode.c | 2 +-
> 13 files changed, 39 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> index 0997c640191d..892d9e935d25 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> @@ -2493,7 +2493,7 @@ vpfe_get_pdata(struct platform_device *pdev)
> 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(&pdev->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 13f1c1c797b0..c8bb60eeb629 100644
> --- a/drivers/media/platform/atmel/atmel-isc.c
> +++ b/drivers/media/platform/atmel/atmel-isc.c
> @@ -2044,7 +2044,7 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
> if (!epn)
> break;
>
> - 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 e900995143a3..eafdf91a4541 100644
> --- a/drivers/media/platform/atmel/atmel-isi.c
> +++ b/drivers/media/platform/atmel/atmel-isi.c
> @@ -1119,7 +1119,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);
> if (!remote) {
> of_node_put(ep);
> return -EINVAL;
> diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> index fca4dc829f73..7c9c2b2bb710 100644
> --- a/drivers/media/platform/davinci/vpif_capture.c
> +++ b/drivers/media/platform/davinci/vpif_capture.c
> @@ -1572,7 +1572,7 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> chan->vpif_if.vd_pol = 1;
>
> - rem = of_graph_get_remote_port_parent(endpoint);
> + rem = of_graph_get_remote_endpoint(endpoint);
> if (!rem) {
> dev_dbg(&pdev->dev, "Remote device at %pOF not found\n",
> endpoint);
> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> index 0ef583cfc424..ab5dfe6d7ac4 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",
> @@ -1363,11 +1363,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.fwnode ==
> - of_fwnode_handle(subdev->dev->of_node))
> + for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++) {
> + struct fwnode_handle *fwnode =
> + fwnode_graph_get_port_parent(
> + of_fwnode_handle(subdev->dev->of_node));
> +
> + if (fmd->sensor[i].asd.match.fwnode.fwnode == fwnode)
> 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 4e0839829e6e..82aaafd113d4 100644
> --- a/drivers/media/platform/pxa_camera.c
> +++ b/drivers/media/platform/pxa_camera.c
> @@ -2334,7 +2334,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.fwnode = of_fwnode_handle(remote);
> of_node_put(remote);
> diff --git a/drivers/media/platform/qcom/camss-8x16/camss.c b/drivers/media/platform/qcom/camss-8x16/camss.c
> index 390a42c17b66..73cac6301756 100644
> --- a/drivers/media/platform/qcom/camss-8x16/camss.c
> +++ b/drivers/media/platform/qcom/camss-8x16/camss.c
> @@ -332,7 +332,7 @@ static int camss_of_parse_ports(struct device *dev,
> return ret;
> }
>
> - remote = of_graph_get_remote_port_parent(node);
> + remote = of_graph_get_remote_endpoint(node);
> of_node_put(node);
>
> if (!remote) {
> diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
> index 63c94f4028a7..f6e0a08d72f4 100644
> --- a/drivers/media/platform/rcar_drif.c
> +++ b/drivers/media/platform/rcar_drif.c
> @@ -1228,7 +1228,7 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr)
> return 0;
>
> notifier->subdevs[notifier->num_subdevs] = &sdr->ep.asd;
> - 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/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> index ac4c450a6c7d..18e0aa8af3b3 100644
> --- a/drivers/media/platform/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> @@ -1511,7 +1511,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);
> if (!remote) {
> of_node_put(ep);
> return -EINVAL;
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index a1748b84deea..f4af6c5a7c6c 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -1697,7 +1697,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 d881cf09876d..17d4ac0a908d 100644
> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> @@ -82,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. */
> next = of_graph_get_next_endpoint(entity->node, ep);
> if (next == NULL)
> @@ -121,8 +123,11 @@ 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 == of_fwnode_handle(xdev->dev->of_node)) {
> dev_dbg(xdev->dev, "skipping DMA port %pOF:%u\n",
> to_of_node(link.local_node),
> link.local_port);
> @@ -367,20 +372,25 @@ static int xvip_graph_parse_one(struct xvip_composite_device *xdev,
> dev_dbg(xdev->dev, "parsing node %pOF\n", node);
>
> while (1) {
> + struct fwnode_handle *fwnode;
> +
> ep = of_graph_get_next_endpoint(node, ep);
> if (ep == NULL)
> break;
>
> dev_dbg(xdev->dev, "handling endpoint %pOF\n", ep);
>
> - remote = of_graph_get_remote_port_parent(ep);
> + remote = of_graph_get_remote_endpoint(ep);
> if (remote == NULL) {
> ret = -EINVAL;
> break;
> }
>
> + fwnode = fwnode_graph_get_port_parent(of_fwnode_handle(remote));
> + fwnode_handle_put(fwnode);
> +
> /* Skip entities that we have already processed. */
> - if (remote == xdev->dev->of_node ||
> + if (fwnode == xdev->dev->of_node ||
> xvip_graph_find_entity(xdev, remote)) {
> of_node_put(remote);
> continue;
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index e5acfab470a5..f53eff07e8b8 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -539,8 +539,12 @@ 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);
> + 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 fb72c7ac04d4..9c17a26d544c 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -360,7 +360,7 @@ static int v4l2_async_notifier_fwnode_parse_endpoint(
>
> asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> asd->match.fwnode.fwnode =
> - fwnode_graph_get_remote_port_parent(endpoint);
> + fwnode_graph_get_remote_endpoint(endpoint);
> if (!asd->match.fwnode.fwnode) {
> dev_warn(dev, "bad remote port parent\n");
> ret = -EINVAL;
> --
> 2.11.0
>
--
Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC 1/1] v4l: async: Use endpoint node, not device node, for fwnode match
2017-12-06 15:57 ` Niklas Söderlund
@ 2017-12-06 16:36 ` Kieran Bingham
2017-12-07 7:39 ` Sakari Ailus
1 sibling, 0 replies; 11+ messages in thread
From: Kieran Bingham @ 2017-12-06 16:36 UTC (permalink / raw)
To: Niklas Söderlund, Sakari Ailus; +Cc: linux-media, jmondi
Hi Niklas,
On 06/12/17 15:57, Niklas Söderlund wrote:
> CC Jacopo, Kieran
>
> Hi Sakari,
>
> Thanks for your patch.
>
> On 2017-12-04 23:03:02 +0200, 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.
>
> Endpoint matching I think is the way to go forward. It will solve a set
> of problems that exists today. So I think this a good step in the right
> direction.
>
>>
>> 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.
>
> This assumption is not true, the adv748x exposes multiple subdevice from
> a single device node in DT and registers them using different endpoints.
> Now the only user of the adv748x driver I know of is the rcar-csi2
> driver which is not yet upstream so this can be consider a special case.
Quite - but the match parent patch still hasn't got upstream yet either - so
it's still not supported.
I'd love to know if there are other devices with an ADV748x out there though!
<snip>
> Here the adv7612 driver would register a subdevice using the endpoint
> 'hdmi-in@4c/ports/port@0/endpoint' while the rcar-vin driver which uses
> the async parsing helpers would register a notifier looking for
> 'hdmi-in@4c/ports/port@2/endpoint'.
>
> Something like Kieran's '[PATCH v5] v4l2-async: Match parent devices'
> would be needed in addition to this patch. I tried Kieran's patch
> together with this and it did not solve the problem upstream. I did make
> a hack on-top of Kieran's patch to add a comparison 'sd_parent ==
> asd_parent' in match_fwnode() which got rcar-gen2 working again, but I'm
> not sure if that will have other side effects.
Matching parent == parent will have the side effect that all devices with
multiple endpoints, will discover all endpoints successfully match on both
device comparisons.
Thus, this in effect will completely undo all endpoint matching efforts.
<snip>
Regards
Kieran
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC 1/1] v4l: async: Use endpoint node, not device node, for fwnode match
2017-12-06 15:57 ` Niklas Söderlund
2017-12-06 16:36 ` Kieran Bingham
@ 2017-12-07 7:39 ` Sakari Ailus
2017-12-19 9:12 ` Niklas Söderlund
1 sibling, 1 reply; 11+ messages in thread
From: Sakari Ailus @ 2017-12-07 7:39 UTC (permalink / raw)
To: Niklas Söderlund; +Cc: linux-media, jmondi, Kieran Bingham
Hej Niklas,
Tack för dina kommentarer!
On Wed, Dec 06, 2017 at 04:57:48PM +0100, Niklas Söderlund wrote:
> CC Jacopo, Kieran
>
> Hi Sakari,
>
> Thanks for your patch.
>
> On 2017-12-04 23:03:02 +0200, 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.
>
> Endpoint matching I think is the way to go forward. It will solve a set
> of problems that exists today. So I think this a good step in the right
> direction.
>
> >
> > 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.
>
> This assumption is not true, the adv748x exposes multiple subdevice from
> a single device node in DT and registers them using different endpoints.
> Now the only user of the adv748x driver I know of is the rcar-csi2
> driver which is not yet upstream so this can be consider a special case.
Right, the adv748x is an exception to this but I could argue it should
never have been merged in its current state: it does not work with other
bridge / ISP drivers.
>
> Unfortunately this patch do break already existing configurations
> upstream :-( For example the Koelsch board, from r8a7791-koelsch.dts:
>
> hdmi-in {
> compatible = "hdmi-connector";
> type = "a";
>
> port {
> hdmi_con_in: endpoint {
> remote-endpoint = <&adv7612_in>;
> };
> };
> };
>
> hdmi-in@4c {
> compatible = "adi,adv7612";
> reg = <0x4c>;
> interrupt-parent = <&gpio4>;
> interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
> default-input = <0>;
>
> 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>;
> };
> };
> };
> };
>
> &vin0 {
> status = "okay";
> pinctrl-0 = <&vin0_pins>;
> pinctrl-names = "default";
>
> port {
> #address-cells = <1>;
> #size-cells = <0>;
>
> vin0ep2: endpoint {
> remote-endpoint = <&adv7612_out>;
> bus-width = <24>;
> hsync-active = <0>;
> vsync-active = <0>;
> pclk-sample = <1>;
> data-active = <1>;
> };
> };
> };
>
> Here the adv7612 driver would register a subdevice using the endpoint
> 'hdmi-in@4c/ports/port@0/endpoint' while the rcar-vin driver which uses
The adv7612 needs to register both of these endpoints. I wonder if there
are repercussions by doing that.
> the async parsing helpers would register a notifier looking for
> 'hdmi-in@4c/ports/port@2/endpoint'.
>
> Something like Kieran's '[PATCH v5] v4l2-async: Match parent devices'
> would be needed in addition to this patch. I tried Kieran's patch
> together with this and it did not solve the problem upstream. I did make
The more I've been working on this problem, the less I think
opportunistically matching devices or endpoints is a good idea. Lens
devices will always use device nodes and flash devices use LED nodes found
under device nodes.
It's getting messy with opportunistic matching. And as this patch shows,
it's not that hard to convert all drivers either, so why not to do just
that?
--
Hälsningar,
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC 1/1] v4l: async: Use endpoint node, not device node, for fwnode match
2017-12-04 21:03 [RFC 1/1] v4l: async: Use endpoint node, not device node, for fwnode match Sakari Ailus
2017-12-06 10:43 ` Kieran Bingham
2017-12-06 15:57 ` Niklas Söderlund
@ 2017-12-07 14:29 ` jacopo mondi
2017-12-14 10:53 ` Sakari Ailus
2 siblings, 1 reply; 11+ messages in thread
From: jacopo mondi @ 2017-12-07 14:29 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, niklas.soderlund, kieran bingham
Hi Sakari!
thanks for proposing this
While we all agree that full endpoint matching is the right
thing to do (see also Kieran's last reply to his "v4l2-async: Match
parent devices" patch) I have some perplexity on this proposal,
please see below
On Mon, Dec 04, 2017 at 11:03:02PM +0200, 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.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Hi Niklas,
>
> What do you think of this one? I've tested this on N9, both sensor and
> flash devices work nicely there. No opportunistic checks for backwards
> compatibility are needed.
>
> The changes were surprisingly simple, there are only two drivers that
> weren't entirely trivial to change (this part is truly weird in exynos4-is
> and xilinx-vipp). Converting the two to use the common parsing functions
> would be quite a bit more work and would be very nice to test. The changes
> in this patch were still relatively simple.
>
> 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/davinci/vpif_capture.c | 2 +-
> drivers/media/platform/exynos4-is/media-dev.c | 14 ++++++++++----
> drivers/media/platform/pxa_camera.c | 2 +-
> drivers/media/platform/qcom/camss-8x16/camss.c | 2 +-
> drivers/media/platform/rcar_drif.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 | 16 +++++++++++++---
> drivers/media/v4l2-core/v4l2-async.c | 8 ++++++--
> drivers/media/v4l2-core/v4l2-fwnode.c | 2 +-
> 13 files changed, 39 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> index 0997c640191d..892d9e935d25 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> @@ -2493,7 +2493,7 @@ vpfe_get_pdata(struct platform_device *pdev)
> 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(&pdev->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 13f1c1c797b0..c8bb60eeb629 100644
> --- a/drivers/media/platform/atmel/atmel-isc.c
> +++ b/drivers/media/platform/atmel/atmel-isc.c
> @@ -2044,7 +2044,7 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
> if (!epn)
> break;
>
> - 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 e900995143a3..eafdf91a4541 100644
> --- a/drivers/media/platform/atmel/atmel-isi.c
> +++ b/drivers/media/platform/atmel/atmel-isi.c
> @@ -1119,7 +1119,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);
> if (!remote) {
> of_node_put(ep);
> return -EINVAL;
> diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> index fca4dc829f73..7c9c2b2bb710 100644
> --- a/drivers/media/platform/davinci/vpif_capture.c
> +++ b/drivers/media/platform/davinci/vpif_capture.c
> @@ -1572,7 +1572,7 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> chan->vpif_if.vd_pol = 1;
>
> - rem = of_graph_get_remote_port_parent(endpoint);
> + rem = of_graph_get_remote_endpoint(endpoint);
> if (!rem) {
> dev_dbg(&pdev->dev, "Remote device at %pOF not found\n",
> endpoint);
> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> index 0ef583cfc424..ab5dfe6d7ac4 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",
> @@ -1363,11 +1363,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.fwnode ==
> - of_fwnode_handle(subdev->dev->of_node))
> + for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++) {
> + struct fwnode_handle *fwnode =
> + fwnode_graph_get_port_parent(
> + of_fwnode_handle(subdev->dev->of_node));
> +
> + if (fmd->sensor[i].asd.match.fwnode.fwnode == fwnode)
> 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 4e0839829e6e..82aaafd113d4 100644
> --- a/drivers/media/platform/pxa_camera.c
> +++ b/drivers/media/platform/pxa_camera.c
> @@ -2334,7 +2334,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.fwnode = of_fwnode_handle(remote);
> of_node_put(remote);
> diff --git a/drivers/media/platform/qcom/camss-8x16/camss.c b/drivers/media/platform/qcom/camss-8x16/camss.c
> index 390a42c17b66..73cac6301756 100644
> --- a/drivers/media/platform/qcom/camss-8x16/camss.c
> +++ b/drivers/media/platform/qcom/camss-8x16/camss.c
> @@ -332,7 +332,7 @@ static int camss_of_parse_ports(struct device *dev,
> return ret;
> }
>
> - remote = of_graph_get_remote_port_parent(node);
> + remote = of_graph_get_remote_endpoint(node);
> of_node_put(node);
>
> if (!remote) {
> diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
> index 63c94f4028a7..f6e0a08d72f4 100644
> --- a/drivers/media/platform/rcar_drif.c
> +++ b/drivers/media/platform/rcar_drif.c
> @@ -1228,7 +1228,7 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr)
> return 0;
>
> notifier->subdevs[notifier->num_subdevs] = &sdr->ep.asd;
> - 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/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> index ac4c450a6c7d..18e0aa8af3b3 100644
> --- a/drivers/media/platform/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> @@ -1511,7 +1511,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);
> if (!remote) {
> of_node_put(ep);
> return -EINVAL;
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index a1748b84deea..f4af6c5a7c6c 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -1697,7 +1697,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 d881cf09876d..17d4ac0a908d 100644
> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> @@ -82,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. */
> next = of_graph_get_next_endpoint(entity->node, ep);
> if (next == NULL)
> @@ -121,8 +123,11 @@ 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 == of_fwnode_handle(xdev->dev->of_node)) {
> dev_dbg(xdev->dev, "skipping DMA port %pOF:%u\n",
> to_of_node(link.local_node),
> link.local_port);
> @@ -367,20 +372,25 @@ static int xvip_graph_parse_one(struct xvip_composite_device *xdev,
> dev_dbg(xdev->dev, "parsing node %pOF\n", node);
>
> while (1) {
> + struct fwnode_handle *fwnode;
> +
> ep = of_graph_get_next_endpoint(node, ep);
> if (ep == NULL)
> break;
>
> dev_dbg(xdev->dev, "handling endpoint %pOF\n", ep);
>
> - remote = of_graph_get_remote_port_parent(ep);
> + remote = of_graph_get_remote_endpoint(ep);
> if (remote == NULL) {
> ret = -EINVAL;
> break;
> }
>
> + fwnode = fwnode_graph_get_port_parent(of_fwnode_handle(remote));
> + fwnode_handle_put(fwnode);
> +
> /* Skip entities that we have already processed. */
> - if (remote == xdev->dev->of_node ||
> + if (fwnode == xdev->dev->of_node ||
> xvip_graph_find_entity(xdev, remote)) {
> of_node_put(remote);
> continue;
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index e5acfab470a5..f53eff07e8b8 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -539,8 +539,12 @@ 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);
> + if (!sd->fwnode)
> + sd->fwnode = dev_fwnode(sd->dev);
> + }
I'm facing these days issues with endpoint matching and imo assuming
the first available endpoint is the one the subdevice should be
matched on is a limitation that will bite us back in future.
What I'm dealing with is a device with 5 'ports' node, and a single
endpoint per 'ports'. Your patch here takes the first available
endpoint with 'fwnode_graph_get_next_endpoint()', and unfortunately
that's not the one the notifier expects to match with.
The only way around it that I have found is set explicitly the
fwnode before registering the subdevice and having the framework
make assumptions like this (match on the first available endpoint) may
actually hide subtle issues difficult to debug.
Should we force subdevices to set their fwnode explicitly before
registering the subdevice or at least WARN() when they don't?
Converting existing sensor drivers to do so, should be as easy as
converting platform drivers to match endpoints, as you did in this
series.
Thanks!
j
>
> mutex_lock(&list_lock);
>
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index fb72c7ac04d4..9c17a26d544c 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -360,7 +360,7 @@ static int v4l2_async_notifier_fwnode_parse_endpoint(
>
> asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> asd->match.fwnode.fwnode =
> - fwnode_graph_get_remote_port_parent(endpoint);
> + fwnode_graph_get_remote_endpoint(endpoint);
> if (!asd->match.fwnode.fwnode) {
> dev_warn(dev, "bad remote port parent\n");
> ret = -EINVAL;
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC 1/1] v4l: async: Use endpoint node, not device node, for fwnode match
2017-12-07 14:29 ` jacopo mondi
@ 2017-12-14 10:53 ` Sakari Ailus
2017-12-14 11:44 ` jacopo mondi
0 siblings, 1 reply; 11+ messages in thread
From: Sakari Ailus @ 2017-12-14 10:53 UTC (permalink / raw)
To: jacopo mondi; +Cc: linux-media, niklas.soderlund, kieran bingham
Hi Jacopo,
On Thu, Dec 07, 2017 at 03:29:40PM +0100, jacopo mondi wrote:
> Hi Sakari!
> thanks for proposing this
>
> While we all agree that full endpoint matching is the right
> thing to do (see also Kieran's last reply to his "v4l2-async: Match
> parent devices" patch) I have some perplexity on this proposal,
> please see below
>
> On Mon, Dec 04, 2017 at 11:03:02PM +0200, 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.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > Hi Niklas,
> >
> > What do you think of this one? I've tested this on N9, both sensor and
> > flash devices work nicely there. No opportunistic checks for backwards
> > compatibility are needed.
> >
> > The changes were surprisingly simple, there are only two drivers that
> > weren't entirely trivial to change (this part is truly weird in exynos4-is
> > and xilinx-vipp). Converting the two to use the common parsing functions
> > would be quite a bit more work and would be very nice to test. The changes
> > in this patch were still relatively simple.
> >
> > 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/davinci/vpif_capture.c | 2 +-
> > drivers/media/platform/exynos4-is/media-dev.c | 14 ++++++++++----
> > drivers/media/platform/pxa_camera.c | 2 +-
> > drivers/media/platform/qcom/camss-8x16/camss.c | 2 +-
> > drivers/media/platform/rcar_drif.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 | 16 +++++++++++++---
> > drivers/media/v4l2-core/v4l2-async.c | 8 ++++++--
> > drivers/media/v4l2-core/v4l2-fwnode.c | 2 +-
> > 13 files changed, 39 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> > index 0997c640191d..892d9e935d25 100644
> > --- a/drivers/media/platform/am437x/am437x-vpfe.c
> > +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> > @@ -2493,7 +2493,7 @@ vpfe_get_pdata(struct platform_device *pdev)
> > 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(&pdev->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 13f1c1c797b0..c8bb60eeb629 100644
> > --- a/drivers/media/platform/atmel/atmel-isc.c
> > +++ b/drivers/media/platform/atmel/atmel-isc.c
> > @@ -2044,7 +2044,7 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
> > if (!epn)
> > break;
> >
> > - 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 e900995143a3..eafdf91a4541 100644
> > --- a/drivers/media/platform/atmel/atmel-isi.c
> > +++ b/drivers/media/platform/atmel/atmel-isi.c
> > @@ -1119,7 +1119,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);
> > if (!remote) {
> > of_node_put(ep);
> > return -EINVAL;
> > diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> > index fca4dc829f73..7c9c2b2bb710 100644
> > --- a/drivers/media/platform/davinci/vpif_capture.c
> > +++ b/drivers/media/platform/davinci/vpif_capture.c
> > @@ -1572,7 +1572,7 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> > if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> > chan->vpif_if.vd_pol = 1;
> >
> > - rem = of_graph_get_remote_port_parent(endpoint);
> > + rem = of_graph_get_remote_endpoint(endpoint);
> > if (!rem) {
> > dev_dbg(&pdev->dev, "Remote device at %pOF not found\n",
> > endpoint);
> > diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> > index 0ef583cfc424..ab5dfe6d7ac4 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",
> > @@ -1363,11 +1363,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.fwnode ==
> > - of_fwnode_handle(subdev->dev->of_node))
> > + for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++) {
> > + struct fwnode_handle *fwnode =
> > + fwnode_graph_get_port_parent(
> > + of_fwnode_handle(subdev->dev->of_node));
> > +
> > + if (fmd->sensor[i].asd.match.fwnode.fwnode == fwnode)
> > 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 4e0839829e6e..82aaafd113d4 100644
> > --- a/drivers/media/platform/pxa_camera.c
> > +++ b/drivers/media/platform/pxa_camera.c
> > @@ -2334,7 +2334,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.fwnode = of_fwnode_handle(remote);
> > of_node_put(remote);
> > diff --git a/drivers/media/platform/qcom/camss-8x16/camss.c b/drivers/media/platform/qcom/camss-8x16/camss.c
> > index 390a42c17b66..73cac6301756 100644
> > --- a/drivers/media/platform/qcom/camss-8x16/camss.c
> > +++ b/drivers/media/platform/qcom/camss-8x16/camss.c
> > @@ -332,7 +332,7 @@ static int camss_of_parse_ports(struct device *dev,
> > return ret;
> > }
> >
> > - remote = of_graph_get_remote_port_parent(node);
> > + remote = of_graph_get_remote_endpoint(node);
> > of_node_put(node);
> >
> > if (!remote) {
> > diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
> > index 63c94f4028a7..f6e0a08d72f4 100644
> > --- a/drivers/media/platform/rcar_drif.c
> > +++ b/drivers/media/platform/rcar_drif.c
> > @@ -1228,7 +1228,7 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr)
> > return 0;
> >
> > notifier->subdevs[notifier->num_subdevs] = &sdr->ep.asd;
> > - 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/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> > index ac4c450a6c7d..18e0aa8af3b3 100644
> > --- a/drivers/media/platform/stm32/stm32-dcmi.c
> > +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> > @@ -1511,7 +1511,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);
> > if (!remote) {
> > of_node_put(ep);
> > return -EINVAL;
> > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> > index a1748b84deea..f4af6c5a7c6c 100644
> > --- a/drivers/media/platform/ti-vpe/cal.c
> > +++ b/drivers/media/platform/ti-vpe/cal.c
> > @@ -1697,7 +1697,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 d881cf09876d..17d4ac0a908d 100644
> > --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> > +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> > @@ -82,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. */
> > next = of_graph_get_next_endpoint(entity->node, ep);
> > if (next == NULL)
> > @@ -121,8 +123,11 @@ 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 == of_fwnode_handle(xdev->dev->of_node)) {
> > dev_dbg(xdev->dev, "skipping DMA port %pOF:%u\n",
> > to_of_node(link.local_node),
> > link.local_port);
> > @@ -367,20 +372,25 @@ static int xvip_graph_parse_one(struct xvip_composite_device *xdev,
> > dev_dbg(xdev->dev, "parsing node %pOF\n", node);
> >
> > while (1) {
> > + struct fwnode_handle *fwnode;
> > +
> > ep = of_graph_get_next_endpoint(node, ep);
> > if (ep == NULL)
> > break;
> >
> > dev_dbg(xdev->dev, "handling endpoint %pOF\n", ep);
> >
> > - remote = of_graph_get_remote_port_parent(ep);
> > + remote = of_graph_get_remote_endpoint(ep);
> > if (remote == NULL) {
> > ret = -EINVAL;
> > break;
> > }
> >
> > + fwnode = fwnode_graph_get_port_parent(of_fwnode_handle(remote));
> > + fwnode_handle_put(fwnode);
> > +
> > /* Skip entities that we have already processed. */
> > - if (remote == xdev->dev->of_node ||
> > + if (fwnode == xdev->dev->of_node ||
> > xvip_graph_find_entity(xdev, remote)) {
> > of_node_put(remote);
> > continue;
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index e5acfab470a5..f53eff07e8b8 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -539,8 +539,12 @@ 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);
> > + if (!sd->fwnode)
> > + sd->fwnode = dev_fwnode(sd->dev);
> > + }
>
> I'm facing these days issues with endpoint matching and imo assuming
> the first available endpoint is the one the subdevice should be
> matched on is a limitation that will bite us back in future.
>
> What I'm dealing with is a device with 5 'ports' node, and a single
> endpoint per 'ports'. Your patch here takes the first available
> endpoint with 'fwnode_graph_get_next_endpoint()', and unfortunately
> that's not the one the notifier expects to match with.
>
> The only way around it that I have found is set explicitly the
> fwnode before registering the subdevice and having the framework
> make assumptions like this (match on the first available endpoint) may
> actually hide subtle issues difficult to debug.
Yes, that's the intention: you can override the default behaviour by
setting the fwnode if you need something else.
>
> Should we force subdevices to set their fwnode explicitly before
> registering the subdevice or at least WARN() when they don't?
> Converting existing sensor drivers to do so, should be as easy as
> converting platform drivers to match endpoints, as you did in this
> series.
I'm not sure it'll be worth it, as the vast majority of drivers don't
really need to care.
That's for now, that is. In the future we need to properly separate async
matching from parsing the endpoints to clean things up. I'd leave this
matter for later.
--
Regards,
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC 1/1] v4l: async: Use endpoint node, not device node, for fwnode match
2017-12-14 10:53 ` Sakari Ailus
@ 2017-12-14 11:44 ` jacopo mondi
2017-12-14 13:45 ` Sakari Ailus
0 siblings, 1 reply; 11+ messages in thread
From: jacopo mondi @ 2017-12-14 11:44 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, niklas.soderlund, kieran bingham
Hi Sakari,
On Thu, Dec 14, 2017 at 12:53:42PM +0200, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Thu, Dec 07, 2017 at 03:29:40PM +0100, jacopo mondi wrote:
> > Hi Sakari!
> > thanks for proposing this
> >
> > While we all agree that full endpoint matching is the right
> > thing to do (see also Kieran's last reply to his "v4l2-async: Match
> > parent devices" patch) I have some perplexity on this proposal,
> > please see below
> >
> > On Mon, Dec 04, 2017 at 11:03:02PM +0200, 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.
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > > Hi Niklas,
> > >
> > > What do you think of this one? I've tested this on N9, both sensor and
> > > flash devices work nicely there. No opportunistic checks for backwards
> > > compatibility are needed.
> > >
> > > The changes were surprisingly simple, there are only two drivers that
> > > weren't entirely trivial to change (this part is truly weird in exynos4-is
> > > and xilinx-vipp). Converting the two to use the common parsing functions
> > > would be quite a bit more work and would be very nice to test. The changes
> > > in this patch were still relatively simple.
> > >
> > > 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/davinci/vpif_capture.c | 2 +-
> > > drivers/media/platform/exynos4-is/media-dev.c | 14 ++++++++++----
> > > drivers/media/platform/pxa_camera.c | 2 +-
> > > drivers/media/platform/qcom/camss-8x16/camss.c | 2 +-
> > > drivers/media/platform/rcar_drif.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 | 16 +++++++++++++---
> > > drivers/media/v4l2-core/v4l2-async.c | 8 ++++++--
> > > drivers/media/v4l2-core/v4l2-fwnode.c | 2 +-
> > > 13 files changed, 39 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> > > index 0997c640191d..892d9e935d25 100644
> > > --- a/drivers/media/platform/am437x/am437x-vpfe.c
> > > +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> > > @@ -2493,7 +2493,7 @@ vpfe_get_pdata(struct platform_device *pdev)
> > > 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(&pdev->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 13f1c1c797b0..c8bb60eeb629 100644
> > > --- a/drivers/media/platform/atmel/atmel-isc.c
> > > +++ b/drivers/media/platform/atmel/atmel-isc.c
> > > @@ -2044,7 +2044,7 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
> > > if (!epn)
> > > break;
> > >
> > > - 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 e900995143a3..eafdf91a4541 100644
> > > --- a/drivers/media/platform/atmel/atmel-isi.c
> > > +++ b/drivers/media/platform/atmel/atmel-isi.c
> > > @@ -1119,7 +1119,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);
> > > if (!remote) {
> > > of_node_put(ep);
> > > return -EINVAL;
> > > diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> > > index fca4dc829f73..7c9c2b2bb710 100644
> > > --- a/drivers/media/platform/davinci/vpif_capture.c
> > > +++ b/drivers/media/platform/davinci/vpif_capture.c
> > > @@ -1572,7 +1572,7 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> > > if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> > > chan->vpif_if.vd_pol = 1;
> > >
> > > - rem = of_graph_get_remote_port_parent(endpoint);
> > > + rem = of_graph_get_remote_endpoint(endpoint);
> > > if (!rem) {
> > > dev_dbg(&pdev->dev, "Remote device at %pOF not found\n",
> > > endpoint);
> > > diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> > > index 0ef583cfc424..ab5dfe6d7ac4 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",
> > > @@ -1363,11 +1363,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.fwnode ==
> > > - of_fwnode_handle(subdev->dev->of_node))
> > > + for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++) {
> > > + struct fwnode_handle *fwnode =
> > > + fwnode_graph_get_port_parent(
> > > + of_fwnode_handle(subdev->dev->of_node));
> > > +
> > > + if (fmd->sensor[i].asd.match.fwnode.fwnode == fwnode)
> > > 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 4e0839829e6e..82aaafd113d4 100644
> > > --- a/drivers/media/platform/pxa_camera.c
> > > +++ b/drivers/media/platform/pxa_camera.c
> > > @@ -2334,7 +2334,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.fwnode = of_fwnode_handle(remote);
> > > of_node_put(remote);
> > > diff --git a/drivers/media/platform/qcom/camss-8x16/camss.c b/drivers/media/platform/qcom/camss-8x16/camss.c
> > > index 390a42c17b66..73cac6301756 100644
> > > --- a/drivers/media/platform/qcom/camss-8x16/camss.c
> > > +++ b/drivers/media/platform/qcom/camss-8x16/camss.c
> > > @@ -332,7 +332,7 @@ static int camss_of_parse_ports(struct device *dev,
> > > return ret;
> > > }
> > >
> > > - remote = of_graph_get_remote_port_parent(node);
> > > + remote = of_graph_get_remote_endpoint(node);
> > > of_node_put(node);
> > >
> > > if (!remote) {
> > > diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
> > > index 63c94f4028a7..f6e0a08d72f4 100644
> > > --- a/drivers/media/platform/rcar_drif.c
> > > +++ b/drivers/media/platform/rcar_drif.c
> > > @@ -1228,7 +1228,7 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr)
> > > return 0;
> > >
> > > notifier->subdevs[notifier->num_subdevs] = &sdr->ep.asd;
> > > - 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/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> > > index ac4c450a6c7d..18e0aa8af3b3 100644
> > > --- a/drivers/media/platform/stm32/stm32-dcmi.c
> > > +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> > > @@ -1511,7 +1511,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);
> > > if (!remote) {
> > > of_node_put(ep);
> > > return -EINVAL;
> > > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> > > index a1748b84deea..f4af6c5a7c6c 100644
> > > --- a/drivers/media/platform/ti-vpe/cal.c
> > > +++ b/drivers/media/platform/ti-vpe/cal.c
> > > @@ -1697,7 +1697,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 d881cf09876d..17d4ac0a908d 100644
> > > --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> > > +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> > > @@ -82,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. */
> > > next = of_graph_get_next_endpoint(entity->node, ep);
> > > if (next == NULL)
> > > @@ -121,8 +123,11 @@ 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 == of_fwnode_handle(xdev->dev->of_node)) {
> > > dev_dbg(xdev->dev, "skipping DMA port %pOF:%u\n",
> > > to_of_node(link.local_node),
> > > link.local_port);
> > > @@ -367,20 +372,25 @@ static int xvip_graph_parse_one(struct xvip_composite_device *xdev,
> > > dev_dbg(xdev->dev, "parsing node %pOF\n", node);
> > >
> > > while (1) {
> > > + struct fwnode_handle *fwnode;
> > > +
> > > ep = of_graph_get_next_endpoint(node, ep);
> > > if (ep == NULL)
> > > break;
> > >
> > > dev_dbg(xdev->dev, "handling endpoint %pOF\n", ep);
> > >
> > > - remote = of_graph_get_remote_port_parent(ep);
> > > + remote = of_graph_get_remote_endpoint(ep);
> > > if (remote == NULL) {
> > > ret = -EINVAL;
> > > break;
> > > }
> > >
> > > + fwnode = fwnode_graph_get_port_parent(of_fwnode_handle(remote));
> > > + fwnode_handle_put(fwnode);
> > > +
> > > /* Skip entities that we have already processed. */
> > > - if (remote == xdev->dev->of_node ||
> > > + if (fwnode == xdev->dev->of_node ||
> > > xvip_graph_find_entity(xdev, remote)) {
> > > of_node_put(remote);
> > > continue;
> > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > > index e5acfab470a5..f53eff07e8b8 100644
> > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > @@ -539,8 +539,12 @@ 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.
> > > */
Some more comments on this:
> > > - 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);
> > > + if (!sd->fwnode)
> > > + sd->fwnode = dev_fwnode(sd->dev);
> > > + }
If we want default behaviour to be "pick the first available endpoint"
are we sure we want to introduce another arbitrary default as "if no
first endpoint available, pick the parent device"?
I feel like if first endpoint is not available, it means that the
subdevice should have set its sd->fwnode explicitly, so I would return
error here.
Also, if "sd->fwnode" is not set and "sd->dev" isn't either the
function continues without any "sd->fwnode" set.
What about:
------------------------------------------------------------------
if (!sd->dev)
return -EINVAL;
if (!sd->fwnode) {
sd->fwnode = fwnode_graph_get_next_endpoint(
dev_fwnode(sd->dev), NULL);
if (!sd->fwnode)
return -ENOENT;
}
------------------------------------------------------------------
> >
> >
> > Should we force subdevices to set their fwnode explicitly before
> > registering the subdevice or at least WARN() when they don't?
> > Converting existing sensor drivers to do so, should be as easy as
> > converting platform drivers to match endpoints, as you did in this
> > series.
>
> I'm not sure it'll be worth it, as the vast majority of drivers don't
> really need to care.
>
> That's for now, that is. In the future we need to properly separate async
> matching from parsing the endpoints to clean things up. I'd leave this
> matter for later.
I partially take it back: moving code from framework to driver makes
no sense. I would just point out (in function prototype comment?) that
"v4l2_async_register_subdev()" assumes the subdevice has to be matched
on its first available endpoint. If that's not how your subdev works,
then sd->fwnode should be set explicitly.
Niklas had a point here though: having subdevices with custom matching
policies would break interoperability between components not
originally designed to work together... But maybe that will just be
for a transition phase until we don't have something more "evolved"?
>
> --
> Regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC 1/1] v4l: async: Use endpoint node, not device node, for fwnode match
2017-12-14 11:44 ` jacopo mondi
@ 2017-12-14 13:45 ` Sakari Ailus
0 siblings, 0 replies; 11+ messages in thread
From: Sakari Ailus @ 2017-12-14 13:45 UTC (permalink / raw)
To: jacopo mondi; +Cc: Sakari Ailus, linux-media, niklas.soderlund, kieran bingham
Hi Jacopo,
On Thu, Dec 14, 2017 at 12:44:38PM +0100, jacopo mondi wrote:
> Hi Sakari,
>
> On Thu, Dec 14, 2017 at 12:53:42PM +0200, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > On Thu, Dec 07, 2017 at 03:29:40PM +0100, jacopo mondi wrote:
> > > Hi Sakari!
> > > thanks for proposing this
> > >
> > > While we all agree that full endpoint matching is the right
> > > thing to do (see also Kieran's last reply to his "v4l2-async: Match
> > > parent devices" patch) I have some perplexity on this proposal,
> > > please see below
> > >
> > > On Mon, Dec 04, 2017 at 11:03:02PM +0200, 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.
> > > >
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > > Hi Niklas,
> > > >
> > > > What do you think of this one? I've tested this on N9, both sensor and
> > > > flash devices work nicely there. No opportunistic checks for backwards
> > > > compatibility are needed.
> > > >
> > > > The changes were surprisingly simple, there are only two drivers that
> > > > weren't entirely trivial to change (this part is truly weird in exynos4-is
> > > > and xilinx-vipp). Converting the two to use the common parsing functions
> > > > would be quite a bit more work and would be very nice to test. The changes
> > > > in this patch were still relatively simple.
> > > >
> > > > 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/davinci/vpif_capture.c | 2 +-
> > > > drivers/media/platform/exynos4-is/media-dev.c | 14 ++++++++++----
> > > > drivers/media/platform/pxa_camera.c | 2 +-
> > > > drivers/media/platform/qcom/camss-8x16/camss.c | 2 +-
> > > > drivers/media/platform/rcar_drif.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 | 16 +++++++++++++---
> > > > drivers/media/v4l2-core/v4l2-async.c | 8 ++++++--
> > > > drivers/media/v4l2-core/v4l2-fwnode.c | 2 +-
> > > > 13 files changed, 39 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/am437x/am437x-vpfe.c b/drivers/media/platform/am437x/am437x-vpfe.c
> > > > index 0997c640191d..892d9e935d25 100644
> > > > --- a/drivers/media/platform/am437x/am437x-vpfe.c
> > > > +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> > > > @@ -2493,7 +2493,7 @@ vpfe_get_pdata(struct platform_device *pdev)
> > > > 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(&pdev->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 13f1c1c797b0..c8bb60eeb629 100644
> > > > --- a/drivers/media/platform/atmel/atmel-isc.c
> > > > +++ b/drivers/media/platform/atmel/atmel-isc.c
> > > > @@ -2044,7 +2044,7 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
> > > > if (!epn)
> > > > break;
> > > >
> > > > - 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 e900995143a3..eafdf91a4541 100644
> > > > --- a/drivers/media/platform/atmel/atmel-isi.c
> > > > +++ b/drivers/media/platform/atmel/atmel-isi.c
> > > > @@ -1119,7 +1119,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);
> > > > if (!remote) {
> > > > of_node_put(ep);
> > > > return -EINVAL;
> > > > diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> > > > index fca4dc829f73..7c9c2b2bb710 100644
> > > > --- a/drivers/media/platform/davinci/vpif_capture.c
> > > > +++ b/drivers/media/platform/davinci/vpif_capture.c
> > > > @@ -1572,7 +1572,7 @@ vpif_capture_get_pdata(struct platform_device *pdev)
> > > > if (flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)
> > > > chan->vpif_if.vd_pol = 1;
> > > >
> > > > - rem = of_graph_get_remote_port_parent(endpoint);
> > > > + rem = of_graph_get_remote_endpoint(endpoint);
> > > > if (!rem) {
> > > > dev_dbg(&pdev->dev, "Remote device at %pOF not found\n",
> > > > endpoint);
> > > > diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
> > > > index 0ef583cfc424..ab5dfe6d7ac4 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",
> > > > @@ -1363,11 +1363,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.fwnode ==
> > > > - of_fwnode_handle(subdev->dev->of_node))
> > > > + for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++) {
> > > > + struct fwnode_handle *fwnode =
> > > > + fwnode_graph_get_port_parent(
> > > > + of_fwnode_handle(subdev->dev->of_node));
> > > > +
> > > > + if (fmd->sensor[i].asd.match.fwnode.fwnode == fwnode)
> > > > 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 4e0839829e6e..82aaafd113d4 100644
> > > > --- a/drivers/media/platform/pxa_camera.c
> > > > +++ b/drivers/media/platform/pxa_camera.c
> > > > @@ -2334,7 +2334,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.fwnode = of_fwnode_handle(remote);
> > > > of_node_put(remote);
> > > > diff --git a/drivers/media/platform/qcom/camss-8x16/camss.c b/drivers/media/platform/qcom/camss-8x16/camss.c
> > > > index 390a42c17b66..73cac6301756 100644
> > > > --- a/drivers/media/platform/qcom/camss-8x16/camss.c
> > > > +++ b/drivers/media/platform/qcom/camss-8x16/camss.c
> > > > @@ -332,7 +332,7 @@ static int camss_of_parse_ports(struct device *dev,
> > > > return ret;
> > > > }
> > > >
> > > > - remote = of_graph_get_remote_port_parent(node);
> > > > + remote = of_graph_get_remote_endpoint(node);
> > > > of_node_put(node);
> > > >
> > > > if (!remote) {
> > > > diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
> > > > index 63c94f4028a7..f6e0a08d72f4 100644
> > > > --- a/drivers/media/platform/rcar_drif.c
> > > > +++ b/drivers/media/platform/rcar_drif.c
> > > > @@ -1228,7 +1228,7 @@ static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr)
> > > > return 0;
> > > >
> > > > notifier->subdevs[notifier->num_subdevs] = &sdr->ep.asd;
> > > > - 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/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> > > > index ac4c450a6c7d..18e0aa8af3b3 100644
> > > > --- a/drivers/media/platform/stm32/stm32-dcmi.c
> > > > +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> > > > @@ -1511,7 +1511,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);
> > > > if (!remote) {
> > > > of_node_put(ep);
> > > > return -EINVAL;
> > > > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> > > > index a1748b84deea..f4af6c5a7c6c 100644
> > > > --- a/drivers/media/platform/ti-vpe/cal.c
> > > > +++ b/drivers/media/platform/ti-vpe/cal.c
> > > > @@ -1697,7 +1697,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 d881cf09876d..17d4ac0a908d 100644
> > > > --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> > > > +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> > > > @@ -82,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. */
> > > > next = of_graph_get_next_endpoint(entity->node, ep);
> > > > if (next == NULL)
> > > > @@ -121,8 +123,11 @@ 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 == of_fwnode_handle(xdev->dev->of_node)) {
> > > > dev_dbg(xdev->dev, "skipping DMA port %pOF:%u\n",
> > > > to_of_node(link.local_node),
> > > > link.local_port);
> > > > @@ -367,20 +372,25 @@ static int xvip_graph_parse_one(struct xvip_composite_device *xdev,
> > > > dev_dbg(xdev->dev, "parsing node %pOF\n", node);
> > > >
> > > > while (1) {
> > > > + struct fwnode_handle *fwnode;
> > > > +
> > > > ep = of_graph_get_next_endpoint(node, ep);
> > > > if (ep == NULL)
> > > > break;
> > > >
> > > > dev_dbg(xdev->dev, "handling endpoint %pOF\n", ep);
> > > >
> > > > - remote = of_graph_get_remote_port_parent(ep);
> > > > + remote = of_graph_get_remote_endpoint(ep);
> > > > if (remote == NULL) {
> > > > ret = -EINVAL;
> > > > break;
> > > > }
> > > >
> > > > + fwnode = fwnode_graph_get_port_parent(of_fwnode_handle(remote));
> > > > + fwnode_handle_put(fwnode);
> > > > +
> > > > /* Skip entities that we have already processed. */
> > > > - if (remote == xdev->dev->of_node ||
> > > > + if (fwnode == xdev->dev->of_node ||
> > > > xvip_graph_find_entity(xdev, remote)) {
> > > > of_node_put(remote);
> > > > continue;
> > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > > > index e5acfab470a5..f53eff07e8b8 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > > @@ -539,8 +539,12 @@ 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.
> > > > */
> Some more comments on this:
> > > > - 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);
> > > > + if (!sd->fwnode)
> > > > + sd->fwnode = dev_fwnode(sd->dev);
> > > > + }
>
> If we want default behaviour to be "pick the first available endpoint"
> are we sure we want to introduce another arbitrary default as "if no
> first endpoint available, pick the parent device"?
For lens and some flash devices this is actually what's needed. I wouldn't
say it's arbitrary. These devices don't have any endpoint nodes.
I do not object making these decisions explicitly in drivers either in
principle. The drivers may need extra error handling code to handle
situations where no endpoints are not found, so this could get somewhat
hairy. We'll see once someone submits a patch. :-)
>
> I feel like if first endpoint is not available, it means that the
> subdevice should have set its sd->fwnode explicitly, so I would return
> error here.
>
> Also, if "sd->fwnode" is not set and "sd->dev" isn't either the
> function continues without any "sd->fwnode" set.
>
> What about:
>
> ------------------------------------------------------------------
> if (!sd->dev)
> return -EINVAL;
>
> if (!sd->fwnode) {
> sd->fwnode = fwnode_graph_get_next_endpoint(
> dev_fwnode(sd->dev), NULL);
> if (!sd->fwnode)
> return -ENOENT;
> }
> ------------------------------------------------------------------
>
> > >
> > >
> > > Should we force subdevices to set their fwnode explicitly before
> > > registering the subdevice or at least WARN() when they don't?
> > > Converting existing sensor drivers to do so, should be as easy as
> > > converting platform drivers to match endpoints, as you did in this
> > > series.
> >
> > I'm not sure it'll be worth it, as the vast majority of drivers don't
> > really need to care.
> >
> > That's for now, that is. In the future we need to properly separate async
> > matching from parsing the endpoints to clean things up. I'd leave this
> > matter for later.
>
> I partially take it back: moving code from framework to driver makes
> no sense. I would just point out (in function prototype comment?) that
> "v4l2_async_register_subdev()" assumes the subdevice has to be matched
> on its first available endpoint. If that's not how your subdev works,
> then sd->fwnode should be set explicitly.
Yes.
>
> Niklas had a point here though: having subdevices with custom matching
> policies would break interoperability between components not
> originally designed to work together... But maybe that will just be
> for a transition phase until we don't have something more "evolved"?
Do we need something more elaborate than this?
With this patch, the endpoint nodes will be used for matching when the device
has endpoint nodes.
If there are no device nodes, then it's up to the driver to assign the
correct fwnodes for matching to the async sub-devices it registers --- this
is defined in DT bindings. The notifier driver gets these directly from DT.
This could be reflected in comments btw. to document why the code does what
it does.
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC 1/1] v4l: async: Use endpoint node, not device node, for fwnode match
2017-12-07 7:39 ` Sakari Ailus
@ 2017-12-19 9:12 ` Niklas Söderlund
2017-12-29 11:17 ` Sakari Ailus
0 siblings, 1 reply; 11+ messages in thread
From: Niklas Söderlund @ 2017-12-19 9:12 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, jmondi, Kieran Bingham
Hej Sakari,
Ledsen för sent svar.
On 2017-12-07 09:39:51 +0200, Sakari Ailus wrote:
> Hej Niklas,
>
> Tack för dina kommentarer!
>
> On Wed, Dec 06, 2017 at 04:57:48PM +0100, Niklas Söderlund wrote:
> > CC Jacopo, Kieran
> >
> > Hi Sakari,
> >
> > Thanks for your patch.
> >
> > On 2017-12-04 23:03:02 +0200, 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.
> >
> > Endpoint matching I think is the way to go forward. It will solve a set
> > of problems that exists today. So I think this a good step in the right
> > direction.
> >
> > >
> > > 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.
> >
> > This assumption is not true, the adv748x exposes multiple subdevice from
> > a single device node in DT and registers them using different endpoints.
> > Now the only user of the adv748x driver I know of is the rcar-csi2
> > driver which is not yet upstream so this can be consider a special case.
>
> Right, the adv748x is an exception to this but I could argue it should
> never have been merged in its current state: it does not work with other
> bridge / ISP drivers.
Agreed, the issue of endpoint matching should have been solved before.
As the adv748x driver poses yet another dimension of this issue,
multiple endpoints decribed in DT which should be matched to different
subdevices registerd from the same driver.
I think this is a use-case we should consider when looking at this, what
do you think?
>
> >
> > Unfortunately this patch do break already existing configurations
> > upstream :-( For example the Koelsch board, from r8a7791-koelsch.dts:
> >
> > hdmi-in {
> > compatible = "hdmi-connector";
> > type = "a";
> >
> > port {
> > hdmi_con_in: endpoint {
> > remote-endpoint = <&adv7612_in>;
> > };
> > };
> > };
> >
> > hdmi-in@4c {
> > compatible = "adi,adv7612";
> > reg = <0x4c>;
> > interrupt-parent = <&gpio4>;
> > interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
> > default-input = <0>;
> >
> > 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>;
> > };
> > };
> > };
> > };
> >
> > &vin0 {
> > status = "okay";
> > pinctrl-0 = <&vin0_pins>;
> > pinctrl-names = "default";
> >
> > port {
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > vin0ep2: endpoint {
> > remote-endpoint = <&adv7612_out>;
> > bus-width = <24>;
> > hsync-active = <0>;
> > vsync-active = <0>;
> > pclk-sample = <1>;
> > data-active = <1>;
> > };
> > };
> > };
> >
> > Here the adv7612 driver would register a subdevice using the endpoint
> > 'hdmi-in@4c/ports/port@0/endpoint' while the rcar-vin driver which uses
>
> The adv7612 needs to register both of these endpoints. I wonder if there
> are repercussions by doing that.
I fear so too.
I don't think there is any support in the framwrk today to register the
same subdevice twice using different fwnodes for matching, is there? If
not this is something which we need to add.
We also need to make sure from a framwrok point of view that its
possible for sub-devices that registers itself using two different
endpoints could be bound to two different notifiers from two different
remote drivers, right? I'm not sure what the state in the the framework
is for this, do you know?
Is there anything else we need to check in the framwork before trying to
make this change? Maybe we can add a helper much like the notifer
helpers you posted but for sub-device drivers, which would be called
from v4l2_async_register_subdev() to parse and register all endpoints
with the async framework together with the device fwnode. Then the
subdevice would work with notifiers who register both endpoints (using
your helpers and this patch) and lens/flash devices as they are a
special case you describe bellow. What do you think of such a solution?
>
> > the async parsing helpers would register a notifier looking for
> > 'hdmi-in@4c/ports/port@2/endpoint'.
> >
> > Something like Kieran's '[PATCH v5] v4l2-async: Match parent devices'
> > would be needed in addition to this patch. I tried Kieran's patch
> > together with this and it did not solve the problem upstream. I did make
>
> The more I've been working on this problem, the less I think
> opportunistically matching devices or endpoints is a good idea. Lens
> devices will always use device nodes and flash devices use LED nodes found
> under device nodes.
Then we need someway to inform the sub-notifer parsing helpers if it
should register using endpoints or device nodes. When the framework
parse for lens/flash devices it would register them with the notifier
using device nodes, but if the helper is called directly from a driver
it would register them using endpoint nodes. Will this cover all the
scenarios?
>
> It's getting messy with opportunistic matching. And as this patch shows,
> it's not that hard to convert all drivers either, so why not to do just
> that?
Yes, I think we all come to the agreement that opportunistic matching is
not the way to go here :-)
>
> --
> Hälsningar,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com
--
Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC 1/1] v4l: async: Use endpoint node, not device node, for fwnode match
2017-12-19 9:12 ` Niklas Söderlund
@ 2017-12-29 11:17 ` Sakari Ailus
0 siblings, 0 replies; 11+ messages in thread
From: Sakari Ailus @ 2017-12-29 11:17 UTC (permalink / raw)
To: Niklas Söderlund; +Cc: linux-media, jmondi, Kieran Bingham
Hejssan, Niklas!
On Tue, Dec 19, 2017 at 10:12:43AM +0100, Niklas Söderlund wrote:
> Hej Sakari,
>
> Ledsen för sent svar.
Detsamma, och det gör ingenting. :-)
>
> On 2017-12-07 09:39:51 +0200, Sakari Ailus wrote:
> > Hej Niklas,
> >
> > Tack för dina kommentarer!
> >
> > On Wed, Dec 06, 2017 at 04:57:48PM +0100, Niklas Söderlund wrote:
> > > CC Jacopo, Kieran
> > >
> > > Hi Sakari,
> > >
> > > Thanks for your patch.
> > >
> > > On 2017-12-04 23:03:02 +0200, 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.
> > >
> > > Endpoint matching I think is the way to go forward. It will solve a set
> > > of problems that exists today. So I think this a good step in the right
> > > direction.
> > >
> > > >
> > > > 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.
> > >
> > > This assumption is not true, the adv748x exposes multiple subdevice from
> > > a single device node in DT and registers them using different endpoints.
> > > Now the only user of the adv748x driver I know of is the rcar-csi2
> > > driver which is not yet upstream so this can be consider a special case.
> >
> > Right, the adv748x is an exception to this but I could argue it should
> > never have been merged in its current state: it does not work with other
> > bridge / ISP drivers.
>
> Agreed, the issue of endpoint matching should have been solved before.
> As the adv748x driver poses yet another dimension of this issue,
> multiple endpoints decribed in DT which should be matched to different
> subdevices registerd from the same driver.
>
> I think this is a use-case we should consider when looking at this, what
> do you think?
If you have a different sub-device (and therefore async sub-device), I
guess that should work fine with just endpoint matching added.
You're still limited to a single media device though.
>
> >
> > >
> > > Unfortunately this patch do break already existing configurations
> > > upstream :-( For example the Koelsch board, from r8a7791-koelsch.dts:
> > >
> > > hdmi-in {
> > > compatible = "hdmi-connector";
> > > type = "a";
> > >
> > > port {
> > > hdmi_con_in: endpoint {
> > > remote-endpoint = <&adv7612_in>;
> > > };
> > > };
> > > };
> > >
> > > hdmi-in@4c {
> > > compatible = "adi,adv7612";
> > > reg = <0x4c>;
> > > interrupt-parent = <&gpio4>;
> > > interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
> > > default-input = <0>;
> > >
> > > 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>;
> > > };
> > > };
> > > };
> > > };
> > >
> > > &vin0 {
> > > status = "okay";
> > > pinctrl-0 = <&vin0_pins>;
> > > pinctrl-names = "default";
> > >
> > > port {
> > > #address-cells = <1>;
> > > #size-cells = <0>;
> > >
> > > vin0ep2: endpoint {
> > > remote-endpoint = <&adv7612_out>;
> > > bus-width = <24>;
> > > hsync-active = <0>;
> > > vsync-active = <0>;
> > > pclk-sample = <1>;
> > > data-active = <1>;
> > > };
> > > };
> > > };
> > >
> > > Here the adv7612 driver would register a subdevice using the endpoint
> > > 'hdmi-in@4c/ports/port@0/endpoint' while the rcar-vin driver which uses
> >
> > The adv7612 needs to register both of these endpoints. I wonder if there
> > are repercussions by doing that.
>
> I fear so too.
>
> I don't think there is any support in the framwrk today to register the
> same subdevice twice using different fwnodes for matching, is there? If
> not this is something which we need to add.
No. For that, I believe we'll need to somehow separate registering async
sub-devices and the notifier's bound callback. Perhaps async sub-devices
could be separated from the sub-device so that multiple async sub-devices
could be related to a single sub-device? And a list of bound and unbound
async sub-devices could be maintained in a sub-device.
>
> We also need to make sure from a framwrok point of view that its
> possible for sub-devices that registers itself using two different
> endpoints could be bound to two different notifiers from two different
> remote drivers, right? I'm not sure what the state in the the framework
> is for this, do you know?
Changes will be needed, that's for sure.
>
> Is there anything else we need to check in the framwork before trying to
> make this change? Maybe we can add a helper much like the notifer
> helpers you posted but for sub-device drivers, which would be called
> from v4l2_async_register_subdev() to parse and register all endpoints
> with the async framework together with the device fwnode. Then the
> subdevice would work with notifiers who register both endpoints (using
> your helpers and this patch) and lens/flash devices as they are a
> special case you describe bellow. What do you think of such a solution?
I'm not sure that'll need to be changed. We'll see. I'd start from allowing
more than one async sub-device per sub-device.
>
> >
> > > the async parsing helpers would register a notifier looking for
> > > 'hdmi-in@4c/ports/port@2/endpoint'.
> > >
> > > Something like Kieran's '[PATCH v5] v4l2-async: Match parent devices'
> > > would be needed in addition to this patch. I tried Kieran's patch
> > > together with this and it did not solve the problem upstream. I did make
> >
> > The more I've been working on this problem, the less I think
> > opportunistically matching devices or endpoints is a good idea. Lens
> > devices will always use device nodes and flash devices use LED nodes found
> > under device nodes.
>
> Then we need someway to inform the sub-notifer parsing helpers if it
> should register using endpoints or device nodes. When the framework
> parse for lens/flash devices it would register them with the notifier
> using device nodes, but if the helper is called directly from a driver
> it would register them using endpoint nodes. Will this cover all the
> scenarios?
That should work for all current devices. If you need something else than
the default, then the driver must assign the fwnode.
--
Hälsningar,
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-12-29 11:17 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-04 21:03 [RFC 1/1] v4l: async: Use endpoint node, not device node, for fwnode match Sakari Ailus
2017-12-06 10:43 ` Kieran Bingham
2017-12-06 15:57 ` Niklas Söderlund
2017-12-06 16:36 ` Kieran Bingham
2017-12-07 7:39 ` Sakari Ailus
2017-12-19 9:12 ` Niklas Söderlund
2017-12-29 11:17 ` Sakari Ailus
2017-12-07 14:29 ` jacopo mondi
2017-12-14 10:53 ` Sakari Ailus
2017-12-14 11:44 ` jacopo mondi
2017-12-14 13:45 ` Sakari Ailus
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.