[PATCH 00/10] OMAPDSS: HDMI: Prepare for OMAP5 and DSS dev driver audio support

[PATCH 00/10] OMAPDSS: HDMI: Prepare for OMAP5 and DSS dev driver audio support

[Ricardo Neri]

Hi,

This set of patches is inteded to prepare the HDMI driver to implement the DSS
device driver interface for audio proposed here:
http://www.spinics.net/lists/linux-omap/msg67303.html

In preparation for that, it removes the current ASoC HDMI codec driver and
decouples HDMI audio build configuration from ASoC. Instead, a config option
may be selected by the parties interested in the HDMI audio functionality. The
last patch effectively implements the DSS audio interface.

Also, this set prepares the HDMI driver for the introduction of the OMAP5 HDMI
audio functionality by further abstracting the portions of code that are
generic to all HDMI implementations (e.g, N/CTS params calculation). Also, an
IP-dependent audio configuration function is introduced as an HDMI IP operation;
this function takes IP-independent parameters and is intended to be implemented
for each individual IP.

For the specific case of OMAP4, the configuration of the IEC-60958 channel
status word is expanded to provide more flexibility. Also, some duplicated
IEC-60958 definitions are removed to, instead, reuse the definitions provided
in asound.h The CEA-861 definitions are not yet added to asound.h. I will
send a patch for that to alsa-devel.

The changes for OMAP4 configuration expand the current support to cover more
audio sample rates: 32, 44.1, 48, 88.2, 176.4 and 192 kHz. Audio sample world
length of 16 through 24 bits as well as up to 8 audio channels.

These changes are based on the 3.3 Linux kernel plus the patches for the
audio MCLK selection (http://www.spinics.net/lists/linux-omap/msg64302.html).

Validation was performed using Onkyo TX-SR508 and Yamaha RX-V367 AV receivers.

BR,

Ricardo

Ricardo Neri (10):
  OMAPDSS: HDMI: Remove ASoC codec
  OMAPDSS: HDMI: OMAP4: Remove CEA-861 audio infoframe and IEC-60958
    enums
  OMAPDSS: HDMI: OMAP4: Correcty typo in I2S definitions
  OMAPDSS: HDMI: OMAP4: Decouple wrapper enable and audio start
  OMAPDSS: HDMI: Decouple HDMI audio from ASoC
  OMAPDSS: HDMI: OMAP4: Expand configuration for IEC-60958 audio
  OMAPDSS: HDMI: Relocate N/CTS calculation
  OMAPDSS: HDMI: Add support for more audio sample rates in N/CTS
    calculation
  OMAPDSS: HDMI: OMAP4: Add an audio configuration function
  OMAPDSS: HDMI: Implement DSS driver interface for audio

 drivers/video/omap2/dss/Kconfig           |    4 +
 drivers/video/omap2/dss/dss.h             |    7 +
 drivers/video/omap2/dss/dss_features.c    |    5 +-
 drivers/video/omap2/dss/hdmi.c            |  339 ++++++++++-------------------
 drivers/video/omap2/dss/hdmi_panel.c      |   76 +++++++
 drivers/video/omap2/dss/ti_hdmi.h         |   17 +-
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |  304 +++++++++++++++++++++-----
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h |  116 ++--------
 8 files changed, 488 insertions(+), 380 deletions(-)

[PATCH 01/10] OMAPDSS: HDMI: Remove ASoC codec

[Ricardo Neri]

Instead of having an ASoC codec embedded into DSS code, use the generic DSS
device driverinterface for audio support. This allows to any potential user,
including an ASoC driver, take advantage of the HDMI audio functionality.

Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
---
 drivers/video/omap2/dss/hdmi.c |  236 ----------------------------------------
 1 files changed, 0 insertions(+), 236 deletions(-)

diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 9c12337..27d4fba 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -33,12 +33,6 @@
 #include <linux/pm_runtime.h>
 #include <linux/clk.h>
 #include <video/omapdss.h>
-#if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
-	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
-#include <sound/soc.h>
-#include <sound/pcm_params.h>
-#include "ti_hdmi_4xxx_ip.h"
-#endif
 
 #include "ti_hdmi.h"
 #include "dss.h"
@@ -583,220 +577,6 @@ void omapdss_hdmi_display_disable(struct omap_dss_device *dssdev)
 	mutex_unlock(&hdmi.lock);
 }
 
-#if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
-	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
-
-static int hdmi_audio_trigger(struct snd_pcm_substream *substream, int cmd,
-				struct snd_soc_dai *dai)
-{
-	struct snd_soc_pcm_runtime *rtd = substream->private_data;
-	struct snd_soc_codec *codec = rtd->codec;
-	struct platform_device *pdev = to_platform_device(codec->dev);
-	struct hdmi_ip_data *ip_data = snd_soc_codec_get_drvdata(codec);
-	int err = 0;
-
-	if (!(ip_data->ops) && !(ip_data->ops->audio_enable)) {
-		dev_err(&pdev->dev, "Cannot enable/disable audio\n");
-		return -ENODEV;
-	}
-
-	switch (cmd) {
-	case SNDRV_PCM_TRIGGER_START:
-	case SNDRV_PCM_TRIGGER_RESUME:
-	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		ip_data->ops->audio_enable(ip_data, true);
-		break;
-	case SNDRV_PCM_TRIGGER_STOP:
-	case SNDRV_PCM_TRIGGER_SUSPEND:
-	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		ip_data->ops->audio_enable(ip_data, false);
-		break;
-	default:
-		err = -EINVAL;
-	}
-	return err;
-}
-
-static int hdmi_audio_hw_params(struct snd_pcm_substream *substream,
-				    struct snd_pcm_hw_params *params,
-				    struct snd_soc_dai *dai)
-{
-	struct snd_soc_pcm_runtime *rtd = substream->private_data;
-	struct snd_soc_codec *codec = rtd->codec;
-	struct hdmi_ip_data *ip_data = snd_soc_codec_get_drvdata(codec);
-	struct hdmi_audio_format audio_format;
-	struct hdmi_audio_dma audio_dma;
-	struct hdmi_core_audio_config core_cfg;
-	struct hdmi_core_infoframe_audio aud_if_cfg;
-	int err, n, cts;
-	enum hdmi_core_audio_sample_freq sample_freq;
-
-	switch (params_format(params)) {
-	case SNDRV_PCM_FORMAT_S16_LE:
-		core_cfg.i2s_cfg.word_max_length =
-			HDMI_AUDIO_I2S_MAX_WORD_20BITS;
-		core_cfg.i2s_cfg.word_length = HDMI_AUDIO_I2S_CHST_WORD_16_BITS;
-		core_cfg.i2s_cfg.in_length_bits =
-			HDMI_AUDIO_I2S_INPUT_LENGTH_16;
-		core_cfg.i2s_cfg.justification = HDMI_AUDIO_JUSTIFY_LEFT;
-		audio_format.samples_per_word = HDMI_AUDIO_ONEWORD_TWOSAMPLES;
-		audio_format.sample_size = HDMI_AUDIO_SAMPLE_16BITS;
-		audio_format.justification = HDMI_AUDIO_JUSTIFY_LEFT;
-		audio_dma.transfer_size = 0x10;
-		break;
-	case SNDRV_PCM_FORMAT_S24_LE:
-		core_cfg.i2s_cfg.word_max_length =
-			HDMI_AUDIO_I2S_MAX_WORD_24BITS;
-		core_cfg.i2s_cfg.word_length = HDMI_AUDIO_I2S_CHST_WORD_24_BITS;
-		core_cfg.i2s_cfg.in_length_bits =
-			HDMI_AUDIO_I2S_INPUT_LENGTH_24;
-		audio_format.samples_per_word = HDMI_AUDIO_ONEWORD_ONESAMPLE;
-		audio_format.sample_size = HDMI_AUDIO_SAMPLE_24BITS;
-		audio_format.justification = HDMI_AUDIO_JUSTIFY_RIGHT;
-		core_cfg.i2s_cfg.justification = HDMI_AUDIO_JUSTIFY_RIGHT;
-		audio_dma.transfer_size = 0x20;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	switch (params_rate(params)) {
-	case 32000:
-		sample_freq = HDMI_AUDIO_FS_32000;
-		break;
-	case 44100:
-		sample_freq = HDMI_AUDIO_FS_44100;
-		break;
-	case 48000:
-		sample_freq = HDMI_AUDIO_FS_48000;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	err = hdmi_config_audio_acr(ip_data, params_rate(params), &n, &cts);
-	if (err < 0)
-		return err;
-
-	/* Audio wrapper config */
-	audio_format.stereo_channels = HDMI_AUDIO_STEREO_ONECHANNEL;
-	audio_format.active_chnnls_msk = 0x03;
-	audio_format.type = HDMI_AUDIO_TYPE_LPCM;
-	audio_format.sample_order = HDMI_AUDIO_SAMPLE_LEFT_FIRST;
-	/* Disable start/stop signals of IEC 60958 blocks */
-	audio_format.en_sig_blk_strt_end = HDMI_AUDIO_BLOCK_SIG_STARTEND_OFF;
-
-	audio_dma.block_size = 0xC0;
-	audio_dma.mode = HDMI_AUDIO_TRANSF_DMA;
-	audio_dma.fifo_threshold = 0x20; /* in number of samples */
-
-	hdmi_wp_audio_config_dma(ip_data, &audio_dma);
-	hdmi_wp_audio_config_format(ip_data, &audio_format);
-
-	/*
-	 * I2S config
-	 */
-	core_cfg.i2s_cfg.en_high_bitrate_aud = false;
-	/* Only used with high bitrate audio */
-	core_cfg.i2s_cfg.cbit_order = false;
-	/* Serial data and word select should change on sck rising edge */
-	core_cfg.i2s_cfg.sck_edge_mode = HDMI_AUDIO_I2S_SCK_EDGE_RISING;
-	core_cfg.i2s_cfg.vbit = HDMI_AUDIO_I2S_VBIT_FOR_PCM;
-	/* Set I2S word select polarity */
-	core_cfg.i2s_cfg.ws_polarity = HDMI_AUDIO_I2S_WS_POLARITY_LOW_IS_LEFT;
-	core_cfg.i2s_cfg.direction = HDMI_AUDIO_I2S_MSB_SHIFTED_FIRST;
-	/* Set serial data to word select shift. See Phillips spec. */
-	core_cfg.i2s_cfg.shift = HDMI_AUDIO_I2S_FIRST_BIT_SHIFT;
-	/* Enable one of the four available serial data channels */
-	core_cfg.i2s_cfg.active_sds = HDMI_AUDIO_I2S_SD0_EN;
-
-	/* Core audio config */
-	core_cfg.freq_sample = sample_freq;
-	core_cfg.n = n;
-	core_cfg.cts = cts;
-	if (dss_has_feature(FEAT_HDMI_CTS_SWMODE)) {
-		core_cfg.aud_par_busclk = 0;
-		core_cfg.cts_mode = HDMI_AUDIO_CTS_MODE_SW;
-		core_cfg.use_mclk = dss_has_feature(FEAT_HDMI_AUDIO_USE_MCLK);
-	} else {
-		core_cfg.aud_par_busclk = (((128 * 31) - 1) << 8);
-		core_cfg.cts_mode = HDMI_AUDIO_CTS_MODE_HW;
-		core_cfg.use_mclk = true;
-	}
-
-	if (core_cfg.use_mclk)
-		core_cfg.mclk_mode = HDMI_AUDIO_MCLK_128FS;
-	core_cfg.layout = HDMI_AUDIO_LAYOUT_2CH;
-	core_cfg.en_spdif = false;
-	/* Use sample frequency from channel status word */
-	core_cfg.fs_override = true;
-	/* Enable ACR packets */
-	core_cfg.en_acr_pkt = true;
-	/* Disable direct streaming digital audio */
-	core_cfg.en_dsd_audio = false;
-	/* Use parallel audio interface */
-	core_cfg.en_parallel_aud_input = true;
-
-	hdmi_core_audio_config(ip_data, &core_cfg);
-
-	/*
-	 * Configure packet
-	 * info frame audio see doc CEA861-D page 74
-	 */
-	aud_if_cfg.db1_coding_type = HDMI_INFOFRAME_AUDIO_DB1CT_FROM_STREAM;
-	aud_if_cfg.db1_channel_count = 2;
-	aud_if_cfg.db2_sample_freq = HDMI_INFOFRAME_AUDIO_DB2SF_FROM_STREAM;
-	aud_if_cfg.db2_sample_size = HDMI_INFOFRAME_AUDIO_DB2SS_FROM_STREAM;
-	aud_if_cfg.db4_channel_alloc = 0x00;
-	aud_if_cfg.db5_downmix_inh = false;
-	aud_if_cfg.db5_lsv = 0;
-
-	hdmi_core_audio_infoframe_config(ip_data, &aud_if_cfg);
-	return 0;
-}
-
-static int hdmi_audio_startup(struct snd_pcm_substream *substream,
-				  struct snd_soc_dai *dai)
-{
-	if (hdmi.mode != HDMI_HDMI) {
-		pr_err("Current video settings do not support audio.\n");
-		return -EIO;
-	}
-	return 0;
-}
-
-static int hdmi_audio_codec_probe(struct snd_soc_codec *codec)
-{
-	struct hdmi_ip_data *priv = &hdmi.ip_data;
-
-	snd_soc_codec_set_drvdata(codec, priv);
-	return 0;
-}
-
-static struct snd_soc_codec_driver hdmi_audio_codec_drv = {
-	.probe = hdmi_audio_codec_probe,
-};
-
-static struct snd_soc_dai_ops hdmi_audio_codec_ops = {
-	.hw_params = hdmi_audio_hw_params,
-	.trigger = hdmi_audio_trigger,
-	.startup = hdmi_audio_startup,
-};
-
-static struct snd_soc_dai_driver hdmi_codec_dai_drv = {
-		.name = "hdmi-audio-codec",
-		.playback = {
-			.channels_min = 2,
-			.channels_max = 2,
-			.rates = SNDRV_PCM_RATE_32000 |
-				SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000,
-			.formats = SNDRV_PCM_FMTBIT_S16_LE |
-				SNDRV_PCM_FMTBIT_S24_LE,
-		},
-		.ops = &hdmi_audio_codec_ops,
-};
-#endif
-
 static int hdmi_get_clocks(struct platform_device *pdev)
 {
 	struct clk *clk;
@@ -858,17 +638,6 @@ static int omapdss_hdmihw_probe(struct platform_device *pdev)
 
 	hdmi_panel_init();
 
-#if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
-	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
-
-	/* Register ASoC codec DAI */
-	r = snd_soc_register_codec(&pdev->dev, &hdmi_audio_codec_drv,
-					&hdmi_codec_dai_drv, 1);
-	if (r) {
-		DSSERR("can't register ASoC HDMI audio codec\n");
-		return r;
-	}
-#endif
 	return 0;
 }
 
@@ -876,11 +645,6 @@ static int omapdss_hdmihw_remove(struct platform_device *pdev)
 {
 	hdmi_panel_exit();
 
-#if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
-	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
-	snd_soc_unregister_codec(&pdev->dev);
-#endif
-
 	pm_runtime_disable(&pdev->dev);
 
 	hdmi_put_clocks();
-- 
1.7.0.4

Re: [PATCH 01/10] OMAPDSS: HDMI: Remove ASoC codec

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]

On Wed, 2012-03-28 at 16:38 -0600, Ricardo Neri wrote:
> Instead of having an ASoC codec embedded into DSS code, use the generic DSS
> device driverinterface for audio support. This allows to any potential user,
> including an ASoC driver, take advantage of the HDMI audio functionality.

The description could be improved. The patch removes lots of code from
the hdmi driver, but the description doesn't really describe what's
going on and where the code goes (if anywhere). In fact, it even feels
that the description is about something else. It basically just says
"use generic DSS interface for audio", but the patch doesn't do that, it
removes code.

Also how does this affect the current driver? I presume it effectively
removes HDMI audio support? Does the code still compile with this patch?

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 01/10] OMAPDSS: HDMI: Remove ASoC codec

[Ricardo Neri]

Hi Tomi,

Thanks for your comments!

On 04/23/2012 08:17 AM, Tomi Valkeinen wrote:
> On Wed, 2012-03-28 at 16:38 -0600, Ricardo Neri wrote:
>> Instead of having an ASoC codec embedded into DSS code, use the generic DSS
>> device driverinterface for audio support. This allows to any potential user,
>> including an ASoC driver, take advantage of the HDMI audio functionality.
>
> The description could be improved. The patch removes lots of code from
> the hdmi driver, but the description doesn't really describe what's
> going on and where the code goes (if anywhere). In fact, it even feels
> that the description is about something else. It basically just says
> "use generic DSS interface for audio", but the patch doesn't do that, it
> removes code.

Yes. It seems that the description is more suitable for the whole patch 
series. I was trying to justify why I was removing the ASoC HDMI codec. 
I will rephrase to state that this patch removes the HDMI audio support 
to restore in a separate patch through the DSS audio interface.
>
> Also how does this affect the current driver? I presume it effectively
> removes HDMI audio support? Does the code still compile with this patch?

Yes, the code still compiles as the current ASoC HDMI codec exists with 
independences of the ASoC HDMI DAI and machine drivers. It will compile 
but it will not probe and ALSA HDMI audio support will be missing.

BR,

Ricardo
>
>   Tomi

[PATCH 02/10] OMAPDSS: HDMI: OMAP4: Remove CEA-861 audio infoframe and IEC-60958 enums

[Ricardo Neri]

In order to avoid duplication of definitions. Use the definitions provided
by asoundef.h. Furthermore, as CEA-861 and IEC-60958 are used by both
DisplayPort and HDMI, this helps to make the code more generic.

Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
---
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |   15 +++---
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h |   80 ++---------------------------
 2 files changed, 12 insertions(+), 83 deletions(-)

diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
index 4740e64..4ab3b19 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -29,7 +29,6 @@
 #include <linux/string.h>
 #include <linux/seq_file.h>
 #include <linux/gpio.h>
-
 #include "ti_hdmi_4xxx_ip.h"
 #include "dss.h"
 
@@ -1156,7 +1155,7 @@ void hdmi_core_audio_config(struct hdmi_ip_data *ip_data,
 }
 
 void hdmi_core_audio_infoframe_config(struct hdmi_ip_data *ip_data,
-		struct hdmi_core_infoframe_audio *info_aud)
+		struct snd_cea_861_aud_if *info_aud)
 {
 	u8 val;
 	u8 sum = 0, checksum = 0;
@@ -1172,22 +1171,24 @@ void hdmi_core_audio_infoframe_config(struct hdmi_ip_data *ip_data,
 	hdmi_write_reg(av_base, HDMI_CORE_AV_AUDIO_LEN, 0x0a);
 	sum += 0x84 + 0x001 + 0x00a;
 
-	val = (info_aud->db1_coding_type << 4)
-			| (info_aud->db1_channel_count - 1);
+	val = (info_aud->coding_type << CEA861_AUDIO_INFOFRAME_DB1CT_SHIFT)
+			| (info_aud->channel_count - 1);
 	hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(0), val);
 	sum += val;
 
-	val = (info_aud->db2_sample_freq << 2) | info_aud->db2_sample_size;
+	val = (info_aud->sample_freq << CEA861_AUDIO_INFOFRAME_DB2SF_SHIFT)
+			| (info_aud->sample_size);
 	hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(1), val);
 	sum += val;
 
 	hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(2), 0x00);
 
