Re: [cbootimage PATCH 1/1] Add Tegra124 support

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3553 bytes --]

On Mon, Aug 26, 2013 at 11:01:36AM -0600, Stephen Warren wrote:
> On 08/26/2013 01:47 AM, Thierry Reding wrote:
> > On Fri, Aug 23, 2013 at 01:05:11PM -0600, Stephen Warren wrote:
> >> On 08/22/2013 09:46 PM, Penny Chiu wrote:
> >>> Add the Tegra124 chip support to cbootimage. User can use "-t124" as
> >>> option to parse .cfg and generate BCT/image for Tegra124.
> >>
> >> This looks fine to me at a quick glance. Just one small comment below:
> >>
> >>> diff --git a/src/cbootimage.c b/src/cbootimage.c
> >>
> >>>  	printf("Usage: cbootimage [options] configfile imagename\n");
> >>>  	printf("    options:\n");
> >>> -	printf("    -h, --help, -?        Display this message.\n");
> >>> -	printf("    -d, --debug           Output debugging information.\n");
> >>> -	printf("    -gbct                 Generate the new bct file.\n");
> >>> -	printf("    -o<ODM_DATA>          Specify the odm_data(in hex).\n");
> >>> -	printf("    [-t20|-t30|-t114]     Select one of the possible target devices,\n");
> >>> -	printf("                          -t20 if unspecified.\n");
> >>> -	printf("    configfile            File with configuration information\n");
> >>> -	printf("    imagename             Output image name\n");
> >>> +	printf("    -h, --help, -?              Display this message.\n");
> >>> +	printf("    -d, --debug                 Output debugging information.\n");
> >>> +	printf("    -gbct                       Generate the new bct file.\n");
> >>> +	printf("    -o<ODM_DATA>                Specify the odm_data(in hex).\n");
> >>> +	printf("    [-t20|-t30|-t114|-t124]     Select one of the possible target devices,\n");
> >>> +	printf("                                -t20 if unspecified.\n");
> >>> +	printf("    configfile                  File with configuration information\n");
> >>> +	printf("    imagename                   Output image name\n");
> >>>  }
> >>
> >> To avoid continually re-formatting that text when adding new SoCs, why
> >> not start the description of the -tNN options on a separate line, and
> >> leave the indentation as-is for all the unmodified lines. i.e.:
> >>
> >>>> +	printf("    [-t20|-t30|-t114|-t124]\n");
> >>>> +	printf("                          Select one of the possible target devices,\n");
> >>>> +	printf("                          -t20 if unspecified.\n");
> > 
> > Perhaps in the long run it would be better to turn this into something
> > like:
> > 
> > 	printf("    -t, --target SOC      Select target device. Must be one of:\n");
> > 	printf("                          20, 30, 114, 124 (default: 20)\n");
> 
> I don't like the idea of "--target 20", since "20" isn't an SoC name;
> it's "tegra20" or just perhaps "t20".

I'd really like to get some kind of consistency. We've been using the
tegra20/tegra30/tegra114 variants pretty consistently within the kernel
and U-Boot, but other tools use the abbreviated form (t20/t30/t114).
Actually I think this is the only one that uses the abbreviation, so...

> I'd be fine with a patch that allowed "-SOC" or "--target SOC", with
> SOC being "t20", "t30", ...

... perhaps introducing a --target {tegra20,tegra30,tegra114,tegra124}
option that deprecates -t20/-t30/-t114/-t124 would be a good option. I
for one sometimes get confused and can't remember the correct form to
use for cbootimage. It's mostly scripted these days, so that issue is
not that important, but using one variant only will also help to make
the various scripts consistent and easier to maintain.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]