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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
