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: bridge: Cadence: Add MHDP8501 DP/HDMI driver
Date: Sun, 12 Apr 2026 13:34:45 +1000
Message-ID: <review-patch4-20260407-dcss-hdmi-upstreaming-v21-4-4681070ab82f@oss.nxp.com>
In-Reply-To: <20260407-dcss-hdmi-upstreaming-v21-4-4681070ab82f@oss.nxp.com>
References: <20260407-dcss-hdmi-upstreaming-v21-0-4681070ab82f@oss.nxp.com> <20260407-dcss-hdmi-upstreaming-v21-4-4681070ab82f@oss.nxp.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

This is the largest patch (~2244 lines across 6 files) and deserves the mos=
t scrutiny.

**Deprecated include:**
```c
#include <linux/of_device.h>
```
`of_device.h` is deprecated in recent kernels. Use `linux/of.h` and `linux/=
platform_device.h` instead.

**`cdns_mhdp8501_dt_parse` returns `true` on success:**
```c
return true;
```
But the caller checks `if (ret < 0)`. Since `true =3D=3D 1`, which is `>=3D=
 0`, this works accidentally, but returning a boolean from a function that =
uses `int` return type and `-EINVAL` error codes is confusing. Should retur=
n `0` on success.

**Lane mapping validation is ineffective:**
```c
lane_value =3D (data_lanes[i] >=3D 0 && data_lanes[i] <=3D 3) ? data_lanes[=
i] : 0;
```
Since `data_lanes[i]` is `u32`, the `>=3D 0` check is always true. Also, si=
lently clamping invalid values to 0 instead of returning an error is wrong =
=E2=80=94 an invalid lane mapping in DT should be an error.

**`endpoint` device_node leak:**
```c
endpoint =3D of_graph_get_endpoint_by_regs(np, 1, -1);
// ... used but never of_node_put(endpoint)
```
Missing `of_node_put(endpoint)` =E2=80=94 this leaks a refcount.

**`kmalloc_obj(*input_fmts)` usage:**
```c
input_fmts =3D kmalloc_obj(*input_fmts);
```
This uses the newer `kmalloc_obj` API which allocates based on the type of =
the argument. This should work but is worth verifying it compiles.

**`struct video_info` in `cdns_mhdp8501_device`:**
```c
struct video_info video_info;
```
This stores `bpc` and `color_fmt` in the device struct, but in the DP path =
these are only used between `atomic_check` and `atomic_enable`. This is fra=
gile since atomic state should be the source of truth =E2=80=94 storing it =
in the device struct means it could become stale. The HDMI path correctly u=
ses `conn_state->hdmi` directly.

**DP `cdns_dp_check_link_state` accesses connector state unsafely:**
```c
conn_state =3D connector->state;
crtc =3D conn_state->crtc;
...
crtc_state =3D crtc->state;
```
Accessing `connector->state` and `crtc->state` outside of an atomic commit =
or without holding appropriate locks is racy. The same issue exists in `cdn=
s_hdmi_handle_hotplug()`.

**HDMI `reset_pipe()` function:**
```c
static int reset_pipe(struct drm_crtc *crtc)
{
    state =3D drm_atomic_state_alloc(crtc->dev);
    ...
    crtc_state->connectors_changed =3D true;
    ret =3D drm_atomic_commit(state);
```
Performing a full modeset from a workqueue (hotplug handler) without `drm_m=
odeset_lock_all` is problematic. The `drm_modeset_acquire_init(&ctx, 0)` is=
 used but the retry loop for `-EDEADLK` is missing, which can cause deadloc=
ks. Compare with `intel_display_reset_finish()` or `drm_client_modeset_comm=
it_atomic()` for proper patterns.

**HDMI I2C adapter is limited but claims broad functionality:**
```c
static u32 cdns_hdmi_i2c_func(struct i2c_adapter *adapter)
{
    return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
}
```
But the `xfer` function only supports `SCDC_I2C_SLAVE_ADDRESS` (0x54). Adve=
rtising `I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL` when only a single slave addre=
ss is supported is misleading.

**HDMI `cdns_hdmi_i2c_read` reads uninitialized vars:**
```c
static int cdns_hdmi_i2c_read(struct cdns_mhdp8501_device *mhdp,
                              struct i2c_msg *msgs, int num)
{
    u8 addr, offset, *buf, len;
    ...
    for (i =3D 0; i < num; i++) {
        if (msgs[i].flags & I2C_M_RD) {
            addr =3D msgs[i].addr;
            buf =3D msgs[i].buf;
            len =3D msgs[i].len;
        } else {
            offset =3D msgs[i].buf[0];
        }
    }
    msg[0] =3D addr;
```
If no message has `I2C_M_RD` set, `addr`, `buf`, and `len` are used uniniti=
alized. There's no validation of message ordering.

**Missing `DRM_BRIDGE_OP_HDMI_AUDIO` for audio support**: The Kconfig selec=
ts `DRM_CDNS_AUDIO` but `bridge.ops` doesn't include `DRM_BRIDGE_OP_HDMI_AU=
DIO`, and no audio infoframe callbacks are implemented. Either drop the `DR=
M_CDNS_AUDIO` select or add audio support.

---
Generated by Claude Code Patch Reviewer