Re: [PATCH v2 2/2] MAINTAINERS: Add maintainer for Diolan U2C-12 I2C adapter driver

Re: [PATCH v2 2/2] MAINTAINERS: Add maintainer for Diolan U2C-12 I2C adapter driver

[Ben Dooks]

On Wed, Jan 19, 2011 at 01:26:38PM -0800, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>

meaningful commit messages please.

> ---
>  MAINTAINERS |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1af022e..12b0be3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2089,6 +2089,12 @@ F:	Documentation/serial/digiepca.txt
>  F:	drivers/char/epca*
>  F:	drivers/char/digi*
>  
> +DIOLAN U2C-12 I2C DRIVER
> +M:	Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
> +L:	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +S:	Maintained
> +F:	drivers/i2c/busses/i2c-diolan-u2c.c
> +
>  DIRECTORY NOTIFICATION (DNOTIFY)
>  M:	Eric Paris <eparis-FjpueFixGhCM4zKIHC2jIg@public.gmane.org>
>  S:	Maintained
> -- 
> 1.7.3.1
> 

-- 
Ben Dooks, ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

Re: [PATCH v2 2/2] MAINTAINERS: Add maintainer for Diolan U2C-12 I2C adapter driver

[Guenter Roeck]

On Wed, 2011-01-19 at 15:56 -0500, Ben Dooks wrote:
> On Wed, Jan 19, 2011 at 01:26:38PM -0800, Guenter Roeck wrote:
> > Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
> 
> meaningful commit messages please.
> 
In general I agree, only if the headline says everything what else do
you want me to say here ? That I am adding myself as maintainer ?

Guenter

> > ---
> >  MAINTAINERS |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 1af022e..12b0be3 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2089,6 +2089,12 @@ F:	Documentation/serial/digiepca.txt
> >  F:	drivers/char/epca*
> >  F:	drivers/char/digi*
> >  
> > +DIOLAN U2C-12 I2C DRIVER
> > +M:	Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
> > +L:	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > +S:	Maintained
> > +F:	drivers/i2c/busses/i2c-diolan-u2c.c
> > +
> >  DIRECTORY NOTIFICATION (DNOTIFY)
> >  M:	Eric Paris <eparis-FjpueFixGhCM4zKIHC2jIg@public.gmane.org>
> >  S:	Maintained
> > -- 
> > 1.7.3.1
> > 
> 

Re: [PATCH v2 1/2] i2c/busses: Diolan U2C-12 USB/I2C adapter driver

[Ben Dooks]

On Wed, Jan 19, 2011 at 01:26:37PM -0800, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/i2c/busses/Kconfig          |   10 +
>  drivers/i2c/busses/Makefile         |    1 +
>  drivers/i2c/busses/i2c-diolan-u2c.c |  529 +++++++++++++++++++++++++++++++++++
>  3 files changed, 540 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/i2c/busses/i2c-diolan-u2c.c
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 113505a..30f8dbd 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -648,6 +648,16 @@ config I2C_EG20T
>  
>  comment "External I2C/SMBus adapter drivers"
>  
> +config I2C_DIOLAN_U2C
> +	tristate "Diolan U2C-12 USB adapter"
> +	depends on USB
> +	help
> +	  If you say yes to this option, support will be included for Diolan
> +	  U2C-12, a USB to I2C interface.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-diolan-u2c.
> +
>  config I2C_PARPORT
>  	tristate "Parallel port adapter"
>  	depends on PARPORT
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 9d2d0ec..3c630b7 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -64,6 +64,7 @@ obj-$(CONFIG_I2C_XILINX)	+= i2c-xiic.o
>  obj-$(CONFIG_I2C_EG20T)         += i2c-eg20t.o
>  
>  # External I2C/SMBus adapter drivers
> +obj-$(CONFIG_I2C_DIOLAN_U2C)	+= i2c-diolan-u2c.o
>  obj-$(CONFIG_I2C_PARPORT)	+= i2c-parport.o
>  obj-$(CONFIG_I2C_PARPORT_LIGHT)	+= i2c-parport-light.o
>  obj-$(CONFIG_I2C_TAOS_EVM)	+= i2c-taos-evm.o
> diff --git a/drivers/i2c/busses/i2c-diolan-u2c.c b/drivers/i2c/busses/i2c-diolan-u2c.c
> new file mode 100644
> index 0000000..44905ce
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-diolan-u2c.c
> @@ -0,0 +1,529 @@
> +/*
> + * Driver for the Diolan u2c-12 USB-I2C adapter
> + *
> + * Copyright (c) 2010-2011 Ericsson AB
> + *
> + * Derived from:
> + *  i2c-tiny-usb.c
> + *  Copyright (C) 2006-2007 Till Harbaum (Till-zicpKgigMvpAfugRpC6u6w@public.gmane.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, version 2.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +#include <linux/i2c.h>
> +
> +#define DRIVER_NAME		"i2c-diolan-u2c"
> +
> +#define USB_VENDOR_ID_DIOLAN		0x0abf
> +#define USB_DEVICE_ID_DIOLAN_U2C	0x3370
> +

might be worth type-defing the commands and responses, but not especially
importnant.

> +#define U2C_I2C_SPEED_FAST	0	/* 400 kHz */
> +#define U2C_I2C_SPEED_STD	1	/* 100 kHz */
> +#define U2C_I2C_SPEED_2KHZ	242	/* 2 kHz, minimum speed */
> +#define U2C_I2C_SPEED(f)	((DIV_ROUND_CLOSEST(1000000, (f)) - 10) / 2 + 1)

how about rounding down? surely the speed requested is the maximum?

