Implement generic ASoC HDMI codec and use it in tda998x

The only change to v7: Remove the unused variable from tda998x_configure_audio().

Not much have changed since v6[1] patch series. I did everything addressed every comment, mostly by robots :), to previous series and there was one bug fixed in tda998x side. Unless there is any significant reaction to this series, I am going to put it to rest for a while now. I have other things to do.

There is currently two other patch series[2][3] that depend on the first two (ALSA-) patches of this series. The third ("ASoC:
hdmi-codec: Add audio abort() callback for video side to use") is currently not used and can be dropped if so decided. The rest depends on those the first two and adds hdmi-audio support for Beaglebone-black.

Best regards,
Jyri

[1] "[PATCH v6 0/6] Implement generic ASoC HDMI codec and use it in tda998x"

    http://mailman.alsa-project.org/pipermail/alsa-devel/2016-March/105524.html
[2] "[PATCH v6 00/10] ASoC: Add mediatek HDMI codec support" by Philipp Zabel 
    http://mailman.alsa-project.org/pipermail/alsa-devel/2016-March/105509.html

[3] "[RFC v2 0/6] sti: add audio interface to the hdmi driver"
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-January/103374.html
and "[PATCH v4 0/6] add IEC958 channel status control helpers"
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-March/105502.html
by Arnaud Pouliquen

Changes since v7
* "drm/i2c: tda998x: Register ASoC hdmi-codec and add audio DT binding"
- Remove the unused "int ret" variable from tda998x_configure_audio()

Changes since v6
* "ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders"
- Add "Acked-by: Arnaud Pouliquen <[email protected]>" and "Tested-by: Philipp Zabel <[email protected]>", no other changes
* "ALSA: pcm: add IEC958 channel status helper for hw_param"
- Added kernel-doc for snd_pcm_create_iec958_consumer_hw_params()
* "drm/i2c: tda998x: Register ASoC hdmi-codec and add audio DT binding"
- Use PTR_ERR_OR_ZERO() in tda998x_audio_codec_init()
- Fix possible uninitialized use of 'size' in tda998x_get_audio_ports()
- Store audio configuration in tda998x_audio_hw_params() instead of tda998x_configure_audio().

Changes since v5
- Rebased on top of the latest drm-next branch
- Allow 32 bit samplewidth in snd_pcm_create_iec958_consumer() and snd_pcm_create_iec958_consumer_hw_params()
- Propose new simpler DT binding for tda998x audio
- Squash tda998x audio DT binding together with hdmi-codec integration

Changes since RFC v4,
- Rebased on top of the latest drm-next branch
- Split the hdmi-codec abort functionality into a separate patch for better visibility of what it is all about
- This does not affect the tda998x patches as the abort functionality is not used
- Drop S18_3* formats from I2S_FORMATS and add a comment about formats not supported by HDMI

Changes since RFC v3,
ASoC side:
- Add "ALSA: pcm: add IEC958 channel status helper for hw_params"
- Add "tda998x: Improve tda998x_configure_audio() audio related pdata"
- use snd_pcm_create_iec958_consumer_hw_params() to construct the stream header
- Remove set_clk() callback from hdmi-codec. It is not needed for now.
- Refer to stream header in AIF as specified in HDMI standard
- Set current_stream to NULL only after video side audio_shutdown() has been called. Avoid potential race if video side attempts to abort audio at the same time.
- No need to have video side device pointer in the hdmi codec's pdata as it is found from dev->parent.
- Fix hdmi-codec enum: DAI_ID_I2C > DAI_ID_I2S
- Improve audio_startup API comment
- Make improved checkpatch happy
- BUG_ON > WARN_ON
- put */ ending the block comment to a separate line

DRM side:
- Fix tda998x get_eld() locking
- Change tda998x audio parameters in pdata to more generic, that can be readily used in tda998x_audio_config()
- Rename and restructure audio port related private data members to be more descriptive
- Require audio configuration trough ASoC hdmi-codec if HDMI audio is configured trough DT binding.

DTS:
- Increase McASP fifo usage form 1 to 32

Jyri Sarha (6):
ALSA: pcm: add IEC958 channel status helper for hw_params
ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders
ASoC: hdmi-codec: Add audio abort() callback for video side to use drm/i2c: tda998x: Improve tda998x_configure_audio() audio related
pdata
drm/i2c: tda998x: Register ASoC hdmi-codec and add audio DT binding
ARM: dts: am335x-boneblack: Add HDMI audio support

 .../devicetree/bindings/display/bridge/tda998x.txt |  18 +
 arch/arm/boot/dts/am335x-boneblack.dts             |  71 +++-
 drivers/gpu/drm/i2c/Kconfig                        |   1 +
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 275 ++++++++++++--
 include/drm/i2c/tda998x.h                          |  24 +-
 include/dt-bindings/display/tda998x.h              |   7 +
 include/sound/hdmi-codec.h                         | 104 ++++++
 include/sound/pcm_iec958.h                         |   2 +
 sound/core/pcm_iec958.c                            |  65 +++-
 sound/soc/codecs/Kconfig                           |   6 +
 sound/soc/codecs/Makefile                          |   2 +
 sound/soc/codecs/hdmi-codec.c                      | 411 +++++++++++++++++++++
 12 files changed, 914 insertions(+), 72 deletions(-)
 create mode 100644 include/dt-bindings/display/tda998x.h
 create mode 100644 include/sound/hdmi-codec.h
 create mode 100644 sound/soc/codecs/hdmi-codec.c

1.9.1

Add IEC958 channel status helper that gets the audio properties from snd_pcm_hw_params instead of snd_pcm_runtime. This is needed to produce the channel status bits already in audio stream configuration phase.

---
 include/sound/pcm_iec958.h |  2 ++
 sound/core/pcm_iec958.c    | 65 ++++++++++++++++++++++++++++++++++------------
 2 files changed, 50 insertions(+), 17 deletions(-)

index 0eed397..36f023a 100644
@@ -6,4 +6,6 @@
 int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
 	size_t len);
 
