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

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20150708/51727fd7/attachment-0001.sig>