[v2,00/13] of: property: add port base loop

  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.

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(-)
  

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.