Hi Pavel, Pavel B. Cheblakov wrote: > Hi! > > This driver (plx_pci) adds support for some sja1000 CAN controllers > which is based on PLX PCI bridges. > It works with: > - Marathon CAN-bus-PCI card (http://www.marathon.ru/) > - Adlink PCI-7841/cPCI-7841 card (http://www.adlinktech.com/) > - Adlink PCI-7841/cPCI-7841 SE card (it's a slight modification of > previous card) > - TEWS TECHNOLOGIES TPMC810 card (http://www.tews.com/)
Nice. > I'm not sure that plx_pci is a right name for this driver. But I think > this driver have certain potentialities to support any other similar > cards (based on PLX bridges). The name sounds reasonable. In principle, also the ixxat_pci cards could be supported by this driver. IIRC, there was just some trick with probing the number of CAN channels. You may want to check/compare? In general it's *good* to share code. > Attached patch against revision 1095 of socketcan SVN. > I hope it will be useful for anybody else. It's usefull, of course. BTW: could you please send you patches inline next time to simplify the review. I'm adding the patch below manually. Some general comments first: - Use the proper style for comments. Please check: http://lxr.linux.no/#linux+v2.6.32/Documentation/CodingStyle#L425 - Consider using (dev)/(!dev) instead of ((dev != NULL)/(dev == NULL). > Index: kernel/2.6/drivers/net/can/sja1000/plx_pci.c > =================================================================== > --- kernel/2.6/drivers/net/can/sja1000/plx_pci.c > (.../svn+ssh://chebla...@v5cvs/socketcan/vendor/socketcan/1095) (revision 0) > +++ kernel/2.6/drivers/net/can/sja1000/plx_pci.c (revision 199) > @@ -0,0 +1,426 @@ > +/* > + * Copyright (C) 2008-2009 Pavel Cheblakov <[email protected]> > + * > + * Derived from the ems_pci.c driver: > + * Copyright (C) 2007 Wolfgang Grandegger <[email protected]> > + * Copyright (C) 2008 Markus Plessing <[email protected]> > + * Copyright (C) 2008 Sebastian Haas <[email protected]> > + * > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the version 2 of the GNU General Public License > + * as published by the Free Software Foundation > + * > + * 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. > + */ > + > +/* > + Driver plx_pci, version 1.1.0, 2009-12-26 > +*/ Please use the required style for comments. > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/interrupt.h> > +#include <linux/netdevice.h> > +#include <linux/delay.h> > +#include <linux/pci.h> > +#include <socketcan/can.h> > +#include <socketcan/can/dev.h> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 16) > +#include <linux/io.h> > +#else > +#include <asm/io.h> > +#endif > + > +#include "sja1000.h" > + > +#define DRV_NAME "plx_pci" > + > +MODULE_AUTHOR("Pavel Cheblakov <[email protected]>"); > +MODULE_DESCRIPTION("Socket-CAN driver for PLX9xxx PCI-bridge cards with " > + "sja1000 chips"); > +MODULE_SUPPORTED_DEVICE("Marathon CAN-bus-PCI, " > + "Adlink PCI-7841/cPCI-7841, " > + "Adlink PCI-7841/cPCI-7841 SE, " > + "TEWS TECHNOLOGIES TPMC810"); > +MODULE_LICENSE("GPL v2"); > + > +#define PLX_PCI_MAX_CHAN 2 > + > +struct plx_pci_card { > + int channels; /* detected channels count */ > + > + struct pci_dev *pci_dev; > + struct net_device *net_dev[PLX_PCI_MAX_CHAN]; > + > + void __iomem *conf_addr; > + void __iomem *base_addr[PLX_PCI_MAX_CHAN]; > +}; > + > +#define PLX_PCI_CAN_CLOCK (16000000 / 2) > + > +/* > + * PLX9xxx registers > + */ > +#define PLX_INTCSR 0x4c /* Interrup Control/Status */ > +#define PLX_CNTRL 0x50 /* User I/O, Direct Slave Response, > + Serial EEPROM, and Initialization > + Control register */ > + > +#define PLX_LINT1_EN 0x1 /* Local interrupt 1 enable */ > +#define PLX_LINT2_EN (1 << 3) /* Local interrupt 2 enable */ > +#define PLX_PCI_INT_EN (1 << 6) /* PCI Interrupt Enable > */ > +#define PLX_PCI_RESET (1 << 30) /* PCI Adapter Software > Reset */ > + > +/* > + * The board configuration is probably following: > + * RX1 is connected to ground. > + * TX1 is not connected. > + * CLKO is not connected. > + * Setting the OCR register to 0xDA is a good idea. > + * This means normal output mode , push-pull and the correct polarity. ^ remove space before ",". > + */ > +#define PLX_PCI_OCR (OCR_TX0_PUSHPULL | OCR_TX1_PUSHPULL) > + > +/* > + * In the CDR register, you should set CBP to 1. > + * You will probably also want to set the clock divider value to 7 > + * (meaning direct oscillator output) because the second SJA1000 chip > + * is driven by the first one CLKOUT output. > + */ > +#define PLX_PCI_CDR (CDR_CBP | CDR_CLKOUT_MASK) > + > +#define ADLINK_PCI_VENDOR_ID 0x144A > +#define ADLINK_PCI_DEVICE_ID 0x7841 > + > +#define MARATHON_PCI_DEVICE_ID 0x2715 > + > +#define TEWS_PCI_VENDOR_ID 0x1498 > +#define TEWS_PCI_DEVICE_ID_TMPC810 0x032A Please check if the vendors are already defined in the kernel's "include/linux/pci_ids.h". > +enum plx_cards { > + MARATHON_CAN_BUS_PCI = 0, > + ADLINK_PCI_7841, > + ADLINK_PCI_7841_SE, > + TEWS_TPMC810 > +}; > + > +struct channel_map { > + u8 bar; As it does not save any space, u32 is fine for "bar" as well. > + u32 offset; > + u32 size; /* 0x00 - auto, e.g. length of entire bar */ > +}; > + > +static struct plx_card_info { > + const char *name; > + u8 channel_count; unsigned int? See above. > + int can_clock; > + > + /* Parameters for mapping local configuration space is located > + * at index 0 in this array. > + * Subsequent array elements correspond to mapping SJA1000 chips. > + */ Please correct comment style. > + struct channel_map ch_map_tbl[PLX_PCI_MAX_CHAN + 1]; > +} plx_card_info_tbl[] __devinitdata = { > + {"Marathon CAN-bus-PCI", Please use just one tab per indention level, put "{" on a separate line and concatenate some lines, e.g.: } plx_card_info_tbl[] __devinitdata = { { "Marathon CAN-bus-PCI", 2, PLX_PCI_CAN_CLOCK, { {0, 0x00, 0x00}, {2, 0x00, 0x00}, {4, 0x00, 0x00} } }, > + 2, PLX_PCI_CAN_CLOCK, { {0, 0x00, 0x00}, > + {2, 0x00, 0x00}, > + {4, 0x00, 0x00} } > + }, > + {"Adlink PCI-7841/cPCI-7841", > + 2, PLX_PCI_CAN_CLOCK, { {1, 0x00, 0x00}, > + {2, 0x00, 0x80}, > + {2, 0x80, 0x80} } > + }, > + {"Adlink PCI-7841/cPCI-7841 SE", > + 2, PLX_PCI_CAN_CLOCK, { {0, 0x00, 0x00}, > + {2, 0x00, 0x80}, > + {2, 0x80, 0x80} } > + }, > + {"TEWS TECHNOLOGIES TPMC810", > + 2, PLX_PCI_CAN_CLOCK, { {0, 0x00, 0x00}, > + {2, 0x000, 0x80}, > + {2, 0x100, 0x80} } > + }, > + { NULL } Do you need the zero termination? Use ARRAY_SIZE() instead. > +}; > + > +static struct pci_device_id plx_pci_tbl[] = { > + > + { /* Marathon CAN-bus-PCI card */ > + PCI_VENDOR_ID_PLX, MARATHON_PCI_DEVICE_ID, > + PCI_ANY_ID, PCI_ANY_ID, > + 0, 0, > + MARATHON_CAN_BUS_PCI > + }, > + > + { /* Adlink PCI-7841/cPCI-7841 */ > + ADLINK_PCI_VENDOR_ID, ADLINK_PCI_DEVICE_ID, > + PCI_ANY_ID, PCI_ANY_ID, > + PCI_CLASS_NETWORK_OTHER << 8, ~0, > + ADLINK_PCI_7841 > + }, > + { /* Adlink PCI-7841/cPCI-7841 SE */ > + ADLINK_PCI_VENDOR_ID, ADLINK_PCI_DEVICE_ID, > + PCI_ANY_ID, PCI_ANY_ID, > + PCI_CLASS_COMMUNICATION_OTHER << 8, ~0, > + ADLINK_PCI_7841_SE > + }, > + { /* TEWS TECHNOLOGIES TPMC810 card */ > + TEWS_PCI_VENDOR_ID, TEWS_PCI_DEVICE_ID_TMPC810, > + PCI_ANY_ID, PCI_ANY_ID, > + 0, 0, > + TEWS_TPMC810 > + }, > + { 0,} > +}; > + Please remove empty line above. > +MODULE_DEVICE_TABLE(pci, plx_pci_tbl); > + > +static u8 plx_pci_read_reg(const struct sja1000_priv *priv, int port) > +{ > + return readb(priv->reg_base + port); > +} > + > +static void plx_pci_write_reg(const struct sja1000_priv *priv, int port, u8 > val) > +{ > + writeb(val, priv->reg_base + port); > +} > + > +/* > + * Check if a CAN controller is present at the specified location > + * by trying to set 'em into the PeliCAN mode > + */ > +static inline int plx_pci_check_sja1000(const struct sja1000_priv *priv) > +{ > + u8 res; > + > + /* Make sure SJA1000 is in reset mode */ > + priv->write_reg(priv, REG_MOD, 1); > + > + priv->write_reg(priv, REG_CDR, CDR_PELICAN); > + > + /* read reset-values */ > + res = priv->read_reg(priv, REG_CDR); > + > + if (res == CDR_PELICAN) > + return 1; The ixxat pci driver uses a more sophisticated probing mechanism, which might fit here as well. > + > + return 0; > +} > + > +/* > + * Software reset PLX9xxx > + * Also LRESET# asserts and brings to reset device on the Local Bus > + */ > +static void plx_pci_reset(struct plx_pci_card *card) > +{ > + u32 cntrl; > + > + cntrl = ioread32(card->conf_addr + PLX_CNTRL); > + cntrl |= PLX_PCI_RESET; > + iowrite32(cntrl, card->conf_addr + PLX_CNTRL); > + udelay(100); > + cntrl ^= PLX_PCI_RESET; > + iowrite32(cntrl, card->conf_addr + PLX_CNTRL); > +}; > + > +static void plx_pci_del_card(struct pci_dev *pdev) > +{ > + struct plx_pci_card *card = pci_get_drvdata(pdev); > + struct net_device *dev; > + int i = 0; > + > + for (i = 0; i < card->channels; i++) { > + dev = card->net_dev[i]; > + Consider removing the empty line above please. > + if (!dev) > + continue; > + > + dev_info(&pdev->dev, "Removing %s.\n", dev->name); Please remove the ".". > + unregister_sja1000dev(dev); > + free_sja1000dev(dev); > + > + if (card->base_addr[i] != NULL) > + pci_iounmap(card->pci_dev, card->base_addr[i]); > + } > + > + plx_pci_reset(card); > + > + /* Disable interrupts from PCI-card (PLX9xxx) and disable Local_1, > + * Local_2 interrupts */ Please fix comment sytle. > + iowrite32(0x0, card->conf_addr + PLX_INTCSR); > + > + if (card->conf_addr != NULL) if (card->conf_addr) ? > + pci_iounmap(card->pci_dev, card->conf_addr); > + > + kfree(card); > + > + pci_disable_device(pdev); > + pci_set_drvdata(pdev, NULL); > +} > + > +/* > + * Probe PLX9xxx based device for SJA1000 chips and register each available > + * CAN channel to SJA1000 Socket-CAN subsystem. > + */ > +static int __devinit plx_pci_add_card(struct pci_dev *pdev, > + const struct pci_device_id *ent) > +{ > + struct sja1000_priv *priv; > + struct net_device *dev; > + struct plx_pci_card *card; > + struct plx_card_info *ci; > + int err, i; > + u32 tmp; s/tmp/val/ ? > + > + ci = &plx_card_info_tbl[ent->driver_data]; > + > + if (pci_enable_device(pdev) < 0) { > + dev_err(&pdev->dev, "Failed to enable PCI device\n"); > + return -ENODEV; > + } > + > + dev_info(&pdev->dev, "Detected \"%s\" card at slot #%i\n", > + ci->name, PCI_SLOT(pdev->devfn)); > + > + /* Allocate card structures to hold addresses, ... */ > + card = kzalloc(sizeof(struct plx_pci_card), GFP_KERNEL); Please use card = kzalloc(*card, ... > + if (card == NULL) { We usually use "!card". > + dev_err(&pdev->dev, "Unable to allocate memory\n"); > + pci_disable_device(pdev); > + return -ENOMEM; > + } > + > + pci_set_drvdata(pdev, card); > + > + card->pci_dev = pdev; > + card->channels = 0; > + > + /* Remap PLX9xxx configuration space */ > + card->conf_addr = pci_iomap(pdev, ci->ch_map_tbl[0].bar, > + ci->ch_map_tbl[0].size) + > + ci->ch_map_tbl[0].offset; > + /* this calculation only for commonality */ I don't understand the comment. > + if (card->conf_addr == NULL) { Hm, this test is wrong if "ci->ch_map_tbl[0].offset != 0", right? > + err = -ENOMEM; > + dev_err(&pdev->dev, > + "Failed to remap configuration space (BAR%d)\n", > + ci->ch_map_tbl[0].bar); > + goto failure_cleanup; > + } > + dev_dbg(&pdev->dev, "Configuration space remaped to 0x%x\n", > + (u32)card->conf_addr); Please use "0x%p" to avoid the cast. > + > + plx_pci_reset(card); > + > + /* Detect available channels */ > + for (i = 0; i < ci->channel_count; i++) { > + dev = alloc_sja1000dev(0); > + if (dev == NULL) { if (!dev) ? > + err = -ENOMEM; > + goto failure_cleanup; > + } > + > + card->net_dev[i] = dev; > + priv = netdev_priv(dev); > + priv->priv = card; > +#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 18) > + priv->irq_flags = SA_SHIRQ; > +#else > + priv->irq_flags = IRQF_SHARED; > +#endif > + > + dev->irq = pdev->irq; > + > + /* > + * Remap IO space of SJA1000 chips > + * This is device-dependent mapping > + */ > + card->base_addr[i] = pci_iomap(pdev, ci->ch_map_tbl[i+1].bar, > + ci->ch_map_tbl[i+1].size) + > + ci->ch_map_tbl[i+1].offset; > + if (card->base_addr[i] == NULL) { if (!card->base_addr[i]) ? > + err = -ENOMEM; > + goto failure_cleanup; > + } Wrong check, see above. > + dev_dbg(&pdev->dev, "IO space of chip #%d remaped to 0x%x\n", > + i+1, (u32)card->base_addr[i]); "0x%p" ? See above. Also "i + 1", please > + dev->base_addr = (u32)card->base_addr[i]; > + > + priv->reg_base = (void *)card->base_addr[i]; > + priv->read_reg = plx_pci_read_reg; > + priv->write_reg = plx_pci_write_reg; > + > + /* Check if channel is present */ > + if (plx_pci_check_sja1000(priv)) { > + priv->can.clock.freq = ci->can_clock; > + priv->ocr = PLX_PCI_OCR; > + priv->cdr = PLX_PCI_CDR; BTW: do all cards work with the same OCR/CDR settings? > + SET_NETDEV_DEV(dev, &pdev->dev); > + > + /* Register SJA1000 device */ > + err = register_sja1000dev(dev); > + if (err) { > + dev_err(&pdev->dev, "Registering device failed " > + "(err=%d)\n", err); > + free_sja1000dev(dev); > + goto failure_cleanup; > + } > + > + card->channels++; > + > + dev_info(&pdev->dev, "Channel #%d at 0x%x, irq %d " > + "registered as %s\n", > + i + 1, (u32)dev->base_addr, > + dev->irq, dev->name); Use "0x%p" to avoid the cast. > + } else { > + dev_err(&pdev->dev, "Channel #%d not detected\n", i+1); "i + 1", please. > + free_sja1000dev(dev); > + } > + } > + > + /* Enable interrupts from PCI-card (PLX9xxx) and enable Local_1, > + * Local_2 interrupts from SJA1000 chips */ Comment style? > + tmp = ioread32(card->conf_addr + PLX_INTCSR); > + tmp |= PLX_LINT1_EN | PLX_LINT2_EN | PLX_PCI_INT_EN; > + iowrite32(tmp, card->conf_addr + PLX_INTCSR); > + > + return 0; > + > +failure_cleanup: > + dev_err(&pdev->dev, "Error: %d. Cleaning Up.\n", err); > + > + plx_pci_del_card(pdev); > + > + return err; > +} > + > +static struct pci_driver plx_pci_driver = { > + .name = DRV_NAME, > + .id_table = plx_pci_tbl, > + .probe = plx_pci_add_card, > + .remove = plx_pci_del_card, > +}; > + > +static int __init plx_pci_init(void) > +{ > + return pci_register_driver(&plx_pci_driver); > +} > + > +static void __exit plx_pci_exit(void) > +{ > + pci_unregister_driver(&plx_pci_driver); > +} > + > +module_init(plx_pci_init); > +module_exit(plx_pci_exit); > + > Index: kernel/2.6/drivers/net/can/sja1000/Kconfig > =================================================================== > --- kernel/2.6/drivers/net/can/sja1000/Kconfig > (.../svn+ssh://chebla...@v5cvs/socketcan/vendor/socketcan/1095) (revision 199) > +++ kernel/2.6/drivers/net/can/sja1000/Kconfig (working copy) > @@ -36,6 +36,17 @@ > This driver is for the one, two or four channel CPC-PCI, > CPC-PCIe and CPC-104P cards from EMS Dr. Thomas Wuensche > (http://www.ems-wuensche.de). > + Remove trailing white space above, please. > +config CAN_PLX_PCI > + tristate "Socket-CAN driver for PLX9xxx PCI-bridge cards with sja1000 > chips" Can you please shorten this string and make it more like for the others SJA1000 drivers, e.g: tristate "PLX9xxx PCI-bridge based Cards" > + depends on PCI && CAN_SJA1000 > + ---help--- > + This driver is for CAN interface cards which based on PLX9xxx PCI > bridge. s/which// ? > + Driver supports now: > + - Marathon CAN-bus-PCI card (http://www.marathon.ru/) > + - Adlink PCI-7841/cPCI-7841 card (http://www.adlinktech.com/) > + - Adlink PCI-7841/cPCI-7841 SE card > + - TEWS TECHNOLOGIES TPMC810 card (http://www.tews.com/) > > config CAN_EMS_PCMCIA > tristate "EMS CPC-CARD Card" > Index: kernel/2.6/drivers/net/can/sja1000/Makefile > =================================================================== > --- kernel/2.6/drivers/net/can/sja1000/Makefile > (.../svn+ssh://chebla...@v5cvs/socketcan/vendor/socketcan/1095) (revision 199) > +++ kernel/2.6/drivers/net/can/sja1000/Makefile (working copy) > @@ -20,6 +20,7 @@ > obj-$(CONFIG_CAN_SJA1000_PLATFORM) += sja1000_platform.o > obj-$(CONFIG_CAN_SJA1000_OF_PLATFORM) += sja1000_of_platform.o > obj-$(CONFIG_CAN_EMS_PCI) += ems_pci.o > +obj-$(CONFIG_CAN_PLX_PCI) += plx_pci.o > obj-$(CONFIG_CAN_EMS_PCMCIA) += ems_pcmcia.o > obj-$(CONFIG_CAN_EMS_104M) += ems_104m.o > obj-$(CONFIG_CAN_ESD_PCI) += esd_pci.o > Index: kernel/2.6/drivers/net/can/Makefile > =================================================================== > --- kernel/2.6/drivers/net/can/Makefile > (.../svn+ssh://chebla...@v5cvs/socketcan/vendor/socketcan/1095) (revision 199) > +++ kernel/2.6/drivers/net/can/Makefile (working copy) > @@ -23,6 +23,7 @@ > export CONFIG_CAN_SOFTING=m > export CONFIG_CAN_SOFTING_CS=m > export CONFIG_CAN_MCP251X=m > +export CONFIG_CAN_PLX_PCI=m > > modules modules_install clean: > $(MAKE) -C $(KERNELDIR) M=$(PWD) $@ TOPDIR=$(TOPDIR) Do you intend to push the driver to mainlain as well? That would be nice. Thanks for your contribution. Wolfgang. _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
