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

[Joe Perches]

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