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

Sam Ravnborg
2008-11-29T18:00:29Z

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