[PATCH v3 01/13] OMAP: DMA: Replace read/write macros with functions

[G, Manjunath Kondaiah]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman at deeprootsystems.com] 
> Sent: Wednesday, November 10, 2010 9:33 PM
> To: G, Manjunath Kondaiah
> Cc: linux-omap at vger.kernel.org; 
> linux-arm-kernel at lists.infradead.org; Cousson, Benoit; 
> Shilimkar, Santosh
> Subject: Re: [PATCH v3 01/13] OMAP: DMA: Replace read/write 
> macros with functions
> 
> "G, Manjunath Kondaiah" <manjugk at ti.com> writes:
> 
> [...]
> 
> >> > +		if (reg > OMAP1_CH_COMMON_START)
> >> > +			__raw_writew(val, dma_base +
> >> > +				(reg_map_omap1[reg] + 
> 0x40 * lch));
> >> > +		else
> >> > +			__raw_writew(val, dma_base + 
> >> reg_map_omap1[reg]);
> >> > +	} else {
> >> > +		if (reg > OMAP2_CH_COMMON_START)
> >> > +			__raw_writel(val, dma_base +
> >> > +				(reg_map_omap2[reg] + 
> 0x60 * lch));
> >> > +		else
> >> > +			__raw_writel(val, dma_base + 
> >> reg_map_omap2[reg]);
> >> > +	}
> >> > +}
> >> 
> >> The register base offset, register array and the stride (offset
> >> between instances: 0x40 or 0x60) are detectable at init time, and
> >> there's no reason to have conditional code for them.  IOW, they
> >> should be init time constants.  Doing so would greatly simply these
> >> functions.  In fact the CH_COMMON_START could also be an init time
> >> constant as well.  So, given the following init_time constants:
> >> dma_ch_common_start, dma_stride, reg_map, the above would 
> be reduced
> >> to something actually worth inlining, for example (not actually
> >> tested):
> >> 
> >> static inline void dma_write(u32 val, int reg, int lch)
> >> {
> >>         u8 stride = (reg > dma_ch_common_start) ? dma_stride : 0;
> >> 
> >>         __raw_writel(val, dma_base + (reg_map[reg] + 
> stride * lch));
> >> }
> >> 
> >> Same applies to dma_read().
> >
> > Thanks for the suggestion. It is taken care along with 
> Tony's comment
> > for handling these read/write's between omap1 and omap2+.
> >
> > Here is code snippet for handling both omap1(includes 16 bit
> > registers) and omap2+ 
> >
> > static inline void dma_write(u32 val, int reg, int lch)
> > {
> >         u8  stride;
> >         u32 offset;
> >
> >         stride = (reg >= dma_common_ch_start) ? dma_stride : 0;
> >         offset = reg_map[reg] + (stride * lch);
> >
> >         if (dma_stride  == 0x40) {
> 
> The use of hard-coded constants still isn't right here.    This is
> bascally the same as a cpu_is_omap1 check.  After you separate out the
> device files, I thought you had separate omap1 and omap2+ versions of
> these, so I don't follow the need for this.

This code will be moved to respective mach-omapx dma files and this 
conditional check vanishes automatically in PATCH 10/13. Since this patch
targets  replacing read/write macros with inline functions, no functionality
changes(except change in logic for handling  16bit registers for omap1) 
are done with new patch hence handling omap1 and omap2+ is 
done this way.

I hope having the conditional check in this interim patch is ok.

-Manjunath