From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <50069E14.3040200@grandegger.com> Date: Wed, 18 Jul 2012 13:29:24 +0200 From: Wolfgang Grandegger MIME-Version: 1.0 References: <4FED4948.8080906@basystemes.fr> <4FED4B42.6000808@wanadoo.fr> <4FEDF65A.50401@grandegger.com> <1012208988.15650.1341343674156.JavaMail.www@wwinf1m30> <4FF3562B.7020902@grandegger.com> <50042E37.8040205@wanadoo.fr> <500524A7.3070907@grandegger.com> <5006892D.1020401@wanadoo.fr> <50069BD3.5060905@grandegger.com> In-Reply-To: <50069BD3.5060905@grandegger.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Xenomai] RTDM driver for the Advantech 1680U CANbus Adapter List-Id: Discussions about the Xenomai project List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thierry Bultel Cc: Xenomai@xenomai.org On 07/18/2012 01:19 PM, Wolfgang Grandegger wrote: > Hi Thierry, > > I removed the Xenomai ML as it is off-topic there... Oops, it's the other way round. Swapping mailing lists... > On 07/18/2012 12:00 PM, Thierry Bultel wrote: >> Hi Wolfgang, >> >> After some tuning of thunderbird things should get better now. > > Yes, you are close... just a few coding style issues remaining. > > [FYI: for Thunderbird I use the add-on "Toggle Word Wrap".] > >> As you required, I have added a comment in Kconfig, about the 1680U which >> is the only board I now about. >> >> Regarding the "80 chars" stuff, what I meant is that it remains a single >> location where checkpatch complains, but I do not know how to fix it; >> line is actually only 64 chars long at most so that is a little puzzling. >> >> WARNING: line over 80 characters >> #361: FILE: ksrc/drivers/can/sja1000/rtcan_adv_pci.c:293: >> + CHANNEL_SLAVE, > > Here I count 9 tabs plus 15 chars = 87 chars! A tab is 8 chars for Linux! > >> All the idents are done with tabs (it was the case in the previous mail as well but thunderbird broke it). >> so checkpatch is happy for everything else. > > The tabs still look wired... > >> Thanks in advance for the integration; >> Cheers >> Thierry >> >> >> diff -pruN xenomai-2.6.0-orig/ksrc/drivers/can/sja1000/Kconfig xenomai-2.6.0/ksrc/drivers/can/sja1000/Kconfig >> --- xenomai-2.6.0-orig/ksrc/drivers/can/sja1000/Kconfig 2011-10-18 20:17:18.000000000 +0200 >> +++ xenomai-2.6.0/ksrc/drivers/can/sja1000/Kconfig 2012-07-18 11:46:44.578890869 +0200 >> @@ -41,6 +41,15 @@ config XENO_DRIVERS_CAN_SJA1000_IXXAT_PC >> the second channel working, Xenomai's shared interrupt support >> must be enabled. >> >> +config XENO_DRIVERS_CAN_SJA1000_ADV_PCI >> + depends on XENO_DRIVERS_CAN_SJA1000 && PCI >> + tristate "ADVANTECH PCI Card" >> + help >> + >> + This driver is for the ADVANTECH PCI cards (1 or more channels) >> + It supports the 1680U and some other ones. >> + >> + >> config XENO_DRIVERS_CAN_SJA1000_PLX_PCI >> depends on XENO_DRIVERS_CAN_SJA1000 && PCI >> tristate "PLX90xx PCI-bridge based Cards" >> diff -pruN xenomai-2.6.0-orig/ksrc/drivers/can/sja1000/Makefile xenomai-2.6.0/ksrc/drivers/can/sja1000/Makefile >> --- xenomai-2.6.0-orig/ksrc/drivers/can/sja1000/Makefile 2011-10-18 20:17:18.000000000 +0200 >> +++ xenomai-2.6.0/ksrc/drivers/can/sja1000/Makefile 2012-07-16 14:00:38.682836737 +0200 >> @@ -9,6 +9,7 @@ obj-$(CONFIG_XENO_DRIVERS_CAN_SJA1000_PE >> obj-$(CONFIG_XENO_DRIVERS_CAN_SJA1000_PEAK_DNG) += xeno_can_peak_dng.o >> obj-$(CONFIG_XENO_DRIVERS_CAN_SJA1000_PLX_PCI) += xeno_can_plx_pci.o >> obj-$(CONFIG_XENO_DRIVERS_CAN_SJA1000_IXXAT_PCI) += xeno_can_ixxat_pci.o >> +obj-$(CONFIG_XENO_DRIVERS_CAN_SJA1000_ADV_PCI) += xeno_can_adv_pci.o >> obj-$(CONFIG_XENO_DRIVERS_CAN_SJA1000_EMS_PCI) += xeno_can_ems_pci.o >> obj-$(CONFIG_XENO_DRIVERS_CAN_SJA1000_ESD_PCI) += xeno_can_esd_pci.o >> obj-$(CONFIG_XENO_DRIVERS_CAN_SJA1000_ISA) += xeno_can_isa.o >> @@ -19,6 +20,7 @@ xeno_can_peak_pci-y := rtcan_peak_pci.o >> xeno_can_peak_dng-y := rtcan_peak_dng.o >> xeno_can_plx_pci-y := rtcan_plx_pci.o >> xeno_can_ixxat_pci-y := rtcan_ixxat_pci.o >> +xeno_can_adv_pci-y := rtcan_adv_pci.o >> xeno_can_ems_pci-y := rtcan_ems_pci.o >> xeno_can_esd_pci-y := rtcan_esd_pci.o >> xeno_can_isa-y := rtcan_isa.o >> @@ -35,6 +37,7 @@ obj-$(CONFIG_XENO_DRIVERS_CAN_SJA1000_PE >> obj-$(CONFIG_XENO_DRIVERS_CAN_SJA1000_PEAK_DNG) += xeno_can_peak_dng.o >> obj-$(CONFIG_XENO_DRIVERS_CAN_SJA1000_PLX_PCI) += xeno_can_plx_pci.o >> obj-$(CONFIG_XENO_DRIVERS_CAN_SJA1000_IXXAT_PCI) += xeno_can_ixxat_pci.o >> +obj-$(CONFIG_XENO_DRIVERS_CAN_SJA1000_ADV_PCI) += xeno_can_adv_pci.o >> obj-$(CONFIG_XENO_DRIVERS_CAN_SJA1000_EMS_PCI) += xeno_can_ems_pci.o >> obj-$(CONFIG_XENO_DRIVERS_CAN_SJA1000_ESD_PCI) += xeno_can_esd_pci.o >> obj-$(CONFIG_XENO_DRIVERS_CAN_SJA1000_ISA) += xeno_can_isa.o >> @@ -47,6 +50,7 @@ xeno_can_peak_pci-objs := rtcan_peak_pci >> xeno_can_peak_dng-objs := rtcan_peak_dng.o >> xeno_can_plx_pci-objs := rtcan_plx_pci.o >> xeno_can_ixxat_pci-objs := rtcan_ixxat_pci.o >> +xeno_can_adv_pci-objs := rtcan_adv_pci.o >> xeno_can_ems_pci-objs := rtcan_ems_pci.o >> xeno_can_esd_pci-objs := rtcan_esd_pci.o >> xeno_can_isa-objs := rtcan_isa.o >> @@ -73,6 +77,9 @@ xeno_can_plx_pci.o: $(xeno_can_plx_pci-o >> xeno_can_ixxat_pci.o: $(xeno_can_ixxat_pci-objs) >> $(LD) -r -o $@ $(xeno_can_ixxat_pci-objs) >> >> +xeno_can_adv_pci.o: $(xeno_can_adv_pci-objs) >> + $(LD) -r -o $@ $(xeno_can_adv_pci-objs) >> + >> xeno_can_ems_pci.o: $(xeno_can_ems_pci-objs) >> $(LD) -r -o $@ $(xeno_can_ems_pci-objs) >> >> diff -pruN xenomai-2.6.0-orig/ksrc/drivers/can/sja1000/rtcan_adv_pci.c xenomai-2.6.0/ksrc/drivers/can/sja1000/rtcan_adv_pci.c >> --- xenomai-2.6.0-orig/ksrc/drivers/can/sja1000/rtcan_adv_pci.c 1970-01-01 01:00:00.000000000 +0100 >> +++ xenomai-2.6.0/ksrc/drivers/can/sja1000/rtcan_adv_pci.c 2012-07-16 16:58:14.175087435 +0200 >> @@ -0,0 +1,351 @@ >> +/* >> + * Copyright (C) 2006 Wolfgang Grandegger > > Your copyright seems to be missing. > >> + * >> + * 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 2 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, write to the Free Software Foundation, >> + * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> + >> +#define ADV_PCI_BASE_SIZE 0x80 >> + >> +/* CAN device profile */ >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#define RTCAN_DEV_NAME "rtcan%d" >> +#define RTCAN_DRV_NAME "ADV-PCI-CAN" >> + >> +static char *adv_pci_board_name = "ADV-PCI"; >> + >> +MODULE_AUTHOR("Thierry Bultel "); >> +MODULE_DESCRIPTION("RTCAN board driver for Advantech PCI cards"); >> +MODULE_SUPPORTED_DEVICE("ADV-PCI card CAN controller"); >> +MODULE_LICENSE("GPL"); >> + >> +struct rtcan_adv_pci { >> + struct pci_dev *pci_dev; >> + struct rtcan_device *slave_dev; >> + void __iomem *conf_addr; >> + void __iomem *base_addr; >> +}; >> + >> +/* >> + * According to the datasheet, >> + * internal clock is 1/2 of the external oscillator frequency >> + * which is 16 MHz >> + */ >> +#define ADV_PCI_CAN_CLOCK (16000000 / 2) >> + >> +/* >> + * Output control register >> + Depends on the board configuration >> + */ >> + >> +#define ADV_PCI_OCR (SJA_OCR_MODE_NORMAL | \ >> + SJA_OCR_TX0_PUSHPULL | \ >> + SJA_OCR_TX1_PUSHPULL | \ >> + SJA_OCR_TX1_INVERT) >> + > > Less tabs please... > >> +/* >> + * In the CDR register, you should set CBP to 1. >> + */ >> +#define ADV_PCI_CDR (SJA_CDR_CBP | SJA_CDR_CAN_MODE) >> + >> + >> +#define ADV_PCI_VENDOR_ID 0x13fe >> + >> +#define CHANNEL_SINGLE 0 /* this is a single channel device */ >> +#define CHANNEL_MASTER 1 /* multi channel device, this device is master */ >> +#define CHANNEL_SLAVE 2 /* multi channel device, this is slave */ >> + >> +#define ADV_PCI_DEVICE(device_id)\ >> + { ADV_PCI_VENDOR_ID, device_id, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 } >> + >> +static DEFINE_PCI_DEVICE_TABLE(adv_pci_tbl) = { >> + ADV_PCI_DEVICE(0x1680), >> + ADV_PCI_DEVICE(0x3680), >> + ADV_PCI_DEVICE(0x2052), >> + ADV_PCI_DEVICE(0x1681), >> + ADV_PCI_DEVICE(0xc001), >> + ADV_PCI_DEVICE(0xc002), >> + ADV_PCI_DEVICE(0xc004), >> + ADV_PCI_DEVICE(0xc101), >> + ADV_PCI_DEVICE(0xc102), >> + ADV_PCI_DEVICE(0xc104), >> + /* required last entry */ >> + { } >> +}; >> + >> +MODULE_DEVICE_TABLE(pci, adv_pci_tbl); >> + >> +static u8 rtcan_adv_pci_read_reg(struct rtcan_device *dev, int port) >> +{ >> + struct rtcan_adv_pci *board = (struct rtcan_adv_pci *)dev->board_priv; >> + return ioread8(board->base_addr + port); >> +} >> + >> +static void rtcan_adv_pci_write_reg(struct rtcan_device *dev, int port, u8 data) >> +{ >> + struct rtcan_adv_pci *board = (struct rtcan_adv_pci *)dev->board_priv; > > Add an empty line here please. > >> + iowrite8(data, board->base_addr + port); >> +} >> + >> +static void rtcan_adv_pci_del_chan( >> + struct pci_dev *pdev, >> + struct rtcan_device *dev) >> +{ >> + struct rtcan_adv_pci *board; >> + >> + if (!dev) >> + return; >> + >> + board = (struct rtcan_adv_pci *)dev->board_priv; >> + >> + rtcan_sja1000_unregister(dev); >> + >> + pci_iounmap(pdev, board->base_addr); >> + >> + rtcan_dev_free(dev); >> +} >> + >> + >> +static int rtcan_adv_pci_add_chan( >> + struct pci_dev *pdev, >> + int channel, > > Please don't use tabs for alignment. Just one space if fine. > >> + unsigned int bar, >> + unsigned int offset, >> + struct rtcan_device **master_dev) >> +{ >> + struct rtcan_device *dev; >> + struct rtcan_sja1000 *chip; >> + struct rtcan_adv_pci *board; >> + void __iomem *base_addr; >> + int ret; >> + >> + dev = rtcan_dev_alloc(sizeof(struct rtcan_sja1000), >> + sizeof(struct rtcan_adv_pci)); > > Less tabs! > >> + if (dev == NULL) >> + return -ENOMEM; >> + >> + chip = (struct rtcan_sja1000 *)dev->priv; >> + board = (struct rtcan_adv_pci *)dev->board_priv; >> + >> + base_addr = pci_iomap(pdev, bar, ADV_PCI_BASE_SIZE) + offset; >> + >> + board->pci_dev = pdev; >> + board->conf_addr = NULL; >> + board->base_addr = base_addr; >> + >> + if (channel == CHANNEL_SLAVE) { >> + struct rtcan_adv_pci *master_board = \ >> + (struct rtcan_adv_pci *)(*master_dev)->board_priv; >> + master_board->slave_dev = dev; >> + } >> + >> + dev->board_name = adv_pci_board_name; >> + >> + chip->read_reg = rtcan_adv_pci_read_reg; >> + chip->write_reg = rtcan_adv_pci_write_reg; >> + >> + /* Clock frequency in Hz */ >> + dev->can_sys_clock = ADV_PCI_CAN_CLOCK; >> + >> + /* Output control register */ >> + chip->ocr = ADV_PCI_OCR; >> + >> + /* Clock divider register */ >> + chip->cdr = ADV_PCI_CDR; >> + >> + strncpy(dev->name, RTCAN_DEV_NAME, IFNAMSIZ); >> + >> + /* Make sure SJA1000 is in reset mode */ > > Tabs, please! > >> + chip->write_reg(dev, SJA_MOD, SJA_MOD_RM); >> + /* Set PeliCAN mode */ >> + chip->write_reg(dev, SJA_CDR, SJA_CDR_CAN_MODE); >> + >> + /* check if mode is set */ >> + ret = chip->read_reg(dev, SJA_CDR); >> + >> + if (ret != SJA_CDR_CAN_MODE) >> + return -EIO; >> + >> + /* Register and setup interrupt handling */ >> + chip->irq_flags = RTDM_IRQTYPE_SHARED; >> + chip->irq_num = pdev->irq; > > See my comment about alignment above. > >> + >> + RTCAN_DBG("%s: base_addr=%p conf_addr=%p irq=%d ocr=%#x cdr=%#x\n", >> + RTCAN_DRV_NAME, board->base_addr, board->conf_addr, >> + chip->irq_num, chip->ocr, chip->cdr); > > Tabs plus spaces? Please use a consistant indention style. > >> + >> + /* Register SJA1000 device */ >> + ret = rtcan_sja1000_register(dev); >> + > > Remove this empty line please! > >> + if (ret) { >> + printk(KERN_ERR "ERROR %d while trying to register SJA1000 device!\n", >> + ret); >> + goto failure; >> + } >> + >> + if (channel != CHANNEL_SLAVE) >> + *master_dev = dev; >> + >> + return 0; >> + >> +failure: >> + rtcan_dev_free(dev); >> + >> + return ret; >> +} >> + >> +static int __devinit adv_pci_init_one( >> + struct pci_dev *pdev, > > Should go one line up. This style is not used in Linux! Also true for > the functions above. > >> + const struct pci_device_id *ent) >> +{ >> + int ret, channel; >> + unsigned int nb_ports = 0; >> + unsigned int bar = 0; >> + unsigned int bar_flag = 0; >> + unsigned int offset = 0; >> + unsigned int ix; >> + >> + struct rtcan_device *master_dev = NULL; >> + >> + dev_info(&pdev->dev, "RTCAN Registering card"); >> + >> + ret = pci_enable_device(pdev); >> + if (ret) >> + goto failure; >> + >> + dev_info(&pdev->dev, "RTCAN Detected Advantech PCI card at slot #%i\n", >> + PCI_SLOT(pdev->devfn)); >> + >> + ret = pci_request_regions(pdev, RTCAN_DRV_NAME); >> + if (ret) >> + goto failure; >> + >> + switch (pdev->device) { >> + case 0xc001: >> + case 0xc002: >> + case 0xc004: >> + case 0xc101: >> + case 0xc102: >> + case 0xc104: >> + nb_ports = pdev->device & 0x7; >> + offset = 0x100; >> + bar = 0; >> + break; >> + case 0x1680: >> + case 0x2052: >> + nb_ports = 2; >> + bar = 2; > > No tabs. Just one space before and after "="! > >> + bar_flag = 1; >> + break; >> + case 0x1681: >> + nb_ports = 1; >> + bar = 2; >> + bar_flag = 1; >> + break; >> + default: > > Is this an error case? > >> + break; >> + } >> + >> + if (nb_ports > 1) >> + channel = CHANNEL_MASTER; >> + else >> + channel = CHANNEL_SINGLE; >> + >> + RTCAN_DBG("%s: Initializing device %04x:%04x:%04x\n",\ >> + RTCAN_DRV_NAME, \ >> + pdev->vendor, \ >> + pdev->device, \ >> + pdev->subsystem_device); > > You do not need the backslashes! > >> + ret = rtcan_adv_pci_add_chan(pdev, channel, bar, offset, &master_dev); >> + if (ret) >> + goto failure_iounmap; >> + >> + /* register slave channel, if any */ >> + >> + for (ix = 1; ix < nb_ports; ix++) { >> + ret = rtcan_adv_pci_add_chan(pdev, >> + CHANNEL_SLAVE, >> + bar + (bar_flag ? ix : 0), >> + offset*ix, >> + &master_dev); > > Too much tabs! Spaces before and after "*"! > >> + if (ret) >> + goto failure_iounmap; >> + } >> + >> + pci_set_drvdata(pdev, master_dev); >> + >> + return 0; >> + >> +failure_iounmap: >> + >> + if (master_dev) >> + rtcan_adv_pci_del_chan(pdev, master_dev); >> + >> + pci_release_regions(pdev); >> + >> +failure: >> + return ret; >> +} >> + >> +static void __devexit adv_pci_remove_one(struct pci_dev *pdev) >> +{ >> + struct rtcan_device *dev = pci_get_drvdata(pdev); >> + struct rtcan_adv_pci *board = (struct rtcan_adv_pci *)dev->board_priv; >> + >> + if (board->slave_dev) >> + rtcan_adv_pci_del_chan(pdev, board->slave_dev); >> + >> + rtcan_adv_pci_del_chan(pdev, dev); >> + >> + pci_release_regions(pdev); >> + pci_disable_device(pdev); >> + pci_set_drvdata(pdev, NULL); >> +} >> + >> +static struct pci_driver rtcan_adv_pci_driver = { >> + .name = RTCAN_DRV_NAME, >> + .id_table = adv_pci_tbl, >> + .probe = adv_pci_init_one, >> + .remove = __devexit_p(adv_pci_remove_one), > > Don't align with tabs, just use one space before and after "=". > >> +}; >> + >> +static int __init rtcan_adv_pci_init(void) >> +{ >> + return pci_register_driver(&rtcan_adv_pci_driver); >> +} >> + >> + > > Remove one empty line. > >> +static void __exit rtcan_adv_pci_exit(void) >> +{ >> + pci_unregister_driver(&rtcan_adv_pci_driver); >> +} >> + >> + > Ditto. > >> +module_init(rtcan_adv_pci_init); >> +module_exit(rtcan_adv_pci_exit); > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-can" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > >