[LinuxBIOS] [patch] flashrom: w863xx helpers and w86327hf_gpio42_raise

[Luc Verhaegen]

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.