On Tue, 2014-03-25 at 11:00 +0100, [email protected] wrote: > From: Olliver Schinagl <[email protected]> > > 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? 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
