Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33155 )
Change subject: drivers/wifi: Add generic WiFi driver ......................................................................
Patch Set 3:
(7 comments)
https://review.coreboot.org/#/c/33155/3/src/drivers/intel/wifi/Kconfig File src/drivers/intel/wifi/Kconfig:
https://review.coreboot.org/#/c/33155/3/src/drivers/intel/wifi/Kconfig@3 PS3, Line 3: depends on ARCH_X86
Followup, try replace this with 'depends on PCI'. […]
I will upload a follow-up patch on that one.
https://review.coreboot.org/#/c/33155/3/src/drivers/intel/wifi/wifi.c File src/drivers/intel/wifi/wifi.c:
https://review.coreboot.org/#/c/33155/3/src/drivers/intel/wifi/wifi.c@68 PS3, Line 68: struct drivers_intel_wifi_config *config = dev->chip_info;
Need to test 'dev->chip != NULL' before 'config->wake'.
Ok.
https://review.coreboot.org/#/c/33155/3/src/drivers/intel/wifi/wifi.c@73 PS3, Line 73: generic_wifi_fill_ssdt(dev, &generic_config);
Consider passing NULL as a second argument for case without chip_info, fill_ssdt() can then skip _PR […]
Ok.
https://review.coreboot.org/#/c/33155/3/src/drivers/intel/wifi/wifi.c@83 PS3, Line 83: val = pci_read_config16(dev, PMCS_DR);
Not the scope, but is this a standard Power Management capability block? It would be better to use p […]
I need to check on this further and update on this comment later.
https://review.coreboot.org/#/c/33155/3/src/drivers/wifi/Kconfig File src/drivers/wifi/Kconfig:
https://review.coreboot.org/#/c/33155/3/src/drivers/wifi/Kconfig@9 PS3, Line 9: config USE_SAR
Rest of file should go inside 'if DRIVERS_GENERIC_WIFI' block.
Ok.
https://review.coreboot.org/#/c/33155/3/src/drivers/wifi/generic.c File src/drivers/wifi/generic.c:
https://review.coreboot.org/#/c/33155/3/src/drivers/wifi/generic.c@174 PS3, Line 174: if (!path || !dev->enabled)
Maybe test 'dev->enabled' before you even call this function? I am not 100% sure if 'dev->bus' refer […]
Ok.
https://review.coreboot.org/#/c/33155/3/src/drivers/wifi/generic.c@191 PS3, Line 191: if (config && config->wake)
To use GPE0 (config->wake == 0) should be allowed. […]
Ok.