diff mbox series

[v2,01/16] wilc1000: add wilc_hif.h

Message ID 1562896697-8002-2-git-send-email-ajay.kathat@microchip.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series wilc1000: move out of staging | expand

Commit Message

Ajay Singh July 12, 2019, 1:58 a.m. UTC
From: Ajay Singh <ajay.kathat@microchip.com>

Moved '/driver/staging/wilc1000/wilc_hif.h' to
'/driver/net/wireless/mirochip/wilc1000/wilc_hif.h'.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/net/wireless/microchip/wilc1000/wilc_hif.h | 235 +++++++++++++++++++++
 1 file changed, 235 insertions(+)
 create mode 100644 drivers/net/wireless/microchip/wilc1000/wilc_hif.h

Comments

Kalle Valo Oct. 23, 2019, 10:03 a.m. UTC | #1
<Ajay.Kathat@microchip.com> wrote:

> From: Ajay Singh <ajay.kathat@microchip.com>
> 
> Moved '/driver/staging/wilc1000/wilc_hif.h' to
> '/driver/net/wireless/mirochip/wilc1000/wilc_hif.h'.
> 
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>

(My patchwork script doesn't support cover letters, yet, so I need to reply to
the first patch)

I was supposed to do a quick 15 minute review, but I got overboard and used
over an hour for this :) But anyway, below are my comments. Mostly looks good
but some work still to do.

+#ifndef HOST_INT_H

Add WILC prefix?

+struct assoc_resp {
+	__le16 capab_info;
+	__le16 status_code;
+	__le16 aid;
+} __packed;

use struct ieee80211_mgmt?

+   	   //extract RSN capabilities

No C++ style comments, please.

+  wid_list[1].val = (s8 *)&auth_type;

A little bit too much of casting, like in this example, for my taste
but I guess it's not that a bit of problem. Didn't investigate why
this particular cast was need, but I think Johannes already commented
how odd this wid_list looks like. And why sometimes it's (s8 *) and
others (u8 *)?

+      case 'S':

Isn't a proper define much more readable and as a bonus it would
document the meaning of this value? For example, it could
WILC_RESPONSE_TYPE_SCAN or whatever it means. Oh, and the same for
'R', 'I' and others.

do {
    ....
} while (1);

I see quite a lot of these unconditonal loops in the driver:

wilc_netdev.c:157:   while (1) {
wilc_wlan.c:532:     } while (1);
wilc_wlan.c:602:     } while (1);
wilc_wlan.c:730:     } while (1);
wilc_wlan.c:753:     } while (1);
wilc_wlan.c:1002:    } while (1);
wilc_wlan.c:1009:    } while (1);
wilc_wlan_cfg.c:151:   	     } while (1);
wilc_wlan_cfg.c:167:	       	     } while (1);
wilc_wlan_cfg.c:183:		       	     } while (1);
wilc_wlan_cfg.c:198:			       	     } while (1);
wilc_wlan_cfg.c:230:				       } while (1);
wilc_wlan_cfg.c:303:				       	 } while (1);
wilc_wlan_cfg.c:315:					   } while
(1);
wilc_wlan_cfg.c:327:		} while (1);
wilc_wlan_cfg.c:346:		  } while (1);

This is not recommended in kernel code because a small bug can cause a
never ending loop in kernel. Put some kind of limit to the loop,
either counter or time based, for example:

count = 0;

do {
    ....
} while (count++ < 100);

Or even some of the while loops could be replaced with for loop, like
the one in wilc_wlan_parse_info_frame().

+   u8 type = (id >> 12) & 0xf;

No magic values, please. My recommendation is to use GENMASK() and
friends, maybe u16_get_bits()?

I also see identical magic values elsewhere, which should be a strong
indication that this needs to be implemented with a proper define.

