Re: [PATCH 2/3] Add fdt network helper functions

On Thu, Aug 12, 2021 at 12:12 PM Tony Dinh mibodhi@gmail.com wrote:

Hi Stefan,

On Wed, Aug 11, 2021 at 11:15 PM Stefan Roese sr@denx.de wrote:

...

Hi Tony,

a few nits...

On 06.08.21 06:49, Tony Dinh wrote:

...

Add fdt network helper functions common/fdt_support_net.c

Signed-off-by: Tony Dinh mibodhi@gmail.com

common/fdt_support_net.c | 46 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 common/fdt_support_net.c

diff --git a/common/fdt_support_net.c b/common/fdt_support_net.c new file mode 100644 index 0000000000..06fa2175b4 --- /dev/null +++ b/common/fdt_support_net.c @@ -0,0 +1,46 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (C) 2021 Tony Dinh mibodhi@gmail.com
• */

+#include <common.h>

Including "common.h" is deprecated. Please remove it and include the missing other headers instead (if any).

...

+#include <linux/libfdt.h> +#include <fdt_support.h> +#include <asm/global_data.h> +#include <fdt_support_net.h>

Sorting would be good as well.

...

+DECLARE_GLOBAL_DATA_PTR;

+int fdt_get_phy_addr(const char *path) +{

• const void *fdt = gd->fdt_blob;
  

• const u32 *reg;
  

• const u32 *val;
  

• int node, phandle, addr;
  

• /* Find the node by its full path */
  

• node = fdt_path_offset(fdt, path);
  

• if (node >= 0) {
  

•         /* Look up phy-handle */
  

•         val = fdt_getprop(fdt, node, "phy-handle", NULL);
  

•         if (!val)
  

•                 /* Look up phy (deprecated property for phy handle) */
  

•                 val = fdt_getprop(fdt, node, "phy", NULL);
  

Above is a multi-line statement which is better included in parentheses.

And I personally would add an empty line here.

...

•         if (val) {
  

•                 phandle = fdt32_to_cpu(*val);
  

•                 if (!phandle)
  

•                         return -1;
  

Could you instead of returning "-1" return some matching error code defines?

And I personally would add an empty line here.

...

•                 /* Follow it to its node */
  

•                 node = fdt_node_offset_by_phandle(fdt, phandle);
  

•                 if (node) {
  

•                         /* Look up reg */
  

•                         reg = fdt_getprop(fdt, node, "reg", NULL);
  

•                         if (reg) {
  

•                                 addr = fdt32_to_cpu(*reg);
  

•                                 return addr;
  

•                         }
  

•                 }
  

•         }
  

• }
  

• return -1;
  

Again, please change to some matching error define here.

Will do all of the above and send in the V2 patch, after Ramon and Joe had any feedback.

Thanks, Tony

...

Thanks, Stefan

Acked-by: Ramon Fried rfried.dev@gmail.com