PERFORCE change 126745 for review
Constantine A. Murenin
cnst at FreeBSD.org
Sun Sep 23 11:21:50 PDT 2007
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 at 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.
More information about the p4-projects
mailing list