+int wilc_wlan_cfg_get_wid(u8 *frame, u32 offset, u16 id)
+{
+	u8 *buf;
+
+	if ((offset + 2) >= WILC_MAX_CFG_FRAME_SIZE)
+	   return 0;
+
+	buf = &frame[offset];
+
+	buf[0] = (u8)id;
+	buf[1] = (u8)(id >> 8);

In general a struct is MUCH better than manually playing with bytes
using 'u8 buf[]', but I think Johannes told you already that. Here you
could just have a simple '__le16 id' and you could assign to it with
cpu_to_le16(id), a lot cleaner than what's above.

+		  /*call host interface info parse as well*/

A space after '/*' and before '*/'. Have you run checkpatch? It should
catch these simple style issues? And you can run with --file directly
on the source tree. Not of course all checkpatch warnings need to be
fixed, but obvious ones like this for sure.

+#define GET_PKT_OFFSET(a) (((a) >> 22) & 0x1ff)

GENMASK() & co

+/*Parameters needed for host interface for  remaining on channel*/

checkpatch

+	struct wilc_vif *vif[WILC_NUM_CONCURRENT_IFC];
+	/*protect vif list*/
+	struct mutex vif_mutex;

I would add a newline before the comment, that would make wilc_wfi_netdevice.h
a lot more readable. But that's a style issue and up to you.

+  if (ch_list_attr_idx) {
+     u8 limit = ch_list_attr_idx + 3 + buf[ch_list_attr_idx + 1];
+
+		for (i = ch_list_attr_idx + 3; i < limit; i++) {
+		       if (buf[i] == 0x51) {
+		       	  	     for (j = i + 2; j < ((i + 2) +
buf[i + 1]); j++)
+			buf[j] = sta_ch;
+					break;
+							}
+								}
+								}

No magic values like 0x51, please. And I think this loop needs a
comment what's happening. But I suspect that if you had proper structs
(and not this ugly buf[] stuff) the code would be self-explanatory and
there would be no need for comments.

+  if (op_ch_attr_idx) {
+     buf[op_ch_attr_idx + 6] = 0x51;
+     			 buf[op_ch_attr_idx + 7] = sta_ch;
+			 }

Ditto. And even more of that in wilc_wfi_cfgoperations.c.

+static struct net_device *get_if_handler(struct wilc *wilc, u8
*mac_header)
+{
+	u8 *bssid, *bssid1;
+	int i = 0;
+	struct net_device *ndev = NULL;
+
+	bssid = mac_header + 10;
+	bssid1 = mac_header + 4;

And here a proper struct for mac_header would be so much cleaner.

+static u8 crc7(u8 crc, const u8 *buffer, u32 len)
+{
+	while (len--)
+	      crc = crc7_byte(crc, *buffer++);
+	      return crc;
+}

What's wrong with <linux/crc7.h>? Why reinvent the wheel?

I see so much this u8 buf[] stuff that I'll stop commenting about it
now. But, for example, spi_cmd_complete() would be a lot cleaner with
proper structs and some refactoring (one function per command or
something like that).

+	  if (addr < 0x30) {

Proper defines for magic values, please. This was even used multiple
times.

+	wilc->hif_func->hif_read_reg(wilc, 0xf0, &reg);
+
+	wilc->hif_func->hif_write_reg(wilc, 0xf0, reg & ~BIT(0));
+	wilc->hif_func->hif_write_reg(wilc, 0xfa, 0);

Magic values. I'll also stop commenting about magic values, I think you got
the point already :)

+#ifdef WILC_DISABLE_PMU
+#else
+	reg |= WILC_HAVE_USE_PMU;
+#endif
+
+#ifdef WILC_SLEEP_CLK_SRC_XO
+	reg |= WILC_HAVE_SLEEP_CLK_SRC_XO;
+#elif defined WILC_SLEEP_CLK_SRC_RTC
+      reg |= WILC_HAVE_SLEEP_CLK_SRC_RTC;
+#endif
+
+#ifdef WILC_EXT_PA_INV_TX_RX
+	reg |= WILC_HAVE_EXT_PA_INV_TX_RX;
+#endif
+	reg |= WILC_HAVE_USE_IRQ_AS_HOST_WAKE;
+	reg |= WILC_HAVE_LEGACY_RF_SETTINGS;
+#ifdef XTAL_24
+	reg |= WILC_HAVE_XTAL_24;
+#endif
+#ifdef DISABLE_WILC_UART
+	reg |= WILC_HAVE_DISABLE_WILC_UART;
+#endif

This kind of configuration should happen on runtime, not compile time.
In other words, the same kernel module should work on _all_ hardware
without recompilation.

+	reg = (BIT(0) | BIT(1) | BIT(2) | BIT(3) | BIT(8) | BIT(9) | BIT(26) |
+	       BIT(29) | BIT(30) | BIT(31));

I said I would stop commenting about magic values, but here I really
have to comment about it :)

Device tree bindings were not visible. And CC
devicetree@vger.kernel.org as we need an ack from the DT maintainers. Also
I have understood that they require the bindings in YAML format now,
at least that was the comment I got with ath11k.

Please remove the 'wilc_' prefix from the filenames, the directory
name is already wilc1000 so no need to replicate that. For example, rename
wilc_hif.c to just hif.c.

+config WILC1000_HW_OOB_INTR
+	bool "WILC1000 out of band interrupt"
+	depends on WILC1000_SDIO
+	help
+	  This option enables out-of-band interrupt support for the WILC1000
+	  chipset. This OOB interrupt is intended to provide a faster interrupt
+	  mechanism for SDIO host controllers that don't support SDIO interrupt.
+	  Select this option If the SDIO host controller in your platform
+	  doesn't support SDIO time devision interrupt.

I think this should be a runtime setting (see my comment about other
compile time settings), for example a module parameter. Would that
work?
Adham Abozaeid Oct. 29, 2019, 3:06 a.m. UTC | #2
On 10/23/19 3:03 AM, Kalle Valo wrote:
> <Ajay.Kathat@microchip.com> wrote:
>
>> From: Ajay Singh <ajay.kathat@microchip.com>
>>
>> Moved '/driver/staging/wilc1000/wilc_hif.h' to
>> '/driver/net/wireless/mirochip/wilc1000/wilc_hif.h'.
>>
>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> (My patchwork script doesn't support cover letters, yet, so I need to reply to
> the first patch)
>
> I was supposed to do a quick 15 minute review, but I got overboard and used
> over an hour for this :) But anyway, below are my comments. Mostly looks good
> but some work still to do.
Thanks a lot for the complete and detailed review of the driver.
I am glad to know that state of the driver is *good* and thanks for your acknowledgment. Defiantly, Johannes's review comments helped us to improve the driver.
I will continue to work on your review comments and try to address all of them. Most of the comments are clear but there are few comments which need some more clarification especially related to *wid_List*.
> +#ifndef HOST_INT_H
>
> Add WILC prefix?
Agree. Will add the WILC_ prefix
> +struct assoc_resp {
> +	__le16 capab_info;
> +	__le16 status_code;
> +	__le16 aid;
> +} __packed;
>
> use struct ieee80211_mgmt?
the ieee8022_mgmt struct includes the mac header, but the frame received from the device doesn't have it, hence, we can't use the ieee802211_mgmt struct directly
> +   	   //extract RSN capabilities
>
> No C++ style comments, please.
Agree. Will change to C style.
> +  wid_list[1].val = (s8 *)&auth_type;
>
> A little bit too much of casting, like in this example, for my taste
> but I guess it's not that a bit of problem. Didn't investigate why
> this particular cast was need, but I think Johannes already commented
> how odd this wid_list looks like. And why sometimes it's (s8 *) and
> others (u8 *)?
You are right, different locations using u8* and other s8*. I will check and submit the required changes.

