Discussion:
[PATCH v6 0/2] ASoC: tda998x: add a codec to the HDMI transmitter
Jean-Francois Moine
2014-09-24 08:23:34 UTC
Permalink
The NXP TDA998x HDMI transmitter may transmit audio to the HDMI link
from 2 different sources, I2S and S/PDIF.

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

The interface is used by the TDA998x driver to update the audio
constraints from the display characteristics (EDID) and by the audio
subsystem to connect the chosen audio source to the HDMI link.

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:codecs: Add a transmitter interface to the HDMI CODEC
drm/i2c:tda998x: Use the HDMI audio CODEC

.../devicetree/bindings/drm/i2c/tda998x.txt | 18 ++
drivers/gpu/drm/i2c/Kconfig | 1 +
drivers/gpu/drm/i2c/tda998x_drv.c | 299 +++++++++++++++++++--
include/sound/hdmi.h | 20 ++
sound/soc/codecs/hdmi.c | 174 +++++++++++-
5 files changed, 488 insertions(+), 24 deletions(-)
create mode 100644 include/sound/hdmi.h
--
2.1.1
Jean-Francois Moine
2014-09-24 07:49:25 UTC
Permalink
The audio constraints of the HDMI interface are defined by the EDID
which is sent by the connected device.

The HDMI transmitters may have one or many audio sources.

This patch adds two functions to the HDMI CODEC:
- it updates the audio constraints from the EDID,
- it gives the audio source type to the HDMI transmitter
on start of audio streaming.

Signed-off-by: Jean-Francois Moine <***@free.fr>
---
include/sound/hdmi.h | 20 ++++++
sound/soc/codecs/hdmi.c | 174 ++++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 188 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 1087fd5..6ea2772 100644
--- a/sound/soc/codecs/hdmi.c
+++ b/sound/soc/codecs/hdmi.c
@@ -22,9 +22,146 @@
#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;
+ 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"),
@@ -77,13 +214,38 @@ static struct snd_soc_codec_driver hdmi_codec = {
.num_dapm_routes = ARRAY_SIZE(hdmi_routes),
};

-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;
@@ -96,8 +258,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
Andrew Jackson
2014-09-29 07:26:13 UTC
Permalink
Post by Jean-Francois Moine
The audio constraints of the HDMI interface are defined by the EDID
which is sent by the connected device.
The HDMI transmitters may have one or many audio sources.
- it updates the audio constraints from the EDID,
- it gives the audio source type to the HDMI transmitter
on start of audio streaming.
---
include/sound/hdmi.h | 20 ++++++
sound/soc/codecs/hdmi.c | 174 ++++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 188 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 1087fd5..6ea2772 100644
--- a/sound/soc/codecs/hdmi.c
+++ b/sound/soc/codecs/hdmi.c
@@ -22,9 +22,146 @@
#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);
It might be a good idea to qualify priv (and perhaps get_audio and audio_switch) here? This might be that the relevant (function) pointers are non-NULL.

Andrew
Post by Jean-Francois Moine
+
+ /* 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;
+ 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"),
@@ -77,13 +214,38 @@ static struct snd_soc_codec_driver hdmi_codec = {
.num_dapm_routes = ARRAY_SIZE(hdmi_routes),
};
-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;
@@ -96,8 +258,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);
Jyri Sarha
2014-10-01 14:04:12 UTC
Permalink
On 09/24/2014 10:49 AM, Jean-Francois Moine wrote:> The audio
constraints of the HDMI interface are defined by the EDID
Post by Jean-Francois Moine
which is sent by the connected device.
The HDMI transmitters may have one or many audio sources.
- it updates the audio constraints from the EDID,
- it gives the audio source type to the HDMI transmitter
on start of audio streaming.
---
include/sound/hdmi.h | 20 ++++++
sound/soc/codecs/hdmi.c | 174
++++++++++++++++++++++++++++++++++++++++++++++--
Post by Jean-Francois Moine
2 files changed, 188 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);
This looks good for the api to HDMI ASoC codec.
Post by Jean-Francois Moine
+ void (*audio_switch)(struct device *dev,
+ int port_index,
+ unsigned sample_rate,
+ int sample_format);
I see nothing about selecting the i2s format. I do not know too much
about tda998x registers but sii9022 can select left and right
justified mode, bit-clock and frame-clock polarity etc. Should we need
a callback that corresponds to snd_soc_dai_ops .set_fmt ?

SiI9022 also uses an external clock as some kind of reference for audio
and that needs to be divided to right ballpark in relation sample-rate
(I have no idea why this is, I am just talking about
experience). Should we need callback that corresponds to
snd_soc_dai_ops .set_sysclk?

Maybe the above two could also be left for later.
Post by Jean-Francois Moine
+ int ndais;
+ struct snd_soc_dai_driver *dais;
+ struct snd_soc_codec_driver *driver;
+};
The need to include sound/soc.h is something I was trying to avoid in
my early design for a similar generic HDMI ASoC codec. I came up with
this requirement from Mark's comments about my OMAP HDMI audio
patches. So the bellow suggestion are really up to Mark.

If I understand right, the need for ndais, dais, and driver is coming
solely from spdif/i2s switching change. Since at least I am not aware
of any other DAIs found in HDMI encoders, could we just give the info
whether i2s and/or spdif is supporter with a simple bit-field or enum?
The snd_soc_dai_driver and snd_soc_codec_driver definitions could then
be all put to ASoC side of source tree.

I have also been wondering why we do not have SND_SOC_DAIFMT_SPDIF
definition in sound/soc-dai.h. With that there would be no need
to define two different dais for spdif and i2s. But this is more a
suggestion for ASoC core future development. Because I do not currently
have any HW to experiment with this the suggestion should probably be
left only as a side note for now.
Post by Jean-Francois Moine
+#endif
diff --git a/sound/soc/codecs/hdmi.c b/sound/soc/codecs/hdmi.c
index 1087fd5..6ea2772 100644
--- a/sound/soc/codecs/hdmi.c
+++ b/sound/soc/codecs/hdmi.c
@@ -22,9 +22,146 @@
#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;
I think we should add here:
formats |= SNDRV_PCM_FMTBIT_S24_3LE;
and probably also
formats |= SNDRV_PCM_FMTBIT_S32_LE;

The S24_3LE is equal to S24_LE at i2s level and the 32bit format would
help to get better than 16bit audio working with if there is trouble
supporting 24bit formats (like I have with wip sii9022 code). Both
tda998x and sii9022 are able to play 32bit i2s audio just fine. The 8
least significant bits are naturally ignored, but we can set
.sig_bits = 24 in snd_soc_pcm_stream.
Post by Jean-Francois Moine
+ 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"),
@@ -77,13 +214,38 @@ static struct snd_soc_codec_driver hdmi_codec = {
.num_dapm_routes = ARRAY_SIZE(hdmi_routes),
};
-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);
Registering the codec under the tda998x driver makes me wonder why
would we need the a separate platform device in the first palce. At
least it makes it possible to unify the old dummy HDMI codec with the
new code, but I am not even sure if that makes eny sense.

Maybe we should have another dummy codec to be used in the situations
where you really just need a dummy endpoint to satisfy ASoC. AFAIK the
only setups that use the current hdmi codec are BBB HDMI audio and
OMAP4+ HDMI audio, which both are currently broken in the upstream.
Post by Jean-Francois Moine
+ 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;
@@ -96,8 +258,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);
What comes to compatibility to the old use of hdmi codec
everything appears to be fine after this patch.

Best regards,
Jyri
Jean-Francois Moine
2014-10-01 16:45:17 UTC
Permalink
On Wed, 1 Oct 2014 17:04:12 +0300
=20
On 09/24/2014 10:49 AM, Jean-Francois Moine wrote:> The audio=20
constraints of the HDMI interface are defined by the EDID
Post by Jean-Francois Moine
which is sent by the connected device.
The HDMI transmitters may have one or many audio sources.
- it updates the audio constraints from the EDID,
- it gives the audio source type to the HDMI transmitter
on start of audio streaming.
---
include/sound/hdmi.h | 20 ++++++
sound/soc/codecs/hdmi.c | 174=20
++++++++++++++++++++++++++++++++++++++++++++++--
Post by Jean-Francois Moine
2 files changed, 188 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);
=20
This looks good for the api to HDMI ASoC codec.
=20
Post by Jean-Francois Moine
+ void (*audio_switch)(struct device *dev,
+ int port_index,
+ unsigned sample_rate,
+ int sample_format);
=20
I see nothing about selecting the i2s format. I do not know too much
about tda998x registers but sii9022 can select left and right
justified mode, bit-clock and frame-clock polarity etc. Should we nee=
d
a callback that corresponds to snd_soc_dai_ops .set_fmt ?
=20
SiI9022 also uses an external clock as some kind of reference for aud=
io
and that needs to be divided to right ballpark in relation sample-rat=
e
(I have no idea why this is, I am just talking about
experience). Should we need callback that corresponds to
snd_soc_dai_ops .set_sysclk?
=20
Maybe the above two could also be left for later.
The HDMI CODEC does not interact with the audio controller.
The audio_switch function just tells the tda998x which is the audio
source and it gives the parameters which impact the HDMI transmission.
The modes, clock... must be defined in the audio card definition
(simple-card or specific card).

About the AIF, this one is sent only once in the tda998x driver.
The values 'refer to stream header' should work in most cases.

The audio source is offered to the HDMI CODEC by the DAIs.
There are 2 DAIs: the first one is S/PDIF and the second is I2S.
If your audio controller delivers I2S only, the audio card shall define
only this one. With a DT (simple-card), this is:

sound {
compatible =3D "simple-audio-card";
...
simple-audio-card,dai-link { /* I2S - HDMI */
cpu {
sound-dai =3D <&audio 0>;
};
codec {
sound-dai =3D <&hdmi 1>; /* tda998x 2nd DAI */
};
};
};

