On 09/24/14 08:49, Jean-Francois Moine wrote:

The audio constraints of the HDMI interface are defined by the EDID which is sent by the connected device.

The HDMI transmitters may have one or many audio sources.

This patch adds two functions to the HDMI CODEC:

• it updates the audio constraints from the EDID,
• it gives the audio source type to the HDMI transmitter on start of audio streaming.

Signed-off-by: Jean-Francois Moine moinejf@free.fr

include/sound/hdmi.h | 20 ++++++ sound/soc/codecs/hdmi.c | 174 ++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 188 insertions(+), 6 deletions(-) create mode 100644 include/sound/hdmi.h

diff --git a/include/sound/hdmi.h b/include/sound/hdmi.h new file mode 100644 index 0000000..49062c7 --- /dev/null +++ b/include/sound/hdmi.h @@ -0,0 +1,20 @@ +#ifndef SND_HDMI_H +#define SND_HDMI_H

+#include <sound/soc.h>

+/* platform_data */ +struct hdmi_data {

• int (*get_audio)(struct device *dev,

• 	 int *max_channels,
  

• 	 int *rate_mask,
  

• 	 int *fmt);
  

• void (*audio_switch)(struct device *dev,

• 	     int port_index,
  

• 	     unsigned sample_rate,
  

• 	     int sample_format);
  

• int ndais;
• struct snd_soc_dai_driver *dais;
• struct snd_soc_codec_driver *driver;

+}; +#endif diff --git a/sound/soc/codecs/hdmi.c b/sound/soc/codecs/hdmi.c index 1087fd5..6ea2772 100644 --- a/sound/soc/codecs/hdmi.c +++ b/sound/soc/codecs/hdmi.c @@ -22,9 +22,146 @@ #include <sound/soc.h> #include <linux/of.h> #include <linux/of_device.h> +#include <sound/pcm_params.h> +#include <sound/hdmi.h>

#define DRV_NAME "hdmi-audio-codec"

+struct hdmi_priv {

• struct hdmi_data hdmi_data;
• struct snd_pcm_hw_constraint_list rate_constraints;

+};

+static int hdmi_dev_match(struct device *dev, void *data) +{

• return !strcmp(dev_name(dev), (char *) data);

+}

+/* get the codec device */ +static struct device *hdmi_get_cdev(struct device *dev) +{

• struct device *cdev;
• cdev = device_find_child(dev,

• 		 DRV_NAME,
  

• 		 hdmi_dev_match);
  

• if (!cdev)

• dev_err(dev, "Cannot get codec device");
  

• else

• put_device(cdev);
  

• return cdev;

+}

+static int hdmi_startup(struct snd_pcm_substream *substream,

• 	struct snd_soc_dai *dai)
  

+{

• struct snd_pcm_runtime *runtime = substream->runtime;
• struct device *cdev;
• struct hdmi_priv *priv;
• struct snd_pcm_hw_constraint_list *rate_constraints;
• int ret, max_channels, rate_mask, fmt;
• u64 formats;
• static const u32 hdmi_rates[] = {

• 32000, 44100, 48000, 88200, 96000, 176400, 192000
  

• };
• cdev = hdmi_get_cdev(dai->dev);
• if (!cdev)

• return -ENODEV;
  

• priv = dev_get_drvdata(cdev);

It might be a good idea to qualify priv (and perhaps get_audio and audio_switch) here? This might be that the relevant (function) pointers are non-NULL.

Andrew

• /* get the EDID values and the rate constraints buffer */
• ret = priv->hdmi_data.get_audio(dai->dev,

• 			&max_channels, &rate_mask, &fmt);
  

• if (ret < 0)

• return ret;				/* no screen */
  

• /* convert the EDID values to audio constraints */
• rate_constraints = &priv->rate_constraints;
• rate_constraints->list = hdmi_rates;
• rate_constraints->count = ARRAY_SIZE(hdmi_rates);
• rate_constraints->mask = rate_mask;
• snd_pcm_hw_constraint_list(runtime, 0,

• 		   SNDRV_PCM_HW_PARAM_RATE,
  

• 		   rate_constraints);
  

• formats = 0;
• if (fmt & 1)

• formats |= SNDRV_PCM_FMTBIT_S16_LE;
  

• if (fmt & 2)

• formats |= SNDRV_PCM_FMTBIT_S20_3LE;
  

• if (fmt & 4)

• formats |= SNDRV_PCM_FMTBIT_S24_LE;
  

• snd_pcm_hw_constraint_mask64(runtime,

• 		SNDRV_PCM_HW_PARAM_FORMAT,
  

• 		formats);
  

• snd_pcm_hw_constraint_minmax(runtime,

• 		SNDRV_PCM_HW_PARAM_CHANNELS,
  

• 		1, max_channels);
  

• return 0;

+}

+static int hdmi_hw_params(struct snd_pcm_substream *substream,

• 	  struct snd_pcm_hw_params *params,
  

• 	  struct snd_soc_dai *dai)
  

+{

• struct device *cdev;
• struct hdmi_priv *priv;
• cdev = hdmi_get_cdev(dai->dev);
• if (!cdev)

• return -ENODEV;
  

• priv = dev_get_drvdata(cdev);
• priv->hdmi_data.audio_switch(dai->dev, dai->id,

• 		     params_rate(params),
  

• 		     params_format(params));
  

• return 0;

+}

