public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: arm64: dts: qcom: Add Glymur-based Lenovo Yoga Slim 7x Gen11
Date: Fri, 05 Jun 2026 06:39:42 +1000	[thread overview]
Message-ID: <review-patch3-20260604-topic-yoga_submission-v1-3-57c70c23d0d6@oss.qualcomm.com> (raw)
In-Reply-To: <20260604-topic-yoga_submission-v1-3-57c70c23d0d6@oss.qualcomm.com>

Patch Review

This is the main patch -- a 1237-line DTS for the laptop. Overall it is well-written and covers a lot of hardware. A few issues to flag:

**1. Orphaned `pcie3b_default` pinctrl state (dead code)**

The `&tlmm` section defines `pcie3b_default` (GPIOs 155-157) but no `&pcie3b` node exists in this DTS that references it. In the CRD, `&pcie3b` uses this for a second NVMe slot. If this laptop doesn't have a second NVMe, this should be removed:

```
+	pcie3b_default: pcie3b-default-state {
+		clkreq-n-pins {
+			pins = "gpio156";
+			function = "pcie3b_clk";
+			...
```

**2. Orphaned `nvme_sec_reg_en` pinctrl state (dead code)**

Similarly, `nvme_sec_reg_en` is defined in `&pmh0110_f_e1_gpios` but never referenced by any regulator or device node in this DTS:

```
+&pmh0110_f_e1_gpios {
+	nvme_sec_reg_en: nvme-reg-en-state {
+		pins = "gpio14";
+		function = "normal";
+		bias-disable;
+	};
+};
```

This pairs with `pcie3b_default` -- both are CRD second-NVMe leftovers.

**3. Orphaned `ec_int_n_default` pinctrl state (dead code)**

The `ec_int_n_default` pinctrl state is defined but not referenced by any node:

```
+	ec_int_n_default: ec-int-n-state {
+		pins = "gpio66";
+		function = "gpio";
+		bias-disable;
+	};
```

The comment `/* EC @ 0x70, irq = TLMM 66 */` in `&i2c9` explains why the pin is reserved, but the pinctrl state itself is unused. Either remove it or add a comment noting it's reserved for future EC driver support.

**4. `key_vol_up_default` pinctrl referenced but no volume-up key defined**

The gpio-keys node references `key_vol_up_default` in its pinctrl but only defines a lid switch -- no volume-up key:

```
+	gpio-keys {
+		compatible = "gpio-keys";
+
+		pinctrl-0 = <&key_vol_up_default>, <&hall_int_n_default>;
+		pinctrl-names = "default";
+
+		switch-lid {
```

Either add the volume-up key child node (the CRD has one on `&pmh0101_gpios 6`), or remove `<&key_vol_up_default>` from pinctrl-0 and drop the `key_vol_up_default` definition from the second `&pmh0101_gpios` block. This looks like a copy-paste oversight from the CRD.

**5. `&i2c5` enabled with no child devices**

```
+&i2c5 {
+	clock-frequency = <400000>;
+
+	status = "okay";
+};
```

Enabling an I2C bus with no devices attached is unusual. The CRD has PTN3222 redrivers on i2c5. If those aren't present on this board, the node should probably not be enabled, to avoid needlessly probing an empty bus.

**6. `&mdss_dp3` uses `/delete-property/ #sound-dai-cells` -- why?**

```
+&mdss_dp3 {
+	/delete-property/ #sound-dai-cells;
```

This suggests the base dtsi defines `#sound-dai-cells` for DP3 (for DP audio), but the Yoga explicitly removes it. A comment explaining why would help -- presumably because the eDP panel doesn't carry audio?

**7. Minor: Duplicate `&pmh0101_gpios` blocks**

Two separate `&pmh0101_gpios` blocks exist. DT overlays merge them, so this is functionally correct. However, per the CRD which follows the same pattern, this is acceptable -- though combining them into one block would be cleaner.

**Overall for Patch 3: Good first submission. The hardware coverage is comprehensive. Clean up the dead pinctrl/regulator definitions and fix the volume-up key inconsistency for v2.**

---
Generated by Claude Code Patch Reviewer

  parent reply	other threads:[~2026-06-04 20:39 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-04  8:06 [PATCH 0/4] X2 Elite Lenovo Yoga Slim 7x Gen11 support Konrad Dybcio
2026-06-04  8:06 ` [PATCH 1/4] dt-bindings: display: panel: samsung,atna33xc20: Add ATNA40HQ08-0 Konrad Dybcio
2026-06-04 15:05   ` [PATCH 1/4] dt-bindings: display: panel: samsung, atna33xc20: " Krzysztof Kozlowski
2026-06-04 20:39   ` Claude review: dt-bindings: display: panel: samsung,atna33xc20: " Claude Code Review Bot
2026-06-04  8:06 ` [PATCH 2/4] dt-bindings: arm: qcom: Add Lenovo Yoga Slim 7x Gen11 Konrad Dybcio
2026-06-04 15:09   ` Krzysztof Kozlowski
2026-06-04 15:13     ` Rob Clark
2026-06-04 15:18       ` Krzysztof Kozlowski
2026-06-04 20:39   ` Claude review: " Claude Code Review Bot
2026-06-04  8:06 ` [PATCH 3/4] arm64: dts: qcom: Add Glymur-based " Konrad Dybcio
2026-06-04  8:22   ` sashiko-bot
2026-06-04  9:59   ` Gopikrishna Garmidi
2026-06-04 20:39   ` Claude Code Review Bot [this message]
2026-06-04  8:06 ` [PATCH 4/4] firmware: qcom: scm: Allow QSEECOM on " Konrad Dybcio
2026-06-04 13:40   ` Dmitry Baryshkov
2026-06-04 20:39   ` Claude review: " Claude Code Review Bot
2026-06-04 20:39 ` Claude review: X2 Elite Lenovo Yoga Slim 7x Gen11 support Claude Code Review 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=review-patch3-20260604-topic-yoga_submission-v1-3-57c70c23d0d6@oss.qualcomm.com \
    --to=claude-review@example.com \
    --cc=dri-devel-reviews@example.com \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox