Re: [alsa-devel] [PATCH v2 5/6] ASoC: Intel: Add Skylake IPC library

On Fri, Jul 03, 2015 at 04:04:06PM +0530, Vinod Koul wrote:

Mostly looks good - a few small things here, nothing too major:

+static struct ipc_message *skl_ipc_reply_find_msg(struct sst_generic_ipc *ipc,

• u64 ipc_header)
  

+{

• struct ipc_message *msg = NULL;
• struct skl_ipc_header *header = (struct skl_ipc_header *)(&ipc_header);
• if (list_empty(&ipc->rx_list)) {

• dev_err(ipc->dev, "ipc: rx list is empty but received 0x%x\n",
  

• 	header->primary);
  

• goto out;
  

• }
• msg = list_first_entry(&ipc->rx_list, struct ipc_message, list);

This says it finds the message but it appears to just return the head of the list without reference to the supplied header other than casting it to skl_ipc_header and possibly using it in an error print?

+irqreturn_t skl_dsp_irq_thread_handler(int irq, void *context) +{

• struct sst_dsp *dsp = (struct sst_dsp *)context;
• struct skl_sst *skl = sst_dsp_get_thread_context(dsp);
• struct sst_generic_ipc *ipc = &skl->ipc;
• struct skl_ipc_header header = {0};
• u32 hipcie, hipct, hipcte;
• int ipc_irq = 0;
• /* Here we handle IPC interrupts only */
• if (!(dsp->intr_status & SKL_ADSPIS_IPC))

• return IRQ_HANDLED;
  

Shouldn't we be returning IRQ_NONE if we didn't handle the interrupt?

• if (IPC_GLB_NOTIFY_RSP_TYPE(header.primary)) {
  

• 	/* Handle Immediate reply from DSP Core */
  

• 	skl_ipc_process_reply(ipc, header);
  

• } else {
  

• 	trace_printk("IPC irq: Notification from firmware\n");
  

• 	skl_ipc_process_notification(ipc, header);
  

• }
  

Why trace_printk()?

+int skl_ipc_set_dx(struct sst_generic_ipc *ipc, u8 instance_id,

• u16 module_id, struct skl_ipc_dxstate_info *dx)
  

+{

• struct skl_ipc_header header = {0};
• u64 *ipc_header = (u64 *)(&header);
• int ret;
• header.primary = IPC_MSG_TARGET(IPC_MOD_MSG);
• header.primary |= IPC_MSG_DIR(IPC_MSG_REQUEST);
• header.primary |= IPC_GLB_TYPE(IPC_MOD_SET_DX);
• header.primary |= IPC_MOD_INSTANCE_ID(instance_id);
• header.primary |= IPC_MOD_ID(module_id);
• dev_dbg(ipc->dev, "In %s primary =%x ext=%x\n", __func__,

• 	 header.primary, header.extension);
  

• ret = sst_ipc_tx_message_wait(ipc, *ipc_header,

• 	(void *)dx, sizeof(dx), NULL, 0);
  

If you need to cast a pointer to void something is wrong... I suspect you just don't need the cast though.