[PATCH 2/3] Add fdt network helper functions

[Ramon Fried]

On Thu, Aug 12, 2021 at 12:12 PM Tony Dinh <mibodhi at gmail.com> wrote:
>
> Hi Stefan,
>
> On Wed, Aug 11, 2021 at 11:15 PM Stefan Roese <sr at 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 at 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 at 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 at gmail.com>