[PATCH v4] drm/bridge: ite-it6263: Move chip initialization code from probe to atomic_enable

[PATCH v4] drm/bridge: ite-it6263: Move chip initialization code from probe to atomic_enable

[Biju]

From: Biju Das <biju.das.jz@bp.renesas.com>

On the RZ/G3L SMARC EVK, suspend to RAM powers down the ITE IT6263 chip.
The display controller driver's system PM callbacks invoke
drm_mode_config_helper_{suspend,resume}, which in turn call the bridge's
atomic_{disable,enable} callbacks to handle suspend/resume for the bridge
without dedicated PM ops.

To support proper reinitialization after power loss, move reset_gpio into
the it6263 struct so it is accessible beyond probe time. Relocate
it6263_hw_reset(), it6263_lvds_set_i2c_addr(), it6263_lvds_config() and
it6263_hdmi_config() from probe to atomic_enable, ensuring the chip is
fully reset and reconfigured on every enable, including after a
suspend/resume cycle.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
Tested s2idle, s2ram and hotplug on Renesas RZ/G3L SMARC EVK platform.
v3->v4:
 * Updated commit header.
v2->v3:
 * Updated commit header and description.
 * Dropped it6263_bridge_{init,uninit}().
 * Restored regulator_bulk_enable in probe().
 * Dropped the variable powered, supplies and num_supplies from
   struct it6263.
 * Added reset, I2C address configuration, and LVDS/HDMI initialisation to
   the atomic_enable callback so that the hardware is fully reinitialised
   after each power cycle. Correspondingly, remove these steps from probe,
   since they are no longer needed there.
 * Dropped the remove callback as it is not needed.
v1->v2:
 * Dropped system PM callbacks instead using bridge's
   atomic_{disable,enable} callbacks to handle suspend/resume.
---
 drivers/gpu/drm/bridge/ite-it6263.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6263.c b/drivers/gpu/drm/bridge/ite-it6263.c
