[PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter

[PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter

[Guenter Roeck]

This patch adds support for the I2C interface of the Devantech USB-ISS
Multifunction adapter.

Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
---
The driver has one problem: It competes with the cdc_acm driver for device
access. Copying the usb mailing list in the hope that someone can tell me
if there is a way to prevent this from happening.

 Documentation/i2c/busses/i2c-devantech-iss |   31 ++
 drivers/i2c/busses/Kconfig                 |   10 +
 drivers/i2c/busses/Makefile                |    1 +
 drivers/i2c/busses/i2c-devantech-iss.c     |  495 ++++++++++++++++++++++++++++
 4 files changed, 537 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/i2c/busses/i2c-devantech-iss
 create mode 100644 drivers/i2c/busses/i2c-devantech-iss.c

diff --git a/Documentation/i2c/busses/i2c-devantech-iss b/Documentation/i2c/busses/i2c-devantech-iss
new file mode 100644
index 0000000..f8199fd
--- /dev/null
+++ b/Documentation/i2c/busses/i2c-devantech-iss
@@ -0,0 +1,31 @@
+Kernel driver i2c-devantech-iss
+
+Supported adapters:
+  * Devantech USB-ISS Multifunction USB Communications Module
+    Documentation:
+        http://www.robot-electronics.co.uk/htm/usb_iss_tech.htm
+
+Author: Guenter Roeck <guenter.roeck@ericsson.com>
+
+
+Description
+-----------
+
+This is the driver for the Devantech USB-ISS Multifunction USB Communications
+Module.
+
+The USB-ISS Multifunction USB Communications Module provides an interface
+to I2C, SPI, Serial port, and general purpose Analogue Input or Digital I/O.
+The module is powered from USB.
+
+The driver only supports the I2C interface of USB-ISS. The driver does not use
+interrupts.
+
+Clock stretching is supported for bus frequencies of 100,000 Hz and above.
+
+
+Module parameters
+-----------------
+
+* frequency: I2C bus frequency in Hz
+  Supported frequencies are 20,000, 50,000, 100,000, 400,000, and 1,000,000 Hz.
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 230601e..0bffacd 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -676,6 +676,16 @@ config I2C_EG20T
 
 comment "External I2C/SMBus adapter drivers"
 
+config I2C_DEVANTECH
+	tristate "Devantech USB-ISS"
+	select I2C_SMBUS
+	help
+	  This supports the I2C interface of the Devantech USB-ISS Multifunction
+	  Communications Module.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-devantech-iss.
+
 config I2C_PARPORT
 	tristate "Parallel port adapter"
 	depends on PARPORT
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 3878c95..5ecec28 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_I2C_XILINX)	+= i2c-xiic.o
 obj-$(CONFIG_I2C_EG20T)         += i2c-eg20t.o
 
 # External I2C/SMBus adapter drivers
+obj-$(CONFIG_I2C_DEVANTECH)	+= i2c-devantech-iss.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-devantech-iss.c b/drivers/i2c/busses/i2c-devantech-iss.c
new file mode 100644
index 0000000..b14daa6
--- /dev/null
+++ b/drivers/i2c/busses/i2c-devantech-iss.c
@@ -0,0 +1,495 @@
+/*
+ * Driver for the Devantech USB-ISS Multifunction Communications Module
+ *
+ * Copyright (c) 2011 Ericsson AB
+ *
+ * Derived from:
+ *  i2c-diolan-u2c.c
+ *  Copyright (c) 2010-2011 Ericsson AB
+ *
+ * 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-devantech-iss"
+
+#define USB_VENDOR_ID_DEVANTECH	0x04d8
+#define USB_DEVICE_ID_ISS	0xffee
+
+/* commands */
+#define ISS_CMD_I2C_DIRECT	0x57	/* Custom I2C sequence */
+#define ISS_CMD_I2C_INTERNAL	0x5a	/* internal commands */
+
+/* direct commands */
+#define ISS_I2C_START		0x01
+#define ISS_I2C_RESTART		0x02
+#define ISS_I2C_STOP		0x03
+#define ISS_I2C_NACK		0x04
+#define ISS_I2C_READ(n)		(0x20 + (n) - 1)
+#define ISS_I2C_WRITE(n)	(0x30 + (n) - 1)
+
+/* internal commands */
+#define ISS_GET_VERSION		0x01
+#define ISS_MODE		0x02
+#define ISS_GET_SERIAL		0x03
+
+/* modes */
+#define ISS_MODE_I2C_S_20KHZ	0x20	/* I2C, SW (bitbang), 20 kHz clock */
+#define ISS_MODE_I2C_S_50KHZ	0x30	/* I2C, SW (bitbang), 50 kHz clock */
+#define ISS_MODE_I2C_S_100KHZ	0x40	/* I2C, SW (bitbang), 100 kHz clock */
+#define ISS_MODE_I2C_S_400KHZ	0x50	/* I2C, SW (bitbang), 400 kHz clock */
+#define ISS_MODE_I2C_H_100KHZ	0x60	/* I2C, HW (PIC), 100 kHz clock */
+#define ISS_MODE_I2C_H_400KHZ	0x70	/* I2C, HW (PIC), 400 kHz clock */
+#define ISS_MODE_I2C_H_1000KHZ	0x80	/* I2C, HW (PIC), 1000 kHz clock */
+
+/* response codes */
+
+#define ISS_RESP_ACK		0xff
+#define ISS_RESP_NACK		0x00
+
+#define ISS_RESP_OK		0x00
+#define ISS_RESP_DEV_ERROR	0x01
+#define ISS_RESP_OVERFLOW	0x02
+#define ISS_RESP_UNDERFLOW	0x03
+#define ISS_RESP_UNKNOWN_CMD	0x04
+
+#define ISS_USB_TIMEOUT		100	/* in ms */
+
+#define ISS_MAX_TRANSFER_LEN	60
+
+/* Structure to hold all of our device specific stuff */
+struct i2c_devantech_iss {
+	u8 buffer[ISS_MAX_TRANSFER_LEN];/* rx and tx buffer */
+	struct usb_interface *usb_if;	/* device data interface */
+	struct usb_interface *interface;/* device control interface */
+	struct usb_device *usb_dev;	/* the usb device for this interface */
+	struct i2c_adapter adapter;	/* i2c related things */
+	int ep_out, ep_in;
+};
+
+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");
+
+static struct usb_driver devantech_iss_driver;
+
+/* usb layer */
+
+/* Send command to device, and get response. */
+static int devantech_usb_transfer(struct i2c_devantech_iss *dev, int len)
+{
+	int ret, actual;
+
+	if (len <= 0 || len > ISS_MAX_TRANSFER_LEN)
+		return -EINVAL;
+
+	ret = usb_bulk_msg(dev->usb_dev,
+			   usb_sndbulkpipe(dev->usb_dev, dev->ep_out),
+			   dev->buffer, len, &actual,
+			   ISS_USB_TIMEOUT);
+
+	if (!ret) {
+		ret = usb_bulk_msg(dev->usb_dev,
+				   usb_rcvbulkpipe(dev->usb_dev, dev->ep_in),
+				   dev->buffer, sizeof(dev->buffer),
+				   &actual, ISS_USB_TIMEOUT);
+		if (!ret && actual > 0)
+			ret = actual;
+	}
+	return ret;
+}
+
+static int devantech_set_speed(struct i2c_devantech_iss *dev, int speed)
+{
+	int ret;
+
+	dev->buffer[0] = ISS_CMD_I2C_INTERNAL;
+	dev->buffer[1] = ISS_MODE;
+	dev->buffer[2] = speed;
+	dev->buffer[3] = 0x04;
+
+	ret = devantech_usb_transfer(dev, 4);
+	if (ret < 0)
+		return ret;
+	if (!dev->buffer[0])
+		return -EINVAL;
+	return 0;
+}
+
+static void devantech_fw_version(struct i2c_devantech_iss *dev)
+{
+	int ret;
+
+	dev->buffer[0] = ISS_CMD_I2C_INTERNAL;
+	dev->buffer[1] = ISS_GET_VERSION;
+
+	ret = devantech_usb_transfer(dev, 2);
+	if (ret >= 3)
+		dev_info(&dev->interface->dev,
+			 "Devantech USB-ISS firmware version %u.%u\n",
+			 dev->buffer[0], dev->buffer[1]);
+	else
+		dev_err(&dev->interface->dev,
+			"Failed to get firmware version, error %d\n", ret);
+}
+
+static void devantech_get_serial(struct i2c_devantech_iss *dev)
+{
+	int ret;
+
+	dev->buffer[0] = ISS_CMD_I2C_INTERNAL;
+	dev->buffer[1] = ISS_GET_SERIAL;
+
+	ret = devantech_usb_transfer(dev, 2);
+	if (ret >= 8) {
+		dev->buffer[ret] = '\0';
+		dev_info(&dev->interface->dev,
+			 "Devantech USS-ISS serial number %s\n", dev->buffer);
+	} else {
+		dev_err(&dev->interface->dev,
+			"Failed to get serial number, error %d\n", ret);
+	}
+}
+
+static int devantech_init(struct i2c_devantech_iss *dev)
+{
+	int speed;
+
+	/*
+	 * Software (bit-bang) I2C implementation does not support I2C clock
+	 * stretching, at least not with firmware version 7.2. The PIC hardware
+	 * implementation supports it, so select the hardware protocol if
+	 * available for a given bus speed.
+	 * If the bus speed is not configured or set to 0, select 100kHz.
+	 */
+	if (frequency >= 1000000) {
+		speed = ISS_MODE_I2C_H_1000KHZ;
+		frequency = 1000000;
+	} else if (frequency >= 400000) {
+		speed = ISS_MODE_I2C_H_400KHZ;
+		frequency = 400000;
+	} else if (frequency >= 100000 || frequency == 0) {
+		speed = ISS_MODE_I2C_H_100KHZ;
+		frequency = 100000;
+	} else if (frequency >= 50000) {
+		speed = ISS_MODE_I2C_S_50KHZ;
+		frequency = 50000;
+	} else {
+		speed = ISS_MODE_I2C_S_20KHZ;
+		frequency = 20000;
+	}
+
+	dev_info(&dev->interface->dev,
+		 "Devantech USB-ISS at USB bus %03d address %03d speed %d Hz\n",
+		 dev->usb_dev->bus->busnum, dev->usb_dev->devnum, frequency);
+
+	devantech_fw_version(dev);
+	devantech_get_serial(dev);
+
+	return devantech_set_speed(dev, speed);
+}
+
+/* i2c layer */
+
+static int devantech_usb_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
+			      int num)
+{
+	struct i2c_devantech_iss *dev = i2c_get_adapdata(adapter);
+	struct i2c_msg *pmsg = NULL;
+	int i, j, len, rlen;
+	int ret;
+
+	len = 0;
+	dev->buffer[len++] = ISS_CMD_I2C_DIRECT;
+	dev->buffer[len++] = ISS_I2C_START;
+
+	for (i = 0; i < num; i++) {
+		pmsg = &msgs[i];
+		if (i)
+			dev->buffer[len++] = ISS_I2C_RESTART;
+
+		if (pmsg->flags & I2C_M_RD) {
+			dev->buffer[len++] = ISS_I2C_WRITE(1);
+			dev->buffer[len++] = (pmsg->addr << 1) | 1;
+			if (pmsg->flags & I2C_M_RECV_LEN) {
+				/*
+				 * Block read returns receive length in 1st
+				 * byte. We must read up to 33 bytes of data
+				 * (len + 32) to avoid adapter hangup.
+				 */
+				dev->buffer[len++] = ISS_I2C_READ(16);
+				dev->buffer[len++] = ISS_I2C_READ(16);
+				dev->buffer[len++] = ISS_I2C_NACK;
+				dev->buffer[len++] = ISS_I2C_READ(1);
+			} else {
+				rlen = pmsg->len;
+				/*
+				 * repeat read sequence until done or out of
+				 * buffer
+				 */
+				if (rlen > ISS_MAX_TRANSFER_LEN - 8)
+					rlen = ISS_MAX_TRANSFER_LEN - 8;
+				while (rlen > 16) {
+					dev->buffer[len++] = ISS_I2C_READ(16);
+					rlen -= 16;
+				}
+				if (rlen > 1)
+					dev->buffer[len++]
+					  = ISS_I2C_READ(rlen - 1);
+				dev->buffer[len++] = ISS_I2C_NACK;
+				dev->buffer[len++] = ISS_I2C_READ(1);
+			}
+		} else {
+			dev->buffer[len++] = ISS_I2C_WRITE(1);
+			dev->buffer[len++] = pmsg->addr << 1;
+			/* Repeat write sequence until done */
+			for (rlen = 0; rlen < pmsg->len; rlen += 16) {
+				int clen = min(pmsg->len - rlen, 16);
+				/*
+				 * Make sure we have enough buffer space. We
+				 * need two additional bytes, one for the write
+				 * command itself and one for the STOP command.
+				 */
+				if (len + clen > ISS_MAX_TRANSFER_LEN - 2) {
+					ret = -EINVAL;
+					goto error;
+				}
+				dev->buffer[len++] = ISS_I2C_WRITE(clen);
+				for (j = 0; j < clen; j++)
+					dev->buffer[len++]
+					  = pmsg->buf[rlen + j];
+			}
+		}
+	}
+	dev->buffer[len++] = ISS_I2C_STOP;
+
+	ret = devantech_usb_transfer(dev, len);
+	if (ret < 0)
+		goto error;
+
+	if (ret < 2) {
+		ret = -EIO;
+		goto error;
+	}
+
+	if (dev->buffer[0] == ISS_RESP_NACK) {
+		ret = dev->buffer[1] == ISS_RESP_DEV_ERROR ? -ENXIO : -EIO;
+		goto error;
+	}
+
+	/*
+	 * Copy received data back into receive buffer. pmsg points to it.
+	 */
+	if (pmsg->flags & I2C_M_RD) {
+		if (pmsg->flags & I2C_M_RECV_LEN) {
+			rlen = dev->buffer[2];
+			if (rlen == 0 || rlen > I2C_SMBUS_BLOCK_MAX ||
+			    rlen > ret - 2) {
+				ret = -EPROTO;
+				goto error;
+			}
+			pmsg->len += rlen;
+			len = pmsg->len;
+		} else {
+			len = ret - 2;
+		}
+		for (i = 0; i < len; i++)
+			pmsg->buf[i] = dev->buffer[i + 2];
+	}
+	ret = 0;
+error:
+	return ret;
+}
+
+/*
+ * Return list of supported functionality.
+ */
+static u32 devantech_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 devantech_usb_algorithm = {
+	.master_xfer = devantech_usb_xfer,
+	.functionality = devantech_usb_func,
+};
+
+/* device layer */
+
+static const struct usb_device_id devantech_iss_table[] = {
+	{ USB_DEVICE(USB_VENDOR_ID_DEVANTECH, USB_DEVICE_ID_ISS) },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(usb, devantech_iss_table);
+
+static void devantech_iss_free(struct i2c_devantech_iss *dev)
+{
+	usb_put_dev(dev->usb_dev);
+	kfree(dev);
+}
+
+static int devantech_iss_probe(struct usb_interface *interface,
+			       const struct usb_device_id *id)
+{
+	struct i2c_devantech_iss *dev;
+	int ret;
+	struct usb_interface *usb_if;
+	struct usb_host_endpoint *ep_out, *ep_in;
+
+	/* allocate memory for our device state and initialize it */
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (dev == NULL) {
+		dev_err(&interface->dev, "no memory for device state\n");
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	dev->usb_dev = usb_get_dev(interface_to_usbdev(interface));
+	dev->interface = interface;
+
+	/* setup i2c adapter description */
+	dev->adapter.owner = THIS_MODULE;
+	dev->adapter.class = I2C_CLASS_HWMON;
+	dev->adapter.algo = &devantech_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;
+
+	/* Only accept probe on control interface */
+	usb_if = usb_ifnum_to_if(dev->usb_dev, 0);
+	if (usb_if != interface) {
+		dev_err(&interface->dev, "not on control interface\n");
+		ret = -ENODEV;
+		goto error_free;
+	}
+
+	usb_if = usb_ifnum_to_if(dev->usb_dev, 1);
+	if (!usb_if) {
+		dev_err(&interface->dev, "no USB interface\n");
+		ret = -ENODEV;
+		goto error_free;
+	}
+	if (usb_interface_claimed(usb_if)) {
+		dev_err(&interface->dev, "USB interface busy\n");
+		ret = -EBUSY;
+		goto error_free;
+	}
+	if (usb_if->cur_altsetting->desc.bNumEndpoints < 2) {
+		dev_err(&interface->dev,
+			"insufficient number of USB endpoints\n");
+		ret = -EINVAL;
+		goto error_free;
+	}
+
+	ep_out = &usb_if->cur_altsetting->endpoint[0];
+	ep_in = &usb_if->cur_altsetting->endpoint[1];
+	if (!ep_out || !ep_in) {
+		dev_err(&interface->dev, "missing USB endpoints\n");
+		ret = -EINVAL;
+		goto error_free;
+	}
+	dev->ep_out = ep_out->desc.bEndpointAddress;
+	dev->ep_in = ep_in->desc.bEndpointAddress;
+	dev->usb_if = usb_get_intf(usb_if);
+
+	/*
+	 * We need to claim the data interface to prevent other drivers
+	 * from accessing and registering it.
+	 */
+	ret = usb_driver_claim_interface(&devantech_iss_driver,
+					 dev->usb_if, dev);
+	if (ret < 0) {
+		dev_err(&usb_if->dev, "failed to claim interface\n");
+		goto error_put;
+	}
+
+	/* save our data pointer in this interface device */
+	usb_set_intfdata(interface, dev);
+
+	/* initialize devantech i2c interface */
+	ret = devantech_init(dev);
+	if (ret < 0) {
+		dev_err(&interface->dev, "failed to initialize adapter\n");
+		goto error_release;
+	}
+
+	/* 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_release;
+	}
+
+	dev_info(&interface->dev, "connected\n");
+	return 0;
+
+error_release:
+	usb_set_intfdata(interface, NULL);
+	usb_set_intfdata(dev->usb_if, NULL);
+	usb_driver_release_interface(&devantech_iss_driver, dev->usb_if);
+error_put:
+	usb_put_intf(dev->usb_if);
+error_free:
+	devantech_iss_free(dev);
+error:
+	return ret;
+}
+
+static void devantech_iss_disconnect(struct usb_interface *interface)
+{
+	struct i2c_devantech_iss *dev = usb_get_intfdata(interface);
+	struct usb_host_interface *alt = interface->cur_altsetting;
+
+	if (alt->desc.bInterfaceNumber)
+		return;
+
+	i2c_del_adapter(&dev->adapter);
+	usb_set_intfdata(interface, NULL);
+	usb_set_intfdata(dev->usb_if, NULL);
+	usb_driver_release_interface(&devantech_iss_driver, dev->usb_if);
+	usb_put_intf(dev->usb_if);
+	devantech_iss_free(dev);
+
+	dev_info(&interface->dev, "disconnected\n");
+}
+
+static struct usb_driver devantech_iss_driver = {
+	.name = DRIVER_NAME,
+	.probe = devantech_iss_probe,
+	.disconnect = devantech_iss_disconnect,
+	.id_table = devantech_iss_table,
+};
+
+static int __init devantech_iss_init(void)
+{
+	/* register this driver with the USB subsystem */
+	return usb_register(&devantech_iss_driver);
+}
+
+static void __exit devantech_iss_exit(void)
+{
+	/* deregister this driver with the USB subsystem */
+	usb_deregister(&devantech_iss_driver);
+}
+
+module_init(devantech_iss_init);
+module_exit(devantech_iss_exit);
+
+MODULE_AUTHOR("Guenter Roeck <guenter.roeck@ericsson.com>");
+MODULE_DESCRIPTION(DRIVER_NAME " driver");
+MODULE_LICENSE("GPL");
-- 
1.7.3.1

Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter

[Greg KH]

On Tue, Mar 22, 2011 at 08:43:47PM -0700, Guenter Roeck wrote:
> This patch adds support for the I2C interface of the Devantech USB-ISS
> Multifunction adapter.
> 
> Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
> ---
> The driver has one problem: It competes with the cdc_acm driver for device
> access. Copying the usb mailing list in the hope that someone can tell me
> if there is a way to prevent this from happening.

Why does it "compete"?
Is it because this driver also exposes a cdc-acm class interface?  Why
is it doing that if it doesn't follow that spec?  We do already support
a number of "quirks" in the cdc-acm driver, I don't see why we couldn't
add a "blacklist this device" one there as well to help you out here if
it's needed.

thanks,

greg k-h

Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter

[Guenter Roeck]

On Tue, Mar 22, 2011 at 11:45:24PM -0400, Greg KH wrote:
> On Tue, Mar 22, 2011 at 08:43:47PM -0700, Guenter Roeck wrote:
> > This patch adds support for the I2C interface of the Devantech USB-ISS
> > Multifunction adapter.
> > 
> > Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
> > ---
> > The driver has one problem: It competes with the cdc_acm driver for device
> > access. Copying the usb mailing list in the hope that someone can tell me
> > if there is a way to prevent this from happening.
> 
> Why does it "compete"?
> Is it because this driver also exposes a cdc-acm class interface?  Why

I guess so.

> is it doing that if it doesn't follow that spec?  We do already support

I don't know if it follows the cdc-acm specification or not, though I would
think it does since the cdc-acm driver recognizes it. I do see the
"This device cannot do calls on its own. It is not a modem." message.

I suspect the device exposes the cdc-acm class interface because it is
a convenient means to make it show up as COM port in Windows.
But I am not associated with the manufacturer, so that is just a wild guess.

If it does follow the cdc-acm specification, does that help me anything ?

> a number of "quirks" in the cdc-acm driver, I don't see why we couldn't
> add a "blacklist this device" one there as well to help you out here if
> it's needed.
> 
Maybe, but only as last resort.

Thanks,
Guenter

Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter

[Greg KH]

On Tue, Mar 22, 2011 at 09:25:07PM -0700, Guenter Roeck wrote:
> On Tue, Mar 22, 2011 at 11:45:24PM -0400, Greg KH wrote:
> > On Tue, Mar 22, 2011 at 08:43:47PM -0700, Guenter Roeck wrote:
> > > This patch adds support for the I2C interface of the Devantech USB-ISS
> > > Multifunction adapter.
> > > 
> > > Signed-off-by: Guenter Roeck <guenter.roeck@ericsson.com>
> > > ---
> > > The driver has one problem: It competes with the cdc_acm driver for device
> > > access. Copying the usb mailing list in the hope that someone can tell me
> > > if there is a way to prevent this from happening.
> > 
> > Why does it "compete"?
> > Is it because this driver also exposes a cdc-acm class interface?  Why
> 
> I guess so.
> 
> > is it doing that if it doesn't follow that spec?  We do already support
> 
> I don't know if it follows the cdc-acm specification or not, though I would
> think it does since the cdc-acm driver recognizes it. I do see the
> "This device cannot do calls on its own. It is not a modem." message.
> 
> I suspect the device exposes the cdc-acm class interface because it is
> a convenient means to make it show up as COM port in Windows.
> But I am not associated with the manufacturer, so that is just a wild guess.
> 
> If it does follow the cdc-acm specification, does that help me anything ?

It should, you can talk to the device from userspace using that virtual
serial port, right?  Isn't that what you need to do here?

Why try to create a new type of interface to the device from what it was
supposed to be by the manufacturer?

thanks,

greg k-h

Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter

[Bernhard Walle]

* Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org> [2011-03-23 05:50]:
> 
> It should, you can talk to the device from userspace using that virtual
> serial port, right?  Isn't that what you need to do here?
> 
> Why try to create a new type of interface to the device from what it was
> supposed to be by the manufacturer?

The device is multi-function. It's quite common to use some (extended)
USB-to-serial-converters also for serial busses like I²C or SPI. I know
that from the FT2232C chip.

Using such a device as normal Linux I²C device makes perfectly sense,
IMO.


Regards,
Bernhard

Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter

[Guenter Roeck]

On Wed, Mar 23, 2011 at 01:41:53AM -0400, Bernhard Walle wrote:
> * Greg KH <greg@kroah.com> [2011-03-23 05:50]:
> > 
> > It should, you can talk to the device from userspace using that virtual
> > serial port, right?  Isn't that what you need to do here?
> > 
> > Why try to create a new type of interface to the device from what it was
> > supposed to be by the manufacturer?
> 
> The device is multi-function. It's quite common to use some (extended)
> USB-to-serial-converters also for serial busses like I²C or SPI. I know
> that from the FT2232C chip.
> 
> Using such a device as normal Linux I²C device makes perfectly sense,
> IMO.
> 
Correct. I use and need it as I2C bus controller. FWIW, that is what it is sold
for. User space access doesn't help me there.

Anyway, the only reason for submitting the driver was that I thought it might
be useful for others. Maybe not useful enough, and defintely not worth arguing
about it. I'll make it available as stand-alone driver; that should be good enough.

Thanks,
Guenter

Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter

[Oliver Neukum]

Am Mittwoch, 23. März 2011, 06:41:53 schrieb Bernhard Walle:
> * Greg KH <greg@kroah.com> [2011-03-23 05:50]:
> > 
> > It should, you can talk to the device from userspace using that virtual
> > serial port, right?  Isn't that what you need to do here?
> > 
> > Why try to create a new type of interface to the device from what it was
> > supposed to be by the manufacturer?
> 
> The device is multi-function. It's quite common to use some (extended)
> USB-to-serial-converters also for serial busses like I²C or SPI. I know
> that from the FT2232C chip.
> 
> Using such a device as normal Linux I²C device makes perfectly sense,
> IMO.

Yes, but not the approach of kicking off the class driver. As those devices
presumably use some form of I2C over Serial, you ought to handle them in the
way PPP handles IP over Serial, especially if thy use a common encapsulation.

	Regards
		Oliver

Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter

[Alan Cox]

On Tue, 22 Mar 2011 20:43:47 -0700
Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org> wrote:

> This patch adds support for the I2C interface of the Devantech USB-ISS
> Multifunction adapter.
> 
> Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
> ---
> The driver has one problem: It competes with the cdc_acm driver for device
> access. Copying the usb mailing list in the hope that someone can tell me
> if there is a way to prevent this from happening.

>From the look of the code you actually want to be sitting on top of
cdc_acm. 


> +/* Send command to device, and get response. */
> +static int devantech_usb_transfer(struct i2c_devantech_iss *dev, int len)
> +{
> +	int ret, actual;
> +
> +	if (len <= 0 || len > ISS_MAX_TRANSFER_LEN)
> +		return -EINVAL;
> +
> +	ret = usb_bulk_msg(dev->usb_dev,
> +			   usb_sndbulkpipe(dev->usb_dev, dev->ep_out),
> +			   dev->buffer, len, &actual,
> +			   ISS_USB_TIMEOUT);
> +
> +	if (!ret) {
> +		ret = usb_bulk_msg(dev->usb_dev,
> +				   usb_rcvbulkpipe(dev->usb_dev, dev->ep_in),
> +				   dev->buffer, sizeof(dev->buffer),
> +				   &actual, ISS_USB_TIMEOUT);
> +		if (!ret && actual > 0)
> +			ret = actual;
> +	}
> +	return ret;
> +}

The whole driver seems to boil down to sending and receiving message
blocks to the bulk endpoint - yes ?

> +	dev->usb_dev = usb_get_dev(interface_to_usbdev(interface));
> +	dev->interface = interface;
> +
> +	/* setup i2c adapter description */
> +	dev->adapter.owner = THIS_MODULE;
> +	dev->adapter.class = I2C_CLASS_HWMON;

Doesn't seem to be HWMON ?


The rest looks like the device doesn't pretend to be CDC ACM class, but
is CDC-ACM class with the "device" the other end talking to the other end
of the serial port.

In fact it would be an interesting test to open the serial port and write
your commands to it from user space to see what happens. If I read the
code right it'll work fine.

>From a tty perspective it would probably make sense to adjust the driver
to be a line discipline sat on CDC-ACM.

I am *very* wary of removing it from the CDC_ACM side because people may
be using the CDC-ACM userspace directly for existing projects and would
get a nasty surprise and breakage.

If setting the tty to raw 8bit noecho and firing commands over the
virtual interface works then turning your driver into an ldisc is pretty
trivial. The ldisc interfaces provide data received and allow data writes
through the relevant tty.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter

[Guenter Roeck]

On Wed, Mar 23, 2011 at 05:28:29AM -0400, Alan Cox wrote:
> On Tue, 22 Mar 2011 20:43:47 -0700
> Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org> wrote:
> 
> > This patch adds support for the I2C interface of the Devantech USB-ISS
> > Multifunction adapter.
> > 
> > Signed-off-by: Guenter Roeck <guenter.roeck-IzeFyvvaP7pWk0Htik3J/w@public.gmane.org>
> > ---
> > The driver has one problem: It competes with the cdc_acm driver for device
> > access. Copying the usb mailing list in the hope that someone can tell me
> > if there is a way to prevent this from happening.
> 
> From the look of the code you actually want to be sitting on top of
> cdc_acm. 
> 
Ok, I'll see if I can do that.

> 
> > +/* Send command to device, and get response. */
> > +static int devantech_usb_transfer(struct i2c_devantech_iss *dev, int len)
> > +{
> > +	int ret, actual;
> > +
> > +	if (len <= 0 || len > ISS_MAX_TRANSFER_LEN)
> > +		return -EINVAL;
> > +
> > +	ret = usb_bulk_msg(dev->usb_dev,
> > +			   usb_sndbulkpipe(dev->usb_dev, dev->ep_out),
> > +			   dev->buffer, len, &actual,
> > +			   ISS_USB_TIMEOUT);
> > +
> > +	if (!ret) {
> > +		ret = usb_bulk_msg(dev->usb_dev,
> > +				   usb_rcvbulkpipe(dev->usb_dev, dev->ep_in),
> > +				   dev->buffer, sizeof(dev->buffer),
> > +				   &actual, ISS_USB_TIMEOUT);
> > +		if (!ret && actual > 0)
> > +			ret = actual;
> > +	}
> > +	return ret;
> > +}
> 
> The whole driver seems to boil down to sending and receiving message
> blocks to the bulk endpoint - yes ?
> 
Yes

> > +	dev->usb_dev = usb_get_dev(interface_to_usbdev(interface));
> > +	dev->interface = interface;
> > +
> > +	/* setup i2c adapter description */
> > +	dev->adapter.owner = THIS_MODULE;
> > +	dev->adapter.class = I2C_CLASS_HWMON;
> 
> Doesn't seem to be HWMON ?
> 
You lost me there. Why not ? Many if not all I2C bus drivers register as
I2C_CLASS_HWMON.

> 
> The rest looks like the device doesn't pretend to be CDC ACM class, but
> is CDC-ACM class with the "device" the other end talking to the other end
> of the serial port.
> 
> In fact it would be an interesting test to open the serial port and write
> your commands to it from user space to see what happens. If I read the
> code right it'll work fine.
> 
Yes, it should work. That is what is done in Windows.

> From a tty perspective it would probably make sense to adjust the driver
> to be a line discipline sat on CDC-ACM.
> 
> I am *very* wary of removing it from the CDC_ACM side because people may
> be using the CDC-ACM userspace directly for existing projects and would
> get a nasty surprise and breakage.
> 
Same here. Not likely for existing projects (the device is brand new),
but at some point someone might want to use it.

> If setting the tty to raw 8bit noecho and firing commands over the
> virtual interface works then turning your driver into an ldisc is pretty
> trivial. The ldisc interfaces provide data received and allow data writes
> through the relevant tty.
> 
I'll try.

Thanks,
Guenter

Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter

[Alan Cox]

> > > +	/* setup i2c adapter description */
> > > +	dev->adapter.owner = THIS_MODULE;
> > > +	dev->adapter.class = I2C_CLASS_HWMON;
> > 
> > Doesn't seem to be HWMON ?
> > 
> You lost me there. Why not ? Many if not all I2C bus drivers register as
> I2C_CLASS_HWMON.

Would you expect platform hardware monitoring devices to be hanging off
it ?

> > If setting the tty to raw 8bit noecho and firing commands over the
> > virtual interface works then turning your driver into an ldisc is pretty
> > trivial. The ldisc interfaces provide data received and allow data writes
> > through the relevant tty.
> > 
> I'll try.

Any queries on the ldisc internals drop me an email

Alan

Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter

[Guenter Roeck]

On Wed, Mar 23, 2011 at 09:52:44AM -0400, Alan Cox wrote:
> > > > +	/* setup i2c adapter description */
> > > > +	dev->adapter.owner = THIS_MODULE;
> > > > +	dev->adapter.class = I2C_CLASS_HWMON;
> > > 
> > > Doesn't seem to be HWMON ?
> > > 
> > You lost me there. Why not ? Many if not all I2C bus drivers register as
> > I2C_CLASS_HWMON.
> 
> Would you expect platform hardware monitoring devices to be hanging off
> it ?
> 
Yes. That is actually my primary use for it - to connect to various hwmon devices.
This way I can connect to prototype boards to develop drivers, and I don't have
to wait for HW to come in.

Guenter

Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter

[Greg KH]

On Tue, Mar 22, 2011 at 11:07:46PM -0700, Guenter Roeck wrote:
> On Wed, Mar 23, 2011 at 01:41:53AM -0400, Bernhard Walle wrote:
> > * Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org> [2011-03-23 05:50]:
> > > 
> > > It should, you can talk to the device from userspace using that virtual
> > > serial port, right?  Isn't that what you need to do here?
> > > 
> > > Why try to create a new type of interface to the device from what it was
> > > supposed to be by the manufacturer?
> > 
> > The device is multi-function. It's quite common to use some (extended)
> > USB-to-serial-converters also for serial busses like I²C or SPI. I know
> > that from the FT2232C chip.
> > 
> > Using such a device as normal Linux I²C device makes perfectly sense,
> > IMO.
> > 
> Correct. I use and need it as I2C bus controller. FWIW, that is what it is sold
> for. User space access doesn't help me there.
> 
> Anyway, the only reason for submitting the driver was that I thought it might
> be useful for others. Maybe not useful enough, and defintely not worth arguing
> about it. I'll make it available as stand-alone driver; that should be good enough.

No need for that, we want drivers in the kernel tree.

But, if this just duplicates the cdc-acm interface, and it looks like it
does, I don't think you need a kernel driver at all for it.  Just use
userspace commands to the serial port, like you do on Windows, and all
should be fine, right?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter

[Alan Cox]

> But, if this just duplicates the cdc-acm interface, and it looks like it
> does, I don't think you need a kernel driver at all for it.  Just use
> userspace commands to the serial port, like you do on Windows, and all
> should be fine, right?

You want a kernel driver because you've got kernel I²C drivers wanting to
talk to the devices down the link in question. Just needs to be an ldisc
and then it call comes together nicely.

Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter

[Greg KH]

On Wed, Mar 23, 2011 at 03:10:21PM +0000, Alan Cox wrote:
> > But, if this just duplicates the cdc-acm interface, and it looks like it
> > does, I don't think you need a kernel driver at all for it.  Just use
> > userspace commands to the serial port, like you do on Windows, and all
> > should be fine, right?
> 
> You want a kernel driver because you've got kernel I²C drivers wanting to
> talk to the devices down the link in question. Just needs to be an ldisc
> and then it call comes together nicely.

Ah, ok, that makes more sense.

Odd that windows does it all from userspace, but I guess it doesn't have
an I2C driver subsystem.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter

[Mark Brown]

On Wed, Mar 23, 2011 at 08:31:15AM -0700, Greg KH wrote:

> Odd that windows does it all from userspace, but I guess it doesn't have
> an I2C driver subsystem.

These devices are typically intended to be used for diagnosis evaluation
of hardware so you want to drive the device from a pointy clicky GUI -
they're not terribly useful in production systems.

Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter

[Guenter Roeck]

On Wed, Mar 23, 2011 at 11:31:15AM -0400, Greg KH wrote:
> On Wed, Mar 23, 2011 at 03:10:21PM +0000, Alan Cox wrote:
> > > But, if this just duplicates the cdc-acm interface, and it looks like it
> > > does, I don't think you need a kernel driver at all for it.  Just use
> > > userspace commands to the serial port, like you do on Windows, and all
> > > should be fine, right?
> > 
> > You want a kernel driver because you've got kernel I²C drivers wanting to
> > talk to the devices down the link in question. Just needs to be an ldisc
> > and then it call comes together nicely.
> 
> Ah, ok, that makes more sense.
> 
> Odd that windows does it all from userspace, but I guess it doesn't have
> an I2C driver subsystem.
> 
Converting the driver to a line discipline as Alan suggested would mean to add 
a new line discipline to linux/tty.h.

Doing this would be purely for the benefit of the community, since the driver
works just fine for my purpose.

So before I spend any more time on it, I'll need to know ... is there any
interest in an i2c bus driver for this device, and would a new line discipline
be accepted into the kernel ?

Thanks,
Guenter

Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter

[Greg KH]

On Wed, Mar 23, 2011 at 09:31:57PM -0700, Guenter Roeck wrote:
> On Wed, Mar 23, 2011 at 11:31:15AM -0400, Greg KH wrote:
> > On Wed, Mar 23, 2011 at 03:10:21PM +0000, Alan Cox wrote:
> > > > But, if this just duplicates the cdc-acm interface, and it looks like it
> > > > does, I don't think you need a kernel driver at all for it.  Just use
> > > > userspace commands to the serial port, like you do on Windows, and all
> > > > should be fine, right?
> > > 
> > > You want a kernel driver because you've got kernel I²C drivers wanting to
> > > talk to the devices down the link in question. Just needs to be an ldisc
> > > and then it call comes together nicely.
> > 
> > Ah, ok, that makes more sense.
> > 
> > Odd that windows does it all from userspace, but I guess it doesn't have
> > an I2C driver subsystem.
> > 
> Converting the driver to a line discipline as Alan suggested would mean to add 
> a new line discipline to linux/tty.h.
> 
> Doing this would be purely for the benefit of the community, since the driver
> works just fine for my purpose.
> 
> So before I spend any more time on it, I'll need to know ... is there any
> interest in an i2c bus driver for this device, and would a new line discipline
> be accepted into the kernel ?

If people need/want it, a new line discipline will be accepted, no
problems with that.

thanks,

greg k-h