[PATCH v2 2/6] spi: s3c64xx: move controller information into driver data
From: Kukjin Kim <hidden>
Date: 2012-05-24 07:18:48
Also in:
linux-devicetree, linux-samsung-soc, linux-spi
Thomas Abraham wrote:
Platform data is used to specify controller hardware specific information such as the tx/rx fifo level mask and bit offset of rx fifo level. Such information is not suitable to be supplied from device tree. Instead, it can be moved into the driver data and removed from platform data. Signed-off-by: Thomas Abraham <redacted> Acked-by: Jaswinder Singh <redacted> --- arch/arm/mach-exynos/clock-exynos4.c | 18 +- arch/arm/mach-exynos/setup-spi.c | 25 --- arch/arm/mach-s3c24xx/clock-s3c2416.c | 2 +- arch/arm/mach-s3c24xx/clock-s3c2443.c | 2 +- arch/arm/mach-s3c24xx/common-s3c2443.c | 4 +- arch/arm/mach-s3c24xx/setup-spi.c | 8 - arch/arm/mach-s3c64xx/clock.c | 20 ++-- arch/arm/mach-s3c64xx/setup-spi.c | 13 -- arch/arm/mach-s5p64x0/clock-s5p6440.c | 12 +- arch/arm/mach-s5p64x0/clock-s5p6450.c | 12 +- arch/arm/mach-s5p64x0/setup-spi.c | 16 -- arch/arm/mach-s5pc100/clock.c | 30 ++-- arch/arm/mach-s5pc100/setup-spi.c | 22 --- arch/arm/mach-s5pv210/clock.c | 14 +- arch/arm/mach-s5pv210/setup-spi.c | 15 -- arch/arm/plat-samsung/include/plat/s3c64xx-spi.h | 15 -- drivers/spi/spi-s3c64xx.c | 180
++++++++++++++++++----
17 files changed, 210 insertions(+), 198 deletions(-)
Basically, looks ok to me and there are small comments.
quoted hunk ↗ jump to hunk
diff --git a/arch/arm/mach-exynos/clock-exynos4.c b/arch/arm/mach-exynos/clock-exynos4.c index bcb7db4..10a46a9 100644--- a/arch/arm/mach-exynos/clock-exynos4.c +++ b/arch/arm/mach-exynos/clock-exynos4.c@@ -586,17 +586,17 @@ static struct clk exynos4_init_clocks_off[] = { .ctrlbit = (1 << 13), }, { .name = "spi", - .devname = "s3c64xx-spi.0", + .devname = "exynos4210-spi.0",
Please consider that this can be used only for exynos4210 or all of exynos4 Socs. We need to keep the consistency for the naming. [...]
quoted hunk ↗ jump to hunk
diff --git a/arch/arm/mach-s3c24xx/clock-s3c2416.c b/arch/arm/mach-s3c24xx/clock-s3c2416.c index 8702ecf..a582ba1 100644--- a/arch/arm/mach-s3c24xx/clock-s3c2416.c +++ b/arch/arm/mach-s3c24xx/clock-s3c2416.c@@ -144,7 +144,7 @@ static struct clk_lookup s3c2416_clk_lookup[] = { CLKDEV_INIT("s3c-sdhci.0", "mmc_busclk.0", &hsmmc0_clk), CLKDEV_INIT("s3c-sdhci.0", "mmc_busclk.2", &hsmmc_mux0.clk), CLKDEV_INIT("s3c-sdhci.1", "mmc_busclk.2", &hsmmc_mux1.clk), - CLKDEV_INIT("s3c64xx-spi.0", "spi_busclk2", &hsspi_mux.clk), + CLKDEV_INIT("s3c2443-spi.0", "spi_busclk2", &hsspi_mux.clk),
I think, in this case, some comment helps to know that 's3c2443-spi' can support s3c2416/s3c2450 together. [...]
quoted hunk ↗ jump to hunk
diff --git a/arch/arm/mach-s5p64x0/clock-s5p6440.c b/arch/arm/mach-s5p64x0/clock-s5p6440.c index ee1e8e7..55ea3ab 100644--- a/arch/arm/mach-s5p64x0/clock-s5p6440.c +++ b/arch/arm/mach-s5p64x0/clock-s5p6440.c
[...]
quoted hunk ↗ jump to hunk
@@ -519,8 +519,8 @@ static struct clk_lookup s5p6440_clk_lookup[] = { CLKDEV_INIT(NULL, "clk_uart_baud2", &clk_pclk_low.clk), CLKDEV_INIT(NULL, "clk_uart_baud3", &clk_sclk_uclk.clk), CLKDEV_INIT(NULL, "spi_busclk0", &clk_p), - CLKDEV_INIT("s3c64xx-spi.0", "spi_busclk1", &clk_sclk_spi0.clk), - CLKDEV_INIT("s3c64xx-spi.1", "spi_busclk1", &clk_sclk_spi1.clk), + CLKDEV_INIT("s5p64x0.0", "spi_busclk1", &clk_sclk_spi0.clk), + CLKDEV_INIT("s5p64x0.1", "spi_busclk1", &clk_sclk_spi1.clk),
Should be s5p64x0-spi.0 and s5p64x0-spi.1 ? [...]
quoted hunk ↗ jump to hunk
diff --git a/arch/arm/plat-samsung/include/plat/s3c64xx-spi.hb/arch/arm/plat-samsung/include/plat/s3c64xx-spi.h index fa95e9a..4e9b9c3 100644--- a/arch/arm/plat-samsung/include/plat/s3c64xx-spi.h +++ b/arch/arm/plat-samsung/include/plat/s3c64xx-spi.h
[...]
quoted hunk ↗ jump to hunk
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c index 6a3d51a..f6bc0e3 100644 --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c@@ -31,6 +31,8 @@ #include <mach/dma.h> #include <plat/s3c64xx-spi.h> +#define MAX_SPI_PORTS 3
As you know, if spi_isp is used, this can be 5 for latest SoCs later. Just note. [...]
/**
+ * struct s3c64xx_spi_info - SPI Controller hardware info
+ * @fifo_lvl_mask: Bit-mask for {TX|RX}_FIFO_LVL bits in SPI_STATUS
register.
+ * @rx_lvl_offset: Bit offset of RX_FIFO_LVL bits in SPI_STATUS regiter.
+ * @tx_st_done: Bit offset of TX_DONE bit in SPI_STATUS regiter.
+ * @high_speed: True, if the controller supports HIGH_SPEED_EN bit.
+ * @clk_from_cmu: True, if the controller does not include a clock mux
and
+ * prescaler unit.
+ *
+ * The Samsung s3c64xx SPI controller are used on various Samsung SoC's
but
+ * differ in some aspects such as the size of the fifo and spi bus clock
+ * setup. Such differences are specified to the driver using this
structure
+ * which is provided as driver data to the driver.
+ */
+struct s3c64xx_spi_port_config {
+ int fifo_lvl_mask[MAX_SPI_PORTS];
+ int rx_lvl_offset;
+ int tx_st_done;
+ bool high_speed;
+ bool clk_from_cmu;
+};When I saw this, I thought this will be used for each SPI port. But I know, above structure is used for each SoC not each SPI port. So I think, how about to change the structure name to avoid confusion? [...]
quoted hunk ↗ jump to hunk
@@ -1000,6 +1029,7 @@ static int __init s3c64xx_spi_probe(structplatform_device *pdev) platform_set_drvdata(pdev, master); sdd = spi_master_get_devdata(master); + sdd->port_conf = s3c64xx_spi_get_port_config(pdev); sdd->master = master; sdd->cntrlr_info = sci; sdd->pdev = pdev;@@ -1008,10 +1038,11 @@ static int __init s3c64xx_spi_probe(structplatform_device *pdev) sdd->tx_dma.direction = DMA_MEM_TO_DEV; sdd->rx_dma.dmach = dmarx_res->start; sdd->rx_dma.direction = DMA_DEV_TO_MEM; + sdd->port_id = pdev->id; sdd->cur_bpw = 8; - master->bus_num = pdev->id; + master->bus_num = sdd->port_id;
[...]
quoted hunk ↗ jump to hunk
@@ -1071,7 +1102,7 @@ static int __init s3c64xx_spi_probe(structplatform_device *pdev) } /* Setup Deufult Mode */ - s3c64xx_spi_hwinit(sdd, pdev->id); + s3c64xx_spi_hwinit(sdd, sdd->port_id);
Do we need this change?
quoted hunk ↗ jump to hunk
spin_lock_init(&sdd->lock); init_completion(&sdd->xfer_completion);@@ -1096,7 +1127,7 @@ static int __init s3c64xx_spi_probe(structplatform_device *pdev) dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d " "with %d Slaves attached\n", - pdev->id, master->num_chipselect); + sdd->port_id,
master->num_chipselect); Same as above.
dev_dbg(&pdev->dev, "\tIOmem=[0x%x-0x%x]\tDMA=[Rx-%d, Tx-%d]\n", mem_res->end, mem_res->start, sdd->rx_dma.dmach,
sdd->tx_dma.dmach);
quoted hunk ↗ jump to hunk
@@ -1189,7 +1220,7 @@ static int s3c64xx_spi_resume(struct device *dev) clk_enable(sdd->src_clk); clk_enable(sdd->clk); - s3c64xx_spi_hwinit(sdd, pdev->id); + s3c64xx_spi_hwinit(sdd, sdd->port_id);
Same as above. [...]
+#ifdef CONFIG_ARCH_EXYNOS4
+struct s3c64xx_spi_port_config exynos4_spi_port_config = {Is this available on exynos4212/exynos4412? If not, this should be 'exynos4210_spi_port_config' and ifdef CONFIG_CPU_EXYNOS4210.
+ .fifo_lvl_mask = { 0x1ff, 0x7F, 0x7F },
+ .rx_lvl_offset = 15,
+ .tx_st_done = 25,
+ .high_speed = 1,
+ .clk_from_cmu = true,
+};
+#define EXYNOS4_SPI_PORT_CONFIG((kernel_ulong_t)&exynos4_spi_port_config)
+#else +#define EXYNOS4_SPI_PORT_CONFIG ((kernel_ulong_t)NULL) +#endif /* CONFIG_ARCH_EXYNOS4 */
And let's think the name of ..._SPI_PORT_CONFIG again. How about just ..._SPI_CONFIG?
+
+static struct platform_device_id s3c64xx_spi_driver_ids[] = {
+ {
+ .name = "s3c2443-spi",
+ .driver_data = S3C2443_SPI_PORT_CONFIG,
+ }, {
+ .name = "s3c6410-spi",
+ .driver_data = S3C6410_SPI_PORT_CONFIG,
+ }, {
+ .name = "s5p64x0-spi",
+ .driver_data = S5P64X0_SPI_PORT_CONFIG,
+ }, {
+ .name = "s5pc100-spi",
+ .driver_data = S5PC100_SPI_PORT_CONFIG,
+ }, {
+ .name = "s5pv210-spi",
+ .driver_data = S5PV210_SPI_PORT_CONFIG,
+ }, {
+ .name = "exynos4210-spi",
+ .driver_data = EXYNOS4_SPI_PORT_CONFIG,As I commented, if this is only for exynos4210, EXYNOS4210_SPI_CONFIG should be used here not EXYNOS4_SPI_. [...] And I think, it's time to change the name of spi driver to spi-samsung.c and samsung_spi as a prefix even though we have another s3c24xx spi driver now. Thanks. Best regards, Kgene. -- Kukjin Kim [off-list ref], Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.