Messages in this thread | | | Date | Mon, 22 Nov 2010 07:48:06 -0700 | From | Jonathan Corbet <> | Subject | Re: [PATCH v2] viafb: I2C/DDC LCD detection for VIA framebuffer |
| |
On Mon, 22 Nov 2010 16:23:21 +0200 Dzianis Kahanovich <mahatma@bspu.unibel.by> wrote:
> I2C/DDC LCD detection for VIA framebuffer > > Adding legacy I2C/DDC support for VIA framebuffer, used to fix LCD > panel size and (if "via_active_dev" is empty default) force LCD > detection. This solving at least defaults on Chrome9 video > on HP mini 2133 notebook, but must be good in other cases, > include double-LCD. Also forcing viafb_active_dev to NULL as bugfix.
As a general note, it's good to Cc the authors and maintainers of the code you're changing. Florian has been acting as maintainer of the viafb subsystem; I've added him to the list.
> > Signed-off-by: Dzianis Kahanovich <mahatma@eu.by> > --- > diff -pruN a/drivers/video/via/via-core.c b/drivers/video/via/via-core.c > --- a/drivers/video/via/via-core.c 2010-11-22 13:11:44.000000000 +0200 > +++ b/drivers/video/via/via-core.c 2010-11-22 13:59:07.000000000 +0200 > @@ -2,6 +2,7 @@ > * Copyright 1998-2009 VIA Technologies, Inc. All Rights Reserved. > * Copyright 2001-2008 S3 Graphics, Inc. All Rights Reserved. > * Copyright 2009 Jonathan Corbet <corbet@lwn.net> > + * Copyright 2010 Dzianis Kahanovich <mahatma@eu.by> > */ > > /* > @@ -16,6 +17,8 @@ > #include <linux/interrupt.h> > #include <linux/platform_device.h> > > +extern char *viafb_active_dev; > + > /* > * The default port config. > */ > @@ -576,6 +579,56 @@ static void via_teardown_subdevs(void) > } > > +static void via_i2c_detect(void) > +{ > + u8 *edid; > + struct fb_var_screeninfo var; > + int i, n = 0; > + struct i2c_adapter *adapter; > + > + for (i = 0; i < VIAFB_NUM_PORTS; i++) { > + adapter = viafb_find_i2c_adapter(i); > + if (!adapter || !adapter->algo_data || > + !(edid = fb_ddc_read(adapter))) > + continue;
What happens if what's there is not an LCD, or if there's nothing there? Might this add significant boot delays?
> + if (fb_parse_edid(edid, &var)) > + goto free_edid; > + if (n) {
What's "n"? Not the most descriptive variable name ever. I take it "n" is the number of monitors you've found so far?
> + fb_edid_to_monspecs(edid, &viafbinfo1->monspecs); > + viaparinfo->lvds_setting_info2->lcd_panel_hres = > + var.xres; > + viaparinfo->lvds_setting_info2->lcd_panel_vres = > + var.yres;
There are times when trying to stick to the 80-column limit just isn't worthwhile.
> + if (!viafb_active_dev) { > + viafb_SAMM_ON = STATE_ON; > + if (viafbinfo1->monspecs.input & FB_DISP_DDI) > + viafb_LCD2_ON = STATE_ON; > + } > + } else { > + fb_edid_to_monspecs(edid, &viafbinfo->monspecs); > + viaparinfo->lvds_setting_info->lcd_panel_hres = > + var.xres; > + viaparinfo->lvds_setting_info->lcd_panel_vres = > + var.yres; > + if (!viafb_active_dev) { > + viafb_DVI_ON = > + viaparinfo->tmds_setting_info-> > + max_hres ? STATE_ON : STATE_OFF; > + if (viafbinfo->monspecs.input & FB_DISP_DDI) { > + viafb_DeviceStatus = LCD_Device; > + viafb_primary_dev = LCD_Device; > + viafb_LCD_ON = STATE_ON; > + } > + } > + } > + n++; > +free_edid: > + kfree(edid); > + if (n > 1) > + break; > + } > +} > + > > static int __devinit via_pci_probe(struct pci_dev *pdev, > const struct pci_device_id *ent) > { > @@ -607,6 +660,7 @@ static int __devinit via_pci_probe(struc > ret = via_fb_pci_probe(&global_dev); > if (ret) > goto out_subdevs; > + via_i2c_detect(); > return 0; > > out_subdevs: > diff -pruN a/drivers/video/via/viafbdev.c b/drivers/video/via/viafbdev.c > --- a/drivers/video/via/viafbdev.c 2010-11-22 13:11:44.000000000 +0200 > +++ b/drivers/video/via/viafbdev.c 2010-11-22 13:59:07.000000000 +0200 > @@ -43,7 +43,7 @@ static int viafb_second_size; > static int viafb_accel = 1; > > /* Added for specifying active devices.*/ > -char *viafb_active_dev; > +char *viafb_active_dev = NULL;
This is a file-static variable, it will be NULL by default.
More general comments:
- I find the code hard to read. I guess it will fit in well with the rest of viafb! :)
- This is framebuffer-specific stuff which shouldn't be in via-core.c. The fact that viafb_active_dev is defined in viafbdev.c is a hint; I'd be inclined to put this kind of detection code there.
- It would be interesting to see how other via-based systems work with this patch; I won't be able to do that in the near future, so I'm adding Daniel (OLPC) to the Cc list.
Thanks,
jon
| |