Hi Christian,
here's my quick review of the MPC251x driver. First some general comments:
- Use proper Linux style for comments.
- Use a consistent style for indention of function declarations and
multi-line expressions.
- Use macro definitions for constant values.
- Check the proper usage of brackets { } for if, for, etc.
- Remove extended debugging message.
- Consider replacing ret with err but only if err==success and
!err=failure (minor issue). Then use "if ([!]err) ...".
- Don't use __func__ but a meaning full error message.
More comments inline:
> /*
> * CAN bus driver for Microchip 251x CAN Controller with SPI Interface
> *
> * MCP2510 support and bug fixes by Christian Pellegrin
> * <[email protected]>
> *
> * 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 <socketcan/can.h>
> #include <linux/spi/spi.h>
> #include <socketcan/can/dev.h>
> #include <socketcan/can/core.h>
> #include <linux/if_arp.h>
> #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 <socketcan/can/platform/mcp251x.h>
>
> #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,22)
> #error This driver does not support Kernel versions < 2.6.22
> #endif
>
> /* 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 CNF2 0x29
> # define CNF2_BTLMODE 0x80
> #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)
> # define TXBCTRL_ABTF 0x40
> # define TXBCTRL_MLOA 0x20
> # define TXBCTRL_TXERR 0x10
> # define TXBCTRL_TXREQ 0x08
> #define RXBCTRL(n) ((n * 0x10) + 0x60)
> # define RXBCTRL_BUKT 0x04
> # define RXBCTRL_RXM0 0x20
> # define RXBCTRL_RXM1 0x40
>
> /* Buffer size required for the largest SPI transfer (i.e., reading a
> * frame). */
See general comments.
> #define CAN_FRAME_MAX_DATA_LEN 8
> #define SPI_TRANSFER_BUF_LEN (2*(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 = {
> .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
> int restart_tx;
> };
>
> static void mcp251x_clean(struct mcp251x_priv *priv)
> {
> 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).
> */
See general comments.
> 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)
> dev_err(&spi->dev, "%s: failed: ret = %d\n", __func__, 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(struct spi_device *spi, struct can_frame *frame,
> int tx_buf_idx)
> {
> struct mcp251x_platform_data *pdata = spi->dev.platform_data;
> struct mcp251x_priv *priv = dev_get_drvdata(&spi->dev);
> u32 sid, eid, exide, rtr;
>
> 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 */
>
> if (pdata->model == CAN_MCP251X_MCP2510) {
> int i;
>
> mcp251x_write_reg(spi, TXBCTRL(tx_buf_idx) + 1, sid >> 3);
> mcp251x_write_reg(spi, TXBCTRL(tx_buf_idx) + 2,
> ((sid & 7) << 5) | (exide << 3) |
> ((eid >> 16) & 3));
> mcp251x_write_reg(spi, TXBCTRL(tx_buf_idx) + 3,
> (eid >> 8) & 0xff);
> mcp251x_write_reg(spi, TXBCTRL(tx_buf_idx) + 4, eid & 0xff);
> mcp251x_write_reg(spi, TXBCTRL(tx_buf_idx) + 5,
> (rtr << 6) | frame->can_dlc);
>
> for (i = 0; i < frame->can_dlc ; i++) {
> mcp251x_write_reg(spi, TXBCTRL(tx_buf_idx) + 6 + i,
> frame->data[i]);
> }
Please remove brackets.
> } else {
> u8 *tx_buf = priv->spi_tx_buf;
>
> mutex_lock(&priv->spi_lock);
>
> tx_buf[0] = INSTRUCTION_LOAD_TXB(tx_buf_idx);
> tx_buf[1] = sid >> 3;
> tx_buf[2] = ((sid & 7) << 5) | (exide << 3) |
> ((eid >> 16) & 3);
> tx_buf[3] = (eid >> 8) & 0xff;
> tx_buf[4] = eid & 0xff;
> tx_buf[5] = (rtr << 6) | frame->can_dlc;
>
> memcpy(tx_buf + 6, frame->data, frame->can_dlc);
>
> mcp251x_spi_trans(spi, 6 + CAN_FRAME_MAX_DATA_LEN);
>
> mutex_unlock(&priv->spi_lock);
> }
I see redundant code above. What about implementing mcp251x_write_regs()
and mcp251x_read_regs() allowing to read and write successive bytes with
one SPI transfer, or many for the MCP2510. This might be used in other
places as well.
> mcp251x_write_reg(spi, TXBCTRL(tx_buf_idx), TXBCTRL_TXREQ);
> }
>
> static void mcp251x_hw_rx(struct spi_device *spi, int buf_idx)
> {
> struct mcp251x_priv *priv = dev_get_drvdata(&spi->dev);
> struct mcp251x_platform_data *pdata = spi->dev.platform_data;
> struct sk_buff *skb;
> struct can_frame *frame;
>
> skb = alloc_can_skb(priv->net, &frame);
> if (!skb) {
> dev_err(&spi->dev, "%s: out of memory for Rx'd frame\n",
> __func__);
> priv->net->stats.rx_dropped++;
> return;
> }
>
> if (pdata->model == CAN_MCP251X_MCP2510) {
> int i;
> u8 rx_buf[6];
>
> rx_buf[1] = mcp251x_read_reg(spi, RXBCTRL(buf_idx) + 1);
> rx_buf[2] = mcp251x_read_reg(spi, RXBCTRL(buf_idx) + 2);
> rx_buf[3] = mcp251x_read_reg(spi, RXBCTRL(buf_idx) + 3);
> rx_buf[4] = mcp251x_read_reg(spi, RXBCTRL(buf_idx) + 4);
> rx_buf[5] = mcp251x_read_reg(spi, RXBCTRL(buf_idx) + 5);
>
> if ((rx_buf[2] >> 3) & 0x1) {
> /* Extended ID format */
> frame->can_id = CAN_EFF_FLAG;
> frame->can_id |= ((rx_buf[2] & 3) << 16) |
> (rx_buf[3] << 8) | rx_buf[4] |
> (((rx_buf[1] << 3) | (rx_buf[2] >> 5)) << 18);
> } else {
> /* Standard ID format */
> frame->can_id = (rx_buf[1] << 3) | (rx_buf[2] >> 5);
> }
Ditto. Also please use macro definitons to make the code more readable.
> if ((rx_buf[5] >> 6) & 0x1) {
> /* Remote transmission request */
> frame->can_id |= CAN_RTR_FLAG;
> }
Please remove brackets.
> /* Data length */
> frame->can_dlc = rx_buf[5] & 0x0f;
> if (frame->can_dlc > 8) {
> dev_warn(&spi->dev, "invalid frame recevied\n");
> priv->net->stats.rx_errors++;
> dev_kfree_skb(skb);
> return;
> }
>
> for (i = 0; i < frame->can_dlc; i++) {
> frame->data[i] = mcp251x_read_reg(spi,
> RXBCTRL(buf_idx) +
> 6 + i);
Please remove brackets.
> }
> } else {
> u8 *tx_buf = priv->spi_tx_buf;
> u8 *rx_buf = priv->spi_rx_buf;
> int ret;
>
> mutex_lock(&priv->spi_lock);
>
> tx_buf[0] = INSTRUCTION_READ_RXB(buf_idx);
>
> ret = mcp251x_spi_trans(spi, 14);
> if (ret < 0) {
> priv->net->stats.rx_errors++;
> mutex_unlock(&priv->spi_lock);
> return;
> }
>
> if ((rx_buf[2] >> 3) & 0x1) {
> /* Extended ID format */
> frame->can_id = CAN_EFF_FLAG;
> frame->can_id |= ((rx_buf[2] & 3) << 16) |
> (rx_buf[3] << 8) | rx_buf[4] |
> (((rx_buf[1] << 3) | (rx_buf[2] >> 5)) << 18);
> } else {
> /* Standard ID format */
> frame->can_id = (rx_buf[1] << 3) | (rx_buf[2] >> 5);
> }
>
> if ((rx_buf[5] >> 6) & 0x1) {
> /* Remote transmission request */
> frame->can_id |= CAN_RTR_FLAG;
> }
Please remove brackets.
>
> /* Data length */
> frame->can_dlc = rx_buf[5] & 0x0f;
> if (frame->can_dlc > 8) {
> dev_warn(&spi->dev, "invalid frame recevied\n");
> priv->net->stats.rx_errors++;
> dev_kfree_skb(skb);
> mutex_unlock(&priv->spi_lock);
> return;
> }
>
> memcpy(frame->data, rx_buf + 6, CAN_FRAME_MAX_DATA_LEN);
>
> mutex_unlock(&priv->spi_lock);
> }
>
> 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");
> }
>
> #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,32)
> static int mcp251x_hard_start_xmit(struct sk_buff *skb, struct net_device
> *net)
> #else
> static netdev_tx_t mcp251x_hard_start_xmit(struct sk_buff *skb,
> struct net_device *net)
> #endif
> {
> 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;
> }
>
> 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->restart_tx = 1;
> 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);
>
> if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
> /* Put device into loopback mode */
> mcp251x_write_reg(spi, CANCTRL, CANCTRL_REQOP_LOOPBACK);
> } 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) & 0xE0) {
> 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;
>
> dev_dbg(&spi->dev, "%s: BRP = %d, PropSeg = %d, PS1 = %d,"
> " PS2 = %d, SJW = %d\n", __func__, bt->brp,
> bt->prop_seg, bt->phase_seg1, bt->phase_seg2,
> bt->sjw);
Just for development?
>
> /* Store original mode and set mode to config */
> state = mcp251x_read_reg(spi, CANCTRL);
> 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) << 6) | (bt->brp - 1));
> mcp251x_write_reg(spi, CNF2, CNF2_BTLMODE |
> ((bt->phase_seg1 - 1) << 3) |
> (bt->prop_seg - 1));
> mcp251x_write_bits(spi, CNF3, CNF3_PHSEG2_MASK,
> (bt->phase_seg2 - 1));
Is triple sampling supported?
Also, please add a dev_info("Setting CNF1=...) call.
> /* 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;
>
> /* Set initial baudrate. Make sure that registers are updated
> always by explicitly calling mcp251x_do_set_bittiming */
> 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)
> dev_err(&spi->dev, "%s: failed: ret = %d\n", __func__, ret);
> /* wait for reset to finish */
> mdelay(10);
> }
>
> static int mcp251x_hw_probe(struct spi_device *spi)
> {
> int st1, st2;
>
> mcp251x_hw_reset(spi);
>
> st1 = mcp251x_read_reg(spi, CANSTAT) & 0xEE;
> st2 = mcp251x_read_reg(spi, CANCTRL) & 0x17;
>
> dev_dbg(&spi->dev, "%s: 0x%02x - 0x%02x\n", __func__,
> st1, st2);
>
> /* check for power up default values */
> return (st1 == 0x80 && st2 == 0x07) ? 1 : 0;
> }
>
> 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);
This driver also need to handle the tranceiver. And this implementation
is quite efficient.
> priv->force_quit = 0;
> priv->tx_skb = NULL;
> priv->tx_len = 0;
> enable_irq(spi->irq);
> mcp251x_hw_wakeup(spi);
> mcp251x_hw_reset(spi);
> ret = mcp251x_setup(net, priv, spi);
> if (ret < 0) {
> disable_irq(spi->irq);
> 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);
> flush_workqueue(priv->wq);
>
> mcp251x_write_reg(spi, TXBCTRL(0), 0);
> if (priv->tx_skb || priv->tx_len) {
> net->stats.tx_errors++;
Do we need to increase tx_dropped as well? Could/should be done in
mcp251x_clean(). What type of TX error is that. As we don't increase
statistics, a dev_dbg message would be nice to have.
> mcp251x_clean(priv);
> }
>
> mcp251x_hw_sleep(spi);
>
> if (pdata->transceiver_enable)
> pdata->transceiver_enable(0);
>
> priv->can.state = CAN_STATE_STOPPED;
> close_candev(net);
>
> 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;
>
> WARN_ON(!priv->tx_skb);
> WARN_ON(priv->tx_len);
> if (priv->tx_skb) {
> frame = (struct can_frame *)priv->tx_skb->data;
> 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 intf;
> u8 txbnctrl;
> enum can_state new_state;
>
> if (priv->after_suspend) {
> /* Wait whilst the device wakes up WARN_ON */
> mdelay(10);
> mcp251x_hw_reset(spi);
> mcp251x_setup(net, priv, spi);
> if (priv->after_suspend & AFTER_SUSPEND_UP) {
> netif_device_attach(net);
> /* clear since we lost tx buffer */
> if (priv->tx_skb || priv->tx_len) {
> net->stats.tx_errors++;
> mcp251x_clean(priv);
> netif_wake_queue(net);
> }
> mcp251x_set_normal_mode(spi);
> } else
> mcp251x_hw_sleep(spi);
Please add brackets above.
> priv->after_suspend = 0;
> return;
> }
>
> while (!priv->force_quit && !freezing(current)) {
> if (priv->restart_tx) {
> priv->restart_tx = 0;
> dev_dbg(&spi->dev,
> "timeout in txing a packet, restarting\n");
> mcp251x_write_reg(spi, TXBCTRL(0), 0);
> if (priv->tx_skb || priv->tx_len) {
> net->stats.tx_errors++;
> mcp251x_clean(priv);
> }
> priv->can.can_stats.restarts++;
> netif_wake_queue(net);
> }
>
> if (priv->wake) {
> /* Wait whilst the device wakes up */
> mdelay(10);
> priv->wake = 0;
> }
>
> intf = mcp251x_read_reg(spi, CANINTF);
> if (intf == 0x00)
> break;
> mcp251x_write_bits(spi, CANINTF, intf, 0x00);
>
> 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)) {
> WARN_ON(priv->tx_skb);
> WARN_ON(!priv->tx_len);
> if (priv->tx_skb || priv->tx_len) {
> net->stats.tx_errors++;
> mcp251x_clean(priv);
> }
> netif_wake_queue(net);
> }
> }
>
> if (intf & CANINTF_ERRIF) {
> struct sk_buff *skb;
> struct can_frame *frame = NULL;
> u8 eflag = mcp251x_read_reg(spi, EFLG);
>
> /* create error frame */
> skb = alloc_can_err_skb(net, &frame);
An error message for !skb would be nice, like for normal messages.
> if (skb) {
> /* Set error frame flags based on bus state */
> if (eflag & EFLG_TXBO) {
> frame->can_id |= CAN_ERR_BUSOFF;
> } else if (eflag & EFLG_TXEP) {
> frame->can_id |= CAN_ERR_CRTL;
> frame->data[1] |=
> CAN_ERR_CRTL_TX_PASSIVE;
> } else if (eflag & EFLG_RXEP) {
> frame->can_id |= CAN_ERR_CRTL;
> frame->data[1] |=
> CAN_ERR_CRTL_RX_PASSIVE;
> } else if (eflag & EFLG_TXWAR) {
> frame->can_id |= CAN_ERR_CRTL;
> frame->data[1] |=
> CAN_ERR_CRTL_TX_WARNING;
> } else if (eflag & EFLG_RXWAR) {
> frame->can_id |= CAN_ERR_CRTL;
> frame->data[1] |=
> CAN_ERR_CRTL_RX_WARNING;
> }
> }
>
> /* update net stats */
> if (eflag & (EFLG_RX0OVR | EFLG_RX1OVR)) {
> if (eflag & EFLG_RX0OVR)
> net->stats.rx_over_errors++;
> if (eflag & EFLG_RX1OVR)
> net->stats.rx_over_errors++;
> if (frame) {
> frame->can_id |= CAN_ERR_CRTL;
> frame->data[1] =
> CAN_ERR_CRTL_RX_OVERFLOW;
> }
> }
>
> /* update can state */
> if (eflag & EFLG_TXBO) {
> new_state = CAN_STATE_BUS_OFF;
> can_bus_off(net);
> } else if (eflag & EFLG_TXEP)
> new_state = CAN_STATE_ERROR_PASSIVE;
> else if (eflag & EFLG_RXEP)
> new_state = CAN_STATE_ERROR_PASSIVE;
> else if (eflag & EFLG_TXWAR)
> new_state = CAN_STATE_ERROR_WARNING;
> else if (eflag & EFLG_RXWAR)
> new_state = CAN_STATE_ERROR_WARNING;
> else
> new_state = CAN_STATE_ERROR_ACTIVE;
Any effort to make the error code handling more compact would be nice.
E.g., this if block could be combined with the above one using a temp.
variable data1. Then: if (skb && data1) {frame->data[1] = data1, ...}
> mcp251x_write_reg(spi, EFLG, 0x00);
>
> if (skb)
> netif_rx(skb);
> } else
> new_state = CAN_STATE_ERROR_ACTIVE;
>
> /* 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;
> case CAN_STATE_BUS_OFF:
> if (new_state <= CAN_STATE_ERROR_PASSIVE)
> netif_carrier_on(net);
In case of automatice bus-off recovery, a RESTARTED error message should
be created (frame->can_id |= CAN_ERR_RESTARTED).
> break;
> default:
> break;
> }
> priv->can.state = new_state;
>
> if (intf & (CANINTF_TX2IF | CANINTF_TX1IF | CANINTF_TX0IF)) {
> net->stats.tx_packets++;
> net->stats.tx_bytes += priv->tx_len - 1;
> WARN_ON(priv->tx_skb);
> WARN_ON(!priv->tx_len);
> if (priv->tx_len) {
> can_get_echo_skb(net, 0);
> priv->tx_len = 0;
> }
> mcp251x_clean(priv);
> netif_wake_queue(net);
> }
>
> if (intf & CANINTF_RX0IF)
> mcp251x_hw_rx(spi, 0);
>
> if (intf & CANINTF_RX1IF)
> mcp251x_hw_rx(spi, 1);
>
> }
>
> mcp251x_read_reg(spi, CANSTAT);
> }
>
> 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;
> }
>
> #if LINUX_VERSION_CODE > KERNEL_VERSION(2,6,28)
> static const struct net_device_ops mcp251x_netdev_ops = {
> .ndo_open = mcp251x_open,
> .ndo_stop = mcp251x_stop,
> .ndo_start_xmit = mcp251x_hard_start_xmit,
> };
> #endif
>
> static struct net_device *alloc_mcp251x_netdev(int sizeof_priv,
> struct mcp251x_platform_data *pdata)
> {
> 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);
>
> #if LINUX_VERSION_CODE > KERNEL_VERSION(2,6,28)
> net->netdev_ops = &mcp251x_netdev_ops;
> #else
> net->open = mcp251x_open;
> net->stop = mcp251x_stop;
> net->hard_start_xmit = mcp251x_hard_start_xmit;
> #endif
> 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;
>
> 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 = DMA_32BIT_MASK;
>
> /* 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;
> }
> }
>
> /* 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);
>
> /* Register IRQ */
> if (request_irq(spi->irq, mcp251x_can_isr,
> IRQF_TRIGGER_FALLING, DEVICE_NAME, net) < 0) {
> dev_err(&spi->dev, "failed to acquire irq %d\n", spi->irq);
> goto error_irq;
> }
Please return the error code of request_irq() in case of error. Also,
consider requesting the irq in the open function (and free it in the
stop function.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
