diff mbox

[1/3] drm/i2c: tda998x: Improve tda998x_configure_audio() audio related pdata

Message ID 5adcd94104080abcd5b072a5d6a85b85c5ea0584.1470129989.git.jsarha@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jyri Sarha Aug. 2, 2016, 12:05 p.m. UTC
Define struct tda998x_audio_params in include/drm/i2c/tda998x.h and
use it in pdata and for tda998x_configure_audio() parameters. Also
updates tda998x_write_aif() to take struct hdmi_audio_infoframe *
directly as a parameter.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 77 ++++++++++++++++++++-------------------
 include/drm/i2c/tda998x.h         | 24 +++++++-----
 2 files changed, 53 insertions(+), 48 deletions(-)

Comments

Russell King (Oracle) Aug. 4, 2016, 1:31 p.m. UTC | #1
On Tue, Aug 02, 2016 at 03:05:07PM +0300, Jyri Sarha wrote:
> @@ -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:

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

> diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
> index 3e419d9..24be7aa 100644
> --- a/include/drm/i2c/tda998x.h
> +++ b/include/drm/i2c/tda998x.h
> @@ -1,6 +1,19 @@
>  #ifndef __DRM_I2C_TDA998X_H__
>  #define __DRM_I2C_TDA998X_H__
>  
> +#define AFMT_UNUSED	0
> +#define AFMT_SPDIF	1
> +#define AFMT_I2S	2

I'd prefer this to stay an enum please.

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

> +	u8 status[4];

A comment here about the missing byte 2 would probably be a good idea.
Jyri Sarha Aug. 5, 2016, 9:02 a.m. UTC | #2
On 08/04/16 16:31, Russell King - ARM Linux wrote:
> On Tue, Aug 02, 2016 at 03:05:07PM +0300, Jyri Sarha wrote:
>> @@ -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:
> 
> +       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.
> 

Oh, yes. I missed that. I think it would be better to increase
tda998x_audio_params status buffer length by one and keep it otherwise
as it is. Then just write the bytes 0-1 and 3-4 separately into channel
status registers, with appropriate comments. I guess the the AES byte 2
can remain 0 for now (source and channel unspecified, with assumption of
consumer stream).

>> diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
>> index 3e419d9..24be7aa 100644
>> --- a/include/drm/i2c/tda998x.h
>> +++ b/include/drm/i2c/tda998x.h
>> @@ -1,6 +1,19 @@
>>  #ifndef __DRM_I2C_TDA998X_H__
>>  #define __DRM_I2C_TDA998X_H__
>>  
>> +#define AFMT_UNUSED	0
>> +#define AFMT_SPDIF	1
>> +#define AFMT_I2S	2
> 
> I'd prefer this to stay an enum please.
> 

Ok.

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

Oh yes, thanks.

>> +	u8 status[4];
> 
> A comment here about the missing byte 2 would probably be a good idea.
> 

As I said earlier, I'd rather keep this as plain channel status buffer
and just increase its size by one.

Thanks a lot for the review!

Best reagards,
Jyri
Russell King (Oracle) Aug. 6, 2016, 4:57 p.m. UTC | #3
On Tue, Aug 02, 2016 at 03:05:07PM +0300, Jyri Sarha wrote:
> @@ -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);

Another couple of points here:

1. I think we should only write the channel status if I2S mode is
selected - channel status won't be used on SPDIF mode as SPDIF
supplies its own channel status.

2. I think we should continue writing all the channel status in a
single I2C transaction, and not breaking it into several different
transactions to cater for the odd register order.  I don't have
any direct evidence that one way is better than the other in terms
of atomicity of the update, but I think having it as a single
transaction stands a better chance of the chip atomically updating
the channel status.
Russell King (Oracle) Aug. 6, 2016, 5:56 p.m. UTC | #4
On Tue, Aug 02, 2016 at 03:05:07PM +0300, Jyri Sarha wrote:
> @@ -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;
...
> @@ -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;
...
> @@ -15,16 +28,7 @@ struct tda998x_encoder_params {
>  	u8 swap_e:3;
>  	u8 mirr_e:1;
>  
> -	u8 audio_cfg;
> -	u8 audio_clk_cfg;
> -	u8 audio_frame[6];
> -
> -	enum {
> -		AFMT_SPDIF,
> -		AFMT_I2S
> -	} audio_format;
> -
> -	unsigned audio_sample_rate;
> +	struct tda998x_audio_params audio;

Another point - it would be nice if the name for both tda998x_audio_params
structures was the same.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index f4315bc..f97b748 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -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;
 
 	wait_queue_head_t wq_edid;
 	volatile int wq_edid_wait;
@@ -666,26 +666,16 @@  tda998x_write_if(struct tda998x_priv *priv, u8 bit, u16 addr,
 	reg_set(priv, REG_DIP_IF_FLAGS, bit);
 }
 
-static void
-tda998x_write_aif(struct tda998x_priv *priv, struct tda998x_encoder_params *p)
+static int tda998x_write_aif(struct tda998x_priv *priv,
+			     struct hdmi_audio_infoframe *cea)
 {
 	union hdmi_infoframe frame;
 
-	hdmi_audio_infoframe_init(&frame.audio);
-
-	frame.audio.channels = p->audio_frame[1] & 0x07;
-	frame.audio.channel_allocation = p->audio_frame[4];
-	frame.audio.level_shift_value = (p->audio_frame[5] & 0x78) >> 3;
-	frame.audio.downmix_inhibit = (p->audio_frame[5] & 0x80) >> 7;
-
-	/*
-	 * L-PCM and IEC61937 compressed audio shall always set sample
-	 * frequency to "refer to stream".  For others, see the HDMI
-	 * specification.
-	 */
-	frame.audio.sample_frequency = (p->audio_frame[2] & 0x1c) >> 2;
+	frame.audio = *cea;
 
 	tda998x_write_if(priv, DIP_IF_FLAGS_IF4, REG_IF4_HB0, &frame);
+
+	return 0;
 }
 
 static void
@@ -710,20 +700,21 @@  static void tda998x_audio_mute(struct tda998x_priv *priv, bool on)
 	}
 }
 
