Date:	Thu, 2 Jan 2014 18:38:16 +0000
From:	Hartley Sweeten <HartleyS@...ionengravers.com>
To:	Rostislav Lisovy <lisovy@...il.com>,
	Ian Abbott <abbotti@....co.uk>,
	"Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>
CC:	"pisa@....felk.cvut.cz" <pisa@....felk.cvut.cz>
Subject: RE: [PATCH] comedi: Humusoft MF634 and MF624 DAQ cards driver

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@...leef.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@...il.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@...il.com>");
> +MODULE_DESCRIPTION("Comedi MF634 and MF624 DAQ cards driver");
> +MODULE_LICENSE("GPL");
> -- 
> 1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists