Re: [PATCH 1/1] edac x38: new MC driver module

[Sam Ravnborg <sam@ravnborg.org>]

On Sat, Nov 29, 2008 at 10:24:49PM +0900, Hitoshi Mitake wrote:
> On Sat, 29 Nov 2008 11:52:29 +0100
> Sam Ravnborg <sam@ravnborg.org> wrote:
> 
> > > 
> > > But this is old way. ARCH_HAS_READQ and ARCH_HAS_WRITEQ are new ways
> > > to determine existence of readq/writeq. Drivers which use readq/writeq should
> > > depend on these values in their Kconfig file.
> > 
> > If we look at arch/x86/Kconfig we see:
> > ### Arch settings
> > config X86
> >         def_bool y
> >         select HAVE_AOUT if X86_32
> >         select HAVE_UNSTABLE_SCHED_CLOCK
> >         select HAVE_IDE
> >         select HAVE_OPROFILE
> >         select HAVE_IOREMAP_PROT
> >         select HAVE_KPROBES
> >         select ARCH_WANT_OPTIONAL_GPIOLIB
> > 	...
> > 
> > So the normal syntax here is "HAVE_XXX_XXX" - not ARCH_HAS_XXX_XXX
> > 
> > If you update your patch please use this syntax,
> > and locate the select under X86 - not under the 32/64 entries.
> 
> Thanks for your notification. I didn't notice that syntax rule.
> 
> > 
> > But I do not see why adding these in the first place.
> > 
> 
> Andrew Morton told that drivers which need readq/writeq should use ones of kernel,
> and if architecture part of kernel does not provide readq/writeq, drivers should be disabled.
> 
> This is Andrew's mail:
> http://marc.info/?l=linux-kernel&m=122625885124798&w=2
> ===> Quote:
> 
> #ifdef readq
> 
> Is a suitable way of determining whether the architecture implements
> readq and writeq.  It isn't pretty, but it will suffice.
> 
> A problem with it is that drivers will then do
> 
> #ifndef readq
> <provide a local implementation here>
> #endif
> 
> which rather sucks - we don't want lots of little private readq/writeq
> implementations all over the tree.
> 
> Perhaps it would be better to have a CONFIG_ARCH_HAS_READQ and to then
> disable these drivers on the architectures which don't provide
> readq/writeq support.
> 
> <====
I see both rationales and you combine them in your patch - OK.

And the reason why you cannot just add this to
include/linux/io.h is that not all architectures
provide a readl()/writel() I assume.

Feel free to add my Acked-by: to the patch.

	Sam