The TDA998x HDMI transmitter accepts audio input from either I2S or S/PDIF.
Theses inputs have different intrinsic constraints and these constraints
may be modified by the audio parameters of the connected video device.
The choice of I2S or S/PDIF may be the done by the user or by automatic
processing (DPCM?) at each audio starting time. This asks for a dynamic
audio input switch in the HDMI driver.
This patch series implements the TDA998x specific CODEC.
A simple function call mechanism is used for exchanges between the
CODEC and the HDMI driver.
Jean-Francois Moine (4):
drm/i2c: tda998x: add a function for dynamic audio input switch
ASoC: tda998x: add a codec driver for TDA998x
ASoC: tda998x: add DT documentation
ASoC: tda998x: adjust the audio hw parameters from EDID
.../devicetree/bindings/sound/tda998x.txt | 14 +
drivers/gpu/drm/i2c/tda998x_drv.c | 58 +++-
include/drm/i2c/tda998x.h | 8 +-
sound/soc/codecs/Kconfig | 7 +
sound/soc/codecs/Makefile | 2 +
sound/soc/codecs/tda998x.c | 304 +++++++++++++++++++++
6 files changed, 390 insertions(+), 3 deletions(-)
create mode 100644 Documentation/devicetree/bindings/sound/tda998x.txt
create mode 100644 sound/soc/codecs/tda998x.c
--
1.8.5.3
When both I2S and S/PDIF are wired from the audio device to the
TDA998x, the user or some internal mechanism may choose to do audio
streaming via either inputs.
This patch adds an exported function in the TDA998x driver which
initializes the audio input parameters according to the audio
subsystem.
The audio format values in the encoder configuration interface are
changed to non null values so that the value 0 is used in the audio
function to indicate that audio streaming is stopped.
Signed-off-by: Jean-Francois Moine <[email protected]>
---
drivers/gpu/drm/i2c/tda998x_drv.c | 43 ++++++++++++++++++++++++++++++++++++++-
include/drm/i2c/tda998x.h | 7 +++++--
2 files changed, 47 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index e5bbaf2..186c751 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -34,6 +34,7 @@ struct tda998x_priv {
struct i2c_client *hdmi;
uint16_t rev;
uint8_t current_page;
+ u8 audio_active;
int dpms;
bool is_hdmi_sink;
u8 vip_cntrl_0;
@@ -729,6 +730,38 @@ tda998x_configure_audio(struct tda998x_priv *priv,
tda998x_write_aif(priv, p);
}
+/* tda998x codec interface */
+void tda998x_audio_update(struct i2c_client *client,
+ int format,
+ int port)
+{
+ struct tda998x_priv *priv = i2c_get_clientdata(client);
+ struct tda998x_encoder_params *p = &priv->params;
+
+ /* if the audio output is active, it may be a second start or a stop */
+ if (format == 0 || priv->audio_active) {
+ if (format == 0) {
+ priv->audio_active = 0;
+ reg_write(priv, REG_ENA_AP, 0);
+ }
+ return;
+ }
+
+ p->audio_cfg = port;
+
+ /* don't restart audio if same input format */
+ if (format == p->audio_format) {
+ priv->audio_active = 1;
+ reg_write(priv, REG_ENA_AP, p->audio_cfg);
+ return;
+ }
+ p->audio_format = format;
+ priv->audio_active = 1;
+
+ tda998x_configure_audio(priv, &priv->encoder->crtc->hwmode, p);
+}
+EXPORT_SYMBOL_GPL(tda998x_audio_update);
+
/* DRM encoder functions */
static void
@@ -751,6 +784,9 @@ tda998x_encoder_set_config(struct drm_encoder *encoder, void *params)
(p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
priv->params = *p;
+
+ if (p->audio_cfg)
+ priv->audio_active = 1;
}
static void
@@ -1001,7 +1037,7 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
tda998x_write_avi(priv, adj_mode);
- if (priv->params.audio_cfg)
+ if (priv->audio_active)
tda998x_configure_audio(priv, adj_mode, &priv->params);
}
}
@@ -1233,10 +1269,15 @@ tda998x_encoder_init(struct i2c_client *client,
if (!priv)
return -ENOMEM;
+ i2c_set_clientdata(client, priv);
+
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);
diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
index f3bb25c..0459931 100644
--- a/include/drm/i2c/tda998x.h
+++ b/include/drm/i2c/tda998x.h
@@ -19,11 +19,14 @@ struct tda998x_encoder_params {
u8 audio_frame[6];
enum {
- AFMT_SPDIF,
- AFMT_I2S
+ AFMT_I2S = 1,
+ AFMT_SPDIF
} audio_format;
unsigned audio_sample_rate;
};
+void tda998x_audio_update(struct i2c_client *client,
+ int format,
+ int port);
#endif
--
1.8.5.3
This patch adds a CODEC driver for the NXP TDA998x HDMI transmitter.
The CODEC handles both I2S and S/PDIF input and does dynamic input
switch in the TDA998x I2C driver on audio streaming start/stop.
Signed-off-by: Jean-Francois Moine <[email protected]>
---
sound/soc/codecs/Kconfig | 7 ++
sound/soc/codecs/Makefile | 2 +
sound/soc/codecs/tda998x.c | 227 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 236 insertions(+)
create mode 100644 sound/soc/codecs/tda998x.c
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index b33b45d..7cec76e 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -71,6 +71,7 @@ config SND_SOC_ALL_CODECS
select SND_SOC_STA529 if I2C
select SND_SOC_STAC9766 if SND_SOC_AC97_BUS
select SND_SOC_TAS5086 if I2C
+ select SND_SOC_TDA998X if I2C
select SND_SOC_TLV320AIC23 if I2C
select SND_SOC_TLV320AIC26 if SPI_MASTER
select SND_SOC_TLV320AIC32X4 if I2C
@@ -352,6 +353,12 @@ config SND_SOC_STAC9766
config SND_SOC_TAS5086
tristate
+config SND_SOC_TDA998X
+ tristate
+ depends on OF
+ default y if DRM_I2C_NXP_TDA998X=y
+ default m if DRM_I2C_NXP_TDA998X=m
+
config SND_SOC_TLV320AIC23
tristate
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index bc12676..a53d09e 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -62,6 +62,7 @@ snd-soc-sta32x-objs := sta32x.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-tlv320aic23-objs := tlv320aic23.o
snd-soc-tlv320aic26-objs := tlv320aic26.o
snd-soc-tlv320aic3x-objs := tlv320aic3x.o
@@ -192,6 +193,7 @@ obj-$(CONFIG_SND_SOC_STA32X) += snd-soc-sta32x.o
obj-$(CONFIG_SND_SOC_STA529) += snd-soc-sta529.o
obj-$(CONFIG_SND_SOC_STAC9766) += snd-soc-stac9766.o
obj-$(CONFIG_SND_SOC_TAS5086) += snd-soc-tas5086.o
+obj-$(CONFIG_SND_SOC_TDA998X) += snd-soc-tda998x.o
obj-$(CONFIG_SND_SOC_TLV320AIC23) += snd-soc-tlv320aic23.o
obj-$(CONFIG_SND_SOC_TLV320AIC26) += snd-soc-tlv320aic26.o
obj-$(CONFIG_SND_SOC_TLV320AIC3X) += snd-soc-tlv320aic3x.o
diff --git a/sound/soc/codecs/tda998x.c b/sound/soc/codecs/tda998x.c
new file mode 100644
index 0000000..d724f7d
--- /dev/null
+++ b/sound/soc/codecs/tda998x.c
@@ -0,0 +1,227 @@
+/*
+ * ALSA SoC TDA998X driver
+ *
+ * This driver is used by the NXP TDA998x HDMI transmitter.
+ *
+ * Copyright (C) 2014 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 <sound/pcm.h>
+#include <linux/of.h>
+#include <linux/i2c.h>
+#include <drm/drm_encoder_slave.h>
+#include <drm/i2c/tda998x.h>
+
+#define TDA998X_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \
+ SNDRV_PCM_FMTBIT_S20_3LE | \
+ SNDRV_PCM_FMTBIT_S24_LE | \
+ SNDRV_PCM_FMTBIT_S32_LE)
+
+struct tda_priv {
+ struct i2c_client *i2c_client;
+ struct snd_soc_codec *codec;
+ u8 ports[2];
+ int dai_id;
+ u8 *eld;
+};
+
+static void tda_get_encoder(struct tda_priv *priv)
+{
+ struct snd_soc_codec *codec = priv->codec;
+ struct device_node *np;
+ struct i2c_client *i2c_client;
+ static const struct of_device_id tda_dt[] = {
+ { .compatible = "nxp,tda998x" },
+ { },
+ };
+
+ /* search the tda998x device */
+ np = of_find_matching_node_and_match(NULL, tda_dt, NULL);
+ if (!np || !of_device_is_available(np)) {
+ dev_err(codec->dev, "No tda998x in DT\n");
+ return;
+ }
+ i2c_client = of_find_i2c_device_by_node(np);
+ of_node_put(np);
+ if (!i2c_client) {
+ dev_err(codec->dev, "no tda998x i2c client\n");
+ return;
+ }
+ if (!i2c_get_clientdata(i2c_client)) {
+ dev_err(codec->dev, "tda998x not initialized\n");
+ return;
+ }
+
+ priv->i2c_client = i2c_client;
+}
+
+static int tda_start_stop(struct tda_priv *priv,
+ int start)
+{
+ int format, port;
+
+ if (!priv->i2c_client) {
+ tda_get_encoder(priv);
+ if (!priv->i2c_client)
+ return -EINVAL;
+ }
+
+ /* give the audio input type and ports to the HDMI encoder */
+ format = start ? priv->dai_id : 0;
+ switch (format) {
+ case AFMT_I2S:
+ port = priv->ports[0];
+ break;
+ default:
+ case AFMT_SPDIF:
+ port = priv->ports[1];
+ break;
+ }
+ tda998x_audio_update(priv->i2c_client, format, port);
+ return 0;
+}
+
+static int tda_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct tda_priv *priv = snd_soc_codec_get_drvdata(dai->codec);
+
+ /* memorize the used DAI */
+ priv->dai_id = dai->id;
+
+ /* start the TDA998x audio */
+ return tda_start_stop(priv, 1);
+}
+
+static void tda_shutdown(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct tda_priv *priv = snd_soc_codec_get_drvdata(dai->codec);
+
+ priv->dai_id = 0;
+ tda_start_stop(priv, 0);
+}
+
+static const struct snd_soc_dai_ops tda_ops = {
+ .startup = tda_startup,
+ .shutdown = tda_shutdown,
+};
+
+static const struct snd_soc_dai_driver tda998x_dai[] = {
+ {
+ .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,
+
+ },
+ .ops = &tda_ops,
+ },
+ {
+ .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,
+ },
+ .ops = &tda_ops,
+ },
+};
+
+static const struct snd_soc_dapm_widget tda_widgets[] = {
+ SND_SOC_DAPM_OUTPUT("hdmi-out"),
+};
+static const struct snd_soc_dapm_route tda_routes[] = {
+ { "hdmi-out", NULL, "HDMI I2S Playback" },
+ { "hdmi-out", NULL, "HDMI SPDIF Playback" },
+};
+
+static int tda_probe(struct snd_soc_codec *codec)
+{
+ struct tda_priv *priv;
+ struct device_node *np;
+ int i, ret;
+
+ priv = devm_kzalloc(codec->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+ snd_soc_codec_set_drvdata(codec, priv);
+ priv->codec = codec;
+
+ /* get the audio input ports (I2s and S/PDIF) */
+ np = codec->dev->of_node;
+ for (i = 0; i < 2; i++) {
+ u32 port;
+
+ ret = of_property_read_u32_index(np, "audio-ports", i, &port);
+ if (ret) {
+ if (i == 0)
+ dev_err(codec->dev,
+ "bad or missing audio-ports\n");
+ break;
+ }
+ priv->ports[i] = port;
+ }
+
+ return 0;
+}
+
+static const struct snd_soc_codec_driver soc_codec_tda998x = {
+ .probe = tda_probe,
+ .dapm_widgets = tda_widgets,
+ .num_dapm_widgets = ARRAY_SIZE(tda_widgets),
+ .dapm_routes = tda_routes,
+ .num_dapm_routes = ARRAY_SIZE(tda_routes),
+};
+
+static int tda998x_dev_probe(struct platform_device *pdev)
+{
+ return snd_soc_register_codec(&pdev->dev,
+ &soc_codec_tda998x,
+ tda998x_dai, ARRAY_SIZE(tda998x_dai));
+}
+
+static int tda998x_dev_remove(struct platform_device *pdev)
+{
+ snd_soc_unregister_codec(&pdev->dev);
+ return 0;
+}
+
+static const struct of_device_id tda998x_codec_ids[] = {
+ { .compatible = "nxp,tda998x-codec", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, tda998x_codec_ids);
+
+static struct platform_driver tda998x_driver = {
+ .probe = tda998x_dev_probe,
+ .remove = tda998x_dev_remove,
+ .driver = {
+ .name = "tda998x-codec",
+ .owner = THIS_MODULE,
+ .of_match_table = tda998x_codec_ids,
+ },
+};
+
+module_platform_driver(tda998x_driver);
+
+MODULE_AUTHOR("Jean-Francois Moine");
+MODULE_DESCRIPTION("TDA998x codec driver");
+MODULE_LICENSE("GPL");
--
1.8.5.3
Add devicetree documentation about the NXP TDA998x CODEC.
Signed-off-by: Jean-Francois Moine <[email protected]>
---
Documentation/devicetree/bindings/sound/tda998x.txt | 14 ++++++++++++++
1 file changed, 14 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/tda998x.txt
diff --git a/Documentation/devicetree/bindings/sound/tda998x.txt b/Documentation/devicetree/bindings/sound/tda998x.txt
new file mode 100644
index 0000000..30273a6
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/tda998x.txt
@@ -0,0 +1,14 @@
+Device-Tree bindings for the NXP TDA998x HDMI transmitter
+
+Required properties:
+ - compatible: must be "nxp,tda998x-codec".
+ - audio-ports: one or two values.
+ The first value defines the I2S input port.
+ The second one, if present, defines the S/PDIF input port.
+
+Example node:
+
+ hdmi_codec: hdmi-codec {
+ compatible = "nxp,tda998x-codec";
+ audio-ports = <0x03>, <0x04>;
+ };
--
1.8.5.3
The supported audio parameters are described in the EDID which is
received by the HDMI transmitter from the connected screen.
Use these ones to adjust the audio stream parameters.
Signed-off-by: Jean-Francois Moine <[email protected]>
---
drivers/gpu/drm/i2c/tda998x_drv.c | 15 ++++++++
include/drm/i2c/tda998x.h | 1 +
sound/soc/codecs/tda998x.c | 79 ++++++++++++++++++++++++++++++++++++++-
3 files changed, 94 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 186c751..3c8b4d7 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -45,6 +45,8 @@ struct tda998x_priv {
wait_queue_head_t wq_edid;
volatile int wq_edid_wait;
struct drm_encoder *encoder;
+
+ u8 *eld;
};
#define to_tda998x_priv(x) ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv)
@@ -731,6 +733,14 @@ tda998x_configure_audio(struct tda998x_priv *priv,
}
/* tda998x codec interface */
+u8 *tda998x_audio_get_eld(struct i2c_client *client)
+{
+ struct tda998x_priv *priv = i2c_get_clientdata(client);
+
+ return priv->eld;
+}
+EXPORT_SYMBOL_GPL(tda998x_audio_get_eld);
+
void tda998x_audio_update(struct i2c_client *client,
int format,
int port)
@@ -1186,6 +1196,11 @@ tda998x_encoder_get_modes(struct drm_encoder *encoder,
drm_mode_connector_update_edid_property(connector, edid);
n = drm_add_edid_modes(connector, edid);
priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid);
+
+ /* keep the EDID as ELD for the audio subsystem */
+ drm_edid_to_eld(connector, edid);
+ priv->eld = connector->eld;
+
kfree(edid);
}
diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
index 0459931..64aaaa8 100644
--- a/include/drm/i2c/tda998x.h
+++ b/include/drm/i2c/tda998x.h
@@ -26,6 +26,7 @@ struct tda998x_encoder_params {
unsigned audio_sample_rate;
};
+u8 *tda998x_audio_get_eld(struct i2c_client *client);
void tda998x_audio_update(struct i2c_client *client,
int format,
int port);
diff --git a/sound/soc/codecs/tda998x.c b/sound/soc/codecs/tda998x.c
index d724f7d..500ac61 100644
--- a/sound/soc/codecs/tda998x.c
+++ b/sound/soc/codecs/tda998x.c
@@ -91,10 +91,79 @@ static int tda_startup(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
struct tda_priv *priv = snd_soc_codec_get_drvdata(dai->codec);
+ u8 *eld = NULL;
+ static unsigned rates_mask[] = {
+ SNDRV_PCM_RATE_32000,
+ SNDRV_PCM_RATE_44100,
+ SNDRV_PCM_RATE_48000,
+ SNDRV_PCM_RATE_88200,
+ SNDRV_PCM_RATE_96000,
+ SNDRV_PCM_RATE_176400,
+ SNDRV_PCM_RATE_192000,
+ };
/* memorize the used DAI */
priv->dai_id = dai->id;
+ /* get the ELD from the tda998x driver */
+ if (!priv->i2c_client)
+ tda_get_encoder(priv);
+ if (priv->i2c_client)
+ eld = tda998x_audio_get_eld(priv->i2c_client);
+
+ /* adjust the hw params from the ELD (EDID) */
+ if (eld) {
+ struct snd_soc_dai_driver *dai_drv = dai->driver;
+ struct snd_soc_pcm_stream *stream = &dai_drv->playback;
+ u8 *sad;
+ int sad_count;
+ unsigned eld_ver, mnl, rates, rate_mask, i;
+ unsigned max_channels, fmt;
+ u64 formats;
+
+ eld_ver = eld[0] >> 3;
+ if (eld_ver != 2 && eld_ver != 31)
+ return 0;
+
+ mnl = eld[4] & 0x1f;
+ if (mnl > 16)
+ return 0;
+
+ sad_count = eld[5] >> 4;
+ sad = eld + 20 + mnl;
+
+ /* Start from the basic audio settings */
+ max_channels = 2;
+ rates = 0;
+ fmt = 0;
+ while (sad_count--) {
+ switch (sad[0] & 0x78) {
+ case 0x08: /* PCM */
+ max_channels = max(max_channels, (sad[0] & 7) + 1u);
+ rates |= sad[1];
+ fmt |= sad[2] & 0x07;
+ break;
+ }
+ sad += 3;
+ }
+
+ for (rate_mask = i = 0; i < ARRAY_SIZE(rates_mask); i++)
+ if (rates & 1 << i)
+ rate_mask |= rates_mask[i];
+ 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;
+
+ /* change the snd_soc_pcm_stream values of the driver */
+ stream->rates = rate_mask;
+ stream->channels_max = max_channels;
+ stream->formats = formats;
+ }
+
/* start the TDA998x audio */
return tda_start_stop(priv, 1);
}
@@ -193,9 +262,17 @@ static const struct snd_soc_codec_driver soc_codec_tda998x = {
static int tda998x_dev_probe(struct platform_device *pdev)
{
+ struct snd_soc_dai_driver *dai_drv;
+
+ /* copy the DAI driver to a writable area */
+ dai_drv = devm_kzalloc(&pdev->dev, sizeof(tda998x_dai), GFP_KERNEL);
+ if (!dai_drv)
+ return -ENOMEM;
+ memcpy(dai_drv, tda998x_dai, sizeof(tda998x_dai));
+
return snd_soc_register_codec(&pdev->dev,
&soc_codec_tda998x,
- tda998x_dai, ARRAY_SIZE(tda998x_dai));
+ dai_drv, ARRAY_SIZE(tda998x_dai));
}
static int tda998x_dev_remove(struct platform_device *pdev)
--
1.8.5.3
On Sun, Jan 26, 2014 at 07:45:36PM +0100, Jean-Francois Moine wrote:
> select SND_SOC_STAC9766 if SND_SOC_AC97_BUS
> select SND_SOC_TAS5086 if I2C
> + select SND_SOC_TDA998X if I2C
> +config SND_SOC_TDA998X
> + tristate
> + depends on OF
> + default y if DRM_I2C_NXP_TDA998X=y
> + default m if DRM_I2C_NXP_TDA998X=m
> +
These two things don't seem to agree with each other, and I expect this
breaks the build for ALL_CODECS.
> +static void tda_get_encoder(struct tda_priv *priv)
> +{
> + struct snd_soc_codec *codec = priv->codec;
> + struct device_node *np;
> + struct i2c_client *i2c_client;
> + static const struct of_device_id tda_dt[] = {
> + { .compatible = "nxp,tda998x" },
> + { },
> + };
> +
> + /* search the tda998x device */
> + np = of_find_matching_node_and_match(NULL, tda_dt, NULL);
> + if (!np || !of_device_is_available(np)) {
> + dev_err(codec->dev, "No tda998x in DT\n");
> + return;
> + }
> + i2c_client = of_find_i2c_device_by_node(np);
> + of_node_put(np);
> + if (!i2c_client) {
> + dev_err(codec->dev, "no tda998x i2c client\n");
> + return;
> + }
> + if (!i2c_get_clientdata(i2c_client)) {
> + dev_err(codec->dev, "tda998x not initialized\n");
> + return;
> + }
> +
> + priv->i2c_client = i2c_client;
> +}
The normal way of doing this would be to make the device a MFD - that
way the frameworks make sure probe ordering and so on are sorted out.
Other than that this looks basically OK, a few small comments below.
> + /* give the audio input type and ports to the HDMI encoder */
> + format = start ? priv->dai_id : 0;
It's not clear to me what the above is doing (I think I know but it
requires thinking about rather than being clear).
> +static const struct snd_soc_dai_driver tda998x_dai[] = {
> + {
> + .name = "i2s-hifi",
Coding style, indents are tabs.
On Mon, Jan 27, 2014 at 09:34:49AM +0100, Jean-Francois Moine wrote:
> + - compatible: must be "nxp,tda998x-codec".
> + - audio-ports: one or two values.
> + The first value defines the I2S input port.
> + The second one, if present, defines the S/PDIF input port.
I take it these are port numbers on the device and it's not possible to
do something like having multiple I2S ports?
On Mon, Jan 27, 2014 at 09:48:54AM +0100, Jean-Francois Moine wrote:
> + eld_ver = eld[0] >> 3;
> + if (eld_ver != 2 && eld_ver != 31)
> + return 0;
> +
> + mnl = eld[4] & 0x1f;
> + if (mnl > 16)
> + return 0;
> +
> + sad_count = eld[5] >> 4;
> + sad = eld + 20 + mnl;
Can this parsing code be factored out - it (or large parts of it) should
be usable by other HDMI devices shouldn't it?
On Mon, Jan 27, 2014 at 08:43:02PM +0000, Mark Brown wrote:
> On Mon, Jan 27, 2014 at 09:34:49AM +0100, Jean-Francois Moine wrote:
>
> > + - compatible: must be "nxp,tda998x-codec".
> > + - audio-ports: one or two values.
> > + The first value defines the I2S input port.
> > + The second one, if present, defines the S/PDIF input port.
>
> I take it these are port numbers on the device and it's not possible to
> do something like having multiple I2S ports?
You can feed it with multiple synchronised I2S streams for the additional
channels.
--
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
On Mon, Jan 27, 2014 at 08:44:41PM +0000, Mark Brown wrote:
> On Mon, Jan 27, 2014 at 09:48:54AM +0100, Jean-Francois Moine wrote:
>
> > + eld_ver = eld[0] >> 3;
> > + if (eld_ver != 2 && eld_ver != 31)
> > + return 0;
> > +
> > + mnl = eld[4] & 0x1f;
> > + if (mnl > 16)
> > + return 0;
> > +
> > + sad_count = eld[5] >> 4;
> > + sad = eld + 20 + mnl;
>
> Can this parsing code be factored out - it (or large parts of it) should
> be usable by other HDMI devices shouldn't it?
Yes, preferably as a generic ALSA helper rather than an ASoC helper -
I don't see any need for this to be ASoC specific (I have a pure ALSA
driver which has very similar code in it.)
--
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
On Mon, Jan 27, 2014 at 08:49:15PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 27, 2014 at 08:44:41PM +0000, Mark Brown wrote:
> > Can this parsing code be factored out - it (or large parts of it) should
> > be usable by other HDMI devices shouldn't it?
> Yes, preferably as a generic ALSA helper rather than an ASoC helper -
> I don't see any need for this to be ASoC specific (I have a pure ALSA
> driver which has very similar code in it.)
Indeed, definitely ALSA generic - ideally we could factor a lot of the
integration with the video side out.
On Mon, Jan 27, 2014 at 08:45:34PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 27, 2014 at 08:43:02PM +0000, Mark Brown wrote:
> > On Mon, Jan 27, 2014 at 09:34:49AM +0100, Jean-Francois Moine wrote:
> > > + - audio-ports: one or two values.
> > > + The first value defines the I2S input port.
> > > + The second one, if present, defines the S/PDIF input port.
> > I take it these are port numbers on the device and it's not possible to
> > do something like having multiple I2S ports?
> You can feed it with multiple synchronised I2S streams for the additional
> channels.
Ah, I feared as much. The bindings ought to take account of that
posibility even if the code can't use additional ports yet. Perhaps
separate properties for I2S and S/PDIF?
At Mon, 27 Jan 2014 20:54:37 +0000,
Mark Brown wrote:
>
> On Mon, Jan 27, 2014 at 08:49:15PM +0000, Russell King - ARM Linux wrote:
> > On Mon, Jan 27, 2014 at 08:44:41PM +0000, Mark Brown wrote:
>
> > > Can this parsing code be factored out - it (or large parts of it) should
> > > be usable by other HDMI devices shouldn't it?
>
> > Yes, preferably as a generic ALSA helper rather than an ASoC helper -
> > I don't see any need for this to be ASoC specific (I have a pure ALSA
> > driver which has very similar code in it.)
>
> Indeed, definitely ALSA generic - ideally we could factor a lot of the
> integration with the video side out.
Yes, indeed.
OTOH, as discussed recently, we're heading to move from ELD parsing to
more direct communication between video and audio drivers for
HD-audio. ELD will be still provided to user-space, but not evaluated
any longer in the new scenario.
Takashi
On Tue, Jan 28, 2014 at 10:23:57AM +0100, Takashi Iwai wrote:
> Mark Brown wrote:
> > On Mon, Jan 27, 2014 at 08:49:15PM +0000, Russell King - ARM Linux wrote:
> > > Yes, preferably as a generic ALSA helper rather than an ASoC helper -
> > > I don't see any need for this to be ASoC specific (I have a pure ALSA
> > > driver which has very similar code in it.)
> > Indeed, definitely ALSA generic - ideally we could factor a lot of the
> > integration with the video side out.
> Yes, indeed.
> OTOH, as discussed recently, we're heading to move from ELD parsing to
> more direct communication between video and audio drivers for
> HD-audio. ELD will be still provided to user-space, but not evaluated
> any longer in the new scenario.
That sort of refactoring being one of the best reasons to keep things
out of individual drivers! Having said all this I don't know if it's
worth blocking Jean-Francois' work on that, it's an improvement in
itself. Splitting the code out a bit would be good to help prepare but
having the full refactoring done might be too much of a blocker.
At Tue, 28 Jan 2014 11:00:51 +0000,
Mark Brown wrote:
>
> On Tue, Jan 28, 2014 at 10:23:57AM +0100, Takashi Iwai wrote:
> > Mark Brown wrote:
> > > On Mon, Jan 27, 2014 at 08:49:15PM +0000, Russell King - ARM Linux wrote:
>
> > > > Yes, preferably as a generic ALSA helper rather than an ASoC helper -
> > > > I don't see any need for this to be ASoC specific (I have a pure ALSA
> > > > driver which has very similar code in it.)
>
> > > Indeed, definitely ALSA generic - ideally we could factor a lot of the
> > > integration with the video side out.
>
> > Yes, indeed.
>
> > OTOH, as discussed recently, we're heading to move from ELD parsing to
> > more direct communication between video and audio drivers for
> > HD-audio. ELD will be still provided to user-space, but not evaluated
> > any longer in the new scenario.
>
> That sort of refactoring being one of the best reasons to keep things
> out of individual drivers! Having said all this I don't know if it's
> worth blocking Jean-Francois' work on that, it's an improvement in
> itself. Splitting the code out a bit would be good to help prepare but
> having the full refactoring done might be too much of a blocker.
I just rather wanted to point out the general direction for the
further development, didn't mean NAK. Jean-Francois' patch itself
looks simple enough, so I see no problem to take it for now as is.
Takashi