Re: [PATCH] arm64: dts: renesas: sparrow-hawk: Add overlay for WaveShare 2CH CANFD HAT

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Potential CAN controller freeze due to edge-triggered interrupt configuration for active-low level-sensitive INT pin.
- [Low] Devicetree schema violation with non-standard node names for clock and SPI nodes.
--

--- Patch [1]: [PATCH] arm64: dts: renesas: sparrow-hawk: Add overlay for WaveShare 2CH CANFD HAT ---
commit b5cd8b12c94510c91e67da4257339818760852d2
Author: Marek Vasut <[email protected]>

arm64: dts: renesas: sparrow-hawk: Add overlay for WaveShare 2CH CANFD HAT

Add a DT overlay to bind the WaveShare 2-Channel Isolated CAN FD Expansion
HAT on the Retronix R-Car V4H Sparrow Hawk board.

> diff --git a/arch/arm64/boot/dts/renesas/r8a779g3-sparrow-hawk-ws-2ch-canfd.dtso b/arch/arm64/boot/dts/renesas/r8a779g3-sparrow-hawk-ws-2ch-canfd.dtso
> new file mode 100644
> index 0000000000000..de72680efa7c6
> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/r8a779g3-sparrow-hawk-ws-2ch-canfd.dtso
[ ... ]
> +&{/} {
> +	/* Fixed clock dedicated to SPI CAN on 2-CH CAN FD HAT */
> +	clk_can0: clk-ws-2ch-canfd0 {

[Severity: Low]
Does this node name violate the fixed-clock devicetree bindings schema?
The schema typically mandates node names matching the pattern clock-* to pass
dtbs_check validation.

> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <40000000>;
> +	};
> +
> +	clk_can1: clk-ws-2ch-canfd1 {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <40000000>;
> +	};
> +
> +	/*
> +	 * SPI_1 is operated as GPIO SPI, because MSIOF5 can not be
> +	 * muxed on the pins allocated for SPI_1 on this CANFD HAT.
> +	 */
> +	spi-ws-2ch-canfd {	/* SPI_1 */

[Severity: Low]
Is this non-generic node name likely to trigger devicetree validation
warnings? Standard devicetree practices recommend using the generic spi node
name rather than spi-ws-2ch-canfd.

> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "spi-gpio";
> +		sck-gpios = <&gpio0 10 0>;		/* CN7 pin 40 */
> +		miso-gpios = <&gpio0 9 0>;		/* CN7 pin 35 */
> +		mosi-gpios = <&gpio0 12 0>;		/* CN7 pin 38 */
> +		cs-gpios = <&gpio2 13 GPIO_ACTIVE_LOW>;	/* CN7 pin 12 */
> +		num-chipselects = <1>;
> +
> +		can@0 {
> +			compatible = "microchip,mcp2518fd";
> +			clocks = <&clk_can1>;
> +			/* CN7 pin 18 */
> +			interrupts-extended = <&gpio1 25 IRQ_TYPE_EDGE_FALLING>;

[Severity: High]
Can configuring this interrupt as IRQ_TYPE_EDGE_FALLING cause the CAN
controller to freeze?

The MCP2518FD INT pin is active-low and level-sensitive. If a new interrupt
condition occurs while the INT pin is already asserted low, a new falling
edge might not be generated, causing the new interrupt to be permanently
missed. Should this use IRQ_TYPE_LEVEL_LOW instead?

> +			reg = <0>;
> +			spi-max-frequency = <20000000>;
> +		};
> +	};
> +};
> +
> +&msiof0 {	/* SPI_0 */
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	pinctrl-0 = <&msiof0_pins>;
> +	pinctrl-names = "default";
> +	status = "okay";
> +
> +	/*
> +	 * Native MSIOF chipselect does not work with MCP2518FD
> +	 * because MCP251xFD driver needs to keep the chipselect
> +	 * line asserted between transfers. This requirement is
> +	 * fulfilled only by using GPIO chipselect.
> +	 */
> +	cs-gpios = <&gpio1 8 GPIO_ACTIVE_LOW>;
> +
> +	can@0 {
> +		compatible = "microchip,mcp2518fd";
> +		clocks = <&clk_can0>;
> +		interrupts-extended = <&gpio1 26 IRQ_TYPE_EDGE_FALLING>;

[Severity: High]
Similarly, can this edge-triggered configuration for the native MSIOF CAN
controller lead to missed interrupts if the active-low level-sensitive INT
pin is already asserted when a new interrupt arrives?

> +		reg = <0>;
> +		spi-max-frequency = <20000000>;
> +	};
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1