On Wed, 3 Nov 2010 17:26:29 -0700, Guenter Roeck wrote: > Signed-off-by: Guenter Roeck <guenter.roeck@xxxxxxxxxxxx> > --- > 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 (Till@xxxxxxxxxxx) > + * > + * 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 <guenter.roeck@xxxxxxxxxxxx>"); > +MODULE_DESCRIPTION("i2c-u2c-usb driver"); > +MODULE_LICENSE("GPL"); -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
