From mboxrd@z Thu Jan  1 00:00:00 1970
From: Anatolij Gustschin <agust@denx.de>
Date: Fri, 6 Apr 2012 01:25:09 +0200
Subject: [U-Boot] [PATCH 6/7] LCD: support S6E8AX0 amoled driver based
 on EXYNOS MIPI DSI.
In-Reply-To: <4F7D3BCC.9060003@samsung.com>
References: <4F7D3BCC.9060003@samsung.com>
Message-ID: <20120406012509.1451a2af@wker>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi,

This patch looks pretty good, only a few comments below.
Please fix and I'll take a look and add by ACK then, so
that the patch can be merged through u-boot-samsung tree.

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

> This patch support S6E8AX0 amoled driver based on EXYNOS MIPI DSI interface.
> 
> 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>
> ---
>  drivers/video/s6e8ax0.c |  289 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 289 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/video/s6e8ax0.c
> 
> diff --git a/drivers/video/s6e8ax0.c b/drivers/video/s6e8ax0.c
> new file mode 100644
> index 0000000..364e137
> --- /dev/null
> +++ b/drivers/video/s6e8ax0.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>

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

> +
> +#include "exynos_mipi_dsi_lowlevel.h"
> +#include "exynos_mipi_dsi_common.h"
...

> +static void s6e8ax0_etc_power_control(struct mipi_dsim_device *dsim_dev)
> +{
> +	struct mipi_dsim_master_ops *ops = dsim_dev->master_ops;
> +	const unsigned char data_to_send[] = {
> +		0xf4, 0xcf, 0x0a, 0x12, 0x10, 0x19, 0x33, 0x02
> +	};
> +
> +	ops->cmd_write(dsim_dev, MIPI_DSI_DCS_LONG_WRITE,
> +		(unsigned int)data_to_send, ARRAY_SIZE(data_to_send));
> +}

Please add an empty line here.

> +static void s6e8ax0_etc_mipi_control3(struct mipi_dsim_device *dsim_dev)
> +{
> +	struct mipi_dsim_master_ops *ops = dsim_dev->master_ops;

Declaration and code should be separated, please add an empty line here.

> +	ops->cmd_write(dsim_dev, MIPI_DSI_DCS_SHORT_WRITE_PARAM, 0xe3, 0x40);
> +}
> +
> +static void s6e8ax0_etc_mipi_control4(struct mipi_dsim_device *dsim_dev)
> +{
> +	struct mipi_dsim_master_ops *ops = dsim_dev->master_ops;
> +	const unsigned char data_to_send[] = {
> +		0xe4, 0x00, 0x00, 0x14, 0x80, 0x00, 0x00, 0x00
> +	};
> +
> +	ops->cmd_write(dsim_dev, MIPI_DSI_DCS_LONG_WRITE,
> +		(unsigned int)data_to_send, ARRAY_SIZE(data_to_send));
> +}

...
> +static void s6e8ax0_display_on(struct mipi_dsim_device *dsim_dev)
> +{
> +	struct mipi_dsim_master_ops *ops = dsim_dev->master_ops;

please add an empty line here.

> +	ops->cmd_write(dsim_dev,
> +		MIPI_DSI_DCS_SHORT_WRITE, 0x29, 0x00);
> +}
> +
> +static void s6e8ax0_sleep_out(struct mipi_dsim_device *dsim_dev)
> +{
> +	struct mipi_dsim_master_ops *ops = dsim_dev->master_ops;

please add an empty line here.

> +	ops->cmd_write(dsim_dev,
> +		MIPI_DSI_DCS_SHORT_WRITE, 0x11, 0x00);
> +}
> +

...
> +static void s6e8ax0_panel_init(struct mipi_dsim_device *dsim_dev)
> +{
> +	/*
> +	 * in case of setting gamma and panel condition at first,
> +	 * it shuold be setting like below.
> +	 * set_gamma() -> set_panel_condition()
> +	 */
> +
> +	s6e8ax0_apply_level1_key(dsim_dev);
> +	s6e8ax0_apply_mtp_key(dsim_dev);
> +
> +	s6e8ax0_sleep_out(dsim_dev);
> +	udelay(5 * 1000);

Please use mdelay(5);

Also as mentioned in comments to the patch 4/7, please extend this
patch to add COBJS-$(CONFIG_S6E8AX0) += s6e8ax0.o to the Makefile.

Thanks,
Anatolij