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

[Krzysztof Kozlowski <krzk@kernel.org>]

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@motor-comm.com>

Didn't you copy user-name as you name?

> ---
>  .../bindings/net/motorcomm,yt8xxx.yaml          | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)

Also, your threading is completely broken. Use git send-email or b4.

> 
> 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?

> +      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.

> +    $ref: /schemas/types.yaml#/definitions/uint8

Make it a string, not uint8.


> +    enum: [ 0, 1 ]
> +    default: 1

Why 1 not 0? Auto seems more logical?

> +
>  unevaluatedProperties: false
>  
>  examples:

Best regards,
Krzysztof