[PATCH v2 19/28] drm/i2c: tda998x: fix the value of the TBG_CNTRL_1 register

[PATCH v2 19/28] drm/i2c: tda998x: fix the value of the TBG_CNTRL_1 register

[Jean-Francois Moine]

This patch fixes the 'toggle flag enable' bit of the TBG_CNTRL_1
register which was set when no toggle was needed.

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 192ddd2..7dbbc6b 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1080,11 +1080,11 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 	 * 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;
+	reg = TBG_CNTRL_1_DWIN_DIS;
 	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
-		reg |= TBG_CNTRL_1_H_TGL;
+		reg |= TBG_CNTRL_1_H_TGL | TBG_CNTRL_1_TGL_EN;
 	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
-		reg |= TBG_CNTRL_1_V_TGL;
+		reg |= TBG_CNTRL_1_V_TGL | TBG_CNTRL_1_TGL_EN;
 	reg_write(priv, REG_TBG_CNTRL_1, reg);
 
 	/* Only setup the info frames if the sink is HDMI */
-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH v2 19/28] drm/i2c: tda998x: fix the value of the TBG_CNTRL_1 register

[Russell King - ARM Linux]

On Thu, Jan 09, 2014 at 12:05:43PM +0100, Jean-Francois Moine wrote:
> This patch fixes the 'toggle flag enable' bit of the TBG_CNTRL_1
> register which was set when no toggle was needed.
> 
> 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 192ddd2..7dbbc6b 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -1080,11 +1080,11 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
>  	 * 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;
> +	reg = TBG_CNTRL_1_DWIN_DIS;
>  	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> -		reg |= TBG_CNTRL_1_H_TGL;
> +		reg |= TBG_CNTRL_1_H_TGL | TBG_CNTRL_1_TGL_EN;
>  	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> -		reg |= TBG_CNTRL_1_V_TGL;
> +		reg |= TBG_CNTRL_1_V_TGL | TBG_CNTRL_1_TGL_EN;
>  	reg_write(priv, REG_TBG_CNTRL_1, reg);

This has me wondering whether you understand the meaning of TGL_EN here,
and what it means.

When TGL_EN is set, the inversion of the syncs is determined by the
settings of the H_TGL and V_TGL bits.  When they're zero, no inversion
happens.

However, when TGL_EN is clear, the inversion is determined by the CEA
mode setting in REG_VIDFORMAT.

What your code above means is that if a mode setting valuates as matching
a CEA mode, but has different syncs (eg, no inversion required) then we
don't set the enable bit, and we fall back to whatever is in the TDA998x
device's internal tables for the CEA mode.  This is wrong behaviour.

If we want to do this, then the right way would be to detect whether a
sync polarity has been specified (iow, whether any [NP][HV]SYNC flags
have been set) and set the TGL_EN if they have.  Otherwise, the TGL_EN
flag can be cleared.

I'm not saying that this will ever result in the TGL_EN flag being
cleared, but to me, your change above is incorrect.

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