| Message ID | 87fryhklhb.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | of: property: add port base loop | expand |
Hello Morimoto-san, (CC'ing Sakari Ailus) On Mon, Jan 29, 2024 at 12:54:24AM +0000, Kuninori Morimoto wrote: > > Hi Rob > > This is v2 of port base loop patch-set > > We have endpoint base functions > - of_graph_get_next_endpoint() > - of_graph_get_endpoint_count() > - for_each_endpoint_of_node() > > But to handling "port" base things, it is not useful. We want to have > "port" base functions, too. This patch-set adds it. > > Because current existing drivers couldn't use "port" base functions, > it were implemented in a different way. This patch-set doesn't try > to full-replace to avoid unknown bug, try easy / quick replace only > for now, but easy to know how "port" base functions are needed. > > Because I can't test the driver which I can't use, non-ASoC drivers > needs Tested-by, Acked-by. The strategy sounds good to me. However, I'm wondering if you shouldn't take one more step in the core, and implement these as fwnode operations. Or is there a reason why OF is special, and iterating over ports would be useful for drivers on OF systems but not on other types of systems ? > v1 -> v2 > - tidyup function explain > - add missing header on each files > > Kuninori Morimoto (13): > of: property: add port base loop > of: property: use of_graph_get_next_port() on of_graph_get_next_endpoint() > of: property: add of_graph_get_next_endpoint_raw() > drm: omapdrm: use of_graph_get_next_endpoint_raw() > media: xilinx-tpg: use of_graph_get_next_endpoint_raw() > ASoC: audio-graph-card: use of_graph_get_next_endpoint_raw() > ASoC: audio-graph-card2: use of_graph_get_next_port() > ASoC: audio-graph-card2: use of_graph_get_next_endpoint_raw() > ASoC: test-component: use for_each_port_of_node() > fbdev: omapfb: use of_graph_get_remote_port() > fbdev: omapfb: use of_graph_get_next_port() > fbdev: omapfb: use of_graph_get_next_endpoint_raw() > fbdev: omapfb: use of_graph_get_next_endpoint() > > drivers/gpu/drm/omapdrm/dss/dpi.c | 2 +- > drivers/gpu/drm/omapdrm/dss/sdi.c | 2 +- > drivers/media/platform/xilinx/xilinx-tpg.c | 2 +- > drivers/of/property.c | 92 +++++++++++++--- > drivers/video/fbdev/omap2/omapfb/dss/dpi.c | 3 +- > drivers/video/fbdev/omap2/omapfb/dss/dsi.c | 3 +- > drivers/video/fbdev/omap2/omapfb/dss/dss-of.c | 101 +----------------- > drivers/video/fbdev/omap2/omapfb/dss/dss.c | 9 +- > drivers/video/fbdev/omap2/omapfb/dss/hdmi4.c | 3 +- > drivers/video/fbdev/omap2/omapfb/dss/hdmi5.c | 3 +- > drivers/video/fbdev/omap2/omapfb/dss/sdi.c | 3 +- > drivers/video/fbdev/omap2/omapfb/dss/venc.c | 3 +- > include/linux/of_graph.h | 23 ++++ > include/video/omapfb_dss.h | 11 -- > sound/soc/generic/audio-graph-card.c | 2 +- > sound/soc/generic/audio-graph-card2.c | 31 ++---- > sound/soc/generic/test-component.c | 2 +- > 17 files changed, 133 insertions(+), 162 deletions(-)
Hi Laurent, Morimoto-san, On Mon, Jan 29, 2024 at 02:27:36PM +0200, Laurent Pinchart wrote: > Hello Morimoto-san, > > (CC'ing Sakari Ailus) Thanks for cc'ing me. > > On Mon, Jan 29, 2024 at 12:54:24AM +0000, Kuninori Morimoto wrote: > > > > Hi Rob > > > > This is v2 of port base loop patch-set > > > > We have endpoint base functions > > - of_graph_get_next_endpoint() > > - of_graph_get_endpoint_count() > > - for_each_endpoint_of_node() > > > > But to handling "port" base things, it is not useful. We want to have > > "port" base functions, too. This patch-set adds it. > > > > Because current existing drivers couldn't use "port" base functions, > > it were implemented in a different way. This patch-set doesn't try > > to full-replace to avoid unknown bug, try easy / quick replace only > > for now, but easy to know how "port" base functions are needed. > > > > Because I can't test the driver which I can't use, non-ASoC drivers > > needs Tested-by, Acked-by. > > The strategy sounds good to me. However, I'm wondering if you shouldn't > take one more step in the core, and implement these as fwnode > operations. Or is there a reason why OF is special, and iterating over > ports would be useful for drivers on OF systems but not on other types > of systems ? I'd prefer that, too. Probably we could use the existing callbacks for endpoint enumeration, for port enumeration, too, as I don't think this is performance critical in any way.
Hi Laurent, Sakari Thank you for your review > > The strategy sounds good to me. However, I'm wondering if you shouldn't > > take one more step in the core, and implement these as fwnode > > operations. Or is there a reason why OF is special, and iterating over > > ports would be useful for drivers on OF systems but not on other types > > of systems ? > > I'd prefer that, too. It is very easy reason, because I'm not fwnode user ;P I'm not familiar with fwnode, but in my quick check, it seems it is easy to expand fwnode side functions if of_graph side function exist ? Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
Hi Morimoto-san, On Tue, Jan 30, 2024 at 12:34:55AM +0000, Kuninori Morimoto wrote: > > Hi Laurent, Sakari > > Thank you for your review > > > > The strategy sounds good to me. However, I'm wondering if you shouldn't > > > take one more step in the core, and implement these as fwnode > > > operations. Or is there a reason why OF is special, and iterating over > > > ports would be useful for drivers on OF systems but not on other types > > > of systems ? > > > > I'd prefer that, too. > > It is very easy reason, because I'm not fwnode user ;P > I'm not familiar with fwnode, but in my quick check, it seems it is easy > to expand fwnode side functions if of_graph side function exist ? That would be one way to do that, yes, but I suggested using the existing endpoint iterators as that would keep the firmware specific implementation more simple. The (slight) drawback is that for each node returned, you'd need to check its parent (i.e. port node) is the same as the port you're interested in. The alternative may involve reworking the struct fwnode_operations interface somewhat, including swnode, DT and ACPI implementations.
On 30/01/2024 09:31, Sakari Ailus wrote: > Hi Morimoto-san, > > On Tue, Jan 30, 2024 at 12:34:55AM +0000, Kuninori Morimoto wrote: >> >> Hi Laurent, Sakari >> >> Thank you for your review >> >>>> The strategy sounds good to me. However, I'm wondering if you shouldn't >>>> take one more step in the core, and implement these as fwnode >>>> operations. Or is there a reason why OF is special, and iterating over >>>> ports would be useful for drivers on OF systems but not on other types >>>> of systems ? >>> >>> I'd prefer that, too. >> >> It is very easy reason, because I'm not fwnode user ;P >> I'm not familiar with fwnode, but in my quick check, it seems it is easy >> to expand fwnode side functions if of_graph side function exist ? > > That would be one way to do that, yes, but I suggested using the existing > endpoint iterators as that would keep the firmware specific implementation > more simple. The (slight) drawback is that for each node returned, you'd > need to check its parent (i.e. port node) is the same as the port you're > interested in. The alternative may involve reworking the struct > fwnode_operations interface somewhat, including swnode, DT and ACPI > implementations. > But we still need the of_* versions, don't we, for patches 4 to 13? Tomi
On Tue, Jan 30, 2024 at 09:37:42AM +0200, Tomi Valkeinen wrote: > On 30/01/2024 09:31, Sakari Ailus wrote: > > Hi Morimoto-san, > > > > On Tue, Jan 30, 2024 at 12:34:55AM +0000, Kuninori Morimoto wrote: > > > > > > Hi Laurent, Sakari > > > > > > Thank you for your review > > > > > > > > The strategy sounds good to me. However, I'm wondering if you shouldn't > > > > > take one more step in the core, and implement these as fwnode > > > > > operations. Or is there a reason why OF is special, and iterating over > > > > > ports would be useful for drivers on OF systems but not on other types > > > > > of systems ? > > > > > > > > I'd prefer that, too. > > > > > > It is very easy reason, because I'm not fwnode user ;P > > > I'm not familiar with fwnode, but in my quick check, it seems it is easy > > > to expand fwnode side functions if of_graph side function exist ? > > > > That would be one way to do that, yes, but I suggested using the existing > > endpoint iterators as that would keep the firmware specific implementation > > more simple. The (slight) drawback is that for each node returned, you'd > > need to check its parent (i.e. port node) is the same as the port you're > > interested in. The alternative may involve reworking the struct > > fwnode_operations interface somewhat, including swnode, DT and ACPI > > implementations. > > > > But we still need the of_* versions, don't we, for patches 4 to 13? Yes, my comment was indeed about the fwnode property API only.
Hi Sakari > > > > I'm not familiar with fwnode, but in my quick check, it seems it is easy > > > > to expand fwnode side functions if of_graph side function exist ? > > > > > > That would be one way to do that, yes, but I suggested using the existing > > > endpoint iterators as that would keep the firmware specific implementation > > > more simple. The (slight) drawback is that for each node returned, you'd > > > need to check its parent (i.e. port node) is the same as the port you're > > > interested in. The alternative may involve reworking the struct > > > fwnode_operations interface somewhat, including swnode, DT and ACPI > > > implementations. > > > > > > > But we still need the of_* versions, don't we, for patches 4 to 13? > > Yes, my comment was indeed about the fwnode property API only. Thank you for your suggestion. But I'm not familiar with fwnode, and it seems we still need of_*, I will keep current style (= non fwnode) in v3 Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
Hi Morimoto-san, On Tue, Jan 30, 2024 at 11:24:07PM +0000, Kuninori Morimoto wrote: > > Hi Sakari > > > > > > I'm not familiar with fwnode, but in my quick check, it seems it is easy > > > > > to expand fwnode side functions if of_graph side function exist ? > > > > > > > > That would be one way to do that, yes, but I suggested using the existing > > > > endpoint iterators as that would keep the firmware specific implementation > > > > more simple. The (slight) drawback is that for each node returned, you'd > > > > need to check its parent (i.e. port node) is the same as the port you're > > > > interested in. The alternative may involve reworking the struct > > > > fwnode_operations interface somewhat, including swnode, DT and ACPI > > > > implementations. > > > > > > > > > > But we still need the of_* versions, don't we, for patches 4 to 13? > > > > Yes, my comment was indeed about the fwnode property API only. > > Thank you for your suggestion. > But I'm not familiar with fwnode, and it seems we still need of_*, > I will keep current style (= non fwnode) in v3 The fwnode API should be kept in sync with the OF (and other firmware specific) API. Merging your set in its current form would leave fwnode API impaired. Therefore I'd very much prefer to see this set add similar fwnode APIs, too.
Hi Sakari > > Thank you for your suggestion. > > But I'm not familiar with fwnode, and it seems we still need of_*, > > I will keep current style (= non fwnode) in v3 > > The fwnode API should be kept in sync with the OF (and other firmware > specific) API. Merging your set in its current form would leave fwnode API > impaired. Therefore I'd very much prefer to see this set add similar fwnode > APIs, too. I will keep current fwnode API behavior, but I can't test it. Now, I'm separating the patch-set into small stages. There is no problem for a while, but I think I will ask you to test it in the final stage. Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
Hello Morimoto-san, On Mon, Feb 05, 2024 at 05:31:25AM +0000, Kuninori Morimoto wrote: > > Hi Sakari > > > > Thank you for your suggestion. > > > But I'm not familiar with fwnode, and it seems we still need of_*, > > > I will keep current style (= non fwnode) in v3 > > > > The fwnode API should be kept in sync with the OF (and other firmware > > specific) API. Merging your set in its current form would leave fwnode API > > impaired. Therefore I'd very much prefer to see this set add similar fwnode > > APIs, too. > > I will keep current fwnode API behavior, but I can't test it. The fwnode API is an abstraction layer on top of the OF or ACPI APIs, and allows drivers to work on both without needing to support OF and ACPI explicitly and separately. You should be able to convert the drivers you're using to the fwnode API, and it should work exactly the same as when using the OF-specific functions. That will give you a way to test the API. For instance, if you look at the drivers/media/platform/rcar_drif.c driver, you will see if (!fwnode_property_read_u32(fwnode, "sync-active", &val)) which, on OF platforms, is equivalent to if (!of_property_read_u32(np, "sync-active", &val)) This particular driver will never be used on an ACPI-based system, but drivers are still encouraged to use the fwnode API. > Now, I'm separating the patch-set into small stages. > There is no problem for a while, but I think I will ask you to test it in the > final stage.