Re: [PATCH v5 17/21] ASoC: qdsp6: audioreach: add topology support

+/**

• • %AR_TKN_U32_SUB_GRAPH_INSTANCE_ID: Sub Graph Instance Id
• • %AR_TKN_U32_SUB_GRAPH_PERF_MODE: Performance mode of subgraph

• • 				APM_SUB_GRAPH_PERF_MODE_LOW_POWER = 1,
    

• • 				APM_SUB_GRAPH_PERF_MODE_LOW_LATENCY = 2
    

• • %AR_TKN_U32_SUB_GRAPH_DIRECTION: Direction of subgraph

• • 				APM_SUB_GRAPH_DIRECTION_TX = 1,
    

• • 				APM_SUB_GRAPH_DIRECTION_RX = 2
    

can you have full-duplex? unclear how you would define the direction in the case of a voice call...

• • %AR_TKN_U32_SUB_GRAPH_SCENARIO_ID: Scenario ID for subgraph

• • 				APM_SUB_GRAPH_SID_AUDIO_PLAYBACK = 1,
    

• • 				APM_SUB_GRAPH_SID_AUDIO_RECORD = 2,
    

• • 				APM_SUB_GRAPH_SID_VOICE_CALL = 3
    

• • %AR_TKN_U32_CONTAINER_INSTANCE_ID: Container Instance ID
• • %AR_TKN_U32_CONTAINER_CAPABILITY_ID: Container capability ID

• • 				APM_CONTAINER_CAP_ID_PP = 1,
    

• • 				APM_CONTAINER_CAP_ID_CD = 2,
    

• • 				APM_CONTAINER_CAP_ID_EP = 3,
    

• • 				APM_CONTAINER_CAP_ID_OLC = 4
    

Acronyms? PP, CD, EP, OLC?

• • %AR_TKN_U32_CONTAINER_STACK_SIZE: Stack size in the container.
• • %AR_TKN_U32_CONTAINER_GRAPH_POS: Graph Position

• • 				APM_CONT_GRAPH_POS_STREAM = 1,
    

• • 				APM_CONT_GRAPH_POS_PER_STR_PER_DEV = 2,
    

• • 				APM_CONT_GRAPH_POS_STR_DEV = 3,
    

• • 				APM_CONT_GRAPH_POS_GLOBAL_DEV = 4
    

explain what this means?

• • %AR_TKN_U32_CONTAINER_PROC_DOMAIN: Processor domain of container

• • 				APM_PROC_DOMAIN_ID_MDSP = 1,
    

• • 				APM_PROC_DOMAIN_ID_ADSP = 2,
    

• • 				APM_PROC_DOMAIN_ID_SDSP = 4,
    

• • 				APM_PROC_DOMAIN_ID_CDSP = 5
    

what happened to 3, is it reserved/illegal?

• • %AR_TKN_U32_MODULE_ID: Module ID
• • %AR_TKN_U32_MODULE_INSTANCE_ID: Module Instance ID.
• • %AR_TKN_U32_MODULE_MAX_IP_PORTS: Module maximum input ports
• • %AR_TKN_U32_MODULE_MAX_OP_PORTS: Module maximum output ports.
• • %AR_TKN_U32_MODULE_IN_PORTS: Number of in ports
• • %AR_TKN_U32_MODULE_OUT_PORTS: Number of out ports.
• • %AR_TKN_U32_MODULE_SRC_OP_PORT_ID: Source module output port ID
• • %AR_TKN_U32_MODULE_DST_IN_PORT_ID: Destination module input port ID
• • %AR_TKN_U32_MODULE_HW_IF_IDX: Interface index types for I2S/LPAIF
• • %AR_TKN_U32_MODULE_HW_IF_TYPE: Interface type

• • 				LPAIF = 0,
    

• • 				LPAIF_RXTX = 1,
    

• • 				LPAIF_WSA = 2,
    

• • 				LPAIF_VA = 3,
    

• • 				LPAIF_AXI = 4
    

• • %AR_TKN_U32_MODULE_FMT_INTERLEAVE: PCM Interleaving

• • 				PCM_INTERLEAVED = 1,
    

• • 				PCM_DEINTERLEAVED_PACKED = 2,
    

• • 				PCM_DEINTERLEAVED_UNPACKED = 3
    

• • %AR_TKN_U32_MODULE_FMT_DATA: data format

• • 				FIXED POINT = 1,
    

• • 				IEC60958 PACKETIZED = 3,
    

• • 				IEC60958 PACKETIZED NON LINEAR = 8,
    

• • 				COMPR OVER PCM PACKETIZED = 7,
    

• • 				IEC61937 PACKETIZED = 2,
    

isn't this precisely compressed over PCM?

• • 				GENERIC COMPRESSED = 5
    

??

• • %AR_TKN_U32_MODULE_FMT_FREQ: bit rate

bit rate or frame rate (aka sampling frequency) ?

• • %AR_TKN_U32_MODULE_FMT_BIT_DEPTH: bit depth

+static struct audioreach_sub_graph *audioreach_parse_sg_tokens(struct q6apm *apm,

• 			struct snd_soc_tplg_private *private)
  

+{

• struct audioreach_graph_info *info = NULL;
• struct snd_soc_tplg_vendor_array *sg_array;
• struct snd_soc_tplg_vendor_value_elem *sg_elem;
• struct audioreach_sub_graph *sg;
• int graph_id, sub_graph_id, tkn_count = 0;
• bool found;
• sg_array = audioreach_get_sg_array(private);
• sg_elem = sg_array->value;
• while (tkn_count <= (le32_to_cpu(sg_array->num_elems) - 1)) {

• switch (le32_to_cpu(sg_elem->token)) {
  

• case AR_TKN_U32_SUB_GRAPH_INSTANCE_ID:
  

• 	sub_graph_id = le32_to_cpu(sg_elem->value);
  

• 	sg = audioreach_tplg_alloc_sub_graph(apm, sub_graph_id, &found);
  

• 	if (IS_ERR(sg)) {
  

• 		return sg;
  

• 	} else if (found) {
  

• 		/* Already parsed data for this sub-graph */
  

