Since you're redoing this already, I thought I nit pick it to death.
On Fri, Mar 16, 2012 at 12:19:25PM +0600, Alexander Pazdnikov wrote:
> Thank you Greg, I've corrected driver taking into account your comments.
> This is my first friver, I've missed to check the first version with checkpatch script.
> Today it goes through checkpatch script with no errors and warnings.
> Appreciate any comments.
>
> The ad7739.h file is need for inclusion into board specific initialization file
> and to configure the exact chip.
>
> Signed-off-by: Alexander Pazdnikov <pazdnikov@xxxxxxx>
^^^^^^^
gmail.com
> +/*
> + Usage example through board-setup.
> +static const struct ad7739_platform_data dd11_adc =.{
^
Remove the typo extra period.
> + .chanselect_p0p1 = 1,
> +};
> +
> +
> +struct ad7739_private *devpriv(struct comedi_device *dev)
Make this function static.
> +{
> + return dev->private;
> +}
> +
> +/* write buffer */
> +static
> +int ad7739_write_msg(struct comedi_device *dev, const u8 *buf, size_t len)
Normally we would either break this up like:
static int ad7739_write_msg(struct comedi_device *dev, const u8 *buf,
size_t len)
Or if you are really old maybe like this:
static int
ad7739_write_msg(struct comedi_device *dev, const u8 *buf, size_t len)
> +{
> + return spi_write(devpriv(dev)->spi, buf, len);
> +}
> +
> +#define COMM_REG_READ 0x40
> +
> +/* write 8-bit register */
> +static int ad7739_write(struct comedi_device *dev, u8 reg, u8 val)
> +{
> + int ret = 0;
^^^^
No need to initialize this.
> + struct spi_device *spi = devpriv(dev)->spi;
> +
> + u8 out[2];
Remove the blank line before the "out" declaration.
> +
> + out[0] = reg & ~COMM_REG_READ;
> + out[1] = val;
> +
> + dev_dbg(&spi->dev, "write reg %#x, val %#x\n", reg, val);
> +
> + ret = spi_write(spi, out, sizeof(out));
> +
> + return ret;
> +}
> +
> +/* read register of desired size */
> +static
> +int ad7739_read(struct comedi_device *dev, u8 reg, u8 *in, size_t size)
> +{
> + struct spi_device *spi = devpriv(dev)->spi;
> + int ret;
> +
> + u8 cmd = reg | COMM_REG_READ;
Remove blank line before the "cmd" declaration.
> +
> + ret = spi_write_then_read(spi, &cmd, 1, in, size);
> +
> +static int ai_insn_read(struct comedi_device *dev, struct comedi_subdevice *s,
> + struct comedi_insn *insn, unsigned int *data)
> +{
> + u8 buf[4];
> + u8 status = 0;
> +
> + const unsigned chan = CR_CHAN(insn->chanspec);
> + const unsigned range = CR_RANGE(insn->chanspec);
> + const unsigned aref = CR_AREF(insn->chanspec);
> + struct ad7739_platform_data *pdata;
> +
> + int ret = 0;
> +
> + u8 setup = range
> + + ((aref == AREF_DIFF) ? SETUP_DIFFER : 0) + SETUP_ENABLE;
> +
Remove the blank lines between the declarations. "ret" and "status"
don't need to be initialized.
> +static
> +int ad7739_attach(struct comedi_device *dev, struct comedi_devconfig *it)
> +{
> + struct comedi_subdevice *s = NULL;
> + struct ad7739_private *priv = NULL;
> + struct device *d = NULL;
> +
> + char devname[64];
Remove blank line in declaration block.
> +
> + if (alloc_private(dev, sizeof(struct ad7739_private)) < 0)
> + return -ENOMEM;
ret = alloc_private(dev, sizeof(struct ad7739_private));
if (ret)
return ret;
> +
> + if (alloc_subdevices(dev, 1) < 0)
> + return -ENOMEM;
ret = alloc_subdevices(dev, 1);
if (ret)
return ret;
> + /* Reset the chip */
> + ad7739_reset(dev);
> +
> + init_completion(&priv->ready);
> +
> + if (request_irq(priv->spi->irq, ad7739_irq, 0, dev->board_name, priv)) {
> +
> + dev_err(dev->class_dev, "IRQ %u request failed\n",
> + priv->spi->irq);
> + return -EBUSY;
> + }
ret = request_irq(priv->spi->irq, ad7739_irq, 0, dev->board_name, priv);
if (ret) {
dev_err(dev->class_dev, "IRQ %u request failed (%d)\n",
priv->spi->irq, ret);
return ret;
}
Anyway, small nit picks. In a way, if this driver were crap like
certain other staging drivers I wouldn't have said anything but it's
actually really nice. Good job.
regards,
dan carpenter
Attachment:
signature.asc
Description: Digital signature
_______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
