Re: [PATCH v3 1/2] drm/bridge: ite-it66121: Set TX mode in the .atomic_enable callback

[Maxime Ripard]

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 <javi...@redhat.com>
> ---
> 
> (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.

Maxime

signature.asc
Description: PGP signature