On Fri, May 04, 2007 at 04:39:30AM +0200, Peter Stuge wrote:
On Fri, May 04, 2007 at 12:50:47AM +0200, Luc Verhaegen wrote:
Add WinBond Super IO helpers.
Looks very good except for one small thing..
+wbsio_mask(unsigned char index, unsigned char data,
Could this be called wbsio_clear() instead, and isn't a _set() needed too?
You can take this sort of thing very far, but a _mask function like here is just far enough to hide most of the complexity and bring out the meaning of the code.
A single bitwise clear is: wbsio_mask(reg, 0x00, 0x10);
A single bitwise set is: wbsio_mask(reg, 0x10, 0x10);
But you also often have cases where you have to set part of a byte to a given value.
Let's set VGA Horizontal Sync End, the lower 5bits of CR05: vga_cr_mask(0x05, mode->HSyncEnd, 0x1F);
With a small bit of practice, _mask becomes a very versatile little function.
The difference between this: wbsio_mask(reg, 0x10, 0x10); and wbsio_set(reg, 0x10); is minimal compared to the difference between _mask and whatever code it replaced already.
You also encourage confusion between _set and _write. If you were a novice, would you see a difference between: wbsio_write(reg, 0x10); and: wbsio_set(reg, 0x10);
Also:
One argument, one return value. WB00 = wbsio_read(0x00);
versus two arguments: wbsio_write(0x00, 0x50);
versus three arguments: wbsio_mask(0x00, 0x50, 0xF0);
It is very hard to mix these up. After a small adjustment period, you can tell at a very superficial glance what is going on. You spend most of your time looking at the bits being written not at the function being called.
Set and Clear seem like good ideas, but for a minimal gain they do muddle things up.
Luc Verhaegen.