Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37638 )
Change subject: superio/*: Don't use conf_mode directly ......................................................................
superio/*: Don't use conf_mode directly
Use the functions defined in device/pnp.h instead of using the conf_mode directly. This will make future refactoring easier.
Change-Id: Ibb94d86b3ee861f44cded469ff58b545dd7311fd Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/superio/nuvoton/nct5572d/superio.c M src/superio/smsc/lpc47n227/superio.c M src/superio/winbond/w83667hg-a/superio.c 3 files changed, 18 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/37638/1
diff --git a/src/superio/nuvoton/nct5572d/superio.c b/src/superio/nuvoton/nct5572d/superio.c index 3084687..fa00f94 100644 --- a/src/superio/nuvoton/nct5572d/superio.c +++ b/src/superio/nuvoton/nct5572d/superio.c @@ -22,7 +22,6 @@ #include <pc80/keyboard.h> #include <pc80/mc146818rtc.h> #include <arch/acpi.h> -#include <superio/conf_mode.h>
#include "nct5572d.h"
@@ -43,29 +42,29 @@ /* TODO: Might potentially need code for HWM or FDC etc. */ case NCT5572D_KBC: /* Enable mouse controller */ - pnp_enter_conf_mode_8787(dev); + pnp_enter_conf_mode(dev); byte = pnp_read_config(dev, 0x2a); byte &= ~(0x1 << 1); pnp_write_config(dev, 0x2a, byte); - pnp_exit_conf_mode_aa(dev); + pnp_exit_conf_mode(dev);
mouse_detected = pc_keyboard_init(PROBE_AUX_DEVICE);
if (!mouse_detected) { printk(BIOS_INFO, "%s: Disable mouse controller.", __func__); - pnp_enter_conf_mode_8787(dev); + pnp_enter_conf_mode(dev); byte = pnp_read_config(dev, 0x2a); byte |= 0x1 << 1; pnp_write_config(dev, 0x2a, byte); - pnp_exit_conf_mode_aa(dev); + pnp_exit_conf_mode(dev); } break; case NCT5572D_ACPI: /* Set power state after power fail */ power_status = CONFIG_MAINBOARD_POWER_FAILURE_STATE; get_option(&power_status, "power_on_after_fail"); - pnp_enter_conf_mode_8787(dev); + pnp_enter_conf_mode(dev); pnp_set_logical_device(dev); byte = pnp_read_config(dev, 0xe4); byte &= ~0x60; @@ -74,7 +73,7 @@ else if (power_status == MAINBOARD_POWER_KEEP) byte |= (0x2 << 5); pnp_write_config(dev, 0xe4, byte); - pnp_exit_conf_mode_aa(dev); + pnp_exit_conf_mode(dev); printk(BIOS_INFO, "set power %s after power fail\n", power_status ? "on" : "off"); break; } diff --git a/src/superio/smsc/lpc47n227/superio.c b/src/superio/smsc/lpc47n227/superio.c index 57297ab..845ed17 100644 --- a/src/superio/smsc/lpc47n227/superio.c +++ b/src/superio/smsc/lpc47n227/superio.c @@ -81,10 +81,10 @@ { struct resource *res;
- pnp_enter_conf_mode_55(dev); + pnp_enter_conf_mode(dev); for (res = dev->resource_list; res; res = res->next) lpc47n227_pnp_set_resource(dev, res); - pnp_exit_conf_mode_aa(dev); + pnp_exit_conf_mode(dev); }
/* @@ -93,9 +93,9 @@ */ void lpc47n227_pnp_enable_resources(struct device *dev) { - pnp_enter_conf_mode_55(dev); + pnp_enter_conf_mode(dev); lpc47n227_pnp_set_enable(dev, 1); - pnp_exit_conf_mode_aa(dev); + pnp_exit_conf_mode(dev); }
/* @@ -104,9 +104,9 @@ */ void lpc47n227_pnp_enable(struct device *dev) { - pnp_enter_conf_mode_55(dev); + pnp_enter_conf_mode(dev); lpc47n227_pnp_set_enable(dev, !!dev->enabled); - pnp_exit_conf_mode_aa(dev); + pnp_exit_conf_mode(dev); }
/** diff --git a/src/superio/winbond/w83667hg-a/superio.c b/src/superio/winbond/w83667hg-a/superio.c index 4a995d6..a5a8c73 100644 --- a/src/superio/winbond/w83667hg-a/superio.c +++ b/src/superio/winbond/w83667hg-a/superio.c @@ -22,7 +22,6 @@ #include <pc80/keyboard.h> #include <pc80/mc146818rtc.h> #include <arch/acpi.h> -#include <superio/conf_mode.h>
#include "w83667hg-a.h"
@@ -43,29 +42,29 @@ /* TODO: Might potentially need code for HWM or FDC etc. */ case W83667HG_A_KBC: /* Enable mouse controller */ - pnp_enter_conf_mode_8787(dev); + pnp_enter_conf_mode(dev); byte = pnp_read_config(dev, 0x2a); byte &= ~(0x1 << 1); pnp_write_config(dev, 0x2a, byte); - pnp_exit_conf_mode_aa(dev); + pnp_exit_conf_mode(dev);
mouse_detected = pc_keyboard_init(PROBE_AUX_DEVICE);
if (!mouse_detected && !acpi_is_wakeup_s3()) { printk(BIOS_INFO, "%s: Disable mouse controller.", __func__); - pnp_enter_conf_mode_8787(dev); + pnp_enter_conf_mode(dev); byte = pnp_read_config(dev, 0x2a); byte |= 0x1 << 1; pnp_write_config(dev, 0x2a, byte); - pnp_exit_conf_mode_aa(dev); + pnp_exit_conf_mode(dev); } break; case W83667HG_A_ACPI: /* Set power state after power fail */ power_status = CONFIG_MAINBOARD_POWER_FAILURE_STATE; get_option(&power_status, "power_on_after_fail"); - pnp_enter_conf_mode_8787(dev); + pnp_enter_conf_mode(dev); pnp_set_logical_device(dev); byte = pnp_read_config(dev, 0xe4); byte &= ~0x60; @@ -74,7 +73,7 @@ else if (power_status == MAINBOARD_POWER_KEEP) byte |= (0x2 << 5); pnp_write_config(dev, 0xe4, byte); - pnp_exit_conf_mode_aa(dev); + pnp_exit_conf_mode(dev); printk(BIOS_INFO, "set power %s after power fail\n", power_status ? "on" : "off"); break; }
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37638 )
Change subject: superio/*: Don't use conf_mode directly ......................................................................
Patch Set 1: Code-Review-1
after removing the includes the structs assigned to ops_pnp_mode are out of scope.
apart from that i like the direction of the patch
Hello Felix Held, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37638
to look at the new patch set (#2).
Change subject: superio/*: Don't use conf_mode directly ......................................................................
superio/*: Don't use conf_mode directly
Use the functions defined in device/pnp.h instead of using the conf_mode directly. This will make future refactoring easier.
Change-Id: Ibb94d86b3ee861f44cded469ff58b545dd7311fd Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/superio/nuvoton/nct5572d/superio.c M src/superio/smsc/lpc47n227/superio.c M src/superio/winbond/w83667hg-a/superio.c 3 files changed, 18 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/37638/2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37638 )
Change subject: superio/*: Don't use conf_mode directly ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37638/2/src/superio/smsc/lpc47n227/... File src/superio/smsc/lpc47n227/superio.c:
https://review.coreboot.org/c/coreboot/+/37638/2/src/superio/smsc/lpc47n227/... PS2, Line 45: static struct device_operations ops = { the ops struct lacks the .ops_pnp_mode assignment, so i'd guess that this will break this sio driver
Hello Felix Held, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37638
to look at the new patch set (#3).
Change subject: superio/*: Don't use conf_mode directly ......................................................................
superio/*: Don't use conf_mode directly
Use the functions defined in device/pnp.h instead of using the conf_mode directly. This will make future refactoring easier.
Change-Id: Ibb94d86b3ee861f44cded469ff58b545dd7311fd Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/superio/nuvoton/nct5572d/superio.c M src/superio/smsc/lpc47n227/superio.c M src/superio/winbond/w83667hg-a/superio.c 3 files changed, 19 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/38/37638/3
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37638 )
Change subject: superio/*: Don't use conf_mode directly ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37638/2/src/superio/smsc/lpc47n227/... File src/superio/smsc/lpc47n227/superio.c:
https://review.coreboot.org/c/coreboot/+/37638/2/src/superio/smsc/lpc47n227/... PS2, Line 45: static struct device_operations ops = {
the ops struct lacks the . […]
Done
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37638 )
Change subject: superio/*: Don't use conf_mode directly ......................................................................
Patch Set 3: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37638 )
Change subject: superio/*: Don't use conf_mode directly ......................................................................
superio/*: Don't use conf_mode directly
Use the functions defined in device/pnp.h instead of using the conf_mode directly. This will make future refactoring easier.
Change-Id: Ibb94d86b3ee861f44cded469ff58b545dd7311fd Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/37638 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Felix Held felix-coreboot@felixheld.de --- M src/superio/nuvoton/nct5572d/superio.c M src/superio/smsc/lpc47n227/superio.c M src/superio/winbond/w83667hg-a/superio.c 3 files changed, 19 insertions(+), 18 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved
diff --git a/src/superio/nuvoton/nct5572d/superio.c b/src/superio/nuvoton/nct5572d/superio.c index 3084687..76c983a 100644 --- a/src/superio/nuvoton/nct5572d/superio.c +++ b/src/superio/nuvoton/nct5572d/superio.c @@ -43,29 +43,29 @@ /* TODO: Might potentially need code for HWM or FDC etc. */ case NCT5572D_KBC: /* Enable mouse controller */ - pnp_enter_conf_mode_8787(dev); + pnp_enter_conf_mode(dev); byte = pnp_read_config(dev, 0x2a); byte &= ~(0x1 << 1); pnp_write_config(dev, 0x2a, byte); - pnp_exit_conf_mode_aa(dev); + pnp_exit_conf_mode(dev);
mouse_detected = pc_keyboard_init(PROBE_AUX_DEVICE);
if (!mouse_detected) { printk(BIOS_INFO, "%s: Disable mouse controller.", __func__); - pnp_enter_conf_mode_8787(dev); + pnp_enter_conf_mode(dev); byte = pnp_read_config(dev, 0x2a); byte |= 0x1 << 1; pnp_write_config(dev, 0x2a, byte); - pnp_exit_conf_mode_aa(dev); + pnp_exit_conf_mode(dev); } break; case NCT5572D_ACPI: /* Set power state after power fail */ power_status = CONFIG_MAINBOARD_POWER_FAILURE_STATE; get_option(&power_status, "power_on_after_fail"); - pnp_enter_conf_mode_8787(dev); + pnp_enter_conf_mode(dev); pnp_set_logical_device(dev); byte = pnp_read_config(dev, 0xe4); byte &= ~0x60; @@ -74,7 +74,7 @@ else if (power_status == MAINBOARD_POWER_KEEP) byte |= (0x2 << 5); pnp_write_config(dev, 0xe4, byte); - pnp_exit_conf_mode_aa(dev); + pnp_exit_conf_mode(dev); printk(BIOS_INFO, "set power %s after power fail\n", power_status ? "on" : "off"); break; } diff --git a/src/superio/smsc/lpc47n227/superio.c b/src/superio/smsc/lpc47n227/superio.c index 57297ab..911343d 100644 --- a/src/superio/smsc/lpc47n227/superio.c +++ b/src/superio/smsc/lpc47n227/superio.c @@ -48,6 +48,7 @@ .enable_resources = lpc47n227_pnp_enable_resources, .enable = lpc47n227_pnp_enable, .init = lpc47n227_init, + .ops_pnp_mode = &pnp_conf_mode_55_aa, };
static struct pnp_info pnp_dev_info[] = { @@ -81,10 +82,10 @@ { struct resource *res;
- pnp_enter_conf_mode_55(dev); + pnp_enter_conf_mode(dev); for (res = dev->resource_list; res; res = res->next) lpc47n227_pnp_set_resource(dev, res); - pnp_exit_conf_mode_aa(dev); + pnp_exit_conf_mode(dev); }
/* @@ -93,9 +94,9 @@ */ void lpc47n227_pnp_enable_resources(struct device *dev) { - pnp_enter_conf_mode_55(dev); + pnp_enter_conf_mode(dev); lpc47n227_pnp_set_enable(dev, 1); - pnp_exit_conf_mode_aa(dev); + pnp_exit_conf_mode(dev); }
/* @@ -104,9 +105,9 @@ */ void lpc47n227_pnp_enable(struct device *dev) { - pnp_enter_conf_mode_55(dev); + pnp_enter_conf_mode(dev); lpc47n227_pnp_set_enable(dev, !!dev->enabled); - pnp_exit_conf_mode_aa(dev); + pnp_exit_conf_mode(dev); }
/** diff --git a/src/superio/winbond/w83667hg-a/superio.c b/src/superio/winbond/w83667hg-a/superio.c index 4a995d6..69ab91b 100644 --- a/src/superio/winbond/w83667hg-a/superio.c +++ b/src/superio/winbond/w83667hg-a/superio.c @@ -43,29 +43,29 @@ /* TODO: Might potentially need code for HWM or FDC etc. */ case W83667HG_A_KBC: /* Enable mouse controller */ - pnp_enter_conf_mode_8787(dev); + pnp_enter_conf_mode(dev); byte = pnp_read_config(dev, 0x2a); byte &= ~(0x1 << 1); pnp_write_config(dev, 0x2a, byte); - pnp_exit_conf_mode_aa(dev); + pnp_exit_conf_mode(dev);
mouse_detected = pc_keyboard_init(PROBE_AUX_DEVICE);
if (!mouse_detected && !acpi_is_wakeup_s3()) { printk(BIOS_INFO, "%s: Disable mouse controller.", __func__); - pnp_enter_conf_mode_8787(dev); + pnp_enter_conf_mode(dev); byte = pnp_read_config(dev, 0x2a); byte |= 0x1 << 1; pnp_write_config(dev, 0x2a, byte); - pnp_exit_conf_mode_aa(dev); + pnp_exit_conf_mode(dev); } break; case W83667HG_A_ACPI: /* Set power state after power fail */ power_status = CONFIG_MAINBOARD_POWER_FAILURE_STATE; get_option(&power_status, "power_on_after_fail"); - pnp_enter_conf_mode_8787(dev); + pnp_enter_conf_mode(dev); pnp_set_logical_device(dev); byte = pnp_read_config(dev, 0xe4); byte &= ~0x60; @@ -74,7 +74,7 @@ else if (power_status == MAINBOARD_POWER_KEEP) byte |= (0x2 << 5); pnp_write_config(dev, 0xe4, byte); - pnp_exit_conf_mode_aa(dev); + pnp_exit_conf_mode(dev); printk(BIOS_INFO, "set power %s after power fail\n", power_status ? "on" : "off"); break; }
-
Felix Held (Code Review) -
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)