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

[Nishanth Menon <nm@ti.com>]

G, Manjunath Kondaiah had written, on 10/26/2010 08:25 AM, the following:
[...]
> 
> diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
> index f5c5b8d..77241e2 100644
> --- a/arch/arm/plat-omap/dma.c
> +++ b/arch/arm/plat-omap/dma.c
> @@ -40,6 +40,147 @@
> 
>  #undef DEBUG
> 
> +enum {
> +       GCR1 = 0,       GSCR,           GRST,           HW_ID,
> +       PCH2_ID,        PCH0_ID,        PCH1_ID,        PCHG_ID,
> +       PCHD_ID,        CAPS1_0_U,      CAPS1_0_L,      CAPS1_1_U,
> +       CAPS1_1_L,      CAPS1_2,        CAPS1_3,        CAPS1_4,
> +       PCH2_SR,        PCH0_SR,        PCH1_SR,        PCHD_SR,
[...]

> +};
> +
> +static u16 reg_map_omap1[] = {
> +       [GCR1]          = 0x400,
> +       [GSCR]          = 0x404,
> +       [GRST]          = 0x408,
> +       [HW_ID]         = 0x442,
> +       [PCH2_ID]       = 0x444,
> +       [PCH0_ID]       = 0x446,
> +       [PCH1_ID]       = 0x448,
> +       [PCHG_ID]       = 0x44a,
> +       [PCHD_ID]       = 0x44c,
[...]
> +       [LCH_CTRL]      = 0x2a,
> +};
> +
> +enum {
> +       REVISION = 0,   GCR2,           IRQSTATUS_L0,   IRQSTATUS_L1,
> +       IRQSTATUS_L2,   IRQSTATUS_L3,   IRQENABLE_L0,   IRQENABLE_L1,
[...]
> +
> +       /* OMAP4 specific registers */
> +       CDP,            CNDP,           CCDN,
> +
> +       OMAP2_CH_COMMON_END,
> +};
> +
> +static u16 reg_map_omap2[] = {
> +       [REVISION]              = 0x00,
> +       [GCR2]                  = 0x78,
> +       [IRQSTATUS_L0]          = 0x08,
> +       [IRQSTATUS_L1]          = 0x0c,
[..]
> +       /* OMAP4 specific registers */
> +       [CDP]                   = 0xd0,
> +       [CNDP]                  = 0xd4,
> +       [CCDN]                  = 0xd8,
> +};
> +
dumb question: any reason why a struct wont do?
struct reg_map_omap2 {
u16 revision;
...
...
}

