Re: [PATCH] Add PEAK System USB adapters core driver
From: Wolfgang Grandegger <hidden>
Date: 2012-01-11 09:59:43
On 01/11/2012 10:23 AM, Grosjean Stephane wrote:
Le 10/01/2012 16:35, Wolfgang Grandegger a écrit :quoted
On 01/10/2012 04:22 PM, Oliver Hartkopp wrote:quoted
On 10.01.2012 11:17, Wolfgang Grandegger wrote:quoted
quoted
drivers/net/can/usb/Kconfig | 1 + drivers/net/can/usb/Makefile | 1 + drivers/net/can/usb/peak_usb/Kconfig | 19 + drivers/net/can/usb/peak_usb/Makefile | 10 + drivers/net/can/usb/peak_usb/pcan_usb_core.c | 893 ++++++++++++++++++++++++++ drivers/net/can/usb/peak_usb/peak_usb.h | 149 +++++ 6 files changed, 1073 insertions(+), 0 deletions(-) create mode 100644 drivers/net/can/usb/peak_usb/Kconfig create mode 100644 drivers/net/can/usb/peak_usb/Makefile create mode 100644 drivers/net/can/usb/peak_usb/pcan_usb_core.cWhy not naming the file peak_usb.c? You already use "peak_usb" for the header file as function prefix inside!AFAIR the driver built results in peak_usb.ko And the driver contains the pcan_usb.c and pcan_usb_pro.c If it's possible from the build process pcan_usb_core.c should be renamed to peak_usb.c - that's right.What I know from the build process doesn't enable to do that (that is, building module.ko from module.c **and** file.c: obj-$(CONFIG_CAN_PEAK_USB) += peak_usb.o pcan_usb.o pcan_usb_pro.o linux-can-next$ CHK include/linux/version.h CHK include/generated/utsrelease.h CALL scripts/checksyscalls.sh CHK include/generated/compile.h CC [M] drivers/net/can/usb/peak_usb/pcan_usb.o CC [M] drivers/net/can/usb/peak_usb/pcan_usb_pro.o Kernel: arch/x86/boot/bzImage is ready (#3) Building modules, stage 2. MODPOST 3012 modules ERROR: "pcan_usb_pro" [drivers/net/can/usb/peak_usb/peak_usb.ko] undefined! ERROR: "pcan_usb" [drivers/net/can/usb/peak_usb/peak_usb.ko] undefined! ERROR: "peak_usb_set_ts_now" [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined! ERROR: "peak_usb_get_ts_tv" [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined! ERROR: "dump_mem" [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined! ERROR: "peak_usb_init_time_ref" [drivers/net/can/usb/peak_usb/pcan_usb_pro.ko] undefined! ERROR: "peak_usb_set_ts_now" [drivers/net/can/usb/peak_usb/pcan_usb.ko] undefined! ERROR: "peak_usb_update_ts_now" [drivers/net/can/usb/peak_usb/pcan_usb.ko] undefined! ERROR: "peak_usb_get_ts_tv" [drivers/net/can/usb/peak_usb/pcan_usb.ko] undefined! ERROR: "peak_usb_init_time_ref" [drivers/net/can/usb/peak_usb/pcan_usb.ko] undefined! WARNING: modpost: Found 23 section mismatch(es). To see full details build your kernel with: 'make CONFIG_DEBUG_SECTION_MISMATCH=y' make[1]: *** [__modpost] Error 1 make: *** [modules] Error 2 linux-can-next$ ... but I'm not an expert in the mainline kernel. Is there another way to do that?quoted
We should remove the device specific Kconfigs including the related #ifdefs. It's then *one* driver which always supports the two USB devices. Anything else does not really make sense. Maybe just for very low end devices where any byte counts.Ok I'll do a driver which supports the two usb adapters without any #ifdef.quoted
Have a look to other USB drivers. They supports tons of devices without any #ifdef.... had a look to driver/net/usb but all of these are single file module drivers, so does not help. Where are the other please?
Search for drivers calling usb_register (they are not in "driver/net/usb"). I was looking in "drivers/media/dvb/dvb-usb/dib0700_devices.c". Anyway, I dislike the way you provide support for the two devices. If you want to separate them, you should register a driver for each using "usb_register" using a common infrastructure. You currently provide one driver for both using a homebrewn interface. Have a look to "drivers/media/dvb/dvb-usb". But I'm not sure if it's worth the effort. What would be the use-case for creating a module for just one device? If you provide a distribution, you need to enable both anyway. Wolfgang.