[PATCH v3 01/24] drm/i2c: tda998x: simplify the i2c read/write functions

[PATCH v3 01/24] drm/i2c: tda998x: simplify the i2c read/write functions

[Jean-Francois Moine]

This patch simplifies the i2c read/write functions and permits them to
be easily called in more contexts.

Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 322 +++++++++++++++++++-------------------
 1 file changed, 162 insertions(+), 160 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 400b0c4..26dd299 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -30,6 +30,7 @@
 
 struct tda998x_priv {
 	struct i2c_client *cec;
+	struct i2c_client *hdmi;
 	uint16_t rev;
 	uint8_t current_page;
 	int dpms;
@@ -328,9 +329,9 @@ struct tda998x_priv {
 #define TDA19988                  0x0301
 
 static void
-cec_write(struct drm_encoder *encoder, uint16_t addr, uint8_t val)
+cec_write(struct tda998x_priv *priv, uint16_t addr, uint8_t val)
 {
-	struct i2c_client *client = to_tda998x_priv(encoder)->cec;
+	struct i2c_client *client = priv->cec;
 	uint8_t buf[] = {addr, val};
 	int ret;
 
@@ -340,9 +341,9 @@ cec_write(struct drm_encoder *encoder, uint16_t addr, uint8_t val)
 }
 
 static uint8_t
-cec_read(struct drm_encoder *encoder, uint8_t addr)
+cec_read(struct tda998x_priv *priv, uint8_t addr)
 {
-	struct i2c_client *client = to_tda998x_priv(encoder)->cec;
+	struct i2c_client *client = priv->cec;
 	uint8_t val;
 	int ret;
 
@@ -362,12 +363,10 @@ fail:
 }
 
 static void
-set_page(struct drm_encoder *encoder, uint16_t reg)
+set_page(struct tda998x_priv *priv, uint16_t reg)
 {
-	struct tda998x_priv *priv = to_tda998x_priv(encoder);
-
 	if (REG2PAGE(reg) != priv->current_page) {
-		struct i2c_client *client = drm_i2c_encoder_get_client(encoder);
+		struct i2c_client *client = priv->hdmi;
 		uint8_t buf[] = {
 				REG_CURPAGE, REG2PAGE(reg)
 		};
@@ -380,13 +379,13 @@ set_page(struct drm_encoder *encoder, uint16_t reg)
 }
 
 static int
-reg_read_range(struct drm_encoder *encoder, uint16_t reg, char *buf, int cnt)
+reg_read_range(struct tda998x_priv *priv, uint16_t reg, char *buf, int cnt)
 {
-	struct i2c_client *client = drm_i2c_encoder_get_client(encoder);
+	struct i2c_client *client = priv->hdmi;
 	uint8_t addr = REG2ADDR(reg);
 	int ret;
 
-	set_page(encoder, reg);
+	set_page(priv, reg);
 
 	ret = i2c_master_send(client, &addr, sizeof(addr));
 	if (ret < 0)
@@ -404,16 +403,16 @@ fail:
 }
 
 static void
-reg_write_range(struct drm_encoder *encoder, uint16_t reg, uint8_t *p, int cnt)
+reg_write_range(struct tda998x_priv *priv, uint16_t reg, uint8_t *p, int cnt)
 {
-	struct i2c_client *client = drm_i2c_encoder_get_client(encoder);
+	struct i2c_client *client = priv->hdmi;
 	uint8_t buf[cnt+1];
 	int ret;
 
 	buf[0] = REG2ADDR(reg);
 	memcpy(&buf[1], p, cnt);
 
-	set_page(encoder, reg);
+	set_page(priv, reg);
 
 	ret = i2c_master_send(client, buf, cnt + 1);
 	if (ret < 0)
@@ -421,21 +420,21 @@ reg_write_range(struct drm_encoder *encoder, uint16_t reg, uint8_t *p, int cnt)
 }
 
 static uint8_t
-reg_read(struct drm_encoder *encoder, uint16_t reg)
+reg_read(struct tda998x_priv *priv, uint16_t reg)
 {
 	uint8_t val = 0;
-	reg_read_range(encoder, reg, &val, sizeof(val));
+	reg_read_range(priv, reg, &val, sizeof(val));
 	return val;
 }
 
 static void
-reg_write(struct drm_encoder *encoder, uint16_t reg, uint8_t val)
+reg_write(struct tda998x_priv *priv, uint16_t reg, uint8_t val)
 {
-	struct i2c_client *client = drm_i2c_encoder_get_client(encoder);
+	struct i2c_client *client = priv->hdmi;
 	uint8_t buf[] = {REG2ADDR(reg), val};
 	int ret;
 
-	set_page(encoder, reg);
+	set_page(priv, reg);
 
 	ret = i2c_master_send(client, buf, ARRAY_SIZE(buf));
 	if (ret < 0)
@@ -443,13 +442,13 @@ reg_write(struct drm_encoder *encoder, uint16_t reg, uint8_t val)
 }
 
 static void
-reg_write16(struct drm_encoder *encoder, uint16_t reg, uint16_t val)
+reg_write16(struct tda998x_priv *priv, uint16_t reg, uint16_t val)
 {
-	struct i2c_client *client = drm_i2c_encoder_get_client(encoder);
+	struct i2c_client *client = priv->hdmi;
 	uint8_t buf[] = {REG2ADDR(reg), val >> 8, val};
 	int ret;
 
-	set_page(encoder, reg);
+	set_page(priv, reg);
 
 	ret = i2c_master_send(client, buf, ARRAY_SIZE(buf));
 	if (ret < 0)
@@ -457,47 +456,47 @@ reg_write16(struct drm_encoder *encoder, uint16_t reg, uint16_t val)
 }
 
 static void
-reg_set(struct drm_encoder *encoder, uint16_t reg, uint8_t val)
+reg_set(struct tda998x_priv *priv, uint16_t reg, uint8_t val)
 {
-	reg_write(encoder, reg, reg_read(encoder, reg) | val);
+	reg_write(priv, reg, reg_read(priv, reg) | val);
 }
 
 static void
-reg_clear(struct drm_encoder *encoder, uint16_t reg, uint8_t val)
+reg_clear(struct tda998x_priv *priv, uint16_t reg, uint8_t val)
 {
-	reg_write(encoder, reg, reg_read(encoder, reg) & ~val);
+	reg_write(priv, reg, reg_read(priv, reg) & ~val);
 }
 
 static void
-tda998x_reset(struct drm_encoder *encoder)
+tda998x_reset(struct tda998x_priv *priv)
 {
 	/* reset audio and i2c master: */
-	reg_set(encoder, REG_SOFTRESET, SOFTRESET_AUDIO | SOFTRESET_I2C_MASTER);
+	reg_set(priv, REG_SOFTRESET, SOFTRESET_AUDIO | SOFTRESET_I2C_MASTER);
 	msleep(50);
-	reg_clear(encoder, REG_SOFTRESET, SOFTRESET_AUDIO | SOFTRESET_I2C_MASTER);
+	reg_clear(priv, REG_SOFTRESET, SOFTRESET_AUDIO | SOFTRESET_I2C_MASTER);
 	msleep(50);
 
 	/* reset transmitter: */
-	reg_set(encoder, REG_MAIN_CNTRL0, MAIN_CNTRL0_SR);
-	reg_clear(encoder, REG_MAIN_CNTRL0, MAIN_CNTRL0_SR);
+	reg_set(priv, REG_MAIN_CNTRL0, MAIN_CNTRL0_SR);
+	reg_clear(priv, REG_MAIN_CNTRL0, MAIN_CNTRL0_SR);
 
 	/* PLL registers common configuration */
-	reg_write(encoder, REG_PLL_SERIAL_1, 0x00);
-	reg_write(encoder, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(1));
-	reg_write(encoder, REG_PLL_SERIAL_3, 0x00);
-	reg_write(encoder, REG_SERIALIZER,   0x00);
-	reg_write(encoder, REG_BUFFER_OUT,   0x00);
-	reg_write(encoder, REG_PLL_SCG1,     0x00);
-	reg_write(encoder, REG_AUDIO_DIV,    AUDIO_DIV_SERCLK_8);
-	reg_write(encoder, REG_SEL_CLK,      SEL_CLK_SEL_CLK1 | SEL_CLK_ENA_SC_CLK);
-	reg_write(encoder, REG_PLL_SCGN1,    0xfa);
-	reg_write(encoder, REG_PLL_SCGN2,    0x00);
-	reg_write(encoder, REG_PLL_SCGR1,    0x5b);
-	reg_write(encoder, REG_PLL_SCGR2,    0x00);
-	reg_write(encoder, REG_PLL_SCG2,     0x10);
+	reg_write(priv, REG_PLL_SERIAL_1, 0x00);
+	reg_write(priv, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(1));
+	reg_write(priv, REG_PLL_SERIAL_3, 0x00);
+	reg_write(priv, REG_SERIALIZER,   0x00);
+	reg_write(priv, REG_BUFFER_OUT,   0x00);
+	reg_write(priv, REG_PLL_SCG1,     0x00);
+	reg_write(priv, REG_AUDIO_DIV,    AUDIO_DIV_SERCLK_8);
+	reg_write(priv, REG_SEL_CLK,      SEL_CLK_SEL_CLK1 | SEL_CLK_ENA_SC_CLK);
+	reg_write(priv, REG_PLL_SCGN1,    0xfa);
+	reg_write(priv, REG_PLL_SCGN2,    0x00);
+	reg_write(priv, REG_PLL_SCGR1,    0x5b);
+	reg_write(priv, REG_PLL_SCGR2,    0x00);
+	reg_write(priv, REG_PLL_SCG2,     0x10);
 
 	/* Write the default value MUX register */
-	reg_write(encoder, REG_MUX_VP_VIP_OUT, 0x24);
+	reg_write(priv, REG_MUX_VP_VIP_OUT, 0x24);
 }
 
 static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes)
@@ -513,18 +512,18 @@ static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes)
 #define PB(x) (HB(2) + 1 + (x))
 
 static void
-tda998x_write_if(struct drm_encoder *encoder, uint8_t bit, uint16_t addr,
+tda998x_write_if(struct tda998x_priv *priv, uint8_t bit, uint16_t addr,
 		 uint8_t *buf, size_t size)
 {
 	buf[PB(0)] = tda998x_cksum(buf, size);
 
-	reg_clear(encoder, REG_DIP_IF_FLAGS, bit);
-	reg_write_range(encoder, addr, buf, size);
-	reg_set(encoder, REG_DIP_IF_FLAGS, bit);
+	reg_clear(priv, REG_DIP_IF_FLAGS, bit);
+	reg_write_range(priv, addr, buf, size);
+	reg_set(priv, REG_DIP_IF_FLAGS, bit);
 }
 
 static void
-tda998x_write_aif(struct drm_encoder *encoder, struct tda998x_encoder_params *p)
+tda998x_write_aif(struct tda998x_priv *priv, struct tda998x_encoder_params *p)
 {
 	uint8_t buf[PB(5) + 1];
 
@@ -537,12 +536,12 @@ tda998x_write_aif(struct drm_encoder *encoder, struct tda998x_encoder_params *p)
 	buf[PB(4)] = p->audio_frame[4];
 	buf[PB(5)] = p->audio_frame[5] & 0xf8; /* DM_INH + LSV */
 
-	tda998x_write_if(encoder, DIP_IF_FLAGS_IF4, REG_IF4_HB0, buf,
+	tda998x_write_if(priv, DIP_IF_FLAGS_IF4, REG_IF4_HB0, buf,
 			 sizeof(buf));
 }
 
 static void
-tda998x_write_avi(struct drm_encoder *encoder, struct drm_display_mode *mode)
+tda998x_write_avi(struct tda998x_priv *priv, struct drm_display_mode *mode)
 {
 	uint8_t buf[PB(13) + 1];
 
@@ -554,36 +553,36 @@ tda998x_write_avi(struct drm_encoder *encoder, struct drm_display_mode *mode)
 	buf[PB(3)] = HDMI_QUANTIZATION_RANGE_FULL << 2;
 	buf[PB(4)] = drm_match_cea_mode(mode);
 
-	tda998x_write_if(encoder, DIP_IF_FLAGS_IF2, REG_IF2_HB0, buf,
+	tda998x_write_if(priv, DIP_IF_FLAGS_IF2, REG_IF2_HB0, buf,
 			 sizeof(buf));
 }
 
-static void tda998x_audio_mute(struct drm_encoder *encoder, bool on)
+static void tda998x_audio_mute(struct tda998x_priv *priv, bool on)
 {
 	if (on) {
-		reg_set(encoder, REG_SOFTRESET, SOFTRESET_AUDIO);
-		reg_clear(encoder, REG_SOFTRESET, SOFTRESET_AUDIO);
-		reg_set(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
+		reg_set(priv, REG_SOFTRESET, SOFTRESET_AUDIO);
+		reg_clear(priv, REG_SOFTRESET, SOFTRESET_AUDIO);
+		reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
 	} else {
-		reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
+		reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
 	}
 }
 
 static void
-tda998x_configure_audio(struct drm_encoder *encoder,
+tda998x_configure_audio(struct tda998x_priv *priv,
 		struct drm_display_mode *mode, struct tda998x_encoder_params *p)
 {
 	uint8_t buf[6], clksel_aip, clksel_fs, ca_i2s, cts_n, adiv;
 	uint32_t n;
 
 	/* Enable audio ports */
-	reg_write(encoder, REG_ENA_AP, p->audio_cfg);
-	reg_write(encoder, REG_ENA_ACLK, p->audio_clk_cfg);
+	reg_write(priv, REG_ENA_AP, p->audio_cfg);
+	reg_write(priv, REG_ENA_ACLK, p->audio_clk_cfg);
 
 	/* Set audio input source */
 	switch (p->audio_format) {
 	case AFMT_SPDIF:
-		reg_write(encoder, REG_MUX_AP, 0x40);
+		reg_write(priv, REG_MUX_AP, 0x40);
 		clksel_aip = AIP_CLKSEL_AIP(0);
 		/* FS64SPDIF */
 		clksel_fs = AIP_CLKSEL_FS(2);
@@ -592,7 +591,7 @@ tda998x_configure_audio(struct drm_encoder *encoder,
 		break;
 
 	case AFMT_I2S:
-		reg_write(encoder, REG_MUX_AP, 0x64);
+		reg_write(priv, REG_MUX_AP, 0x64);
 		clksel_aip = AIP_CLKSEL_AIP(1);
 		/* ACLK */
 		clksel_fs = AIP_CLKSEL_FS(0);
@@ -605,12 +604,12 @@ tda998x_configure_audio(struct drm_encoder *encoder,
 		return;
 	}
 
-	reg_write(encoder, REG_AIP_CLKSEL, clksel_aip);
-	reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_LAYOUT);
+	reg_write(priv, REG_AIP_CLKSEL, clksel_aip);
+	reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_LAYOUT);
 
 	/* Enable automatic CTS generation */
-	reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_ACR_MAN);
-	reg_write(encoder, REG_CTS_N, cts_n);
+	reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_ACR_MAN);
+	reg_write(priv, REG_CTS_N, cts_n);
 
 	/*
 	 * Audio input somehow depends on HDMI line rate which is
@@ -623,7 +622,7 @@ tda998x_configure_audio(struct drm_encoder *encoder,
 		adiv = AUDIO_DIV_SERCLK_16;
 	else
 		adiv = AUDIO_DIV_SERCLK_8;
-	reg_write(encoder, REG_AUDIO_DIV, adiv);
+	reg_write(priv, REG_AUDIO_DIV, adiv);
 
 	/*
 	 * This is the approximate value of N, which happens to be
@@ -638,28 +637,28 @@ tda998x_configure_audio(struct drm_encoder *encoder,
 	buf[3] = n;
 	buf[4] = n >> 8;
 	buf[5] = n >> 16;
-	reg_write_range(encoder, REG_ACR_CTS_0, buf, 6);
+	reg_write_range(priv, REG_ACR_CTS_0, buf, 6);
 
 	/* Set CTS clock reference */
-	reg_write(encoder, REG_AIP_CLKSEL, clksel_aip | clksel_fs);
+	reg_write(priv, REG_AIP_CLKSEL, clksel_aip | clksel_fs);
 
 	/* Reset CTS generator */
-	reg_set(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
-	reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
+	reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
+	reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
 
 	/* Write the channel status */
 	buf[0] = 0x04;
 	buf[1] = 0x00;
 	buf[2] = 0x00;
 	buf[3] = 0xf1;
-	reg_write_range(encoder, REG_CH_STAT_B(0), buf, 4);
+	reg_write_range(priv, REG_CH_STAT_B(0), buf, 4);
 
-	tda998x_audio_mute(encoder, true);
+	tda998x_audio_mute(priv, true);
 	mdelay(20);
-	tda998x_audio_mute(encoder, false);
+	tda998x_audio_mute(priv, false);
 
 	/* Write the audio information packet */
-	tda998x_write_aif(encoder, p);
+	tda998x_write_aif(priv, p);
 }
 
 /* DRM encoder functions */
@@ -701,19 +700,19 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
 	switch (mode) {
 	case DRM_MODE_DPMS_ON:
 		/* enable video ports, audio will be enabled later */
-		reg_write(encoder, REG_ENA_VP_0, 0xff);
-		reg_write(encoder, REG_ENA_VP_1, 0xff);
-		reg_write(encoder, REG_ENA_VP_2, 0xff);
+		reg_write(priv, REG_ENA_VP_0, 0xff);
+		reg_write(priv, REG_ENA_VP_1, 0xff);
+		reg_write(priv, REG_ENA_VP_2, 0xff);
 		/* set muxing after enabling ports: */
-		reg_write(encoder, REG_VIP_CNTRL_0, priv->vip_cntrl_0);
-		reg_write(encoder, REG_VIP_CNTRL_1, priv->vip_cntrl_1);
-		reg_write(encoder, REG_VIP_CNTRL_2, priv->vip_cntrl_2);
+		reg_write(priv, REG_VIP_CNTRL_0, priv->vip_cntrl_0);
+		reg_write(priv, REG_VIP_CNTRL_1, priv->vip_cntrl_1);
+		reg_write(priv, REG_VIP_CNTRL_2, priv->vip_cntrl_2);
 		break;
 	case DRM_MODE_DPMS_OFF:
 		/* disable video ports */
-		reg_write(encoder, REG_ENA_VP_0, 0x00);
-		reg_write(encoder, REG_ENA_VP_1, 0x00);
-		reg_write(encoder, REG_ENA_VP_2, 0x00);
+		reg_write(priv, REG_ENA_VP_0, 0x00);
+		reg_write(priv, REG_ENA_VP_1, 0x00);
+		reg_write(priv, REG_ENA_VP_2, 0x00);
 		break;
 	}
 
@@ -826,57 +825,57 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	div = 148500 / mode->clock;
 
 	/* mute the audio FIFO: */
-	reg_set(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
+	reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
 
 	/* set HDMI HDCP mode off: */
-	reg_set(encoder, REG_TBG_CNTRL_1, TBG_CNTRL_1_DWIN_DIS);
-	reg_clear(encoder, REG_TX33, TX33_HDMI);
+	reg_set(priv, REG_TBG_CNTRL_1, TBG_CNTRL_1_DWIN_DIS);
+	reg_clear(priv, REG_TX33, TX33_HDMI);
+	reg_write(priv, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(0));
 
-	reg_write(encoder, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(0));
 	/* no pre-filter or interpolator: */
-	reg_write(encoder, REG_HVF_CNTRL_0, HVF_CNTRL_0_PREFIL(0) |
+	reg_write(priv, REG_HVF_CNTRL_0, HVF_CNTRL_0_PREFIL(0) |
 			HVF_CNTRL_0_INTPOL(0));
-	reg_write(encoder, REG_VIP_CNTRL_5, VIP_CNTRL_5_SP_CNT(0));
-	reg_write(encoder, REG_VIP_CNTRL_4, VIP_CNTRL_4_BLANKIT(0) |
+	reg_write(priv, REG_VIP_CNTRL_5, VIP_CNTRL_5_SP_CNT(0));
+	reg_write(priv, REG_VIP_CNTRL_4, VIP_CNTRL_4_BLANKIT(0) |
 			VIP_CNTRL_4_BLC(0));
-	reg_clear(encoder, REG_PLL_SERIAL_3, PLL_SERIAL_3_SRL_CCIR);
+	reg_clear(priv, REG_PLL_SERIAL_3, PLL_SERIAL_3_SRL_CCIR);
 
-	reg_clear(encoder, REG_PLL_SERIAL_1, PLL_SERIAL_1_SRL_MAN_IZ);
-	reg_clear(encoder, REG_PLL_SERIAL_3, PLL_SERIAL_3_SRL_DE);
-	reg_write(encoder, REG_SERIALIZER, 0);
-	reg_write(encoder, REG_HVF_CNTRL_1, HVF_CNTRL_1_VQR(0));
+	reg_clear(priv, REG_PLL_SERIAL_1, PLL_SERIAL_1_SRL_MAN_IZ);
+	reg_clear(priv, REG_PLL_SERIAL_3, PLL_SERIAL_3_SRL_DE);
+	reg_write(priv, REG_SERIALIZER, 0);
+	reg_write(priv, REG_HVF_CNTRL_1, HVF_CNTRL_1_VQR(0));
 
 	/* TODO enable pixel repeat for pixel rates less than 25Msamp/s */
 	rep = 0;
-	reg_write(encoder, REG_RPT_CNTRL, 0);
-	reg_write(encoder, REG_SEL_CLK, SEL_CLK_SEL_VRF_CLK(0) |
+	reg_write(priv, REG_RPT_CNTRL, 0);
+	reg_write(priv, REG_SEL_CLK, SEL_CLK_SEL_VRF_CLK(0) |
 			SEL_CLK_SEL_CLK1 | SEL_CLK_ENA_SC_CLK);
 
-	reg_write(encoder, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(div) |
+	reg_write(priv, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(div) |
 			PLL_SERIAL_2_SRL_PR(rep));
 
 	/* set color matrix bypass flag: */
-	reg_set(encoder, REG_MAT_CONTRL, MAT_CONTRL_MAT_BP);
+	reg_set(priv, REG_MAT_CONTRL, MAT_CONTRL_MAT_BP);
 
 	/* set BIAS tmds value: */
-	reg_write(encoder, REG_ANA_GENERAL, 0x09);
+	reg_write(priv, REG_ANA_GENERAL, 0x09);
 
-	reg_clear(encoder, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_MTHD);
+	reg_clear(priv, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_MTHD);
 
 	/*
 	 * Sync on rising HSYNC/VSYNC
 	 */
-	reg_write(encoder, REG_VIP_CNTRL_3, 0);
-	reg_set(encoder, REG_VIP_CNTRL_3, VIP_CNTRL_3_SYNC_HS);
+	reg_write(priv, REG_VIP_CNTRL_3, 0);
+	reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_SYNC_HS);
 
 	/*
 	 * TDA19988 requires high-active sync at input stage,
 	 * so invert low-active sync provided by master encoder here
 	 */
 	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
-		reg_set(encoder, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL);
+		reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL);
 	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
-		reg_set(encoder, REG_VIP_CNTRL_3, VIP_CNTRL_3_V_TGL);
+		reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_V_TGL);
 
 	/*
 	 * Always generate sync polarity relative to input sync and
@@ -887,49 +886,49 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 		reg |= TBG_CNTRL_1_H_TGL;
 	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
 		reg |= TBG_CNTRL_1_V_TGL;
-	reg_write(encoder, REG_TBG_CNTRL_1, reg);
-
-	reg_write(encoder, REG_VIDFORMAT, 0x00);
-	reg_write16(encoder, REG_REFPIX_MSB, ref_pix);
-	reg_write16(encoder, REG_REFLINE_MSB, ref_line);
-	reg_write16(encoder, REG_NPIX_MSB, n_pix);
-	reg_write16(encoder, REG_NLINE_MSB, n_line);
-	reg_write16(encoder, REG_VS_LINE_STRT_1_MSB, vs1_line_s);
-	reg_write16(encoder, REG_VS_PIX_STRT_1_MSB, vs1_pix_s);
-	reg_write16(encoder, REG_VS_LINE_END_1_MSB, vs1_line_e);
-	reg_write16(encoder, REG_VS_PIX_END_1_MSB, vs1_pix_e);
-	reg_write16(encoder, REG_VS_LINE_STRT_2_MSB, vs2_line_s);
-	reg_write16(encoder, REG_VS_PIX_STRT_2_MSB, vs2_pix_s);
-	reg_write16(encoder, REG_VS_LINE_END_2_MSB, vs2_line_e);
-	reg_write16(encoder, REG_VS_PIX_END_2_MSB, vs2_pix_e);
-	reg_write16(encoder, REG_HS_PIX_START_MSB, hs_pix_s);
-	reg_write16(encoder, REG_HS_PIX_STOP_MSB, hs_pix_e);
-	reg_write16(encoder, REG_VWIN_START_1_MSB, vwin1_line_s);
-	reg_write16(encoder, REG_VWIN_END_1_MSB, vwin1_line_e);
-	reg_write16(encoder, REG_VWIN_START_2_MSB, vwin2_line_s);
-	reg_write16(encoder, REG_VWIN_END_2_MSB, vwin2_line_e);
-	reg_write16(encoder, REG_DE_START_MSB, de_pix_s);
-	reg_write16(encoder, REG_DE_STOP_MSB, de_pix_e);
+	reg_write(priv, REG_TBG_CNTRL_1, reg);
+
+	reg_write(priv, REG_VIDFORMAT, 0x00);
+	reg_write16(priv, REG_REFPIX_MSB, ref_pix);
+	reg_write16(priv, REG_REFLINE_MSB, ref_line);
+	reg_write16(priv, REG_NPIX_MSB, n_pix);
+	reg_write16(priv, REG_NLINE_MSB, n_line);
+	reg_write16(priv, REG_VS_LINE_STRT_1_MSB, vs1_line_s);
+	reg_write16(priv, REG_VS_PIX_STRT_1_MSB, vs1_pix_s);
+	reg_write16(priv, REG_VS_LINE_END_1_MSB, vs1_line_e);
+	reg_write16(priv, REG_VS_PIX_END_1_MSB, vs1_pix_e);
+	reg_write16(priv, REG_VS_LINE_STRT_2_MSB, vs2_line_s);
+	reg_write16(priv, REG_VS_PIX_STRT_2_MSB, vs2_pix_s);
+	reg_write16(priv, REG_VS_LINE_END_2_MSB, vs2_line_e);
+	reg_write16(priv, REG_VS_PIX_END_2_MSB, vs2_pix_e);
+	reg_write16(priv, REG_HS_PIX_START_MSB, hs_pix_s);
+	reg_write16(priv, REG_HS_PIX_STOP_MSB, hs_pix_e);
+	reg_write16(priv, REG_VWIN_START_1_MSB, vwin1_line_s);
+	reg_write16(priv, REG_VWIN_END_1_MSB, vwin1_line_e);
+	reg_write16(priv, REG_VWIN_START_2_MSB, vwin2_line_s);
+	reg_write16(priv, REG_VWIN_END_2_MSB, vwin2_line_e);
+	reg_write16(priv, REG_DE_START_MSB, de_pix_s);
+	reg_write16(priv, REG_DE_STOP_MSB, de_pix_e);
 
 	if (priv->rev == TDA19988) {
 		/* let incoming pixels fill the active space (if any) */
-		reg_write(encoder, REG_ENABLE_SPACE, 0x01);
+		reg_write(priv, REG_ENABLE_SPACE, 0x01);
 	}
 
 	/* must be last register set: */
-	reg_clear(encoder, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_ONCE);
+	reg_clear(priv, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_ONCE);
 
 	/* Only setup the info frames if the sink is HDMI */
 	if (priv->is_hdmi_sink) {
 		/* We need to turn HDMI HDCP stuff on to get audio through */
-		reg_clear(encoder, REG_TBG_CNTRL_1, TBG_CNTRL_1_DWIN_DIS);
-		reg_write(encoder, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(1));
-		reg_set(encoder, REG_TX33, TX33_HDMI);
+		reg_clear(priv, REG_TBG_CNTRL_1, TBG_CNTRL_1_DWIN_DIS);
+		reg_write(priv, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(1));
+		reg_set(priv, REG_TX33, TX33_HDMI);
 
-		tda998x_write_avi(encoder, adjusted_mode);
+		tda998x_write_avi(priv, adjusted_mode);
 
 		if (priv->params.audio_cfg)
-			tda998x_configure_audio(encoder, adjusted_mode,
+			tda998x_configure_audio(priv, adjusted_mode,
 						&priv->params);
 	}
 }
@@ -938,7 +937,9 @@ static enum drm_connector_status
 tda998x_encoder_detect(struct drm_encoder *encoder,
 		      struct drm_connector *connector)
 {
-	uint8_t val = cec_read(encoder, REG_CEC_RXSHPDLEV);
+	struct tda998x_priv *priv = to_tda998x_priv(encoder);
+	uint8_t val = cec_read(priv, REG_CEC_RXSHPDLEV);
+
 	return (val & CEC_RXSHPDLEV_HPD) ? connector_status_connected :
 			connector_status_disconnected;
 }
@@ -946,29 +947,30 @@ tda998x_encoder_detect(struct drm_encoder *encoder,
 static int
 read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk)
 {
+	struct tda998x_priv *priv = to_tda998x_priv(encoder);
 	uint8_t offset, segptr;
 	int ret, i;
 
 	/* enable EDID read irq: */
-	reg_set(encoder, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
+	reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
 
 	offset = (blk & 1) ? 128 : 0;
 	segptr = blk / 2;
 
-	reg_write(encoder, REG_DDC_ADDR, 0xa0);
-	reg_write(encoder, REG_DDC_OFFS, offset);
-	reg_write(encoder, REG_DDC_SEGM_ADDR, 0x60);
-	reg_write(encoder, REG_DDC_SEGM, segptr);
+	reg_write(priv, REG_DDC_ADDR, 0xa0);
+	reg_write(priv, REG_DDC_OFFS, offset);
+	reg_write(priv, REG_DDC_SEGM_ADDR, 0x60);
+	reg_write(priv, REG_DDC_SEGM, segptr);
 
 	/* enable reading EDID: */
-	reg_write(encoder, REG_EDID_CTRL, 0x1);
+	reg_write(priv, REG_EDID_CTRL, 0x1);
 
 	/* flag must be cleared by sw: */
-	reg_write(encoder, REG_EDID_CTRL, 0x0);
+	reg_write(priv, REG_EDID_CTRL, 0x0);
 
 	/* wait for block read to complete: */
 	for (i = 100; i > 0; i--) {
-		uint8_t val = reg_read(encoder, REG_INT_FLAGS_2);
+		uint8_t val = reg_read(priv, REG_INT_FLAGS_2);
 		if (val & INT_FLAGS_2_EDID_BLK_RD)
 			break;
 		msleep(1);
@@ -977,14 +979,14 @@ read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk)
 	if (i == 0)
 		return -ETIMEDOUT;
 
-	ret = reg_read_range(encoder, REG_EDID_DATA_0, buf, EDID_LENGTH);
+	ret = reg_read_range(priv, REG_EDID_DATA_0, buf, EDID_LENGTH);
 	if (ret != EDID_LENGTH) {
 		dev_err(encoder->dev->dev, "failed to read edid block %d: %d",
 				blk, ret);
 		return ret;
 	}
 
-	reg_clear(encoder, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
+	reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
 
 	return 0;
 }
@@ -1001,7 +1003,7 @@ do_get_edid(struct drm_encoder *encoder)
 		return NULL;
 
 	if (priv->rev == TDA19988)
-		reg_clear(encoder, REG_TX4, TX4_PD_RAM);
+		reg_clear(priv, REG_TX4, TX4_PD_RAM);
 
 	/* base block fetch */
 	if (read_edid_block(encoder, block, 0))
@@ -1041,13 +1043,13 @@ do_get_edid(struct drm_encoder *encoder)
 
 done:
 	if (priv->rev == TDA19988)
-		reg_set(encoder, REG_TX4, TX4_PD_RAM);
+		reg_set(priv, REG_TX4, TX4_PD_RAM);
 
 	return block;
 
 fail:
 	if (priv->rev == TDA19988)
-		reg_set(encoder, REG_TX4, TX4_PD_RAM);
+		reg_set(priv, REG_TX4, TX4_PD_RAM);
 	dev_warn(encoder->dev->dev, "failed to read EDID\n");
 	kfree(block);
 	return NULL;
@@ -1131,7 +1133,6 @@ tda998x_encoder_init(struct i2c_client *client,
 		    struct drm_device *dev,
 		    struct drm_encoder_slave *encoder_slave)
 {
-	struct drm_encoder *encoder = &encoder_slave->base;
 	struct tda998x_priv *priv;
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
@@ -1143,6 +1144,7 @@ tda998x_encoder_init(struct i2c_client *client,
 	priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
 
 	priv->current_page = 0;
+	priv->hdmi = client;
 	priv->cec = i2c_new_dummy(client->adapter, 0x34);
 	priv->dpms = DRM_MODE_DPMS_OFF;
 
@@ -1150,14 +1152,14 @@ tda998x_encoder_init(struct i2c_client *client,
 	encoder_slave->slave_funcs = &tda998x_encoder_funcs;
 
 	/* wake up the device: */
-	cec_write(encoder, REG_CEC_ENAMODS,
+	cec_write(priv, REG_CEC_ENAMODS,
 			CEC_ENAMODS_EN_RXSENS | CEC_ENAMODS_EN_HDMI);
 
-	tda998x_reset(encoder);
+	tda998x_reset(priv);
 
 	/* read version: */
-	priv->rev = reg_read(encoder, REG_VERSION_LSB) |
-			reg_read(encoder, REG_VERSION_MSB) << 8;
+	priv->rev = reg_read(priv, REG_VERSION_LSB) |
+			reg_read(priv, REG_VERSION_MSB) << 8;
 
 	/* mask off feature bits: */
 	priv->rev &= ~0x30; /* not-hdcp and not-scalar bit */
@@ -1173,16 +1175,16 @@ tda998x_encoder_init(struct i2c_client *client,
 	}
 
 	/* after reset, enable DDC: */
-	reg_write(encoder, REG_DDC_DISABLE, 0x00);
+	reg_write(priv, REG_DDC_DISABLE, 0x00);
 
 	/* set clock on DDC channel: */
-	reg_write(encoder, REG_TX3, 39);
+	reg_write(priv, REG_TX3, 39);
 
 	/* if necessary, disable multi-master: */
 	if (priv->rev == TDA19989)
-		reg_set(encoder, REG_I2C_MASTER, I2C_MASTER_DIS_MM);
+		reg_set(priv, REG_I2C_MASTER, I2C_MASTER_DIS_MM);
 
-	cec_write(encoder, REG_CEC_FRO_IM_CLK_CTRL,
+	cec_write(priv, REG_CEC_FRO_IM_CLK_CTRL,
 			CEC_FRO_IM_CLK_CTRL_GHOST_DIS | CEC_FRO_IM_CLK_CTRL_IMCLK_SEL);
 
 	return 0;
-- 
1.8.5.3

[PATCH v3 04/24] drm/i2c: tda998x: change probe message origin

[Jean-Francois Moine]

On probe, a message giving the TDA chip version seems to come from the
DRM driver:

	armada-drm armada-510-drm: found TDA19988

This patch changes the originator of the message to the TDA driver:

	tda998x 0-0070: found TDA19988

Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 320b37f..fbd7937 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1195,12 +1195,21 @@ tda998x_encoder_init(struct i2c_client *client,
 	priv->rev &= ~0x30; /* not-hdcp and not-scalar bit */
 
 	switch (priv->rev) {
-	case TDA9989N2:  dev_info(dev->dev, "found TDA9989 n2");  break;
-	case TDA19989:   dev_info(dev->dev, "found TDA19989");    break;
-	case TDA19989N2: dev_info(dev->dev, "found TDA19989 n2"); break;
-	case TDA19988:   dev_info(dev->dev, "found TDA19988");    break;
+	case TDA9989N2:
+		dev_info(&client->dev, "found TDA9989 n2");
+		break;
+	case TDA19989:
+		dev_info(&client->dev, "found TDA19989");
+		break;
+	case TDA19989N2:
+		dev_info(&client->dev, "found TDA19989 n2");
+		break;
+	case TDA19988:
+		dev_info(&client->dev, "found TDA19988");
+		break;
 	default:
-		DBG("found unsupported device: %04x", priv->rev);
+		dev_err(&client->dev, "found unsupported device: %04x\n",
+			priv->rev);
 		goto fail;
 	}
 
-- 
1.8.5.3

[PATCH v3 02/24] drm/i2c: tda998x: check more I/O errors

[Jean-Francois Moine]

This patch adds more error checking inn I2C I/O functions.
In case of I/O error, this permits to avoid writing in bad controller
pages, a bad chipset detection or looping when getting the EDID.

Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 57 +++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 26dd299..11f0607 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -362,7 +362,7 @@ fail:
 	return 0;
 }
 
-static void
+static int
 set_page(struct tda998x_priv *priv, uint16_t reg)
 {
 	if (REG2PAGE(reg) != priv->current_page) {
@@ -371,11 +371,14 @@ set_page(struct tda998x_priv *priv, uint16_t reg)
 				REG_CURPAGE, REG2PAGE(reg)
 		};
 		int ret = i2c_master_send(client, buf, sizeof(buf));
-		if (ret < 0)
+		if (ret < 0) {
 			dev_err(&client->dev, "Error %d writing to REG_CURPAGE\n", ret);
+			return ret;
+		}
 
 		priv->current_page = REG2PAGE(reg);
 	}
+	return 0;
 }
 
 static int
@@ -385,7 +388,9 @@ reg_read_range(struct tda998x_priv *priv, uint16_t reg, char *buf, int cnt)
 	uint8_t addr = REG2ADDR(reg);
 	int ret;
 
-	set_page(priv, reg);
+	ret = set_page(priv, reg);
+	if (ret < 0)
+		return ret;
 
 	ret = i2c_master_send(client, &addr, sizeof(addr));
 	if (ret < 0)
@@ -412,18 +417,24 @@ reg_write_range(struct tda998x_priv *priv, uint16_t reg, uint8_t *p, int cnt)
 	buf[0] = REG2ADDR(reg);
 	memcpy(&buf[1], p, cnt);
 
-	set_page(priv, reg);
+	ret = set_page(priv, reg);
+	if (ret < 0)
+		return;
 
 	ret = i2c_master_send(client, buf, cnt + 1);
 	if (ret < 0)
 		dev_err(&client->dev, "Error %d writing to 0x%x\n", ret, reg);
 }
 
-static uint8_t
+static int
 reg_read(struct tda998x_priv *priv, uint16_t reg)
 {
 	uint8_t val = 0;
-	reg_read_range(priv, reg, &val, sizeof(val));
+	int ret;
+
+	ret = reg_read_range(priv, reg, &val, sizeof(val));
+	if (ret < 0)
+		return ret;
 	return val;
 }
 
@@ -434,7 +445,9 @@ reg_write(struct tda998x_priv *priv, uint16_t reg, uint8_t val)
 	uint8_t buf[] = {REG2ADDR(reg), val};
 	int ret;
 
-	set_page(priv, reg);
+	ret = set_page(priv, reg);
+	if (ret < 0)
+		return;
 
 	ret = i2c_master_send(client, buf, ARRAY_SIZE(buf));
 	if (ret < 0)
@@ -448,7 +461,9 @@ reg_write16(struct tda998x_priv *priv, uint16_t reg, uint16_t val)
 	uint8_t buf[] = {REG2ADDR(reg), val >> 8, val};
 	int ret;
 
-	set_page(priv, reg);
+	ret = set_page(priv, reg);
+	if (ret < 0)
+		return;
 
 	ret = i2c_master_send(client, buf, ARRAY_SIZE(buf));
 	if (ret < 0)
@@ -458,13 +473,21 @@ reg_write16(struct tda998x_priv *priv, uint16_t reg, uint16_t val)
 static void
 reg_set(struct tda998x_priv *priv, uint16_t reg, uint8_t val)
 {
-	reg_write(priv, reg, reg_read(priv, reg) | val);
+	int old_val;
+
+	old_val = reg_read(priv, reg);
+	if (old_val >= 0)
+		reg_write(priv, reg, old_val | val);
 }
 
 static void
 reg_clear(struct tda998x_priv *priv, uint16_t reg, uint8_t val)
 {
-	reg_write(priv, reg, reg_read(priv, reg) & ~val);
+	int old_val;
+
+	old_val = reg_read(priv, reg);
+	if (old_val >= 0)
+		reg_write(priv, reg, old_val & ~val);
 }
 
 static void
@@ -970,8 +993,10 @@ read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk)
 
 	/* wait for block read to complete: */
 	for (i = 100; i > 0; i--) {
-		uint8_t val = reg_read(priv, REG_INT_FLAGS_2);
-		if (val & INT_FLAGS_2_EDID_BLK_RD)
+		ret = reg_read(priv, REG_INT_FLAGS_2);
+		if (ret < 0)
+			return ret;
+		if (ret & INT_FLAGS_2_EDID_BLK_RD)
 			break;
 		msleep(1);
 	}
@@ -1134,6 +1159,7 @@ tda998x_encoder_init(struct i2c_client *client,
 		    struct drm_encoder_slave *encoder_slave)
 {
 	struct tda998x_priv *priv;
+	int ret;
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -1158,8 +1184,11 @@ tda998x_encoder_init(struct i2c_client *client,
 	tda998x_reset(priv);
 
 	/* read version: */
-	priv->rev = reg_read(priv, REG_VERSION_LSB) |
-			reg_read(priv, REG_VERSION_MSB) << 8;
+	ret = reg_read(priv, REG_VERSION_LSB) |
+		(reg_read(priv, REG_VERSION_MSB) << 8);
+	if (ret < 0)
+		goto fail;
+	priv->rev = ret;
 
 	/* mask off feature bits: */
 	priv->rev &= ~0x30; /* not-hdcp and not-scalar bit */
-- 
1.8.5.3

[PATCH v3 02/24] drm/i2c: tda998x: check more I/O errors

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 07:58:38PM +0100, Jean-Francois Moine wrote:
> This patch adds more error checking inn I2C I/O functions.
> In case of I/O error, this permits to avoid writing in bad controller
> pages, a bad chipset detection or looping when getting the EDID.
> 
> Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>

Thanks.

Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH v3 03/24] drm/i2c: tda998x: code cleanup

[Jean-Francois Moine]

This patch:
- replaces ARRAY_SIZE() by sizeof() when a number of bytes is needed,
- adds a linefeed in an error message and
- removes an useless variable setting.

Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 11f0607..320b37f 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -335,7 +335,7 @@ cec_write(struct tda998x_priv *priv, uint16_t addr, uint8_t val)
 	uint8_t buf[] = {addr, val};
 	int ret;
 
-	ret = i2c_master_send(client, buf, ARRAY_SIZE(buf));
+	ret = i2c_master_send(client, buf, sizeof(buf));
 	if (ret < 0)
 		dev_err(&client->dev, "Error %d writing to cec:0x%x\n", ret, addr);
 }
@@ -372,7 +372,8 @@ set_page(struct tda998x_priv *priv, uint16_t reg)
 		};
 		int ret = i2c_master_send(client, buf, sizeof(buf));
 		if (ret < 0) {
-			dev_err(&client->dev, "Error %d writing to REG_CURPAGE\n", ret);
+			dev_err(&client->dev, "setpage %04x err %d\n",
+					reg, ret);
 			return ret;
 		}
 
@@ -449,7 +450,7 @@ reg_write(struct tda998x_priv *priv, uint16_t reg, uint8_t val)
 	if (ret < 0)
 		return;
 
-	ret = i2c_master_send(client, buf, ARRAY_SIZE(buf));
+	ret = i2c_master_send(client, buf, sizeof(buf));
 	if (ret < 0)
 		dev_err(&client->dev, "Error %d writing to 0x%x\n", ret, reg);
 }
@@ -465,7 +466,7 @@ reg_write16(struct tda998x_priv *priv, uint16_t reg, uint16_t val)
 	if (ret < 0)
 		return;
 
-	ret = i2c_master_send(client, buf, ARRAY_SIZE(buf));
+	ret = i2c_master_send(client, buf, sizeof(buf));
 	if (ret < 0)
 		dev_err(&client->dev, "Error %d writing to 0x%x\n", ret, reg);
 }
@@ -1006,7 +1007,7 @@ read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk)
 
 	ret = reg_read_range(priv, REG_EDID_DATA_0, buf, EDID_LENGTH);
 	if (ret != EDID_LENGTH) {
-		dev_err(encoder->dev->dev, "failed to read edid block %d: %d",
+		dev_err(encoder->dev->dev, "failed to read edid block %d: %d\n",
 				blk, ret);
 		return ret;
 	}
@@ -1020,7 +1021,7 @@ static uint8_t *
 do_get_edid(struct drm_encoder *encoder)
 {
 	struct tda998x_priv *priv = to_tda998x_priv(encoder);
-	int j = 0, valid_extensions = 0;
+	int j, valid_extensions = 0;
 	uint8_t *block, *new;
 	bool print_bad_edid = drm_debug & DRM_UT_KMS;
 
-- 
1.8.5.3

[PATCH v3 03/24] drm/i2c: tda998x: code cleanup

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 07:58:38PM +0100, Jean-Francois Moine wrote:
> This patch:
> - replaces ARRAY_SIZE() by sizeof() when a number of bytes is needed,
> - adds a linefeed in an error message and
> - removes an useless variable setting.
> 
> Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>

Thanks.

Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH v3 06/24] drm/i2c: tda998x: force the page register at startup time

[Jean-Francois Moine]

This patch forces the page register to be set on the first I/O operation.

Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 96b5966..b688801 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1170,7 +1170,7 @@ tda998x_encoder_init(struct i2c_client *client,
 	priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1);
 	priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
 
-	priv->current_page = 0;
+	priv->current_page = 0xff;
 	priv->hdmi = client;
 	priv->cec = i2c_new_dummy(client->adapter, 0x34);
 	priv->dpms = DRM_MODE_DPMS_OFF;
-- 
1.8.5.3

[PATCH v3 05/24] drm/i2c: tda998x: don't freeze the system at audio startup time

[Jean-Francois Moine]

This patch prevents the system to be freezed at audio startup time,
replacing mdelay by msleep.

Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index fbd7937..96b5966 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -678,7 +678,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	reg_write_range(priv, REG_CH_STAT_B(0), buf, 4);
 
 	tda998x_audio_mute(priv, true);
-	mdelay(20);
+	msleep(20);
 	tda998x_audio_mute(priv, false);
 
 	/* Write the audio information packet */
-- 
1.8.5.3

[PATCH v3 07/24] drm/i2c: tda998x: set the video mode from the adjusted value

[Jean-Francois Moine]

This patch uses always the adjusted video mode instead of a mix of
original and adjusted mode.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 66 +++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index b688801..5d82301 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -773,7 +773,7 @@ tda998x_encoder_mode_valid(struct drm_encoder *encoder,
 static void
 tda998x_encoder_mode_set(struct drm_encoder *encoder,
 			struct drm_display_mode *mode,
-			struct drm_display_mode *adjusted_mode)
+			struct drm_display_mode *adj_mode)
 {
 	struct tda998x_priv *priv = to_tda998x_priv(encoder);
 	uint16_t ref_pix, ref_line, n_pix, n_line;
@@ -802,13 +802,13 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	 * So we add +1 to all horizontal and vertical register values,
 	 * plus an additional +3 for REFPIX as we are using RGB input only.
 	 */
-	n_pix        = mode->htotal;
-	n_line       = mode->vtotal;
+	n_pix        = adj_mode->htotal;
+	n_line       = adj_mode->vtotal;
 
-	hs_pix_e     = mode->hsync_end - mode->hdisplay;
-	hs_pix_s     = mode->hsync_start - mode->hdisplay;
-	de_pix_e     = mode->htotal;
-	de_pix_s     = mode->htotal - mode->hdisplay;
+	hs_pix_e     = adj_mode->hsync_end - adj_mode->hdisplay;
+	hs_pix_s     = adj_mode->hsync_start - adj_mode->hdisplay;
+	de_pix_e     = adj_mode->htotal;
+	de_pix_s     = adj_mode->htotal - adj_mode->hdisplay;
 	ref_pix      = 3 + hs_pix_s;
 
 	/*
@@ -816,37 +816,38 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	 * those to adjust the position of the rising VS edge by adding
 	 * HSKEW to ref_pix.
 	 */
-	if (adjusted_mode->flags & DRM_MODE_FLAG_HSKEW)
-		ref_pix += adjusted_mode->hskew;
+	if (adj_mode->flags & DRM_MODE_FLAG_HSKEW)
+		ref_pix += adj_mode->hskew;
 
-	if ((mode->flags & DRM_MODE_FLAG_INTERLACE) == 0) {
-		ref_line     = 1 + mode->vsync_start - mode->vdisplay;
-		vwin1_line_s = mode->vtotal - mode->vdisplay - 1;
-		vwin1_line_e = vwin1_line_s + mode->vdisplay;
+	if ((adj_mode->flags & DRM_MODE_FLAG_INTERLACE) == 0) {
+		ref_line     = 1 + adj_mode->vsync_start - adj_mode->vdisplay;
+		vwin1_line_s = adj_mode->vtotal - adj_mode->vdisplay - 1;
+		vwin1_line_e = vwin1_line_s + adj_mode->vdisplay;
 		vs1_pix_s    = vs1_pix_e = hs_pix_s;
-		vs1_line_s   = mode->vsync_start - mode->vdisplay;
+		vs1_line_s   = adj_mode->vsync_start - adj_mode->vdisplay;
 		vs1_line_e   = vs1_line_s +
-			       mode->vsync_end - mode->vsync_start;
+			       adj_mode->vsync_end - adj_mode->vsync_start;
 		vwin2_line_s = vwin2_line_e = 0;
 		vs2_pix_s    = vs2_pix_e  = 0;
 		vs2_line_s   = vs2_line_e = 0;
 	} else {
-		ref_line     = 1 + (mode->vsync_start - mode->vdisplay)/2;
-		vwin1_line_s = (mode->vtotal - mode->vdisplay)/2;
-		vwin1_line_e = vwin1_line_s + mode->vdisplay/2;
+		ref_line     = 1 + (adj_mode->vsync_start -
+						adj_mode->vdisplay)/2;
+		vwin1_line_s = (adj_mode->vtotal - adj_mode->vdisplay)/2;
+		vwin1_line_e = vwin1_line_s + adj_mode->vdisplay/2;
 		vs1_pix_s    = vs1_pix_e = hs_pix_s;
-		vs1_line_s   = (mode->vsync_start - mode->vdisplay)/2;
+		vs1_line_s   = (adj_mode->vsync_start - adj_mode->vdisplay)/2;
 		vs1_line_e   = vs1_line_s +
-			       (mode->vsync_end - mode->vsync_start)/2;
-		vwin2_line_s = vwin1_line_s + mode->vtotal/2;
-		vwin2_line_e = vwin2_line_s + mode->vdisplay/2;
-		vs2_pix_s    = vs2_pix_e = hs_pix_s + mode->htotal/2;
-		vs2_line_s   = vs1_line_s + mode->vtotal/2 ;
+			       (adj_mode->vsync_end - adj_mode->vsync_start)/2;
+		vwin2_line_s = vwin1_line_s + adj_mode->vtotal/2;
+		vwin2_line_e = vwin2_line_s + adj_mode->vdisplay/2;
+		vs2_pix_s    = vs2_pix_e = hs_pix_s + adj_mode->htotal/2;
+		vs2_line_s   = vs1_line_s + adj_mode->vtotal/2 ;
 		vs2_line_e   = vs2_line_s +
-			       (mode->vsync_end - mode->vsync_start)/2;
+			       (adj_mode->vsync_end - adj_mode->vsync_start)/2;
 	}
 
-	div = 148500 / mode->clock;
+	div = 148500 / adj_mode->clock;
 
 	/* mute the audio FIFO: */
 	reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
@@ -896,9 +897,9 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	 * TDA19988 requires high-active sync at input stage,
 	 * so invert low-active sync provided by master encoder here
 	 */
-	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+	if (adj_mode->flags & DRM_MODE_FLAG_NHSYNC)
 		reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL);
-	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
+	if (adj_mode->flags & DRM_MODE_FLAG_NVSYNC)
 		reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_V_TGL);
 
 	/*
@@ -906,9 +907,9 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	 * revert input stage toggled sync at output stage
 	 */
 	reg = TBG_CNTRL_1_TGL_EN;
-	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+	if (adj_mode->flags & DRM_MODE_FLAG_NHSYNC)
 		reg |= TBG_CNTRL_1_H_TGL;
-	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
+	if (adj_mode->flags & DRM_MODE_FLAG_NVSYNC)
 		reg |= TBG_CNTRL_1_V_TGL;
 	reg_write(priv, REG_TBG_CNTRL_1, reg);
 
@@ -949,11 +950,10 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 		reg_write(priv, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(1));
 		reg_set(priv, REG_TX33, TX33_HDMI);
 
-		tda998x_write_avi(priv, adjusted_mode);
+		tda998x_write_avi(priv, adj_mode);
 
 		if (priv->params.audio_cfg)
-			tda998x_configure_audio(priv, adjusted_mode,
-						&priv->params);
+			tda998x_configure_audio(priv, adj_mode, &priv->params);
 	}
 }
 
-- 
1.8.5.3

[PATCH v3 07/24] drm/i2c: tda998x: set the video mode from the adjusted value

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 07:58:40PM +0100, Jean-Francois Moine wrote:
> This patch uses always the adjusted video mode instead of a mix of
> original and adjusted mode.
> 
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>

Nothing obviously wrong and appears to work, thanks.

Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH v3 07/24] drm/i2c: tda998x: set the video mode from the adjusted value

[Darren Etheridge]

Jean-Francois Moine <moinejf@free.fr> wrote on Sun [2014-Jan-19 19:58:40 +0100]:
> This patch uses always the adjusted video mode instead of a mix of
> original and adjusted mode.
> 
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 66 +++++++++++++++++++--------------------
>  1 file changed, 33 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index b688801..5d82301 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -773,7 +773,7 @@ tda998x_encoder_mode_valid(struct drm_encoder *encoder,
>  static void
>  tda998x_encoder_mode_set(struct drm_encoder *encoder,
>  			struct drm_display_mode *mode,
> -			struct drm_display_mode *adjusted_mode)
> +			struct drm_display_mode *adj_mode)
>  {
>  	struct tda998x_priv *priv = to_tda998x_priv(encoder);
>  	uint16_t ref_pix, ref_line, n_pix, n_line;
> @@ -802,13 +802,13 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
>  	 * So we add +1 to all horizontal and vertical register values,
>  	 * plus an additional +3 for REFPIX as we are using RGB input only.
>  	 */
> -	n_pix        = mode->htotal;
> -	n_line       = mode->vtotal;
> +	n_pix        = adj_mode->htotal;
> +	n_line       = adj_mode->vtotal;
>  
> -	hs_pix_e     = mode->hsync_end - mode->hdisplay;
> -	hs_pix_s     = mode->hsync_start - mode->hdisplay;
> -	de_pix_e     = mode->htotal;
> -	de_pix_s     = mode->htotal - mode->hdisplay;
> +	hs_pix_e     = adj_mode->hsync_end - adj_mode->hdisplay;
> +	hs_pix_s     = adj_mode->hsync_start - adj_mode->hdisplay;
> +	de_pix_e     = adj_mode->htotal;
> +	de_pix_s     = adj_mode->htotal - adj_mode->hdisplay;
>  	ref_pix      = 3 + hs_pix_s;
>  
>  	/*
> @@ -816,37 +816,38 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
>  	 * those to adjust the position of the rising VS edge by adding
>  	 * HSKEW to ref_pix.
>  	 */
> -	if (adjusted_mode->flags & DRM_MODE_FLAG_HSKEW)
> -		ref_pix += adjusted_mode->hskew;
> +	if (adj_mode->flags & DRM_MODE_FLAG_HSKEW)
> +		ref_pix += adj_mode->hskew;
>  
> -	if ((mode->flags & DRM_MODE_FLAG_INTERLACE) == 0) {
> -		ref_line     = 1 + mode->vsync_start - mode->vdisplay;
> -		vwin1_line_s = mode->vtotal - mode->vdisplay - 1;
> -		vwin1_line_e = vwin1_line_s + mode->vdisplay;
> +	if ((adj_mode->flags & DRM_MODE_FLAG_INTERLACE) == 0) {
> +		ref_line     = 1 + adj_mode->vsync_start - adj_mode->vdisplay;
> +		vwin1_line_s = adj_mode->vtotal - adj_mode->vdisplay - 1;
> +		vwin1_line_e = vwin1_line_s + adj_mode->vdisplay;
>  		vs1_pix_s    = vs1_pix_e = hs_pix_s;
> -		vs1_line_s   = mode->vsync_start - mode->vdisplay;
> +		vs1_line_s   = adj_mode->vsync_start - adj_mode->vdisplay;
>  		vs1_line_e   = vs1_line_s +
> -			       mode->vsync_end - mode->vsync_start;
> +			       adj_mode->vsync_end - adj_mode->vsync_start;
>  		vwin2_line_s = vwin2_line_e = 0;
>  		vs2_pix_s    = vs2_pix_e  = 0;
>  		vs2_line_s   = vs2_line_e = 0;
>  	} else {
> -		ref_line     = 1 + (mode->vsync_start - mode->vdisplay)/2;
> -		vwin1_line_s = (mode->vtotal - mode->vdisplay)/2;
> -		vwin1_line_e = vwin1_line_s + mode->vdisplay/2;
> +		ref_line     = 1 + (adj_mode->vsync_start -
> +						adj_mode->vdisplay)/2;
> +		vwin1_line_s = (adj_mode->vtotal - adj_mode->vdisplay)/2;
> +		vwin1_line_e = vwin1_line_s + adj_mode->vdisplay/2;
>  		vs1_pix_s    = vs1_pix_e = hs_pix_s;
> -		vs1_line_s   = (mode->vsync_start - mode->vdisplay)/2;
> +		vs1_line_s   = (adj_mode->vsync_start - adj_mode->vdisplay)/2;
>  		vs1_line_e   = vs1_line_s +
> -			       (mode->vsync_end - mode->vsync_start)/2;
> -		vwin2_line_s = vwin1_line_s + mode->vtotal/2;
> -		vwin2_line_e = vwin2_line_s + mode->vdisplay/2;
> -		vs2_pix_s    = vs2_pix_e = hs_pix_s + mode->htotal/2;
> -		vs2_line_s   = vs1_line_s + mode->vtotal/2 ;
> +			       (adj_mode->vsync_end - adj_mode->vsync_start)/2;
> +		vwin2_line_s = vwin1_line_s + adj_mode->vtotal/2;
> +		vwin2_line_e = vwin2_line_s + adj_mode->vdisplay/2;
> +		vs2_pix_s    = vs2_pix_e = hs_pix_s + adj_mode->htotal/2;
> +		vs2_line_s   = vs1_line_s + adj_mode->vtotal/2 ;
>  		vs2_line_e   = vs2_line_s +
> -			       (mode->vsync_end - mode->vsync_start)/2;
> +			       (adj_mode->vsync_end - adj_mode->vsync_start)/2;
>  	}
>  
> -	div = 148500 / mode->clock;
> +	div = 148500 / adj_mode->clock;
>  
>  	/* mute the audio FIFO: */
>  	reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
> @@ -896,9 +897,9 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
>  	 * TDA19988 requires high-active sync at input stage,
>  	 * so invert low-active sync provided by master encoder here
>  	 */
> -	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> +	if (adj_mode->flags & DRM_MODE_FLAG_NHSYNC)
>  		reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL);
> -	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> +	if (adj_mode->flags & DRM_MODE_FLAG_NVSYNC)
>  		reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_V_TGL);
>  

Using the adj_mode->flags breaks a workaround I had done on BeagleBone Black
(tilcdc + tda998x) to resolve an issue with out of spec syncs from the
tlcdc.  I invert the HSYNC in adj_mode->flags but don't want the tda998x to
really know that I am doing that so I use adj_mode in the tilcdc driver, and
mode here in the tda998x driver.  The theory being adj_mode contains whatever
workarounds I need to do for the driving device and mode has the pristine
values that I want to send to the monitor.  I would need to look if there is a
different way to solve this as I am guessing you are actually using adj_mode in
the manner it was intended.

Otherwise this patch series is working on BeagleBone Black - I have only tried
video so far (not audio).

Darren

>  	/*
> @@ -906,9 +907,9 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
>  	 * revert input stage toggled sync at output stage
>  	 */
>  	reg = TBG_CNTRL_1_TGL_EN;
> -	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> +	if (adj_mode->flags & DRM_MODE_FLAG_NHSYNC)
>  		reg |= TBG_CNTRL_1_H_TGL;
> -	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> +	if (adj_mode->flags & DRM_MODE_FLAG_NVSYNC)
>  		reg |= TBG_CNTRL_1_V_TGL;
>  	reg_write(priv, REG_TBG_CNTRL_1, reg);
>  
> @@ -949,11 +950,10 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
>  		reg_write(priv, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(1));
>  		reg_set(priv, REG_TX33, TX33_HDMI);
>  
> -		tda998x_write_avi(priv, adjusted_mode);
> +		tda998x_write_avi(priv, adj_mode);
>  
>  		if (priv->params.audio_cfg)
> -			tda998x_configure_audio(priv, adjusted_mode,
> -						&priv->params);
> +			tda998x_configure_audio(priv, adj_mode, &priv->params);
>  	}
>  }
>  
> -- 
> 1.8.5.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 07/24] drm/i2c: tda998x: set the video mode from the adjusted value

[Jean-Francois Moine]

On Thu, 23 Jan 2014 17:29:07 -0600
Darren Etheridge <detheridge@ti.com> wrote:

> > @@ -896,9 +897,9 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
> >  	 * TDA19988 requires high-active sync at input stage,
> >  	 * so invert low-active sync provided by master encoder here
> >  	 */
> > -	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> > +	if (adj_mode->flags & DRM_MODE_FLAG_NHSYNC)
> >  		reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL);
> > -	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> > +	if (adj_mode->flags & DRM_MODE_FLAG_NVSYNC)
> >  		reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_V_TGL);
> >    
> 
> Using the adj_mode->flags breaks a workaround I had done on BeagleBone Black
> (tilcdc + tda998x) to resolve an issue with out of spec syncs from the
> tlcdc.  I invert the HSYNC in adj_mode->flags but don't want the tda998x to
> really know that I am doing that so I use adj_mode in the tilcdc driver, and
> mode here in the tda998x driver.  The theory being adj_mode contains whatever
> workarounds I need to do for the driving device and mode has the pristine
> values that I want to send to the monitor.  I would need to look if there is a
> different way to solve this as I am guessing you are actually using adj_mode in
> the manner it was intended.

