This patch adds a driver for the platform:softing device.
This will create (up to) 2 CAN network devices from 1
platform:softing device

Signed-off-by: Kurt Van Dijck <kurt.van.dijck@eia.be>

---
 drivers/net/can/Kconfig                    |    2 +
 drivers/net/can/Makefile                   |    1 +
 drivers/net/can/softing/Kconfig            |   16 +
 drivers/net/can/softing/Makefile           |    5 +
 drivers/net/can/softing/softing.h          |  216 +++++++
 drivers/net/can/softing/softing_fw.c       |  664 ++++++++++++++++++++
 drivers/net/can/softing/softing_main.c     |  935 ++++++++++++++++++++++++++++
 drivers/net/can/softing/softing_platform.h |   38 ++
 8 files changed, 1877 insertions(+), 0 deletions(-)

diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index d5a9db6..986195e 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -117,6 +117,8 @@ source "drivers/net/can/sja1000/Kconfig"
 
 source "drivers/net/can/usb/Kconfig"
 
+source "drivers/net/can/softing/Kconfig"
+
 config CAN_DEBUG_DEVICES
 	bool "CAN devices debugging messages"
 	depends on CAN
diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
index 07ca159..53c82a7 100644
--- a/drivers/net/can/Makefile
+++ b/drivers/net/can/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_CAN_DEV)		+= can-dev.o
 can-dev-y			:= dev.o
 
 obj-y				+= usb/
+obj-y				+= softing/
 
 obj-$(CONFIG_CAN_SJA1000)	+= sja1000/
 obj-$(CONFIG_CAN_MSCAN)		+= mscan/
diff --git a/drivers/net/can/softing/Kconfig b/drivers/net/can/softing/Kconfig
new file mode 100644
index 0000000..072f337
--- /dev/null
+++ b/drivers/net/can/softing/Kconfig
@@ -0,0 +1,16 @@
+config CAN_SOFTING
+	tristate "Softing Gmbh CAN generic support"
+	depends on CAN_DEV
+	---help---
+	  Support for CAN cards from Softing Gmbh & some cards
+	  from Vector Gmbh.
+	  Softing Gmbh CAN cards come with 1 or 2 physical busses.
+	  Those cards typically use Dual Port RAM to communicate
+	  with the host CPU. The interface ...
[ message continues ]

I did some review, but gotta go now, will do more later.


I think it will (at least marginally) speed up the Kernel build process



/*
 * please fix multi-line comments to
 * this style


Please create a header file for these prototypes. The comma "," should



Just curious, why did they put a padding byte here, that makes the rest

Can you renumber the dummy variables (there are some "x" in there), or


An enum can be used here, too, you also can use BIT(x) to define

param[] is an array of s16 and cmd is an int.

hmmm..all stuff behind dpram is __iomem, isn't it? I think it should
only be accessed with via the ioread/iowrite operators. Please check

         ^^

                         ^^


That's not good. I don't remember the name, but there are some

In the worst case this means you lock up the system for one second. Does
the card issue an interrupt if it's finished? Another option is to write
                 ^^


same applies here, too. Although this command seems not to be called

                     ^^^^^^^


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

[ message continues ]

Hello Kurt,


MODULE_DESCRIPTION("Softing CAN driver");

MODULE_DESCRIPTION("Softing DPRAM CAN driver");


Shouldn't this file be placed in include/linux/can/platform/softing.h ??

Regards,
Oliver
--

[ message continues ]

Marc,

A lot of your remarks do make sense, without further comment.
Some however, I'm not completely sure ...



Due to the independant driver design, I should
for 'up', yes, but the lock stays. It protects the startup/shutdown
I did not design the DPRAM layout. It's just the way it is ...
I did an ioremap_nocache. Since it is unaligned, ioread/iowrite would render

Thanks for your review,

Kurt
--

[ message continues ]

In the second patch I see:

+config CAN_SOFTING_CS
+	tristate "Softing CAN pcmcia cards"




The thing is, ioremapped mem should not be accessed directly. Instead
ioread/iowrite should be used. The softing driver should work on non x86

Sparse, a static syntax analyser tool, see "Documentation/sparse.txt".

You should start with fixing the assignment of the ioremapped memory

It's "time_after"

sure, but a *sleep might be better.

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   |

[ message continues ]

I use __attribute__((packed)) structs to refer to the iomemory.
To read an unaligned uint16_t, is should then use 2 readb()'s ??

I could of course turn that sequence into a macro ....

Kurt

--

[ message continues ]

From: Kurt Van Dijck <kurt.van.dijck@eia.be>

Yes, this is what you'll need to do.
--

[ message continues ]

There's ioread() and get_unaligned(). Do we have a combination of this?

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   |

[ message continues ]

From: Marc Kleine-Budde <mkl@pengutronix.de>

No.  And there will never be.

Because it's never well defined whether it is safe to break up an
I/O operation into two sub-operations, nor in what order those
sub-operations can be done in.

For example, just reading one byte of a two-byte register might
clear the status bits in other byte if the two bytes are read in
the wrong order.

On some chips, avoiding losing information might be completely
impossible.

You have to do it by hand in your driver, because only you know
what implementation will work correctly.
--

[ message continues ]

(Thanks for the explanation)^2.

I left my code now with the only warning from sparse:
warning: Using plain integer as NULL pointer

That means, I got all __iomem references fixed.
Is this '0' instead of 'NULL' a big problem? I got plenty of those.

Kurt
--

[ message continues ]

No problem for the compiler, but it's bad style :)
Please fix it.

cheers, 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   |

[ message continues ]

It seems a threaded irq is the way to go.
During the implementation, a locking issue came up.
Am I right that within a threaded irq handler, I should use spin_lock_bh()

Kurt
--

[ message continues ]