Re: [PATCH] ath10k: support get/set antenna configurations.

[Yeoh Chun-Yeow <yeohchunyeow@gmail.com>]

Sorry for all the noise. The patch is indeed working.

I have verified that the number of spatial streams are reduced
accordingly if you reduce the tx chainmask.

----
Chun-Yeow

On Wed, May 14, 2014 at 8:28 PM, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Avery Pennarun <apenwarr@gmail.com> writes:
>
>> On Tue, May 13, 2014 at 2:15 PM, Ben Greear <greearb@candelatech.com> wrote:
>>> On 05/13/2014 11:10 AM, Kalle Valo wrote:
>>>>> --- a/drivers/net/wireless/ath/ath10k/wmi.c
>>>>> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
>>>>> @@ -2558,6 +2558,8 @@ static int ath10k_wmi_main_cmd_init(struct ath10k *ar)
>>>>>      config.ast_skid_limit = __cpu_to_le32(TARGET_AST_SKID_LIMIT);
>>>>>      config.tx_chain_mask = __cpu_to_le32(TARGET_TX_CHAIN_MASK);
>>>>>      config.rx_chain_mask = __cpu_to_le32(TARGET_RX_CHAIN_MASK);
>>>>> +    ar->supp_tx_chainmask = TARGET_TX_CHAIN_MASK;
>>>>> +    ar->supp_rx_chainmask = TARGET_RX_CHAIN_MASK;
>>>>>      config.rx_timeout_pri_vo = __cpu_to_le32(TARGET_RX_TIMEOUT_LO_PRI);
>>>>>      config.rx_timeout_pri_vi = __cpu_to_le32(TARGET_RX_TIMEOUT_LO_PRI);
>>>>>      config.rx_timeout_pri_be = __cpu_to_le32(TARGET_RX_TIMEOUT_LO_PRI);
>>>>> @@ -2652,6 +2654,9 @@ static int ath10k_wmi_10x_cmd_init(struct ath10k *ar)
>>>>>      config.ast_skid_limit = __cpu_to_le32(TARGET_10X_AST_SKID_LIMIT);
>>>>>      config.tx_chain_mask = __cpu_to_le32(TARGET_10X_TX_CHAIN_MASK);
>>>>>      config.rx_chain_mask = __cpu_to_le32(TARGET_10X_RX_CHAIN_MASK);
>>>>> +    /* TODO:  Have to deal with 2x2 chips if/when the come out. */
>>>>> +    ar->supp_tx_chainmask = TARGET_10X_TX_CHAIN_MASK;
>>>>> +    ar->supp_rx_chainmask = TARGET_10X_RX_CHAIN_MASK;
>>>>>      config.rx_timeout_pri_vo = __cpu_to_le32(TARGET_10X_RX_TIMEOUT_LO_PRI);
>>>>>      config.rx_timeout_pri_vi = __cpu_to_le32(TARGET_10X_RX_TIMEOUT_LO_PRI);
>>>>>      config.rx_timeout_pri_be = __cpu_to_le32(TARGET_10X_RX_TIMEOUT_LO_PRI);
>>>>
>>>> This initialisation looks out of place as we these variables have
>>>> nothing to do with the actual WMI_INIT_CMDID command. And besides, they
>>>> would get overwritten every time we start the firmware. Is that on
>>>> purpose?
>>>>
>>>> I think we should find more approriate place, for example
>>>> ath10k_mac_register() would be one to look at.
>>>
>>> It doesn't hurt that they are over-written, but probably it
>>> can be done better.
>>
>> As far as I can see, supp_{tx,rx}_chainmask are effectively constants.
>>  So it kind of makes sense to me to initialize them here with a bunch
>> of other constants.  cfg_{tx,rx}_chainmask do not seem to be
>> initialized ever (so I guess they default to zero, and we don't change
>> the antenna mask unless they are set).  I think this looks safe.
>
> The purpose of ath10k_wmi_main_cmd_init() to initiatialise 'struct
> wmi_init_cmd' and send it to the firmware, it should not do anything
> else. Initialisation of the fields in question do not belong to that
> function, that kind of higher level logic should be the responsibility
> of mac.c.
>
> My idea here is that wmi.c is implementening WMI as a protocol, and
> mac.c then just uses wmi.c as protocol abstraction mac.c. The exception
> we have are the WMI event handling and that's just to keep the code
> simple.
>
>>> I'm knee deep in other bugs though...maybe Avery has time to
>>> address this.
>>
>> I'll respin the patch with the other suggested changed.  If you guys
>> think the supp_{tx,rx}_chainmask stuff should be moved elsewhere, let
>> me know where and I can make another one :)
>
> I want to keep the architecture of ath10k clean and that's why I really
> would prefer to have this somewhere else than in wmi.c.
>
> BTW, please also CC linux-wireless when submitting ath10k patches. I
> have documented the process here:
>
> http://wireless.kernel.org/en/users/Drivers/ath10k/sources#Submitting_patches
>
> --
> Kalle Valo
>
> _______________________________________________
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k