Subject: Re: [PATCH net-next-2.6] Driver for the Microchip MCP251x SPI CAN controllers

From: Wolfgang Grandegger <[email-masked]>
Date: Sun, 01 Nov 2009 10:31:53 +0100

Hi Christian,

we already discusses various driver issues on the Socket-CAN ML. Still,
there are a few. In general, please check the usage of {} for if
statements and check if "if (ret)" should be used instead of "if (ret <
0)" if 0 means success and !0 failure. I don't have a MCP251x hardware
at hand, but maybe Paul (on CC now) has a chance to test it. More
comments inline...

Christian Pellegrin wrote:
> Signed-off-by: Christian Pellegrin <[email-masked]>

Please use the subject prefix "can: Driver for the..."

> ---
>  drivers/net/can/Kconfig              |    6 +
>  drivers/net/can/Makefile             |    1 +
>  drivers/net/can/mcp251x.c            | 1182 ++++++++++++++++++++++++++++++++++
>  include/linux/can/platform/mcp251x.h |   34 +
>  4 files changed, 1223 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/net/can/mcp251x.c
>  create mode 100644 include/linux/can/platform/mcp251x.h
> 
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 26d77cc..e987526 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -102,6 +102,12 @@ config CAN_TI_HECC
>  	  Driver for TI HECC (High End CAN Controller) module found on many
>  	  TI devices. The device specifications are available from www.ti.com
>  
> +config CAN_MCP251X
> +	tristate "Microchip MCP251x SPI CAN controllers"
> +	depends on CAN && CAN_DEV && SPI

You can drop the redundant dependency on "CAN".

> +	---help---
> +	  Driver for the Microchip MCP251x SPI CAN controllers.
> +
>  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 31f4ab5..1489181 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -12,5 +12,6 @@ obj-y				+= usb/
>  obj-$(CONFIG_CAN_SJA1000)	+= sja1000/
>  obj-$(CONFIG_CAN_AT91)		+= at91_can.o
>  obj-$(CONFIG_CAN_TI_HECC)	+= ti_hecc.o
> +obj-$(CONFIG_CAN_MCP251X)	+= mcp251x.o
>  
>  ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/mcp251x.c b/drivers/net/can/mcp251x.c
> new file mode 100644
> index 0000000..f444cac
> --- /dev/null
> +++ b/drivers/net/can/mcp251x.c
> @@ -0,0 +1,1182 @@
> +/*
> + * CAN bus driver for Microchip 251x CAN Controller with SPI Interface
> + *
> + * MCP2510 support and bug fixes by Christian Pellegrin
> + * <[email-masked]>

Please add your "Copyright ...".

> + *
> + * Copyright 2007 Raymarine UK, Ltd. All Rights Reserved.
> + * Written under contract by:
> + *   Chris Elston, Katalix Systems, Ltd.
> + *
> + * Based on Microchip MCP251x CAN controller driver written by
> + * David Vrabel, Copyright 2006 Arcom Control Systems Ltd.
> + *
> + * Based on CAN bus driver for the CCAN controller written by
> + * - Sascha Hauer, Marc Kleine-Budde, Pengutronix
> + * - Simon Kallweit, intefo AG
> + * Copyright 2007
> + *
> + * 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
> + *
> + *
> + *
> + * Your platform definition file should specify something like:
> + *
> + * static struct mcp251x_platform_data mcp251x_info = {
> + *         .oscillator_frequency = 8000000,
> + *         .board_specific_setup = &mcp251x_setup,
> + *         .model = CAN_MCP251X_MCP2510,
> + *         .power_enable = mcp251x_power_enable,
> + *         .transceiver_enable = NULL,
> + * };
> + *
> + * static struct spi_board_info spi_board_info[] = {
> + *         {
> + *                 .modalias      = "mcp251x",
> + *                 .platform_data = &mcp251x_info,
> + *                 .irq           = IRQ_EINT13,
> + *                 .max_speed_hz  = 2*1000*1000,
> + *                 .chip_select   = 2,
> + *         },
> + * };
> + *
> + * Please see mcp251x.h for a description of the fields in
> + * struct mcp251x_platform_data.
> + *
> + */
> +
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/netdevice.h>
> +#include <linux/can.h>
> +#include <linux/spi/spi.h>
> +#include <linux/can/dev.h>
> +#include <linux/can/core.h>

I don't think you need "can/core.h"?

> +#include <linux/if_arp.h>

And that one either.

> +#include <linux/dma-mapping.h>
> +#include <linux/delay.h>
> +#include <linux/completion.h>
> +#include <linux/freezer.h>
> +#include <linux/uaccess.h>
> +#include <linux/io.h>
> +#include <linux/can/platform/mcp251x.h>
> +
> +/* SPI interface instruction set */
> +#define INSTRUCTION_WRITE	0x02
> +#define INSTRUCTION_READ	0x03
> +#define INSTRUCTION_BIT_MODIFY	0x05
> +#define INSTRUCTION_LOAD_TXB(n)	(0x40 + 2 * (n))
> +#define INSTRUCTION_READ_RXB(n)	(((n) == 0) ? 0x90 : 0x94)
> +#define INSTRUCTION_RESET	0xC0
> +
> +/* MPC251x registers */
> +#define CANSTAT	      0x0e
> +#define CANCTRL	      0x0f
> +#  define CANCTRL_REQOP_MASK	    0xe0
> +#  define CANCTRL_REQOP_CONF	    0x80
> +#  define CANCTRL_REQOP_LISTEN_ONLY 0x60
> +#  define CANCTRL_REQOP_LOOPBACK    0x40
> +#  define CANCTRL_REQOP_SLEEP	    0x20
> +#  define CANCTRL_REQOP_NORMAL	    0x00
> +#  define CANCTRL_OSM		    0x08
> +#  define CANCTRL_ABAT		    0x10
> +#define TEC	      0x1c
> +#define REC	      0x1d
> +#define CNF1	      0x2a
> +#  define CNF1_SJW_SHIFT   6
> +#define CNF2	      0x29
> +#  define CNF2_BTLMODE	   0x80
> +#  define CNF2_SAM         0x40
> +#  define CNF2_PS1_SHIFT   3
> +#define CNF3	      0x28
> +#  define CNF3_SOF	   0x08
> +#  define CNF3_WAKFIL	   0x04
> +#  define CNF3_PHSEG2_MASK 0x07
> +#define CANINTE	      0x2b
> +#  define CANINTE_MERRE 0x80
> +#  define CANINTE_WAKIE 0x40
> +#  define CANINTE_ERRIE 0x20
> +#  define CANINTE_TX2IE 0x10
> +#  define CANINTE_TX1IE 0x08
> +#  define CANINTE_TX0IE 0x04
> +#  define CANINTE_RX1IE 0x02
> +#  define CANINTE_RX0IE 0x01
> +#define CANINTF	      0x2c
> +#  define CANINTF_MERRF 0x80
> +#  define CANINTF_WAKIF 0x40
> +#  define CANINTF_ERRIF 0x20
> +#  define CANINTF_TX2IF 0x10
> +#  define CANINTF_TX1IF 0x08
> +#  define CANINTF_TX0IF 0x04
> +#  define CANINTF_RX1IF 0x02
> +#  define CANINTF_RX0IF 0x01
> +#define EFLG	      0x2d
> +#  define EFLG_EWARN	0x01
> +#  define EFLG_RXWAR	0x02
> +#  define EFLG_TXWAR	0x04
> +#  define EFLG_RXEP	0x08
> +#  define EFLG_TXEP	0x10
> +#  define EFLG_TXBO	0x20
> +#  define EFLG_RX0OVR	0x40
> +#  define EFLG_RX1OVR	0x80
> +#define TXBCTRL(n)  ((n * 0x10) + 0x30)

