Claude review: drm/bridge: Add Lontium LT9611C(EX/UXD) MIPI DSI to HDMI driver

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

Patch Review

#### Critical Issues

**1. Missing atomic state helper callbacks (will crash or fail to enable)**

The driver sets `DRM_BRIDGE_OP_HDMI` and implements `atomic_enable`, but does not implement the required atomic state management callbacks. The existing `lontium-lt9611.c` driver correctly provides these:
```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,
```
Without these, the HDMI state helper infrastructure will not function correctly. These must be added to `lt9611c_bridge_funcs`.

**2. Double-free of IRQ: `devm_request_threaded_irq` + manual `free_irq`**

The IRQ is allocated with `devm_request_threaded_irq` at line ~1453, but `free_irq` is called manually in both the `err_remove_bridge` error path and in `lt9611c_remove`:
```c
err_remove_bridge:
	free_irq(client->irq, lt9611c);  /* manual free of devm resource */
```
```c
static void lt9611c_remove(struct i2c_client *client)
{
	free_irq(client->irq, lt9611c);  /* double-free: devm will also free */
```
Either use `devm_request_threaded_irq` and let devm handle teardown, or use `request_threaded_irq` with manual cleanup. Don't mix them.

**3. Double-removal of bridge: `devm_drm_bridge_add` + manual `drm_bridge_remove`**

Same pattern: `devm_drm_bridge_add` is called at line ~1476, but `drm_bridge_remove` is called in `err_remove_bridge`. `devm_drm_bridge_add` registers a devm action that calls `drm_bridge_remove` automatically. The manual call causes a double-removal.

**4. of_node reference leak from `lt9611c_parse_dt` on failure**

When `lt9611c_parse_dt` fails, the probe function returns directly:
```c
ret = lt9611c_parse_dt(dev, lt9611c);
if (ret)
    return dev_err_probe(dev, ret, "failed to parse device tree\n");
```
But `lt9611c_parse_dt` may have already acquired references via `of_graph_get_remote_node` for `dsi0_node` (and possibly `dsi1_node`) before failing at `drm_of_find_panel_or_bridge`. Those references are never released because the code returns instead of jumping to `err_of_put`. The early returns before the goto-based error cleanup section (for `lt9611c_gpio_init` and `lt9611c_regulator_init`) have the same problem if `lt9611c_parse_dt` succeeded.

**5. `lt9611c_upgrade_result` returns bare `-1`**

```c
if (crc_result != fw_crc) {
    ...
    return -1;
}
```
Should return a proper errno like `-EIO`.

#### Moderate Issues

**6. Missing `atomic_disable` callback**

The driver has `atomic_enable` but no `atomic_disable`. Compare with the existing lt9611 driver which implements both. Even if the on-chip MCU handles everything, the framework expects symmetry. At minimum, consider if any state needs to be cleaned up on disable (e.g., clearing infoframes).

**7. `SET_SYSTEM_SLEEP_PM_OPS` is deprecated**

```c
static const struct dev_pm_ops lt9611c_bridge_pm_ops = {
    SET_SYSTEM_SLEEP_PM_OPS(lt9611c_bridge_suspend,
                            lt9611c_bridge_resume)
};
```
Use the modern pattern (as `lontium-lt8912b.c` does):
```c
static DEFINE_SIMPLE_DEV_PM_OPS(lt9611c_bridge_pm_ops,
                                lt9611c_bridge_suspend,
                                lt9611c_bridge_resume);
```
And reference it via `pm_sleep_ptr(&lt9611c_bridge_pm_ops)` in the driver struct.

**8. `i2c_device_id` table missing `const`**

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

**9. `lt9611c_block_erase` ignores erase failures**

The busy-wait loop breaks after 50 iterations but doesn't return an error:
```c
if (i > 50)
    break;
```
If flash erase fails, the driver proceeds to write firmware on top of unerased flash, silently producing corrupted firmware. The function should return `int` and propagate the timeout.

**10. `lt9611c_write_data` doesn't poll flash write completion**

After `lt9611c_sram_to_flash`, there's no check that the page write completed before proceeding to the next page. Compare with the erase path which at least polls `read_flash_reg_status`.

**11. Firmware sysfs attribute ignores input**

```c
static ssize_t lt9611c_firmware_store(struct device *dev, struct device_attribute *attr,
                                      const char *buf, size_t len)
{
    ...
    ret = lt9611c_firmware_upgrade(lt9611c);
```
The `buf` parameter is completely ignored. Any write triggers a full firmware flash. Consider at minimum requiring a specific value (e.g., "1") to trigger the upgrade to prevent accidental writes.

**12. Inconsistent use of `sizeof` vs `ARRAY_SIZE` for u8 arrays**

In `lt9611c_hdmi_audio_prepare`:
```c
ret = lt9611c_read_write_flow(lt9611c, audio_cmd, sizeof(audio_cmd),
                              data, sizeof(data));
```
Elsewhere the driver uses `ARRAY_SIZE`. For `u8[]` these are equivalent, but using `ARRAY_SIZE` consistently avoids bugs if the type ever changes.

#### Minor Issues

**13. `lt9611c_regulator_init` has unnecessary temporary variable**

```c
ret = devm_regulator_bulk_get(dev, 2, lt9611c->supplies);
return ret;
```
Can just be `return devm_regulator_bulk_get(...)`.

**14. Stale numbered comments in `lt9611c_firmware_upgrade`**

```c
/*1. load firmware*/
...
/*2. check size*/
...
/*3. calculate crc8 */
...
/*5. check upgrade of result*/
```
Comment 4 is missing (was presumably the write+erase step). Either fix the numbering or remove these comments entirely.

**15. `bridge_to_lt9611c_const` with `/*read only*/` comment**

```c
/*read only*/
static const struct lt9611c *bridge_to_lt9611c_const(const struct drm_bridge *bridge)
```
The comment adds nothing — the `const` qualifier already conveys the intent. Remove the comment.

**16. Regmap error checking in `lt9611c_read_write_flow`**

The `regmap_write` calls for the initial protocol handshake (writing to 0xe0de, writing parameters to 0xe0b0+i) don't check return values. A failed I2C transaction here would go unnoticed.

---
Generated by Claude Code Patch Reviewer