[i2c] "driver forgot to specify physical device"

Jean Delvare khali at linux-fr.org
Wed Apr 18 12:07:35 CEST 2007


Hi Till,

On Tue, 17 Apr 2007 19:14:04 +0200, Till Harbaum / Lists wrote:
> ok, that did the trick. Thanks a lot. I read the coding style as you
> suggested and the attached version should meet it ... i hope.
> 
> Can you please have another look?

First of all: please provide your driver as a patch against a recent
kernel tree. Something that I can easily apply and test-compile.

> /*
>  * driver for the i2c-tiny-usb adapter - 1.0
>  * http://www.harbaum.org/till/i2c_tiny_usb
>  *
>  * Copyright (C) 2006-2007 Till Harbaum (Till at Harbaum.org)
>  *
>  *	This program is free software; you can redistribute it and/or
>  *	modify it under the terms of the GNU General Public License as
>  *	published by the Free Software Foundation, version 2.
>  *
>  * This driver is based on the 2.6.3 version of drivers/usb/usb-skeleton.c
>  * but has been rewritten to be easy to read and use, as no locks are now
>  * needed anymore.

If the skeleton driver is out-of-date, can you please submit an update
for it? It would be more valuable for the community than this comment
(which quite frankly doesn't add any value to your driver.)

>  *
>  */
> 
> #include <linux/kernel.h>
> #include <linux/errno.h>
> #include <linux/init.h>
> #include <linux/slab.h>
> #include <linux/module.h>
> #include <linux/kref.h>

Do you really need this one?

> #include <asm/uaccess.h>

And this one?

> 
> /* include interfaces to usb layer */
> #include <linux/usb.h>
> 
> /* include interface to i2c layer */
> #include <linux/i2c.h>

Also, <asm/...> includes should always come after <linux/...> includes.

> 
> /* Version Information */
> #define DRIVER_VERSION "v1.0"
> #define DRIVER_AUTHOR  "Till Harbaum <Till at Harbaum.org>"
> #define DRIVER_DESC    "i2c-tiny-usb driver"
> #define DRIVER_URL     "http://www.harbaum.org/till/i2c_tiny_usb"

All alignments must be done using tabs, not spaces.

Also, these defines aren't quite needed. Some are used only once, the
last one isn't even used at all. You'd rather call MODULE_VERSION,
MODULE_AUTHOR and MODULE_DESCRIPTION directly on the strings and be
done with it.

> 
> /* commands via USB, must e.g. match command ids in the firmware */

The "e.g." doesn't make sense here, it's not an example.

> #define CMD_ECHO       0

Not used anywhere.

> #define CMD_GET_FUNC   1
> #define CMD_SET_DELAY  2
> #define CMD_GET_STATUS 3
> 
> #define CMD_I2C_IO     4
> #define CMD_I2C_BEGIN  1     /* flag to I2C_IO */
> #define CMD_I2C_END    2     /* flag to I2C_IO */

I would suggest:
#define CMD_I2C_IO_BEGIN	(1 << 0)
#define CMD_I2C_IO_END		(1 << 1)

That way it's immediately obvious that these are flags related to to
CMD_I2C_IO.

> 
> #define DEFAULT_DELAY  10    /* default is 10us -> 100kHz */
> static int delay;

Please don't split this from the module_param call.

> 
> static int usb_read(struct i2c_adapter *i2c_adap, int cmd,
> 		     int value, int index, void *data, int len);
> 
> static int usb_write(struct i2c_adapter *adapter, int cmd,
> 		      int value, int index, void *data, int len);

Indentation of the second line is off-by-one each time.

