Re: [PATCH v9 3/4] ASoC: tda998x: add a codec to the HDMI transmitter
From: Jyri Sarha
Date: Sun Jan 11 2015 - 16:03:49 EST
Some more comments based on my - finally successful - attempt to use
this code with BBB HDMI audio.
On 01/07/2015 12:51 PM, Jean-Francois Moine wrote:
This patch adds a CODEC to the NXP TDA998x HDMI transmitter.
The CODEC handles both I2S and S/PDIF inputs.
It maintains the audio format and rate constraints according
to the HDMI device parameters (EDID) and sets dynamically the input
ports in the TDA998x I2C driver on start/stop audio streaming.
Signed-off-by: Jean-Francois Moine <moinejf@xxxxxxx>
---
drivers/gpu/drm/i2c/Kconfig | 1 +
drivers/gpu/drm/i2c/tda998x_drv.c | 139 ++++++++++++++++++++++++++++--
include/sound/tda998x.h | 23 +++++
sound/soc/codecs/Kconfig | 4 +
sound/soc/codecs/Makefile | 2 +
sound/soc/codecs/tda998x.c | 175 ++++++++++++++++++++++++++++++++++++++
6 files changed, 339 insertions(+), 5 deletions(-)
create mode 100644 include/sound/tda998x.h
create mode 100644 sound/soc/codecs/tda998x.c
diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig
index 22c7ed6..24fc975 100644
--- a/drivers/gpu/drm/i2c/Kconfig
+++ b/drivers/gpu/drm/i2c/Kconfig
@@ -28,6 +28,7 @@ config DRM_I2C_SIL164
config DRM_I2C_NXP_TDA998X
tristate "NXP Semiconductors TDA998X HDMI encoder"
default m if DRM_TILCDC
+ select SND_SOC_TDA998X
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 ce1478d..a26a516 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -27,6 +27,11 @@
#include <drm/drm_encoder_slave.h>
#include <drm/drm_edid.h>
#include <drm/i2c/tda998x.h>
+#include <sound/tda998x.h>
+
+#if defined(CONFIG_SND_SOC_TDA998X) || defined(CONFIG_SND_SOC_TDA998X_MODULE)
+#define WITH_CODEC 1
+#endif
#define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
@@ -47,6 +52,10 @@ struct tda998x_priv {
struct drm_encoder *encoder;
u8 audio_ports[2];
+#ifdef WITH_CODEC
+ u8 sad[3]; /* Short Audio Descriptor */
+ struct snd_pcm_hw_constraint_list rate_constraints;
+#endif
It feels a bit backwards to me to store these audio side variables here
in DRM side driver - especially the rate_constraint - and provide a
function (tda998x_get_audio_var()) for getting their individual
addresses to ASoC side. Wouldn't it be more straight forward to provide
functions for setting and getting the audio side data from a void
pointer in DRM side device drvdata?
};
#define to_tda998x_priv(x) ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv)
@@ -730,11 +739,109 @@ tda998x_configure_audio(struct tda998x_priv *priv,
tda998x_write_aif(priv, p);
}
+#ifdef WITH_CODEC
+/* tda998x audio codec interface */
+
+/* return the audio parameters extracted from the last EDID */
+static int tda998x_get_audio_var(struct device *dev,
+ u8 **sad,
+ struct snd_pcm_hw_constraint_list **rate_constraints)
+{
+ struct tda998x_priv *priv = dev_get_drvdata(dev);
+
+ if (!priv->encoder->crtc
+ || !priv->is_hdmi_sink)
+ return -ENODEV;
+
+ *sad = priv->sad;
+ *rate_constraints = &priv->rate_constraints;
+ return 0;
+}
+
+/* switch the audio port and initialize the audio parameters for streaming */
+static int tda998x_set_audio_input(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 -ENODEV;
+
+ /* if no port, just disable the audio port */
+ if (port_index == PORT_NONE) {
+ reg_write(priv, REG_ENA_AP, 0);
+ return 0;
+ }
+
+ /* if same audio parameters, just enable the audio port */
+ if (p->audio_cfg == priv->audio_ports[port_index] &&
+ p->audio_sample_rate == sample_rate) {
+ reg_write(priv, REG_ENA_AP, p->audio_cfg);
+ return 0;
+ }
+
+ p->audio_format = port_index == PORT_SPDIF ? AFMT_SPDIF : AFMT_I2S;
+ p->audio_clk_cfg = port_index == PORT_SPDIF ? 0 : 1;
+ p->audio_cfg = priv->audio_ports[port_index];
+ p->audio_sample_rate = sample_rate;
+ tda998x_configure_audio(priv, &priv->encoder->crtc->hwmode, p);
+ return 0;
+}
+
+/* get the audio capabilities from the EDID */
+static void tda998x_get_audio_caps(struct tda998x_priv *priv,
+ struct drm_connector *connector)
+{
+ u8 *eld = connector->eld;
+ u8 *sad;
+ int sad_count;
+ unsigned eld_ver, mnl;
+ u8 max_channels, rate_mask, 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;
+SNDRV_PCM_RATE_CONTINUOUS
+ /* Start from the basic audio settings */
+ max_channels = 1;
+ rate_mask = 0;
+ fmt = 0;
+ while (sad_count--) {
+ switch (sad[0] & 0x78) {
+ case 0x08: /* SAD uncompressed audio */
+ if ((sad[0] & 7) > max_channels)
+ max_channels = sad[0] & 7;
+ rate_mask |= sad[1];
+ fmt |= sad[2] & 0x07;
+ break;
+ }
+ sad += 3;
+ }
+
+ priv->sad[0] = max_channels;
+ priv->sad[1] = rate_mask;
+ priv->sad[2] = fmt;
+}
+#endif /* WITH_CODEC */
+
/* DRM encoder functions */
static void tda998x_encoder_set_config(struct tda998x_priv *priv,
const struct tda998x_encoder_params *p)
{
+ int port_index;
+
priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) |
(p->mirr_a ? VIP_CNTRL_0_MIRR_A : 0) |
VIP_CNTRL_0_SWAP_B(p->swap_b) |
@@ -749,6 +856,8 @@ static void tda998x_encoder_set_config(struct tda998x_priv *priv,
(p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
priv->params = *p;
+ port_index = p->audio_format == AFMT_SPDIF ? PORT_SPDIF : PORT_I2S;
+ priv->audio_ports[port_index] = p->audio_cfg;
}
static void tda998x_encoder_dpms(struct tda998x_priv *priv, int mode)
@@ -1142,6 +1251,15 @@ 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);
+
+#ifdef WITH_CODEC
+ /* set the audio parameters from the EDID */
+ if (priv->is_hdmi_sink) {
+ drm_edid_to_eld(connector, edid);
+ tda998x_get_audio_caps(priv, connector);
+ }
+#endif
+
kfree(edid);
}
@@ -1176,6 +1294,10 @@ static void tda998x_destroy(struct tda998x_priv *priv)
if (priv->hdmi->irq)
free_irq(priv->hdmi->irq, priv);
+#ifdef WITH_CODEC
+ tda9998x_codec_unregister(&priv->hdmi->dev);
+#endif
+
i2c_unregister_device(priv->cec);
}
@@ -1384,26 +1506,33 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
break;
}
if (strcmp(p, "spdif") == 0) {
- priv->audio_ports[0] = port;
+ priv->audio_ports[PORT_SPDIF] = port;
} else if (strcmp(p, "i2s") == 0) {
- priv->audio_ports[1] = port;
+ priv->audio_ports[PORT_I2S] = port;
} else {
dev_err(&client->dev,
"bad audio-port-names '%s'\n", p);
break;
}
}
- if (priv->audio_ports[0]) {
- priv->params.audio_cfg = priv->audio_ports[0];
+ if (priv->audio_ports[PORT_SPDIF]) {
+ priv->params.audio_cfg = priv->audio_ports[PORT_SPDIF];
priv->params.audio_format = AFMT_SPDIF;
priv->params.audio_clk_cfg = 0;
} else {
- priv->params.audio_cfg = priv->audio_ports[1];
+ priv->params.audio_cfg = priv->audio_ports[PORT_I2S];
priv->params.audio_format = AFMT_I2S;
priv->params.audio_clk_cfg = 1;
}
}
+#ifdef WITH_CODEC
+ /* create the audio CODEC */
+ if (priv->audio_ports[PORT_SPDIF] || priv->audio_ports[PORT_I2S])
+ tda9998x_codec_register(&client->dev,
+ tda998x_get_audio_var,
+ tda998x_set_audio_input);
+#endif
return 0;
fail:
diff --git a/include/sound/tda998x.h b/include/sound/tda998x.h
new file mode 100644
index 0000000..97cff30
--- /dev/null
+++ b/include/sound/tda998x.h
@@ -0,0 +1,23 @@
+#ifndef SND_TDA998X_H
+#define SND_TDA998X_H
+
+#include <sound/pcm.h>
+
+/* port indexes */
+#define PORT_NONE (-1)
+#define PORT_SPDIF 0
+#define PORT_I2S 1
+
+typedef int (*get_audio_var_t)(struct device *dev,
+ u8 **sad,
+ struct snd_pcm_hw_constraint_list **rate_constraints);
+typedef int (*set_audio_input_t)(struct device *dev,
+ int port_index,
+ unsigned sample_rate,
+ int sample_format);
+
Why don't you simply declare tda998x_get_audio_var() and
tda998x_set_audio_input() here and export them in DRM side driver? This
is a tda998x specific codec after all. I see no point to this this
function pointer typedef game, when the pointers are anyway stored to
global variables in ASoC the side module.
+int tda9998x_codec_register(struct device *dev,
+ get_audio_var_t get_audio_var_i,
+ set_audio_input_t set_audio_input_i);
+void tda9998x_codec_unregister(struct device *dev);
+#endif
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 8349f98..456c549 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -102,6 +102,7 @@ config SND_SOC_ALL_CODECS
select SND_SOC_STAC9766 if SND_SOC_AC97_BUS
select SND_SOC_TAS2552 if I2C
select SND_SOC_TAS5086 if I2C
+ select SND_SOC_TDA998X if DRM_I2C_NXP_TDA998X
select SND_SOC_TFA9879 if I2C
select SND_SOC_TLV320AIC23_I2C if I2C
select SND_SOC_TLV320AIC23_SPI if SPI_MASTER
@@ -600,6 +601,9 @@ config SND_SOC_TAS5086
tristate "Texas Instruments TAS5086 speaker amplifier"
depends on I2C
+config SND_SOC_TDA998X
+ tristate
+
config SND_SOC_TFA9879
tristate "NXP Semiconductors TFA9879 amplifier"
depends on I2C
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index bbdfd1e..44b4fda 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -104,6 +104,7 @@ snd-soc-sta350-objs := sta350.o
snd-soc-sta529-objs := sta529.o
snd-soc-stac9766-objs := stac9766.o
snd-soc-tas5086-objs := tas5086.o
+snd-soc-tda998x-objs := tda998x.o
snd-soc-tfa9879-objs := tfa9879.o
snd-soc-tlv320aic23-objs := tlv320aic23.o
snd-soc-tlv320aic23-i2c-objs := tlv320aic23-i2c.o
@@ -282,6 +283,7 @@ obj-$(CONFIG_SND_SOC_STA529) += snd-soc-sta529.o
obj-$(CONFIG_SND_SOC_STAC9766) += snd-soc-stac9766.o
obj-$(CONFIG_SND_SOC_TAS2552) += snd-soc-tas2552.o
obj-$(CONFIG_SND_SOC_TAS5086) += snd-soc-tas5086.o
+obj-$(CONFIG_SND_SOC_TDA998X) += snd-soc-tda998x.o
obj-$(CONFIG_SND_SOC_TFA9879) += snd-soc-tfa9879.o
obj-$(CONFIG_SND_SOC_TLV320AIC23) += snd-soc-tlv320aic23.o
obj-$(CONFIG_SND_SOC_TLV320AIC23_I2C) += snd-soc-tlv320aic23-i2c.o
diff --git a/sound/soc/codecs/tda998x.c b/sound/soc/codecs/tda998x.c
new file mode 100644
index 0000000..b055570
--- /dev/null
+++ b/sound/soc/codecs/tda998x.c
@@ -0,0 +1,175 @@
+/*
+ * ALSA SoC TDA998x CODEC
+ *
+ * Copyright (C) 2015 Jean-Francois Moine
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <sound/soc.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <sound/pcm_params.h>
+#include <sound/tda998x.h>
+
+/* functions in tda998x_drv */
+static get_audio_var_t get_audio_var;
+static set_audio_input_t set_audio_input;
+
+static int tda998x_codec_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct snd_pcm_runtime *runtime = substream->runtime;
+ struct snd_pcm_hw_constraint_list *rate_constraints;
+ u8 *sad; /* Short Audio Descriptor (3 bytes) */
+#define SAD_MX_CHAN_I 0 /* max number of chennels - 1 */
+#define SAD_RATE_MASK_I 1 /* rate mask */
+#define SAD_FMT_I 2 /* formats */
+#define SAD_FMT_16 0x01
+#define SAD_FMT_20 0x02
+#define SAD_FMT_24 0x04
+ u64 formats;
+ int ret;
+ static const u32 hdmi_rates[] = {
+ 32000, 44100, 48000, 88200, 96000, 176400, 192000
+ };
+
+ /* get the EDID values and the rate constraints buffer */
+ ret = get_audio_var(dai->dev, &sad, &rate_constraints);
+ if (ret < 0)
+ return ret; /* no screen */
+
+ /* convert the EDID values to audio constraints */
+ rate_constraints->list = hdmi_rates;
+ rate_constraints->count = ARRAY_SIZE(hdmi_rates);
+ rate_constraints->mask = sad[SAD_RATE_MASK_I];
+ snd_pcm_hw_constraint_list(runtime, 0,
+ SNDRV_PCM_HW_PARAM_RATE,
+ rate_constraints);
+
+ formats = 0;
+ if (sad[SAD_FMT_I] & SAD_FMT_16)
+ formats |= SNDRV_PCM_FMTBIT_S16_LE;
+ if (sad[SAD_FMT_I] & SAD_FMT_20)
+ formats |= SNDRV_PCM_FMTBIT_S20_3LE;
+ if (sad[SAD_FMT_I] & SAD_FMT_24)
+ 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, sad[SAD_MX_CHAN_I]);
SAD byte 1 bits 2-0 provides the maximum channel count _minus_one_. You
should _add_one_ to sad[SAD_MX_CHAN_I] when setting the constraint.
+ return 0;
+}
+
+static int tda998x_codec_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params,
+ struct snd_soc_dai *dai)
+{
+ return set_audio_input(dai->dev, dai->id,
+ params_rate(params),
+ params_format(params));
+}
+
+static void tda998x_codec_shutdown(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ set_audio_input(dai->dev, PORT_NONE, 0, 0);
+}
+
+static const struct snd_soc_dai_ops tda998x_codec_ops = {
+ .startup = tda998x_codec_startup,
+ .hw_params = tda998x_codec_hw_params,
+ .shutdown = tda998x_codec_shutdown,
+};
+
+#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 = PORT_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,
+ },
+ .ops = &tda998x_codec_ops,
+ },
+ {
+ .name = "i2s-hifi",
+ .id = PORT_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,
+ },
+ .ops = &tda998x_codec_ops,
+ },
+};
Why do you use SNDRV_PCM_RATE_CONTINUOUS, when HDMI standard only
supports 32000, 44100, 48000, 88200, 96000, 176400, and 192000 Hz-rates?
+
+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_drv = {
+ .dapm_widgets = tda998x_widgets,
+ .num_dapm_widgets = ARRAY_SIZE(tda998x_widgets),
+ .dapm_routes = tda998x_routes,
+ .num_dapm_routes = ARRAY_SIZE(tda998x_routes),
+ .ignore_pmdown_time = true,
+};
+
+int tda9998x_codec_register(struct device *dev,
+ get_audio_var_t get_audio_var_i,
+ set_audio_input_t set_audio_input_i)
+{
+ get_audio_var = get_audio_var_i;
+ set_audio_input = set_audio_input_i;
+ return snd_soc_register_codec(dev,
+ &tda998x_codec_drv,
+ tda998x_dais, ARRAY_SIZE(tda998x_dais));
So this always registers a two DAI codec whether or not both the i2s and
spdif is used. The DT binding document could state that the DAI index 0
is always the spdif DAI and index 1 is the i2s DAI, or even better you
could #define them under include/dt-bindings/.
While you are at it, you could also mention the DAPM output name
"hdmi-out" for the codec in the DT-binding document.
+}
+EXPORT_SYMBOL(tda9998x_codec_register);
+
+void tda9998x_codec_unregister(struct device *dev)
+{
+ snd_soc_unregister_codec(dev);
+}
+EXPORT_SYMBOL(tda9998x_codec_unregister);
+
+static int __init tda998x_codec_init(void)
+{
+ return 0;
+}
+static void __exit tda998x_codec_exit(void)
+{
+}
+module_init(tda998x_codec_init);
+module_exit(tda998x_codec_exit);
+
+MODULE_AUTHOR("Jean-Francois Moine <moinejf@xxxxxxx>");
+MODULE_DESCRIPTION("TDA998X CODEC");
+MODULE_LICENSE("GPL");
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/