Re: [PATCH 3/4] arm: dts: sunplus: add Tibbo LTPP3G2 board

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] i2c: open-drain bus stuck LOW due to hardware output inversion
- [Medium] pinctrl: requested RTS output inversion is silently ignored for PMUX pins
- [Low] dts: hardcoded OS-specific root partition in bootargs
- [Low] dts: non-generic device tree node names for I2C and LEDs
- [Low] dts: unused pinctrl definition for UART3 (dead code)
- [Low] dt-bindings: missing compatible string documentation for tibbo,ltpp3g2
--

commit 10e32790a2e05cfde60226acde658b3dc19b67ab
Author: Andrew Gaylard <ag@ffroot.co.za>
Subject: arm: dts: sunplus: add Tibbo LTPP3G2 board

This adds a device tree for the Tibbo LTPP3G2 board based on the Sunplus
SP7021 SoC. The hardware description includes eMMC, SPI, bit-banged I2C
EEPROM, PWM, a watchdog, RTC, and two GPIO LEDs.

> diff --git a/arch/arm/boot/dts/sunplus/sunplus-sp7021-ltpp3g2.dts b/arch/arm/boot/dts/sunplus/sunplus-sp7021-ltpp3g2.dts
> new file mode 100644
> index 0000000000000..f91b0de79c3d0
> --- /dev/null
> +++ b/arch/arm/boot/dts/sunplus/sunplus-sp7021-ltpp3g2.dts
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree Source for Tibbo LTPP board G2
> + *
> + * Copyright (C) 2021 Sunplus Technology Co.
> + */
> +
> +/dts-v1/;
> +
> +#include "sunplus-sp7021-achip.dtsi"
> +
> +/ {
> +	compatible = "tibbo,ltpp3g2", "sunplus,sp7021";

[Severity: Low]
Is the tibbo,ltpp3g2 compatible string documented in the YAML bindings?
Running dtbs_check will likely fail since this board string does not
appear to be documented.

[ ... ]

> +	chosen {
> +		bootargs = "console=ttySUP0,115200 earlycon root=/dev/mmcblk0p8 rootwait";

[Severity: Low]
Does hardcoding root=/dev/mmcblk0p8 tie the device tree to a highly specific
OS partitioning scheme? Booting a standard kernel image that expects the root
filesystem on a different partition will fail to mount root without overriding
bootargs. Could this be removed to better separate the hardware description
from software configuration?

[ ... ]

> +	i2c_tps: i2c-tps { /* eeprom */

[Severity: Low]
Does the i2c-tps node name violate standard Devicetree naming schemas?
Node names should typically be generic, such as i2c.

[ ... ]

> +&leds {
> +	pinctrl-names = "default", "sleep";
> +	pinctrl-0 = <&gpio_leds_pins>;
> +	pinctrl-1 = <&leds_s0_slp>;
> +	system-led {

[Severity: Low]
Should this node use a generic name like led-0 to comply with standard
validation schemas?

[ ... ]

> +&uart3 {
> +	status = "disabled";
> +};

[Severity: Low]
Should uart3 claim the pins_uart3 pinctrl configuration here, similar to
uart1, uart2, and uart4? If a user enables uart3 via an overlay, the
hardware pins will remain unconfigured.

[ ... ]

> +	pins_i2ctps: pinmux_i2ctps-pins {
> +		sunplus,pins = <
> +			SPPCTL_IOPAD(1, SPPCTL_PCTL_G_GPIO, 0,
> +				     SPPCTL_PCTL_L_OUT | SPPCTL_PCTL_L_ONV | SPPCTL_PCTL_L_ODR)
> +			SPPCTL_IOPAD(2, SPPCTL_PCTL_G_GPIO, 0,
> +				     SPPCTL_PCTL_L_OUT | SPPCTL_PCTL_L_ONV | SPPCTL_PCTL_L_ODR)
> +		>;
> +	};

[Severity: High]
Will applying SPPCTL_PCTL_L_ONV to these pins break open-drain signaling
and persistently hold the bus LOW?

When the i2c-gpio driver requests the bus to float HIGH by writing 1, the
SPPCTL_PCTL_L_ONV configuration bit hardware-inverts this 1 into a 0,
pulling the open-drain line strongly LOW.

Should SPPCTL_PCTL_L_OU1 be used instead of combining SPPCTL_PCTL_L_OUT
with SPPCTL_PCTL_L_ONV to set an initial HIGH output without breaking logic?

[ ... ]

> +	/* AP6256 Bluetooth */
> +	pins_btuart: pinmux_btuart-pins {
> +		sunplus,pins = <
> +			SPPCTL_IOPAD(40, SPPCTL_PCTL_G_PMUX, MUXF_UA4_RX, 0)
> +			SPPCTL_IOPAD(41, SPPCTL_PCTL_G_PMUX, MUXF_UA4_TX, 0)
> +			SPPCTL_IOPAD(56, SPPCTL_PCTL_G_PMUX, MUXF_UA4_RTS, SPPCTL_PCTL_L_ONV)

[Severity: Medium]
Is the SPPCTL_PCTL_L_ONV inversion request actually taking effect for this
PMUX pin?

Looking at the SP7021 pinctrl driver in sppctl_dt_node_to_map(), it generates
a PIN_MAP_TYPE_MUX_GROUP for SPPCTL_PCTL_G_PMUX pin types and explicitly
discards configuration bits.

As a result, the requested inversion will be silently ignored, potentially
causing UART flow control failure if the AP6256 module actually requires an
inverted signal.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917082632.626614-1-ag@ffroot.co.za?part=3