Re: [RESEND PATCH v1 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: 2918 bytes --]

Hi, thanks for the submission...


> +#define DRIVER_AUTHOR  "Barry Carter <barry.carter-hIwygO75nSHCXR85Y8Hu3Q@public.gmane.org>,"	\
> +	"Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>"
> +#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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]