Implement generic ASoC HDMI codec and use it in tda998x

The hdmi stub codec has not been used since refactoring of OMAP HDMI audio support.

Quoted 2 lines · Jyri Sarha

The hdmi stub codec has not been used since refactoring of OMAP HDMI audio support.

grep tells me that the OMAP HDMI4 and HDMI5 drivers are still registering this device in -next...

On 08/14/15 19:10, Mark Brown wrote:

Don't you mean "omap-hdmi-audio", that is implemented in sound/soc/omap/omap-hdmi-audio.c ?

That driver is bit different. It implements ASoC card and uses generic dummy codec. The "hdmi-audio-codec" has not been used for OMAP HDMI audio for several releases already.

drivers/gpu/drm/i2c/tda998x_drv.c +8 -0

+static int tda998x_write_aif(struct tda998x_priv *priv,
+			     struct hdmi_audio_infoframe *cea)
+{
+	uint8_t buf[HDMI_INFOFRAME_SIZE(AUDIO)];
+	int len;
+
+	len = hdmi_audio_infoframe_pack(cea, buf, sizeof(buf));
+	if (len < 0) {

static void
tda998x_write_avi(struct tda998x_priv *priv, struct drm_display_mode *mode) {

drivers/gpu/drm/i2c/tda998x_drv.c +0 -0

@@ -670,19 +691,24 @@ static void tda998x_audio_mute(struct tda998x_priv *priv, bool on)
 	}
 }
 

drivers/gpu/drm/i2c/tda998x_drv.c +6 -0

+static int tda998x_audio_get_eld(struct device *dev, uint8_t *buf, size_t len)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	struct drm_mode_config *config = &priv->encoder->dev->mode_config;
+	struct drm_connector *connector;
+	int ret = -ENODEV;

Quoted 15 lines · Jyri Sarha

drivers/gpu/drm/i2c/tda998x_drv.c +15 -0

+static int tda998x_write_aif(struct tda998x_priv *priv,
+			     struct hdmi_audio_infoframe *cea)
+{
+	uint8_t buf[HDMI_INFOFRAME_SIZE(AUDIO)];
+	int len;
+
+	len = hdmi_audio_infoframe_pack(cea, buf, sizeof(buf));
+	if (len < 0) {
+		dev_err(&priv->hdmi->dev,
+			"Failed to pack audio infoframe: %d\n", len);
+		return len;
+	}
+
+	/* Write the audio information packet */
+	tda998x_write_if(priv, DIP_IF_FLAGS_IF4, REG_IF4_HB0, buf, len);

drivers/gpu/drm/i2c/tda998x_drv.c tda998x_write_aif(struct tda998x_priv *priv, struct tda998x_encoder_params *p) · line 661

Quoted 3 lines

drivers/gpu/drm/i2c/tda998x_drv.c +3 -0

+	return 0;
+}
+

I have such a function already queued up, but I can't push it out at the moment because of too many conflicts across all my DRM work. I'm waiting for after 4.3-rc1 before publishing anything from or accepting anything else into DRM branches.

Quoted 7 lines

  static void
  tda998x_write_avi(struct tda998x_priv *priv, struct drm_display_mode *mode)
  {

(quoted diff) +0 -0

@@ -670,19 +691,24 @@ static void tda998x_audio_mute(struct tda998x_priv *priv, bool on)
  	}
  }
 

drivers/gpu/drm/i2c/tda998x_drv.c static void tda998x_audio_mute(struct tda998x_priv *priv, bool on) · line 701

Quoted 10 lines

drivers/gpu/drm/i2c/tda998x_drv.c +7 -2

-static void
+static int
  tda998x_configure_audio(struct tda998x_priv *priv,
-		struct drm_display_mode *mode, struct tda998x_encoder_params *p)
+			int mode_clock,
+			int ena_ap,
+			int dai_format,
+			int sample_width,
+			int sample_rate,
+			const u8 *status)

I don't think this is an improvement.

Quoted 10 lines

drivers/gpu/drm/i2c/tda998x_drv.c +10 -0

+static int tda998x_audio_get_eld(struct device *dev, uint8_t *buf, size_t len)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	struct drm_mode_config *config = &priv->encoder->dev->mode_config;
+	struct drm_connector *connector;
+	int ret = -ENODEV;
+
+	mutex_lock(&config.mutex);
+	list_for_each_entry(connector, &config->connector_list, head) {
+		if (priv->encoder == connector->encoder) {

drivers/gpu/drm/i2c/tda998x_drv.c static struct drm_encoder_slave_funcs tda998x_encoder_slave_funcs = { · line 1380

Quoted 6 lines

drivers/gpu/drm/i2c/tda998x_drv.c +6 -0

+			memcpy(buf, connector->eld,
+			       min(sizeof(connector->eld), len));
+			ret = 0;
+		}
+	}
+	mutex_unlock(&config.mutex);

Obviously untested. Should be config->mutex.

But in any case, when I kill the DRM slave encoder code in here, this becomes unnecessary.

sound/soc/codecs/hdmi-codec.c new file +7 -0

+		.iec = {
+			.status = {
+				IEC958_AES0_CON_NOT_COPYRIGHT,
+				IEC958_AES1_CON_GENERAL,
+				IEC958_AES2_CON_SOURCE_UNSPEC,
+				IEC958_AES3_CON_CLOCK_VARIABLE,
+			},

sound/soc/codecs/hdmi-codec.c new file +9 -0

+	hp.cea.channels = params_channels(params);
+
+	switch (params_width(params)) {
+	case 16:
+		hp.iec.status[4] |= IEC958_AES4_CON_WORDLEN_20_16;
+		hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_16;
+		break;
+	case 18:
+		hp.iec.status[4] |= IEC958_AES4_CON_WORDLEN_22_18;

sound/soc/codecs/hdmi-codec.c line 142

Quoted 7 lines · Jyri Sarha

sound/soc/codecs/hdmi-codec.c new file +7 -0

+static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
+				struct snd_pcm_hw_params *params,
+				struct snd_soc_dai *dai)
+{
+	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
+	struct hdmi_codec_params hp = {
+		.cea = { 0 },

Unnecessary initialisation - because you are initialising this structure, all unnamed fields will be zeroed.

Quoted 7 lines

sound/soc/codecs/hdmi-codec.c new file +7 -0

+		.iec = {
+			.status = {
+				IEC958_AES0_CON_NOT_COPYRIGHT,
+				IEC958_AES1_CON_GENERAL,
+				IEC958_AES2_CON_SOURCE_UNSPEC,
+				IEC958_AES3_CON_CLOCK_VARIABLE,
+			},

...

sound/soc/codecs/hdmi-codec.c line 164

Quoted 2 lines

sound/soc/codecs/hdmi-codec.c new file +2 -0

+	hdmi_audio_infoframe_init(&hp.cea);
+	hp.cea.coding_type = HDMI_AUDIO_CODING_TYPE_PCM;

Something tells me here that you haven't read the HDMI specification. HDMI says that the coding type will be zero (refer to stream header). The same goes for much of the CEA audio infoframe. Please see the Audio InfoFrame details in the HDMI specification.

Quoted 14 lines

sound/soc/codecs/hdmi-codec.c new file +14 -0

+	hp.cea.channels = params_channels(params);
+
+	switch (params_width(params)) {
+	case 16:
+		hp.iec.status[4] |= IEC958_AES4_CON_WORDLEN_20_16;
+		hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_16;
+		break;
+	case 18:
+		hp.iec.status[4] |= IEC958_AES4_CON_WORDLEN_22_18;
+		hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_20;
+		break;
+	case 20:
+		hp.iec.status[4] |= IEC958_AES4_CON_WORDLEN_24_20;
+		hp.cea.sample_size = HDMI_AUDIO_SAMPLE_SIZE_20;

sound/soc/codecs/hdmi-codec.c line 183

Quoted 5 lines

sound/soc/codecs/hdmi-codec.c new file +5 -0

+		break;
+	case 24:
+	case 32:
+		hp.iec.status[4] |= IEC958_AES4_CON_MAX_WORDLEN_24 |
+			IEC958_AES4_CON_WORDLEN_24_20;

Why not use the generic code to generate the AES channel status bits? See sound/core/pcm_iec958.c.

include/sound/hdmi-codec.h line 60

Quoted 5 lines · Jyri Sarha

include/sound/hdmi-codec.h new file +5 -0

+struct hdmi_codec_ops {
+	/* For runtime clock configuration from ASoC machine driver.
+	 * A direct forward from set_sysclk in struct snd_soc_dai_ops.
+	 * Optional */
+	int (*set_clk)(struct device *dev, int clk_id, int freq);

I'd be much happier if we were using the clock API as the external interface here, it's where we want to be internally too and it's going to be easier to not introduce any external dependencies on the ASoC internal stuff.

include/sound/hdmi-codec.h line 67

Quoted 6 lines

include/sound/hdmi-codec.h new file +6 -0

+	/* Called when ASoC starts an audio stream setup. The call
+	 * provides an audio abort callback for stoping an ongoing
+	 * stream if the HDMI audio becomes unavailable.
+	 * Optional */
+	int (*audio_startup)(struct device *dev,
+			     void (*abort_cb)(struct device *dev));

I'm a bit confused about what is going to use abort_cb() and why they wouldn't just call shutdown instead?

include/sound/hdmi-codec.h line 90

Quoted 3 lines

include/sound/hdmi-codec.h new file +3 -0

+/* HDMI codec initalization data */
+struct hdmi_codec_pdata {
+	struct device *dev; /* The HDMI encoder registering the codec */

Shouldn't this just be dev->parent?

sound/soc/codecs/hdmi-codec.c line 47

Quoted 4 lines

sound/soc/codecs/hdmi-codec.c new file +4 -0

+enum {
+	DAI_ID_I2C = 0,
+	DAI_ID_SPDIF,
+};

I2C? :P

Quoted 11 lines · Mark Brown

include/sound/hdmi-codec.h line 67

Quoted 6 lines · Jyri Sarha

include/sound/hdmi-codec.h new file +6 -0

+	/* Called when ASoC starts an audio stream setup. The call
+	 * provides an audio abort callback for stoping an ongoing
+	 * stream if the HDMI audio becomes unavailable.
+	 * Optional */
+	int (*audio_startup)(struct device *dev,
+			     void (*abort_cb)(struct device *dev));

I'm a bit confused about what is going to use abort_cb() and why they wouldn't just call shutdown instead?

audio_shutdown() is for ASoC side to tell video side that audio playback has stopped.

The abort_cb() is for video side to inform ASoC that current audio stream can not continue anymore and it should be aborted. The similar mechanism is currently in use in sound/soc/omap/omap-hdmi-audio.c.

Quoted 12 lines · Jyri Sarha

Quoted 11 lines · Mark Brown

include/sound/hdmi-codec.h line 67

Quoted 6 lines · Jyri Sarha

include/sound/hdmi-codec.h new file +6 -0

+	/* Called when ASoC starts an audio stream setup. The call
+	 * provides an audio abort callback for stoping an ongoing
+	 * stream if the HDMI audio becomes unavailable.
+	 * Optional */
+	int (*audio_startup)(struct device *dev,
+			     void (*abort_cb)(struct device *dev));

I'm a bit confused about what is going to use abort_cb() and why they wouldn't just call shutdown instead?

Quoted 6 lines

audio_shutdown() is for ASoC side to tell video side that audio playback has stopped.

The abort_cb() is for video side to inform ASoC that current audio stream can not continue anymore and it should be aborted. The similar mechanism is currently in use in sound/soc/omap/omap-hdmi-audio.c.

Someone reading the code needs to be able to understand this.

On Fri, Aug 14, 2015 at 12:30:41PM +0300, Jyri Sarha wrote:

include/sound/hdmi-codec.h line 90

Quoted 3 lines

include/sound/hdmi-codec.h new file +3 -0

+/* HDMI codec initalization data */
+struct hdmi_codec_pdata {
+	struct device *dev; /* The HDMI encoder registering the codec */

Shouldn't this just be dev->parent?

Missed one commet first time around...

Quoted 1 line · Mark Brown

On Fri, Aug 14, 2015 at 12:30:41PM +0300, Jyri Sarha wrote:

...

Quoted 6 lines

include/sound/hdmi-codec.h line 90

Quoted 3 lines

include/sound/hdmi-codec.h new file +3 -0

+/* HDMI codec initalization data */
+struct hdmi_codec_pdata {
+    struct device *dev; /* The HDMI encoder registering the codec */

Shouldn't this just be dev->parent?

No. The HDMI encoder device is the parent for HDMI-codec.

The patch you took in in the last round uses the ASoC component drivers parent's of-node if the component driver does not have one itself. In this case the phandle in the binding points to the HDMI encoder's node, which is the parent of the HDMI codec.