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." -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html