| Message ID | 20240208-feature_poe-v3-10-531d2674469e@bootlin.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | net: Add support for Power over Ethernet (PoE) | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 115 lines checked |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
On Thu, 08 Feb 2024 14:08:47 +0100, Kory Maincent wrote: > Before hand we set "#pse-cell" to 1 to define a PSE controller with > several PIs (Power Interface). The drawback of this was that we could not > have any information on the PI except its number. > Add support for pse_pis and pse_pi node to be able to have more information > on the PI like the number of pairset used and the pairset pinout. > > Sponsored-by: Dent Project <dentproject@linuxfoundation.org> > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> > --- > > Changes in v3: > - New patch > --- > .../bindings/net/pse-pd/pse-controller.yaml | 101 ++++++++++++++++++++- > 1 file changed, 98 insertions(+), 3 deletions(-) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/net/pse-pd/pse-controller.yaml:80:13: [warning] wrong indentation: expected 14 but found 12 (indentation) ./Documentation/devicetree/bindings/net/pse-pd/pse-controller.yaml:80:15: [error] string value is redundantly quoted with any quotes (quoted-strings) ./Documentation/devicetree/bindings/net/pse-pd/pse-controller.yaml:81:15: [error] string value is redundantly quoted with any quotes (quoted-strings) dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/pse-pd/pse-controller.yaml: $defs:pse_pi:properties:pairset-names: {'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 should correspond to a phandle in the \'pairset\' property pointing to the power supply for that pairset.', '$ref': '/schemas/types.yaml#/definitions/string-array', 'minItems': 1, 'maxItems': 2, 'items': [{'enum': ['alternative-a', 'alternative-b']}]} should not be valid under {'required': ['maxItems']} hint: "maxItems" is not needed with an "items" list from schema $id: http://devicetree.org/meta-schemas/items.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/pse-pd/pse-controller.yaml: $defs:pse_pi:properties:pairset-names: 'oneOf' conditional failed, one must be fixed: [{'enum': ['alternative-a', 'alternative-b']}] is too short False schema does not allow 1 hint: "minItems" is only needed if less than the "items" list length from schema $id: http://devicetree.org/meta-schemas/items.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240208-feature_poe-v3-10-531d2674469e@bootlin.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
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 #pse-cells > several PIs (Power Interface). The drawback of this was that we could not > have any information on the PI except its number. Then increase it to what you need. The whole point of #foo-cells is that it is variable depending on what the provider needs. > Add support for pse_pis and pse_pi node to be able to have more information > on the PI like the number of pairset used and the pairset pinout. Please explain the problem you are trying to solve, not your solution. I don't understand what the problem is to provide any useful suggestions on the design. > > Sponsored-by: Dent Project <dentproject@linuxfoundation.org> Is this a recognized tag? First I've seen it. > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> > --- > > Changes in v3: > - New patch > --- > .../bindings/net/pse-pd/pse-controller.yaml | 101 ++++++++++++++++++++- > 1 file changed, 98 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..dd5fb53e527a 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,105 @@ 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: > + $ref: "#/$defs/pse_pis" > + > +$defs: $defs is for when you need multiple copies of the same thing. I don't see that here. > + 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]+$": Unit-addresses are hex. > + $ref: "#/$defs/pse_pi" > + > + required: > + - "#address-cells" > + - "#size-cells" > + > + pse_pi: > + description: > + PSE PI device 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. > + type: object > + properties: > + reg: > + maxItems: 1 As you are defining the addressing here, you need to define what the "addresses" are. > + > + "#pse-cells": > + const: 0 > + > + pairset-names: > + 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 should > + correspond to a phandle in the 'pairset' property pointing to the > + power supply for that pairset. > + $ref: /schemas/types.yaml#/definitions/string-array > + minItems: 1 > + maxItems: 2 > + items: > + - enum: > + - "alternative-a" > + - "alternative-b" This leaves the 2nd entry undefined. You need the dictionary form of 'items' rather than a list. IOW, Drop the '-' under items. > + > + pairsets: > + 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–3) > + | Conductor | Alternative A (MDI-X) | Alternative A (MDI) | Alternative B(X) | Alternative B(S) | > + |-----------|-----------------------|---------------------|------------------|------------------| > + | 1 | Negative VPSE | Positive VPSE | — | — | > + | 2 | Negative VPSE | Positive VPSE | — | — | > + | 3 | Positive VPSE | Negative VPSE | — | — | > + | 4 | — | — | Negative VPSE | Positive VPSE | > + | 5 | — | — | Negative VPSE | Positive VPSE | > + | 6 | Positive VPSE | Negative VPSE | — | — | > + | 7 | — | — | Positive VPSE | Negative VPSE | > + | 8 | — | — | Positive VPSE | Negative VPSE | > + $ref: /schemas/types.yaml#/definitions/phandle-array > + minItems: 1 > + maxItems: 2 > + > + required: > + - reg > + - "#pse-cells" > + - pairset-names > + - pairsets > + > +allOf: > + - if: > + required: > + - "#pse-cells" > + then: > + not: > + required: > + - pse-pis > + > + - if: > + required: > + - pse-pis > + then: > + not: > + required: > + - "#pse-cells" > > additionalProperties: true > > > -- > 2.25.1 >
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 > > #pse-cells > > > several PIs (Power Interface). The drawback of this was that we could not > > have any information on the PI except its number. > > Then increase it to what you need. The whole point of #foo-cells is that > it is variable depending on what the provider needs. > > > Add support for pse_pis and pse_pi node to be able to have more information > > on the PI like the number of pairset used and the pairset pinout. > > Please explain the problem you are trying to solve, not your solution. I > don't understand what the problem is to provide any useful suggestions > on the design. Please see Oleksij's reply. Thank you Oleksij, for the documentation!! > > > > Sponsored-by: Dent Project <dentproject@linuxfoundation.org> > > 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. > > > > -required: > > - - "#pse-cells" > > + pse_pis: > > + $ref: "#/$defs/pse_pis" > > + > > +$defs: > > $defs is for when you need multiple copies of the same thing. I don't > 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]+$": > > Unit-addresses are hex. Oops sorry for the mistake. > > > + $ref: "#/$defs/pse_pi" > > + > > + required: > > + - "#address-cells" > > + - "#size-cells" > > + > > + pse_pi: > > + description: > > + PSE PI device 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. > > + type: object > > + properties: > > + reg: > > + maxItems: 1 > > As you are defining the addressing here, you need to define what the > "addresses" are. Yes I will add some documentation in next version. > > + values are "alternative-a" and "alternative-b". Each name should > > + correspond to a phandle in the 'pairset' property pointing to the > > + power supply for that pairset. > > + $ref: /schemas/types.yaml#/definitions/string-array > > + minItems: 1 > > + maxItems: 2 > > + items: > > + - enum: > > + - "alternative-a" > > + - "alternative-b" > > This leaves the 2nd entry undefined. You need the dictionary form of > '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,
On Wed, 14 Feb 2024 14:13:10 +0100 Köry Maincent <kory.maincent@bootlin.com> wrote: > 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 > > > > #pse-cells > > > > > several PIs (Power Interface). The drawback of this was that we could not > > > have any information on the PI except its number. > > > > Then increase it to what you need. The whole point of #foo-cells is that > > it is variable depending on what the provider needs. > > > > > Add support for pse_pis and pse_pi node to be able to have more > > > information on the PI like the number of pairset used and the pairset > > > pinout. > > > > Please explain the problem you are trying to solve, not your solution. I > > don't understand what the problem is to provide any useful suggestions > > on the design. > > Please see Oleksij's reply. > Thank you Oleksij, for the documentation!! > > > > > > > Sponsored-by: Dent Project <dentproject@linuxfoundation.org> > > > > 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. Not so much used indeed: $ git log --grep="Sponsored" | grep Sponsored Sponsored by: The FreeBSD Foundation Sponsored by: The FreeBSD Foundation Sponsored by: The FreeBSD Foundation Sponsored by: The FreeBSD Foundation Sponsored-by: Google Chromium project Sponsored: Google ChromeOS Sponsored: Google ChromeOS Is it ok to keep it? Regards,
On Wed, Feb 14, 2024 at 04:41:50PM +0100, Köry Maincent wrote: > On Wed, 14 Feb 2024 14:13:10 +0100 > Köry Maincent <kory.maincent@bootlin.com> wrote: > > > 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 > > > > > > #pse-cells > > > > > > > several PIs (Power Interface). The drawback of this was that we could not > > > > have any information on the PI except its number. > > > > > > Then increase it to what you need. The whole point of #foo-cells is that > > > it is variable depending on what the provider needs. > > > > > > > Add support for pse_pis and pse_pi node to be able to have more > > > > information on the PI like the number of pairset used and the pairset > > > > pinout. > > > > > > Please explain the problem you are trying to solve, not your solution. I > > > don't understand what the problem is to provide any useful suggestions > > > on the design. > > > > Please see Oleksij's reply. > > Thank you Oleksij, for the documentation!! > > > > > > > > > > Sponsored-by: Dent Project <dentproject@linuxfoundation.org> > > > > > > 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. > > Not so much used indeed: > $ git log --grep="Sponsored" | grep Sponsored > Sponsored by: The FreeBSD Foundation > Sponsored by: The FreeBSD Foundation > Sponsored by: The FreeBSD Foundation > Sponsored by: The FreeBSD Foundation > Sponsored-by: Google Chromium project > Sponsored: Google ChromeOS > Sponsored: Google ChromeOS > > Is it ok to keep it? IMO, its use should be documented like other tags, or it should not be used. Just write a sentence to the same effect. Rob
> > Not so much used indeed: > > $ git log --grep="Sponsored" | grep Sponsored > > Sponsored by: The FreeBSD Foundation > > Sponsored by: The FreeBSD Foundation > > Sponsored by: The FreeBSD Foundation > > Sponsored by: The FreeBSD Foundation > > Sponsored-by: Google Chromium project > > Sponsored: Google ChromeOS > > Sponsored: Google ChromeOS > > > > Is it ok to keep it? > > IMO, its use should be documented like other tags, or it should not be > used. Just write a sentence to the same effect. Or include a patch to document it :-) Andrew
On Thu, 15 Feb 2024 15:01:08 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > > > Not so much used indeed: > > > $ git log --grep="Sponsored" | grep Sponsored > > > Sponsored by: The FreeBSD Foundation > > > Sponsored by: The FreeBSD Foundation > > > Sponsored by: The FreeBSD Foundation > > > Sponsored by: The FreeBSD Foundation > > > Sponsored-by: Google Chromium project > > > Sponsored: Google ChromeOS > > > Sponsored: Google ChromeOS > > > > > > Is it ok to keep it? > > > > IMO, its use should be documented like other tags, or it should not be > > used. Just write a sentence to the same effect. > > Or include a patch to document it :-) It seems someone has already tried to send a patch to add this tag but it has not been accepted due to maintainers extra works bring by the tag: https://lore.kernel.org/lkml/20230817220957.41582-1-giulio.benetti@benettiengineering.com/ I will replace it by a small sentence then. Regards,
diff --git a/Documentation/devicetree/bindings/net/pse-pd/pse-controller.yaml b/Documentation/devicetree/bindings/net/pse-pd/pse-controller.yaml index 2d382faca0e6..dd5fb53e527a 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,105 @@ 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: + $ref: "#/$defs/pse_pis" + +$defs: + 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]+$": + $ref: "#/$defs/pse_pi" + + required: + - "#address-cells" + - "#size-cells" + + pse_pi: + description: + PSE PI device 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. + type: object + properties: + reg: + maxItems: 1 + + "#pse-cells": + const: 0 + + pairset-names: + 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 should + correspond to a phandle in the 'pairset' property pointing to the + power supply for that pairset. + $ref: /schemas/types.yaml#/definitions/string-array + minItems: 1 + maxItems: 2 + items: + - enum: + - "alternative-a" + - "alternative-b" + + pairsets: + 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–3) + | Conductor | Alternative A (MDI-X) | Alternative A (MDI) | Alternative B(X) | Alternative B(S) | + |-----------|-----------------------|---------------------|------------------|------------------| + | 1 | Negative VPSE | Positive VPSE | — | — | + | 2 | Negative VPSE | Positive VPSE | — | — | + | 3 | Positive VPSE | Negative VPSE | — | — | + | 4 | — | — | Negative VPSE | Positive VPSE | + | 5 | — | — | Negative VPSE | Positive VPSE | + | 6 | Positive VPSE | Negative VPSE | — | — | + | 7 | — | — | Positive VPSE | Negative VPSE | + | 8 | — | — | Positive VPSE | Negative VPSE | + $ref: /schemas/types.yaml#/definitions/phandle-array + minItems: 1 + maxItems: 2 + + required: + - reg + - "#pse-cells" + - pairset-names + - pairsets + +allOf: + - if: + required: + - "#pse-cells" + then: + not: + required: + - pse-pis + + - if: + required: + - pse-pis + then: + not: + required: + - "#pse-cells" additionalProperties: true
Before hand we set "#pse-cell" to 1 to define a PSE controller with several PIs (Power Interface). The drawback of this was that we could not have any information on the PI except its number. Add support for pse_pis and pse_pi node to be able to have more information on the PI like the number of pairset used and the pairset pinout. Sponsored-by: Dent Project <dentproject@linuxfoundation.org> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com> --- Changes in v3: - New patch --- .../bindings/net/pse-pd/pse-controller.yaml | 101 ++++++++++++++++++++- 1 file changed, 98 insertions(+), 3 deletions(-)