| Message ID | 20240303104853.31511-1-brgl@bgdev.pl |
|---|---|
| State | Accepted |
| Headers | show |
| Series | of: make for_each_property_of_node() available to to !OF | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 18 lines checked |
| robh/patch-applied | fail | build log |
On Sun, 03 Mar 2024 11:48:53 +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > 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 <bartosz.golaszewski@linaro.org> > --- > 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!
Hi Bartosz, On Sun, Mar 3, 2024 at 11:49 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > 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 > Fixes: 611cad720148 ("dt: add of_alias_scan and of_alias_get_id") How is this related? > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > 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? > --- a/include/linux/of.h > +++ b/include/linux/of.h > @@ -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) Is this safe if !OF? Can dn be NULL? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
On Tue, Mar 5, 2024 at 9:32 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Bartosz, > > On Sun, Mar 3, 2024 at 11:49 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > 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 > > > 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. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > --- > > 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 > > --- a/include/linux/of.h > > +++ b/include/linux/of.h > > @@ -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) > > 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 > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > 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
On Tue, Mar 5, 2024 at 2:32 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Bartosz, > > On Sun, Mar 3, 2024 at 11:49 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > 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 > > > Fixes: 611cad720148 ("dt: add of_alias_scan and of_alias_get_id") > > How is this related? > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > --- > > 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? > > > --- a/include/linux/of.h > > +++ b/include/linux/of.h > > @@ -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) > > 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
On Tue, 5 Mar 2024 18:56:04 +0100, Rob Herring <robh+dt@kernel.org> said: > > 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
On Tue, Mar 5, 2024 at 12:46 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Tue, 5 Mar 2024 18:56:04 +0100, Rob Herring <robh+dt@kernel.org> said: > > > > 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? 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) > 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
On Wed, Mar 6, 2024 at 8:34 PM Rob Herring <robh+dt@kernel.org> wrote: > > On Tue, Mar 5, 2024 at 12:46 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > On Tue, 5 Mar 2024 18:56:04 +0100, Rob Herring <robh+dt@kernel.org> said: > > > > > > 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? > > 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) > > > 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
diff --git a/include/linux/of.h b/include/linux/of.h index 6a9ddf20e79a..a3e8e429ad7f 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -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