Re: [PATCH v2] drm/bridge: it6505: support hdmi_codec_ops for audio stream setup

[Dmitry Baryshkov]

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