Re: [PATCH 1/2] staging/comedi/drivers: add driver for ad7739 analog to digital converter chip on an spi bus

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

Looks better, some minor comments below.

Note, I don't know the comedi subsystem that well, so I'll let others
say if you messed up anything there.

> The ad7739.h file is need for inclusion into board specific initialization file
> and to configure the exact chip.

Ok, so it will end up in include/linux/platform_data/ eventually, right?


> +/* #define DEBUG 1 */
> +
> +#include "../comedidev.h"
> +#include "ad7739.h"
> +#include <linux/gpio.h>
> +#include <linux/completion.h>
> +#include <linux/spi/spi.h>
> +#include <linux/interrupt.h>

Extra line here would be nice.

> +#define MAX_DATA 0xFFFFFF	/* 24-bit mode only */
> +#define CONVERT_TIMEOUT_MSECS 100	/* spec max conv time = 2689 usecs */
> +#define DRIVER_NAME "ad7739"

Why not just use KBUILD_MODNAME wherever you were using DRIVER_NAME?
That ensures you get things right.

> +#ifdef DEBUG
> +static int ad7739_print(struct device *dev, void *data)
> +{
> +	dev_info(dev, "dev_name = %s\n", dev_name(dev));
> +
> +	return 0;
> +}
> +#endif

You should probably take this out, along with the call below where you
use it, as it's not needed anymore, right?

> +	snprintf(devname, sizeof(devname), "%s%u.%u", spi_bus_type.name,
> +		 (unsigned)((dev->iobase >> 8) & 0xFF),
> +		 (unsigned)(dev->iobase & 0xFF));
> +
> +	d = bus_find_device_by_name(&spi_bus_type, NULL, devname);
> +	if (!d) {
> +		dev_err(dev->class_dev, "devices %s not found\n", devname);
> +		return -EINVAL;
> +	}
> +
> +	priv->spi = to_spi_device(d);

Is that how you register a spi device normally?  I don't see any other
SPI drivers doing this, why are you not just registering the driver with
the bus like others do?

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel


[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux