On 6/19/2024 11:05 PM, Kalle Valo wrote:
> Ramya Gnanasekar <quic_rgnanase@xxxxxxxxxxx> writes:
>
>> From: Lingbo Kong <quic_lingbok@xxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxx>
>> Signed-off-by: Ramya Gnanasekar <quic_rgnanase@xxxxxxxxxxx>
>
> [...]
>
>> +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.
