[alsa-devel] [PATCH 07/12] ASoC: SOF: Implement Probe IPC API

[Cezary Rojewski]

On 2020-01-24 20:55, Pierre-Louis Bossart wrote:
> 
>> +/**
>> + * sof_ipc_probe_dma_add - attach to specified dmas
>> + * @sdev:    SOF sound device
>> + * @dma:    List of streams (dmas) to attach to
>> + * @num_dma:    Number of elements in @dma
>> + *
>> + * Contrary to extraction, injection streams are never assigned
>> + * on init. Before attempting any data injection, host is responsible
>> + * for specifying streams which will be later used to transfer data
>> + * to connected probe points.
>> + */
>> +int sof_ipc_probe_dma_add(struct snd_sof_dev *sdev,
>> +        struct sof_probe_dma *dma, size_t num_dma)
>> +{
>> +    struct sof_ipc_probe_dma_add_params *msg;
>> +    struct sof_ipc_reply reply;
>> +    size_t bytes = sizeof(*dma) * num_dma;
>> +    int ret;
>> +
>> +    msg = kmalloc(sizeof(*msg) + bytes, GFP_KERNEL);
>> +    if (!msg)
>> +        return -ENOMEM;
>> +    msg->hdr.size = sizeof(*msg);
>> +    msg->num_elems = num_dma;
>> +    msg->hdr.cmd = SOF_IPC_GLB_PROBE | SOF_IPC_PROBE_DMA_ADD;
>> +    memcpy(&msg->dma[0], dma, bytes);
>> +
>> +    ret = sof_ipc_tx_message(sdev->ipc, msg->hdr.cmd, msg,
>> +            msg->hdr.size + bytes, &reply, sizeof(reply));
>> +    kfree(msg);
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL(sof_ipc_probe_dma_add);
>> +
>> +/**
>> + * sof_ipc_probe_dma_remove - detach from specified dmas
>> + * @sdev:        SOF sound device
>> + * @stream_tag:    List of stream tags to detach from
>> + * @num_stream_tag:    Number of elements in @stream_tag
>> + *
>> + * Host sends DMA_REMOVE request to free previously attached stream
>> + * from being occupied for injection. Each detach operation should
>> + * match equivalent DMA_ADD. Detach only when all probes tied to
>> + * given stream have been disconnected.
>> + */
>> +int sof_ipc_probe_dma_remove(struct snd_sof_dev *sdev,
>> +        unsigned int *stream_tag, size_t num_stream_tag)
>> +{
>> +    struct sof_ipc_probe_dma_remove_params *msg;
>> +    struct sof_ipc_reply reply;
>> +    size_t bytes = sizeof(*stream_tag) * num_stream_tag;
>> +    int ret;
>> +
>> +    msg = kmalloc(sizeof(*msg) + bytes, GFP_KERNEL);
>> +    if (!msg)
>> +        return -ENOMEM;
>> +    msg->hdr.size = sizeof(*msg);
>> +    msg->num_elems = num_stream_tag;
>> +    msg->hdr.cmd = SOF_IPC_GLB_PROBE | SOF_IPC_PROBE_DMA_REMOVE;
>> +    memcpy(&msg->stream_tag[0], stream_tag, bytes);
>> +
>> +    ret = sof_ipc_tx_message(sdev->ipc, msg->hdr.cmd, msg,
>> +            msg->hdr.size + bytes, &reply, sizeof(reply));
>> +    kfree(msg);
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL(sof_ipc_probe_dma_remove);
> 
> a lot of the code is identical with only minor difference in num_elems 
> and the flags, could we use helper functions here?
> 
> This would help btw when we transition to the multi-client support, we'd 
> only need to update the IPC stuff in few locations.
> 

Structures used in the ipc handlers are different, that is why only 
_INFO requests are combined. Maybe I'm just blind or not enough tea 
drunk today.

>> + * Copyright(c) 2019 Intel Corporation. All rights reserved.
> 
> 2019-2020. Happy New Year.
> 
> I'd prefer it if we have the new header file in a separate patch added 
> first, I find it odd to review an implementation with the header added 
> last.
> 

Updated the headers in v2. probe.c and .h are added simultaneously in 
this patch. I do not see the problem with the patchset structure. Code 
is rather small, dividing it further can make it just harder to review.

Czarek