From owner-p4-projects@FreeBSD.ORG Sun Sep 23 18:21:50 2007 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 0BB3816A421; Sun, 23 Sep 2007 18:21:50 +0000 (UTC) Delivered-To: perforce@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id BBDBE16A418; Sun, 23 Sep 2007 18:21:49 +0000 (UTC) (envelope-from cnst@FreeBSD.org) Received: from mojo.ru (mojo.ru [84.252.152.63]) by mx1.freebsd.org (Postfix) with ESMTP id 083C713C4B9; Sun, 23 Sep 2007 18:21:48 +0000 (UTC) (envelope-from cnst@FreeBSD.org) Received: from [192.168.0.16] (nc-76-4-28-21.dhcp.embarqhsd.net [76.4.28.21]) (authenticated bits=0) by mojo.ru (8.12.11.20060308/8.12.10) with ESMTP id l8NHZMl9014246 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 23 Sep 2007 21:35:26 +0400 Message-ID: <46F6A3C3.6010408@FreeBSD.org> Date: Sun, 23 Sep 2007 13:34:59 -0400 From: "Constantine A. Murenin" Organization: Google Summer of Code 2007 Student @ The FreeBSD Project User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.7.5) Gecko/20041217 X-Accept-Language: en-gb, en-gb-oed, en, en-us, ru, ru-ru, ru-su MIME-Version: 1.0 To: Hans Petter Selasky References: <200709231625.l8NGPhaR097038@repoman.freebsd.org> In-Reply-To: <200709231625.l8NGPhaR097038@repoman.freebsd.org> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Cc: Perforce Change Reviews , "Constantine A. Murenin" Subject: Re: PERFORCE change 126745 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 23 Sep 2007 18:21:50 -0000 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.