All of lore.kernel.org
 help / color / mirror / Atom feed
From: sashiko-bot@kernel.org
To: "Jiqi Li" <lijq9@lenovo.com>
Cc: conor+dt@kernel.org, robh@kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH 2/2] arm64: tegra: Add Lenovo ThinkEdge SE70 carrier board DTS
Date: Fri, 26 Jun 2026 03:47:07 +0000	[thread overview]
Message-ID: <20260626034708.3A62F1F000E9@smtp.kernel.org> (raw)
In-Reply-To: <20260626033418.520762-3-lijq9@lenovo.com>

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Pin configurations are instantiated as standalone dummy nodes instead of subnodes of the pinctrl controllers.
- [High] SD card controller override fails due to a node name mismatch (`sdhci` vs `mmc`).
- [High] The pwm-fan status override fails because it is incorrectly nested under bus@0.
- [Medium] Device tree contains dead overrides for non-existent downstream nodes.
- [Medium] The fixed-regulators node incorrectly uses simple-bus with `#address-cells = <1>` for a sizeless child.
--

commit 124a29c4818b88a33b77f6bc336c64c7bd9a06ec
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.

> 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..d44eb7c9b4742
> --- /dev/null
> +++ b/arch/arm64/boot/dts/nvidia/tegra194-lenovo-thinkedge-se70.dts

[ ... ]

> +	/* Fixed 3.3V regulator for external SD card slot */
> +	fixed-regulators {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		ap2306gn_3v3_sd: ap2306gn-3v3-sd {

[Severity: Medium]
The fixed-regulators node is marked as compatible = "simple-bus" and sets
#address-cells = <1>, but its child ap2306gn-3v3-sd does not have a unit
address or a reg property.

Since the simple-bus binding requires children to have a reg or ranges
property, will this cause dtbs_check validation to fail?

[ ... ]

> +	bus@0 {
> +
> +		/* Custom pinmux configurations for 40-pin expansion header */
> +		pinmux@c302000 {
> +			status = "okay";
> +			touch-clk-pcc4 {
> +				nvidia,pins = "touch_clk_pcc4";
> +				nvidia,function = "rsvd2";
> +				nvidia,pull = <TEGRA_PIN_PULL_UP>;
> +				nvidia,tristate = <TEGRA_PIN_ENABLE>;
> +				nvidia,enable-input = <TEGRA_PIN_ENABLE>;
> +			};
> +		};
> +
> +		pinmux@2430028 {
> +			status = "okay";
> +			soc-gpio41-pq5 {

[Severity: High]
Is it intended to create standalone nodes at the exact register offsets of
the individual pins (pinmux@c302000 and pinmux@2430028), instead of placing
them as subnodes under the main pinctrl controllers (which reside at
pinmux@c300000 and pinmux@2430000 in tegra194.dtsi)?

Because they are standalone nodes and lack a pinctrl-0 reference, the pinctrl
driver won't parse them, which will cause these pinmux configurations to be
ignored.

[ ... ]

> +		/* SDMMC3 for external user SD card slot with dedicated 3.3V power */
> +		sdhci@3440000 {
> +			vmmc-supply = <&ap2306gn_3v3_sd>;
> +			cd-gpios = <&gpio_aon TEGRA194_AON_GPIO(CC, 4) GPIO_ACTIVE_HIGH>;
> +			status = "okay";
> +		};

[Severity: High]
In the upstream tegra194.dtsi, the SDMMC3 controller is defined as
mmc@3440000.

Does using sdhci@3440000 here create a new, inert dummy node instead of
overriding the existing mmc@3440000 node? This would leave the actual
controller in its default disabled state.

> +
> +		/* Disable fan and tachometer hardware not populated on SE70 carrier board */
> +		pwm-fan {
> +			status = "disabled";
> +		};

[Severity: High]
In tegra194-p3509-0000.dtsi, the pwm-fan node is located at the root of the
device tree (/pwm-fan).

Since this override is nested inside /bus@0/, doesn't this create a new dummy
node at /bus@0/pwm-fan while leaving the actual root /pwm-fan node enabled?

> +		thermal-fan-est {
> +			status = "disabled";
> +		};
> +		tachometer@39c0000 {
> +			status = "disabled";
> +		};

[Severity: Medium]
It looks like nodes such as thermal-fan-est, tachometer@39c0000,
generic-pwm-tachometer, spi@3210000, and spi@3230000 do not exist in the
upstream tegra194 device tree sources.

Does overriding them here just create new dummy disabled nodes instead of
affecting real hardware?

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

  reply	other threads:[~2026-06-26  3:47 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-26  3:34 [RFC v2 PATCH 0/2] Add Lenovo ThinkEdge SE70 carrier board support Jiqi Li
2026-06-26  3:34 ` [PATCH 1/2] dt-bindings: arm: tegra: Add lenovo,thinkedge-se70 compatible string Jiqi Li
2026-06-26  3:43   ` sashiko-bot
2026-06-26  3:34 ` [PATCH 2/2] arm64: tegra: Add Lenovo ThinkEdge SE70 carrier board DTS Jiqi Li
2026-06-26  3:47   ` sashiko-bot [this message]
  -- strict thread matches above, loose matches on Subject: below --
2026-06-28 12:43 [v4 PATCH 0/2] Add Lenovo ThinkEdge SE70 carrier board support Jiqi Li
2026-06-28 12:43 ` [PATCH 2/2] arm64: tegra: Add Lenovo ThinkEdge SE70 carrier board DTS Jiqi Li
2026-06-28 12:52   ` sashiko-bot
2026-06-29  6:13   ` Mikko Perttunen
2026-06-26 10:56 [RFC v4 PATCH 0/2] Add Lenovo ThinkEdge SE70 carrier board support Jiqi Li
2026-06-26 10:56 ` [PATCH 2/2] arm64: tegra: Add Lenovo ThinkEdge SE70 carrier board DTS Jiqi Li
2026-06-26 11:10   ` sashiko-bot
2026-06-26  9:13 [RFC v3 PATCH 0/2] Add Lenovo ThinkEdge SE70 carrier board support Jiqi Li
2026-06-26  9:13 ` [PATCH 2/2] arm64: tegra: Add Lenovo ThinkEdge SE70 carrier board DTS Jiqi Li
2026-06-26  9:28   ` sashiko-bot
2026-06-26  1:09 [RFC PATCH 0/2] Add Lenovo ThinkEdge SE70 carrier board support Jiqi Li
2026-06-26  1:09 ` [PATCH 2/2] arm64: tegra: Add Lenovo ThinkEdge SE70 carrier board DTS Jiqi Li
2026-06-26  1:23   ` sashiko-bot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260626034708.3A62F1F000E9@smtp.kernel.org \
    --to=sashiko-bot@kernel.org \
    --cc=conor+dt@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=lijq9@lenovo.com \
    --cc=robh@kernel.org \
    --cc=sashiko-reviews@lists.linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.