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

[Uwe Hermann]

On Thu, Oct 22, 2009 at 03:16:16PM +0200, Luc Verhaegen wrote:
> > > Ok, big change here which will require several of the people on the list 
> > > below to test and Ack this patch. But one has to admit; the result is 
> > > worth it.

Yes, I think so too, but see below for one change request.

Will test the patch on ASUS P4B266 soon.


> > Besides that,
> > duplicating pci_dev_find in every board function strikes me as something
> > that was better in the old code.

Ack.

  
> I prefer to see this:
> 
> {
> 	struct pci_dev *dev;
> 
> 	dev = pci_dev_find(0x8086, 0x27b8);	/* Intel ICH7 LPC */
> 	if (!dev) {
> 		fprintf(stderr, "\nERROR: Intel ICH7 LPC bridge not found.\n");
> 		return -1;
> 	}
> 
> 	ich_gpio_raise(dev, 34); /* #TBL */
> 	ich_gpio_raise(dev, 35); /* #WP */
> 
> 	return 0;
> }
> 
> Over the completely impossible:
> 
> {
> 	int ret;
>  	ret = ich_gpio_raise(name, 0x8086, 0x27B8, 0x48, 0x0c, 0xffc0, 34);
> 	if (!ret)
> 		ret = ich_gpio_raise(name, 0x8086, 0x27B8, 0x48, 0x0c, 0xffc0, 35);
> 
> 	return ret;
> }

I agree we should drop the totally useless 0x48 register index, 0x0c
offset, and 0xffc0 bitmask from the function prototype.
Also, IMHO "name" is pretty useless, it's on my TODO list to drop that anyway.

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.


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


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


I'll report back on the P4B266 status when I tested that, but I don't
expect breakage, the code looks good!


Thanks, Uwe.
-- 
http://www.hermann-uwe.de  | http://www.randomprojects.org
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org