Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Mon, 18 Sep 2023 18:31:13 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     Jay Monkman <jtm@lopingdog.com>
Cc:     devicetree@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Piergiorgio Beruto <piergiorgio.beruto@gmail.com>,
        Arndt Schuebel <Arndt.Schuebel@onsemi.com>
Subject: Re: [PATCH 1/4] dt-bindings: net: Add bindings for onsemi NCN26000
 PHY
Message-ID: <a467d803-3c71-4008-8d6e-7bb03128ea44@lunn.ch>
References: <ZQf1LMnICzEnt9XK@lopingdog.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZQf1LMnICzEnt9XK@lopingdog.com>
Precedence: bulk

> +properties:
> +  description: |
> +    Sets the transmitter amplitude gain. If not specified,
> +    gain is set to 1.0V (1v0)
> +  tx-gain:
> +    enum:
> +      - 1v1
> +      - 1v0
> +      - 0v9
> +      - 0v8

How is a gain in volts? Isn't gain just a multiplier, so it unitless?

> +  tx-slew:
> +    description: |
> +      Sets the slew rate of the TX line driver output. Defaults
> +      to slow if not set.
> +    enum:
> +      - fast
> +      - slow

How does this map to a standard? I would expect the standard to
specify the slew. So why is this needed?

> +  dig-slew:
> +    description: |
> +      Sets the slew rate of the digital output pins. Defaults
> +      to slow if not set.

By Digital output pins, do you them the GPO pins you are adding a GPIO
driver for?

> +  cmc-comp:
> +    description: |
> +      Sets the common mode choke resistance (CMC compensation).
> +      Defaults to 0-0.5 ohm (0p25) if not set.
> +    enum:
> +      - 0p25
> +      - 1p38
> +      - 3p00
> +      - 3p37

What is the mapping between 0-0.5 ohm and 0p25? Can we just use ohms
here?

> +  plca-precedence:
> +    description: |
> +      Enables PLCA precedence mode. Defaults to off if not
> +      set.
> +    type: boolean

What is PLCA precedence? Should this be an ethtool parameter, along
side all the other PLCA controls?

> +  eni-mode:
> +    description: |
> +      Enables Enhanced Noise Immunity mode. Defaults to off if
> +      not set.
> +    enum:
> +      - force-on
> +      - force-off
> +      - auto

phy tunable?

> +
> +  tx-pkt-loop:
> +    description: |
> +      Enables packet loopback mode. Defaults to off is not set.
> +    type: boolean

How does this differ from struct phy_driver::set_loopback()?

> +  unjab-tmr-disable:
> +    description: |
> +      Disables the Unjab Timer. When disabled, device transmission
> +      will be stopped due to a jabber error and only restarted on
> +      device reset. If not set, this defaults to enabled.
> +    type: boolean
> +
> +  col-disable:
> +    description: |
> +      Disables collision masking. Defaults to enabled if not set.
> +    type: boolean

I could be wrong, but this feels like an SDK dump of all the features
the device has, but nobody will ever user in reality. We need some
justification why all these properties are really needed, and ideally
a .dts file for a board actually using them. If there is no user, i
suggest waiting until somebody really does need them.

	Andrew