Re: [PATCH] Add PEAK System USB adapters core driver

[Wolfgang Grandegger <wg@grandegger.com>]

On 01/11/2012 10:23 AM, Grosjean Stephane wrote:
> 
> 
> Le 10/01/2012 16:35, Wolfgang Grandegger a écrit :
>> On 01/10/2012 04:22 PM, Oliver Hartkopp wrote:
>>> On 10.01.2012 11:17, Wolfgang Grandegger wrote:
>>>
>>>>>   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.c
>>>> Why 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?
> 
>> 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.
> 
>> 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.