Ramon Fried <rfried@xxxxxxxxxxxxxx> writes:
> From: Eyal Ilsar <eilsar@xxxxxxxxxxxxxx>
>
> Introduced infrastructure for supporting FTM WLAN in user space exposing
> the netlink channel from the kernel WLAN driver.
What's FTM WLAN? Don't assume that people know all the acronyms.
This included:
> 1) Registered wcn36xx driver to testmode callback from netlink
> 2) Added testmode callback implementation to handle incoming FTM commands
> 3) Added FTM command packet structure
> 4) Added handling for GET_BUILD_RELEASE_NUMBER (msgid=0x32A2)
> 5) Added generic handling for all PTT_MSG packets
I don't remember the english grammar terminology, but in the commit log
use the form "register", "add" instead of "registered", "added".
> +struct wcn36xx_hal_process_ptt_msg_rsp_msg {
> + struct wcn36xx_hal_msg_header header;
> +
> + /* FTM Command response status */
> + u32 ptt_msg_resp_status;
Unnecessary whitespace after u32.
> +static int wcn36xx_smd_process_ptt_msg_rsp(void *buf, size_t len,
> + void **p_ptt_rsp_msg)
> +{
> + struct wcn36xx_hal_process_ptt_msg_rsp_msg *rsp;
> + int ret = 0;
> +
> + ret = wcn36xx_smd_rsp_status_check(buf, len);
> + if (ret)
> + return ret;
> + rsp = (struct wcn36xx_hal_process_ptt_msg_rsp_msg *)buf;
> + wcn36xx_dbg(WCN36XX_DBG_HAL, "process ptt msg responded with length %d\n",
> + rsp->header.len);
> + wcn36xx_dbg_dump(WCN36XX_DBG_HAL_DUMP, "HAL_PTT_MSG_RSP:", rsp->ptt_msg,
> + rsp->header.len - sizeof(rsp->ptt_msg_resp_status));
> + if (rsp->header.len > 0) {
> + *p_ptt_rsp_msg = kmalloc(rsp->header.len, GFP_ATOMIC);
> + memcpy(*p_ptt_rsp_msg, rsp->ptt_msg, rsp->header.len);
> + }
> + return ret;
> +}
Adding few empty lines to the function would make it more readable.
> @@ -2330,6 +2399,7 @@ int wcn36xx_smd_rsp_process(struct rpmsg_device *rpdev,
> struct ieee80211_hw *hw = priv;
> struct wcn36xx *wcn = hw->priv;
> struct wcn36xx_hal_ind_msg *msg_ind;
> +
> wcn36xx_dbg_dump(WCN36XX_DBG_SMD_DUMP, "SMD <<< ", buf, len);
Unrelated change.
> +int wcn36xx_tm_cmd(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> + void *data, int len)
> +{
> + struct wcn36xx *wcn = hw->priv;
> + struct nlattr *tb[WCN36XX_TM_ATTR_MAX + 1];
> + int ret = 0;
> + unsigned short attr;
> +
> + wcn36xx_dbg_dump(WCN36XX_DBG_TESTMODE_DUMP, "Data:", data, len);
> + ret = nla_parse(tb, WCN36XX_TM_ATTR_MAX, data, len,
> + wcn36xx_tm_policy, NULL);
> + if (ret)
> + return ret;
> +
> + if (!tb[WCN36XX_TM_ATTR_CMD])
> + return -EINVAL;
> +
> + attr = nla_get_u16(tb[WCN36XX_TM_ATTR_CMD]);
> +
> + switch (attr) {
> + case WCN36XX_TM_CMD_START:
> + case WCN36XX_TM_CMD_STOP:
> + // N/A to this driver as it does not need to switch state
> + break;
[...]
> +enum wcn36xx_tm_cmd {
> + /* For backwards compatibility
> + */
> + WCN36XX_TM_CMD_START = 1,
> +
> + /* For backwards compatibility
> + */
> + WCN36XX_TM_CMD_STOP = 2,
This looks odd. If wcn36xx does not need START and STOP commands why add
those in the first place?
--
Kalle Valo
