Received-SPF: pass (google.com: domain of linux-kernel+bounces-65256-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 14 Feb 2024 14:13:10 +0100
From: =?UTF-8?B?S8O2cnk=?= Maincent <kory.maincent@bootlin.com>
To: Rob Herring <robh@kernel.org>
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 v3 10/17] dt-bindings: net: pse-pd: Add another
 way of describing several PSE PIs
Message-ID: <20240214141310.119364c4@kmaincent-XPS-13-7390>
In-Reply-To: <20240209144349.GA3678044-robh@kernel.org>
References: <20240208-feature_poe-v3-0-531d2674469e@bootlin.com>
	<20240208-feature_poe-v3-10-531d2674469e@bootlin.com>
	<20240209144349.GA3678044-robh@kernel.org>
Organization: bootlin
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hello Rob,

Thanks for your review!

On Fri, 9 Feb 2024 14:43:49 +0000
Rob Herring <robh@kernel.org> wrote:

> On Thu, Feb 08, 2024 at 02:08:47PM +0100, Kory Maincent wrote:
> > Before hand we set "#pse-cell" to 1 to define a PSE controller with =20
>=20
> #pse-cells
>=20
> > several PIs (Power Interface). The drawback of this was that we could n=
ot
> > have any information on the PI except its number. =20
>=20
> Then increase it to what you need. The whole point of #foo-cells is that=
=20
> it is variable depending on what the provider needs.=20
>=20
> > Add support for pse_pis and pse_pi node to be able to have more informa=
tion
> > on the PI like the number of pairset used and the pairset pinout. =20
>=20
> Please explain the problem you are trying to solve, not your solution. I=
=20
> don't understand what the problem is to provide any useful suggestions=20
> on the design.

Please see Oleksij's reply.
Thank you Oleksij, for the documentation!!

> >=20
> > Sponsored-by: Dent Project <dentproject@linuxfoundation.org> =20
>=20
> Is this a recognized tag? First I've seen it.

This is not a standard tag but it has been used several times in the past.

> > =20
> > -required:
> > -  - "#pse-cells"
> > +  pse_pis:
> > +    $ref: "#/$defs/pse_pis"
> > +
> > +$defs: =20
>=20
> $defs is for when you need multiple copies of the same thing. I don't=20
> see that here.

I made this choice for better readability but indeed it is used only once.
I will remove it then.

> > +  pse_pis:
> > +    type: object
> > +    description:
> > +      Kind of a matrix to identify the concordance between a PSE Power
> > +      Interface and one or two (PoE4) physical ports.
> > +
> > +    properties:
> > +      "#address-cells":
> > +        const: 1
> > +
> > +      "#size-cells":
> > +        const: 0
> > +
> > +    patternProperties:
> > +      "^pse_pi@[0-9]+$": =20
>=20
> Unit-addresses are hex.

Oops sorry for the mistake.

>=20
> > +        $ref: "#/$defs/pse_pi"
> > +
> > +    required:
> > +      - "#address-cells"
> > +      - "#size-cells"
> > +
> > +  pse_pi:
> > +    description:
> > +      PSE PI device for power delivery via pairsets, compliant with IE=
EE
> > +      802.3-2022, Section 145.2.4. Each pairset comprises a positive a=
nd a
> > +      negative VPSE pair, adhering to the pinout configurations detail=
ed in
> > +      the standard.
> > +    type: object
> > +    properties:
> > +      reg:
> > +        maxItems: 1 =20
>=20
> As you are defining the addressing here, you need to define what the=20
> "addresses" are.

Yes I will add some documentation in next version.

> > +          values are "alternative-a" and "alternative-b". Each name sh=
ould
> > +          correspond to a phandle in the 'pairset' property pointing t=
o the
> > +          power supply for that pairset.
> > +        $ref: /schemas/types.yaml#/definitions/string-array
> > +        minItems: 1
> > +        maxItems: 2
> > +        items:
> > +          - enum:
> > +            - "alternative-a"
> > +            - "alternative-b" =20
>=20
> This leaves the 2nd entry undefined. You need the dictionary form of=20
> 'items' rather than a list. IOW, Drop the '-' under items.

Oh thanks! That is what I was looking for. I was struggling using the right
description.

Regards,
--=20
K=C3=B6ry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com