[U-Boot] [PATCH v2] fdt: Pass the device serial number through devicetree

[Simon Glass]

Hi Paul,

On 22 May 2015 at 04:55, Paul Kocialkowski <contact at paulk.fr> wrote:
>
> Le jeudi 21 mai 2015 à 14:32 -0600, Simon Glass a écrit :
> > Hi Paul,
> >
> > On 21 May 2015 at 03:27, Paul Kocialkowski <contact at paulk.fr> wrote:
> > > Before device-tree, the device serial number used to be passed to the kernel
> > > using ATAGs (on ARM). This is now deprecated and all the handover to the kernel
> > > should now be done using device-tree. Thus, this passes the serial-number
> > > property to the kernel using the serial-number property of the root node, as
> > > expected by the kernel.
> > >
> > > The serial number is a string that somewhat represents the device's serial
> > > number. It might come from some form of storage (e.g. an eeprom) and be
> > > programmed at factory-time by the manufacturer or come from identification
> > > bits available in e.g. the SoC.
> > >
> > > Signed-off-by: Paul Kocialkowski <contact at paulk.fr>
> >
> > Reviewed-by: Simon Glass <sgj at chromium.org>
> >
> > But please see a nit below.
> >
> > > ---
> > >  common/fdt_support.c              | 25 +++++++++++++++++++++++++
> > >  common/image-fdt.c                |  4 ++++
> > >  doc/device-tree-bindings/root.txt |  4 ++++
> > >  include/fdt_support.h             |  1 +
> > >  4 files changed, 34 insertions(+)
> > >  create mode 100644 doc/device-tree-bindings/root.txt
> > >
> > > diff --git a/common/fdt_support.c b/common/fdt_support.c
> > > index 9e50148..10648b5 100644
> > > --- a/common/fdt_support.c
> > > +++ b/common/fdt_support.c
> > > @@ -194,6 +194,31 @@ static inline int fdt_setprop_uxx(void *fdt, int nodeoffset, const char *name,
> > >                 return fdt_setprop_u32(fdt, nodeoffset, name, (uint32_t)val);
> > >  }
> > >
> > > +int fdt_root(void *fdt)
> > > +{
> > > +       char *serial;
> > > +       int err;
> > > +
> > > +       err = fdt_check_header(fdt);
> > > +       if (err < 0) {
> > > +               printf("fdt_root: %s\n", fdt_strerror(err));
> > > +               return err;
> > > +       }
> > > +
> > > +       serial = getenv("serial#");
> > > +       if (serial) {
> > > +               err = fdt_setprop(fdt, 0, "serial-number", serial,
> > > +                                 strlen(serial) + 1);
> > > +
> > > +               if (err < 0) {
> > > +                       printf("WARNING: could not set serial-number %s.\n",
> > > +                              fdt_strerror(err));
> > > +                       return err;
> > > +               }
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > >
> > >  int fdt_initrd(void *fdt, ulong initrd_start, ulong initrd_end)
> > >  {
> > > diff --git a/common/image-fdt.c b/common/image-fdt.c
> > > index 7e2da7b..80e3e63 100644
> > > --- a/common/image-fdt.c
> > > +++ b/common/image-fdt.c
> > > @@ -471,6 +471,10 @@ int image_setup_libfdt(bootm_headers_t *images, void *blob,
> > >         int ret = -EPERM;
> > >         int fdt_ret;
> > >
> > > +       if (fdt_root(blob) < 0) {
> > > +               printf("ERROR: root node setup failed\n");
> > > +               goto err;
> > > +       }
> > >         if (fdt_chosen(blob) < 0) {
> > >                 printf("ERROR: /chosen node create failed\n");
> > >                 goto err;
> > > diff --git a/doc/device-tree-bindings/root.txt b/doc/device-tree-bindings/root.txt
> > > new file mode 100644
> > > index 0000000..001ccf3
> > > --- /dev/null
> > > +++ b/doc/device-tree-bindings/root.txt
> > > @@ -0,0 +1,4 @@
> > > +The root node
> > > +
> > > +Optional properties:
> > > +  - serial-number : a string representing the device's serial number
> > > diff --git a/include/fdt_support.h b/include/fdt_support.h
> > > index 5d4f28d..56185c9 100644
> > > --- a/include/fdt_support.h
> > > +++ b/include/fdt_support.h
> > > @@ -16,6 +16,7 @@ u32 fdt_getprop_u32_default_node(const void *fdt, int off, int cell,
> > >                                 const char *prop, const u32 dflt);
> > >  u32 fdt_getprop_u32_default(const void *fdt, const char *path,
> > >                                 const char *prop, const u32 dflt);
> > > +int fdt_root(void *fdt);
> >
> > Please can you add a comment for this in the standard style?
>
> As far as I can see, fdt_root plays a similar role to fdt_chosen and
> fdt_initrd, all of which are defined in fdt_support.c and used in
> image-fdt.c's image_setup_libfdt.
>
> Their prototypes are defined in fdt_support.h and neither fdt_chosen nor
> fdt_initrd have such a comment, so I didn't think it was very consistent
> to add one for fdt_root when writing the patch.
>
> Now if you think it's worth adding such a comment for the sake of
> documentation, I don't object to it but it still leaves me with a
> feeling of inconsistency with regard to other similar prototypes.

You want uncommented code for consistency?? :-)

If you are concerned, you could add a new patch that adds comments for
some other functions in fdt_support.h. That would be welcome. But at
least, please add comments for new code.

The problem with that is that no one is going to go through and
comment all these APIs. Generally the only way the code quality
improves is when people add new features or refactor the code for some
reason.

>
> > >  int fdt_chosen(void *fdt);
> > >  int fdt_initrd(void *fdt, ulong initrd_start, ulong initrd_end);
> > >  void do_fixup_by_path(void *fdt, const char *path, const char *prop,
> > > --
> > > 1.9.1
> > >

Regards,
Simon