On Sun, Dec 5, 2021 at 1:48 PM Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > > 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()/ It's not equivalent, but in an ideal world they would have been. Most of the time, one should be using for_each_available_child_of_node() as that treats disabled nodes as if they were non-existent. Unfortunately, there are some cases where walking the disabled nodes is desirable/needed. On !Arm, disabled CPU nodes are ones that are offline for example. Ideally, we would have had for_each_child_of_node() and for_each_yes_I_really_want_disabled_child_of_node() instead. > 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(). That may have been based on my feedback so that fwnode has the 'right' interface... > > 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
