of: make for_each_property_of_node() available to to !OF

PATCH Applied Bartosz Golaszewski 4 participants 8 messages Mar 3, 2024 – Mar 7, 2024 Device tree

lore

Share & tools

lore

Bartosz Golaszewski

author

2024-03-03 10:48 UTC #0 lore raw

PATCH 1/1 of: make for_each_property_of_node() available to to !OF

From: Bartosz Golaszewski <[email protected]>

for_each_property_of_node() is a macro and so doesn't have a stub inline function for !OF. Move it out of the relevant #ifdef to make it available to all users.

Fixes 611cad720148 ("dt: add of_alias_scan and of_alias_get_id")

Signed-off-by Bartosz Golaszewski <[email protected]>

---
I have an upcoming driver that will use this but which can also be built on non-DT systems. I'd like to get that in as a fix to avoid inter-tree dependencies later.

 include/linux/of.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

include/linux/of.h +3 -3

index 6a9ddf20e79a..a3e8e429ad7f 100644
@@ -362,9 +362,6 @@ extern struct device_node *of_get_cpu_state_node(struct device_node *cpu_node,
 						 int index);
 extern u64 of_get_cpu_hwid(struct device_node *cpun, unsigned int thread);
 
-#define for_each_property_of_node(dn, pp) \
-	for (pp = dn->properties; pp != NULL; pp = pp->next)
-
 extern int of_n_addr_cells(struct device_node *np);
 extern int of_n_size_cells(struct device_node *np);
 extern const struct of_device_id *of_match_node(

@@ -892,6 +889,9 @@ static inline int of_prop_val_eq(struct property *p1, struct property *p2)
 	       !memcmp(p1->value, p2->value, (size_t)p1->length);
 }
 
+#define for_each_property_of_node(dn, pp) \
+	for (pp = dn->properties; pp != NULL; pp = pp->next)
+
 #if defined(CONFIG_OF) && defined(CONFIG_NUMA)
 extern int of_node_to_nid(struct device_node *np);
 #else

Signature · 1 line

2.40.1

Rob Herring

maintainer Applied

2024-03-04 19:27 UTC #1 lore raw

Quoted 8 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
for_each_property_of_node() is a macro and so doesn't have a stub inline function for !OF. Move it out of the relevant #ifdef to make it available to all users.
Fixes 611cad720148 ("dt: add of_alias_scan and of_alias_get_id")
Signed-off-by Bartosz Golaszewski <[email protected]>

Quoted 8 lines

---
I have an upcoming driver that will use this but which can also be built on non-DT systems. I'd like to get that in as a fix to avoid inter-tree dependencies later.
 include/linux/of.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Applied, thanks!

Geert Uytterhoeven

2024-03-05 08:32 UTC #2 lore raw

In reply to Bartosz Golaszewski

Hi Bartosz,

Quoted 5 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>
for_each_property_of_node() is a macro and so doesn't have a stub inline function for !OF. Move it out of the relevant #ifdef to make it available to all users.

Thanks for your patch, which is now commit ad8ee969d7e34dd3 ("of: make for_each_property_of_node() available to to !OF") in dt-rh/for-next

Quoted 1 line

Fixes 611cad720148 ("dt: add of_alias_scan and of_alias_get_id")

How is this related?

Quoted 5 lines

Signed-off-by Bartosz Golaszewski <[email protected]>
---
I have an upcoming driver that will use this but which can also be built on non-DT systems. I'd like to get that in as a fix to avoid inter-tree dependencies later.

Do you have a link?

Quoted 13 lines

include/linux/of.h +0 -3

@@ -362,9 +362,6 @@ extern struct device_node *of_get_cpu_state_node(struct device_node *cpu_node,
                                                 int index);
 extern u64 of_get_cpu_hwid(struct device_node *cpun, unsigned int thread);
 
-#define for_each_property_of_node(dn, pp) \
-       for (pp = dn->properties; pp != NULL; pp = pp->next)
-
 extern int of_n_addr_cells(struct device_node *np);
 extern int of_n_size_cells(struct device_node *np);
 extern const struct of_device_id *of_match_node(

include/linux/of.h static inline int of_prop_val_eq(struct property *p1, struct property *p2) · line 893

Quoted 5 lines

include/linux/of.h +2 -0

@@ -892,6 +889,9 @@ static inline int of_prop_val_eq(struct property *p1, struct property *p2)
               !memcmp(p1->value, p2->value, (size_t)p1->length);
 }
 
