Subject: Re: [PATCH] net-next:can: add TI CAN (HECC) driver

From: Wolfgang Grandegger <[email-masked]>
Date: Sat, 29 Aug 2009 09:06:22 +0200

> TI HECC (High End CAN Controller) module is found on many TI devices. It has
> 32 harwdare mailboxes with full implementation of CAN protocol version 2.0B
> and bus speeds up to 1Mbps. The module specifications are available at TI web
> <http://www.ti.com>.
> 
> This driver is tested on OMAP3517 EVM. Suspend/Resume not tested as yet.

Nice driver, even using the NAPI interface. First some general comments.
Please remove debugging code mainly useful for development, e.g. the
CONFIG_DEBUG_FS block, many dev_info calls and special statistics
counters. Also use dev_dbg for the remaining debug messages useful for
the normal user, like state changes. More comments inline.

> 
> Signed-off-by: Anant Gole <[email-masked]>
> ---
>  drivers/net/can/Kconfig                       |    9 +
>  drivers/net/can/Makefile                      |    2 +
>  drivers/net/can/ti_hecc.c                     | 1352 +++++++++++++++++++++++++
>  include/linux/can/platform/ti_hecc_platform.h |   40 +
>  4 files changed, 1403 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/net/can/ti_hecc.c
>  create mode 100644 include/linux/can/platform/ti_hecc_platform.h
> 
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 30ae55d..fae62df 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -75,6 +75,15 @@ config CAN_KVASER_PCI
>           This driver is for the the PCIcanx and PCIcan cards (1, 2 or
>           4 channel) from Kvaser (http://www.kvaser.com).
> 
> +config CAN_TI_HECC
> +        depends on CAN_DEV
> +        tristate "TI High End CAN Controller (HECC)"
> +        default N
> +        ---help---
> +         This driver adds support for TI High End CAN Controller module
> +         found on many TI devices. The specifications of this module are
> +         are available from TI web (http://www.ti.com)
> +
>  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 523a941..d923512 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -9,4 +9,6 @@ can-dev-y                       := dev.o
> 
>  obj-$(CONFIG_CAN_SJA1000)      += sja1000/
> 
> +obj-$(CONFIG_CAN_TI_HECC)      += ti_hecc.o
> +
>  ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
> new file mode 100644
> index 0000000..4741b4a
> --- /dev/null
> +++ b/drivers/net/can/ti_hecc.c
> @@ -0,0 +1,1352 @@
> +/*
> + * TI HECC (CAN) device driver
> + *
> + * This driver supports TI's HECC (High End CAN Controller module) and the
> + * specs for the same is available at <http://www.ti.com>
> + *
> + * Copyright (C) 2009 Texas Instruments Incorporated - http://www.ti.com/
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed as is WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *

It would be nice if you could mention the related platform data here,
similar to the mcp251x.c driver.

> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/sched.h>
> +#include <linux/types.h>
> +#include <linux/fcntl.h>
> +#include <linux/interrupt.h>
> +#include <linux/ptrace.h>
> +#include <linux/string.h>
> +#include <linux/errno.h>
> +#include <linux/netdevice.h>
> +#include <linux/if_arp.h>
> +#include <linux/if_ether.h>
> +#include <linux/skbuff.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/debugfs.h>
> +#include <linux/can.h>
> +#include <linux/can/dev.h>
> +#include <linux/can/error.h>
> +#include <linux/can/platform/ti_hecc_platform.h>
> +
> +#define DRV_NAME "TI HECC"

#define DRV_NAME "ti_hecc"

Other drivers also use the file name here.

> +#define HECC_MODULE_VERSION     "0.2"

A version number is will usually not maintained. May drivers have it but
it's never changed.

> +MODULE_VERSION(HECC_MODULE_VERSION);
> +#define DRV_DESC "TI High End CAN Controller Driver " HECC_MODULE_VERSION
> +
> +#define HECC_MAX_MAILBOXES     32      /* hardware mboxes - do not change */
> +
> +#if (CAN_ECHO_SKB_MAX > 16)
> +#define TI_HECC_MAX_TX_MBOX    16
> +#else
> +#define TI_HECC_MAX_TX_MBOX    CAN_ECHO_SKB_MAX
> +#endif
> +#define TI_HECC_MAX_RX_MBOX    (HECC_MAX_MAILBOXES - TI_HECC_MAX_TX_MBOX)
> +
> +#define TI_HECC_DEF_NAPI_WEIGHT        TI_HECC_MAX_RX_MBOX
> +
> +/* TI HECC module registers */
> +
> +#define HECC_CANME             0x0     /* Mailbox enable */
> +#define HECC_CANMD             0x4     /* Mailbox direction */
> +#define HECC_CANTRS            0x8     /* Transmit request set */
> +#define HECC_CANTRR            0xC     /* Transmit request */
> +#define HECC_CANTA             0x10    /* Transmission acknowledge */
> +#define HECC_CANAA             0x14    /* Abort acknowledge */
> +#define HECC_CANRMP            0x18    /* Receive message pending */
> +#define HECC_CANRML            0x1C    /* Remote message lost */
> +#define HECC_CANRFP            0x20    /* Remote frame pending */
> +#define HECC_CANGAM            0x24    /* SECC only:Global acceptance mask */
> +#define HECC_CANMC             0x28    /* Master control */
> +#define HECC_CANBTC            0x2C    /* Bit timing configuration */
> +#define HECC_CANES             0x30    /* Error and status */
> +#define HECC_CANTEC            0x34    /* Transmit error counter */
> +#define HECC_CANREC            0x38    /* Receive error counter */
> +#define HECC_CANGIF0           0x3C    /* Global interrupt flag 0 */
> +#define HECC_CANGIM            0x40    /* Global interrupt mask */
> +#define HECC_CANGIF1           0x44    /* Global interrupt flag 1 */
> +#define HECC_CANMIM            0x48    /* Mailbox interrupt mask */
> +#define HECC_CANMIL            0x4C    /* Mailbox interrupt level */
> +#define HECC_CANOPC            0x50    /* Overwrite protection control */
> +#define HECC_CANTIOC           0x54    /* Transmit I/O control */
> +#define HECC_CANRIOC           0x58    /* Receive I/O control */
> +#define HECC_CANLNT            0x5C    /* HECC only: Local network time */
> +#define HECC_CANTOC            0x60    /* HECC only: Time-out control */
> +#define HECC_CANTOS            0x64    /* HECC only: Time-out status */
> +#define HECC_CANTIOCE          0x68    /* SCC only:Enhanced TX I/O control */
> +#define HECC_CANRIOCE          0x6C    /* SCC only:Enhanced RX I/O control */
> +
> +/* SCC only:Local acceptance registers */
> +#define HECC_CANLAM0           (priv->scc_ram_offset + 0x0)
> +#define HECC_CANLAM3           (priv->scc_ram_offset + 0xC)
> +
> +/* HECC only */
> +#define HECC_CANLAM(mbxno)     (priv->hecc_ram_offset + ((mbxno) * 4))
> +#define HECC_CANMOTS(mbxno)    (priv->hecc_ram_offset + ((mbxno) * 4) + 0x80)
> +#define HECC_CANMOTO(mbxno)    (priv->hecc_ram_offset + ((mbxno) * 4) + 0x100)
> +
> +/* Mailbox registers */
> +#define HECC_CANMID(mbxno)     (priv->mbox_offset + ((mbxno) * 0x10))
> +#define HECC_CANMCF(mbxno)     (priv->mbox_offset + ((mbxno) * 0x10) + 0x4)
> +#define HECC_CANMDL(mbxno)     (priv->mbox_offset + ((mbxno) * 0x10) + 0x8)
> +#define HECC_CANMDH(mbxno)     (priv->mbox_offset + ((mbxno) * 0x10) + 0xC)
> +
> +#define HECC_SET_REG           0xFFFFFFFF
> +#define HECC_CANID_MASK                0x3FF   /* 18 bits mask for extended id's */
> +
> +#define HECC_CANMC_SCM         BIT(13) /* SCC compat mode */
> +#define HECC_CANMC_CCR         BIT(12) /* Change config request */
> +#define HECC_CANMC_PDR         BIT(11) /* Local Power down - for sleep mode */
> +#define HECC_CANMC_ABO         BIT(7)  /* Auto Bus On */
> +#define HECC_CANMC_STM         BIT(6)  /* Self test mode - loopback */
> +#define HECC_CANMC_SRES                BIT(5)  /* Software reset */
> +
> +#define HECC_CANTIOC_EN                BIT(3)  /* Enable CAN TX I/O pin */
> +#define HECC_CANRIOC_EN                BIT(3)  /* Enable CAN RX I/O pin */
> +
> +#define HECC_CANMID_IDE                BIT(31) /* Extended frame format */
> +#define HECC_CANMID_AME                BIT(30) /* Acceptance mask enable */
> +#define HECC_CANMID_AAM                BIT(29) /* Auto answer mode */
> +
> +#define HECC_CANES_FE          BIT(24) /* form error */
> +#define HECC_CANES_BE          BIT(23) /* bit error */
> +#define HECC_CANES_SA1         BIT(22) /* stuck at dominant error */
> +#define HECC_CANES_CRCE                BIT(21) /* CRC error */
> +#define HECC_CANES_SE          BIT(20) /* stuff bit error */
> +#define HECC_CANES_ACKE                BIT(19) /* ack error */
> +#define HECC_CANES_BO          BIT(18) /* Bus off status */
> +#define HECC_CANES_EP          BIT(17) /* Error passive status */
> +#define HECC_CANES_EW          BIT(16) /* Error warning status */
> +#define HECC_CANES_SMA         BIT(5)  /* suspend mode ack */
> +#define HECC_CANES_CCE         BIT(4)  /* Change config enabled */
> +#define HECC_CANES_PDA         BIT(3)  /* Power down mode ack */
> +
> +#define HECC_CANBTC_SAM                BIT(7)  /* sample points */
> +
> +#define HECC_BUS_ERROR         (HECC_CANES_FE | HECC_CANES_BE |\
> +                               HECC_CANES_CRCE | HECC_CANES_SE |\
> +                               HECC_CANES_ACKE)
> +
> +#define HECC_CANMCF_RTR                BIT(4)  /* Remote xmit request */
> +
> +#define HECC_CANGIF_MAIF       BIT(17) /* Message alarm interrupt */
> +#define HECC_CANGIF_TCOIF      BIT(16) /* Timer counter overflow int */
> +#define HECC_CANGIF_GMIF       BIT(15) /* Global mailbox interrupt */
> +#define HECC_CANGIF_AAIF       BIT(14) /* Abort ack interrupt */
> +#define HECC_CANGIF_WDIF       BIT(13) /* Write denied interrupt */
> +#define HECC_CANGIF_WUIF       BIT(12) /* Wake up interrupt */
> +#define HECC_CANGIF_RMLIF      BIT(11) /* Receive message lost interrupt */
> +#define HECC_CANGIF_BOIF       BIT(10) /* Bus off interrupt */
> +#define HECC_CANGIF_EPIF       BIT(9)  /* Error passive interrupt */
> +#define HECC_CANGIF_WLIF       BIT(8)  /* Warning level interrupt */
> +#define HECC_CANGIF_MBOX_MASK  0x1F    /* Mailbox number mask */
> +#define HECC_CANGIM_I1EN       BIT(1)  /* Int line 1 enable */
> +#define HECC_CANGIM_I0EN       BIT(0)  /* Int line 0 enable */
> +#define HECC_CANGIM_DEF_MASK   0xFF00  /* all except maif and tcoif */
> +#define HECC_CANGIM_SIL                BIT(2)  /* system interrupts to int line 1 */
> +
> +/* CAN Bittiming constants as per HECC specs */
> +static struct can_bittiming_const ti_hecc_bittiming_const = {
> +       .name = DRV_NAME,
> +       .tseg1_min = 1,
> +       .tseg1_max = 16,
> +       .tseg2_min = 1,

Please check if tseg2_min is a valid value. Usually it's larger.

> +       .tseg2_max = 8,
> +       .sjw_max = 4,
> +       .brp_min = 1,
> +       .brp_max = 256,
> +       .brp_inc = 1,
> +};
> +
> +struct ti_hecc_priv {
> +       struct can_priv can;

Please add "must be the first member/field".

> +       struct napi_struct napi;
> +       struct net_device *ndev;
> +       struct clk *clk;
> +       void __iomem *base;
> +       unsigned int scc_ram_offset;
> +       unsigned int hecc_ram_offset;
> +       unsigned int mbox_offset;
> +       unsigned int int_line;
> +       DECLARE_BITMAP(tx_free_mbx, TI_HECC_MAX_TX_MBOX);
> +       spinlock_t tx_lock;

Please document the spinlock tx_lock. What is it used for.

> +
> +       /* Statistics */
> +       unsigned out_of_tx_mbox;
> +       unsigned write_denied_cnt;
> +       unsigned message_lost_cnt;
> +       unsigned wake_up_cnt;
> +       unsigned message_alarm_cnt;
> +       unsigned timer_overflow_cnt;#

Debugging!? Should be removed. There is no interface to list these fields.

> +};
> +
> +static inline
> +void hecc_write(struct ti_hecc_priv *priv, int reg, unsigned int val)
> +{
> +       __raw_writel(val, priv->base + reg);
> +}
> +
> +static inline unsigned int hecc_read(struct ti_hecc_priv *priv, int reg)
> +{
> +       return __raw_readl(priv->base + reg);
> +}
> +
> +static inline
> +void hecc_set_bit(struct ti_hecc_priv *priv, int reg, unsigned bit)
> +{
> +       hecc_write(priv, reg, (hecc_read(priv, reg) | bit));
> +}
> +
> +static inline
> +void hecc_clear_bit(struct ti_hecc_priv *priv, int reg, unsigned bit)
> +{
> +       hecc_write(priv, reg, (hecc_read(priv, reg) & ~bit));
> +}
> +
> +static inline
> +unsigned int hecc_get_bit(struct ti_hecc_priv *priv, int reg, int bit)
> +{
> +       return (hecc_read(priv, reg) & bit) ? 1 : 0;
> +}
> +
[snip]
> +
> +static int ti_hecc_get_state(const struct net_device *ndev,
> +       enum can_state *state)
> +{
> +       struct ti_hecc_priv *priv = netdev_priv(ndev);
> +       *state = priv->can.state;
> +       return 0;
> +}
> +
> +static int ti_hecc_set_bittiming(struct net_device *ndev)
> +{
> +       struct ti_hecc_priv *priv = netdev_priv(ndev);
> +       struct can_bittiming *bit_timing = &priv->can.bittiming;
> +       unsigned int can_btc = 0;
> +
> +       can_btc = ((bit_timing->phase_seg2 - 1) & 0x7);
> +       can_btc |= (((bit_timing->phase_seg1 + bit_timing->prop_seg - 1)
> +                       & 0xF) << 3);
> +       if (bit_timing->brp > 4)
> +               can_btc |= HECC_CANBTC_SAM;

Please use:

	if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
	          can_btc |= HECC_CANBTC_SAM;

CAN controller modes can be set via "ip" utility. Note that also
loopback and listen-only is supported.

> +       can_btc |= (((bit_timing->sjw - 1) & 0x3) << 8);
> +       can_btc |= (((bit_timing->brp - 1) & 0xFF) << 16);
> +
> +       /* ERM being set to 0 by default meaning resync at falling edge */
> +
> +       hecc_write(priv, HECC_CANBTC, can_btc);

Other drivers use dev_info here:

	dev_info(ND2D(dev), "setting CANBTC=%#x\n", can_btc);

> +       return 0;
> +}
> +
> +/**
> + * ti_hecc_reset: Reset HECC module and set bit timings
> + *
> + * Resets HECC by writing to change config request bit and then sets
> + * bit-timing registers in the module to enable the module for operation
> + */
> +static void ti_hecc_reset(struct net_device *ndev)
> +{
> +#define HECC_CCE_WAIT_COUNT    1000
> +       unsigned int cnt;
> +       struct ti_hecc_priv *priv = netdev_priv(ndev);
> +
> +       dev_dbg(ndev->dev.parent, "resetting hecc ...\n");
> +
> +       hecc_set_bit(priv, HECC_CANMC, HECC_CANMC_SRES);
> +
> +       /* if change control request not enabled */
> +       if (!hecc_get_bit(priv, HECC_CANES, HECC_CANES_CCE)) {
> +               /* Set change control request and wait till enabled */
> +               hecc_set_bit(priv, HECC_CANMC, HECC_CANMC_CCR);
> +
> +               /* INFO: It has been observed that at times CCE bit may not be
> +                * set and hw seems to be ok even if this bit is not set so
> +                * timing out with a large counter to respect the specs
> +                */
> +               cnt = HECC_CCE_WAIT_COUNT;
> +               while (!hecc_get_bit(priv, HECC_CANES, HECC_CANES_CCE)) {
> +                       --cnt;
> +                       if (0 == cnt) {
> +                               dev_info(ndev->dev.parent,
> +                                       "timing out on CCE after reset\n");
> +                               break;
> +                       }
> +                       if (printk_ratelimit())
> +                               dev_dbg(ndev->dev.parent,
> +                                       "waiting CCE after reset\n");

I think you don't need the ratelimit if you check after the while loop.

> +               }

The while loop above does not use any defined delay and therefore the
timing depends on the processor speed. Adding udelay(1|10) would solve
the issue.

> +       }
> +

> +       /* Set bit timing on the device */
> +       ti_hecc_set_bittiming(priv->ndev);

Hm, you re-set the bittiming here!? It is alread done via netlink
interface before the device is opened/started.

> +
> +       /* Clear CCR (and CANMC register) and wait for CCE = 0 enable */
> +       hecc_write(priv, HECC_CANMC, 0);
> +
> +       /* INFO: CAN net stack handles bus off and hence disabling auto bus on
> +       hecc_set_bit(priv, HECC_CANMC, HECC_CANMC_ABO);
> +       */

Right, automatic bus-off recovery should be disabled. To make that clear:

           hecc_clear_bit(priv, HECC_CANMC, HECC_CANMC_ABO);

> +       /* Wait till CCE bit clears */
> +       /* INFO: It has been observed that at times CCE bit may not be
> +        * set and hw seems to be ok even if this bit is not set so
> +        * timing out with a large counter to respect the specs
> +        */
> +       cnt = HECC_CCE_WAIT_COUNT;
> +       while (hecc_get_bit(priv, HECC_CANES, HECC_CANES_CCE)) {
> +               --cnt;
> +               if (0 == cnt) {
> +                       dev_info(ndev->dev.parent,
> +                               "timing out on CCE after bittiming\n");
> +                       break;
> +               }
> +               if (printk_ratelimit())
> +                       dev_dbg(ndev->dev.parent,
> +                               "waiting CCE after bittiming\n");
> +       }

Consider adding a udelay(1|10) as mentioned above.

> +       /* Enable TX and RX I/O Control pins */
> +       hecc_write(priv, HECC_CANTIOC, HECC_CANTIOC_EN);
> +       hecc_write(priv, HECC_CANRIOC, HECC_CANRIOC_EN);
> +
> +       /* Clear registers for clean operation */
> +       hecc_write(priv, HECC_CANTA, HECC_SET_REG);
> +       hecc_write(priv, HECC_CANRMP, HECC_SET_REG);
> +       hecc_write(priv, HECC_CANGIF0, HECC_SET_REG);
> +       hecc_write(priv, HECC_CANGIF1, HECC_SET_REG);
> +       hecc_write(priv, HECC_CANME, 0);
> +       hecc_write(priv, HECC_CANMD, 0);
> +
> +       /* SCC compat mode NOT supported (and not needed too) */
> +       hecc_set_bit(priv, HECC_CANMC, HECC_CANMC_SCM);
> +}
> +
> +/**
> + * ti_hecc_start: Starts HECC module
> + *
> + * If CAN state is not stopped, reset the module, init bit timings
> + * and start the device for operation
> + */
> +static void ti_hecc_start(struct net_device *ndev)
> +{
> +       struct ti_hecc_priv *priv = netdev_priv(ndev);
> +       int cnt, mbxno;
> +
> +       ti_hecc_reset(ndev);
> +
> +       bitmap_zero(priv->tx_free_mbx, TI_HECC_MAX_TX_MBOX);
> +
> +       /* Enable local and global acceptance mask registers */
> +       hecc_write(priv, HECC_CANGAM, HECC_SET_REG);
> +       hecc_write(priv, HECC_CANLAM0, HECC_SET_REG);
> +       hecc_write(priv, HECC_CANLAM3, HECC_SET_REG);
> +
> +       /* Prepare configured mailboxes to receive messages */
> +       for (cnt = 0; cnt < TI_HECC_MAX_RX_MBOX; cnt++) {
> +               mbxno = (HECC_MAX_MAILBOXES - 1 - cnt);
> +               hecc_clear_bit(priv, HECC_CANME, (1 << mbxno));
> +               hecc_write(priv, HECC_CANMID(mbxno), HECC_CANMID_AME);
> +               hecc_write(priv, HECC_CANLAM(mbxno), HECC_SET_REG);
> +               hecc_set_bit(priv, HECC_CANMD, (1 << mbxno));
> +               hecc_set_bit(priv, HECC_CANME, (1 << mbxno));
> +               hecc_set_bit(priv, HECC_CANMIM, (1 << mbxno));
> +       }
> +
> +       /* Prevent message over-write & Enable interrupts */
> +       hecc_write(priv, HECC_CANTRS, 0);
> +       hecc_write(priv, HECC_CANOPC, HECC_SET_REG);
> +       if (priv->int_line) {
> +               hecc_write(priv, HECC_CANMIL, HECC_SET_REG);
> +               hecc_write(priv, HECC_CANGIM, (HECC_CANGIM_DEF_MASK |
> +                       HECC_CANGIM_I1EN | HECC_CANGIM_SIL));
> +       } else {
> +               hecc_write(priv, HECC_CANMIL, 0);
> +               hecc_write(priv, HECC_CANGIM,
> +                       (HECC_CANGIM_DEF_MASK | HECC_CANGIM_I0EN));
> +       }
> +       priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +}
> +
> +static void ti_hecc_stop(struct net_device *ndev)
> +{
> +       struct ti_hecc_priv *priv = netdev_priv(ndev);
> +
> +       /* Disable interrupts and disable mailboxes */
> +       hecc_write(priv, HECC_CANGIM, 0);
> +       hecc_write(priv, HECC_CANMIM, 0);
> +       hecc_write(priv, HECC_CANME, 0);
> +       priv->can.state = CAN_STATE_STOPPED;
> +}
> +
> +static int ti_hecc_do_set_mode(struct net_device *ndev, enum can_mode mode)
> +{
> +       struct ti_hecc_priv *priv = netdev_priv(ndev);
> +       int ret = 0;
> +
> +       switch (mode) {
> +       case CAN_MODE_SLEEP:
> +               dev_info(priv->ndev->dev.parent, "device going to sleep\n");
> +               if (netif_running(ndev)) {
> +                       netif_stop_queue(ndev);
> +                       netif_device_detach(ndev);
> +                       /* Put HECC in low power mode */
> +                       hecc_set_bit(priv, HECC_CANMC, HECC_CANMC_PDR);
> +               }
> +               priv->can.state = CAN_STATE_SLEEPING;
> +               break;

Has sleeping been tested? Actually, we do not have an interface yet to
enter sleep mode. Please remove. If there is a need for it, we should
work togehter to refine the interface and to provide a solution.

> +       case CAN_MODE_STOP:
> +               dev_info(priv->ndev->dev.parent, "device stopping\n");
> +               ti_hecc_stop(ndev);
> +               break;

Only CAN_MODE_START is used by the CAN devicde interface for bus-off
recovery.

> +       case CAN_MODE_START:
> +               dev_info(priv->ndev->dev.parent, "device (re)starting\n");
> +               ++priv->can.can_stats.restarts;
> +               ti_hecc_start(ndev);
> +               if (netif_queue_stopped(ndev))
> +                       netif_wake_queue(ndev);
> +               break;
> +       default:
> +               ret = -EOPNOTSUPP;
> +               break;
> +       }
> +
> +       return ret;
> +}
> +
> +static int ti_hecc_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
> +       struct ti_hecc_priv *priv = netdev_priv(ndev);
> +       struct net_device_stats *stats = &ndev->stats;
> +       struct can_frame *cf = (struct can_frame *)skb->data;
> +       u32 mbxno = 0;
> +       u32 data;
> +       unsigned long flags;
> +
> +       /* Find the first mailbox that is free for xmit */
> +       spin_lock_irqsave(&priv->tx_lock, flags);
> +       mbxno = find_first_zero_bit(priv->tx_free_mbx, TI_HECC_MAX_TX_MBOX);
> +       if (mbxno == TI_HECC_MAX_TX_MBOX) {
> +               netif_stop_queue(ndev);
> +               if (printk_ratelimit())
> +                       dev_err(priv->ndev->dev.parent,
> +                               "Out of TX buffers ...\n");
> +               spin_unlock_irqrestore(&priv->tx_lock, flags);
> +               return NETDEV_TX_BUSY;

Could'nt the NETDEV_TX_BUSY be avoided by stopping the queue earlier?

> +
> +       }
> +       set_bit(mbxno, priv->tx_free_mbx);
> +       spin_unlock_irqrestore(&priv->tx_lock, flags);

Hm, I wonder how the driver ensures that packages go out in order. How
does the CAN hardware schedule pending TX message objects? Vladislav
posted a test programs recently to check message ordering. See:

https://lists.berlios.de/pipermail/socketcan-core/2009-August/002871.html

> +
> +       /* Prepare mailbox for transmission */
> +       hecc_clear_bit(priv, HECC_CANME, (1 << mbxno));
> +       data = cf->can_dlc & 0xF;
> +       if (cf->can_id & CAN_RTR_FLAG) /* Remote transmission request */
> +               data |= HECC_CANMCF_RTR;
> +       hecc_write(priv, HECC_CANMCF(mbxno), data);
> +       if (cf->can_id & CAN_EFF_FLAG) { /* Extended frame format */
> +               data = ((cf->can_id & CAN_EFF_MASK) | HECC_CANMID_IDE);
> +       } else { /* Standard frame format */
> +               data = ((cf->can_id & CAN_SFF_MASK) << 18);
> +       }
> +       hecc_write(priv, HECC_CANMID(mbxno), data);
> +       data = (cf->data[0] << 24) | (cf->data[1] << 16) |
> +                       (cf->data[2] << 8) | cf->data[3];
> +       hecc_write(priv, HECC_CANMDL(mbxno), data);
> +       if (cf->can_dlc > 4) {
> +               data = (cf->data[4] << 24) | (cf->data[5] << 16) |
> +                       (cf->data[6] << 8) | cf->data[7];
> +               hecc_write(priv, HECC_CANMDH(mbxno), data);
> +       }
> +
[slip]
> +
> +       /* Enable interrupt for mbox and start transmission */
> +       hecc_clear_bit(priv, HECC_CANMD, (1 << mbxno));
> +       hecc_set_bit(priv, HECC_CANME, (1 << mbxno));
> +       hecc_set_bit(priv, HECC_CANMIM, (1 << mbxno));
> +       hecc_set_bit(priv, HECC_CANTRS, (1 << mbxno));
> +
> +       stats->tx_bytes += cf->can_dlc;

Should be done when the TX done interrupr is handled.

> +       ndev->trans_start = jiffies;
> +       can_put_echo_skb(skb, ndev, mbxno);

Please call can_put_echo_skb() before starting transmission.

> +
> +       return NETDEV_TX_OK;
> +}
> +
> +static int ti_hecc_rx_pkt(struct ti_hecc_priv *priv, int mbxno)
> +{
> +       struct net_device_stats *stats = &priv->ndev->stats;
> +       struct can_frame *cf;
> +       struct sk_buff *skb;
> +       u32 data;
> +
> +       skb = dev_alloc_skb(sizeof(struct can_frame));
> +       if (!skb) {
> +               if (printk_ratelimit())
> +                       dev_err(priv->ndev->dev.parent,
> +                               "dev_alloc_skb() failed\n");
> +               return -ENOMEM;
> +       }
> +       skb->dev = priv->ndev;
> +       skb->protocol = htons(ETH_P_CAN);
> +       skb->ip_summed = CHECKSUM_UNNECESSARY;
> +
> +       cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
> +       memset(cf, 0, sizeof(struct can_frame));
> +       data = hecc_read(priv, HECC_CANMID(mbxno));
> +       if (data & HECC_CANMID_IDE)
> +               cf->can_id = (data & CAN_EFF_MASK) | CAN_EFF_FLAG;
> +       else
> +               cf->can_id = ((data >> 18) & CAN_SFF_MASK);
> +       data = hecc_read(priv, HECC_CANMCF(mbxno));
> +       if (data & HECC_CANMCF_RTR)
> +               cf->can_id |= CAN_RTR_FLAG;
> +       cf->can_dlc = data & 0xF;
> +       data = hecc_read(priv, HECC_CANMDL(mbxno));
> +       /* The below statements are for readability sake */
> +       cf->data[0] = ((data & 0xFF000000) >> 24);
> +       cf->data[1] = ((data & 0xFF0000) >> 16);
> +       cf->data[2] = ((data & 0xFF00) >> 8);
> +       cf->data[3] = (data & 0xFF);
> +       if (cf->can_dlc > 4) {
> +               data = hecc_read(priv, HECC_CANMDH(mbxno));
> +               cf->data[4] = ((data & 0xFF000000) >> 24);
> +               cf->data[5] = ((data & 0xFF0000) >> 16);
> +               cf->data[6] = ((data & 0xFF00) >> 8);
> +               cf->data[7] = (data & 0xFF);
> +       }
> +
[snip]
> +
> +       /* prepare mailbox for next receive */
> +       hecc_clear_bit(priv, HECC_CANME, (1 << mbxno));
> +       hecc_write(priv, HECC_CANMID(mbxno), HECC_CANMID_AME);
> +       hecc_write(priv, HECC_CANLAM(mbxno), HECC_SET_REG);
> +       hecc_set_bit(priv, HECC_CANMD, (1 << mbxno));
> +       hecc_set_bit(priv, HECC_CANME, (1 << mbxno));
> +
> +       stats->rx_bytes += cf->can_dlc;
> +       netif_rx(skb);

Hm, IIRC, netif_receive_skb(skb) should be used with NAPI.

> +       stats->rx_packets++;
> +       priv->ndev->last_rx = jiffies;
> +
> +       return 0;
> +}
> +
> +static int ti_hecc_rx_poll(struct napi_struct *napi, int quota)
> +{
> +       struct net_device *ndev = napi->dev;
> +       struct ti_hecc_priv *priv = netdev_priv(ndev);
> +       int num_pkts = 0;
> +       unsigned long pending_pkts;
> +       int mbxno;
> +
> +       if (!netif_running(ndev))
> +               return 0;
> +
> +       pending_pkts = hecc_read(priv, HECC_CANRMP);
> +       while (pending_pkts && (num_pkts < quota)) {
> +               mbxno = find_first_bit(&pending_pkts, HECC_MAX_MAILBOXES);

Here I also wonder if the messages are handled in the correct order.

> +               if (mbxno == HECC_MAX_MAILBOXES) {
> +                       dev_info(priv->ndev->dev.parent,
> +                               "Reached max mailboxes. No rx pkts\n");
> +                       return num_pkts;
> +               }
> +
> +               if (ti_hecc_rx_pkt(priv, mbxno) < 0)
> +                       return num_pkts;
> +
> +               clear_bit(mbxno, &pending_pkts);
> +               hecc_set_bit(priv, HECC_CANRMP, (1 << mbxno));
> +               ++num_pkts;
> +       }
> +
> +       /* Enable packet interrupt if all pkts are handled */
> +       if (0 == hecc_read(priv, HECC_CANRMP)) {
> +               napi_complete(napi);
> +               /* Re-enable RX mailbox interrupts */
> +               mbxno = hecc_read(priv, HECC_CANMIM);
> +               mbxno |= (~((1 << TI_HECC_MAX_TX_MBOX) - 1));
> +               hecc_write(priv, HECC_CANMIM, mbxno);
> +       }
> +
> +       return num_pkts;
> +}
> +
> +/**
> + * ti_hecc_error: TI HECC error routine
> + *
> + * Handles HECC error - handles error condition and send a packet up the stack
> + */
> +static int
> +ti_hecc_error(struct net_device *ndev, int int_status, int err_status)
> +{
> +       struct ti_hecc_priv *priv = netdev_priv(ndev);
> +       struct net_device_stats *stats = &ndev->stats;
> +       struct can_frame *cf;
> +       struct sk_buff *skb;
> +       int data;
> +
> +       /* propogate the error condition to the can stack */
> +       skb = dev_alloc_skb(sizeof(struct can_frame));
> +       if (!skb) {
> +               if (printk_ratelimit())
> +                       dev_err(priv->ndev->dev.parent,
> +                               "dev_alloc_skb() failed in err processing\n");
> +               return -ENOMEM;
> +       }
> +       skb->dev = ndev;
> +       skb->protocol = htons(ETH_P_CAN);
> +       cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
> +       memset(cf, 0, sizeof(struct can_frame));
> +       cf->can_id = CAN_ERR_FLAG;
> +       cf->can_dlc = CAN_ERR_DLC;
> +
> +       if (int_status & HECC_CANGIF_RMLIF) { /* Message lost interrupt */
> +               data = hecc_read(priv, HECC_CANRML);
> +               hecc_write(priv, HECC_CANRML, data);
> +               ++priv->message_lost_cnt;

Debugging!?

> +               cf->can_id |= CAN_ERR_CRTL;
> +               cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> +               stats->rx_over_errors++;
> +               stats->rx_errors++;
> +               dev_info(priv->ndev->dev.parent, "Message lost interrupt\n");

Debugging!?

> +       }
> +
> +       if (int_status & HECC_CANGIF_WLIF) { /* warning level int */
> +               if (0 == (int_status & HECC_CANGIF_BOIF)) {
> +                       priv->can.state = CAN_STATE_ERROR_WARNING;
> +                       ++priv->can.can_stats.error_warning;
> +                       cf->can_id |= CAN_ERR_CRTL;
> +                       if (hecc_read(priv, HECC_CANTEC) > 96)
> +                               cf->data[1] |= CAN_ERR_CRTL_TX_WARNING;
> +                       if (hecc_read(priv, HECC_CANREC) > 96)
> +                               cf->data[1] |= CAN_ERR_CRTL_RX_WARNING;
> +               }
> +               hecc_set_bit(priv, HECC_CANES, HECC_CANES_EW);
> +               dev_info(priv->ndev->dev.parent, "Error Warning interrupt\n");
> +               hecc_clear_bit(priv, HECC_CANMC, HECC_CANMC_CCR);
> +       }
> +
> +       if (int_status & HECC_CANGIF_EPIF) { /* error passive int */
> +               if (0 == (int_status & HECC_CANGIF_BOIF)) {
> +                       priv->can.state = CAN_STATE_ERROR_PASSIVE;
> +                       ++priv->can.can_stats.error_passive;
> +                       cf->can_id |= CAN_ERR_CRTL;
> +                       if (hecc_read(priv, HECC_CANTEC) > 127)
> +                               cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE;
> +                       if (hecc_read(priv, HECC_CANREC) > 127)
> +                               cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
> +               }
> +               hecc_set_bit(priv, HECC_CANES, HECC_CANES_EP);
> +               dev_info(priv->ndev->dev.parent, "Error passive interrupt\n");

Please use dev_dbg.

> +               hecc_clear_bit(priv, HECC_CANMC, HECC_CANMC_CCR);
> +       }
> +
> +       /* Need to check busoff condition in error status register too to
> +        * ensure warning interrupts don't hog the system
> +        */
> +       if (int_status & HECC_CANGIF_BOIF) {
> +               priv->can.state = CAN_STATE_BUS_OFF;
> +               cf->can_id |= CAN_ERR_BUSOFF;
> +               hecc_set_bit(priv, HECC_CANES, HECC_CANES_BO);
> +               dev_info(priv->ndev->dev.parent, "Bus Off interrupt\n");

can_bus_off() already calls dev_dbg("bus-off").

> +               hecc_clear_bit(priv, HECC_CANMC, HECC_CANMC_CCR);
> +               can_bus_off(ndev);
> +               /* Disable all interrupts in bus-off to avoid int hog */
> +               hecc_write(priv, HECC_CANGIM, 0);
> +       }
> +
> +       if (err_status & HECC_CANES_BO) {
> +               priv->can.state = CAN_STATE_BUS_OFF;
> +               cf->can_id |= CAN_ERR_BUSOFF;
> +               hecc_set_bit(priv, HECC_CANES, HECC_CANES_BO);
> +               dev_info(priv->ndev->dev.parent, "Bus Off condition\n");
> +               hecc_clear_bit(priv, HECC_CANMC, HECC_CANMC_CCR);
> +               can_bus_off(ndev);
> +               /* Disable all interrupts in bus-off to avoid int hog */
> +               hecc_write(priv, HECC_CANGIM, 0);
> +       }

I think the two if blocks above use idendical code. What is the
difference. Using if (a || b) would be more appropriate.

> +
> +       if (err_status & HECC_BUS_ERROR) {
> +               ++priv->can.can_stats.bus_error;
> +               cf->can_id |= (CAN_ERR_BUSERROR | CAN_ERR_PROT);
> +               cf->data[2] |= CAN_ERR_PROT_UNSPEC;
> +               if (err_status & HECC_CANES_FE) {
> +                       hecc_set_bit(priv, HECC_CANES, HECC_CANES_FE);
> +                       cf->data[2] |= CAN_ERR_PROT_FORM;
> +               }
> +               if (err_status & HECC_CANES_BE) {
> +                       hecc_set_bit(priv, HECC_CANES, HECC_CANES_BE);
> +                       cf->data[2] |= CAN_ERR_PROT_BIT;
> +               }
> +               if (err_status & HECC_CANES_SE) {
> +                       hecc_set_bit(priv, HECC_CANES, HECC_CANES_SE);
> +                       cf->data[2] |= CAN_ERR_PROT_STUFF;
> +               }
> +               if (err_status & HECC_CANES_CRCE) {
> +                       hecc_set_bit(priv, HECC_CANES, HECC_CANES_CRCE);
> +                       cf->data[2] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
> +                                       CAN_ERR_PROT_LOC_CRC_DEL);
> +               }
> +               if (err_status & HECC_CANES_ACKE) {
> +                       hecc_set_bit(priv, HECC_CANES, HECC_CANES_ACKE);
> +                       cf->data[2] |= (CAN_ERR_PROT_LOC_ACK |
> +                                       CAN_ERR_PROT_LOC_ACK_DEL);
> +               }
> +       }
> +
> +       netif_receive_skb(skb);

