Re: [PATCH 9/9] dt-bindings: net: ipq4019-mdio: Document ipq5332 platform

[Krzysztof Kozlowski]

On 15/11/2023 04:25, Luo Jie wrote:
> On platform IPQ5332, the MDIO address of qca8084 can be programed
> when the device tree property "fixup" defined, the clock sequence
> needs to be completed before the PHY probeable.
> 
> Signed-off-by: Luo Jie <quic_luoj@xxxxxxxxxxx>
> ---
>  .../bindings/net/qcom,ipq4019-mdio.yaml       | 138 +++++++++++++++++-
>  1 file changed, 130 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> index 3407e909e8a7..7ff92be14ee1 100644
> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
> @@ -15,11 +15,13 @@ properties:
>        - enum:
>            - qcom,ipq4019-mdio
>            - qcom,ipq5018-mdio
> +          - qcom,ipq5332-mdio
>  
>        - items:
>            - enum:
>                - qcom,ipq6018-mdio
>                - qcom,ipq8074-mdio
> +              - qcom,ipq9574-mdio
>            - const: qcom,ipq4019-mdio
>  
>    "#address-cells":
> @@ -30,19 +32,47 @@ properties:
>  
>    reg:
>      minItems: 1
> -    maxItems: 2
> +    maxItems: 5
>      description:
> -      the first Address and length of the register set for the MDIO controller.
> -      the second Address and length of the register for ethernet LDO, this second
> -      address range is only required by the platform IPQ50xx.
> +      the first Address and length of the register set for the MDIO controller,
> +      the optional second, third and fourth address and length of the register
> +      for ethernet LDO, these three address range are required by the platform
> +      IPQ50xx/IPQ5332, the last address and length is for the CMN clock to
> +      select the reference clock.
> +
> +  reg-names:
> +    minItems: 1
> +    maxItems: 5

You must describe the items and constrain them per each variant.

>  
>    clocks:
> -    items:
> -      - description: MDIO clock source frequency fixed to 100MHZ
> +    minItems: 1
> +    maxItems: 5
> +    description:

Doesn't this make all other variants with incorrect constraints?

> +      MDIO system clock frequency fixed to 100MHZ, and the GCC uniphy
> +      clocks enabled for resetting ethernet PHY.
>  
>    clock-names:
> -    items:
> -      - const: gcc_mdio_ahb_clk
> +    minItems: 1
> +    maxItems: 5
> +
> +  phy-reset-gpio:

No, for multiple reasons. It's gpios first of all. Where do you see such
property? Where is the existing definition?

Then it is "reset-gpios" if this is MDIO. Why do you put phy properties
in MDIO?

> +    minItems: 1
> +    maxItems: 3
> +    description:
> +      GPIO used to reset the PHY, each GPIO is for resetting the connected
> +      ethernet PHY device.
> +
> +  phyaddr-fixup:
> +    description: Register address for programing MDIO address of PHY devices

You did not test code which you sent.

> +
> +  pcsaddr-fixup:
> +    description: Register address for programing MDIO address of PCS devices
> +
> +  mdio-clk-fixup:
> +    description: The initialization clocks to be configured
> +
> +  fixup:
> +    description: The MDIO address of PHY/PCS device to be programed

Please do not send untested code.


...

> +
> +      qca8kphy2: ethernet-phy@3 {
> +        reg = <3>;
> +        fixup;
> +      };
> +
> +      qca8kphy3: ethernet-phy@4 {
> +        reg = <4>;
> +        fixup;
> +      };
> +
> +      qca8kpcs0: pcsphy0@5 {
> +        compatible = "qcom,qca8k_pcs";
> +        reg = <5>;
> +        fixup;
> +        };

Fix indentation.

Best regards,
Krzysztof