Re: [PATCH v3] Enable wifi on the BeagleV-Ahead

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The `host-wake` interrupt pin is configured with `input-schmitt-disable`, which makes it susceptible to noise and false interrupts.
- [Medium] The pinctrl for the Wi-Fi `reg-on` GPIO is incorrectly assigned to the SDIO host (`&sdio1`) instead of the power sequence provider (`brcmf_pwrseq`).
- [Low] The power sequence node uses a non-generic name (`brcmf-pwrseq`), violating Devicetree naming guidelines.
--

commit aef7ec6dc80f633bb4abb51e59f66c088fa8d55f
Author: Thomas Gerner <thomas.gerner@muenchen-mail.de>

Enable wifi on the BeagleV-Ahead

This commit enables the AP6203BM WiFi chip on the BeagleV-Ahead board by
adding the necessary device tree nodes. It sets up the SDIO interface,
pin configurations, and power sequence for the wireless module.

> diff --git a/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts b/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
> --- a/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
> +++ b/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
[ ... ]
> @@ -86,6 +86,11 @@ hdmi_con_in: endpoint {
>  			};
>  		};
>  	};
> +
> +	brcmf_pwrseq: brcmf-pwrseq {

This isn't a bug, but should this node use a generic name like wifi-pwrseq
instead of the vendor-specific brcmf-pwrseq? Section 2.2.2 of the Devicetree
Specification suggests node names should reflect the function of the device.

> +		compatible = "mmc-pwrseq-simple";
> +		reset-gpios = <&gpio2 31 GPIO_ACTIVE_LOW>;	/* WL-REG-ON */
> +	};
>  };
[ ... ]
> @@ -239,6 +244,28 @@ rx-pins {
>  			slew-rate = <0>;
>  		};
>  	};
> +
> +	wifi_pins: wifi-0 {
> +		host-wake-pins {
> +			pins = "GPIO2_25";
> +			function = "gpio";
> +			bias-disable;
> +			drive-strength = <1>;
> +			input-enable;
> +			input-schmitt-disable;

Could disabling the Schmitt trigger here cause issues with false interrupts?

The host-wake pin is configured later in this patch as an external,
asynchronous signal triggering on a rising edge (IRQ_TYPE_EDGE_RISING).

Disabling hysteresis removes protection against signal noise and slow
transitions, which might lead to multiple false triggers or interrupt storms
on a single edge.

> +			slew-rate = <0>;
> +		};
> +
> +		reg-on-pins {
> +			pins = "GPIO2_31";
> +			function = "gpio";
> +			bias-disable;
> +			drive-strength = <3>;
> +			input-disable;
> +			input-schmitt-disable;
> +			slew-rate = <0>;
> +		};
> +	};
>  };
[ ... ]
> +&sdio1 {
> +	bus-width = <4>;
> +	max-frequency = <198000000>;
> +	status = "okay";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	non-removable;
> +	keep-power-in-suspend;
> +
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&wifi_pins>;

Does the reg-on-pins pinctrl configuration need to be assigned to the
brcmf_pwrseq node rather than the sdio1 node?

The mmc-pwrseq-simple device is an independent platform device that
requests and drives the reset-gpios (GPIO2_31) during its own probe
sequence.

By keeping the pinctrl assignment here in the sdio1 node, the power
sequencer might attempt to drive the GPIO before the SDIO host has
multiplexed the pin correctly.

> +	mmc-pwrseq = <&brcmf_pwrseq>;
> +
> +	wifi@1 {
> +		compatible = "cypress,cyw43012-fmac", "brcm,bcm4329-fmac";
> +		reg = <1>;
> +		interrupt-parent = <&gpio2>;
> +		interrupts = <25 IRQ_TYPE_EDGE_RISING>;
> +		interrupt-names = "host-wake";
> +	};
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514144836.50101-1-thomas.gerner@muenchen-mail.de?part=1