> 
> /* ----- begin of i2c layer ---------------------------------------------- */
> 
> #define STATUS_IDLE          0
> #define STATUS_ADDRESS_ACK   1
> #define STATUS_ADDRESS_NAK   2
> 
> static int usb_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
> {
> 	unsigned char status;
> 	struct i2c_msg *pmsg;
> 	int i;
> 	int ret=0;

You don't actually use this variable.

> 
> 	dbg("master xfer %d messages:", num);

Please use dev_dbg() instead. Same for others call to dbg(), info()
etc. These are redundant macros defined in <linux/usb.h>, they should
be discarded. Use the dev_*() variants wherever possible instead, and
pr_info() and pr_debug() the rest of the time.

And _always_ finish your prints with a new line.

> 
> 	for (i = 0;ret >= 0 && i < num; i++) {
> 		int cmd = CMD_I2C_IO;
> 
> 		if (i == 0)
> 			cmd |= CMD_I2C_BEGIN;
> 
> 		if (i == num-1)
> 			cmd |= CMD_I2C_END;
> 
> 		pmsg = &msgs[i];
> 
> 		dbg("  %d: %s (flags %d) %d bytes to 0x%02x",
> 		    i, pmsg->flags & I2C_M_RD ? "read" : "write", pmsg->flags,
> 		    pmsg->len, pmsg->addr);
> 
> 		/* and directly send the message */
> 		if (pmsg->flags & I2C_M_RD) {
> 			/* read data */
> 			if (usb_read(adapter, cmd,
> 				     pmsg->flags, pmsg->addr,
> 				     pmsg->buf, pmsg->len) != pmsg->len) {
> 				

Drop this blank line.

> 				err("failure reading data");
> 				return -EREMOTEIO;
> 			}
> 		} else {
> 			/* write data */
> 			if (usb_write(adapter, cmd,
> 				      pmsg->flags, pmsg->addr,
> 				      pmsg->buf, pmsg->len) != pmsg->len) {
> 				err("failure writing data");
> 				return -EREMOTEIO;
> 			}
> 		}
> 
> 		/* read status */
> 		if (usb_read(adapter, CMD_GET_STATUS, 0, 0, &status, 1) != 1) {
> 			err("failure reading status");
> 			return -EREMOTEIO;
> 		}
> 
> 		dbg("  status = %d", status);
> 		if (status == STATUS_ADDRESS_NAK)
> 			return -EREMOTEIO;
> 	}
> 
> 	return i;
> }
> 
> static u32 usb_func(struct i2c_adapter *adapter)
> {
> 	u32 func;
> 
> 	/* get functionality from adapter */
> 	if (usb_read(adapter, CMD_GET_FUNC, 0, 0, &func, sizeof(func)) !=
> 	    sizeof(func)) {
> 

Extra blank line.

> 		err("failure reading functionality\n");
> 		return 0;
> 	}
> 
> 	info("got adapter functionality %x", func);
> 	return func;
> }
> 
> /* This is the actual algorithm we define */
> static struct i2c_algorithm usb_algorithm = {

You should be able to declare it const, too.

> 	.master_xfer	= usb_xfer,
> 	.functionality  = usb_func,

Use a tab for alignment, not spaces.

> };
> 
> /* ----- end of i2c layer ------------------------------------------------ */
> 
> /* ----- begin of usb layer ---------------------------------------------- */
> 
> /* the usb i2c interface uses a vid/pid pair donated by ftdi */

What is "ftdi"?

> #define USB_I2C_TINY_USB_VENDOR_ID	0x0403
> #define USB_I2C_TINY_USB_PRODUCT_ID	0xc631

Could this be added to usb.ids?

BTW, I don't think there is much value in having defines for these -
putting them directly in the table below would work just fine.

> 
> /* table of devices that work with this driver */
> static struct usb_device_id i2c_tiny_usb_table [] = {
> 	{ USB_DEVICE(USB_I2C_TINY_USB_VENDOR_ID, USB_I2C_TINY_USB_PRODUCT_ID) },
> 	{ }					/* Terminating entry */
> };

Could be const too, I think.

> 
> MODULE_DEVICE_TABLE (usb, i2c_tiny_usb_table);

Remove the space before the opening parenthesis.

> 
> /* Structure to hold all of our device specific stuff */
> struct i2c_tiny_usb {
> 	struct usb_device *usb_dev;	   /* the usb device for this device */
> 	struct usb_interface *interface;   /* the interface for this device */
> 
> 	/* i2c related things */
> 	struct i2c_adapter      i2c_adap;
> };

Alignment is inconsistent.

> 
> static int usb_read(struct i2c_adapter *adapter, int cmd,
> 		    int value, int index, void *data, int len)
> {
> 	struct i2c_tiny_usb *dev = (struct i2c_tiny_usb *)adapter->algo_data;
> 	int retval;
> 
> 	/* do control transfer */
> 	retval = usb_control_msg(dev->usb_dev, usb_rcvctrlpipe(dev->usb_dev, 0),
> 				 cmd, USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_IN,

Line too long, please fold.

> 				 value, index, data, len, 2000);
> 
> 	return retval;
> }
> 
> static int usb_write(struct i2c_adapter *adapter, int cmd,
> 		     int value, int index, void *data, int len)
> {
> 	struct i2c_tiny_usb *dev = (struct i2c_tiny_usb *)adapter->algo_data;
> 	int retval;
> 
> 	/* do control transfer */
> 	retval = usb_control_msg(dev->usb_dev, usb_sndctrlpipe(dev->usb_dev, 0),
> 				 cmd, USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
> 				 value, index, data, len, 2000);
> 
> 	return retval;
> }

Did you try inlining these two functions? I suspect your driver would
be faster _and_ smaller.

You don't really need retval, BTW.

> 
> static void i2c_tiny_usb_free(struct i2c_tiny_usb *dev)
> {
> 	usb_put_dev(dev->usb_dev);
> 	kfree(dev);
> }
> 
> static int i2c_tiny_usb_probe(struct usb_interface *interface,
> 			 const struct usb_device_id *id)

