Re: [PATCH 07/17] ASoC: Intel: avs: Add module management requests

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

> +int avs_ipc_init_instance(struct avs_dev *adev, u16 module_id, u8 instance_id,
> +			  u8 ppl_id, u8 core_id, u8 domain,

you should explain the relationship between ppl_id and core_id. It seems
that in the same pipeline different modules instances can be pegged to
different cores, which isn't very intuitive given the previous
explanation that a pipeline is a scheduling unit.

The domain as a u8 is not very clear either, I was under the impression
there were only two domains (LL and EDF)?

> +			  void *param, u32 param_size)
> +{
> +	union avs_module_msg msg = AVS_MODULE_REQUEST(INIT_INSTANCE);
> +	struct avs_ipc_msg request;
> +	int ret;
> +
> +	msg.module_id = module_id;
> +	msg.instance_id = instance_id;
> +	/* firmware expects size provided in dwords */
> +	msg.ext.init_instance.param_block_size =
> +			DIV_ROUND_UP(param_size, sizeof(u32));
> +	msg.ext.init_instance.ppl_instance_id = ppl_id;
> +	msg.ext.init_instance.core_id = core_id;
> +	msg.ext.init_instance.proc_domain = domain;
> +
> +	request.header = msg.val;
> +	request.data = param;
> +	request.size = param_size;

isn't there a need to check if the module can be initialized? there's
got to be some dependency on pipeline state?

> +
> +	ret = avs_dsp_send_msg(adev, &request, NULL);
> +	if (ret)
> +		avs_ipc_err(adev, &request, "init instance", ret);
> +
> +	return ret;
> +}
> +
> +int avs_ipc_delete_instance(struct avs_dev *adev, u16 module_id, u8 instance_id)
> +{
> +	union avs_module_msg msg = AVS_MODULE_REQUEST(DELETE_INSTANCE);
> +	struct avs_ipc_msg request = {0};
> +	int ret;
> +
> +	msg.module_id = module_id;
> +	msg.instance_id = instance_id;
> +	request.header = msg.val;
> +
> +	ret = avs_dsp_send_msg(adev, &request, NULL);
> +	if (ret)
> +		avs_ipc_err(adev, &request, "delete instance", ret);
> +
> +	return ret;

same here, can this be used in any pipeline state?

> +}
> +
> +int avs_ipc_bind(struct avs_dev *adev, u16 module_id, u8 instance_id,
> +		 u16 dst_module_id, u8 dst_instance_id,
> +		 u8 dst_queue, u8 src_queue)

what does a queue represent?

> +{
> +	union avs_module_msg msg = AVS_MODULE_REQUEST(BIND);
> +	struct avs_ipc_msg request = {0};
> +	int ret;
> +
> +	msg.module_id = module_id;
> +	msg.instance_id = instance_id;
> +	msg.ext.bind_unbind.dst_module_id = dst_module_id;
> +	msg.ext.bind_unbind.dst_instance_id = dst_instance_id;
> +	msg.ext.bind_unbind.dst_queue = dst_queue;
> +	msg.ext.bind_unbind.src_queue = src_queue;
> +	request.header = msg.val;
> +
> +	ret = avs_dsp_send_msg(adev, &request, NULL);
> +	if (ret)
> +		avs_ipc_err(adev, &request, "bind modules", ret);
> +
> +	return ret;
> +}
> +
> +int avs_ipc_unbind(struct avs_dev *adev, u16 module_id, u8 instance_id,
> +		   u16 dst_module_id, u8 dst_instance_id,
> +		   u8 dst_queue, u8 src_queue)
> +{
> +	union avs_module_msg msg = AVS_MODULE_REQUEST(UNBIND);
> +	struct avs_ipc_msg request = {0};
> +	int ret;
> +
> +	msg.module_id = module_id;
> +	msg.instance_id = instance_id;
> +	msg.ext.bind_unbind.dst_module_id = dst_module_id;
> +	msg.ext.bind_unbind.dst_instance_id = dst_instance_id;
> +	msg.ext.bind_unbind.dst_queue = dst_queue;
> +	msg.ext.bind_unbind.src_queue = src_queue;
> +	request.header = msg.val;
> +
> +	ret = avs_dsp_send_msg(adev, &request, NULL);
> +	if (ret)
> +		avs_ipc_err(adev, &request, "unbind modules", ret);
> +
> +	return ret;

can this be merged with the bind in a helper, the code looks
quasi-identical with just two lines different.

> +}