+static void hdmi_shutdown(struct snd_pcm_substream *substream,

• 	  struct snd_soc_dai *dai)
  

+{

• struct device *cdev;
• struct hdmi_priv *priv;
• cdev = hdmi_get_cdev(dai->dev);
• if (!cdev)

• return;
  

• priv = dev_get_drvdata(cdev);
• priv->hdmi_data.audio_switch(dai->dev, -1, 0, 0); /* stop */

+}

+static const struct snd_soc_dai_ops hdmi_ops = {

• .startup = hdmi_startup,
• .hw_params = hdmi_hw_params,
• .shutdown = hdmi_shutdown,

+};

+static int hdmi_codec_probe(struct snd_soc_codec *codec) +{

• struct hdmi_priv *priv;
• struct device *dev = codec->dev; /* encoder device */
• struct device *cdev; /* codec device */
• cdev = hdmi_get_cdev(dev);
• if (!cdev)

• return -ENODEV;
  

• /* allocate some memory to store

• * the encoder callback functions and the rate constraints */
  

• priv = devm_kzalloc(cdev, sizeof *priv, GFP_KERNEL);
• if (!priv)

• return -ENOMEM;
  

• dev_set_drvdata(cdev, priv);
• memcpy(&priv->hdmi_data, cdev->platform_data,

• 		sizeof priv->hdmi_data);
  

• return 0;

+}

