[PATCH RFC v5 0/8] Implement generic ASoC HDMI codec and use it in tda998x

[PATCH RFC v5 0/8] Implement generic ASoC HDMI codec and use it in tda998x

[Jyri Sarha]

There is very little change to this series since the RFC v4, but because
of the recent interest on the hdmi-codec patch I decided to send
another version of the series.

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

There was also some comments about the DT ports binding for tda998x by
Jean-Francois Moine. However I did not have time to come up with
alternative approach. All in all my tda998x patches should be
considered more as a proof of concept for the hdmi-codec part, rather
than a serious attempt to get those patches in. However, all comments
to those patch are more than welcome as they will help me to come up
with something that could finally get merged.

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

The binding for tda998x is taken from Jean Francois' patch series[1] on
the same subject. The implementation of the of-node parsing has some
minor changes from my self.

Here is what I think at least could or should still be done, but non of
that stuff does not sounds critical right now.
 
Missing from tda998x driver side
- hdmi_codec_ops audio_startup() implementation for audio abort support
- multi channel audio support (I would need specs and preferably some
  HW to test for this).

Missing from ASoC side generic implementation:
- channel_allocation handling is completely left for the video side driver,
  see if ASoC side could help in any way
- snd_soc_jack functionality to handle hdmi cable plug/unplug events

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2015-July/095596.html

Best regards,
Jyri

Jean-Francois Moine (1):
  drm/i2c: tda998x: Add support of a DT graph of ports

Jyri Sarha (7):
  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: Remove include/sound/tda998x.h and fix graph parsing
  drm/i2c: tda998x: Improve tda998x_configure_audio() audio related
    pdata
  drm/i2c: tda998x: Register ASoC HDMI codec for audio functionality
  ARM: dts: am335x-boneblack: Add HDMI audio support

 .../devicetree/bindings/display/bridge/tda998x.txt |  51 +++
 arch/arm/boot/dts/am335x-boneblack.dts             |  90 ++++-
 drivers/gpu/drm/i2c/Kconfig                        |   1 +
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 300 ++++++++++++---
 include/drm/i2c/tda998x.h                          |  24 +-
 include/sound/hdmi-codec.h                         | 104 ++++++
 include/sound/pcm_iec958.h                         |   2 +
 sound/core/pcm_iec958.c                            |  52 ++-
 sound/soc/codecs/Kconfig                           |   6 +
 sound/soc/codecs/Makefile                          |   2 +
 sound/soc/codecs/hdmi-codec.c                      | 411 +++++++++++++++++++++
 11 files changed, 964 insertions(+), 79 deletions(-)
 create mode 100644 include/sound/hdmi-codec.h
 create mode 100644 sound/soc/codecs/hdmi-codec.c

-- 
1.9.1

[PATCH RFC v5 1/8] ALSA: pcm: add IEC958 channel status helper for hw_params

[Jyri Sarha]

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.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 include/sound/pcm_iec958.h |  2 ++
 sound/core/pcm_iec958.c    | 52 +++++++++++++++++++++++++++++++---------------
 2 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h
index 0eed397..36f023a 100644
--- a/include/sound/pcm_iec958.h
+++ b/include/sound/pcm_iec958.h
@@ -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
diff --git a/sound/core/pcm_iec958.c b/sound/core/pcm_iec958.c
index 36b2d7a..c9f8b66 100644
--- a/sound/core/pcm_iec958.c
+++ b/sound/core/pcm_iec958.c
@@ -9,30 +9,18 @@
 #include <linux/types.h>
 #include <sound/asoundef.h>
 #include <sound/pcm.h>
+#include <sound/pcm_params.h>
 #include <sound/pcm_iec958.h>
 