-	val = info_aud->db4_channel_alloc;
+	val = info_aud->channel_alloc;
 	hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(3), val);
 	sum += val;
 
-	val = (info_aud->db5_downmix_inh << 7) | (info_aud->db5_lsv << 3);
+	val = (info_aud->st_downmix << CEA861_AUDIO_INFOFRAME_DB5_DM_INH_SHIFT)
+		| (info_aud->lsv << CEA861_AUDIO_INFOFRAME_DB5_LSV_SHIFT);
 	hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(4), val);
 	sum += val;
 
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
index a442998..d6b49b6 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
@@ -28,6 +28,8 @@
 	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
 #include <sound/soc.h>
 #include <sound/pcm_params.h>
+#include <sound/asound.h>
+#include <sound/asoundef.h>
 #endif
 
 /* HDMI Wrapper */
@@ -284,35 +286,6 @@ enum hdmi_core_infoframe {
 	HDMI_INFOFRAME_AVI_DB5PR_8 = 7,
 	HDMI_INFOFRAME_AVI_DB5PR_9 = 8,
 	HDMI_INFOFRAME_AVI_DB5PR_10 = 9,
-	HDMI_INFOFRAME_AUDIO_DB1CT_FROM_STREAM = 0,
-	HDMI_INFOFRAME_AUDIO_DB1CT_IEC60958 = 1,
-	HDMI_INFOFRAME_AUDIO_DB1CT_AC3 = 2,
-	HDMI_INFOFRAME_AUDIO_DB1CT_MPEG1 = 3,
-	HDMI_INFOFRAME_AUDIO_DB1CT_MP3 = 4,
-	HDMI_INFOFRAME_AUDIO_DB1CT_MPEG2_MULTICH = 5,
-	HDMI_INFOFRAME_AUDIO_DB1CT_AAC = 6,
-	HDMI_INFOFRAME_AUDIO_DB1CT_DTS = 7,
-	HDMI_INFOFRAME_AUDIO_DB1CT_ATRAC = 8,
-	HDMI_INFOFRAME_AUDIO_DB1CT_ONEBIT = 9,
-	HDMI_INFOFRAME_AUDIO_DB1CT_DOLBY_DIGITAL_PLUS = 10,
-	HDMI_INFOFRAME_AUDIO_DB1CT_DTS_HD = 11,
-	HDMI_INFOFRAME_AUDIO_DB1CT_MAT = 12,
-	HDMI_INFOFRAME_AUDIO_DB1CT_DST = 13,
-	HDMI_INFOFRAME_AUDIO_DB1CT_WMA_PRO = 14,
-	HDMI_INFOFRAME_AUDIO_DB2SF_FROM_STREAM = 0,
-	HDMI_INFOFRAME_AUDIO_DB2SF_32000 = 1,
-	HDMI_INFOFRAME_AUDIO_DB2SF_44100 = 2,
-	HDMI_INFOFRAME_AUDIO_DB2SF_48000 = 3,
-	HDMI_INFOFRAME_AUDIO_DB2SF_88200 = 4,
-	HDMI_INFOFRAME_AUDIO_DB2SF_96000 = 5,
-	HDMI_INFOFRAME_AUDIO_DB2SF_176400 = 6,
-	HDMI_INFOFRAME_AUDIO_DB2SF_192000 = 7,
-	HDMI_INFOFRAME_AUDIO_DB2SS_FROM_STREAM = 0,
-	HDMI_INFOFRAME_AUDIO_DB2SS_16BIT = 1,
-	HDMI_INFOFRAME_AUDIO_DB2SS_20BIT = 2,
-	HDMI_INFOFRAME_AUDIO_DB2SS_24BIT = 3,
-	HDMI_INFOFRAME_AUDIO_DB5_DM_INH_PERMITTED = 0,
-	HDMI_INFOFRAME_AUDIO_DB5_DM_INH_PROHIBITED = 1
 };
 
 enum hdmi_packing_mode {
@@ -322,17 +295,6 @@ enum hdmi_packing_mode {
 	HDMI_PACK_ALREADYPACKED = 7
 };
 
-enum hdmi_core_audio_sample_freq {
-	HDMI_AUDIO_FS_32000 = 0x3,
-	HDMI_AUDIO_FS_44100 = 0x0,
-	HDMI_AUDIO_FS_48000 = 0x2,
-	HDMI_AUDIO_FS_88200 = 0x8,
-	HDMI_AUDIO_FS_96000 = 0xA,
-	HDMI_AUDIO_FS_176400 = 0xC,
-	HDMI_AUDIO_FS_192000 = 0xE,
-	HDMI_AUDIO_FS_NOT_INDICATED = 0x1
-};
-
 enum hdmi_core_audio_layout {
 	HDMI_AUDIO_LAYOUT_2CH = 0,
 	HDMI_AUDIO_LAYOUT_8CH = 1
@@ -393,31 +355,10 @@ enum hdmi_audio_i2s_config {
 	HDMI_AUDIO_I2S_LSB_SHIFTED_FIRST = 1,
 	HDMI_AUDIO_I2S_MAX_WORD_20BITS = 0,
 	HDMI_AUDIO_I2S_MAX_WORD_24BITS = 1,
-	HDMI_AUDIO_I2S_CHST_WORD_NOT_SPECIFIED = 0,
-	HDMI_AUDIO_I2S_CHST_WORD_16_BITS = 1,
-	HDMI_AUDIO_I2S_CHST_WORD_17_BITS = 6,
-	HDMI_AUDIO_I2S_CHST_WORD_18_BITS = 2,
-	HDMI_AUDIO_I2S_CHST_WORD_19_BITS = 4,
-	HDMI_AUDIO_I2S_CHST_WORD_20_BITS_20MAX = 5,
-	HDMI_AUDIO_I2S_CHST_WORD_20_BITS_24MAX = 1,
-	HDMI_AUDIO_I2S_CHST_WORD_21_BITS = 6,
-	HDMI_AUDIO_I2S_CHST_WORD_22_BITS = 2,
-	HDMI_AUDIO_I2S_CHST_WORD_23_BITS = 4,
-	HDMI_AUDIO_I2S_CHST_WORD_24_BITS = 5,
 	HDMI_AUDIO_I2S_SCK_EDGE_FALLING = 0,
 	HDMI_AUDIO_I2S_SCK_EDGE_RISING = 1,
 	HDMI_AUDIO_I2S_VBIT_FOR_PCM = 0,
 	HDMI_AUDIO_I2S_VBIT_FOR_COMPRESSED = 1,
-	HDMI_AUDIO_I2S_INPUT_LENGTH_NA = 0,
-	HDMI_AUDIO_I2S_INPUT_LENGTH_16 = 2,
-	HDMI_AUDIO_I2S_INPUT_LENGTH_17 = 12,
-	HDMI_AUDIO_I2S_INPUT_LENGTH_18 = 4,
-	HDMI_AUDIO_I2S_INPUT_LENGTH_19 = 8,
-	HDMI_AUDIO_I2S_INPUT_LENGTH_20 = 10,
-	HDMI_AUDIO_I2S_INPUT_LENGTH_21 = 13,
-	HDMI_AUDIO_I2S_INPUT_LENGTH_22 = 5,
-	HDMI_AUDIO_I2S_INPUT_LENGTH_23 = 9,
-	HDMI_AUDIO_I2S_INPUT_LENGTH_24 = 11,
 	HDMI_AUDIO_I2S_FIRST_BIT_SHIFT = 0,
 	HDMI_AUDIO_I2S_FIRST_BIT_NO_SHIFT = 1,
 	HDMI_AUDIO_I2S_SD0_EN = 1,
@@ -486,19 +427,6 @@ struct hdmi_core_infoframe_avi {
 	/* Pixel number start of right bar */
 	u16	db12_13_pixel_sofright;
 };
-/*
- * Refer to section 8.2 in HDMI 1.3 specification for
- * details about infoframe databytes
- */
-struct hdmi_core_infoframe_audio {
-	u8 db1_coding_type;
-	u8 db1_channel_count;
-	u8 db2_sample_freq;
-	u8 db2_sample_size;
-	u8 db4_channel_alloc;
-	bool db5_downmix_inh;
-	u8 db5_lsv;	/* Level shift values for downmix */
-};
 
 struct hdmi_core_packet_enable_repeat {
 	u32	audio_pkt;
@@ -559,7 +487,7 @@ struct hdmi_core_audio_i2s_config {
 
 struct hdmi_core_audio_config {
 	struct hdmi_core_audio_i2s_config	i2s_cfg;
-	enum hdmi_core_audio_sample_freq	freq_sample;
+	u32 freq_sample;
 	bool					fs_override;
 	u32					n;
 	u32					cts;
@@ -579,7 +507,7 @@ struct hdmi_core_audio_config {
 int hdmi_config_audio_acr(struct hdmi_ip_data *ip_data,
 				u32 sample_freq, u32 *n, u32 *cts);
 void hdmi_core_audio_infoframe_config(struct hdmi_ip_data *ip_data,
-		struct hdmi_core_infoframe_audio *info_aud);
+		struct snd_cea_861_aud_if *info_aud);
 void hdmi_core_audio_config(struct hdmi_ip_data *ip_data,
 					struct hdmi_core_audio_config *cfg);
 void hdmi_wp_audio_config_dma(struct hdmi_ip_data *ip_data,
-- 
1.7.0.4

Re: [PATCH 02/10] OMAPDSS: HDMI: OMAP4: Remove CEA-861 audio infoframe and IEC-60958 enums

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 7657 bytes --]

On Wed, 2012-03-28 at 16:38 -0600, Ricardo Neri wrote:
> In order to avoid duplication of definitions. Use the definitions provided
> by asoundef.h. Furthermore, as CEA-861 and IEC-60958 are used by both
> DisplayPort and HDMI, this helps to make the code more generic.

The first two sentences should probably be just one sentence.

> Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
> ---
>  drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |   15 +++---
>  drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h |   80 ++---------------------------
>  2 files changed, 12 insertions(+), 83 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> index 4740e64..4ab3b19 100644
> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> @@ -29,7 +29,6 @@
>  #include <linux/string.h>
>  #include <linux/seq_file.h>
>  #include <linux/gpio.h>
> -

Unrelated change.

>  #include "ti_hdmi_4xxx_ip.h"
>  #include "dss.h"
>  
> @@ -1156,7 +1155,7 @@ void hdmi_core_audio_config(struct hdmi_ip_data *ip_data,
>  }
>  
>  void hdmi_core_audio_infoframe_config(struct hdmi_ip_data *ip_data,
> -		struct hdmi_core_infoframe_audio *info_aud)
> +		struct snd_cea_861_aud_if *info_aud)
>  {
>  	u8 val;
>  	u8 sum = 0, checksum = 0;
> @@ -1172,22 +1171,24 @@ void hdmi_core_audio_infoframe_config(struct hdmi_ip_data *ip_data,
>  	hdmi_write_reg(av_base, HDMI_CORE_AV_AUDIO_LEN, 0x0a);
>  	sum += 0x84 + 0x001 + 0x00a;
>  
> -	val = (info_aud->db1_coding_type << 4)
> -			| (info_aud->db1_channel_count - 1);
> +	val = (info_aud->coding_type << CEA861_AUDIO_INFOFRAME_DB1CT_SHIFT)
> +			| (info_aud->channel_count - 1);
>  	hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(0), val);
>  	sum += val;
>  
> -	val = (info_aud->db2_sample_freq << 2) | info_aud->db2_sample_size;
> +	val = (info_aud->sample_freq << CEA861_AUDIO_INFOFRAME_DB2SF_SHIFT)
> +			| (info_aud->sample_size);
>  	hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(1), val);
>  	sum += val;
>  
>  	hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(2), 0x00);
>  
> -	val = info_aud->db4_channel_alloc;
> +	val = info_aud->channel_alloc;
>  	hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(3), val);
>  	sum += val;
>  
> -	val = (info_aud->db5_downmix_inh << 7) | (info_aud->db5_lsv << 3);
> +	val = (info_aud->st_downmix << CEA861_AUDIO_INFOFRAME_DB5_DM_INH_SHIFT)
> +		| (info_aud->lsv << CEA861_AUDIO_INFOFRAME_DB5_LSV_SHIFT);
>  	hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(4), val);
>  	sum += val;
>  
> diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
> index a442998..d6b49b6 100644
> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
> @@ -28,6 +28,8 @@
>  	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
>  #include <sound/soc.h>
>  #include <sound/pcm_params.h>
> +#include <sound/asound.h>
> +#include <sound/asoundef.h>

You don't need to include these in the header file.

>  #endif
>  
>  /* HDMI Wrapper */
> @@ -284,35 +286,6 @@ enum hdmi_core_infoframe {
>  	HDMI_INFOFRAME_AVI_DB5PR_8 = 7,
>  	HDMI_INFOFRAME_AVI_DB5PR_9 = 8,
>  	HDMI_INFOFRAME_AVI_DB5PR_10 = 9,
> -	HDMI_INFOFRAME_AUDIO_DB1CT_FROM_STREAM = 0,
> -	HDMI_INFOFRAME_AUDIO_DB1CT_IEC60958 = 1,
> -	HDMI_INFOFRAME_AUDIO_DB1CT_AC3 = 2,
> -	HDMI_INFOFRAME_AUDIO_DB1CT_MPEG1 = 3,
> -	HDMI_INFOFRAME_AUDIO_DB1CT_MP3 = 4,
> -	HDMI_INFOFRAME_AUDIO_DB1CT_MPEG2_MULTICH = 5,
> -	HDMI_INFOFRAME_AUDIO_DB1CT_AAC = 6,
> -	HDMI_INFOFRAME_AUDIO_DB1CT_DTS = 7,
> -	HDMI_INFOFRAME_AUDIO_DB1CT_ATRAC = 8,
> -	HDMI_INFOFRAME_AUDIO_DB1CT_ONEBIT = 9,
> -	HDMI_INFOFRAME_AUDIO_DB1CT_DOLBY_DIGITAL_PLUS = 10,
> -	HDMI_INFOFRAME_AUDIO_DB1CT_DTS_HD = 11,
> -	HDMI_INFOFRAME_AUDIO_DB1CT_MAT = 12,
> -	HDMI_INFOFRAME_AUDIO_DB1CT_DST = 13,
> -	HDMI_INFOFRAME_AUDIO_DB1CT_WMA_PRO = 14,
> -	HDMI_INFOFRAME_AUDIO_DB2SF_FROM_STREAM = 0,
> -	HDMI_INFOFRAME_AUDIO_DB2SF_32000 = 1,
> -	HDMI_INFOFRAME_AUDIO_DB2SF_44100 = 2,
> -	HDMI_INFOFRAME_AUDIO_DB2SF_48000 = 3,
> -	HDMI_INFOFRAME_AUDIO_DB2SF_88200 = 4,
> -	HDMI_INFOFRAME_AUDIO_DB2SF_96000 = 5,
> -	HDMI_INFOFRAME_AUDIO_DB2SF_176400 = 6,
> -	HDMI_INFOFRAME_AUDIO_DB2SF_192000 = 7,
> -	HDMI_INFOFRAME_AUDIO_DB2SS_FROM_STREAM = 0,
> -	HDMI_INFOFRAME_AUDIO_DB2SS_16BIT = 1,
> -	HDMI_INFOFRAME_AUDIO_DB2SS_20BIT = 2,
> -	HDMI_INFOFRAME_AUDIO_DB2SS_24BIT = 3,
> -	HDMI_INFOFRAME_AUDIO_DB5_DM_INH_PERMITTED = 0,
> -	HDMI_INFOFRAME_AUDIO_DB5_DM_INH_PROHIBITED = 1
>  };
>  
>  enum hdmi_packing_mode {
> @@ -322,17 +295,6 @@ enum hdmi_packing_mode {
>  	HDMI_PACK_ALREADYPACKED = 7
>  };
>  
> -enum hdmi_core_audio_sample_freq {
> -	HDMI_AUDIO_FS_32000 = 0x3,
> -	HDMI_AUDIO_FS_44100 = 0x0,
> -	HDMI_AUDIO_FS_48000 = 0x2,
> -	HDMI_AUDIO_FS_88200 = 0x8,
> -	HDMI_AUDIO_FS_96000 = 0xA,
> -	HDMI_AUDIO_FS_176400 = 0xC,
> -	HDMI_AUDIO_FS_192000 = 0xE,
> -	HDMI_AUDIO_FS_NOT_INDICATED = 0x1
> -};
> -
>  enum hdmi_core_audio_layout {
>  	HDMI_AUDIO_LAYOUT_2CH = 0,
>  	HDMI_AUDIO_LAYOUT_8CH = 1
> @@ -393,31 +355,10 @@ enum hdmi_audio_i2s_config {

Are the defines left in the hdmi_audio_i2s_config something that are IP
specific? Are they even used? I'm just wondering why many of the defines
are in sound headers, but some are left here.

>  	HDMI_AUDIO_I2S_LSB_SHIFTED_FIRST = 1,
>  	HDMI_AUDIO_I2S_MAX_WORD_20BITS = 0,
>  	HDMI_AUDIO_I2S_MAX_WORD_24BITS = 1,
> -	HDMI_AUDIO_I2S_CHST_WORD_NOT_SPECIFIED = 0,
> -	HDMI_AUDIO_I2S_CHST_WORD_16_BITS = 1,
> -	HDMI_AUDIO_I2S_CHST_WORD_17_BITS = 6,
> -	HDMI_AUDIO_I2S_CHST_WORD_18_BITS = 2,
> -	HDMI_AUDIO_I2S_CHST_WORD_19_BITS = 4,
> -	HDMI_AUDIO_I2S_CHST_WORD_20_BITS_20MAX = 5,
> -	HDMI_AUDIO_I2S_CHST_WORD_20_BITS_24MAX = 1,
> -	HDMI_AUDIO_I2S_CHST_WORD_21_BITS = 6,
> -	HDMI_AUDIO_I2S_CHST_WORD_22_BITS = 2,
> -	HDMI_AUDIO_I2S_CHST_WORD_23_BITS = 4,
> -	HDMI_AUDIO_I2S_CHST_WORD_24_BITS = 5,
>  	HDMI_AUDIO_I2S_SCK_EDGE_FALLING = 0,
>  	HDMI_AUDIO_I2S_SCK_EDGE_RISING = 1,
>  	HDMI_AUDIO_I2S_VBIT_FOR_PCM = 0,
>  	HDMI_AUDIO_I2S_VBIT_FOR_COMPRESSED = 1,
> -	HDMI_AUDIO_I2S_INPUT_LENGTH_NA = 0,
> -	HDMI_AUDIO_I2S_INPUT_LENGTH_16 = 2,
> -	HDMI_AUDIO_I2S_INPUT_LENGTH_17 = 12,
> -	HDMI_AUDIO_I2S_INPUT_LENGTH_18 = 4,
> -	HDMI_AUDIO_I2S_INPUT_LENGTH_19 = 8,
> -	HDMI_AUDIO_I2S_INPUT_LENGTH_20 = 10,
> -	HDMI_AUDIO_I2S_INPUT_LENGTH_21 = 13,
> -	HDMI_AUDIO_I2S_INPUT_LENGTH_22 = 5,
> -	HDMI_AUDIO_I2S_INPUT_LENGTH_23 = 9,
> -	HDMI_AUDIO_I2S_INPUT_LENGTH_24 = 11,
>  	HDMI_AUDIO_I2S_FIRST_BIT_SHIFT = 0,
>  	HDMI_AUDIO_I2S_FIRST_BIT_NO_SHIFT = 1,
>  	HDMI_AUDIO_I2S_SD0_EN = 1,
> @@ -486,19 +427,6 @@ struct hdmi_core_infoframe_avi {
>  	/* Pixel number start of right bar */
>  	u16	db12_13_pixel_sofright;
>  };
> -/*
> - * Refer to section 8.2 in HDMI 1.3 specification for
> - * details about infoframe databytes
> - */
> -struct hdmi_core_infoframe_audio {
> -	u8 db1_coding_type;
> -	u8 db1_channel_count;
> -	u8 db2_sample_freq;
> -	u8 db2_sample_size;
> -	u8 db4_channel_alloc;
> -	bool db5_downmix_inh;
> -	u8 db5_lsv;	/* Level shift values for downmix */
> -};
>  
>  struct hdmi_core_packet_enable_repeat {
>  	u32	audio_pkt;
> @@ -559,7 +487,7 @@ struct hdmi_core_audio_i2s_config {
>  
>  struct hdmi_core_audio_config {
>  	struct hdmi_core_audio_i2s_config	i2s_cfg;
> -	enum hdmi_core_audio_sample_freq	freq_sample;
> +	u32 freq_sample;

Try to keep consistent code formatting. If the original code indents the
field names, indent your field name also.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 02/10] OMAPDSS: HDMI: OMAP4: Remove CEA-861 audio infoframe and IEC-60958 enums

[Ricardo Neri]

On 04/23/2012 08:12 AM, Tomi Valkeinen wrote:
> On Wed, 2012-03-28 at 16:38 -0600, Ricardo Neri wrote:
>> In order to avoid duplication of definitions. Use the definitions provided
>> by asoundef.h. Furthermore, as CEA-861 and IEC-60958 are used by both
>> DisplayPort and HDMI, this helps to make the code more generic.
>
> The first two sentences should probably be just one sentence.
Actually, I just rephrased the whole description :). I will submit it in 
v2 of the patch series.
>
>> Signed-off-by: Ricardo Neri<ricardo.neri@ti.com>
>> ---
>>   drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |   15 +++---
>>   drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h |   80 ++---------------------------
>>   2 files changed, 12 insertions(+), 83 deletions(-)
>>
>> diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>> index 4740e64..4ab3b19 100644
>> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>> @@ -29,7 +29,6 @@
>>   #include<linux/string.h>
>>   #include<linux/seq_file.h>
>>   #include<linux/gpio.h>
>> -
>
> Unrelated change.
I will remove it.
>
>>   #include "ti_hdmi_4xxx_ip.h"
>>   #include "dss.h"
>>
>> @@ -1156,7 +1155,7 @@ void hdmi_core_audio_config(struct hdmi_ip_data *ip_data,
>>   }
>>
>>   void hdmi_core_audio_infoframe_config(struct hdmi_ip_data *ip_data,
>> -		struct hdmi_core_infoframe_audio *info_aud)
>> +		struct snd_cea_861_aud_if *info_aud)
>>   {
>>   	u8 val;
>>   	u8 sum = 0, checksum = 0;
>> @@ -1172,22 +1171,24 @@ void hdmi_core_audio_infoframe_config(struct hdmi_ip_data *ip_data,
>>   	hdmi_write_reg(av_base, HDMI_CORE_AV_AUDIO_LEN, 0x0a);
>>   	sum += 0x84 + 0x001 + 0x00a;
>>
>> -	val = (info_aud->db1_coding_type<<  4)
>> -			| (info_aud->db1_channel_count - 1);
>> +	val = (info_aud->coding_type<<  CEA861_AUDIO_INFOFRAME_DB1CT_SHIFT)
>> +			| (info_aud->channel_count - 1);
>>   	hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(0), val);
>>   	sum += val;
>>
>> -	val = (info_aud->db2_sample_freq<<  2) | info_aud->db2_sample_size;
>> +	val = (info_aud->sample_freq<<  CEA861_AUDIO_INFOFRAME_DB2SF_SHIFT)
>> +			| (info_aud->sample_size);
>>   	hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(1), val);
>>   	sum += val;
>>
>>   	hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(2), 0x00);
>>
>> -	val = info_aud->db4_channel_alloc;
>> +	val = info_aud->channel_alloc;
>>   	hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(3), val);
>>   	sum += val;
>>
>> -	val = (info_aud->db5_downmix_inh<<  7) | (info_aud->db5_lsv<<  3);
>> +	val = (info_aud->st_downmix<<  CEA861_AUDIO_INFOFRAME_DB5_DM_INH_SHIFT)
>> +		| (info_aud->lsv<<  CEA861_AUDIO_INFOFRAME_DB5_LSV_SHIFT);
>>   	hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(4), val);
>>   	sum += val;
>>
>> diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
>> index a442998..d6b49b6 100644
>> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
>> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
>> @@ -28,6 +28,8 @@
>>   	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
>>   #include<sound/soc.h>
>>   #include<sound/pcm_params.h>
>> +#include<sound/asound.h>
>> +#include<sound/asoundef.h>
>
> You don't need to include these in the header file.
I will remove it.
>
>>   #endif
>>
>>   /* HDMI Wrapper */
>> @@ -284,35 +286,6 @@ enum hdmi_core_infoframe {
>>   	HDMI_INFOFRAME_AVI_DB5PR_8 = 7,
>>   	HDMI_INFOFRAME_AVI_DB5PR_9 = 8,
>>   	HDMI_INFOFRAME_AVI_DB5PR_10 = 9,
>> -	HDMI_INFOFRAME_AUDIO_DB1CT_FROM_STREAM = 0,
>> -	HDMI_INFOFRAME_AUDIO_DB1CT_IEC60958 = 1,
>> -	HDMI_INFOFRAME_AUDIO_DB1CT_AC3 = 2,
>> -	HDMI_INFOFRAME_AUDIO_DB1CT_MPEG1 = 3,
>> -	HDMI_INFOFRAME_AUDIO_DB1CT_MP3 = 4,
>> -	HDMI_INFOFRAME_AUDIO_DB1CT_MPEG2_MULTICH = 5,
>> -	HDMI_INFOFRAME_AUDIO_DB1CT_AAC = 6,
>> -	HDMI_INFOFRAME_AUDIO_DB1CT_DTS = 7,
>> -	HDMI_INFOFRAME_AUDIO_DB1CT_ATRAC = 8,
>> -	HDMI_INFOFRAME_AUDIO_DB1CT_ONEBIT = 9,
>> -	HDMI_INFOFRAME_AUDIO_DB1CT_DOLBY_DIGITAL_PLUS = 10,
>> -	HDMI_INFOFRAME_AUDIO_DB1CT_DTS_HD = 11,
>> -	HDMI_INFOFRAME_AUDIO_DB1CT_MAT = 12,
>> -	HDMI_INFOFRAME_AUDIO_DB1CT_DST = 13,
>> -	HDMI_INFOFRAME_AUDIO_DB1CT_WMA_PRO = 14,
>> -	HDMI_INFOFRAME_AUDIO_DB2SF_FROM_STREAM = 0,
>> -	HDMI_INFOFRAME_AUDIO_DB2SF_32000 = 1,
>> -	HDMI_INFOFRAME_AUDIO_DB2SF_44100 = 2,
>> -	HDMI_INFOFRAME_AUDIO_DB2SF_48000 = 3,
>> -	HDMI_INFOFRAME_AUDIO_DB2SF_88200 = 4,
>> -	HDMI_INFOFRAME_AUDIO_DB2SF_96000 = 5,
>> -	HDMI_INFOFRAME_AUDIO_DB2SF_176400 = 6,
>> -	HDMI_INFOFRAME_AUDIO_DB2SF_192000 = 7,
>> -	HDMI_INFOFRAME_AUDIO_DB2SS_FROM_STREAM = 0,
>> -	HDMI_INFOFRAME_AUDIO_DB2SS_16BIT = 1,
>> -	HDMI_INFOFRAME_AUDIO_DB2SS_20BIT = 2,
>> -	HDMI_INFOFRAME_AUDIO_DB2SS_24BIT = 3,
>> -	HDMI_INFOFRAME_AUDIO_DB5_DM_INH_PERMITTED = 0,
>> -	HDMI_INFOFRAME_AUDIO_DB5_DM_INH_PROHIBITED = 1
>>   };
>>
>>   enum hdmi_packing_mode {
>> @@ -322,17 +295,6 @@ enum hdmi_packing_mode {
>>   	HDMI_PACK_ALREADYPACKED = 7
>>   };
>>
>> -enum hdmi_core_audio_sample_freq {
>> -	HDMI_AUDIO_FS_32000 = 0x3,
>> -	HDMI_AUDIO_FS_44100 = 0x0,
>> -	HDMI_AUDIO_FS_48000 = 0x2,
>> -	HDMI_AUDIO_FS_88200 = 0x8,
>> -	HDMI_AUDIO_FS_96000 = 0xA,
>> -	HDMI_AUDIO_FS_176400 = 0xC,
>> -	HDMI_AUDIO_FS_192000 = 0xE,
>> -	HDMI_AUDIO_FS_NOT_INDICATED = 0x1
>> -};
>> -
>>   enum hdmi_core_audio_layout {
>>   	HDMI_AUDIO_LAYOUT_2CH = 0,
>>   	HDMI_AUDIO_LAYOUT_8CH = 1
>> @@ -393,31 +355,10 @@ enum hdmi_audio_i2s_config {
>
> Are the defines left in the hdmi_audio_i2s_config something that are IP
> specific? Are they even used? I'm just wondering why many of the defines
> are in sound headers, but some are left here.
Some are specific to the OMAP4 HDMI IP, such as HDMI_AUDIO_I2S_SDx_EN. 
Some others refer to generic I2S concepts (such as 
I2S_SCK_EDGE_FALLING/RISING) but defines are used to configure registers 
and such configuration may be different in other hardware. The defines 
that this patch removes are values that are effectively transmitted to 
the sink and are clearly defined in the relevant standards. Anyways, I 
will look at it further to see if some of them can be removed as well. 
Also, the I2S is the same for most of the supported use-cases, if not 
all of them. Maybe I can remove the unused ones.
>
>>   	HDMI_AUDIO_I2S_LSB_SHIFTED_FIRST = 1,
>>   	HDMI_AUDIO_I2S_MAX_WORD_20BITS = 0,
>>   	HDMI_AUDIO_I2S_MAX_WORD_24BITS = 1,
>> -	HDMI_AUDIO_I2S_CHST_WORD_NOT_SPECIFIED = 0,
>> -	HDMI_AUDIO_I2S_CHST_WORD_16_BITS = 1,
>> -	HDMI_AUDIO_I2S_CHST_WORD_17_BITS = 6,
>> -	HDMI_AUDIO_I2S_CHST_WORD_18_BITS = 2,
>> -	HDMI_AUDIO_I2S_CHST_WORD_19_BITS = 4,
>> -	HDMI_AUDIO_I2S_CHST_WORD_20_BITS_20MAX = 5,
>> -	HDMI_AUDIO_I2S_CHST_WORD_20_BITS_24MAX = 1,
>> -	HDMI_AUDIO_I2S_CHST_WORD_21_BITS = 6,
>> -	HDMI_AUDIO_I2S_CHST_WORD_22_BITS = 2,
>> -	HDMI_AUDIO_I2S_CHST_WORD_23_BITS = 4,
>> -	HDMI_AUDIO_I2S_CHST_WORD_24_BITS = 5,
>>   	HDMI_AUDIO_I2S_SCK_EDGE_FALLING = 0,
>>   	HDMI_AUDIO_I2S_SCK_EDGE_RISING = 1,
>>   	HDMI_AUDIO_I2S_VBIT_FOR_PCM = 0,
>>   	HDMI_AUDIO_I2S_VBIT_FOR_COMPRESSED = 1,
>> -	HDMI_AUDIO_I2S_INPUT_LENGTH_NA = 0,
>> -	HDMI_AUDIO_I2S_INPUT_LENGTH_16 = 2,
>> -	HDMI_AUDIO_I2S_INPUT_LENGTH_17 = 12,
>> -	HDMI_AUDIO_I2S_INPUT_LENGTH_18 = 4,
>> -	HDMI_AUDIO_I2S_INPUT_LENGTH_19 = 8,
>> -	HDMI_AUDIO_I2S_INPUT_LENGTH_20 = 10,
>> -	HDMI_AUDIO_I2S_INPUT_LENGTH_21 = 13,
>> -	HDMI_AUDIO_I2S_INPUT_LENGTH_22 = 5,
>> -	HDMI_AUDIO_I2S_INPUT_LENGTH_23 = 9,
>> -	HDMI_AUDIO_I2S_INPUT_LENGTH_24 = 11,
>>   	HDMI_AUDIO_I2S_FIRST_BIT_SHIFT = 0,
>>   	HDMI_AUDIO_I2S_FIRST_BIT_NO_SHIFT = 1,
>>   	HDMI_AUDIO_I2S_SD0_EN = 1,
>> @@ -486,19 +427,6 @@ struct hdmi_core_infoframe_avi {
>>   	/* Pixel number start of right bar */
>>   	u16	db12_13_pixel_sofright;
>>   };
>> -/*
>> - * Refer to section 8.2 in HDMI 1.3 specification for
>> - * details about infoframe databytes
>> - */
>> -struct hdmi_core_infoframe_audio {
>> -	u8 db1_coding_type;
>> -	u8 db1_channel_count;
>> -	u8 db2_sample_freq;
>> -	u8 db2_sample_size;
>> -	u8 db4_channel_alloc;
>> -	bool db5_downmix_inh;
>> -	u8 db5_lsv;	/* Level shift values for downmix */
>> -};
>>
>>   struct hdmi_core_packet_enable_repeat {
>>   	u32	audio_pkt;
>> @@ -559,7 +487,7 @@ struct hdmi_core_audio_i2s_config {
>>
>>   struct hdmi_core_audio_config {
>>   	struct hdmi_core_audio_i2s_config	i2s_cfg;
>> -	enum hdmi_core_audio_sample_freq	freq_sample;
>> +	u32 freq_sample;
>
> Try to keep consistent code formatting. If the original code indents the
> field names, indent your field name also.

I will fix the indentation.

BR,
Ricardo
>
>   Tomi

Re: [PATCH 02/10] OMAPDSS: HDMI: OMAP4: Remove CEA-861 audio infoframe and IEC-60958 enums

[Ricardo Neri]

On 04/24/2012 10:37 PM, Ricardo Neri wrote:
> On 04/23/2012 08:12 AM, Tomi Valkeinen wrote:
>> On Wed, 2012-03-28 at 16:38 -0600, Ricardo Neri wrote:
>>> In order to avoid duplication of definitions. Use the definitions
>>> provided
>>> by asoundef.h. Furthermore, as CEA-861 and IEC-60958 are used by both
>>> DisplayPort and HDMI, this helps to make the code more generic.
>>
>> The first two sentences should probably be just one sentence.
> Actually, I just rephrased the whole description :). I will submit it in
> v2 of the patch series.
>>
>>> Signed-off-by: Ricardo Neri<ricardo.neri@ti.com>
>>> ---
>>> drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c | 15 +++---
>>> drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h | 80
>>> ++---------------------------
>>> 2 files changed, 12 insertions(+), 83 deletions(-)
>>>
>>> diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>>> b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>>> index 4740e64..4ab3b19 100644
>>> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>>> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>>> @@ -29,7 +29,6 @@
>>> #include<linux/string.h>
>>> #include<linux/seq_file.h>
>>> #include<linux/gpio.h>
>>> -
>>
>> Unrelated change.
> I will remove it.
>>
>>> #include "ti_hdmi_4xxx_ip.h"
>>> #include "dss.h"
>>>
>>> @@ -1156,7 +1155,7 @@ void hdmi_core_audio_config(struct hdmi_ip_data
>>> *ip_data,
>>> }
>>>
>>> void hdmi_core_audio_infoframe_config(struct hdmi_ip_data *ip_data,
>>> - struct hdmi_core_infoframe_audio *info_aud)
>>> + struct snd_cea_861_aud_if *info_aud)
>>> {
>>> u8 val;
>>> u8 sum = 0, checksum = 0;
>>> @@ -1172,22 +1171,24 @@ void hdmi_core_audio_infoframe_config(struct
>>> hdmi_ip_data *ip_data,
>>> hdmi_write_reg(av_base, HDMI_CORE_AV_AUDIO_LEN, 0x0a);
>>> sum += 0x84 + 0x001 + 0x00a;
>>>
>>> - val = (info_aud->db1_coding_type<< 4)
>>> - | (info_aud->db1_channel_count - 1);
>>> + val = (info_aud->coding_type<< CEA861_AUDIO_INFOFRAME_DB1CT_SHIFT)
>>> + | (info_aud->channel_count - 1);
>>> hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(0), val);
>>> sum += val;
>>>
>>> - val = (info_aud->db2_sample_freq<< 2) | info_aud->db2_sample_size;
>>> + val = (info_aud->sample_freq<< CEA861_AUDIO_INFOFRAME_DB2SF_SHIFT)
>>> + | (info_aud->sample_size);
>>> hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(1), val);
>>> sum += val;
>>>
>>> hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(2), 0x00);
>>>
>>> - val = info_aud->db4_channel_alloc;
>>> + val = info_aud->channel_alloc;
>>> hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(3), val);
>>> sum += val;
>>>
>>> - val = (info_aud->db5_downmix_inh<< 7) | (info_aud->db5_lsv<< 3);
>>> + val = (info_aud->st_downmix<< CEA861_AUDIO_INFOFRAME_DB5_DM_INH_SHIFT)
>>> + | (info_aud->lsv<< CEA861_AUDIO_INFOFRAME_DB5_LSV_SHIFT);
>>> hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_DBYTE(4), val);
>>> sum += val;
>>>
>>> diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
>>> b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
>>> index a442998..d6b49b6 100644
>>> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
>>> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
>>> @@ -28,6 +28,8 @@
>>> defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
>>> #include<sound/soc.h>
>>> #include<sound/pcm_params.h>
>>> +#include<sound/asound.h>
>>> +#include<sound/asoundef.h>
>>
>> You don't need to include these in the header file.
> I will remove it.
>>
>>> #endif
>>>
>>> /* HDMI Wrapper */
>>> @@ -284,35 +286,6 @@ enum hdmi_core_infoframe {
>>> HDMI_INFOFRAME_AVI_DB5PR_8 = 7,
>>> HDMI_INFOFRAME_AVI_DB5PR_9 = 8,
>>> HDMI_INFOFRAME_AVI_DB5PR_10 = 9,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_FROM_STREAM = 0,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_IEC60958 = 1,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_AC3 = 2,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_MPEG1 = 3,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_MP3 = 4,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_MPEG2_MULTICH = 5,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_AAC = 6,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_DTS = 7,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_ATRAC = 8,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_ONEBIT = 9,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_DOLBY_DIGITAL_PLUS = 10,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_DTS_HD = 11,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_MAT = 12,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_DST = 13,
>>> - HDMI_INFOFRAME_AUDIO_DB1CT_WMA_PRO = 14,
>>> - HDMI_INFOFRAME_AUDIO_DB2SF_FROM_STREAM = 0,
>>> - HDMI_INFOFRAME_AUDIO_DB2SF_32000 = 1,
>>> - HDMI_INFOFRAME_AUDIO_DB2SF_44100 = 2,
>>> - HDMI_INFOFRAME_AUDIO_DB2SF_48000 = 3,
>>> - HDMI_INFOFRAME_AUDIO_DB2SF_88200 = 4,
>>> - HDMI_INFOFRAME_AUDIO_DB2SF_96000 = 5,
>>> - HDMI_INFOFRAME_AUDIO_DB2SF_176400 = 6,
>>> - HDMI_INFOFRAME_AUDIO_DB2SF_192000 = 7,
>>> - HDMI_INFOFRAME_AUDIO_DB2SS_FROM_STREAM = 0,
>>> - HDMI_INFOFRAME_AUDIO_DB2SS_16BIT = 1,
>>> - HDMI_INFOFRAME_AUDIO_DB2SS_20BIT = 2,
>>> - HDMI_INFOFRAME_AUDIO_DB2SS_24BIT = 3,
>>> - HDMI_INFOFRAME_AUDIO_DB5_DM_INH_PERMITTED = 0,
>>> - HDMI_INFOFRAME_AUDIO_DB5_DM_INH_PROHIBITED = 1
>>> };
>>>
>>> enum hdmi_packing_mode {
>>> @@ -322,17 +295,6 @@ enum hdmi_packing_mode {
>>> HDMI_PACK_ALREADYPACKED = 7
>>> };
>>>
>>> -enum hdmi_core_audio_sample_freq {
>>> - HDMI_AUDIO_FS_32000 = 0x3,
>>> - HDMI_AUDIO_FS_44100 = 0x0,
>>> - HDMI_AUDIO_FS_48000 = 0x2,
>>> - HDMI_AUDIO_FS_88200 = 0x8,
>>> - HDMI_AUDIO_FS_96000 = 0xA,
>>> - HDMI_AUDIO_FS_176400 = 0xC,
>>> - HDMI_AUDIO_FS_192000 = 0xE,
>>> - HDMI_AUDIO_FS_NOT_INDICATED = 0x1
>>> -};
>>> -
>>> enum hdmi_core_audio_layout {
>>> HDMI_AUDIO_LAYOUT_2CH = 0,
>>> HDMI_AUDIO_LAYOUT_8CH = 1
>>> @@ -393,31 +355,10 @@ enum hdmi_audio_i2s_config {
>>
>> Are the defines left in the hdmi_audio_i2s_config something that are IP
>> specific? Are they even used? I'm just wondering why many of the defines
>> are in sound headers, but some are left here.
> Some are specific to the OMAP4 HDMI IP, such as HDMI_AUDIO_I2S_SDx_EN.
> Some others refer to generic I2S concepts (such as
> I2S_SCK_EDGE_FALLING/RISING) but defines are used to configure registers
> and such configuration may be different in other hardware. The defines
> that this patch removes are values that are effectively transmitted to
> the sink and are clearly defined in the relevant standards. Anyways, I
> will look at it further to see if some of them can be removed as well.
> Also, the I2S is the same for most of the supported use-cases, if not
> all of them. Maybe I can remove the unused ones.

I took at a second look at the hdmi_audio_i2s_config. As they are used 
to set IP-specific registers, I think they should be kept. Regarding the 
unused defines, they are not too many, do not do harm and let you know 
what other config values are available. I would like to keep them. What 
do you think?

BR,

Ricardo
>>
>>> HDMI_AUDIO_I2S_LSB_SHIFTED_FIRST = 1,
>>> HDMI_AUDIO_I2S_MAX_WORD_20BITS = 0,
>>> HDMI_AUDIO_I2S_MAX_WORD_24BITS = 1,
>>> - HDMI_AUDIO_I2S_CHST_WORD_NOT_SPECIFIED = 0,
>>> - HDMI_AUDIO_I2S_CHST_WORD_16_BITS = 1,
>>> - HDMI_AUDIO_I2S_CHST_WORD_17_BITS = 6,
>>> - HDMI_AUDIO_I2S_CHST_WORD_18_BITS = 2,
>>> - HDMI_AUDIO_I2S_CHST_WORD_19_BITS = 4,
>>> - HDMI_AUDIO_I2S_CHST_WORD_20_BITS_20MAX = 5,
>>> - HDMI_AUDIO_I2S_CHST_WORD_20_BITS_24MAX = 1,
>>> - HDMI_AUDIO_I2S_CHST_WORD_21_BITS = 6,
>>> - HDMI_AUDIO_I2S_CHST_WORD_22_BITS = 2,
>>> - HDMI_AUDIO_I2S_CHST_WORD_23_BITS = 4,
>>> - HDMI_AUDIO_I2S_CHST_WORD_24_BITS = 5,
>>> HDMI_AUDIO_I2S_SCK_EDGE_FALLING = 0,
>>> HDMI_AUDIO_I2S_SCK_EDGE_RISING = 1,
>>> HDMI_AUDIO_I2S_VBIT_FOR_PCM = 0,
>>> HDMI_AUDIO_I2S_VBIT_FOR_COMPRESSED = 1,
>>> - HDMI_AUDIO_I2S_INPUT_LENGTH_NA = 0,
>>> - HDMI_AUDIO_I2S_INPUT_LENGTH_16 = 2,
>>> - HDMI_AUDIO_I2S_INPUT_LENGTH_17 = 12,
>>> - HDMI_AUDIO_I2S_INPUT_LENGTH_18 = 4,
>>> - HDMI_AUDIO_I2S_INPUT_LENGTH_19 = 8,
>>> - HDMI_AUDIO_I2S_INPUT_LENGTH_20 = 10,
>>> - HDMI_AUDIO_I2S_INPUT_LENGTH_21 = 13,
>>> - HDMI_AUDIO_I2S_INPUT_LENGTH_22 = 5,
>>> - HDMI_AUDIO_I2S_INPUT_LENGTH_23 = 9,
>>> - HDMI_AUDIO_I2S_INPUT_LENGTH_24 = 11,
>>> HDMI_AUDIO_I2S_FIRST_BIT_SHIFT = 0,
>>> HDMI_AUDIO_I2S_FIRST_BIT_NO_SHIFT = 1,
>>> HDMI_AUDIO_I2S_SD0_EN = 1,
>>> @@ -486,19 +427,6 @@ struct hdmi_core_infoframe_avi {
>>> /* Pixel number start of right bar */
>>> u16 db12_13_pixel_sofright;
>>> };
>>> -/*
>>> - * Refer to section 8.2 in HDMI 1.3 specification for
>>> - * details about infoframe databytes
>>> - */
>>> -struct hdmi_core_infoframe_audio {
>>> - u8 db1_coding_type;
>>> - u8 db1_channel_count;
>>> - u8 db2_sample_freq;
>>> - u8 db2_sample_size;
>>> - u8 db4_channel_alloc;
>>> - bool db5_downmix_inh;
>>> - u8 db5_lsv; /* Level shift values for downmix */
>>> -};
>>>
>>> struct hdmi_core_packet_enable_repeat {
>>> u32 audio_pkt;
>>> @@ -559,7 +487,7 @@ struct hdmi_core_audio_i2s_config {
>>>
>>> struct hdmi_core_audio_config {
>>> struct hdmi_core_audio_i2s_config i2s_cfg;
>>> - enum hdmi_core_audio_sample_freq freq_sample;
>>> + u32 freq_sample;
>>
>> Try to keep consistent code formatting. If the original code indents the
>> field names, indent your field name also.
>
> I will fix the indentation.
>
> BR,
> Ricardo
>>
>> Tomi
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/10] OMAPDSS: HDMI: OMAP4: Remove CEA-861 audio infoframe and IEC-60958 enums

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1481 bytes --]

On Thu, 2012-04-26 at 20:32 -0500, Ricardo Neri wrote:
> On 04/24/2012 10:37 PM, Ricardo Neri wrote:

> >> Are the defines left in the hdmi_audio_i2s_config something that are IP
> >> specific? Are they even used? I'm just wondering why many of the defines
> >> are in sound headers, but some are left here.
> > Some are specific to the OMAP4 HDMI IP, such as HDMI_AUDIO_I2S_SDx_EN.
> > Some others refer to generic I2S concepts (such as
> > I2S_SCK_EDGE_FALLING/RISING) but defines are used to configure registers
> > and such configuration may be different in other hardware. The defines
> > that this patch removes are values that are effectively transmitted to
> > the sink and are clearly defined in the relevant standards. Anyways, I
> > will look at it further to see if some of them can be removed as well.
> > Also, the I2S is the same for most of the supported use-cases, if not
> > all of them. Maybe I can remove the unused ones.
> 
> I took at a second look at the hdmi_audio_i2s_config. As they are used 
> to set IP-specific registers, I think they should be kept. Regarding the 
> unused defines, they are not too many, do not do harm and let you know 
> what other config values are available. I would like to keep them. What 
> do you think?

Sounds ok to me. I was just worried that there could be some kind of
mixup as only parts of the hdmi_audio_i2s_config were removed. But if
the rest are IP specific, I think it's fine.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 03/10] OMAPDSS: HDMI: OMAP4: Correcty typo in I2S definitions

[Ricardo Neri]

Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
---
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
index d6b49b6..9fa5cb1 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
@@ -350,7 +350,7 @@ enum hdmi_audio_blk_strt_end_sig {
 
 enum hdmi_audio_i2s_config {
 	HDMI_AUDIO_I2S_WS_POLARITY_LOW_IS_LEFT = 0,
-	HDMI_AUDIO_I2S_WS_POLARIT_YLOW_IS_RIGHT = 1,
+	HDMI_AUDIO_I2S_WS_POLARITY_LOW_IS_RIGHT = 1,
 	HDMI_AUDIO_I2S_MSB_SHIFTED_FIRST = 0,
 	HDMI_AUDIO_I2S_LSB_SHIFTED_FIRST = 1,
 	HDMI_AUDIO_I2S_MAX_WORD_20BITS = 0,
-- 
1.7.0.4

Re: [PATCH 03/10] OMAPDSS: HDMI: OMAP4: Correcty typo in I2S definitions

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 282 bytes --]

On Wed, 2012-03-28 at 16:38 -0600, Ricardo Neri wrote:
> Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>

There's a typo in the subject of the typo fix =).

And always provide a patch description, please. In the simplest cases it
can be the same as the subject.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 03/10] OMAPDSS: HDMI: OMAP4: Correcty typo in I2S definitions

[Ricardo Neri]

On 04/23/2012 07:42 AM, Tomi Valkeinen wrote:
> On Wed, 2012-03-28 at 16:38 -0600, Ricardo Neri wrote:
>> Signed-off-by: Ricardo Neri<ricardo.neri@ti.com>
>
> There's a typo in the subject of the typo fix =).
Oops! I totally missed it. I will fix.
>
> And always provide a patch description, please. In the simplest cases it
> can be the same as the subject.
Understood.

BR,
Ricardo
>
>   Tomi

[PATCH 04/10] OMAPDSS: HDMI: OMAP4: Decouple wrapper enable and audio start

[Ricardo Neri]

Decouple the enablement of the HDMI audio wrapper from audio start.
Otherwise, an audio FIFO underflow may occur. The audio wrapper
enablement must be done after configuration and before audio playback
is started.

Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
---
 drivers/video/omap2/dss/dss_features.c    |    1 +
 drivers/video/omap2/dss/ti_hdmi.h         |    3 +++
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |    8 ++++++--
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c
index 162c9a9..c1839e2 100644
--- a/drivers/video/omap2/dss/dss_features.c
+++ b/drivers/video/omap2/dss/dss_features.c
@@ -500,6 +500,7 @@ static const struct ti_hdmi_ip_ops omap4_hdmi_functions = {
 #if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
 	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
 	.audio_enable		=       ti_hdmi_4xxx_wp_audio_enable,
+	.audio_start		=       ti_hdmi_4xxx_audio_start,
 #endif
 
 };
diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
index 50dadba..529e227 100644
--- a/drivers/video/omap2/dss/ti_hdmi.h
+++ b/drivers/video/omap2/dss/ti_hdmi.h
@@ -113,6 +113,8 @@ struct ti_hdmi_ip_ops {
 #if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
 	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
 	void (*audio_enable)(struct hdmi_ip_data *ip_data, bool start);
+
+	void (*audio_start)(struct hdmi_ip_data *ip_data, bool start);
 #endif
 
 };
@@ -146,5 +148,6 @@ void ti_hdmi_4xxx_phy_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
 #if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
 	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
 void ti_hdmi_4xxx_wp_audio_enable(struct hdmi_ip_data *ip_data, bool enable);
+void ti_hdmi_4xxx_audio_start(struct hdmi_ip_data *ip_data, bool enable);
 #endif
 #endif
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
index 4ab3b19..e6fa61d 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -1267,10 +1267,14 @@ int hdmi_config_audio_acr(struct hdmi_ip_data *ip_data,
 
 void ti_hdmi_4xxx_wp_audio_enable(struct hdmi_ip_data *ip_data, bool enable)
 {
-	REG_FLD_MOD(hdmi_av_base(ip_data),
-				HDMI_CORE_AV_AUD_MODE, enable, 0, 0);
 	REG_FLD_MOD(hdmi_wp_base(ip_data),
 				HDMI_WP_AUDIO_CTRL, enable, 31, 31);
+}
+
+void ti_hdmi_4xxx_audio_start(struct hdmi_ip_data *ip_data, bool enable)
+{
+	REG_FLD_MOD(hdmi_av_base(ip_data),
+				HDMI_CORE_AV_AUD_MODE, enable, 0, 0);
 	REG_FLD_MOD(hdmi_wp_base(ip_data),
 				HDMI_WP_AUDIO_CTRL, enable, 30, 30);
 }
-- 
1.7.0.4

[PATCH 05/10] OMAPDSS: HDMI: Decouple HDMI audio from ASoC

[Ricardo Neri]

Instead of having OMAPDSS HDMI audio functionality depending on the
ASoC HDMI audio driver, use a new config option so that
potential users, including ASoC,  may select if needed.

Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
---
 drivers/video/omap2/dss/Kconfig           |    4 ++++
 drivers/video/omap2/dss/dss_features.c    |    3 +--
 drivers/video/omap2/dss/ti_hdmi.h         |    6 ++----
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |    3 +--
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h |    5 +----
 5 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/video/omap2/dss/Kconfig b/drivers/video/omap2/dss/Kconfig
index 7be7c06..b492001 100644
--- a/drivers/video/omap2/dss/Kconfig
+++ b/drivers/video/omap2/dss/Kconfig
@@ -68,6 +68,10 @@ config OMAP4_DSS_HDMI
 	  HDMI Interface. This adds the High Definition Multimedia Interface.
 	  See http://www.hdmi.org/ for HDMI specification.
 
+config OMAP4_DSS_HDMI_AUDIO
+	bool
+	depends on OMAP4_DSS_HDMI
+
 config OMAP2_DSS_SDI
 	bool "SDI support"
 	depends on ARCH_OMAP3
diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c
index c1839e2..399a28a 100644
--- a/drivers/video/omap2/dss/dss_features.c
+++ b/drivers/video/omap2/dss/dss_features.c
@@ -497,8 +497,7 @@ static const struct ti_hdmi_ip_ops omap4_hdmi_functions = {
 	.dump_core		=	ti_hdmi_4xxx_core_dump,
 	.dump_pll		=	ti_hdmi_4xxx_pll_dump,
 	.dump_phy		=	ti_hdmi_4xxx_phy_dump,
-#if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
-	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
+#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
 	.audio_enable		=       ti_hdmi_4xxx_wp_audio_enable,
 	.audio_start		=       ti_hdmi_4xxx_audio_start,
 #endif
diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
index 529e227..211da6f 100644
--- a/drivers/video/omap2/dss/ti_hdmi.h
+++ b/drivers/video/omap2/dss/ti_hdmi.h
@@ -110,8 +110,7 @@ struct ti_hdmi_ip_ops {
 
 	void (*dump_phy)(struct hdmi_ip_data *ip_data, struct seq_file *s);
 
-#if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
-	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
+#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
 	void (*audio_enable)(struct hdmi_ip_data *ip_data, bool start);
 
 	void (*audio_start)(struct hdmi_ip_data *ip_data, bool start);
@@ -145,8 +144,7 @@ void ti_hdmi_4xxx_wp_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
 void ti_hdmi_4xxx_pll_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
 void ti_hdmi_4xxx_core_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
 void ti_hdmi_4xxx_phy_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
-#if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
-	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
+#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
 void ti_hdmi_4xxx_wp_audio_enable(struct hdmi_ip_data *ip_data, bool enable);
 void ti_hdmi_4xxx_audio_start(struct hdmi_ip_data *ip_data, bool enable);
 #endif
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
index e6fa61d..e06139a 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -1030,8 +1030,7 @@ void ti_hdmi_4xxx_phy_dump(struct hdmi_ip_data *ip_data, struct seq_file *s)
 	DUMPPHY(HDMI_TXPHY_PAD_CFG_CTRL);
 }
 
-#if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
-	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
+#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
 void hdmi_wp_audio_config_format(struct hdmi_ip_data *ip_data,
 					struct hdmi_audio_format *aud_fmt)
 {
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
index 9fa5cb1..222cc16 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
@@ -26,8 +26,6 @@
 #include "ti_hdmi.h"
 #if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
 	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
-#include <sound/soc.h>
-#include <sound/pcm_params.h>
 #include <sound/asound.h>
 #include <sound/asoundef.h>
 #endif
@@ -502,8 +500,7 @@ struct hdmi_core_audio_config {
 	bool					en_spdif;
 };
 
-#if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
-	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
+#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
 int hdmi_config_audio_acr(struct hdmi_ip_data *ip_data,
 				u32 sample_freq, u32 *n, u32 *cts);
 void hdmi_core_audio_infoframe_config(struct hdmi_ip_data *ip_data,
-- 
1.7.0.4

Re: [PATCH 05/10] OMAPDSS: HDMI: Decouple HDMI audio from ASoC

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 4410 bytes --]

On Wed, 2012-03-28 at 16:38 -0600, Ricardo Neri wrote:
> Instead of having OMAPDSS HDMI audio functionality depending on the
> ASoC HDMI audio driver, use a new config option so that
> potential users, including ASoC,  may select if needed.
> 
> Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
> ---
>  drivers/video/omap2/dss/Kconfig           |    4 ++++
>  drivers/video/omap2/dss/dss_features.c    |    3 +--
>  drivers/video/omap2/dss/ti_hdmi.h         |    6 ++----
>  drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |    3 +--
>  drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h |    5 +----
>  5 files changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/Kconfig b/drivers/video/omap2/dss/Kconfig
> index 7be7c06..b492001 100644
> --- a/drivers/video/omap2/dss/Kconfig
> +++ b/drivers/video/omap2/dss/Kconfig
> @@ -68,6 +68,10 @@ config OMAP4_DSS_HDMI
>  	  HDMI Interface. This adds the High Definition Multimedia Interface.
>  	  See http://www.hdmi.org/ for HDMI specification.
>  
> +config OMAP4_DSS_HDMI_AUDIO
> +	bool
> +	depends on OMAP4_DSS_HDMI
> +
>  config OMAP2_DSS_SDI
>  	bool "SDI support"
>  	depends on ARCH_OMAP3
> diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c
> index c1839e2..399a28a 100644
> --- a/drivers/video/omap2/dss/dss_features.c
> +++ b/drivers/video/omap2/dss/dss_features.c
> @@ -497,8 +497,7 @@ static const struct ti_hdmi_ip_ops omap4_hdmi_functions = {
>  	.dump_core		=	ti_hdmi_4xxx_core_dump,
>  	.dump_pll		=	ti_hdmi_4xxx_pll_dump,
>  	.dump_phy		=	ti_hdmi_4xxx_phy_dump,
> -#if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
> -	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
> +#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
>  	.audio_enable		=       ti_hdmi_4xxx_wp_audio_enable,
>  	.audio_start		=       ti_hdmi_4xxx_audio_start,
>  #endif
> diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
> index 529e227..211da6f 100644
> --- a/drivers/video/omap2/dss/ti_hdmi.h
> +++ b/drivers/video/omap2/dss/ti_hdmi.h
> @@ -110,8 +110,7 @@ struct ti_hdmi_ip_ops {
>  
>  	void (*dump_phy)(struct hdmi_ip_data *ip_data, struct seq_file *s);
>  
> -#if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
> -	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
> +#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
>  	void (*audio_enable)(struct hdmi_ip_data *ip_data, bool start);
>  
>  	void (*audio_start)(struct hdmi_ip_data *ip_data, bool start);
> @@ -145,8 +144,7 @@ void ti_hdmi_4xxx_wp_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
>  void ti_hdmi_4xxx_pll_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
>  void ti_hdmi_4xxx_core_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
>  void ti_hdmi_4xxx_phy_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
> -#if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
> -	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
> +#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
>  void ti_hdmi_4xxx_wp_audio_enable(struct hdmi_ip_data *ip_data, bool enable);
>  void ti_hdmi_4xxx_audio_start(struct hdmi_ip_data *ip_data, bool enable);
>  #endif
> diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> index e6fa61d..e06139a 100644
> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> @@ -1030,8 +1030,7 @@ void ti_hdmi_4xxx_phy_dump(struct hdmi_ip_data *ip_data, struct seq_file *s)
>  	DUMPPHY(HDMI_TXPHY_PAD_CFG_CTRL);
>  }
>  
> -#if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
> -	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
> +#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
>  void hdmi_wp_audio_config_format(struct hdmi_ip_data *ip_data,
>  					struct hdmi_audio_format *aud_fmt)
>  {
> diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
> index 9fa5cb1..222cc16 100644
> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
> @@ -26,8 +26,6 @@
>  #include "ti_hdmi.h"
>  #if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
>  	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
> -#include <sound/soc.h>
> -#include <sound/pcm_params.h>

Unrelated change. And... is there something wrong with this? Shouldn't
the defines above be changed?

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 05/10] OMAPDSS: HDMI: Decouple HDMI audio from ASoC

[Ricardo Neri]

On 04/23/2012 08:25 AM, Tomi Valkeinen wrote:
> On Wed, 2012-03-28 at 16:38 -0600, Ricardo Neri wrote:
>> Instead of having OMAPDSS HDMI audio functionality depending on the
>> ASoC HDMI audio driver, use a new config option so that
>> potential users, including ASoC,  may select if needed.
>>
>> Signed-off-by: Ricardo Neri<ricardo.neri@ti.com>
>> ---
>>   drivers/video/omap2/dss/Kconfig           |    4 ++++
>>   drivers/video/omap2/dss/dss_features.c    |    3 +--
>>   drivers/video/omap2/dss/ti_hdmi.h         |    6 ++----
>>   drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |    3 +--
>>   drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h |    5 +----
>>   5 files changed, 9 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/video/omap2/dss/Kconfig b/drivers/video/omap2/dss/Kconfig
>> index 7be7c06..b492001 100644
>> --- a/drivers/video/omap2/dss/Kconfig
>> +++ b/drivers/video/omap2/dss/Kconfig
>> @@ -68,6 +68,10 @@ config OMAP4_DSS_HDMI
>>   	  HDMI Interface. This adds the High Definition Multimedia Interface.
>>   	  See http://www.hdmi.org/ for HDMI specification.
>>
>> +config OMAP4_DSS_HDMI_AUDIO
>> +	bool
>> +	depends on OMAP4_DSS_HDMI
>> +
>>   config OMAP2_DSS_SDI
>>   	bool "SDI support"
>>   	depends on ARCH_OMAP3
>> diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c
>> index c1839e2..399a28a 100644
>> --- a/drivers/video/omap2/dss/dss_features.c
>> +++ b/drivers/video/omap2/dss/dss_features.c
>> @@ -497,8 +497,7 @@ static const struct ti_hdmi_ip_ops omap4_hdmi_functions = {
>>   	.dump_core		=	ti_hdmi_4xxx_core_dump,
>>   	.dump_pll		=	ti_hdmi_4xxx_pll_dump,
>>   	.dump_phy		=	ti_hdmi_4xxx_phy_dump,
>> -#if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
>> -	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
>> +#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
>>   	.audio_enable		=       ti_hdmi_4xxx_wp_audio_enable,
>>   	.audio_start		=       ti_hdmi_4xxx_audio_start,
>>   #endif
>> diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
>> index 529e227..211da6f 100644
>> --- a/drivers/video/omap2/dss/ti_hdmi.h
>> +++ b/drivers/video/omap2/dss/ti_hdmi.h
>> @@ -110,8 +110,7 @@ struct ti_hdmi_ip_ops {
>>
>>   	void (*dump_phy)(struct hdmi_ip_data *ip_data, struct seq_file *s);
>>
>> -#if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
>> -	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
>> +#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
>>   	void (*audio_enable)(struct hdmi_ip_data *ip_data, bool start);
>>
>>   	void (*audio_start)(struct hdmi_ip_data *ip_data, bool start);
>> @@ -145,8 +144,7 @@ void ti_hdmi_4xxx_wp_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
>>   void ti_hdmi_4xxx_pll_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
>>   void ti_hdmi_4xxx_core_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
>>   void ti_hdmi_4xxx_phy_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
>> -#if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
>> -	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
>> +#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
>>   void ti_hdmi_4xxx_wp_audio_enable(struct hdmi_ip_data *ip_data, bool enable);
>>   void ti_hdmi_4xxx_audio_start(struct hdmi_ip_data *ip_data, bool enable);
>>   #endif
>> diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>> index e6fa61d..e06139a 100644
>> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>> @@ -1030,8 +1030,7 @@ void ti_hdmi_4xxx_phy_dump(struct hdmi_ip_data *ip_data, struct seq_file *s)
>>   	DUMPPHY(HDMI_TXPHY_PAD_CFG_CTRL);
>>   }
>>
>> -#if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
>> -	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
>> +#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
>>   void hdmi_wp_audio_config_format(struct hdmi_ip_data *ip_data,
>>   					struct hdmi_audio_format *aud_fmt)
>>   {
>> diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
>> index 9fa5cb1..222cc16 100644
>> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
>> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
>> @@ -26,8 +26,6 @@
>>   #include "ti_hdmi.h"
>>   #if defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI) || \
>>   	defined(CONFIG_SND_OMAP_SOC_OMAP4_HDMI_MODULE)
>> -#include<sound/soc.h>
>> -#include<sound/pcm_params.h>
>
> Unrelated change. And... is there something wrong with this? Shouldn't
> the defines above be changed?
Yes. The defines should have changed. I will also remove the #includes 
in a separate patch.

BR,
Ricardo
>
>   Tomi

[PATCH 06/10] OMAPDSS: HDMI: OMAP4: Expand configuration for IEC-60958 audio

[Ricardo Neri]

Provide more configuration parameters for the IEC-60958 channel status word.
For this, a new structure containing the relevant parameters is created. Some
of the parameters previously located in the I2S structure are moved
to the new IEC-60958 struct to improve the logical organization of the code.

Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
---
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |   39 +++++++++++++++++++++-------
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h |   23 ++++++++++++++---
 2 files changed, 48 insertions(+), 14 deletions(-)

diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
index e06139a..da2806c 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -1119,12 +1119,37 @@ void hdmi_core_audio_config(struct hdmi_ip_data *ip_data,
 	REG_FLD_MOD(av_base, HDMI_CORE_AV_SPDIF_CTRL,
 						cfg->fs_override, 1, 1);
 
-	/* I2S parameters */
-	REG_FLD_MOD(av_base, HDMI_CORE_AV_I2S_CHST4,
-						cfg->freq_sample, 3, 0);
+	/* I2S and IEC60958 parameters */
+
+	r = hdmi_read_reg(av_base, HDMI_CORE_AV_I2S_CHST0);
+	r = FLD_MOD(r, cfg->iec60958_cfg.professional, 0, 0);
+	r = FLD_MOD(r, cfg->iec60958_cfg.for_lpcm_aud, 1, 1);
+	r = FLD_MOD(r, cfg->iec60958_cfg.copyright, 2, 2);
+	r = FLD_MOD(r, cfg->iec60958_cfg.emphasis, 5, 3);
+	r = FLD_MOD(r, cfg->iec60958_cfg.mode, 7, 6);
+	hdmi_write_reg(av_base, HDMI_CORE_AV_I2S_CHST0, r);
+
+	hdmi_write_reg(av_base, HDMI_CORE_AV_I2S_CHST1,
+						cfg->iec60958_cfg.category);
+
+	r = hdmi_read_reg(av_base, HDMI_CORE_AV_I2S_CHST2);
+	r = FLD_MOD(r, cfg->iec60958_cfg.source_nr, 3, 0);
+	r = FLD_MOD(r, cfg->iec60958_cfg.channel_nr, 7, 4);
+	hdmi_write_reg(av_base, HDMI_CORE_AV_I2S_CHST2, r);
+
+	r = hdmi_read_reg(av_base, HDMI_CORE_AV_I2S_CHST4);
+	r = FLD_MOD(r, cfg->iec60958_cfg.freq_sample, 3, 0);
+	r = FLD_MOD(r, cfg->iec60958_cfg.clock_accuracy, 7, 4);
+	hdmi_write_reg(av_base, HDMI_CORE_AV_I2S_CHST4, r);
+
+	r = hdmi_read_reg(av_base, HDMI_CORE_AV_I2S_CHST5);
+	r = FLD_MOD(r, cfg->iec60958_cfg.freq_sample, 7, 4);
+	r = FLD_MOD(r, cfg->iec60958_cfg.word_length, 3, 1);
+	r = FLD_MOD(r, cfg->iec60958_cfg.word_max_length, 0, 0);
+	hdmi_write_reg(av_base, HDMI_CORE_AV_I2S_CHST5, r);
 
 	r = hdmi_read_reg(av_base, HDMI_CORE_AV_I2S_IN_CTRL);
-	r = FLD_MOD(r, cfg->i2s_cfg.en_high_bitrate_aud, 7, 7);
+	r = FLD_MOD(r, cfg->en_high_bitrate_aud, 7, 7);
 	r = FLD_MOD(r, cfg->i2s_cfg.sck_edge_mode, 6, 6);
 	r = FLD_MOD(r, cfg->i2s_cfg.cbit_order, 5, 5);
 	r = FLD_MOD(r, cfg->i2s_cfg.vbit, 4, 4);
@@ -1134,12 +1159,6 @@ void hdmi_core_audio_config(struct hdmi_ip_data *ip_data,
 	r = FLD_MOD(r, cfg->i2s_cfg.shift, 0, 0);
 	hdmi_write_reg(av_base, HDMI_CORE_AV_I2S_IN_CTRL, r);
 
-	r = hdmi_read_reg(av_base, HDMI_CORE_AV_I2S_CHST5);
-	r = FLD_MOD(r, cfg->freq_sample, 7, 4);
-	r = FLD_MOD(r, cfg->i2s_cfg.word_length, 3, 1);
-	r = FLD_MOD(r, cfg->i2s_cfg.word_max_length, 0, 0);
-	hdmi_write_reg(av_base, HDMI_CORE_AV_I2S_CHST5, r);
-
 	REG_FLD_MOD(av_base, HDMI_CORE_AV_I2S_IN_LEN,
 			cfg->i2s_cfg.in_length_bits, 3, 0);
 
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
index 222cc16..4359001 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
@@ -469,11 +469,8 @@ struct hdmi_audio_dma {
 };
 
 struct hdmi_core_audio_i2s_config {
-	u8 word_max_length;
-	u8 word_length;
 	u8 in_length_bits;
 	u8 justification;
-	u8 en_high_bitrate_aud;
 	u8 sck_edge_mode;
 	u8 cbit_order;
 	u8 vbit;
@@ -483,9 +480,26 @@ struct hdmi_core_audio_i2s_config {
 	u8 active_sds;
 };
 
+/* TODO: Consider if having this is better than parsing the audio word
+ * directly from the status word */
+struct hdmi_core_audio_iec60958_config {
+	bool professional;
+	bool for_lpcm_aud;
+	bool copyright;
+	u8 emphasis;
+	u8 mode;
+	u8 category;
+	u8 source_nr;
+	u8 channel_nr;
+	u8 freq_sample;
+	u8 clock_accuracy;
+	u8 word_max_length;
+	u8 word_length;
+};
+
 struct hdmi_core_audio_config {
 	struct hdmi_core_audio_i2s_config	i2s_cfg;
-	u32 freq_sample;
+	struct hdmi_core_audio_iec60958_config iec60958_cfg;
 	bool					fs_override;
 	u32					n;
 	u32					cts;
@@ -498,6 +512,7 @@ struct hdmi_core_audio_config {
 	bool					en_dsd_audio;
 	bool					en_parallel_aud_input;
 	bool					en_spdif;
+	bool					en_high_bitrate_aud;
 };
 
 #if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
-- 
1.7.0.4

[PATCH 07/10] OMAPDSS: HDMI: Relocate N/CTS calculation

[Ricardo Neri]

The N and CTS parameters are relevant to all HDMI implmentations and
not specific to a given IP. Hence, the calculation is relocated
into the generic HDMI driver.

Also, deep color is not queried but it is still considered in the
calculation of N. This is to be changed when deep color functionality is
implemented in the driver.

Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
---
 drivers/video/omap2/dss/hdmi.c            |   42 +++++++++++++++++++++
 drivers/video/omap2/dss/ti_hdmi.h         |    1 +
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |   57 -----------------------------
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h |    2 -
 4 files changed, 43 insertions(+), 59 deletions(-)

diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 27d4fba..7fcc22f 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -598,6 +598,48 @@ static void hdmi_put_clocks(void)
 		clk_put(hdmi.sys_clk);
 }
 
+#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
+int hdmi_compute_acr(u32 sample_freq, u32 *n, u32 *cts)
+{
+	u32 deep_color;
+	u32 pclk = hdmi.ip_data.cfg.timings.timings.pixel_clock;
+
+	if (n == NULL || cts == NULL)
+		return -EINVAL;
+
+	/* TODO: When implemented, query deep color mode here. */
+	deep_color = 100;
+
+	switch (sample_freq) {
+	case 32000:
+		if ((deep_color == 125) && ((pclk == 54054)
+				|| (pclk == 74250)))
+			*n = 8192;
+		else
+			*n = 4096;
+		break;
+	case 44100:
+		*n = 6272;
+		break;
+	case 48000:
+		if ((deep_color == 125) && ((pclk == 54054)
+				|| (pclk == 74250)))
+			*n = 8192;
+		else
+			*n = 6144;
+		break;
+	default:
+		*n = 0;
+		return -EINVAL;
+	}
+
+	/* Calculate CTS. See HDMI 1.3a or 1.4a specifications */
+	*cts = pclk * (*n / 128) * deep_color / (sample_freq / 10);
+
+	return 0;
+}
+#endif
+
 /* HDMI HW IP initialisation */
 static int omapdss_hdmihw_probe(struct platform_device *pdev)
 {
diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
index 211da6f..db22a02 100644
--- a/drivers/video/omap2/dss/ti_hdmi.h
+++ b/drivers/video/omap2/dss/ti_hdmi.h
@@ -145,6 +145,7 @@ void ti_hdmi_4xxx_pll_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
 void ti_hdmi_4xxx_core_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
 void ti_hdmi_4xxx_phy_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
 #if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
+int hdmi_compute_acr(u32 sample_freq, u32 *n, u32 *cts);
 void ti_hdmi_4xxx_wp_audio_enable(struct hdmi_ip_data *ip_data, bool enable);
 void ti_hdmi_4xxx_audio_start(struct hdmi_ip_data *ip_data, bool enable);
 #endif
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
index da2806c..d587b20 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -1226,63 +1226,6 @@ void hdmi_core_audio_infoframe_config(struct hdmi_ip_data *ip_data,
 	 */
 }
 
-int hdmi_config_audio_acr(struct hdmi_ip_data *ip_data,
-				u32 sample_freq, u32 *n, u32 *cts)
-{
-	u32 r;
-	u32 deep_color = 0;
-	u32 pclk = ip_data->cfg.timings.timings.pixel_clock;
-
-	if (n == NULL || cts == NULL)
-		return -EINVAL;
-	/*
-	 * Obtain current deep color configuration. This needed
-	 * to calculate the TMDS clock based on the pixel clock.
-	 */
-	r = REG_GET(hdmi_wp_base(ip_data), HDMI_WP_VIDEO_CFG, 1, 0);
-	switch (r) {
-	case 1: /* No deep color selected */
-		deep_color = 100;
-		break;
-	case 2: /* 10-bit deep color selected */
-		deep_color = 125;
-		break;
-	case 3: /* 12-bit deep color selected */
-		deep_color = 150;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	switch (sample_freq) {
-	case 32000:
-		if ((deep_color == 125) && ((pclk == 54054)
-				|| (pclk == 74250)))
-			*n = 8192;
-		else
-			*n = 4096;
-		break;
-	case 44100:
-		*n = 6272;
-		break;
-	case 48000:
-		if ((deep_color == 125) && ((pclk == 54054)
-				|| (pclk == 74250)))
-			*n = 8192;
-		else
-			*n = 6144;
-		break;
-	default:
-		*n = 0;
-		return -EINVAL;
-	}
-
-	/* Calculate CTS. See HDMI 1.3a or 1.4a specifications */
-	*cts = pclk * (*n / 128) * deep_color / (sample_freq / 10);
-
-	return 0;
-}
-
 void ti_hdmi_4xxx_wp_audio_enable(struct hdmi_ip_data *ip_data, bool enable)
 {
 	REG_FLD_MOD(hdmi_wp_base(ip_data),
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
index 4359001..e4f973f 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
@@ -516,8 +516,6 @@ struct hdmi_core_audio_config {
 };
 
 #if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
-int hdmi_config_audio_acr(struct hdmi_ip_data *ip_data,
-				u32 sample_freq, u32 *n, u32 *cts);
 void hdmi_core_audio_infoframe_config(struct hdmi_ip_data *ip_data,
 		struct snd_cea_861_aud_if *info_aud);
 void hdmi_core_audio_config(struct hdmi_ip_data *ip_data,
-- 
1.7.0.4

[PATCH 08/10] OMAPDSS: HDMI: Add support for more audio sample rates in N/CTS calculation

[Ricardo Neri]

Add support for more sample rates when calculating N and CTS. This
covers all the audio sample rates that an HDMI source is allowed
to transmit according to the HDMI 1.4a specification.

Also, reorganize the logic for the calculation when using deep color.

Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
---
 drivers/video/omap2/dss/hdmi.c |   88 +++++++++++++++++++++++++++++++++------
 1 files changed, 74 insertions(+), 14 deletions(-)

diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 7fcc22f..bd44891 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -602,6 +602,7 @@ static void hdmi_put_clocks(void)
 int hdmi_compute_acr(u32 sample_freq, u32 *n, u32 *cts)
 {
 	u32 deep_color;
+	bool deep_color_correct = false;
 	u32 pclk = hdmi.ip_data.cfg.timings.timings.pixel_clock;
 
 	if (n == NULL || cts == NULL)
@@ -610,29 +611,88 @@ int hdmi_compute_acr(u32 sample_freq, u32 *n, u32 *cts)
 	/* TODO: When implemented, query deep color mode here. */
 	deep_color = 100;
 
+	/*
+	 * When using deep color, the default N value (as in the HDMI
+	 * specification) yields to an non-integer CTS. Hence, we
+	 * modify it while keeping the restrictions described in
+	 * section 7.2.1 of the HDMI 1.4a specification.
+	 */
 	switch (sample_freq) {
 	case 32000:
-		if ((deep_color == 125) && ((pclk == 54054)
-				|| (pclk == 74250)))
-			*n = 8192;
-		else
-			*n = 4096;
+	case 48000:
+	case 96000:
+	case 192000:
+		if (deep_color == 125)
+			if (pclk == 27027 || pclk == 74250)
+				deep_color_correct = true;
+		if (deep_color == 150)
+			if (pclk == 27027)
+				deep_color_correct = true;
 		break;
 	case 44100:
-		*n = 6272;
-		break;
-	case 48000:
-		if ((deep_color == 125) && ((pclk == 54054)
-				|| (pclk == 74250)))
-			*n = 8192;
-		else
-			*n = 6144;
+	case 88200:
+	case 176400:
+		if (deep_color == 125)
+			if (pclk == 27027)
+				deep_color_correct = true;
 		break;
 	default:
-		*n = 0;
 		return -EINVAL;
 	}
 
+	if (deep_color_correct) {
+		switch (sample_freq) {
+		case 32000:
+			*n = 8192;
+			break;
+		case 44100:
+			*n = 12544;
+			break;
+		case 48000:
+			*n = 8192;
+			break;
+		case 88200:
+			*n = 25088;
+			break;
+		case 96000:
+			*n = 16384;
+			break;
+		case 176400:
+			*n = 50176;
+			break;
+		case 192000:
+			*n = 32768;
+			break;
+		default:
+			return -EINVAL;
+		}
+	} else {
+		switch (sample_freq) {
+		case 32000:
+			*n = 4096;
+			break;
+		case 44100:
+			*n = 6272;
+			break;
+		case 48000:
+			*n = 6144;
+			break;
+		case 88200:
+			*n = 12544;
+			break;
+		case 96000:
+			*n = 12288;
+			break;
+		case 176400:
+			*n = 25088;
+			break;
+		case 192000:
+			*n = 24576;
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
 	/* Calculate CTS. See HDMI 1.3a or 1.4a specifications */
 	*cts = pclk * (*n / 128) * deep_color / (sample_freq / 10);
 
-- 
1.7.0.4

[PATCH 09/10] OMAPDSS: HDMI: OMAP4: Add an audio configuration function

[Ricardo Neri]

The generic HDMI driver does not need to know about the specific
settings of a given IP. Hence, it just passes the audio configuration
and the IP library parses such configuration and sets the IP
accordingly. This patch introduces an IP-specific audio configuration
function. Also the DMA, format and core config functions are no longer
exposed to the generic HDMI driver as they are IP-specific.

The audio configuration functions caters for 16-bit through 24-bit
audio samples with sample rates from 32kHz and up to 192kHz as well
as up to 8 audio channels.

Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
---
 drivers/video/omap2/dss/dss_features.c    |    1 +
 drivers/video/omap2/dss/ti_hdmi.h         |    7 +
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c |  226 ++++++++++++++++++++++++++++-
 drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h |   10 --
 4 files changed, 230 insertions(+), 14 deletions(-)

diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c
index 399a28a..9ed112a 100644
--- a/drivers/video/omap2/dss/dss_features.c
+++ b/drivers/video/omap2/dss/dss_features.c
@@ -500,6 +500,7 @@ static const struct ti_hdmi_ip_ops omap4_hdmi_functions = {
 #if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
 	.audio_enable		=       ti_hdmi_4xxx_wp_audio_enable,
 	.audio_start		=       ti_hdmi_4xxx_audio_start,
+	.audio_config		=	ti_hdmi_4xxx_audio_config,
 #endif
 
 };
diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
index db22a02..9864484 100644
--- a/drivers/video/omap2/dss/ti_hdmi.h
+++ b/drivers/video/omap2/dss/ti_hdmi.h
@@ -22,6 +22,8 @@
 #define _TI_HDMI_H
 
 struct hdmi_ip_data;
+struct snd_aes_iec958;
+struct snd_cea_861_aud_if;
 
 enum hdmi_pll_pwr {
 	HDMI_PLLPWRCMD_ALLOFF = 0,
@@ -114,6 +116,9 @@ struct ti_hdmi_ip_ops {
 	void (*audio_enable)(struct hdmi_ip_data *ip_data, bool start);
 
 	void (*audio_start)(struct hdmi_ip_data *ip_data, bool start);
+
+	int (*audio_config)(struct hdmi_ip_data *ip_data,
+		struct snd_aes_iec958 *iec, struct snd_cea_861_aud_if *aud_if);
 #endif
 
 };
@@ -148,5 +153,7 @@ void ti_hdmi_4xxx_phy_dump(struct hdmi_ip_data *ip_data, struct seq_file *s);
 int hdmi_compute_acr(u32 sample_freq, u32 *n, u32 *cts);
 void ti_hdmi_4xxx_wp_audio_enable(struct hdmi_ip_data *ip_data, bool enable);
 void ti_hdmi_4xxx_audio_start(struct hdmi_ip_data *ip_data, bool enable);
+int ti_hdmi_4xxx_audio_config(struct hdmi_ip_data *ip_data,
+		struct snd_aes_iec958 *iec, struct snd_cea_861_aud_if *aud_if);
 #endif
 #endif
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
index d587b20..ef96524 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
@@ -31,6 +31,7 @@
 #include <linux/gpio.h>
 #include "ti_hdmi_4xxx_ip.h"
 #include "dss.h"
+#include "dss_features.h"
 
 static inline void hdmi_write_reg(void __iomem *base_addr,
 				const u16 idx, u32 val)
@@ -1031,7 +1032,7 @@ void ti_hdmi_4xxx_phy_dump(struct hdmi_ip_data *ip_data, struct seq_file *s)
 }
 
 #if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
-void hdmi_wp_audio_config_format(struct hdmi_ip_data *ip_data,
+static void ti_hdmi_4xxx_wp_audio_config_format(struct hdmi_ip_data *ip_data,
 					struct hdmi_audio_format *aud_fmt)
 {
 	u32 r;
@@ -1050,7 +1051,7 @@ void hdmi_wp_audio_config_format(struct hdmi_ip_data *ip_data,
 	hdmi_write_reg(hdmi_wp_base(ip_data), HDMI_WP_AUDIO_CFG, r);
 }
 
-void hdmi_wp_audio_config_dma(struct hdmi_ip_data *ip_data,
+static void ti_hdmi_4xxx_wp_audio_config_dma(struct hdmi_ip_data *ip_data,
 					struct hdmi_audio_dma *aud_dma)
 {
 	u32 r;
@@ -1068,7 +1069,7 @@ void hdmi_wp_audio_config_dma(struct hdmi_ip_data *ip_data,
 	hdmi_write_reg(hdmi_wp_base(ip_data), HDMI_WP_AUDIO_CTRL, r);
 }
 
-void hdmi_core_audio_config(struct hdmi_ip_data *ip_data,
+static void ti_hdmi_4xxx_core_audio_config(struct hdmi_ip_data *ip_data,
 					struct hdmi_core_audio_config *cfg)
 {
 	u32 r;
@@ -1172,7 +1173,7 @@ void hdmi_core_audio_config(struct hdmi_ip_data *ip_data,
 	hdmi_write_reg(av_base, HDMI_CORE_AV_AUD_MODE, r);
 }
 
-void hdmi_core_audio_infoframe_config(struct hdmi_ip_data *ip_data,
+static void ti_hdmi_4xxx_core_audio_infoframe_cfg(struct hdmi_ip_data *ip_data,
 		struct snd_cea_861_aud_if *info_aud)
 {
 	u8 val;
@@ -1226,6 +1227,223 @@ void hdmi_core_audio_infoframe_config(struct hdmi_ip_data *ip_data,
 	 */
 }
 
+int ti_hdmi_4xxx_audio_config(struct hdmi_ip_data *ip_data,
+		struct snd_aes_iec958 *iec, struct snd_cea_861_aud_if *aud_if)
+{
+	struct hdmi_audio_format audio_format;
+	struct hdmi_audio_dma audio_dma;
+	struct hdmi_core_audio_config core;
+	int err, n, cts;
+	unsigned int fs_nr;
+	bool word_length_16b = false;
+
+	/*
+	 * parse IEC60958-3 channel status word
+	 */
+	core.iec60958_cfg.professional =
+		iec->status[0] & IEC958_AES0_PROFESSIONAL;
+	core.iec60958_cfg.for_lpcm_aud =
+		(iec->status[0] & IEC958_AES0_NONAUDIO)
+		>> IEC958_AES0_NONAUDIO_SHIFT;
+	core.iec60958_cfg.copyright =
+		(iec->status[0] & IEC958_AES0_CON_NOT_COPYRIGHT)
+		>> IEC958_AES0_CON_NOT_COPYRIGHT_SHIFT;
+	core.iec60958_cfg.emphasis =
+		(iec->status[0] & IEC958_AES0_CON_EMPHASIS)
+		>> IEC958_AES0_CON_EMPHASIS_SHIFT;
+	core.iec60958_cfg.mode =
+		(iec->status[0] & IEC958_AES0_CON_MODE)
+		>> IEC958_AES0_CON_MODE_SHIFT;
+	core.iec60958_cfg.category = iec->status[1];
+	core.iec60958_cfg.source_nr =
+		(iec->status[2] & IEC958_AES2_CON_SOURCE)
+		>> IEC958_AES2_CON_SOURCE_SHIFT;
+	core.iec60958_cfg.channel_nr =
+		(iec->status[2] & IEC958_AES2_CON_CHANNEL)
+		>> IEC958_AES2_CON_CHANNEL_SHIFT;
+	core.iec60958_cfg.freq_sample =
+		(iec->status[3] & IEC958_AES3_CON_FS)
+		>> IEC958_AES3_CON_FS_SHIFT;
+	core.iec60958_cfg.clock_accuracy =
+		(iec->status[3] & IEC958_AES3_CON_CLOCK)
+		>> IEC958_AES3_CON_CLOCK_SHIFT;
+	core.iec60958_cfg.word_max_length =
+		iec->status[4] & IEC958_AES4_CON_MAX_WORDLEN_24;
+	core.iec60958_cfg.word_length =
+		(iec->status[4] & IEC958_AES4_CON_WORDLEN)
+		>> IEC958_AES4_CON_WORDLEN_SHIFT;
+
+	/*
+	 * check if word length is 16-bit as several optimizations can be
+	 * performed in such case
+	 */
+	if ((core.iec60958_cfg.word_max_length !=
+		IEC958_AES4_CON_MAX_WORDLEN_24)
+		&& (core.iec60958_cfg.word_length ==
+			(IEC958_AES4_CON_WORDLEN_20_16
+				>> IEC958_AES4_CON_WORDLEN_SHIFT)))
+		word_length_16b = true;
+
+	/* I2S configuration. See Phillips' specification */
+	if (word_length_16b)
+		core.i2s_cfg.justification = HDMI_AUDIO_JUSTIFY_LEFT;
+	else
+		core.i2s_cfg.justification = HDMI_AUDIO_JUSTIFY_RIGHT;
+	/*
+	 * The I2S input word size is the IEC60958 word size shifted one
+	 * position to the right. If the word size is greater than
+	 * 20 bits, increment by one.
+	 */
+	core.i2s_cfg.in_length_bits = core.iec60958_cfg.word_length << 1;
+	if (core.iec60958_cfg.word_max_length ==
+		IEC958_AES4_CON_MAX_WORDLEN_24)
+		core.i2s_cfg.in_length_bits++;
+	core.i2s_cfg.sck_edge_mode = HDMI_AUDIO_I2S_SCK_EDGE_RISING;
+	core.i2s_cfg.cbit_order = false;
+	core.i2s_cfg.vbit = HDMI_AUDIO_I2S_VBIT_FOR_PCM;
+	core.i2s_cfg.ws_polarity = HDMI_AUDIO_I2S_WS_POLARITY_LOW_IS_LEFT;
+	core.i2s_cfg.direction = HDMI_AUDIO_I2S_MSB_SHIFTED_FIRST;
+	core.i2s_cfg.shift = HDMI_AUDIO_I2S_FIRST_BIT_SHIFT;
+
+	/* convert sample frequency to a number */
+	switch (iec->status[3] & IEC958_AES3_CON_FS) {
+	case IEC958_AES3_CON_FS_32000:
+		fs_nr = 32000;
+		break;
+	case IEC958_AES3_CON_FS_44100:
+		fs_nr = 44100;
+		break;
+	case IEC958_AES3_CON_FS_48000:
+		fs_nr = 48000;
+		break;
+	case IEC958_AES3_CON_FS_88200:
+		fs_nr = 88200;
+		break;
+	case IEC958_AES3_CON_FS_96000:
+		fs_nr = 96000;
+		break;
+	case IEC958_AES3_CON_FS_176400:
+		fs_nr = 176400;
+		break;
+	case IEC958_AES3_CON_FS_192000:
+		fs_nr = 192000;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	err = hdmi_compute_acr(fs_nr, &n, &cts);
+
+	/* Audio clock regeneration settings */
+	core.n = n;
+	core.cts = cts;
+	if (dss_has_feature(FEAT_HDMI_CTS_SWMODE)) {
+		core.aud_par_busclk = 0;
+		core.cts_mode = HDMI_AUDIO_CTS_MODE_SW;
+		core.use_mclk = dss_has_feature(FEAT_HDMI_AUDIO_USE_MCLK);
+	} else {
+		core.aud_par_busclk = (((128 * 31) - 1) << 8);
+		core.cts_mode = HDMI_AUDIO_CTS_MODE_HW;
+		core.use_mclk = true;
+	}
+
+	if (core.use_mclk)
+		core.mclk_mode = HDMI_AUDIO_MCLK_128FS;
+
+	/* Audio channels settings */
+	switch (aud_if->channel_count+1) {
+	case 2:
+		audio_format.active_chnnls_msk = 0x03;
+		break;
+	case 3:
+		audio_format.active_chnnls_msk = 0x07;
+		break;
+	case 4:
+		audio_format.active_chnnls_msk = 0x0f;
+		break;
+	case 5:
+		audio_format.active_chnnls_msk = 0x1f;
+		break;
+	case 6:
+		audio_format.active_chnnls_msk = 0x3f;
+		break;
+	case 7:
+		audio_format.active_chnnls_msk = 0x7f;
+		break;
+	case 8:
+		audio_format.active_chnnls_msk = 0xff;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/*
+	 * the HDMI IP needs to enable four stereo channels when transmitting
+	 * more than 2 audio channels
+	 */
+	if (aud_if->channel_count + 1 == 2) {
+		audio_format.stereo_channels = HDMI_AUDIO_STEREO_ONECHANNEL;
+		core.i2s_cfg.active_sds = HDMI_AUDIO_I2S_SD0_EN;
+		core.layout = HDMI_AUDIO_LAYOUT_2CH;
+	} else {
+		audio_format.stereo_channels = HDMI_AUDIO_STEREO_FOURCHANNELS;
+		core.i2s_cfg.active_sds = HDMI_AUDIO_I2S_SD0_EN |
+				HDMI_AUDIO_I2S_SD1_EN | HDMI_AUDIO_I2S_SD2_EN |
+				HDMI_AUDIO_I2S_SD3_EN;
+		core.layout = HDMI_AUDIO_LAYOUT_8CH;
+	}
+
+	core.en_spdif = false;
+	/* use sample frequency from channel status word */
+	core.fs_override = true;
+	/* enable ACR packets */
+	core.en_acr_pkt = true;
+	/* disable direct streaming digital audio */
+	core.en_dsd_audio = false;
+	/* use parallel audio interface */
+	core.en_parallel_aud_input = true;
+	/* disable high bit-rate audio */
+	core.en_high_bitrate_aud = false;
+
+	/* DMA settings */
+	if (word_length_16b)
+		audio_dma.transfer_size = 0x10;
+	else
+		audio_dma.transfer_size = 0x20;
+	audio_dma.block_size = 0xC0;
+	audio_dma.mode = HDMI_AUDIO_TRANSF_DMA;
+	audio_dma.fifo_threshold = 0x20; /* in number of samples */
+
+	/* audio FIFO format settings */
+	if (word_length_16b) {
+		audio_format.samples_per_word = HDMI_AUDIO_ONEWORD_TWOSAMPLES;
+		audio_format.sample_size = HDMI_AUDIO_SAMPLE_16BITS;
+		audio_format.justification = HDMI_AUDIO_JUSTIFY_LEFT;
+	} else {
+		audio_format.samples_per_word = HDMI_AUDIO_ONEWORD_ONESAMPLE;
+		audio_format.sample_size = HDMI_AUDIO_SAMPLE_24BITS;
+		audio_format.justification = HDMI_AUDIO_JUSTIFY_RIGHT;
+	}
+	audio_format.type = HDMI_AUDIO_TYPE_LPCM;
+	audio_format.sample_order = HDMI_AUDIO_SAMPLE_LEFT_FIRST;
+	/* disable start/stop signals of IEC 60958 blocks */
+	audio_format.en_sig_blk_strt_end = HDMI_AUDIO_BLOCK_SIG_STARTEND_ON;
+
+	/* configure DMA and audio FIFO format*/
+	ti_hdmi_4xxx_wp_audio_config_dma(ip_data, &audio_dma);
+	ti_hdmi_4xxx_wp_audio_config_format(ip_data, &audio_format);
+
+	/* configure the core*/
+	ti_hdmi_4xxx_core_audio_config(ip_data, &core);
+
+	/* configure CEA 861 audio infoframe*/
+	ti_hdmi_4xxx_core_audio_infoframe_cfg(ip_data, aud_if);
+
+	/* TODO: check video dependency (HDMI 1.4a, table 7-5) */
+
+	return 0;
+}
+
 void ti_hdmi_4xxx_wp_audio_enable(struct hdmi_ip_data *ip_data, bool enable)
 {
 	REG_FLD_MOD(hdmi_wp_base(ip_data),
diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
index e4f973f..b3454c0 100644
--- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
+++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.h
@@ -515,14 +515,4 @@ struct hdmi_core_audio_config {
 	bool					en_high_bitrate_aud;
 };
 
-#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
-void hdmi_core_audio_infoframe_config(struct hdmi_ip_data *ip_data,
-		struct snd_cea_861_aud_if *info_aud);
-void hdmi_core_audio_config(struct hdmi_ip_data *ip_data,
-					struct hdmi_core_audio_config *cfg);
-void hdmi_wp_audio_config_dma(struct hdmi_ip_data *ip_data,
-					struct hdmi_audio_dma *aud_dma);
-void hdmi_wp_audio_config_format(struct hdmi_ip_data *ip_data,
-					struct hdmi_audio_format *aud_fmt);
-#endif
 #endif
-- 
1.7.0.4

[PATCH 10/10] OMAPDSS: HDMI: Implement DSS driver interface for audio

[Ricardo Neri]

Implement the DSS device driver audio support interface in the HDMI
panel driver and generic driver. The implementation relies on the
IP-specific functions that are defined at DSS probe time.

A HW-safe spinlock is used to protect the audio functions. This is because
the audio functions may be called while holding a lock; in such case,
the panel's driver mutex is not suitable. Functions should be used
to set registers and should not wait for any other event.

Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
---
 drivers/video/omap2/dss/dss.h        |    7 +++
 drivers/video/omap2/dss/hdmi.c       |   33 +++++++++++++++
 drivers/video/omap2/dss/hdmi_panel.c |   76 ++++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+), 0 deletions(-)

diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 32ff69f..fca4490 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -520,6 +520,13 @@ int omapdss_hdmi_read_edid(u8 *buf, int len);
 bool omapdss_hdmi_detect(void);
 int hdmi_panel_init(void);
 void hdmi_panel_exit(void);
+#ifdef CONFIG_OMAP4_DSS_HDMI_AUDIO
+int hdmi_audio_enable(bool enable);
+int hdmi_audio_start(bool start);
+bool hdmi_mode_has_audio(void);
+int hdmi_audio_config(struct snd_aes_iec958 *iec,
+		struct snd_cea_861_aud_if *aud_if);
+#endif
 
 /* RFBI */
 #ifdef CONFIG_OMAP2_DSS_RFBI
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index bd44891..880509d 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -698,6 +698,39 @@ int hdmi_compute_acr(u32 sample_freq, u32 *n, u32 *cts)
 
 	return 0;
 }
+
+int hdmi_audio_enable(bool enable)
+{
+	DSSDBG("audio_enable\n");
+
+	hdmi.ip_data.ops->audio_enable(&hdmi.ip_data, enable);
+
+	return 0;
+}
+
+int hdmi_audio_start(bool start)
+{
+	DSSDBG("audio_enable\n");
+
+	hdmi.ip_data.ops->audio_start(&hdmi.ip_data, start);
+
+	return 0;
+}
+
+bool hdmi_mode_has_audio(void)
+{
+	if (hdmi.ip_data.cfg.cm.mode == HDMI_HDMI)
+		return true;
+	else
+		return false;
+}
+
+int hdmi_audio_config(struct snd_aes_iec958 *iec,
+		struct snd_cea_861_aud_if *aud_if)
+{
+	return hdmi.ip_data.ops->audio_config(&hdmi.ip_data, iec, aud_if);
+}
+
 #endif
 
 /* HDMI HW IP initialisation */
diff --git a/drivers/video/omap2/dss/hdmi_panel.c b/drivers/video/omap2/dss/hdmi_panel.c
index 533d5dc..dac1ac2 100644
--- a/drivers/video/omap2/dss/hdmi_panel.c
+++ b/drivers/video/omap2/dss/hdmi_panel.c
@@ -31,6 +31,10 @@
 
 static struct {
 	struct mutex hdmi_lock;
+#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
+	/* protects calls to HDMI driver audio functionality */
+	spinlock_t hdmi_sp_lock;
+#endif
 } hdmi;
 
 
@@ -222,6 +226,68 @@ err:
 	return r;
 }
 
+#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
+static int hdmi_panel_audio_enable(struct omap_dss_device *dssdev, bool enable)
+{
+	unsigned long flags;
+	int r;
+
+	spin_lock_irqsave(&hdmi.hdmi_sp_lock, flags);
+
+	r = hdmi_audio_enable(enable);
+
+	spin_unlock_irqrestore(&hdmi.hdmi_sp_lock, flags);
+	return r;
+}
+
+static int hdmi_panel_audio_start(struct omap_dss_device *dssdev, bool start)
+{
+	unsigned long flags;
+	int r;
+
+	spin_lock_irqsave(&hdmi.hdmi_sp_lock, flags);
+
+	r = hdmi_audio_start(start);
+
+	spin_unlock_irqrestore(&hdmi.hdmi_sp_lock, flags);
+	return r;
+}
+
+static bool hdmi_panel_audio_detect(struct omap_dss_device *dssdev)
+{
+	unsigned long flags;
+	bool r = false;
+
+	spin_lock_irqsave(&hdmi.hdmi_sp_lock, flags);
+
+	if (dssdev->state != OMAP_DSS_DISPLAY_ACTIVE)
+		goto err;
+
+	if (!hdmi_mode_has_audio())
+		goto err;
+
+	r = true;
+err:
+	spin_unlock_irqrestore(&hdmi.hdmi_sp_lock, flags);
+	return r;
+}
+
+static int hdmi_panel_audio_config(struct omap_dss_device *dssdev,
+		struct snd_aes_iec958 *iec, struct snd_cea_861_aud_if *aud_if)
+{
+	unsigned long flags;
+	int r;
+
+	spin_lock_irqsave(&hdmi.hdmi_sp_lock, flags);
+
+	r = hdmi_audio_config(iec, aud_if);
+
+	spin_unlock_irqrestore(&hdmi.hdmi_sp_lock, flags);
+	return r;
+}
+
+#endif
+
 static struct omap_dss_driver hdmi_driver = {
 	.probe		= hdmi_panel_probe,
 	.remove		= hdmi_panel_remove,
@@ -234,6 +300,12 @@ static struct omap_dss_driver hdmi_driver = {
 	.check_timings	= hdmi_check_timings,
 	.read_edid	= hdmi_read_edid,
 	.detect		= hdmi_detect,
+#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
+	.audio_enable	= hdmi_panel_audio_enable,
+	.audio_start	= hdmi_panel_audio_start,
+	.audio_detect	= hdmi_panel_audio_detect,
+	.audio_config	= hdmi_panel_audio_config,
+#endif
 	.driver			= {
 		.name   = "hdmi_panel",
 		.owner  = THIS_MODULE,
@@ -244,6 +316,10 @@ int hdmi_panel_init(void)
 {
 	mutex_init(&hdmi.hdmi_lock);
 
+#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
+	spin_lock_init(&hdmi.hdmi_sp_lock);
+#endif
+
 	omap_dss_register_driver(&hdmi_driver);
 
 	return 0;
-- 
1.7.0.4

Re: [PATCH 10/10] OMAPDSS: HDMI: Implement DSS driver interface for audio

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3573 bytes --]

On Wed, 2012-03-28 at 16:38 -0600, Ricardo Neri wrote:
> Implement the DSS device driver audio support interface in the HDMI
> panel driver and generic driver. The implementation relies on the
> IP-specific functions that are defined at DSS probe time.
> 
> A HW-safe spinlock is used to protect the audio functions. This is because

What is a "HW-safe spinlock"?

> the audio functions may be called while holding a lock; in such case,
> the panel's driver mutex is not suitable. Functions should be used
> to set registers and should not wait for any other event.

Are you sure this is the only option? What lock is being held? While a
spinlock may be ok for now, quite often enabling/disabling things do not
happen immediately, and it's much easier to do the wait synchronously.

> Signed-off-by: Ricardo Neri <ricardo.neri@ti.com>
> ---
>  drivers/video/omap2/dss/dss.h        |    7 +++
>  drivers/video/omap2/dss/hdmi.c       |   33 +++++++++++++++
>  drivers/video/omap2/dss/hdmi_panel.c |   76 ++++++++++++++++++++++++++++++++++
>  3 files changed, 116 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
> index 32ff69f..fca4490 100644
> --- a/drivers/video/omap2/dss/dss.h
> +++ b/drivers/video/omap2/dss/dss.h
> @@ -520,6 +520,13 @@ int omapdss_hdmi_read_edid(u8 *buf, int len);
>  bool omapdss_hdmi_detect(void);
>  int hdmi_panel_init(void);
>  void hdmi_panel_exit(void);
> +#ifdef CONFIG_OMAP4_DSS_HDMI_AUDIO
> +int hdmi_audio_enable(bool enable);
> +int hdmi_audio_start(bool start);
> +bool hdmi_mode_has_audio(void);
> +int hdmi_audio_config(struct snd_aes_iec958 *iec,
> +		struct snd_cea_861_aud_if *aud_if);
> +#endif
>  
>  /* RFBI */
>  #ifdef CONFIG_OMAP2_DSS_RFBI
> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> index bd44891..880509d 100644
> --- a/drivers/video/omap2/dss/hdmi.c
> +++ b/drivers/video/omap2/dss/hdmi.c
> @@ -698,6 +698,39 @@ int hdmi_compute_acr(u32 sample_freq, u32 *n, u32 *cts)
>  
>  	return 0;
>  }
> +
> +int hdmi_audio_enable(bool enable)
> +{
> +	DSSDBG("audio_enable\n");
> +
> +	hdmi.ip_data.ops->audio_enable(&hdmi.ip_data, enable);

Shouldn't this, and the others below, return the value from the called
function, instead of always returning 0?

> +
> +	return 0;
> +}
> +
> +int hdmi_audio_start(bool start)
> +{
> +	DSSDBG("audio_enable\n");
> +
> +	hdmi.ip_data.ops->audio_start(&hdmi.ip_data, start);
> +
> +	return 0;
> +}
> +
> +bool hdmi_mode_has_audio(void)
> +{
> +	if (hdmi.ip_data.cfg.cm.mode == HDMI_HDMI)
> +		return true;
> +	else
> +		return false;
> +}
> +
> +int hdmi_audio_config(struct snd_aes_iec958 *iec,
> +		struct snd_cea_861_aud_if *aud_if)
> +{
> +	return hdmi.ip_data.ops->audio_config(&hdmi.ip_data, iec, aud_if);
> +}
> +
>  #endif
>  
>  /* HDMI HW IP initialisation */
> diff --git a/drivers/video/omap2/dss/hdmi_panel.c b/drivers/video/omap2/dss/hdmi_panel.c
> index 533d5dc..dac1ac2 100644
> --- a/drivers/video/omap2/dss/hdmi_panel.c
> +++ b/drivers/video/omap2/dss/hdmi_panel.c
> @@ -31,6 +31,10 @@
>  
>  static struct {
>  	struct mutex hdmi_lock;
> +#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
> +	/* protects calls to HDMI driver audio functionality */
> +	spinlock_t hdmi_sp_lock;

What does "sp" stand for? Spinlock? Perhaps a better name would be
"audio_lock", if it's audio specific. And probably no reason to prefix
it with "hdmi", as it's inside "hdmi" struct already.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 10/10] OMAPDSS: HDMI: Implement DSS driver interface for audio

[Ricardo Neri]

On 04/23/2012 08:01 AM, Tomi Valkeinen wrote:
> On Wed, 2012-03-28 at 16:38 -0600, Ricardo Neri wrote:
>> Implement the DSS device driver audio support interface in the HDMI
>> panel driver and generic driver. The implementation relies on the
>> IP-specific functions that are defined at DSS probe time.
>>
>> A HW-safe spinlock is used to protect the audio functions. This is because
>
> What is a "HW-safe spinlock"?
Sorry, I meant a spinlock that disables the HW irqs when held:hardirq-safe.

>
>> the audio functions may be called while holding a lock; in such case,
>> the panel's driver mutex is not suitable. Functions should be used
>> to set registers and should not wait for any other event.
>
> Are you sure this is the only option? What lock is being held?
For instance, ALSA calls the start audio function while holding a 
hardirq-safe readlock. Hence, when reaching the HDMI panel start 
function, a lock is held and irqs are disabled. Using a mutex, that 
might sleep, is not correct; nor it is using an hardirq-unsafe spinlock. 
Otherwise, deadlocks and/or inverse lock ordering may arise. This 
situation was signaled by lockdep.

IMHO, as the DSS device driver does not know who is going to use it (at 
least the audio part), it should not assume that no locks are held when 
its functions are called.

> While a spinlock may be ok for now, quite often enabling/disabling things do not
> happen immediately,and it's much easier to do the wait synchronously.
I don't understand this comment. To me, holding a lock until the 
enabling function returns is synchronous. Would you please clarify?

>
>> Signed-off-by: Ricardo Neri<ricardo.neri@ti.com>
>> ---
>>   drivers/video/omap2/dss/dss.h        |    7 +++
>>   drivers/video/omap2/dss/hdmi.c       |   33 +++++++++++++++
>>   drivers/video/omap2/dss/hdmi_panel.c |   76 ++++++++++++++++++++++++++++++++++
>>   3 files changed, 116 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
>> index 32ff69f..fca4490 100644
>> --- a/drivers/video/omap2/dss/dss.h
>> +++ b/drivers/video/omap2/dss/dss.h
>> @@ -520,6 +520,13 @@ int omapdss_hdmi_read_edid(u8 *buf, int len);
>>   bool omapdss_hdmi_detect(void);
>>   int hdmi_panel_init(void);
>>   void hdmi_panel_exit(void);
>> +#ifdef CONFIG_OMAP4_DSS_HDMI_AUDIO
>> +int hdmi_audio_enable(bool enable);
>> +int hdmi_audio_start(bool start);
>> +bool hdmi_mode_has_audio(void);
>> +int hdmi_audio_config(struct snd_aes_iec958 *iec,
>> +		struct snd_cea_861_aud_if *aud_if);
>> +#endif
>>
>>   /* RFBI */
>>   #ifdef CONFIG_OMAP2_DSS_RFBI
>> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
>> index bd44891..880509d 100644
>> --- a/drivers/video/omap2/dss/hdmi.c
>> +++ b/drivers/video/omap2/dss/hdmi.c
>> @@ -698,6 +698,39 @@ int hdmi_compute_acr(u32 sample_freq, u32 *n, u32 *cts)
>>
>>   	return 0;
>>   }
>> +
>> +int hdmi_audio_enable(bool enable)
>> +{
>> +	DSSDBG("audio_enable\n");
>> +
>> +	hdmi.ip_data.ops->audio_enable(&hdmi.ip_data, enable);
>
> Shouldn't this, and the others below, return the value from the called
> function, instead of always returning 0?

Yes, at the time of writing this patch, the HDMI ops that you refer to 
returned void. In v2, I will submit a patch for the HDMI ops to return a 
result value which will also be returned by the DSS audio interface 
functions just as you point out.
>
>> +
>> +	return 0;
>> +}
>> +
>> +int hdmi_audio_start(bool start)
>> +{
>> +	DSSDBG("audio_enable\n");
>> +
>> +	hdmi.ip_data.ops->audio_start(&hdmi.ip_data, start);
>> +
>> +	return 0;
>> +}
>> +
>> +bool hdmi_mode_has_audio(void)
>> +{
>> +	if (hdmi.ip_data.cfg.cm.mode == HDMI_HDMI)
>> +		return true;
>> +	else
>> +		return false;
>> +}
>> +
>> +int hdmi_audio_config(struct snd_aes_iec958 *iec,
>> +		struct snd_cea_861_aud_if *aud_if)
>> +{
>> +	return hdmi.ip_data.ops->audio_config(&hdmi.ip_data, iec, aud_if);
>> +}
>> +
>>   #endif
>>
>>   /* HDMI HW IP initialisation */
>> diff --git a/drivers/video/omap2/dss/hdmi_panel.c b/drivers/video/omap2/dss/hdmi_panel.c
>> index 533d5dc..dac1ac2 100644
>> --- a/drivers/video/omap2/dss/hdmi_panel.c
>> +++ b/drivers/video/omap2/dss/hdmi_panel.c
>> @@ -31,6 +31,10 @@
>>
>>   static struct {
>>   	struct mutex hdmi_lock;
>> +#if defined(CONFIG_OMAP4_DSS_HDMI_AUDIO)
>> +	/* protects calls to HDMI driver audio functionality */
>> +	spinlock_t hdmi_sp_lock;
>
> What does "sp" stand for? Spinlock? Perhaps a better name would be
> "audio_lock", if it's audio specific. And probably no reason to prefix
> it with "hdmi", as it's inside "hdmi" struct already.
Yes, sp stands for spinlock. I prefixed the name with "hdmi" to align 
with the naming of the HDMI mutex. Maybe audio_lock is a better name. I 
could also submit a patch to remove the "hdmi" prefix in the mutex name.

BR,

Ricardo
>
>   Tomi

Re: [PATCH 10/10] OMAPDSS: HDMI: Implement DSS driver interface for audio

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3322 bytes --]

On Tue, 2012-04-24 at 23:48 -0500, Ricardo Neri wrote:
> On 04/23/2012 08:01 AM, Tomi Valkeinen wrote:
> > On Wed, 2012-03-28 at 16:38 -0600, Ricardo Neri wrote:
> >> Implement the DSS device driver audio support interface in the HDMI
> >> panel driver and generic driver. The implementation relies on the
> >> IP-specific functions that are defined at DSS probe time.
> >>
> >> A HW-safe spinlock is used to protect the audio functions. This is because
> >
> > What is a "HW-safe spinlock"?
> Sorry, I meant a spinlock that disables the HW irqs when held:hardirq-safe.
> 
> >
> >> the audio functions may be called while holding a lock; in such case,
> >> the panel's driver mutex is not suitable. Functions should be used
> >> to set registers and should not wait for any other event.
> >
> > Are you sure this is the only option? What lock is being held?
> For instance, ALSA calls the start audio function while holding a 
> hardirq-safe readlock. Hence, when reaching the HDMI panel start 
> function, a lock is held and irqs are disabled. Using a mutex, that 
> might sleep, is not correct; nor it is using an hardirq-unsafe spinlock. 
> Otherwise, deadlocks and/or inverse lock ordering may arise. This 
> situation was signaled by lockdep.
> 
> IMHO, as the DSS device driver does not know who is going to use it (at 
> least the audio part), it should not assume that no locks are held when 
> its functions are called.
> 
> > While a spinlock may be ok for now, quite often enabling/disabling things do not
> > happen immediately,and it's much easier to do the wait synchronously.
> I don't understand this comment. To me, holding a lock until the 
> enabling function returns is synchronous. Would you please clarify?

I meant that quite often when enabling things on hardware it takes time
until the HW is actually up and running. Perhaps a regulator needs to be
started, or such. And it's usually simpler to wait for the HW to be up
synchronously in the enable function, instead of some kind of
asynchronous mechanism. And if a function waits synchronously, a mutex
is better than spinlock.

And in that sense it's often better to define (define meaning, adding a
comment, or just mentally taking note about it) that the functions in
the API may sleep, so that the driver is free to do what is best for the
case, and it's also future-proof in a way that you can easily add
function calls that sleep to the functions in the future.

But you are also right in your previous comment, it's better to define
functions so that they never sleep, as that gives the callers the
freedom to call the funcs in atomic context.

Perhaps we can have audio_start() that never sleeps, it just enables a
few bits that start the audio. But how about audio_enable()? Is it
possible that on some OMAP version it would need to enable a regulator,
or set a gpio that's in an external i2c controlled mux chip, or such?

If so, we need to make sure it's not currently called in an atomic
context, because it would break if the function will sleep in the
future. And with "make sure" I just mean that we check the code and keep
it in mind. Or perhaps adding a comment in the header, that says
"audio_enable may sleep, other audio functions do not sleep" or such.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 10/10] OMAPDSS: HDMI: Implement DSS driver interface for audio

[Ricardo Neri]

+alsa-devel list

On 04/25/2012 01:19 AM, Tomi Valkeinen wrote:
> On Tue, 2012-04-24 at 23:48 -0500, Ricardo Neri wrote:
>> On 04/23/2012 08:01 AM, Tomi Valkeinen wrote:
>>> On Wed, 2012-03-28 at 16:38 -0600, Ricardo Neri wrote:
>>>> Implement the DSS device driver audio support interface in the HDMI
>>>> panel driver and generic driver. The implementation relies on the
>>>> IP-specific functions that are defined at DSS probe time.
>>>>
>>>> A HW-safe spinlock is used to protect the audio functions. This is because
>>>
>>> What is a "HW-safe spinlock"?
>> Sorry, I meant a spinlock that disables the HW irqs when held:hardirq-safe.
>>
>>>
>>>> the audio functions may be called while holding a lock; in such case,
>>>> the panel's driver mutex is not suitable. Functions should be used
>>>> to set registers and should not wait for any other event.
>>>
>>> Are you sure this is the only option? What lock is being held?
>> For instance, ALSA calls the start audio function while holding a
>> hardirq-safe readlock. Hence, when reaching the HDMI panel start
>> function, a lock is held and irqs are disabled. Using a mutex, that
>> might sleep, is not correct; nor it is using an hardirq-unsafe spinlock.
>> Otherwise, deadlocks and/or inverse lock ordering may arise. This
>> situation was signaled by lockdep.
>>
>> IMHO, as the DSS device driver does not know who is going to use it (at
>> least the audio part), it should not assume that no locks are held when
>> its functions are called.
>>
>>> While a spinlock may be ok for now, quite often enabling/disabling things do not
>>> happen immediately,and it's much easier to do the wait synchronously.
>> I don't understand this comment. To me, holding a lock until the
>> enabling function returns is synchronous. Would you please clarify?
>
> I meant that quite often when enabling things on hardware it takes time
> until the HW is actually up and running. Perhaps a regulator needs to be
> started, or such. And it's usually simpler to wait for the HW to be up
> synchronously in the enable function, instead of some kind of
> asynchronous mechanism. And if a function waits synchronously, a mutex
> is better than spinlock.
>
> And in that sense it's often better to define (define meaning, adding a
> comment, or just mentally taking note about it) that the functions in
> the API may sleep, so that the driver is free to do what is best for the
> case, and it's also future-proof in a way that you can easily add
> function calls that sleep to the functions in the future.
>
> But you are also right in your previous comment, it's better to define
> functions so that they never sleep, as that gives the callers the
> freedom to call the funcs in atomic context.
>
> Perhaps we can have audio_start() that never sleeps, it just enables a
> few bits that start the audio. But how about audio_enable()? Is it
> possible that on some OMAP version it would need to enable a regulator,
> or set a gpio that's in an external i2c controlled mux chip, or such?

I think it might be possible to have such a scenario if, for instance, 
an external chip is used to support DisplayPort on OMAP4/5. As 
DisplayPort can support audio-only use-cases, it would have to enable 
the adapter chip (but HDMI output would have to be enabled to feed the 
chip, though).
>
> If so, we need to make sure it's not currently called in an atomic
> context, because it would break if the function will sleep in the
> future. And with "make sure" I just mean that we check the code and keep
> it in mind. Or perhaps adding a comment in the header, that says
> "audio_enable may sleep, other audio functions do not sleep" or such.

I revisited the ALSA code. Only the audio_start function is atomic. 
Although ALSA may not be the only user, it makes sense to me to think 
that they will follow a similar approach in terms of locks.

Hence, based on that and on the reasons you describe (audio_enable 
potentially taking too long to return), Rephrasing what you stated, a 
mutex may be used for the enable/disable and config operations. Only 
start/stop would be protected by a spinlock. This should be described in 
comments in the header file. Does it make sense to you?

BR,

Ricardo
>
>   Tomi

Re: [PATCH 10/10] OMAPDSS: HDMI: Implement DSS driver interface for audio

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1383 bytes --]

On Wed, 2012-04-25 at 18:01 -0500, Ricardo Neri wrote:

> > If so, we need to make sure it's not currently called in an atomic
> > context, because it would break if the function will sleep in the
> > future. And with "make sure" I just mean that we check the code and keep
> > it in mind. Or perhaps adding a comment in the header, that says
> > "audio_enable may sleep, other audio functions do not sleep" or such.
> 
> I revisited the ALSA code. Only the audio_start function is atomic. 
> Although ALSA may not be the only user, it makes sense to me to think 
> that they will follow a similar approach in terms of locks.
> 
> Hence, based on that and on the reasons you describe (audio_enable 
> potentially taking too long to return), Rephrasing what you stated, a 
> mutex may be used for the enable/disable and config operations. Only 
> start/stop would be protected by a spinlock. This should be described in 
> comments in the header file. Does it make sense to you?

Yes. Well, you don't need to mention the locks, they are internal
implementation details. It should be enough to say that start/stop never
sleeps and the other functions may sleep.

And, this is obvious but just to be sure, note that you should use
spinlocks in all of the functions if possible. We just need to make sure
we can use mutex in the future if we need to.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]