lkml.org 
[lkml]   [2020]   [Dec]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] [v11] wireless: Initial driver submission for pureLiFi STA devices
    Date
    Srinivasan Raju <srini.raju@purelifi.com> 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@purelifi.com>

    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

    \
     
     \ /
      Last update: 2020-12-19 13:53    [W:9.433 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site