[PATCH] Radeon driver fixes

[Michel Dänzer]

On Fri, 2004-11-12 at 14:20 +1100, Benjamin Herrenschmidt wrote:
> 
> This patch fixes various issues with the Radeon driver in current Xorg
> and add a few options useful for PPC owners or other platforms who don't
> have the x86 BIOS image giving all the details about the panel, clocks,
> etc...

Basically looks good to me, I only have a couple of rather cosmetic
comments:

> 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.

> @@ -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.

> +	xf86DrvMsg(pScrn->scrnIndex, X_INFO, "Loading VGA module...\n");	

This one seems rather redundant?

> @@ -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?

> +    /* 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;


-- 
Earthling Michel Dänzer      |     Debian (powerpc), X and DRI developer
Libre software enthusiast    |   http://svcs.affero.net/rm.php?r=daenzer