Re: [PATCH 2/2] i2c: Add bus driver for for OSIF USB i2c device.

[Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4364 bytes --]

On Mon, Jan 06, 2014 at 04:55:36AM +0100, Andrew Lunn wrote:
> OSIF, Open Source InterFace, is a USB based i2c bus master.  The
> origional design was based on i2c-tiny-usb, but more modern versions
> of the firmware running on the MegaAVR microcontroller use a different
> protocol over the USB. This code is based on Barry Carter
> <barry.carter-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> driver.
> 
> Signed-off-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>

We are getting close...


> +#define OSIFI2C_READ		20 /* Read from i2c bus */
> +#define OSIFI2C_WRITE		21 /* Write to i2c bus */
> +#define OSIFI2C_STOP		22 /* Send stop condition */
> +#define OSIFI2C_STATUS		23 /* Get status from i2c action */
> +#define OSIFI2C_SET_BIT_RATE	24 /* Set the bit rate & prescaler */

Comments could be seen as obvious, too. I'll leave the decision to you.

> +struct priv {

Namespace: 'osif_priv' please.

> +	struct usb_device *usb_dev;
> +	struct usb_interface *interface;
> +	struct i2c_adapter adapter;
> +	unsigned char status;
> +};
> +
> +static int usb_read(struct i2c_adapter *adapter, int cmd,

Namespace: 'osif_usb_read'.

> +		    int value, int index, void *data, int len)
> +{
> +	struct priv *priv = (struct priv *)adapter->algo_data;

No need to cast a void*.

> +
> +	return usb_control_msg(priv->usb_dev, usb_rcvctrlpipe(priv->usb_dev, 0),
> +			       cmd, USB_TYPE_VENDOR | USB_RECIP_INTERFACE |
> +			       USB_DIR_IN, value, index, data, len, 2000);
> +}
> +
> +static int usb_write(struct i2c_adapter *adapter, int cmd,

Namespace

> +		    int value, int index, void *data, int len)
> +{
> +
> +	struct priv *priv = (struct priv *)adapter->algo_data;

no cast.

> +
> +	return usb_control_msg(priv->usb_dev, usb_sndctrlpipe(priv->usb_dev, 0),
> +			       cmd, USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
> +			       value, index, data, len, 2000);
> +}
> +
> +static int usb_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)

namespace

> +{
> +	struct priv *priv = (struct priv *)adapter->algo_data;

no cast

> +	struct i2c_msg *pmsg;
> +	int ret = 0;
> +	int i, cmd;
> +
> +	for (i = 0; ret >= 0 && i < num; i++) {
> +		pmsg = &msgs[i];
> +
> +		if (pmsg->flags & I2C_M_RD) {
> +			cmd = OSIFI2C_READ;
> +
> +			ret = usb_read(adapter, cmd, pmsg->flags, pmsg->addr,
> +				       pmsg->buf, pmsg->len);
> +			if (ret != pmsg->len) {
> +				dev_err(&adapter->dev,
> +					"failure reading data\n");

One line is more readable IMO. 80 chars is not a hard limit.

> +				return -EREMOTEIO;
> +			}
> +		} else {
> +			cmd = OSIFI2C_WRITE;
> +
> +			ret = usb_write(adapter, cmd, pmsg->flags, pmsg->addr,
> +					pmsg->buf, pmsg->len);
> +			if (ret != pmsg->len) {
> +				dev_err(&adapter->dev,
> +					"failure writing data\n");

ditto

> +				return -EREMOTEIO;
> +			}
> +		}
> +
> +		ret = usb_read(adapter, OSIFI2C_STOP, 0, 0, NULL, 0);
> +		if (ret != 0) {

'if (ret)' is enough

> +			dev_err(&adapter->dev, "failure sending STOP\n");
> +			return -EREMOTEIO;
> +		}
> +
> +		/* read status */
> +		ret = usb_read(adapter, OSIFI2C_STATUS, 0, 0, &priv->status, 1);
> +		if (ret != 1) {
> +			dev_err(&adapter->dev, "failure reading status\n");
> +			return -EREMOTEIO;
> +		}
> +
> +		if (priv->status != STATUS_ADDRESS_ACK) {
> +			dev_dbg(&adapter->dev, "status = %d\n", priv->status);
> +			return -EREMOTEIO;
> +		}
> +	}
> +
> +	return i;
> +}
> +
> +static u32 usb_func(struct i2c_adapter *adapter)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;

Have you tried SMBUS_QUICK by the way?

> +}
> +
> +static struct i2c_algorithm usb_algorithm = {
> +	.master_xfer	= usb_xfer,
> +	.functionality	= usb_func,
> +};
> +
> +#define USB_OSIF_VENDOR_ID	0x1964
> +#define USB_OSIF_PRODUCT_ID	0x0001
> +
> +static struct usb_device_id osif_table[] = {
> +	{ USB_DEVICE(USB_OSIF_VENDOR_ID, USB_OSIF_PRODUCT_ID) },
> +	{ }
> +};
> +

Empty line can go.

> +MODULE_DEVICE_TABLE(usb, osif_table);
> +
> +static int osif_probe(struct usb_interface *interface,
> +			     const struct usb_device_id *id)
> +{
> +	int retval;

maybe just 'ret' for consistency? Very minor nit, though...

> +	struct priv *priv;
> +	u16 version;

Rest looks ready to go!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]