[PATCH 0/2] usb: misc: onboard_usb_hub: add support for XMOS XVF3500

[PATCH 0/2] usb: misc: onboard_usb_hub: add support for XMOS XVF3500

[Javier Carrasco]

This series adds support for the XMOS XVF3500 VocalFusion Voice
Processor[1], a low-latency, 32-bit multicore controller for voice
processing.

The XVF3500 requires a specific power sequence, which consists of
enabling the regulators that control the 3V3 and 1V0 device supplies,
and a reset de-assertion after a delay of at least 100ns. Once in normal
operation, the XVF3500 registers itself as a regular USB device and no
device-specific management is required.

During a previous attempt to add a specific driver for this device, its
addition to the existing onboard_hub driver was suggested as (possibly)
the simplest solution[2].

The power management provided by onboard_hub is not specific for hubs
and any other USB device with the same power sequence could profit from
that driver, provided that the device does not have any specific
requirements beyond the power management.

The device binding has been added to sound/ because it is the subsystem
that covers its functionality (voice processing) during normal
operation. If it should reside under usb/ instead, it will be moved as
required.

This series has been tested with a Rockchip-based SoC and an XMOS
XVF3500-FB167-C.

[1] https://www.xmos.com/xvf3500/
[2] https://lore.kernel.org/all/aeeb0dfb-87e2-4024-9d4a-0b9529477315@linaro.org/

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
Javier Carrasco (2):
      ASoC: dt-bindings: xmos,xvf3500: add XMOS XVF3500 voice processor
      usb: misc: onboard_hub: add support for XMOS XVF3500

 .../devicetree/bindings/sound/xmos,xvf3500.yaml    | 51 ++++++++++++++++++++++
 drivers/usb/misc/onboard_usb_hub.c                 |  2 +
 drivers/usb/misc/onboard_usb_hub.h                 |  6 +++
 3 files changed, 59 insertions(+)
---
base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3
change-id: 20240130-onboard_xvf3500-6c0e78d11a1b

Best regards,
-- 
Javier Carrasco <javier.carrasco@wolfvision.net>

[PATCH 1/2] ASoC: dt-bindings: xmos,xvf3500: add XMOS XVF3500 voice processor

[Javier Carrasco]

The XMOS XVF3500 VocalFusion Voice Processor[1] is a low-latency, 32-bit
multicore controller for voice processing.

Add new bindings to define the device properties.

[1] https://www.xmos.com/xvf3500/

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
 .../devicetree/bindings/sound/xmos,xvf3500.yaml    | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/xmos,xvf3500.yaml b/Documentation/devicetree/bindings/sound/xmos,xvf3500.yaml
new file mode 100644
index 000000000000..d7d5bda23b1b
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/xmos,xvf3500.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/xmos,xvf3500.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: XMOS XVF3500 VocalFusion Voice Processor
+
+maintainers:
+  - Javier Carrasco <javier.carrasco@wolfvision.net>
+
+description:
+  The XMOS XVF3500 VocalFusion Voice Processor is a low-latency, 32-bit
+  multicore controller for voice processing.
+  https://www.xmos.com/xvf3500/
+
+properties:
+  compatible:
+    const: usb20b1,0013
+
+  reset-gpios:
+    maxItems: 1
+
+  vdd-supply:
+    description:
+      Regulator for the 1V0 supply.
+
+  vdd2-supply:
+    description:
+      Regulator for the 3V3 supply.
+
+required:
+  - compatible
+  - reset-gpios
+  - vdd-supply
+  - vdd2-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    xvf3500 {
+      compatible = "usb20b1,0013";
+      reset-gpios = <&gpio 5 GPIO_ACTIVE_LOW>;
+      vdd-supply = <&vcc1v0>;
+      vdd2-supply = <&vcc3v3>;
+    };
+
+...

-- 
2.39.2

Re: [PATCH 1/2] ASoC: dt-bindings: xmos,xvf3500: add XMOS XVF3500 voice processor

[Krzysztof Kozlowski]

On 30/01/2024 13:26, Javier Carrasco wrote:
> The XMOS XVF3500 VocalFusion Voice Processor[1] is a low-latency, 32-bit
> multicore controller for voice processing.
> 
> Add new bindings to define the device properties.
> 
> [1] https://www.xmos.com/xvf3500/
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---

This should be v2. Also, please provide changelog either here or in
cover letter.

>  .../devicetree/bindings/sound/xmos,xvf3500.yaml    | 51 ++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/xmos,xvf3500.yaml b/Documentation/devicetree/bindings/sound/xmos,xvf3500.yaml
> new file mode 100644
> index 000000000000..d7d5bda23b1b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/xmos,xvf3500.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/xmos,xvf3500.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: XMOS XVF3500 VocalFusion Voice Processor
> +
> +maintainers:
> +  - Javier Carrasco <javier.carrasco@wolfvision.net>
> +
> +description:
> +  The XMOS XVF3500 VocalFusion Voice Processor is a low-latency, 32-bit
> +  multicore controller for voice processing.
> +  https://www.xmos.com/xvf3500/
> +
> +properties:
> +  compatible:
> +    const: usb20b1,0013
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  vdd-supply:
> +    description:
> +      Regulator for the 1V0 supply.
> +
> +  vdd2-supply:
> +    description:
> +      Regulator for the 3V3 supply.
> +
> +required:
> +  - compatible
> +  - reset-gpios
> +  - vdd-supply
> +  - vdd2-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    xvf3500 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Previous version had different code here, so I don't understand why this
became model number.

Best regards,
Krzysztof

Re: [PATCH 1/2] ASoC: dt-bindings: xmos,xvf3500: add XMOS XVF3500 voice processor

[Javier Carrasco]

On 30.01.24 13:34, Krzysztof Kozlowski wrote:
> On 30/01/2024 13:26, Javier Carrasco wrote:
>> The XMOS XVF3500 VocalFusion Voice Processor[1] is a low-latency, 32-bit
>> multicore controller for voice processing.
>>
>> Add new bindings to define the device properties.
>>
>> [1] https://www.xmos.com/xvf3500/
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
>> ---
> 
> This should be v2. Also, please provide changelog either here or in
> cover letter.
> 
That would also have saved me from giving a reference to a previous
discussion. In that case I will use v3 for the next version and the
previous series for the sound subsystem will be mentioned in the
changelog as v1.
>>  .../devicetree/bindings/sound/xmos,xvf3500.yaml    | 51 ++++++++++++++++++++++
>>  1 file changed, 51 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/sound/xmos,xvf3500.yaml b/Documentation/devicetree/bindings/sound/xmos,xvf3500.yaml
>> new file mode 100644
>> index 000000000000..d7d5bda23b1b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sound/xmos,xvf3500.yaml
>> @@ -0,0 +1,51 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/sound/xmos,xvf3500.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: XMOS XVF3500 VocalFusion Voice Processor
>> +
>> +maintainers:
>> +  - Javier Carrasco <javier.carrasco@wolfvision.net>
>> +
>> +description:
>> +  The XMOS XVF3500 VocalFusion Voice Processor is a low-latency, 32-bit
>> +  multicore controller for voice processing.
>> +  https://www.xmos.com/xvf3500/
>> +
>> +properties:
>> +  compatible:
>> +    const: usb20b1,0013
>> +
>> +  reset-gpios:
>> +    maxItems: 1
>> +
>> +  vdd-supply:
>> +    description:
>> +      Regulator for the 1V0 supply.
>> +
>> +  vdd2-supply:
>> +    description:
>> +      Regulator for the 3V3 supply.
>> +
>> +required:
>> +  - compatible
>> +  - reset-gpios
>> +  - vdd-supply
>> +  - vdd2-supply
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +
>> +    xvf3500 {
> 
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 
> Previous version had different code here, so I don't understand why this
> became model number.
> 
Thanks for the reference, I will change it to the previous "voice-processor"
> Best regards,
> Krzysztof
> 

Best regards,
Javier Carrasco

Re: [PATCH 1/2] ASoC: dt-bindings: xmos,xvf3500: add XMOS XVF3500 voice processor

[Rob Herring]

On Tue, Jan 30, 2024 at 01:26:56PM +0100, Javier Carrasco wrote:
> The XMOS XVF3500 VocalFusion Voice Processor[1] is a low-latency, 32-bit
> multicore controller for voice processing.
> 
> Add new bindings to define the device properties.
> 
> [1] https://www.xmos.com/xvf3500/
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
>  .../devicetree/bindings/sound/xmos,xvf3500.yaml    | 51 ++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/xmos,xvf3500.yaml b/Documentation/devicetree/bindings/sound/xmos,xvf3500.yaml
> new file mode 100644
> index 000000000000..d7d5bda23b1b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/xmos,xvf3500.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/xmos,xvf3500.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: XMOS XVF3500 VocalFusion Voice Processor
> +
> +maintainers:
> +  - Javier Carrasco <javier.carrasco@wolfvision.net>
> +
> +description:
> +  The XMOS XVF3500 VocalFusion Voice Processor is a low-latency, 32-bit
> +  multicore controller for voice processing.
> +  https://www.xmos.com/xvf3500/
> +
> +properties:
> +  compatible:
> +    const: usb20b1,0013
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  vdd-supply:
> +    description:
> +      Regulator for the 1V0 supply.
> +
> +  vdd2-supply:
> +    description:
> +      Regulator for the 3V3 supply.
> +
> +required:
> +  - compatible
> +  - reset-gpios
> +  - vdd-supply
> +  - vdd2-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    xvf3500 {
> +      compatible = "usb20b1,0013";
> +      reset-gpios = <&gpio 5 GPIO_ACTIVE_LOW>;
> +      vdd-supply = <&vcc1v0>;
> +      vdd2-supply = <&vcc3v3>;
> +    };

A USB device should hang off a USB hub (or root hub). Looks like you 
just have a random node here. You should also have a $ref to 
usb-device.yaml. You'll see you need a reg property for example.

Rob

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

[Javier Carrasco]

The XMOS XVF3500 VocalFusion Voice Processor[1] is a low-latency, 32-bit
multicore controller for voice processing.

This device requires a specific power sequence, which consists of
enabling the regulators that control the 3V3 and 1V0 device supplies,
and a reset de-assertion after a delay of at least 100ns. Such power
sequence is already supported by the onboard_hub driver, and it can be
reused for non-hub USB devices as well.

Once in normal operation, the XVF3500 registers itself as a USB device,
and it does not require any device-specific operations in the driver.

[1] https://www.xmos.com/xvf3500/

Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
 drivers/usb/misc/onboard_usb_hub.c | 2 ++
 drivers/usb/misc/onboard_usb_hub.h | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
index 0dd2b032c90b..f16ea3053f84 100644
--- a/drivers/usb/misc/onboard_usb_hub.c
+++ b/drivers/usb/misc/onboard_usb_hub.c
@@ -366,6 +366,7 @@ static struct platform_driver onboard_hub_driver = {
 #define VENDOR_ID_REALTEK	0x0bda
 #define VENDOR_ID_TI		0x0451
 #define VENDOR_ID_VIA		0x2109
+#define VENDOR_ID_XMOS		0x20B1
 
 /*
  * Returns the onboard_hub platform device that is associated with the USB
@@ -458,6 +459,7 @@ static const struct usb_device_id onboard_hub_id_table[] = {
 	{ USB_DEVICE(VENDOR_ID_TI, 0x8142) }, /* TI USB8041 2.0 */
 	{ USB_DEVICE(VENDOR_ID_VIA, 0x0817) }, /* VIA VL817 3.1 */
 	{ USB_DEVICE(VENDOR_ID_VIA, 0x2817) }, /* VIA VL817 2.0 */
+	{ USB_DEVICE(VENDOR_ID_XMOS, 0x0013) }, /* XMOS XVF3500 */
 	{}
 };
 MODULE_DEVICE_TABLE(usb, onboard_hub_id_table);
diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h
index f360d5cf8d8a..1809c0923b98 100644
--- a/drivers/usb/misc/onboard_usb_hub.h
+++ b/drivers/usb/misc/onboard_usb_hub.h
@@ -56,6 +56,11 @@ static const struct onboard_hub_pdata vialab_vl817_data = {
 	.num_supplies = 1,
 };
 
+static const struct onboard_hub_pdata xmos_xvf3500_data = {
+	.reset_us = 1,
+	.num_supplies = 2,
+};
+
 static const struct of_device_id onboard_hub_match[] = {
 	{ .compatible = "usb424,2412", .data = &microchip_usb424_data, },
 	{ .compatible = "usb424,2514", .data = &microchip_usb424_data, },
@@ -77,6 +82,7 @@ static const struct of_device_id onboard_hub_match[] = {
 	{ .compatible = "usbbda,5414", .data = &realtek_rts5411_data, },
 	{ .compatible = "usb2109,817", .data = &vialab_vl817_data, },
 	{ .compatible = "usb2109,2817", .data = &vialab_vl817_data, },
+	{ .compatible = "usb20b1,0013", .data = &xmos_xvf3500_data, },
 	{}
 };
 

-- 
2.39.2

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

[Matthias Kaehlcke]

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?

Also there is an implication that might not matter for the
XVF3500, but could for other non-hub devices with wakeup support:
by default the driver powers the hub/device down during suspend,
unless (in case of a hub) wakeup capable devices are connected.
This behavior can be changed through sysfs, but the default
would still be unexpected. In hindsight I think the default
should have been to keep the hub powered. Not sure if it's an
option to change the default at this point, since folks might
rely on it (I know systems that do, but those could be adapted).
We could possibly change the behavior for non-hub devices and
keep the device powered if wakeup is supported and enabled

m.

On Tue, Jan 30, 2024 at 01:26:57PM +0100, Javier Carrasco wrote:
> The XMOS XVF3500 VocalFusion Voice Processor[1] is a low-latency, 32-bit
> multicore controller for voice processing.
> 
> This device requires a specific power sequence, which consists of
> enabling the regulators that control the 3V3 and 1V0 device supplies,
> and a reset de-assertion after a delay of at least 100ns. Such power
> sequence is already supported by the onboard_hub driver, and it can be
> reused for non-hub USB devices as well.
> 
> Once in normal operation, the XVF3500 registers itself as a USB device,
> and it does not require any device-specific operations in the driver.
> 
> [1] https://www.xmos.com/xvf3500/
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
> ---
>  drivers/usb/misc/onboard_usb_hub.c | 2 ++
>  drivers/usb/misc/onboard_usb_hub.h | 6 ++++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
> index 0dd2b032c90b..f16ea3053f84 100644
> --- a/drivers/usb/misc/onboard_usb_hub.c
> +++ b/drivers/usb/misc/onboard_usb_hub.c
> @@ -366,6 +366,7 @@ static struct platform_driver onboard_hub_driver = {
>  #define VENDOR_ID_REALTEK	0x0bda
>  #define VENDOR_ID_TI		0x0451
>  #define VENDOR_ID_VIA		0x2109
> +#define VENDOR_ID_XMOS		0x20B1
>  
>  /*
>   * Returns the onboard_hub platform device that is associated with the USB
> @@ -458,6 +459,7 @@ static const struct usb_device_id onboard_hub_id_table[] = {
>  	{ USB_DEVICE(VENDOR_ID_TI, 0x8142) }, /* TI USB8041 2.0 */
>  	{ USB_DEVICE(VENDOR_ID_VIA, 0x0817) }, /* VIA VL817 3.1 */
>  	{ USB_DEVICE(VENDOR_ID_VIA, 0x2817) }, /* VIA VL817 2.0 */
> +	{ USB_DEVICE(VENDOR_ID_XMOS, 0x0013) }, /* XMOS XVF3500 */
>  	{}
>  };
>  MODULE_DEVICE_TABLE(usb, onboard_hub_id_table);
> diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h
> index f360d5cf8d8a..1809c0923b98 100644
> --- a/drivers/usb/misc/onboard_usb_hub.h
> +++ b/drivers/usb/misc/onboard_usb_hub.h
> @@ -56,6 +56,11 @@ static const struct onboard_hub_pdata vialab_vl817_data = {
>  	.num_supplies = 1,
>  };
>  
> +static const struct onboard_hub_pdata xmos_xvf3500_data = {
> +	.reset_us = 1,
> +	.num_supplies = 2,
> +};
> +
>  static const struct of_device_id onboard_hub_match[] = {
>  	{ .compatible = "usb424,2412", .data = &microchip_usb424_data, },
>  	{ .compatible = "usb424,2514", .data = &microchip_usb424_data, },
> @@ -77,6 +82,7 @@ static const struct of_device_id onboard_hub_match[] = {
>  	{ .compatible = "usbbda,5414", .data = &realtek_rts5411_data, },
>  	{ .compatible = "usb2109,817", .data = &vialab_vl817_data, },
>  	{ .compatible = "usb2109,2817", .data = &vialab_vl817_data, },
> +	{ .compatible = "usb20b1,0013", .data = &xmos_xvf3500_data, },
>  	{}
>  };
>  
> 
> -- 
> 2.39.2
> 

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

[Greg Kroah-Hartman]

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.
Why can't this all just be done in an individual driver for this device
itself?

Javier, what kernel driver supports this device to communicate with it?
Having 2 different drivers poke the same device is just going to cause
problems in the end, as there is no way to communicate between the two,
right?

thanks,

greg k-h

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

[Matthias Kaehlcke]

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.

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.

> 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.

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