[PATCH] lxfb: Program panel v/h sync output polarity correctly

[PATCH] lxfb: Program panel v/h sync output polarity correctly

[Daniel Drake]

Commit b5c26f97ec4a17c65 introduced some breakage for the OLPC XO-1 laptop,
differences in the output video signal after the patch caused some problems
with the XO's display controller chip.

Reviewing of that commit against the AMD Geode LX Data Book, it seems
that these bits were being set inversely. In both cases, active high
output is denoted by a value of 0. See section 6.8.3.44 of the databook
from February 2009 (Publication ID: 33234H)

Signed-off-by: Daniel Drake <dsd@laptop.org>
---
 drivers/video/geode/lxfb_ops.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/geode/lxfb_ops.c b/drivers/video/geode/lxfb_ops.c
index bc35a95..85ec7f6 100644
--- a/drivers/video/geode/lxfb_ops.c
+++ b/drivers/video/geode/lxfb_ops.c
@@ -276,10 +276,10 @@ static void lx_graphics_enable(struct fb_info *info)
 		write_fp(par, FP_PT1, 0);
 		temp = FP_PT2_SCRC;
 
-		if (info->var.sync & FB_SYNC_HOR_HIGH_ACT)
+		if (!(info->var.sync & FB_SYNC_HOR_HIGH_ACT))
 			temp |= FP_PT2_HSP;
 
-		if (info->var.sync & FB_SYNC_VERT_HIGH_ACT)
+		if (!(info->var.sync & FB_SYNC_VERT_HIGH_ACT))
 			temp |= FP_PT2_VSP;
 
 		write_fp(par, FP_PT2, temp);
-- 
1.7.3.2

Re: [PATCH] lxfb: Program panel v/h sync output polarity correctly

[Paul Mundt]

On Sun, Nov 14, 2010 at 02:12:31PM +0000, Daniel Drake wrote:
> Commit b5c26f97ec4a17c65 introduced some breakage for the OLPC XO-1 laptop,
> differences in the output video signal after the patch caused some problems
> with the XO's display controller chip.
> 
> Reviewing of that commit against the AMD Geode LX Data Book, it seems
> that these bits were being set inversely. In both cases, active high
> output is denoted by a value of 0. See section 6.8.3.44 of the databook
> from February 2009 (Publication ID: 33234H)
> 
> Signed-off-by: Daniel Drake <dsd@laptop.org>

I'm intending on queuing this for -rc3, which should give Michael enough
time to test on his configuration and make sure nothing has broken there.

Re: [PATCH] lxfb: Program panel v/h sync output polarity correctly

[Michael Grzeschik]

Hi Daniel,

On Sun, Nov 14, 2010 at 02:12:31PM +0000, Daniel Drake wrote:
> Commit b5c26f97ec4a17c65 introduced some breakage for the OLPC XO-1 laptop,
> differences in the output video signal after the patch caused some problems
> with the XO's display controller chip.
> 
> Reviewing of that commit against the AMD Geode LX Data Book, it seems
> that these bits were being set inversely. In both cases, active high
> output is denoted by a value of 0. See section 6.8.3.44 of the databook
> from February 2009 (Publication ID: 33234H)
> 

I rechecked the Datasheet and it seems you are right with that.
But i don't like the logical mess in the statements. So here is what i
would prefer.

> Signed-off-by: Daniel Drake <dsd@laptop.org>
> ---
>  drivers/video/geode/lxfb_ops.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/geode/lxfb_ops.c b/drivers/video/geode/lxfb_ops.c
> index bc35a95..85ec7f6 100644
> --- a/drivers/video/geode/lxfb_ops.c
> +++ b/drivers/video/geode/lxfb_ops.c
> @@ -276,10 +276,10 @@ static void lx_graphics_enable(struct fb_info *info)
>  		write_fp(par, FP_PT1, 0);
>  		temp = FP_PT2_SCRC;
>  
> -		if (info->var.sync & FB_SYNC_HOR_HIGH_ACT)
> +		if (!(info->var.sync & FB_SYNC_HOR_HIGH_ACT))
>  			temp |= FP_PT2_HSP;

