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

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.

[ ... ]

+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