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

[Johannes Berg]

On Fri, 2019-07-12 at 01:58 +0000, ajay.kat...@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?

> +                             if (w->id == wid) {
> +                                     w->val = get_unaligned_le32(&info[4]);

You use the opposite one here :-)

> +     /*
> +      * The valid types of response messages are
> +      * 'R' (Response),
> +      * 'I' (Information), and
> +      * 'N' (Network Information)
> +      */
> +
> +     switch (msg_type) {
[...]

> +     case 'S':

Hmm. :-)

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

etc?

johannes