Re: [PATCH v2 05/16] wilc1000: add wilc_wlan_cfg.c

[<Ajay.Kathat@microchip.com>]

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@microchip.com 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
>