Re: [PATCH V3 3/5] clk: mmp: add clock definition for pxa910

Chao Xie
2012-08-16T07:43:46Z

On Thu, Aug 16, 2012 at 3:17 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 16 August 2012, Chao Xie wrote:
>
>> +enum {
>> + clk32, vctcxo, pll1, pll1_2, pll1_4, pll1_8, pll1_16, pll1_6, pll1_12,
>> + pll1_24, pll1_48, pll1_96, pll1_13, pll1_13_1_5, pll1_2_1_5,
>> + pll1_3_16, uart_pll, twsi0, twsi1, gpio, kpc, rtc, pwm0, pwm1, pwm2,
>> + pwm3, uart0_mux, uart0, uart1_mux, uart1, uart2_mux, uart2,
>> + ssp0_mux, ssp0, ssp1_mux, ssp1, dfc, sdh0_mux, sdh0, sdh1_mux, sdh1,
>> + usb, sph, disp0_mux, disp0, ccic0_mux, ccic0, ccic0_phy_mux,
>> + ccic0_phy, ccic0_sphy_div, ccic0_sphy, clk_max
>> +};
>
> I wonder whether you should just get rid of this enum
>
>> +void __init pxa910_clk_init(void)
>> +{
>> + struct clk *clocks[clk_max];
>
> and this array,
>
>> + clocks[clk32] =
>> + clk_register_fixed_rate(NULL, "clk32", NULL, CLK_IS_ROOT, 3200);
>> + clk_register_clkdev(clocks[clk32], "clk32", NULL);
>> +
>> + clocks[vctcxo] =
>> + clk_register_fixed_rate(NULL, "vctcxo", NULL, CLK_IS_ROOT,
>> + 26000000);
>> + clk_register_clkdev(clocks[vctcxo], "vctcxo", NULL);
>
> because now that the "obfuscation" macro is gone, it's clear that each index
> is used only once here and then passed right into the next function. If you
> write it like:
>
> struct clk *clk;
>
> clk = clk_register_fixed_rate(NULL, "clk32", NULL, CLK_IS_ROOT, 3200);
> clk_register_clkdev(clk, "clk32", NULL);
>
> it becomes much shorter, partly because things start fitting into one
> line again. The only exception in this file is
>
>> + clocks[uart_pll] =
>> + mmp_clk_register_factor("uart_pll", "pll1_4", 0,
>> + mpmu_base + MPMU_UART_PLL,
>> + &uart_factor_masks, uart_factor_tbl,
>> + ARRAY_SIZE(uart_factor_tbl));
>> + clk_set_rate(clocks[uart_pll], 14745600);
>> + clk_register_clkdev(clocks[uart_pll], "uart_pll", NULL);
>
> with
>
>> + clocks[uart0_mux] =
>> + clk_register_mux(NULL, "uart0_mux", uart_parent,
>> + ARRAY_SIZE(uart_parent), CLK_SET_RATE_PARENT,
>> + apbc_base + APBC_UART0, 4, 3, 0, &clk_lock);
>> + clk_set_parent(clocks[uart0_mux], clocks[uart_pll]);
>> + clk_register_clkdev(clocks[uart0_mux], "uart_mux.0", NULL);
>> +
>> + clocks[uart0] =
>> + mmp_clk_register_apbc("uart0", "uart0_mux", apbc_base + APBC_UART0,
>> + 10, 0, &clk_lock);
>> + clk_register_clkdev(clocks[uart0], NULL, "pxa2xx-uart.0");
>
> so just add another
>
> struct clk *clk_uart;
>
> clk_uart = mmp_clk_register_factor("uart_pll", "pll1_4", 0,
> mpmu_base + MPMU_UART_PLL, &uart_factor_masks, uart_factor_tbl,
> ARRAY_SIZE(uart_factor_tbl));
>
>
>
> Arnd
i can change remove the clocks array, but even make the sentence
shorter, most of them still can not fit in one line.