Without DT, the DAI link is defined the same way:

.codec_name =3D "tda998x.0-0070";
.codec_dai_name =3D "i2s-hifi";
Post by Jean-Francois Moine
+ int ndais;
+ struct snd_soc_dai_driver *dais;
+ struct snd_soc_codec_driver *driver;
+};
=20
The need to include sound/soc.h is something I was trying to avoid in
my early design for a similar generic HDMI ASoC codec. I came up with
this requirement from Mark's comments about my OMAP HDMI audio
patches. So the bellow suggestion are really up to Mark.
=20
If I understand right, the need for ndais, dais, and driver is coming
solely from spdif/i2s switching change. Since at least I am not aware
of any other DAIs found in HDMI encoders, could we just give the info
whether i2s and/or spdif is supporter with a simple bit-field or enum=
?
The snd_soc_dai_driver and snd_soc_codec_driver definitions could the=
n
be all put to ASoC side of source tree.
=20
I have also been wondering why we do not have SND_SOC_DAIFMT_SPDIF
definition in sound/soc-dai.h. With that there would be no need
to define two different dais for spdif and i2s. But this is more a
suggestion for ASoC core future development. Because I do not current=
ly
have any HW to experiment with this the suggestion should probably be
left only as a side note for now.
The kirkwood audio subsystem which is used in the Cubox also has 2
audio outputs, I2S and S/PDIF. The audio route 'I2S --> HDMI I2S' or
'S/PDIF --> HDMI S/PDIF + optical S/PDIF' is chosen by the application
(PCM).

