Hi Olof,
On 30 May 2012 15:23, Olof Johansson <olof@xxxxxxxxx> wrote:
> Hi,
>
> Some comments below.
>
> On Tue, May 8, 2012 at 3:04 PM, Thomas Abraham
> <thomas.abraham@xxxxxxxxxx> 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.
>>
>> Cc: Jaswinder Singh <jaswinder.singh@xxxxxxxxxx>
>> Signed-off-by: Thomas Abraham <thomas.abraham@xxxxxxxxxx>
>> ---
>> drivers/spi/spi-s3c64xx.c | 180 ++++++++++++++++++++++++++++++++++++++-------
>> 1 files changed, 153 insertions(+), 27 deletions(-)
>>
>> 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
>
>> @@ -171,6 +198,8 @@ struct s3c64xx_spi_driver_data {
>> struct s3c64xx_spi_dma_data rx_dma;
>> struct s3c64xx_spi_dma_data tx_dma;
>> struct samsung_dma_ops *ops;
>> + struct s3c64xx_spi_port_config *port_conf;
>> + unsigned port_id;
>
> unsigned int
Ok.
>
>
>> @@ -942,6 +964,13 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd, int channel)
>> flush_fifo(sdd);
>> }
>>
>> +static inline struct s3c64xx_spi_port_config *s3c64xx_spi_get_port_config(
>> + struct platform_device *pdev)
>> +{
>> + return (struct s3c64xx_spi_port_config *)
>> + platform_get_device_id(pdev)->driver_data;
>> +}
>> +
>> static int __init s3c64xx_spi_probe(struct platform_device *pdev)
>> {
>> struct resource *mem_res, *dmatx_res, *dmarx_res;
>> @@ -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;
>
> Single-use helper? Might as well open code it in this case.
This helper function 's3c64xx_spi_get_port_config' is populated with
more code later in the 'add spi support' patch. Which simplifies the
code flow here. So I prefer to maintain this as a separate function.
>
>
>> @@ -1227,6 +1258,100 @@ static const struct dev_pm_ops s3c64xx_spi_pm = {
>> s3c64xx_spi_runtime_resume, NULL)
>> };
>>
>> +#if defined(CONFIG_CPU_S3C2416) || defined(CONFIG_CPU_S3C2443)
>> +struct s3c64xx_spi_port_config s3c2443_spi_port_config = {
>> + .fifo_lvl_mask = { 0x7f },
>> + .rx_lvl_offset = 13,
>> + .tx_st_done = 21,
>> + .high_speed = true,
>> +};
>> +#define S3C2443_SPI_PORT_CONFIG ((kernel_ulong_t)&s3c2443_spi_port_config)
>> +#else
>> +#define S3C2443_SPI_PORT_CONFIG ((kernel_ulong_t)NULL)
>> +#endif
>
> Is it really worth it to do the ifdefs here for just 16 bytes of data
> per entry? The table itself below takes more space.
Ok. The ifdefs do reduce the readability of this code. So I will leave
the ifdefs in this patch.
>
>
> [..]
>
>> +#ifdef CONFIG_ARCH_S5PV210
>> +struct s3c64xx_spi_port_config s5pv210_spi_port_config = {
>> + .fifo_lvl_mask = { 0x1ff, 0x7F },
>> + .rx_lvl_offset = 15,
>> + .tx_st_done = 25,
>> + .high_speed = 1,
>
> high_speed = true
>
>> +};
>> +#define S5PV210_SPI_PORT_CONFIG ((kernel_ulong_t)&s5pv210_spi_port_config)
>> +#else
>> +#define S5PV210_SPI_PORT_CONFIG ((kernel_ulong_t)NULL)
>> +#endif /* CONFIG_ARCH_S5PV210 */
>> +
>> +#ifdef CONFIG_ARCH_EXYNOS4
>> +struct s3c64xx_spi_port_config exynos4_spi_port_config = {
>> + .fifo_lvl_mask = { 0x1ff, 0x7F, 0x7F },
>> + .rx_lvl_offset = 15,
>> + .tx_st_done = 25,
>> + .high_speed = 1,
>
> high_speed = true
Ok.
Thanks for your comments.
Regards,
Thomas.
>
>
>
> -Olof
--
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
