Change in ...flashrom[master]: spi25: Fix layering violation in probe_spi_rdid4()

[Nico Huber (Code Review)]

Hello HAOUAS Elyes, David Hendricks, Stefan Reinauer,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: spi25: Fix layering violation in probe_spi_rdid4()
......................................................................

spi25: Fix layering violation in probe_spi_rdid4()

Move the message to a lower level where we can do a more generic check
and don't need internal knowledge of the SPI-master driver.

Change-Id: Idd21d20465cb214f3ff5bf3267b9014f8beee3f3
Signed-off-by: Nico Huber 
---
M spi25.c
1 file changed, 5 insertions(+), 20 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/50/33650/1

diff --git a/spi25.c b/spi25.c
index 06e451f..006b81f 100644
--- a/spi25.c
+++ b/spi25.c
@@ -100,9 +100,11 @@
    uint32_t id1;
    uint32_t id2;

-	if (spi_rdid(flash, readarr, bytes)) {
+	const int ret = spi_rdid(flash, readarr, bytes);
+	if (ret == SPI_INVALID_LENGTH)
+		msg_cinfo("%d byte RDID not supported on this SPI controller\n", bytes);
+	if (ret)
    	return 0;
-	}

if (!oddparity(readarr[0]))
    	msg_cdbg("RDID byte 0 parity violation. ");
@@ -147,24 +149,7 @@

int probe_spi_rdid4(struct flashctx *flash)
 {
-	/* Some SPI controllers do not support commands with writecnt=1 and
-	 * readcnt=4.
-	 */
-	switch (flash->mst->spi.type) {
-#if CONFIG_INTERNAL == 1
-#if defined(__i386__) || defined(__x86_64__)
-	case SPI_CONTROLLER_IT87XX:
-	case SPI_CONTROLLER_WBSIO:
-		msg_cinfo("4 byte RDID not supported on this SPI controller\n");
-		return 0;
-		break;
-#endif
-#endif
-	default:
-		return probe_spi_rdid_generic(flash, 4);
-	}
-
-	return 0;
+	return probe_spi_rdid_generic(flash, 4);
 }

int probe_spi_rems(struct flashctx *flash)

[Nico Huber (Code Review)]

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

Change subject: spi25: Fix layering violation in probe_spi_rdid4()
......................................................................

Patch Set 1:

Cherry picked from
https://review.coreboot.org/c/flashrom/+/19420

Rewritten more elegantly, reusing the master driver's
own length checks.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/33650 )

Change subject: spi25: Fix layering violation in probe_spi_rdid4()
......................................................................

Patch Set 1: Code-Review+1

[Angel Pons (Code Review)]

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

Change subject: spi25: Fix layering violation in probe_spi_rdid4()
......................................................................

Patch Set 2: Code-Review+1

[Angel Pons (Code Review)]

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

Change subject: spi25: Fix layering violation in probe_spi_rdid4()
......................................................................

Patch Set 2: Code-Review+2

[Nico Huber (Code Review)]

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/flashrom/+/33650 )

Change subject: spi25: Fix layering violation in probe_spi_rdid4()
......................................................................

spi25: Fix layering violation in probe_spi_rdid4()

Move the message to a lower level where we can do a more generic check
and don't need internal knowledge of the SPI-master driver.

Change-Id: Idd21d20465cb214f3ff5bf3267b9014f8beee3f3
Signed-off-by: Nico Huber 
Reviewed-on: https://review.coreboot.org/c/flashrom/+/33650
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
Reviewed-by: Paul Menzel 
---
M spi25.c
1 file changed, 5 insertions(+), 20 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Angel Pons: Looks good to me, approved

diff --git a/spi25.c b/spi25.c
index ecb1893..36f265a 100644
--- a/spi25.c
+++ b/spi25.c
@@ -100,9 +100,11 @@
    uint32_t id1;
    uint32_t id2;

-	if (spi_rdid(flash, readarr, bytes)) {
+	const int ret = spi_rdid(flash, readarr, bytes);
+	if (ret == SPI_INVALID_LENGTH)
+		msg_cinfo("%d byte RDID not supported on this SPI controller\n", bytes);
+	if (ret)
    	return 0;
-	}

if (!oddparity(readarr[0]))
    	msg_cdbg("RDID byte 0 parity violation. ");
@@ -147,24 +149,7 @@

int probe_spi_rdid4(struct flashctx *flash)
 {
-	/* Some SPI controllers do not support commands with writecnt=1 and
-	 * readcnt=4.
-	 */
-	switch (flash->mst->spi.type) {
-#if CONFIG_INTERNAL == 1
-#if defined(__i386__) || defined(__x86_64__)
-	case SPI_CONTROLLER_IT87XX:
-	case SPI_CONTROLLER_WBSIO:
-		msg_cinfo("4 byte RDID not supported on this SPI controller\n");
-		return 0;
-		break;
-#endif
-#endif
-	default:
-		return probe_spi_rdid_generic(flash, 4);
-	}
-
-	return 0;
+	return probe_spi_rdid_generic(flash, 4);
 }

int probe_spi_rems(struct flashctx *flash)