lkml.org 
[lkml]   [2018]   [Apr]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 9/9] brcmfmac: use request_firmware_nowait2 to load firmware without warnings
From
Date


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

\
 
 \ /
  Last update: 2018-04-20 21:34    [W:0.174 / U:1.364 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site