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

[Kevin Hilman <khilman@deeprootsystems.com>]

"G, Manjunath Kondaiah" <manjugk@ti.com> writes:

>> -----Original Message-----
>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com] 
>> Sent: Wednesday, November 10, 2010 9:33 PM
>> To: G, Manjunath Kondaiah
>> Cc: linux-omap@vger.kernel.org; 
>> linux-arm-kernel@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@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.

Personally, I would rather not have an intermediate step, but if it
makes the series smoother, I guess it's OK.

Kevin