by Mathy Vanhoef

[permalink] [raw]

Subject: Re: [PATCH 4/7] brcmfmac: obtain feature info using 'cap' firmware command

On Fri, Jan 1, 2016 at 8:05 AM, Arend van Spriel <[email protected]> wrote:
> On 12/29/2015 09:58 PM, Mathy Vanhoef wrote:
>>
>> On Mon, Dec 14, 2015 at 2:39 PM, Arend van Spriel <[email protected]>
>> wrote:
>>>
>>> Several features in the driver directly map to a firmware feature
>>> listed in response of the 'cap' firmware command. For those features
>>> this response will be examined instead of attempting individual
>>> firmware commands.
>>>
>>> Reviewed-by: Hante Meuleman <[email protected]>
>>> Reviewed-by: Franky (Zhenhui) Lin <[email protected]>
>>> Reviewed-by: Pieter-Paul Giesberts <[email protected]>
>
>
> Hi Mathy,
>
> Do you want credits, ie. have "Reviewed-by:" tag added?

Sure, glad I could help.

Kr,
Mathy

>
> Regards,
> Arend
>
>
>>> Signed-off-by: Arend van Spriel <[email protected]>
>>> ---
>>> .../wireless/broadcom/brcm80211/brcmfmac/feature.c | 51
>>> +++++++++++++---------
>>> 1 file changed, 30 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>>> index d9d1ca4..08b7200 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>>> @@ -40,6 +40,17 @@ static const char *brcmf_feat_names[] = {
>>> };
>>> #undef BRCMF_FEAT_DEF
>>>
>>> +struct brcmf_feat_fwcap {
>>> + enum brcmf_feat_id feature;
>>> + const char * const fwcap_id;
>>> +};
>>> +
>>> +static const struct brcmf_feat_fwcap brcmf_fwcap_map[] = {
>>> + { BRCMF_FEAT_MBSS, "mbss" },
>>> + { BRCMF_FEAT_MCHAN, "mchan" },
>>> + { BRCMF_FEAT_P2P, "p2p" },
>>> +};
>>> +
>>> #ifdef DEBUG
>>> /*
>>> * expand quirk list to array of quirk strings.
>>> @@ -104,25 +115,22 @@ static void brcmf_feat_iovar_int_get(struct
>>> brcmf_if *ifp,
>>> }
>>> }
>>>
>>> -/**
>>> - * brcmf_feat_iovar_int_set() - determine feature through iovar set.
>>> - *
>>> - * @ifp: interface to query.
>>> - * @id: feature id.
>>> - * @name: iovar name.
>>> - */
>>> -static void brcmf_feat_iovar_int_set(struct brcmf_if *ifp,
>>> - enum brcmf_feat_id id, char *name,
>>> u32 val)
>>> +static void brcmf_feat_firmware_capabilities(struct brcmf_if *ifp)
>>> {
>>> - int err;
>>> -
>>> - err = brcmf_fil_iovar_int_set(ifp, name, val);
>>> - if (err == 0) {
>>> - brcmf_dbg(INFO, "enabling feature: %s\n",
>>> brcmf_feat_names[id]);
>>> - ifp->drvr->feat_flags |= BIT(id);
>>> - } else {
>>> - brcmf_dbg(TRACE, "%s feature check failed: %d\n",
>>> - brcmf_feat_names[id], err);
>>> + char caps[256];
>>> + enum brcmf_feat_id id;
>>> + int i;
>>> +
>>> + brcmf_fil_iovar_data_get(ifp, "cap", caps, sizeof(caps));
>>> + brcmf_dbg(INFO, "[ %s]\n", caps);
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(brcmf_fwcap_map); i++) {
>>> + if (strnstr(caps, brcmf_fwcap_map[i].fwcap_id,
>>> sizeof(caps))) {
>>> + id = brcmf_fwcap_map[i].feature;
>>> + brcmf_dbg(INFO, "enabling feature: %s\n",
>>> + brcmf_feat_names[id]);
>>> + ifp->drvr->feat_flags |= BIT(id);
>>> + }
>>> }
>>> }
>>>
>>> @@ -130,13 +138,14 @@ void brcmf_feat_attach(struct brcmf_pub *drvr)
>>> {
>>> struct brcmf_if *ifp = brcmf_get_ifp(drvr, 0);
>>>
>>> - brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_MCHAN, "mchan");
>>> + brcmf_feat_firmware_capabilities(ifp);
>>> +
>>> brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_PNO, "pfn");
>>> if (drvr->bus_if->wowl_supported)
>>> brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_WOWL, "wowl");
>>> + /* MBSS does not work for 43362 */
>>> if (drvr->bus_if->chip != BRCM_CC_43362_CHIP_ID)
>>> - brcmf_feat_iovar_int_set(ifp, BRCMF_FEAT_MBSS, "mbss",
>>> 0);
>>> - brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_P2P, "p2p");
>>> + ifp->drvr->feat_flags &= BIT(BRCMF_FEAT_MBSS);
>>
>>
>> Missing ~ before the BIT() declaration? If the if-test fails, all bits are
>> cleared except BRCMF_FEAT_MBSS. I think the if-test also needs to be
>> updated
>> to an equals `==` instead of the old inequality. So one would get:
>>
>> /* clear MBSS feature for the 43362 (does not work) */
>> if (drvr->bus_if->chip == BRCM_CC_43362_CHIP_ID)
>> ifp->drvr->feat_flags &= ~BIT(BRCMF_FEAT_MBSS);
>>
>> Also, happy holidays!
>> Mathy
>>
>>> brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_RSDB, "rsdb_mode");
>>> brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_TDLS, "tdls_enable");
>>>
>>> --
>>> 1.9.1
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-wireless"
>>> in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-wireless"
>> in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>

