* [PATCH v2 20/28] drm/i2c: tda998x: move the TBG_CNTRL_0 register setting @ 2014-01-09 11:06 Jean-Francois Moine 2014-01-11 18:36 ` Russell King - ARM Linux 0 siblings, 1 reply; 6+ messages in thread From: Jean-Francois Moine @ 2014-01-09 11:06 UTC (permalink / raw) To: dri-devel; +Cc: Dave Airlie, Rob Clark, linux-arm-kernel, linux-kernel 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 <moinejf@free.fr> --- 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/ ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 20/28] drm/i2c: tda998x: move the TBG_CNTRL_0 register setting 2014-01-09 11:06 [PATCH v2 20/28] drm/i2c: tda998x: move the TBG_CNTRL_0 register setting Jean-Francois Moine @ 2014-01-11 18:36 ` Russell King - ARM Linux 2014-01-12 12:23 ` Jean-Francois Moine 0 siblings, 1 reply; 6+ messages in thread From: Russell King - ARM Linux @ 2014-01-11 18:36 UTC (permalink / raw) To: Jean-Francois Moine Cc: dri-devel, Rob Clark, Dave Airlie, linux-kernel, linux-arm-kernel 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". ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 20/28] drm/i2c: tda998x: move the TBG_CNTRL_0 register setting 2014-01-11 18:36 ` Russell King - ARM Linux @ 2014-01-12 12:23 ` Jean-Francois Moine 2014-01-12 12:31 ` Russell King - ARM Linux 0 siblings, 1 reply; 6+ messages in thread From: Jean-Francois Moine @ 2014-01-12 12:23 UTC (permalink / raw) To: Russell King - ARM Linux Cc: dri-devel, Rob Clark, Dave Airlie, linux-kernel, linux-arm-kernel On Sat, 11 Jan 2014 18:36:48 +0000 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > 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. 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/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 20/28] drm/i2c: tda998x: move the TBG_CNTRL_0 register setting 2014-01-12 12:23 ` Jean-Francois Moine @ 2014-01-12 12:31 ` Russell King - ARM Linux 2014-01-12 13:20 ` Jean-Francois Moine 0 siblings, 1 reply; 6+ messages in thread From: Russell King - ARM Linux @ 2014-01-12 12:31 UTC (permalink / raw) To: Jean-Francois Moine Cc: dri-devel, Rob Clark, Dave Airlie, linux-kernel, linux-arm-kernel 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 <linux@arm.linux.org.uk> wrote: > > > 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. > > 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". ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 20/28] drm/i2c: tda998x: move the TBG_CNTRL_0 register setting 2014-01-12 12:31 ` Russell King - ARM Linux @ 2014-01-12 13:20 ` Jean-Francois Moine 2014-01-12 13:38 ` Russell King - ARM Linux 0 siblings, 1 reply; 6+ messages in thread From: Jean-Francois Moine @ 2014-01-12 13:20 UTC (permalink / raw) To: Russell King - ARM Linux Cc: dri-devel, Rob Clark, Dave Airlie, linux-kernel, linux-arm-kernel On Sun, 12 Jan 2014 12:31:59 +0000 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > > 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/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 20/28] drm/i2c: tda998x: move the TBG_CNTRL_0 register setting 2014-01-12 13:20 ` Jean-Francois Moine @ 2014-01-12 13:38 ` Russell King - ARM Linux 0 siblings, 0 replies; 6+ messages in thread From: Russell King - ARM Linux @ 2014-01-12 13:38 UTC (permalink / raw) To: Jean-Francois Moine Cc: dri-devel, Rob Clark, Dave Airlie, linux-kernel, linux-arm-kernel 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 <linux@arm.linux.org.uk> wrote: > > > > 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". ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-01-12 13:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-01-09 11:06 [PATCH v2 20/28] drm/i2c: tda998x: move the TBG_CNTRL_0 register setting Jean-Francois Moine 2014-01-11 18:36 ` Russell King - ARM Linux 2014-01-12 12:23 ` Jean-Francois Moine 2014-01-12 12:31 ` Russell King - ARM Linux 2014-01-12 13:20 ` Jean-Francois Moine 2014-01-12 13:38 ` Russell King - ARM Linux
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox