All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/5] [RFC]intel_hdmi_audio:include header
@ 2010-11-22 13:39 ramesh.babu
  2010-11-22 13:39 ` [PATCH 2/5] [RFC]intel_hdmi_audio: interface module ramesh.babu
                   ` (5 more replies)
  0 siblings, 6 replies; 22+ messages in thread
From: ramesh.babu @ 2010-11-22 13:39 UTC (permalink / raw)
  To: alsa-devel; +Cc: Sailaja Bandarupalli, Ramesh Babu K V

From: Ramesh Babu K V <ramesh.babu@intel.com>

This patch creates intel_mid_hdmi_audio.h file. This file contains
Intel MID platform specific HDMI Audio register definitions, enums
and structure definitions used in this driver.  This also exposes
interface function.

Signed-off-by: Ramesh Babu K V <ramesh.babu@intel.com>
Signed-off-by: Sailaja Bandarupalli	<sailaja.bandarupalli@intel.com>
---
 .../drivers/intel_mid_hdmi/intel_mid_hdmi_audio.h  |  424 ++++++++++++++++++++
 1 files changed, 424 insertions(+), 0 deletions(-)
 create mode 100644 sound/drivers/intel_mid_hdmi/intel_mid_hdmi_audio.h

diff --git a/sound/drivers/intel_mid_hdmi/intel_mid_hdmi_audio.h b/sound/drivers/intel_mid_hdmi/intel_mid_hdmi_audio.h
new file mode 100644
index 0000000..19e47ee
--- /dev/null
+++ b/sound/drivers/intel_mid_hdmi/intel_mid_hdmi_audio.h
@@ -0,0 +1,424 @@
+/*
+ *   intel_mid_hdmi_audio.h - Intel HDMI audio driver for MID
+ *
+ *  Copyright (C) 2010 Intel Corp
+ *  Authors:	Sailaja Bandarupalli <sailaja.bandarupalli@intel.com>
+ *		Ramesh Babu K V	<ramesh.babu@intel.com>
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ * ALSA driver for Intel MID HDMI audio controller
+ */
+#ifndef __INTEL_MID_HDMI_AUDIO_H
+#define __INTEL_MID_HDMI_AUDIO_H
+
+#include <linux/types.h>
+#include <sound/initval.h>
+#include "../../../drivers/staging/mrst/drv/mdfld_hdmi_audio_if.h"
+#include "../../../drivers/staging/mrst/drv/psb_intel_hdmi.h"
+
+#define HAD_DRIVER_VERSION	"0.01.002"
+#define HAD_MAX_DEVICES		1
+#define HAD_MIN_CHANNEL		1
+#define HAD_MAX_CHANNEL		8
+#define HAD_NUM_OF_RING_BUFS	4
+#define HAD_MIN_RATE		32000
+#define HAD_MAX_RATE		192000
+#define HAD_MAX_BUFFER		(800*1024)
+#define HAD_MIN_BUFFER		(800*1024)
+#define HAD_MAX_PERIODS		4
+#define HAD_MIN_PERIODS		1
+#define HAD_MAX_PERIOD_BYTES	HAD_MAX_BUFFER
+#define HAD_MIN_PERIOD_BYTES	32
+#define HAD_FIFO_SIZE		0 /* fifo not being used */
+
+#define AUD_SAMPLE_RATE_32	HAD_MIN_RATE
+#define AUD_SAMPLE_RATE_44_1	44100
+#define AUD_SAMPLE_RATE_48	48000
+#define AUD_SAMPLE_RATE_88_2	88200
+#define AUD_SAMPLE_RATE_96	96000
+#define AUD_SAMPLE_RATE_176_4	176400
+#define AUD_SAMPLE_RATE_192	HAD_MAX_RATE
+
+#define DRIVER_NAME		"intelmid_hdmi_audio"
+#define DIS_SAMPLE_RATE_25_2	25200
+#define DIS_SAMPLE_RATE_27	27000
+#define DIS_SAMPLE_RATE_54	54000
+#define DIS_SAMPLE_RATE_74_25	74250
+#define DIS_SAMPLE_RATE_148_5	148500
+#define REG_BIT_0		0x1
+#define REG_BIT_1		0x2
+#define REG_BIT_2		0x4
+#define REG_BIT_3		0x8
+#define REG_BIT_20		0x100000
+#define SET_BIT0		1
+#define CLEAR_BIT0		0
+#define REG_WIDTH		0x08
+#define MAX_HW_BUFS		0x04
+#define MAX_DIP_WORDS		16
+#define MAX_PERIODS		4
+#define INTEL_HAD		"INTEL_HAD"
+
+/**
+ * enum had_status - Audio stream states
+ *
+ * @STREAM_INIT: Stream initialized
+ * @STREAM_RUNNING: Stream running
+ * @STREAM_PAUSED: Stream paused
+ * @STREAM_DROPPED: Stream dropped
+ * */
+enum had_stream_status {
+	STREAM_INIT = 0,
+	STREAM_RUNNING = 1,
+	STREAM_PAUSED = 2,
+	STREAM_DROPPED = 3
+};
+
+/**
+ * enum intel_had_aud_buf_type - HDMI controller ring buffer types
+ *
+ * @HAD_BUF_TYPE_A: ring buffer A
+ * @HAD_BUF_TYPE_B: ring buffer B
+ * @HAD_BUF_TYPE_C: ring buffer C
+ * @HAD_BUF_TYPE_D: ring buffer D
+ */
+enum intel_had_aud_buf_type {
+	HAD_BUF_TYPE_A = 0,
+	HAD_BUF_TYPE_B = 1,
+	HAD_BUF_TYPE_C = 2,
+	HAD_BUF_TYPE_D = 3,
+};
+
+enum num_aud_ch {
+	CH_STEREO = 0,
+	CH_THREE_FOUR = 1,
+	CH_FIVE_SIX = 2,
+	CH_SEVEN_EIGHT = 3
+};
+
+/*HDMI controller register offsets*/
+enum hdmi_ctrl_reg {
+	AUD_CONFIG		= 0x69000,
+	AUD_CH_STATUS_0		= 0x69008,
+	AUD_CH_STATUS_1		= 0x6900C,
+	AUD_HDMI_CTS		= 0x69010,
+	AUD_N_ENABLE		= 0x69014,
+	AUD_SAMPLE_RATE		= 0x69018,
+	AUD_BUF_CONFIG		= 0x69020,
+	AUD_BUF_CH_SWAP		= 0x69024,
+	AUD_BUF_A_ADDR		= 0x69040,
+	AUD_BUF_A_LENGTH	= 0x69044,
+	AUD_BUF_B_ADDR		= 0x69048,
+	AUD_BUF_B_LENGTH	= 0x6904c,
+	AUD_BUF_C_ADDR		= 0x69050,
+	AUD_BUF_C_LENGTH	= 0x69054,
+	AUD_BUF_D_ADDR		= 0x69058,
+	AUD_BUF_D_LENGTH	= 0x6905c,
+	AUD_CNTL_ST		= 0x69060,
+	AUD_HDMI_STATUS		= 0x69068,
+	AUD_HDMIW_INFOFR	= 0x69114,
+};
+/**
+ * union aud_cfg - Audio configuration offset - 69000
+ *
+ * @cfg_regx: individual register bits
+ * @cfg_regval: full register value
+ *
+ * */
+union aud_cfg {
+	struct {
+		u32 aud_en:1;
+		u32 layout:1;
+		u32 fmt:2;
+		u32 num_ch:2;
+		u32 rsvd0:1;
+		u32 set:1;
+		u32 flat:1;
+		u32 val_bit:1;
+		u32 user_bit:1;
+		u32 underrun:1;
+		u32 rsvd1:20;
+	} cfg_regx;
+	u32 cfg_regval;
+};
+/**
+ * union aud_ch_status_0 - Audio Channel Status 0 Attributes offset - 0x69008
+ *
+ * @status_0_regx:individual register bits
+ * @status_0_regval:full register value
+ *
+ * */
+union aud_ch_status_0 {
+	struct {
+		u32 ch_status:1;
+		u32 lpcm_id:1;
+		u32 cp_info:1;
+		u32 format:3;
+		u32 mode:2;
+		u32 ctg_code:8;
+		u32 src_num:4;
+		u32 ch_num:4;
+		u32 samp_freq:4;
+		u32 clk_acc:2;
+		u32 rsvd:2;
+	} status_0_regx;
+	u32 status_0_regval;
+};
+
+/**
+ * union aud_ch_status_1 - Audio Channel Status 1 Attributes offset - 0x6900c
+ *
+ * @status_1_regx: individual register bits
+ * @status_1_regval: full register value
+ *
+ **/
+union aud_ch_status_1 {
+	struct {
+		u32 max_wrd_len:1;
+		u32 wrd_len:3;
+		u32 rsvd:28;
+		} status_1_regx;
+	u32 status_1_regval;
+};
+
+/**
+ * union aud_hdmi_cts - CTS register offset -0x69010
+ *
+ * @cts_regx: individual register bits
+ * @cts_regval: full register value
+ *
+ * */
+union aud_hdmi_cts {
+	struct {
+		u32 cts_val:20;
+		u32 en_cts_prog:1;
+		u32 rsvd:11;
+	} cts_regx;
+	u32 cts_regval;
+};
+
+/**
+ * union aud_hdmi_n_enable - N register offset -0x69014
+ *
+ * @n_regx: individual register bits
+ * @n_regval: full register value
+ *
+*/
+union aud_hdmi_n_enable {
+	struct {
+		u32 n_val:20;
+		u32 en_n_prog:1;
+		u32 rsvd:11;
+	} n_regx;
+	u32 n_regval;
+};
+
+/**
+ * union aud_buf_config -  Audio Buffer configurations offset -0x69020
+ *
+ * @buf_cfg_regx: individual register bits
+ * @buf_cfgval: full register value
+ *
+*/
+union aud_buf_config {
+	struct {
+		u32 fifo_width:8;
+		u32 rsvd0:8;
+		u32 aud_delay:8;
+		u32 rsvd1:8;
+	} buf_cfg_regx;
+	u32 buf_cfgval;
+};
+
+/**
+ * union aud_buf_ch_swap - Audio Sample Swapping offset - 0x69024
+ *
+ * @buf_ch_swap_regx: individual register bits
+ * @buf_ch_swap_val: full register value
+ *
+ * */
+union aud_buf_ch_swap {
+	struct {
+		u32 first_0:3;
+		u32 second_0:3;
+		u32 first_1:3;
+		u32 second_1:3;
+		u32 first_2:3;
+		u32 second_2:3;
+		u32 first_3:3;
+		u32 second_3:3;
+		u32 rsvd:8;
+	} buf_ch_swap_regx;
+	u32 buf_ch_swap_val;
+};
+
+/**
+ * union aud_buf_addr - Address for Audio Buffer
+ *
+ * @buf_addr_regx: individual register bits
+ * @buf_addr_val: full register value
+ *
+ * */
+union aud_buf_addr {
+	struct {
+		u32 valid:1;
+		u32 intr_en:1;
+		u32 rsvd:4;
+		u32 addr:26;
+	} buf_addr_regx;
+	u32 buf_addr_val;
+};
+/**
+ * union aud_buf_len - Length of Audio Buffer
+ *
+ * @buf_len_regx: individual register bits
+ * @buf_len_val: full register value
+ *
+ * */
+union aud_buf_len {
+	struct {
+		u32 buf_len:20;
+		u32 rsvd:12;
+	} buf_len_regx;
+	u32 buf_len_val;
+};
+
+/**
+ * union aud_ctrl_st - Audio Control State Register offset 0x69060
+ *
+ * @ctrl_regx: individual register bits
+ * @ctrl_val: full register value
+ *
+ * */
+union aud_ctrl_st {
+	struct {
+		u32 ram_addr:4;
+		u32 eld_ack:1;
+		u32 eld_addr:4;
+		u32 eld_buf_size:5;
+		u32 eld_valid:1;
+		u32 cp_ready:1;
+		u32 dip_freq:2;
+		u32 dip_idx:3;
+		u32 dip_en_sta:4;
+		u32 rsvd:7;
+	} ctrl_regx;
+	u32 ctrl_val;
+};
+/**
+ * union aud_info_frame1 - Audio HDMI Widget Data Island Packet offset 0x69114
+ *
+ * @fr1_regx: individual register bits
+ * @fr1_val: full register value
+ *
+ * */
+union aud_info_frame1 {
+	struct {
+		u32 pkt_type:8;
+		u32 ver_num:8;
+		u32 len:5;
+		u32 rsvd:11;
+	} fr1_regx;
+	u32 fr1_val;
+};
+/**
+ * union aud_info_frame2 - DIP frame 2
+ *
+ * @fr2_regx: individual register bits
+ * @fr2_val: full register value
+ *
+ */
+union aud_info_frame2 {
+	struct {
+		u32 chksum:8;
+		u32 chnl_cnt:3;
+		u32 rsvd0:1;
+		u32 coding_type:4;
+		u32 smpl_size:2;
+		u32 smpl_freq:3;
+		u32 rsvd1:3;
+		u32 format:8;
+	} fr2_regx;
+	u32 fr2_val;
+};
+/**
+ * union aud_info_frame3 - DIP frame 3
+ *
+ * @fr3_regx: individual register bits
+ * @fr3_val: full register value
+ *
+ */
+union aud_info_frame3 {
+	struct {
+		u32 chnl_alloc:8;
+		u32 rsvd0:3;
+		u32 lsv:4;
+		u32 dm_inh:1;
+		u32 rsvd1:16;
+	} fr3_regx;
+	u32 fr3_val;
+};
+
+
+struct pcm_stream_info {
+	int str_id;
+	void *had_substream;
+	void (*period_elapsed) (void *had_substream);
+	u32 buffer_ptr;
+	u64 buffer_rendered;
+	u32 ring_buf_size;
+	int sfreq;
+};
+
+struct ring_buf_info {
+	uint32_t buf_addr;
+	uint32_t buf_size;
+	uint8_t	 is_valid;
+};
+
+struct had_stream_pvt {
+	enum had_stream_status	stream_status;
+	int	stream_ops;
+	struct snd_pcm_substream *substream;
+	ssize_t	dbg_cum_bytes;
+};
+
+struct had_callback_ops {
+	had_event_call_back intel_had_event_call_back;
+};
+
+/*HAD Driver context*/
+struct snd_intelhad {
+	struct snd_card	*card; /*holds details of snd_card*/
+	int		card_index; /*card index in alsa*/
+	char		*card_id; /*name of card*/
+	/*holds regsiters read/write callbacks*/
+	struct hdmi_audio_registers_ops *reg_ops;
+	/*holds caps get/set callbacks*/
+	struct hdmi_audio_query_set_ops *query_ops;
+	int		playback_cnt;
+	struct ring_buf_info buf_info[HAD_NUM_OF_RING_BUFS];
+	struct pcm_stream_info stream_info;
+	hdmi_eeld_t	eeld;
+	enum intel_had_aud_buf_type curr_buf;
+	int	valid_buf_cnt;
+};
+int had_event_handler(enum had_event_type event_type, void *data);
+extern struct snd_intelhad *intelhaddata;
+extern struct snd_pcm_ops snd_intelhad_playback_ops;
+extern int hdmi_card_index;
+extern char *hdmi_card_id;
+#endif
-- 
1.6.2.5

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 2/5] [RFC]intel_hdmi_audio: interface module
  2010-11-22 13:39 [PATCH 1/5] [RFC]intel_hdmi_audio:include header ramesh.babu
@ 2010-11-22 13:39 ` ramesh.babu
  2010-11-22 13:58   ` Mark Brown
  2010-11-22 13:39 ` [PATCH 3/5] [RFC]intel_hdmi_audio: driver module - ALSA intereaction ramesh.babu
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 22+ messages in thread
From: ramesh.babu @ 2010-11-22 13:39 UTC (permalink / raw)
  To: alsa-devel; +Cc: Sailaja Bandarupalli, Ramesh Babu K V

From: Ramesh Babu K V <ramesh.babu@intel.com>

This patch creates intel_mid_hdmi_audio_if.c file.  This
interface module of the driver communicates with client driver
(HDMI display module) for receiving hot plug/unplug and other
events such as Buffer complete interrupt.  ALSA sound card
will be created dynamically when hot plug event received and
removed when unplug event is received.

Signed-off-by: Ramesh Babu K V <ramesh.babu@intel.com>
Signed-off-by: Sailaja Bandarupalli <sailaja.bandarupalli@intel.com>
---
 .../intel_mid_hdmi/intel_mid_hdmi_audio_if.c       |  224 ++++++++++++++++++++
 1 files changed, 224 insertions(+), 0 deletions(-)
 create mode 100644 sound/drivers/intel_mid_hdmi/intel_mid_hdmi_audio_if.c

diff --git a/sound/drivers/intel_mid_hdmi/intel_mid_hdmi_audio_if.c b/sound/drivers/intel_mid_hdmi/intel_mid_hdmi_audio_if.c
new file mode 100644
index 0000000..aac9335
--- /dev/null
+++ b/sound/drivers/intel_mid_hdmi/intel_mid_hdmi_audio_if.c
@@ -0,0 +1,224 @@
+/*
+ *   intel_mid_hdmi_audio_if.h - Intel HDMI audio driver for MID
+ *
+ *  Copyright (C) 2010 Intel Corp
+ *  Authors:	Sailaja Bandarupalli <sailaja.bandarupalli@intel.com>
+ *		Ramesh Babu K V <ramesh.babu@intel.com>
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ * ALSA driver for Intel MID HDMI audio controller.  This file contains
+ * interface functions exposed to HDMI Display driver and code to register
+ * with ALSA framework..
+ */
+
+#include <sound/initval.h>
+#include <sound/pcm.h>
+#include <sound/core.h>
+#include "intel_mid_hdmi_audio.h"
+#include "../../../drivers/staging/mrst/drv/psb_intel_hdmi.h"
+
+#define PCM_INDEX		0
+#define MAX_PB_STREAMS		1
+#define MAX_CAP_STREAMS		0
+
+/**
+ * snd_intelhad_dev_free - to free alsa card instance
+ *
+ * @device: pointer to device
+ *
+ * This function is called when the hdmi cable is un plugged
+ */
+static int snd_intelhad_dev_free(struct snd_device *device)
+{
+	struct snd_intelhad *intelhaddata;
+
+	BUG_ON(!device);
+	pr_debug("had: snd_intelhad_dev_free called\n");
+	intelhaddata = device->device_data;
+	return 0;
+}
+
+/**
+ * snd_intelhad_create - to crete alsa card instance
+ *
+ * @intelhaddata: pointer to internal context
+ * @card: pointer to card
+ *
+ * This function is called when the hdmi cable is plugged in
+ */
+static int __devinit snd_intelhad_create(
+		struct snd_intelhad *intelhaddata,
+		struct snd_card *card)
+{
+	int retval;
+	static struct snd_device_ops ops = {
+		.dev_free =	snd_intelhad_dev_free,
+	};
+
+	BUG_ON(!intelhaddata);
+	BUG_ON(!card);
+	/* ALSA api to register the device */
+	retval = snd_device_new(card, SNDRV_DEV_LOWLEVEL, intelhaddata, &ops);
+	return retval;
+}
+/**
+ * snd_intelhad_pcm_free - to free the memory allocated
+ *
+ * @pcm: pointer to pcm instance
+ * This function is called when the device is removed
+ */
+static void snd_intelhad_pcm_free(struct snd_pcm *pcm)
+{
+	pr_debug("had:Freeing PCM preallocated pages\n");
+	snd_pcm_lib_preallocate_free_for_all(pcm);
+}
+
+/**
+ * had_hot_plug - to create sound card instance for HDMI audio playabck
+ *
+ *
+ * This function is called when the hdmi cable is plugged in. This function
+ * creates and registers the sound card with ALSA
+ */
+static int had_hot_plug(void)
+{
+	int retval;
+	struct snd_pcm *pcm;
+	struct snd_card *card;
+
+	pr_debug("had: Hot plug event received\n");
+	/* create a card instance with ALSA framework */
+	retval = snd_card_create(hdmi_card_index, hdmi_card_id,
+				THIS_MODULE, 0, &card);
+	if (retval) {
+		pr_err("had:Error while creating snd card\n");
+		return retval;
+	}
+
+	intelhaddata->card = card;
+	intelhaddata->card_id = hdmi_card_id;
+	intelhaddata->card_index = card->number;
+	intelhaddata->playback_cnt = 0;
+	strncpy(card->driver, INTEL_HAD, strlen(INTEL_HAD));
+	strncpy(card->shortname, INTEL_HAD, strlen(INTEL_HAD));
+
+	retval = snd_pcm_new(card, INTEL_HAD, PCM_INDEX, MAX_PB_STREAMS,
+						MAX_CAP_STREAMS, &pcm);
+	if (retval) {
+		pr_err("had:_pcm_new failed\n");
+		goto err;
+	}
+	/* setup private data which can be retrieved when required */
+	pcm->private_data = intelhaddata;
+	pcm->private_free = snd_intelhad_pcm_free;
+	pcm->info_flags = 0;
+	strncpy(pcm->name, card->shortname, strlen(card->shortname));
+	/* setup the ops for palyabck */
+	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK,
+			    &snd_intelhad_playback_ops);
+	/* allocate dma pages for ALSA stream operations */
+	retval = snd_pcm_lib_preallocate_pages_for_all(pcm,
+			SNDRV_DMA_TYPE_CONTINUOUS,
+			snd_dma_continuous_data(GFP_KERNEL),
+			HAD_MIN_BUFFER, HAD_MAX_BUFFER);
+	if (retval) {
+		pr_err("had:preallocation failed\n");
+		goto err;
+	}
+	/* internal function call to register device with ALSA */
+	retval = snd_intelhad_create(intelhaddata, card);
+	if (retval)
+		goto err;
+	card->private_data = &intelhaddata;
+	retval = snd_card_register(card);
+	if (retval) {
+		pr_err("had: snd_card_register failed\n");
+		goto err;
+	}
+	intelhaddata->query_ops->hdmi_audio_get_caps(
+				HAD_GET_ELD, &intelhaddata->eeld);
+	retval = intelhaddata->reg_ops->hdmi_audio_write_register(
+						AUD_HDMI_STATUS, SET_BIT0);
+	return retval;
+err:
+	pr_err("had: Error returned from snd api\n");
+	snd_card_free(card);
+	return retval;
+}
+
+/**
+ * had_event_handler - Call back function to handle events
+ *
+ * @event_type: Event type to handle
+ * @data: data related to the event_type
+ *
+ * This function is invoked to handle HDMI events from client driver.
+ */
+int had_event_handler(enum had_event_type event_type, void *data)
+{
+	int retval = 0;
+	enum intel_had_aud_buf_type buf_id;
+	struct pcm_stream_info *stream;
+	u32 buf_size;
+
+	pr_debug("had:called _event_handler with event#%d\n", event_type);
+	switch (event_type) {
+	case HAD_EVENT_HOT_PLUG:
+		retval = had_hot_plug();
+		break;
+	case HAD_EVENT_HOT_UNPLUG:
+		retval = snd_card_free(intelhaddata->card);
+		break;
+	case HAD_EVENT_PM_CHANGING:
+		/*Run-time PM is not yet supported*/
+		retval = -EIO;
+		break;
+	case HAD_EVENT_AUDIO_BUFFER_DONE:
+		stream = &intelhaddata->stream_info;
+
+
+		buf_id = intelhaddata->curr_buf;
+		pr_debug("had:called _event_handler with BUFFER DONE event for\
+							 Buf#%d\n", buf_id);
+		buf_size =  intelhaddata->buf_info[buf_id].buf_size;
+		intelhaddata->stream_info.buffer_rendered += buf_size;
+		intelhaddata->buf_info[buf_id].is_valid = true;
+		stream->period_elapsed(stream->had_substream);
+		if (intelhaddata->valid_buf_cnt-1 == buf_id)
+			intelhaddata->curr_buf = HAD_BUF_TYPE_A;
+		else
+			intelhaddata->curr_buf++;
+		/*Reprogram the registers with addr and length*/
+		retval = intelhaddata->reg_ops->hdmi_audio_write_register(
+				AUD_BUF_A_LENGTH + (buf_id * REG_WIDTH),
+				buf_size);
+		retval = intelhaddata->reg_ops->hdmi_audio_write_register(
+				AUD_BUF_A_ADDR+(buf_id * REG_WIDTH),
+				intelhaddata->buf_info[buf_id].buf_addr|
+				REG_BIT_0 | REG_BIT_1);
+		break;
+	case HAD_EVENT_AUDIO_BUFFER_UNDERRUN:
+		/*To be handle  error handling*/
+		retval = -EIO;
+		break;
+	default:
+		pr_debug("had:error un-handled event !!\n");
+		retval = -EINVAL;
+		break;
+	}
+	return retval;
+}
-- 
1.6.2.5

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 3/5] [RFC]intel_hdmi_audio: driver module - ALSA intereaction
  2010-11-22 13:39 [PATCH 1/5] [RFC]intel_hdmi_audio:include header ramesh.babu
  2010-11-22 13:39 ` [PATCH 2/5] [RFC]intel_hdmi_audio: interface module ramesh.babu
