* Re: [PATCH net-next 08/12] dt-bindings: net: toshiba,tc965x-dwmac: add TC956x Ethernet bridge
[not found] ` <20260502155615.AF378C19425@smtp.kernel.org>
@ 2026-05-07 19:02 ` Rob Herring
2026-05-08 14:36 ` Alex Elder
0 siblings, 1 reply; 2+ messages in thread
From: Rob Herring @ 2026-05-07 19:02 UTC (permalink / raw)
To: sashiko; +Cc: Alex Elder, bpf
On Sat, May 02, 2026 at 03:56:15PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] The binding adds GPIO provider capabilities but omits `gpio-controller` and `#gpio-cells` from the `required` list.
> - [Medium] The Ethernet endpoint is incorrectly modeled as a PCI bridge, applying bus properties to an endpoint.
> --
Please answer sashiko reports because if maintainers are, it's just more
work for us. While agreeing with maintainer comments don't need a reply,
I think we're still at the point any AI review does. Though sashiko is
2/2 here.
>
> commit c5a5d0d7a89f5ad66c878c805902efc5e0bd117a
> Author: Daniel Thompson <daniel@riscstar.com>
>
> dt-bindings: net: toshiba,tc965x-dwmac: add TC956x Ethernet bridge
>
> This commit adds devicetree bindings for the Toshiba TC956x family of
> Ethernet-AVB/TSN bridges.
>
> > diff --git a/Documentation/devicetree/bindings/net/toshiba,tc956x-dwmac.yaml b/Documentation/devicetree/bindings/net/toshiba,tc956x-dwmac.yaml
> > new file mode 100644
> > index 000000000000..d95d22a3761d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/toshiba,tc956x-dwmac.yaml
> > @@ -0,0 +1,111 @@
>
> [ ... ]
>
> > +allOf:
> > + - $ref: /schemas/pci/pci-bus-common.yaml#
>
> Is it correct to reference pci-bus-common.yaml# here? Since the tc956x_emac0
> node is a PCI endpoint (an Ethernet controller attached to an internal PCIe
> switch) rather than a PCI bus or bridge, applying bridge-specific properties
> could be semantically invalid.
Correct, should be dropped.
>
> > + - $ref: /schemas/pci/pci-device.yaml#
> > +
> > +unevaluatedProperties: false
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - pci1179,0220 # Toshiba TC9564 (a.k.a. Qualcomm QPS615)
> > +
> > + "#gpio-cells":
> > + const: 2
> > +
> > + gpio-controller: true
>
> [ ... ]
>
> > +required:
> > + - compatible
> > + - reg
>
> Should gpio-controller and #gpio-cells be explicitly included in this
> required list?
>
> The binding defines gpio-controller and #gpio-cells above, indicating the
> hardware acts as a GPIO provider. Omitting them from the required list could
> allow incomplete device tree nodes to silently pass schema validation.
>
> > +examples:
> > + - |
> > + pcie {
> > + #address-cells = <3>;
> > + #size-cells = <2>;
> > +
> > + tc956x_emac0: pci@0,0 {
> > + compatible = "pci1179,0220";
> > + reg = <0x50000 0x0 0x0 0x0 0x0>;
> > + #address-cells = <3>;
> > + #size-cells = <2>;
> > + device_type = "pci";
> > + ranges;
>
> Does this endpoint node need bridge properties like device_type = "pci",
> #address-cells = <3>, #size-cells = <2>, and ranges?
No, it doesn't.
> The child of this node is an mdio bus, which does not use PCI addresses and
> does not require PCI bridge translation. Using device_type = "pci" on an
> endpoint might confuse OS PCI enumeration logic, which expects nodes with
> this device_type to be actual PCI-to-PCI bridges.
^ permalink raw reply [flat|nested] 2+ messages in thread