Re: [PATCH v1 1/6] ASoC: qcom: qdsp6: add topology-driven Audio IF support

[Srinivas Kandagatla <srini@kernel.org>]

On 6/10/26 4:45 PM, Prasad Kumpatla wrote:
> Add topology parsing and media-format programming for Audio IF
> source and sink modules.
> 
> Add the Audio IF module IDs, the required topology tokens, and a
> dedicated topology loader that stores the parsed interface
> configuration in the AudioReach module state. Also add the Audio IF
> media-format path that sends the interface configuration, hardware
> endpoint media format, and frame-duration parameters for Audio IF
> modules.
> 
> This keeps the serial-interface configuration topology-driven while
> still allowing the machine driver to provide runtime slot and media
> format settings. The same Audio IF path can then be reused for TDM,
> PCM, and I2S style backends.
> 
> The new UAPI tokens (AR_TKN_U32_MODULE_SYNC_SRC=262 through
> AR_TKN_U32_MODULE_INV_EXT_BIT_CLK=276) are added.
> 
> MODULE_ID_AUDIO_IF_SINK (0x0700117C) and MODULE_ID_AUDIO_IF_SOURCE
> (0x0700117D) are introduced in this patch.
> 

Which platform is this tested on, also please send a PR to
github.com/linux-msm/audioreach-topology to add thse new tokens.

