On 08/01/2011 02:17 PM, Thomas Wiedemann wrote: > Hi, > > i've attached a patch for the Peak CAN PCI driver, which enables support > for the 3rd and 4th channel available on some cards. It works* for me.
Thanks for the patch. Please don't attach patches, rather post them inline. Please follow the cannonical patch format as documented here: http://lxr.linux.no/linux+v3.0/Documentation/SubmittingPatches See further remarks inline. I just noticed that this patch is against the berlios tree. I don't know if there is a mainline driver that supports the peak pci cards. > --- peak_pci.c.orig 2011-07-28 13:43:20.000000000 +0200 > +++ peak_pci.c 2011-07-28 14:08:08.000000000 +0200 > @@ -38,6 +38,9 @@ > > #define DRV_NAME "peak_pci" > > +// Maximum number of PEAK CAN channels on a PCI device No C++ comments, please use /* */ for comments. > +#define MAX_PCAN_CHANNELS_PER_CARD 4 > + > #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,23) > #error This driver does not support Kernel versions < 2.6.23 > #endif > @@ -50,14 +53,10 @@ > struct peak_pci { > int channel; > struct pci_dev *pci_dev; > - struct net_device *slave_dev; > + struct net_device *slave_dev[MAX_PCAN_CHANNELS_PER_CARD - 1]; > volatile void __iomem *conf_addr; > }; > > -#define PEAK_PCI_SINGLE 0 /* single channel device */ > -#define PEAK_PCI_MASTER 1 /* multi channel master device */ > -#define PEAK_PCI_SLAVE 2 /* multi channel slave device */ > - > #define PEAK_PCI_CAN_CLOCK (16000000 / 2) > > #define PEAK_PCI_CDR_SINGLE (CDR_CBP | CDR_CLKOUT_MASK | CDR_CLK_OFF) > @@ -108,12 +107,14 @@ > > /* Select and clear in Pita stored interrupt */ > icr_low = readw(board->conf_addr + PITA_ICR); > - if (board->channel == PEAK_PCI_SLAVE) { > - if (icr_low & 0x0001) > - writew(0x0001, board->conf_addr + PITA_ICR); > - } else { > - if (icr_low & 0x0002) > - writew(0x0002, board->conf_addr + PITA_ICR); > + switch(board->channel) { > + case 0: if (icr_low & 0x0002) { writew(0x0002, board->conf_addr > + PITA_ICR); } break; > + case 1: if (icr_low & 0x0001) { writew(0x0001, board->conf_addr > + PITA_ICR); } break; > + case 2: if (icr_low & 0x0040) { writew(0x0040, board->conf_addr > + PITA_ICR); } break; > + case 3: if (icr_low & 0x0080) { writew(0x0080, board->conf_addr > + PITA_ICR); } break; > + default: > + printk(KERN_INFO "Peak CAN card with an > enormous amount of channels detected: %d, channel disabled.\n", > board->channel); Use dev_<LEVEL> instead of printk(<LEVEL>, ). If possible stay below 80 chars per line. I suggest to make a static inline function to that returns the bitmask instead of numerous switch case. > + break; > } > } > > @@ -139,15 +140,17 @@ > unregister_sja1000dev(dev); > case 4: > icr_high = readw(board->conf_addr + PITA_ICR + 2); > - if (board->channel == PEAK_PCI_SLAVE) > - icr_high &= ~0x0001; > - else > - icr_high &= ~0x0002; > + switch(board->channel) { > + case 0: icr_high &= ~0x0002; break; > + case 1: icr_high &= ~0x0001; break; > + case 2: icr_high &= ~0x0040; break; > + case 3: icr_high &= ~0x0080; break; > + } make use of the static inlne function I proposed earlier > writew(icr_high, board->conf_addr + PITA_ICR + 2); > case 3: > iounmap(priv->reg_base); > case 2: > - if (board->channel != PEAK_PCI_SLAVE) > + if (board->channel == 0) // master channel fix comment > iounmap((void *)board->conf_addr); > case 1: > free_sja1000dev(dev); > @@ -177,7 +180,7 @@ > board->pci_dev = pdev; > board->channel = channel; > > - if (channel != PEAK_PCI_SLAVE) { > + if (channel == 0) { // master channel only fix comment > > addr = pci_resource_start(pdev, 0); > board->conf_addr = ioremap(addr, PCI_CONFIG_PORT_SIZE); > @@ -190,11 +193,9 @@ > /* Set GPIO control register */ > writew(0x0005, board->conf_addr + PITA_GPIOICR + 2); > > - /* Enable single or dual channel */ > - if (channel == PEAK_PCI_MASTER) > - writeb(0x00, board->conf_addr + PITA_GPIOICR); > - else > - writeb(0x04, board->conf_addr + PITA_GPIOICR); > + // enable all channels of this card fix comment > + writeb(0x00, board->conf_addr + PITA_GPIOICR); > + > /* Toggle reset */ > writeb(0x05, board->conf_addr + PITA_MISC + 3); > mdelay(5); > @@ -203,13 +204,14 @@ > } else { > struct sja1000_priv *master_priv = netdev_priv(*master_dev); > struct peak_pci *master_board = master_priv->priv; > - master_board->slave_dev = dev; > + BUG_ON(! (channel - 1 < sizeof(board->slave_dev)/sizeof(struct > net_device *))); ^ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ please remove that space we have ARRAY_SIZE for that > + master_board->slave_dev[channel - 1] = dev; > board->conf_addr = master_board->conf_addr; > } > > addr = pci_resource_start(pdev, 1); > - if (channel == PEAK_PCI_SLAVE) > - addr += PCI_PORT_SIZE; > + if (channel > 0) you can remove that if now :) > + addr += PCI_PORT_SIZE * board->channel; > > priv->reg_base = ioremap(addr, PCI_PORT_SIZE); > if (priv->reg_base == 0) { > @@ -226,7 +228,7 @@ > > priv->ocr = PEAK_PCI_OCR; > > - if (channel == PEAK_PCI_MASTER) > + if (channel == 0) > priv->cdr = PEAK_PCI_CDR_MASTER; > else > priv->cdr = PEAK_PCI_CDR_SINGLE; > @@ -239,10 +241,12 @@ > #endif > dev->irq = pdev->irq; > icr_high = readw(board->conf_addr + PITA_ICR + 2); > - if (channel == PEAK_PCI_SLAVE) > - icr_high |= 0x0001; > - else > - icr_high |= 0x0002; > + switch(channel) { > + case 0: icr_high |= 0x0002; break; > + case 1: icr_high |= 0x0001; break; > + case 2: icr_high |= 0x0040; break; > + case 3: icr_high |= 0x0080; break; > + } use the static inline > writew(icr_high, board->conf_addr + PITA_ICR + 2); > init_step = 4; > > @@ -256,7 +260,7 @@ > goto failure; > } > > - if (channel != PEAK_PCI_SLAVE) > + if (channel == 0) > *master_dev = dev; > > printk(KERN_INFO "%s: %s at reg_base=0x%p conf_addr=%p irq=%d\n", > @@ -295,21 +299,20 @@ > if (err) > goto failure_cleanup; > > - if (sub_sys_id > 3) { > - err = peak_pci_add_chan(pdev, > - PEAK_PCI_MASTER, &master_dev); > - if (err) > - goto failure_cleanup; > - > - err = peak_pci_add_chan(pdev, > - PEAK_PCI_SLAVE, &master_dev); > - if (err) > - goto failure_cleanup; > - } else { > - err = peak_pci_add_chan(pdev, PEAK_PCI_SINGLE, > - &master_dev); > - if (err) > - goto failure_cleanup; > + /* Add the CAN channels. The number of channels available depends on > sub_sys_id */ > + // 1st channel > is always present fix comment...multiline comments should look like this: /* * no * comment */ > + if ((err = peak_pci_add_chan(pdev, 0, &master_dev))) { goto > failure_cleanup; } > + > + if (sub_sys_id >= 4) { // 2nd channel > + if ((err = peak_pci_add_chan(pdev, 1, &master_dev))) { goto > failure_cleanup; } > + } > + > + if (sub_sys_id >= 10) { // 3rd channel ^^ I don't know the datasheet of the card. If I designed the hardware, "8" would be the correct value here. But hardware guys are sometimes strange people :P > + if ((err = peak_pci_add_chan(pdev, 2, &master_dev))) { goto > failure_cleanup; } > + } > + > + if (sub_sys_id >= 12) { // 4th channel > + if ((err = peak_pci_add_chan(pdev, 3, &master_dev))) { goto > failure_cleanup; } > } please fix the comments, and reformat the if: if (foo) bar(); If the "8" would be correct, you cae use a "for" look to activate the slave channels. > > pci_set_drvdata(pdev, master_dev); > @@ -332,8 +335,13 @@ > struct sja1000_priv *priv = netdev_priv(dev); > struct peak_pci *board = priv->priv; > > - if (board->slave_dev) > - peak_pci_del_chan(board->slave_dev, 0); > + // close all associated CAN channels of this card dix comment. > + int i; the declaration should go to the top... > + for(i = 0; i < sizeof(board->slave_dev)/sizeof(struct net_device *) ; > i++) { please use ARRAY_SIZE > + if(board->slave_dev[i] != NULL) { ^ space please you cann remove the != NULL, YMMV > + peak_pci_del_chan(board->slave_dev[i], 0); > + } You can remove the { }... > + } > peak_pci_del_chan(dev, 0); > > pci_release_regions(pdev); > > > > _______________________________________________ > Socketcan-users mailing list > [email protected] > https://lists.berlios.de/mailman/listinfo/socketcan-users cheers, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Industrial Linux Solutions | Phone: +49-231-2826-924 | Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Socketcan-users mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-users