-static void
+static int
 tda998x_configure_audio(struct tda998x_priv *priv,
-		struct drm_display_mode *mode, struct tda998x_encoder_params *p)
+			struct tda998x_audio_params *params,
+			unsigned mode_clock)
 {
 	u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv;
 	u32 n;
 
 	/* Enable audio ports */
-	reg_write(priv, REG_ENA_AP, p->audio_cfg);
-	reg_write(priv, REG_ENA_ACLK, p->audio_clk_cfg);
+	reg_write(priv, REG_ENA_AP, params->config);
 
 	/* Set audio input source */
-	switch (p->audio_format) {
+	switch (params->format) {
 	case AFMT_SPDIF:
+		reg_write(priv, REG_ENA_ACLK, 0);
 		reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_SPDIF);
 		clksel_aip = AIP_CLKSEL_AIP_SPDIF;
 		clksel_fs = AIP_CLKSEL_FS_FS64SPDIF;
@@ -731,15 +722,29 @@  tda998x_configure_audio(struct tda998x_priv *priv,
 		break;
 
 	case AFMT_I2S:
+		reg_write(priv, REG_ENA_ACLK, 1);
 		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);
+		switch (params->sample_width) {
+		case 16:
+			cts_n = CTS_N_M(3) | CTS_N_K(1);
+			break;
+		case 18:
+		case 20:
+		case 24:
+			cts_n = CTS_N_M(3) | CTS_N_K(2);
+			break;
+		default:
+		case 32:
+			cts_n = CTS_N_M(3) | CTS_N_K(3);
+			break;
+		}
 		break;
 
 	default:
 		BUG();
-		return;
+		return -EINVAL;
 	}
 
 	reg_write(priv, REG_AIP_CLKSEL, clksel_aip);
@@ -755,11 +760,11 @@  tda998x_configure_audio(struct tda998x_priv *priv,
 	 * assume 100MHz requires larger divider.
 	 */
 	adiv = AUDIO_DIV_SERCLK_8;
-	if (mode->clock > 100000)
+	if (mode_clock > 100000)
 		adiv++;			/* AUDIO_DIV_SERCLK_16 */
 
 	/* S/PDIF asks for a larger divider */
-	if (p->audio_format == AFMT_SPDIF)
+	if (params->format == AFMT_SPDIF)
 		adiv++;			/* AUDIO_DIV_SERCLK_16 or _32 */
 
 	reg_write(priv, REG_AUDIO_DIV, adiv);
@@ -768,7 +773,7 @@  tda998x_configure_audio(struct tda998x_priv *priv,
 	 * This is the approximate value of N, which happens to be
 	 * the recommended values for non-coherent clocks.
 	 */
-	n = 128 * p->audio_sample_rate / 1000;
+	n = 128 * params->sample_rate / 1000;
 
 	/* Write the CTS and N values */
 	buf[0] = 0x44;
@@ -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);
 
 	tda998x_audio_mute(priv, true);
 	msleep(20);
 	tda998x_audio_mute(priv, false);
 
-	/* Write the audio information packet */
-	tda998x_write_aif(priv, p);
+	return tda998x_write_aif(priv, &params->cea);
 }
 
 /* DRM encoder functions */
@@ -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;
 }
 
 static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
@@ -1057,9 +1056,11 @@  tda998x_encoder_mode_set(struct drm_encoder *encoder,
 
 		tda998x_write_avi(priv, adjusted_mode);
 
-		if (priv->params.audio_cfg)
-			tda998x_configure_audio(priv, adjusted_mode,
-						&priv->params);
+		if (priv->audio_params.format != AFMT_UNUSED) {
+			tda998x_configure_audio(priv,
+						&priv->audio_params,
+						adjusted_mode->clock);
+		}
 	}
 }
 
diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
index 3e419d9..24be7aa 100644
--- a/include/drm/i2c/tda998x.h
+++ b/include/drm/i2c/tda998x.h
@@ -1,6 +1,19 @@ 
 #ifndef __DRM_I2C_TDA998X_H__
 #define __DRM_I2C_TDA998X_H__
 
+#define AFMT_UNUSED	0
+#define AFMT_SPDIF	1
+#define AFMT_I2S	2
+
+struct tda998x_audio_params {
+	u8 config;
+	u8 format;
+	unsigned sample_width;
+	unsigned sample_rate;
+	struct hdmi_audio_infoframe cea;
+	u8 status[4];
+};
+
 struct tda998x_encoder_params {
 	u8 swap_b:3;
 	u8 mirr_b:1;
@@ -15,16 +28,7 @@  struct tda998x_encoder_params {
 	u8 swap_e:3;
 	u8 mirr_e:1;
 
-	u8 audio_cfg;
-	u8 audio_clk_cfg;
-	u8 audio_frame[6];
-
-	enum {
-		AFMT_SPDIF,
-		AFMT_I2S
-	} audio_format;
-
-	unsigned audio_sample_rate;
+	struct tda998x_audio_params audio;
 };
 
 #endif