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: drm: Introduce drmm_connector_dp_init() with link training state properties
Date: Sun, 12 Apr 2026 10:51:25 +1000	[thread overview]
Message-ID: <review-patch9-20260409-feat_link_cap-v1-9-7069e8199ce2@bootlin.com> (raw)
In-Reply-To: <20260409-feat_link_cap-v1-9-7069e8199ce2@bootlin.com>

Patch Review

This is the core of the series and has several issues:

**Bug -- kerneldoc mismatch:**
```c
/**
 * drm_connector_dp_link_reset_properties() - Reset DisplayPort link configuration
 * @connector: DRM connector
 * @dp_link: Link training informations
 */
int
drm_connector_dp_set_link_train_properties(...)
```
The kerneldoc name says `reset` but the function is `set`. The `@dp_link` parameter name doesn't match the actual parameter `dp_link_train`. "informations" should be "information".

**Bug -- copyright header:**
```c
 * Copyright (C) 2026 Google
 * Author: Kory Maincent <kory.maincent@bootlin.com>
```
Author is from Bootlin, copyright says Google. This is likely wrong.

**Design issue -- no uevent on property change:** `drm_object_property_set_value()` updates the stored value but does not send a uevent to userspace. There is no mechanism for userspace to learn that link training has completed or that values have changed without polling. For a monitoring/diagnostic feature, this severely limits usefulness. Consider calling `drm_sysfs_connector_property_event()` or similar after updating properties.

**Design issue -- bitmask for single current values:** The `nlanes` property uses bitmask semantics. When reporting current state (e.g., "using 2 lanes"), the driver sets `nlanes = DRM_DP_2LANE = BIT(1)`. A bitmask with a single bit set is semantically an enum value, not a bitmask. The same bitmask property type is used for both "supported lane counts" (multiple bits) and "current lane count" (single bit), which is confusing. Consider using enum properties for runtime state.

**Performance concern -- link rate lookup via string parsing:**
```c
if (!kstrtou32(prop_enum->name, 10, &parsed_rate) &&
    dp_link_train->rate * 10 == parsed_rate) {
```
This iterates over enum entries and parses string names to find a matching rate. This is called on every link training completion. Storing the rates as integers and comparing directly would be more efficient and less fragile.

**Minor -- `{0}` initialization:**
```c
struct drm_connector_dp_link_train dp_link_train = {0};
```
Preferred kernel style is `= { }` or `= { 0 }` (with space). The `{0}` form works but is not idiomatic in kernel code.

---
Generated by Claude Code Patch Reviewer

  parent reply	other threads:[~2026-04-12  0:51 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-09 17:08 [PATCH RFC 00/12] Add support for DisplayPort link training information report Kory Maincent
2026-04-09 17:08 ` [PATCH RFC 01/12] drm/i915/display/intel_sdvo: Fix double connector destroy in error paths Kory Maincent
2026-04-12  0:51   ` Claude review: " Claude Code Review Bot
2026-04-09 17:08 ` [PATCH RFC 02/12] drm/i915/display/intel_lvds: Drop redundant manual cleanup on init failure Kory Maincent
2026-04-12  0:51   ` Claude review: " Claude Code Review Bot
2026-04-09 17:08 ` [PATCH RFC 03/12] drm/i915/display/intel_dp: Drop redundant intel_dp_aux_fini() " Kory Maincent
2026-04-12  0:51   ` Claude review: " Claude Code Review Bot
2026-04-09 17:08 ` [PATCH RFC 04/12] drm/i915/display: Switch to drmm_mode_config_init() and drop manual cleanup Kory Maincent
2026-04-12  0:51   ` Claude review: " Claude Code Review Bot
2026-04-09 17:08 ` [PATCH RFC 05/12] drm/i915/display: Switch to managed for crtc Kory Maincent
2026-04-12  0:51   ` Claude review: " Claude Code Review Bot
2026-04-09 17:08 ` [PATCH RFC 06/12] drm/i915/display: Switch to managed for plane Kory Maincent
2026-04-12  0:51   ` Claude review: " Claude Code Review Bot
2026-04-09 17:08 ` [PATCH RFC 07/12] drm/i915/display: Switch to managed for encoder Kory Maincent
2026-04-12  0:51   ` Claude review: " Claude Code Review Bot
2026-04-09 17:08 ` [PATCH RFC 08/12] drm/i915/display: Switch to managed for connector Kory Maincent
2026-04-12  0:51   ` Claude review: " Claude Code Review Bot
2026-04-09 17:08 ` [PATCH RFC 09/12] drm: Introduce drmm_connector_dp_init() with link training state properties Kory Maincent
2026-04-09 21:53   ` Dmitry Baryshkov
2026-04-10 16:20   ` Jani Nikula
2026-04-12  0:51   ` Claude Code Review Bot [this message]
2026-04-09 17:08 ` [PATCH RFC 10/12] drm/i915/display/dp: Adopt dp_connector helpers to expose link training state Kory Maincent
2026-04-10 16:26   ` Jani Nikula
2026-04-12  0:51   ` Claude review: " Claude Code Review Bot
2026-04-09 17:08 ` [PATCH RFC 11/12] drm/bridge: Wire drmm_connector_dp_init() via new DRM_BRIDGE_OP_DP flag Kory Maincent
2026-04-12  0:51   ` Claude review: " Claude Code Review Bot
2026-04-09 17:08 ` [PATCH RFC 12/12] drm/mediatek: Use dp_connector helpers to report link training state Kory Maincent
2026-04-12  0:51   ` Claude review: " Claude Code Review Bot
2026-04-09 20:36 ` [PATCH RFC 00/12] Add support for DisplayPort link training information report Ville Syrjälä
2026-04-09 21:36   ` Dmitry Baryshkov
2026-04-12  0:51 ` Claude review: " 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-patch9-20260409-feat_link_cap-v1-9-7069e8199ce2@bootlin.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