public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* Claude review: dt-bindings: display: tegra: document Tegra20 HDMI port
  2026-02-10  9:49 ` [PATCH v1 1/2] dt-bindings: display: " Svyatoslav Ryhel
@ 2026-02-11  6:27   ` Claude Code Review Bot
  0 siblings, 0 replies; 7+ messages in thread
From: Claude Code Review Bot @ 2026-02-11  6:27 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**COMMIT MESSAGE:**
The commit message is minimal but adequate for a binding addition. It states what is being added without explaining why the change is needed.

**TECHNICAL ISSUES:**

1. **YAML indentation errors (BLOCKING):**
```yaml
+anyOf:
+  - required:
+    - nvidia,ddc-i2c-bus
+    - nvidia,hpd-gpio
+  - required:
+    - port
```

The yamllint warnings indicate lines 107 and 110 have incorrect indentation. The `- required:` entries under `anyOf` should be indented with 6 spaces, not 4. This causes the binding validation to fail.

**Correct format should be:**
```yaml
anyOf:
  - required:
      - nvidia,ddc-i2c-bus
      - nvidia,hpd-gpio
  - required:
      - port
```

2. **Port description clarity:**
```yaml
+  port:
+    description: connection to controller receiving HDMI signals
```

Rob Herring correctly points out this describes the remote endpoint, not the local port. The description should focus on what this HDMI block outputs.

**Suggested improvement:**
```yaml
  port:
    description: HDMI output port for connection to HDMI connector or bridge
```

**STRUCTURAL REVIEW:**

The logic of making `nvidia,ddc-i2c-bus`/`nvidia,hpd-gpio` and `port` mutually exclusive via `anyOf` is correct:
- Old binding: direct DDC/HPD properties (deprecated but still supported)
- New binding: OF graph port (connects to hdmi-connector node)

This maintains backward compatibility while enabling the modern approach.

**RATING:** Needs work (indentation must be fixed, description should be improved)

---

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 0/2] ARM: tegra: document Tegra20 HDMI port
@ 2026-02-23  6:54 Svyatoslav Ryhel
  2026-02-23  6:54 ` [PATCH v2 1/2] dt-bindings: display: " Svyatoslav Ryhel
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Svyatoslav Ryhel @ 2026-02-23  6:54 UTC (permalink / raw)
  To: David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Thierry Reding, Thierry Reding, Jonathan Hunter, Svyatoslav Ryhel
  Cc: dri-devel, devicetree, linux-tegra, linux-kernel

Document port which can be used in the HDMI to model it using OF
graph.

---
Changes in v2:
- fixed intendation
- adjusted port description
---

Svyatoslav Ryhel (2):
  dt-bindings: display: tegra: document Tegra20 HDMI port
  ARM: tegra: transformers: add connector node

 .../display/tegra/nvidia,tegra20-hdmi.yaml    | 13 ++++++++++--
 .../boot/dts/nvidia/tegra30-asus-tf600t.dts   | 21 +++++++++++++++++--
 2 files changed, 30 insertions(+), 4 deletions(-)

-- 
2.51.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2 1/2] dt-bindings: display: tegra: document Tegra20 HDMI port
  2026-02-23  6:54 [PATCH v2 0/2] ARM: tegra: document Tegra20 HDMI port Svyatoslav Ryhel
@ 2026-02-23  6:54 ` Svyatoslav Ryhel
  2026-02-24  0:34   ` Claude review: " Claude Code Review Bot
  2026-02-23  6:55 ` [PATCH v2 2/2] ARM: tegra: transformers: add connector node Svyatoslav Ryhel
  2026-02-24  0:34 ` Claude review: ARM: tegra: document Tegra20 HDMI port Claude Code Review Bot
  2 siblings, 1 reply; 7+ messages in thread
From: Svyatoslav Ryhel @ 2026-02-23  6:54 UTC (permalink / raw)
  To: David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Thierry Reding, Thierry Reding, Jonathan Hunter, Svyatoslav Ryhel
  Cc: dri-devel, devicetree, linux-tegra, linux-kernel

Tegra HDMI can be modeled using an OF graph. Reflect this in the bindings.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 .../bindings/display/tegra/nvidia,tegra20-hdmi.yaml | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-hdmi.yaml b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-hdmi.yaml
index f77197e4869f..b4bf2662780b 100644
--- a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-hdmi.yaml
+++ b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-hdmi.yaml
@@ -82,6 +82,10 @@ properties:
     description: phandle of a display panel
     $ref: /schemas/types.yaml#/definitions/phandle
 
+  port:
+    description: HDMI output port for connection to HDMI connector or bridge
+    $ref: /schemas/graph.yaml#/properties/port
+
   "#sound-dai-cells":
     const: 0
 
@@ -97,8 +101,13 @@ required:
   - reset-names
   - pll-supply
   - vdd-supply
-  - nvidia,ddc-i2c-bus
-  - nvidia,hpd-gpio
+
+anyOf:
+  - required:
+      - nvidia,ddc-i2c-bus
+      - nvidia,hpd-gpio
+  - required:
+      - port
 
 examples:
   - |
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 2/2] ARM: tegra: transformers: add connector node
  2026-02-23  6:54 [PATCH v2 0/2] ARM: tegra: document Tegra20 HDMI port Svyatoslav Ryhel
  2026-02-23  6:54 ` [PATCH v2 1/2] dt-bindings: display: " Svyatoslav Ryhel
