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: Move chip initialization code from probe to atomic_enable Date: Tue, 05 May 2026 09:39:38 +1000 Message-ID: In-Reply-To: <20260501061200.20129-1-biju.das.jz@bp.renesas.com> References: <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 Overall Series Review Subject: drm/bridge: ite-it6263: Move chip initialization code from probe t= o atomic_enable Author: Biju 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 SMAR= C 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 f= ully reinitialized on each enable cycle. The approach is reasonable and follows the pattern used by other bridge dri= vers that need to survive power loss. The patch is small and well-scoped. H= owever, there are a few concerns worth raising: **Concern 1 =E2=80=94 Error handling regression in `atomic_enable`.** In `p= robe()`, the original `it6263_lvds_set_i2c_addr()` failure was fatal and re= turned an error via `dev_err_probe()`. In `atomic_enable()`, the failure is= downgraded to a `dev_err()` log and execution continues. If the I2C addres= s setup fails, subsequent writes to the LVDS regmap will go to the wrong ad= dress, likely causing silent corruption or a hang. The `atomic_enable` call= back is void-returning so a direct return isn't possible, but the function = should at least bail out early rather than proceeding with configuration th= at depends on the I2C address being set. **Concern 2 =E2=80=94 `it6263_detect()` / EDID reads before `atomic_enable`= .** The `it6263_detect()` and `it6263_read_edid()` functions access the HDM= I regmap to read HPD status and EDID. After this patch, the chip is no long= er initialized in `probe()`, so these functions may be called against an un= initialized chip (e.g., during connector probing at boot). The `devm_regula= tor_bulk_get_enable` still powers the chip in probe, and the GPIO is reques= ted 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 conf= ig registers (e.g., `HDMI_REG_DDC_MASTER_CTRL`) are never written until `at= omic_enable` now. This deserves a closer look at whether detect/EDID work r= eliably without `it6263_hdmi_config()` being called first. **Concern 3 =E2=80=94 No `atomic_disable` counterpart.** The `atomic_disabl= e` only mutes audio/video and powers down the AFE driver. It does not asser= t the reset GPIO. This means on a normal disable/enable cycle (not involvin= g a full power loss), the chip gets a gratuitous hardware reset pulse follo= wed by full reinitialization. This should be harmless but adds ~46ms of lat= ency to every enable. --- --- Generated by Claude Code Patch Reviewer