Re: [PATCH 2/5] dt-bindings: usb: cypress,hx3: Add support for all variants

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Conor, Krzysztof, Rob,

On 4/9/25 11:27 PM, Conor Dooley wrote:
> On Wed, Apr 09, 2025 at 06:26:43PM +0200, Quentin Schulz wrote:
>> Hi Conor,
>>
>> On 4/9/25 6:18 PM, Conor Dooley wrote:
>>> On Tue, Apr 08, 2025 at 06:36:04PM +0200, Łukasz Czechowski wrote:
>>>> Hello,
>>>>
>>>> śr., 26 mar 2025 o 18:58 Conor Dooley <conor@kernel.org> napisał(a):
>>>>>
>>>>> On Wed, Mar 26, 2025 at 05:22:57PM +0100, Lukasz Czechowski wrote:
>>>>>> The Cypress HX3 hubs use different default PID value depending
>>>>>> on the variant. Update compatibles list.
>>>>>>
>>>>>> Fixes: 1eca51f58a10 ("dt-bindings: usb: Add binding for Cypress HX3 USB 3.0 family")
>>>>>> Cc: stable@vger.kernel.org # 6.6
>>>>>> Cc: stable@vger.kernel.org # Backport of the patch in this series fixing product ID in onboard_dev_id_table and onboard_dev_match in drivers/usb/misc/onboard_usb_dev.{c,h} driver
>>>>>> Signed-off-by: Lukasz Czechowski <lukasz.czechowski@thaumatec.com>
>>>>>> ---
>>>>>>    Documentation/devicetree/bindings/usb/cypress,hx3.yaml | 6 ++++++
>>>>>>    1 file changed, 6 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/usb/cypress,hx3.yaml b/Documentation/devicetree/bindings/usb/cypress,hx3.yaml
>>>>>> index 1033b7a4b8f9..f0b93002bd02 100644
>>>>>> --- a/Documentation/devicetree/bindings/usb/cypress,hx3.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/usb/cypress,hx3.yaml
>>>>>> @@ -15,8 +15,14 @@ allOf:
>>>>>>    properties:
>>>>>>      compatible:
>>>>>>        enum:
>>>>>> +      - usb4b4,6500
>>>>>> +      - usb4b4,6502
>>>>>> +      - usb4b4,6503
>>>>>>          - usb4b4,6504
>>>>>>          - usb4b4,6506
>>>>>> +      - usb4b4,6507
>>>>>> +      - usb4b4,6508
>>>>>> +      - usb4b4,650a
>>>>>
>>>>> All these devices seem to have the same match data, why is a fallback
>>>>> not suitable?
>>>>>
>>>>
>>>> Thank you for the suggestion. Indeed the fallback compatible appears
>>>> to work fine in this case,
>>>> and I am able to avoid additional entries in onboard_dev_match table
>>>> as added in the first patch in this series.
>>>>
>>>> However, after I've updated the cypress,hx3.yaml schema file and
>>>> rk3399-puma.dtsi file,
>>>> I get the following warnings, when running "make dtbs_check":
>>>>
>>>> linux/arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dtb: hub@1:
>>>> compatible: ['usb4b4,6502', 'usb4b4,6506'] is too long
>>>>   from schema $id: http://devicetree.org/schemas/usb/cypress,hx3.yaml#
>>>> linux/arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dtb: hub@2:
>>>> compatible: ['usb4b4,6500', 'usb4b4,6504'] is too long
>>>>   from schema $id: http://devicetree.org/schemas/usb/cypress,hx3.yaml#
>>>>
>>>> Below is the diff of my changes:
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/cypress,hx3.yaml
>>>> b/Documentation/devicetree/bindings/usb/cypress,hx3.yaml
>>>> index f0b93002bd02..d6eac1213228 100644
>>>> --- a/Documentation/devicetree/bindings/usb/cypress,hx3.yaml
>>>> +++ b/Documentation/devicetree/bindings/usb/cypress,hx3.yaml
>>>> @@ -14,15 +14,22 @@ allOf:
>>>>
>>>>    properties:
>>>>      compatible:
>>>> -    enum:
>>>> -      - usb4b4,6500
>>>> -      - usb4b4,6502
>>>> -      - usb4b4,6503
>>>> -      - usb4b4,6504
>>>> -      - usb4b4,6506
>>>> -      - usb4b4,6507
>>>> -      - usb4b4,6508
>>>> -      - usb4b4,650a
>>>> +    oneOf:
>>>> +      - enum:
>>>> +          - usb4b4,6504
>>>> +          - usb4b4,6506
>>>> +      - items:
>>>> +          - enum:
>>>> +              - usb4b4,6500
>>>> +              - usb4b4,6508
>>>> +          - const: usb4b4,6504
>>>> +      - items:
>>>> +          - enum:
>>>> +              - usb4b4,6502
>>>> +              - usb4b4,6503
>>>> +              - usb4b4,6507
>>>> +              - usb4b4,650a
>>>> +          - const: usb4b4,6506
>>>>
>>>>      reg: true
>>>>
>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
>>>> b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
>>>> index d0d867374b3f..7fac61f95fc6 100644
>>>> --- a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
>>>> @@ -594,14 +594,14 @@ &usbdrd_dwc3_1 {
>>>>           #size-cells = <0>;
>>>>
>>>>           hub_2_0: hub@1 {
>>>> -               compatible = "usb4b4,6502";
>>>> +               compatible = "usb4b4,6502", "usb4b4,6506";
>>>>                   reg = <1>;
>>>>                   peer-hub = <&hub_3_0>;
>>>>                   reset-gpios = <&gpio4 RK_PA3 GPIO_ACTIVE_HIGH>;
>>>>           };
>>>>
>>>>           hub_3_0: hub@2 {
>>>> -               compatible = "usb4b4,6500";
>>>> +               compatible = "usb4b4,6500", "usb4b4,6504";
>>>>                   reg = <2>;
>>>>                   peer-hub = <&hub_2_0>;
>>>>                   reset-gpios = <&gpio4 RK_PA3 GPIO_ACTIVE_HIGH>;
>>>>
>>>>
>>>> Do you have any suggestions on how I can properly update the schema
>>>> files to avoid the above warnings?
>>>
>>> The diffs you have here look okay, not really sure what you're asking
>>> for.
>>
>> It fails dtbs_check:
>>
>> linux/arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dtb: hub@1:
>> compatible: ['usb4b4,6502', 'usb4b4,6506'] is too long
>>   from schema $id: http://devicetree.org/schemas/usb/cypress,hx3.yaml#
>> linux/arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dtb: hub@2:
>> compatible: ['usb4b4,6500', 'usb4b4,6504'] is too long
>>   from schema $id: http://devicetree.org/schemas/usb/cypress,hx3.yaml#
>>
>> I'm not sure we're allowed to add new errors with dtbs_check (and would like
>> to avoid it in any case).
> 
> Ah, the restriction I think comes from the usb-device binding. Maybe
> just leave it as you had it?

Thanks to some previous debugging done by Lukasz, I believe there's 
something we can do to make everybody happy.

The issue is that the usb-device binding enforces a single pattern 
string in the compatible property[1] but that isn't what usually is 
expected by the dt-core[2].

I would suggest to patch the usb-device binding so that it can accept a 
list of strings matching the given pattern.

This is the diff I'm suggesting:

"""
diff --git a/Documentation/devicetree/bindings/usb/usb-device.yaml 
b/Documentation/devicetree/bindings/usb/usb-device.yaml
index c676956810331..42c2e8d6dc06d 100644
--- a/Documentation/devicetree/bindings/usb/usb-device.yaml
+++ b/Documentation/devicetree/bindings/usb/usb-device.yaml
@@ -28,7 +28,9 @@ description: |

  properties:
    compatible:
-    pattern: "^usb[0-9a-f]{1,4},[0-9a-f]{1,4}$"
+    items:
+      pattern: "^usb[0-9a-f]{1,4},[0-9a-f]{1,4}$"
+
      description: Device nodes or combined nodes.
        "usbVID,PID", where VID is the vendor id and PID the product id.
        The textual representation of VID and PID shall be in lower case
"""

I believe this should be a reasonable change except if we really are 
only expecting the real VID PID in the compatible.

What do you think?

Cheers,
Quentin

[1] 
https://elixir.bootlin.com/linux/v6.13.7/source/Documentation/devicetree/bindings/usb/usb-device.yaml#L31
[2] 
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/dt-core.yaml#L21-L25