+int snd_pcm_create_iec958_consumer_hw_params(struct snd_pcm_hw_params *params,
+					     u8 *cs, size_t len);
 #endif

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Takashi?

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

This patch looks almost good to me, but...

Does it even make sense - AES doesn't have support for 32-bit samples, it can only ever truncate them down to 24-bit.

RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Well, using SNDRV_PCM_FORMAT_S32_* for 24 bit samples is seen often on real devices, mostly on PCI ones. So, adding the value 32 check there itself is valid, I suppose. It's rather due to a bad design in the current API.

The actual bits should be specified hw_params.msbits field, instead. But, not all drivers set this properly, unfortunately.

Takashi
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

That also happens on SoC environment i2s interfaces for various reasons. For instance if the i2s bit-clock for sample-rate * 24 * 2 is not available, but sample-rate * 32 * 2 is.

How about allowing the 32-bit format only if hw_params.msbits is set to 24 bits?

I can set the hw_params.msbits to 24 in ASoC hdmi-codec's hw_params, can't I? I could also fake the whole hw_params struct that I pass to snd_pcm_create_iec958_consumer_hw_params(), but would that make sense?

What ever the approach, I can of course split the 32 bit support into a separate subsequent patch, but for instance Beaglebone-black 48kHz 24-bit HDMI audio needs 32 bit format on i2s bus because of the available i2s bit-clocks.

Best regards,
Jyri
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

I'm thinking whether msbits is updated automatically upon hw_params refinement, or it's just used as the constraint. If msbits is always set, we can refer this value reliably.

But, I guess just accepting the format width 32 would be good enough as a start. The driver is responsible to fill in the right data format, and the helper doesn't care much about that.

Once when we figure out that msbits can be used reliably, we can switch to it later.

The setup of msbits is anyway good no matter whether we add the check in iec958 layer.

Yes, splitting the 32bit support is more appropriate than hiding in a patch to add the new helper.

Takashi
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

The hdmi-codec is a platform device driver to be registered from drivers of external HDMI encoders with I2S and/or spdif interface. The driver in turn registers an ASoC codec for the HDMI encoder's audio functionality.

The structures and definitions in the API header are mostly redundant copies of similar structures in ASoC headers. This is on purpose to avoid direct dependencies to ASoC structures in video side driver.

