RE: [PATCH v11 4/7] usb:gadget: Add SuperSpeed support to the Gadget Framework

["Tanya Brokhman" <tlinder@codeaurora.org>]

Hi Sebastian

> >@@ -157,7 +167,35 @@ ep_found:
> > 	/* commit results */
> > 	_ep->maxpacket = le16_to_cpu(chosen_desc->wMaxPacketSize);
> > 	_ep->desc = chosen_desc;
> >-
> >+	_ep->comp_desc = NULL;
> >+	_ep->maxburst = 0;
> >+	_ep->mult = 0;
> >+	if (want_comp_desc) {
> 
>     if (!want_comp_desc)
>         return 0;
> 
> I have one ident level less :)
Done :)

> >+/**
> >+ * bos_desc() - prepares the BOS descriptor.
> >+ * @cdev: pointer to usb_composite device to generate the bos
> >+ *	descriptor for
> >+ *
> >+ * This function generates the BOS (Binary Device Object)
> >+ * descriptor and its device capabilities descriptors. The BOS
> >+ * descriptor should be supported by a SuperSpeed device.
> >+ */
> >+static int bos_desc(struct usb_composite_dev *cdev)
> >+{
> >+	struct usb_ext_cap_descriptor	*usb_ext;
> >+	struct usb_ss_cap_descriptor	*ss_cap;
> >+	struct usb_dcd_config_params	dcd_config_params;
> >+	struct usb_bos_descriptor	*bos = cdev->req->buf;
> >+
> >+	bos->bLength = USB_DT_BOS_SIZE;
> >+	bos->bDescriptorType = USB_DT_BOS;
> >+
> >+	bos->wTotalLength = cpu_to_le16(USB_DT_BOS_SIZE);
> >+	bos->bNumDeviceCaps = 0;
> >+
> >+	/*
> >+	 * A SuperSpeed device shall include the USB2.0 extension
> descriptor
> >+	 * and shall support LPM when operating in USB2.0 HS mode.
> >+	 */
> >+	usb_ext = (struct usb_ext_cap_descriptor *)
> cdev->req->buf is (void *) so you can skip that cast.
> 
> >+			(cdev->req->buf+bos->wTotalLength);
> a space between + please. bos->wTotalLength is le16 so you can't simply
> do that way.
> 
> What about something like
> 
>   usb_ext = (struct usb_ext_cap_descriptor *)(bos + 1)
> 
> ?

Added the spaces and the le16_to_cpu(bos->wTotalLength). 
It seems clearer to me to leave it as 
	usb_ext = cdev->req->buf + le16_to_cpu(bos->wTotalLength);
if that's ok with you.

> >@@ -499,6 +633,9 @@ static int set_config(struct usb_composite_dev
> *cdev,
> > 		case USB_SPEED_LOW:	speed = "low"; break;
> > 		case USB_SPEED_FULL:	speed = "full"; break;
> > 		case USB_SPEED_HIGH:	speed = "high"; break;
> >+		case USB_SPEED_SUPER:
> >+			speed = "super";
> >+			break;
> 
> This is not my favorite style either but please do it the way the other
> three are done.

Well here is the dilemma: if I do it the other tree were done - I get
checkpatch error. 
You're right, adding this the way it's above doesn't look too good but when
I fixed the other three I was asked not to do so in this patch, which also
makes sense since it has nothing to do with SS support...
So what do I do? Submit with a checkpatch error?


Best regards,
Tanya Brokhman
Consultant for Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum