Claude review: Re: [PATCH v3 2/2] drm/bridge: This patch add new DRM bridge driver for LT9611C(EX/UXD) chip

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Subject line**: Same issue — should be `drm/bridge: lontium-lt9611c: Add DSI-to-HDMI bridge driver`.

**File permissions**: The new file is created with `100755` (executable). C source files should be `100644`.
```
 create mode 100755 drivers/gpu/drm/bridge/lontium-lt9611c.c
```

#### Critical: Missing DRM_BRIDGE_OP_HDMI flag

The driver implements `hdmi_write_avi_infoframe`, `hdmi_clear_avi_infoframe`, `hdmi_write_hdmi_infoframe`, `hdmi_clear_hdmi_infoframe`, `hdmi_write_spd_infoframe`, `hdmi_clear_spd_infoframe` but only sets:
```c
lt9611c->bridge.ops = DRM_BRIDGE_OP_DETECT |
		DRM_BRIDGE_OP_EDID |
		DRM_BRIDGE_OP_HPD |
		DRM_BRIDGE_OP_HDMI_AUDIO;
```

The `DRM_BRIDGE_OP_HDMI` flag is mandatory for AVI and HDMI infoframe callbacks, and `DRM_BRIDGE_OP_HDMI_SPD_INFOFRAME` is needed for SPD infoframes. Without these flags, the framework will never invoke these callbacks. Compare with the lt9611 driver which correctly sets:
```c
lt9611->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
		     DRM_BRIDGE_OP_HPD | DRM_BRIDGE_OP_MODES |
		     DRM_BRIDGE_OP_HDMI | DRM_BRIDGE_OP_HDMI_AUDIO |
		     DRM_BRIDGE_OP_HDMI_SPD_INFOFRAME;
```

Additionally, when `DRM_BRIDGE_OP_HDMI` is set, the bridge must also set:
- `bridge.vendor` (e.g. `"Lontium"`)
- `bridge.product` (e.g. `"LT9611C"`)
- `bridge.supported_formats`
- `bridge.max_bpc`

None of these are set.

#### Critical: Buffer overflow in infoframe write functions

All four `hdmi_write_*_infoframe` functions copy `len` bytes into a fixed 16-byte array:
```c
static int lt9611c_hdmi_write_avi_infoframe(struct drm_bridge *bridge,
					    const u8 *buffer, size_t len)
{
	...
	u8 avi_infoframe_cmd[16] = {0x57, 0x48, 0x35, 0x3a, 0x01};
	...
	for (i = 0; i <  len; i++)
		avi_infoframe_cmd[i + 5] = buffer[i];
```
With 5 bytes already used for the command prefix, only 11 bytes remain. An AVI infoframe is typically 17 bytes (4-byte header + 13-byte body), and HDMI Vendor Specific infoframes can be larger. This is a stack buffer overflow. The buffer needs to be either dynamically sized or properly bounds-checked. The same pattern repeats in `lt9611c_hdmi_write_audio_infoframe`, `lt9611c_hdmi_write_spd_infoframe`, and `lt9611c_hdmi_write_hdmi_infoframe`.

The `lt9611c_read_write_flow` function also limits params to `0xdd - 0xb0` (45) bytes, so a larger buffer (e.g., 48 bytes) could be used with a `len` check capped to 43 bytes.

#### Improper error codes

```c
static int lt9611c_read_write_flow(...)
{
	...
	if (temp != 0x01)
		return -1;
	...
	if (temp != 0x02)
		return -2;
```
These should return proper errno values like `-ETIMEDOUT` or `-EIO`. The callers check `ret < 0` which works, but `-1` is `-EPERM` and `-2` is `-ENOENT`, which are misleading.

Similarly:
```c
static int lt9611c_upgrade_result(...)
{
	...
	if (crc_result != lt9611c->fw_crc) {
		...
		return -1;
	}
```
Should be `-EIO` or similar.

#### Missing atomic state helpers

The bridge funcs do not include `atomic_duplicate_state` and `atomic_reset_state` callbacks. The lt9611 driver (and most modern bridge drivers) set:
```c
.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
.atomic_reset = drm_atomic_helper_bridge_reset,
```
These are needed when using `DRM_BRIDGE_OP_HDMI`.

#### Missing `atomic_disable` callback

The driver implements `atomic_enable` but not `atomic_disable`. When the display is turned off, nothing is sent to the chip to stop video output. The lt9611 driver properly implements this.

#### Missing `DRM_BRIDGE_OP_MODES`

The driver does not implement `mode_valid` or set `DRM_BRIDGE_OP_MODES`. While `hdmi_tmds_char_rate_valid` provides some rate limiting, a `mode_valid` callback could reject modes the chip cannot handle more cleanly.

#### Deprecated PM macro

