Re: [PATCH] [v7] wireless: Initial driver submission for pureLiFi STA devices

Joe Perches
2020-11-16T20:45:41Z

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...