Re: [PATCH 2/2] usb: misc: onboard_hub: add support for XMOS XVF3500

[Javier Carrasco]

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