[alsa-devel] [PATCH] ASoC: tda998x: add a codec to the HDMI transmitter
This patch adds a CODEC function to the NXP TDA998x HDMI transmitter.
The CODEC handles both I2S and S/PDIF inputs and does dynamic input switch in the TDA998x I2C driver on start/stop audio streaming.
Signed-off-by: Jean-Francois Moine moinejf@free.fr --- This patch applies against linux-next. --- .../devicetree/bindings/drm/i2c/tda998x.txt | 13 ++ drivers/gpu/drm/i2c/Makefile | 2 +- drivers/gpu/drm/i2c/tda998x_codec.c | 241 +++++++++++++++++++++ drivers/gpu/drm/i2c/tda998x_drv.c | 74 +++++-- drivers/gpu/drm/i2c/tda998x_drv.h | 31 +++ include/drm/i2c/tda998x.h | 1 + 6 files changed, 341 insertions(+), 21 deletions(-) create mode 100644 drivers/gpu/drm/i2c/tda998x_codec.c create mode 100644 drivers/gpu/drm/i2c/tda998x_drv.h
diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt index d7df01c..7ce4eac 100644 --- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt @@ -15,6 +15,15 @@ Optional properties: - video-ports: 24 bits value which defines how the video controller output is wired to the TDA998x input - default: <0x230145>
+ - audio-ports: one or two values corresponding to entries in + the audio-port-names property. + + - audio-port-names: must contain "i2s", "spdif" entries + matching entries in the audio-ports property. + + - #sound-dai-cells: must be set to <1> for use with the simple-card. + The DAI 0 is the I2S input and the DAI 1 is the S/PDIF input. + Example:
tda998x: hdmi-encoder { @@ -24,4 +33,8 @@ Example: interrupts = <27 2>; /* falling edge */ pinctrl-0 = <&pmx_camera>; pinctrl-names = "default"; + + audio-ports = <0x03>, <0x04>; + audio-port-names = "i2s", "spdif"; + #sound-dai-cells = <1>; }; diff --git a/drivers/gpu/drm/i2c/Makefile b/drivers/gpu/drm/i2c/Makefile index 43aa33b..f2d625c 100644 --- a/drivers/gpu/drm/i2c/Makefile +++ b/drivers/gpu/drm/i2c/Makefile @@ -6,5 +6,5 @@ obj-$(CONFIG_DRM_I2C_CH7006) += ch7006.o sil164-y := sil164_drv.o obj-$(CONFIG_DRM_I2C_SIL164) += sil164.o
-tda998x-y := tda998x_drv.o +tda998x-y := tda998x_drv.o tda998x_codec.o obj-$(CONFIG_DRM_I2C_NXP_TDA998X) += tda998x.o diff --git a/drivers/gpu/drm/i2c/tda998x_codec.c b/drivers/gpu/drm/i2c/tda998x_codec.c new file mode 100644 index 0000000..dff6afb --- /dev/null +++ b/drivers/gpu/drm/i2c/tda998x_codec.c @@ -0,0 +1,241 @@ +/* + * ALSA SoC TDA998X CODEC + * + * 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 <sound/pcm_params.h> +#include <linux/of.h> +#include <linux/i2c.h> +#include <drm/drm_encoder_slave.h> +#include <drm/i2c/tda998x.h> + +#include "tda998x_drv.h" + +#define TDA998X_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \ + SNDRV_PCM_FMTBIT_S20_3LE | \ + SNDRV_PCM_FMTBIT_S24_LE | \ + SNDRV_PCM_FMTBIT_S32_LE) + +static int tda_startup(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct tda998x_priv *priv = snd_soc_codec_get_drvdata(dai->codec); + u8 *eld = priv->eld; + struct snd_pcm_runtime *runtime = substream->runtime; + u8 *sad; + int sad_count; + unsigned eld_ver, mnl, rate_mask; + unsigned max_channels, fmt; + u64 formats; + struct snd_pcm_hw_constraint_list *rate_constraints = + &priv->rate_constraints; + static const u32 hdmi_rates[] = { + 32000, 44100, 48000, 88200, 9600, 176400, 192000 + }; + + if (!eld) + return 0; + + /* adjust the hw params from the ELD (EDID) */ + 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; + 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; + } + + /* set the 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 tda_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + struct snd_soc_dai *dai) +{ + struct tda998x_priv *priv = snd_soc_codec_get_drvdata(dai->codec); + + /* Requires an attached display */ + if (!priv->encoder->crtc) + return -ENODEV; + + /* if same input and same parameters, do not do a full switch */ + if (dai->id == priv->params.audio_format && + params_format(params) == priv->audio_sample_format) { + tda998x_audio_start(priv, 0); + return 0; + } + priv->params.audio_format = dai->id; + priv->audio_sample_format = params_format(params); + priv->params.audio_cfg = + priv->audio_ports[dai->id == AFMT_I2S ? 0 : 1]; + tda998x_audio_start(priv, 1); + return 0; +} + +static void tda_shutdown(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct tda998x_priv *priv = snd_soc_codec_get_drvdata(dai->codec); + + tda998x_audio_stop(priv); +} + +static const struct snd_soc_dai_ops tda_ops = { + .startup = tda_startup, + .hw_params = tda_hw_params, + .shutdown = tda_shutdown, +}; + +static 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 i2c_client *i2c_client = to_i2c_client(codec->dev); + struct tda998x_priv *priv = i2c_get_clientdata(i2c_client); + struct device_node *np = codec->dev->of_node; + int i, j, ret; + const char *p; + + if (!priv) + return -ENODEV; + snd_soc_codec_set_drvdata(codec, priv); + + if (!np) + return 0; + + /* get the audio input ports*/ + 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; + } + ret = of_property_read_string_index(np, "audio-port-names", + i, &p); + if (ret) { + dev_err(codec->dev, + "missing audio-port-names[%d]\n", i); + break; + } + if (strcmp(p, "i2s") == 0) { + j = 0; + } else if (strcmp(p, "spdif") == 0) { + j = 1; + } else { + dev_err(codec->dev, + "bad audio-port-names '%s'\n", p); + break; + } + priv->audio_ports[j] = 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), +}; + +int tda998x_codec_register(struct device *dev) +{ + return snd_soc_register_codec(dev, + &soc_codec_tda998x, + tda998x_dai, ARRAY_SIZE(tda998x_dai)); +} + +void tda998x_codec_unregister(struct device *dev) +{ + snd_soc_unregister_codec(dev); +} diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 240c331..7e9f9dc 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -21,6 +21,7 @@ #include <linux/module.h> #include <linux/irq.h> #include <sound/asoundef.h> +#include <sound/pcm_params.h>
#include <drm/drmP.h> #include <drm/drm_crtc_helper.h> @@ -28,24 +29,9 @@ #include <drm/drm_edid.h> #include <drm/i2c/tda998x.h>
-#define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__) +#include "tda998x_drv.h"
-struct tda998x_priv { - struct i2c_client *cec; - struct i2c_client *hdmi; - uint16_t rev; - uint8_t current_page; - int dpms; - bool is_hdmi_sink; - u8 vip_cntrl_0; - u8 vip_cntrl_1; - u8 vip_cntrl_2; - struct tda998x_encoder_params params; - - wait_queue_head_t wq_edid; - volatile int wq_edid_wait; - struct drm_encoder *encoder; -}; +#define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
#define to_tda998x_priv(x) ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv)
@@ -640,12 +626,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) { @@ -654,13 +639,28 @@ 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: @@ -672,6 +672,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 @@ -728,6 +729,24 @@ tda998x_configure_audio(struct tda998x_priv *priv, tda998x_write_aif(priv, p); }
+/* tda998x codec interface */ +void tda998x_audio_start(struct tda998x_priv *priv, + int full) +{ + struct tda998x_encoder_params *p = &priv->params; + + if (!full) { + reg_write(priv, REG_ENA_AP, p->audio_cfg); + return; + } + tda998x_configure_audio(priv, &priv->encoder->crtc->hwmode, p); +} + +void tda998x_audio_stop(struct tda998x_priv *priv) +{ + reg_write(priv, REG_ENA_AP, 0); +} + /* DRM encoder functions */
static void @@ -1149,6 +1168,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); }
@@ -1191,6 +1215,8 @@ tda998x_encoder_destroy(struct drm_encoder *encoder) if (priv->hdmi->irq) free_irq(priv->hdmi->irq, priv);
+ tda998x_codec_unregister(&priv->hdmi->dev); + if (priv->cec) i2c_unregister_device(priv->cec); kfree(priv); @@ -1239,10 +1265,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); @@ -1340,6 +1371,9 @@ tda998x_encoder_init(struct i2c_client *client, /* enable EDID read irq: */ reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
+ /* register the audio CODEC */ + tda998x_codec_register(&client->dev); + if (!np) return 0; /* non-DT */
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.h b/drivers/gpu/drm/i2c/tda998x_drv.h new file mode 100644 index 0000000..6db5160 --- /dev/null +++ b/drivers/gpu/drm/i2c/tda998x_drv.h @@ -0,0 +1,31 @@ +/* tda998x private data */ + +struct tda998x_priv { + struct i2c_client *cec; + struct i2c_client *hdmi; + uint16_t rev; + uint8_t current_page; + int dpms; + bool is_hdmi_sink; + u8 vip_cntrl_0; + u8 vip_cntrl_1; + u8 vip_cntrl_2; + struct tda998x_encoder_params params; + + wait_queue_head_t wq_edid; + volatile int wq_edid_wait; + struct drm_encoder *encoder; + + u8 audio_ports[2]; + int audio_sample_format; + + u8 *eld; + + struct snd_pcm_hw_constraint_list rate_constraints; +}; + +int tda998x_codec_register(struct device *dev); +void tda998x_codec_unregister(struct device *dev); + +void tda998x_audio_start(struct tda998x_priv *priv, int full); +void tda998x_audio_stop(struct tda998x_priv *priv); diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h index 3e419d9..31757df 100644 --- a/include/drm/i2c/tda998x.h +++ b/include/drm/i2c/tda998x.h @@ -20,6 +20,7 @@ struct tda998x_encoder_params { u8 audio_frame[6];
enum { + AFMT_NO_AUDIO = 0, AFMT_SPDIF, AFMT_I2S } audio_format;
On Wed, Jul 02, 2014 at 06:38:41PM +0200, Jean-Francois Moine wrote:
This patch adds a CODEC function to the NXP TDA998x HDMI transmitter.
The CODEC handles both I2S and S/PDIF inputs and does dynamic input switch in the TDA998x I2C driver on start/stop audio streaming.
Hi Jean-Francois
How well will this work with Russell concept of a front end and two backends? Can you uses your CODEC twice, once with the I2S backend and a second time with the S/PDIF backend?
Andrew
On Wed, 2 Jul 2014 18:56:28 +0200 Andrew Lunn andrew@lunn.ch wrote:
How well will this work with Russell concept of a front end and two backends? Can you uses your CODEC twice, once with the I2S backend and a second time with the S/PDIF backend?
Hi Andrew,
The TDA998x CODEC has two functions: - it sets the possible formats and rates when the screen connects and - it sets the audio input port when audio streaming starts.
I tested this CODEC with both DAPM and DPCM. If the audio subsystem asks for streaming on both I2S and S/PDIF, only the last call is served (this depends on the order of the DAI links in the audio card creation table).
As I told to Russell, DPCM just asks for a 'system' DAI (front-end), but it also asks for some additional code in the kirkwood DMA driver because all PCMs are activated on streaming start.
On Wed, Jul 02, 2014 at 07:51:54PM +0200, Jean-Francois Moine wrote:
On Wed, 2 Jul 2014 18:56:28 +0200 Andrew Lunn andrew@lunn.ch wrote:
How well will this work with Russell concept of a front end and two backends? Can you uses your CODEC twice, once with the I2S backend and a second time with the S/PDIF backend?
Hi Andrew,
The TDA998x CODEC has two functions:
- it sets the possible formats and rates when the screen connects and
- it sets the audio input port when audio streaming starts.
I tested this CODEC with both DAPM and DPCM. If the audio subsystem asks for streaming on both I2S and S/PDIF, only the last call is served (this depends on the order of the DAI links in the audio card creation table).
As I told to Russell, DPCM just asks for a 'system' DAI (front-end), but it also asks for some additional code in the kirkwood DMA driver because all PCMs are activated on streaming start.
Well, as far as I'm concerned right now, it's for Mark to sort out this situation, and tell us what he wants to do with the Kirkwood stuff. I suspect he's keeping quiet because he doesn't care about it.
Given that he's the one with the knowledge of DPCM, and he's the maintainer of ASoC, he has the ultimate responsibility for sorting out this and giving direction as to how he wants to proceed.
As I understand it though, my solution is the full and proper DPCM solution, and I believe that what's currently in mainline is not a DPCM solution.
Mark needs to correct that, because he is one of the few who knows this stuff.
On Wed, Jul 02, 2014 at 07:02:12PM +0100, Russell King - ARM Linux wrote:
Well, as far as I'm concerned right now, it's for Mark to sort out this situation, and tell us what he wants to do with the Kirkwood stuff. I suspect he's keeping quiet because he doesn't care about it.
Well, that and the general level of stress around the support for these boards in general.
Given that he's the one with the knowledge of DPCM, and he's the maintainer of ASoC, he has the ultimate responsibility for sorting out this and giving direction as to how he wants to proceed.
As I understand it though, my solution is the full and proper DPCM solution, and I believe that what's currently in mainline is not a DPCM solution.
That's right, from what I remember your last set of patches looked like it was pretty much there in terms of the code for the CPU. Liam's obviously the most in depth expert with DPCM but he's incredibly snowed under with work right now.
On Wed, Jul 02, 2014 at 07:51:54PM +0200, Jean-Francois Moine wrote:
I tested this CODEC with both DAPM and DPCM. If the audio subsystem asks for streaming on both I2S and S/PDIF, only the last call is served (this depends on the order of the DAI links in the audio card creation table).
I'd expect this to return an error for the busy DAI rather than just silently ignore it failing to start or (better) implement some control to let the user select which of the DAIs is active.
On Wed, 2 Jul 2014 20:42:52 +0100 Mark Brown broonie@kernel.org wrote:
I tested this CODEC with both DAPM and DPCM. If the audio subsystem asks for streaming on both I2S and S/PDIF, only the last call is served (this depends on the order of the DAI links in the audio card creation table).
I'd expect this to return an error for the busy DAI rather than just silently ignore it failing to start or (better) implement some control to let the user select which of the DAIs is active.
Mark,
This is not an error. If the audio subsystem (DPCM, not the user) chooses to activate both I2S and S/PDIF, this means the HDMI audio may be taken either from I2S or from S/PDIF: both inputs have the right format and rate.
On Thu, Jul 03, 2014 at 07:49:59AM +0200, Jean-Francois Moine wrote:
Mark Brown broonie@kernel.org wrote:
I'd expect this to return an error for the busy DAI rather than just silently ignore it failing to start or (better) implement some control to let the user select which of the DAIs is active.
This is not an error. If the audio subsystem (DPCM, not the user) chooses to activate both I2S and S/PDIF, this means the HDMI audio may be taken either from I2S or from S/PDIF: both inputs have the right format and rate.
Your board happens to only be able to present the same input on both I2S and S/PDIF but that might not apply to other boards, they may be able to route different signals to each which would present a practical problem.
On Thu, 3 Jul 2014 11:44:32 +0100 Mark Brown broonie@kernel.org wrote:
I'd expect this to return an error for the busy DAI rather than just silently ignore it failing to start or (better) implement some control to let the user select which of the DAIs is active.
This is not an error. If the audio subsystem (DPCM, not the user) chooses to activate both I2S and S/PDIF, this means the HDMI audio may be taken either from I2S or from S/PDIF: both inputs have the right format and rate.
Your board happens to only be able to present the same input on both I2S and S/PDIF but that might not apply to other boards, they may be able to route different signals to each which would present a practical problem.
If there are two different streamss on I2S and S/PDIF, and if the audio subsystem wants to route these streams to the same connector (widget 'hdmi-out'), then, somewhere, there should be a software or a design bug. No?
Anyway, the tda998x cannot know if the double route is wanted or not.
On Thu, Jul 03, 2014 at 01:34:06PM +0200, Jean-Francois Moine wrote:
Mark Brown broonie@kernel.org wrote:
Your board happens to only be able to present the same input on both I2S and S/PDIF but that might not apply to other boards, they may be able to route different signals to each which would present a practical problem.
If there are two different streamss on I2S and S/PDIF, and if the audio subsystem wants to route these streams to the same connector (widget 'hdmi-out'), then, somewhere, there should be a software or a design bug. No?
Yes, which is why the driver shouldn't silently ignore the situation.
Anyway, the tda998x cannot know if the double route is wanted or not.
It doesn't need to know, it just needs to identify something it can't support either by providing a way to pick which interface is used or by rejecting the second interface.
On Thu, 3 Jul 2014 12:59:24 +0100 Mark Brown broonie@kernel.org wrote:
Your board happens to only be able to present the same input on both I2S and S/PDIF but that might not apply to other boards, they may be able to route different signals to each which would present a practical problem.
If there are two different streamss on I2S and S/PDIF, and if the audio subsystem wants to route these streams to the same connector (widget 'hdmi-out'), then, somewhere, there should be a software or a design bug. No?
Yes, which is why the driver shouldn't silently ignore the situation.
Anyway, the tda998x cannot know if the double route is wanted or not.
It doesn't need to know, it just needs to identify something it can't support either by providing a way to pick which interface is used or by rejecting the second interface.
OK. no problem, I can do that: only the first stream is switched and the second is rejected.
But, this means that there will be a lot of errors when DPCM will be used, because, in most cases for the Cubox (kirkwood audio + tda998x), both ways I2S and S/PDIF will be activated at the same time for a single stream (you may note that the routes from the second input cannot be blocked by the CODEC after it received the first input, because these routes have already been computed).
On Thu, Jul 03, 2014 at 03:28:26PM +0200, Jean-Francois Moine wrote:
OK. no problem, I can do that: only the first stream is switched and the second is rejected.
But, this means that there will be a lot of errors when DPCM will be used, because, in most cases for the Cubox (kirkwood audio + tda998x), both ways I2S and S/PDIF will be activated at the same time for a single stream (you may note that the routes from the second input cannot be blocked by the CODEC after it received the first input, because these routes have already been computed).
This is /very/ easy to solve on the Cubox, if only you would listen to me - I've stated many times that I2S should not be used there.
Just because the hardware is wired up with both the SPDIF connected and the I2S connected, it does *not* mean that we have to wire them both up in software.
Indeed, *everything* works with just SPDIF. The same is not true of I2S. So, what we do for the Cubox is we just wire up SPDIF in software and be done with it - we then get a fully functional setup. So using I2S on the Cubox is mostly pointless - unless you wish to use I2S for testing purposes.
Let me also point out that adding your changes - including the addition of this codec patch - explicitly deny the possibility of: * using compressed audio streams via the optical SPDIF out socket * using compressed audio streams over HDMI * using audio rates and formats not supported by the attached HDMI device via the optical SPDIF socket.
These are serious issues which thus far you have so far failed to respond on. People who use the Cubox as a media platform rather than (presumably) just a music jukebox want things like compressed audio out and SPDIF out to work.
Look, one reason to use the optical socket is because you want to push out a stream at (eg) 96kHz to your audio system, but your TV doesn't support it. With your approach, you explicitly block that because the TDA998x codec assocated with the Kirkwood CPU DAI refuses to allow that sample rate. Fine, if the attached device does not support that rate, then the right thing to do may be to disable audio transmission over HDMI, but with the Cubox hardware, limiting the sample rates is totally the wrong solution.
On Thu, 3 Jul 2014 14:43:46 +0100 Russell King - ARM Linux linux@arm.linux.org.uk wrote:
But, this means that there will be a lot of errors when DPCM will be used, because, in most cases for the Cubox (kirkwood audio + tda998x), both ways I2S and S/PDIF will be activated at the same time for a single stream (you may note that the routes from the second input cannot be blocked by the CODEC after it received the first input, because these routes have already been computed).
This is /very/ easy to solve on the Cubox, if only you would listen to me - I've stated many times that I2S should not be used there.
Just because the hardware is wired up with both the SPDIF connected and the I2S connected, it does *not* mean that we have to wire them both up in software.
Indeed, *everything* works with just SPDIF. The same is not true of I2S. So, what we do for the Cubox is we just wire up SPDIF in software and be done with it - we then get a fully functional setup. So using I2S on the Cubox is mostly pointless - unless you wish to use I2S for testing purposes.
Let me also point out that adding your changes - including the addition of this codec patch - explicitly deny the possibility of:
- using compressed audio streams via the optical SPDIF out socket
- using compressed audio streams over HDMI
- using audio rates and formats not supported by the attached HDMI device via the optical SPDIF socket.
These are serious issues which thus far you have so far failed to respond on. People who use the Cubox as a media platform rather than (presumably) just a music jukebox want things like compressed audio out and SPDIF out to work.
Look, one reason to use the optical socket is because you want to push out a stream at (eg) 96kHz to your audio system, but your TV doesn't support it. With your approach, you explicitly block that because the TDA998x codec assocated with the Kirkwood CPU DAI refuses to allow that sample rate. Fine, if the attached device does not support that rate, then the right thing to do may be to disable audio transmission over HDMI, but with the Cubox hardware, limiting the sample rates is totally the wrong solution.
I think that you did not look at my code.
Both S/PDIF and I2S work with the TDA998x. It is a choice of the audio subsystem to do this choice, not Russell King. If you want only S/PDIF, it is easy: just declare the S/PDIF DAIs, on both sides, source (kirkwood) and sinks (tda998x and S/PDIF):
dai-link@1 { /* S/PDIF - HDMI */ cpu { sound-dai = <&audio1 1>; }; ,codec { sound-dai = <&hdmi 1>; }; };
dai-link@2 { /* S/PDIF - S/PDIF */ cpu { sound-dai = <&audio1 1>; }; codec { sound-dai = <&spdif_codec>; }; };
When DPCM will handle the formats and rates, the audio subsystem will find by itself if such stream will go to the HDMI only or to the S/PDIF only or to both. The kirwood audio driver will work as it is and the tda998x CODEC will work as it is. There will be no need to change these drivers. All we need actually is some more code in DPCM or multi-codec or whatever mechanism which will route the stream according to the rates and formats.
Actually, as DPCM does not work, the user has to choose which PCM to use, otherwise, the application does a format and rate translation.
Anyway, if you want S/PDIF output with the actual kernel (3.16-rc3), in the DT, put:
spdif_codec: spdif-codec { compatible = "linux,spdif-dit"; #sound-dai-cells = <0>; };
sound { compatible = "simple-audio-card"; simple-audio-card,name = "Cubox Audio"; simple-audio-card,format = "i2s";
simple-audio-card,dai-link { /* S/PDIF - S/PDIF */ simple-audio-card,cpu { sound-dai = <&audio1 1>; }; simple-audio-card,codec { sound-dai = <&spdif_codec>; }; }; };
&audio1 { status = "okay"; clocks = <&gate_clk 13>, <&si5351 2>; clock-names = "internal", "extclk"; pinctrl-0 = <&pmx_audio1_i2s1_spdifo &pmx_audio1_extclk>; pinctrl-names = "default"; #sound-dai-cells = <1>; };
and the sound should get out of your S/PDIF optical connector.
Eventually, the TDA998x is used in other machines. Are you sure that the audio controllers of all these machines have a S/PDIF output connected to the S/PDIF input of the tda998x?
The tda998x CODEC I propose works in all configurations.
On Thu, Jul 03, 2014 at 05:29:09PM +0200, Jean-Francois Moine wrote:
I think that you did not look at my code.
Both S/PDIF and I2S work with the TDA998x. It is a choice of the audio subsystem to do this choice, not Russell King.
If you think I'm arguing because I personally want something, then we're not going to make any progress.
What I want is for this to be _correct_ and _not_ to introduce silly restrictions which break people's setups.
When DPCM will handle the formats and rates, the audio subsystem will find by itself if such stream will go to the HDMI only or to the S/PDIF only or to both. The kirwood audio driver will work as it is and the tda998x CODEC will work as it is. There will be no need to change these drivers.
Except, as Mark has already pointed out, and as I keep pointing out to you, and you keep refusing to accept, the kirkwood driver as it stands is *not* a DPCM driver. It makes no use of that stuff at all.
What you're doing in kirkwood-i2s is providing two plain DAI links, and then insisting that only one can be active any any one time.
A DPCM solution provides at least one frontend DAI link and at least one backend DAI link. Your code does not do this.
The second point is that - and as I keep telling you - that as soon as you couple your existing code (which restricts things like the sample rates) you can no longer output anything else through the optical out ON THE CUBOX. For this statement, I'm not talking _generally_, I'm talking _specifically_ about _one_ _platform_.
All we need actually is some more code in DPCM or multi-codec or whatever mechanism which will route the stream according to the rates and formats.
Yes, DPCM currently lacks full support, that's been known about for some time - I've reported it to Mark, but it seems that this isn't something that is going to get fixed until others put work into it.
Eventually, the TDA998x is used in other machines. Are you sure that the audio controllers of all these machines have a S/PDIF output connected to the S/PDIF input of the tda998x?
Which bit of "we need to support both I2S and SPDIF" in my previous emails was not clear. Which bit of "We should only support SPDIF on the Cubox" was not clear?
I *fully* acknowledge that we need to support both, but I'm putting a _strong_ recommendation to you _with_ technical reasons why we should _only_ support SPDIF on the Cubox.
On Thu, 3 Jul 2014 16:43:25 +0100 Russell King - ARM Linux linux@arm.linux.org.uk wrote:
What you're doing in kirkwood-i2s is providing two plain DAI links, and then insisting that only one can be active any any one time.
A DPCM solution provides at least one frontend DAI link and at least one backend DAI link. Your code does not do this.
You know why I did not insert the DPCM code in the kirkwood driver: it does not work. But, as I tested it end 2013, I still have the code. Do you want I propose a patch?
Which bit of "we need to support both I2S and SPDIF" in my previous emails was not clear. Which bit of "We should only support SPDIF on the Cubox" was not clear?
I *fully* acknowledge that we need to support both, but I'm putting a _strong_ recommendation to you _with_ technical reasons why we should _only_ support SPDIF on the Cubox.
Sorry, I still don't see why only S/PDIF should be supported on the Cubox. I have both, and both are working on HDMI. I hope someone will tell me it also works on S/PDIF: there is no reason it could not.
I don't see your technical reasons: you know the constraints of each protocol I2S and S/PDIF. If you don't want I2S, just don't declare it in the DT (see my previous mail). You may also note that I did not put any change relative to the Cubox DT in my patch.
participants (4)
-
Andrew Lunn -
Jean-Francois Moine -
Mark Brown -
Russell King - ARM Linux