Re: [PATCH 1/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver

[Krzysztof Kozlowski]

On Wed, Aug 26, 2020 at 04:59:09PM +0200, Lukasz Stelmach wrote:
> It was <2020-08-25 wto 20:44>, when Krzysztof Kozlowski wrote:
> > On Tue, Aug 25, 2020 at 07:03:09PM +0200, Łukasz Stelmach wrote:
> >> ASIX AX88796[1] is a versatile ethernet adapter chip, that can be
> >> connected to a CPU with a 8/16-bit bus or with an SPI. This driver
> >> supports SPI connection.
> >> 
> >> The driver has been ported from the vendor kernel for ARTIK5[2]
> >> boards. Several changes were made to adapt it to the current kernel
> >> which include:
> >> 
> >> + updated DT configuration,
> >> + clock configuration moved to DT,
> >> + new timer, ethtool and gpio APIs
> >> + dev_* instead of pr_* and custom printk() wrappers.
> >> 
> >> [1] https://protect2.fireeye.com/v1/url?k=074e9e9d-5a9dc212-074f15d2-0cc47a31ce52-0f896a3d08738907&q=1&e=bcaebfa2-4f00-46b6-a35d-096f39710f47&u=https%3A%2F%2Fwww.asix.com.tw%2Fproducts.php%3Fop%3DpItemdetail%26PItemID%3D104%3B65%3B86%26PLine%3D65
> >> [2] https://protect2.fireeye.com/v1/url?k=553869ec-08eb3563-5539e2a3-0cc47a31ce52-fc42424019c6fd8f&q=1&e=bcaebfa2-4f00-46b6-a35d-096f39710f47&u=https%3A%2F%2Fgit.tizen.org%2Fcgit%2Fprofile%2Fcommon%2Fplatform%2Fkernel%2Flinux-3.10-artik%2F
> >> 
> >> The other ax88796 driver is for NE2000 compatible AX88796L chip. These
> >> chips are not compatible. Hence, two separate drivers are required.
> >
> > Hi,
> >
> > Thanks for the driver, nice work. Few comments below.
> >
> 
> Thank you. I fixed most problems and asked some question where I didn't
> understand.
> 
> >> 
> >> Signed-off-by: Łukasz Stelmach <l.stelmach@xxxxxxxxxxx>
> >> ---
> >>  drivers/net/ethernet/Kconfig               |    1 +
> >>  drivers/net/ethernet/Makefile              |    1 +
> >>  drivers/net/ethernet/asix/Kconfig          |   20 +
> >>  drivers/net/ethernet/asix/Makefile         |    6 +
> >>  drivers/net/ethernet/asix/ax88796c_ioctl.c |  293 +++++
> >>  drivers/net/ethernet/asix/ax88796c_ioctl.h |   21 +
> >>  drivers/net/ethernet/asix/ax88796c_main.c  | 1373 ++++++++++++++++++++
> >>  drivers/net/ethernet/asix/ax88796c_main.h  |  596 +++++++++
> >>  drivers/net/ethernet/asix/ax88796c_spi.c   |  103 ++
> >>  drivers/net/ethernet/asix/ax88796c_spi.h   |   67 +
> >>  10 files changed, 2481 insertions(+)
> >>  create mode 100644 drivers/net/ethernet/asix/Kconfig
> >>  create mode 100644 drivers/net/ethernet/asix/Makefile
> >>  create mode 100644 drivers/net/ethernet/asix/ax88796c_ioctl.c
> >>  create mode 100644 drivers/net/ethernet/asix/ax88796c_ioctl.h
> >>  create mode 100644 drivers/net/ethernet/asix/ax88796c_main.c
> >>  create mode 100644 drivers/net/ethernet/asix/ax88796c_main.h
> >>  create mode 100644 drivers/net/ethernet/asix/ax88796c_spi.c
> >>  create mode 100644 drivers/net/ethernet/asix/ax88796c_spi.h
> >> 
> >> diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
> >> index de50e8b9e656..f3b218e45ea5 100644
> >> --- a/drivers/net/ethernet/Kconfig
> >> +++ b/drivers/net/ethernet/Kconfig
> >> @@ -32,6 +32,7 @@ source "drivers/net/ethernet/apm/Kconfig"
> >>  source "drivers/net/ethernet/apple/Kconfig"
> >>  source "drivers/net/ethernet/aquantia/Kconfig"
> >>  source "drivers/net/ethernet/arc/Kconfig"
> >> +source "drivers/net/ethernet/asix/Kconfig"
> >>  source "drivers/net/ethernet/atheros/Kconfig"
> >>  source "drivers/net/ethernet/aurora/Kconfig"
> >>  source "drivers/net/ethernet/broadcom/Kconfig"
> >> diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
> >> index f8f38dcb5f8a..9eb368d93607 100644
> >> --- a/drivers/net/ethernet/Makefile
> >> +++ b/drivers/net/ethernet/Makefile
> >> @@ -18,6 +18,7 @@ obj-$(CONFIG_NET_XGENE) += apm/
> >>  obj-$(CONFIG_NET_VENDOR_APPLE) += apple/
> >>  obj-$(CONFIG_NET_VENDOR_AQUANTIA) += aquantia/
> >>  obj-$(CONFIG_NET_VENDOR_ARC) += arc/
> >> +obj-$(CONFIG_NET_VENDOR_ASIX) += asix/
> >>  obj-$(CONFIG_NET_VENDOR_ATHEROS) += atheros/
> >>  obj-$(CONFIG_NET_VENDOR_AURORA) += aurora/
> >>  obj-$(CONFIG_NET_VENDOR_CADENCE) += cadence/
> >> diff --git a/drivers/net/ethernet/asix/Kconfig b/drivers/net/ethernet/asix/Kconfig
> >> new file mode 100644
> >> index 000000000000..4b127a4a659a
> >> --- /dev/null
> >> +++ b/drivers/net/ethernet/asix/Kconfig
> >> @@ -0,0 +1,20 @@
> >> +#
> >> +# Asix network device configuration
> >> +#
> >> +
> >> +config NET_VENDOR_ASIX
> >> +	bool "Asix devices"
> >> +	depends on SPI
> >> +	help
> >> +	  If you have a network (Ethernet) interface based on a chip from ASIX, say Y
> >
> > Looks like too long, did it pass checkpatch?
> 
> Yes? Let me try again. Yes, this one passed, but I missed a few other
> problems. Thank you.

I noticed that now the limit is 100 when improves readability, so this
one is good.

(...)

> >> +
> >> +u8 ax88796c_check_power(struct ax88796c_device *ax_local)
> >
> > Looks here like pointer to const. Unless it is because of
> > AX_READ_STATUS() which cannot take const?
> 
> It can. I changed other stuff in ax88796c_spi.[hc] to const too.
> 
> >> +{
> >
> > Please put file-scope definitions first, so this should go to the end.
> 
> I don't understand.

Functions and variables which (file scope) are static go to the
beginning of file. Ones visible externally (non static), go after them.

(...)

> >> +
> >> +	AX_WRITE(&ax_local->ax_spi, rx_ctl, P2_RXCR);
> >> +
> >
> > No need for empty line.
> >
> 
> Fixed.
> 
> >> +}
> >> +
> >> +#if 0
> >
> > Please comment why it is commented out.
> >
> 
> Always has been (-; This is how it came from the vendor I missed it when
> I focused on making things work. I will investigate it and either
> uncomment or remove it.

Then just remove it.

(...)

> >> +#include <linux/of.h>
> >> +#endif
> >> +#include <linux/crc32.h>
> >> +#include <linux/etherdevice.h>
> >> +#include <linux/ethtool.h>
> >> +#include <linux/gpio/consumer.h>
> >> +#include <linux/init.h>
> >> +#include <linux/io.h>
> >> +#include <linux/kmod.h>
> >> +#include <linux/mii.h>
> >> +#include <linux/module.h>
> >> +#include <linux/netdevice.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/sched.h>
> >> +#include <linux/spi/spi.h>
> >> +#include <linux/timer.h>
> >> +#include <linux/uaccess.h>
> >> +#include <linux/usb.h>
> >> +#include <linux/version.h>
> >> +#include <linux/workqueue.h>
> >
> > All of these should be removed except the headers used directly in this
> > header.
> >
> 
> This is "private" header file included in all ax88796c_*.c files and
> these are headers required in them. It seems more conveninet to have
> them all listed in one place. What is the reason to do otherwise?

Because:
1. The header is included in other files (more than one) so each other
compilation unit will include all these headers, while not all of them
need. This has a performance penalty during preprocessing.

2. You will loose the track which headers are needed, which are not. We
tend to keep it local, which means each compilation unit includes stuff
it needs. This helps removing obsolete includes later.

3. Otherwise you could make one header, including all headers of Linux,
and then include this one header in each of C files. One to rule them
all.

Best regards,
Krzysztof