I'm not very clear though on why the current implementation of wid_list is odd, and what's the recommendation to fix it, so I'd appreciate more clarification on that.
The wid_list is an array of struct that holds the wid parameters, which is the configuration data to be sent to the device.
This array of structs is passed to cfg_send_config_pkt, which takes care of formatting a config packet that the device understands, from the wid struct, then sends it to the device. It also takes care of waiting for the response from the device.
> +      case 'S':
>
> Isn't a proper define much more readable and as a bonus it would
> document the meaning of this value? For example, it could
> WILC_RESPONSE_TYPE_SCAN or whatever it means. Oh, and the same for
> 'R', 'I' and others.
Agree. Will change to meaningful macros.
> do {
>     ....
> } while (1);
>
> I see quite a lot of these unconditonal loops in the driver:
>
> wilc_netdev.c:157:   while (1) {
> wilc_wlan.c:532:     } while (1);
> wilc_wlan.c:602:     } while (1);
> wilc_wlan.c:730:     } while (1);
> wilc_wlan.c:753:     } while (1);
> wilc_wlan.c:1002:    } while (1);
> wilc_wlan.c:1009:    } while (1);
> wilc_wlan_cfg.c:151:   	     } while (1);
> wilc_wlan_cfg.c:167:	       	     } while (1);
> wilc_wlan_cfg.c:183:		       	     } while (1);
> wilc_wlan_cfg.c:198:			       	     } while (1);
> wilc_wlan_cfg.c:230:				       } while (1);
> wilc_wlan_cfg.c:303:				       	 } while (1);
> wilc_wlan_cfg.c:315:					   } while
> (1);
> wilc_wlan_cfg.c:327:		} while (1);
> wilc_wlan_cfg.c:346:		  } while (1);
>
> This is not recommended in kernel code because a small bug can cause a
> never ending loop in kernel. Put some kind of limit to the loop,
> either counter or time based, for example:
>
> count = 0;
>
> do {
>     ....
> } while (count++ < 100);
>
> Or even some of the while loops could be replaced with for loop, like
> the one in wilc_wlan_parse_info_frame().
Agree. Will go through the infinite loops and try to limit them.
> +   u8 type = (id >> 12) & 0xf;
>
> No magic values, please. My recommendation is to use GENMASK() and
> friends, maybe u16_get_bits()?
>
> I also see identical magic values elsewhere, which should be a strong
> indication that this needs to be implemented with a proper define.
Agree. Will use macros to define magic values, and GENMASk/u16_get_bits() to parse bit fields
> +int wilc_wlan_cfg_get_wid(u8 *frame, u32 offset, u16 id)
> +{
> +	u8 *buf;
> +
> +	if ((offset + 2) >= WILC_MAX_CFG_FRAME_SIZE)
> +	   return 0;
> +
> +	buf = &frame[offset];
> +
> +	buf[0] = (u8)id;
> +	buf[1] = (u8)(id >> 8);
>
> In general a struct is MUCH better than manually playing with bytes
> using 'u8 buf[]', but I think Johannes told you already that. Here you
> could just have a simple '__le16 id' and you could assign to it with
> cpu_to_le16(id), a lot cleaner than what's above.
Agree. These changes were already submitted to staging after we received Johannes' feedback to use put_unaligned_le16(), so will be included in v3 of the patch
> +		  /*call host interface info parse as well*/
>
> A space after '/*' and before '*/'. Have you run checkpatch? It should
> catch these simple style issues? And you can run with --file directly
> on the source tree. Not of course all checkpatch warnings need to be
> fixed, but obvious ones like this for sure.
Agree. Will add spaces as recommended.
Actually we ran checkpatch, but it didn't catch this warning.
Currently the driver has only one warning in wilc_wfi_cfgoperations.c for a line being over 80 chars. not sure why this wasn't caught by the checkpatch script.
I'm using the parameters below to run the script, so please let me know if we should be using something else.
./scripts/checkpatch.pl --no-tree --fix-inplace -f drivers/staging/wilc1000/*.c
./scripts/checkpatch.pl --no-tree --fix-inplace -f drivers/staging/wilc1000/*.h
> +#define GET_PKT_OFFSET(a) (((a) >> 22) & 0x1ff)
>
> GENMASK() & co
>
> +/*Parameters needed for host interface for  remaining on channel*/
>
> checkpatch
>
> +	struct wilc_vif *vif[WILC_NUM_CONCURRENT_IFC];
> +	/*protect vif list*/
> +	struct mutex vif_mutex;
>
> I would add a newline before the comment, that would make wilc_wfi_netdevice.h
> a lot more readable. But that's a style issue and up to you.
>
> +  if (ch_list_attr_idx) {
> +     u8 limit = ch_list_attr_idx + 3 + buf[ch_list_attr_idx + 1];
> +
> +		for (i = ch_list_attr_idx + 3; i < limit; i++) {
> +		       if (buf[i] == 0x51) {
> +		       	  	     for (j = i + 2; j < ((i + 2) +
> buf[i + 1]); j++)
> +			buf[j] = sta_ch;
> +					break;
> +							}
> +								}
> +								}
>
> No magic values like 0x51, please. And I think this loop needs a
> comment what's happening. But I suspect that if you had proper structs
> (and not this ugly buf[] stuff) the code would be self-explanatory and
> there would be no need for comments.
>
> +  if (op_ch_attr_idx) {
> +     buf[op_ch_attr_idx + 6] = 0x51;
> +     			 buf[op_ch_attr_idx + 7] = sta_ch;
> +			 }
>
> Ditto. And even more of that in wilc_wfi_cfgoperations.c.
>
> +static struct net_device *get_if_handler(struct wilc *wilc, u8
> *mac_header)
> +{
> +	u8 *bssid, *bssid1;
> +	int i = 0;
> +	struct net_device *ndev = NULL;
> +
> +	bssid = mac_header + 10;
> +	bssid1 = mac_header + 4;
>
> And here a proper struct for mac_header would be so much cleaner.
ok
> +static u8 crc7(u8 crc, const u8 *buffer, u32 len)
> +{
> +	while (len--)
> +	      crc = crc7_byte(crc, *buffer++);
> +	      return crc;
> +}
>
> What's wrong with <linux/crc7.h>? Why reinvent the wheel?
The new implementation of crc7 after commit 1836eea209546b870dd83f3f4ef234d6598a560d uses a different syndrome table than what the WILC SPI uses, and we can't change that since crc7 calculation is in done in the hardware IP.
> I see so much this u8 buf[] stuff that I'll stop commenting about it
> now. But, for example, spi_cmd_complete() would be a lot cleaner with
> proper structs and some refactoring (one function per command or
> something like that).
Sure, will check and refactor spi_cmd_complete().
> +	  if (addr < 0x30) {
>
> Proper defines for magic values, please. This was even used multiple
> times.
>
> +	wilc->hif_func->hif_read_reg(wilc, 0xf0, &reg);
> +
> +	wilc->hif_func->hif_write_reg(wilc, 0xf0, reg & ~BIT(0));
> +	wilc->hif_func->hif_write_reg(wilc, 0xfa, 0);
>
> Magic values. I'll also stop commenting about magic values, I think you got
> the point already :)
Ok.
> +#ifdef WILC_DISABLE_PMU
> +#else
> +	reg |= WILC_HAVE_USE_PMU;
> +#endif
> +
> +#ifdef WILC_SLEEP_CLK_SRC_XO
> +	reg |= WILC_HAVE_SLEEP_CLK_SRC_XO;
> +#elif defined WILC_SLEEP_CLK_SRC_RTC
> +      reg |= WILC_HAVE_SLEEP_CLK_SRC_RTC;
> +#endif
> +
> +#ifdef WILC_EXT_PA_INV_TX_RX
> +	reg |= WILC_HAVE_EXT_PA_INV_TX_RX;
> +#endif
> +	reg |= WILC_HAVE_USE_IRQ_AS_HOST_WAKE;
> +	reg |= WILC_HAVE_LEGACY_RF_SETTINGS;
> +#ifdef XTAL_24
> +	reg |= WILC_HAVE_XTAL_24;
> +#endif
> +#ifdef DISABLE_WILC_UART
> +	reg |= WILC_HAVE_DISABLE_WILC_UART;
> +#endif
>
> This kind of configuration should happen on runtime, not compile time.
> In other words, the same kernel module should work on _all_ hardware
> without recompilation.
>
> +	reg = (BIT(0) | BIT(1) | BIT(2) | BIT(3) | BIT(8) | BIT(9) | BIT(26) |
> +	       BIT(29) | BIT(30) | BIT(31));
>
> I said I would stop commenting about magic values, but here I really
> have to comment about it :)
>
> Device tree bindings were not visible. And CC
> devicetree@vger.kernel.org as we need an ack from the DT maintainers. Also
> I have understood that they require the bindings in YAML format now,
> at least that was the comment I got with ath11k.
I see, the device tree binding file is not shown properly in the patch because patch indicate the changes as file movement from "staging" to "Documentation/devicetree/bindings/net/wireless" folder.
We don't have binding in YAML format yet so I will make the changes and sent them to review to devicetree@vger.kernel.org also.

> Please remove the 'wilc_' prefix from the filenames, the directory
> name is already wilc1000 so no need to replicate that. For example, rename
> wilc_hif.c to just hif.c.
ok.
> +config WILC1000_HW_OOB_INTR
> +	bool "WILC1000 out of band interrupt"
> +	depends on WILC1000_SDIO
> +	help
> +	  This option enables out-of-band interrupt support for the WILC1000
> +	  chipset. This OOB interrupt is intended to provide a faster interrupt
> +	  mechanism for SDIO host controllers that don't support SDIO interrupt.
> +	  Select this option If the SDIO host controller in your platform
> +	  doesn't support SDIO time devision interrupt.
>
> I think this should be a runtime setting (see my comment about other
> compile time settings), for example a module parameter. Would that
> work?
Okay. Will change this to runtime setting using module paramter.
diff mbox series

Patch

diff --git a/drivers/net/wireless/microchip/wilc1000/wilc_hif.h b/drivers/net/wireless/microchip/wilc1000/wilc_hif.h
new file mode 100644
index 0000000..be1d249
--- /dev/null
+++ b/drivers/net/wireless/microchip/wilc1000/wilc_hif.h
@@ -0,0 +1,235 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2012 - 2018 Microchip Technology Inc., and its subsidiaries
+ * All rights reserved.
+ */
+
+#ifndef HOST_INT_H
+#define HOST_INT_H
+#include <linux/ieee80211.h>
+#include "wilc_wlan_if.h"
+
+enum {
+	WILC_IDLE_MODE = 0x0,
+	WILC_AP_MODE = 0x1,
+	WILC_STATION_MODE = 0x2,
+	WILC_GO_MODE = 0x3,
+	WILC_CLIENT_MODE = 0x4
+};
+
+#define WILC_MAX_NUM_STA			9
+#define WILC_MAX_NUM_SCANNED_CH			14
+#define WILC_MAX_NUM_PROBED_SSID		10
+
+#define WILC_TX_MIC_KEY_LEN			8
+#define WILC_RX_MIC_KEY_LEN			8
+
+#define WILC_MAX_NUM_PMKIDS			16
+#define WILC_ADD_STA_LENGTH			40
+#define WILC_NUM_CONCURRENT_IFC			2
+
+enum {
+	WILC_SET_CFG = 0,
+	WILC_GET_CFG
+};
+
+#define WILC_MAX_ASSOC_RESP_FRAME_SIZE   256
+
+struct assoc_resp {
+	__le16 capab_info;
+	__le16 status_code;
+	__le16 aid;
+} __packed;
+
+struct rf_info {
+	u8 link_speed;
+	s8 rssi;
+	u32 tx_cnt;
+	u32 rx_cnt;
+	u32 tx_fail_cnt;
+};
+
+enum host_if_state {
+	HOST_IF_IDLE			= 0,
+	HOST_IF_SCANNING		= 1,
+	HOST_IF_CONNECTING		= 2,
+	HOST_IF_WAITING_CONN_RESP	= 3,
+	HOST_IF_CONNECTED		= 4,
+	HOST_IF_P2P_LISTEN		= 5,
+	HOST_IF_FORCE_32BIT		= 0xFFFFFFFF
+};
+
+struct wilc_pmkid {
+	u8 bssid[ETH_ALEN];
+	u8 pmkid[WLAN_PMKID_LEN];
+} __packed;
+
+struct wilc_pmkid_attr {
+	u8 numpmkid;
+	struct wilc_pmkid pmkidlist[WILC_MAX_NUM_PMKIDS];
+} __packed;
+
+struct cfg_param_attr {
+	u32 flag;
+	u16 short_retry_limit;
+	u16 long_retry_limit;
+	u16 frag_threshold;
+	u16 rts_threshold;
+};
+
+enum cfg_param {
+	WILC_CFG_PARAM_RETRY_SHORT = BIT(0),
+	WILC_CFG_PARAM_RETRY_LONG = BIT(1),
+	WILC_CFG_PARAM_FRAG_THRESHOLD = BIT(2),
+	WILC_CFG_PARAM_RTS_THRESHOLD = BIT(3)
+};
+
+enum scan_event {
+	SCAN_EVENT_NETWORK_FOUND	= 0,
+	SCAN_EVENT_DONE			= 1,
+	SCAN_EVENT_ABORTED		= 2,
+	SCAN_EVENT_FORCE_32BIT		= 0xFFFFFFFF
+};
+
+enum conn_event {
+	CONN_DISCONN_EVENT_CONN_RESP		= 0,
+	CONN_DISCONN_EVENT_DISCONN_NOTIF	= 1,
+	CONN_DISCONN_EVENT_FORCE_32BIT		= 0xFFFFFFFF
+};
+
+enum {
+	WILC_HIF_SDIO = 0,
+	WILC_HIF_SPI = BIT(0)
+};
+
+enum {
+	WILC_MAC_STATUS_INIT = -1,
+	WILC_MAC_STATUS_DISCONNECTED = 0,
+	WILC_MAC_STATUS_CONNECTED = 1
+};
+
+struct wilc_rcvd_net_info {
+	s8 rssi;
+	u8 ch;
+	u16 frame_len;
+	struct ieee80211_mgmt *mgmt;
+};
+
+struct wilc_user_scan_req {
+	void (*scan_result)(enum scan_event evt,
+			    struct wilc_rcvd_net_info *info, void *priv);
+	void *arg;
+	u32 ch_cnt;
+};
+
+struct wilc_conn_info {
+	u8 bssid[ETH_ALEN];
+	u8 security;
+	enum authtype auth_type;
+	u8 ch;
+	u8 *req_ies;
+	size_t req_ies_len;
+	u8 *resp_ies;
+	u16 resp_ies_len;
+	u16 status;
+	void (*conn_result)(enum conn_event evt, u8 status, void *priv_data);
+	void *arg;
+	void *param;
+};
+
+struct wilc_remain_ch {
+	u16 ch;
+	u32 duration;
+	void (*expired)(void *priv, u64 cookie);
+	void *arg;
+	u32 cookie;
+};
+
+struct wilc;
+struct host_if_drv {
+	struct wilc_user_scan_req usr_scan_req;
+	struct wilc_conn_info conn_info;
+	struct wilc_remain_ch remain_on_ch;
+	u64 p2p_timeout;
+
+	enum host_if_state hif_state;
+
+	u8 assoc_bssid[ETH_ALEN];
+
+	struct timer_list scan_timer;
+	struct wilc_vif *scan_timer_vif;
+
+	struct timer_list connect_timer;
+	struct wilc_vif *connect_timer_vif;
+
+	struct timer_list remain_on_ch_timer;
+	struct wilc_vif *remain_on_ch_timer_vif;
+
+	bool ifc_up;
+	u8 assoc_resp[WILC_MAX_ASSOC_RESP_FRAME_SIZE];
+};
+
+struct wilc_vif;
+int wilc_remove_wep_key(struct wilc_vif *vif, u8 index);
+int wilc_set_wep_default_keyid(struct wilc_vif *vif, u8 index);
+int wilc_add_wep_key_bss_sta(struct wilc_vif *vif, const u8 *key, u8 len,
+			     u8 index);
+int wilc_add_wep_key_bss_ap(struct wilc_vif *vif, const u8 *key, u8 len,
+			    u8 index, u8 mode, enum authtype auth_type);
+int wilc_add_ptk(struct wilc_vif *vif, const u8 *ptk, u8 ptk_key_len,
+		 const u8 *mac_addr, const u8 *rx_mic, const u8 *tx_mic,
+		 u8 mode, u8 cipher_mode, u8 index);
+s32 wilc_get_inactive_time(struct wilc_vif *vif, const u8 *mac,
+			   u32 *out_val);
+int wilc_add_rx_gtk(struct wilc_vif *vif, const u8 *rx_gtk, u8 gtk_key_len,
+		    u8 index, u32 key_rsc_len, const u8 *key_rsc,
+		    const u8 *rx_mic, const u8 *tx_mic, u8 mode,
+		    u8 cipher_mode);
+int wilc_set_pmkid_info(struct wilc_vif *vif, struct wilc_pmkid_attr *pmkid);
+int wilc_get_mac_address(struct wilc_vif *vif, u8 *mac_addr);
+int wilc_set_join_req(struct wilc_vif *vif, u8 *bssid, const u8 *ies,
+		      size_t ies_len);
+int wilc_disconnect(struct wilc_vif *vif);
+int wilc_set_mac_chnl_num(struct wilc_vif *vif, u8 channel);
+int wilc_get_rssi(struct wilc_vif *vif, s8 *rssi_level);
+int wilc_scan(struct wilc_vif *vif, u8 scan_source, u8 scan_type,
+	      u8 *ch_freq_list, u8 ch_list_len,
+	      void (*scan_result_fn)(enum scan_event,
+				     struct wilc_rcvd_net_info *, void *),
+	      void *user_arg, struct cfg80211_scan_request *request);
+int wilc_hif_set_cfg(struct wilc_vif *vif,
+		     struct cfg_param_attr *cfg_param);
+int wilc_init(struct net_device *dev, struct host_if_drv **hif_drv_handler);
+int wilc_deinit(struct wilc_vif *vif);
+int wilc_add_beacon(struct wilc_vif *vif, u32 interval, u32 dtim_period,
+		    struct cfg80211_beacon_data *params);
+int wilc_del_beacon(struct wilc_vif *vif);
+int wilc_add_station(struct wilc_vif *vif, const u8 *mac,
+		     struct station_parameters *params);
+int wilc_del_allstation(struct wilc_vif *vif, u8 mac_addr[][ETH_ALEN]);
+int wilc_del_station(struct wilc_vif *vif, const u8 *mac_addr);
+int wilc_edit_station(struct wilc_vif *vif, const u8 *mac,
+		      struct station_parameters *params);
+int wilc_set_power_mgmt(struct wilc_vif *vif, bool enabled, u32 timeout);
+int wilc_setup_multicast_filter(struct wilc_vif *vif, u32 enabled, u32 count,
+				u8 *mc_list);
+int wilc_remain_on_channel(struct wilc_vif *vif, u64 cookie,
+			   u32 duration, u16 chan,
+			   void (*expired)(void *, u64),
+			   void *user_arg);
+int wilc_listen_state_expired(struct wilc_vif *vif, u64 cookie);
+void wilc_frame_register(struct wilc_vif *vif, u16 frame_type, bool reg);
+int wilc_set_wfi_drv_handler(struct wilc_vif *vif, int index, u8 mode,
+			     u8 ifc_id);
+int wilc_set_operation_mode(struct wilc_vif *vif, u32 mode);
+int wilc_get_statistics(struct wilc_vif *vif, struct rf_info *stats);
+void wilc_resolve_disconnect_aberration(struct wilc_vif *vif);
+int wilc_get_vif_idx(struct wilc_vif *vif);
+int wilc_set_tx_power(struct wilc_vif *vif, u8 tx_power);
+int wilc_get_tx_power(struct wilc_vif *vif, u8 *tx_power);
+void wilc_scan_complete_received(struct wilc *wilc, u8 *buffer, u32 length);
+void wilc_network_info_received(struct wilc *wilc, u8 *buffer, u32 length);
+void wilc_gnrl_async_info_received(struct wilc *wilc, u8 *buffer, u32 length);
+void *wilc_parse_join_bss_param(struct cfg80211_bss *bss,
+				struct cfg80211_crypto_settings *crypto);
+#endif