Re: [PATCH 10/20] drm: move __OS_HAS_AGP into drm_agpsupport.h

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Aug 29, 2014 at 02:03:12PM +0200, Thierry Reding wrote:
> On Fri, Aug 29, 2014 at 12:12:36PM +0200, David Herrmann wrote:
> > With drm_memory.h gone, there is no header left that uses __OS_HAS_AGP.
> > Move it into drm_agpsupport.h (which is itself included from drmP.h) to
> > hide it harder from public eyes.
> > 
> > Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> > ---
> >  include/drm/drmP.h           | 2 --
> >  include/drm/drm_agpsupport.h | 3 +++
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index 294f7da..c6f337c 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -67,8 +67,6 @@
> >  
> >  #include <linux/idr.h>
> >  
> > -#define __OS_HAS_AGP (defined(CONFIG_AGP) || (defined(CONFIG_AGP_MODULE) && defined(MODULE)))
> > -
> >  struct module;
> >  
> >  struct drm_file;
> > diff --git a/include/drm/drm_agpsupport.h b/include/drm/drm_agpsupport.h
> > index 3bebeb4..4f1724c 100644
> > --- a/include/drm/drm_agpsupport.h
> > +++ b/include/drm/drm_agpsupport.h
> > @@ -8,6 +8,9 @@
> >  #include <linux/agp_backend.h>
> >  #include <drm/drmP.h>
> >  
> > +#define __OS_HAS_AGP (defined(CONFIG_AGP) || (defined(CONFIG_AGP_MODULE) && \
> > +					      defined(MODULE)))
> 
> I'm not really sure what the intent was of the final defined(MODULE).
> Surely the fact whether a driver is being built as a module or not does
> not influence whether or not the OS supports AGP.

I think this was to make sure agp drivers are loaded before the drm
drivers. But since ages we can build the different agp drivers
indidivudally as modules, so this stopped making any sense at all.

> And if this is merely meant to make sure that drivers that are built-in
> don't break to build if AGP is a module, then that should be a job for
> Kconfig rather than some macro.
> 
> So I think the above could simply become:
> 
> 	#define __OS_HAS_AGP IS_ENABLED(CONFIG_AGP)
> 
> And once we have that I think we could even easily get rid of the custom
> __OS_HAS_AGP macro and use IS_ENABLED(CONFIG_AGP) directly.

Yeah, I think a simple IS_ENABLED(CONFIG_AGP) sould be good enough.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch