[U-Boot] [PATCH] usb: dwc2: Add driver for Synopsis DWC2 USB IP block

[Marek Vasut]

On Monday, October 20, 2014 at 09:30:42 AM, Pavel Machek wrote:
> Hi!
> 
> > This is the USB host controller used on the Altera SoCFPGA and Raspbery
> > Pi.
> > 
> > This code has three checkpatch warnings, but to make sure it stays at
> > least readable and clear, these are not fixed. These bugs are in the USB
> > request handling combinatorial logic, so any abstracting of those is out
> > of question.
> > 
> > Tested on DENX MCV (Altera SoCFPGA 5CSFXC6C6U23C8N) and RPi B+
> > (BCM2835).
> 
> The code also has ton of unused defines. You said you want them to
> document hardware, but do we really need
> 
> DWC2_HAINTMSK_CH7 _and_ DWC2_HAINTMSK_CH7_OFFSET, both unused? They
> both contain same information...

They're mask and offset, two different things in fact.

> > NOTE: Unless there are no objections, I would like to apply this.
> 
> Parse error :-).

Excuse me ?
[...]
> > +	switch (cmd->requesttype & ~USB_DIR_IN) {
> > +	case 0:
> > +		*(uint16_t *)buffer = cpu_to_le16(1);
> > +		len = 2;
> > +		break;
> > +	case USB_RECIP_INTERFACE:
> > +		*(uint16_t *)buffer = cpu_to_le16(0);
> > +		len = 2;
> > +		break;
> > +	case USB_RECIP_ENDPOINT:
> > +		*(uint16_t *)buffer = cpu_to_le16(0);
> > +		len = 2;
> > +		break;
> > +	case USB_TYPE_CLASS:
> > +		*(uint32_t *)buffer = cpu_to_le32(0);
> > +		len = 4;
> > +		break;
> 
> You can get rid of endianness conversion for zeros.

Yes, but being inconsistent about the buffer endianness conversion would
only lead to bugs in case someone started hacking on this code. So I would
really like to keep this in place for consistency's sake.

> And can use same code for USB_RECIP_INTERFACE and USB_RECIP_ENDPOINT.

Done.

> > +	switch (cmd->requesttype & ~USB_DIR_IN) {
> > +	case 0:
> > +		switch (wValue & 0xff00) {
> > +		case 0x0100:	/* device descriptor */
> > +			len = min3(txlen, sizeof(root_hub_dev_des), wLength);
> > +			memcpy(buffer, root_hub_dev_des, len);
> > +			break;
> > +		case 0x0200:	/* configuration descriptor */
> > +			len = min3(txlen, sizeof(root_hub_config_des), wLength);
> > +			memcpy(buffer, root_hub_config_des, len);
> > +			break;
> > +		case 0x0300:	/* string descriptors */
> > +			switch (wValue & 0xff) {
> > +			case 0x00:
> > +				len = min3(txlen, sizeof(root_hub_str_index0),
> > +					   wLength);
> > +				memcpy(buffer, root_hub_str_index0, len);
> > +				break;
> > +			case 0x01:
> > +				len = min3(txlen, sizeof(root_hub_str_index1),
> > +					   wLength);
> > +				memcpy(buffer, root_hub_str_index1, len);
> > +				break;
> > +			}
> > +			break;
> 
> Helper function that takes root_hub_str_index0 or similar, then does
> len and memcpy?

A helper function which would basically take parameters of the min3() and 
memcpy() as parameters? Do you consider this worth it ?

> Acked-by: Pavel Machek <pavel at denx.de>
[...]