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@xxxxxxxxx> driver.
>
> Signed-off-by: Andrew Lunn <andrew@xxxxxxx>
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:
signature.asc
Description: Digital signature
