Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4e5c9cc1-a00e-c3de-3186-6d4cf5694339@gmail.com>
Date:   Fri, 29 Jul 2022 11:32:25 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH 3/7] brcmfmac: add support for vendor-specific firmware
 api
Content-Language: en-US
To:     Kalle Valo <kvalo@kernel.org>
Cc:     arend.vanspriel@broadcom.com, linux-wireless@vger.kernel.org
References: <20220613091915.18884-1-arend.vanspriel@broadcom.com>
 <CAJ65rDxy3ERmDAV1BYH_BCUM3_GnzotWpp7yYO6OtA_CZVgbzw@mail.gmail.com>
 <87sfmlo9yi.fsf@kernel.org>
From:   Arend Van Spriel <aspriel@gmail.com>
In-Reply-To: <87sfmlo9yi.fsf@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/28/2022 11:36 AM, Kalle Valo wrote:
> aspriel@gmail.com writes:
> 
>> The driver is being used by multiple vendors who develop the firmware
>> api independently. So far the firmware api as used by the driver has
>> not diverged (yet). This change adds framework for supporting multiple
>> firmware apis. The vendor-specific support code has to provide a number
>> of callback operations. Right now it is only attach and detach callbacks
>> so no real functionality as the api is still common. This code only
>> adds WCC variant anyway, which is selected for all devices right now.
>>
>> Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
>> Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
>> Reviewed-by: Franky Lin <franky.lin@broadcom.com>
>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> 
> [...]
> 
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Kconfig
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Kconfig
>> @@ -8,6 +8,22 @@ config BRCMFMAC
>>   	  interface support. If you choose to build a module, it'll be called
>>   	  brcmfmac.ko.
>>
>> +config BRCMFMAC_VENDOR_MODULES
>> +	bool "Use vendor-specific modules"
>> +	depends on BRCMFMAC = m
>> +	help
>> +	  This option will build separate modules for the vendor-specific
>> +	  firmware support. If not selected the vendor-specific support
>> +	  will be build in brcmfmac.ko.
>> +
>> +config BRCMFMAC_VENDOR_WCC
>> +	bool "Broadcom WCC"
>> +	default y
>> +	depends on BRCMFMAC
>> +        help
>> +          This option will allow the driver to communicate with devices
>> +          shipped by Broadcom WCC division.
>> +
> 
> I'm not really a fan of these Kconfig options, I would rather have them
> always enabled. Why do we need these options, what would be the use case
> when user disables these?

I assume with "always enabled" you mean "drop these options". Obviously 
I would prefer to keep them. The default will result in a single module 
with all vendor support built-in, but this allows people to trim down 
their configuration based on what they have. So the choices are:

1) single module with all vendor support built-in
2) single module with partial vendor support built-in (as needed)

    allows users to select vendor for their specific device not carrying
    stuff they don't need. If they have a Cypress/Infineon device they
    only need support for that.

3) separate vendor support modules loaded as needed during device probe

    build one or more vendor support modules and they are loaded into
    memory only when they are needed for the device detected.

Regards,
Arend