Change in flashrom[master]: sb600spi.c: Remove 'amd_gen' out of global state.

[Edward O'Callaghan (Code Review)]

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


Change subject: sb600spi.c: Remove 'amd_gen' out of global state.
......................................................................

sb600spi.c: Remove 'amd_gen' out of global state.

Have 'determine_generation()' explicitly return 'amd_gen'
and then pass the state into what requires it. Thus making
the code more pure, easier to read and more unit-testable.

Change-Id: I99fbad9486123c6b921eab83756de54a53ddfa7a
Signed-off-by: Edward O'Callaghan <[e-mail]>
---
M sb600spi.c
1 file changed, 12 insertions(+), 13 deletions(-)



  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/36432/1

diff --git a/sb600spi.c b/sb600spi.c
index 8daaf03..23b36ee 100644
--- a/sb600spi.c
+++ b/sb600spi.c
@@ -51,7 +51,6 @@
 	CHIPSET_YANGTZE,
 	CHIPSET_PROMONTORY,
 };
-static enum amd_chipset amd_gen = CHIPSET_AMD_UNKNOWN;
 
 #define FIFO_SIZE_OLD		8
 #define FIFO_SIZE_YANGTZE	71
@@ -110,7 +109,7 @@
 /* Determine the chipset's version and identify the respective SMBUS device. */
 static int determine_generation(struct pci_dev *dev)
 {
-	amd_gen = CHIPSET_AMD_UNKNOWN;
+	enum amd_chipset amd_gen = CHIPSET_AMD_UNKNOWN;
 	msg_pdbg2("Trying to determine the generation of the SPI interface... ");
 	if (dev->device_id == 0x438d) {
 		amd_gen = CHIPSET_SB6XX;
@@ -173,11 +172,10 @@
 			  "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) {
+	if (amd_gen == CHIPSET_AMD_UNKNOWN)
 		msg_perr("Could not determine chipset generation.");
-		return -1;
-	}
-	return 0;
+
+	return amd_gen;
 }
 
 static void reset_internal_fifo_pointer(void)
@@ -373,7 +371,7 @@
 	{ "800 kHz",	0x07 },
 };
 
-static int set_speed(struct pci_dev *dev, const struct spispeed *spispeed)
+static int set_speed(struct pci_dev *dev, enum amd_chipset amd_gen, const struct spispeed *spispeed)
 {
 	bool success = false;
 	uint8_t speed = spispeed->speed;
@@ -407,7 +405,7 @@
 	return 0;
 }
 
-static int handle_speed(struct pci_dev *dev)
+static int handle_speed(struct pci_dev *dev, enum amd_chipset amd_gen)
 {
 	uint32_t tmp;
 	uint8_t spispeed_idx = 3; /* Default to 16.5 MHz */
@@ -496,10 +494,10 @@
 		tmp = (mmio_readb(sb600_spibar + 0xd) >> 4) & 0x3;
 		msg_pdbg("NormSpeed is %s\n", spispeeds[tmp].name);
 	}
-	return set_speed(dev, &spispeeds[spispeed_idx]);
+	return set_speed(dev, amd_gen, &spispeeds[spispeed_idx]);
 }
 
-static int handle_imc(struct pci_dev *dev)
+static int handle_imc(struct pci_dev *dev, enum amd_chipset amd_gen)
 {
 	/* Handle IMC everywhere but sb600 which does not have one. */
 	if (amd_gen == CHIPSET_SB6XX)
@@ -592,7 +590,8 @@
 	 */
 	sb600_spibar += tmp & 0xfff;
 
-	if (determine_generation(dev) < 0)
+	int amd_gen = determine_generation(dev);
+	if (amd_gen < 0)
 		return ERROR_NONFATAL;
 
 	/* How to read the following table and similar ones in this file:
@@ -725,10 +724,10 @@
 		return 0;
 	}
 
-	if (handle_speed(dev) != 0)
+	if (handle_speed(dev, amd_gen) != 0)
 		return ERROR_FATAL;
 
-	if (handle_imc(dev) != 0)
+	if (handle_imc(dev, amd_gen) != 0)
 		return ERROR_FATAL;
 
 	/* Starting with Yangtze the SPI controller got a different interface with a much bigger buffer. */

-- 
To view, visit https://review.coreboot.org/c/flashrom/+/36432
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I99fbad9486123c6b921eab83756de54a53ddfa7a
Gerrit-Change-Number: 36432
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-MessageType: newchange

[Nico Huber (Code Review)]

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

Change subject: sb600spi.c: Remove 'amd_gen' out of global state.
......................................................................


Patch Set 1: Code-Review+2


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/36432
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I99fbad9486123c6b921eab83756de54a53ddfa7a
Gerrit-Change-Number: 36432
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-Comment-Date: Thu, 14 Nov 2019 21:57:49 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Angel Pons (Code Review)]

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

Change subject: sb600spi.c: Remove 'amd_gen' out of global state.
......................................................................


Patch Set 1:

(4 comments)

https://review.coreboot.org/c/flashrom/+/36432/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/36432/1//COMMIT_MSG@7 
PS1, Line 7: .
no trailing period please


https://review.coreboot.org/c/flashrom/+/36432/1//COMMIT_MSG@10 
PS1, Line 10: . Thus
, thus


https://review.coreboot.org/c/flashrom/+/36432/1/sb600spi.c 
File sb600spi.c:

https://review.coreboot.org/c/flashrom/+/36432/1/sb600spi.c@110 
PS1, Line 110: int
update?

Also change the `return -1`


https://review.coreboot.org/c/flashrom/+/36432/1/sb600spi.c@594 
PS1, Line 594: amd_gen
Instead, check if it's not `_UNKNOWN`?



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/36432
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I99fbad9486123c6b921eab83756de54a53ddfa7a
Gerrit-Change-Number: 36432
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-Comment-Date: Wed, 24 Jun 2020 16:16:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in flashrom[master]: sb600spi.c: Remove 'amd_gen' out of global state