index 2ea49245e700..4a8268d0eac2 100644
--- a/drivers/gpu/drm/bridge/ite-it6263.c
+++ b/drivers/gpu/drm/bridge/ite-it6263.c
@@ -200,6 +200,7 @@ struct it6263 {
 	struct regmap *lvds_regmap;
 	struct drm_bridge bridge;
 	struct drm_bridge *next_bridge;
+	struct gpio_desc *reset_gpio;
 	int lvds_data_mapping;
 	bool lvds_dual_link;
 	bool lvds_link12_swap;
@@ -603,6 +604,15 @@ static void it6263_bridge_atomic_enable(struct drm_bridge *bridge,
 	bool pclk_high;
 	int i, ret;
 
+	it6263_hw_reset(it->reset_gpio);
+
+	ret = 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);
+
 	connector = drm_atomic_get_new_connector_for_encoder(state,
 							     bridge->encoder);
 	crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
@@ -838,7 +848,6 @@ static const struct drm_bridge_funcs it6263_bridge_funcs = {
 static int it6263_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
-	struct gpio_desc *reset_gpio;
 	struct it6263 *it;
 	int ret;
 
@@ -856,9 +865,9 @@ static int it6263_probe(struct i2c_client *client)
 		return dev_err_probe(dev, PTR_ERR(it->hdmi_regmap),
 				     "failed to init I2C regmap for HDMI\n");
 
-	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
-	if (IS_ERR(reset_gpio))
-		return dev_err_probe(dev, PTR_ERR(reset_gpio),
+	it->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(it->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(it->reset_gpio),
 				     "failed to get reset gpio\n");
 
 	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(it6263_supplies),
@@ -870,12 +879,6 @@ static int it6263_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
-	it6263_hw_reset(reset_gpio);
-
-	ret = it6263_lvds_set_i2c_addr(it);
-	if (ret)
-		return dev_err_probe(dev, ret, "failed to set I2C addr\n");
-
 	it->lvds_i2c = devm_i2c_new_dummy_device(dev, client->adapter,
 						 LVDS_INPUT_CTRL_I2C_ADDR);
 	if (IS_ERR(it->lvds_i2c))
@@ -888,9 +891,6 @@ static int it6263_probe(struct i2c_client *client)
 		return dev_err_probe(dev, PTR_ERR(it->lvds_regmap),
 				     "failed to init I2C regmap for LVDS\n");
 
-	it6263_lvds_config(it);
-	it6263_hdmi_config(it);
-
 	i2c_set_clientdata(client, it);
 
 	it->bridge.of_node = dev->of_node;
-- 
2.43.0

Claude review: drm/bridge: ite-it6263: Move chip initialization code from probe to atomic_enable

[Claude Code Review Bot]

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–614):**
```c
it6263_hw_reset(it->reset_gpio);

ret = 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 — it already handles a NULL `reset_gpio` gracefully.

- **Issue: Continuing after `it6263_lvds_set_i2c_addr` failure.** If this call 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_INPUT_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 operations will be talking to an address where nothing is responding, and you'll get I2C errors. The function should `return` early here instead of continuing:

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

**Removal from probe (lines 879–882 area of patched file):**
```c
-	it6263_hw_reset(reset_gpio);
-
-	ret = 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 line 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 actionable 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()`** having been called. The DDC master select (`MASTER_SEL_HOST`) is only written 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 — confirm 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

Claude review: drm/bridge: ite-it6263: Move chip initialization code from probe to atomic_enable

[Claude Code Review Bot]

Overall Series Review

Subject: drm/bridge: ite-it6263: Move chip initialization code from probe to atomic_enable
Author: Biju <biju.das.au@gmail.com>
Patches: 1
Reviewed: 2026-05-05T09:39:38.356348

---

This is a single-patch series that moves IT6263 bridge chip initialization (hardware reset, I2C address configuration, LVDS config, HDMI config) from `probe()` to `atomic_enable()`. The motivation is sound: on the RZ/G3L SMARC EVK, suspend-to-RAM powers down the IT6263, and the display controller's PM callbacks invoke `atomic_disable`/`atomic_enable`, so the chip must be fully reinitialized on each enable cycle.

The approach is reasonable and follows the pattern used by other bridge drivers that need to survive power loss. The patch is small and well-scoped. However, there are a few concerns worth raising:

**Concern 1 — Error handling regression in `atomic_enable`.** In `probe()`, the original `it6263_lvds_set_i2c_addr()` failure was fatal and returned an error via `dev_err_probe()`. In `atomic_enable()`, the failure is downgraded to a `dev_err()` log and execution continues. If the I2C address setup fails, subsequent writes to the LVDS regmap will go to the wrong address, likely causing silent corruption or a hang. The `atomic_enable` callback is void-returning so a direct return isn't possible, but the function should at least bail out early rather than proceeding with configuration that depends on the I2C address being set.

**Concern 2 — `it6263_detect()` / EDID reads before `atomic_enable`.** The `it6263_detect()` and `it6263_read_edid()` functions access the HDMI regmap to read HPD status and EDID. After this patch, the chip is no longer initialized in `probe()`, so these functions may be called against an uninitialized chip (e.g., during connector probing at boot). The `devm_regulator_bulk_get_enable` still powers the chip in probe, and the GPIO is requested as `GPIOD_OUT_LOW` (de-asserted reset on active-low), so the chip *may* come up in a usable-enough state for I2C register reads. But the HDMI config registers (e.g., `HDMI_REG_DDC_MASTER_CTRL`) are never written until `atomic_enable` now. This deserves a closer look at whether detect/EDID work reliably without `it6263_hdmi_config()` being called first.

**Concern 3 — No `atomic_disable` counterpart.** The `atomic_disable` only mutes audio/video and powers down the AFE driver. It does not assert the reset GPIO. This means on a normal disable/enable cycle (not involving a full power loss), the chip gets a gratuitous hardware reset pulse followed by full reinitialization. This should be harmless but adds ~46ms of latency to every enable.

---

---
Generated by Claude Code Patch Reviewer