+#define for_each_property_of_node(dn, pp) \
+       for (pp = dn->properties; pp != NULL; pp = pp->next)

Is this safe if !OF? Can dn be NULL?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Bartosz Golaszewski

author

2024-03-05 17:47 UTC #3 lore raw

Quoted 10 lines · Geert Uytterhoeven

Hi Bartosz,
Quoted 5 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
for_each_property_of_node() is a macro and so doesn't have a stub inline function for !OF. Move it out of the relevant #ifdef to make it available to all users.

Quoted 7 lines

Thanks for your patch, which is now commit ad8ee969d7e34dd3 ("of: make for_each_property_of_node() available to to !OF") in dt-rh/for-next
Quoted 1 line
Fixes 611cad720148 ("dt: add of_alias_scan and of_alias_get_id")
How is this related?

This commit added that macro in the wrong place. Back then it was called differently, it got later renamed but this is the original commit that provided it.

Quoted 8 lines

Quoted 5 lines
Signed-off-by Bartosz Golaszewski <[email protected]>
---
I have an upcoming driver that will use this but which can also be built on non-DT systems. I'd like to get that in as a fix to avoid inter-tree dependencies later.
Do you have a link?

Sure, it's here: https://github.com/brgl/linux/tree/topic/gpio-virtual-consumer

Quoted 16 lines

include/linux/of.h +0 -3

@@ -362,9 +362,6 @@ extern struct device_node *of_get_cpu_state_node(struct device_node *cpu_node,
                                                 int index);
 extern u64 of_get_cpu_hwid(struct device_node *cpun, unsigned int thread);
 
-#define for_each_property_of_node(dn, pp) \
-       for (pp = dn->properties; pp != NULL; pp = pp->next)
-
 extern int of_n_addr_cells(struct device_node *np);
 extern int of_n_size_cells(struct device_node *np);
 extern const struct of_device_id *of_match_node(

@@ -892,6 +889,9 @@ static inline int of_prop_val_eq(struct property *p1, struct property *p2)
               !memcmp(p1->value, p2->value, (size_t)p1->length);
 }

Quoted 5 lines

include/linux/of.h static inline int of_prop_val_eq(struct property *p1, struct property *p2) · line 893
Quoted 2 lines
include/linux/of.h +2 -0
+#define for_each_property_of_node(dn, pp) \
+       for (pp = dn->properties; pp != NULL; pp = pp->next)
Is this safe if !OF? Can dn be NULL?

The way I use it[1], it's safe but it's a good point, I'll send a follow-up.

Thanks,
Bart

Quoted 10 lines

Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

[1] https://github.com/brgl/linux/blob/topic/gpio-virtual-consumer/drivers/gpio/gpio-virtuser.c#L878

Rob Herring

2024-03-05 17:56 UTC #4 lore raw

In reply to Geert Uytterhoeven

Quoted 41 lines · Geert Uytterhoeven

Hi Bartosz,
Quoted 5 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
for_each_property_of_node() is a macro and so doesn't have a stub inline function for !OF. Move it out of the relevant #ifdef to make it available to all users.
Thanks for your patch, which is now commit ad8ee969d7e34dd3 ("of: make for_each_property_of_node() available to to !OF") in dt-rh/for-next
Quoted 1 line
Fixes 611cad720148 ("dt: add of_alias_scan and of_alias_get_id")
How is this related?
Quoted 5 lines
Signed-off-by Bartosz Golaszewski <[email protected]>
---
I have an upcoming driver that will use this but which can also be built on non-DT systems. I'd like to get that in as a fix to avoid inter-tree dependencies later.
Do you have a link?
Quoted 16 lines
include/linux/of.h +0 -3
@@ -362,9 +362,6 @@ extern struct device_node *of_get_cpu_state_node(struct device_node *cpu_node,
                                                 int index);
 extern u64 of_get_cpu_hwid(struct device_node *cpun, unsigned int thread);
 
