lkml.org 
[lkml]   [2008]   [Oct]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] via-agp.c fixed compilation error and warnings for 2.6.26
On Tue, Oct 21, 2008 at 04:57:13AM +0100, Dave Airlie wrote:
> > > - printk(KERN_ERR PFX "Unknown aperture size from AGP bridge (0x%x)\n",
> > > temp);
> > > + printk(KERN_ERR PFX "Unknown aperture size from AGP
> > > + bridge (0x%x)\n", temp);
>
> Uglier code.

Not just that, but it won;t even compile with a recent compiler. You'd
need to do it as:

printk(KERN_ERR PFX "Unknown aperture size from AGP "
"bridge (0x%x)\n", temp);

and that would seem like not much of a win. What might be a win would
be:
dev_err(&agp_bridge->dev->dev, "Unknown AGP aperture size 0x%x\n",
temp);

> > > if (temp == values[i].size_value) {
> > > agp_bridge->previous_size =
> > > - agp_bridge->current_size = (void *) (values + i);
> > > + agp_bridge->current_size =
> > > + (void *) (values + i);
>
> arguably uglier code.

No argument about it ... it's uglier. The (void *) cast is unnecessary.

What I'd do to this function is:

for (i = 0; i < agp_bridge->driver->num_aperture_sizes; i++) {
if (temp != values[i].size_value)
continue;

agp_bridge->previous_size = agp_bridge->current_size =
values + i;
agp_bridge->aperture_size_idx = i;
return values[i].size;
}

dev_err(&agp_bridge->dev->dev, "Unknown AGP aperture size 0x%x\n",
temp);
return 0;
}


> > > @@ -142,11 +144,12 @@ static int via_configure_agp3(void)
> > >
> > > /* 1. Enable GTLB in RX90<7>, all AGP aperture access needs to fetch
> > > * translation table first.
> > > - * 2. Enable AGP aperture in RX91<0>. This bit controls the enabling
> > > of the
> > > - * graphics AGP aperture for the AGP3.0 port.
> > > + * 2. Enable AGP aperture in RX91<0>. This bit controls the
> > > + * enabling of the graphics AGP aperture for the AGP3.0 port.
>
> this is okay.

Except the missing spaces between the '*' and 'enabling'.

> > > */
> > > pci_read_config_dword(agp_bridge->dev, VIA_AGP3_GARTCTRL, &temp);
> > > - pci_write_config_dword(agp_bridge->dev, VIA_AGP3_GARTCTRL, temp |
> > > (3<<7));
> > > + pci_write_config_dword(agp_bridge->dev, VIA_AGP3_GARTCTRL,
> > > + temp | (3<<7));
>
> this one is probably okay.

I'd add some more spacing:

+ temp | (3 << 7));

The driver seems to suffer from a lack of spaces around << throughout.
And most of those << should probably be defines somewhere ...

--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."


\
 
 \ /
  Last update: 2008-10-21 06:27    [W:3.598 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site