-/**
- * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status
- * @runtime: pcm runtime structure with ->rate filled in
- * @cs: channel status buffer, at least four bytes
- * @len: length of channel status buffer
- *
- * Create the consumer format channel status data in @cs of maximum size
- * @len corresponding to the parameters of the PCM runtime @runtime.
- *
- * Drivers may wish to tweak the contents of the buffer after creation.
- *
- * Returns: length of buffer, or negative error code if something failed.
- */
-int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
-	size_t len)
+static int create_iec958_consumer(uint rate, uint sample_width,
+				  u8 *cs, size_t len)
 {
 	unsigned int fs, ws;
 
 	if (len < 4)
 		return -EINVAL;
 
-	switch (runtime->rate) {
+	switch (rate) {
 	case 32000:
 		fs = IEC958_AES3_CON_FS_32000;
 		break;
@@ -59,7 +47,7 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
 	}
 
 	if (len > 4) {
-		switch (snd_pcm_format_width(runtime->format)) {
+		switch (sample_width) {
 		case 16:
 			ws = IEC958_AES4_CON_WORDLEN_20_16;
 			break;
@@ -92,4 +80,34 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
 
 	return len;
 }
+
+/**
+ * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status
+ * @runtime: pcm runtime structure with ->rate filled in
+ * @cs: channel status buffer, at least four bytes
+ * @len: length of channel status buffer
+ *
+ * Create the consumer format channel status data in @cs of maximum size
+ * @len corresponding to the parameters of the PCM runtime @runtime.
+ *
+ * Drivers may wish to tweak the contents of the buffer after creation.
+ *
+ * Returns: length of buffer, or negative error code if something failed.
+ */
+int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
+	size_t len)
+{
+	return create_iec958_consumer(runtime->rate,
+				      snd_pcm_format_width(runtime->format),
+				      cs, len);
+}
 EXPORT_SYMBOL(snd_pcm_create_iec958_consumer);
+
+
+int snd_pcm_create_iec958_consumer_hw_params(struct snd_pcm_hw_params *params,
+					     u8 *cs, size_t len)
+{
+	return create_iec958_consumer(params_rate(params), params_width(params),
+				      cs, len);
+}
+EXPORT_SYMBOL(snd_pcm_create_iec958_consumer_hw_params);
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC v5 2/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Jyri Sarha]

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 <jsarha@ti.com>
---
 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

diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h
new file mode 100644
index 0000000..fc3a481
--- /dev/null
+++ b/include/sound/hdmi-codec.h
@@ -0,0 +1,100 @@
+/*
+ * hdmi-codec.h - HDMI Codec driver API
+ *
+ * Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com
+ *
+ * Author: Jyri Sarha <jsarha@ti.com>
+ *
+ * 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__ */
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index cfdafc4..4d5915e 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -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"
 
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index f632fc4..49c1824 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -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
diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
new file mode 100644
index 0000000..bc47b9a
--- /dev/null
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -0,0 +1,393 @@
+/*
+ * ALSA SoC codec for HDMI encoder drivers
+ * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
+ * Author: Jyri Sarha <jsarha@ti.com>
+ *
+ * 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 <jsarha@ti.com>");
+MODULE_DESCRIPTION("HDMI Audio Codec Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" HDMI_CODEC_DRV_NAME);
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC v5 2/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Arnaud Pouliquen]

Hello Jyri,

Integrated and tested on sti platform with success.

I will re-submit patch-set associated to sti platform, when (if) patches
accepted.

Regards
Arnaud

On 02/17/2016 03:49 PM, Jyri Sarha wrote:
> 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 <jsarha@ti.com>
> ---
>  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
> 
> diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h
> new file mode 100644
> index 0000000..fc3a481
> --- /dev/null
> +++ b/include/sound/hdmi-codec.h
> @@ -0,0 +1,100 @@
> +/*
> + * hdmi-codec.h - HDMI Codec driver API
> + *
> + * Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com
> + *
> + * Author: Jyri Sarha <jsarha@ti.com>
> + *
> + * 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__ */
> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> index cfdafc4..4d5915e 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -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"
>  
> diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
> index f632fc4..49c1824 100644
> --- a/sound/soc/codecs/Makefile
> +++ b/sound/soc/codecs/Makefile
> @@ -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
> diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
> new file mode 100644
> index 0000000..bc47b9a
> --- /dev/null
> +++ b/sound/soc/codecs/hdmi-codec.c
> @@ -0,0 +1,393 @@
> +/*
> + * ALSA SoC codec for HDMI encoder drivers
> + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
> + * Author: Jyri Sarha <jsarha@ti.com>
> + *
> + * 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 <jsarha@ti.com>");
> +MODULE_DESCRIPTION("HDMI Audio Codec Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" HDMI_CODEC_DRV_NAME);
> 

Re: [PATCH RFC v5 2/8] ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders

[Arnaud Pouliquen]

Hello Jyri,

> +
> +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));
I ran into an issue during my test.
For I2S, an error is returned by create_iec958_consumer for 32-bits
stream (SNDRV_PCM_FMTBIT_S32_LE).
I suppose that it makes sense to handle 32-bits configuration for I2S
(but also SPDIF), to support software IEC958 formating/pre-formating...
To fix issue, just need to add 32-bits with 24-bits case in
create_iec958_consumer

Regards
Arnaud

[PATCH RFC v5 3/8] ASoC: hdmi-codec: Add audio abort() callback for video side to use

[Jyri Sarha]

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.

Signed-off-by: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>
---
 include/sound/hdmi-codec.h    |  8 ++++++--
 sound/soc/codecs/hdmi-codec.c | 20 +++++++++++++++++++-
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h
index fc3a481..15fe70f 100644
--- a/include/sound/hdmi-codec.h
+++ b/include/sound/hdmi-codec.h
@@ -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.
diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
index bc47b9a..cc08097 100644
--- a/sound/soc/codecs/hdmi-codec.c
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH RFC v5 4/8] drm/i2c: tda998x: Add support of a DT graph of ports

[Jyri Sarha]

From: Jean-Francois Moine <moinejf@free.fr>

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 <moinejf@free.fr>
Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 .../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

diff --git a/Documentation/devicetree/bindings/display/bridge/tda998x.txt b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
index e9e4bce..35f6a80 100644
--- a/Documentation/devicetree/bindings/display/bridge/tda998x.txt
+++ b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
@@ -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.
+	There may be only one video port.
+	This one must contain the following property:
+
+	- port-type: must be "rgb"
+
+	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>.
+
+    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)
+
+[1] Documentation/devicetree/bindings/graph.txt
 
 Example:
 
@@ -26,4 +55,26 @@ Example:
 		interrupts = <27 2>;		/* falling edge */
 		pinctrl-0 = <&pmx_camera>;
 		pinctrl-names = "default";
+
+		port@230145 {
+			port-type = "rgb";
+			reg = <0x230145>;
+			hdmi_0: endpoint {
+				remote-endpoint = <&lcd0_0>;
+			};
+		};
+		port@3 {			/* AP1 = I2S */
+			port-type = "i2s";
+			reg = <0x03>;
+			tda998x_i2s: endpoint {
+				remote-endpoint = <&audio1_i2s>;
+			};
+		};
+		port@4 {			 /* AP2 = S/PDIF */
+			port-type = "spdif";
+			reg = <0x04>;
+			tda998x_spdif: endpoint {
+				remote-endpoint = <&audio1_spdif1>;
+			};
+		};
 	};
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 34e3874..6db1663 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -27,6 +27,7 @@
 #include <drm/drm_edid.h>
 #include <drm/drm_of.h>
 #include <drm/i2c/tda998x.h>
+#include <sound/tda998x.h>
 
 #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
 
@@ -53,6 +54,8 @@ struct tda998x_priv {
 
 	struct drm_encoder encoder;
 	struct drm_connector connector;
+
+	struct tda998x_audio audio;
 };
 
 #define conn_to_tda998x_priv(x) \
@@ -821,6 +824,8 @@ static void tda998x_encoder_set_config(struct tda998x_priv *priv,
 			    (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
 
 	priv->params = *p;
+	priv->audio.port_types[0] = p->audio_format;
+	priv->audio.ports[0] = p->audio_cfg;
 }
 
 static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
@@ -1199,9 +1204,57 @@ static void tda998x_destroy(struct tda998x_priv *priv)
 
 /* I2C driver functions */
 
+static int tda998x_parse_ports(struct tda998x_priv *priv,
+				struct device_node *np)
+{
+	struct device_node *of_port;
+	const char *port_type;
+	int ret, audio_index, reg, afmt;
+
+	audio_index = 0;
+	for_each_child_of_node(np, of_port) {
+		if (!of_port->name
+		 || of_node_cmp(of_port->name, "port") != 0)
+			continue;
+		ret = of_property_read_string(of_port, "port-type",
+					&port_type);
+		if (ret < 0)
+			continue;
+		ret = of_property_read_u32(of_port, "reg", &reg);
+		if (strcmp(port_type, "rgb") == 0) {
+			if (!ret) {		/* video reg is optional */
+				priv->vip_cntrl_0 = reg >> 16;
+				priv->vip_cntrl_1 = reg >> 8;
+				priv->vip_cntrl_2 = reg;
+			}
+			continue;
+		}
+		if (strcmp(port_type, "i2s") == 0)
+			afmt = AFMT_I2S;
+		else if (strcmp(port_type, "spdif") == 0)
+			afmt = AFMT_SPDIF;
+		else
+			continue;
+		if (ret < 0) {
+			dev_err(&priv->hdmi->dev, "missing reg for %s\n",
+				port_type);
+			return ret;
+		}
+		if (audio_index >= ARRAY_SIZE(priv->audio.ports)) {
+			dev_err(&priv->hdmi->dev, "too many audio ports\n");
+			break;
+		}
+		priv->audio.ports[audio_index] = reg;
+		priv->audio.port_types[audio_index] = afmt;
+		audio_index++;
+	}
+	return 0;
+}
+
 static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 {
 	struct device_node *np = client->dev.of_node;
+	struct device_node *of_port;
 	u32 video;
 	int rev_lo, rev_hi, ret;
 	unsigned short cec_addr;
@@ -1309,15 +1362,34 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 	/* enable EDID read irq: */
 	reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
 
-	if (!np)
-		return 0;		/* non-DT */
-
-	/* get the optional video properties */
-	ret = of_property_read_u32(np, "video-ports", &video);
-	if (ret == 0) {
-		priv->vip_cntrl_0 = video >> 16;
-		priv->vip_cntrl_1 = video >> 8;
-		priv->vip_cntrl_2 = video;
+	/* get the device tree parameters */
+	if (np) {
+		of_port = of_get_child_by_name(np, "port");
+		if (of_port) {				/* graph of ports */
+			of_node_put(of_port);
+			ret = tda998x_parse_ports(priv, np);
+			if (ret < 0)
+				goto fail;
+
+			/* initialize the default audio configuration */
+			if (priv->audio.ports[0]) {
+				priv->params.audio_cfg = priv->audio.ports[0];
+				priv->params.audio_format =
+						priv->audio.port_types[0];
+				priv->params.audio_clk_cfg =
+					priv->params.audio_format ==
+							AFMT_SPDIF ? 0 : 1;
+			}
+		} else {
+
+			/* optional video properties */
+			ret = of_property_read_u32(np, "video-ports", &video);
+			if (ret == 0) {
+				priv->vip_cntrl_0 = video >> 16;
+				priv->vip_cntrl_1 = video >> 8;
+				priv->vip_cntrl_2 = video;
+			}
+		}
 	}
 
 	return 0;
diff --git a/include/sound/tda998x.h b/include/sound/tda998x.h
new file mode 100644
index 0000000..bef1da7
--- /dev/null
+++ b/include/sound/tda998x.h
@@ -0,0 +1,8 @@
+#ifndef SND_TDA998X_H
+#define SND_TDA998X_H
+
+struct tda998x_audio {
+	u8 ports[2];			/* AP value */
+	u8 port_types[2];		/* AFMT_xxx */
+};
+#endif
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC v5 4/8] drm/i2c: tda998x: Add support of a DT graph of ports

[Rob Herring]

On Wed, Feb 17, 2016 at 04:49:05PM +0200, Jyri Sarha wrote:
> From: Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>
> 
> 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 <moinejf-GANU6spQydw@public.gmane.org>
> Signed-off-by: Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org>
> ---
>  .../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
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/tda998x.txt b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
> index e9e4bce..35f6a80 100644
> --- a/Documentation/devicetree/bindings/display/bridge/tda998x.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
> @@ -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.
> +	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.

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

> +    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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC v5 4/8] drm/i2c: tda998x: Add support of a DT graph of ports

[Jean-Francois Moine]

On Thu, 18 Feb 2016 08:35:30 -0600
Rob Herring <robh@kernel.org> wrote:

> On Wed, Feb 17, 2016 at 04:49:05PM +0200, Jyri Sarha wrote:
> > From: Jean-Francois Moine <moinejf@free.fr>
> > 
> > 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 <moinejf@free.fr>
> > Signed-off-by: Jyri Sarha <jsarha@ti.com>
> > ---
> >  .../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
> > 
> > diff --git a/Documentation/devicetree/bindings/display/bridge/tda998x.txt b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
> > index e9e4bce..35f6a80 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/tda998x.txt
> > +++ b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
> > @@ -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.
> > +	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.
> 
> > +	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?
> 
> > +    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.

I would have changed to 'port-value', but as I will not update the
kernel of my Dove Cubox anymore, I will not propose a new version of
this patch (and, sorry Jyri, I will not test your patch series).

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC v5 4/8] drm/i2c: tda998x: Add support of a DT graph of ports

[Russell King - ARM Linux]

On Thu, Feb 18, 2016 at 04:18:23PM +0100, Jean-Francois Moine wrote:
> On Thu, 18 Feb 2016 08:35:30 -0600
> Rob Herring <robh@kernel.org> wrote:
> 
> > On Wed, Feb 17, 2016 at 04:49:05PM +0200, Jyri Sarha wrote:
> > > From: Jean-Francois Moine <moinejf@free.fr>
> > > 
> > > 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 <moinejf@free.fr>
> > > Signed-off-by: Jyri Sarha <jsarha@ti.com>
> > > ---
> > >  .../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
> > > 
> > > diff --git a/Documentation/devicetree/bindings/display/bridge/tda998x.txt b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
> > > index e9e4bce..35f6a80 100644
> > > --- a/Documentation/devicetree/bindings/display/bridge/tda998x.txt
> > > +++ b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
> > > @@ -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.
> > > +	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.
> > 
> > > +	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?
> > 
> > > +    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.

However, ePAPR requires that a node name with @n also have a reg property
with the same n value.  See ePAPR 2.2.1.1.  This statement in it seems
to be particularly clear on this subject:

"The unit-address must match the first address specified in the reg
property of the node. If the node has no reg property, the @ and
unit-address must be omitted and the node-name alone differentiates
the node from other nodes at the same level in the tree."

-- 
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
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC v5 4/8] drm/i2c: tda998x: Add support of a DT graph of ports