@ 2010-11-22 13:39 ` ramesh.babu
  2010-11-24  9:16   ` Takashi Iwai
  2010-11-22 13:39 ` [PATCH 4/5] [RFC]intel_hdmi_audio:driver module-hdmi hw interface ramesh.babu
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 22+ messages in thread
From: ramesh.babu @ 2010-11-22 13:39 UTC (permalink / raw)
  To: alsa-devel; +Cc: Sailaja Bandarupalli, Ramesh Babu K V

From: Ramesh Babu K V <ramesh.babu@intel.com>

This patch creates intel_mid_hdmi_audio.c file.  It intereacts
with ALSA framework to enable the audio playback through HDMI
interface.  This code uses standard ALSA APIs.

Signed-off-by: Ramesh Babu K V <ramesh.babu@intel.com>
Signed-off-by: Sailaja Bandarupalli <sailaja.bandarupalli@intel.com>
---
 .../drivers/intel_mid_hdmi/intel_mid_hdmi_audio.c  |  546 ++++++++++++++++++++
 1 files changed, 546 insertions(+), 0 deletions(-)
 create mode 100644 sound/drivers/intel_mid_hdmi/intel_mid_hdmi_audio.c

diff --git a/sound/drivers/intel_mid_hdmi/intel_mid_hdmi_audio.c b/sound/drivers/intel_mid_hdmi/intel_mid_hdmi_audio.c
new file mode 100644
index 0000000..3ef4d48
--- /dev/null
+++ b/sound/drivers/intel_mid_hdmi/intel_mid_hdmi_audio.c
@@ -0,0 +1,546 @@
+/*
+ *   intel_mid_hdmi_audio.c - Intel HDMI audio driver for MID
+ *
+ *  Copyright (C) 2010 Intel Corp
+ *  Authors:	Sailaja Bandarupalli <sailaja.bandarupalli@intel.com>
+ *		Ramesh Babu K V	<ramesh.babu@intel.com>
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ * ALSA driver for Intel MID HDMI audio controller
+ */
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/initval.h>
+#include <sound/control.h>
+#include <sound/initval.h>
+#include "intel_mid_hdmi_audio.h"
+
+MODULE_AUTHOR("Sailaja Bandarupalli <sailaja.bandarupalli@intel.com>");
+MODULE_AUTHOR("Ramesh Babu K V <ramesh.babu@intel.com>");
+MODULE_DESCRIPTION("Intel HDMI Audio driver");
+MODULE_LICENSE("GPL v2");
+MODULE_SUPPORTED_DEVICE("{Intel,Intel_HAD}");
+
+#define INFO_FRAME_WORD1	0x000a0184
+#define FIFO_THRESHOLD		0xFE
+#define BYTES_PER_WORD		0x4
+#define CH_STATUS_MAP_32KHZ	0x3
+#define CH_STATUS_MAP_44KHZ	0x0
+#define CH_STATUS_MAP_48KHZ	0x2
+#define MAX_SMPL_WIDTH_20	0x0
+#define MAX_SMPL_WIDTH_24	0x1
+#define SMPL_WIDTH_16BITS	0x1
+#define SMPL_WIDTH_24BITS	0x5
+#define CHANNEL_ALLOCATION	0x1F
+#define SET_BYTE0		0x000000FF
+#define VALID_DIP_WORDS		3
+#define LAYOUT0			0
+#define LAYOUT1			1
+
+/*standard module options for ALSA. This module supports only one card*/
+int hdmi_card_index = SNDRV_DEFAULT_IDX1;
+char *hdmi_card_id = SNDRV_DEFAULT_STR1;
+
+module_param(hdmi_card_index, int, 0444);
+MODULE_PARM_DESC(hdmi_card_index,
+		"Index value for INTEL Intel HDMI Audio controller.");
+module_param(hdmi_card_id, charp, 0444);
+MODULE_PARM_DESC(hdmi_card_id,
+		"ID string for INTEL Intel HDMI Audio controller.");
+
+/* hardware capability structure */
+static const struct snd_pcm_hardware snd_intel_hadstream = {
+	.info =	(SNDRV_PCM_INFO_INTERLEAVED |
+		SNDRV_PCM_INFO_DOUBLE |
+		SNDRV_PCM_INFO_PAUSE |
+		SNDRV_PCM_INFO_RESUME |
+		SNDRV_PCM_INFO_MMAP|
+		SNDRV_PCM_INFO_MMAP_VALID |
+		SNDRV_PCM_INFO_BATCH |
+		SNDRV_PCM_INFO_SYNC_START),
+	.formats = (SNDRV_PCM_FMTBIT_S24 |
+		SNDRV_PCM_FMTBIT_U24),
+	.rates = SNDRV_PCM_RATE_32000 |
+		SNDRV_PCM_RATE_44100 |
+		SNDRV_PCM_RATE_48000 |
+		SNDRV_PCM_RATE_64000 |
+		SNDRV_PCM_RATE_88200 |
+		SNDRV_PCM_RATE_96000 |
+		SNDRV_PCM_RATE_176400 |
+		SNDRV_PCM_RATE_192000,
+	.rate_min = HAD_MIN_RATE,
+	.rate_max = HAD_MAX_RATE,
+	.channels_min = HAD_MIN_CHANNEL,
+	.channels_max = HAD_MAX_CHANNEL,
+	.buffer_bytes_max = HAD_MAX_PERIOD_BYTES,
+	.period_bytes_min = HAD_MIN_PERIOD_BYTES,
+	.period_bytes_max = HAD_MAX_BUFFER,
+	.periods_min = HAD_MIN_PERIODS,
+	.periods_max = HAD_MAX_PERIODS,
+	.fifo_size = HAD_FIFO_SIZE,
+};
+
+struct snd_intelhad *intelhaddata;
+
+/**
+* snd_intelhad_open - stream initializations are done here
+* @substream:substream for which the stream function is called
+*
+* This function is called whenever a PCM stream is opened
+*/
+static int snd_intelhad_open(struct snd_pcm_substream *substream)
+{
+	struct snd_intelhad *intelhaddata;
+	struct snd_pcm_runtime *runtime;
+	struct had_stream_pvt *stream;
+	int retval;
+
+	BUG_ON(!substream);
+
+	pr_debug("had: snd_intelhad_open called\n");
+
+	intelhaddata = snd_pcm_substream_chip(substream);
+	runtime = substream->runtime;
+	/* set the runtime hw parameter with local snd_pcm_hardware struct */
+	runtime->hw = snd_intel_hadstream;
+
+	stream = kzalloc(sizeof(*stream), GFP_KERNEL);
+	if (!stream)
+		return -ENOMEM;
+	stream->stream_status = STREAM_INIT;
+	runtime->private_data = stream;
+	intelhaddata->reg_ops->hdmi_audio_write_register(AUD_HDMI_STATUS, 0);
+	retval = snd_pcm_hw_constraint_integer(runtime,
+			 SNDRV_PCM_HW_PARAM_PERIODS);
+	return retval;
+}
+
+/**
+* had_period_elapsed - updates the hardware pointer status
+* @had_substream:substream for which the stream function is called
+*
+*/
+static void had_period_elapsed(void *had_substream)
+{
+	struct snd_pcm_substream *substream = had_substream;
+	struct had_stream_pvt *stream;
+
+	pr_debug("had: calling period elapsed\n");
+	if (!substream || !substream->runtime)
+		return;
+	stream = substream->runtime->private_data;
+	if (!stream)
+		return;
+
+	if (stream->stream_status != STREAM_RUNNING)
+		return;
+	snd_pcm_period_elapsed(substream);
+	return;
+}
+
+/**
+* snd_intelhad_init_stream - internal function to initialize stream info
+* @substream:substream for which the stream function is called
+*
+*/
+static int snd_intelhad_init_stream(struct snd_pcm_substream *substream)
+{
+	struct snd_intelhad *intelhaddata = snd_pcm_substream_chip(substream);
+
+	pr_debug("had: setting buffer ptr param\n");
+	intelhaddata->stream_info.period_elapsed = had_period_elapsed;
+	intelhaddata->stream_info.had_substream = substream;
+	intelhaddata->stream_info.buffer_ptr = 0;
+	intelhaddata->stream_info.buffer_rendered = 0;
+	intelhaddata->stream_info.sfreq = substream->runtime->rate;
+	return 0;
+}
+
+/**
+ * snd_intelhad_close- to free parameteres when stream is stopped
+ *
+ * @substream:  substream for which the function is called
+ *
+ * This function is called by ALSA framework when stream is stopped
+ */
+static int snd_intelhad_close(struct snd_pcm_substream *substream)
+{
+	struct snd_intelhad *intelhaddata;
+	struct had_stream_pvt *stream;
+	BUG_ON(!substream);
+
+	stream = substream->runtime->private_data;
+
+	pr_debug("had: snd_intelhad_close called\n");
+	intelhaddata = snd_pcm_substream_chip(substream);
+	if (intelhaddata->stream_info.str_id) {
+		intelhaddata->playback_cnt--;
+		intelhaddata->stream_info.str_id = 0;
+	}
+	intelhaddata->stream_info.buffer_rendered = 0;
+	intelhaddata->stream_info.buffer_ptr = 0;
+
+	kfree(substream->runtime->private_data);
+	return 0;
+}
+
+/**
+ * snd_intelhad_hw_params- to setup the hardware parameters
+ * like allocating the buffers
+ *
+ * @substream:  substream for which the function is called
+ * @hw_params: hardware parameters
+ *
+ * This function is called by ALSA framework when hardware params are set
+ */
+static int snd_intelhad_hw_params(struct snd_pcm_substream *substream,
+				    struct snd_pcm_hw_params *hw_params)
+{
+	int retval;
+	struct snd_pcm_runtime *runtime;
+
+	BUG_ON(!substream);
+	BUG_ON(!hw_params);
+	pr_debug("had: snd_intelhad_hw_params called\n");
+
+	runtime = substream->runtime;
+
+	retval = snd_pcm_lib_malloc_pages(substream,
+					params_buffer_bytes(hw_params));
+	if (retval < 0)
+		return -ENOMEM;
+	pr_debug("had: allocated memory = %d\n",
+					params_buffer_bytes(hw_params));
+	memset(substream->runtime->dma_area, 0,
+			params_buffer_bytes(hw_params));
+
+	pr_debug("had: snd_intelhad_hw_params exited\n");
+	return retval;
+}
+
+/**
+ * snd_intelhad_hw_free- to release the resources allocated during
+ * hardware params setup
+ *
+ * @substream:  substream for which the function is called
+ *
+ * This function is called by ALSA framework before close callback.
+ *
+ */
+static int snd_intelhad_hw_free(struct snd_pcm_substream *substream)
+{
+	BUG_ON(!substream);
+	pr_debug("had: snd_intelhad_hw_free called\n");
+	return snd_pcm_lib_free_pages(substream);
+}
+
+/**
+* snd_intelhad_pcm_trigger - stream activities are handled here
+* @substream:substream for which the stream function is called
+* @cmd:the stream commamd thats requested from upper layer
+* This function is called whenever an a stream activity is invoked
+*/
+static int snd_intelhad_pcm_trigger(struct snd_pcm_substream *substream,
+					int cmd)
+{
+	int i, caps, retval = 0;
+	u32 regval = 0;
+	struct snd_intelhad *intelhaddata;
+	struct had_stream_pvt *stream;
+	struct hdmi_audio_registers_ops *reg_ops;
+	struct hdmi_audio_query_set_ops *query_ops;
+
+	BUG_ON(!substream);
+
+	intelhaddata = snd_pcm_substream_chip(substream);
+	stream = substream->runtime->private_data;
+	reg_ops = intelhaddata->reg_ops;
+	query_ops = intelhaddata->query_ops;
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+		pr_debug("had: Trigger Start\n");
+		caps = HDMI_AUDIO_UNDERRUN | HDMI_AUDIO_BUFFER_DONE;
+		retval = query_ops->hdmi_audio_set_caps(
+					HAD_SET_ENABLE_AUDIO_INT, &caps);
+		if (retval)
+			return retval;
+		retval = query_ops->hdmi_audio_set_caps(
+					HAD_SET_ENABLE_AUDIO, NULL);
+		if (retval)
+			return retval;
+
+		retval = reg_ops->hdmi_audio_read_modify(AUD_CONFIG,
+					SET_BIT0, REG_BIT_0);
+		stream->substream = substream;
+		stream->stream_status = STREAM_RUNNING;
+		break;
+
+	case SNDRV_PCM_TRIGGER_STOP:
+		pr_debug("had: Trigger Stop\n");
+		caps = HDMI_AUDIO_UNDERRUN | HDMI_AUDIO_BUFFER_DONE;
+		retval = query_ops->hdmi_audio_set_caps(
+					HAD_SET_DISABLE_AUDIO_INT, &caps);
+		if (retval)
+			return retval;
+		retval = query_ops->hdmi_audio_set_caps(
+					HAD_SET_DISABLE_AUDIO, NULL);
+		if (retval)
+			return retval;
+		stream->stream_status = STREAM_DROPPED;
+		break;
+
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		pr_debug("had: Trigger Pause\n");
+		/* disable the validity bits */
+		for (i = 0; i < MAX_HW_BUFS; i++) {
+			retval = reg_ops->hdmi_audio_read_register(
+					AUD_BUF_A_ADDR+(i*REG_WIDTH), &regval);
+			if (retval)
+				return retval;
+			if (regval & REG_BIT_0) {
+				retval = reg_ops->hdmi_audio_read_modify(
+						AUD_BUF_A_ADDR+(i * REG_WIDTH),
+						0, REG_BIT_0);
+				if (retval)
+					return retval;
+				intelhaddata->buf_info[HAD_BUF_TYPE_A + i].
+						is_valid = true;
+			}
+		}
+		stream->stream_status = STREAM_PAUSED;
+		break;
+
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		pr_debug("had: Trigger Resume\n");
+		/* enable the validity bits */
+		for (i = 0; i < MAX_HW_BUFS; i++) {
+			if (intelhaddata->buf_info[HAD_BUF_TYPE_A + i].is_valid)
+				retval = reg_ops->hdmi_audio_read_modify(
+					AUD_BUF_A_ADDR + (i * REG_WIDTH),
+					SET_BIT0, REG_BIT_0);
+		}
+		stream->stream_status = STREAM_RUNNING;
+		break;
+
+	default:
+		retval = -EINVAL;
+	}
+	return retval;
+}
+
+/**
+* snd_intelhad_pcm_prepare- internal preparation before starting a stream
+*
+* @substream:  substream for which the function is called
+*
+* This function is called when a stream is started for internal preparation.
+*/
+static int snd_intelhad_pcm_prepare(struct snd_pcm_substream *substream)
+{
+	struct had_stream_pvt *stream;
+	int retval;
+	u32 disp_samp_freq, n_param;
+	struct snd_intelhad *intelhaddata;
+	struct snd_pcm_runtime *runtime;
+
+	pr_debug("had: pcm_prepare called\n");
+
+	BUG_ON(!substream);
+	runtime = substream->runtime;
+	pr_debug("had:period_size=%d\n",
+				frames_to_bytes(runtime, runtime->period_size));
+	pr_debug("had:periods=%d\n", runtime->periods);
+	pr_debug("had:buffer_size=%d\n", snd_pcm_lib_buffer_bytes(substream));
+	pr_debug("had:rate=%d\n", runtime->rate);
+	pr_debug("had:channels=%d\n", runtime->channels);
+
+	stream = substream->runtime->private_data;
+	intelhaddata = snd_pcm_substream_chip(substream);
+	if (intelhaddata->stream_info.str_id) {
+		pr_debug("had:_prepare is called for existing str_id#%d\n",
+					intelhaddata->stream_info.str_id);
+		retval = snd_intelhad_pcm_trigger(substream,
+						SNDRV_PCM_TRIGGER_STOP);
+		return retval;
+	}
+
+	intelhaddata->playback_cnt++;
+	intelhaddata->stream_info.str_id = intelhaddata->playback_cnt;
+	snprintf(substream->pcm->id, sizeof(substream->pcm->id),
+			"%d", intelhaddata->stream_info.str_id);
+	retval = snd_intelhad_init_stream(substream);
+	if (retval)
+		goto prep_end;
+	/* Get N value in KHz */
+	retval = intelhaddata->query_ops->hdmi_audio_get_caps(
+				HAD_GET_SAMPLING_FREQ, &disp_samp_freq);
+	if (retval) {
+		pr_debug("had: querying display sampling freq failed\n");
+		goto prep_end;
+	} else
+		pr_debug("had: TMDS freq = %d kHz\n", disp_samp_freq);
+
+	retval = snd_intelhad_prog_n(substream->runtime->rate, &n_param);
+	if (retval) {
+		pr_debug("had: programming N value failed\n");
+		goto prep_end;
+	}
+	retval = snd_intelhad_prog_cts(
+			substream->runtime->rate/1000, disp_samp_freq, n_param);
+	if (retval) {
+		pr_debug("had: programming CTS value failed\n");
+		goto prep_end;
+	}
+
+	retval = snd_intelhad_prog_DIP(substream);
+	if (retval) {
+		pr_debug("had: programming DIP values failed\n");
+		goto prep_end;
+	}
+	retval = snd_intelhad_init_audio_ctrl(substream);
+	if (retval) {
+		pr_debug("had: initializing audio controller regs failed\n");
+		goto prep_end;
+	}
+	retval = snd_intelhad_prog_ring_buf(substream) ;
+	if (retval)
+		pr_debug("had: initializing ring buffer regs failed\n");
+
+prep_end:
+	return retval;
+}
+
+/**
+ * snd_intelhad_pcm_pointer- to send the current buffer pointer processed by hw
+ *
+ * @substream:  substream for which the function is called
+ *
+ * This function is called by ALSA framework to get the current hw buffer ptr
+ * when a period is elapsed
+ */
+static snd_pcm_uframes_t snd_intelhad_pcm_pointer(
+					struct snd_pcm_substream *substream)
+{
+	struct had_stream_pvt *stream;
+	struct snd_intelhad *intelhaddata;
+	u32 bytes_rendered;
+
+	pr_debug("had: Called snd_intelhad_pcm_pointer\n");
+	BUG_ON(!substream);
+
+	intelhaddata = snd_pcm_substream_chip(substream);
+	stream = substream->runtime->private_data;
+
+	div_u64_rem(intelhaddata->stream_info.buffer_rendered,
+			intelhaddata->stream_info.ring_buf_size,
+			&(bytes_rendered));
+	intelhaddata->stream_info.buffer_ptr = bytes_to_frames(
+						substream->runtime,
+						bytes_rendered);
+	pr_debug("had:pcm_pointer = %#x\n",
+			intelhaddata->stream_info.buffer_ptr);
+	return intelhaddata->stream_info.buffer_ptr;
+}
+
+/*PCM operations structure and the calls back for the same */
+struct snd_pcm_ops snd_intelhad_playback_ops = {
+	.open =	snd_intelhad_open,
+	.close = snd_intelhad_close,
+	.ioctl = snd_pcm_lib_ioctl,
+	.hw_params = snd_intelhad_hw_params,
+	.hw_free = snd_intelhad_hw_free,
+	.prepare = snd_intelhad_pcm_prepare,
+	.trigger = snd_intelhad_pcm_trigger,
+	.pointer = snd_intelhad_pcm_pointer,
+};
+
+/*
+* alsa_card_intelhad_init- driver init function
+* This function is called when driver module is inserted
+*/
+static int __init alsa_card_intelhad_init(void)
+{
+	int retval;
+	struct had_callback_ops ops_cb;
+
+	pr_debug("had: init called\n");
+	pr_info(KERN_INFO "INFO: ******** HAD DRIVER loading.. Ver: %s\n",
+					HAD_DRIVER_VERSION);
+
+	/* allocate memory for saving internal context and working */
+	intelhaddata = kzalloc(sizeof(*intelhaddata), GFP_KERNEL);
+	if (!intelhaddata) {
+		pr_debug("had: mem alloc failed\n");
+		return -ENOMEM;
+	}
+	/* allocate memory for display driver api set */
+	intelhaddata->reg_ops = kzalloc(
+				sizeof(struct hdmi_audio_registers_ops),
+				GFP_KERNEL);
+	if (!intelhaddata->reg_ops) {
+		pr_debug("had: mem alloc failed\n");
+		retval = -ENOMEM;
+		goto free_context;
+	}
+	intelhaddata->query_ops = kzalloc(
+				sizeof(struct hdmi_audio_query_set_ops),
+				GFP_KERNEL);
+	if (!intelhaddata->query_ops) {
+		pr_debug("had: mem alloc failed\n");
+		retval = -ENOMEM;
+		goto free_regops;
+	}
+	ops_cb.intel_had_event_call_back = had_event_handler;
+	/* registering with display driver to get access to display APIs */
+	retval = intel_hdmi_audio_query_capabilities(
+			ops_cb.intel_had_event_call_back,
+			&intelhaddata->reg_ops,
+			&intelhaddata->query_ops);
+	if (retval) {
+		pr_debug("had: registering with display driver failed\n");
+		goto free_allocs;
+	}
+	pr_debug("had:...init complete\n");
+	return retval;
+free_allocs:
+	kfree(intelhaddata->query_ops);
+free_regops:
+	kfree(intelhaddata->reg_ops);
+free_context:
+	kfree(intelhaddata);
+	pr_err("had: driver init failed\n");
+	return retval;
+}
+
+/**
+* alsa_card_intelhad_exit- driver exit function
+* This function is called when driver module is removed
+*/
+static void __exit alsa_card_intelhad_exit(void)
+{
+	pr_debug("had:had_exit called\n");
+	if (intelhaddata) {
+		kfree(intelhaddata->query_ops);
+		kfree(intelhaddata->reg_ops);
+		kfree(intelhaddata);
+	}
+}
+late_initcall(alsa_card_intelhad_init);
+module_exit(alsa_card_intelhad_exit);
-- 
1.6.2.5

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 4/5] [RFC]intel_hdmi_audio:driver module-hdmi hw interface
  2010-11-22 13:39 [PATCH 1/5] [RFC]intel_hdmi_audio:include header ramesh.babu
  2010-11-22 13:39 ` [PATCH 2/5] [RFC]intel_hdmi_audio: interface module ramesh.babu
  2010-11-22 13:39 ` [PATCH 3/5] [RFC]intel_hdmi_audio: driver module - ALSA intereaction ramesh.babu
@ 2010-11-22 13:39 ` ramesh.babu
  2010-11-22 13:39 ` [PATCH 5/5] [RFC]intel_hdmi_audio: Kconfig and Makefile ramesh.babu
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 22+ messages in thread
From: ramesh.babu @ 2010-11-22 13:39 UTC (permalink / raw)
  To: alsa-devel; +Cc: Sailaja Bandarupalli, Ramesh Babu K V

From: Ramesh Babu K V <ramesh.babu@intel.com>

This patch programs hdmi audio hw registers whenever
ALSA _prepare function is invoked.  It calculates N and CTS
values according HDMI 1.3a spec and programs it.
It also programs hw buffer pointer and length registers.

Signed-off-by: Ramesh Babu K V <ramesh.babu@intel.com>
Signed-off-by: Sailaja Bandarupalli <sailaja.bandarupalli@intel.com>
---
 .../drivers/intel_mid_hdmi/intel_mid_hdmi_audio.c  |  281 ++++++++++++++++++++
 1 files changed, 281 insertions(+), 0 deletions(-)

diff --git a/sound/drivers/intel_mid_hdmi/intel_mid_hdmi_audio.c b/sound/drivers/intel_mid_hdmi/intel_mid_hdmi_audio.c
index 3ef4d48..6701a88 100644
--- a/sound/drivers/intel_mid_hdmi/intel_mid_hdmi_audio.c
+++ b/sound/drivers/intel_mid_hdmi/intel_mid_hdmi_audio.c
@@ -99,6 +99,287 @@ static const struct snd_pcm_hardware snd_intel_hadstream = {
 struct snd_intelhad *intelhaddata;
 
 /**
+ * snd_intelhad_init_audio_ctrl - to initialize audio channel status
+ * registers and confgiuration registers
+ *
+ * @substream:substream for which the prepare function is called
+ *
+ * This function is called in the prepare callback
+ */
+static int snd_intelhad_init_audio_ctrl(struct snd_pcm_substream *substream)
+{
+	int retval;
+	union aud_cfg cfg_val = {.cfg_regval = 0};
+	union aud_ch_status_0 ch_stat0 = {.status_0_regval = 0};
+	union aud_ch_status_1 ch_stat1 = {.status_1_regval = 0};
+	union aud_buf_config buf_cfg = {.buf_cfgval = 0};
+	u8 channels;
+
+	/* Currently only PCM stream is supported */
+	ch_stat0.status_0_regx.lpcm_id = 0;
+
+	switch (substream->runtime->rate) {
+	case AUD_SAMPLE_RATE_32:
+		ch_stat0.status_0_regx.samp_freq = CH_STATUS_MAP_32KHZ;
+	break;
+	case AUD_SAMPLE_RATE_44_1:
+	case AUD_SAMPLE_RATE_88_2:
+	case AUD_SAMPLE_RATE_176_4:
+		ch_stat0.status_0_regx.samp_freq = CH_STATUS_MAP_44KHZ;
+	break;
+	case AUD_SAMPLE_RATE_48:
+	case AUD_SAMPLE_RATE_96:
+	case HAD_MAX_RATE:
+		ch_stat0.status_0_regx.samp_freq = CH_STATUS_MAP_48KHZ;
+	break;
+	default:
+		retval = -EINVAL;
+	break;
+	}
+	ch_stat0.status_0_regx.clk_acc = 0;
+	retval = intelhaddata->reg_ops->hdmi_audio_write_register(
+				AUD_CH_STATUS_0, ch_stat0.status_0_regval);
+	if (retval)
+		return retval;
+
+	if (substream->runtime->format == SNDRV_PCM_FORMAT_S16_LE) {
+		ch_stat1.status_1_regx.max_wrd_len = MAX_SMPL_WIDTH_20;
+		ch_stat1.status_1_regx.wrd_len = SMPL_WIDTH_16BITS;
+	} else if (substream->runtime->format == SNDRV_PCM_FORMAT_S24_LE) {
+		ch_stat1.status_1_regx.max_wrd_len = MAX_SMPL_WIDTH_24;
+		ch_stat1.status_1_regx.wrd_len = SMPL_WIDTH_24BITS;
+	} else {
+		ch_stat1.status_1_regx.max_wrd_len = 0;
+		ch_stat1.status_1_regx.wrd_len = 0;
+	}
+	retval = intelhaddata->reg_ops->hdmi_audio_write_register(
+				AUD_CH_STATUS_1, ch_stat1.status_1_regval);
+	if (retval)
+		return retval;
+
+	buf_cfg.buf_cfg_regx.fifo_width	 = FIFO_THRESHOLD;
+	buf_cfg.buf_cfg_regx.aud_delay	 = 0; /* TODO: Read from EDID */
+	retval = intelhaddata->reg_ops->hdmi_audio_write_register(
+					AUD_BUF_CONFIG, buf_cfg.buf_cfgval);
+	if (retval)
+		return retval;
+
+	channels = substream->runtime->channels;
+	switch (channels) {
+	case 1:
+	case 2:
+		cfg_val.cfg_regx.num_ch = CH_STEREO;
+		cfg_val.cfg_regx.layout = LAYOUT0;
+		break;
+	case 3:
+	case 4:
+		cfg_val.cfg_regx.num_ch = CH_THREE_FOUR;
+		cfg_val.cfg_regx.layout = LAYOUT1;
+		break;
+	case 5:
+	case 6:
+		cfg_val.cfg_regx.num_ch = CH_FIVE_SIX;
+		cfg_val.cfg_regx.layout = LAYOUT1;
+		break;
+	case 7:
+	case 8:
+		cfg_val.cfg_regx.num_ch = CH_SEVEN_EIGHT;
+		cfg_val.cfg_regx.layout = LAYOUT1;
+		break;
+	}
+	cfg_val.cfg_regx.val_bit = 1;
+	retval = intelhaddata->reg_ops->hdmi_audio_write_register(
+						AUD_CONFIG, cfg_val.cfg_regval);
+	if (retval)
+		return retval;
+	return 0;
+}
+
+/**
+ * snd_intelhad_prog_DIP - to initialize Data Island Packets registers
+ *
+ * @substream:substream for which the prepare function is called
+ *
+ * This function is called in the prepare callback
+ */
+static int snd_intelhad_prog_DIP(struct snd_pcm_substream *substream)
+{
+	int retval, i;
+	union aud_ctrl_st ctrl_state = {.ctrl_val = 0};
+	union aud_info_frame2 frame2 = {.fr2_val = 0};
+	union aud_info_frame3 frame3 = {.fr3_val = 0};
+	u8 checksum = 0;
+
+	retval = intelhaddata->reg_ops->hdmi_audio_write_register(
+					AUD_CNTL_ST, ctrl_state.ctrl_val);
+	if (retval)
+		return retval;
+
+	frame2.fr2_regx.chnl_cnt = substream->runtime->channels - 1;
+
+	/*TODO: Read from intelhaddata->eeld.speaker_allocation_block;*/
+	frame3.fr3_regx.chnl_alloc = CHANNEL_ALLOCATION;
+
+	/*Calculte the byte wide checksum for all valid DIP words*/
+	for (i = 0; i < BYTES_PER_WORD; i++)
+		checksum += (INFO_FRAME_WORD1 >> i*BITS_PER_BYTE) & SET_BYTE0;
+	for (i = 0; i < BYTES_PER_WORD; i++)
+		checksum += (frame2.fr2_val >> i*BITS_PER_BYTE) & SET_BYTE0;
+	for (i = 0; i < BYTES_PER_WORD; i++)
+		checksum += (frame3.fr3_val >> i*BITS_PER_BYTE) & SET_BYTE0;
+
+	frame2.fr2_regx.chksum = ~(checksum)+1;
+
+	retval = intelhaddata->reg_ops->hdmi_audio_write_register(
+					AUD_HDMIW_INFOFR, INFO_FRAME_WORD1);
+	if (retval)
+		return retval;
+	retval = intelhaddata->reg_ops->hdmi_audio_write_register(
+					AUD_HDMIW_INFOFR, frame2.fr2_val);
+	if (retval)
+		return retval;
+	retval = intelhaddata->reg_ops->hdmi_audio_write_register(
+					AUD_HDMIW_INFOFR, frame3.fr3_val);
+	/* program remaining DIP words with zero */
+	for (i = 0; i < MAX_DIP_WORDS-VALID_DIP_WORDS; i++) {
+		retval = intelhaddata->reg_ops->hdmi_audio_write_register(
+					AUD_HDMIW_INFOFR, 0x0);
+		if (retval)
+			return retval;
+	}
+
+	ctrl_state.ctrl_regx.dip_freq = SET_BIT0;
+	ctrl_state.ctrl_regx.dip_en_sta = SET_BIT0;
+	retval = intelhaddata->reg_ops->hdmi_audio_write_register(
+					AUD_CNTL_ST, ctrl_state.ctrl_val);
+	return retval;
+}
+
+/**
+ * snd_intelhad_prog_ring_buf - programs A , B, C and D
+ * address and length registers
+ *
+ * @substream:substream for which the prepare function is called
+ *
+ * This function programs ring buffer address and length into registers.
+ */
+static int snd_intelhad_prog_ring_buf(struct snd_pcm_substream *substream)
+{
+	int retval;
+	u32 ring_buf_addr, ring_buf_size, period_bytes;
+	u8 i, num_periods;
+	struct snd_intelhad *intelhaddata =  snd_pcm_substream_chip(substream);
+
+	ring_buf_addr = virt_to_phys(substream->runtime->dma_area);
+	pr_debug("had: Ring buffer address = %#x\n", ring_buf_addr);
+	ring_buf_size = snd_pcm_lib_buffer_bytes(substream);
+	pr_debug("had: Ring buffer size = %#x\n", ring_buf_size);
+	intelhaddata->stream_info.ring_buf_size = ring_buf_size;
+
+	period_bytes = frames_to_bytes(substream->runtime,
+				substream->runtime->period_size);
+	pr_debug("had: period size in bytes = %d\n", period_bytes);
+	num_periods = substream->runtime->periods;
+
+	/* Hardware supports MAX_PERIODS buffers */
+	if (num_periods > MAX_PERIODS)
+		return -EINVAL;
+
+	for (i = 0; i < num_periods; i++) {
+		/* Program the buf registers with addr and len */
+		intelhaddata->buf_info[i].buf_addr = ring_buf_addr  +
+							 (i * period_bytes);
+		intelhaddata->buf_info[i].buf_size = period_bytes;
+		retval = intelhaddata->reg_ops->hdmi_audio_write_register(
+					AUD_BUF_A_ADDR + (i * REG_WIDTH),
+					intelhaddata->buf_info[i].buf_addr |
+					REG_BIT_0 | REG_BIT_1);
+		if (retval)
+			return retval;
+		retval = intelhaddata->reg_ops->hdmi_audio_write_register(
+					AUD_BUF_A_LENGTH + (i * REG_WIDTH),
+					period_bytes);
+		if (retval)
+			return retval;
+		intelhaddata->buf_info[i].is_valid = true;
+	}
+	intelhaddata->valid_buf_cnt = num_periods;
+	intelhaddata->curr_buf = HAD_BUF_TYPE_A;
+	return 0;
+}
+
+/**
+ * snd_intelhad_prog_cts - Program HDMI audio CTS value
+ *
+ * @aud_samp_freq: sampling frequency of audio data
+ * @tmds: sampling frequency of the display data
+ * @n_param: N value, depends on aud_samp_freq
+ *
+ * Program CTS register based on the audio and display sampling frequency
+ */
+static int snd_intelhad_prog_cts(u32 aud_samp_freq, u32 tmds, u32 n_param)
+{
+	u32 cts_val;
+	int retval = 0;
+
+	/* Calculate CTS according to HDMI 1.3a spec*/
+	cts_val = (tmds * n_param)/(128 * aud_samp_freq);
+	pr_debug("had:CTS Value=%d\n", cts_val);
+	retval = intelhaddata->reg_ops->hdmi_audio_write_register(
+				AUD_HDMI_CTS, (REG_BIT_20 | cts_val));
+	return retval;
+}
+
+/**
+ * snd_intelhad_prog_n - Program HDMI audio N value
+ *
+ * @aud_samp_freq: sampling frequency of audio data
+ * @n_param: N value, depends on aud_samp_freq
+ *
+ * This function is called in the prepare callback.
+ * It programs based on the audio and display sampling frequency
+ */
+static int snd_intelhad_prog_n(u32 aud_samp_freq, u32 *n_param)
+{
+	u32 n_val;
+	int retval = 0;
+
+	/* Select N according to HDMI 1.3a spec*/
+	switch (aud_samp_freq) {
+	case AUD_SAMPLE_RATE_32:
+		n_val = 4096;
+	break;
+	case AUD_SAMPLE_RATE_44_1:
+		n_val = 6272;
+	break;
+	case AUD_SAMPLE_RATE_48:
+		n_val = 6144;
+	break;
+	case AUD_SAMPLE_RATE_88_2:
+		n_val = 12544;
+	break;
+	case AUD_SAMPLE_RATE_96:
+		n_val = 12288;
+	break;
+	case AUD_SAMPLE_RATE_176_4:
+		n_val = 25088;
+	break;
+	case HAD_MAX_RATE:
+		n_val = 24576;
+	break;
+	default:
+		retval = -EINVAL;
+	break;
+	}
+	if (retval)
+		return retval;
+	retval = intelhaddata->reg_ops->hdmi_audio_write_register(
+				AUD_N_ENABLE, (REG_BIT_20 | n_val));
+	*n_param = n_val;
+	return retval;
+}
+
+/**
 * snd_intelhad_open - stream initializations are done here
 * @substream:substream for which the stream function is called
 *
-- 
1.6.2.5

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 5/5] [RFC]intel_hdmi_audio: Kconfig and Makefile
  2010-11-22 13:39 [PATCH 1/5] [RFC]intel_hdmi_audio:include header ramesh.babu
                   ` (2 preceding siblings ...)
  2010-11-22 13:39 ` [PATCH 4/5] [RFC]intel_hdmi_audio:driver module-hdmi hw interface ramesh.babu
@ 2010-11-22 13:39 ` ramesh.babu
  2010-11-22 13:48 ` [PATCH 1/5] [RFC]intel_hdmi_audio:include header Mark Brown
  2010-11-24  9:20 ` Takashi Iwai
  5 siblings, 0 replies; 22+ messages in thread
From: ramesh.babu @ 2010-11-22 13:39 UTC (permalink / raw)
  To: alsa-devel; +Cc: Sailaja Bandarupalli, Ramesh Babu K V

From: Ramesh Babu K V <ramesh.babu@intel.com>

This patch adds Kconfig and Makefile.  For compilation, this
driver is dependent on client driver (platform display driver).

Change-Id: I786b82b7a8cd9136e81a21e076e394199572cd66
Signed-off-by: Ramesh Babu K V <ramesh.babu@intel.com>
Signed-off-by: Sailaja Bandarupalli <sailaja.bandarupalli@intel.com>
---
 sound/drivers/Kconfig                 |   10 ++++++++++
 sound/drivers/Makefile                |    1 +
 sound/drivers/intel_mid_hdmi/Makefile |    2 ++
 3 files changed, 13 insertions(+), 0 deletions(-)
 create mode 100644 sound/drivers/intel_mid_hdmi/Makefile

diff --git a/sound/drivers/Kconfig b/sound/drivers/Kconfig
index 84714a6..e11a607 100644
--- a/sound/drivers/Kconfig
+++ b/sound/drivers/Kconfig
@@ -182,4 +182,14 @@ config SND_AC97_POWER_SAVE_DEFAULT
 	  The default time-out value in seconds for AC97 automatic
 	  power-save mode.  0 means to disable the power-save mode.
 