@ 2026-02-23  6:55 ` Svyatoslav Ryhel
  2026-02-24  0:34   ` Claude review: " Claude Code Review Bot
  2026-02-24  0:34 ` Claude review: ARM: tegra: document Tegra20 HDMI port Claude Code Review Bot
  2 siblings, 1 reply; 7+ messages in thread
From: Svyatoslav Ryhel @ 2026-02-23  6:55 UTC (permalink / raw)
  To: David Airlie, Simona Vetter, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
	Thierry Reding, Thierry Reding, Jonathan Hunter, Svyatoslav Ryhel
  Cc: dri-devel, devicetree, linux-tegra, linux-kernel

All ASUS Transformers have micro-HDMI connector directly available. After
Tegra HDMI got bridge/connector support, we should use connector framework
for proper HW description.

Tested-by: Andreas Westman Dorcsak <hedmoo@yahoo.com> # ASUS TF T30
Tested-by: Robert Eckelmann <longnoserob@gmail.com> # ASUS TF101 T20
Tested-by: Svyatoslav Ryhel <clamor95@gmail.com> # ASUS TF201 T30
Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 .../boot/dts/nvidia/tegra30-asus-tf600t.dts   | 21 +++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/nvidia/tegra30-asus-tf600t.dts b/arch/arm/boot/dts/nvidia/tegra30-asus-tf600t.dts
index 1ed0536ae3fa..498780a96cf9 100644
--- a/arch/arm/boot/dts/nvidia/tegra30-asus-tf600t.dts
+++ b/arch/arm/boot/dts/nvidia/tegra30-asus-tf600t.dts
@@ -67,8 +67,11 @@ hdmi: hdmi@54280000 {
 			pll-supply = <&vdd_1v8_vio>;
 			vdd-supply = <&vdd_3v3_sys>;
 
-			nvidia,hpd-gpio = <&gpio TEGRA_GPIO(N, 7) GPIO_ACTIVE_HIGH>;
-			nvidia,ddc-i2c-bus = <&hdmi_ddc>;
+			port {
+				hdmi_out: endpoint {
+					remote-endpoint = <&connector_in>;
+				};
+			};
 		};
 
 		lcd: dsi@54300000 {
@@ -2302,6 +2305,20 @@ clk32k_in: clock-32k {
 		clock-output-names = "pmic-oscillator";
 	};
 
+	connector {
+		compatible = "hdmi-connector";
+		type = "d";
+
+		hpd-gpios = <&gpio TEGRA_GPIO(N, 7) GPIO_ACTIVE_HIGH>;
+		ddc-i2c-bus = <&hdmi_ddc>;
+
+		port {
+			connector_in: endpoint {
+				remote-endpoint = <&hdmi_out>;
+			};
+		};
+	};
+
 	cpus {
 		cpu0: cpu@0 {
 			cpu-supply = <&vdd_cpu>;
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Claude review: ARM: tegra: document Tegra20 HDMI port
  2026-02-23  6:54 [PATCH v2 0/2] ARM: tegra: document Tegra20 HDMI port Svyatoslav Ryhel
  2026-02-23  6:54 ` [PATCH v2 1/2] dt-bindings: display: " Svyatoslav Ryhel
  2026-02-23  6:55 ` [PATCH v2 2/2] ARM: tegra: transformers: add connector node Svyatoslav Ryhel
@ 2026-02-24  0:34 ` Claude Code Review Bot
  2 siblings, 0 replies; 7+ messages in thread
From: Claude Code Review Bot @ 2026-02-24  0:34 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: ARM: tegra: document Tegra20 HDMI port
Author: Svyatoslav Ryhel <clamor95@gmail.com>
Patches: 3
Reviewed: 2026-02-24T10:34:48.093002

---

This is a straightforward two-patch device tree series that adds OF graph port support to the Tegra HDMI binding and converts the ASUS TF600T DTS to use an explicit `hdmi-connector` node instead of inline HPD/DDC properties. The approach is standard for modeling HDMI outputs through the connector framework, moving the hotplug detect GPIO and DDC I2C bus from the HDMI encoder node to a separate connector node connected via OF graph endpoints.

The binding change in patch 1 uses `anyOf` to preserve backward compatibility -- existing DTS files with `nvidia,hpd-gpio` and `nvidia,ddc-i2c-bus` remain valid, while new DTS files can use the `port` property instead. The DTS change in patch 2 correctly uses the `hdmi-connector` binding with type `"d"` (micro HDMI) and valid properties.

