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@xxxxxxx>
> ---
>   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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/