Re: [PATCH] [v11] wireless: Initial driver submission for pureLiFi STA devices
> I haven't had time to do a throrough review yet, but I suggest fixing > the stuff I commented and submitting v12. I'll then do a new review with v12. Hi Kalle, We have submitted v12 of the driver for review. Thanks Srini
Re: [PATCH] [v11] wireless: Initial driver submission for pureLiFi STA devices
Srinivasan Raju writes: >> I see lots of magic numbers in the driver like 2, 0x33 and 0x34 here. >> Please convert the magic numbers to proper defines explaining the >> meaning. And for vendor commands you could even use enum to group >> them better in .h file somewhere. > > Hi Kalle, > > Thanks for reviewing the driver, We will work on the comments. I haven't had time to do a throrough review yet, but I suggest fixing the stuff I commented and submitting v12. I'll then do a new review with v12. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
RE: [PATCH] [v11] wireless: Initial driver submission for pureLiFi STA devices
> I see lots of magic numbers in the driver like 2, 0x33 and 0x34 here. > Please convert the magic numbers to proper defines explaining the meaning. > And for vendor commands you could even use enum to group them better in .h > file somewhere. Hi Kalle, Thanks for reviewing the driver, We will work on the comments. Regards, Srini
Re: [PATCH] [v11] wireless: Initial driver submission for pureLiFi STA devices
Srinivasan Raju writes:
> 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
[...]
> +int download_fpga(struct usb_interface *intf)
> +{
> + int r, actual_length;
> + int fw_data_i, blk_tran_len = 16384;
> + const char *fw_name;
> + unsigned char fpga_setting[2];
> + unsigned char *fpga_dmabuff;
> + unsigned char *fw_data;
> + unsigned char fpga_state[9];
> + struct firmware *fw = NULL;
> + struct usb_device *udev = interface_to_usbdev(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/LiFi-X.bin";
> + dev_info(>dev, "bin 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/LiFi-XC.bin";
> + dev_info(>dev, "bin file for XC selected\n");
> +
> + } else {
> + r = -EINVAL;
> + goto error;
> + }
> +
> + r = request_firmware((const struct firmware **), fw_name,
> + >dev);
> + if (r) {
> + dev_err(>dev, "request_firmware failed (%d)\n", r);
> + goto error;
> + }
> + fpga_dmabuff = NULL;
> + fpga_dmabuff = kmalloc(2, GFP_KERNEL);
> +
> + if (!fpga_dmabuff) {
> + r = -ENOMEM;
> + goto error_free_fw;
> + }
> + send_vendor_request(udev, 0x33, fpga_dmabuff, sizeof(fpga_setting));
> + memcpy(fpga_setting, fpga_dmabuff, 2);
> + kfree(fpga_dmabuff);
> +
> + send_vendor_command(udev, 0x34, NULL, 0);
I see lots of magic numbers in the driver like 2, 0x33 and 0x34 here.
Please convert the magic numbers to proper defines explaining the
meaning. And for vendor commands you could even use enum to group them
better in .h file somewhere.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] [v11] wireless: Initial driver submission for pureLiFi STA devices
Srinivasan Raju writes: > 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. Is endianess support is properly implemented? > + 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); Is this cpu_to_le16() or what? Try avoid reinventing the wheel and use what kernel provides you. Also noticed lots of dev_info() spamming, please convert those to debug messages. And rx_usb_enabled is racy and it will not work if there are multiple devices. Maybe move it to struct purelifi_usb or similar? -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] [v11] wireless: Initial driver submission for pureLiFi STA devices
Srinivasan Raju writes:
> 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
My first quick comments after 10 minutes of looking at this driver, so
not complete in any way:
Does not compile:
ERROR: modpost: "upload_mac_and_serial"
[drivers/net/wireless/purelifi/purelifi.ko] undefined!
> MAINTAINERS |5 +
> drivers/net/wireless/Kconfig |1 +
> drivers/net/wireless/Makefile|1 +
> drivers/net/wireless/purelifi/Kconfig| 27 +
> drivers/net/wireless/purelifi/Makefile |3 +
> drivers/net/wireless/purelifi/chip.c | 93 ++
> drivers/net/wireless/purelifi/chip.h | 81 ++
> drivers/net/wireless/purelifi/dbgfs.c| 150 +++
> drivers/net/wireless/purelifi/firmware.c | 384
> drivers/net/wireless/purelifi/intf.h | 38 +
> drivers/net/wireless/purelifi/mac.c | 873 ++
> drivers/net/wireless/purelifi/mac.h | 189
> drivers/net/wireless/purelifi/usb.c | 1075 ++
> drivers/net/wireless/purelifi/usb.h | 199
The directory structure should be:
drivers/net/wireless///
So please come up with a unique name for the driver which describes the
supported hardware somehow. Calling the driver "purelifi" is imho too
generic, what happens if/when there's a second generation hardware and
that needs a completely new driver? Just to give examples I like names
like rtw88 and mt76.
And I would prefer that the driver name is also used as the directory
name for firmware files, easier to find that way.
> --- /dev/null
> +++ b/drivers/net/wireless/purelifi/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_PURELIFI) := purelifi.o
> +purelifi-objs+= chip.o usb.o mac.o firmware.o dbgfs.o
> diff --git a/drivers/net/wireless/purelifi/chip.c
> b/drivers/net/wireless/purelifi/chip.c
> new file mode 100644
> index ..9a7ccd0f98f2
> --- /dev/null
> +++ b/drivers/net/wireless/purelifi/chip.c
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0-only
Copyright missing in all files.
> +#undef LOAD_MAC_AND_SERIAL_FROM_FILE
> +#undef LOAD_MAC_AND_SERIAL_FROM_FLASH
> +#define LOAD_MAC_AND_SERIAL_FROM_EP0
This should be dynamic and not compile time configurable. For example
try file first, next flash and EP0 last, or something like that.
> +const struct device_attribute purelifi_attr_frequency = {
> + .attr = {.name = "frequency", .mode = (0666)},
> + .show = purelifi_show_sysfs,
> + .store = purelifi_store_frequency,
> +};
> +
> +struct device_attribute purelifi_attr_modulation = {
> + .attr = {.name = "modulation", .mode = (0666)},
> + .show = purelifi_show_modulation,
> + .store = purelifi_store_modulation,
> +};
> +
> +const struct proc_ops modulation_fops = {
> + .proc_open = modulation_open,
> + .proc_read = modulation_read,
> + .proc_write = modulation_write
> +};
No procfs or sysfs files in wireless drivers, please. Needs a strong
reason to have an exception for that rule.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] [v11] wireless: Initial driver submission for pureLiFi STA devices
Srinivasan Raju writes: > 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 Please limit how often you send a new version of this driver. It does not speed up the review, quite the opposite actually as you just flood the patchwork (and my inbox). Especially if there are only cosmetic changes there's no point of sending a new version immediately, only send a new version when there are major changes. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