+config SND_INTELMID_HDMI_AUDIO
+	tristate "Intel MID based HDMI AUDIO driver"
+	depends on MDFD_HDMI
+	default n
+	help
+	   Intel MID HDMI Audio Driver (HAD) enables audio playback through
+	   HDMI interface on Intel Medfield MID platform.  This is an ALSA
+	   driver.  This driver programs HDMI audio sub-system registers
+	   according to the HDMI 13.a spec.
+
 endif	# SND_DRIVERS
diff --git a/sound/drivers/Makefile b/sound/drivers/Makefile
index d4a07f9..fdf9add 100644
--- a/sound/drivers/Makefile
+++ b/sound/drivers/Makefile
@@ -21,3 +21,4 @@ obj-$(CONFIG_SND_PORTMAN2X4) += snd-portman2x4.o
 obj-$(CONFIG_SND_ML403_AC97CR) += snd-ml403-ac97cr.o
 
 obj-$(CONFIG_SND) += opl3/ opl4/ mpu401/ vx/ pcsp/
+obj-$(CONFIG_SND_INTELMID_HDMI_AUDIO) 	+= intel_mid_hdmi/
diff --git a/sound/drivers/intel_mid_hdmi/Makefile b/sound/drivers/intel_mid_hdmi/Makefile
new file mode 100644
index 0000000..a53e4f0
--- /dev/null
+++ b/sound/drivers/intel_mid_hdmi/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_SND_INTELMID_HDMI_AUDIO)	+= intel_mid_had.o
+intel_mid_had-objs := intel_mid_hdmi_audio.o intel_mid_hdmi_audio_if.o
-- 
1.6.2.5

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/5] [RFC]intel_hdmi_audio:include header
  2010-11-22 13:39 [PATCH 1/5] [RFC]intel_hdmi_audio:include header ramesh.babu
                   ` (3 preceding siblings ...)
  2010-11-22 13:39 ` [PATCH 5/5] [RFC]intel_hdmi_audio: Kconfig and Makefile ramesh.babu
@ 2010-11-22 13:48 ` Mark Brown
  2010-11-25  4:59   ` Bandarupalli, Sailaja
  2010-11-24  9:20 ` Takashi Iwai
  5 siblings, 1 reply; 22+ messages in thread
From: Mark Brown @ 2010-11-22 13:48 UTC (permalink / raw)
  To: ramesh.babu; +Cc: alsa-devel, Sailaja Bandarupalli

On Mon, Nov 22, 2010 at 07:09:42PM +0530, ramesh.babu@intel.com wrote:

> +#define AUD_SAMPLE_RATE_32	HAD_MIN_RATE
> +#define AUD_SAMPLE_RATE_44_1	44100
> +#define AUD_SAMPLE_RATE_48	48000
> +#define AUD_SAMPLE_RATE_88_2	88200
> +#define AUD_SAMPLE_RATE_96	96000
> +#define AUD_SAMPLE_RATE_176_4	176400
> +#define AUD_SAMPLE_RATE_192	HAD_MAX_RATE

The defines for the individual rates don't seem to serve any purpose
here - they're just defined to the sample rate as an integer, and the
one place they're used looks to be a switch statement where I found
myself wondering why either standard ALSA defines or the raw numbers
weren't being used.

> +#define DRIVER_NAME		"intelmid_hdmi_audio"
> +#define DIS_SAMPLE_RATE_25_2	25200
> +#define DIS_SAMPLE_RATE_27	27000
> +#define DIS_SAMPLE_RATE_54	54000
> +#define DIS_SAMPLE_RATE_74_25	74250
> +#define DIS_SAMPLE_RATE_148_5	148500

Ditto here.

> +#define REG_BIT_0		0x1
> +#define REG_BIT_1		0x2
> +#define REG_BIT_2		0x4
> +#define REG_BIT_3		0x8
> +#define REG_BIT_20		0x100000

There's a standard BIT() macro IIRC.

> +#define SET_BIT0		1
> +#define CLEAR_BIT0		0

These macros don't do anything like what one would expect...

> +/**
> + * enum intel_had_aud_buf_type - HDMI controller ring buffer types
> + *
> + * @HAD_BUF_TYPE_A: ring buffer A
> + * @HAD_BUF_TYPE_B: ring buffer B
> + * @HAD_BUF_TYPE_C: ring buffer C
> + * @HAD_BUF_TYPE_D: ring buffer D
> + */
> +enum intel_had_aud_buf_type {
> +	HAD_BUF_TYPE_A = 0,
> +	HAD_BUF_TYPE_B = 1,
> +	HAD_BUF_TYPE_C = 2,
> +	HAD_BUF_TYPE_D = 3,
> +};

These could use a little more explanation.

> +extern struct snd_intelhad *intelhaddata;
> +extern struct snd_pcm_ops snd_intelhad_playback_ops;
> +extern int hdmi_card_index;
> +extern char *hdmi_card_id;

These global variables look very suspicous - they look like they should
be local to some file?

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/5] [RFC]intel_hdmi_audio: interface module
  2010-11-22 13:39 ` [PATCH 2/5] [RFC]intel_hdmi_audio: interface module ramesh.babu
@ 2010-11-22 13:58   ` Mark Brown
  2010-11-24  9:05     ` Takashi Iwai
  0 siblings, 1 reply; 22+ messages in thread
From: Mark Brown @ 2010-11-22 13:58 UTC (permalink / raw)
  To: ramesh.babu; +Cc: alsa-devel, Sailaja Bandarupalli

On Mon, Nov 22, 2010 at 07:09:43PM +0530, ramesh.babu@intel.com wrote:

> +/**
> + * snd_intelhad_dev_free - to free alsa card instance
> + *
> + * @device: pointer to device
> + *
> + * This function is called when the hdmi cable is un plugged
> + */
> +static int snd_intelhad_dev_free(struct snd_device *device)
> +{
> +	struct snd_intelhad *intelhaddata;
> +
> +	BUG_ON(!device);
> +	pr_debug("had: snd_intelhad_dev_free called\n");
> +	intelhaddata = device->device_data;
> +	return 0;
> +}

This function doesn't appear to do anything like what either the
description or name would suggest?  I'd expect to see some deallocation
going on...

> +/**
> + * had_hot_plug - to create sound card instance for HDMI audio playabck
> + *
> + *
> + * This function is called when the hdmi cable is plugged in. This function
> + * creates and registers the sound card with ALSA
> + */
> +static int had_hot_plug(void)
> +{

It would seem more natural for the HDMI device to be created and
instatiated via the normal Linux device model - when a HDMI connection
is detected the thing doing the detection creates and registers a new
device of some kind which the driver core then binds to a separate
driver.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/5] [RFC]intel_hdmi_audio: interface module
  2010-11-22 13:58   ` Mark Brown
@ 2010-11-24  9:05     ` Takashi Iwai
  2010-11-24 10:11       ` Mark Brown
  2010-11-25  5:54       ` Bandarupalli, Sailaja
  0 siblings, 2 replies; 22+ messages in thread
From: Takashi Iwai @ 2010-11-24  9:05 UTC (permalink / raw)
  To: Mark Brown; +Cc: alsa-devel, Sailaja Bandarupalli, ramesh.babu

At Mon, 22 Nov 2010 13:58:12 +0000,
Mark Brown wrote:
> 
> On Mon, Nov 22, 2010 at 07:09:43PM +0530, ramesh.babu@intel.com wrote:
> 
> > +/**
> > + * snd_intelhad_dev_free - to free alsa card instance
> > + *
> > + * @device: pointer to device
> > + *
> > + * This function is called when the hdmi cable is un plugged
> > + */
> > +static int snd_intelhad_dev_free(struct snd_device *device)
> > +{
> > +	struct snd_intelhad *intelhaddata;
> > +
> > +	BUG_ON(!device);
> > +	pr_debug("had: snd_intelhad_dev_free called\n");
> > +	intelhaddata = device->device_data;
> > +	return 0;
> > +}
> 
> This function doesn't appear to do anything like what either the
> description or name would suggest?  I'd expect to see some deallocation
> going on...

Reading through the code, the construct doesn't seem to allocate any
private stuff but only standard components, so this callback doesn't
have to do anything much, I suppose.

