public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH 0/2] drm/tidss: Two minor fixes
@ 2026-03-11  8:16 Tomi Valkeinen
  2026-03-11  8:16 ` [PATCH 1/2] drm/tidss: Drop extra drm_mode_config_reset() call Tomi Valkeinen
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Tomi Valkeinen @ 2026-03-11  8:16 UTC (permalink / raw)
  To: Jyri Sarha, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Simona Vetter, Sam Ravnborg,
	Javier Martinez Canillas, Aradhya Bhatia
  Cc: dri-devel, linux-kernel, Devarsh Thakkar, Tomi Valkeinen, stable

Two minor fixes. One to drop an extra drm_mode_config_reset() call,
another to fix a "Missing drm_bridge_add() before attach" warning print.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
Tomi Valkeinen (2):
      drm/tidss: Drop extra drm_mode_config_reset() call
      drm/tidss: Fix missing drm_bridge_attach() call

 drivers/gpu/drm/tidss/tidss_encoder.c | 2 ++
 drivers/gpu/drm/tidss/tidss_kms.c     | 2 --
 2 files changed, 2 insertions(+), 2 deletions(-)
---
base-commit: 11439c4635edd669ae435eec308f4ab8a0804808
change-id: 20260311-tidss-minor-fixes-c0f853a5326f

Best regards,
-- 
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>


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

* [PATCH 1/2] drm/tidss: Drop extra drm_mode_config_reset() call
  2026-03-11  8:16 [PATCH 0/2] drm/tidss: Two minor fixes Tomi Valkeinen
@ 2026-03-11  8:16 ` Tomi Valkeinen
  2026-03-11  9:09   ` Maxime Ripard
  2026-03-11 21:12   ` Claude review: " Claude Code Review Bot
  2026-03-11  8:16 ` [PATCH 2/2] drm/tidss: Fix missing drm_bridge_attach() call Tomi Valkeinen
  2026-03-11 21:12 ` Claude review: drm/tidss: Two minor fixes Claude Code Review Bot
  2 siblings, 2 replies; 10+ messages in thread
From: Tomi Valkeinen @ 2026-03-11  8:16 UTC (permalink / raw)
  To: Jyri Sarha, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Simona Vetter, Sam Ravnborg,
	Javier Martinez Canillas, Aradhya Bhatia
  Cc: dri-devel, linux-kernel, Devarsh Thakkar, Tomi Valkeinen

We are calling drm_mode_config_reset() twice at probe time. There's no
reason for this and the second call can be removed, reducing work at
probe time slightly.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
---
 drivers/gpu/drm/tidss/tidss_kms.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
index 8bb93194e5ac..b4779c09a1bf 100644
--- a/drivers/gpu/drm/tidss/tidss_kms.c
+++ b/drivers/gpu/drm/tidss/tidss_kms.c
@@ -287,8 +287,6 @@ int tidss_modeset_init(struct tidss_device *tidss)
 	if (ret)
 		return ret;
 
-	drm_mode_config_reset(ddev);
-
 	dev_dbg(tidss->dev, "%s done\n", __func__);
 
 	return 0;

-- 
2.43.0


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

