[U-Boot] [U-Boot, v3, 5/5] mx6cuboxi: Load the correct 'fdt_file' variable

[Vagrant Cascadian <vagrant@aikidev.net>]

On 2015-04-25, Fabio Estevam wrote:
> On Sat, Apr 25, 2015 at 3:05 AM, Stefano Babic <sbabic@denx.de> wrote:
>
>> Are you sure ? I think Fabio's intention is to have setenv fdt_file as
>> part of check_suffix, and it is not if you add a trailing \0
>
> That's correct.

Yes, I understood that intention, but there's no \0 in check_suffix now,
which means that whatever comes after the check_suffix code will be
appended to check_suffix. The check_suffix code needs a \0 to define
where it ends and the next line begins...


>>> and maybe should
>>> be indented to line up with the if statement:
>>>
>>>   +           "setenv fdt_file ${dts_prefix}${dts_suffix}\0" \
>>
>> If checkpatch does not complain...
>
> checkpatch did not complain, but for better readability I could do as
> Vagrant suggested and write it like:
>
>   #define CONFIG_EXTRA_ENV_SETTINGS \
>        "script=boot.scr\0" \
>        "image=zImage\0" \
>  -     "fdt_file=" CONFIG_DEFAULT_FDT_FILE "\0" \
>  +     "check_suffix=" \
>  +             "if is_hummingboard; then " \
>  +                     "setenv dts_suffix -hummingboard.dtb;" \
>  +             "else " \
>  +                     "setenv dts_suffix -cubox-i.dtb;" \
>  +             "fi; "\
>  +             "setenv fdt_file ${dts_prefix}${dts_suffix};" \

Just to be clearer about my earlier point, I think you really want the
\0 at the end of check_suffix:

 #define CONFIG_EXTRA_ENV_SETTINGS \
      "script=boot.scr\0" \
      "image=zImage\0" \
-     "fdt_file=" CONFIG_DEFAULT_FDT_FILE "\0" \
+     "check_suffix=" \
+             "if is_hummingboard; then " \
+                     "setenv dts_suffix -hummingboard.dtb;" \
+             "else " \
+                     "setenv dts_suffix -cubox-i.dtb;" \
+             "fi; "\
+             "setenv fdt_file ${dts_prefix}${dts_suffix}\0" \


I'm also wondering if "check_suffix" is a good description for the code;
It's actually setting the dts_suffix and fdt_file variables. Some of the
ti boards call their corresponding code "findfdt" which seems more
accurate, although something like "set_fdt_vars" might even more
appropriate.

Another minor point: the variables are actually working with dtb files,
not dts files. I think the dts_prefix/dts_suffix should probably be
named dtb_prefix/dtb_suffix or fdt_prefix/fdt_suffix.

And while I'm at it, Dare I make the case again for fdtfile
vs. fdt_file?


Thanks for working on getting hummingboard/cubox-i support into mainline
u-boot!

live well,
  vagrant
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150425/10ff1a40/attachment.sig>