[PATCH v3] drm/bridge: ite-it6263: Support power cycle in runtime

[PATCH v3] drm/bridge: ite-it6263: Support power cycle in runtime

[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.
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 4f3ebb7af4d4..efb8aacef8ff 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;
@@ -840,7 +850,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;
 
@@ -858,9 +867,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),
@@ -872,12 +881,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))
@@ -890,9 +893,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

Re: [PATCH v3] drm/bridge: ite-it6263: Support power cycle in runtime

[Liu Ying]

Hi Biju,

On Tue, Apr 21, 2026 at 11:53:32AM +0100, Biju wrote:
> 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.
> 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(-)

The subject no longer summaries what this patch does.
Can you change it to be something like:
drm/bridge: ite-it6263: Move chip initialization code from probe to atomic_enable
?

Otherwise, I'll provide my R-b tag.

--
Regards,
Liu Ying

RE: [PATCH v3] drm/bridge: ite-it6263: Support power cycle in runtime

[Biju Das]

Hi Liu Ying,

> -----Original Message-----
> From: Liu Ying <victor.liu@nxp.com>
> Sent: 22 April 2026 07:05
> Subject: Re: [PATCH v3] drm/bridge: ite-it6263: Support power cycle in runtime
> 
> Hi Biju,
> 
> On Tue, Apr 21, 2026 at 11:53:32AM +0100, Biju wrote:
> > 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.
> > 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(-)
> 
> The subject no longer summaries what this patch does.
> Can you change it to be something like:
> drm/bridge: ite-it6263: Move chip initialization code from probe to atomic_enable ?
> 
> Otherwise, I'll provide my R-b tag.

OK for me. Will incorporate this in v4.

I will wait for feedback from others if any, before posting v4.

Cheers,
Biju

Claude review: drm/bridge: ite-it6263: Support power cycle in runtime

[Claude Code Review Bot]

Overall Series Review

Subject: drm/bridge: ite-it6263: Support power cycle in runtime
Author: Biju <biju.das.au@gmail.com>
Patches: 3
Reviewed: 2026-04-23T08:33:48.760925

---

This is a single patch that moves IT6263 hardware reset and initialization from `probe` to `atomic_enable`, enabling the chip to be fully reinitialized after a suspend/resume power cycle. The approach of leveraging the existing bridge `atomic_{disable,enable}` callbacks instead of adding dedicated PM ops is reasonable and matches how other bridge drivers handle this.

However, the patch has a **significant issue with regmap cache coherency** after a power cycle, and a **minor error-handling concern** in the atomic_enable path. Without addressing the regmap cache, this patch will produce incorrect hardware configuration on resume (the second and subsequent enable cycles after a power-loss event).

**Verdict: Needs revision** — the regmap cache staleness issue is a correctness bug that can cause silent misconfigurations after suspend/resume, which is the exact scenario this patch aims to fix.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/bridge: ite-it6263: Support power cycle in runtime

[Claude Code Review Bot]

Patch Review

**1. Regmap cache stale after power cycle (bug)**

Both the HDMI and LVDS regmaps are configured with `REGCACHE_MAPLE` (lines 305, 344 of the original). After `it6263_hw_reset()` resets the chip in `atomic_enable`, all hardware registers return to their default values, but the regmap caches still hold the values from the previous enable cycle.

This is a problem because `it6263_lvds_config()` → `it6263_lvds_reset()`, `it6263_lvds_set_interface()`, `it6263_lvds_set_afe()`, and `it6263_lvds_sys_cfg()` all use `regmap_write_bits()`, which performs read-modify-write against the cached value. For example:

```c
/* it6263_lvds_set_interface */
regmap_write_bits(it->lvds_regmap, LVDS_REG_2C, REG_COL_DEP, BIT8);
regmap_write_bits(it->lvds_regmap, LVDS_REG_2C, OUT_MAP, fmt);
regmap_write_bits(it->lvds_regmap, LVDS_REG_2C, DMODE, DISO);
```

On the second `atomic_enable` (after resume), `LVDS_REG_2C` cache holds the value written during the first enable. The actual hardware has its power-on-reset default. The `write_bits` call will preserve the non-masked bits from the stale cache, which may differ from the actual hardware default. The same applies to `LVDS_REG_0B`, `LVDS_REG_52`, and `HDMI_REG_GCP`.

After `it6263_hw_reset()`, the patch should add:
```c
regcache_mark_dirty(it->hdmi_regmap);
regcache_mark_dirty(it->lvds_regmap);
```

This ensures the cache knows it's out of sync with hardware, so subsequent `regmap_write_bits()` reads will go to the actual device rather than returning stale cached values.

**2. Error handling after `it6263_lvds_set_i2c_addr` failure (minor)**

```c
+	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);
```

If `it6263_lvds_set_i2c_addr()` fails, execution continues into `it6263_lvds_config()` which writes to the LVDS regmap. Since the I2C address routing hasn't been configured, those writes will go to an unrouted address — either silently failing or potentially hitting another device on the I2C bus. While `atomic_enable` returns void and can't propagate an error, it should `return` early after this failure to avoid cascading I2C writes to a misconfigured address.

**3. Probe no longer validates hardware presence (design consideration)**

By removing `it6263_hw_reset()` and `it6263_lvds_set_i2c_addr()` from probe, the driver no longer verifies during probe that the chip is physically present and responding over I2C. In the original code, `it6263_lvds_set_i2c_addr()` writes to HDMI registers — a failure there would cause probe to fail, giving early indication of a hardware problem. With this patch, a missing or broken chip won't be detected until the first `atomic_enable`, which changes error semantics. This is acceptable but worth noting.

**4. The structural changes themselves look correct**

The addition of `reset_gpio` to the struct:
```c
+	struct gpio_desc *reset_gpio;
```

and the probe-side change from local to struct member:
```c
-	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),
```

are straightforward and correct. The placement of the new init calls at the top of `atomic_enable` (before the mode/connector lookups) is the right ordering.

---
Generated by Claude Code Patch Reviewer