Re: fwnode_for_each_child_node() and OF backend discrepancy

[Sakari Ailus]

Hi Krzysztof, Rafael,

On Tue, Jun 28, 2022 at 12:32:12PM +0200, Krzysztof Kozlowski wrote:
> On 27/06/2022 15:33, Rafael J. Wysocki wrote:
> > On Mon, Jun 27, 2022 at 3:08 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> >>
> >> On 27/06/2022 14:49, Michael Walle wrote:
> >>> Hi,
> >>>
> >>> I tired to iterate over all child nodes, regardless if they are
> >>> available
> >>> or not. Now there is that handy fwnode_for_each_child_node() (and the
> >>> fwnode_for_each_available_child_node()). The only thing is the OF
> >>> backend
> >>> already skips disabled nodes [1], making fwnode_for_each_child_node()
> >>> and
> >>> fwnode_for_each_available_child_node() behave the same with the OF
> >>> backend.
> >>>
> >>> Doesn't seem to be noticed by anyone for now. I'm not sure how to fix
> >>> that
> >>> one. fwnode_for_each_child_node() and also fwnode_get_next_child_node()
> >>> are
> >>> used by a handful of drivers. I've looked at some, but couldn't decide
> >>> whether they really want to iterate over all child nodes or just the
> >>> enabled
> >>> ones.
> >>
> >> If I get it correctly, this was introduced  by 8a0662d9ed29 ("Driver
> >> core: Unified interface for firmware node properties")
> >> .
> > 
> > Originally it was, but then it has been reworked a few times.
> > 
> > The backend callbacks were introduced by Sakari, in particular.
> 
> I see you as an author of 8a0662d9ed29 which adds
> device_get_next_child_node() and uses of_get_next_available_child()
> instead of of_get_next_child(). Although it was back in 2014, so maybe
> it will be tricky to get original intention. :)
> 
> Which commit do you mean when you refer to Sakari's work?
> 
> > 
> >> The question to Rafael - what was your intention when you added
> >> device_get_next_child_node() looking only for available nodes?
> > 
> > That depends on the backend.
> 
> We talk about OF backend. In your commit device_get_next_child_node for
> OF uses explicitly available node, not any node.

Well spotted.

I suppose that when the only function to get the next (available) child was
added, the expection was perhaps that only available child nodes did matter
in this API. On ACPI the two are almost always the same thing --- the
property API originates from OF and ACPI primarily uses different means to
work with what's under the level of devices.

What I'd perhaps do is to change the OF behaviour and switch callers to use
a different variant for drivers that do not appear solely ACPI-oriented.

acpi_fwnode_device_is_available() should return true for data nodes.
Otherwise getting the next available child isn't meaningful at all --- and
property API is primarily dealing with data nodes when it comes to ACPI.

I might not still try to backport the fixes as it matters more from API
consistency point of view than being an actual problem _right now_.

Also cc Heikki.

> 
> > fwnode_for_each_available_child_node() is more specific and IIRC it
> > was introduced for fw_devlink (CC Saravana).
> > 
> >> My understanding is that this implementation should be consistent with
> >> OF implementation, so fwnode_get_next_child_node=get any child.
> > 
> > IIUC, the OF implementation is not consistent with the
> > fwnode_get_next_child_node=get any child thing.
> > 
> >> However maybe ACPI treats it somehow differently?
> > 
> > acpi_get_next_subnode() simply returns the next subnode it can find.

-- 
Kind regards,

Sakari Ailus