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 | 9 +++++++++
2 files changed, 50 insertions(+)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 0edd639898a6..0c18bdefbbee 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_flat_dt_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 || len % elen) {
+ *entries = 0;
+ return NULL;
+ }
+
+ *entries = len / elen;
+ return prop;
+}
+
+bool __init of_flat_dt_get_addr_size(unsigned long node, const char *name,
+ u64 *addr, u64 *size)
+{
+ const __be32 *prop;
+ int entries;
+
+ prop = of_flat_dt_get_addr_size_prop(node, name, &entries);
+ if (!prop || entries != 1)
+ return false;
+
+ of_flat_dt_read_addr_size(prop, 0, addr, size);
+ return true;
+}
+
+void __init of_flat_dt_read_addr_size(const __be32 *prop, int entry_index,
+ u64 *addr, u64 *size)
+{
+ int entry_cells = dt_root_addr_cells + dt_root_size_cells;
+ prop += entry_cells * entry_index;
+
+ *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..51dadbaa3d63 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -55,6 +55,15 @@ 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_flat_dt_get_addr_size_prop(unsigned long node,
+ const char *name,
+ int *entries);
+extern bool of_flat_dt_get_addr_size(unsigned long node, const char *name,
+ u64 *addr, u64 *size);
+extern void of_flat_dt_read_addr_size(const __be32 *prop, int entry_index,
+ 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.0On Sat, Nov 15, 2025 at 09:47:46PM +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. Not much improved. Please go to previous version and read the comments. Best regards, Krzysztof
On Mon, 17 Nov 2025 08:01:59 +0100, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Sat, Nov 15, 2025 at 09:47:46PM +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.
>
> Not much improved. Please go to previous version and read the comments.
>
> Best regards,
> Krzysztof
Hi Krzysztof,
scripts/checkpatch.pl indeed still reports some warnings. I noticed them,
but I intentionally didn't fix them.
Below is a list of all the warnings, along with my reasons for leaving
them unaddressed.
1. WARNING: Missing a blank line after declarations
#60: FILE: drivers/of/fdt.c:663:
+ int entry_cells = dt_root_addr_cells + dt_root_size_cells;
+ prop += entry_cells * entry_index;
The function that triggers this warning is:
void __init of_flat_dt_read_addr_size(const __be32 *prop, int entry_index,
u64 *addr, u64 *size)
{
int entry_cells = dt_root_addr_cells + dt_root_size_cells;
prop += entry_cells * entry_index;
*addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
*size = dt_mem_next_cell(dt_root_size_cells, &prop);
}
The warning suggests adding a blank line before
prop += entry_cells * entry_index;
I didn't add it because, logically,
int entry_cells = dt_root_addr_cells + dt_root_size_cells;
prop += entry_cells * entry_index;
forms a single block, just like
*addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
*size = dt_mem_next_cell(dt_root_size_cells, &prop);
I think the code is more readable without the blank line.
In fact, I initially combined these two lines
int entry_cells = dt_root_addr_cells + dt_root_size_cells;
prop += entry_cells * entry_index;
into a single line:
prop += (dt_root_addr_cells + dt_root_size_cells) * entry_index;
I added the entry_cells local variable specifically to improve readability.
2. CHECK: extern prototypes should be avoided in .h files
#78: FILE: include/linux/of_fdt.h:59:
+extern const __be32 *of_flat_dt_get_addr_size_prop(unsigned long node,
This is another warning reported by `scripts/checkpatch.pl --strict`.
This warning says that `extern` should be removed.
The reason I didn't remove it was to maintain consistency with the existing
function declarations.
In include/linux/of_fdt.h, all function declarations use the `extern` keyword.
Yes, the Linux kernel coding style document
https://docs.kernel.org/process/coding-style.html#function-prototypes
emphasizes
Do not use the extern keyword with function declarations as this makes lines longer and isn’t strictly necessary.
I agree with this.
However, if extern needs to be removed, I think it should be done in a
separate patch that removes all instances of extern.
Thanks,
Yuntao
Hi Yuntao,
On Mon, 17 Nov 2025 at 12:57, Yuntao Wang <yuntao.wang@linux.dev> wrote:
> On Mon, 17 Nov 2025 08:01:59 +0100, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On Sat, Nov 15, 2025 at 09:47:46PM +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.
> >
> > Not much improved. Please go to previous version and read the comments.
> >
> > Best regards,
> > Krzysztof
>
> Hi Krzysztof,
>
> scripts/checkpatch.pl indeed still reports some warnings. I noticed them,
> but I intentionally didn't fix them.
>
> Below is a list of all the warnings, along with my reasons for leaving
> them unaddressed.
>
> 1. WARNING: Missing a blank line after declarations
> #60: FILE: drivers/of/fdt.c:663:
> + int entry_cells = dt_root_addr_cells + dt_root_size_cells;
> + prop += entry_cells * entry_index;
>
> The function that triggers this warning is:
>
> void __init of_flat_dt_read_addr_size(const __be32 *prop, int entry_index,
> u64 *addr, u64 *size)
> {
> int entry_cells = dt_root_addr_cells + dt_root_size_cells;
> prop += entry_cells * entry_index;
>
> *addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
> *size = dt_mem_next_cell(dt_root_size_cells, &prop);
> }
>
> The warning suggests adding a blank line before
>
> prop += entry_cells * entry_index;
>
> I didn't add it because, logically,
>
> int entry_cells = dt_root_addr_cells + dt_root_size_cells;
> prop += entry_cells * entry_index;
>
> forms a single block, just like
>
> *addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
> *size = dt_mem_next_cell(dt_root_size_cells, &prop);
>
> I think the code is more readable without the blank line.
>
> In fact, I initially combined these two lines
>
> int entry_cells = dt_root_addr_cells + dt_root_size_cells;
> prop += entry_cells * entry_index;
>
> into a single line:
>
> prop += (dt_root_addr_cells + dt_root_size_cells) * entry_index;
>
> I added the entry_cells local variable specifically to improve readability.
What about:
void __init of_flat_dt_read_addr_size(const __be32 *prop, int entry_index,
u64 *addr, u64 *size)
{
int entry_cells = dt_root_addr_cells + dt_root_size_cells;
prop += entry_cells * entry_index;
*addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
*size = dt_mem_next_cell(dt_root_size_cells, &prop);
}
?
1. entry_cells is an intermediate variable,
2. prop is prepared just before its use.
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 Mon, 17 Nov 2025 13:34:20 +0100, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Yuntao,
>
> On Mon, 17 Nov 2025 at 12:57, Yuntao Wang <yuntao.wang@linux.dev> wrote:
> > On Mon, 17 Nov 2025 08:01:59 +0100, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > On Sat, Nov 15, 2025 at 09:47:46PM +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.
> > >
> > > Not much improved. Please go to previous version and read the comments.
> > >
> > > Best regards,
> > > Krzysztof
> >
> > Hi Krzysztof,
> >
> > scripts/checkpatch.pl indeed still reports some warnings. I noticed them,
> > but I intentionally didn't fix them.
> >
> > Below is a list of all the warnings, along with my reasons for leaving
> > them unaddressed.
> >
> > 1. WARNING: Missing a blank line after declarations
> > #60: FILE: drivers/of/fdt.c:663:
> > + int entry_cells = dt_root_addr_cells + dt_root_size_cells;
> > + prop += entry_cells * entry_index;
> >
> > The function that triggers this warning is:
> >
> > void __init of_flat_dt_read_addr_size(const __be32 *prop, int entry_index,
> > u64 *addr, u64 *size)
> > {
> > int entry_cells = dt_root_addr_cells + dt_root_size_cells;
> > prop += entry_cells * entry_index;
> >
> > *addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
> > *size = dt_mem_next_cell(dt_root_size_cells, &prop);
> > }
> >
> > The warning suggests adding a blank line before
> >
> > prop += entry_cells * entry_index;
> >
> > I didn't add it because, logically,
> >
> > int entry_cells = dt_root_addr_cells + dt_root_size_cells;
> > prop += entry_cells * entry_index;
> >
> > forms a single block, just like
> >
> > *addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
> > *size = dt_mem_next_cell(dt_root_size_cells, &prop);
> >
> > I think the code is more readable without the blank line.
> >
> > In fact, I initially combined these two lines
> >
> > int entry_cells = dt_root_addr_cells + dt_root_size_cells;
> > prop += entry_cells * entry_index;
> >
> > into a single line:
> >
> > prop += (dt_root_addr_cells + dt_root_size_cells) * entry_index;
> >
> > I added the entry_cells local variable specifically to improve readability.
>
> What about:
>
> void __init of_flat_dt_read_addr_size(const __be32 *prop, int entry_index,
> u64 *addr, u64 *size)
> {
> int entry_cells = dt_root_addr_cells + dt_root_size_cells;
>
> prop += entry_cells * entry_index;
> *addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
> *size = dt_mem_next_cell(dt_root_size_cells, &prop);
> }
>
> ?
>
> 1. entry_cells is an intermediate variable,
> 2. prop is prepared just before its use.
Hi Geert,
Yes, if this warning really needs to be fixed, that's exactly how it should be done.
Thanks,
Yuntao
>
> 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
© 2016 - 2026 Red Hat, Inc.