[alsa-devel] [PATCH v13 0/3] ASoC: tda998x: add a codec to the HDMI transmitter
This patch series asks for Russell King's patch
http://article.gmane.org/gmane.linux.ports.arm.kernel/411574 drm/edid: add function to help find SADs
to be applayed.
v13: - rebase on 4.2.0-rc3 - remove Russell's patches v12: - use Russell King's DRM ELD helper (Mark Brown) v11: - reduce the patch series to adding the tda998x codec only v10: - add the generic dt-card - define the audio ports from a DT graph of ports (Russell King) - reuse HDMI constants (Andrew Jackson - Jyri Sarha) - alloc rate_constraints in codec (Jyri Sarha) - fix bad number of channels (Jyri Sarha) - correct codec generation from config (Russell King - Jyri Sarha) - no module init/exit (Russell King) v9: - back to a TDA998x specific CODEC - more comments - change magic values to constants v8: - change some comments about the patches v7: - remove the change of the K predivider (Jyri Sarha) - add S24_3LE and S32_LE as possible audio formats (Jyri Sarha) - don't move the struct priv2 definition and use the slave encoder private data as the device private data (Russell King) - remove the useless request_module (Russell King/Mark Brown) - don't lock the HDMI module (Russell King) - use platform_device_unregister to remove the codec (Russell King) 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 (3): drm/i2c: tda998x: Add support of a DT graph of ports drm/i2c: tda998x: Change drvdata for audio extension ASoC: tda998x: add a codec to the HDMI transmitter
.../devicetree/bindings/drm/i2c/tda998x.txt | 51 ++++++ drivers/gpu/drm/i2c/tda998x_drv.c | 178 +++++++++++++++++++-- include/sound/tda998x.h | 23 +++ sound/soc/codecs/Kconfig | 6 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/tda998x.c | 132 +++++++++++++++ 6 files changed, 381 insertions(+), 11 deletions(-) create mode 100644 include/sound/tda998x.h create mode 100644 sound/soc/codecs/tda998x.c
Two kinds of ports may be declared in a DT graph of ports: video and audio. This patch accepts the port value from a video port as an alternative to the video-ports property. It also accepts audio ports in the case the transmitter is not used as a slave encoder. The new file include/sound/tda998x.h prepares to the definition of a tda998x CODEC.
Signed-off-by: Jean-Francois Moine moinejf@free.fr --- .../devicetree/bindings/drm/i2c/tda998x.txt | 51 ++++++++++++ drivers/gpu/drm/i2c/tda998x_drv.c | 90 +++++++++++++++++++--- include/sound/tda998x.h | 8 ++ 3 files changed, 140 insertions(+), 9 deletions(-) create mode 100644 include/sound/tda998x.h
diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt index e9e4bce..386b6c3 100644 --- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt @@ -16,6 +16,35 @@ Optional properties:
- video-ports: 24 bits value which defines how the video controller output is wired to the TDA998x input - default: <0x230145> + This property is not used when ports are defined. + +Optional nodes: + + - port: up to three ports. + The ports are defined according to [1]. + + Video port. + There may be only one video port. + This one must contain the following property: + + - port-type: must be "rgb" + + and may contain the optional property: + + - reg: 24 bits value which defines how the video controller + output is wired to the TDA998x input (video pins) + When absent, the default value is <0x230145>. + + Audio ports. + There may be one or two audio ports. + These ones must contain the following properties: + + - port-type: must be "i2s" or "spdif" + + - reg: 8 bits value which defines how the audio controller + output is wired to the TDA998x input (audio pins) + +[1] Documentation/devicetree/bindings/graph.txt
Example:
@@ -26,4 +55,26 @@ Example: interrupts = <27 2>; /* falling edge */ pinctrl-0 = <&pmx_camera>; pinctrl-names = "default"; + + port@230145 { + port-type = "rgb"; + reg = <0x230145>; + hdmi_0: endpoint { + remote-endpoint = <&lcd0_0>; + }; + }; + port@3 { /* AP1 = I2S */ + port-type = "i2s"; + reg = <0x03>; + tda998x_i2s: endpoint { + remote-endpoint = <&audio1_i2s>; + }; + }; + port@4 { /* AP2 = S/PDIF */ + port-type = "spdif"; + reg = <0x04>; + tda998x_spdif: endpoint { + remote-endpoint = <&audio1_spdif1>; + }; + }; }; diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 424228b..b0a730a 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -27,6 +27,7 @@ #include <drm/drm_edid.h> #include <drm/drm_of.h> #include <drm/i2c/tda998x.h> +#include <sound/tda998x.h>
#define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
@@ -47,6 +48,8 @@ struct tda998x_priv { wait_queue_head_t wq_edid; volatile int wq_edid_wait; struct drm_encoder *encoder; + + struct tda998x_audio_s audio; };
#define to_tda998x_priv(x) ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv) @@ -774,6 +777,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.port_types[0] = p->audio_format; + priv->audio.ports[0] = p->audio_cfg; }
static void tda998x_encoder_dpms(struct tda998x_priv *priv, int mode) @@ -1230,9 +1235,57 @@ static struct drm_encoder_slave_funcs tda998x_encoder_slave_funcs = {
/* I2C driver functions */
+static int tda998x_parse_ports(struct tda998x_priv *priv, + struct device_node *np) +{ + struct device_node *of_port; + const char *port_type; + int ret, audio_index, reg, afmt; + + audio_index = 0; + for_each_child_of_node(np, of_port) { + if (!of_port->name + || of_node_cmp(of_port->name, "port") != 0) + continue; + ret = of_property_read_string(of_port, "port-type", + &port_type); + if (ret < 0) + continue; + ret = of_property_read_u32(of_port, "reg", ®); + if (strcmp(port_type, "rgb") == 0) { + if (!ret) { /* video reg is optional */ + priv->vip_cntrl_0 = reg >> 16; + priv->vip_cntrl_1 = reg >> 8; + priv->vip_cntrl_2 = reg; + } + continue; + } + if (strcmp(port_type, "i2s") == 0) + afmt = AFMT_I2S; + else if (strcmp(port_type, "spdif") == 0) + afmt = AFMT_SPDIF; + else + continue; + if (ret < 0) { + dev_err(&priv->hdmi->dev, "missing reg for %s\n", + port_type); + return ret; + } + if (audio_index >= ARRAY_SIZE(priv->audio.ports)) { + dev_err(&priv->hdmi->dev, "too many audio ports\n"); + break; + } + priv->audio.ports[audio_index] = reg; + priv->audio.port_types[audio_index] = afmt; + audio_index++; + } + return 0; +} + static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) { struct device_node *np = client->dev.of_node; + struct device_node *of_port; u32 video; int rev_lo, rev_hi, ret; unsigned short cec_addr; @@ -1337,15 +1390,34 @@ 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 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; + /* get the device tree parameters */ + if (np) { + of_port = of_get_child_by_name(np, "port"); + if (of_port) { /* graph of ports */ + of_node_put(of_port); + ret = tda998x_parse_ports(priv, np); + if (ret < 0) + goto fail; + + /* initialize the default audio configuration */ + if (priv->audio.ports[0]) { + priv->params.audio_cfg = priv->audio.ports[0]; + priv->params.audio_format = + priv->audio.port_types[0]; + priv->params.audio_clk_cfg = + priv->params.audio_format == + AFMT_SPDIF ? 0 : 1; + } + } else { + + /* 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; + } + } }
return 0; diff --git a/include/sound/tda998x.h b/include/sound/tda998x.h new file mode 100644 index 0000000..487a809 --- /dev/null +++ b/include/sound/tda998x.h @@ -0,0 +1,8 @@ +#ifndef SND_TDA998X_H +#define SND_TDA998X_H + +struct tda998x_audio_s { + u8 ports[2]; /* AP value */ + u8 port_types[2]; /* AFMT_xxx */ +}; +#endif
The device drvdata is used for component bind, but points to the encoder/connector structure which is hidden from the slave encoder. For audio extension, the slave encoder private data must be accessible, so, this patch changes drvdata to the slave encoder private data and sets it in case of slave encoder use.
Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index b0a730a..655ebb0 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1453,6 +1453,8 @@ static int tda998x_encoder_init(struct i2c_client *client, encoder_slave->slave_priv = priv; encoder_slave->slave_funcs = &tda998x_encoder_slave_funcs;
+ dev_set_drvdata(&client->dev, priv); + return 0; }
@@ -1580,7 +1582,7 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data) if (!priv) return -ENOMEM;
- dev_set_drvdata(dev, priv); + dev_set_drvdata(dev, &priv->base);
if (dev->of_node) crtcs = drm_of_find_possible_crtcs(drm, dev->of_node); @@ -1639,7 +1641,9 @@ err_encoder: static void tda998x_unbind(struct device *dev, struct device *master, void *data) { - struct tda998x_priv2 *priv = dev_get_drvdata(dev); + struct tda998x_priv *priv_s = dev_get_drvdata(dev); + struct tda998x_priv2 *priv = + container_of(priv_s, struct tda998x_priv2, base);
drm_connector_cleanup(&priv->connector); drm_encoder_cleanup(&priv->encoder);
The tda998x CODEC maintains the audio 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@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 80 +++++++++++++++++++++++ include/sound/tda998x.h | 15 +++++ sound/soc/codecs/Kconfig | 6 ++ sound/soc/codecs/Makefile | 2 + sound/soc/codecs/tda998x.c | 132 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 235 insertions(+) create mode 100644 sound/soc/codecs/tda998x.c
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 655ebb0..90a508f 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -758,6 +758,66 @@ tda998x_configure_audio(struct tda998x_priv *priv, tda998x_write_aif(priv, p); }
+#if IS_ENABLED(CONFIG_SND_SOC_TDA998X) +/* tda998x audio codec interface */ + +/* return the ELD to the CODEC */ +static u8 *tda998x_get_eld(struct device *dev) +{ + struct tda998x_priv *priv = dev_get_drvdata(dev); + struct drm_encoder *encoder = priv->encoder; + struct drm_device *drm = encoder->dev; + struct drm_connector *connector; + + if (!priv->is_hdmi_sink + || !encoder->crtc) + return NULL; + + list_for_each_entry(connector, &drm->mode_config.connector_list, head) { + if (connector->encoder == encoder) + return connector->eld; + } + return NULL; +} + +/* 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) +{ + 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 = priv->audio.port_types[port_index]; + p->audio_clk_cfg = p->audio_format == AFMT_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; +} + +static struct tda998x_ops_s tda998x_codec_ops = { + .get_eld = tda998x_get_eld, + .set_audio_input = tda998x_set_audio_input, +}; +#endif /* SND_SOC */ + /* DRM encoder functions */
static void tda998x_encoder_set_config(struct tda998x_priv *priv, @@ -1123,6 +1183,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); + +#if IS_ENABLED(CONFIG_SND_SOC_TDA998X) + if (priv->is_hdmi_sink) + drm_edid_to_eld(connector, edid); +#endif + kfree(edid);
return n; @@ -1158,6 +1224,10 @@ static void tda998x_destroy(struct tda998x_priv *priv) cancel_delayed_work_sync(&priv->dwork); }
+#if IS_ENABLED(CONFIG_SND_SOC_TDA998X) + if (priv->audio.ports[0]) + tda9998x_codec_unregister(&priv->hdmi->dev); +#endif i2c_unregister_device(priv->cec); }
@@ -1294,6 +1364,9 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) 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; /* CEC I2C address bound to TDA998x I2C addr by configuration pins */ @@ -1407,6 +1480,13 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) priv->params.audio_clk_cfg = priv->params.audio_format == AFMT_SPDIF ? 0 : 1; + +#if IS_ENABLED(CONFIG_SND_SOC_TDA998X) + /* and create the audio CODEC */ + tda9998x_codec_register(&client->dev, + &priv->audio, + &tda998x_codec_ops); +#endif } } else {
diff --git a/include/sound/tda998x.h b/include/sound/tda998x.h index 487a809..8e89db4 100644 --- a/include/sound/tda998x.h +++ b/include/sound/tda998x.h @@ -1,8 +1,23 @@ #ifndef SND_TDA998X_H #define SND_TDA998X_H
+/* port index for audio stream stop */ +#define PORT_NONE (-1) + struct tda998x_audio_s { u8 ports[2]; /* AP value */ u8 port_types[2]; /* AFMT_xxx */ }; + +struct tda998x_ops_s { + u8 *(*get_eld)(struct device *dev); + int (*set_audio_input)(struct device *dev, + int port_index, + unsigned sample_rate); +}; + +int tda9998x_codec_register(struct device *dev, + struct tda998x_audio_s *tda998x_audio_i, + struct tda998x_ops_s *tda998x_ops); +void tda9998x_codec_unregister(struct device *dev); #endif diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index efaafce..08b73bd 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -105,6 +105,7 @@ config SND_SOC_ALL_CODECS select SND_SOC_TAS2552 if I2C select SND_SOC_TAS5086 if I2C select SND_SOC_TAS571X 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 @@ -622,6 +623,11 @@ config SND_SOC_TAS571X tristate "Texas Instruments TAS5711/TAS5717/TAS5719 power amplifiers" depends on I2C
+config SND_SOC_TDA998X + def_tristate y + select SND_PCM_ELD + depends on DRM_I2C_NXP_TDA998X + 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 cf160d9..819d689 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -108,6 +108,7 @@ snd-soc-sta529-objs := sta529.o snd-soc-stac9766-objs := stac9766.o snd-soc-tas5086-objs := tas5086.o snd-soc-tas571x-objs := tas571x.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 @@ -292,6 +293,7 @@ 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_TAS571X) += snd-soc-tas571x.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..f588a75 --- /dev/null +++ b/sound/soc/codecs/tda998x.c @@ -0,0 +1,132 @@ +/* + * 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_drm_eld.h> +#include <drm/i2c/tda998x.h> +#include <sound/tda998x.h> + +/* functions in tda998x_drv */ +static struct tda998x_ops_s *tda998x_ops; + +static int tda998x_codec_startup(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + u8 *eld; + + eld = tda998x_ops->get_eld(dai->dev); + if (!eld) + return -ENODEV; + return snd_pcm_hw_constraint_eld(runtime, eld); +} + +/* ask the HDMI transmitter to activate the audio input port */ +static int tda998x_codec_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + struct snd_soc_dai *dai) +{ + return tda998x_ops->set_audio_input(dai->dev, dai->id, + params_rate(params)); +} + +static void tda998x_codec_shutdown(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + tda998x_ops->set_audio_input(dai->dev, PORT_NONE, 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_dai_i2s = { + .name = "i2s-hifi", + .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, +}; + +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, + struct tda998x_audio_s *tda998x_audio, + struct tda998x_ops_s *tda998x_ops_i) +{ + struct snd_soc_dai_driver *dais, *p_dai; + int i, ndais; + + tda998x_ops = tda998x_ops_i; + + /* build the DAIs */ + for (ndais = 0; ndais < ARRAY_SIZE(tda998x_audio->ports); ndais++) { + if (!tda998x_audio->ports[ndais]) + break; + } + dais = devm_kzalloc(dev, sizeof(*dais) * ndais, GFP_KERNEL); + if (!dais) + return -ENOMEM; + for (i = 0, p_dai = dais; i < ndais ; i++, p_dai++) { + memcpy(p_dai, &tda998x_dai_i2s, sizeof(*p_dai)); + p_dai->id = i; + if (tda998x_audio->port_types[i] == AFMT_SPDIF) { + p_dai->name = "spdif-hifi"; + p_dai->playback.stream_name = "HDMI SPDIF Playback"; + p_dai->playback.channels_max = 2; + p_dai->playback.rate_min = 22050; + } + } + + return snd_soc_register_codec(dev, + &tda998x_codec_drv, + dais, ndais); +} +EXPORT_SYMBOL(tda9998x_codec_register); + +void tda9998x_codec_unregister(struct device *dev) +{ + snd_soc_unregister_codec(dev); +} +EXPORT_SYMBOL(tda9998x_codec_unregister); + +MODULE_AUTHOR("Jean-Francois Moine moinejf@free.fr"); +MODULE_DESCRIPTION("TDA998X CODEC"); +MODULE_LICENSE("GPL");
On Fri, May 08, 2015 at 10:41:12AM +0200, Jean-Francois Moine wrote:
- if (!priv->is_hdmi_sink
|| !encoder->crtc)return NULL;
That's weird indentation.
- list_for_each_entry(connector, &drm->mode_config.connector_list, head) {
if (connector->encoder == encoder)return connector->eld;- }
What guarantees that connector->eld stays valid after it's returned?
+struct tda998x_ops_s {
- u8 *(*get_eld)(struct device *dev);
Why _ops_s - what does the _s mean? If it's sound perhaps it makes sense to spell it out so people aren't guessing.
+int tda9998x_codec_register(struct device *dev,
struct tda998x_audio_s *tda998x_audio_i,struct tda998x_ops_s *tda998x_ops);+void tda9998x_codec_unregister(struct device *dev);
I'd expect these to be internal to the DRM driver.
+config SND_SOC_TDA998X
- def_tristate y
- select SND_PCM_ELD
- depends on DRM_I2C_NXP_TDA998X
def_tristate y? Why?
+/* functions in tda998x_drv */ +static struct tda998x_ops_s *tda998x_ops;
I'd expect this to be stored in the driver data rather than a static global, what if a system has two HDMI outputs?
+static int tda998x_codec_startup(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)+{
struct snd_pcm_runtime *runtime = substream->runtime;u8 *eld;eld = tda998x_ops->get_eld(dai->dev);if (!eld)return -ENODEV;return snd_pcm_hw_constraint_eld(runtime, eld);+}
Do we really need a device specific mechanism for fishing the ELD out of the graphics code? I'd have expected this to be more generic.
+/* ask the HDMI transmitter to activate the audio input port */ +static int tda998x_codec_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,struct snd_soc_dai *dai)+{
- return tda998x_ops->set_audio_input(dai->dev, dai->id,
params_rate(params));+}
The set_audio_input() function doesn't appear to have anything that checks if the device is busy before enabling things, what happens if the user tries to switch between I2S and S/PDIF? It looks like only one DAI can be active at once.
- for (i = 0, p_dai = dais; i < ndais ; i++, p_dai++) {
memcpy(p_dai, &tda998x_dai_i2s, sizeof(*p_dai));p_dai->id = i;if (tda998x_audio->port_types[i] == AFMT_SPDIF) {p_dai->name = "spdif-hifi";p_dai->playback.stream_name = "HDMI SPDIF Playback";p_dai->playback.channels_max = 2;p_dai->playback.rate_min = 22050;}- }
It would be a bit clearer if the template were just a template and this copying initialised both I2S and S/PDIF specific settings.
- return snd_soc_register_codec(dev,
&tda998x_codec_drv,dais, ndais);+} +EXPORT_SYMBOL(tda9998x_codec_register);
ASoC is all EXPORT_SYMBOL_GPL, you shouldn't reexport functionality with plain EXPORT_SYMBOL.
On Mon, 20 Jul 2015 19:06:06 +0100 Mark Brown broonie@kernel.org wrote:
On Fri, May 08, 2015 at 10:41:12AM +0200, Jean-Francois Moine wrote:
- if (!priv->is_hdmi_sink
|| !encoder->crtc)return NULL;That's weird indentation.
But the conditions are aligned... This sequence will be removed.
- list_for_each_entry(connector, &drm->mode_config.connector_list, head) {
if (connector->encoder == encoder)return connector->eld;- }
What guarantees that connector->eld stays valid after it's returned?
You are right, the ELD content may change. I will use a pointer to a stable content.
+struct tda998x_ops_s {
- u8 *(*get_eld)(struct device *dev);
Why _ops_s - what does the _s mean? If it's sound perhaps it makes sense to spell it out so people aren't guessing.
I will remove it.
+int tda9998x_codec_register(struct device *dev,
struct tda998x_audio_s *tda998x_audio_i,struct tda998x_ops_s *tda998x_ops);+void tda9998x_codec_unregister(struct device *dev);
I'd expect these to be internal to the DRM driver.
I don't see where. What is your idea?
+config SND_SOC_TDA998X
- def_tristate y
- select SND_PCM_ELD
- depends on DRM_I2C_NXP_TDA998X
def_tristate y? Why?
The TDA998x CODEC is always generated when the DRM TDA998x is generated and when audio is wanted. Its type, built-in or module, depends on the TDA998x driver type.
+/* functions in tda998x_drv */ +static struct tda998x_ops_s *tda998x_ops;
I'd expect this to be stored in the driver data rather than a static global, what if a system has two HDMI outputs?
Each TDA998x has only one HDMI output and there is only one driver.
I will put the pointer to the HDMI driver in the device private area.
+static int tda998x_codec_startup(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)+{
struct snd_pcm_runtime *runtime = substream->runtime;u8 *eld;eld = tda998x_ops->get_eld(dai->dev);if (!eld)return -ENODEV;return snd_pcm_hw_constraint_eld(runtime, eld);+}
Do we really need a device specific mechanism for fishing the ELD out of the graphics code? I'd have expected this to be more generic.
I will put the ELD in the private data of the DRM driver.
+/* ask the HDMI transmitter to activate the audio input port */ +static int tda998x_codec_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,struct snd_soc_dai *dai)+{
- return tda998x_ops->set_audio_input(dai->dev, dai->id,
params_rate(params));+}
The set_audio_input() function doesn't appear to have anything that checks if the device is busy before enabling things, what happens if the user tries to switch between I2S and S/PDIF? It looks like only one DAI can be active at once.
Right. I will check this double streaming.
- for (i = 0, p_dai = dais; i < ndais ; i++, p_dai++) {
memcpy(p_dai, &tda998x_dai_i2s, sizeof(*p_dai));p_dai->id = i;if (tda998x_audio->port_types[i] == AFMT_SPDIF) {p_dai->name = "spdif-hifi";p_dai->playback.stream_name = "HDMI SPDIF Playback";p_dai->playback.channels_max = 2;p_dai->playback.rate_min = 22050;}- }
It would be a bit clearer if the template were just a template and this copying initialised both I2S and S/PDIF specific settings.
OK, I will change this.
- return snd_soc_register_codec(dev,
&tda998x_codec_drv,dais, ndais);+} +EXPORT_SYMBOL(tda9998x_codec_register);
ASoC is all EXPORT_SYMBOL_GPL, you shouldn't reexport functionality with plain EXPORT_SYMBOL.
Sorry, bug of mine.
On Tue, Jul 28, 2015 at 12:19:45PM +0200, Jean-Francois Moine wrote:
Mark Brown broonie@kernel.org wrote:
+int tda9998x_codec_register(struct device *dev,
struct tda998x_audio_s *tda998x_audio_i,struct tda998x_ops_s *tda998x_ops);+void tda9998x_codec_unregister(struct device *dev);
I'd expect these to be internal to the DRM driver.
I don't see where. What is your idea?
What I said above - put them in the DRM driver. They're just registering a device IIRC.
+config SND_SOC_TDA998X
- def_tristate y
- select SND_PCM_ELD
- depends on DRM_I2C_NXP_TDA998X
def_tristate y? Why?
The TDA998x CODEC is always generated when the DRM TDA998x is generated and when audio is wanted. Its type, built-in or module, depends on the TDA998x driver type.
Just make this like any other driver with no default specified.
+/* functions in tda998x_drv */ +static struct tda998x_ops_s *tda998x_ops;
I'd expect this to be stored in the driver data rather than a static global, what if a system has two HDMI outputs?
Each TDA998x has only one HDMI output and there is only one driver.
Someone might put two chips on the board.
+static int tda998x_codec_startup(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)+{
struct snd_pcm_runtime *runtime = substream->runtime;u8 *eld;eld = tda998x_ops->get_eld(dai->dev);if (!eld)return -ENODEV;return snd_pcm_hw_constraint_eld(runtime, eld);+}
Do we really need a device specific mechanism for fishing the ELD out of the graphics code? I'd have expected this to be more generic.
I will put the ELD in the private data of the DRM driver.
That's not the issue here - the issue is that the need to get the ELD out of the video subsystem is not specific to this driver, it's pretty common and something that it seems we should factor out.
On Tue, 28 Jul 2015 11:24:10 +0100 Mark Brown broonie@kernel.org wrote:
On Tue, Jul 28, 2015 at 12:19:45PM +0200, Jean-Francois Moine wrote:
Mark Brown broonie@kernel.org wrote:
+int tda9998x_codec_register(struct device *dev,
struct tda998x_audio_s *tda998x_audio_i,struct tda998x_ops_s *tda998x_ops);+void tda9998x_codec_unregister(struct device *dev);
I'd expect these to be internal to the DRM driver.
I don't see where. What is your idea?
What I said above - put them in the DRM driver. They're just registering a device IIRC.
There is no CODEC device. The HDMI device supports both audio and video.
+config SND_SOC_TDA998X
- def_tristate y
- select SND_PCM_ELD
- depends on DRM_I2C_NXP_TDA998X
def_tristate y? Why?
The TDA998x CODEC is always generated when the DRM TDA998x is generated and when audio is wanted. Its type, built-in or module, depends on the TDA998x driver type.
Just make this like any other driver with no default specified.
If I understand correctly, you want that the user might choose to use the HDMI for video only and to have audio by some other mean. No problem.
+/* functions in tda998x_drv */ +static struct tda998x_ops_s *tda998x_ops;
I'd expect this to be stored in the driver data rather than a static global, what if a system has two HDMI outputs?
Each TDA998x has only one HDMI output and there is only one driver.
Someone might put two chips on the board.
The pointer was from the tda998x codec to the tda998x driver. There might be any number of chips on the board.
+static int tda998x_codec_startup(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)+{
struct snd_pcm_runtime *runtime = substream->runtime;u8 *eld;eld = tda998x_ops->get_eld(dai->dev);if (!eld)return -ENODEV;return snd_pcm_hw_constraint_eld(runtime, eld);+}
Do we really need a device specific mechanism for fishing the ELD out of the graphics code? I'd have expected this to be more generic.
I will put the ELD in the private data of the DRM driver.
That's not the issue here - the issue is that the need to get the ELD out of the video subsystem is not specific to this driver, it's pretty common and something that it seems we should factor out.
Yes, but how?
The EDID arrives in the DRM connector when video starts. The built ELD may be stored either in the connector itself (default), in the encoder (TDA998x device) or in some DRM/encoder/connector private data.
On the audio side, the CODEC is supported by a driver which is either a CODEC driver (as in the dummy HDMI codec) or the DRM encoder (TDA998x device).
You may note that, in DRM, there is no relation between the encoder and the connector except at video startup time, and no relation between the DRM connector and the audio CODEC (no CODEC information is returned on CODEC creation and the CODEC has no private data).
I tried many solutions to solve this problem, and the one of may latest patchset (v14) seems the simpleset: set the audio/video common part at the beginning of the TDA998x (HDMI) private data.
On Tue, Jul 28, 2015 at 03:23:29PM +0200, Jean-Francois Moine wrote:
The EDID arrives in the DRM connector when video starts. The built ELD may be stored either in the connector itself (default), in the encoder (TDA998x device) or in some DRM/encoder/connector private data.
The ELD is stored in the DRM connector, and there _should_ be a system of locking which ensures that you can get at that information safely.
The ELD is only updated when the connector's get_modes() method is called, and the driver itself calls drm_edid_to_eld(). This all happens under the drm_device's struct_mutex.
So, to safely read the ELD from outside DRM, you need to take the top-level drm_device's struct_mutex. That could be fraught, as that's quite a big lock, so an alternative would be to introduce an 'eld' lock to the driver, which protects the call to drm_edid_to_eld() and the reading of the ELD.
However, that doesn't solve the problem of passing the data to an audio driver. What's needed there is a notification system which allows a video driver to broadcast HDMI events such as:
* connected * new EDID available * new ELD available * disconnected
With such a system, different components driving the facilities of a HDMI connector can make decisions on what to do - which not only includes things like the audio driver, but also a driver for the CEC interface (which also needs to see the EDID to get at its "address".) This can be done in a safe manner as the HDMI video driver would have control over the generation of these messages.
The point that Mark is trying to get you to see is that this problem is not specific to TDA998x. The same problem exists for many other HDMI interfaces (except maybe Intel's i9x5/HDA stuff which has a hardware mechanism of passing the ELD data from the video driver, through the hardware to the audio driver.)
It needs solving in a driver-agnostic way, and the normal way that happens is when someone comes along, wanting to add support in that area, they get to do the hard work to propose that infrastructure.
You may note that, in DRM, there is no relation between the encoder and the connector except at video startup time, and no relation between the DRM connector and the audio CODEC (no CODEC information is returned on CODEC creation and the CODEC has no private data).
In the case of the TDA998x, there is a 1:1 fixed relationship between the connector and the encoder. The connector part can't be used with any other encoder, and the encoder part can't be used with any other connector. The same is true of many other HDMI implementations (such as the Synopsis Designware HDMI interface found on iMX6 and Rockchip.)
On Tue, 28 Jul 2015 15:53:58 +0200, Russell King - ARM Linux wrote:
On Tue, Jul 28, 2015 at 03:23:29PM +0200, Jean-Francois Moine wrote:
The EDID arrives in the DRM connector when video starts. The built ELD may be stored either in the connector itself (default), in the encoder (TDA998x device) or in some DRM/encoder/connector private data.
The ELD is stored in the DRM connector, and there _should_ be a system of locking which ensures that you can get at that information safely.
The ELD is only updated when the connector's get_modes() method is called, and the driver itself calls drm_edid_to_eld(). This all happens under the drm_device's struct_mutex.
So, to safely read the ELD from outside DRM, you need to take the top-level drm_device's struct_mutex. That could be fraught, as that's quite a big lock, so an alternative would be to introduce an 'eld' lock to the driver, which protects the call to drm_edid_to_eld() and the reading of the ELD.
However, that doesn't solve the problem of passing the data to an audio driver. What's needed there is a notification system which allows a video driver to broadcast HDMI events such as:
- connected
- new EDID available
- new ELD available
- disconnected
With such a system, different components driving the facilities of a HDMI connector can make decisions on what to do - which not only includes things like the audio driver, but also a driver for the CEC interface (which also needs to see the EDID to get at its "address".) This can be done in a safe manner as the HDMI video driver would have control over the generation of these messages.
The point that Mark is trying to get you to see is that this problem is not specific to TDA998x. The same problem exists for many other HDMI interfaces (except maybe Intel's i9x5/HDA stuff which has a hardware mechanism of passing the ELD data from the video driver, through the hardware to the audio driver.)
FWIW, we're currently discussing about extending the i915/hda component binding so that the audio driver gets a notification and queries the ELD via callbacks instead of the flaky hardware access.
It'd be best if we have a common infrastructure for that, of course. But currently it's a start, just a bit step forward there.
Takashi
On Tue, Jul 28, 2015 at 03:59:45PM +0200, Takashi Iwai wrote:
FWIW, we're currently discussing about extending the i915/hda component binding so that the audio driver gets a notification and queries the ELD via callbacks instead of the flaky hardware access.
It'd be best if we have a common infrastructure for that, of course. But currently it's a start, just a bit step forward there.
Okay, so it sounds like i915/hda also needs this.
I think in addition to what I said above, we need whatever provides the notification system to generate state notifications for new "listeners" that would otherwise be unaware of the current state.
I'm thinking at the moment that something along these lines to go into drivers/video/hdmi.c / include/linux/hdmi.h:
struct hdmi_state { struct mutex mutex; struct raw_notifier_head head; bool connected; bool edid_valid; u8 edid[MAX_EDID_SIZE]; };
enum { HDMI_CONNECTED, HDMI_DISCONNECTED, HDMI_NEW_EDID, };
int hdmi_register_notifier(struct notifier_block *nb) { struct hdmi_state *state = ...; int ret;
mutex_lock(&state->mutex); if (state->connected) { nb->notifier_call(nb, HDMI_CONNECTED, NULL); if (state->edid_valid) nb->notifier_call(nb, HDMI_NEW_EDID, state->edid); } else { nb->notifier_call(nb, HDMI_DISCONNECTED, NULL); } ret = raw_notifier_chain_register(&state->head, nb); mutex_unlock(&state->mutex);
return ret; }
int hdmi_unregister_notifier(struct notifier_block *nb) { struct hdmi_state *state = ...; int ret;
mutex_lock(&state->mutex); ret = raw_notifier_chain_unregister(&state->head, nb); mutex_unlock(&state->mutex);
return ret; }
void hdmi_event_connect(struct device *dev) { struct hdmi_state *state = lookup_state_from_dev(dev);
mutex_lock(&state->mutex); state->connected = true; raw_notifier_call_chain(&state->head, HDMI_CONNECTED, NULL); mutex_unlock(&state->mutex); }
void hdmi_event_disconnect(struct device *dev) { struct hdmi_state *state = lookup_state_from_dev(dev);
mutex_lock(&state->mutex); state->connected = false; state->valid_edid = false; raw_notifier_call_chain(&state->head, HDMI_DISCONNECTED, NULL); mutex_unlock(&state->mutex); }
int hdmi_event_new_edid(struct device *dev, const void *edid, size_t size) { struct hdmi_state *state = lookup_state_from_dev(dev);
if (size > sizeof(state->edid)) return -EINVAL;
mutex_lock(&state->mutex); state->valid_edid = true; memcpy(state->edid, edid, size); raw_notifier_call_chain(&state->head, HDMI_NEW_EDID, state->edid); mutex_unlock(&state->mutex);
return 0; }
The problems here are: how to go from a struct device to the state (iow, the implementation of lookup_state_from_dev()) and how to convey on the client side which 'state' to attach to. I'd rather not involve DT at this level: DT is not the world's only firmware system, we have to cater for x86 too here, so we need something that is firmware agnostic.
This is something I've just banged out in this email...
On Tue, 28 Jul 2015 14:53:58 +0100 Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Tue, Jul 28, 2015 at 03:23:29PM +0200, Jean-Francois Moine wrote:
The EDID arrives in the DRM connector when video starts. The built ELD may be stored either in the connector itself (default), in the encoder (TDA998x device) or in some DRM/encoder/connector private data.
The ELD is stored in the DRM connector, and there _should_ be a system of locking which ensures that you can get at that information safely.
The ELD is only updated when the connector's get_modes() method is called, and the driver itself calls drm_edid_to_eld(). This all happens under the drm_device's struct_mutex.
So, to safely read the ELD from outside DRM, you need to take the top-level drm_device's struct_mutex. That could be fraught, as that's quite a big lock, so an alternative would be to introduce an 'eld' lock to the driver, which protects the call to drm_edid_to_eld() and the reading of the ELD.
I think that my solution should work: - the CODEC uses a pointer in the driver private data to the ELD. - when get_modes() is called, this pointer is reset to NULL. (no audio streaming is permitted). - this function reads the EDID and this asks for hardware exchanges with the HDMI device. - the EDID is then translated to ELD and the ELD pointer is set (audio streaming is permitted again).
Then, if audio streaming is started before get_modes() is called, the memory treatment of the ELD may be safely done during the harware exchanges.
However, that doesn't solve the problem of passing the data to an audio driver. What's needed there is a notification system which allows a video driver to broadcast HDMI events such as:
- connected
- new EDID available
- new ELD available
- disconnected
My patch just wants to offer basic audio to the TDA998x. Especially, the display device state is of no importance: audio streaming may continue while there is no connected device.
With such a system, different components driving the facilities of a HDMI connector can make decisions on what to do - which not only includes things like the audio driver, but also a driver for the CEC interface (which also needs to see the EDID to get at its "address".) This can be done in a safe manner as the HDMI video driver would have control over the generation of these messages.
The point that Mark is trying to get you to see is that this problem is not specific to TDA998x. The same problem exists for many other HDMI interfaces (except maybe Intel's i9x5/HDA stuff which has a hardware mechanism of passing the ELD data from the video driver, through the hardware to the audio driver.)
I know that, but I don't know enough about all the DRM and ASoC drivers to propose a global mechanism.
It needs solving in a driver-agnostic way, and the normal way that happens is when someone comes along, wanting to add support in that area, they get to do the hard work to propose that infrastructure.
OK. I'll stop here.
You may note that, in DRM, there is no relation between the encoder and the connector except at video startup time, and no relation between the DRM connector and the audio CODEC (no CODEC information is returned on CODEC creation and the CODEC has no private data).
In the case of the TDA998x, there is a 1:1 fixed relationship between the connector and the encoder. The connector part can't be used with any other encoder, and the encoder part can't be used with any other connector. The same is true of many other HDMI implementations (such as the Synopsis Designware HDMI interface found on iMX6 and Rockchip.)
But there is no direct link (pointer) between the encoder and the connector.
On Tue, Jul 28, 2015 at 04:39:12PM +0200, Jean-Francois Moine wrote:
On Tue, 28 Jul 2015 14:53:58 +0100 Russell King - ARM Linux linux@arm.linux.org.uk wrote:
In the case of the TDA998x, there is a 1:1 fixed relationship between the connector and the encoder. The connector part can't be used with any other encoder, and the encoder part can't be used with any other connector. The same is true of many other HDMI implementations (such as the Synopsis Designware HDMI interface found on iMX6 and Rockchip.)
But there is no direct link (pointer) between the encoder and the connector.
There's no direct link from an encoder to a connector because the DRM model allows a single encoder to be associated with multiple connectors. Adding a link from the encoder to a connector breaks that ability (an encoder would then need an array of connectors.)
If we kill the old drm_slave_encoder support in TDA998x, then this would solve the problem - we can then get at the TDA998x's drm_connector easily as it then becomes part of TDA998x's private data.
However, this doesn't matter. All that we need from the DRM side of the TDA998x is to know is the EDID or ELD. That can be broadcast via a notification (using something like the code I illustrated in a follow up email) to any listeners, whether they be an audio driver or a CEC bus driver. The audio driver may not care about HDMI connect/disconnect events, but that's no reason not to include them. Some consumers of these events need to know (CEC certainly needs to know.)
participants (4)
-
Jean-Francois Moine -
Mark Brown -
Russell King - ARM Linux -
Takashi Iwai