[PATCH v6 16/22] ASoC: qdsp6: audioreach: add module configuration command helpers

[Srinivas Kandagatla]

Thanks Pierre,

On 15/09/2021 17:11, Pierre-Louis Bossart wrote:
> 
>> +int q6apm_map_memory_regions(struct q6apm_graph *graph, unsigned int dir, phys_addr_t phys,
>> +			     size_t period_sz, unsigned int periods)
>> +{
>> +	struct audioreach_graph_data *data;
>> +	struct audio_buffer *buf;
>> +	int cnt;
>> +	int rc;
>> +
>> +	if (dir == SNDRV_PCM_STREAM_PLAYBACK)
>> +		data = &graph->rx_data;
>> +	else
>> +		data = &graph->tx_data;
>> +
>> +	mutex_lock(&graph->lock);
>> +
>> +	if (data->buf) {
>> +		dev_err(graph->dev, "Buffer already allocated\n");
>> +		mutex_unlock(&graph->lock);
>> +		return 0;
> 
> is this an error worth of dev_err() if you return 0?
No, its removed now.

> 
>> +	}
>> +
>> +	buf = kzalloc(((sizeof(struct audio_buffer)) * periods), GFP_KERNEL);
>> +	if (!buf) {
>> +		mutex_unlock(&graph->lock);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	if (dir == SNDRV_PCM_STREAM_PLAYBACK)
>> +		data = &graph->rx_data;
>> +	else
>> +		data = &graph->tx_data;
>> +
>> +	data->buf = buf;
>> +
>> +	buf[0].phys = phys;
>> +	buf[0].size = period_sz;
>> +
>> +	for (cnt = 1; cnt < periods; cnt++) {
>> +		if (period_sz > 0) {
>> +			buf[cnt].phys = buf[0].phys + (cnt * period_sz);
>> +			buf[cnt].size = period_sz;
>> +		}
>> +	}
>> +	data->num_periods = periods;
>> +
>> +	mutex_unlock(&graph->lock);
>> +
>> +	rc = audioreach_map_memory_regions(graph, dir, period_sz, periods, 1);
>> +	if (rc < 0) {
>> +		dev_err(graph->dev, "Memory_map_regions failed\n");
>> +		audioreach_graph_free_buf(graph);
>> +	}
>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_GPL(q6apm_map_memory_regions);
> 
>> +int q6apm_write_async(struct q6apm_graph *graph, uint32_t len, uint32_t msw_ts,
>> +		      uint32_t lsw_ts, uint32_t wflags)
>> +{
>> +	struct apm_data_cmd_wr_sh_mem_ep_data_buffer_v2 *write;
>> +	int rc, payload_size, iid;
>> +	struct audio_buffer *ab;
>> +	struct gpr_pkt *pkt;
>> +
>> +	payload_size = sizeof(*write);
> 
> nit-pick on variable-naming: I get confused between actions and objects.

this is payload_size for the command packet.

> 
>> +
>> +	iid = q6apm_graph_get_rx_shmem_module_iid(graph);
>> +	pkt = audioreach_alloc_pkt(payload_size, DATA_CMD_WR_SH_MEM_EP_DATA_BUFFER_V2,
>> +				 graph->rx_data.dsp_buf | (len << APM_WRITE_TOKEN_LEN_SHIFT),
>> +				 graph->port->id, iid);
>> +	if (IS_ERR(pkt))
>> +		return -ENOMEM;
>> +
>> +	write = (void *)pkt + GPR_HDR_SIZE;
>> +
>> +	mutex_lock(&graph->lock);
>> +	ab = &graph->rx_data.buf[graph->rx_data.dsp_buf];
>> +
>> +	write->buf_addr_lsw = lower_32_bits(ab->phys);
>> +	write->buf_addr_msw = upper_32_bits(ab->phys);
>> +	write->buf_size = len;
>> +	write->timestamp_lsw = lsw_ts;
>> +	write->timestamp_msw = msw_ts;
>> +	write->mem_map_handle = graph->rx_data.mem_map_handle;
>> +	write->flags = wflags;
>> +
>> +	graph->rx_data.dsp_buf++;
>> +
>> +	if (graph->rx_data.dsp_buf >= graph->rx_data.num_periods)
>> +		graph->rx_data.dsp_buf = 0;
>> +
>> +	mutex_unlock(&graph->lock);
>> +
>> +	rc = gpr_send_port_pkt(graph->port, pkt);
>> +
>> +	kfree(pkt);
>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_GPL(q6apm_write_async);
>> +
>> +int q6apm_read(struct q6apm_graph *graph)
>> +{
>> +	struct data_cmd_rd_sh_mem_ep_data_buffer_v2 *read;
>> +	struct audioreach_graph_data *port;
>> +	struct audio_buffer *ab;
>> +	struct gpr_pkt *pkt;
>> +	int rc, iid;
>> +
>> +	iid = q6apm_graph_get_tx_shmem_module_iid(graph);
>> +	pkt = audioreach_alloc_pkt(sizeof(*read), DATA_CMD_RD_SH_MEM_EP_DATA_BUFFER_V2,
>> +				 graph->tx_data.dsp_buf, graph->port->id, iid);
>> +	if (IS_ERR(pkt))
>> +		return -ENOMEM;
>> +
>> +	read = (void *)pkt + GPR_HDR_SIZE;
> 
> same nit-pick on variable naming, with the additional present/past
> grammar issue that you don't know if it's a read buffer or a pointer to
> data read in the past.
> 

do you think adding "_cmd" suffix like read_cmd would make more sense?

--srini


>> +
>> +	mutex_lock(&graph->lock);
>> +	port = &graph->tx_data;
>> +	ab = &port->buf[port->dsp_buf];
>> +
>> +	read->buf_addr_lsw = lower_32_bits(ab->phys);
>> +	read->buf_addr_msw = upper_32_bits(ab->phys);
>> +	read->mem_map_handle = port->mem_map_handle;
>> +	read->buf_size = ab->size;
>> +
>> +	port->dsp_buf++;
>> +
>> +	if (port->dsp_buf >= port->num_periods)
>> +		port->dsp_buf = 0;
>> +
>> +	mutex_unlock(&graph->lock);
>> +
>> +	rc = gpr_send_port_pkt(graph->port, pkt);
>> +	kfree(pkt);
>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_GPL(q6apm_read);
>> +
>>   static int graph_callback(struct gpr_resp_pkt *data, void *priv, int op)
>>   {
>>   	struct data_cmd_rsp_rd_sh_mem_ep_data_buffer_done_v2 *rd_done;
>>