Messages in this thread | | | Subject | Re: [PATCH 9/9] brcmfmac: use request_firmware_nowait2 to load firmware without warnings | From | Andres Rodriguez <> | Date | Fri, 20 Apr 2018 15:33:36 -0400 |
| |
On 2018-04-20 06:26 AM, Kalle Valo wrote: > Andres Rodriguez <andresx7@gmail.com> writes: > >> This reduces the unnecessary spew when trying to load optional firmware: >> "Direct firmware load for ... failed with error -2" >> >> Signed-off-by: Andres Rodriguez <andresx7@gmail.com> >> --- >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) > > With wireless patches always CC linux-wireless list, please. Adding it > now. > >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >> index 091b52979e03..26db3ebd52dc 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >> @@ -503,8 +503,9 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx) >> goto done; >> >> fwctx->code = fw; >> - ret = request_firmware_nowait(THIS_MODULE, true, fwctx->nvram_name, >> - fwctx->dev, GFP_KERNEL, fwctx, >> + ret = request_firmware_nowait(THIS_MODULE, true, false, > > A perfect example why enums should be in function calls instead of > booleans, that "true, false" tells nothing to me and it would be time > consuming to check from headers files what it means. If you had proper > enums, for example "FIRMWARE_MODE_FOO, FIRMWARE_STATE_BAR", it would be > immediately obvious for the reader what the parameters are. Of course > the first boolean was already there before, but maybe change the new > boolean to an enum >
Anyone else got any feedback before I re-spin the _nowait() API. I'm on board for the boolean->enum change.
>> + fwctx->nvram_name, fwctx->dev, >> + GFP_KERNEL, fwctx, >> brcmf_fw_request_nvram_done); >> >> /* pass NULL to nvram callback for bcm47xx fallback */ >> @@ -547,7 +548,7 @@ int brcmf_fw_get_firmwares_pcie(struct device *dev, u16 flags, >> fwctx->domain_nr = domain_nr; >> fwctx->bus_nr = bus_nr; >> >> - return request_firmware_nowait(THIS_MODULE, true, code, dev, >> + return request_firmware_nowait2(THIS_MODULE, true, false, code, dev, >> GFP_KERNEL, fwctx, >> brcmf_fw_request_code_done); >> } > > Also the number two in the function name is not really telling anything. > I think that something like request_firmware_nowait_nowarn() would be > better, even if it's so ugly. >
The 2 is meant to signify that this is an new version of the api with different parameters. I don't think we need to codify into the name what the new parameters mean (mostly because when a 2 version of an api is implemented, usage of the original version tends to be discouraged).
If we go for something like _nowait_nowarn(), then we would need to drop the warn parameter altogether.
Another alternative, drop both bool warn and bool uevent and expose take in enum fw_opt directly.
Any thought on exposing the enum directly Luis for _nowait(). I know you mentioned this was for some reason decided against for the rest of the API.
Regards, Andres
| |