static const struct snd_soc_dapm_widget hdmi_widgets[] = { SND_SOC_DAPM_INPUT("RX"), SND_SOC_DAPM_OUTPUT("TX"), @@ -77,13 +214,38 @@ static struct snd_soc_codec_driver hdmi_codec = { .num_dapm_routes = ARRAY_SIZE(hdmi_routes), };

-static int hdmi_codec_probe(struct platform_device *pdev) +static int hdmi_codec_dev_probe(struct platform_device *pdev) {

• return snd_soc_register_codec(&pdev->dev, &hdmi_codec,

• 	&hdmi_codec_dai, 1);
  

• struct hdmi_data *pdata = pdev->dev.platform_data;
• struct snd_soc_dai_driver *dais;
• struct snd_soc_codec_driver *driver;
• int i, ret;
• if (!pdata)

• return snd_soc_register_codec(&pdev->dev, &hdmi_codec,
  

• 				&hdmi_codec_dai, 1);
  

• /* creation from a video encoder as a child device */
• dais = devm_kmemdup(&pdev->dev,

• 	    pdata->dais,
  

• 	    sizeof *pdata->dais * pdata->ndais,
  

• 	    GFP_KERNEL);
  

• for (i = 0; i < pdata->ndais; i++)

• dais[i].ops = &hdmi_ops;
  

• driver = devm_kmemdup(&pdev->dev,

• 	    pdata->driver,
  

• 	    sizeof *pdata->driver,
  

• 	    GFP_KERNEL);
  

• driver->probe = hdmi_codec_probe;
• /* register the codec on the video encoder */
• ret = snd_soc_register_codec(pdev->dev.parent, driver,

• 			dais, pdata->ndais);
  

• return ret;

}

-static int hdmi_codec_remove(struct platform_device *pdev) +static int hdmi_codec_dev_remove(struct platform_device *pdev) { snd_soc_unregister_codec(&pdev->dev); return 0; @@ -96,8 +258,8 @@ static struct platform_driver hdmi_codec_driver = { .of_match_table = of_match_ptr(hdmi_audio_codec_ids), },

• .probe = hdmi_codec_probe,
• .remove = hdmi_codec_remove,

• .probe = hdmi_codec_dev_probe,
• .remove = hdmi_codec_dev_remove,

};

module_platform_driver(hdmi_codec_driver);

On Wed, 1 Oct 2014 17:04:12 +0300 Jyri Sarha jsarha@ti.com wrote:

On 09/24/2014 10:49 AM, Jean-Francois Moine wrote:> The audio constraints of the HDMI interface are defined by the EDID

...

which is sent by the connected device.

The HDMI transmitters may have one or many audio sources.

This patch adds two functions to the HDMI CODEC:

• it updates the audio constraints from the EDID,
• it gives the audio source type to the HDMI transmitter on start of audio streaming.

Signed-off-by: Jean-Francois Moine moinejf@free.fr

include/sound/hdmi.h | 20 ++++++ sound/soc/codecs/hdmi.c | 174

++++++++++++++++++++++++++++++++++++++++++++++--

...

2 files changed, 188 insertions(+), 6 deletions(-) create mode 100644 include/sound/hdmi.h

diff --git a/include/sound/hdmi.h b/include/sound/hdmi.h new file mode 100644 index 0000000..49062c7 --- /dev/null +++ b/include/sound/hdmi.h @@ -0,0 +1,20 @@ +#ifndef SND_HDMI_H +#define SND_HDMI_H

+#include <sound/soc.h>

+/* platform_data */ +struct hdmi_data {

• int (*get_audio)(struct device *dev,

• 	 int *max_channels,
  

• 	 int *rate_mask,
  

• 	 int *fmt);
  

This looks good for the api to HDMI ASoC codec.

...

• void (*audio_switch)(struct device *dev,

• 	     int port_index,
  

• 	     unsigned sample_rate,
  

• 	     int sample_format);
  

I see nothing about selecting the i2s format. I do not know too much about tda998x registers but sii9022 can select left and right justified mode, bit-clock and frame-clock polarity etc. Should we need a callback that corresponds to snd_soc_dai_ops .set_fmt ?

SiI9022 also uses an external clock as some kind of reference for audio and that needs to be divided to right ballpark in relation sample-rate (I have no idea why this is, I am just talking about experience). Should we need callback that corresponds to snd_soc_dai_ops .set_sysclk?

Maybe the above two could also be left for later.

The HDMI CODEC does not interact with the audio controller. The audio_switch function just tells the tda998x which is the audio source and it gives the parameters which impact the HDMI transmission. The modes, clock... must be defined in the audio card definition (simple-card or specific card).

About the AIF, this one is sent only once in the tda998x driver. The values 'refer to stream header' should work in most cases.

The audio source is offered to the HDMI CODEC by the DAIs. There are 2 DAIs: the first one is S/PDIF and the second is I2S. If your audio controller delivers I2S only, the audio card shall define only this one. With a DT (simple-card), this is:

sound { compatible = "simple-audio-card"; ... simple-audio-card,dai-link { /* I2S - HDMI */ cpu { sound-dai = <&audio 0>; }; codec { sound-dai = <&hdmi 1>; /* tda998x 2nd DAI */ }; }; };

Without DT, the DAI link is defined the same way:

.codec_name = "tda998x.0-0070"; .codec_dai_name = "i2s-hifi";

...

• int ndais;
• struct snd_soc_dai_driver *dais;
• struct snd_soc_codec_driver *driver;

+};

The need to include sound/soc.h is something I was trying to avoid in my early design for a similar generic HDMI ASoC codec. I came up with this requirement from Mark's comments about my OMAP HDMI audio patches. So the bellow suggestion are really up to Mark.

If I understand right, the need for ndais, dais, and driver is coming solely from spdif/i2s switching change. Since at least I am not aware of any other DAIs found in HDMI encoders, could we just give the info whether i2s and/or spdif is supporter with a simple bit-field or enum? The snd_soc_dai_driver and snd_soc_codec_driver definitions could then be all put to ASoC side of source tree.

I have also been wondering why we do not have SND_SOC_DAIFMT_SPDIF definition in sound/soc-dai.h. With that there would be no need to define two different dais for spdif and i2s. But this is more a suggestion for ASoC core future development. Because I do not currently have any HW to experiment with this the suggestion should probably be left only as a side note for now.

The kirkwood audio subsystem which is used in the Cubox also has 2 audio outputs, I2S and S/PDIF. The audio route 'I2S --> HDMI I2S' or 'S/PDIF --> HDMI S/PDIF + optical S/PDIF' is chosen by the application (PCM).

Then, the audio constraints are not the same on the audio controller and HDMI transmitter sides. There must be a full description of these constraints in the HDMI CODEC. I was thinking to build the DAIs from a reduced description in the TDA998x transmitter, but, as all parameters are required, it was simpler to include sound/soc.h.

...

+#endif diff --git a/sound/soc/codecs/hdmi.c b/sound/soc/codecs/hdmi.c index 1087fd5..6ea2772 100644 --- a/sound/soc/codecs/hdmi.c +++ b/sound/soc/codecs/hdmi.c @@ -22,9 +22,146 @@ #include <sound/soc.h> #include <linux/of.h> #include <linux/of_device.h> +#include <sound/pcm_params.h> +#include <sound/hdmi.h>

#define DRV_NAME "hdmi-audio-codec"

+struct hdmi_priv {

• struct hdmi_data hdmi_data;
• struct snd_pcm_hw_constraint_list rate_constraints;

+};

+static int hdmi_dev_match(struct device *dev, void *data) +{

• return !strcmp(dev_name(dev), (char *) data);

+}

+/* get the codec device */ +static struct device *hdmi_get_cdev(struct device *dev) +{

• struct device *cdev;
• cdev = device_find_child(dev,

• 		 DRV_NAME,
  

• 		 hdmi_dev_match);
  

• if (!cdev)

• dev_err(dev, "Cannot get codec device");
  

• else

• put_device(cdev);
  

• return cdev;

+}

+static int hdmi_startup(struct snd_pcm_substream *substream,

• 	struct snd_soc_dai *dai)
  

+{

• struct snd_pcm_runtime *runtime = substream->runtime;
• struct device *cdev;
• struct hdmi_priv *priv;
• struct snd_pcm_hw_constraint_list *rate_constraints;
• int ret, max_channels, rate_mask, fmt;
• u64 formats;
• static const u32 hdmi_rates[] = {

• 32000, 44100, 48000, 88200, 96000, 176400, 192000
  

• };
• cdev = hdmi_get_cdev(dai->dev);
• if (!cdev)

• return -ENODEV;
  

• priv = dev_get_drvdata(cdev);
• /* get the EDID values and the rate constraints buffer */
• ret = priv->hdmi_data.get_audio(dai->dev,

• 			&max_channels, &rate_mask, &fmt);
  

• if (ret < 0)

• return ret;				/* no screen */
  

• /* convert the EDID values to audio constraints */
• rate_constraints = &priv->rate_constraints;
• rate_constraints->list = hdmi_rates;
• rate_constraints->count = ARRAY_SIZE(hdmi_rates);
• rate_constraints->mask = rate_mask;
• snd_pcm_hw_constraint_list(runtime, 0,

• 		   SNDRV_PCM_HW_PARAM_RATE,
  

• 		   rate_constraints);
  

• formats = 0;
• if (fmt & 1)

• formats |= SNDRV_PCM_FMTBIT_S16_LE;
  

• if (fmt & 2)

• formats |= SNDRV_PCM_FMTBIT_S20_3LE;
  

• if (fmt & 4)

• formats |= SNDRV_PCM_FMTBIT_S24_LE;
  

I think we should add here: formats |= SNDRV_PCM_FMTBIT_S24_3LE; and probably also formats |= SNDRV_PCM_FMTBIT_S32_LE;

The S24_3LE is equal to S24_LE at i2s level and the 32bit format would help to get better than 16bit audio working with if there is trouble supporting 24bit formats (like I have with wip sii9022 code). Both tda998x and sii9022 are able to play 32bit i2s audio just fine. The 8 least significant bits are naturally ignored, but we can set .sig_bits = 24 in snd_soc_pcm_stream.

OK.

...

• snd_pcm_hw_constraint_mask64(runtime,

• 		SNDRV_PCM_HW_PARAM_FORMAT,
  

• 		formats);
  

• snd_pcm_hw_constraint_minmax(runtime,

• 		SNDRV_PCM_HW_PARAM_CHANNELS,
  

• 		1, max_channels);
  

• return 0;

+}

+static int hdmi_hw_params(struct snd_pcm_substream *substream,

• 	  struct snd_pcm_hw_params *params,
  

• 	  struct snd_soc_dai *dai)
  

+{

• struct device *cdev;
• struct hdmi_priv *priv;
• cdev = hdmi_get_cdev(dai->dev);
• if (!cdev)

• return -ENODEV;
  

• priv = dev_get_drvdata(cdev);
• priv->hdmi_data.audio_switch(dai->dev, dai->id,

• 		     params_rate(params),
  

• 		     params_format(params));
  

• return 0;

+}

+static void hdmi_shutdown(struct snd_pcm_substream *substream,

• 	  struct snd_soc_dai *dai)
  

+{

• struct device *cdev;
• struct hdmi_priv *priv;
• cdev = hdmi_get_cdev(dai->dev);
• if (!cdev)

• return;
  

• priv = dev_get_drvdata(cdev);
• priv->hdmi_data.audio_switch(dai->dev, -1, 0, 0); /* stop */

+}

+static const struct snd_soc_dai_ops hdmi_ops = {

• .startup = hdmi_startup,
• .hw_params = hdmi_hw_params,
• .shutdown = hdmi_shutdown,

+};

+static int hdmi_codec_probe(struct snd_soc_codec *codec) +{

• struct hdmi_priv *priv;
• struct device *dev = codec->dev; /* encoder device */
• struct device *cdev; /* codec device */
• cdev = hdmi_get_cdev(dev);
• if (!cdev)

• return -ENODEV;
  

• /* allocate some memory to store

• * the encoder callback functions and the rate constraints */
  

• priv = devm_kzalloc(cdev, sizeof *priv, GFP_KERNEL);
• if (!priv)

• return -ENOMEM;
  

• dev_set_drvdata(cdev, priv);
• memcpy(&priv->hdmi_data, cdev->platform_data,

• 		sizeof priv->hdmi_data);
  

• return 0;

+}

• static const struct snd_soc_dapm_widget hdmi_widgets[] = { SND_SOC_DAPM_INPUT("RX"), SND_SOC_DAPM_OUTPUT("TX"),

@@ -77,13 +214,38 @@ static struct snd_soc_codec_driver hdmi_codec = { .num_dapm_routes = ARRAY_SIZE(hdmi_routes), };

-static int hdmi_codec_probe(struct platform_device *pdev) +static int hdmi_codec_dev_probe(struct platform_device *pdev) {

• return snd_soc_register_codec(&pdev->dev, &hdmi_codec,

• 	&hdmi_codec_dai, 1);
  

• struct hdmi_data *pdata = pdev->dev.platform_data;
• struct snd_soc_dai_driver *dais;
• struct snd_soc_codec_driver *driver;
• int i, ret;
• if (!pdata)

• return snd_soc_register_codec(&pdev->dev, &hdmi_codec,
  

• 				&hdmi_codec_dai, 1);
  

• /* creation from a video encoder as a child device */
• dais = devm_kmemdup(&pdev->dev,

• 	    pdata->dais,
  

• 	    sizeof *pdata->dais * pdata->ndais,
  

• 	    GFP_KERNEL);
  

• for (i = 0; i < pdata->ndais; i++)

• dais[i].ops = &hdmi_ops;
  

• driver = devm_kmemdup(&pdev->dev,

• 	    pdata->driver,
  

• 	    sizeof *pdata->driver,
  

• 	    GFP_KERNEL);
  

• driver->probe = hdmi_codec_probe;
• /* register the codec on the video encoder */
• ret = snd_soc_register_codec(pdev->dev.parent, driver,

• 			dais, pdata->ndais);
  

Registering the codec under the tda998x driver makes me wonder why would we need the a separate platform device in the first palce. At least it makes it possible to unify the old dummy HDMI codec with the new code, but I am not even sure if that makes eny sense.

The CODEC platform device is needed for 2 reasons: - it permits the HDMI transmitter to give the callback functions and the DAIs to the CODEC, - the CODEC needs to memorize the callback functions and the rate constraints, but, as a CODEC, it has no private data, and, on the other side, the drvdata of the HDMI transmitter is already used.

Maybe we should have another dummy codec to be used in the situations where you really just need a dummy endpoint to satisfy ASoC. AFAIK the only setups that use the current hdmi codec are BBB HDMI audio and OMAP4+ HDMI audio, which both are currently broken in the upstream.

My patch does not break the dummy codec as soon as the device does not get a platform_data at probe time.

...

• return ret; }

-static int hdmi_codec_remove(struct platform_device *pdev) +static int hdmi_codec_dev_remove(struct platform_device *pdev) { snd_soc_unregister_codec(&pdev->dev); return 0; @@ -96,8 +258,8 @@ static struct platform_driver hdmi_codec_driver = { .of_match_table = of_match_ptr(hdmi_audio_codec_ids), },

• .probe = hdmi_codec_probe,
• .remove = hdmi_codec_remove,

• .probe = hdmi_codec_dev_probe,

• .remove = hdmi_codec_dev_remove, };

  module_platform_driver(hdmi_codec_driver);

What comes to compatibility to the old use of hdmi codec everything appears to be fine after this patch.

On 09/24/2014 11:11 AM, Jean-Francois Moine wrote:

This patch interfaces the HDMI transmitter with the audio system.

Signed-off-by: Jean-Francois Moine moinejf@free.fr

.../devicetree/bindings/drm/i2c/tda998x.txt | 18 ++ drivers/gpu/drm/i2c/Kconfig | 1 + drivers/gpu/drm/i2c/tda998x_drv.c | 299

+++++++++++++++++++--

3 files changed, 300 insertions(+), 18 deletions(-)

diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt

b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt

index e9e4bce..e50e7cd 100644 --- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt @@ -17,6 +17,20 @@ Optional properties: - video-ports: 24 bits value which defines how the video controller output is wired to the TDA998x input - default: <0x230145>

• • audio-ports: must contain one or two values selecting the source

• in the audio port.

• The source type is given by the corresponding entry in

• the audio-port-names property.

• • audio-port-names: must contain entries matching the entries in

• the audio-ports property.

• Each value may be "i2s" or "spdif", giving the type of

• the audio source.

• • #sound-dai-cells: must be set to <1> for use with the simple-card.

• The TDA998x audio CODEC always defines two DAIs.

• The DAI 0 is the S/PDIF input and the DAI 1 is the I2S input.

• Example:

  tda998x: hdmi-encoder {

@@ -26,4 +40,8 @@ Example: interrupts = <27 2>; /* falling edge */ pinctrl-0 = <&pmx_camera>; pinctrl-names = "default";

• audio-ports = <0x04>, <0x03>;
  

• audio-port-names = "spdif", "i2s";
  

• #sound-dai-cells = <1>;
  

  };

diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig index 4d341db..42ca744 100644 --- a/drivers/gpu/drm/i2c/Kconfig +++ b/drivers/gpu/drm/i2c/Kconfig @@ -22,6 +22,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 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 d476279..66c41c0 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -20,12 +20,14 @@ #include <linux/module.h> #include <linux/irq.h> #include <sound/asoundef.h> +#include <linux/platform_device.h>

#include <drm/drmP.h> #include <drm/drm_crtc_helper.h> #include <drm/drm_encoder_slave.h> #include <drm/drm_edid.h> #include <drm/i2c/tda998x.h> +#include <sound/hdmi.h>

#define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)

@@ -44,6 +46,22 @@ struct tda998x_priv { wait_queue_head_t wq_edid; volatile int wq_edid_wait; struct drm_encoder *encoder;

• /* audio variables */
• struct platform_device *pdev_codec;
• u8 audio_ports[2];
• u8 max_channels; /* EDID parameters */
• u8 rate_mask;
• u8 fmt;
• int audio_sample_format;

+};

+struct tda998x_priv2 {

• struct tda998x_priv base;

• struct drm_encoder encoder;

• struct drm_connector connector; };

  #define to_tda998x_priv(x) ((struct tda998x_priv

*)to_encoder_slave(x)->slave_priv)

