On Sun, 11 Jan 2015 23:03:14 +0200 Jyri Sarha jsarha@ti.com wrote:
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.
[snip]
@@ -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?
Good idea. The rate_constraints would be allocated by the codec and set by a transmitter function.
[snip]
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.
There cannot be both ways of function call with modules: the transmitter may call exported functions of the codec, but the codec cannot call exported functions of the transmitter.
[snip]
- 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.
Right. Thanks.
[snip]
+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?
Before I treated the EDID, I directly used these definitions, and my TV display accepted many more rates as 22050 with S/PDIF input or 7875 with I2S input.
+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.
[snip]
This will be changed when using the graph of ports: the DAIs will be dynamically created from the DT.