> Signed-off-by: Prasad Kumpatla <prasad.kumpatla@oss.qualcomm.com>
> ---
>  include/uapi/sound/snd_ar_tokens.h |  58 ++++++++++++++++
>  sound/soc/qcom/qdsp6/audioreach.c  |  97 ++++++++++++++++++++++++++
>  sound/soc/qcom/qdsp6/audioreach.h  |  62 +++++++++++++++++
>  sound/soc/qcom/qdsp6/topology.c    | 108 +++++++++++++++++++++++++++++
>  4 files changed, 325 insertions(+)
> 
> diff --git a/include/uapi/sound/snd_ar_tokens.h b/include/uapi/sound/snd_ar_tokens.h
> index 6b8102eaa..355a1e629 100644
> --- a/include/uapi/sound/snd_ar_tokens.h
> +++ b/include/uapi/sound/snd_ar_tokens.h
> @@ -168,6 +168,48 @@ enum ar_event_types {
>   *						LOG_WAIT = 0,
>   *						LOG_IMMEDIATELY = 1
>   *
> + * %AR_TKN_U32_MODULE_SYNC_SRC:			Frame sync source
> + *						0 = external, 1 = internal
> + *
> + * %AR_TKN_U32_MODULE_CTRL_DATA_OUT_ENABLE:	Enable data-out tri-state control
> + *						0 = disable, 1 = enable
> + *
> + * %AR_TKN_U32_MODULE_SLOT_MASK:			Active TDM slot bitmask
> + *
> + * %AR_TKN_U32_MODULE_NSLOTS_PER_FRAME:		Number of slots per TDM frame
> + *
> + * %AR_TKN_U32_MODULE_SLOT_WIDTH:			Slot width in bits (16 or 32)
> + *
> + * %AR_TKN_U32_MODULE_SYNC_MODE:			Frame sync mode
> + *						0 = short pulse, 1 = long pulse

We have 3 possible values, please correct this, also you could add
defines for these values.
> + *
> + * %AR_TKN_U32_MODULE_CTRL_INVERT_SYNC_PULSE:	Invert frame sync pulse polarity
> + *						0 = normal, 1 = inverted
> + *
> + * %AR_TKN_U32_MODULE_CTRL_SYNC_DATA_DELAY:	Data delay relative to frame sync
> + *						0 = no delay, 1 = one cycle delay

Exactly same here, we have 2 cyle delay too.

> + *
> + * %AR_TKN_U32_MODULE_INTF_MODE:			Audio IF interface mode
> + *						AUDIO_IF_INTF_MODE_TDM = 0,
> + *						AUDIO_IF_INTF_MODE_PCM = 1,
> + *						AUDIO_IF_INTF_MODE_I2S = 2

Same here, defines for these.

> + *
> + * %AR_TKN_U32_MODULE_QAIF_TYPE:			QAIF hardware port type index
> + *
> + * %AR_TKN_U32_MODULE_ACTIVE_LANE_MASK:		Active lane bitmask for multi-lane
> + *
> + * %AR_TKN_U32_MODULE_FRAME_SYNC_RATE:		Frame sync rate in Hz
> + *
> + * %AR_TKN_U32_MODULE_BIT_CLK_TYPE:			Bit clock type
> + *						0 = internal, 1 = external,
> + *						2 = skip (bypass bit clock enable)
> + *
> + * %AR_TKN_U32_MODULE_INV_INT_BIT_CLK:		Invert internal bit clock
> + *						0 = normal, 1 = inverted
> + *
> + * %AR_TKN_U32_MODULE_INV_EXT_BIT_CLK:		Invert external bit clock
> + *						0 = normal, 1 = inverted
> + *
>   * %AR_TKN_DAI_INDEX:				dai index
>   *
>   */
> @@ -240,6 +282,22 @@ enum ar_event_types {
>  #define AR_TKN_U32_MODULE_LOG_TAP_POINT_ID	260
>  #define AR_TKN_U32_MODULE_LOG_MODE		261
>  
> +#define AR_TKN_U32_MODULE_SYNC_SRC		262
> +#define AR_TKN_U32_MODULE_CTRL_DATA_OUT_ENABLE	263
> +#define AR_TKN_U32_MODULE_SLOT_MASK		264
> +#define AR_TKN_U32_MODULE_NSLOTS_PER_FRAME	265
> +#define AR_TKN_U32_MODULE_SLOT_WIDTH		266
> +#define AR_TKN_U32_MODULE_SYNC_MODE		267
> +#define AR_TKN_U32_MODULE_CTRL_INVERT_SYNC_PULSE	268
> +#define AR_TKN_U32_MODULE_CTRL_SYNC_DATA_DELAY	269
> +#define AR_TKN_U32_MODULE_INTF_MODE		270
> +#define AR_TKN_U32_MODULE_QAIF_TYPE		271
> +#define AR_TKN_U32_MODULE_ACTIVE_LANE_MASK	272
> +#define AR_TKN_U32_MODULE_FRAME_SYNC_RATE	273
> +#define AR_TKN_U32_MODULE_BIT_CLK_TYPE		274
> +#define AR_TKN_U32_MODULE_INV_INT_BIT_CLK	275
> +#define AR_TKN_U32_MODULE_INV_EXT_BIT_CLK	276
> +
Here you prefix the tokens with U32, however in dirver this values are
validated against U8 and U16, So please fix the prefixes to reflect the
range.


...

>  
>  	default:
>  		rc = 0;
> diff --git a/sound/soc/qcom/qdsp6/audioreach.h b/sound/soc/qcom/qdsp6/audioreach.h
> index 62a2fd79b..1dc29ddfd 100644
> --- a/sound/soc/qcom/qdsp6/audioreach.h
> +++ b/sound/soc/qcom/qdsp6/audioreach.h
> @@ -22,6 +22,8 @@ struct q6apm_graph;
>  #define MODULE_ID_PLACEHOLDER_DECODER	0x07001009
>  #define MODULE_ID_I2S_SINK		0x0700100A
>  #define MODULE_ID_I2S_SOURCE		0x0700100B
> +#define MODULE_ID_AUDIO_IF_SINK		0x0700117C
> +#define MODULE_ID_AUDIO_IF_SOURCE	0x0700117D
Please place it in the assending order.

>  #define MODULE_ID_SAL			0x07001010
>  #define MODULE_ID_MFC			0x07001015
>  #define MODULE_ID_DATA_LOGGING		0x0700101A
> @@ -544,6 +546,41 @@ struct param_id_i2s_intf_cfg {
>  #define PORT_ID_I2S_OUPUT		1
>  #define I2S_STACK_SIZE			2048
>  
> +#define PARAM_ID_AUDIO_IF_INTF_CFG	0x08001B11
> +
> +#define AUDIO_IF_INTF_MODE_TDM		0x0
> +#define AUDIO_IF_INTF_MODE_PCM		0x1
> +#define AUDIO_IF_INTF_MODE_I2S		0x2
> +
> +struct param_id_audio_if_intf_cfg {

I know that we have not added documentation for all the structures, but
Am in process of adding them. Can you add kernel doc to these structs.
> +	uint16_t qaif_type;
> +	uint16_t intf_idx;
> +	uint16_t intf_mode;
> +	uint16_t ctrl_data_out_enable;
> +	uint32_t active_slot_mask;
> +	uint16_t nslots_per_frame;
> +	uint16_t slot_width;
> +	uint32_t active_lane_mask;
> +	uint32_t frame_sync_rate;
> +	uint16_t frame_sync_src;
> +	uint16_t frame_sync_mode;
> +	uint16_t invert_frame_sync_pulse;
> +	uint16_t frame_sync_data_delay;
> +	uint16_t bit_clk_type;
> +	uint8_t inv_int_bit_clk;
> +	uint8_t inv_ext_bit_clk;
> +} __packed;
> +
> +#define PARAM_ID_HW_EP_FRAME_DURATION		0x08001B2F
> +#define AUDIO_IF_FRAME_DURATION_US		1000
Why is this hardcoded?

> +
> +struct param_id_hw_ep_frame_duration {
> +	uint32_t frame_duration_in_us;
> +	uint32_t allow_frame_duration_normalization;
> +	uint32_t min_normalized_frame_dur_us;
> +	uint32_t max_normalized_frame_dur_us;
> +} __packed;
> +
>  #define PARAM_ID_DISPLAY_PORT_INTF_CFG		0x08001154
>  
>  struct param_id_display_port_intf_cfg {
> @@ -877,6 +914,28 @@ struct audioreach_module {
>  	uint32_t data_format;
>  	uint32_t hw_interface_type;
>  
> +	/* Audio IF module (TDM/PCM/I2S) */
> +	/*
> +	 * uint32_t fields first to minimise intra-block padding;
Why do we need this comments does not add a real value here?
> +	 * 2 bytes of trailing padding remain after inv_ext_bit_clk
> +	 * before the next uint32_t field (interleave_type).
> +	 */
> +	uint32_t slot_mask;
> +	uint32_t active_lane_mask;
> +	uint32_t frame_sync_rate;
> +	uint16_t qaif_type;
> +	uint16_t sync_src;
> +	uint16_t ctrl_data_out_enable;
> +	uint16_t nslots_per_frame;
> +	uint16_t slot_width;
> +	uint16_t intf_mode;
> +	uint16_t sync_mode;
> +	uint16_t ctrl_invert_sync_pulse;
> +	uint16_t ctrl_sync_data_delay;
> +	uint16_t bit_clk_type;
> +	uint8_t inv_int_bit_clk;
> +	uint8_t inv_ext_bit_clk;
> +
>  	/* PCM module specific */
>  	uint32_t interleave_type;
>  
> @@ -907,6 +966,9 @@ struct audioreach_module_config {
>  	u32	channel_allocation;
>  	u32	sd_line_mask;
>  	int	fmt;
> +	u32	slot_mask;
> +	u16	nslots_per_frame;
> +	u16	slot_width;
>  	struct snd_codec codec;
>  	u8 channel_map[AR_PCM_MAX_NUM_CHANNEL];
>  };
> diff --git a/sound/soc/qcom/qdsp6/topology.c b/sound/soc/qcom/qdsp6/topology.c
> index 1f69fba6d..2ae7ac3d2 100644
> --- a/sound/soc/qcom/qdsp6/topology.c
> +++ b/sound/soc/qcom/qdsp6/topology.c
> @@ -753,6 +753,108 @@ static int audioreach_widget_i2s_module_load(struct audioreach_module *mod,
>  	return 0;
>  }
>  
> +static int audioreach_widget_audio_if_module_load(struct audioreach_module *mod,
> +						  const struct snd_soc_tplg_vendor_array *mod_array)
> +{
> +	const struct snd_soc_tplg_vendor_value_elem *mod_elem;
> +	int tkn_count = 0;
> +	u32 val;
> +
> +	mod_elem = mod_array->value;
> +
> +	while (tkn_count < le32_to_cpu(mod_array->num_elems)) {
> +		val = le32_to_cpu(mod_elem->value);
> +		switch (le32_to_cpu(mod_elem->token)) {
> +		case AR_TKN_U32_MODULE_HW_IF_IDX:
> +			if (val > U16_MAX)
> +				return -EINVAL;
Plese fix such instances as suggested at the top.

> +			mod->hw_interface_idx = val;
> +			break;
> +		case AR_TKN_U32_MODULE_FMT_DATA:
> +			mod->data_format = val;
> +			break;
> +		case AR_TKN_U32_MODULE_HW_IF_TYPE:
> +			mod->hw_interface_type = val;
where are we using this?

> +			break;
> +		case AR_TKN_U32_MODULE_SYNC_SRC:
> +			if (val > U16_MAX)
> +				return -EINVAL;
> +			mod->sync_src = (u16)val;
> +			break;
> +		case AR_TKN_U32_MODULE_CTRL_DATA_OUT_ENABLE:
> +			if (val > U16_MAX)
> +				return -EINVAL;
> +			mod->ctrl_data_out_enable = (u16)val;
> +			break;
> +		case AR_TKN_U32_MODULE_SLOT_MASK:
> +			mod->slot_mask = val;
> +			break;
> +		case AR_TKN_U32_MODULE_NSLOTS_PER_FRAME:
> +			if (val > U16_MAX)
> +				return -EINVAL;
> +			mod->nslots_per_frame = (u16)val;
> +			break;
> +		case AR_TKN_U32_MODULE_SLOT_WIDTH:
> +			if (val > U16_MAX)
> +				return -EINVAL;
> +			mod->slot_width = (u16)val;
> +			break;
> +		case AR_TKN_U32_MODULE_INTF_MODE:
> +			if (val > U16_MAX)
> +				return -EINVAL;
> +			mod->intf_mode = (u16)val;
> +			break;
> +		case AR_TKN_U32_MODULE_SYNC_MODE:
> +			if (val > U16_MAX)
> +				return -EINVAL;
> +			mod->sync_mode = (u16)val;
> +			break;
> +		case AR_TKN_U32_MODULE_CTRL_INVERT_SYNC_PULSE:
> +			if (val > U16_MAX)
> +				return -EINVAL;
> +			mod->ctrl_invert_sync_pulse = (u16)val;
> +			break;
> +		case AR_TKN_U32_MODULE_CTRL_SYNC_DATA_DELAY:
> +			if (val > U16_MAX)
> +				return -EINVAL;
> +			mod->ctrl_sync_data_delay = (u16)val;
> +			break;
> +		case AR_TKN_U32_MODULE_QAIF_TYPE:
> +			if (val > U16_MAX)
> +				return -EINVAL;
> +			mod->qaif_type = (u16)val;
> +			break;
> +		case AR_TKN_U32_MODULE_ACTIVE_LANE_MASK:
> +			mod->active_lane_mask = val;
> +			break;
> +		case AR_TKN_U32_MODULE_FRAME_SYNC_RATE:
> +			mod->frame_sync_rate = val;
> +			break;
> +		case AR_TKN_U32_MODULE_BIT_CLK_TYPE:
> +			if (val > U16_MAX)
> +				return -EINVAL;
> +			mod->bit_clk_type = (u16)val;
> +			break;
> +		case AR_TKN_U32_MODULE_INV_INT_BIT_CLK:
> +			if (val > U8_MAX)
> +				return -EINVAL;
> +			mod->inv_int_bit_clk = (u8)val;
> +			break;
> +		case AR_TKN_U32_MODULE_INV_EXT_BIT_CLK:
> +			if (val > U8_MAX)
> +				return -EINVAL;
> +			mod->inv_ext_bit_clk = (u8)val;
> +			break;
> +		default:
> +			break;
> +		}
> +		tkn_count++;
> +		mod_elem++;
> +	}
> +
> +	return 0;
> +}