> > +/**
> > + * had_hot_plug - to create sound card instance for HDMI audio playabck
> > + *
> > + *
> > + * This function is called when the hdmi cable is plugged in. This function
> > + * creates and registers the sound card with ALSA
> > + */
> > +static int had_hot_plug(void)
> > +{
> 
> It would seem more natural for the HDMI device to be created and
> instatiated via the normal Linux device model - when a HDMI connection
> is detected the thing doing the detection creates and registers a new
> device of some kind which the driver core then binds to a separate
> driver.

Yeah, the driver structure here is somehow wacky, although it's not
to hard to trace.


Takashi

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/5] [RFC]intel_hdmi_audio: driver module - ALSA intereaction
  2010-11-22 13:39 ` [PATCH 3/5] [RFC]intel_hdmi_audio: driver module - ALSA intereaction ramesh.babu
@ 2010-11-24  9:16   ` Takashi Iwai
  2010-11-25  5:40     ` Bandarupalli, Sailaja
  0 siblings, 1 reply; 22+ messages in thread
From: Takashi Iwai @ 2010-11-24  9:16 UTC (permalink / raw)
  To: ramesh.babu; +Cc: alsa-devel, Sailaja Bandarupalli

At Mon, 22 Nov 2010 19:09:44 +0530,
ramesh.babu@intel.com wrote:
> 
> From: Ramesh Babu K V <ramesh.babu@intel.com>
> 
> This patch creates intel_mid_hdmi_audio.c file.  It intereacts
> with ALSA framework to enable the audio playback through HDMI
> interface.  This code uses standard ALSA APIs.
> 
> Signed-off-by: Ramesh Babu K V <ramesh.babu@intel.com>
> Signed-off-by: Sailaja Bandarupalli <sailaja.bandarupalli@intel.com>
> ---
>  .../drivers/intel_mid_hdmi/intel_mid_hdmi_audio.c  |  546 ++++++++++++++++++++
>  1 files changed, 546 insertions(+), 0 deletions(-)
>  create mode 100644 sound/drivers/intel_mid_hdmi/intel_mid_hdmi_audio.c
> 
> diff --git a/sound/drivers/intel_mid_hdmi/intel_mid_hdmi_audio.c b/sound/drivers/intel_mid_hdmi/intel_mid_hdmi_audio.c
> new file mode 100644
> index 0000000..3ef4d48
> --- /dev/null
> +++ b/sound/drivers/intel_mid_hdmi/intel_mid_hdmi_audio.c
> @@ -0,0 +1,546 @@
> +/*
> + *   intel_mid_hdmi_audio.c - Intel HDMI audio driver for MID
> + *
> + *  Copyright (C) 2010 Intel Corp
> + *  Authors:	Sailaja Bandarupalli <sailaja.bandarupalli@intel.com>
> + *		Ramesh Babu K V	<ramesh.babu@intel.com>
> + *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; version 2 of the License.
> + *
> + *  This program is distributed in the hope that it will be useful, but
> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + * ALSA driver for Intel MID HDMI audio controller
> + */
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <sound/pcm.h>
> +#include <sound/pcm_params.h>
> +#include <sound/initval.h>
> +#include <sound/control.h>
> +#include <sound/initval.h>
> +#include "intel_mid_hdmi_audio.h"
> +
> +MODULE_AUTHOR("Sailaja Bandarupalli <sailaja.bandarupalli@intel.com>");
> +MODULE_AUTHOR("Ramesh Babu K V <ramesh.babu@intel.com>");
> +MODULE_DESCRIPTION("Intel HDMI Audio driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_SUPPORTED_DEVICE("{Intel,Intel_HAD}");
> +
> +#define INFO_FRAME_WORD1	0x000a0184
> +#define FIFO_THRESHOLD		0xFE
> +#define BYTES_PER_WORD		0x4
> +#define CH_STATUS_MAP_32KHZ	0x3
> +#define CH_STATUS_MAP_44KHZ	0x0
> +#define CH_STATUS_MAP_48KHZ	0x2
> +#define MAX_SMPL_WIDTH_20	0x0
> +#define MAX_SMPL_WIDTH_24	0x1
> +#define SMPL_WIDTH_16BITS	0x1
> +#define SMPL_WIDTH_24BITS	0x5
> +#define CHANNEL_ALLOCATION	0x1F
> +#define SET_BYTE0		0x000000FF
> +#define VALID_DIP_WORDS		3
> +#define LAYOUT0			0
> +#define LAYOUT1			1
> +
> +/*standard module options for ALSA. This module supports only one card*/
> +int hdmi_card_index = SNDRV_DEFAULT_IDX1;
> +char *hdmi_card_id = SNDRV_DEFAULT_STR1;
> +
> +module_param(hdmi_card_index, int, 0444);
> +MODULE_PARM_DESC(hdmi_card_index,
> +		"Index value for INTEL Intel HDMI Audio controller.");
> +module_param(hdmi_card_id, charp, 0444);
> +MODULE_PARM_DESC(hdmi_card_id,
> +		"ID string for INTEL Intel HDMI Audio controller.");

Drivers should expose the standard module options "index" and "id"
like others.  Also they should be static.

> +
> +/* hardware capability structure */
> +static const struct snd_pcm_hardware snd_intel_hadstream = {
> +	.info =	(SNDRV_PCM_INFO_INTERLEAVED |
> +		SNDRV_PCM_INFO_DOUBLE |
> +		SNDRV_PCM_INFO_PAUSE |
> +		SNDRV_PCM_INFO_RESUME |

You have to implement corresponding stuff if you mark the driver as
RESUME-able.

> +		SNDRV_PCM_INFO_MMAP|
> +		SNDRV_PCM_INFO_MMAP_VALID |
> +		SNDRV_PCM_INFO_BATCH |
> +		SNDRV_PCM_INFO_SYNC_START),

Is sync stuff implemented?

> +	.formats = (SNDRV_PCM_FMTBIT_S24 |
> +		SNDRV_PCM_FMTBIT_U24),
> +	.rates = SNDRV_PCM_RATE_32000 |
> +		SNDRV_PCM_RATE_44100 |
> +		SNDRV_PCM_RATE_48000 |
> +		SNDRV_PCM_RATE_64000 |
> +		SNDRV_PCM_RATE_88200 |
> +		SNDRV_PCM_RATE_96000 |
> +		SNDRV_PCM_RATE_176400 |
> +		SNDRV_PCM_RATE_192000,
> +	.rate_min = HAD_MIN_RATE,
> +	.rate_max = HAD_MAX_RATE,
> +	.channels_min = HAD_MIN_CHANNEL,
> +	.channels_max = HAD_MAX_CHANNEL,
> +	.buffer_bytes_max = HAD_MAX_PERIOD_BYTES,
> +	.period_bytes_min = HAD_MIN_PERIOD_BYTES,
> +	.period_bytes_max = HAD_MAX_BUFFER,
> +	.periods_min = HAD_MIN_PERIODS,
> +	.periods_max = HAD_MAX_PERIODS,
> +	.fifo_size = HAD_FIFO_SIZE,
> +};
> +
> +struct snd_intelhad *intelhaddata;
> +
> +/**
> +* snd_intelhad_open - stream initializations are done here
> +* @substream:substream for which the stream function is called
> +*
> +* This function is called whenever a PCM stream is opened
> +*/
> +static int snd_intelhad_open(struct snd_pcm_substream *substream)
> +{
> +	struct snd_intelhad *intelhaddata;
> +	struct snd_pcm_runtime *runtime;
> +	struct had_stream_pvt *stream;
> +	int retval;
> +
> +	BUG_ON(!substream);

These BUG_ON() are superfluous.  Better to remove.


> +	pr_debug("had: snd_intelhad_open called\n");
> +
> +	intelhaddata = snd_pcm_substream_chip(substream);

So... you assign the global variable at this open callback?
Any race?

> +	runtime = substream->runtime;
> +	/* set the runtime hw parameter with local snd_pcm_hardware struct */
> +	runtime->hw = snd_intel_hadstream;
> +
> +	stream = kzalloc(sizeof(*stream), GFP_KERNEL);
> +	if (!stream)
> +		return -ENOMEM;
> +	stream->stream_status = STREAM_INIT;
> +	runtime->private_data = stream;
> +	intelhaddata->reg_ops->hdmi_audio_write_register(AUD_HDMI_STATUS, 0);
> +	retval = snd_pcm_hw_constraint_integer(runtime,
> +			 SNDRV_PCM_HW_PARAM_PERIODS);

stream is leaked when the error returned here.

> +	return retval;
> +}
> +
> +/**
> +* had_period_elapsed - updates the hardware pointer status
> +* @had_substream:substream for which the stream function is called
> +*
> +*/
> +static void had_period_elapsed(void *had_substream)
> +{
> +	struct snd_pcm_substream *substream = had_substream;
> +	struct had_stream_pvt *stream;
> +
> +	pr_debug("had: calling period elapsed\n");
> +	if (!substream || !substream->runtime)
> +		return;
> +	stream = substream->runtime->private_data;
> +	if (!stream)
> +		return;
> +
> +	if (stream->stream_status != STREAM_RUNNING)
> +		return;
> +	snd_pcm_period_elapsed(substream);
> +	return;
> +}
> +
> +/**
> +* snd_intelhad_init_stream - internal function to initialize stream info
> +* @substream:substream for which the stream function is called
> +*
> +*/
> +static int snd_intelhad_init_stream(struct snd_pcm_substream *substream)
> +{
> +	struct snd_intelhad *intelhaddata = snd_pcm_substream_chip(substream);
> +
> +	pr_debug("had: setting buffer ptr param\n");
> +	intelhaddata->stream_info.period_elapsed = had_period_elapsed;
> +	intelhaddata->stream_info.had_substream = substream;
> +	intelhaddata->stream_info.buffer_ptr = 0;
> +	intelhaddata->stream_info.buffer_rendered = 0;
> +	intelhaddata->stream_info.sfreq = substream->runtime->rate;
> +	return 0;
> +}
> +
> +/**
> + * snd_intelhad_close- to free parameteres when stream is stopped
> + *
> + * @substream:  substream for which the function is called
> + *
> + * This function is called by ALSA framework when stream is stopped
> + */
> +static int snd_intelhad_close(struct snd_pcm_substream *substream)
> +{
> +	struct snd_intelhad *intelhaddata;
> +	struct had_stream_pvt *stream;
> +	BUG_ON(!substream);
> +
> +	stream = substream->runtime->private_data;
> +
> +	pr_debug("had: snd_intelhad_close called\n");
> +	intelhaddata = snd_pcm_substream_chip(substream);
> +	if (intelhaddata->stream_info.str_id) {
> +		intelhaddata->playback_cnt--;
> +		intelhaddata->stream_info.str_id = 0;
> +	}
> +	intelhaddata->stream_info.buffer_rendered = 0;
> +	intelhaddata->stream_info.buffer_ptr = 0;
> +
> +	kfree(substream->runtime->private_data);
> +	return 0;
> +}
> +
> +/**
> + * snd_intelhad_hw_params- to setup the hardware parameters
> + * like allocating the buffers
> + *
> + * @substream:  substream for which the function is called
> + * @hw_params: hardware parameters
> + *
> + * This function is called by ALSA framework when hardware params are set
> + */
> +static int snd_intelhad_hw_params(struct snd_pcm_substream *substream,
> +				    struct snd_pcm_hw_params *hw_params)
> +{
> +	int retval;
> +	struct snd_pcm_runtime *runtime;
> +
> +	BUG_ON(!substream);
> +	BUG_ON(!hw_params);
> +	pr_debug("had: snd_intelhad_hw_params called\n");
> +
> +	runtime = substream->runtime;
> +
> +	retval = snd_pcm_lib_malloc_pages(substream,
> +					params_buffer_bytes(hw_params));
> +	if (retval < 0)
> +		return -ENOMEM;
> +	pr_debug("had: allocated memory = %d\n",
> +					params_buffer_bytes(hw_params));
> +	memset(substream->runtime->dma_area, 0,
> +			params_buffer_bytes(hw_params));
> +
> +	pr_debug("had: snd_intelhad_hw_params exited\n");
> +	return retval;
> +}
> +
> +/**
> + * snd_intelhad_hw_free- to release the resources allocated during
> + * hardware params setup
> + *
> + * @substream:  substream for which the function is called
> + *
> + * This function is called by ALSA framework before close callback.
> + *
> + */
> +static int snd_intelhad_hw_free(struct snd_pcm_substream *substream)
> +{
> +	BUG_ON(!substream);
> +	pr_debug("had: snd_intelhad_hw_free called\n");
> +	return snd_pcm_lib_free_pages(substream);
> +}
> +
> +/**
> +* snd_intelhad_pcm_trigger - stream activities are handled here
> +* @substream:substream for which the stream function is called
> +* @cmd:the stream commamd thats requested from upper layer
> +* This function is called whenever an a stream activity is invoked
> +*/
> +static int snd_intelhad_pcm_trigger(struct snd_pcm_substream *substream,
> +					int cmd)
> +{
> +	int i, caps, retval = 0;
> +	u32 regval = 0;
> +	struct snd_intelhad *intelhaddata;
> +	struct had_stream_pvt *stream;
> +	struct hdmi_audio_registers_ops *reg_ops;
> +	struct hdmi_audio_query_set_ops *query_ops;
> +
> +	BUG_ON(!substream);
> +
> +	intelhaddata = snd_pcm_substream_chip(substream);
> +	stream = substream->runtime->private_data;
> +	reg_ops = intelhaddata->reg_ops;
> +	query_ops = intelhaddata->query_ops;
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +		pr_debug("had: Trigger Start\n");
> +		caps = HDMI_AUDIO_UNDERRUN | HDMI_AUDIO_BUFFER_DONE;
> +		retval = query_ops->hdmi_audio_set_caps(
> +					HAD_SET_ENABLE_AUDIO_INT, &caps);
> +		if (retval)
> +			return retval;
> +		retval = query_ops->hdmi_audio_set_caps(
> +					HAD_SET_ENABLE_AUDIO, NULL);
> +		if (retval)
> +			return retval;

Don't you need to reset audio_caps at error?

> +
> +		retval = reg_ops->hdmi_audio_read_modify(AUD_CONFIG,
> +					SET_BIT0, REG_BIT_0);
> +		stream->substream = substream;
> +		stream->stream_status = STREAM_RUNNING;
> +		break;
> +
> +	case SNDRV_PCM_TRIGGER_STOP:
> +		pr_debug("had: Trigger Stop\n");
> +		caps = HDMI_AUDIO_UNDERRUN | HDMI_AUDIO_BUFFER_DONE;
> +		retval = query_ops->hdmi_audio_set_caps(
> +					HAD_SET_DISABLE_AUDIO_INT, &caps);
> +		if (retval)
> +			return retval;
> +		retval = query_ops->hdmi_audio_set_caps(
> +					HAD_SET_DISABLE_AUDIO, NULL);
> +		if (retval)
> +			return retval;
> +		stream->stream_status = STREAM_DROPPED;
> +		break;
> +
> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +		pr_debug("had: Trigger Pause\n");
> +		/* disable the validity bits */
> +		for (i = 0; i < MAX_HW_BUFS; i++) {
> +			retval = reg_ops->hdmi_audio_read_register(
> +					AUD_BUF_A_ADDR+(i*REG_WIDTH), &regval);
> +			if (retval)
> +				return retval;
> +			if (regval & REG_BIT_0) {
> +				retval = reg_ops->hdmi_audio_read_modify(
> +						AUD_BUF_A_ADDR+(i * REG_WIDTH),
> +						0, REG_BIT_0);
> +				if (retval)
> +					return retval;
> +				intelhaddata->buf_info[HAD_BUF_TYPE_A + i].
> +						is_valid = true;
> +			}
> +		}
> +		stream->stream_status = STREAM_PAUSED;
> +		break;
> +
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		pr_debug("had: Trigger Resume\n");
> +		/* enable the validity bits */
> +		for (i = 0; i < MAX_HW_BUFS; i++) {
> +			if (intelhaddata->buf_info[HAD_BUF_TYPE_A + i].is_valid)
> +				retval = reg_ops->hdmi_audio_read_modify(
> +					AUD_BUF_A_ADDR + (i * REG_WIDTH),
> +					SET_BIT0, REG_BIT_0);
> +		}
> +		stream->stream_status = STREAM_RUNNING;
> +		break;
> +
> +	default:
> +		retval = -EINVAL;
> +	}
> +	return retval;
> +}
> +
> +/**
> +* snd_intelhad_pcm_prepare- internal preparation before starting a stream
> +*
> +* @substream:  substream for which the function is called
> +*
> +* This function is called when a stream is started for internal preparation.
> +*/
> +static int snd_intelhad_pcm_prepare(struct snd_pcm_substream *substream)
> +{
> +	struct had_stream_pvt *stream;
> +	int retval;
> +	u32 disp_samp_freq, n_param;
> +	struct snd_intelhad *intelhaddata;
> +	struct snd_pcm_runtime *runtime;
> +
> +	pr_debug("had: pcm_prepare called\n");
> +
> +	BUG_ON(!substream);
> +	runtime = substream->runtime;
> +	pr_debug("had:period_size=%d\n",
> +				frames_to_bytes(runtime, runtime->period_size));
> +	pr_debug("had:periods=%d\n", runtime->periods);
> +	pr_debug("had:buffer_size=%d\n", snd_pcm_lib_buffer_bytes(substream));
> +	pr_debug("had:rate=%d\n", runtime->rate);
> +	pr_debug("had:channels=%d\n", runtime->channels);
> +
> +	stream = substream->runtime->private_data;
> +	intelhaddata = snd_pcm_substream_chip(substream);
> +	if (intelhaddata->stream_info.str_id) {
> +		pr_debug("had:_prepare is called for existing str_id#%d\n",
> +					intelhaddata->stream_info.str_id);
> +		retval = snd_intelhad_pcm_trigger(substream,
> +						SNDRV_PCM_TRIGGER_STOP);
> +		return retval;
> +	}
> +
> +	intelhaddata->playback_cnt++;
> +	intelhaddata->stream_info.str_id = intelhaddata->playback_cnt;

Note that the prepare callback might be called multiple times before
triggering.  This counter doesn't work.  If any, it should be counted
in the open/close callback.

> +	snprintf(substream->pcm->id, sizeof(substream->pcm->id),
> +			"%d", intelhaddata->stream_info.str_id);
> +	retval = snd_intelhad_init_stream(substream);
> +	if (retval)
> +		goto prep_end;
> +	/* Get N value in KHz */
> +	retval = intelhaddata->query_ops->hdmi_audio_get_caps(
> +				HAD_GET_SAMPLING_FREQ, &disp_samp_freq);
> +	if (retval) {
> +		pr_debug("had: querying display sampling freq failed\n");
> +		goto prep_end;
> +	} else
> +		pr_debug("had: TMDS freq = %d kHz\n", disp_samp_freq);
> +
> +	retval = snd_intelhad_prog_n(substream->runtime->rate, &n_param);
> +	if (retval) {
> +		pr_debug("had: programming N value failed\n");
> +		goto prep_end;
> +	}
> +	retval = snd_intelhad_prog_cts(
> +			substream->runtime->rate/1000, disp_samp_freq, n_param);
> +	if (retval) {
> +		pr_debug("had: programming CTS value failed\n");
> +		goto prep_end;
> +	}
> +
> +	retval = snd_intelhad_prog_DIP(substream);
> +	if (retval) {
> +		pr_debug("had: programming DIP values failed\n");
> +		goto prep_end;
> +	}
> +	retval = snd_intelhad_init_audio_ctrl(substream);
> +	if (retval) {
> +		pr_debug("had: initializing audio controller regs failed\n");
> +		goto prep_end;
> +	}
> +	retval = snd_intelhad_prog_ring_buf(substream) ;
> +	if (retval)
> +		pr_debug("had: initializing ring buffer regs failed\n");
> +
> +prep_end:
> +	return retval;
> +}
> +
> +/**
> + * snd_intelhad_pcm_pointer- to send the current buffer pointer processed by hw
> + *
> + * @substream:  substream for which the function is called
> + *
> + * This function is called by ALSA framework to get the current hw buffer ptr
> + * when a period is elapsed
> + */
> +static snd_pcm_uframes_t snd_intelhad_pcm_pointer(
> +					struct snd_pcm_substream *substream)
> +{
> +	struct had_stream_pvt *stream;
> +	struct snd_intelhad *intelhaddata;
> +	u32 bytes_rendered;
> +
> +	pr_debug("had: Called snd_intelhad_pcm_pointer\n");
> +	BUG_ON(!substream);
> +
> +	intelhaddata = snd_pcm_substream_chip(substream);
> +	stream = substream->runtime->private_data;
> +
> +	div_u64_rem(intelhaddata->stream_info.buffer_rendered,
> +			intelhaddata->stream_info.ring_buf_size,
> +			&(bytes_rendered));
> +	intelhaddata->stream_info.buffer_ptr = bytes_to_frames(
> +						substream->runtime,
> +						bytes_rendered);
> +	pr_debug("had:pcm_pointer = %#x\n",
> +			intelhaddata->stream_info.buffer_ptr);
> +	return intelhaddata->stream_info.buffer_ptr;
> +}
> +
> +/*PCM operations structure and the calls back for the same */
> +struct snd_pcm_ops snd_intelhad_playback_ops = {

Does it need to be global?

> +	.open =	snd_intelhad_open,
> +	.close = snd_intelhad_close,
> +	.ioctl = snd_pcm_lib_ioctl,
> +	.hw_params = snd_intelhad_hw_params,
> +	.hw_free = snd_intelhad_hw_free,
> +	.prepare = snd_intelhad_pcm_prepare,
> +	.trigger = snd_intelhad_pcm_trigger,
> +	.pointer = snd_intelhad_pcm_pointer,
> +};
> +
> +/*
> +* alsa_card_intelhad_init- driver init function
> +* This function is called when driver module is inserted
> +*/
> +static int __init alsa_card_intelhad_init(void)
> +{
> +	int retval;
> +	struct had_callback_ops ops_cb;
> +
> +	pr_debug("had: init called\n");
> +	pr_info(KERN_INFO "INFO: ******** HAD DRIVER loading.. Ver: %s\n",
> +					HAD_DRIVER_VERSION);
> +
> +	/* allocate memory for saving internal context and working */
> +	intelhaddata = kzalloc(sizeof(*intelhaddata), GFP_KERNEL);
> +	if (!intelhaddata) {
> +		pr_debug("had: mem alloc failed\n");
> +		return -ENOMEM;
> +	}
> +	/* allocate memory for display driver api set */
> +	intelhaddata->reg_ops = kzalloc(
> +				sizeof(struct hdmi_audio_registers_ops),
> +				GFP_KERNEL);
> +	if (!intelhaddata->reg_ops) {
> +		pr_debug("had: mem alloc failed\n");
> +		retval = -ENOMEM;
> +		goto free_context;
> +	}
> +	intelhaddata->query_ops = kzalloc(
> +				sizeof(struct hdmi_audio_query_set_ops),
> +				GFP_KERNEL);
> +	if (!intelhaddata->query_ops) {
> +		pr_debug("had: mem alloc failed\n");
> +		retval = -ENOMEM;
> +		goto free_regops;
> +	}

Any reason to allocate extra two *_ops instead of just put these flat
in the structure itself?  I mean,
    struct intelhda {
        ...
        struct hdmi_audio_registers_ops reg_ops;
        struct hdmi_audio_query_set_ops query_ops;
        ...
    }

then you don't need to malloc them.

> +	ops_cb.intel_had_event_call_back = had_event_handler;
> +	/* registering with display driver to get access to display APIs */
> +	retval = intel_hdmi_audio_query_capabilities(
> +			ops_cb.intel_had_event_call_back,
> +			&intelhaddata->reg_ops,
> +			&intelhaddata->query_ops);
> +	if (retval) {
> +		pr_debug("had: registering with display driver failed\n");
> +		goto free_allocs;
> +	}
> +	pr_debug("had:...init complete\n");
> +	return retval;
> +free_allocs:
> +	kfree(intelhaddata->query_ops);
> +free_regops:
> +	kfree(intelhaddata->reg_ops);
> +free_context:
> +	kfree(intelhaddata);
> +	pr_err("had: driver init failed\n");
> +	return retval;
> +}
> +
> +/**
> +* alsa_card_intelhad_exit- driver exit function
> +* This function is called when driver module is removed
> +*/
> +static void __exit alsa_card_intelhad_exit(void)
> +{
> +	pr_debug("had:had_exit called\n");
> +	if (intelhaddata) {
> +		kfree(intelhaddata->query_ops);
> +		kfree(intelhaddata->reg_ops);
> +		kfree(intelhaddata);
> +	}
> +}
> +late_initcall(alsa_card_intelhad_init);
> +module_exit(alsa_card_intelhad_exit);

Overall I feel uneasy about the handling of global variables.
They should be cleaned up more...


thanks,

Takashi

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/5] [RFC]intel_hdmi_audio:include header
  2010-11-22 13:39 [PATCH 1/5] [RFC]intel_hdmi_audio:include header ramesh.babu
                   ` (4 preceding siblings ...)
  2010-11-22 13:48 ` [PATCH 1/5] [RFC]intel_hdmi_audio:include header Mark Brown
@ 2010-11-24  9:20 ` Takashi Iwai
  2010-11-25  5:06   ` Bandarupalli, Sailaja
  5 siblings, 1 reply; 22+ messages in thread
From: Takashi Iwai @ 2010-11-24  9:20 UTC (permalink / raw)
  To: ramesh.babu; +Cc: alsa-devel, Sailaja Bandarupalli

In addition to Mark's comments...

At Mon, 22 Nov 2010 19:09:42 +0530,
ramesh.babu@intel.com wrote:
> 
> +#include <linux/types.h>
> +#include <sound/initval.h>

Usually this header isn't needed in the global header file.

> +#include "../../../drivers/staging/mrst/drv/mdfld_hdmi_audio_if.h"
> +#include "../../../drivers/staging/mrst/drv/psb_intel_hdmi.h"

This is bad...  If these files are really used over several
subsystems, they should be put somewhere in a better public place.


> +/**
> + * union aud_cfg - Audio configuration offset - 69000
> + *
> + * @cfg_regx: individual register bits
> + * @cfg_regval: full register value
> + *
> + * */
> +union aud_cfg {
> +	struct {
> +		u32 aud_en:1;
> +		u32 layout:1;
> +		u32 fmt:2;
> +		u32 num_ch:2;
> +		u32 rsvd0:1;
> +		u32 set:1;
> +		u32 flat:1;
> +		u32 val_bit:1;
> +		u32 user_bit:1;
> +		u32 underrun:1;
> +		u32 rsvd1:20;
> +	} cfg_regx;
> +	u32 cfg_regval;
> +};

In general, better to avoid bit fields if it's used for the data
transfer.  The bit-fields are never portable.


thanks,

Takashi

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/5] [RFC]intel_hdmi_audio: interface module
  2010-11-24  9:05     ` Takashi Iwai
