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

[Peter Stuge]

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?

//Peter

[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.

[Peter Stuge]

On Fri, May 04, 2007 at 06:10:29AM +0200, Luc Verhaegen wrote:
> You can take this sort of thing very far
> A single bitwise clear is:
>    wbsio_mask(reg, 0x00, 0x10);
>
> A single bitwise set is:
>    wbsio_mask(reg, 0x10, 0x10);

Sorry about not looking at the code properly. I should go do
something else now until I have time to do things properly
around here.

I assumed _mask was _clear and did not realize it could be used both
for set and clear. The code is fine. I'd wish for a more descriptive
but still consice name but can't come up with one.

Committed r2627.

//Peter

[Luc Verhaegen]

On Fri, May 04, 2007 at 06:47:14AM +0200, Peter Stuge wrote:
> On Fri, May 04, 2007 at 06:10:29AM +0200, Luc Verhaegen wrote:
> > You can take this sort of thing very far
> > A single bitwise clear is:
> >    wbsio_mask(reg, 0x00, 0x10);
> >
> > A single bitwise set is:
> >    wbsio_mask(reg, 0x10, 0x10);
>
> Sorry about not looking at the code properly. I should go do
> something else now until I have time to do things properly
> around here.
>
> I assumed _mask was _clear and did not realize it could be used both
> for set and clear. The code is fine. I'd wish for a more descriptive
> but still consice name but can't come up with one.

I've used these things for at least three years now, i too can't come up 
with anything better.
> Committed r2627.

Thanks :)

Luc Verhaegen.