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(-)

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
-- 
2.40.1

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 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

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 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!