[Jyri Sarha]

On 02/18/16 16:35, Rob Herring wrote:
> On Wed, Feb 17, 2016 at 04:49:05PM +0200, Jyri Sarha wrote:
>> From: Jean-Francois Moine <moinejf@free.fr>
>>
>> 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 <moinejf@free.fr>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
>>   .../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
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/tda998x.txt b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
>> index e9e4bce..35f6a80 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/tda998x.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/tda998x.txt
>> @@ -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.
>> +	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.
>

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?

Having the port-type information explicitly written serves the purpose 
keeping the dts files human readable. Is saving couple of bytes this 
important or is there some other reason to not to have the port-type 
property?

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

I never liked this unorthodox usage of reg property either. I'll replace 
the reg -properties with something more explicit.

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

Re: [PATCH RFC v5 4/8] drm/i2c: tda998x: Add support of a DT graph of ports

[Russell King - ARM Linux]

On Thu, Feb 25, 2016 at 03:42:50PM +0200, Jyri Sarha wrote:
> On 02/18/16 16:35, Rob Herring wrote:
> >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.

-- 
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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC v5 4/8] drm/i2c: tda998x: Add support of a DT graph of ports

[Jyri Sarha]

On 02/26/16 02:43, Russell King - ARM Linux wrote:
> On Thu, Feb 25, 2016 at 03:42:50PM +0200, Jyri Sarha wrote:
>> On 02/18/16 16:35, Rob Herring wrote:
>>> 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.
>

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.

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

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