@@ -624,6 +642,8 @@ tda998x_write_avi(struct tda998x_priv *priv,

struct drm_display_mode *mode)

	 sizeof(buf));

}

+/* audio functions */

• static void tda998x_audio_mute(struct tda998x_priv *priv, bool on) { if (on) {

@@ -639,12 +659,11 @@ static void tda998x_configure_audio(struct tda998x_priv *priv, struct drm_display_mode *mode, struct tda998x_encoder_params *p) {

• uint8_t buf[6], clksel_aip, clksel_fs, cts_n, adiv;

• uint8_t buf[6], clksel_aip, clksel_fs, cts_n, adiv, aclk; uint32_t n;

  /* Enable audio ports */ reg_write(priv, REG_ENA_AP, p->audio_cfg);

• reg_write(priv, REG_ENA_ACLK, p->audio_clk_cfg);

  /* Set audio input source */ switch (p->audio_format) {

@@ -653,13 +672,29 @@ tda998x_configure_audio(struct tda998x_priv *priv, clksel_aip = AIP_CLKSEL_AIP_SPDIF; clksel_fs = AIP_CLKSEL_FS_FS64SPDIF; cts_n = CTS_N_M(3) | CTS_N_K(3);

• aclk = 0;				/* no clock */
  

  break;

  case AFMT_I2S: reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S); clksel_aip = AIP_CLKSEL_AIP_I2S; clksel_fs = AIP_CLKSEL_FS_ACLK;

• cts_n = CTS_N_M(3) | CTS_N_K(3);
  

• /* with I2S input, the CTS_N predivider depends on
  

•  * the sample width */
  

• switch (priv->audio_sample_format) {
  

• case SNDRV_PCM_FORMAT_S16_LE:
  

• 	cts_n = CTS_N_M(3) | CTS_N_K(1);
  

• 	break;
  

• case SNDRV_PCM_FORMAT_S24_LE:
  

• 	cts_n = CTS_N_M(3) | CTS_N_K(2);
  

• 	break;
  

• default:
  

Setting the default here does not really help, because priv->audio_sample_format is initialized to SNDRV_PCM_FORMAT_S24_LE in tda998x_encoder_set_config(). But I am Ok with the default being changed for 24 bit samples on i2s interface.

• case SNDRV_PCM_FORMAT_S32_LE:
  

• 	cts_n = CTS_N_M(3) | CTS_N_K(3);
  

• 	break;
  

• }
  

• aclk = 1;				/* clock enable */
  

  break;

  default:

@@ -671,6 +706,7 @@ tda998x_configure_audio(struct tda998x_priv *priv, reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_LAYOUT | AIP_CNTRL_0_ACR_MAN); /* auto CTS */ reg_write(priv, REG_CTS_N, cts_n);

• reg_write(priv, REG_ENA_ACLK, aclk);

  /*

  • Audio input somehow depends on HDMI line rate which is

@@ -727,6 +763,144 @@ tda998x_configure_audio(struct tda998x_priv *priv, tda998x_write_aif(priv, p); }

+/* audio codec interface */

+/* return the audio parameters extracted from the last EDID */ +static int tda998x_get_audio(struct device *dev,

• 	int *max_channels,
  

• 	int *rate_mask,
  

• 	int *fmt)
  

+{

• struct tda998x_priv2 *priv2 = dev_get_drvdata(dev);
• struct tda998x_priv *priv = &priv2->base;
• if (!priv->encoder->crtc)

• return -ENODEV;
  

• *max_channels = priv->max_channels;
• *rate_mask = priv->rate_mask;
• *fmt = priv->fmt;
• return 0;

+}

+/* switch the audio port and initialize the audio parameters for

streaming */

+static void tda998x_audio_switch(struct device *dev,

• 		 int port_index,
  

• 		 unsigned sample_rate,
  

• 		 int sample_format)
  

+{

• struct tda998x_priv2 *priv2 = dev_get_drvdata(dev);
• struct tda998x_priv *priv = &priv2->base;
• struct tda998x_encoder_params *p = &priv->params;
• if (!priv->encoder->crtc)

• return;
  

• /*

• * if port_index is negative (streaming stop),
  

• * disable the audio port
  

• */
  

• if (port_index < 0) {

• reg_write(priv, REG_ENA_AP, 0);
  

• return;
  

• }
• /* if same audio parameters, just enable the audio port */
• if (p->audio_cfg == priv->audio_ports[port_index] &&

•    p->audio_sample_rate == sample_rate &&
  

•    priv->audio_sample_format == sample_format) {
  

• reg_write(priv, REG_ENA_AP, p->audio_cfg);
  

• return;
  

• }
• p->audio_format = port_index;
• p->audio_cfg = priv->audio_ports[port_index];
• p->audio_sample_rate = sample_rate;
• priv->audio_sample_format = sample_format;
• tda998x_configure_audio(priv, &priv->encoder->crtc->hwmode, p);

+}

+#define TDA998X_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \

• 	SNDRV_PCM_FMTBIT_S20_3LE | \
  

• 	SNDRV_PCM_FMTBIT_S24_LE | \
  

• 	SNDRV_PCM_FMTBIT_S32_LE)
  

+static struct snd_soc_dai_driver tda998x_dais[] = {

• {

• .name = "spdif-hifi",
  

• .id = AFMT_SPDIF,
  

• .playback = {
  

• 	.stream_name	= "HDMI SPDIF Playback",
  

• 	.channels_min	= 1,
  

• 	.channels_max	= 2,
  

• 	.rates		= SNDRV_PCM_RATE_CONTINUOUS,
  

• 	.rate_min	= 22050,
  

• 	.rate_max	= 192000,
  

• 	.formats	= TDA998X_FORMATS,
  

• },
  

• },
• {

• .name = "i2s-hifi",
  

• .id = AFMT_I2S,
  

• .playback = {
  

• 	.stream_name	= "HDMI I2S Playback",
  

• 	.channels_min	= 1,
  

• 	.channels_max	= 8,
  

• 	.rates		= SNDRV_PCM_RATE_CONTINUOUS,
  

• 	.rate_min	= 5512,
  

• 	.rate_max	= 192000,
  

• 	.formats	= TDA998X_FORMATS,
  

• },
  

• },

+};

+static const struct snd_soc_dapm_widget tda998x_widgets[] = {

• SND_SOC_DAPM_OUTPUT("hdmi-out"),

+}; +static const struct snd_soc_dapm_route tda998x_routes[] = {

• { "hdmi-out", NULL, "HDMI I2S Playback" },
• { "hdmi-out", NULL, "HDMI SPDIF Playback" },

+};

+static struct snd_soc_codec_driver tda998x_codec_driver = {

• .dapm_widgets = tda998x_widgets,
• .num_dapm_widgets = ARRAY_SIZE(tda998x_widgets),
• .dapm_routes = tda998x_routes,
• .num_dapm_routes = ARRAY_SIZE(tda998x_routes),

+};

+static struct hdmi_data tda998x_hdmi_data = {

• .get_audio = tda998x_get_audio,
• .audio_switch = tda998x_audio_switch,
• .ndais = ARRAY_SIZE(tda998x_dais),
• .dais = tda998x_dais,
• .driver = &tda998x_codec_driver,

+};

+static void tda998x_create_audio_codec(struct tda998x_priv *priv) +{

• struct platform_device *pdev;
• struct module *module;
• request_module("snd-soc-hdmi-codec");
• pdev = platform_device_register_resndata(&priv->hdmi->dev,

• 				 "hdmi-audio-codec",
  

• 				  PLATFORM_DEVID_NONE,
  

• 				  NULL, 0,
  

• 				  &tda998x_hdmi_data,
  

• 				  sizeof tda998x_hdmi_data);
  

• if (IS_ERR(pdev)) {

• dev_err(&priv->hdmi->dev, "cannot create codec: %ld\n",
  

• 	PTR_ERR(pdev));
  

• return;
  

• }
• priv->pdev_codec = pdev;
• module = pdev->dev.driver->owner;
• if (module)

• try_module_get(module);
  

+}

• /* DRM encoder functions */

  static void tda998x_encoder_set_config(struct tda998x_priv *priv,

@@ -746,6 +920,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_ports[p->audio_format] = p->audio_cfg;
• priv->audio_sample_format = SNDRV_PCM_FORMAT_S24_LE;

See the previous comment.

}

static void tda998x_encoder_dpms(struct tda998x_priv *priv, int mode) @@ -1128,6 +1304,47 @@ fail: return NULL; }

+static void tda998x_set_audio(struct tda998x_priv *priv,

• 	      struct drm_connector *connector)
  

+{

• u8 *eld = connector->eld;
• u8 *sad;
• int sad_count;
• unsigned eld_ver, mnl, rate_mask;
• unsigned max_channels, fmt;
• /* adjust the hw params from the ELD (EDID) */
• eld_ver = eld[0] >> 3;
• if (eld_ver != 2 && eld_ver != 31)

• return;
  

• mnl = eld[4] & 0x1f;
• if (mnl > 16)

• return;
  

• sad_count = eld[5] >> 4;
• sad = eld + 20 + mnl;
• /* Start from the basic audio settings */
• max_channels = 2;
• rate_mask = 0;
• fmt = 0;
• while (sad_count--) {

• switch (sad[0] & 0x78) {
  

• case 0x08: /* PCM */
  

• 	max_channels = max(max_channels, (sad[0] & 7) + 1u);
  

• 	rate_mask |= sad[1];
  

• 	fmt |= sad[2] & 0x07;
  

• 	break;
  

• }
  

• sad += 3;
  

• }
• priv->max_channels = max_channels;
• priv->rate_mask = rate_mask;
• priv->fmt = fmt;

+}

• static int tda998x_encoder_get_modes(struct tda998x_priv *priv, struct drm_connector *connector)

@@ -1139,6 +1356,12 @@ tda998x_encoder_get_modes(struct tda998x_priv

*priv,

drm_mode_connector_update_edid_property(connector, edid);
n = drm_add_edid_modes(connector, edid);
priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid);

• /* set the audio parameters from the EDID */
  

• if (priv->is_hdmi_sink) {
  

• 	drm_edid_to_eld(connector, edid);
  

• 	tda998x_set_audio(priv, connector);
  

• }
  

  kfree(edid); }

@@ -1167,12 +1390,19 @@ tda998x_encoder_set_property(struct

drm_encoder *encoder,

static void tda998x_destroy(struct tda998x_priv *priv) {

• struct module *module;

• /* disable all IRQs and free the IRQ handler */ cec_write(priv, REG_CEC_RXSHPDINTENA, 0); reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD); if (priv->hdmi->irq) free_irq(priv->hdmi->irq, priv);

• if (priv->pdev_codec) {

• module = priv->pdev_codec->dev.driver->owner;
  

• module_put(module);
  

• platform_device_del(priv->pdev_codec);
  

• } i2c_unregister_device(priv->cec); }

@@ -1254,12 +1484,16 @@ static int tda998x_create(struct i2c_client

*client, struct tda998x_priv *priv)

{ struct device_node *np = client->dev.of_node; u32 video;

• int rev_lo, rev_hi, ret;

• int i, j, rev_lo, rev_hi, ret;

• const char *p;

  priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3); priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1); priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);

