On Mon, 2020-10-19 at 08:47 +0530, Srinivasan Raju wrote:
> This introduces the pureLiFi LiFi driver for LiFi-X, LiFi-XC
> and LiFi-XL USB devices.
Mostly trivial comments:
> diff --git a/drivers/net/wireless/purelifi/chip.c
> b/drivers/net/wireless/purelifi/chip.c
[]
> +int purelifi_chip_set_rate(struct purelifi_chip *chip, u8 rate)
> +{
> + int r;
> + static struct purelifi_chip *chip_p;
Isn't chip_p pointless?
> +
> + if (chip)
> + chip_p = chip;
> + if (!chip_p)
> + return -EINVAL;
> +
chip_p is otherwise unused.
> diff --git a/drivers/net/wireless/purelifi/mac.c
> b/drivers/net/wireless/purelifi/mac.c
[]
> +int purelifi_mac_init_hw(struct ieee80211_hw *hw)
> +{
> + int r;
> + struct purelifi_mac *mac = purelifi_hw_mac(hw);
> + struct purelifi_chip *chip = &mac->chip;
> +
> + r = purelifi_chip_init_hw(chip);
> + if (r) {
> + dev_warn(purelifi_mac_dev(mac), "init hw failed (%d)\n", r);
> + goto out;
> + }
> +
> + dev_dbg(purelifi_mac_dev(mac), "irq_disabled %d\n", irqs_disabled());
> +
> + r = regulatory_hint(hw->wiphy, "CA");
> +out:
> + return r;
> +}
Simpler without the out: label and a direct return
if (r) {
dev_warn(...)
return r;
}
...
return regulator_hint(hw->wiphy, "CA");
}
> +static int download_fpga(struct usb_interface *intf)
> +{
[]
> + if ((le16_to_cpu(udev->descriptor.idVendor) ==
> + PURELIFI_X_VENDOR_ID_0) &&
> + (le16_to_cpu(udev->descriptor.idProduct) ==
> + PURELIFI_X_PRODUCT_ID_0)) {
> + fw_name = "purelifi/li_fi_x/fpga.bit";
> + dev_info(&intf->dev, "bit file for X selected.\n");
> +
> + } else if ((le16_to_cpu(udev->descriptor.idVendor)) ==
> + PURELIFI_XC_VENDOR_ID_0 &&
> + (le16_to_cpu(udev->descriptor.idProduct) ==
> + PURELIFI_XC_PRODUCT_ID_0)) {
> + fw_name = "purelifi/li_fi_x/fpga_xc.bit";
> + dev_info(&intf->dev, "bit filefor XC selected.\n");
Inconsistent dev_info spacing: "file for" vs "filefor"
> + 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);
> +
> + for (tbuf_idx = 0; tbuf_idx < blk_tran_len; tbuf_idx++) {
> + fw_data[tbuf_idx] =
> + ((fw_data[tbuf_idx] & 128) >> 7) |
> + ((fw_data[tbuf_idx] & 64) >> 5) |
> + ((fw_data[tbuf_idx] & 32) >> 3) |
> + ((fw_data[tbuf_idx] & 16) >> 1) |
> + ((fw_data[tbuf_idx] & 8) << 1) |
> + ((fw_data[tbuf_idx] & 4) << 3) |
> + ((fw_data[tbuf_idx] & 2) << 5) |
> + ((fw_data[tbuf_idx] & 1) << 7);
> + }
pity there isn't an u8_bit_reverse function/mechanism
> +static void pretty_print_mac(struct usb_interface *intf, char *serial_number,
> + unsigned char *hw_address
> + )
> +{
> + unsigned char i;
> +
> + for (i = 0; i < ETH_ALEN; i++)
> + dev_info(&intf->dev, "hw_address[%d]=%x\n", i, hw_address[i]);
I don't think this prettier than %pM
> +}
> +
> +static int upload_mac_and_serial_number(struct usb_interface *intf,
> + unsigned char *hw_address,
> + unsigned char *serial_number)
> +{
[]
> + do {
> + unsigned char buf[8];
> +
> + msleep(200);
> +
> + send_vendor_request(udev, 0x40, buf, sizeof(buf));
> + flash.enabled = buf[0] & 0xFF;
> +
> + if (flash.enabled) {
> + flash.sectors = ((buf[6] & 255) << 24) |
buf is unsigned char[], rather pointless using & 255
> diff --git a/drivers/net/wireless/purelifi/usb.h
> b/drivers/net/wireless/purelifi/usb.h
[]
> +struct station_t {
> + // 7...3 | 2 | 1 | 0 |
> + // Reserved | Hearbeat | FIFO full | Connected |
heartbeat
