Patchwork [4/8] can: Driver for the SJA1000 CAN controller

login
register
mail settings
Submitter Wolfgang Grandegger
Date Feb. 19, 2009, 7:01 p.m.
Message ID <1235070082-7069-5-git-send-email-wg@grandegger.com>
Download mbox | patch
Permalink /patch/23439/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Wolfgang Grandegger - Feb. 19, 2009, 7:01 p.m.
This patch adds the generic Socket-CAN driver for the Philips SJA1000
full CAN controller.

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
Signed-off-by: Oliver Hartkopp <oliver.hartkopp@volkswagen.de>
---
 drivers/net/can/Kconfig           |   11 +
 drivers/net/can/Makefile          |    2 +
 drivers/net/can/sja1000/Makefile  |    7 +
 drivers/net/can/sja1000/sja1000.c |  681 +++++++++++++++++++++++++++++++++++++
 drivers/net/can/sja1000/sja1000.h |  177 ++++++++++
 5 files changed, 878 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/can/sja1000/Makefile
 create mode 100644 drivers/net/can/sja1000/sja1000.c
 create mode 100644 drivers/net/can/sja1000/sja1000.h
Jonathan Corbet - Feb. 20, 2009, 12:14 a.m.
I won't be able to look at all of these...

> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index d609895..78a412b 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -35,6 +35,17 @@ config CAN_CALC_BITTIMING
>  	  files "tq", "prop_seg", "phase_seg1", "phase_seg2" and "sjw".
>  	  If unsure, say Y.
>  
> +config CAN_SJA1000
> +	depends on CAN_DEV
> +	tristate "Philips SJA1000"
> +	---help---
> +	  The SJA1000 is one of the top CAN controllers out there. As it
> +	  has a multiplexed interface it fits directly to 8051
> +	  microcontrollers or into the PC I/O port space. The SJA1000
> +	  is a full CAN controller, with shadow registers for RX and TX.
> +	  It can send and receive any kinds of CAN frames (SFF/EFF/RTR)
> +	  with a single (simple) filter setup.

This sounds more like advertising text.  But what people need to know is
whether they should enable it or not.

[...]

> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
> new file mode 100644
> index 0000000..6fe516d
> --- /dev/null
> +++ b/drivers/net/can/sja1000/sja1000.c
> @@ -0,0 +1,681 @@

[...]

> +static void sja1000_start(struct net_device *dev)
> +{
> +	struct sja1000_priv *priv = netdev_priv(dev);
> +
> +	/* leave reset mode */
> +	if (priv->can.state != CAN_STATE_STOPPED)
> +		set_reset_mode(dev);
> +
> +	/* Clear error counters and error code capture */
> +	priv->write_reg(dev, REG_TXERR, 0x0);
> +	priv->write_reg(dev, REG_RXERR, 0x0);
> +	priv->read_reg(dev, REG_ECC);
> +
> +	/* leave reset mode */
> +	set_normal_mode(dev);
> +}

It's about here that I begin to wonder about locking again.  What is
preventing concurrent access to the device?

[...]

> +static int sja1000_get_state(struct net_device *dev, enum can_state *state)
> +{
> +	struct sja1000_priv *priv = netdev_priv(dev);
> +	u8 status;
> +
> +	/* FIXME: inspecting the status register to get the current state
> +	 * is not really necessary, because state changes are handled by
> +	 * in the ISR and the variable priv->can.state gets updated. The
> +	 * CAN devicde interface needs fixing!
> +	 */
> +
> +	spin_lock_irq(&priv->can.irq_lock);

Interesting, here we do have a lock.  What is it protecting?  *state??  It
can't be the device registers, since they are accessed without locks in
many other places.

> +	if (priv->can.state == CAN_STATE_STOPPED) {
> +		*state =  CAN_STATE_STOPPED;
> +	} else {
> +		status = priv->read_reg(dev, REG_SR);
> +		if (status & SR_BS)
> +			*state = CAN_STATE_BUS_OFF;
> +		else if (status & SR_ES) {
> +			if (priv->read_reg(dev, REG_TXERR) > 127 ||
> +			    priv->read_reg(dev, REG_RXERR) > 127)
> +				*state = CAN_STATE_BUS_PASSIVE;
> +			else
> +				*state = CAN_STATE_BUS_WARNING;
> +		} else
> +			*state = CAN_STATE_ACTIVE;
> +	}
> +	/* Check state */
> +	if (*state != priv->can.state)
> +		dev_err(ND2D(dev),
> +			"Oops, state mismatch: hard %d != soft %d\n",
> +			*state, priv->can.state);
> +	spin_unlock_irq(&priv->can.irq_lock);
> +	return 0;
> +}

[...]

> +/*
> + * transmit a CAN message
> + * message layout in the sk_buff should be like this:
> + * xx xx xx xx	 ff	 ll   00 11 22 33 44 55 66 77
> + * [  can-id ] [flags] [len] [can data (up to 8 bytes]
> + */
> +static int sja1000_start_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct sja1000_priv *priv = netdev_priv(dev);
> +	struct net_device_stats *stats = &dev->stats;
> +	struct can_frame *cf = (struct can_frame *)skb->data;
> +	uint8_t fi;
> +	uint8_t dlc;
> +	canid_t id;
> +	uint8_t dreg;
> +	int i;
> +
> +	netif_stop_queue(dev);
> +
> +	fi = dlc = cf->can_dlc;
> +	id = cf->can_id;
> +
> +	if (id & CAN_RTR_FLAG)
> +		fi |= FI_RTR;
> +
> +	if (id & CAN_EFF_FLAG) {
> +		fi |= FI_FF;
> +		dreg = EFF_BUF;
> +		priv->write_reg(dev, REG_FI, fi);
> +		priv->write_reg(dev, REG_ID1, (id & 0x1fe00000) >> (5 + 16));
> +		priv->write_reg(dev, REG_ID2, (id & 0x001fe000) >> (5 + 8));
> +		priv->write_reg(dev, REG_ID3, (id & 0x00001fe0) >> 5);
> +		priv->write_reg(dev, REG_ID4, (id & 0x0000001f) << 3);
> +	} else {
> +		dreg = SFF_BUF;
> +		priv->write_reg(dev, REG_FI, fi);
> +		priv->write_reg(dev, REG_ID1, (id & 0x000007f8) >> 3);
> +		priv->write_reg(dev, REG_ID2, (id & 0x00000007) << 5);
> +	}
> +
> +	for (i = 0; i < dlc; i++)
> +		priv->write_reg(dev, dreg++, cf->data[i]);
> +
> +	stats->tx_bytes += dlc;
> +	dev->trans_start = jiffies;
> +
> +	can_put_echo_skb(skb, dev, 0);

Hmm...looking back at can_put_echo_skb(), I see that it expects dev->priv
to point to a struct can_priv.  Here, though, we see it pointing to a
struct sja1000_prive instead.  I begin to suspect dangerous trickery going
on behind our backs...

> +
> +	priv->write_reg(dev, REG_CMR, CMD_TR);
> +
> +	return 0;
> +}

