[PATCH v6 0/2] ASoC: tda998x: add a codec to the HDMI transmitter

[PATCH v6 0/2] ASoC: tda998x: add a codec to the HDMI transmitter

[Jean-Francois Moine]

The NXP TDA998x HDMI transmitter may transmit audio to the HDMI link
from 2 different sources, I2S and S/PDIF.

This patch set adds an interface between the HDMI transmitter and
the HDMI CODEC.

The interface is used by the TDA998x driver to update the audio
constraints from the display characteristics (EDID) and by the audio
subsystem to connect the chosen audio source to the HDMI link.

v6:
	- extend the HDMI CODEC instead of using a specific CODEC
v5:
	- use the TDA998x private data instead of a specific area
	  for the CODEC interface
	- the CODEC is TDA998x specific (Mark Brown)
v4:
	- remove all the TDA998x specific stuff from the CODEC
	- move the EDID scan from the CODEC to the TDA998x
	- move the CODEC to sound/soc (Mark Brown)
	- update the audio_sample_rate from the EDID (Andrew Jackson)
v3: fix bad rate (Andrew Jackson)
v2: check double stream start (Mark Brown)

Jean-Francois Moine (2):
  ASoC:codecs: Add a transmitter interface to the HDMI CODEC
  drm/i2c:tda998x: Use the HDMI audio CODEC

 .../devicetree/bindings/drm/i2c/tda998x.txt        |  18 ++
 drivers/gpu/drm/i2c/Kconfig                        |   1 +
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 299 +++++++++++++++++++--
 include/sound/hdmi.h                               |  20 ++
 sound/soc/codecs/hdmi.c                            | 174 +++++++++++-
 5 files changed, 488 insertions(+), 24 deletions(-)
 create mode 100644 include/sound/hdmi.h

-- 
2.1.1

[PATCH v6 1/2] ASoC:codecs: Add a transmitter interface to the HDMI CODEC CODEC

[Jean-Francois Moine]

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);
+
+	/* 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);
-- 
2.1.1

Re: [PATCH v6 1/2] ASoC:codecs: Add a transmitter interface to the HDMI CODEC CODEC

[Andrew Jackson]

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);
> 

Re: [PATCH v6 1/2] ASoC:codecs: Add a transmitter interface to the HDMI CODEC CODEC

[Jyri Sarha]

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.

 > +	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.

 > +#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.

 > +	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.

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.

 > +	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.

Best regards,
Jyri

Re: [PATCH v6 1/2] ASoC:codecs: Add a transmitter interface to the HDMI CODEC CODEC

[Jean-Francois Moine]

On Wed, 1 Oct 2014 17:04:12 +0300
Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org> 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-GANU6spQydw@public.gmane.org>
>  > ---
>  >   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.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 2/2] drm/i2c:tda998x: Use the HDMI audio CODEC

[Jean-Francois Moine]

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:
+		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;
 }
 
 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);
 
-- 
2.1.1

Re: [PATCH v6 2/2] drm/i2c:tda998x: Use the HDMI audio CODEC

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]

On Wed, Sep 24, 2014 at 10:11:21AM +0200, Jean-Francois Moine wrote:
> This patch interfaces the HDMI transmitter with the audio system.

Please use subject lines corresponding to the style for the subsystem.

> +	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);

Why is this request_module() needed?  If there is a good reason for it
we should have some documentation.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v6 2/2] drm/i2c:tda998x: Use the HDMI audio CODEC

[Jean-Francois Moine]

On Tue, 30 Sep 2014 20:25:40 +0100
Mark Brown <broonie@kernel.org> wrote:

> > +	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);  
> 
> Why is this request_module() needed?  If there is a good reason for it
> we should have some documentation.

The reason is simple: as the HDMI CODEC is not declared in the DT, the
associated module must be loaded in memory.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH v6 2/2] drm/i2c:tda998x: Use the HDMI audio CODEC

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

On Wed, Oct 01, 2014 at 11:28:46AM +0200, Jean-Francois Moine wrote:
> Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > > +	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);  

> > Why is this request_module() needed?  If there is a good reason for it
> > we should have some documentation.

> The reason is simple: as the HDMI CODEC is not declared in the DT, the
> associated module must be loaded in memory.

No, that doesn't make sense - there's absolutely no dependency on DT for
module loading, non-DT boards work perfectly fine.  Think about this for
a minute, we don't have lots of request_module() calls in board files.
You're working around some other bug here.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v6 2/2] drm/i2c:tda998x: Use the HDMI audio CODEC

[Russell King - ARM Linux]

On Wed, Oct 01, 2014 at 11:28:46AM +0200, Jean-Francois Moine wrote:
> On Tue, 30 Sep 2014 20:25:40 +0100
> Mark Brown <broonie@kernel.org> wrote:
> 
> > > +	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);  
> > 
> > Why is this request_module() needed?  If there is a good reason for it
> > we should have some documentation.
> 
> The reason is simple: as the HDMI CODEC is not declared in the DT, the
> associated module must be loaded in memory.

Module auto-loading works in non-DT environments too.  Unlike the
direction that the arm port is going, the core kernel features, such
as driver autoloading, are coded *not* to require DT (or indeed any
particular firmware.)

There are circumstances where this has been lost sight of (such as
the gpiod stuff), but as a general rule, features do not rely on DT.

Platform drivers will be auto-loaded if they have:

MODULE_ALIAS(PLATFORM_MODULE_PREFIX DRIVER_NAME);

and DRIVER_NAME matches the non-id part of the platform device name.
In the case of platform driver ID tables:

MODULE_DEVICE_TABLE(platform, id-table);

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v6 2/2] drm/i2c:tda998x: Use the HDMI audio CODEC

[Jyri Sarha]

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

Re: [PATCH v6 2/2] drm/i2c:tda998x: Use the HDMI audio CODEC

[Jean-Francois Moine]

On Wed, 1 Oct 2014 17:05:32 +0300
Jyri Sarha <jsarha@ti.com> wrote:

> >   	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;
>  > +		}

I looked again at the original driver and they set K = 1 for 16 bits
and K = 3 for 24 or 32 bits.

Anyway, letting K = 3 for 16 bits works for me, so, I will not change
this code in the next version.

Thanks.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 2/2] drm/i2c:tda998x: Use the HDMI audio CODEC

[Russell King - ARM Linux]

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.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v6 2/2] drm/i2c:tda998x: Use the HDMI audio CODEC

[Jean-Francois Moine]

On Wed, 1 Oct 2014 15:23:41 +0100
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> 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.

The second option seems easier for use as a slave encoder.

Thanks for all your remarks. I will send an other version.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 0/2] ASoC: tda998x: add a codec to the HDMI transmitter

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 533 bytes --]

On Wed, Sep 24, 2014 at 10:23:34AM +0200, Jean-Francois Moine wrote:
> The NXP TDA998x HDMI transmitter may transmit audio to the HDMI link
> from 2 different sources, I2S and S/PDIF.
> 
> This patch set adds an interface between the HDMI transmitter and
> the HDMI CODEC.

I'd really like to see some of the other people who are working with
HDMI reviewing this - this is a generic part which seems to be very
widely used so we need to make sure that what's being done works for
other systems.  Jyri, I see you're on CC?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]