Re: [PATCH v3 1/8] of/fdt: Consolidate duplicate code into helper functions

[Geert Uytterhoeven]

Hi Yuntao,

On Mon, 17 Nov 2025 at 12:57, Yuntao Wang <yuntao.wang@xxxxxxxxx> wrote:
> On Mon, 17 Nov 2025 08:01:59 +0100, Krzysztof Kozlowski <krzk@xxxxxxxxxx> 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@xxxxxxxxxxxxxx

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