@ 2010-11-24 10:11       ` Mark Brown
  2010-11-24 14:20         ` Takashi Iwai
  2010-11-25  5:54       ` Bandarupalli, Sailaja
  1 sibling, 1 reply; 22+ messages in thread
From: Mark Brown @ 2010-11-24 10:11 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel, Sailaja Bandarupalli, ramesh.babu

On Wed, Nov 24, 2010 at 10:05:05AM +0100, Takashi Iwai wrote:
> Mark Brown wrote:
> > On Mon, Nov 22, 2010 at 07:09:43PM +0530, ramesh.babu@intel.com wrote:

> > > +	intelhaddata = device->device_data;

> > This function doesn't appear to do anything like what either the
> > description or name would suggest?  I'd expect to see some deallocation
> > going on...

> Reading through the code, the construct doesn't seem to allocate any
> private stuff but only standard components, so this callback doesn't
> have to do anything much, I suppose.

To be honest half the issue was the above quoted line - if the function
had been empty I'd have been less concerned (and probably just said to
remove it).

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/5] [RFC]intel_hdmi_audio: interface module
  2010-11-24 10:11       ` Mark Brown
@ 2010-11-24 14:20         ` Takashi Iwai
  0 siblings, 0 replies; 22+ messages in thread
From: Takashi Iwai @ 2010-11-24 14:20 UTC (permalink / raw)
  To: Mark Brown; +Cc: alsa-devel, Sailaja Bandarupalli, ramesh.babu

At Wed, 24 Nov 2010 10:11:00 +0000,
Mark Brown wrote:
> 
> On Wed, Nov 24, 2010 at 10:05:05AM +0100, Takashi Iwai wrote:
> > Mark Brown wrote:
> > > On Mon, Nov 22, 2010 at 07:09:43PM +0530, ramesh.babu@intel.com wrote:
> 
> > > > +	intelhaddata = device->device_data;
> 
> > > This function doesn't appear to do anything like what either the
> > > description or name would suggest?  I'd expect to see some deallocation
> > > going on...
> 
> > Reading through the code, the construct doesn't seem to allocate any
> > private stuff but only standard components, so this callback doesn't
> > have to do anything much, I suppose.
> 
> To be honest half the issue was the above quoted line - if the function
> had been empty I'd have been less concerned (and probably just said to
> remove it).

I understand better now :)


Takashi

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/5] [RFC]intel_hdmi_audio:include header
  2010-11-22 13:48 ` [PATCH 1/5] [RFC]intel_hdmi_audio:include header Mark Brown
@ 2010-11-25  4:59   ` Bandarupalli, Sailaja
  0 siblings, 0 replies; 22+ messages in thread
From: Bandarupalli, Sailaja @ 2010-11-25  4:59 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: Wu, Fengguang, alsa-devel, Mark Brown, Babu, Ramesh

Hi Mark and Takashi,
Thanks a lot for providing review comments for RFC patches.

> 
> > +#define AUD_SAMPLE_RATE_32	HAD_MIN_RATE
> > +#define AUD_SAMPLE_RATE_44_1	44100
> > +#define AUD_SAMPLE_RATE_48	48000
> > +#define AUD_SAMPLE_RATE_88_2	88200
> > +#define AUD_SAMPLE_RATE_96	96000
> > +#define AUD_SAMPLE_RATE_176_4	176400
> > +#define AUD_SAMPLE_RATE_192	HAD_MAX_RATE
> 
> The defines for the individual rates don't seem to serve any purpose
> here - they're just defined to the sample rate as an integer, and the
> one place they're used looks to be a switch statement where I found
> myself wondering why either standard ALSA defines or the raw numbers
> weren't being used.

We will use the standard ALSA macros if available otherwise will use 
raw numbers. Overall, we will use the standard definitions where ever
possible and clean the global variables.

Thanks,
Sailaja and Ramesh.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/5] [RFC]intel_hdmi_audio:include header
  2010-11-24  9:20 ` Takashi Iwai
@ 2010-11-25  5:06   ` Bandarupalli, Sailaja
  2010-11-25  5:46     ` Takashi Iwai
  0 siblings, 1 reply; 22+ messages in thread
From: Bandarupalli, Sailaja @ 2010-11-25  5:06 UTC (permalink / raw)
  To: Takashi Iwai, alsa-devel; +Cc: Wu, Fengguang, Mark Brown, Babu, Ramesh



> > +#include <linux/types.h>
> > +#include <sound/initval.h>
> 
> Usually this header isn't needed in the global header file.
> 

We will remove un necessary global header files.

> > +#include "../../../drivers/staging/mrst/drv/mdfld_hdmi_audio_if.h"
> > +#include "../../../drivers/staging/mrst/drv/psb_intel_hdmi.h"
> 
> This is bad...  If these files are really used over several
> subsystems, they should be put somewhere in a better public place.
>

Currently display driver code is in the staging area. Can we move
header files to include folder?

> 
> > +/**
> > + * union aud_cfg - Audio configuration offset - 69000
> > + *
> > + * @cfg_regx: individual register bits
> > + * @cfg_regval: full register value
> > + *
> > + * */
> > +union aud_cfg {
> > +	struct {
> > +		u32 aud_en:1;
> > +		u32 layout:1;
> > +		u32 fmt:2;
> > +		u32 num_ch:2;
> > +		u32 rsvd0:1;
> > +		u32 set:1;
> > +		u32 flat:1;
> > +		u32 val_bit:1;
> > +		u32 user_bit:1;
> > +		u32 underrun:1;
> > +		u32 rsvd1:20;
> > +	} cfg_regx;
> > +	u32 cfg_regval;
> > +};
> 
> In general, better to avoid bit fields if it's used for the data
> transfer.  The bit-fields are never portable.
> 
These bit fields are used for programming the HDMI controller subsystem.

Thanks,
Sailaja and Ramesh.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/5] [RFC]intel_hdmi_audio: driver module - ALSA intereaction
  2010-11-24  9:16   ` Takashi Iwai
@ 2010-11-25  5:40     ` Bandarupalli, Sailaja
  2010-11-25  5:50       ` Takashi Iwai
  0 siblings, 1 reply; 22+ messages in thread
From: Bandarupalli, Sailaja @ 2010-11-25  5:40 UTC (permalink / raw)
  To: Takashi Iwai, alsa-devel; +Cc: Wu, Fengguang, Mark Brown, Babu, Ramesh

> > +/*standard module options for ALSA. This module supports only one
> card*/
> > +int hdmi_card_index = SNDRV_DEFAULT_IDX1;
> > +char *hdmi_card_id = SNDRV_DEFAULT_STR1;
> > +
> > +module_param(hdmi_card_index, int, 0444);
> > +MODULE_PARM_DESC(hdmi_card_index,
> > +		"Index value for INTEL Intel HDMI Audio controller.");
> > +module_param(hdmi_card_id, charp, 0444);
> > +MODULE_PARM_DESC(hdmi_card_id,
> > +		"ID string for INTEL Intel HDMI Audio controller.");
> 
> Drivers should expose the standard module options "index" and "id"
> like others.  Also they should be static.
> 
Agree.
> > +
> > +/* hardware capability structure */
> > +static const struct snd_pcm_hardware snd_intel_hadstream = {
> > +	.info =	(SNDRV_PCM_INFO_INTERLEAVED |
> > +		SNDRV_PCM_INFO_DOUBLE |
> > +		SNDRV_PCM_INFO_PAUSE |
> > +		SNDRV_PCM_INFO_RESUME |
> 
> You have to implement corresponding stuff if you mark the driver as
> RESUME-able.

Yes we have implemented SNDRV_PCM_TRIGGER_PAUSE_RELEASE

> 
> > +		SNDRV_PCM_INFO_MMAP|
> > +		SNDRV_PCM_INFO_MMAP_VALID |
> > +		SNDRV_PCM_INFO_BATCH |
> > +		SNDRV_PCM_INFO_SYNC_START),
> 
> Is sync stuff implemented?

It's not implemented, we will remove it.

> 
> 
> > +	pr_debug("had: snd_intelhad_open called\n");
> > +
> > +	intelhaddata = snd_pcm_substream_chip(substream);
> 
> So... you assign the global variable at this open callback?
> Any race?
>
It's not required, since its global variable. We will remove this.

> > +	stream = kzalloc(sizeof(*stream), GFP_KERNEL);
> > +	if (!stream)
> > +		return -ENOMEM;
> > +	stream->stream_status = STREAM_INIT;
> > +	runtime->private_data = stream;
> > +	intelhaddata->reg_ops->hdmi_audio_write_register(AUD_HDMI_STATUS,
> 0);
> > +	retval = snd_pcm_hw_constraint_integer(runtime,
> > +			 SNDRV_PCM_HW_PARAM_PERIODS);
> 
> stream is leaked when the error returned here.
> 

We will free the stream in error path.

> > +
> > +	switch (cmd) {
> > +	case SNDRV_PCM_TRIGGER_START:
> > +		pr_debug("had: Trigger Start\n");
> > +		caps = HDMI_AUDIO_UNDERRUN | HDMI_AUDIO_BUFFER_DONE;
> > +		retval = query_ops->hdmi_audio_set_caps(
> > +					HAD_SET_ENABLE_AUDIO_INT, &caps);
> > +		if (retval)
> > +			return retval;
> > +		retval = query_ops->hdmi_audio_set_caps(
> > +					HAD_SET_ENABLE_AUDIO, NULL);
> > +		if (retval)
> > +			return retval;
> 
> Don't you need to reset audio_caps at error?

ENABLE_AUDIO_INT and ENABLE_AUDIO just sets register bits and no 
impact even if it fails.

> > +	intelhaddata->playback_cnt++;
> > +	intelhaddata->stream_info.str_id = intelhaddata->playback_cnt;
> 
> Note that the prepare callback might be called multiple times before
> triggering.  This counter doesn't work.  If any, it should be counted
> in the open/close callback.

We will move the counters to _open.

> > +
> > +/*PCM operations structure and the calls back for the same */
> > +struct snd_pcm_ops snd_intelhad_playback_ops = {
> 
> Does it need to be global?

We will make it static.

> 
> Any reason to allocate extra two *_ops instead of just put these flat
> in the structure itself?  I mean,
>     struct intelhda {
>         ...
>         struct hdmi_audio_registers_ops reg_ops;
>         struct hdmi_audio_query_set_ops query_ops;
>         ...
>     }
> 
> then you don't need to malloc them.

We will declare them as flat structures.

> 
> Overall I feel uneasy about the handling of global variables.
> They should be cleaned up more...

We will clean up the global variables.

Thanks,
Sailaja and Ramesh.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/5] [RFC]intel_hdmi_audio:include header
  2010-11-25  5:06   ` Bandarupalli, Sailaja