OK, here you use netif_receive_skb().

> +       ndev->last_rx = jiffies;
> +       stats->rx_packets++;
> +       stats->rx_bytes += cf->can_dlc;
> +       return 0;
> +}

At a first glance, error message creation looks good. I will have a
closer look some time later.

> +
> +/**
> + * ti_hecc_interrupt: TI HECC interrupt routine
> + *
> + * Handles HECC interrupts - disables interrupt if receive pkts that will
> + * be enabled when rx pkts are compelte (napi_complete is done)
> + */
> +static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
> +{
> +       struct net_device *ndev = (struct net_device *)dev_id;
> +       struct ti_hecc_priv *priv = netdev_priv(ndev);
> +       struct net_device_stats *stats = &ndev->stats;
> +       struct sk_buff *skb;
> +       struct can_frame *cf;
> +       unsigned int int_status;
> +       unsigned long ack;
> +       int mbxno;
> +       unsigned long flags;
> +
> +       if (priv->int_line)
> +               int_status = hecc_read(priv, HECC_CANGIF1);
> +       else
> +               int_status = hecc_read(priv, HECC_CANGIF0);
> +
> +       if (0 == int_status)
> +               return IRQ_NONE;
> +
> +       /* Handle message alarm interrupt */
> +       if (int_status & HECC_CANGIF_MAIF) {
> +               ++priv->message_alarm_cnt;
> +               dev_info(priv->ndev->dev.parent, "Message alarm interrupt\n");
> +       }
> +
> +       /* Handle local network timer counter overflow interrupt */
> +       if (int_status & HECC_CANGIF_TCOIF) {
> +               ++priv->timer_overflow_cnt;
> +               dev_info(priv->ndev->dev.parent,
> +                       "Local network timer counter overflow interrupt\n");
> +       }
> +
> +       /* Handle write denied interrupt */
> +       if (int_status & HECC_CANGIF_WDIF) {
> +               ++priv->write_denied_cnt;
> +               dev_info(priv->ndev->dev.parent, "Write denied interrupt\n");
> +       }
> +
> +       /* Handle wake up interrupt */
> +       if (int_status & HECC_CANGIF_WUIF) {
> +               ++priv->wake_up_cnt;
> +               dev_info(priv->ndev->dev.parent, "Wake up interrupt\n");
> +       }

Do the interrupt sources above occur. Does it harm or even signal a
malfunctioning? If yes, use dev_err and dev_dbg otherwise. And as
mentioned above, the statistic counters are most likely for debugging
purposes only.

> +
> +       ti_hecc_error(ndev, int_status, hecc_read(priv, HECC_CANES));

Hm, you create an error frame for each interrupt!? What do you see with:

 # candump any,0:0,#FFFFFFFF

> +
> +       /* Handle Abort acknowledge interrupt */
> +       if (int_status & HECC_CANGIF_AAIF) {
> +               ack = hecc_read(priv, HECC_CANAA);
> +               while (ack) {
> +                       mbxno = find_first_bit(&ack, HECC_MAX_MAILBOXES);
> +                       if (mbxno == HECC_MAX_MAILBOXES) {
> +                               break;
> +                       } else {
> +                               clear_bit(mbxno, &ack);
> +                               /* release echo pkt & update counters */
> +                               hecc_set_bit(priv, HECC_CANAA, (1 << mbxno));
> +                               hecc_clear_bit(priv, HECC_CANME, (1 << mbxno));
> +                               /* FIXME: since net-next tree's dev.h does not
> +                                * include can_free_echo_skb() doing equivalent
> +                                * of can_free_echo_skb(ndev, mbxno);
> +                                */
> +                               if (priv->can.echo_skb[mbxno]) {
> +                                       kfree_skb(priv->can.echo_skb[mbxno]);
> +                                       priv->can.echo_skb[mbxno] = NULL;
> +                               }
> +                               if (netif_queue_stopped(ndev))
> +                                       netif_wake_queue(ndev);
> +                               spin_lock_irqsave(&priv->tx_lock, flags);
> +                               clear_bit(mbxno, priv->tx_free_mbx);
> +                               spin_unlock_irqrestore(&priv->tx_lock, flags);
> +                       }
> +               }
> +       }

Can that interrupt happen? I have not found any code aborting messages.

> +
> +       /* Handle mailbox interrupt */
> +       if (int_status & HECC_CANGIF_GMIF) {
> +               ack = hecc_read(priv, HECC_CANTA);
> +               while (ack) {
> +                       mbxno = find_first_bit(&ack, HECC_MAX_MAILBOXES);
> +                       if (mbxno == HECC_MAX_MAILBOXES) {
> +                               break;
> +                       } else {
> +                               clear_bit(mbxno, &ack);
> +                               hecc_clear_bit(priv, HECC_CANME, (1 << mbxno));
> +                               hecc_set_bit(priv, HECC_CANTA, (1 << mbxno));
> +                               skb = priv->can.echo_skb[mbxno];
> +                               cf = (struct can_frame *) (skb->data);

Please don't access the echo skb's directly. Try to get the dlc from the
hardware.

> +                               can_get_echo_skb(ndev, mbxno);
> +                               stats->tx_bytes += cf->can_dlc;
> +                               spin_lock_irqsave(&priv->tx_lock, flags);
> +                               clear_bit(mbxno, priv->tx_free_mbx);
> +                               spin_unlock_irqrestore(&priv->tx_lock, flags);
> +                               stats->tx_packets++;
> +                       }
> +               }
> +               if (netif_queue_stopped(ndev))
> +                       netif_wake_queue(ndev);
> +
> +               /* Disable RX mailbox interrupts and let NAPI reenable them */
> +               ack = hecc_read(priv, HECC_CANMIM);
> +               ack &= ((1 << TI_HECC_MAX_TX_MBOX) - 1);
> +               hecc_write(priv, HECC_CANMIM, ack);
> +               napi_schedule(&priv->napi);

You schedule an RX event even if no RX message is pending? This does not
look very efficient to me.

> +       }
> +
> +       /* clear all interrupt conditions - read back to avoid spurious ints */
> +       if (priv->int_line) {
> +               hecc_write(priv, HECC_CANGIF1, HECC_SET_REG);
> +               int_status = hecc_read(priv, HECC_CANGIF1);
> +       } else {
> +               hecc_write(priv, HECC_CANGIF0, HECC_SET_REG);
> +               int_status = hecc_read(priv, HECC_CANGIF0);
> +       }
> +
> +       return IRQ_HANDLED;
> +}
> +
> +/* NOTE: yet to test suspend/resume */

