Re: [PATCH 1/1] usb: new driver for Cypress CY7C63xxx mirco controllers

[Pavel Machek <pavel@ucw.cz>]

Hi!

Sorry, now the review begins...

> +/* used to send usb control messages to device */
> +int vendor_command(struct cy7c63 *dev, unsigned char request,
> +			 unsigned char address, unsigned char data) {

Codingstyle: { goes to new line.


> +#define get_set_port(num,read_id,write_id) \
> +static ssize_t set_port##num(struct device *dev, struct device_attribute *attr,	\
> +					const char *buf, size_t count) {	\
> +										\
> +	int value;								\
> +	int result = 0;								\
> +										\
> +	struct usb_interface *intf = to_usb_interface(dev);			\
> +	struct cy7c63 *cyp = usb_get_intfdata(intf);				\
> +										\
> +	dev_dbg(&cyp->udev->dev, "WRITE_PORT%d called\n", num);			\
> +										\
> +	/* validate input data */						\
> +	if (sscanf(buf, "%d", &value) < 1) {					\
> +		result = -EINVAL;						\
> +		goto error;							\
> +	}									\
> +	if (value>255 || value<0) {						\
> +		result = -EINVAL;						\
> +		goto error;							\
> +	}									\
> +										\
> +	result = vendor_command(cyp, CY7C63_WRITE_PORT, write_id,		\
> +					 (unsigned char)value);			\
> +										\
> +	dev_dbg(&cyp->udev->dev, "Result of vendor_command: %d\n\n",result);	\
> +error:										\
> +	return result < 0 ? result : count;					\
> +}										\
> +										\
> +static ssize_t get_port##num(struct device *dev,				\
> +				 struct device_attribute *attr, char *buf) {	\
> +										\
> +	int result = 0;								\
> +										\
> +	struct usb_interface *intf = to_usb_interface(dev);			\
> +	struct cy7c63 *cyp = usb_get_intfdata(intf);				\
> +										\
> +	dev_dbg(&cyp->udev->dev, "READ_PORT%d called\n", num);			\
> +										\
> +	result = vendor_command(cyp, CY7C63_READ_PORT, read_id, 0);		\
> +										\
> +	dev_dbg(&cyp->udev->dev, "Result of vendor_command: %d\n\n", result);	\
> +										\
> +	return sprintf(buf, "%d", cyp->port##num);				\
> +}										\
> +static DEVICE_ATTR(port##num, S_IWUGO | S_IRUGO, get_port##num, set_port##num);
> +
> +get_set_port(0, CY7C63_READ_PORT_ID0, CY7C63_WRITE_PORT_ID0);
> +get_set_port(1, CY7C63_READ_PORT_ID1, CY7C63_WRITE_PORT_ID1);

You get "best abuse of the macros" prize. Can you just use functions,
and pass num as aditional argument? Then just wrap the long functions
in small ones... converting cyp->port0/1 into array will be handy..


> +static int cy7c63_probe(struct usb_interface *interface,
> +			const struct usb_device_id *id) {

{ on new line, please...

BTW could we get come better name for the driver? cy7c63 looks like
password of very paranoid sysadmin.

> +	/* let the user know what node this device is now attached to */
> +	dev_info(&interface->dev,
> +		"Cypress CY7C63xxx device now attached\n");

In cases like this we aling " one character to the right.

Otherwise it looks okay.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html