Received-SPF: pass (google.com: domain of linux-kernel+bounces-128040-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 2 Apr 2024 08:26:37 -0500
From: Rob Herring <robh@kernel.org>
To: Kory Maincent <kory.maincent@bootlin.com>
Cc: "David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Luis Chamberlain <mcgrof@kernel.org>,
	Russ Weight <russ.weight@linux.dev>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Oleksij Rempel <o.rempel@pengutronix.de>,
	Mark Brown <broonie@kernel.org>,
	Frank Rowand <frowand.list@gmail.com>, Andrew Lunn <andrew@lunn.ch>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Russell King <linux@armlinux.org.uk>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-doc@vger.kernel.org, devicetree@vger.kernel.org,
	Dent Project <dentproject@linuxfoundation.org>
Subject: Re: [PATCH net-next v6 11/17] dt-bindings: net: pse-pd: Add another
 way of describing several PSE PIs
Message-ID: <20240402132637.GA3744978-robh@kernel.org>
References: <20240326-feature_poe-v6-0-c1011b6ea1cb@bootlin.com>
 <20240326-feature_poe-v6-11-c1011b6ea1cb@bootlin.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240326-feature_poe-v6-11-c1011b6ea1cb@bootlin.com>

On Tue, Mar 26, 2024 at 03:04:48PM +0100, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> 
> PSE PI setup may encompass multiple PSE controllers or auxiliary circuits
> that collectively manage power delivery to one Ethernet port.
> Such configurations might support a range of PoE standards and require
> the capability to dynamically configure power delivery based on the
> operational mode (e.g., PoE2 versus PoE4) or specific requirements of
> connected devices. In these instances, a dedicated PSE PI node becomes
> essential for accurately documenting the system architecture. This node
> would serve to detail the interactions between different PSE controllers,
> the support for various PoE modes, and any additional logic required to
> coordinate power delivery across the network infrastructure.
> 
> The old usage of "#pse-cells" is unsuficient as it carries only the PSE PI
> index information.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
> 
> Changes in v3:
> - New patch
> 
> Changes in v4:
> - Remove $def
> - Fix pairset-names item list
> - Upgrade few properties description
> - Update the commit message
> 
> Changes in v5:
> - Fix yamllint error.
> - Replace underscore by dash in properties names.
> - Add polarity-supported property.
> 
> Changes in v6:
> - Reorder the pairset pinout table documentation to shrink the lines size.
> - Remove pairset and polarity as required fields.
> - Add vpwr-supply regulator supply.
> ---
>  .../bindings/net/pse-pd/pse-controller.yaml        | 102 ++++++++++++++++++++-
>  1 file changed, 99 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/pse-pd/pse-controller.yaml b/Documentation/devicetree/bindings/net/pse-pd/pse-controller.yaml
> index 2d382faca0e6..03f7f215c162 100644
> --- a/Documentation/devicetree/bindings/net/pse-pd/pse-controller.yaml
> +++ b/Documentation/devicetree/bindings/net/pse-pd/pse-controller.yaml
> @@ -13,6 +13,7 @@ description: Binding for the Power Sourcing Equipment (PSE) as defined in the
>  
>  maintainers:
>    - Oleksij Rempel <o.rempel@pengutronix.de>
> +  - Kory Maincent <kory.maincent@bootlin.com>
>  
>  properties:
>    $nodename:
> @@ -22,11 +23,106 @@ properties:
>      description:
>        Used to uniquely identify a PSE instance within an IC. Will be
>        0 on PSE nodes with only a single output and at least 1 on nodes
> -      controlling several outputs.
> +      controlling several outputs which are not described in the pse-pis
> +      subnode. This property is deprecated, please use pse-pis instead.
>      enum: [0, 1]
>  
> -required:
> -  - "#pse-cells"
> +  pse-pis:
> +    type: object
> +    description:
> +      Overview of the PSE PIs provided by the controller.
> +
> +    properties:
> +      "#address-cells":
> +        const: 1
> +
> +      "#size-cells":
> +        const: 0
> +
> +    required:
> +      - "#address-cells"
> +      - "#size-cells"
> +
> +    patternProperties:
> +      "^pse-pi@[0-9a-f]+$":
> +        type: object
> +        description:
> +          PSE PI for power delivery via pairsets, compliant with IEEE
> +          802.3-2022, Section 145.2.4. Each pairset comprises a positive and
> +          a negative VPSE pair, adhering to the pinout configurations
> +          detailed in the standard.
> +          See Documentation/networking/pse-pd/pse-pi.rst for details.
> +
> +        properties:
> +          reg:
> +            description:
> +              Address describing the PSE PI index.
> +            maxItems: 1
> +
> +          "#pse-cells":
> +            const: 0
> +
> +          pairset-names:
> +            $ref: /schemas/types.yaml#/definitions/string-array
> +            description:
> +              Names of the pairsets as per IEEE 802.3-2022, Section 145.2.4.
> +              Valid values are "alternative-a" and "alternative-b". Each name

Don't state constraints in prose which are defined as schema 
constraints.

> +              should correspond to a phandle in the 'pairset' property
> +              pointing to the power supply for that pairset.
> +            minItems: 1
> +            maxItems: 2
> +            items:
> +              enum:
> +                - alternative-a
> +                - alternative-b
> +
> +          pairsets:
> +            $ref: /schemas/types.yaml#/definitions/phandle-array
> +            description:
> +              List of phandles, each pointing to the power supply for the
> +              corresponding pairset named in 'pairset-names'. This property
> +              aligns with IEEE 802.3-2022, Section 33.2.3 and 145.2.4.
> +              PSE Pinout Alternatives (as per IEEE 802.3-2022 Table 145\u20133)
> +              |-----------|---------------|---------------|---------------|---------------|
> +              | Conductor | Alternative A | Alternative A | Alternative B | Alternative B |
> +              |           |    (MDI-X)    |     (MDI)     |      (X)      |      (S)      |
> +              |-----------|---------------|---------------|---------------|---------------|
> +              | 1         | Negative VPSE | Positive VPSE | \u2014             | \u2014             |
> +              | 2         | Negative VPSE | Positive VPSE | \u2014             | \u2014             |
> +              | 3         | Positive VPSE | Negative VPSE | \u2014             | \u2014             |
> +              | 4         | \u2014             | \u2014             | Negative VPSE | Positive VPSE |
> +              | 5         | \u2014             | \u2014             | Negative VPSE | Positive VPSE |
> +              | 6         | Positive VPSE | Negative VPSE | \u2014             | \u2014             |
> +              | 7         | \u2014             | \u2014             | Positive VPSE | Negative VPSE |
> +              | 8         | \u2014             | \u2014             | Positive VPSE | Negative VPSE |
> +            minItems: 1
> +            maxItems: 2

"pairsets" does not follow the normal design pattern of foos, foo-names, 
and #foo-cells. You could add #foo-cells I suppose, but what would cells 
convey? I don't think it's a good fit for what you need.

The other oddity is the number of entries and the names are fixed. That 
is usually defined per consumer. 

As each entry is just a power rail, why can't the regulator binding be 
used here?

> +
> +          polarity-supported:
> +            $ref: /schemas/types.yaml#/definitions/string-array
> +            description:
> +              Polarity configuration supported by the PSE PI pairsets.
> +            minItems: 1
> +            maxItems: 4
> +            items:
> +              enum:
> +                - MDI-X
> +                - MDI
> +                - X
> +                - S
> +
> +          vpwr-supply:
> +            description: Regulator power supply for the PSE PI.

I don't see this being used anywhere.

> +
> +        required:
> +          - reg
> +          - "#pse-cells"
> +
> +oneOf:
> +  - required:
> +      - "#pse-cells"
> +  - required:
> +      - pse-pis
>  
>  additionalProperties: true
>  
> 
> -- 
> 2.25.1
>