| Message ID | 23b36acc1f769418a06f3e929eba288b5911da12.1420628786.git.moinejf@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/07/15 10:51, Jean-Francois Moine wrote: > This patch adds a CODEC to the NXP TDA998x HDMI transmitter. > > The CODEC handles both I2S and S/PDIF inputs. > It maintains the audio format and rate constraints according > to the HDMI device parameters (EDID) and sets dynamically the input > ports in the TDA998x I2C driver on start/stop audio streaming. > > Signed-off-by: Jean-Francois Moine <moinejf@free.fr> > --- > drivers/gpu/drm/i2c/Kconfig | 1 + > drivers/gpu/drm/i2c/tda998x_drv.c | 139 ++++++++++++++++++++++++++++-- > include/sound/tda998x.h | 23 +++++ > sound/soc/codecs/Kconfig | 4 + > sound/soc/codecs/Makefile | 2 + > sound/soc/codecs/tda998x.c | 175 ++++++++++++++++++++++++++++++++++++++ > 6 files changed, 339 insertions(+), 5 deletions(-) > create mode 100644 include/sound/tda998x.h > create mode 100644 sound/soc/codecs/tda998x.c > > diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig > index 22c7ed6..24fc975 100644 > --- a/drivers/gpu/drm/i2c/Kconfig > +++ b/drivers/gpu/drm/i2c/Kconfig > @@ -28,6 +28,7 @@ config DRM_I2C_SIL164 > config DRM_I2C_NXP_TDA998X > tristate "NXP Semiconductors TDA998X HDMI encoder" > default m if DRM_TILCDC > + select SND_SOC_TDA998X Do you need this since sound/soc/codecs/Kconfig conditionally brings in the CODEC driver? > help > Support for NXP Semiconductors TDA998X HDMI encoders. > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > index ce1478d..a26a516 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > @@ -27,6 +27,11 @@ > #include <drm/drm_encoder_slave.h> > #include <drm/drm_edid.h> > #include <drm/i2c/tda998x.h> > +#include <sound/tda998x.h> > + > +#if defined(CONFIG_SND_SOC_TDA998X) || defined(CONFIG_SND_SOC_TDA998X_MODULE) > +#define WITH_CODEC 1 > +#endif > > #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__) > > @@ -47,6 +52,10 @@ struct tda998x_priv { > struct drm_encoder *encoder; > > u8 audio_ports[2]; > +#ifdef WITH_CODEC > + u8 sad[3]; /* Short Audio Descriptor */ > + struct snd_pcm_hw_constraint_list rate_constraints; > +#endif > }; > > #define to_tda998x_priv(x) ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv) > @@ -730,11 +739,109 @@ tda998x_configure_audio(struct tda998x_priv *priv, > tda998x_write_aif(priv, p); > } > > +#ifdef WITH_CODEC > +/* tda998x audio codec interface */ > + > +/* return the audio parameters extracted from the last EDID */ > +static int tda998x_get_audio_var(struct device *dev, > + u8 **sad, > + struct snd_pcm_hw_constraint_list **rate_constraints) > +{ > + struct tda998x_priv *priv = dev_get_drvdata(dev); > + > + if (!priv->encoder->crtc > + || !priv->is_hdmi_sink) > + return -ENODEV; > + > + *sad = priv->sad; > + *rate_constraints = &priv->rate_constraints; > + return 0; > +} > + > +/* switch the audio port and initialize the audio parameters for streaming */ > +static int tda998x_set_audio_input(struct device *dev, > + int port_index, > + unsigned sample_rate, > + int sample_format) > +{ > + struct tda998x_priv *priv = dev_get_drvdata(dev); > + struct tda998x_encoder_params *p = &priv->params; > + > + if (!priv->encoder->crtc) > + return -ENODEV; > + > + /* if no port, just disable the audio port */ > + if (port_index == PORT_NONE) { > + reg_write(priv, REG_ENA_AP, 0); > + return 0; > + } > + > + /* if same audio parameters, just enable the audio port */ > + if (p->audio_cfg == priv->audio_ports[port_index] && > + p->audio_sample_rate == sample_rate) { > + reg_write(priv, REG_ENA_AP, p->audio_cfg); > + return 0; > + } > + > + p->audio_format = port_index == PORT_SPDIF ? AFMT_SPDIF : AFMT_I2S; > + p->audio_clk_cfg = port_index == PORT_SPDIF ? 0 : 1; > + p->audio_cfg = priv->audio_ports[port_index]; > + p->audio_sample_rate = sample_rate; > + tda998x_configure_audio(priv, &priv->encoder->crtc->hwmode, p); > + return 0; > +} > + > +/* get the audio capabilities from the EDID */ > +static void tda998x_get_audio_caps(struct tda998x_priv *priv, > + struct drm_connector *connector) > +{ > + u8 *eld = connector->eld; > + u8 *sad; > + int sad_count; > + unsigned eld_ver, mnl; > + u8 max_channels, rate_mask, fmt; > + > + /* adjust the hw params from the ELD (EDID) */ > + eld_ver = eld[0] >> 3; > + if (eld_ver != 2 && eld_ver != 31) > + return; > + > + mnl = eld[4] & 0x1f; > + if (mnl > 16) > + return; > + > + sad_count = eld[5] >> 4; > + sad = eld + 20 + mnl; > + > + /* Start from the basic audio settings */ > + max_channels = 1; > + rate_mask = 0; > + fmt = 0; > + while (sad_count--) { > + switch (sad[0] & 0x78) { > + case 0x08: /* SAD uncompressed audio */ > + if ((sad[0] & 7) > max_channels) > + max_channels = sad[0] & 7; > + rate_mask |= sad[1]; > + fmt |= sad[2] & 0x07; > + break; > + } > + sad += 3; > + } > + > + priv->sad[0] = max_channels; > + priv->sad[1] = rate_mask; > + priv->sad[2] = fmt; > +} > +#endif /* WITH_CODEC */ > + > /* DRM encoder functions */ > > static void tda998x_encoder_set_config(struct tda998x_priv *priv, > const struct tda998x_encoder_params *p) > { > + int port_index; > + > priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) | > (p->mirr_a ? VIP_CNTRL_0_MIRR_A : 0) | > VIP_CNTRL_0_SWAP_B(p->swap_b) | > @@ -749,6 +856,8 @@ static void tda998x_encoder_set_config(struct tda998x_priv *priv, > (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0); > > priv->params = *p; > + port_index = p->audio_format == AFMT_SPDIF ? PORT_SPDIF : PORT_I2S; > + priv->audio_ports[port_index] = p->audio_cfg; > } > > static void tda998x_encoder_dpms(struct tda998x_priv *priv, int mode) > @@ -1142,6 +1251,15 @@ tda998x_encoder_get_modes(struct tda998x_priv *priv, > drm_mode_connector_update_edid_property(connector, edid); > n = drm_add_edid_modes(connector, edid); > priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid); > + > +#ifdef WITH_CODEC > + /* set the audio parameters from the EDID */ > + if (priv->is_hdmi_sink) { > + drm_edid_to_eld(connector, edid); > + tda998x_get_audio_caps(priv, connector); > + } > +#endif > + > kfree(edid); > } > > @@ -1176,6 +1294,10 @@ static void tda998x_destroy(struct tda998x_priv *priv) > if (priv->hdmi->irq) > free_irq(priv->hdmi->irq, priv); > > +#ifdef WITH_CODEC > + tda9998x_codec_unregister(&priv->hdmi->dev); > +#endif > + > i2c_unregister_device(priv->cec); > } > > @@ -1384,26 +1506,33 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) > break; > } > if (strcmp(p, "spdif") == 0) { > - priv->audio_ports[0] = port; > + priv->audio_ports[PORT_SPDIF] = port; > } else if (strcmp(p, "i2s") == 0) { > - priv->audio_ports[1] = port; > + priv->audio_ports[PORT_I2S] = port; > } else { > dev_err(&client->dev, > "bad audio-port-names '%s'\n", p); > break; > } > } > - if (priv->audio_ports[0]) { > - priv->params.audio_cfg = priv->audio_ports[0]; > + if (priv->audio_ports[PORT_SPDIF]) { > + priv->params.audio_cfg = priv->audio_ports[PORT_SPDIF]; > priv->params.audio_format = AFMT_SPDIF; > priv->params.audio_clk_cfg = 0; > } else { > - priv->params.audio_cfg = priv->audio_ports[1]; > + priv->params.audio_cfg = priv->audio_ports[PORT_I2S]; > priv->params.audio_format = AFMT_I2S; > priv->params.audio_clk_cfg = 1; > } > } > > +#ifdef WITH_CODEC > + /* create the audio CODEC */ > + if (priv->audio_ports[PORT_SPDIF] || priv->audio_ports[PORT_I2S]) > + tda9998x_codec_register(&client->dev, > + tda998x_get_audio_var, > + tda998x_set_audio_input); > +#endif > return 0; > > fail: > diff --git a/include/sound/tda998x.h b/include/sound/tda998x.h > new file mode 100644 > index 0000000..97cff30 > --- /dev/null > +++ b/include/sound/tda998x.h > @@ -0,0 +1,23 @@ > +#ifndef SND_TDA998X_H > +#define SND_TDA998X_H > + > +#include <sound/pcm.h> > + > +/* port indexes */ > +#define PORT_NONE (-1) > +#define PORT_SPDIF 0 > +#define PORT_I2S 1 > + > +typedef int (*get_audio_var_t)(struct device *dev, > + u8 **sad, > + struct snd_pcm_hw_constraint_list **rate_constraints); > +typedef int (*set_audio_input_t)(struct device *dev, > + int port_index, > + unsigned sample_rate, > + int sample_format); > + > +int tda9998x_codec_register(struct device *dev, > + get_audio_var_t get_audio_var_i, > + set_audio_input_t set_audio_input_i); > +void tda9998x_codec_unregister(struct device *dev); > +#endif > diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig > index 8349f98..456c549 100644 > --- a/sound/soc/codecs/Kconfig > +++ b/sound/soc/codecs/Kconfig > @@ -102,6 +102,7 @@ config SND_SOC_ALL_CODECS > select SND_SOC_STAC9766 if SND_SOC_AC97_BUS > select SND_SOC_TAS2552 if I2C > select SND_SOC_TAS5086 if I2C > + select SND_SOC_TDA998X if DRM_I2C_NXP_TDA998X > select SND_SOC_TFA9879 if I2C > select SND_SOC_TLV320AIC23_I2C if I2C > select SND_SOC_TLV320AIC23_SPI if SPI_MASTER > @@ -600,6 +601,9 @@ config SND_SOC_TAS5086 > tristate "Texas Instruments TAS5086 speaker amplifier" > depends on I2C > > +config SND_SOC_TDA998X > + tristate > + > config SND_SOC_TFA9879 > tristate "NXP Semiconductors TFA9879 amplifier" > depends on I2C > diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile > index bbdfd1e..44b4fda 100644 > --- a/sound/soc/codecs/Makefile > +++ b/sound/soc/codecs/Makefile > @@ -104,6 +104,7 @@ snd-soc-sta350-objs := sta350.o > snd-soc-sta529-objs := sta529.o > snd-soc-stac9766-objs := stac9766.o > snd-soc-tas5086-objs := tas5086.o > +snd-soc-tda998x-objs := tda998x.o > snd-soc-tfa9879-objs := tfa9879.o > snd-soc-tlv320aic23-objs := tlv320aic23.o > snd-soc-tlv320aic23-i2c-objs := tlv320aic23-i2c.o > @@ -282,6 +283,7 @@ obj-$(CONFIG_SND_SOC_STA529) += snd-soc-sta529.o > obj-$(CONFIG_SND_SOC_STAC9766) += snd-soc-stac9766.o > obj-$(CONFIG_SND_SOC_TAS2552) += snd-soc-tas2552.o > obj-$(CONFIG_SND_SOC_TAS5086) += snd-soc-tas5086.o > +obj-$(CONFIG_SND_SOC_TDA998X) += snd-soc-tda998x.o > obj-$(CONFIG_SND_SOC_TFA9879) += snd-soc-tfa9879.o > obj-$(CONFIG_SND_SOC_TLV320AIC23) += snd-soc-tlv320aic23.o > obj-$(CONFIG_SND_SOC_TLV320AIC23_I2C) += snd-soc-tlv320aic23-i2c.o > diff --git a/sound/soc/codecs/tda998x.c b/sound/soc/codecs/tda998x.c > new file mode 100644 > index 0000000..b055570 > --- /dev/null > +++ b/sound/soc/codecs/tda998x.c > @@ -0,0 +1,175 @@ > +/* > + * ALSA SoC TDA998x CODEC > + * > + * Copyright (C) 2015 Jean-Francois Moine > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/module.h> > +#include <sound/soc.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <sound/pcm_params.h> > +#include <sound/tda998x.h> > + > +/* functions in tda998x_drv */ > +static get_audio_var_t get_audio_var; > +static set_audio_input_t set_audio_input; > + > +static int tda998x_codec_startup(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct snd_pcm_runtime *runtime = substream->runtime; > + struct snd_pcm_hw_constraint_list *rate_constraints; > + u8 *sad; /* Short Audio Descriptor (3 bytes) */ > +#define SAD_MX_CHAN_I 0 /* max number of chennels - 1 */ > +#define SAD_RATE_MASK_I 1 /* rate mask */ > +#define SAD_FMT_I 2 /* formats */ > +#define SAD_FMT_16 0x01 > +#define SAD_FMT_20 0x02 > +#define SAD_FMT_24 0x04 Wouldn't these defines be better in sound/tda998x.h, with an appropriate prefix, then you could use them in gpu/drm/i2c/tda998x_drv.c. Andrew > + u64 formats; > + int ret; > + static const u32 hdmi_rates[] = { > + 32000, 44100, 48000, 88200, 96000, 176400, 192000 > + }; > + > + /* get the EDID values and the rate constraints buffer */ > + ret = get_audio_var(dai->dev, &sad, &rate_constraints); > + if (ret < 0) > + return ret; /* no screen */ > + > + /* convert the EDID values to audio constraints */ > + rate_constraints->list = hdmi_rates; > + rate_constraints->count = ARRAY_SIZE(hdmi_rates); > + rate_constraints->mask = sad[SAD_RATE_MASK_I]; > + snd_pcm_hw_constraint_list(runtime, 0, > + SNDRV_PCM_HW_PARAM_RATE, > + rate_constraints); > + > + formats = 0; > + if (sad[SAD_FMT_I] & SAD_FMT_16) > + formats |= SNDRV_PCM_FMTBIT_S16_LE; > + if (sad[SAD_FMT_I] & SAD_FMT_20) > + formats |= SNDRV_PCM_FMTBIT_S20_3LE; > + if (sad[SAD_FMT_I] & SAD_FMT_24) > + formats |= SNDRV_PCM_FMTBIT_S24_LE | > + SNDRV_PCM_FMTBIT_S24_3LE | > + SNDRV_PCM_FMTBIT_S32_LE; > + snd_pcm_hw_constraint_mask64(runtime, > + SNDRV_PCM_HW_PARAM_FORMAT, > + formats); > + > + snd_pcm_hw_constraint_minmax(runtime, > + SNDRV_PCM_HW_PARAM_CHANNELS, > + 1, sad[SAD_MX_CHAN_I]); > + return 0; > +} > + > +static int tda998x_codec_hw_params(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *params, > + struct snd_soc_dai *dai) > +{ > + return set_audio_input(dai->dev, dai->id, > + params_rate(params), > + params_format(params)); > +} > + > +static void tda998x_codec_shutdown(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + set_audio_input(dai->dev, PORT_NONE, 0, 0); > +} > + > +static const struct snd_soc_dai_ops tda998x_codec_ops = { > + .startup = tda998x_codec_startup, > + .hw_params = tda998x_codec_hw_params, > + .shutdown = tda998x_codec_shutdown, > +}; > + > +#define TDA998X_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \ > + SNDRV_PCM_FMTBIT_S20_3LE | \ > + SNDRV_PCM_FMTBIT_S24_LE | \ > + SNDRV_PCM_FMTBIT_S32_LE) > + > +static struct snd_soc_dai_driver tda998x_dais[] = { > + { > + .name = "spdif-hifi", > + .id = PORT_SPDIF, > + .playback = { > + .stream_name = "HDMI SPDIF Playback", > + .channels_min = 1, > + .channels_max = 2, > + .rates = SNDRV_PCM_RATE_CONTINUOUS, > + .rate_min = 22050, > + .rate_max = 192000, > + .formats = TDA998X_FORMATS, > + }, > + .ops = &tda998x_codec_ops, > + }, > + { > + .name = "i2s-hifi", > + .id = PORT_I2S, > + .playback = { > + .stream_name = "HDMI I2S Playback", > + .channels_min = 1, > + .channels_max = 8, > + .rates = SNDRV_PCM_RATE_CONTINUOUS, > + .rate_min = 5512, > + .rate_max = 192000, > + .formats = TDA998X_FORMATS, > + }, > + .ops = &tda998x_codec_ops, > + }, > +}; > + > +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)); > +} > +EXPORT_SYMBOL(tda9998x_codec_register); > + > +void tda9998x_codec_unregister(struct device *dev) > +{ > + snd_soc_unregister_codec(dev); > +} > +EXPORT_SYMBOL(tda9998x_codec_unregister); > + > +static int __init tda998x_codec_init(void) > +{ > + return 0; > +} > +static void __exit tda998x_codec_exit(void) > +{ > +} > +module_init(tda998x_codec_init); > +module_exit(tda998x_codec_exit); > + > +MODULE_AUTHOR("Jean-Francois Moine <moinejf@free.fr>"); > +MODULE_DESCRIPTION("TDA998X CODEC"); > +MODULE_LICENSE("GPL"); > -- > 2.1.4 > >
On Wed, Jan 07, 2015 at 03:10:47PM +0000, Andrew Jackson wrote: > On 01/07/15 10:51, Jean-Francois Moine wrote: > > diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig > > index 22c7ed6..24fc975 100644 > > --- a/drivers/gpu/drm/i2c/Kconfig > > +++ b/drivers/gpu/drm/i2c/Kconfig > > @@ -28,6 +28,7 @@ config DRM_I2C_SIL164 > > config DRM_I2C_NXP_TDA998X > > tristate "NXP Semiconductors TDA998X HDMI encoder" > > default m if DRM_TILCDC > > + select SND_SOC_TDA998X > > Do you need this since sound/soc/codecs/Kconfig conditionally brings in the > CODEC driver? More importantly, it's broken, because it'll cause Kconfig to complain if you enable DRM_I2C_NXP_TDA998X, but have ASoC disabled. Moreover, if you decide you want the sound and ASoC built as a module, but want to build in DRM and TDA998x support (so you can get video output working on boot before initramfs/rootfs), Kconfig may well complain. > > +static int __init tda998x_codec_init(void) > > +{ > > + return 0; > > +} > > +static void __exit tda998x_codec_exit(void) > > +{ > > +} > > +module_init(tda998x_codec_init); > > +module_exit(tda998x_codec_exit); There's no need for these if they remain as the above. Modules can have both init/exit functions, or neither, and they are still unloadable. Only modules which provide only an init function get locked in on load.
On Wed, Jan 07, 2015 at 03:10:47PM +0000, Andrew Jackson wrote: > On 01/07/15 10:51, Jean-Francois Moine wrote: > > This patch adds a CODEC to the NXP TDA998x HDMI transmitter. > > > > The CODEC handles both I2S and S/PDIF inputs. Please delete unneeded context from your messages, it makes it much easier to find any new content you've added.
On Wed, 7 Jan 2015 15:41:38 +0000 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Wed, Jan 07, 2015 at 03:10:47PM +0000, Andrew Jackson wrote: > > On 01/07/15 10:51, Jean-Francois Moine wrote: > > > diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig > > > index 22c7ed6..24fc975 100644 > > > --- a/drivers/gpu/drm/i2c/Kconfig > > > +++ b/drivers/gpu/drm/i2c/Kconfig > > > @@ -28,6 +28,7 @@ config DRM_I2C_SIL164 > > > config DRM_I2C_NXP_TDA998X > > > tristate "NXP Semiconductors TDA998X HDMI encoder" > > > default m if DRM_TILCDC > > > + select SND_SOC_TDA998X > > > > Do you need this since sound/soc/codecs/Kconfig conditionally brings in the > > CODEC driver? For SND_SOC_ALL_CODECS only. > More importantly, it's broken, because it'll cause Kconfig to complain > if you enable DRM_I2C_NXP_TDA998X, but have ASoC disabled. > > Moreover, if you decide you want the sound and ASoC built as a module, > but want to build in DRM and TDA998x support (so you can get video > output working on boot before initramfs/rootfs), Kconfig may well > complain. select SND_SOC_TDA998X if SND_SOC should work in all cases. > > > +static int __init tda998x_codec_init(void) > > > +{ > > > + return 0; > > > +} > > > +static void __exit tda998x_codec_exit(void) > > > +{ > > > +} > > > +module_init(tda998x_codec_init); > > > +module_exit(tda998x_codec_exit); > > There's no need for these if they remain as the above. Modules can have > both init/exit functions, or neither, and they are still unloadable. > Only modules which provide only an init function get locked in on load. Thanks.
On 01/07/2015 12:51 PM, Jean-Francois Moine wrote: > This patch adds a CODEC to the NXP TDA998x HDMI transmitter. > > The CODEC handles both I2S and S/PDIF inputs. > It maintains the audio format and rate constraints according > to the HDMI device parameters (EDID) and sets dynamically the input > ports in the TDA998x I2C driver on start/stop audio streaming. > > Signed-off-by: Jean-Francois Moine <moinejf@free.fr> > --- > drivers/gpu/drm/i2c/Kconfig | 1 + > drivers/gpu/drm/i2c/tda998x_drv.c | 139 ++++++++++++++++++++++++++++-- > include/sound/tda998x.h | 23 +++++ > sound/soc/codecs/Kconfig | 4 + > sound/soc/codecs/Makefile | 2 + > sound/soc/codecs/tda998x.c | 175 ++++++++++++++++++++++++++++++++++++++ > 6 files changed, 339 insertions(+), 5 deletions(-) > create mode 100644 include/sound/tda998x.h > create mode 100644 sound/soc/codecs/tda998x.c > > diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig > index 22c7ed6..24fc975 100644 > --- a/drivers/gpu/drm/i2c/Kconfig > +++ b/drivers/gpu/drm/i2c/Kconfig > @@ -28,6 +28,7 @@ config DRM_I2C_SIL164 > config DRM_I2C_NXP_TDA998X > tristate "NXP Semiconductors TDA998X HDMI encoder" > default m if DRM_TILCDC > + select SND_SOC_TDA998X > help > Support for NXP Semiconductors TDA998X HDMI encoders. > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > index ce1478d..a26a516 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > @@ -27,6 +27,11 @@ > #include <drm/drm_encoder_slave.h> > #include <drm/drm_edid.h> > #include <drm/i2c/tda998x.h> > +#include <sound/tda998x.h> > + > +#if defined(CONFIG_SND_SOC_TDA998X) || defined(CONFIG_SND_SOC_TDA998X_MODULE) > +#define WITH_CODEC 1 > +#endif Why not simply #if IS_ENABLED(CONFIG_SND_SOC_TDA998X), no need for WITH_CODEC at all IMO. > > #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__) > > @@ -47,6 +52,10 @@ struct tda998x_priv { > struct drm_encoder *encoder; > > u8 audio_ports[2]; > +#ifdef WITH_CODEC > + u8 sad[3]; /* Short Audio Descriptor */ Why not use struct cea_sad and friends from drm/drm_edid.h ? > + struct snd_pcm_hw_constraint_list rate_constraints; > +#endif > }; > > #define to_tda998x_priv(x) ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv) > @@ -730,11 +739,109 @@ tda998x_configure_audio(struct tda998x_priv *priv, > tda998x_write_aif(priv, p); > } > > +#ifdef WITH_CODEC > +/* tda998x audio codec interface */ > + > +/* return the audio parameters extracted from the last EDID */ > +static int tda998x_get_audio_var(struct device *dev, > + u8 **sad, See comment above about struct cea_sad. > + struct snd_pcm_hw_constraint_list **rate_constraints) > +{ > + struct tda998x_priv *priv = dev_get_drvdata(dev); > + > + if (!priv->encoder->crtc > + || !priv->is_hdmi_sink) > + return -ENODEV; > + > + *sad = priv->sad; > + *rate_constraints = &priv->rate_constraints; > + return 0; > +} > + > +/* switch the audio port and initialize the audio parameters for streaming */ > +static int tda998x_set_audio_input(struct device *dev, > + int port_index, > + unsigned sample_rate, > + int sample_format) > +{ > + struct tda998x_priv *priv = dev_get_drvdata(dev); > + struct tda998x_encoder_params *p = &priv->params; > + > + if (!priv->encoder->crtc) > + return -ENODEV; > + > + /* if no port, just disable the audio port */ > + if (port_index == PORT_NONE) { > + reg_write(priv, REG_ENA_AP, 0); > + return 0; > + } > + > + /* if same audio parameters, just enable the audio port */ > + if (p->audio_cfg == priv->audio_ports[port_index] && > + p->audio_sample_rate == sample_rate) { > + reg_write(priv, REG_ENA_AP, p->audio_cfg); > + return 0; > + } > + > + p->audio_format = port_index == PORT_SPDIF ? AFMT_SPDIF : AFMT_I2S; > + p->audio_clk_cfg = port_index == PORT_SPDIF ? 0 : 1; > + p->audio_cfg = priv->audio_ports[port_index]; > + p->audio_sample_rate = sample_rate; > + tda998x_configure_audio(priv, &priv->encoder->crtc->hwmode, p); > + return 0; > +} > + > +/* get the audio capabilities from the EDID */ > +static void tda998x_get_audio_caps(struct tda998x_priv *priv, > + struct drm_connector *connector) > +{ See comment above about struct cea_sad. > + u8 *eld = connector->eld; > + u8 *sad; > + int sad_count; > + unsigned eld_ver, mnl; > + u8 max_channels, rate_mask, fmt; > + > + /* adjust the hw params from the ELD (EDID) */ > + eld_ver = eld[0] >> 3; > + if (eld_ver != 2 && eld_ver != 31) > + return; > + > + mnl = eld[4] & 0x1f; > + if (mnl > 16) > + return; > + > + sad_count = eld[5] >> 4; > + sad = eld + 20 + mnl; > + > + /* Start from the basic audio settings */ > + max_channels = 1; > + rate_mask = 0; > + fmt = 0; > + while (sad_count--) { > + switch (sad[0] & 0x78) { > + case 0x08: /* SAD uncompressed audio */ > + if ((sad[0] & 7) > max_channels) > + max_channels = sad[0] & 7; > + rate_mask |= sad[1]; > + fmt |= sad[2] & 0x07; > + break; > + } > + sad += 3; > + } > + > + priv->sad[0] = max_channels; > + priv->sad[1] = rate_mask; > + priv->sad[2] = fmt; > +} > +#endif /* WITH_CODEC */ > + > /* DRM encoder functions */ > > static void tda998x_encoder_set_config(struct tda998x_priv *priv, > const struct tda998x_encoder_params *p) > { > + int port_index; > + > priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) | > (p->mirr_a ? VIP_CNTRL_0_MIRR_A : 0) | > VIP_CNTRL_0_SWAP_B(p->swap_b) | > @@ -749,6 +856,8 @@ static void tda998x_encoder_set_config(struct tda998x_priv *priv, > (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0); > > priv->params = *p; > + port_index = p->audio_format == AFMT_SPDIF ? PORT_SPDIF : PORT_I2S; > + priv->audio_ports[port_index] = p->audio_cfg; > } > > static void tda998x_encoder_dpms(struct tda998x_priv *priv, int mode) > @@ -1142,6 +1251,15 @@ tda998x_encoder_get_modes(struct tda998x_priv *priv, > drm_mode_connector_update_edid_property(connector, edid); > n = drm_add_edid_modes(connector, edid); > priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid); > + > +#ifdef WITH_CODEC > + /* set the audio parameters from the EDID */ > + if (priv->is_hdmi_sink) { > + drm_edid_to_eld(connector, edid); > + tda998x_get_audio_caps(priv, connector); > + } > +#endif > + > kfree(edid); > } > > @@ -1176,6 +1294,10 @@ static void tda998x_destroy(struct tda998x_priv *priv) > if (priv->hdmi->irq) > free_irq(priv->hdmi->irq, priv); > > +#ifdef WITH_CODEC > + tda9998x_codec_unregister(&priv->hdmi->dev); > +#endif > + > i2c_unregister_device(priv->cec); > } > > @@ -1384,26 +1506,33 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) > break; > } > if (strcmp(p, "spdif") == 0) { > - priv->audio_ports[0] = port; > + priv->audio_ports[PORT_SPDIF] = port; > } else if (strcmp(p, "i2s") == 0) { > - priv->audio_ports[1] = port; > + priv->audio_ports[PORT_I2S] = port; > } else { > dev_err(&client->dev, > "bad audio-port-names '%s'\n", p); > break; > } > } > - if (priv->audio_ports[0]) { > - priv->params.audio_cfg = priv->audio_ports[0]; > + if (priv->audio_ports[PORT_SPDIF]) { > + priv->params.audio_cfg = priv->audio_ports[PORT_SPDIF]; > priv->params.audio_format = AFMT_SPDIF; > priv->params.audio_clk_cfg = 0; > } else { > - priv->params.audio_cfg = priv->audio_ports[1]; > + priv->params.audio_cfg = priv->audio_ports[PORT_I2S]; > priv->params.audio_format = AFMT_I2S; > priv->params.audio_clk_cfg = 1; > } > } > > +#ifdef WITH_CODEC > + /* create the audio CODEC */ > + if (priv->audio_ports[PORT_SPDIF] || priv->audio_ports[PORT_I2S]) > + tda9998x_codec_register(&client->dev, > + tda998x_get_audio_var, > + tda998x_set_audio_input); > +#endif > return 0; > > fail: > diff --git a/include/sound/tda998x.h b/include/sound/tda998x.h > new file mode 100644 > index 0000000..97cff30 > --- /dev/null > +++ b/include/sound/tda998x.h > @@ -0,0 +1,23 @@ > +#ifndef SND_TDA998X_H > +#define SND_TDA998X_H > + > +#include <sound/pcm.h> > + > +/* port indexes */ > +#define PORT_NONE (-1) > +#define PORT_SPDIF 0 > +#define PORT_I2S 1 > + > +typedef int (*get_audio_var_t)(struct device *dev, > + u8 **sad, See comment above about struct cea_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); > + > +int tda9998x_codec_register(struct device *dev, > + get_audio_var_t get_audio_var_i, > + set_audio_input_t set_audio_input_i); > +void tda9998x_codec_unregister(struct device *dev); > +#endif > diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig > index 8349f98..456c549 100644 > --- a/sound/soc/codecs/Kconfig > +++ b/sound/soc/codecs/Kconfig > @@ -102,6 +102,7 @@ config SND_SOC_ALL_CODECS > select SND_SOC_STAC9766 if SND_SOC_AC97_BUS > select SND_SOC_TAS2552 if I2C > select SND_SOC_TAS5086 if I2C > + select SND_SOC_TDA998X if DRM_I2C_NXP_TDA998X > select SND_SOC_TFA9879 if I2C > select SND_SOC_TLV320AIC23_I2C if I2C > select SND_SOC_TLV320AIC23_SPI if SPI_MASTER > @@ -600,6 +601,9 @@ config SND_SOC_TAS5086 > tristate "Texas Instruments TAS5086 speaker amplifier" > depends on I2C > > +config SND_SOC_TDA998X > + tristate > + > config SND_SOC_TFA9879 > tristate "NXP Semiconductors TFA9879 amplifier" > depends on I2C > diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile > index bbdfd1e..44b4fda 100644 > --- a/sound/soc/codecs/Makefile > +++ b/sound/soc/codecs/Makefile > @@ -104,6 +104,7 @@ snd-soc-sta350-objs := sta350.o > snd-soc-sta529-objs := sta529.o > snd-soc-stac9766-objs := stac9766.o > snd-soc-tas5086-objs := tas5086.o > +snd-soc-tda998x-objs := tda998x.o > snd-soc-tfa9879-objs := tfa9879.o > snd-soc-tlv320aic23-objs := tlv320aic23.o > snd-soc-tlv320aic23-i2c-objs := tlv320aic23-i2c.o > @@ -282,6 +283,7 @@ obj-$(CONFIG_SND_SOC_STA529) += snd-soc-sta529.o > obj-$(CONFIG_SND_SOC_STAC9766) += snd-soc-stac9766.o > obj-$(CONFIG_SND_SOC_TAS2552) += snd-soc-tas2552.o > obj-$(CONFIG_SND_SOC_TAS5086) += snd-soc-tas5086.o > +obj-$(CONFIG_SND_SOC_TDA998X) += snd-soc-tda998x.o > obj-$(CONFIG_SND_SOC_TFA9879) += snd-soc-tfa9879.o > obj-$(CONFIG_SND_SOC_TLV320AIC23) += snd-soc-tlv320aic23.o > obj-$(CONFIG_SND_SOC_TLV320AIC23_I2C) += snd-soc-tlv320aic23-i2c.o > diff --git a/sound/soc/codecs/tda998x.c b/sound/soc/codecs/tda998x.c > new file mode 100644 > index 0000000..b055570 > --- /dev/null > +++ b/sound/soc/codecs/tda998x.c > @@ -0,0 +1,175 @@ > +/* > + * ALSA SoC TDA998x CODEC > + * > + * Copyright (C) 2015 Jean-Francois Moine > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/module.h> > +#include <sound/soc.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <sound/pcm_params.h> > +#include <sound/tda998x.h> > + > +/* functions in tda998x_drv */ > +static get_audio_var_t get_audio_var; > +static set_audio_input_t set_audio_input; > + > +static int tda998x_codec_startup(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct snd_pcm_runtime *runtime = substream->runtime; > + struct snd_pcm_hw_constraint_list *rate_constraints; > + u8 *sad; /* Short Audio Descriptor (3 bytes) */ > +#define SAD_MX_CHAN_I 0 /* max number of chennels - 1 */ > +#define SAD_RATE_MASK_I 1 /* rate mask */ > +#define SAD_FMT_I 2 /* formats */ > +#define SAD_FMT_16 0x01 > +#define SAD_FMT_20 0x02 > +#define SAD_FMT_24 0x04 See comment above about struct cea_sad. > + u64 formats; > + int ret; > + static const u32 hdmi_rates[] = { > + 32000, 44100, 48000, 88200, 96000, 176400, 192000 > + }; > + > + /* get the EDID values and the rate constraints buffer */ > + ret = get_audio_var(dai->dev, &sad, &rate_constraints); > + if (ret < 0) > + return ret; /* no screen */ > + > + /* convert the EDID values to audio constraints */ > + rate_constraints->list = hdmi_rates; > + rate_constraints->count = ARRAY_SIZE(hdmi_rates); > + rate_constraints->mask = sad[SAD_RATE_MASK_I]; > + snd_pcm_hw_constraint_list(runtime, 0, > + SNDRV_PCM_HW_PARAM_RATE, > + rate_constraints); > + > + formats = 0; > + if (sad[SAD_FMT_I] & SAD_FMT_16) > + formats |= SNDRV_PCM_FMTBIT_S16_LE; > + if (sad[SAD_FMT_I] & SAD_FMT_20) > + formats |= SNDRV_PCM_FMTBIT_S20_3LE; > + if (sad[SAD_FMT_I] & SAD_FMT_24) > + formats |= SNDRV_PCM_FMTBIT_S24_LE | > + SNDRV_PCM_FMTBIT_S24_3LE | > + SNDRV_PCM_FMTBIT_S32_LE; > + snd_pcm_hw_constraint_mask64(runtime, > + SNDRV_PCM_HW_PARAM_FORMAT, > + formats); > + > + snd_pcm_hw_constraint_minmax(runtime, > + SNDRV_PCM_HW_PARAM_CHANNELS, > + 1, sad[SAD_MX_CHAN_I]); > + return 0; > +} > + > +static int tda998x_codec_hw_params(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *params, > + struct snd_soc_dai *dai) > +{ > + return set_audio_input(dai->dev, dai->id, > + params_rate(params), > + params_format(params)); > +} > + > +static void tda998x_codec_shutdown(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + set_audio_input(dai->dev, PORT_NONE, 0, 0); > +} > + > +static const struct snd_soc_dai_ops tda998x_codec_ops = { > + .startup = tda998x_codec_startup, > + .hw_params = tda998x_codec_hw_params, > + .shutdown = tda998x_codec_shutdown, > +}; > + > +#define TDA998X_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \ > + SNDRV_PCM_FMTBIT_S20_3LE | \ > + SNDRV_PCM_FMTBIT_S24_LE | \ > + SNDRV_PCM_FMTBIT_S32_LE) > + > +static struct snd_soc_dai_driver tda998x_dais[] = { > + { > + .name = "spdif-hifi", > + .id = PORT_SPDIF, > + .playback = { > + .stream_name = "HDMI SPDIF Playback", > + .channels_min = 1, > + .channels_max = 2, > + .rates = SNDRV_PCM_RATE_CONTINUOUS, > + .rate_min = 22050, > + .rate_max = 192000, > + .formats = TDA998X_FORMATS, > + }, > + .ops = &tda998x_codec_ops, > + }, > + { > + .name = "i2s-hifi", > + .id = PORT_I2S, > + .playback = { > + .stream_name = "HDMI I2S Playback", > + .channels_min = 1, > + .channels_max = 8, > + .rates = SNDRV_PCM_RATE_CONTINUOUS, > + .rate_min = 5512, > + .rate_max = 192000, > + .formats = TDA998X_FORMATS, > + }, > + .ops = &tda998x_codec_ops, > + }, > +}; > + > +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)); > +} > +EXPORT_SYMBOL(tda9998x_codec_register); > + > +void tda9998x_codec_unregister(struct device *dev) > +{ > + snd_soc_unregister_codec(dev); > +} > +EXPORT_SYMBOL(tda9998x_codec_unregister); > + > +static int __init tda998x_codec_init(void) > +{ > + return 0; > +} > +static void __exit tda998x_codec_exit(void) > +{ > +} > +module_init(tda998x_codec_init); > +module_exit(tda998x_codec_exit); > + > +MODULE_AUTHOR("Jean-Francois Moine <moinejf@free.fr>"); > +MODULE_DESCRIPTION("TDA998X CODEC"); > +MODULE_LICENSE("GPL"); >
On 01/07/2015 08:02 PM, Jean-Francois Moine wrote: > On Wed, 7 Jan 2015 15:41:38 +0000 > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > >> On Wed, Jan 07, 2015 at 03:10:47PM +0000, Andrew Jackson wrote: >>> On 01/07/15 10:51, Jean-Francois Moine wrote: >>>> diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig >>>> index 22c7ed6..24fc975 100644 >>>> --- a/drivers/gpu/drm/i2c/Kconfig >>>> +++ b/drivers/gpu/drm/i2c/Kconfig >>>> @@ -28,6 +28,7 @@ config DRM_I2C_SIL164 >>>> config DRM_I2C_NXP_TDA998X >>>> tristate "NXP Semiconductors TDA998X HDMI encoder" >>>> default m if DRM_TILCDC >>>> + select SND_SOC_TDA998X >>> >>> Do you need this since sound/soc/codecs/Kconfig conditionally brings in the >>> CODEC driver? > > For SND_SOC_ALL_CODECS only. > >> More importantly, it's broken, because it'll cause Kconfig to complain >> if you enable DRM_I2C_NXP_TDA998X, but have ASoC disabled. >> >> Moreover, if you decide you want the sound and ASoC built as a module, >> but want to build in DRM and TDA998x support (so you can get video >> output working on boot before initramfs/rootfs), Kconfig may well >> complain. > > select SND_SOC_TDA998X if SND_SOC > > should work in all cases. > I think that would still fail if DRM and TDA998x is built in and SND_SOC is built as modules. A request_module() call before tda9998x_codec_register() should help. Or could could write: select SND_SOC_TDA998X if (SND_SOC=DRM_I2C_NXP_TDA998X || SND_SOC=y) >>>> +static int __init tda998x_codec_init(void) >>>> +{ >>>> + return 0; >>>> +} >>>> +static void __exit tda998x_codec_exit(void) >>>> +{ >>>> +} >>>> +module_init(tda998x_codec_init); >>>> +module_exit(tda998x_codec_exit); >> >> There's no need for these if they remain as the above. Modules can have >> both init/exit functions, or neither, and they are still unloadable. >> Only modules which provide only an init function get locked in on load. > > Thanks. >
On Fri, 9 Jan 2015 12:24:12 +0200 Jyri Sarha <jsarha@ti.com> wrote: > > select SND_SOC_TDA998X if SND_SOC > > > > should work in all cases. > > > > I think that would still fail if DRM and TDA998x is built in and SND_SOC > is built as modules. A request_module() call before > tda9998x_codec_register() should help. Or could could write: > > select SND_SOC_TDA998X if (SND_SOC=DRM_I2C_NXP_TDA998X || SND_SOC=y) request_module() is not needed because there are functions calls from the transmitter to the codec. With the simple "if SND_SOC", make menuconfig refuses to have TDA998x built in and SND_SOC built as modules, as it should.
On Fri, Jan 09, 2015 at 12:24:12PM +0200, Jyri Sarha wrote: > I think that would still fail if DRM and TDA998x is built in and SND_SOC is > built as modules. A request_module() call before tda9998x_codec_register() > should help. That doesn't fix the problem. If the DRM driver is built in, but the codec is not, and the DRM driver has a reference to tda9998x_codec_register(), then the vmlinux file will fail to link, and you'll never get the opportunity to call request_module(). > Or could could write: > > select SND_SOC_TDA998X if (SND_SOC=DRM_I2C_NXP_TDA998X || SND_SOC=y) I'm not sure that's right either. Let's go back and think about this: why should SND_SOC_TDA998X be *selected*. Let me put that a different way: why should this symbol be forced on just because we're building the DRM TDA998x driver? Would it be more sensible to make SND_SOC_TDA998X depend on DRM_I2C_NXP_TDA998X instead, maybe with a 'default y' - which is a kinder way to have SND_SOC_TDA998X be enabled. If SND_SOC_TDA998X doesn't have a prompt, then it'll automatically enable itself too this way when all its dependencies are satisfied. IMHO "select" is a very over-used, and in many cases an evil construct because its very hard to avoid breaking dependencies with it.
On Fri, 9 Jan 2015 11:19:35 +0000 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > Would it be more sensible to make SND_SOC_TDA998X depend on > DRM_I2C_NXP_TDA998X instead, maybe with a 'default y' - which is a > kinder way to have SND_SOC_TDA998X be enabled. If SND_SOC_TDA998X > doesn't have a prompt, then it'll automatically enable itself too > this way when all its dependencies are satisfied. You are right, I will set back the way I had in the version 3: > diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig > index b33b45d..747e387 100644 > --- a/sound/soc/codecs/Kconfig > +++ b/sound/soc/codecs/Kconfig > @@ -352,6 +352,12 @@ config SND_SOC_STAC9766 > config SND_SOC_TAS5086 > tristate > > +config SND_SOC_TDA998X > + tristate > + default y if DRM_I2C_NXP_TDA998X=y > + default m if DRM_I2C_NXP_TDA998X=m > +
On Fri, Jan 09, 2015 at 12:45:31PM +0100, Jean-Francois Moine wrote: > On Fri, 9 Jan 2015 11:19:35 +0000 > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > > > Would it be more sensible to make SND_SOC_TDA998X depend on > > DRM_I2C_NXP_TDA998X instead, maybe with a 'default y' - which is a > > kinder way to have SND_SOC_TDA998X be enabled. If SND_SOC_TDA998X > > doesn't have a prompt, then it'll automatically enable itself too > > this way when all its dependencies are satisfied. > > You are right, I will set back the way I had in the version 3: > > > diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig > > index b33b45d..747e387 100644 > > --- a/sound/soc/codecs/Kconfig > > +++ b/sound/soc/codecs/Kconfig > > @@ -352,6 +352,12 @@ config SND_SOC_STAC9766 > > config SND_SOC_TAS5086 > > tristate > > > > +config SND_SOC_TDA998X > > + tristate > > + default y if DRM_I2C_NXP_TDA998X=y > > + default m if DRM_I2C_NXP_TDA998X=m > > + An easier way to write this is: config SND_SOC_TDA998X def_tristate y depends on DRM_I2C_NXP_TDA998X That's because internally, Kconfig knows how to do ternary operations.
On 01/07/15 10:51, Jean-Francois Moine wrote: > This patch adds a CODEC to the NXP TDA998x HDMI transmitter. > > The CODEC handles both I2S and S/PDIF inputs. > It maintains the audio format and rate constraints according > to the HDMI device parameters (EDID) and sets dynamically the input > ports in the TDA998x I2C driver on start/stop audio streaming. > > Signed-off-by: Jean-Francois Moine <moinejf@free.fr> > --- [snip] > +static int tda998x_codec_startup(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct snd_pcm_runtime *runtime = substream->runtime; > + struct snd_pcm_hw_constraint_list *rate_constraints; > + u8 *sad; /* Short Audio Descriptor (3 bytes) */ [...] > + snd_pcm_hw_constraint_minmax(runtime, > + SNDRV_PCM_HW_PARAM_CHANNELS, > + 1, sad[SAD_MX_CHAN_I]); In the light of our discussions elsewhere [1], shouldn't this be constrained by the number of hardware channels that the TDA998x supports too? That is, the maximum number of channels should be the lesser of sd[SAD_MX_CHAN_I] and number_of_I2S channels (or S/PDIF channels if so configured). Andrew [1] http://mailman.alsa-project.org/pipermail/alsa-devel/2015-January/086090.html
On Fri, Jan 09, 2015 at 05:39:08PM +0000, Andrew Jackson wrote: > In the light of our discussions elsewhere [1], shouldn't this > be constrained by the number of hardware channels that the TDA998x > supports too? That is, the maximum number of channels should > be the lesser of sd[SAD_MX_CHAN_I] and number_of_I2S channels > (or S/PDIF channels if so configured). Yes, that's a good idea in general - we should do more of this.
Some more comments based on my - finally successful - attempt to use this code with BBB HDMI audio. On 01/07/2015 12:51 PM, Jean-Francois Moine wrote: > This patch adds a CODEC to the NXP TDA998x HDMI transmitter. > > The CODEC handles both I2S and S/PDIF inputs. > It maintains the audio format and rate constraints according > to the HDMI device parameters (EDID) and sets dynamically the input > ports in the TDA998x I2C driver on start/stop audio streaming. > > Signed-off-by: Jean-Francois Moine <moinejf@free.fr> > --- > drivers/gpu/drm/i2c/Kconfig | 1 + > drivers/gpu/drm/i2c/tda998x_drv.c | 139 ++++++++++++++++++++++++++++-- > include/sound/tda998x.h | 23 +++++ > sound/soc/codecs/Kconfig | 4 + > sound/soc/codecs/Makefile | 2 + > sound/soc/codecs/tda998x.c | 175 ++++++++++++++++++++++++++++++++++++++ > 6 files changed, 339 insertions(+), 5 deletions(-) > create mode 100644 include/sound/tda998x.h > create mode 100644 sound/soc/codecs/tda998x.c > > diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig > index 22c7ed6..24fc975 100644 > --- a/drivers/gpu/drm/i2c/Kconfig > +++ b/drivers/gpu/drm/i2c/Kconfig > @@ -28,6 +28,7 @@ config DRM_I2C_SIL164 > config DRM_I2C_NXP_TDA998X > tristate "NXP Semiconductors TDA998X HDMI encoder" > default m if DRM_TILCDC > + select SND_SOC_TDA998X > help > Support for NXP Semiconductors TDA998X HDMI encoders. > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > index ce1478d..a26a516 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > @@ -27,6 +27,11 @@ > #include <drm/drm_encoder_slave.h> > #include <drm/drm_edid.h> > #include <drm/i2c/tda998x.h> > +#include <sound/tda998x.h> > + > +#if defined(CONFIG_SND_SOC_TDA998X) || defined(CONFIG_SND_SOC_TDA998X_MODULE) > +#define WITH_CODEC 1 > +#endif > > #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__) > > @@ -47,6 +52,10 @@ struct tda998x_priv { > struct drm_encoder *encoder; > > u8 audio_ports[2]; > +#ifdef WITH_CODEC > + u8 sad[3]; /* Short Audio Descriptor */ > + struct snd_pcm_hw_constraint_list rate_constraints; > +#endif It feels a bit backwards to me to store these audio side variables here in DRM side driver - especially the rate_constraint - and provide a function (tda998x_get_audio_var()) for getting their individual addresses to ASoC side. Wouldn't it be more straight forward to provide functions for setting and getting the audio side data from a void pointer in DRM side device drvdata? > }; > > #define to_tda998x_priv(x) ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv) > @@ -730,11 +739,109 @@ tda998x_configure_audio(struct tda998x_priv *priv, > tda998x_write_aif(priv, p); > } > > +#ifdef WITH_CODEC > +/* tda998x audio codec interface */ > + > +/* return the audio parameters extracted from the last EDID */ > +static int tda998x_get_audio_var(struct device *dev, > + u8 **sad, > + struct snd_pcm_hw_constraint_list **rate_constraints) > +{ > + struct tda998x_priv *priv = dev_get_drvdata(dev); > + > + if (!priv->encoder->crtc > + || !priv->is_hdmi_sink) > + return -ENODEV; > + > + *sad = priv->sad; > + *rate_constraints = &priv->rate_constraints; > + return 0; > +} > + > +/* switch the audio port and initialize the audio parameters for streaming */ > +static int tda998x_set_audio_input(struct device *dev, > + int port_index, > + unsigned sample_rate, > + int sample_format) > +{ > + struct tda998x_priv *priv = dev_get_drvdata(dev); > + struct tda998x_encoder_params *p = &priv->params; > + > + if (!priv->encoder->crtc) > + return -ENODEV; > + > + /* if no port, just disable the audio port */ > + if (port_index == PORT_NONE) { > + reg_write(priv, REG_ENA_AP, 0); > + return 0; > + } > + > + /* if same audio parameters, just enable the audio port */ > + if (p->audio_cfg == priv->audio_ports[port_index] && > + p->audio_sample_rate == sample_rate) { > + reg_write(priv, REG_ENA_AP, p->audio_cfg); > + return 0; > + } > + > + p->audio_format = port_index == PORT_SPDIF ? AFMT_SPDIF : AFMT_I2S; > + p->audio_clk_cfg = port_index == PORT_SPDIF ? 0 : 1; > + p->audio_cfg = priv->audio_ports[port_index]; > + p->audio_sample_rate = sample_rate; > + tda998x_configure_audio(priv, &priv->encoder->crtc->hwmode, p); > + return 0; > +} > + > +/* get the audio capabilities from the EDID */ > +static void tda998x_get_audio_caps(struct tda998x_priv *priv, > + struct drm_connector *connector) > +{ > + u8 *eld = connector->eld; > + u8 *sad; > + int sad_count; > + unsigned eld_ver, mnl; > + u8 max_channels, rate_mask, fmt; > + > + /* adjust the hw params from the ELD (EDID) */ > + eld_ver = eld[0] >> 3; > + if (eld_ver != 2 && eld_ver != 31) > + return; > + > + mnl = eld[4] & 0x1f; > + if (mnl > 16) > + return; > + > + sad_count = eld[5] >> 4; > + sad = eld + 20 + mnl; > +SNDRV_PCM_RATE_CONTINUOUS > + /* Start from the basic audio settings */ > + max_channels = 1; > + rate_mask = 0; > + fmt = 0; > + while (sad_count--) { > + switch (sad[0] & 0x78) { > + case 0x08: /* SAD uncompressed audio */ > + if ((sad[0] & 7) > max_channels) > + max_channels = sad[0] & 7; > + rate_mask |= sad[1]; > + fmt |= sad[2] & 0x07; > + break; > + } > + sad += 3; > + } > + > + priv->sad[0] = max_channels; > + priv->sad[1] = rate_mask; > + priv->sad[2] = fmt; > +} > +#endif /* WITH_CODEC */ > + > /* DRM encoder functions */ > > static void tda998x_encoder_set_config(struct tda998x_priv *priv, > const struct tda998x_encoder_params *p) > { > + int port_index; > + > priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) | > (p->mirr_a ? VIP_CNTRL_0_MIRR_A : 0) | > VIP_CNTRL_0_SWAP_B(p->swap_b) | > @@ -749,6 +856,8 @@ static void tda998x_encoder_set_config(struct tda998x_priv *priv, > (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0); > > priv->params = *p; > + port_index = p->audio_format == AFMT_SPDIF ? PORT_SPDIF : PORT_I2S; > + priv->audio_ports[port_index] = p->audio_cfg; > } > > static void tda998x_encoder_dpms(struct tda998x_priv *priv, int mode) > @@ -1142,6 +1251,15 @@ tda998x_encoder_get_modes(struct tda998x_priv *priv, > drm_mode_connector_update_edid_property(connector, edid); > n = drm_add_edid_modes(connector, edid); > priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid); > + > +#ifdef WITH_CODEC > + /* set the audio parameters from the EDID */ > + if (priv->is_hdmi_sink) { > + drm_edid_to_eld(connector, edid); > + tda998x_get_audio_caps(priv, connector); > + } > +#endif > + > kfree(edid); > } > > @@ -1176,6 +1294,10 @@ static void tda998x_destroy(struct tda998x_priv *priv) > if (priv->hdmi->irq) > free_irq(priv->hdmi->irq, priv); > > +#ifdef WITH_CODEC > + tda9998x_codec_unregister(&priv->hdmi->dev); > +#endif > + > i2c_unregister_device(priv->cec); > } > > @@ -1384,26 +1506,33 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) > break; > } > if (strcmp(p, "spdif") == 0) { > - priv->audio_ports[0] = port; > + priv->audio_ports[PORT_SPDIF] = port; > } else if (strcmp(p, "i2s") == 0) { > - priv->audio_ports[1] = port; > + priv->audio_ports[PORT_I2S] = port; > } else { > dev_err(&client->dev, > "bad audio-port-names '%s'\n", p); > break; > } > } > - if (priv->audio_ports[0]) { > - priv->params.audio_cfg = priv->audio_ports[0]; > + if (priv->audio_ports[PORT_SPDIF]) { > + priv->params.audio_cfg = priv->audio_ports[PORT_SPDIF]; > priv->params.audio_format = AFMT_SPDIF; > priv->params.audio_clk_cfg = 0; > } else { > - priv->params.audio_cfg = priv->audio_ports[1]; > + priv->params.audio_cfg = priv->audio_ports[PORT_I2S]; > priv->params.audio_format = AFMT_I2S; > priv->params.audio_clk_cfg = 1; > } > } > > +#ifdef WITH_CODEC > + /* create the audio CODEC */ > + if (priv->audio_ports[PORT_SPDIF] || priv->audio_ports[PORT_I2S]) > + tda9998x_codec_register(&client->dev, > + tda998x_get_audio_var, > + tda998x_set_audio_input); > +#endif > return 0; > > fail: > diff --git a/include/sound/tda998x.h b/include/sound/tda998x.h > new file mode 100644 > index 0000000..97cff30 > --- /dev/null > +++ b/include/sound/tda998x.h > @@ -0,0 +1,23 @@ > +#ifndef SND_TDA998X_H > +#define SND_TDA998X_H > + > +#include <sound/pcm.h> > + > +/* port indexes */ > +#define PORT_NONE (-1) > +#define PORT_SPDIF 0 > +#define PORT_I2S 1 > + > +typedef int (*get_audio_var_t)(struct device *dev, > + u8 **sad, > + struct snd_pcm_hw_constraint_list **rate_constraints); > +typedef int (*set_audio_input_t)(struct device *dev, > + int port_index, > + unsigned sample_rate, > + int sample_format); > + Why don't you simply declare tda998x_get_audio_var() and tda998x_set_audio_input() here and export them in DRM side driver? This is a tda998x specific codec after all. I see no point to this this function pointer typedef game, when the pointers are anyway stored to global variables in ASoC the side module. > +int tda9998x_codec_register(struct device *dev, > + get_audio_var_t get_audio_var_i, > + set_audio_input_t set_audio_input_i); > +void tda9998x_codec_unregister(struct device *dev); > +#endif > diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig > index 8349f98..456c549 100644 > --- a/sound/soc/codecs/Kconfig > +++ b/sound/soc/codecs/Kconfig > @@ -102,6 +102,7 @@ config SND_SOC_ALL_CODECS > select SND_SOC_STAC9766 if SND_SOC_AC97_BUS > select SND_SOC_TAS2552 if I2C > select SND_SOC_TAS5086 if I2C > + select SND_SOC_TDA998X if DRM_I2C_NXP_TDA998X > select SND_SOC_TFA9879 if I2C > select SND_SOC_TLV320AIC23_I2C if I2C > select SND_SOC_TLV320AIC23_SPI if SPI_MASTER > @@ -600,6 +601,9 @@ config SND_SOC_TAS5086 > tristate "Texas Instruments TAS5086 speaker amplifier" > depends on I2C > > +config SND_SOC_TDA998X > + tristate > + > config SND_SOC_TFA9879 > tristate "NXP Semiconductors TFA9879 amplifier" > depends on I2C > diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile > index bbdfd1e..44b4fda 100644 > --- a/sound/soc/codecs/Makefile > +++ b/sound/soc/codecs/Makefile > @@ -104,6 +104,7 @@ snd-soc-sta350-objs := sta350.o > snd-soc-sta529-objs := sta529.o > snd-soc-stac9766-objs := stac9766.o > snd-soc-tas5086-objs := tas5086.o > +snd-soc-tda998x-objs := tda998x.o > snd-soc-tfa9879-objs := tfa9879.o > snd-soc-tlv320aic23-objs := tlv320aic23.o > snd-soc-tlv320aic23-i2c-objs := tlv320aic23-i2c.o > @@ -282,6 +283,7 @@ obj-$(CONFIG_SND_SOC_STA529) += snd-soc-sta529.o > obj-$(CONFIG_SND_SOC_STAC9766) += snd-soc-stac9766.o > obj-$(CONFIG_SND_SOC_TAS2552) += snd-soc-tas2552.o > obj-$(CONFIG_SND_SOC_TAS5086) += snd-soc-tas5086.o > +obj-$(CONFIG_SND_SOC_TDA998X) += snd-soc-tda998x.o > obj-$(CONFIG_SND_SOC_TFA9879) += snd-soc-tfa9879.o > obj-$(CONFIG_SND_SOC_TLV320AIC23) += snd-soc-tlv320aic23.o > obj-$(CONFIG_SND_SOC_TLV320AIC23_I2C) += snd-soc-tlv320aic23-i2c.o > diff --git a/sound/soc/codecs/tda998x.c b/sound/soc/codecs/tda998x.c > new file mode 100644 > index 0000000..b055570 > --- /dev/null > +++ b/sound/soc/codecs/tda998x.c > @@ -0,0 +1,175 @@ > +/* > + * ALSA SoC TDA998x CODEC > + * > + * Copyright (C) 2015 Jean-Francois Moine > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <linux/module.h> > +#include <sound/soc.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <sound/pcm_params.h> > +#include <sound/tda998x.h> > + > +/* functions in tda998x_drv */ > +static get_audio_var_t get_audio_var; > +static set_audio_input_t set_audio_input; > + > +static int tda998x_codec_startup(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + struct snd_pcm_runtime *runtime = substream->runtime; > + struct snd_pcm_hw_constraint_list *rate_constraints; > + u8 *sad; /* Short Audio Descriptor (3 bytes) */ > +#define SAD_MX_CHAN_I 0 /* max number of chennels - 1 */ > +#define SAD_RATE_MASK_I 1 /* rate mask */ > +#define SAD_FMT_I 2 /* formats */ > +#define SAD_FMT_16 0x01 > +#define SAD_FMT_20 0x02 > +#define SAD_FMT_24 0x04 > + u64 formats; > + int ret; > + static const u32 hdmi_rates[] = { > + 32000, 44100, 48000, 88200, 96000, 176400, 192000 > + }; > + > + /* get the EDID values and the rate constraints buffer */ > + ret = get_audio_var(dai->dev, &sad, &rate_constraints); > + if (ret < 0) > + return ret; /* no screen */ > + > + /* convert the EDID values to audio constraints */ > + rate_constraints->list = hdmi_rates; > + rate_constraints->count = ARRAY_SIZE(hdmi_rates); > + rate_constraints->mask = sad[SAD_RATE_MASK_I]; > + snd_pcm_hw_constraint_list(runtime, 0, > + SNDRV_PCM_HW_PARAM_RATE, > + rate_constraints); > + > + formats = 0; > + if (sad[SAD_FMT_I] & SAD_FMT_16) > + formats |= SNDRV_PCM_FMTBIT_S16_LE; > + if (sad[SAD_FMT_I] & SAD_FMT_20) > + formats |= SNDRV_PCM_FMTBIT_S20_3LE; > + if (sad[SAD_FMT_I] & SAD_FMT_24) > + formats |= SNDRV_PCM_FMTBIT_S24_LE | > + SNDRV_PCM_FMTBIT_S24_3LE | > + SNDRV_PCM_FMTBIT_S32_LE; > + snd_pcm_hw_constraint_mask64(runtime, > + SNDRV_PCM_HW_PARAM_FORMAT, > + formats); > + > + snd_pcm_hw_constraint_minmax(runtime, > + SNDRV_PCM_HW_PARAM_CHANNELS, > + 1, sad[SAD_MX_CHAN_I]); SAD byte 1 bits 2-0 provides the maximum channel count _minus_one_. You should _add_one_ to sad[SAD_MX_CHAN_I] when setting the constraint. > + return 0; > +} > + > +static int tda998x_codec_hw_params(struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *params, > + struct snd_soc_dai *dai) > +{ > + return set_audio_input(dai->dev, dai->id, > + params_rate(params), > + params_format(params)); > +} > + > +static void tda998x_codec_shutdown(struct snd_pcm_substream *substream, > + struct snd_soc_dai *dai) > +{ > + set_audio_input(dai->dev, PORT_NONE, 0, 0); > +} > + > +static const struct snd_soc_dai_ops tda998x_codec_ops = { > + .startup = tda998x_codec_startup, > + .hw_params = tda998x_codec_hw_params, > + .shutdown = tda998x_codec_shutdown, > +}; > + > +#define TDA998X_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \ > + SNDRV_PCM_FMTBIT_S20_3LE | \ > + SNDRV_PCM_FMTBIT_S24_LE | \ > + SNDRV_PCM_FMTBIT_S32_LE) > + > +static struct snd_soc_dai_driver tda998x_dais[] = { > + { > + .name = "spdif-hifi", > + .id = PORT_SPDIF, > + .playback = { > + .stream_name = "HDMI SPDIF Playback", > + .channels_min = 1, > + .channels_max = 2, > + .rates = SNDRV_PCM_RATE_CONTINUOUS, > + .rate_min = 22050, > + .rate_max = 192000, > + .formats = TDA998X_FORMATS, > + }, > + .ops = &tda998x_codec_ops, > + }, > + { > + .name = "i2s-hifi", > + .id = PORT_I2S, > + .playback = { > + .stream_name = "HDMI I2S Playback", > + .channels_min = 1, > + .channels_max = 8, > + .rates = SNDRV_PCM_RATE_CONTINUOUS, > + .rate_min = 5512, > + .rate_max = 192000, > + .formats = TDA998X_FORMATS, > + }, > + .ops = &tda998x_codec_ops, > + }, > +}; Why do you use SNDRV_PCM_RATE_CONTINUOUS, when HDMI standard only supports 32000, 44100, 48000, 88200, 96000, 176400, and 192000 Hz-rates? > + > +static const struct snd_soc_dapm_widget tda998x_widgets[] = { > + SND_SOC_DAPM_OUTPUT("hdmi-out"), > +}; > +static const struct snd_soc_dapm_route tda998x_routes[] = { > + { "hdmi-out", NULL, "HDMI I2S Playback" }, > + { "hdmi-out", NULL, "HDMI SPDIF Playback" }, > +}; > + > +static struct snd_soc_codec_driver tda998x_codec_drv = { > + .dapm_widgets = tda998x_widgets, > + .num_dapm_widgets = ARRAY_SIZE(tda998x_widgets), > + .dapm_routes = tda998x_routes, > + .num_dapm_routes = ARRAY_SIZE(tda998x_routes), > + .ignore_pmdown_time = true, > +}; > + > +int tda9998x_codec_register(struct device *dev, > + get_audio_var_t get_audio_var_i, > + set_audio_input_t set_audio_input_i) > +{ > + get_audio_var = get_audio_var_i; > + set_audio_input = set_audio_input_i; > + return snd_soc_register_codec(dev, > + &tda998x_codec_drv, > + tda998x_dais, ARRAY_SIZE(tda998x_dais)); So this always registers a two DAI codec whether or not both the i2s and spdif is used. The DT binding document could state that the DAI index 0 is always the spdif DAI and index 1 is the i2s DAI, or even better you could #define them under include/dt-bindings/. While you are at it, you could also mention the DAPM output name "hdmi-out" for the codec in the DT-binding document. > +} > +EXPORT_SYMBOL(tda9998x_codec_register); > + > +void tda9998x_codec_unregister(struct device *dev) > +{ > + snd_soc_unregister_codec(dev); > +} > +EXPORT_SYMBOL(tda9998x_codec_unregister); > + > +static int __init tda998x_codec_init(void) > +{ > + return 0; > +} > +static void __exit tda998x_codec_exit(void) > +{ > +} > +module_init(tda998x_codec_init); > +module_exit(tda998x_codec_exit); > + > +MODULE_AUTHOR("Jean-Francois Moine <moinejf@free.fr>"); > +MODULE_DESCRIPTION("TDA998X CODEC"); > +MODULE_LICENSE("GPL"); >
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.
On Fri, 09 Jan 2015 17:39:08 +0000 Andrew Jackson <Andrew.Jackson@arm.com> wrote: > > + snd_pcm_hw_constraint_minmax(runtime, > > + SNDRV_PCM_HW_PARAM_CHANNELS, > > + 1, sad[SAD_MX_CHAN_I]); > > In the light of our discussions elsewhere [1], shouldn't this > be constrained by the number of hardware channels that the TDA998x > supports too? That is, the maximum number of channels should > be the lesser of sd[SAD_MX_CHAN_I] and number_of_I2S channels > (or S/PDIF channels if so configured). snd_pcm_hw_constraint_minmax() does the job from the min/max numbers of channels in the DAI definition and the real max number of channels will be set from audio ports declared in the DT.
diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig index 22c7ed6..24fc975 100644 --- a/drivers/gpu/drm/i2c/Kconfig +++ b/drivers/gpu/drm/i2c/Kconfig @@ -28,6 +28,7 @@ config DRM_I2C_SIL164 config DRM_I2C_NXP_TDA998X tristate "NXP Semiconductors TDA998X HDMI encoder" default m if DRM_TILCDC + select SND_SOC_TDA998X help Support for NXP Semiconductors TDA998X HDMI encoders. diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index ce1478d..a26a516 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -27,6 +27,11 @@ #include <drm/drm_encoder_slave.h> #include <drm/drm_edid.h> #include <drm/i2c/tda998x.h> +#include <sound/tda998x.h> + +#if defined(CONFIG_SND_SOC_TDA998X) || defined(CONFIG_SND_SOC_TDA998X_MODULE) +#define WITH_CODEC 1 +#endif #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__) @@ -47,6 +52,10 @@ struct tda998x_priv { struct drm_encoder *encoder; u8 audio_ports[2]; +#ifdef WITH_CODEC + u8 sad[3]; /* Short Audio Descriptor */ + struct snd_pcm_hw_constraint_list rate_constraints; +#endif }; #define to_tda998x_priv(x) ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv) @@ -730,11 +739,109 @@ tda998x_configure_audio(struct tda998x_priv *priv, tda998x_write_aif(priv, p); } +#ifdef WITH_CODEC +/* tda998x audio codec interface */ + +/* return the audio parameters extracted from the last EDID */ +static int tda998x_get_audio_var(struct device *dev, + u8 **sad, + struct snd_pcm_hw_constraint_list **rate_constraints) +{ + struct tda998x_priv *priv = dev_get_drvdata(dev); + + if (!priv->encoder->crtc + || !priv->is_hdmi_sink) + return -ENODEV; + + *sad = priv->sad; + *rate_constraints = &priv->rate_constraints; + return 0; +} + +/* switch the audio port and initialize the audio parameters for streaming */ +static int tda998x_set_audio_input(struct device *dev, + int port_index, + unsigned sample_rate, + int sample_format) +{ + struct tda998x_priv *priv = dev_get_drvdata(dev); + struct tda998x_encoder_params *p = &priv->params; + + if (!priv->encoder->crtc) + return -ENODEV; + + /* if no port, just disable the audio port */ + if (port_index == PORT_NONE) { + reg_write(priv, REG_ENA_AP, 0); + return 0; + } + + /* if same audio parameters, just enable the audio port */ + if (p->audio_cfg == priv->audio_ports[port_index] && + p->audio_sample_rate == sample_rate) { + reg_write(priv, REG_ENA_AP, p->audio_cfg); + return 0; + } + + p->audio_format = port_index == PORT_SPDIF ? AFMT_SPDIF : AFMT_I2S; + p->audio_clk_cfg = port_index == PORT_SPDIF ? 0 : 1; + p->audio_cfg = priv->audio_ports[port_index]; + p->audio_sample_rate = sample_rate; + tda998x_configure_audio(priv, &priv->encoder->crtc->hwmode, p); + return 0; +} + +/* get the audio capabilities from the EDID */ +static void tda998x_get_audio_caps(struct tda998x_priv *priv, + struct drm_connector *connector) +{ + u8 *eld = connector->eld; + u8 *sad; + int sad_count; + unsigned eld_ver, mnl; + u8 max_channels, rate_mask, fmt; + + /* adjust the hw params from the ELD (EDID) */ + eld_ver = eld[0] >> 3; + if (eld_ver != 2 && eld_ver != 31) + return; + + mnl = eld[4] & 0x1f; + if (mnl > 16) + return; + + sad_count = eld[5] >> 4; + sad = eld + 20 + mnl; + + /* Start from the basic audio settings */ + max_channels = 1; + rate_mask = 0; + fmt = 0; + while (sad_count--) { + switch (sad[0] & 0x78) { + case 0x08: /* SAD uncompressed audio */ + if ((sad[0] & 7) > max_channels) + max_channels = sad[0] & 7; + rate_mask |= sad[1]; + fmt |= sad[2] & 0x07; + break; + } + sad += 3; + } + + priv->sad[0] = max_channels; + priv->sad[1] = rate_mask; + priv->sad[2] = fmt; +} +#endif /* WITH_CODEC */ + /* DRM encoder functions */ static void tda998x_encoder_set_config(struct tda998x_priv *priv, const struct tda998x_encoder_params *p) { + int port_index; + priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) | (p->mirr_a ? VIP_CNTRL_0_MIRR_A : 0) | VIP_CNTRL_0_SWAP_B(p->swap_b) | @@ -749,6 +856,8 @@ static void tda998x_encoder_set_config(struct tda998x_priv *priv, (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0); priv->params = *p; + port_index = p->audio_format == AFMT_SPDIF ? PORT_SPDIF : PORT_I2S; + priv->audio_ports[port_index] = p->audio_cfg; } static void tda998x_encoder_dpms(struct tda998x_priv *priv, int mode) @@ -1142,6 +1251,15 @@ tda998x_encoder_get_modes(struct tda998x_priv *priv, drm_mode_connector_update_edid_property(connector, edid); n = drm_add_edid_modes(connector, edid); priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid); + +#ifdef WITH_CODEC + /* set the audio parameters from the EDID */ + if (priv->is_hdmi_sink) { + drm_edid_to_eld(connector, edid); + tda998x_get_audio_caps(priv, connector); + } +#endif + kfree(edid); } @@ -1176,6 +1294,10 @@ static void tda998x_destroy(struct tda998x_priv *priv) if (priv->hdmi->irq) free_irq(priv->hdmi->irq, priv); +#ifdef WITH_CODEC + tda9998x_codec_unregister(&priv->hdmi->dev); +#endif + i2c_unregister_device(priv->cec); } @@ -1384,26 +1506,33 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) break; } if (strcmp(p, "spdif") == 0) { - priv->audio_ports[0] = port; + priv->audio_ports[PORT_SPDIF] = port; } else if (strcmp(p, "i2s") == 0) { - priv->audio_ports[1] = port; + priv->audio_ports[PORT_I2S] = port; } else { dev_err(&client->dev, "bad audio-port-names '%s'\n", p); break; } } - if (priv->audio_ports[0]) { - priv->params.audio_cfg = priv->audio_ports[0]; + if (priv->audio_ports[PORT_SPDIF]) { + priv->params.audio_cfg = priv->audio_ports[PORT_SPDIF]; priv->params.audio_format = AFMT_SPDIF; priv->params.audio_clk_cfg = 0; } else { - priv->params.audio_cfg = priv->audio_ports[1]; + priv->params.audio_cfg = priv->audio_ports[PORT_I2S]; priv->params.audio_format = AFMT_I2S; priv->params.audio_clk_cfg = 1; } } +#ifdef WITH_CODEC + /* create the audio CODEC */ + if (priv->audio_ports[PORT_SPDIF] || priv->audio_ports[PORT_I2S]) + tda9998x_codec_register(&client->dev, + tda998x_get_audio_var, + tda998x_set_audio_input); +#endif return 0; fail: diff --git a/include/sound/tda998x.h b/include/sound/tda998x.h new file mode 100644 index 0000000..97cff30 --- /dev/null +++ b/include/sound/tda998x.h @@ -0,0 +1,23 @@ +#ifndef SND_TDA998X_H +#define SND_TDA998X_H + +#include <sound/pcm.h> + +/* port indexes */ +#define PORT_NONE (-1) +#define PORT_SPDIF 0 +#define PORT_I2S 1 + +typedef int (*get_audio_var_t)(struct device *dev, + u8 **sad, + struct snd_pcm_hw_constraint_list **rate_constraints); +typedef int (*set_audio_input_t)(struct device *dev, + int port_index, + unsigned sample_rate, + int sample_format); + +int tda9998x_codec_register(struct device *dev, + get_audio_var_t get_audio_var_i, + set_audio_input_t set_audio_input_i); +void tda9998x_codec_unregister(struct device *dev); +#endif diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index 8349f98..456c549 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -102,6 +102,7 @@ config SND_SOC_ALL_CODECS select SND_SOC_STAC9766 if SND_SOC_AC97_BUS select SND_SOC_TAS2552 if I2C select SND_SOC_TAS5086 if I2C + select SND_SOC_TDA998X if DRM_I2C_NXP_TDA998X select SND_SOC_TFA9879 if I2C select SND_SOC_TLV320AIC23_I2C if I2C select SND_SOC_TLV320AIC23_SPI if SPI_MASTER @@ -600,6 +601,9 @@ config SND_SOC_TAS5086 tristate "Texas Instruments TAS5086 speaker amplifier" depends on I2C +config SND_SOC_TDA998X + tristate + config SND_SOC_TFA9879 tristate "NXP Semiconductors TFA9879 amplifier" depends on I2C diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile index bbdfd1e..44b4fda 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -104,6 +104,7 @@ snd-soc-sta350-objs := sta350.o snd-soc-sta529-objs := sta529.o snd-soc-stac9766-objs := stac9766.o snd-soc-tas5086-objs := tas5086.o +snd-soc-tda998x-objs := tda998x.o snd-soc-tfa9879-objs := tfa9879.o snd-soc-tlv320aic23-objs := tlv320aic23.o snd-soc-tlv320aic23-i2c-objs := tlv320aic23-i2c.o @@ -282,6 +283,7 @@ obj-$(CONFIG_SND_SOC_STA529) += snd-soc-sta529.o obj-$(CONFIG_SND_SOC_STAC9766) += snd-soc-stac9766.o obj-$(CONFIG_SND_SOC_TAS2552) += snd-soc-tas2552.o obj-$(CONFIG_SND_SOC_TAS5086) += snd-soc-tas5086.o +obj-$(CONFIG_SND_SOC_TDA998X) += snd-soc-tda998x.o obj-$(CONFIG_SND_SOC_TFA9879) += snd-soc-tfa9879.o obj-$(CONFIG_SND_SOC_TLV320AIC23) += snd-soc-tlv320aic23.o obj-$(CONFIG_SND_SOC_TLV320AIC23_I2C) += snd-soc-tlv320aic23-i2c.o diff --git a/sound/soc/codecs/tda998x.c b/sound/soc/codecs/tda998x.c new file mode 100644 index 0000000..b055570 --- /dev/null +++ b/sound/soc/codecs/tda998x.c @@ -0,0 +1,175 @@ +/* + * ALSA SoC TDA998x CODEC + * + * Copyright (C) 2015 Jean-Francois Moine + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/module.h> +#include <sound/soc.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <sound/pcm_params.h> +#include <sound/tda998x.h> + +/* functions in tda998x_drv */ +static get_audio_var_t get_audio_var; +static set_audio_input_t set_audio_input; + +static int tda998x_codec_startup(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + struct snd_pcm_hw_constraint_list *rate_constraints; + u8 *sad; /* Short Audio Descriptor (3 bytes) */ +#define SAD_MX_CHAN_I 0 /* max number of chennels - 1 */ +#define SAD_RATE_MASK_I 1 /* rate mask */ +#define SAD_FMT_I 2 /* formats */ +#define SAD_FMT_16 0x01 +#define SAD_FMT_20 0x02 +#define SAD_FMT_24 0x04 + u64 formats; + int ret; + static const u32 hdmi_rates[] = { + 32000, 44100, 48000, 88200, 96000, 176400, 192000 + }; + + /* get the EDID values and the rate constraints buffer */ + ret = get_audio_var(dai->dev, &sad, &rate_constraints); + if (ret < 0) + return ret; /* no screen */ + + /* convert the EDID values to audio constraints */ + rate_constraints->list = hdmi_rates; + rate_constraints->count = ARRAY_SIZE(hdmi_rates); + rate_constraints->mask = sad[SAD_RATE_MASK_I]; + snd_pcm_hw_constraint_list(runtime, 0, + SNDRV_PCM_HW_PARAM_RATE, + rate_constraints); + + formats = 0; + if (sad[SAD_FMT_I] & SAD_FMT_16) + formats |= SNDRV_PCM_FMTBIT_S16_LE; + if (sad[SAD_FMT_I] & SAD_FMT_20) + formats |= SNDRV_PCM_FMTBIT_S20_3LE; + if (sad[SAD_FMT_I] & SAD_FMT_24) + formats |= SNDRV_PCM_FMTBIT_S24_LE | + SNDRV_PCM_FMTBIT_S24_3LE | + SNDRV_PCM_FMTBIT_S32_LE; + snd_pcm_hw_constraint_mask64(runtime, + SNDRV_PCM_HW_PARAM_FORMAT, + formats); + + snd_pcm_hw_constraint_minmax(runtime, + SNDRV_PCM_HW_PARAM_CHANNELS, + 1, sad[SAD_MX_CHAN_I]); + return 0; +} + +static int tda998x_codec_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + struct snd_soc_dai *dai) +{ + return set_audio_input(dai->dev, dai->id, + params_rate(params), + params_format(params)); +} + +static void tda998x_codec_shutdown(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + set_audio_input(dai->dev, PORT_NONE, 0, 0); +} + +static const struct snd_soc_dai_ops tda998x_codec_ops = { + .startup = tda998x_codec_startup, + .hw_params = tda998x_codec_hw_params, + .shutdown = tda998x_codec_shutdown, +}; + +#define TDA998X_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \ + SNDRV_PCM_FMTBIT_S20_3LE | \ + SNDRV_PCM_FMTBIT_S24_LE | \ + SNDRV_PCM_FMTBIT_S32_LE) + +static struct snd_soc_dai_driver tda998x_dais[] = { + { + .name = "spdif-hifi", + .id = PORT_SPDIF, + .playback = { + .stream_name = "HDMI SPDIF Playback", + .channels_min = 1, + .channels_max = 2, + .rates = SNDRV_PCM_RATE_CONTINUOUS, + .rate_min = 22050, + .rate_max = 192000, + .formats = TDA998X_FORMATS, + }, + .ops = &tda998x_codec_ops, + }, + { + .name = "i2s-hifi", + .id = PORT_I2S, + .playback = { + .stream_name = "HDMI I2S Playback", + .channels_min = 1, + .channels_max = 8, + .rates = SNDRV_PCM_RATE_CONTINUOUS, + .rate_min = 5512, + .rate_max = 192000, + .formats = TDA998X_FORMATS, + }, + .ops = &tda998x_codec_ops, + }, +}; + +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)); +} +EXPORT_SYMBOL(tda9998x_codec_register); + +void tda9998x_codec_unregister(struct device *dev) +{ + snd_soc_unregister_codec(dev); +} +EXPORT_SYMBOL(tda9998x_codec_unregister); + +static int __init tda998x_codec_init(void) +{ + return 0; +} +static void __exit tda998x_codec_exit(void) +{ +} +module_init(tda998x_codec_init); +module_exit(tda998x_codec_exit); + +MODULE_AUTHOR("Jean-Francois Moine <moinejf@free.fr>"); +MODULE_DESCRIPTION("TDA998X CODEC"); +MODULE_LICENSE("GPL");
This patch adds a CODEC to the NXP TDA998x HDMI transmitter. The CODEC handles both I2S and S/PDIF inputs. It maintains the audio format and rate constraints according to the HDMI device parameters (EDID) and sets dynamically the input ports in the TDA998x I2C driver on start/stop audio streaming. Signed-off-by: Jean-Francois Moine <moinejf@free.fr> --- drivers/gpu/drm/i2c/Kconfig | 1 + drivers/gpu/drm/i2c/tda998x_drv.c | 139 ++++++++++++++++++++++++++++-- include/sound/tda998x.h | 23 +++++ sound/soc/codecs/Kconfig | 4 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/tda998x.c | 175 ++++++++++++++++++++++++++++++++++++++ 6 files changed, 339 insertions(+), 5 deletions(-) create mode 100644 include/sound/tda998x.h create mode 100644 sound/soc/codecs/tda998x.c