Re: [PATCH RFC 2/4] wifi: ath10k: support board-specific firmware overrides

From: Kalle Valo <kvalo@kernel.org>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: Jeff Johnson <quic_jjohnson@quicinc.com>,
	 "David S. Miller" <davem@davemloft.net>,
	 Eric Dumazet <edumazet@google.com>,
	 Jakub Kicinski <kuba@kernel.org>,
	 Paolo Abeni <pabeni@redhat.com>,
	 Rob Herring <robh+dt@kernel.org>,
	 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	 Conor Dooley <conor+dt@kernel.org>,
	Bjorn Andersson <andersson@kernel.org>,
	 Konrad Dybcio <konrad.dybcio@linaro.org>,
	 ath10k@lists.infradead.org, linux-wireless@vger.kernel.org,
	 netdev@vger.kernel.org, devicetree@vger.kernel.org,
	 linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH RFC 2/4] wifi: ath10k: support board-specific firmware overrides
Date: Fri, 01 Mar 2024 10:01:17 +0200	[thread overview]
Message-ID: <87h6hq8joy.fsf@kernel.org> (raw)
In-Reply-To: <20240130-wcn3990-firmware-path-v1-2-826b93202964@linaro.org> (Dmitry Baryshkov's message of "Tue, 30 Jan 2024 18:38:38 +0200")

Dmitry Baryshkov <dmitry.baryshkov@linaro.org> writes:

> Different Qualcomm platforms using WCN3990 WiFI chip use SoC-specific
> firmware versions with different features. For example firmware for
> SDM845 doesn't use single-chan-info-per-channel feature, while firmware
> for QRB2210 / QRB4210 requires that feature. Allow board DT files to
> override the subdir of the fw dir used to lookup the firmware-N.bin file
> decribing corresponding WiFi firmware.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Sorry for the delay, too many drivers... But this looks good to me, few
small comments.

In the commit message it would it would be good to have an example of
the new firmware path. And also mention that board file (board-2.bin)
handling is not affected, at least that's how understood from reading
the code.

> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -942,11 +942,20 @@ static const struct firmware *ath10k_fetch_fw_file(struct ath10k *ar,
>  	if (dir == NULL)
>  		dir = ".";
>  
> +	if (ar->board_name) {
> +		snprintf(filename, sizeof(filename), "%s/%s/%s",
> +			 dir, ar->board_name, file);
> +		ret = firmware_request_nowarn(&fw, filename, ar->dev);
> +		ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot fw request '%s': %d\n",
> +			   filename, ret);
> +		if (!ret)
> +			return fw;
> +	}

So here you test if ar->board_name is NULL.

> --- a/drivers/net/wireless/ath/ath10k/snoc.c
> +++ b/drivers/net/wireless/ath/ath10k/snoc.c
> @@ -1337,6 +1337,9 @@ static void ath10k_snoc_quirks_init(struct ath10k *ar)
>  	struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
>  	struct device *dev = &ar_snoc->dev->dev;
>  
> +	/* ignore errors, default to empty string */
> +	of_property_read_string(dev->of_node, "firmware-name", &ar->board_name);

What do you mean with empty string in this case, "\n" (with length of 1)
or NULL? Should we also test for strlen(0) in ath10k_fetch_fw_file()?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches