Re: [RFC 2/9] ASoC: hda: Add IPC library for SKL platform

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2338 bytes --]

On Fri, Apr 17, 2015 at 06:46:00PM +0530, Vinod Koul wrote:

> +
> +/* IPC data */
> +struct ssth_ipc {
> +	struct device *dev;
> +	struct ssth_lib *dsp;
> +
> +/* IPC messaging */
> +	struct list_head tx_list;

Lots of odd indentation of comments in this code (well, several examples
I noticed so far anyway).

> +	if (ret == 0)
> +		ret = -ETIMEDOUT;
> +	else {
> +	/* copy the data returned from DSP */

Coding style - { } on both sides of the if (and the comments again).

> +		if (msg->rx_size) {
> +			if (rx_data)
> +				memcpy(rx_data, msg->rx_data, msg->rx_size);
> +			else
> +				dev_err(ipc->dev, "error: no output buffer");
> +		}
> +		ret = msg->errno;
> +	}
> +
> +	ssth_ipc_msg_put_empty(ipc, msg);
> +
> +	spin_unlock_irqrestore(&ipc->ipc_lock, irq_flags);

Can we pop the message off the list, release the lock and then copy?
That way we can avoid having interrupts disabled while we do the
memcpy().  In general there seems to be a lot of interrupts disabled
copying going on (which is there for some of the other drivers too)
which might be avoidable.

> +static void ssth_ipc_reply_remove(struct ssth_ipc *ipc, struct ssth_ipc_message *msg)
> +{
> +	unsigned long irq_flags;
> +
> +	spin_lock_irqsave(&ipc->ipc_lock, irq_flags);
> +	if (list_empty(&ipc->rx_list)) {
> +		dev_dbg(ipc->dev, "empty rx list");
> +		goto out;
> +	}
> +	list_del(&msg->list);
> +
> +out:
> +	spin_unlock_irqrestore(&ipc->ipc_lock, irq_flags);
> +}

Are we expecting to not find the message/

> +	if (IPC_GLB_NOTIFI_MSG_TYPE(header.primary)) {
> +		switch (IPC_GLB_NOTIFI_TYPE(header.primary)) {
> +		case IPC_GLB_NOTIFCATION_GLITCH:
> +			break;
> +		case IPC_GLB_NOTIFCATION_OVERRUN:
> +			break;
> +		case IPC_GLB_NOTIFCATION_UNDERRUN:
> +			dev_dbg(ipc->dev, "FW UNDERRUN\n");
> +			break;
> +		case IPC_GLB_NOTIFCATION_END_STREAM:
> +			break;
> +		case IPC_GLB_NOTIFCATION_PHRASE_DETECTED:
> +			break;
> +		case IPC_GLB_NOTIFCATION_RESOURCE_EVENT:
> +			dev_dbg(ipc->dev, "MCPS Budget Violation\n");
> +			break;
> +		case IPC_GLB_NOTIFCATION_LOG_BUFFER_STATUS:
> +			break;
> +		case IPC_GLB_NOTIFCATION_TIMESTAMP_CAPTURED:
> +			break;
> +		case IPC_GLB_NOTIFCATION_FW_READY:
> +			ipc->boot_complete = true;
> +			wake_up(&ipc->boot_wait);
> +			break;

More prints perhaps?  _PHRASE_DETECTED looks interesting, as does
_OVERRUN.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]