Re: [PATCH v3 1/1] USB: core: let USB device know device node

[Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>]

On Friday 22 January 2016 14:59:01 Peter Chen wrote:
> On Thu, Jan 21, 2016 at 11:24:21PM +0100, Arnd Bergmann wrote:
> > On Thursday 21 January 2016 10:21:15 Alan Stern wrote:
> > > On Thu, 21 Jan 2016, Arnd Bergmann wrote:
> > > > On Thursday 21 January 2016 17:48:32 Peter Chen wrote:
> > 		hub@1 { /* external hub, superspeed mode class 9/subclass 0/proto 3 */
> > 			compatible = "usb2109,0812.591",
> > 				     "usb2109,0812",
> > 				     "usb2109,class9.0.3",
> > 				     "usb2109,class9.0",
> > 				     "usb2109,class9";
> > 			compatible = "usb2109,0812";
> 
> Do we really need to write "compatible" so complicated?

The binding mandates it this way, but I guess we could decide to
make it a Linux-specific extension that we allow some of them to
be left out.

> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 			reg = <1>;
> > 
> > 			communications@4 { /* superspeed ethernet device */
> > 				compatible = "usb0b95,1790.100",
> > 					     "usb0b95,1790",
> > 					     "usb0b95,class255.255.0",
> > 					     "usb0b95,class255.255",
> > 					     "usb0b95,class255",
> > 					     "usbif0b95,1790.100",
> > 					     "usbif0b95,1790",
> > 					     "usbif0b95,class255.255.0",
> > 					     "usbif0b95,class255.255,
> > 					     "usbif0b95,class255";
> > 				reg = <4>;
> > 			};
> > 
> > 			storage@1 { /* superspeed flash drive */
> > 				compatible = "usb1234,5678.600",
> > 					     "usb1234,5678",
> > 					     "usbif1234,class8.6.80",
> > 					     "usbif1234,class8.6",
> > 					     "usbif1234,class8",
> > 					     "usbif,class8.6.80",
> > 					     "usbif,class8.6",
> > 					     "usbif,class8";
> > 				reg = <1>;
> > 			};
> > 		};
> > 	
> > 		hub@3 { /* same external  hub, highspeed mode */
> > 			compatible = "usb2109,0812.591",
> > 				     "usb2109,0812",
> > 				     "usb2109,class9.0.1",
> > 				     "usb2109,class9.0",
> > 				     "usb2109,class9";
> > 
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 			reg = <3>;
> > 
> 
> Why "reg" is 3 here?

My mistake. It should be hub@1 and reg=<1>;

I accidentally confused the port number and the device number.

> > 				wireless@0,1 { /* bluetooth config 0, if 1 */
> > 					compatible = "usbif0a12,0001.134.config0.1",
> > 							"usbif0a12,0001.config0.1",
> > 							"usbif0a12,class224.1.1",
> > 							"usbif0a12,class224.1",
> > 							"usbif0a12,class224",
> > 							"usbif,class224.1.1",
> > 							"usbif,class224.1",
> > 							"usbif,class224";
> > 					reg = <0 0>;
> > 				};
> > 			};
> > 		};
> > 	};
> > 
> > In that description, I have included all four kinds of nodes from
> > the spec: host controller, device (wireless@2), interface (wireless@0.1,
> > wireless@0.2) and combined (hub@1, hub@3, storage@1, communications@4).
> > 
> > Peter's example only contained hubs in combined nodes, no device or
> > interface nodes. I wonder if the code is able to parse all four
> > kinds of nodes though, and if we actually need that.
> > 
> 
> My proposal patch only handles the node under the USB device, not include
> the USB interfaces under such device, we can add it after finalize
> how to describe it at device tree.

We should at least handle the case of multiple hubs connected to one
another I think. No need to limit it to directly connected devices
when it's easy enough to do hubs as well.

> > Do we have a 'struct device' for each interface?
> > 
> 
> Yes, we have, but there are different 'struct device' between USB device
> and USB interfaces under this USB device. See usb_set_configuration,
> drivers/usb/core/message.c

Ok, got it. So we can set the of_node pointer of a struct usb_interface
to the child node of the usb->interface->usb_dev that matches the
configuration/interface tuple.

For combined device nodes (class 0 device, single configuration, single
interface), I suppose we have both a 'struct usb_device' and a 'struct
usb_interface' that we could attach the of_node to, but we can
decide to always just use one of the two, to avoid having the
same of_node pointer in multiple 'struct device' instances.

Or we simplify it so we always put the of_node just in the usb_device
or the usb_interface, even if both are listed.

> > Is it possible to have a hub in an interface of a multifunction device
> > or are they always single-configuration single-interface devices?
> > 
> 
> I have not seen such kinds of devices, but it is possible in theory.

Ok, so if the USB spec allows it, we should probably try to handle it too.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html