* [PATCH 2/2] drm/tidss: Fix missing drm_bridge_attach() call
  2026-03-11  8:16 [PATCH 0/2] drm/tidss: Two minor fixes Tomi Valkeinen
  2026-03-11  8:16 ` [PATCH 1/2] drm/tidss: Drop extra drm_mode_config_reset() call Tomi Valkeinen
@ 2026-03-11  8:16 ` Tomi Valkeinen
  2026-03-11  9:08   ` Maxime Ripard
  2026-03-11 21:12   ` Claude review: " Claude Code Review Bot
  2026-03-11 21:12 ` Claude review: drm/tidss: Two minor fixes Claude Code Review Bot
  2 siblings, 2 replies; 10+ messages in thread
From: Tomi Valkeinen @ 2026-03-11  8:16 UTC (permalink / raw)
  To: Jyri Sarha, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
	David Airlie, Simona Vetter, Sam Ravnborg,
	Javier Martinez Canillas, Aradhya Bhatia
  Cc: dri-devel, linux-kernel, Devarsh Thakkar, Tomi Valkeinen, stable

tidss encoder-bridge is not added with drm_bridge_add() call, which
leads to:

[drm] Missing drm_bridge_add() before attach

Add the missing call, using devm_drm_bridge_add() variant to get the
drm_bridge_remove() handled automatically.

The commit marked with the Fixes tag (from v6.6) is the commit that
added the encoder bridge without drm_bridge_add(). However, this fix is
not directly applicable there as devm_drm_bridge_alloc() was not used to
alloc the bridge, so using devm version for drm_bridge_add() wouldn't be
safe. Instead, drm_bridge_add() and drm_bridge_remove() would be needed
there, but that would require new plumbing code as we don't have a
separate cleanup function in the tidss_encoder.c, not in the tidss_kms.c
from which the encoder is created.

Also, there has been no reported bugs caused by the missing
drm_bridge_add(). The drm_bridge_add() initializes the bridge's
hpd_mutex, but HPD is not used for the encoder bridge. drm_bridge_add()
also adds the bridge to the global bridge_list, which is only used in
of_drm_find_bridge(), and again that is not used for the encoder bridge.

Thus, while the original commit is not right, there should be no bugs
caused by it, and for the time being I'm not sending a patch for the
stable kernels for the original commit.

This fix applies on top of commit 66cdf05f8548 ("drm/tidss: encoder:
convert to devm_drm_bridge_alloc()"), which changes the tidss_encoder.c
to use the devm variant (added in v6.17). The warning print was added in
v6.19, so applying this fix to v6.17+ gets rid of the warning for all
kernel versions.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: <stable@vger.kernel.org> # v6.17+
Fixes: c932ced6b585 ("drm/tidss: Update encoder/bridge chain connect model")
---
 drivers/gpu/drm/tidss/tidss_encoder.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
index 81a04f767770..db467bbcdb77 100644
--- a/drivers/gpu/drm/tidss/tidss_encoder.c
+++ b/drivers/gpu/drm/tidss/tidss_encoder.c
@@ -106,6 +106,8 @@ int tidss_encoder_create(struct tidss_device *tidss,
 	enc = &t_enc->encoder;
 	enc->possible_crtcs = possible_crtcs;
 
+	devm_drm_bridge_add(tidss->dev, &t_enc->bridge);
+
 	/* Attaching first bridge to the encoder */
 	ret = drm_bridge_attach(enc, &t_enc->bridge, NULL,
 				DRM_BRIDGE_ATTACH_NO_CONNECTOR);

-- 
2.43.0


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

* Re: [PATCH 2/2] drm/tidss: Fix missing drm_bridge_attach() call
  2026-03-11  8:16 ` [PATCH 2/2] drm/tidss: Fix missing drm_bridge_attach() call Tomi Valkeinen
@ 2026-03-11  9:08   ` Maxime Ripard
  2026-03-11  9:15     ` Tomi Valkeinen
  2026-03-11 21:12   ` Claude review: " Claude Code Review Bot
  1 sibling, 1 reply; 10+ messages in thread
From: Maxime Ripard @ 2026-03-11  9:08 UTC (permalink / raw)
  To: Tomi Valkeinen
  Cc: Jyri Sarha, Maarten Lankhorst, Thomas Zimmermann, David Airlie,
	Simona Vetter, Sam Ravnborg, Javier Martinez Canillas,
	Aradhya Bhatia, dri-devel, linux-kernel, Devarsh Thakkar, stable

[-- Attachment #1: Type: text/plain, Size: 528 bytes --]

On Wed, Mar 11, 2026 at 10:16:29AM +0200, Tomi Valkeinen wrote:
> tidss encoder-bridge is not added with drm_bridge_add() call, which
> leads to:
> 
> [drm] Missing drm_bridge_add() before attach
> 
> Add the missing call, using devm_drm_bridge_add() variant to get the
> drm_bridge_remove() handled automatically.

The patch itself is fine, but the commit title mentions a missing
drm_bridge_attach() call when it should be drm_bridge_add()

With that fixed
Acked-by: Maxime Ripard <mripard@kernel.org>

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

* Re: [PATCH 1/2] drm/tidss: Drop extra drm_mode_config_reset() call
  2026-03-11  8:16 ` [PATCH 1/2] drm/tidss: Drop extra drm_mode_config_reset() call Tomi Valkeinen
