[Sound-open-firmware] [alsa-devel] [PATCH v3 04/14] ASoC: SOF: Add support for IPC IO between DSP and Host

[Pierre-Louis Bossart]

>> diff --git a/include/sound/sof/control.h b/include/sound/sof/control.h
>>
>> +/* generic channel mapped value data */
>> +struct sof_ipc_ctrl_value_chan {
>> +	uint32_t channel;	/**< channel map - enum sof_ipc_chmap */
>> +	uint32_t value;
> Any reason to avoid s32 and u32?
> If this is supposed to be shared with user-space (or user-space may
> use this as a reference of data struct), we should consider placing in
> uapi directory, too.

it's intentional

The includes shared with userspace are in include/uapi/sound/sof.

All the files in include/sound/sof, and this one in particular, are more 
for host-dsp IPC.

In those two cases, uapi and IPC files, we don't use s32 and u32. we 
could move this directory under include/uapi/sound/sof-ipc if you prefer?

>
>
>> +/* wait for IPC message reply */
>> +static int tx_wait_done(struct snd_sof_ipc *ipc, struct snd_sof_ipc_msg *msg,
>> +			void *reply_data)
>> +{
>> +	struct snd_sof_dev *sdev = ipc->sdev;
>> +	struct sof_ipc_cmd_hdr *hdr = (struct sof_ipc_cmd_hdr *)msg->msg_data;
>> +	unsigned long flags;
>> +	int ret;
>> +
>> +	/* wait for DSP IPC completion */
>> +	ret = wait_event_timeout(msg->waitq, msg->ipc_complete,
>> +				 msecs_to_jiffies(IPC_TIMEOUT_MSECS));
>> +
>> +	spin_lock_irqsave(&sdev->ipc_lock, flags);
> Since this must be a sleepable context, you can safely use
> spin_lock_irq() here.
>
>> +/* send IPC message from host to DSP */
>> +int sof_ipc_tx_message(struct snd_sof_ipc *ipc, u32 header,
>> +		       void *msg_data, size_t msg_bytes, void *reply_data,
>> +		       size_t reply_bytes)
>> +{
>> +	struct snd_sof_dev *sdev = ipc->sdev;
>> +	struct snd_sof_ipc_msg *msg;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&sdev->ipc_lock, flags);
> Ditto.  This one calls tx_wait_done() later.
>
> It's better to define more strictly which one can be called from the
> spinlocked context and which not.

This one is for Keyon and team. I've asked that question multiple times 
and was told the irqsave was needed. Keyon, can you please chime in?

>
>
>> +void snd_sof_ipc_free(struct snd_sof_dev *sdev)
>> +{
>> +	cancel_work_sync(&sdev->ipc->tx_kwork);
>> +	cancel_work_sync(&sdev->ipc->rx_kwork);
>> +}
>> +EXPORT_SYMBOL(snd_sof_ipc_free);
> Not specific to this function but a general question:
> why not EXPORT_SYMBOL_GPL() in general in the whole SOF codes?

We use a dual license (copied below)

// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
//
// This file is provided under a dual BSD/GPLv2 license.  When using or
// redistributing this file, you may do so under either license.