Ira W. Snyder wrote:
> Hello all,
> 
> I posted last week asking about a driver for boards running on a PLX
> chip. It turns out that these are passive boards. I have been looking
> for a driver for the Janz CMOD-IO board CAN interfaces.
> 
> I finally found the datasheets and took the time to write a driver. This
> board is an intelligent CAN interface: it has onboard microprocessors to
> help process CAN traffic.
> 
> This is a very rough first try at a CAN driver. I'm sure it still has
> problems, and I would appreciate if you can take a look and help me add
> what is needed. I'm am not extremely knowledgeable about the CAN bus.
> 
> The things that are known to be wrong:
> 
> - bus-on / bus-off handling
> 
> I did this straight in the network device open()/stop() methods. I don't
> handle the condition where we get too many bus errors and the bus goes
> into bus-off state. What should I be doing here?
> 
> - state changes, bit timing, etc.
> 
> I'm not at all sure how this is supposed to work. Perhaps someone that
> knows CAN bus better that I do can help.
> 
> - CAN bus termination
> 
> This board supports optionally terminating the CAN bus. In order to test
> this driver, I connected both CAN modules on a single board together. To
> get this to work, I needed to turn on termination on both modules.
> 
> Is this wrong? Is there a way to enable/disable termination via the "ip"
> tool?
> 
> - module probing
> 
> This board is really a MODULbus carrier board, into which plugs 4
> daughterboards. These can be CAN modules, or others. On my board, I have
> 2x CAN modules and 1x TTL GPIO module. I need support for both of these
> for my application. For the time being, I *did not* add support for the
> TTL module. That will come once the CAN part is finished.
> 
> Also, there is no way I am aware of to determine what type of board is
> plugged into which MODULbus slot on the carrier board. My CAN modules
> support identification, but the TTL module does not.
> 
> I hard-coded the module layout into the driver itself. This is
> sufficient for my purposes, but probably is not sufficient for mainline
> Linux :'(. Any ideas or suggestions?
> 
> 
> Any review will be much appreciated!

Will do a quick review...

You are using way to much dev_dbg() messages. OK for debugging, but you
should remove most of them for the final version.

> --------------------- cut here ----------------------
> 
> 
> /*
>  * Janz PCI CAN 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.
>  */
> 
> #define DEBUG 1

You can enable DEBUG with the Kconfig option CONFIG_CAN_DEBUG_DEVICES.


> #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/netdevice.h>
> #include <linux/can.h>
> #include <linux/can/dev.h>
> #include <linux/can/error.h>
> 
> static const char drv_name[] = "janz";
> 
> /* PLX bridge chip onboard registers */
> #define JANZ_OB_INT_STAT      0x1             /* read  access */
> #define JANZ_OB_INT_DISABLE   0x1             /* write access */
> #define JANZ_OB_MBUS_NUM      0x3             /* read  access */
> #define JANZ_OB_INT_ENABLE    0x3             /* write access */
> #define JANZ_OB_RESET_ASSERT  0x5             /* write access */
> #define JANZ_OB_RESET_DEASSERT        0x7             /* write access */
> #define JANZ_OB_EEP           0x9             /* rw access */
> #define JANZ_OB_ENID          0xb             /* write access */
> /* 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 "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
> 
> /* Maximum number of buffers on a CMOD-IO carrier board */
> #define JANZ_MAX_MODULES 4
> 
> /*
>  * This is a terrible solution to an ugly problem
>  *
>  * The Janz CMOD-IO carrier board supports both MODULbus and MODULbus+
>  * modules, however, only MODULbus+ modules are required to have a serial
>  * EEPROM onboard for automatic identification.
>  *
>  * This means that it is impossible to detect which modules are actually
>  * present on any one CMOD-IO board.
>  *
>  * I chose the solution that the user will have to change the defines below
>  * to match their actual hardware. There isn't really anything else I can do.
>  * This is sufficient for my purposes: all boards I own have the same
>  * configuration.
>  *
>  * Janz's character driver does this setup via string module parameters,
>  * which are fairly ugly to parse inside the kernel.
>  */
> 
> /* Module Types */
> #define JANZ_MODULE_NONE      0
> #define JANZ_MODULE_TTL               1
> #define JANZ_MODULE_ICAN3     2
> 
> /* Modules Present */
> static const int janz_modules_present[JANZ_MAX_MODULES] = {
>       JANZ_MODULE_ICAN3,
>       JANZ_MODULE_ICAN3,
>       JANZ_MODULE_NONE,
>       JANZ_MODULE_TTL,
> };

This could be specified via module parameter, e.g.

 insmod janz_pci.ko modues=ican3,ican3,,ttl

> struct janz_module {
> 
>       /* must be the first member */
>       struct can_priv can;
> 
>       /* CAN network device */
>       struct net_device *ndev;
>       struct napi_struct napi;
> 
>       /* parent PCI device */
>       struct janz_device *parent;
> 
>       /* module number */
>       unsigned int num;
> 
>       /* base address of registers */
>       void __iomem *regs;
> 
>       /* 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_device {
>       struct device *dev;
>       struct pci_dev *pdev;
> 
>       void __iomem *control_regs;
>       void __iomem *modulbus_regs;
>       void __iomem *onboard_regs;
> 
>       /* hex switch position */
>       u8 hex;

Not a good name, I think.

> 
>       struct janz_module *modules[JANZ_MAX_MODULES];
> };
> 
> 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];
> };
> 
> /*----------------------------------------------------------------------------*/
> /* Dual Ported Memory (DPM) Access                                            
> */
> /*----------------------------------------------------------------------------*/

This comment sytel is not OK according to the coding style.

> /*
>  * The DPM has a fixed size of 64K byte, organized into 256x 256 byte pages
>  *
>  * You have a 0x200 byte view into DPM memory, split into two windows:
>  *
>  * 0x0000 - 0x00ff: DPM memory page
>  * 0x0100 - 0x01ff: DPM control registers
>  *
>  * You can switch the page shown in the first window by using the control
>  * registers. They also have other uses, not described here.
>  */
> 
> static u8 janz_dpm_read8(struct janz_module *mod, unsigned int addr)
> {
>       /* the DPM pages are only 256 bytes long */
>       BUG_ON(addr >= DPM_PAGE_SIZE);
>       return ioread8(mod->regs + addr);
> }
> 
> static void janz_dpm_write8(struct janz_module *mod, unsigned int addr, u8 
> val)
> {
>       /* the DPM pages are only 256 bytes long */
>       BUG_ON(addr >= DPM_PAGE_SIZE);
>       iowrite8(val, mod->regs + addr);
> }
> 
> /*----------------------------------------------------------------------------*/
> /* DPM Register Access                                                        
> */
> /*----------------------------------------------------------------------------*/
> 
> /* write to the window basic address register */
> static void janz_set_page(struct janz_module *mod, unsigned int page)
> {
>       /* the DPM only has 256 pages */
>       BUG_ON(page >= 256);
>       iowrite8(page, mod->regs + 0x100);
> }
> 
> /* clear a MODULbus interrupt */
> static void janz_clr_int(struct janz_module *mod)
> {
>       ioread8(mod->regs + 0x102);
> }
>
> /* generate a MODULbus interrupt */
> static void janz_set_int(struct janz_module *mod)
> {
>       iowrite8(0x01, mod->regs + 0x102);
> }
> 
> #if 0
> /* generate a hardware reset on the ICAN3 */
> static void janz_set_hwreset(struct janz_module *mod)
> {
>       iowrite8(0x01, mod->regs + 0x104);
> }
> 
> /* generate a TPU interrupt on the ICAN3 */
> static void janz_set_tpuint(struct janz_module *mod)
> {
>       iowrite8(0x01, mod->regs + 0x106);
> }
> #endif

I do not find these single line function really useful. Use a macro
definitions, inline functions or even better a structure to define your
register layout, e.g.:

        iwrite8(0x01, &mod->regs->tpuint);

> /*----------------------------------------------------------------------------*/
> /* Onboard Registers                                                          
> */
> /*----------------------------------------------------------------------------*/
> 
> static void janz_disable_interrupts(struct janz_module *mod)
> {
>       struct janz_device *priv = mod->parent;

Here and in other similar cases you should insert an empty line.

>       iowrite8(1 << mod->num, priv->onboard_regs + JANZ_OB_INT_DISABLE);
> }
> 
> static void janz_enable_interrupts(struct janz_module *mod)
> {
>       struct janz_device *priv = mod->parent;
>       iowrite8(1 << mod->num, priv->onboard_regs + JANZ_OB_INT_ENABLE);
> }
> 
> #if 0
> 
> static inline void eep(struct janz_device *priv, u8 eck, u8 edio)
> {
>       u8 d = ((eck << 1) | (edio & 1)) ^ 0x03;
>       iowrite8(d, priv->onboard_regs + JANZ_OB_EEP);
> }
> 
> static inline void selhi(struct janz_device *priv, unsigned int mod)
> {
>       iowrite8(1 << mod, priv->onboard_regs + JANZ_OB_ENID);
> }
> 
> static inline void selnone(struct janz_device *priv)
> {
>       iowrite8(0x00, priv->onboard_regs + JANZ_OB_ENID);
> }

Ditto

> /*
>  * Read a MODULbus+ EEPROM on a module
>  *
>  * This requires generating waveforms in software, which is pretty
>  * uncomfortable and unfriendly. For reasons stated above, this
>  * is a useless feature anyway, since it doesn't work on non-MODULbus+
>  * modules
>  */
> static void janz_read_module_id(struct janz_device *priv, unsigned int mod)
> {
>       int i, cell;
>       u16 val;
>       u8 bit;
> 
>       /

That made me think that it does not compile yet.

>       /*
>        * This requires that you generate EEPROM waveforms in software, making
>        * it pretty much impractical. You need to use the ENID register to
>        * enable the module id lines, and then use the EEP register to
>        * generate the waveforms for the EEPROM. Painful.
>        */
> 
>       /* put the bus in an idle state */
>       selnone(priv);
>       eep(priv, 0, 1);
> 
>       /* Chip Select the correct module */
>       selhi(priv, mod);
> 
>       /* generate the start bit (data == 1) */
>       eep(priv, 1, 1);
>       eep(priv, 0, 1);
> 
>       /* opcode 10 */
>       eep(priv, 1, 1);
>       eep(priv, 0, 1);
> 
>       eep(priv, 1, 0);
>       eep(priv, 0, 0);
> 
>       /* 6 address bits, all 0 */
>       eep(priv, 1, 0);
>       eep(priv, 0, 0);
> 
>       eep(priv, 1, 0);
>       eep(priv, 0, 0);
> 
>       eep(priv, 1, 0);
>       eep(priv, 0, 0);
> 
>       eep(priv, 1, 0);
>       eep(priv, 0, 0);
> 
>       eep(priv, 1, 0);
>       eep(priv, 0, 0);
> 
>       eep(priv, 1, 0);
>       eep(priv, 0, 0);

This code could be shortend a lot by using some helper function and for
loops.

>       /* tri-state the data line */
>       eep(priv, 0, 1);
> 
>       for (cell = 0; cell < 64; cell++) {
> 
>               /* read 16 bits at a time */
>               val = 0;
>               for (i = 0; i < 16; i++) {
>                       eep(priv, 1, 1);
>                       eep(priv, 0, 1);
>                       bit = (ioread8(priv->onboard_regs + JANZ_OB_EEP) & 
> 0x01) ^ 0x01;
>                       val |= (bit << (15 - i));
>               }
> 
>               dev_dbg(priv->dev, "%s: DATA16 CELL %d -> %.4x\n", __func__, 
> cell, val);
>       }
> 
>       /* de-assert chip select, reset the data and clock lines */
>       selnone(priv);
>       eep(priv, 1, 1);
> }
> #endif
> 
> /*----------------------------------------------------------------------------*/
> /* Janz "old-style" host interface                                            
> */
> /*----------------------------------------------------------------------------*/
> 
> /*
>  * Get the MSYNC bits from the "old-style" interface control
>  * registers
>  */
> static void janz_get_msync(struct janz_module *mod, u8 *locl, u8 *peer)
> {
>       janz_set_page(mod, 0);
>       *peer = janz_dpm_read8(mod, MSYNC_PEER);
>       *locl = janz_dpm_read8(mod, MSYNC_LOCL);

I fould the following more readable and transparent:

        *perr = ioread8(&mod->dpm_regs->mysnc_peer);

> 
> /*
>  * Recieve a message from the Janz "old-style" firmware interface
>  *
>  * @priv: device private data
>  * @mod: MODULbus module number
>  * @msg: message buffer storage
>  * @return: 0 on success, -ENOMEM when no message exists
>  */
> static int janz_old_recv_msg(struct janz_module *mod, struct janz_msg *msg)
> {
>       struct janz_device *priv = mod->parent;
>       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(priv->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);
>       janz_dpm_write8(mod, MSYNC_LOCL, locl);
>       return 0;
> }
> 
> /*
>  * Send a message through the "old-style" firmware interface
>  *
>  * @priv: device private data
>  * @mod: MODULbus module number
>  * @msg: message buffer storage
>  * @return: 0 on success, -ENOMEM when no free space exists
>  */

Either use doc books style documentation consistantly or drop it.

> static int janz_old_send_msg(struct janz_module *mod, struct janz_msg *msg)
> {
>       struct janz_device *priv = mod->parent;
>       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(priv->dev, "no mbox for writing\n");
>               return -ENOMEM;
>       }
> 
>       /* 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);
>       janz_dpm_write8(mod, MSYNC_LOCL, locl);
>       return 0;
> }
> 
> /*----------------------------------------------------------------------------*/
> /* Janz "new-style" Host Interface Setup                                      
> */
> /*----------------------------------------------------------------------------*/
> 
> static void janz_init_new_host_interface(struct janz_module *mod)
> {
>       struct janz_device *priv = mod->parent;
>       struct janz_new_desc desc;
>       unsigned long flags;
>       void __iomem *dst;
>       int i;
> 
>       spin_lock_irqsave(&mod->lock, flags);
> 
>       dev_dbg(priv->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(priv->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_module *mod)
> {
>       struct janz_device *priv = mod->parent;
>       struct janz_fast_desc desc;
>       unsigned long flags;
>       unsigned int addr;
>       void __iomem *dst;
>       int i;
> 
>       spin_lock_irqsave(&mod->lock, flags);
>       dev_dbg(priv->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) {
>                       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(priv->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_module *mod, struct janz_msg *msg)
> {
>       struct janz_new_desc desc;
>       struct janz_device *priv = mod->parent;
>       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(priv->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(priv->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_module *mod, struct janz_msg *msg)
> {
>       struct janz_new_desc desc;
>       struct janz_device *priv = mod->parent;
>       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(priv->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(priv->dev, "%s: update RX num -> %d\n", __func__, mod->rx_num);
> 
>       return 0;
> }
> 
> /*----------------------------------------------------------------------------*/
> /* Message Send / Recv Helpers                                                
> */
> /*----------------------------------------------------------------------------*/
> 
> static int janz_send_msg(struct janz_module *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_module *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_module *mod)
> {
>       struct janz_device *priv = mod->parent;
>       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(priv->dev, "unable to send CONNECT message\n");
>               return ret;
>       }
> 
>       return 0;
> }
> 
> static int janz_msg_disconnect(struct janz_module *mod)
> {
>       struct janz_device *priv = mod->parent;
>       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(priv->dev, "unable to send DISCONNECT message\n");
>               return ret;
>       }
> 
>       return 0;
> }

I see duplicated code above.

> static int janz_msg_newhostif(struct janz_module *mod)
> {
>       struct janz_device *priv = mod->parent;
>       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(priv->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_module *mod)
> {
>       struct janz_device *priv = mod->parent;
>       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);
> 
>       /* 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(priv->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_module *mod, bool accept)
> {
>       struct janz_device *priv = mod->parent;
>       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);
>       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(priv->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(priv->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_module *mod, bool on)
> {
>       struct janz_device *priv = mod->parent;
>       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(priv->dev, "%s: %s request: spec %.2x\n", __func__, on ? "on" : 
> "off", msg.spec);
>       ret = janz_send_msg(mod, &msg);
>       if (ret) {
>               dev_dbg(priv->dev, "unable to send CONREQ/COFFREQ message\n");
>               return ret;
>       }
> 
>       return 0;
> }

Other controllers use the name reset/init mode and normal mode-

> 
> static int janz_set_termination(struct janz_module *mod, bool on)
> {
>       struct janz_device *priv = mod->parent;
>       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(priv->dev, "unable to send HWCONF message\n");
>               return ret;
>       }
> 
>       return 0;
> }
> 
> /*----------------------------------------------------------------------------*/
> /* Interrupt Handling                                                         
> */
> /*----------------------------------------------------------------------------*/
> 
> static void janz_handle_idvers(struct janz_module *mod, struct janz_msg *msg)
> {
>       struct janz_device *priv = mod->parent;
> 
>       dev_dbg(priv->dev, "%s: %s\n", __func__, msg->data);
> }
> 
> static void janz_handle_msglost(struct janz_module *mod, struct janz_msg *msg)
> {
>       struct janz_device *priv = mod->parent;
>       char *queue;
> 
>       if (msg->spec == MSG_MSGLOST)
>               queue = "new";
>       else
>               queue = "fast";
> 
>       dev_dbg(priv->dev, "%s: %s hostif: %d messages lost\n",
>                          __func__, queue, msg->data[0]);
> }
> 
> static void janz_handle_cevtind(struct janz_module *mod, struct janz_msg *msg)
> {
>       struct janz_device *priv = mod->parent;
>       char *s;
>       int i;
> 
>       dev_dbg(priv->dev, "%s: message len: %d\n", __func__, 
> le16_to_cpu(msg->len));
>       for (i = 0; i < le16_to_cpu(msg->len); i++)
>               dev_dbg(priv->dev, "%s: data[%.2d] -> %.2x\n", __func__, i, 
> msg->data[i]);
> 
>       switch (msg->data[0]) {
>       case 0x01:
>               s = "error interrupt occurred";
>               break;
>       case 0x02:
>               s = "overrun interrupt occurred";
>               break;
>       case 0x04:
>               s = "interrupts lost";
>               break;
>       case 0x08:
>               s = "send queue full";
>               break;
>       case 0x10:
>               s = "CANbus bus-error";
>               break;
>       default:
>               s = "unknown error";
>               break;
>       }

Does the Firmware provide more information on the cause of the error.


>       dev_dbg(priv->dev, "%s: %s\n", __func__, s);
> }
> 
> static void janz_handle_unknown(struct janz_module *mod, struct janz_msg *msg)
> {
>       struct janz_device *priv = mod->parent;
>       u16 len;
>       int i;
> 
>       len = le16_to_cpu(msg->len);
>       dev_dbg(priv->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(priv->dev, "msg->data[%.2d] -> 0x%.2x\n", i, 
> msg->data[i]);
> }
> 
> static void janz_handle_message(struct janz_module *mod, struct janz_msg *msg)
> {
>       struct janz_device *priv = mod->parent;
> 
>       dev_dbg(priv->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;
>       case MSG_CEVTIND:
>               janz_handle_cevtind(mod, msg);
>               break;
>       default:
>               janz_handle_unknown(mod, msg);
>               break;
>       }
> }
> 
> static void janz_work(struct work_struct *work)
> {
>       struct janz_module *mod = container_of(work, struct janz_module, work);
>       struct janz_device *priv = mod->parent;
>       unsigned int handled = 0;
>       struct janz_msg msg;
>       int ret;
> 
>       dev_dbg(priv->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(priv->dev, "%s: no more messages\n", __func__);
>                       break;
>               }
> 
>               janz_handle_message(mod, &msg);
>               handled++;
>       }
> 
>       dev_dbg(priv->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 NAPI poller or
>  * work function to go ahead and 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_module *mod)
> {
>       struct janz_device *priv = mod->parent;
>       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(priv->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(priv->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(priv->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);
> 
>               /*
>                * update the interrupt handler's position and schedule
>                * the NAPI poller to run at some point in the future
>                */
>               mod->fastrx_int = (control & DESC_WRAP) ? 0 : (mod->fastrx_int 
> + 1);
>               napi_schedule(&mod->napi);
>       }
> 
> out_unlock:
>       janz_clr_int(mod);
>       spin_unlock_irqrestore(&mod->lock, flags);
> }
> 
> static irqreturn_t janz_irq(int irq, void *dev_id)
> {
>       struct janz_device *priv = dev_id;
>       struct janz_module *mod;
>       u8 stat;
>       int i;
> 
>       /*
>        * The interrupt status register on this device reports interrupts
>        * as zeroes instead of using ones like most other devices
>        */
>       stat = ioread8(priv->onboard_regs + JANZ_OB_INT_STAT) & 0x0f;
>       dev_dbg(priv->dev, "IRQ: enter stat 0x%.2x\n", stat);
> 
>       if (stat == 0x0f) {
>               dev_dbg(priv->dev, "IRQ: none pending\n");
>               return IRQ_NONE;
>       }
> 
>       /* Figure out which module interrupted, and run its work function */
>       for (i = 0; i < JANZ_MAX_MODULES; i++) {
>               mod = priv->modules[i];
>               if ((stat & (1 << i)) == 0x00) {
>                       dev_dbg(priv->dev, "IRQ: module %d\n", i);
>                       janz_handle_interrupt(mod);
>               }
>       }
> 
>       stat = ioread8(priv->onboard_regs + JANZ_OB_INT_STAT) & 0x0f;
>       dev_dbg(priv->dev, "IRQ: exit stat 0x%.2x\n", stat);
> 
>       return IRQ_HANDLED;
> }
> 
> /*----------------------------------------------------------------------------*/
> /* TEST CODE                                                                  
> */
> /*----------------------------------------------------------------------------*/
> 
> /*
>  * Initialize the data structure for a module
>  */
> static void janz_init_module(struct janz_module *mod)
> {
>       mod->iftype = 0;
>       mod->rx_int = 0;
>       mod->rx_num = 0;
>       mod->tx_num = 0;
> 
>       mod->fastrx_start = 0;
>       mod->fastrx_int   = 0;
>       mod->fastrx_num   = 0;
>       mod->fasttx_start = 0;
>       mod->fasttx_num   = 0;

There is no need to set these values to zero.

>       /* the first unallocated page in the DPM is 9 */
>       mod->free_page = 9;
> }
> 
> /*
>  * Reset an ICAN module to its power-on state
>  *
>  * CONTEXT: no network device registered
>  * LOCKING: napi + work disabled
>  */
> static int janz_reset_module(struct janz_module *mod)
> {
>       struct janz_device *priv = mod->parent;
>       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();
> 
>       /* re-initialize the software state */
>       janz_init_module(mod);
> 
>       janz_set_page(mod, 0);
>       runold = janz_dpm_read8(mod, TARGET_RUNNING);
> 
>       /* reset the module */
>       iowrite8(val, priv->onboard_regs + JANZ_OB_RESET_ASSERT);
>       iowrite8(val, priv->onboard_regs + JANZ_OB_RESET_DEASSERT);
> 
>       /* wait until the module has finished resetting and is running */
>       start = jiffies;
>       do {
>               janz_set_page(mod, 0);
>               runnew = janz_dpm_read8(mod, TARGET_RUNNING);
>               if (runnew == (runold ^ 0xff)) {
>                       dev_dbg(priv->dev, "%s: success\n", __func__);
>                       return 0;
>               }
> 
>               dev_dbg(priv->dev, "%s: msleep(10)\n", __func__);
>               msleep(10);
>       } while (time_before(jiffies, start + HZ / 4));
> 
>       dev_dbg(priv->dev, "%s: timed out\n", __func__);
>       return -ETIMEDOUT;
> }
> 
> static void janz_shutdown_module(struct janz_module *mod)
> {
>       struct janz_device *priv = mod->parent;
>       int ret;
> 
>       dev_dbg(priv->dev, "%s: disconnect and reset module\n", __func__);
>       janz_msg_disconnect(mod);
>       ret = janz_reset_module(mod);
>       if (ret)
>               dev_err(priv->dev, "unable to reset module\n");
> }
> 
> /*
>  * Startup an ICAN module, bringing it into fast mode
>  */
> static int janz_startup_module(struct janz_module *mod)
> {
>       struct janz_device *priv = mod->parent;
>       int ret;
> 
>       dev_dbg(priv->dev, "%s: reset module\n", __func__);
>       ret = janz_reset_module(mod);
>       if (ret) {
>               dev_err(priv->dev, "unable to reset module\n");
>               return ret;
>       }
> 
>       /* re-enable interrupts so we can send messages */
>       janz_enable_interrupts(mod);
> 
>       dev_dbg(priv->dev, "%s: connect and switch to new if\n", __func__);
>       ret = janz_msg_connect(mod);
>       if (ret) {
>               dev_err(priv->dev, "unable to connect to module\n");
>               return ret;
>       }
> 
>       janz_init_new_host_interface(mod);
>       ret = janz_msg_newhostif(mod);
>       if (ret) {
>               dev_err(priv->dev, "unable to switch to new-style interface\n");
>               return ret;
>       }
> 
>       dev_dbg(priv->dev, "%s: enable termination\n", __func__);
>       ret = janz_set_termination(mod, true);
>       if (ret) {
>               dev_err(priv->dev, "unable to enable termination\n");
>               return ret;
>       }
> 
>       dev_dbg(priv->dev, "%s: start fast host if\n", __func__);
>       janz_init_fast_host_interface(mod);
>       ret = janz_msg_fasthostif(mod);
>       if (ret) {
>               dev_err(priv->dev, "unable to switch to fast host interface\n");
>               return ret;
>       }
> 
>       dev_dbg(priv->dev, "%s: set filter to accept everything\n", __func__);
>       ret = janz_set_id_filter(mod, true);
>       if (ret) {
>               dev_err(priv->dev, "unable to set acceptance filter\n");
>               return ret;
>       }
> 
>       return 0;
> }
> 
> /*----------------------------------------------------------------------------*/
> /* Janz to CAN Frame Conversion                                               
> */
> /*----------------------------------------------------------------------------*/
> 
> static void janz_to_can(struct janz_module *mod, struct janz_fast_desc *desc,
>                       struct can_frame *cf)

Better name?

> {
>       struct janz_device *priv = mod->parent;
> 
>       if ((desc->command & 0x0f) == 0) {
>               dev_dbg(priv->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(priv->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_module *mod, struct can_frame *cf,
>                       struct janz_fast_desc *desc)

Ditto.

> {
>       struct janz_device *priv = mod->parent;
> 
>       /* 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(priv->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(priv->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));
> }
> 
> /*----------------------------------------------------------------------------*/
> /* CAN Network Device                                                         
> */
> /*----------------------------------------------------------------------------*/
> 
> static int janz_open(struct net_device *ndev)
> {
>       struct janz_module *mod = netdev_priv(ndev);
>       struct janz_device *priv = mod->parent;
>       int ret;
> 
>       dev_dbg(priv->dev, "%s: called\n", __func__);
> 
>       /* bring the bus online */
>       ret = janz_set_bus_state(mod, true);
>       if (ret) {
>               dev_err(priv->dev, "unable to set bus-on\n");
>               return ret;
>       }
> 
>       /* start up the network device */
>       napi_enable(&mod->napi);
>       netif_start_queue(ndev);
> 
>       return 0;
> }
> 
> static int janz_stop(struct net_device *ndev)
> {
>       struct janz_module *mod = netdev_priv(ndev);
>       struct janz_device *priv = mod->parent;
>       int ret;
> 
>       dev_dbg(priv->dev, "%s: called\n", __func__);
> 
>       /* stop the network device xmit routine */
>       netif_stop_queue(ndev);
> 
>       /* bring the bus offline */
>       ret = janz_set_bus_state(mod, false);
>       if (ret) {
>               dev_err(priv->dev, "unable to set bus-off\n");
>               return ret;
>       }
> 
>       /* stop receiving packets */
>       napi_disable(&mod->napi);
> 
>       return 0;
> }
> 
> static int janz_xmit(struct sk_buff *skb, struct net_device *ndev)
> {
>       struct janz_module *mod = netdev_priv(ndev);
>       struct janz_device *priv = mod->parent;
>       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(priv->dev, "%s: no buffers\n", __func__);
>               stats->tx_dropped++;
>               kfree_skb(skb);
>               goto out_unlock;

Not sure if you want to drop the skb. You could also return
NETDEV_TX_BUSY without freeing the packet.

>       }
> 
>       /* 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(priv->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 int janz_napi(struct napi_struct *napi, int quota)
> {
>       struct net_device *ndev = napi->dev;
>       struct net_device_stats *stats = &ndev->stats;
>       struct janz_module *mod = netdev_priv(ndev);
>       struct janz_device *priv = mod->parent;
>       struct janz_fast_desc desc;
>       void __iomem *desc_addr;
>       struct can_frame *cf;
>       struct sk_buff *skb;
>       unsigned long flags;
>       int handled = 0;
> 
>       dev_dbg(priv->dev, "%s: modno %d called quota %d\n", __func__, 
> mod->num, quota);
>       spin_lock_irqsave(&mod->lock, flags);
> 
>       while (handled < quota) {
> 
>               /* 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(priv->dev, "%s: no more frames\n", __func__);
>                       break;
>               }
> 
>               /* allocate an skb */
>               skb = alloc_can_skb(ndev, &cf);
>               if (unlikely(skb == NULL)) {
>                       dev_dbg(priv->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_receive_skb(skb);
>               stats->rx_packets++;
>               stats->rx_bytes += cf->can_dlc;
>               handled++;
> 
> 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(priv->dev, "%s: update fast RX num -> %d\n", __func__, 
> mod->fastrx_num);
>       }
> 
>       if (handled < quota)
>               napi_complete(napi);
> 
>       dev_dbg(priv->dev, "%s: modno %d handled %d CAN frames\n", __func__, 
> mod->num, handled);
>       spin_unlock_irqrestore(&mod->lock, flags);
>       return handled;
> }
> 
> 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                                                       
> */
> /*----------------------------------------------------------------------------*/
> 
> struct janz_baud_entry {
>       u32 rate;
>       u8 btr0;
>       u8 btr1;
> };
> 
> static struct janz_baud_entry janz_baud_table[] = {
> #if 0
>       {1000000, 0x00, 0x23}, /* early sampling */
> #endif
>       {1000000, 0x00, 0x14}, /* late sampling */
>       {500000,  0x00, 0x1c},
>       {250000,  0x01, 0x1c},
>       {125000,  0x03, 0x1c},
>       {100000,  0xc7, 0x34},
>       {50000,   0xcf, 0x34},
>       {20000,   0xcf, 0x7f},
> };

You could use the calculated bittiming paramters for the SJA1000. Check
sja1000.c on how to do it.

> static int janz_set_bittiming(struct net_device *ndev)
> {
>       struct janz_module *mod = netdev_priv(ndev);
>       struct janz_device *priv = mod->parent;
>       struct janz_baud_entry *entry = NULL;
>       struct janz_msg msg;
>       int i, ret;
> 
>       dev_dbg(priv->dev, "%s: called bitrate %d\n",
>                          __func__, mod->can.bittiming.bitrate);
> 
>       for (i = 0; i < ARRAY_SIZE(janz_baud_table); i++) {
>               if (mod->can.bittiming.bitrate == janz_baud_table[i].rate) {
>                       entry = &janz_baud_table[i];
>                       break;
>               }
>       }
> 
>       if (!entry) {
>               dev_dbg(priv->dev, "%s: no matching bittiming\n", __func__);
>               return -EINVAL;
>       }
> 
>       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] = entry->btr0;
>       msg.data[3] = entry->btr1;
> 
>       ret = janz_send_msg(mod, &msg);
>       if (ret) {
>               dev_dbg(priv->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_module *mod = netdev_priv(ndev);
>       struct janz_device *priv = mod->parent;
> 

This is called to restart to device after bus-off.


>       dev_dbg(priv->dev, "%s: called mode %d\n", __func__, mode);
>       return 0;
> }
> 
> /*----------------------------------------------------------------------------*/
> /* PCI Subsystem                                                              
> */
> /*----------------------------------------------------------------------------*/
> 
> static void janz_free_one(struct janz_device *priv, unsigned int modno)
> {
>       struct net_device *ndev;
>       struct janz_module *mod;
> 
>       mod = priv->modules[modno];
>       priv->modules[modno] = NULL;
>       ndev = mod->ndev;
> 
>       unregister_netdev(ndev);
>       free_netdev(ndev);
> 
>       janz_shutdown_module(mod);
> }
> 
> /* setup a single CAN module on the MODULbus carrier board */
> static int janz_alloc_one(struct janz_device *priv, unsigned int modno)
> {
>       struct net_device *ndev;
>       struct janz_module *mod;
>       int ret;
> 
>       ndev = alloc_candev(sizeof(*mod), 16);
>       if (!ndev) {
>               dev_err(priv->dev, "%s: unable to allocate CANdev\n", __func__);
>               ret = -ENOMEM;
>               goto out_return;
>       }
> 
>       mod = netdev_priv(ndev);
>       priv->modules[modno] = mod;
> 
>       mod->ndev = ndev;
>       mod->parent = priv;
>       mod->num = modno;
>       mod->regs = priv->modulbus_regs + (0x200 * modno);
>       INIT_WORK(&mod->work, janz_work);
>       spin_lock_init(&mod->lock);
> 
>       /* initialize the software state */
>       janz_init_module(mod);
> 
>       ndev->netdev_ops = &janz_netdev_ops;
>       ndev->irq = priv->pdev->irq;
>       ndev->flags |= IFF_ECHO;
> 
>       mod->can.do_set_bittiming = &janz_set_bittiming;
>       mod->can.do_set_mode = &janz_set_mode;

Remove the two "&" above.

>       netif_napi_add(ndev, &mod->napi, janz_napi, 16);
>       SET_NETDEV_DEV(ndev, &priv->pdev->dev);
> 
>       /* reset and initialize the CAN controller into fast mode */
>       ret = janz_startup_module(mod);
>       if (ret) {
>               dev_err(priv->dev, "%s: unable to start CANdev\n", __func__);
>               goto out_free_ndev;
>       }
> 
>       /* register with the Linux CAN layer */
>       ret = register_candev(ndev);
>       if (ret) {
>               dev_err(priv->dev, "%s: unable to register CANdev\n", __func__);
>               goto out_free_ndev;
>       }
> 
>       dev_info(priv->dev, "module %d: registered CAN device\n", modno);
>       return 0;
> 
> out_free_ndev:
>       priv->modules[modno] = NULL;
>       free_netdev(ndev);
> out_return:
>       return ret;
> }
> 
> static int janz_probe_modules(struct janz_device *priv)
> {
>       struct janz_module *mod;
>       int i;
> 
>       for (i = 0; i < ARRAY_SIZE(janz_modules_present); i++) {
>               mod = priv->modules[i];
>               switch (janz_modules_present[i]) {
>               case JANZ_MODULE_NONE:
>                       dev_dbg(priv->dev, "modno %d: NONE\n", i);
>                       break;
>               case JANZ_MODULE_ICAN3:
>                       dev_dbg(priv->dev, "modno %d: ICAN3\n", i);
>                       janz_alloc_one(priv, i);
>                       break;
>               case JANZ_MODULE_TTL:
>                       dev_dbg(priv->dev, "modno %d: TTL\n", i);
>                       break;
>               default:
>                       dev_err(priv->dev, "modno %d: UNKNOWN\n", i);
>                       break;
>               }
>       }
> 
>       return 0;
> }
> 
> static void janz_remove_modules(struct janz_device *priv)
> {
>       struct janz_module *mod;
>       int i;
> 
>       for (i = 0; i < ARRAY_SIZE(janz_modules_present); i++) {
>               mod = priv->modules[i];
>               if (mod == NULL)
>                       continue;
> 
>               switch (janz_modules_present[i]) {
>               case JANZ_MODULE_NONE:
>                       dev_dbg(priv->dev, "modno %d: NONE\n", i);
>                       break;
>               case JANZ_MODULE_ICAN3:
>                       dev_dbg(priv->dev, "modno %d: ICAN3\n", i);
>                       janz_free_one(priv, i);
>                       break;
>               case JANZ_MODULE_TTL:
>                       dev_dbg(priv->dev, "modno %d: TTL\n", i);
>                       break;
>               default:
>                       dev_err(priv->dev, "modno %d: UNKNOWN\n", i);
>                       break;
>               }
>       }
> }
> 
> static int janz_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> {
>       struct janz_device *priv;
>       int ret;
> 
>       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>       if (!priv) {
>               dev_err(&dev->dev, "unable to allocate private data\n");
>               ret = -ENOMEM;
>               goto out_return;
>       }
> 
>       pci_set_drvdata(dev, priv);
>       priv->dev = &dev->dev;
>       priv->pdev = dev;
> 
>       /* Hardware Initialization */
>       ret = pci_enable_device(dev);
>       if (ret) {
>               dev_err(&dev->dev, "unable to enable device\n");
>               goto out_free_priv;
>       }
> 
>       pci_set_master(dev);
>       ret = pci_request_regions(dev, drv_name);
>       if (ret) {
>               dev_err(&dev->dev, "unable to request regions\n");
>               goto out_pci_disable_device;
>       }
> 
>       /* Local Configuration Registers */
>       priv->control_regs = pci_ioremap_bar(dev, 0);
>       if (!priv->control_regs) {
>               dev_err(&dev->dev, "unable to remap control regs\n");
>               ret = -ENOMEM;
>               goto out_pci_release_regions;
>       }
> 
>       /* MODULbus memory space, big endian access */
>       priv->modulbus_regs = pci_ioremap_bar(dev, 3);
>       if (!priv->modulbus_regs) {
>               dev_err(&dev->dev, "unable to remap MODULbus regs\n");
>               ret = -ENOMEM;
>               goto out_unmap_control_regs;
>       }
> 
>       /* Onboard configuration registers */
>       priv->onboard_regs = pci_ioremap_bar(dev, 4);
>       if (!priv->onboard_regs) {
>               dev_err(&dev->dev, "unable to remap onboard regs\n");
>               ret = -ENOMEM;
>               goto out_unmap_modulbus_regs;
>       }
> 
>       /* Read the hex switch on the carrier board */
>       priv->hex = ioread8(priv->onboard_regs + JANZ_OB_MBUS_NUM);
>       dev_info(&dev->dev, "detected board with HEX switch %X\n", priv->hex);
> 
>       /* Disable all interrupt lines, hookup the handler */
>       iowrite8(0xf, priv->onboard_regs + JANZ_OB_INT_DISABLE);
>       ret = request_irq(dev->irq, janz_irq, IRQF_SHARED, drv_name, priv);
>       if (ret) {
>               dev_err(&dev->dev, "unable to register IRQ handler\n");
>               goto out_unmap_onboard_regs;
>       }
> 
>       /* Probe all of the modules on the CMOD-IO carrier board */
>       ret = janz_probe_modules(priv);
>       if (ret) {
>               dev_err(&dev->dev, "unable to probe MODULbus modules\n");
>               goto out_free_irq;
>       }
> 
>       return 0;
> 
> out_free_irq:
>       free_irq(dev->irq, priv);
> out_unmap_onboard_regs:
>       iounmap(priv->onboard_regs);
> out_unmap_modulbus_regs:
>       iounmap(priv->modulbus_regs);
> out_unmap_control_regs:
>       iounmap(priv->control_regs);
> out_pci_release_regions:
>       pci_release_regions(dev);
> out_pci_disable_device:
>       pci_disable_device(dev);
> out_free_priv:
>       kfree(priv);
> out_return:
>       return ret;
> }
> 
> static void janz_pci_remove(struct pci_dev *dev)
> {
>       struct janz_device *priv = pci_get_drvdata(dev);
> 
>       janz_remove_modules(priv);
>       free_irq(dev->irq, priv);
>       iounmap(priv->onboard_regs);
>       iounmap(priv->modulbus_regs);
>       iounmap(priv->control_regs);
>       pci_release_regions(dev);
>       pci_disable_device(dev);
>       kfree(priv);
> }
> 
> #define PCI_VENDOR_ID_JANZ            0x13c3
> 
> /* The list of devices that this module will support */
> static struct pci_device_id janz_pci_ids[] = {

Please use DEFINE_PCI_DEVICE_TABLE() here.

>       { PCI_VENDOR_ID_PLX, PCI_DEVICE_ID_PLX_9030, PCI_VENDOR_ID_JANZ, 0x0101 
> },
>       { PCI_VENDOR_ID_PLX, PCI_DEVICE_ID_PLX_9050, PCI_VENDOR_ID_JANZ, 0x0100 
> },
>       { 0, }
> };
> MODULE_DEVICE_TABLE(pci, janz_pci_ids);
> 
> static struct pci_driver janz_pci_driver = {
>       .name     = (char *)drv_name,
>       .id_table = janz_pci_ids,
>       .probe    = janz_pci_probe,
>       .remove   = janz_pci_remove,


You should also use __devinit where appropriate. See:
http://lxr.linux.no/#linux+v2.6.32/Documentation/PCI/pci.txt#L177

> };
> 
> /*----------------------------------------------------------------------------*/
> /* Module Init / Exit                                                         
> */
> /*----------------------------------------------------------------------------*/
> 
> static int __init janz_init(void)
> {
>       return pci_register_driver(&janz_pci_driver);
> }
> 
> static void __exit janz_exit(void)
> {
>       pci_unregister_driver(&janz_pci_driver);
> }
> 
> MODULE_AUTHOR("Ira W. Snyder <[email protected]>");
> MODULE_DESCRIPTION("Janz PCI CAN Driver");
> MODULE_LICENSE("GPL");
> 
> module_init(janz_init);
> module_exit(janz_exit);

Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core

Reply via email to