Please put brackets around "n": (((n) * 0x10) + 0x30)
Also the proper offset definition should be used.

#define TXBCTRL(n)  (((n) * 0x10) + 0x30 + TXBCTRL_OFF)

Here and in similar cases below.

> +#  define TXBCTRL_ABTF	0x40
> +#  define TXBCTRL_MLOA	0x20
> +#  define TXBCTRL_TXERR 0x10
> +#  define TXBCTRL_TXREQ 0x08
> +#define TXBSIDH(n)  ((n * 0x10) + 0x31)
> +#  define SIDH_SHIFT    3
> +#define TXBSIDL(n)  ((n * 0x10) + 0x32)
> +#  define SIDL_SID_MASK    7
> +#  define SIDL_SID_SHIFT   5
> +#  define SIDL_EXIDE_SHIFT 3
> +#  define SIDL_EID_SHIFT   16
> +#  define SIDL_EID_MASK    3
> +#define TXBEID8(n)  ((n * 0x10) + 0x33)
> +#define TXBEID0(n)  ((n * 0x10) + 0x34)
> +#define TXBDLC(n)   ((n * 0x10) + 0x35)
> +#  define DLC_RTR_SHIFT    6
> +#define TXBCTRL_OFF 0
> +#define TXBSIDH_OFF 1
> +#define TXBSIDL_OFF 2
> +#define TXBEID8_OFF 3
> +#define TXBEID0_OFF 4
> +#define TXBDLC_OFF  5
> +#define TXBDAT_OFF  6
> +#define RXBCTRL(n)  ((n * 0x10) + 0x60)
> +#  define RXBCTRL_BUKT	0x04
> +#  define RXBCTRL_RXM0	0x20
> +#  define RXBCTRL_RXM1	0x40
> +#define RXBSIDH(n)  ((n * 0x10) + 0x61)
> +#  define RXBSIDH_SHIFT 3
> +#define RXBSIDL(n)  ((n * 0x10) + 0x62)
> +#  define RXBSIDL_IDE   0x08
> +#  define RXBSIDL_EID   3
> +#  define RXBSIDL_SHIFT 5
> +#define RXBEID8(n)  ((n * 0x10) + 0x63)
> +#define RXBEID0(n)  ((n * 0x10) + 0x64)
> +#define RXBDLC(n)   ((n * 0x10) + 0x65)
> +#  define RXBDLC_LEN_MASK  0x0f
> +#  define RXBDLC_RTR       0x40
> +#define RXBCTRL_OFF 0
> +#define RXBSIDH_OFF 1
> +#define RXBSIDL_OFF 2
> +#define RXBEID8_OFF 3
> +#define RXBEID0_OFF 4
> +#define RXBDLC_OFF  5
> +#define RXBDAT_OFF  6

I was thinking to use structure(s) for the offsets (register layout)
above, but fiddling with offsetof() does probably not make the code more
readable.

