Re: radeonfb i2c regression post-2.6.18.

Michael Buesch
2007-11-23T16:02:48Z

On Monday 19 November 2007 00:16:06 Benjamin Herrenschmidt wrote:
> (Also, Michel, can you check if it fixes your other problem with this
> code ? ie. your "hot crash")

> -------- Forwarded Message --------
> From: Jean Delvare <khali@linux-fr.org>
> To: linux-fbdev-devel@lists.sourceforge.net
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>, Antonino Daplas
> <adaplas@gmail.com>
> Subject: [PATCH] fb_ddc: Fix DDC lines quirk
> Date: Sun, 18 Nov 2007 14:21:41 +0100
>
> 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?

This patch fixes my crash problem.
Thanks a lot guys!

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

Acked-by: Michael Buesch <mb@bu3sch.de>

> ---
> 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;
> }
>
>
>
>
>

--
Greetings Michael.