Am Donnerstag, den 29.07.2021, 09:16 +0200 schrieb Marc Kleine-Budde: > On 28.07.2021 22:36:47, Stefan Mätje wrote: > > This patch adds support for the PCI based PCIe/402 CAN interface family > > from esd GmbH that is available with various form factors > > (https://esd.eu/en/products/402-series-can-interfaces). > > > > All boards utilize a FPGA based CAN controller solution developed > > by esd (esdACC). For more information on the esdACC see > > https://esd.eu/en/products/esdacc. > > Thanks for the patch! > > > This driver detects all available CAN interface boards but atm. > > operates the CAN-FD capable devices in Classic-CAN mode only! > > Are you planing to change this? Yes, we will provide support for CAN-FD too. I mentioned this already in the cover letter. Should I mention this explicitely in the commit description too? > For now just some nitpicks: > > Compilation throws this error message on 32 bit ARM: > > > drivers/net/can/esd/esd402_pci.c: In function > > ‘pci402_init_dma’: > > > > drivers/net/can/esd/esd402_pci.c:304:32: warning: right shift count >= width of type [-Wshift-count- > > overflow] > > 304 | iowrite32((u32)(card->dma_hnd >> > > 32), > > > > | ^~ > > > > > > CHECK /srv/work/frogger/socketcan/linux/drivers/net/can/esd/esd402_pci.c > > > > drivers/net/can/esd/esd402_pci.c:304:42: warning: shift too big (32) for type unsigned int I'll change this to iowrite32(0U, card->addr_pciep + PCI402_PCIEP_OF_BM_ADDR_HI); which is enough for now because the card->dma_hnd value is limited to a 32-bit address with pci_set_consistent_dma_mask() atm. > > diff --git a/drivers/net/can/esd/Makefile b/drivers/net/can/esd/Makefile > > new file mode 100644 > > index 000000000000..a960e8b97c6f > > --- /dev/null > > +++ b/drivers/net/can/esd/Makefile > > @@ -0,0 +1,11 @@ > > +# SPDX-License-Identifier: GPL-2.0-only > > +# > > +# Makefile for esd gmbh ESDACC controller driver > > +# > > +esd_402_pci-y := esdacc.o esd402_pci.o > > + > > +ifeq ($(CONFIG_CAN_ESD_402_PCI),) > > +obj-m += esd_402_pci.o > > +else > > +obj-$(CONFIG_CAN_ESD_402_PCI) += esd_402_pci.o > > +endif > > Why do you build the driver, if it has not been enabled? I was not aware of that fact and it was not intended. > The straight forward way to build the driver would be: > > > obj-$(CONFIG_CAN_ESD_402_PCI) += esd_402_pci.o > > > > esd_402_pci-objs := esdacc.o esd402_pci.o > > You can rename the esd_402_pci.c to esd_402_pci-core.c to avoid > inconsistent naming, (C file is called esd402_pci.c, while the driver > module is esd_402_pci.ko) > > Marc I will change the Makefile incorporating your recommendations. Best regards, Stefan Mätje System Design Phone: +49-511-37298-146 E-Mail: stefan.maetje@xxxxxx _______________________________________ esd electronics gmbh Vahrenwalder Str. 207 30165 Hannover www.esd.eu Quality Products – Made in Germany _______________________________________ Register Hannover HRB 51373 - VAT-ID DE 115672832 General Manager: Klaus Detering