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

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@intel.com Signed-off-by: Harsha Priya priya.harsha@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