[...]

> +irqreturn_t sja1000_interrupt(int irq, void *dev_id)
> +{
> +	struct net_device *dev = (struct net_device *)dev_id;
> +	struct sja1000_priv *priv = netdev_priv(dev);
> +	struct net_device_stats *stats = &dev->stats;
> +	uint8_t isrc, status;
> +	int n = 0;
> +
> +	/* Shared interrupts and IRQ off? */
> +	if (priv->read_reg(dev, REG_IER) == IRQ_OFF)
> +		return IRQ_NONE;
> +
> +	if (priv->pre_irq)
> +		priv->pre_irq(dev);
> +
> +	while ((isrc = priv->read_reg(dev, REG_IR)) && (n < SJA1000_MAX_IRQ)) {
> +		n++;
> +		status = priv->read_reg(dev, REG_SR);
> +
> +		if (isrc & IRQ_WUI) {
> +			/* wake-up interrupt */
> +			priv->can.can_stats.wakeup++;
> +		}
> +		if (isrc & IRQ_TI) {
> +			/* transmission complete interrupt */
> +			stats->tx_packets++;
> +			can_get_echo_skb(dev, 0);
> +			netif_wake_queue(dev);
> +		}
> +		if (isrc & IRQ_RI) {
> +			/* receive interrupt */
> +			while (status & SR_RBS) {
> +				sja1000_rx(dev);
> +				status = priv->read_reg(dev, REG_SR);
> +			}
> +		}
> +		if (isrc & (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {
> +			/* error interrupt */
> +			if (sja1000_err(dev, isrc, status))
> +				break;
> +		}
> +	}
> +
> +	if (priv->post_irq)
> +		priv->post_irq(dev);
> +
> +	if (n >= SJA1000_MAX_IRQ)
> +		dev_dbg(ND2D(dev), "%d messages handled in ISR", n);
> +
> +	return (n) ? IRQ_HANDLED : IRQ_NONE;
> +}
> +EXPORT_SYMBOL_GPL(sja1000_interrupt);

You used spin_lock_irq(&irq_lock) above, but the interrupt handler doesn't
take that lock?  So (above) you could acquire the lock while the interrupt
handler is running?  I hate to keep asking this question, but...what does
that lock protect?

[...]

> +static int sja1000_close(struct net_device *dev)
> +{
> +	struct sja1000_priv *priv = netdev_priv(dev);
> +
> +	set_reset_mode(dev);
> +	netif_stop_queue(dev);
> +	priv->open_time = 0;
> +	can_close_cleanup(dev);

What happens if your device interrupts right here?  Maybe you should
disconnect the handler earlier?

> +	if (!(priv->flags & SJA1000_CUSTOM_IRQ_HANDLER))
> +		free_irq(dev->irq, (void *)dev);
> +
> +	return 0;
> +}

[...]

> +int register_sja1000dev(struct net_device *dev)
> +{
> +	struct sja1000_priv *priv = netdev_priv(dev);
> +	int err;
> +
> +	if (!sja1000_probe_chip(dev))
> +		return -ENODEV;
> +
> +	dev->flags |= IFF_ECHO;	/* we support local echo */
> +
> +	dev->netdev_ops = &sja1000_netdev_ops;
> +
> +	priv->can.bittiming_const = &sja1000_bittiming_const;
> +	priv->can.do_set_bittiming = sja1000_set_bittiming;
> +	priv->can.do_get_state = sja1000_get_state;
> +	priv->can.do_set_mode = sja1000_set_mode;
> +	priv->dev = dev;
> +
> +	err = register_candev(dev);

Here we've registered our device with the CAN and networking core...

> +	if (err) {
> +		printk(KERN_INFO
> +		       "%s: registering netdev failed\n", DRV_NAME);
> +		free_netdev(dev);
> +		return err;
> +	}
> +
> +	set_reset_mode(dev);
> +	chipset_init(dev);

...but only here have we gotten it ready to operate.  If the higher levels
decide to do something with your device in the mean time, will the right
thing happen?

> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(register_sja1000dev);

[...]

> diff --git a/drivers/net/can/sja1000/sja1000.h b/drivers/net/can/sja1000/sja1000.h
> new file mode 100644
> index 0000000..60d4cd6
> --- /dev/null
> +++ b/drivers/net/can/sja1000/sja1000.h

[...]

> +/*
> + * SJA1000 private data structure
> + */
> +struct sja1000_priv {
> +	struct can_priv can;	/* must be the first member! */

AHA!  I knew it!

This kind of pointer trickery is fragile and dangerous, please don't do
it.  Much better would be something like:

	dev->priv = &dev_specific_priv->can;

Then the higher layers know they have a proper struct can_priv pointer.
Then you can use container_of() at this level to get the outer structure
pointer.  Much more robust and in line with normal kernel coding idiom.

> +	long open_time;
> +	struct sk_buff *echo_skb;
> +
> +	u8 (*read_reg) (struct net_device *dev, int reg);
> +	void (*write_reg) (struct net_device *dev, int reg, u8 val);
> +	void (*pre_irq) (struct net_device *dev);
> +	void (*post_irq) (struct net_device *dev);
> +
> +	void *priv;		/* for board-specific data */
> +	struct net_device *dev;
> +
> +	u8 ocr;
> +	u8 cdr;
> +	u32 flags;
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Feb. 20, 2009, 8:46 a.m.
From: Jonathan Corbet <corbet@lwn.net>
Date: Thu, 19 Feb 2009 17:14:40 -0700

> I won't be able to look at all of these...
> 
> > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> > index d609895..78a412b 100644
> > --- a/drivers/net/can/Kconfig
> > +++ b/drivers/net/can/Kconfig
> > @@ -35,6 +35,17 @@ config CAN_CALC_BITTIMING
> >  	  files "tq", "prop_seg", "phase_seg1", "phase_seg2" and "sjw".
> >  	  If unsure, say Y.
> >  
> > +config CAN_SJA1000
> > +	depends on CAN_DEV
> > +	tristate "Philips SJA1000"
> > +	---help---
> > +	  The SJA1000 is one of the top CAN controllers out there. As it
> > +	  has a multiplexed interface it fits directly to 8051
> > +	  microcontrollers or into the PC I/O port space. The SJA1000
> > +	  is a full CAN controller, with shadow registers for RX and TX.
> > +	  It can send and receive any kinds of CAN frames (SFF/EFF/RTR)
> > +	  with a single (simple) filter setup.
> 
> This sounds more like advertising text.  But what people need to know is
> whether they should enable it or not.

Totally agreed, this Kconfig help text is inappropriate
at best.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfgang Grandegger - Feb. 20, 2009, 9:35 a.m.
Hi Jonathan,

Jonathan Corbet wrote:
> I won't be able to look at all of these...

OK, I will check the others for similar issues.

>> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
>> index d609895..78a412b 100644
>> --- a/drivers/net/can/Kconfig
>> +++ b/drivers/net/can/Kconfig
>> @@ -35,6 +35,17 @@ config CAN_CALC_BITTIMING
>>  	  files "tq", "prop_seg", "phase_seg1", "phase_seg2" and "sjw".
>>  	  If unsure, say Y.
>>  
>> +config CAN_SJA1000
>> +	depends on CAN_DEV
>> +	tristate "Philips SJA1000"
>> +	---help---
>> +	  The SJA1000 is one of the top CAN controllers out there. As it
>> +	  has a multiplexed interface it fits directly to 8051
>> +	  microcontrollers or into the PC I/O port space. The SJA1000
>> +	  is a full CAN controller, with shadow registers for RX and TX.
>> +	  It can send and receive any kinds of CAN frames (SFF/EFF/RTR)
>> +	  with a single (simple) filter setup.
> 
> This sounds more like advertising text.  But what people need to know is
> whether they should enable it or not.

Yes,

  "Enables support for the SJA1000 CAN controller from Philips or NXP"

should be sufficient.

> [...]
> 
>> diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
>> new file mode 100644
>> index 0000000..6fe516d
>> --- /dev/null
>> +++ b/drivers/net/can/sja1000/sja1000.c
>> @@ -0,0 +1,681 @@
> 
> [...]
> 
>> +static void sja1000_start(struct net_device *dev)
>> +{
>> +	struct sja1000_priv *priv = netdev_priv(dev);
>> +
>> +	/* leave reset mode */
>> +	if (priv->can.state != CAN_STATE_STOPPED)
>> +		set_reset_mode(dev);
>> +
>> +	/* Clear error counters and error code capture */
>> +	priv->write_reg(dev, REG_TXERR, 0x0);
>> +	priv->write_reg(dev, REG_RXERR, 0x0);
>> +	priv->read_reg(dev, REG_ECC);
>> +
>> +	/* leave reset mode */
>> +	set_normal_mode(dev);
>> +}
> 
> It's about here that I begin to wonder about locking again.  What is
> preventing concurrent access to the device?

The device is usually stopped when this function is called but I will
check for corner cases due to the restart feature.

> [...]
> 
>> +static int sja1000_get_state(struct net_device *dev, enum can_state *state)
>> +{
>> +	struct sja1000_priv *priv = netdev_priv(dev);
>> +	u8 status;
>> +
>> +	/* FIXME: inspecting the status register to get the current state
>> +	 * is not really necessary, because state changes are handled by
>> +	 * in the ISR and the variable priv->can.state gets updated. The
>> +	 * CAN devicde interface needs fixing!
>> +	 */
>> +
>> +	spin_lock_irq(&priv->can.irq_lock);
> 
> Interesting, here we do have a lock.  What is it protecting?  *state??  It
> can't be the device registers, since they are accessed without locks in
> many other places.

This lock is indeed required to protect priv->can.irq_lock not be
changed by the ISR. But it should be a lock private for the SJA1000.

> 
>> +	if (priv->can.state == CAN_STATE_STOPPED) {
>> +		*state =  CAN_STATE_STOPPED;
>> +	} else {
>> +		status = priv->read_reg(dev, REG_SR);
>> +		if (status & SR_BS)
>> +			*state = CAN_STATE_BUS_OFF;
>> +		else if (status & SR_ES) {
>> +			if (priv->read_reg(dev, REG_TXERR) > 127 ||
>> +			    priv->read_reg(dev, REG_RXERR) > 127)
>> +				*state = CAN_STATE_BUS_PASSIVE;
>> +			else
>> +				*state = CAN_STATE_BUS_WARNING;
>> +		} else
>> +			*state = CAN_STATE_ACTIVE;
>> +	}
>> +	/* Check state */
>> +	if (*state != priv->can.state)
>> +		dev_err(ND2D(dev),
>> +			"Oops, state mismatch: hard %d != soft %d\n",
>> +			*state, priv->can.state);
>> +	spin_unlock_irq(&priv->can.irq_lock);
>> +	return 0;
>> +}
> 
> [...]
> 
>> +/*
>> + * transmit a CAN message
>> + * message layout in the sk_buff should be like this:
>> + * xx xx xx xx	 ff	 ll   00 11 22 33 44 55 66 77
>> + * [  can-id ] [flags] [len] [can data (up to 8 bytes]
>> + */
>> +static int sja1000_start_xmit(struct sk_buff *skb, struct net_device *dev)
>> +{
>> +	struct sja1000_priv *priv = netdev_priv(dev);
>> +	struct net_device_stats *stats = &dev->stats;
>> +	struct can_frame *cf = (struct can_frame *)skb->data;
>> +	uint8_t fi;
>> +	uint8_t dlc;
>> +	canid_t id;
>> +	uint8_t dreg;
>> +	int i;
>> +
>> +	netif_stop_queue(dev);
>> +
>> +	fi = dlc = cf->can_dlc;
>> +	id = cf->can_id;
>> +
>> +	if (id & CAN_RTR_FLAG)
>> +		fi |= FI_RTR;
>> +
>> +	if (id & CAN_EFF_FLAG) {
>> +		fi |= FI_FF;
>> +		dreg = EFF_BUF;
>> +		priv->write_reg(dev, REG_FI, fi);
>> +		priv->write_reg(dev, REG_ID1, (id & 0x1fe00000) >> (5 + 16));
>> +		priv->write_reg(dev, REG_ID2, (id & 0x001fe000) >> (5 + 8));
>> +		priv->write_reg(dev, REG_ID3, (id & 0x00001fe0) >> 5);
>> +		priv->write_reg(dev, REG_ID4, (id & 0x0000001f) << 3);
>> +	} else {
>> +		dreg = SFF_BUF;
>> +		priv->write_reg(dev, REG_FI, fi);
>> +		priv->write_reg(dev, REG_ID1, (id & 0x000007f8) >> 3);
>> +		priv->write_reg(dev, REG_ID2, (id & 0x00000007) << 5);
>> +	}
>> +
>> +	for (i = 0; i < dlc; i++)
>> +		priv->write_reg(dev, dreg++, cf->data[i]);
>> +
>> +	stats->tx_bytes += dlc;
>> +	dev->trans_start = jiffies;
>> +
>> +	can_put_echo_skb(skb, dev, 0);
> 
> Hmm...looking back at can_put_echo_skb(), I see that it expects dev->priv
> to point to a struct can_priv.  Here, though, we see it pointing to a
> struct sja1000_prive instead.  I begin to suspect dangerous trickery going
> on behind our backs...

I see it coming...

>> +
>> +	priv->write_reg(dev, REG_CMR, CMD_TR);
>> +
>> +	return 0;
>> +}
> 
> [...]
> 
>> +irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>> +{
>> +	struct net_device *dev = (struct net_device *)dev_id;
>> +	struct sja1000_priv *priv = netdev_priv(dev);
>> +	struct net_device_stats *stats = &dev->stats;
>> +	uint8_t isrc, status;
>> +	int n = 0;
>> +
>> +	/* Shared interrupts and IRQ off? */
>> +	if (priv->read_reg(dev, REG_IER) == IRQ_OFF)
>> +		return IRQ_NONE;
>> +
>> +	if (priv->pre_irq)
>> +		priv->pre_irq(dev);
>> +
>> +	while ((isrc = priv->read_reg(dev, REG_IR)) && (n < SJA1000_MAX_IRQ)) {
>> +		n++;
>> +		status = priv->read_reg(dev, REG_SR);
>> +
>> +		if (isrc & IRQ_WUI) {
>> +			/* wake-up interrupt */
>> +			priv->can.can_stats.wakeup++;
>> +		}
>> +		if (isrc & IRQ_TI) {
>> +			/* transmission complete interrupt */
>> +			stats->tx_packets++;
>> +			can_get_echo_skb(dev, 0);
>> +			netif_wake_queue(dev);
>> +		}
>> +		if (isrc & IRQ_RI) {
>> +			/* receive interrupt */
>> +			while (status & SR_RBS) {
>> +				sja1000_rx(dev);
>> +				status = priv->read_reg(dev, REG_SR);
>> +			}
>> +		}
>> +		if (isrc & (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {
>> +			/* error interrupt */
>> +			if (sja1000_err(dev, isrc, status))
>> +				break;
>> +		}
>> +	}
>> +
>> +	if (priv->post_irq)
>> +		priv->post_irq(dev);
>> +
>> +	if (n >= SJA1000_MAX_IRQ)
>> +		dev_dbg(ND2D(dev), "%d messages handled in ISR", n);
>> +
>> +	return (n) ? IRQ_HANDLED : IRQ_NONE;
>> +}
>> +EXPORT_SYMBOL_GPL(sja1000_interrupt);
> 
> You used spin_lock_irq(&irq_lock) above, but the interrupt handler doesn't
> take that lock?  So (above) you could acquire the lock while the interrupt
> handler is running?  I hate to keep asking this question, but...what does
> that lock protect?

That's wrong, indeed.

> [...]
> 
>> +static int sja1000_close(struct net_device *dev)
>> +{
>> +	struct sja1000_priv *priv = netdev_priv(dev);
>> +
>> +	set_reset_mode(dev);
>> +	netif_stop_queue(dev);
>> +	priv->open_time = 0;
>> +	can_close_cleanup(dev);
> 
> What happens if your device interrupts right here?  Maybe you should
> disconnect the handler earlier?

It will not interrupt here because set_reset_mode(dev) already disabled
the interrupts.

>> +	if (!(priv->flags & SJA1000_CUSTOM_IRQ_HANDLER))
>> +		free_irq(dev->irq, (void *)dev);
>> +
>> +	return 0;
>> +}
> 
> [...]
> 
>> +int register_sja1000dev(struct net_device *dev)
>> +{
>> +	struct sja1000_priv *priv = netdev_priv(dev);
>> +	int err;
>> +
>> +	if (!sja1000_probe_chip(dev))
>> +		return -ENODEV;
>> +
>> +	dev->flags |= IFF_ECHO;	/* we support local echo */
>> +
>> +	dev->netdev_ops = &sja1000_netdev_ops;
>> +
>> +	priv->can.bittiming_const = &sja1000_bittiming_const;
>> +	priv->can.do_set_bittiming = sja1000_set_bittiming;
>> +	priv->can.do_get_state = sja1000_get_state;
>> +	priv->can.do_set_mode = sja1000_set_mode;
>> +	priv->dev = dev;
>> +
>> +	err = register_candev(dev);
> 
> Here we've registered our device with the CAN and networking core...
> 
>> +	if (err) {
>> +		printk(KERN_INFO
>> +		       "%s: registering netdev failed\n", DRV_NAME);
>> +		free_netdev(dev);
>> +		return err;
>> +	}
>> +
>> +	set_reset_mode(dev);
>> +	chipset_init(dev);
> 
> ...but only here have we gotten it ready to operate.  If the higher levels
> decide to do something with your device in the mean time, will the right
> thing happen?

Right, these two lines must be moved before register_candev().

> 
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(register_sja1000dev);
> 
> [...]
> 
>> diff --git a/drivers/net/can/sja1000/sja1000.h b/drivers/net/can/sja1000/sja1000.h
>> new file mode 100644
>> index 0000000..60d4cd6
>> --- /dev/null
>> +++ b/drivers/net/can/sja1000/sja1000.h
> 
> [...]
> 
>> +/*
>> + * SJA1000 private data structure
>> + */
>> +struct sja1000_priv {
>> +	struct can_priv can;	/* must be the first member! */
> 
> AHA!  I knew it!
> 
> This kind of pointer trickery is fragile and dangerous, please don't do
> it.  Much better would be something like:
> 
> 	dev->priv = &dev_specific_priv->can;
> 
> Then the higher layers know they have a proper struct can_priv pointer.
> Then you can use container_of() at this level to get the outer structure
> pointer.  Much more robust and in line with normal kernel coding idiom.

Our approach allows a more elegant usage and is still used in the kernel
but I agree, it's more error-prone.

I will come up with a revised patch a.s.a.p.

Thanks.

Wolfgang.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfgang Grandegger - May 1, 2009, 6:21 p.m.
I'm now back fixing the open issues of this patch series...

Jonathan Corbet wrote:
[snip]
>> +/*
>> + * SJA1000 private data structure
>> + */
>> +struct sja1000_priv {
>> +	struct can_priv can;	/* must be the first member! */
> 
> AHA!  I knew it!
> 
> This kind of pointer trickery is fragile and dangerous, please don't do
> it.  Much better would be something like:
> 
> 	dev->priv = &dev_specific_priv->can;
> 
> Then the higher layers know they have a proper struct can_priv pointer.
> Then you can use container_of() at this level to get the outer structure
> pointer.  Much more robust and in line with normal kernel coding idiom.

Unfortunately, the "struct net_device" does not have a "priv" field. Using

	struct can_priv *can = netdev_priv(dev);
	struct sja1000_priv *sja1000 = candev_priv(dev);

instead is awkward. That's what we had at first. Any other more elegant
solution in mind?

Wolfgang.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index d609895..78a412b 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -35,6 +35,17 @@  config CAN_CALC_BITTIMING
 	  files "tq", "prop_seg", "phase_seg1", "phase_seg2" and "sjw".
 	  If unsure, say Y.
 
+config CAN_SJA1000
+	depends on CAN_DEV
+	tristate "Philips SJA1000"
+	---help---
+	  The SJA1000 is one of the top CAN controllers out there. As it
+	  has a multiplexed interface it fits directly to 8051
+	  microcontrollers or into the PC I/O port space. The SJA1000
+	  is a full CAN controller, with shadow registers for RX and TX.
+	  It can send and receive any kinds of CAN frames (SFF/EFF/RTR)
+	  with a single (simple) filter setup.
+
 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 557114b..728d83f 100644
--- a/drivers/net/can/Makefile
+++ b/drivers/net/can/Makefile
@@ -7,4 +7,6 @@  obj-$(CONFIG_CAN_VCAN)		+= vcan.o
 obj-$(CONFIG_CAN_DEV)		+= can-dev.o
 can-dev-y			:= dev.o sysfs.o
 
+obj-$(CONFIG_CAN_SJA1000)	+= sja1000/
+
 ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
diff --git a/drivers/net/can/sja1000/Makefile b/drivers/net/can/sja1000/Makefile
new file mode 100644
index 0000000..893ab2c
--- /dev/null
+++ b/drivers/net/can/sja1000/Makefile
@@ -0,0 +1,7 @@ 
+#
+#  Makefile for the SJA1000 CAN controller drivers.
+#
+
+obj-$(CONFIG_CAN_SJA1000) += sja1000.o
+
+ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
new file mode 100644
index 0000000..6fe516d
--- /dev/null
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -0,0 +1,681 @@ 
+/*
+ * sja1000.c -  Philips SJA1000 network device driver
+ *
+ * Copyright (c) 2003 Matthias Brukner, Trajet Gmbh, Rebenring 33,
+ * 38106 Braunschweig, GERMANY
+ *
+ * Copyright (c) 2002-2007 Volkswagen Group Electronic Research
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. Neither the name of Volkswagen nor the names of its contributors
+ *    may be used to endorse or promote products derived from this software
+ *    without specific prior written permission.
+ *
+ * Alternatively, provided that this notice is retained in full, this
+ * software may be distributed under the terms of the GNU General
+ * Public License ("GPL") version 2, in which case the provisions of the
+ * GPL apply INSTEAD OF those given above.
+ *
+ * The provided data structures and external interfaces from this code
+ * are not restricted to be used by modules with a GPL compatible license.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
+ * DAMAGE.
+ *
+ * Send feedback to <socketcan-users@lists.berlios.de>
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/sched.h>
+#include <linux/types.h>
+#include <linux/fcntl.h>
+#include <linux/interrupt.h>
+#include <linux/ptrace.h>
+#include <linux/string.h>
+#include <linux/errno.h>
+#include <linux/netdevice.h>
+#include <linux/if_arp.h>
+#include <linux/if_ether.h>
+#include <linux/skbuff.h>
+#include <linux/delay.h>
+
+#include <linux/can.h>
+#include <linux/can/dev.h>
+#include <linux/can/error.h>
+#include <linux/can/dev.h>
+
+#include "sja1000.h"
+
+#define DRV_NAME "sja1000"
+
+MODULE_AUTHOR("Oliver Hartkopp <oliver.hartkopp@volkswagen.de>");
+MODULE_LICENSE("Dual BSD/GPL");
+MODULE_DESCRIPTION(DRV_NAME " CAN netdevice driver");
+
+static struct can_bittiming_const sja1000_bittiming_const = {
+	.tseg1_min = 1,
+	.tseg1_max = 16,
+	.tseg2_min = 1,
+	.tseg2_max = 8,
+	.sjw_max = 4,
+	.brp_min = 1,
+	.brp_max = 64,
+	.brp_inc = 1,
+};
+
+static int sja1000_probe_chip(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+
+	if (dev->base_addr && (priv->read_reg(dev, 0) == 0xFF)) {
+		printk(KERN_INFO "%s: probing @0x%lX failed\n",
+		       DRV_NAME, dev->base_addr);
+		return 0;
+	}
+	return 1;
+}
+
+static int set_reset_mode(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	unsigned char status = priv->read_reg(dev, REG_MOD);
+	int i;
+
+	/* disable interrupts */
+	priv->write_reg(dev, REG_IER, IRQ_OFF);
+
+	for (i = 0; i < 100; i++) {
+		/* check reset bit */
+		if (status & MOD_RM) {
+			priv->can.state = CAN_STATE_STOPPED;
+			return 0;
+		}
+
+		priv->write_reg(dev, REG_MOD, MOD_RM);	/* reset chip */
+		status = priv->read_reg(dev, REG_MOD);
+		udelay(10);
+	}
+
+	dev_err(ND2D(dev), "setting SJA1000 into reset mode failed!\n");
+	return 1;
+
+}
+
+static int set_normal_mode(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	unsigned char status = priv->read_reg(dev, REG_MOD);
+	int i;
+
+	for (i = 0; i < 100; i++) {
+		/* check reset bit */
+		if ((status & MOD_RM) == 0) {
+			priv->can.state = CAN_STATE_ACTIVE;
+			/* enable all interrupts */
+			priv->write_reg(dev, REG_IER, IRQ_ALL);
+
+			return 0;
+		}
+
+		/* set chip to normal mode */
+		priv->write_reg(dev, REG_MOD, 0x00);
+		status = priv->read_reg(dev, REG_MOD);
+		udelay(10);
+	}
+
+	dev_err(ND2D(dev), "setting SJA1000 into normal mode failed!\n");
+	return 1;
+
+}
+
+static void sja1000_start(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+
+	/* leave reset mode */
+	if (priv->can.state != CAN_STATE_STOPPED)
+		set_reset_mode(dev);
+
+	/* Clear error counters and error code capture */
+	priv->write_reg(dev, REG_TXERR, 0x0);
+	priv->write_reg(dev, REG_RXERR, 0x0);
+	priv->read_reg(dev, REG_ECC);
+
+	/* leave reset mode */
+	set_normal_mode(dev);
+}
+
+static int sja1000_set_mode(struct net_device *dev, enum can_mode mode)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+
+	switch (mode) {
+	case CAN_MODE_START:
+		if (!priv->open_time)
+			return -EINVAL;
+
+		sja1000_start(dev);
+		if (netif_queue_stopped(dev))
+			netif_wake_queue(dev);
+		break;
+
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int sja1000_get_state(struct net_device *dev, enum can_state *state)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	u8 status;
+
+	/* FIXME: inspecting the status register to get the current state
+	 * is not really necessary, because state changes are handled by
+	 * in the ISR and the variable priv->can.state gets updated. The
+	 * CAN devicde interface needs fixing!
+	 */
+
+	spin_lock_irq(&priv->can.irq_lock);
+
+	if (priv->can.state == CAN_STATE_STOPPED) {
+		*state =  CAN_STATE_STOPPED;
+	} else {
+		status = priv->read_reg(dev, REG_SR);
+		if (status & SR_BS)
+			*state = CAN_STATE_BUS_OFF;
+		else if (status & SR_ES) {
+			if (priv->read_reg(dev, REG_TXERR) > 127 ||
+			    priv->read_reg(dev, REG_RXERR) > 127)
+				*state = CAN_STATE_BUS_PASSIVE;
+			else
+				*state = CAN_STATE_BUS_WARNING;
+		} else
+			*state = CAN_STATE_ACTIVE;
+	}
+	/* Check state */
+	if (*state != priv->can.state)
+		dev_err(ND2D(dev),
+			"Oops, state mismatch: hard %d != soft %d\n",
+			*state, priv->can.state);
+	spin_unlock_irq(&priv->can.irq_lock);
+	return 0;
+}
+
+static int sja1000_set_bittiming(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	struct can_bittiming *bt = &priv->can.bittiming;
+	u8 btr0, btr1;
+
+	btr0 = ((bt->brp - 1) & 0x3f) | (((bt->sjw - 1) & 0x3) << 6);
+	btr1 = ((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) |
+		(((bt->phase_seg2 - 1) & 0x7) << 4) |
+	  ((priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES) << 7);
+
+	dev_info(ND2D(dev), "setting BTR0=0x%02x BTR1=0x%02x\n", btr0, btr1);
+
+	priv->write_reg(dev, REG_BTR0, btr0);
+	priv->write_reg(dev, REG_BTR1, btr1);
+
+	return 0;
+}
+
+/*
+ * initialize SJA1000 chip:
+ *   - reset chip
+ *   - set output mode
+ *   - set baudrate
+ *   - enable interrupts
+ *   - start operating mode
+ */
+static void chipset_init(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+
+	/* set clock divider and output control register */
+	priv->write_reg(dev, REG_CDR, priv->cdr | CDR_PELICAN);
+
+	/* set acceptance filter (accept all) */
+	priv->write_reg(dev, REG_ACCC0, 0x00);
+	priv->write_reg(dev, REG_ACCC1, 0x00);
+	priv->write_reg(dev, REG_ACCC2, 0x00);
+	priv->write_reg(dev, REG_ACCC3, 0x00);
+
+	priv->write_reg(dev, REG_ACCM0, 0xFF);
+	priv->write_reg(dev, REG_ACCM1, 0xFF);
+	priv->write_reg(dev, REG_ACCM2, 0xFF);
+	priv->write_reg(dev, REG_ACCM3, 0xFF);
+
+	priv->write_reg(dev, REG_OCR, priv->ocr | OCR_MODE_NORMAL);
+}
+
+/*
+ * transmit a CAN message
+ * message layout in the sk_buff should be like this:
+ * xx xx xx xx	 ff	 ll   00 11 22 33 44 55 66 77
+ * [  can-id ] [flags] [len] [can data (up to 8 bytes]
+ */
+static int sja1000_start_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	struct can_frame *cf = (struct can_frame *)skb->data;
+	uint8_t fi;
+	uint8_t dlc;
+	canid_t id;
+	uint8_t dreg;
+	int i;
+
+	netif_stop_queue(dev);
+
+	fi = dlc = cf->can_dlc;
+	id = cf->can_id;
+
+	if (id & CAN_RTR_FLAG)
+		fi |= FI_RTR;
+
+	if (id & CAN_EFF_FLAG) {
+		fi |= FI_FF;
+		dreg = EFF_BUF;
+		priv->write_reg(dev, REG_FI, fi);
+		priv->write_reg(dev, REG_ID1, (id & 0x1fe00000) >> (5 + 16));
+		priv->write_reg(dev, REG_ID2, (id & 0x001fe000) >> (5 + 8));
+		priv->write_reg(dev, REG_ID3, (id & 0x00001fe0) >> 5);
+		priv->write_reg(dev, REG_ID4, (id & 0x0000001f) << 3);
+	} else {
+		dreg = SFF_BUF;
+		priv->write_reg(dev, REG_FI, fi);
+		priv->write_reg(dev, REG_ID1, (id & 0x000007f8) >> 3);
+		priv->write_reg(dev, REG_ID2, (id & 0x00000007) << 5);
+	}
+
+	for (i = 0; i < dlc; i++)
+		priv->write_reg(dev, dreg++, cf->data[i]);
+
+	stats->tx_bytes += dlc;
+	dev->trans_start = jiffies;
+
+	can_put_echo_skb(skb, dev, 0);
+
+	priv->write_reg(dev, REG_CMR, CMD_TR);
+
+	return 0;
+}
+
+static void sja1000_rx(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	struct can_frame *cf;
+	struct sk_buff *skb;
+	uint8_t fi;
+	uint8_t dreg;
+	canid_t id;
+	uint8_t dlc;
+	int i;
+
+	skb = dev_alloc_skb(sizeof(struct can_frame));
+	if (skb == NULL)
+		return;
+	skb->dev = dev;
+	skb->protocol = htons(ETH_P_CAN);
+
+	fi = priv->read_reg(dev, REG_FI);
+	dlc = fi & 0x0F;
+
+	if (fi & FI_FF) {
+		/* extended frame format (EFF) */
+		dreg = EFF_BUF;
+		id = (priv->read_reg(dev, REG_ID1) << (5 + 16))
+		    | (priv->read_reg(dev, REG_ID2) << (5 + 8))
+		    | (priv->read_reg(dev, REG_ID3) << 5)
+		    | (priv->read_reg(dev, REG_ID4) >> 3);
+		id |= CAN_EFF_FLAG;
+	} else {
+		/* standard frame format (SFF) */
+		dreg = SFF_BUF;
+		id = (priv->read_reg(dev, REG_ID1) << 3)
+		    | (priv->read_reg(dev, REG_ID2) >> 5);
+	}
+
+	if (fi & FI_RTR)
+		id |= CAN_RTR_FLAG;
+
+	cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
+	memset(cf, 0, sizeof(struct can_frame));
+	cf->can_id = id;
+	cf->can_dlc = dlc;
+	for (i = 0; i < dlc; i++)
+		cf->data[i] = priv->read_reg(dev, dreg++);
+
+	while (i < 8)
+		cf->data[i++] = 0;
+
+	/* release receive buffer */
+	priv->write_reg(dev, REG_CMR, CMD_RRB);
+
+	netif_rx(skb);
+
+	dev->last_rx = jiffies;
+	stats->rx_packets++;
+	stats->rx_bytes += dlc;
+}
+
+static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	struct can_frame *cf;
+	struct sk_buff *skb;
+	enum can_state state = priv->can.state;
+	uint8_t ecc, alc;
+
+	skb = dev_alloc_skb(sizeof(struct can_frame));
+	if (skb == NULL)
+		return -ENOMEM;
+	skb->dev = dev;
+	skb->protocol = htons(ETH_P_CAN);
+	cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
+	memset(cf, 0, sizeof(struct can_frame));
+	cf->can_id = CAN_ERR_FLAG;
+	cf->can_dlc = CAN_ERR_DLC;
+
+	if (isrc & IRQ_DOI) {
+		/* data overrun interrupt */
+		dev_dbg(ND2D(dev), "data overrun interrupt\n");
+		cf->can_id |= CAN_ERR_CRTL;
+		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
+		priv->can.can_stats.data_overrun++;
+		priv->write_reg(dev, REG_CMR, CMD_CDO);	/* clear bit */
+	}
+
+	if (isrc & IRQ_EI) {
+		/* error warning interrupt */
+		priv->can.can_stats.error_warning++;
+		dev_dbg(ND2D(dev), "error warning interrupt\n");
+
+		if (status & SR_BS) {
+			state = CAN_STATE_BUS_OFF;
+			cf->can_id |= CAN_ERR_BUSOFF;
+			can_bus_off(dev);
+		} else if (status & SR_ES) {
+			state = CAN_STATE_BUS_WARNING;
+		} else
+			state = CAN_STATE_ACTIVE;
+	}
+	if (isrc & IRQ_BEI) {
+		/* bus error interrupt */
+		priv->can.can_stats.bus_error++;
+		ecc = priv->read_reg(dev, REG_ECC);
+
+		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+
+		switch (ecc & ECC_MASK) {
+		case ECC_BIT:
+			cf->data[2] |= CAN_ERR_PROT_BIT;
+			break;
+		case ECC_FORM:
+			cf->data[2] |= CAN_ERR_PROT_FORM;
+			break;
+		case ECC_STUFF:
+			cf->data[2] |= CAN_ERR_PROT_STUFF;
+			break;
+		default:
+			cf->data[2] |= CAN_ERR_PROT_UNSPEC;
+			cf->data[3] = ecc & ECC_SEG;
+			break;
+		}
+		/* Error occured during transmission? */
+		if ((ecc & ECC_DIR) == 0)
+			cf->data[2] |= CAN_ERR_PROT_TX;
+	}
+	if (isrc & IRQ_EPI) {
+		/* error passive interrupt */
+		dev_dbg(ND2D(dev), "error passive interrupt\n");
+		priv->can.can_stats.error_passive++;
+		if (status & SR_ES)
+			state = CAN_STATE_BUS_PASSIVE;
+		else
+			state = CAN_STATE_ACTIVE;
+	}
+	if (isrc & IRQ_ALI) {
+		/* arbitration lost interrupt */
+		dev_dbg(ND2D(dev), "arbitration lost interrupt\n");
+		alc = priv->read_reg(dev, REG_ALC);
+		priv->can.can_stats.arbitration_lost++;
+		cf->can_id |= CAN_ERR_LOSTARB;
+		cf->data[0] = alc & 0x1f;
+	}
+
+	if (state != priv->can.state && (state == CAN_STATE_BUS_WARNING ||
+					 state == CAN_STATE_BUS_PASSIVE)) {
+		uint8_t rxerr = priv->read_reg(dev, REG_RXERR);
+		uint8_t txerr = priv->read_reg(dev, REG_TXERR);
+		cf->can_id |= CAN_ERR_CRTL;
+		if (state == CAN_STATE_BUS_WARNING)
+			cf->data[1] = (txerr > rxerr) ?
+				CAN_ERR_CRTL_TX_WARNING :
+				CAN_ERR_CRTL_RX_WARNING;
+		else
+			cf->data[1] = (txerr > rxerr) ?
+				CAN_ERR_CRTL_TX_PASSIVE :
+				CAN_ERR_CRTL_RX_PASSIVE;
+	}
+
+	priv->can.state = state;
+
+	netif_rx(skb);
+
+	dev->last_rx = jiffies;
+	stats->rx_packets++;
+	stats->rx_bytes += cf->can_dlc;
+
+	return 0;
+}
+
+irqreturn_t sja1000_interrupt(int irq, void *dev_id)
+{
+	struct net_device *dev = (struct net_device *)dev_id;
+	struct sja1000_priv *priv = netdev_priv(dev);
+	struct net_device_stats *stats = &dev->stats;
+	uint8_t isrc, status;
+	int n = 0;
+
+	/* Shared interrupts and IRQ off? */
+	if (priv->read_reg(dev, REG_IER) == IRQ_OFF)
+		return IRQ_NONE;
+
+	if (priv->pre_irq)
+		priv->pre_irq(dev);
+
+	while ((isrc = priv->read_reg(dev, REG_IR)) && (n < SJA1000_MAX_IRQ)) {
+		n++;
+		status = priv->read_reg(dev, REG_SR);
+
+		if (isrc & IRQ_WUI) {
+			/* wake-up interrupt */
+			priv->can.can_stats.wakeup++;
+		}
+		if (isrc & IRQ_TI) {
+			/* transmission complete interrupt */
+			stats->tx_packets++;
+			can_get_echo_skb(dev, 0);
+			netif_wake_queue(dev);
+		}
+		if (isrc & IRQ_RI) {
+			/* receive interrupt */
+			while (status & SR_RBS) {
+				sja1000_rx(dev);
+				status = priv->read_reg(dev, REG_SR);
+			}
+		}
+		if (isrc & (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {
+			/* error interrupt */
+			if (sja1000_err(dev, isrc, status))
+				break;
+		}
+	}
+
+	if (priv->post_irq)
+		priv->post_irq(dev);
+
+	if (n >= SJA1000_MAX_IRQ)
+		dev_dbg(ND2D(dev), "%d messages handled in ISR", n);
+
+	return (n) ? IRQ_HANDLED : IRQ_NONE;
+}
+EXPORT_SYMBOL_GPL(sja1000_interrupt);
+
+static int sja1000_open(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	int err;
+
+	/* set chip into reset mode */
+	set_reset_mode(dev);
+
+	/* determine and set bittime */
+	err = can_set_bittiming(dev);
+	if (err)
+		return err;
+
+	/* register interrupt handler, if not done by the device driver */
+	if (!(priv->flags & SJA1000_CUSTOM_IRQ_HANDLER)) {
+		err = request_irq(dev->irq, &sja1000_interrupt, IRQF_SHARED,
+				  dev->name, (void *)dev);
+		if (err)
+			return -EAGAIN;
+	}
+
+	/* init and start chi */
+	sja1000_start(dev);
+	priv->open_time = jiffies;
+
+	netif_start_queue(dev);
+
+	return 0;
+}
+
+static int sja1000_close(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+
+	set_reset_mode(dev);
+	netif_stop_queue(dev);
+	priv->open_time = 0;
+	can_close_cleanup(dev);
+
+	if (!(priv->flags & SJA1000_CUSTOM_IRQ_HANDLER))
+		free_irq(dev->irq, (void *)dev);
+
+	return 0;
+}
+
+struct net_device *alloc_sja1000dev(int sizeof_priv)
+{
+	struct net_device *dev;
+	struct sja1000_priv *priv;
+
+	dev = alloc_candev(sizeof(struct sja1000_priv) + sizeof_priv);
+	if (!dev)
+		return NULL;
+
+	priv = netdev_priv(dev);
+	priv->dev = dev;
+
+	if (sizeof_priv)
+		priv->priv = (void *)priv + sizeof(struct sja1000_priv);
+
+	return dev;
+}
+EXPORT_SYMBOL_GPL(alloc_sja1000dev);
+
+void free_sja1000dev(struct net_device *dev)
+{
+	free_candev(dev);
+}
+EXPORT_SYMBOL_GPL(free_sja1000dev);
+
+static const struct net_device_ops sja1000_netdev_ops = {
+       .ndo_open               = sja1000_open,
+       .ndo_stop               = sja1000_close,
+       .ndo_start_xmit         = sja1000_start_xmit,
+};
+
+int register_sja1000dev(struct net_device *dev)
+{
+	struct sja1000_priv *priv = netdev_priv(dev);
+	int err;
+
+	if (!sja1000_probe_chip(dev))
+		return -ENODEV;
+
+	dev->flags |= IFF_ECHO;	/* we support local echo */
+
+	dev->netdev_ops = &sja1000_netdev_ops;
+
+	priv->can.bittiming_const = &sja1000_bittiming_const;
+	priv->can.do_set_bittiming = sja1000_set_bittiming;
+	priv->can.do_get_state = sja1000_get_state;
+	priv->can.do_set_mode = sja1000_set_mode;
+	priv->dev = dev;
+
+	err = register_candev(dev);
+	if (err) {
+		printk(KERN_INFO
+		       "%s: registering netdev failed\n", DRV_NAME);
+		free_netdev(dev);
+		return err;
+	}
+
+	set_reset_mode(dev);
+	chipset_init(dev);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(register_sja1000dev);
+
+void unregister_sja1000dev(struct net_device *dev)
+{
+	set_reset_mode(dev);
+	unregister_candev(dev);
+}
+EXPORT_SYMBOL_GPL(unregister_sja1000dev);
+
+static __init int sja1000_init(void)
+{
+	printk(KERN_INFO "%s CAN netdevice driver\n", DRV_NAME);
+
+	return 0;
+}
+
+module_init(sja1000_init);
+
+static __exit void sja1000_exit(void)
+{
+	printk(KERN_INFO "%s: driver removed\n", DRV_NAME);
+}
+
+module_exit(sja1000_exit);
diff --git a/drivers/net/can/sja1000/sja1000.h b/drivers/net/can/sja1000/sja1000.h
new file mode 100644
index 0000000..60d4cd6
--- /dev/null
+++ b/drivers/net/can/sja1000/sja1000.h
@@ -0,0 +1,177 @@ 
+/*
+ * sja1000.h -  Philips SJA1000 network device driver
+ *
+ * Copyright (c) 2003 Matthias Brukner, Trajet Gmbh, Rebenring 33,
+ * 38106 Braunschweig, GERMANY
+ *
+ * Copyright (c) 2002-2007 Volkswagen Group Electronic Research
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ * 3. Neither the name of Volkswagen nor the names of its contributors
+ *    may be used to endorse or promote products derived from this software
+ *    without specific prior written permission.
+ *
+ * Alternatively, provided that this notice is retained in full, this
+ * software may be distributed under the terms of the GNU General
+ * Public License ("GPL") version 2, in which case the provisions of the
+ * GPL apply INSTEAD OF those given above.
+ *
+ * The provided data structures and external interfaces from this code
+ * are not restricted to be used by modules with a GPL compatible license.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
+ * DAMAGE.
+ *
+ * Send feedback to <socketcan-users@lists.berlios.de>
+ *
+ */
+
+#ifndef SJA1000_DEV_H
+#define SJA1000_DEV_H
+
+#include <linux/can/dev.h>
+#include <linux/can/platform/sja1000.h>
+
+#define SJA1000_MAX_IRQ 20	/* max. number of interrupts handled in ISR */
+
+/* SJA1000 registers - manual section 6.4 (Pelican Mode) */
+#define REG_MOD		0x00
+#define REG_CMR		0x01
+#define REG_SR		0x02
+#define REG_IR		0x03
+#define REG_IER		0x04
+#define REG_ALC		0x0B
+#define REG_ECC		0x0C
+#define REG_EWL		0x0D
+#define REG_RXERR	0x0E
+#define REG_TXERR	0x0F
+#define REG_ACCC0	0x10
+#define REG_ACCC1	0x11
+#define REG_ACCC2	0x12
+#define REG_ACCC3	0x13
+#define REG_ACCM0	0x14
+#define REG_ACCM1	0x15
+#define REG_ACCM2	0x16
+#define REG_ACCM3	0x17
+#define REG_RMC		0x1D
+#define REG_RBSA	0x1E
+
+/* Common registers - manual section 6.5 */
+#define REG_BTR0	0x06
+#define REG_BTR1	0x07
+#define REG_OCR		0x08
+#define REG_CDR		0x1F
+
+#define REG_FI		0x10
+#define SFF_BUF		0x13
+#define EFF_BUF		0x15
+
+#define FI_FF		0x80
+#define FI_RTR		0x40
+
+#define REG_ID1		0x11
+#define REG_ID2		0x12
+#define REG_ID3		0x13
+#define REG_ID4		0x14
+
+#define CAN_RAM		0x20
+
+/* mode register */
+#define MOD_RM		0x01
+#define MOD_LOM		0x02
+#define MOD_STM		0x04
+#define MOD_AFM		0x08
+#define MOD_SM		0x10
+
+/* commands */
+#define CMD_SRR		0x10
+#define CMD_CDO		0x08
+#define CMD_RRB		0x04
+#define CMD_AT		0x02
+#define CMD_TR		0x01
+
+/* interrupt sources */
+#define IRQ_BEI		0x80
+#define IRQ_ALI		0x40
+#define IRQ_EPI		0x20
+#define IRQ_WUI		0x10
+#define IRQ_DOI		0x08
+#define IRQ_EI		0x04
+#define IRQ_TI		0x02
+#define IRQ_RI		0x01
+#define IRQ_ALL		0xFF
+#define IRQ_OFF		0x00
+
+/* status register content */
+#define SR_BS		0x80
+#define SR_ES		0x40
+#define SR_TS		0x20
+#define SR_RS		0x10
+#define SR_TCS		0x08
+#define SR_TBS		0x04
+#define SR_DOS		0x02
+#define SR_RBS		0x01
+
+#define SR_CRIT (SR_BS|SR_ES)
+
+/* ECC register */
+#define ECC_SEG		0x1F
+#define ECC_DIR		0x20
+#define ECC_ERR		6
+#define ECC_BIT		0x00
+#define ECC_FORM	0x40
+#define ECC_STUFF	0x80
+#define ECC_MASK	0xc0
+
+/*
+ * Flags for sja1000priv.flags
+ */
+#define SJA1000_CUSTOM_IRQ_HANDLER 0x1
+
+/*
+ * SJA1000 private data structure
+ */
+struct sja1000_priv {
+	struct can_priv can;	/* must be the first member! */
+	long open_time;
+	struct sk_buff *echo_skb;
+
+	u8 (*read_reg) (struct net_device *dev, int reg);
+	void (*write_reg) (struct net_device *dev, int reg, u8 val);
+	void (*pre_irq) (struct net_device *dev);
+	void (*post_irq) (struct net_device *dev);
+
+	void *priv;		/* for board-specific data */
+	struct net_device *dev;
+
+	u8 ocr;
+	u8 cdr;
+	u32 flags;
+};
+
+struct net_device *alloc_sja1000dev(int sizeof_priv);
+void free_sja1000dev(struct net_device *dev);
+int register_sja1000dev(struct net_device *dev);
+void unregister_sja1000dev(struct net_device *dev);
+
+irqreturn_t sja1000_interrupt(int irq, void *dev_id);
+
+#endif /* SJA1000_DEV_H */