Hi Rob, Helge
This is v3 patch-set
We already have for_each_endpoint_of_node(), but some drivers are
not using it. This patch-set replace it.
This patch-set is related to "OF" (= Rob), but many driveres are for
"MultiMedia" (= Helge). I'm not sure who handle these.
I noticed that my posted 1 patch on (A) was not yet included on
linus/master. I have included it.
[o] done
[*] this patch-set
[o] tidyup of_graph_get_endpoint_count()
(A) [o] replace endpoint func - use endpoint_by_regs()
[*] replace endpoint func - use for_each()
[ ] add new port function
[ ] add new endpoint function
v2 -> v3
- don't initialize pointer.
- add Reviewed-by / Acked-by
- include not-yet applied missing patch
v1 -> v2
- fixup TI patch
Link: https://lore.kernel.org/r/8734sf6mgn.wl-kuninori.morimoto.gx@renesas.com
Link: https://lore.kernel.org/r/87cyrauf0x.wl-kuninori.morimoto.gx@renesas.com
Kuninori Morimoto (9):
gpu: drm: replace of_graph_get_next_endpoint()
gpu: drm: use for_each_endpoint_of_node()
hwtracing: use for_each_endpoint_of_node()
media: platform: microchip: use for_each_endpoint_of_node()
media: platform: ti: use for_each_endpoint_of_node()
media: platform: xilinx: use for_each_endpoint_of_node()
staging: media: atmel: use for_each_endpoint_of_node()
video: fbdev: use for_each_endpoint_of_node()
fbdev: omapfb: use of_graph_get_remote_port()
drivers/gpu/drm/drm_of.c | 4 +++-
drivers/gpu/drm/omapdrm/dss/base.c | 3 +--
.../drm/panel/panel-raspberrypi-touchscreen.c | 2 +-
drivers/gpu/drm/tiny/arcpgu.c | 2 +-
.../hwtracing/coresight/coresight-platform.c | 4 ++--
.../microchip/microchip-sama5d2-isc.c | 21 +++++++------------
.../microchip/microchip-sama7g5-isc.c | 21 +++++++------------
.../media/platform/ti/am437x/am437x-vpfe.c | 12 +++++------
.../media/platform/ti/davinci/vpif_capture.c | 14 ++++++-------
drivers/media/platform/xilinx/xilinx-vipp.c | 9 ++------
.../deprecated/atmel/atmel-sama5d2-isc.c | 8 ++-----
.../deprecated/atmel/atmel-sama7g5-isc.c | 8 ++-----
drivers/video/fbdev/omap2/omapfb/dss/dss-of.c | 15 +------------
.../omap2/omapfb/dss/omapdss-boot-init.c | 3 +--
14 files changed, 44 insertions(+), 82 deletions(-)
--
2.43.0
From DT point of view, in general, drivers should be asking for a
specific port number because their function is fixed in the binding.
of_graph_get_next_endpoint() doesn't match to this concept.
Simply replace
- of_graph_get_next_endpoint(xxx, NULL);
+ of_graph_get_endpoint_by_regs(xxx, 0, -1);
Link: https://lore.kernel.org/r/20240202174941.GA310089-robh@kernel.org
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
drivers/gpu/drm/drm_of.c | 4 +++-
drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c | 2 +-
drivers/gpu/drm/tiny/arcpgu.c | 2 +-
3 files changed, 5 insertions(+), 3 deletions(-)
@@ -400,7 +400,7 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c)rpi_touchscreen_i2c_write(ts,REG_POWERON,0);/* Look up the DSI host. It needs to probe before we do. */-endpoint=of_graph_get_next_endpoint(dev->of_node,NULL);+endpoint=of_graph_get_endpoint_by_regs(dev->of_node,0,-1);if(!endpoint)return-ENODEV;
@@ -275,7 +275,7 @@ static int of_get_coresight_platform_data(struct device *dev,*/if(!parent){/*-*Avoidwarningsinof_graph_get_next_endpoint()+*Avoidwarningsinfor_each_endpoint_of_node()*ifthedevicedoesn'thaveanygraphconnections*/if(!of_graph_is_present(node))
@@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct device *dev,}/* Iterate through each output port to discover topology */-while((ep=of_graph_get_next_endpoint(parent,ep))){+for_each_endpoint_of_node(parent,ep){/**Legacybindingmixesinput/outputportsunderthe*sameparent.So,skiptheinputportsifwearedealing
@@ -199,18 +199,13 @@ static int xvip_graph_build_dma(struct xvip_composite_device *xdev)structmedia_pad*sink_pad;structxvip_graph_entity*ent;structv4l2_fwnode_linklink;-structdevice_node*ep=NULL;+structdevice_node*ep;structxvip_dma*dma;intret=0;dev_dbg(xdev->dev,"creating links for DMA engines\n");-while(1){-/* Get the next endpoint and parse its link. */-ep=of_graph_get_next_endpoint(node,ep);-if(ep==NULL)-break;-+for_each_endpoint_of_node(node,ep){dev_dbg(xdev->dev,"processing endpoint %pOF\n",ep);ret=v4l2_fwnode_parse_link(of_fwnode_handle(ep),&link);
@@ -353,33 +353,29 @@ static const u32 isc_sama5d2_gamma_table[][GAMMA_ENTRIES] = {staticintisc_parse_dt(structdevice*dev,structisc_device*isc){structdevice_node*np=dev->of_node;-structdevice_node*epn=NULL;+structdevice_node*epn;structisc_subdev_entity*subdev_entity;unsignedintflags;-intret;INIT_LIST_HEAD(&isc->subdev_entities);-while(1){+for_each_endpoint_of_node(np,epn){structv4l2_fwnode_endpointv4l2_epn={.bus_type=0};--epn=of_graph_get_next_endpoint(np,epn);-if(!epn)-return0;+intret;ret=v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),&v4l2_epn);if(ret){-ret=-EINVAL;+of_node_put(epn);dev_err(dev,"Could not parse the endpoint\n");-break;+return-EINVAL;}subdev_entity=devm_kzalloc(dev,sizeof(*subdev_entity),GFP_KERNEL);if(!subdev_entity){-ret=-ENOMEM;-break;+of_node_put(epn);+return-ENOMEM;}subdev_entity->epn=epn;
This code is an example of what Laurent was talking about. We're taking
storing "subdev_entity->epn = epn" but then not incrementing the
refcount. Perhaps it's not necessary?
The difference between this and _scoped() would be if we stored epn and
then returned. I feel like that's less common. We could detect that
sort of thing using static analysis if it turned out to be an issue.
regards,
dan carpenter
This introduces a Smatch warning because now "ret" is uninitialized if
the for_each_endpoint_of_node() list is empty. Is that something which
is possible?
I've been meaning to make a list of loops which always iterate at least
one time. for_each_cpu() etc.
regards,
dan carpenter
- while (1) {+ for_each_endpoint_of_node(np, epn) { struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };- epn = of_graph_get_next_endpoint(np, epn);- if (!epn)- return 0;- ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn), &v4l2_epn); if (ret) {
This introduces a Smatch warning because now "ret" is uninitialized if
the for_each_endpoint_of_node() list is empty. Is that something which
is possible?
I've been meaning to make a list of loops which always iterate at least
one time. for_each_cpu() etc.
Oh, OK thank you for pointing it.
I will fixup and post it next week
Thank you for your help !!
Best regards
---
Kuninori Morimoto
- while (1) {+ for_each_endpoint_of_node(np, epn) { struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };-- epn = of_graph_get_next_endpoint(np, epn);- if (!epn)- return 0;+ int ret; ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn), &v4l2_epn); if (ret) {- ret = -EINVAL;+ of_node_put(epn); dev_err(dev, "Could not parse the endpoint\n");- break;+ return -EINVAL; } subdev_entity = devm_kzalloc(dev, sizeof(*subdev_entity), GFP_KERNEL); if (!subdev_entity) {- ret = -ENOMEM;- break;+ of_node_put(epn);+ return -ENOMEM; } subdev_entity->epn = epn;
This code is an example of what Laurent was talking about. We're taking
storing "subdev_entity->epn = epn" but then not incrementing the
refcount. Perhaps it's not necessary?
The difference between this and _scoped() would be if we stored epn and
then returned. I feel like that's less common. We could detect that
sort of thing using static analysis if it turned out to be an issue.
Thank you for pointing good sample, Dan.
But I wonder should this patch-set include and use _scoped() macro ?
If above is just a comment, _scoped() macro will be separate patch-set.
If above is pointing the issue, v4 need to have _scoped() macro.
Thank you for your help !!
Best regards
---
Kuninori Morimoto