"G, Manjunath Kondaiah" <manjugk@xxxxxx> writes: >> -----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. Personally, I would rather not have an intermediate step, but if it makes the series smoother, I guess it's OK. 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
