Hello Mike Banon,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/31219
to review the following change.
Change subject: src/drivers/intel/wifi/Kconfig: add NO_INTEL_WIFI option ......................................................................
src/drivers/intel/wifi/Kconfig: add NO_INTEL_WIFI option
This NO_INTEL_WIFI option will help to disable DRIVERS_INTEL_WIFI by default for the boards which are unlikely to have Intel PCI-e WiFi adapters.
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: Ia21a141e25e58f6126a6d48d83cdfb4f746f3320 --- M src/drivers/intel/wifi/Kconfig 1 file changed, 5 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/31219/1
diff --git a/src/drivers/intel/wifi/Kconfig b/src/drivers/intel/wifi/Kconfig index 4dc4d7f..4082d7e 100644 --- a/src/drivers/intel/wifi/Kconfig +++ b/src/drivers/intel/wifi/Kconfig @@ -1,11 +1,15 @@ config DRIVERS_INTEL_WIFI bool "Support Intel PCI-e WiFi adapters" depends on ARCH_X86 - default y if PCIEXP_PLUGIN_SUPPORT + default y if PCIEXP_PLUGIN_SUPPORT && !NO_INTEL_WIFI help When enabled, add identifiers in ACPI and SMBIOS tables to make OS drivers work with certain Intel PCI-e WiFi chipsets.
+# Select this for mainboard which is unlikely to have Intel PCI-e WiFi adapter. +config NO_INTEL_WIFI + def_bool n + config USE_SAR bool default n
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31219 )
Change subject: src/drivers/intel/wifi/Kconfig: add NO_INTEL_WIFI option ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31219/1/src/drivers/intel/wifi/Kconfig File src/drivers/intel/wifi/Kconfig:
https://review.coreboot.org/#/c/31219/1/src/drivers/intel/wifi/Kconfig@4 PS1, Line 4: default y if PCIEXP_PLUGIN_SUPPORT && !NO_INTEL_WIFI iirc, the decision to default to `y` was because you can never tell what the user did to his hardware.
If we'd change that, I would prefer a per-board opt-in rather than an opt-out.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31219 )
Change subject: src/drivers/intel/wifi/Kconfig: add NO_INTEL_WIFI option ......................................................................
Patch Set 1: Code-Review-1
(1 comment)
I would like to know the reasoning behind this change.
AFAIUI, the current default is meant to make Intel wireless cards work by default, since different people have different hardware configurations. It does no harm to those without such hardware, and makes the default configuration for a board more likely to work well. And anyone who doesn't want to use the driver can disable it in Kconfig.
https://review.coreboot.org/#/c/31219/1/src/drivers/intel/wifi/Kconfig File src/drivers/intel/wifi/Kconfig:
https://review.coreboot.org/#/c/31219/1/src/drivers/intel/wifi/Kconfig@4 PS1, Line 4: default y if PCIEXP_PLUGIN_SUPPORT && !NO_INTEL_WIFI
iirc, the decision to default to `y` was because you can never […]
I agree, negative Kconfig options are usually troublesome.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31219 )
Change subject: src/drivers/intel/wifi/Kconfig: add NO_INTEL_WIFI option ......................................................................
Patch Set 1:
AFAIUI, the current default is meant to make Intel wireless cards work by default, since different people have different hardware configurations. [...] and makes the default configuration for a board more likely to work well.
This is why NO_INTEL_WIFI is going to be enabled only at systems where it is highly improbable to meet the Intel PCI-e WiFi module. For example, you can't meet Intel WiFi at Emulation boards, but DRIVERS_INTEL_WIFI is erroneously enabled even for such boards.
It does no harm to those without such hardware, and anyone who doesn't want to use the driver can disable it in Kconfig.
It consumes a few kilobytes at CBFS, and after seeing that all the other Generic Drivers are disabled by default I can't understand why there should be made an exception for Intel.
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31219 )
Change subject: src/drivers/intel/wifi/Kconfig: add NO_INTEL_WIFI option ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31219/1/src/drivers/intel/wifi/Kconfig File src/drivers/intel/wifi/Kconfig:
https://review.coreboot.org/#/c/31219/1/src/drivers/intel/wifi/Kconfig@4 PS1, Line 4: default y if PCIEXP_PLUGIN_SUPPORT && !NO_INTEL_WIFI
iirc, the decision to default to `y` was because you can never
tell what the user did to his hardware.
But there are boards where it is highly improbable to meet the Intel PCI-e WiFi module (like G505S laptop) or impossible at all (Emulation boards). I believe that DRIVERS_INTEL_WIFI should be disabled by default for such boards, just like the other Generic Drivers.
If we'd change that, I would prefer a per-board opt-in rather than an opt-out.
I agree, negative Kconfig options are usually troublesome.
I went with a negative option because it seemed to provide the easiest way of deselecting DRIVERS_INTEL_WIFI config from the board's Kconfig, but I am looking for the better alternatives. If you have more ideas about this, please share
mikeb mikeb has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31219 )
Change subject: src/drivers/intel/wifi/Kconfig: add NO_INTEL_WIFI option ......................................................................
Patch Set 1:
After looking at Nico's suggestion at CB:31220 it seems we can do it without adding the new options, so I will abandon this change
mikeb mikeb has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31219 )
Change subject: src/drivers/intel/wifi/Kconfig: add NO_INTEL_WIFI option ......................................................................
Abandoned
new options aren't needed to do this
-
Angel Pons (Code Review) -
mikeb mikeb (Code Review)
-
Nico Huber (Code Review)