-#define for_each_property_of_node(dn, pp) \
-       for (pp = dn->properties; pp != NULL; pp = pp->next)
-
 extern int of_n_addr_cells(struct device_node *np);
 extern int of_n_size_cells(struct device_node *np);
 extern const struct of_device_id *of_match_node(
@@ -892,6 +889,9 @@ static inline int of_prop_val_eq(struct property *p1, struct property *p2)
               !memcmp(p1->value, p2->value, (size_t)p1->length);
 }
 

Quoted 4 lines

include/linux/of.h static inline int of_prop_val_eq(struct property *p1, struct property *p2) · line 893
Quoted 2 lines
include/linux/of.h +2 -0
+#define for_each_property_of_node(dn, pp) \
+       for (pp = dn->properties; pp != NULL; pp = pp->next)
Is this safe if !OF? Can dn be NULL?

Good point. I would say running code shouldn't reach this though. Also, it should be written in a way it gets optimized away if !OF is valid.

Long term, I want to make struct device_node opaque. So if we really want to fix this, I think we'd want to convert this to use an iterator function. Though I guess any user would be mucking with struct property too, so the whole loop would need to be reworked. So in conclusion, don't use for_each_property_of_node(). :) Shrug.

Rob

Bartosz Golaszewski

author

2024-03-05 18:46 UTC #5 lore raw

Quoted 7 lines · Rob Herring

Long term, I want to make struct device_node opaque. So if we really want to fix this, I think we'd want to convert this to use an iterator function. Though I guess any user would be mucking with struct property too, so the whole loop would need to be reworked. So in conclusion, don't use for_each_property_of_node(). :) Shrug.

I basically just need to get the list of all properties of a node. Even just names. I'm working on a testing driver that needs to request all GPIOs assigned to it over DT so it must find all foo-gpios properties.

How about:

int of_node_for_each_property(struct device_node *dn, int (*func)(struct property *, void *), void *data)

as the iterator? You didn't say if you want to make struct property opaque as well but even then it can be used with provided interfaces.

Bart

Rob Herring

2024-03-06 19:34 UTC #6 lore raw

Quoted 14 lines · Bartosz Golaszewski

Quoted 7 lines · Rob Herring
Long term, I want to make struct device_node opaque. So if we really want to fix this, I think we'd want to convert this to use an iterator function. Though I guess any user would be mucking with struct property too, so the whole loop would need to be reworked. So in conclusion, don't use for_each_property_of_node(). :) Shrug.
I basically just need to get the list of all properties of a node. Even just names. I'm working on a testing driver that needs to request all GPIOs assigned to it over DT so it must find all foo-gpios properties.

Quoted 6 lines

How about:
int of_node_for_each_property(struct device_node *dn, int (*func)(struct property *, void *), void *data)
as the iterator?

Why would we make the caller provide the iterator? We just need a function call like the other iterators rather than directly dereferencing the pointers: of_next_property_iter(node, last_prop)

Quoted 2 lines

You didn't say if you want to make struct property opaque as well but even then it can be used with provided interfaces.

Yes, I'd like to make struct property opaque as well. That's probably the first step.

Rob

Bartosz Golaszewski

author

2024-03-07 14:26 UTC #7 lore raw

Quoted 27 lines · Rob Herring

Quoted 14 lines · Bartosz Golaszewski
Quoted 7 lines · Rob Herring
Long term, I want to make struct device_node opaque. So if we really want to fix this, I think we'd want to convert this to use an iterator function. Though I guess any user would be mucking with struct property too, so the whole loop would need to be reworked. So in conclusion, don't use for_each_property_of_node(). :) Shrug.
I basically just need to get the list of all properties of a node. Even just names. I'm working on a testing driver that needs to request all GPIOs assigned to it over DT so it must find all foo-gpios properties.
Quoted 6 lines
How about:
int of_node_for_each_property(struct device_node *dn, int (*func)(struct property *, void *), void *data)
as the iterator?
Why would we make the caller provide the iterator? We just need a function call like the other iterators rather than directly dereferencing the pointers: of_next_property_iter(node, last_prop)

Quoted 7 lines

Quoted 2 lines
You didn't say if you want to make struct property opaque as well but even then it can be used with provided interfaces.
Yes, I'd like to make struct property opaque as well. That's probably the first step.
Rob

Or maybe we should implement it at the fwnode_operations level? Although not sure how to handle it as fwnode doesn't have a separate structure for properties.

Bart