Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752340AbaABSiU (ORCPT ); Thu, 2 Jan 2014 13:38:20 -0500 Received: from p3plex2out01.prod.phx3.secureserver.net ([184.168.131.12]:47355 "EHLO p3plex2out01.prod.phx3.secureserver.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751994AbaABSiS convert rfc822-to-8bit (ORCPT ); Thu, 2 Jan 2014 13:38:18 -0500 From: Hartley Sweeten To: Rostislav Lisovy , Ian Abbott , "Greg Kroah-Hartman" , "linux-kernel@vger.kernel.org" , "devel@driverdev.osuosl.org" CC: "pisa@cmp.felk.cvut.cz" Subject: RE: [PATCH] comedi: Humusoft MF634 and MF624 DAQ cards driver Thread-Topic: [PATCH] comedi: Humusoft MF634 and MF624 DAQ cards driver Thread-Index: AQHPBcjGqFwDBZPZhEODpNGWx9LkB5pt+0MAgAPCqeA= Date: Thu, 2 Jan 2014 18:38:16 +0000 Message-ID: References: <1388453796-17088-1-git-send-email-lisovy@gmail.com> <1388453796-17088-2-git-send-email-lisovy@gmail.com> In-Reply-To: <1388453796-17088-2-git-send-email-lisovy@gmail.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [184.183.19.121] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 15046 Lines: 522 On Monday, December 30, 2013 6:37 PM, Rostislav Lisovy wrote: > > create mode 100644 drivers/staging/comedi/drivers/mf6x4.c Hello Rostislav, As pointed out by Dan Carpenter, you need to add a change log and Signed-off-by lines to this patch. Overall this looks pretty good. Comments below. > diff --git a/drivers/staging/comedi/Kconfig b/drivers/staging/comedi/Kconfig > index bfa27e7..89e25b4 100644 > --- a/drivers/staging/comedi/Kconfig > +++ b/drivers/staging/comedi/Kconfig > @@ -884,6 +884,12 @@ config COMEDI_GSC_HPDI > To compile this driver as a module, choose M here: the module will be > called gsc_hpdi. > > +config COMEDI_MF6X4 > + tristate "Humusoft MF634 and MF624 DAQ Card support" > + ---help--- > + This driver supports both Humusoft MF634 and MF624 Data acquisition > + cards. The legacy Humusoft MF614 card is not supported. > + > config COMEDI_ICP_MULTI > tristate "Inova ICP_MULTI support" > ---help--- > diff --git a/drivers/staging/comedi/drivers/Makefile b/drivers/staging/comedi/drivers/Makefile > index 94cbd26..9e979a9 100644 > --- a/drivers/staging/comedi/drivers/Makefile > +++ b/drivers/staging/comedi/drivers/Makefile > @@ -110,6 +110,7 @@ obj-$(CONFIG_COMEDI_NI_PCIMIO) += ni_pcimio.o > obj-$(CONFIG_COMEDI_RTD520) += rtd520.o > obj-$(CONFIG_COMEDI_S626) += s626.o > obj-$(CONFIG_COMEDI_SSV_DNP) += ssv_dnp.o > +obj-$(CONFIG_COMEDI_MF6X4) += mf6x4.o > > # Comedi PCMCIA drivers > obj-$(CONFIG_COMEDI_CB_DAS16_CS) += cb_das16_cs.o > diff --git a/drivers/staging/comedi/drivers/mf6x4.c b/drivers/staging/comedi/drivers/mf6x4.c > new file mode 100644 > index 0000000..46c7ce5 > --- /dev/null > +++ b/drivers/staging/comedi/drivers/mf6x4.c > @@ -0,0 +1,368 @@ > +/* > + * comedi/drivers/mf6x4.c > + * Driver for Humusoft MF634 and MF624 Data acquisition cards > + * > + * COMEDI - Linux Control and Measurement Device Interface > + * Copyright (C) 2000 David A. Schleef > + * > + * 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. > + */ > +/* > + * Driver: mf6x4 > + * Description: Humusoft MF634 and MF624 Data acquisition card driver > + * Devices: Humusoft MF634, Humusoft MF624 > + * Author: Rostislav Lisovy > + * Status: works > + * Updated: > + * Configuration Options: none > + */ > + > +#include > +#include > +#include "../comedidev.h" > + > +/* PCI Vendor ID, Device ID */ > +#define PCI_VENDOR_ID_HUMUSOFT 0x186c > +#define PCI_DEVICE_ID_MF624 0x0624 > +#define PCI_DEVICE_ID_MF634 0x0634 Please put the PCI_VENDOR_ID_* in comedidev.h with the other PCI Vendor IDs. Also, remove the PCI_DEVICE_ID_* defines and just open code the values in the pci_device_id table. The mf6x4_boardid enums provide adequate documentation. > + > +/* Registers present in BAR0 memory region */ > +#define MF624_GPIOC_reg 0x54 > + > +#define MF6X4_GPIOC_EOLC /* End Of Last Conversion */ (1 << 17) > +#define MF6X4_GPIOC_LDAC /* Load DACs */ (1 << 23) > +#define MF6X4_GPIOC_DACEN (1 << 26) > + > +/* BAR1 registers */ > +#define MF6X4_DIN_reg 0x10 > +#define MF6X4_DIN_mask 0xff > +#define MF6X4_DOUT_reg 0x10 > +#define MF6X4_DOUT_mask 0xff > + > +#define MF6X4_ADSTART_reg 0x20 > +#define MF6X4_ADDATA_reg 0x00 > +#define MF6X4_ADDATA_mask 0x3fff > +#define MF6X4_ADCTRL_reg 0x00 > +#define MF6X4_ADCTRL_mask 0xff > + > +#define MF6X4_DA0_reg 0x20 > +#define MF6X4_DA1_reg 0x22 > +#define MF6X4_DA2_reg 0x24 > +#define MF6X4_DA3_reg 0x26 > +#define MF6X4_DA4_reg 0x28 > +#define MF6X4_DA5_reg 0x2a > +#define MF6X4_DA6_reg 0x2c > +#define MF6X4_DA7_reg 0x2e > +#define MF6X4_DA_mask 0x3fff > +#define MF6X4_DAC_CHANN_CNT 8 > + > +/* BAR2 registers */ > +#define MF634_GPIOC_reg 0x68 Please rename these CamelCase defines (i.e. s/_reg/_REG). > + > +/* Make a fault-tolerant mapping from DAC cahnnel id obtained as > +an int to real HW-dependent offset value */ Please follow the CodingStyle for all multi-line comments in this driver. > +static unsigned int mf6x4_dac_channels[MF6X4_DAC_CHANN_CNT] = { > + [0] = MF6X4_DA0_reg, > + [1] = MF6X4_DA1_reg, > + [2] = MF6X4_DA2_reg, > + [3] = MF6X4_DA3_reg, > + [4] = MF6X4_DA4_reg, > + [5] = MF6X4_DA5_reg, > + [6] = MF6X4_DA6_reg, > + [7] = MF6X4_DA7_reg, > +}; Is this static global array really needed? How about just, #define MF6X4_DA_REG(x) (0x20 + ((x) * 2)) > + > +enum mf6x4_boardid { > + BOARD_MF634, > + BOARD_MF624, > +}; > + > +struct mf6x4_board { > + const char *name; > + unsigned int bar_nums[3]; /* We need to keep track of the > + order of BARs used by the cards */ > +}; > + > +static const struct mf6x4_board mf6x4_boards[] = { > + [BOARD_MF634] = { > + .name = "mf634", > + .bar_nums = {0, 2, 3}, > + }, > + [BOARD_MF624] = { > + .name = "mf624", > + .bar_nums = {0, 2, 4}, > + }, > +}; > + > +struct mf6x4_private { > + struct pci_dev *pci_dev; This private data member does not appear to be used. Please remove it. > + > + /* Documentation for both MF634 and MF624 describes registers > + present in BAR0, 1 and 2 regions. > + The real (i.e. in HW) BAR numbers are different for MF624 and > + MF634 yet we will call them 0, 1, 2 */ > + void __iomem *bar0_mem; > + void __iomem *bar1_mem; > + void __iomem *bar2_mem; > + > + void __iomem *gpioc_reg; /* This configuration register has the same > + content for the both cards however it liess in different BARs > + on different offsets -- this helps to make the access easier */ > +}; > + > +static int mf6x4_dio_insn_bits(struct comedi_device *dev, > + struct comedi_subdevice *s, > + struct comedi_insn *insn, unsigned int *data) > +{ > + struct mf6x4_private *devpriv = dev->private; > + > + switch (s->type) { > + case COMEDI_SUBD_DI: /* DIN */ > + data[1] = ioread16(devpriv->bar1_mem + MF6X4_DIN_reg) & MF6X4_DIN_mask; > + break; > + > + case COMEDI_SUBD_DO: /* DOUT */ > + /* data[0] -- mask > + data[1] -- actual value */ > + if (data[0]) { > + s->state &= ~data[0]; > + s->state |= (data[0] & data[1]); Please use comedi_dio_update_state() to handle this boilerplate code. > + > + iowrite16(s->state & MF6X4_DOUT_mask, > + devpriv->bar1_mem + MF6X4_DOUT_reg); > + > + data[1] = s->state; > + } > + break; > + } > + > + return insn->n; > +} Please split this function into a mf6x4_di_insn_bits() and mf6x4_do_insn_bits(). That will remove an indent level and make the usage a bit clearer. > + > +static int mf6x4_ai_insn_read(struct comedi_device *dev, > + struct comedi_subdevice *s, > + struct comedi_insn *insn, unsigned int *data) > +{ > + struct mf6x4_private *devpriv = dev->private; > + unsigned int chan = CR_CHAN(insn->chanspec); > + unsigned int timeout = 100; > + unsigned int eolc; > + unsigned int n; > + unsigned int i; > + int d; > + > + /* Set the ADC channel number in the scan list */ > + iowrite16((1 << chan) & MF6X4_ADCTRL_mask, > + devpriv->bar1_mem + MF6X4_ADCTRL_reg); > + > + for (n = 0; n < insn->n; n++) { > + /* Trigger ADC conversion by reading ADSTART */ > + ioread16(devpriv->bar1_mem + MF6X4_ADSTART_reg); > + > + /* Wait until the conversion finishes or times out */ > + for (i = 0; i < timeout; i++) { > + eolc = ioread32(devpriv->gpioc_reg) & MF6X4_GPIOC_EOLC; > + > + if (!eolc) > + break; > + } > + if (i == timeout) { > + dev_warn(dev->class_dev, "ADC conversion timed out\n"); > + return -ETIMEDOUT; > + } Please factor out the timeout loop as a helper function. See pcl711.c for an example. > + > + /* Read the actual value */ > + d = ioread16(devpriv->bar1_mem + MF6X4_ADDATA_reg); > + d &= MF6X4_ADDATA_mask; This could just be: d &= s->maxdata; > + data[n] = d; > + } > + > + iowrite16(0x0, devpriv->bar1_mem + MF6X4_ADCTRL_reg); > + > + return n; return insn->n; > +} > + > +static int mf6x4_ao_insn_write(struct comedi_device *dev, > + struct comedi_subdevice *s, > + struct comedi_insn *insn, unsigned int *data) > +{ > + struct mf6x4_private *devpriv = dev->private; > + int chan = CR_CHAN(insn->chanspec); > + uint32_t gpioc; > + int i; > + > + chan = (chan < MF6X4_DAC_CHANN_CNT) ? chan : 0; > + > + /* Enable instantaneous update of converters outputs + Enable DACs */ > + gpioc = ioread32(devpriv->gpioc_reg); > + iowrite32((gpioc & ~MF6X4_GPIOC_LDAC) | MF6X4_GPIOC_DACEN, devpriv->gpioc_reg); > + > + /* Writing a list of values to an AO channel is probably not > + very useful, but that's how the interface is defined. */ Please remove the comment above. > + for (i = 0; i < insn->n; i++) { > + iowrite16(data[i] & MF6X4_DA_mask, > + devpriv->bar1_mem + mf6x4_dac_channels[chan]); > + } You should provide an (*insn_read) for the analog output subdevice. The last data written to the channel can be cached in the private data. Something like: unsigned int val = devpriv->ao_readback[chan]; for (i = 0; i < insn->n; i++) { val = data[i]; val &= s->maxdata; iowrite16(val, devpriv->bar1_mem + MF6X4_DA_REG(chan)); } devpriv->ao_readback[chan] = val; > + > + return i; return insn->n; > +} > + > +static int mf6x4_auto_attach(struct comedi_device *dev, unsigned long context) > +{ > + struct pci_dev *pcidev = comedi_to_pci_dev(dev); > + const struct mf6x4_board *board = NULL; > + struct mf6x4_private *devpriv; > + struct comedi_subdevice *s; > + int ret; > + > + if (context < ARRAY_SIZE(mf6x4_boards)) > + board = &mf6x4_boards[context]; > + else > + return -ENODEV; > + > + dev->board_ptr = board; > + dev->board_name = board->name; > + > + ret = comedi_pci_enable(dev); /* pci_enable_device(), pci_request_regions() */ The comment is not needed. > + if (ret) > + return ret; > + > + devpriv = comedi_alloc_devpriv(dev, sizeof(*devpriv)); > + if (!devpriv) > + return -ENOMEM; > + > + devpriv->bar0_mem = pci_ioremap_bar(pcidev, board->bar_nums[0]); > + if (!devpriv->bar0_mem) { > + dev_err(dev->class_dev, "Failed to remap Device configuration BAR\n"); > + ret = -ENODEV; Just return the error here and below. > + goto out; > + } > + > + devpriv->bar1_mem = pci_ioremap_bar(pcidev, board->bar_nums[1]); > + if (!devpriv->bar1_mem) { > + dev_err(dev->class_dev, "Failed to remap ADC, DAC, DIO BAR\n"); > + ret = -ENODEV; > + goto out; > + } > + > + devpriv->bar2_mem = pci_ioremap_bar(pcidev, board->bar_nums[2]); > + if (!devpriv->bar2_mem) { > + dev_err(dev->class_dev, "Failed to remap Counter/timer BAR\n"); > + ret = -ENODEV; > + goto out; > + } > + > + if (board == &mf6x4_boards[BOARD_MF634]) { > + devpriv->gpioc_reg = devpriv->bar2_mem + MF634_GPIOC_reg; > + } else if (board == &mf6x4_boards[BOARD_MF624]) { > + devpriv->gpioc_reg = devpriv->bar0_mem + MF624_GPIOC_reg; > + } else { /* Definitely should not happen */ > + devpriv->gpioc_reg = NULL; > + } The curly braces are not needed. Also, since the else case cannot happen just remove it. > + > + ret = comedi_alloc_subdevices(dev, 4); > + if (ret) > + goto out; > + > + /* ADC */ > + s = &dev->subdevices[0]; > + s->type = COMEDI_SUBD_AI; > + s->subdev_flags = SDF_READABLE | SDF_GROUND; > + s->n_chan = 8; > + s->range_table = &range_bipolar10; > + s->maxdata = (1 << 14) - 1; /* 14 bits ADC */ s->maxdata = 0x3fff; > + s->insn_read = mf6x4_ai_insn_read; > + > + /* DAC */ > + s = &dev->subdevices[1]; > + s->type = COMEDI_SUBD_AO; > + s->subdev_flags = SDF_WRITABLE; > + s->n_chan = 8; > + s->range_table = &range_bipolar10; > + s->maxdata = (1 << 14) - 1; /* 14 bits DAC */ s->maxdata = 0x3fff; > + s->insn_write = mf6x4_ao_insn_write; > + > + /* DIN */ > + s = &dev->subdevices[2]; > + s->type = COMEDI_SUBD_DI; > + s->subdev_flags = SDF_READABLE; > + s->n_chan = 8; > + s->range_table = &range_digital; > + s->maxdata = 1; > + s->insn_bits = mf6x4_dio_insn_bits; > + > + /* DOUT */ > + s = &dev->subdevices[3]; > + s->type = COMEDI_SUBD_DO; > + s->subdev_flags = SDF_WRITABLE; > + s->n_chan = 8; > + s->range_table = &range_digital; > + s->maxdata = 1; > + s->insn_bits = mf6x4_dio_insn_bits; > + Please add some whitespace to the subdevice init. Nitpick, please reorder the subdevice init as: s = ... s->type = ... s->subdev_flags = ... s->n_chan = ... s->maxdata = ... s-> range_table = ... s->insn... = ... > + return 0; > + > +out: > + /* dev->private is freed automatically */ > + return ret; This should be removed after changing the code above to just return the error. > +} > + > +/* > + * _detach is called to deconfigure a device. It should deallocate resources. > + * This function is also called when _attach() fails, so it should be careful > + * not to release resources that were not necessarily allocated by _attach(). > + * dev->private and dev->subdevices are deallocated automatically by the core. > + */ Please remove this comment. > +static void mf6x4_detach(struct comedi_device *dev) > +{ > + struct mf6x4_private *devpriv = dev->private; > + > + if (devpriv->bar0_mem) > + iounmap(devpriv->bar0_mem); > + if (devpriv->bar1_mem) > + iounmap(devpriv->bar1_mem); > + if (devpriv->bar2_mem) > + iounmap(devpriv->bar2_mem); > + > + comedi_pci_disable(dev); /* pci_release_regions(), pci_disable_device() */ > +} > + > +static struct comedi_driver mf6x4_driver = { > + .driver_name = "mf6x4", > + .module = THIS_MODULE, > + .auto_attach = mf6x4_auto_attach, > + .detach = mf6x4_detach, > +}; > + > +static int mf6x4_pci_probe(struct pci_dev *dev, const struct pci_device_id *id) > +{ > + return comedi_pci_auto_config(dev, &mf6x4_driver, id->driver_data); > +} > + > +static const struct pci_device_id mf6x4_pci_table[] = { > + { PCI_VDEVICE(HUMUSOFT, PCI_DEVICE_ID_MF634), BOARD_MF634 }, > + { PCI_VDEVICE(HUMUSOFT, PCI_DEVICE_ID_MF624), BOARD_MF624 }, > + { 0 } > +}; > +MODULE_DEVICE_TABLE(pci, mf6x4_pci_table); > + > +static struct pci_driver mf6x4_pci_driver = { > + .name = "mf6x4", > + .id_table = mf6x4_pci_table, > + .probe = mf6x4_pci_probe, > + .remove = comedi_pci_auto_unconfig, > +}; > + > +module_comedi_pci_driver(mf6x4_driver, mf6x4_pci_driver); > + > +MODULE_AUTHOR("Rostislav Lisovy "); > +MODULE_DESCRIPTION("Comedi MF634 and MF624 DAQ cards driver"); > +MODULE_LICENSE("GPL"); > -- > 1.8.3.2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/