Then, the audio constraints are not the same on the audio controller
and HDMI transmitter sides. There must be a full description of these
constraints in the HDMI CODEC. I was thinking to build the DAIs from a
reduced description in the TDA998x transmitter, but, as all parameters
are required, it was simpler to include sound/soc.h.
Post by Jean-Francois Moine
+#endif
diff --git a/sound/soc/codecs/hdmi.c b/sound/soc/codecs/hdmi.c
index 1087fd5..6ea2772 100644
--- a/sound/soc/codecs/hdmi.c
+++ b/sound/soc/codecs/hdmi.c
@@ -22,9 +22,146 @@
#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 =3D 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 =3D 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[] =3D {
+ 32000, 44100, 48000, 88200, 96000, 176400, 192000
+ };
+
+ cdev =3D hdmi_get_cdev(dai->dev);
+ if (!cdev)
+ return -ENODEV;
+ priv =3D dev_get_drvdata(cdev);
+
+ /* get the EDID values and the rate constraints buffer */
+ ret =3D 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 =3D &priv->rate_constraints;
+ rate_constraints->list =3D hdmi_rates;
+ rate_constraints->count =3D ARRAY_SIZE(hdmi_rates);
+ rate_constraints->mask =3D rate_mask;
+ snd_pcm_hw_constraint_list(runtime, 0,
+ SNDRV_PCM_HW_PARAM_RATE,
+ rate_constraints);
+
+ formats =3D 0;
+ if (fmt & 1)
+ formats |=3D SNDRV_PCM_FMTBIT_S16_LE;
+ if (fmt & 2)
+ formats |=3D SNDRV_PCM_FMTBIT_S20_3LE;
+ if (fmt & 4)
+ formats |=3D SNDRV_PCM_FMTBIT_S24_LE;
=20
formats |=3D SNDRV_PCM_FMTBIT_S24_3LE;
and probably also
formats |=3D SNDRV_PCM_FMTBIT_S32_LE;
=20
The S24_3LE is equal to S24_LE at i2s level and the 32bit format woul=
d
help to get better than 16bit audio working with if there is trouble
supporting 24bit formats (like I have with wip sii9022 code). Both
tda998x and sii9022 are able to play 32bit i2s audio just fine. The 8
least significant bits are naturally ignored, but we can set
.sig_bits =3D 24 in snd_soc_pcm_stream.
OK.
Post by Jean-Francois Moine
+ 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 =3D hdmi_get_cdev(dai->dev);
+ if (!cdev)
+ return -ENODEV;
+ priv =3D 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 =3D hdmi_get_cdev(dai->dev);
+ if (!cdev)
+ return;
+ priv =3D dev_get_drvdata(cdev);
+
+ priv->hdmi_data.audio_switch(dai->dev, -1, 0, 0); /* stop */
+}
+
+static const struct snd_soc_dai_ops hdmi_ops =3D {
+ .startup =3D hdmi_startup,
+ .hw_params =3D hdmi_hw_params,
+ .shutdown =3D hdmi_shutdown,
+};
+
+static int hdmi_codec_probe(struct snd_soc_codec *codec)
+{
+ struct hdmi_priv *priv;
+ struct device *dev =3D codec->dev; /* encoder device */
+ struct device *cdev; /* codec device */
+
+ cdev =3D hdmi_get_cdev(dev);
+ if (!cdev)
+ return -ENODEV;
+
+ /* allocate some memory to store
+ * the encoder callback functions and the rate constraints */
+ priv =3D 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[] =3D {
SND_SOC_DAPM_INPUT("RX"),
SND_SOC_DAPM_OUTPUT("TX"),
@@ -77,13 +214,38 @@ static struct snd_soc_codec_driver hdmi_codec=
=3D {
Post by Jean-Francois Moine
.num_dapm_routes =3D ARRAY_SIZE(hdmi_routes),
};
-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 =3D 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 =3D devm_kmemdup(&pdev->dev,
+ pdata->dais,
+ sizeof *pdata->dais * pdata->ndais,
+ GFP_KERNEL);
+ for (i =3D 0; i < pdata->ndais; i++)
+ dais[i].ops =3D &hdmi_ops;
+
+ driver =3D devm_kmemdup(&pdev->dev,
+ pdata->driver,
+ sizeof *pdata->driver,
+ GFP_KERNEL);
+ driver->probe =3D hdmi_codec_probe;
+
+ /* register the codec on the video encoder */
+ ret =3D snd_soc_register_codec(pdev->dev.parent, driver,
+ dais, pdata->ndais);
=20
Registering the codec under the tda998x driver makes me wonder why
would we need the a separate platform device in the first palce. At
least it makes it possible to unify the old dummy HDMI codec with the
new code, but I am not even sure if that makes eny sense.
The CODEC platform device is needed for 2 reasons:
- it permits the HDMI transmitter to give the callback functions and th=
e
DAIs to the CODEC,
- the CODEC needs to memorize the callback functions and the rate
constraints, but, as a CODEC, it has no private data, and, on the
other side, the drvdata of the HDMI transmitter is already used.
Maybe we should have another dummy codec to be used in the situations
where you really just need a dummy endpoint to satisfy ASoC. AFAIK th=
e
only setups that use the current hdmi codec are BBB HDMI audio and
OMAP4+ HDMI audio, which both are currently broken in the upstream.
My patch does not break the dummy codec as soon as the device does not
get a platform_data at probe time.
Post by Jean-Francois Moine
+ 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;
@@ -96,8 +258,8 @@ static struct platform_driver hdmi_codec_driver=
=3D {
Post by Jean-Francois Moine
.of_match_table =3D of_match_ptr(hdmi_audio_codec_ids),
},
- .probe =3D hdmi_codec_probe,
- .remove =3D hdmi_codec_remove,
+ .probe =3D hdmi_codec_dev_probe,
+ .remove =3D hdmi_codec_dev_remove,
};
module_platform_driver(hdmi_codec_driver);
=20
What comes to compatibility to the old use of hdmi codec
everything appears to be fine after this patch.
--=20
Ken ar c'henta=C3=B1 | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" i=
n
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Jean-Francois Moine
2014-09-24 08:11:21 UTC
Permalink
This patch interfaces the HDMI transmitter with the audio system.

Signed-off-by: Jean-Francois Moine <***@free.fr>
---
.../devicetree/bindings/drm/i2c/tda998x.txt | 18 ++
drivers/gpu/drm/i2c/Kconfig | 1 +
drivers/gpu/drm/i2c/tda998x_drv.c | 299 +++++++++++++++++++--
3 files changed, 300 insertions(+), 18 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..66c41c0 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,22 @@ 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;
+};
+
+struct tda998x_priv2 {
+ struct tda998x_priv base;
+ struct drm_encoder encoder;
+ struct drm_connector connector;
};