• 		return sg;
  

• 	}
  

• 	dev_err(apm->dev, "%s: New subgraph id : 0x%08x\n", __func__,
  

• 		sub_graph_id);
  

• 	break;
  

• case AR_TKN_DAI_INDEX:
  

• 	/* Sub graph is associated with predefined graph */
  

• 	graph_id = le32_to_cpu(sg_elem->value);
  

• 	info = audioreach_tplg_alloc_graph_info(apm, graph_id, &found);
  

• 	if (IS_ERR(info))
  

• 		return ERR_CAST(info);
  

• 	break;
  

• case AR_TKN_U32_SUB_GRAPH_PERF_MODE:
  

• 	sg->perf_mode = le32_to_cpu(sg_elem->value);
  

• 	break;
  

• case AR_TKN_U32_SUB_GRAPH_DIRECTION:
  

• 	sg->direction = le32_to_cpu(sg_elem->value);
  

• 	break;
  

• case AR_TKN_U32_SUB_GRAPH_SCENARIO_ID:
  

• 	sg->scenario_id = le32_to_cpu(sg_elem->value);
  

• 	break;
  

• default:
  

• 	dev_err(apm->dev, "Not a valid token %d for graph\n",
  

• 		sg_elem->token);
  

• break;
  

indentation is off

• }
  

• tkn_count++;
  

• sg_elem++;
  

• }
• /* Sub graph is associated with predefined graph */
• if (info) {

• dev_err(apm->dev, "%s: adding subgraph id : 0x%08x -> %d\n", __func__,
  

• sub_graph_id, graph_id);
  

• audioreach_tplg_add_sub_graph(sg, info);
  

• }
• return sg;

+}

+static struct audioreach_container *audioreach_parse_cont_tokens(struct q6apm *apm,

• 				struct audioreach_sub_graph *sg,
  

• 				struct snd_soc_tplg_private *private)
  

+{

• struct snd_soc_tplg_vendor_array *cont_array;
• struct snd_soc_tplg_vendor_value_elem *cont_elem;
• struct audioreach_container *cont;
• int container_id, tkn_count = 0;
• bool found = false;
• cont_array = audioreach_get_cont_array(private);
• cont_elem = cont_array->value;
• while (tkn_count <= (le32_to_cpu(cont_array->num_elems) - 1)) {

• switch (le32_to_cpu(cont_elem->token)) {
  

• case AR_TKN_U32_CONTAINER_INSTANCE_ID:
  

• 	container_id = le32_to_cpu(cont_elem->value);
  

• 	cont = audioreach_tplg_alloc_container(apm, sg, container_id, &found);
  

• 	if (IS_ERR(cont))
  

• 		return ERR_PTR(-ENOMEM);
  

• 	else if (found) /* Already parsed container data */
  

• 		return cont;
  

• 	break;
  

• case AR_TKN_U32_CONTAINER_CAPABILITY_ID:
  

• 	cont->capability_id = le32_to_cpu(cont_elem->value);
  

• 	break;
  

• case AR_TKN_U32_CONTAINER_STACK_SIZE:
  

• 	cont->stack_size = le32_to_cpu(cont_elem->value);
  

• 	break;
  

• case AR_TKN_U32_CONTAINER_GRAPH_POS:
  

• 	cont->graph_pos = le32_to_cpu(cont_elem->value);
  

• 	break;
  

• case AR_TKN_U32_CONTAINER_PROC_DOMAIN:
  

• 	cont->proc_domain = le32_to_cpu(cont_elem->value);
  

• 	break;
  

• default:
  

• 	dev_err(apm->dev, "Not a valid token %d for graph\n",
  

• 		cont_elem->token);
  

• break;
  

indentation?

• }
  

• tkn_count++;
  

• cont_elem++;
  

• }
• return cont;

+}

