[U-Boot] [PATCH 5/7] EXYNOS: support EXYNOS MIPI DSI interface driver.

[Anatolij Gustschin <agust@denx.de>]

Hi,

Please see minor comments below.

On Thu, 05 Apr 2012 15:29:29 +0900
Donghwa Lee <dh09.lee@samsung.com> wrote:

> EXYNOS SoC platform has MIPI-DSI controller and MIPI-DSI
> based LCD Panel could be used with it. This patch supports MIPI-DSI driver
> based Samsung SoC chip.
> 
> LCD panel driver based MIPI-DSI should be registered to MIPI-DSI driver at
> board file and LCD panel driver specific function registered to mipi_dsim_ddi
> structure at lcd panel init function called system init.
> In the MIPI-DSI driver, find lcd panel driver by using registered
> lcd panel name, and then initialize lcd panel driver.
> 
> Signed-off-by: Donghwa Lee <dh09.lee@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> ---
>  arch/arm/include/asm/arch-exynos/dsim.h      |  181 +++++++
>  arch/arm/include/asm/arch-exynos/mipi_dsim.h |  400 +++++++++++++++
>  drivers/video/exynos_mipi_dsi.c              |  257 ++++++++++
>  drivers/video/exynos_mipi_dsi_common.c       |  645 +++++++++++++++++++++++++
>  drivers/video/exynos_mipi_dsi_common.h       |   48 ++
>  drivers/video/exynos_mipi_dsi_lowlevel.c     |  669 ++++++++++++++++++++++++++
>  drivers/video/exynos_mipi_dsi_lowlevel.h     |  111 +++++
>  7 files changed, 2311 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/include/asm/arch-exynos/dsim.h
>  create mode 100644 arch/arm/include/asm/arch-exynos/mipi_dsim.h
>  create mode 100644 drivers/video/exynos_mipi_dsi.c
>  create mode 100644 drivers/video/exynos_mipi_dsi_common.c
>  create mode 100644 drivers/video/exynos_mipi_dsi_common.h
>  create mode 100644 drivers/video/exynos_mipi_dsi_lowlevel.c
>  create mode 100644 drivers/video/exynos_mipi_dsi_lowlevel.h

...
> diff --git a/arch/arm/include/asm/arch-exynos/mipi_dsim.h b/arch/arm/include/asm/arch-exynos/mipi_dsim.h
> new file mode 100644
> index 0000000..4ae6830
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-exynos/mipi_dsim.h
...
> +/*
> + * struct mipi_dsim_device - global interface for mipi-dsi driver.
> + *
> + * @dev: driver model representation of the device.
> + * @id: unique device id.
> + * @clock: pointer to MIPI-DSI clock of clock framework.
> + * @irq: interrupt number to MIPI-DSI controller.
> + * @reg_base: base address to memory mapped SRF of MIPI-DSI controller.
> + *	(virtual address)
> + * @lock: the mutex protecting this data structure.

These above documented structure members are not in the below
struct for U-Boot, so please remove their description.

> + * @dsim_info: infomation for configuring mipi-dsi controller.

The member in the struct is named dsim_config, please update this
description accordingly.

> + * @master_ops: callbacks to mipi-dsi operations.
> + * @dsim_lcd_dev: pointer to activated ddi device.
> + *	(it would be registered by mipi-dsi driver.)
> + * @dsim_lcd_drv: pointer to activated_ddi driver.
> + *	(it would be registered by mipi-dsi driver.)
> + * @lcd_info: pointer to mipi_lcd_info structure.

Drop @lcd_info description.

> + * @state: specifies status of MIPI-DSI controller.
> + *	the status could be RESET, INIT, STOP, HSCLKEN and ULPS.
> + * @resume_complete: indicates whether resume operation is completed or not.

Drop @resume_complete.

> + * @data_lane: specifiec enabled data lane number.
> + *	this variable would be set by driver according to e_no_data_lane
> + *	automatically.
> + * @e_clk_src: select byte clock source.
> + * @pd: pointer to MIPI-DSI driver platform data.
> + */
> +struct mipi_dsim_device {
> +	struct mipi_dsim_config		*dsim_config;
> +	struct mipi_dsim_master_ops	*master_ops;
> +	struct mipi_dsim_lcd_device	*dsim_lcd_dev;
> +	struct mipi_dsim_lcd_driver	*dsim_lcd_drv;
> +
> +	unsigned int			state;
> +	unsigned int			data_lane;
> +	enum mipi_dsim_byte_clk_src	e_clk_src;
> +
> +	struct exynos_platform_mipi_dsim	*pd;
> +};
> +
> +/*
> + * struct exynos_platform_mipi_dsim - interface to platform data
> + *	for mipi-dsi driver.
> + *
> + * @lcd_panel_name: specifies lcd panel name registered to mipi-dsi driver.
> + *	lcd panel driver searched would be actived.
> + * @dsim_config: pointer of structure for configuring mipi-dsi controller.
> + * @lcd_panel_info: pointer for lcd panel specific structure.
> + *	this structure specifies width, height, timing and polarity and so on.
> + * @mipi_power: callback pointer for enabling or disabling mipi power.
> + * @phy_enable: pointer to a callback controlling D-PHY enable/reset

lcd_power from below struct is not in the description above.

> + */
> +struct exynos_platform_mipi_dsim {
> +	char				lcd_panel_name[PANEL_NAME_SIZE];
> +
> +	struct mipi_dsim_config		*dsim_config;
> +	void				*lcd_panel_info;
> +
> +	int (*lcd_power)(void);
> +	int (*mipi_power)(void);
> +	void (*phy_enable)(unsigned int enable, unsigned int dev_index);
> +};

Please add an empty line here.

> +/*
> + * struct mipi_dsim_master_ops - callbacks to mipi-dsi operations.
> + *
> + * @cmd_write: transfer command to lcd panel at LP mode.
> + * @cmd_read: read command from rx register.
> + * @get_dsim_frame_done: get the status that all screen data have been
> + *	transferred to mipi-dsi.
> + * @clear_dsim_frame_done: clear frame done status.
> + * @get_fb_frame_done: get frame done status of display controller.
> + * @trigger: trigger display controller.
> + *	- this one would be used only in case of CPU mode.
> + */
> +

and drop this empty line here.

> +struct mipi_dsim_master_ops {
> +	int (*cmd_write)(struct mipi_dsim_device *dsim, unsigned int data_id,
> +		unsigned int data0, unsigned int data1);
> +	int (*cmd_read)(struct mipi_dsim_device *dsim, unsigned int data_id,
> +		unsigned int data0, unsigned int data1);
> +	int (*get_dsim_frame_done)(struct mipi_dsim_device *dsim);
> +	int (*clear_dsim_frame_done)(struct mipi_dsim_device *dsim);
> +
> +	int (*get_fb_frame_done)(void);
> +	void (*trigger)(struct fb_info *info);
> +};
> +
> +/*
> + * device structure for mipi-dsi based lcd panel.
> + *
> + * @name: name of the device to use with this device, or an
> + *	alias for that name.
> + * @dev: driver model representation of the device.

drop @dev description.

> + * @id: id of device to be registered.
> + * @bus_id: bus id for identifing connected bus
> + *	and this bus id should be same as id of mipi_dsim_device.
> + * @irq: irq number for signaling when framebuffer transfer of
> + *	lcd panel module is completed.
> + *	this irq would be used only for MIPI-DSI based CPU mode lcd panel.

drop @irq description.

> + * @master: pointer to mipi-dsi master device object.
> + * @platform_data: lcd panel specific platform data.
> + */
> +struct mipi_dsim_lcd_device {
> +	char			*name;
> +	char			*panel_id;

This panel_id seems to be not used anywhere? Drop it?

> +	int			panel_type;
> +	int			id;
> +	int			bus_id;
> +	int			reverse;
> +
> +	struct mipi_dsim_device *master;
> +	void			*platform_data;
> +};
> +
> +/*
> + * driver structure for mipi-dsi based lcd panel.
> + *
> + * this structure should be registered by lcd panel driver.
> + * mipi-dsi driver seeks lcd panel registered through name field
> + * and calls these callback functions in appropriate time.
> + *
> + * @name: name of the driver to use with this device, or an
> + *	alias for that name.
> + * @id: id of driver to be registered.
> + *	this id would be used for finding device object registered.
> + */
> +struct mipi_dsim_lcd_driver {
> +	char			*name;
> +	int			id;
> +
> +	int	(*mipi_panel_init)(struct mipi_dsim_device *dsim_dev);
> +	void	(*mipi_display_on)(struct mipi_dsim_device *dsim_dev);

Only 'name' and 'id' is documented above. Document these callbacks, too.

...
> +/*
> + * exynos_dsim_phy_enable - global MIPI-DSI receiver D-PHY control
> + * @pdev: MIPI-DSIM platform device

Drop @pdev description.

> + * @on: true to enable D-PHY and deassert its reset
> + *	false to disable D-PHY
> + */
> +int exynos_dsim_phy_enable(int on);

...
> diff --git a/drivers/video/exynos_mipi_dsi.c b/drivers/video/exynos_mipi_dsi.c
> new file mode 100644
> index 0000000..704f6a7
> --- /dev/null
> +++ b/drivers/video/exynos_mipi_dsi.c
> @@ -0,0 +1,257 @@
> +/*
> + * Copyright (C) 2012 Samsung Electronics
> + *
> + * Author: InKi Dae <inki.dae@samsung.com>
> + * Author: Donghwa Lee <dh09.lee@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	 See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <ubi_uboot.h>
> +#include <common.h>
> +#include <lcd.h>
> +#include <asm/errno.h>

Please drop ubi_uboot.h, lcd.h and errno.h here and better use

#include <common.h>
#include <malloc.h>
#include <linux/err.h>

> +#include <asm/arch/dsim.h>
> +#include <asm/arch/mipi_dsim.h>
> +#include <asm/arch/power.h>
> +#include <asm/arch/cpu.h>
> +#include <asm/arch/clk.h>
> +#include <linux/types.h>

types.h can be dropped too, I think.

> +#include <linux/list.h>
> +
> +#include "exynos_mipi_dsi_lowlevel.h"
> +#include "exynos_mipi_dsi_common.h"
> +
> +#define master_to_driver(a)	(a->dsim_lcd_drv)
> +#define master_to_device(a)	(a->dsim_lcd_dev)
> +
> +static struct exynos_platform_mipi_dsim *dsim_pd;
> +
> +struct mipi_dsim_ddi {
> +	int				bus_id;
> +	struct list_head		list;
> +	struct mipi_dsim_lcd_device	*dsim_lcd_dev;
> +	struct mipi_dsim_lcd_driver	*dsim_lcd_drv;
> +};
> +
> +static LIST_HEAD(dsim_ddi_list);
> +static LIST_HEAD(dsim_lcd_dev_list);
> +
> +int exynos_mipi_dsi_register_lcd_device(struct mipi_dsim_lcd_device *lcd_dev)
> +{
> +	struct mipi_dsim_ddi *dsim_ddi;
> +
> +	if (!lcd_dev) {
> +		debug("mipi_dsim_lcd_device is NULL.\n");
> +		return -EFAULT;
> +	}
> +
> +	if (!lcd_dev->name) {
> +		debug("dsim_lcd_device name is NULL.\n");
> +		return -EFAULT;
> +	}
> +
> +	dsim_ddi = kzalloc(sizeof(struct mipi_dsim_ddi), GFP_KERNEL);
> +	if (!dsim_ddi) {
> +		debug("failed to allocate dsim_ddi object.\n");
> +		return -EFAULT;
> +	}
> +
> +	dsim_ddi->dsim_lcd_dev = lcd_dev;
> +
> +	list_add_tail(&dsim_ddi->list, &dsim_ddi_list);
> +
> +	return 0;
> +}
> +
> +struct mipi_dsim_ddi
> +	*exynos_mipi_dsi_find_lcd_device(struct mipi_dsim_lcd_driver *lcd_drv)
> +{
> +	struct mipi_dsim_ddi *dsim_ddi;
> +	struct mipi_dsim_lcd_device *lcd_dev;
> +
> +	list_for_each_entry(dsim_ddi, &dsim_ddi_list, list) {
> +		lcd_dev = dsim_ddi->dsim_lcd_dev;
> +		if (!lcd_dev)
> +			continue;
> +
> +		if (lcd_drv->id >= 0) {
> +			if ((strcmp(lcd_drv->name, lcd_dev->name)) == 0 &&
> +					lcd_drv->id == lcd_dev->id) {
> +				/**
> +				 * bus_id would be used to identify
> +				 * connected bus.
> +				 */
> +				dsim_ddi->bus_id = lcd_dev->bus_id;
> +
> +				return dsim_ddi;
> +			}
> +		} else {
> +			if ((strcmp(lcd_drv->name, lcd_dev->name)) == 0) {
> +				/**
> +				 * bus_id would be used to identify
> +				 * connected bus.
> +				 */
> +				dsim_ddi->bus_id = lcd_dev->bus_id;
> +
> +				return dsim_ddi;
> +			}
> +		}
> +
> +		kfree(dsim_ddi);
> +		list_del(&dsim_ddi_list);
> +	}
> +
> +	return NULL;
> +}
> +
> +int exynos_mipi_dsi_register_lcd_driver(struct mipi_dsim_lcd_driver *lcd_drv)
> +{
> +	struct mipi_dsim_ddi *dsim_ddi;
> +
> +	if (!lcd_drv) {
> +		debug("mipi_dsim_lcd_driver is NULL.\n");
> +		return -EFAULT;
> +	}
> +
> +	if (!lcd_drv->name) {
> +		debug("dsim_lcd_driver name is NULL.\n");
> +		return -EFAULT;
> +	}
> +
> +	dsim_ddi = exynos_mipi_dsi_find_lcd_device(lcd_drv);
> +	if (!dsim_ddi) {
> +		debug("mipi_dsim_ddi object not found.\n");
> +		return -EFAULT;
> +	}
> +
> +	dsim_ddi->dsim_lcd_drv = lcd_drv;
> +
> +	debug("registered panel driver(%s) to mipi-dsi driver.\n",
> +		lcd_drv->name);
> +
> +	return 0;
> +
> +}
> +
> +struct mipi_dsim_ddi
> +	*exynos_mipi_dsi_bind_lcd_ddi(struct mipi_dsim_device *dsim,
> +			const char *name)
> +{
> +	struct mipi_dsim_ddi *dsim_ddi;
> +	struct mipi_dsim_lcd_driver *lcd_drv;
> +	struct mipi_dsim_lcd_device *lcd_dev;
> +
> +	list_for_each_entry(dsim_ddi, &dsim_ddi_list, list) {
> +		lcd_drv = dsim_ddi->dsim_lcd_drv;
> +		lcd_dev = dsim_ddi->dsim_lcd_dev;
> +		if (!lcd_drv || !lcd_dev)
> +				continue;

Please remove one tab before 'contunue'.

> +
> +		debug("lcd_drv->id = %d, lcd_dev->id = %d\n",
> +					lcd_drv->id, lcd_dev->id);
> +
> +		if ((strcmp(lcd_drv->name, name) == 0)) {
> +			lcd_dev->master = dsim;
> +
> +			dsim->dsim_lcd_dev = lcd_dev;
> +			dsim->dsim_lcd_drv = lcd_drv;
> +
> +			return dsim_ddi;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +/* define MIPI-DSI Master operations. */
> +static struct mipi_dsim_master_ops master_ops = {
> +	.cmd_write			= exynos_mipi_dsi_wr_data,
> +	.get_dsim_frame_done		= exynos_mipi_dsi_get_frame_done_status,
> +	.clear_dsim_frame_done		= exynos_mipi_dsi_clear_frame_done,
> +};
> +
> +int exynos_mipi_dsi_init(void)
> +{
> +	struct mipi_dsim_device *dsim;
> +	struct mipi_dsim_config *dsim_config;
> +	struct mipi_dsim_ddi *dsim_ddi;
> +
> +	dsim = kzalloc(sizeof(struct mipi_dsim_device), GFP_KERNEL);
> +	if (!dsim) {
> +		debug("failed to allocate dsim object.\n");
> +		return -EFAULT;
> +	}
> +
> +	/* get mipi_dsim_config. */
> +	dsim_config = dsim_pd->dsim_config;
> +	if (dsim_config == NULL) {
> +		debug("failed to get dsim config data.\n");
> +		return -EFAULT;
> +	}
> +
> +	dsim->pd = dsim_pd;
> +	dsim->dsim_config = dsim_config;
> +	dsim->master_ops = &master_ops;
> +
> +

Please remove one empty line.

> +	/* bind lcd ddi matched with panel name. */
> +	dsim_ddi = exynos_mipi_dsi_bind_lcd_ddi(dsim, dsim_pd->lcd_panel_name);
> +	if (!dsim_ddi) {
> +		debug("mipi_dsim_ddi object not found.\n");
> +		return -ENOSYS;
> +	}

...
> diff --git a/drivers/video/exynos_mipi_dsi_common.c b/drivers/video/exynos_mipi_dsi_common.c
> new file mode 100644
> index 0000000..08611b6
> --- /dev/null
> +++ b/drivers/video/exynos_mipi_dsi_common.c
...
> +static unsigned int dpll_table[15] = {
> +	100, 120, 170, 220, 270,
> +	320, 390, 450, 510, 560,
> +	640, 690, 770, 870, 950 };

Please put '};' to the next line.

> +
> +static void exynos_mipi_dsi_long_data_wr(struct mipi_dsim_device *dsim,
> +		unsigned int data0, unsigned int data1)
> +{
> +	unsigned int data_cnt = 0, payload = 0;
> +
> +	/* in case that data count is more then 4 */
> +	for (data_cnt = 0; data_cnt < data1; data_cnt += 4) {
> +		/*
> +		 * after sending 4bytes per one time,
> +		 * send remainder data less then 4.
> +		 */
> +		if ((data1 - data_cnt) < 4) {
> +			if ((data1 - data_cnt) == 3) {
> +				payload = *(u8 *)(data0 + data_cnt) |
> +				    (*(u8 *)(data0 + (data_cnt + 1))) << 8 |

Please indent by tab in the above line, it will fit into 80 chars limit.

> +					(*(u8 *)(data0 + (data_cnt + 2))) << 16;
> +			debug("count = 3 payload = %x, %x %x %x\n",
> +				payload, *(u8 *)(data0 + data_cnt),
> +				*(u8 *)(data0 + (data_cnt + 1)),
> +				*(u8 *)(data0 + (data_cnt + 2)));
> +			} else if ((data1 - data_cnt) == 2) {
> +				payload = *(u8 *)(data0 + data_cnt) |
> +					(*(u8 *)(data0 + (data_cnt + 1))) << 8;
> +			debug("count = 2 payload = %x, %x %x\n", payload,
> +				*(u8 *)(data0 + data_cnt),
> +				*(u8 *)(data0 + (data_cnt + 1)));
> +			} else if ((data1 - data_cnt) == 1) {
> +				payload = *(u8 *)(data0 + data_cnt);
> +			}
> +
> +			exynos_mipi_dsi_wr_tx_data(dsim, payload);
> +		/* send 4bytes per one time. */
> +		} else {
> +			payload = *(u8 *)(data0 + data_cnt) |
> +				(*(u8 *)(data0 + (data_cnt + 1))) << 8 |
> +				(*(u8 *)(data0 + (data_cnt + 2))) << 16 |
> +				(*(u8 *)(data0 + (data_cnt + 3))) << 24;
> +
> +			debug("count = 4 payload = %x, %x %x %x %x\n",
> +				payload, *(u8 *)(data0 + data_cnt),
> +				*(u8 *)(data0 + (data_cnt + 1)),
> +				*(u8 *)(data0 + (data_cnt + 2)),
> +				*(u8 *)(data0 + (data_cnt + 3)));
> +
> +			exynos_mipi_dsi_wr_tx_data(dsim, payload);
> +		}

exynos_mipi_dsi_wr_tx_data() is called in both above cases, so you can
simplify to call exynos_mipi_dsi_wr_tx_data() only here.

> +	}
> +}
> +
> +int exynos_mipi_dsi_wr_data(struct mipi_dsim_device *dsim, unsigned int data_id,
> +	unsigned int data0, unsigned int data1)
> +{
> +	unsigned int timeout = TRY_GET_FIFO_TIMEOUT;
> +	unsigned long delay_val, delay;
> +	unsigned int check_rx_ack = 0;
> +
> +	if (dsim->state == DSIM_STATE_ULPS) {
> +		debug("state is ULPS.\n");
> +
> +		return -EINVAL;
> +	}
> +
> +	delay_val = MHZ / dsim->dsim_config->esc_clk;
> +	delay = 10 * delay_val;
> +
> +	udelay(delay * 1000);

we have a common mdelay(), so please use

	mdelay(delay);

> +
> +	/* only if transfer mode is LPDT, wait SFR becomes empty. */
> +	if (dsim->state == DSIM_STATE_STOP) {
> +		while (!(exynos_mipi_dsi_get_fifo_state(dsim) &
> +				SFR_HEADER_EMPTY)) {
> +			if ((timeout--) > 0)
> +				udelay(1000);
> +			else {
> +				debug("SRF header fifo is not empty.\n");
> +				return -EINVAL;
> +			}
> +		}
> +	}
> +
> +	switch (data_id) {
> +	/* short packet types of packet types for command. */
> +	case MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM:
> +	case MIPI_DSI_GENERIC_SHORT_WRITE_1_PARAM:
> +	case MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM:
> +	case MIPI_DSI_DCS_SHORT_WRITE:
> +	case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
> +	case MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE:
> +		debug("data0 = %x data1 = %x\n",
> +				data0, data1);
> +		exynos_mipi_dsi_wr_tx_header(dsim, data_id, data0, data1);
> +		if (check_rx_ack)
> +			/* process response func should be implemented */
> +			return 0;
> +		else
> +			return -EINVAL;

Please use following style for multiline if statements

                if (check_rx_ack) {
                        /* process response func should be implemented */
                        return 0;
                } else {
                        return -EINVAL;
                }

> +
> +	/* general command */
> +	case MIPI_DSI_COLOR_MODE_OFF:
> +	case MIPI_DSI_COLOR_MODE_ON:
> +	case MIPI_DSI_SHUTDOWN_PERIPHERAL:
> +	case MIPI_DSI_TURN_ON_PERIPHERAL:
> +		exynos_mipi_dsi_wr_tx_header(dsim, data_id, data0, data1);
> +		if (check_rx_ack)
> +			/* process response func should be implemented. */
> +			return 0;
> +		else
> +			return -EINVAL;

same applies here. There are other similar statements in this file.
Please fix them too.

> +
> +	/* packet types for video data */
> +	case MIPI_DSI_V_SYNC_START:
> +	case MIPI_DSI_V_SYNC_END:
> +	case MIPI_DSI_H_SYNC_START:
> +	case MIPI_DSI_H_SYNC_END:
> +	case MIPI_DSI_END_OF_TRANSMISSION:
> +		return 0;
> +
> +	/* short and response packet types for command */
> +	case MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM:
> +	case MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM:
> +	case MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM:
> +	case MIPI_DSI_DCS_READ:
> +		exynos_mipi_dsi_clear_all_interrupt(dsim);
> +		exynos_mipi_dsi_wr_tx_header(dsim, data_id, data0, data1);
> +		/* process response func should be implemented. */
> +		return 0;
> +
> +	/* long packet type and null packet */
> +	case MIPI_DSI_NULL_PACKET:
> +	case MIPI_DSI_BLANKING_PACKET:
> +		return 0;
> +	case MIPI_DSI_GENERIC_LONG_WRITE:
> +	case MIPI_DSI_DCS_LONG_WRITE:
> +	{
> +		unsigned int size, data_cnt = 0, payload = 0;
> +
> +		size = data1 * 4;
> +

The local variable 'size' is not really used here, I get a compiler
warning:
exynos_mipi_dsi_common.c: In function 'exynos_mipi_dsi_wr_data':
exynos_mipi_dsi_common.c:200:16: warning: variable 'size' set but not used
[-Wunused-but-set-variable]

So please drop it.

> +		/* if data count is less then 4, then send 3bytes data.  */
> +		if (data1 < 4) {
> +			payload = *(u8 *)(data0) |
> +				*(u8 *)(data0 + 1) << 8 |
> +				*(u8 *)(data0 + 2) << 16;
> +
> +			exynos_mipi_dsi_wr_tx_data(dsim, payload);
> +
> +			debug("count = %d payload = %x,%x %x %x\n",
> +				data1, payload,
> +				*(u8 *)(data0 + data_cnt),
> +				*(u8 *)(data0 + (data_cnt + 1)),
> +				*(u8 *)(data0 + (data_cnt + 2)));
> +		/* in case that data count is more then 4 */
> +		} else
> +			exynos_mipi_dsi_long_data_wr(dsim, data0, data1);

		} else {
			/* in case that data count is more then 4 */
			exynos_mipi_dsi_long_data_wr(dsim, data0, data1);
		}

> +
> +		/* put data into header fifo */
> +		exynos_mipi_dsi_wr_tx_header(dsim, data_id, data1 & 0xff,
> +			(data1 & 0xff00) >> 8);
> +
> +	}
> +	if (check_rx_ack)
> +		/* process response func should be implemented. */
> +		return 0;
> +	else
> +		return -EINVAL;
> +
> +	/* packet typo for video data */
> +	case MIPI_DSI_PACKED_PIXEL_STREAM_16:
> +	case MIPI_DSI_PACKED_PIXEL_STREAM_18:
> +	case MIPI_DSI_PIXEL_STREAM_3BYTE_18:
> +	case MIPI_DSI_PACKED_PIXEL_STREAM_24:
> +		if (check_rx_ack)
> +			/* process response func should be implemented. */
> +			return 0;
> +		else
> +			return -EINVAL;
> +	default:
> +		debug("data id %x is not supported current DSI spec.\n",
> +			data_id);
> +
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

...
> +int exynos_mipi_dsi_set_clock(struct mipi_dsim_device *dsim,
> +	unsigned int byte_clk_sel, unsigned int enable)
> +{
> +	unsigned int esc_div;
> +	unsigned long esc_clk_error_rate;
> +	unsigned long hs_clk = 0, byte_clk = 0, escape_clk = 0;
> +
> +	if (enable) {
> +		dsim->e_clk_src = byte_clk_sel;
> +
> +		/* Escape mode clock and byte clock source */
> +		exynos_mipi_dsi_set_byte_clock_src(dsim, byte_clk_sel);
> +
> +		/* DPHY, DSIM Link : D-PHY clock out */
> +		if (byte_clk_sel == DSIM_PLL_OUT_DIV8) {
> +			hs_clk = exynos_mipi_dsi_change_pll(dsim,
> +				dsim->dsim_config->p, dsim->dsim_config->m,
> +				dsim->dsim_config->s);
> +			if (hs_clk == 0) {
> +				debug("failed to get hs clock.\n");
> +				return -EINVAL;
> +			}
> +
> +			byte_clk = hs_clk / 8;
> +			exynos_mipi_dsi_enable_pll_bypass(dsim, 0);
> +			exynos_mipi_dsi_pll_on(dsim, 1);
> +		/* DPHY : D-PHY clock out, DSIM link : external clock out */
> +		} else if (byte_clk_sel == DSIM_EXT_CLK_DIV8)
> +			debug("not support EXT CLK source for MIPI DSIM\n");
> +		else if (byte_clk_sel == DSIM_EXT_CLK_BYPASS)
> +			debug("not support EXT CLK source for MIPI DSIM\n");
> +
> +		/* escape clock divider */
> +		esc_div = byte_clk / (dsim->dsim_config->esc_clk);
> +		debug("esc_div = %d, byte_clk = %lu, esc_clk = %lu\n",
> +			esc_div, byte_clk, dsim->dsim_config->esc_clk);
> +		if ((byte_clk / esc_div) >= (20 * MHZ) ||
> +				(byte_clk / esc_div) >
> +					dsim->dsim_config->esc_clk)
> +			esc_div += 1;

                if ((byte_clk / esc_div) >= (20 * MHZ) ||
                    (byte_clk / esc_div) > dsim->dsim_config->esc_clk)
                        esc_div += 1;

...
> +int exynos_mipi_dsi_init_link(struct mipi_dsim_device *dsim)
> +{
> +	unsigned int time_out = 100;
> +
> +	switch (dsim->state) {
> +	case DSIM_STATE_INIT:
> +		exynos_mipi_dsi_init_fifo_pointer(dsim, 0x1f);
> +
> +		/* dsi configuration */
> +		exynos_mipi_dsi_init_config(dsim);
> +		exynos_mipi_dsi_enable_lane(dsim, DSIM_LANE_CLOCK, 1);
> +		exynos_mipi_dsi_enable_lane(dsim, dsim->data_lane, 1);
> +
> +		/* set clock configuration */
> +		exynos_mipi_dsi_set_clock(dsim,
> +					dsim->dsim_config->e_byte_clk, 1);
> +
> +		/* check clock and data lane state are stop state */
> +		while (!(exynos_mipi_dsi_is_lane_state(dsim))) {
> +			time_out--;
> +			if (time_out == 0) {
> +				debug("DSI Master is not stop state.\n");
> +				debug("Check initialization process\n");
> +
> +				return -EINVAL;
> +			}
> +		}
> +
> +		if (time_out != 0) {

This check is not necessary here since this condition is always true.
Please remove it.

> +			debug("DSI Master driver has been completed.\n");
> +			debug("DSI Master state is stop state\n");
> +		}
> +
> +		dsim->state = DSIM_STATE_STOP;
> +
> +		/* BTA sequence counters */
> +		exynos_mipi_dsi_set_stop_state_counter(dsim,
> +			dsim->dsim_config->stop_holding_cnt);
> +		exynos_mipi_dsi_set_bta_timeout(dsim,
> +			dsim->dsim_config->bta_timeout);
> +		exynos_mipi_dsi_set_lpdr_timeout(dsim,
> +			dsim->dsim_config->rx_timeout);
> +
> +

Please drop empty line.

> +		return 0;
> +	default:
> +		debug("DSI Master is already init.\n");
> +		return 0;
> +	}
> +
> +	return 0;
> +}

...
> diff --git a/drivers/video/exynos_mipi_dsi_lowlevel.c b/drivers/video/exynos_mipi_dsi_lowlevel.c
> new file mode 100644
> index 0000000..cc52ede
> --- /dev/null
> +++ b/drivers/video/exynos_mipi_dsi_lowlevel.c
...

> +#include <common.h>
> +#include <lcd.h>
> +#include <asm/errno.h>

Please drop lcd.h and asm/errno.h

> +#include <asm/arch/dsim.h>
> +#include <asm/arch/mipi_dsim.h>
> +#include <asm/arch/power.h>
> +#include <asm/arch/cpu.h>
> +#include <linux/types.h>
> +#include <linux/list.h>

Please drop linux/types.h and linux/list.h

...
> +void exynos_mipi_dsi_sw_release(struct mipi_dsim_device *dsim)
> +{
> +	struct exynos_mipi_dsim *mipi_dsim =
> +		(struct exynos_mipi_dsim *)samsung_get_base_mipi_dsim();
> +	unsigned int reg = readl(&mipi_dsim->intsrc);
> +
> +	reg |= INTSRC_SWRST_RELEASE;
> +
> +	writel(reg, &mipi_dsim->intsrc);
> +
> +	reg = 0;
> +	reg = readl(&mipi_dsim->intsrc);

Please replace the above two lines by

	readl(&mipi_dsim->intsrc);

...
> +void exynos_mipi_dsi_init_config(struct mipi_dsim_device *dsim)
> +{
> +	struct mipi_dsim_config *dsim_config = dsim->dsim_config;
> +	struct exynos_mipi_dsim *mipi_dsim =
> +		(struct exynos_mipi_dsim *)samsung_get_base_mipi_dsim();
> +	unsigned int cfg = (readl(&mipi_dsim->config)) &
> +		~((1 << DSIM_EOT_PACKET_SHIFT) |
> +		(0x1f << DSIM_HSA_MODE_SHIFT) |
> +		(0x3 << DSIM_NUM_OF_DATALANE_SHIFT));
> +
> +	cfg =	(dsim_config->auto_flush << DSIM_AUTO_FLUSH_SHIFT) |

Shouldn't this be
	cfg |=  (dsim_config->auto_flush << DSIM_AUTO_FLUSH_SHIFT) |
?

> +		(dsim_config->eot_disable << DSIM_EOT_PACKET_SHIFT) |
> +		(dsim_config->auto_vertical_cnt << DSIM_AUTO_MODE_SHIFT) |
> +		(dsim_config->hse << DSIM_HSE_MODE_SHIFT) |
> +		(dsim_config->hfp << DSIM_HFP_MODE_SHIFT) |
> +		(dsim_config->hbp << DSIM_HBP_MODE_SHIFT) |
> +		(dsim_config->hsa << DSIM_HSA_MODE_SHIFT) |
> +		(dsim_config->e_no_data_lane << DSIM_NUM_OF_DATALANE_SHIFT);
> +
> +	writel(cfg, &mipi_dsim->config);
> +}

...
> +void exynos_mipi_dsi_enable_lane(struct mipi_dsim_device *dsim,
> +			unsigned int lane, unsigned int enable)
> +{
> +	unsigned int reg;
> +	struct exynos_mipi_dsim *mipi_dsim =
> +		(struct exynos_mipi_dsim *)samsung_get_base_mipi_dsim();
> +
> +	reg = readl(&mipi_dsim->config);
> +
> +	if (enable)
> +		reg |= DSIM_LANE_ENx(lane);
> +	else
> +		reg &= ~DSIM_LANE_ENx(lane);
> +
> +	writel(reg, &mipi_dsim->config);
> +}
> +
> +

Please drop one empty line.

...

> +void exynos_mipi_dsi_enable_esc_clk_on_lane(struct mipi_dsim_device *dsim,
> +		unsigned int lane_sel, unsigned int enable)
> +{
> +	struct exynos_mipi_dsim *mipi_dsim =
> +		(struct exynos_mipi_dsim *)samsung_get_base_mipi_dsim();
> +	unsigned int reg = readl(&mipi_dsim->clkctrl);
> +
> +	if (enable)
> +		reg |= DSIM_LANE_ESC_CLKEN(lane_sel);
> +	else
> +

Please drop empty line.

> +		reg &= ~DSIM_LANE_ESC_CLKEN(lane_sel);
> +
> +	writel(reg, &mipi_dsim->clkctrl);
> +}

...

> +void exynos_mipi_dsi_clear_all_interrupt(struct mipi_dsim_device *dsim)
> +{
> +	struct exynos_mipi_dsim *mipi_dsim =
> +		(struct exynos_mipi_dsim *)samsung_get_base_mipi_dsim();
> +	unsigned int reg = readl(&mipi_dsim->intsrc);
> +
> +	reg |= 0xffffffff;
> +
> +	writel(reg, &mipi_dsim->intsrc);

Is reading the intsrc register necessary here? Can't you just do
	writel(0xffffffff, &mipi_dsim->intsrc);
?

...
> +unsigned int exynos_mipi_dsi_get_fifo_state(struct mipi_dsim_device *dsim)
> +{
> +	unsigned int ret;
> +	struct exynos_mipi_dsim *mipi_dsim =
> +		(struct exynos_mipi_dsim *)samsung_get_base_mipi_dsim();
> +
> +	ret = readl(&mipi_dsim->fifoctrl) & ~(0x1f);
> +
> +	return ret;

you can eliminate local variable 'ret' here and just do

	return readl(&mipi_dsim->fifoctrl) & ~(0x1f);
> +}

Thanks,
Anatolij