#define to_tda998x_priv(x) ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv)
@@ -624,6 +642,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 +659,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,13 +672,29 @@ 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:
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);
+
+ /* with I2S input, the CTS_N predivider depends on
+ * the sample width */
+ switch (priv->audio_sample_format) {
+ case SNDRV_PCM_FORMAT_S16_LE:
+ cts_n = CTS_N_M(3) | CTS_N_K(1);
+ break;
+ case SNDRV_PCM_FORMAT_S24_LE:
+ cts_n = CTS_N_M(3) | CTS_N_K(2);
+ break;
+ default:
+ case SNDRV_PCM_FORMAT_S32_LE:
+ cts_n = CTS_N_M(3) | CTS_N_K(3);
+ break;
+ }
+ aclk = 1; /* clock enable */
break;

default:
@@ -671,6 +706,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 +763,144 @@ 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_priv2 *priv2 = dev_get_drvdata(dev);
+ struct tda998x_priv *priv = &priv2->base;
+
+ 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_priv2 *priv2 = dev_get_drvdata(dev);
+ struct tda998x_priv *priv = &priv2->base;
+ 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;
+ struct module *module;
+
+ request_module("snd-soc-hdmi-codec");
+ 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;
+ module = pdev->dev.driver->owner;
+ if (module)
+ try_module_get(module);
+}
+
/* DRM encoder functions */

static void tda998x_encoder_set_config(struct tda998x_priv *priv,
@@ -746,6 +920,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 +1304,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 +1356,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);
}

@@ -1167,12 +1390,19 @@ tda998x_encoder_set_property(struct drm_encoder *encoder,

static void tda998x_destroy(struct tda998x_priv *priv)
{
+ struct module *module;
+
/* disable all IRQs and free the IRQ handler */
cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
if (priv->hdmi->irq)
free_irq(priv->hdmi->irq, priv);

+ if (priv->pdev_codec) {
+ module = priv->pdev_codec->dev.driver->owner;
+ module_put(module);
+ platform_device_del(priv->pdev_codec);
+ }
i2c_unregister_device(priv->cec);
}

@@ -1254,12 +1484,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 +1585,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) {

- /* 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;
+ /* 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;
+
+ 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,15 +1660,13 @@ 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 to priv2 for CODEC calls */
+ dev_set_drvdata(&client->dev,
+ container_of(priv, struct tda998x_priv2, base));
+
return 0;
}

