Attention is currently required from: Daniel Maslowski, Felix Singer, Iru Cai, Martin L Roth, Paul Menzel.
Nicholas Chin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/77534?usp=email )
Change subject: ec/dell/mec5035: Add command to control radio state ......................................................................
Patch Set 9:
(4 comments)
This change is ready for review.
Patchset:
PS6:
Suggestion for a more human speak implementation: […]
Went with `mec5035_control_radio(enum ec_radio_dev dev, enum ec_radio_state state)`, so that something like `mec5035_control_radio(RADIO_WLAN, RADIO_ON)` could be used. I think this balances readability and avoiding duplicating code.
File src/ec/dell/mec5035/mec5035.h:
https://review.coreboot.org/c/coreboot/+/77534/comment/a8062af8_38bceed4 : PS6, Line 19: EN
So is this an Enable, or a toggle? The commit message suggests it might be a toggle.
The 0x2b command is used for both turning on and off the radio, with arguments to specify which device to target and whether to turn it on or off. The switch on the side is a slide switch with 2 positions, on and off. I've changed EN to CTRL
File src/ec/dell/mec5035/mec5035.c:
https://review.coreboot.org/c/coreboot/+/77534/comment/384bfe03_66701a8e : PS6, Line 91: dev
The compiler will probably do the right thing here, but it wouldn't hurt to cast this to a u8
Done
https://review.coreboot.org/c/coreboot/+/77534/comment/74803dba_2e3bab6a : PS6, Line 92: 3
I know the other commands in this file just use bare values, but could this maybe be `sizeof(buf)`? […]
Done
-
Nicholas Chin (Code Review)