Ramon Fried <rfr...@codeaurora.org> writes: > From: Eyal Ilsar <eil...@codeaurora.org> > > 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