• priv->params.audio_frame[1] = 1; /* channels - 1 */

• priv->params.audio_sample_rate = 48000; /* 48kHz */

• priv->current_page = 0xff; priv->hdmi = client; priv->cec = i2c_new_dummy(client->adapter, 0x34);

@@ -1351,17 +1585,48 @@ 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 device tree parameters */
• if (np) {

• /* 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;
  

• /* 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;
  

• }
  

• /* audio properties */
  

• for (i = 0; i < 2; i++) {
  

• 	u32 port;
  

• 	ret = of_property_read_u32_index(np, "audio-ports", i, &port);
  

• 	if (ret)
  

• 		break;
  

• 	ret = of_property_read_string_index(np, "audio-port-names",
  

• 					i, &p);
  

• 	if (ret) {
  

• 		dev_err(&client->dev,
  

• 			"missing audio-port-names[%d]\n", i);
  

• 		break;
  

• 	}
  

• 	if (strcmp(p, "spdif") == 0) {
  

• 		j = AFMT_SPDIF;
  

• 	} else if (strcmp(p, "i2s") == 0) {
  

• 		j = AFMT_I2S;
  

• 	} else {
  

• 		dev_err(&client->dev,
  

• 			"bad audio-port-names '%s'\n", p);
  

• 		break;
  

• 	}
  

• 	priv->audio_ports[j] = port;
  

• }
  

  }

