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

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

[Wolfgang Grandegger]

On 11/29/2011 11:10 AM, Grosjean Stephane wrote:
> Hi Oliver,
> 
>>
>> "The right way" (TM) would be to clone David Millers net-next git tree:
>>
>> http://git.kernel.org/?p=linux/kernel/git/davem/net-next.git;a=summary
> $ git clone
> http://git.kernel.org/?p=linux/kernel/git/davem/net-next.git;a=summary
> Initialized empty Git repository in
> /home/peak/linux/drivers/linux-can/net-next/.git/
> f*atal:
> http://git.kernel.org/?p=linux/kernel/git/davem/net-next.git/info/refs
> not found: did you run git update-server-info on the server?*

Please use:

$ git clone git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next

See also http://git.kernel.org/

Wolfgang.

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 563 bytes --]

On 11/29/2011 11:39 AM, Wolfgang Grandegger wrote:
> Please use:
> 
> $ git clone git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next

or, if you're trapped behind a http-only firewall:

git clone http://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git

Marc
-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Oliver Hartkopp]

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

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

[Oliver Hartkopp]

On 06.12.2011 20:49, Oliver Hartkopp wrote:

>> +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.


I just tried out the suggestion myself ...

As my idea was to link the common functions directly to the module binary this
may double the needed size. Maybe your approach is really better 8-)

But the fact that the select should be omitted is still true.

Better you define a

config CAN_PEAK_USB
	tristate "PEAK System USB adapters"

and then

config CAN_PCAN_USB
	tristate "PEAK PCAN USB adapter"
	depends on CAN_PEAK_USB


config CAN_PCAN_USB_PRO
	tristate "PEAK PCAN USB Pro adapter"
	depends on CAN_PEAK_USB

...

Regards,
Oliver

PEAK CAN USB driver v0.4.0

[Grosjean Stephane]

[-- Attachment #1: Type: text/plain, Size: 1788 bytes --]

Hi Oliver,

Please find a new version of the candev driver for our PEAK USB adapters.
I tried to follow your suggestions, especially those concerning the 
names of the files and the Kconfig file content. However, I kept the 
original Kbuild file since I didn't find (for the moment) another way to 
add one or the other pcan_usb.o/pcan_usb_pro.o file, according to their 
corresponding CONFIG_xxx ...

This version also handles the PCAN-USB Pro adapter as well as the 
timestamps coming from PCAN-USB and PCAN-USB Pro (new from v0.2.0).
There's something which should be fixed too and I wanted to talk about 
that too:

I don't know why for the moment, but sending data through the adapters 
works without any problems, except when data are sent through one or the 
other can channel of the the PCAN-USB Pro: the data are correclty sent 
on the bus but, for each sent frame, the Kernel adds the following text 
to the syslog:

     [98724.131218] ehci_hcd 0000:00:1a.7: dma_pool_free buffer-32, 
f4e93000/34e93000 (bad dma)

Seems to be a warning-like, since nothing else occurs anywhere (data are 
sent, the kernel is/seems always stable...).
That's the main reason why I decided to give you that new version, and 
also to ask you if you had got any idea about the reason of such a log? 
I mean, that "problem" doesnot occur when sending through PCAN-USB 
adapter channel... If you have a look to the sources, the management of 
the tx buffer(s) is done into pcan_usb_core.c, that is, tx buffers 
allocation/free doesn't depend on the type of USB adapter (well I 
suppose it is, but why and where?).

So, while I'm delving into the code, I hope you'll be able to have 
enough time to test'n play with your PEAK USB CAN adapters and this new 
driver.

Many thanks and regards,

Stéphane


[-- Attachment #2: peak_usb-0.4.0.tar.gz --]
[-- Type: application/gzip, Size: 45725 bytes --]

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

[Sam Ravnborg]

Hi Oliver.

One not to your comments..

> 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

Today it is really recommended to do:

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

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

Both solutions will work - but the latter allows for some nice
"kbuild" assignments to conditional stuff.

    Sam

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

[Oliver Hartkopp]

On 06.12.2011 21:47, Sam Ravnborg wrote:

> Hi Oliver.
> 
> One not to your comments..
> 
>> 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
> 
> Today it is really recommended to do:
> 
> obj-$(CONFIG_CAN_PEAK_USB) += peak_usb.o
> peak_usb-y := pcan_usb_core.o pcan_usb.o
> 
> obj-$(CONFIG_CAN_PEAK_USB_PRO) += peak_usb_pro.o
> peak_usb_pro-y := pcan_usb_core.o pcan_usb_pro.o
> 
> Both solutions will work - but the latter allows for some nice
> "kbuild" assignments to conditional stuff.
> 


Ah ok,

i picked that from here:

http://patchwork.ozlabs.org/patch/129539/

Good to know that it can be made 'even better' :-)

Thanks,
Oliver

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

[Grosjean Stephane]

Hi,

First of all, thank you for your time and all your comments.

