Re: [Xenomai-core] [PATCH] rtcan_mem - CAN driver for memory mapped SJA1000 controllers

[Jan Kiszka <jan.kiszka@domain.hid>]

[-- Attachment #1: Type: text/plain, Size: 11418 bytes --]

Matthias Fuchs wrote:
> Hi,
> 
> attached you will find a patch that adds support for memory mapped SJA1000 CAN 
> controllers as they often can be found on embedded boards. The driver is 
> based on the rtmen_isa driver.

Thanks for your patch! Below are only cleanup comments, not all directed
to you as some of them originate back to the rtcan_isa driver.

> 
> The driver has been tested on esd's embedded PowerPC boards with AMCC PPC405 
> CPUs.
> 
> Thanks to Jan for giving me some introduction to Xenomai during a nightly 
> session last friday.
> 
> There's one thing a I am not very satisfied with :-) Why passing half of the 
> external clock frequency to the module. Because of compatiblity reasons I 
> kept this behavior of the clock paramter from the ISA driver.

Wolfgang promised to rework this. I was also already about hacking some
can_sys_clock = clock[idx]/2 patch, but hesitated when I noticed that
the output in rtcan_raw_dev.c will print half of the provided frequency
then. Some smart documentation concept for those frequencies is required
first.

> 
> Matthias
> 
> 
> ------------------------------------------------------------------------
> 
> Index: ksrc/drivers/can/sja1000/Kconfig
> ===================================================================
> --- ksrc/drivers/can/sja1000/Kconfig	(revision 1564)
> +++ ksrc/drivers/can/sja1000/Kconfig	(working copy)
> @@ -13,6 +13,16 @@
>         int "Maximum number of ISA devices"
>         default 4       
>  
> +config XENO_DRIVERS_RTCAN_SJA1000_MEM
> +       depends on XENO_DRIVERS_RTCAN_SJA1000
> +       tristate "Memory mapped controllers"
> +       default n
> +
> +config XENO_DRIVERS_RTCAN_SJA1000_MEM_MAX_DEV
> +       depends on XENO_DRIVERS_RTCAN_SJA1000_MEM
> +       int "Maximum number of memory mapped controllers"
> +       default 4       
> +
>  config XENO_DRIVERS_RTCAN_SJA1000_PEAK_PCI
>         depends on XENO_DRIVERS_RTCAN_SJA1000
>         tristate "PEAK PCI Card"
> Index: ksrc/drivers/can/sja1000/rtcan_mem.c
> ===================================================================
> --- ksrc/drivers/can/sja1000/rtcan_mem.c	(revision 0)
> +++ ksrc/drivers/can/sja1000/rtcan_mem.c	(revision 0)
> @@ -0,0 +1,206 @@
> +/*
> + * Copyright (C) 2006 Matthias Fuchs <matthias.fuchs@domain.hidom>,
> + *                    Jan Kiszka <jan.kiszka@domain.hid>
> + * 
> + * RTCAN driver for memory mapped SJA1000 CAN controller
> + * This code has been tested on esd's CPCI405/EPPC405 PPC405 systems.
> + * 
> + * This driver is derived from the rtcan-isa driver by 
> + * Wolfgang Grandegger and Sebastian Smolorz. 
> + *
> + * Copyright (C) 2006 Wolfgang Grandegger <wg@domain.hid>
> + * Copyright (C) 2005, 2006 Sebastian Smolorz
> + *                          <Sebastian.Smolorz@domain.hid>
> + *
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; eitherer version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software Foundation,
> + * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/ioport.h>
> +#include <linux/delay.h>
> +
> +#include <rtdm/rtdm_driver.h>
> +
> +#include <rtdm/rtcan.h>
> +#include <rtcan_dev.h>
> +#include <rtcan_raw.h>
> +#include <rtcan_internal.h>
> +#include <rtcan_sja1000.h>
> +#include <rtcan_sja1000_regs.h>
> +
> +#define RTCAN_DEV_NAME    "rtcan%d"
> +#define RTCAN_DRV_NAME    "sja1000-mem"

You don't use RTCAN_DRV_NAME, so we can kill it.

But this line triggered an idea about a new naming scheme for our CAN
drivers (the shorter, the better):

xeno_can		(core)

xeno_can_virt		(virtual CAN bus)

xeno_sja1000		(SJA1000 core)
xeno_sja1000_port (or _io)
xeno_sja1000_mem
xeno_sja1000_pci 	(generalised peak_pci if feasible)
xeno_sja1000_peakdng	(Peak parport dongle)

xeno_mscan


Moreover, file and CONFIG names should be shortened. I see no need for
rtcan-prefixes if the file reside in the drivers/can folder, or for
XENO_DRIVER_RTCAN when XENO already expresses "RT" (=>XENO_DRIVER_CAN).

> +
> +#ifdef CONFIG_XENO_DRIVERS_RTCAN_MEM_MAX_DEVS
> +#define RTCAN_MEM_MAX_DEV CONFIG_XENO_DRIVERS_RTCAN_MEM_MAX_DEV
> +#else
> +#define RTCAN_MEM_MAX_DEV 2
> +#endif

This #ifdef is redundant (but copy&paste for rtcan_isa): there should be
no undefined CONFIG_XENO_DRIVERS_RTCAN_MEM_MAX_DEVS if this driver is
enabled for compilation.

> +
> +static char *mem_board_name = "mem mapped";
> +
> +MODULE_AUTHOR("Matthias Fuchs <matthias.fuchs@domain.hid>");
> +MODULE_DESCRIPTION("RTCAN driver for memory mapped SJA1000 controller");
> +MODULE_SUPPORTED_DEVICE("mem mapped");
> +MODULE_LICENSE("GPL");
> +
> +static u32 mem[RTCAN_MEM_MAX_DEV];
> +static int irq[RTCAN_MEM_MAX_DEV];
> +static u32 clock[RTCAN_MEM_MAX_DEV];
> +static u8 ocr[RTCAN_MEM_MAX_DEV];
> +static u8 cdr[RTCAN_MEM_MAX_DEV];
> +
> +compat_module_int_param_array(mem, RTCAN_MEM_MAX_DEV);
> +compat_module_int_param_array(irq, RTCAN_MEM_MAX_DEV);
> +compat_module_int_param_array(clock, RTCAN_MEM_MAX_DEV);
> +compat_module_byte_param_array(ocr, RTCAN_MEM_MAX_DEV);
> +compat_module_byte_param_array(cdr, RTCAN_MEM_MAX_DEV);
> +
> +MODULE_PARM_DESC(mem, "The io-memory address");
> +MODULE_PARM_DESC(irq, "The interrupt number");
> +MODULE_PARM_DESC(clock, "CAN system clock frequency (default 8 MHz)");
> +MODULE_PARM_DESC(ocr, "Value of output control register (default 0x1a)");
> +MODULE_PARM_DESC(cdr, "Value of clock divider register (default 0xc8");
> +
> +#define RTCAN_MEM_RANGE 0x80
> +
> +struct rtcan_mem
> +{
> +    u8 *mem; 
> +};
> +
> +static struct rtcan_device *rtcan_mem_devs[RTCAN_MEM_MAX_DEV];
> +
> +static u8 rtcan_mem_readreg(struct rtcan_device *dev, int reg)
> +{
> +    struct rtcan_mem *board = (struct rtcan_mem *)dev->board_priv;
> +    return readb(board->mem + reg);
> +}
> +
> +static void rtcan_mem_writereg(struct rtcan_device *dev, int reg, u8 val)
> +{
> +    struct rtcan_mem *board = (struct rtcan_mem *)dev->board_priv;
> +    writeb(val, board->mem + reg);
> +}
> +
> +int __init rtcan_mem_init_one(int idx)
> +{
> +    struct rtcan_device *dev;
> +    struct rtcan_sja1000 *chip;
> +    struct rtcan_mem *board;
> +    int ret;
> +    
> +    if ((dev = rtcan_dev_alloc(sizeof(struct rtcan_sja1000),
> +			       sizeof(struct rtcan_mem))) == NULL)
> +        return -ENOMEM;
> +
> +    chip = (struct rtcan_sja1000 *)dev->priv;
> +    board = (struct rtcan_mem *)dev->board_priv;
> +
> +    dev->board_name = mem_board_name; 
> +
> +    chip->irq_num = irq[idx];
> +    chip->irq_flags = RTDM_IRQTYPE_SHARED;
> +    chip->read_reg = rtcan_mem_readreg;
> +    chip->write_reg = rtcan_mem_writereg;
> +
> +    /* ioremap io memory */
> +    if (!(board->mem = (u8*)ioremap(mem[idx], RTCAN_MEM_RANGE))) {
> +	ret = -EBUSY;
> +	goto out_dev_free;
> +    }
> +
> +    /* Clock frequency in Hz */
> +    if (clock[idx])
> +	dev->can_sys_clock = clock[idx];
> +    else
> +	dev->can_sys_clock = 8000000; /* 16/2 MHz */
> +
> +    /* Output control register */
> +    if (ocr[idx])
> +	chip->ocr = ocr[idx];
> +    else
> +	chip->ocr = SJA_OCR_MODE_NORMAL | SJA_OCR_TX0_PUSHPULL;
> +
> +    if (cdr[idx])
> +	chip->cdr = cdr[idx];
> +    else
> +	chip->cdr = SJA_CDR_CAN_MODE | SJA_CDR_CLK_OFF | SJA_CDR_CBP;
> +
> +    strncpy(dev->name, RTCAN_DEV_NAME, IFNAMSIZ);
> +
> +    ret = rtcan_sja1000_register(dev);
> +    if (ret) {
> +	printk(KERN_ERR "ERROR %d while trying to register SJA1000 device!\n",
> +	       ret);
> +	goto out_free_region;
> +    }
> +    
> +    rtcan_mem_devs[idx] = dev;
> +    return 0;
> +    
> + out_free_region:
> +    iounmap(board->mem);
> +
> + out_dev_free:
> +    rtcan_dev_free(dev);
> +
> +    return ret;
> +}
> +
> +/** Init module */
> +static int __init rtcan_mem_init(void)
> +{
> +    int                         i;
> +    int                         ret;
> +    int done = 0;

Not nicely formatted. BTW, as this is new code to be included, we should
take the chance and convert it to kernel style right from the beginning. :)

