Re: [PATCH 2/2] arm64: dts: ti: Add support for the phyCORE-AM67x

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The PCF8574 GPIO expanders specify `IRQ_TYPE_LEVEL_LOW`, which is unsupported by the K3 GPIO controller and will cause probe failure.
- [Medium] The `reset-gpios` for `pcie0_rc` is specified as `GPIO_ACTIVE_HIGH`, which inverses the standard PCIe PERST# active-low logic.
- [Medium] Mismatch between `mcasp1` serializer configuration and its pin multiplexing: AXR2 is configured for RX but is not pinned out.
- [Medium] Button 'key-menu' connected to PCF8574 uses GPIO_ACTIVE_HIGH, causing inverted logic.
- [Low] Typo in the micbias voltage property for the `tlv320aic3110` audio codec.
--

commit 736b51d7a471b4b2aa4f1281a1e87be6e988051c
Author: Nathan Morrisson <nmorrisson@phytec.com>

arm64: dts: ti: Add support for the phyCORE-AM67x

Add support for the PHYTEC phyCORE-AM67x SoM and the corresponding
phyBOARD-Rigel carrier board. The phyCORE-AM67x SoM uses the TI AM67x SoC
and can come with different sizes and models of DDR, eMMC, and SPI NOR
Flash.

Link: https://www.phytec.com/product/phycore-am67x/
Link: https://www.phytec.com/product/phyboard-am67x-development-kit/

> diff --git a/arch/arm64/boot/dts/ti/k3-am6754-phyboard-rigel.dts b/arch/arm64/boot/dts/ti/k3-am6754-phyboard-rigel.dts
> new file mode 100644
> index 0000000000000..7853d4f5d3b95
> --- /dev/null
> +++ b/arch/arm64/boot/dts/ti/k3-am6754-phyboard-rigel.dts
[ ... ]
> +	keys {
> +		compatible = "gpio-keys";
> +		autorepeat;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&gpio_keys_pins_default>;
[ ... ]
> +		key-menu {
> +			label = "menu";
> +			linux,code = <KEY_MENU>;
> +			gpios = <&gpio_exp1 4 GPIO_ACTIVE_HIGH>;

[Severity: Medium]
Does this configuration cause the button to register as permanently pressed?

The key-menu button is connected to gpio_exp1, which is a PCF8574 expander. 
These pins have internal weak pull-ups, meaning their default resting state 
is HIGH, and buttons typically pull them to GND (LOW). Specifying 
GPIO_ACTIVE_HIGH inverses the logical state unless a strong external 
pull-down resistor is present.

> +		};
> +	};
[ ... ]
> +&main_pmx0 {
[ ... ]
> +	main_mcasp1_pins_default: main-mcasp1-default-pins {
> +		pinctrl-single,pins = <
> +			J722S_IOPAD(0x0090, PIN_INPUT, 2)	/* (P27) GPMC0_BE0n_CLE.MCASP1_ACLKX */
> +			J722S_IOPAD(0x0098, PIN_INPUT, 2)	/* (V21) GPMC0_WAIT0.MCASP1_AFSX */
> +			J722S_IOPAD(0x008c, PIN_OUTPUT, 2)	/* (N23) GPMC0_WEn.MCASP1_AXR0 */
> +		>;
> +	};

[Severity: Medium]
Will audio capture on the mcasp1 interface fail here because the data pin
is not connected?

The mcasp1 node below configures serial-dir index 2 to RX, enabling
MCASP1_AXR2. However, the corresponding pin configuration for MCASP1_AXR2
is missing from main_mcasp1_pins_default. The pin remains in its default
state, meaning the controller receives no data.

[ ... ]
> +&main_i2c1 {
[ ... ]
> +	audio_codec: audio-codec@18 {
> +		compatible = "ti,tlv320aic3110";
> +		reg = <0x18>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&audio_ext_refclk1_pins_default>;
> +		#sound-dai-cells = <0>;
> +		ai3xx-micbias-vg = <2>;

[Severity: Low]
Is there a typo in the micbias voltage property name?

The ti,tlv320aic3110 codec driver expects the property ai31xx-micbias-vg.
Using ai3xx-micbias-vg causes the driver to ignore this configuration and
fall back to the default 2.0V bias, which could cause incorrect microphone
gain or distortion.

> +		reset-gpios = <&gpio_exp1 7 GPIO_ACTIVE_LOW>;
[ ... ]
> +	gpio_exp0: gpio@20 {
> +		compatible = "nxp,pcf8574";
> +		reg = <0x20>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&gpio_exp0_int_pins_default>;
> +		interrupt-parent = <&main_gpio0>;
> +		interrupts = <21 IRQ_TYPE_LEVEL_LOW>;

[Severity: High]
Will this expander fail to probe due to an unsupported interrupt type?

The K3 GPIO controller driver (gpio-davinci.c) strictly supports only
edge-triggered interrupts and returns -EINVAL for level types in its
irq_set_type callback. This would prevent the expanders and all attached
devices from probing.

> +		gpio-line-names = "CSI3_STROBE", "CSI3_TRIGGER",
[ ... ]
> +&pcie0_rc {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&main_pcie_pins_default>;
> +	num-lanes = <1>;
> +	phys = <&serdes1_pcie_link>;
> +	phy-names = "pcie-phy";
> +	reset-gpios = <&main_gpio0 31 GPIO_ACTIVE_HIGH>;

[Severity: Medium]
Could this cause the PCIe endpoint to fail enumeration by keeping the
reset line asserted?

The standard PCIe PERST# signal is active-low. Specifying GPIO_ACTIVE_HIGH
causes the host controller to invert this logic, driving the line high when
it intends to assert reset, and low when it releases it. Unless the board
has a physical inverter on this line, the endpoint will remain in reset.

> +	status = "okay";
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625160214.4001298-1-nmorrisson@phytec.com?part=2