```c
static const struct dev_pm_ops lt9611c_bridge_pm_ops = {
	SET_SYSTEM_SLEEP_PM_OPS(lt9611c_bridge_suspend,
				lt9611c_bridge_resume)
};
```
`SET_SYSTEM_SLEEP_PM_OPS` is deprecated. Use `DEFINE_SIMPLE_DEV_PM_OPS` and `pm_sleep_ptr()`:
```c
static DEFINE_SIMPLE_DEV_PM_OPS(lt9611c_bridge_pm_ops,
				lt9611c_bridge_suspend,
				lt9611c_bridge_resume);
...
.pm = pm_sleep_ptr(&lt9611c_bridge_pm_ops),
```

#### i2c_device_id table missing `const`

```c
static struct i2c_device_id lt9611c_id[] = {
```
Should be `static const struct i2c_device_id`.

#### Regulator disable order is wrong

```c
static int lt9611c_regulator_disable(struct lt9611c *lt9611c)
{
	ret = regulator_disable(lt9611c->supplies[0].consumer);  /* vcc */
	...
	ret = regulator_disable(lt9611c->supplies[1].consumer);  /* vdd */
```
The enable order is vcc (3.3V) then vdd (1.8V). The disable order should be the reverse: vdd first, then vcc. Also, both `lt9611c_regulator_enable` and `lt9611c_regulator_disable` exist but `regulator_bulk_disable()` is used in the error path and remove function. Pick one approach and be consistent. Consider using `regulator_bulk_enable`/`regulator_bulk_disable` throughout if sequencing doesn't matter, or use the manual approach consistently if it does.

#### Firmware upgrade at probe time with EPROBE_DEFER on missing firmware

```c
static int lt9611c_prepare_firmware_data(struct lt9611c *lt9611c)
{
	ret = request_firmware(&lt9611c->fw, FW_FILE, dev);
	if (ret) {
		...
		return -EPROBE_DEFER;
	}
```
Returning `-EPROBE_DEFER` when firmware loading fails will cause the kernel to retry probe indefinitely. If the firmware file doesn't exist, this will never succeed. This should return the actual error from `request_firmware()`, or use `firmware_request_nowarn()` with a limited retry strategy.

#### Sysfs firmware upgrade interface

```c
static DEVICE_ATTR_RW(lt9611c_firmware);
```
The sysfs attribute name will be `lt9611c_firmware`, but sysfs attribute naming conventions suggest just `firmware` or `firmware_version`. More importantly, the store function ignores the input buffer content entirely — any write triggers a firmware upgrade:
```c
static ssize_t lt9611c_firmware_store(struct device *dev, ...)
{
	...
	ret = lt9611c_firmware_upgrade(lt9611c);
```
This is fragile and unusual. A firmware upgrade triggered by any sysfs write (including accidental ones) is risky. Consider at minimum validating the input (e.g., expecting "1" or "update").

#### `lt9611c_firmware_store` calls `lt9611c_reset` after unlock

```c
out:
	lt9611c_unlock(lt9611c);
	lt9611c_reset(lt9611c);
```
`lt9611c_reset()` does register writes but the MCU was just unlocked (re-enabled). There may be a race between the MCU resuming execution and the reset sequence. This also calls reset unconditionally even on failure, and calls it twice on success (once inside the `if (ret < 0)` goto path isn't taken, the reset before `lt9611c_upgrade_result` already happened).

#### `bridge_to_lt9611c_const` is unnecessary boilerplate

```c
/*read only*/
static const struct lt9611c *bridge_to_lt9611c_const(const struct drm_bridge *bridge)
{
	return container_of(bridge, const struct lt9611c, bridge);
}
```
This is only used in one place (`lt9611c_hdmi_tmds_char_rate_valid`). The `const` correctness approach is fine but the `/*read only*/` comment is unnecessary, and the function name doesn't match kernel conventions. Consider just casting in the one callsite or dropping the `const` qualification.

#### IRQ handler returns `IRQ_HANDLED` for spurious interrupts

```c
if (!(irq_status & BIT(0))) {
	mutex_unlock(&lt9611c->ocm_lock);
	return IRQ_HANDLED;
}
```
If the interrupt status bit is not set, the interrupt was not from this device and should return `IRQ_NONE` to allow shared interrupt handling to work properly.

#### Residual comments

Several comments remain that should have been removed per the v3 changelog:
```c
REG_SEQ0(0xe103, 0x3f), //fifo rst
...
/*clear interrupt*/
...
//hardware need delay
```
These should either be removed or converted to proper `/* */` style. The `//` comment style is discouraged in kernel code.

#### `video_timing_set_cmd` inconsistent hex case

```c
video_timing_set_cmd[17] = vactive & 0xFF;
```
All other masks use lowercase `0xff`. This one uses `0xFF`. The v3 changelog says "Lowercase all hex values" but this was missed.

#### `MODULE_LICENSE` mismatch

```c
MODULE_LICENSE("GPL v2");
```
The SPDX header says `GPL-2.0` which maps to `"GPL v2"`. However, the modern convention is to use `"GPL"` (which means "GPL v2 or later"). If the intent is GPL-2.0-only, the SPDX should be `GPL-2.0-only` and `MODULE_LICENSE("GPL")` is still correct for GPL-2.0-only. The `"GPL v2"` string is deprecated — use `"GPL"`.

---
Generated by Claude Code Patch Reviewer