Re: Atari mach64 support (fwd)

Re: Atari mach64 support (fwd)

[David D. Kilzer]

Random code review (see below).

On Sep 22, 2005, at 7:15 AM, Geert Uytterhoeven wrote:

> ---------- Forwarded message ----------
> Date: Wed, 21 Sep 2005 22:07:01 +0100 (BST)
> From: James Simmons <jsimmons@infradead.org>
> To: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Linux Fbdev development list <linux-fbdev- 
> devel@lists.sourceforge.net>
> Subject: Atari mach64 support
>
>
> It's back. Please give this a try. It does compile but I can't test  
> it.
>
> diff -urN -X /home/jsimmons/dontdiff linus-2.6/drivers/video/aty/ 
> atyfb_base.c fbdev-2.6/drivers/video/aty/atyfb_base.c
> --- linus-2.6/drivers/video/aty/atyfb_base.c    2005-09-13  
> 14:01:40.000000000 -0700
> +++ fbdev-2.6/drivers/video/aty/atyfb_base.c    2005-09-21  
> 13:54:02.000000000 -0700
> [...]
> @@ -2276,18 +2268,20 @@
>              par->pll_limits.mclk = 63;
>      }
>
> -    if (M64_HAS(GTB_DSP)
> -        && (pll_ref_div = aty_ld_pll_ct(PLL_REF_DIV, par))) {
> -        int diff1, diff2;
> -        diff1 = 510 * 14 / pll_ref_div - par->pll_limits.pll_max;
> -        diff2 = 510 * 29 / pll_ref_div - par->pll_limits.pll_max;
> -        if (diff1 < 0)
> -            diff1 = -diff1;
> -        if (diff2 < 0)
> -            diff2 = -diff2;
> -        if (diff2 < diff1) {
> -            par->ref_clk_per = 1000000000000ULL / 29498928;
> -            xtal = "29.498928";
> +    if (M64_HAS(GTB_DSP)) {
> +        u8 pll_ref_div = aty_ld_pll_ct(PLL_REF_DIV, par);
> +        if (pll_ref_dev) {
> +            int diff1, diff2;
> +            diff1 = 510 * 14 / pll_ref_div - par->pll_limits.pll_max;
> +            diff2 = 510 * 29 / pll_ref_div - par->pll_limits.pll_max;
> +            if (diff1 < 0)
> +                diff1 = -diff1;
> +            if (diff2 < 0)
> +                diff2 = -diff2;
> +            if (diff2 < diff1) {
> +                par->ref_clk_per = 1000000000000ULL / 29498928;
> +                xtal = "29.498928";
> +            }
>          }
>      }
>  #endif /* CONFIG_FB_ATY_CT */

Should "if (pll_ref_dev) {" be "if (pll_ref_div) {" to keep the logic  
the same as the old code, or was this a bug fix?

Dave



-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl

Re: Re: Atari mach64 support (fwd)

[James Simmons]

Just code merger. First you had the M64_HAS test then test for 
pll_ref_div. I combined the two test into one test.
Its not needed tho so I broke it into 2 again. I think it is time to send 
this to Andrew.


> Random code review (see below).
> 
> On Sep 22, 2005, at 7:15 AM, Geert Uytterhoeven wrote:
> 
> > ---------- Forwarded message ----------
> > Date: Wed, 21 Sep 2005 22:07:01 +0100 (BST)
> > From: James Simmons <jsimmons@infradead.org>
> > To: Geert Uytterhoeven <geert@linux-m68k.org>
> > Cc: Linux Fbdev development list <linux-fbdev- 
> > devel@lists.sourceforge.net>
> > Subject: Atari mach64 support
> >
> >
> > It's back. Please give this a try. It does compile but I can't test  
> > it.
> >
> > diff -urN -X /home/jsimmons/dontdiff linus-2.6/drivers/video/aty/ 
> > atyfb_base.c fbdev-2.6/drivers/video/aty/atyfb_base.c
> > --- linus-2.6/drivers/video/aty/atyfb_base.c    2005-09-13  
> > 14:01:40.000000000 -0700
> > +++ fbdev-2.6/drivers/video/aty/atyfb_base.c    2005-09-21  
> > 13:54:02.000000000 -0700
> > [...]
> > @@ -2276,18 +2268,20 @@
> >              par->pll_limits.mclk = 63;
> >      }
> >
> > -    if (M64_HAS(GTB_DSP)
> > -        && (pll_ref_div = aty_ld_pll_ct(PLL_REF_DIV, par))) {
> > -        int diff1, diff2;
> > -        diff1 = 510 * 14 / pll_ref_div - par->pll_limits.pll_max;
> > -        diff2 = 510 * 29 / pll_ref_div - par->pll_limits.pll_max;
> > -        if (diff1 < 0)
> > -            diff1 = -diff1;
> > -        if (diff2 < 0)
> > -            diff2 = -diff2;
> > -        if (diff2 < diff1) {
> > -            par->ref_clk_per = 1000000000000ULL / 29498928;
> > -            xtal = "29.498928";
> > +    if (M64_HAS(GTB_DSP)) {
> > +        u8 pll_ref_div = aty_ld_pll_ct(PLL_REF_DIV, par);
> > +        if (pll_ref_dev) {
> > +            int diff1, diff2;
> > +            diff1 = 510 * 14 / pll_ref_div - par->pll_limits.pll_max;
> > +            diff2 = 510 * 29 / pll_ref_div - par->pll_limits.pll_max;
> > +            if (diff1 < 0)
> > +                diff1 = -diff1;
> > +            if (diff2 < 0)
> > +                diff2 = -diff2;
> > +            if (diff2 < diff1) {
> > +                par->ref_clk_per = 1000000000000ULL / 29498928;
> > +                xtal = "29.498928";
> > +            }
> >          }
> >      }
> >  #endif /* CONFIG_FB_ATY_CT */
> 
> Should "if (pll_ref_dev) {" be "if (pll_ref_div) {" to keep the logic  
> the same as the old code, or was this a bug fix?


-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.
Get Certified Today * Register for a JBoss Training Course
Free Certification Exam for All Training Attendees Through End of 2005
Visit http://www.jboss.com/services/certification for more information

Re: Re: Atari mach64 support (fwd)

[Ville Syrjälä]

On Mon, Oct 24, 2005 at 09:42:42PM +0100, James Simmons wrote:
> 
> Just code merger. First you had the M64_HAS test then test for 
> pll_ref_div. I combined the two test into one test.
> Its not needed tho so I broke it into 2 again. I think it is time to send 
> this to Andrew.

He probably meant that you've mistyped pll_ref_div as pll_ref_dev.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/


-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.
Get Certified Today * Register for a JBoss Training Course
Free Certification Exam for All Training Attendees Through End of 2005
Visit http://www.jboss.com/services/certification for more information

Re: Re: Atari mach64 support (fwd)

[James Simmons]

Yeap. Your right. I will send a patch with the correct spelling.


On Tue, 25 Oct 2005, Ville [iso-8859-1] Syrjälä wrote:

> On Mon, Oct 24, 2005 at 09:42:42PM +0100, James Simmons wrote:
> > 
> > Just code merger. First you had the M64_HAS test then test for 
> > pll_ref_div. I combined the two test into one test.
> > Its not needed tho so I broke it into 2 again. I think it is time to send 
> > this to Andrew.
> 
> He probably meant that you've mistyped pll_ref_div as pll_ref_dev.
> 
> -- 
> Ville Syrjälä
> syrjala@sci.fi
> http://www.sci.fi/~syrjala/
> 
> 
> -------------------------------------------------------
> This SF.Net email is sponsored by the JBoss Inc.
> Get Certified Today * Register for a JBoss Training Course
> Free Certification Exam for All Training Attendees Through End of 2005
> Visit http://www.jboss.com/services/certification for more information
> _______________________________________________
> Linux-fbdev-devel mailing list
> Linux-fbdev-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel
> 


-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.
Get Certified Today * Register for a JBoss Training Course
Free Certification Exam for All Training Attendees Through End of 2005
Visit http://www.jboss.com/services/certification for more information