[alsa-devel] [PATCH] [RFC 4/13] Intel SST driver common private headers

Takashi Iwai tiwai at suse.de
Fri Jul 3 13:04:27 CEST 2009


At Fri,  3 Jul 2009 12:33:32 +0530,
Vinod Koul wrote:
> 
> This patch adds header files private to the SST
> driver and contains the common structures like driver context,
> DSP register definitions, and inline utility
> functions used by the SST driver and function prototypes of
> common functions that are implemented in SST driver
> 
> Signed-off-by: Vinod Koul <vinod.koul at intel.com>
> Signed-off-by: Harsha Priya <priya.harsha at intel.com>
> 
> 	new file:   sound/pci/sst/intel_sst_common.h
> 	new file:   sound/pci/sst/intel_sst_pvt.h

BTW, please don't add comments after sing-off.
If any, write below '---' line so that git will ignore it.

> ---
>  sound/pci/sst/intel_sst_common.h |  292 +++++++++++++++++++++++++++++
>  sound/pci/sst/intel_sst_pvt.h    |  378 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 670 insertions(+), 0 deletions(-)
>  create mode 100644 sound/pci/sst/intel_sst_common.h
>  create mode 100644 sound/pci/sst/intel_sst_pvt.h
> 
> diff --git a/sound/pci/sst/intel_sst_common.h b/sound/pci/sst/intel_sst_common.h
> new file mode 100644
> index 0000000..e039489
> --- /dev/null
> +++ b/sound/pci/sst/intel_sst_common.h
> +/*
> +SST shim registers to structure mapping
> +*/
> +union config_status_reg {
> +	struct {
> +		u32 rsvd0:1;
> +		u32 sst_reset:1;
> +		u32 hw_rsvd:3;
> +		u32 sst_clk:2;
> +		u32 bypass:3;
> +		u32 run_stall:1;
> +		u32 rsvd:21;
> +	} part;
> +	u32 full;

A union with bit fields is very bad for portability.
If it's used as a communication parameter, don't use it.
Just pass u32 and get/set via normal bit and/or operations.

> +struct snd_sst_user_cap_list {
> +	unsigned int iov_index; /*index of iov*/
> +	unsigned long iov_offset; /*offset in iov*/
> +	unsigned long offset; /*offset in kmem*/
> +	unsigned long size; /*size copied*/

Sure to use long here?

> diff --git a/sound/pci/sst/intel_sst_pvt.h b/sound/pci/sst/intel_sst_pvt.h
> new file mode 100644
> index 0000000..a1e786e
> --- /dev/null
> +++ b/sound/pci/sst/intel_sst_pvt.h
(snip)
> +static inline int sst_scu_ipc_write(u32 addr, u32 value)
> +{

What this must be inline?

> +#ifndef CONFIG_SST_IPC_NOT_INCLUDED
> +	int retval = 0, retry = 3;
> +	struct ipc_reg_data ipc_reg = {0};
> +
> +	ipc_reg.address = addr;
> +	ipc_reg.data = value;
> +	ipc_reg.ioc = 1;
> +
> +	while (retry) {
> +		retval = mrst_ipc_write32(&ipc_reg);
> +		if (retval == 0)
> +			break;
> +		retry--;
> +		/*error*/
> +		sst_err("IPC write failed %x\n", retval);
> +	}
> +	return retval;
> +#else
> +	return 0;
> +#endif
> +}
> +static inline void sst_fill_header(union ipc_header *header,
> +				int msg, int large, int strID)
> +{
> +	header->part.msg_id = msg;
> +	header->part.str_id = strID;
> +	header->part.large = large;
> +	header->part.done = 0;
> +	header->part.busy = 1;
> +	header->part.data = 0;
> +}
> +
> +static inline int sst_get_stream_id(struct intel_sst_drv *sst_ops, int pvt_id)
> +{
> +	int i;
> +
> +	for (i = 1; i < MAX_NUM_STREAMS; i++) {
> +		if (pvt_id == sst_ops->streams[i].sst_id)
> +			return i;
> +	}
> +	return -EINVAL;
> +}
> +
> +static inline unsigned int sst_assign_pvt_id(struct intel_sst_drv *sst_ops)
> +{
> +	sst_ops->unique_id++;
> +	if (sst_ops->unique_id >= MAX_NUM_STREAMS)
> +		sst_ops->unique_id = 1;
> +	return sst_ops->unique_id;
> +}
> +
> +static inline int sst_get_block_stream(struct intel_sst_drv *sst_ops)

This looks too big for inline.

> +{
> +	int i;
> +
> +	for (i = 0; i < MAX_ACTIVE_STREAM; i++) {
> +		if (BLOCK_UNINIT == sst_ops->alloc_block[i].sst_id) {
> +			sst_ops->alloc_block[i].ops_block.condition = false;
> +			sst_ops->alloc_block[i].ops_block.ret_code = 0;
> +			sst_ops->alloc_block[i].sst_id = 0;
> +			break;
> +		}
> +	}
> +	if (MAX_ACTIVE_STREAM == i) {
> +		sst_err("max alloc_stream reached \n");
> +		i = -EBUSY; /*active stream limit reached*/
> +	}
> +	return i;
> +}
> +
> +
> +
> +static inline void sst_print_hex(unsigned char *buf, unsigned int size)

And this doesn't have to be.

> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < size; i++) {
> +		sst_dbg("%02x ", buf[i]);
> +		if ((i != 0) && ((i % 8) == 0))
> +			sst_dbg("\n");
> +	}
> +}
> +
> +static inline void sst_init_stream(struct stream_info *stream,
> +		int codec, int sst_id, int ops)
> +{
> +	stream->status = STREAM_INIT;
> +	stream->prev = STREAM_UN_INIT;
> +	stream->codec = codec;
> +	stream->sst_id = sst_id;
> +	stream->ops = ops;
> +	stream->data_blk.on = false;
> +	stream->data_blk.condition = false;
> +	stream->data_blk.ret_code = 0;
> +	stream->data_blk.data = NULL;
> +	stream->ctrl_blk.on = false;
> +	stream->ctrl_blk.condition = false;
> +	stream->ctrl_blk.ret_code = 0;
> +	stream->ctrl_blk.data = NULL;
> +	stream->mmapped = false;
> +}
> +
> +static inline void sst_clean_stream(struct stream_info *stream)
> +{
> +	struct sst_stream_bufs *bufs = NULL, *_bufs = NULL;
> +	stream->status = STREAM_UN_INIT;
> +	stream->prev = STREAM_UN_INIT;
> +	list_for_each_entry_safe(bufs, _bufs, &stream->bufs, node) {
> +		list_del(&bufs->node);
> +		kfree(bufs);
> +	}
> +}
> +
> +static inline int sst_wait_interruptible(struct intel_sst_drv *sst_ops,
> +				struct sst_block *block)
> +{
> +	int retval = 0;
> +
> +	if (!wait_event_interruptible(sst_ops->wait_queue, block->condition)) {
> +		/*event wake*/
> +		if (0 != block->ret_code) {
> +			sst_err("stream failed %d\n", block->ret_code);
> +			retval = -EBUSY;
> +		} else {
> +			sst_dbg("event up\n");
> +			retval = 0;
> +		}
> +	} else {
> +		sst_err("signal interrupted\n");
> +		retval = -EINTR;
> +	}
> +	return retval;
> +
> +}
> +
> +static inline int sst_wait_interruptible_timeout(struct intel_sst_drv *sst_ops,
> +				struct sst_block *block, int timeout)
> +{
> +	int retval = 0;
> +
> +	sst_dbg("waiting....\n");
> +	if (wait_event_interruptible_timeout(sst_ops->wait_queue,
> +						block->condition,
> +						msecs_to_jiffies(timeout))) {
> +		/*event wake*/
> +		sst_dbg("Event wake ...\n");
> +		if (0 != block->ret_code)
> +			sst_err("stream failed %d\n", block->ret_code);
> +		else
> +			sst_dbg("event up\n");
> +		retval = block->ret_code;
> +	} else {
> +		sst_err("timeout occured...\n");
> +		retval = -EBUSY;
> +	}
> +	return retval;
> +
> +}
> +
> +static inline int sst_wait_timeout(struct intel_sst_drv *sst_ops,
> +		struct stream_alloc_block *block)
> +{
> +	int retval = 0;
> +
> +	/*NOTE:
> +	Observed that FW processes the alloc msg and replies even
> +	before the alloc thread has finished execution*/
> +	sst_dbg("waiting for %x, condition %x \n", block->sst_id,
> +			block->ops_block.condition);
> +	if (wait_event_interruptible_timeout(sst_ops->wait_queue,
> +				block->ops_block.condition,
> +				msecs_to_jiffies(SST_BLOCK_TIMEOUT))) {
> +		/*event wake*/
> +		sst_dbg("Event wake ... %x \n", block->ops_block.condition);
> +		/*check return*/
> +		if (0 > block->ops_block.ret_code)
> +			sst_err("blk failed %d\n", block->ops_block.ret_code);
> +		else
> +			sst_dbg("blk ret: %d\n", block->ops_block.ret_code);
> +		retval = block->ops_block.ret_code;
> +	} else {
> +		sst_err("Wait timed-out %x\n", block->ops_block.condition);
> +		retval = -EBUSY;
> +	}
> +	return retval;
> +
> +}
> +
> +
> +static inline int sst_create_large_msg(struct ipc_post **arg)
> +{
> +	struct ipc_post *msg;
> +
> +	msg = kzalloc(sizeof(struct ipc_post), GFP_ATOMIC);
> +	if (NULL == msg) {
> +		sst_err("kzalloc msg failed \n");
> +		return -ENOMEM;
> +	}
> +
> +	msg->mailbox_data = kzalloc(SST_MAILBOX_SIZE, GFP_ATOMIC);
> +	if (NULL == msg->mailbox_data) {
> +		kfree(msg);
> +		sst_err("kzalloc mailbox_data failed \n");
> +		return -ENOMEM;
> +	};
> +	*arg = msg;
> +	return 0;
> +}
> +
> +static inline int sst_create_short_msg(struct ipc_post **arg)
> +{
> +	struct ipc_post *msg;
> +
> +	msg = kzalloc(sizeof(*msg), GFP_ATOMIC);
> +	if (NULL == msg) {
> +		sst_err("kzalloc msg failed \n");
> +		return -ENOMEM;
> +	}
> +	msg->mailbox_data = NULL;
> +	*arg = msg;
> +	return 0;
> +}
> +
> +static inline void sst_print_params(struct snd_sst_stream_params *sparam)
> +{
> +	switch (sparam->uc.pcm_params.codec) {
> +	case SST_CODEC_TYPE_PCM:
> +		sst_dbg("pcm \n");
> +		sst_dbg("chan=%d, sfreq = %d, wd_sz = %d \
> +				 brate = %d framesize = %d \
> +				 samples_per_frame = %d \
> +				 period_cnt = %d\n",
> +			sparam->uc.pcm_params.num_chan,
> +			sparam->uc.pcm_params.sfreq,
> +			sparam->uc.pcm_params.pcm_wd_sz,
> +			sparam->uc.pcm_params.brate,
> +			sparam->uc.pcm_params.frame_size,
> +			sparam->uc.pcm_params.samples_per_frame,
> +			sparam->uc.pcm_params.period_count);
> +		break;
> +
> +	case SST_CODEC_TYPE_MP3:
> +		sst_dbg("mp3 \n");
> +		sst_dbg("chan=%d, brate=%d, sfreq = %d, wd_sz = %d\n",
> +			sparam->uc.mp3_params.num_chan,
> +			sparam->uc.mp3_params.brate,
> +			sparam->uc.mp3_params.sfreq,
> +			sparam->uc.mp3_params.pcm_wd_sz);
> +		break;
> +
> +	case SST_CODEC_TYPE_AAC:
> +		sst_dbg("aac \n");
> +		sst_dbg("chan=%d, brate=%d, sfreq = %d, wd_sz = %d,asrate=%d\n",
> +			sparam->uc.aac_params.num_chan,
> +			sparam->uc.aac_params.brate,
> +			sparam->uc.aac_params.sfreq,
> +			sparam->uc.aac_params.pcm_wd_sz,
> +			sparam->uc.aac_params.aac_srate);
> +		sst_dbg("mpgid=%d profile=%d, aot = %d\n",
> +			sparam->uc.aac_params.mpg_id,
> +			sparam->uc.aac_params.aac_profile,
> +			sparam->uc.aac_params.aot);
> +		break;
> +	case SST_CODEC_TYPE_WMA9:
> +		sst_dbg("wma type \n");
> +		sst_dbg("chan=%d, brate=%d, sfreq = %d, wd_sz = %d, tag=%d\n",
> +			sparam->uc.wma_params.num_chan,
> +			sparam->uc.wma_params.brate,
> +			sparam->uc.wma_params.sfreq,
> +			sparam->uc.wma_params.pcm_wd_sz,
> +			sparam->uc.wma_params.format_tag);
> +		sst_dbg("mask=%d, b align=%d, enc opt =%d, op align =%d\n",
> +			sparam->uc.wma_params.channel_mask,
> +			sparam->uc.wma_params.block_align,
> +			sparam->uc.wma_params.wma_encode_opt,
> +			sparam->uc.wma_params.op_align);
> +		break;
> +	default:
> +		sst_dbg("other codec 0x%x\n", sparam->uc.pcm_params.codec);
> +	}
> +}
> +static inline int sst_validate_strid(int str_id)
> +{
> +	if (str_id <= 0 || str_id >= MAX_NUM_STREAMS) {
> +		sst_err("invalid stream id \n");
> +		return -EINVAL;
> +	} else
> +		return 0;
> +}
> +
> +/*NOTE: status will +ve for good cases and -ve for error ones*/
> +#define MAX_STREAM_FIELD 255
> +static inline void sst_wake_up_alloc_block(struct intel_sst_drv *sst_ops,
> +		u8 sst_id, int status, void *data)
> +{
> +	int i;
> +
> +	/*Unblock with retval code*/
> +	for (i = 0; i < MAX_STREAM_FIELD; i++) {
> +		if (sst_id == sst_ops->alloc_block[i].sst_id) {
> +			sst_ops->alloc_block[i].ops_block.condition = true;
> +			sst_ops->alloc_block[i].ops_block.ret_code = status;
> +			sst_ops->alloc_block[i].ops_block.data = data;
> +			sst_dbg("wake id %d, sst_id %d condition %x\n", i,
> +				sst_ops->alloc_block[i].sst_id,
> +				sst_ops->alloc_block[i].ops_block.condition);
> +			wake_up(&sst_ops->wait_queue);
> +			break;
> +		}
> +	}
> +}

Well, all these shouldn't be inline if you have no concrete reason
to inline them.  It's no C++ template.  Better to put as normal functions.


Takashi


More information about the Alsa-devel mailing list