On 30.01.24 18:26, Matthias Kaehlcke wrote: > On Tue, Jan 30, 2024 at 08:19:40AM -0800, Greg Kroah-Hartman wrote: >> On Tue, Jan 30, 2024 at 04:11:29PM +0000, Matthias Kaehlcke wrote: >>> Hi Javier, >>> >>> I understand your motivation for using the onboard_usb_hub driver >>> for powering up a non-hub device, it feels a bit hacky to use it >>> as is though. Re-using the driver might be the right thing to do, >>> but then it should probably be renamed to onboard_usb_dev (or >>> similar) and do the hub specific bits as special case. >>> >>> Greg, do you have any thoughts on this? >> >> Yeah, this worries me, adding non-hub support to this driver feels odd. > > It is odd as long as this driver claims to be hub-specific, but truth > is that the hub-specific bits are a small part of the driver, I think > it might be worthwhile to consider adapting the driver to other devices > if there is no clear better solution. The driver was programmed for hubs from the beginning, and that makes non-hub additions look weird, but I wonder if the other way round would have been seen less odd: a generic onboard_usb_dev that ended up being extended to support hub-specific capabilities. As Matthias pointed out, most of the driver is generic, but I have to admit that adding a non-hub device directly (without any renaming) does not look 100% right. > A possible alternative could be a separate onboard_usb_dev driver for > non-hub devices, with a similar structure as the onboard_hub driver, > but without the hub-specific bits. > I was thinking about that possibility too, but the new driver would be a renamed onboard_usb_hub with less functionality. Its only added value would be that it keeps onboard_usb_hub only for hubs. But if that is exactly the goal, I have nothing against an onboard_usb_dev driver for the next patch version. Adding a device-specific driver for such a generic power sequence and resume/suspend support might not be the best approach, though, especially if more USB devices with similar needs arise. >> Why can't this all just be done in an individual driver for this device >> itself? > > I suppose the reason is the good old chicken-egg situation that the (USB) > driver is only instantiated after the device has bee powered on, which is > what the driver is supposed to take care of. For the onboard_hub driver > this was solved by having a platform driver that is instantiated by the > parent hub if the onboard hub has a device tree entry. Probably something > similar would be needed for an individual driver, and the generic hub > driver would have to call the equivalent of onboard_hub_create_pdevs() > for all drivers of this type (or a wrapper that does this). > > m. The XVF3500 does not have kernel support so far, and once it reaches normal operation and registers itself as a USB device, the communication is achieved in userspace. What this device needs from a driver is only the power sequence and eventually the resume/suspend support, which makes it a good candidate for generic implementations. Best regards, Javier Carrasco
