> -----Original Message----- > From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] > Sent: Wednesday, November 10, 2010 9:33 PM > To: G, Manjunath Kondaiah > Cc: linux-omap@xxxxxxxxxxxxxxx; > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Cousson, Benoit; > Shilimkar, Santosh > Subject: Re: [PATCH v3 01/13] OMAP: DMA: Replace read/write > macros with functions > > "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. 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 -- 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
