[PATCH] fb_ddc: Fix DDC lines quirk

[PATCH] fb_ddc: Fix DDC lines quirk

[Jean Delvare]

The code in fb_ddc_read() is said to be based on the implementation
of the radeon driver:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=fc5891c8a3ba284f13994d7bc1f1bfa8283982de

However, comparing the old radeon driver code with the new fb_ddc code
reveals some differences. Most notably, the I2C bus lines are held at
the end of the function, while the original code was releasing them
(as the comment above correctly says.)

There are a few other differences, which appear to be responsible for
read failures on my system. While tracing low-level I2C code in
i2c-algo-bit, I noticed that the initial attempt to read the EDID
always failed. It takes one retry for the read to succeed. As we are
about to remove this automatic retry property from i2c-algo-bit,
reading the EDID would really fail.

As a summary, the I2C lines quirk which is supposedly needed to read
EDID on some older monitors is currently breaking the (first) read on
all other monitors (and might not even work with older ones - did
anyone try since October 2006?)

After applying the patch below, which makes the code in fb_ddc_read()
really similar to what the radeon driver used to have, the first EDID
read succeeds again.

On top of that, as it appears that this code has been broken for one
year now and nobody seems to have complained, I'm curious if it makes
sense to keep this quirk in place. It makes the code more complex and
slower just for the sake of monitors which I guess nobody uses
anymore. Can't we just get rid of it?

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 drivers/video/fb_ddc.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

--- linux-2.6.24-rc3.orig/drivers/video/fb_ddc.c	2007-11-17 20:23:03.000000000 +0100
+++ linux-2.6.24-rc3/drivers/video/fb_ddc.c	2007-11-18 12:49:14.000000000 +0100
@@ -56,13 +56,12 @@ unsigned char *fb_ddc_read(struct i2c_ad
 	int i, j;
 
 	algo_data->setscl(algo_data->data, 1);
-	algo_data->setscl(algo_data->data, 0);
 
 	for (i = 0; i < 3; i++) {
 		/* For some old monitors we need the
 		 * following process to initialize/stop DDC
 		 */
-		algo_data->setsda(algo_data->data, 0);
+		algo_data->setsda(algo_data->data, 1);
 		msleep(13);
 
 		algo_data->setscl(algo_data->data, 1);
@@ -97,14 +96,15 @@ unsigned char *fb_ddc_read(struct i2c_ad
 		algo_data->setsda(algo_data->data, 1);
 		msleep(15);
 		algo_data->setscl(algo_data->data, 0);
+		algo_data->setsda(algo_data->data, 0);
 		if (edid)
 			break;
 	}
 	/* Release the DDC lines when done or the Apple Cinema HD display
 	 * will switch off
 	 */
-	algo_data->setsda(algo_data->data, 0);
-	algo_data->setscl(algo_data->data, 0);
+	algo_data->setsda(algo_data->data, 1);
+	algo_data->setscl(algo_data->data, 1);
 
 	return edid;
 }


-- 
Jean Delvare

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] fb_ddc: Fix DDC lines quirk

[Benjamin Herrenschmidt]

On Sun, 2007-11-18 at 14:21 +0100, Jean Delvare wrote:
> 
> On top of that, as it appears that this code has been broken for one
> year now and nobody seems to have complained, I'm curious if it makes
> sense to keep this quirk in place. It makes the code more complex and
> slower just for the sake of monitors which I guess nobody uses
> anymore. Can't we just get rid of it?
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>

I'll have to double check, I suspect some Apple monitors may choke on
the "proper" way of releasing the lines (shut down assuming no more host
is connected, some stupid trick they do). I'll test when I get a chance.

Regarding the quirk, well... I think the X folks are tempted to get rid
of it too.

Ben.



-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] fb_ddc: Fix DDC lines quirk

[Benjamin Herrenschmidt]

On Sun, 2007-11-18 at 14:21 +0100, Jean Delvare wrote:
> The code in fb_ddc_read() is said to be based on the implementation
> of the radeon driver:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=fc5891c8a3ba284f13994d7bc1f1bfa8283982de
> 
> However, comparing the old radeon driver code with the new fb_ddc code
> reveals some differences. Most notably, the I2C bus lines are held at
> the end of the function, while the original code was releasing them
> (as the comment above correctly says.)

 .../...

> Signed-off-by: Jean Delvare <khali@linux-fr.org>

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

> ---
>  drivers/video/fb_ddc.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> --- linux-2.6.24-rc3.orig/drivers/video/fb_ddc.c	2007-11-17 20:23:03.000000000 +0100
> +++ linux-2.6.24-rc3/drivers/video/fb_ddc.c	2007-11-18 12:49:14.000000000 +0100
> @@ -56,13 +56,12 @@ unsigned char *fb_ddc_read(struct i2c_ad
>  	int i, j;
>  
>  	algo_data->setscl(algo_data->data, 1);
> -	algo_data->setscl(algo_data->data, 0);
>  
>  	for (i = 0; i < 3; i++) {
>  		/* For some old monitors we need the
>  		 * following process to initialize/stop DDC
>  		 */
> -		algo_data->setsda(algo_data->data, 0);
> +		algo_data->setsda(algo_data->data, 1);
>  		msleep(13);
>  
>  		algo_data->setscl(algo_data->data, 1);
> @@ -97,14 +96,15 @@ unsigned char *fb_ddc_read(struct i2c_ad
>  		algo_data->setsda(algo_data->data, 1);
>  		msleep(15);
>  		algo_data->setscl(algo_data->data, 0);
> +		algo_data->setsda(algo_data->data, 0);
>  		if (edid)
>  			break;
>  	}
>  	/* Release the DDC lines when done or the Apple Cinema HD display
>  	 * will switch off
>  	 */
> -	algo_data->setsda(algo_data->data, 0);
> -	algo_data->setscl(algo_data->data, 0);
> +	algo_data->setsda(algo_data->data, 1);
> +	algo_data->setscl(algo_data->data, 1);
>  
>  	return edid;
>  }
> 
> 


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] fb_ddc: Fix DDC lines quirk

[Jean Delvare]

On Thu, 22 Nov 2007 06:58:49 +1100, Benjamin Herrenschmidt wrote:
> 
> On Sun, 2007-11-18 at 14:21 +0100, Jean Delvare wrote:
> > The code in fb_ddc_read() is said to be based on the implementation
> > of the radeon driver:
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=fc5891c8a3ba284f13994d7bc1f1bfa8283982de
> > 
> > However, comparing the old radeon driver code with the new fb_ddc code
> > reveals some differences. Most notably, the I2C bus lines are held at
> > the end of the function, while the original code was releasing them
> > (as the comment above correctly says.)
> 
>  .../...
> 
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> 
> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Thanks. Antonino, this patch fixes two reported regressions. Can you
please send it to Linus before 2.6.24 is released? Thanks.

> 
> > ---
> >  drivers/video/fb_ddc.c |    8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > --- linux-2.6.24-rc3.orig/drivers/video/fb_ddc.c	2007-11-17 20:23:03.000000000 +0100
> > +++ linux-2.6.24-rc3/drivers/video/fb_ddc.c	2007-11-18 12:49:14.000000000 +0100
> > @@ -56,13 +56,12 @@ unsigned char *fb_ddc_read(struct i2c_ad
> >  	int i, j;
> >  
> >  	algo_data->setscl(algo_data->data, 1);
> > -	algo_data->setscl(algo_data->data, 0);
> >  
> >  	for (i = 0; i < 3; i++) {
> >  		/* For some old monitors we need the
> >  		 * following process to initialize/stop DDC
> >  		 */
> > -		algo_data->setsda(algo_data->data, 0);
> > +		algo_data->setsda(algo_data->data, 1);
> >  		msleep(13);
> >  
> >  		algo_data->setscl(algo_data->data, 1);
> > @@ -97,14 +96,15 @@ unsigned char *fb_ddc_read(struct i2c_ad
> >  		algo_data->setsda(algo_data->data, 1);
> >  		msleep(15);
> >  		algo_data->setscl(algo_data->data, 0);
> > +		algo_data->setsda(algo_data->data, 0);
> >  		if (edid)
> >  			break;
> >  	}
> >  	/* Release the DDC lines when done or the Apple Cinema HD display
> >  	 * will switch off
> >  	 */
> > -	algo_data->setsda(algo_data->data, 0);
> > -	algo_data->setscl(algo_data->data, 0);
> > +	algo_data->setsda(algo_data->data, 1);
> > +	algo_data->setscl(algo_data->data, 1);
> >  
> >  	return edid;
> >  }
> > 
> > 

-- 
Jean Delvare

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] fb_ddc: Fix DDC lines quirk

[Benjamin Herrenschmidt]

On Fri, 2007-11-23 at 20:53 +0100, Jean Delvare wrote:
> On Thu, 22 Nov 2007 06:58:49 +1100, Benjamin Herrenschmidt wrote:
> > 
> > On Sun, 2007-11-18 at 14:21 +0100, Jean Delvare wrote:
> > > The code in fb_ddc_read() is said to be based on the implementation
> > > of the radeon driver:
> > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=fc5891c8a3ba284f13994d7bc1f1bfa8283982de
> > > 
> > > However, comparing the old radeon driver code with the new fb_ddc code
> > > reveals some differences. Most notably, the I2C bus lines are held at
> > > the end of the function, while the original code was releasing them
> > > (as the comment above correctly says.)
> > 
> >  .../...
> > 
> > > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > 
> > Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> Thanks. Antonino, this patch fixes two reported regressions. Can you
> please send it to Linus before 2.6.24 is released? Thanks.

It should also be send to -stable I believe and possibly distros.

Cheers,
Ben.



-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/