| Message ID | 20240805-device_for_each_child_node-available-v3-0-48243a4aa5c0@gmail.com |
|---|---|
| Headers | show |
| Series | use device_for_each_child_node() to access device child nodes | expand |
Hi Javier, Thanks for the patch. On Mon, Aug 05, 2024 at 04:49:44PM +0200, Javier Carrasco wrote: > There have been some misconceptions about this macro, which iterates > over available child nodes from different backends. > > As that is not obvious by its name, some users have opted for the > `fwnode_for_each_available_child_node()` macro instead. > That requires an unnecessary, explicit access to the fwnode member > of the device structure. > > Passing the device to `device_for_each_child_node()` is shorter, > reflects more clearly the nature of the child nodes, and renders the > same result. > > In general, `fwnode_for_each_available_child_node()` should be used > whenever the parent node of the children to iterate over is a firmware > node, and not the device itself. > > Document the `device_for_each_child node(dev, child)` macro to clarify > its functionality. > > Suggested-by: Jonathan Cameron <jic23@kernel.org> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > --- > include/linux/property.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/include/linux/property.h b/include/linux/property.h > index 61fc20e5f81f..da8f1208de38 100644 > --- a/include/linux/property.h > +++ b/include/linux/property.h > @@ -171,6 +171,16 @@ struct fwnode_handle *fwnode_get_next_available_child_node( > struct fwnode_handle *device_get_next_child_node(const struct device *dev, > struct fwnode_handle *child); > > +/** > + * device_for_each_child_node - iterate over available child nodes of a device > + * @dev: Pointer to the struct device > + * @child: Pointer to an available child node in each loop iteration > + * > + * Unavailable nodes are skipped i.e. this macro is implicitly _available_. Is it? I think this was brought up previously but I understand there's an available check on OF but not on ACPI, so on OF the availability-agnostic and availability-aware variants are the same. Both in OF and ACPI a node that's been marked not available simply isn't there so as far as I understand, the semantics are the same. There's a difference though: in OF every node can be tested for availability whereas on ACPI only device nodes can (there are data nodes, too, for which the availability test is always false as it's pointless to test them in the first place). So overall I guess the code is mostly ok be but the caller does need to be careful with the differences. > + * The reference to the child node must be dropped on early exits. > + * See fwnode_handle_put(). > + * For a scoped version of this macro, use device_for_each_child_node_scoped(). > + */ > #define device_for_each_child_node(dev, child) \ > for (child = device_get_next_child_node(dev, NULL); child; \ > child = device_get_next_child_node(dev, child)) >
On Mon, 05 Aug 2024 16:49:43 +0200 Javier Carrasco wrote: > net: mvpp2: use port_count to remove ports > net: mvpp2: use device_for_each_child_node() to access device child nodes Please repost these two separately so we can take the whole series via networking. As is the series doesn't apply so it's too much manual twiddling to fit into our tree.
This series aims to clarify the use cases of: - device_for_each_child_node[_scoped]() - fwnode_for_each_available_child_node[_scoped]() to access firmware nodes. There have been multiple discussions [1][2] about what the first macro implies in the sense of availability, and a number of users have opted for the second macro in cases where the first one should have been preferred. The second macro is intended to be used over child nodes of a firmware node, not direct child nodes of the device node. Instead, those users retrieve the fwnode member from the device struct just to have access to a macro that explicitly indicates node availability. That workaround is not necessary because `device_for_each_child_node()` implies availability for the existing backends (ACPI, DT, swnode). This series does not cover other points discussed in [2] like addressing uses of `fwnode_for_each_child_node()` where `device_*` should have been used, using the `_avaialble_` variant of the fwnode loop whenever possible, or adding new `_scoped` macros. Such points will be covered by subsequent series to keep focus on the "availability" issue. The conversion has been validated with an LTC2992 hwmon sensor, which is one of the affected drivers. The rest of the drivers could only be compiled and checked with static-analysis tools. Link: https://lore.kernel.org/all/20211205190101.26de4a57@jic23-huawei/ [1] Link: https://lore.kernel.org/all/20240523-fwnode_for_each_available_child_node_scoped-v2-0-701f3a03f2fb@gmail.com/ [2] Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> --- Changes in v3: - Rebase to next/20240805 and drop applied patches. - mvpp2: fix typos in commit description. - mvpp2: add patch to use port_count instead of node iteration. - Link to v2: https://lore.kernel.org/r/20240721-device_for_each_child_node-available-v2-0-f33748fd8b2d@gmail.com Changes in v2: - [1/6] property.h: drop "if found" from the description of device_for_each_child_node() - [3/6] bd2607mvv.c: fix child node usage. - Link to v1: https://lore.kernel.org/r/20240706-device_for_each_child_node-available-v1-0-8a3f7615e41c@gmail.com --- Javier Carrasco (4): device property: document device_for_each_child_node macro leds: pca995x: use device_for_each_child_node() to access device child nodes net: mvpp2: use port_count to remove ports net: mvpp2: use device_for_each_child_node() to access device child nodes drivers/leds/leds-pca995x.c | 15 ++++-------- drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 31 +++++++------------------ include/linux/property.h | 10 ++++++++ 3 files changed, 24 insertions(+), 32 deletions(-) --- base-commit: d6dbc9f56c3a70e915625b6f1887882c23dc5c91 change-id: 20240701-device_for_each_child_node-available-1c1eca4b6495 Best regards,