[PATCH v3 1/2] dt-bindings: phy: add realtek,rtl8380m-serdes

[Krzysztof Kozlowski]

On Sat, Oct 12, 2024 at 09:48:33AM -0400, Markus Stockhausen wrote:
> Add bindings for the SerDes of the Realtek Otto platform. These are
> MIPS based network Switch SoCs with up to 52 ports divided into four
> different model lines.
> 
> Changes in v3
> 

<form letter>
This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.
</form letter>

>  - renamed to realtek,rtl8380m-serdes.yaml
>  - removed parameter controlled-ports
>  - verified with make dt_binding_check
>  - recipient list according to get_maintainers
> 
> Changes in v2:
> 
>  - new subject
>  - removed patch command sequences
>  - renamed parameter controlled-ports to realtek,controlled-ports
> 
> Signed-off-by: Markus Stockhausen <markus.stockhausen at gmx.de>
> ---
>  .../bindings/phy/realtek,rtl8380m-serdes.yaml | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/realtek,rtl8380m-serdes.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/realtek,rtl8380m-serdes.yaml b/Documentation/devicetree/bindings/phy/realtek,rtl8380m-serdes.yaml
> new file mode 100644
> index 000000000000..c1deef8ec63c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/realtek,rtl8380m-serdes.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/realtek,rtl8380m-serdes.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek Otto SerDes controller
> +
> +maintainers:
> +  - Markus Stockhausen <markus.stockhausen at gmx.de>
> +
> +description:
> +  The MIPS based Realtek Switch SoCs of the Realtek RTL838x, RTL839x, RTL930x
> +  and RTL931x series have multiple SerDes built in. They are linked to single,
> +  quad or octa PHYs like the RTL8218B, RTL8218D or RTL8214FC and are one of
> +  the integral part of the up-to-52-port switch architecture. Although these
> +  SerDes controllers have common basics they are designed differently in the
> +  SoC families.
> +
> +properties:
> +  $nodename:
> +    pattern: "^phy@[0-9a-f]+$"
> +
> +  compatible:
> +    items:
> +      - enum:
> +          - realtek,rtl8380m-serdes
> +          - realtek,rtl8392m-serdes
> +          - realtek,rtl9302b-serdes
> +          - realtek,rtl9311-serdes
> +
> +  reg:
> +    items:
> +      description:
> +        The primary register memory location. On RTL83xx devices this is the
> +        address to the I/O register range, on RTL93xx devices this is the
> +        address of the MDIO style command/data registers.

Not much improved, still missing constraints.

I don't understand why you introduce changes like this.


> +
> +  "#phy-cells":
> +    const: 4
> +    description:
> +      The first number defines the SerDes to use. The second number a linked
> +      SerDes. E.g. if a octa 1G PHY is attached to two QSGMII SerDes. The third
> +      number is the first switch port this SerDes is working for, the fourth
> +      number is the last switch port the SerDes is working for.
> +
> +  firmware-name:
> +    maxItems: 1
> +    description:
> +      An alternative name of the SerDes firmware image file located in the
> +      firmware search path. Set to "" to disable firmware loading.

Missing property, not empty string, should rather indicate unneeded
firmware.

> +
> +required:
> +  - compatible
> +  - reg
> +  - "#phy-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    serdes: phy at 1b00e780 {
> +      compatible = "realtek,rtl9302b-serdes";
> +      reg = <0x1b0003b0 0x8>;

This does notmatch unit address... and again: this was not an issue in
previous version. Your new versions of patchset introduce errors and
bugs. This is not how the process should look like. Review should
improve the patch, not reduce its quality.

Please start carefully reviewing your changes *before* you post.

Best regards,
Krzysztof