Hi, thanks for the submission...
> +#define DRIVER_AUTHOR "Barry Carter <barry.carter@xxxxxxxxxxxxx>," \
> + "Andrew Lunn <andrew@xxxxxxx>"
> +#define DRIVER_DESC "OSIF driver"
No defines, use directly.
> +static int usb_read(struct i2c_adapter *adapter, 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);
With a bit of reshuffling, these can be skipped.
> + dev_dbg(&adapter->dev, "master xfer %d messages:\n", num);
Skip. This info is available with I2C core debug messages.
> + pstatus = kmalloc(sizeof(*pstatus), GFP_KERNEL);
> + if (!pstatus)
> + return -ENOMEM;
Does it really make sense to allocate this every time?
> + dev_dbg(&adapter->dev,
> + " %d: %s (flags %d) %d bytes to 0x%02x\n",
> + i, pmsg->flags & I2C_M_RD ? "read" : "write",
> + pmsg->flags, pmsg->len, pmsg->addr);
This is also available with i2c core debug messages.
> +/* Structure to hold all of our device specific stuff */
> +struct priv {
> + 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 */
> +};
Remove comments, too obvious IMO.
> +static int usb_read(struct i2c_adapter *adapter, int cmd,
> + int value, int index, void *data, int len)
> +{
> + struct priv *priv = (struct priv *)adapter->algo_data;
> +
> + /* do control transfer */
ditto.
> + 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,
> + int value, int index, void *data, int len)
> +{
> +
> + struct priv *priv = (struct priv *)adapter->algo_data;
> +
> + /* do control transfer */
ditto.
> + 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 void osif_free(struct priv *priv)
> +{
> + usb_put_dev(priv->usb_dev);
> +}
Not sure if this is worth a seperate function, but well...
> +static int osif_probe(struct usb_interface *interface,
> + const struct usb_device_id *id)
> +{
> + struct priv *priv = NULL;
Unneeded assignment.
> + int retval = -ENOMEM;
> + u16 version;
> +
> + dev_dbg(&interface->dev, "probing usb device");
This is available via driver core debug messages.
> + /* inform user about successful attachment to i2c layer */
> + dev_info(&priv->adapter.dev, "connected OSIF device\n");
I think this message should be merged with the "version xx found"
message above.
> +MODULE_LICENSE("GPL");
"GPL v2" according to header.
Attachment:
signature.asc
Description: Digital signature
