Implement generic ASoC HDMI codec and use it in tda998x

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.

Signed-off-by Jyri Sarha <[email protected]>

---
 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

include/sound/hdmi-codec.h new file +100 -0

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__ */

sound/soc/codecs/Kconfig +6 -0

index cfdafc4..4d5915e 100644
@@ -82,6 +82,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_PCM1792A if SPI_MASTER
 	select SND_SOC_PCM3008

@@ -454,6 +455,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"
 

sound/soc/codecs/Makefile +2 -0

index f632fc4..49c1824 100644
@@ -75,6 +75,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-pcm1792a-codec-objs := pcm1792a.o
 snd-soc-pcm3008-objs := pcm3008.o

@@ -270,6 +271,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_PCM1792A)	+= snd-soc-pcm1792a-codec.o
 obj-$(CONFIG_SND_SOC_PCM3008)	+= snd-soc-pcm3008.o

sound/soc/codecs/hdmi-codec.c new file +393 -0

new file mode 100644
index 0000000..bc47b9a
@@ -0,0 +1,393 @@
+/*
+ * ALSA SoC codec for HDMI encoder drivers
+ * Copyright (C) 2015 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.
+ */
+#include <linux/module.h>
+#include <linux/string.h>
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/pcm_drm_eld.h>
+#include <sound/hdmi-codec.h>
+#include <sound/pcm_iec958.h>
+
+#include <drm/drm_crtc.h> /* This is only to get MAX_ELD_BYTES */
+
+struct hdmi_codec_priv {
+	struct hdmi_codec_pdata hcd;
+	struct snd_soc_dai_driver *daidrv;
+	struct hdmi_codec_daifmt daifmt[2];
+	struct mutex current_stream_lock;
+	struct snd_pcm_substream *current_stream;
+	struct snd_pcm_hw_constraint_list ratec;
+	uint8_t eld[MAX_ELD_BYTES];
+};
+
+static const struct snd_soc_dapm_widget hdmi_widgets[] = {
+	SND_SOC_DAPM_OUTPUT("TX"),
+};
+
+static const struct snd_soc_dapm_route hdmi_routes[] = {
+	{ "TX", NULL, "Playback" },
+};
+
+enum {
+	DAI_ID_I2S = 0,
+	DAI_ID_SPDIF,
+};
+
+static int hdmi_codec_new_stream(struct snd_pcm_substream *substream,
+				 struct snd_soc_dai *dai)
+{
+	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
+	int ret = 0;
+
+	mutex_lock(&hcp->current_stream_lock);
+	if (!hcp->current_stream) {
+		hcp->current_stream = substream;
+	} else if (hcp->current_stream != substream) {
+		dev_err(dai->dev, "Only one simultaneous stream supported!\n");
+		ret = -EINVAL;
+	}
+	mutex_unlock(&hcp->current_stream_lock);
+
+	return ret;
+}
+
+static int hdmi_codec_startup(struct snd_pcm_substream *substream,
+			      struct snd_soc_dai *dai)
+{
+	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
+	int ret = 0;
+
+	dev_dbg(dai->dev, "%s()\n", __func__);
+
+	ret = hdmi_codec_new_stream(substream, dai);
+	if (ret)
+		return ret;
+
+	if (hcp->hcd.ops->audio_startup) {
+		ret = hcp->hcd.ops->audio_startup(dai->dev->parent);
+		if (ret) {
+			mutex_lock(&hcp->current_stream_lock);
+			hcp->current_stream = NULL;
+			mutex_unlock(&hcp->current_stream_lock);
+			return ret;
+		}
+	}
+
+	if (hcp->hcd.ops->get_eld) {
+		ret = hcp->hcd.ops->get_eld(dai->dev->parent, hcp->eld,
+					    sizeof(hcp->eld));
+
+		if (!ret) {
+			ret = snd_pcm_hw_constraint_eld(substream->runtime,
+							hcp->eld);
+			if (ret)
+				return ret;
+		}
+	}
+	return 0;
+}
+
+static void hdmi_codec_shutdown(struct snd_pcm_substream *substream,
+				struct snd_soc_dai *dai)
+{
+	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
+
+	dev_dbg(dai->dev, "%s()\n", __func__);
+
+	WARN_ON(hcp->current_stream != substream);
+
+	hcp->hcd.ops->audio_shutdown(dai->dev->parent);
+
+	mutex_lock(&hcp->current_stream_lock);
+	hcp->current_stream = NULL;
+	mutex_unlock(&hcp->current_stream_lock);
+}
+
+static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
+				struct snd_pcm_hw_params *params,
+				struct snd_soc_dai *dai)
+{
+	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
+	struct hdmi_codec_params hp = {
+		.iec = {
+			.status = { 0 },
+			.subcode = { 0 },
+			.pad = 0,
+			.dig_subframe = { 0 },
+		}
+	};
+	int ret;
+
+	dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__,
+		params_width(params), params_rate(params),
+		params_channels(params));
+
+	ret = snd_pcm_create_iec958_consumer_hw_params(params, hp.iec.status,
+						       sizeof(hp.iec.status));
+	if (ret < 0) {
+		dev_err(dai->dev, "Creating IEC958 channel status failed %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = hdmi_codec_new_stream(substream, dai);
+	if (ret)
+		return ret;
+
+	hdmi_audio_infoframe_init(&hp.cea);
+	hp.cea.channels = params_channels(params);
+	hp.cea.coding_type = HDMI_AUDIO_CODING_TYPE_STREAM;
+	hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM;
+	hp.cea.sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM;
+
+	hp.sample_width = params_width(params);
+	hp.sample_rate = params_rate(params);
+	hp.channels = params_channels(params);
+
+	return hcp->hcd.ops->hw_params(dai->dev->parent, &hcp->daifmt[dai->id],
+				       &hp);
+}
+
+static int hdmi_codec_set_fmt(struct snd_soc_dai *dai,
+			      unsigned int fmt)
+{
+	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
+	struct hdmi_codec_daifmt cf = { 0 };
+	int ret = 0;
+
+	dev_dbg(dai->dev, "%s()\n", __func__);
+
+	if (dai->id == DAI_ID_SPDIF) {
+		cf.fmt = HDMI_SPDIF;
+	} else {
+		switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
+		case SND_SOC_DAIFMT_CBM_CFM:
+			cf.bit_clk_master = 1;
+			cf.frame_clk_master = 1;
+			break;
+		case SND_SOC_DAIFMT_CBS_CFM:
+			cf.frame_clk_master = 1;
+			break;
+		case SND_SOC_DAIFMT_CBM_CFS:
+			cf.bit_clk_master = 1;
+			break;
+		case SND_SOC_DAIFMT_CBS_CFS:
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
+		case SND_SOC_DAIFMT_NB_NF:
+			break;
+		case SND_SOC_DAIFMT_NB_IF:
+			cf.frame_clk_inv = 1;
+			break;
+		case SND_SOC_DAIFMT_IB_NF:
+			cf.bit_clk_inv = 1;
+			break;
+		case SND_SOC_DAIFMT_IB_IF:
+			cf.frame_clk_inv = 1;
+			cf.bit_clk_inv = 1;
+			break;
+		}
+
+		switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+		case SND_SOC_DAIFMT_I2S:
+			cf.fmt = HDMI_I2S;
+			break;
+		case SND_SOC_DAIFMT_DSP_A:
+			cf.fmt = HDMI_DSP_A;
+			break;
+		case SND_SOC_DAIFMT_DSP_B:
+			cf.fmt = HDMI_DSP_B;
+			break;
+		case SND_SOC_DAIFMT_RIGHT_J:
+			cf.fmt = HDMI_RIGHT_J;
+			break;
+		case SND_SOC_DAIFMT_LEFT_J:
+			cf.fmt = HDMI_LEFT_J;
+			break;
+		case SND_SOC_DAIFMT_AC97:
+			cf.fmt = HDMI_AC97;
+			break;
+		default:
+			dev_err(dai->dev, "Invalid DAI interface format\n");
+			return -EINVAL;
+		}
+	}
+
+	hcp->daifmt[dai->id] = cf;
+
+	return ret;
+}
+
+static int hdmi_codec_digital_mute(struct snd_soc_dai *dai, int mute)
+{
+	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
+
+	dev_dbg(dai->dev, "%s()\n", __func__);
+
+	if (hcp->hcd.ops->digital_mute)
+		return hcp->hcd.ops->digital_mute(dai->dev->parent, mute);
+
+	return 0;
+}
+
+static const struct snd_soc_dai_ops hdmi_dai_ops = {
+	.startup	= hdmi_codec_startup,
+	.shutdown	= hdmi_codec_shutdown,
+	.hw_params	= hdmi_codec_hw_params,
+	.set_fmt	= hdmi_codec_set_fmt,
+	.digital_mute	= hdmi_codec_digital_mute,
+};
+
+
+#define HDMI_RATES	(SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
+			 SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\
+			 SNDRV_PCM_RATE_96000 | SNDRV_PCM_RATE_176400 |\
+			 SNDRV_PCM_RATE_192000)
+
+#define SPDIF_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
+			 SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\
+			 SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\
+			 SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE)
+
+/*
+ * This list is only for formats allowed on the I2S bus. So there is
+ * some formats listed that are not supported by HDMI interface. For
+ * instance allowing the 32-bit formats enables 24-precision with CPU
+ * DAIs that do not support 24-bit formats. If the extra formats cause
+ * problems, we should add the video side driver an option to disable
+ * them.
+ */
+#define I2S_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
+			 SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\
+			 SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\
+			 SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE |\
+			 SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S32_BE)
+
+static struct snd_soc_dai_driver hdmi_i2s_dai = {
+	.name = "i2s-hifi",
+	.id = DAI_ID_I2S,
+	.playback = {
+		.stream_name = "Playback",
+		.channels_min = 2,
+		.channels_max = 8,
+		.rates = HDMI_RATES,
+		.formats = I2S_FORMATS,
+		.sig_bits = 24,
+	},
+	.ops = &hdmi_dai_ops,
+};
+
+static const struct snd_soc_dai_driver hdmi_spdif_dai = {
+	.name = "spdif-hifi",
+	.id = DAI_ID_SPDIF,
+	.playback = {
+		.stream_name = "Playback",
+		.channels_min = 2,
+		.channels_max = 2,
+		.rates = HDMI_RATES,
+		.formats = SPDIF_FORMATS,
+	},
+	.ops = &hdmi_dai_ops,
+};
+
+static struct snd_soc_codec_driver hdmi_codec = {
+	.dapm_widgets = hdmi_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(hdmi_widgets),
+	.dapm_routes = hdmi_routes,
+	.num_dapm_routes = ARRAY_SIZE(hdmi_routes),
+};
+
+static int hdmi_codec_probe(struct platform_device *pdev)
+{
+	struct hdmi_codec_pdata *hcd = pdev->dev.platform_data;
+	struct device *dev = &pdev->dev;
+	struct hdmi_codec_priv *hcp;
+	int dai_count, i = 0;
+	int ret;
+
+	dev_dbg(dev, "%s()\n", __func__);
+
+	if (!hcd) {
+		dev_err(dev, "%s: No plalform data\n", __func__);
+		return -EINVAL;
+	}
+
+	dai_count = hcd->i2s + hcd->spdif;
+	if (dai_count < 1 || !hcd->ops || !hcd->ops->hw_params ||
+	    !hcd->ops->audio_shutdown) {
+		dev_err(dev, "%s: Invalid parameters\n", __func__);
+		return -EINVAL;
+	}
+
+	hcp = devm_kzalloc(dev, sizeof(*hcp), GFP_KERNEL);
+	if (!hcp)
+		return -ENOMEM;
+
+	hcp->hcd = *hcd;
+	mutex_init(&hcp->current_stream_lock);
+
+	hcp->daidrv = devm_kzalloc(dev, dai_count * sizeof(*hcp->daidrv),
+				   GFP_KERNEL);
+	if (!hcp->daidrv)
+		return -ENOMEM;
+
+	if (hcd->i2s) {
+		hcp->daidrv[i] = hdmi_i2s_dai;
+		hcp->daidrv[i].playback.channels_max =
+			hcd->max_i2s_channels;
+		i++;
+	}
+
+	if (hcd->spdif)
+		hcp->daidrv[i] = hdmi_spdif_dai;
+
+	ret = snd_soc_register_codec(dev, &hdmi_codec, hcp->daidrv,
+				     dai_count);
+	if (ret) {
+		dev_err(dev, "%s: snd_soc_register_codec() failed (%d)\n",
+			__func__, ret);
+		return ret;
+	}
+
+	dev_set_drvdata(dev, hcp);
+	return 0;
+}
+
+static int hdmi_codec_remove(struct platform_device *pdev)
+{
+	snd_soc_unregister_codec(&pdev->dev);
+	return 0;
+}
+
+static struct platform_driver hdmi_codec_driver = {
+	.driver = {
+		.name = HDMI_CODEC_DRV_NAME,
+	},
+	.probe = hdmi_codec_probe,
+	.remove = hdmi_codec_remove,
+};
+
+module_platform_driver(hdmi_codec_driver);
+
+MODULE_AUTHOR("Jyri Sarha <[email protected]>");
+MODULE_DESCRIPTION("HDMI Audio Codec Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" HDMI_CODEC_DRV_NAME);

sound/soc/codecs/hdmi-codec.c new file +12 -0

+
+static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
+				struct snd_pcm_hw_params *params,
+				struct snd_soc_dai *dai)
+{
+	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
+	struct hdmi_codec_params hp = {
+		.iec = {
+			.status = { 0 },
+			.subcode = { 0 },
+			.pad = 0,
+			.dig_subframe = { 0 },

From: Jean-Francois Moine <[email protected]>

Two kinds of ports may be declared in a DT graph of ports: video and audio. This patch accepts the port value from a video port as an alternative to the video-ports property.
It also accepts audio ports in the case the transmitter is not used as a slave encoder.
The new file include/sound/tda998x.h prepares to the definition of a tda998x CODEC.

Signed-off-by Jean-Francois Moine <[email protected]>

Signed-off-by Jyri Sarha <[email protected]>

---
 .../devicetree/bindings/display/bridge/tda998x.txt | 51 ++++++++++++
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 90 +++++++++++++++++++---
 include/sound/tda998x.h                            |  8 ++
 3 files changed, 140 insertions(+), 9 deletions(-)
 create mode 100644 include/sound/tda998x.h

Documentation/devicetree/bindings/display/bridge/tda998x.txt +2 -0

index e9e4bce..35f6a80 100644
@@ -16,6 +16,35 @@ Optional properties:
 
   - video-ports: 24 bits value which defines how the video controller
 	output is wired to the TDA998x input - default: <0x230145>
+	This property is not used when ports are defined.
+

Quoted 29 lines · Jyri Sarha

From: Jean-Francois Moine <[email protected]>

Two kinds of ports may be declared in a DT graph of ports: video and audio. This patch accepts the port value from a video port as an alternative to the video-ports property.
It also accepts audio ports in the case the transmitter is not used as a slave encoder.
The new file include/sound/tda998x.h prepares to the definition of a tda998x CODEC.

Signed-off-by Jean-Francois Moine <[email protected]>

Signed-off-by Jyri Sarha <[email protected]>

---
 .../devicetree/bindings/display/bridge/tda998x.txt | 51 ++++++++++++
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 90 +++++++++++++++++++---
 include/sound/tda998x.h                            |  8 ++
 3 files changed, 140 insertions(+), 9 deletions(-)
 create mode 100644 include/sound/tda998x.h

Documentation/devicetree/bindings/display/bridge/tda998x.txt +2 -0

index e9e4bce..35f6a80 100644
@@ -16,6 +16,35 @@ Optional properties:
 
   - video-ports: 24 bits value which defines how the video controller
 	output is wired to the TDA998x input - default: <0x230145>
+	This property is not used when ports are defined.
+

Documentation/devicetree/bindings/display/bridge/tda998x.txt Optional properties: · line 30

Quoted 10 lines

Documentation/devicetree/bindings/display/bridge/tda998x.txt +10 -0

+Optional nodes:
+
+  - port: up to three ports.
+	The ports are defined according to [1].
+
+    Video port.
+	There may be only one video port.
+	This one must contain the following property:
+
+	- port-type: must be "rgb"

This should be implied from the port unit address. In other words, port@0 is defined to be the rgb port. Now, if this is one of several modes for the video port, then that is a different story.

Documentation/devicetree/bindings/display/bridge/tda998x.txt Optional properties: · line 36

Quoted 5 lines

Documentation/devicetree/bindings/display/bridge/tda998x.txt +5 -0

+	and may contain the optional property:
+
+	- reg: 24 bits value which defines how the video controller
+		output is wired to the TDA998x input (video pins)
+		When absent, the default value is <0x230145>.

This is not really how reg is intended to be used. Can you explain how this value is determined?

Documentation/devicetree/bindings/display/bridge/tda998x.txt Optional properties: · line 45

Quoted 8 lines

Documentation/devicetree/bindings/display/bridge/tda998x.txt +8 -0

+    Audio ports.
+	There may be one or two audio ports.
+	These ones must contain the following properties:
+
+	- port-type: must be "i2s" or "spdif"
+
+	- reg: 8 bits value which defines how the audio controller
+		output is wired to the TDA998x input (audio pins)

Same here.

Quoted 54 lines · On Thu, 18 Feb 2016 08:35:30 -0600 Rob Herring <[email protected]> wrote:

Quoted 29 lines · Jyri Sarha

From: Jean-Francois Moine <[email protected]>

Two kinds of ports may be declared in a DT graph of ports: video and audio. This patch accepts the port value from a video port as an alternative to the video-ports property.
It also accepts audio ports in the case the transmitter is not used as a slave encoder.
The new file include/sound/tda998x.h prepares to the definition of a tda998x CODEC.

Signed-off-by Jean-Francois Moine <[email protected]>

Signed-off-by Jyri Sarha <[email protected]>

---
 .../devicetree/bindings/display/bridge/tda998x.txt | 51 ++++++++++++
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 90 +++++++++++++++++++---
 include/sound/tda998x.h                            |  8 ++
 3 files changed, 140 insertions(+), 9 deletions(-)
 create mode 100644 include/sound/tda998x.h

Documentation/devicetree/bindings/display/bridge/tda998x.txt +2 -0

index e9e4bce..35f6a80 100644
@@ -16,6 +16,35 @@ Optional properties:
 
   - video-ports: 24 bits value which defines how the video controller
 	output is wired to the TDA998x input - default: <0x230145>
+	This property is not used when ports are defined.
+

Documentation/devicetree/bindings/display/bridge/tda998x.txt Optional properties: · line 30

Quoted 10 lines

Documentation/devicetree/bindings/display/bridge/tda998x.txt +10 -0

+Optional nodes:
+
+  - port: up to three ports.
+	The ports are defined according to [1].
+
+    Video port.
+	There may be only one video port.
+	This one must contain the following property:
+
+	- port-type: must be "rgb"

This should be implied from the port unit address. In other words, port@0 is defined to be the rgb port. Now, if this is one of several modes for the video port, then that is a different story.

Documentation/devicetree/bindings/display/bridge/tda998x.txt Optional properties: · line 36

Quoted 5 lines

Documentation/devicetree/bindings/display/bridge/tda998x.txt +5 -0

+	and may contain the optional property:
+
+	- reg: 24 bits value which defines how the video controller
+		output is wired to the TDA998x input (video pins)
+		When absent, the default value is <0x230145>.

This is not really how reg is intended to be used. Can you explain how this value is determined?

Quoted 10 lines

Documentation/devicetree/bindings/display/bridge/tda998x.txt Optional properties: · line 45

Quoted 8 lines

Documentation/devicetree/bindings/display/bridge/tda998x.txt +8 -0

+    Audio ports.
+	There may be one or two audio ports.
+	These ones must contain the following properties:
+
+	- port-type: must be "i2s" or "spdif"
+
+	- reg: 8 bits value which defines how the audio controller
+		output is wired to the TDA998x input (audio pins)

Same here.

Hi Rob,

These audio/video port definitions were discussed in the thread
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-August/095836.html
but, you are right, and as you already said in the thread
https://lists.freedesktop.org/archives/dri-devel/2015-September/090544.html
'reg' should not be used here.

Quoted 35 lines · Jyri Sarha

From: Jean-Francois Moine <[email protected]>

Two kinds of ports may be declared in a DT graph of ports: video and audio. This patch accepts the port value from a video port as an alternative to the video-ports property.
It also accepts audio ports in the case the transmitter is not used as a slave encoder.
The new file include/sound/tda998x.h prepares to the definition of a tda998x CODEC.

Signed-off-by Jean-Francois Moine <[email protected]>

Signed-off-by Jyri Sarha <[email protected]>

---
  .../devicetree/bindings/display/bridge/tda998x.txt | 51 ++++++++++++
  drivers/gpu/drm/i2c/tda998x_drv.c                  | 90 +++++++++++++++++++---
  include/sound/tda998x.h                            |  8 ++
  3 files changed, 140 insertions(+), 9 deletions(-)

create mode 100644 include/sound/tda998x.h

Documentation/devicetree/bindings/display/bridge/tda998x.txt +8 -0

index e9e4bce..35f6a80 100644
@@ -16,6 +16,35 @@ Optional properties:
 
    - video-ports: 24 bits value which defines how the video controller
  	output is wired to the TDA998x input - default: <0x230145>
+	This property is not used when ports are defined.
+
+Optional nodes:
+
+  - port: up to three ports.
+	The ports are defined according to [1].
+
+    Video port.

Documentation/devicetree/bindings/display/bridge/tda998x.txt Optional properties: · line 30

Quoted 4 lines

Documentation/devicetree/bindings/display/bridge/tda998x.txt +4 -0

+	There may be only one video port.
+	This one must contain the following property:
+
+	- port-type: must be "rgb"

This should be implied from the port unit address. In other words, port@0 is defined to be the rgb port. Now, if this is one of several modes for the video port, then that is a different story.

Documentation/devicetree/bindings/display/bridge/tda998x.txt Optional properties: · line 36

Quoted 5 lines

Documentation/devicetree/bindings/display/bridge/tda998x.txt +5 -0

+	and may contain the optional property:
+
+	- reg: 24 bits value which defines how the video controller
+		output is wired to the TDA998x input (video pins)
+		When absent, the default value is <0x230145>.

This is not really how reg is intended to be used. Can you explain how this value is determined?

Documentation/devicetree/bindings/display/bridge/tda998x.txt Optional properties: · line 45

Quoted 8 lines

Documentation/devicetree/bindings/display/bridge/tda998x.txt +8 -0

+    Audio ports.
+	There may be one or two audio ports.
+	These ones must contain the following properties:
+
+	- port-type: must be "i2s" or "spdif"
+
+	- reg: 8 bits value which defines how the audio controller
+		output is wired to the TDA998x input (audio pins)

Same here.

Rob

Quoted 4 lines · Rob Herring

This should be implied from the port unit address. In other words, port@0 is defined to be the rgb port. Now, if this is one of several modes for the video port, then that is a different story.

Do you suggest that also the audio i2s and s/p-dif port-types should be coded in the port unit addresses? Something like: port@0 is always rgb, port@1 is i2s, and port@2 is spdif?

Quoted 9 lines · Jyri Sarha

Quoted 4 lines · Rob Herring

This should be implied from the port unit address. In other words, port@0 is defined to be the rgb port. Now, if this is one of several modes for the video port, then that is a different story.

Do you suggest that also the audio i2s and s/p-dif port-types should be coded in the port unit addresses? Something like: port@0 is always rgb, port@1 is i2s, and port@2 is spdif?

For the audio inputs, the port address corresponds to the input pin. TDA998x devices can have multiple streams routed to the pins, and can select between them.

For example, there may be four I2S data pins and one I2S clock pin. When using stereo, you can select which of the four I2S data pins carries the audio data.

When using SPDIF, there may be two SPDIF inputs, and you can select which SPDIF input is used.

So, "reg" may not be an address in terms of a CPU visible address, but it's an address as far as selecting the appropriate input - and it fits in with the requirements of ePAPR, which are that if you have a unit-address (which is required to distinguish different port nodes) then you must have a matching "reg" property.

I don't particularly like the video node using the RGB routing register value either for the reg property, but I've kept quiet because I have nothing to offer there: again, this comes down to ePAPR requirements and the need to specify multiple "port { }" nodes. You can't have two "port { }" nodes without using a unit-address, and we'd need to chose a unit-address for it which doesn't conflict with the audio ports... so there's a kind of logic to using the RGB routing value, which will never conflict.

Quoted 11 lines · Russell King - ARM Linux

Quoted 9 lines · Jyri Sarha

Quoted 4 lines · Rob Herring

This should be implied from the port unit address. In other words, port@0 is defined to be the rgb port. Now, if this is one of several modes for the video port, then that is a different story.

Do you suggest that also the audio i2s and s/p-dif port-types should be coded in the port unit addresses? Something like: port@0 is always rgb, port@1 is i2s, and port@2 is spdif?

Quoted 8 lines

For the audio inputs, the port address corresponds to the input pin. TDA998x devices can have multiple streams routed to the pins, and can select between them.

For example, there may be four I2S data pins and one I2S clock pin. When using stereo, you can select which of the four I2S data pins carries the audio data.

Sure, but I do not think that would be the usual setup. The only "normal" situation I can think for having a need to have two alternative audio setups would one for i2s and another for s/p-dif. But then again it is possible to come up with a design with multiple alternative audio wirings and it relatively simple handle that in DT binding, so why not.

Quoted 8 lines

When using SPDIF, there may be two SPDIF inputs, and you can select which SPDIF input is used.

So, "reg" may not be an address in terms of a CPU visible address, but it's an address as far as selecting the appropriate input - and it fits in with the requirements of ePAPR, which are that if you have a unit-address (which is required to distinguish different port nodes) then you must have a matching "reg" property.

Still I do not see why it is desirable to reuse reg property, when we can introduce new property for describing the audio wiring.

Quoted 9 lines

I don't particularly like the video node using the RGB routing register value either for the reg property, but I've kept quiet because I have nothing to offer there: again, this comes down to ePAPR requirements and the need to specify multiple "port { }" nodes. You can't have two "port { }" nodes without using a unit-address, and we'd need to chose a unit-address for it which doesn't conflict with the audio ports... so there's a kind of logic to using the RGB routing value, which will never conflict.

If we after all decide to go with using reg property for audio wiring (and essentially writing the value directly to AP_ENA register), then we could also agree that video port's unit address is always 0 as it corresponds to audio disabled in AP_ENA register and would not collide with any audio "address". Then we could keep the old video-ports property to configure the video wiring. How does this sound?

Quoted 21 lines · Jyri Sarha

Quoted 11 lines · Russell King - ARM Linux

Quoted 9 lines · Jyri Sarha

Quoted 4 lines · Rob Herring

This should be implied from the port unit address. In other words, port@0 is defined to be the rgb port. Now, if this is one of several modes for the video port, then that is a different story.

Do you suggest that also the audio i2s and s/p-dif port-types should be coded in the port unit addresses? Something like: port@0 is always rgb, port@1 is i2s, and port@2 is spdif?

Quoted 8 lines

For the audio inputs, the port address corresponds to the input pin. TDA998x devices can have multiple streams routed to the pins, and can select between them.

For example, there may be four I2S data pins and one I2S clock pin. When using stereo, you can select which of the four I2S data pins carries the audio data.

Quoted 5 lines

Sure, but I do not think that would be the usual setup. The only "normal" situation I can think for having a need to have two alternative audio setups would one for i2s and another for s/p-dif. But then again it is possible to come up with a design with multiple alternative audio wirings and it relatively simple handle that in DT binding, so why not.

There's another reason: if you want to support 8 channel audio using I2S rather than SPDIF, then you need to use four I2S data inputs. Each I2S data input can support only two channels.

Quoted 9 lines

Quoted 8 lines

When using SPDIF, there may be two SPDIF inputs, and you can select which SPDIF input is used.

So, "reg" may not be an address in terms of a CPU visible address, but it's an address as far as selecting the appropriate input - and it fits in with the requirements of ePAPR, which are that if you have a unit-address (which is required to distinguish different port nodes) then you must have a matching "reg" property.

Quoted 2 lines

Still I do not see why it is desirable to reuse reg property, when we can introduce new property for describing the audio wiring.

Different people have different opinions. Your opinion is just another example of someone holding a different view.

You _have_ to have a unit address, and therefore you _have_ to have a reg property. If you want to use some other property to describe the

audio input pin, then you will need to make up a totally ficticious unit-address and reg property for each audio input pin.

That's adding complexity, arguably unnecessary complexity, and making the binding unnecessarily more complex for no good reason.

Quoted 10 lines

Quoted 9 lines

I don't particularly like the video node using the RGB routing register value either for the reg property, but I've kept quiet because I have nothing to offer there: again, this comes down to ePAPR requirements and the need to specify multiple "port { }" nodes. You can't have two "port { }" nodes without using a unit-address, and we'd need to chose a unit-address for it which doesn't conflict with the audio ports... so there's a kind of logic to using the RGB routing value, which will never conflict.

Quoted 6 lines

If we after all decide to go with using reg property for audio wiring (and essentially writing the value directly to AP_ENA register), then we could also agree that video port's unit address is always 0 as it corresponds to audio disabled in AP_ENA register and would not collide with any audio "address". Then we could keep the old video-ports property to configure the video wiring. How does this sound?

Sub-standard :)

This has actually been discussed before. See the thread:

"[PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio"

from January 2015.

Ok, here is just one more simple alternative for tda998x audio binding. I feel that the graph ports binding for audio does not make sense without a graph based ASoC machine driver implementation. The ASoC simple-card is already here and it so widely used that there is no getting rid of that any time soon. This proposal provides the same functionality as the patch in the root of this thread, but in a simpler way and is equally compatible with simple-card.

Quoted 7 lines · On Tue, 1 Mar 2016 16:26:50 +0200 Jyri Sarha <[email protected]> wrote:

Ok, here is just one more simple alternative for tda998x audio binding. I feel that the graph ports binding for audio does not make sense without a graph based ASoC machine driver implementation. The ASoC simple-card is already here and it so widely used that there is no getting rid of that any time soon. This proposal provides the same functionality as the patch in the root of this thread, but in a simpler way and is equally compatible with simple-card.

Hi Jyri,

The graph port binding for the tda998x works fine with the simple card driver when multi-codec support is added. See
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-January/085855.html

I know that it works, I have used it myself until now, but it is not needed and there is no driver that parses audio port endpoints. I see no point specifying something in the binding that is not used and there no specific plan to ever use it.

AFAIU my proposed binding should work equally well with simple-card, with or without multi-codec support.

Quoted 5 lines · On Tue, 1 Mar 2016 17:51:09 +0200 Jyri Sarha <[email protected]> wrote:

I know that it works, I have used it myself until now, but it is not needed and there is no driver that parses audio port endpoints. I see no point specifying something in the binding that is not used and there no specific plan to ever use it.

Quoted 2 lines

AFAIU my proposed binding should work equally well with simple-card, with or without multi-codec support.

As told many times, the simple card is a pure Linux specific entity. It does not describe any hardware. It should not appear in a DT, or, if it does, its compatible should be "linux, simple-audio-card". Then, how can the other OSs know the links between the audio devices and the audio encoders/connectors?

On the other way, the audio graph does not impose any particular software design. It just describes the links between the different hardware components and each OS is free to implement its own layout.

I understand the short comings of simple-card and it's binding. However, the binding is documented and it is feasible to extract the audio connections from a simple-card binding too. In fact it models the I2S connections better than straight out of the box graph binding. Actually a graph is not the best way describe an i2s-bus with multiple DAIs (codec or CPU) connected to it.

Quoted 6 lines · On Tue, 1 Mar 2016 20:29:17 +0200 Jyri Sarha <[email protected]> wrote:

I understand the short comings of simple-card and it's binding. However, the binding is documented and it is feasible to extract the audio connections from a simple-card binding too. In fact it models the I2S connections better than straight out of tehe box graph binding. Actually a graph is not the best way describe an i2s-bus with multiple DAIs (codec or CPU) connected to it.

I still don't understand your problem. You want something like:

	audio-ports = <	TDA998x_SPDIF	0x04
			TDA998x_I2S	0x03>;

and the graph definition would be:

	port@03 {
		reg = <0x03>;
		port-type = "audio-i2s";
		...
	};

	port@04 {
		reg = <0x04>;
		port-type = "audio-spdif";
		...
	};

Apart the syntax, I don't really see the difference.

As told many times, the simple card is a pure Linux specific entity. It does not describe any hardware. It should not appear in a DT, or,

if it does, its compatible should be "linux, simple-audio-card".

Then, how can the other OSs know the links between the audio devices and the audio encoders/connectors?

On the other way, the audio graph does not impose any particular software design. It just describes the links between the different hardware components and each OS is free to implement its own layout.

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.

Signed-off-by Jyri Sarha <[email protected]>

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

arch/arm/boot/dts/am335x-boneblack.dts +0 -0

index eadbba3..05347dc 100644
@@ -64,6 +64,16 @@
                        0x1b0 0x03      /* xdma_event_intr0, OMAP_MUX_MODE3 | AM33XX_PIN_OUTPUT */