Date: Mon, 2 Jun 2008 19:35:27 -0700
From: Arjan van de Ven <arjan@infradead.org>
To: Robert Richter <robert.richter@amd.com>
Cc: Yinghai Lu <yhlu.kernel@gmail.com>, Thomas Gleixner <tglx@linutronix.de>,
       Ingo Molnar <mingo@elte.hu>, LKML <linux-kernel@vger.kernel.org>,
       Andi Kleen <andi@firstfloor.org>
Subject: Re: [PATCH] x86: Add PCI extended config space access for AMD
 Barcelona
Message-ID: <20080602193527.722fbbce@infradead.org>
In-Reply-To: <20080602141928.GH23679@erda.amd.com>
References: <20080526180616.GB2752@erda.amd.com>
	<20080528165813.8D8E5814C@erda.amd.com>
	<20080528120253.26fcdb9d@infradead.org>
	<20080602141928.GH23679@erda.amd.com>
Organization: Intel
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1811
Lines: 46

On Mon, 2 Jun 2008 16:19:29 +0200
Robert Richter <robert.richter@amd.com> wrote:

> Arjan,
> 
> On 28.05.08 12:02:53, Arjan van de Ven wrote:
> > Comment 1:
> > Can we make the 256/4096 thing conditional on actually having the
> > feature somehow? (while not making the code TOO ugly)
> 
> In the first version I had 2 functions also. The patch have had lots
> of duplicate code or inline functions. Since the conditional check is
> already in raw_pci_* I decided to not implement an additional check
> and use only one function.

ok fair enough.

> 
> > Comment 2: 
> > The cpu_has_XXX is a bit dubious; while it's dependent on your cpu
> > model right now, I'm a bit hesitant to consider a PCI feature
> > something that belongs in the cpu_has_XXX namespace. (Yes I know
> > PCI is moving into the cpu package, but on a logical level it seems
> > just the wrong place).
> > Do we need a platform_has_XXX namespace for things like this?
> 
> An alternative implementation would be here to use a check something
> like pci_probe & PCI_HAS_EXT_CFG. If needed, I will send an updated
> patch.

I kind of prefer this, since logically this is a PCI not a CPU property.
Would you mind doing this ?
(not that your current patch is wrong, it's just nicer to keep CPU
stuff with the CPU and PCI stuff with PCI :)
(This wouldn't need to stop inclusion of your current patch, it can
just be an incremental cleanup)

-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/