> +    for (i = 0; 
> +	 i < RTCAN_MEM_MAX_DEV && mem[i] != 0; 
> +	 i++) {
> +
> +	if ((ret = rtcan_mem_init_one(i) != 0)) {
> +	    return ret;
> +	}
> +	done++;
> +    }
> +    if (done)
> +	return 0;
> +    printk(KERN_ERR "ERROR! No devices specified! "
> +	   "Use mem=<base_addr1>[,...] irq=<irq1>[,...]\n");
> +
> +    return -EINVAL;
> +}
> +
> +
> +/** Cleanup module */
> +static void __exit rtcan_mem_exit(void)
> +{
> +    int i;
> +    struct rtcan_device *dev;
> +    u8 *mem;
> +
> +    for (i = 0; i < RTCAN_MEM_MAX_DEV; i++) {
> +	    dev = rtcan_mem_devs[i];
> +	    if (!dev)
> +		    continue;
> +	    mem = ((struct rtcan_mem *)dev->board_priv)->mem;
> +	    rtcan_sja1000_unregister(dev);
> +	    iounmap(mem);
> +    }
> +}
> +
> +module_init(rtcan_mem_init);
> +module_exit(rtcan_mem_exit);
> Index: ksrc/drivers/can/sja1000/Makefile
> ===================================================================
> --- ksrc/drivers/can/sja1000/Makefile	(revision 1564)
> +++ ksrc/drivers/can/sja1000/Makefile	(working copy)
> @@ -8,11 +8,13 @@
>  obj-$(CONFIG_XENO_DRIVERS_RTCAN_SJA1000_PEAK_PCI) += xeno_rtcan_peak_pci.o
>  obj-$(CONFIG_XENO_DRIVERS_RTCAN_SJA1000_PEAK_DNG) += xeno_rtcan_peak_dng.o
>  obj-$(CONFIG_XENO_DRIVERS_RTCAN_SJA1000_ISA) += xeno_rtcan_isa.o
> +obj-$(CONFIG_XENO_DRIVERS_RTCAN_SJA1000_MEM) += xeno_rtcan_mem.o
>  
>  xeno_rtcan_sja1000-y := rtcan_sja1000.o rtcan_sja1000_proc.o
>  xeno_rtcan_peak_pci-y := rtcan_peak_pci.o
>  xeno_rtcan_peak_dng-y := rtcan_peak_dng.o
>  xeno_rtcan_isa-y := rtcan_isa.o
> +xeno_rtcan_mem-y := rtcan_mem.o
>  
>  else
>  
> @@ -24,6 +26,7 @@
>  obj-$(CONFIG_XENO_DRIVERS_RTCAN_SJA1000_PEAK_PCI) += xeno_rtcan_peak_pci.o 
>  obj-$(CONFIG_XENO_DRIVERS_RTCAN_SJA1000_PEAK_DNG) += xeno_rtcan_peak_dng.o 
>  obj-$(CONFIG_XENO_DRIVERS_RTCAN_SJA1000_ISA) += xeno_rtcan_isa.o 
> +obj-$(CONFIG_XENO_DRIVERS_RTCAN_SJA1000_MEM) += xeno_rtcan_mem.o
>  
>  list-multi := xeno_rtcan_sja1000.o 
>  
> @@ -31,6 +34,7 @@
>  xeno_rtcan_peak_pci-objs := rtcan_peak_pci.o
>  xeno_rtcan_peak_dng-objs := rtcan_peak_dng.o
>  xeno_rtcan_isa-objs := rtcan_isa.o
> +xeno_rtcan_mem-objs := rtcan_mem.o
>  
>  export-objs := $(xeno_rtcan_sja1000-objs)
>  
> @@ -49,4 +53,7 @@
>  
>  xeno_rtcan_isa.o: $(xeno_rtcan_isa-objs)
>  	$(LD) -r -o $@ $(xeno_rtcan_isa-objs)
> +
> +xeno_rtcan_mem.o: $(xeno_rtcan_mem-objs)
> +	$(LD) -r -o $@ $(xeno_rtcan_mem-objs)
>  endif
> 

Patch for scripts/Modules.frag is missing (=> built-in compilation under
2.4) - just like for rtcan_isa... :-/

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]