On Mon, Oct 4, 2010 at 6:14 PM, ron minnich <rminn...@gmail.com> wrote: > On Sun, Oct 3, 2010 at 4:10 PM, Peter Stuge <pe...@stuge.se> wrote: >> Rudolf just found a bug in the sb700 code: >> >> u32 dword; >> .. >> dword = pci_read_config8(dev, 0x64); >> dword |= 1 << 10; >> pci_write_config8(dev, 0x64, dword); > > > Actually, I don't even have a problem with this construct. Why? > Because it's in just about every kernel I've ever worked with. It's a > common technique. Sure, in this case, it's a trivially simple change > and you can write a function for it. But, as soon as things get more > complex, with multiline tests and bit sets, you can't use the > functions, and we're back to the same type of coding. Then we end up > with mixed idioms. > > It's a good idea for our code base to adhere to such common idioms. It > makes for an easier time for people coming in from, e.g., Linux. I > don't find the functions easier. > > Also, as pointed out, the proposed functions solve one special case. > Better to fix the real problem, which is that the compiler can tell us > about this type of error but we're not letting it. That will fix all > such problems, not just the pci subsystem. > > Just another penny or so. > > ron
I'm fully in agreement with Ron here. Lets fix whatever's broken so we can let the compiler tell us when we make foolish mistakes, rather then writing some mess that's sure to cause some WTFs down the road. This is *one* instance in which someone used an 8-bit instead of a 16-bit function by mistake. Lets not go nuts trying to idiot-proof things. Instead of hacking away at all the pci functions, and almost definitely causing some unintended breakage, can't we just pay a little more attention during reviews? -Corey -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot
