Change in flashrom[master]: ft2232_spi: Enhance csgpiol parameter for FT2232

[Samir Ibradžić (Code Review)]

Samir Ibradžić has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/38705 )

Change subject: ft2232_spi: Enhance csgpiol parameter for FT2232
......................................................................

ft2232_spi: Enhance csgpiol parameter for FT2232

This allows multiple 'csgpiol' bits to be set to active state at the
same time. Previously, only one GPIOL could be activated. I have an
use-case such that FT4232H is wired to two different SPI chips, and in
order to select one of them two GPIOLs have to be set.

Now, one can enable any particular GPIOL, for example:

csgpiol=01

would activate GPIOL0 and GPIOL1 at the same time.

The change is backward-compatible with previous csgpiol formatting.

Signed-off-by: Samir Ibradzic 
Change-Id: I645ddaa9852e9995bd2a6764862fda2b2ef0c26b
---
M ft2232_spi.c
1 file changed, 26 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/05/38705/1

diff --git a/ft2232_spi.c b/ft2232_spi.c
index 5bdc2a7..947050f 100644
--- a/ft2232_spi.c
+++ b/ft2232_spi.c
@@ -86,10 +86,17 @@

/* The variables cs_bits and pindir store the values for the "set data bits low byte" MPSSE command that
  * sets the initial state and the direction of the I/O pins. The pin offsets are as follows:
- * SCK is bit 0.
- * DO  is bit 1.
- * DI  is bit 2.
- * CS  is bit 3.
+ * TCK/SK is bit 0.
+ * TDI/DO is bit 1.
+ * TDO/DI is bit 2.
+ * TMS/CS is bit 3.
+ * GPIOL0 is bit 4.
+ * GPIOL1 is bit 5.
+ * GPIOL2 is bit 6.
+ * GPIOL3 is bit 7.
+ *
+ * The pin signal direction bit offsets follow the same order; 0 means that
+ * pin at the matching bit index is an input, 1 means pin is an output.
  *
  * The default values (set below) are used for most devices:
  *  value: 0x08  CS=high, DI=low, DO=low, SK=low
@@ -331,19 +338,24 @@
    }
    free(arg);

+	/* Allows setting multiple GPIOL states, for example: csgpiol=012 */
    arg = extract_programmer_param("csgpiol");
    if (arg) {
-		char *endptr;
-		unsigned int temp = strtoul(arg, &endptr, 10);
-		if (*endptr || endptr == arg || temp > 3) {
-			msg_perr("Error: Invalid GPIOL specified: "%s".\n"
-				 "Valid values are between 0 and 3.\n", arg);
-			free(arg);
-			return -2;
+		unsigned int ngpios = strlen(arg);
+		for (unsigned int i=0; i<=ngpios; i++) {
+			int temp = arg[i] - '0';
+			if (ngpios == 0 || (ngpios != i && (temp < 0 || temp > 3))) {
+				msg_perr("Error: Invalid GPIOLs specified: "%s".\n"
+					 "Valid values are numbers between 0 and 3. "
+					 "Multiple GPIOLs can be specified.\n", arg);
+				free(arg);
+				return -2;
+			} else {
+				unsigned int pin = temp + 4;
+				cs_bits |= 1 << pin;
+				pindir |= 1 << pin;
+			}
    	}
-		unsigned int pin = temp + 4;
-		cs_bits |= 1 << pin;
-		pindir |= 1 << pin;
    }
    free(arg);

[Samir Ibradžić (Code Review)]

Samir Ibradžić has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38705 )

Change subject: ft2232_spi: Enhance csgpiol parameter for FT2232
......................................................................

Patch Set 1:

Hi, can someone please review this?

[Edward O'Callaghan (Code Review)]

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

Change subject: ft2232_spi: Enhance csgpiol parameter for FT2232
......................................................................

Patch Set 1: Code-Review+2
> Patch Set 1:
>
> Hi, can someone please review this?

Sorry this took so much time for review. Thanks very much for the patch!

[Samir Ibradžić (Code Review)]

Samir Ibradžić has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38705 )

Change subject: ft2232_spi: Enhance csgpiol parameter for FT2232
......................................................................

Patch Set 1:

Who's attention do we need in order to get this merged?

[Angel Pons (Code Review)]

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

Change subject: ft2232_spi: Enhance csgpiol parameter for FT2232
......................................................................

Patch Set 1: Code-Review-1

(1 comment)

Only giving a -1 to draw attention. It's a minor thing, but should be fixed before submitting.

https://review.coreboot.org/c/flashrom/+/38705/1/ft2232_spi.c 
File ft2232_spi.c:

https://review.coreboot.org/c/flashrom/+/38705/1/ft2232_spi.c@345 
PS1, Line 345: i=0; i<=ngpios;
There should be spaces around these operators.

[Samir Ibradžić (Code Review)]

Hello Edward O'Callaghan, Angel Pons, David Hendricks, Stefan Reinauer, build bot (Jenkins), Nico Huber,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#2).

Change subject: ft2232_spi: Enhance csgpiol parameter for FT2232
......................................................................

ft2232_spi: Enhance csgpiol parameter for FT2232

This allows multiple 'csgpiol' bits to be set to active state at the
same time. Previously, only one GPIOL could be activated. I have an
use-case such that FT4232H is wired to two different SPI chips, and in
order to select one of them two GPIOLs have to be set.

Now, one can enable any particular GPIOL, for example:

csgpiol=01

would activate GPIOL0 and GPIOL1 at the same time.

The change is backward-compatible with previous csgpiol formatting.

Signed-off-by: Samir Ibradzic 
Change-Id: I645ddaa9852e9995bd2a6764862fda2b2ef0c26b
---
M ft2232_spi.c
1 file changed, 26 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/05/38705/2

[Angel Pons (Code Review)]

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

Change subject: ft2232_spi: Enhance csgpiol parameter for FT2232
......................................................................

Patch Set 2: Code-Review+2

(1 comment)

Thank you!

https://review.coreboot.org/c/flashrom/+/38705/1/ft2232_spi.c 
File ft2232_spi.c:

https://review.coreboot.org/c/flashrom/+/38705/1/ft2232_spi.c@345 
PS1, Line 345: i=0; i<=ngpios;
> There should be spaces around these operators.

Done

[Samir Ibradžić (Code Review)]

Samir Ibradžić has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38705 )

Change subject: ft2232_spi: Enhance csgpiol parameter for FT2232
......................................................................

Patch Set 2:

(1 comment)

Hi there. Can we have this merged please?

https://review.coreboot.org/c/flashrom/+/38705/1/ft2232_spi.c 
File ft2232_spi.c:

https://review.coreboot.org/c/flashrom/+/38705/1/ft2232_spi.c@345 
PS1, Line 345: i=0; i<=ngpios;
> There should be spaces around these operators.

Done

[Samir Ibradžić (Code Review)]

Samir Ibradžić has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38705 )

Change subject: ft2232_spi: Enhance csgpiol parameter for FT2232
......................................................................

Patch Set 2:

David, Edward, Stefan,

Gentlemen, can we have this merged please?

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/38705 )

Change subject: ft2232_spi: Enhance csgpiol parameter for FT2232
......................................................................

ft2232_spi: Enhance csgpiol parameter for FT2232

This allows multiple 'csgpiol' bits to be set to active state at the
same time. Previously, only one GPIOL could be activated. I have an
use-case such that FT4232H is wired to two different SPI chips, and in
order to select one of them two GPIOLs have to be set.

Now, one can enable any particular GPIOL, for example:

csgpiol=01

would activate GPIOL0 and GPIOL1 at the same time.

The change is backward-compatible with previous csgpiol formatting.

Signed-off-by: Samir Ibradzic 
Change-Id: I645ddaa9852e9995bd2a6764862fda2b2ef0c26b
Reviewed-on: https://review.coreboot.org/c/flashrom/+/38705
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M ft2232_spi.c
1 file changed, 26 insertions(+), 14 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/ft2232_spi.c b/ft2232_spi.c
index 5bdc2a7..3e4dc9e 100644
--- a/ft2232_spi.c
+++ b/ft2232_spi.c
@@ -86,10 +86,17 @@

/* The variables cs_bits and pindir store the values for the "set data bits low byte" MPSSE command that
  * sets the initial state and the direction of the I/O pins. The pin offsets are as follows:
- * SCK is bit 0.
- * DO  is bit 1.
- * DI  is bit 2.
- * CS  is bit 3.
+ * TCK/SK is bit 0.
+ * TDI/DO is bit 1.
+ * TDO/DI is bit 2.
+ * TMS/CS is bit 3.
+ * GPIOL0 is bit 4.
+ * GPIOL1 is bit 5.
+ * GPIOL2 is bit 6.
+ * GPIOL3 is bit 7.
+ *
+ * The pin signal direction bit offsets follow the same order; 0 means that
+ * pin at the matching bit index is an input, 1 means pin is an output.
  *
  * The default values (set below) are used for most devices:
  *  value: 0x08  CS=high, DI=low, DO=low, SK=low
@@ -331,19 +338,24 @@
    }
    free(arg);

+	/* Allows setting multiple GPIOL states, for example: csgpiol=012 */
    arg = extract_programmer_param("csgpiol");
    if (arg) {
-		char *endptr;
-		unsigned int temp = strtoul(arg, &endptr, 10);
-		if (*endptr || endptr == arg || temp > 3) {
-			msg_perr("Error: Invalid GPIOL specified: "%s".\n"
-				 "Valid values are between 0 and 3.\n", arg);
-			free(arg);
-			return -2;
+		unsigned int ngpios = strlen(arg);
+		for (unsigned int i = 0; i <= ngpios; i++) {
+			int temp = arg[i] - '0';
+			if (ngpios == 0 || (ngpios != i && (temp < 0 || temp > 3))) {
+				msg_perr("Error: Invalid GPIOLs specified: "%s".\n"
+					 "Valid values are numbers between 0 and 3. "
+					 "Multiple GPIOLs can be specified.\n", arg);
+				free(arg);
+				return -2;
+			} else {
+				unsigned int pin = temp + 4;
+				cs_bits |= 1 << pin;
+				pindir |= 1 << pin;
+			}
    	}
-		unsigned int pin = temp + 4;
-		cs_bits |= 1 << pin;
-		pindir |= 1 << pin;
    }
    free(arg);