Best regards,
Jyri

ps. Then we still have the alternative of not using the graph/ports 
binding for audio at all. No other audio driver is currently using that 
and the graph binding is not particularly well suited for i2s 
connections as there may be more than two components connected to the 
same wires and sharing the bandwidth with both TDM and different data 
wires. But I am not sure if I want to go there any more as I have a 
feeling that I have been running in circles for couple of years with 
this now.

Re: [PATCH RFC v5 4/8] drm/i2c: tda998x: Add support of a DT graph of ports

[Russell King - ARM Linux]

On Fri, Feb 26, 2016 at 12:14:44PM +0200, Jyri Sarha wrote:
> On 02/26/16 02:43, Russell King - ARM Linux wrote:
> >On Thu, Feb 25, 2016 at 03:42:50PM +0200, Jyri Sarha wrote:
> >>On 02/18/16 16:35, Rob Herring wrote:
> >>>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.
> >
> 
> 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.

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

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.

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

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.

-- 
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
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC v5 4/8] drm/i2c: tda998x: Add support of a DT graph of ports

[Jyri Sarha]

On 02/26/16 13:21, Russell King - ARM Linux wrote:
> On Fri, Feb 26, 2016 at 12:14:44PM +0200, Jyri Sarha wrote:
>> On 02/26/16 02:43, Russell King - ARM Linux wrote:
>>> On Thu, Feb 25, 2016 at 03:42:50PM +0200, Jyri Sarha wrote:
>>>> On 02/18/16 16:35, Rob Herring wrote:
>>>>> 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.
>>>
>>
>> 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.
>

Yes, but surely in the situation where there is 4 data wires, those 
should all be seen as a single audio port, as they are all sharing the 
same bit and frame clock wires. In such a situation I can't see why we 
would ever want to configure anything less that all four connected 
wires. ALSA and CPU—DAI driver can then deal with the situations when we 
only have two channels of data, or the HDMI sink can only play stereo.

If we start to think how to describe multiple alternative endpoints for 
I have to have a unit address, but do I have to have a reg property? I 
did not know about such a rule when I already made an alternative 
binding and implementation that appears to work just fine with multiple 
ports without reg property, but maybe I have over looked some piece of 
specification that forbids that.essentially the same entity (=the single 
i2s port, with up to 4 data wires), then there is no upper limit to the 
amount of ports we'd need to support.

On the other hand, if there are multiple i2s components sharing the same 
clock wires (and maybe some data wires with TDM too), then graph-style 
binding is probably not the best way to describe that kind of setup.

The more I think about this the more I think the graph binding is a bad 
choice to for i2s connections. For SPDIF the graph works better, but why 
use something that does not work for every DAI format?

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

I have to have a unit address, but do I have to have a reg property? I 
did not know about such a rule when I already made an alternative 
binding and implementation that appears to work just fine with multiple 
ports without reg property, but maybe I have over looked some piece of 
specification that forbids that.

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

Yes, this started sound strangely familiar :). Well, I've been trying to 
come up with an upstremable implementation for Beaglebone-Black HDMI 
audio for almost three years already. At this point I am ready to make 
almost what ever compromise if I just could get the code in. I just want 
to explain my point of view.

Luckily the HDMI-codec part is mostly agnostic about these things so 
maybe I can have life of its own.

Best regards,
Jyri

Alternative binding proposal for tda998x audio (Was: Re: [PATCH RFC v5 4/8] drm/i2c: tda998x: Add support of a DT graph of ports)

[Jyri Sarha]

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.

The example assumes a dt-include file with integer definitions for 
TDA998x_SPDIF and TDA998x_I2S.


	tda19988 {
		compatible = "nxp,tda998x";
		reg = <0x70>;
		pinctrl-names = "default", "off";
		pinctrl-0 = <&nxp_hdmi_bonelt_pins>;
		pinctrl-1 = <&nxp_hdmi_bonelt_off_pins>;
		video-ports = <0x230145>;

		/* Required by simple-card. The number of sound-dai
		   cells must agree with the number of audio ports
		   described in audio-ports property. */
		#sound-dai-cells = <2>;

			     /*	DAI-format	AP_ENA reg value */
		audio-ports = <	TDA998x_SPDIF	0x04
				TDA998x_I2S	0x03>;

		port {
			hdmi_0: endpoint@0 {
				remote-endpoint = <&lcdc_0>;
			};
		};
	};

Russell, Rob? Which one would you prefer, the old graph ports binding or 
this simpler version?

Best regards,
Jyri
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Alternative binding proposal for tda998x audio (Was: Re: [PATCH RFC v5 4/8] drm/i2c: tda998x: Add support of a DT graph of ports)

[Jean-Francois Moine]

On Tue, 1 Mar 2016 16:26:50 +0200
Jyri Sarha <jsarha@ti.com> 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

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Alternative binding proposal for tda998x audio (Was: Re: [PATCH RFC v5 4/8] drm/i2c: tda998x: Add support of a DT graph of ports)

[Jyri Sarha]

On 03/01/16 17:35, Jean-Francois Moine wrote:
> On Tue, 1 Mar 2016 16:26:50 +0200
> Jyri Sarha <jsarha@ti.com> 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.

Best regards,
Jyri

Re: Alternative binding proposal for tda998x audio (Was: Re: [PATCH RFC v5 4/8] drm/i2c: tda998x: Add support of a DT graph of ports)

[Jean-Francois Moine]

On Tue, 1 Mar 2016 17:51:09 +0200
Jyri Sarha <jsarha@ti.com> 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.
> 
> 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.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Alternative binding proposal for tda998x audio (Was: Re: [PATCH RFC v5 4/8] drm/i2c: tda998x: Add support of a DT graph of ports)

[Jyri Sarha]

On 03/01/16 18:16, Jean-Francois Moine wrote:
> On Tue, 1 Mar 2016 17:51:09 +0200
> Jyri Sarha <jsarha@ti.com> 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.
>>
>> 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?
>

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.

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

That is true. In the most narrow sense the i2s protocol details, or even 
TDM time-slot selections should not be in the dtb. However, is not 
feasible to write a generic machine driver that would deduce the ideal 
audio configuration just based on the i2s wiring between the audio 
components simply because that is not enough information*. So to put it 
simply the simple-card is not the perfect solution for the problem, but 
even with its flaws it is better than straight out of the box graph 
binding, and it is still entirely feasible to extract all needed 
information for any audio implementation from that binding.

Still even with my proposed binding there is nothing that prevents 
adding the graph binding on top of that if it is ever needed.

Best regards,
Jyri

* With a complete set of information of all audio wiring and component 
capabilities, including the analog only components, it would probably be 
possible to deduce a generic configuration that would work in the most 
common - simple cases, but let's not go there now.

Re: Alternative binding proposal for tda998x audio (Was: Re: [PATCH RFC v5 4/8] drm/i2c: tda998x: Add support of a DT graph of ports)

[Jean-Francois Moine]

On Tue, 1 Mar 2016 20:29:17 +0200
Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org> 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 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.

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.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Alternative binding proposal for tda998x audio (Was: Re: [PATCH RFC v5 4/8] drm/i2c: tda998x: Add support of a DT graph of ports)

[Jyri Sarha]

On 03/01/16 21:26, Jean-Francois Moine wrote:
> On Tue, 1 Mar 2016 20:29:17 +0200
> Jyri Sarha <jsarha@ti.com> 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:
>

The problem is adding redundant unused details into binding without any 
plan of ever using them.

Fundamentally my problem is finding some consensus on the tda998x ASoC 
implementation. I've been reusing your binding for couple of review 
rounds and there has been some well justified critique towards it. I 
feel stupid in pushing forward something that I do not completely agree 
myself, so I decided to try something else.

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

Yes, the necessary information is contained in both bindings. I can live 
with either one of them, but I would prefer my version. Essentially I 
would just like to move forward.

Best regards,
Jyri

Re: Alternative binding proposal for tda998x audio (Was: Re: [PATCH RFC v5 4/8] drm/i2c: tda998x: Add support of a DT graph of ports)

[Mark Brown]

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

On Tue, Mar 01, 2016 at 05:16:30PM +0100, Jean-Francois Moine wrote:

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

The physical integration of audio systems is meaningful hardware that
physically exists and matters to software.  I am completely fed up of 
having to go through this, I'm fairly sure I've been through it with you
before.

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

Documentation/SubmittingPatches.

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

Documentation/devicetree/bindings/sound/simple-card.txt

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

So long as there is no effort on actually upstreaming a graph based card
that shows realistic signs of getting merged in a useful form it doesn't
meaningfully exist.  Right now nobody is even trying to do that.

If you are concerned about simple-card being too Linux specific then by
all means add board specific bindings, preferably ones that can just be
added to simple-card as a compatible string.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH RFC v5 5/8] drm/i2c: tda998x: Remove include/sound/tda998x.h and fix graph parsing

[Jyri Sarha]

Move struct tda998x_audio definition to tda998x_drv.c and remove
include/sound/tda998x.h. There is no external use for struct
tda998x_audio.

Fix graph parsing to allow ports to be inside a separate "ports"-node as
specified in Documentation/devicetree/bindings/graph.txt.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 59 +++++++++++++++++++++------------------
 include/sound/tda998x.h           |  8 ------
 2 files changed, 32 insertions(+), 35 deletions(-)
 delete mode 100644 include/sound/tda998x.h

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 6db1663..b534f94 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -27,10 +27,14 @@
 #include <drm/drm_edid.h>
 #include <drm/drm_of.h>
 #include <drm/i2c/tda998x.h>
-#include <sound/tda998x.h>
 
 #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
 