Le 06/12/2011 22:24, Oliver Hartkopp a écrit :
 > On 06.12.2011 21:47, Sam Ravnborg wrote:
 >
 >> Today it is really recommended to do:
 >>
 >> obj-$(CONFIG_CAN_PEAK_USB) += peak_usb.o
 >> peak_usb-y := pcan_usb_core.o pcan_usb.o
 >>
 >> obj-$(CONFIG_CAN_PEAK_USB_PRO) += peak_usb_pro.o
 >> peak_usb_pro-y := pcan_usb_core.o pcan_usb_pro.o
 >>
 >> Both solutions will work - but the latter allows for some nice
 >> "kbuild" assignments to conditional stuff.
 >>

right...

 > I just tried out the suggestion myself ...
 >
 > As my idea was to link the common functions directly to the module 
binary this
 > may double the needed size. Maybe your approach is really better 8-)
 >
 > But the fact that the select should be omitted is still true.
 >
 > Better you define a
 >
 > config CAN_PEAK_USB
 >     tristate "PEAK System USB adapters"
 >
 > and then
 >
 > config CAN_PCAN_USB
 >     tristate "PEAK PCAN USB adapter"
 >     depends on CAN_PEAK_USB
 >
 >
 > config CAN_PCAN_USB_PRO
 >     tristate "PEAK PCAN USB Pro adapter"
 >     depends on CAN_PEAK_USB
 >
 > ...
 >
 > Regards,
 > Oliver

I thought this was better for memory size and also for easier handling 
of (any) further PEAK USB adapter, never know...
But, what next? According to your below Kconfig and Sam's Kbuild, and if 
I understand well your suggestions, the Kbuild file always builds 2 
module files, while I wanted to build only a single one, capable of 
handling one or both adapters.

Please, confirm that a mainline kernel driver should (must?) handle one 
and only one kind of device.
If it should (must), I will modify the Kbuild file as proposed by Sam, 
in order to build a peak_usb.ko and/or a peak_usb_pro.ko file.

But in that case, I wonder why the Kernel API offers some data 
structures like "struct usb_driver.id_table" which are supposed to 
describe more than one device id.

Regards,

Stéphane

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

[Oliver Hartkopp]

On 07.12.2011 11:29, Grosjean Stephane wrote:

> Hi,
> 
> First of all, thank you for your time and all your comments.
> 
> Le 06/12/2011 22:24, Oliver Hartkopp a écrit :
>> On 06.12.2011 21:47, Sam Ravnborg wrote:
>>
>>> Today it is really recommended to do:
>>>
>>> obj-$(CONFIG_CAN_PEAK_USB) += peak_usb.o
>>> peak_usb-y := pcan_usb_core.o pcan_usb.o
>>>
>>> obj-$(CONFIG_CAN_PEAK_USB_PRO) += peak_usb_pro.o
>>> peak_usb_pro-y := pcan_usb_core.o pcan_usb_pro.o
>>>
>>> Both solutions will work - but the latter allows for some nice
>>> "kbuild" assignments to conditional stuff.
>>>
> 
> right...
> 
>> I just tried out the suggestion myself ...
>>
>> As my idea was to link the common functions directly to the module binary this
>> may double the needed size. Maybe your approach is really better 8-)
>>
>> But the fact that the select should be omitted is still true.
>>
>> Better you define a
>>
>> config CAN_PEAK_USB
>>     tristate "PEAK System USB adapters"
>>
>> and then
>>
>> config CAN_PCAN_USB
>>     tristate "PEAK PCAN USB adapter"
>>     depends on CAN_PEAK_USB
>>
>>
>> config CAN_PCAN_USB_PRO
>>     tristate "PEAK PCAN USB Pro adapter"
>>     depends on CAN_PEAK_USB
>>
>> ...
>>
>> Regards,
>> Oliver
> 
> I thought this was better for memory size and also for easier handling of
> (any) further PEAK USB adapter, never know...
> But, what next? According to your below Kconfig and Sam's Kbuild, and if I
> understand well your suggestions, the Kbuild file always builds 2 module
> files, while I wanted to build only a single one, capable of handling one or
> both adapters.


Ah, ok. I formerly thought that you plan to have three separated kernel
modules (core, usb, usbpro) - but i just checked that you only create a
peak_usb.ko which is perfectly fine.

> Please, confirm that a mainline kernel driver should (must?) handle one and

> only one kind of device.


No. If your driver supports "PEAK PCAN USB / USB Pro adapters" you can put
this into a single Kconfig entry and build a single peak_usb.ko if you like.

I don't know whether it's worth the effort to build separate drivers and
kernel modules. The current code has some space for improvements and cleanups :-)

When you're finished your work with the USB Pro it's time to post it on
linux-can ML and the we can discuss together how the sources and the driver
should be packaged best.

Post early & often :-)

> If it should (must), I will modify the Kbuild file as proposed by Sam, in
> order to build a peak_usb.ko and/or a peak_usb_pro.ko file.


This was just a suggestion.

> 
> But in that case, I wonder why the Kernel API offers some data structures like
> "struct usb_driver.id_table" which are supposed to describe more than one
> device id.


Yes you can do that also (as written above).

Regards,
Oliver