> +
> +#define GET_BYTE(val, byte)			\
> +	(((val) >> ((byte) * 8)) & 0xff)
> +#define SET_BYTE(val, byte)			\
> +	(((val) & 0xff) << ((byte) * 8))
> +
> +/*
> + * Buffer size required for the largest SPI transfer (i.e., reading a
> + * frame)
> + */
> +#define CAN_FRAME_MAX_DATA_LEN	8
> +#define SPI_TRANSFER_BUF_LEN	(6 + CAN_FRAME_MAX_DATA_LEN)
> +#define CAN_FRAME_MAX_BITS	128
> +
> +#define TX_ECHO_SKB_MAX	1
> +
> +#define DEVICE_NAME "mcp251x"
> +
> +static int mcp251x_enable_dma; /* Enable SPI DMA. Default: 0 (Off) */
> +module_param(mcp251x_enable_dma, int, S_IRUGO);
> +MODULE_PARM_DESC(mcp251x_enable_dma, "Enable SPI DMA. Default: 0 (Off)");
> +
> +static struct can_bittiming_const mcp251x_bittiming_const = {
> +	.name = DEVICE_NAME,
> +	.tseg1_min = 3,
> +	.tseg1_max = 16,
> +	.tseg2_min = 2,
> +	.tseg2_max = 8,
> +	.sjw_max = 4,
> +	.brp_min = 1,
> +	.brp_max = 64,
> +	.brp_inc = 1,
> +};
> +
> +struct mcp251x_priv {
> +	struct can_priv	   can;
> +	struct net_device *net;
> +	struct spi_device *spi;
> +
> +	struct mutex spi_lock; /* SPI buffer lock */
> +	u8 *spi_tx_buf;
> +	u8 *spi_rx_buf;
> +	dma_addr_t spi_tx_dma;
> +	dma_addr_t spi_rx_dma;
> +
> +	struct sk_buff *tx_skb;
> +	int tx_len;
> +	struct workqueue_struct *wq;
> +	struct work_struct tx_work;
> +	struct work_struct irq_work;
> +	struct completion awake;
> +	int wake;
> +	int force_quit;
> +	int after_suspend;
> +#define AFTER_SUSPEND_UP 1
> +#define AFTER_SUSPEND_DOWN 2
> +#define AFTER_SUSPEND_POWER 4
> +#define AFTER_SUSPEND_RESTART 8
> +	int restart_tx;
> +};
> +
> +static void mcp251x_clean(struct net_device *net)
> +{
> +	struct mcp251x_priv *priv = netdev_priv(net);
> +
> +	net->stats.tx_errors++;
> +	if (priv->tx_skb)
> +		dev_kfree_skb(priv->tx_skb);
> +	if (priv->tx_len)
> +		can_free_echo_skb(priv->net, 0);
> +	priv->tx_skb = NULL;
> +	priv->tx_len = 0;
> +}
> +
> +/*
> + * Note about handling of error return of mcp251x_spi_trans: accessing
> + * registers via SPI is not really different conceptually than using
> + * normal I/O assembler instructions, although it's much more
> + * complicated from a practical POV. So it's not advisable to always
> + * check the return value of this function. Imagine that every
> + * read{b,l}, write{b,l} and friends would be bracketed in "if ( < 0)
> + * error();", it would be a great mess (well there are some situation
> + * when exception handling C++ like could be useful after all). So we
> + * just check that transfers are OK at the beginning of our
> + * conversation with the chip and to avoid doing really nasty things
> + * (like injecting bogus packets in the network stack).
> + */
> +static int mcp251x_spi_trans(struct spi_device *spi, int len)
> +{
> +	struct mcp251x_priv *priv = dev_get_drvdata(&spi->dev);
> +	struct spi_transfer t = {
> +		.tx_buf = priv->spi_tx_buf,
> +		.rx_buf = priv->spi_rx_buf,
> +		.len = len,
> +		.cs_change = 0,
> +	};
> +	struct spi_message m;
> +	int ret;
> +
> +	spi_message_init(&m);
> +
> +	if (mcp251x_enable_dma) {
> +		t.tx_dma = priv->spi_tx_dma;
> +		t.rx_dma = priv->spi_rx_dma;
> +		m.is_dma_mapped = 1;
> +	}
> +
> +	spi_message_add_tail(&t, &m);
> +
> +	ret = spi_sync(spi, &m);
> +	if (ret < 0)

if (ret) ?

> +		dev_err(&spi->dev, "spi transfer failed: ret = %d\n", ret);
> +	return ret;
> +}
> +
> +static u8 mcp251x_read_reg(struct spi_device *spi, uint8_t reg)
> +{
> +	struct mcp251x_priv *priv = dev_get_drvdata(&spi->dev);
> +	u8 val = 0;
> +
> +	mutex_lock(&priv->spi_lock);
> +
> +	priv->spi_tx_buf[0] = INSTRUCTION_READ;
> +	priv->spi_tx_buf[1] = reg;
> +
> +	mcp251x_spi_trans(spi, 3);
> +	val = priv->spi_rx_buf[2];
> +
> +	mutex_unlock(&priv->spi_lock);
> +
> +	return val;
> +}
> +
> +static void mcp251x_write_reg(struct spi_device *spi, u8 reg, uint8_t val)
> +{
> +	struct mcp251x_priv *priv = dev_get_drvdata(&spi->dev);
> +
> +	mutex_lock(&priv->spi_lock);
> +
> +	priv->spi_tx_buf[0] = INSTRUCTION_WRITE;
> +	priv->spi_tx_buf[1] = reg;
> +	priv->spi_tx_buf[2] = val;
> +
> +	mcp251x_spi_trans(spi, 3);
> +
> +	mutex_unlock(&priv->spi_lock);
> +}
> +
> +static void mcp251x_write_bits(struct spi_device *spi, u8 reg,
> +			       u8 mask, uint8_t val)
> +{
> +	struct mcp251x_priv *priv = dev_get_drvdata(&spi->dev);
> +
> +	mutex_lock(&priv->spi_lock);
> +
> +	priv->spi_tx_buf[0] = INSTRUCTION_BIT_MODIFY;
> +	priv->spi_tx_buf[1] = reg;
> +	priv->spi_tx_buf[2] = mask;
> +	priv->spi_tx_buf[3] = val;
> +
> +	mcp251x_spi_trans(spi, 4);
> +
> +	mutex_unlock(&priv->spi_lock);
> +}
> +
> +static void mcp251x_hw_tx_frame(struct spi_device *spi, u8 *data,

s/data/buf/ to avoid confusion with the CAN payload data.

> +				int len, int tx_buf_idx)
> +{
> +	struct mcp251x_platform_data *pdata = spi->dev.platform_data;
> +	struct mcp251x_priv *priv = dev_get_drvdata(&spi->dev);
> +
> +	if (pdata->model == CAN_MCP251X_MCP2510) {
> +		int i;
> +
> +		for (i = 1; i < TXBDAT_OFF + len; i++)
> +			mcp251x_write_reg(spi, TXBCTRL(tx_buf_idx) + i,
> +					  data[i]);
> +	} else {
> +		mutex_lock(&priv->spi_lock);
> +		memcpy(priv->spi_tx_buf, data, TXBDAT_OFF + len);
> +		mcp251x_spi_trans(spi, TXBDAT_OFF + len);
> +		mutex_unlock(&priv->spi_lock);
> +	}
> +}
> +
> +static void mcp251x_hw_tx(struct spi_device *spi, struct can_frame *frame,
> +			  int tx_buf_idx)
> +{
> +	u32 sid, eid, exide, rtr;
> +	u8 buf[SPI_TRANSFER_BUF_LEN];
> +
> +	exide = (frame->can_id & CAN_EFF_FLAG) ? 1 : 0; /* Extended ID Enable */
> +	if (exide)
> +		sid = (frame->can_id & CAN_EFF_MASK) >> 18;
> +	else
> +		sid = frame->can_id & CAN_SFF_MASK; /* Standard ID */
> +	eid = frame->can_id & CAN_EFF_MASK; /* Extended ID */
> +	rtr = (frame->can_id & CAN_RTR_FLAG) ? 1 : 0; /* Remote transmission */
> +
> +	buf[TXBCTRL_OFF] = INSTRUCTION_LOAD_TXB(tx_buf_idx);
> +	buf[TXBSIDH_OFF] = sid >> SIDH_SHIFT;
> +	buf[TXBSIDL_OFF] = ((sid & SIDL_SID_MASK) << SIDL_SID_SHIFT) |
> +		(exide << SIDL_EXIDE_SHIFT) |
> +		((eid >> SIDL_EID_SHIFT) & SIDL_EID_MASK);
> +	buf[TXBEID8_OFF] = GET_BYTE(eid, 1);
> +	buf[TXBEID0_OFF] = GET_BYTE(eid, 0);
> +	buf[TXBDLC_OFF]  = (rtr << DLC_RTR_SHIFT) | frame->can_dlc;

Maybe you understand now my comment about using structs, e.g. for buf,
at the beginning.

> +	memcpy(buf + TXBDAT_OFF, frame->data, frame->can_dlc);
> +	mcp251x_hw_tx_frame(spi, buf, frame->can_dlc, tx_buf_idx);
> +	mcp251x_write_reg(spi, TXBCTRL(tx_buf_idx), TXBCTRL_TXREQ);
> +}
> +
> +static void mcp251x_hw_rx_frame(struct spi_device *spi, u8 *data,

s/data/buf/, see above.

> +				int buf_idx)
> +{
> +	struct mcp251x_priv *priv = dev_get_drvdata(&spi->dev);
> +	struct mcp251x_platform_data *pdata = spi->dev.platform_data;
> +
> +	if (pdata->model == CAN_MCP251X_MCP2510) {
> +		int i, len;
> +
> +		for (i = 1; i < RXBDAT_OFF; i++)
> +			data[i] = mcp251x_read_reg(spi, RXBCTRL(buf_idx) + i);
> +		len = data[RXBDLC_OFF] & RXBDLC_LEN_MASK;
> +		if (len > 8)
> +			len = 8;
> +		for (; i < (RXBDAT_OFF + len); i++)
> +			data[i] = mcp251x_read_reg(spi, RXBCTRL(buf_idx) + i);
> +	} else {
> +		mutex_lock(&priv->spi_lock);
> +
> +		priv->spi_tx_buf[RXBCTRL_OFF] = INSTRUCTION_READ_RXB(buf_idx);
> +		mcp251x_spi_trans(spi, SPI_TRANSFER_BUF_LEN);
> +		memcpy(data, priv->spi_rx_buf, SPI_TRANSFER_BUF_LEN);
> +
> +		mutex_unlock(&priv->spi_lock);
> +	}
> +}
> +
> +static void mcp251x_hw_rx(struct spi_device *spi, int buf_idx)
> +{
> +	struct mcp251x_priv *priv = dev_get_drvdata(&spi->dev);
> +	struct sk_buff *skb;
> +	struct can_frame *frame;
> +	u8 buf[SPI_TRANSFER_BUF_LEN];
> +
> +	skb = alloc_can_skb(priv->net, &frame);
> +	if (!skb) {
> +		dev_err(&spi->dev, "cannot allocate RX skb\n");
> +		priv->net->stats.rx_dropped++;
> +		return;
> +	}
> +
> +	mcp251x_hw_rx_frame(spi, buf, buf_idx);
> +	if (buf[RXBSIDL_OFF] & RXBSIDL_IDE) {
> +		/* Extended ID format */
> +		frame->can_id = CAN_EFF_FLAG;
> +		frame->can_id |=
> +			/* Extended ID part */
> +			SET_BYTE(buf[RXBSIDL_OFF] & RXBSIDL_EID, 2) |
> +			SET_BYTE(buf[RXBEID8_OFF],               1) |
> +			SET_BYTE(buf[RXBEID0_OFF],               0) |

Please don't align arguments or variables.

> +			/* Standard ID part */
> +			(((buf[RXBSIDH_OFF] << RXBSIDH_SHIFT) |
> +			  (buf[RXBSIDL_OFF] >> RXBSIDL_SHIFT)) << 18);
> +		if (buf[RXBDLC_OFF] & RXBDLC_RTR) {
> +			/* Remote transmission request */
> +			frame->can_id |= CAN_RTR_FLAG;
> +		}

Remove {}, please.

> +	} else
> +		/* Standard ID format */
> +		frame->can_id =
> +			(buf[RXBSIDH_OFF] << RXBSIDH_SHIFT) |
> +			(buf[RXBSIDL_OFF] >> RXBSIDL_SHIFT);

Please use {} here as well.

> +	/* Data length */
> +	frame->can_dlc = buf[RXBDLC_OFF] & RXBDLC_LEN_MASK;
> +	if (frame->can_dlc > 8) {
> +		dev_warn(&spi->dev, "invalid frame recevied\n");
> +		priv->net->stats.rx_errors++;
> +		dev_kfree_skb(skb);
> +		return;
> +	}
> +	memcpy(frame->data, buf + RXBDAT_OFF, frame->can_dlc);
> +
> +	priv->net->stats.rx_packets++;
> +	priv->net->stats.rx_bytes += frame->can_dlc;
> +	netif_rx(skb);
> +}
> +
> +static void mcp251x_hw_sleep(struct spi_device *spi)
> +{
> +	mcp251x_write_reg(spi, CANCTRL, CANCTRL_REQOP_SLEEP);
> +}
> +
> +static void mcp251x_hw_wakeup(struct spi_device *spi)
> +{
> +	struct mcp251x_priv *priv = dev_get_drvdata(&spi->dev);
> +
> +	priv->wake = 1;
> +
> +	/* Can only wake up by generating a wake-up interrupt. */
> +	mcp251x_write_bits(spi, CANINTE, CANINTE_WAKIE, CANINTE_WAKIE);
> +	mcp251x_write_bits(spi, CANINTF, CANINTF_WAKIF, CANINTF_WAKIF);
> +
> +	/* Wait until the device is awake */
> +	if (!wait_for_completion_timeout(&priv->awake, HZ))
> +		dev_err(&spi->dev, "MCP251x didn't wake-up\n");
> +}
> +
> +static netdev_tx_t mcp251x_hard_start_xmit(struct sk_buff *skb,
> +					   struct net_device *net)
> +{
> +	struct mcp251x_priv *priv = netdev_priv(net);
> +	struct spi_device *spi = priv->spi;
> +
> +	if (priv->tx_skb || priv->tx_len) {
> +		dev_warn(&spi->dev, "hard_xmit called while tx busy\n");
> +		netif_stop_queue(net);
> +		return NETDEV_TX_BUSY;
> +	}
> +
> +	if (skb->len != sizeof(struct can_frame)) {
> +		dev_err(&spi->dev, "dropping packet - bad length\n");
> +		dev_kfree_skb(skb);
> +		net->stats.tx_dropped++;
> +		return 0;

return NETDEV_TX_OK; ?

> +	}
> +
> +	netif_stop_queue(net);
> +	priv->tx_skb = skb;
> +	net->trans_start = jiffies;
> +	queue_work(priv->wq, &priv->tx_work);
> +
> +	return NETDEV_TX_OK;
> +}
> +
> +static int mcp251x_do_set_mode(struct net_device *net, enum can_mode mode)
> +{
> +	struct mcp251x_priv *priv = netdev_priv(net);
> +
> +	switch (mode) {
> +	case CAN_MODE_START:
> +		/* We have to delay work since SPI I/O may sleep */
> +		priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +		priv->restart_tx = 1;
> +		if (priv->can.restart_ms == 0)
> +			priv->after_suspend = AFTER_SUSPEND_RESTART;
> +		queue_work(priv->wq, &priv->irq_work);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static void mcp251x_set_normal_mode(struct spi_device *spi)
> +{
> +	struct mcp251x_priv *priv = dev_get_drvdata(&spi->dev);
> +	unsigned long timeout;
> +
> +	/* Enable interrupts */
> +	mcp251x_write_reg(spi, CANINTE,
> +			  CANINTE_ERRIE | CANINTE_TX2IE | CANINTE_TX1IE |
> +			  CANINTE_TX0IE | CANINTE_RX1IE | CANINTE_RX0IE |
> +			  CANINTF_MERRF);
> +
> +	if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> +		/* Put device into loopback mode */
> +		mcp251x_write_reg(spi, CANCTRL, CANCTRL_REQOP_LOOPBACK);

Please use brackets here as well. See:

http://lxr.linux.no/#linux+v2.6.31/Documentation/CodingStyle#L171

> +	else {
> +		/* Put device into normal mode */
> +		mcp251x_write_reg(spi, CANCTRL, CANCTRL_REQOP_NORMAL);
> +
> +		/* Wait for the device to enter normal mode */
> +		timeout = jiffies + HZ;
> +		while (mcp251x_read_reg(spi, CANSTAT) & CANCTRL_REQOP_MASK) {
> +			schedule();
> +			if (time_after(jiffies, timeout)) {
> +				dev_err(&spi->dev, "MCP251x didn't"
> +					" enter in normal mode\n");
> +				return;
> +			}
> +		}
> +	}
> +	priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +}
> +
> +static int mcp251x_do_set_bittiming(struct net_device *net)
> +{
> +	struct mcp251x_priv *priv = netdev_priv(net);
> +	struct can_bittiming *bt = &priv->can.bittiming;
> +	struct spi_device *spi = priv->spi;
> +	u8 state;
> +
> +	/* Store original mode and set mode to config */

Do you need that. The bit-timing can only be set when the device is
stopped (down).

> +	state = mcp251x_read_reg(spi, CANSTAT) & CANCTRL_REQOP_MASK;
> +	mcp251x_write_bits(spi, CANCTRL, CANCTRL_REQOP_MASK,
> +			   CANCTRL_REQOP_CONF);
> +
> +	mcp251x_write_reg(spi, CNF1, ((bt->sjw - 1) << CNF1_SJW_SHIFT) |
> +			  (bt->brp - 1));
> +	mcp251x_write_reg(spi, CNF2, CNF2_BTLMODE |
> +			  (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES ?
> +			   CNF2_SAM : 0) |
> +			  ((bt->phase_seg1 - 1) << CNF2_PS1_SHIFT) |
> +			  (bt->prop_seg - 1));
> +	mcp251x_write_bits(spi, CNF3, CNF3_PHSEG2_MASK,
> +			   (bt->phase_seg2 - 1));
> +	dev_info(&spi->dev, "CNF: 0x%02x 0x%02x 0x%02x\n",
> +		 mcp251x_read_reg(spi, CNF1),
> +		 mcp251x_read_reg(spi, CNF2),
> +		 mcp251x_read_reg(spi, CNF3));
> +	/* Restore original state */
> +	mcp251x_write_bits(spi, CANCTRL, CANCTRL_REQOP_MASK, state);
> +
> +	return 0;
> +}
> +
> +static int mcp251x_setup(struct net_device *net, struct mcp251x_priv *priv,
> +			 struct spi_device *spi)
> +{
> +	int ret;
> +
> +	ret = open_candev(net);
> +	if (ret) {
> +		dev_err(&spi->dev, "unable to set initial baudrate!\n");
> +		return ret;
> +	}
> +
> +	/* Enable RX0->RX1 buffer roll over and disable filters */
> +	mcp251x_write_bits(spi, RXBCTRL(0),
> +			   RXBCTRL_BUKT | RXBCTRL_RXM0 | RXBCTRL_RXM1,
> +			   RXBCTRL_BUKT | RXBCTRL_RXM0 | RXBCTRL_RXM1);
> +	mcp251x_write_bits(spi, RXBCTRL(1),
> +			   RXBCTRL_RXM0 | RXBCTRL_RXM1,
> +			   RXBCTRL_RXM0 | RXBCTRL_RXM1);
> +	return 0;
> +}
> +
> +static void mcp251x_hw_reset(struct spi_device *spi)
> +{
> +	struct mcp251x_priv *priv = dev_get_drvdata(&spi->dev);
> +	int ret;
> +
> +	mutex_lock(&priv->spi_lock);
> +
> +	priv->spi_tx_buf[0] = INSTRUCTION_RESET;
> +
> +	ret = spi_write(spi, priv->spi_tx_buf, 1);
> +
> +	mutex_unlock(&priv->spi_lock);
> +
> +	if (ret < 0)

if (ret) ?

> +		dev_err(&spi->dev, "reset failed: ret = %d\n", ret);
> +	/* Wait for reset to finish */
> +	mdelay(10);
> +}
> +
> +static int mcp251x_hw_probe(struct spi_device *spi)
> +{
> +	int st1, st2;
> +
> +	mcp251x_hw_reset(spi);
> +
> +	/*
> +	 * Please note that these are "magic values" based on after
> +	 * reset defaults taken from data sheet which allows us to see
> +	 * if we really have a chip on the bus (we avoid common all
> +	 * zeroes or all ones situations)
> +	 */
> +	st1 = mcp251x_read_reg(spi, CANSTAT) & 0xEE;
> +	st2 = mcp251x_read_reg(spi, CANCTRL) & 0x17;
> +
> +	dev_dbg(&spi->dev, "CANSTAT 0x%02x CANCTRL 0x%02x\n", st1, st2);
> +
> +	/* Check for power up default values */
> +	return (st1 == 0x80 && st2 == 0x07) ? 1 : 0;
> +}
> +
> +static irqreturn_t mcp251x_can_isr(int irq, void *dev_id)
> +{
> +	struct net_device *net = (struct net_device *)dev_id;
> +	struct mcp251x_priv *priv = netdev_priv(net);
> +
> +	/* Schedule bottom half */
> +	if (!work_pending(&priv->irq_work))
> +		queue_work(priv->wq, &priv->irq_work);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mcp251x_open(struct net_device *net)
> +{
> +	struct mcp251x_priv *priv = netdev_priv(net);
> +	struct spi_device *spi = priv->spi;
> +	struct mcp251x_platform_data *pdata = spi->dev.platform_data;
> +	int ret;
> +
> +	if (pdata->transceiver_enable)
> +		pdata->transceiver_enable(1);
> +
> +	priv->force_quit = 0;
> +	priv->tx_skb = NULL;
> +	priv->tx_len = 0;
> +
> +	ret = request_irq(spi->irq, mcp251x_can_isr,
> +			  IRQF_TRIGGER_FALLING, DEVICE_NAME, net);
> +	if (ret < 0) {

"if (ret)" ?

> +		dev_err(&spi->dev, "failed to acquire irq %d\n", spi->irq);
> +		return ret;
> +	}
> +
> +	mcp251x_hw_wakeup(spi);
> +	mcp251x_hw_reset(spi);
> +	ret = mcp251x_setup(net, priv, spi);
> +	if (ret < 0) {

"if (ret)" ?

> +		disable_irq(spi->irq);

free_irq? And what about the transeiver? The usual goto cleanup method
would make sense here.

> +		return ret;
> +	}
> +	mcp251x_set_normal_mode(spi);
> +	netif_wake_queue(net);
> +
> +	return 0;
> +}
> +
> +static int mcp251x_stop(struct net_device *net)
> +{
> +	struct mcp251x_priv *priv = netdev_priv(net);
> +	struct spi_device *spi = priv->spi;
> +	struct mcp251x_platform_data *pdata = spi->dev.platform_data;
> +
> +	/* Disable and clear pending interrupts */
> +	mcp251x_write_reg(spi, CANINTE, 0x00);
> +	mcp251x_write_reg(spi, CANINTF, 0x00);
> +
> +	priv->force_quit = 1;
> +	disable_irq(spi->irq);

Why not freeing the irq already here?

> +	flush_workqueue(priv->wq);
> +
> +	mcp251x_write_reg(spi, TXBCTRL(0), 0);

Hm, but you still need the interrupt!?

> +	if (priv->tx_skb || priv->tx_len)
> +		mcp251x_clean(net);
> +
> +	mcp251x_hw_sleep(spi);
> +
> +	free_irq(spi->irq, net);
> +
> +	if (pdata->transceiver_enable)
> +		pdata->transceiver_enable(0);
> +
> +	priv->can.state = CAN_STATE_STOPPED;
> +	close_candev(net);

You should call close_candev early to cancel the buf-off recovery timer.

> +	return 0;
> +}
> +
> +static void mcp251x_tx_work_handler(struct work_struct *ws)
> +{
> +	struct mcp251x_priv *priv = container_of(ws, struct mcp251x_priv,
> +						 tx_work);
> +	struct spi_device *spi = priv->spi;
> +	struct net_device *net = priv->net;
> +	struct can_frame *frame;
> +
> +	if (priv->tx_skb) {
> +		frame = (struct can_frame *)priv->tx_skb->data;
> +
> +		if (priv->can.state == CAN_STATE_BUS_OFF) {
> +			mcp251x_clean(net);
> +			netif_wake_queue(net);
> +			return;
> +		}
> +		if (frame->can_dlc > CAN_FRAME_MAX_DATA_LEN)
> +			frame->can_dlc = CAN_FRAME_MAX_DATA_LEN;
> +		mcp251x_hw_tx(spi, frame, 0);
> +		priv->tx_len = 1 + frame->can_dlc;
> +		can_put_echo_skb(priv->tx_skb, net, 0);
> +		priv->tx_skb = NULL;
> +	}
> +}
> +
> +static void mcp251x_irq_work_handler(struct work_struct *ws)
> +{
> +	struct mcp251x_priv *priv = container_of(ws, struct mcp251x_priv,
> +						 irq_work);
> +	struct spi_device *spi = priv->spi;
> +	struct net_device *net = priv->net;
> +	u8 txbnctrl;
> +	u8 intf;
> +	enum can_state new_state;
> +
> +	if (priv->after_suspend) {
> +		mdelay(10);
> +		mcp251x_hw_reset(spi);
> +		mcp251x_setup(net, priv, spi);
> +		if (priv->after_suspend & AFTER_SUSPEND_RESTART)
> +			mcp251x_set_normal_mode(spi);

Please use {} here as well.

> +		else if (priv->after_suspend & AFTER_SUSPEND_UP) {
> +			netif_device_attach(net);
> +			/* Clean since we lost tx buffer */
> +			if (priv->tx_skb || priv->tx_len) {
> +				mcp251x_clean(net);
> +				netif_wake_queue(net);
> +			}
> +			mcp251x_set_normal_mode(spi);
> +		} else
> +			mcp251x_hw_sleep(spi);

Please use {} here as well.

> +		priv->after_suspend = 0;
> +	}
> +
> +	if (priv->can.restart_ms == 0 && priv->can.state == CAN_STATE_BUS_OFF) {
> +		while (!priv->force_quit && !freezing(current) &&
> +		       (intf = mcp251x_read_reg(spi, CANINTF)))
> +			mcp251x_write_bits(spi, CANINTF, intf, 0x00);
> +		return;
> +	}
> +
> +	while (!priv->force_quit && !freezing(current)) {
> +		u8 eflag = mcp251x_read_reg(spi, EFLG);
> +		int can_id = 0, data1 = 0;
> +
> +		mcp251x_write_reg(spi, EFLG, 0x00);
> +
> +		if (priv->restart_tx) {
> +			priv->restart_tx = 0;
> +			mcp251x_write_reg(spi, TXBCTRL(0), 0);
> +			if (priv->tx_skb || priv->tx_len)
> +				mcp251x_clean(net);
> +			netif_wake_queue(net);
> +			can_id |= CAN_ERR_RESTARTED;
> +		}
> +
> +		if (priv->wake) {
> +			/* Wait whilst the device wakes up */
> +			mdelay(10);
> +			priv->wake = 0;
> +		}
> +
> +		intf = mcp251x_read_reg(spi, CANINTF);
> +		mcp251x_write_bits(spi, CANINTF, intf, 0x00);
> +
> +		/* Update can state */
> +		if (eflag & EFLG_TXBO) {
> +			new_state = CAN_STATE_BUS_OFF;
> +			can_id |= CAN_ERR_BUSOFF;
> +		} else if (eflag & EFLG_TXEP) {
> +			new_state = CAN_STATE_ERROR_PASSIVE;
> +			can_id |= CAN_ERR_CRTL;
> +			data1 |= CAN_ERR_CRTL_TX_PASSIVE;
> +		} else if (eflag & EFLG_RXEP) {
> +			new_state = CAN_STATE_ERROR_PASSIVE;
> +			can_id |= CAN_ERR_CRTL;
> +			data1 |= CAN_ERR_CRTL_RX_PASSIVE;
> +		} else if (eflag & EFLG_TXWAR) {
> +			new_state = CAN_STATE_ERROR_WARNING;
> +			can_id |= CAN_ERR_CRTL;
> +			data1 |= CAN_ERR_CRTL_TX_WARNING;
> +		} else if (eflag & EFLG_RXWAR) {
> +			new_state = CAN_STATE_ERROR_WARNING;
> +			can_id |= CAN_ERR_CRTL;
> +			data1 |= CAN_ERR_CRTL_RX_WARNING;
> +		} else
> +			new_state = CAN_STATE_ERROR_ACTIVE;

Please use {} here as well.

> +
> +		/* Update can state statistics */
> +		switch (priv->can.state) {
> +		case CAN_STATE_ERROR_ACTIVE:
> +			if (new_state >= CAN_STATE_ERROR_WARNING &&
> +			    new_state <= CAN_STATE_BUS_OFF)
> +				priv->can.can_stats.error_warning++;
> +		case CAN_STATE_ERROR_WARNING:	/* fallthrough */
> +			if (new_state >= CAN_STATE_ERROR_PASSIVE &&
> +			    new_state <= CAN_STATE_BUS_OFF)
> +				priv->can.can_stats.error_passive++;
> +			break;
> +		default:
> +			break;
> +		}
> +		priv->can.state = new_state;
> +
> +		if ((intf & CANINTF_ERRIF) || (can_id & CAN_ERR_RESTARTED)) {
> +			struct sk_buff *skb;
> +			struct can_frame *frame;
> +
> +			/* Create error frame */
> +			skb = alloc_can_err_skb(net, &frame);
> +			if (skb) {
> +				/* Set error frame flags based on bus state */
> +				frame->can_id = can_id;
> +				frame->data[1] = data1;
> +
> +				/* Update net stats for overflows */
> +				if (eflag & (EFLG_RX0OVR | EFLG_RX1OVR)) {
> +					if (eflag & EFLG_RX0OVR)
> +						net->stats.rx_over_errors++;
> +					if (eflag & EFLG_RX1OVR)
> +						net->stats.rx_over_errors++;
> +					frame->can_id |= CAN_ERR_CRTL;
> +					frame->data[1] |=
> +						CAN_ERR_CRTL_RX_OVERFLOW;
> +				}
> +
> +				netif_rx(skb);
> +			} else
> +				dev_info(&spi->dev,
> +					 "cannot allocate error skb\n");

Please use {} here as well.

> +		}
> +
> +		if (priv->can.state == CAN_STATE_BUS_OFF) {
> +			if (priv->can.restart_ms == 0) {
> +				can_bus_off(net);
> +				mcp251x_hw_sleep(spi);
> +				return;
> +			}
> +		}
> +
> +		if (intf == 0)
> +			break;
> +
> +		if (intf & CANINTF_WAKIF)
> +			complete(&priv->awake);
> +
> +		if (intf & CANINTF_MERRF) {
> +			/* If there are pending Tx buffers, restart queue */
> +			txbnctrl = mcp251x_read_reg(spi, TXBCTRL(0));
> +			if (!(txbnctrl & TXBCTRL_TXREQ)) {
> +				if (priv->tx_skb || priv->tx_len)
> +					mcp251x_clean(net);
> +				netif_wake_queue(net);
> +			}
> +		}
> +
> +		if (intf & (CANINTF_TX2IF | CANINTF_TX1IF | CANINTF_TX0IF)) {
> +			net->stats.tx_packets++;
> +			net->stats.tx_bytes += priv->tx_len - 1;
> +			if (priv->tx_len) {
> +				can_get_echo_skb(net, 0);
> +				priv->tx_len = 0;
> +			}
> +			netif_wake_queue(net);
> +		}
> +
> +		if (intf & CANINTF_RX0IF)
> +			mcp251x_hw_rx(spi, 0);
> +
> +		if (intf & CANINTF_RX1IF)
> +			mcp251x_hw_rx(spi, 1);
> +	}
> +}
> +
> +static const struct net_device_ops mcp251x_netdev_ops = {
> +	.ndo_open	= mcp251x_open,
> +	.ndo_stop	= mcp251x_stop,
> +	.ndo_start_xmit	= mcp251x_hard_start_xmit,
> +};
> +
> +static struct net_device
> +*alloc_mcp251x_netdev(int sizeof_priv,
> +		      struct mcp251x_platform_data *pdata)

Add __devinit or, even better, put the code into mcp251x_can_probe?

> +{
> +	struct net_device *net;
> +	struct mcp251x_priv *priv;
> +
> +	net = alloc_candev(sizeof_priv, TX_ECHO_SKB_MAX);
> +	if (!net)
> +		return NULL;
> +
> +	priv = netdev_priv(net);
> +
> +	net->netdev_ops		= &mcp251x_netdev_ops;
> +	net->flags		|= IFF_ECHO;
> +
> +	priv->can.bittiming_const = &mcp251x_bittiming_const;
> +	priv->can.do_set_mode	  = mcp251x_do_set_mode;
> +	priv->can.clock.freq      = pdata->oscillator_frequency / 2;
> +	priv->can.do_set_bittiming	= mcp251x_do_set_bittiming;

Don't align expressions. Use just *one* space before and after "=".

> +
> +	priv->net = net;
> +
> +	return net;
> +}
> +
> +static int __devinit mcp251x_can_probe(struct spi_device *spi)
> +{
> +	struct net_device *net;
> +	struct mcp251x_priv *priv;
> +	struct mcp251x_platform_data *pdata = spi->dev.platform_data;
> +	int ret = -ENODEV;
> +
> +	if (!pdata)
> +		/* Platform data is required for osc freq */
> +		goto error_out;
> +
> +	/* Allocate can/net device */
> +	net = alloc_mcp251x_netdev(sizeof(struct mcp251x_priv), pdata);
> +	if (!net) {
> +		ret = -ENOMEM;
> +		goto error_alloc;
> +	}
> +
> +	priv = netdev_priv(net);
> +	dev_set_drvdata(&spi->dev, priv);
> +
> +	priv->spi = spi;
> +	mutex_init(&priv->spi_lock);
> +
> +	/* If requested, allocate DMA buffers */
> +	if (mcp251x_enable_dma) {
> +		spi->dev.coherent_dma_mask = ~0;
> +
> +		/*
> +		 * Minimum coherent DMA allocation is PAGE_SIZE, so allocate
> +		 * that much and share it between Tx and Rx DMA buffers.
> +		 */
> +		priv->spi_tx_buf = dma_alloc_coherent(&spi->dev,
> +						      PAGE_SIZE,
> +						      &priv->spi_tx_dma,
> +						      GFP_DMA);
> +
> +		if (priv->spi_tx_buf) {
> +			priv->spi_rx_buf = (u8 *)(priv->spi_tx_buf +
> +						  (PAGE_SIZE / 2));
> +			priv->spi_rx_dma = (dma_addr_t)(priv->spi_tx_dma +
> +							(PAGE_SIZE / 2));
> +		} else
> +			/* Fall back to non-DMA */
> +			mcp251x_enable_dma = 0;

Please use {} here as well.

> +	}
> +
> +	/* Allocate non-DMA buffers */
> +	if (!mcp251x_enable_dma) {
> +		priv->spi_tx_buf = kmalloc(SPI_TRANSFER_BUF_LEN, GFP_KERNEL);
> +		if (!priv->spi_tx_buf) {
> +			ret = -ENOMEM;
> +			goto error_tx_buf;
> +		}
> +		priv->spi_rx_buf = kmalloc(SPI_TRANSFER_BUF_LEN, GFP_KERNEL);
> +		if (!priv->spi_tx_buf) {
> +			ret = -ENOMEM;
> +			goto error_rx_buf;
> +		}
> +	}
> +
> +	if (pdata->power_enable)
> +		pdata->power_enable(1);
> +
> +	/* Call out to platform specific setup */
> +	if (pdata->board_specific_setup)
> +		pdata->board_specific_setup(spi);
> +
> +	SET_NETDEV_DEV(net, &spi->dev);
> +
> +	priv->wq = create_freezeable_workqueue("mcp251x_wq");
> +
> +	INIT_WORK(&priv->tx_work, mcp251x_tx_work_handler);
> +	INIT_WORK(&priv->irq_work, mcp251x_irq_work_handler);
> +
> +	init_completion(&priv->awake);
> +
> +	/* Configure the SPI bus */
> +	spi->mode = SPI_MODE_0;
> +	spi->bits_per_word = 8;
> +	spi_setup(spi);
> +
> +	if (!mcp251x_hw_probe(spi)) {
> +		dev_info(&spi->dev, "Probe failed\n");
> +		goto error_probe;
> +	}
> +	mcp251x_hw_sleep(spi);
> +
> +	if (pdata->transceiver_enable)
> +		pdata->transceiver_enable(0);
> +
> +	ret = register_candev(net);
> +	if (ret >= 0) {
> +		dev_info(&spi->dev, "probed\n");
> +		return ret;
> +	}
> +error_probe:
> +	if (!mcp251x_enable_dma)
> +		kfree(priv->spi_rx_buf);
> +error_rx_buf:
> +	if (!mcp251x_enable_dma)
> +		kfree(priv->spi_tx_buf);
> +error_tx_buf:
> +	free_candev(net);
> +	if (mcp251x_enable_dma)
> +		dma_free_coherent(&spi->dev, PAGE_SIZE,
> +				  priv->spi_tx_buf, priv->spi_tx_dma);
> +error_alloc:
> +	dev_err(&spi->dev, "probe failed\n");
> +error_out:
> +	return ret;
> +}
> +
> +static int __devexit mcp251x_can_remove(struct spi_device *spi)
> +{
> +	struct mcp251x_platform_data *pdata = spi->dev.platform_data;
> +	struct mcp251x_priv *priv = dev_get_drvdata(&spi->dev);
> +	struct net_device *net = priv->net;
> +
> +	unregister_candev(net);
> +	free_candev(net);
> +
> +	priv->force_quit = 1;
> +	flush_workqueue(priv->wq);
> +	destroy_workqueue(priv->wq);
> +
> +	if (mcp251x_enable_dma)
> +		dma_free_coherent(&spi->dev, PAGE_SIZE,
> +				  priv->spi_tx_buf, priv->spi_tx_dma);

Please use {} here as well.

> +	else {
> +		kfree(priv->spi_tx_buf);
> +		kfree(priv->spi_rx_buf);
> +	}
> +
> +	if (pdata->power_enable)
> +		pdata->power_enable(0);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int mcp251x_can_suspend(struct spi_device *spi, pm_message_t state)
> +{
> +	struct mcp251x_platform_data *pdata = spi->dev.platform_data;
> +	struct mcp251x_priv *priv = dev_get_drvdata(&spi->dev);
> +	struct net_device *net = priv->net;
> +
> +	if (netif_running(net)) {
> +		netif_device_detach(net);
> +
> +		mcp251x_hw_sleep(spi);
> +		if (pdata->transceiver_enable)
> +			pdata->transceiver_enable(0);
> +		priv->after_suspend = AFTER_SUSPEND_UP;
> +	} else
> +		priv->after_suspend = AFTER_SUSPEND_DOWN;

Please use {} here as well.

> +
> +	if (pdata->power_enable) {
> +		pdata->power_enable(0);
> +		priv->after_suspend |= AFTER_SUSPEND_POWER;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mcp251x_can_resume(struct spi_device *spi)
> +{
> +	struct mcp251x_platform_data *pdata = spi->dev.platform_data;
> +	struct mcp251x_priv *priv = dev_get_drvdata(&spi->dev);
> +
> +	if (priv->after_suspend & AFTER_SUSPEND_POWER) {
> +		pdata->power_enable(1);
> +		queue_work(priv->wq, &priv->irq_work);
> +	} else {
> +		if (priv->after_suspend & AFTER_SUSPEND_UP) {
> +			if (pdata->transceiver_enable)
> +				pdata->transceiver_enable(1);
> +			queue_work(priv->wq, &priv->irq_work);
> +		} else
> +			priv->after_suspend = 0;

Please use {} here as well and check for similar cases. I might not have
spotted all.

> +	}
> +	return 0;
> +}
> +#else
> +#define mcp251x_can_suspend NULL
> +#define mcp251x_can_resume NULL
> +#endif
> +
> +static struct spi_driver mcp251x_can_driver = {
> +	.driver = {
> +		.name		= DEVICE_NAME,
> +		.bus		= &spi_bus_type,
> +		.owner		= THIS_MODULE,
> +	},
> +
> +	.probe		= mcp251x_can_probe,
> +	.remove		= __devexit_p(mcp251x_can_remove),
> +	.suspend	= mcp251x_can_suspend,
> +	.resume		= mcp251x_can_resume,

Use just *one* space before and after "=".

> +};
> +
> +static int __init mcp251x_can_init(void)
> +{
> +	return spi_register_driver(&mcp251x_can_driver);
> +}
> +
> +static void __exit mcp251x_can_exit(void)
> +{
> +	spi_unregister_driver(&mcp251x_can_driver);
> +}
> +
> +module_init(mcp251x_can_init);
> +module_exit(mcp251x_can_exit);
> +
> +MODULE_AUTHOR("Chris Elston <[email-masked]>, "
> +	      "Christian Pellegrin <[email-masked]>");
> +MODULE_DESCRIPTION("Microchip 251x CAN driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/can/platform/mcp251x.h b/include/linux/can/platform/mcp251x.h
> new file mode 100644
> index 0000000..d217ffa
> --- /dev/null
> +++ b/include/linux/can/platform/mcp251x.h
> @@ -0,0 +1,34 @@
> +#ifndef __CAN_PLATFORM_MCP251X_H__
> +#define __CAN_PLATFORM_MCP251X_H__
> +
> +/*
> + *
> + * CAN bus driver for Microchip 251x CAN Controller with SPI Interface
> + *
> + */
> +
> +/**
> + * struct mcp251x_platform_data - MCP251X SPI CAN controller platform data
> + * @oscillator_frequency:       - oscillator frequency in Hz
> + * @model:                      - actual type of chip
> + * @board_specific_setup:       - called before probing the chip (power,reset)
> + * @transceiver_enable:         - called to power on/off the transceiver
> + * @power_enable:               - called to power on/off the mcp *and* the
> + *                                transceiver
> + *
> + * Please note that you should define power_enable or transceiver_enable or
> + * none of them. Defining both of them is no use.
> + *
> + */
> +
> +struct mcp251x_platform_data {
> +	unsigned long oscillator_frequency;
> +	int model;
> +#define CAN_MCP251X_MCP2510 0
> +#define CAN_MCP251X_MCP2515 1
> +	int (*board_specific_setup)(struct spi_device *spi);
> +	int (*transceiver_enable)(int enable);
> +	int (*power_enable) (int enable);
> +};
> +
> +#endif /* __CAN_PLATFORM_MCP251X_H__ */

Thanks,

Wolfgang.