Change in ...coreboot[master]: drivers/intel/wifi: Select DRIVERS_INTEL_WIFI if your mainboard needs it
HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33827
Change subject: drivers/intel/wifi: Select DRIVERS_INTEL_WIFI if your mainboard needs it ......................................................................
drivers/intel/wifi: Select DRIVERS_INTEL_WIFI if your mainboard needs it
Change-Id: Iaaab6df43968abada6397acf7402264dea669c21 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/drivers/intel/wifi/Kconfig 1 file changed, 3 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/33827/1
diff --git a/src/drivers/intel/wifi/Kconfig b/src/drivers/intel/wifi/Kconfig index 4dc4d7f..a597d40 100644 --- a/src/drivers/intel/wifi/Kconfig +++ b/src/drivers/intel/wifi/Kconfig @@ -1,10 +1,9 @@ config DRIVERS_INTEL_WIFI - bool "Support Intel PCI-e WiFi adapters" + bool "Intel WiFi" depends on ARCH_X86 - default y if PCIEXP_PLUGIN_SUPPORT + default n help - When enabled, add identifiers in ACPI and SMBIOS tables to - make OS drivers work with certain Intel PCI-e WiFi chipsets. + Enabled, if the mainboard needs Intel WiFi driver.
config USE_SAR bool
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33827 )
Change subject: drivers/intel/wifi: Select DRIVERS_INTEL_WIFI if your mainboard needs it ......................................................................
Patch Set 1: Code-Review-2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33827 )
Change subject: drivers/intel/wifi: Select DRIVERS_INTEL_WIFI if your mainboard needs it ......................................................................
Patch Set 1: Code-Review-1
(2 comments)
What do default builds gain from this change?
https://review.coreboot.org/#/c/33827/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33827/1//COMMIT_MSG@7 PS1, Line 7: drivers/intel/wifi: Select DRIVERS_INTEL_WIFI if your mainboard needs it Too long for a commit summary.
And it is missing a good reason to break intel wireless on default builds.
https://review.coreboot.org/#/c/33827/1/src/drivers/intel/wifi/Kconfig File src/drivers/intel/wifi/Kconfig:
PS1: So most of the information about this Kconfig option gets dropped?
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33827 )
Change subject: drivers/intel/wifi: Select DRIVERS_INTEL_WIFI if your mainboard needs it ......................................................................
Patch Set 1:
Patch Set 1: Code-Review-2
why Should I compile "src/drivers/intel/wifi/wifi.c" if the board I have do not have intel's wifi ? is it for smbios and acpi ?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33827 )
Change subject: drivers/intel/wifi: Select DRIVERS_INTEL_WIFI if your mainboard needs it ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1: Code-Review-2
why Should I compile "src/drivers/intel/wifi/wifi.c" if the board I have do not have intel's wifi ? is it for smbios and acpi ?
Revisit previous reviews, e.g. search DRIVERS_INTEL_WIFI in gerrit.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33827 )
Change subject: drivers/intel/wifi: Select DRIVERS_INTEL_WIFI if your mainboard needs it ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1: Code-Review-2
why Should I compile "src/drivers/intel/wifi/wifi.c" if the board I have do not have intel's wifi ? is it for smbios and acpi ?
The only way to determine at compile-time if that driver is needed is to have the user specify so. Default builds should work in as many cases as possible, so disabling this driver by default means NOBODY with an intel wireless card will be able to use it with a default coreboot build.
If you do not want that driver because you do not have an intel wireless card, use site-local to disable that option by default for yourself.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33827 )
Change subject: drivers/intel/wifi: Select DRIVERS_INTEL_WIFI if your mainboard needs it ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1: Code-Review-2
why Should I compile "src/drivers/intel/wifi/wifi.c" if the board I have do not have intel's wifi ? is it for smbios and acpi ?
Revisit previous reviews, e.g. search DRIVERS_INTEL_WIFI in gerrit.
CB:31222 CB:31308 CB:31225
HAOUAS Elyes has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33827 )
Change subject: drivers/intel/wifi: Select DRIVERS_INTEL_WIFI if your mainboard needs it ......................................................................
Abandoned
see https://review.coreboot.org/c/coreboot/+/33155
HAOUAS Elyes has restored this change. ( https://review.coreboot.org/c/coreboot/+/33827 )
Change subject: drivers/intel/wifi: Select DRIVERS_INTEL_WIFI if your mainboard needs it ......................................................................
Restored
Hello Kyösti Mälkki, Angel Pons, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33827
to look at the new patch set (#2).
Change subject: wifi/Kconfig: Add new config MAINBOARD_HAS_NO_WIFI ......................................................................
wifi/Kconfig: Add new config MAINBOARD_HAS_NO_WIFI
This is allow boards that doesn't have any WIFI to unselect DRIVERS_INTEL_WIFI.
Change-Id: Iaaab6df43968abada6397acf7402264dea669c21 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/drivers/intel/wifi/Kconfig 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/33827/2
Hello Kyösti Mälkki, Angel Pons, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33827
to look at the new patch set (#3).
Change subject: wifi/Kconfig: Add new config MAINBOARD_HAS_NO_WIFI ......................................................................
wifi/Kconfig: Add new config MAINBOARD_HAS_NO_WIFI
This is allow boards that doesn't have any WIFI to unselect DRIVERS_INTEL_WIFI.
Change-Id: Iaaab6df43968abada6397acf7402264dea669c21 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/drivers/intel/wifi/Kconfig 1 file changed, 4 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/33827/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33827 )
Change subject: wifi/Kconfig: Add new config MAINBOARD_HAS_NO_WIFI ......................................................................
Patch Set 3: Code-Review-1
(1 comment)
Same reasons as before. I didn't give a -2 only because I can't.
https://review.coreboot.org/#/c/33827/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33827/3//COMMIT_MSG@9 PS3, Line 9: This is allow boards that doesn't have any WIFI to unselect : DRIVERS_INTEL_WIFI. 1. What would you, or anyone else for the matter, want this option for?
2. Will, e.g., Kyösti, be able to select DRIVERS_INTEL_WIFI for his G505s?
3. Will, e.g., I, be able to select DRIVERS_INTEL_WIFI for my desktop boards? And for qemu? (I could PCIe pass through a wireless card to a VM, and it would need the tables this driver creates)
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33827 )
Change subject: wifi/Kconfig: Add new config MAINBOARD_HAS_NO_WIFI ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33827/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33827/3//COMMIT_MSG@9 PS3, Line 9: This is allow boards that doesn't have any WIFI to unselect : DRIVERS_INTEL_WIFI.
- What would you, or anyone else for the matter, want this option for? […]
1- this is allow to get ride of that driver shit 2- no need, MAINBOARD_HAS_NO_WIFI is = n by default, so the the default config will load that DRIVERS_INTEL_WIFI as (!MAINBOARD_HAS_NO_WIFI) =true 3- see #2
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33827 )
Change subject: wifi/Kconfig: Add new config MAINBOARD_HAS_NO_WIFI ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33827/3/src/drivers/intel/wifi/Kconfig File src/drivers/intel/wifi/Kconfig:
https://review.coreboot.org/#/c/33827/3/src/drivers/intel/wifi/Kconfig@7 PS3, Line 7: !MAINBOARD_HAS_NO_WIFI as long as you do not select "MAINBOARD_HAS_NO_WIFI", the default value for RIVERS_INTEL_WIFI is yes.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33827 )
Change subject: wifi/Kconfig: Add new config MAINBOARD_HAS_NO_WIFI ......................................................................
Patch Set 3:
Elyes, can you please calm down. If I read "shit" one more time, I'll likely give -2, too, and ignore further comments.
That you named it "MAINBOARD_HAS_NO_WIFI" shows that you still didn't understand what the `default y` for the Intel driver is about. It is about plug-in cards that somebody might plug into a board. Not about something that ships with the board. Just like somebody might plug an SATA drive. With your argumentation we could as well disable ATA spport by default unless somebody opts in.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33827 )
Change subject: wifi/Kconfig: Add new config MAINBOARD_HAS_NO_WIFI ......................................................................
Patch Set 4: Code-Review-1
what we have is the same as:
config INSTALL_WINDOS depends on ARCH_X86 default y if YOU_HAVE_A_PC
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33827 )
Change subject: wifi/Kconfig: Add new config MAINBOARD_HAS_NO_WIFI ......................................................................
Patch Set 4:
(1 comment)
Patch Set 4: Code-Review-1
what we have is the same as:
config INSTALL_WINDOS depends on ARCH_X86 default y if YOU_HAVE_A_PC
Definitely not.
Have you checked the code behind DRIVERS_INTEL_WIFI? Does said code get run if no device with a matching PCI ID is found?
https://review.coreboot.org/#/c/33827/3/src/drivers/intel/wifi/Kconfig File src/drivers/intel/wifi/Kconfig:
https://review.coreboot.org/#/c/33827/3/src/drivers/intel/wifi/Kconfig@7 PS3, Line 7: !MAINBOARD_HAS_NO_WIFI
as long as you do not select "MAINBOARD_HAS_NO_WIFI", the default value for RIVERS_INTEL_WIFI is yes […]
And which mainboard would select that?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33827 )
Change subject: wifi/Kconfig: Add new config MAINBOARD_HAS_NO_WIFI ......................................................................
Patch Set 4:
what we have is the same as:
config INSTALL_WINDOS depends on ARCH_X86 default y if YOU_HAVE_A_PC
That's your very own interpretation. I don't know why you see it like this. What's bad about Intel WIFI? compared to what is available on the market, they are not less open- source friendly than anyone else. It seems rather the opposite is true.
For me it's more like
config INSTALL_KERNEL_MODULES depends on HAVE_FILESYSTEM default y if MODULES_MIGHT_BE_USEFUL
I mean this is how things look like in a regular Linux distribution: You install all drivers by default, because otherwise it might give a bad user experience.
HAOUAS Elyes has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33827 )
Change subject: wifi/Kconfig: Add new config MAINBOARD_HAS_NO_WIFI ......................................................................
Abandoned
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33827 )
Change subject: wifi/Kconfig: Add new config MAINBOARD_HAS_NO_WIFI ......................................................................
Patch Set 4:
Abandoned
*shrug* so was this really just FUD?
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33827 )
Change subject: wifi/Kconfig: Add new config MAINBOARD_HAS_NO_WIFI ......................................................................
Patch Set 4:
I'm neither the first NOR the LAST to be annoyed by this option that I deactivate each time as I definitely don't need it at all.
The solution is to do it on local change (current one) ...
(out of topic) do you use a DVB-C PCI card? I'm sure, no and you don't even compile the module :p
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33827 )
Change subject: wifi/Kconfig: Add new config MAINBOARD_HAS_NO_WIFI ......................................................................
Patch Set 4:
I'm neither the first NOR the LAST to be annoyed by this option that I deactivate each time as I definitely don't need it at all.
That's ok. Not everybody needs it. But what you suggested was to disable it for everyone for specific boards. Why do you want to make that decision for other people?
As a developer I'm also annoyed by many defaults that make my work harder, e.g. SeaBIOS is enabled by default and its integration is usually broken (`make -j` doesn't work, it gets recompiled on every `make`). Still having a default that works for many users seems reasonable, no?
Btw. this SeaBIOS example is about actual issues in the work- flow. So far I guess you disable INTEL_WIFI because you don't like it?
The solution is to do it on local change (current one) ...
... or override the default in your site-local/Kconfig, or keep your .config file. Or leave INTEL_WIFI enabled, because it doesn't hurt?
(out of topic) do you use a DVB-C PCI card?
No, but I used a USB DVB-C dongle once. And I was happy that I didn't had to recompile my kernel to use it.
I'm sure, no and you don't even compile the module :p
If I wanted to compile my own kernel, I would most likely start with the .config of my Linux distribution. And then I would have to disable DVB-C PCI modules if I don't want to compile them.
-
Angel Pons (Code Review) -
HAOUAS Elyes (Code Review)
-
Kyösti Mälkki (Code Review)
-
Nico Huber (Code Review)