Re: [PATCH net-next-2.6 v2 1/2] can: add driver for Softing card

[Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>]

Hi Kurt,

here comes my review... First some general remarks. As Mark already
pointed out, there are still some coding style issues:

- Please use the following style for multi line comments:

  /*
   * Comment
   */

- Please separate the function header from the body by one empty line.

- Please avoid alignment of expressions and structure members.

- Please use a consistent style for function declaration and
  continuation lines.

More comments inline...

On 12/23/2010 10:43 On 01/04/2011 04:07 PM, Kurt Van Dijck wrote:
> 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-/BeEPy95v10@public.gmane.org>
> 
> ---
>  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          |  193 ++++++
>  drivers/net/can/softing/softing_fw.c       |  648 ++++++++++++++++++++
>  drivers/net/can/softing/softing_main.c     |  903 ++++++++++++++++++++++++++++
>  drivers/net/can/softing/softing_platform.h |   38 ++
>  8 files changed, 1806 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/

Please use "obj-$(CONFIG_CAN_SOFTING)" here.

>  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 is then identical for PCI
> +	  and PCMCIA cards. This driver operates on a platform device,
> +	  which has been created by softing_cs or softing_pci driver.
> +	  Warning:
> +	  The API of the card does not allow fine control per bus, but
> +	  controls the 2 busses on the card together.
> +	  As such, some actions (start/stop/busoff recovery) on 1 bus
> +	  must bring down the other bus too temporarily.
> diff --git a/drivers/net/can/softing/Makefile b/drivers/net/can/softing/Makefile
> new file mode 100644
> index 0000000..7878b7b
> --- /dev/null
> +++ b/drivers/net/can/softing/Makefile
> @@ -0,0 +1,5 @@
> +
> +softing-y := softing_main.o softing_fw.o
> +obj-$(CONFIG_CAN_SOFTING)        += softing.o
> +
> +ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/softing/softing.h b/drivers/net/can/softing/softing.h
> new file mode 100644
> index 0000000..72d3adb
> --- /dev/null
> +++ b/drivers/net/can/softing/softing.h
> @@ -0,0 +1,193 @@
> +/*
> + * softing common interfaces
> + *
> + * by Kurt Van Dijck, 06-2008
> + */
> +
> +#include <linux/atomic.h>
> +#include <linux/netdevice.h>
> +#include <linux/ktime.h>
> +#include <linux/mutex.h>
> +#include <linux/spinlock.h>
> +#include <linux/can.h>
> +#include <linux/can/dev.h>
> +
> +#include "softing_platform.h"
> +
> +#ifndef CAN_CTRLMODE_BERR_REPORTING
> +#define CAN_CTRLMODE_BERR_REPORTING 0
> +#endif

Hm, do you really need that definition?

