Claude review: drm/bridge: Add ITE IT6162 MIPI DSI to HDMI bridge driver

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

Patch Review

This is the main driver patch. There are a number of issues, ranging from outright bugs to things worth cleaning up.

**License header contradiction:**

> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.

The SPDX identifier says `GPL-2.0-only` but the boilerplate text says "version 2 of the License, or (at your option) any later version", which is `GPL-2.0-or-later`. These contradict each other. The SPDX identifier is what tools parse, so whichever is intended, the other should be made consistent. Probably simplest to just use the SPDX line and remove the boilerplate text entirely.

**Duplicate register offset definition:**

> +#define OFFSET_VERSION_L 0x03
> +#define OFFSET_VERSION_M 0x04
> +#define OFFSET_VERSION_H 0x03

`OFFSET_VERSION_L` and `OFFSET_VERSION_H` are both defined as 0x03. Given the bulk read of 6 bytes starting at 0x00 and the pattern of the chip ID offsets (0x00, 0x01, 0x02), `OFFSET_VERSION_H` should presumably be 0x05. Neither macro is actually used directly (the code does a bulk read), but this should still be fixed to avoid confusion.

**`it6162_infoblock_read` return type mismatch:**

> +static unsigned int it6162_infoblock_read(struct it6162 *it6162,
> +					  unsigned int reg)
> +{
> +	...
> +	if (err < 0) {
> +		dev_err(dev, "read failed rx reg[0x%x] err: %d", reg, err);
> +		return err;
> +	}

This function returns `unsigned int` but returns a negative error code on failure. The negative value gets implicitly converted to a large unsigned value. Every caller treats the result as a valid register value -- for example in `it6162_interrupt_handler`, `it6162_detect`, `it6162_hdcp_handler`, etc. If the I2C read fails, all the bit-field extractions will produce garbage results silently. The function should either return `int` and have callers check for errors, or use an output parameter pattern like `regmap_read` does.

**Uninitialized variable in `it6162_attach_dsi` -- this is a clear bug:**

> +	struct mipi_dsi_device *dsi;
> +	...
> +	for (int port = 0; port < 2; port++) {
> +		dsi_host = it6162_of_get_dsi_host_by_port(it6162, port);
> +		if (IS_ERR(dsi_host))
> +			continue;
> +
> +		mipi_cfg->en_port[port] = true;
> +
> +		if (!it6162->dsi) {
> +			dev_info(dev, "DSI host loaded paras for port %d", port);
> +			it6162->dsi = dsi;
> +			it6162_load_mipi_pars_from_port(it6162, port);
> +		}
> +
> +		dsi = devm_mipi_dsi_device_register_full(dev, dsi_host, &info);

On the first loop iteration (port 0), `dsi` is uninitialized when `it6162->dsi = dsi` executes. The `dsi` variable is only assigned a meaningful value by `devm_mipi_dsi_device_register_full` *after* this block. This stores a garbage pointer in `it6162->dsi`. It looks like the intent was for `it6162->dsi` to point to the first successfully registered DSI device, so the assignment should be moved after the register call.

**Shallow copy of `struct drm_connector`:**

> +static void it6162_bridge_atomic_enable(struct drm_bridge *bridge,
> +					struct drm_atomic_state *state)
> +{
> +	...
> +	it6162->connector = *connector;

This performs a shallow copy of the entire `struct drm_connector`, which contains mutexes, list heads, refcounted pointers, and many other fields that must not be copied. The driver should store a pointer to the connector instead, or better yet, just use the local `connector` pointer throughout this function since it doesn't appear to be needed outside of `atomic_enable`.

**Uninitialized `avi_info` passed to timing function:**

> +	struct hdmi_avi_infoframe avi_info;
> +	...
> +	if (it6162->is_hdmi) {
> +		ret = drm_hdmi_avi_infoframe_from_display_mode(&avi_info,
> +							       connector,
> +							       mode);
> +		if (ret)
> +			drm_err(it6162->drm, "Failed to setup AVI infoframe: %d", ret);
> +	}
> +
> +	...
> +	it6162_mipi_set_d2v_video_timing(it6162, &vm, &avi_info);

When connecting to a DVI monitor (`is_hdmi` is false), `avi_info` is never initialized but is still passed to `it6162_mipi_set_d2v_video_timing`, which reads `avi_info->video_code`, `avi_info->picture_aspect`, `avi_info->pixel_repeat`, and `avi_info->colorspace`. This reads uninitialized stack data. The struct should be zeroed at declaration or the DVI case should be handled separately.

**Missing `DRM_BRIDGE_OP_HPD`:**

> +	it6162->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
> +			     DRM_BRIDGE_OP_MODES;

The driver implements `hpd_enable`, `hpd_disable` callbacks and calls `drm_bridge_hpd_notify` from its interrupt handler, but does not set `DRM_BRIDGE_OP_HPD` in the bridge ops. Without this flag, the DRM core will not call `hpd_enable`/`hpd_disable` and the HPD notification path is effectively dead code. `DRM_BRIDGE_OP_HPD` needs to be added.

**Swapped sample width mappings:**

> +	switch (hparms->sample_width) {
> +	case 16:
> +		config->sample_width = WORD_LENGTH_16BIT;
> +		break;
> +	case 24:
> +		config->sample_width = WORD_LENGTH_18BIT;
> +		break;
> +	case 20:
> +		config->sample_width = WORD_LENGTH_24BIT;
> +		break;

Is this intentional? 24-bit sample width maps to `WORD_LENGTH_18BIT` (0x1) and 20-bit maps to `WORD_LENGTH_24BIT` (0x3). This looks like the 20 and 24 cases are swapped.

**Regulator error handling leaks enables:**

> +static int it6162_platform_set_power(struct it6162 *it6162)
> +{
> +	...
> +	if (it6162->ivdd) {
> +		err = regulator_enable(it6162->ivdd);
> +		if (err) { ... return err; }
> +	}
> +	if (it6162->pwr1833) {
> +		err = regulator_enable(it6162->pwr1833);
> +		if (err) { ... return err; }
> +	}
> +	if (it6162->ovdd) {
> +		err = regulator_enable(it6162->ovdd);
> +		if (err)
> +			return err;
> +	}

If `pwr1833` enable fails, `ivdd` is left enabled. If `ovdd` enable fails, both `ivdd` and `pwr1833` are left enabled. Error paths should disable any regulators that were successfully enabled.

**Regulator acquisition silently NULLs errors:**

> +	it6162->ivdd = devm_regulator_get(dev, "ivdd");
> +	if (IS_ERR(it6162->ivdd)) {
> +		dev_info(dev, "ivdd regulator not found");
> +		it6162->ivdd = NULL;
> +	}

Since the DT binding lists `ivdd-supply`, `ovdd-supply`, and `ovdd1833-supply` as properties (not required, but present in the example), silently NULLing them hides probe-ordering issues. Consider using `devm_regulator_get_optional` if they're truly optional, or make them required and propagate errors (especially `-EPROBE_DEFER`).

**`it6162_infoblock_host_set` ignores error from wait:**

> +static int it6162_infoblock_host_set(struct it6162 *it6162, u8 setting)
> +{
> +	dev_info(it6162->dev, "%s %x", __func__, setting);
> +	it6162_infoblock_write(it6162, OFFSET_HOST_SETTING, setting);
> +	/*wait command complete*/
> +	it6162_infoblock_wait_complete(it6162);
> +
> +	return 0;
> +}

The return value of `it6162_infoblock_wait_complete` is ignored; the function always returns 0. Callers of `it6162_infoblock_host_set` also ignore its return value, so timeouts communicating with the firmware are silently swallowed.

**`dev_info` noise:** The driver has many `dev_info`/`drm_info` calls for routine operations (entering functions, interrupt status, etc.). These should be `dev_dbg`/`drm_dbg` since they'll spam the kernel log during normal operation. For example:

> +	drm_info(it6162->drm, "evnet change");

Also a typo: "evnet" should be "event".

> +	dev_err(dev, "wait 6162 rdy %x %x %u", status, regEF, i);

This uses `dev_err` for what is a normal wait loop iteration. Should be `dev_dbg`.

**`it6162_bridge_read_edid` returns 0 instead of NULL:**

> +	if (!edid) {
> +		drm_err(it6162->drm, "failed to read EDID");
> +		return 0;
> +	}

The function returns `const struct drm_edid *`, so returning 0 works (it's NULL), but returning `NULL` explicitly would be clearer.

**Vendor-specific DT properties used but not in binding:** The driver reads several properties from device tree endpoints (`ite,mipi-dsi-phy-pn-swap`, `ite,mipi-dsi-phy-link-swap`, `ite,mipi-dsi-mode-video-sync-pulse`, `ite,mipi-dsi-clock-non-continous`) and a node property (`ite,hdcp-version`), none of which are documented in the YAML binding. These need to be added to the binding, or preferably, standard DSI properties should be used where they exist (e.g., `clock-non-continuous` is a standard MIPI DSI property).

**`__maybe_unused` on `it6162_poweron`/`it6162_poweroff`:** These functions are marked `__maybe_unused` but `it6162_poweron` is called directly from `it6162_attach_dsi`. The `__maybe_unused` annotation suggests these were intended for PM callbacks that aren't wired up. `it6162_poweroff` is indeed never called (the remove function doesn't call it), which means regulators are never disabled on removal.

**`it6162_parse_dt` return type mismatch:**

> +static unsigned int it6162_parse_dt(struct it6162 *it6162)

Returns `unsigned int` but always returns 0 and the caller doesn't check the return value anyway. Should be `void` or `int`.

---
Generated by Claude Code Patch Reviewer