On Mon, Feb 22, 2010 at 08:42:56PM +0100, Wolfgang Grandegger wrote:
> Ira W. Snyder wrote:
> > This patch series adds support for the Janz CMOD-IO carrier board, as well
> > as the Janz VMOD-ICAN3 Intelligent CAN controller. The CMOD-IO carrier
> > board is a PCI to MODULbus bridge, into which plug MODULbus daughterboards.
> > I only have access to two types of daughtercards, the VMOD-ICAN3 mentioned
> > above, and the VMOD-TTL GPIO controller.
> >
> > All of my boards have two VMOD-ICAN3 modules and one VMOD-TTL module. This
> > posting only contains drivers for the CMOD-IO carrier board and VMOD-ICAN3
> > CAN interfaces. A driver for the VMOD-TTL GPIO module is under development,
> > and will be posted shortly. This module is even worse to program nicely
> > than the ICAN3 module.
> >
> > Since the RFCv2 posting, the CAN driver has been much more thoroughly
> > tested. CAN bus-off works correctly, as does the generation of error
> > frames. The bus-off and error frame code has been adapted from the SJA1000
> > driver, as the ICAN3 firmware reports most of the status registers used by
> > the SJA1000 code.
>
> Sounds good and from my point of view the driver is more or less ready
> for mainline inclusion. If that is your primary goal and you feel it is
> mature and stable enough, please send a proper patch series as described
> here:
>
> http://svn.berlios.de/svnroot/repos/socketcan/trunk/README.submitting-patches.
>
> As an alternative, I could apply it to the SVN trunk for the time being.
> There, the requirements for acceptance are not that high.
>
> I briefly browsed the patches. Here some quick comments:
>
> - I do still not find __devinit, __devexit, and friends in your drivers
> as described here:
>
> http://lxr.linux.no/#linux+v2.6.32/Documentation/PCI/pci.txt#L177
>
> They are also missing in janz-ican3.c.
>
> - You may need to declare some structures "__attribute__((packed))",
>
> - Don't include sja1000/sja1000.h. It's only for drivers in sja1000.
> I know that some other drivers use SJA1000 definitions as well, but
> that requires a general solution.
>
Why not? I need some of the definitions for the SJA1000 error registers.
Is there any reason why it can't be include/linux/can/sja1000.h instead?
It seems stupid to duplicate the register definitions in each new driver
that comes along.
> - Some time ago we agreed to use "_" for the Socket-CAN file names:
> s/janz-ican3/janz_ican3/
>
> - You still use many hard-code numbers in the code. Please define
> values for most of them to make the code more readable.
>
I missed a few of these in the version I sent. They'll be fixed for the
next version.
> - There are still to much dev_dbg(). They should especially not be used
> in the xmit and recv path.
>
> - I see still a lot of duplicated code, especially for desc handling.
> Maybe some helper functions or combined i/o functions for send/recv
> could make the code more compact.
>
> - Checkpatch reports "lines too long".
>
> - s+<linux/janz.h>+<linux/mfd/janz.h>+ ?
>
Ok.
> - Check MODULE_LICENSE(). It does not match with your copyright notes.
>
It will be changed to "GPL v2". I didn't know there was a difference
between "GPL" and "GPL v2" until I hunted down include/linux/module.h's
comments. I don't mind GPL v2 or later licensing, but I thought the
Linux kernel was GPL v2 only. I guess not.
> - About xmit flow control. What happens if you send messages quickly at
> 125 KB/s. You could use "cangen -g 0 can0" for that test. How many
> messages get dropped?
>
I let the cangen command run for a while:
$ ifconfig -a
can0 Link encap:UNSPEC HWaddr
00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
UP RUNNING NOARP MTU:16 Metric:1
RX packets:473455 errors:0 dropped:0 overruns:0 frame:0
TX packets:473455 errors:0 dropped:1831983 overruns:0 carrier:0
collisions:0 txqueuelen:10
RX bytes:2719863 (2.5 MiB) TX bytes:2719863 (2.5 MiB)
Interrupt:22
can1 Link encap:UNSPEC HWaddr
00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00
UP RUNNING NOARP MTU:16 Metric:1
RX packets:473455 errors:0 dropped:0 overruns:0 frame:0
TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
collisions:0 txqueuelen:10
RX bytes:2719863 (2.5 MiB) TX bytes:0 (0.0 b)
Interrupt:22
When running cangen, the TX/RX rate is about 32KB/sec (258 kbit/sec) at
roughly 5800 packets/sec. Seems pretty low for the CAN devices
configured like this:
5: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN qlen 10
link/can
can state ERROR-ACTIVE restart-ms 0
bitrate 1000000 sample-point 0.750
tq 125 prop-seg 2 phase-seg1 3 phase-seg2 2 sjw 1
janz-ican3: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
clock 8000000
6: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN qlen 10
link/can
can state ERROR-ACTIVE restart-ms 0
bitrate 1000000 sample-point 0.750
tq 125 prop-seg 2 phase-seg1 3 phase-seg2 2 sjw 1
janz-ican3: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
clock 8000000
Any ideas on how I can go faster? The kernel appears to be spending ~63%
of its CPU time running cangen, and ~37% in softirq context, running
events/0 (the workqueue thread).
Ira
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