Strange indentation.

> {
> 	struct i2c_tiny_usb *dev = NULL;

Useless initialization.

> 	int retval = -ENOMEM;
> 	u16 version;
> 
> 	dbg("probing usb device");
> 
> 	/* allocate memory for our device state and initialize it */
> 	dev = kmalloc(sizeof(*dev), GFP_KERNEL);
> 	if (dev == NULL) {
> 		err("Out of memory");
> 		goto error;
> 	}
> 
> 	/* clear memory */
> 	memset(dev, 0, sizeof(*dev));

Please use kzalloc instead.

> 
> 	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);
> 
> 	version = le16_to_cpu(dev->usb_dev->descriptor.bcdDevice);
> 	info("version %x.%02x found at bus %03d address %03d",
> 	     version>>8, version&0xff,  

Please add spaces around ">>" and "&" for better readability.

> 	     dev->usb_dev->bus->busnum, dev->usb_dev->devnum);
> 
> 	/* setup i2c adapter description */
> 	dev->i2c_adap.owner	  = THIS_MODULE;
> 	dev->i2c_adap.class	  = I2C_CLASS_HWMON;
> 	dev->i2c_adap.algo	  = &usb_algorithm;
> 	dev->i2c_adap.algo_data   = dev;

This kind of alignment inside the code is discouraged.

> 	sprintf(dev->i2c_adap.name, "i2c-tiny-usb at bus %03d device %03d",
> 		dev->usb_dev->bus->busnum, dev->usb_dev->devnum);

Unsafe, please use snprintf.

> 
> 	/* set default delay value */
> 	if (!delay) 
> 	        delay = DEFAULT_DELAY;

Why don't you simply initialize delay to DEFAULT_DELAY at module level?

> 
> 	if (usb_write(&(dev->i2c_adap), CMD_SET_DELAY,

Parentheses around dev->i2c_adap aren't needed.

> 		      le16_to_cpu(delay), 0, NULL, 0) != 0) {

This looks broken, shouldn't it be cpu_to_le16? (Which I guess is the
same, but...)

> 		err("failure setting delay to %dus", delay);
> 		retval = -EIO;
> 		goto error;
> 	}
> 
> 	dev->i2c_adap.dev.parent = &dev->interface->dev;
> 
> 	/* and finally attach to i2c layer */
> 	i2c_add_adapter(&(dev->i2c_adap));

Here too, innermost parentheses aren't needed.

> 
> 	/* inform user about successful attachment to i2c layer */
> 	dev_info(&dev->i2c_adap.dev, "connected i2c-tiny-usb device\n");
> 
> 	return 0;
> 
>  error:
> 	if (dev)
> 		i2c_tiny_usb_free(dev);
> 
> 	return retval;
> }
> 
> static void i2c_tiny_usb_disconnect(struct usb_interface *interface)
> {
> 	struct i2c_tiny_usb *dev = usb_get_intfdata(interface);
> 
> 	i2c_del_adapter(&(dev->i2c_adap));

Unneeded parentheses.

> 
> 	usb_set_intfdata(interface, NULL);
> 
> 	i2c_tiny_usb_free(dev);
> 
> 	dbg("disconnected");
> }

I think you can drop a few blank lines in this function.

> 
> static struct usb_driver i2c_tiny_usb_driver = {
> 	.name =	        "i2c-tiny-usb",
> 	.probe =	i2c_tiny_usb_probe,
> 	.disconnect =	i2c_tiny_usb_disconnect,
> 	.id_table =	i2c_tiny_usb_table,
> };

Align with tabs not spaces. This could be made const.

> 
> static int __init usb_i2c_tiny_usb_init(void)
> {
> 	int result;
> 
> 	info(DRIVER_DESC " " DRIVER_VERSION " of " __DATE__);
> 
> 	/* register this driver with the USB subsystem */
> 	result = usb_register(&i2c_tiny_usb_driver);
> 	if (result)
> 		err("usb_register failed. Error number %d", result);
> 
> 	return result;
> }

No need for an error message, modprobe will give the error code. As for
the info messages, all other i2c bus drivers are silent when being
loaded.

> 
> static void __exit usb_i2c_tiny_usb_exit(void)
> {
> 	/* deregister this driver with the USB subsystem */
> 	usb_deregister(&i2c_tiny_usb_driver);
> }
> 
> module_init(usb_i2c_tiny_usb_init);
> module_exit(usb_i2c_tiny_usb_exit);
> 
> /* ----- end of usb layer ------------------------------------------------ */
> 
> module_param(delay, int, 0);

Please add a parameter description for this one.

> 
> MODULE_AUTHOR( DRIVER_AUTHOR );
> MODULE_DESCRIPTION( DRIVER_DESC );

No spaces inside the parentheses please.

> MODULE_LICENSE("GPL");


-- 
Jean Delvare



More information about the i2c mailing list