+struct tda998x_audio {
+	u8 ports[2];			/* AP value */
+	u8 port_types[2];		/* AFMT_xxx */
+};
+
 struct tda998x_priv {
 	struct i2c_client *cec;
 	struct i2c_client *hdmi;
@@ -1209,9 +1213,10 @@ static int tda998x_parse_ports(struct tda998x_priv *priv,
 {
 	struct device_node *of_port;
 	const char *port_type;
-	int ret, audio_index, reg, afmt;
+	int ret, audio_index, reg, afmt, rgb_initialized;
 
 	audio_index = 0;
+	rgb_initialized = 0;
 	for_each_child_of_node(np, of_port) {
 		if (!of_port->name
 		 || of_node_cmp(of_port->name, "port") != 0)
@@ -1221,11 +1226,17 @@ static int tda998x_parse_ports(struct tda998x_priv *priv,
 		if (ret < 0)
 			continue;
 		ret = of_property_read_u32(of_port, "reg", &reg);
+		if (ret < 0) {
+			dev_err(&priv->hdmi->dev, "missing reg for %s\n",
+				port_type);
+			return ret;
+		}
 		if (strcmp(port_type, "rgb") == 0) {
 			if (!ret) {		/* video reg is optional */
 				priv->vip_cntrl_0 = reg >> 16;
 				priv->vip_cntrl_1 = reg >> 8;
 				priv->vip_cntrl_2 = reg;
+				rgb_initialized = 1;
 			}
 			continue;
 		}
@@ -1235,11 +1246,6 @@ static int tda998x_parse_ports(struct tda998x_priv *priv,
 			afmt = AFMT_SPDIF;
 		else
 			continue;
-		if (ret < 0) {
-			dev_err(&priv->hdmi->dev, "missing reg for %s\n",
-				port_type);
-			return ret;
-		}
 		if (audio_index >= ARRAY_SIZE(priv->audio.ports)) {
 			dev_err(&priv->hdmi->dev, "too many audio ports\n");
 			break;
@@ -1248,13 +1254,13 @@ static int tda998x_parse_ports(struct tda998x_priv *priv,
 		priv->audio.port_types[audio_index] = afmt;
 		audio_index++;
 	}
-	return 0;
+	return rgb_initialized;
 }
 
 static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 {
 	struct device_node *np = client->dev.of_node;
-	struct device_node *of_port;
+	struct device_node *ports;
 	u32 video;
 	int rev_lo, rev_hi, ret;
 	unsigned short cec_addr;
@@ -1364,24 +1370,15 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 
 	/* get the device tree parameters */
 	if (np) {
-		of_port = of_get_child_by_name(np, "port");
-		if (of_port) {				/* graph of ports */
-			of_node_put(of_port);
-			ret = tda998x_parse_ports(priv, np);
-			if (ret < 0)
-				goto fail;
-
-			/* initialize the default audio configuration */
-			if (priv->audio.ports[0]) {
-				priv->params.audio_cfg = priv->audio.ports[0];
-				priv->params.audio_format =
-						priv->audio.port_types[0];
-				priv->params.audio_clk_cfg =
-					priv->params.audio_format ==
-							AFMT_SPDIF ? 0 : 1;
-			}
-		} else {
-
+		ports = of_get_child_by_name(np, "ports");
+		if (!ports)
+			ports = of_node_get(np);
+		/* graph of ports */
+		ret = tda998x_parse_ports(priv, ports);
+		of_node_put(ports);
+		if (ret < 0)
+			goto fail;
+		if (ret == 0) {
 			/* optional video properties */
 			ret = of_property_read_u32(np, "video-ports", &video);
 			if (ret == 0) {
@@ -1390,6 +1387,14 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 				priv->vip_cntrl_2 = video;
 			}
 		}
+		if (priv->audio.ports[0]) {
+			priv->params.audio_cfg = priv->audio.ports[0];
+			priv->params.audio_format =
+				priv->audio.port_types[0];
+			priv->params.audio_clk_cfg =
+				priv->params.audio_format ==
+				AFMT_SPDIF ? 0 : 1;
+		}
 	}
 
 	return 0;
diff --git a/include/sound/tda998x.h b/include/sound/tda998x.h
deleted file mode 100644
index bef1da7..0000000
--- a/include/sound/tda998x.h
+++ /dev/null
@@ -1,8 +0,0 @@
-#ifndef SND_TDA998X_H
-#define SND_TDA998X_H
-
-struct tda998x_audio {
-	u8 ports[2];			/* AP value */
-	u8 port_types[2];		/* AFMT_xxx */
-};
-#endif
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC v5 6/8] drm/i2c: tda998x: Improve tda998x_configure_audio() audio related pdata

[Jyri Sarha]

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

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 106 +++++++++++++++++++++-----------------
 include/drm/i2c/tda998x.h         |  23 +++++----
 2 files changed, 73 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index b534f94..42d256a 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -46,7 +46,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;
@@ -673,26 +673,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
@@ -717,19 +707,20 @@ 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);
+	reg_write(priv, REG_ENA_ACLK, params->format == AFMT_SPDIF ? 0 : 1);
 
 	/* Set audio input source */
-	switch (p->audio_format) {
+	switch (params->format) {
 	case AFMT_SPDIF:
 		reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_SPDIF);
 		clksel_aip = AIP_CLKSEL_AIP_SPDIF;
@@ -741,12 +732,25 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 		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);
@@ -762,11 +766,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);
@@ -775,7 +779,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;
@@ -794,19 +798,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 */
@@ -827,9 +825,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.port_types[0] = p->audio_format;
-	priv->audio.ports[0] = p->audio_cfg;
+	priv->audio_params = p->audio;
 }
 
 static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
@@ -1074,9 +1070,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.config) {
+			tda998x_configure_audio(priv,
+						&priv->audio_params,
+						adjusted_mode->clock);
+		}
 	}
 }
 
@@ -1388,12 +1386,28 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 			}
 		}
 		if (priv->audio.ports[0]) {
-			priv->params.audio_cfg = priv->audio.ports[0];
-			priv->params.audio_format =
-				priv->audio.port_types[0];
-			priv->params.audio_clk_cfg =
-				priv->params.audio_format ==
-				AFMT_SPDIF ? 0 : 1;
+			struct tda998x_audio_params params = {
+				.config = priv->audio.ports[0],
+				.format = priv->audio.port_types[0],
+				.sample_width = 24,
+				.sample_rate = 44100,
+				.cea = {
+					.channels = 2,
+					.coding_type = HDMI_AUDIO_CODING_TYPE_STREAM,
+					.sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM,
+					.sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM,
+				},
+				.status = {
+					IEC958_AES0_CON_NOT_COPYRIGHT,
+					IEC958_AES1_CON_GENERAL,
+					IEC958_AES2_CON_SOURCE_UNSPEC |
+					IEC958_AES2_CON_CHANNEL_UNSPEC,
+					IEC958_AES3_CON_CLOCK_1000PPM |
+					IEC958_AES3_CON_FS_NOTID,
+				},
+			};
+
+			priv->audio_params = params;
 		}
 	}
 
diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
index 3e419d9..dfe7829 100644
--- a/include/drm/i2c/tda998x.h
+++ b/include/drm/i2c/tda998x.h
@@ -1,6 +1,18 @@
 #ifndef __DRM_I2C_TDA998X_H__
 #define __DRM_I2C_TDA998X_H__
 
+struct tda998x_audio_params {
+	u8 config;
+	enum {
+		AFMT_SPDIF,
+		AFMT_I2S
+	} 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 +27,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
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC v5 7/8] drm/i2c: tda998x: Register ASoC HDMI codec for audio functionality

[Jyri Sarha]

Register ASoC HDMI codec for audio functionality. This is an initial
ASoC audio implementation for tda998x driver and it does not use all
the features provided by hdmi-codec.

HDMI audio info-frame and audio stream header is generated by the ASoC
HDMI codec. The codec also applies constraints for available
sample-rates.

Implementation of audio_startup for hdmi_codec_ops would enable
tda998x driver to abort ongoing playback if the HDMI cable is
unplugged or re-plugged to a device without audio capability.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/i2c/Kconfig       |   1 +
 drivers/gpu/drm/i2c/tda998x_drv.c | 179 +++++++++++++++++++++++++++++++-------
 include/drm/i2c/tda998x.h         |   1 +
 3 files changed, 148 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig
index 22c7ed6..088f278 100644
--- a/drivers/gpu/drm/i2c/Kconfig
+++ b/drivers/gpu/drm/i2c/Kconfig
@@ -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.
 
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 42d256a..b3b9559 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -20,6 +20,7 @@
 #include <linux/module.h>
 #include <linux/irq.h>
 #include <sound/asoundef.h>
+#include <sound/hdmi-codec.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_atomic_helper.h>
@@ -30,9 +31,9 @@
 
 #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
 