No. In fact, I just wanted the function to use only one mode.

Looking at the other drivers, it seems that they don't touch the
adjusted_mode, so, for the Cubox, mode and adjusted_mode have same
values.

I will do an other patch so that you will not have to touch the tilcdc
driver.

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH v3 07/24] drm/i2c: tda998x: set the video mode from the adjusted value

[Darren Etheridge]

Jean-Francois Moine <moinejf@free.fr> wrote on Tue [2014-Jan-28 18:12:18 +0100]:
> On Thu, 23 Jan 2014 17:29:07 -0600
> Darren Etheridge <detheridge@ti.com> wrote:
> 
> > > @@ -896,9 +897,9 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
> > >  	 * TDA19988 requires high-active sync at input stage,
> > >  	 * so invert low-active sync provided by master encoder here
> > >  	 */
> > > -	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> > > +	if (adj_mode->flags & DRM_MODE_FLAG_NHSYNC)
> > >  		reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL);
> > > -	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> > > +	if (adj_mode->flags & DRM_MODE_FLAG_NVSYNC)
> > >  		reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_V_TGL);
> > >    
> > 
> > Using the adj_mode->flags breaks a workaround I had done on BeagleBone Black
> > (tilcdc + tda998x) to resolve an issue with out of spec syncs from the
> > tlcdc.  I invert the HSYNC in adj_mode->flags but don't want the tda998x to
> > really know that I am doing that so I use adj_mode in the tilcdc driver, and
> > mode here in the tda998x driver.  The theory being adj_mode contains whatever
> > workarounds I need to do for the driving device and mode has the pristine
> > values that I want to send to the monitor.  I would need to look if there is a
> > different way to solve this as I am guessing you are actually using adj_mode in
> > the manner it was intended.
> 
> No. In fact, I just wanted the function to use only one mode.
> 
> Looking at the other drivers, it seems that they don't touch the
> adjusted_mode, so, for the Cubox, mode and adjusted_mode have same
> values.
> 
> I will do an other patch so that you will not have to touch the tilcdc
> driver.
> 
Thanks, that would certainly be the easiest path to avoid the regression given
the other drivers that use tda998x don't currently use adj_mode.  However I
don't disagree with your reasoning, it would make sense to only use values
from one of the strctures and not a mix and match.  Anybody looking at only
the tda998x driver would be confused :) I'll work on finding a different
solution for this.

