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:

[...]

>> > +		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.

>                 __raw_writew(val, omap_dma_base + offset);

This could be moved before the 'if' and the 'else' clause removed.

>                 if ((reg > CLNK_CTRL && reg < CCR2) ||
>                                 (reg > PCHD_ID && reg < CAPS_2)) {
>                         u32 offset2 = reg_map[reg] + 2 + (stride * lch);
>                         __raw_writew(val >> 16, omap_dma_base + offset2);
>                 }
>         } else
>                 __raw_writel(val, omap_dma_base + offset);
> }

Kevin