> +struct softing;
> +
> +struct softing_priv {
> +	struct can_priv can;	/* must be the first member! */
> +	struct net_device *netdev;
> +	struct softing *card;
> +	struct {
> +		int pending;
> +		/* variables wich hold the circular buffer */
> +		int echo_put;
> +		int echo_get;
> +	} tx;
> +	struct can_bittiming_const btr_const;
> +	int index;
> +	u8 output;
> +	u16 chip;
> +};
> +#define netdev2softing(netdev)	((struct softing_priv *)netdev_priv(netdev))
> +
> +struct softing {
> +	const struct softing_platform_data *pdat;
> +	struct platform_device *pdev;
> +	struct net_device *net[2];
> +	spinlock_t spin; /* protect this structure & DPRAM access */
> +	ktime_t ts_ref;
> +	ktime_t ts_overflow; /* timestamp overflow value, in ktime */
> +
> +	struct {
> +		/* indication of firmware status */
> +		int up;
> +		/* protection of the 'up' variable */
> +		struct mutex lock;
> +	} fw;
> +	struct {
> +		int nr;
> +		int requested;
> +		int svc_count;
> +		unsigned int dpram_position;
> +	} irq;
> +	struct {
> +		int pending;
> +		int last_bus;
> +		/*
> +		 * keep the bus that last tx'd a message,
> +		 * in order to let every netdev queue resume
> +		 */
> +	} tx;
> +	__iomem uint8_t *dpram;
> +	unsigned long dpram_phys;
> +	unsigned long dpram_size;
> +	struct {
> +		u32  serial, fw, hw, lic;
> +		u16  chip[2];
> +		u32  freq;
> +	} id;
> +};
> +
> +extern int softing_default_output(struct net_device *netdev);
> +
> +extern ktime_t softing_raw2ktime(struct softing *card, u32 raw);
> +
> +extern int softing_fct_cmd(struct softing *card, int16_t cmd, uint16_t vector,
> +		const char *msg);
> +
> +extern int softing_bootloader_command(struct softing *card, int16_t cmd,
> +		const char *msg);
> +
> +/* Load firmware after reset */
> +extern int softing_load_fw(const char *file, struct softing *card,
> +			__iomem uint8_t *virt, unsigned int size, int offset);
> +
> +/* Load final application firmware after bootloader */
> +extern int softing_load_app_fw(const char *file, struct softing *card);
> +
> +/*
> + * enable or disable irq
> + * only called with fw.lock locked
> + */
> +extern int softing_enable_irq(struct softing *card, int enable);
> +
> +/* start/stop 1 bus on card */
> +extern int softing_startstop(struct net_device *netdev, int up);
> +
> +/* netif_rx() */
> +extern int softing_netdev_rx(struct net_device *netdev,
> +		const struct can_frame *msg, ktime_t ktime);
> +
> +/* SOFTING DPRAM mappings */
> +#define DPRAM_RX		0x0000
> +	#define DPRAM_RX_SIZE	32
> +	#define DPRAM_RX_CNT	16
> +#define DPRAM_RX_RD		0x0201	/* uint8_t */
> +#define DPRAM_RX_WR		0x0205	/* uint8_t */
> +#define DPRAM_RX_LOST		0x0207	/* uint8_t */
> +
> +#define DPRAM_FCT_PARAM		0x0300	/* int16_t [20] */
> +#define DPRAM_FCT_RESULT	0x0328	/* int16_t */
> +#define DPRAM_FCT_HOST		0x032b	/* uint16_t */
> +
> +#define DPRAM_INFO_BUSSTATE	0x0331	/* uint16_t */
> +#define DPRAM_INFO_BUSSTATE2	0x0335	/* uint16_t */
> +#define DPRAM_INFO_ERRSTATE	0x0339	/* uint16_t */
> +#define DPRAM_INFO_ERRSTATE2	0x033d	/* uint16_t */
> +#define DPRAM_RESET		0x0341	/* uint16_t */
> +#define DPRAM_CLR_RECV_FIFO	0x0345	/* uint16_t */
> +#define DPRAM_RESET_TIME	0x034d	/* uint16_t */
> +#define DPRAM_TIME		0x0350	/* uint64_t */
> +#define DPRAM_WR_START		0x0358	/* uint8_t */
> +#define DPRAM_WR_END		0x0359	/* uint8_t */
> +#define DPRAM_RESET_RX_FIFO	0x0361	/* uint16_t */
> +#define DPRAM_RESET_TX_FIFO	0x0364	/* uint8_t */
> +#define DPRAM_READ_FIFO_LEVEL	0x0365	/* uint8_t */
> +#define DPRAM_RX_FIFO_LEVEL	0x0366	/* uint16_t */
> +#define DPRAM_TX_FIFO_LEVEL	0x0366	/* uint16_t */
> +
> +#define DPRAM_TX		0x0400	/* uint16_t */
> +	#define DPRAM_TX_SIZE	16
> +	#define DPRAM_TX_CNT	32
> +#define DPRAM_TX_RD		0x0601	/* uint8_t */
> +#define DPRAM_TX_WR		0x0605	/* uint8_t */
> +
> +#define DPRAM_COMMAND		0x07e0	/* uint16_t */
> +#define DPRAM_RECEIPT		0x07f0	/* uint16_t */
> +#define DPRAM_IRQ_TOHOST	0x07fe	/* uint8_t */
> +#define DPRAM_IRQ_TOCARD	0x07ff	/* uint8_t */
> +
> +#define DPRAM_V2_RESET		0x0e00	/* uint8_t */
> +#define DPRAM_V2_IRQ_TOHOST	0x0e02	/* uint8_t */
> +
> +#define TXMAX	(DPRAM_TX_CNT - 1)
> +
> +/* DPRAM return codes */
> +#define RES_NONE	0
> +#define RES_OK		1
> +#define RES_NOK		2
> +#define RES_UNKNOWN	3
> +/* DPRAM flags */
> +#define CMD_TX		0x01
> +#define CMD_ACK		0x02
> +#define CMD_XTD		0x04
> +#define CMD_RTR		0x08
> +#define CMD_ERR		0x10
> +#define CMD_BUS2	0x80
> +
> +/*
> + * some inline DPRAM acces function
> + * to prevent extra dependency between softing & softingcs
> + */
> +/* reset DPRAM */
> +static inline void softing_set_reset_dpram(struct softing *card)
> +{
> +	if (card->pdat->generation >= 2) {
> +		uint8_t tmp;
> +		spin_lock_bh(&card->spin);
> +		tmp = ioread8(&card->dpram[DPRAM_V2_RESET]);
> +		tmp &= ~1;
> +		iowrite8(tmp, &card->dpram[DPRAM_V2_RESET]);
> +		spin_unlock_bh(&card->spin);
> +	}
> +}
> +
> +static inline void softing_clr_reset_dpram(struct softing *card)
> +{
> +	if (card->pdat->generation >= 2) {
> +		uint8_t tmp;

Empty line please.

> +		spin_lock_bh(&card->spin);
> +		tmp = ioread8(&card->dpram[DPRAM_V2_RESET]);
> +		tmp |= 1;

Could be done in one line or even without tmp.

> +		iowrite8(tmp, &card->dpram[DPRAM_V2_RESET]);
> +		spin_unlock_bh(&card->spin);
> +	}
> +}
> +
> diff --git a/drivers/net/can/softing/softing_fw.c b/drivers/net/can/softing/softing_fw.c
> new file mode 100644
> index 0000000..03ed853
> --- /dev/null
> +++ b/drivers/net/can/softing/softing_fw.c
> @@ -0,0 +1,648 @@
> +/*
> +* drivers/net/can/softing/softing_fw.c
> +*
> +* Copyright (C) 2008-2010
> +*
> +* - Kurt Van Dijck, EIA Electronics
> +*
> +* This program is free software; you can redistribute it and/or modify
> +* it under the terms of the version 2 of the GNU General Public License
> +* as published by the Free Software Foundation
> +*
> +* This program is distributed in the hope that it will be useful,
> +* but WITHOUT ANY WARRANTY; without even the implied warranty of
> +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +* GNU General Public License for more details.
> +*
> +* You should have received a copy of the GNU General Public License
> +* along with this program; if not, write to the Free Software
> +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> +*/
> +
> +#include <linux/firmware.h>
> +#include <linux/sched.h>
> +#include <asm/div64.h>
> +
> +#include "softing.h"
> +
> +int softing_fct_cmd(struct softing *card, int16_t cmd, uint16_t vector,
> +		const char *msg)
> +{
> +	int ret;
> +	unsigned long stamp;
> +
> +	if (vector == RES_OK)
> +		vector = RES_NONE;
> +	iowrite16(cmd, &card->dpram[DPRAM_FCT_PARAM]);
> +	iowrite8(vector >> 8, &card->dpram[DPRAM_FCT_HOST + 1]);
> +	iowrite8(vector, &card->dpram[DPRAM_FCT_HOST]);
> +
> +	/* be sure to flush this to the card */
> +	wmb();
> +	stamp = jiffies + 1 * HZ;
> +	/* wait for card */
> +	do {
> +		/* DPRAM_FCT_HOST is _not_ aligned */
> +		ret = ioread8(&card->dpram[DPRAM_FCT_HOST]) +
> +			(ioread8(&card->dpram[DPRAM_FCT_HOST + 1]) << 8);
> +		/* don't have any cached variables */
> +		rmb();
> +		if (ret == RES_OK) {
> +			/* don't read return-value now */
> +			ret = ioread16(&card->dpram[DPRAM_FCT_RESULT]);
> +			if (ret)
> +				dev_alert(&card->pdev->dev,
> +					"%s returned %u\n", msg, ret);
> +			return 0;

Why do you not return an error here?

> +		}
> +		if (time_after(jiffies, stamp))
> +			break;
> +		/* process context => relax */
> +		usleep_range(500, 10000);
> +	} while (!signal_pending(current));
> +
> +	if (ret == RES_NONE) {
> +		dev_alert(&card->pdev->dev,
> +			"%s, no response from card on %u/0x%02x\n",
> +			msg, cmd, vector);
> +		return 1;
> +	} else {
> +		dev_alert(&card->pdev->dev,
> +			"%s, bad response from card on %u/0x%02x, 0x%04x\n",
> +			msg, cmd, vector, ret);
> +		/* make sure to return something not 0 */
> +		return ret ?: 1;

What does it return if ret > 0?

> +	}
> +}
> +
> +int softing_bootloader_command(struct softing *card, int16_t cmd,
> +		const char *msg)
> +{
> +	int ret;
> +	unsigned long stamp;

Empty line please.

> +	iowrite16(RES_NONE, &card->dpram[DPRAM_RECEIPT]);
> +	iowrite16(cmd, &card->dpram[DPRAM_COMMAND]);
> +	/* be sure to flush this to the card */
> +	wmb();
> +	stamp = jiffies + 3 * HZ;
> +	/* wait for card */
> +	do {
> +		ret = ioread16(&card->dpram[DPRAM_RECEIPT]);
> +		/* don't have any cached variables */
> +		rmb();
> +		if (ret == RES_OK)
> +			return 0;
> +		if (time_after(jiffies, stamp))
> +			break;
> +		/* process context => relax */
> +		usleep_range(500, 10000);
> +	} while (!signal_pending(current));
> +
> +	switch (ret) {
> +	case RES_NONE:
> +		dev_alert(&card->pdev->dev, "%s: no response from card\n", msg);
> +		break;
> +	case RES_NOK:
> +		dev_alert(&card->pdev->dev, "%s: response from card nok\n",
> +				msg);
> +		break;
> +	case RES_UNKNOWN:
> +		dev_alert(&card->pdev->dev, "%s: command 0x%04x unknown\n",
> +			msg, cmd);
> +		break;
> +	default:
> +		dev_alert(&card->pdev->dev, "%s: bad response from card: %i\n",
> +			msg, ret);
> +		break;
> +	}
> +	return ret ?: 1;

Ditto.

> +}
> +
> +static int fw_parse(const uint8_t **pmem, uint16_t *ptype, uint32_t *paddr,
> +		uint16_t *plen, const uint8_t **pdat)
> +{
> +	uint16_t checksum[2];
> +	const uint8_t *mem;
> +	const uint8_t *end;
> +
> +	mem = *pmem;
> +	*ptype = le16_to_cpup((void *)&mem[0]);
> +	*paddr = le32_to_cpup((void *)&mem[2]);
> +	*plen = le16_to_cpup((void *)&mem[6]);
> +	*pdat = &mem[8];

You often handle arrays of specific data. Couldn't those be described
better by structs also avoiding ugly casts??

> +	/* verify checksum */
> +	end = &mem[8 + *plen];
> +	checksum[0] = le16_to_cpup((void *)end);
> +	for (checksum[1] = 0; mem < end; ++mem)
> +		checksum[1] += *mem;
> +	if (checksum[0] != checksum[1])
> +		return -EINVAL;
> +	/* increment */
> +	*pmem += 10 + *plen;
> +	return 0;
> +}
> +
> +int softing_load_fw(const char *file, struct softing *card,
> +			__iomem uint8_t *dpram, unsigned int size, int offset)
> +{
> +	const struct firmware *fw;
> +	int ret, ok = 0;
> +	const uint8_t *mem, *end, *dat;
> +	uint16_t type, len;
> +	uint32_t addr;
> +	uint8_t buf[1024];

Please avoid allocating large arrays on the stack.

> +
> +	ret = request_firmware(&fw, file, &card->pdev->dev);
> +	if (ret) {
> +		dev_alert(&card->pdev->dev, "request_firmware(%s) got %i\n",
> +			file, ret);
> +		return ret;
> +	}
> +	dev_dbg(&card->pdev->dev, "%s, firmware(%s) got %u bytes"
> +		", offset %c0x%04x\n",
> +		card->pdat->name, file, (unsigned int)fw->size,
> +		(offset >= 0) ? '+' : '-', (unsigned int)abs(offset));
> +	/* parse the firmware */
> +	mem = fw->data;
> +	end = &mem[fw->size];
> +	/* look for header record */
> +	ret = fw_parse(&mem, &type, &addr, &len, &dat);
> +	if (ret < 0)
> +		goto fw_end;
> +	if (type != 0xffff) {
> +		dev_alert(&card->pdev->dev, "firware starts with type 0x%04x\n",
> +			type);
> +		goto fw_end;
> +	}
> +	if (strncmp("Structured Binary Format, Softing GmbH" , dat, len)) {
> +		dev_info(&card->pdev->dev, "firware string '%.*s'\n", len, dat);
> +		goto fw_end;
> +	}
> +	ok |= 1;
> +	/* ok, we had a header */
> +	while (mem < end) {
> +		ret = fw_parse(&mem, &type, &addr, &len, &dat);
> +		if (ret)
> +			break;
> +		if (type == 3) {
> +			/* start address */
> +			ok |= 2;
> +			continue;
> +		} else if (type == 1) {
> +			/* eof */
> +			ok |= 4;
> +			goto fw_end;
> +		} else if (type != 0) {
> +			dev_alert(&card->pdev->dev,
> +					"unknown record type 0x%04x\n", type);
> +			break;
> +		}

You still use a lot of magic constants. Giving them a name would make
the code more readable.

> +		if ((addr + len + offset) > size) {
> +			dev_alert(&card->pdev->dev,
> +				"firmware out of range (0x%08x / 0x%08x)\n",
> +				(addr + len + offset), size);
> +			goto fw_end;
> +		}
> +		memcpy_toio(&dpram[addr + offset], dat, len);
> +		/* be sure to flush caches from IO space */
> +		mb();
> +		if (len > sizeof(buf)) {
> +			dev_info(&card->pdev->dev,
> +				"record too big for verify (%u)\n", len);
> +			continue;
> +		}
> +		/* verify record data */
> +		memcpy_fromio(buf, &dpram[addr + offset], len);
> +		if (!memcmp(buf, dat, len))
> +			/* is ok */
> +			continue;
> +		dev_alert(&card->pdev->dev, "0x%08x:0x%03x at 0x%u failed\n",
> +				addr, len, addr + offset);
> +		goto fw_end;
> +	}
> +fw_end:
> +	release_firmware(fw);
> +	if (0x5 == (ok & 0x5))
> +		/* got eof & start */
> +		return 0;

Ditto.

> +	dev_info(&card->pdev->dev, "firmware %s failed\n", file);
> +	return ret ?: -EINVAL;
> +}
> +
> +int softing_load_app_fw(const char *file, struct softing *card)
> +{
> +	const struct firmware *fw;
> +	const uint8_t *mem, *end, *dat;
> +	int ret, ok = 0, j;
> +	uint16_t type, len;
> +	uint32_t addr, start_addr = 0;
> +	unsigned int sum, rx_sum;
> +
> +	ret = request_firmware(&fw, file, &card->pdev->dev);
> +	if (ret) {
> +		dev_alert(&card->pdev->dev, "request_firmware(%s) got %i\n",
> +			file, ret);
> +		return ret;
> +	}
> +	dev_dbg(&card->pdev->dev, "firmware(%s) got %lu bytes\n",
> +		file, (unsigned long)fw->size);
> +	/* parse the firmware */
> +	mem = fw->data;
> +	end = &mem[fw->size];
> +	/* look for header record */
> +	ret = fw_parse(&mem, &type, &addr, &len, &dat);
> +	if (ret)
> +		goto fw_end;
> +	if (type != 0xffff) {
> +		dev_alert(&card->pdev->dev, "firware starts with type 0x%04x\n",

Typo?

> +			type);
> +		goto fw_end;
> +	}
> +	if (strncmp("Structured Binary Format, Softing GmbH", dat, len)) {
> +		dev_alert(&card->pdev->dev, "firware string '%.*s' fault\n",
> +				len, dat);
> +		goto fw_end;
> +	}
> +	ok |= 1;
> +	/* ok, we had a header */
> +	while (mem < end) {
> +		ret = fw_parse(&mem, &type, &addr, &len, &dat);
> +		if (ret)
> +			break;
> +
> +		if (type == 3) {
> +			/* start address */
> +			start_addr = addr;
> +			ok |= 2;
> +			continue;
> +		} else if (type == 1) {
> +			/* eof */
> +			ok |= 4;
> +			goto fw_end;
> +		} else if (type != 0) {
> +			dev_alert(&card->pdev->dev,
> +					"unknown record type 0x%04x\n", type);
> +			break;
> +		}
> +
> +		/* regualar data */
> +		for (sum = 0, j = 0; j < len; ++j)
> +			sum += dat[j];
> +		/* work in 16bit (target) */
> +		sum &= 0xffff;
> +
> +		memcpy_toio(&card->dpram[card->pdat->app.offs], dat, len);
> +		iowrite32(card->pdat->app.offs + card->pdat->app.addr,
> +				&card->dpram[DPRAM_COMMAND + 2]);
> +		iowrite32(addr, &card->dpram[DPRAM_COMMAND + 6]);
> +		iowrite16(len, &card->dpram[DPRAM_COMMAND + 10]);
> +		iowrite8(1, &card->dpram[DPRAM_COMMAND + 12]);

See my comment about using arrays above.

> +		if (softing_bootloader_command(card, 1, "loading app."))
> +			goto fw_end;
> +		/* verify checksum */
> +		rx_sum = ioread16(&card->dpram[DPRAM_RECEIPT + 2]);
> +		if (rx_sum != sum) {
> +			dev_alert(&card->pdev->dev, "SRAM seems to be damaged"
> +				", wanted 0x%04x, got 0x%04x\n", sum, rx_sum);
> +			goto fw_end;
> +		}
> +	}
> +fw_end:
> +	release_firmware(fw);
> +	if (ok != 7)
> +		goto fw_failed;
> +	/* got start, start_addr, & eof */
> +	iowrite32(start_addr, &card->dpram[DPRAM_COMMAND + 2]);
> +	iowrite8(1, &card->dpram[DPRAM_COMMAND + 6]);
> +	if (softing_bootloader_command(card, 3, "start app."))
> +		goto fw_failed;
> +	dev_info(&card->pdev->dev, "firmware %s up\n", file);
> +	return 0;
> +fw_failed:
> +	dev_info(&card->pdev->dev, "firmware %s failed\n", file);
> +	return ret ?: -EINVAL;
> +}
> +
> +static int softing_reset_chip(struct softing *card)
> +{
> +	do {
> +		/* reset chip */
> +		iowrite8(0, &card->dpram[DPRAM_RESET_RX_FIFO]);
> +		iowrite8(0, &card->dpram[DPRAM_RESET_RX_FIFO+1]);
> +		iowrite8(1, &card->dpram[DPRAM_RESET]);
> +		iowrite8(0, &card->dpram[DPRAM_RESET+1]);
> +
> +		if (!softing_fct_cmd(card, 0, 0, "reset_chip"))
> +			break;
> +		if (signal_pending(current))
> +			goto failed;
> +		/* sync */
> +		if (softing_fct_cmd(card, 99, 0x55, "sync-a"))
> +			goto failed;
> +		if (softing_fct_cmd(card, 99, 0xaa, "sync-a"))
> +			goto failed;
> +	} while (1);
> +	card->tx.pending = 0;
> +	return 0;
> +failed:
> +	return -EIO;
> +}
> +
> +static void softing_initialize_timestamp(struct softing *card)
> +{
> +	uint64_t ovf;
> +
> +	card->ts_ref = ktime_get();
> +
> +	/* 16MHz is the reference */
> +	ovf = 0x100000000ULL * 16;
> +	do_div(ovf, card->pdat->freq ?: 16);
> +
> +	card->ts_overflow = ktime_add_us(ktime_set(0, 0), ovf);
> +}
> +
> +ktime_t softing_raw2ktime(struct softing *card, u32 raw)
> +{
> +	uint64_t rawl;
> +	ktime_t now, real_offset;
> +	ktime_t target;
> +	ktime_t tmp;
> +
> +	now = ktime_get();
> +	real_offset = ktime_sub(ktime_get_real(), now);
> +
> +	/* find nsec from card */
> +	rawl = raw * 16;
> +	do_div(rawl, card->pdat->freq ?: 16);
> +	target = ktime_add_us(card->ts_ref, rawl);
> +	/* test for overflows */
> +	tmp = ktime_add(target, card->ts_overflow);
> +	while (unlikely(ktime_to_ns(tmp) > ktime_to_ns(now))) {
> +		card->ts_ref = ktime_add(card->ts_ref, card->ts_overflow);
> +		target = tmp;
> +		tmp = ktime_add(target, card->ts_overflow);
> +	}
> +	return ktime_add(target, real_offset);
> +}
> +
> +static inline int softing_error_reporting(struct net_device *netdev)
> +{
> +	struct softing_priv *priv = netdev_priv(netdev);
> +
> +	return (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
> +		? 1 : 0;
> +}
> +
> +int softing_startstop(struct net_device *dev, int up)
> +{
> +	int ret;
> +	struct softing *card;
> +	struct softing_priv *priv;
> +	struct net_device *netdev;
> +	int mask_start;
> +	int j, error_reporting;
> +	struct can_frame msg;
> +	const struct can_bittiming *bt;
> +
> +	priv = netdev_priv(dev);
> +	card = priv->card;
> +
> +	if (!card->fw.up)
> +		return -EIO;
> +
> +	ret = mutex_lock_interruptible(&card->fw.lock);
> +	if (ret)
> +		return ret;
> +
> +	mask_start = 0;
> +	if (dev && up)
> +		/* prepare to start this bus as well */
> +		mask_start |= (1 << priv->index);
> +	/* bring netdevs down */
> +	for (j = 0; j < ARRAY_SIZE(card->net); ++j) {
> +		netdev = card->net[j];
> +		if (!netdev)
> +			continue;
> +		priv = netdev_priv(netdev);
> +
> +		if (dev != netdev)
> +			netif_stop_queue(netdev);
> +
> +		if (netif_running(netdev)) {
> +			if (dev != netdev)
> +				mask_start |= (1 << j);
> +			priv->tx.pending = 0;
> +			priv->tx.echo_put = 0;
> +			priv->tx.echo_get = 0;
> +			/* this bus' may just have called open_candev()

Please use

  /*
   * Comment
   */

> +			 * which is rather stupid to call close_candev()
> +			 * already
> +			 * but we may come here from busoff recovery too
> +			 * in which case the echo_skb _needs_ flushing too.
> +			 * just be sure to call open_candev() again
> +			 */
> +			close_candev(netdev);
> +		}
> +		priv->can.state = CAN_STATE_STOPPED;
> +	}
> +	card->tx.pending = 0;

> +
> +	softing_enable_irq(card, 0);
> +	ret = softing_reset_chip(card);
> +	if (ret)
> +		goto failed;
> +	if (!mask_start)
> +		/* no busses to be brought up */
> +		goto card_done;
> +
> +	if ((mask_start & 1) && (mask_start & 2)
> +			&& (softing_error_reporting(card->net[0])
> +				!= softing_error_reporting(card->net[1]))) {
> +		dev_alert(&card->pdev->dev,
> +				"err_reporting flag differs for busses\n");
> +		goto invalid;
> +	}
> +	error_reporting = 0;
> +	if (mask_start & 1) {
> +		netdev = card->net[0];
> +		priv = netdev_priv(netdev);
> +		error_reporting += softing_error_reporting(netdev);
> +		/* init chip 1 */
> +		bt = &priv->can.bittiming;
> +		iowrite16(bt->brp, &card->dpram[DPRAM_FCT_PARAM + 2]);
> +		iowrite16(bt->sjw, &card->dpram[DPRAM_FCT_PARAM + 4]);
> +		iowrite16(bt->phase_seg1 + bt->prop_seg,
> +				&card->dpram[DPRAM_FCT_PARAM + 6]);
> +		iowrite16(bt->phase_seg2, &card->dpram[DPRAM_FCT_PARAM + 8]);
> +		iowrite16((priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES) ? 1 : 0,
> +				&card->dpram[DPRAM_FCT_PARAM + 10]);
> +		if (softing_fct_cmd(card, 1, 0, "initialize_chip[0]"))
> +			goto failed;
> +		/* set mode */
> +		iowrite16(0, &card->dpram[DPRAM_FCT_PARAM + 2]);
> +		iowrite16(0, &card->dpram[DPRAM_FCT_PARAM + 4]);
> +		if (softing_fct_cmd(card, 3, 0, "set_mode[0]"))
> +			goto failed;
> +		/* set filter */
> +		/* 11bit id & mask */
> +		iowrite16(0x0000, &card->dpram[DPRAM_FCT_PARAM + 2]);
> +		iowrite16(0x07ff, &card->dpram[DPRAM_FCT_PARAM + 4]);
> +		/* 29bit id.lo & mask.lo & id.hi & mask.hi */
> +		iowrite16(0x0000, &card->dpram[DPRAM_FCT_PARAM + 6]);
> +		iowrite16(0xffff, &card->dpram[DPRAM_FCT_PARAM + 8]);
> +		iowrite16(0x0000, &card->dpram[DPRAM_FCT_PARAM + 10]);
> +		iowrite16(0x1fff, &card->dpram[DPRAM_FCT_PARAM + 12]);
> +		if (softing_fct_cmd(card, 7, 0, "set_filter[0]"))
> +			goto failed;
> +		/* set output control */
> +		iowrite16(priv->output, &card->dpram[DPRAM_FCT_PARAM + 2]);
> +		if (softing_fct_cmd(card, 5, 0, "set_output[0]"))
> +			goto failed;
> +	}
> +	if (mask_start & 2) {

Magic constants?

> +		netdev = card->net[1];
> +		priv = netdev_priv(netdev);
> +		error_reporting += softing_error_reporting(netdev);
> +		/* init chip2 */
> +		bt = &priv->can.bittiming;
> +		iowrite16(bt->brp, &card->dpram[DPRAM_FCT_PARAM + 2]);
> +		iowrite16(bt->sjw, &card->dpram[DPRAM_FCT_PARAM + 4]);
> +		iowrite16(bt->phase_seg1 + bt->prop_seg,
> +				&card->dpram[DPRAM_FCT_PARAM + 6]);
> +		iowrite16(bt->phase_seg2, &card->dpram[DPRAM_FCT_PARAM + 8]);
> +		iowrite16((priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES) ? 1 : 0,
> +				&card->dpram[DPRAM_FCT_PARAM + 10]);
> +		if (softing_fct_cmd(card, 2, 0, "initialize_chip[1]"))
> +			goto failed;
> +		/* set mode2 */
> +		iowrite16(0, &card->dpram[DPRAM_FCT_PARAM + 2]);
> +		iowrite16(0, &card->dpram[DPRAM_FCT_PARAM + 4]);
> +		if (softing_fct_cmd(card, 4, 0, "set_mode[1]"))
> +			goto failed;
> +		/* set filter2 */
> +		/* 11bit id & mask */
> +		iowrite16(0x0000, &card->dpram[DPRAM_FCT_PARAM + 2]);
> +		iowrite16(0x07ff, &card->dpram[DPRAM_FCT_PARAM + 4]);
> +		/* 29bit id.lo & mask.lo & id.hi & mask.hi */
> +		iowrite16(0x0000, &card->dpram[DPRAM_FCT_PARAM + 6]);
> +		iowrite16(0xffff, &card->dpram[DPRAM_FCT_PARAM + 8]);
> +		iowrite16(0x0000, &card->dpram[DPRAM_FCT_PARAM + 10]);
> +		iowrite16(0x1fff, &card->dpram[DPRAM_FCT_PARAM + 12]);
> +		if (softing_fct_cmd(card, 8, 0, "set_filter[1]"))
> +			goto failed;
> +		/* set output control2 */
> +		iowrite16(priv->output, &card->dpram[DPRAM_FCT_PARAM + 2]);
> +		if (softing_fct_cmd(card, 6, 0, "set_output[1]"))
> +			goto failed;
> +	}
> +	/* enable_error_frame */
> +	/*
> +	if (error_reporting) {
> +		if (softing_fct_cmd(card, 51, 0, "enable_error_frame"))
> +			goto failed;
> +	}
> +	*/

Please remove dead code!

> +	/* initialize interface */
> +	iowrite16(1, &card->dpram[DPRAM_FCT_PARAM + 2]);
> +	iowrite16(1, &card->dpram[DPRAM_FCT_PARAM + 4]);
> +	iowrite16(1, &card->dpram[DPRAM_FCT_PARAM + 6]);
> +	iowrite16(1, &card->dpram[DPRAM_FCT_PARAM + 8]);
> +	iowrite16(1, &card->dpram[DPRAM_FCT_PARAM + 10]);
> +	iowrite16(1, &card->dpram[DPRAM_FCT_PARAM + 12]);
> +	iowrite16(1, &card->dpram[DPRAM_FCT_PARAM + 14]);
> +	iowrite16(1, &card->dpram[DPRAM_FCT_PARAM + 16]);
> +	iowrite16(1, &card->dpram[DPRAM_FCT_PARAM + 18]);
> +	iowrite16(1, &card->dpram[DPRAM_FCT_PARAM + 20]);

Could be coded more efficiently with a for loop.

> +	if (softing_fct_cmd(card, 17, 0, "initialize_interface"))
> +		goto failed;
> +	/* enable_fifo */
> +	if (softing_fct_cmd(card, 36, 0, "enable_fifo"))
> +		goto failed;
> +	/* enable fifo tx ack */
> +	if (softing_fct_cmd(card, 13, 0, "fifo_tx_ack[0]"))
> +		goto failed;
> +	/* enable fifo tx ack2 */
> +	if (softing_fct_cmd(card, 14, 0, "fifo_tx_ack[1]"))
> +		goto failed;
> +	/* enable timestamps */
> +	/* is default, no code found */
> +	/* start_chip */
> +	if (softing_fct_cmd(card, 11, 0, "start_chip"))
> +		goto failed;
> +	iowrite8(0, &card->dpram[DPRAM_INFO_BUSSTATE]);
> +	iowrite8(0, &card->dpram[DPRAM_INFO_BUSSTATE2]);
> +	dev_info(&card->pdev->dev, "%s up\n", __func__);
> +	if (card->pdat->generation < 2) {
> +		iowrite8(0, &card->dpram[DPRAM_V2_IRQ_TOHOST]);
> +		/* flush the DPRAM caches */
> +		wmb();
> +	}
> +
> +	softing_initialize_timestamp(card);
> +
> +	/*
> +	 * do socketcan notifications/status changes
> +	 * from here, no errors should occur, or the failed: part
> +	 * must be reviewed
> +	 */
> +	memset(&msg, 0, sizeof(msg));
> +	msg.can_id = CAN_ERR_FLAG | CAN_ERR_RESTARTED;
> +	msg.can_dlc = CAN_ERR_DLC;
> +	for (j = 0; j < ARRAY_SIZE(card->net); ++j) {
> +		if (!(mask_start & (1 << j)))
> +			continue;
> +		netdev = card->net[j];
> +		if (!netdev)
> +			continue;
> +		priv = netdev_priv(netdev);
> +		priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +		open_candev(netdev);
> +		if (dev != netdev) {
> +			/* notify other busses on the restart */
> +			softing_netdev_rx(netdev, &msg, ktime_set(0, 0));
> +			++priv->can.can_stats.restarts;
> +		}
> +		netif_wake_queue(netdev);
> +	}
> +
> +	/* enable interrupts */
> +	ret = softing_enable_irq(card, 1);
> +	if (ret)
> +		goto failed;
> +card_done:
> +	mutex_unlock(&card->fw.lock);
> +	return 0;
> +failed:
> +	dev_alert(&card->pdev->dev, "firmware failed, going idle\n");
> +invalid:
> +	softing_enable_irq(card, 0);
> +	softing_reset_chip(card);
> +	mutex_unlock(&card->fw.lock);
> +	/* bring all other interfaces down */
> +	for (j = 0; j < ARRAY_SIZE(card->net); ++j) {
> +		netdev = card->net[j];
> +		if (!netdev)
> +			continue;
> +		dev_close(netdev);
> +	}
> +	return -EIO;
> +}
> +
> +int softing_default_output(struct net_device *netdev)
> +{
> +	struct softing_priv *priv = netdev_priv(netdev);
> +	struct softing *card = priv->card;
> +
> +	switch (priv->chip) {
> +	case 1000:
> +		if (card->pdat->generation < 2)
> +			return 0xfb;
> +		return 0xfa;
> +	case 5:
> +		return 0x60;
> +	default:
> +		return 0x40;
> +	}
> +}

Again, some magic constants.

> diff --git a/drivers/net/can/softing/softing_main.c b/drivers/net/can/softing/softing_main.c
> new file mode 100644
> index 0000000..4f74075
> --- /dev/null
> +++ b/drivers/net/can/softing/softing_main.c
> @@ -0,0 +1,903 @@
> +/*
> +* drivers/net/can/softing/softing_main.c
> +*
> +* Copyright (C) 2008-2010
> +*
> +* - Kurt Van Dijck, EIA Electronics
> +*
> +* This program is free software; you can redistribute it and/or modify
> +* it under the terms of the version 2 of the GNU General Public License
> +* as published by the Free Software Foundation
> +*
> +* This program is distributed in the hope that it will be useful,
> +* but WITHOUT ANY WARRANTY; without even the implied warranty of
> +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +* GNU General Public License for more details.
> +*
> +* You should have received a copy of the GNU General Public License
> +* along with this program; if not, write to the Free Software
> +* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> +*/
> +
> +#include <linux/version.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +
> +#include "softing.h"
> +
> +#define TX_ECHO_SKB_MAX (TXMAX/2)
> +
> +/*
> + * test is a specific CAN netdev
> + * is online (ie. up 'n running, not sleeping, not busoff
> + */
> +static inline int canif_is_active(struct net_device *netdev)
> +{
> +	struct can_priv *can = netdev_priv(netdev);

Empty line, please.

> +	if (!netif_running(netdev))
> +		return 0;
> +	return (can->state <= CAN_STATE_ERROR_PASSIVE);
> +}
> +
> +/* trigger the tx queue-ing */
> +static netdev_tx_t
> +softing_netdev_start_xmit(struct sk_buff *skb, struct net_device *dev)

See general comments.

> +{
> +	struct softing_priv *priv = netdev_priv(dev);
> +	struct softing *card = priv->card;
> +	int ret;
> +	uint8_t *ptr;
> +	uint8_t fifo_wr, fifo_rd;
> +	struct can_frame *cf = (struct can_frame *)skb->data;
> +	uint8_t buf[DPRAM_TX_SIZE];
> +
> +	if (can_dropped_invalid_skb(dev, skb))
> +		return NETDEV_TX_OK;
> +
> +	spin_lock(&card->spin);
> +
> +	ret = NETDEV_TX_BUSY;
> +	if (!card->fw.up)
> +		goto xmit_done;
> +	if (card->tx.pending >= TXMAX)
> +		goto xmit_done;
> +	if (priv->tx.pending >= TX_ECHO_SKB_MAX)
> +		goto xmit_done;

What about using "||"?

> +	fifo_wr = ioread8(&card->dpram[DPRAM_TX_WR]);
> +	fifo_rd = ioread8(&card->dpram[DPRAM_TX_RD]);
> +	if (fifo_wr == fifo_rd)
> +		/* fifo full */
> +		goto xmit_done;
> +	memset(buf, 0, sizeof(buf));
> +	ptr = buf;
> +	*ptr = CMD_TX;
> +	if (cf->can_id & CAN_RTR_FLAG)
> +		*ptr |= CMD_RTR;
> +	if (cf->can_id & CAN_EFF_FLAG)
> +		*ptr |= CMD_XTD;
> +	if (priv->index)
> +		*ptr |= CMD_BUS2;
> +	++ptr;
> +	*ptr++ = cf->can_dlc;
> +	*ptr++ = (cf->can_id >> 0);
> +	*ptr++ = (cf->can_id >> 8);
> +	if (cf->can_id & CAN_EFF_FLAG) {
> +		*ptr++ = (cf->can_id >> 16);
> +		*ptr++ = (cf->can_id >> 24);
> +	} else {
> +		/* increment 1, not 2 as you might think */
> +		ptr += 1;
> +	}
> +	if (!(cf->can_id & CAN_RTR_FLAG))
> +		memcpy(ptr, &cf->data[0], cf->can_dlc);
> +	memcpy_toio(&card->dpram[DPRAM_TX + DPRAM_TX_SIZE * fifo_wr],
> +			buf, DPRAM_TX_SIZE);
> +	if (++fifo_wr >= DPRAM_TX_CNT)
> +		fifo_wr = 0;
> +	iowrite8(fifo_wr, &card->dpram[DPRAM_TX_WR]);
> +	card->tx.last_bus = priv->index;
> +	++card->tx.pending;
> +	++priv->tx.pending;
> +	can_put_echo_skb(skb, dev, priv->tx.echo_put);
> +	++priv->tx.echo_put;
> +	if (priv->tx.echo_put >= TX_ECHO_SKB_MAX)
> +		priv->tx.echo_put = 0;
> +	/* can_put_echo_skb() saves the skb, safe to return TX_OK */
> +	ret = NETDEV_TX_OK;
> +xmit_done:
> +	spin_unlock(&card->spin);
> +	if (card->tx.pending >= TXMAX) {
> +		int j;

Empty line please.

> +		for (j = 0; j < ARRAY_SIZE(card->net); ++j) {
> +			if (card->net[j])
> +				netif_stop_queue(card->net[j]);
> +		}
> +	}
> +	if (ret != NETDEV_TX_OK)
> +		netif_stop_queue(dev);
> +
> +	return ret;
> +}
> +
> +/*
> + * shortcut for skb delivery
> + */
> +int softing_netdev_rx(struct net_device *netdev,
> +		const struct can_frame *msg, ktime_t ktime)
> +{
> +	struct sk_buff *skb;
> +	struct can_frame *cf;
> +	int ret;
> +
> +	skb = alloc_can_skb(netdev, &cf);
> +	if (!skb)
> +		return -ENOMEM;
> +	memcpy(cf, msg, sizeof(*msg));
> +	skb->tstamp = ktime;
> +	ret = netif_rx(skb);
> +	if (ret == NET_RX_DROP)
> +		++netdev->stats.rx_dropped;

Hm, I wonder if all Socket-CAN drivers should handle the return value of
netif_rx that way?

> +	return ret;
> +}
> +
> +/*
> + * softing_handle_1
> + * pop 1 entry from the DPRAM queue, and process
> + */
> +static int softing_handle_1(struct softing *card)
> +{
> +	int j;
> +	struct net_device *netdev;
> +	struct softing_priv *priv;
> +	ktime_t ktime;
> +	struct can_frame msg;
> +
> +	int lost_msg;
> +	uint8_t fifo_rd, fifo_wr;
> +	unsigned int cnt = 0;
> +	uint8_t *ptr;
> +	u32 tmp;
> +	u8 cmd;
> +	uint8_t buf[DPRAM_RX_SIZE];
> +
> +	memset(&msg, 0, sizeof(msg));
> +	/* test for lost msgs */
> +	lost_msg = ioread8(&card->dpram[DPRAM_RX_LOST]);
> +	if (lost_msg) {
> +		/* reset condition */
> +		iowrite8(0, &card->dpram[DPRAM_RX_LOST]);
> +		/* prepare msg */
> +		msg.can_id = CAN_ERR_FLAG | CAN_ERR_CRTL;
> +		msg.can_dlc = CAN_ERR_DLC;
> +		msg.data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> +		/*
> +		 * service to all busses, we don't know which it was applicable
> +		 * but only service busses that are online
> +		 */
> +		for (j = 0; j < ARRAY_SIZE(card->net); ++j) {
> +			netdev = card->net[j];
> +			if (!netdev)
> +				continue;
> +			if (!canif_is_active(netdev))
> +				/* a dead bus has no overflows */
> +				continue;
> +			++netdev->stats.rx_over_errors;
> +			softing_netdev_rx(netdev, &msg, ktime_set(0, 0));
> +		}
> +		/* prepare for other use */
> +		memset(&msg, 0, sizeof(msg));
> +		++cnt;
> +	}
> +
> +	fifo_rd = ioread8(&card->dpram[DPRAM_RX_RD]);
> +	fifo_wr = ioread8(&card->dpram[DPRAM_RX_WR]);
> +
> +	if (++fifo_rd >= DPRAM_RX_CNT)
> +		fifo_rd = 0;
> +	if (fifo_wr == fifo_rd)
> +		return cnt;
> +
> +	memcpy_fromio(buf, &card->dpram[DPRAM_RX + DPRAM_RX_SIZE*fifo_rd],
> +			DPRAM_RX_SIZE);
> +	mb();
> +	/* trigger dual port RAM */
> +	iowrite8(fifo_rd, &card->dpram[DPRAM_RX_RD]);
> +
> +	ptr = buf;
> +	cmd = *ptr++;
> +	if (cmd == 0xff) {
> +		/* not quite usefull, probably the card has got out */
> +		dev_alert(&card->pdev->dev, "got cmd 0x%02x,"
> +			" I suspect the card is lost\n", cmd);
> +		return 0;
> +	}
> +	netdev = card->net[0];
> +	if (cmd & CMD_BUS2)
> +		netdev = card->net[1];
> +	priv = netdev_priv(netdev);
> +
> +	if (cmd & CMD_ERR) {
> +		u8 can_state;
> +		u8 state;
> +		state = *ptr++;
> +
> +		msg.can_id = CAN_ERR_FLAG;
> +		msg.can_dlc = CAN_ERR_DLC;
> +
> +		if (state & 0x80) {

Again some magic constants!

> +			can_state = CAN_STATE_BUS_OFF;
> +			msg.can_id |= CAN_ERR_BUSOFF;
> +			state = 2;

Ditto.

> +		} else if (state & 0x60) {
> +			can_state = CAN_STATE_ERROR_PASSIVE;
> +			msg.can_id |= CAN_ERR_BUSERROR;

A state change is not a bus error! You should use:

  msg.can_id |= CAN_ERR_CRTL;

> +			msg.data[1] = CAN_ERR_CRTL_TX_PASSIVE;
> +			state = 1;
> +		} else {
> +			can_state = CAN_STATE_ERROR_ACTIVE;
> +			state = 0;
> +			msg.can_id |= CAN_ERR_BUSERROR;

Ditto.

> +		}
> +		/* update DPRAM */
> +		iowrite8(state, &card->dpram[priv->index ?
> +				DPRAM_INFO_BUSSTATE2 : DPRAM_INFO_BUSSTATE]);
> +		/* timestamp */
> +		tmp = le32_to_cpup((void *)ptr);
> +		ptr += 4;
> +		ktime = softing_raw2ktime(card, tmp);
> +
> +		++priv->can.can_stats.bus_error;

Ditto.

> +		++netdev->stats.rx_errors;
> +		/* update internal status */
> +		if (can_state != priv->can.state) {
> +			priv->can.state = can_state;
> +			if (can_state == CAN_STATE_ERROR_PASSIVE)
> +				++priv->can.can_stats.error_passive;
> +			if (can_state == CAN_STATE_BUS_OFF) {

else if?

> +				/* this calls can_close_cleanup() */
> +				can_bus_off(netdev);
> +				netif_stop_queue(netdev);
> +			}
> +			/* trigger socketcan */
> +			softing_netdev_rx(netdev, &msg, ktime);
> +		}
> +
> +	} else {
> +		if (cmd & CMD_RTR)
> +			msg.can_id |= CAN_RTR_FLAG;
> +		/* acknowledge, was tx msg
> +		 * no real tx flag to set
> +		if (cmd & CMD_ACK) {
> +		}
> +		 */
> +		msg.can_dlc = get_can_dlc(*ptr++);
> +		if (cmd & CMD_XTD) {
> +			msg.can_id |= CAN_EFF_FLAG;
> +			msg.can_id |= le32_to_cpup((void *)ptr);
> +			ptr += 4;
> +		} else {
> +			msg.can_id |= le16_to_cpup((void *)ptr);
> +			ptr += 2;
> +		}
> +		/* timestamp */
> +		tmp = le32_to_cpup((void *)ptr);
> +		ptr += 4;
> +		ktime = softing_raw2ktime(card, tmp);
> +		if (!(msg.can_id & CAN_RTR_FLAG))
> +			memcpy(&msg.data[0], ptr, 8);
> +		ptr += 8;
> +		/* update socket */
> +		if (cmd & CMD_ACK) {
> +			struct sk_buff *skb;
> +			skb = priv->can.echo_skb[priv->tx.echo_get];
> +			if (skb)
> +				skb->tstamp = ktime;
> +			can_get_echo_skb(netdev, priv->tx.echo_get);
> +			++priv->tx.echo_get;
> +			if (priv->tx.echo_get >= TX_ECHO_SKB_MAX)
> +				priv->tx.echo_get = 0;
> +			if (priv->tx.pending)
> +				--priv->tx.pending;
> +			if (card->tx.pending)
> +				--card->tx.pending;
> +			++netdev->stats.tx_packets;
> +			netdev->stats.tx_bytes += msg.can_dlc;
> +		} else {
> +			++netdev->stats.rx_packets;
> +			netdev->stats.rx_bytes += msg.can_dlc;
> +			softing_netdev_rx(netdev, &msg, ktime);
> +		}
> +	}
> +	++cnt;
> +	return cnt;
> +}
> +
> +/*
> + * real interrupt handler
> + */
> +static irqreturn_t softing_irq_thread(int irq, void *dev_id)
> +{
> +	struct softing *card = (struct softing *)dev_id;
> +	struct net_device *netdev;
> +	struct softing_priv *priv;
> +	int j, offset, work_done;
> +
> +	work_done = 0;
> +	spin_lock_bh(&card->spin);
> +	while (softing_handle_1(card) > 0) {
> +		++card->irq.svc_count;
> +		++work_done;
> +	}
> +	spin_unlock_bh(&card->spin);
> +	/* resume tx queue's */
> +	offset = card->tx.last_bus;
> +	for (j = 0; j < ARRAY_SIZE(card->net); ++j) {
> +		if (card->tx.pending >= TXMAX)
> +			break;
> +		netdev = card->net[(j + offset + 1) % card->pdat->nbus];
> +		if (!netdev)
> +			continue;
> +		priv = netdev_priv(netdev);
> +		if (!canif_is_active(netdev))
> +			/* it makes no sense to wake dead busses */
> +			continue;
> +		if (priv->tx.pending >= TX_ECHO_SKB_MAX)
> +			continue;
> +		++work_done;
> +		netif_wake_queue(netdev);
> +	}
> +	return work_done ? IRQ_HANDLED : IRQ_NONE;
> +}
> +
> +/*
> + * interrupt routines:
> + * schedule the 'real interrupt handler'
> + */
> +static
> +irqreturn_t softing_irq_v2(int irq, void *dev_id)
> +{
> +	struct softing *card = (struct softing *)dev_id;
> +	uint8_t ir;
> +
> +	ir = ioread8(&card->dpram[DPRAM_V2_IRQ_TOHOST]);
> +	iowrite8(0, &card->dpram[DPRAM_V2_IRQ_TOHOST]);
> +	return (1 == ir) ? IRQ_WAKE_THREAD : IRQ_NONE;
> +}
> +
> +static
> +irqreturn_t softing_irq_v1(int irq, void *dev_id)

See general comments.

> +{
> +	struct softing *card = (struct softing *)dev_id;
> +	uint8_t ir;
> +
> +	ir = ioread8(&card->dpram[DPRAM_IRQ_TOHOST]);
> +	iowrite8(0, &card->dpram[DPRAM_IRQ_TOHOST]);
> +	return ir ? IRQ_WAKE_THREAD : IRQ_NONE;
> +}
> +
> +/*
> + * netdev/candev inter-operability
> + */
> +static int softing_netdev_open(struct net_device *ndev)
> +{
> +	int ret;
> +
> +	/* check or determine and set bittime */
> +	ret = open_candev(ndev);
> +	if (ret)
> +		goto failed;
> +	ret = softing_startstop(ndev, 1);
> +	if (ret)
> +		goto failed;
> +	return 0;
> +failed:
> +	return ret;

Do you really need that label?

> +}
> +
> +static int softing_netdev_stop(struct net_device *ndev)
> +{
> +	int ret;
> +
> +	netif_stop_queue(ndev);
> +
> +	/* softing cycle does close_candev() */
> +	ret = softing_startstop(ndev, 0);
> +	return ret;
> +}
> +
> +static int softing_candev_set_mode(struct net_device *ndev, enum can_mode mode)
> +{
> +	int ret;
> +
> +	switch (mode) {
> +	case CAN_MODE_START:
> +		/* softing_startstop does close_candev() */
> +		ret = softing_startstop(ndev, 1);
> +		return ret;
> +	case CAN_MODE_STOP:
> +	case CAN_MODE_SLEEP:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +/*
> + * Softing device management helpers
> + */
> +int softing_enable_irq(struct softing *card, int enable)
> +{
> +	int ret;

Empty line, please.

> +	if (!enable) {
> +		if (card->irq.requested && card->irq.nr) {
> +			free_irq(card->irq.nr, card);
> +			card->irq.requested = 0;
> +		}
> +		return 0;
> +	}
> +	if (!card->irq.requested && (card->irq.nr)) {
> +		ret = request_threaded_irq(card->irq.nr,
> +				(card->pdat->generation >= 2)
> +					? softing_irq_v2 : softing_irq_v1,
> +				softing_irq_thread, IRQF_SHARED,
> +				dev_name(&card->pdev->dev), card);
> +		if (ret) {
> +			dev_alert(&card->pdev->dev,
> +					"request_threaded_irq(%u) failed\n",
> +					card->irq.nr);
> +			return ret;
> +		}
> +		card->irq.requested = 1;
> +	}
> +	return 0;
> +}
> +
> +static void softing_card_shutdown(struct softing *card)
> +{
> +	int fw_up = 0;

Empty line, please.

> +	dev_dbg(&card->pdev->dev, "%s()\n", __func__);

Please reduce the debugging output to a few useful messages (for the
final user).

> +	if (mutex_lock_interruptible(&card->fw.lock))
> +		/* return -ERESTARTSYS*/;

What is the "if" then good for? Do you want to handle the return code?

> +	fw_up = card->fw.up;
> +	card->fw.up = 0;
> +
> +	if (card->irq.requested && card->irq.nr) {
> +		free_irq(card->irq.nr, card);
> +		card->irq.requested = 0;
> +	}
> +	if (fw_up) {
> +		if (card->pdat->enable_irq)
> +			card->pdat->enable_irq(card->pdev, 0);
> +		softing_set_reset_dpram(card);
> +		if (card->pdat->reset)
> +			card->pdat->reset(card->pdev, 1);
> +	}
> +	mutex_unlock(&card->fw.lock);
> +}
> +
> +static int softing_card_boot(struct softing *card)
> +{
> +	int j;
> +	static const uint8_t stream[] = {
> +		0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, };
> +	unsigned char back[sizeof(stream)];

Empty line please.

> +	dev_dbg(&card->pdev->dev, "%s()\n", __func__);

See comment above.

> +	if (mutex_lock_interruptible(&card->fw.lock))
> +		return -ERESTARTSYS;
> +	if (card->fw.up) {
> +		mutex_unlock(&card->fw.lock);
> +		return 0;
> +	}
> +	/* reset board */
> +	if (card->pdat->enable_irq)
> +		card->pdat->enable_irq(card->pdev, 1);
> +	/* boot card */
> +	softing_set_reset_dpram(card);
> +	if (card->pdat->reset)
> +		card->pdat->reset(card->pdev, 1);
> +	for (j = 0; (j + sizeof(stream)) < card->dpram_size;
> +			j += sizeof(stream)) {
> +
> +		memcpy_toio(&card->dpram[j], stream, sizeof(stream));
> +		/* flush IO cache */
> +		mb();
> +		memcpy_fromio(back, &card->dpram[j], sizeof(stream));
> +
> +		if (!memcmp(back, stream, sizeof(stream)))
> +			continue;
> +		/* memory is not equal */
> +		dev_alert(&card->pdev->dev, "dpram failed at 0x%04x\n", j);
> +		goto open_failed;
> +	}
> +	wmb();
> +	/* load boot firmware */
> +	if (softing_load_fw(card->pdat->boot.fw, card, card->dpram,
> +				 card->dpram_size,
> +				 card->pdat->boot.offs -
> +				 card->pdat->boot.addr))
> +		goto open_failed;
> +	/* load loader firmware */
> +	if (softing_load_fw(card->pdat->load.fw, card, card->dpram,
> +				 card->dpram_size,
> +				 card->pdat->load.offs -
> +				 card->pdat->load.addr))
> +		goto open_failed;
> +
> +	if (card->pdat->reset)
> +		card->pdat->reset(card->pdev, 0);
> +	softing_clr_reset_dpram(card);
> +	if (softing_bootloader_command(card, 0, "card boot"))
> +		goto open_failed;
> +	if (softing_load_app_fw(card->pdat->app.fw, card))
> +		goto open_failed;
> +	/* reset chip */
> +	iowrite8(0, &card->dpram[DPRAM_RESET_RX_FIFO]);
> +	iowrite8(0, &card->dpram[DPRAM_RESET_RX_FIFO+1]);
> +	iowrite8(1, &card->dpram[DPRAM_RESET]);
> +	iowrite8(0, &card->dpram[DPRAM_RESET+1]);
> +	/* sync */
> +	if (softing_fct_cmd(card, 99, 0x55, "sync-a"))
> +		goto open_failed;
> +	if (softing_fct_cmd(card, 99, 0xaa, "sync-a"))
> +		goto open_failed;
> +	/* reset chip */
> +	if (softing_fct_cmd(card, 0, 0, "reset_chip"))
> +		goto open_failed;
> +	/* get_serial */
> +	if (softing_fct_cmd(card, 43, 0, "get_serial_number"))
> +		goto open_failed;
> +	card->id.serial = ioread32(&card->dpram[DPRAM_FCT_PARAM]);
> +	/* get_version */
> +	if (softing_fct_cmd(card, 12, 0, "get_version"))
> +		goto open_failed;
> +	card->id.fw = ioread16(&card->dpram[DPRAM_FCT_PARAM + 2]);
> +	card->id.hw = ioread16(&card->dpram[DPRAM_FCT_PARAM + 4]);
> +	card->id.lic = ioread16(&card->dpram[DPRAM_FCT_PARAM + 6]);
> +	card->id.chip[0] = ioread16(&card->dpram[DPRAM_FCT_PARAM + 8]);
> +	card->id.chip[1] = ioread16(&card->dpram[DPRAM_FCT_PARAM + 10]);
> +
> +	dev_info(&card->pdev->dev, "card booted, type %s, "
> +			"serial %u, fw %u, hw %u, lic %u, chip (%u,%u)\n",
> +		  card->pdat->name, card->id.serial, card->id.fw, card->id.hw,
> +		  card->id.lic, card->id.chip[0], card->id.chip[1]);
> +
> +	card->fw.up = 1;
> +	mutex_unlock(&card->fw.lock);
> +	return 0;
> +open_failed:
> +	card->fw.up = 0;
> +	if (card->pdat->enable_irq)
> +		card->pdat->enable_irq(card->pdev, 0);
> +	softing_set_reset_dpram(card);
> +	if (card->pdat->reset)
> +		card->pdat->reset(card->pdev, 1);
> +	mutex_unlock(&card->fw.lock);
> +	return -EIO;
> +}
> +
> +/*
> + * netdev sysfs
> + */
> +static ssize_t show_channel(struct device *dev
> +		, struct device_attribute *attr, char *buf)

See general comments. Here and for the following function declarations.

> +{
> +	struct net_device *ndev = to_net_dev(dev);
> +	struct softing_priv *priv = netdev2softing(ndev);
> +	return sprintf(buf, "%i\n", priv->index);
> +}
> +
> +static ssize_t show_chip(struct device *dev
> +		, struct device_attribute *attr, char *buf)
> +{
> +	struct net_device *ndev = to_net_dev(dev);
> +	struct softing_priv *priv = netdev2softing(ndev);
> +	return sprintf(buf, "%i\n", priv->chip);
> +}
> +
> +static ssize_t show_output(struct device *dev
> +		, struct device_attribute *attr, char *buf)
> +{
> +	struct net_device *ndev = to_net_dev(dev);
> +	struct softing_priv *priv = netdev2softing(ndev);
> +	return sprintf(buf, "0x%02x\n", priv->output);
> +}
> +
> +static ssize_t store_output(struct device *dev
> +		, struct device_attribute *attr
> +		, const char *buf, size_t count)
> +{
> +	struct net_device *ndev = to_net_dev(dev);
> +	struct softing_priv *priv = netdev2softing(ndev);
> +	struct softing *card = priv->card;
> +	unsigned long val;
> +	int ret;
> +
> +	ret = strict_strtoul(buf, 0, &val);
> +	if (ret < 0)
> +		return ret;
> +	val &= 0xFF;
> +
> +	ret = mutex_lock_interruptible(&card->fw.lock);
> +	if (ret)
> +		return -ERESTARTSYS;
> +	if (netif_running(ndev)) {
> +		mutex_unlock(&card->fw.lock);
> +		return -EBUSY;
> +	}
> +	priv->output = val;
> +	mutex_unlock(&card->fw.lock);
> +	return count;
> +}
> +
> +static const DEVICE_ATTR(channel, S_IRUGO, show_channel, NULL);
> +static const DEVICE_ATTR(chip, S_IRUGO, show_chip, NULL);
> +static const DEVICE_ATTR(output, S_IRUGO | S_IWUSR, show_output, store_output);
> +
> +static const struct attribute *const netdev_sysfs_attrs[] = {
> +	&dev_attr_channel.attr,
> +	&dev_attr_chip.attr,
> +	&dev_attr_output.attr,
> +	NULL,
> +};
> +static const struct attribute_group netdev_sysfs_group = {
> +	.name  = NULL,
> +	.attrs = (struct attribute **)netdev_sysfs_attrs,
> +};
> +
> +static const struct net_device_ops softing_netdev_ops = {
> +	.ndo_open = softing_netdev_open,
> +	.ndo_stop = softing_netdev_stop,
> +	.ndo_start_xmit	= softing_netdev_start_xmit,
> +};
> +
> +static const struct can_bittiming_const softing_btr_const = {
> +	.tseg1_min = 1,
> +	.tseg1_max = 16,
> +	.tseg2_min = 1,
> +	.tseg2_max = 8,
> +	.sjw_max = 4, /* overruled */
> +	.brp_min = 1,
> +	.brp_max = 32, /* overruled */
> +	.brp_inc = 1,
> +};
> +
> +
> +static struct net_device *softing_netdev_create(
> +		struct softing *card, u16 chip_id)
> +{
> +	struct net_device *netdev;
> +	struct softing_priv *priv;
> +
> +	netdev = alloc_candev(sizeof(*priv), TX_ECHO_SKB_MAX);
> +	if (!netdev) {
> +		dev_alert(&card->pdev->dev, "alloc_candev failed\n");
> +		return NULL;
> +	}
> +	priv = netdev_priv(netdev);
> +	priv->netdev = netdev;
> +	priv->card = card;
> +	memcpy(&priv->btr_const, &softing_btr_const, sizeof(priv->btr_const));
> +	priv->btr_const.brp_max = card->pdat->max_brp;
> +	priv->btr_const.sjw_max = card->pdat->max_sjw;
> +	priv->can.bittiming_const = &priv->btr_const;
> +	priv->can.clock.freq = 8000000;

Another magic constant.

> +	priv->chip = chip_id;
> +	priv->output = softing_default_output(netdev);
> +	SET_NETDEV_DEV(netdev, &card->pdev->dev);
> +
> +	netdev->flags |= IFF_ECHO;
> +	netdev->netdev_ops	= &softing_netdev_ops;
> +	priv->can.do_set_mode	= softing_candev_set_mode;

See general comments.

> +	priv->can.ctrlmode_supported =
> +		CAN_CTRLMODE_3_SAMPLES;/* | CAN_CTRLMODE_BERR_REPORTING */;

Hm, any chance to support CAN_CTRLMODE_BERR_REPORTING? If not, please
remove the comment.

> +	return netdev;
> +}
> +
> +static int softing_netdev_register(struct net_device *netdev)
> +{
> +	int ret;
> +
> +	/*
> +	 * provide bus-specific sysfs attributes _during_ the uevent
> +	 */
> +	netdev->sysfs_groups[0] = &netdev_sysfs_group;
> +	ret = register_candev(netdev);
> +	if (ret) {
> +		dev_alert(&netdev->dev, "register failed\n");
> +		return ret;
> +	}
> +	return 0;
> +}
> +
> +static void softing_netdev_cleanup(struct net_device *netdev)
> +{
> +	unregister_candev(netdev);
> +	free_candev(netdev);
> +}
> +
> +/*
> + * sysfs for Platform device
> + */
> +#define DEV_ATTR_RO(name, member) \
> +static ssize_t show_##name(struct device *dev, \
> +		struct device_attribute *attr, char *buf) \
> +{ \
> +	struct softing *card = platform_get_drvdata(to_platform_device(dev)); \
> +	return sprintf(buf, "%u\n", card->member); \
> +} \
> +static DEVICE_ATTR(name, 0444, show_##name, NULL)
> +
> +DEV_ATTR_RO(serial	, id.serial);
> +DEV_ATTR_RO(firmware	, id.fw);
> +DEV_ATTR_RO(hardware	, id.hw);
> +DEV_ATTR_RO(license	, id.lic);
> +DEV_ATTR_RO(freq	, id.freq);
> +DEV_ATTR_RO(txpending	, tx.pending);
> +
> +static struct attribute *softing_pdev_attrs[] = {
> +	&dev_attr_serial.attr,
> +	&dev_attr_firmware.attr,
> +	&dev_attr_hardware.attr,
> +	&dev_attr_license.attr,
> +	&dev_attr_freq.attr,
> +	&dev_attr_txpending.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group softing_pdev_group = {
> +	.attrs = softing_pdev_attrs,
> +};
> +
> +/*
> + * platform driver
> + */
> +static int softing_pdev_remove(struct platform_device *pdev)
> +{
> +	struct softing *card = platform_get_drvdata(pdev);
> +	int j;
> +
> +	/* first, disable card*/
> +	softing_card_shutdown(card);
> +
> +	for (j = 0; j < ARRAY_SIZE(card->net); ++j) {
> +		if (!card->net[j])
> +			continue;
> +		softing_netdev_cleanup(card->net[j]);
> +		card->net[j] = NULL;
> +	}
> +	sysfs_remove_group(&pdev->dev.kobj, &softing_pdev_group);
> +
> +	iounmap(card->dpram);
> +	kfree(card);
> +	return 0;
> +}
> +
> +static int softing_pdev_probe(struct platform_device *pdev)
> +{
> +	const struct softing_platform_data *pdat = pdev->dev.platform_data;
> +	struct softing *card;
> +	struct net_device *netdev;
> +	struct softing_priv *priv;
> +	struct resource *pres;
> +	int ret;
> +	int j;
> +
> +	if (!pdat) {
> +		dev_warn(&pdev->dev, "no platform data\n");
> +		return -EINVAL;
> +	}
> +	if (pdat->nbus > ARRAY_SIZE(card->net)) {
> +		dev_warn(&pdev->dev, "%u nets??\n", pdat->nbus);
> +		return -EINVAL;
> +	}
> +
> +	card = kzalloc(sizeof(*card), GFP_KERNEL);
> +	if (!card)
> +		return -ENOMEM;
> +	card->pdat = pdat;
> +	card->pdev = pdev;
> +	platform_set_drvdata(pdev, card);
> +	/* try_module_get(THIS_MODULE); */
> +	mutex_init(&card->fw.lock);
> +	spin_lock_init(&card->spin);
> +
> +	ret = -EINVAL;
> +	pres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!pres)
> +		goto platform_resource_failed;;
> +	card->dpram_phys = pres->start;
> +	card->dpram_size = pres->end - pres->start + 1;
> +	card->dpram = ioremap_nocache(card->dpram_phys, card->dpram_size);
> +	if (!card->dpram) {
> +		dev_alert(&card->pdev->dev, "dpram ioremap failed\n");
> +		goto ioremap_failed;
> +	}
> +
> +	pres = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (pres)
> +		card->irq.nr = pres->start;
> +
> +	/* reset card */
> +	ret = -EIO;
> +	if (softing_card_boot(card)) {
> +		dev_alert(&pdev->dev, "failed to boot\n");
> +		goto boot_failed;
> +	}
> +
> +	/* only now, the chip's are known */
> +	card->id.freq = card->pdat->freq * 1000000UL;

It would be more flexible to specific the frequency in Hz!? Or use a
more logical member name, frey_mhz, at least.

> +
> +	ret = sysfs_create_group(&pdev->dev.kobj, &softing_pdev_group);
> +	if (ret < 0) {
> +		dev_alert(&card->pdev->dev, "sysfs failed\n");
> +		goto sysfs_failed;
> +	}
> +
> +	ret = -ENOMEM;
> +	for (j = 0; j < ARRAY_SIZE(card->net); ++j) {
> +		card->net[j] = netdev =
> +			softing_netdev_create(card, card->id.chip[j]);
> +		if (!netdev) {
> +			dev_alert(&pdev->dev, "failed to make can[%i]", j);
> +			goto netdev_failed;
> +		}
> +		priv = netdev_priv(card->net[j]);
> +		priv->index = j;
> +		ret = softing_netdev_register(netdev);
> +		if (ret) {
> +			free_candev(netdev);
> +			card->net[j] = NULL;
> +			dev_alert(&card->pdev->dev,
> +				"failed to register can[%i]\n", j);
> +			goto netdev_failed;
> +		}
> +	}
> +	dev_info(&card->pdev->dev, "card initialised\n");
> +	return 0;
> +
> +netdev_failed:
> +	for (j = 0; j < ARRAY_SIZE(card->net); ++j) {
> +		if (!card->net[j])
> +			continue;
> +		softing_netdev_cleanup(card->net[j]);
> +	}
> +	sysfs_remove_group(&pdev->dev.kobj, &softing_pdev_group);
> +sysfs_failed:
> +	softing_card_shutdown(card);
> +boot_failed:
> +	iounmap(card->dpram);
> +ioremap_failed:
> +platform_resource_failed:
> +	kfree(card);
> +	return ret;
> +}
> +
> +static struct platform_driver softing_driver = {
> +	.driver = {
> +		.name = "softing",
> +		.owner = THIS_MODULE,
> +	},
> +	.probe = softing_pdev_probe,
> +	.remove = softing_pdev_remove,
> +};

I'm missing the use of __devinit and friends.


> +MODULE_ALIAS("platform:softing");
> +
> +static int __init softing_start(void)
> +{
> +	return platform_driver_register(&softing_driver);
> +}
> +
> +static void __exit softing_stop(void)
> +{
> +	platform_driver_unregister(&softing_driver);
> +}
> +
> +module_init(softing_start);
> +module_exit(softing_stop);
> +
> +MODULE_DESCRIPTION("Softing DPRAM CAN driver");
> +MODULE_AUTHOR("Kurt Van Dijck <kurt.van.dijck-/BeEPy95v10@public.gmane.org>");
> +MODULE_LICENSE("GPL");

GPL v2 ?

> diff --git a/drivers/net/can/softing/softing_platform.h b/drivers/net/can/softing/softing_platform.h
> new file mode 100644
> index 0000000..678df36
> --- /dev/null
> +++ b/drivers/net/can/softing/softing_platform.h
> @@ -0,0 +1,38 @@
> +
> +#include <linux/platform_device.h>
> +
> +#ifndef _SOFTING_DEVICE_H_
> +#define _SOFTING_DEVICE_H_
> +
> +/* softing firmware directory prefix */
> +#define fw_dir "softing-4.6/"
> +
> +struct softing_platform_data {
> +	unsigned int manf;
> +	unsigned int prod;
> +	/* generation
> +	 * 1st with NEC or SJA1000
> +	 * 8bit, exclusive interrupt, ...
> +	 * 2nd only SJA1000
> +	 * 16bit, shared interrupt
> +	 */

Please the usual multiline comment style.

> +	int generation;
> +	int nbus; /* # busses on device */
> +	unsigned int freq; /* crystal in MHz */
> +	unsigned int max_brp;
> +	unsigned int max_sjw;
> +	unsigned long dpram_size;
> +	char name[32];
> +	struct {
> +		unsigned long offs;
> +		unsigned long addr;
> +		const char *fw;
> +	} boot, load, app;
> +	/* reset() function, bring pdev in or out of reset, depending on
> +	   value */
> +	int (*reset)(struct platform_device *pdev, int value);
> +	int (*enable_irq)(struct platform_device *pdev, int value);
> +};
> +
> +#endif
> +

Wolfgang.