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;
Message-ID: <5460619d-38e3-9960-5bc7-63854bd01b46@quicinc.com>
Date:   Thu, 18 Aug 2022 18:11:52 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH 47/50] wifi: ath12k: add wmi.h
Content-Language: en-US
To:     Kalle Valo <kvalo@kernel.org>, <linux-wireless@vger.kernel.org>
CC:     <ath12k@lists.infradead.org>
References: <20220812161003.27279-1-kvalo@kernel.org>
 <20220812161003.27279-48-kvalo@kernel.org>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20220812161003.27279-48-kvalo@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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/

[...]

> +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,
> +};

[...]

> +#define WMI_CMD_UNSUPPORTED 0

why not just add this to enum wmi_tlv_cmd_id?

> +
> +enum wmi_tlv_cmd_id {
> +	WMI_INIT_CMDID = 0x1,
> +	WMI_START_SCAN_CMDID = WMI_TLV_CMD(WMI_GRP_SCAN),
[...]
> +#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?

> +
> +#define WMI_VDEV_SLOT_TIME_LONG         0x1
> +#define WMI_VDEV_SLOT_TIME_SHORT        0x2

another enum?

> +#define WMI_VDEV_PREAMBLE_LONG          0x1
> +#define WMI_VDEV_PREAMBLE_SHORT         0x2

another enum?
[...]

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

s/Regaulatory/Regulatory/

[...]

> +#define WMI_KEY_PAIRWISE 0x00
> +#define WMI_KEY_GROUP    0x01

create an enum?

> +
> +#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?

[...]