Le jeudi 21 mai 2015 à 14:32 -0600, Simon Glass a écrit :

Hi Paul,

On 21 May 2015 at 03:27, Paul Kocialkowski contact@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@paulk.fr

Reviewed-by: Simon Glass sgj@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.

...

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

Hi Simon,

[…]

...

...

...

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.

Fair enough, I've just sent out another patch that adds (minimalistic) documentation for all 3 functions.

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.

I understand.

Thanks for the review!

...

...

...

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