[PATCH v2 20/28] drm/i2c: tda998x: move the TBG_CNTRL_0 register setting

According to the comment, the TBG_CNTRL_0 register must be set at the
end of the mode change sequence.

Signed-off-by: Jean-Francois Moine <redacted>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 7dbbc6b..864b9f5 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c

@@ -1073,9 +1073,6 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 		}
 	}
 
-	/* must be last register set: */
-	reg_clear(priv, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_ONCE);
-
 	/*
 	 * Always generate sync polarity relative to input sync and
 	 * revert input stage toggled sync at output stage

@@ -1100,6 +1097,9 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 		if (priv->audio_type)
 			tda998x_configure_audio(priv, mode);
 	}
+
+	/* must be last register set: */
+	reg_write(priv, REG_TBG_CNTRL_0, 0);
 }
 
 static enum drm_connector_status

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

On Thu, Jan 09, 2014 at 12:06:07PM +0100, Jean-Francois Moine wrote:

According to the comment, the TBG_CNTRL_0 register must be set at the
end of the mode change sequence.

So you believe comments without understanding the history, and you move
code around due to those.

No, this is again wrong.  That write to REG_TBG_CNTRL_0 in the sequence
writing the video information to the chip.  This doesn't encompass the
HDMI/DVI mode setting nor the audio configuration - the audio configuration
can change independently of the video setting, and does not require this
register to be written.

This also brings up a bug in one of your previous patches which I now
must go back and comment upon.

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

On Sat, 11 Jan 2014 18:36:48 +0000
Russell King - ARM Linux [off-list ref] wrote:

On Thu, Jan 09, 2014 at 12:06:07PM +0100, Jean-Francois Moine wrote:

quoted

According to the comment, the TBG_CNTRL_0 register must be set at the
end of the mode change sequence.

So you believe comments without understanding the history, and you move
code around due to those.

No, this is again wrong.  That write to REG_TBG_CNTRL_0 in the sequence
writing the video information to the chip.  This doesn't encompass the
HDMI/DVI mode setting nor the audio configuration - the audio configuration
can change independently of the video setting, and does not require this
register to be written.

This also brings up a bug in one of your previous patches which I now
must go back and comment upon.

Well, I have not the full spec of the TDA998x's, and I don't know what
is important or not. I was hoping that Rob had a better knowledge than I.

So, in my patch 9, I was writing the REG_TBG_CNTRL_1 after writing
REG_TBG_CNTRL_0, and you refused it. Here, I write REG_TBG_CNTRL_0
after the write of REG_TBG_CNTRL_1 in the HDMI sequence, and you still
don't agree.

What is the right way?

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

On Sun, Jan 12, 2014 at 01:23:21PM +0100, Jean-Francois Moine wrote:

On Sat, 11 Jan 2014 18:36:48 +0000
Russell King - ARM Linux [off-list ref] wrote:

quoted

On Thu, Jan 09, 2014 at 12:06:07PM +0100, Jean-Francois Moine wrote:

quoted

According to the comment, the TBG_CNTRL_0 register must be set at the
end of the mode change sequence.

So you believe comments without understanding the history, and you move
code around due to those.

No, this is again wrong.  That write to REG_TBG_CNTRL_0 in the sequence
writing the video information to the chip.  This doesn't encompass the
HDMI/DVI mode setting nor the audio configuration - the audio configuration
can change independently of the video setting, and does not require this
register to be written.

This also brings up a bug in one of your previous patches which I now
must go back and comment upon.

Well, I have not the full spec of the TDA998x's, and I don't know what
is important or not. I was hoping that Rob had a better knowledge than I.

So, in my patch 9, I was writing the REG_TBG_CNTRL_1 after writing
REG_TBG_CNTRL_0, and you refused it. Here, I write REG_TBG_CNTRL_0
after the write of REG_TBG_CNTRL_1 in the HDMI sequence, and you still
don't agree.

What is the right way?

No, both NAKS are for the exact same issue.

Patch 9 inserted the write to REG_TBG_CNTRL_1 after REG_TBG_CNTRL_0.
Then in this patch you move REG_TBG_CNTRL_0 after all writes.

Had you appropriately placed the write to REG_TBG_CNTRL_1 in patch 9
in the first place, _this_ patch (patch 20) would not be required to
then move REG_TBG_CNTRL_0 after it.  So, fixing patch 9 removes the
need for patch 20.

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

On Sun, 12 Jan 2014 12:31:59 +0000
Russell King - ARM Linux [off-list ref] wrote:

quoted

So, in my patch 9, I was writing the REG_TBG_CNTRL_1 after writing
REG_TBG_CNTRL_0, and you refused it. Here, I write REG_TBG_CNTRL_0
after the write of REG_TBG_CNTRL_1 in the HDMI sequence, and you still
don't agree.

What is the right way?  

No, both NAKS are for the exact same issue.

Patch 9 inserted the write to REG_TBG_CNTRL_1 after REG_TBG_CNTRL_0.
Then in this patch you move REG_TBG_CNTRL_0 after all writes.

Had you appropriately placed the write to REG_TBG_CNTRL_1 in patch 9
in the first place, _this_ patch (patch 20) would not be required to
then move REG_TBG_CNTRL_0 after it.  So, fixing patch 9 removes the
need for patch 20.

Fixing the patch 9 gives:

	/*
	 * 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);

	/* 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 &= ~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);

		tda998x_write_avi(priv, adj_mode);

		if (priv->params.audio_cfg)
			tda998x_configure_audio(priv, adj_mode, &priv->params);
	}

and REG_TBG_CNTRL_1 is set in the HDMI branch (with REG_ENC_CNTRL and
REG_TX33).

Is this OK?

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

On Sun, Jan 12, 2014 at 02:20:00PM +0100, Jean-Francois Moine wrote:

On Sun, 12 Jan 2014 12:31:59 +0000
Russell King - ARM Linux [off-list ref] wrote:

quoted

quoted

So, in my patch 9, I was writing the REG_TBG_CNTRL_1 after writing
REG_TBG_CNTRL_0, and you refused it. Here, I write REG_TBG_CNTRL_0
after the write of REG_TBG_CNTRL_1 in the HDMI sequence, and you still
don't agree.

What is the right way?  

No, both NAKS are for the exact same issue.

Patch 9 inserted the write to REG_TBG_CNTRL_1 after REG_TBG_CNTRL_0.
Then in this patch you move REG_TBG_CNTRL_0 after all writes.

Had you appropriately placed the write to REG_TBG_CNTRL_1 in patch 9
in the first place, _this_ patch (patch 20) would not be required to
then move REG_TBG_CNTRL_0 after it.  So, fixing patch 9 removes the
need for patch 20.

Fixing the patch 9 gives:

	/*
	 * 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);

	/* 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 &= ~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);

		tda998x_write_avi(priv, adj_mode);

		if (priv->params.audio_cfg)
			tda998x_configure_audio(priv, adj_mode, &priv->params);
	}

and REG_TBG_CNTRL_1 is set in the HDMI branch (with REG_ENC_CNTRL and
REG_TX33).

Is this OK?

I would find that acceptable to ack it as a replacement patch 9, 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".