Change in flashrom[master]: sb600spi.c: Fold up debug logic into determine_generation()

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/36425 )

Change subject: sb600spi.c: Fold up debug logic into determine_generation()
......................................................................

sb600spi.c: Fold up debug logic into determine_generation()

Change-Id: I6c722e29b321285bf20fb5ee30c912dcdd83411b
Signed-off-by: Edward O'Callaghan 
---
M sb600spi.c
1 file changed, 12 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/25/36425/1

diff --git a/sb600spi.c b/sb600spi.c
index c89d5a9..48baf15 100644
--- a/sb600spi.c
+++ b/sb600spi.c
@@ -91,7 +91,8 @@
    return pci_read_byte(smbus_dev, PCI_REVISION_ID);
 }

-static void determine_generation(struct pci_dev *dev)
+/* Determine the chipset's version and identify the respective SMBUS device. */
+static int determine_generation(struct pci_dev *dev)
 {
    amd_gen = CHIPSET_AMD_UNKNOWN;
    msg_pdbg2("Trying to determine the generation of the SPI interface... ");
@@ -101,7 +102,7 @@
    } else if (dev->device_id == 0x439d) {
    	int rev = find_smbus_dev(0x1002, 0x4385);
    	if (rev < 0)
-			return;
+			return -1;
    	if (rev >= 0x39 && rev <= 0x3D) {
    		amd_gen = CHIPSET_SB7XX;
    		msg_pdbg("SB7xx/SP5100 detected.\n");
@@ -127,7 +128,7 @@
    		if (mmio_readb(sb600_spibar + i) != 0xff) {
    			amd_gen = CHIPSET_YANGTZE;
    			msg_pdbg("found.\n");
-				return;
+				return 0;
    		}
    	}
    	msg_pdbg("not found. Assuming Hudson.\n");
@@ -135,7 +136,7 @@
 #else
    	int rev = find_smbus_dev(0x1022, 0x780B);
    	if (rev < 0)
-			return;
+			return -1;
    	if (rev >= 0x11 && rev <= 0x15) {
    		amd_gen = CHIPSET_HUDSON234;
    		msg_pdbg("Hudson-2/3/4 detected.\n");
@@ -153,7 +154,7 @@
    } else if (dev->device_id == 0x790e) {
    	int rev = find_smbus_dev(0x1022, 0x790B);
    	if (rev < 0)
-			return;
+			return -1;
    	if (rev == 0x4a) {
    		amd_gen = CHIPSET_YANGTZE;
    		msg_pdbg("Yangtze detected.\n");
@@ -170,6 +171,11 @@
    		  "Please report this to [e-mail] and include this log and\n"
    		  "the output of lspci -nnvx, thanks!\n",
    		  __func__, dev->vendor_id, dev->device_id);
+	if (amd_gen == CHIPSET_AMD_UNKNOWN) {
+		msg_perr("Could not determine chipset generation.");
+		return -1;
+	}
+	return 0;
 }

static void reset_internal_fifo_pointer(void)
@@ -565,11 +571,8 @@
     */
    sb600_spibar += tmp & 0xfff;

-	determine_generation(dev);
-	if (amd_gen == CHIPSET_AMD_UNKNOWN) {
-		msg_perr("Could not determine chipset generation.");
+	if (determine_generation(dev) < 0)
    	return ERROR_NONFATAL;
-	}

/* How to read the following table and similar ones in this file:
     * "?" means we have no datasheet for this chipset generation or it doesn't have any relevant info.

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/36425 )

Change subject: sb600spi.c: Fold up debug logic into determine_generation()
......................................................................

Patch Set 1: Code-Review+2

[Edward O'Callaghan (Code Review)]

Hello Arthur Heymans, build bot (Jenkins),

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

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

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

Change subject: sb600spi.c: Fold up debug logic into determine_generation()
......................................................................

sb600spi.c: Fold up debug logic into determine_generation()

Change-Id: I6c722e29b321285bf20fb5ee30c912dcdd83411b
Signed-off-by: Edward O'Callaghan 
---
M sb600spi.c
1 file changed, 21 insertions(+), 14 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/25/36425/2

[Edward O'Callaghan (Code Review)]

Hello Arthur Heymans, build bot (Jenkins),

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

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

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

Change subject: sb600spi.c: Fold up debug logic into determine_generation()
......................................................................

sb600spi.c: Fold up debug logic into determine_generation()

Change-Id: I6c722e29b321285bf20fb5ee30c912dcdd83411b
Signed-off-by: Edward O'Callaghan 
---
M sb600spi.c
1 file changed, 12 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/25/36425/3

[Nico Huber (Code Review)]

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

Change subject: sb600spi.c: Fold up debug logic into determine_generation()
......................................................................

Patch Set 3: Code-Review+1

[Nico Huber (Code Review)]

Nico Huber has submitted this change. ( https://review.coreboot.org/c/flashrom/+/36425 )

Change subject: sb600spi.c: Fold up debug logic into determine_generation()
......................................................................

sb600spi.c: Fold up debug logic into determine_generation()

Change-Id: I6c722e29b321285bf20fb5ee30c912dcdd83411b
Signed-off-by: Edward O'Callaghan 
Reviewed-on: https://review.coreboot.org/c/flashrom/+/36425
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
Reviewed-by: Arthur Heymans 
---
M sb600spi.c
1 file changed, 12 insertions(+), 9 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, but someone else must approve
  Arthur Heymans: Looks good to me, approved

diff --git a/sb600spi.c b/sb600spi.c
index 6466795..050d210 100644
--- a/sb600spi.c
+++ b/sb600spi.c
@@ -91,7 +91,8 @@
    return pci_read_byte(smbus_dev, PCI_REVISION_ID);
 }

-static void determine_generation(struct pci_dev *dev)
+/* Determine the chipset's version and identify the respective SMBUS device. */
+static int determine_generation(struct pci_dev *dev)
 {
    amd_gen = CHIPSET_AMD_UNKNOWN;
    msg_pdbg2("Trying to determine the generation of the SPI interface... ");
@@ -101,7 +102,7 @@
    } else if (dev->device_id == 0x439d) {
    	int rev = find_smbus_dev_rev(0x1002, 0x4385);
    	if (rev < 0)
-			return;
+			return -1;
    	if (rev >= 0x39 && rev <= 0x3D) {
    		amd_gen = CHIPSET_SB7XX;
    		msg_pdbg("SB7xx/SP5100 detected.\n");
@@ -127,7 +128,7 @@
    		if (mmio_readb(sb600_spibar + i) != 0xff) {
    			amd_gen = CHIPSET_YANGTZE;
    			msg_pdbg("found.\n");
-				return;
+				return 0;
    		}
    	}
    	msg_pdbg("not found. Assuming Hudson.\n");
@@ -135,7 +136,7 @@
 #else
    	int rev = find_smbus_dev_rev(0x1022, 0x780B);
    	if (rev < 0)
-			return;
+			return -1;
    	if (rev >= 0x11 && rev <= 0x15) {
    		amd_gen = CHIPSET_HUDSON234;
    		msg_pdbg("Hudson-2/3/4 detected.\n");
@@ -153,7 +154,7 @@
    } else if (dev->device_id == 0x790e) {
    	int rev = find_smbus_dev_rev(0x1022, 0x790B);
    	if (rev < 0)
-			return;
+			return -1;
    	if (rev == 0x4a) {
    		amd_gen = CHIPSET_YANGTZE;
    		msg_pdbg("Yangtze detected.\n");
@@ -170,6 +171,11 @@
    		  "Please report this to [e-mail] and include this log and\n"
    		  "the output of lspci -nnvx, thanks!\n",
    		  __func__, dev->vendor_id, dev->device_id);
+	if (amd_gen == CHIPSET_AMD_UNKNOWN) {
+		msg_perr("Could not determine chipset generation.");
+		return -1;
+	}
+	return 0;
 }

static void reset_internal_fifo_pointer(void)
@@ -565,11 +571,8 @@
     */
    sb600_spibar += tmp & 0xfff;

-	determine_generation(dev);
-	if (amd_gen == CHIPSET_AMD_UNKNOWN) {
-		msg_perr("Could not determine chipset generation.");
+	if (determine_generation(dev) < 0)
    	return ERROR_NONFATAL;
-	}

/* How to read the following table and similar ones in this file:
     * "?" means we have no datasheet for this chipset generation or it doesn't have any relevant info.