> -- 
> Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
> Jef		|		http://moinejf.free.fr/

[PATCH v3 08/24] drm/i2c: tda998x: fix bad value in the AIF

[Jean-Francois Moine]

The AIF has an uninitialized byte. This patch clears the whole buffer
before filling it.

Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 5d82301..78d324b 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -551,10 +551,10 @@ tda998x_write_aif(struct tda998x_priv *priv, struct tda998x_encoder_params *p)
 {
 	uint8_t buf[PB(5) + 1];
 
+	memset(buf, 0, sizeof(buf));
 	buf[HB(0)] = 0x84;
 	buf[HB(1)] = 0x01;
 	buf[HB(2)] = 10;
-	buf[PB(0)] = 0;
 	buf[PB(1)] = p->audio_frame[1] & 0x07; /* CC */
 	buf[PB(2)] = p->audio_frame[2] & 0x1c; /* SF */
 	buf[PB(4)] = p->audio_frame[4];
-- 
1.8.5.3

[PATCH v3 11/24] drm/i2c: tda998x: free the CEC device on encoder_destroy

[Jean-Francois Moine]

The cec i2c device is created in tda998x_encoder_init() when the DRM
driver starts.
This patch frees it when the DRM driver is unloaded.

Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index b6fdeb7..1adc794 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1124,6 +1124,8 @@ tda998x_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct tda998x_priv *priv = to_tda998x_priv(encoder);
 	drm_i2c_encoder_destroy(encoder);
+	if (priv->cec)
+		i2c_unregister_device(priv->cec);
 	kfree(priv);
 }
 