+static struct audioreach_module *audioreach_find_widget(struct snd_soc_component *comp,

• 					const char *name)
  

+{

• struct q6apm *apm = dev_get_drvdata(comp->dev);
• struct audioreach_module *module;
• int id = 0;

unnecessary init?

• idr_for_each_entry(&apm->modules_idr, module, id) {

• if (!strcmp(name, module->widget->name))
  

• 	return module;
  

• }
• return NULL;

+}

+/* DAI link - used for any driver specific init */ +static int audioreach_link_load(struct snd_soc_component *component, int index,

• 		struct snd_soc_dai_link *link,
  

• 		struct snd_soc_tplg_link_config *cfg)
  

+{

• link->nonatomic = true;
• link->dynamic = true;
• link->platforms->name = NULL;
• link->platforms->of_node = of_get_compatible_child(component->dev->of_node,

• 		"qcom,q6apm-dais");
  

• link->trigger[0] = SND_SOC_DPCM_TRIGGER_POST;
• link->trigger[1] = SND_SOC_DPCM_TRIGGER_POST;

can you add a comment on why you don't use the default order for FE/BE triggers?

• return 0;

+}

+static int audioreach_get_vol_ctrl_audio_mixer(struct snd_kcontrol *kcontrol,

• 		       struct snd_ctl_elem_value *ucontrol)
  

+{

• struct snd_soc_dapm_widget *dw = snd_soc_dapm_kcontrol_widget(kcontrol);
• struct audioreach_module *mod = dw->dobj.private;
• /* Check if the graph is active or not */

that comment seems like a copy/paste, there's no check.

• ucontrol->value.integer.value[0] = mod->gain;
• return 0;

+}

+static int audioreach_put_vol_ctrl_audio_mixer(struct snd_kcontrol *kcontrol,

• 		      struct snd_ctl_elem_value *ucontrol)
  

+{

• struct snd_soc_dapm_widget *dw = snd_soc_dapm_kcontrol_widget(kcontrol);
• struct snd_soc_dapm_context *dapm = snd_soc_dapm_kcontrol_dapm(kcontrol);
• struct snd_soc_component *c = snd_soc_dapm_to_component(dapm);
• struct audioreach_module *mod = dw->dobj.private;
• struct q6apm *apm = dev_get_drvdata(c->dev);
• int vol = ucontrol->value.integer.value[0];
• /* Check if the graph is active or not */
• if (dw->power) {

• audioreach_gain_set_vol_ctrl(apm, mod, vol);
  

• mod->gain = vol;
  

• return 1;
  

• }

shouldn't you cache the value and apply it when the graph is powered?

Also wondering why this isn't using pm_runtime or something? Is this re-inventing your own power management?

• dev_err(apm->dev, "Unable to set volume as graph is not active\n");
• return 0;

+}

+static int audioreach_control_load_mix(struct snd_soc_component *scomp,

• 			  struct snd_ar_control *scontrol,
  

• 			  struct snd_kcontrol_new *kc,
  

• 			  struct snd_soc_tplg_ctl_hdr *hdr)
  

+{

• struct snd_soc_tplg_mixer_control *mc;
• struct snd_soc_tplg_vendor_array *c_array;
• struct snd_soc_tplg_vendor_value_elem *c_elem;
• int tkn_count = 0;
• mc = container_of(hdr, struct snd_soc_tplg_mixer_control, hdr);
• c_array = (struct snd_soc_tplg_vendor_array *)mc->priv.data;
• c_elem = c_array->value;
• while (tkn_count <= (le32_to_cpu(c_array->num_elems) - 1)) {

• switch (le32_to_cpu(c_elem->token)) {
  

• case AR_TKN_U32_SUB_GRAPH_INSTANCE_ID:
  

• 	scontrol->sgid = le32_to_cpu(c_elem->value);
  

• 	break;
  

• default:
  

• 	/* Ignore other tokens */
  

• break;
  

indentation?

• }
  

• c_elem++;
  

• tkn_count++;
  

• }
• return 0;

+}

+int audioreach_tplg_init(struct snd_soc_component *component) +{

• struct device *dev = component->dev;
• const struct firmware *fw;
• int ret;
• ret = request_firmware(&fw, "audioreach.bin", dev);
• if (ret < 0) {

• dev_err(dev, "tplg fw audioreach.bin load failed with %d\n", ret);
  

• return ret;
  

• }
• ret = snd_soc_tplg_component_load(component, &audioreach_tplg_ops, fw);
• if (ret < 0) {

• dev_err(dev, "tplg component load failed%d\n", ret);
  

• release_firmware(fw);
  

• return -EINVAL;
  

• }

should you not release the firmware on success as well? that's what we do for SOF"

ret = snd_soc_tplg_component_load(scomp, &sof_tplg_ops, fw); if (ret < 0) { dev_err(scomp->dev, "error: tplg component load failed %d\n", ret); ret = -EINVAL; }

release_firmware(fw); return ret;

• return 0;

+} +EXPORT_SYMBOL_GPL(audioreach_tplg_init);