[PATCH v8 0/2] ASoC: tda998x: add a codec to the HDMI transmitter

[PATCH v8 0/2] ASoC: tda998x: add a codec to the HDMI transmitter

[Jean-Francois Moine]

The NXP TDA998x HDMI transmitter may transmit audio to the HDMI link
from 2 different sources, I2S and S/PDIF.

This patch set first adds an interface between a HDMI transmitter and
the HDMI CODEC.

The interface is then used by the TDA998x driver to describe its audio
capabilities (DAIs), to give the audio constraints of the HDMI
device (EDID) to the audio subsystem, and to connect the chosen audio
source to the HDMI link.

v8:
	- change some comments about the patches
v7:
	- remove the change of the K predivider (Jyri Sarha)
	- add S24_3LE and S32_LE as possible audio formats (Jyri Sarha)
	- don't move the struct priv2 definition and use the
	  slave encoder private data as the device private data
	  (Russell King)
	- remove the useless request_module (Russell King/Mark Brown)
	- don't lock the HDMI module (Russell King)
	- use platform_device_unregister to remove the codec
	  (Russell King)
v6:
	- extend the HDMI CODEC instead of using a specific CODEC
v5:
	- use the TDA998x private data instead of a specific area
	  for the CODEC interface
	- the CODEC is TDA998x specific (Mark Brown)
v4:
	- remove all the TDA998x specific stuff from the CODEC
	- move the EDID scan from the CODEC to the TDA998x
	- move the CODEC to sound/soc (Mark Brown)
	- update the audio_sample_rate from the EDID (Andrew Jackson)
v3: fix bad rate (Andrew Jackson)
v2: check double stream start (Mark Brown)

Jean-Francois Moine (2):
  ASoC: hdmi: Add a transmitter interface to the HDMI CODEC
  drm/i2c: tda998x: Use the HDMI audio CODEC interface

 .../devicetree/bindings/drm/i2c/tda998x.txt        |  18 ++
 drivers/gpu/drm/i2c/Kconfig                        |   1 +
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 264 ++++++++++++++++++++-
 include/sound/hdmi.h                               |  20 ++
 sound/soc/codecs/hdmi.c                            | 176 +++++++++++++-
 5 files changed, 460 insertions(+), 19 deletions(-)
 create mode 100644 include/sound/hdmi.h

-- 
2.1.1

[PATCH v8 1/2] ASoC: hdmi: Add a transmitter interface to the HDMI CODEC

[Jean-Francois Moine]

Audio in HDMI asks for:
- the audio constraints of the HDMI device to be known when choosing
  the audio routes in the audio subsystem, and
- the HDMI transmitter to know which of its audio inputs has been
  chosen when audio streaming starts.

This patch adds the interface between a HDMI transmitter and the
HDMI CODEC.

At startup time, the HDMI transmitter device creates the HDMI device as
a child device giving a description of its DAIs and 2 callback functions,
these functions realizing the HDMI audio requirements.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 include/sound/hdmi.h    |  20 ++++++
 sound/soc/codecs/hdmi.c | 176 ++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 190 insertions(+), 6 deletions(-)
 create mode 100644 include/sound/hdmi.h

diff --git a/include/sound/hdmi.h b/include/sound/hdmi.h
new file mode 100644
index 0000000..49062c7
--- /dev/null
+++ b/include/sound/hdmi.h
@@ -0,0 +1,20 @@
+#ifndef SND_HDMI_H
+#define SND_HDMI_H
+
+#include <sound/soc.h>
+
+/* platform_data */
+struct hdmi_data {
+	int (*get_audio)(struct device *dev,
+			 int *max_channels,
+			 int *rate_mask,
+			 int *fmt);
+	void (*audio_switch)(struct device *dev,
+			     int port_index,
+			     unsigned sample_rate,
+			     int sample_format);
+	int ndais;
+	struct snd_soc_dai_driver *dais;
+	struct snd_soc_codec_driver *driver;
+};
+#endif
diff --git a/sound/soc/codecs/hdmi.c b/sound/soc/codecs/hdmi.c
index 1391ad5..f66f1f6 100644
--- a/sound/soc/codecs/hdmi.c
+++ b/sound/soc/codecs/hdmi.c
@@ -22,9 +22,148 @@
 #include <sound/soc.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <sound/pcm_params.h>
+#include <sound/hdmi.h>
 
 #define DRV_NAME "hdmi-audio-codec"
 