-struct tda998x_audio {
-	u8 ports[2];			/* AP value */
-	u8 port_types[2];		/* AFMT_xxx */
+struct tda998x_audio_port {
+	u8 format;		/* AFMT_xxx */
+	u8 config;		/* AP value */
 };
 
 struct tda998x_priv {
@@ -48,6 +49,8 @@ struct tda998x_priv {
 	u8 vip_cntrl_2;
 	struct tda998x_audio_params audio_params;
 
+	struct platform_device *audio_pdev;
+
 	wait_queue_head_t wq_edid;
 	volatile int wq_edid_wait;
 
@@ -59,7 +62,7 @@ struct tda998x_priv {
 	struct drm_encoder encoder;
 	struct drm_connector connector;
 
-	struct tda998x_audio audio;
+	struct tda998x_audio_port audio_port[2];
 };
 
 #define conn_to_tda998x_priv(x) \
@@ -749,7 +752,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 		break;
 
 	default:
-		BUG();
+		dev_err(&priv->hdmi->dev, "Unsupported I2S format\n");
 		return -EINVAL;
 	}
 
@@ -1070,7 +1073,7 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 
 		tda998x_write_avi(priv, adjusted_mode);
 
-		if (priv->audio_params.config) {
+		if (priv->audio_params.format != AFMT_UNUSED) {
 			tda998x_configure_audio(priv,
 						&priv->audio_params,
 						adjusted_mode->clock);
@@ -1174,6 +1177,8 @@ static int tda998x_connector_get_modes(struct drm_connector *connector)
 	drm_mode_connector_update_edid_property(connector, edid);
 	n = drm_add_edid_modes(connector, edid);
 	priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid);
+	drm_edid_to_eld(connector, edid);
+
 	kfree(edid);
 
 	return n;
@@ -1195,6 +1200,9 @@ static void tda998x_destroy(struct tda998x_priv *priv)
 	cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
 	reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
 
+	if (priv->audio_pdev)
+		platform_device_unregister(priv->audio_pdev);
+
 	if (priv->hdmi->irq)
 		free_irq(priv->hdmi->irq, priv);
 
@@ -1204,6 +1212,133 @@ static void tda998x_destroy(struct tda998x_priv *priv)
 	i2c_unregister_device(priv->cec);
 }
 
+static int tda998x_audio_hw_params(struct device *dev,
+				   struct hdmi_codec_daifmt *daifmt,
+				   struct hdmi_codec_params *params)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	int i, ret;
+	struct tda998x_audio_params audio = {
+		.sample_width = params->sample_width,
+		.sample_rate = params->sample_rate,
+		.cea = params->cea,
+	};
+
+	if (!priv->encoder.crtc)
+		return -ENODEV;
+
+	memcpy(audio.status, params->iec.status,
+	       min(sizeof(audio.status), sizeof(params->iec.status)));
+
+	switch (daifmt->fmt) {
+	case HDMI_I2S:
+		if (daifmt->bit_clk_inv || daifmt->frame_clk_inv ||
+		    daifmt->bit_clk_master || daifmt->frame_clk_master) {
+			dev_err(dev, "%s: Bad flags %d %d %d %d\n", __func__,
+				daifmt->bit_clk_inv, daifmt->frame_clk_inv,
+				daifmt->bit_clk_master,
+				daifmt->frame_clk_master);
+			return -EINVAL;
+		}
+		for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
+			if (priv->audio_port[i].format == AFMT_I2S)
+				audio.config = priv->audio_port[i].config;
+		audio.format = AFMT_I2S;
+		break;
+	case HDMI_SPDIF:
+		for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
+			if (priv->audio_port[i].format == AFMT_SPDIF)
+				audio.config = priv->audio_port[i].config;
+		audio.format = AFMT_SPDIF;
+		break;
+	default:
+		dev_err(dev, "%s: Invalid format %d\n", __func__, daifmt->fmt);
+		return -EINVAL;
+	}
+
+	if (audio.config == 0) {
+		dev_err(dev, "%s: No audio configutation found\n", __func__);
+		return -EINVAL;
+	}
+
+	ret = tda998x_configure_audio(priv,
+				      &audio,
+				      priv->encoder.crtc->hwmode.clock);
+
+	return ret;
+}
+
+static void tda998x_audio_shutdown(struct device *dev)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+
+	reg_write(priv, REG_ENA_AP, 0);
+}
+
+int tda998x_audio_digital_mute(struct device *dev, bool enable)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+
+	tda998x_audio_mute(priv, enable);
+
+	return 0;
+}
+
+static int tda998x_audio_get_eld(struct device *dev, uint8_t *buf, size_t len)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	struct drm_mode_config *config = &priv->encoder.dev->mode_config;
+	struct drm_connector *connector;
+	int ret = -ENODEV;
+
+	mutex_lock(&config->mutex);
+	list_for_each_entry(connector, &config->connector_list, head) {
+		if (&priv->encoder == connector->encoder) {
+			memcpy(buf, connector->eld,
+			       min(sizeof(connector->eld), len));
+			ret = 0;
+		}
+	}
+	mutex_unlock(&config->mutex);
+
+	return ret;
+}
+
+static const struct hdmi_codec_ops audio_codec_ops = {
+	.hw_params = tda998x_audio_hw_params,
+	.audio_shutdown = tda998x_audio_shutdown,
+	.digital_mute = tda998x_audio_digital_mute,
+	.get_eld = tda998x_audio_get_eld,
+};
+
+static int tda998x_audio_codec_init(struct tda998x_priv *priv,
+				    struct device *dev)
+{
+	struct hdmi_codec_pdata codec_data = {
+		.ops = &audio_codec_ops,
+		.max_i2s_channels = 2,
+	};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) {
+		if (priv->audio_port[i].format == AFMT_I2S &&
+		    priv->audio_port[i].config != 0)
+			codec_data.i2s = 1;
+		if (priv->audio_port[i].format == AFMT_SPDIF &&
+		    priv->audio_port[i].config != 0)
+			codec_data.spdif = 1;
+	}
+
+	priv->audio_pdev = platform_device_register_data(
+		dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO,
+		&codec_data, sizeof(codec_data));
+
+	if (IS_ERR(priv->audio_pdev))
+		return PTR_ERR(priv->audio_pdev);
+
+	return 0;
+}
+
 /* I2C driver functions */
 
 static int tda998x_parse_ports(struct tda998x_priv *priv,
@@ -1244,12 +1379,12 @@ static int tda998x_parse_ports(struct tda998x_priv *priv,
 			afmt = AFMT_SPDIF;
 		else
 			continue;
-		if (audio_index >= ARRAY_SIZE(priv->audio.ports)) {
+		if (audio_index >= ARRAY_SIZE(priv->audio_port)) {
 			dev_err(&priv->hdmi->dev, "too many audio ports\n");
 			break;
 		}
-		priv->audio.ports[audio_index] = reg;
-		priv->audio.port_types[audio_index] = afmt;
+		priv->audio_port[audio_index].format = afmt;
+		priv->audio_port[audio_index].config = reg;
 		audio_index++;
 	}
 	return rgb_initialized;
@@ -1385,30 +1520,8 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 				priv->vip_cntrl_2 = video;
 			}
 		}
