Linux-Wireless Archive on lore.kernel.org
 help / color / Atom feed
From: <Ajay.Kathat@microchip.com>
To: <johannes@sipsolutions.net>, <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>
Subject: Re: [PATCH v2 05/16] wilc1000: add wilc_wlan_cfg.c
Date: Fri, 12 Jul 2019 10:58:52 +0000
Message-ID: <c1b7691a-7504-938c-acb0-1d4f47c1bc75@microchip.com> (raw)
In-Reply-To: <86bc79ccd379497d56bade79ec8f717603110ef7.camel@sipsolutions.net>

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
> 

  reply index

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-12  1:58 [PATCH v2 00/16] wilc1000: move out of staging Ajay.Kathat
2019-07-12  1:58 ` [PATCH v2 01/16] wilc1000: add wilc_hif.h Ajay.Kathat
2019-10-23 10:03   ` Kalle Valo
     [not found]   ` <20191023100312.B1D2760A7E@smtp.codeaurora.org>
2019-10-29  3:06     ` Adham.Abozaeid
2019-07-12  1:58 ` [PATCH v2 02/16] wilc1000: add wilc_hif.c Ajay.Kathat
2019-07-12  7:42   ` Johannes Berg
2019-07-12 14:52     ` Ajay.Kathat
2019-07-12  1:58 ` [PATCH v2 03/16] wilc1000: add wilc_wlan_if.h Ajay.Kathat
2019-07-12  1:58 ` [PATCH v2 04/16] wilc1000: add wilc_wlan_cfg.h Ajay.Kathat
2019-07-12  1:58 ` [PATCH v2 05/16] wilc1000: add wilc_wlan_cfg.c Ajay.Kathat
2019-07-12  7:31   ` Johannes Berg
2019-07-12 10:58     ` Ajay.Kathat [this message]
2019-07-12  1:58 ` [PATCH v2 06/16] wilc1000: add wilc_wfi_netdevice.h Ajay.Kathat
2019-07-12  1:59 ` [PATCH v2 07/16] wilc1000: add wilc_wfi_cfgoperations.h Ajay.Kathat
2019-07-12  1:59 ` [PATCH v2 08/16] wilc1000: add wilc_wfi_cfgoperations.c Ajay.Kathat
2019-07-12  1:59 ` [PATCH v2 09/16] wilc1000: add wilc_netdev.c Ajay.Kathat
2019-07-12  1:59 ` [PATCH v2 10/16] wilc1000: add wilc_mon.c Ajay.Kathat
2019-07-12  1:59 ` [PATCH v2 11/16] wilc1000: add wilc_spi.c Ajay.Kathat
2019-07-12  1:59 ` [PATCH v2 12/16] wilc1000: add wilc_wlan.c Ajay.Kathat
2019-07-12  1:59 ` [PATCH v2 13/16] wilc1000: add wilc_wlan.h Ajay.Kathat
2019-07-12  1:59 ` [PATCH v2 14/16] wilc1000: add wilc_sdio.c Ajay.Kathat
2019-07-12  1:59 ` [PATCH v2 15/16] wilc1000: updated DT device binding for wilc1000 device Ajay.Kathat
2019-07-12  1:59 ` [PATCH v2 16/16] wilc1000: add Makefile and Kconfig files for wilc1000 compilation Ajay.Kathat

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c1b7691a-7504-938c-acb0-1d4f47c1bc75@microchip.com \
    --to=ajay.kathat@microchip.com \
    --cc=Adham.Abozaeid@microchip.com \
    --cc=Claudiu.Beznea@microchip.com \
    --cc=Nicolas.Ferre@microchip.com \
    --cc=Venkateswara.Kaja@microchip.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=johannes@sipsolutions.net \
    --cc=kvalo@codeaurora.org \
    --cc=linux-wireless@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Wireless Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-wireless/0 linux-wireless/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-wireless linux-wireless/ https://lore.kernel.org/linux-wireless \
		linux-wireless@vger.kernel.org
	public-inbox-index linux-wireless

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-wireless


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git