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

[Vinod Koul]

On Wed, Jul 08, 2015 at 07:46:27PM +0100, Mark Brown wrote:
> 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?
Yes you are right. Actually currently we dont expect to queue up so we never
bother looking up. But yes intention to find messages when they are quued, I
will add that bit

> > +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?
Yes this is wrong, will fix

> 
> > +		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()?
Dont see a reason, this should be a printk, will fix
> 
> > +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.
yes i rechecked. The API expects void pointer for buffer for IPC payload and
will copy that. We need not cast it though

Thanks for quick review
-- 
~Vinod
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20150709/6ac64906/attachment.sig>