[flashrom] [PATCH] Boards: Unify all intel ICH GPIO raising.

[Luc Verhaegen]

On Fri, Oct 23, 2009 at 01:46:52AM +0200, Carl-Daniel Hailfinger wrote:
> On 22.10.2009 23:44, Uwe Hermann wrote:
> > On Thu, Oct 22, 2009 at 03:16:16PM +0200, Luc Verhaegen wrote:
> >
> > I agree we should drop the totally useless 0x48 register index, 0x0c
> > offset, and 0xffc0 bitmask from the function prototype.
> >   
> 
> Absolutely.
> 
> 
> > Also, IMHO "name" is pretty useless, it's on my TODO list to drop that anyway.
> >   
> 
> Indeed.
> 
> 
> > However, I very much think that we _do_ want the PCI IDs to stay in there.
> >
> > Example call would look like this:
> >
> >   ich_gpio_raise(name, 0x8086, 0x27B8, 34);
> >
> > I'm fine with also dropping 0x8086 (PCI vendor ID) as that should always
> > be the same for the ich_* function. If we later also drop "name" this
> > becomes:
> >
> >   ich_gpio_raise(0x27B8, 34);
> >
> > Which is perfect.
> >   
> 
> Hm yes. It is a bit ugly in the case of multiple GPIOs per board, but
> other than that... please see my mail from a few minutes ago for more
> commentary.
> 
> 
> >> Or even this, which still does not make me comfortable, and which 
> >> requires the big switch statement in the ich_gpio_raise function anyway.
> >>
> >> {
> >>         int ret;
> >>         ret = ich_gpio_raise(name, 0x27B8, 34);
> >>         if (!ret)
> >>                 ret = ich_gpio_raise(name, 0x27B8, 35);
> >>
> >>         return ret;     
> >> }
> >>     
> >
> > This is perfectly fine if you ask me. Many board enables only raise
> > one pin and thus are as simple as:
> >
> >   {
> >   	return ich_gpio_raise(name, 0x27B8, 34);
> >   }
> >
> > Which I like a lot. And the above example is very nice and readable,too.
> >   
> 
> Well yes.
> 
> 
> >> Once you have seen a few of those pci dev finding things, and they all 
> >> get copy/pasted around anyway, you quickly see what is important to 
> >> them, and they are all very formulaic anyway. You learn to ignore the 
> >> rest.
> >>     
> >
> > No need to copy any of it around, that's just useless code duplication.
> > Just put them in the ich_gpio_raise() function, they're fine in there.
> >   
> 
> We should strive to keep the board enables
> 1. short and
> 2. easy to understand.
> 
> The old code clearly fails #2, while some of the new proposals fail #1.
> 
> Apart from that, this discussion really helps us define our goals for
> the future.
> 
> Regards,
> Carl-Daniel

As said before, do not abstract away too much, then people will end up 
working around the abstraction later on when they find they need to.

The dev finding can be improved, but this is not the sort of improvement 
that should alter the behaviour of the gpio setting itself, and therefor 
is of less importance than the harder to test things this patch does.

Luc Verhaegen.