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 <[email protected]> > Signed-off-by: Javier Martinez Canillas <[email protected]> > --- > > 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
signature.asc
Description: PGP signature
