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

[Kalle Valo]

Srinivasan Raju <srini.raju@xxxxxxxxxxxx> 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 <srini.raju@xxxxxxxxxxxx>

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/<vendor>/<drivername>/<file>

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