Please remove suspend/resume code if it's not tested.

> +static int ti_hecc_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +       struct net_device *ndev = platform_get_drvdata(pdev);
> +       struct ti_hecc_priv *priv = netdev_priv(ndev);
> +
> +       if (netif_running(ndev)) {
> +               netif_stop_queue(ndev);
> +               netif_device_detach(ndev);
> +       }
> +
> +       hecc_set_bit(priv, HECC_CANMC, HECC_CANMC_PDR);
> +       priv->can.state = CAN_STATE_SLEEPING;
> +       clk_disable(priv->clk);
> +
> +       return 0;
> +}
> +
> +/* NOTE: yet to test suspend/resume */
> +static int ti_hecc_resume(struct platform_device *pdev)
> +{
> +       struct net_device *ndev = platform_get_drvdata(pdev);
> +       struct ti_hecc_priv *priv = netdev_priv(ndev);
> +
> +       clk_enable(priv->clk);
> +       hecc_clear_bit(priv, HECC_CANMC, HECC_CANMC_PDR);
> +       priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +       if (netif_running(ndev)) {
> +               netif_device_attach(ndev);
> +               netif_start_queue(ndev);
> +       }
> +
> +       return 0;
> +}
> +
> +static int ti_hecc_open(struct net_device *ndev)
> +{
> +       struct ti_hecc_priv *priv = netdev_priv(ndev);
> +       int err;
> +
> +       dev_info(ndev->dev.parent, "opening device\n");
> +
> +       if (request_irq(ndev->irq, ti_hecc_interrupt, IRQF_DISABLED,
> +                               ndev->name, ndev)) {
> +               dev_err(ndev->dev.parent, "error requesting interrupt\n");
> +               return -EAGAIN;

Please return the error returned by request_irq.

> +       }
> +
> +       /* Open common can device */
> +       err = open_candev(ndev);
> +       if (err) {
> +               dev_err(ndev->dev.parent, "open_candev() failed %08X\n", err);

free_irq?

> +               return err;
> +       }
> +
> +       /* Initialize device and start net queue */
> +       spin_lock_init(&priv->tx_lock);
> +
> +       clk_enable(priv->clk);
> +       ti_hecc_start(ndev);
> +       napi_enable(&priv->napi);
> +       netif_start_queue(ndev);
> +
> +       return 0;
> +}
> +
> +static int ti_hecc_close(struct net_device *ndev)
> +{
> +       struct ti_hecc_priv *priv = netdev_priv(ndev);
> +
> +       dev_info(ndev->dev.parent, "closing device\n");
> +       napi_disable(&priv->napi);
> +       netif_stop_queue(ndev);
> +       ti_hecc_stop(ndev);
> +       free_irq(ndev->irq, ndev);
> +       clk_disable(priv->clk);
> +       close_candev(ndev);
> +
> +       return 0;
> +}
> +
> +static const struct net_device_ops ti_hecc_netdev_ops = {
> +       .ndo_open               = ti_hecc_open,
> +       .ndo_stop               = ti_hecc_close,
> +       .ndo_start_xmit         = ti_hecc_xmit,
> +};
> +
> +static int ti_hecc_probe(struct platform_device *pdev)
> +{
> +       struct net_device *ndev = (struct net_device *)0;
> +       struct ti_hecc_priv *priv;
> +       struct ti_hecc_platform_data *pdata;
> +       struct resource *mem, *irq;
> +       void __iomem *addr;
> +       int err;
> +
> +       printk(KERN_INFO DRV_NAME " probing devices...\n");
> +       pdata = pdev->dev.platform_data;
> +       if (!pdata) {
> +               printk(KERN_ERR "No platform data available - exiting\n");

dev_err here and below.is more appropriate.

> +               return -ENODEV;
> +       }
> +
> +       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!mem) {
> +               printk(KERN_ERR "no mem resource?\n");
> +               err = -ENODEV;
> +               goto probe_exit;
> +       }
> +       irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +       if (!irq) {
> +               printk(KERN_ERR "no irq resource?\n");
> +               err = -ENODEV;
> +               goto probe_exit;
> +       }
> +       if (!request_mem_region(mem->start, (mem->end - mem->start) + 1,
> +               pdev->name)) {
> +               printk(KERN_ERR "HECC region already claimed\n");
> +               err = -EBUSY;
> +               goto probe_exit;
> +       }
> +       addr = ioremap(mem->start, mem->end - mem->start + 1);
> +       if (!addr) {
> +               printk(KERN_ERR "ioremap failed\n");
> +               err = -ENOMEM;
> +               goto probe_exit_free_region;
> +       }
> +
> +       ndev = alloc_candev(sizeof(struct ti_hecc_priv));
> +       if (!ndev) {
> +               printk(KERN_ERR "alloc_candev failed\n");
> +               err = -ENOMEM;
> +               goto probe_exit_iounmap;
> +       }
> +
> +       priv = netdev_priv(ndev);
> +       priv->ndev = ndev;
> +       priv->base = addr;
> +       priv->scc_ram_offset = pdata->scc_ram_offset;
> +       priv->hecc_ram_offset = pdata->hecc_ram_offset;
> +       priv->mbox_offset = pdata->mbox_offset;
> +       priv->int_line = pdata->int_line;
> +
> +       priv->can.bittiming_const       = &ti_hecc_bittiming_const;
> +       priv->can.do_set_bittiming      = ti_hecc_set_bittiming;
> +       priv->can.do_set_mode           = ti_hecc_do_set_mode;
> +       priv->can.do_get_state          = ti_hecc_get_state;

