Re: [PATCH net-next v3 1/2] dt-bindings: net: pse-pd: Add bindings for LTC4266 PSE Controller

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The binding does not describe the LTC4266's board-level control
  and supply resources.
- [Low] The commit message for "dt-bindings: net: pse-pd: Add bindings
  for LTC4266 PSE Controller" is one sentence ("Add the LTC4266 Power…
- [Low] The `pse-pi@[0-3]` subschema describes `pairsets` as "exactly one
  channel phandle is expected", and the `channels` description says the…
- [Low] `interrupts` is in the top-level `required` list of
  lltc,ltc4266.yaml (line 94).

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but the base pse-
  controller.yaml (which lltc,ltc4266.yaml inherits) and the LTC4266…

--

dt-bindings: net: pse-pd: Add bindings for LTC4266 PSE Controller

This commit adds a YAML device tree binding for the Linear Technology
LTC4266 PSE controller. The binding describes the chip's four delivery
channels, each with a sense resistor value, and PSE PIs that each map to
one channel.

> dt-bindings: net: pse-pd: Add bindings for LTC4266 PSE Controller
>
> Add the LTC4266 Power Sourcing Equipment controller device tree bindings
> documentation.

[Severity: Low]
This isn't a bug, but the subject says "binding" twice.
Documentation/devicetree/bindings/submitting-patches.rst asks submitters
to avoid that:

  Repeating "binding" again should also be avoided, so for a new device it
  is often enough for example: "dt-bindings: iio: adc: Add ROHM BD79100G"

Could the body also describe the hardware, a 4-port 802.3at PSE on I2C?
Could it also explain the ABI choices that differ from the sibling PSE
bindings? These are:

- a new required per-channel sense-resistor-micro-ohms property with a
  fixed 250000/500000 enum
- channels and pse-pis are both required (ti,tps23881.yaml requires
  neither)
- each PI is limited to a single pairset

> diff --git a/Documentation/devicetree/bindings/net/pse-pd/lltc,ltc4266.yaml b/Documentation/devicetree/bindings/net/pse-pd/lltc,ltc4266.yaml
> new file mode 100644
> index 0000000000000..e11c9d601eee9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/pse-pd/lltc,ltc4266.yaml
> @@ -0,0 +1,180 @@

[ ... ]

> +properties:
> +  compatible:
> +    enum:
> +      - lltc,ltc4266
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1

[Severity: Low]
Should this list also have reset-gpios for the chip's active-low RESET
input? Should it have supply properties for the VDD and VEE rails?

The schema ends with unevaluatedProperties: false. A board DT that routes
RESET to a SoC GPIO, or names its VDD/VEE regulators, would fail
validation. writing-bindings.rst says:

  DO attempt to make bindings complete even if a driver doesn't support
  some features.

The sibling ti,tps23881.yaml in the same directory documents its reset
line:

  reset-gpios:
    maxItems: 1

The driver patch in this series ("net: pse-pd: Add LTC4266 PSE controller
driver") only resets the chip in software, through the RSTPB register in
ltc4266_probe():

  ret = regmap_write(regmap, LTC4266_REG_RSTPB, LTC4266_RSTPB_INTCLR | LTC4266_RSTPB_RSTALL);

Suppose a board's RESET GPIO is asserted by default. Won't the chip stay
in reset, so the ID read in probe fails? Nothing in the DT would let the
driver deassert the line.

[ ... ]

> +    patternProperties:
> +      '^pse-pi@[0-3]$':
> +        type: object
> +        additionalProperties: true
> +        properties:
> +          pairsets:
> +            description: The LTC4266 delivers power to a PI over a single
> +              pairset, driven by one of the controller's four channels. There
> +              is no 4-pair mode spreading a PI over two channels, so exactly
> +              one channel phandle is expected.
> +            maxItems: 1
> +          pairset-names:
> +            maxItems: 1

[Severity: Low]
The description says exactly one channel phandle is expected. Should
pairsets and pairset-names also be marked required in this subschema?

The only required entries inherited from pse-controller.yaml are reg and
#pse-cells. The PSE core's of_load_pse_pis() accepts a pse-pi that has no
pairsets. It takes the -ENOENT branch and leaves pairset[0].np NULL:

  } else if (ret != -ENOENT) {

The driver patch in this series then rejects that case in
ltc4266_map_pis():

  if (!pi->pairset[0].np)
          return dev_err_probe(ltc4266->dev, -EINVAL,
                               "%pOF has no pairsets\n", pi->np);

So there is no NULL dereference. However, a DT with a PI that has no
channel mapping passes dtbs_check and then makes pse_controller_register()
fail. That takes down all four ports.

Would adding this to the pse-pi subschema close the gap?

  required:
    - pairsets
    - pairset-names

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch.
Neither this subschema nor the base pse-controller.yaml requires pairsets
and pairset-names to appear together, for example with dependentRequired.

of_load_pse_pi_pairsets() in drivers/net/pse-pd/pse_core.c checks this at
runtime:

  ret = of_property_count_strings(node, "pairset-names");
  if (ret != npairsets) {
          pr_err(...);
          return -EINVAL;
  }

So a node with pairsets = <&phys0>; and no pairset-names passes schema
validation and then fails controller registration.

ti,tps23881.yaml and skyworks,si3474.yaml have the same gap. Would this be
better fixed once in pse-controller.yaml?

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - channels
> +  - pse-pis

[Severity: Low]
Does interrupts need to be required? The requirement seems to come from
the driver rather than the hardware. Without an IRQ, the driver patch in
this series fails probe in ltc4266_probe():

  if (!client->irq)
          return dev_err_probe(&client->dev, -EINVAL,
                               "Interrupt is required for power budget management\n");

On the LTC4266, INT is an open-drain output that summarises the event and
status registers. Those registers can be read over I2C, so the chip can
also be managed by polling. writing-bindings.rst says:

  - DON'T refer to Linux or "device driver" in bindings. Bindings should
    be based on what the hardware has, not what an OS and driver
    currently support.

A board that leaves INT unconnected is valid hardware, but it would fail
dtbs_check. For comparison, skyworks,si3474.yaml in the same directory
does not require interrupts, while ti,tps23881.yaml does.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927212929.593366-1-kyle.swenson%40est.tech