DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2A88B20685
Subject: Re: [PATCH 5/6] brcmfmac: Set board_type from DMI on x86 based
 machines
To:     Arend van Spriel <arend.vanspriel@broadcom.com>,
        Franky Lin <franky.lin@broadcom.com>,
        Hante Meuleman <hante.meuleman@broadcom.com>,
        Kalle Valo <kvalo@codeaurora.org>,
        Chi-Hsien Lin <chi-hsien.lin@cypress.com>,
        Wright Feng <wright.feng@cypress.com>
Cc:     linux-wireless@vger.kernel.org, brcm80211-dev-list.pdl@broadcom.com
References: <20181009124755.25402-1-hdegoede@redhat.com>
 <20181009124755.25402-5-hdegoede@redhat.com>
 <39789888-ddad-afcb-3abd-104ef544ca26@broadcom.com>
From:   Hans de Goede <hdegoede@redhat.com>
Message-ID: <84ddea24-4e1b-fade-5310-118383453279@redhat.com>
Date:   Mon, 5 Nov 2018 15:42:10 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.8.0
MIME-Version: 1.0
In-Reply-To: <39789888-ddad-afcb-3abd-104ef544ca26@broadcom.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Hi,

On 05-11-18 12:41, Arend van Spriel wrote:
> On 10/9/2018 2:47 PM, Hans de Goede wrote:
>> For x86 based machines, set the board_type used for nvram file selection
>> based on the DMI sys-vendor and product-name strings.
>>
>> Since on some models these strings are too generic, this commit also adds
>> a quirk table overriding the strings for models listed in that table.
>>
>> The board_type setting is used to load the board-specific nvram file with
>> a board-specific name so that we can ship files for each supported board
>> in linux-firmware.
> 
> some comments below....
> 
> Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> ?.../broadcom/brcm80211/brcmfmac/Makefile????? |?? 2 +
>> ?.../broadcom/brcm80211/brcmfmac/common.c????? |?? 3 +-
>> ?.../broadcom/brcm80211/brcmfmac/common.h????? |?? 7 ++
>> ?.../broadcom/brcm80211/brcmfmac/dmi.c???????? | 104 ++++++++++++++++++
>> ?4 files changed, 115 insertions(+), 1 deletion(-)
>> ?create mode 100644 drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile
>> index 1f5a9b948abf..22fd95a736a8 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile
>> @@ -54,3 +54,5 @@ brcmfmac-$(CONFIG_BRCM_TRACING) += \
>> ???????? tracepoint.o
>> ?brcmfmac-$(CONFIG_OF) += \
>> ???????? of.o
>> +brcmfmac-$(CONFIG_DMI) += \
>> +??????? dmi.o
> 
> Assuming OF and DMI are mutual exclusive, right?

Not necessarily ACPI tables can embed of/devicetree data now, so an x86
machine could have of-data, but having both compiled in is not an issue
I would expect only one of them to actually be present (so either of-data
or an entry in the DMI platform-data table).


>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c
>> new file mode 100644
>> index 000000000000..fadc0ec745b8
>> --- /dev/null
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c
> 
> [...]
> 
>> +static const struct dmi_system_id dmi_platform_data[] = {
> 
> maybe call this dmi_platform_quirk as in brcmf_dmi_probe() you call this a "quirk table".

OK, will fix for v3. I will also switch back to the SPDX license header
for v3, since the ISC license text is now in Linus' tree.

Regards,

Hans