There is very little change to this series since the RFC v4, but because of the recent interest on the hdmi-codec patch I decided to send another version of the series.
Changes since RFC v4, - Rebased on top of the latest drm-next branch - Split the hdmi-codec abort functionality into a separate patch for better visibility of what it is all about - This does not affect the tda998x patches as the abort functionality is not used - Drop S18_3* formats from I2S_FORMATS and add a comment about formats not supported by HDMI
There was also some comments about the DT ports binding for tda998x by Jean-Francois Moine. However I did not have time to come up with alternative approach. All in all my tda998x patches should be considered more as a proof of concept for the hdmi-codec part, rather than a serious attempt to get those patches in. However, all comments to those patch are more than welcome as they will help me to come up with something that could finally get merged.
Changes since RFC v3, ASoC side: - Add "ALSA: pcm: add IEC958 channel status helper for hw_params" - Add "tda998x: Improve tda998x_configure_audio() audio related pdata" - use snd_pcm_create_iec958_consumer_hw_params() to construct the stream header - Remove set_clk() callback from hdmi-codec. It is not needed for now. - Refer to stream header in AIF as specified in HDMI standard - Set current_stream to NULL only after video side audio_shutdown() has been called. Avoid potential race if video side attempts to abort audio at the same time. - No need to have video side device pointer in the hdmi codec's pdata as it is found from dev->parent. - Fix hdmi-codec enum: DAI_ID_I2C > DAI_ID_I2S - Improve audio_startup API comment - Make improved checkpatch happy - BUG_ON > WARN_ON - put */ ending the block comment to a separate line
DRM side: - Fix tda998x get_eld() locking - Change tda998x audio parameters in pdata to more generic, that can be readily used in tda998x_audio_config() - Rename and restructure audio port related private data members to be more descriptive - Require audio configuration trough ASoC hdmi-codec if HDMI audio is configured trough DT binding.
DTS: - Increase McASP fifo usage form 1 to 32
The binding for tda998x is taken from Jean Francois' patch series[1] on the same subject. The implementation of the of-node parsing has some minor changes from my self.
Here is what I think at least could or should still be done, but non of that stuff does not sounds critical right now.
Missing from tda998x driver side - hdmi_codec_ops audio_startup() implementation for audio abort support - multi channel audio support (I would need specs and preferably some HW to test for this).
Missing from ASoC side generic implementation: - channel_allocation handling is completely left for the video side driver, see if ASoC side could help in any way - snd_soc_jack functionality to handle hdmi cable plug/unplug events
[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2015-July/095596.html
Best regards, Jyri
Jean-Francois Moine (1): drm/i2c: tda998x: Add support of a DT graph of ports
Jyri Sarha (7): ALSA: pcm: add IEC958 channel status helper for hw_params ASoC: hdmi-codec: Add hdmi-codec for external HDMI-encoders ASoC: hdmi-codec: Add audio abort() callback for video side to use drm/i2c: tda998x: Remove include/sound/tda998x.h and fix graph parsing drm/i2c: tda998x: Improve tda998x_configure_audio() audio related pdata drm/i2c: tda998x: Register ASoC HDMI codec for audio functionality ARM: dts: am335x-boneblack: Add HDMI audio support
.../devicetree/bindings/display/bridge/tda998x.txt | 51 +++ arch/arm/boot/dts/am335x-boneblack.dts | 90 ++++- drivers/gpu/drm/i2c/Kconfig | 1 + drivers/gpu/drm/i2c/tda998x_drv.c | 300 ++++++++++++--- include/drm/i2c/tda998x.h | 24 +- include/sound/hdmi-codec.h | 104 ++++++ include/sound/pcm_iec958.h | 2 + sound/core/pcm_iec958.c | 52 ++- sound/soc/codecs/Kconfig | 6 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/hdmi-codec.c | 411 +++++++++++++++++++++ 11 files changed, 964 insertions(+), 79 deletions(-) create mode 100644 include/sound/hdmi-codec.h create mode 100644 sound/soc/codecs/hdmi-codec.c
Add IEC958 channel status helper that gets the audio properties from snd_pcm_hw_params instead of snd_pcm_runtime. This is needed to produce the channel status bits already in audio stream configuration phase.
Signed-off-by: Jyri Sarha jsarha@ti.com --- include/sound/pcm_iec958.h | 2 ++ sound/core/pcm_iec958.c | 52 +++++++++++++++++++++++++++++++--------------- 2 files changed, 37 insertions(+), 17 deletions(-)
diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h index 0eed397..36f023a 100644 --- a/include/sound/pcm_iec958.h +++ b/include/sound/pcm_iec958.h @@ -6,4 +6,6 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, size_t len);
+int snd_pcm_create_iec958_consumer_hw_params(struct snd_pcm_hw_params *params, + u8 *cs, size_t len); #endif diff --git a/sound/core/pcm_iec958.c b/sound/core/pcm_iec958.c index 36b2d7a..c9f8b66 100644 --- a/sound/core/pcm_iec958.c +++ b/sound/core/pcm_iec958.c @@ -9,30 +9,18 @@ #include <linux/types.h> #include <sound/asoundef.h> #include <sound/pcm.h> +#include <sound/pcm_params.h> #include <sound/pcm_iec958.h>
-/** - * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status - * @runtime: pcm runtime structure with ->rate filled in - * @cs: channel status buffer, at least four bytes - * @len: length of channel status buffer - * - * Create the consumer format channel status data in @cs of maximum size - * @len corresponding to the parameters of the PCM runtime @runtime. - * - * Drivers may wish to tweak the contents of the buffer after creation. - * - * Returns: length of buffer, or negative error code if something failed. - */ -int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, - size_t len) +static int create_iec958_consumer(uint rate, uint sample_width, + u8 *cs, size_t len) { unsigned int fs, ws;
if (len < 4) return -EINVAL;
- switch (runtime->rate) { + switch (rate) { case 32000: fs = IEC958_AES3_CON_FS_32000; break; @@ -59,7 +47,7 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, }
if (len > 4) { - switch (snd_pcm_format_width(runtime->format)) { + switch (sample_width) { case 16: ws = IEC958_AES4_CON_WORDLEN_20_16; break; @@ -92,4 +80,34 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
return len; } + +/** + * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status + * @runtime: pcm runtime structure with ->rate filled in + * @cs: channel status buffer, at least four bytes + * @len: length of channel status buffer + * + * Create the consumer format channel status data in @cs of maximum size + * @len corresponding to the parameters of the PCM runtime @runtime. + * + * Drivers may wish to tweak the contents of the buffer after creation. + * + * Returns: length of buffer, or negative error code if something failed. + */ +int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, + size_t len) +{ + return create_iec958_consumer(runtime->rate, + snd_pcm_format_width(runtime->format), + cs, len); +} EXPORT_SYMBOL(snd_pcm_create_iec958_consumer); + + +int snd_pcm_create_iec958_consumer_hw_params(struct snd_pcm_hw_params *params, + u8 *cs, size_t len) +{ + return create_iec958_consumer(params_rate(params), params_width(params), + cs, len); +} +EXPORT_SYMBOL(snd_pcm_create_iec958_consumer_hw_params);
The hdmi-codec is a platform device driver to be registered from drivers of external HDMI encoders with I2S and/or spdif interface. The driver in turn registers an ASoC codec for the HDMI encoder's audio functionality.
The structures and definitions in the API header are mostly redundant copies of similar structures in ASoC headers. This is on purpose to avoid direct dependencies to ASoC structures in video side driver.
Signed-off-by: Jyri Sarha jsarha@ti.com --- include/sound/hdmi-codec.h | 100 +++++++++++ sound/soc/codecs/Kconfig | 6 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/hdmi-codec.c | 393 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 501 insertions(+) create mode 100644 include/sound/hdmi-codec.h create mode 100644 sound/soc/codecs/hdmi-codec.c
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h new file mode 100644 index 0000000..fc3a481 --- /dev/null +++ b/include/sound/hdmi-codec.h @@ -0,0 +1,100 @@ +/* + * hdmi-codec.h - HDMI Codec driver API + * + * Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com + * + * Author: Jyri Sarha jsarha@ti.com + * + * 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. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + */ + +#ifndef __HDMI_CODEC_H__ +#define __HDMI_CODEC_H__ + +#include <linux/hdmi.h> +#include <drm/drm_edid.h> +#include <sound/asoundef.h> +#include <uapi/sound/asound.h> + +/* + * Protocol between ASoC cpu-dai and HDMI-encoder + */ +struct hdmi_codec_daifmt { + enum { + HDMI_I2S, + HDMI_RIGHT_J, + HDMI_LEFT_J, + HDMI_DSP_A, + HDMI_DSP_B, + HDMI_AC97, + HDMI_SPDIF, + } fmt; + int bit_clk_inv:1; + int frame_clk_inv:1; + int bit_clk_master:1; + int frame_clk_master:1; +}; + +/* + * HDMI audio parameters + */ +struct hdmi_codec_params { + struct hdmi_audio_infoframe cea; + struct snd_aes_iec958 iec; + int sample_rate; + int sample_width; + int channels; +}; + +struct hdmi_codec_ops { + /* + * Called when ASoC starts an audio stream setup. + * Optional + */ + int (*audio_startup)(struct device *dev); + + /* + * Configures HDMI-encoder for audio stream. + * Mandatory + */ + int (*hw_params)(struct device *dev, + struct hdmi_codec_daifmt *fmt, + struct hdmi_codec_params *hparms); + + /* + * Shuts down the audio stream. + * Mandatory + */ + void (*audio_shutdown)(struct device *dev); + + /* + * Mute/unmute HDMI audio stream. + * Optional + */ + int (*digital_mute)(struct device *dev, bool enable); + + /* + * Provides EDID-Like-Data from connected HDMI device. + * Optional + */ + int (*get_eld)(struct device *dev, uint8_t *buf, size_t len); +}; + +/* HDMI codec initalization data */ +struct hdmi_codec_pdata { + const struct hdmi_codec_ops *ops; + uint i2s:1; + uint spdif:1; + int max_i2s_channels; +}; + +#define HDMI_CODEC_DRV_NAME "hdmi-audio-codec" + +#endif /* __HDMI_CODEC_H__ */ diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index cfdafc4..4d5915e 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -82,6 +82,7 @@ config SND_SOC_ALL_CODECS select SND_SOC_MC13783 if MFD_MC13XXX select SND_SOC_ML26124 if I2C select SND_SOC_NAU8825 if I2C + select SND_SOC_HDMI_CODEC select SND_SOC_PCM1681 if I2C select SND_SOC_PCM1792A if SPI_MASTER select SND_SOC_PCM3008 @@ -454,6 +455,11 @@ config SND_SOC_BT_SCO config SND_SOC_DMIC tristate
+config SND_SOC_HDMI_CODEC + tristate + select SND_PCM_ELD + select SND_PCM_IEC958 + config SND_SOC_ES8328 tristate "Everest Semi ES8328 CODEC"
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile index f632fc4..49c1824 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -75,6 +75,7 @@ snd-soc-max9850-objs := max9850.o snd-soc-mc13783-objs := mc13783.o snd-soc-ml26124-objs := ml26124.o snd-soc-nau8825-objs := nau8825.o +snd-soc-hdmi-codec-objs := hdmi-codec.o snd-soc-pcm1681-objs := pcm1681.o snd-soc-pcm1792a-codec-objs := pcm1792a.o snd-soc-pcm3008-objs := pcm3008.o @@ -270,6 +271,7 @@ obj-$(CONFIG_SND_SOC_MAX9850) += snd-soc-max9850.o obj-$(CONFIG_SND_SOC_MC13783) += snd-soc-mc13783.o obj-$(CONFIG_SND_SOC_ML26124) += snd-soc-ml26124.o obj-$(CONFIG_SND_SOC_NAU8825) += snd-soc-nau8825.o +obj-$(CONFIG_SND_SOC_HDMI_CODEC) += snd-soc-hdmi-codec.o obj-$(CONFIG_SND_SOC_PCM1681) += snd-soc-pcm1681.o obj-$(CONFIG_SND_SOC_PCM1792A) += snd-soc-pcm1792a-codec.o obj-$(CONFIG_SND_SOC_PCM3008) += snd-soc-pcm3008.o diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c new file mode 100644 index 0000000..bc47b9a --- /dev/null +++ b/sound/soc/codecs/hdmi-codec.c @@ -0,0 +1,393 @@ +/* + * ALSA SoC codec for HDMI encoder drivers + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/ + * Author: Jyri Sarha jsarha@ti.com + * + * 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. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + */ +#include <linux/module.h> +#include <linux/string.h> +#include <sound/core.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> +#include <sound/pcm_drm_eld.h> +#include <sound/hdmi-codec.h> +#include <sound/pcm_iec958.h> + +#include <drm/drm_crtc.h> /* This is only to get MAX_ELD_BYTES */ + +struct hdmi_codec_priv { + struct hdmi_codec_pdata hcd; + struct snd_soc_dai_driver *daidrv; + struct hdmi_codec_daifmt daifmt[2]; + struct mutex current_stream_lock; + struct snd_pcm_substream *current_stream; + struct snd_pcm_hw_constraint_list ratec; + uint8_t eld[MAX_ELD_BYTES]; +}; + +static const struct snd_soc_dapm_widget hdmi_widgets[] = { + SND_SOC_DAPM_OUTPUT("TX"), +}; + +static const struct snd_soc_dapm_route hdmi_routes[] = { + { "TX", NULL, "Playback" }, +}; + +enum { + DAI_ID_I2S = 0, + DAI_ID_SPDIF, +}; + +static int hdmi_codec_new_stream(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai); + int ret = 0; + + mutex_lock(&hcp->current_stream_lock); + if (!hcp->current_stream) { + hcp->current_stream = substream; + } else if (hcp->current_stream != substream) { + dev_err(dai->dev, "Only one simultaneous stream supported!\n"); + ret = -EINVAL; + } + mutex_unlock(&hcp->current_stream_lock); + + return ret; +} + +static int hdmi_codec_startup(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai); + int ret = 0; + + dev_dbg(dai->dev, "%s()\n", __func__); + + ret = hdmi_codec_new_stream(substream, dai); + if (ret) + return ret; + + if (hcp->hcd.ops->audio_startup) { + ret = hcp->hcd.ops->audio_startup(dai->dev->parent); + if (ret) { + mutex_lock(&hcp->current_stream_lock); + hcp->current_stream = NULL; + mutex_unlock(&hcp->current_stream_lock); + return ret; + } + } + + if (hcp->hcd.ops->get_eld) { + ret = hcp->hcd.ops->get_eld(dai->dev->parent, hcp->eld, + sizeof(hcp->eld)); + + if (!ret) { + ret = snd_pcm_hw_constraint_eld(substream->runtime, + hcp->eld); + if (ret) + return ret; + } + } + return 0; +} + +static void hdmi_codec_shutdown(struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai); + + dev_dbg(dai->dev, "%s()\n", __func__); + + WARN_ON(hcp->current_stream != substream); + + hcp->hcd.ops->audio_shutdown(dai->dev->parent); + + mutex_lock(&hcp->current_stream_lock); + hcp->current_stream = NULL; + mutex_unlock(&hcp->current_stream_lock); +} + +static int hdmi_codec_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + struct snd_soc_dai *dai) +{ + struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai); + struct hdmi_codec_params hp = { + .iec = { + .status = { 0 }, + .subcode = { 0 }, + .pad = 0, + .dig_subframe = { 0 }, + } + }; + int ret; + + dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__, + params_width(params), params_rate(params), + params_channels(params)); + + ret = snd_pcm_create_iec958_consumer_hw_params(params, hp.iec.status, + sizeof(hp.iec.status)); + if (ret < 0) { + dev_err(dai->dev, "Creating IEC958 channel status failed %d\n", + ret); + return ret; + } + + ret = hdmi_codec_new_stream(substream, dai); + if (ret) + return ret; + + hdmi_audio_infoframe_init(&hp.cea); + hp.cea.channels = params_channels(params); + hp.cea.coding_type = HDMI_AUDIO_CODING_TYPE_STREAM; + hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM; + hp.cea.sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM; + + hp.sample_width = params_width(params); + hp.sample_rate = params_rate(params); + hp.channels = params_channels(params); + + return hcp->hcd.ops->hw_params(dai->dev->parent, &hcp->daifmt[dai->id], + &hp); +} + +static int hdmi_codec_set_fmt(struct snd_soc_dai *dai, + unsigned int fmt) +{ + struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai); + struct hdmi_codec_daifmt cf = { 0 }; + int ret = 0; + + dev_dbg(dai->dev, "%s()\n", __func__); + + if (dai->id == DAI_ID_SPDIF) { + cf.fmt = HDMI_SPDIF; + } else { + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { + case SND_SOC_DAIFMT_CBM_CFM: + cf.bit_clk_master = 1; + cf.frame_clk_master = 1; + break; + case SND_SOC_DAIFMT_CBS_CFM: + cf.frame_clk_master = 1; + break; + case SND_SOC_DAIFMT_CBM_CFS: + cf.bit_clk_master = 1; + break; + case SND_SOC_DAIFMT_CBS_CFS: + break; + default: + return -EINVAL; + } + + switch (fmt & SND_SOC_DAIFMT_INV_MASK) { + case SND_SOC_DAIFMT_NB_NF: + break; + case SND_SOC_DAIFMT_NB_IF: + cf.frame_clk_inv = 1; + break; + case SND_SOC_DAIFMT_IB_NF: + cf.bit_clk_inv = 1; + break; + case SND_SOC_DAIFMT_IB_IF: + cf.frame_clk_inv = 1; + cf.bit_clk_inv = 1; + break; + } + + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { + case SND_SOC_DAIFMT_I2S: + cf.fmt = HDMI_I2S; + break; + case SND_SOC_DAIFMT_DSP_A: + cf.fmt = HDMI_DSP_A; + break; + case SND_SOC_DAIFMT_DSP_B: + cf.fmt = HDMI_DSP_B; + break; + case SND_SOC_DAIFMT_RIGHT_J: + cf.fmt = HDMI_RIGHT_J; + break; + case SND_SOC_DAIFMT_LEFT_J: + cf.fmt = HDMI_LEFT_J; + break; + case SND_SOC_DAIFMT_AC97: + cf.fmt = HDMI_AC97; + break; + default: + dev_err(dai->dev, "Invalid DAI interface format\n"); + return -EINVAL; + } + } + + hcp->daifmt[dai->id] = cf; + + return ret; +} + +static int hdmi_codec_digital_mute(struct snd_soc_dai *dai, int mute) +{ + struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai); + + dev_dbg(dai->dev, "%s()\n", __func__); + + if (hcp->hcd.ops->digital_mute) + return hcp->hcd.ops->digital_mute(dai->dev->parent, mute); + + return 0; +} + +static const struct snd_soc_dai_ops hdmi_dai_ops = { + .startup = hdmi_codec_startup, + .shutdown = hdmi_codec_shutdown, + .hw_params = hdmi_codec_hw_params, + .set_fmt = hdmi_codec_set_fmt, + .digital_mute = hdmi_codec_digital_mute, +}; + + +#define HDMI_RATES (SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\ + SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\ + SNDRV_PCM_RATE_96000 | SNDRV_PCM_RATE_176400 |\ + SNDRV_PCM_RATE_192000) + +#define SPDIF_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\ + SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\ + SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\ + SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE) + +/* + * This list is only for formats allowed on the I2S bus. So there is + * some formats listed that are not supported by HDMI interface. For + * instance allowing the 32-bit formats enables 24-precision with CPU + * DAIs that do not support 24-bit formats. If the extra formats cause + * problems, we should add the video side driver an option to disable + * them. + */ +#define I2S_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\ + SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\ + SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\ + SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE |\ + SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S32_BE) + +static struct snd_soc_dai_driver hdmi_i2s_dai = { + .name = "i2s-hifi", + .id = DAI_ID_I2S, + .playback = { + .stream_name = "Playback", + .channels_min = 2, + .channels_max = 8, + .rates = HDMI_RATES, + .formats = I2S_FORMATS, + .sig_bits = 24, + }, + .ops = &hdmi_dai_ops, +}; + +static const struct snd_soc_dai_driver hdmi_spdif_dai = { + .name = "spdif-hifi", + .id = DAI_ID_SPDIF, + .playback = { + .stream_name = "Playback", + .channels_min = 2, + .channels_max = 2, + .rates = HDMI_RATES, + .formats = SPDIF_FORMATS, + }, + .ops = &hdmi_dai_ops, +}; + +static struct snd_soc_codec_driver hdmi_codec = { + .dapm_widgets = hdmi_widgets, + .num_dapm_widgets = ARRAY_SIZE(hdmi_widgets), + .dapm_routes = hdmi_routes, + .num_dapm_routes = ARRAY_SIZE(hdmi_routes), +}; + +static int hdmi_codec_probe(struct platform_device *pdev) +{ + struct hdmi_codec_pdata *hcd = pdev->dev.platform_data; + struct device *dev = &pdev->dev; + struct hdmi_codec_priv *hcp; + int dai_count, i = 0; + int ret; + + dev_dbg(dev, "%s()\n", __func__); + + if (!hcd) { + dev_err(dev, "%s: No plalform data\n", __func__); + return -EINVAL; + } + + dai_count = hcd->i2s + hcd->spdif; + if (dai_count < 1 || !hcd->ops || !hcd->ops->hw_params || + !hcd->ops->audio_shutdown) { + dev_err(dev, "%s: Invalid parameters\n", __func__); + return -EINVAL; + } + + hcp = devm_kzalloc(dev, sizeof(*hcp), GFP_KERNEL); + if (!hcp) + return -ENOMEM; + + hcp->hcd = *hcd; + mutex_init(&hcp->current_stream_lock); + + hcp->daidrv = devm_kzalloc(dev, dai_count * sizeof(*hcp->daidrv), + GFP_KERNEL); + if (!hcp->daidrv) + return -ENOMEM; + + if (hcd->i2s) { + hcp->daidrv[i] = hdmi_i2s_dai; + hcp->daidrv[i].playback.channels_max = + hcd->max_i2s_channels; + i++; + } + + if (hcd->spdif) + hcp->daidrv[i] = hdmi_spdif_dai; + + ret = snd_soc_register_codec(dev, &hdmi_codec, hcp->daidrv, + dai_count); + if (ret) { + dev_err(dev, "%s: snd_soc_register_codec() failed (%d)\n", + __func__, ret); + return ret; + } + + dev_set_drvdata(dev, hcp); + return 0; +} + +static int hdmi_codec_remove(struct platform_device *pdev) +{ + snd_soc_unregister_codec(&pdev->dev); + return 0; +} + +static struct platform_driver hdmi_codec_driver = { + .driver = { + .name = HDMI_CODEC_DRV_NAME, + }, + .probe = hdmi_codec_probe, + .remove = hdmi_codec_remove, +}; + +module_platform_driver(hdmi_codec_driver); + +MODULE_AUTHOR("Jyri Sarha jsarha@ti.com"); +MODULE_DESCRIPTION("HDMI Audio Codec Driver"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:" HDMI_CODEC_DRV_NAME);
Hello Jyri,
Integrated and tested on sti platform with success.
I will re-submit patch-set associated to sti platform, when (if) patches accepted.
Regards Arnaud
On 02/17/2016 03:49 PM, Jyri Sarha wrote:
The hdmi-codec is a platform device driver to be registered from drivers of external HDMI encoders with I2S and/or spdif interface. The driver in turn registers an ASoC codec for the HDMI encoder's audio functionality.
The structures and definitions in the API header are mostly redundant copies of similar structures in ASoC headers. This is on purpose to avoid direct dependencies to ASoC structures in video side driver.
Signed-off-by: Jyri Sarha jsarha@ti.com
include/sound/hdmi-codec.h | 100 +++++++++++ sound/soc/codecs/Kconfig | 6 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/hdmi-codec.c | 393 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 501 insertions(+) create mode 100644 include/sound/hdmi-codec.h create mode 100644 sound/soc/codecs/hdmi-codec.c
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h new file mode 100644 index 0000000..fc3a481 --- /dev/null +++ b/include/sound/hdmi-codec.h @@ -0,0 +1,100 @@ +/*
- hdmi-codec.h - HDMI Codec driver API
- Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com
- Author: Jyri Sarha jsarha@ti.com
- 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.
- This program is distributed in the hope that it will be useful, but
- WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- General Public License for more details.
- */
+#ifndef __HDMI_CODEC_H__ +#define __HDMI_CODEC_H__
+#include <linux/hdmi.h> +#include <drm/drm_edid.h> +#include <sound/asoundef.h> +#include <uapi/sound/asound.h>
+/*
- Protocol between ASoC cpu-dai and HDMI-encoder
- */
+struct hdmi_codec_daifmt {
- enum {
HDMI_I2S,HDMI_RIGHT_J,HDMI_LEFT_J,HDMI_DSP_A,HDMI_DSP_B,HDMI_AC97,HDMI_SPDIF,- } fmt;
- int bit_clk_inv:1;
- int frame_clk_inv:1;
- int bit_clk_master:1;
- int frame_clk_master:1;
+};
+/*
- HDMI audio parameters
- */
+struct hdmi_codec_params {
- struct hdmi_audio_infoframe cea;
- struct snd_aes_iec958 iec;
- int sample_rate;
- int sample_width;
- int channels;
+};
+struct hdmi_codec_ops {
- /*
* Called when ASoC starts an audio stream setup.* Optional*/- int (*audio_startup)(struct device *dev);
- /*
* Configures HDMI-encoder for audio stream.* Mandatory*/- int (*hw_params)(struct device *dev,
struct hdmi_codec_daifmt *fmt,struct hdmi_codec_params *hparms);- /*
* Shuts down the audio stream.* Mandatory*/- void (*audio_shutdown)(struct device *dev);
- /*
* Mute/unmute HDMI audio stream.* Optional*/- int (*digital_mute)(struct device *dev, bool enable);
- /*
* Provides EDID-Like-Data from connected HDMI device.* Optional*/- int (*get_eld)(struct device *dev, uint8_t *buf, size_t len);
+};
+/* HDMI codec initalization data */ +struct hdmi_codec_pdata {
- const struct hdmi_codec_ops *ops;
- uint i2s:1;
- uint spdif:1;
- int max_i2s_channels;
+};
+#define HDMI_CODEC_DRV_NAME "hdmi-audio-codec"
+#endif /* __HDMI_CODEC_H__ */ diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index cfdafc4..4d5915e 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -82,6 +82,7 @@ config SND_SOC_ALL_CODECS select SND_SOC_MC13783 if MFD_MC13XXX select SND_SOC_ML26124 if I2C select SND_SOC_NAU8825 if I2C
- select SND_SOC_HDMI_CODEC select SND_SOC_PCM1681 if I2C select SND_SOC_PCM1792A if SPI_MASTER select SND_SOC_PCM3008
@@ -454,6 +455,11 @@ config SND_SOC_BT_SCO config SND_SOC_DMIC tristate
+config SND_SOC_HDMI_CODEC
tristateselect SND_PCM_ELDselect SND_PCM_IEC958config SND_SOC_ES8328 tristate "Everest Semi ES8328 CODEC"
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile index f632fc4..49c1824 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -75,6 +75,7 @@ snd-soc-max9850-objs := max9850.o snd-soc-mc13783-objs := mc13783.o snd-soc-ml26124-objs := ml26124.o snd-soc-nau8825-objs := nau8825.o +snd-soc-hdmi-codec-objs := hdmi-codec.o snd-soc-pcm1681-objs := pcm1681.o snd-soc-pcm1792a-codec-objs := pcm1792a.o snd-soc-pcm3008-objs := pcm3008.o @@ -270,6 +271,7 @@ obj-$(CONFIG_SND_SOC_MAX9850) += snd-soc-max9850.o obj-$(CONFIG_SND_SOC_MC13783) += snd-soc-mc13783.o obj-$(CONFIG_SND_SOC_ML26124) += snd-soc-ml26124.o obj-$(CONFIG_SND_SOC_NAU8825) += snd-soc-nau8825.o +obj-$(CONFIG_SND_SOC_HDMI_CODEC) += snd-soc-hdmi-codec.o obj-$(CONFIG_SND_SOC_PCM1681) += snd-soc-pcm1681.o obj-$(CONFIG_SND_SOC_PCM1792A) += snd-soc-pcm1792a-codec.o obj-$(CONFIG_SND_SOC_PCM3008) += snd-soc-pcm3008.o diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c new file mode 100644 index 0000000..bc47b9a --- /dev/null +++ b/sound/soc/codecs/hdmi-codec.c @@ -0,0 +1,393 @@ +/*
- ALSA SoC codec for HDMI encoder drivers
- Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
- Author: Jyri Sarha jsarha@ti.com
- 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.
- This program is distributed in the hope that it will be useful, but
- WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- General Public License for more details.
- */
+#include <linux/module.h> +#include <linux/string.h> +#include <sound/core.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/soc.h> +#include <sound/pcm_drm_eld.h> +#include <sound/hdmi-codec.h> +#include <sound/pcm_iec958.h>
+#include <drm/drm_crtc.h> /* This is only to get MAX_ELD_BYTES */
+struct hdmi_codec_priv {
- struct hdmi_codec_pdata hcd;
- struct snd_soc_dai_driver *daidrv;
- struct hdmi_codec_daifmt daifmt[2];
- struct mutex current_stream_lock;
- struct snd_pcm_substream *current_stream;
- struct snd_pcm_hw_constraint_list ratec;
- uint8_t eld[MAX_ELD_BYTES];
+};
+static const struct snd_soc_dapm_widget hdmi_widgets[] = {
- SND_SOC_DAPM_OUTPUT("TX"),
+};
+static const struct snd_soc_dapm_route hdmi_routes[] = {
- { "TX", NULL, "Playback" },
+};
+enum {
- DAI_ID_I2S = 0,
- DAI_ID_SPDIF,
+};
+static int hdmi_codec_new_stream(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)+{
- struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
- int ret = 0;
- mutex_lock(&hcp->current_stream_lock);
- if (!hcp->current_stream) {
hcp->current_stream = substream;- } else if (hcp->current_stream != substream) {
dev_err(dai->dev, "Only one simultaneous stream supported!\n");ret = -EINVAL;- }
- mutex_unlock(&hcp->current_stream_lock);
- return ret;
+}
+static int hdmi_codec_startup(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)+{
- struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
- int ret = 0;
- dev_dbg(dai->dev, "%s()\n", __func__);
- ret = hdmi_codec_new_stream(substream, dai);
- if (ret)
return ret;- if (hcp->hcd.ops->audio_startup) {
ret = hcp->hcd.ops->audio_startup(dai->dev->parent);if (ret) {mutex_lock(&hcp->current_stream_lock);hcp->current_stream = NULL;mutex_unlock(&hcp->current_stream_lock);return ret;}- }
- if (hcp->hcd.ops->get_eld) {
ret = hcp->hcd.ops->get_eld(dai->dev->parent, hcp->eld,sizeof(hcp->eld));if (!ret) {ret = snd_pcm_hw_constraint_eld(substream->runtime,hcp->eld);if (ret)return ret;}- }
- return 0;
+}
+static void hdmi_codec_shutdown(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)+{
- struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
- dev_dbg(dai->dev, "%s()\n", __func__);
- WARN_ON(hcp->current_stream != substream);
- hcp->hcd.ops->audio_shutdown(dai->dev->parent);
- mutex_lock(&hcp->current_stream_lock);
- hcp->current_stream = NULL;
- mutex_unlock(&hcp->current_stream_lock);
+}
+static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,struct snd_soc_dai *dai)+{
- struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
- struct hdmi_codec_params hp = {
.iec = {.status = { 0 },.subcode = { 0 },.pad = 0,.dig_subframe = { 0 },}- };
- int ret;
- dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__,
params_width(params), params_rate(params),params_channels(params));- ret = snd_pcm_create_iec958_consumer_hw_params(params, hp.iec.status,
sizeof(hp.iec.status));- if (ret < 0) {
dev_err(dai->dev, "Creating IEC958 channel status failed %d\n",ret);return ret;- }
- ret = hdmi_codec_new_stream(substream, dai);
- if (ret)
return ret;- hdmi_audio_infoframe_init(&hp.cea);
- hp.cea.channels = params_channels(params);
- hp.cea.coding_type = HDMI_AUDIO_CODING_TYPE_STREAM;
- hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM;
- hp.cea.sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM;
- hp.sample_width = params_width(params);
- hp.sample_rate = params_rate(params);
- hp.channels = params_channels(params);
- return hcp->hcd.ops->hw_params(dai->dev->parent, &hcp->daifmt[dai->id],
&hp);+}
+static int hdmi_codec_set_fmt(struct snd_soc_dai *dai,
unsigned int fmt)+{
- struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
- struct hdmi_codec_daifmt cf = { 0 };
- int ret = 0;
- dev_dbg(dai->dev, "%s()\n", __func__);
- if (dai->id == DAI_ID_SPDIF) {
cf.fmt = HDMI_SPDIF;- } else {
switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {case SND_SOC_DAIFMT_CBM_CFM:cf.bit_clk_master = 1;cf.frame_clk_master = 1;break;case SND_SOC_DAIFMT_CBS_CFM:cf.frame_clk_master = 1;break;case SND_SOC_DAIFMT_CBM_CFS:cf.bit_clk_master = 1;break;case SND_SOC_DAIFMT_CBS_CFS:break;default:return -EINVAL;}switch (fmt & SND_SOC_DAIFMT_INV_MASK) {case SND_SOC_DAIFMT_NB_NF:break;case SND_SOC_DAIFMT_NB_IF:cf.frame_clk_inv = 1;break;case SND_SOC_DAIFMT_IB_NF:cf.bit_clk_inv = 1;break;case SND_SOC_DAIFMT_IB_IF:cf.frame_clk_inv = 1;cf.bit_clk_inv = 1;break;}switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {case SND_SOC_DAIFMT_I2S:cf.fmt = HDMI_I2S;break;case SND_SOC_DAIFMT_DSP_A:cf.fmt = HDMI_DSP_A;break;case SND_SOC_DAIFMT_DSP_B:cf.fmt = HDMI_DSP_B;break;case SND_SOC_DAIFMT_RIGHT_J:cf.fmt = HDMI_RIGHT_J;break;case SND_SOC_DAIFMT_LEFT_J:cf.fmt = HDMI_LEFT_J;break;case SND_SOC_DAIFMT_AC97:cf.fmt = HDMI_AC97;break;default:dev_err(dai->dev, "Invalid DAI interface format\n");return -EINVAL;}- }
- hcp->daifmt[dai->id] = cf;
- return ret;
+}
+static int hdmi_codec_digital_mute(struct snd_soc_dai *dai, int mute) +{
- struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
- dev_dbg(dai->dev, "%s()\n", __func__);
- if (hcp->hcd.ops->digital_mute)
return hcp->hcd.ops->digital_mute(dai->dev->parent, mute);- return 0;
+}
+static const struct snd_soc_dai_ops hdmi_dai_ops = {
- .startup = hdmi_codec_startup,
- .shutdown = hdmi_codec_shutdown,
- .hw_params = hdmi_codec_hw_params,
- .set_fmt = hdmi_codec_set_fmt,
- .digital_mute = hdmi_codec_digital_mute,
+};
+#define HDMI_RATES (SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |\
SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_88200 |\SNDRV_PCM_RATE_96000 | SNDRV_PCM_RATE_176400 |\SNDRV_PCM_RATE_192000)+#define SPDIF_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE)+/*
- This list is only for formats allowed on the I2S bus. So there is
- some formats listed that are not supported by HDMI interface. For
- instance allowing the 32-bit formats enables 24-precision with CPU
- DAIs that do not support 24-bit formats. If the extra formats cause
- problems, we should add the video side driver an option to disable
- them.
- */
+#define I2S_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE |\SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S32_BE)+static struct snd_soc_dai_driver hdmi_i2s_dai = {
- .name = "i2s-hifi",
- .id = DAI_ID_I2S,
- .playback = {
.stream_name = "Playback",.channels_min = 2,.channels_max = 8,.rates = HDMI_RATES,.formats = I2S_FORMATS,.sig_bits = 24,- },
- .ops = &hdmi_dai_ops,
+};
+static const struct snd_soc_dai_driver hdmi_spdif_dai = {
- .name = "spdif-hifi",
- .id = DAI_ID_SPDIF,
- .playback = {
.stream_name = "Playback",.channels_min = 2,.channels_max = 2,.rates = HDMI_RATES,.formats = SPDIF_FORMATS,- },
- .ops = &hdmi_dai_ops,
+};
+static struct snd_soc_codec_driver hdmi_codec = {
- .dapm_widgets = hdmi_widgets,
- .num_dapm_widgets = ARRAY_SIZE(hdmi_widgets),
- .dapm_routes = hdmi_routes,
- .num_dapm_routes = ARRAY_SIZE(hdmi_routes),
+};
+static int hdmi_codec_probe(struct platform_device *pdev) +{
- struct hdmi_codec_pdata *hcd = pdev->dev.platform_data;
- struct device *dev = &pdev->dev;
- struct hdmi_codec_priv *hcp;
- int dai_count, i = 0;
- int ret;
- dev_dbg(dev, "%s()\n", __func__);
- if (!hcd) {
dev_err(dev, "%s: No plalform data\n", __func__);return -EINVAL;- }
- dai_count = hcd->i2s + hcd->spdif;
- if (dai_count < 1 || !hcd->ops || !hcd->ops->hw_params ||
!hcd->ops->audio_shutdown) {dev_err(dev, "%s: Invalid parameters\n", __func__);return -EINVAL;- }
- hcp = devm_kzalloc(dev, sizeof(*hcp), GFP_KERNEL);
- if (!hcp)
return -ENOMEM;- hcp->hcd = *hcd;
- mutex_init(&hcp->current_stream_lock);
- hcp->daidrv = devm_kzalloc(dev, dai_count * sizeof(*hcp->daidrv),
GFP_KERNEL);- if (!hcp->daidrv)
return -ENOMEM;- if (hcd->i2s) {
hcp->daidrv[i] = hdmi_i2s_dai;hcp->daidrv[i].playback.channels_max =hcd->max_i2s_channels;i++;- }
- if (hcd->spdif)
hcp->daidrv[i] = hdmi_spdif_dai;- ret = snd_soc_register_codec(dev, &hdmi_codec, hcp->daidrv,
dai_count);- if (ret) {
dev_err(dev, "%s: snd_soc_register_codec() failed (%d)\n",__func__, ret);return ret;- }
- dev_set_drvdata(dev, hcp);
- return 0;
+}
+static int hdmi_codec_remove(struct platform_device *pdev) +{
- snd_soc_unregister_codec(&pdev->dev);
- return 0;
+}
+static struct platform_driver hdmi_codec_driver = {
- .driver = {
.name = HDMI_CODEC_DRV_NAME,- },
- .probe = hdmi_codec_probe,
- .remove = hdmi_codec_remove,
+};
+module_platform_driver(hdmi_codec_driver);
+MODULE_AUTHOR("Jyri Sarha jsarha@ti.com"); +MODULE_DESCRIPTION("HDMI Audio Codec Driver"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:" HDMI_CODEC_DRV_NAME);
Hello Jyri,
+static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params,struct snd_soc_dai *dai)+{
- struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
- struct hdmi_codec_params hp = {
.iec = {.status = { 0 },.subcode = { 0 },.pad = 0,.dig_subframe = { 0 },}- };
- int ret;
- dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__,
params_width(params), params_rate(params),params_channels(params));- ret = snd_pcm_create_iec958_consumer_hw_params(params, hp.iec.status,
sizeof(hp.iec.status));
I ran into an issue during my test. For I2S, an error is returned by create_iec958_consumer for 32-bits stream (SNDRV_PCM_FMTBIT_S32_LE). I suppose that it makes sense to handle 32-bits configuration for I2S (but also SPDIF), to support software IEC958 formating/pre-formating... To fix issue, just need to add 32-bits with 24-bits case in create_iec958_consumer
Regards Arnaud
Add audio abort() callback, that is provided at audio stream start, for video side. This is for video side to use in case there is a pressing need to tear down the audio playback for some reason.
Signed-off-by: Jyri Sarha jsarha@ti.com --- include/sound/hdmi-codec.h | 8 ++++++-- sound/soc/codecs/hdmi-codec.c | 20 +++++++++++++++++++- 2 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h index fc3a481..15fe70f 100644 --- a/include/sound/hdmi-codec.h +++ b/include/sound/hdmi-codec.h @@ -55,10 +55,14 @@ struct hdmi_codec_params {
struct hdmi_codec_ops { /* - * Called when ASoC starts an audio stream setup. + * Called when ASoC starts an audio stream setup. The call + * provides an audio abort callback for stoping an ongoing + * stream from video side driver if the HDMI audio becomes + * unavailable. * Optional */ - int (*audio_startup)(struct device *dev); + int (*audio_startup)(struct device *dev, + void (*abort_cb)(struct device *dev));
/* * Configures HDMI-encoder for audio stream. diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c index bc47b9a..cc08097 100644 --- a/sound/soc/codecs/hdmi-codec.c +++ b/sound/soc/codecs/hdmi-codec.c @@ -47,6 +47,23 @@ enum { DAI_ID_SPDIF, };
+static void hdmi_codec_abort(struct device *dev) +{ + struct hdmi_codec_priv *hcp = dev_get_drvdata(dev); + + dev_dbg(dev, "%s()\n", __func__); + + mutex_lock(&hcp->current_stream_lock); + if (hcp->current_stream && hcp->current_stream->runtime && + snd_pcm_running(hcp->current_stream)) { + dev_info(dev, "HDMI audio playback aborted\n"); + snd_pcm_stream_lock_irq(hcp->current_stream); + snd_pcm_stop(hcp->current_stream, SNDRV_PCM_STATE_DISCONNECTED); + snd_pcm_stream_unlock_irq(hcp->current_stream); + } + mutex_unlock(&hcp->current_stream_lock); +} + static int hdmi_codec_new_stream(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { @@ -78,7 +95,8 @@ static int hdmi_codec_startup(struct snd_pcm_substream *substream, return ret;
if (hcp->hcd.ops->audio_startup) { - ret = hcp->hcd.ops->audio_startup(dai->dev->parent); + ret = hcp->hcd.ops->audio_startup(dai->dev->parent, + hdmi_codec_abort); if (ret) { mutex_lock(&hcp->current_stream_lock); hcp->current_stream = NULL;
From: Jean-Francois Moine moinejf@free.fr
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 Signed-off-by: Jyri Sarha jsarha@ti.com --- .../devicetree/bindings/display/bridge/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/display/bridge/tda998x.txt b/Documentation/devicetree/bindings/display/bridge/tda998x.txt index e9e4bce..35f6a80 100644 --- a/Documentation/devicetree/bindings/display/bridge/tda998x.txt +++ b/Documentation/devicetree/bindings/display/bridge/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 34e3874..6db1663 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__)
@@ -53,6 +54,8 @@ struct tda998x_priv {
struct drm_encoder encoder; struct drm_connector connector; + + struct tda998x_audio audio; };
#define conn_to_tda998x_priv(x) \ @@ -821,6 +824,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 drm_encoder *encoder, int mode) @@ -1199,9 +1204,57 @@ static void tda998x_destroy(struct tda998x_priv *priv)
/* 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; @@ -1309,15 +1362,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..bef1da7 --- /dev/null +++ b/include/sound/tda998x.h @@ -0,0 +1,8 @@ +#ifndef SND_TDA998X_H +#define SND_TDA998X_H + +struct tda998x_audio { + u8 ports[2]; /* AP value */ + u8 port_types[2]; /* AFMT_xxx */ +}; +#endif
On Wed, Feb 17, 2016 at 04:49:05PM +0200, Jyri Sarha wrote:
From: Jean-Francois Moine moinejf@free.fr
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 Signed-off-by: Jyri Sarha jsarha@ti.com
.../devicetree/bindings/display/bridge/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/display/bridge/tda998x.txt b/Documentation/devicetree/bindings/display/bridge/tda998x.txt index e9e4bce..35f6a80 100644 --- a/Documentation/devicetree/bindings/display/bridge/tda998x.txt +++ b/Documentation/devicetree/bindings/display/bridge/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"
This should be implied from the port unit address. In other words, port@0 is defined to be the rgb port. Now, if this is one of several modes for the video port, then that is a different story.
- 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>.
This is not really how reg is intended to be used. Can you explain how this value is determined?
- 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)
Same here.
Rob
On Thu, 18 Feb 2016 08:35:30 -0600 Rob Herring robh@kernel.org wrote:
On Wed, Feb 17, 2016 at 04:49:05PM +0200, Jyri Sarha wrote:
From: Jean-Francois Moine moinejf@free.fr
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 Signed-off-by: Jyri Sarha jsarha@ti.com
.../devicetree/bindings/display/bridge/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/display/bridge/tda998x.txt b/Documentation/devicetree/bindings/display/bridge/tda998x.txt index e9e4bce..35f6a80 100644 --- a/Documentation/devicetree/bindings/display/bridge/tda998x.txt +++ b/Documentation/devicetree/bindings/display/bridge/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"
This should be implied from the port unit address. In other words, port@0 is defined to be the rgb port. Now, if this is one of several modes for the video port, then that is a different story.
- 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>.This is not really how reg is intended to be used. Can you explain how this value is determined?
- 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)Same here.
Hi Rob,
These audio/video port definitions were discussed in the thread http://mailman.alsa-project.org/pipermail/alsa-devel/2015-August/095836.html but, you are right, and as you already said in the thread https://lists.freedesktop.org/archives/dri-devel/2015-September/090544.html 'reg' should not be used here.
I would have changed to 'port-value', but as I will not update the kernel of my Dove Cubox anymore, I will not propose a new version of this patch (and, sorry Jyri, I will not test your patch series).
On Thu, Feb 18, 2016 at 04:18:23PM +0100, Jean-Francois Moine wrote:
On Thu, 18 Feb 2016 08:35:30 -0600 Rob Herring robh@kernel.org wrote:
On Wed, Feb 17, 2016 at 04:49:05PM +0200, Jyri Sarha wrote:
From: Jean-Francois Moine moinejf@free.fr
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 Signed-off-by: Jyri Sarha jsarha@ti.com
.../devicetree/bindings/display/bridge/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/display/bridge/tda998x.txt b/Documentation/devicetree/bindings/display/bridge/tda998x.txt index e9e4bce..35f6a80 100644 --- a/Documentation/devicetree/bindings/display/bridge/tda998x.txt +++ b/Documentation/devicetree/bindings/display/bridge/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"
This should be implied from the port unit address. In other words, port@0 is defined to be the rgb port. Now, if this is one of several modes for the video port, then that is a different story.
- 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>.This is not really how reg is intended to be used. Can you explain how this value is determined?
- 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)Same here.
Hi Rob,
These audio/video port definitions were discussed in the thread http://mailman.alsa-project.org/pipermail/alsa-devel/2015-August/095836.html but, you are right, and as you already said in the thread https://lists.freedesktop.org/archives/dri-devel/2015-September/090544.html 'reg' should not be used here.
However, ePAPR requires that a node name with @n also have a reg property with the same n value. See ePAPR 2.2.1.1. This statement in it seems to be particularly clear on this subject:
"The unit-address must match the first address specified in the reg property of the node. If the node has no reg property, the @ and unit-address must be omitted and the node-name alone differentiates the node from other nodes at the same level in the tree."
On 02/18/16 16:35, Rob Herring wrote:
On Wed, Feb 17, 2016 at 04:49:05PM +0200, Jyri Sarha wrote:
From: Jean-Francois Moine moinejf@free.fr
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 Signed-off-by: Jyri Sarha jsarha@ti.com
.../devicetree/bindings/display/bridge/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/display/bridge/tda998x.txt b/Documentation/devicetree/bindings/display/bridge/tda998x.txt index e9e4bce..35f6a80 100644 --- a/Documentation/devicetree/bindings/display/bridge/tda998x.txt +++ b/Documentation/devicetree/bindings/display/bridge/tda998x.txt @@ -16,6 +16,35 @@ Optional properties:
- video-ports: 24 bits value which defines how the video controlleroutput 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"
This should be implied from the port unit address. In other words, port@0 is defined to be the rgb port. Now, if this is one of several modes for the video port, then that is a different story.
Do you suggest that also the audio i2s and s/p-dif port-types should be coded in the port unit addresses? Something like: port@0 is always rgb, port@1 is i2s, and port@2 is spdif?
Having the port-type information explicitly written serves the purpose keeping the dts files human readable. Is saving couple of bytes this important or is there some other reason to not to have the port-type property?
- 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>.This is not really how reg is intended to be used. Can you explain how this value is determined?
I never liked this unorthodox usage of reg property either. I'll replace the reg -properties with something more explicit.
- 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)Same here.
Rob
On Thu, Feb 25, 2016 at 03:42:50PM +0200, Jyri Sarha wrote:
On 02/18/16 16:35, Rob Herring wrote:
This should be implied from the port unit address. In other words, port@0 is defined to be the rgb port. Now, if this is one of several modes for the video port, then that is a different story.
Do you suggest that also the audio i2s and s/p-dif port-types should be coded in the port unit addresses? Something like: port@0 is always rgb, port@1 is i2s, and port@2 is spdif?
For the audio inputs, the port address corresponds to the input pin. TDA998x devices can have multiple streams routed to the pins, and can select between them.
For example, there may be four I2S data pins and one I2S clock pin. When using stereo, you can select which of the four I2S data pins carries the audio data.
When using SPDIF, there may be two SPDIF inputs, and you can select which SPDIF input is used.
So, "reg" may not be an address in terms of a CPU visible address, but it's an address as far as selecting the appropriate input - and it fits in with the requirements of ePAPR, which are that if you have a unit-address (which is required to distinguish different port nodes) then you must have a matching "reg" property.
I don't particularly like the video node using the RGB routing register value either for the reg property, but I've kept quiet because I have nothing to offer there: again, this comes down to ePAPR requirements and the need to specify multiple "port { }" nodes. You can't have two "port { }" nodes without using a unit-address, and we'd need to chose a unit-address for it which doesn't conflict with the audio ports... so there's a kind of logic to using the RGB routing value, which will never conflict.
On 02/26/16 02:43, Russell King - ARM Linux wrote:
On Thu, Feb 25, 2016 at 03:42:50PM +0200, Jyri Sarha wrote:
On 02/18/16 16:35, Rob Herring wrote:
This should be implied from the port unit address. In other words, port@0 is defined to be the rgb port. Now, if this is one of several modes for the video port, then that is a different story.
Do you suggest that also the audio i2s and s/p-dif port-types should be coded in the port unit addresses? Something like: port@0 is always rgb, port@1 is i2s, and port@2 is spdif?
For the audio inputs, the port address corresponds to the input pin. TDA998x devices can have multiple streams routed to the pins, and can select between them.
For example, there may be four I2S data pins and one I2S clock pin. When using stereo, you can select which of the four I2S data pins carries the audio data.
Sure, but I do not think that would be the usual setup. The only "normal" situation I can think for having a need to have two alternative audio setups would one for i2s and another for s/p-dif. But then again it is possible to come up with a design with multiple alternative audio wirings and it relatively simple handle that in DT binding, so why not.
When using SPDIF, there may be two SPDIF inputs, and you can select which SPDIF input is used.
So, "reg" may not be an address in terms of a CPU visible address, but it's an address as far as selecting the appropriate input - and it fits in with the requirements of ePAPR, which are that if you have a unit-address (which is required to distinguish different port nodes) then you must have a matching "reg" property.
Still I do not see why it is desirable to reuse reg property, when we can introduce new property for describing the audio wiring.
I don't particularly like the video node using the RGB routing register value either for the reg property, but I've kept quiet because I have nothing to offer there: again, this comes down to ePAPR requirements and the need to specify multiple "port { }" nodes. You can't have two "port { }" nodes without using a unit-address, and we'd need to chose a unit-address for it which doesn't conflict with the audio ports... so there's a kind of logic to using the RGB routing value, which will never conflict.
If we after all decide to go with using reg property for audio wiring (and essentially writing the value directly to AP_ENA register), then we could also agree that video port's unit address is always 0 as it corresponds to audio disabled in AP_ENA register and would not collide with any audio "address". Then we could keep the old video-ports property to configure the video wiring. How does this sound?
Best regards, Jyri
ps. Then we still have the alternative of not using the graph/ports binding for audio at all. No other audio driver is currently using that and the graph binding is not particularly well suited for i2s connections as there may be more than two components connected to the same wires and sharing the bandwidth with both TDM and different data wires. But I am not sure if I want to go there any more as I have a feeling that I have been running in circles for couple of years with this now.
On Fri, Feb 26, 2016 at 12:14:44PM +0200, Jyri Sarha wrote:
On 02/26/16 02:43, Russell King - ARM Linux wrote:
On Thu, Feb 25, 2016 at 03:42:50PM +0200, Jyri Sarha wrote:
On 02/18/16 16:35, Rob Herring wrote:
This should be implied from the port unit address. In other words, port@0 is defined to be the rgb port. Now, if this is one of several modes for the video port, then that is a different story.
Do you suggest that also the audio i2s and s/p-dif port-types should be coded in the port unit addresses? Something like: port@0 is always rgb, port@1 is i2s, and port@2 is spdif?
For the audio inputs, the port address corresponds to the input pin. TDA998x devices can have multiple streams routed to the pins, and can select between them.
For example, there may be four I2S data pins and one I2S clock pin. When using stereo, you can select which of the four I2S data pins carries the audio data.
Sure, but I do not think that would be the usual setup. The only "normal" situation I can think for having a need to have two alternative audio setups would one for i2s and another for s/p-dif. But then again it is possible to come up with a design with multiple alternative audio wirings and it relatively simple handle that in DT binding, so why not.
There's another reason: if you want to support 8 channel audio using I2S rather than SPDIF, then you need to use four I2S data inputs. Each I2S data input can support only two channels.
When using SPDIF, there may be two SPDIF inputs, and you can select which SPDIF input is used.
So, "reg" may not be an address in terms of a CPU visible address, but it's an address as far as selecting the appropriate input - and it fits in with the requirements of ePAPR, which are that if you have a unit-address (which is required to distinguish different port nodes) then you must have a matching "reg" property.
Still I do not see why it is desirable to reuse reg property, when we can introduce new property for describing the audio wiring.
Different people have different opinions. Your opinion is just another example of someone holding a different view.
You _have_ to have a unit address, and therefore you _have_ to have a reg property. If you want to use some other property to describe the audio input pin, then you will need to make up a totally ficticious unit-address and reg property for each audio input pin.
That's adding complexity, arguably unnecessary complexity, and making the binding unnecessarily more complex for no good reason.
I don't particularly like the video node using the RGB routing register value either for the reg property, but I've kept quiet because I have nothing to offer there: again, this comes down to ePAPR requirements and the need to specify multiple "port { }" nodes. You can't have two "port { }" nodes without using a unit-address, and we'd need to chose a unit-address for it which doesn't conflict with the audio ports... so there's a kind of logic to using the RGB routing value, which will never conflict.
If we after all decide to go with using reg property for audio wiring (and essentially writing the value directly to AP_ENA register), then we could also agree that video port's unit address is always 0 as it corresponds to audio disabled in AP_ENA register and would not collide with any audio "address". Then we could keep the old video-ports property to configure the video wiring. How does this sound?
Sub-standard :)
This has actually been discussed before. See the thread:
"[PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio"
from January 2015.
On 02/26/16 13:21, Russell King - ARM Linux wrote:
On Fri, Feb 26, 2016 at 12:14:44PM +0200, Jyri Sarha wrote:
On 02/26/16 02:43, Russell King - ARM Linux wrote:
On Thu, Feb 25, 2016 at 03:42:50PM +0200, Jyri Sarha wrote:
On 02/18/16 16:35, Rob Herring wrote:
This should be implied from the port unit address. In other words, port@0 is defined to be the rgb port. Now, if this is one of several modes for the video port, then that is a different story.
Do you suggest that also the audio i2s and s/p-dif port-types should be coded in the port unit addresses? Something like: port@0 is always rgb, port@1 is i2s, and port@2 is spdif?
For the audio inputs, the port address corresponds to the input pin. TDA998x devices can have multiple streams routed to the pins, and can select between them.
For example, there may be four I2S data pins and one I2S clock pin. When using stereo, you can select which of the four I2S data pins carries the audio data.
Sure, but I do not think that would be the usual setup. The only "normal" situation I can think for having a need to have two alternative audio setups would one for i2s and another for s/p-dif. But then again it is possible to come up with a design with multiple alternative audio wirings and it relatively simple handle that in DT binding, so why not.
There's another reason: if you want to support 8 channel audio using I2S rather than SPDIF, then you need to use four I2S data inputs. Each I2S data input can support only two channels.
Yes, but surely in the situation where there is 4 data wires, those should all be seen as a single audio port, as they are all sharing the same bit and frame clock wires. In such a situation I can't see why we would ever want to configure anything less that all four connected wires. ALSA and CPU—DAI driver can then deal with the situations when we only have two channels of data, or the HDMI sink can only play stereo.
If we start to think how to describe multiple alternative endpoints for I have to have a unit address, but do I have to have a reg property? I did not know about such a rule when I already made an alternative binding and implementation that appears to work just fine with multiple ports without reg property, but maybe I have over looked some piece of specification that forbids that.essentially the same entity (=the single i2s port, with up to 4 data wires), then there is no upper limit to the amount of ports we'd need to support.
On the other hand, if there are multiple i2s components sharing the same clock wires (and maybe some data wires with TDM too), then graph-style binding is probably not the best way to describe that kind of setup.
The more I think about this the more I think the graph binding is a bad choice to for i2s connections. For SPDIF the graph works better, but why use something that does not work for every DAI format?
When using SPDIF, there may be two SPDIF inputs, and you can select which SPDIF input is used.
So, "reg" may not be an address in terms of a CPU visible address, but it's an address as far as selecting the appropriate input - and it fits in with the requirements of ePAPR, which are that if you have a unit-address (which is required to distinguish different port nodes) then you must have a matching "reg" property.
Still I do not see why it is desirable to reuse reg property, when we can introduce new property for describing the audio wiring.
Different people have different opinions. Your opinion is just another example of someone holding a different view.
You _have_ to have a unit address, and therefore you _have_ to have a reg property. If you want to use some other property to describe the
I have to have a unit address, but do I have to have a reg property? I did not know about such a rule when I already made an alternative binding and implementation that appears to work just fine with multiple ports without reg property, but maybe I have over looked some piece of specification that forbids that.
audio input pin, then you will need to make up a totally ficticious unit-address and reg property for each audio input pin.
That's adding complexity, arguably unnecessary complexity, and making the binding unnecessarily more complex for no good reason.
I don't particularly like the video node using the RGB routing register value either for the reg property, but I've kept quiet because I have nothing to offer there: again, this comes down to ePAPR requirements and the need to specify multiple "port { }" nodes. You can't have two "port { }" nodes without using a unit-address, and we'd need to chose a unit-address for it which doesn't conflict with the audio ports... so there's a kind of logic to using the RGB routing value, which will never conflict.
If we after all decide to go with using reg property for audio wiring (and essentially writing the value directly to AP_ENA register), then we could also agree that video port's unit address is always 0 as it corresponds to audio disabled in AP_ENA register and would not collide with any audio "address". Then we could keep the old video-ports property to configure the video wiring. How does this sound?
Sub-standard :)
This has actually been discussed before. See the thread:
"[PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio"
from January 2015.
Yes, this started sound strangely familiar :). Well, I've been trying to come up with an upstremable implementation for Beaglebone-Black HDMI audio for almost three years already. At this point I am ready to make almost what ever compromise if I just could get the code in. I just want to explain my point of view.
Luckily the HDMI-codec part is mostly agnostic about these things so maybe I can have life of its own.
Best regards, Jyri
Ok, here is just one more simple alternative for tda998x audio binding. I feel that the graph ports binding for audio does not make sense without a graph based ASoC machine driver implementation. The ASoC simple-card is already here and it so widely used that there is no getting rid of that any time soon. This proposal provides the same functionality as the patch in the root of this thread, but in a simpler way and is equally compatible with simple-card.
The example assumes a dt-include file with integer definitions for TDA998x_SPDIF and TDA998x_I2S.
tda19988 { compatible = "nxp,tda998x"; reg = <0x70>; pinctrl-names = "default", "off"; pinctrl-0 = <&nxp_hdmi_bonelt_pins>; pinctrl-1 = <&nxp_hdmi_bonelt_off_pins>; video-ports = <0x230145>;
/* Required by simple-card. The number of sound-dai cells must agree with the number of audio ports described in audio-ports property. */ #sound-dai-cells = <2>;
/* DAI-format AP_ENA reg value */ audio-ports = < TDA998x_SPDIF 0x04 TDA998x_I2S 0x03>;
port { hdmi_0: endpoint@0 { remote-endpoint = <&lcdc_0>; }; }; };
Russell, Rob? Which one would you prefer, the old graph ports binding or this simpler version?
Best regards, Jyri
On Tue, 1 Mar 2016 16:26:50 +0200 Jyri Sarha jsarha@ti.com wrote:
Ok, here is just one more simple alternative for tda998x audio binding. I feel that the graph ports binding for audio does not make sense without a graph based ASoC machine driver implementation. The ASoC simple-card is already here and it so widely used that there is no getting rid of that any time soon. This proposal provides the same functionality as the patch in the root of this thread, but in a simpler way and is equally compatible with simple-card.
Hi Jyri,
The graph port binding for the tda998x works fine with the simple card driver when multi-codec support is added. See http://mailman.alsa-project.org/pipermail/alsa-devel/2015-January/085855.htm...
On 03/01/16 17:35, Jean-Francois Moine wrote:
On Tue, 1 Mar 2016 16:26:50 +0200 Jyri Sarha jsarha@ti.com wrote:
Ok, here is just one more simple alternative for tda998x audio binding. I feel that the graph ports binding for audio does not make sense without a graph based ASoC machine driver implementation. The ASoC simple-card is already here and it so widely used that there is no getting rid of that any time soon. This proposal provides the same functionality as the patch in the root of this thread, but in a simpler way and is equally compatible with simple-card.
Hi Jyri,
The graph port binding for the tda998x works fine with the simple card driver when multi-codec support is added. See http://mailman.alsa-project.org/pipermail/alsa-devel/2015-January/085855.htm...
I know that it works, I have used it myself until now, but it is not needed and there is no driver that parses audio port endpoints. I see no point specifying something in the binding that is not used and there no specific plan to ever use it.
AFAIU my proposed binding should work equally well with simple-card, with or without multi-codec support.
Best regards, Jyri
On Tue, 1 Mar 2016 17:51:09 +0200 Jyri Sarha jsarha@ti.com wrote:
I know that it works, I have used it myself until now, but it is not needed and there is no driver that parses audio port endpoints. I see no point specifying something in the binding that is not used and there no specific plan to ever use it.
AFAIU my proposed binding should work equally well with simple-card, with or without multi-codec support.
As told many times, the simple card is a pure Linux specific entity. It does not describe any hardware. It should not appear in a DT, or, if it does, its compatible should be "linux, simple-audio-card". Then, how can the other OSs know the links between the audio devices and the audio encoders/connectors?
On the other way, the audio graph does not impose any particular software design. It just describes the links between the different hardware components and each OS is free to implement its own layout.
On 03/01/16 18:16, Jean-Francois Moine wrote:
On Tue, 1 Mar 2016 17:51:09 +0200 Jyri Sarha jsarha@ti.com wrote:
I know that it works, I have used it myself until now, but it is not needed and there is no driver that parses audio port endpoints. I see no point specifying something in the binding that is not used and there no specific plan to ever use it.
AFAIU my proposed binding should work equally well with simple-card, with or without multi-codec support.
As told many times, the simple card is a pure Linux specific entity. It does not describe any hardware. It should not appear in a DT, or, if it does, its compatible should be "linux, simple-audio-card". Then, how can the other OSs know the links between the audio devices and the audio encoders/connectors?
I understand the short comings of simple-card and it's binding. However, the binding is documented and it is feasible to extract the audio connections from a simple-card binding too. In fact it models the I2S connections better than straight out of the box graph binding. Actually a graph is not the best way describe an i2s-bus with multiple DAIs (codec or CPU) connected to it.
On the other way, the audio graph does not impose any particular software design. It just describes the links between the different hardware components and each OS is free to implement its own layout.
That is true. In the most narrow sense the i2s protocol details, or even TDM time-slot selections should not be in the dtb. However, is not feasible to write a generic machine driver that would deduce the ideal audio configuration just based on the i2s wiring between the audio components simply because that is not enough information*. So to put it simply the simple-card is not the perfect solution for the problem, but even with its flaws it is better than straight out of the box graph binding, and it is still entirely feasible to extract all needed information for any audio implementation from that binding.
Still even with my proposed binding there is nothing that prevents adding the graph binding on top of that if it is ever needed.
Best regards, Jyri
* With a complete set of information of all audio wiring and component capabilities, including the analog only components, it would probably be possible to deduce a generic configuration that would work in the most common - simple cases, but let's not go there now.
On Tue, 1 Mar 2016 20:29:17 +0200 Jyri Sarha jsarha@ti.com wrote:
I understand the short comings of simple-card and it's binding. However, the binding is documented and it is feasible to extract the audio connections from a simple-card binding too. In fact it models the I2S connections better than straight out of the box graph binding. Actually a graph is not the best way describe an i2s-bus with multiple DAIs (codec or CPU) connected to it.
I still don't understand your problem. You want something like:
audio-ports = < TDA998x_SPDIF 0x04 TDA998x_I2S 0x03>;
and the graph definition would be:
port@03 { reg = <0x03>; port-type = "audio-i2s"; ... };
port@04 { reg = <0x04>; port-type = "audio-spdif"; ... };
Apart the syntax, I don't really see the difference.
On 03/01/16 21:26, Jean-Francois Moine wrote:
On Tue, 1 Mar 2016 20:29:17 +0200 Jyri Sarha jsarha@ti.com wrote:
I understand the short comings of simple-card and it's binding. However, the binding is documented and it is feasible to extract the audio connections from a simple-card binding too. In fact it models the I2S connections better than straight out of tehe box graph binding. Actually a graph is not the best way describe an i2s-bus with multiple DAIs (codec or CPU) connected to it.
I still don't understand your problem. You want something like:
The problem is adding redundant unused details into binding without any plan of ever using them.
Fundamentally my problem is finding some consensus on the tda998x ASoC implementation. I've been reusing your binding for couple of review rounds and there has been some well justified critique towards it. I feel stupid in pushing forward something that I do not completely agree myself, so I decided to try something else.
audio-ports = < TDA998x_SPDIF 0x04 TDA998x_I2S 0x03>;
and the graph definition would be:
port@03 { reg = <0x03>; port-type = "audio-i2s"; ... };
port@04 { reg = <0x04>; port-type = "audio-spdif"; ... };
Apart the syntax, I don't really see the difference.
Yes, the necessary information is contained in both bindings. I can live with either one of them, but I would prefer my version. Essentially I would just like to move forward.
Best regards, Jyri
On Tue, Mar 01, 2016 at 05:16:30PM +0100, Jean-Francois Moine wrote:
As told many times, the simple card is a pure Linux specific entity. It does not describe any hardware. It should not appear in a DT, or,
The physical integration of audio systems is meaningful hardware that physically exists and matters to software. I am completely fed up of having to go through this, I'm fairly sure I've been through it with you before.
if it does, its compatible should be "linux, simple-audio-card".
Documentation/SubmittingPatches.
Then, how can the other OSs know the links between the audio devices and the audio encoders/connectors?
Documentation/devicetree/bindings/sound/simple-card.txt
On the other way, the audio graph does not impose any particular software design. It just describes the links between the different hardware components and each OS is free to implement its own layout.
So long as there is no effort on actually upstreaming a graph based card that shows realistic signs of getting merged in a useful form it doesn't meaningfully exist. Right now nobody is even trying to do that.
If you are concerned about simple-card being too Linux specific then by all means add board specific bindings, preferably ones that can just be added to simple-card as a compatible string.
Move struct tda998x_audio definition to tda998x_drv.c and remove include/sound/tda998x.h. There is no external use for struct tda998x_audio.
Fix graph parsing to allow ports to be inside a separate "ports"-node as specified in Documentation/devicetree/bindings/graph.txt.
Signed-off-by: Jyri Sarha jsarha@ti.com --- drivers/gpu/drm/i2c/tda998x_drv.c | 59 +++++++++++++++++++++------------------ include/sound/tda998x.h | 8 ------ 2 files changed, 32 insertions(+), 35 deletions(-) delete mode 100644 include/sound/tda998x.h
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 6db1663..b534f94 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -27,10 +27,14 @@ #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__)
+struct tda998x_audio { + u8 ports[2]; /* AP value */ + u8 port_types[2]; /* AFMT_xxx */ +}; + struct tda998x_priv { struct i2c_client *cec; struct i2c_client *hdmi; @@ -1209,9 +1213,10 @@ static int tda998x_parse_ports(struct tda998x_priv *priv, { struct device_node *of_port; const char *port_type; - int ret, audio_index, reg, afmt; + int ret, audio_index, reg, afmt, rgb_initialized;
audio_index = 0; + rgb_initialized = 0; for_each_child_of_node(np, of_port) { if (!of_port->name || of_node_cmp(of_port->name, "port") != 0) @@ -1221,11 +1226,17 @@ static int tda998x_parse_ports(struct tda998x_priv *priv, if (ret < 0) continue; ret = of_property_read_u32(of_port, "reg", ®); + if (ret < 0) { + dev_err(&priv->hdmi->dev, "missing reg for %s\n", + port_type); + return ret; + } 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; + rgb_initialized = 1; } continue; } @@ -1235,11 +1246,6 @@ static int tda998x_parse_ports(struct tda998x_priv *priv, 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; @@ -1248,13 +1254,13 @@ static int tda998x_parse_ports(struct tda998x_priv *priv, priv->audio.port_types[audio_index] = afmt; audio_index++; } - return 0; + return rgb_initialized; }
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; + struct device_node *ports; u32 video; int rev_lo, rev_hi, ret; unsigned short cec_addr; @@ -1364,24 +1370,15 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
/* 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 { - + ports = of_get_child_by_name(np, "ports"); + if (!ports) + ports = of_node_get(np); + /* graph of ports */ + ret = tda998x_parse_ports(priv, ports); + of_node_put(ports); + if (ret < 0) + goto fail; + if (ret == 0) { /* optional video properties */ ret = of_property_read_u32(np, "video-ports", &video); if (ret == 0) { @@ -1390,6 +1387,14 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) priv->vip_cntrl_2 = video; } } + 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; + } }
return 0; diff --git a/include/sound/tda998x.h b/include/sound/tda998x.h deleted file mode 100644 index bef1da7..0000000 --- a/include/sound/tda998x.h +++ /dev/null @@ -1,8 +0,0 @@ -#ifndef SND_TDA998X_H -#define SND_TDA998X_H - -struct tda998x_audio { - u8 ports[2]; /* AP value */ - u8 port_types[2]; /* AFMT_xxx */ -}; -#endif
Declare struct tda998x_audio_params in include/drm/i2c/tda998x.h and use it in pdata and for tda998x_configure_audio() parameters. Also updates tda998x_write_aif() to take struct hdmi_audio_infoframe * directly as a parameter.
Signed-off-by: Jyri Sarha jsarha@ti.com --- drivers/gpu/drm/i2c/tda998x_drv.c | 106 +++++++++++++++++++++----------------- include/drm/i2c/tda998x.h | 23 +++++---- 2 files changed, 73 insertions(+), 56 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index b534f94..42d256a 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -46,7 +46,7 @@ struct tda998x_priv { u8 vip_cntrl_0; u8 vip_cntrl_1; u8 vip_cntrl_2; - struct tda998x_encoder_params params; + struct tda998x_audio_params audio_params;
wait_queue_head_t wq_edid; volatile int wq_edid_wait; @@ -673,26 +673,16 @@ tda998x_write_if(struct tda998x_priv *priv, u8 bit, u16 addr, reg_set(priv, REG_DIP_IF_FLAGS, bit); }
-static void -tda998x_write_aif(struct tda998x_priv *priv, struct tda998x_encoder_params *p) +static int tda998x_write_aif(struct tda998x_priv *priv, + struct hdmi_audio_infoframe *cea) { union hdmi_infoframe frame;
- hdmi_audio_infoframe_init(&frame.audio); - - frame.audio.channels = p->audio_frame[1] & 0x07; - frame.audio.channel_allocation = p->audio_frame[4]; - frame.audio.level_shift_value = (p->audio_frame[5] & 0x78) >> 3; - frame.audio.downmix_inhibit = (p->audio_frame[5] & 0x80) >> 7; - - /* - * L-PCM and IEC61937 compressed audio shall always set sample - * frequency to "refer to stream". For others, see the HDMI - * specification. - */ - frame.audio.sample_frequency = (p->audio_frame[2] & 0x1c) >> 2; + frame.audio = *cea;
tda998x_write_if(priv, DIP_IF_FLAGS_IF4, REG_IF4_HB0, &frame); + + return 0; }
static void @@ -717,19 +707,20 @@ static void tda998x_audio_mute(struct tda998x_priv *priv, bool on) } }
-static void +static int tda998x_configure_audio(struct tda998x_priv *priv, - struct drm_display_mode *mode, struct tda998x_encoder_params *p) + struct tda998x_audio_params *params, + unsigned mode_clock) { u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv; u32 n;
/* Enable audio ports */ - reg_write(priv, REG_ENA_AP, p->audio_cfg); - reg_write(priv, REG_ENA_ACLK, p->audio_clk_cfg); + reg_write(priv, REG_ENA_AP, params->config); + reg_write(priv, REG_ENA_ACLK, params->format == AFMT_SPDIF ? 0 : 1);
/* Set audio input source */ - switch (p->audio_format) { + switch (params->format) { case AFMT_SPDIF: reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_SPDIF); clksel_aip = AIP_CLKSEL_AIP_SPDIF; @@ -741,12 +732,25 @@ tda998x_configure_audio(struct tda998x_priv *priv, reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S); clksel_aip = AIP_CLKSEL_AIP_I2S; clksel_fs = AIP_CLKSEL_FS_ACLK; - cts_n = CTS_N_M(3) | CTS_N_K(3); + switch (params->sample_width) { + case 16: + cts_n = CTS_N_M(3) | CTS_N_K(1); + break; + case 18: + case 20: + case 24: + cts_n = CTS_N_M(3) | CTS_N_K(2); + break; + default: + case 32: + cts_n = CTS_N_M(3) | CTS_N_K(3); + break; + } break;
default: BUG(); - return; + return -EINVAL; }
reg_write(priv, REG_AIP_CLKSEL, clksel_aip); @@ -762,11 +766,11 @@ tda998x_configure_audio(struct tda998x_priv *priv, * assume 100MHz requires larger divider. */ adiv = AUDIO_DIV_SERCLK_8; - if (mode->clock > 100000) + if (mode_clock > 100000) adiv++; /* AUDIO_DIV_SERCLK_16 */
/* S/PDIF asks for a larger divider */ - if (p->audio_format == AFMT_SPDIF) + if (params->format == AFMT_SPDIF) adiv++; /* AUDIO_DIV_SERCLK_16 or _32 */
reg_write(priv, REG_AUDIO_DIV, adiv); @@ -775,7 +779,7 @@ tda998x_configure_audio(struct tda998x_priv *priv, * This is the approximate value of N, which happens to be * the recommended values for non-coherent clocks. */ - n = 128 * p->audio_sample_rate / 1000; + n = 128 * params->sample_rate / 1000;
/* Write the CTS and N values */ buf[0] = 0x44; @@ -794,19 +798,13 @@ tda998x_configure_audio(struct tda998x_priv *priv, reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
/* Write the channel status */ - buf[0] = IEC958_AES0_CON_NOT_COPYRIGHT; - buf[1] = 0x00; - buf[2] = IEC958_AES3_CON_FS_NOTID; - buf[3] = IEC958_AES4_CON_ORIGFS_NOTID | - IEC958_AES4_CON_MAX_WORDLEN_24; - reg_write_range(priv, REG_CH_STAT_B(0), buf, 4); + reg_write_range(priv, REG_CH_STAT_B(0), params->status, 4);
tda998x_audio_mute(priv, true); msleep(20); tda998x_audio_mute(priv, false);
- /* Write the audio information packet */ - tda998x_write_aif(priv, p); + return tda998x_write_aif(priv, ¶ms->cea); }
/* DRM encoder functions */ @@ -827,9 +825,7 @@ static void tda998x_encoder_set_config(struct tda998x_priv *priv, VIP_CNTRL_2_SWAP_F(p->swap_f) | (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; + priv->audio_params = p->audio; }
static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) @@ -1074,9 +1070,11 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
tda998x_write_avi(priv, adjusted_mode);
- if (priv->params.audio_cfg) - tda998x_configure_audio(priv, adjusted_mode, - &priv->params); + if (priv->audio_params.config) { + tda998x_configure_audio(priv, + &priv->audio_params, + adjusted_mode->clock); + } } }
@@ -1388,12 +1386,28 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) } } 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; + struct tda998x_audio_params params = { + .config = priv->audio.ports[0], + .format = priv->audio.port_types[0], + .sample_width = 24, + .sample_rate = 44100, + .cea = { + .channels = 2, + .coding_type = HDMI_AUDIO_CODING_TYPE_STREAM, + .sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM, + .sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM, + }, + .status = { + IEC958_AES0_CON_NOT_COPYRIGHT, + IEC958_AES1_CON_GENERAL, + IEC958_AES2_CON_SOURCE_UNSPEC | + IEC958_AES2_CON_CHANNEL_UNSPEC, + IEC958_AES3_CON_CLOCK_1000PPM | + IEC958_AES3_CON_FS_NOTID, + }, + }; + + priv->audio_params = params; } }
diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h index 3e419d9..dfe7829 100644 --- a/include/drm/i2c/tda998x.h +++ b/include/drm/i2c/tda998x.h @@ -1,6 +1,18 @@ #ifndef __DRM_I2C_TDA998X_H__ #define __DRM_I2C_TDA998X_H__
+struct tda998x_audio_params { + u8 config; + enum { + AFMT_SPDIF, + AFMT_I2S + } format; + unsigned sample_width; + unsigned sample_rate; + struct hdmi_audio_infoframe cea; + u8 status[4]; +}; + struct tda998x_encoder_params { u8 swap_b:3; u8 mirr_b:1; @@ -15,16 +27,7 @@ struct tda998x_encoder_params { u8 swap_e:3; u8 mirr_e:1;
- u8 audio_cfg; - u8 audio_clk_cfg; - u8 audio_frame[6]; - - enum { - AFMT_SPDIF, - AFMT_I2S - } audio_format; - - unsigned audio_sample_rate; + struct tda998x_audio_params audio; };
#endif
Register ASoC HDMI codec for audio functionality. This is an initial ASoC audio implementation for tda998x driver and it does not use all the features provided by hdmi-codec.
HDMI audio info-frame and audio stream header is generated by the ASoC HDMI codec. The codec also applies constraints for available sample-rates.
Implementation of audio_startup for hdmi_codec_ops would enable tda998x driver to abort ongoing playback if the HDMI cable is unplugged or re-plugged to a device without audio capability.
Signed-off-by: Jyri Sarha jsarha@ti.com --- drivers/gpu/drm/i2c/Kconfig | 1 + drivers/gpu/drm/i2c/tda998x_drv.c | 179 +++++++++++++++++++++++++++++++------- include/drm/i2c/tda998x.h | 1 + 3 files changed, 148 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig index 22c7ed6..088f278 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_HDMI_CODEC if SND_SOC 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 42d256a..b3b9559 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -20,6 +20,7 @@ #include <linux/module.h> #include <linux/irq.h> #include <sound/asoundef.h> +#include <sound/hdmi-codec.h>
#include <drm/drmP.h> #include <drm/drm_atomic_helper.h> @@ -30,9 +31,9 @@
#define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
-struct tda998x_audio { - u8 ports[2]; /* AP value */ - u8 port_types[2]; /* AFMT_xxx */ +struct tda998x_audio_port { + u8 format; /* AFMT_xxx */ + u8 config; /* AP value */ };
struct tda998x_priv { @@ -48,6 +49,8 @@ struct tda998x_priv { u8 vip_cntrl_2; struct tda998x_audio_params audio_params;
+ struct platform_device *audio_pdev; + wait_queue_head_t wq_edid; volatile int wq_edid_wait;
@@ -59,7 +62,7 @@ struct tda998x_priv { struct drm_encoder encoder; struct drm_connector connector;
- struct tda998x_audio audio; + struct tda998x_audio_port audio_port[2]; };
#define conn_to_tda998x_priv(x) \ @@ -749,7 +752,7 @@ tda998x_configure_audio(struct tda998x_priv *priv, break;
default: - BUG(); + dev_err(&priv->hdmi->dev, "Unsupported I2S format\n"); return -EINVAL; }
@@ -1070,7 +1073,7 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
tda998x_write_avi(priv, adjusted_mode);
- if (priv->audio_params.config) { + if (priv->audio_params.format != AFMT_UNUSED) { tda998x_configure_audio(priv, &priv->audio_params, adjusted_mode->clock); @@ -1174,6 +1177,8 @@ static int tda998x_connector_get_modes(struct drm_connector *connector) drm_mode_connector_update_edid_property(connector, edid); n = drm_add_edid_modes(connector, edid); priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid); + drm_edid_to_eld(connector, edid); + kfree(edid);
return n; @@ -1195,6 +1200,9 @@ static void tda998x_destroy(struct tda998x_priv *priv) cec_write(priv, REG_CEC_RXSHPDINTENA, 0); reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
+ if (priv->audio_pdev) + platform_device_unregister(priv->audio_pdev); + if (priv->hdmi->irq) free_irq(priv->hdmi->irq, priv);
@@ -1204,6 +1212,133 @@ static void tda998x_destroy(struct tda998x_priv *priv) i2c_unregister_device(priv->cec); }
+static int tda998x_audio_hw_params(struct device *dev, + struct hdmi_codec_daifmt *daifmt, + struct hdmi_codec_params *params) +{ + struct tda998x_priv *priv = dev_get_drvdata(dev); + int i, ret; + struct tda998x_audio_params audio = { + .sample_width = params->sample_width, + .sample_rate = params->sample_rate, + .cea = params->cea, + }; + + if (!priv->encoder.crtc) + return -ENODEV; + + memcpy(audio.status, params->iec.status, + min(sizeof(audio.status), sizeof(params->iec.status))); + + switch (daifmt->fmt) { + case HDMI_I2S: + if (daifmt->bit_clk_inv || daifmt->frame_clk_inv || + daifmt->bit_clk_master || daifmt->frame_clk_master) { + dev_err(dev, "%s: Bad flags %d %d %d %d\n", __func__, + daifmt->bit_clk_inv, daifmt->frame_clk_inv, + daifmt->bit_clk_master, + daifmt->frame_clk_master); + return -EINVAL; + } + for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) + if (priv->audio_port[i].format == AFMT_I2S) + audio.config = priv->audio_port[i].config; + audio.format = AFMT_I2S; + break; + case HDMI_SPDIF: + for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) + if (priv->audio_port[i].format == AFMT_SPDIF) + audio.config = priv->audio_port[i].config; + audio.format = AFMT_SPDIF; + break; + default: + dev_err(dev, "%s: Invalid format %d\n", __func__, daifmt->fmt); + return -EINVAL; + } + + if (audio.config == 0) { + dev_err(dev, "%s: No audio configutation found\n", __func__); + return -EINVAL; + } + + ret = tda998x_configure_audio(priv, + &audio, + priv->encoder.crtc->hwmode.clock); + + return ret; +} + +static void tda998x_audio_shutdown(struct device *dev) +{ + struct tda998x_priv *priv = dev_get_drvdata(dev); + + reg_write(priv, REG_ENA_AP, 0); +} + +int tda998x_audio_digital_mute(struct device *dev, bool enable) +{ + struct tda998x_priv *priv = dev_get_drvdata(dev); + + tda998x_audio_mute(priv, enable); + + return 0; +} + +static int tda998x_audio_get_eld(struct device *dev, uint8_t *buf, size_t len) +{ + struct tda998x_priv *priv = dev_get_drvdata(dev); + struct drm_mode_config *config = &priv->encoder.dev->mode_config; + struct drm_connector *connector; + int ret = -ENODEV; + + mutex_lock(&config->mutex); + list_for_each_entry(connector, &config->connector_list, head) { + if (&priv->encoder == connector->encoder) { + memcpy(buf, connector->eld, + min(sizeof(connector->eld), len)); + ret = 0; + } + } + mutex_unlock(&config->mutex); + + return ret; +} + +static const struct hdmi_codec_ops audio_codec_ops = { + .hw_params = tda998x_audio_hw_params, + .audio_shutdown = tda998x_audio_shutdown, + .digital_mute = tda998x_audio_digital_mute, + .get_eld = tda998x_audio_get_eld, +}; + +static int tda998x_audio_codec_init(struct tda998x_priv *priv, + struct device *dev) +{ + struct hdmi_codec_pdata codec_data = { + .ops = &audio_codec_ops, + .max_i2s_channels = 2, + }; + int i; + + for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) { + if (priv->audio_port[i].format == AFMT_I2S && + priv->audio_port[i].config != 0) + codec_data.i2s = 1; + if (priv->audio_port[i].format == AFMT_SPDIF && + priv->audio_port[i].config != 0) + codec_data.spdif = 1; + } + + priv->audio_pdev = platform_device_register_data( + dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO, + &codec_data, sizeof(codec_data)); + + if (IS_ERR(priv->audio_pdev)) + return PTR_ERR(priv->audio_pdev); + + return 0; +} + /* I2C driver functions */
static int tda998x_parse_ports(struct tda998x_priv *priv, @@ -1244,12 +1379,12 @@ static int tda998x_parse_ports(struct tda998x_priv *priv, afmt = AFMT_SPDIF; else continue; - if (audio_index >= ARRAY_SIZE(priv->audio.ports)) { + if (audio_index >= ARRAY_SIZE(priv->audio_port)) { dev_err(&priv->hdmi->dev, "too many audio ports\n"); break; } - priv->audio.ports[audio_index] = reg; - priv->audio.port_types[audio_index] = afmt; + priv->audio_port[audio_index].format = afmt; + priv->audio_port[audio_index].config = reg; audio_index++; } return rgb_initialized; @@ -1385,30 +1520,8 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) priv->vip_cntrl_2 = video; } } - if (priv->audio.ports[0]) { - struct tda998x_audio_params params = { - .config = priv->audio.ports[0], - .format = priv->audio.port_types[0], - .sample_width = 24, - .sample_rate = 44100, - .cea = { - .channels = 2, - .coding_type = HDMI_AUDIO_CODING_TYPE_STREAM, - .sample_size = HDMI_AUDIO_SAMPLE_SIZE_STREAM, - .sample_frequency = HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM, - }, - .status = { - IEC958_AES0_CON_NOT_COPYRIGHT, - IEC958_AES1_CON_GENERAL, - IEC958_AES2_CON_SOURCE_UNSPEC | - IEC958_AES2_CON_CHANNEL_UNSPEC, - IEC958_AES3_CON_CLOCK_1000PPM | - IEC958_AES3_CON_FS_NOTID, - }, - }; - - priv->audio_params = params; - } + if (priv->audio_port[0].format != AFMT_UNUSED) + tda998x_audio_codec_init(priv, &client->dev); }
return 0; diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h index dfe7829..5a0cabb 100644 --- a/include/drm/i2c/tda998x.h +++ b/include/drm/i2c/tda998x.h @@ -4,6 +4,7 @@ struct tda998x_audio_params { u8 config; enum { + AFMT_UNUSED = 0, AFMT_SPDIF, AFMT_I2S } format;
Add HDMI audio support. Adds mcasp0_pins, clk_mcasp0_fixed, clk_mcasp0, mcasp0, sound node, and updates the tda19988 node to follow the new binding.
Signed-off-by: Jyri Sarha jsarha@ti.com --- arch/arm/boot/dts/am335x-boneblack.dts | 90 ++++++++++++++++++++++++++++++++-- 1 file changed, 86 insertions(+), 4 deletions(-)
diff --git a/arch/arm/boot/dts/am335x-boneblack.dts b/arch/arm/boot/dts/am335x-boneblack.dts index eadbba3..05347dc 100644 --- a/arch/arm/boot/dts/am335x-boneblack.dts +++ b/arch/arm/boot/dts/am335x-boneblack.dts @@ -64,6 +64,16 @@ 0x1b0 0x03 /* xdma_event_intr0, OMAP_MUX_MODE3 | AM33XX_PIN_OUTPUT */ >; }; + + mcasp0_pins: mcasp0_pins { + pinctrl-single,pins = < + 0x1ac (PIN_INPUT_PULLUP | MUX_MODE0) /* mcasp0_ahclkx.mcasp0_ahclkx */ + 0x19c (PIN_OUTPUT_PULLDOWN | MUX_MODE2) /* mcasp0_ahclkr.mcasp0_axr2 */ + 0x194 (PIN_OUTPUT_PULLUP | MUX_MODE0) /* mcasp0_fsx.mcasp0_fsx */ + 0x190 (PIN_OUTPUT_PULLDOWN | MUX_MODE0) /* mcasp0_aclkx.mcasp0_aclkx */ + 0x06c (PIN_OUTPUT_PULLDOWN | MUX_MODE7) /* gpmc_a11.GPIO1_27 */ + >; + }; };
&lcdc { @@ -76,17 +86,89 @@ };
&i2c0 { - tda19988 { + tda19988: tda19988 { compatible = "nxp,tda998x"; reg = <0x70>; + + #sound-dai-cells = <0>; + pinctrl-names = "default", "off"; pinctrl-0 = <&nxp_hdmi_bonelt_pins>; pinctrl-1 = <&nxp_hdmi_bonelt_off_pins>;
- port { - hdmi_0: endpoint@0 { - remote-endpoint = <&lcdc_0>; + ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + port-type = "rgb"; + reg = <0x230145>; + hdmi_0: endpoint@0 { + remote-endpoint = <&lcdc_0>; + }; }; + port@1 { + port-type = "i2s"; + reg = <0x03>; + tda19988_i2s: endpoint { + remote-endpoint = <&mcasp0_i2s>; + }; + }; + }; + }; +}; + +&rtc { + system-power-controller; +}; + +&mcasp0 { + #sound-dai-cells = <0>; + pinctrl-names = "default"; + pinctrl-0 = <&mcasp0_pins>; + status = "okay"; + op-mode = <0>; /* MCASP_IIS_MODE */ + tdm-slots = <2>; + serial-dir = < /* 0: INACTIVE, 1: TX, 2: RX */ + 0 0 1 0 + >; + tx-num-evt = <32>; + rx-num-evt = <32>; + + port { + mcasp0_i2s: endpoint { + remote-endpoint = <&tda19988_i2s>; + }; + }; +}; + +/ { + clk_mcasp0_fixed: clk_mcasp0_fixed { + #clock-cells = <0>; + compatible = "fixed-clock"; + clock-frequency = <24576000>; + }; + + clk_mcasp0: clk_mcasp0 { + #clock-cells = <0>; + compatible = "gpio-gate-clock"; + clocks = <&clk_mcasp0_fixed>; + enable-gpios = <&gpio1 27 0>; /* BeagleBone Black Clk enable on GPIO1_27 */ + }; + + sound { + compatible = "simple-audio-card"; + simple-audio-card,name = "TI BeagleBone Black"; + simple-audio-card,format = "i2s"; + simple-audio-card,bitclock-master = <&dailink0_master>; + simple-audio-card,frame-master = <&dailink0_master>; + + dailink0_master: simple-audio-card,cpu { + sound-dai = <&mcasp0>; + clocks = <&clk_mcasp0>; + }; + + simple-audio-card,codec { + sound-dai = <&tda19988>; }; }; };
On Wed, Feb 17, 2016 at 8:49 AM, Jyri Sarha jsarha@ti.com wrote:
Add HDMI audio support. Adds mcasp0_pins, clk_mcasp0_fixed, clk_mcasp0, mcasp0, sound node, and updates the tda19988 node to follow the new binding.
Signed-off-by: Jyri Sarha jsarha@ti.com
arch/arm/boot/dts/am335x-boneblack.dts | 90 ++++++++++++++++++++++++++++++++-- 1 file changed, 86 insertions(+), 4 deletions(-)
diff --git a/arch/arm/boot/dts/am335x-boneblack.dts b/arch/arm/boot/dts/am335x-boneblack.dts index eadbba3..05347dc 100644 --- a/arch/arm/boot/dts/am335x-boneblack.dts +++ b/arch/arm/boot/dts/am335x-boneblack.dts @@ -64,6 +64,16 @@ 0x1b0 0x03 /* xdma_event_intr0, OMAP_MUX_MODE3 | AM33XX_PIN_OUTPUT */ >; };
mcasp0_pins: mcasp0_pins {pinctrl-single,pins = <0x1ac (PIN_INPUT_PULLUP | MUX_MODE0) /* mcasp0_ahclkx.mcasp0_ahclkx */0x19c (PIN_OUTPUT_PULLDOWN | MUX_MODE2) /* mcasp0_ahclkr.mcasp0_axr2 */0x194 (PIN_OUTPUT_PULLUP | MUX_MODE0) /* mcasp0_fsx.mcasp0_fsx */0x190 (PIN_OUTPUT_PULLDOWN | MUX_MODE0) /* mcasp0_aclkx.mcasp0_aclkx */0x06c (PIN_OUTPUT_PULLDOWN | MUX_MODE7) /* gpmc_a11.GPIO1_27 */
Hi Jyri,
We've switched to AM33XX_IOPAD, please squash this in here:
AM33XX_IOPAD(0x9ac, PIN_INPUT_PULLUP | MUX_MODE0) /* mcasp0_ahcklx.mcasp0_ahclkx */ AM33XX_IOPAD(0x99c, PIN_OUTPUT_PULLDOWN | MUX_MODE2) /* mcasp0_ahclkr.mcasp0_axr2*/ AM33XX_IOPAD(0x994, PIN_OUTPUT_PULLUP | MUX_MODE0) /* mcasp0_fsx.mcasp0_fsx */ AM33XX_IOPAD(0x990, PIN_OUTPUT_PULLDOWN | MUX_MODE0) /* mcasp0_aclkx.mcasp0_aclkx */ AM33XX_IOPAD(0x86c, PIN_OUTPUT_PULLDOWN | MUX_MODE7) /* gpmc_a11.GPIO1_27 */
Regards,
dri-devel@lists.freedesktop.org
-
Arnaud Pouliquen -
Jean-Francois Moine -
Jyri Sarha -
Mark Brown -
Rob Herring -
Robert Nelson -
Russell King - ARM Linux