From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/bridge: ite-it6263: Support power cycle in runtime Date: Thu, 23 Apr 2026 08:33:49 +1000 Message-ID: In-Reply-To: <20260421105334.43411-1-biju.das.jz@bp.renesas.com> References: <20260421105334.43411-1-biju.das.jz@bp.renesas.com> <20260421105334.43411-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 **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 `at= omic_enable`, all hardware registers return to their default values, but th= e regmap caches still hold the values from the previous enable cycle. This is a problem because `it6263_lvds_config()` =E2=86=92 `it6263_lvds_res= et()`, `it6263_lvds_set_interface()`, `it6263_lvds_set_afe()`, and `it6263_= lvds_sys_cfg()` all use `regmap_write_bits()`, which performs read-modify-w= rite 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-o= n-reset default. The `write_bits` call will preserve the non-masked bits fr= om 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 return= ing stale cached values. **2. Error handling after `it6263_lvds_set_i2c_addr` failure (minor)** ```c + 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); ``` If `it6263_lvds_set_i2c_addr()` fails, execution continues into `it6263_lvd= s_config()` which writes to the LVDS regmap. Since the I2C address routing = hasn't been configured, those writes will go to an unrouted address =E2=80= =94 either silently failing or potentially hitting another device on the I2= C 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 pr= esent and responding over I2C. In the original code, `it6263_lvds_set_i2c_a= ddr()` writes to HDMI registers =E2=80=94 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 =3D 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 =3D 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 or= dering. --- Generated by Claude Code Patch Reviewer