[U-Boot] [PATCH v4 01/10] imx: Homogenize and fix fuse register definitions

[Benoît Thébaudeau]

Hi Stefano,

On Friday, April 26, 2013 9:31:15 AM, Stefano Babic wrote:
> On 23/04/2013 22:17, Benoît Thébaudeau wrote:
> > IIM:
> >  - Homogenize prg_p naming (the reference manuals are not always
> >  self-consistent
> >    for that).
> >  - Add missing SCSx and bank registers.
> >  - Fix the number of banks on i.MX53.
> > 
> > OCOTP:
> >  - Rename iim to ocotp in order to avoid confusion.
> >  - Rename fuse_data to read_fuse_data, and sticky to sw_sticky, according
> >  to the
> >    reference manual.
> >  - Merge the existing spinoff gp1 fuse definition on i.MX6.
> >  - Fix the number of banks on i.MX6.
> > 
> > Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau at advansee.com>
> > ---
> 
> Hi Benoît.
> 
> I would like to apply your patches and I see some warning from
> clearpatch. Some of them are very annoying and I think to ignore them,
> but before doing that I ask to ML about it. It seems to me that
> checkpatch make stonger tests as in the past, and I do not know if all
> of them are meaningful. Or better, I do not think they are.

Indeed, I have noticed that too since last release. I ignored those on purpose
because they all seem wrong.

> Here the warnings:
> 
> CHECK: Alignment should match open parenthesis
> #589: FILE: common/cmd_fuse.c:77:
> +	if (argc < 2 || strtou32(argv[0], 0, &bank) ||
> +			strtou32(argv[1], 0, &word))
> 
> CHECK: Alignment should match open parenthesis
> #637: FILE: common/cmd_fuse.c:125:
> +			printf("Programming bank %u word 0x%.8x to 0x%.8x...\n",
> +					bank, word, val);
> 
> 
> I would say that this enforcement is crap. The code in the patch is easy
> to read. I want to ignore this warning.

Yes, and that's usual Linux coding style, which is the official coding style for
U-Boot as well.

I'd say that the rule here should be to indent such lines at least at the
position of the opening parenthesis of the previous line, and that the
indentation should be at least 1 or 2 more tabs than on the previous line, which
is the case in my code above.

> WARNING: quoted string split across lines
> #653: FILE: common/cmd_fuse.c:141:
> +			printf("Overriding bank %u word 0x%.8x with "
> +					"0x%.8x...\n", bank, word, val);
> 
> 
> Again, this is the solution when a line is too long. checkpatch
> complains if a line is too long, and in case of static string complains
> that is splitted. This is too much, I would like to ignore also this one.

Yes. The purpose of not splitting long literal strings is only grep-ability of
error messages. Here, it is neither an error message nor a constant sub-string
of it that is split away.

> CHECK: Alignment should match open parenthesis
> #1102: FILE: drivers/misc/fsl_iim.c:118:
> +	if (bank >= ARRAY_SIZE((*regs)->bank) ||
> +			word >= ARRAY_SIZE((*regs)->bank[0].word) ||
> 
> Ditto
> 
> WARNING: line over 80 characters
> #1190: FILE: drivers/misc/fsl_iim.c:206:
> +		puts("fsl_iim fuse_sense(): Explicit sense cycle did not complete\n");
> 
> 
> Ok, this is the one that must be fixed, even if then we have the warning
> with "quoted string"

Initially, this line was less than 80 characters because I had split the string.
But checkpatch complained about the literal string being split, so I kept it on
a single line, which makes sense here for grep-ability: It is an error message,
and all parts of this string are constant (i.e. not printf-ed). And Scott Wood
had already made me do such changes in the past. What should I do?

Would it be possible to have an official detailed coding style everybody agrees
on somewhere? Apparently, the rules are not the same for all custodians.

> WARNING: line over 80 characters
> #2017: FILE: drivers/misc/mxc_ocotp.c:8:
> + *
> http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/tree/drivers/misc/imx_otp.c?h=imx_v2009.08_1.1.0&id=9aa74e6,
> 
> Ditto

I did not know what to do with this one. As it is a URL, I considered it as
unsplittable. So should the 80-character limit really apply here too?

> CHECK: Alignment should match open parenthesis
> #2088: FILE: drivers/misc/mxc_ocotp.c:79:
> +	if (bank >= ARRAY_SIZE((*regs)->bank) ||
> +			word >= ARRAY_SIZE((*regs)->bank[0].fuse_regs) >> 2 ||
> 
> I will not care about this.

OK.

Best regards,
Benoît