From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Tue, 19 Mar 2019 09:24:30 +0800 Subject: [U-Boot] [PATCH 2/6] fdtdec: Implement fdtdec_get_max_phandle() In-Reply-To: <20190311092721.GA11545@ulmo> References: <20190308201140.2383-1-thierry.reding@gmail.com> <20190308201140.2383-2-thierry.reding@gmail.com> <20190311092721.GA11545@ulmo> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Thierry, On Mon, 11 Mar 2019 at 17:27, Thierry Reding 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 wrote: > > > > > > From: Thierry Reding > > > > > > 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 > > > --- > > > 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