Instead i would like to see something like this:


		if (info->var.sync & FB_SYNC_HOR_HIGH_ACT)
                        temp &= ~(FP_PT2_HSP);

>  
> -		if (info->var.sync & FB_SYNC_VERT_HIGH_ACT)
> +		if (!(info->var.sync & FB_SYNC_VERT_HIGH_ACT))
>  			temp |= FP_PT2_VSP;

		if (info->var.sync & FB_SYNC_VERT_HIGH_ACT)
                        temp &= ~(FP_PT2_VSP);

>  
>  		write_fp(par, FP_PT2, temp);
> -- 
> 1.7.3.2

So the meaning of the switch FB_SYNC_{HOR,VERT}_HIGH_ACT stays correct
for the user, but the commits on the register act as described in the
datasheet.

Regards,
Michael

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] lxfb: Program panel v/h sync output polarity correctly

[Daniel Drake]

On 16 November 2010 08:26, Michael Grzeschik <mgr@pengutronix.de> wrote:
> I rechecked the Datasheet and it seems you are right with that.
> But i don't like the logical mess in the statements. So here is what i
> would prefer.
>
>>
>> -             if (info->var.sync & FB_SYNC_HOR_HIGH_ACT)
>> +             if (!(info->var.sync & FB_SYNC_HOR_HIGH_ACT))
>>                       temp |= FP_PT2_HSP;
>
> Instead i would like to see something like this:
>
>
>                if (info->var.sync & FB_SYNC_HOR_HIGH_ACT)
>                        temp &= ~(FP_PT2_HSP);

You'd have to add an else condition to actually set the bit (in the
active low case) though.
I'd be happy to do that, but my opinion is that my original version is cleaner.
Apples vs oranges. Paul, which approach do you prefer?

Thanks,
Daniel

Re: [PATCH] lxfb: Program panel v/h sync output polarity correctly

[Paul Mundt]

On Tue, Nov 16, 2010 at 09:33:52AM +0000, Daniel Drake wrote:
> On 16 November 2010 08:26, Michael Grzeschik <mgr@pengutronix.de> wrote:
> > I rechecked the Datasheet and it seems you are right with that.
> > But i don't like the logical mess in the statements. So here is what i
> > would prefer.
> >
> >>
> >> - ? ? ? ? ? ? if (info->var.sync & FB_SYNC_HOR_HIGH_ACT)
> >> + ? ? ? ? ? ? if (!(info->var.sync & FB_SYNC_HOR_HIGH_ACT))
> >> ? ? ? ? ? ? ? ? ? ? ? temp |= FP_PT2_HSP;
> >
> > Instead i would like to see something like this:
> >
> >
> > ? ? ? ? ? ? ? ?if (info->var.sync & FB_SYNC_HOR_HIGH_ACT)
> > ? ? ? ? ? ? ? ? ? ? ? ?temp &= ~(FP_PT2_HSP);
> 
> You'd have to add an else condition to actually set the bit (in the
> active low case) though.
> I'd be happy to do that, but my opinion is that my original version is cleaner.
> Apples vs oranges. Paul, which approach do you prefer?
> 
I'm fairly ambivalent. Some people like to write these things out
verbosely so it's immediately apparent what is going on, which can often
help when staring at non-obvious logic inversion. However, your original
version matches the current variable utilization fine, and the cases
involved are not that sufficiently complex that they're likely to be
misinterpreted.

The trying to make inverted logic appear more obvious angle is what
necessitated this change in the first place, so keeping it simple and
concise is certainly preferable (and indeed, another subtle bug would
have been introduced by flipping it around in the way proposed, as you
pointed out). Given that, and that we now have Michael's implicit
acknowledgement of the problem, I'll just take your original patch.

I can of course apply a follow-up patch that brings it in line with a
stylistic consensus if you two manage to come up with one at a later
point in time ;-)