[Edward O'Callaghan (Code Review)]

Hello build bot (Jenkins), Nico Huber, 

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

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

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

Change subject: sb600spi.c: Remove 'amd_gen' out of global state
......................................................................

sb600spi.c: Remove 'amd_gen' out of global state

Have 'determine_generation()' explicitly return 'amd_gen'
and then pass the state into what requires it, thus making
the code more pure, easier to read and more unit-testable.

Change-Id: I99fbad9486123c6b921eab83756de54a53ddfa7a
Signed-off-by: Edward O'Callaghan <[e-mail]>
---
M sb600spi.c
1 file changed, 16 insertions(+), 17 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/36432/2
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/36432
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I99fbad9486123c6b921eab83756de54a53ddfa7a
Gerrit-Change-Number: 36432
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-MessageType: newpatchset

Change in flashrom[master]: sb600spi.c: Remove 'amd_gen' out of global state

[Edward O'Callaghan (Code Review)]

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

Change subject: sb600spi.c: Remove 'amd_gen' out of global state
......................................................................


Patch Set 2:

(4 comments)

https://review.coreboot.org/c/flashrom/+/36432/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/flashrom/+/36432/1//COMMIT_MSG@7 
PS1, Line 7: .
> no trailing period please
> Done

https://review.coreboot.org/c/flashrom/+/36432/1//COMMIT_MSG@10 
PS1, Line 10: . Thus
> , thus
> Done

https://review.coreboot.org/c/flashrom/+/36432/1/sb600spi.c 
File sb600spi.c:

https://review.coreboot.org/c/flashrom/+/36432/1/sb600spi.c@110 
PS1, Line 110: int
> update? […]
> Good idea, Done.

https://review.coreboot.org/c/flashrom/+/36432/1/sb600spi.c@594 
PS1, Line 594: amd_gen
> Instead, check if it's not `_UNKNOWN`?
> Done

Change in flashrom[master]: sb600spi.c: Remove 'amd_gen' out of global state

[Anastasia Klimchuk (Code Review)]

Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/36432 )

Change subject: sb600spi.c: Remove 'amd_gen' out of global state
......................................................................


Patch Set 2:

(1 comment)

https://review.coreboot.org/c/flashrom/+/36432/2/sb600spi.c 
File sb600spi.c:

https://review.coreboot.org/c/flashrom/+/36432/2/sb600spi.c@345 
PS2, Line 345:  enum amd_chipset amd_gen,
There are no other changes in set_speed, seems like it is not using amd_gen - does it need this as an argument?
Also handle_speed below?



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/36432
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I99fbad9486123c6b921eab83756de54a53ddfa7a
Gerrit-Change-Number: 36432
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Anastasia Klimchuk <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Sam McNally <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Tue, 24 Nov 2020 03:42:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in flashrom[master]: sb600spi.c: Remove 'amd_gen' out of global state

[Edward O'Callaghan (Code Review)]

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

Change subject: sb600spi.c: Remove 'amd_gen' out of global state
......................................................................


Patch Set 2:

(1 comment)

https://review.coreboot.org/c/flashrom/+/36432/2/sb600spi.c 
File sb600spi.c:

https://review.coreboot.org/c/flashrom/+/36432/2/sb600spi.c@345 
PS2, Line 345:  enum amd_chipset amd_gen,
> There are no other changes in set_speed, seems like it is not using amd_gen - does it need this as a […]
> That isn't true, look again. `amd_gen` was previously in the global scope and now it is locally scoped to the function. The function was a closure on the global state before and so no other changes are needed.

Change in flashrom[master]: sb600spi.c: Remove 'amd_gen' out of global state

[Anastasia Klimchuk (Code Review)]

Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/36432 )

Change subject: sb600spi.c: Remove 'amd_gen' out of global state
......................................................................


Patch Set 2: Code-Review+1


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/36432
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I99fbad9486123c6b921eab83756de54a53ddfa7a
Gerrit-Change-Number: 36432
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Anastasia Klimchuk <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Sam McNally <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Tue, 24 Nov 2020 04:21:33 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Change in flashrom[master]: sb600spi.c: Remove 'amd_gen' out of global state

[Sam McNally (Code Review)]

Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/36432 )

Change subject: sb600spi.c: Remove 'amd_gen' out of global state
......................................................................


Patch Set 2:

(1 comment)

https://review.coreboot.org/c/flashrom/+/36432/2/sb600spi.c 
File sb600spi.c:

https://review.coreboot.org/c/flashrom/+/36432/2/sb600spi.c@75 
PS2, Line 75: amd_gen = CHIPSET_SB6XX;
Could I convince you to make all of these early returns?



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/36432
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I99fbad9486123c6b921eab83756de54a53ddfa7a
Gerrit-Change-Number: 36432
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Anastasia Klimchuk <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Sam McNally <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Tue, 24 Nov 2020 06:42:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in flashrom[master]: sb600spi.c: Remove 'amd_gen' out of global state

[Edward O'Callaghan (Code Review)]

Hello Sam McNally, build bot (Jenkins), Nico Huber, Anastasia Klimchuk, 

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

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

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

Change subject: sb600spi.c: Remove 'amd_gen' out of global state
......................................................................

sb600spi.c: Remove 'amd_gen' out of global state

Have 'determine_generation()' explicitly return 'amd_gen'
and then pass the state into what requires it, thus making
the code more pure, easier to read and more unit-testable.

Change-Id: I99fbad9486123c6b921eab83756de54a53ddfa7a
Signed-off-by: Edward O'Callaghan <[e-mail]>
---
M sb600spi.c
1 file changed, 24 insertions(+), 27 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/36432/3
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/36432
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I99fbad9486123c6b921eab83756de54a53ddfa7a
Gerrit-Change-Number: 36432
Gerrit-PatchSet: 3
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Anastasia Klimchuk <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Sam McNally <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-MessageType: newpatchset

Change in flashrom[master]: sb600spi.c: Remove 'amd_gen' out of global state

[Edward O'Callaghan (Code Review)]

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

Change subject: sb600spi.c: Remove 'amd_gen' out of global state
......................................................................


Patch Set 2:

(1 comment)

https://review.coreboot.org/c/flashrom/+/36432/2/sb600spi.c 
File sb600spi.c:

https://review.coreboot.org/c/flashrom/+/36432/2/sb600spi.c@75 
PS2, Line 75: amd_gen = CHIPSET_SB6XX;
> Could I convince you to make all of these early returns?
> Done

Change in flashrom[master]: sb600spi.c: Remove 'amd_gen' out of global state

[Sam McNally (Code Review)]

Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/36432 )

Change subject: sb600spi.c: Remove 'amd_gen' out of global state
......................................................................


Patch Set 3:

(1 comment)

https://review.coreboot.org/c/flashrom/+/36432/3/sb600spi.c 
File sb600spi.c:

https://review.coreboot.org/c/flashrom/+/36432/3/sb600spi.c@589 
PS3, Line 589: int
enum amd_chipset?



-- 
To view, visit https://review.coreboot.org/c/flashrom/+/36432
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I99fbad9486123c6b921eab83756de54a53ddfa7a
Gerrit-Change-Number: 36432
Gerrit-PatchSet: 3
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Anastasia Klimchuk <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Sam McNally <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Tue, 24 Nov 2020 10:07:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in flashrom[master]: sb600spi.c: Remove 'amd_gen' out of global state

[Edward O'Callaghan (Code Review)]

Hello Sam McNally, build bot (Jenkins), Nico Huber, Anastasia Klimchuk, 

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

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

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

Change subject: sb600spi.c: Remove 'amd_gen' out of global state
......................................................................

sb600spi.c: Remove 'amd_gen' out of global state

Have 'determine_generation()' explicitly return 'amd_gen'
and then pass the state into what requires it, thus making
the code more pure, easier to read and more unit-testable.

Change-Id: I99fbad9486123c6b921eab83756de54a53ddfa7a
Signed-off-by: Edward O'Callaghan <[e-mail]>
---
M sb600spi.c
1 file changed, 24 insertions(+), 27 deletions(-)


  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/36432/4
-- 
To view, visit https://review.coreboot.org/c/flashrom/+/36432
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I99fbad9486123c6b921eab83756de54a53ddfa7a
Gerrit-Change-Number: 36432
Gerrit-PatchSet: 4
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Anastasia Klimchuk <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Sam McNally <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-MessageType: newpatchset

Change in flashrom[master]: sb600spi.c: Remove 'amd_gen' out of global state

[Edward O'Callaghan (Code Review)]

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

Change subject: sb600spi.c: Remove 'amd_gen' out of global state
......................................................................


Patch Set 4:

(1 comment)

https://review.coreboot.org/c/flashrom/+/36432/3/sb600spi.c 
File sb600spi.c:

https://review.coreboot.org/c/flashrom/+/36432/3/sb600spi.c@589 
PS3, Line 589: int
> enum amd_chipset?
> Done

Change in flashrom[master]: sb600spi.c: Remove 'amd_gen' out of global state

[Sam McNally (Code Review)]

Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/36432 )

Change subject: sb600spi.c: Remove 'amd_gen' out of global state
......................................................................


Patch Set 4: Code-Review+2


-- 
To view, visit https://review.coreboot.org/c/flashrom/+/36432
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I99fbad9486123c6b921eab83756de54a53ddfa7a
Gerrit-Change-Number: 36432
Gerrit-PatchSet: 4
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Anastasia Klimchuk <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Sam McNally <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Tue, 24 Nov 2020 11:24:36 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Change in flashrom[master]: sb600spi.c: Remove 'amd_gen' out of global state

[Edward O'Callaghan (Code Review)]

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

Change subject: sb600spi.c: Remove 'amd_gen' out of global state
......................................................................

sb600spi.c: Remove 'amd_gen' out of global state

Have 'determine_generation()' explicitly return 'amd_gen'
and then pass the state into what requires it, thus making
the code more pure, easier to read and more unit-testable.

Change-Id: I99fbad9486123c6b921eab83756de54a53ddfa7a
Signed-off-by: Edward O'Callaghan <[e-mail]>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/36432
Tested-by: build bot (Jenkins) <[e-mail]>
Reviewed-by: Sam McNally <[e-mail]>
---
M sb600spi.c
1 file changed, 24 insertions(+), 27 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Sam McNally: Looks good to me, approved



diff --git a/sb600spi.c b/sb600spi.c
index 8eaf41f..40434f0 100644
--- a/sb600spi.c
+++ b/sb600spi.c
@@ -51,7 +51,6 @@
 	CHIPSET_YANGTZE,
 	CHIPSET_PROMONTORY,
 };
-static enum amd_chipset amd_gen = CHIPSET_AMD_UNKNOWN;
 
 #define FIFO_SIZE_OLD		8
 #define FIFO_SIZE_YANGTZE	71
@@ -68,44 +67,43 @@
 }
 
 /* Determine the chipset's version and identify the respective SMBUS device. */
-static int determine_generation(struct pci_dev *dev)
+static enum amd_chipset determine_generation(struct pci_dev *dev)
 {
-	amd_gen = CHIPSET_AMD_UNKNOWN;
 	msg_pdbg2("Trying to determine the generation of the SPI interface... ");
 	if (dev->device_id == 0x438d) {
-		amd_gen = CHIPSET_SB6XX;
 		msg_pdbg("SB6xx detected.\n");
+		return CHIPSET_SB6XX;
 	} else if (dev->device_id == 0x439d) {
 		int rev = find_smbus_dev_rev(0x1002, 0x4385);
 		if (rev < 0)
-			return -1;
+			return CHIPSET_AMD_UNKNOWN;
 		if (rev >= 0x39 && rev <= 0x3D) {
-			amd_gen = CHIPSET_SB7XX;
 			msg_pdbg("SB7xx/SP5100 detected.\n");
+			return CHIPSET_SB7XX;
 		} else if (rev >= 0x40 && rev <= 0x42) {
-			amd_gen = CHIPSET_SB89XX;
 			msg_pdbg("SB8xx/SB9xx/Hudson-1 detected.\n");
+			return CHIPSET_SB89XX;
 		} else {
 			msg_pwarn("SB device found but SMBus revision 0x%02x does not match known values.\n"
 				  "Assuming SB8xx/SB9xx/Hudson-1. Please send a log to [e-mail]\n",
 				   rev);
-			amd_gen = CHIPSET_SB89XX;
+			return CHIPSET_SB89XX;
 		}
 	} else if (dev->device_id == 0x780e) {
 		/* The PCI ID of the LPC bridge doesn't change between Hudson-2/3/4 and Yangtze (Kabini/Temash)
 		 * although they use different SPI interfaces. */
 		int rev = find_smbus_dev_rev(0x1022, 0x780B);
 		if (rev < 0)
-			return -1;
+			return CHIPSET_AMD_UNKNOWN;
 		if (rev >= 0x11 && rev <= 0x15) {
-			amd_gen = CHIPSET_HUDSON234;
 			msg_pdbg("Hudson-2/3/4 detected.\n");
+			return CHIPSET_HUDSON234;
 		} else if (rev == 0x16) {
-			amd_gen = CHIPSET_BOLTON;
 			msg_pdbg("Bolton detected.\n");
+			return CHIPSET_BOLTON;
 		} else if ((rev >= 0x39 && rev <= 0x3A) || rev == 0x42) {
-			amd_gen = CHIPSET_YANGTZE;
 			msg_pdbg("Yangtze detected.\n");
+			return CHIPSET_YANGTZE;
 		} else {
 			msg_pwarn("FCH device found but SMBus revision 0x%02x does not match known values.\n"
 				  "Please report this to [e-mail] and include this log and\n"
@@ -114,13 +112,13 @@
 	} else if (dev->device_id == 0x790e) {
 		int rev = find_smbus_dev_rev(0x1022, 0x790B);
 		if (rev < 0)
-			return -1;
+			return CHIPSET_AMD_UNKNOWN;
 		if (rev == 0x4a) {
-			amd_gen = CHIPSET_YANGTZE;
 			msg_pdbg("Yangtze detected.\n");
+			return CHIPSET_YANGTZE;
 		} else if (rev == 0x4b) {
-			amd_gen = CHIPSET_PROMONTORY;
 			msg_pdbg("Promontory detected.\n");
+			return CHIPSET_PROMONTORY;
 		} else {
 			msg_pwarn("FCH device found but SMBus revision 0x%02x does not match known values.\n"
 				  "Please report this to [e-mail] and include this log and\n"
@@ -133,11 +131,9 @@
 			  "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;
+
+	msg_perr("Could not determine chipset generation.");
+	return CHIPSET_AMD_UNKNOWN;
 }
 
 static void reset_internal_fifo_pointer(void)
@@ -344,7 +340,7 @@
 	"Fast Read",
 };
 
-static int set_speed(struct pci_dev *dev, uint8_t speed)
+static int set_speed(struct pci_dev *dev, enum amd_chipset amd_gen, uint8_t speed)
 {
 	bool success = false;
 
@@ -382,7 +378,7 @@
 	return 0;
 }
 
-static int handle_speed(struct pci_dev *dev)
+static int handle_speed(struct pci_dev *dev, enum amd_chipset amd_gen)
 {
 	uint32_t tmp;
 	int16_t spispeed_idx = -1;
@@ -493,10 +489,10 @@
 		msg_perr("Warning: spispeed not set, leaving spispeed unchanged.");
 		return 0;
 	}
-	return set_speed(dev, spispeed_idx);
+	return set_speed(dev, amd_gen, spispeed_idx);
 }
 
-static int handle_imc(struct pci_dev *dev)
+static int handle_imc(struct pci_dev *dev, enum amd_chipset amd_gen)
 {
 	/* Handle IMC everywhere but sb600 which does not have one. */
 	if (amd_gen == CHIPSET_SB6XX)
@@ -590,7 +586,8 @@
 	 */
 	sb600_spibar += tmp & 0xfff;
 
-	if (determine_generation(dev) < 0)
+	enum amd_chipset amd_gen = determine_generation(dev);
+	if (amd_gen == CHIPSET_AMD_UNKNOWN)
 		return ERROR_NONFATAL;
 
 	/* How to read the following table and similar ones in this file:
@@ -723,10 +720,10 @@
 		return 0;
 	}
 
-	if (handle_speed(dev) != 0)
+	if (handle_speed(dev, amd_gen) != 0)
 		return ERROR_FATAL;
 
-	if (handle_imc(dev) != 0)
+	if (handle_imc(dev, amd_gen) != 0)
 		return ERROR_FATAL;
 
 	/* Starting with Yangtze the SPI controller got a different interface with a much bigger buffer. */

-- 
To view, visit https://review.coreboot.org/c/flashrom/+/36432
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I99fbad9486123c6b921eab83756de54a53ddfa7a
Gerrit-Change-Number: 36432
Gerrit-PatchSet: 5
Gerrit-Owner: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Anastasia Klimchuk <[e-mail]>
Gerrit-Reviewer: Edward O'Callaghan <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Sam McNally <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-MessageType: merged