2016-01-01 07:05:35

by Arend van Spriel

[permalink] [raw]

Subject: Re: [PATCH 4/7] brcmfmac: obtain feature info using 'cap' firmware command

On 12/29/2015 09:58 PM, Mathy Vanhoef wrote:
> On Mon, Dec 14, 2015 at 2:39 PM, Arend van Spriel <[email protected]> wrote:
>> Several features in the driver directly map to a firmware feature
>> listed in response of the 'cap' firmware command. For those features
>> this response will be examined instead of attempting individual
>> firmware commands.
>>
>> Reviewed-by: Hante Meuleman <[email protected]>
>> Reviewed-by: Franky (Zhenhui) Lin <[email protected]>
>> Reviewed-by: Pieter-Paul Giesberts <[email protected]>

Hi Mathy,

Do you want credits, ie. have "Reviewed-by:" tag added?

Regards,
Arend

>> Signed-off-by: Arend van Spriel <[email protected]>
>> ---
>> .../wireless/broadcom/brcm80211/brcmfmac/feature.c | 51 +++++++++++++---------
>> 1 file changed, 30 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>> index d9d1ca4..08b7200 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
>> @@ -40,6 +40,17 @@ static const char *brcmf_feat_names[] = {
>> };
>> #undef BRCMF_FEAT_DEF
>>
>> +struct brcmf_feat_fwcap {
>> + enum brcmf_feat_id feature;
>> + const char * const fwcap_id;
>> +};
>> +
>> +static const struct brcmf_feat_fwcap brcmf_fwcap_map[] = {
>> + { BRCMF_FEAT_MBSS, "mbss" },
>> + { BRCMF_FEAT_MCHAN, "mchan" },
>> + { BRCMF_FEAT_P2P, "p2p" },
>> +};
>> +
>> #ifdef DEBUG
>> /*
>> * expand quirk list to array of quirk strings.
>> @@ -104,25 +115,22 @@ static void brcmf_feat_iovar_int_get(struct brcmf_if *ifp,
>> }
>> }
>>
>> -/**
>> - * brcmf_feat_iovar_int_set() - determine feature through iovar set.
>> - *
>> - * @ifp: interface to query.
>> - * @id: feature id.
>> - * @name: iovar name.
>> - */
>> -static void brcmf_feat_iovar_int_set(struct brcmf_if *ifp,
>> - enum brcmf_feat_id id, char *name, u32 val)
>> +static void brcmf_feat_firmware_capabilities(struct brcmf_if *ifp)
>> {
>> - int err;
>> -
>> - err = brcmf_fil_iovar_int_set(ifp, name, val);
>> - if (err == 0) {
>> - brcmf_dbg(INFO, "enabling feature: %s\n", brcmf_feat_names[id]);
>> - ifp->drvr->feat_flags |= BIT(id);
>> - } else {
>> - brcmf_dbg(TRACE, "%s feature check failed: %d\n",
>> - brcmf_feat_names[id], err);
>> + char caps[256];
>> + enum brcmf_feat_id id;
>> + int i;
>> +
>> + brcmf_fil_iovar_data_get(ifp, "cap", caps, sizeof(caps));
>> + brcmf_dbg(INFO, "[ %s]\n", caps);
>> +
>> + for (i = 0; i < ARRAY_SIZE(brcmf_fwcap_map); i++) {
>> + if (strnstr(caps, brcmf_fwcap_map[i].fwcap_id, sizeof(caps))) {
>> + id = brcmf_fwcap_map[i].feature;
>> + brcmf_dbg(INFO, "enabling feature: %s\n",
>> + brcmf_feat_names[id]);
>> + ifp->drvr->feat_flags |= BIT(id);
>> + }
>> }
>> }
>>
>> @@ -130,13 +138,14 @@ void brcmf_feat_attach(struct brcmf_pub *drvr)
>> {
>> struct brcmf_if *ifp = brcmf_get_ifp(drvr, 0);
>>
>> - brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_MCHAN, "mchan");
>> + brcmf_feat_firmware_capabilities(ifp);
>> +
>> brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_PNO, "pfn");
>> if (drvr->bus_if->wowl_supported)
>> brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_WOWL, "wowl");
>> + /* MBSS does not work for 43362 */
>> if (drvr->bus_if->chip != BRCM_CC_43362_CHIP_ID)
>> - brcmf_feat_iovar_int_set(ifp, BRCMF_FEAT_MBSS, "mbss", 0);
>> - brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_P2P, "p2p");
>> + ifp->drvr->feat_flags &= BIT(BRCMF_FEAT_MBSS);
>
> Missing ~ before the BIT() declaration? If the if-test fails, all bits are
> cleared except BRCMF_FEAT_MBSS. I think the if-test also needs to be updated
> to an equals `==` instead of the old inequality. So one would get:
>
> /* clear MBSS feature for the 43362 (does not work) */
> if (drvr->bus_if->chip == BRCM_CC_43362_CHIP_ID)
> ifp->drvr->feat_flags &= ~BIT(BRCMF_FEAT_MBSS);
>
> Also, happy holidays!
> Mathy
>
>> brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_RSDB, "rsdb_mode");
>> brcmf_feat_iovar_int_get(ifp, BRCMF_FEAT_TDLS, "tdls_enable");
>>
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>