On 2015-04-25, Fabio Estevam wrote: > On Sat, Apr 25, 2015 at 3:05 AM, Stefano Babic <sba...@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
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