-struct tda998x_priv2 {
- struct tda998x_priv base;
- struct drm_encoder encoder;
- struct drm_connector connector;
-};
-
#define conn_to_tda998x_priv2(x) \
container_of(x, struct tda998x_priv2, connector);
--
2.1.1
Mark Brown
2014-09-30 19:25:40 UTC
Permalink
Post by Jean-Francois Moine
This patch interfaces the HDMI transmitter with the audio system.
Please use subject lines corresponding to the style for the subsystem.
Post by Jean-Francois Moine
+ request_module("snd-soc-hdmi-codec");
+ pdev = platform_device_register_resndata(&priv->hdmi->dev,
+ "hdmi-audio-codec",
+ PLATFORM_DEVID_NONE,
+ NULL, 0,
+ &tda998x_hdmi_data,
+ sizeof tda998x_hdmi_data);
Why is this request_module() needed? If there is a good reason for it
we should have some documentation.
Jean-Francois Moine
2014-10-01 09:28:46 UTC
Permalink
On Tue, 30 Sep 2014 20:25:40 +0100
Post by Jean-Francois Moine
+ request_module("snd-soc-hdmi-codec");
+ pdev =3D platform_device_register_resndata(&priv->hdmi->dev,
+ "hdmi-audio-codec",
+ PLATFORM_DEVID_NONE,
+ NULL, 0,
+ &tda998x_hdmi_data,
+ sizeof tda998x_hdmi_data); =20
=20
Why is this request_module() needed? If there is a good reason for i=
t
we should have some documentation.
The reason is simple: as the HDMI CODEC is not declared in the DT, the
associated module must be loaded in memory.

--=20
Ken ar c'henta=C3=B1 | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
Mark Brown
2014-10-01 12:37:08 UTC
Permalink
Post by Jean-Francois Moine
Post by Mark Brown
Post by Jean-Francois Moine
+ request_module("snd-soc-hdmi-codec");
+ pdev = platform_device_register_resndata(&priv->hdmi->dev,
+ "hdmi-audio-codec",
+ PLATFORM_DEVID_NONE,
+ NULL, 0,
+ &tda998x_hdmi_data,
+ sizeof tda998x_hdmi_data);
Why is this request_module() needed? If there is a good reason for it
we should have some documentation.
The reason is simple: as the HDMI CODEC is not declared in the DT, the
associated module must be loaded in memory.
No, that doesn't make sense - there's absolutely no dependency on DT for
module loading, non-DT boards work perfectly fine. Think about this for
a minute, we don't have lots of request_module() calls in board files.
You're working around some other bug here.
Russell King - ARM Linux
2014-10-01 13:47:30 UTC
Permalink
Post by Jean-Francois Moine
On Tue, 30 Sep 2014 20:25:40 +0100
Post by Mark Brown
Post by Jean-Francois Moine
+ request_module("snd-soc-hdmi-codec");
+ pdev = platform_device_register_resndata(&priv->hdmi->dev,
+ "hdmi-audio-codec",
+ PLATFORM_DEVID_NONE,
+ NULL, 0,
+ &tda998x_hdmi_data,
+ sizeof tda998x_hdmi_data);
Why is this request_module() needed? If there is a good reason for it
we should have some documentation.
The reason is simple: as the HDMI CODEC is not declared in the DT, the
associated module must be loaded in memory.
Module auto-loading works in non-DT environments too. Unlike the
direction that the arm port is going, the core kernel features, such
as driver autoloading, are coded *not* to require DT (or indeed any
particular firmware.)

There are circumstances where this has been lost sight of (such as
the gpiod stuff), but as a general rule, features do not rely on DT.

Platform drivers will be auto-loaded if they have:

MODULE_ALIAS(PLATFORM_MODULE_PREFIX DRIVER_NAME);

and DRIVER_NAME matches the non-id part of the platform device name.
In the case of platform driver ID tables:

