drm/i2c: tda998x ASoC hdmi-codec support + BeagleBoneBlack audio support

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

Quoted 11 lines · Jyri Sarha

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

@@ -787,19 +792,13 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
 
 	/* Write the channel status */
-	buf[0] = IEC958_AES0_CON_NOT_COPYRIGHT;
-	buf[1] = 0x00;
-	buf[2] = IEC958_AES3_CON_FS_NOTID;
-	buf[3] = IEC958_AES4_CON_ORIGFS_NOTID |
-			IEC958_AES4_CON_MAX_WORDLEN_24;
-	reg_write_range(priv, REG_CH_STAT_B(0), buf, 4);
+	reg_write_range(priv, REG_CH_STAT_B(0), params->status, 4);

Take a close look at the code you are replacing here - the buffer contents is not AES bytes 0, 1, 2, 3, 4, but AES bytes 0, 1, 3, 4
- byte 2 is not present in this array. I don't think you've noticed this as your second patch merely copies verboten the IEC status:

(quoted diff) +2 -0

+       memcpy(audio.status, params->iec.status,
+              min(sizeof(audio.status), sizeof(params->iec.status)));

assuming that it is bytes 0-3. Byte 2 is stored separately for each I2S channel in the following registers.

Quoted 8 lines

include/drm/i2c/tda998x.h +0 -0

index 3e419d9..24be7aa 100644
@@ -1,6 +1,19 @@
 #ifndef __DRM_I2C_TDA998X_H__
 #define __DRM_I2C_TDA998X_H__
 

include/drm/i2c/tda998x.h line 6

Quoted 3 lines

include/drm/i2c/tda998x.h +3 -0

+#define AFMT_UNUSED	0
+#define AFMT_SPDIF	1
+#define AFMT_I2S	2

I'd prefer this to stay an enum please.

include/drm/i2c/tda998x.h line 13

Quoted 6 lines

include/drm/i2c/tda998x.h +6 -0

