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

[Jakub Kicinski <kuba@kernel.org>]

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.

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

Should serial.yaml's pattern list gain ethernet-pse as part of this series,
or is the intent to reuse the existing generic mcu name (which would
conflict with pse-controller.yaml's $nodename pattern)?

I checked the end of the series and serial.yaml is unchanged there, so no
counterpart update appears later either.

> +unevaluatedProperties: false
> +
> +examples:

[ ... ]

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

[ ... ]
-- 
pw-bot: cr