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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 15/12/2023 09:28, Jie Luo wrote:
> 
> 
> On 12/15/2023 3:27 PM, Krzysztof Kozlowski wrote:
>> On 14/12/2023 10:03, Luo Jie wrote:
>>> Update the yaml file for the new DTS properties.
>>>
>>> 1. cmn-reference-clock for the CMN PLL source clock select.
>>> 2. clock-frequency for MDIO clock frequency config.
>>> 3. add uniphy AHB & SYS GCC clocks.
>>> 4. add reset-gpios for MDIO bus level reset.
>>>
>>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>>> ---
>>>   .../bindings/net/qcom,ipq4019-mdio.yaml       | 143 +++++++++++++++++-
>>>   1 file changed, 139 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>> index 3407e909e8a7..79f8513739e7 100644
>>> --- a/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>> +++ b/Documentation/devicetree/bindings/net/qcom,ipq4019-mdio.yaml
>>> @@ -20,6 +20,8 @@ properties:
>>>             - enum:
>>>                 - qcom,ipq6018-mdio
>>>                 - qcom,ipq8074-mdio
>>> +              - qcom,ipq9574-mdio
>>> +              - qcom,ipq5332-mdio

Why do you add entries to the end of the list? In reversed order?

>>>             - const: qcom,ipq4019-mdio
>>>   
>>>     "#address-cells":
>>> @@ -30,19 +32,77 @@ 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
>>>   
>>>     clocks:
>>> +    minItems: 1
>>>       items:
>>>         - description: MDIO clock source frequency fixed to 100MHZ
>>> +      - description: UNIPHY0 AHB clock source frequency fixed to 100MHZ
>>> +      - description: UNIPHY1 AHB clock source frequency fixed to 100MHZ
>>> +      - description: UNIPHY0 SYS clock source frequency fixed to 24MHZ
>>> +      - description: UNIPHY1 SYS clock source frequency fixed to 24MHZ
>>>   
>>>     clock-names:
>>> +    minItems: 1
>>>       items:
>>>         - const: gcc_mdio_ahb_clk
>>> +      - const: uniphy0_ahb
>>> +      - const: uniphy1_ahb
>>> +      - const: uniphy0_sys
>>> +      - const: uniphy1_sys
>>> +
>>> +  cmn-reference-clock:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>
>> Nothing improved here
> 
> With this change, the warning is not reported when i run
> dt_binding_check, looks the new added property needs
> the type ref to avoid the warning reported.

Nothing improved in the property name, nor its style, nor in the actual
contents/values.

...

>>> +  reset-gpios:
>>> +    maxItems: 1
>>> +
>>> +  reset-assert-us:
>>> +    maxItems: 1
>>
>> This does not look related to ipq5332.
> 
> The reset gpio properties are needed on ipq5332, since qca8084 phy is
> connected, which uses the MDIO bus level gpio reset.

I am talking about this property, not these properties.

> 
> Without declaring these gpio properties, the warning will be reported
> by dt_binding_check.

How is it even possible to have warnings if there is no such node in
DTS? We do not care about warnings in your downstream code.


Best regards,
Krzysztof