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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Sat, Sep 26, 2020 at 10:58:58AM +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 int catpt_dsp_do_send_msg(struct catpt_dev *cdev,
> +				 struct catpt_ipc_msg request,
> +				 struct catpt_ipc_msg *reply, int timeout)
> +{
> +	struct catpt_ipc *ipc = &cdev->ipc;
> +	unsigned long flags;
> +	int ret;
> +
> +	if (!ipc->ready)
> +		return -EPERM;
> +	if (request.size > ipc->config.outbox_size ||
> +	    (reply && reply->size > ipc->config.outbox_size))
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&ipc->lock, flags);
> +	catpt_ipc_msg_init(ipc, reply);
> +	catpt_dsp_send_tx(cdev, &request);
> +	spin_unlock_irqrestore(&ipc->lock, flags);
> +
> +	ret = catpt_wait_msg_completion(cdev, timeout);
> +	if (ret) {
> +		dev_crit(cdev->dev, "communication severed: %d, rebooting dsp..\n",
> +			 ret);
> +		ipc->ready = false;
> +		/* TODO: attempt recovery */
> +		return ret;
> +	}

> +	ret = ipc->rx.rsp.status;
> +	if (reply) {
> +		reply->header = ipc->rx.header;
> +		if (!ret && reply->data)
> +			memcpy(reply->data, ipc->rx.data, reply->size);
> +	}
> +
> +	return ret;

One more looking into this... What about

	if (reply)
		reply->header = ipc->rx.header;

	ret = ipc->rx.rsp.status; // or even directly if (status) return status.
	if (ret)
		return ret;

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

	return 0;

It may be verbose but I think readability is better here.

> +}

...

> +	CATPT_CHANNEL_CONFIG_5_POINT_0	= 7, /* L, C, R, Ls & Rs */
> +	CATPT_CHANNEL_CONFIG_5_POINT_1	= 8, /* L, C, R, Ls, Rs & LFE */
> +	CATPT_CHANNEL_CONFIG_DUAL_MONO	= 9, /* One channel replicated in two */
> +	CATPT_CHANNEL_CONFIG_INVALID	= 10,

Hmm... I think I got the point why DUAL_MONO was at the end of the switch-case.

...

> +enum catpt_module_id {
> +	CATPT_MODID_BASE_FW = 0x0,
> +	CATPT_MODID_MP3 = 0x1,
> +	CATPT_MODID_AAC_5_1 = 0x2,
> +	CATPT_MODID_AAC_2_0 = 0x3,
> +	CATPT_MODID_SRC = 0x4,
> +	CATPT_MODID_WAVES = 0x5,
> +	CATPT_MODID_DOLBY = 0x6,
> +	CATPT_MODID_BOOST = 0x7,
> +	CATPT_MODID_LPAL = 0x8,
> +	CATPT_MODID_DTS = 0x9,
> +	CATPT_MODID_PCM_CAPTURE = 0xA,
> +	CATPT_MODID_PCM_SYSTEM = 0xB,
> +	CATPT_MODID_PCM_REFERENCE = 0xC,
> +	CATPT_MODID_PCM = 0xD, /* offload */
> +	CATPT_MODID_BLUETOOTH_RENDER = 0xE,
> +	CATPT_MODID_BLUETOOTH_CAPTURE = 0xF,
> +	CATPT_MODID_LAST = CATPT_MODID_BLUETOOTH_CAPTURE,
> +};

if you indent the values to be on the same column it will increase readability.

...

> +enum catpt_mclk_frequency {
> +	CATPT_MCLK_OFF = 0,
> +	CATPT_MCLK_FREQ_6_MHZ = 1,
> +	CATPT_MCLK_FREQ_21_MHZ = 2,
> +	CATPT_MCLK_FREQ_24_MHZ = 3,
> +};

Looks like a 3 MHz as base frequency with multiplicators 0, 2, 7, 8. 

...

> +#define CATPT_STREAM_MSG(msg_type) \
> +{ \
> +	.stream_msg_type = CATPT_STRM_##msg_type, \
> +	.global_msg_type = CATPT_GLB_STREAM_MESSAGE }
> +#define CATPT_STAGE_MSG(msg_type) \
> +{ \
> +	.stage_action = CATPT_STG_##msg_type, \
> +	.stream_msg_type = CATPT_STRM_STAGE_MESSAGE, \
> +	.global_msg_type = CATPT_GLB_STREAM_MESSAGE }

Hmm... This split is interesting. I would rather move } to a new line.

...

> @@ -15,7 +15,6 @@
>  #define CATPT_SHIM_REGS_SIZE	4096
>  #define CATPT_DMA_REGS_SIZE	1024
>  #define CATPT_DMA_COUNT		2

> -#define CATPT_SSP_COUNT		2

Why is this?

>  #define CATPT_SSP_REGS_SIZE	512
>  
>  /* DSP Shim registers */

-- 
With Best Regards,
Andy Shevchenko