Re: [PATCH v4 1/3] drm/bridge: ite-it66121: Switch to the HDMI connector helpers

[Maxime Ripard <mripard@kernel.org>]

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

On Fri, May 15, 2026 at 11:02:09AM +0200, Javier Martinez Canillas wrote:
> Instead of open coding the HDMI AVI Infoframes buffer management,
> use the helpers provided by the HDMI connector framework.
> 
> Suggested-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
> Changes in v4:
> - New patch for v4
> 
>  drivers/gpu/drm/bridge/Kconfig       |   2 +
>  drivers/gpu/drm/bridge/ite-it66121.c | 132 ++++++++++++++++-----------
>  2 files changed, 83 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index f81b566c82a1..4a57d49b4c6d 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -205,6 +205,8 @@ config DRM_LONTIUM_LT8713SX
>  config DRM_ITE_IT66121
>  	tristate "ITE IT66121 HDMI bridge"
>  	depends on OF
> +	select DRM_DISPLAY_HDMI_STATE_HELPER
> +	select DRM_DISPLAY_HELPER
>  	select DRM_KMS_HELPER
>  	select REGMAP_I2C
>  	help
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 19a027d75b61..947b7a0f0a45 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -20,6 +20,8 @@
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/regulator/consumer.h>
>  
> +#include <drm/display/drm_hdmi_helper.h>
> +#include <drm/display/drm_hdmi_state_helper.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_edid.h>
> @@ -304,7 +306,6 @@ struct it66121_ctx {
>  	struct i2c_client *client;
>  	u32 bus_width;
>  	struct mutex lock; /* Protects fields below and device registers */
> -	struct hdmi_avi_infoframe hdmi_avi_infoframe;
>  	struct {
>  		struct platform_device *pdev;
>  		u8 ch_enable;
> @@ -727,6 +728,10 @@ static void it66121_bridge_enable(struct drm_bridge *bridge,
>  	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
>  
>  	ctx->connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
> +	if (!ctx->connector)
> +		return;
> +
> +	drm_atomic_helper_connector_hdmi_update_infoframes(ctx->connector, state);
>  
>  	it66121_set_mute(ctx, false);
>  }
> @@ -764,40 +769,10 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge,
>  			     const struct drm_display_mode *mode,
>  			     const struct drm_display_mode *adjusted_mode)
>  {
> -	u8 buf[HDMI_INFOFRAME_SIZE(AVI)];
>  	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
> -	int ret;
>  
>  	mutex_lock(&ctx->lock);
>  
> -	ret = drm_hdmi_avi_infoframe_from_display_mode(&ctx->hdmi_avi_infoframe, ctx->connector,
> -						       adjusted_mode);
> -	if (ret) {
> -		DRM_ERROR("Failed to setup AVI infoframe: %d\n", ret);
> -		goto unlock;
> -	}
> -
> -	ret = hdmi_avi_infoframe_pack(&ctx->hdmi_avi_infoframe, buf, sizeof(buf));
> -	if (ret < 0) {
> -		DRM_ERROR("Failed to pack infoframe: %d\n", ret);
> -		goto unlock;
> -	}
> -
> -	/* Write new AVI infoframe packet */
> -	ret = regmap_bulk_write(ctx->regmap, IT66121_AVIINFO_DB1_REG,
> -				&buf[HDMI_INFOFRAME_HEADER_SIZE],
> -				HDMI_AVI_INFOFRAME_SIZE);
> -	if (ret)
> -		goto unlock;
> -
> -	if (regmap_write(ctx->regmap, IT66121_AVIINFO_CSUM_REG, buf[3]))
> -		goto unlock;
> -
> -	/* 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 */
>  	if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, IT66121_HDMI_MODE_HDMI))
>  		goto unlock;
> @@ -825,24 +800,6 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge,
>  	mutex_unlock(&ctx->lock);
>  }
>  
> -static enum drm_mode_status it66121_bridge_mode_valid(struct drm_bridge *bridge,
> -						      const struct drm_display_info *info,
> -						      const struct drm_display_mode *mode)
> -{
> -	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
> -	unsigned long max_clock;
> -
> -	max_clock = (ctx->bus_width == 12) ? 74250 : 148500;
> -
> -	if (mode->clock > max_clock)
> -		return MODE_CLOCK_HIGH;
> -
> -	if (mode->clock < 25000)
> -		return MODE_CLOCK_LOW;
> -
> -	return MODE_OK;
> -}
> -
>  static enum drm_connector_status
>  it66121_bridge_detect(struct drm_bridge *bridge, struct drm_connector *connector)
>  {
> @@ -873,6 +830,72 @@ static void it66121_bridge_hpd_disable(struct drm_bridge *bridge)
>  		dev_err(ctx->dev, "failed to disable HPD IRQ\n");
>  }
>  
> +static enum drm_mode_status
> +it66121_bridge_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
> +					 const struct drm_display_mode *mode,
> +					 unsigned long long tmds_rate)
> +{
> +	const struct it66121_ctx *ctx =
> +		container_of(bridge, const struct it66121_ctx, bridge);
> +	unsigned long max_clock;
> +
> +	max_clock = (ctx->bus_width == 12) ? 74250 : 148500;
> +
> +	if (mode->clock > max_clock)
> +		return MODE_CLOCK_HIGH;
> +
> +	if (mode->clock < 25000)
> +		return MODE_CLOCK_LOW;
> +
> +	return MODE_OK;
> +}
> +
> +static int it66121_bridge_hdmi_clear_avi_infoframe(struct drm_bridge *bridge)
> +{
> +	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
> +
> +	return regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG, 0);
> +}
> +
> +static int it66121_bridge_hdmi_clear_hdmi_infoframe(struct drm_bridge *bridge)
> +{
> +	return 0;
> +}
> +
> +static int it66121_bridge_hdmi_write_avi_infoframe(struct drm_bridge *bridge,
> +						   const u8 *buffer, size_t len)
> +{
> +	struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge);
> +	int ret;
> +
> +	mutex_lock(&ctx->lock);
> +
> +	/* Write new AVI infoframe packet */
> +	ret = regmap_bulk_write(ctx->regmap, IT66121_AVIINFO_DB1_REG,
> +				&buffer[HDMI_INFOFRAME_HEADER_SIZE],
> +				HDMI_AVI_INFOFRAME_SIZE);
> +	if (ret)
> +		goto unlock;
> +
> +	ret = regmap_write(ctx->regmap, IT66121_AVIINFO_CSUM_REG, buffer[3]);
> +	if (ret)
> +		goto unlock;
> +
> +	/* Enable AVI infoframe */
> +	ret = regmap_write(ctx->regmap, IT66121_AVI_INFO_PKT_REG,
> +			   IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT);
> +
> +unlock:
> +	mutex_unlock(&ctx->lock);
> +	return ret;
> +}
> +
> +static int it66121_bridge_hdmi_write_hdmi_infoframe(struct drm_bridge *bridge,
> +						    const u8 *buffer, size_t len)
> +{
> +	return 0;
> +}
> +

When the code says it's mandatory, it means "it needs to be implemented" ;)

Looks good otherwise
Maxime

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