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

[Oliver Bock]

On Saturday 10 June 2006 00:49, Pavel Machek wrote:
> Sorry, now the review begins...

No problem at all.

> > +/* 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.

Ok, I missed that one and remembered only the if-styling. I'll change it and 
I'm glad to see that as it's according to my style.

> > +#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..

Well, thanks but I think I've to "share the price" with at least one other:
drivers/usb/misc/phidgetservo.c

I agree that this is no excuse for bad style, but I was just trying to keep 
the code compliant with the kernel coding conventions - my personal style 
looks a bit different anyway. I tried to avoid any formatting issues by 
looking at other drivers and the one mentioned above was recommended to me 
when I did the porting from ioctls to sysfs. Due to this I assumed that this 
might be a common way you guys try to avoid redundant code...

To be sure (and because he's the author of the USB skeleton I also used) I 
asked Greg K-H for an initial review of my code before I sent it to the list, 
and he didn't complain a bit about this marco. So is there any common rule 
for this?

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

Hm, the chipset family is just called like that and there're at least three 
other Cypress related drivers (cypress, cypress_m8 and cytherm) with generic 
names. I think this name shows clearly what kind of device it supports, 
doesn't it?

Apart from that there are again other drivers (ark3116.c, cp2101.c) which do 
it the same way, and I assumed that this might be some sort of naming 
convention...

> > +	/* 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.

You mean the whole string (line) one character to the right, correct?


Regards,
Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/