[PATCH 2/3] clk: samsung: Add clock driver for s5pc100
Yadwinder Singh Brar
yadi.brar01 at gmail.com
Thu Sep 26 08:08:58 EDT 2013
Hi Mateusz,
After a quick view, just few things regarding coding styles.
[...]
> +#define GENERAL_STATUS 0x0104
> +#define CAM_MUX_SEL 0x0300
> +#define MIXER_OUT_SEL 0x0304
> +#define LPMP3_MODE_SEL 0x0308
> +#define MIPI_PHY_CON0 0x0400
> +#define MIPI_PHY_CON1 0x0414
> +#define HDMI_PHY_CON0 0x0420
How about aligning all above with same no. of tabs?
> +
> +/* Helper macros to define clock arrays. */
> +#define FIXED_RATE_CLOCKS(name) \
> + static struct samsung_fixed_rate_clock name[]
> +#define MUX_CLOCKS(name) \
> + static struct samsung_mux_clock name[]
> +#define DIV_CLOCKS(name) \
> + static struct samsung_div_clock name[]
> +#define GATE_CLOCKS(name) \
> + static struct samsung_gate_clock name[]
> +
These macros seems little bit odd in our common practice,
perhaps these are making code harder to read below.
> +/* Helper macros for gate types present on S5PC100. */
> +#define GATE_BUS(_id, cname, pname, o, b) \
> + GATE(_id, cname, pname, o, b, 0, 0)
> +#define GATE_SCLK(_id, cname, pname, o, b) \
> + GATE(_id, cname, pname, o, b, CLK_SET_RATE_PARENT, 0)
> +#define GATE_ON(_id, cname, pname, o, b) \
> + GATE(_id, cname, pname, o, b, CLK_IGNORE_UNUSED, 0)
> +
[...]
> +PNAME(mout_hclk_2_p) = {
> + "fout_epll",
> + "i2scdclk0"
> +};
> +
> +PNAME(mout_i2s_2_p) = {
> + "fout_epll",
> + "i2scdclk0",
> + "dout_audio0",
> + "none"
> +};
> +
Using one line per parent isn't increasing length of file unnecessarily?
[ ... ]
> +
> +/* list of all parent clock list */
> +static struct samsung_clock_alias s5pc100_clock_aliases[] = {
> + ALIAS(FIMC0, "s5pc100-fimc.0", "fimc"),
> + ALIAS(FIMC1, "s5pc100-fimc.1", "fimc"),
> + ALIAS(FIMC2, "s5pc100-fimc.2", "fimc"),
> + ALIAS(MOUT_FIMC2, NULL, "mout_fimc2"),
> + ALIAS(MOUT_FIMC1, NULL, "mout_fimc1"),
> + ALIAS(MOUT_FIMC0, NULL, "mout_fimc0"),
> + ALIAS(SCLK_FIMC0, "s5pc100-fimc.0", "sclk_fimc"),
> + ALIAS(SCLK_FIMC1, "s5pc100-fimc.1", "sclk_fimc"),
> + ALIAS(SCLK_FIMC2, "s5pc100-fimc.2", "sclk_fimc"),
> +
> + ALIAS(MOUT_APLL, NULL, "mout_apll"),
> + ALIAS(MOUT_MPLL, NULL, "mout_mpll"),
> + ALIAS(MOUT_EPLL, NULL, "mout_epll"),
> + ALIAS(MOUT_HPLL, NULL, "mout_hpll"),
> + ALIAS(MOUT_HPLL, NULL, "sclk_hpll"),
> + ALIAS(UART0, "s3c6400-uart.0", "uart"),
> + ALIAS(UART1, "s3c6400-uart.1", "uart"),
> + ALIAS(UART2, "s3c6400-uart.2", "uart"),
> + ALIAS(UART3, "s3c6400-uart.3", "uart"),
> + ALIAS(UART0, "s3c6400-uart.0", "clk_uart_baud0"),
> + ALIAS(UART1, "s3c6400-uart.1", "clk_uart_baud0"),
> + ALIAS(UART2, "s3c6400-uart.2", "clk_uart_baud0"),
> + ALIAS(UART3, "s3c6400-uart.3", "clk_uart_baud0"),
> + ALIAS(UART0, "s3c6400-uart.0", "clk_uart_baud2"),
> + ALIAS(UART1, "s3c6400-uart.1", "clk_uart_baud2"),
> + ALIAS(UART2, "s3c6400-uart.2", "clk_uart_baud2"),
> + ALIAS(UART3, "s3c6400-uart.3", "clk_uart_baud2"),
> + ALIAS(SCLK_UART, "s3c6400-uart.0", "clk_uart_baud3"),
> + ALIAS(SCLK_UART, "s3c6400-uart.1", "clk_uart_baud3"),
> + ALIAS(SCLK_UART, "s3c6400-uart.2", "clk_uart_baud3"),
> + ALIAS(SCLK_UART, "s3c6400-uart.3", "clk_uart_baud3"),
> +
> + ALIAS(HSMMC0, "s3c-sdhci.0", "hsmmc"),
> + ALIAS(HSMMC1, "s3c-sdhci.1", "hsmmc"),
> + ALIAS(HSMMC2, "s3c-sdhci.2", "hsmmc"),
> + ALIAS(HSMMC0, "s3c-sdhci.0", "mmc_busclk.0"),
> + ALIAS(HSMMC1, "s3c-sdhci.1", "mmc_busclk.0"),
> + ALIAS(HSMMC2, "s3c-sdhci.2", "mmc_busclk.0"),
> + ALIAS(SCLK_MMC0, "s3c-sdhci.0", "mmc_busclk.2"),
> + ALIAS(SCLK_MMC1, "s3c-sdhci.1", "mmc_busclk.2"),
> + ALIAS(SCLK_MMC2, "s3c-sdhci.2", "mmc_busclk.2"),
> + ALIAS(SCLK_MMC0_48, "s3c-sdhci.0", "mmc_busclk.3"),
> + ALIAS(SCLK_MMC1_48, "s3c-sdhci.1", "mmc_busclk.3"),
> + ALIAS(SCLK_MMC2_48, "s3c-sdhci.2", "mmc_busclk.3"),
> +
> + ALIAS(SPI0, "s5pc100-spi.0", "spi"),
> + ALIAS(SPI1, "s5pc100-spi.1", "spi"),
> + ALIAS(SPI2, "s5pc100-spi.2", "spi"),
> + ALIAS(SPI0, "s5pc100-spi.0", "spi_busclk0"),
> + ALIAS(SPI1, "s5pc100-spi.1", "spi_busclk0"),
> + ALIAS(SPI2, "s5pc100-spi.2", "spi_busclk0"),
> + ALIAS(SCLK_SPI0_48, "s5pc100-spi.0", "spi_busclk1"),
> + ALIAS(SCLK_SPI1_48, "s5pc100-spi.1", "spi_busclk1"),
> + ALIAS(SCLK_SPI2_48, "s5pc100-spi.2", "spi_busclk1"),
> + ALIAS(SCLK_SPI0, "s5pc100-spi.0", "spi_busclk2"),
> + ALIAS(SCLK_SPI1, "s5pc100-spi.1", "spi_busclk2"),
> + ALIAS(SCLK_SPI2, "s5pc100-spi.2", "spi_busclk2"),
> + ALIAS(PDMA0, "dma-pl330.0", "apb_pclk"),
> + ALIAS(PDMA1, "dma-pl330.1", "apb_pclk"),
> + ALIAS(PWM, NULL, "timers"),
> +
> + ALIAS(JPEG, NULL, "jpeg"),
> + ALIAS(MFC, "s5p-mfc", "mfc"),
> + ALIAS(TV, "s5p-sdo", "dac"),
> + ALIAS(MIXER, "s5p-mixer", "mixer"),
> + ALIAS(VP, "s5p-mixer", "vp"),
> + ALIAS(HDMI, "s5p-hdmi", "hdmi"),
> + ALIAS(SCLK_HDMI, "s5p-hdmi", "hdmiphy"),
> +
> + ALIAS(USB_OTG, NULL, "otg"),
> + ALIAS(USB_HOST, NULL, "usb-host"),
> + ALIAS(USB_HOST, NULL, "usbhost"),
> + ALIAS(LCDCON, "s5pc100-fb", "lcd"),
> + ALIAS(CFCON, NULL, "cfcon"),
> + ALIAS(SYSTIMER, NULL, "systimer"),
> + ALIAS(WDT, NULL, "watchdog"),
> + ALIAS(RTC, NULL, "rtc"),
> + ALIAS(I2C, "s3c2440-i2c.0", "i2c"),
> + ALIAS(CCAN0, NULL, "ccan"),
> + ALIAS(CCAN1, NULL, "ccan"),
> + ALIAS(I2C_HDMI, "s3c2440-i2c.1", "i2c"),
> + ALIAS(HSITX, NULL, "hsitx"),
> + ALIAS(HSIRX, NULL, "hsirx"),
> + ALIAS(TSADC, NULL, "adc"),
> + ALIAS(KEYIF, "s5pc100-keypad", "keypad"),
> + ALIAS(I2S0, "samsung-i2s.0", "iis"),
> + ALIAS(I2S1, "samsung-i2s.1", "iis"),
> + ALIAS(I2S2, "samsung-i2s.2", "iis"),
> + ALIAS(SPDIF, NULL, "spdif"),
> + ALIAS(ROTATOR, NULL, "rot"),
> + ALIAS(DOUT_ARM, NULL, "armclk"),
> + ALIAS(SCLK_AUDIO0, "soc-audio.0", "sclk_audio"),
> + ALIAS(SCLK_AUDIO1, "soc-audio.1", "sclk_audio"),
> + ALIAS(SCLK_AUDIO2, "soc-audio.2", "sclk_audio"),
> + ALIAS(KEYIF, NULL, "keypad"),
> +
> + ALIAS(MFC, "s5p-mfc", "sclk_mfc"),
> + ALIAS(G2D, "s5p-g2d", "fimg2d"),
> +
> +};
> +
Any reason/hidden advantage for using a separate of ALIAS,
instead of using MUX_A/GATE_A ?
[ ... ]
> + s5pc100_clk_register_finpll(xom);
> +
> + /* Register PLLs. */
Why this special comment/treatment for PLLs :) ?
Others below doesn't get such treatment.
> + samsung_clk_register_pll(s5pc100_pll_clks,
> + ARRAY_SIZE(s5pc100_pll_clks), reg_base);
> +
> + samsung_clk_register_fixed_rate(s5pc100_fixed_rate_clks,
> + ARRAY_SIZE(s5pc100_fixed_rate_clks));
> +
>
Regards,
Yadwinder
More information about the linux-arm-kernel
mailing list