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@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>
> +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...
