Messages in this thread Patch in this message | | | Date | Mon, 20 Oct 2008 22:23:54 -0600 | From | Matthew Wilcox <> | Subject | Re: [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."
| |