> +/* Structure to hold all of our device specific stuff */
> +struct i2c_diolan_u2c {
> +	struct usb_device *usb_dev;	/* the usb device for this device */
> +	struct usb_interface *interface;/* the interface for this device */
> +	struct i2c_adapter adapter;	/* i2c related things */
> +	int olen;			/* Output buffer length */
> +	int ocount;			/* Number of enqueued messages */
> +	u8 obuffer[DIOLAN_OUTBUF_LEN];	/* output buffer */
> +	u8 ibuffer[DIOLAN_INBUF_LEN];	/* input buffer */

you should cache-line align the buffers to avoid any problems with
dma-vs-cache, etc.

> +/* Send command to device, and get response. */
> +static int diolan_usb_transfer(struct i2c_diolan_u2c *dev)
> +{
> +	int ret = 0;
> +	int actual;
> +	int i;
> +
> +	if (!dev->olen || !dev->ocount)
> +		return -EINVAL;
> +
> +	ret = usb_bulk_msg(dev->usb_dev,
> +			   usb_sndbulkpipe(dev->usb_dev, DIOLAN_OUT_EP),
> +			   dev->obuffer, dev->olen, &actual,
> +			   DIOLAN_USB_TIMEOUT);
> +	if (!ret) {
> +		for (i = 0; i < dev->ocount; i++) {
> +			int tmpret;
> +
> +			tmpret = usb_bulk_msg(dev->usb_dev,
> +					      usb_rcvbulkpipe(dev->usb_dev,
> +							      DIOLAN_IN_EP),
> +					      dev->ibuffer,
> +					      sizeof(dev->ibuffer), &actual,
> +					      DIOLAN_USB_TIMEOUT);
> +			if (tmpret < 0 && ret == 0)
> +				ret = tmpret;
> +			else if (ret >= 0 && tmpret == 0 && actual > 0) {
> +				switch (dev->ibuffer[actual - 1]) {
> +				case RESP_NACK:
> +					ret = -ENXIO;
> +					break;
> +				case RESP_TIMEOUT:
> +					ret = -ETIMEDOUT;
> +					break;
> +				case RESP_OK:
> +					/* strip off return code */
> +					ret = actual - 1;
> +					break;
> +				default:
> +					ret = -EIO;
> +					break;
> +				}
> +			}
> +		}
> +	}

this looks a little complicated with ret and tmpret.

> +	dev->olen = 0;
> +	dev->ocount = 0;
> +	return ret;
> +}
> +

> +static int diolan_usb_cmd_data2(struct i2c_diolan_u2c *dev, u8 command, u8 d1,
> +				u8 d2, bool flush)
> +{
> +	int ret = 0;
> +
> +	dev->obuffer[dev->olen++] = command;
> +	dev->obuffer[dev->olen++] = d1;
> +	dev->obuffer[dev->olen++] = d2;
> +	dev->ocount++;
> +	if (flush || dev->olen >= DIOLAN_FLUSH_LEN)
> +		ret = diolan_usb_transfer(dev);
> +	return ret;
> +}

how about wrapping the check for usb_transfer into one call?
say diolan_written_cmd(dev, flush) and then just use that as a
return and remove any need for 'int ret = 0' as well.

> +/*
> + * Flush input queue.
> + * If we don't do this at startup and the controller has queued up
> + * messages which were not retrieved, it will stop responding
> + * at some point.
> + */
> +static void diolan_flush_input(struct i2c_diolan_u2c *dev)
> +{
> +	int i;
> +
> +	for (i = 0; i < 10; i++) {
> +		int actual = 0;
> +		int ret;
> +
> +		ret = usb_bulk_msg(dev->usb_dev,
> +				   usb_rcvbulkpipe(dev->usb_dev, DIOLAN_IN_EP),
> +				   dev->ibuffer, sizeof(dev->ibuffer), &actual,
> +				   DIOLAN_USB_TIMEOUT);
> +		if (ret < 0 || actual == 0)
> +			break;
> +	}
> +	if (i == 10)
> +		dev_err(&dev->interface->dev, "Failed to flush input buffer\n");

[snip]


> +		 "Diolan U2C at USB bus %03d address %03d speed %d Hz\n",
> +		 dev->usb_dev->bus->busnum, dev->usb_dev->devnum, frequency);
> +
> +	diolan_flush_input(dev);
> +	diolan_fw_version(dev);

why bother returning ret from some of these when they're not checked?

> +	ret = diolan_i2c_start(dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < num; i++) {
> +		pmsg = &msgs[i];
> +		if (i) {
> +			ret = diolan_i2c_repeated_start(dev);
> +			if (ret < 0)
> +				goto abort;
> +		}

i think this is ok.

> +	int ret;
> +
> +	/* allocate memory for our device state and initialize it */
> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +	if (dev == NULL) {
> +		dev_err(&interface->dev, "Out of memory\n");

how about "no memory for device state"

> +		ret = -ENOMEM;
> +		goto error;
> +	}
> +
> +	dev->usb_dev = usb_get_dev(interface_to_usbdev(interface));
> +	dev->interface = interface;
> +
> +	/* save our data pointer in this interface device */
> +	usb_set_intfdata(interface, dev);
> +
> +	/* setup i2c adapter description */
> +	dev->adapter.owner = THIS_MODULE;
> +	dev->adapter.class = I2C_CLASS_HWMON;
> +	dev->adapter.algo = &diolan_usb_algorithm;
> +	i2c_set_adapdata(&dev->adapter, dev);
> +	snprintf(dev->adapter.name, sizeof(dev->adapter.name),
> +		 DRIVER_NAME " at bus %03d device %03d",
> +		 dev->usb_dev->bus->busnum, dev->usb_dev->devnum);
> +
> +	dev->adapter.dev.parent = &dev->interface->dev;
> +
> +	/* initialize diolan i2c interface */
> +	ret = diolan_init(dev);
> +	if (ret < 0)
> +		goto error_free;

do all return paths of diolan_init() print error?

> +
> +	/* and finally attach to i2c layer */
> +	ret = i2c_add_adapter(&dev->adapter);
> +	if (ret < 0) {
> +		dev_err(&interface->dev, "failed to add I2C adapter\n");
> +		goto error_free;
> +	}
> +
> +	dev_dbg(&interface->dev, "connected " DRIVER_NAME "\n");
> +
> +	return 0;
> +
> +error_free:
> +	usb_set_intfdata(interface, NULL);
> +	diolan_u2c_free(dev);
> +error:
> +	return ret;
> +}
> +

-- 
Ben Dooks, ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

[PATCH v2 0/2] Add support for Diolan U2C-12 USB/I2C adapter.

[Guenter Roeck]

v2 changes:
- Moved receive and transmit buffers to allocated driver memory
- diolan_usb_transfer() no longer requires buffer and length arguments
- Pass "struct i2c_diolan_u2c *" instead of "struct i2c_adapter *" to many of
  the internal usb access functions.
- Introduced diolan_usb_cmd(), diolan_usb_cmd_data(), and diolan_usb_cmd_data2()
  as front-end functions for actual commands.
- Function prefix is now diolan_ for all functions.
- Removed diolan_scan().
- Fixed race conditions in diolan_u2c_probe().
- Use i2c_{set,get}_adapdata instead of misusing adapter.algo_data.
- Driver name is now i2c-diolan-u2c (instead of i2c-u2c-usb).
- Added support for "frequency" module parameter to set i2c clock frequency
- Enabled I2C clock stretching support
- Support sending multiple commands in single usb transfer
- Several other minor changes to address review comments.

[PATCH v2 1/2] i2c/busses: Diolan U2C-12 USB/I2C adapter driver

[Guenter Roeck]

Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
---
 drivers/i2c/busses/Kconfig          |   10 +
 drivers/i2c/busses/Makefile         |    1 +
 drivers/i2c/busses/i2c-diolan-u2c.c |  529 +++++++++++++++++++++++++++++++++++
 3 files changed, 540 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-diolan-u2c.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 113505a..30f8dbd 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -648,6 +648,16 @@ config I2C_EG20T
 
 comment "External I2C/SMBus adapter drivers"
 
+config I2C_DIOLAN_U2C
+	tristate "Diolan U2C-12 USB adapter"
+	depends on USB
+	help
+	  If you say yes to this option, support will be included for Diolan
+	  U2C-12, a USB to I2C interface.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-diolan-u2c.
+
 config I2C_PARPORT
 	tristate "Parallel port adapter"
 	depends on PARPORT
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 9d2d0ec..3c630b7 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -64,6 +64,7 @@ obj-$(CONFIG_I2C_XILINX)	+= i2c-xiic.o
 obj-$(CONFIG_I2C_EG20T)         += i2c-eg20t.o
 
 # External I2C/SMBus adapter drivers
+obj-$(CONFIG_I2C_DIOLAN_U2C)	+= i2c-diolan-u2c.o
 obj-$(CONFIG_I2C_PARPORT)	+= i2c-parport.o
 obj-$(CONFIG_I2C_PARPORT_LIGHT)	+= i2c-parport-light.o
 obj-$(CONFIG_I2C_TAOS_EVM)	+= i2c-taos-evm.o
diff --git a/drivers/i2c/busses/i2c-diolan-u2c.c b/drivers/i2c/busses/i2c-diolan-u2c.c
new file mode 100644
index 0000000..44905ce
--- /dev/null
+++ b/drivers/i2c/busses/i2c-diolan-u2c.c
@@ -0,0 +1,529 @@
+/*
+ * Driver for the Diolan u2c-12 USB-I2C adapter
+ *
+ * Copyright (c) 2010-2011 Ericsson AB
+ *
+ * Derived from:
+ *  i2c-tiny-usb.c
+ *  Copyright (C) 2006-2007 Till Harbaum (Till-zicpKgigMvpAfugRpC6u6w@public.gmane.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, version 2.
+ */
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/slab.h>
+#include <linux/usb.h>
+#include <linux/i2c.h>
+
+#define DRIVER_NAME		"i2c-diolan-u2c"
+
+#define USB_VENDOR_ID_DIOLAN		0x0abf
+#define USB_DEVICE_ID_DIOLAN_U2C	0x3370
+
+#define DIOLAN_OUT_EP		0x02
+#define DIOLAN_IN_EP		0x84
+
+/* commands via USB, must match command ids in the firmware */
+#define CMD_I2C_READ		0x01
+#define CMD_I2C_WRITE		0x02
+#define CMD_I2C_SCAN		0x03	/* Returns list of detected devices */
+#define CMD_I2C_RELEASE_SDA	0x04
+#define CMD_I2C_RELEASE_SCL	0x05
+#define CMD_I2C_DROP_SDA	0x06
+#define CMD_I2C_DROP_SCL	0x07
+#define CMD_I2C_READ_SDA	0x08
+#define CMD_I2C_READ_SCL	0x09
+#define CMD_GET_FW_VERSION	0x0a
+#define CMD_GET_SERIAL		0x0b
+#define CMD_I2C_START		0x0c
+#define CMD_I2C_STOP		0x0d
+#define CMD_I2C_REPEATED_START	0x0e
+#define CMD_I2C_PUT_BYTE	0x0f
+#define CMD_I2C_GET_BYTE	0x10
+#define CMD_I2C_PUT_ACK		0x11
+#define CMD_I2C_GET_ACK		0x12
+#define CMD_I2C_PUT_BYTE_ACK	0x13
+#define CMD_I2C_GET_BYTE_ACK	0x14
+#define CMD_I2C_SET_SPEED	0x1b
+#define CMD_I2C_GET_SPEED	0x1c
+#define CMD_I2C_SET_CLK_SYNC	0x24
+#define CMD_I2C_GET_CLK_SYNC	0x25
+#define CMD_I2C_SET_CLK_SYNC_TO	0x26
+#define CMD_I2C_GET_CLK_SYNC_TO	0x27
+
+#define RESP_OK			0x00
+#define RESP_FAILED		0x01
+#define RESP_BAD_MEMADDR	0x04
+#define RESP_DATA_ERR		0x05
+#define RESP_NOT_IMPLEMENTED	0x06
+#define RESP_NACK		0x07
+#define RESP_TIMEOUT		0x09
+
+#define U2C_I2C_SPEED_FAST	0	/* 400 kHz */
+#define U2C_I2C_SPEED_STD	1	/* 100 kHz */
+#define U2C_I2C_SPEED_2KHZ	242	/* 2 kHz, minimum speed */
+#define U2C_I2C_SPEED(f)	((DIV_ROUND_CLOSEST(1000000, (f)) - 10) / 2 + 1)
+
+#define U2C_I2C_FREQ_FAST	400000
+#define U2C_I2C_FREQ_STD	100000
+#define U2C_I2C_FREQ(s)		(1000000 / (2 * (s - 1) + 10))
+
+#define DIOLAN_USB_TIMEOUT	100	/* in ms */
+#define DIOLAN_SYNC_TIMEOUT	10	/* in ms */
+
+#define DIOLAN_OUTBUF_LEN	128
+#define DIOLAN_FLUSH_LEN	(DIOLAN_OUTBUF_LEN - 4)
+#define DIOLAN_INBUF_LEN	257	/* Maximum supported receive length */
+
+/* Structure to hold all of our device specific stuff */
+struct i2c_diolan_u2c {
+	struct usb_device *usb_dev;	/* the usb device for this device */
+	struct usb_interface *interface;/* the interface for this device */
+	struct i2c_adapter adapter;	/* i2c related things */
+	int olen;			/* Output buffer length */
+	int ocount;			/* Number of enqueued messages */
+	u8 obuffer[DIOLAN_OUTBUF_LEN];	/* output buffer */
+	u8 ibuffer[DIOLAN_INBUF_LEN];	/* input buffer */
+};
+
+static uint frequency = 100000;	/* I2C clock frequency in Hz */
+
+module_param(frequency, uint, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(frequency, "I2C clock frequency in hertz");
+
+/* usb layer */
+
+/* Send command to device, and get response. */
+static int diolan_usb_transfer(struct i2c_diolan_u2c *dev)
+{
+	int ret = 0;
+	int actual;
+	int i;
+
+	if (!dev->olen || !dev->ocount)
+		return -EINVAL;
+
+	ret = usb_bulk_msg(dev->usb_dev,
+			   usb_sndbulkpipe(dev->usb_dev, DIOLAN_OUT_EP),
+			   dev->obuffer, dev->olen, &actual,
+			   DIOLAN_USB_TIMEOUT);
+	if (!ret) {
+		for (i = 0; i < dev->ocount; i++) {
+			int tmpret;
+
+			tmpret = usb_bulk_msg(dev->usb_dev,
+					      usb_rcvbulkpipe(dev->usb_dev,
+							      DIOLAN_IN_EP),
+					      dev->ibuffer,
+					      sizeof(dev->ibuffer), &actual,
+					      DIOLAN_USB_TIMEOUT);
+			if (tmpret < 0 && ret == 0)
+				ret = tmpret;
+			else if (ret >= 0 && tmpret == 0 && actual > 0) {
+				switch (dev->ibuffer[actual - 1]) {
+				case RESP_NACK:
+					ret = -ENXIO;
+					break;
+				case RESP_TIMEOUT:
+					ret = -ETIMEDOUT;
+					break;
+				case RESP_OK:
+					/* strip off return code */
+					ret = actual - 1;
+					break;
+				default:
+					ret = -EIO;
+					break;
+				}
+			}
+		}
+	}
+	dev->olen = 0;
+	dev->ocount = 0;
+	return ret;
+}
+
+/* Send command (no data) */
+static int diolan_usb_cmd(struct i2c_diolan_u2c *dev, u8 command, bool flush)
+{
+	int ret = 0;
+
+	dev->obuffer[dev->olen++] = command;
+	dev->ocount++;
+	if (flush || dev->olen >= DIOLAN_FLUSH_LEN)
+		ret = diolan_usb_transfer(dev);
+	return ret;
+}
+
+/* Send command with one byte of data */
+static int diolan_usb_cmd_data(struct i2c_diolan_u2c *dev, u8 command, u8 data,
+			       bool flush)
+{
+	int ret = 0;
+
+	dev->obuffer[dev->olen++] = command;
+	dev->obuffer[dev->olen++] = data;
+	dev->ocount++;
+	if (flush || dev->olen >= DIOLAN_FLUSH_LEN)
+		ret = diolan_usb_transfer(dev);
+	return ret;
+}
+
+/* Send command with two bytes of data */
+static int diolan_usb_cmd_data2(struct i2c_diolan_u2c *dev, u8 command, u8 d1,
+				u8 d2, bool flush)
+{
+	int ret = 0;
+
+	dev->obuffer[dev->olen++] = command;
+	dev->obuffer[dev->olen++] = d1;
+	dev->obuffer[dev->olen++] = d2;
+	dev->ocount++;
+	if (flush || dev->olen >= DIOLAN_FLUSH_LEN)
+		ret = diolan_usb_transfer(dev);
+	return ret;
+}
+
+/*
+ * Flush input queue.
+ * If we don't do this at startup and the controller has queued up
+ * messages which were not retrieved, it will stop responding
+ * at some point.
+ */
+static void diolan_flush_input(struct i2c_diolan_u2c *dev)
+{
+	int i;
+
+	for (i = 0; i < 10; i++) {
+		int actual = 0;
+		int ret;
+
+		ret = usb_bulk_msg(dev->usb_dev,
+				   usb_rcvbulkpipe(dev->usb_dev, DIOLAN_IN_EP),
+				   dev->ibuffer, sizeof(dev->ibuffer), &actual,
+				   DIOLAN_USB_TIMEOUT);
+		if (ret < 0 || actual == 0)
+			break;
+	}
+	if (i == 10)
+		dev_err(&dev->interface->dev, "Failed to flush input buffer\n");
+}
+
+static int diolan_i2c_start(struct i2c_diolan_u2c *dev)
+{
+	return diolan_usb_cmd(dev, CMD_I2C_START, false);
+}
+
+static int diolan_i2c_repeated_start(struct i2c_diolan_u2c *dev)
+{
+	return diolan_usb_cmd(dev, CMD_I2C_REPEATED_START, false);
+}
+
+static int diolan_i2c_stop(struct i2c_diolan_u2c *dev)
+{
+	return diolan_usb_cmd(dev, CMD_I2C_STOP, true);
+}
+
+static int diolan_i2c_get_byte_ack(struct i2c_diolan_u2c *dev, bool ack,
+				   u8 *byte)
+{
+	int ret;
+
+	ret = diolan_usb_cmd_data(dev, CMD_I2C_GET_BYTE_ACK, ack, true);
+	if (ret > 0)
+		*byte = dev->ibuffer[0];
+	else if (ret == 0)
+		ret = -EIO;
+
+	return ret;
+}
+
+static int diolan_i2c_put_byte_ack(struct i2c_diolan_u2c *dev, u8 byte)
+{
+	return diolan_usb_cmd_data(dev, CMD_I2C_PUT_BYTE_ACK, byte, false);
+}
+
+static int diolan_set_speed(struct i2c_diolan_u2c *dev, u8 speed)
+{
+	return diolan_usb_cmd_data(dev, CMD_I2C_SET_SPEED, speed, true);
+}
+
+/* Enable or disable clock synchronization (stretching) */
+static int diolan_set_clock_synch(struct i2c_diolan_u2c *dev, bool enable)
+{
+	return diolan_usb_cmd_data(dev, CMD_I2C_SET_CLK_SYNC, enable, true);
+}
+
+/* Set clock synchronization timeout in ms */
+static int diolan_set_clock_synch_timeout(struct i2c_diolan_u2c *dev, int ms)
+{
+	int to_val = ms * 10;
+
+	return diolan_usb_cmd_data2(dev, CMD_I2C_SET_CLK_SYNC_TO,
+				    to_val & 0xff, (to_val >> 8) & 0xff, true);
+}
+
+static int diolan_fw_version(struct i2c_diolan_u2c *dev)
+{
+	int ret;
+
+	ret = diolan_usb_cmd(dev, CMD_GET_FW_VERSION, true);
+	if (ret >= 2)
+		dev_info(&dev->interface->dev,
+			 "Diolan U2C firmware version %u.%u\n",
+			 (unsigned int)dev->ibuffer[0],
+			 (unsigned int)dev->ibuffer[1]);
+	return ret;
+}
+
+static int diolan_get_serial(struct i2c_diolan_u2c *dev)
+{
+	int ret;
+	u32 serial;
+
+	ret = diolan_usb_cmd(dev, CMD_GET_SERIAL, true);
+	if (ret >= 4) {
+		serial = le32_to_cpu(*(u32 *)dev->ibuffer);
+		dev_info(&dev->interface->dev,
+			 "Diolan U2C serial number %u\n", serial);
+	}
+	return ret;
+}
+
+static int diolan_init(struct i2c_diolan_u2c *dev)
+{
+	int speed, ret;
+
+	if (frequency >= 200000) {
+		speed = U2C_I2C_SPEED_FAST;
+		frequency = U2C_I2C_FREQ_FAST;
+	} else if (frequency >= 100000 || frequency == 0) {
+		speed = U2C_I2C_SPEED_STD;
+		frequency = U2C_I2C_FREQ_STD;
+	} else {
+		speed = U2C_I2C_SPEED(frequency);
+		if (speed > U2C_I2C_SPEED_2KHZ)
+			speed = U2C_I2C_SPEED_2KHZ;
+		frequency = U2C_I2C_FREQ(speed);
+	}
+
+	dev_info(&dev->interface->dev,
+		 "Diolan U2C at USB bus %03d address %03d speed %d Hz\n",
+		 dev->usb_dev->bus->busnum, dev->usb_dev->devnum, frequency);
+
+	diolan_flush_input(dev);
+	diolan_fw_version(dev);
+	diolan_get_serial(dev);
+
+	/* Set I2C speed */
+	ret = diolan_set_speed(dev, speed);
+	if (ret < 0)
+		return ret;
+
+	/* Configure I2C clock synchronization */
+	ret = diolan_set_clock_synch(dev, speed != U2C_I2C_SPEED_FAST);
+	if (ret < 0)
+		return ret;
+
+	if (speed != U2C_I2C_SPEED_FAST)
+		ret = diolan_set_clock_synch_timeout(dev, DIOLAN_SYNC_TIMEOUT);
+
+	return ret;
+}
+
+/* i2c layer */
+
+static int diolan_usb_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
+			   int num)
+{
+	struct i2c_diolan_u2c *dev = i2c_get_adapdata(adapter);
+	struct i2c_msg *pmsg;
+	int i, j;
+	int ret, sret;
+
+	ret = diolan_i2c_start(dev);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < num; i++) {
+		pmsg = &msgs[i];
+		if (i) {
+			ret = diolan_i2c_repeated_start(dev);
+			if (ret < 0)
+				goto abort;
+		}
+		if (pmsg->flags & I2C_M_RD) {
+			ret =
+			    diolan_i2c_put_byte_ack(dev, (pmsg->addr << 1) | 1);
+			if (ret < 0)
+				goto abort;
+			for (j = 0; j < pmsg->len; j++) {
+				u8 byte;
+				bool ack = j < pmsg->len - 1;
+
+				/*
+				 * Don't send NACK if this is the first byte
+				 * of a SMBUS_BLOCK message.
+				 */
+				if (j == 0 && (pmsg->flags & I2C_M_RECV_LEN))
+					ack = true;
+
+				ret = diolan_i2c_get_byte_ack(dev, ack, &byte);
+				if (ret < 0)
+					goto abort;
+				/*
+				 * Adjust count if first received byte is length
+				 */
+				if (j == 0 && (pmsg->flags & I2C_M_RECV_LEN)) {
+					if (byte == 0
+					    || byte > I2C_SMBUS_BLOCK_MAX) {
+						ret = -EPROTO;
+						goto abort;
+					}
+					pmsg->len += byte;
+				}
+				pmsg->buf[j] = byte;
+			}
+		} else {
+			ret = diolan_i2c_put_byte_ack(dev, pmsg->addr << 1);
+			if (ret < 0)
+				goto abort;
+			for (j = 0; j < pmsg->len; j++) {
+				ret = diolan_i2c_put_byte_ack(dev,
+							      pmsg->buf[j]);
+				if (ret < 0)
+					goto abort;
+			}
+		}
+	}
+abort:
+	sret = diolan_i2c_stop(dev);
+	if (sret < 0 && ret >= 0)
+		ret = sret;
+	return ret;
+}
+
+/*
+ * Return list of supported functionality.
+ */
+static u32 diolan_usb_func(struct i2c_adapter *a)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
+	       I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_BLOCK_PROC_CALL;
+}
+
+static const struct i2c_algorithm diolan_usb_algorithm = {
+	.master_xfer = diolan_usb_xfer,
+	.functionality = diolan_usb_func,
+};
+
+/* device layer */
+
+static const struct usb_device_id diolan_u2c_table[] = {
+	{ USB_DEVICE(USB_VENDOR_ID_DIOLAN, USB_DEVICE_ID_DIOLAN_U2C) },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(usb, diolan_u2c_table);
+
+static void diolan_u2c_free(struct i2c_diolan_u2c *dev)
+{
+	usb_put_dev(dev->usb_dev);
+	kfree(dev);
+}
+
+static int diolan_u2c_probe(struct usb_interface *interface,
+			    const struct usb_device_id *id)
+{
+	struct i2c_diolan_u2c *dev;
+	int ret;
+
+	/* allocate memory for our device state and initialize it */
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (dev == NULL) {
+		dev_err(&interface->dev, "Out of memory\n");
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	dev->usb_dev = usb_get_dev(interface_to_usbdev(interface));
+	dev->interface = interface;
+
+	/* save our data pointer in this interface device */
+	usb_set_intfdata(interface, dev);
+
+	/* setup i2c adapter description */
+	dev->adapter.owner = THIS_MODULE;
+	dev->adapter.class = I2C_CLASS_HWMON;
+	dev->adapter.algo = &diolan_usb_algorithm;
+	i2c_set_adapdata(&dev->adapter, dev);
+	snprintf(dev->adapter.name, sizeof(dev->adapter.name),
+		 DRIVER_NAME " at bus %03d device %03d",
+		 dev->usb_dev->bus->busnum, dev->usb_dev->devnum);
+
+	dev->adapter.dev.parent = &dev->interface->dev;
+
+	/* initialize diolan i2c interface */
+	ret = diolan_init(dev);
+	if (ret < 0)
+		goto error_free;
+
+	/* and finally attach to i2c layer */
+	ret = i2c_add_adapter(&dev->adapter);
+	if (ret < 0) {
+		dev_err(&interface->dev, "failed to add I2C adapter\n");
+		goto error_free;
+	}
+
+	dev_dbg(&interface->dev, "connected " DRIVER_NAME "\n");
+
+	return 0;
+
+error_free:
+	usb_set_intfdata(interface, NULL);
+	diolan_u2c_free(dev);
+error:
+	return ret;
+}
+
+static void diolan_u2c_disconnect(struct usb_interface *interface)
+{
+	struct i2c_diolan_u2c *dev = usb_get_intfdata(interface);
+
+	i2c_del_adapter(&dev->adapter);
+	usb_set_intfdata(interface, NULL);
+	diolan_u2c_free(dev);
+
+	dev_dbg(&interface->dev, "disconnected\n");
+}
+
+static struct usb_driver diolan_u2c_driver = {
+	.name = DRIVER_NAME,
+	.probe = diolan_u2c_probe,
+	.disconnect = diolan_u2c_disconnect,
+	.id_table = diolan_u2c_table,
+};
+
+static int __init diolan_u2c_init(void)
+{
+	/* register this driver with the USB subsystem */
+	return usb_register(&diolan_u2c_driver);
+}
+
+static void __exit diolan_u2c_exit(void)
+{
+	/* deregister this driver with the USB subsystem */
+	usb_deregister(&diolan_u2c_driver);
+}
+
+module_init(diolan_u2c_init);
+module_exit(diolan_u2c_exit);
+
+MODULE_AUTHOR("Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>");
+MODULE_DESCRIPTION(DRIVER_NAME " driver");
+MODULE_LICENSE("GPL");
-- 
1.7.3.1

[PATCH v2 2/2] MAINTAINERS: Add maintainer for Diolan U2C-12 I2C adapter driver

[Guenter Roeck]

Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
---
 MAINTAINERS |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 1af022e..12b0be3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2089,6 +2089,12 @@ F:	Documentation/serial/digiepca.txt
 F:	drivers/char/epca*
 F:	drivers/char/digi*
 
+DIOLAN U2C-12 I2C DRIVER
+M:	Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
+L:	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+S:	Maintained
+F:	drivers/i2c/busses/i2c-diolan-u2c.c
+
 DIRECTORY NOTIFICATION (DNOTIFY)
 M:	Eric Paris <eparis-FjpueFixGhCM4zKIHC2jIg@public.gmane.org>
 S:	Maintained
-- 
1.7.3.1

Re: [PATCH v2 1/2] i2c/busses: Diolan U2C-12 USB/I2C adapter driver

[Guenter Roeck]

On Wed, 2011-01-19 at 16:14 -0500, Ben Dooks wrote:
> On Wed, Jan 19, 2011 at 01:26:37PM -0800, Guenter Roeck wrote:
> > Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
> > ---
> >  drivers/i2c/busses/Kconfig          |   10 +
> >  drivers/i2c/busses/Makefile         |    1 +
> >  drivers/i2c/busses/i2c-diolan-u2c.c |  529 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 540 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/i2c/busses/i2c-diolan-u2c.c
> > 
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index 113505a..30f8dbd 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -648,6 +648,16 @@ config I2C_EG20T
> >  
> >  comment "External I2C/SMBus adapter drivers"
> >  
> > +config I2C_DIOLAN_U2C
> > +	tristate "Diolan U2C-12 USB adapter"
> > +	depends on USB
> > +	help
> > +	  If you say yes to this option, support will be included for Diolan
> > +	  U2C-12, a USB to I2C interface.
> > +
> > +	  This driver can also be built as a module.  If so, the module
> > +	  will be called i2c-diolan-u2c.
> > +
> >  config I2C_PARPORT
> >  	tristate "Parallel port adapter"
> >  	depends on PARPORT
> > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> > index 9d2d0ec..3c630b7 100644
> > --- a/drivers/i2c/busses/Makefile
> > +++ b/drivers/i2c/busses/Makefile
> > @@ -64,6 +64,7 @@ obj-$(CONFIG_I2C_XILINX)	+= i2c-xiic.o
> >  obj-$(CONFIG_I2C_EG20T)         += i2c-eg20t.o
> >  
> >  # External I2C/SMBus adapter drivers
> > +obj-$(CONFIG_I2C_DIOLAN_U2C)	+= i2c-diolan-u2c.o
> >  obj-$(CONFIG_I2C_PARPORT)	+= i2c-parport.o
> >  obj-$(CONFIG_I2C_PARPORT_LIGHT)	+= i2c-parport-light.o
> >  obj-$(CONFIG_I2C_TAOS_EVM)	+= i2c-taos-evm.o
> > diff --git a/drivers/i2c/busses/i2c-diolan-u2c.c b/drivers/i2c/busses/i2c-diolan-u2c.c
> > new file mode 100644
> > index 0000000..44905ce
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-diolan-u2c.c
> > @@ -0,0 +1,529 @@
> > +/*
> > + * Driver for the Diolan u2c-12 USB-I2C adapter
> > + *
> > + * Copyright (c) 2010-2011 Ericsson AB
> > + *
> > + * Derived from:
> > + *  i2c-tiny-usb.c
> > + *  Copyright (C) 2006-2007 Till Harbaum (Till-zicpKgigMvpAfugRpC6u6w@public.gmane.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, version 2.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/errno.h>
> > +#include <linux/module.h>
> > +#include <linux/types.h>
> > +#include <linux/slab.h>
> > +#include <linux/usb.h>
> > +#include <linux/i2c.h>
> > +
> > +#define DRIVER_NAME		"i2c-diolan-u2c"
> > +
> > +#define USB_VENDOR_ID_DIOLAN		0x0abf
> > +#define USB_DEVICE_ID_DIOLAN_U2C	0x3370
> > +
> 
> might be worth type-defing the commands and responses, but not especially
> important.
> 
I think it is useful.

> > +#define U2C_I2C_SPEED_FAST	0	/* 400 kHz */
> > +#define U2C_I2C_SPEED_STD	1	/* 100 kHz */
> > +#define U2C_I2C_SPEED_2KHZ	242	/* 2 kHz, minimum speed */
> > +#define U2C_I2C_SPEED(f)	((DIV_ROUND_CLOSEST(1000000, (f)) - 10) / 2 + 1)
> 
> how about rounding down? surely the speed requested is the maximum?
> 
No strong opinion either way. Rounding down would mean rounding up the
speed, though (higher number is lower speed). I'll round it up.

> > +/* Structure to hold all of our device specific stuff */
> > +struct i2c_diolan_u2c {
> > +	struct usb_device *usb_dev;	/* the usb device for this device */
> > +	struct usb_interface *interface;/* the interface for this device */
> > +	struct i2c_adapter adapter;	/* i2c related things */
> > +	int olen;			/* Output buffer length */
> > +	int ocount;			/* Number of enqueued messages */
> > +	u8 obuffer[DIOLAN_OUTBUF_LEN];	/* output buffer */
> > +	u8 ibuffer[DIOLAN_INBUF_LEN];	/* input buffer */
> 
> you should cache-line align the buffers to avoid any problems with
> dma-vs-cache, etc.
> 
Can you point me to some code doing that ? Or do you mean I should
allocate the buffers separately ?

> > +/* Send command to device, and get response. */
> > +static int diolan_usb_transfer(struct i2c_diolan_u2c *dev)
> > +{
> > +	int ret = 0;
> > +	int actual;
> > +	int i;
> > +
> > +	if (!dev->olen || !dev->ocount)
> > +		return -EINVAL;
> > +
> > +	ret = usb_bulk_msg(dev->usb_dev,
> > +			   usb_sndbulkpipe(dev->usb_dev, DIOLAN_OUT_EP),
> > +			   dev->obuffer, dev->olen, &actual,
> > +			   DIOLAN_USB_TIMEOUT);
> > +	if (!ret) {
> > +		for (i = 0; i < dev->ocount; i++) {
> > +			int tmpret;
> > +
> > +			tmpret = usb_bulk_msg(dev->usb_dev,
> > +					      usb_rcvbulkpipe(dev->usb_dev,
> > +							      DIOLAN_IN_EP),
> > +					      dev->ibuffer,
> > +					      sizeof(dev->ibuffer), &actual,
> > +					      DIOLAN_USB_TIMEOUT);
> > +			if (tmpret < 0 && ret == 0)
> > +				ret = tmpret;
> > +			else if (ret >= 0 && tmpret == 0 && actual > 0) {
> > +				switch (dev->ibuffer[actual - 1]) {
> > +				case RESP_NACK:
> > +					ret = -ENXIO;
> > +					break;
> > +				case RESP_TIMEOUT:
> > +					ret = -ETIMEDOUT;
> > +					break;
> > +				case RESP_OK:
> > +					/* strip off return code */
> > +					ret = actual - 1;
> > +					break;
> > +				default:
> > +					ret = -EIO;
> > +					break;
> > +				}
> > +			}
> > +		}
> > +	}
> 
> this looks a little complicated with ret and tmpret.
> 
Yes. Problem is that I have to retrieve all messages one by one even if
there was an error. I'll see if I can come up with something better, but
right now I have no idea how to simplify it.

> > +	dev->olen = 0;
> > +	dev->ocount = 0;
> > +	return ret;
> > +}
> > +
> 
> > +static int diolan_usb_cmd_data2(struct i2c_diolan_u2c *dev, u8 command, u8 d1,
> > +				u8 d2, bool flush)
> > +{
> > +	int ret = 0;
> > +
> > +	dev->obuffer[dev->olen++] = command;
> > +	dev->obuffer[dev->olen++] = d1;
> > +	dev->obuffer[dev->olen++] = d2;
> > +	dev->ocount++;
> > +	if (flush || dev->olen >= DIOLAN_FLUSH_LEN)
> > +		ret = diolan_usb_transfer(dev);
> > +	return ret;
> > +}
> 
> how about wrapping the check for usb_transfer into one call?
> say diolan_written_cmd(dev, flush) and then just use that as a
> return and remove any need for 'int ret = 0' as well.
> 
Ok.

> > +/*
> > + * Flush input queue.
> > + * If we don't do this at startup and the controller has queued up
> > + * messages which were not retrieved, it will stop responding
> > + * at some point.
> > + */
> > +static void diolan_flush_input(struct i2c_diolan_u2c *dev)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < 10; i++) {
> > +		int actual = 0;
> > +		int ret;
> > +
> > +		ret = usb_bulk_msg(dev->usb_dev,
> > +				   usb_rcvbulkpipe(dev->usb_dev, DIOLAN_IN_EP),
> > +				   dev->ibuffer, sizeof(dev->ibuffer), &actual,
> > +				   DIOLAN_USB_TIMEOUT);
> > +		if (ret < 0 || actual == 0)
> > +			break;
> > +	}
> > +	if (i == 10)
> > +		dev_err(&dev->interface->dev, "Failed to flush input buffer\n");
> 
> [snip]
> 
> 
> > +		 "Diolan U2C at USB bus %03d address %03d speed %d Hz\n",
> > +		 dev->usb_dev->bus->busnum, dev->usb_dev->devnum, frequency);
> > +
> > +	diolan_flush_input(dev);
> > +	diolan_fw_version(dev);
> 
> why bother returning ret from some of these when they're not checked?
> 
Good point. Made it void.

> > +	ret = diolan_i2c_start(dev);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	for (i = 0; i < num; i++) {
> > +		pmsg = &msgs[i];
> > +		if (i) {
> > +			ret = diolan_i2c_repeated_start(dev);
> > +			if (ret < 0)
> > +				goto abort;
> > +		}
> 
> i think this is ok.
> 
> > +	int ret;
> > +
> > +	/* allocate memory for our device state and initialize it */
> > +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > +	if (dev == NULL) {
> > +		dev_err(&interface->dev, "Out of memory\n");
> 
> how about "no memory for device state"
> 
Ok.

> > +		ret = -ENOMEM;
> > +		goto error;
> > +	}
> > +
> > +	dev->usb_dev = usb_get_dev(interface_to_usbdev(interface));
> > +	dev->interface = interface;
> > +
> > +	/* save our data pointer in this interface device */
> > +	usb_set_intfdata(interface, dev);
> > +
> > +	/* setup i2c adapter description */
> > +	dev->adapter.owner = THIS_MODULE;
> > +	dev->adapter.class = I2C_CLASS_HWMON;
> > +	dev->adapter.algo = &diolan_usb_algorithm;
> > +	i2c_set_adapdata(&dev->adapter, dev);
> > +	snprintf(dev->adapter.name, sizeof(dev->adapter.name),
> > +		 DRIVER_NAME " at bus %03d device %03d",
> > +		 dev->usb_dev->bus->busnum, dev->usb_dev->devnum);
> > +
> > +	dev->adapter.dev.parent = &dev->interface->dev;
> > +
> > +	/* initialize diolan i2c interface */
> > +	ret = diolan_init(dev);
> > +	if (ret < 0)
> > +		goto error_free;
> 
> do all return paths of diolan_init() print error?
> 
No. I'll add a message.

> > +
> > +	/* and finally attach to i2c layer */
> > +	ret = i2c_add_adapter(&dev->adapter);
> > +	if (ret < 0) {
> > +		dev_err(&interface->dev, "failed to add I2C adapter\n");
> > +		goto error_free;
> > +	}
> > +
> > +	dev_dbg(&interface->dev, "connected " DRIVER_NAME "\n");
> > +
> > +	return 0;
> > +
> > +error_free:
> > +	usb_set_intfdata(interface, NULL);
> > +	diolan_u2c_free(dev);
> > +error:
> > +	return ret;
> > +}
> > +
> 
Thanks a lot for the review!

Guenter

Re: [PATCH v2 1/2] i2c/busses: Diolan U2C-12 USB/I2C adapter driver

[Guenter Roeck]

On Wed, 2011-01-19 at 16:14 -0500, Ben Dooks wrote:
> On Wed, Jan 19, 2011 at 01:26:37PM -0800, Guenter Roeck wrote:
> > Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
 [ ... ]

> > +/* Structure to hold all of our device specific stuff */
> > +struct i2c_diolan_u2c {
> > +	struct usb_device *usb_dev;	/* the usb device for this device */
> > +	struct usb_interface *interface;/* the interface for this device */
> > +	struct i2c_adapter adapter;	/* i2c related things */
> > +	int olen;			/* Output buffer length */
> > +	int ocount;			/* Number of enqueued messages */
> > +	u8 obuffer[DIOLAN_OUTBUF_LEN];	/* output buffer */
> > +	u8 ibuffer[DIOLAN_INBUF_LEN];	/* input buffer */
> 
> you should cache-line align the buffers to avoid any problems with
> dma-vs-cache, etc.
> 
There are several other drivers which don't cache-align the buffers used
with usb_bulk_msg(). Do those all have potential problems, or is this
only relevant for optimization ?

If it is just for optimization, I'd argue that the speed here is so slow
that it won't make much of a difference, if any, if the buffers were
aligned.

Thanks,
Guenter

Re: [PATCH v2 2/2] MAINTAINERS: Add maintainer for Diolan U2C-12 I2C adapter driver

[Ben Dooks]

On Wed, Jan 19, 2011 at 01:10:44PM -0800, Guenter Roeck wrote:
> On Wed, 2011-01-19 at 15:56 -0500, Ben Dooks wrote:
> > On Wed, Jan 19, 2011 at 01:26:38PM -0800, Guenter Roeck wrote:
> > > Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
> > 
> > meaningful commit messages please.
> > 
> In general I agree, only if the headline says everything what else do
> you want me to say here ? That I am adding myself as maintainer ?

I would also like to see information about the maintainer, such as is
this a work related or personal project, and anything else that may
be relevant.
 
> Guenter
> 
> > > ---
> > >  MAINTAINERS |    6 ++++++
> > >  1 files changed, 6 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 1af022e..12b0be3 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -2089,6 +2089,12 @@ F:	Documentation/serial/digiepca.txt
> > >  F:	drivers/char/epca*
> > >  F:	drivers/char/digi*
> > >  
> > > +DIOLAN U2C-12 I2C DRIVER
> > > +M:	Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
> > > +L:	linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > +S:	Maintained
> > > +F:	drivers/i2c/busses/i2c-diolan-u2c.c
> > > +
> > >  DIRECTORY NOTIFICATION (DNOTIFY)
> > >  M:	Eric Paris <eparis-FjpueFixGhCM4zKIHC2jIg@public.gmane.org>
> > >  S:	Maintained
> > > -- 
> > > 1.7.3.1
> > > 
> > 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ben Dooks, ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

Re: [PATCH v2 1/2] i2c/busses: Diolan U2C-12 USB/I2C adapter driver

[Ben Dooks]

On Wed, Jan 19, 2011 at 02:16:13PM -0800, Guenter Roeck wrote:
> On Wed, 2011-01-19 at 16:14 -0500, Ben Dooks wrote:
> > On Wed, Jan 19, 2011 at 01:26:37PM -0800, Guenter Roeck wrote:
> > > Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
>  [ ... ]
> 
> > > +/* Structure to hold all of our device specific stuff */
> > > +struct i2c_diolan_u2c {
> > > +	struct usb_device *usb_dev;	/* the usb device for this device */
> > > +	struct usb_interface *interface;/* the interface for this device */
> > > +	struct i2c_adapter adapter;	/* i2c related things */
> > > +	int olen;			/* Output buffer length */
> > > +	int ocount;			/* Number of enqueued messages */
> > > +	u8 obuffer[DIOLAN_OUTBUF_LEN];	/* output buffer */
> > > +	u8 ibuffer[DIOLAN_INBUF_LEN];	/* input buffer */
> > 
> > you should cache-line align the buffers to avoid any problems with
> > dma-vs-cache, etc.
> > 
> There are several other drivers which don't cache-align the buffers used
> with usb_bulk_msg(). Do those all have potential problems, or is this
> only relevant for optimization ?
> 
> If it is just for optimization, I'd argue that the speed here is so slow
> that it won't make much of a difference, if any, if the buffers were
> aligned.

It is more to do with the way some architectures/cpus deal with cache
coherency with resepect to DMA... in some cases it is possible for hardware
to be dealing with a buffer that the cpu may still be using if they are
not appropriately aligned. I forget the exact case, but losing a few bytes
in a buffer is preferable to the other case.

I also believe that X86 style architectures do not have this problem.

-- 
Ben Dooks, ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.