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

[Yuntao Wang]

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.

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