Hi Pavel, at a closer look, I still see some issues and possible improvements.
You inline patch is somehow white space mangled. Tabs are replaced with 4 spaces, etc. Please make sure that your mail client does not change the patch. P.B.Cheblakov wrote: > 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 201) > @@ -0,0 +1,436 @@ > +/* > + * 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.1, 2009-12-26 > + */ > + > +#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]; I don't think you really need base_addr[]? More later. > +}; > + > +#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. > + */ > +#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 > + > +enum plx_cards { > + MARATHON_CAN_BUS_PCI = 0, > + ADLINK_PCI_7841, > + ADLINK_PCI_7841_SE, > + TEWS_TPMC810 > +}; > + > +struct channel_map { Please consider using the prefix "plx_pci_" here are well. > + u32 bar; > + u32 offset; > + u32 size; /* 0x00 - auto, e.g. length of entire bar */ > +}; > + > +static struct plx_card_info { Ditto. > + const char *name; > + u32 channel_count; > + 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. > + */ > + struct channel_map ch_map_tbl[PLX_PCI_MAX_CHAN + 1]; I find it confusing that you combine config and channel data in that array: struct channel_map conf_map; struct channel_map chan_map_tbl[PLX_PCI_MAX_CHAN]; seems more appropriate and will make the code more readable. > +} plx_card_info_tbl[] __devinitdata = { > + { > + "Marathon CAN-bus-PCI", 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} } > + }, > +}; Instead of passing the index to the pci_device_id table, it's more elegant and straight forward to pass a pointer to the relevant data, e.g. static struct plx_card_info plx_card_info_marathon = { "Marathon CAN-bus-PCI", 2, PLX_PCI_CAN_CLOCK, { {0, 0x00, 0x00}, {2, 0x00, 0x00}, {4, 0x00, 0x00} } }; > +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 &plx_card_info_marathon Making "enum plx_cards" obsolete. > + }, > + { > + /* 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,} > +}; > +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); > +} You already use io[read/write]32 below and therefore it would make sense to use io[read/write]32 here as well. > +/* > + * 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; > + > + return 0; > +} See my previous comment on probing the sja1000 chip. > +/* > + * 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]; > + if (!dev) > + continue; > + > + dev_info(&pdev->dev, "Removing %s\n", dev->name); > + 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 > + */ > + iowrite32(0x0, card->conf_addr + PLX_INTCSR); > + > + 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 val; > + void __iomem *addr; > + > + ci = &plx_card_info_tbl[ent->driver_data]; ci = ent->driver_data; See comments above (some cast might be needed, though). > + > + 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(*card), GFP_KERNEL); > + if (!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 */ > + addr = pci_iomap(pdev, ci->ch_map_tbl[0].bar, > + ci->ch_map_tbl[0].size); ci->conf_map.bar? See comments above. > + if (!addr) { > + err = -ENOMEM; > + dev_err(&pdev->dev, > + "Failed to remap configuration space (BAR%d)\n", > + ci->ch_map_tbl[0].bar); > + goto failure_cleanup; > + } > + card->conf_addr = addr + ci->ch_map_tbl[0].offset; > + dev_dbg(&pdev->dev, "Configuration space remaped to 0x%p\n", > + card->conf_addr); > + > + plx_pci_reset(card); > + > + /* Detect available channels */ > + for (i = 0; i < ci->channel_count; i++) { struct channel_map *cm = &ci->chan_map_tbl[i]; would be handy here. > + dev = alloc_sja1000dev(0); > + 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 > + */ > + addr = pci_iomap(pdev, ci->ch_map_tbl[i + 1].bar, > + ci->ch_map_tbl[i + 1].size); > + if (!addr) { > + err = -ENOMEM; > + dev_err(&pdev->dev, > + "Failed to remap BAR%d\n", > + ci->ch_map_tbl[i + 1].bar); > + goto failure_cleanup; > + } > + card->base_addr[i] = addr + ci->ch_map_tbl[i + 1].offset; > + dev_dbg(&pdev->dev, "IO space of chip #%d remaped to 0x%p\n", > + i + 1, card->base_addr[i]); > + dev->base_addr = (u32)card->base_addr[i]; We should not use or even set dev->base_addr. Please remove. > + > + priv->reg_base = (void *)card->base_addr[i]; As commented above, I don't see any need for card->base_addr[i] as priv->reg_base is available later-on for cleanup. > + 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; > + > + 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%lx, irq %d " > + "registered as %s\n", > + i + 1, dev->base_addr, > + dev->irq, dev->name); > + } else { > + dev_err(&pdev->dev, "Channel #%d not detected\n", > + i + 1); > + free_sja1000dev(dev); > + } > + } > + > + if (!card->channels) { > + err = -ENODEV; > + goto failure_cleanup; > + } > + > + /* > + * Enable interrupts from PCI-card (PLX9xxx) and enable Local_1, > + * Local_2 interrupts from SJA1000 chips > + */ > + val = ioread32(card->conf_addr + PLX_INTCSR); > + val |= PLX_LINT1_EN | PLX_LINT2_EN | PLX_PCI_INT_EN; > + iowrite32(val, 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); Wolfgang. _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
