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

On 01/08/2024 11:49, Frank.Sae wrote:

On 7/27/24 02:25, Krzysztof Kozlowski wrote:

quoted

On 27/07/2024 11:20, Frank.Sae wrote:

quoted

  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

quoted

Signed-off-by: Frank.Sae<Frank.Sae@motor-comm.com>

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?

quoted

quoted

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

quoted

quoted

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?

quoted

quoted

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

quoted

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

quoted

quoted

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