Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Kalle Valo <kvalo@kernel.org>
To:     Jeff Johnson <quic_jjohnson@quicinc.com>
Cc:     <linux-wireless@vger.kernel.org>, <ath12k@lists.infradead.org>
Subject: Re: [PATCH 47/50] wifi: ath12k: add wmi.h
References: <20220812161003.27279-1-kvalo@kernel.org>
        <20220812161003.27279-48-kvalo@kernel.org>
        <5460619d-38e3-9960-5bc7-63854bd01b46@quicinc.com>
Date:   Tue, 08 Nov 2022 17:43:27 +0200
In-Reply-To: <5460619d-38e3-9960-5bc7-63854bd01b46@quicinc.com> (Jeff
        Johnson's message of "Thu, 18 Aug 2022 18:11:52 -0700")
Message-ID: <87h6z9cuzk.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Jeff Johnson <quic_jjohnson@quicinc.com> writes:

> On 8/12/2022 9:10 AM, Kalle Valo wrote:
>> From: Kalle Valo <quic_kvalo@quicinc.com>
>>
>> (Patches split into one patch per file for easier review, but the final
>> commit will be one big patch. See the cover letter for more info.)
>>
>> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
>> ---
>>   drivers/net/wireless/ath/ath12k/wmi.h | 4796 +++++++++++++++++++++++++++++++++
>>   1 file changed, 4796 insertions(+)
>>
>> diff --git a/drivers/net/wireless/ath/ath12k/wmi.h
>> b/drivers/net/wireless/ath/ath12k/wmi.h
> [...]
>> +/* defines to set Packet extension values whic can be 0 us, 8 usec or 16 usec */
>
> s/whic/which/

Fixed.

>> +enum {
>
> naming the enum allows you to reference the enum in code and comments
>
>> +	WMI_HOST_WLAN_2G_CAP	= 0x1,
>> +	WMI_HOST_WLAN_5G_CAP	= 0x2,
>> +	WMI_HOST_WLAN_2G_5G_CAP	= 0x3,
>> +};

Renamed to enum WMI_HOST_WLAN_BAND.

>> +#define WMI_CMD_UNSUPPORTED 0
>
> why not just add this to enum wmi_tlv_cmd_id?

Added.

>> +#define WMI_PEER_MIMO_PS_STATE                          0x1
>> +#define WMI_PEER_AMPDU                                  0x2
>> +#define WMI_PEER_AUTHORIZE                              0x3
>> +#define WMI_PEER_CHWIDTH                                0x4
>> +#define WMI_PEER_NSS                                    0x5
>> +#define WMI_PEER_USE_4ADDR                              0x6
>> +#define WMI_PEER_MEMBERSHIP                             0x7
>> +#define WMI_PEER_USERPOS                                0x8
>> +#define WMI_PEER_CRIT_PROTO_HINT_ENABLED                0x9
>> +#define WMI_PEER_TX_FAIL_CNT_THR                        0xA
>> +#define WMI_PEER_SET_HW_RETRY_CTS2S                     0xB
>> +#define WMI_PEER_IBSS_ATIM_WINDOW_LENGTH                0xC
>> +#define WMI_PEER_PHYMODE                                0xD
>> +#define WMI_PEER_USE_FIXED_PWR                          0xE
>> +#define WMI_PEER_PARAM_FIXED_RATE                       0xF
>> +#define WMI_PEER_SET_MU_WHITELIST                       0x10
>> +#define WMI_PEER_SET_MAX_TX_RATE                        0x11
>> +#define WMI_PEER_SET_MIN_TX_RATE                        0x12
>> +#define WMI_PEER_SET_DEFAULT_ROUTING                    0x13
>
> perhaps create an enum for these?

Added enum wmi_peer_param.

>> +#define WMI_VDEV_SLOT_TIME_LONG         0x1
>> +#define WMI_VDEV_SLOT_TIME_SHORT        0x2
>
> another enum?

enum wmi_slot_time

>> +#define WMI_VDEV_PREAMBLE_LONG          0x1
>> +#define WMI_VDEV_PREAMBLE_SHORT         0x2
>
> another enum?

enum wmi_preamble

>> +/* Regaulatory Rule Flags Passed by FW */
>
> s/Regaulatory/Regulatory/

Fixed.

>> +#define WMI_KEY_PAIRWISE 0x00
>> +#define WMI_KEY_GROUP    0x01
>
> create an enum?

enum wmi_key_type

>> +#define WMI_CIPHER_NONE     0x0 /* clear key */
>> +#define WMI_CIPHER_WEP      0x1
>> +#define WMI_CIPHER_TKIP     0x2
>> +#define WMI_CIPHER_AES_OCB  0x3
>> +#define WMI_CIPHER_AES_CCM  0x4
>> +#define WMI_CIPHER_WAPI     0x5
>> +#define WMI_CIPHER_CKIP     0x6
>> +#define WMI_CIPHER_AES_CMAC 0x7
>> +#define WMI_CIPHER_ANY      0x8
>> +#define WMI_CIPHER_AES_GCM  0x9
>> +#define WMI_CIPHER_AES_GMAC 0xa
>
> create an enum?

enum wmi_cipher_type

Jeff, thank you so much for all the review comments. Very much
appreciated!

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

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