On Mon, Apr 20, 2015 at 10:56:39PM +0100, Mark Brown wrote:
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).
Ah sorry about that, I did try to fix style issues in code but looks like few were missed. I will fix them in the patch series.
if (msg->rx_size) {if (rx_data)memcpy(rx_data, msg->rx_data, msg->rx_size);elsedev_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.
Thanks for pointing, yes this should be done.
+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/
For reply case, I am not sure. I think if thats true would make sense to complain loudly..
- 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.
Overrun is FW detectiong its pipelines are overunning the buffer, no the ALSA ring buffer though. _PHRASE_DETECTED is for keyword detection which can run on dsp
Thanks