[..]
> 
> -#define dma_read(reg)                                                  \
> -({                                                                     \
> -       u32 __val;                                                      \
> -       if (cpu_class_is_omap1())                                       \
> -               __val = __raw_readw(omap_dma_base + OMAP1_DMA_##reg);   \
> -       else                                                            \
> -               __val = __raw_readl(omap_dma_base + OMAP_DMA4_##reg);   \
> -       __val;                                                          \
> -})
> -
> -#define dma_write(val, reg)                                            \
> -({                                                                     \
> -       if (cpu_class_is_omap1())                                       \
> -               __raw_writew((u16)(val), omap_dma_base + OMAP1_DMA_##reg); \
> -       else                                                            \
> -               __raw_writel((val), omap_dma_base + OMAP_DMA4_##reg);   \
> -})
> +static inline void dma_write(u32 val, int reg, int lch)
> +{
> +       if (cpu_class_is_omap1()) {
> +               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]);
> +       }
> +}
> +
> +static inline u32 dma_read(int reg, int lch)
> +{
> +       u32 val;
> +
> +       if (cpu_class_is_omap1()) {
> +               if (reg > OMAP1_CH_COMMON_START)
> +                       val = __raw_readw(dma_base +
> +                                       (reg_map_omap1[reg] + 0x40 * lch));
> +               else
> +                       val = __raw_readw(dma_base + reg_map_omap1[reg]);
> +       } else {
> +               if (reg > OMAP2_CH_COMMON_START)
> +                       val = __raw_readl(dma_base +
> +                                       (reg_map_omap2[reg] + 0x60 * lch));
> +               else
> +                       val = __raw_readl(dma_base + reg_map_omap2[reg]);
> +       }
> +       return val;
> +}
What is the benefit of using inline function here? would'nt we increase 
code size? cant we use a function pointer initialized to class1 or rest? 
Quote from CodingStyle (15):
"A reasonable rule of thumb is to not put inline at functions that have more
than 3 lines of code in them. An exception to this rule are the cases where
a parameter is known to be a compiletime constant, and as a result of this
constantness you *know* the compiler will be able to optimize most of your
function away at compile time. For a good example of this later case, see
the kmalloc() inline function.
"
is the expectation that cpu_class_is_omap1 compile time constant hence 
the actual compiled in code is smaller -candidate for inline?

> 
>  #ifdef CONFIG_ARCH_OMAP15XX
>  /* Returns 1 if the DMA module is in OMAP1510-compatible mode, 0 otherwise */
> @@ -209,11 +369,10 @@ static inline void set_gdma_dev(int req, int dev)
>  /* Omap1 only */
>  static void clear_lch_regs(int lch)
>  {
> -       int i;
> -       void __iomem *lch_base = omap_dma_base + OMAP1_DMA_CH_BASE(lch);
> +       int i = OMAP1_CH_COMMON_START;
> 
> -       for (i = 0; i < 0x2c; i += 2)
> -               __raw_writew(0, lch_base + i);
> +       for (; i <= OMAP1_CH_COMMON_END; i += 1)
> +               dma_write(0, i, lch);
>  }
> 
>  void omap_set_dma_priority(int lch, int dst_port, int priority)
> @@ -248,12 +407,12 @@ void omap_set_dma_priority(int lch, int dst_port, int priority)
>         if (cpu_class_is_omap2()) {
>                 u32 ccr;
> 
> -               ccr = dma_read(CCR(lch));
> +               ccr = dma_read(CCR2, lch);
>                 if (priority)
>                         ccr |= (1 << 6);
>                 else
>                         ccr &= ~(1 << 6);
> -               dma_write(ccr, CCR(lch));
> +               dma_write(ccr, CCR2, lch);
>         }
>  }
>  EXPORT_SYMBOL(omap_set_dma_priority);
> @@ -264,31 +423,36 @@ void omap_set_dma_transfer_params(int lch, int data_type, int elem_count,
>  {
>         u32 l;
> 
> -       l = dma_read(CSDP(lch));
> -       l &= ~0x03;
> -       l |= data_type;
> -       dma_write(l, CSDP(lch));
> -
>         if (cpu_class_is_omap1()) {
>                 u16 ccr;
> 
> -               ccr = dma_read(CCR(lch));
> +               l = dma_read(CSDP1, lch);
> +               l &= ~0x03;
> +               l |= data_type;
> +               dma_write(l, CSDP1, lch);
> +
> +               ccr = dma_read(CCR1, lch);
>                 ccr &= ~(1 << 5);
>                 if (sync_mode == OMAP_DMA_SYNC_FRAME)
>                         ccr |= 1 << 5;
> -               dma_write(ccr, CCR(lch));
> +               dma_write(ccr, CCR1, lch);
> 
> -               ccr = dma_read(CCR2(lch));
> +               ccr = dma_read(CCR1_2, lch);
>                 ccr &= ~(1 << 2);
>                 if (sync_mode == OMAP_DMA_SYNC_BLOCK)
>                         ccr |= 1 << 2;
> -               dma_write(ccr, CCR2(lch));
> +               dma_write(ccr, CCR1_2, lch);
>         }
> 
>         if (cpu_class_is_omap2() && dma_trigger) {
>                 u32 val;
> 
> -               val = dma_read(CCR(lch));
> +               l = dma_read(CSDP2, lch);
> +               l &= ~0x03;
> +               l |= data_type;
> +               dma_write(l, CSDP2, lch);
This seems to me to be a replication mess :( compared to the original 
CSDP(lch) - which mapped to class1 or class2 as needed - same elsewhere. 
Code is becoming pretty much needing a refactoring as a result :(

[...]

-- 
Regards,
Nishanth Menon