From: Joe Perches <joe@perches.com>
To: Srinivasan Raju <srini.raju@purelifi.com>
Cc: mostafa.afgani@purelifi.com, Kalle Valo <kvalo@codeaurora.org>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
Rob Herring <robh@kernel.org>,
Lukas Bulwahn <lukas.bulwahn@gmail.com>,
open list <linux-kernel@vger.kernel.org>,
"open list:NETWORKING DRIVERS (WIRELESS)"
<linux-wireless@vger.kernel.org>,
"open list:NETWORKING DRIVERS" <netdev@vger.kernel.org>
Subject: Re: [PATCH] [v7] wireless: Initial driver submission for pureLiFi STA devices
Date: Mon, 16 Nov 2020 12:45:18 -0800 [thread overview]
Message-ID: <e246d2d2feed162e2f8f7bf46481dec7b6ce729a.camel@perches.com> (raw)
In-Reply-To: <20201116092253.1302196-1-srini.raju@purelifi.com>
On Mon, 2020-11-16 at 14:52 +0530, Srinivasan Raju wrote:
> This introduces the pureLiFi LiFi driver for LiFi-X, LiFi-XC
> and LiFi-XL USB devices.
>
> This driver implementation has been based on the zd1211rw driver.
>
> Driver is based on 802.11 softMAC Architecture and uses
> native 802.11 for configuration and management.
>
> The driver is compiled and tested in ARM, x86 architectures and
> compiled in powerpc architecture.
>
> Signed-off-by: Srinivasan Raju <srini.raju@purelifi.com>
trivial notes and some style and content defects:
(I stopped reading after awhile)
Commonly this changelog would go below the --- separator line.
>
> Changes v6->v7:
> - Magic numbers removed and used IEEE80211 macors
> - usb.c is split into two files firmware.c and dbgfs.c
> - Other code style and timer function fixes (mod_timer)
> Changes v5->v6:
> - Code style fix patch from Joe Perches
> Changes v4->v5:
> - Code refactoring for clarity and redundnacy removal
> - Fix warnings from kernel test robot
> Changes v3->v4:
> - Code refactoring based on kernel code guidelines
> - Remove multi level macors and use kernel debug macros
> Changes v2->v3:
> - Code style fixes kconfig fix
> Changes v1->v2:
> - v1 was submitted to staging, v2 submitted to wireless-next
> - Code style fixes and copyright statement fix
> ---
> MAINTAINERS | 5 +
[]
> diff --git a/MAINTAINERS b/MAINTAINERS
[]
> @@ -14108,6 +14108,11 @@ T: git git://linuxtv.org/media_tree.git
[]
> +PUREILIFI USB DRIVER
> +M: Srinivasan Raju <srini.raju@purelifi.com>
> +S: Maintained
If you are employed there and are paid to maintain this code the
more common S: marking is "Supported"
> diff --git a/drivers/net/wireless/purelifi/Kconfig b/drivers/net/wireless/purelifi/Kconfig
[]
> +++ b/drivers/net/wireless/purelifi/Kconfig
> @@ -0,0 +1,27 @@
> +# SPDX-License-Identifier: GPL-2.0
For clarity, I think it'd be nicer to use GPL-2.0-only here and elsewhere.
> diff --git a/drivers/net/wireless/purelifi/chip.c b/drivers/net/wireless/purelifi/chip.c
[]
> +int purelifi_set_beacon_interval(struct purelifi_chip *chip, u16 interval,
> + u8 dtim_period, int type)
> +{
> + if (!interval || (chip->beacon_set &&
> + chip->beacon_interval == interval)) {
> + return 0;
> + }
It's ddd that checkpatch isn't complaining about single statement uses
with braces. I would write this like the below, but there isn't really
anything wrong with what you did either.
if (!interval ||
(chip->beacon_set && chip->beacon_interval == interval))
return 0;
> +void purelifi_chip_disable_rxtx(struct purelifi_chip *chip)
> +{
> + u8 value;
> +
> + value = 0;
why not make this:
static const u8 value = 0;
> + usb_write_req((const u8 *)&value, sizeof(value), USB_REQ_RXTX_WR);
so this is doesn't need a cast
usb_write_req(&value, sizeof(value), USB_REQ_RXTX_WR);
> +int purelifi_chip_set_rate(struct purelifi_chip *chip, u8 rate)
> +{
> + int r;
> +
> + if (!chip)
> + return -EINVAL;
> +
> + r = usb_write_req((const u8 *)&rate, sizeof(rate), USB_REQ_RATE_WR);
why is the cast needed here?
> +static inline void purelifi_mc_add_addr(struct purelifi_mc_hash *hash,
> + u8 *addr
> + )
Odd close parenthesis location
> diff --git a/drivers/net/wireless/purelifi/dbgfs.c b/drivers/net/wireless/purelifi/dbgfs.c
[]
> +ssize_t purelifi_store_frequency(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t len)
> +{
[]
> + if (valid) {
> + usr_input[0] = (char)predivider;
> + usr_input[1] = (char)feedback_divider;
> + usr_input[2] = (char)output_div_0;
> + usr_input[3] = (char)output_div_1;
> + usr_input[4] = (char)output_div_2;
> + usr_input[5] = (char)clkout3_phase;
> +
> + dev_err(dev, "options IP_DIV: %d VCO_MULT: %d OP_DIV_PHY: %d",
> + usr_input[0], usr_input[1], usr_input[2]);
> + dev_err(dev, "OP_DIV_CPU: %d OP_DIV_USB: %d CLK3_PH: %d\n",
> + usr_input[3], usr_input[4], usr_input[5]);
why is this dev_err? It's not an error.
Shouldn't this be dev_notice or dev_info?
> +ssize_t purelifi_show_sysfs(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + return 0;
> +}
This doesn't seem useful.
> +ssize_t purelifi_show_modulation(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + return 0;
> +}
This either.
> diff --git a/drivers/net/wireless/purelifi/firmware.c b/drivers/net/wireless/purelifi/firmware.c
[]
> +int download_fpga(struct usb_interface *intf)
> +{
[]
> + for (fw_data_i = 0; fw_data_i < fw->size;) {
> + int tbuf_idx;
> +
> + if ((fw->size - fw_data_i) < blk_tran_len)
> + blk_tran_len = fw->size - fw_data_i;
> +
> + fw_data = kmalloc(blk_tran_len, GFP_KERNEL);
> +
> + memcpy(fw_data, &fw->data[fw_data_i], blk_tran_len);
Unchecked alloc, and this should probably use kmemdup()
> + dev_info(&intf->dev, "fpga_status");
> + for (i = 0; i <= 8; i++)
> + dev_info(&intf->dev, " %x ", fpga_dmabuff[i]);
> + dev_info(&intf->dev, "\n");
pr_cont rather than dev_info for the subsequent dev_info uses
otherwise these are all on separate lines.
Or just a single array print of the results and/or use print_hex_dump.
I'd just use:
dev_info(&intf->dev, "fpga status: %x %x %x %x %x %x %x %x\n",
fpga_dmabuff[0], fpga_dmabuff[1],
fpga_dmabuff[2], fpga_dmabuff[3],
fpga_dmabuff[4], fpga_dmabuff[5],
fpga_dmabuff[6], fpga_dmabuff[7]);
[]
> +int download_xl_firmware(struct usb_interface *intf)
> +{
[]
> + buf = kzalloc(64, GFP_KERNEL);
> + r = -ENOMEM;
> + if (!buf)
> + goto error;
Odd use of setting r before if (!buf) test
> +
> + for (step = 0; step < no_of_files; step++) {
> + buf[0] = step;
> + r = send_vendor_command(udev, 0x82, buf, 64);
> +
> + if (step < no_of_files - 1)
> + size = *(u32 *)&fw_packed->data[4 + ((step + 1) * 4)]
> + - *(u32 *)&fw_packed->data[4 + (step) * 4];
> + else
> + size = tot_size -
> + *(u32 *)&fw_packed->data[4 + (step) * 4];
> +
> + start_addr = *(u32 *)&fw_packed->data[4 + (step * 4)];
> +
> + if ((size % 64) && step < 2)
> + size += 64 - size % 64;
> + nmb_of_control_packets = size / 64;
> +
> + for (i = 0; i < nmb_of_control_packets; i++) {
> + memcpy(buf,
> + &fw_packed->data[start_addr + (i * 64)], 64);
> + r = send_vendor_command(udev, 0x81, buf, 64);
unchecked return
> + }
> + dev_info(&intf->dev, "fw-dw step %d,r=%d size %d\n", step, r, size);
Odd indentation too
> +int upload_mac_and_serial_number(struct usb_interface *intf,
> + unsigned char *hw_address,
> + unsigned char *serial_number)
> +{
> +#ifdef LOAD_MAC_AND_SERIAL_FROM_FILE
> + struct firmware *fw = NULL;
> + struct usb_device *udev = interface_to_usbdev(intf);
> + int r;
> + char *mac_file_name = "purelifi/li_fi_x/mac.ascii";
> +
> + r = request_firmware((const struct firmware **)&fw, mac_file_name,
> + &intf->dev);
> + if (r) {
> + dev_err(&intf->dev, "request_firmware fail (%d)\n", r);
> + goto ERROR;
> + }
> + dev_info(&intf->dev, "fw->data=%s\n", fw->data);
> + r = sscanf(fw->data,
> + "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx",
> + &hw_address[0], &hw_address[1],
> + &hw_address[2], &hw_address[3],
> + &hw_address[4], &hw_address[5]);
> + if (r != 6) {
> + dev_err(&intf->dev, "fw_dw - usage args fail (%d)\n", r);
> + goto ERROR;
> + }
> + release_firmware(fw);
> +
> + char *serial_file_name = "purelifi/li_fi_x/serial.ascii";
Please move this to the start of the block below the #ifdef
It may make more sense to separate this into multiple functions.
> diff --git a/drivers/net/wireless/purelifi/mac.c b/drivers/net/wireless/purelifi/mac.c
[]
> +static struct plf_reg_alpha2_map reg_alpha2_map[] = {
static const?
> +static int filter_ack(struct ieee80211_hw *hw, struct ieee80211_hdr *rx_hdr,
> + struct ieee80211_rx_status *stats)
> +{
> + struct purelifi_mac *mac = purelifi_hw_mac(hw);
> + struct sk_buff *skb;
> + struct sk_buff_head *q;
> + unsigned long flags;
> + int found = 0;
bool ?
I stopped reading here...
next prev parent reply other threads:[~2020-11-16 20:45 UTC|newest]
Thread overview: 118+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-24 15:18 [PATCH] staging: Initial driver submission for pureLiFi devices Srinivasan Raju
2020-09-24 15:36 ` Greg Kroah-Hartman
2020-09-24 17:24 ` Srinivasan Raju
2020-09-24 17:29 ` Greg Kroah-Hartman
2020-09-28 10:25 ` Srinivasan Raju
2020-09-24 15:37 ` Greg Kroah-Hartman
2020-09-24 18:28 ` Randy Dunlap
2020-09-28 10:27 ` Srinivasan Raju
2020-09-24 19:07 ` Dan Carpenter
2020-09-28 10:26 ` Srinivasan Raju
2020-09-28 10:19 ` [PATCH] [v2] wireless: " Srinivasan Raju
2020-09-28 12:07 ` Joe Perches
2020-09-28 12:53 ` Srinivasan Raju
2020-09-30 5:16 ` Leon Romanovsky
2020-09-30 5:29 ` Srinivasan Raju
2020-09-30 8:01 ` Kalle Valo
2020-09-30 9:55 ` Leon Romanovsky
2020-09-30 10:11 ` Johannes Berg
2020-09-30 10:44 ` Leon Romanovsky
2020-10-16 8:23 ` Kalle Valo
2020-09-30 8:05 ` Kalle Valo
2020-09-30 10:04 ` Leon Romanovsky
2020-10-14 6:19 ` [PATCH] [PATCH] [v3] wireless: Initial driver submission for pureLiFi STA devices Srinivasan Raju
2020-10-14 10:17 ` kernel test robot
2020-10-15 22:35 ` Joe Perches
2020-10-16 6:36 ` Srinivasan Raju
2020-10-16 6:34 ` [PATCH] [v4] " Srinivasan Raju
2020-10-16 8:58 ` Joe Perches
2020-10-16 10:13 ` Srinivasan Raju
2020-10-19 3:17 ` [PATCH] [v5] " Srinivasan Raju
2020-10-19 4:55 ` Joe Perches
2020-10-19 6:05 ` Srinivasan Raju
2020-10-19 8:38 ` [PATCH] [v6] " Srinivasan Raju
2020-10-19 16:07 ` Krishna Chaitanya
2020-10-19 16:40 ` Srinivasan Raju
2020-10-19 16:54 ` Joe Perches
2020-10-19 17:05 ` Srinivasan Raju
2020-11-16 9:22 ` [PATCH] [v7] " Srinivasan Raju
2020-11-16 20:45 ` Joe Perches [this message]
2020-11-18 3:24 ` Srinivasan Raju
2020-11-24 14:44 ` Kalle Valo
[not found] ` <20201124144448.4E95EC43460@smtp.codeaurora.org>
2020-11-26 5:01 ` Srinivasan Raju
2020-12-03 4:43 ` Srinivasan Raju
2020-12-03 15:58 ` Kalle Valo
2020-12-03 16:50 ` Srinivasan Raju
2020-12-19 13:15 ` Kalle Valo
2020-12-03 4:38 ` [PATCH] [v8] " Srinivasan Raju
2020-12-03 5:09 ` [PATCH] [v9] " Srinivasan Raju
2020-12-03 7:53 ` Joe Perches
2020-12-08 5:53 ` [PATCH] [v10] " Srinivasan Raju
2020-12-08 11:57 ` [PATCH] [v11] " Srinivasan Raju
2020-12-08 14:37 ` Kalle Valo
2020-12-19 12:51 ` Kalle Valo
2020-12-19 13:06 ` Kalle Valo
2020-12-19 13:14 ` Kalle Valo
2020-12-21 5:52 ` Srinivasan Raju
2020-12-21 5:57 ` Kalle Valo
2021-01-15 12:13 ` Srinivasan Raju
2021-01-05 13:19 ` [PATCH] [PATCH] [v12] " Srinivasan Raju
2021-02-12 11:49 ` [PATCH] [v13] " Srinivasan Raju
2021-02-12 13:44 ` Johannes Berg
2021-02-17 10:05 ` Kalle Valo
2021-02-19 5:15 ` Srinivasan Raju
2021-02-19 8:25 ` Johannes Berg
2021-02-24 10:41 ` Srinivasan Raju
2021-02-12 15:06 ` kernel test robot
2021-02-12 17:57 ` kernel test robot
2021-02-17 10:02 ` Kalle Valo
2021-02-17 10:13 ` Kalle Valo
2021-02-17 10:16 ` Srinivasan Raju
2021-02-17 10:09 ` Kalle Valo
2021-02-17 10:19 ` Kalle Valo
2021-02-24 10:44 ` Srinivasan Raju
2021-02-26 13:07 ` [PATCH] [v14] " Srinivasan Raju
2021-04-19 11:52 ` Srinivasan Raju
2021-08-10 13:02 ` Srinivasan Raju
2021-08-21 13:42 ` Kalle Valo
2021-08-18 14:13 ` [PATCH] [v15] " Srinivasan Raju
2021-09-20 13:05 ` Kalle Valo
[not found] ` <CWLP265MB3217BB5AA5F102629A3AD204E0A19@CWLP265MB3217.GBRP265.PROD.OUTLOOK.COM>
2021-09-21 12:30 ` [EXTERNAL] " Kalle Valo
2021-09-22 7:33 ` Johannes Berg
2021-09-24 13:27 ` [EXTERNAL] " Srinivasan Raju
2021-09-20 14:11 ` Kalle Valo
2021-09-24 11:11 ` Kalle Valo
2021-09-24 13:26 ` [PATCH] [v16] wireless: Initial driver submission for pureLiFi LiFi Station Srinivasan Raju
2021-09-24 13:40 ` Kalle Valo
2021-10-05 11:22 ` [PATCH] [v17] wireless: Initial driver submission for pureLiFi STA devices Srinivasan Raju
2021-10-05 11:26 ` Johannes Berg
2021-10-05 12:30 ` [PATCH] [v18 1/2] nl80211: Add LC placeholder band definition to enum nl80211_band Srinivasan Raju
2021-10-05 12:31 ` [PATCH] [v18 2/2] wireless: Initial driver submission for pureLiFi STA devices Srinivasan Raju
2021-10-05 22:09 ` Jeff Johnson
2021-10-06 10:04 ` [PATCH] [v19 " Srinivasan Raju
2021-10-11 6:16 ` Kalle Valo
2021-10-12 12:50 ` [PATCH 0/2] wireless: New Driver " Srinivasan Raju
2021-10-12 12:50 ` [PATCH 1/2] [v19 1/2] nl80211: Add LC placeholder band definition to enum nl80211_band Srinivasan Raju
2021-10-12 12:50 ` [PATCH 2/2] [v19 2/2] wireless: Initial driver submission for pureLiFi STA devices Srinivasan Raju
2021-10-14 6:03 ` kernel test robot
2021-10-24 17:58 ` kernel test robot
2021-10-18 10:00 ` [PATCH v20 0/2] wireless: New Driver " Srinivasan Raju
2021-10-18 10:00 ` [PATCH v20 1/2] nl80211: Add LC placeholder band definition to nl80211_band Srinivasan Raju
2021-10-18 10:00 ` [PATCH v20 2/2] wireless: Initial driver submission for pureLiFi STA devices Srinivasan Raju
2021-10-25 9:59 ` Kari Argillander
[not found] ` <CWLP265MB321780AB502EF147F6AAF197E0839@CWLP265MB3217.GBRP265.PROD.OUTLOOK.COM>
2021-10-25 12:17 ` [EXTERNAL] " Kalle Valo
2021-10-27 11:34 ` kernel test robot
2021-10-27 12:38 ` Kari Argillander
2021-10-28 7:24 ` Kalle Valo
2021-10-31 13:10 ` [PATCH v21 0/2] wireless: New Driver " Srinivasan Raju
2021-10-31 13:10 ` [PATCH 1/2] nl80211: Add LC placeholder band definition to nl80211_band Srinivasan Raju
2021-10-31 13:10 ` [PATCH 2/2] wireless: Initial driver submission for pureLiFi STA devices Srinivasan Raju
2021-12-20 19:13 ` Kalle Valo
2022-02-24 15:35 ` Kalle Valo
2022-02-24 18:20 ` [PATCH v22 0/2] wireless: New Driver " Srinivasan Raju
2022-02-24 18:20 ` [PATCH v22 1/2] nl80211: Add LC placeholder band definition to nl80211_band Srinivasan Raju
2022-02-25 9:52 ` Kalle Valo
2022-02-24 18:20 ` [PATCH v22 1/2] wireless: Initial driver submission for pureLiFi STA devices Srinivasan Raju
2022-04-25 13:06 ` Kalle Valo
[not found] ` <CWLP265MB32173F6188304F6B2CB90C79E0F89@CWLP265MB3217.GBRP265.PROD.OUTLOOK.COM>
2022-04-26 4:17 ` [EXTERNAL] " Kalle Valo
2022-04-27 4:55 ` Kalle Valo
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=e246d2d2feed162e2f8f7bf46481dec7b6ce729a.camel@perches.com \
--to=joe@perches.com \
--cc=davem@davemloft.net \
--cc=kuba@kernel.org \
--cc=kvalo@codeaurora.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=lukas.bulwahn@gmail.com \
--cc=mchehab+huawei@kernel.org \
--cc=mostafa.afgani@purelifi.com \
--cc=netdev@vger.kernel.org \
--cc=robh@kernel.org \
--cc=srini.raju@purelifi.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).