On 24 May 2012 12:48, Kukjin Kim <kgene.kim@xxxxxxxxxxx> wrote:
> 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 <thomas.abraham@xxxxxxxxxx>
>> Acked-by: Jaswinder Singh <jaswinder.singh@xxxxxxxxxx>
>> ---
>> 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.
>
>> 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.
It is always easier to assign a specific device name such as
Exynos4210. Any other SoC in the Exynos4 family which has a exactly
similar SPI controller(s) as that of Exynos4210 can claim
compatibility to the Exynos4210 type by using Exynos4210 as the device
name.
Using generic name like Exynos4 as problems. If any one of the SoC's
in the Exynos4 family (present or upcoming) uses different properties
from that of Exynos4210 SPI controller (such as different tx/rx fifo
size, different bit positions in a register or new functionality),
then it can no more be classified as a Exynos4 compatible SPI
controller.
>
> [...]
>
>> 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.
Ok.
>
> [...]
>
>> 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
>
> [...]
>
>> @@ -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 ?
Yes, you are right. I will fix this.
>
> [...]
>
>> diff --git a/arch/arm/plat-samsung/include/plat/s3c64xx-spi.h
>> b/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
>
> [...]
>
>> 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.
Ok.
>
> [...]
>
>> /**
>> + * 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?
The name of the above structure is following the existing name
conventions which the driver is using to name its structures.
>
> [...]
>
>> @@ -1000,6 +1029,7 @@ static int __init s3c64xx_spi_probe(struct
>> platform_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(struct
>> platform_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;
>
> [...]
>
>> @@ -1071,7 +1102,7 @@ static int __init s3c64xx_spi_probe(struct
>> platform_device *pdev)
>> }
>>
>> /* Setup Deufult Mode */
>> - s3c64xx_spi_hwinit(sdd, pdev->id);
>> + s3c64xx_spi_hwinit(sdd, sdd->port_id);
>
> Do we need this change?
Yes, this change is required. In dt mode, the port_id is obtained from
the alias node (since pdev->id is not valid).
>
>>
>> spin_lock_init(&sdd->lock);
>> init_completion(&sdd->xfer_completion);
>> @@ -1096,7 +1127,7 @@ static int __init s3c64xx_spi_probe(struct
>> platform_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.
Yes. This is required.
>
>> 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);
>> @@ -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.
Yes. This is required.
>
> [...]
>
>> +#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?
Ok. I will change it.
>
>> +
>> +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_.
Ok.
>
> [...]
>
> 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.
Ok. It would be better to handle this change outside of this patch
series. This patch series is mainly about adding device tree support.
Thanks for your review and comments.
Regards,
Thomas.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
