|
From: | Hans de Goede |
Subject: | Re: [Qemu-devel] [PATCH 10/11] usb-uhci: Add support for being a companion controller |
Date: | Wed, 29 Jun 2011 13:19:14 +0200 |
User-agent: | Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.17) Gecko/20110428 Fedora/3.1.10-1.fc15 Lightning/1.0b2 Thunderbird/3.1.10 |
Hi, On 06/29/2011 12:57 PM, Gerd Hoffmann wrote:
Hi,+ if (s->masterbus) { + USBPort *ports[NB_PORTS]; + for(i = 0; i< NB_PORTS; i++) { + s->ports[i].port.ops =&uhci_port_ops; + s->ports[i].port.opaque = s; + s->ports[i].port.index = i; + s->ports[i].port.speedmask = + USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL; + usb_port_location(&s->ports[i].port, NULL, i+1); + ports[i] =&s->ports[i].port; + } + if (usb_bus_register_companion(s->masterbus, ports, NB_PORTS, + s->firstport) != 0) { + return -1; + } + } else { + usb_bus_new(&s->bus,&uhci_bus_ops,&s->dev.qdev); + for(i = 0; i< NB_PORTS; i++) { + usb_register_port(&s->bus,&s->ports[i].port, s, i,&uhci_port_ops, + USB_SPEED_MASK_LOW | USB_SPEED_MASK_FULL); + usb_port_location(&s->ports[i].port, NULL, i+1); + }This looks like we'll want a usb_register_companion_port() function which looks like usb_register_port() but accepts masterbus & portindex instead of a USBBus pointer.
> Then register the companion ports one by one, so that the code path for the companion case looks almost identical to the non-companion case. I agree, but there is a reason why I went with a usb_bus_register_companion function instead of with a usb_bus_register_companion_port function, the uhci controller needs to know how many companion controllers it has (to report this in one of its registers). When we're registering ports 1 by 1, it does not know. We could also pass in a port owner pointer, and make the uhci code keep a list of known companions and check how many companions there are that way, but that is quite ugly. Another problem with registering ports 1 by 1 is that registering a companion port can fail, and if the 2nd or higher register fails we would need to undo the previous registers. Granted this is only an issue on hotplug, and to support hot-unplug we would also need an unregister .. Thinking more about this I think that the best approach would be to move the port setup code (setting index, ops, speedmask, etc.) to usb_bus_register_companion, and keep doing the entire registration of all the ports in one single call. Would that work for you? This still leaves the building of the port pointers array, we could pass in a stride parameter to usb_bus_register_companion and make it build that list too, I'm not sure if that is a good idea though? > Otherwise the whole patchset looks very good. I'm glad you like it :) Regards, Hans
[Prev in Thread] | Current Thread | [Next in Thread] |