From mboxrd@z Thu Jan  1 00:00:00 1970
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
Message-ID: <review-patch9-20260409-feat_link_cap-v1-9-7069e8199ce2@bootlin.com>
In-Reply-To: <20260409-feat_link_cap-v1-9-7069e8199ce2@bootlin.com>
References: <20260409-feat_link_cap-v1-0-7069e8199ce2@bootlin.com> <20260409-feat_link_cap-v1-9-7069e8199ce2@bootlin.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

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