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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ac84b12f-ae91-4a2f-a5f7-88febd13911c@kernel.org>
Date: Sat, 27 Jul 2024 11:25:25 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: "Frank.Sae" <Frank.Sae@...or-comm.com>, andrew@...n.ch,
 hkallweit1@...il.com, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, linux@...linux.org.uk
Cc: netdev@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, yuanlai.cui@...or-comm.com,
 hua.sun@...or-comm.com, xiaoyong.li@...or-comm.com,
 suting.hu@...or-comm.com, jie.han@...or-comm.com
Subject: Re: [PATCH 1/2] dt-bindings: net: motorcomm: Add chip mode cfg

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@...or-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