[PATCH] Radeon driver fixes

[Benjamin Herrenschmidt]

> 
> > diff -urN xc.orig/programs/Xserver/hw/xfree86/drivers/ati/radeon_driver.c xc/programs/Xserver/hw/xfree86/drivers/ati/radeon_driver.c
> > --- xc.orig/programs/Xserver/hw/xfree86/drivers/ati/radeon_driver.c	2004-11-11 11:56:59.000000000 +1100
> > +++ xc/programs/Xserver/hw/xfree86/drivers/ati/radeon_driver.c	2004-11-12 13:56:09.000000000 +1100
> 
> [...]
> 
> > @@ -223,6 +226,9 @@
> >  #endif
> >      { OPTION_SHOWCACHE,      "ShowCache",        OPTV_BOOLEAN, {0}, FALSE },
> >      { OPTION_DYNAMIC_CLOCKS, "DynamicClocks",    OPTV_BOOLEAN, {0}, FALSE },
> > +    { OPTION_NO_VGA,         "NoVGA",            OPTV_BOOLEAN, {0}, FALSE },
> 
> An option name that starts with "No" (or any other negative term, for
> that matter) is a no no IMHO. :) The option parser handles this as well
> as a number of other prefixes and other things.

Blame my inexperience with X option parser... you mean that if I call
the option "SaveRestoreVGA" instead with a default to TRUE, and put
Option "NoSaveRestorVGA" in my config file, the option parser will deal
with it ?
 
> > @@ -4237,6 +4331,24 @@
> >      memcpy(info->Options, RADEONOptions, sizeof(RADEONOptions));
> >      xf86ProcessOptions(pScrn->scrnIndex, pScrn->options, info->Options);
> >  
> > +    info->NoVGA = TRUE;
> > +    if (!xf86ReturnOptValBool(info->Options, OPTION_NO_VGA, FALSE)) {
> 
> Please consider using xf86GetOptValBool() instead of
> xf86ReturnOptValBool() and setting the second argument to xf86DrvMsg()
> according to whether something is the default or set by the
> configuration or not.

Hrm... care to rephrase ? Not sure I got what you meant about the
default ... 

> > +	xf86DrvMsg(pScrn->scrnIndex, X_INFO, "Loading VGA module...\n");	
> 
> This one seems rather redundant?

Yah, debug stuff, wanted to make sure it got there, I can remove it.

> > @@ -6576,6 +6705,12 @@
> >  
> >      save->crtc2_offset      = 0;
> >      save->crtc2_offset_cntl = INREG(RADEON_CRTC2_OFFSET_CNTL);
> 
> This should be removed in favour of the assignment below?

Yup, probably. 

> > +    /* We must make sure Tiling is disabled. It seem all other fancy
> > +     * options in there can be safely disabled too
> > +     */
> > +    save->crtc2_offset_cntl = 0;
> 
> 
-- 
Benjamin Herrenschmidt <benh at kernel.crashing.org>