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: 3314 bytes --]

On Tue, May 12, 2026 at 08:54:13PM +0200, Javier Martinez Canillas wrote:
> Maxime Ripard <mripard@kernel.org> 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@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.
> >
> 
> 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.

The thing is that you're now making the driver and its interaction with
the hardware more complex. Like, what would happen if mode_set is now
called after atomic_enable? How does the hardware behavies if you modify
the SII902X_SYS_CTRL_DATA register only after it's been enabled?

Moving everything to atomic_enable keeps things simple and keep the
hardware access ordering the same.

And if you don't plan on sending that to fixes, the fact that the patch
is small is kind of irrelevant anyway.

Maxime

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