"G, Manjunath Kondaiah" <manjugk@xxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
