Change in flashrom[master]: lspcon_i2c_spi.c: Fix style

[Shiyu Sun (Code Review)]

Shiyu Sun has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/47874 )

Change subject: lspcon_i2c_spi.c: Fix style
......................................................................

lspcon_i2c_spi.c: Fix style

Avoid define the variable i at the for loop statement.

BUG=None
TEST=None

Signed-off-by: Shiyu Sun 
Change-Id: I4068ae7bdf6e053606cb063f7781dbb2d77792ff
---
M lspcon_i2c_spi.c
1 file changed, 2 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/74/47874/1

diff --git a/lspcon_i2c_spi.c b/lspcon_i2c_spi.c
index 7b9f1c0..f468031 100644
--- a/lspcon_i2c_spi.c
+++ b/lspcon_i2c_spi.c
@@ -377,6 +377,7 @@
    			unsigned int start, unsigned int len)
 {
    int ret = 0;
+	unsigned int i;
    if (start & 0xff)
    	return default_spi_write_256(flash, buf, start, len);

@@ -389,7 +390,7 @@
    ret |= lspcon_i2c_spi_enable_hw_write(fd);
    ret |= lspcon_i2c_clt2_spi_reset(fd);

-	for (unsigned int i = 0; i < len; i += PAGE_SIZE) {
+	for (i = 0; i < len; i += PAGE_SIZE) {
    	ret |= lspcon_i2c_spi_map_page(fd, start + i);
    	ret |= lspcon_i2c_spi_write_page(fd, buf + i, min(len - i, PAGE_SIZE));
    }

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47874 )

Change subject: lspcon_i2c_spi.c: Fix style
......................................................................

Patch Set 1: Code-Review+2

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47874 )

Change subject: lspcon_i2c_spi.c: Fix style
......................................................................

Patch Set 1: Code-Review-2

C99 should allow for the iterator declaration to be inside the loop construct predicate. I think the CrOS tree should be fixed and not here imho.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47874 )

Change subject: lspcon_i2c_spi.c: Fix style
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review-2
>
> C99 should allow for the iterator declaration to be inside the loop construct predicate. I think the CrOS tree should be fixed and not here imho.

c.f. CB:38034

[Shiyu Sun (Code Review)]

Shiyu Sun has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/47874 )

Change subject: lspcon_i2c_spi.c: Fix style
......................................................................

Abandoned

Fix not necessary

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47874 )

Change subject: lspcon_i2c_spi.c: Fix style
......................................................................

Patch Set 1:

What? No! Don't abandon this change, please!

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47874 )

Change subject: lspcon_i2c_spi.c: Fix style
......................................................................

Patch Set 1:
> Patch Set 1:
>
> What? No! Don't abandon this change, please!

Please no more unnecessary C89'ism. Just use C99 already.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47874 )

Change subject: lspcon_i2c_spi.c: Fix style
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> >
> > What? No! Don't abandon this change, please!
>
> Please no more unnecessary C89'ism. Just use C99 already.

Did you check the change I linked to earlier? Old GCC versions don't like these kinds of initializers, and it caused problems at some point. And even if that is no longer an issue, the other three loops in this file as well as nearly every other file in flashrom use the C89 style.

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47874 )

Change subject: lspcon_i2c_spi.c: Fix style
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> > > Patch Set 1:
> > >
> > > What? No! Don't abandon this change, please!
> >
> > Please no more unnecessary C89'ism. Just use C99 already.
>
> Did you check the change I linked to earlier? Old GCC versions don't like these kinds of initializers, and it caused problems at some point. And even if that is no longer an issue, the other three loops in this file as well as nearly every other file in flashrom use the C89 style.

I did indeed check. We shouldn't walk backwards though. If your GCC is so old that it doesn't support C99 (~20years) I don't we should support that. The issue above is that newer GCC's since 8-9y ago? defaulted to C99 and so you don't need to be explicit about -std=c99 as it is assumed. However just assuming isn't good either, we should be explicit about using C99 and fix C89'ism across the code base.

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47874 )

Change subject: lspcon_i2c_spi.c: Fix style
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> > > Patch Set 1:
> > > > Patch Set 1:
> > > >
> > > > What? No! Don't abandon this change, please!
> > >
> > > Please no more unnecessary C89'ism. Just use C99 already.
> >
> > Did you check the change I linked to earlier? Old GCC versions don't like these kinds of initializers, and it caused problems at some point. And even if that is no longer an issue, the other three loops in this file as well as nearly every other file in flashrom use the C89 style.
>
> I did indeed check. We shouldn't walk backwards though. If your GCC is so old that it doesn't support C99 (~20years) I don't we should support that. The issue above is that newer GCC's since 8-9y ago? defaulted to C99 and so you don't need to be explicit about -std=c99 as it is assumed. However just assuming isn't good either, we should be explicit about using C99 and fix C89'ism across the code base.

https://review.coreboot.org/c/flashrom/+/47908

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47874 )

Change subject: lspcon_i2c_spi.c: Fix style
......................................................................

Patch Set 1:

Please just make use of the existing tools to settle such things,
e.g. checkpatch seems right for this as we follow the Linux coding
style.

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47874 )

Change subject: lspcon_i2c_spi.c: Fix style
......................................................................

Patch Set 1:

Shiyu, I think we should follow the communities wisdom here until we can establish the issues of defaulting to C99. Can you try with checkpatch to determine the right course of action for this patch? I'll handle the C99 discussion piece and try to coordinate that back to you to keep us all aligned.

Thanks Nico and Angel for helping to coordinate the other side here. Hopefully we can figure out the blockers for C99 over C89 idioms on the ML.

[Shiyu Sun (Code Review)]

Shiyu Sun has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/47874 )

Change subject: lspcon_i2c_spi.c: Fix style
......................................................................

Patch Set 1:
> Patch Set 1:
>
> Shiyu, I think we should follow the communities wisdom here until we can establish the issues of defaulting to C99. Can you try with checkpatch to determine the right course of action for this patch? I'll handle the C99 discussion piece and try to coordinate that back to you to keep us all aligned.
>
> Thanks Nico and Angel for helping to coordinate the other side here. Hopefully we can figure out the blockers for C99 over C89 idioms on the ML.

