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

[Javier Martinez Canillas]

Maxime Ripard <mripard@xxxxxxxxxx> 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 <javierm@xxxxxxxxxx>
>> ---
>> 
>> (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