[PATCH v3] (new way) I2C/DDC defaults & I2C/DDC detection for VIA

[PATCH v3] (new way) I2C/DDC defaults & I2C/DDC detection for VIA

[Dzianis Kahanovich]

I2C/DDC defaults & I2C/DDC detection for VIA framebuffer.

Adding defaults & legacy I2C/DDC support for VIA framebuffer.
Detecting legacy I2C/DDC outputs (if undetected else) and
set autodetected defaults on empty "viafb_active_dev".

v2: fixed configurable way LCD order
v3: fixed CRT

Signed-off-by: Dzianis Kahanovich <mahatma@eu.by>
---
diff -pruN a/viafbdev.c c/viafbdev.c
--- a/drivers/video/via/viafbdev.c	2010-12-07 18:35:22.000000000 +0200
+++ c/drivers/video/via/viafbdev.c	2010-12-09 04:04:49.000000000 +0200
@@ -1485,6 +1485,135 @@ static int parse_mode(const char *str, u
 	return 0;
 }

+/* stage=0: count devices to set dual and/or SAMM;
+   stage=1: add devices, detected only via i2c legacy;
+   set LCD/DVI/CRT if viafb_active_dev unset */
+static void viafb_detect_dev(int stage, struct viafb_dev *vdev)
+{
+	u8 *edid;
+	struct fb_var_screeninfo var;
+	int i, t, ndev = 0, nlcd = 0, unknown = 0;
+	struct i2c_adapter *adapter;
+	struct lvds_setting_information *inf;
+
+	/* FIXME: viaparinfo1->chip_info looks equal to viaparinfo */
+	i = !viafb_active_dev && stage;
+	if (viaparinfo->chip_info->tmds_chip_info.tmds_chip_name) {
+		ndev++;
+		if (i)
+			viafb_DVI_ON = STATE_ON;
+	}
+	if (viaparinfo->chip_info->lvds_chip_info.lvds_chip_name) {
+		ndev++;
+		nlcd = 1;
+		if (i)
+			viafb_LCD_ON = STATE_ON;
+	}
+	if (viaparinfo->chip_info->lvds_chip_info2.lvds_chip_name) {
+		ndev++;
+		nlcd = 2;
+		if (i)
+			viafb_LCD2_ON = STATE_ON;
+	}
+	/* enabling CRT in textmode is at least no bad */
+	if (viafb_CRT_ON) {
+		ndev++;
+		viafb_crt_enable();
+	}
+	for (i = 0; i < VIAFB_NUM_PORTS; i++) {
+		t = vdev->port_cfg[i].type;
+		/* detect only i2c ports, undetected in other places */
+		if ((viaparinfo && viaparinfo->chip_info && (
+		    (viaparinfo->chip_info->tmds_chip_info.tmds_chip_name &&
+		    viaparinfo->chip_info->tmds_chip_info.i2c_port = t) ||
+		    (viaparinfo->chip_info->lvds_chip_info.lvds_chip_name &&
+		    viaparinfo->chip_info->lvds_chip_info.i2c_port = t) ||
+		    (viaparinfo->chip_info->lvds_chip_info2.lvds_chip_name &&
+		    viaparinfo->chip_info->lvds_chip_info2.i2c_port = t)
+		    )) || (viaparinfo1 && viaparinfo1->chip_info && (
+		    (viaparinfo1->chip_info->tmds_chip_info.tmds_chip_name &&
+		    viaparinfo1->chip_info->tmds_chip_info.i2c_port = t) ||
+		    (viaparinfo1->chip_info->lvds_chip_info.lvds_chip_name &&
+		    viaparinfo1->chip_info->lvds_chip_info.i2c_port = t) ||
+		    (viaparinfo1->chip_info->lvds_chip_info2.lvds_chip_name &&
+		    viaparinfo1->chip_info->lvds_chip_info2.i2c_port = t)
+		    )) || !(adapter = viafb_find_i2c_adapter(i)) ||
+		    !(edid = fb_ddc_read(adapter)))
+			continue;
+		memset(&var, 0, sizeof(var));
+		if (fb_parse_edid(edid, &var))
+			goto free_edid;
+		printk(KERN_INFO "viafb: %48s\n", adapter->name);
+		inf = NULL;
+		if (!stage) {
+		} else if (!nlcd) {
+			fb_edid_to_monspecs(edid, &viafbinfo->monspecs);
+			if (viafbinfo->monspecs.input & FB_DISP_DDI)
+				inf = viaparinfo->lvds_setting_info;
+			else
+				unknown++;
+		} else if (nlcd > 1) {
+			printk(KERN_ERR "viafb: too many LCD\n");
+			unknown++;
+		} else if (viafb_dual_fb) {
+			fb_edid_to_monspecs(edid, &viafbinfo1->monspecs);
+			if (viafbinfo1->monspecs.input & FB_DISP_DDI)
+				inf = viaparinfo1->lvds_setting_info;
+			else
+				unknown++;
+		} else {
+			fb_edid_to_monspecs(edid, &viafbinfo->monspecs);
+			if (viafbinfo->monspecs.input & FB_DISP_DDI)
+				inf = viaparinfo->lvds_setting_info2;
+			else
+				unknown++;
+		}
+		if (inf) {
+			if (!viafb_active_dev) {
+				if (nlcd)
+					viafb_LCD2_ON = STATE_ON;
+				else
+					viafb_LCD_ON = STATE_ON;
+			}
+			nlcd++;
+			if (var.xres)
+				inf->lcd_panel_hres = var.xres;
+			if (var.yres)
+				inf->lcd_panel_vres = var.yres;
+		}
+		ndev++;
+free_edid:
+		kfree(edid);
+	}
+	if (!viafb_active_dev) {
+		/* prefer CRT OFF if other devices */
+#if 1
+		if (unknown) {
+			if (!viafb_CRT_ON) {
+				viafb_CRT_ON = STATE_ON;
+				ndev++;
+			}
+		} else if (ndev > 1 && viafb_CRT_ON) {
+			viafb_CRT_ON = STATE_OFF;
+			ndev--;
+		}
+#endif
+		/* SAMM may be detected on stage 1,
+		   but troubles coming together & dual not wrong */
+		if (ndev > 1 && !stage)
+			viafb_SAMM_ON = viafb_dual_fb = STATE_ON;
+		viafb_DeviceStatus = viafb_primary_dev +		    viafb_CRT_ON ? CRT_Device :
+		    viafb_DVI_ON ? DVI_Device :
+		    viafb_LCD_ON ? LCD_Device : None_Device;
+		if (stage) {
+			if (!viafb_CRT_ON)
+				viafb_crt_disable();
+			viafb_set_iga_path();
+		}
+	}
+}
+

 int __devinit via_fb_pci_probe(struct viafb_dev *vdev)
 {
@@ -1526,6 +1655,10 @@ int __devinit via_fb_pci_probe(struct vi
 	parse_dvi_port();

 	viafb_init_chip_info(vdev->chip_type);
+	/* detect dual_fb & SAMM_ON, but let's keep it to options */
+#if 0
+	viafb_detect_dev(0, vdev);
+#endif
 	/*
 	 * The framebuffer will have been successfully mapped by
 	 * the core (or we'd not be here), but we still need to
@@ -1675,6 +1808,8 @@ int __devinit via_fb_pci_probe(struct vi
 	viafb_init_proc(&viaparinfo->shared->proc_entry);
 #endif
 	viafb_init_dac(IGA2);
+	/* update from legacy i2c DDC info */
+	viafb_detect_dev(1, vdev);
 	return 0;

 out_fb_unreg:
diff -pruN a/via_i2c.c c/via_i2c.c
--- a/drivers/video/via/via_i2c.c	2010-12-08 15:12:05.000000000 +0200
+++ c/drivers/video/via/via_i2c.c	2010-11-29 18:04:24.000000000 +0200
@@ -167,7 +167,7 @@ struct i2c_adapter *viafb_find_i2c_adapt
 {
 	struct via_i2c_stuff *stuff = &via_i2c_par[which];

-	return &stuff->adapter;
+	return stuff->is_active ? &stuff->adapter : NULL;
 }
 EXPORT_SYMBOL_GPL(viafb_find_i2c_adapter);

--

Re: [PATCH v3] (new way) I2C/DDC defaults & I2C/DDC detection for

[Florian Tobias Schandinat]

Hello,

Dzianis Kahanovich schrieb:
> I2C/DDC defaults & I2C/DDC detection for VIA framebuffer.
> 
> Adding defaults & legacy I2C/DDC support for VIA framebuffer.
> Detecting legacy I2C/DDC outputs (if undetected else) and
> set autodetected defaults on empty "viafb_active_dev".
> 
> v2: fixed configurable way LCD order
> v3: fixed CRT

Again, the v2/v3 stuff is good but it would be even better if you wrote it below 
your Signed-off-by, separating it by a line with "---" as such things should not 
be shown in "git log".

> 
> Signed-off-by: Dzianis Kahanovich <mahatma@eu.by>
> ---
> diff -pruN a/viafbdev.c c/viafbdev.c
> --- a/drivers/video/via/viafbdev.c	2010-12-07 18:35:22.000000000 +0200
> +++ c/drivers/video/via/viafbdev.c	2010-12-09 04:04:49.000000000 +0200

I'm wondering which tree are you using as a base?
Your patch does not apply and not compile. Please use at least latest mainline 
as a base or even better current linux-next.

> @@ -1485,6 +1485,135 @@ static int parse_mode(const char *str, u
>  	return 0;
>  }
> 
> +/* stage=0: count devices to set dual and/or SAMM;
> +   stage=1: add devices, detected only via i2c legacy;
> +   set LCD/DVI/CRT if viafb_active_dev unset */
> +static void viafb_detect_dev(int stage, struct viafb_dev *vdev)
> +{
> +	u8 *edid;
> +	struct fb_var_screeninfo var;
> +	int i, t, ndev = 0, nlcd = 0, unknown = 0;
> +	struct i2c_adapter *adapter;
> +	struct lvds_setting_information *inf;
> +
> +	/* FIXME: viaparinfo1->chip_info looks equal to viaparinfo */
> +	i = !viafb_active_dev && stage;
> +	if (viaparinfo->chip_info->tmds_chip_info.tmds_chip_name) {
> +		ndev++;
> +		if (i)
> +			viafb_DVI_ON = STATE_ON;
> +	}
> +	if (viaparinfo->chip_info->lvds_chip_info.lvds_chip_name) {
> +		ndev++;
> +		nlcd = 1;
> +		if (i)
> +			viafb_LCD_ON = STATE_ON;
> +	}
> +	if (viaparinfo->chip_info->lvds_chip_info2.lvds_chip_name) {
> +		ndev++;
> +		nlcd = 2;
> +		if (i)
> +			viafb_LCD2_ON = STATE_ON;
> +	}
> +	/* enabling CRT in textmode is at least no bad */
> +	if (viafb_CRT_ON) {
> +		ndev++;
> +		viafb_crt_enable();
> +	}
> +	for (i = 0; i < VIAFB_NUM_PORTS; i++) {
> +		t = vdev->port_cfg[i].type;
> +		/* detect only i2c ports, undetected in other places */
> +		if ((viaparinfo && viaparinfo->chip_info && (
> +		    (viaparinfo->chip_info->tmds_chip_info.tmds_chip_name &&
> +		    viaparinfo->chip_info->tmds_chip_info.i2c_port = t) ||
> +		    (viaparinfo->chip_info->lvds_chip_info.lvds_chip_name &&
> +		    viaparinfo->chip_info->lvds_chip_info.i2c_port = t) ||
> +		    (viaparinfo->chip_info->lvds_chip_info2.lvds_chip_name &&
> +		    viaparinfo->chip_info->lvds_chip_info2.i2c_port = t)
> +		    )) || (viaparinfo1 && viaparinfo1->chip_info && (
> +		    (viaparinfo1->chip_info->tmds_chip_info.tmds_chip_name &&
> +		    viaparinfo1->chip_info->tmds_chip_info.i2c_port = t) ||
> +		    (viaparinfo1->chip_info->lvds_chip_info.lvds_chip_name &&
> +		    viaparinfo1->chip_info->lvds_chip_info.i2c_port = t) ||
> +		    (viaparinfo1->chip_info->lvds_chip_info2.lvds_chip_name &&
> +		    viaparinfo1->chip_info->lvds_chip_info2.i2c_port = t)
> +		    )) || !(adapter = viafb_find_i2c_adapter(i)) ||
> +		    !(edid = fb_ddc_read(adapter)))
> +			continue;
> +		memset(&var, 0, sizeof(var));
> +		if (fb_parse_edid(edid, &var))
> +			goto free_edid;
> +		printk(KERN_INFO "viafb: %48s\n", adapter->name);
> +		inf = NULL;
> +		if (!stage) {
> +		} else if (!nlcd) {
> +			fb_edid_to_monspecs(edid, &viafbinfo->monspecs);
> +			if (viafbinfo->monspecs.input & FB_DISP_DDI)
> +				inf = viaparinfo->lvds_setting_info;
> +			else
> +				unknown++;
> +		} else if (nlcd > 1) {
> +			printk(KERN_ERR "viafb: too many LCD\n");
> +			unknown++;
> +		} else if (viafb_dual_fb) {
> +			fb_edid_to_monspecs(edid, &viafbinfo1->monspecs);
> +			if (viafbinfo1->monspecs.input & FB_DISP_DDI)
> +				inf = viaparinfo1->lvds_setting_info;
> +			else
> +				unknown++;
> +		} else {
> +			fb_edid_to_monspecs(edid, &viafbinfo->monspecs);
> +			if (viafbinfo->monspecs.input & FB_DISP_DDI)
> +				inf = viaparinfo->lvds_setting_info2;
> +			else
> +				unknown++;
> +		}
> +		if (inf) {
> +			if (!viafb_active_dev) {
> +				if (nlcd)
> +					viafb_LCD2_ON = STATE_ON;
> +				else
> +					viafb_LCD_ON = STATE_ON;
> +			}
> +			nlcd++;
> +			if (var.xres)
> +				inf->lcd_panel_hres = var.xres;
> +			if (var.yres)
> +				inf->lcd_panel_vres = var.yres;
> +		}
> +		ndev++;
> +free_edid:
> +		kfree(edid);
> +	}
> +	if (!viafb_active_dev) {
> +		/* prefer CRT OFF if other devices */
> +#if 1
> +		if (unknown) {
> +			if (!viafb_CRT_ON) {
> +				viafb_CRT_ON = STATE_ON;
> +				ndev++;
> +			}
> +		} else if (ndev > 1 && viafb_CRT_ON) {
> +			viafb_CRT_ON = STATE_OFF;
> +			ndev--;
> +		}
> +#endif
> +		/* SAMM may be detected on stage 1,
> +		   but troubles coming together & dual not wrong */
> +		if (ndev > 1 && !stage)
> +			viafb_SAMM_ON = viafb_dual_fb = STATE_ON;
> +		viafb_DeviceStatus = viafb_primary_dev > +		    viafb_CRT_ON ? CRT_Device :
> +		    viafb_DVI_ON ? DVI_Device :
> +		    viafb_LCD_ON ? LCD_Device : None_Device;
> +		if (stage) {
> +			if (!viafb_CRT_ON)
> +				viafb_crt_disable();
> +			viafb_set_iga_path();
> +		}
> +	}
> +}
> +

You are trying to do 2 sorts of things in this function:

- You are trying to detect the lcd_panel_hres/vres. This is usually a good thing 
but you shouldn't blindly call them after the existing detection is done but 
rather sanely integrate it in (extend) the already "detection" in lcd.c (I am 
not happy that this would move the EDID code in the LCD area but as long as we 
modify the lvds structure the only sane way is to call it from lcd.c)

- You are trying to change the global default configuration. This is a lot 
trickier as it is easy to make it work for you but break it for others. If you 
do this you have to prove (code does the obvious correct thing) that it is 
closer to what everybody needs

> 
>  int __devinit via_fb_pci_probe(struct viafb_dev *vdev)
>  {
> @@ -1526,6 +1655,10 @@ int __devinit via_fb_pci_probe(struct vi
>  	parse_dvi_port();
> 
>  	viafb_init_chip_info(vdev->chip_type);
> +	/* detect dual_fb & SAMM_ON, but let's keep it to options */
> +#if 0
> +	viafb_detect_dev(0, vdev);

As this is never true, please remove it. Adding dead code is strictly forbidden 
and I've spent lots of days kicking such code out of the driver. Removing it of 
course includes removing your whole stage stuff.

> +#endif
>  	/*
>  	 * The framebuffer will have been successfully mapped by
>  	 * the core (or we'd not be here), but we still need to
> @@ -1675,6 +1808,8 @@ int __devinit via_fb_pci_probe(struct vi
>  	viafb_init_proc(&viaparinfo->shared->proc_entry);
>  #endif
>  	viafb_init_dac(IGA2);
> +	/* update from legacy i2c DDC info */
> +	viafb_detect_dev(1, vdev);
>  	return 0;
> 
>  out_fb_unreg:
> diff -pruN a/via_i2c.c c/via_i2c.c
> --- a/drivers/video/via/via_i2c.c	2010-12-08 15:12:05.000000000 +0200
> +++ c/drivers/video/via/via_i2c.c	2010-11-29 18:04:24.000000000 +0200
> @@ -167,7 +167,7 @@ struct i2c_adapter *viafb_find_i2c_adapt
>  {
>  	struct via_i2c_stuff *stuff = &via_i2c_par[which];
> 
> -	return &stuff->adapter;
> +	return stuff->is_active ? &stuff->adapter : NULL;

Perhaps you should send this one as a separate patch. It is unrelated to your 
other stuff and looks good to me.

>  }
>  EXPORT_SYMBOL_GPL(viafb_find_i2c_adapter);
> 
> --


Thanks,

Florian Tobias Schandinat

Re: [PATCH v3] (new way) I2C/DDC defaults & I2C/DDC detection for

[Dzianis Kahanovich]

Florian Tobias Schandinat wrote:

I'm don't read my mail even more, sorry.
OK, I will try to complete most of your recommendations. But few questions:

> I'm wondering which tree are you using as a base?
> Your patch does not apply and not compile. Please use at least latest mainline
> as a base or even better current linux-next.

OK. First I was use linux-git, but may be (now not check) fall to release (at
home) (git & release differs over line numbers only on that moment). Now I will
use only linux-next latest.

[...]
> 
> You are trying to do 2 sorts of things in this function:
> 
> - You are trying to detect the lcd_panel_hres/vres. This is usually a good thing
> but you shouldn't blindly call them after the existing detection is done but
> rather sanely integrate it in (extend) the already "detection" in lcd.c (I am
> not happy that this would move the EDID code in the LCD area but as long as we
> modify the lvds structure the only sane way is to call it from lcd.c)

IMHO this tasks are not too same, but I will think. Really I detect not LCD, but
"any EDID". But while my CRT (second device) don't return EDID and I unable to
sure detect something else - I detect LCD only. Againg, various data required
after detection. But I will look to code and think.

> 
> - You are trying to change the global default configuration. This is a lot
> trickier as it is easy to make it work for you but break it for others. If you
> do this you have to prove (code does the obvious correct thing) that it is
> closer to what everybody needs

HMM. May be I do wrong. But IMHO defaults vs. "dead device" (this is dead by
default for anybody, exclude CRT) is not crime ;)

>>      viafb_init_chip_info(vdev->chip_type);
>> +    /* detect dual_fb & SAMM_ON, but let's keep it to options */
>> +#if 0
>> +    viafb_detect_dev(0, vdev);
> 
> As this is never true, please remove it. Adding dead code is strictly forbidden
> and I've spent lots of days kicking such code out of the driver. Removing it of
> course includes removing your whole stage stuff.

OK. I just keep dual/SAMM detection working up to last moment. But finally I
start to think, SAMM/dual defaults are not same with "viafb_active_dev" and may
be better to keep it (I don't know now if SAMM/dual not set by user). But it it
work even on my not "dual_fb" card (CRT+LCD workd anymore) and if anybody think
this is good... But while you say about changing defaults is wrong - I will
remove it.

In terms of "best default config" SAMM/dual detection is good, but in terms of
"don't touch" - no. Still unsure even now ;)

> Perhaps you should send this one as a separate patch. It is unrelated to your
> other stuff and looks good to me.

OK, I separate it. But relative related - while it is completely "dead code" and
don't used nowhere else and I cared to using it. ;)


About CRT detection: I think, CRT don't detected becouse it must be detected via
i2c+gpio. No? But i2c+gpio broken in many places (includes pieces of dead
i2c+gpio code, newer be reached now). May be it related more to
drivers/i2c/busses/i2c-gpio.ko, even it start and bint to viafb (after some i2c
code fixes), but without results and I don't understand how it must work with
current code and while it not completed, I think there are not too trivial task.
Are you know how CRT/DDC must/may be detected?

Summary: I will think more, then IMHO first I will send separated patches with
this behaviour...

-- 
WBR, Dzianis Kahanovich AKA Denis Kaganovich, http://mahatma.bspu.unibel.by/