<Ajay.Kathat@xxxxxxxxxxxxx> wrote:
> From: Ajay Singh <ajay.kathat@xxxxxxxxxxxxx>
>
> Moved '/driver/staging/wilc1000/wilc_hif.h' to
> '/driver/net/wireless/mirochip/wilc1000/wilc_hif.h'.
>
> Signed-off-by: Ajay Singh <ajay.kathat@xxxxxxxxxxxxx>
(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, ®);
+
+ 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@xxxxxxxxxxxxxxx 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?
--
https://patchwork.kernel.org/patch/11040999/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
