ASoC: tda998x: add a codec to the HDMI transmitter

drivers/gpu/drm/i2c/tda998x_drv.c tda998x_configure_audio(struct tda998x_priv *priv, · line 774

Quoted 4 lines · Jean-Francois Moine

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

+
+	if (!priv->is_hdmi_sink
+	 || !encoder->crtc)
+		return NULL;

That's weird indentation.

drivers/gpu/drm/i2c/tda998x_drv.c tda998x_configure_audio(struct tda998x_priv *priv, · line 779

Quoted 4 lines

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

+	list_for_each_entry(connector, &drm->mode_config.connector_list, head) {
+		if (connector->encoder == encoder)
+			return connector->eld;
+	}

What guarantees that connector->eld stays valid after it's returned?

include/sound/tda998x.h line 13

Quoted 2 lines

include/sound/tda998x.h +2 -0

+struct tda998x_ops_s {
+	u8 *(*get_eld)(struct device *dev);

Why _ops_s - what does the _s mean? If it's sound perhaps it makes sense to spell it out so people aren't guessing.

include/sound/tda998x.h line 22

Quoted 4 lines

include/sound/tda998x.h +4 -0

+int tda9998x_codec_register(struct device *dev,
+			struct tda998x_audio_s *tda998x_audio_i,
+			struct tda998x_ops_s *tda998x_ops);
+void tda9998x_codec_unregister(struct device *dev);

I'd expect these to be internal to the DRM driver.

sound/soc/codecs/Kconfig config SND_SOC_TAS571X · line 630

Quoted 5 lines

sound/soc/codecs/Kconfig +5 -0

+config SND_SOC_TDA998X
+	def_tristate y
+	select SND_PCM_ELD
+	depends on DRM_I2C_NXP_TDA998X
+

def_tristate y? Why?

sound/soc/codecs/tda998x.c line 20

Quoted 2 lines

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

+/* functions in tda998x_drv */
+static struct tda998x_ops_s *tda998x_ops;

I'd expect this to be stored in the driver data rather than a static global, what if a system has two HDMI outputs?

Quoted 6 lines

sound/soc/codecs/tda998x.c new file +6 -0

+static int tda998x_codec_startup(struct snd_pcm_substream *substream,
+                       struct snd_soc_dai *dai)
+{
+       struct snd_pcm_runtime *runtime = substream->runtime;
+       u8 *eld;
+

sound/soc/codecs/tda998x.c line 32

Quoted 5 lines

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

+       eld = tda998x_ops->get_eld(dai->dev);
+       if (!eld)
+               return -ENODEV;
+       return snd_pcm_hw_constraint_eld(runtime, eld);
+}

Do we really need a device specific mechanism for fishing the ELD out of the graphics code? I'd have expected this to be more generic.

Quoted 5 lines

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

+/* ask the HDMI transmitter to activate the audio input port */
+static int tda998x_codec_hw_params(struct snd_pcm_substream *substream,
+				   struct snd_pcm_hw_params *params,
+				   struct snd_soc_dai *dai)
+{

sound/soc/codecs/tda998x.c line 41

Quoted 3 lines

sound/soc/codecs/tda998x.c new file +3 -0

+	return tda998x_ops->set_audio_input(dai->dev, dai->id,
+					params_rate(params));
+}

The set_audio_input() function doesn't appear to have anything that checks if the device is busy before enabling things, what happens if the user tries to switch between I2S and S/PDIF? It looks like only one DAI can be active at once.

Quoted 5 lines

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

+	for (i = 0, p_dai = dais; i < ndais ; i++, p_dai++) {
+		memcpy(p_dai, &tda998x_dai_i2s, sizeof(*p_dai));
+		p_dai->id = i;
+		if (tda998x_audio->port_types[i] == AFMT_SPDIF) {
+			p_dai->name = "spdif-hifi";

sound/soc/codecs/tda998x.c line 116

Quoted 5 lines

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

+			p_dai->playback.stream_name = "HDMI SPDIF Playback";
+			p_dai->playback.channels_max = 2;
+			p_dai->playback.rate_min = 22050;
+		}
+	}

It would be a bit clearer if the template were just a template and this copying initialised both I2S and S/PDIF specific settings.

sound/soc/codecs/tda998x.c line 122

Quoted 5 lines

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

+	return snd_soc_register_codec(dev,
+				&tda998x_codec_drv,
+				dais, ndais);
+}
+EXPORT_SYMBOL(tda9998x_codec_register);

ASoC is all EXPORT_SYMBOL_GPL, you shouldn't reexport functionality with plain EXPORT_SYMBOL.

Quoted 6 lines · Mark Brown

include/sound/tda998x.h line 22

Quoted 4 lines

include/sound/tda998x.h +4 -0

+int tda9998x_codec_register(struct device *dev,
+			struct tda998x_audio_s *tda998x_audio_i,
+			struct tda998x_ops_s *tda998x_ops);
+void tda9998x_codec_unregister(struct device *dev);

I'd expect these to be internal to the DRM driver.

I don't see where. What is your idea?

Quoted 6 lines

sound/soc/codecs/Kconfig config SND_SOC_TAS571X · line 629

Quoted 4 lines

sound/soc/codecs/Kconfig +4 -0

+config SND_SOC_TDA998X
+	def_tristate y
+	select SND_PCM_ELD
+	depends on DRM_I2C_NXP_TDA998X

def_tristate y? Why?

The TDA998x CODEC is always generated when the DRM TDA998x is generated and when audio is wanted.
Its type, built-in or module, depends on the TDA998x driver type.

Quoted 5 lines

sound/soc/codecs/tda998x.c line 20

Quoted 2 lines

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

+/* functions in tda998x_drv */
+static struct tda998x_ops_s *tda998x_ops;

I'd expect this to be stored in the driver data rather than a static global, what if a system has two HDMI outputs?

Each TDA998x has only one HDMI output and there is only one driver.

Quoted 6 lines

Quoted 6 lines

sound/soc/codecs/tda998x.c new file +6 -0

+static int tda998x_codec_startup(struct snd_pcm_substream *substream,
+                       struct snd_soc_dai *dai)
+{
+       struct snd_pcm_runtime *runtime = substream->runtime;
+       u8 *eld;
+

Quoted 8 lines

sound/soc/codecs/tda998x.c line 32

Quoted 5 lines

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

+       eld = tda998x_ops->get_eld(dai->dev);
+       if (!eld)
+               return -ENODEV;
+       return snd_pcm_hw_constraint_eld(runtime, eld);
+}

Do we really need a device specific mechanism for fishing the ELD out of the graphics code? I'd have expected this to be more generic.

I will put the ELD in the private data of the DRM driver.

Quoted 8 lines · Jean-Francois Moine

Quoted 6 lines · Mark Brown

include/sound/tda998x.h line 22

Quoted 4 lines

include/sound/tda998x.h +4 -0

+int tda9998x_codec_register(struct device *dev,
+			struct tda998x_audio_s *tda998x_audio_i,
+			struct tda998x_ops_s *tda998x_ops);
+void tda9998x_codec_unregister(struct device *dev);

I'd expect these to be internal to the DRM driver.

Quoted 1 line

I don't see where. What is your idea?

What I said above - put them in the DRM driver. They're just registering a device IIRC.

Quoted 6 lines

Quoted 6 lines

sound/soc/codecs/Kconfig config SND_SOC_TAS571X · line 629

Quoted 4 lines

sound/soc/codecs/Kconfig +4 -0

+config SND_SOC_TDA998X
+	def_tristate y
+	select SND_PCM_ELD
+	depends on DRM_I2C_NXP_TDA998X

def_tristate y? Why?

Quoted 3 lines

The TDA998x CODEC is always generated when the DRM TDA998x is generated and when audio is wanted.
Its type, built-in or module, depends on the TDA998x driver type.

Just make this like any other driver with no default specified.

Quoted 7 lines

Quoted 5 lines

sound/soc/codecs/tda998x.c line 20

Quoted 2 lines

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

+/* functions in tda998x_drv */
+static struct tda998x_ops_s *tda998x_ops;

I'd expect this to be stored in the driver data rather than a static global, what if a system has two HDMI outputs?

Each TDA998x has only one HDMI output and there is only one driver.

Someone might put two chips on the board.

Quoted 14 lines

Quoted 14 lines

sound/soc/codecs/tda998x.c line 32

Quoted 11 lines

sound/soc/codecs/tda998x.c new file +11 -0

+static int tda998x_codec_startup(struct snd_pcm_substream *substream,
+                       struct snd_soc_dai *dai)
+{
+       struct snd_pcm_runtime *runtime = substream->runtime;
+       u8 *eld;
+
+       eld = tda998x_ops->get_eld(dai->dev);
+       if (!eld)
+               return -ENODEV;
+       return snd_pcm_hw_constraint_eld(runtime, eld);
+}

Do we really need a device specific mechanism for fishing the ELD out of the graphics code? I'd have expected this to be more generic.

Quoted 1 line

I will put the ELD in the private data of the DRM driver.

That's not the issue here - the issue is that the need to get the ELD out of the video subsystem is not specific to this driver, it's pretty common and something that it seems we should factor out.

The EDID arrives in the DRM connector when video starts. The built ELD may be stored either in the connector itself (default), in the encoder (TDA998x device) or in some DRM/encoder/connector private data.

You may note that, in DRM, there is no relation between the encoder and the connector except at video startup time, and no relation between the DRM connector and the audio CODEC (no CODEC information is returned on CODEC creation and the CODEC has no private data).

Quoted 3 lines · Jean-Francois Moine

The EDID arrives in the DRM connector when video starts. The built ELD may be stored either in the connector itself (default), in the encoder (TDA998x device) or in some DRM/encoder/connector private data.

The ELD is stored in the DRM connector, and there _should_ be a system of locking which ensures that you can get at that information safely.

The ELD is only updated when the connector's get_modes() method is called, and the driver itself calls drm_edid_to_eld(). This all happens under the drm_device's struct_mutex.

So, to safely read the ELD from outside DRM, you need to take the top-level drm_device's struct_mutex. That could be fraught, as that's quite a big lock, so an alternative would be to introduce an 'eld' lock to the driver, which protects the call to drm_edid_to_eld() and the reading of the ELD.

However, that doesn't solve the problem of passing the data to an audio driver. What's needed there is a notification system which allows a video driver to broadcast HDMI events such as:

* connected
* new EDID available
* new ELD available
* disconnected

With such a system, different components driving the facilities of a HDMI connector can make decisions on what to do - which not only includes things like the audio driver, but also a driver for the CEC interface (which also needs to see the EDID to get at its "address".) This can be done in a safe manner as the HDMI video driver would have control over the generation of these messages.

The point that Mark is trying to get you to see is that this problem is not specific to TDA998x. The same problem exists for many other HDMI interfaces (except maybe Intel's i9x5/HDA stuff which has a hardware mechanism of passing the ELD data from the video driver, through the hardware to the audio driver.)

FWIW, we're currently discussing about extending the i915/hda component binding so that the audio driver gets a notification and queries the ELD via callbacks instead of the flaky hardware access.

It'd be best if we have a common infrastructure for that, of course. But currently it's a start, just a bit step forward there.

Quoted 3 lines · Jean-Francois Moine

The EDID arrives in the DRM connector when video starts. The built ELD may be stored either in the connector itself (default), in the encoder (TDA998x device) or in some DRM/encoder/connector private data.

The ELD is stored in the DRM connector, and there _should_ be a system of locking which ensures that you can get at that information safely.

The ELD is only updated when the connector's get_modes() method is called, and the driver itself calls drm_edid_to_eld(). This all happens under the drm_device's struct_mutex.

So, to safely read the ELD from outside DRM, you need to take the top-level drm_device's struct_mutex. That could be fraught, as that's quite a big lock, so an alternative would be to introduce an 'eld' lock to the driver, which protects the call to drm_edid_to_eld() and the reading of the ELD.

However, that doesn't solve the problem of passing the data to an audio driver. What's needed there is a notification system which allows a video driver to broadcast HDMI events such as:

* connected
* new EDID available
* new ELD available
* disconnected

With such a system, different components driving the facilities of a HDMI connector can make decisions on what to do - which not only includes things like the audio driver, but also a driver for the CEC interface (which also needs to see the EDID to get at its "address".) This can be done in a safe manner as the HDMI video driver would have control over the generation of these messages.

The point that Mark is trying to get you to see is that this problem is not specific to TDA998x. The same problem exists for many other HDMI interfaces (except maybe Intel's i9x5/HDA stuff which has a hardware mechanism of passing the ELD data from the video driver, through the hardware to the audio driver.)

It needs solving in a driver-agnostic way, and the normal way that happens is when someone comes along, wanting to add support in that area, they get to do the hard work to propose that infrastructure.

Quoted 4 lines

You may note that, in DRM, there is no relation between the encoder and the connector except at video startup time, and no relation between the DRM connector and the audio CODEC (no CODEC information is returned on CODEC creation and the CODEC has no private data).

In the case of the TDA998x, there is a 1:1 fixed relationship between the connector and the encoder. The connector part can't be used with any other encoder, and the encoder part can't be used with any other connector. The same is true of many other HDMI implementations (such as the Synopsis Designware HDMI interface found on iMX6 and Rockchip.)

Quoted 5 lines · On Tue, 28 Jul 2015 14:53:58 +0100 Russell King - ARM Linux…

In the case of the TDA998x, there is a 1:1 fixed relationship between the connector and the encoder. The connector part can't be used with any other encoder, and the encoder part can't be used with any other connector. The same is true of many other HDMI implementations (such as the Synopsis Designware HDMI interface found on iMX6 and Rockchip.)

But there is no direct link (pointer) between the encoder and the connector.