From: Gregory Fuchedgi <gfuchedgi@gmail.com>
Update schema after per-port poe class restrictions and a few other options
were implemented.
Signed-off-by: Gregory Fuchedgi <gfuchedgi@gmail.com>
---
.../devicetree/bindings/hwmon/ti,tps23861.yaml | 86 ++++++++++++++++++++++
1 file changed, 86 insertions(+)
diff --git a/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml b/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
index ee7de53e19184d4c3df7564624532306d885f6e4..578f4dad7eab630b218e9e30b23fc611a760d332 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
@@ -24,12 +24,62 @@ properties:
reg:
maxItems: 1
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
shunt-resistor-micro-ohms:
description: The value of current sense resistor in microohms.
default: 255000
minimum: 250000
maximum: 255000
+ reset-gpios:
+ description: Optional GPIO for the reset pin.
+ maxItems: 1
+
+ shutdown-gpios:
+ description: |
+ Optional GPIO for the shutdown pin. Used to prevent PoE activity before
+ the driver had a chance to configure the chip.
+ maxItems: 1
+
+ interrupts:
+ description: |
+ The interrupt specifier. Only required if PoE class is restricted to less
+ than class 4 in the device tree.
+ maxItems: 1
+
+patternProperties:
+ "^port@[0-3]$":
+ type: object
+ description: Port specific nodes.
+ unevaluatedProperties: false
+ required:
+ - reg
+
+ properties:
+ reg:
+ description: Port index.
+ items:
+ minimum: 0
+ maximum: 3
+
+ class:
+ description: The maximum power class a port should accept.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ minimum: 0
+ maximum: 4
+
+ off-by-default:
+ description: Indicates the port is off by default.
+ type: boolean
+
+ label:
+ description: Optional port label
+
required:
- compatible
- reg
@@ -51,3 +101,39 @@ examples:
shunt-resistor-micro-ohms = <255000>;
};
};
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ tps23861@28 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "ti,tps23861";
+ reg = <0x28>;
+ shunt-resistor-micro-ohms = <255000>;
+ reset-gpios = <&gpio1 13 GPIO_ACTIVE_LOW>;
+ shutdown-gpios = <&gpio1 12 GPIO_ACTIVE_LOW>;
+ interrupt-parent = <&gpio1>;
+ interrupts = <14 0>;
+ label = "my_poe_controller";
+ port@0 {
+ reg = <0>;
+ class = <2>; // Max PoE class allowed.
+ off-by-default;
+ label = "myport";
+ };
+ port@1 {
+ reg = <1>;
+ status = "disabled";
+ };
+ port@2 {
+ reg = <2>;
+ status = "disabled";
+ };
+ port@3 {
+ reg = <3>;
+ status = "disabled";
+ };
+ };
+ };
--
2.43.0On 11/08/2025 18:13, Gregory Fuchedgi via B4 Relay wrote:
> From: Gregory Fuchedgi <gfuchedgi@gmail.com>
>
> Update schema after per-port poe class restrictions and a few other options
> were implemented.
A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>
> Signed-off-by: Gregory Fuchedgi <gfuchedgi@gmail.com>
> ---
> .../devicetree/bindings/hwmon/ti,tps23861.yaml | 86 ++++++++++++++++++++++
> 1 file changed, 86 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml b/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
> index ee7de53e19184d4c3df7564624532306d885f6e4..578f4dad7eab630b218e9e30b23fc611a760d332 100644
> --- a/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
> @@ -24,12 +24,62 @@ properties:
> reg:
> maxItems: 1
>
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 0
> +
> shunt-resistor-micro-ohms:
> description: The value of current sense resistor in microohms.
> default: 255000
> minimum: 250000
> maximum: 255000
>
> + reset-gpios:
> + description: Optional GPIO for the reset pin.
> + maxItems: 1
> +
> + shutdown-gpios:
powerdown-gpios, see gpio-consumer-common.yaml
> + description: |
Drop |
> + Optional GPIO for the shutdown pin. Used to prevent PoE activity before
> + the driver had a chance to configure the chip.
> + maxItems: 1
> +
> + interrupts:
> + description: |
> + The interrupt specifier. Only required if PoE class is restricted to less
Drop first sentence, redundant. Interrupts property cannot be anything
else than interrupt specifier.
> + than class 4 in the device tree.
> + maxItems: 1
> +
> +patternProperties:
> + "^port@[0-3]$":
This goes to ports property.
> + type: object
> + description: Port specific nodes.
> + unevaluatedProperties: false
> + required:
> + - reg
required goes to the end.
> +
> + properties:
> + reg:
> + description: Port index.
> + items:
> + minimum: 0
Drop minimum.
> + maximum: 3
> +
> + class:
> + description: The maximum power class a port should accept.
What are the values? Where is the property defined - which schema - that
you do not use vendor prefix?
> + $ref: /schemas/types.yaml#/definitions/uint32
> + minimum: 0
Drop minimum.
> + maximum: 4
> +
> + off-by-default:
Same question - which common schema defines this?
> + description: Indicates the port is off by default.
> + type: boolean
> +
> + label:
> + description: Optional port label
Skip all "optional" here and other places. Schema tells it, not free
form text. Say something useful here or just ": true".
> +
> required:
> - compatible
> - reg
> @@ -51,3 +101,39 @@ examples:
> shunt-resistor-micro-ohms = <255000>;
> };
> };
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + tps23861@28 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + #address-cells = <1>;
> + #size-cells = <0>;
Follow closely DTS coding style.
> + compatible = "ti,tps23861";
> + reg = <0x28>;
> + shunt-resistor-micro-ohms = <255000>;
> + reset-gpios = <&gpio1 13 GPIO_ACTIVE_LOW>;
> + shutdown-gpios = <&gpio1 12 GPIO_ACTIVE_LOW>;
> + interrupt-parent = <&gpio1>;
> + interrupts = <14 0>;
0 looks like invalid flag. Use proper defines and proper values.
> + label = "my_poe_controller";
Use useful names or just drop it.
> + port@0 {
> + reg = <0>;
> + class = <2>; // Max PoE class allowed.
> + off-by-default;
> + label = "myport";
Also not useful.
Best regards,
Krzysztof
On Tue, Aug 12, 2025 at 12:20 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > + shutdown-gpios:
> powerdown-gpios, see gpio-consumer-common.yaml
It is called shutdown in the datasheet, but seems like neither powerdown nor
shutdown truly reflects its purpose. This pin doesn't power down the controller
itself. It shuts down the ports while keeping the controller available for
configuration over i2c. Should I call it ti,ports-shutdown-gpios or maybe
ti,shutdown-gpios? Any other suggestions?
> > +patternProperties:
> > + "^port@[0-3]$":
> This goes to ports property.
Do you mean I should add another DT node that groups all ports? such as:
compatible = "ti,tps23861"; ports { port@0 {...} port@1 {...} }
If that's the case would it make sense to use "^.*$" pattern to allow any name
and drop the port label? Is patternProperties even needed in this case?
> > + tps23861@28 {
> Node names should be generic. See also an explanation and list of
Ack. Should I also fix the existing example in this patch?
> > + label = "my_poe_controller";
> Use useful names or just drop it.
I thought this is good as an example? A useful name would be board specific.
On 13/08/2025 05:00, Gregory Fuchedgi wrote:
> On Tue, Aug 12, 2025 at 12:20 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>> + shutdown-gpios:
>> powerdown-gpios, see gpio-consumer-common.yaml
> It is called shutdown in the datasheet, but seems like neither powerdown nor
> shutdown truly reflects its purpose. This pin doesn't power down the controller
> itself. It shuts down the ports while keeping the controller available for
> configuration over i2c. Should I call it ti,ports-shutdown-gpios or maybe
> ti,shutdown-gpios? Any other suggestions?
Feels more like enable-gpios.
>
>>> +patternProperties:
>>> + "^port@[0-3]$":
>> This goes to ports property.
> Do you mean I should add another DT node that groups all ports? such as:
> compatible = "ti,tps23861"; ports { port@0 {...} port@1 {...} }
Yes.
>
> If that's the case would it make sense to use "^.*$" pattern to allow any name
> and drop the port label? Is patternProperties even needed in this case?
You should use standard graph bindings, so:
git grep 'ref' -- Documentation/devicetree/bindings/ | grep ports
>
>>> + tps23861@28 {
>> Node names should be generic. See also an explanation and list of
> Ack. Should I also fix the existing example in this patch?
You can, up to you.
>
>>> + label = "my_poe_controller";
>> Use useful names or just drop it.
> I thought this is good as an example? A useful name would be board specific.
Then it should be board specific. You add here real and the most
complete example.
Best regards,
Krzysztof
On Sun, Aug 17, 2025 at 09:23:09AM +0200, Krzysztof Kozlowski wrote:
> On 13/08/2025 05:00, Gregory Fuchedgi wrote:
> > On Tue, Aug 12, 2025 at 12:20 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>> + shutdown-gpios:
> >> powerdown-gpios, see gpio-consumer-common.yaml
> > It is called shutdown in the datasheet, but seems like neither powerdown nor
> > shutdown truly reflects its purpose. This pin doesn't power down the controller
> > itself. It shuts down the ports while keeping the controller available for
> > configuration over i2c. Should I call it ti,ports-shutdown-gpios or maybe
> > ti,shutdown-gpios? Any other suggestions?
>
>
> Feels more like enable-gpios.
>
> >
> >>> +patternProperties:
> >>> + "^port@[0-3]$":
> >> This goes to ports property.
> > Do you mean I should add another DT node that groups all ports? such as:
> > compatible = "ti,tps23861"; ports { port@0 {...} port@1 {...} }
>
>
> Yes.
Except this is not an OF graph. Don't re-use it when it is not that.
Maybe 'poe-port@'? Is multiple ports/channels something common on PoE
chips? I'd guess so. If so, then come up with something common.
Whether you should have a container node like 'ports' is a separate
question. You get exactly 1 address space for any given node. So if you
ever might need to address multiple disjoint things, then you probably
want a container node.
Rob
On Sun, Aug 17, 2025 at 09:23:09AM +0200, Krzysztof Kozlowski wrote:
> On 13/08/2025 05:00, Gregory Fuchedgi wrote:
> > On Tue, Aug 12, 2025 at 12:20 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>> + shutdown-gpios:
> >> powerdown-gpios, see gpio-consumer-common.yaml
> > It is called shutdown in the datasheet, but seems like neither powerdown nor
> > shutdown truly reflects its purpose. This pin doesn't power down the controller
> > itself. It shuts down the ports while keeping the controller available for
> > configuration over i2c. Should I call it ti,ports-shutdown-gpios or maybe
> > ti,shutdown-gpios? Any other suggestions?
> Feels more like enable-gpios.
Wouldn't that be confusing, since there's no enable pin in the datasheet? Also
it doesn't enable/disable the controller itself, but its ports.
In my mind ti,ports-shutdown-gpios is the most meaningful name for it. That said
I appreciate the guidance, since I do not know what's the usual way to do this?
Happy to go with enable-gpios if that's the convention.
On Mon, Aug 18, 2025 at 10:31 AM Rob Herring <robh@kernel.org> wrote:
> > >>> +patternProperties:
> > >>> + "^port@[0-3]$":
> > >> This goes to ports property.
> > > Do you mean I should add another DT node that groups all ports? such as:
> > > compatible = "ti,tps23861"; ports { port@0 {...} port@1 {...} }
> > Yes.
> Except this is not an OF graph. Don't re-use it when it is not that.
> Maybe 'poe-port@'? Is multiple ports/channels something common on PoE
> chips? I'd guess so. If so, then come up with something common.
poe-port@ sounds good to me. When you say come up with something common, does
that imply adding it to a new file, like bindings/hwmon/poe-common.yaml? Or just
using poe-port in this dt without the parent ports node?
> Whether you should have a container node like 'ports' is a separate
> question. You get exactly 1 address space for any given node. So if you
> ever might need to address multiple disjoint things, then you probably
> want a container node.
I do not want to address anything else in this case, so I'd keep it simple.
But let me know if I'm missing any important details.
Appreciate the guidance.
© 2016 - 2025 Red Hat, Inc.