I think we need Rob here (or anybody with DT API knowledge) to explain this subtle detail you found, i.e. checking node for availability in of_fwnode_get_next_child_node(). This raises another question why do we have for_each_available_child_of_node() in the first place if it's equivalent (is it?) to for_each_child_of_node()/ On Sun, Dec 5, 2021 at 8:55 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > > Hi All, > > This came up in review of > https://lore.kernel.org/linux-iio/20210725172458.487343-1-jic23@xxxxxxxxxx/ > which is a series converting a dt only driver over to generic properties. > I'm sending a separate email to raise the profile of the question rather > higher than it was buried in a driver review. > > The original code used for_each_available_child_of_node(np, child) > and the patch converted it to device_for_each_child_node(). > > Andy raised the question of whether it should have been > device_for_each_available_child_node() but that doesn't exist currently. > > Things get more interesting when you look at the implementation of > device_for_each_child_node() which uses device_get_next_child_node() > which in turn calls fwnode_get_next_child_node() which calls > the get_next_child_node() op and for of that is > of_fwnode_get_next_child_node() which uses of_get_next_available_child() > rather than of_get_next_child(). > > So I think under the hood device_for_each_child_node() on of_ is going to > end up checking the node is available anyway. > > So this all seemed a little odd given there were obvious calls to use > if we wanted to separate the two cases for device tree and they weren't > the ones used. However, if we conclude that there is a bug here and > the two cases should be handled separately then it will be really hard > to be sure no driver is relying on this behaviour. > > So, ultimately the question is: Should I add a > device_for_each_available_child_node()? It will be something like: > > struct fwnode_handle *device_get_next_child_node(struct device *dev, > struct fwnode_handle *child) > { > const struct fwnode_handle *fwnode = dev_fwnode(dev); > struct fwnode_handle *next; > > /* Try to find a child in primary fwnode */ > next = fwnode_get_next_available_child_node(fwnode, child); > if (next) > return next; > > /* When no more children in primary, continue with secondary */ > if (fwnode && !IS_ERR_OR_NULL(fwnode->secondary)) > next = fwnode_get_next_available_child_node(fwnode->secondary, child); > > return next; > } > > #define device_for_each_child_node(dev, child) \ > for (child = device_get_next_available_child_node(dev, NULL); child; \ > child = device_get_next_avaialble_child_node(dev, child)) > > As far as I can tell it doesn't make any difference for my particular bit > of refactoring in the sense of I won't break anything that currently > works by using device_for_each_child_node() but it may cause issues with > other firmware by enumerating disabled child nodes. > > Jonathan > > > > > -- With Best Regards, Andy Shevchenko
