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: ite-it6263: Move chip initialization code from probe to atomic_enable
Date: Tue, 05 May 2026 09:39:38 +1000
Message-ID: <review-patch1-20260501061200.20129-1-biju.das.jz@bp.renesas.com>
In-Reply-To: <20260501061200.20129-1-biju.das.jz@bp.renesas.com>
References: <20260501061200.20129-1-biju.das.jz@bp.renesas.com> <20260501061200.20129-1-biju.das.jz@bp.renesas.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

**Adding `reset_gpio` to struct (line 203 of patched file):**
```c
struct gpio_desc *reset_gpio;
```
Straightforward. Needed to make the GPIO accessible beyond probe scope.

**Initialization block in `atomic_enable` (lines 607=E2=80=93614):**
```c
it6263_hw_reset(it->reset_gpio);

ret =3D it6263_lvds_set_i2c_addr(it);
if (ret)
    dev_err(it->dev, "failed to set I2C addr\n");

it6263_lvds_config(it);
it6263_hdmi_config(it);
```

- The `it6263_hw_reset()` call is fine here =E2=80=94 it already handles a =
NULL `reset_gpio` gracefully.

- **Issue: Continuing after `it6263_lvds_set_i2c_addr` failure.** If this c=
all fails, `it6263_lvds_config()` will write to the LVDS regmap, which uses=
 the I2C address that was supposed to be configured by `it6263_lvds_set_i2c=
_addr()`. Looking at the code, the LVDS I2C device is created with `LVDS_IN=
PUT_CTRL_I2C_ADDR` as a fixed address via `devm_i2c_new_dummy_device()` in =
probe, and `it6263_lvds_set_i2c_addr()` tells the *HDMI chip* to expose the=
 LVDS sub-block at that address. So if this fails, subsequent LVDS regmap o=
perations will be talking to an address where nothing is responding, and yo=
u'll get I2C errors. The function should `return` early here instead of con=
tinuing:

  ```c
  ret =3D it6263_lvds_set_i2c_addr(it);
  if (ret) {
      dev_err(it->dev, "failed to set I2C addr\n");
      return;
  }
  ```

**Removal from probe (lines 879=E2=80=93882 area of patched file):**
```c
-	it6263_hw_reset(reset_gpio);
-
-	ret =3D it6263_lvds_set_i2c_addr(it);
-	if (ret)
-		return dev_err_probe(dev, ret, "failed to set I2C addr\n");
```

The removal is clean. However, note that `it6263_parse_dt()` (called at lin=
e 880) does not access the chip hardware, and `devm_i2c_new_dummy_device()`=
 at line 884 reserves the I2C address but doesn't communicate with the chip=
, so this ordering is fine.

```c
-	it6263_lvds_config(it);
-	it6263_hdmi_config(it);
```

Also cleanly removed.

**Summary:** The patch is fundamentally correct in its approach. The main a=
ctionable feedback is:

1. **Return early on `it6263_lvds_set_i2c_addr` failure in `atomic_enable`*=
* rather than proceeding with LVDS/HDMI config that will fail.

2. **Verify that `it6263_detect()` works without `it6263_hdmi_config()`** h=
aving been called. The DDC master select (`MASTER_SEL_HOST`) is only writte=
n in `it6263_read_edid`, not in `it6263_hdmi_config`, so EDID reads may be =
fine. But `it6263_detect()` reads `HDMI_REG_SYS_STATUS` for HPD =E2=80=94 c=
onfirm this register is readable in the chip's power-on-reset state without=
 the soft-reset sequence from `it6263_hdmi_config()`.

---
Generated by Claude Code Patch Reviewer