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

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2228 bytes --]

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 <javierm@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]