Re: [PATCH 2/2] arm: sunplus: add Tibbo LTPP3G2 board support; update defconfig

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Medium] The `rtc` node uses a non-existent `charging-mode` property that is unsupported by the driver and violates device tree bindings.
- [Medium] The EEPROM node uses an invalid compatible string `at,24c16` instead of `atmel,24c16`.
- [Medium] The `CONFIG_I2C_GPIO` option is not enabled in `sp7021_defconfig`, which is required for the `i2c_tps` bus to function.
- [Medium] The board DTS creates a new root `led` node rather than overriding the SoC's `&leds` node, leading to duplicate `gpio-leds` instances.
- [Low] Invalid properties `spi-cpha` and `spi-cpol` applied directly to SPI controller nodes.
- [Low] The `i2c_tps` node uses deprecated properties `gpios`, `i2c-gpio,scl-open-drain`, and `i2c-gpio,sda-open-drain`, violating the `i2c-gpio` schema.
- [Low] The `led@2` node uses a unit address in its name but lacks a `reg` property.
--

commit 21c4fb2cd185ac71c6b0d8d79e82b451794aa5c2
Author: Andrew Gaylard <ag@ffroot.co.za>

arm: sunplus: add Tibbo LTPP3G2 board support; update defconfig

This commit adds a device tree source for the Tibbo LTPP3G2 board based on
the SP7021 SoC. It includes node configurations for various peripherals like
I2C, SPI, RTC, and Ethernet. It also updates the defconfig to enable the
necessary options for the board to boot correctly.

> 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..c42e6f779ce36
> --- /dev/null
> +++ b/arch/arm/boot/dts/sunplus/sunplus-sp7021-ltpp3g2.dts
> @@ -0,0 +1,301 @@

[ ... ]

> +	led {
> +		compatible = "gpio-leds";

[Severity: Medium]
Does creating a new root "led" node here rather than overriding the SoC's
existing "&leds" node from sunplus-sp7021.dtsi create duplicate gpio-leds
instances? This could result in an led subsystem sysfs naming conflict and
potential issues with the SoC's default floating pin mapping, since both
nodes contain a "system-led" subnode.

> +		pinctrl-names = "default", "sleep";
> +		pinctrl-0 = <&gpio_leds_pins>;
> +		pinctrl-1 = <&leds_s0_slp>;
> +		system-led {
> +			label = "system-led";
> +			gpios = <&pctl 6 GPIO_ACTIVE_HIGH>;
> +			default-state = "off";
> +			linux,default-trigger = "heartbeat";
> +		};
> +		led@2 {

[Severity: Low]
Does this node violate the Devicetree Specification by including a unit
address ("@2") without a corresponding "reg" property? This will trigger a
unit_address_vs_reg compiler warning.

> +			label = "SG";
> +			gpios = <&pctl 7 GPIO_ACTIVE_HIGH>;
> +			linux,default-trigger = "heartbeat";
> +			default-state = "off";
> +		};
> +	};
> +
> +	i2c_tps: i2c-tps { /* eeprom */
> +		compatible = "i2c-gpio";
> +		i2c-gpio,scl-open-drain;
> +		i2c-gpio,sda-open-drain;
> +		i2c-gpio,delay-us = <1>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pins_i2ctps>;
> +		gpios = <&pctl 2 GPIO_ACTIVE_HIGH  /* SDA */
> +			 &pctl 1 GPIO_ACTIVE_HIGH  /* SCL */
> +			>;

[Severity: Low]
Is this node using deprecated properties for the i2c-gpio binding? The
schema requires "sda-gpios" and "scl-gpios", whereas using "gpios" alongside
the boolean open-drain flags triggers dtbs_check validation errors.

> +		eeprom: eeprom@50 {
> +			compatible = "at,24c16";

[Severity: Medium]
Is "at,24c16" an invalid compatible string for the Atmel EEPROM? The correct
vendor prefix for Atmel is "atmel". Using this non-existent string could
prevent the at24 EEPROM driver from matching and probing, making the device
inaccessible.

> +			reg = <0x50>;
> +			pagesize = <16>;
> +		};
> +	};
> +};

[ ... ]

> +&spi_controller0 {
> +	spi-cpha;
> +	spi-cpol;

[Severity: Low]
Are these slave-specific flags incorrectly placed on the master controller
node instead of the peripheral nodes? While the SPI controller driver ignores
them on the controller node, this violates device tree schemas and causes
dtbs_check validation errors.

> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pins_spi0>;
> +	status = "disabled";
> +};
> +
> +&spi_controller1 {
> +	spi-cpha;
> +	spi-cpol;

[Severity: Low]
Are these slave-specific flags also incorrectly placed on this master
controller node?

> +	status = "disabled";
> +};
> +
> +&spi_controller2 {
> +	spi-cpha;
> +	spi-cpol;

[Severity: Low]
Are these slave-specific flags also incorrectly placed on this master
controller node?

> +	status = "disabled";
> +};

[ ... ]

> +&rtc {
> +	charging-mode = <0xE>;

[Severity: Medium]
Is "charging-mode" a non-existent legacy property? This violates the schema
and is unsupported by the driver, silently failing to configure the battery
trickle charger hardware, which could prevent the RTC battery from charging.

> +	/*
> +	 *      0xE     Disable
> +	 *      0x1     0.86mA (2K Ohm with diode)
> +	 *      0x5     1.81mA (250 Ohm with diode)

[ ... ]

> diff --git a/arch/arm/configs/sp7021_defconfig b/arch/arm/configs/sp7021_defconfig
> index ec723401b4405..e0c0692f455de 100644
> --- a/arch/arm/configs/sp7021_defconfig
> +++ b/arch/arm/configs/sp7021_defconfig

[ ... ]

> @@ -36,6 +45,25 @@ CONFIG_INPUT_EVDEV=y
>  # CONFIG_LEGACY_PTYS is not set
>  # CONFIG_HW_RANDOM is not set
>  # CONFIG_HWMON is not set
> +CONFIG_SPI=y
> +CONFIG_SPI_SUNPLUS_SP7021=y
> +CONFIG_I2C=y
> +CONFIG_I2C_CHARDEV=y
> +CONFIG_EEPROM_AT24=y

[Severity: Medium]
Is CONFIG_I2C_GPIO missing from this configuration? Since the board adds an
EEPROM on the i2c_tps bus using the "i2c-gpio" compatible, failing to enable
CONFIG_I2C_GPIO (which is a tristate and not selected by default) will
prevent the bus from probing and break access to the EEPROM.

> +CONFIG_MMC=y
> +CONFIG_MMC_SUNPLUS=y

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