v4l: async: Use endpoint node, not device node, for fwnode match
Subsystems (11)
Sakari Ailus
maintainerRFC 1/1 v4l: async: Use endpoint node, not device node, for fwnode match
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.
---
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(-)
drivers/media/platform/am437x/am437x-vpfe.c +1 -1
@@ -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);
drivers/media/platform/atmel/atmel-isc.c +1 -1
@@ -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);
drivers/media/platform/atmel/atmel-isi.c +1 -1
@@ -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 ;
drivers/media/platform/davinci/vpif_capture.c +1 -1
@@ -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);
drivers/media/platform/exynos4-is/media-dev.c +10 -4
@@ -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 ;
drivers/media/platform/pxa_camera.c +1 -1
@@ -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);
drivers/media/platform/qcom/camss-8x16/camss.c +1 -1
@@ -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) {
drivers/media/platform/rcar_drif.c +1 -1
@@ -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);
drivers/media/platform/stm32/stm32-dcmi.c +1 -1
@@ -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 ;
drivers/media/platform/ti-vpe/cal.c +1 -1
@@ -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;
drivers/media/platform/xilinx/xilinx-vipp.c +13 -3
@@ -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 ;
drivers/media/v4l2-core/v4l2-async.c +6 -2
@@ -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);
drivers/media/v4l2-core/v4l2-fwnode.c +1 -1
@@ -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 ;
Signature · 1 line
2.11.0
Hi Sakari,
Thanks for the patch.
Quoted 10 lines · Sakari Ailus
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.
Quoted 4 lines
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 <[email protected]>
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
Quoted 145 lines
---
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(-)drivers/media/platform/am437x/am437x-vpfe.c +1
-1@@ -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);drivers/media/platform/atmel/atmel-isc.c +1
-1@@ -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);drivers/media/platform/atmel/atmel-isi.c +1
-1@@ -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 ;drivers/media/platform/davinci/vpif_capture.c +1
-1@@ -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);drivers/media/platform/exynos4-is/media-dev.c +10
-4@@ -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 ;drivers/media/platform/pxa_camera.c +1
-1@@ -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);drivers/media/platform/qcom/camss-8x16/camss.c +1
-1@@ -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) {drivers/media/platform/rcar_drif.c +0
-0
drivers/media/platform/rcar_drif.c static int rcar_drif_parse_subdevs(struct rcar_drif_sdr *sdr) · line 1233
Quoted 7 lines
drivers/media/platform/rcar_drif.c +1
-1@@ -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/
Quoted 105 lines
drivers/media/platform/rcar_drif.c +0
-0fwnode_handle_put (ep);drivers/media/platform/stm32/stm32-dcmi.c +1
-1@@ -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 ;drivers/media/platform/ti-vpe/cal.c +1
-1@@ -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;drivers/media/platform/xilinx/xilinx-vipp.c +13
-3@@ -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 ;drivers/media/v4l2-core/v4l2-async.c +6
-2@@ -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);drivers/media/v4l2-core/v4l2-fwnode.c +0
-0
drivers/media/v4l2-core/v4l2-fwnode.c static int v4l2_async_notifier_fwnode_parse_endpoint( · line 365
Quoted 7 lines
drivers/media/v4l2-core/v4l2-fwnode.c +1
-1@@ -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/
Quoted 2 lines
drivers/media/v4l2-core/v4l2-fwnode.c +0
-0ret = -EINVAL ;
CC Jacopo, Kieran
Hi Sakari,
Thanks for your patch.
Quoted 4 lines · Sakari Ailus
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.
Quoted 5 lines
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.
Quoted 11 lines
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 <[email protected]>---
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.
Quoted 262 lines
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(-)drivers/media/platform/am437x/am437x-vpfe.c +1
-1@@ -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);drivers/media/platform/atmel/atmel-isc.c +1
-1@@ -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);drivers/media/platform/atmel/atmel-isi.c +1
-1@@ -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 ;drivers/media/platform/davinci/vpif_capture.c +1
-1@@ -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);drivers/media/platform/exynos4-is/media-dev.c +10
-4@@ -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 ;drivers/media/platform/pxa_camera.c +1
-1@@ -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);drivers/media/platform/qcom/camss-8x16/camss.c +1
-1@@ -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) {drivers/media/platform/rcar_drif.c +1
-1@@ -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);drivers/media/platform/stm32/stm32-dcmi.c +1
-1@@ -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 ;drivers/media/platform/ti-vpe/cal.c +1
-1@@ -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;drivers/media/platform/xilinx/xilinx-vipp.c +13
-3@@ -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 ;drivers/media/v4l2-core/v4l2-async.c +6
-2@@ -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);drivers/media/v4l2-core/v4l2-fwnode.c +1
-1@@ -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 ;Signature · 1 line
2.11.0
Signature · 2 lines
Regards, Niklas Söderlund
Hi Niklas,
Quoted 22 lines · Niklas Söderlund
CC Jacopo, Kieran
Hi Sakari,
Thanks for your patch.
Quoted 4 lines · Sakari Ailus
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.
Quoted 5 lines
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.
Quoted 4 lines
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>
Quoted 11 lines
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' inmatch_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
Sakari Ailus
maintainerHej Niklas,
Tack för dina kommentarer!
Quoted 22 lines · Niklas Söderlund
CC Jacopo, Kieran
Hi Sakari,
Thanks for your patch.
Quoted 4 lines · Sakari Ailus
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.
Quoted 5 lines
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.
Quoted 4 lines
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.
Quoted 62 lines
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>; }; }; };
Quoted 2 lines
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.
Quoted 6 lines
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?
Signature · 4 lines
Hälsningar, Sakari Ailus [email protected]
Hej Sakari,
Ledsen för sent svar.
Quoted 27 lines · Sakari Ailus
Hej Niklas,
Tack för dina kommentarer!
Quoted 22 lines · Niklas Söderlund
CC Jacopo, Kieran
Hi Sakari,
Thanks for your patch.
Quoted 4 lines · Sakari Ailus
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.
Quoted 5 lines
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.
Quoted 8 lines
Quoted 4 lines
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?
Quoted 63 lines
Quoted 63 lines
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>; }; }; };
Quoted 5 lines
Quoted 2 lines
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?
Quoted 4 lines
Quoted 4 lines
the async parsing helpers would register a notifier looking for 'hdmi-in@4c/ports/port@2/endpoint'.
Quoted 8 lines
Quoted 3 lines
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?
Quoted 4 lines
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 :-)
Quoted 6 lines
Signature · 5 lines
-- Hälsningar, Sakari Ailus [email protected]
Signature · 2 lines
Regards, Niklas Söderlund
Sakari Ailus
maintainerHejssan, Niklas!
Quoted 3 lines · Niklas Söderlund
Hej Sakari,
Ledsen för sent svar.
Detsamma, och det gör ingenting. :-)
Quoted 38 lines · Sakari Ailus
Quoted 27 lines
Hej Niklas,
Tack för dina kommentarer!
Quoted 22 lines · Niklas Söderlund
CC Jacopo, Kieran
Hi Sakari,
Thanks for your patch.
Quoted 4 lines · Sakari Ailus
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.
Quoted 5 lines
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.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 7 lines
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.
Quoted 67 lines
Quoted 67 lines
Quoted 66 lines
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
Quoted 8 lines
Quoted 2 lines
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.
Quoted 6 lines
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.
Quoted 9 lines
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.
Quoted 14 lines
Quoted 5 lines
Quoted 5 lines
the async parsing helpers would register a notifier looking for 'hdmi-in@4c/ports/port@2/endpoint'.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 6 lines
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.
Signature · 4 lines
Hälsningar, Sakari Ailus [email protected]
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
Quoted 253 lines · Sakari Ailus
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 <[email protected]>---
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(-)drivers/media/platform/am437x/am437x-vpfe.c +1
-1@@ -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);drivers/media/platform/atmel/atmel-isc.c +1
-1@@ -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);drivers/media/platform/atmel/atmel-isi.c +1
-1@@ -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 ;drivers/media/platform/davinci/vpif_capture.c +1
-1@@ -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);drivers/media/platform/exynos4-is/media-dev.c +10
-4@@ -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 ;drivers/media/platform/pxa_camera.c +1
-1@@ -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);drivers/media/platform/qcom/camss-8x16/camss.c +1
-1@@ -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) {drivers/media/platform/rcar_drif.c +1
-1@@ -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);drivers/media/platform/stm32/stm32-dcmi.c +1
-1@@ -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 ;drivers/media/platform/ti-vpe/cal.c +1
-1@@ -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;drivers/media/platform/xilinx/xilinx-vipp.c +13
-3@@ -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 ;drivers/media/v4l2-core/v4l2-async.c +0
-0@@ -539,8 +539,12 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)* (struct v4l2_subdev.dev), and async sub-device does not
drivers/media/v4l2-core/v4l2-async.c int v4l2_async_register_subdev(struct v4l2_subdev *sd) · line 547
Quoted 10 lines
drivers/media/v4l2-core/v4l2-async.c +6
-2* 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
Quoted 19 lines
drivers/media/v4l2-core/v4l2-async.c +0
-0mutex_lock (&list_lock);drivers/media/v4l2-core/v4l2-fwnode.c +1
-2@@ -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
Sakari Ailus
maintainerHi Jacopo,
Quoted 282 lines · jacopo mondi
Hi Sakari!
thanks for proposing thisWhile 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
Quoted 253 lines · Sakari Ailus
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 <[email protected]>---
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(-)drivers/media/platform/am437x/am437x-vpfe.c +1
-1@@ -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);drivers/media/platform/atmel/atmel-isc.c +1
-1@@ -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);drivers/media/platform/atmel/atmel-isi.c +1
-1@@ -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 ;drivers/media/platform/davinci/vpif_capture.c +1
-1@@ -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);drivers/media/platform/exynos4-is/media-dev.c +10
-4@@ -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 ;drivers/media/platform/pxa_camera.c +1
-1@@ -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);drivers/media/platform/qcom/camss-8x16/camss.c +1
-1@@ -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) {drivers/media/platform/rcar_drif.c +1
-1@@ -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);drivers/media/platform/stm32/stm32-dcmi.c +1
-1@@ -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 ;drivers/media/platform/ti-vpe/cal.c +1
-1@@ -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;drivers/media/platform/xilinx/xilinx-vipp.c +13
-3@@ -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 ;drivers/media/v4l2-core/v4l2-async.c +0
-0@@ -539,8 +539,12 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)* (struct v4l2_subdev.dev), and async sub-device does not drivers/media/v4l2-core/v4l2-async.c int v4l2_async_register_subdev(struct v4l2_subdev *sd) · line 547
Quoted 10 lines
drivers/media/v4l2-core/v4l2-async.c +6
-2* 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.
Quoted 4 lines
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.
Quoted 6 lines
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.
Signature · 4 lines
Regards, Sakari Ailus [email protected]
Hi Sakari,
Quoted 259 lines · Sakari Ailus
Hi Jacopo,
Quoted 256 lines · jacopo mondi
Hi Sakari!
thanks for proposing thisWhile 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
Quoted 247 lines · Sakari Ailus
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 <[email protected]>---
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(-)drivers/media/platform/am437x/am437x-vpfe.c +1
-1@@ -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);drivers/media/platform/atmel/atmel-isc.c +1
-1@@ -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);drivers/media/platform/atmel/atmel-isi.c +1
-1@@ -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 ;drivers/media/platform/davinci/vpif_capture.c +1
-1@@ -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);drivers/media/platform/exynos4-is/media-dev.c +10
-4@@ -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 ;drivers/media/platform/pxa_camera.c +1
-1@@ -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);drivers/media/platform/qcom/camss-8x16/camss.c +1
-1@@ -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) {drivers/media/platform/rcar_drif.c +1
-1@@ -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);drivers/media/platform/stm32/stm32-dcmi.c +1
-1@@ -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 ;drivers/media/platform/ti-vpe/cal.c +1
-1@@ -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;drivers/media/platform/xilinx/xilinx-vipp.c +13
-3@@ -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 ;
Quoted 8 lines
Quoted 8 lines
drivers/media/v4l2-core/v4l2-async.c int v4l2_async_register_subdev(struct v4l2_subdev *sd) · line 541
Quoted 8 lines
drivers/media/v4l2-core/v4l2-async.c +0
-0@@ -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:
Quoted 8 lines
Quoted 8 lines
drivers/media/v4l2-core/v4l2-async.c int v4l2_async_register_subdev(struct v4l2_subdev *sd) · line 547
Quoted 8 lines
drivers/media/v4l2-core/v4l2-async.c +6
-2-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;
}------------------------------------------------------------------
Quoted 8 lines
Quoted 7 lines
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.
Quoted 6 lines
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"?
Quoted 6 lines
--
Regards,Sakari Ailus
[email protected]
Hi Jacopo,
Quoted 271 lines · jacopo mondi
Hi Sakari,
Quoted 259 lines · Sakari Ailus
Hi Jacopo,
Quoted 256 lines · jacopo mondi
Hi Sakari!
thanks for proposing thisWhile 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
Quoted 247 lines · Sakari Ailus
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 <[email protected]>---
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(-)drivers/media/platform/am437x/am437x-vpfe.c +1
-1@@ -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);drivers/media/platform/atmel/atmel-isc.c +1
-1@@ -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);drivers/media/platform/atmel/atmel-isi.c +1
-1@@ -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 ;drivers/media/platform/davinci/vpif_capture.c +1
-1@@ -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);drivers/media/platform/exynos4-is/media-dev.c +10
-4@@ -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 ;drivers/media/platform/pxa_camera.c +1
-1@@ -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);drivers/media/platform/qcom/camss-8x16/camss.c +1
-1@@ -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) {drivers/media/platform/rcar_drif.c +1
-1@@ -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);drivers/media/platform/stm32/stm32-dcmi.c +1
-1@@ -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 ;drivers/media/platform/ti-vpe/cal.c +1
-1@@ -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;drivers/media/platform/xilinx/xilinx-vipp.c +13
-3@@ -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 ;Quoted 8 lines
Quoted 8 lines
drivers/media/v4l2-core/v4l2-async.c int v4l2_async_register_subdev(struct v4l2_subdev *sd) · line 541
Quoted 8 lines
drivers/media/v4l2-core/v4l2-async.c +0
-0@@ -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:
Quoted 12 lines
Quoted 8 lines
Quoted 8 lines
drivers/media/v4l2-core/v4l2-async.c int v4l2_async_register_subdev(struct v4l2_subdev *sd) · line 547
Quoted 8 lines
drivers/media/v4l2-core/v4l2-async.c +6
-2-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. :-)
Quoted 37 lines
I feel like if first endpoint is not available, it means that the subdevice should have set its
sd->fwnodeexplicitly, 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; }------------------------------------------------------------------
Quoted 8 lines
Quoted 7 lines
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.Quoted 6 lines
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.
Quoted 5 lines
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, thensd->fwnodeshould be set explicitly.
Yes.
Quoted 5 lines
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.
Signature · 4 lines
Regards, Sakari Ailus e-mail: [email protected]