Re: WG: PCAN-USB: new socketCAN driver available

[Oliver Hartkopp <peak@hartkopp.net>]

Hello Stephane,

i checked your PCAN-USB driver - and also got the printk msg when plugging the
USB pro ;-)

Some remarks to the current state and some details for naming conventions.

On 29.11.2011 11:10, Grosjean Stephane wrote:


~/peak_usb-0.2.0$ wc -l *
   10 Kbuild
   27 Kconfig
  113 Makefile
  813 peak_usb_core.c
  109 peak_usb.h
  854 peak_usb_pcan.c
   31 peak_usb_pcan.h
   64 peak_usb_pcan_pro.c
   32 peak_usb_pcan_pro.h
 2053 total


The number of files should be reduced.

The drivers module name should be

peak_usb.ko
peak_usb_pro.ko

'pcan' is a vendor brand name which should not go into the module name. Having
PCAN in the Kconfig (like it is for the peak_pci driver) or in source file
names is fine.

Eg. if you have a shared source like peak_usb_core.c a peak_usb.h header file
is ok - but you should omit peak_usb_pcan[_pro].h , at least they contain
almost nothing ;-)

The files should then reduce to:

Kbuild
Kconfig
Makefile
pcan_usb_core.h
pcan_usb_core.c
pcan_usb.c
pcan_usb_pro.c

Checking the Kconfig:

> diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
> index 0452549..9ec2f9c 100644
> --- a/drivers/net/can/usb/Kconfig
> +++ b/drivers/net/can/usb/Kconfig
> @@ -13,4 +13,21 @@ config CAN_ESD_USB2
>            This driver supports the CAN-USB/2 interface
>            from esd electronic system design gmbh (http://www.esd.eu).
>
> +config CAN_PEAK_PCAN_USB

name it CAN_PEAK_USB

> +       tristate "PEAK-System's PCAN-USB adapter"

"PEAK PCAN USB adapter"

> +       select CAN_PEAK_USB

remove the select (see Makefile comment below)

> +       ---help---
> +         This driver is for the one channel PCAN-USB interface
> +         from PEAK-System (http://www.peak-system.com).

ok.

> +
> +config CAN_PEAK_PCAN_USB_PRO

CAN_PEAK_USB_PRO

> +       tristate "PEAK-System's PCAN-USB-PRO adapter"

"PEAK PCAN USB Pro adapter"

> +       select CAN_PEAK_USB

remove the select (see Makefile comment below)

> +       ---help---
> +         This driver is for the two channels PCAN-USB-PRO interface

PCAN-USB Pro

> +         from PEAK-System (http://www.peak-system.com).
> +
> +config CAN_PEAK_USB
> +       tristate
> +

Remove this.

To select config entries should be omitted if possible.

And now (tada!) the Makefile:

> diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
> index fce3cf1..7cb115b 100644
> --- a/drivers/net/can/usb/Makefile
> +++ b/drivers/net/can/usb/Makefile
> @@ -6,3 +6,14 @@ obj-$(CONFIG_CAN_EMS_USB) += ems_usb.o
>  obj-$(CONFIG_CAN_ESD_USB2) += esd_usb2.o
>
>  ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> +
> +obj-$(CONFIG_CAN_PEAK_USB) += peak_usb.o
> +peak_usb-y = peak_usb_core.o
> +
> +ifneq ($(CONFIG_CAN_PEAK_PCAN_USB),)
> +peak_usb-y += peak_usb_pcan.o
> +endif
> +
> +ifneq ($(CONFIG_CAN_PEAK_PCAN_USB_PRO),)
> +peak_usb-y += peak_usb_pcan_pro.o
> +endif

Ok it worked, but ...

in the linux/drivers/net/can/usb/Makefile it should look like this:

obj-$(CONFIG_CAN_PEAK_USB) += peak_usb.o
peak_usb-objs := pcan_usb_core.o pcan_usb.o

obj-$(CONFIG_CAN_PEAK_USB_PRO) += peak_usb_pro.o
peak_usb_pro-objs := pcan_usb_core.o pcan_usb_pro.o


> 
> Load the module, then "$cat /sys/modules/peak_usb/version" and plug the
> PCAN-USB adapter... Yes you can also try with the PCAN-USB-PRO...


Is having a version string here a common thing?

> Hope you enjoy!


I did. No crash reports :-)

> Any feedback is welcome.


Done.

Best regards,
Oliver