[PATCH v4 04/17] ASoC: Intel: avs: Inter process communication

[Pierre-Louis Bossart]

> +/*
> + * struct avs_dsp_ops - Platform-specific DSP operations
> + *
> + * @power: Power on or off DSP cores
> + * @reset: Enter or exit reset state on DSP cores
> + * @stall: Stall or run DSP cores

nit-pick: the description sounds like boolean states. add "callback to"

> + * @irq_handler: Top half of IPC servicing
> + * @irq_thread: Bottom half of IPC servicing
> + * @int_control: Enable or disable IPC interrupts

callback to ...

> + */
>   struct avs_dsp_ops {
>   	int (* const power)(struct avs_dev *, u32, bool);
>   	int (* const reset)(struct avs_dev *, u32, bool);
>   	int (* const stall)(struct avs_dev *, u32, bool);
> +	irqreturn_t (* const irq_handler)(int, void *);
> +	irqreturn_t (* const irq_thread)(int, void *);
> +	void (* const int_control)(struct avs_dev *, bool);
>   };

> +/*
> + * struct avs_ipc - DSP IPC context
> + *
> + * @dev: PCI device
> + * @rx: Reply message cache

cache? I find this confusing, what are you trying to say here?

> + * @default_timeout_ms: default message timeout in MS
> + * @ready: whether firmware is ready and communication is open
> + * @rx_completed: whether RX for previously sent TX has been received
> + * @rx_lock: for serializating manipulation of rx_* fields

typo: serializing

> + * @msg_lock: for synchronizing request handling
> + * @done_completion: DONE-part of IPC i.e. ROM and ACKs from FW
> + * @busy_completion: BUSY-part of IPC i.e. receiving responses from FW
> + */
> +struct avs_ipc {
> +	struct device *dev;
> +
> +	struct avs_ipc_msg rx;
> +	u32 default_timeout_ms;
> +	bool ready;
> +
> +	bool rx_completed;
> +	spinlock_t rx_lock;
> +	struct mutex msg_mutex;
> +	struct completion done_completion;
> +	struct completion busy_completion;
> +};
> +
> +#define AVS_EIPC	EREMOTEIO

I don't recall if I already mentioned this but I don't see the need for 
an intermediate redefinition of a Linux error code.

> +/*
> + * IPC handlers may return positive value (firmware error code) what denotes
> + * successful HOST <-> DSP communication yet failure to process specific request.
> + *
> + * Below macro converts returned value to linux kernel error code.
> + * All IPC callers MUST use it as soon as firmware error code is consumed.
> + */
> +#define AVS_IPC_RET(ret) \
> +	(((ret) <= 0) ? (ret) : -AVS_EIPC)
> +
> +static inline void avs_ipc_err(struct avs_dev *adev, struct avs_ipc_msg *tx,
> +			       const char *name, int error)
> +{
> +	/*
> +	 * If IPC channel is blocked e.g.: due to ongoing recovery,
> +	 * -EPERM error code is expected and thus it's not an actual error.
> +	 */
> +	if (error == -EPERM)
> +		dev_dbg(adev->dev, "%s 0x%08x 0x%08x failed: %d\n", name,

%# adds the 0x for you.

> +			tx->glb.primary, tx->glb.ext.val, error);
> +	else
> +		dev_err(adev->dev, "%s 0x%08x 0x%08x failed: %d\n", name,
> +			tx->glb.primary, tx->glb.ext.val, error);
> +}

> +static void avs_dsp_process_notification(struct avs_dev *adev, u64 header)
> +{
> +	struct avs_notify_mod_data mod_data;
> +	union avs_notify_msg msg = AVS_MSG(header);
> +	size_t data_size = 0;
> +	void *data = NULL;
> +
> +	/* Ignore spurious notifications until handshake is established. */

there's no handshake here, just an initial notification after which the 
IPC protocol can start?

> +	if (!adev->ipc->ready && msg.notify_msg_type != AVS_NOTIFY_FW_READY) {
> +		dev_dbg(adev->dev, "FW not ready, skip notification: 0x%08x\n",
> +			msg.primary);
> +		return;
> +	}
> +
> +	/* Calculate notification payload size. */
> +	switch (msg.notify_msg_type) {
> +	case AVS_NOTIFY_FW_READY:
> +		break;
> +
> +	case AVS_NOTIFY_PHRASE_DETECTED:
> +		data_size = sizeof(struct avs_notify_voice_data);
> +		break;
> +
> +	case AVS_NOTIFY_RESOURCE_EVENT:
> +		data_size = sizeof(struct avs_notify_res_data);
> +		break;
> +
> +	case AVS_NOTIFY_MODULE_EVENT:
> +		/* To know the total payload size, header needs to be read first. */
> +		memcpy_fromio(&mod_data, avs_uplink_addr(adev), sizeof(mod_data));
> +		data_size = sizeof(mod_data) + mod_data.data_size;
> +		break;
> +
> +	default:
> +		dev_info(adev->dev, "unknown notification: 0x%08x\n",
> +			 msg.primary);
> +		break;
> +	}
> +
> +	if (data_size) {
> +		data = kmalloc(data_size, GFP_KERNEL);
> +		if (!data)
> +			return;
> +
> +		memcpy_fromio(data, avs_uplink_addr(adev), data_size);
> +	}
> +
> +	/* Perform notification-specific operations. */
> +	switch (msg.notify_msg_type) {
> +	case AVS_NOTIFY_FW_READY:
> +		dev_dbg(adev->dev, "FW READY 0x%08x\n", msg.primary);
> +		adev->ipc->ready = true;
> +		complete(&adev->fw_ready);
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	kfree(data);
> +}