Date: Sun, 23 Sep 2007 13:34:59 -0400 From: "Constantine A. Murenin" <cnst@FreeBSD.org> To: Hans Petter Selasky <hselasky@FreeBSD.org> Cc: Perforce Change Reviews <perforce@FreeBSD.org>, "Constantine A. Murenin" <cnst@FreeBSD.org> Subject: Re: PERFORCE change 126745 for review Message-ID: <46F6A3C3.6010408@FreeBSD.org> In-Reply-To: <200709231625.l8NGPhaR097038@repoman.freebsd.org> References: <200709231625.l8NGPhaR097038@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Most of this diff violates KNF. Some comments are inline. On 23/09/2007 12:25, Hans Petter Selasky wrote: > http://perforce.freebsd.org/chv.cgi?CH=126745 > > Change 126745 by hselasky@hselasky_laptop001 on 2007/09/23 16:25:37 > > > - moved and renamed two functions into "usb_hid.c" from > "usb_subr.c" > > Affected files ... > > .. //depot/projects/usb/src/sys/dev/usb/usb_hid.c#4 edit > > Differences ... > > ==== //depot/projects/usb/src/sys/dev/usb/usb_hid.c#4 (text+ko) ==== > > @@ -464,3 +464,79 @@ > hid_end_parse(hd); > return (err); > } > + > +usb_hid_descriptor_t * > +hid_get_descriptor_from_usb(usb_config_descriptor_t *cd, > + usb_interface_descriptor_t *id) All tabs and adjacent spaces on the line above should be replaced with a total of four spaces. > +{ > + usb_descriptor_t *desc = (void *)id; > + > + if(desc == NULL) { > + return NULL; > + } A space after the 'if' keyword is missing. Curly brackets should be omitted above. E.g. it should have been: if (desc == NULL) return NULL; > + > + while ((desc = usbd_desc_foreach(cd, desc))) > + { > + if ((desc->bDescriptorType == UDESC_HID) && > + (desc->bLength >= USB_HID_DESCRIPTOR_SIZE(0))) > + { > + return (void *)desc; > + } > + > + if (desc->bDescriptorType == UDESC_INTERFACE) > + { > + break; > + } > + } > + return NULL; The opening curly bracket should appear on a line by itself only for the definition of a function, not for the 'while' or 'if' statements. Curly brackets in the above 'if' statements are excessive. > +} > + > +usbd_status > +hid_read_report_desc_from_usb(struct usbd_device *udev, struct mtx *mtx, > + void **descp, uint16_t *sizep, > + usb_malloc_type mem, uint8_t iface_index) > +{ Too many spaces, see one of the comments above. > + struct usbd_interface *iface = usbd_get_iface(udev, iface_index); > + usb_hid_descriptor_t *hid; > + usbd_status err; > + > + if((iface == NULL) || (iface->idesc == NULL)) > + { > + return USBD_INVAL; > + } > + > + hid = hid_get_descriptor_from_usb > + (usbd_get_config_descriptor(udev), iface->idesc); > + > + if(hid == NULL) > + { > + return USBD_IOERROR; > + } > + > + *sizep = UGETW(hid->descrs[0].wDescriptorLength); > + if (*sizep == 0) { > + return USBD_IOERROR; > + } > + > + if (mtx) mtx_unlock(mtx); > + > + *descp = malloc(*sizep, mem, M_ZERO|M_WAITOK); > + > + if (mtx) mtx_lock(mtx); > + > + if(*descp == NULL) > + { > + return USBD_NOMEM; > + } > + > + err = usbreq_get_report_descriptor > + (udev, mtx, *descp, *sizep, iface_index); > + > + if(err) > + { > + free(*descp, mem); > + *descp = NULL; > + return err; > + } > + return USBD_NORMAL_COMPLETION; > +} Some spaces are missing after the 'if' keyword, some curly brackets appear on the wrong lines, and most curly brackets should be omitted in the first place. :) Also, I'm not sure it's a good idea to have such a long function names as hid_get_descriptor_from_usb() and usbreq_get_report_descriptor(), where you are basically going outside of style(9) in the way that you are calling these functions in the above code. "if (mtx) mtx_lock(mtx);" should be splitted into two lines. In general, you might want to refer to style(9): http://www.freebsd.org/cgi/man.cgi?query=style&sektion=9 Best regards, Constantine.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?46F6A3C3.6010408>