[PATCH] PCI PHB unit id detection interface change

[linas at austin.ibm.com]

Hi Olof,

Thanks,

On Mon, Jun 14, 2004 at 02:57:59PM -0500, Olof Johansson wrote:
> >
> >+void init_pci_config_tokens (void)
> >
> Should this be __init?

Yes, I missed that.

> >+	if (NULL == phb->parent) return 0;
> >
> It's common practice in the Linux kernel to do tests in form (<variable>
> == <constant>). Good compilers should warn about missing equal signs
> anyway. :)

Yes, 'should' being the operative word in that sentance :).
I'd like to convince the world to change its coding style; no luck so far...

> >+	if (NULL == buid_vals) return 0;
> >
> Break if statements into two lines, please, all 4 above

Yes, this is another one that bugs me.  I know this is not the kernel
coding style but the style I like is "use {} if its more than one line".
I have debugged code that looked like this

if (x)
   if (y)
      p;
 else
    q;

and have come to beleieve that the "one line or use {}" rule is the best.

I have also seen people that took code that worked:

if (x)
  p;

And change it to the following

if (x)
  q;
  p;

when the reallly meant

if (x) {
  q;
  p;
}

and so I've come to beleive that the 'one-line-rule' is better.

Of course, I want to say that kernel programmers are usually
smarter than the rest, but, if that were the really the case,
we wouldn't need tools like 'sparse', right?


> Take out space between function name and ()

Done.

> > 	/* Enable EEH for all adapters.  Note that eeh requires buid's */
> >+	init_pci_config_tokens();
> >
>
> Why do you need this here? Is EEH code ever called before
> find_and_init_phbs?

yes, in fact, its called immediately before.  I think the idea is that
one must not access any pci spaces until eeh is enabled first.

> If so, can't you make the init_pci_config_tokens()
> an early_initcall() to avoid calling it twice?

I called it twice out of a sense of paranoia.  I'll fix that.
What is early_initcall?

--linas

** Sent via the linuxppc64-dev mail list. See http://lists.linuxppc.org/