Re: [PATCH 1/2] dt-bindings: net: motorcomm: Add chip mode cfg

[Krzysztof Kozlowski]

On 01/08/2024 11:49, Frank.Sae wrote:
> 
> On 7/27/24 02:25, Krzysztof Kozlowski wrote:
>> On 27/07/2024 11:20, Frank.Sae wrote:
>>>   The motorcomm phy (yt8821) supports the ability to
>>>   config the chip mode of serdes.
>>>   The yt8821 serdes could be set to AUTO_BX2500_SGMII or
>>>   FORCE_BX2500.
>>>   In AUTO_BX2500_SGMII mode, SerDes
>>>   speed is determined by UTP, if UTP link up
>>>   at 2.5GBASE-T, SerDes will work as
>>>   2500BASE-X, if UTP link up at
>>>   1000BASE-T/100BASE-Tx/10BASE-T, SerDes will work
>>>   as SGMII.
>>>   In FORCE_BX2500, SerDes always works
>>>   as 2500BASE-X.
>> Very weird wrapping.
>>
>> Please wrap commit message according to Linux coding style / submission
>> process (neither too early nor over the limit):
>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
>>
>>> Signed-off-by: Frank.Sae<Frank.Sae@xxxxxxxxxxxxxx>
>> Didn't you copy user-name as you name?
> 
> sorry, not understand your mean.

Does your ID (e.g. passport) has exactly that name? With dot? Really?

> 
>>> ---
>>>   .../bindings/net/motorcomm,yt8xxx.yaml          | 17 +++++++++++++++++
>>>   1 file changed, 17 insertions(+)
>> Also, your threading is completely broken. Use git send-email or b4.
> 
> sorry, not understand your mean of threading broken. the patch used git
> send-email.

Email threading is completely broken. It means all reviewers see it as
complete unrelated emails which makes any review annoyingly more difficult.


> 
> 
>>> diff --git a/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml b/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
>>> index 26688e2302ea..ba34260f889d 100644
>>> --- a/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
>>> +++ b/Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
>>> @@ -110,6 +110,23 @@ properties:
>>>         Transmit PHY Clock delay train configuration when speed is 1000Mbps.
>>>       type: boolean
>>>   
>>> +  motorcomm,chip-mode:
>>> +    description: |
>>> +      Only for yt8821 2.5G phy, it supports two chip working modes,
>> Then allOf:if:then disallowing it for the other variant?
> 
> sorry, not understand your mean.

So you did not understand anything from any comments?

You miss if:then: block which disallows this for other chips. You claim
only one device supports it, so why the property should be valid for
other devices?


> 
> 
>>> +      one is AUTO_BX2500_SGMII, the other is FORCE_BX2500.
>>> +      If this property is not set in device tree node then driver
>>> +      selects chip mode FORCE_BX2500 by default.
>> Don't repeat constraints in free form text.
>>
>>> +      0: AUTO_BX2500_SGMII
>>> +      1: FORCE_BX2500
>>> +      In AUTO_BX2500_SGMII mode, serdes speed is determined by UTP,
>>> +      if UTP link up at 2.5GBASE-T, serdes will work as 2500BASE-X,
>>> +      if UTP link up at 1000BASE-T/100BASE-Tx/10BASE-T, serdes will
>>> +      work as SGMII.
>>> +      In FORCE_BX2500 mode, serdes always works as 2500BASE-X.
>>
>> Explain why this is even needed and why "auto" is not correct in all
>> cases. In commit msg or property description.
> 
> yt8821 phy does not support strapping to config the serdes mode, so config the
> serdes mode by dts instead.
> 
> even if auto 2500base-x serdes mode is default mode after phy hard reset, and
> auto as default must be make sense, but from most our customers's feedback,
> force 2500base-x serdes mode is used in project usually to adapt to mac's serdes
> settings. for customer's convenience and use simplicity, force 2500base-x serdes
> mode selected as default here.

Sorry, this is not a proper sentence and I cannot parse it.

> 
> 
>>> +    $ref: /schemas/types.yaml#/definitions/uint8
>> Make it a string, not uint8.
>>
> why do you suggest string, the property value(uint8) will be wrote to phy
> register.

Because it is much more readable. I don't care about your phy register.

Best regards,
Krzysztof