Hi Rob,

thanks for your review!

On Wed, Feb 17, 2021 at 03:04:41PM -0600, Rob Herring wrote:

On Wed, Feb 10, 2021 at 09:10:36AM -0800, Matthias Kaehlcke wrote:

quoted

Discrete onboard USB hubs (an example for such a hub is the Realtek
RTS5411) need to be powered and may require initialization of other
resources (like GPIOs or clocks) to work properly. This adds a device
tree binding for these hubs.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---

Changes in v5:
- updated 'title'
- only use standard USB compatible strings
- deleted 'usb_hub' node
- renamed 'usb_controller' node to 'usb-controller'
- removed labels from USB nodes
- added 'vdd-supply' to USB nodes

Changes in v4:
- none

Changes in v3:
- updated commit message
- removed recursive reference to $self
- adjusted 'compatible' definition to support multiple entries
- changed USB controller phandle to be a node

Changes in v2:
- removed 'wakeup-source' and 'power-off-in-suspend' properties
- consistently use spaces for indentation in example

 .../bindings/usb/onboard_usb_hub.yaml         | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml

diff --git a/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
new file mode 100644
index 000000000000..bf4ec52e6c7b
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml

@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/onboard_usb_hub.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Binding for discrete onboard USB hubs

This isn't really generic. Maybe there's a set of hubs with only a 
single supply much like 'simple-panel', but I kind of doubt that here.
There aren't hundreds of hub chips like panels. Though, we should put 
this into bindings/usb/hub/ so we start collecting hub bindings in one 
place.

Ok, I agree that the name of the binding is too generic, I anticipated that
the power supply section would need to be extended to support other hub
chips.

A generic driver doesn't have to have a generic binding.

That's a good point, it seems to make sense to have separate bindings in
this case.

You can have a specific device binding which is handled by a generic
driver. Or not. Who knows. Maybe a simple user like u-boot has a generic
driver while something more feature rich has a device specific binding.

quoted

+
+maintainers:
+  - Matthias Kaehlcke [off-list ref]

Now we have usb-device.yaml, you need:

allOf:
  - $ref: usb-device.yaml#

ok

So with your comments addressed it seems we have a binding that could be
acceptable. I'll still hold back a bit to see if we can make progress with
the discussion about using the 'graph' binding (https://lore.kernel.org/patchwork/patch/1379002/#1578294).
The one thing I don't like about the current binding is that it wouldn't
work out of the box with a hierarchy of hubs. To make that work on the
driver side an additional property would be needed to indicate that two
(or more) USB hub devices are related (i.e. are provided by the same
chip). This is needed to be able to decide whether the hub should be
powered down during system suspend.