+struct hdmi_priv {
+	struct hdmi_data hdmi_data;
+	struct snd_pcm_hw_constraint_list rate_constraints;
+};
+
+static int hdmi_dev_match(struct device *dev, void *data)
+{
+	return !strcmp(dev_name(dev), (char *) data);
+}
+
+/* get the codec device */
+static struct device *hdmi_get_cdev(struct device *dev)
+{
+	struct device *cdev;
+
+	cdev = device_find_child(dev,
+				 DRV_NAME,
+				 hdmi_dev_match);
+	if (!cdev)
+		dev_err(dev, "Cannot get codec device");
+	else
+		put_device(cdev);
+	return cdev;
+}
+
+static int hdmi_startup(struct snd_pcm_substream *substream,
+			struct snd_soc_dai *dai)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct device *cdev;
+	struct hdmi_priv *priv;
+	struct snd_pcm_hw_constraint_list *rate_constraints;
+	int ret, max_channels, rate_mask, fmt;
+	u64 formats;
+	static const u32 hdmi_rates[] = {
+		32000, 44100, 48000, 88200, 96000, 176400, 192000
+	};
+
+	cdev = hdmi_get_cdev(dai->dev);
+	if (!cdev)
+		return -ENODEV;
+	priv = dev_get_drvdata(cdev);
+
+	/* get the EDID values and the rate constraints buffer */
+	ret = priv->hdmi_data.get_audio(dai->dev,
+					&max_channels, &rate_mask, &fmt);
+	if (ret < 0)
+		return ret;				/* no screen */
+
+	/* convert the EDID values to audio constraints */
+	rate_constraints = &priv->rate_constraints;
+	rate_constraints->list = hdmi_rates;
+	rate_constraints->count = ARRAY_SIZE(hdmi_rates);
+	rate_constraints->mask = rate_mask;
+	snd_pcm_hw_constraint_list(runtime, 0,
+				   SNDRV_PCM_HW_PARAM_RATE,
+				   rate_constraints);
+
+	formats = 0;
+	if (fmt & 1)
+		formats |= SNDRV_PCM_FMTBIT_S16_LE;
+	if (fmt & 2)
+		formats |= SNDRV_PCM_FMTBIT_S20_3LE;
+	if (fmt & 4)
+		formats |= SNDRV_PCM_FMTBIT_S24_LE |
+			   SNDRV_PCM_FMTBIT_S24_3LE |
+			   SNDRV_PCM_FMTBIT_S32_LE;
+	snd_pcm_hw_constraint_mask64(runtime,
+				SNDRV_PCM_HW_PARAM_FORMAT,
+				formats);
+
+	snd_pcm_hw_constraint_minmax(runtime,
+				SNDRV_PCM_HW_PARAM_CHANNELS,
+				1, max_channels);
+	return 0;
+}
+
+static int hdmi_hw_params(struct snd_pcm_substream *substream,
+			  struct snd_pcm_hw_params *params,
+			  struct snd_soc_dai *dai)
+{
+	struct device *cdev;
+	struct hdmi_priv *priv;
+
+	cdev = hdmi_get_cdev(dai->dev);
+	if (!cdev)
+		return -ENODEV;
+	priv = dev_get_drvdata(cdev);
+
+	priv->hdmi_data.audio_switch(dai->dev, dai->id,
+				     params_rate(params),
+				     params_format(params));
+	return 0;
+}
+
+static void hdmi_shutdown(struct snd_pcm_substream *substream,
+			  struct snd_soc_dai *dai)
+{
+	struct device *cdev;
+	struct hdmi_priv *priv;
+
+	cdev = hdmi_get_cdev(dai->dev);
+	if (!cdev)
+		return;
+	priv = dev_get_drvdata(cdev);
+
+	priv->hdmi_data.audio_switch(dai->dev, -1, 0, 0);	/* stop */
+}
+
+static const struct snd_soc_dai_ops hdmi_ops = {
+	.startup = hdmi_startup,
+	.hw_params = hdmi_hw_params,
+	.shutdown = hdmi_shutdown,
+};
+
+static int hdmi_codec_probe(struct snd_soc_codec *codec)
+{
+	struct hdmi_priv *priv;
+	struct device *dev = codec->dev;	/* encoder device */
+	struct device *cdev;			/* codec device */
+
+	cdev = hdmi_get_cdev(dev);
+	if (!cdev)
+		return -ENODEV;
+
+	/* allocate some memory to store
+	 * the encoder callback functions and the rate constraints */
+	priv = devm_kzalloc(cdev, sizeof *priv, GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	dev_set_drvdata(cdev, priv);
+
+	memcpy(&priv->hdmi_data, cdev->platform_data,
+				sizeof priv->hdmi_data);
+	return 0;
+}
+
 static const struct snd_soc_dapm_widget hdmi_widgets[] = {
 	SND_SOC_DAPM_INPUT("RX"),
 	SND_SOC_DAPM_OUTPUT("TX"),
@@ -79,13 +218,38 @@ static struct snd_soc_codec_driver hdmi_codec = {
 	.ignore_pmdown_time = true,
 };
 
-static int hdmi_codec_probe(struct platform_device *pdev)
+static int hdmi_codec_dev_probe(struct platform_device *pdev)
 {
-	return snd_soc_register_codec(&pdev->dev, &hdmi_codec,
-			&hdmi_codec_dai, 1);
+	struct hdmi_data *pdata = pdev->dev.platform_data;
+	struct snd_soc_dai_driver *dais;
+	struct snd_soc_codec_driver *driver;
+	int i, ret;
+
+	if (!pdata)
+		return snd_soc_register_codec(&pdev->dev, &hdmi_codec,
+						&hdmi_codec_dai, 1);
+
+	/* creation from a video encoder as a child device */
+	dais = devm_kmemdup(&pdev->dev,
+			    pdata->dais,
+			    sizeof *pdata->dais * pdata->ndais,
+			    GFP_KERNEL);
+	for (i = 0; i < pdata->ndais; i++)
+		dais[i].ops = &hdmi_ops;
+
+	driver = devm_kmemdup(&pdev->dev,
+			    pdata->driver,
+			    sizeof *pdata->driver,
+			    GFP_KERNEL);
+	driver->probe = hdmi_codec_probe;
+
+	/* register the codec on the video encoder */
+	ret = snd_soc_register_codec(pdev->dev.parent, driver,
+					dais, pdata->ndais);
+	return ret;
 }
 
-static int hdmi_codec_remove(struct platform_device *pdev)
+static int hdmi_codec_dev_remove(struct platform_device *pdev)
 {
 	snd_soc_unregister_codec(&pdev->dev);
 	return 0;
@@ -98,8 +262,8 @@ static struct platform_driver hdmi_codec_driver = {
 		.of_match_table = of_match_ptr(hdmi_audio_codec_ids),
 	},
 
-	.probe		= hdmi_codec_probe,
-	.remove		= hdmi_codec_remove,
+	.probe		= hdmi_codec_dev_probe,
+	.remove		= hdmi_codec_dev_remove,
 };
 
 module_platform_driver(hdmi_codec_driver);
-- 
2.1.1

Re: [PATCH v8 1/2] ASoC: hdmi: Add a transmitter interface to the HDMI CODEC

[Mark Brown]

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

On Thu, Oct 23, 2014 at 09:09:35AM +0200, Jean-Francois Moine wrote:

> +struct hdmi_data {
> +	int (*get_audio)(struct device *dev,
> +			 int *max_channels,
> +			 int *rate_mask,
> +			 int *fmt);
> +	void (*audio_switch)(struct device *dev,
> +			     int port_index,
> +			     unsigned sample_rate,
> +			     int sample_format);

I can't really tell from these function names what these functions are
supposed to do.  I think get_audio() should be get_audio_caps() or
similar since it reads the capabilities and audio_switch() is supposed
to be set_audio_params() or similar to set the settings for the running
stream.

> +	snd_pcm_hw_constraint_list(runtime, 0,
> +				   SNDRV_PCM_HW_PARAM_RATE,
> +				   rate_constraints);
> +
> +	formats = 0;
> +	if (fmt & 1)
> +		formats |= SNDRV_PCM_FMTBIT_S16_LE;
> +	if (fmt & 2)
> +		formats |= SNDRV_PCM_FMTBIT_S20_3LE;
> +	if (fmt & 4)
> +		formats |= SNDRV_PCM_FMTBIT_S24_LE |
> +			   SNDRV_PCM_FMTBIT_S24_3LE |
> +			   SNDRV_PCM_FMTBIT_S32_LE;

Magic numbers here - can't we have some constants defined?

> +	priv->hdmi_data.audio_switch(dai->dev, dai->id,
> +				     params_rate(params),
> +				     params_format(params));

I'd be happier if this were able to return an error; even if the
constraints are satisfied perhaps something changed or some operation
fails for some reason.

> +static void hdmi_shutdown(struct snd_pcm_substream *substream,
> +			  struct snd_soc_dai *dai)
> +{
> +	struct device *cdev;
> +	struct hdmi_priv *priv;
> +
> +	cdev = hdmi_get_cdev(dai->dev);
> +	if (!cdev)
> +		return;
> +	priv = dev_get_drvdata(cdev);
> +
> +	priv->hdmi_data.audio_switch(dai->dev, -1, 0, 0);	/* stop */
> +}

So the set parameters operation has to support these magic values as
being "off" - why not have an explicit shutdown operation here?

> +static int hdmi_codec_probe(struct snd_soc_codec *codec)
> +{
> +	struct hdmi_priv *priv;
> +	struct device *dev = codec->dev;	/* encoder device */
> +	struct device *cdev;			/* codec device */
> +
> +	cdev = hdmi_get_cdev(dev);
> +	if (!cdev)
> +		return -ENODEV;

This is (I think) only called for cases where the driver is being used
from a parent device with ops but the name makes it sound like it should
be called all the time so errors like that shouldn't happen.  This
should be clearer, or perhaps we should just have a separate device
(perhaps rename the existing one to say that it's for a dumb device with
no control?).

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

[PATCH v8 2/2] drm/i2c: tda998x: Use the HDMI audio CODEC interface

[Jean-Francois Moine]

This patch adds the necessary functions to interface the HDMI audio CODEC.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 .../devicetree/bindings/drm/i2c/tda998x.txt        |  18 ++
 drivers/gpu/drm/i2c/Kconfig                        |   1 +
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 264 ++++++++++++++++++++-
 3 files changed, 270 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
index e9e4bce..e50e7cd 100644
--- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
+++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
@@ -17,6 +17,20 @@ Optional properties:
   - video-ports: 24 bits value which defines how the video controller
 	output is wired to the TDA998x input - default: <0x230145>
 
+  - audio-ports: must contain one or two values selecting the source
+	in the audio port.
+	The source type is given by the corresponding entry in
+	the audio-port-names property.
+
+  - audio-port-names: must contain entries matching the entries in
+	the audio-ports property.
+	Each value may be "i2s" or "spdif", giving the type of
+	the audio source.
+
+  - #sound-dai-cells: must be set to <1> for use with the simple-card.
+	The TDA998x audio CODEC always defines two DAIs.
+	The DAI 0 is the S/PDIF input and the DAI 1 is the I2S input.
+
 Example:
 
 	tda998x: hdmi-encoder {
@@ -26,4 +40,8 @@ Example:
 		interrupts = <27 2>;		/* falling edge */
 		pinctrl-0 = <&pmx_camera>;
 		pinctrl-names = "default";
+
+		audio-ports = <0x04>, <0x03>;
+		audio-port-names = "spdif", "i2s";
+		#sound-dai-cells = <1>;
 	};
diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig
index 4d341db..42ca744 100644
--- a/drivers/gpu/drm/i2c/Kconfig
+++ b/drivers/gpu/drm/i2c/Kconfig
@@ -22,6 +22,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
 	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 d476279..e558e1e 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -20,12 +20,14 @@
 #include <linux/module.h>
 #include <linux/irq.h>
 #include <sound/asoundef.h>
+#include <linux/platform_device.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_encoder_slave.h>
 #include <drm/drm_edid.h>
 #include <drm/i2c/tda998x.h>
+#include <sound/hdmi.h>
 
 #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
 
@@ -44,6 +46,16 @@ struct tda998x_priv {
 	wait_queue_head_t wq_edid;
 	volatile int wq_edid_wait;
 	struct drm_encoder *encoder;
+
+	/* audio variables */
+	struct platform_device *pdev_codec;
+	u8 audio_ports[2];
+
+	u8 max_channels;		/* EDID parameters */
+	u8 rate_mask;
+	u8 fmt;
+
+	int audio_sample_format;
 };
 
 #define to_tda998x_priv(x)  ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv)
@@ -624,6 +636,8 @@ tda998x_write_avi(struct tda998x_priv *priv, struct drm_display_mode *mode)
 			 sizeof(buf));
 }
 
+/* audio functions */
+
 static void tda998x_audio_mute(struct tda998x_priv *priv, bool on)
 {
 	if (on) {
@@ -639,12 +653,11 @@ static void
 tda998x_configure_audio(struct tda998x_priv *priv,
 		struct drm_display_mode *mode, struct tda998x_encoder_params *p)
 {
-	uint8_t buf[6], clksel_aip, clksel_fs, cts_n, adiv;
+	uint8_t buf[6], clksel_aip, clksel_fs, cts_n, adiv, aclk;
 	uint32_t n;
 
 	/* Enable audio ports */
 	reg_write(priv, REG_ENA_AP, p->audio_cfg);
-	reg_write(priv, REG_ENA_ACLK, p->audio_clk_cfg);
 
 	/* Set audio input source */
 	switch (p->audio_format) {
@@ -653,6 +666,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 		clksel_aip = AIP_CLKSEL_AIP_SPDIF;
 		clksel_fs = AIP_CLKSEL_FS_FS64SPDIF;
 		cts_n = CTS_N_M(3) | CTS_N_K(3);
+		aclk = 0;				/* no clock */
 		break;
 
 	case AFMT_I2S:
@@ -660,6 +674,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 		clksel_aip = AIP_CLKSEL_AIP_I2S;
 		clksel_fs = AIP_CLKSEL_FS_ACLK;
 		cts_n = CTS_N_M(3) | CTS_N_K(3);
+		aclk = 1;				/* clock enable */
 		break;
 
 	default:
@@ -671,6 +686,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_LAYOUT |
 					AIP_CNTRL_0_ACR_MAN);	/* auto CTS */
 	reg_write(priv, REG_CTS_N, cts_n);
+	reg_write(priv, REG_ENA_ACLK, aclk);
 
 	/*
 	 * Audio input somehow depends on HDMI line rate which is
@@ -727,6 +743,137 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	tda998x_write_aif(priv, p);
 }
 
+/* audio codec interface */
+
+/* return the audio parameters extracted from the last EDID */
+static int tda998x_get_audio(struct device *dev,
+			int *max_channels,
+			int *rate_mask,
+			int *fmt)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+
+	if (!priv->encoder->crtc)
+		return -ENODEV;
+
+	*max_channels = priv->max_channels;
+	*rate_mask = priv->rate_mask;
+	*fmt = priv->fmt;
+	return 0;
+}
+
+/* switch the audio port and initialize the audio parameters for streaming */
+static void tda998x_audio_switch(struct device *dev,
+				 int port_index,
+				 unsigned sample_rate,
+				 int sample_format)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	struct tda998x_encoder_params *p = &priv->params;
+
+	if (!priv->encoder->crtc)
+		return;
+
+	/*
+	 * if port_index is negative (streaming stop),
+	 * disable the audio port
+	 */
+	if (port_index < 0) {
+		reg_write(priv, REG_ENA_AP, 0);
+		return;
+	}
+
+	/* if same audio parameters, just enable the audio port */
+	if (p->audio_cfg == priv->audio_ports[port_index] &&
+	    p->audio_sample_rate == sample_rate &&
+	    priv->audio_sample_format == sample_format) {
+		reg_write(priv, REG_ENA_AP, p->audio_cfg);
+		return;
+	}
+
+	p->audio_format = port_index;
+	p->audio_cfg = priv->audio_ports[port_index];
+	p->audio_sample_rate = sample_rate;
+	priv->audio_sample_format = sample_format;
+	tda998x_configure_audio(priv, &priv->encoder->crtc->hwmode, p);
+}
+
+#define TDA998X_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | \
+			SNDRV_PCM_FMTBIT_S20_3LE | \
+			SNDRV_PCM_FMTBIT_S24_LE | \
+			SNDRV_PCM_FMTBIT_S32_LE)
+
+static struct snd_soc_dai_driver tda998x_dais[] = {
+	{
+		.name = "spdif-hifi",
+		.id = AFMT_SPDIF,
+		.playback = {
+			.stream_name	= "HDMI SPDIF Playback",
+			.channels_min	= 1,
+			.channels_max	= 2,
+			.rates		= SNDRV_PCM_RATE_CONTINUOUS,
+			.rate_min	= 22050,
+			.rate_max	= 192000,
+			.formats	= TDA998X_FORMATS,
+		},
+	},
+	{
+		.name = "i2s-hifi",
+		.id = AFMT_I2S,
+		.playback = {
+			.stream_name	= "HDMI I2S Playback",
+			.channels_min	= 1,
+			.channels_max	= 8,
+			.rates		= SNDRV_PCM_RATE_CONTINUOUS,
+			.rate_min	= 5512,
+			.rate_max	= 192000,
+			.formats	= TDA998X_FORMATS,
+		},
+	},
+};
+
+static const struct snd_soc_dapm_widget tda998x_widgets[] = {
+	SND_SOC_DAPM_OUTPUT("hdmi-out"),
+};
+static const struct snd_soc_dapm_route tda998x_routes[] = {
+	{ "hdmi-out", NULL, "HDMI I2S Playback" },
+	{ "hdmi-out", NULL, "HDMI SPDIF Playback" },
+};
+
+static struct snd_soc_codec_driver tda998x_codec_driver = {
+	.dapm_widgets = tda998x_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(tda998x_widgets),
+	.dapm_routes = tda998x_routes,
+	.num_dapm_routes = ARRAY_SIZE(tda998x_routes),
+};
+
+static struct hdmi_data tda998x_hdmi_data = {
+	.get_audio = tda998x_get_audio,
+	.audio_switch = tda998x_audio_switch,
+	.ndais = ARRAY_SIZE(tda998x_dais),
+	.dais = tda998x_dais,
+	.driver = &tda998x_codec_driver,
+};
+
+static void tda998x_create_audio_codec(struct tda998x_priv *priv)
+{
+	struct platform_device *pdev;
+
+	pdev = platform_device_register_resndata(&priv->hdmi->dev,
+						 "hdmi-audio-codec",
+						  PLATFORM_DEVID_NONE,
+						  NULL, 0,
+						  &tda998x_hdmi_data,
+						  sizeof tda998x_hdmi_data);
+	if (IS_ERR(pdev)) {
+		dev_err(&priv->hdmi->dev, "cannot create codec: %ld\n",
+			PTR_ERR(pdev));
+		return;
+	}
+
+	priv->pdev_codec = pdev;
+}
+
 /* DRM encoder functions */
 
 static void tda998x_encoder_set_config(struct tda998x_priv *priv,
@@ -746,6 +893,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_ports[p->audio_format] = p->audio_cfg;
+	priv->audio_sample_format = SNDRV_PCM_FORMAT_S24_LE;
 }
 
 static void tda998x_encoder_dpms(struct tda998x_priv *priv, int mode)
@@ -1128,6 +1277,47 @@ fail:
 	return NULL;
 }
 
+static void tda998x_set_audio(struct tda998x_priv *priv,
+			      struct drm_connector *connector)
+{
+	u8 *eld = connector->eld;
+	u8 *sad;
+	int sad_count;
+	unsigned eld_ver, mnl, rate_mask;
+	unsigned max_channels, fmt;
+
+	/* adjust the hw params from the ELD (EDID) */
+	eld_ver = eld[0] >> 3;
+	if (eld_ver != 2 && eld_ver != 31)
+		return;
+
+	mnl = eld[4] & 0x1f;
+	if (mnl > 16)
+		return;
+
+	sad_count = eld[5] >> 4;
+	sad = eld + 20 + mnl;
+
+	/* Start from the basic audio settings */
+	max_channels = 2;
+	rate_mask = 0;
+	fmt = 0;
+	while (sad_count--) {
+		switch (sad[0] & 0x78) {
+		case 0x08: /* PCM */
+			max_channels = max(max_channels, (sad[0] & 7) + 1u);
+			rate_mask |= sad[1];
+			fmt |= sad[2] & 0x07;
+			break;
+		}
+		sad += 3;
+	}
+
+	priv->max_channels = max_channels;
+	priv->rate_mask = rate_mask;
+	priv->fmt = fmt;
+}
+
 static int
 tda998x_encoder_get_modes(struct tda998x_priv *priv,
 			  struct drm_connector *connector)
@@ -1139,6 +1329,12 @@ tda998x_encoder_get_modes(struct tda998x_priv *priv,
 		drm_mode_connector_update_edid_property(connector, edid);
 		n = drm_add_edid_modes(connector, edid);
 		priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid);
+
+		/* set the audio parameters from the EDID */
+		if (priv->is_hdmi_sink) {
+			drm_edid_to_eld(connector, edid);
+			tda998x_set_audio(priv, connector);
+		}
 		kfree(edid);
 	}
 