---
 include/sound/hdmi-codec.h    | 100 +++++++++++
 sound/soc/codecs/Kconfig      |   6 +
 sound/soc/codecs/Makefile     |   2 +
 sound/soc/codecs/hdmi-codec.c | 393 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 501 insertions(+)
 create mode 100644 include/sound/hdmi-codec.h
 create mode 100644 sound/soc/codecs/hdmi-codec.c

new file mode 100644
index 0000000..fc3a481
@@ -0,0 +1,100 @@
+/*
+ * hdmi-codec.h - HDMI Codec driver API
+ *
+ * Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com
+ *
+ * Author: Jyri Sarha <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#ifndef __HDMI_CODEC_H__
+#define __HDMI_CODEC_H__
+
+#include <linux/hdmi.h>
+#include <drm/drm_edid.h>
+#include <sound/asoundef.h>
+#include <uapi/sound/asound.h>
+
+/*
+ * Protocol between ASoC cpu-dai and HDMI-encoder
+ */
+struct hdmi_codec_daifmt {
+	enum {
+		HDMI_I2S,
+		HDMI_RIGHT_J,
+		HDMI_LEFT_J,
+		HDMI_DSP_A,
+		HDMI_DSP_B,
+		HDMI_AC97,
+		HDMI_SPDIF,
+	} fmt;
+	int bit_clk_inv:1;
+	int frame_clk_inv:1;
+	int bit_clk_master:1;
+	int frame_clk_master:1;
+};
+
+/*
+ * HDMI audio parameters
+ */
+struct hdmi_codec_params {
+	struct hdmi_audio_infoframe cea;
+	struct snd_aes_iec958 iec;
+	int sample_rate;
+	int sample_width;
+	int channels;
+};
+
+struct hdmi_codec_ops {
+	/*
+	 * Called when ASoC starts an audio stream setup.
+	 * Optional
+	 */
+	int (*audio_startup)(struct device *dev);
+
+	/*
+	 * Configures HDMI-encoder for audio stream.
+	 * Mandatory
+	 */
+	int (*hw_params)(struct device *dev,
+			 struct hdmi_codec_daifmt *fmt,
+			 struct hdmi_codec_params *hparms);
+
+	/*
+	 * Shuts down the audio stream.
+	 * Mandatory
+	 */
+	void (*audio_shutdown)(struct device *dev);
+
+	/*
+	 * Mute/unmute HDMI audio stream.
+	 * Optional
+	 */
+	int (*digital_mute)(struct device *dev, bool enable);
+
+	/*
+	 * Provides EDID-Like-Data from connected HDMI device.
+	 * Optional
+	 */
+	int (*get_eld)(struct device *dev, uint8_t *buf, size_t len);
+};
+
+/* HDMI codec initalization data */
+struct hdmi_codec_pdata {
+	const struct hdmi_codec_ops *ops;
+	uint i2s:1;
+	uint spdif:1;
+	int max_i2s_channels;
+};
+
+#define HDMI_CODEC_DRV_NAME "hdmi-audio-codec"
+
+#endif /* __HDMI_CODEC_H__ */

index 50693c8..62b62fe 100644
@@ -86,6 +86,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_MC13783 if MFD_MC13XXX
 	select SND_SOC_ML26124 if I2C
 	select SND_SOC_NAU8825 if I2C
+	select SND_SOC_HDMI_CODEC
 	select SND_SOC_PCM1681 if I2C
 	select SND_SOC_PCM179X if SPI_MASTER
 	select SND_SOC_PCM3008

@@ -473,6 +474,11 @@ config SND_SOC_BT_SCO
 config SND_SOC_DMIC
 	tristate
 
+config SND_SOC_HDMI_CODEC
+       tristate
+       select SND_PCM_ELD
+       select SND_PCM_IEC958
+
 config SND_SOC_ES8328
 	tristate "Everest Semi ES8328 CODEC"
 

index d44f7d3..5f7b002 100644
@@ -79,6 +79,7 @@ snd-soc-max9850-objs := max9850.o
 snd-soc-mc13783-objs := mc13783.o
 snd-soc-ml26124-objs := ml26124.o
 snd-soc-nau8825-objs := nau8825.o
