Hello, your patch looks already pretty good. You can find some general guidelines on how to prepare Socketcan patches for kernel inclusion here:
http://svn.berlios.de/wsvn/socketcan/trunk/README.submitting-patches Please use a subject as suggested therein, provide a commit message including your signed-off-by and run checkpatch.pl. On 10/06/2010 12:49 PM, Andre B. Oliveira wrote: > 2010/10/6 Andre B. Oliveira <[email protected]>: >> 2010/10/6 Wolfram Sang <[email protected]>: >>> On Wed, Oct 06, 2010 at 10:48:30AM +0100, Andre B. Oliveira wrote: >>>> Here is a driver for the Technologic Systems TS-CAN1 PC104 peripheral >>>> board: >>>> http://clientes.netvisao.pt/anbadeol/can.html >>> Can't you post the patch itself? Only this makes it possible to review. >> Sorry! Here is the patch. > > Sorry, again! Here is the patch, inlined: > > diff -uprN -X linux-2.6.35.4/Documentation/dontdiff > linux-2.6.35.4/drivers/net/can/sja1000/Kconfig > linux-2.6.35.4-tscan1/drivers/net/can/sja1000/Kconfig > --- linux-2.6.35.4/drivers/net/can/sja1000/Kconfig 2010-08-27 > 00:47:12.000000000 +0100 > +++ linux-2.6.35.4-tscan1/drivers/net/can/sja1000/Kconfig 2010-09-15 > 15:59:46.000000000 +0100 > @@ -58,4 +58,16 @@ config CAN_PLX_PCI > - esd CAN-PCIe/2000 > - Marathon CAN-bus-PCI card (http://www.marathon.ru/) > - TEWS TECHNOLOGIES TPMC810 card (http://www.tews.com/) > + > +config CAN_TSCAN1 > + tristate "TS-CAN1 PC104 boards" > + depends on ISA > + help > + This driver is for Technologic Systems' TSCAN-1 PC104 boards. > + http://www.embeddedarm.com/products/board-detail.php?product=TS-CAN1 > + The driver supports multiple boards and automatically configures them: > + PLD IO base addresses are read from jumpers JP1 and JP2, > + IRQ numbers are read from jumpers JP4 and JP5, > + SJA1000 IO base addresses are chosen heuristically (first that works). > + > endif > diff -uprN -X linux-2.6.35.4/Documentation/dontdiff > linux-2.6.35.4/drivers/net/can/sja1000/Makefile > linux-2.6.35.4-tscan1/drivers/net/can/sja1000/Makefile > --- linux-2.6.35.4/drivers/net/can/sja1000/Makefile 2010-08-27 > 00:47:12.000000000 +0100 > +++ linux-2.6.35.4-tscan1/drivers/net/can/sja1000/Makefile 2010-09-06 > 11:08:23.000000000 +0100 > @@ -9,5 +9,6 @@ obj-$(CONFIG_CAN_SJA1000_OF_PLATFORM) += > obj-$(CONFIG_CAN_EMS_PCI) += ems_pci.o > obj-$(CONFIG_CAN_KVASER_PCI) += kvaser_pci.o > obj-$(CONFIG_CAN_PLX_PCI) += plx_pci.o > +obj-$(CONFIG_CAN_TSCAN1) += tscan1.o > > ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG > diff -uprN -X linux-2.6.35.4/Documentation/dontdiff > linux-2.6.35.4/drivers/net/can/sja1000/tscan1.c > linux-2.6.35.4-tscan1/drivers/net/can/sja1000/tscan1.c > --- linux-2.6.35.4/drivers/net/can/sja1000/tscan1.c 1970-01-01 > 01:00:00.000000000 +0100 > +++ linux-2.6.35.4-tscan1/drivers/net/can/sja1000/tscan1.c 2010-09-15 > 16:10:26.000000000 +0100 > @@ -0,0 +1,184 @@ > +/* > + * linux/drivers/net/can/sja1000/tscan1.c - driver for TS-CAN1 PC104 boards Please drop the redundant path above. > + * Copyright 2010 Andre B. Oliveira > + * > + * 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, either version 3 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, see <http://www.gnu.org/licenses/>. > + */ > + > +/* > + * References: > + * - Getting started with TS-CAN1, Technologic Systems, Jun 2009 > + * http://www.embeddedarm.com/documentation/ts-can1-manual.pdf > + */ > + > +#include <asm/io.h> checkpatch.pl will complain about the above line. > +#include <linux/init.h> > +#include <linux/ioport.h> > +#include <linux/isa.h> > +#include <linux/module.h> > +#include <linux/netdevice.h> > +#include "sja1000.h" > + > +MODULE_DESCRIPTION("Driver for TS-CAN1 PC104 CAN interface boards"); > +MODULE_AUTHOR("Andre B. Oliveira <[email protected]>"); > +MODULE_LICENSE("GPL"); > + > +/* PLD register space size */ > +#define TSCAN1_PLD_SIZE 8 > + > +/* SJA1000 register space size */ > +#define TSCAN1_SJA1000_SIZE 32 > + > +/* Read SJA1000 register */ > +static u8 tscan1_read(const struct sja1000_priv *priv, int reg) > +{ > + return inb((int)priv->reg_base + reg); > +} > + > +/* Write SJA1000 register */ > +static void tscan1_write(const struct sja1000_priv *priv, int reg, u8 val) > +{ > + outb(val, (int)priv->reg_base + reg); > +} Is the cast really needed? > +/* Probe for a TSCAN1 device with JP1:JP2 jumper selection ID */ > +static int tscan1_match(struct device *dev, unsigned id) > +{ > + int pld, sja1000, irq, i; > + struct net_device *netdev; > + struct sja1000_priv *priv; > + > + /* Request PLD IO register region */ > + pld = 0x150 + id * 8; Please use #define's here and in other places to define constant values. > + if (!request_region(pld, TSCAN1_PLD_SIZE, "tscan1")) > + return -EBUSY; > + > + /* Check magic values in PLD board identifier bytes */ > + if (inb(pld) != 0xf6 || inb(pld + 1) != 0xb9) > + goto err1; Ditto. > + > + /* Read JP4:JP5 jumper settings to find out the selected IRQ */ > + switch (inb(pld + 6) & 0x30) { > + case 0x10: irq = 6; break; checkpatch does not like that. > + case 0x20: irq = 7; break; > + case 0x30: irq = 5; break; > + default: goto err1; Ditto. > + } > + > + netdev = alloc_sja1000dev(0); > + if (!netdev) > + goto err1; > + > + dev_set_drvdata(dev, netdev); > + SET_NETDEV_DEV(netdev, dev); > + > + netdev->base_addr = pld; > + netdev->irq = irq; > + > + priv = netdev_priv(netdev); > + priv->read_reg = tscan1_read; > + priv->write_reg = tscan1_write; > + priv->can.clock.freq = 16000000 / 2; > + priv->cdr = 0x48; /* CBP | CLKOFF */ > + priv->ocr = 0x18; /* TP0 | TN0 */ Please use values from http://lxr.linux.no/#linux/include/linux/can/platform/sja1000.h > + /* Select the first SJA1000 IO address that is free and that works */ > + for (i = 0; i < 8; i++) { > + const unsigned short addr[] = { > + 0x100, 0x120, 0x180, 0x1a0, 0x200, 0x240, 0x280, 0x320 > + }; I would make addr a global declaration and use ARRAY_SIZE(). > + sja1000 = addr[i]; s/sja1000/base/ ? > + > + if (!request_region(sja1000, TSCAN1_SJA1000_SIZE, "tscan1")) > + continue; > + > + /* Set the SJA1000 IO base address and enable it */ > + outb(0x40 | i, pld + 5); > + > + /* Probe and register SJA1000 */ > + priv->reg_base = (void __iomem *)sja1000; > + if (!register_sja1000dev(netdev)) > + break; Hm, does the simple probing of sja1000_probe_chip() work reliably? You can find a more intelligent test in the plx_pci driver. I wonder if it's *save* to access an invalid IO port? Anyway, why do you need to probe the IO port? Can't it be derived from the JP1:JP2 jumper settings? > + /* The probe for an SJA1000 at this address failed: release */ > + outb(0, pld + 5); > + release_region(sja1000, TSCAN1_SJA1000_SIZE); > + } > + if (i == 8) > + goto err2; > + > + /* Turn the LED off */ > + outb(0, pld + 3); > + > + printk(KERN_INFO "%s: TS-CAN1 at 0x%x 0x%x irq %d\n", > + netdev->name, pld, sja1000, irq); > + > + return 0; > + > +err2: dev_set_drvdata(dev, NULL); > + free_sja1000dev(netdev); > +err1: release_region(pld, TSCAN1_PLD_SIZE); > + return -ENODEV; > +} > + > +/* Remove TSCAN1 device */ > +static int __devexit tscan1_remove(struct device *dev, unsigned id > /*unused*/) > +{ > + struct net_device *netdev; > + struct sja1000_priv *priv; > + int pld, sja1000; > + > + netdev = dev_get_drvdata(dev); > + unregister_sja1000dev(netdev); > + dev_set_drvdata(dev, NULL); > + > + priv = netdev_priv(netdev); > + pld = netdev->base_addr; > + sja1000 = (int)priv->reg_base; > + > + /* Release SJA1000 IO space */ > + outb(0, pld + 5); > + release_region(sja1000, TSCAN1_SJA1000_SIZE); > + > + /* Release PLD IO space */ > + release_region(pld, TSCAN1_PLD_SIZE); > + > + /* Free net device */ > + free_sja1000dev(netdev); > + > + return 0; > +} > + > +static struct isa_driver tscan1_isa_driver = { > + .match = tscan1_match, > + .remove = __devexit_p(tscan1_remove), > + .driver = { > + .name = "tscan1", > + }, > +}; > + > +/* Register 4 devices, one for each JP1:JP2 jumper selection of IO address */ > +static int __init tscan1_init(void) > +{ > + return isa_register_driver(&tscan1_isa_driver, 4); Please use a #define for 4. > +} > +module_init(tscan1_init); > + > +static void __exit tscan1_exit(void) > +{ > + isa_unregister_driver(&tscan1_isa_driver); > +} > +module_exit(tscan1_exit); Thanks, Wolfgang. _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
