On Mon, Feb 03, 2025 at 02:04:30PM +0800, Hermes Wu via B4 Relay wrote: > From: Hermes Wu <Hermes.wu@xxxxxxxxxx> > > For supporting audio form I2S to DP audio data sub stream. > Add hdmi_audio callbacks to drm_bridge_funcs for the > HDMI codec framework. The DRM_BRIDGE_OP_HDMI flag in bridge.ops > must be set, and hdmi_write_infoframe and hdmi_clear_infoframe > are necessary for the drm_bridge_connector to enable the HDMI codec. Please split this into two commits: one adding OP_HDMI, second one adding audio support. > > Signed-off-by: Hermes Wu <Hermes.wu@xxxxxxxxxx> > --- > Changes in v2: > - Use DRM HDMI codec framewrok for audio stream setup. > - Link to v1: https://lore.kernel.org/r/20250121-add-audio-codec-v1-1-e3ff71b3c819@xxxxxxxxxx > --- > drivers/gpu/drm/bridge/ite-it6505.c | 151 +++++++++++++++++++++++++++++++----- > 1 file changed, 132 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c > index 88ef76a37fe6accacdd343839ff2569b31b18ceb..361e2412b8e8f040cf4254479b60588d99e8c99a 100644 > --- a/drivers/gpu/drm/bridge/ite-it6505.c > +++ b/drivers/gpu/drm/bridge/ite-it6505.c > @@ -1414,32 +1414,43 @@ static void it6505_variable_config(struct it6505 *it6505) > memset(it6505->bksvs, 0, sizeof(it6505->bksvs)); > } > > +static int it6505_write_avi_infoframe(struct it6505 *it6505, > + const u8 *buffer, size_t len) > +{ > + struct device *dev = it6505->dev; > + > + if (len - HDMI_INFOFRAME_HEADER_SIZE > 13) { > + DRM_DEV_DEBUG_DRIVER(dev, "AVI size fail %d", len); > + return -EINVAL; > + } > + > + it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_AVI_PKT, 0); > + regmap_bulk_write(it6505->regmap, REG_AVI_INFO_DB1, > + buffer + HDMI_INFOFRAME_HEADER_SIZE, > + len - HDMI_INFOFRAME_HEADER_SIZE); > + > + it6505_write(it6505, REG_AVI_INFO_SUM, buffer[3]); > + it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_AVI_PKT, > + EN_AVI_PKT); > + > + return 0; > +} > + > static int it6505_send_video_infoframe(struct it6505 *it6505, > struct hdmi_avi_infoframe *frame) > { > u8 buffer[HDMI_INFOFRAME_HEADER_SIZE + HDMI_AVI_INFOFRAME_SIZE]; > - int err; > + int err, length; > struct device *dev = it6505->dev; > > - err = hdmi_avi_infoframe_pack(frame, buffer, sizeof(buffer)); > - if (err < 0) { > - dev_err(dev, "Failed to pack AVI infoframe: %d", err); > - return err; > + length = hdmi_avi_infoframe_pack(frame, buffer, sizeof(buffer)); > + if (length < 0) { > + dev_err(dev, "Failed to pack AVI infoframe: %d", length); > + return length; > } > > - err = it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_AVI_PKT, 0x00); > - if (err) > - return err; > - > - err = regmap_bulk_write(it6505->regmap, REG_AVI_INFO_DB1, > - buffer + HDMI_INFOFRAME_HEADER_SIZE, > - frame->length); > - if (err) > - return err; > - > - err = it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_AVI_PKT, > - EN_AVI_PKT); > - if (err) > + err = it6505_write_avi_infoframe(it6505, buffer, length); You mustn't to do this. Please instead call drm_atomic_helper_connector_hdmi_update_infoframes() from your .atomic_enable instead. > + if (err < 0) > return err; > > return 0; > @@ -1625,6 +1636,18 @@ static void it6505_enable_audio_infoframe(struct it6505 *it6505) > EN_AUD_CTRL_PKT); > } > > +static void it6505_write_audio_infoframe(struct it6505 *it6505, > + const u8 *buffer, size_t len) > +{ > + it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_AUD_PKT, 0); > + regmap_bulk_write(it6505->regmap, REG_AUD_INFOFRAM_DB1, > + buffer + HDMI_INFOFRAME_HEADER_SIZE, > + 4); > + it6505_write(it6505, REG_AUD_INFOFRAM_SUM, buffer[3]); > + it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_AUD_PKT, > + EN_AUD_PKT); > +} > + > static void it6505_disable_audio(struct it6505 *it6505) > { > it6505_set_bits(it6505, REG_DATA_MUTE_CTRL, EN_AUD_MUTE, EN_AUD_MUTE); > @@ -3302,6 +3325,85 @@ static const struct drm_edid *it6505_bridge_edid_read(struct drm_bridge *bridge, > return drm_edid_dup(it6505->cached_edid); > } > > +static int it6505_bridge_hdmi_audio_startup(struct drm_connector *connector, > + struct drm_bridge *bridge) > +{ > + struct it6505 *it6505 = bridge_to_it6505(bridge); > + struct device *dev = it6505->dev; > + > + if (!it6505->powered || it6505->enable_drv_hold) > + return -EIO; > + > + DRM_DEV_DEBUG_DRIVER(dev, "Audio enable"); > + it6505_enable_audio(it6505); > + > + return 0; > +} > + > +static int it6505_bridge_hdmi_audio_prepare(struct drm_connector *connector, > + struct drm_bridge *bridge, > + struct hdmi_codec_daifmt *fmt, > + struct hdmi_codec_params *hparms) > +{ > + struct it6505 *it6505 = bridge_to_it6505(bridge); > + > + return it6505_audio_setup_hw_params(it6505, hparms); > +} > + > +static void it6505_bridge_hdmi_audio_shutdown(struct drm_connector *connector, > + struct drm_bridge *bridge) > +{ > + struct it6505 *it6505 = bridge_to_it6505(bridge); > + > + if (it6505->powered && !it6505->enable_drv_hold) > + it6505_disable_audio(it6505); > +} > + > +static int it6505_bridge_hdmi_clear_infoframe(struct drm_bridge *bridge, > + enum hdmi_infoframe_type type) > +{ > + struct it6505 *it6505 = bridge_to_it6505(bridge); > + struct device *dev = it6505->dev; > + > + switch (type) { > + case HDMI_INFOFRAME_TYPE_AUDIO: > + it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_AUD_PKT, 0); > + break; > + > + case HDMI_INFOFRAME_TYPE_AVI: > + it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_AVI_PKT, 0); > + break; Are SPD / Vendor InfoFrames not supported by the HW? > + default: > + dev_dbg(dev, "unsupported HDMI infoframe 0x%x\n", type); > + break; > + } > + > + return 0; > +} > + > +static int it6505_bridge_hdmi_write_infoframe(struct drm_bridge *bridge, > + enum hdmi_infoframe_type type, > + const u8 *buffer, size_t len) > +{ > + struct it6505 *it6505 = bridge_to_it6505(bridge); > + struct device *dev = it6505->dev; > + > + switch (type) { > + case HDMI_INFOFRAME_TYPE_AUDIO: > + it6505_write_audio_infoframe(it6505, buffer, len); > + break; > + > + case HDMI_INFOFRAME_TYPE_AVI: > + it6505_write_avi_infoframe(it6505, buffer, len); > + break; > + default: > + dev_dbg(dev, "unsupported HDMI infoframe 0x%x\n", type); > + break; > + } > + > + return 0; > +} Please group functions logically, by having all InfoFrame functions next to each other. > + > static const struct drm_bridge_funcs it6505_bridge_funcs = { > .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > @@ -3315,6 +3417,12 @@ static const struct drm_bridge_funcs it6505_bridge_funcs = { > .atomic_post_disable = it6505_bridge_atomic_post_disable, > .detect = it6505_bridge_detect, > .edid_read = it6505_bridge_edid_read, > + .hdmi_audio_startup = it6505_bridge_hdmi_audio_startup, > + .hdmi_audio_prepare = it6505_bridge_hdmi_audio_prepare, > + .hdmi_audio_shutdown = it6505_bridge_hdmi_audio_shutdown, > + .hdmi_clear_infoframe = it6505_bridge_hdmi_clear_infoframe, > + .hdmi_write_infoframe = it6505_bridge_hdmi_write_infoframe, No .hdmi_tmds_char_rate_valid? > + > }; > > static __maybe_unused int it6505_bridge_resume(struct device *dev) > @@ -3700,7 +3808,12 @@ static int it6505_i2c_probe(struct i2c_client *client) > it6505->bridge.funcs = &it6505_bridge_funcs; > it6505->bridge.type = DRM_MODE_CONNECTOR_DisplayPort; > it6505->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | > - DRM_BRIDGE_OP_HPD; > + DRM_BRIDGE_OP_HPD | DRM_BRIDGE_OP_HDMI; > + it6505->bridge.vendor = "iTE"; > + it6505->bridge.product = "IT6505"; > + it6505->bridge.hdmi_audio_dev = dev; > + it6505->bridge.hdmi_audio_max_i2s_playback_channels = 2; > + it6505->bridge.hdmi_audio_dai_port = 1; > drm_bridge_add(&it6505->bridge); > > return 0; > > --- > base-commit: fe003bcb69f7bff9ff2b30b659b004dbafe52907 > change-id: 20250114-add-audio-codec-8c9d47062a6c > > Best regards, > -- > Hermes Wu <Hermes.wu@xxxxxxxxxx> > > -- With best wishes Dmitry