+snd-soc-hdmi-codec-objs := hdmi-codec.o
 snd-soc-pcm1681-objs := pcm1681.o
 snd-soc-pcm179x-codec-objs := pcm179x.o
 snd-soc-pcm3008-objs := pcm3008.o

@@ -283,6 +284,7 @@ obj-$(CONFIG_SND_SOC_MAX9850)	+= snd-soc-max9850.o
 obj-$(CONFIG_SND_SOC_MC13783)	+= snd-soc-mc13783.o
 obj-$(CONFIG_SND_SOC_ML26124)	+= snd-soc-ml26124.o
 obj-$(CONFIG_SND_SOC_NAU8825)   += snd-soc-nau8825.o
+obj-$(CONFIG_SND_SOC_HDMI_CODEC)	+= snd-soc-hdmi-codec.o
 obj-$(CONFIG_SND_SOC_PCM1681)	+= snd-soc-pcm1681.o
 obj-$(CONFIG_SND_SOC_PCM179X)	+= snd-soc-pcm179x-codec.o
 obj-$(CONFIG_SND_SOC_PCM3008)	+= snd-soc-pcm3008.o

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Add audio abort() callback, that is provided at audio stream start, for video side. This is for video side to use in case there is a pressing need to tear down the audio playback for some reason.

---
 include/sound/hdmi-codec.h    |  8 ++++++--
 sound/soc/codecs/hdmi-codec.c | 20 +++++++++++++++++++-
 2 files changed, 25 insertions(+), 3 deletions(-)

index fc3a481..15fe70f 100644
@@ -55,10 +55,14 @@ struct hdmi_codec_params {
 
 struct hdmi_codec_ops {
 	/*
-	 * Called when ASoC starts an audio stream setup.
+	 * Called when ASoC starts an audio stream setup. The call
+	 * provides an audio abort callback for stoping an ongoing
+	 * stream from video side driver if the HDMI audio becomes
+	 * unavailable.
 	 * Optional
 	 */
-	int (*audio_startup)(struct device *dev);
+	int (*audio_startup)(struct device *dev,
+			     void (*abort_cb)(struct device *dev));
 
 	/*
 	 * Configures HDMI-encoder for audio stream.

index bc47b9a..cc08097 100644
@@ -47,6 +47,23 @@ enum {
 	DAI_ID_SPDIF,
 };
 
+static void hdmi_codec_abort(struct device *dev)
+{
+	struct hdmi_codec_priv *hcp = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "%s()\n", __func__);
+
+	mutex_lock(&hcp->current_stream_lock);
+	if (hcp->current_stream && hcp->current_stream->runtime &&
+	    snd_pcm_running(hcp->current_stream)) {
+		dev_info(dev, "HDMI audio playback aborted\n");
+		snd_pcm_stream_lock_irq(hcp->current_stream);
+		snd_pcm_stop(hcp->current_stream, SNDRV_PCM_STATE_DISCONNECTED);
+		snd_pcm_stream_unlock_irq(hcp->current_stream);
+	}
+	mutex_unlock(&hcp->current_stream_lock);
+}
+
 static int hdmi_codec_new_stream(struct snd_pcm_substream *substream,
 				 struct snd_soc_dai *dai)
 {

@@ -78,7 +95,8 @@ static int hdmi_codec_startup(struct snd_pcm_substream *substream,
 		return ret;
 
 	if (hcp->hcd.ops->audio_startup) {
-		ret = hcp->hcd.ops->audio_startup(dai->dev->parent);
+		ret = hcp->hcd.ops->audio_startup(dai->dev->parent,
+						  hdmi_codec_abort);
 		if (ret) {
 			mutex_lock(&hcp->current_stream_lock);
 			hcp->current_stream = NULL;

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Define struct tda998x_audio_params in include/drm/i2c/tda998x.h and use it in pdata and for tda998x_configure_audio() parameters. Also updates tda998x_write_aif() to take struct hdmi_audio_infoframe * directly as a parameter.

---
 drivers/gpu/drm/i2c/tda998x_drv.c | 77 ++++++++++++++++++++-------------------
 include/drm/i2c/tda998x.h         | 24 +++++++-----
 2 files changed, 53 insertions(+), 48 deletions(-)

index f4315bc..f97b748 100644
@@ -41,7 +41,7 @@ struct tda998x_priv {
 	u8 vip_cntrl_0;
 	u8 vip_cntrl_1;
 	u8 vip_cntrl_2;
-	struct tda998x_encoder_params params;
+	struct tda998x_audio_params audio_params;
 
 	wait_queue_head_t wq_edid;
 	volatile int wq_edid_wait;

@@ -666,26 +666,16 @@ tda998x_write_if(struct tda998x_priv *priv, u8 bit, u16 addr,
 	reg_set(priv, REG_DIP_IF_FLAGS, bit);
 }
 
-static void
-tda998x_write_aif(struct tda998x_priv *priv, struct tda998x_encoder_params *p)
+static int tda998x_write_aif(struct tda998x_priv *priv,
+			     struct hdmi_audio_infoframe *cea)
 {
 	union hdmi_infoframe frame;
 
-	hdmi_audio_infoframe_init(&frame.audio);
-
-	frame.audio.channels = p->audio_frame[1] & 0x07;
-	frame.audio.channel_allocation = p->audio_frame[4];
-	frame.audio.level_shift_value = (p->audio_frame[5] & 0x78) >> 3;
-	frame.audio.downmix_inhibit = (p->audio_frame[5] & 0x80) >> 7;
-
-	/*
-	 * L-PCM and IEC61937 compressed audio shall always set sample
-	 * frequency to "refer to stream".  For others, see the HDMI
-	 * specification.
-	 */
-	frame.audio.sample_frequency = (p->audio_frame[2] & 0x1c) >> 2;
+	frame.audio = *cea;
 
 	tda998x_write_if(priv, DIP_IF_FLAGS_IF4, REG_IF4_HB0, &frame);
+
+	return 0;
 }
 
 static void

@@ -710,20 +700,21 @@ static void tda998x_audio_mute(struct tda998x_priv *priv, bool on)
 	}
 }
 
-static void
+static int
 tda998x_configure_audio(struct tda998x_priv *priv,
-		struct drm_display_mode *mode, struct tda998x_encoder_params *p)
+			struct tda998x_audio_params *params,
+			unsigned mode_clock)
 {
 	u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv;
 	u32 n;
 
 	/* Enable audio ports */
-	reg_write(priv, REG_ENA_AP, p->audio_cfg);
-	reg_write(priv, REG_ENA_ACLK, p->audio_clk_cfg);
+	reg_write(priv, REG_ENA_AP, params->config);
 
 	/* Set audio input source */
-	switch (p->audio_format) {
+	switch (params->format) {
 	case AFMT_SPDIF:
+		reg_write(priv, REG_ENA_ACLK, 0);
 		reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_SPDIF);
 		clksel_aip = AIP_CLKSEL_AIP_SPDIF;
 		clksel_fs = AIP_CLKSEL_FS_FS64SPDIF;

@@ -731,15 +722,29 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 		break;
 
 	case AFMT_I2S:
+		reg_write(priv, REG_ENA_ACLK, 1);
 		reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S);
 		clksel_aip = AIP_CLKSEL_AIP_I2S;
 		clksel_fs = AIP_CLKSEL_FS_ACLK;
-		cts_n = CTS_N_M(3) | CTS_N_K(3);
+		switch (params->sample_width) {
+		case 16:
+			cts_n = CTS_N_M(3) | CTS_N_K(1);
+			break;
+		case 18:
+		case 20:
+		case 24:
+			cts_n = CTS_N_M(3) | CTS_N_K(2);
+			break;
+		default:
+		case 32:
+			cts_n = CTS_N_M(3) | CTS_N_K(3);
+			break;
+		}
 		break;
 
 	default:
 		BUG();
-		return;
+		return -EINVAL;
 	}
 
 	reg_write(priv, REG_AIP_CLKSEL, clksel_aip);

@@ -755,11 +760,11 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	 * assume 100MHz requires larger divider.
 	 */
 	adiv = AUDIO_DIV_SERCLK_8;
-	if (mode->clock > 100000)
+	if (mode_clock > 100000)
 		adiv++;			/* AUDIO_DIV_SERCLK_16 */
 
 	/* S/PDIF asks for a larger divider */
-	if (p->audio_format == AFMT_SPDIF)
+	if (params->format == AFMT_SPDIF)
 		adiv++;			/* AUDIO_DIV_SERCLK_16 or _32 */
 
 	reg_write(priv, REG_AUDIO_DIV, adiv);

@@ -768,7 +773,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	 * This is the approximate value of N, which happens to be
 	 * the recommended values for non-coherent clocks.
 	 */
-	n = 128 * p->audio_sample_rate / 1000;
+	n = 128 * params->sample_rate / 1000;
 
 	/* Write the CTS and N values */
 	buf[0] = 0x44;

@@ -787,19 +792,13 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
 
 	/* Write the channel status */
-	buf[0] = IEC958_AES0_CON_NOT_COPYRIGHT;
-	buf[1] = 0x00;
-	buf[2] = IEC958_AES3_CON_FS_NOTID;
-	buf[3] = IEC958_AES4_CON_ORIGFS_NOTID |
-			IEC958_AES4_CON_MAX_WORDLEN_24;
-	reg_write_range(priv, REG_CH_STAT_B(0), buf, 4);
+	reg_write_range(priv, REG_CH_STAT_B(0), params->status, 4);
 
 	tda998x_audio_mute(priv, true);
 	msleep(20);
 	tda998x_audio_mute(priv, false);
 
-	/* Write the audio information packet */
-	tda998x_write_aif(priv, p);
+	return tda998x_write_aif(priv, &params->cea);
 }
 
 /* DRM encoder functions */

@@ -820,7 +819,7 @@ static void tda998x_encoder_set_config(struct tda998x_priv *priv,
 			    VIP_CNTRL_2_SWAP_F(p->swap_f) |
 			    (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
 
-	priv->params = *p;
+	priv->audio_params = p->audio;
 }
 
 static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)

@@ -1057,9 +1056,11 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 
 		tda998x_write_avi(priv, adjusted_mode);
 
-		if (priv->params.audio_cfg)
-			tda998x_configure_audio(priv, adjusted_mode,
-						&priv->params);
+		if (priv->audio_params.format != AFMT_UNUSED) {
+			tda998x_configure_audio(priv,
+						&priv->audio_params,
+						adjusted_mode->clock);
+		}
 	}
 }
 

index 3e419d9..24be7aa 100644
@@ -1,6 +1,19 @@
 #ifndef __DRM_I2C_TDA998X_H__
 #define __DRM_I2C_TDA998X_H__
 
+#define AFMT_UNUSED	0
+#define AFMT_SPDIF	1
+#define AFMT_I2S	2
+
+struct tda998x_audio_params {
+	u8 config;
+	u8 format;
+	unsigned sample_width;
+	unsigned sample_rate;
+	struct hdmi_audio_infoframe cea;
+	u8 status[4];
+};
+
 struct tda998x_encoder_params {
 	u8 swap_b:3;
 	u8 mirr_b:1;

@@ -15,16 +28,7 @@ struct tda998x_encoder_params {
 	u8 swap_e:3;
 	u8 mirr_e:1;
 
-	u8 audio_cfg;
-	u8 audio_clk_cfg;
-	u8 audio_frame[6];
-
-	enum {
-		AFMT_SPDIF,
-		AFMT_I2S
-	} audio_format;
-
-	unsigned audio_sample_rate;
+	struct tda998x_audio_params audio;
 };
 
 #endif

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Register ASoC HDMI codec for audio functionality and adds device tree binding for audio configuration.

With the registered HDMI codec the tda998x node can be used like a regular codec node in ASoC card configurations. HDMI audio info-frame and audio stream header is generated by the ASoC HDMI codec. The codec also applies constraints for available sample-rates based on Edid Like Data from the display. The device tree binding document has been updated [1].

Part of this patch has been inspired by Jean Francoise's "drm/i2c: tda998x:
Add support of a DT graph of ports"-patch [2]. There may still be some identical lines left from the original patch and some of the ideas have come from there.

[1] Documentation/devicetree/bindings/display/bridge/tda998x.txt
[2] http://mailman.alsa-project.org/pipermail/alsa-devel/2015-July/095255.html

---
 .../devicetree/bindings/display/bridge/tda998x.txt |  18 ++
 drivers/gpu/drm/i2c/Kconfig                        |   1 +
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 198 ++++++++++++++++++++-
 include/drm/i2c/tda998x.h                          |   4 +-
 include/dt-bindings/display/tda998x.h              |   7 +
 5 files changed, 223 insertions(+), 5 deletions(-)
 create mode 100644 include/dt-bindings/display/tda998x.h

index e178e6b..24cc246 100644
@@ -21,8 +21,19 @@ Optional properties:
   - video-ports: 24 bits value which defines how the video controller
 	output is wired to the TDA998x input - default: <0x230145>
 
+  - audio-ports: array of 8-bit values, 2 values per one DAI[1].
+	The first value defines the DAI type: TDA998x_SPDIF or TDA998x_I2S[2].
+	The second value defines the tda998x AP_ENA reg content when the DAI
+	in question is used. The implementation allows one or two DAIs. If two
+	DAIs are defined, they must be of different type.
+
+[1] Documentation/sound/alsa/soc/DAI.txt
+[2] include/dt-bindings/display/tda998x.h
+
 Example:
 
+#include <dt-bindings/display/tda998x.h>
+
 	tda998x: hdmi-encoder {
 		compatible = "nxp,tda998x";
 		reg = <0x70>;

@@ -30,4 +41,11 @@ Example:
 		interrupts = <27 2>;		/* falling edge */
 		pinctrl-0 = <&pmx_camera>;
 		pinctrl-names = "default";
+		video-ports = <0x230145>;
+
+		#sound-dai-cells = <2>;
+			     /*	DAI-format	AP_ENA reg value */
+		audio-ports = <	TDA998x_SPDIF	0x04
+				TDA998x_I2S	0x03>;
+
 	};

index 22c7ed6..088f278 100644
@@ -28,6 +28,7 @@ config DRM_I2C_SIL164
 config DRM_I2C_NXP_TDA998X
 	tristate "NXP Semiconductors TDA998X HDMI encoder"
 	default m if DRM_TILCDC
+	select SND_SOC_HDMI_CODEC if SND_SOC
 	help
 	  Support for NXP Semiconductors TDA998X HDMI encoders.
 

index 24be7aa..b575332 100644
@@ -1,9 +1,9 @@
 #ifndef __DRM_I2C_TDA998X_H__
 #define __DRM_I2C_TDA998X_H__
 
+#include <dt-bindings/display/tda998x.h>
+
 #define AFMT_UNUSED	0
-#define AFMT_SPDIF	1
-#define AFMT_I2S	2
 
 struct tda998x_audio_params {
 	u8 config;

new file mode 100644
index 0000000..38ef741
@@ -0,0 +1,7 @@
+#ifndef _DT_BINDINGS_TDA998X_H
+#define _DT_BINDINGS_TDA998X_H
+
+#define AFMT_SPDIF	1
+#define AFMT_I2S	2
+
+#endif /*_DT_BINDINGS_TDA998X_H */

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Add HDMI audio support. Adds mcasp0_pins, clk_mcasp0_fixed, clk_mcasp0, mcasp0, sound node, and updates the tda19988 node to follow the new binding.

---
 arch/arm/boot/dts/am335x-boneblack.dts | 71 ++++++++++++++++++++++++++++++++--
 1 file changed, 67 insertions(+), 4 deletions(-)

index 55c0e95..2bae4d1 100644
@@ -9,6 +9,7 @@
 
 #include "am33xx.dtsi"
 #include "am335x-bone-common.dtsi"
+#include <dt-bindings/display/tda998x.h>
 
 / {
 	model = "TI AM335x BeagleBone Black";

@@ -64,6 +65,16 @@
 			AM33XX_IOPAD(0x9b0, PIN_OUTPUT_PULLDOWN | MUX_MODE3)	/* xdma_event_intr0 */
 		>;
 	};
+
+	mcasp0_pins: mcasp0_pins {
+		pinctrl-single,pins = <
+			AM33XX_IOPAD(0x9ac, PIN_INPUT_PULLUP | MUX_MODE0) /* mcasp0_ahcklx.mcasp0_ahclkx */
+			AM33XX_IOPAD(0x99c, PIN_OUTPUT_PULLDOWN | MUX_MODE2) /* mcasp0_ahclkr.mcasp0_axr2*/
+			AM33XX_IOPAD(0x994, PIN_OUTPUT_PULLUP | MUX_MODE0) /* mcasp0_fsx.mcasp0_fsx */
+			AM33XX_IOPAD(0x990, PIN_OUTPUT_PULLDOWN | MUX_MODE0) /* mcasp0_aclkx.mcasp0_aclkx */
+			AM33XX_IOPAD(0x86c, PIN_OUTPUT_PULLDOWN | MUX_MODE7) /* gpmc_a11.GPIO1_27 */
+		>;
+	};
 };
 
 &lcdc {

@@ -76,16 +87,22 @@
 };
 
 &i2c0 {
-	tda19988 {
+	tda19988: tda19988 {
 		compatible = "nxp,tda998x";
 		reg = <0x70>;
+
 		pinctrl-names = "default", "off";
 		pinctrl-0 = <&nxp_hdmi_bonelt_pins>;
 		pinctrl-1 = <&nxp_hdmi_bonelt_off_pins>;
 
-		port {
-			hdmi_0: endpoint@0 {
-				remote-endpoint = <&lcdc_0>;
+		#sound-dai-cells = <0>;
+		audio-ports = <	AFMT_I2S	0x03>;
+
+		ports {
+			port@0 {
+				hdmi_0: endpoint@0 {
+					remote-endpoint = <&lcdc_0>;
+				};
 			};
 		};
 	};

@@ -94,3 +111,49 @@
 &rtc {
 	system-power-controller;
 };
+
+&mcasp0	{
+	#sound-dai-cells = <0>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&mcasp0_pins>;
+	status = "okay";
+	op-mode = <0>;	/* MCASP_IIS_MODE */
+	tdm-slots = <2>;
+	serial-dir = <	/* 0: INACTIVE, 1: TX, 2: RX */
+			0 0 1 0
+		>;
+	tx-num-evt = <32>;
+	rx-num-evt = <32>;
+};
+
+/ {
+	clk_mcasp0_fixed: clk_mcasp0_fixed {
+		#clock-cells = <0>;
+		compatible = "fixed-clock";
+		clock-frequency = <24576000>;
+	};
+
+	clk_mcasp0: clk_mcasp0 {
+		#clock-cells = <0>;
+		compatible = "gpio-gate-clock";
+		clocks = <&clk_mcasp0_fixed>;
+		enable-gpios = <&gpio1 27 0>; /* BeagleBone Black Clk enable on GPIO1_27 */
+	};
+
+	sound {
+		compatible = "simple-audio-card";
+		simple-audio-card,name = "TI BeagleBone Black";
+		simple-audio-card,format = "i2s";
+		simple-audio-card,bitclock-master = <&dailink0_master>;
+		simple-audio-card,frame-master = <&dailink0_master>;
+
+		dailink0_master: simple-audio-card,cpu {
+			sound-dai = <&mcasp0>;
+			clocks = <&clk_mcasp0>;
+		};
+
+		simple-audio-card,codec {
+			sound-dai = <&tda19988>;
+		};
+	};
+};

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Well, I just left it there. The node parsing code should handle the container if it follows graph[1] binding. Putting the ports container node is equally correct as leaving it out, but surely I can leave it out next time (when ever it comes).

I am still not sure about the "must" part. From all that I have read[2] the unit number should be the same as reg property if possible. But then again if the reg property consists of multiple addresses, how do you put them in the unit address? To me no unit address at all is an analogue situation and I see no point in adding an artificial reg property if I need multiple nodes by same name. This actually happens quite often with sound-nodes when I have multiple audio devices in the same system.

But still, there is no need for the unit address here and it should be removed. I'll do that next time.

Best regard,
Jyri

[1] Documentation/devicetree/bindings/graph.txt
[2] http://devicetree.org/Device_Tree_Usage