Signed-off-by: Guenter Roeck <[email protected]>
---
This is an usb-i2c adapter I am using to connect to i2c evaluation and test
boards. Not sure if it is worth adding it into the kernel. If yes, I'll be
happy to add myself as maintainer.
drivers/i2c/busses/Kconfig | 10 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-diolan-u2c.c | 455 +++++++++++++++++++++++++++++++++++
3 files changed, 466 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 3a6321c..d73be36 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -640,6 +640,16 @@ config I2C_XILINX
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 84cb16a..46315db 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -63,6 +63,7 @@ obj-$(CONFIG_I2C_OCTEON) += i2c-octeon.o
obj-$(CONFIG_I2C_XILINX) += i2c-xiic.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..5f4fb74
--- /dev/null
+++ b/drivers/i2c/busses/i2c-diolan-u2c.c
@@ -0,0 +1,455 @@
+/*
+ * driver for the Diolan u2c-12 usb adapter
+ *
+ * Copyright (c) 2010 Ericsson AB
+ *
+ * Derived from:
+ * i2c-tiny-usb.c
+ * Copyright (C) 2006-2007 Till Harbaum ([email protected])
+ *
+ * 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 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_SET_CLOCK_SYNCH 0x24
+#define CMD_GET_CLOCK_SYNCH 0x25
+#define CMD_SET_CLOCK_SYNCH_TO 0x26
+#define CMD_GET_CLOCK_SYNCH_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 U2C_I2C_FREQ_FAST 0 /* 400 kHz */
+#define U2C_I2C_FREQ_STD 1 /* 100 kHz */
+#define U2C_I2C_FREQ_83KHZ 2
+#define U2C_I2C_FREQ_71KHZ 3
+#define U2C_I2C_FREQ_62KHZ 4
+#define U2C_I2C_FREQ_50KHZ 6
+#define U2C_I2C_FREQ_25KHZ 16
+#define U2C_I2C_FREQ_10KHZ 46
+#define U2C_I2C_FREQ_5KHZ 96
+#define U2C_I2C_FREQ_2KHZ 242
+
+#define DIOLAN_USB_TIMEOUT 100
+
+/* 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 */
+};
+
+/* usb layer */
+
+static int diolan_usb_transfer(struct i2c_adapter *adapter, u8 * obuffer,
+ int olen, u8 *ibuffer, int ilen)
+{
+ struct i2c_diolan_u2c *dev = adapter->algo_data;
+ int ret = 0;
+ int actual;
+ unsigned char inbuffer[257];
+
+ if (olen) {
+ ret = usb_bulk_msg(dev->usb_dev,
+ usb_sndbulkpipe(dev->usb_dev, DIOLAN_OUT_EP),
+ obuffer, olen, &actual, DIOLAN_USB_TIMEOUT);
+ }
+ if (!ret) {
+ ret = usb_bulk_msg(dev->usb_dev,
+ usb_rcvbulkpipe(dev->usb_dev, DIOLAN_IN_EP),
+ inbuffer, sizeof(inbuffer), &actual,
+ DIOLAN_USB_TIMEOUT);
+ if (ret == 0 && actual > 0) {
+ ret = min(actual, ilen);
+ switch (inbuffer[actual - 1]) {
+ case RESP_NACK:
+ ret = -EINVAL;
+ goto abort;
+ case RESP_OK:
+ break;
+ default:
+ ret = -EIO;
+ goto abort;
+ }
+ if (ret)
+ memcpy(ibuffer, inbuffer, ret);
+ }
+ }
+abort:
+ 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 usb_device *dev)
+{
+ int i;
+
+ for (i = 0; i < 10; i++) {
+ int actual = 0;
+ int ret;
+ u8 inbuffer[257];
+
+ ret = usb_bulk_msg(dev, usb_rcvbulkpipe(dev, DIOLAN_IN_EP),
+ inbuffer, sizeof(inbuffer), &actual,
+ DIOLAN_USB_TIMEOUT);
+ if (ret < 0 || actual == 0)
+ break;
+ }
+}
+
+static int diolan_i2c_start(struct i2c_adapter *adapter)
+{
+ u8 buffer[1];
+
+ buffer[0] = CMD_I2C_START;
+
+ return diolan_usb_transfer(adapter, buffer, 1, buffer, 1);
+}
+
+static int diolan_i2c_repeated_start(struct i2c_adapter *adapter)
+{
+ u8 buffer[1];
+
+ buffer[0] = CMD_I2C_REPEATED_START;
+
+ return diolan_usb_transfer(adapter, buffer, 1, buffer, 1);
+}
+
+static int diolan_i2c_stop(struct i2c_adapter *adapter)
+{
+ u8 buffer[1];
+
+ buffer[0] = CMD_I2C_STOP;
+
+ return diolan_usb_transfer(adapter, buffer, 1, buffer, 1);
+}
+
+static int diolan_i2c_get_byte_ack(struct i2c_adapter *adapter, bool ack,
+ u8 *byte)
+{
+ u8 buffer[2];
+ int rv;
+
+ buffer[0] = CMD_I2C_GET_BYTE_ACK;
+ buffer[1] = ack;
+
+ rv = diolan_usb_transfer(adapter, buffer, 2, buffer, 2);
+ if (rv > 0)
+ *byte = buffer[0];
+ else if (rv == 0)
+ rv = -EIO;
+
+ return rv;
+}
+
+static int diolan_i2c_put_byte_ack(struct i2c_adapter *adapter, u8 byte)
+{
+ u8 buffer[2];
+
+ buffer[0] = CMD_I2C_PUT_BYTE_ACK;
+ buffer[1] = byte;
+
+ return diolan_usb_transfer(adapter, buffer, 2, buffer, 1);
+}
+
+static int diolan_set_speed(struct i2c_adapter *adapter, u8 speed)
+{
+ u8 buffer[2];
+
+ buffer[0] = CMD_I2C_SET_SPEED;
+ buffer[1] = speed;
+
+ return diolan_usb_transfer(adapter, buffer, 2, buffer, 1);
+}
+
+static int diolan_fw_version(struct i2c_adapter *adapter)
+{
+ u8 buffer[3];
+ int ret;
+
+ buffer[0] = CMD_GET_FW_VERSION;
+ ret = diolan_usb_transfer(adapter, buffer, 1, buffer, 3);
+ if (ret == 3)
+ dev_info(&adapter->dev,
+ "Diolan U2C firmware version %d.%d\n",
+ buffer[0], buffer[1]);
+ return ret;
+}
+
+static int diolan_get_serial(struct i2c_adapter *adapter)
+{
+ u8 buffer[5];
+ int ret;
+
+ buffer[0] = CMD_GET_SERIAL;
+ ret = diolan_usb_transfer(adapter, buffer, 1, buffer, 5);
+ if (ret == 5)
+ dev_info(&adapter->dev,
+ "Diolan U2C serial number %d\n", *(u32 *) &buffer[0]);
+ return ret;
+}
+
+static int diolan_scan(struct i2c_adapter *adapter)
+{
+ u8 buffer[257];
+ int i, ret;
+
+ buffer[0] = CMD_I2C_SCAN;
+ ret = diolan_usb_transfer(adapter, buffer, 1, buffer, 257);
+ if (ret > 0) {
+ for (i = 0; i < ret - 1; i++) {
+ if (buffer[i])
+ dev_info(&adapter->dev,
+ "Found I2C device at address 0x%x\n",
+ buffer[i] >> 1);
+ }
+ }
+ return ret;
+}
+
+/* i2c layer */
+
+static int usb_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
+{
+ struct i2c_msg *pmsg;
+ int i, j;
+ int rc = 0;
+
+ rc = diolan_i2c_start(adapter);
+ if (rc < 0)
+ return rc;
+
+ for (i = 0; i < num; i++) {
+ pmsg = &msgs[i];
+ if (i) {
+ rc = diolan_i2c_repeated_start(adapter);
+ if (rc < 0)
+ goto abort;
+ }
+ if (pmsg->flags & I2C_M_RD) {
+ rc = diolan_i2c_put_byte_ack(adapter,
+ ((pmsg->addr & 0x7f) << 1) | 1);
+ if (rc < 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;
+
+ rc = diolan_i2c_get_byte_ack(adapter, ack,
+ &byte);
+ if (rc < 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) {
+ rc = -EREMOTEIO;
+ goto abort;
+ }
+ pmsg->len += byte;
+ }
+ pmsg->buf[j] = byte;
+ }
+ } else {
+ rc = diolan_i2c_put_byte_ack(adapter,
+ (pmsg->addr & 0x7f) << 1);
+ if (rc < 0)
+ goto abort;
+ for (j = 0; j < pmsg->len; j++) {
+ rc = diolan_i2c_put_byte_ack(adapter,
+ pmsg->buf[j]);
+ if (rc < 0)
+ goto abort;
+ }
+ }
+ }
+abort:
+ diolan_i2c_stop(adapter);
+ return rc;
+}
+
+/*
+ * Return list of supported functionality.
+ */
+static u32 usb_func(struct i2c_adapter *a)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
+ I2C_FUNC_SMBUS_READ_BLOCK_DATA;
+}
+
+static const struct i2c_algorithm usb_algorithm = {
+ .master_xfer = usb_xfer,
+ .functionality = usb_func,
+};
+
+/* device layer */
+
+static struct usb_device_id i2c_diolan_u2c_table[] = {
+ {USB_DEVICE(USB_VENDOR_ID_DIOLAN, USB_DEVICE_ID_DIOLAN_U2C)},
+ {}
+};
+
+MODULE_DEVICE_TABLE(usb, i2c_diolan_u2c_table);
+
+static void i2c_diolan_u2c_free(struct i2c_diolan_u2c *dev)
+{
+ usb_put_dev(dev->usb_dev);
+ kfree(dev);
+}
+
+static int i2c_diolan_u2c_probe(struct usb_interface *interface,
+ const struct usb_device_id *id)
+{
+ struct i2c_diolan_u2c *dev;
+ int retval = -ENOMEM;
+
+ /* 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");
+ 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);
+
+ dev_info(&interface->dev,
+ "Diolan U2C at bus %03d address %03d\n",
+ dev->usb_dev->bus->busnum, dev->usb_dev->devnum);
+
+ /* setup i2c adapter description */
+ dev->adapter.owner = THIS_MODULE;
+ dev->adapter.class = I2C_CLASS_HWMON;
+ dev->adapter.algo = &usb_algorithm;
+ dev->adapter.algo_data = dev;
+ snprintf(dev->adapter.name, sizeof(dev->adapter.name),
+ "i2c-u2c-usb at bus %03d device %03d",
+ dev->usb_dev->bus->busnum, dev->usb_dev->devnum);
+
+ dev->adapter.dev.parent = &dev->interface->dev;
+
+ diolan_flush_input(dev->usb_dev);
+
+ /* and finally attach to i2c layer */
+ i2c_add_adapter(&dev->adapter);
+
+ diolan_fw_version(&dev->adapter);
+
+ retval = diolan_set_speed(&dev->adapter, U2C_I2C_FREQ_STD);
+ if (retval < 0) {
+ dev_err(&dev->adapter.dev,
+ "failure %d setting I2C bus frequency\n", retval);
+ goto error_del;
+ }
+ diolan_get_serial(&dev->adapter);
+ diolan_scan(&dev->adapter);
+
+ dev_dbg(&dev->adapter.dev, "connected i2c-u2c-usb device\n");
+
+ return 0;
+
+error_del:
+ i2c_del_adapter(&dev->adapter);
+ i2c_diolan_u2c_free(dev);
+error:
+ return retval;
+}
+
+static void i2c_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);
+ i2c_diolan_u2c_free(dev);
+
+ dev_dbg(&interface->dev, "disconnected\n");
+}
+
+static struct usb_driver i2c_diolan_u2c_driver = {
+ .name = "i2c-u2c-usb",
+ .probe = i2c_diolan_u2c_probe,
+ .disconnect = i2c_diolan_u2c_disconnect,
+ .id_table = i2c_diolan_u2c_table,
+};
+
+static int __init usb_i2c_diolan_u2c_init(void)
+{
+ /* register this driver with the USB subsystem */
+ return usb_register(&i2c_diolan_u2c_driver);
+}
+
+static void __exit usb_i2c_diolan_u2c_exit(void)
+{
+ /* deregister this driver with the USB subsystem */
+ usb_deregister(&i2c_diolan_u2c_driver);
+}
+
+module_init(usb_i2c_diolan_u2c_init);
+module_exit(usb_i2c_diolan_u2c_exit);
+
+/* ----- end of usb layer ------------------------------------------------ */
+
+MODULE_AUTHOR("Guenter Roeck <[email protected]>");
+MODULE_DESCRIPTION("i2c-u2c-usb driver");
+MODULE_LICENSE("GPL");
--
1.7.3.1
On Wed, 3 Nov 2010 17:26:29 -0700, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> This is an usb-i2c adapter I am using to connect to i2c evaluation and test
> boards. Not sure if it is worth adding it into the kernel. If yes, I'll be
> happy to add myself as maintainer.
Why not? This is a device other developers may want to use, and your
driver is relatively small, so I'm totally fine having it in the
upstream kernel.
> drivers/i2c/busses/Kconfig | 10 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-diolan-u2c.c | 455 +++++++++++++++++++++++++++++++++++
> 3 files changed, 466 insertions(+), 0 deletions(-)
> create mode 100644 drivers/i2c/busses/i2c-diolan-u2c.c
Review:
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 3a6321c..d73be36 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -640,6 +640,16 @@ config I2C_XILINX
>
> 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 84cb16a..46315db 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -63,6 +63,7 @@ obj-$(CONFIG_I2C_OCTEON) += i2c-octeon.o
> obj-$(CONFIG_I2C_XILINX) += i2c-xiic.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..5f4fb74
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-diolan-u2c.c
> @@ -0,0 +1,455 @@
> +/*
> + * driver for the Diolan u2c-12 usb adapter
> + *
> + * Copyright (c) 2010 Ericsson AB
> + *
> + * Derived from:
> + * i2c-tiny-usb.c
> + * Copyright (C) 2006-2007 Till Harbaum ([email protected])
> + *
> + * 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 USB_VENDOR_ID_DIOLAN 0x0abf
> +#define USB_DEVICE_ID_DIOLAN_U2C 0x3370
Maybe you can submit these to http://www.linux-usb.org/usb-ids.html so
that lsusb identifies the device?
> +
> +#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_SET_CLOCK_SYNCH 0x24
> +#define CMD_GET_CLOCK_SYNCH 0x25
> +#define CMD_SET_CLOCK_SYNCH_TO 0x26
> +#define CMD_GET_CLOCK_SYNCH_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 U2C_I2C_FREQ_FAST 0 /* 400 kHz */
> +#define U2C_I2C_FREQ_STD 1 /* 100 kHz */
Doubled spaces at end of comments.
> +#define U2C_I2C_FREQ_83KHZ 2
> +#define U2C_I2C_FREQ_71KHZ 3
> +#define U2C_I2C_FREQ_62KHZ 4
> +#define U2C_I2C_FREQ_50KHZ 6
> +#define U2C_I2C_FREQ_25KHZ 16
> +#define U2C_I2C_FREQ_10KHZ 46
> +#define U2C_I2C_FREQ_5KHZ 96
> +#define U2C_I2C_FREQ_2KHZ 242
> +
> +#define DIOLAN_USB_TIMEOUT 100
Unit?
> +
> +/* 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 */
> +};
> +
> +/* usb layer */
> +
Please document what the function below returns.
> +static int diolan_usb_transfer(struct i2c_adapter *adapter, u8 * obuffer,
No space between * and obuffer.
obuffer could be a const pointer, couldn't it?
> + int olen, u8 *ibuffer, int ilen)
> +{
> + struct i2c_diolan_u2c *dev = adapter->algo_data;
> + int ret = 0;
> + int actual;
> + unsigned char inbuffer[257];
I know it doesn't matter in practice, but it's a little inconsistent to
use unsigned char for this buffer and u8 in all other functions.
I'm also unsure what is the point of having such a large buffer when
the largest block you ever transfer in practice is 5 bytes?
> +
> + if (olen) {
> + ret = usb_bulk_msg(dev->usb_dev,
> + usb_sndbulkpipe(dev->usb_dev, DIOLAN_OUT_EP),
> + obuffer, olen, &actual, DIOLAN_USB_TIMEOUT);
> + }
> + if (!ret) {
> + ret = usb_bulk_msg(dev->usb_dev,
> + usb_rcvbulkpipe(dev->usb_dev, DIOLAN_IN_EP),
> + inbuffer, sizeof(inbuffer), &actual,
> + DIOLAN_USB_TIMEOUT);
> + if (ret == 0 && actual > 0) {
> + ret = min(actual, ilen);
This could be done after checking for errors.
> + switch (inbuffer[actual - 1]) {
> + case RESP_NACK:
> + ret = -EINVAL;
> + goto abort;
According to Documentation/i2c/fault-codes, nacks should be translated
to -ENXIO.
> + case RESP_OK:
> + break;
> + default:
> + ret = -EIO;
> + goto abort;
> + }
I don't see the value of gotos here, breaks would work just fine, all
you have to do is change your test below to "ret > 0" - or even better,
move the memcpy inside the switch.
> + if (ret)
> + memcpy(ibuffer, inbuffer, ret);
BTW, I'm not sure why you don't use the original buffer directly?
memcpy is bad performance-wise.
> + }
> + }
> +abort:
> + 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 usb_device *dev)
> +{
> + int i;
> +
> + for (i = 0; i < 10; i++) {
> + int actual = 0;
> + int ret;
> + u8 inbuffer[257];
> +
> + ret = usb_bulk_msg(dev, usb_rcvbulkpipe(dev, DIOLAN_IN_EP),
> + inbuffer, sizeof(inbuffer), &actual,
> + DIOLAN_USB_TIMEOUT);
> + if (ret < 0 || actual == 0)
> + break;
> + }
Shouldn't you emit a warning of some sort and/or fail driver loading if
all retries were exhausted?
> +}
> +
> +static int diolan_i2c_start(struct i2c_adapter *adapter)
> +{
> + u8 buffer[1];
> +
> + buffer[0] = CMD_I2C_START;
> +
> + return diolan_usb_transfer(adapter, buffer, 1, buffer, 1);
> +}
> +
> +static int diolan_i2c_repeated_start(struct i2c_adapter *adapter)
> +{
> + u8 buffer[1];
> +
> + buffer[0] = CMD_I2C_REPEATED_START;
> +
> + return diolan_usb_transfer(adapter, buffer, 1, buffer, 1);
> +}
> +
> +static int diolan_i2c_stop(struct i2c_adapter *adapter)
> +{
> + u8 buffer[1];
> +
> + buffer[0] = CMD_I2C_STOP;
> +
> + return diolan_usb_transfer(adapter, buffer, 1, buffer, 1);
> +}
> +
> +static int diolan_i2c_get_byte_ack(struct i2c_adapter *adapter, bool ack,
> + u8 *byte)
> +{
> + u8 buffer[2];
> + int rv;
Why "rv" when all other functions use "ret"?
> +
> + buffer[0] = CMD_I2C_GET_BYTE_ACK;
> + buffer[1] = ack;
> +
> + rv = diolan_usb_transfer(adapter, buffer, 2, buffer, 2);
> + if (rv > 0)
> + *byte = buffer[0];
> + else if (rv == 0)
> + rv = -EIO;
> +
> + return rv;
> +}
> +
> +static int diolan_i2c_put_byte_ack(struct i2c_adapter *adapter, u8 byte)
> +{
> + u8 buffer[2];
> +
> + buffer[0] = CMD_I2C_PUT_BYTE_ACK;
> + buffer[1] = byte;
> +
> + return diolan_usb_transfer(adapter, buffer, 2, buffer, 1);
> +}
> +
> +static int diolan_set_speed(struct i2c_adapter *adapter, u8 speed)
> +{
> + u8 buffer[2];
> +
> + buffer[0] = CMD_I2C_SET_SPEED;
> + buffer[1] = speed;
> +
> + return diolan_usb_transfer(adapter, buffer, 2, buffer, 1);
> +}
> +
> +static int diolan_fw_version(struct i2c_adapter *adapter)
> +{
> + u8 buffer[3];
> + int ret;
> +
> + buffer[0] = CMD_GET_FW_VERSION;
> + ret = diolan_usb_transfer(adapter, buffer, 1, buffer, 3);
> + if (ret == 3)
> + dev_info(&adapter->dev,
> + "Diolan U2C firmware version %d.%d\n",
> + buffer[0], buffer[1]);
Unless you expect negative versions, %u would be more appropriate. Also
note that to be completely correct you should cast the values to
unsigned int before printing them.
> + return ret;
> +}
> +
> +static int diolan_get_serial(struct i2c_adapter *adapter)
> +{
> + u8 buffer[5];
> + int ret;
> +
> + buffer[0] = CMD_GET_SERIAL;
> + ret = diolan_usb_transfer(adapter, buffer, 1, buffer, 5);
> + if (ret == 5)
> + dev_info(&adapter->dev,
> + "Diolan U2C serial number %d\n", *(u32 *) &buffer[0]);
Will the value be displayed correctly on big-endian machines? Doesn't
seem so. You probably have to use le32_to_cpu().
Also, %d to print an unsigned number isn't good.
> + return ret;
> +}
> +
> +static int diolan_scan(struct i2c_adapter *adapter)
> +{
> + u8 buffer[257];
> + int i, ret;
> +
> + buffer[0] = CMD_I2C_SCAN;
> + ret = diolan_usb_transfer(adapter, buffer, 1, buffer, 257);
> + if (ret > 0) {
> + for (i = 0; i < ret - 1; i++) {
> + if (buffer[i])
> + dev_info(&adapter->dev,
> + "Found I2C device at address 0x%x\n",
> + buffer[i] >> 1);
> + }
> + }
> + return ret;
> +}
I don't know how exactly the device is scanning for I2C slaves, but
there is no provision for device discovery in the I2C specification. I
wouldn't do that unconditionally at driver bind time, it might confuse
some I2C slaves.
If the user wants to probe for devices, we have i2c-dev + i2cdetect for
this, which is more flexible.
> +
> +/* i2c layer */
> +
> +static int usb_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
> +{
> + struct i2c_msg *pmsg;
> + int i, j;
> + int rc = 0;
And now rc instead of ret as everywhere else? You are being creative ;)
> +
> + rc = diolan_i2c_start(adapter);
> + if (rc < 0)
> + return rc;
> +
> + for (i = 0; i < num; i++) {
> + pmsg = &msgs[i];
> + if (i) {
> + rc = diolan_i2c_repeated_start(adapter);
> + if (rc < 0)
> + goto abort;
> + }
> + if (pmsg->flags & I2C_M_RD) {
> + rc = diolan_i2c_put_byte_ack(adapter,
> + ((pmsg->addr & 0x7f) << 1) | 1);
Note that the mask is useless: the address is already a 7-bit value.
> + if (rc < 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;
> +
> + rc = diolan_i2c_get_byte_ack(adapter, ack,
> + &byte);
> + if (rc < 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) {
> + rc = -EREMOTEIO;
Should be -EPROTO according to Documentation/i2c/fault-codes.
> + goto abort;
> + }
> + pmsg->len += byte;
> + }
> + pmsg->buf[j] = byte;
> + }
> + } else {
> + rc = diolan_i2c_put_byte_ack(adapter,
> + (pmsg->addr & 0x7f) << 1);
Useless mask.
> + if (rc < 0)
> + goto abort;
> + for (j = 0; j < pmsg->len; j++) {
> + rc = diolan_i2c_put_byte_ack(adapter,
> + pmsg->buf[j]);
> + if (rc < 0)
> + goto abort;
> + }
> + }
> + }
> +abort:
> + diolan_i2c_stop(adapter);
> + return rc;
> +}
> +
> +/*
> + * Return list of supported functionality.
> + */
> +static u32 usb_func(struct i2c_adapter *a)
> +{
> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
> + I2C_FUNC_SMBUS_READ_BLOCK_DATA;
Odd indentation/alignment.
As far as I can see you also support I2C_FUNC_SMBUS_BLOCK_PROC_CALL
(even though it is not used by any driver I know of.)
> +}
> +
> +static const struct i2c_algorithm usb_algorithm = {
> + .master_xfer = usb_xfer,
> + .functionality = usb_func,
> +};
> +
> +/* device layer */
> +
> +static struct usb_device_id i2c_diolan_u2c_table[] = {
Could this be made const?
> + {USB_DEVICE(USB_VENDOR_ID_DIOLAN, USB_DEVICE_ID_DIOLAN_U2C)},
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(usb, i2c_diolan_u2c_table);
> +
> +static void i2c_diolan_u2c_free(struct i2c_diolan_u2c *dev)
> +{
> + usb_put_dev(dev->usb_dev);
> + kfree(dev);
> +}
> +
> +static int i2c_diolan_u2c_probe(struct usb_interface *interface,
> + const struct usb_device_id *id)
> +{
> + struct i2c_diolan_u2c *dev;
> + int retval = -ENOMEM;
This initialization could be delayed to the point where you actually
need it.
> +
> + /* 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");
> + 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);
> +
> + dev_info(&interface->dev,
> + "Diolan U2C at bus %03d address %03d\n",
> + dev->usb_dev->bus->busnum, dev->usb_dev->devnum);
> +
> + /* setup i2c adapter description */
> + dev->adapter.owner = THIS_MODULE;
> + dev->adapter.class = I2C_CLASS_HWMON;
> + dev->adapter.algo = &usb_algorithm;
> + dev->adapter.algo_data = dev;
You are abusing algo_data here. You are supposed to use
i2c_get/set_adapdata() instead. algo_data is only there for providing
platform specific implementation details to generic i2c algorithms such
as i2c-algo-bit.
> + snprintf(dev->adapter.name, sizeof(dev->adapter.name),
> + "i2c-u2c-usb at bus %03d device %03d",
> + dev->usb_dev->bus->busnum, dev->usb_dev->devnum);
> +
> + dev->adapter.dev.parent = &dev->interface->dev;
> +
> + diolan_flush_input(dev->usb_dev);
> +
> + /* and finally attach to i2c layer */
> + i2c_add_adapter(&dev->adapter);
Please check for error here. It could happen!
> +
> + diolan_fw_version(&dev->adapter);
This seems racy, and the commands below as well. Serialization of calls
to usb_xfer is guaranteed by i2c-core, but here you are calling other
functions which will access your USB interface. I'm no USB expert, but
diolan_usb_transfer() doesn't seem to be designed for parallel
execution. As your i2c adapter is already registered, usb_xfer() could
run in parallel with diolan_fw_version(), diolan_set_speed() etc.
So either you add a mutex to serialize the access yourself (which will
cause a run-time performance hit) or you do all your stuff _before_ the
adapter is publicly usable.
> +
> + retval = diolan_set_speed(&dev->adapter, U2C_I2C_FREQ_STD);
> + if (retval < 0) {
> + dev_err(&dev->adapter.dev,
> + "failure %d setting I2C bus frequency\n", retval);
> + goto error_del;
> + }
Beyond the race issue, you want to fully initialize the adapter before
you make it visible to consumers, so speed should be set before calling
i2c_add_adapter().
> + diolan_get_serial(&dev->adapter);
> + diolan_scan(&dev->adapter);
> +
> + dev_dbg(&dev->adapter.dev, "connected i2c-u2c-usb device\n");
> +
> + return 0;
> +
> +error_del:
> + i2c_del_adapter(&dev->adapter);
> + i2c_diolan_u2c_free(dev);
> +error:
> + return retval;
> +}
> +
> +static void i2c_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);
If you have to do this here, then you also have to do it in the failure
path of i2c_diolan_u2c_probe(), don't you?
> + i2c_diolan_u2c_free(dev);
> +
> + dev_dbg(&interface->dev, "disconnected\n");
> +}
> +
> +static struct usb_driver i2c_diolan_u2c_driver = {
> + .name = "i2c-u2c-usb",
Why not "i2c-diolan-u2c" as the module name? Would be more consistent.
> + .probe = i2c_diolan_u2c_probe,
> + .disconnect = i2c_diolan_u2c_disconnect,
> + .id_table = i2c_diolan_u2c_table,
> +};
> +
> +static int __init usb_i2c_diolan_u2c_init(void)
> +{
> + /* register this driver with the USB subsystem */
> + return usb_register(&i2c_diolan_u2c_driver);
> +}
> +
> +static void __exit usb_i2c_diolan_u2c_exit(void)
> +{
> + /* deregister this driver with the USB subsystem */
> + usb_deregister(&i2c_diolan_u2c_driver);
> +}
> +
> +module_init(usb_i2c_diolan_u2c_init);
> +module_exit(usb_i2c_diolan_u2c_exit);
> +
> +/* ----- end of usb layer ------------------------------------------------ */
This comment is inconsistent (and useless, if you ask me.)
> +
> +MODULE_AUTHOR("Guenter Roeck <[email protected]>");
> +MODULE_DESCRIPTION("i2c-u2c-usb driver");
> +MODULE_LICENSE("GPL");
--
Jean Delvare
On Wed, Nov 03, 2010 at 05:26:29PM -0700, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> This is an usb-i2c adapter I am using to connect to i2c evaluation and test
> boards. Not sure if it is worth adding it into the kernel. If yes, I'll be
> happy to add myself as maintainer.
>
> drivers/i2c/busses/Kconfig | 10 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-diolan-u2c.c | 455 +++++++++++++++++++++++++++++++++++
> 3 files changed, 466 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 3a6321c..d73be36 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -640,6 +640,16 @@ config I2C_XILINX
>
> 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 84cb16a..46315db 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -63,6 +63,7 @@ obj-$(CONFIG_I2C_OCTEON) += i2c-octeon.o
> obj-$(CONFIG_I2C_XILINX) += i2c-xiic.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..5f4fb74
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-diolan-u2c.c
> @@ -0,0 +1,455 @@
> +/*
> + * driver for the Diolan u2c-12 usb adapter
> + *
> + * Copyright (c) 2010 Ericsson AB
> + *
> + * Derived from:
> + * i2c-tiny-usb.c
> + * Copyright (C) 2006-2007 Till Harbaum ([email protected])
> + *
> + * 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 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_SET_CLOCK_SYNCH 0x24
> +#define CMD_GET_CLOCK_SYNCH 0x25
> +#define CMD_SET_CLOCK_SYNCH_TO 0x26
> +#define CMD_GET_CLOCK_SYNCH_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 U2C_I2C_FREQ_FAST 0 /* 400 kHz */
> +#define U2C_I2C_FREQ_STD 1 /* 100 kHz */
> +#define U2C_I2C_FREQ_83KHZ 2
> +#define U2C_I2C_FREQ_71KHZ 3
> +#define U2C_I2C_FREQ_62KHZ 4
> +#define U2C_I2C_FREQ_50KHZ 6
> +#define U2C_I2C_FREQ_25KHZ 16
> +#define U2C_I2C_FREQ_10KHZ 46
> +#define U2C_I2C_FREQ_5KHZ 96
> +#define U2C_I2C_FREQ_2KHZ 242
> +
> +#define DIOLAN_USB_TIMEOUT 100
> +
> +/* 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 */
> +};
> +
> +/* usb layer */
> +
> +static int diolan_usb_transfer(struct i2c_adapter *adapter, u8 * obuffer,
> + int olen, u8 *ibuffer, int ilen)
> +{
> + struct i2c_diolan_u2c *dev = adapter->algo_data;
> + int ret = 0;
> + int actual;
> + unsigned char inbuffer[257];
> +
> + if (olen) {
> + ret = usb_bulk_msg(dev->usb_dev,
> + usb_sndbulkpipe(dev->usb_dev, DIOLAN_OUT_EP),
> + obuffer, olen, &actual, DIOLAN_USB_TIMEOUT);
> + }
> + if (!ret) {
> + ret = usb_bulk_msg(dev->usb_dev,
> + usb_rcvbulkpipe(dev->usb_dev, DIOLAN_IN_EP),
> + inbuffer, sizeof(inbuffer), &actual,
> + DIOLAN_USB_TIMEOUT);
> + if (ret == 0 && actual > 0) {
> + ret = min(actual, ilen);
> + switch (inbuffer[actual - 1]) {
> + case RESP_NACK:
> + ret = -EINVAL;
> + goto abort;
> + case RESP_OK:
> + break;
> + default:
> + ret = -EIO;
> + goto abort;
> + }
> + if (ret)
> + memcpy(ibuffer, inbuffer, ret);
> + }
> + }
> +abort:
> + 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 usb_device *dev)
> +{
> + int i;
> +
> + for (i = 0; i < 10; i++) {
> + int actual = 0;
> + int ret;
> + u8 inbuffer[257];
> +
> + ret = usb_bulk_msg(dev, usb_rcvbulkpipe(dev, DIOLAN_IN_EP),
> + inbuffer, sizeof(inbuffer), &actual,
> + DIOLAN_USB_TIMEOUT);
> + if (ret < 0 || actual == 0)
> + break;
> + }
> +}
> +
> +static int diolan_i2c_start(struct i2c_adapter *adapter)
> +{
> + u8 buffer[1];
> +
> + buffer[0] = CMD_I2C_START;
> +
> + return diolan_usb_transfer(adapter, buffer, 1, buffer, 1);
> +}
> +
> +static int diolan_i2c_repeated_start(struct i2c_adapter *adapter)
> +{
> + u8 buffer[1];
note, transfers to/from stack memory are not advised, esp. if this
involves the hardware dma-mapping the stack. Please move the buffer
to your private data.
> + buffer[0] = CMD_I2C_REPEATED_START;
> +
> + return diolan_usb_transfer(adapter, buffer, 1, buffer, 1);
> +}
why not doing something like
diolan_usb_cmd1(struct i2c_adapter *adapter, u8 command)
{
struct i2c_diolan_u2c *dev = adapter->algo_data;
dev->buffer[0] = command;
return diolan_usb_treansfer(adapter, dev->buffer, 1, dev->buffer, 1);
}
then a lot of these little transfer functions could be turned into much
smaller ones.
ie:
static int diolan_i2c_stop(struct i2c_adapter *adapter)
{
return diolan_usb_cmd1(adapter, CMD_I2C_STOP);
}
> +static int diolan_i2c_stop(struct i2c_adapter *adapter)
> +{
> + u8 buffer[1];
> +
> + buffer[0] = CMD_I2C_STOP;
> +
> + return diolan_usb_transfer(adapter, buffer, 1, buffer, 1);
> +}
> +
> +static int diolan_i2c_get_byte_ack(struct i2c_adapter *adapter, bool ack,
> + u8 *byte)
> +{
> + u8 buffer[2];
> + int rv;
> +
> + buffer[0] = CMD_I2C_GET_BYTE_ACK;
> + buffer[1] = ack;
> +
> + rv = diolan_usb_transfer(adapter, buffer, 2, buffer, 2);
> + if (rv > 0)
> + *byte = buffer[0];
> + else if (rv == 0)
> + rv = -EIO;
> +
> + return rv;
> +}
see notes about stack-mapping buffer.
> +static int diolan_i2c_put_byte_ack(struct i2c_adapter *adapter, u8 byte)
> +{
> + u8 buffer[2];
> +
> + buffer[0] = CMD_I2C_PUT_BYTE_ACK;
> + buffer[1] = byte;
> +
> + return diolan_usb_transfer(adapter, buffer, 2, buffer, 1);
> +}
> +
> +static int diolan_set_speed(struct i2c_adapter *adapter, u8 speed)
> +{
> + u8 buffer[2];
> +
> + buffer[0] = CMD_I2C_SET_SPEED;
> + buffer[1] = speed;
> +
> + return diolan_usb_transfer(adapter, buffer, 2, buffer, 1);
> +}
maybe roll-up the 2 byte transfers in the same was a one-byte.
> +static int diolan_fw_version(struct i2c_adapter *adapter)
> +{
> + u8 buffer[3];
> + int ret;
> +
> + buffer[0] = CMD_GET_FW_VERSION;
> + ret = diolan_usb_transfer(adapter, buffer, 1, buffer, 3);
> + if (ret == 3)
> + dev_info(&adapter->dev,
> + "Diolan U2C firmware version %d.%d\n",
> + buffer[0], buffer[1]);
> + return ret;
> +}
> +
> +static int diolan_get_serial(struct i2c_adapter *adapter)
> +{
> + u8 buffer[5];
> + int ret;
> +
> + buffer[0] = CMD_GET_SERIAL;
> + ret = diolan_usb_transfer(adapter, buffer, 1, buffer, 5);
> + if (ret == 5)
> + dev_info(&adapter->dev,
> + "Diolan U2C serial number %d\n", *(u32 *) &buffer[0]);
*(u32 *) isn't nice, esp. as you're not going to know the endian-ness of
the host you are running on. suggest trying a explict le32 ot be32 get of
the data.
> + return ret;
> +}
> +static int diolan_scan(struct i2c_adapter *adapter)
> +{
> + u8 buffer[257];
> + int i, ret;
suggest kmalloc() the buffer.
> + buffer[0] = CMD_I2C_SCAN;
> + ret = diolan_usb_transfer(adapter, buffer, 1, buffer, 257);
> + if (ret > 0) {
> + for (i = 0; i < ret - 1; i++) {
> + if (buffer[i])
> + dev_info(&adapter->dev,
> + "Found I2C device at address 0x%x\n",
> + buffer[i] >> 1);
> + }
> + }
> + return ret;
do you really need to do this, we've already got a userspace i2cdetect
tool?
> +}
> +
> +/* i2c layer */
> +
> +static int usb_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
> +{
> + struct i2c_msg *pmsg;
> + int i, j;
> + int rc = 0;
> +
> + rc = diolan_i2c_start(adapter);
> + if (rc < 0)
> + return rc;
> +
> + for (i = 0; i < num; i++) {
> + pmsg = &msgs[i];
> + if (i) {
> + rc = diolan_i2c_repeated_start(adapter);
> + if (rc < 0)
> + goto abort;
> + }
> + if (pmsg->flags & I2C_M_RD) {
> + rc = diolan_i2c_put_byte_ack(adapter,
> + ((pmsg->addr & 0x7f) << 1) | 1);
> + if (rc < 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;
> +
> + rc = diolan_i2c_get_byte_ack(adapter, ack,
> + &byte);
> + if (rc < 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) {
> + rc = -EREMOTEIO;
> + goto abort;
> + }
> + pmsg->len += byte;
> + }
> + pmsg->buf[j] = byte;
> + }
> + } else {
> + rc = diolan_i2c_put_byte_ack(adapter,
> + (pmsg->addr & 0x7f) << 1);
> + if (rc < 0)
> + goto abort;
> + for (j = 0; j < pmsg->len; j++) {
> + rc = diolan_i2c_put_byte_ack(adapter,
> + pmsg->buf[j]);
> + if (rc < 0)
> + goto abort;
> + }
> + }
> + }
> +abort:
> + diolan_i2c_stop(adapter);
> + return rc;
> +}
> +
> +/*
> + * Return list of supported functionality.
> + */
> +static u32 usb_func(struct i2c_adapter *a)
> +{
> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
> + I2C_FUNC_SMBUS_READ_BLOCK_DATA;
> +}
please make sure everything is prefixed with dio_ for nicer debugging.
> +static const struct i2c_algorithm usb_algorithm = {
> + .master_xfer = usb_xfer,
> + .functionality = usb_func,
> +};
> +
> +/* device layer */
> +
> +static struct usb_device_id i2c_diolan_u2c_table[] = {
> + {USB_DEVICE(USB_VENDOR_ID_DIOLAN, USB_DEVICE_ID_DIOLAN_U2C)},
spaces between { and text please, same with }
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(usb, i2c_diolan_u2c_table);
> +
> +static void i2c_diolan_u2c_free(struct i2c_diolan_u2c *dev)
> +{
> + usb_put_dev(dev->usb_dev);
> + kfree(dev);
> +}
> +
> +static int i2c_diolan_u2c_probe(struct usb_interface *interface,
> + const struct usb_device_id *id)
> +{
> + struct i2c_diolan_u2c *dev;
> + int retval = -ENOMEM;
> +
> + /* 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");
> + 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);
> +
> + dev_info(&interface->dev,
> + "Diolan U2C at bus %03d address %03d\n",
> + dev->usb_dev->bus->busnum, dev->usb_dev->devnum);
> +
> + /* setup i2c adapter description */
> + dev->adapter.owner = THIS_MODULE;
> + dev->adapter.class = I2C_CLASS_HWMON;
> + dev->adapter.algo = &usb_algorithm;
> + dev->adapter.algo_data = dev;
> + snprintf(dev->adapter.name, sizeof(dev->adapter.name),
> + "i2c-u2c-usb at bus %03d device %03d",
> + dev->usb_dev->bus->busnum, dev->usb_dev->devnum);
> +
> + dev->adapter.dev.parent = &dev->interface->dev;
> +
> + diolan_flush_input(dev->usb_dev);
> +
> + /* and finally attach to i2c layer */
> + i2c_add_adapter(&dev->adapter);
> +
> + diolan_fw_version(&dev->adapter);
> +
> + retval = diolan_set_speed(&dev->adapter, U2C_I2C_FREQ_STD);
> + if (retval < 0) {
> + dev_err(&dev->adapter.dev,
> + "failure %d setting I2C bus frequency\n", retval);
> + goto error_del;
> + }
> + diolan_get_serial(&dev->adapter);
> + diolan_scan(&dev->adapter);
do you really need to scan the bus on connection?
> + dev_dbg(&dev->adapter.dev, "connected i2c-u2c-usb device\n");
> +
> + return 0;
> +
> +error_del:
> + i2c_del_adapter(&dev->adapter);
> + i2c_diolan_u2c_free(dev);
> +error:
> + return retval;
> +}
> +
> +static void i2c_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);
> + i2c_diolan_u2c_free(dev);
> +
> + dev_dbg(&interface->dev, "disconnected\n");
> +}
> +
> +static struct usb_driver i2c_diolan_u2c_driver = {
> + .name = "i2c-u2c-usb",
> + .probe = i2c_diolan_u2c_probe,
> + .disconnect = i2c_diolan_u2c_disconnect,
> + .id_table = i2c_diolan_u2c_table,
> +};
> +
> +static int __init usb_i2c_diolan_u2c_init(void)
> +{
> + /* register this driver with the USB subsystem */
> + return usb_register(&i2c_diolan_u2c_driver);
> +}
> +
> +static void __exit usb_i2c_diolan_u2c_exit(void)
> +{
> + /* deregister this driver with the USB subsystem */
> + usb_deregister(&i2c_diolan_u2c_driver);
> +}
> +
> +module_init(usb_i2c_diolan_u2c_init);
> +module_exit(usb_i2c_diolan_u2c_exit);
> +
> +/* ----- end of usb layer ------------------------------------------------ */
> +
> +MODULE_AUTHOR("Guenter Roeck <[email protected]>");
> +MODULE_DESCRIPTION("i2c-u2c-usb driver");
> +MODULE_LICENSE("GPL");
--
Ben
Q: What's a light-year?
A: One-third less calories than a regular year.
Hi Jean,
On Thu, 2010-11-04 at 08:43 -0400, Jean Delvare wrote:
> On Wed, 3 Nov 2010 17:26:29 -0700, Guenter Roeck wrote:
> > Signed-off-by: Guenter Roeck <[email protected]>
> > ---
> > This is an usb-i2c adapter I am using to connect to i2c evaluation and test
> > boards. Not sure if it is worth adding it into the kernel. If yes, I'll be
> > happy to add myself as maintainer.
>
> Why not? This is a device other developers may want to use, and your
> driver is relatively small, so I'm totally fine having it in the
> upstream kernel.
>
Ok.
> > drivers/i2c/busses/Kconfig | 10 +
> > drivers/i2c/busses/Makefile | 1 +
> > drivers/i2c/busses/i2c-diolan-u2c.c | 455 +++++++++++++++++++++++++++++++++++
> > 3 files changed, 466 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/i2c/busses/i2c-diolan-u2c.c
>
> Review:
> >
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index 3a6321c..d73be36 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -640,6 +640,16 @@ config I2C_XILINX
> >
> > 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 84cb16a..46315db 100644
> > --- a/drivers/i2c/busses/Makefile
> > +++ b/drivers/i2c/busses/Makefile
> > @@ -63,6 +63,7 @@ obj-$(CONFIG_I2C_OCTEON) += i2c-octeon.o
> > obj-$(CONFIG_I2C_XILINX) += i2c-xiic.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..5f4fb74
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-diolan-u2c.c
> > @@ -0,0 +1,455 @@
> > +/*
> > + * driver for the Diolan u2c-12 usb adapter
> > + *
> > + * Copyright (c) 2010 Ericsson AB
> > + *
> > + * Derived from:
> > + * i2c-tiny-usb.c
> > + * Copyright (C) 2006-2007 Till Harbaum ([email protected])
> > + *
> > + * 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 USB_VENDOR_ID_DIOLAN 0x0abf
> > +#define USB_DEVICE_ID_DIOLAN_U2C 0x3370
>
> Maybe you can submit these to http://www.linux-usb.org/usb-ids.html so
> that lsusb identifies the device?
>
Sure. Will do.
> > +
> > +#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_SET_CLOCK_SYNCH 0x24
> > +#define CMD_GET_CLOCK_SYNCH 0x25
> > +#define CMD_SET_CLOCK_SYNCH_TO 0x26
> > +#define CMD_GET_CLOCK_SYNCH_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 U2C_I2C_FREQ_FAST 0 /* 400 kHz */
> > +#define U2C_I2C_FREQ_STD 1 /* 100 kHz */
>
> Doubled spaces at end of comments.
>
Fixed.
> > +#define U2C_I2C_FREQ_83KHZ 2
> > +#define U2C_I2C_FREQ_71KHZ 3
> > +#define U2C_I2C_FREQ_62KHZ 4
> > +#define U2C_I2C_FREQ_50KHZ 6
> > +#define U2C_I2C_FREQ_25KHZ 16
> > +#define U2C_I2C_FREQ_10KHZ 46
> > +#define U2C_I2C_FREQ_5KHZ 96
> > +#define U2C_I2C_FREQ_2KHZ 242
> > +
> > +#define DIOLAN_USB_TIMEOUT 100
>
> Unit?
>
ms, added.
> > +
> > +/* 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 */
> > +};
> > +
> > +/* usb layer */
> > +
>
> Please document what the function below returns.
>
Done
> > +static int diolan_usb_transfer(struct i2c_adapter *adapter, u8 * obuffer,
>
> No space between * and obuffer.
>
ok. Someone should teach Lindent to stop doing that ;).
> obuffer could be a const pointer, couldn't it?
>
In theory yes, but then the compiler complains when calling
usb_bulk_msg().
> > + int olen, u8 *ibuffer, int ilen)
> > +{
> > + struct i2c_diolan_u2c *dev = adapter->algo_data;
> > + int ret = 0;
> > + int actual;
> > + unsigned char inbuffer[257];
>
> I know it doesn't matter in practice, but it's a little inconsistent to
> use unsigned char for this buffer and u8 in all other functions.
>
Changed to u8
> I'm also unsure what is the point of having such a large buffer when
> the largest block you ever transfer in practice is 5 bytes?
>
I took that from the Diolan code. They always use a 257 byte temp
buffer, since that is the maximum data size sent by the adapter.
You are right, I should not really need that since I don't send any long
commands. Ultimate reason is to account for possible adapter errors, if
it replies (or tries to reply) with more bytes than expected. Pretty
much just playing safe.
I'll have to change that to account for Ben's comments about moving DMA
data from/to the stack.
> > +
> > + if (olen) {
> > + ret = usb_bulk_msg(dev->usb_dev,
> > + usb_sndbulkpipe(dev->usb_dev, DIOLAN_OUT_EP),
> > + obuffer, olen, &actual, DIOLAN_USB_TIMEOUT);
> > + }
> > + if (!ret) {
> > + ret = usb_bulk_msg(dev->usb_dev,
> > + usb_rcvbulkpipe(dev->usb_dev, DIOLAN_IN_EP),
> > + inbuffer, sizeof(inbuffer), &actual,
> > + DIOLAN_USB_TIMEOUT);
> > + if (ret == 0 && actual > 0) {
> > + ret = min(actual, ilen);
>
> This could be done after checking for errors.
>
Moved.
> > + switch (inbuffer[actual - 1]) {
> > + case RESP_NACK:
> > + ret = -EINVAL;
> > + goto abort;
>
> According to Documentation/i2c/fault-codes, nacks should be translated
> to -ENXIO.
Fixed.
>
> > + case RESP_OK:
> > + break;
> > + default:
> > + ret = -EIO;
> > + goto abort;
> > + }
>
> I don't see the value of gotos here, breaks would work just fine, all
> you have to do is change your test below to "ret > 0" - or even better,
> move the memcpy inside the switch.
>
Moved the memcpy. Good idea.
> > + if (ret)
> > + memcpy(ibuffer, inbuffer, ret);
>
> BTW, I'm not sure why you don't use the original buffer directly?
> memcpy is bad performance-wise.
>
To account for the possibility that the adapter returns more bytes than
I am expecting. Sure, that would be a bug, but I wanted to play safe.
> > + }
> > + }
> > +abort:
> > + 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 usb_device *dev)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < 10; i++) {
> > + int actual = 0;
> > + int ret;
> > + u8 inbuffer[257];
> > +
> > + ret = usb_bulk_msg(dev, usb_rcvbulkpipe(dev, DIOLAN_IN_EP),
> > + inbuffer, sizeof(inbuffer), &actual,
> > + DIOLAN_USB_TIMEOUT);
> > + if (ret < 0 || actual == 0)
> > + break;
> > + }
>
> Shouldn't you emit a warning of some sort and/or fail driver loading if
> all retries were exhausted?
>
Makes sense. Added.
> > +}
> > +
> > +static int diolan_i2c_start(struct i2c_adapter *adapter)
> > +{
> > + u8 buffer[1];
> > +
> > + buffer[0] = CMD_I2C_START;
> > +
> > + return diolan_usb_transfer(adapter, buffer, 1, buffer, 1);
> > +}
> > +
> > +static int diolan_i2c_repeated_start(struct i2c_adapter *adapter)
> > +{
> > + u8 buffer[1];
> > +
> > + buffer[0] = CMD_I2C_REPEATED_START;
> > +
> > + return diolan_usb_transfer(adapter, buffer, 1, buffer, 1);
> > +}
> > +
> > +static int diolan_i2c_stop(struct i2c_adapter *adapter)
> > +{
> > + u8 buffer[1];
> > +
> > + buffer[0] = CMD_I2C_STOP;
> > +
> > + return diolan_usb_transfer(adapter, buffer, 1, buffer, 1);
> > +}
> > +
> > +static int diolan_i2c_get_byte_ack(struct i2c_adapter *adapter, bool ack,
> > + u8 *byte)
> > +{
> > + u8 buffer[2];
> > + int rv;
>
> Why "rv" when all other functions use "ret"?
>
No idea. No, actually I do. It depends on where I copied the code from,
if I wrote it myself, and on the other driver I am working on in
parallel. Changed all to "ret".
> > +
> > + buffer[0] = CMD_I2C_GET_BYTE_ACK;
> > + buffer[1] = ack;
> > +
> > + rv = diolan_usb_transfer(adapter, buffer, 2, buffer, 2);
> > + if (rv > 0)
> > + *byte = buffer[0];
> > + else if (rv == 0)
> > + rv = -EIO;
> > +
> > + return rv;
> > +}
> > +
> > +static int diolan_i2c_put_byte_ack(struct i2c_adapter *adapter, u8 byte)
> > +{
> > + u8 buffer[2];
> > +
> > + buffer[0] = CMD_I2C_PUT_BYTE_ACK;
> > + buffer[1] = byte;
> > +
> > + return diolan_usb_transfer(adapter, buffer, 2, buffer, 1);
> > +}
> > +
> > +static int diolan_set_speed(struct i2c_adapter *adapter, u8 speed)
> > +{
> > + u8 buffer[2];
> > +
> > + buffer[0] = CMD_I2C_SET_SPEED;
> > + buffer[1] = speed;
> > +
> > + return diolan_usb_transfer(adapter, buffer, 2, buffer, 1);
> > +}
> > +
> > +static int diolan_fw_version(struct i2c_adapter *adapter)
> > +{
> > + u8 buffer[3];
> > + int ret;
> > +
> > + buffer[0] = CMD_GET_FW_VERSION;
> > + ret = diolan_usb_transfer(adapter, buffer, 1, buffer, 3);
> > + if (ret == 3)
> > + dev_info(&adapter->dev,
> > + "Diolan U2C firmware version %d.%d\n",
> > + buffer[0], buffer[1]);
>
> Unless you expect negative versions, %u would be more appropriate. Also
> note that to be completely correct you should cast the values to
> unsigned int before printing them.
>
Ok.
> > + return ret;
> > +}
> > +
> > +static int diolan_get_serial(struct i2c_adapter *adapter)
> > +{
> > + u8 buffer[5];
> > + int ret;
> > +
> > + buffer[0] = CMD_GET_SERIAL;
> > + ret = diolan_usb_transfer(adapter, buffer, 1, buffer, 5);
> > + if (ret == 5)
> > + dev_info(&adapter->dev,
> > + "Diolan U2C serial number %d\n", *(u32 *) &buffer[0]);
>
> Will the value be displayed correctly on big-endian machines? Doesn't
> seem so. You probably have to use le32_to_cpu().
> Also, %d to print an unsigned number isn't good.
>
Ok.
> > + return ret;
> > +}
> > +
> > +static int diolan_scan(struct i2c_adapter *adapter)
> > +{
> > + u8 buffer[257];
> > + int i, ret;
> > +
> > + buffer[0] = CMD_I2C_SCAN;
> > + ret = diolan_usb_transfer(adapter, buffer, 1, buffer, 257);
> > + if (ret > 0) {
> > + for (i = 0; i < ret - 1; i++) {
> > + if (buffer[i])
> > + dev_info(&adapter->dev,
> > + "Found I2C device at address 0x%x\n",
> > + buffer[i] >> 1);
> > + }
> > + }
> > + return ret;
> > +}
>
> I don't know how exactly the device is scanning for I2C slaves, but
> there is no provision for device discovery in the I2C specification. I
> wouldn't do that unconditionally at driver bind time, it might confuse
> some I2C slaves.
>
> If the user wants to probe for devices, we have i2c-dev + i2cdetect for
> this, which is more flexible.
>
Agreed. This is pretty much a leftover from testing. I took it out.
> > +
> > +/* i2c layer */
> > +
> > +static int usb_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
> > +{
> > + struct i2c_msg *pmsg;
> > + int i, j;
> > + int rc = 0;
>
> And now rc instead of ret as everywhere else? You are being creative ;)
>
Just trying to give you something to comment on ;)
> > +
> > + rc = diolan_i2c_start(adapter);
> > + if (rc < 0)
> > + return rc;
> > +
> > + for (i = 0; i < num; i++) {
> > + pmsg = &msgs[i];
> > + if (i) {
> > + rc = diolan_i2c_repeated_start(adapter);
> > + if (rc < 0)
> > + goto abort;
> > + }
> > + if (pmsg->flags & I2C_M_RD) {
> > + rc = diolan_i2c_put_byte_ack(adapter,
> > + ((pmsg->addr & 0x7f) << 1) | 1);
>
> Note that the mask is useless: the address is already a 7-bit value.
>
The variable is defined as __u16. But you are right, it should be 7-bit
since I did not register for 10-bit devices. I took it out. Makes the
code better readable.
> > + if (rc < 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;
> > +
> > + rc = diolan_i2c_get_byte_ack(adapter, ack,
> > + &byte);
> > + if (rc < 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) {
> > + rc = -EREMOTEIO;
>
> Should be -EPROTO according to Documentation/i2c/fault-codes.
>
Ok. Note that I got that from i2c-algo-bit.c.
> > + goto abort;
> > + }
> > + pmsg->len += byte;
> > + }
> > + pmsg->buf[j] = byte;
> > + }
> > + } else {
> > + rc = diolan_i2c_put_byte_ack(adapter,
> > + (pmsg->addr & 0x7f) << 1);
>
> Useless mask.
>
Ok.
> > + if (rc < 0)
> > + goto abort;
> > + for (j = 0; j < pmsg->len; j++) {
> > + rc = diolan_i2c_put_byte_ack(adapter,
> > + pmsg->buf[j]);
> > + if (rc < 0)
> > + goto abort;
> > + }
> > + }
> > + }
> > +abort:
> > + diolan_i2c_stop(adapter);
> > + return rc;
> > +}
> > +
> > +/*
> > + * Return list of supported functionality.
> > + */
> > +static u32 usb_func(struct i2c_adapter *a)
> > +{
> > + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
> > + I2C_FUNC_SMBUS_READ_BLOCK_DATA;
>
> Odd indentation/alignment.
>
Seems to be exactly what other drivers do, so I am a bit at loss here.
> As far as I can see you also support I2C_FUNC_SMBUS_BLOCK_PROC_CALL
> (even though it is not used by any driver I know of.)
>
I'll add it in.
> > +}
> > +
> > +static const struct i2c_algorithm usb_algorithm = {
> > + .master_xfer = usb_xfer,
> > + .functionality = usb_func,
> > +};
> > +
> > +/* device layer */
> > +
> > +static struct usb_device_id i2c_diolan_u2c_table[] = {
>
> Could this be made const?
>
Yes, done.
> > + {USB_DEVICE(USB_VENDOR_ID_DIOLAN, USB_DEVICE_ID_DIOLAN_U2C)},
> > + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(usb, i2c_diolan_u2c_table);
> > +
> > +static void i2c_diolan_u2c_free(struct i2c_diolan_u2c *dev)
> > +{
> > + usb_put_dev(dev->usb_dev);
> > + kfree(dev);
> > +}
> > +
> > +static int i2c_diolan_u2c_probe(struct usb_interface *interface,
> > + const struct usb_device_id *id)
> > +{
> > + struct i2c_diolan_u2c *dev;
> > + int retval = -ENOMEM;
>
> This initialization could be delayed to the point where you actually
> need it.
>
Ok.
> > +
> > + /* 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");
> > + 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);
> > +
> > + dev_info(&interface->dev,
> > + "Diolan U2C at bus %03d address %03d\n",
> > + dev->usb_dev->bus->busnum, dev->usb_dev->devnum);
> > +
> > + /* setup i2c adapter description */
> > + dev->adapter.owner = THIS_MODULE;
> > + dev->adapter.class = I2C_CLASS_HWMON;
> > + dev->adapter.algo = &usb_algorithm;
> > + dev->adapter.algo_data = dev;
>
> You are abusing algo_data here. You are supposed to use
> i2c_get/set_adapdata() instead. algo_data is only there for providing
> platform specific implementation details to generic i2c algorithms such
> as i2c-algo-bit.
>
Copied from i2c-tiny-usb.c. I didn't really think about it. Fixed.
> > + snprintf(dev->adapter.name, sizeof(dev->adapter.name),
> > + "i2c-u2c-usb at bus %03d device %03d",
> > + dev->usb_dev->bus->busnum, dev->usb_dev->devnum);
> > +
> > + dev->adapter.dev.parent = &dev->interface->dev;
> > +
> > + diolan_flush_input(dev->usb_dev);
> > +
> > + /* and finally attach to i2c layer */
> > + i2c_add_adapter(&dev->adapter);
>
> Please check for error here. It could happen!
>
Same as above. Added a check.
> > +
> > + diolan_fw_version(&dev->adapter);
>
> This seems racy, and the commands below as well. Serialization of calls
> to usb_xfer is guaranteed by i2c-core, but here you are calling other
> functions which will access your USB interface. I'm no USB expert, but
> diolan_usb_transfer() doesn't seem to be designed for parallel
> execution. As your i2c adapter is already registered, usb_xfer() could
> run in parallel with diolan_fw_version(), diolan_set_speed() etc.
>
> So either you add a mutex to serialize the access yourself (which will
> cause a run-time performance hit) or you do all your stuff _before_ the
> adapter is publicly usable.
>
Good catch. Moved to above i2c_add_adapter.
> > +
> > + retval = diolan_set_speed(&dev->adapter, U2C_I2C_FREQ_STD);
> > + if (retval < 0) {
> > + dev_err(&dev->adapter.dev,
> > + "failure %d setting I2C bus frequency\n", retval);
> > + goto error_del;
> > + }
>
> Beyond the race issue, you want to fully initialize the adapter before
> you make it visible to consumers, so speed should be set before calling
> i2c_add_adapter().
>
Ok, done.
> > + diolan_get_serial(&dev->adapter);
> > + diolan_scan(&dev->adapter);
> > +
> > + dev_dbg(&dev->adapter.dev, "connected i2c-u2c-usb device\n");
> > +
> > + return 0;
> > +
> > +error_del:
> > + i2c_del_adapter(&dev->adapter);
> > + i2c_diolan_u2c_free(dev);
> > +error:
> > + return retval;
> > +}
> > +
> > +static void i2c_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);
>
> If you have to do this here, then you also have to do it in the failure
> path of i2c_diolan_u2c_probe(), don't you?
>
Presumably. Another copy from tiny-usb.c
> > + i2c_diolan_u2c_free(dev);
> > +
> > + dev_dbg(&interface->dev, "disconnected\n");
> > +}
> > +
> > +static struct usb_driver i2c_diolan_u2c_driver = {
> > + .name = "i2c-u2c-usb",
>
> Why not "i2c-diolan-u2c" as the module name? Would be more consistent.
>
Agreed.
> > + .probe = i2c_diolan_u2c_probe,
> > + .disconnect = i2c_diolan_u2c_disconnect,
> > + .id_table = i2c_diolan_u2c_table,
> > +};
> > +
> > +static int __init usb_i2c_diolan_u2c_init(void)
> > +{
> > + /* register this driver with the USB subsystem */
> > + return usb_register(&i2c_diolan_u2c_driver);
> > +}
> > +
> > +static void __exit usb_i2c_diolan_u2c_exit(void)
> > +{
> > + /* deregister this driver with the USB subsystem */
> > + usb_deregister(&i2c_diolan_u2c_driver);
> > +}
> > +
> > +module_init(usb_i2c_diolan_u2c_init);
> > +module_exit(usb_i2c_diolan_u2c_exit);
> > +
> > +/* ----- end of usb layer ------------------------------------------------ */
>
> This comment is inconsistent (and useless, if you ask me.)
>
Another cut-and-paste thing. Removed.
> > +
> > +MODULE_AUTHOR("Guenter Roeck <[email protected]>");
> > +MODULE_DESCRIPTION("i2c-u2c-usb driver");
> > +MODULE_LICENSE("GPL");
>
>
> --
> Jean Delvare
Thanks a lot for the review!
Guenter
Hi Ben,
wow, this is creating way more interest than I thought...
On Thu, 2010-11-04 at 08:47 -0400, Ben Dooks wrote:
> On Wed, Nov 03, 2010 at 05:26:29PM -0700, Guenter Roeck wrote:
> > Signed-off-by: Guenter Roeck <[email protected]>
> > ---
> > This is an usb-i2c adapter I am using to connect to i2c evaluation and test
> > boards. Not sure if it is worth adding it into the kernel. If yes, I'll be
> > happy to add myself as maintainer.
> >
> > drivers/i2c/busses/Kconfig | 10 +
> > drivers/i2c/busses/Makefile | 1 +
> > drivers/i2c/busses/i2c-diolan-u2c.c | 455 +++++++++++++++++++++++++++++++++++
> > 3 files changed, 466 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 3a6321c..d73be36 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -640,6 +640,16 @@ config I2C_XILINX
> >
> > 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 84cb16a..46315db 100644
> > --- a/drivers/i2c/busses/Makefile
> > +++ b/drivers/i2c/busses/Makefile
> > @@ -63,6 +63,7 @@ obj-$(CONFIG_I2C_OCTEON) += i2c-octeon.o
> > obj-$(CONFIG_I2C_XILINX) += i2c-xiic.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..5f4fb74
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-diolan-u2c.c
> > @@ -0,0 +1,455 @@
> > +/*
> > + * driver for the Diolan u2c-12 usb adapter
> > + *
> > + * Copyright (c) 2010 Ericsson AB
> > + *
> > + * Derived from:
> > + * i2c-tiny-usb.c
> > + * Copyright (C) 2006-2007 Till Harbaum ([email protected])
> > + *
> > + * 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 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_SET_CLOCK_SYNCH 0x24
> > +#define CMD_GET_CLOCK_SYNCH 0x25
> > +#define CMD_SET_CLOCK_SYNCH_TO 0x26
> > +#define CMD_GET_CLOCK_SYNCH_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 U2C_I2C_FREQ_FAST 0 /* 400 kHz */
> > +#define U2C_I2C_FREQ_STD 1 /* 100 kHz */
> > +#define U2C_I2C_FREQ_83KHZ 2
> > +#define U2C_I2C_FREQ_71KHZ 3
> > +#define U2C_I2C_FREQ_62KHZ 4
> > +#define U2C_I2C_FREQ_50KHZ 6
> > +#define U2C_I2C_FREQ_25KHZ 16
> > +#define U2C_I2C_FREQ_10KHZ 46
> > +#define U2C_I2C_FREQ_5KHZ 96
> > +#define U2C_I2C_FREQ_2KHZ 242
> > +
> > +#define DIOLAN_USB_TIMEOUT 100
> > +
> > +/* 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 */
> > +};
> > +
> > +/* usb layer */
> > +
> > +static int diolan_usb_transfer(struct i2c_adapter *adapter, u8 * obuffer,
> > + int olen, u8 *ibuffer, int ilen)
> > +{
> > + struct i2c_diolan_u2c *dev = adapter->algo_data;
> > + int ret = 0;
> > + int actual;
> > + unsigned char inbuffer[257];
> > +
> > + if (olen) {
> > + ret = usb_bulk_msg(dev->usb_dev,
> > + usb_sndbulkpipe(dev->usb_dev, DIOLAN_OUT_EP),
> > + obuffer, olen, &actual, DIOLAN_USB_TIMEOUT);
> > + }
> > + if (!ret) {
> > + ret = usb_bulk_msg(dev->usb_dev,
> > + usb_rcvbulkpipe(dev->usb_dev, DIOLAN_IN_EP),
> > + inbuffer, sizeof(inbuffer), &actual,
> > + DIOLAN_USB_TIMEOUT);
> > + if (ret == 0 && actual > 0) {
> > + ret = min(actual, ilen);
> > + switch (inbuffer[actual - 1]) {
> > + case RESP_NACK:
> > + ret = -EINVAL;
> > + goto abort;
> > + case RESP_OK:
> > + break;
> > + default:
> > + ret = -EIO;
> > + goto abort;
> > + }
> > + if (ret)
> > + memcpy(ibuffer, inbuffer, ret);
> > + }
> > + }
> > +abort:
> > + 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 usb_device *dev)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < 10; i++) {
> > + int actual = 0;
> > + int ret;
> > + u8 inbuffer[257];
> > +
> > + ret = usb_bulk_msg(dev, usb_rcvbulkpipe(dev, DIOLAN_IN_EP),
> > + inbuffer, sizeof(inbuffer), &actual,
> > + DIOLAN_USB_TIMEOUT);
> > + if (ret < 0 || actual == 0)
> > + break;
> > + }
> > +}
> > +
> > +static int diolan_i2c_start(struct i2c_adapter *adapter)
> > +{
> > + u8 buffer[1];
> > +
> > + buffer[0] = CMD_I2C_START;
> > +
> > + return diolan_usb_transfer(adapter, buffer, 1, buffer, 1);
> > +}
> > +
> > +static int diolan_i2c_repeated_start(struct i2c_adapter *adapter)
> > +{
> > + u8 buffer[1];
>
> note, transfers to/from stack memory are not advised, esp. if this
> involves the hardware dma-mapping the stack. Please move the buffer
> to your private data.
>
Thanks for the heads-up. I was not aware that there is a potential
problem.
> > + buffer[0] = CMD_I2C_REPEATED_START;
> > +
> > + return diolan_usb_transfer(adapter, buffer, 1, buffer, 1);
> > +}
>
> why not doing something like
>
> diolan_usb_cmd1(struct i2c_adapter *adapter, u8 command)
> {
> struct i2c_diolan_u2c *dev = adapter->algo_data;
>
> dev->buffer[0] = command;
>
> return diolan_usb_treansfer(adapter, dev->buffer, 1, dev->buffer, 1);
> }
>
> then a lot of these little transfer functions could be turned into much
> smaller ones.
>
> ie:
>
> static int diolan_i2c_stop(struct i2c_adapter *adapter)
> {
> return diolan_usb_cmd1(adapter, CMD_I2C_STOP);
> }
>
Good idea. I'll do that.
> > +static int diolan_i2c_stop(struct i2c_adapter *adapter)
> > +{
> > + u8 buffer[1];
> > +
> > + buffer[0] = CMD_I2C_STOP;
> > +
> > + return diolan_usb_transfer(adapter, buffer, 1, buffer, 1);
> > +}
> > +
> > +static int diolan_i2c_get_byte_ack(struct i2c_adapter *adapter, bool ack,
> > + u8 *byte)
> > +{
> > + u8 buffer[2];
> > + int rv;
> > +
> > + buffer[0] = CMD_I2C_GET_BYTE_ACK;
> > + buffer[1] = ack;
> > +
> > + rv = diolan_usb_transfer(adapter, buffer, 2, buffer, 2);
> > + if (rv > 0)
> > + *byte = buffer[0];
> > + else if (rv == 0)
> > + rv = -EIO;
> > +
> > + return rv;
> > +}
>
> see notes about stack-mapping buffer.
>
Ok.
> > +static int diolan_i2c_put_byte_ack(struct i2c_adapter *adapter, u8 byte)
> > +{
> > + u8 buffer[2];
> > +
> > + buffer[0] = CMD_I2C_PUT_BYTE_ACK;
> > + buffer[1] = byte;
> > +
> > + return diolan_usb_transfer(adapter, buffer, 2, buffer, 1);
> > +}
> > +
> > +static int diolan_set_speed(struct i2c_adapter *adapter, u8 speed)
> > +{
> > + u8 buffer[2];
> > +
> > + buffer[0] = CMD_I2C_SET_SPEED;
> > + buffer[1] = speed;
> > +
> > + return diolan_usb_transfer(adapter, buffer, 2, buffer, 1);
> > +}
>
> maybe roll-up the 2 byte transfers in the same was a one-byte.
>
Ok.
> > +static int diolan_fw_version(struct i2c_adapter *adapter)
> > +{
> > + u8 buffer[3];
> > + int ret;
> > +
> > + buffer[0] = CMD_GET_FW_VERSION;
> > + ret = diolan_usb_transfer(adapter, buffer, 1, buffer, 3);
> > + if (ret == 3)
> > + dev_info(&adapter->dev,
> > + "Diolan U2C firmware version %d.%d\n",
> > + buffer[0], buffer[1]);
> > + return ret;
> > +}
> > +
> > +static int diolan_get_serial(struct i2c_adapter *adapter)
> > +{
> > + u8 buffer[5];
> > + int ret;
> > +
> > + buffer[0] = CMD_GET_SERIAL;
> > + ret = diolan_usb_transfer(adapter, buffer, 1, buffer, 5);
> > + if (ret == 5)
> > + dev_info(&adapter->dev,
> > + "Diolan U2C serial number %d\n", *(u32 *) &buffer[0]);
>
> *(u32 *) isn't nice, esp. as you're not going to know the endian-ness of
> the host you are running on. suggest trying a explict le32 ot be32 get of
> the data.
>
Yes, will do.
> > + return ret;
> > +}
>
> > +static int diolan_scan(struct i2c_adapter *adapter)
> > +{
> > + u8 buffer[257];
> > + int i, ret;
>
> suggest kmalloc() the buffer.
>
> > + buffer[0] = CMD_I2C_SCAN;
> > + ret = diolan_usb_transfer(adapter, buffer, 1, buffer, 257);
> > + if (ret > 0) {
> > + for (i = 0; i < ret - 1; i++) {
> > + if (buffer[i])
> > + dev_info(&adapter->dev,
> > + "Found I2C device at address 0x%x\n",
> > + buffer[i] >> 1);
> > + }
> > + }
> > + return ret;
>
> do you really need to do this, we've already got a userspace i2cdetect
> tool?
>
No, dropped.
> > +}
> > +
> > +/* i2c layer */
> > +
> > +static int usb_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
> > +{
> > + struct i2c_msg *pmsg;
> > + int i, j;
> > + int rc = 0;
> > +
> > + rc = diolan_i2c_start(adapter);
> > + if (rc < 0)
> > + return rc;
> > +
> > + for (i = 0; i < num; i++) {
> > + pmsg = &msgs[i];
> > + if (i) {
> > + rc = diolan_i2c_repeated_start(adapter);
> > + if (rc < 0)
> > + goto abort;
> > + }
> > + if (pmsg->flags & I2C_M_RD) {
> > + rc = diolan_i2c_put_byte_ack(adapter,
> > + ((pmsg->addr & 0x7f) << 1) | 1);
> > + if (rc < 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;
> > +
> > + rc = diolan_i2c_get_byte_ack(adapter, ack,
> > + &byte);
> > + if (rc < 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) {
> > + rc = -EREMOTEIO;
> > + goto abort;
> > + }
> > + pmsg->len += byte;
> > + }
> > + pmsg->buf[j] = byte;
> > + }
> > + } else {
> > + rc = diolan_i2c_put_byte_ack(adapter,
> > + (pmsg->addr & 0x7f) << 1);
> > + if (rc < 0)
> > + goto abort;
> > + for (j = 0; j < pmsg->len; j++) {
> > + rc = diolan_i2c_put_byte_ack(adapter,
> > + pmsg->buf[j]);
> > + if (rc < 0)
> > + goto abort;
> > + }
> > + }
> > + }
> > +abort:
> > + diolan_i2c_stop(adapter);
> > + return rc;
> > +}
> > +
> > +/*
> > + * Return list of supported functionality.
> > + */
> > +static u32 usb_func(struct i2c_adapter *a)
> > +{
> > + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
> > + I2C_FUNC_SMBUS_READ_BLOCK_DATA;
> > +}
>
> please make sure everything is prefixed with dio_ for nicer debugging.
>
Ok.
> > +static const struct i2c_algorithm usb_algorithm = {
> > + .master_xfer = usb_xfer,
> > + .functionality = usb_func,
> > +};
> > +
> > +/* device layer */
> > +
> > +static struct usb_device_id i2c_diolan_u2c_table[] = {
> > + {USB_DEVICE(USB_VENDOR_ID_DIOLAN, USB_DEVICE_ID_DIOLAN_U2C)},
>
> spaces between { and text please, same with }
>
Ok.
> > + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(usb, i2c_diolan_u2c_table);
> > +
> > +static void i2c_diolan_u2c_free(struct i2c_diolan_u2c *dev)
> > +{
> > + usb_put_dev(dev->usb_dev);
> > + kfree(dev);
> > +}
> > +
> > +static int i2c_diolan_u2c_probe(struct usb_interface *interface,
> > + const struct usb_device_id *id)
> > +{
> > + struct i2c_diolan_u2c *dev;
> > + int retval = -ENOMEM;
> > +
> > + /* 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");
> > + 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);
> > +
> > + dev_info(&interface->dev,
> > + "Diolan U2C at bus %03d address %03d\n",
> > + dev->usb_dev->bus->busnum, dev->usb_dev->devnum);
> > +
> > + /* setup i2c adapter description */
> > + dev->adapter.owner = THIS_MODULE;
> > + dev->adapter.class = I2C_CLASS_HWMON;
> > + dev->adapter.algo = &usb_algorithm;
> > + dev->adapter.algo_data = dev;
> > + snprintf(dev->adapter.name, sizeof(dev->adapter.name),
> > + "i2c-u2c-usb at bus %03d device %03d",
> > + dev->usb_dev->bus->busnum, dev->usb_dev->devnum);
> > +
> > + dev->adapter.dev.parent = &dev->interface->dev;
> > +
> > + diolan_flush_input(dev->usb_dev);
> > +
> > + /* and finally attach to i2c layer */
> > + i2c_add_adapter(&dev->adapter);
> > +
> > + diolan_fw_version(&dev->adapter);
> > +
> > + retval = diolan_set_speed(&dev->adapter, U2C_I2C_FREQ_STD);
> > + if (retval < 0) {
> > + dev_err(&dev->adapter.dev,
> > + "failure %d setting I2C bus frequency\n", retval);
> > + goto error_del;
> > + }
> > + diolan_get_serial(&dev->adapter);
> > + diolan_scan(&dev->adapter);
>
> do you really need to scan the bus on connection?
No, dropped.
> > + dev_dbg(&dev->adapter.dev, "connected i2c-u2c-usb device\n");
> > +
> > + return 0;
> > +
> > +error_del:
> > + i2c_del_adapter(&dev->adapter);
> > + i2c_diolan_u2c_free(dev);
> > +error:
> > + return retval;
> > +}
> > +
> > +static void i2c_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);
> > + i2c_diolan_u2c_free(dev);
> > +
> > + dev_dbg(&interface->dev, "disconnected\n");
> > +}
> > +
> > +static struct usb_driver i2c_diolan_u2c_driver = {
> > + .name = "i2c-u2c-usb",
> > + .probe = i2c_diolan_u2c_probe,
> > + .disconnect = i2c_diolan_u2c_disconnect,
> > + .id_table = i2c_diolan_u2c_table,
> > +};
> > +
> > +static int __init usb_i2c_diolan_u2c_init(void)
> > +{
> > + /* register this driver with the USB subsystem */
> > + return usb_register(&i2c_diolan_u2c_driver);
> > +}
> > +
> > +static void __exit usb_i2c_diolan_u2c_exit(void)
> > +{
> > + /* deregister this driver with the USB subsystem */
> > + usb_deregister(&i2c_diolan_u2c_driver);
> > +}
> > +
> > +module_init(usb_i2c_diolan_u2c_init);
> > +module_exit(usb_i2c_diolan_u2c_exit);
> > +
> > +/* ----- end of usb layer ------------------------------------------------ */
> > +
> > +MODULE_AUTHOR("Guenter Roeck <[email protected]>");
> > +MODULE_DESCRIPTION("i2c-u2c-usb driver");
> > +MODULE_LICENSE("GPL");
>
> --
> Ben
>
> Q: What's a light-year?
> A: One-third less calories than a regular year.
>
Thanks a lot for the review!
Guenter
Hi Guenter,
On Thu, 4 Nov 2010 09:41:42 -0700, Guenter Roeck wrote:
> On Thu, 2010-11-04 at 08:43 -0400, Jean Delvare wrote:
> > I'm also unsure what is the point of having such a large buffer when
> > the largest block you ever transfer in practice is 5 bytes?
>
> I took that from the Diolan code. They always use a 257 byte temp
> buffer, since that is the maximum data size sent by the adapter.
> You are right, I should not really need that since I don't send any long
> commands. Ultimate reason is to account for possible adapter errors, if
> it replies (or tries to reply) with more bytes than expected. Pretty
> much just playing safe.
Which commands are they using, which require such a large buffer? In
your driver, bytes are all processed one by one, which is certainly not
good performance-wise. If there is a way to read or write mode than one
byte at a time, this would be worth a try.
> > (...)
> > BTW, I'm not sure why you don't use the original buffer directly?
> > memcpy is bad performance-wise.
>
> To account for the possibility that the adapter returns more bytes than
> I am expecting. Sure, that would be a bug, but I wanted to play safe.
Hmm, OK, that makes sense.
> > (...)
> > Should be -EPROTO according to Documentation/i2c/fault-codes.
>
> Ok. Note that I got that from i2c-algo-bit.c.
I would welcome a patch fixing this.
> > > (...)
> > > +static u32 usb_func(struct i2c_adapter *a)
> > > +{
> > > + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
> > > + I2C_FUNC_SMBUS_READ_BLOCK_DATA;
> >
> > Odd indentation/alignment.
>
> Seems to be exactly what other drivers do, so I am a bit at loss here.
Really? Using 4 spaces for indentation is wrong. Either use a tab, or
align using 7 spaces.
> > (...)
> > You are abusing algo_data here. You are supposed to use
> > i2c_get/set_adapdata() instead. algo_data is only there for providing
> > platform specific implementation details to generic i2c algorithms such
> > as i2c-algo-bit.
>
> Copied from i2c-tiny-usb.c. I didn't really think about it. Fixed.
I take patches ;)
--
Jean Delvare
Hi Jean,
On Thu, 2010-11-04 at 16:18 -0400, Jean Delvare wrote:
> Hi Guenter,
>
> On Thu, 4 Nov 2010 09:41:42 -0700, Guenter Roeck wrote:
> > On Thu, 2010-11-04 at 08:43 -0400, Jean Delvare wrote:
> > > I'm also unsure what is the point of having such a large buffer when
> > > the largest block you ever transfer in practice is 5 bytes?
> >
> > I took that from the Diolan code. They always use a 257 byte temp
> > buffer, since that is the maximum data size sent by the adapter.
> > You are right, I should not really need that since I don't send any long
> > commands. Ultimate reason is to account for possible adapter errors, if
> > it replies (or tries to reply) with more bytes than expected. Pretty
> > much just playing safe.
>
> Which commands are they using, which require such a large buffer? In
> your driver, bytes are all processed one by one, which is certainly not
> good performance-wise. If there is a way to read or write mode than one
> byte at a time, this would be worth a try.
>
There are read/write block commands. Unfortunately, I never got it
working reliably :-(.
> > > (...)
> > > BTW, I'm not sure why you don't use the original buffer directly?
> > > memcpy is bad performance-wise.
> >
> > To account for the possibility that the adapter returns more bytes than
> > I am expecting. Sure, that would be a bug, but I wanted to play safe.
>
> Hmm, OK, that makes sense.
>
> > > (...)
> > > Should be -EPROTO according to Documentation/i2c/fault-codes.
> >
> > Ok. Note that I got that from i2c-algo-bit.c.
>
> I would welcome a patch fixing this.
>
Ok, I'll put that on my list. Note that there are several other i2c
drivers returning -EREMOTEIO.
> > > > (...)
> > > > +static u32 usb_func(struct i2c_adapter *a)
> > > > +{
> > > > + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
> > > > + I2C_FUNC_SMBUS_READ_BLOCK_DATA;
> > >
> > > Odd indentation/alignment.
> >
> > Seems to be exactly what other drivers do, so I am a bit at loss here.
>
> Really? Using 4 spaces for indentation is wrong. Either use a tab, or
> align using 7 spaces.
>
Ok.
> > > (...)
> > > You are abusing algo_data here. You are supposed to use
> > > i2c_get/set_adapdata() instead. algo_data is only there for providing
> > > platform specific implementation details to generic i2c algorithms such
> > > as i2c-algo-bit.
> >
> > Copied from i2c-tiny-usb.c. I didn't really think about it. Fixed.
>
> I take patches ;)
>
Ok. Another one to look at if I need a break ;).
Guenter