Hi Peter, here finally comes my review, sorry for the delay.
As discussed, there is already a Socket-CAN driver for the PEAK-PCI card in the SVN trunk. Please compare and double check. In general, for kernel inclusion you need to prepare patches for David Millers "net-next-2.6" GIT tree, which you can get as shown below: $ git clone \ git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next-2.6.git The patch should then be sent to the netdev mailing list wich CC to the Socketcan-core mailing list. Furthermore, your patch should be checked with checkpatch.pl to find the obvious coding style violations. More details about the coding style requirements can be found in the kernel "Documentation/CodingStyle" file. Peter Horton wrote: > The patch below adds support for the Peak PCI CAN interface cards. The > cards are SJA1000 based so only the PCI glue is needed. > > Tested with a 2 channel card but should work with 1, 2, 3 or 4 channel > cards. > > Signed-off-by: Peter Horton <[email protected]> > > -- > > diff -urpN linux-2.6.31.orig/drivers/net/can/Kconfig > linux-2.6.31/drivers/net/can/Kconfig > --- linux-2.6.31.orig/drivers/net/can/Kconfig 2009-09-09 23:13:59.000000000 > +0100 > +++ linux-2.6.31/drivers/net/can/Kconfig 2009-11-05 10:17:05.000000000 > +0000 > @@ -74,6 +74,12 @@ config CAN_KVASER_PCI > This driver is for the the PCIcanx and PCIcan cards (1, 2 or > 4 channel) from Kvaser (http://www.kvaser.com). > > +config CAN_PEAK_PCI > + tristate "Peak PCI CAN Cards" > + depends on PCI && CAN_SJA1000 > + ---help--- > + This driver is for the Peak PCI CAN cards (1, 2, 3 or 4 channels). Please mention the vendor like above for the Kvasar PCI card. > config CAN_DEBUG_DEVICES > bool "CAN devices debugging messages" > depends on CAN > diff -urpN linux-2.6.31.orig/drivers/net/can/sja1000/Makefile > linux-2.6.31/drivers/net/can/sja1000/Makefile > --- linux-2.6.31.orig/drivers/net/can/sja1000/Makefile 2009-09-09 > 23:13:59.000000000 +0100 > +++ linux-2.6.31/drivers/net/can/sja1000/Makefile 2009-11-05 > 10:17:05.000000000 +0000 > @@ -7,5 +7,6 @@ obj-$(CONFIG_CAN_SJA1000_PLATFORM) += sj > obj-$(CONFIG_CAN_SJA1000_OF_PLATFORM) += sja1000_of_platform.o > obj-$(CONFIG_CAN_EMS_PCI) += ems_pci.o > obj-$(CONFIG_CAN_KVASER_PCI) += kvaser_pci.o > +obj-$(CONFIG_CAN_PEAK_PCI) += peak_pci.o > > ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG > diff -urpN linux-2.6.31.orig/drivers/net/can/sja1000/peak_pci.c > linux-2.6.31/drivers/net/can/sja1000/peak_pci.c > --- linux-2.6.31.orig/drivers/net/can/sja1000/peak_pci.c 1970-01-01 > 01:00:00.000000000 +0100 > +++ linux-2.6.31/drivers/net/can/sja1000/peak_pci.c 2009-11-05 > 10:21:54.000000000 +0000 > @@ -0,0 +1,287 @@ > +/* > + * (C) BitBox Ltd 2009 A name and a mail address would be nice. > + * > + * PCI glue for Peak PCI SJA1000 CAN cards > + * > + * mostly lifted from ems_pci.c Then please keep the copyright notes of that file and add further in case you used code from other sources, e.g. from Peak's PCAN drivers. Furthermore, add a proper copyright note (the usual for GPL vX bla bla). > + */ > + > +#include <linux/module.h> > +#include <linux/pci.h> > +#include <linux/netdevice.h> I think you also need: #include <linux/interrupt.h> #include <linux/delay.h> > + > +#include "sja1000.h" > + > +#define DRIVER_NAME "peak-pci-can" Should be "peak_pci", like the file name. > + > +#define PEAK_PCI_VENDOR_ID 0x001c > +#define PEAK_PCI_DEVICE_ID 0x0001 > + > +#define PEAK_PCI_MAP_SIZE (4 << 10) I find "0x4000" more readable. > + > +#define PEAK_PCI_CHAN_COUNT 4 PEAK_PCI_CHAN_MAX? > + > +#define PEAK_PCI_REG_SIZE 4 > +#define PEAK_PCI_CHAN_SIZE (1 << 10) Ditto, see above. > +#define PEAK_PCI_CAN_CLOCK (8 * 1000 * 1000) Why not just "8000000"? Or even better (16000000 / 2) to make clear that it's half of the oscillator frequency. > +#define PEAK_PCI_OCR (OCR_TX0_PUSHPULL | OCR_MODE_NORMAL) > +#define PEAK_PCI_CDR (CDR_PELICAN | CDR_CBP | CDR_CLKOUT_MASK) I remember a different setting for a single channel card (using CDR_CLK_OFF). Also, you can remove CDR_PELICAN, which will be set by the SJA1000 driver. > +#define PITA_ICR 0x0000 > +#define PITA_GPIOICR 0x0018 > +#define PITA_MISC 0x001c > + > +struct peak_pci_card > +{ Move this brace one line up. > + unsigned int channels; > + struct pci_dev *pdev; > + void __iomem *conf_addr; > + void __iomem *base_addr; > + struct net_device *net_dev[PEAK_PCI_CHAN_COUNT]; > +}; Please don't align member names. > +static struct pci_device_id const peak_pci_table[] = > +{ Move this brace one line up. > + { > + PEAK_PCI_VENDOR_ID, PEAK_PCI_DEVICE_ID, > + PEAK_PCI_VENDOR_ID, PCI_ANY_ID > + }, > + { }, > +}; > + > +MODULE_DEVICE_TABLE(pci, peak_pci_table); > + > +static u8 peak_pci_can_read(struct sja1000_priv const *ctlr, int reg) "const struct sja1000_priv" please. > +{ > + return ioread8(ctlr->reg_base + reg * PEAK_PCI_REG_SIZE); > +} > + > +static void > +peak_pci_can_write(struct sja1000_priv const *ctlr, int reg, u8 data) Ditto, see above. And please break the declaration differently, as suggested below. > +{ > + iowrite8(data, ctlr->reg_base + reg * PEAK_PCI_REG_SIZE); > +} > + > +static unsigned int peak_pci_intr_mask(struct sja1000_priv const *ctlr) > +{ > + static unsigned int const mask[] = > + { > + 1 << 1, > + 1 << 0, > + 1 << 6, > + 1 << 7, > + }; > + > + struct peak_pci_card *card; > + unsigned int chan; > + > + card = ctlr->priv; > + > + chan = (ctlr->reg_base - card->base_addr) / PEAK_PCI_CHAN_SIZE; > + > + BUG_ON(chan >= ARRAY_SIZE(mask)); > + > + return mask[chan]; > +} Hm, calculating the channel from the base address frequently seems not to be a good idea. Also please put the interrupt mask declaration into the header, e.g.: static const unsigned int peak_pci_intr_mask[PEAK_PCI_CHAN_MAX] = ... > + > +static void peak_pci_post_irq(struct sja1000_priv const *ctlr) > +{ > + struct peak_pci_card *card; > + > + card = ctlr->priv; Please merge with the line above. > + /* acknowledge interrupt */ > + iowrite16(peak_pci_intr_mask(ctlr), card->conf_addr + PITA_ICR + 0); It would make sense to use a per channel structure with the member intr_mask. > +} > + > +static int > +peak_pci_add_card(struct pci_dev *pdev, struct pci_device_id const *id) Please don't put "static int" on a separate line but put the arguments on the continuation line. > +{ > + struct peak_pci_card *card; > + struct sja1000_priv *ctlr; > + struct net_device *ndev; > + unsigned int mask, nchan; > + u16 ident; > + int err; > + > + err = pci_enable_device(pdev); > + if (err < 0) { if (err) ? > + dev_err(&pdev->dev, "failed to enable PCI device\n"); > + return err; > + } > + > + err = -ENOMEM; > + > + card = kzalloc(sizeof(struct peak_pci_card), GFP_KERNEL); sizeof(*card) is less error prune. > + if (card == NULL) { > + dev_err(&pdev->dev, "failed to allocate memory\n"); > + goto disable_device; > + } > + > + card->pdev = pdev; > + pci_set_drvdata(pdev, card); > + > + card->conf_addr = pci_iomap(pdev, 0, PEAK_PCI_MAP_SIZE); > + if (card->conf_addr == NULL) { > + dev_err(&pdev->dev, "failed to map PCI resource #0\n"); > + goto free_context; > + } > + > + card->base_addr = pci_iomap(pdev, 1, PEAK_PCI_MAP_SIZE); > + if (card->base_addr == NULL) { > + dev_err(&pdev->dev, "failed to map PCI resource #1\n"); > + goto unmap_config; > + } > + > + err = pci_read_config_word(pdev, PCI_SUBSYSTEM_ID, &ident); s/ident/subsys_id/ ? > + if (err) { > + dev_err(&pdev->dev, "failed to get PCI device identifier\n"); > + goto unmap_base; > + } > + > + nchan = 1; > + if (ident >= 4) > + ++nchan; > + if (ident >= 10) > + ++nchan; > + if (ident >= 12) > + ++nchan; I think the following code is more transparent: if (ident >= 12) nchan = 4; else if (ident >= 10) nchan = 3; else if (ident >= 4) nchan = 2; else nchan = 1; > + dev_info(&pdev->dev, "%u channel card\n", nchan); > + > + /* set GP0 and GP2 as outputs */ > + iowrite16(0x0005, card->conf_addr + PITA_GPIOICR + 2); > + /* set GP0 and GP2 low */ > + iowrite8(0x00, card->conf_addr + PITA_GPIOICR + 0); Please check the corresponding SVN trunk or PCAN driver. I remember a channel dependent setting there. > + /* parallel mode, soft reset */ > + iowrite8(0x05, card->conf_addr + PITA_MISC + 3); > + mdelay(5); > + /* parallel mode */ > + iowrite8(0x04, card->conf_addr + PITA_MISC + 3); > + mask = 0; > + > + err = -ENOMEM; > + > + while (card->channels < nchan) { > + Remove empty line please. > + ndev = alloc_sja1000dev(0); > + if (ndev == NULL) { > + dev_err(&pdev->dev, "failed to allocate memory\n"); > + goto free_devices; > + } > + > + ndev->irq = pdev->irq; > + > + ctlr = netdev_priv(ndev); > + > + ctlr->priv = card; > + ctlr->irq_flags = IRQF_SHARED; > + ctlr->reg_base = > + card->base_addr + card->channels * PEAK_PCI_CHAN_SIZE; > + ctlr->read_reg = peak_pci_can_read; > + ctlr->write_reg = peak_pci_can_write; > + ctlr->post_irq = peak_pci_post_irq; > + ctlr->can.clock.freq = PEAK_PCI_CAN_CLOCK; > + ctlr->ocr = PEAK_PCI_OCR; > + ctlr->cdr = PEAK_PCI_CDR; Please use just *one* space before and after the "=". > + SET_NETDEV_DEV(ndev, &pdev->dev); > + > + mask |= peak_pci_intr_mask(ctlr); mask |= peak_pci_intr_mask[card->channels]; ? I also would prefer reading the ICR first and just setting/clearing the bit relevant for the channel. > + /* acknowledge interrupt */ > + iowrite16(mask, card->conf_addr + PITA_ICR + 0); > + /* enable interrupt */ > + iowrite16(mask, card->conf_addr + PITA_ICR + 2); > + > + err = register_sja1000dev(ndev); > + if (err < 0) { if (err) ? > + dev_err(&pdev->dev, > + "failed to register SJA1000 device\n"); > + free_sja1000dev(ndev); > + goto free_devices; > + } > + > + card->net_dev[card->channels++] = ndev; > + } > + > + return 0; > + > +free_devices: > + /* disable interrupts */ > + iowrite16(0x0000, card->conf_addr + PITA_ICR + 2); > + > + while (card->channels) { > + Please remove the empty line above. > + ndev = card->net_dev[--card->channels]; > + unregister_sja1000dev(ndev); > + free_sja1000dev(ndev); > + } > + > +unmap_base: > + pci_iounmap(pdev, card->base_addr); > + > +unmap_config: > + pci_iounmap(pdev, card->conf_addr); > + > +free_context: > + kfree(card); > + > +disable_device: > + pci_disable_device(pdev); > + > + return err; > +} > + > +static void peak_pci_del_card(struct pci_dev *pdev) > +{ > + struct peak_pci_card *card; > + struct net_device *ndev; > + > + card = pci_get_drvdata(pdev); Please merge that with the corresponding declaration above. > + while (card->channels) { > + > + ndev = card->net_dev[--card->channels]; > + dev_info(&pdev->dev, "removing device %s\n", ndev->name); > + unregister_sja1000dev(ndev); > + free_sja1000dev(ndev); > + } > + > + pci_iounmap(pdev, card->base_addr); > + pci_iounmap(pdev, card->conf_addr); > + > + pci_set_drvdata(pdev, NULL); > + > + kfree(card); > + > + pci_disable_device(pdev); > +} > + > +static struct pci_driver peak_pci_driver = > +{ Move this brace one line up. > + .name = DRIVER_NAME, > + .id_table = peak_pci_table, > + .probe = peak_pci_add_card, > + .remove = peak_pci_del_card, Please use just *one* space before and after the "=". > +}; > + > +static int __init peak_pci_init(void) > +{ > + return pci_register_driver(&peak_pci_driver); > +} > + > +static void __exit peak_pci_exit(void) > +{ > + pci_unregister_driver(&peak_pci_driver); > +} > + > +module_init(peak_pci_init); > +module_exit(peak_pci_exit); > + > +MODULE_LICENSE("GPL v2"); Please add the usual MODULE_AUTHOR, MODULE_DESCRIPTION, and MODULE_SUPPORTED_DEVICE definitions and move these up to the beginning of the file. > + > +/* vi:set ts=8 sw=8 cin: */ Please remove vi formating rules. I have a PCAN PCI 2-channel card and I will use it to test v2 of your patch. Thanks for your contribution. Wolfgang. _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