-- 
1.8.5.3

[PATCH v3 12/24] drm/i2c: tda998x: check the CEC device creation

[Jean-Francois Moine]

This patch checks if the CEC device is well created at intialization
time.

Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 1adc794..a3ebc08 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1177,6 +1177,8 @@ tda998x_encoder_init(struct i2c_client *client,
 	priv->current_page = 0xff;
 	priv->hdmi = client;
 	priv->cec = i2c_new_dummy(client->adapter, 0x34);
+	if (!priv->cec)
+		return -ENODEV;
 	priv->dpms = DRM_MODE_DPMS_OFF;
 
 	encoder_slave->slave_priv = priv;
-- 
1.8.5.3

[PATCH v3 10/24] drm/i2c: tda998x: don't read write-only registers

[Jean-Francois Moine]

This patch takes care of the write-only registers of the tda998x.

The value 'MAT_CONTRL_MAT_SC(1)' in the register MAT_CONTRL has been
set as it is at reset time.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
v3
    - remarks from Russell King
	- don't move the sync polarity setting after the setting of the
	  register TBG_CNTRL_0 which must be the last setting of the
	  init sequence

---
 drivers/gpu/drm/i2c/tda998x_drv.c | 46 ++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 978edaf..b6fdeb7 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -495,9 +495,9 @@ static void
 tda998x_reset(struct tda998x_priv *priv)
 {
 	/* reset audio and i2c master: */
-	reg_set(priv, REG_SOFTRESET, SOFTRESET_AUDIO | SOFTRESET_I2C_MASTER);
+	reg_write(priv, REG_SOFTRESET, SOFTRESET_AUDIO | SOFTRESET_I2C_MASTER);
 	msleep(50);
-	reg_clear(priv, REG_SOFTRESET, SOFTRESET_AUDIO | SOFTRESET_I2C_MASTER);
+	reg_write(priv, REG_SOFTRESET, 0);
 	msleep(50);
 
 	/* reset transmitter: */
@@ -853,7 +853,7 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
 
 	/* set HDMI HDCP mode off: */
-	reg_set(priv, REG_TBG_CNTRL_1, TBG_CNTRL_1_DWIN_DIS);
+	reg_write(priv, REG_TBG_CNTRL_1, TBG_CNTRL_1_DWIN_DIS);
 	reg_clear(priv, REG_TX33, TX33_HDMI);
 	reg_write(priv, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(0));
 
@@ -880,38 +880,28 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 			PLL_SERIAL_2_SRL_PR(rep));
 
 	/* set color matrix bypass flag: */
-	reg_set(priv, REG_MAT_CONTRL, MAT_CONTRL_MAT_BP);
+	reg_write(priv, REG_MAT_CONTRL, MAT_CONTRL_MAT_BP |
+				MAT_CONTRL_MAT_SC(1));
 
 	/* set BIAS tmds value: */
 	reg_write(priv, REG_ANA_GENERAL, 0x09);
 
-	reg_clear(priv, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_MTHD);
+	reg_write(priv, REG_TBG_CNTRL_0, 0);
 
 	/*
 	 * Sync on rising HSYNC/VSYNC
 	 */
-	reg_write(priv, REG_VIP_CNTRL_3, 0);
-	reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_SYNC_HS);
+	reg = VIP_CNTRL_3_SYNC_HS;
 
 	/*
 	 * TDA19988 requires high-active sync at input stage,
 	 * so invert low-active sync provided by master encoder here
 	 */
 	if (adj_mode->flags & DRM_MODE_FLAG_NHSYNC)
-		reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL);
+		reg |= VIP_CNTRL_3_H_TGL;
 	if (adj_mode->flags & DRM_MODE_FLAG_NVSYNC)
-		reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_V_TGL);
-
-	/*
-	 * Always generate sync polarity relative to input sync and
-	 * revert input stage toggled sync at output stage
-	 */
-	reg = TBG_CNTRL_1_TGL_EN;
-	if (adj_mode->flags & DRM_MODE_FLAG_NHSYNC)
-		reg |= TBG_CNTRL_1_H_TGL;
-	if (adj_mode->flags & DRM_MODE_FLAG_NVSYNC)
-		reg |= TBG_CNTRL_1_V_TGL;
-	reg_write(priv, REG_TBG_CNTRL_1, reg);
+		reg |= VIP_CNTRL_3_V_TGL;
+	reg_write(priv, REG_VIP_CNTRL_3, reg);
 
 	reg_write(priv, REG_VIDFORMAT, 0x00);
 	reg_write16(priv, REG_REFPIX_MSB, ref_pix);
@@ -940,13 +930,25 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 		reg_write(priv, REG_ENABLE_SPACE, 0x01);
 	}
 
+	/*
+	 * Always generate sync polarity relative to input sync and
+	 * revert input stage toggled sync at output stage
+	 */
+	reg = TBG_CNTRL_1_DWIN_DIS | TBG_CNTRL_1_TGL_EN;
+	if (adj_mode->flags & DRM_MODE_FLAG_NHSYNC)
+		reg |= TBG_CNTRL_1_H_TGL;
+	if (adj_mode->flags & DRM_MODE_FLAG_NVSYNC)
+		reg |= TBG_CNTRL_1_V_TGL;
+	reg_write(priv, REG_TBG_CNTRL_1, reg);
+
 	/* must be last register set: */
-	reg_clear(priv, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_ONCE);
+	reg_write(priv, REG_TBG_CNTRL_0, 0);
 
 	/* Only setup the info frames if the sink is HDMI */
 	if (priv->is_hdmi_sink) {
 		/* We need to turn HDMI HDCP stuff on to get audio through */
-		reg_clear(priv, REG_TBG_CNTRL_1, TBG_CNTRL_1_DWIN_DIS);
+		reg &= ~TBG_CNTRL_1_DWIN_DIS;
+		reg_write(priv, REG_TBG_CNTRL_1, reg);
 		reg_write(priv, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(1));
 		reg_set(priv, REG_TX33, TX33_HDMI);
 
-- 
1.8.5.3

[PATCH v3 10/24] drm/i2c: tda998x: don't read write-only registers

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 07:58:42PM +0100, Jean-Francois Moine wrote:
> This patch takes care of the write-only registers of the tda998x.
> 
> The value 'MAT_CONTRL_MAT_SC(1)' in the register MAT_CONTRL has been
> set as it is at reset time.
> 
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> ---
> v3
>     - remarks from Russell King
> 	- don't move the sync polarity setting after the setting of the
> 	  register TBG_CNTRL_0 which must be the last setting of the
> 	  init sequence

This is better, except I find that there's an additional change in this
version which wasn't in the original patch 9:

>  	/* must be last register set: */
> -	reg_clear(priv, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_ONCE);
> +	reg_write(priv, REG_TBG_CNTRL_0, 0);

Register changes which have a potential effect shouldn't be part of a
patch which is really only trying to avoid reading from write only
registers.

This could be a potential functional change - and it's probably one
which Rob Clark should at least be made aware of.  As I commented last
time, when you're changing register values in an otherwise innocuous
patch, you should comment about them in the patch description.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH v3 09/24] drm/i2c: tda998x: use HDMI constants

[Jean-Francois Moine]

