| Message ID | 20140109120607.6a33bee5@armhf |
|---|---|
| State | New |
| Headers | show |
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.
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?
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.
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?
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.
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
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(-)