@ 2010-11-25  5:46     ` Takashi Iwai
  2010-11-25  6:07       ` Bandarupalli, Sailaja
  0 siblings, 1 reply; 22+ messages in thread
From: Takashi Iwai @ 2010-11-25  5:46 UTC (permalink / raw)
  To: Bandarupalli, Sailaja; +Cc: Wu, Fengguang, alsa-devel, Mark Brown, Babu, Ramesh

At Thu, 25 Nov 2010 10:36:49 +0530,
Bandarupalli, Sailaja wrote:
> 
> 
> 
> > > +#include <linux/types.h>
> > > +#include <sound/initval.h>
> > 
> > Usually this header isn't needed in the global header file.
> > 
> 
> We will remove un necessary global header files.
> 
> > > +#include "../../../drivers/staging/mrst/drv/mdfld_hdmi_audio_if.h"
> > > +#include "../../../drivers/staging/mrst/drv/psb_intel_hdmi.h"
> > 
> > This is bad...  If these files are really used over several
> > subsystems, they should be put somewhere in a better public place.
> >
> 
> Currently display driver code is in the staging area. Can we move
> header files to include folder?

I think yes.  If not, you can create a include directory in your
staging driver path, then set $(EXTRA_CFLAGS) to point there in
Makefile of the hdmi audio driver.


> > > +/**
> > > + * union aud_cfg - Audio configuration offset - 69000
> > > + *
> > > + * @cfg_regx: individual register bits
> > > + * @cfg_regval: full register value
> > > + *
> > > + * */
> > > +union aud_cfg {
> > > +	struct {
> > > +		u32 aud_en:1;
> > > +		u32 layout:1;
> > > +		u32 fmt:2;
> > > +		u32 num_ch:2;
> > > +		u32 rsvd0:1;
> > > +		u32 set:1;
> > > +		u32 flat:1;
> > > +		u32 val_bit:1;
> > > +		u32 user_bit:1;
> > > +		u32 underrun:1;
> > > +		u32 rsvd1:20;
> > > +	} cfg_regx;
> > > +	u32 cfg_regval;
> > > +};
> > 
> > In general, better to avoid bit fields if it's used for the data
> > transfer.  The bit-fields are never portable.
> > 
> These bit fields are used for programming the HDMI controller subsystem.

So, these bit-fields data aren't exposed to the hardware as is, but
used only between the hdmi audio and the controller drivers?
(But then, why these have to be defined in here?)


Takashi

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/5] [RFC]intel_hdmi_audio: driver module - ALSA intereaction
  2010-11-25  5:40     ` Bandarupalli, Sailaja
@ 2010-11-25  5:50       ` Takashi Iwai
  2010-11-25  6:17         ` Bandarupalli, Sailaja
  0 siblings, 1 reply; 22+ messages in thread
From: Takashi Iwai @ 2010-11-25  5:50 UTC (permalink / raw)
  To: Bandarupalli, Sailaja; +Cc: Wu, Fengguang, alsa-devel, Mark Brown, Babu, Ramesh

At Thu, 25 Nov 2010 11:10:53 +0530,
Bandarupalli, Sailaja wrote:
> 
> > > +
> > > +/* hardware capability structure */
> > > +static const struct snd_pcm_hardware snd_intel_hadstream = {
> > > +	.info =	(SNDRV_PCM_INFO_INTERLEAVED |
> > > +		SNDRV_PCM_INFO_DOUBLE |
> > > +		SNDRV_PCM_INFO_PAUSE |
> > > +		SNDRV_PCM_INFO_RESUME |
> > 
> > You have to implement corresponding stuff if you mark the driver as
> > RESUME-able.
> 
> Yes we have implemented SNDRV_PCM_TRIGGER_PAUSE_RELEASE

PAUSE != RESUME.
For implementing the PM resume, you need more works.

Also I don't remember whether you implemented PM suspend/resume
callbacks, did you...?


Takashi

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/5] [RFC]intel_hdmi_audio: interface module
  2010-11-24  9:05     ` Takashi Iwai
  2010-11-24 10:11       ` Mark Brown
@ 2010-11-25  5:54       ` Bandarupalli, Sailaja
  2010-12-01  9:55         ` Babu, Ramesh
  1 sibling, 1 reply; 22+ messages in thread
From: Bandarupalli, Sailaja @ 2010-11-25  5:54 UTC (permalink / raw)
  To: Takashi Iwai, Mark Brown, alsa-devel; +Cc: Wu, Fengguang, Babu, Ramesh

 
> > > +/**
> > > + * had_hot_plug - to create sound card instance for HDMI audio
> playabck
> > > + *
> > > + *
> > > + * This function is called when the hdmi cable is plugged in. This
> function
> > > + * creates and registers the sound card with ALSA
> > > + */
> > > +static int had_hot_plug(void)
> > > +{
> >
> > It would seem more natural for the HDMI device to be created and
> > instatiated via the normal Linux device model - when a HDMI
> connection
> > is detected the thing doing the detection creates and registers a new
> > device of some kind which the driver core then binds to a separate
> > driver.
> 
> Yeah, the driver structure here is somehow wacky, although it's not
> to hard to trace.
> 
> 

Agree, we will work on this and revert back to you.

Thanks,
Sailaja & Ramesh.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/5] [RFC]intel_hdmi_audio:include header
  2010-11-25  5:46     ` Takashi Iwai
@ 2010-11-25  6:07       ` Bandarupalli, Sailaja
  2010-11-25  6:25         ` Takashi Iwai
  0 siblings, 1 reply; 22+ messages in thread
From: Bandarupalli, Sailaja @ 2010-11-25  6:07 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: Wu, Fengguang, alsa-devel, Mark Brown, Babu, Ramesh


> > > > +/**
> > > > + * union aud_cfg - Audio configuration offset - 69000
> > > > + *
> > > > + * @cfg_regx: individual register bits
> > > > + * @cfg_regval: full register value
> > > > + *
> > > > + * */
> > > > +union aud_cfg {
> > > > +	struct {
> > > > +		u32 aud_en:1;
> > > > +		u32 layout:1;
> > > > +		u32 fmt:2;
> > > > +		u32 num_ch:2;
> > > > +		u32 rsvd0:1;
> > > > +		u32 set:1;
> > > > +		u32 flat:1;
> > > > +		u32 val_bit:1;
> > > > +		u32 user_bit:1;
> > > > +		u32 underrun:1;
> > > > +		u32 rsvd1:20;
> > > > +	} cfg_regx;
> > > > +	u32 cfg_regval;
> > > > +};
> > >
> > > In general, better to avoid bit fields if it's used for the data
> > > transfer.  The bit-fields are never portable.
> > >
> > These bit fields are used for programming the HDMI controller
> subsystem.
> 
> So, these bit-fields data aren't exposed to the hardware as is, but
> used only between the hdmi audio and the controller drivers?
> (But then, why these have to be defined in here?)
> 
> 
These bits  fields are used between HDMI audio driver and hardware.
They are exposed by the hardware as is, and we need to write to 
specific bits to enable HDMI audio.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/5] [RFC]intel_hdmi_audio: driver module - ALSA intereaction
  2010-11-25  5:50       ` Takashi Iwai
@ 2010-11-25  6:17         ` Bandarupalli, Sailaja
  0 siblings, 0 replies; 22+ messages in thread
From: Bandarupalli, Sailaja @ 2010-11-25  6:17 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: alsa-devel, Wu, Fengguang, Babu, Ramesh, Mark Brown

> > > > +
> > > > +/* hardware capability structure */
> > > > +static const struct snd_pcm_hardware snd_intel_hadstream = {
> > > > +	.info =	(SNDRV_PCM_INFO_INTERLEAVED |
> > > > +		SNDRV_PCM_INFO_DOUBLE |
> > > > +		SNDRV_PCM_INFO_PAUSE |
> > > > +		SNDRV_PCM_INFO_RESUME |
> > >
> > > You have to implement corresponding stuff if you mark the driver as
> > > RESUME-able.
> >
> > Yes we have implemented SNDRV_PCM_TRIGGER_PAUSE_RELEASE
> 
> PAUSE != RESUME.
> For implementing the PM resume, you need more works.
> 
> Also I don't remember whether you implemented PM suspend/resume
> callbacks, did you...?

We will remove SNDRV_PCM_INFO_RESUME capability as the driver is 
not resume-able.
RFC patches didn't have PM suspend/resume.  However we will 
implement platform PM functionality before submitting final patches.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/5] [RFC]intel_hdmi_audio:include header
  2010-11-25  6:07       ` Bandarupalli, Sailaja
@ 2010-11-25  6:25         ` Takashi Iwai
  0 siblings, 0 replies; 22+ messages in thread
From: Takashi Iwai @ 2010-11-25  6:25 UTC (permalink / raw)
  To: Bandarupalli, Sailaja; +Cc: Wu, Fengguang, alsa-devel, Mark Brown, Babu, Ramesh

At Thu, 25 Nov 2010 11:37:58 +0530,
Bandarupalli, Sailaja wrote:
> 
> 
> > > > > +/**
> > > > > + * union aud_cfg - Audio configuration offset - 69000
> > > > > + *
> > > > > + * @cfg_regx: individual register bits
> > > > > + * @cfg_regval: full register value
> > > > > + *
> > > > > + * */
> > > > > +union aud_cfg {
> > > > > +	struct {
> > > > > +		u32 aud_en:1;
> > > > > +		u32 layout:1;
> > > > > +		u32 fmt:2;
> > > > > +		u32 num_ch:2;
> > > > > +		u32 rsvd0:1;
> > > > > +		u32 set:1;
> > > > > +		u32 flat:1;
> > > > > +		u32 val_bit:1;
> > > > > +		u32 user_bit:1;
> > > > > +		u32 underrun:1;
> > > > > +		u32 rsvd1:20;
> > > > > +	} cfg_regx;
> > > > > +	u32 cfg_regval;
> > > > > +};
> > > >
> > > > In general, better to avoid bit fields if it's used for the data
> > > > transfer.  The bit-fields are never portable.
> > > >
> > > These bit fields are used for programming the HDMI controller
> > subsystem.
> > 
> > So, these bit-fields data aren't exposed to the hardware as is, but
> > used only between the hdmi audio and the controller drivers?
> > (But then, why these have to be defined in here?)
> > 
> > 
> These bits  fields are used between HDMI audio driver and hardware.
> They are exposed by the hardware as is, and we need to write to 
> specific bits to enable HDMI audio.

Then the bit-fields aren't suitable regarding the portability.


Takashi

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/5] [RFC]intel_hdmi_audio: interface module
  2010-11-25  5:54       ` Bandarupalli, Sailaja
@ 2010-12-01  9:55         ` Babu, Ramesh
  0 siblings, 0 replies; 22+ messages in thread
From: Babu, Ramesh @ 2010-12-01  9:55 UTC (permalink / raw)
  To: Takashi Iwai, Mark Brown, alsa-devel; +Cc: Wu, Fengguang, Bandarupalli, Sailaja

> > > > + * had_hot_plug - to create sound card instance for HDMI audio
> > playabck
> > > > + *
> > > > + *
> > > > + * This function is called when the hdmi cable is plugged in. This
> > function
> > > > + * creates and registers the sound card with ALSA
> > > > + */
> > > > +static int had_hot_plug(void)
> > > > +{
> > >
> > > It would seem more natural for the HDMI device to be created and
> > > instatiated via the normal Linux device model - when a HDMI
> > connection
> > > is detected the thing doing the detection creates and registers a new
> > > device of some kind which the driver core then binds to a separate
> > > driver.
> >
> > Yeah, the driver structure here is somehow wacky, although it's not
> > to hard to trace.
> >
> >
> 
> Agree, we will work on this and revert back to you.
> 

We are planning to follow the USB audio driver model.  During _init
audio driver callbacks will be registered with display driver. Whenever
device is hot plugged, .probe will be invoked and sound card is created.
During un-plug, .disconnect will be invoked and sound card is removed.
Similarly .suspend and .resume will be called when the display device
getting suspended and resumed. Please provide your suggestions on this
driver model.

static struct hdmi_driver hdmi_audio_driver = {
    .probe =    hdmi_audio_probe,
    .disconnect =   hdmi_audio_disconnect,
    .suspend =  hdmi_audio_suspend,
    .resume =   hdmi_audio_resume,
 };
 
 static int __init alsa_card_intelhad_init(void)
 {
 	return hdmi_register(&hdmi_audio_driver);
 }

Thanks,
Ramesh & Sailaja

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2010-12-01  9:57 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-22 13:39 [PATCH 1/5] [RFC]intel_hdmi_audio:include header ramesh.babu
2010-11-22 13:39 ` [PATCH 2/5] [RFC]intel_hdmi_audio: interface module ramesh.babu
2010-11-22 13:58   ` Mark Brown
2010-11-24  9:05     ` Takashi Iwai
2010-11-24 10:11       ` Mark Brown
2010-11-24 14:20         ` Takashi Iwai
2010-11-25  5:54       ` Bandarupalli, Sailaja
2010-12-01  9:55         ` Babu, Ramesh
2010-11-22 13:39 ` [PATCH 3/5] [RFC]intel_hdmi_audio: driver module - ALSA intereaction ramesh.babu
2010-11-24  9:16   ` Takashi Iwai
2010-11-25  5:40     ` Bandarupalli, Sailaja
2010-11-25  5:50       ` Takashi Iwai
2010-11-25  6:17         ` Bandarupalli, Sailaja
2010-11-22 13:39 ` [PATCH 4/5] [RFC]intel_hdmi_audio:driver module-hdmi hw interface ramesh.babu
2010-11-22 13:39 ` [PATCH 5/5] [RFC]intel_hdmi_audio: Kconfig and Makefile ramesh.babu
2010-11-22 13:48 ` [PATCH 1/5] [RFC]intel_hdmi_audio:include header Mark Brown
2010-11-25  4:59   ` Bandarupalli, Sailaja
2010-11-24  9:20 ` Takashi Iwai
2010-11-25  5:06   ` Bandarupalli, Sailaja
2010-11-25  5:46     ` Takashi Iwai
2010-11-25  6:07       ` Bandarupalli, Sailaja
2010-11-25  6:25         ` Takashi Iwai

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.