[PATCH v8 02/14] ASoC: Intel: catpt: Implement IPC protocol

[Andy Shevchenko]

On Wed, Sep 23, 2020 at 02:24:56PM +0200, Cezary Rojewski wrote:
> Implement IRQ handlers for immediate and delayed replies and
> notifications. Communication is synchronous and allows for serialization
> of maximum one message at a time.
> 
> DSP may respond with ADSP_PENDING status for a request - known as
> delayed reply - and when situation occurs, framework keeps the lock and
> awaits upcoming response through IPCD channel which is handled in
> bottom-half. Immediate replies spawn no BH at all as their processing is
> very short.

...

> +static void catpt_dsp_send_tx(struct catpt_dev *cdev,
> +			      const struct catpt_ipc_msg *tx)
> +{
> +	u32 header = tx->header | CATPT_IPCC_BUSY;

> +	if (tx->size)

This check is not needed.

> +		memcpy_toio(catpt_outbox_addr(cdev), tx->data, tx->size);
> +	catpt_writel_shim(cdev, IPCC, header);
> +}

...

> +	ret = ipc->rx.rsp.status;
> +	if (reply) {

> +		reply->header = ipc->rx.header;

So, even in case of error the header is still updated?

> +		if (!ret && reply->data && reply->size)

> +			memcpy(reply->data, ipc->rx.data, ipc->rx.size);

This I didn't get. You copy data by using source size?!

> +	}
> +
> +	return ret;

I guess the above piece may be refactored, but I don't know how until it is
clear why it's written like this.

...

> +static void catpt_dsp_copy_rx(struct catpt_dev *cdev, u32 header)
> +{
> +	struct catpt_ipc *ipc = &cdev->ipc;
> +
> +	ipc->rx.header = header;
> +	if (ipc->rx.size && ipc->rx.rsp.status == CATPT_REPLY_SUCCESS) {

Check for size is redundant.

> +		memcpy_fromio(ipc->rx.data, catpt_outbox_addr(cdev),
> +			      ipc->rx.size);
> +	}

Can be done like

	if (status != SUCCESS)
		return;
	memcpy_fromio(...);

> +}

-- 
With Best Regards,
Andy Shevchenko