@ 2026-03-11  9:09   ` Maxime Ripard
  2026-03-11 21:12   ` Claude review: " Claude Code Review Bot
  1 sibling, 0 replies; 10+ messages in thread
From: Maxime Ripard @ 2026-03-11  9:09 UTC (permalink / raw)
  To: Tomi Valkeinen
  Cc: dri-devel, linux-kernel, Aradhya Bhatia, David Airlie,
	Devarsh Thakkar, Javier Martinez Canillas, Jyri Sarha,
	Maarten Lankhorst, Maxime Ripard, Sam Ravnborg, Simona Vetter,
	Thomas Zimmermann

On Wed, 11 Mar 2026 10:16:28 +0200, Tomi Valkeinen wrote:
> We are calling drm_mode_config_reset() twice at probe time. There's no
> reason for this and the second call can be removed, reducing work at
> probe time slightly.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

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

* Re: [PATCH 2/2] drm/tidss: Fix missing drm_bridge_attach() call
  2026-03-11  9:08   ` Maxime Ripard
@ 2026-03-11  9:15     ` Tomi Valkeinen
  0 siblings, 0 replies; 10+ messages in thread
From: Tomi Valkeinen @ 2026-03-11  9:15 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Jyri Sarha, Maarten Lankhorst, Thomas Zimmermann, David Airlie,
	Simona Vetter, Sam Ravnborg, Javier Martinez Canillas,
	Aradhya Bhatia, dri-devel, linux-kernel, Devarsh Thakkar, stable

Hi,

On 11/03/2026 11:08, Maxime Ripard wrote:
> On Wed, Mar 11, 2026 at 10:16:29AM +0200, Tomi Valkeinen wrote:
>> tidss encoder-bridge is not added with drm_bridge_add() call, which
>> leads to:
>>
>> [drm] Missing drm_bridge_add() before attach
>>
>> Add the missing call, using devm_drm_bridge_add() variant to get the
>> drm_bridge_remove() handled automatically.
> 
> The patch itself is fine, but the commit title mentions a missing
> drm_bridge_attach() call when it should be drm_bridge_add()
> 
> With that fixed
> Acked-by: Maxime Ripard <mripard@kernel.org>
Indeed. Fixed in v2. Thanks!

 Tomi


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

* Claude review: drm/tidss: Drop extra drm_mode_config_reset() call
  2026-03-11  9:14 ` [PATCH v2 1/2] drm/tidss: Drop extra drm_mode_config_reset() call Tomi Valkeinen
@ 2026-03-11 21:10   ` Claude Code Review Bot
  0 siblings, 0 replies; 10+ messages in thread
From: Claude Code Review Bot @ 2026-03-11 21:10 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Status: Looks good.**

The patch removes `drm_mode_config_reset(ddev)` from `tidss_kms.c` (`tidss_modeset_init()`). Checking the kernel tree confirms there are currently two calls:
- `tidss_kms.c:290` — inside `tidss_modeset_init()` (the one being removed)
- `tidss_drv.c:174` — in the probe path, called *after* `tidss_modeset_init()` returns

Since `tidss_drv.c:174` runs later (after `drm_kms_helper_poll_init()` and before `drm_dev_register()`), the earlier call in `tidss_kms.c` is indeed redundant — the state will just get reset again. Removing it is correct and the Fixes tag to the original driver commit is appropriate.

No issues.

---
Generated by Claude Code Patch Reviewer

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

* Claude review: drm/tidss: Two minor fixes
  2026-03-11  8:16 [PATCH 0/2] drm/tidss: Two minor fixes Tomi Valkeinen
  2026-03-11  8:16 ` [PATCH 1/2] drm/tidss: Drop extra drm_mode_config_reset() call Tomi Valkeinen
  2026-03-11  8:16 ` [PATCH 2/2] drm/tidss: Fix missing drm_bridge_attach() call Tomi Valkeinen
