From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Frysinger Subject: Re: [PATCH v13 5/8] usb:gadget: Add SuperSpeed support to the Gadget Framework Date: Tue, 24 May 2011 13:20:24 -0400 Message-ID: References: <1306231330-11158-1-git-send-email-tlinder@codeaurora.org> <1306231330-11158-6-git-send-email-tlinder@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from mail-fx0-f46.google.com ([209.85.161.46]:58096 "EHLO mail-fx0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752153Ab1EXRUp convert rfc822-to-8bit (ORCPT ); Tue, 24 May 2011 13:20:45 -0400 In-Reply-To: <1306231330-11158-6-git-send-email-tlinder@codeaurora.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Tatyana Brokhman Cc: greg@kroah.com, linux-usb@vger.kernel.org, linux-arm-msm@vger.kernel.org, balbi@ti.com, ablay@codeaurora.org, open list On Tue, May 24, 2011 at 06:02, Tatyana Brokhman wrote: > +config USB_GADGET_SUPERSPEED > + =C2=A0 =C2=A0 =C2=A0 boolean "Gadget operating in Super Speed" perhaps better phrasing would be: Enable Super Speed support maybe i didnt look too closely, but it seems like very little even depends upon this Kconfig option in the actual source. only the "gadget_is_superspeed()", and that is used lightly. so i wonder how useful this is even having ... > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 value =3D min(w_length, (u16) value= ); i know you're just following existing style, but i wonder if these all shouldnt just be min_t(u16, ...) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ERROR(cdev, "func_suspend() returne= d " > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 "error %d\n", value); please dont split string literals over multiple lines > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 default: > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 break; > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 } isnt that the default behavior already ? so these two lines are pointl= ess ? > --- a/include/linux/usb/ch9.h > +++ b/include/linux/usb/ch9.h > @@ -142,8 +142,6 @@ > =C2=A0#define USB_DEVICE_LTM_ENABLE =C2=A050 =C2=A0 =C2=A0 =C2=A0/* d= ev may send LTM */ > =C2=A0#define USB_INTRF_FUNC_SUSPEND 0 =C2=A0 =C2=A0 =C2=A0 /* functi= on suspend */ > > -#define USB_INTR_FUNC_SUSPEND_OPT_MASK 0xFF00 > - > =C2=A0#define USB_ENDPOINT_HALT =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A00 =C2=A0 =C2=A0 =C2=A0 /* IN/OUT will STALL */ > > =C2=A0/* Bit array elements as returned by the USB_REQ_GET_STATUS req= uest. */ erm, seems unrelated to this patchset ? did this sneak in by accident = ? > +static inline int gadget_is_superspeed(struct usb_gadget *g) > +{ > +#ifdef CONFIG_USB_GADGET_SUPERSPEED > + =C2=A0 =C2=A0 =C2=A0 /* runtime test would check "g->is_superspeed"= ... that might be > + =C2=A0 =C2=A0 =C2=A0 =C2=A0* useful to work around hardware bugs, b= ut is mostly pointless > + =C2=A0 =C2=A0 =C2=A0 =C2=A0*/ multiline comments should read: /* * foo ..... */ -mike