Re: [PATCH net-next 21/37] dt-bindings: net: can: convert grcan to DT schema

[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] This patch deletes
  Documentation/devicetree/bindings/net/can/grcan.txt.
- [Low] The binding description (aeroflexgaisler,grcan.yaml lines 18-20)
  and the commit message both say the node name "<core…

--

dt-bindings: net: can: convert grcan to DT schema

This converts the free-form grcan.txt binding to a DT schema. The nodes
are built by the PROM and have no compatible string, so the schema selects
on the node name. It also moves systemid into the description, because
that property is read from /ambapp0 and not from the controller node.

Link: https://patch.msgid.link/20260929073703.2748220-2-quchaosheng000406@163.com

> diff --git a/Documentation/devicetree/bindings/net/can/aeroflexgaisler,grcan.yaml b/Documentation/devicetree/bindings/net/can/aeroflexgaisler,grcan.yaml
> new file mode 100644
> index 000000000000..d5cba5b1bac1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/aeroflexgaisler,grcan.yaml
> @@ -0,0 +1,65 @@

[ ... ]

> +  On a LEON SPARC system there are no device tree source files. The nodes
> +  are built by the PROM from the AMBA plug&play information, which does not
> +  provide a compatible string. The driver therefore matches on the node
> +  name, which is built as "<core name>@<irq>,<address>" from the "name" and
> +  "reg" properties of the AMBA plug&play core.

[Severity: Low]
Does this describe the source of the <irq> field correctly? In
ambapp_path_component() in arch/sparc/kernel/prom_32.c, that field comes
from the first cell of the "interrupts" property, not from "reg":

arch/sparc/kernel/prom_32.c:ambapp_path_component() {
    ...
	prop = of_find_property(dp, "reg", NULL);
	if (!prop) {
		reg0 = (unsigned int)dp->phandle;
	} else {
		regs = prop->value;
		reg0 = regs->phys_addr;
	}

	/* Not all cores have Interrupt */
	prop = of_find_property(dp, "interrupts", NULL);
	if (!prop)
		intr = &interrupt; /* IRQ0 does not exist */
	else
		intr = prop->value;

	sprintf(tmp_buf, "%s@%x,%x", name, *intr, reg0);
}

Only <address> comes from "reg", and it falls back to the phandle when
"reg" is missing.

The commit message uses the same wording:

  The node name format is "<core name>@<irq>,<address>", as built by
  ambapp_path_component() in arch/sparc/kernel/prom_32.c from the "name"
  and "reg" properties of the AMBA plug&play core.

Could both places say the name is built from the "name", "interrupts" and
"reg" properties? The example and the select pattern already match the
real behaviour.

[ ... ]

> diff --git a/Documentation/devicetree/bindings/net/can/grcan.txt b/Documentation/devicetree/bindings/net/can/grcan.txt
> deleted file mode 100644
> index 34ef3498f887..000000000000
> --- a/Documentation/devicetree/bindings/net/can/grcan.txt
> +++ /dev/null
> @@ -1,28 +0,0 @@
> -Aeroflex Gaisler GRCAN and GRHCAN CAN controllers.

[ ... ]

[Severity: Low]
At this commit grcan.txt is deleted, but the header comment in
drivers/net/can/grcan.c still refers to it:

drivers/net/can/grcan.c:
 * See "Documentation/devicetree/bindings/net/can/grcan.txt" for information on
 * open firmware properties.

The commit message says the driver comment is updated by a separate patch.
The follow-up "can: grcan: update the binding file reference in the driver
comment" points it at aeroflexgaisler,grcan.yaml, so the stale reference
is fixed by the end of the series.

Could that one-line change be folded into this patch? Then the reference
would not dangle at this commit, or if this patch is backported without
its companion.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015015336.816543-2-mkl%40pengutronix.de