[PATCH] wifi: ath12k: Fix pdev id sent to firmware for single phy devices

[Ramya Gnanasekar]

On 6/19/2024 11:05 PM, Kalle Valo wrote:
> Ramya Gnanasekar <quic_rgnanase at quicinc.com> writes:
> 
>> From: Lingbo Kong <quic_lingbok at quicinc.com>
>>
>> Pdev id from mac phy capabilities will be sent as a part of
>> HTT/WMI command to firmware. This causes issue with single pdev
>> devices where firmware does not respond to the WMI/HTT request
>> sent from host.
>>
>> For single pdev devices firmware expects pdev id as 1 for 5 GHz/6 GHz
>> phy and 2 for 2 GHz band. Add wrapper ath12k_mac_get_target_pdev_id()
>> to help fetch right pdev for single pdev devices.
>>
>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>>
>> Signed-off-by: Lingbo Kong <quic_lingbok at quicinc.com>
>> Signed-off-by: Ramya Gnanasekar <quic_rgnanase at quicinc.com>
> 
> [...]
> 
>> +static bool ath12k_mac_band_match(enum nl80211_band band1, enum WMI_HOST_WLAN_BAND band2)
>> +{
>> +	return (((band1 == NL80211_BAND_2GHZ) && (band2 & WMI_HOST_WLAN_2G_CAP)) ||
>> +		(((band1 == NL80211_BAND_5GHZ) || (band1 == NL80211_BAND_6GHZ)) &&
>> +		   (band2 & WMI_HOST_WLAN_5G_CAP)));
>> +}
> 
> This is not really pleasent to read. What about something like this:
> 
> switch (band1) {
> case NL80211_BAND_2GHZ:
>         if (band2 & WMI_HOST_WLAN_2G_CAP)
>                 return true;
> 
>         break;
> case NL80211_BAND_5GHZ:
> case NL80211_BAND_6GHZ:
>         if (band2 & WMI_HOST_WLAN_5G_CAP)
>                 return true;
> 
>         break;
> }
> 
> return false;
> 
> Or any other ideas?

Thanks for the suggestion Kalle. Switch looks aesthetic compared to
conditional checks. I will refactor the code as above for better
readability.
> 
>> +u8 ath12k_mac_get_target_pdev_id(struct ath12k *ar)
>> +{
>> +	struct ath12k_vif *arvif;
>> +	struct ath12k_base *ab = ar->ab;
>> +
>> +	if (!ab->hw_params->single_pdev_only)
>> +		return ar->pdev->pdev_id;
>> +
>> +	arvif = ath12k_mac_get_vif_up(ar);
>> +
>> +	if (arvif)
>> +		return ath12k_mac_get_target_pdev_id_from_vif(arvif);
>> +	else
>> +		return ar->ab->fw_pdev[0].pdev_id;
>> +}
> 
> I find this easier to read:
> 
> arvif = ath12k_mac_get_vif_up(ar);
> if (!arvif)
> 	return ar->ab->fw_pdev[0].pdev_id;
> 
> return ath12k_mac_get_target_pdev_id_from_vif(arvif);
> 
> But I still would prefer to have some a code comment explaining the idea
> behind here, especially why it's safe to use fw_pdev[0].pdev_id directly
> and what scenario that is.
> 

Sure. I will get the necessary info added as comments.
Thanks for the review Kalle.