Hi Johannes, Thanks a lot for again reviewing our driver. On 7/12/2019 1:01 PM, Johannes Berg wrote: > On Fri, 2019-07-12 at 01:58 +0000, Ajay.Kathat@xxxxxxxxxxxxx wrote: >> >> + buf[0] = (u8)id; >> + buf[1] = (u8)(id >> 8); >> + buf[2] = 2; >> + buf[3] = 0; >> + buf[4] = (u8)val16; >> + buf[5] = (u8)(val16 >> 8); > > There are helpers for that, put_le16_unaligned() or so? > Thanks for pointing out and I will modify the same. >> + if (w->id == wid) { >> + w->val = get_unaligned_le32(&info[4]); > > You use the opposite one here :-) > As I see, the data received from the firmware in *le* format, which is parsed in wilc_wlan_parse_response_frame(). So to extract value in correct byteorder we should use get_unaligned_le32(). right? >> + /* >> + * The valid types of response messages are >> + * 'R' (Response), >> + * 'I' (Information), and >> + * 'N' (Network Information) >> + */ >> + >> + switch (msg_type) { > [...] > >> + case 'S': > > Hmm. :-) > Yes, the comments has not capture about 'S' message type. 'S' type is used to notify scan completed from firmware to host. >> + wl->cfg.str_vals = str_vals; >> + /* store the string cfg parameters */ >> + wl->cfg.s[i].id = WID_FIRMWARE_VERSION; >> + wl->cfg.s[i].str = str_vals->firmware_version; >> + i++; >> + wl->cfg.s[i].id = WID_MAC_ADDR; >> + wl->cfg.s[i].str = str_vals->mac_address; >> + i++; >> + wl->cfg.s[i].id = WID_ASSOC_RES_INFO; >> + wl->cfg.s[i].str = str_vals->assoc_rsp; >> + i++; >> + wl->cfg.s[i].id = WID_NIL; >> + wl->cfg.s[i].str = NULL; > > I really don't understand this style. Why not give it a proper struct > and just say > > wl->cfg.assoc_rsp = str_vals->assoc_rsp; Actually, WID’s are used to send the configuration data from host to firmware. Its generic approach to manage the different size of wid. The different WID’s are categorized based on their data size. Instead of managing different variables for each WID's its kept under the single array. These entries are managed like 'id' and 'value'. struct wilc_cfg { struct wilc_cfg_byte *b; struct wilc_cfg_hword *hw; struct wilc_cfg_word *w; struct wilc_cfg_str *s; struct wilc_cfg_str_vals *str_vals; }; The 1-byte data are managed in ‘b’ array, 2-byte in ‘hw’ array and so on. So whenever the WID is set/get by host, the corresponding value also get updated in this array which can be access later. The response from firmware also contains the WID id which is used to udpate the corresponding value. This categorization helps in search and update the exact WID value *wilc_cfg* struct. Regard Ajay > > etc? > > johannes >