Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Kalle Valo <kvalo@codeaurora.org>
To: Srinivasan Raju <srini.raju@purelifi.com>
Cc: mostafa.afgani@purelifi.com,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
	Rob Herring <robh@kernel.org>,
	Lukas Bulwahn <lukas.bulwahn@gmail.com>,
	linux-kernel@vger.kernel.org (open list),
	linux-wireless@vger.kernel.org (open list:NETWORKING DRIVERS
	(WIRELESS)),
	netdev@vger.kernel.org (open list:NETWORKING DRIVERS)
Subject: Re: [PATCH] [v11] wireless: Initial driver submission for pureLiFi STA devices
Date: Sat, 19 Dec 2020 14:51:25 +0200	[thread overview]
Message-ID: <87wnxeq7qq.fsf@codeaurora.org> (raw)
In-Reply-To: <20201208115719.349553-1-srini.raju@purelifi.com> (Srinivasan Raju's message of "Tue, 8 Dec 2020 17:27:04 +0530")

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

  parent reply	other threads:[~2020-12-19 12:52 UTC|newest]

Thread overview: 102+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20200924151910.21693-1-srini.raju@purelifi.com>
2020-09-28 10:19 ` [PATCH] [v2] wireless: Initial driver submission for pureLiFi devices Srinivasan Raju
2020-09-28 12:07   ` Joe Perches
2020-09-28 12:53     ` Srinivasan Raju
2020-09-30  5:16   ` Leon Romanovsky
2020-09-30  5:29     ` Srinivasan Raju
2020-09-30  8:01     ` Kalle Valo
2020-09-30  9:55       ` Leon Romanovsky
2020-09-30 10:11         ` Johannes Berg
2020-09-30 10:44           ` Leon Romanovsky
2020-10-16  8:23             ` Kalle Valo
2020-09-30  8:05     ` Kalle Valo
2020-09-30 10:04       ` Leon Romanovsky
2020-10-14  6:19   ` [PATCH] [PATCH] [v3] wireless: Initial driver submission for pureLiFi STA devices Srinivasan Raju
2020-10-15 22:35     ` Joe Perches
2020-10-16  6:36       ` Srinivasan Raju
2020-10-16  6:34   ` [PATCH] [v4] " Srinivasan Raju
2020-10-16  8:58     ` Joe Perches
2020-10-16 10:13       ` Srinivasan Raju
2020-10-19  3:17     ` [PATCH] [v5] " Srinivasan Raju
2020-10-19  4:55       ` Joe Perches
2020-10-19  6:05         ` Srinivasan Raju
2020-10-19  8:38       ` [PATCH] [v6] " Srinivasan Raju
2020-10-19 16:07         ` Krishna Chaitanya
2020-10-19 16:40           ` Srinivasan Raju
2020-10-19 16:54             ` Joe Perches
2020-10-19 17:05               ` Srinivasan Raju
2020-11-16  9:22     ` [PATCH] [v7] " Srinivasan Raju
2020-11-16 20:45       ` Joe Perches
2020-11-18  3:24         ` Srinivasan Raju
2020-11-24 14:44       ` Kalle Valo
     [not found]       ` <20201124144448.4E95EC43460@smtp.codeaurora.org>
2020-11-26  5:01         ` Srinivasan Raju
2020-12-03  4:43           ` Srinivasan Raju
2020-12-03 15:58             ` Kalle Valo
2020-12-03 16:50               ` Srinivasan Raju
2020-12-19 13:15                 ` Kalle Valo
2020-12-03  4:38   ` [PATCH] [v8] " Srinivasan Raju
2020-12-03  5:09   ` [PATCH] [v9] " Srinivasan Raju
2020-12-03  7:53     ` Joe Perches
2020-12-08  5:53   ` [PATCH] [v10] " Srinivasan Raju
2020-12-08 11:57   ` [PATCH] [v11] " Srinivasan Raju
2020-12-08 14:37     ` Kalle Valo
2020-12-19 12:51     ` Kalle Valo [this message]
2020-12-19 13:06     ` Kalle Valo
2020-12-19 13:14     ` Kalle Valo
2020-12-21  5:52       ` Srinivasan Raju
2020-12-21  5:57         ` Kalle Valo
2021-01-15 12:13           ` Srinivasan Raju
2021-01-05 13:19   ` [PATCH] [PATCH] [v12] " Srinivasan Raju
2021-02-12 11:49   ` [PATCH] [v13] " Srinivasan Raju
2021-02-12 13:44     ` Johannes Berg
2021-02-17 10:05       ` Kalle Valo
2021-02-19  5:15       ` Srinivasan Raju
2021-02-19  8:25         ` Johannes Berg
2021-02-24 10:41           ` Srinivasan Raju
2021-02-17 10:02     ` Kalle Valo
2021-02-17 10:13       ` Kalle Valo
2021-02-17 10:16         ` Srinivasan Raju
2021-02-17 10:09     ` Kalle Valo
2021-02-17 10:19     ` Kalle Valo
2021-02-24 10:44       ` Srinivasan Raju
2021-02-26 13:07   ` [PATCH] [v14] " Srinivasan Raju
2021-04-19 11:52     ` Srinivasan Raju
2021-08-10 13:02       ` Srinivasan Raju
2021-08-21 13:42         ` Kalle Valo
2021-08-18 14:13     ` [PATCH] [v15] " Srinivasan Raju
2021-09-20 13:05       ` Kalle Valo
     [not found]         ` <CWLP265MB3217BB5AA5F102629A3AD204E0A19@CWLP265MB3217.GBRP265.PROD.OUTLOOK.COM>
2021-09-21 12:30           ` [EXTERNAL] " Kalle Valo
2021-09-22  7:33             ` Johannes Berg
2021-09-24 13:27               ` [EXTERNAL] " Srinivasan Raju
2021-09-20 14:11       ` Kalle Valo
2021-09-24 11:11       ` Kalle Valo
2021-09-24 13:26   ` [PATCH] [v16] wireless: Initial driver submission for pureLiFi LiFi Station Srinivasan Raju
2021-09-24 13:40     ` Kalle Valo
2021-10-05 11:22   ` [PATCH] [v17] wireless: Initial driver submission for pureLiFi STA devices Srinivasan Raju
2021-10-05 11:26     ` Johannes Berg
2021-10-05 12:30   ` [PATCH] [v18 1/2] nl80211: Add LC placeholder band definition to enum nl80211_band Srinivasan Raju
2021-10-05 12:31   ` [PATCH] [v18 2/2] wireless: Initial driver submission for pureLiFi STA devices Srinivasan Raju
2021-10-05 22:09     ` Jeff Johnson
2021-10-06 10:04   ` [PATCH] [v19 " Srinivasan Raju
2021-10-11  6:16     ` Kalle Valo
2021-10-12 12:50   ` [PATCH 0/2] wireless: New Driver " Srinivasan Raju
2021-10-12 12:50     ` [PATCH 1/2] [v19 1/2] nl80211: Add LC placeholder band definition to enum nl80211_band Srinivasan Raju
2021-10-12 12:50     ` [PATCH 2/2] [v19 2/2] wireless: Initial driver submission for pureLiFi STA devices Srinivasan Raju
2021-10-18 10:00   ` [PATCH v20 0/2] wireless: New Driver " Srinivasan Raju
2021-10-18 10:00     ` [PATCH v20 1/2] nl80211: Add LC placeholder band definition to nl80211_band Srinivasan Raju
2021-10-18 10:00     ` [PATCH v20 2/2] wireless: Initial driver submission for pureLiFi STA devices Srinivasan Raju
2021-10-25  9:59       ` Kari Argillander
     [not found]         ` <CWLP265MB321780AB502EF147F6AAF197E0839@CWLP265MB3217.GBRP265.PROD.OUTLOOK.COM>
2021-10-25 12:17           ` [EXTERNAL] " Kalle Valo
2021-10-27 12:38       ` Kari Argillander
2021-10-28  7:24         ` Kalle Valo
2021-10-31 13:10   ` [PATCH v21 0/2] wireless: New Driver " Srinivasan Raju
2021-10-31 13:10     ` [PATCH 1/2] nl80211: Add LC placeholder band definition to nl80211_band Srinivasan Raju
2021-10-31 13:10     ` [PATCH 2/2] wireless: Initial driver submission for pureLiFi STA devices Srinivasan Raju
2021-12-20 19:13       ` Kalle Valo
2022-02-24 15:35       ` Kalle Valo
2022-02-24 18:20   ` [PATCH v22 0/2] wireless: New Driver " Srinivasan Raju
2022-02-24 18:20     ` [PATCH v22 1/2] nl80211: Add LC placeholder band definition to nl80211_band Srinivasan Raju
2022-02-25  9:52       ` Kalle Valo
2022-02-24 18:20     ` [PATCH v22 1/2] wireless: Initial driver submission for pureLiFi STA devices Srinivasan Raju
2022-04-25 13:06       ` Kalle Valo
     [not found]         ` <CWLP265MB32173F6188304F6B2CB90C79E0F89@CWLP265MB3217.GBRP265.PROD.OUTLOOK.COM>
2022-04-26  4:17           ` [EXTERNAL] " Kalle Valo
2022-04-27  4:55       ` Kalle Valo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87wnxeq7qq.fsf@codeaurora.org \
    --to=kvalo@codeaurora.org \
    --cc=davem@davemloft.net \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=lukas.bulwahn@gmail.com \
    --cc=mchehab+huawei@kernel.org \
    --cc=mostafa.afgani@purelifi.com \
    --cc=netdev@vger.kernel.org \
    --cc=robh@kernel.org \
    --cc=srini.raju@purelifi.com \
    --subject='Re: [PATCH] [v11] wireless: Initial driver submission for pureLiFi STA devices' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).