Coding style!?

> +       ndev->irq = irq->start;
> +       ndev->flags |= IFF_ECHO;
> +       platform_set_drvdata(pdev, ndev);
> +       SET_NETDEV_DEV(ndev, &pdev->dev);
> +       ndev->netdev_ops = &ti_hecc_netdev_ops;
> +
> +       /* Note: clk name would change using hecc_vbusp_ck temporarily */
> +       priv->clk = clk_get(&pdev->dev, "hecc_vbusp_ck");
> +       if (IS_ERR(priv->clk)) {
> +               dev_err(ndev->dev.parent, "no clock available\n");
> +               err = PTR_ERR(priv->clk);
> +               priv->clk = NULL;
> +               goto probe_exit_candev;
> +       }
> +       priv->can.clock.freq = clk_get_rate(priv->clk);
> +       netif_napi_add(ndev, &priv->napi, ti_hecc_rx_poll,
> +                       TI_HECC_DEF_NAPI_WEIGHT);
> +
> +       err = register_candev(ndev);
> +       if (err) {
> +               dev_err(ndev->dev.parent, "register_candev() failed\n");
> +               err = -ENODEV;
> +               goto probe_exit_clk;
> +       }
> +       dev_info(ndev->dev.parent, "regs=%p, irq=%d\n",
> +               priv->base, (unsigned int) ndev->irq);

