Re: [PATCH 1/5] dt-bindings: net: qca,ar803x: Add IPQ5018 Internal GE PHY support

[Krzysztof Kozlowski <krzk@kernel.org>]

On 26/05/2025 08:43, George Moussalem wrote:
>>> +  qca,dac:
>>> +    description:
>>> +      Values for MDAC and EDAC to adjust amplitude, bias current settings,
>>> +      and error detection and correction algorithm. Only set in a PHY to PHY
>>> +      link architecture to accommodate for short cable length.
>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>> +    items:
>>> +      - items:
>>> +          - description: value for MDAC. Expected 0x10, if set
>>> +          - description: value for EDAC. Expected 0x10, if set
>>
>> If this is fixed to 0x10, then this is fully deducible from compatible.
>> Drop entire property.
> 
> as mentioned to Andrew, I can move the required values to the driver 
> itself, but a property would still be required to indicate that this PHY 
> is connected to an external PHY (ex. qca8337 switch). In that case, the 
> values need to be set. Otherwise, not..
> 
> Would qcom,phy-to-phy-dac (boolean) do?

Seems fine to me.

> 
>>
>>> +      - maxItems: 1
>>> +
>>> +  qca,eth-ldo-enable:
>>
>> qcom,tcsr-syscon to match property already used.
> 
> to make sure I understand correctly, rename it to qcom,tcsr-syscon?

Yes

> 
>>
>>> +    description:
>>> +      Register in TCSR to enable the LDO controller to supply
>>> +      low voltages to the common ethernet block (CMN BLK).
>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +    items:
>>> +      - items:
>>> +          - description: phandle of TCSR syscon
>>> +          - description: offset of TCSR register to enable the LDO controller
>>> +      - maxItems: 1
>> You listed two items, but second is just one item? Drop.
> 
> What is expected is one item that has two values, in this case: <&tcsr 
> 0x019475c4>

I know.

> 
> I could move the offset to the driver itself as it's a fixed offset, so 
> ultimately the property would become:
> 
> qcom,tcsr-syscon = <&tscr>;
> 
> agreed?
No. Just fix the syntax.

Best regards,
Krzysztof