Re: [PATCH net-next v11 1/4] dt-bindings: net: pse-pd: add bindings for Realtek PSE MCU

[Jonas Jelonek]

On 06.08.26 03:11, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> dt-bindings: net: pse-pd: add bindings for Realtek PSE MCU
>
> This adds a new YAML binding for the MCU that fronts the PSE silicon on a
> range of managed Realtek-based switches, covering both protocol generations
> and the I2C/SMBus and UART attachments.  Two questions below about the file
> naming and about the serial slave node name.
>
>> diff --git a/Documentation/devicetree/bindings/net/pse-pd/realtek,pse-mcu-gen1.yaml b/Documentation/devicetree/bindings/net/pse-pd/realtek,pse-mcu-gen1.yaml
>> new file mode 100644
>> index 0000000000000..3bb32349c28c4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/pse-pd/realtek,pse-mcu-gen1.yaml
>> @@ -0,0 +1,182 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/net/pse-pd/realtek,pse-mcu-gen1.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> [ ... ]
>
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      # UART
>> +      - items:
>> +          - enum:
>> +              - zyxel,gs1900-10hp-a1-pse
>> +          - const: realtek,pse-mcu-gen1
>> +
>> +      # I2C, SMBus framing
>> +      - items:
>> +          - enum:
>> +              - zyxel,gs1920-24hp-v2-pse
>> +          - const: realtek,pse-mcu-gen1-smbus
>> +
>> +      # UART
>> +      - items:
>> +          - enum:
>> +              - zyxel,gs1900-10hp-b1-pse
>> +              - zyxel,xmg1915-10ep-pse
>> +          - const: realtek,pse-mcu-gen2
>> +
>> +      # I2C, SMBus framing
>> +      - items:
>> +          - enum:
>> +              - zyxel,xs1930-12hp-pse
>> +          - const: realtek,pse-mcu-gen2-smbus
>> +
>> +      # I2C, raw framing
>> +      - items:
>> +          - enum:
>> +              - linksys,lgs328mpc-v2-pse
>> +          - const: realtek,pse-mcu-gen2-i2c
> [Severity: Low]
> This isn't a bug, but should the file be named after the family rather than
> after one compatible?
>
> The file (and $id) is realtek,pse-mcu-gen1.yaml, while the compatible list
> here documents five strings across two protocol generations:
>
>     realtek,pse-mcu-gen1
>     realtek,pse-mcu-gen1-smbus
>     realtek,pse-mcu-gen2
>     realtek,pse-mcu-gen2-smbus
>     realtek,pse-mcu-gen2-i2c
>
> There is no file bearing the name of any realtek,pse-mcu-gen2* compatible,
> so searching by that compatible name finds nothing, and the commit message
> does not mention that gen2 is described in a ...-gen1.yaml file.
>
> Would realtek,pse-mcu.yaml (with the $id updated to match) describe the
> actual scope better?
>
> To be clear, the $id path itself is consistent with the current file path
> (net/pse-pd/realtek,pse-mcu-gen1.yaml#), so $ref resolution and
> dt_binding_check are not affected here - this is only about the declared
> scope of the name.

Could someone please take a decision on this and tell the bot how it
should be? This is being raised again and again... Conor explicitly said
the filename should carry one of the compatibles.


>> +required:
>> +  - compatible
>> +
>> +allOf:
>> +  - $ref: pse-controller.yaml#
> [ ... ]
>
>> +    else:
>> +      allOf:
>> +        - $ref: /schemas/serial/serial-peripheral-props.yaml#
>> +
>> +      properties:
>> +        reg: false
> [Severity: Low]
> This else branch declares the bare gen compatibles to be UART (serial
> slave) attachments, and the node name is forced to ethernet-pse by
> pse-controller.yaml:
>
> Documentation/devicetree/bindings/net/pse-pd/pse-controller.yaml
>   $nodename:
>     pattern: "^ethernet-pse(@.*|-([0-9]|[1-9][0-9]+))?$"
>
> But serial.yaml only enumerates these serial slave child node names:
>
> Documentation/devicetree/bindings/serial/serial.yaml
> patternProperties:
>   "^(bluetooth|bluetooth-gnss|embedded-controller|gnss|gps|mcu|onewire)$":
>     if:
>       type: object
>     then:
>       additionalProperties: true
>       $ref: serial-peripheral-props.yaml#
>       ...
>       required:
>         - compatible
>
> Does a board device tree with an ethernet-pse child of serial then get the
> shared slave-node subschema applied at all?  Since serial.yaml ends with
> additionalProperties: true, the unmatched child looks like it just falls
> through silently rather than being flagged.

It gets applied since it's referenced in my bindings. It just doesn't use the
"routing" present in serial.yaml.

> Should serial.yaml's pattern list gain ethernet-pse as part of this series,

I would avoid this since it adds no value in what is validated. From what
I've seen the pattern in serial.yaml isn't exhaustive so other bindings
like "qca,qca7000" also use a node name not being in that list.

> or is the intent to reuse the existing generic mcu name (which would
> conflict with pse-controller.yaml's $nodename pattern)?

No, reason already given in ( ).

> [ ... ]
>
>> +  # UART attachment
>> +  - |
>> +    serial {
>> +        ethernet-pse {
>> +            compatible = "zyxel,gs1900-10hp-a1-pse", "realtek,pse-mcu-gen1";
>> +            current-speed = <19200>;
> [ ... ]


Regards,
Jonas