Please use a more meaningful message, e.g.:

	dev_info(&pdev->dev,
	         "device registered (reg_base=%#p, irq=%d)\n",
		 priv->reg_base, dev->irq);

> +
> +#ifdef CONFIG_DEBUG_FS
> +       hecc_debug_init(priv);
> +#endif
> +       return 0;
> +
> +probe_exit_clk:
> +       clk_put(priv->clk);
> +probe_exit_candev:
> +       free_candev(ndev);
> +probe_exit_iounmap:
> +       iounmap(addr);
> +probe_exit_free_region:
> +       release_mem_region(mem->start, mem->end - mem->start + 1);
> +probe_exit:
> +       dev_err(ndev->dev.parent, "probe error = %08X\n", err);
> +       return err;
> +}
> +
> +static int __devexit ti_hecc_remove(struct platform_device *pdev)
> +{
> +       struct resource *res;
> +       struct net_device *ndev = platform_get_drvdata(pdev);
> +       struct ti_hecc_priv *priv = netdev_priv(ndev);
> +
> +#ifdef CONFIG_DEBUG_FS
> +       hecc_debug_exit();
> +#endif /* CONFIG_DEBUG_FS */
> +
> +       clk_put(priv->clk);
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       iounmap(priv->base);
> +       release_mem_region(res->start, res->end - res->start + 1);
> +       unregister_candev(ndev);
> +       free_candev(ndev);
> +       platform_set_drvdata(pdev, NULL);
> +       dev_info(ndev->dev.parent, "driver removed\n");
> +
> +       return 0;
> +}
> +
> +/* TI HECC netdevice driver: platform driver structure */
> +static struct platform_driver ti_hecc_driver = {
> +       .driver = {
> +               .name    = "ti_hecc",

Maybe:
                  .name    = DRV_NAME,

> +               .owner   = THIS_MODULE,
> +       },
> +       .probe = ti_hecc_probe,
> +       .remove = __devexit_p(ti_hecc_remove),
> +       .suspend = ti_hecc_suspend,
> +       .resume = ti_hecc_resume,
> +};
> +
> +static int __init ti_hecc_init_driver(void)
> +{
> +       printk(KERN_INFO DRV_DESC "\n");
> +       return platform_driver_register(&ti_hecc_driver);
> +}
> +module_init(ti_hecc_init_driver);
> +
> +static void __exit ti_hecc_exit_driver(void)
> +{
> +       printk(KERN_INFO DRV_DESC " :Exit\n");
> +       platform_driver_unregister(&ti_hecc_driver);
> +}
> +module_exit(ti_hecc_exit_driver);
> +
> +MODULE_AUTHOR("Anant Gole <[email-masked]>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION(DRV_DESC);
> diff --git a/include/linux/can/platform/ti_hecc_platform.h b/include/linux/can/platform/ti_hecc_platform.h
> new file mode 100644
> index 0000000..4a57daf
> --- /dev/null
> +++ b/include/linux/can/platform/ti_hecc_platform.h
> @@ -0,0 +1,40 @@
> +/*
> + * TI HECC (High End CAN Controller) driver platform header
> + *
> + * Copyright (C) 2009 Texas Instruments Incorporated - http://www.ti.com/
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed as is WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +/**
> + * struct hecc_platform_data - HECC Platform Data
> + *
> + * @scc_hecc_offset:   mostly 0 - should really never change
> + * @scc_ram_offset:    SCC RAM offset
> + * @hecc_ram_offset:   HECC RAM offset
> + * @mbox_offset:       Mailbox RAM offset
> + * @int_line:          Interrupt line to use - 0 or 1
> + * @version:           version for future use
> + *
> + * Platform data structure to get all platform specific settings.
> + * this structure also accounts the fact that the IP may have different
> + * RAM and mailbox offsets for different SOC's
> + */
> +struct ti_hecc_platform_data {
> +       unsigned int scc_hecc_offset;
> +       unsigned int scc_ram_offset;
> +       unsigned int hecc_ram_offset;
> +       unsigned int mbox_offset;
> +       unsigned int int_line;
> +       unsigned int version;
> +};

Thanks for your contribution.

Wolfgang.