At Fri, 16 Mar 2012 16:18:03 -0700, Keith Packard wrote: > > <#part sign=pgpmime> > On Fri, 16 Mar 2012 22:41:12 +0100, Takashi Iwai <tiwai at suse.de> wrote: > > > +/* read the initial LVDS register value for the given panel mode */ > > +static unsigned int get_lvds_reg_val(const struct bdb_header *bdb, > > + const struct bdb_lvds_lfp_data_ptrs *ptrs, > > + int index, > > + struct drm_display_mode *mode) > > To follow the style of intel_bios.c, I think it would make sense to have > the function: > > static const struct lvds_dvo_timing * > get_lvds_fp_timing(const struct bdb_lvds_lfp_data *lvds_lfp_data, > const struct bdb_lvds_lfp_data_ptrs *lvds_lfp_data_ptrs, > int index) > > then use the results of this in parse_lfp_panel_data, instead of putting > the whole computation into this new function. Well, the LVDS reg data isn't in lvds_dvo_timing but in lvds_fp_timing, thus you need to look at a different entry in anyway. > I'd also like to see this code only use the BDB value when the LVDS is > disabled at startup time; otherwise, we'll be changing the behavior for > all LVDS users, and as BIOS tables are notoriously unreliable, I fear > that we'll cause a lot more problems than we solve. I skipped it to simplify the code, but that'd be safer, indeed. (Though, the chance of getting a wrong value is fairly small since the function verifies whether the resolution of the entry matches with the given mode, too.) thanks, Takashi