• /* create the audio CODEC */

• if (priv->audio_ports[AFMT_SPDIF] || priv->audio_ports[AFMT_I2S])

• tda998x_create_audio_codec(priv);
  

• return 0;

  fail:

@@ -1395,15 +1660,13 @@ static int tda998x_encoder_init(struct

i2c_client *client,

encoder_slave->slave_priv = priv; encoder_slave->slave_funcs = &tda998x_encoder_slave_funcs;

• /* set the drvdata pointer to priv2 for CODEC calls */
• dev_set_drvdata(&client->dev,

• 	container_of(priv, struct tda998x_priv2, base));
  

• return 0; }

-struct tda998x_priv2 {

• struct tda998x_priv base;
• struct drm_encoder encoder;
• struct drm_connector connector;

-};

• #define conn_to_tda998x_priv2(x) \ container_of(x, struct tda998x_priv2, connector);

The only audio side change in the platform data usage of tda998x_drv I can see is the change in the default value of CTS_N.

Best regards, Jyri

On Wed, Sep 24, 2014 at 10:11:21AM +0200, Jean-Francois Moine wrote:

This patch interfaces the HDMI transmitter with the audio system.

...

+struct tda998x_priv2 {

• struct tda998x_priv base;
• struct drm_encoder encoder;
• struct drm_connector connector;

};

