Re: [PATCH v4, for-next] OMAP: DMA: Use some define rather than a hexadecimal constant for LCD register

From: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
To: Tony Lindgren <tony@atomide.com>
Cc: linux-omap@vger.kernel.org
Subject: Re: [PATCH v4, for-next] OMAP: DMA: Use some define rather than a hexadecimal constant for LCD register
Date: Tue, 17 Nov 2009 11:45:53 +0100	[thread overview]
Message-ID: <200911171145.55658.jkrzyszt@tis.icnet.pl> (raw)
In-Reply-To: <20091117012435.GA3684@atomide.com>

Tuesday 17 November 2009 02:24:35 Tony Lindgren napisał(a):
> * Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> [091116 16:38]:
> > Tuesday 17 November 2009 01:16:58 Tony Lindgren napisał(a):
> > > * Janusz Krzysztofik <jkrzyszt@tis.icnet.pl> [091116 15:13]:
> > > > diff -uprN a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
> > > > --- a/arch/arm/plat-omap/dma.c	2009-11-14 23:48:41.000000000 +0100
> > > > +++ b/arch/arm/plat-omap/dma.c	2009-11-16 23:49:30.000000000 +0100
> > > > @@ -36,6 +36,10 @@
> > > >
> > > >  #include <plat/tc.h>
> > > >
> > > > +#ifdef CONFIG_ARCH_OMAP1
> > > > +#include <mach/lcdc.h>
> > > > +#endif
> > > > +
> > > >  #undef DEBUG
> > > >
> > > >  #ifndef CONFIG_ARCH_OMAP1
> > > > @@ -1124,9 +1128,11 @@ int omap_dma_running(void)
> > > >  	 * On OMAP1510, internal LCD controller will start the transfer
> > > >  	 * when it gets enabled, so assume DMA running if LCD enabled.
> > > >  	 */
> > > > +#ifdef CONFIG_ARCH_OMAP1
> > > >  	if (cpu_is_omap1510())
> > > > -		if (omap_readw(0xfffec000 + 0x00) & (1 << 0))
> > > > +		if (omap_readw(OMAP_LCDC_CONTROL) & OMAP_LCDC_CTRL_LCD_EN)
> > > >  			return 1;
> > > > +#endif
> > > >
> > > >  	/* Check if LCD DMA is running */
> > > >  	if (cpu_is_omap16xx())
> > >
> > > Hmm, this is getting complicated... How about just add function to
> > > drivers/video/omap/lcdc.c for something like omap_lcdc_get_status()
> > > or similar?
> >
> > Good idea.

... for a function itself, but not for putting it there, since omapfb, that 
lcdc is a part of, can be build as a module :).

> > > Then you can define that function in some header as:
> > >
> > > #if defined(CONFIG_ARCH_OMAP1) && defined(CONFIG_FB_OMAP)
> > > extern int omap_lcdc_get_status(void);
> > > #else
> > > static inline int omap_lcdc_get_status(void)
> > > {
> > > 	return -ENODEV;
> > > }
> > > #endif
> > >
> > > That way the defines can stay where they are and you don't need the
> > > ugly ifdefs.
> >
> > Sorry if my question seems stupid, but assuming the function can be as
> > simple as:
> >
> > int omap_lcdc_get_status(void)
> > {
> > 	return omap_readw(OMAP_LCDC_CONTROL) & OMAP_LCDC_CTRL_LCD_EN;
> > }
> >
> > could't it be defined as static inline too and put inside lcdc.h?
>
> Sure, but then you need to move the defines again, no?

I can see 3 options:

1. While keeping those hex constants as they are for now, solve the issue 
during refactoring arch/arm/plat-omap/dma.c for a separate 
arch/arm/mach-omap1/lcd_dma.c, as Paul suggested (may take some time). Since 
the register test would be put there, some headers will have to be moved out 
of drivers/video/omap/lcdc.c anyway.

2. Temporarily apply v2 of the patch. It compiles cleanly since defines are 
put into <plat/omapfb.h> that exists for all OMAP classes. Then, clean up 
things while doing #1. I'm not sure if that temporary solution could result 
in one more conflict with DSS2.

3. Based on v4 and your suggestion for a function, work out an intermediate 
solution as soon as possible. I would appreciate any further suggestions on 
how that could be arranged.

If there are no better options, please suggest which of those three you would 
like me to take.

Thanks,
Janusz
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html