Re: [PATCH 2/2] arm64: tegra: Add Lenovo ThinkEdge SE70 carrier board DTS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Peripheral overrides and configuration nodes are incorrectly placed at the root of the device tree, failing to override the actual bus children.
- [High] The `fixed-regulators` container lacks the `compatible = "simple-bus"` property, preventing its children from probing.
- [High] The `cd-inverted` property combined with `GPIO_ACTIVE_LOW` causes a double-inversion of the SD card detect signal.
- [Medium] Use of downstream Linux-for-Tegra (L4T) device tree properties that are invalid in mainline.
- [Low] Device tree node names use underscores instead of hyphens.
- [Low] Mismatch between inline code comment and regulator voltage configuration.
--

commit ce1b543209b572c99dba284d418dec1c8ce39718
Author: Jiqi Li <lijq9@lenovo.com>

arm64: tegra: Add Lenovo ThinkEdge SE70 carrier board DTS

ThinkEdge SE70 is a fanless industrial edge gateway built around
NVIDIA Tegra194 Xavier NX SOM. This patch adds a standalone device
tree file describing core carrier board peripherals:
- Custom 40-pin header pinmux configuration
- External SD card slot with dedicated 3.3V fixed regulator

No fan, PWM, tachometer, extra camera/spi peripherals are present
on this passively cooled platform, so unused nodes are explicitly
disabled following mainline device tree best practices.

Static verification passed: dt_binding_check and dtbs compilation
complete without errors.

[ ... ]

> diff --git a/arch/arm64/boot/dts/nvidia/tegra194-lenovo-thinkedge-se70.dts b/arch/arm64/boot/dts/nvidia/tegra194-lenovo-thinkedge-se70.dts
> new file mode 100644
> index 0000000000000..075e057c90954
> --- /dev/null
> +++ b/arch/arm64/boot/dts/nvidia/tegra194-lenovo-thinkedge-se70.dts
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/dts-v1/;
> +
> +#include "tegra194-p3668-0001.dtsi"
> +#include "tegra194-p3509-0000.dtsi"
> +
> +/ {
> +	model = "Lenovo ThinkEdge SE70";
> +	compatible = "lenovo,thinkedge-se70", "nvidia,p3509-0000+p3668-0001", "nvidia,tegra194";
> +
> +	chosen {
> +		bootargs = "console=ttyTCU0,115200";
> +	};
> +
> +	/* Fixed 2.3V regulator for external SD card slot */
[Severity: Low]
Is this comment accurate? It mentions a 2.3V regulator, but the configuration
properties below (regulator-min-microvolt and regulator-max-microvolt) enforce
3.3V.

> +	fixed-regulators {
[Severity: High]
Does this fixed-regulators container need a compatible = "simple-bus" property?
Without a recognized bus compatible string, of_platform_populate() won't
recurse into this node at boot. This would prevent the ap2306gn_3v3_sd child
regulator from probing, causing the SDHCI controller requesting it to fail with
-EPROBE_DEFER.

> +		ap2306gn_3v3_sd: ap2306gn_3v3_sd {
[Severity: Low]
Should this node name use hyphens instead of underscores (e.g.,
ap2306gn-3v3-sd) to comply with Devicetree specification naming rules and avoid
node_name_chars warnings during compilation?

> +			compatible = "regulator-fixed";
> +			regulator-name = "ap2306gn-3v3-sd";
> +			regulator-min-microvolt = <3300000>;
> +			regulator-max-microvolt = <3300000>;

[ ... ]

> +	/* Custom pinmux configurations for 40-pin expansion header */
> +	pinmux@c302000 {
[Severity: High]
Are these peripheral and pinmux configuration nodes incorrectly placed at the
root level of the device tree (/ {}) rather than being children of the SoC bus?
Placing nodes like pinmux@..., i2c@..., and sdhci@... directly under the root
creates disconnected dummy nodes instead of overriding the actual bus-attached
controllers, causing these intended configurations to be completely ignored.

> +		status = "okay";
> +		touch_clk_pcc4 {

[ ... ]

> +	pinmux@2430028 {
> +		status = "okay";
> +		soc_gpio41_pq5 {
> +			nvidia,pins = "soc_gpio41_pq5";
> +			nvidia,function = "rsvd2";
> +			nvidia,pull = <TEGRA_PIN_PULL_NONE>;
> +			nvidia,tristate = <TEGRA_PIN_DISABLE>;
> +			nvidia,enable-input = <TEGRA_PIN_DISABLE>;
> +			nvidia,lpdr = <TEGRA_PIN_DISABLE>;
> +			nvidia,io-high-voltage = <TEGRA_PIN_DISABLE>;
[Severity: Medium]
Are nvidia,lpdr and nvidia,io-high-voltage downstream Linux-for-Tegra (L4T)
properties? They don't appear to exist in the mainline devicetree bindings,
which means they will be ignored by upstream drivers and fail dt_binding_check
schema validation.

> +		};
> +	};

[ ... ]

> +	/* SDMMC3 for external user SD card slot with dedicated 3.3V power */
> +	sdhci@3440000 {
[Severity: High]
Similar to the pinmux nodes above, is this sdhci node placed as a direct child
of the root (/ {}) instead of the SoC bus? This creates a new orphaned node
rather than overriding the actual bus-attached SDHCI controller.

> +		uhs-mask = <0x78>;
> +		mmc-ocr-mask = <0x0>;
[Severity: Medium]
Are uhs-mask and mmc-ocr-mask valid mainline properties? Like the pinmux
properties above, these appear to be downstream L4T properties that will fail
dt_binding_check schema validation.

> +		cd-inverted;
> +		cd-gpios = <&gpio_aon TEGRA194_AON_GPIO(CC, 4) GPIO_ACTIVE_LOW>;
[Severity: High]
Does combining the cd-inverted property with GPIO_ACTIVE_LOW in cd-gpios cause
a logical double inversion of the SD card detect signal? The GPIO subsystem
will invert the active-low physical signal, and then the MMC core will invert
that result again due to cd-inverted. This could mean an inserted card is seen
as removed and vice versa, breaking card hotplugging.

> +		nvidia,cd-wakeup-capable;
[Severity: Medium]
Is nvidia,cd-wakeup-capable another downstream property unsupported in
mainline?

> +		vmmc-supply = <&ap2306gn_3v3_sd>;
> +		status = "okay";
> +	};

[ ... ]

> +	pwm@32f0000 {
> +		status = "disabled";
> +	};
> +	generic_pwm_tachometer {
[Severity: Low]
Should this node name use hyphens instead of underscores (e.g.,
generic-pwm-tachometer) to avoid devicetree specification violations and
node_name_chars warnings?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626010950.459899-1-lijq9@lenovo.com?part=2