This patch replaces hard coded values by hdmi constants.

Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 78d324b..978edaf 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -549,12 +549,12 @@ tda998x_write_if(struct tda998x_priv *priv, uint8_t bit, uint16_t addr,
 static void
 tda998x_write_aif(struct tda998x_priv *priv, struct tda998x_encoder_params *p)
 {
-	uint8_t buf[PB(5) + 1];
+	u8 buf[PB(HDMI_AUDIO_INFOFRAME_SIZE) + 1];
 
 	memset(buf, 0, sizeof(buf));
-	buf[HB(0)] = 0x84;
+	buf[HB(0)] = HDMI_INFOFRAME_TYPE_AUDIO;
 	buf[HB(1)] = 0x01;
-	buf[HB(2)] = 10;
+	buf[HB(2)] = HDMI_AUDIO_INFOFRAME_SIZE;
 	buf[PB(1)] = p->audio_frame[1] & 0x07; /* CC */
 	buf[PB(2)] = p->audio_frame[2] & 0x1c; /* SF */
 	buf[PB(4)] = p->audio_frame[4];
@@ -567,12 +567,12 @@ tda998x_write_aif(struct tda998x_priv *priv, struct tda998x_encoder_params *p)
 static void
 tda998x_write_avi(struct tda998x_priv *priv, struct drm_display_mode *mode)
 {
-	uint8_t buf[PB(13) + 1];
+	u8 buf[PB(HDMI_AVI_INFOFRAME_SIZE) + 1];
 
 	memset(buf, 0, sizeof(buf));
-	buf[HB(0)] = 0x82;
+	buf[HB(0)] = HDMI_INFOFRAME_TYPE_AVI;
 	buf[HB(1)] = 0x02;
-	buf[HB(2)] = 13;
+	buf[HB(2)] = HDMI_AVI_INFOFRAME_SIZE;
 	buf[PB(1)] = HDMI_SCAN_MODE_UNDERSCAN;
 	buf[PB(3)] = HDMI_QUANTIZATION_RANGE_FULL << 2;
 	buf[PB(4)] = drm_match_cea_mode(mode);
-- 
1.8.5.3

[PATCH v3 15/24] drm/i2c: tda998x: use irq for connection status and EDID read

[Jean-Francois Moine]

This patch adds the optional treatment of the tda998x IRQ.

The interrupt function is used to know the display connection status
without polling and to speedup reading the EDID.

The interrupt number may be defined either in the DT or at encoder set
config time for non-DT boards.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
v3
    - remarks from Russell King
	- move the setting of the wq_edid_wait flag before asking the
	  device to read the EDID
	  Thanks about this bug fix, but I don't see the other problem:
	  there is no reason for the read_edid_block function to be
	  called more than once...
	- use '0' as 'no irq'
	- remove the useless delay after irq init
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 126 +++++++++++++++++++++++++++++++++-----
 include/drm/i2c/tda998x.h         |   2 +
 2 files changed, 114 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 98142db..013a67c 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -19,6 +19,7 @@
 
 #include <linux/hdmi.h>
 #include <linux/module.h>
+#include <linux/of_irq.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
@@ -44,6 +45,11 @@ struct tda998x_priv {
 	u8 audio_format;
 	u8 audio_frame[6];
 	u32 audio_port;
+
+	int irq;
+	wait_queue_head_t wq_edid;
+	volatile int wq_edid_wait;
+	struct drm_encoder *encoder;
 };
 
 #define to_tda998x_priv(x)  ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv)
@@ -310,11 +316,16 @@ struct tda998x_priv {
 
 /* CEC registers: (not paged)
  */
+#define REG_CEC_INTSTATUS         0xee                /* read */
+# define CEC_INTSTATUS_CEC        (1 << 0)
+# define CEC_INTSTATUS_HDMI       (1 << 1)
 #define REG_CEC_FRO_IM_CLK_CTRL   0xfb                /* read/write */
 # define CEC_FRO_IM_CLK_CTRL_GHOST_DIS (1 << 7)
 # define CEC_FRO_IM_CLK_CTRL_ENA_OTP   (1 << 6)
 # define CEC_FRO_IM_CLK_CTRL_IMCLK_SEL (1 << 1)
 # define CEC_FRO_IM_CLK_CTRL_FRO_DIV   (1 << 0)
+#define REG_CEC_RXSHPDINTENA      0xfc                /* read/write */
+#define REG_CEC_RXSHPDINT         0xfd                /* read */
 #define REG_CEC_RXSHPDLEV         0xfe                /* read */
 # define CEC_RXSHPDLEV_RXSENS     (1 << 0)
 # define CEC_RXSHPDLEV_HPD        (1 << 1)
@@ -528,6 +539,62 @@ tda998x_reset(struct tda998x_priv *priv)
 	reg_write(priv, REG_MUX_VP_VIP_OUT, 0x24);
 }
 
+/*
+ * only 2 interrupts may occur: screen plug/unplug and EDID read
+ */
+static irqreturn_t tda998x_irq_thread(int irq, void *data)
+{
+	struct tda998x_priv *priv = data;
+	u8 sta, cec, lvl, flag0, flag1, flag2;
+
+	if (!priv)
+		return IRQ_HANDLED;
+	sta = cec_read(priv, REG_CEC_INTSTATUS);
+	cec = cec_read(priv, REG_CEC_RXSHPDINT);
+	lvl = cec_read(priv, REG_CEC_RXSHPDLEV);
+	flag0 = reg_read(priv, REG_INT_FLAGS_0);
+	flag1 = reg_read(priv, REG_INT_FLAGS_1);
+	flag2 = reg_read(priv, REG_INT_FLAGS_2);
+	DRM_DEBUG_DRIVER(
+		"tda irq sta %02x cec %02x lvl %02x f0 %02x f1 %02x f2 %02x\n",
+		sta, cec, lvl, flag0, flag1, flag2);
+	if ((flag2 & INT_FLAGS_2_EDID_BLK_RD) && priv->wq_edid_wait) {
+		priv->wq_edid_wait = 0;
+		wake_up(&priv->wq_edid);
+	} else if (cec != 0) {			/* level change */
+		if (priv->encoder && priv->encoder->dev)
+			drm_helper_hpd_irq_event(priv->encoder->dev);
+	}
+	return IRQ_HANDLED;
+}
+
+static void tda_irq_init(struct tda998x_priv *priv)
+{
+	int ret;
+
+	/* init read EDID waitqueue */
+	init_waitqueue_head(&priv->wq_edid);
+
+	/* clear pending interrupts */
+	reg_read(priv, REG_INT_FLAGS_0);
+	reg_read(priv, REG_INT_FLAGS_1);
+	reg_read(priv, REG_INT_FLAGS_2);
+
+	ret = request_threaded_irq(priv->irq, NULL,
+				   tda998x_irq_thread,
+				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+				   "tda998x", priv);
+	if (ret) {
+		dev_err(&priv->hdmi->dev, "failed to request IRQ#%u: %d\n",
+			priv->irq, ret);
+		return;
+	}
+
+	/* enable HPD irq */
+	cec_write(priv, REG_CEC_RXSHPDINTENA,
+			CEC_RXSHPDLEV_HPD | CEC_RXSHPDLEV_RXSENS);
+}
+
 static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes)
 {
 	uint8_t sum = 0;
@@ -720,6 +787,10 @@ tda998x_encoder_set_config(struct drm_encoder *encoder, void *params)
 		priv->audio_port = p->audio_cfg;
 		priv->audio_format = p->audio_format;
 	}
+
+	priv->irq = p->irq;
+	if (p->irq)
+		tda_irq_init(priv);
 }
 
 static void
@@ -990,9 +1061,6 @@ read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk)
 	uint8_t offset, segptr;
 	int ret, i;
 
-	/* enable EDID read irq: */
-	reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
-
 	offset = (blk & 1) ? 128 : 0;
 	segptr = blk / 2;
 
@@ -1002,23 +1070,36 @@ read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk)
 	reg_write(priv, REG_DDC_SEGM, segptr);
 
 	/* enable reading EDID: */
+	priv->wq_edid_wait = 1;
 	reg_write(priv, REG_EDID_CTRL, 0x1);
 
 	/* flag must be cleared by sw: */
 	reg_write(priv, REG_EDID_CTRL, 0x0);
 
 	/* wait for block read to complete: */
-	for (i = 100; i > 0; i--) {
-		ret = reg_read(priv, REG_INT_FLAGS_2);
-		if (ret < 0)
-			return ret;
-		if (ret & INT_FLAGS_2_EDID_BLK_RD)
-			break;
-		msleep(1);
+	if (priv->irq != 0) {
+		i = wait_event_timeout(priv->wq_edid,
+					!priv->wq_edid_wait,
+					msecs_to_jiffies(100));
+		if (i < 0) {
+			dev_err(encoder->dev->dev, "read edid wait err %d\n", i);
+			return i;
+		}
+	} else {
+		for (i = 10; i > 0; i--) {
+			msleep(10);
+			ret = reg_read(priv, REG_INT_FLAGS_2);
+			if (ret < 0)
+				return ret;
+			if (ret & INT_FLAGS_2_EDID_BLK_RD)
+				break;
+		}
 	}
 
-	if (i == 0)
+	if (i == 0) {
+		dev_err(encoder->dev->dev, "read edid timeout\n");
 		return -ETIMEDOUT;
+	}
 
 	ret = reg_read_range(priv, REG_EDID_DATA_0, buf, EDID_LENGTH);
 	if (ret != EDID_LENGTH) {
@@ -1027,8 +1108,6 @@ read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk)
 		return ret;
 	}
 
-	reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
-
 	return 0;
 }
 
@@ -1046,6 +1125,9 @@ do_get_edid(struct drm_encoder *encoder)
 	if (priv->rev == TDA19988)
 		reg_clear(priv, REG_TX4, TX4_PD_RAM);
 
+	/* enable EDID read irq: */
+	reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
+
 	/* base block fetch */
 	if (read_edid_block(encoder, block, 0))
 		goto fail;
@@ -1118,7 +1200,13 @@ static int
 tda998x_encoder_create_resources(struct drm_encoder *encoder,
 				struct drm_connector *connector)
 {
-	DBG("");
+	struct tda998x_priv *priv = to_tda998x_priv(encoder);
+
+	if (priv->irq != 0)
+		connector->polled = DRM_CONNECTOR_POLL_HPD;
+	else
+		connector->polled = DRM_CONNECTOR_POLL_CONNECT |
+			DRM_CONNECTOR_POLL_DISCONNECT;
 	return 0;
 }
 
@@ -1137,6 +1225,8 @@ tda998x_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct tda998x_priv *priv = to_tda998x_priv(encoder);
 	drm_i2c_encoder_destroy(encoder);
+	if (priv->irq != 0)
+		free_irq(priv->irq, priv);
 	if (priv->cec)
 		i2c_unregister_device(priv->cec);
 	kfree(priv);
@@ -1195,6 +1285,7 @@ tda998x_encoder_init(struct i2c_client *client,
 	priv->cec = i2c_new_dummy(client->adapter, 0x34);
 	if (!priv->cec)
 		return -ENODEV;
+	priv->encoder = &encoder_slave->base;
 	priv->dpms = DRM_MODE_DPMS_OFF;
 
 	encoder_slave->slave_priv = priv;
@@ -1259,6 +1350,13 @@ tda998x_encoder_init(struct i2c_client *client,
 		priv->vip_cntrl_2 = video;
 	}
 
+	/* install the optional HDMI connect IRQ */
+	priv->irq = irq_of_parse_and_map(np, 0);
+	if (priv->irq < 0)
+		priv->irq = 0;
+	if (priv->irq != 0)
+		tda_irq_init(priv);
+
 	return 0;
 
 fail:
diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
index d469b07..8dc2982 100644
--- a/include/drm/i2c/tda998x.h
+++ b/include/drm/i2c/tda998x.h
@@ -25,6 +25,8 @@ struct tda998x_encoder_params {
 	} audio_format;
 
 	unsigned audio_sample_rate;
+
+	int irq;
 };
 
 #endif
-- 
1.8.5.3

[PATCH v3 15/24] drm/i2c: tda998x: use irq for connection status and EDID read

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 07:58:43PM +0100, Jean-Francois Moine wrote:
> This patch adds the optional treatment of the tda998x IRQ.
> 
> The interrupt function is used to know the display connection status
> without polling and to speedup reading the EDID.
> 
> The interrupt number may be defined either in the DT or at encoder set
> config time for non-DT boards.
> 
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> ---
> v3
>     - remarks from Russell King
> 	- move the setting of the wq_edid_wait flag before asking the
> 	  device to read the EDID
> 	  Thanks about this bug fix, but I don't see the other problem:
> 	  there is no reason for the read_edid_block function to be
> 	  called more than once...

It will happen whenever DRM asks the connector for the modes.  However,
since the read process is triggered each time, I think this is fine.

> 	- use '0' as 'no irq'
> 	- remove the useless delay after irq init

Some further comments...

> @@ -720,6 +787,10 @@ tda998x_encoder_set_config(struct drm_encoder *encoder, void *params)
>  		priv->audio_port = p->audio_cfg;
>  		priv->audio_format = p->audio_format;
>  	}
> +
> +	priv->irq = p->irq;
> +	if (p->irq)
> +		tda_irq_init(priv);

If we're going to do it this way, this should probably release the IRQ if
there was one before re-claiming it, just in case this function gets called
more than once by some driver using it.

The alternative is, as I said before, to use the infrastructure which is
already there, namely setting the interrupt via struct i2c_client's
irq member.  Yes, that doesn't satisfy Sebastian's comment about using
a GPIO, but there's no sign of GPIO usage in here at the moment anyway.
So we might as well use what's already provided.

That would then avoid the need to deal with IRQ changes etc in
tda998x_encoder_set_config().

In any case, I've tested this patch in both non-IRQ and IRQ modes, and it
appears to work - but I'd like the suggestion above before I provide any
attributations.

Thanks.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH v3 15/24] drm/i2c: tda998x: use irq for connection status and EDID read

[Sebastian Hesselbarth]

On 01/22/14 23:27, Russell King - ARM Linux wrote:
> On Sun, Jan 19, 2014 at 07:58:43PM +0100, Jean-Francois Moine wrote:
>> This patch adds the optional treatment of the tda998x IRQ.
>>
>> The interrupt function is used to know the display connection status
>> without polling and to speedup reading the EDID.
>>
>> The interrupt number may be defined either in the DT or at encoder set
>> config time for non-DT boards.
>>
>> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
>> ---
[...]
>> @@ -720,6 +787,10 @@ tda998x_encoder_set_config(struct drm_encoder *encoder, void *params)
>>   		priv->audio_port = p->audio_cfg;
>>   		priv->audio_format = p->audio_format;
>>   	}
>> +
>> +	priv->irq = p->irq;
>> +	if (p->irq)
>> +		tda_irq_init(priv);
>
> If we're going to do it this way, this should probably release the IRQ if
> there was one before re-claiming it, just in case this function gets called
> more than once by some driver using it.
>
> The alternative is, as I said before, to use the infrastructure which is
> already there, namely setting the interrupt via struct i2c_client's
> irq member.  Yes, that doesn't satisfy Sebastian's comment about using
> a GPIO, but there's no sign of GPIO usage in here at the moment anyway.
> So we might as well use what's already provided.

Russell,

I am fine with using an irq instead of gpio here. I remember you telling
me on a similar patch, that from the gpio you can derive the irq but
not the other way round. Anyway, I also remember reading discussions
about DT gpios vs interrupts, and IIRC the outcome was that passing
interrupts is fine, too.

We usually have both interrupt-controller; and gpio-controller; set on
DT gpio controllers, so let's stick with irq.

And: Thanks for reviewing this again, I am still too busy to keep up
with it.

Sebastian

[PATCH v3 15/24] drm/i2c: tda998x: use irq for connection status and EDID read

[Russell King - ARM Linux]

On Fri, Jan 24, 2014 at 06:29:12PM +0100, Sebastian Hesselbarth wrote:
> On 01/22/14 23:27, Russell King - ARM Linux wrote:
>> On Sun, Jan 19, 2014 at 07:58:43PM +0100, Jean-Francois Moine wrote:
>>> This patch adds the optional treatment of the tda998x IRQ.
>>>
>>> The interrupt function is used to know the display connection status
>>> without polling and to speedup reading the EDID.
>>>
>>> The interrupt number may be defined either in the DT or at encoder set
>>> config time for non-DT boards.
>>>
>>> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
>>> ---
> [...]
>>> @@ -720,6 +787,10 @@ tda998x_encoder_set_config(struct drm_encoder *encoder, void *params)
>>>   		priv->audio_port = p->audio_cfg;
>>>   		priv->audio_format = p->audio_format;
>>>   	}
>>> +
>>> +	priv->irq = p->irq;
>>> +	if (p->irq)
>>> +		tda_irq_init(priv);
>>
>> If we're going to do it this way, this should probably release the IRQ if
>> there was one before re-claiming it, just in case this function gets called
>> more than once by some driver using it.
>>
>> The alternative is, as I said before, to use the infrastructure which is
>> already there, namely setting the interrupt via struct i2c_client's
>> irq member.  Yes, that doesn't satisfy Sebastian's comment about using
>> a GPIO, but there's no sign of GPIO usage in here at the moment anyway.
>> So we might as well use what's already provided.
>
> Russell,
>
> I am fine with using an irq instead of gpio here. I remember you telling
> me on a similar patch, that from the gpio you can derive the irq but
> not the other way round. Anyway, I also remember reading discussions
> about DT gpios vs interrupts, and IIRC the outcome was that passing
> interrupts is fine, too.

Sebastian,

You can derive the irq from a gpio (using gpio_to_irq()), but you can't
derive a gpio from an irq.

It's annoying that i2c_client and the i2c board data do not allow a GPIO
to be specified, but only an IRQ, but that's a separate problem which when
solved will also get solved here.  So, I'm not too worried about the
"should this be an IRQ or should it be a GPIO" question here.

For us on ARM, it's less of a problem because we can just deal with GPIOs
or IRQs at the DT level, and so we don't care what's in i2c_client.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH v3 13/24] drm/i2c: tda998x: fix a NULL pointer dereference

[Jean-Francois Moine]

This patch fixes a NULL pointer dereference when the set_config
function has not been called (priv->params == NULL).

As a side effect, now, the audio sample rate is always 48kHz and the
audio clock is always set for I2S, and never set for S/PDIF.

The audio_format enum is modified so that this format is not null when
audio is configured.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 46 +++++++++++++++++++++++++--------------
 include/drm/i2c/tda998x.h         |  4 ++--
 2 files changed, 32 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index a3ebc08..d3e2bb9 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -28,6 +28,8 @@
 
 #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
 