There are a couple of minor observations but no significant correctness issues in the series.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Claude review: dt-bindings: display: tegra: document Tegra20 HDMI port
  2026-02-23  6:54 ` [PATCH v2 1/2] dt-bindings: display: " Svyatoslav Ryhel
@ 2026-02-24  0:34   ` Claude Code Review Bot
  0 siblings, 0 replies; 7+ messages in thread
From: Claude Code Review Bot @ 2026-02-24  0:34 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

The binding change adds a `port` property and restructures the required properties to allow either the legacy approach or the OF graph approach.

> +  port:
> +    description: HDMI output port for connection to HDMI connector or bridge
> +    $ref: /schemas/graph.yaml#/properties/port

This is a clean addition. The reference to the graph port schema is correct, and since the existing binding has `additionalProperties: false`, the property must be listed here to be allowed.

> -  - nvidia,ddc-i2c-bus
> -  - nvidia,hpd-gpio
> +
> +anyOf:
> +  - required:
> +      - nvidia,ddc-i2c-bus
> +      - nvidia,hpd-gpio
> +  - required:
> +      - port

The `anyOf` logic is correct: at least one of the two groups must be satisfied. This preserves backward compatibility with existing DTS files that use the legacy properties. One thing to note is that `anyOf` also permits a node to have all three properties simultaneously (`nvidia,ddc-i2c-bus`, `nvidia,hpd-gpio`, and `port`). That may or may not be intentional -- if the driver doesn't handle having both the legacy properties and a port simultaneously, it might be worth considering `oneOf` instead. But this is a design choice rather than a bug, and having both present wouldn't violate the schema.

The commit message is accurate but brief. No other issues.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Claude review: ARM: tegra: transformers: add connector node
  2026-02-23  6:55 ` [PATCH v2 2/2] ARM: tegra: transformers: add connector node Svyatoslav Ryhel
@ 2026-02-24  0:34   ` Claude Code Review Bot
  0 siblings, 0 replies; 7+ messages in thread
From: Claude Code Review Bot @ 2026-02-24  0:34 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

> -			nvidia,hpd-gpio = <&gpio TEGRA_GPIO(N, 7) GPIO_ACTIVE_HIGH>;
> -			nvidia,ddc-i2c-bus = <&hdmi_ddc>;
> +			port {
> +				hdmi_out: endpoint {
> +					remote-endpoint = <&connector_in>;
> +				};
> +			};

The legacy properties are removed and replaced with an OF graph port endpoint. This satisfies the `port` clause in the `anyOf` from patch 1.

> +	connector {
> +		compatible = "hdmi-connector";
> +		type = "d";
> +
> +		hpd-gpios = <&gpio TEGRA_GPIO(N, 7) GPIO_ACTIVE_HIGH>;
> +		ddc-i2c-bus = <&hdmi_ddc>;
> +
> +		port {
> +			connector_in: endpoint {
> +				remote-endpoint = <&hdmi_out>;
> +			};
> +		};
> +	};

The connector node looks correct. Type `"d"` is micro HDMI, which matches the cover letter's description of a micro-HDMI connector. The `hpd-gpios` and `ddc-i2c-bus` properties are both valid per the `hdmi-connector` binding, and the HPD GPIO specification is carried over from its previous location in the HDMI node. The OF graph endpoints are properly cross-referenced.

Minor observation: the subject says "transformers" (plural) but this patch only modifies `tegra30-asus-tf600t.dts`. The Tested-by tags reference three different Transformer models (TF600T, TF101, TF201), suggesting similar DTS changes may come for those boards in a separate series. The subject could be more precise (e.g., "tegra: tf600t: add connector node"), but this is a trivial nit.

No correctness issues found in this patch.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-02-24  0:34 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-02-23  6:54 [PATCH v2 0/2] ARM: tegra: document Tegra20 HDMI port Svyatoslav Ryhel
2026-02-23  6:54 ` [PATCH v2 1/2] dt-bindings: display: " Svyatoslav Ryhel
2026-02-24  0:34   ` Claude review: " Claude Code Review Bot
2026-02-23  6:55 ` [PATCH v2 2/2] ARM: tegra: transformers: add connector node Svyatoslav Ryhel
2026-02-24  0:34   ` Claude review: " Claude Code Review Bot
2026-02-24  0:34 ` Claude review: ARM: tegra: document Tegra20 HDMI port Claude Code Review Bot
  -- strict thread matches above, loose matches on Subject: below --
2026-02-10  9:49 [PATCH v1 0/2] " Svyatoslav Ryhel
2026-02-10  9:49 ` [PATCH v1 1/2] dt-bindings: display: " Svyatoslav Ryhel
2026-02-11  6:27   ` Claude review: " Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox