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

From: Mark Brown <broonie@kernel.org>
To: Vinod Koul <vinod.koul@intel.com>
Cc: alsa-devel@alsa-project.org, tiwai@suse.de,
	liam.r.girdwood@linux.intel.com, patches.audio@intel.com,
	Jeeja KP <jeeja.kp@intel.com>,
	"Subhransu S. Prusty" <subhransu.s.prusty@intel.com>
Subject: Re: [PATCH v2 5/6] ASoC: Intel: Add Skylake IPC library
Date: Wed, 8 Jul 2015 19:46:27 +0100	[thread overview]
Message-ID: <20150708184627.GB11162@sirena.org.uk> (raw)
In-Reply-To: <1435919647-14049-6-git-send-email-vinod.koul@intel.com>

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

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.

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

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