MODULE_DEVICE_TABLE(platform, id-table);
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
Jyri Sarha
2014-10-01 14:05:32 UTC
Permalink
Post by Jean-Francois Moine
This patch interfaces the HDMI transmitter with the audio system.
---
.../devicetree/bindings/drm/i2c/tda998x.txt | 18 ++
drivers/gpu/drm/i2c/Kconfig | 1 +
drivers/gpu/drm/i2c/tda998x_drv.c | 299
+++++++++++++++++++--
Post by Jean-Francois Moine
3 files changed, 300 insertions(+), 18 deletions(-)
diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
Post by Jean-Francois Moine
index e9e4bce..e50e7cd 100644
--- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
+++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
- 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.
+
tda998x: hdmi-encoder {
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
Post by Jean-Francois Moine
index d476279..66c41c0 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,22 @@ 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;
+};
+
+struct tda998x_priv2 {
+ struct tda998x_priv base;
+ struct drm_encoder encoder;
+ struct drm_connector connector;
};
#define to_tda998x_priv(x) ((struct tda998x_priv
*)to_encoder_slave(x)->slave_priv)
Post by Jean-Francois Moine
@@ -624,6 +642,8 @@ tda998x_write_avi(struct tda998x_priv *priv,
struct drm_display_mode *mode)
Post by Jean-Francois Moine
sizeof(buf));
}
+/* audio functions */
+
static void tda998x_audio_mute(struct tda998x_priv *priv, bool on)
{
if (on) {
@@ -639,12 +659,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,13 +672,29 @@ 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;
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);
+
+ /* with I2S input, the CTS_N predivider depends on
+ * the sample width */
+ switch (priv->audio_sample_format) {
+ cts_n = CTS_N_M(3) | CTS_N_K(1);
+ break;
+ cts_n = CTS_N_M(3) | CTS_N_K(2);
+ break;
Setting the default here does not really help, because
priv->audio_sample_format is initialized to SNDRV_PCM_FORMAT_S24_LE in
tda998x_encoder_set_config(). But I am Ok with the default being
changed for 24 bit samples on i2s interface.
Post by Jean-Francois Moine
+ cts_n = CTS_N_M(3) | CTS_N_K(3);
+ break;
+ }
+ aclk = 1; /* clock enable */
break;
@@ -671,6 +706,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 +763,144 @@ 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_priv2 *priv2 = dev_get_drvdata(dev);
+ struct tda998x_priv *priv = &priv2->base;
+
+ 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_priv2 *priv2 = dev_get_drvdata(dev);
+ struct tda998x_priv *priv = &priv2->base;
+ 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;
+ struct module *module;
+
+ request_module("snd-soc-hdmi-codec");
+ 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;
+ module = pdev->dev.driver->owner;
+ if (module)
+ try_module_get(module);
+}
+
/* DRM encoder functions */
static void tda998x_encoder_set_config(struct tda998x_priv *priv,
@@ -746,6 +920,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;
See the previous comment.
Post by Jean-Francois Moine
}
static void tda998x_encoder_dpms(struct tda998x_priv *priv, int mode)
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 +1356,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);
}
@@ -1167,12 +1390,19 @@ tda998x_encoder_set_property(struct
drm_encoder *encoder,
Post by Jean-Francois Moine
static void tda998x_destroy(struct tda998x_priv *priv)
{
+ struct module *module;
+
/* disable all IRQs and free the IRQ handler */
cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
if (priv->hdmi->irq)
free_irq(priv->hdmi->irq, priv);
+ if (priv->pdev_codec) {
+ module = priv->pdev_codec->dev.driver->owner;
+ module_put(module);
+ platform_device_del(priv->pdev_codec);
+ }
i2c_unregister_device(priv->cec);
}
@@ -1254,12 +1484,16 @@ static int tda998x_create(struct i2c_client
*client, struct tda998x_priv *priv)
Post by Jean-Francois Moine
{
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 +1585,48 @@ static int tda998x_create(struct i2c_client
*client, struct tda998x_priv *priv)
Post by Jean-Francois Moine
/* 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) {
- /* 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;
+ /* 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;
+
+ 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;
@@ -1395,15 +1660,13 @@ static int tda998x_encoder_init(struct
i2c_client *client,
Post by Jean-Francois Moine
encoder_slave->slave_priv = priv;
encoder_slave->slave_funcs = &tda998x_encoder_slave_funcs;
+ /* set the drvdata pointer to priv2 for CODEC calls */
+ dev_set_drvdata(&client->dev,
+ container_of(priv, struct tda998x_priv2, base));
+
return 0;
}
-struct tda998x_priv2 {
- struct tda998x_priv base;
- struct drm_encoder encoder;
- struct drm_connector connector;
-};
-
#define conn_to_tda998x_priv2(x) \
container_of(x, struct tda998x_priv2, connector);
The only audio side change in the platform data usage of tda998x_drv I
can see is the change in the default value of CTS_N.

Best regards,
Jyri
Jean-Francois Moine
2014-10-02 18:37:10 UTC
Permalink
On Wed, 1 Oct 2014 17:05:32 +0300
Post by Jyri Sarha
Post by Jean-Francois Moine
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);
+
+ /* with I2S input, the CTS_N predivider depends on
+ * the sample width */
+ switch (priv->audio_sample_format) {
+ cts_n = CTS_N_M(3) | CTS_N_K(1);
+ break;
+ cts_n = CTS_N_M(3) | CTS_N_K(2);
+ break;
Setting the default here does not really help, because
priv->audio_sample_format is initialized to SNDRV_PCM_FORMAT_S24_LE in
tda998x_encoder_set_config(). But I am Ok with the default being
changed for 24 bit samples on i2s interface.
Post by Jean-Francois Moine
+ cts_n = CTS_N_M(3) | CTS_N_K(3);
+ break;
+ }
I looked again at the original driver and they set K = 1 for 16 bits
and K = 3 for 24 or 32 bits.

