Paul Menzel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45970 )
Change subject: superio/nuvoton/common: Collapse two if statements ......................................................................
superio/nuvoton/common: Collapse two if statements
There are more devices requiring this code, so avoid duplicating the if block over and over.
Change-Id: Ib4f787e3c883b1fec941de77bc8e19ccf0d5224c Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de --- M src/superio/nuvoton/common/early_serial.c 1 file changed, 2 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/45970/1
diff --git a/src/superio/nuvoton/common/early_serial.c b/src/superio/nuvoton/common/early_serial.c index 22d882e..cc21f40 100644 --- a/src/superio/nuvoton/common/early_serial.c +++ b/src/superio/nuvoton/common/early_serial.c @@ -52,11 +52,8 @@
nuvoton_pnp_enter_conf_state(dev);
- if (CONFIG(SUPERIO_NUVOTON_NCT5539D_COM_A)) - /* Route COM A to GPIO8 pin group */ - pnp_write_config(dev, 0x2a, 0x40); - - if (CONFIG(SUPERIO_NUVOTON_NCT6776_COM_A)) + if (CONFIG(SUPERIO_NUVOTON_NCT5539D_COM_A) || + CONFIG(SUPERIO_NUVOTON_NCT6776_COM_A)) /* Route COM A to GPIO8 pin group */ pnp_write_config(dev, 0x2a, 0x40);
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45970 )
Change subject: superio/nuvoton/common: Collapse two if statements ......................................................................
Patch Set 1: Code-Review+1
i think in the original patch this was intentional, but i do prefer the version with less code duplication. one thing that would be great to be fixed is that this functionality clubbers the other bits of the register; https://review.coreboot.org/c/coreboot/+/42134 provides functionality to avoid that issue; iirc this issue was one of the motivations for said patch
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45970 )
Change subject: superio/nuvoton/common: Collapse two if statements ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/45970/2/src/superio/nuvoton/common/... File src/superio/nuvoton/common/early_serial.c:
https://review.coreboot.org/c/coreboot/+/45970/2/src/superio/nuvoton/common/... PS2, Line 55: if (CONFIG(SUPERIO_NUVOTON_NCT5539D_COM_A) || Actually, this should be fixed to not overwrite the entire register contents.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45970 )
Change subject: superio/nuvoton/common: Collapse two if statements ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/45970/2/src/superio/nuvoton/common/... File src/superio/nuvoton/common/early_serial.c:
https://review.coreboot.org/c/coreboot/+/45970/2/src/superio/nuvoton/common/... PS2, Line 55: if (CONFIG(SUPERIO_NUVOTON_NCT5539D_COM_A) ||
Actually, this should be fixed to not overwrite the entire register contents.
Done in a follow up.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45970 )
Change subject: superio/nuvoton/common: Collapse two if statements ......................................................................
Patch Set 4: Code-Review+2
Actually, all three SIOs use the same bit to control this: bit 7 to zero.
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45970 )
Change subject: superio/nuvoton/common: Collapse two if statements ......................................................................
superio/nuvoton/common: Collapse two if statements
There are more devices requiring this code, so avoid duplicating the if block over and over.
Change-Id: Ib4f787e3c883b1fec941de77bc8e19ccf0d5224c Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/45970 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Felix Held felix-coreboot@felixheld.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/superio/nuvoton/common/early_serial.c 1 file changed, 2 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Felix Held: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved
diff --git a/src/superio/nuvoton/common/early_serial.c b/src/superio/nuvoton/common/early_serial.c index 22d882e..cc21f40 100644 --- a/src/superio/nuvoton/common/early_serial.c +++ b/src/superio/nuvoton/common/early_serial.c @@ -52,11 +52,8 @@
nuvoton_pnp_enter_conf_state(dev);
- if (CONFIG(SUPERIO_NUVOTON_NCT5539D_COM_A)) - /* Route COM A to GPIO8 pin group */ - pnp_write_config(dev, 0x2a, 0x40); - - if (CONFIG(SUPERIO_NUVOTON_NCT6776_COM_A)) + if (CONFIG(SUPERIO_NUVOTON_NCT5539D_COM_A) || + CONFIG(SUPERIO_NUVOTON_NCT6776_COM_A)) /* Route COM A to GPIO8 pin group */ pnp_write_config(dev, 0x2a, 0x40);
-
Angel Pons (Code Review) -
Felix Held (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)