<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