Currently, there are many pieces of nearly identical code scattered across
different places. Consolidate the duplicate code into helper functions to
improve maintainability and reduce the likelihood of errors.

Signed-off-by: Yuntao Wang <yuntao.wang@linux.dev>
---
 drivers/of/fdt.c       | 41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/of_fdt.h |  5 +++++
 2 files changed, 46 insertions(+)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 0edd639898a6..5e0eabc1449f 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -625,6 +625,47 @@ const void *__init of_get_flat_dt_prop(unsigned long node, const char *name,
 	return fdt_getprop(initial_boot_params, node, name, size);
 }
 
+const __be32 *__init of_fdt_get_addr_size_prop(unsigned long node,
+                                               const char *name, int *entries)
+{
+	const __be32 *prop;
+	int len, elen = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
+
+	prop = of_get_flat_dt_prop(node, name, &len);
+	if (!prop) {
+		*entries = 0;
+		return NULL;
+	}
+
+	if (len % elen) {
+		*entries = -1;
+		return NULL;
+	}
+
+	*entries = len / elen;
+	return prop;
+}
+
+bool __init of_fdt_get_addr_size(unsigned long node, const char *name,
+                                 u64 *addr, u64 *size)
+{
+	const __be32 *prop;
+	int len, elen = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
+
+	prop = of_get_flat_dt_prop(node, name, &len);
+	if (!prop || len < elen)
+		return false;
+
+	of_fdt_read_addr_size(prop, addr, size);
+	return true;
+}
+
+void __init of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size)
+{
+	*addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
+	*size = dt_mem_next_cell(dt_root_size_cells, &prop);
+}
+
 /**
  * of_fdt_is_compatible - Return true if given node from the given blob has
  * compat in its compatible list
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index b8d6c0c20876..3a0805ff6c7b 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -55,6 +55,11 @@ extern int of_get_flat_dt_subnode_by_name(unsigned long node,
 					  const char *uname);
 extern const void *of_get_flat_dt_prop(unsigned long node, const char *name,
 				       int *size);
+extern const __be32 *of_fdt_get_addr_size_prop(unsigned long node,
+                                               const char *name, int *entries);
+extern bool of_fdt_get_addr_size(unsigned long node, const char *name,
+                                 u64 *addr, u64 *size);
+extern void of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size);
 extern int of_flat_dt_is_compatible(unsigned long node, const char *name);
 extern unsigned long of_get_flat_dt_root(void);
 extern uint32_t of_get_flat_dt_phandle(unsigned long node);
-- 
2.51.0

On Thu, Nov 13, 2025 at 11:50:58PM +0800, Yuntao Wang wrote:
> Currently, there are many pieces of nearly identical code scattered across
> different places. Consolidate the duplicate code into helper functions to
> improve maintainability and reduce the likelihood of errors.
> 
> Signed-off-by: Yuntao Wang <yuntao.wang@linux.dev>
> ---
>  drivers/of/fdt.c       | 41 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_fdt.h |  5 +++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 0edd639898a6..5e0eabc1449f 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -625,6 +625,47 @@ const void *__init of_get_flat_dt_prop(unsigned long node, const char *name,
>  	return fdt_getprop(initial_boot_params, node, name, size);
>  }
>  
> +const __be32 *__init of_fdt_get_addr_size_prop(unsigned long node,
> +                                               const char *name, int *entries)
> +{
> +	const __be32 *prop;
> +	int len, elen = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
> +
> +	prop = of_get_flat_dt_prop(node, name, &len);
> +	if (!prop) {
> +		*entries = 0;
> +		return NULL;
> +	}
> +
> +	if (len % elen) {
> +		*entries = -1;

I don't think it's really important to distinguish a length error from 
any other error. Either we can read the property or we can't.

> +		return NULL;
> +	}
> +
> +	*entries = len / elen;
> +	return prop;
> +}
> +
> +bool __init of_fdt_get_addr_size(unsigned long node, const char *name,
> +                                 u64 *addr, u64 *size)
> +{
> +	const __be32 *prop;
> +	int len, elen = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);

Still have 2 locations to get the same calculation wrong...

> +
> +	prop = of_get_flat_dt_prop(node, name, &len);
> +	if (!prop || len < elen)
> +		return false;

Why doesn't calling of_fdt_get_addr_size_prop() work here? If 'len < 
elen', then 'len % elen' will also be true except in the 0 length case. 
For that case, of_fdt_get_addr_size_prop() needs to handle it too.

> +
> +	of_fdt_read_addr_size(prop, addr, size);
> +	return true;
> +}
> +
> +void __init of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size)
> +{
> +	*addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
> +	*size = dt_mem_next_cell(dt_root_size_cells, &prop);
> +}
> +
>  /**
>   * of_fdt_is_compatible - Return true if given node from the given blob has
>   * compat in its compatible list
> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> index b8d6c0c20876..3a0805ff6c7b 100644
> --- a/include/linux/of_fdt.h
> +++ b/include/linux/of_fdt.h
> @@ -55,6 +55,11 @@ extern int of_get_flat_dt_subnode_by_name(unsigned long node,
>  					  const char *uname);
>  extern const void *of_get_flat_dt_prop(unsigned long node, const char *name,
>  				       int *size);
> +extern const __be32 *of_fdt_get_addr_size_prop(unsigned long node,
> +                                               const char *name, int *entries);
> +extern bool of_fdt_get_addr_size(unsigned long node, const char *name,
> +                                 u64 *addr, u64 *size);
> +extern void of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size);
>  extern int of_flat_dt_is_compatible(unsigned long node, const char *name);

Looks like of_flat_dt_* would be more consistent with existing naming.

>  extern unsigned long of_get_flat_dt_root(void);
>  extern uint32_t of_get_flat_dt_phandle(unsigned long node);
> -- 
> 2.51.0
>

On Thu, 13 Nov 2025 16:38:59 -0600, Rob Herring <robh@kernel.org> wrote:

> On Thu, Nov 13, 2025 at 11:50:58PM +0800, Yuntao Wang wrote:
> > Currently, there are many pieces of nearly identical code scattered across
> > different places. Consolidate the duplicate code into helper functions to
> > improve maintainability and reduce the likelihood of errors.
> > 
> > Signed-off-by: Yuntao Wang <yuntao.wang@linux.dev>
> > ---
> >  drivers/of/fdt.c       | 41 +++++++++++++++++++++++++++++++++++++++++
> >  include/linux/of_fdt.h |  5 +++++
> >  2 files changed, 46 insertions(+)
> > 
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index 0edd639898a6..5e0eabc1449f 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -625,6 +625,47 @@ const void *__init of_get_flat_dt_prop(unsigned long node, const char *name,
> >  	return fdt_getprop(initial_boot_params, node, name, size);
> >  }
> >  
> > +const __be32 *__init of_fdt_get_addr_size_prop(unsigned long node,
> > +                                               const char *name, int *entries)
> > +{
> > +	const __be32 *prop;
> > +	int len, elen = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
> > +
> > +	prop = of_get_flat_dt_prop(node, name, &len);
> > +	if (!prop) {
> > +		*entries = 0;
> > +		return NULL;
> > +	}
> > +
> > +	if (len % elen) {
> > +		*entries = -1;
> 
> I don't think it's really important to distinguish a length error from 
> any other error. Either we can read the property or we can't.

Hi Rob,

I didn't originally split it into two checks, but later I noticed that in
__reserved_mem_reserve_reg(), the two error conditions return different
error codes. I was concerned about breaking compatibility, so I made the
change this way.

If compatibility isn't an issue, I'd be happy to merge the two checks into one.

> 
> > +		return NULL;
> > +	}
> > +
> > +	*entries = len / elen;
> > +	return prop;
> > +}
> > +
> > +bool __init of_fdt_get_addr_size(unsigned long node, const char *name,
> > +                                 u64 *addr, u64 *size)
> > +{
> > +	const __be32 *prop;
> > +	int len, elen = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
> 
> Still have 2 locations to get the same calculation wrong...
> 
> > +
> > +	prop = of_get_flat_dt_prop(node, name, &len);
> > +	if (!prop || len < elen)
> > +		return false;
> 
> Why doesn't calling of_fdt_get_addr_size_prop() work here? If 'len < 
> elen', then 'len % elen' will also be true except in the 0 length case. 
> For that case, of_fdt_get_addr_size_prop() needs to handle it too.

I'm fully in favor of calling of_fdt_get_addr_size_prop() directly here,
that was my original intention as well, which is also why I placed this
function right after of_fdt_get_addr_size_prop().

But again, due to compatibility concerns, I had to implement it this way.

For example, suppose prop points to data like:

[addr, size, other data]

With the previous `len < elen` check, addr and size could still be read
successfully. But if we switch to the `len % elen` check, this type of
data may fail.

If compatibility is not a concern, I can certainly change it to something
like the following:

prop = of_fdt_get_addr_size_prop(node, name, &len);
if (!prop || len != 1)
  return false;

> 
> > +
> > +	of_fdt_read_addr_size(prop, addr, size);
> > +	return true;
> > +}
> > +
> > +void __init of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size)
> > +{
> > +	*addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
> > +	*size = dt_mem_next_cell(dt_root_size_cells, &prop);
> > +}
> > +
> >  /**
> >   * of_fdt_is_compatible - Return true if given node from the given blob has
> >   * compat in its compatible list
> > diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> > index b8d6c0c20876..3a0805ff6c7b 100644
> > --- a/include/linux/of_fdt.h
> > +++ b/include/linux/of_fdt.h
> > @@ -55,6 +55,11 @@ extern int of_get_flat_dt_subnode_by_name(unsigned long node,
> >  					  const char *uname);
> >  extern const void *of_get_flat_dt_prop(unsigned long node, const char *name,
> >  				       int *size);
> > +extern const __be32 *of_fdt_get_addr_size_prop(unsigned long node,
> > +                                               const char *name, int *entries);
> > +extern bool of_fdt_get_addr_size(unsigned long node, const char *name,
> > +                                 u64 *addr, u64 *size);
> > +extern void of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size);
> >  extern int of_flat_dt_is_compatible(unsigned long node, const char *name);
> 
> Looks like of_flat_dt_* would be more consistent with existing naming.

Naming is hard :-)

I spent quite a while thinking about the names of these functions.

In drivers/of/fdt.c and include/linux/of_fdt.h, there are several naming
styles in use, such as of_fdt_, of_flat_dt_, and others.

I chose of_fdt_ as the prefix, or namespace, for these functions mainly
because:

1. Compared to of_flat_dt_, it's simpler and shorter, and fdt can represent
   flat_dt, or flattened device tree.

2. of_fdt_ matches the file names drivers/of/fdt.c and include/linux/of_fdt.h better.

3. In the libfdt library, functions consistently use the fdt_ prefix, so using
   a similar of_fdt_ prefix in of/fdt.c seems reasonable.

But if you prefer the of_flat_dt_ nameing convention, I can change it.

> 
> >  extern unsigned long of_get_flat_dt_root(void);
> >  extern uint32_t of_get_flat_dt_phandle(unsigned long node);
> > -- 
> > 2.51.0
> >

On Thu, Nov 13, 2025 at 9:10 PM Yuntao Wang <yuntao.wang@linux.dev> wrote:
>
> On Thu, 13 Nov 2025 16:38:59 -0600, Rob Herring <robh@kernel.org> wrote:
>
> > On Thu, Nov 13, 2025 at 11:50:58PM +0800, Yuntao Wang wrote:
> > > Currently, there are many pieces of nearly identical code scattered across
> > > different places. Consolidate the duplicate code into helper functions to
> > > improve maintainability and reduce the likelihood of errors.
> > >
> > > Signed-off-by: Yuntao Wang <yuntao.wang@linux.dev>
> > > ---
> > >  drivers/of/fdt.c       | 41 +++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/of_fdt.h |  5 +++++
> > >  2 files changed, 46 insertions(+)
> > >
> > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > index 0edd639898a6..5e0eabc1449f 100644
> > > --- a/drivers/of/fdt.c
> > > +++ b/drivers/of/fdt.c
> > > @@ -625,6 +625,47 @@ const void *__init of_get_flat_dt_prop(unsigned long node, const char *name,
> > >     return fdt_getprop(initial_boot_params, node, name, size);
> > >  }
> > >
> > > +const __be32 *__init of_fdt_get_addr_size_prop(unsigned long node,
> > > +                                               const char *name, int *entries)
> > > +{
> > > +   const __be32 *prop;
> > > +   int len, elen = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
> > > +
> > > +   prop = of_get_flat_dt_prop(node, name, &len);
> > > +   if (!prop) {
> > > +           *entries = 0;
> > > +           return NULL;
> > > +   }
> > > +
> > > +   if (len % elen) {
> > > +           *entries = -1;
> >
> > I don't think it's really important to distinguish a length error from
> > any other error. Either we can read the property or we can't.
>
> Hi Rob,
>
> I didn't originally split it into two checks, but later I noticed that in
> __reserved_mem_reserve_reg(), the two error conditions return different
> error codes. I was concerned about breaking compatibility, so I made the
> change this way.
>
> If compatibility isn't an issue, I'd be happy to merge the two checks into one.

You'll have to adjust the handling of -ENOENT case, but yes I think
that is fine. IMO, the kernel can either read and parse a property or
it can't. The exact reason it can't is generally not important.

> > > +           return NULL;
> > > +   }
> > > +
> > > +   *entries = len / elen;
> > > +   return prop;
> > > +}
> > > +
> > > +bool __init of_fdt_get_addr_size(unsigned long node, const char *name,
> > > +                                 u64 *addr, u64 *size)
> > > +{
> > > +   const __be32 *prop;
> > > +   int len, elen = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
> >
> > Still have 2 locations to get the same calculation wrong...
> >
> > > +
> > > +   prop = of_get_flat_dt_prop(node, name, &len);
> > > +   if (!prop || len < elen)
> > > +           return false;
> >
> > Why doesn't calling of_fdt_get_addr_size_prop() work here? If 'len <
> > elen', then 'len % elen' will also be true except in the 0 length case.
> > For that case, of_fdt_get_addr_size_prop() needs to handle it too.
>
> I'm fully in favor of calling of_fdt_get_addr_size_prop() directly here,
> that was my original intention as well, which is also why I placed this
> function right after of_fdt_get_addr_size_prop().
>
> But again, due to compatibility concerns, I had to implement it this way.
>
> For example, suppose prop points to data like:
>
> [addr, size, other data]
>
> With the previous `len < elen` check, addr and size could still be read
> successfully. But if we switch to the `len % elen` check, this type of
> data may fail.

Only if 'other data' is not a multiple of [addr,size], but that's
completely invalid*.

(*The dts format does allow something as complex as '<0x12345678>,
"string", /bits 8/ <0xab>', but you would have to be completely insane
to do that when there's no type information in the DTB.)

> If compatibility is not a concern, I can certainly change it to something
> like the following:
>
> prop = of_fdt_get_addr_size_prop(node, name, &len);
> if (!prop || len != 1)
>   return false;
> >
> > > +
> > > +   of_fdt_read_addr_size(prop, addr, size);
> > > +   return true;
> > > +}
> > > +
> > > +void __init of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size)
> > > +{
> > > +   *addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
> > > +   *size = dt_mem_next_cell(dt_root_size_cells, &prop);
> > > +}
> > > +
> > >  /**
> > >   * of_fdt_is_compatible - Return true if given node from the given blob has
> > >   * compat in its compatible list
> > > diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> > > index b8d6c0c20876..3a0805ff6c7b 100644
> > > --- a/include/linux/of_fdt.h
> > > +++ b/include/linux/of_fdt.h
> > > @@ -55,6 +55,11 @@ extern int of_get_flat_dt_subnode_by_name(unsigned long node,
> > >                                       const char *uname);
> > >  extern const void *of_get_flat_dt_prop(unsigned long node, const char *name,
> > >                                    int *size);
> > > +extern const __be32 *of_fdt_get_addr_size_prop(unsigned long node,
> > > +                                               const char *name, int *entries);
> > > +extern bool of_fdt_get_addr_size(unsigned long node, const char *name,
> > > +                                 u64 *addr, u64 *size);
> > > +extern void of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size);
> > >  extern int of_flat_dt_is_compatible(unsigned long node, const char *name);
> >
> > Looks like of_flat_dt_* would be more consistent with existing naming.
>
> Naming is hard :-)

Indeed.

> I spent quite a while thinking about the names of these functions.
>
> In drivers/of/fdt.c and include/linux/of_fdt.h, there are several naming
> styles in use, such as of_fdt_, of_flat_dt_, and others.

It's a bit of a mess...

> I chose of_fdt_ as the prefix, or namespace, for these functions mainly
> because:
>
> 1. Compared to of_flat_dt_, it's simpler and shorter, and fdt can represent
>    flat_dt, or flattened device tree.
>
> 2. of_fdt_ matches the file names drivers/of/fdt.c and include/linux/of_fdt.h better.
>
> 3. In the libfdt library, functions consistently use the fdt_ prefix, so using
>    a similar of_fdt_ prefix in of/fdt.c seems reasonable.

If we started fresh, I would agree with all of this.

> But if you prefer the of_flat_dt_ nameing convention, I can change it.

I do primarily because that aligns with the other functions which read
specific properties (e.g. of_flat_dt_is_compatible(),
of_flat_dt_translate_address()).

Rob