Not sure how exactly we suppose to use that checkpatch, I tried with './checkpatch.pl -f --terse --no-tree lspcon_i2c_spi.c' and looks like it give me nothing useful regarding the declaration. Paste the output here:
lspcon_i2c_spi.c:1: WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
lspcon_i2c_spi.c:72: WARNING: struct  should normally be const
lspcon_i2c_spi.c:76: WARNING: do not add new typedefs
lspcon_i2c_spi.c:83: WARNING: line over 80 characters
lspcon_i2c_spi.c:86: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:92: WARNING: line over 80 characters
lspcon_i2c_spi.c:95: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:107: ERROR: code indent should use tabs where possible
lspcon_i2c_spi.c:107: WARNING: please, no spaces at the start of a line
lspcon_i2c_spi.c:113: WARNING: line over 80 characters
lspcon_i2c_spi.c:116: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:119: WARNING: line over 80 characters
lspcon_i2c_spi.c:123: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:131: WARNING: line over 80 characters
lspcon_i2c_spi.c:132: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:137: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:137: WARNING: braces {} are not necessary for single statement blocks
lspcon_i2c_spi.c:138: WARNING: line over 80 characters
lspcon_i2c_spi.c:147: WARNING: line over 80 characters
lspcon_i2c_spi.c:152: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:169: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:170: WARNING: line over 80 characters
lspcon_i2c_spi.c:175: WARNING: line over 80 characters
lspcon_i2c_spi.c:176: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:177: WARNING: line over 80 characters
lspcon_i2c_spi.c:179: WARNING: line over 80 characters
lspcon_i2c_spi.c:199: WARNING: line over 80 characters
lspcon_i2c_spi.c:211: WARNING: line over 80 characters
lspcon_i2c_spi.c:223: WARNING: line over 80 characters
lspcon_i2c_spi.c:232: WARNING: line over 80 characters
lspcon_i2c_spi.c:238: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:248: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:260: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:262: ERROR: code indent should use tabs where possible
lspcon_i2c_spi.c:262: WARNING: please, no spaces at the start of a line
lspcon_i2c_spi.c:267: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:271: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:274: WARNING: line over 80 characters
lspcon_i2c_spi.c:275: WARNING: line over 80 characters
lspcon_i2c_spi.c:275: WARNING: Block comments use * on subsequent lines
lspcon_i2c_spi.c:276: ERROR: code indent should use tabs where possible
lspcon_i2c_spi.c:276: WARNING: Block comments use a trailing */ on a separate line
lspcon_i2c_spi.c:278: WARNING: line over 80 characters
lspcon_i2c_spi.c:283: WARNING: line over 80 characters
lspcon_i2c_spi.c:288: WARNING: braces {} are not necessary for single statement blocks
lspcon_i2c_spi.c:289: WARNING: line over 80 characters
lspcon_i2c_spi.c:300: WARNING: line over 80 characters
lspcon_i2c_spi.c:300: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:301: WARNING: line over 80 characters
lspcon_i2c_spi.c:313: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:314: WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
lspcon_i2c_spi.c:314: WARNING: struct  should normally be const
lspcon_i2c_spi.c:315: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:324: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:334: WARNING: line over 80 characters
lspcon_i2c_spi.c:340: WARNING: struct  should normally be const
lspcon_i2c_spi.c:345: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:349: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:354: WARNING: line over 80 characters
lspcon_i2c_spi.c:360: WARNING: line over 80 characters
lspcon_i2c_spi.c:363: ERROR: code indent should use tabs where possible
lspcon_i2c_spi.c:364: ERROR: code indent should use tabs where possible
lspcon_i2c_spi.c:365: ERROR: code indent should use tabs where possible
lspcon_i2c_spi.c:367: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:370: WARNING: line over 80 characters
lspcon_i2c_spi.c:373: WARNING: line over 80 characters
lspcon_i2c_spi.c:380: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:384: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:394: WARNING: line over 80 characters
lspcon_i2c_spi.c:407: ERROR: code indent should use tabs where possible
lspcon_i2c_spi.c:407: WARNING: please, no spaces at the start of a line
lspcon_i2c_spi.c:411: WARNING: struct  should normally be const
lspcon_i2c_spi.c:425: ERROR: code indent should use tabs where possible
lspcon_i2c_spi.c:425: WARNING: please, no spaces at the start of a line
lspcon_i2c_spi.c:425: WARNING: struct  should normally be const
lspcon_i2c_spi.c:426: WARNING: struct  should normally be const
lspcon_i2c_spi.c:428: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:440: ERROR: code indent should use tabs where possible
lspcon_i2c_spi.c:440: WARNING: please, no spaces at the start of a line
lspcon_i2c_spi.c:443: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:445: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:451: WARNING: line over 80 characters
lspcon_i2c_spi.c:452: ERROR: code indent should use tabs where possible
lspcon_i2c_spi.c:452: WARNING: please, no spaces at the start of a line
lspcon_i2c_spi.c:458: ERROR: code indent should use tabs where possible
lspcon_i2c_spi.c:458: WARNING: please, no spaces at the start of a line
lspcon_i2c_spi.c:478: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:483: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:492: WARNING: line over 80 characters
lspcon_i2c_spi.c:492: WARNING: struct  should normally be const
lspcon_i2c_spi.c:493: WARNING: Missing a blank line after declarations
lspcon_i2c_spi.c:494: WARNING: line over 80 characters

That looks like we have plenty of minor style to fix but there is nothing related to the declaration. (I tried with declaration in and outside the for loop and it gives no difference in report). Maybe need further investigate on how to use the tool.