@ 2026-03-11 21:12 ` Claude Code Review Bot
  2 siblings, 0 replies; 10+ messages in thread
From: Claude Code Review Bot @ 2026-03-11 21:12 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/tidss: Two minor fixes
Author: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Patches: 6
Reviewed: 2026-03-12T07:12:05.473683

---

This is a clean, well-described 2-patch series with straightforward fixes to the TI TIDSS display driver. Patch 1 removes a redundant `drm_mode_config_reset()` call, and Patch 2 adds a missing `devm_drm_bridge_add()` call to silence a runtime warning. The commit messages are excellent — particularly Patch 2, which provides thorough context on the Fixes tag, backport applicability, and why there were no user-visible bugs despite the missing call.

One issue needs addressing: the return value of `devm_drm_bridge_add()` is not checked.

---
Generated by Claude Code Patch Reviewer

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

* Claude review: drm/tidss: Drop extra drm_mode_config_reset() call
  2026-03-11  8:16 ` [PATCH 1/2] drm/tidss: Drop extra drm_mode_config_reset() call Tomi Valkeinen
  2026-03-11  9:09   ` Maxime Ripard
@ 2026-03-11 21:12   ` Claude Code Review Bot
  1 sibling, 0 replies; 10+ messages in thread
From: Claude Code Review Bot @ 2026-03-11 21:12 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Reviewed-by worthy.** The fix is correct and trivially verifiable.

The probe path calls `drm_mode_config_reset()` twice:
1. In `tidss_modeset_init()` at `tidss_kms.c:290`
2. In `tidss_probe()` at `tidss_drv.c:174`

The patch removes the first call (in `tidss_kms.c`), keeping the one in `tidss_drv.c:174` which runs later — after `tidss_irq_install()` and `drm_kms_helper_poll_init()`. This is the better one to keep, as it's the final reset before `drm_dev_register()`.

No issues found.

---
Generated by Claude Code Patch Reviewer

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

* Claude review: drm/tidss: Fix missing drm_bridge_attach() call
  2026-03-11  8:16 ` [PATCH 2/2] drm/tidss: Fix missing drm_bridge_attach() call Tomi Valkeinen
  2026-03-11  9:08   ` Maxime Ripard
@ 2026-03-11 21:12   ` Claude Code Review Bot
  1 sibling, 0 replies; 10+ messages in thread
From: Claude Code Review Bot @ 2026-03-11 21:12 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**One issue:** The subject line says "Fix missing drm_bridge_attach() call" but the actual fix adds a `drm_bridge_add()` call. The subject should read **"Fix missing drm_bridge_add() call"**. The body text is correct.

**Missing error check:** `devm_drm_bridge_add()` returns `int` (0 on success, negative errno on failure). The patch does not check the return value:

```c
+	devm_drm_bridge_add(tidss->dev, &t_enc->bridge);
```

This should be:

```c
	ret = devm_drm_bridge_add(tidss->dev, &t_enc->bridge);
	if (ret)
		return ret;
```

The `ret` variable is already declared and available in scope at `tidss_encoder.c:91`.

The placement of the call (after `devm_drm_bridge_alloc()` and encoder init, but before `drm_bridge_attach()`) is correct — the bridge must be added to the global list before it can be attached.

The Fixes tag, Cc: stable annotation, and the thorough explanation of why the fix is only needed for v6.17+ are all well done.

---
Generated by Claude Code Patch Reviewer

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

end of thread, other threads:[~2026-03-11 21:12 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-11  8:16 [PATCH 0/2] drm/tidss: Two minor fixes Tomi Valkeinen
2026-03-11  8:16 ` [PATCH 1/2] drm/tidss: Drop extra drm_mode_config_reset() call Tomi Valkeinen
2026-03-11  9:09   ` Maxime Ripard
2026-03-11 21:12   ` Claude review: " Claude Code Review Bot
2026-03-11  8:16 ` [PATCH 2/2] drm/tidss: Fix missing drm_bridge_attach() call Tomi Valkeinen
2026-03-11  9:08   ` Maxime Ripard
2026-03-11  9:15     ` Tomi Valkeinen
2026-03-11 21:12   ` Claude review: " Claude Code Review Bot
2026-03-11 21:12 ` Claude review: drm/tidss: Two minor fixes Claude Code Review Bot
  -- strict thread matches above, loose matches on Subject: below --
2026-03-11  9:14 [PATCH v2 0/2] " Tomi Valkeinen
2026-03-11  9:14 ` [PATCH v2 1/2] drm/tidss: Drop extra drm_mode_config_reset() call Tomi Valkeinen
2026-03-11 21:10   ` 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