NAK on moving this up here. It was specifically placed below the core and below the older drm_slave_encoder code because nothing but the new component based interfaces to TDA998x have any business knowing that these are packaged up in this way.

+static void tda998x_create_audio_codec(struct tda998x_priv *priv) +{

• struct platform_device *pdev;
• struct module *module;
• request_module("snd-soc-hdmi-codec");
• pdev = platform_device_register_resndata(&priv->hdmi->dev,

• 				 "hdmi-audio-codec",
  

• 				  PLATFORM_DEVID_NONE,
  

• 				  NULL, 0,
  

• 				  &tda998x_hdmi_data,
  

• 				  sizeof tda998x_hdmi_data);
  

• if (IS_ERR(pdev)) {

• dev_err(&priv->hdmi->dev, "cannot create codec: %ld\n",
  

• 	PTR_ERR(pdev));
  

• return;
  

• }
• priv->pdev_codec = pdev;
• module = pdev->dev.driver->owner;
• if (module)

• try_module_get(module);
  

This is really not on. Firstly, registering a platform device has no guarantee that it will bind to a driver. So, pdev->dev.driver may be NULL here -> kernel oops.

Secondly, what's the purpose of the unchecked try_module_get() there? You can't stop the device being unbound from its driver, so all you're doing is possibly locking the module into module space for no other benefit.

@@ -1167,12 +1390,19 @@ tda998x_encoder_set_property(struct drm_encoder *encoder,

static void tda998x_destroy(struct tda998x_priv *priv) {

• struct module *module;

• /* disable all IRQs and free the IRQ handler */ cec_write(priv, REG_CEC_RXSHPDINTENA, 0); reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD); if (priv->hdmi->irq) free_irq(priv->hdmi->irq, priv);

• if (priv->pdev_codec) {

• module = priv->pdev_codec->dev.driver->owner;
  

• module_put(module);
  

This is wrong for all the reasons the other part above is wrong. Userspace can decide to unbind the platform device from the associated platform driver whenever it wishes. At that point, priv->pdev_codec->dev.driver becomes NULL.

So, please get rid of this.

• platform_device_del(priv->pdev_codec);
  

This leaks the memory associated with the platform device.

@@ -1395,15 +1660,13 @@ static int tda998x_encoder_init(struct i2c_client *client, encoder_slave->slave_priv = priv; encoder_slave->slave_funcs = &tda998x_encoder_slave_funcs;

• /* set the drvdata pointer to priv2 for CODEC calls */
• dev_set_drvdata(&client->dev,

• 	container_of(priv, struct tda998x_priv2, base));
  

• return 0;

}

-struct tda998x_priv2 {

• struct tda998x_priv base;
• struct drm_encoder encoder;
• struct drm_connector connector;

-};

I would prefer this structure to stay here, as code above this point should have no business knowing how these are packaged together. I would suggest either:

- moving the audio codec code below this point, or - storing struct tda998x_priv in the device private pointer, and converting it to struct tda998x_priv2 via container_of() where necessary below this point.