Hi Ira, Ira W. Snyder wrote: > The Janz ICAN3 is a MODULbus daughterboard which fits on the Janz CMOD-IO > PCI carrier board. It is an intelligent CAN controller, with a > microcontroller and associated firmware. > > Signed-off-by: Ira W. Snyder <[email protected]> > --- > > I'm quite sure this driver still has style issues and other problems > that would prevent it from going to mainline immediately. I have > included the drivers for the CMOD-IO PCI carrier board, as well as the > driver for the VMOD-ICAN3 CAN daughterboard and VMOD-TTL GPIO > daughterboard. When this driver is ready for mainline, the GPIO driver > will not be included with the CAN driver. Feel free to ignore it for > now, it is just a placeholder at the moment.
Nice, splitting the code into a generic MODULbus driver and sub-module drivers it the right way to go. But they should also go into different kernel locations: CMOD-IO to drivers/mfd, VMOD-TTL GPIO to drivers/gpio and VMOD-ICAN3 to drivers/net/can, I think. They also belong to different kernel sub-systems managed by different maintainers and you therefore need to split up the patches before you post them to the relevant mailing lists. > Other than the above issues, I believe that the CAN bus error state > handling is correct, as well as the bit timing issues mentioned > previously. I will focus on the CAN driver. > Any review is appreciated. Thanks to everyone who has provided comments > on the earlier posting of this driver. > > Ira > > RFCv1 -> RFCv2: > - converted to a multi-driver model > - addressed many review comments > - added CAN bus error handling > - use a work function only instead of work + NAPI > - use SJA1000 bittiming calculation code > > drivers/net/can/Kconfig | 2 + > drivers/net/can/Makefile | 1 + > drivers/net/can/janz/Kconfig | 29 + > drivers/net/can/janz/Makefile | 5 + > drivers/net/can/janz/janz-cmodio.c | 343 ++++++++ > drivers/net/can/janz/janz-ican3.c | 1657 > ++++++++++++++++++++++++++++++++++++ > drivers/net/can/janz/janz-ttl.c | 72 ++ > drivers/net/can/janz/janz.h | 29 + > 8 files changed, 2138 insertions(+), 0 deletions(-) > create mode 100644 drivers/net/can/janz/Kconfig > create mode 100644 drivers/net/can/janz/Makefile > create mode 100644 drivers/net/can/janz/janz-cmodio.c > create mode 100644 drivers/net/can/janz/janz-ican3.c > create mode 100644 drivers/net/can/janz/janz-ttl.c > create mode 100644 drivers/net/can/janz/janz.h > > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig > index 05b7517..a643ccd 100644 > --- a/drivers/net/can/Kconfig > +++ b/drivers/net/can/Kconfig > @@ -69,6 +69,8 @@ source "drivers/net/can/sja1000/Kconfig" > > source "drivers/net/can/usb/Kconfig" > > +source "drivers/net/can/janz/Kconfig" > + > 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 7a702f2..00c24cd 100644 > --- a/drivers/net/can/Makefile > +++ b/drivers/net/can/Makefile > @@ -15,5 +15,6 @@ obj-$(CONFIG_CAN_AT91) += at91_can.o > obj-$(CONFIG_CAN_TI_HECC) += ti_hecc.o > obj-$(CONFIG_CAN_MCP251X) += mcp251x.o > obj-$(CONFIG_CAN_BFIN) += bfin_can.o > +obj-$(CONFIG_CAN_JANZ_CMODIO) += janz/ > > ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG > diff --git a/drivers/net/can/janz/Kconfig b/drivers/net/can/janz/Kconfig > new file mode 100644 > index 0000000..6f260f7 > --- /dev/null > +++ b/drivers/net/can/janz/Kconfig > @@ -0,0 +1,29 @@ > +config CAN_JANZ_CMODIO > + depends on CAN_DEV > + tristate "Support for Janz CMOD-IO PCI Carrier Board" > + ---help--- > + The Janz CMOD-IO PCI Carrier Board is a MODULbus to PCI bridge > + which allows many types of MODULbus modules to be used. > + > +if CAN_JANZ_CMODIO > + > +config CAN_JANZ_ICAN3 > + tristate "Janz VMOD-ICAN3 CAN controller" > + ---help--- > + If you say yes here you get support for the Janz VMOD-ICAN3 > + intelligent CAN module. > + > + This driver can also be built as a module. If so, the module > + will be called janz-ican3.ko. > + > +config CAN_JANZ_TTL > + tristate "Janz VMOD-TTL GPIO controller" > + ---help--- > + If you say yes here you get support for the Janz VMOD-TTL > + GPIO module. > + > + This driver can also be built as a module. If so, the module > + will be called janz-ttl.ko > + > +endif > + > diff --git a/drivers/net/can/janz/Makefile b/drivers/net/can/janz/Makefile > new file mode 100644 > index 0000000..a5c5e67 > --- /dev/null > +++ b/drivers/net/can/janz/Makefile > @@ -0,0 +1,5 @@ > +obj-$(CONFIG_JANZ_CMODIO) += janz-cmodio.o > +obj-$(CONFIG_JANZ_ICAN3) += janz-ican3.o > +obj-$(CONFIG_JANZ_TTL) += janz-ttl.o > + > +ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG > diff --git a/drivers/net/can/janz/janz-cmodio.c > b/drivers/net/can/janz/janz-cmodio.c > new file mode 100644 > index 0000000..3827902 > --- /dev/null > +++ b/drivers/net/can/janz/janz-cmodio.c > @@ -0,0 +1,343 @@ > +/* > + * Janz CMOD-IO MODULbus Carrier Board PCI Driver > + * > + * Copyright (c) 2010 Ira W. Snyder <[email protected]> > + * > + * Lots of inspiration and code was copied from drivers/mfd/sm501.c > + * > + * This file is licensed under the terms of the GNU General Public License > + * version 2. This program is licensed "as is" without any warranty of any > + * kind, whether express or implied. > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/pci.h> > +#include <linux/interrupt.h> > +#include <linux/delay.h> > +#include <linux/platform_device.h> > + > +#include "janz.h" > + > +#define DRV_NAME "janz-cmodio" > + > +/* Maximum number of buffers on a CMOD-IO carrier board */ > +#define JANZ_MAX_MODULES 4 > + > +struct janz_device { > + struct device *dev; > + struct pci_dev *pdev; > + > + void __iomem *modulbus_regs; > + void __iomem *onboard_regs; > + > + /* hex switch position */ > + u8 hex; > + > + /* list of available modules */ > + struct list_head devices; > +}; > + > +/*----------------------------------------------------------------------------*/ > +/* Subdevice Support > */ > +/*----------------------------------------------------------------------------*/ As multi-line comment should look like: /* * Subdevice Support */ What you have looks similar to the MFD deriver in drivers/mfd. They also serve as example. For the time being, I will focus on the CAN driver. [snip] > diff --git a/drivers/net/can/janz/janz-ican3.c > b/drivers/net/can/janz/janz-ican3.c > new file mode 100644 > index 0000000..640c4bb > --- /dev/null > +++ b/drivers/net/can/janz/janz-ican3.c > @@ -0,0 +1,1657 @@ > +/* > + * Janz MODULbus VMOD-ICAN3 CAN Interface Driver > + * > + * Copyright (c) 2010 Ira W. Snyder <[email protected]> > + * > + * This file is licensed under the terms of the GNU General Public License > + * version 2. This program is licensed "as is" without any warranty of any > + * kind, whether express or implied. > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/interrupt.h> > +#include <linux/delay.h> > +#include <linux/platform_device.h> > + > +#include <linux/netdevice.h> > +#include <linux/can.h> > +#include <linux/can/dev.h> > +#include <linux/can/error.h> > + > +#include "janz.h" > + > +/* the DPM has 64k of memory, organized into 256x 256 byte pages */ > +#define DPM_NUM_PAGES 256 > +#define DPM_PAGE_SIZE 256 > +#define DPM_PAGE_ADDR(p) ((p) * DPM_PAGE_SIZE) > + > +/* Janz ICAN3 DPM control registers */ > +#define DPM_REG_ADDR 0x100 > +#define DPM_REG_INT 0x102 > +#define DPM_REG_HWRESET 0x104 > +#define DPM_REG_TPUINT 0x106 > + > +/* Janz "old-style" host interface control registers */ > +#define MSYNC_PEER 0x00 /* ICAN only */ > +#define MSYNC_LOCL 0x01 /* host only */ > +#define TARGET_RUNNING 0x02 > + > +/* Janz "new-style" host interface queue page numbers */ > +#define QUEUE_TOHOST 5 > +#define QUEUE_FROMHOST_MID 6 > +#define QUEUE_FROMHOST_HIGH 7 > +#define QUEUE_FROMHOST_LOW 8 > + > +/* Janz "new-style" and "fast" host interface descriptor flags */ > +#define DESC_VALID 0x80 > +#define DESC_WRAP 0x40 > +#define DESC_INTERRUPT 0x20 > +#define DESC_IVALID 0x10 > +#define DESC_LEN(len) (len) > + > +/* Janz Firmware Messages */ > +#define MSG_CONNECTI 0x02 > +#define MSG_DISCONNECT 0x03 > +#define MSG_IDVERS 0x04 > +#define MSG_MSGLOST 0x05 > +#define MSG_NEWHOSTIF 0x08 > +#define MSG_SETAFILMASK 0x10 > +#define MSG_INITFDPMQUEUE 0x11 > +#define MSG_HWCONF 0x12 > +#define MSG_FMSGLOST 0x15 > +#define MSG_CEVTIND 0x37 > +#define MSG_CBTRREQ 0x41 > +#define MSG_COFFREQ 0x42 > +#define MSG_CONREQ 0x43 > + > +/* Number of buffers for use in the "new-style" host interface */ > +#define JANZ_NEW_BUFFERS 16 > + > +/* Number of buffers for use in the "fast" host interface */ > +#define JANZ_FAST_BUFFERS 256 > + > +/* Driver Name */ > +#define DRV_NAME "janz-ican3" > + > +struct janz_ican3 { > + > + /* must be the first member */ > + struct can_priv can; > + > + /* CAN network device */ > + struct net_device *ndev; > + > + /* Device for printing */ > + struct device *dev; > + > + /* module number */ > + unsigned int num; > + > + /* base address of registers and IRQ */ > + void __iomem *ctrl; > + void __iomem *regs; > + int irq; > + > + /* old and new style host interface */ > + unsigned int iftype; > + spinlock_t lock; > + > + /* new host interface */ > + unsigned int rx_int; > + unsigned int rx_num; > + unsigned int tx_num; > + > + /* fast host interface */ > + unsigned int fastrx_start; > + unsigned int fastrx_int; > + unsigned int fastrx_num; > + unsigned int fasttx_start; > + unsigned int fasttx_num; > + > + /* first free DPM page */ > + unsigned int free_page; > + > + /* interrupt handling */ > + struct work_struct work; > +}; > + > +struct janz_msg { > + u8 control; > + u8 spec; > + __le16 len; > + u8 data[252]; > +}; > + > +struct janz_new_desc { > + u8 control; > + u8 pointer; > +}; > + > +struct janz_fast_desc { > + u8 control; > + u8 command; > + u8 data[14]; > +}; > + > +/*----------------------------------------------------------------------------*/ > +/* DPM Register Access > */ > +/*----------------------------------------------------------------------------*/ See above. > +/* write to the window basic address register */ > +static inline void janz_set_page(struct janz_ican3 *mod, unsigned int page) > +{ > + /* the DPM only has 256 pages */ > + BUG_ON(page >= 256); > + iowrite8(page, mod->regs + DPM_REG_ADDR); > +} > + > +/* clear a MODULbus interrupt */ > +static inline void janz_clr_int(struct janz_ican3 *mod) > +{ > + ioread8(mod->regs + DPM_REG_INT); > +} > + > +/* generate a MODULbus interrupt */ > +static inline void janz_set_int(struct janz_ican3 *mod) > +{ > + iowrite8(0x01, mod->regs + DPM_REG_INT); > +} > + > +/*----------------------------------------------------------------------------*/ > +/* Onboard Registers > */ > +/*----------------------------------------------------------------------------*/ > + > +static inline void janz_disable_interrupts(struct janz_ican3 *mod) > +{ > + iowrite8(1 << mod->num, mod->ctrl + JANZ_OB_INT_DISABLE); > +} > + > +static inline void janz_enable_interrupts(struct janz_ican3 *mod) > +{ > + iowrite8(1 << mod->num, mod->ctrl + JANZ_OB_INT_ENABLE); > +} > + > +/*----------------------------------------------------------------------------*/ > +/* Janz "old-style" host interface > */ > +/*----------------------------------------------------------------------------*/ > + > +/* Get the MSYNC bits from the "old-style" interface control registers */ > +static void janz_get_msync(struct janz_ican3 *mod, u8 *locl, u8 *peer) > +{ > + janz_set_page(mod, 0); > + *peer = ioread8(mod->regs + MSYNC_PEER); > + *locl = ioread8(mod->regs + MSYNC_LOCL); > +} What are your arguments against using structures to describe the register layout? > +/* > + * Recieve a message from the Janz "old-style" firmware interface > + * > + * returns 0 on success, -ENOMEM when no message exists > + */ > +static int janz_old_recv_msg(struct janz_ican3 *mod, struct janz_msg *msg) > +{ > + u8 locl, peer, xord; > + unsigned int mbox; > + > + /* get the MSYNC registers */ > + janz_get_msync(mod, &locl, &peer); > + xord = locl ^ peer; > + > + if ((xord & 0x03) == 0x00) { > + dev_dbg(mod->dev, "no mbox for reading\n"); > + return -ENOMEM; > + } > + > + /* find the first free mbox to read */ > + if ((xord & 0x03) == 0x03) > + mbox = (xord & 0x04) ? 0 : 1; > + else > + mbox = (xord & 0x01) ? 0 : 1; > + > + /* copy the message */ > + janz_set_page(mod, mbox + 1); > + memcpy_fromio(msg, mod->regs, sizeof(*msg)); > + > + /* > + * notify the firmware that the read buffer is available > + * for it to fill again > + */ > + locl ^= (1 << mbox); > + > + janz_set_page(mod, 0); > + iowrite8(locl, mod->regs + MSYNC_LOCL); > + return 0; > +} > + > +/* > + * Send a message through the "old-style" firmware interface > + * > + * returns 0 on success, -ENOMEM when no free space exists > + */ > +static int janz_old_send_msg(struct janz_ican3 *mod, struct janz_msg *msg) > +{ > + u8 locl, peer, xord; > + unsigned int mbox; > + > + /* get the MSYNC registers */ > + janz_get_msync(mod, &locl, &peer); > + xord = locl ^ peer; > + > + if ((xord & 0x30) == 0x30) { > + dev_err(mod->dev, "no mbox for writing\n"); > + return -ENOMEM; > + } Here and in many other places macro definitions would make the code more readable. > + /* calculate a free mbox to use */ > + mbox = (xord & 0x10) ? 1 : 0; > + > + /* copy the message to the DPM */ > + janz_set_page(mod, mbox + 3); > + memcpy_toio(mod->regs, msg, sizeof(*msg)); > + > + locl ^= (mbox == 0) ? 0x10 : 0x20; > + locl |= (mbox == 0) ? 0x00 : 0x40; > + > + janz_set_page(mod, 0); > + iowrite8(locl, mod->regs + MSYNC_LOCL); > + return 0; > +} > + > +/*----------------------------------------------------------------------------*/ > +/* Janz "new-style" Host Interface Setup > */ > +/*----------------------------------------------------------------------------*/ > + > +static void janz_init_new_host_interface(struct janz_ican3 *mod) > +{ > + struct janz_new_desc desc; > + unsigned long flags; > + void __iomem *dst; > + int i; > + > + spin_lock_irqsave(&mod->lock, flags); > + > + dev_dbg(mod->dev, "%s: starting at page %d\n", __func__, > mod->free_page); > + > + /* setup the internal datastructures for RX */ > + mod->rx_num = 0; > + mod->rx_int = 0; > + > + /* tohost queue descriptors are in page 5 */ > + janz_set_page(mod, 5); > + dst = mod->regs; > + > + /* initialize the tohost (rx) queue descriptors: pages 9-24 */ > + for (i = 0; i < JANZ_NEW_BUFFERS; i++) { > + desc.control = DESC_INTERRUPT | DESC_LEN(1); /* I L=1 */ > + desc.pointer = mod->free_page; > + > + /* set wrap flag on last buffer */ > + if (i == JANZ_NEW_BUFFERS - 1) > + desc.control |= DESC_WRAP; > + > + memcpy_toio(dst, &desc, sizeof(desc)); > + dst += sizeof(desc); > + mod->free_page++; > + } > + > + /* fromhost (tx) mid queue descriptors are in page 6 */ > + janz_set_page(mod, 6); > + dst = mod->regs; > + > + /* setup the internal datastructures for TX */ > + mod->tx_num = 0; > + > + /* initialize the fromhost mid queue descriptors: pages 25-40 */ > + for (i = 0; i < JANZ_NEW_BUFFERS; i++) { > + desc.control = DESC_VALID | DESC_LEN(1); /* V L=1 */ > + desc.pointer = mod->free_page; > + > + /* set wrap flag on last buffer */ > + if (i == JANZ_NEW_BUFFERS - 1) > + desc.control |= DESC_WRAP; > + > + memcpy_toio(dst, &desc, sizeof(desc)); > + dst += sizeof(desc); > + mod->free_page++; > + } > + > + /* fromhost hi queue descriptors are in page 7 */ > + janz_set_page(mod, 7); > + dst = mod->regs; > + > + /* initialize only a single buffer in the fromhost hi queue (unused) */ > + desc.control = DESC_VALID | DESC_WRAP | DESC_LEN(1); /* VW L=1 */ > + desc.pointer = mod->free_page; > + memcpy_toio(dst, &desc, sizeof(desc)); > + mod->free_page++; > + > + /* fromhost low queue descriptors are in page 8 */ > + janz_set_page(mod, 8); > + dst = mod->regs; > + > + /* initialize only a single buffer in the fromhost low queue (unused) */ > + desc.control = DESC_VALID | DESC_WRAP | DESC_LEN(1); /* VW L=1 */ > + desc.pointer = mod->free_page; > + memcpy_toio(dst, &desc, sizeof(desc)); > + mod->free_page++; > + > + dev_dbg(mod->dev, "%s: next free page %d\n", __func__, mod->free_page); > + spin_unlock_irqrestore(&mod->lock, flags); > +} > + > +/*----------------------------------------------------------------------------*/ > +/* Janz Fast Host Interface Setup > */ > +/*----------------------------------------------------------------------------*/ > + > +static void janz_init_fast_host_interface(struct janz_ican3 *mod) > +{ > + struct janz_fast_desc desc; > + unsigned long flags; > + unsigned int addr; > + void __iomem *dst; > + int i; > + > + spin_lock_irqsave(&mod->lock, flags); > + dev_dbg(mod->dev, "%s: starting at page %d\n", __func__, > mod->free_page); > + > + /* save the start recv page */ > + mod->fastrx_start = mod->free_page; > + mod->fastrx_num = 0; > + mod->fastrx_int = 0; > + > + /* build a single fast tohost queue descriptor */ > + memset(&desc, 0, sizeof(desc)); > + desc.control = 0x00; > + desc.command = 1; > + > + /* build the tohost queue descriptor ring in memory */ > + addr = 0; > + for (i = 0; i < JANZ_FAST_BUFFERS; i++) { > + > + /* set the wrap bit on the last buffer */ > + if (i == JANZ_FAST_BUFFERS - 1) > + desc.control |= DESC_WRAP; > + > + /* switch to the correct page */ > + janz_set_page(mod, mod->free_page); > + > + /* copy the descriptor to the DPM */ > + dst = mod->regs + addr; > + memcpy_toio(dst, &desc, sizeof(desc)); > + addr += sizeof(desc); > + > + /* move to the next page if necessary */ > + if (addr >= 256) { Macro definitons! See above. > + addr = 0; > + mod->free_page++; > + } > + } > + > + /* make sure we page-align the next queue */ > + if (addr != 0) > + mod->free_page++; > + > + /* save the start xmit page */ > + mod->fasttx_start = mod->free_page; > + mod->fasttx_num = 0; > + > + /* build a single fast fromhost queue descriptor */ > + memset(&desc, 0, sizeof(desc)); > + desc.control = DESC_VALID; > + desc.command = 1; > + > + /* build the fromhost queue descriptor ring in memory */ > + addr = 0; > + for (i = 0; i < JANZ_FAST_BUFFERS; i++) { > + > + /* set the wrap bit on the last buffer */ > + if (i == JANZ_FAST_BUFFERS - 1) > + desc.control |= DESC_WRAP; > + > + /* switch to the correct page */ > + janz_set_page(mod, mod->free_page); > + > + /* copy the descriptor to the DPM */ > + dst = mod->regs + addr; > + memcpy_toio(dst, &desc, sizeof(desc)); > + addr += sizeof(desc); > + > + /* move to the next page if necessary */ > + if (addr >= 256) { > + addr = 0; > + mod->free_page++; > + } > + } > + > + dev_dbg(mod->dev, "%s: next free page %d\n", __func__, mod->free_page); > + spin_unlock_irqrestore(&mod->lock, flags); > +} > + > +/*----------------------------------------------------------------------------*/ > +/* Janz "new-style" Host Interface Message Helpers > */ > +/*----------------------------------------------------------------------------*/ > + > +/* > + * LOCKING: must hold mod->lock > + */ > +static int janz_new_send_msg(struct janz_ican3 *mod, struct janz_msg *msg) > +{ > + struct janz_new_desc desc; > + void __iomem *desc_addr = mod->regs + (mod->tx_num * sizeof(desc)); > + > + /* switch to the fromhost mid queue, and read the buffer descriptor */ > + janz_set_page(mod, 6); > + memcpy_fromio(&desc, desc_addr, sizeof(desc)); > + > + if (!(desc.control & DESC_VALID)) { > + dev_dbg(mod->dev, "%s: no free buffers\n", __func__); > + return -ENOMEM; > + } > + > + /* switch to the data page, copy the data */ > + janz_set_page(mod, desc.pointer); > + memcpy_toio(mod->regs, msg, sizeof(*msg)); > + > + /* switch back to the descriptor, set the valid bit, write it back */ > + janz_set_page(mod, 6); > + desc.control ^= DESC_VALID; > + memcpy_toio(desc_addr, &desc, sizeof(desc)); > + > + /* update the tx number */ > + mod->tx_num = (desc.control & DESC_WRAP) ? 0 : (mod->tx_num + 1); > + dev_dbg(mod->dev, "%s: update TX num -> %d\n", __func__, mod->tx_num); > + > + return 0; > +} > + > +/* > + * LOCKING: must hold mod->lock > + */ > +static int janz_new_recv_msg(struct janz_ican3 *mod, struct janz_msg *msg) > +{ > + struct janz_new_desc desc; > + void __iomem *desc_addr = mod->regs + (mod->rx_num * sizeof(desc)); > + > + /* switch to the tohost queue, and read the buffer descriptor */ > + janz_set_page(mod, 5); > + memcpy_fromio(&desc, desc_addr, sizeof(desc)); > + > + if (!(desc.control & DESC_VALID)) { > + dev_dbg(mod->dev, "%s: no buffers to recv\n", __func__); > + return -ENOMEM; > + } > + > + /* switch to the data page, copy the data */ > + janz_set_page(mod, desc.pointer); > + memcpy_fromio(msg, mod->regs, sizeof(*msg)); > + > + /* switch back to the descriptor, toggle the valid bit, write it back */ > + janz_set_page(mod, 5); > + desc.control ^= DESC_VALID; > + memcpy_toio(desc_addr, &desc, sizeof(desc)); > + > + /* update the rx number */ > + mod->rx_num = (desc.control & DESC_WRAP) ? 0 : (mod->rx_num + 1); > + dev_dbg(mod->dev, "%s: update RX num -> %d\n", __func__, mod->rx_num); > + > + return 0; > +} > + > +/*----------------------------------------------------------------------------*/ > +/* Message Send / Recv Helpers > */ > +/*----------------------------------------------------------------------------*/ > + > +static int janz_send_msg(struct janz_ican3 *mod, struct janz_msg *msg) > +{ > + unsigned long flags; > + int ret; > + > + spin_lock_irqsave(&mod->lock, flags); > + > + if (mod->iftype == 0) > + ret = janz_old_send_msg(mod, msg); > + else > + ret = janz_new_send_msg(mod, msg); > + > + spin_unlock_irqrestore(&mod->lock, flags); > + return ret; > +} > + > +static int janz_recv_msg(struct janz_ican3 *mod, struct janz_msg *msg) > +{ > + unsigned long flags; > + int ret; > + > + spin_lock_irqsave(&mod->lock, flags); > + > + if (mod->iftype == 0) > + ret = janz_old_recv_msg(mod, msg); > + else > + ret = janz_new_recv_msg(mod, msg); > + > + spin_unlock_irqrestore(&mod->lock, flags); > + return ret; > +} > + > +/*----------------------------------------------------------------------------*/ > +/* Quick Pre-constructed Messages > */ > +/*----------------------------------------------------------------------------*/ > + > +static int janz_msg_connect(struct janz_ican3 *mod) > +{ > + struct janz_msg msg; > + int ret; > + > + memset(&msg, 0, sizeof(msg)); > + msg.control = 0x00; > + msg.spec = MSG_CONNECTI; > + msg.len = cpu_to_le16(0); > + > + ret = janz_send_msg(mod, &msg); > + if (ret) { > + dev_dbg(mod->dev, "unable to send CONNECT message\n"); > + return ret; > + } > + > + return 0; > +} > + > +static int janz_msg_disconnect(struct janz_ican3 *mod) > +{ > + struct janz_msg msg; > + int ret; > + > + memset(&msg, 0, sizeof(msg)); > + msg.control = 0x00; > + msg.spec = MSG_DISCONNECT; > + msg.len = cpu_to_le16(0); > + > + ret = janz_send_msg(mod, &msg); > + if (ret) { > + dev_dbg(mod->dev, "unable to send DISCONNECT message\n"); > + return ret; > + } > + > + return 0; > +} > + > +static int janz_msg_newhostif(struct janz_ican3 *mod) > +{ > + struct janz_msg msg; > + int ret; > + > + memset(&msg, 0, sizeof(msg)); > + msg.control = 0x00; > + msg.spec = MSG_NEWHOSTIF; > + msg.len = cpu_to_le16(0); > + > + /* If we're not using the old interface, switching seems bogus */ > + WARN_ON(mod->iftype != 0); > + > + ret = janz_send_msg(mod, &msg); > + if (ret) { > + dev_dbg(mod->dev, "unable to send NEWHOSTIF message\n"); > + return ret; > + } > + > + /* mark the module as using the new host interface */ > + mod->iftype = 1; > + return 0; > +} > + > +static int janz_msg_fasthostif(struct janz_ican3 *mod) > +{ > + struct janz_msg msg; > + unsigned int addr; > + int ret; > + > + memset(&msg, 0, sizeof(msg)); > + msg.control = 0x00; > + msg.spec = MSG_INITFDPMQUEUE; > + msg.len = cpu_to_le16(8); Please do not align expressions. Just use *one* space before and after "=". Please fix globally. > + > + /* write the tohost queue start address */ > + addr = DPM_PAGE_ADDR(mod->fastrx_start); > + msg.data[0] = addr & 0xff; > + msg.data[1] = (addr >> 8) & 0xff; > + msg.data[2] = (addr >> 16) & 0xff; > + msg.data[3] = (addr >> 24) & 0xff; > + > + /* write the fromhost queue start address */ > + addr = DPM_PAGE_ADDR(mod->fasttx_start); > + msg.data[4] = addr & 0xff; > + msg.data[5] = (addr >> 8) & 0xff; > + msg.data[6] = (addr >> 16) & 0xff; > + msg.data[7] = (addr >> 24) & 0xff; > + > + /* If we're not using the new interface yet, we cannot do this */ > + WARN_ON(mod->iftype != 1); > + > + ret = janz_send_msg(mod, &msg); > + if (ret) { > + dev_dbg(mod->dev, "unable to send FASTHOSTIF message\n"); > + return ret; > + } > + > + return 0; > +} > + > +/* > + * Setup the CAN filter to either accept or reject all > + * messages from the CAN bus. > + */ > +static int janz_set_id_filter(struct janz_ican3 *mod, bool accept) > +{ > + struct janz_msg msg; > + int ret; > + > + /* Standard Frame Format */ > + memset(&msg, 0, sizeof(msg)); > + msg.control = 0x00; > + msg.spec = MSG_SETAFILMASK; > + msg.len = cpu_to_le16(5); Ditto. > + msg.data[0] = 0x00; /* IDLo LSB */ > + msg.data[1] = 0x00; /* IDLo MSB */ > + msg.data[2] = 0xff; /* IDHi LSB */ > + msg.data[3] = 0x07; /* IDHi MSB */ > + > + /* accept all frames for fast host if, or reject all frames */ > + msg.data[4] = accept ? 0x02 : 0x00; > + > + ret = janz_send_msg(mod, &msg); > + if (ret) { > + dev_dbg(mod->dev, "unable to send SETAFILMASK message\n"); > + return ret; > + } > + > + /* Extended Frame Format */ > + memset(&msg, 0, sizeof(msg)); > + msg.control = 0x00; > + msg.spec = MSG_SETAFILMASK; > + msg.len = cpu_to_le16(13); > + msg.data[0] = 0; /* MUX = 0 */ > + msg.data[1] = 0x00; /* IDLo LSB */ > + msg.data[2] = 0x00; > + msg.data[3] = 0x00; > + msg.data[4] = 0x20; /* IDLo MSB */ > + msg.data[5] = 0xff; /* IDHi LSB */ > + msg.data[6] = 0xff; > + msg.data[7] = 0xff; > + msg.data[8] = 0x3f; /* IDHi MSB */ > + > + /* accept all frames for fast host if, or reject all frames */ > + msg.data[9] = accept ? 0x02 : 0x00; > + > + ret = janz_send_msg(mod, &msg); > + if (ret) { > + dev_dbg(mod->dev, "unable to send SETAFILMASK message\n"); > + return ret; > + } > + > + return 0; > +} > + > +/* > + * Bring the CAN bus online or offline > + */ > +static int janz_set_bus_state(struct janz_ican3 *mod, bool on) > +{ > + struct janz_msg msg; > + int ret; > + > + memset(&msg, 0, sizeof(msg)); > + msg.control = 0x00; > + msg.spec = on ? MSG_CONREQ : MSG_COFFREQ; > + msg.len = cpu_to_le16(0); > + > + dev_dbg(mod->dev, "%s: %s request: spec %.2x\n", __func__, on ? "on" : > "off", msg.spec); > + ret = janz_send_msg(mod, &msg); > + if (ret) { > + dev_dbg(mod->dev, "unable to send CONREQ/COFFREQ message\n"); > + return ret; > + } > + > + return 0; > +} > + > +static int janz_set_termination(struct janz_ican3 *mod, bool on) > +{ > + struct janz_msg msg; > + int ret; > + > + memset(&msg, 0, sizeof(msg)); > + msg.control = 0x00; > + msg.spec = MSG_HWCONF; > + msg.len = cpu_to_le16(2); > + msg.data[0] = 0x00; > + msg.data[1] = on ? 0x01 : 0x00; > + > + ret = janz_send_msg(mod, &msg); > + if (ret) { > + dev_dbg(mod->dev, "unable to send HWCONF message\n"); > + return ret; > + } > + > + return 0; > +} > + > +/*----------------------------------------------------------------------------*/ > +/* Janz to CAN Frame Conversion > */ > +/*----------------------------------------------------------------------------*/ > + > +static void janz_to_can(struct janz_ican3 *mod, struct janz_fast_desc *desc, > + struct can_frame *cf) > +{ > + if ((desc->command & 0x0f) == 0) { > + dev_dbg(mod->dev, "%s: old frame format\n", __func__); > + if (desc->data[1] & 0x10) > + cf->can_id |= CAN_RTR_FLAG; > + > + cf->can_id |= desc->data[0] << 3; > + cf->can_id |= (desc->data[1] & 0xe0) >> 5; > + cf->can_dlc = desc->data[1] & 0x0f; > + memcpy(cf->data, &desc->data[2], sizeof(cf->data)); > + } else { > + dev_dbg(mod->dev, "%s: new frame format\n", __func__); > + cf->can_dlc = desc->data[0] & 0x0f; > + if (desc->data[0] & 0x40) > + cf->can_id |= CAN_RTR_FLAG; > + > + if (desc->data[0] & 0x80) { > + cf->can_id |= CAN_EFF_FLAG; > + cf->can_id |= desc->data[2] << 21; /* 28-21 */ > + cf->can_id |= desc->data[3] << 13; /* 20-13 */ > + cf->can_id |= desc->data[4] << 5; /* 12-5 */ > + cf->can_id |= (desc->data[5] & 0xf8) >> 3; > + } else { > + cf->can_id |= desc->data[2] << 3; /* 10-3 */ > + cf->can_id |= desc->data[3] >> 5; /* 2-0 */ > + } > + > + memcpy(cf->data, &desc->data[6], sizeof(cf->data)); > + } > +} > + > +static void can_to_janz(struct janz_ican3 *mod, struct can_frame *cf, > + struct janz_fast_desc *desc) Use a better name please. > +{ > + /* clear out any stale data in the descriptor */ > + memset(desc->data, 0, sizeof(desc->data)); > + > + /* we always use the extended format, with the ECHO flag set */ > + desc->command = 1; > + desc->data[0] |= cf->can_dlc; > + desc->data[1] |= 0x10; /* echo */ > + > + if (cf->can_id & CAN_RTR_FLAG) > + desc->data[0] |= 0x40; > + > + /* pack the id into the correct places */ > + if (cf->can_id & CAN_EFF_FLAG) { > + dev_dbg(mod->dev, "%s: extended frame\n", __func__); > + desc->data[0] |= 0x80; /* extended id frame */ > + desc->data[2] = (cf->can_id & 0x1fe00000) >> 21; /* 28-21 */ > + desc->data[3] = (cf->can_id & 0x001fe000) >> 13; /* 20-13 */ > + desc->data[4] = (cf->can_id & 0x00001fe0) >> 5; /* 12-5 */ > + desc->data[5] = (cf->can_id & 0x0000001f) << 3; /* 4-0 */ > + } else { > + dev_dbg(mod->dev, "%s: standard frame\n", __func__); > + desc->data[2] = (cf->can_id & 0x7F8) >> 3; /* bits 10-3 */ > + desc->data[3] = (cf->can_id & 0x007) << 5; /* bits 2-0 */ > + } > + > + /* copy the data bits into the descriptor */ > + memcpy(&desc->data[6], cf->data, sizeof(cf->data)); > +} > + > +static int janz_err_frame(struct janz_ican3 *mod, canid_t idflags, u8 d1) > +{ > + struct net_device *ndev = mod->ndev; > + struct net_device_stats *stats = &ndev->stats; > + struct can_frame *cf; > + struct sk_buff *skb; > + > + skb = alloc_can_err_skb(ndev, &cf); > + if (unlikely(skb == NULL)) > + return -ENOMEM; > + > + cf->can_id |= idflags; > + cf->data[1] = d1; Hm, the data field to be used depends on the error type. > + > + netif_rx(skb); > + > + stats->rx_packets++; > + stats->rx_bytes += cf->can_dlc; > + return 0; > +} > + > +/*----------------------------------------------------------------------------*/ > +/* Interrupt Handling > */ > +/*----------------------------------------------------------------------------*/ > + > +static void janz_handle_idvers(struct janz_ican3 *mod, struct janz_msg *msg) > +{ > + dev_dbg(mod->dev, "%s: %s\n", __func__, msg->data); > +} > + > +static void janz_handle_msglost(struct janz_ican3 *mod, struct janz_msg *msg) > +{ > + char *queue; > + > + if (msg->spec == MSG_MSGLOST) > + queue = "new"; > + else > + queue = "fast"; > + > + dev_dbg(mod->dev, "%s: %s hostif: %d messages lost\n", > + __func__, queue, msg->data[0]); > +} > + > +static void janz_handle_cevtind(struct janz_ican3 *mod, struct janz_msg *msg) > +{ > + enum can_state state; > + u8 rxerr, txerr, err; > + int i; > + > + dev_dbg(mod->dev, "%s: message len: %d\n", __func__, > le16_to_cpu(msg->len)); > + for (i = 0; i < le16_to_cpu(msg->len); i++) > + dev_dbg(mod->dev, "%s: data[%.2d] -> %.2x\n", __func__, i, > msg->data[i]); > + > + /* we can only handle the SJA1000 part */ > + if (msg->data[1] != 0x02) { > + dev_err(mod->dev, "unable to handle errors on non-SJA1000\n"); > + return; > + } > + > + /* check the message length for sanity */ > + if (msg->len < 6) { > + dev_dbg(mod->dev, "unable to handle short error message\n"); > + return; > + } > + > + rxerr = msg->data[4]; > + txerr = msg->data[5]; Should go to field 6 and 7. > + > + /* state is error-active by default */ > + state = CAN_STATE_ERROR_ACTIVE; > + > + if (rxerr >= 96 || txerr >= 96) > + state = CAN_STATE_ERROR_WARNING; > + > + if (rxerr >= 128 || txerr >= 128) > + state = CAN_STATE_ERROR_PASSIVE; > + > + if (rxerr >= 255 || txerr >= 255) > + state = CAN_STATE_BUS_OFF; You could use "else if" if you revert the order. Also, 255 does not yet mean bus-error, strictly speaking. Have you seen the device going bus-off? > + > + /* check if we should generate an error frame at all */ > + if (state == mod->can.state || mod->can.state == CAN_STATE_STOPPED) { > + dev_dbg(mod->dev, "no error frame needed: state %d\n", state); > + return; > + } Shouldn't this check be done first? > + dev_dbg(mod->dev, "state change: state %d\n", state); > + mod->can.state = state; > + > + if (state == CAN_STATE_BUS_OFF) { > + janz_err_frame(mod, CAN_ERR_BUSOFF, 0); > + return; > + } > + > + if (state == CAN_STATE_ERROR_PASSIVE) { > + err = (rxerr >= 128) ? CAN_ERR_CRTL_RX_PASSIVE > + : CAN_ERR_CRTL_TX_PASSIVE; > + janz_err_frame(mod, CAN_ERR_CRTL, err); > + return; > + } > + > + if (state == CAN_STATE_ERROR_WARNING) { > + err = (rxerr >= 96) ? CAN_ERR_CRTL_RX_WARNING > + : CAN_ERR_CRTL_TX_WARNING; > + janz_err_frame(mod, CAN_ERR_CRTL, err); > + return; > + } If you use "else if" as suggested, the code could be shortened a lot, I think (by doing everything within the if/else block). Just for curiosity, what does "candump -t d any,0:0,#FFFFFFFF" report when you trigger a bus-error or when you send a message with no cable connected. > + /* nothing needed for error-active state */ > +} > + > +static void janz_handle_unknown(struct janz_ican3 *mod, struct janz_msg *msg) Handle what? > +{ > + u16 len; > + int i; > + > + len = le16_to_cpu(msg->len); > + dev_dbg(mod->dev, "%s: modno %d UNKNOWN spec 0x%.2x len %d\n", > + __func__, mod->num, msg->spec, len); > + for (i = 0; i < len; i++) > + dev_dbg(mod->dev, "msg->data[%.2d] -> 0x%.2x\n", i, > msg->data[i]); > +} > + > +/* > + * Handle a control message from the firmware > + */ > +static void janz_handle_message(struct janz_ican3 *mod, struct janz_msg *msg) > +{ > + dev_dbg(mod->dev, "%s: modno %d spec 0x%.2x len %d bytes\n", __func__, > + mod->num, msg->spec, le16_to_cpu(msg->len)); > + > + switch (msg->spec) { > + case MSG_IDVERS: > + janz_handle_idvers(mod, msg); > + break; > + case MSG_MSGLOST: > + case MSG_FMSGLOST: > + janz_handle_msglost(mod, msg); > + break; You shoud create error messages for msglost as well. > + case MSG_CEVTIND: > + janz_handle_cevtind(mod, msg); > + break; > + default: > + janz_handle_unknown(mod, msg); > + break; > + } > +} > + > +/* > + * Recieve one CAN frame from the hardware > + * > + * This works like the core of a NAPI function, but is intended to be called > + * from workqueue context instead. This driver already needs a workqueue to > + * process control messages, so we use the workqueue instead of using NAPI. > + * This was done to simplify locking. > + * > + * CONTEXT: must be called from user context > + */ > +static int janz_recv_skb(struct janz_ican3 *mod) > +{ > + struct net_device *ndev = mod->ndev; > + struct net_device_stats *stats = &ndev->stats; > + struct janz_fast_desc desc; > + void __iomem *desc_addr; > + struct can_frame *cf; > + struct sk_buff *skb; > + unsigned long flags; > + int ret; > + > + dev_dbg(mod->dev, "%s: modno %d called\n", __func__, mod->num); > + spin_lock_irqsave(&mod->lock, flags); > + > + /* copy the whole descriptor */ > + janz_set_page(mod, mod->fastrx_start + (mod->fastrx_num / 16)); > + desc_addr = mod->regs + ((mod->fastrx_num % 16) * sizeof(desc)); > + memcpy_fromio(&desc, desc_addr, sizeof(desc)); > + > + /* check that we actually have a CAN frame */ > + if (!(desc.control & DESC_VALID)) { > + dev_dbg(mod->dev, "%s: no more frames\n", __func__); > + ret = -ENOBUFS; > + goto out_unlock; > + } > + > + /* allocate an skb */ > + skb = alloc_can_skb(ndev, &cf); > + if (unlikely(skb == NULL)) { > + dev_dbg(mod->dev, "%s: no more skbs\n", __func__); > + stats->rx_dropped++; > + goto err_noalloc; > + } > + > + /* convert the Janz frame into CAN format */ > + janz_to_can(mod, &desc, cf); > + > + /* receive the skb, update statistics */ > + netif_rx(skb); > + stats->rx_packets++; > + stats->rx_bytes += cf->can_dlc; > + > +err_noalloc: > + /* toggle the valid bit and return the descriptor to the ring */ > + desc.control ^= DESC_VALID; > + memcpy_toio(desc_addr, &desc, 1); > + > + /* update the next buffer pointer */ > + mod->fastrx_num = (desc.control & DESC_WRAP) ? 0 : (mod->fastrx_num + > 1); > + dev_dbg(mod->dev, "%s: update fast RX num -> %d\n", __func__, > mod->fastrx_num); > + > + /* there are still more buffers to process */ > + ret = 0; > + > +out_unlock: > + spin_unlock_irqrestore(&mod->lock, flags); > + return ret; > +} > + > +static void janz_work(struct work_struct *work) > +{ > + struct janz_ican3 *mod = container_of(work, struct janz_ican3, work); > + unsigned int handled = 0; > + struct janz_msg msg; > + int ret; > + > + dev_dbg(mod->dev, "%s: module number %d\n", __func__, mod->num); > + > + /* process all communication messages */ > + while (true) { > + > + ret = janz_recv_msg(mod, &msg); > + if (ret) { > + dev_dbg(mod->dev, "%s: no more messages\n", __func__); > + break; > + } > + > + janz_handle_message(mod, &msg); > + handled++; > + } > + > + /* process all CAN frames from the fast interface */ > + while (true) { > + > + ret = janz_recv_skb(mod); > + if (ret) { > + dev_dbg(mod->dev, "%s: no more CAN frames\n", __func__); > + break; > + } > + > + handled++; > + } > + > + dev_dbg(mod->dev, "%s: handled %d messages\n", __func__, handled); > +} > + > +/* > + * Handle a MODULbus interrupt > + * > + * Due to the way the firmware works, we must first go through all of the > + * buffers and unset their IVALID flag, then notify our work function to > + * process the message. The IVALID flag must be unset before clearing the > + * interrupt. > + * > + * Only after the message has been processed can the VALID flag be unset. > + */ > +static void janz_handle_interrupt(struct janz_ican3 *mod) > +{ > + unsigned long flags; > + void __iomem *addr; > + u8 control; > + > + spin_lock_irqsave(&mod->lock, flags); > + > + /* > + * If we're using the old-style host interface, we only need to > + * start the work function, since the fast host interface (and > + * therefore CAN frame reception) cannot be working yet > + */ > + if (mod->iftype == 0) { > + dev_dbg(mod->dev, "%s: old style host interface\n", __func__); > + schedule_work(&mod->work); > + goto out_unlock; > + } > + > + /* > + * Ok, at least the new-style host interface must be running, so we > + * need to go through it's buffers and unset all of their DESC_IVALID > + * bits before clearing the interrupt > + */ > + while (true) { > + > + dev_dbg(mod->dev, "%s: modno %d new style host interface\n", > __func__, mod->num); > + > + /* check the new host interface tohost queue */ > + janz_set_page(mod, 5); > + addr = mod->regs + (mod->rx_int * sizeof(struct janz_new_desc)); > + control = ioread8(addr); > + > + /* check if we're finished with buffers */ > + if (!(control & DESC_IVALID)) > + break; > + > + /* write the control bits back with IVALID unset */ > + control &= ~DESC_IVALID; > + iowrite8(control, addr); > + > + /* > + * update the interrupt handler's position and schedule > + * the work function to run at some point in the future > + */ > + mod->rx_int = (control & DESC_WRAP) ? 0 : (mod->rx_int + 1); > + schedule_work(&mod->work); > + } > + > + /* Check the fast host interface for interrupts */ > + while (true) { > + > + dev_dbg(mod->dev, "%s: modno %d fast interface\n", __func__, > mod->num); > + > + /* check the fast host interface */ > + janz_set_page(mod, mod->fastrx_start + (mod->fastrx_int / 16)); > + addr = mod->regs + ((mod->fastrx_int % 16) * sizeof(struct > janz_fast_desc)); > + control = ioread8(addr); > + > + /* check if we're finished with buffers */ > + if (!(control & DESC_IVALID)) > + break; > + > + /* write back the control bits with IVALID unset */ > + control &= ~DESC_IVALID; > + iowrite8(control, addr); This seems to be duplicated code. Here a helper function would make sense in contrast to your *one* line functions, e.g. to enable the interrupts, which just increases code size. > + > + /* > + * update the interrupt handler's position and schedule > + * the work function to run at some point in the future > + */ > + mod->fastrx_int = (control & DESC_WRAP) ? 0 : (mod->fastrx_int > + 1); > + schedule_work(&mod->work); > + } > + > +out_unlock: > + janz_clr_int(mod); > + spin_unlock_irqrestore(&mod->lock, flags); > +} > + > +static irqreturn_t janz_irq(int irq, void *dev_id) > +{ > + struct janz_ican3 *mod = dev_id; > + u8 stat; > + > + /* > + * The interrupt status register on this device reports interrupts > + * as zeroes instead of using ones like most other devices > + */ > + stat = ioread8(mod->ctrl + JANZ_OB_INT_STAT) & (1 << mod->num); > + dev_dbg(mod->dev, "IRQ: enter stat 0x%.2x\n", stat); > + if (stat == (1 << mod->num)) { > + dev_dbg(mod->dev, "IRQ: none pending\n"); > + return IRQ_NONE; > + } > + > + dev_dbg(mod->dev, "IRQ: module %d\n", mod->num); > + janz_handle_interrupt(mod); > + > + stat = ioread8(mod->ctrl + JANZ_OB_INT_STAT) & (1 << mod->num); > + dev_dbg(mod->dev, "IRQ: exit stat 0x%.2x\n", stat); > + > + return IRQ_HANDLED; > +} > + > +/*----------------------------------------------------------------------------*/ > +/* TEST CODE > */ > +/*----------------------------------------------------------------------------*/ > + > +/* > + * Reset an ICAN module to its power-on state > + * > + * CONTEXT: no network device registered > + * LOCKING: work function disabled > + */ > +static int janz_reset_module(struct janz_ican3 *mod) > +{ > + u8 val = 1 << mod->num; > + unsigned long start; > + u8 runold, runnew; > + > + /* disable interrupts so no more work is scheduled */ > + janz_disable_interrupts(mod); > + > + /* flush any pending work */ > + flush_scheduled_work(); > + > + /* the first unallocated page in the DPM is 9 */ > + mod->free_page = 9; > + > + janz_set_page(mod, 0); > + runold = ioread8(mod->regs + TARGET_RUNNING); > + > + /* reset the module */ > + iowrite8(val, mod->ctrl + JANZ_OB_RESET_ASSERT); > + iowrite8(val, mod->ctrl + JANZ_OB_RESET_DEASSERT); > + > + /* wait until the module has finished resetting and is running */ > + start = jiffies; > + do { > + janz_set_page(mod, 0); > + runnew = ioread8(mod->regs + TARGET_RUNNING); > + if (runnew == (runold ^ 0xff)) { > + dev_dbg(mod->dev, "%s: success\n", __func__); > + return 0; > + } > + > + dev_dbg(mod->dev, "%s: msleep(10)\n", __func__); > + msleep(10); > + } while (time_before(jiffies, start + HZ / 4)); > + > + dev_dbg(mod->dev, "%s: timed out\n", __func__); > + return -ETIMEDOUT; > +} > + > +static void janz_shutdown_module(struct janz_ican3 *mod) > +{ > + int ret; > + > + dev_dbg(mod->dev, "%s: disconnect and reset module\n", __func__); > + janz_msg_disconnect(mod); > + ret = janz_reset_module(mod); > + if (ret) > + dev_err(mod->dev, "unable to reset module\n"); > +} > + > +/* > + * Startup an ICAN module, bringing it into fast mode > + */ > +static int janz_startup_module(struct janz_ican3 *mod) > +{ > + int ret; > + > + dev_dbg(mod->dev, "%s: reset module\n", __func__); > + ret = janz_reset_module(mod); > + if (ret) { > + dev_err(mod->dev, "unable to reset module\n"); > + return ret; > + } > + > + /* re-enable interrupts so we can send messages */ > + janz_enable_interrupts(mod); > + > + dev_dbg(mod->dev, "%s: connect and switch to new if\n", __func__); > + ret = janz_msg_connect(mod); > + if (ret) { > + dev_err(mod->dev, "unable to connect to module\n"); > + return ret; > + } > + > + janz_init_new_host_interface(mod); > + ret = janz_msg_newhostif(mod); > + if (ret) { > + dev_err(mod->dev, "unable to switch to new-style interface\n"); > + return ret; > + } > + > + dev_dbg(mod->dev, "%s: enable termination\n", __func__); > + ret = janz_set_termination(mod, true); > + if (ret) { > + dev_err(mod->dev, "unable to enable termination\n"); > + return ret; > + } > + > + dev_dbg(mod->dev, "%s: start fast host if\n", __func__); > + janz_init_fast_host_interface(mod); > + ret = janz_msg_fasthostif(mod); > + if (ret) { > + dev_err(mod->dev, "unable to switch to fast host interface\n"); > + return ret; > + } > + > + dev_dbg(mod->dev, "%s: set filter to accept everything\n", __func__); > + ret = janz_set_id_filter(mod, true); > + if (ret) { > + dev_err(mod->dev, "unable to set acceptance filter\n"); > + return ret; > + } > + > + return 0; > +} > + > +/*----------------------------------------------------------------------------*/ > +/* CAN Network Device > */ > +/*----------------------------------------------------------------------------*/ > + > +static int janz_open(struct net_device *ndev) > +{ > + struct janz_ican3 *mod = netdev_priv(ndev); > + int ret; > + > + dev_dbg(mod->dev, "%s: called\n", __func__); > + > + /* bring the bus online */ > + ret = janz_set_bus_state(mod, true); > + if (ret) { > + dev_err(mod->dev, "unable to set bus-on\n"); > + return ret; > + } > + > + /* start up the network device */ > + mod->can.state = CAN_STATE_ERROR_ACTIVE; > + netif_start_queue(ndev); > + > + return 0; > +} > + > +static int janz_stop(struct net_device *ndev) > +{ > + struct janz_ican3 *mod = netdev_priv(ndev); > + int ret; > + > + dev_dbg(mod->dev, "%s: called\n", __func__); > + > + /* stop the network device xmit routine */ > + netif_stop_queue(ndev); > + mod->can.state = CAN_STATE_STOPPED; > + > + /* bring the bus offline, stop receiving packets */ > + ret = janz_set_bus_state(mod, false); > + if (ret) { > + dev_err(mod->dev, "unable to set bus-off\n"); > + return ret; > + } > + > + return 0; > +} > + > +static int janz_xmit(struct sk_buff *skb, struct net_device *ndev) > +{ > + struct janz_ican3 *mod = netdev_priv(ndev); > + struct net_device_stats *stats = &ndev->stats; > + struct can_frame *cf = (struct can_frame *)skb->data; > + struct janz_fast_desc desc; > + void __iomem *desc_addr; > + unsigned long flags; > + > + spin_lock_irqsave(&mod->lock, flags); > + > + /* copy the control bits of the descriptor */ > + janz_set_page(mod, mod->fasttx_start + (mod->fasttx_num / 16)); > + desc_addr = mod->regs + ((mod->fasttx_num % 16) * sizeof(desc)); > + memset(&desc, 0, sizeof(desc)); > + memcpy_fromio(&desc, desc_addr, 1); > + > + /* check that we can actually transmit */ > + if (!(desc.control & DESC_VALID)) { > + dev_err(mod->dev, "%s: no buffers\n", __func__); > + stats->tx_dropped++; > + kfree_skb(skb); > + goto out_unlock; > + } > + > + /* convert the CAN frame into Janz format */ > + can_to_janz(mod, cf, &desc); > + > + /* > + * the programming manual says that you must set the IVALID bit, then > + * interrupt, then set the valid bit. Quite weird, but it seems to be > + * required for this to work > + */ > + desc.control |= DESC_IVALID; > + memcpy_toio(desc_addr, &desc, sizeof(desc)); > + janz_set_int(mod); > + desc.control ^= DESC_VALID; > + memcpy_toio(desc_addr, &desc, sizeof(desc)); > + > + /* update the next buffer pointer */ > + mod->fasttx_num = (desc.control & DESC_WRAP) ? 0 : (mod->fasttx_num + > 1); > + dev_dbg(mod->dev, "%s: update fast TX num -> %d\n", __func__, > mod->fasttx_num); > + > + /* update statistics */ > + stats->tx_packets++; > + stats->tx_bytes += cf->can_dlc; > + kfree_skb(skb); > + > +out_unlock: > + spin_unlock_irqrestore(&mod->lock, flags); > + return NETDEV_TX_OK; > +} > + > +static const struct net_device_ops janz_netdev_ops = { > + .ndo_open = janz_open, > + .ndo_stop = janz_stop, > + .ndo_start_xmit = janz_xmit, > +}; > + > +/*----------------------------------------------------------------------------*/ > +/* Low-level CAN Device > */ > +/*----------------------------------------------------------------------------*/ > + > +/* This structure was stolen from drivers/net/can/sja1000/sja1000.c */ > +static struct can_bittiming_const janz_bittiming_const = { > + .name = DRV_NAME, > + .tseg1_min = 1, > + .tseg1_max = 16, > + .tseg2_min = 1, > + .tseg2_max = 8, > + .sjw_max = 4, > + .brp_min = 1, > + .brp_max = 64, > + .brp_inc = 1, > +}; > + > +/* > + * This routine was stolen from drivers/net/can/sja1000/sja1000.c > + * > + * The bittiming register command for the ICAN3 just sets the bit timing > + * registers on the SJA1000 chip directly > + */ > +static int janz_set_bittiming(struct net_device *ndev) > +{ > + struct janz_ican3 *mod = netdev_priv(ndev); > + struct can_bittiming *bt = &mod->can.bittiming; > + struct janz_msg msg; > + u8 btr0, btr1; > + int ret; > + > + btr0 = ((bt->brp - 1) & 0x3f) | (((bt->sjw - 1) & 0x3) << 6); > + btr1 = ((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) | > + (((bt->phase_seg2 - 1) & 0x7) << 4); > + if (mod->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES) > + btr1 |= 0x80; > + > + dev_info(mod->dev, "setting BTR0=0x%02x BTR1=0x%02x\n", btr0, btr1); > + > + memset(&msg, 0, sizeof(msg)); > + msg.spec = MSG_CBTRREQ; > + msg.len = cpu_to_le16(4); > + msg.data[0] = 0x00; > + msg.data[1] = 0x00; > + msg.data[2] = btr0; > + msg.data[3] = btr1; > + > + ret = janz_send_msg(mod, &msg); > + if (ret) { > + dev_dbg(mod->dev, "unable to send CBTRREQ message\n"); > + return ret; > + } > + > + return 0; > +} > + > +static int janz_set_mode(struct net_device *ndev, enum can_mode mode) > +{ > + struct janz_ican3 *mod = netdev_priv(ndev); > + int ret; > + > + if (mode != CAN_MODE_START) > + return -ENOTSUPP; > + > + /* bring the bus online */ > + ret = janz_set_bus_state(mod, true); > + if (ret) { > + dev_err(mod->dev, "unable to set bus-on\n"); > + return ret; > + } How is bus-off recovery supposed to work? In general, if the card/hw recovery automatically, we use the following procedure: restart_ms == 0: the device should be *stopped* on bus-off allowing to user/app to restart it manually using this function. restart_ms > 0: the device is allowed to recover from bus-off automatically. Could that be implemented? > + /* start up the network device */ > + mod->can.state = CAN_STATE_ERROR_ACTIVE; > + > + if (netif_queue_stopped(ndev)) > + netif_wake_queue(ndev); > + > + return 0; > +} > + > +/*----------------------------------------------------------------------------*/ > +/* PCI Subsystem > */ > +/*----------------------------------------------------------------------------*/ > + > +static int __devinit ican3_probe(struct platform_device *pdev) > +{ > + struct janz_platform_data *pdata; > + struct net_device *ndev; > + struct janz_ican3 *mod; > + struct resource *res; > + struct device *dev; > + int ret; > + > + pdata = pdev->dev.platform_data; > + if (!pdata) > + return -ENXIO; > + > + dev_dbg(&pdev->dev, "probe: module number %d\n", pdata->modno); > + > + /* save the struct device for printing */ > + dev = &pdev->dev; > + > + /* allocate the CAN device and private data */ > + ndev = alloc_candev(sizeof(*mod), 0); > + if (!ndev) { > + dev_err(dev, "unable to allocate CANdev\n"); > + ret = -ENOMEM; > + goto out_return; > + } > + > + platform_set_drvdata(pdev, ndev); > + mod = netdev_priv(ndev); > + mod->ndev = ndev; > + mod->dev = &pdev->dev; > + mod->num = pdata->modno; > + INIT_WORK(&mod->work, janz_work); > + spin_lock_init(&mod->lock); > + > + /* initialize the software state */ > + > + /* the first unallocated page in the DPM is 9 */ > + mod->free_page = 9; Macro definition? > + > + ndev->netdev_ops = &janz_netdev_ops; > + ndev->flags |= IFF_ECHO; > + > + mod->can.bittiming_const = &janz_bittiming_const; > + mod->can.do_set_bittiming = janz_set_bittiming; > + mod->can.do_set_mode = janz_set_mode; > + > + SET_NETDEV_DEV(ndev, &pdev->dev); > + > + /* find our IRQ number */ > + mod->irq = platform_get_irq(pdev, 0); > + if (mod->irq < 0) { > + dev_err(dev, "IRQ line not found\n"); > + ret = -ENODEV; > + goto out_free_ndev; > + } > + > + ndev->irq = mod->irq; > + > + /* get access to the MODULbus registers for this module */ > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) { > + dev_err(dev, "MODULbus registers not found\n"); > + ret = -ENODEV; > + goto out_free_ndev; > + } > + > + mod->regs = ioremap(res->start, resource_size(res)); > + if (!mod->regs) { > + dev_err(dev, "MODULbus registers not ioremap\n"); > + ret = -ENOMEM; > + goto out_free_ndev; > + } > + > + /* get access to the control registers for this module */ > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + if (!res) { > + dev_err(dev, "CONTROL registers not found\n"); > + ret = -ENODEV; > + goto out_iounmap_regs; > + } > + > + mod->ctrl = ioremap(res->start, resource_size(res)); > + if (!mod->ctrl) { > + dev_err(dev, "CONTROL registers not ioremap\n"); > + ret = -ENOMEM; > + goto out_iounmap_regs; > + } > + > + /* disable our IRQ, then hookup the IRQ handler */ > + janz_disable_interrupts(mod); > + ret = request_irq(mod->irq, janz_irq, IRQF_SHARED, DRV_NAME, mod); > + if (ret) { > + dev_err(dev, "unable to request IRQ\n"); > + goto out_iounmap_ctrl; > + } Is this interrupt exclisively for CAN? ... or do you need a dispatcher in the MODULbus driver? > + /* reset and initialize the CAN controller into fast mode */ > + ret = janz_startup_module(mod); > + if (ret) { > + dev_err(dev, "%s: unable to start CANdev\n", __func__); > + goto out_free_irq; > + } > + > + /* register with the Linux CAN layer */ > + ret = register_candev(ndev); > + if (ret) { > + dev_err(dev, "%s: unable to register CANdev\n", __func__); > + goto out_free_irq; > + } > + > + dev_info(dev, "module %d: registered CAN device\n", pdata->modno); > + return 0; > + > +out_free_irq: > + janz_disable_interrupts(mod); > + free_irq(mod->irq, mod); > +out_iounmap_ctrl: > + iounmap(mod->ctrl); > +out_iounmap_regs: > + iounmap(mod->regs); > +out_free_ndev: > + free_netdev(ndev); > +out_return: > + return ret; > +} > + > +static int __devexit ican3_remove(struct platform_device *pdev) > +{ > + struct net_device *ndev = platform_get_drvdata(pdev); > + struct janz_ican3 *mod = netdev_priv(ndev); > + > + /* unregister the netdevice, stop interrupts */ > + unregister_netdev(ndev); unregister_candev? [snip] Also, dev_dbg() cleanup is required and try to reduce your one/two line functions. Wolfgang. _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
