public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/tegra: sor: Remove usage of drm_simple_encoder_init()
@ 2026-05-23  1:26 Jacob McLemore
  2026-05-25  8:06 ` Claude review: " Claude Code Review Bot
  2026-05-25  8:06 ` Claude Code Review Bot
  0 siblings, 2 replies; 3+ messages in thread
From: Jacob McLemore @ 2026-05-23  1:26 UTC (permalink / raw)
  To: jmclemore.lkml
  Cc: tzimmermann, Thierry Reding, Mikko Perttunen, David Airlie,
	Simona Vetter, Jonathan Hunter, dri-devel, linux-tegra,
	linux-kernel

Remove the deprecated trivial helper drm_simple_encoder_init(). Inline
the call to drm_encoder_init and add instance of
drm_encoder_funcs.

Signed-off-by: Jacob McLemore <jmclemore.lkml@gmail.com>
---
Saw this was a good first task in Documentation/gpu/todo.rst.
This is my first patch, so apologies if I've set up anything wrong
in either my email client or gitconfig and the patch ends up invalid.
---
 drivers/gpu/drm/tegra/sor.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
index de8b2dfc4984..4ac23aedb682 100644
--- a/drivers/gpu/drm/tegra/sor.c
+++ b/drivers/gpu/drm/tegra/sor.c
@@ -25,7 +25,6 @@
 #include <drm/drm_file.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_print.h>
-#include <drm/drm_simple_kms_helper.h>
 
 #include "dc.h"
 #include "dp.h"
@@ -3038,6 +3037,10 @@ static const struct tegra_sor_ops tegra_sor_dp_ops = {
 	.probe = tegra_sor_dp_probe,
 };
 
+static const struct drm_encoder_funcs tegra_sor_encoder_funcs_cleanup = {
+	.destroy = drm_encoder_cleanup,
+};
+
 static int tegra_sor_init(struct host1x_client *client)
 {
 	struct drm_device *drm = dev_get_drvdata(client->host);
@@ -3081,7 +3084,7 @@ static int tegra_sor_init(struct host1x_client *client)
 				 &tegra_sor_connector_helper_funcs);
 	sor->output.connector.dpms = DRM_MODE_DPMS_OFF;
 
-	drm_simple_encoder_init(drm, &sor->output.encoder, encoder);
+	drm_encoder_init(drm, &sor->output.encoder, &tegra_sor_encoder_funcs_cleanup, encoder, NULL);
 	drm_encoder_helper_add(&sor->output.encoder, helpers);
 
 	drm_connector_attach_encoder(&sor->output.connector,

base-commit: 79bd2dded182b1d458b18e62684b7f82ffc682e5
-- 
2.43.0


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

* Claude review: drm/tegra: sor: Remove usage of drm_simple_encoder_init()
  2026-05-23  1:26 [PATCH] drm/tegra: sor: Remove usage of drm_simple_encoder_init() Jacob McLemore
@ 2026-05-25  8:06 ` Claude Code Review Bot
  2026-05-25  8:06 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-05-25  8:06 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/tegra: sor: Remove usage of drm_simple_encoder_init()
Author: Jacob McLemore <jmclemore.lkml@gmail.com>
Patches: 1
Reviewed: 2026-05-25T18:06:52.645017

---

This is a single-patch series that removes one usage of the deprecated `drm_simple_encoder_init()` helper from the tegra SOR driver, as described in `Documentation/gpu/todo.rst`. The patch correctly inlines the helper by defining a local `drm_encoder_funcs` struct with `.destroy = drm_encoder_cleanup` and replacing the `drm_simple_encoder_init()` call with a direct `drm_encoder_init()` call. This is a straightforward mechanical conversion and the author has followed the todo.rst instructions correctly.

The patch is functionally correct — the inlined code is a faithful reproduction of what `drm_simple_encoder_init()` does internally (confirmed by reading `drm_simple_kms_helper.c:23-30`). The `#include <drm/drm_simple_kms_helper.h>` removal is also correct since no other symbols from that header are used in `sor.c`.

**Verdict: The patch looks good. Two minor style nits below, neither blocking.**

---
Generated by Claude Code Patch Reviewer

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

* Claude review: drm/tegra: sor: Remove usage of drm_simple_encoder_init()
  2026-05-23  1:26 [PATCH] drm/tegra: sor: Remove usage of drm_simple_encoder_init() Jacob McLemore
  2026-05-25  8:06 ` Claude review: " Claude Code Review Bot
@ 2026-05-25  8:06 ` Claude Code Review Bot
  1 sibling, 0 replies; 3+ messages in thread
From: Claude Code Review Bot @ 2026-05-25  8:06 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Correctness: Good.** The transformation is faithful to the original helper implementation:

Original in `drm_simple_kms_helper.c`:
```c
static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = {
    .destroy = drm_encoder_cleanup,
};
int drm_simple_encoder_init(...) {
    return drm_encoder_init(dev, encoder,
                            &drm_simple_encoder_funcs_cleanup,
                            encoder_type, NULL);
}
```

New code in `sor.c`:
```c
static const struct drm_encoder_funcs tegra_sor_encoder_funcs_cleanup = {
    .destroy = drm_encoder_cleanup,
};
...
drm_encoder_init(drm, &sor->output.encoder, &tegra_sor_encoder_funcs_cleanup, encoder, NULL);
```

This is a correct 1:1 translation.

**Nit 1 — Naming convention.** The struct is named `tegra_sor_encoder_funcs_cleanup`, which appends `_cleanup` to emphasize that the only op is `.destroy = drm_encoder_cleanup`. This mirrors the internal naming in `drm_simple_kms_helper.c`, but the more conventional name in DRM drivers would be simply `tegra_sor_encoder_funcs`. The `_cleanup` suffix is slightly unusual for a `drm_encoder_funcs` struct name and could be confusing (it reads like a cleanup function rather than a funcs table). That said, this is subjective and minor.

**Nit 2 — Line length.** The replacement call:
```c
drm_encoder_init(drm, &sor->output.encoder, &tegra_sor_encoder_funcs_cleanup, encoder, NULL);
```
is 88 characters, which exceeds the preferred 80-column limit. The existing tegra code is not strict about 80 columns but does generally wrap long function calls. Consider wrapping it for consistency with the surrounding style, e.g.:
```c
drm_encoder_init(drm, &sor->output.encoder,
                 &tegra_sor_encoder_funcs_cleanup,
                 encoder, NULL);
```

**Note — unchecked return value.** Both `drm_simple_encoder_init()` (before) and `drm_encoder_init()` (after) return `int`, and neither is checked. This is a pre-existing issue in the tegra driver (same pattern in `hdmi.c`, `rgb.c`, `dsi.c`) and not something this patch needs to fix, but worth noting. The todo.rst task is specifically about removing the `drm_simple_encoder_init` wrapper, not fixing error handling.

**Overall: Patch is correct and suitable for merging, optionally with the naming and line-length adjustments.**

---
Generated by Claude Code Patch Reviewer

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

end of thread, other threads:[~2026-05-25  8:06 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-23  1:26 [PATCH] drm/tegra: sor: Remove usage of drm_simple_encoder_init() Jacob McLemore
2026-05-25  8:06 ` Claude review: " Claude Code Review Bot
2026-05-25  8:06 ` 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