* [PATCH] comedi: Humusoft MF634 and MF624 DAQ cards driver
@ 2013-12-31 1:36 Rostislav Lisovy
2013-12-31 1:36 ` Rostislav Lisovy
0 siblings, 1 reply; 14+ messages in thread
From: Rostislav Lisovy @ 2013-12-31 1:36 UTC (permalink / raw)
To: Ian Abbott, H Hartley Sweeten, Greg Kroah-Hartman, linux-kernel, devel
Cc: pisa, Rostislav Lisovy
This patch adds Comedi driver for Humusoft MF634 (PCIe) and MF624 (PCI)
data acquisition cards. The legacy card Humusoft MF614 is not supported.
More info about the cards may be found at http://humusoft.cz/produkty/datacq/
The driver was tested with both cards. Everything seems to work properly.
Just the basic functionality of the card (DIO, ADC, DAC) is supported by
this driver. I hope I will be able to add some more functionality soon.
Rostislav Lisovy (1):
comedi: Humusoft MF634 and MF624 DAQ cards driver
drivers/staging/comedi/Kconfig | 6 +
drivers/staging/comedi/drivers/Makefile | 1 +
drivers/staging/comedi/drivers/mf6x4.c | 368 ++++++++++++++++++++++++++++++++
3 files changed, 375 insertions(+)
create mode 100644 drivers/staging/comedi/drivers/mf6x4.c
--
1.8.3.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] comedi: Humusoft MF634 and MF624 DAQ cards driver
2013-12-31 1:36 [PATCH] comedi: Humusoft MF634 and MF624 DAQ cards driver Rostislav Lisovy
@ 2013-12-31 1:36 ` Rostislav Lisovy
2013-12-31 2:31 ` Dan Carpenter
2014-01-02 18:38 ` Hartley Sweeten
0 siblings, 2 replies; 14+ messages in thread
From: Rostislav Lisovy @ 2013-12-31 1:36 UTC (permalink / raw)
To: Ian Abbott, H Hartley Sweeten, Greg Kroah-Hartman, linux-kernel, devel
Cc: pisa, Rostislav Lisovy
create mode 100644 drivers/staging/comedi/drivers/mf6x4.c
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 <ds@schleef.org>
+ *
+ * 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 <lisovy@gmail.com>
+ * Status: works
+ * Updated:
+ * Configuration Options: none
+ */
+
+#include <linux/module.h>
+#include <linux/pci.h>
+#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
+
+/* 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
+
+/* Make a fault-tolerant mapping from DAC cahnnel id obtained as
+an int to real HW-dependent offset value */
+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,
+};
+
+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;
+
+ /* 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]);
+
+ iowrite16(s->state & MF6X4_DOUT_mask,
+ devpriv->bar1_mem + MF6X4_DOUT_reg);
+
+ data[1] = s->state;
+ }
+ break;
+ }
+
+ return insn->n;
+}
+
+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;
+ }
+
+ /* Read the actual value */
+ d = ioread16(devpriv->bar1_mem + MF6X4_ADDATA_reg);
+ d &= MF6X4_ADDATA_mask;
+ data[n] = d;
+ }
+
+ iowrite16(0x0, devpriv->bar1_mem + MF6X4_ADCTRL_reg);
+
+ return 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. */
+ for (i = 0; i < insn->n; i++) {
+ iowrite16(data[i] & MF6X4_DA_mask,
+ devpriv->bar1_mem + mf6x4_dac_channels[chan]);
+ }
+
+ return i;
+}
+
+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() */
+ 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;
+ 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;
+ }
+
+ 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->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->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;
+
+ return 0;
+
+out:
+ /* dev->private is freed automatically */
+ return ret;
+}
+
+/*
+ * _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.
+ */
+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 <lisovy@gmail.com>");
+MODULE_DESCRIPTION("Comedi MF634 and MF624 DAQ cards driver");
+MODULE_LICENSE("GPL");
--
1.8.3.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Humusoft MF634 and MF624 DAQ cards driver
2013-12-31 1:36 ` Rostislav Lisovy
@ 2013-12-31 2:31 ` Dan Carpenter
2014-01-02 18:38 ` Hartley Sweeten
1 sibling, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2013-12-31 2:31 UTC (permalink / raw)
To: Rostislav Lisovy
Cc: Ian Abbott, H Hartley Sweeten, Greg Kroah-Hartman, linux-kernel,
devel, pisa
These days comedi drivers are looking really nice. :) You will need to
resend because it's missing a change log and a Signed-off-by line. Wait
for Ian or Hartley to comment before resending. I had a few minor style
nits mentioned inline below. Run the patch through
`scripts/checkpatch.pl --strict`.
On Tue, Dec 31, 2013 at 02:36:36AM +0100, Rostislav Lisovy wrote:
>
> create mode 100644 drivers/staging/comedi/drivers/mf6x4.c
>
> 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 <ds@schleef.org>
> + *
> + * 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 <lisovy@gmail.com>
> + * Status: works
> + * Updated:
> + * Configuration Options: none
> + */
> +
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#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
> +
> +/* 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
> +
> +/* Make a fault-tolerant mapping from DAC cahnnel id obtained as
> +an int to real HW-dependent offset value */
> +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,
> +};
> +
> +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;
> +
> + /* 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 */
Use kernel commenting style described in Documentation/CodingStyle.
> + 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]);
> +
> + iowrite16(s->state & MF6X4_DOUT_mask,
> + devpriv->bar1_mem + MF6X4_DOUT_reg);
> +
> + data[1] = s->state;
> + }
> + break;
> + }
> +
> + return insn->n;
> +}
> +
> +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;
Just put 100 inline.
> + 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;
> + }
> +
> + /* Read the actual value */
> + d = ioread16(devpriv->bar1_mem + MF6X4_ADDATA_reg);
> + d &= MF6X4_ADDATA_mask;
> + data[n] = d;
> + }
> +
> + iowrite16(0x0, devpriv->bar1_mem + MF6X4_ADCTRL_reg);
> +
> + return 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;
Can this condition actually happen?
> +
> + /* 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. */
> + for (i = 0; i < insn->n; i++) {
> + iowrite16(data[i] & MF6X4_DA_mask,
> + devpriv->bar1_mem + mf6x4_dac_channels[chan]);
> + }
> +
> + return i;
> +}
> +
> +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() */
> + 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;
> + goto out;
Just return directly.
> + }
> +
> + 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;
> + }
No curly braces needed.
> +
> + 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->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->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;
> +
> + return 0;
> +
> +out:
> + /* dev->private is freed automatically */
> + return ret;
> +}
> +
> +/*
> + * _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.
> + */
> +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 <lisovy@gmail.com>");
> +MODULE_DESCRIPTION("Comedi MF634 and MF624 DAQ cards driver");
> +MODULE_LICENSE("GPL");
> --
> 1.8.3.2
>
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH] comedi: Humusoft MF634 and MF624 DAQ cards driver
2013-12-31 1:36 ` Rostislav Lisovy
2013-12-31 2:31 ` Dan Carpenter
@ 2014-01-02 18:38 ` Hartley Sweeten
2014-01-05 15:04 ` Rostislav Lisovy
2014-01-06 12:37 ` Ian Abbott
1 sibling, 2 replies; 14+ messages in thread
From: Hartley Sweeten @ 2014-01-02 18:38 UTC (permalink / raw)
To: Rostislav Lisovy, Ian Abbott, Greg Kroah-Hartman, linux-kernel, devel
Cc: pisa
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 <ds@schleef.org>
> + *
> + * 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 <lisovy@gmail.com>
> + * Status: works
> + * Updated:
> + * Configuration Options: none
> + */
> +
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#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 <lisovy@gmail.com>");
> +MODULE_DESCRIPTION("Comedi MF634 and MF624 DAQ cards driver");
> +MODULE_LICENSE("GPL");
> --
> 1.8.3.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Humusoft MF634 and MF624 DAQ cards driver
2014-01-02 18:38 ` Hartley Sweeten
@ 2014-01-05 15:04 ` Rostislav Lisovy
2014-01-05 18:18 ` Dan Carpenter
2014-01-06 12:37 ` Ian Abbott
1 sibling, 1 reply; 14+ messages in thread
From: Rostislav Lisovy @ 2014-01-05 15:04 UTC (permalink / raw)
To: Hartley Sweeten
Cc: Ian Abbott, Greg Kroah-Hartman, linux-kernel, devel, pisa, Dan Carpenter
Hello Hartley (and Dan);
Thank you for the review. I do agree with most of the things, however I
would like to explain/discuss a few of them...
On Thu, 2014-01-02 at 18:38 +0000, Hartley Sweeten wrote:
> 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.
I agree, the missing Signed-off-by is my mistake. I always thought that
the change log should explain the changes to previous version of the
same patch (when resending the patch). What is the reason to have a
change log in the first version of the patch (everything is a change)?
>
> 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 <ds@schleef.org>
> > + *
> > + * 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 <lisovy@gmail.com>
> > + * Status: works
> > + * Updated:
> > + * Configuration Options: none
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#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).
I would not call it CamelCase -- it is more like a suffix. The name is
thus composed of the prefix (MF6X4), the name (same as in the
documentation) and a suffix (saying if this is a register name or a
field in a register or mask, etc.) -- the reason why I use lower case is
that it helps readability.
>
> > +
> > +/* 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))
Not needed at all. I just wanted to make it very "fault-tolerant" but I
agree it is unnecessary overkill.
>
> > +
> > +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.
The reason I did it in this way is the comment next to the 'out' label.
For somebody not experienced with comedi drivers (like me or somebody
else reading the code in the future) the sequence of memory allocation
(or 'pci_ioremap_bar') followed by a 'return' statement looks quite
scary. Either I can write the comment to each return or do it with the
single point of exit.
>
> > + 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 <lisovy@gmail.com>");
> > +MODULE_DESCRIPTION("Comedi MF634 and MF624 DAQ cards driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 1.8.3.2
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Humusoft MF634 and MF624 DAQ cards driver
2014-01-05 15:04 ` Rostislav Lisovy
@ 2014-01-05 18:18 ` Dan Carpenter
0 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2014-01-05 18:18 UTC (permalink / raw)
To: Rostislav Lisovy
Cc: Hartley Sweeten, devel, Greg Kroah-Hartman, linux-kernel,
Ian Abbott, pisa
On Sun, Jan 05, 2014 at 04:04:04PM +0100, Rostislav Lisovy wrote:
> Hello Hartley (and Dan);
> Thank you for the review. I do agree with most of the things, however I
> would like to explain/discuss a few of them...
>
> On Thu, 2014-01-02 at 18:38 +0000, Hartley Sweeten wrote:
> > 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.
>
> I agree, the missing Signed-off-by is my mistake. I always thought that
> the change log should explain the changes to previous version of the
> same patch (when resending the patch). What is the reason to have a
> change log in the first version of the patch (everything is a change)?
Right now the change log is just "create mode 100644
drivers/staging/comedi/drivers/mf6x4.c" which is not English. Just
rephrase it into English and add the bit from the Kconfig file. It
doesn't have to be a novel.
> > > +/* BAR2 registers */
> > > +#define MF634_GPIOC_reg 0x68
> >
> > Please rename these CamelCase defines (i.e. s/_reg/_REG).
>
> I would not call it CamelCase -- it is more like a suffix. The name is
> thus composed of the prefix (MF6X4), the name (same as in the
> documentation) and a suffix (saying if this is a register name or a
> field in a register or mask, etc.) -- the reason why I use lower case is
> that it helps readability.
>
It's not really kernel style... And especially since we're going to
make this into a function. MF6X4_DA_reg() looks really bad to me.
> > > + 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.
>
> The reason I did it in this way is the comment next to the 'out' label.
> For somebody not experienced with comedi drivers (like me or somebody
> else reading the code in the future) the sequence of memory allocation
> (or 'pci_ioremap_bar') followed by a 'return' statement looks quite
> scary. Either I can write the comment to each return or do it with the
> single point of exit.
>
Doing the empty goto is annoying because you assume that a goto has a
point instead of just hopping to the bottom of the function for no
reason.
Looking at this code again, I bet Hartley meant to remove the dev_err()
and as well as the goto. Yep. Looking through pci_ioremap_bar() it
prints its own error messages so the dev_err() is redundant.
Comedi's cleanup routines confused me at first too. There should
definitely be a comment on this somewhere. I suspect that there is a
commented on this already but I wasn't able to find it. Maybe it
belongs in skel.c with a comment next to the struct comedi_driver
definition to "see skel.c". But We don't want these kind of "known
issue for someone after writing their first comedi driver" comments in
every .c file.
For the driver code we want it to be as little fluff as possible:
devpriv->bar0_mem = pci_ioremap_bar(pcidev, board->bar_nums[0]);
if (!devpriv->bar0_mem)
return -ENODEV;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Humusoft MF634 and MF624 DAQ cards driver
2014-01-02 18:38 ` Hartley Sweeten
2014-01-05 15:04 ` Rostislav Lisovy
@ 2014-01-06 12:37 ` Ian Abbott
1 sibling, 0 replies; 14+ messages in thread
From: Ian Abbott @ 2014-01-06 12:37 UTC (permalink / raw)
To: Hartley Sweeten, Rostislav Lisovy, Ian Abbott,
Greg Kroah-Hartman, linux-kernel, devel
Cc: pisa
On 2014-01-02 18:38, Hartley Sweeten wrote:
> On Monday, December 30, 2013 6:37 PM, Rostislav Lisovy wrote:
[snip]
>> +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.
I'll also add that the `data[1] = s->state;` should be done even if
data[0] is zero, i.e. it should be moved after the end of the `if`
statement.
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Humusoft MF634 and MF624 DAQ cards driver
2014-01-09 22:46 ` [PATCH] " Rostislav Lisovy
@ 2014-01-10 10:29 ` Ian Abbott
0 siblings, 0 replies; 14+ messages in thread
From: Ian Abbott @ 2014-01-10 10:29 UTC (permalink / raw)
To: Rostislav Lisovy, Ian Abbott, Hartley Sweeten, Dan Carpenter,
Greg Kroah-Hartman, linux-kernel, devel
Cc: pisa
On 2014-01-09 22:46, Rostislav Lisovy wrote:
> This patch adds Comedi driver for Humusoft MF634 (PCIe) and
> MF624 (PCI) data acquisition cards. The legacy card Humusoft
> MF614 is not supported. More info about the cards may be found
> at http://humusoft.cz/produkty/datacq/
> The driver was tested with both cards. Everything seems to work
> properly. Just the basic functionality of the card (DIO, ADC, DAC)
> is supported by this driver.
>
> Signed-off-by: Rostislav Lisovy <lisovy@gmail.com>
> ---
> create mode 100644 drivers/staging/comedi/drivers/mf6x4.c
>
> 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/comedidev.h b/drivers/staging/comedi/comedidev.h
> index 143be80..161bdd2 100644
> --- a/drivers/staging/comedi/comedidev.h
> +++ b/drivers/staging/comedi/comedidev.h
> @@ -410,6 +410,7 @@ void comedi_driver_unregister(struct comedi_driver *);
> #define PCI_VENDOR_ID_IOTECH 0x1616
> #define PCI_VENDOR_ID_CONTEC 0x1221
> #define PCI_VENDOR_ID_RTD 0x1435
> +#define PCI_VENDOR_ID_HUMUSOFT 0x186c
>
> struct pci_dev;
> struct pci_driver;
> 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..81b78e0
> --- /dev/null
> +++ b/drivers/staging/comedi/drivers/mf6x4.c
> @@ -0,0 +1,354 @@
> +/*
> + * 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 <ds@schleef.org>
> + *
> + * 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 <lisovy@gmail.com>
> + * Status: works
> + * Updated:
> + * Configuration Options: none
> + */
> +
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/delay.h>
> +#include "../comedidev.h"
> +
> +/* Registers present in BAR0 memory region */
> +#define MF624_GPIOC_R 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_R 0x10
> +#define MF6X4_DIN_M 0xff
> +#define MF6X4_DOUT_R 0x10
> +#define MF6X4_DOUT_M 0xff
> +
> +#define MF6X4_ADSTART_R 0x20
> +#define MF6X4_ADDATA_R 0x00
> +#define MF6X4_ADCTRL_R 0x00
> +#define MF6X4_ADCTRL_M 0xff
> +
> +#define MF6X4_DA0_R 0x20
> +#define MF6X4_DA1_R 0x22
> +#define MF6X4_DA2_R 0x24
> +#define MF6X4_DA3_R 0x26
> +#define MF6X4_DA4_R 0x28
> +#define MF6X4_DA5_R 0x2a
> +#define MF6X4_DA6_R 0x2c
> +#define MF6X4_DA7_R 0x2e
> +/* Map DAC cahnnel id to real HW-dependent offset value */
> +#define MF6X4_DAC_R(x) (0x20 + ((x) * 2))
> +#define MF6X4_DA_M 0x3fff
> +
> +/* BAR2 registers */
> +#define MF634_GPIOC_R 0x68
> +
> +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 {
> + /*
> + * 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;
> +
> + /*
> + * This configuration register has the same function and fields
> + * for both cards however it lies in different BARs on different
> + * offsets -- this variable makes the access easier
> + */
> + void __iomem *gpioc_R;
> +
> + /* DAC value cache -- used for insn_read function */
> + int ao_readback[8];
> +};
> +
> +static int mf6x4_di_insn_bits(struct comedi_device *dev,
> + struct comedi_subdevice *s,
> + struct comedi_insn *insn, unsigned int *data)
> +{
> + struct mf6x4_private *devpriv = dev->private;
> +
> + data[1] = ioread16(devpriv->bar1_mem + MF6X4_DIN_R) & MF6X4_DIN_M;
> +
> + return insn->n;
> +}
> +
> +static int mf6x4_do_insn_bits(struct comedi_device *dev,
> + struct comedi_subdevice *s,
> + struct comedi_insn *insn, unsigned int *data)
> +{
> + struct mf6x4_private *devpriv = dev->private;
> +
> + if (comedi_dio_update_state(s, data))
> + iowrite16(s->state & MF6X4_DOUT_M,
> + devpriv->bar1_mem + MF6X4_DOUT_R);
> +
> + data[1] = s->state;
> +
> + return insn->n;
> +}
> +
> +static int mf6x4_ai_wait_for_eoc(struct comedi_device *dev,
> + unsigned int timeout)
> +{
> + struct mf6x4_private *devpriv = dev->private;
> + unsigned int eolc;
> +
> + while (timeout--) {
> + eolc = ioread32(devpriv->gpioc_R) & MF6X4_GPIOC_EOLC;
> + if (eolc)
> + return 0;
> +
> + udelay(1);
> + }
> +
> + return -ETIME;
> +}
> +
> +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;
> + int chan = CR_CHAN(insn->chanspec);
> + int ret;
> + int i;
> + int d;
> +
> + /* Set the ADC channel number in the scan list */
> + iowrite16((1 << chan) & MF6X4_ADCTRL_M,
> + devpriv->bar1_mem + MF6X4_ADCTRL_R);
> +
> + for (i = 0; i < insn->n; i++) {
> + /* Trigger ADC conversion by reading ADSTART */
> + ioread16(devpriv->bar1_mem + MF6X4_ADSTART_R);
> +
> + ret = mf6x4_ai_wait_for_eoc(dev, 100);
> + if (ret)
> + return ret;
> +
> + /* Read the actual value */
> + d = ioread16(devpriv->bar1_mem + MF6X4_ADDATA_R);
> + d &= s->maxdata;
> + data[i] = d;
> + }
> +
> + iowrite16(0x0, devpriv->bar1_mem + MF6X4_ADCTRL_R);
> +
> + 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;
> + unsigned int chan = CR_CHAN(insn->chanspec);
> + uint32_t gpioc;
> + int i;
> +
> + /* Enable instantaneous update of converters outputs + Enable DACs */
> + gpioc = ioread32(devpriv->gpioc_R);
> + iowrite32((gpioc & ~MF6X4_GPIOC_LDAC) | MF6X4_GPIOC_DACEN,
> + devpriv->gpioc_R);
> +
> + for (i = 0; i < insn->n; i++) {
> + iowrite16(data[i] & MF6X4_DA_M,
> + devpriv->bar1_mem + MF6X4_DAC_R(chan));
> +
> + devpriv->ao_readback[chan] = data[i];
> + }
> +
> + return insn->n;
> +}
> +
> +static int mf6x4_ao_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);
> + int i;
> +
> + for (i = 0; i < insn->n; i++)
> + data[i] = devpriv->ao_readback[chan];
> +
> + 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);
> + 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)
> + return -ENODEV;
> +
> + devpriv->bar1_mem = pci_ioremap_bar(pcidev, board->bar_nums[1]);
> + if (!devpriv->bar1_mem)
> + return -ENODEV;
> +
> + devpriv->bar2_mem = pci_ioremap_bar(pcidev, board->bar_nums[2]);
> + if (!devpriv->bar2_mem)
> + return -ENODEV;
> +
> + if (board == &mf6x4_boards[BOARD_MF634])
> + devpriv->gpioc_R = devpriv->bar2_mem + MF634_GPIOC_R;
> + else
> + devpriv->gpioc_R = devpriv->bar0_mem + MF624_GPIOC_R;
> +
> +
> + ret = comedi_alloc_subdevices(dev, 4);
> + if (ret)
> + return ret;
> +
> + /* ADC */
> + s = &dev->subdevices[0];
> + s->type = COMEDI_SUBD_AI;
> + s->subdev_flags = SDF_READABLE | SDF_GROUND;
> + s->n_chan = 8;
> + s->maxdata = 0x3fff; /* 14 bits ADC */
> + s->range_table = &range_bipolar10;
> + 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->maxdata = 0x3fff; /* 14 bits DAC */
> + s->range_table = &range_bipolar10;
> + s->insn_write = mf6x4_ao_insn_write;
> + s->insn_read = mf6x4_ao_insn_read;
> +
> + /* DIN */
> + s = &dev->subdevices[2];
> + s->type = COMEDI_SUBD_DI;
> + s->subdev_flags = SDF_READABLE;
> + s->n_chan = 8;
> + s->maxdata = 1;
> + s->range_table = &range_digital;
> + s->insn_bits = mf6x4_di_insn_bits;
> +
> + /* DOUT */
> + s = &dev->subdevices[3];
> + s->type = COMEDI_SUBD_DO;
> + s->subdev_flags = SDF_WRITABLE;
> + s->n_chan = 8;
> + s->maxdata = 1;
> + s->range_table = &range_digital;
> + s->insn_bits = mf6x4_do_insn_bits;
> +
> + return 0;
> +}
> +
> +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);
> +}
> +
> +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, 0x0634), BOARD_MF634 },
> + { PCI_VDEVICE(HUMUSOFT, 0x0624), 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 <lisovy@gmail.com>");
> +MODULE_DESCRIPTION("Comedi MF634 and MF624 DAQ cards driver");
> +MODULE_LICENSE("GPL");
> --
> 1.8.3.2
>
Looks fine to me!
Reviewed-by: Ian Abbott <abbotti@mev.co.uk>
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Humusoft MF634 and MF624 DAQ cards driver
2014-01-08 7:47 ` Dan Carpenter
2014-01-08 8:35 ` Rostislav Lisovy
@ 2014-01-09 22:55 ` Rostislav Lisovy
1 sibling, 0 replies; 14+ messages in thread
From: Rostislav Lisovy @ 2014-01-09 22:55 UTC (permalink / raw)
To: Dan Carpenter
Cc: Ian Abbott, Hartley Sweeten, Greg Kroah-Hartman, linux-kernel,
devel, pisa
On Wed, 2014-01-08 at 10:47 +0300, Dan Carpenter wrote:
> On Tue, Jan 07, 2014 at 11:24:57PM +0100, Rostislav Lisovy wrote:
> > This patch adds Comedi driver for Humusoft MF634 (PCIe) and
> > MF624 (PCI) data acquisition cards. The legacy card Humusoft
> > MF614 is not supported. More info about the cards may be found
> > at http://humusoft.cz/produkty/datacq/
> > The driver was tested with both cards. Everything seems to work
> > properly. Just the basic functionality of the card (DIO, ADC, DAC)
> > is supported by this driver.
> >
> > Signed-off-by: Rostislav Lisovy <lisovy@gmail.com>
> >
> > create mode 100644 drivers/staging/comedi/drivers/mf6x4.c
> >
>
> There should be a "---" after the Signed-off-by line and before the
> diffstat "create mode 100644 drivers/staging/comedi/drivers/mf6x4.c"
> line. Otherwise, the diffstat gets included in the changelog.
I just resent the v3 patch with this flaw corrected. What surprises me a
bit is the fact that the issue was caused by the 'git format-patch'
command called with '--summary' argument. The dashes are included
properly when the argument is not used. Does it make sense?
Regards;
Rostislav Lisovy
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] comedi: Humusoft MF634 and MF624 DAQ cards driver
2014-01-09 22:46 [PATCH v3] " Rostislav Lisovy
@ 2014-01-09 22:46 ` Rostislav Lisovy
2014-01-10 10:29 ` Ian Abbott
0 siblings, 1 reply; 14+ messages in thread
From: Rostislav Lisovy @ 2014-01-09 22:46 UTC (permalink / raw)
To: Ian Abbott, Hartley Sweeten, Dan Carpenter, Greg Kroah-Hartman,
linux-kernel, devel
Cc: pisa, Rostislav Lisovy
This patch adds Comedi driver for Humusoft MF634 (PCIe) and
MF624 (PCI) data acquisition cards. The legacy card Humusoft
MF614 is not supported. More info about the cards may be found
at http://humusoft.cz/produkty/datacq/
The driver was tested with both cards. Everything seems to work
properly. Just the basic functionality of the card (DIO, ADC, DAC)
is supported by this driver.
Signed-off-by: Rostislav Lisovy <lisovy@gmail.com>
---
create mode 100644 drivers/staging/comedi/drivers/mf6x4.c
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/comedidev.h b/drivers/staging/comedi/comedidev.h
index 143be80..161bdd2 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -410,6 +410,7 @@ void comedi_driver_unregister(struct comedi_driver *);
#define PCI_VENDOR_ID_IOTECH 0x1616
#define PCI_VENDOR_ID_CONTEC 0x1221
#define PCI_VENDOR_ID_RTD 0x1435
+#define PCI_VENDOR_ID_HUMUSOFT 0x186c
struct pci_dev;
struct pci_driver;
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..81b78e0
--- /dev/null
+++ b/drivers/staging/comedi/drivers/mf6x4.c
@@ -0,0 +1,354 @@
+/*
+ * 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 <ds@schleef.org>
+ *
+ * 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 <lisovy@gmail.com>
+ * Status: works
+ * Updated:
+ * Configuration Options: none
+ */
+
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/delay.h>
+#include "../comedidev.h"
+
+/* Registers present in BAR0 memory region */
+#define MF624_GPIOC_R 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_R 0x10
+#define MF6X4_DIN_M 0xff
+#define MF6X4_DOUT_R 0x10
+#define MF6X4_DOUT_M 0xff
+
+#define MF6X4_ADSTART_R 0x20
+#define MF6X4_ADDATA_R 0x00
+#define MF6X4_ADCTRL_R 0x00
+#define MF6X4_ADCTRL_M 0xff
+
+#define MF6X4_DA0_R 0x20
+#define MF6X4_DA1_R 0x22
+#define MF6X4_DA2_R 0x24
+#define MF6X4_DA3_R 0x26
+#define MF6X4_DA4_R 0x28
+#define MF6X4_DA5_R 0x2a
+#define MF6X4_DA6_R 0x2c
+#define MF6X4_DA7_R 0x2e
+/* Map DAC cahnnel id to real HW-dependent offset value */
+#define MF6X4_DAC_R(x) (0x20 + ((x) * 2))
+#define MF6X4_DA_M 0x3fff
+
+/* BAR2 registers */
+#define MF634_GPIOC_R 0x68
+
+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 {
+ /*
+ * 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;
+
+ /*
+ * This configuration register has the same function and fields
+ * for both cards however it lies in different BARs on different
+ * offsets -- this variable makes the access easier
+ */
+ void __iomem *gpioc_R;
+
+ /* DAC value cache -- used for insn_read function */
+ int ao_readback[8];
+};
+
+static int mf6x4_di_insn_bits(struct comedi_device *dev,
+ struct comedi_subdevice *s,
+ struct comedi_insn *insn, unsigned int *data)
+{
+ struct mf6x4_private *devpriv = dev->private;
+
+ data[1] = ioread16(devpriv->bar1_mem + MF6X4_DIN_R) & MF6X4_DIN_M;
+
+ return insn->n;
+}
+
+static int mf6x4_do_insn_bits(struct comedi_device *dev,
+ struct comedi_subdevice *s,
+ struct comedi_insn *insn, unsigned int *data)
+{
+ struct mf6x4_private *devpriv = dev->private;
+
+ if (comedi_dio_update_state(s, data))
+ iowrite16(s->state & MF6X4_DOUT_M,
+ devpriv->bar1_mem + MF6X4_DOUT_R);
+
+ data[1] = s->state;
+
+ return insn->n;
+}
+
+static int mf6x4_ai_wait_for_eoc(struct comedi_device *dev,
+ unsigned int timeout)
+{
+ struct mf6x4_private *devpriv = dev->private;
+ unsigned int eolc;
+
+ while (timeout--) {
+ eolc = ioread32(devpriv->gpioc_R) & MF6X4_GPIOC_EOLC;
+ if (eolc)
+ return 0;
+
+ udelay(1);
+ }
+
+ return -ETIME;
+}
+
+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;
+ int chan = CR_CHAN(insn->chanspec);
+ int ret;
+ int i;
+ int d;
+
+ /* Set the ADC channel number in the scan list */
+ iowrite16((1 << chan) & MF6X4_ADCTRL_M,
+ devpriv->bar1_mem + MF6X4_ADCTRL_R);
+
+ for (i = 0; i < insn->n; i++) {
+ /* Trigger ADC conversion by reading ADSTART */
+ ioread16(devpriv->bar1_mem + MF6X4_ADSTART_R);
+
+ ret = mf6x4_ai_wait_for_eoc(dev, 100);
+ if (ret)
+ return ret;
+
+ /* Read the actual value */
+ d = ioread16(devpriv->bar1_mem + MF6X4_ADDATA_R);
+ d &= s->maxdata;
+ data[i] = d;
+ }
+
+ iowrite16(0x0, devpriv->bar1_mem + MF6X4_ADCTRL_R);
+
+ 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;
+ unsigned int chan = CR_CHAN(insn->chanspec);
+ uint32_t gpioc;
+ int i;
+
+ /* Enable instantaneous update of converters outputs + Enable DACs */
+ gpioc = ioread32(devpriv->gpioc_R);
+ iowrite32((gpioc & ~MF6X4_GPIOC_LDAC) | MF6X4_GPIOC_DACEN,
+ devpriv->gpioc_R);
+
+ for (i = 0; i < insn->n; i++) {
+ iowrite16(data[i] & MF6X4_DA_M,
+ devpriv->bar1_mem + MF6X4_DAC_R(chan));
+
+ devpriv->ao_readback[chan] = data[i];
+ }
+
+ return insn->n;
+}
+
+static int mf6x4_ao_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);
+ int i;
+
+ for (i = 0; i < insn->n; i++)
+ data[i] = devpriv->ao_readback[chan];
+
+ 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);
+ 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)
+ return -ENODEV;
+
+ devpriv->bar1_mem = pci_ioremap_bar(pcidev, board->bar_nums[1]);
+ if (!devpriv->bar1_mem)
+ return -ENODEV;
+
+ devpriv->bar2_mem = pci_ioremap_bar(pcidev, board->bar_nums[2]);
+ if (!devpriv->bar2_mem)
+ return -ENODEV;
+
+ if (board == &mf6x4_boards[BOARD_MF634])
+ devpriv->gpioc_R = devpriv->bar2_mem + MF634_GPIOC_R;
+ else
+ devpriv->gpioc_R = devpriv->bar0_mem + MF624_GPIOC_R;
+
+
+ ret = comedi_alloc_subdevices(dev, 4);
+ if (ret)
+ return ret;
+
+ /* ADC */
+ s = &dev->subdevices[0];
+ s->type = COMEDI_SUBD_AI;
+ s->subdev_flags = SDF_READABLE | SDF_GROUND;
+ s->n_chan = 8;
+ s->maxdata = 0x3fff; /* 14 bits ADC */
+ s->range_table = &range_bipolar10;
+ 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->maxdata = 0x3fff; /* 14 bits DAC */
+ s->range_table = &range_bipolar10;
+ s->insn_write = mf6x4_ao_insn_write;
+ s->insn_read = mf6x4_ao_insn_read;
+
+ /* DIN */
+ s = &dev->subdevices[2];
+ s->type = COMEDI_SUBD_DI;
+ s->subdev_flags = SDF_READABLE;
+ s->n_chan = 8;
+ s->maxdata = 1;
+ s->range_table = &range_digital;
+ s->insn_bits = mf6x4_di_insn_bits;
+
+ /* DOUT */
+ s = &dev->subdevices[3];
+ s->type = COMEDI_SUBD_DO;
+ s->subdev_flags = SDF_WRITABLE;
+ s->n_chan = 8;
+ s->maxdata = 1;
+ s->range_table = &range_digital;
+ s->insn_bits = mf6x4_do_insn_bits;
+
+ return 0;
+}
+
+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);
+}
+
+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, 0x0634), BOARD_MF634 },
+ { PCI_VDEVICE(HUMUSOFT, 0x0624), 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 <lisovy@gmail.com>");
+MODULE_DESCRIPTION("Comedi MF634 and MF624 DAQ cards driver");
+MODULE_LICENSE("GPL");
--
1.8.3.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Humusoft MF634 and MF624 DAQ cards driver
2014-01-08 8:35 ` Rostislav Lisovy
@ 2014-01-08 10:42 ` Ian Abbott
0 siblings, 0 replies; 14+ messages in thread
From: Ian Abbott @ 2014-01-08 10:42 UTC (permalink / raw)
To: Rostislav Lisovy, Dan Carpenter
Cc: Ian Abbott, Hartley Sweeten, Greg Kroah-Hartman, linux-kernel,
devel, pisa
On 2014-01-08 08:35, Rostislav Lisovy wrote:
> Hello Dan;
>
> On Wed, 2014-01-08 at 10:47 +0300, Dan Carpenter wrote:
>> On Tue, Jan 07, 2014 at 11:24:57PM +0100, Rostislav Lisovy wrote:
>>> This patch adds Comedi driver for Humusoft MF634 (PCIe) and
>>> MF624 (PCI) data acquisition cards. The legacy card Humusoft
>>> MF614 is not supported. More info about the cards may be found
>>> at http://humusoft.cz/produkty/datacq/
>>> The driver was tested with both cards. Everything seems to work
>>> properly. Just the basic functionality of the card (DIO, ADC, DAC)
>>> is supported by this driver.
>>>
>>> Signed-off-by: Rostislav Lisovy <lisovy@gmail.com>
>>>
>>> create mode 100644 drivers/staging/comedi/drivers/mf6x4.c
>>>
>>
>> There should be a "---" after the Signed-off-by line and before the
>> diffstat "create mode 100644 drivers/staging/comedi/drivers/mf6x4.c"
>> line. Otherwise, the diffstat gets included in the changelog.
>>
>> Signed-off-by: Rostislav Lisovy <lisovy@gmail.com>
>> ---
>> create mode 100644 drivers/staging/comedi/drivers/mf6x4.c
>>
>
> You are right, my mistake. But even though there should be v3 patch
> because I just realized there is an unnecessary comment in
> mf6x4_detach() and MF6X4_DAC_CHANN_CNT constant is no more used.
>
> Regards;
> Rostislav Lisovy
I'll wait for the v3 patch, but apart from that it looks fine to me. I
suppose it could be argued that the driver only uses 2 of the 3 BARs it
ioremaps (a different 2 for each card), but they're only single page
mappings.
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Humusoft MF634 and MF624 DAQ cards driver
2014-01-08 7:47 ` Dan Carpenter
@ 2014-01-08 8:35 ` Rostislav Lisovy
2014-01-08 10:42 ` Ian Abbott
2014-01-09 22:55 ` Rostislav Lisovy
1 sibling, 1 reply; 14+ messages in thread
From: Rostislav Lisovy @ 2014-01-08 8:35 UTC (permalink / raw)
To: Dan Carpenter
Cc: Ian Abbott, Hartley Sweeten, Greg Kroah-Hartman, linux-kernel,
devel, pisa
Hello Dan;
On Wed, 2014-01-08 at 10:47 +0300, Dan Carpenter wrote:
> On Tue, Jan 07, 2014 at 11:24:57PM +0100, Rostislav Lisovy wrote:
> > This patch adds Comedi driver for Humusoft MF634 (PCIe) and
> > MF624 (PCI) data acquisition cards. The legacy card Humusoft
> > MF614 is not supported. More info about the cards may be found
> > at http://humusoft.cz/produkty/datacq/
> > The driver was tested with both cards. Everything seems to work
> > properly. Just the basic functionality of the card (DIO, ADC, DAC)
> > is supported by this driver.
> >
> > Signed-off-by: Rostislav Lisovy <lisovy@gmail.com>
> >
> > create mode 100644 drivers/staging/comedi/drivers/mf6x4.c
> >
>
> There should be a "---" after the Signed-off-by line and before the
> diffstat "create mode 100644 drivers/staging/comedi/drivers/mf6x4.c"
> line. Otherwise, the diffstat gets included in the changelog.
>
> Signed-off-by: Rostislav Lisovy <lisovy@gmail.com>
> ---
> create mode 100644 drivers/staging/comedi/drivers/mf6x4.c
>
You are right, my mistake. But even though there should be v3 patch
because I just realized there is an unnecessary comment in
mf6x4_detach() and MF6X4_DAC_CHANN_CNT constant is no more used.
Regards;
Rostislav Lisovy
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Humusoft MF634 and MF624 DAQ cards driver
2014-01-07 22:24 ` [PATCH] " Rostislav Lisovy
@ 2014-01-08 7:47 ` Dan Carpenter
2014-01-08 8:35 ` Rostislav Lisovy
2014-01-09 22:55 ` Rostislav Lisovy
0 siblings, 2 replies; 14+ messages in thread
From: Dan Carpenter @ 2014-01-08 7:47 UTC (permalink / raw)
To: Rostislav Lisovy
Cc: Ian Abbott, Hartley Sweeten, Greg Kroah-Hartman, linux-kernel,
devel, pisa
On Tue, Jan 07, 2014 at 11:24:57PM +0100, Rostislav Lisovy wrote:
> This patch adds Comedi driver for Humusoft MF634 (PCIe) and
> MF624 (PCI) data acquisition cards. The legacy card Humusoft
> MF614 is not supported. More info about the cards may be found
> at http://humusoft.cz/produkty/datacq/
> The driver was tested with both cards. Everything seems to work
> properly. Just the basic functionality of the card (DIO, ADC, DAC)
> is supported by this driver.
>
> Signed-off-by: Rostislav Lisovy <lisovy@gmail.com>
>
> create mode 100644 drivers/staging/comedi/drivers/mf6x4.c
>
There should be a "---" after the Signed-off-by line and before the
diffstat "create mode 100644 drivers/staging/comedi/drivers/mf6x4.c"
line. Otherwise, the diffstat gets included in the changelog.
Signed-off-by: Rostislav Lisovy <lisovy@gmail.com>
---
create mode 100644 drivers/staging/comedi/drivers/mf6x4.c
It's a small thing... I almost feel bad for pointing it out.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] comedi: Humusoft MF634 and MF624 DAQ cards driver
2014-01-07 22:24 [PATCH v2] " Rostislav Lisovy
@ 2014-01-07 22:24 ` Rostislav Lisovy
2014-01-08 7:47 ` Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: Rostislav Lisovy @ 2014-01-07 22:24 UTC (permalink / raw)
To: Ian Abbott, Hartley Sweeten, Dan Carpenter, Greg Kroah-Hartman,
linux-kernel, devel
Cc: pisa, Rostislav Lisovy
This patch adds Comedi driver for Humusoft MF634 (PCIe) and
MF624 (PCI) data acquisition cards. The legacy card Humusoft
MF614 is not supported. More info about the cards may be found
at http://humusoft.cz/produkty/datacq/
The driver was tested with both cards. Everything seems to work
properly. Just the basic functionality of the card (DIO, ADC, DAC)
is supported by this driver.
Signed-off-by: Rostislav Lisovy <lisovy@gmail.com>
create mode 100644 drivers/staging/comedi/drivers/mf6x4.c
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/comedidev.h b/drivers/staging/comedi/comedidev.h
index 143be80..161bdd2 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -410,6 +410,7 @@ void comedi_driver_unregister(struct comedi_driver *);
#define PCI_VENDOR_ID_IOTECH 0x1616
#define PCI_VENDOR_ID_CONTEC 0x1221
#define PCI_VENDOR_ID_RTD 0x1435
+#define PCI_VENDOR_ID_HUMUSOFT 0x186c
struct pci_dev;
struct pci_driver;
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..d230d48
--- /dev/null
+++ b/drivers/staging/comedi/drivers/mf6x4.c
@@ -0,0 +1,355 @@
+/*
+ * 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 <ds@schleef.org>
+ *
+ * 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 <lisovy@gmail.com>
+ * Status: works
+ * Updated:
+ * Configuration Options: none
+ */
+
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/delay.h>
+#include "../comedidev.h"
+
+/* Registers present in BAR0 memory region */
+#define MF624_GPIOC_R 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_R 0x10
+#define MF6X4_DIN_M 0xff
+#define MF6X4_DOUT_R 0x10
+#define MF6X4_DOUT_M 0xff
+
+#define MF6X4_ADSTART_R 0x20
+#define MF6X4_ADDATA_R 0x00
+#define MF6X4_ADCTRL_R 0x00
+#define MF6X4_ADCTRL_M 0xff
+
+#define MF6X4_DA0_R 0x20
+#define MF6X4_DA1_R 0x22
+#define MF6X4_DA2_R 0x24
+#define MF6X4_DA3_R 0x26
+#define MF6X4_DA4_R 0x28
+#define MF6X4_DA5_R 0x2a
+#define MF6X4_DA6_R 0x2c
+#define MF6X4_DA7_R 0x2e
+/* Map DAC cahnnel id to real HW-dependent offset value */
+#define MF6X4_DAC_R(x) (0x20 + ((x) * 2))
+#define MF6X4_DA_M 0x3fff
+#define MF6X4_DAC_CHANN_CNT 8
+
+/* BAR2 registers */
+#define MF634_GPIOC_R 0x68
+
+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 {
+ /*
+ * 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;
+
+ /*
+ * This configuration register has the same function and fields
+ * for both cards however it lies in different BARs on different
+ * offsets -- this variable makes the access easier
+ */
+ void __iomem *gpioc_R;
+
+ /* DAC value cache -- used for insn_read function */
+ int ao_readback[8];
+};
+
+static int mf6x4_di_insn_bits(struct comedi_device *dev,
+ struct comedi_subdevice *s,
+ struct comedi_insn *insn, unsigned int *data)
+{
+ struct mf6x4_private *devpriv = dev->private;
+
+ data[1] = ioread16(devpriv->bar1_mem + MF6X4_DIN_R) & MF6X4_DIN_M;
+
+ return insn->n;
+}
+
+static int mf6x4_do_insn_bits(struct comedi_device *dev,
+ struct comedi_subdevice *s,
+ struct comedi_insn *insn, unsigned int *data)
+{
+ struct mf6x4_private *devpriv = dev->private;
+
+ if (comedi_dio_update_state(s, data))
+ iowrite16(s->state & MF6X4_DOUT_M,
+ devpriv->bar1_mem + MF6X4_DOUT_R);
+
+ data[1] = s->state;
+
+ return insn->n;
+}
+
+static int mf6x4_ai_wait_for_eoc(struct comedi_device *dev,
+ unsigned int timeout)
+{
+ struct mf6x4_private *devpriv = dev->private;
+ unsigned int eolc;
+
+ while (timeout--) {
+ eolc = ioread32(devpriv->gpioc_R) & MF6X4_GPIOC_EOLC;
+ if (eolc)
+ return 0;
+
+ udelay(1);
+ }
+
+ return -ETIME;
+}
+
+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;
+ int chan = CR_CHAN(insn->chanspec);
+ int ret;
+ int i;
+ int d;
+
+ /* Set the ADC channel number in the scan list */
+ iowrite16((1 << chan) & MF6X4_ADCTRL_M,
+ devpriv->bar1_mem + MF6X4_ADCTRL_R);
+
+ for (i = 0; i < insn->n; i++) {
+ /* Trigger ADC conversion by reading ADSTART */
+ ioread16(devpriv->bar1_mem + MF6X4_ADSTART_R);
+
+ ret = mf6x4_ai_wait_for_eoc(dev, 100);
+ if (ret)
+ return ret;
+
+ /* Read the actual value */
+ d = ioread16(devpriv->bar1_mem + MF6X4_ADDATA_R);
+ d &= s->maxdata;
+ data[i] = d;
+ }
+
+ iowrite16(0x0, devpriv->bar1_mem + MF6X4_ADCTRL_R);
+
+ 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;
+ unsigned int chan = CR_CHAN(insn->chanspec);
+ uint32_t gpioc;
+ int i;
+
+ /* Enable instantaneous update of converters outputs + Enable DACs */
+ gpioc = ioread32(devpriv->gpioc_R);
+ iowrite32((gpioc & ~MF6X4_GPIOC_LDAC) | MF6X4_GPIOC_DACEN,
+ devpriv->gpioc_R);
+
+ for (i = 0; i < insn->n; i++) {
+ iowrite16(data[i] & MF6X4_DA_M,
+ devpriv->bar1_mem + MF6X4_DAC_R(chan));
+
+ devpriv->ao_readback[chan] = data[i];
+ }
+
+ return insn->n;
+}
+
+static int mf6x4_ao_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);
+ int i;
+
+ for (i = 0; i < insn->n; i++)
+ data[i] = devpriv->ao_readback[chan];
+
+ 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);
+ 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)
+ return -ENODEV;
+
+ devpriv->bar1_mem = pci_ioremap_bar(pcidev, board->bar_nums[1]);
+ if (!devpriv->bar1_mem)
+ return -ENODEV;
+
+ devpriv->bar2_mem = pci_ioremap_bar(pcidev, board->bar_nums[2]);
+ if (!devpriv->bar2_mem)
+ return -ENODEV;
+
+ if (board == &mf6x4_boards[BOARD_MF634])
+ devpriv->gpioc_R = devpriv->bar2_mem + MF634_GPIOC_R;
+ else
+ devpriv->gpioc_R = devpriv->bar0_mem + MF624_GPIOC_R;
+
+
+ ret = comedi_alloc_subdevices(dev, 4);
+ if (ret)
+ return ret;
+
+ /* ADC */
+ s = &dev->subdevices[0];
+ s->type = COMEDI_SUBD_AI;
+ s->subdev_flags = SDF_READABLE | SDF_GROUND;
+ s->n_chan = 8;
+ s->maxdata = 0x3fff; /* 14 bits ADC */
+ s->range_table = &range_bipolar10;
+ 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->maxdata = 0x3fff; /* 14 bits DAC */
+ s->range_table = &range_bipolar10;
+ s->insn_write = mf6x4_ao_insn_write;
+ s->insn_read = mf6x4_ao_insn_read;
+
+ /* DIN */
+ s = &dev->subdevices[2];
+ s->type = COMEDI_SUBD_DI;
+ s->subdev_flags = SDF_READABLE;
+ s->n_chan = 8;
+ s->maxdata = 1;
+ s->range_table = &range_digital;
+ s->insn_bits = mf6x4_di_insn_bits;
+
+ /* DOUT */
+ s = &dev->subdevices[3];
+ s->type = COMEDI_SUBD_DO;
+ s->subdev_flags = SDF_WRITABLE;
+ s->n_chan = 8;
+ s->maxdata = 1;
+ s->range_table = &range_digital;
+ s->insn_bits = mf6x4_do_insn_bits;
+
+ return 0;
+}
+
+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_Rions(), 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, 0x0634), BOARD_MF634 },
+ { PCI_VDEVICE(HUMUSOFT, 0x0624), 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 <lisovy@gmail.com>");
+MODULE_DESCRIPTION("Comedi MF634 and MF624 DAQ cards driver");
+MODULE_LICENSE("GPL");
--
1.8.3.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-01-10 10:30 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-12-31 1:36 [PATCH] comedi: Humusoft MF634 and MF624 DAQ cards driver Rostislav Lisovy
2013-12-31 1:36 ` Rostislav Lisovy
2013-12-31 2:31 ` Dan Carpenter
2014-01-02 18:38 ` Hartley Sweeten
2014-01-05 15:04 ` Rostislav Lisovy
2014-01-05 18:18 ` Dan Carpenter
2014-01-06 12:37 ` Ian Abbott
2014-01-07 22:24 [PATCH v2] " Rostislav Lisovy
2014-01-07 22:24 ` [PATCH] " Rostislav Lisovy
2014-01-08 7:47 ` Dan Carpenter
2014-01-08 8:35 ` Rostislav Lisovy
2014-01-08 10:42 ` Ian Abbott
2014-01-09 22:55 ` Rostislav Lisovy
2014-01-09 22:46 [PATCH v3] " Rostislav Lisovy
2014-01-09 22:46 ` [PATCH] " Rostislav Lisovy
2014-01-10 10:29 ` Ian Abbott
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).