-		if (priv->audio.ports[0]) {
-			struct tda998x_audio_params params = {
-				.config = priv->audio.ports[0],
-				.format = priv->audio.port_types[0],
-				.sample_width = 24,
-				.sample_rate = 44100,
-				.cea = {
-					.channels = 2,
-					.coding_type = HDMI_AUDIO_CODING_TYPE_STREAM,
-					.sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM,
-					.sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM,
-				},
-				.status = {
-					IEC958_AES0_CON_NOT_COPYRIGHT,
-					IEC958_AES1_CON_GENERAL,
-					IEC958_AES2_CON_SOURCE_UNSPEC |
-					IEC958_AES2_CON_CHANNEL_UNSPEC,
-					IEC958_AES3_CON_CLOCK_1000PPM |
-					IEC958_AES3_CON_FS_NOTID,
-				},
-			};
-
-			priv->audio_params = params;
-		}
+		if (priv->audio_port[0].format != AFMT_UNUSED)
+			tda998x_audio_codec_init(priv, &client->dev);
 	}
 
 	return 0;
diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
index dfe7829..5a0cabb 100644
--- a/include/drm/i2c/tda998x.h
+++ b/include/drm/i2c/tda998x.h
@@ -4,6 +4,7 @@
 struct tda998x_audio_params {
 	u8 config;
 	enum {
+		AFMT_UNUSED = 0,
 		AFMT_SPDIF,
 		AFMT_I2S
 	} format;
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH RFC v5 8/8] ARM: dts: am335x-boneblack: Add HDMI audio support

[Jyri Sarha]

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 <jsarha@ti.com>
---
 arch/arm/boot/dts/am335x-boneblack.dts | 90 ++++++++++++++++++++++++++++++++--
 1 file changed, 86 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/am335x-boneblack.dts b/arch/arm/boot/dts/am335x-boneblack.dts
index eadbba3..05347dc 100644
--- a/arch/arm/boot/dts/am335x-boneblack.dts
+++ b/arch/arm/boot/dts/am335x-boneblack.dts
@@ -64,6 +64,16 @@
 			0x1b0 0x03      /* xdma_event_intr0, OMAP_MUX_MODE3 | AM33XX_PIN_OUTPUT */
 		>;
 	};
+
+	mcasp0_pins: mcasp0_pins {
+		pinctrl-single,pins = <
+			0x1ac (PIN_INPUT_PULLUP | MUX_MODE0)	/* mcasp0_ahclkx.mcasp0_ahclkx */
+			0x19c (PIN_OUTPUT_PULLDOWN | MUX_MODE2) /* mcasp0_ahclkr.mcasp0_axr2 */
+			0x194 (PIN_OUTPUT_PULLUP | MUX_MODE0)	/* mcasp0_fsx.mcasp0_fsx */
+			0x190 (PIN_OUTPUT_PULLDOWN | MUX_MODE0)	/* mcasp0_aclkx.mcasp0_aclkx */
+			0x06c (PIN_OUTPUT_PULLDOWN | MUX_MODE7)	/* gpmc_a11.GPIO1_27 */
+		>;
+	};
 };
 
 &lcdc {
@@ -76,17 +86,89 @@
 };
 
 &i2c0 {
-	tda19988 {
+	tda19988: tda19988 {
 		compatible = "nxp,tda998x";
 		reg = <0x70>;
+
+		#sound-dai-cells = <0>;
+
 		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>;
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			port@0 {
+				port-type = "rgb";
+				reg = <0x230145>;
+				hdmi_0: endpoint@0 {
+					remote-endpoint = <&lcdc_0>;
+				};
 			};
+			port@1 {
+				port-type = "i2s";
+				reg = <0x03>;
+				tda19988_i2s: endpoint {
+					remote-endpoint = <&mcasp0_i2s>;
+				};
+			};
+		};
+	};
+};
+
+&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>;
+
+	port {
+		mcasp0_i2s: endpoint {
+			remote-endpoint = <&tda19988_i2s>;
+		};
+	};
+};
+
+/ {
+	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
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC v5 8/8] ARM: dts: am335x-boneblack: Add HDMI audio support

[Robert Nelson]

On Wed, Feb 17, 2016 at 8:49 AM, Jyri Sarha <jsarha@ti.com> wrote:
> 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 <jsarha@ti.com>
> ---
>  arch/arm/boot/dts/am335x-boneblack.dts | 90 ++++++++++++++++++++++++++++++++--
>  1 file changed, 86 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/boot/dts/am335x-boneblack.dts b/arch/arm/boot/dts/am335x-boneblack.dts
> index eadbba3..05347dc 100644
> --- a/arch/arm/boot/dts/am335x-boneblack.dts
> +++ b/arch/arm/boot/dts/am335x-boneblack.dts
> @@ -64,6 +64,16 @@
>                         0x1b0 0x03      /* xdma_event_intr0, OMAP_MUX_MODE3 | AM33XX_PIN_OUTPUT */
>                 >;
>         };
> +
> +       mcasp0_pins: mcasp0_pins {
> +               pinctrl-single,pins = <
> +                       0x1ac (PIN_INPUT_PULLUP | MUX_MODE0)    /* mcasp0_ahclkx.mcasp0_ahclkx */
> +                       0x19c (PIN_OUTPUT_PULLDOWN | MUX_MODE2) /* mcasp0_ahclkr.mcasp0_axr2 */
> +                       0x194 (PIN_OUTPUT_PULLUP | MUX_MODE0)   /* mcasp0_fsx.mcasp0_fsx */
> +                       0x190 (PIN_OUTPUT_PULLDOWN | MUX_MODE0) /* mcasp0_aclkx.mcasp0_aclkx */
> +                       0x06c (PIN_OUTPUT_PULLDOWN | MUX_MODE7) /* gpmc_a11.GPIO1_27 */

Hi Jyri,

We've switched to AM33XX_IOPAD,  please squash this in here:

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

Regards,

-- 
Robert Nelson
https://rcn-ee.com/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel