[U-Boot] [PATCH 2/6] fdtdec: Implement fdtdec_get_max_phandle()

[Simon Glass]

Hi Thierry,

On Mon, 11 Mar 2019 at 17:27, Thierry Reding <thierry.reding at gmail.com> wrote:
>
> On Sun, Mar 10, 2019 at 03:51:31PM -0600, Simon Glass wrote:
> > Hi Thierry,
> >
> > On Fri, 8 Mar 2019 at 13:11, Thierry Reding <thierry.reding at gmail.com> wrote:
> > >
> > > From: Thierry Reding <treding at nvidia.com>
> > >
> > > This function allows looking up the highest phandle value stored in a
> > > device tree, which is useful to determine the next best phandle value
> > > for new nodes.
> > >
> > > Signed-off-by: Thierry Reding <treding at nvidia.com>
> > > ---
> > >  include/fdtdec.h | 12 ++++++++++++
> > >  lib/fdtdec.c     | 28 ++++++++++++++++++++++++++++
> > >  2 files changed, 40 insertions(+)
> >
> > Can we use fdt_get_max_phandle() instead? If not, could you please add
> > a bit more detail to the commit message as we might consider changing
> > the upstream function.
>
> fdt_get_max_phandle() has a slightly awkward signature. It returns the
> phandle via the return value, which means that it can distinguish
> between error conditions and also uses 0 and -1 to signal no phandle
> found and error conditions, respectively. Using the function requires
> weird looking code like this:
>
>         uint32_t phandle;
>
>         phandle = fdt_get_max_phandle(fdt);
>         if (phandle == 0 || phandle == (uint32_t)-1)
>                 /* process error */;
>
> So the goal here was to add an alternative that would provide a more
> standard interface where a regular error could be returned via the
> return value and the phandle would be stored in an output parameter on
> success.
>
> I specifically didn't want to update the upstream function because it
> was introduced about 2.5 years ago and will probably be used by some
> number of users. I'm not sure if there's a stable API policy for libfdt,
> but I would expect a lot of users to be annoyed if we just changed the
> signature of fdt_get_max_phandle().
>
> That said, perhaps another alternative would be to implement this as a
> different function. If you look at the subsequent patches, the goal is
> to generate a new phandle for newly added nodes, so perhaps something
> like this could work:
>
>         int fdtdec_generate_phandle(const void *fdt, uint32_t *phandle);

That seems like a good idea.
>
> That would be slightly more in line with the higher level of fdtdec
> compared to libfdt.
>
> Or perhaps you'd prefer fdt_generate_phandle() in libfdt?

Well yes, then David weighs in and we avoid a blind alley which won't
pass muster upstream. If you do send an upstream patch, make sure to
add tests.

Regards,
Simon