Anyway, letting K = 3 for 16 bits works for me, so, I will not change
this code in the next version.

Thanks.
--
Ken ar c'hentaƱ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
Russell King - ARM Linux
2014-10-01 14:23:41 UTC
Permalink
Post by Jean-Francois Moine
This patch interfaces the HDMI transmitter with the audio system.
...
Post by Jean-Francois Moine
+struct tda998x_priv2 {
+ struct tda998x_priv base;
+ struct drm_encoder encoder;
+ struct drm_connector connector;
};
NAK on moving this up here. It was specifically placed below the core
and below the older drm_slave_encoder code because nothing but the new
component based interfaces to TDA998x have any business knowing that
these are packaged up in this way.
Post by Jean-Francois Moine
+static void tda998x_create_audio_codec(struct tda998x_priv *priv)
+{
+ struct platform_device *pdev;
+ struct module *module;
+
+ request_module("snd-soc-hdmi-codec");
+ 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;
+ module = pdev->dev.driver->owner;
+ if (module)
+ try_module_get(module);
This is really not on. Firstly, registering a platform device has no
guarantee that it will bind to a driver. So, pdev->dev.driver may be
NULL here -> kernel oops.

Secondly, what's the purpose of the unchecked try_module_get() there?
You can't stop the device being unbound from its driver, so all you're
doing is possibly locking the module into module space for no other
benefit.
Post by Jean-Francois Moine
@@ -1167,12 +1390,19 @@ tda998x_encoder_set_property(struct drm_encoder *encoder,
static void tda998x_destroy(struct tda998x_priv *priv)
{
+ struct module *module;
+
/* disable all IRQs and free the IRQ handler */
cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
if (priv->hdmi->irq)
free_irq(priv->hdmi->irq, priv);
+ if (priv->pdev_codec) {
+ module = priv->pdev_codec->dev.driver->owner;
+ module_put(module);
This is wrong for all the reasons the other part above is wrong.
Userspace can decide to unbind the platform device from the associated
platform driver whenever it wishes. At that point,
priv->pdev_codec->dev.driver becomes NULL.

So, please get rid of this.
Post by Jean-Francois Moine
+ platform_device_del(priv->pdev_codec);
This leaks the memory associated with the platform device.
Post by Jean-Francois Moine
@@ -1395,15 +1660,13 @@ 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 to priv2 for CODEC calls */
+ dev_set_drvdata(&client->dev,
+ container_of(priv, struct tda998x_priv2, base));
+
return 0;
}
-struct tda998x_priv2 {
- struct tda998x_priv base;
- struct drm_encoder encoder;
- struct drm_connector connector;
-};
-
I would prefer this structure to stay here, as code above this point should
have no business knowing how these are packaged together. I would suggest
either:

- moving the audio codec code below this point, or
- storing struct tda998x_priv in the device private pointer, and
converting it to struct tda998x_priv2 via container_of() where
necessary below this point.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
Jean-Francois Moine
2014-10-02 17:59:51 UTC
Permalink
On Wed, 1 Oct 2014 15:23:41 +0100
Post by Russell King - ARM Linux
I would prefer this structure to stay here, as code above this point should
have no business knowing how these are packaged together. I would suggest
- moving the audio codec code below this point, or
- storing struct tda998x_priv in the device private pointer, and
converting it to struct tda998x_priv2 via container_of() where
necessary below this point.
The second option seems easier for use as a slave encoder.

Thanks for all your remarks. I will send an other version.
--
Ken ar c'hentaƱ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
Mark Brown
2014-09-30 19:10:30 UTC
Permalink
Post by 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 adds an interface between the HDMI transmitter and
the HDMI CODEC.
I'd really like to see some of the other people who are working with
HDMI reviewing this - this is a generic part which seems to be very
widely used so we need to make sure that what's being done works for
other systems. Jyri, I see you're on CC?
Loading...