From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vagrant Cascadian Date: Sat, 25 Apr 2015 10:02:56 -0700 Subject: [U-Boot] [U-Boot, v3, 5/5] mx6cuboxi: Load the correct 'fdt_file' variable In-Reply-To: References: <1429876015-22540-5-git-send-email-festevam@gmail.com> <87zj5xozr3.fsf@aikidev.net> <553B2EC1.8080203@denx.de> Message-ID: <87tww4p2db.fsf@aikidev.net> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 2015-04-25, Fabio Estevam wrote: > On Sat, Apr 25, 2015 at 3:05 AM, Stefano Babic 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: