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

[Luc Verhaegen]

On Thu, Oct 22, 2009 at 11:44:24PM +0200, Uwe Hermann wrote:
> 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.

People will naturally add a comment here with the chipset name. If they 
don't, then people who come in afterwards will want to see such a 
comment.

> > 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 do not agree.

One issue with the "stick the discovery of the pci device in the gpio 
set routine" is the multiple gpio pin poking. Another is when you need 
to touch some other bits of that same pci device.

It is also the pci device finding which can fail on a badly matched 
board here (or some programming issue which we should not encounter if 
we have board enables tested). So intel_ich_gpio_raise itself, 
later on, should not fail anymore.

Getting the dev, and then working from there is atomic enough, without 
overly abstracting or keeping things.

Also, retrieving the dev, can be made a tiny bit cleaner, at a later 
stage, which will then be uniform for all, even the non-ich board 
enables. Then you really can apply a more general formula.

If you no longer have to pass the name, then you have the situation 
where people put comments in there which state the name, so that is not 
an improvement either.

Let's get the bugs out of this patch, and then worry about the looks of 
it in further patches.

Luc Verhaegen.