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 <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. > 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 (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? > 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 <guenter.roeck@xxxxxxxxxxxx>"); > > +MODULE_DESCRIPTION("i2c-u2c-usb driver"); > > +MODULE_LICENSE("GPL"); > > > -- > Jean Delvare Thanks a lot for the review! Guenter -- 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
