Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <86bc79ccd379497d56bade79ec8f717603110ef7.camel@sipsolutions.net>
Subject: Re: [PATCH v2 05/16] wilc1000: add wilc_wlan_cfg.c
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Ajay.Kathat@microchip.com, linux-wireless@vger.kernel.org
Cc:     gregkh@linuxfoundation.org, kvalo@codeaurora.org,
        Adham.Abozaeid@microchip.com, Venkateswara.Kaja@microchip.com,
        Nicolas.Ferre@microchip.com, Claudiu.Beznea@microchip.com
Date:   Fri, 12 Jul 2019 09:31:52 +0200
In-Reply-To: <1562896697-8002-6-git-send-email-ajay.kathat@microchip.com>
References: <1562896697-8002-1-git-send-email-ajay.kathat@microchip.com>
         <1562896697-8002-6-git-send-email-ajay.kathat@microchip.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

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?

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