@@ -1173,6 +1369,8 @@ static void tda998x_destroy(struct tda998x_priv *priv)
 	if (priv->hdmi->irq)
 		free_irq(priv->hdmi->irq, priv);
 
+	if (priv->pdev_codec)
+		platform_device_unregister(priv->pdev_codec);
 	i2c_unregister_device(priv->cec);
 }
 
@@ -1254,12 +1452,16 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 {
 	struct device_node *np = client->dev.of_node;
 	u32 video;
-	int rev_lo, rev_hi, ret;
+	int i, j, rev_lo, rev_hi, ret;
+	const char *p;
 
 	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3);
 	priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1);
 	priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
 
+	priv->params.audio_frame[1] = 1;		/* channels - 1 */
+	priv->params.audio_sample_rate = 48000;		/* 48kHz */
+
 	priv->current_page = 0xff;
 	priv->hdmi = client;
 	priv->cec = i2c_new_dummy(client->adapter, 0x34);
@@ -1351,17 +1553,48 @@ 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 device tree parameters */
+	if (np) {
+
+		/* 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;
+		}
+
+		/* audio properties */
+		for (i = 0; i < 2; i++) {
+			u32 port;
 
-	/* 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;
+			ret = of_property_read_u32_index(np, "audio-ports", i, &port);
+			if (ret)
+				break;
+			ret = of_property_read_string_index(np, "audio-port-names",
+							i, &p);
+			if (ret) {
+				dev_err(&client->dev,
+					"missing audio-port-names[%d]\n", i);
+				break;
+			}
+			if (strcmp(p, "spdif") == 0) {
+				j = AFMT_SPDIF;
+			} else if (strcmp(p, "i2s") == 0) {
+				j = AFMT_I2S;
+			} else {
+				dev_err(&client->dev,
+					"bad audio-port-names '%s'\n", p);
+				break;
+			}
+			priv->audio_ports[j] = port;
+		}
 	}
 
+	/* create the audio CODEC */
+	if (priv->audio_ports[AFMT_SPDIF] || priv->audio_ports[AFMT_I2S])
+		tda998x_create_audio_codec(priv);
+
 	return 0;
 
 fail:
@@ -1395,6 +1628,9 @@ static int tda998x_encoder_init(struct i2c_client *client,
 	encoder_slave->slave_priv = priv;
 	encoder_slave->slave_funcs = &tda998x_encoder_slave_funcs;
 
+	/* set the drvdata pointer for CODEC calls */
+	dev_set_drvdata(&client->dev, priv);
+
 	return 0;
 }
 
@@ -1521,7 +1757,7 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
 	if (!priv)
 		return -ENOMEM;
 
-	dev_set_drvdata(dev, priv);
+	dev_set_drvdata(dev, &priv->base);
 
 	priv->base.encoder = &priv->encoder;
 	priv->connector.interlace_allowed = 1;
@@ -1571,7 +1807,9 @@ err_encoder:
 static void tda998x_unbind(struct device *dev, struct device *master,
 			   void *data)
 {
-	struct tda998x_priv2 *priv = dev_get_drvdata(dev);
+	struct tda998x_priv *priv_s = dev_get_drvdata(dev);
+	struct tda998x_priv2 *priv =
+			container_of(priv_s, struct tda998x_priv2, base);
 
 	drm_connector_cleanup(&priv->connector);
 	drm_encoder_cleanup(&priv->encoder);
-- 
2.1.1

Re: [PATCH v8 0/2] ASoC: tda998x: add a codec to the HDMI transmitter

[Mark Brown]

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

On Thu, Oct 23, 2014 at 10:32:49AM +0200, Jean-Francois Moine wrote:
> The NXP TDA998x HDMI transmitter may transmit audio to the HDMI link
> from 2 different sources, I2S and S/PDIF.

So, I'm not seeing *any* interest here from any other HDMI users.  This
is a continuing theme with HDMI patches and is really very concerning,
everyone appears to be working in their own bubbles coming up with their
own things and ignoring everyone else's work - what little review I'm
seeing seems to be happening only after I explicitly prompt it.  I'm
following up to Jean-Francois' patches here but this isn't specific to
them, it seems like a general thing with HDMI code.

This in turn makes me think there's some abstraction problems with
what's going on and we're going to have to go through yet more
refactorings to fix things up as we do manage to come up with better
abstractions.  What I'd really like to see as a bare minimum is some
visible conversation about what we're doing and sign that people are
at least keeping in mind generic solutions when working on HDMI code.
Some commentary on the similarities and differences between hardware
and which abstractions work with which devices would also be really
helpful in working out if we're going in the right direction.

Basically at the minute I'm worried that we may be making the problems
we've got here worse not better, I've not personally had the time to sit
down and study the hardware sufficiently to form a firm impression
myself which isn't helping.

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

Re: [PATCH v8 0/2] ASoC: tda998x: add a codec to the HDMI transmitter

[Jyri Sarha]

On 12/29/2014 06:52 PM, Mark Brown wrote:
> On Thu, Oct 23, 2014 at 10:32:49AM +0200, Jean-Francois Moine wrote:
>> The NXP TDA998x HDMI transmitter may transmit audio to the HDMI link
>> from 2 different sources, I2S and S/PDIF.
>
> So, I'm not seeing *any* interest here from any other HDMI users.  This
> is a continuing theme with HDMI patches and is really very concerning,
> everyone appears to be working in their own bubbles coming up with their
> own things and ignoring everyone else's work - what little review I'm
> seeing seems to be happening only after I explicitly prompt it.  I'm
> following up to Jean-Francois' patches here but this isn't specific to
> them, it seems like a general thing with HDMI code.
>
> This in turn makes me think there's some abstraction problems with
> what's going on and we're going to have to go through yet more
> refactorings to fix things up as we do manage to come up with better
> abstractions.  What I'd really like to see as a bare minimum is some
> visible conversation about what we're doing and sign that people are
> at least keeping in mind generic solutions when working on HDMI code.
> Some commentary on the similarities and differences between hardware
> and which abstractions work with which devices would also be really
> helpful in working out if we're going in the right direction.
>
> Basically at the minute I'm worried that we may be making the problems
> we've got here worse not better, I've not personally had the time to sit
> down and study the hardware sufficiently to form a firm impression
> myself which isn't helping.
>

I have not seen any significant new development since v7 of these 
patches. My comments for v6 were mostly[1] addressed and I can live with 
these changes, even develop this approach further if it gets merged.

However, as a general note I see a need for a generic ASoC hdmi codec 
abstraction and I don't think this is generic enough. More of the audio 
specific implementation and HDMI standard specific things should be 
pushed away from the hdmi encoder driver (tda998x in this case) to the 
generic ASoC side hdmi codec driver (or library).

I am currently working on something that tries to be a generic solution 
that should be usable for different encoders and platforms, but 
unfortunately I have been busy with other things lately and have not 
been able to put anything working together yet. As soon as I have at 
least a bare bone API and and a proof of concept implementation working, 
I send RFC/POF patches out.

Best regards,
Jyri

[1] I personally do not like the hdmi_get_cdev() approach. I would 
rather go with only a library for registering from ASoC codec component 
under the HDMI encoder device or a completely separate device with only 
a reference to the HDMI encoder.

Re: [PATCH v8 0/2] ASoC: tda998x: add a codec to the HDMI transmitter

[Mark Brown]

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

On Mon, Dec 29, 2014 at 07:37:21PM +0200, Jyri Sarha wrote:
> On 12/29/2014 06:52 PM, Mark Brown wrote:

> >So, I'm not seeing *any* interest here from any other HDMI users.  This
> >is a continuing theme with HDMI patches and is really very concerning,
> >everyone appears to be working in their own bubbles coming up with their
> >own things and ignoring everyone else's work - what little review I'm

> I have not seen any significant new development since v7 of these patches.
> My comments for v6 were mostly[1] addressed and I can live with these
> changes, even develop this approach further if it gets merged.

OK, so this sort of feedback is really useful - even a qualified
Reviwed-by is useful.  Total silence could mean anything.

> However, as a general note I see a need for a generic ASoC hdmi codec
> abstraction and I don't think this is generic enough. More of the audio
> specific implementation and HDMI standard specific things should be pushed
> away from the hdmi encoder driver (tda998x in this case) to the generic ASoC
> side hdmi codec driver (or library).

This is something I'm expecting, yes - what I don't have is a clear
enough picture of how consistent the different hardware is in how it
models these things.

> [1] I personally do not like the hdmi_get_cdev() approach. I would rather go
> with only a library for registering from ASoC codec component under the HDMI
> encoder device or a completely separate device with only a reference to the
> HDMI encoder.

It does seem somewhat complicated, yes.  I don't know if matches idioms
for DRM somehow?

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

Re: [PATCH v8 0/2] ASoC: tda998x: add a codec to the HDMI transmitter

[Jyri Sarha]

On 12/30/2014 07:00 PM, Mark Brown wrote:
> On Mon, Dec 29, 2014 at 07:37:21PM +0200, Jyri Sarha wrote:
>> On 12/29/2014 06:52 PM, Mark Brown wrote:
>
>>> So, I'm not seeing *any* interest here from any other HDMI users.  This
>>> is a continuing theme with HDMI patches and is really very concerning,
>>> everyone appears to be working in their own bubbles coming up with their
>>> own things and ignoring everyone else's work - what little review I'm
>
>> I have not seen any significant new development since v7 of these patches.
>> My comments for v6 were mostly[1] addressed and I can live with these
>> changes, even develop this approach further if it gets merged.
>
> OK, so this sort of feedback is really useful - even a qualified
> Reviwed-by is useful.  Total silence could mean anything.
>
>> However, as a general note I see a need for a generic ASoC hdmi codec
>> abstraction and I don't think this is generic enough. More of the audio
>> specific implementation and HDMI standard specific things should be pushed
>> away from the hdmi encoder driver (tda998x in this case) to the generic ASoC
>> side hdmi codec driver (or library).
>
> This is something I'm expecting, yes - what I don't have is a clear
> enough picture of how consistent the different hardware is in how it
> models these things.
>

After taking another look the patchesthere is not much functionality 
that could be moved away from tda998x. It is just that the ASoC side 
(and tda998x) could do more in terms of HDMI standard, but that is fine 
because all that stuff can be added later if/when needed. However, 
instantiating snd_soc_dai_driver etc. in DRM side makes a nasty 
dependency between DRM and ASoC. All that stuff could be in ASoC side 
and simple enum/bitfield parameters defining i2s/spdif selection could 
be passed between DRM and ASoC side.


>> [1] I personally do not like the hdmi_get_cdev() approach. I would rather go
>> with only a library for registering from ASoC codec component under the HDMI
>> encoder device or a completely separate device with only a reference to the
>> HDMI encoder.
>
> It does seem somewhat complicated, yes.  I don't know if matches idioms
> for DRM somehow?
>

Me neither. A more straight forward option would be adding an "audio 
header" to the beginning of hdmi encoders drvdata. However, it would be 
tempting to add a pointer for drvdata to snd_soc_dai_driver to add audio 
specific driver data to any device that provides a DAI in addition to 
other functionality. It could even simplify drivers for some audio only 
devices with multiple DAIs.

Best regards,
Jyri