Message-ID: <20241115150210.GA2680735-robh@kernel.org>
Date: Fri, 15 Nov 2024 09:02:10 -0600
From: Rob Herring <robh@...nel.org>
To: Geert Uytterhoeven <geert+renesas@...der.be>
Cc: Ben Dooks <ben.dooks@...ethink.co.uk>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH] [RFC] dt-bindings: net: micrel: Convert to json-schema

On Thu, Nov 14, 2024 at 11:42:50AM +0100, Geert Uytterhoeven wrote:
> Convert the Micrel PHY Device Tree binding documentation to json-schema.
> 
> Add a simple example.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@...der.be>
> ---
> Notes:
>   1. I specified Ben Dooks as the maintainer, as he wrote the original
>      bindings. Ben, are you OK with that?
>   2. This schema is never applied, as there is no compatible value or
>      select statement. Adding
> 
> 	select:
> 	  properties:
> 	    $nodename:
> 	      pattern: "^ethernet-phy(@[a-f0-9]+)?$"
> 
> 	  required:
> 	    - $nodename
> 
>      and changing
> 
> 	-unevaluatedProperties: false
> 	+additionalProperties: true
> 
>      would fix that, and is mostly harmless, except for possible
>      conflicts with other Ethernet PHYs having more than one clock, or
>      using different clock-names.
>      Documentation/devicetree/bindings/net/qca,ar803x.yaml has the same
>      issue.
>      Is there a proper way to handle this?  Are there other options than
>      mandating specific compatible values for Ethernet PHYs?

The proper way is simply, if you need to describe your phy in DT, it 
needs a compatible string. MDIO phys are not special.

We really need to split ethernet-phy.yaml into common properties and a 
specific schema for the compatibles it contains so that we can change 
'additionalProperties: true'. That's one reason why all these properties 
and typos didn't get flagged.

If you don't want to retro-actively add a compatible, you can also do 
something like this:

select:
  anyOf:
    - required: ['micrel,led-mode']
    - required: ['micrel,rmii-reference-clock-select-25-mhz']
    - required: ['micrel,fiber-mode']
    - required: ['coma-mode-gpios']

That doesn't catch every case nor if you have a typo in the property 
names.

> Thanks for your comments!
> ---
>  .../devicetree/bindings/net/micrel,phy.yaml   | 93 +++++++++++++++++++
>  .../devicetree/bindings/net/micrel.txt        | 57 ------------
>  2 files changed, 93 insertions(+), 57 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/micrel,phy.yaml
>  delete mode 100644 Documentation/devicetree/bindings/net/micrel.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/micrel,phy.yaml b/Documentation/devicetree/bindings/net/micrel,phy.yaml
> new file mode 100644
> index 0000000000000000..609bbd9729efe516
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/micrel,phy.yaml
> @@ -0,0 +1,93 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/micrel,phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Micrel PHY properties
> +
> +maintainers:
> +  - Ben Dooks <ben.dooks@...ethink.co.uk>
> +
> +properties:
> +  micrel,led-mode:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [ 0, 1, 2, 3 ]
> +    description: |
> +      LED mode value to set for PHYs with configurable LEDs.
> +
> +      Configure the LED mode with single value. The list of PHYs and the
> +      bits that are currently supported:
> +
> +      KSZ8001: register 0x1e, bits 15..14
> +      KSZ8041: register 0x1e, bits 15..14
> +      KSZ8021: register 0x1f, bits 5..4
> +      KSZ8031: register 0x1f, bits 5..4
> +      KSZ8051: register 0x1f, bits 5..4
> +      KSZ8081: register 0x1f, bits 5..4
> +      KSZ8091: register 0x1f, bits 5..4
> +      LAN8814: register EP5.0, bit 6
> +
> +      See the respective PHY datasheet for the mode values.
> +
> +  micrel,rmii-reference-clock-select-25-mhz:
> +    description: |
> +      RMII Reference Clock Select bit selects 25 MHz mode
> +
> +      Setting the RMII Reference Clock Select bit enables 25 MHz rather
> +      than 50 MHz clock mode.
> +
> +      Note that this option in only needed for certain PHY revisions with a
> +      non-standard, inverted function of this configuration bit.
> +      Specifically, a clock reference ("rmii-ref" below) is always needed to
> +      actually select a mode.
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: rmii-ref
> +    description: |
> +      supported clocks:
> +        - KSZ8021, KSZ8031, KSZ8081, KSZ8091: "rmii-ref": The RMII reference
> +          input clock. Used to determine the XI input clock.

Don't repeat the clock name in the description.

> +
> +  micrel,fiber-mode:
> +    type: boolean
> +    description: |
> +      If present the PHY is configured to operate in fiber mode.
> +
> +      Some PHYs, such as the KSZ8041FTL variant, support fiber mode, enabled
> +      by the FXEN boot strapping pin. It can't be determined from the PHY
> +      registers whether the PHY is in fiber mode, so this boolean device tree
> +      property can be used to describe it.
> +
> +      In fiber mode, auto-negotiation is disabled and the PHY can only work in
> +      100base-fx (full and half duplex) modes.
> +
> +  coma-mode-gpios:
> +    description: |
> +      If present the given gpio will be deasserted when the PHY is probed.
> +
> +      Some PHYs have a COMA mode input pin which puts the PHY into
> +      isolate and power-down mode. On some boards this input is connected
> +      to a GPIO of the SoC.
> +
> +      Supported on the LAN8814.

Another reason to add compatible. You have per device properties.

> +
> +dependencies:
> +  micrel,rmii-reference-clock-select-25-mhz: [ clock-names ]
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    ethernet {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ethernet-phy@1 {
> +            reg = <1>;
> +            micrel,led-mode = <1>;
> +        };
> +    };

Powered by blists - more mailing lists