| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4AED5589.3090106@grandegger.com>
Date: Sun, 01 Nov 2009 10:31:53 +0100
From: Wolfgang Grandegger <wg@...ndegger.com>
To: Christian Pellegrin <chripell@...e.org>
CC: socketcan-core@...ts.berlios.de,
spi-devel-general@...ts.sourceforge.net, netdev@...r.kernel.org,
Paul Thomas <pthomas8589@...il.com>
Subject: Re: [PATCH net-next-2.6] Driver for the Microchip MCP251x SPI CAN
controllers
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 <chripell@...e.org>
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
> + * <chripell@...lware.org>
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 <celston@...alix.com>, "
> + "Christian Pellegrin <chripell@...lware.org>");
> +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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists