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