+#define AUDIO_SAMPLE 48000
+
 struct tda998x_priv {
 	struct i2c_client *cec;
 	struct i2c_client *hdmi;
@@ -38,7 +40,10 @@ struct tda998x_priv {
 	u8 vip_cntrl_0;
 	u8 vip_cntrl_1;
 	u8 vip_cntrl_2;
-	struct tda998x_encoder_params params;
+
+	u8 audio_format;
+	u8 audio_frame[6];
+	u32 audio_port;
 };
 
 #define to_tda998x_priv(x)  ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv)
@@ -547,7 +552,7 @@ tda998x_write_if(struct tda998x_priv *priv, uint8_t bit, uint16_t addr,
 }
 
 static void
-tda998x_write_aif(struct tda998x_priv *priv, struct tda998x_encoder_params *p)
+tda998x_write_aif(struct tda998x_priv *priv)
 {
 	u8 buf[PB(HDMI_AUDIO_INFOFRAME_SIZE) + 1];
 
@@ -555,10 +560,10 @@ tda998x_write_aif(struct tda998x_priv *priv, struct tda998x_encoder_params *p)
 	buf[HB(0)] = HDMI_INFOFRAME_TYPE_AUDIO;
 	buf[HB(1)] = 0x01;
 	buf[HB(2)] = HDMI_AUDIO_INFOFRAME_SIZE;
-	buf[PB(1)] = p->audio_frame[1] & 0x07; /* CC */
-	buf[PB(2)] = p->audio_frame[2] & 0x1c; /* SF */
-	buf[PB(4)] = p->audio_frame[4];
-	buf[PB(5)] = p->audio_frame[5] & 0xf8; /* DM_INH + LSV */
+	buf[PB(1)] = priv->audio_frame[1] & 0x07; /* CC */
+	buf[PB(2)] = priv->audio_frame[2] & 0x1c; /* SF */
+	buf[PB(4)] = priv->audio_frame[4];
+	buf[PB(5)] = priv->audio_frame[5] & 0xf8; /* DM_INH + LSV */
 
 	tda998x_write_if(priv, DIP_IF_FLAGS_IF4, REG_IF4_HB0, buf,
 			 sizeof(buf));
@@ -594,23 +599,23 @@ static void tda998x_audio_mute(struct tda998x_priv *priv, bool on)
 
 static void
 tda998x_configure_audio(struct tda998x_priv *priv,
-		struct drm_display_mode *mode, struct tda998x_encoder_params *p)
+		struct drm_display_mode *mode)
 {
 	uint8_t buf[6], clksel_aip, clksel_fs, ca_i2s, cts_n, adiv;
-	uint32_t n;
+	uint32_t aclk, 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, priv->audio_port);
 
 	/* Set audio input source */
-	switch (p->audio_format) {
+	switch (priv->audio_format) {
 	case AFMT_SPDIF:
 		reg_write(priv, REG_MUX_AP, 0x40);
 		clksel_aip = AIP_CLKSEL_AIP(0);
 		/* FS64SPDIF */
 		clksel_fs = AIP_CLKSEL_FS(2);
 		cts_n = CTS_N_M(3) | CTS_N_K(3);
+		aclk = 0;				/* no clock */
 		ca_i2s = 0;
 		break;
 
@@ -620,6 +625,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 		/* ACLK */
 		clksel_fs = AIP_CLKSEL_FS(0);
 		cts_n = CTS_N_M(3) | CTS_N_K(3);
+		aclk = 1;				/* clock enable */
 		ca_i2s = CA_I2S_CA_I2S(0);
 		break;
 
@@ -634,6 +640,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	/* Enable automatic CTS generation */
 	reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_ACR_MAN);
 	reg_write(priv, REG_CTS_N, cts_n);
+	reg_write(priv, REG_ENA_ACLK, aclk);
 
 	/*
 	 * Audio input somehow depends on HDMI line rate which is
@@ -652,7 +659,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 * AUDIO_SAMPLE / 1000;
 
 	/* Write the CTS and N values */
 	buf[0] = 0x44;
@@ -682,7 +689,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	tda998x_audio_mute(priv, false);
 
 	/* Write the audio information packet */
-	tda998x_write_aif(priv, p);
+	tda998x_write_aif(priv);
 }
 
 /* DRM encoder functions */
@@ -706,7 +713,13 @@ tda998x_encoder_set_config(struct drm_encoder *encoder, void *params)
 			    VIP_CNTRL_2_SWAP_F(p->swap_f) |
 			    (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
 
-	priv->params = *p;
+	memcpy(priv->audio_frame, p->audio_frame,
+			sizeof priv->audio_frame);
+
+	if (p->audio_cfg) {
+		priv->audio_port = p->audio_cfg;
+		priv->audio_format = p->audio_format;
+	}
 }
 
 static void
@@ -954,8 +967,8 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 
 		tda998x_write_avi(priv, adj_mode);
 
-		if (priv->params.audio_cfg)
-			tda998x_configure_audio(priv, adj_mode, &priv->params);
+		if (priv->audio_format)
+			tda998x_configure_audio(priv, adj_mode);
 	}
 }
 
@@ -1173,6 +1186,7 @@ tda998x_encoder_init(struct i2c_client *client,
 	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3);
 	priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1);
 	priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
+	priv->audio_frame[1] = 1;		/* channels - 1 */
 
 	priv->current_page = 0xff;
 	priv->hdmi = client;
diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
index 3e419d9..d469b07 100644
--- a/include/drm/i2c/tda998x.h
+++ b/include/drm/i2c/tda998x.h
@@ -20,8 +20,8 @@ struct tda998x_encoder_params {
 	u8 audio_frame[6];
 
 	enum {
-		AFMT_SPDIF,
-		AFMT_I2S
+		AFMT_I2S = 1,
+		AFMT_SPDIF
 	} audio_format;
 
 	unsigned audio_sample_rate;
-- 
1.8.5.3

[PATCH v3 13/24] drm/i2c: tda998x: fix a NULL pointer dereference

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 07:58:43PM +0100, Jean-Francois Moine wrote:
> This patch fixes a NULL pointer dereference when the set_config
> function has not been called (priv->params == NULL).

No, that's not what this patch is doing.  Maybe you could enlighten me
how priv->params could ever be NULL when that is _not_ a pointer?  That's
completely impossible as it isn't a pointer.

If you tried "priv->params = NULL" the C compiler would barf on it.

I suspect you've misunderstood the code, and this change isn't actually
necessary.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH v3 14/24] drm/i2c: tda998x: add DT support

[Jean-Francois Moine]

This patch adds DT support to the tda998x.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
v3
    - remarks from Russell King
	- change the AUDIO_SAMPLE value to 48000 instead of 48
	- don't change the variable type of 'dpms'
	- don't set the clock when S/PDIF input
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index d3e2bb9..98142db 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1177,6 +1177,8 @@ tda998x_encoder_init(struct i2c_client *client,
 		    struct drm_encoder_slave *encoder_slave)
 {
 	struct tda998x_priv *priv;
+	struct device_node *np = client->dev.of_node;
+	u32 video;
 	int ret;
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
@@ -1246,6 +1248,17 @@ tda998x_encoder_init(struct i2c_client *client,
 	cec_write(priv, REG_CEC_FRO_IM_CLK_CTRL,
 			CEC_FRO_IM_CLK_CTRL_GHOST_DIS | CEC_FRO_IM_CLK_CTRL_IMCLK_SEL);
 
+	if (!np)
+		return 0;		/* non-DT */
+
+	/* get the optional video properties */
+	ret = of_property_read_u32(np, "video-ports", &video);
+	if (ret == 0) {
+		priv->vip_cntrl_0 = video >> 16;
+		priv->vip_cntrl_1 = video >> 8;
+		priv->vip_cntrl_2 = video;
+	}
+
 	return 0;
 
 fail:
@@ -1260,6 +1273,14 @@ fail:
 	return -ENXIO;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id tda998x_dt_ids[] = {
+	{ .compatible = "nxp,tda998x", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, tda998x_dt_ids);
+#endif
+
 static struct i2c_device_id tda998x_ids[] = {
 	{ "tda998x", 0 },
 	{ }
@@ -1272,6 +1293,7 @@ static struct drm_i2c_encoder_driver tda998x_driver = {
 		.remove = tda998x_remove,
 		.driver = {
 			.name = "tda998x",
+			.of_match_table = of_match_ptr(tda998x_dt_ids),
 		},
 		.id_table = tda998x_ids,
 	},
-- 
1.8.5.3

[PATCH v3 14/24] drm/i2c: tda998x: add DT support

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 07:58:43PM +0100, Jean-Francois Moine wrote:
> This patch adds DT support to the tda998x.
> 
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>

Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH v3 16/24] drm/i2c: tda998x: add DT documentation

[Jean-Francois Moine]

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 .../devicetree/bindings/drm/i2c/tda998x.txt        | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/drm/i2c/tda998x.txt

diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
new file mode 100644
index 0000000..72da71d
--- /dev/null
+++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
@@ -0,0 +1,24 @@
+Device-Tree bindings for the NXP TDA998x HDMI transmitter
+
+Required properties;
+  - compatible: must be "nxp,tda998x"
+
+Optional properties:
+  - interrupts: interrupt number for HDMI exchanges - default: by polling
+
+  - pinctrl-0: pin control group to be used for this controller (IRQ).
+
+  - pinctrl-names: must contain a "default" entry.
+
+  - video-ports: 24 bits value - default: <0x230145>
+
+Example:
+
+	tda998x: hdmi-encoder {
+		compatible = "nxp,tda998x";
+		reg = <0x70>;
+		interrupt-parent = <&gpio0>;
+		interrupts = <27 2>;		/* falling edge */
+		pinctrl-0 = <&pmx_camera>;
+		pinctrl-names = "default";
+	};
-- 
1.8.5.3

[PATCH v3 16/24] drm/i2c: tda998x: add DT documentation

[Olof Johansson]

Hi,

On Sun, Jan 19, 2014 at 10:58 AM, Jean-Francois Moine <moinejf@free.fr> wrote:
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> ---
>  .../devicetree/bindings/drm/i2c/tda998x.txt        | 24 ++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/drm/i2c/tda998x.txt

Please cc bindings for review to devicetree at vger.kernel.org (cc:d here now)

> diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> new file mode 100644
> index 0000000..72da71d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> @@ -0,0 +1,24 @@
> +Device-Tree bindings for the NXP TDA998x HDMI transmitter
> +
> +Required properties;
> +  - compatible: must be "nxp,tda998x"
> +
> +Optional properties:
> +  - interrupts: interrupt number for HDMI exchanges - default: by polling

What are HDMI exchanges, and how do they differ from other interrupts?

> +
> +  - pinctrl-0: pin control group to be used for this controller (IRQ).
> +
> +  - pinctrl-names: must contain a "default" entry.
> +
> +  - video-ports: 24 bits value - default: <0x230145>

What is this?


-Olof

[PATCH v3 16/24] drm/i2c: tda998x: add DT documentation

[Jean-Francois Moine]

On Sun, 19 Jan 2014 20:06:09 -0800
Olof Johansson <olof@lixom.net> wrote:

> Hi,
> 
> On Sun, Jan 19, 2014 at 10:58 AM, Jean-Francois Moine <moinejf@free.fr> wrote:
> > Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> > ---
> >  .../devicetree/bindings/drm/i2c/tda998x.txt        | 24 ++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> 
> Please cc bindings for review to devicetree at vger.kernel.org (cc:d here now)
> 
> > diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> > new file mode 100644
> > index 0000000..72da71d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> > @@ -0,0 +1,24 @@
> > +Device-Tree bindings for the NXP TDA998x HDMI transmitter
> > +
> > +Required properties;
> > +  - compatible: must be "nxp,tda998x"
> > +
> > +Optional properties:
> > +  - interrupts: interrupt number for HDMI exchanges - default: by polling
> 
> What are HDMI exchanges, and how do they differ from other interrupts?

The used HDMI interrupt events are screen plug/unplug and EDID read.
There are also CEC read/write which are not yet implemented in the
tda998x driver.

There is no difference from normal interrupts, except that the events
may be get by polling, so, the interrupt number is optional.

> > +
> > +  - pinctrl-0: pin control group to be used for this controller (IRQ).
> > +
> > +  - pinctrl-names: must contain a "default" entry.
> > +
> > +  - video-ports: 24 bits value - default: <0x230145>
> 
> What is this?

The video-ports value defines how the video controller is connected to
the tda998x chip. Each 4 bits value tells from which input pins comes
the video data and if there is any bit inversion. Each byte of this
video-ports is used to load the VIP_CNTRL_{0,1,2} registers. These ones
are described in the TDA9983B documentation which is the closer
available document about the TDA998x family.

The default value is the one defined for TI boards.
A known other value is <0x234501> which is used for Russell's Armada
DRM driver in the Cubox (Marvell A510), but this driver has no DT
support.

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH v3 16/24] drm/i2c: tda998x: add DT documentation

[Olof Johansson]

On Mon, Jan 20, 2014 at 1:54 AM, Jean-Francois Moine <moinejf@free.fr> wrote:
> On Sun, 19 Jan 2014 20:06:09 -0800
> Olof Johansson <olof@lixom.net> wrote:
>
>> Hi,
>>
>> On Sun, Jan 19, 2014 at 10:58 AM, Jean-Francois Moine <moinejf@free.fr> wrote:
>> > Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
>> > ---
>> >  .../devicetree/bindings/drm/i2c/tda998x.txt        | 24 ++++++++++++++++++++++
>> >  1 file changed, 24 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/drm/i2c/tda998x.txt
>>
>> Please cc bindings for review to devicetree at vger.kernel.org (cc:d here now)
>>
>> > diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
>> > new file mode 100644
>> > index 0000000..72da71d
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
>> > @@ -0,0 +1,24 @@
>> > +Device-Tree bindings for the NXP TDA998x HDMI transmitter
>> > +
>> > +Required properties;
>> > +  - compatible: must be "nxp,tda998x"
>> > +
>> > +Optional properties:
>> > +  - interrupts: interrupt number for HDMI exchanges - default: by polling
>>
>> What are HDMI exchanges, and how do they differ from other interrupts?
>
> The used HDMI interrupt events are screen plug/unplug and EDID read.
> There are also CEC read/write which are not yet implemented in the
> tda998x driver.
>
> There is no difference from normal interrupts, except that the events
> may be get by polling, so, the interrupt number is optional.]

Ok, then it looks like the property description is a little confusing.
I'd remove the mentioning of HDMI exchanges from it.

>> > +
>> > +  - pinctrl-0: pin control group to be used for this controller (IRQ).
>> > +
>> > +  - pinctrl-names: must contain a "default" entry.
>> > +
>> > +  - video-ports: 24 bits value - default: <0x230145>
>>
>> What is this?
>
> The video-ports value defines how the video controller is connected to
> the tda998x chip. Each 4 bits value tells from which input pins comes
> the video data and if there is any bit inversion. Each byte of this
> video-ports is used to load the VIP_CNTRL_{0,1,2} registers. These ones
> are described in the TDA9983B documentation which is the closer
> available document about the TDA998x family.
>
> The default value is the one defined for TI boards.
> A known other value is <0x234501> which is used for Russell's Armada
> DRM driver in the Cubox (Marvell A510), but this driver has no DT
> support.

Ok, this is a classic case where the binding should describe how
things are configured/wired up instead of hardcoding a register value
like this. From the data sheet there seems to be a _lot_ of settings,
so selecting the needed subset for now might be acceptable (and go
with defaults on the rest).


-Olof

[PATCH v3 16/24] drm/i2c: tda998x: add DT documentation

[Russell King - ARM Linux]

On Tue, Jan 21, 2014 at 10:17:56AM -0800, Olof Johansson wrote:
> Ok, this is a classic case where the binding should describe how
> things are configured/wired up instead of hardcoding a register value
> like this. From the data sheet there seems to be a _lot_ of settings,
> so selecting the needed subset for now might be acceptable (and go
> with defaults on the rest).

The issue is not that simple, because the register is kind of backwards.

Rather than describing what is on each nibble of the input bus, you
specify how each nibble of the internal bus maps to the external world.
The internal bus is mostly fixed as to which nibbles are red, green and
blue (there is internal support for swapping red/blue on the internal
bus, but we don't use that - it just adds extra unnecessary headaches
to the mapping.)

The way I ended up describing it in platform data was via the individual
nibble settings for the register:

        .swap_a = 2,
        .swap_b = 3,
        .swap_c = 4,
        .swap_d = 5,
        .swap_e = 0,
        .swap_f = 1,

which gives you a way to refer back to the data sheet.  There's also the
control whether the nibble is bit-big-endian or bit-little-endian.

So basically, swap_a controls which nibble of the input bus is mapped to
the internal bus bits 23:20, and mirror_a whether it's bit 0123 -> 0123
or 0123 -> 3210.

This means it's possible to tell the chip that each nibble of the interal
bus comes from the same four pins!

If we want to describe what each nibble set on the input pins does, it's
going to make for some complex code to convert that from "forward" notation
(which we'll have to invent) to the "reverse" notation that the chip needs.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH v3 17/24] drm/i2c: tda998x: get a better status of the connection

[Jean-Francois Moine]

This patch refines the connection status testing both bits RXSENS and
HPD of the CEC register giving the connection level.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 013a67c..ea7d1b4 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1050,7 +1050,9 @@ tda998x_encoder_detect(struct drm_encoder *encoder,
 	struct tda998x_priv *priv = to_tda998x_priv(encoder);
 	uint8_t val = cec_read(priv, REG_CEC_RXSHPDLEV);
 
-	return (val & CEC_RXSHPDLEV_HPD) ? connector_status_connected :
+	return (val & (CEC_RXSHPDLEV_RXSENS | CEC_RXSHPDLEV_HPD)) ==
+				(CEC_RXSHPDLEV_RXSENS | CEC_RXSHPDLEV_HPD) ?
+			connector_status_connected :
 			connector_status_disconnected;
 }
 
-- 
1.8.5.3

[PATCH v3 17/24] drm/i2c: tda998x: get a better status of the connection

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 07:58:44PM +0100, Jean-Francois Moine wrote:
> This patch refines the connection status testing both bits RXSENS and
> HPD of the CEC register giving the connection level.

Can you explain why this is necessary?

I believe this is not necessary for the following reason:

HDMI cables are required to mate in a certain order:
- First to mate is the shell.
- Second are all pins with the exception of the +5V line.
- Third is the +5V line.

Sinks are not permitted to assert the HPD line until the EDID memory is
ready to be read, and +5V is indicated from the source.

Hence, HPD will not be asserted until the connector is fully mated, and
there are receivers available on the line.

This doesn't indicate whether the sink is powered and able to display
a picture - it merely means that the sink is present and valid EDID is
available to be read.  That's all that we really need here.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH v3 19/24] drm/i2c: tda998x: use global constants

[Jean-Francois Moine]

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index ce832f0..324dc13 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -132,6 +132,8 @@ struct tda998x_priv {
 # define VIP_CNTRL_5_CKCASE       (1 << 0)
 # define VIP_CNTRL_5_SP_CNT(x)    (((x) & 3) << 1)
 #define REG_MUX_AP                REG(0x00, 0x26)     /* read/write */
+# define MUX_AP_SELECT_I2S	  0x64
+# define MUX_AP_SELECT_SPDIF	  0x40
 #define REG_MUX_VP_VIP_OUT        REG(0x00, 0x27)     /* read/write */
 #define REG_MAT_CONTRL            REG(0x00, 0x80)     /* write */
 # define MAT_CONTRL_MAT_SC(x)     (((x) & 3) << 0)
@@ -209,10 +211,11 @@ struct tda998x_priv {
 #define REG_I2S_FORMAT            REG(0x00, 0xfc)     /* read/write */
 # define I2S_FORMAT(x)            (((x) & 3) << 0)
 #define REG_AIP_CLKSEL            REG(0x00, 0xfd)     /* write */
-# define AIP_CLKSEL_FS(x)         (((x) & 3) << 0)
-# define AIP_CLKSEL_CLK_POL(x)    (((x) & 1) << 2)
-# define AIP_CLKSEL_AIP(x)        (((x) & 7) << 3)
-
+# define AIP_CLKSEL_AIP_SPDIF     (0 << 3)
+# define AIP_CLKSEL_AIP_I2S       (1 << 3)
+# define AIP_CLKSEL_FS_ACLK       (0 << 0)
+# define AIP_CLKSEL_FS_MCLK       (1 << 0)
+# define AIP_CLKSEL_FS_FS64SPDIF  (2 << 0)
 
 /* Page 02h: PLL settings */
 #define REG_PLL_SERIAL_1          REG(0x02, 0x00)     /* read/write */
@@ -677,20 +680,18 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	/* Set audio input source */
 	switch (priv->audio_format) {
 	case AFMT_SPDIF:
-		reg_write(priv, REG_MUX_AP, 0x40);
-		clksel_aip = AIP_CLKSEL_AIP(0);
-		/* FS64SPDIF */
-		clksel_fs = AIP_CLKSEL_FS(2);
+		reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_SPDIF);
+		clksel_aip = AIP_CLKSEL_AIP_SPDIF;
+		clksel_fs = AIP_CLKSEL_FS_FS64SPDIF;
 		cts_n = CTS_N_M(3) | CTS_N_K(3);
 		aclk = 0;				/* no clock */
 		ca_i2s = 0;
 		break;
 
 	case AFMT_I2S:
-		reg_write(priv, REG_MUX_AP, 0x64);
-		clksel_aip = AIP_CLKSEL_AIP(1);
-		/* ACLK */
-		clksel_fs = AIP_CLKSEL_FS(0);
+		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);
 		aclk = 1;				/* clock enable */
 		ca_i2s = CA_I2S_CA_I2S(0);
-- 
1.8.5.3

[PATCH v3 19/24] drm/i2c: tda998x: use global constants

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 07:58:45PM +0100, Jean-Francois Moine wrote:
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>

Might be worth also saying "and tidy up AIP_CLKSEL" since arguably they
were already using global constants.  In any case, no one likes single
line patch descriptions.  Always try to write something more about the
patch.

I've verified this via inspection, and tested it.

Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH v3 18/24] drm/i2c: tda998x: fix the ENABLE_SPACE register

[Jean-Francois Moine]

This patch fixes the ENABLE_SPACE register, the value of which was
inverted.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index ea7d1b4..ce832f0 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -220,7 +220,7 @@ struct tda998x_priv {
 # define PLL_SERIAL_1_SRL_IZ(x)   (((x) & 3) << 1)
 # define PLL_SERIAL_1_SRL_MAN_IZ  (1 << 6)
 #define REG_PLL_SERIAL_2          REG(0x02, 0x01)     /* read/write */
-# define PLL_SERIAL_2_SRL_NOSC(x) (((x) & 3) << 0)
+# define PLL_SERIAL_2_SRL_NOSC(x) ((x) << 0)
 # define PLL_SERIAL_2_SRL_PR(x)   (((x) & 0xf) << 4)
 #define REG_PLL_SERIAL_3          REG(0x02, 0x02)     /* read/write */
 # define PLL_SERIAL_3_SRL_CCIR    (1 << 0)
@@ -932,6 +932,11 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	}
 
 	div = 148500 / adj_mode->clock;
+	if (div != 0) {
+		div--;
+		if (div > 3)
+			div = 3;
+	}
 
 	/* mute the audio FIFO: */
 	reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
@@ -1011,7 +1016,7 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 
 	if (priv->rev == TDA19988) {
 		/* let incoming pixels fill the active space (if any) */
-		reg_write(priv, REG_ENABLE_SPACE, 0x01);
+		reg_write(priv, REG_ENABLE_SPACE, 0x00);
 	}
 
 	/*
-- 
1.8.5.3

[PATCH v3 18/24] drm/i2c: tda998x: fix the ENABLE_SPACE register

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 07:58:45PM +0100, Jean-Francois Moine wrote:
> This patch fixes the ENABLE_SPACE register, the value of which was
> inverted.
> 
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>

I've researched the change to PLL_SERIAL_2, and this is correct.  I think
your change to REG_ENABLE_SPACE is also correct.  This looks like a bug
fix to me, so maybe consider having it applied to previous kernel
versions as well?

Also... please try putting yourself in the position of a reviewer of your
own patches - read the patch and then read the description, and ask whether
the description is adequate for the patch... the description talks just
about the ENABLE_SPACE register.  But what about the PLL_SERIAL_2 register
changes?  They're *totally* not described.

Hence...

Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>

and no acked-by.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH v3 20/24] drm/i2c: tda998x: remove the unused variable ca_i2s

[Jean-Francois Moine]

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 324dc13..a2ad2ac 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -671,7 +671,7 @@ static void
 tda998x_configure_audio(struct tda998x_priv *priv,
 		struct drm_display_mode *mode)
 {
-	uint8_t buf[6], clksel_aip, clksel_fs, ca_i2s, cts_n, adiv;
+	uint8_t buf[6], clksel_aip, clksel_fs, cts_n, adiv;
 	uint32_t aclk, n;
 
 	/* Enable audio ports */
@@ -685,7 +685,6 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 		clksel_fs = AIP_CLKSEL_FS_FS64SPDIF;
 		cts_n = CTS_N_M(3) | CTS_N_K(3);
 		aclk = 0;				/* no clock */
-		ca_i2s = 0;
 		break;
 
 	case AFMT_I2S:
@@ -694,7 +693,6 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 		clksel_fs = AIP_CLKSEL_FS_ACLK;
 		cts_n = CTS_N_M(3) | CTS_N_K(3);
 		aclk = 1;				/* clock enable */
-		ca_i2s = CA_I2S_CA_I2S(0);
 		break;
 
 	default:
-- 
1.8.5.3

[PATCH v3 20/24] drm/i2c: tda998x: remove the unused variable ca_i2s

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 07:58:46PM +0100, Jean-Francois Moine wrote:
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>

"ca_i2s is only ever written to, but never read, so let's get rid of it."

Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH v3 21/24] drm/i2c: tda998x: add the active aspect in HDMI AVI frame

[Jean-Francois Moine]

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index a2ad2ac..05713f7 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -649,6 +649,7 @@ tda998x_write_avi(struct tda998x_priv *priv, struct drm_display_mode *mode)
 	buf[HB(1)] = 0x02;
 	buf[HB(2)] = HDMI_AVI_INFOFRAME_SIZE;
 	buf[PB(1)] = HDMI_SCAN_MODE_UNDERSCAN;
+	buf[PB(2)] = HDMI_ACTIVE_ASPECT_PICTURE;
 	buf[PB(3)] = HDMI_QUANTIZATION_RANGE_FULL << 2;
 	buf[PB(4)] = drm_match_cea_mode(mode);
 
-- 
1.8.5.3

[PATCH v3 21/24] drm/i2c: tda998x: add the active aspect in HDMI AVI frame

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 07:58:46PM +0100, Jean-Francois Moine wrote:
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>

"The picture aspect setting was zero, which is reserved.  A setting of
Same As Picture makes more sense."

Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH v3 23/24] drm/i2c: tda998x: code optimization

[Jean-Francois Moine]

This patch reduces the number of I2C exchanges by setting many bits in
one write and removing a useless write.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 2fcc432..0716a75 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -702,10 +702,8 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	}
 
 	reg_write(priv, REG_AIP_CLKSEL, clksel_aip);
-	reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_LAYOUT);
-
-	/* Enable automatic CTS generation */
-	reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_ACR_MAN);
+	reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_LAYOUT |
+					AIP_CNTRL_0_ACR_MAN);	/* auto CTS */
 	reg_write(priv, REG_CTS_N, cts_n);
 	reg_write(priv, REG_ENA_ACLK, aclk);
 
@@ -953,10 +951,10 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	reg_write(priv, REG_VIP_CNTRL_5, VIP_CNTRL_5_SP_CNT(0));
 	reg_write(priv, REG_VIP_CNTRL_4, VIP_CNTRL_4_BLANKIT(0) |
 			VIP_CNTRL_4_BLC(0));
-	reg_clear(priv, REG_PLL_SERIAL_3, PLL_SERIAL_3_SRL_CCIR);
 
 	reg_clear(priv, REG_PLL_SERIAL_1, PLL_SERIAL_1_SRL_MAN_IZ);
-	reg_clear(priv, REG_PLL_SERIAL_3, PLL_SERIAL_3_SRL_DE);
+	reg_clear(priv, REG_PLL_SERIAL_3, PLL_SERIAL_3_SRL_CCIR |
+					  PLL_SERIAL_3_SRL_DE);
 	reg_write(priv, REG_SERIALIZER, 0);
 	reg_write(priv, REG_HVF_CNTRL_1, HVF_CNTRL_1_VQR(0));
 
@@ -976,8 +974,6 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	/* set BIAS tmds value: */
 	reg_write(priv, REG_ANA_GENERAL, 0x09);
 
-	reg_write(priv, REG_TBG_CNTRL_0, 0);
-
 	/*
 	 * Sync on rising HSYNC/VSYNC
 	 */
-- 
1.8.5.3

[PATCH v3 22/24] drm/i2c: tda998x: change the frequence in the audio channel

[Jean-Francois Moine]

This patch sets the frequence as 'not indicated' instead of '48kHz'
and adds some comments.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 05713f7..2fcc432 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -745,10 +745,11 @@ 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] = 0x04;
+	buf[0] = (1 << 2);			/* copyright unprotected */
 	buf[1] = 0x00;
-	buf[2] = 0x00;
-	buf[3] = 0xf1;
+	buf[2] = 1;				/* freq not indicated */
+	buf[3] = (0 << 4) |			/* orig freq = not indicated */
+			1;			/* max word length 24 bits */
 	reg_write_range(priv, REG_CH_STAT_B(0), buf, 4);
 
 	tda998x_audio_mute(priv, true);
-- 
1.8.5.3

[PATCH v3 22/24] drm/i2c: tda998x: change the frequence in the audio channel

[Russell King - ARM Linux]

On Sun, Jan 19, 2014 at 07:58:47PM +0100, Jean-Francois Moine wrote:
> This patch sets the frequence as 'not indicated' instead of '48kHz'
> and adds some comments.
> 
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>

Good catch that byte 2 doesn't exist in this set.

sound/asounddef.h has definitions for these:

IEC958_AES0_CON_NOT_COPYRIGHT
IEC958_AES3_CON_FS_NOTID
IEC958_AES4_CON_MAX_WORDLEN_24
IEC958_AES4_CON_ORIGFS_NOTID

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH v3 24/24] drm/i2c: tda998x: adjust the audio clock divider for S/PDIF

[Jean-Francois Moine]

The S/PDIF input asks for a greater audio clock divider.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 0716a75..06c484d 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -707,6 +707,10 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	reg_write(priv, REG_CTS_N, cts_n);
 	reg_write(priv, REG_ENA_ACLK, aclk);
 
+	/* S/PDIF asks for a large divider */
+	if (priv->audio_format == AFMT_SPDIF)
+		adiv = AUDIO_DIV_SERCLK_32;
+
 	/*
 	 * Audio input somehow depends on HDMI line rate which is
 	 * related to pixclk. Testing showed that modes with pixclk
@@ -714,7 +718,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	 * There is no detailed info in the datasheet, so we just
 	 * assume 100MHz requires larger divider.
 	 */
-	if (mode->clock > 100000)
+	else if (mode->clock > 100000)
 		adiv = AUDIO_DIV_SERCLK_16;
 	else
 		adiv = AUDIO_DIV_SERCLK_8;
-- 
1.8.5.3