Re: [coreboot] patch: lxirq understander

[Uwe Hermann]

On Wed, Jul 09, 2008 at 09:10:26AM -0700, ron minnich wrote:
> Can I get feedback on this one? It's the lx irq understander. I'd like
> to get this in so others can give it a try.
>
> I want to finish this as I need to write an LX NB understander, in an
> attempt to do a first-principals pass at 'what the hell is wrong with
> the dbe62'.
>
> Signed-off-by: Ronald G. Minnich
> Index: util/lxirq/lxirq.c
>
> --- util/lxirq/lxirq.c	(revision 0)
> +++ util/lxirq/lxirq.c	(revision 0)
> @@ -0,0 +1,252 @@
> +/*
> This file is part of the flashrom project.

s/flashrom/lxirq/
> Copyright (C) 2000 Silicon Integrated System Corporation
> Copyright (C) 2004 Tyan Corp
> Copyright (C) 2005-2007 coresystems GmbH

You probably copied some file from flashrom but nothing that's left
looks relevant I think, so this is

Copyright (C) 2008 Ronald G. Minnich
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; either version 2 of the License, or
> (at your option) any later version.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */

[...]
> +struct pci_access *pacc;	/* For board and chipset_enable */
> +int verbose = 0;
> +struct pci_dev *cs5536_find(void)
> +{
> struct pci_dev *temp;
> struct pci_filter filter;
> pci_filter_init(NULL, &filter);
> filter.vendor = 0x1022;
> filter.device = 0x2090;

Should probably be #defines a la pci_ids.h. I do realize that this is
a quick solution, but making it more readable is still a good idea.
However, is it intended for longer-term, general-prupose usage or
just a temporary hack for finding a specific bug? I.e. do we really want
to commit it and will we need it ever again after the bug was found?
> for (temp = pacc->devices; temp; temp = temp->next)
> if (pci_filter_match(&filter, temp))
> 	return temp;
> return NULL;
>
> +}
>
> +void usage(const char *name)
> +{
> printf("usage: %s\n", name);
> exit(1);
>
> +}
>
> +void print_version(void)
> +{
> printf("flashrom r%s\n", LXIRQ_VERSION);
>
> +}

Incorrect, can be fixed or dropped.
> Index: util/lxirq/Makefile
>
> --- util/lxirq/Makefile	(revision 0)
> +++ util/lxirq/Makefile	(revision 0)

Lots of unneeded / wrong stuff in the Makefile (leftovers from flashrom).

Uwe.