Maxime Ripard <[email protected]> writes: Thanks again for your feedback!
> Hi, > > On Tue, May 12, 2026 at 03:22:15PM +0200, Javier Martinez Canillas wrote: >> The HDMI transmission mode set and AVI infoframes enable are done in the >> .mode_set callback, but it is more correct to do this in .atomic_enable. >> >> Because the information about the sink type is in the struct drm_connector >> display_info.is_hdmi and this might not be available when the .mode_set >> callback is executed. >> >> Currently the driver is not checking display info to determine whether the >> mode has to be set to HDMI or DVI, but this is a bug that will be fixed by >> a follow-up change. >> >> Signed-off-by: Javier Martinez Canillas <[email protected]> >> --- >> >> (no changes since v1) >> >> drivers/gpu/drm/bridge/ite-it66121.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c >> b/drivers/gpu/drm/bridge/ite-it66121.c >> index 19a027d75b61..648ca50712df 100644 >> --- a/drivers/gpu/drm/bridge/ite-it66121.c >> +++ b/drivers/gpu/drm/bridge/ite-it66121.c >> @@ -669,6 +669,22 @@ static int it66121_set_mute(struct it66121_ctx *ctx, >> bool mute) >> IT66121_PKT_GEN_CTRL_ON | IT66121_PKT_GEN_CTRL_RPT); >> } >> >> +static void it66121_set_tx_mode(struct it66121_ctx *ctx) >> +{ >> + mutex_lock(&ctx->lock); >> + >> + /* Enable AVI infoframe */ >> + if (regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG, >> + IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT)) >> + goto unlock; >> + >> + /* Set TX mode to HDMI */ >> + regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, >> IT66121_HDMI_MODE_HDMI); >> + >> +unlock: >> + mutex_unlock(&ctx->lock); >> +} >> + >> #define MAX_OUTPUT_SEL_FORMATS 1 >> >> static u32 *it66121_bridge_atomic_get_output_bus_fmts(struct drm_bridge >> *bridge, >> @@ -729,6 +745,8 @@ static void it66121_bridge_enable(struct drm_bridge >> *bridge, >> ctx->connector = drm_atomic_get_new_connector_for_encoder(state, >> bridge->encoder); >> >> it66121_set_mute(ctx, false); >> + >> + it66121_set_tx_mode(ctx); >> } > > Having some part of it in mode_set and some part in enable is still kind > of weird. The best there would be to put everything in enable (and > pre_enable), and drop mode_set entirely. > I agree that this will be the correct approach. I wonder though if could be acceptable to land the changes in this series as a minimal fix for the DVI mode issue, and then do further refactoring as a follow-up. Note that what I'm doing in this version is aligned to what the sii902x bridge driver also does: static void sii902x_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_atomic_commit *state) { ... u8 output_mode = SII902X_SYS_CTRL_OUTPUT_DVI; connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder); if (connector && connector->display_info.is_hdmi) output_mode = SII902X_SYS_CTRL_OUTPUT_HDMI; ... regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA, SII902X_SYS_CTRL_OUTPUT_MODE, output_mode); ... } So that driver would also need the cleanup to get rid of the legacy .mode_set. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