+struct tda998x_audio_params {
+	u8 config;
+	u8 format;
+	unsigned sample_width;
+	unsigned sample_rate;
+	struct hdmi_audio_infoframe cea;

With this addition, this file will need to include linux/hdmi.h.

include/drm/i2c/tda998x.h line 14

Quoted 1 line

include/drm/i2c/tda998x.h +1 -0

+	u8 status[4];

A comment here about the missing byte 2 would probably be a good idea.

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

@@ -41,7 +41,7 @@ struct tda998x_priv {
 	u8 vip_cntrl_0;
 	u8 vip_cntrl_1;
 	u8 vip_cntrl_2;
-	struct tda998x_encoder_params params;
+	struct tda998x_audio_params audio_params;

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

@@ -820,7 +819,7 @@ static void tda998x_encoder_set_config(struct tda998x_priv *priv,
 			    VIP_CNTRL_2_SWAP_F(p->swap_f) |
 			    (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
 
-	priv->params = *p;
+	priv->audio_params = p->audio;

include/drm/i2c/tda998x.h +0 -1

@@ -15,16 +28,7 @@ struct tda998x_encoder_params {
 	u8 swap_e:3;
 	u8 mirr_e:1;
 
-	u8 audio_cfg;

drivers/gpu/drm/i2c/tda998x_drv.c static void tda998x_destroy(struct tda998x_priv *priv) · line 1224

Quoted 2 lines · Jyri Sarha

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

+	memcpy(audio.status, params->iec.status,
+	       min(sizeof(audio.status), sizeof(params->iec.status)));

As mentioned in the other patch, the audio status does not directly correspond with the AES bytes, so this ends up causing the driver to write the wrong bytes to the wrong registers.

drivers/gpu/drm/i2c/tda998x_drv.c static void tda998x_destroy(struct tda998x_priv *priv) · line 1259

Quoted 3 lines

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

+	ret = tda998x_configure_audio(priv,
+				      &audio,
+				      priv->encoder.crtc->hwmode.clock);

What happens if audio changes at the same time that the video mode changes? What protection is there against two threads concurrently executing tda998x_configure_audio() ?

drivers/gpu/drm/i2c/tda998x_drv.c static void tda998x_destroy(struct tda998x_priv *priv) · line 1333

Quoted 3 lines

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

+	priv->audio_pdev = platform_device_register_data(
+		dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO,
+		&codec_data, sizeof(codec_data));

I'd much prefer that this was a child of the I2C device, which will show the relationship between this virtual platform device and the device which it's associated with. That can be done via platform_device_register_full().

Quoted 10 lines · Russell King - ARM Linux

drivers/gpu/drm/i2c/tda998x_drv.c static void tda998x_destroy(struct tda998x_priv *priv) · line 1333

Quoted 3 lines · Jyri Sarha

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

+	priv->audio_pdev = platform_device_register_data(
+		dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO,
+		&codec_data, sizeof(codec_data));

I'd much prefer that this was a child of the I2C device, which will show the relationship between this virtual platform device and the device which it's associated with. That can be done via platform_device_register_full().

That may be a problem. The ASoC card device tree binding current looks for the ASoC DAI's parent's of-node if it can not find the node for the DAI-device itself.

Quoted 3 lines · Jyri Sarha

That may be a problem. The ASoC card device tree binding current looks for the ASoC DAI's parent's of-node if it can not find the node for the DAI-device itself.

I don't follow. The "parent" of this audio_pdev device above is the "platform" device - /sys/devices/platform. If ASoC is referencing the parent of the above platform device for some reason, it's probably not going to get anything useful from such an attempt.

Any other ASoC codec driver where the codec platform device was declared with a NULL parent pointer also ends up as a child of that same location.

I'd _really_ be surprised if ASoC is even doing what you describe, because such an action is totally illogical (as can be seen from my description above.)

Quoted 12 lines · Russell King - ARM Linux

Quoted 3 lines · Jyri Sarha

That may be a problem. The ASoC card device tree binding current looks for the ASoC DAI's parent's of-node if it can not find the node for the DAI-device itself.

I don't follow. The "parent" of this audio_pdev device above is the "platform" device - /sys/devices/platform. If ASoC is referencing the parent of the above platform device for some reason, it's probably not going to get anything useful from such an attempt.

Quoted 7 lines

Any other ASoC codec driver where the codec platform device was declared with a NULL parent pointer also ends up as a child of that same location.

I'd _really_ be surprised if ASoC is even doing what you describe, because such an action is totally illogical (as can be seen from my description above.)

We do have some stuff in there in order to handle MFDs - they'll instantiate a Linux-internal virtual platform device as a child of the DT device that represents the device as a whole. We're definitely not expecting to find anything for parentless platform devices though, it's only done if the ASoC level device has no of_node and the parent does.

Quoted 5 lines · Mark Brown

We do have some stuff in there in order to handle MFDs - they'll instantiate a Linux-internal virtual platform device as a child of the DT device that represents the device as a whole. We're definitely not expecting to find anything for parentless platform devices though, it's only done if the ASoC level device has no of_node and the parent does.

And if we make the hdmi-codec device a child of the I2C device which does have an of-node, what will happen?

Quoted 7 lines · Russell King - ARM Linux

Quoted 5 lines · Mark Brown

We do have some stuff in there in order to handle MFDs - they'll instantiate a Linux-internal virtual platform device as a child of the DT device that represents the device as a whole. We're definitely not expecting to find anything for parentless platform devices though, it's only done if the ASoC level device has no of_node and the parent does.

Quoted 2 lines

And if we make the hdmi-codec device a child of the I2C device which does have an of-node, what will happen?

It'll pick up that as the DT device to hang things off which I'd expect to be the desired outcome given that this is a very similar situation to the MFD situation. I've not been following the full thread so there is probably context I'm missing here...

Quoted 4 lines

Quoted 4 lines

It'll pick up that as the DT device to hang things off which I'd expect to be the desired outcome given that this is a very similar situation to the MFD situation. I've not been following the full thread so there is probably context I'm missing here...

Okay, and that sounds to me like a very reasonable thing to want to happen - so that the audio side can be clearly identified as being coupled with the video side.

So, I'm not seeing a problem with my suggestion... I'm actually more reasons why it's a good thing to want.

Quoted 3 lines

(quoted diff) +1 -0

+	priv->audio_pdev = platform_device_register_data(

Review

Quoted 2 lines

(quoted diff) +2 -0

+		dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO,
+		&codec_data, sizeof(codec_data));

I'd much prefer that this was a child of the I2C device, which will show the relationship between this virtual platform device and the device which it's associated with. That can be done via platform_device_register_full().