On 03/27/2014 10:06 PM, Ian Campbell wrote:
On Tue, 2014-03-25 at 11:00 +0100, oliver+l...@schinagl.nl wrote:
From: Olliver Schinagl <oli...@schinagl.nl>

This patch cleans up several macro's to remove magic values etc from
clock.c and clock.h. Casualties being dragged in are some macro's from
dram.c and the i2c driver.

This addresses (some of) Marek's review on the upstreaming series, is
that right?
Yes, but this is just some of the stuff i had done for some early sunxi review. I since have cleaned it up better, changed it a little; and with you workflow explanation, have to redo it. On top of that, i have to split it out in seperate patches *yay*

I will work on that the next few days; but you now know what i'm doing, and I know how to test it :)

so just diff the output of objdump -d is what you do? I should be able to do that.

You mentioned in the cover letter that you had only compile tested, for
this sort of cleanup the approach I would take is to make sure that
objdump -d before and after is identical, this way you can be absolutely
sure you didn't accidentally break something. Any changes which
intentionally change something should be split out. Unfortunately the
sr32 changes fall into the latter bucket, oh well.

I comments on the sr32 transforms already. Some others:

        /* open the clock for uart */
-       sr32(&ccm->apb1_gate, 16 + CONFIG_CONS_INDEX - 1, 1, CLK_GATE_OPEN);
+       clrsetbits_le32(&ccm->apb1_clk_div_cfg,
+                       CCM_APB_GATE_UART(
+                       SUNXI_CONS_TO_UART(CONFIG_CONS_INDEX)),

Consider a helper which combines CCM_APB_GATE_UART and
SUNXI_CONS_TO_UART?

@@ -85,9 +108,10 @@ unsigned int clock_get_pll5(void)
        struct sunxi_ccm_reg *const ccm =
                (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
        uint32_t rval = readl(&ccm->pll5_cfg);
-       int n = (rval >> 8) & 0x1f;
-       int k = ((rval >> 4) & 3) + 1;
-       int p = 1 << ((rval >> 16) & 3);
+       int n = CCM_PLL5_CFG_N_GET(rval);
+       int k = CCM_PLL5_CFG_K_GET(rval);
+       int p = CCM_PLL5_CFG_OUT_EXT_DIV_P_GET(rval);
+
        return 24000000 * n * k / p;

Is 24MHz #defined somewhere?

  }

@@ -96,40 +120,52 @@ int clock_twi_onoff(int port, int state)
        struct sunxi_ccm_reg *const ccm =
                (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;

-       if (port > 2)
+       if (!(port & (CCM_APB_GATE_TWI1 | CCM_APB_GATE_TWI2 |
+                     CCM_APB_GATE_TWI3 | CCM_APB_GATE_TWI4)))

Maybe #define a TWI_MASK?

                return -1;

        /* set the apb1 clock gate for twi */
-       sr32(&ccm->apb1_gate, 0 + port, 1, state);
+       if (state)
+               clrsetbits_le32(&ccm->apb1_gate, CCM_APB_GATE_TWI(port),
+                               CCM_APB_GATE_TWI(port));

Shouldn't these macros involve state and nor port, or maybe both? The
original code was passing state to sr32.

+       else
+               clrbits_le32(&ccm->apb1_gate, CCM_APB_GATE_TWI(port));

        return 0;
  }

Ian.


--
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to