[PATCH v2 4/8] drm/i2c: tda998x: prepare for video input configuration

[PATCH v2 4/8] drm/i2c: tda998x: prepare for video input configuration

[Sebastian Hesselbarth]

From: Russell King <rmk+kernel@arm.linux.org.uk>

The video-input-port (VIP) is highly configurable. This prepares
current driver to allow to configure VIP configuration, as some
boards connect lcd controller and TDA998x "pin-swapped" and depend
on VIP to swap the pins by register configuration.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
Tested-by: Darren Etheridge <detheridge@ti.com>
Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: David Airlie <airlied@linux.ie>
Cc: Darren Etheridge <detheridge@ti.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/i2c/tda998x_drv.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index a701411..527d11b 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -32,6 +32,9 @@ struct tda998x_priv {
 	uint16_t rev;
 	uint8_t current_page;
 	int dpms;
+	u8 vip_cntrl_0;
+	u8 vip_cntrl_1;
+	u8 vip_cntrl_2;
 };
 
 #define to_tda998x_priv(x)  ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv)
@@ -448,12 +451,9 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
 		reg_write(encoder, REG_ENA_VP_1, 0xff);
 		reg_write(encoder, REG_ENA_VP_2, 0xff);
 		/* set muxing after enabling ports: */
-		reg_write(encoder, REG_VIP_CNTRL_0,
-				VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3));
-		reg_write(encoder, REG_VIP_CNTRL_1,
-				VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1));
-		reg_write(encoder, REG_VIP_CNTRL_2,
-				VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5));
+		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);
 		break;
 	case DRM_MODE_DPMS_OFF:
 		/* disable audio and video ports */
@@ -823,6 +823,10 @@ tda998x_encoder_init(struct i2c_client *client,
 	if (!priv)
 		return -ENOMEM;
 
+	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->current_page = 0;
 	priv->cec = i2c_new_dummy(client->adapter, 0x34);
 	priv->dpms = DRM_MODE_DPMS_OFF;
-- 
1.7.10.4

Re: [PATCH v2 4/8] drm/i2c: tda998x: prepare for video input configuration

[Jean-Francois Moine]

On Wed Aug 14 12:43:29 PDT 2013, Sebastian Hesselbarth wrote:
> From: Russell King <rmk+kernel at arm.linux.org.uk>
> 
> The video-input-port (VIP) is highly configurable. This prepares
> current driver to allow to configure VIP configuration, as some
> boards connect lcd controller and TDA998x "pin-swapped" and depend
> on VIP to swap the pins by register configuration.
> 
> Signed-off-by: Russell King <rmk+kernel at arm.linux.org.uk>
> Tested-by: Darren Etheridge <detheridge at ti.com>
> Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth at gmail.com>
	[snip]

AFAIK, the TI boards have no "pin-swapped", nor has the Cubox (there is
no need to set the bit CFG_GRA_SWAPRB of the register LCD_SPU_DMA_CTRL0
of the Dove lcd for RGB or YUV formats).

Which board needs a special VIP configuration?

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

Re: [PATCH v2 4/8] drm/i2c: tda998x: prepare for video input configuration

[Russell King - ARM Linux]

On Wed, Aug 21, 2013 at 08:26:46PM +0200, Jean-Francois Moine wrote:
> On Wed Aug 14 12:43:29 PDT 2013, Sebastian Hesselbarth wrote:
> > From: Russell King <rmk+kernel at arm.linux.org.uk>
> > 
> > The video-input-port (VIP) is highly configurable. This prepares
> > current driver to allow to configure VIP configuration, as some
> > boards connect lcd controller and TDA998x "pin-swapped" and depend
> > on VIP to swap the pins by register configuration.
> > 
> > Signed-off-by: Russell King <rmk+kernel at arm.linux.org.uk>
> > Tested-by: Darren Etheridge <detheridge at ti.com>
> > Tested-by: Sebastian Hesselbarth <sebastian.hesselbarth at gmail.com>
> 	[snip]
> 
> AFAIK, the TI boards have no "pin-swapped", nor has the Cubox (there is
> no need to set the bit CFG_GRA_SWAPRB of the register LCD_SPU_DMA_CTRL0
> of the Dove lcd for RGB or YUV formats).
> 
> Which board needs a special VIP configuration?

If you run the NXP driver, and then run this driver, things get messed
up - which has already been covered months ago when this patch was first
brought up.

It's there to ensure that the TDA998x is correctly configured no matter
what it's previous state is, and prevent the thing being fragile as hell.
No, reset doesn't restore its settings, only a power cycle does.

Re: [PATCH v2 4/8] drm/i2c: tda998x: prepare for video input configuration

[Jean-Francois Moine]

On Wed, 21 Aug 2013 23:36:05 +0100
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> > AFAIK, the TI boards have no "pin-swapped", nor has the Cubox (there is
> > no need to set the bit CFG_GRA_SWAPRB of the register LCD_SPU_DMA_CTRL0
> > of the Dove lcd for RGB or YUV formats).
> > 
> > Which board needs a special VIP configuration?  
> 
> If you run the NXP driver, and then run this driver, things get messed
> up - which has already been covered months ago when this patch was first
> brought up.
> 
> It's there to ensure that the TDA998x is correctly configured no matter
> what it's previous state is, and prevent the thing being fragile as hell.

The NXP driver will never go to the mainline, so, I don't see the
problem. If you want to use it to test some other drivers, you should
better patch it instead of adding useless code in the TDA998x driver.

> No, reset doesn't restore its settings, only a power cycle does.

Sorry, all VIP control registers may be changed at any time and the
change appears immediately (thank you for the /sys i2c_read/write).

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

Re: [PATCH v2 4/8] drm/i2c: tda998x: prepare for video input configuration

[Russell King - ARM Linux]

On Thu, Aug 22, 2013 at 08:53:13AM +0200, Jean-Francois Moine wrote:
> On Wed, 21 Aug 2013 23:36:05 +0100
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > > AFAIK, the TI boards have no "pin-swapped", nor has the Cubox (there is
> > > no need to set the bit CFG_GRA_SWAPRB of the register LCD_SPU_DMA_CTRL0
> > > of the Dove lcd for RGB or YUV formats).
> > > 
> > > Which board needs a special VIP configuration?  
> > 
> > If you run the NXP driver, and then run this driver, things get messed
> > up - which has already been covered months ago when this patch was first
> > brought up.
> > 
> > It's there to ensure that the TDA998x is correctly configured no matter
> > what it's previous state is, and prevent the thing being fragile as hell.
> 
> The NXP driver will never go to the mainline, so, I don't see the
> problem. If you want to use it to test some other drivers, you should
> better patch it instead of adding useless code in the TDA998x driver.

Sorry, you're wrong.

Re: [PATCH v2 4/8] drm/i2c: tda998x: prepare for video input configuration

[Rob Clark]

On Thu, Aug 22, 2013 at 2:53 AM, Jean-Francois Moine <moinejf@free.fr> wrote:
> On Wed, 21 Aug 2013 23:36:05 +0100
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
>
>> > AFAIK, the TI boards have no "pin-swapped", nor has the Cubox (there is
>> > no need to set the bit CFG_GRA_SWAPRB of the register LCD_SPU_DMA_CTRL0
>> > of the Dove lcd for RGB or YUV formats).
>> >
>> > Which board needs a special VIP configuration?
>>
>> If you run the NXP driver, and then run this driver, things get messed
>> up - which has already been covered months ago when this patch was first
>> brought up.
>>
>> It's there to ensure that the TDA998x is correctly configured no matter
>> what it's previous state is, and prevent the thing being fragile as hell.
>
> The NXP driver will never go to the mainline, so, I don't see the
> problem. If you want to use it to test some other drivers, you should
> better patch it instead of adding useless code in the TDA998x driver.

I don't think it really matters for the end user if NXP isn't
mainline.  If they are jumping between vendor kernel and mainline, and
inheriting some state left over from the NXP driver in vendor kernel,
it makes debugging very confusing.  It would be less of an issue if a
warm reset actually reset the tda998x part, but that is not the case,
it is better to rely less on the hw state when the driver is loaded,
IMHO.

BR,
-R


>> No, reset doesn't restore its settings, only a power cycle does.
>
> Sorry, all VIP control registers may be changed at any time and the
> change appears immediately (thank you for the /sys i2c_read/write).
>
> --
> Ken ar c'hentañ |             ** Breizh ha Linux atav! **
> Jef             |               http://moinejf.free.fr/

Re: [PATCH v2 4/8] drm/i2c: tda998x: prepare for video input configuration

[Russell King - ARM Linux]

On Thu, Aug 22, 2013 at 07:33:43AM -0400, Rob Clark wrote:
> On Thu, Aug 22, 2013 at 2:53 AM, Jean-Francois Moine <moinejf@free.fr> wrote:
> > On Wed, 21 Aug 2013 23:36:05 +0100
> > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> >
> >> > AFAIK, the TI boards have no "pin-swapped", nor has the Cubox (there is
> >> > no need to set the bit CFG_GRA_SWAPRB of the register LCD_SPU_DMA_CTRL0
> >> > of the Dove lcd for RGB or YUV formats).
> >> >
> >> > Which board needs a special VIP configuration?
> >>
> >> If you run the NXP driver, and then run this driver, things get messed
> >> up - which has already been covered months ago when this patch was first
> >> brought up.
> >>
> >> It's there to ensure that the TDA998x is correctly configured no matter
> >> what it's previous state is, and prevent the thing being fragile as hell.
> >
> > The NXP driver will never go to the mainline, so, I don't see the
> > problem. If you want to use it to test some other drivers, you should
> > better patch it instead of adding useless code in the TDA998x driver.
> 
> I don't think it really matters for the end user if NXP isn't
> mainline.  If they are jumping between vendor kernel and mainline, and
> inheriting some state left over from the NXP driver in vendor kernel,
> it makes debugging very confusing.  It would be less of an issue if a
> warm reset actually reset the tda998x part, but that is not the case,
> it is better to rely less on the hw state when the driver is loaded,
> IMHO.

Absolutely right, thanks for backing up what I've said.  I've done exactly
that - switching between the NXP driver and the mainline driver to debug
other problems, and not having the TDA998x setup correctly just makes the
job much harder and time consuming.

I keep both drivers available in my internal git tree so that I can switch
between them when necessary.