[PATCH v6 00/14] drm/exynos: support LCD I80 interface display

[PATCH v6 00/14] drm/exynos: support LCD I80 interface display

[YoungJun Cho]

Hi,

This series adds LCD I80 interface display support for Exynos DRM driver.
The FIMD(display controller) specification describes it as "LCD I80 interface"
and the DSI specification describes it as "Command mode interface".

This is based on exynos-drm-next branch.

The previous patches,
RFC: http://www.spinics.net/lists/dri-devel/msg58898.html
V1: http://www.spinics.net/lists/dri-devel/msg59291.html
V2: http://www.spinics.net/lists/dri-devel/msg59867.html
V3: http://www.spinics.net/lists/dri-devel/msg60708.html
V4: http://www.spinics.net/lists/dri-devel/msg60943.html
V5: http://www.spinics.net/lists/dri-devel/msg62956.html

Changelog v2:
- Fixes typo and removes unnecessary error log. (commented by Andrzej Hazda)
- Adds missed pendlig_flip flag clear points. (commented by Daniel Kurtz)

Changelog v3:
- Removes generic command mode and command mode display timing interface.
- Moves I80 interface timings from panel DT to the FIMD(display controller) DT.

Changelog v4:
- Removes exynos5 sysreg(syscon) DT bindings and node from dtsi because
  it was already updated by linux-samsung-soc. (commented by Vivek Gautam)

Changelog v5:
- Fixes FIMD vidcon0 register relevant code.
- Fixes panel gamma table, disable sequence.
- Slitely updates for code cleanup.

Changelog v6:
- Removes pass TE host ops in dsi and exynos dsi uses TE irq handler instead,
  and it is related with the TE GPIO of panel. (commented by Thierry Reding)

Patches 1 and 2 fix trivial bugs.

Patches 3, 4, 5 and 6 implement FIMD(display controller) I80 interface.
The MIPI DSI command mode based panel generates Tearing Effect synchronization
signal between MCU and FB to display video image, and FIMD should trigger to
transfer video image at this signal.
So the panel generates it and the dsi should receive the TE IRQ and call TE
handler chains to notify it to the FIMD.

Patches 7 and 8 implement to use Exynos5410 / 5420 / 5440 SoC DSI driver
which is different from previous Exynos4 SoCs for some registers control.

Patches 9 and 10 introduce MIPI DSI command mode based Samsung S6E3FA0 AMOLED
5.7" LCD drm panel driver.

The ohters add DT property nodes to support MIPI DSI command mode.

I welcome any comments.

Thank you.
Best regards YJ

YoungJun Cho (14):
  drm/exynos: dsi: move the EoT packets configuration point
  drm/exynos: use wait_event_timeout() for safety usage
  ARM: dts: samsung-fimd: add LCD I80 interface specific properties
  drm/exynos: add TE handler to support LCD I80 interface
  drm/exynos: dsi: add TE interrupt handler to support LCD I80 interface
  drm/exynos: fimd: support LCD I80 interface
  ARM: dts: exynos_dsim: add exynos5410 compatible to DT bindings
  drm/exynos: dsi: add driver data to support Exynos5410/5420/5440 SoCs
  ARM: dts: s6e3fa0: add DT bindings
  drm/panel: add S6E3FA0 driver
  ARM: dts: exynos4: add system register property
  ARM: dts: exynos5: add system register property
  ARM: dts: exynos5420: add mipi-phy node
  ARM: dts: exynos5420: add dsi node

 .../devicetree/bindings/panel/samsung,s6e3fa0.txt  |  46 ++
 .../devicetree/bindings/video/exynos_dsim.txt      |   4 +-
 .../devicetree/bindings/video/samsung-fimd.txt     |  28 ++
 arch/arm/boot/dts/exynos4.dtsi                     |   1 +
 arch/arm/boot/dts/exynos5.dtsi                     |   1 +
 arch/arm/boot/dts/exynos5420.dtsi                  |  20 +
 drivers/gpu/drm/exynos/Kconfig                     |   1 +
 drivers/gpu/drm/exynos/exynos_drm_crtc.c           |  15 +-
 drivers/gpu/drm/exynos/exynos_drm_crtc.h           |   7 +
 drivers/gpu/drm/exynos/exynos_drm_drv.h            |   3 +
 drivers/gpu/drm/exynos/exynos_drm_dsi.c            | 266 +++++++++-
 drivers/gpu/drm/exynos/exynos_drm_fimd.c           | 276 +++++++++--
 drivers/gpu/drm/panel/Kconfig                      |   7 +
 drivers/gpu/drm/panel/Makefile                     |   1 +
 drivers/gpu/drm/panel/panel-s6e3fa0.c              | 541 +++++++++++++++++++++
 include/video/samsung_fimd.h                       |   3 +-
 16 files changed, 1146 insertions(+), 74 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
 create mode 100644 drivers/gpu/drm/panel/panel-s6e3fa0.c

-- 
1.9.0

[PATCH v6 01/14] drm/exynos: dsi: move the EoT packets configuration point

[YoungJun Cho]

This configuration could be used in MIPI DSI command mode also.
And adds user manual description for display configuration.

Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 2df3592..58bfb2a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -468,13 +468,20 @@ static int exynos_dsi_init_link(struct exynos_dsi *dsi)
 	/* DSI configuration */
 	reg = 0;
 
+	/*
+	 * The first bit of mode_flags specifies display configuration.
+	 * If this bit is set[= MIPI_DSI_MODE_VIDEO], dsi will support video
+	 * mode, otherwise it will support command mode.
+	 */
 	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
 		reg |= DSIM_VIDEO_MODE;
 
+		/*
+		 * The user manual describes that following bits are ignored in
+		 * command mode.
+		 */
 		if (!(dsi->mode_flags & MIPI_DSI_MODE_VSYNC_FLUSH))
 			reg |= DSIM_MFLUSH_VS;
-		if (!(dsi->mode_flags & MIPI_DSI_MODE_EOT_PACKET))
-			reg |= DSIM_EOT_DISABLE;
 		if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
 			reg |= DSIM_SYNC_INFORM;
 		if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST)
@@ -491,6 +498,9 @@ static int exynos_dsi_init_link(struct exynos_dsi *dsi)
 			reg |= DSIM_HSA_MODE;
 	}
 
+	if (!(dsi->mode_flags & MIPI_DSI_MODE_EOT_PACKET))
+		reg |= DSIM_EOT_DISABLE;
+
 	switch (dsi->format) {
 	case MIPI_DSI_FMT_RGB888:
 		reg |= DSIM_MAIN_PIX_FORMAT_RGB888;
-- 
1.9.0

[PATCH v6 02/14] drm/exynos: use wait_event_timeout() for safety usage

[YoungJun Cho]

There could be the case that the page flip operation isn't finished correctly
with some abnormal condition such as panel reset. So this patch replaces
wait_event() with wait_event_timeout() to avoid waiting for page flip completion
infinitely.
And clears exynos_crtc->pending_flip in exynos_drm_crtc_page_flip()
when exynos_drm_crtc_mode_set_commit() is failed.

Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index 95c9435..3bf091d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -69,8 +69,10 @@ static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode)
 
 	if (mode > DRM_MODE_DPMS_ON) {
 		/* wait for the completion of page flip. */
-		wait_event(exynos_crtc->pending_flip_queue,
-				atomic_read(&exynos_crtc->pending_flip) == 0);
+		if (!wait_event_timeout(exynos_crtc->pending_flip_queue,
+				!atomic_read(&exynos_crtc->pending_flip),
+				HZ/20))
+			atomic_set(&exynos_crtc->pending_flip, 0);
 		drm_vblank_off(crtc->dev, exynos_crtc->pipe);
 	}
 
@@ -259,6 +261,7 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
 			spin_lock_irq(&dev->event_lock);
 			drm_vblank_put(dev, exynos_crtc->pipe);
 			list_del(&event->base.link);
+			atomic_set(&exynos_crtc->pending_flip, 0);
 			spin_unlock_irq(&dev->event_lock);
 
 			goto out;
-- 
1.9.0

[PATCH v6 03/14] ARM: dts: samsung-fimd: add LCD I80 interface specific properties

[YoungJun Cho]

In case of using MIPI DSI based I80 interface panel,
the relevant registers should be set.
So this patch adds relevant DT bindings.

Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 .../devicetree/bindings/video/samsung-fimd.txt     | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/devicetree/bindings/video/samsung-fimd.txt b/Documentation/devicetree/bindings/video/samsung-fimd.txt
index 2dad41b..59ff61e 100644
--- a/Documentation/devicetree/bindings/video/samsung-fimd.txt
+++ b/Documentation/devicetree/bindings/video/samsung-fimd.txt
@@ -44,6 +44,34 @@ Optional Properties:
 - display-timings: timing settings for FIMD, as described in document [1].
 		Can be used in case timings cannot be provided otherwise
 		or to override timings provided by the panel.
+- samsung,sysreg: handle to syscon used to control the system registers
+- i80-if-timings: timing configuration for lcd i80 interface support.
+  - cs-setup: clock cycles for the active period of address signal is enabled
+              until chip select is enabled.
+              If not specified, the default value(0) will be used.
+  - wr-setup: clock cycles for the active period of CS signal is enabled until
+              write signal is enabled.
+              If not specified, the default value(0) will be used.
+  - wr-active: clock cycles for the active period of CS is enabled.
+               If not specified, the default value(1) will be used.
+  - wr-hold: clock cycles for the active period of CS is disabled until write
+             signal is disabled.
+             If not specified, the default value(0) will be used.
+
+  The parameters are defined as:
+
+    VCLK(internal)  __|¯¯¯¯¯¯|_____|¯¯¯¯¯¯|_____|¯¯¯¯¯¯|_____|¯¯¯¯¯¯|_____|¯¯
+                      :            :            :            :            :
+    Address Output  --:<XXXXXXXXXXX:XXXXXXXXXXXX:XXXXXXXXXXXX:XXXXXXXXXXXX:XX
+                      | cs-setup+1 |            :            :            :
+                      |<---------->|            :            :            :
+    Chip Select     ¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯|____________:____________:____________|¯¯
+                                   | wr-setup+1 |            | wr-hold+1  |
+                                   |<---------->|            |<---------->|
+    Write Enable    ¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯|____________|¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯
+                                                | wr-active+1|
+                                                |<---------->|
+    Video Data      ----------------------------<XXXXXXXXXXXXXXXXXXXXXXXXX>--
 
 The device node can contain 'port' child nodes according to the bindings defined
 in [2]. The following are properties specific to those nodes:
-- 
1.9.0

[PATCH v6 04/14] drm/exynos: add TE handler to support LCD I80 interface

[YoungJun Cho]

To support LCD I80 interface, the panel should generate
Tearing Effect synchronization signal between MCU and FB
to display video images.
And the display controller should trigger to transfer
video image at this signal.
So the panel receives the TE IRQ, then calls these handler
chains to notify it to the display controller.

Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c | 8 ++++++++
 drivers/gpu/drm/exynos/exynos_drm_crtc.h | 7 +++++++
 drivers/gpu/drm/exynos/exynos_drm_drv.h  | 3 +++
 3 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index 3bf091d..b68e58f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -511,3 +511,11 @@ int exynos_drm_crtc_get_pipe_from_type(struct drm_device *drm_dev,
 
 	return -EPERM;
 }
+
+void exynos_drm_crtc_te_handler(struct drm_crtc *crtc)
+{
+	struct exynos_drm_manager *manager = to_exynos_crtc(crtc)->manager;
+
+	if (manager->ops->te_handler)
+		manager->ops->te_handler(manager);
+}
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.h b/drivers/gpu/drm/exynos/exynos_drm_crtc.h
index 9f74b10..690dcdd 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.h
@@ -36,4 +36,11 @@ void exynos_drm_crtc_plane_disable(struct drm_crtc *crtc, int zpos);
 int exynos_drm_crtc_get_pipe_from_type(struct drm_device *drm_dev,
 					unsigned int out_type);
 
+/*
+ * This function calls the crtc device(manager)'s te_handler() callback
+ * to trigger to transfer video image at the tearing effect synchronization
+ * signal.
+ */
+void exynos_drm_crtc_te_handler(struct drm_crtc *crtc);
+
 #endif
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index 02f3b3d..13be498 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -186,6 +186,8 @@ struct exynos_drm_display {
  * @win_commit: apply hardware specific overlay data to registers.
  * @win_enable: enable hardware specific overlay.
  * @win_disable: disable hardware specific overlay.
+ * @te_handler: trigger to transfer video image at the tearing effect
+ *	synchronization signal if there is a page flip request.
  */
 struct exynos_drm_manager;
 struct exynos_drm_manager_ops {
@@ -204,6 +206,7 @@ struct exynos_drm_manager_ops {
 	void (*win_commit)(struct exynos_drm_manager *mgr, int zpos);
 	void (*win_enable)(struct exynos_drm_manager *mgr, int zpos);
 	void (*win_disable)(struct exynos_drm_manager *mgr, int zpos);
+	void (*te_handler)(struct exynos_drm_manager *mgr);
 };
 
 /*
-- 
1.9.0

[PATCH v6 05/14] drm/exynos: dsi: add TE interrupt handler to support LCD I80 interface

[YoungJun Cho]

To support LCD I80 interface, the DSI host should register
TE interrupt handler from the TE GPIO of attached panel.
So the panel generates a tearing effect synchronization signal
then the DSI host calls the CRTC device manager to trigger
to transfer video image.

Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 95 ++++++++++++++++++++++++++++++++-
 1 file changed, 93 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 58bfb2a..4997bfe 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -16,7 +16,9 @@
 #include <drm/drm_panel.h>
 
 #include <linux/clk.h>
+#include <linux/gpio/consumer.h>
 #include <linux/irq.h>
+#include <linux/of_gpio.h>
 #include <linux/phy/phy.h>
 #include <linux/regulator/consumer.h>
 #include <linux/component.h>
@@ -24,6 +26,7 @@
 #include <video/mipi_display.h>
 #include <video/videomode.h>
 
+#include "exynos_drm_crtc.h"
 #include "exynos_drm_drv.h"
 
 /* returns true iff both arguments logically differs */
@@ -247,6 +250,7 @@ struct exynos_dsi {
 	struct clk *bus_clk;
 	struct regulator_bulk_data supplies[2];
 	int irq;
+	int te_gpio;
 
 	u32 pll_clk_rate;
 	u32 burst_clk_rate;
@@ -954,17 +958,89 @@ static irqreturn_t exynos_dsi_irq(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t exynos_dsi_te_irq_handler(int irq, void *dev_id)
+{
+	struct exynos_dsi *dsi = (struct exynos_dsi *)dev_id;
+	struct drm_encoder *encoder = dsi->encoder;
+
+	if (dsi->state & DSIM_STATE_ENABLED)
+		exynos_drm_crtc_te_handler(encoder->crtc);
+
+	return IRQ_HANDLED;
+}
+
+static void exynos_dsi_enable_irq(struct exynos_dsi *dsi)
+{
+	enable_irq(dsi->irq);
+
+	if (gpio_is_valid(dsi->te_gpio))
+		enable_irq(gpio_to_irq(dsi->te_gpio));
+}
+
+static void exynos_dsi_disable_irq(struct exynos_dsi *dsi)
+{
+	if (gpio_is_valid(dsi->te_gpio))
+		disable_irq(gpio_to_irq(dsi->te_gpio));
+
+	disable_irq(dsi->irq);
+}
+
 static int exynos_dsi_init(struct exynos_dsi *dsi)
 {
 	exynos_dsi_enable_clock(dsi);
 	exynos_dsi_reset(dsi);
-	enable_irq(dsi->irq);
+	exynos_dsi_enable_irq(dsi);
 	exynos_dsi_wait_for_reset(dsi);
 	exynos_dsi_init_link(dsi);
 
 	return 0;
 }
 
+static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi)
+{
+	int ret;
+
+	dsi->te_gpio = of_get_named_gpio(dsi->panel_node, "te-gpios", 0);
+	if (!gpio_is_valid(dsi->te_gpio)) {
+		dev_err(dsi->dev, "no te-gpios specified\n");
+		ret = dsi->te_gpio;
+		goto out;
+	}
+
+	ret = gpio_request_one(dsi->te_gpio, GPIOF_IN, "te_gpio");
+	if (ret) {
+		dev_err(dsi->dev, "gpio request failed with %d\n", ret);
+		goto out;
+	}
+
+	/*
+	 * This TE GPIO IRQ should not be set to IRQ_NOAUTOEN, because panel
+	 * calls drm_panel_init() first then calls mipi_dsi_attach() in probe().
+	 * It means that te_gpio is invalid when exynos_dsi_enable_irq() is
+	 * called by drm_panel_init() before panel is attached.
+	 */
+	ret = request_threaded_irq(gpio_to_irq(dsi->te_gpio),
+					exynos_dsi_te_irq_handler, NULL,
+					IRQF_TRIGGER_RISING, "TE", dsi);
+	if (ret) {
+		dev_err(dsi->dev, "request interrupt failed with %d\n", ret);
+		gpio_free(dsi->te_gpio);
+		goto out;
+	}
+
+out:
+	return ret;
+}
+
+static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi)
+{
+	if (gpio_is_valid(dsi->te_gpio)) {
+		free_irq(gpio_to_irq(dsi->te_gpio), dsi);
+		gpio_free(dsi->te_gpio);
+		dsi->te_gpio = -ENOENT;
+	}
+}
+
 static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 				  struct mipi_dsi_device *device)
 {
@@ -978,6 +1054,16 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 	if (dsi->connector.dev)
 		drm_helper_hpd_irq_event(dsi->connector.dev);
 
+	/*
+	 * If attached panel device is for command mode one, dsi should
+	 * register TE interrupt handler.
+	 */
+	if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO)) {
+		int ret = exynos_dsi_register_te_irq(dsi);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -986,6 +1072,8 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
 {
 	struct exynos_dsi *dsi = host_to_dsi(host);
 
+	exynos_dsi_unregister_te_irq(dsi);
+
 	dsi->panel_node = NULL;
 
 	if (dsi->connector.dev)
@@ -1099,7 +1187,7 @@ static void exynos_dsi_poweroff(struct exynos_dsi *dsi)
 
 		exynos_dsi_disable_clock(dsi);
 
-		disable_irq(dsi->irq);
+		exynos_dsi_disable_irq(dsi);
 	}
 
 	dsi->state &= ~DSIM_STATE_CMD_LPM;
@@ -1445,6 +1533,9 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 		goto err_del_component;
 	}
 
+	/* To be checked as invalid one */
+	dsi->te_gpio = -ENOENT;
+
 	init_completion(&dsi->completed);
 	spin_lock_init(&dsi->transfer_lock);
 	INIT_LIST_HEAD(&dsi->transfer_list);
-- 
1.9.0

Re: [PATCH v6 05/14] drm/exynos: dsi: add TE interrupt handler to support LCD I80 interface

[Andrzej Hajda]

On 07/17/2014 11:01 AM, YoungJun Cho wrote:
> To support LCD I80 interface, the DSI host should register
> TE interrupt handler from the TE GPIO of attached panel.
> So the panel generates a tearing effect synchronization signal
> then the DSI host calls the CRTC device manager to trigger
> to transfer video image.
> 

This whole patch seems to be a hack.

I think it is not a good idea to parse property of one device by another
device's driver.

Especially here TE GPIO belongs to the panel. The panel driver should
know how to configure it and how to use it or not. The panel driver will
generate TE signal based on this GPIO, DSI/BTA mechanism or any other
mechanism provided by the panel.
TE signal should be delivered to the display controller. The only role
of DSIM here is that it is between the panel and the display controller
so it should be used to route this signal to DC.

According to specs TE signal should/can be generated by DBI and DSI
command mode panels, so its handling should be more generic, not Exynos
specific.

Regards
Andrzej

> Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
> Acked-by: Inki Dae <inki.dae@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 95 ++++++++++++++++++++++++++++++++-
>  1 file changed, 93 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index 58bfb2a..4997bfe 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -16,7 +16,9 @@
>  #include <drm/drm_panel.h>
>  
>  #include <linux/clk.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/irq.h>
> +#include <linux/of_gpio.h>
>  #include <linux/phy/phy.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/component.h>
> @@ -24,6 +26,7 @@
>  #include <video/mipi_display.h>
>  #include <video/videomode.h>
>  
> +#include "exynos_drm_crtc.h"
>  #include "exynos_drm_drv.h"
>  
>  /* returns true iff both arguments logically differs */
> @@ -247,6 +250,7 @@ struct exynos_dsi {
>  	struct clk *bus_clk;
>  	struct regulator_bulk_data supplies[2];
>  	int irq;
> +	int te_gpio;
>  
>  	u32 pll_clk_rate;
>  	u32 burst_clk_rate;
> @@ -954,17 +958,89 @@ static irqreturn_t exynos_dsi_irq(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static irqreturn_t exynos_dsi_te_irq_handler(int irq, void *dev_id)
> +{
> +	struct exynos_dsi *dsi = (struct exynos_dsi *)dev_id;
> +	struct drm_encoder *encoder = dsi->encoder;
> +
> +	if (dsi->state & DSIM_STATE_ENABLED)
> +		exynos_drm_crtc_te_handler(encoder->crtc);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void exynos_dsi_enable_irq(struct exynos_dsi *dsi)
> +{
> +	enable_irq(dsi->irq);
> +
> +	if (gpio_is_valid(dsi->te_gpio))
> +		enable_irq(gpio_to_irq(dsi->te_gpio));
> +}
> +
> +static void exynos_dsi_disable_irq(struct exynos_dsi *dsi)
> +{
> +	if (gpio_is_valid(dsi->te_gpio))
> +		disable_irq(gpio_to_irq(dsi->te_gpio));
> +
> +	disable_irq(dsi->irq);
> +}
> +
>  static int exynos_dsi_init(struct exynos_dsi *dsi)
>  {
>  	exynos_dsi_enable_clock(dsi);
>  	exynos_dsi_reset(dsi);
> -	enable_irq(dsi->irq);
> +	exynos_dsi_enable_irq(dsi);
>  	exynos_dsi_wait_for_reset(dsi);
>  	exynos_dsi_init_link(dsi);
>  
>  	return 0;
>  }
>  
> +static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi)
> +{
> +	int ret;
> +
> +	dsi->te_gpio = of_get_named_gpio(dsi->panel_node, "te-gpios", 0);
> +	if (!gpio_is_valid(dsi->te_gpio)) {
> +		dev_err(dsi->dev, "no te-gpios specified\n");
> +		ret = dsi->te_gpio;
> +		goto out;
> +	}
> +
> +	ret = gpio_request_one(dsi->te_gpio, GPIOF_IN, "te_gpio");
> +	if (ret) {
> +		dev_err(dsi->dev, "gpio request failed with %d\n", ret);
> +		goto out;
> +	}
> +
> +	/*
> +	 * This TE GPIO IRQ should not be set to IRQ_NOAUTOEN, because panel
> +	 * calls drm_panel_init() first then calls mipi_dsi_attach() in probe().
> +	 * It means that te_gpio is invalid when exynos_dsi_enable_irq() is
> +	 * called by drm_panel_init() before panel is attached.
> +	 */
> +	ret = request_threaded_irq(gpio_to_irq(dsi->te_gpio),
> +					exynos_dsi_te_irq_handler, NULL,
> +					IRQF_TRIGGER_RISING, "TE", dsi);
> +	if (ret) {
> +		dev_err(dsi->dev, "request interrupt failed with %d\n", ret);
> +		gpio_free(dsi->te_gpio);
> +		goto out;
> +	}
> +
> +out:
> +	return ret;
> +}
> +
> +static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi)
> +{
> +	if (gpio_is_valid(dsi->te_gpio)) {
> +		free_irq(gpio_to_irq(dsi->te_gpio), dsi);
> +		gpio_free(dsi->te_gpio);
> +		dsi->te_gpio = -ENOENT;
> +	}
> +}
> +
>  static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>  				  struct mipi_dsi_device *device)
>  {
> @@ -978,6 +1054,16 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>  	if (dsi->connector.dev)
>  		drm_helper_hpd_irq_event(dsi->connector.dev);
>  
> +	/*
> +	 * If attached panel device is for command mode one, dsi should
> +	 * register TE interrupt handler.
> +	 */
> +	if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO)) {
> +		int ret = exynos_dsi_register_te_irq(dsi);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -986,6 +1072,8 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>  {
>  	struct exynos_dsi *dsi = host_to_dsi(host);
>  
> +	exynos_dsi_unregister_te_irq(dsi);
> +
>  	dsi->panel_node = NULL;
>  
>  	if (dsi->connector.dev)
> @@ -1099,7 +1187,7 @@ static void exynos_dsi_poweroff(struct exynos_dsi *dsi)
>  
>  		exynos_dsi_disable_clock(dsi);
>  
> -		disable_irq(dsi->irq);
> +		exynos_dsi_disable_irq(dsi);
>  	}
>  
>  	dsi->state &= ~DSIM_STATE_CMD_LPM;
> @@ -1445,6 +1533,9 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>  		goto err_del_component;
>  	}
>  
> +	/* To be checked as invalid one */
> +	dsi->te_gpio = -ENOENT;
> +
>  	init_completion(&dsi->completed);
>  	spin_lock_init(&dsi->transfer_lock);
>  	INIT_LIST_HEAD(&dsi->transfer_list);
> 

Re: [PATCH v6 05/14] drm/exynos: dsi: add TE interrupt handler to support LCD I80 interface

[Inki Dae]

On 2014년 07월 21일 23:01, Andrzej Hajda wrote:
> On 07/17/2014 11:01 AM, YoungJun Cho wrote:
>> To support LCD I80 interface, the DSI host should register
>> TE interrupt handler from the TE GPIO of attached panel.
>> So the panel generates a tearing effect synchronization signal
>> then the DSI host calls the CRTC device manager to trigger
>> to transfer video image.
>>
> 
> This whole patch seems to be a hack.
> 
> I think it is not a good idea to parse property of one device by another
> device's driver.
> 
> Especially here TE GPIO belongs to the panel. The panel driver should
> know how to configure it and how to use it or not. The panel driver will
> generate TE signal based on this GPIO, DSI/BTA mechanism or any other
> mechanism provided by the panel.
> TE signal should be delivered to the display controller. The only role
> of DSIM here is that it is between the panel and the display controller
> so it should be used to route this signal to DC.
> 
> According to specs TE signal should/can be generated by DBI and DSI
> command mode panels, so its handling should be more generic, not Exynos
> specific.
> 

Right. However, it seems that there are no much users using command mode
panel so we would need more times to discuss the generic way. I think we
can have this feature in specific to Exynos drm - it doesn't affect
other SoC -.  Actually, I know OMAP people handle this feature in a way
specific to OMAP SoC. If other users need more generic way to this
feature then we could have a discussion about the generic way at that time.

That is why Mr. Cho implemented TE feature like this. Do you have more
good idea? I wish the idea would be specific so that Mr. Cho can
implement it.

P.s. Thierry already opposed the use of mipi_dsi_host_ops, I agree with
him. And also it seems not good to use crtc and encoder/connector
because these frameworks are common to all architecture including x86 so
other architectures wouldn't need TE feature.

Thanks,
Inki Dae

> Regards
> Andrzej
> 
>> Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
>> Acked-by: Inki Dae <inki.dae@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 95 ++++++++++++++++++++++++++++++++-
>>  1 file changed, 93 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>> index 58bfb2a..4997bfe 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>> @@ -16,7 +16,9 @@
>>  #include <drm/drm_panel.h>
>>  
>>  #include <linux/clk.h>
>> +#include <linux/gpio/consumer.h>
>>  #include <linux/irq.h>
>> +#include <linux/of_gpio.h>
>>  #include <linux/phy/phy.h>
>>  #include <linux/regulator/consumer.h>
>>  #include <linux/component.h>
>> @@ -24,6 +26,7 @@
>>  #include <video/mipi_display.h>
>>  #include <video/videomode.h>
>>  
>> +#include "exynos_drm_crtc.h"
>>  #include "exynos_drm_drv.h"
>>  
>>  /* returns true iff both arguments logically differs */
>> @@ -247,6 +250,7 @@ struct exynos_dsi {
>>  	struct clk *bus_clk;
>>  	struct regulator_bulk_data supplies[2];
>>  	int irq;
>> +	int te_gpio;
>>  
>>  	u32 pll_clk_rate;
>>  	u32 burst_clk_rate;
>> @@ -954,17 +958,89 @@ static irqreturn_t exynos_dsi_irq(int irq, void *dev_id)
>>  	return IRQ_HANDLED;
>>  }
>>  
>> +static irqreturn_t exynos_dsi_te_irq_handler(int irq, void *dev_id)
>> +{
>> +	struct exynos_dsi *dsi = (struct exynos_dsi *)dev_id;
>> +	struct drm_encoder *encoder = dsi->encoder;
>> +
>> +	if (dsi->state & DSIM_STATE_ENABLED)
>> +		exynos_drm_crtc_te_handler(encoder->crtc);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static void exynos_dsi_enable_irq(struct exynos_dsi *dsi)
>> +{
>> +	enable_irq(dsi->irq);
>> +
>> +	if (gpio_is_valid(dsi->te_gpio))
>> +		enable_irq(gpio_to_irq(dsi->te_gpio));
>> +}
>> +
>> +static void exynos_dsi_disable_irq(struct exynos_dsi *dsi)
>> +{
>> +	if (gpio_is_valid(dsi->te_gpio))
>> +		disable_irq(gpio_to_irq(dsi->te_gpio));
>> +
>> +	disable_irq(dsi->irq);
>> +}
>> +
>>  static int exynos_dsi_init(struct exynos_dsi *dsi)
>>  {
>>  	exynos_dsi_enable_clock(dsi);
>>  	exynos_dsi_reset(dsi);
>> -	enable_irq(dsi->irq);
>> +	exynos_dsi_enable_irq(dsi);
>>  	exynos_dsi_wait_for_reset(dsi);
>>  	exynos_dsi_init_link(dsi);
>>  
>>  	return 0;
>>  }
>>  
>> +static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi)
>> +{
>> +	int ret;
>> +
>> +	dsi->te_gpio = of_get_named_gpio(dsi->panel_node, "te-gpios", 0);
>> +	if (!gpio_is_valid(dsi->te_gpio)) {
>> +		dev_err(dsi->dev, "no te-gpios specified\n");
>> +		ret = dsi->te_gpio;
>> +		goto out;
>> +	}
>> +
>> +	ret = gpio_request_one(dsi->te_gpio, GPIOF_IN, "te_gpio");
>> +	if (ret) {
>> +		dev_err(dsi->dev, "gpio request failed with %d\n", ret);
>> +		goto out;
>> +	}
>> +
>> +	/*
>> +	 * This TE GPIO IRQ should not be set to IRQ_NOAUTOEN, because panel
>> +	 * calls drm_panel_init() first then calls mipi_dsi_attach() in probe().
>> +	 * It means that te_gpio is invalid when exynos_dsi_enable_irq() is
>> +	 * called by drm_panel_init() before panel is attached.
>> +	 */
>> +	ret = request_threaded_irq(gpio_to_irq(dsi->te_gpio),
>> +					exynos_dsi_te_irq_handler, NULL,
>> +					IRQF_TRIGGER_RISING, "TE", dsi);
>> +	if (ret) {
>> +		dev_err(dsi->dev, "request interrupt failed with %d\n", ret);
>> +		gpio_free(dsi->te_gpio);
>> +		goto out;
>> +	}
>> +
>> +out:
>> +	return ret;
>> +}
>> +
>> +static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi)
>> +{
>> +	if (gpio_is_valid(dsi->te_gpio)) {
>> +		free_irq(gpio_to_irq(dsi->te_gpio), dsi);
>> +		gpio_free(dsi->te_gpio);
>> +		dsi->te_gpio = -ENOENT;
>> +	}
>> +}
>> +
>>  static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>  				  struct mipi_dsi_device *device)
>>  {
>> @@ -978,6 +1054,16 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>  	if (dsi->connector.dev)
>>  		drm_helper_hpd_irq_event(dsi->connector.dev);
>>  
>> +	/*
>> +	 * If attached panel device is for command mode one, dsi should
>> +	 * register TE interrupt handler.
>> +	 */
>> +	if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO)) {
>> +		int ret = exynos_dsi_register_te_irq(dsi);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>  	return 0;
>>  }
>>  
>> @@ -986,6 +1072,8 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>>  {
>>  	struct exynos_dsi *dsi = host_to_dsi(host);
>>  
>> +	exynos_dsi_unregister_te_irq(dsi);
>> +
>>  	dsi->panel_node = NULL;
>>  
>>  	if (dsi->connector.dev)
>> @@ -1099,7 +1187,7 @@ static void exynos_dsi_poweroff(struct exynos_dsi *dsi)
>>  
>>  		exynos_dsi_disable_clock(dsi);
>>  
>> -		disable_irq(dsi->irq);
>> +		exynos_dsi_disable_irq(dsi);
>>  	}
>>  
>>  	dsi->state &= ~DSIM_STATE_CMD_LPM;
>> @@ -1445,6 +1533,9 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>>  		goto err_del_component;
>>  	}
>>  
>> +	/* To be checked as invalid one */
>> +	dsi->te_gpio = -ENOENT;
>> +
>>  	init_completion(&dsi->completed);
>>  	spin_lock_init(&dsi->transfer_lock);
>>  	INIT_LIST_HEAD(&dsi->transfer_list);
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 05/14] drm/exynos: dsi: add TE interrupt handler to support LCD I80 interface

[YoungJun Cho]

Hi,

On 07/22/2014 10:23 AM, Inki Dae wrote:
> On 2014년 07월 21일 23:01, Andrzej Hajda wrote:
>> On 07/17/2014 11:01 AM, YoungJun Cho wrote:
>>> To support LCD I80 interface, the DSI host should register
>>> TE interrupt handler from the TE GPIO of attached panel.
>>> So the panel generates a tearing effect synchronization signal
>>> then the DSI host calls the CRTC device manager to trigger
>>> to transfer video image.
>>>
>>
>> This whole patch seems to be a hack.
>>
>> I think it is not a good idea to parse property of one device by another
>> device's driver.
>>
>> Especially here TE GPIO belongs to the panel. The panel driver should
>> know how to configure it and how to use it or not. The panel driver will
>> generate TE signal based on this GPIO, DSI/BTA mechanism or any other
>> mechanism provided by the panel.
>> TE signal should be delivered to the display controller. The only role
>> of DSIM here is that it is between the panel and the display controller
>> so it should be used to route this signal to DC.

It looks like DSIM transfers only TE signal to FIMD, but there is one 
thing important role, DSIM transfers TE signal only it is activated.
Without this thing, a broken screen would be showed at booting time.

>>
>> According to specs TE signal should/can be generated by DBI and DSI
>> command mode panels, so its handling should be more generic, not Exynos
>> specific.
>>
>
> Right. However, it seems that there are no much users using command mode
> panel so we would need more times to discuss the generic way. I think we
> can have this feature in specific to Exynos drm - it doesn't affect
> other SoC -.  Actually, I know OMAP people handle this feature in a way

For your information, there is a dsicm_te_isr() in 
drivers/video/fbdev/omap2/displays-new.
It seems like that panel -> dsi -> display controller.

Thank you.
Best regards YJ

> specific to OMAP SoC. If other users need more generic way to this
> feature then we could have a discussion about the generic way at that time.
>
> That is why Mr. Cho implemented TE feature like this. Do you have more
> good idea? I wish the idea would be specific so that Mr. Cho can
> implement it.
>
> P.s. Thierry already opposed the use of mipi_dsi_host_ops, I agree with
> him. And also it seems not good to use crtc and encoder/connector
> because these frameworks are common to all architecture including x86 so
> other architectures wouldn't need TE feature.
>
> Thanks,
> Inki Dae
>
>> Regards
>> Andrzej
>>
>>> Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
>>> Acked-by: Inki Dae <inki.dae@samsung.com>
>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>> ---
>>>   drivers/gpu/drm/exynos/exynos_drm_dsi.c | 95 ++++++++++++++++++++++++++++++++-
>>>   1 file changed, 93 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> index 58bfb2a..4997bfe 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> @@ -16,7 +16,9 @@
>>>   #include <drm/drm_panel.h>
>>>
>>>   #include <linux/clk.h>
>>> +#include <linux/gpio/consumer.h>
>>>   #include <linux/irq.h>
>>> +#include <linux/of_gpio.h>
>>>   #include <linux/phy/phy.h>
>>>   #include <linux/regulator/consumer.h>
>>>   #include <linux/component.h>
>>> @@ -24,6 +26,7 @@
>>>   #include <video/mipi_display.h>
>>>   #include <video/videomode.h>
>>>
>>> +#include "exynos_drm_crtc.h"
>>>   #include "exynos_drm_drv.h"
>>>
>>>   /* returns true iff both arguments logically differs */
>>> @@ -247,6 +250,7 @@ struct exynos_dsi {
>>>   	struct clk *bus_clk;
>>>   	struct regulator_bulk_data supplies[2];
>>>   	int irq;
>>> +	int te_gpio;
>>>
>>>   	u32 pll_clk_rate;
>>>   	u32 burst_clk_rate;
>>> @@ -954,17 +958,89 @@ static irqreturn_t exynos_dsi_irq(int irq, void *dev_id)
>>>   	return IRQ_HANDLED;
>>>   }
>>>
>>> +static irqreturn_t exynos_dsi_te_irq_handler(int irq, void *dev_id)
>>> +{
>>> +	struct exynos_dsi *dsi = (struct exynos_dsi *)dev_id;
>>> +	struct drm_encoder *encoder = dsi->encoder;
>>> +
>>> +	if (dsi->state & DSIM_STATE_ENABLED)
>>> +		exynos_drm_crtc_te_handler(encoder->crtc);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static void exynos_dsi_enable_irq(struct exynos_dsi *dsi)
>>> +{
>>> +	enable_irq(dsi->irq);
>>> +
>>> +	if (gpio_is_valid(dsi->te_gpio))
>>> +		enable_irq(gpio_to_irq(dsi->te_gpio));
>>> +}
>>> +
>>> +static void exynos_dsi_disable_irq(struct exynos_dsi *dsi)
>>> +{
>>> +	if (gpio_is_valid(dsi->te_gpio))
>>> +		disable_irq(gpio_to_irq(dsi->te_gpio));
>>> +
>>> +	disable_irq(dsi->irq);
>>> +}
>>> +
>>>   static int exynos_dsi_init(struct exynos_dsi *dsi)
>>>   {
>>>   	exynos_dsi_enable_clock(dsi);
>>>   	exynos_dsi_reset(dsi);
>>> -	enable_irq(dsi->irq);
>>> +	exynos_dsi_enable_irq(dsi);
>>>   	exynos_dsi_wait_for_reset(dsi);
>>>   	exynos_dsi_init_link(dsi);
>>>
>>>   	return 0;
>>>   }
>>>
>>> +static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi)
>>> +{
>>> +	int ret;
>>> +
>>> +	dsi->te_gpio = of_get_named_gpio(dsi->panel_node, "te-gpios", 0);
>>> +	if (!gpio_is_valid(dsi->te_gpio)) {
>>> +		dev_err(dsi->dev, "no te-gpios specified\n");
>>> +		ret = dsi->te_gpio;
>>> +		goto out;
>>> +	}
>>> +
>>> +	ret = gpio_request_one(dsi->te_gpio, GPIOF_IN, "te_gpio");
>>> +	if (ret) {
>>> +		dev_err(dsi->dev, "gpio request failed with %d\n", ret);
>>> +		goto out;
>>> +	}
>>> +
>>> +	/*
>>> +	 * This TE GPIO IRQ should not be set to IRQ_NOAUTOEN, because panel
>>> +	 * calls drm_panel_init() first then calls mipi_dsi_attach() in probe().
>>> +	 * It means that te_gpio is invalid when exynos_dsi_enable_irq() is
>>> +	 * called by drm_panel_init() before panel is attached.
>>> +	 */
>>> +	ret = request_threaded_irq(gpio_to_irq(dsi->te_gpio),
>>> +					exynos_dsi_te_irq_handler, NULL,
>>> +					IRQF_TRIGGER_RISING, "TE", dsi);
>>> +	if (ret) {
>>> +		dev_err(dsi->dev, "request interrupt failed with %d\n", ret);
>>> +		gpio_free(dsi->te_gpio);
>>> +		goto out;
>>> +	}
>>> +
>>> +out:
>>> +	return ret;
>>> +}
>>> +
>>> +static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi)
>>> +{
>>> +	if (gpio_is_valid(dsi->te_gpio)) {
>>> +		free_irq(gpio_to_irq(dsi->te_gpio), dsi);
>>> +		gpio_free(dsi->te_gpio);
>>> +		dsi->te_gpio = -ENOENT;
>>> +	}
>>> +}
>>> +
>>>   static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>   				  struct mipi_dsi_device *device)
>>>   {
>>> @@ -978,6 +1054,16 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>   	if (dsi->connector.dev)
>>>   		drm_helper_hpd_irq_event(dsi->connector.dev);
>>>
>>> +	/*
>>> +	 * If attached panel device is for command mode one, dsi should
>>> +	 * register TE interrupt handler.
>>> +	 */
>>> +	if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO)) {
>>> +		int ret = exynos_dsi_register_te_irq(dsi);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>>   	return 0;
>>>   }
>>>
>>> @@ -986,6 +1072,8 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>>>   {
>>>   	struct exynos_dsi *dsi = host_to_dsi(host);
>>>
>>> +	exynos_dsi_unregister_te_irq(dsi);
>>> +
>>>   	dsi->panel_node = NULL;
>>>
>>>   	if (dsi->connector.dev)
>>> @@ -1099,7 +1187,7 @@ static void exynos_dsi_poweroff(struct exynos_dsi *dsi)
>>>
>>>   		exynos_dsi_disable_clock(dsi);
>>>
>>> -		disable_irq(dsi->irq);
>>> +		exynos_dsi_disable_irq(dsi);
>>>   	}
>>>
>>>   	dsi->state &= ~DSIM_STATE_CMD_LPM;
>>> @@ -1445,6 +1533,9 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>>>   		goto err_del_component;
>>>   	}
>>>
>>> +	/* To be checked as invalid one */
>>> +	dsi->te_gpio = -ENOENT;
>>> +
>>>   	init_completion(&dsi->completed);
>>>   	spin_lock_init(&dsi->transfer_lock);
>>>   	INIT_LIST_HEAD(&dsi->transfer_list);
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
>

Re: [PATCH v6 05/14] drm/exynos: dsi: add TE interrupt handler to support LCD I80 interface

[Andrzej Hajda]

On 07/22/2014 03:23 AM, Inki Dae wrote:
> On 2014년 07월 21일 23:01, Andrzej Hajda wrote:
>> On 07/17/2014 11:01 AM, YoungJun Cho wrote:
>>> To support LCD I80 interface, the DSI host should register
>>> TE interrupt handler from the TE GPIO of attached panel.
>>> So the panel generates a tearing effect synchronization signal
>>> then the DSI host calls the CRTC device manager to trigger
>>> to transfer video image.
>>>
>> This whole patch seems to be a hack.
>>
>> I think it is not a good idea to parse property of one device by another
>> device's driver.
>>
>> Especially here TE GPIO belongs to the panel. The panel driver should
>> know how to configure it and how to use it or not. The panel driver will
>> generate TE signal based on this GPIO, DSI/BTA mechanism or any other
>> mechanism provided by the panel.
>> TE signal should be delivered to the display controller. The only role
>> of DSIM here is that it is between the panel and the display controller
>> so it should be used to route this signal to DC.
>>
>> According to specs TE signal should/can be generated by DBI and DSI
>> command mode panels, so its handling should be more generic, not Exynos
>> specific.
>>
> Right. However, it seems that there are no much users using command mode
> panel so we would need more times to discuss the generic way. I think we
> can have this feature in specific to Exynos drm - it doesn't affect
> other SoC -.  Actually, I know OMAP people handle this feature in a way
> specific to OMAP SoC. If other users need more generic way to this
> feature then we could have a discussion about the generic way at that time.
>
> That is why Mr. Cho implemented TE feature like this. Do you have more
> good idea? I wish the idea would be specific so that Mr. Cho can
> implement it.
>
> P.s. Thierry already opposed the use of mipi_dsi_host_ops, I agree with
> him. And also it seems not good to use crtc and encoder/connector
> because these frameworks are common to all architecture including x86 so
> other architectures wouldn't need TE feature.

The good thing is that DT bindings in this case are OK, TE GPIO is in
panel node.
Maybe we can leave it this way for now, but at least lets add a comment to
the code describing that it is temporary solution and should be make
more generic in the future.

Regards
Andrzej
>
> Thanks,
> Inki Dae
>
>> Regards
>> Andrzej
>>
>>> Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
>>> Acked-by: Inki Dae <inki.dae@samsung.com>
>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 95 ++++++++++++++++++++++++++++++++-
>>>  1 file changed, 93 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> index 58bfb2a..4997bfe 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>> @@ -16,7 +16,9 @@
>>>  #include <drm/drm_panel.h>
>>>  
>>>  #include <linux/clk.h>
>>> +#include <linux/gpio/consumer.h>
>>>  #include <linux/irq.h>
>>> +#include <linux/of_gpio.h>
>>>  #include <linux/phy/phy.h>
>>>  #include <linux/regulator/consumer.h>
>>>  #include <linux/component.h>
>>> @@ -24,6 +26,7 @@
>>>  #include <video/mipi_display.h>
>>>  #include <video/videomode.h>
>>>  
>>> +#include "exynos_drm_crtc.h"
>>>  #include "exynos_drm_drv.h"
>>>  
>>>  /* returns true iff both arguments logically differs */
>>> @@ -247,6 +250,7 @@ struct exynos_dsi {
>>>  	struct clk *bus_clk;
>>>  	struct regulator_bulk_data supplies[2];
>>>  	int irq;
>>> +	int te_gpio;
>>>  
>>>  	u32 pll_clk_rate;
>>>  	u32 burst_clk_rate;
>>> @@ -954,17 +958,89 @@ static irqreturn_t exynos_dsi_irq(int irq, void *dev_id)
>>>  	return IRQ_HANDLED;
>>>  }
>>>  
>>> +static irqreturn_t exynos_dsi_te_irq_handler(int irq, void *dev_id)
>>> +{
>>> +	struct exynos_dsi *dsi = (struct exynos_dsi *)dev_id;
>>> +	struct drm_encoder *encoder = dsi->encoder;
>>> +
>>> +	if (dsi->state & DSIM_STATE_ENABLED)
>>> +		exynos_drm_crtc_te_handler(encoder->crtc);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static void exynos_dsi_enable_irq(struct exynos_dsi *dsi)
>>> +{
>>> +	enable_irq(dsi->irq);
>>> +
>>> +	if (gpio_is_valid(dsi->te_gpio))
>>> +		enable_irq(gpio_to_irq(dsi->te_gpio));
>>> +}
>>> +
>>> +static void exynos_dsi_disable_irq(struct exynos_dsi *dsi)
>>> +{
>>> +	if (gpio_is_valid(dsi->te_gpio))
>>> +		disable_irq(gpio_to_irq(dsi->te_gpio));
>>> +
>>> +	disable_irq(dsi->irq);
>>> +}
>>> +
>>>  static int exynos_dsi_init(struct exynos_dsi *dsi)
>>>  {
>>>  	exynos_dsi_enable_clock(dsi);
>>>  	exynos_dsi_reset(dsi);
>>> -	enable_irq(dsi->irq);
>>> +	exynos_dsi_enable_irq(dsi);
>>>  	exynos_dsi_wait_for_reset(dsi);
>>>  	exynos_dsi_init_link(dsi);
>>>  
>>>  	return 0;
>>>  }
>>>  
>>> +static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi)
>>> +{
>>> +	int ret;
>>> +
>>> +	dsi->te_gpio = of_get_named_gpio(dsi->panel_node, "te-gpios", 0);
>>> +	if (!gpio_is_valid(dsi->te_gpio)) {
>>> +		dev_err(dsi->dev, "no te-gpios specified\n");
>>> +		ret = dsi->te_gpio;
>>> +		goto out;
>>> +	}
>>> +
>>> +	ret = gpio_request_one(dsi->te_gpio, GPIOF_IN, "te_gpio");
>>> +	if (ret) {
>>> +		dev_err(dsi->dev, "gpio request failed with %d\n", ret);
>>> +		goto out;
>>> +	}
>>> +
>>> +	/*
>>> +	 * This TE GPIO IRQ should not be set to IRQ_NOAUTOEN, because panel
>>> +	 * calls drm_panel_init() first then calls mipi_dsi_attach() in probe().
>>> +	 * It means that te_gpio is invalid when exynos_dsi_enable_irq() is
>>> +	 * called by drm_panel_init() before panel is attached.
>>> +	 */
>>> +	ret = request_threaded_irq(gpio_to_irq(dsi->te_gpio),
>>> +					exynos_dsi_te_irq_handler, NULL,
>>> +					IRQF_TRIGGER_RISING, "TE", dsi);
>>> +	if (ret) {
>>> +		dev_err(dsi->dev, "request interrupt failed with %d\n", ret);
>>> +		gpio_free(dsi->te_gpio);
>>> +		goto out;
>>> +	}
>>> +
>>> +out:
>>> +	return ret;
>>> +}
>>> +
>>> +static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi)
>>> +{
>>> +	if (gpio_is_valid(dsi->te_gpio)) {
>>> +		free_irq(gpio_to_irq(dsi->te_gpio), dsi);
>>> +		gpio_free(dsi->te_gpio);
>>> +		dsi->te_gpio = -ENOENT;
>>> +	}
>>> +}
>>> +
>>>  static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>  				  struct mipi_dsi_device *device)
>>>  {
>>> @@ -978,6 +1054,16 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>  	if (dsi->connector.dev)
>>>  		drm_helper_hpd_irq_event(dsi->connector.dev);
>>>  
>>> +	/*
>>> +	 * If attached panel device is for command mode one, dsi should
>>> +	 * register TE interrupt handler.
>>> +	 */
>>> +	if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO)) {
>>> +		int ret = exynos_dsi_register_te_irq(dsi);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>>  	return 0;
>>>  }
>>>  
>>> @@ -986,6 +1072,8 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>>>  {
>>>  	struct exynos_dsi *dsi = host_to_dsi(host);
>>>  
>>> +	exynos_dsi_unregister_te_irq(dsi);
>>> +
>>>  	dsi->panel_node = NULL;
>>>  
>>>  	if (dsi->connector.dev)
>>> @@ -1099,7 +1187,7 @@ static void exynos_dsi_poweroff(struct exynos_dsi *dsi)
>>>  
>>>  		exynos_dsi_disable_clock(dsi);
>>>  
>>> -		disable_irq(dsi->irq);
>>> +		exynos_dsi_disable_irq(dsi);
>>>  	}
>>>  
>>>  	dsi->state &= ~DSIM_STATE_CMD_LPM;
>>> @@ -1445,6 +1533,9 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>>>  		goto err_del_component;
>>>  	}
>>>  
>>> +	/* To be checked as invalid one */
>>> +	dsi->te_gpio = -ENOENT;
>>> +
>>>  	init_completion(&dsi->completed);
>>>  	spin_lock_init(&dsi->transfer_lock);
>>>  	INIT_LIST_HEAD(&dsi->transfer_list);
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 05/14] drm/exynos: dsi: add TE interrupt handler to support LCD I80 interface

[YoungJun Cho]

Hi Andrzej,

On 07/22/2014 07:12 PM, Andrzej Hajda wrote:
> On 07/22/2014 03:23 AM, Inki Dae wrote:
>> On 2014년 07월 21일 23:01, Andrzej Hajda wrote:
>>> On 07/17/2014 11:01 AM, YoungJun Cho wrote:
>>>> To support LCD I80 interface, the DSI host should register
>>>> TE interrupt handler from the TE GPIO of attached panel.
>>>> So the panel generates a tearing effect synchronization signal
>>>> then the DSI host calls the CRTC device manager to trigger
>>>> to transfer video image.
>>>>
>>> This whole patch seems to be a hack.
>>>
>>> I think it is not a good idea to parse property of one device by another
>>> device's driver.
>>>
>>> Especially here TE GPIO belongs to the panel. The panel driver should
>>> know how to configure it and how to use it or not. The panel driver will
>>> generate TE signal based on this GPIO, DSI/BTA mechanism or any other
>>> mechanism provided by the panel.
>>> TE signal should be delivered to the display controller. The only role
>>> of DSIM here is that it is between the panel and the display controller
>>> so it should be used to route this signal to DC.
>>>
>>> According to specs TE signal should/can be generated by DBI and DSI
>>> command mode panels, so its handling should be more generic, not Exynos
>>> specific.
>>>
>> Right. However, it seems that there are no much users using command mode
>> panel so we would need more times to discuss the generic way. I think we
>> can have this feature in specific to Exynos drm - it doesn't affect
>> other SoC -.  Actually, I know OMAP people handle this feature in a way
>> specific to OMAP SoC. If other users need more generic way to this
>> feature then we could have a discussion about the generic way at that time.
>>
>> That is why Mr. Cho implemented TE feature like this. Do you have more
>> good idea? I wish the idea would be specific so that Mr. Cho can
>> implement it.
>>
>> P.s. Thierry already opposed the use of mipi_dsi_host_ops, I agree with
>> him. And also it seems not good to use crtc and encoder/connector
>> because these frameworks are common to all architecture including x86 so
>> other architectures wouldn't need TE feature.
>
> The good thing is that DT bindings in this case are OK, TE GPIO is in
> panel node.
> Maybe we can leave it this way for now, but at least lets add a comment to
> the code describing that it is temporary solution and should be make
> more generic in the future.
>

Okay, I'll leave this comment at exynos_dsi_host_attach() before current 
exynos_dsi_register_te_irq() relevant comment.

Thank you.
Best regards YJ

> Regards
> Andrzej
>>
>> Thanks,
>> Inki Dae
>>
>>> Regards
>>> Andrzej
>>>
>>>> Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
>>>> Acked-by: Inki Dae <inki.dae@samsung.com>
>>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>> ---
>>>>   drivers/gpu/drm/exynos/exynos_drm_dsi.c | 95 ++++++++++++++++++++++++++++++++-
>>>>   1 file changed, 93 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>> index 58bfb2a..4997bfe 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>> @@ -16,7 +16,9 @@
>>>>   #include <drm/drm_panel.h>
>>>>
>>>>   #include <linux/clk.h>
>>>> +#include <linux/gpio/consumer.h>
>>>>   #include <linux/irq.h>
>>>> +#include <linux/of_gpio.h>
>>>>   #include <linux/phy/phy.h>
>>>>   #include <linux/regulator/consumer.h>
>>>>   #include <linux/component.h>
>>>> @@ -24,6 +26,7 @@
>>>>   #include <video/mipi_display.h>
>>>>   #include <video/videomode.h>
>>>>
>>>> +#include "exynos_drm_crtc.h"
>>>>   #include "exynos_drm_drv.h"
>>>>
>>>>   /* returns true iff both arguments logically differs */
>>>> @@ -247,6 +250,7 @@ struct exynos_dsi {
>>>>   	struct clk *bus_clk;
>>>>   	struct regulator_bulk_data supplies[2];
>>>>   	int irq;
>>>> +	int te_gpio;
>>>>
>>>>   	u32 pll_clk_rate;
>>>>   	u32 burst_clk_rate;
>>>> @@ -954,17 +958,89 @@ static irqreturn_t exynos_dsi_irq(int irq, void *dev_id)
>>>>   	return IRQ_HANDLED;
>>>>   }
>>>>
>>>> +static irqreturn_t exynos_dsi_te_irq_handler(int irq, void *dev_id)
>>>> +{
>>>> +	struct exynos_dsi *dsi = (struct exynos_dsi *)dev_id;
>>>> +	struct drm_encoder *encoder = dsi->encoder;
>>>> +
>>>> +	if (dsi->state & DSIM_STATE_ENABLED)
>>>> +		exynos_drm_crtc_te_handler(encoder->crtc);
>>>> +
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static void exynos_dsi_enable_irq(struct exynos_dsi *dsi)
>>>> +{
>>>> +	enable_irq(dsi->irq);
>>>> +
>>>> +	if (gpio_is_valid(dsi->te_gpio))
>>>> +		enable_irq(gpio_to_irq(dsi->te_gpio));
>>>> +}
>>>> +
>>>> +static void exynos_dsi_disable_irq(struct exynos_dsi *dsi)
>>>> +{
>>>> +	if (gpio_is_valid(dsi->te_gpio))
>>>> +		disable_irq(gpio_to_irq(dsi->te_gpio));
>>>> +
>>>> +	disable_irq(dsi->irq);
>>>> +}
>>>> +
>>>>   static int exynos_dsi_init(struct exynos_dsi *dsi)
>>>>   {
>>>>   	exynos_dsi_enable_clock(dsi);
>>>>   	exynos_dsi_reset(dsi);
>>>> -	enable_irq(dsi->irq);
>>>> +	exynos_dsi_enable_irq(dsi);
>>>>   	exynos_dsi_wait_for_reset(dsi);
>>>>   	exynos_dsi_init_link(dsi);
>>>>
>>>>   	return 0;
>>>>   }
>>>>
>>>> +static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	dsi->te_gpio = of_get_named_gpio(dsi->panel_node, "te-gpios", 0);
>>>> +	if (!gpio_is_valid(dsi->te_gpio)) {
>>>> +		dev_err(dsi->dev, "no te-gpios specified\n");
>>>> +		ret = dsi->te_gpio;
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	ret = gpio_request_one(dsi->te_gpio, GPIOF_IN, "te_gpio");
>>>> +	if (ret) {
>>>> +		dev_err(dsi->dev, "gpio request failed with %d\n", ret);
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * This TE GPIO IRQ should not be set to IRQ_NOAUTOEN, because panel
>>>> +	 * calls drm_panel_init() first then calls mipi_dsi_attach() in probe().
>>>> +	 * It means that te_gpio is invalid when exynos_dsi_enable_irq() is
>>>> +	 * called by drm_panel_init() before panel is attached.
>>>> +	 */
>>>> +	ret = request_threaded_irq(gpio_to_irq(dsi->te_gpio),
>>>> +					exynos_dsi_te_irq_handler, NULL,
>>>> +					IRQF_TRIGGER_RISING, "TE", dsi);
>>>> +	if (ret) {
>>>> +		dev_err(dsi->dev, "request interrupt failed with %d\n", ret);
>>>> +		gpio_free(dsi->te_gpio);
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +out:
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi)
>>>> +{
>>>> +	if (gpio_is_valid(dsi->te_gpio)) {
>>>> +		free_irq(gpio_to_irq(dsi->te_gpio), dsi);
>>>> +		gpio_free(dsi->te_gpio);
>>>> +		dsi->te_gpio = -ENOENT;
>>>> +	}
>>>> +}
>>>> +
>>>>   static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>>   				  struct mipi_dsi_device *device)
>>>>   {
>>>> @@ -978,6 +1054,16 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>>>>   	if (dsi->connector.dev)
>>>>   		drm_helper_hpd_irq_event(dsi->connector.dev);
>>>>
>>>> +	/*
>>>> +	 * If attached panel device is for command mode one, dsi should
>>>> +	 * register TE interrupt handler.
>>>> +	 */
>>>> +	if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO)) {
>>>> +		int ret = exynos_dsi_register_te_irq(dsi);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> +
>>>>   	return 0;
>>>>   }
>>>>
>>>> @@ -986,6 +1072,8 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>>>>   {
>>>>   	struct exynos_dsi *dsi = host_to_dsi(host);
>>>>
>>>> +	exynos_dsi_unregister_te_irq(dsi);
>>>> +
>>>>   	dsi->panel_node = NULL;
>>>>
>>>>   	if (dsi->connector.dev)
>>>> @@ -1099,7 +1187,7 @@ static void exynos_dsi_poweroff(struct exynos_dsi *dsi)
>>>>
>>>>   		exynos_dsi_disable_clock(dsi);
>>>>
>>>> -		disable_irq(dsi->irq);
>>>> +		exynos_dsi_disable_irq(dsi);
>>>>   	}
>>>>
>>>>   	dsi->state &= ~DSIM_STATE_CMD_LPM;
>>>> @@ -1445,6 +1533,9 @@ static int exynos_dsi_probe(struct platform_device *pdev)
>>>>   		goto err_del_component;
>>>>   	}
>>>>
>>>> +	/* To be checked as invalid one */
>>>> +	dsi->te_gpio = -ENOENT;
>>>> +
>>>>   	init_completion(&dsi->completed);
>>>>   	spin_lock_init(&dsi->transfer_lock);
>>>>   	INIT_LIST_HEAD(&dsi->transfer_list);
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v6 05/14] drm/exynos: dsi: add TE interrupt handler to support LCD I80 interface

[YoungJun Cho]

This is a temporary solution and should be made by more
generic way.

To support LCD I80 interface, the DSI host should register
TE interrupt handler from the TE GPIO of attached panel.
So the panel generates a tearing effect synchronization signal
then the DSI host calls the CRTC device manager to trigger
to transfer video image.

Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 97 ++++++++++++++++++++++++++++++++-
 1 file changed, 95 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 58bfb2a..3adad44 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -16,7 +16,9 @@
 #include <drm/drm_panel.h>
 
 #include <linux/clk.h>
+#include <linux/gpio/consumer.h>
 #include <linux/irq.h>
+#include <linux/of_gpio.h>
 #include <linux/phy/phy.h>
 #include <linux/regulator/consumer.h>
 #include <linux/component.h>
@@ -24,6 +26,7 @@
 #include <video/mipi_display.h>
 #include <video/videomode.h>
 
+#include "exynos_drm_crtc.h"
 #include "exynos_drm_drv.h"
 
 /* returns true iff both arguments logically differs */
@@ -247,6 +250,7 @@ struct exynos_dsi {
 	struct clk *bus_clk;
 	struct regulator_bulk_data supplies[2];
 	int irq;
+	int te_gpio;
 
 	u32 pll_clk_rate;
 	u32 burst_clk_rate;
@@ -954,17 +958,89 @@ static irqreturn_t exynos_dsi_irq(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t exynos_dsi_te_irq_handler(int irq, void *dev_id)
+{
+	struct exynos_dsi *dsi = (struct exynos_dsi *)dev_id;
+	struct drm_encoder *encoder = dsi->encoder;
+
+	if (dsi->state & DSIM_STATE_ENABLED)
+		exynos_drm_crtc_te_handler(encoder->crtc);
+
+	return IRQ_HANDLED;
+}
+
+static void exynos_dsi_enable_irq(struct exynos_dsi *dsi)
+{
+	enable_irq(dsi->irq);
+
+	if (gpio_is_valid(dsi->te_gpio))
+		enable_irq(gpio_to_irq(dsi->te_gpio));
+}
+
+static void exynos_dsi_disable_irq(struct exynos_dsi *dsi)
+{
+	if (gpio_is_valid(dsi->te_gpio))
+		disable_irq(gpio_to_irq(dsi->te_gpio));
+
+	disable_irq(dsi->irq);
+}
+
 static int exynos_dsi_init(struct exynos_dsi *dsi)
 {
 	exynos_dsi_enable_clock(dsi);
 	exynos_dsi_reset(dsi);
-	enable_irq(dsi->irq);
+	exynos_dsi_enable_irq(dsi);
 	exynos_dsi_wait_for_reset(dsi);
 	exynos_dsi_init_link(dsi);
 
 	return 0;
 }
 
+static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi)
+{
+	int ret;
+
+	dsi->te_gpio = of_get_named_gpio(dsi->panel_node, "te-gpios", 0);
+	if (!gpio_is_valid(dsi->te_gpio)) {
+		dev_err(dsi->dev, "no te-gpios specified\n");
+		ret = dsi->te_gpio;
+		goto out;
+	}
+
+	ret = gpio_request_one(dsi->te_gpio, GPIOF_IN, "te_gpio");
+	if (ret) {
+		dev_err(dsi->dev, "gpio request failed with %d\n", ret);
+		goto out;
+	}
+
+	/*
+	 * This TE GPIO IRQ should not be set to IRQ_NOAUTOEN, because panel
+	 * calls drm_panel_init() first then calls mipi_dsi_attach() in probe().
+	 * It means that te_gpio is invalid when exynos_dsi_enable_irq() is
+	 * called by drm_panel_init() before panel is attached.
+	 */
+	ret = request_threaded_irq(gpio_to_irq(dsi->te_gpio),
+					exynos_dsi_te_irq_handler, NULL,
+					IRQF_TRIGGER_RISING, "TE", dsi);
+	if (ret) {
+		dev_err(dsi->dev, "request interrupt failed with %d\n", ret);
+		gpio_free(dsi->te_gpio);
+		goto out;
+	}
+
+out:
+	return ret;
+}
+
+static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi)
+{
+	if (gpio_is_valid(dsi->te_gpio)) {
+		free_irq(gpio_to_irq(dsi->te_gpio), dsi);
+		gpio_free(dsi->te_gpio);
+		dsi->te_gpio = -ENOENT;
+	}
+}
+
 static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 				  struct mipi_dsi_device *device)
 {
@@ -978,6 +1054,18 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 	if (dsi->connector.dev)
 		drm_helper_hpd_irq_event(dsi->connector.dev);
 
+	/*
+	 * This is a temporary solution and should be made by more generic way.
+	 *
+	 * If attached panel device is for command mode one, dsi should register
+	 * TE interrupt handler.
+	 */
+	if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO)) {
+		int ret = exynos_dsi_register_te_irq(dsi);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
@@ -986,6 +1074,8 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
 {
 	struct exynos_dsi *dsi = host_to_dsi(host);
 
+	exynos_dsi_unregister_te_irq(dsi);
+
 	dsi->panel_node = NULL;
 
 	if (dsi->connector.dev)
@@ -1099,7 +1189,7 @@ static void exynos_dsi_poweroff(struct exynos_dsi *dsi)
 
 		exynos_dsi_disable_clock(dsi);
 
-		disable_irq(dsi->irq);
+		exynos_dsi_disable_irq(dsi);
 	}
 
 	dsi->state &= ~DSIM_STATE_CMD_LPM;
@@ -1445,6 +1535,9 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 		goto err_del_component;
 	}
 
+	/* To be checked as invalid one */
+	dsi->te_gpio = -ENOENT;
+
 	init_completion(&dsi->completed);
 	spin_lock_init(&dsi->transfer_lock);
 	INIT_LIST_HEAD(&dsi->transfer_list);
-- 
1.9.0

Re: [PATCH v6 05/14] drm/exynos: dsi: add TE interrupt handler to support LCD I80 interface

[Varka Bhadram]

On 07/22/2014 04:19 PM, YoungJun Cho wrote:

(...)

> +	ret = gpio_request_one(dsi->te_gpio, GPIOF_IN, "te_gpio");

devm_* APIs..?

> +	if (ret) {
> +		dev_err(dsi->dev, "gpio request failed with %d\n", ret);
> +		goto out;
> +	}
> +
> +	/*
> +	 * This TE GPIO IRQ should not be set to IRQ_NOAUTOEN, because panel
> +	 * calls drm_panel_init() first then calls mipi_dsi_attach() in probe().
> +	 * It means that te_gpio is invalid when exynos_dsi_enable_irq() is
> +	 * called by drm_panel_init() before panel is attached.
> +	 */
> +	ret = request_threaded_irq(gpio_to_irq(dsi->te_gpio),
> +					exynos_dsi_te_irq_handler, NULL,
> +					IRQF_TRIGGER_RISING, "TE", dsi);

why don't we use devm_request_threaded_irq()..?


-- 
Regards,
Varka Bhadram.

Re: [PATCH v6 05/14] drm/exynos: dsi: add TE interrupt handler to support LCD I80 interface

[YoungJun Cho]

Hi Varka,

This irq handler should be registered in attach() and unregistered in 
detach().

The devm_* APIs are released(freed) in remove(), right?

Logically the panel could be attached and detached several times after 
dsi is probed and not removed.
So I don't use devm_* APIs.

Thank you.
Best regards YJ

On 07/22/2014 07:57 PM, Varka Bhadram wrote:
> On 07/22/2014 04:19 PM, YoungJun Cho wrote:
>
> (...)
>
>> +    ret = gpio_request_one(dsi->te_gpio, GPIOF_IN, "te_gpio");
>
> devm_* APIs..?
>
>> +    if (ret) {
>> +        dev_err(dsi->dev, "gpio request failed with %d\n", ret);
>> +        goto out;
>> +    }
>> +
>> +    /*
>> +     * This TE GPIO IRQ should not be set to IRQ_NOAUTOEN, because panel
>> +     * calls drm_panel_init() first then calls mipi_dsi_attach() in
>> probe().
>> +     * It means that te_gpio is invalid when exynos_dsi_enable_irq() is
>> +     * called by drm_panel_init() before panel is attached.
>> +     */
>> +    ret = request_threaded_irq(gpio_to_irq(dsi->te_gpio),
>> +                    exynos_dsi_te_irq_handler, NULL,
>> +                    IRQF_TRIGGER_RISING, "TE", dsi);
>
> why don't we use devm_request_threaded_irq()..?
>
>

Re: [PATCH v6 05/14] drm/exynos: dsi: add TE interrupt handler to support LCD I80 interface

[Varka Bhadram]

On 07/22/2014 04:40 PM, YoungJun Cho wrote:
> Hi Varka,
>
> This irq handler should be registered in attach() and unregistered in 
> detach().
>
> The devm_* APIs are released(freed) in remove(), right?
>
> Logically the panel could be attached and detached several times after 
> dsi is probed and not removed.
> So I don't use devm_* APIs.

You meant to say that in-case of GPIOs also you are following the same thing ..?

Means requesting the GPIOs and Releasing several times ..?

>
>>
>


-- 
Regards,
Varka Bhadram.

Re: [PATCH v6 05/14] drm/exynos: dsi: add TE interrupt handler to support LCD I80 interface

[YoungJun Cho]

Hi Varka,

On 07/22/2014 08:14 PM, Varka Bhadram wrote:
> On 07/22/2014 04:40 PM, YoungJun Cho wrote:
>> Hi Varka,
>>
>> This irq handler should be registered in attach() and unregistered in
>> detach().
>>
>> The devm_* APIs are released(freed) in remove(), right?
>>
>> Logically the panel could be attached and detached several times after
>> dsi is probed and not removed.
>> So I don't use devm_* APIs.
>
> You meant to say that in-case of GPIOs also you are following the same
> thing ..?
>
> Means requesting the GPIOs and Releasing several times ..?
>

Yes, this TE gpio is came from panel.
So it should be refresh whenever a (new) panel is attached.

Thank you.
Best regards YJ

>>
>>>
>>
>
>

Re: [PATCH v6 05/14] drm/exynos: dsi: add TE interrupt handler to support LCD I80 interface

[Varka Bhadram]

On 07/22/2014 05:23 PM, YoungJun Cho wrote:
> Hi Varka,
>
> On 07/22/2014 08:14 PM, Varka Bhadram wrote:
>> On 07/22/2014 04:40 PM, YoungJun Cho wrote:
>>> Hi Varka,
>>>
>>> This irq handler should be registered in attach() and unregistered in
>>> detach().
>>>
>>> The devm_* APIs are released(freed) in remove(), right?
>>>
>>> Logically the panel could be attached and detached several times after
>>> dsi is probed and not removed.
>>> So I don't use devm_* APIs.
>>
>> You meant to say that in-case of GPIOs also you are following the same
>> thing ..?
>>
>> Means requesting the GPIOs and Releasing several times ..?
>>
>
> Yes, this TE gpio is came from panel.
> So it should be refresh whenever a (new) panel is attached.
>
In this case it would be fine. Thanks for the clarity.

-- 
Regards,
Varka Bhadram.

[PATCH v6 06/14] drm/exynos: fimd: support LCD I80 interface

[YoungJun Cho]

To support MIPI command mode based I80 interface panel,
FIMD should do followings:
- Sets LCD I80 interface timings configuration.
- Uses "lcd_sys" as an IRQ resource and sets relevant IRQ configuration.
- Sets LCD block configuration for I80 interface.
- Sets ideal(pixel) clock is 2 times faster than the original one
  to generate frame done IRQ prior to the next TE signal.
- Implements trigger feature that transfers image data if there is page
  flip request, and implements TE handler to call trigger function.

Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/Kconfig           |   1 +
 drivers/gpu/drm/exynos/exynos_drm_fimd.c | 276 ++++++++++++++++++++++++++-----
 include/video/samsung_fimd.h             |   3 +-
 3 files changed, 235 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/exynos/Kconfig b/drivers/gpu/drm/exynos/Kconfig
index 178d2a9..9ba1aae 100644
--- a/drivers/gpu/drm/exynos/Kconfig
+++ b/drivers/gpu/drm/exynos/Kconfig
@@ -28,6 +28,7 @@ config DRM_EXYNOS_FIMD
 	bool "Exynos DRM FIMD"
 	depends on DRM_EXYNOS && !FB_S3C
 	select FB_MODE_HELPERS
+	select MFD_SYSCON
 	help
 	  Choose this option if you want to use Exynos FIMD for DRM.
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 33161ad..28a3168 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -20,6 +20,8 @@
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/component.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
 
 #include <video/of_display_timing.h>
 #include <video/of_videomode.h>
@@ -61,6 +63,24 @@
 /* color key value register for hardware window 1 ~ 4. */
 #define WKEYCON1_BASE(x)		((WKEYCON1 + 0x140) + ((x - 1) * 8))
 
+/* I80 / RGB trigger control register */
+#define TRIGCON				0x1A4
+#define TRGMODE_I80_RGB_ENABLE_I80	(1 << 0)
+#define SWTRGCMD_I80_RGB_ENABLE		(1 << 1)
+
+/* display mode change control register except exynos4 */
+#define VIDOUT_CON			0x000
+#define VIDOUT_CON_F_I80_LDI0		(0x2 << 8)
+
+/* I80 interface control for main LDI register */
+#define I80IFCONFAx(x)			(0x1B0 + (x) * 4)
+#define I80IFCONFBx(x)			(0x1B8 + (x) * 4)
+#define LCD_CS_SETUP(x)			((x) << 16)
+#define LCD_WR_SETUP(x)			((x) << 12)
+#define LCD_WR_ACTIVE(x)		((x) << 8)
+#define LCD_WR_HOLD(x)			((x) << 4)
+#define I80IFEN_ENABLE			(1 << 0)
+
 /* FIMD has totally five hardware windows. */
 #define WINDOWS_NR	5
 
@@ -68,10 +88,14 @@
 
 struct fimd_driver_data {
 	unsigned int timing_base;
+	unsigned int lcdblk_offset;
+	unsigned int lcdblk_vt_shift;
+	unsigned int lcdblk_bypass_shift;
 
 	unsigned int has_shadowcon:1;
 	unsigned int has_clksel:1;
 	unsigned int has_limited_fmt:1;
+	unsigned int has_vidoutcon:1;
 };
 
 static struct fimd_driver_data s3c64xx_fimd_driver_data = {
@@ -82,12 +106,19 @@ static struct fimd_driver_data s3c64xx_fimd_driver_data = {
 
 static struct fimd_driver_data exynos4_fimd_driver_data = {
 	.timing_base = 0x0,
+	.lcdblk_offset = 0x210,
+	.lcdblk_vt_shift = 10,
+	.lcdblk_bypass_shift = 1,
 	.has_shadowcon = 1,
 };
 
 static struct fimd_driver_data exynos5_fimd_driver_data = {
 	.timing_base = 0x20000,
+	.lcdblk_offset = 0x214,
+	.lcdblk_vt_shift = 24,
+	.lcdblk_bypass_shift = 15,
 	.has_shadowcon = 1,
+	.has_vidoutcon = 1,
 };
 
 struct fimd_win_data {
@@ -112,15 +143,22 @@ struct fimd_context {
 	struct clk			*bus_clk;
 	struct clk			*lcd_clk;
 	void __iomem			*regs;
+	struct regmap			*sysreg;
 	struct drm_display_mode		mode;
 	struct fimd_win_data		win_data[WINDOWS_NR];
 	unsigned int			default_win;
 	unsigned long			irq_flags;
+	u32				vidcon0;
 	u32				vidcon1;
+	u32				vidout_con;
+	u32				i80ifcon;
+	bool				i80_if;
 	bool				suspended;
 	int				pipe;
 	wait_queue_head_t		wait_vsync_queue;
 	atomic_t			wait_vsync_event;
+	atomic_t			win_updated;
+	atomic_t			triggering;
 
 	struct exynos_drm_panel_info panel;
 	struct fimd_driver_data *driver_data;
@@ -243,6 +281,14 @@ static u32 fimd_calc_clkdiv(struct fimd_context *ctx,
 	unsigned long ideal_clk = mode->htotal * mode->vtotal * mode->vrefresh;
 	u32 clkdiv;
 
+	if (ctx->i80_if) {
+		/*
+		 * The frame done interrupt should be occurred prior to the
+		 * next TE signal.
+		 */
+		ideal_clk *= 2;
+	}
+
 	/* Find the clock divider value that gets us closest to ideal_clk */
 	clkdiv = DIV_ROUND_UP(clk_get_rate(ctx->lcd_clk), ideal_clk);
 
@@ -271,11 +317,10 @@ static void fimd_commit(struct exynos_drm_manager *mgr)
 {
 	struct fimd_context *ctx = mgr->ctx;
 	struct drm_display_mode *mode = &ctx->mode;
-	struct fimd_driver_data *driver_data;
-	u32 val, clkdiv, vidcon1;
-	int vsync_len, vbpd, vfpd, hsync_len, hbpd, hfpd;
+	struct fimd_driver_data *driver_data = ctx->driver_data;
+	void *timing_base = ctx->regs + driver_data->timing_base;
+	u32 val, clkdiv;
 
-	driver_data = ctx->driver_data;
 	if (ctx->suspended)
 		return;
 
@@ -283,33 +328,65 @@ static void fimd_commit(struct exynos_drm_manager *mgr)
 	if (mode->htotal == 0 || mode->vtotal == 0)
 		return;
 
-	/* setup polarity values */
-	vidcon1 = ctx->vidcon1;
-	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
-		vidcon1 |= VIDCON1_INV_VSYNC;
-	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
-		vidcon1 |= VIDCON1_INV_HSYNC;
-	writel(vidcon1, ctx->regs + driver_data->timing_base + VIDCON1);
-
-	/* setup vertical timing values. */
-	vsync_len = mode->crtc_vsync_end - mode->crtc_vsync_start;
-	vbpd = mode->crtc_vtotal - mode->crtc_vsync_end;
-	vfpd = mode->crtc_vsync_start - mode->crtc_vdisplay;
-
-	val = VIDTCON0_VBPD(vbpd - 1) |
-		VIDTCON0_VFPD(vfpd - 1) |
-		VIDTCON0_VSPW(vsync_len - 1);
-	writel(val, ctx->regs + driver_data->timing_base + VIDTCON0);
-
-	/* setup horizontal timing values.  */
-	hsync_len = mode->crtc_hsync_end - mode->crtc_hsync_start;
-	hbpd = mode->crtc_htotal - mode->crtc_hsync_end;
-	hfpd = mode->crtc_hsync_start - mode->crtc_hdisplay;
-
-	val = VIDTCON1_HBPD(hbpd - 1) |
-		VIDTCON1_HFPD(hfpd - 1) |
-		VIDTCON1_HSPW(hsync_len - 1);
-	writel(val, ctx->regs + driver_data->timing_base + VIDTCON1);
+	if (ctx->i80_if) {
+		val = ctx->i80ifcon | I80IFEN_ENABLE;
+		writel(val, timing_base + I80IFCONFAx(0));
+
+		/* disable auto frame rate */
+		writel(0, timing_base + I80IFCONFBx(0));
+
+		/* set video type selection to I80 interface */
+		if (ctx->sysreg && regmap_update_bits(ctx->sysreg,
+					driver_data->lcdblk_offset,
+					0x3 << driver_data->lcdblk_vt_shift,
+					0x1 << driver_data->lcdblk_vt_shift)) {
+			DRM_ERROR("Failed to update sysreg for I80 i/f.\n");
+			return;
+		}
+	} else {
+		int vsync_len, vbpd, vfpd, hsync_len, hbpd, hfpd;
+		u32 vidcon1;
+
+		/* setup polarity values */
+		vidcon1 = ctx->vidcon1;
+		if (mode->flags & DRM_MODE_FLAG_NVSYNC)
+			vidcon1 |= VIDCON1_INV_VSYNC;
+		if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+			vidcon1 |= VIDCON1_INV_HSYNC;
+		writel(vidcon1, ctx->regs + driver_data->timing_base + VIDCON1);
+
+		/* setup vertical timing values. */
+		vsync_len = mode->crtc_vsync_end - mode->crtc_vsync_start;
+		vbpd = mode->crtc_vtotal - mode->crtc_vsync_end;
+		vfpd = mode->crtc_vsync_start - mode->crtc_vdisplay;
+
+		val = VIDTCON0_VBPD(vbpd - 1) |
+			VIDTCON0_VFPD(vfpd - 1) |
+			VIDTCON0_VSPW(vsync_len - 1);
+		writel(val, ctx->regs + driver_data->timing_base + VIDTCON0);
+
+		/* setup horizontal timing values.  */
+		hsync_len = mode->crtc_hsync_end - mode->crtc_hsync_start;
+		hbpd = mode->crtc_htotal - mode->crtc_hsync_end;
+		hfpd = mode->crtc_hsync_start - mode->crtc_hdisplay;
+
+		val = VIDTCON1_HBPD(hbpd - 1) |
+			VIDTCON1_HFPD(hfpd - 1) |
+			VIDTCON1_HSPW(hsync_len - 1);
+		writel(val, ctx->regs + driver_data->timing_base + VIDTCON1);
+	}
+
+	if (driver_data->has_vidoutcon)
+		writel(ctx->vidout_con, timing_base + VIDOUT_CON);
+
+	/* set bypass selection */
+	if (ctx->sysreg && regmap_update_bits(ctx->sysreg,
+				driver_data->lcdblk_offset,
+				0x1 << driver_data->lcdblk_bypass_shift,
+				0x1 << driver_data->lcdblk_bypass_shift)) {
+		DRM_ERROR("Failed to update sysreg for bypass setting.\n");
+		return;
+	}
 
 	/* setup horizontal and vertical display size. */
 	val = VIDTCON2_LINEVAL(mode->vdisplay - 1) |
@@ -322,7 +399,8 @@ static void fimd_commit(struct exynos_drm_manager *mgr)
 	 * fields of register with prefix '_F' would be updated
 	 * at vsync(same as dma start)
 	 */
-	val = VIDCON0_ENVID | VIDCON0_ENVID_F;
+	val = ctx->vidcon0;
+	val |= VIDCON0_ENVID | VIDCON0_ENVID_F;
 
 	if (ctx->driver_data->has_clksel)
 		val |= VIDCON0_CLKSEL_LCD;
@@ -660,6 +738,9 @@ static void fimd_win_commit(struct exynos_drm_manager *mgr, int zpos)
 	}
 
 	win_data->enabled = true;
+
+	if (ctx->i80_if)
+		atomic_set(&ctx->win_updated, 1);
 }
 
 static void fimd_win_disable(struct exynos_drm_manager *mgr, int zpos)
@@ -838,6 +919,58 @@ static void fimd_dpms(struct exynos_drm_manager *mgr, int mode)
 	}
 }
 
+static void fimd_trigger(struct device *dev)
+{
+	struct exynos_drm_manager *mgr = get_fimd_manager(dev);
+	struct fimd_context *ctx = mgr->ctx;
+	struct fimd_driver_data *driver_data = ctx->driver_data;
+	void *timing_base = ctx->regs + driver_data->timing_base;
+	u32 reg;
+
+	atomic_set(&ctx->triggering, 1);
+
+	reg = readl(ctx->regs + VIDINTCON0);
+	reg |= (VIDINTCON0_INT_ENABLE | VIDINTCON0_INT_I80IFDONE |
+						VIDINTCON0_INT_SYSMAINCON);
+	writel(reg, ctx->regs + VIDINTCON0);
+
+	reg = readl(timing_base + TRIGCON);
+	reg |= (TRGMODE_I80_RGB_ENABLE_I80 | SWTRGCMD_I80_RGB_ENABLE);
+	writel(reg, timing_base + TRIGCON);
+}
+
+static void fimd_te_handler(struct exynos_drm_manager *mgr)
+{
+	struct fimd_context *ctx = mgr->ctx;
+
+	/* Checks the crtc is detached already from encoder */
+	if (ctx->pipe < 0 || !ctx->drm_dev)
+		return;
+
+	 /*
+	 * Skips to trigger if in triggering state, because multiple triggering
+	 * requests can cause panel reset.
+	 */
+	if (atomic_read(&ctx->triggering))
+		return;
+
+	/*
+	 * If there is a page flip request, triggers and handles the page flip
+	 * event so that current fb can be updated into panel GRAM.
+	 */
+	if (atomic_add_unless(&ctx->win_updated, -1, 0))
+		fimd_trigger(ctx->dev);
+
+	/* Wakes up vsync event queue */
+	if (atomic_read(&ctx->wait_vsync_event)) {
+		atomic_set(&ctx->wait_vsync_event, 0);
+		wake_up(&ctx->wait_vsync_queue);
+
+		if (!atomic_read(&ctx->triggering))
+			drm_handle_vblank(ctx->drm_dev, ctx->pipe);
+	}
+}
+
 static struct exynos_drm_manager_ops fimd_manager_ops = {
 	.dpms = fimd_dpms,
 	.mode_fixup = fimd_mode_fixup,
@@ -849,6 +982,7 @@ static struct exynos_drm_manager_ops fimd_manager_ops = {
 	.win_mode_set = fimd_win_mode_set,
 	.win_commit = fimd_win_commit,
 	.win_disable = fimd_win_disable,
+	.te_handler = fimd_te_handler,
 };
 
 static struct exynos_drm_manager fimd_manager = {
@@ -859,26 +993,40 @@ static struct exynos_drm_manager fimd_manager = {
 static irqreturn_t fimd_irq_handler(int irq, void *dev_id)
 {
 	struct fimd_context *ctx = (struct fimd_context *)dev_id;
-	u32 val;
+	u32 val, clear_bit;
 
 	val = readl(ctx->regs + VIDINTCON1);
 
-	if (val & VIDINTCON1_INT_FRAME)
-		/* VSYNC interrupt */
-		writel(VIDINTCON1_INT_FRAME, ctx->regs + VIDINTCON1);
+	clear_bit = ctx->i80_if ? VIDINTCON1_INT_I80 : VIDINTCON1_INT_FRAME;
+	if (val & clear_bit)
+		writel(clear_bit, ctx->regs + VIDINTCON1);
 
 	/* check the crtc is detached already from encoder */
 	if (ctx->pipe < 0 || !ctx->drm_dev)
 		goto out;
 
-	drm_handle_vblank(ctx->drm_dev, ctx->pipe);
-	exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe);
+	if (ctx->i80_if) {
+		/* unset I80 frame done interrupt */
+		val = readl(ctx->regs + VIDINTCON0);
+		val &= ~(VIDINTCON0_INT_I80IFDONE | VIDINTCON0_INT_SYSMAINCON);
+		writel(val, ctx->regs + VIDINTCON0);
 
-	/* set wait vsync event to zero and wake up queue. */
-	if (atomic_read(&ctx->wait_vsync_event)) {
-		atomic_set(&ctx->wait_vsync_event, 0);
-		wake_up(&ctx->wait_vsync_queue);
+		/* exit triggering mode */
+		atomic_set(&ctx->triggering, 0);
+
+		drm_handle_vblank(ctx->drm_dev, ctx->pipe);
+		exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe);
+	} else {
+		drm_handle_vblank(ctx->drm_dev, ctx->pipe);
+		exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe);
+
+		/* set wait vsync event to zero and wake up queue. */
+		if (atomic_read(&ctx->wait_vsync_event)) {
+			atomic_set(&ctx->wait_vsync_event, 0);
+			wake_up(&ctx->wait_vsync_queue);
+		}
 	}
+
 out:
 	return IRQ_HANDLED;
 }
@@ -923,6 +1071,7 @@ static int fimd_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct fimd_context *ctx;
+	struct device_node *i80_if_timings;
 	struct resource *res;
 	int ret = -EINVAL;
 
@@ -944,12 +1093,51 @@ static int fimd_probe(struct platform_device *pdev)
 
 	ctx->dev = dev;
 	ctx->suspended = true;
+	ctx->driver_data = drm_fimd_get_driver_data(pdev);
 
 	if (of_property_read_bool(dev->of_node, "samsung,invert-vden"))
 		ctx->vidcon1 |= VIDCON1_INV_VDEN;
 	if (of_property_read_bool(dev->of_node, "samsung,invert-vclk"))
 		ctx->vidcon1 |= VIDCON1_INV_VCLK;
 
+	i80_if_timings = of_get_child_by_name(dev->of_node, "i80-if-timings");
+	if (i80_if_timings) {
+		u32 val;
+
+		ctx->i80_if = true;
+
+		if (ctx->driver_data->has_vidoutcon)
+			ctx->vidout_con |= VIDOUT_CON_F_I80_LDI0;
+		else
+			ctx->vidcon0 |= VIDCON0_VIDOUT_I80_LDI0;
+		/*
+		 * The user manual describes that this "DSI_EN" bit is required
+		 * to enable I80 24-bit data interface.
+		 */
+		ctx->vidcon0 |= VIDCON0_DSI_EN;
+
+		if (of_property_read_u32(i80_if_timings, "cs-setup", &val))
+			val = 0;
+		ctx->i80ifcon = LCD_CS_SETUP(val);
+		if (of_property_read_u32(i80_if_timings, "wr-setup", &val))
+			val = 0;
+		ctx->i80ifcon |= LCD_WR_SETUP(val);
+		if (of_property_read_u32(i80_if_timings, "wr-active", &val))
+			val = 1;
+		ctx->i80ifcon |= LCD_WR_ACTIVE(val);
+		if (of_property_read_u32(i80_if_timings, "wr-hold", &val))
+			val = 0;
+		ctx->i80ifcon |= LCD_WR_HOLD(val);
+	}
+	of_node_put(i80_if_timings);
+
+	ctx->sysreg = syscon_regmap_lookup_by_phandle(dev->of_node,
+							"samsung,sysreg");
+	if (IS_ERR(ctx->sysreg)) {
+		dev_warn(dev, "failed to get system register.\n");
+		ctx->sysreg = NULL;
+	}
+
 	ctx->bus_clk = devm_clk_get(dev, "fimd");
 	if (IS_ERR(ctx->bus_clk)) {
 		dev_err(dev, "failed to get bus clock\n");
@@ -972,7 +1160,8 @@ static int fimd_probe(struct platform_device *pdev)
 		goto err_del_component;
 	}
 
-	res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, "vsync");
+	res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
+					   ctx->i80_if ? "lcd_sys" : "vsync");
 	if (!res) {
 		dev_err(dev, "irq request failed.\n");
 		ret = -ENXIO;
@@ -986,7 +1175,6 @@ static int fimd_probe(struct platform_device *pdev)
 		goto err_del_component;
 	}
 
-	ctx->driver_data = drm_fimd_get_driver_data(pdev);
 	init_waitqueue_head(&ctx->wait_vsync_queue);
 	atomic_set(&ctx->wait_vsync_event, 0);
 
diff --git a/include/video/samsung_fimd.h b/include/video/samsung_fimd.h
index b039320..eaad58b 100644
--- a/include/video/samsung_fimd.h
+++ b/include/video/samsung_fimd.h
@@ -19,6 +19,7 @@
 /* VIDCON0 */
 
 #define VIDCON0					0x00
+#define VIDCON0_DSI_EN				(1 << 30)
 #define VIDCON0_INTERLACE			(1 << 29)
 #define VIDCON0_VIDOUT_MASK			(0x7 << 26)
 #define VIDCON0_VIDOUT_SHIFT			26
@@ -355,7 +356,7 @@
 #define VIDINTCON0_INT_ENABLE			(1 << 0)
 
 #define VIDINTCON1				0x134
-#define VIDINTCON1_INT_I180			(1 << 2)
+#define VIDINTCON1_INT_I80			(1 << 2)
 #define VIDINTCON1_INT_FRAME			(1 << 1)
 #define VIDINTCON1_INT_FIFO			(1 << 0)
 
-- 
1.9.0

[PATCH v6 07/14] ARM: dts: exynos_dsim: add exynos5410 compatible to DT bindings

[YoungJun Cho]

This patch adds relevant to exynos5410 compatible
for exynos5410 / 5420 / 5440 SoCs support.

Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 Documentation/devicetree/bindings/video/exynos_dsim.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/video/exynos_dsim.txt b/Documentation/devicetree/bindings/video/exynos_dsim.txt
index 33b5730..31036c6 100644
--- a/Documentation/devicetree/bindings/video/exynos_dsim.txt
+++ b/Documentation/devicetree/bindings/video/exynos_dsim.txt
@@ -1,7 +1,9 @@
 Exynos MIPI DSI Master
 
 Required properties:
-  - compatible: "samsung,exynos4210-mipi-dsi"
+  - compatible: value should be one of the following
+		"samsung,exynos4210-mipi-dsi" /* for Exynos4 SoCs */
+		"samsung,exynos5410-mipi-dsi" /* for Exynos5410/5420/5440 SoCs */
   - reg: physical base address and length of the registers set for the device
   - interrupts: should contain DSI interrupt
   - clocks: list of clock specifiers, must contain an entry for each required
-- 
1.9.0

[PATCH v6 08/14] drm/exynos: dsi: add driver data to support Exynos5410/5420/5440 SoCs

[YoungJun Cho]

The offset of register DSIM_PLLTMR_REG in Exynos5410 / 5420 / 5440
SoCs is different from the one in Exynos4 SoCs.

In case of Exynos5410 / 5420 / 5440 SoCs, there is no frequency
band bit in DSIM_PLLCTRL_REG, and it uses DSIM_PHYCTRL_REG and
DSIM_PHYTIMING*_REG instead.
So this patch adds driver data to distinguish it.

Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 157 +++++++++++++++++++++++++++-----
 1 file changed, 135 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 4997bfe..93ad4a2 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -18,6 +18,7 @@
 #include <linux/clk.h>
 #include <linux/gpio/consumer.h>
 #include <linux/irq.h>
+#include <linux/of_device.h>
 #include <linux/of_gpio.h>
 #include <linux/phy/phy.h>
 #include <linux/regulator/consumer.h>
@@ -57,9 +58,12 @@
 
 /* FIFO memory AC characteristic register */
 #define DSIM_PLLCTRL_REG	0x4c	/* PLL control register */
-#define DSIM_PLLTMR_REG		0x50	/* PLL timer register */
 #define DSIM_PHYACCHR_REG	0x54	/* D-PHY AC characteristic register */
 #define DSIM_PHYACCHR1_REG	0x58	/* D-PHY AC characteristic register1 */
+#define DSIM_PHYCTRL_REG	0x5c
+#define DSIM_PHYTIMING_REG	0x64
+#define DSIM_PHYTIMING1_REG	0x68
+#define DSIM_PHYTIMING2_REG	0x6c
 
 /* DSIM_STATUS */
 #define DSIM_STOP_STATE_DAT(x)		(((x) & 0xf) << 0)
@@ -203,6 +207,24 @@
 #define DSIM_PLL_M(x)			((x) << 4)
 #define DSIM_PLL_S(x)			((x) << 1)
 
+/* DSIM_PHYCTRL */
+#define DSIM_PHYCTRL_ULPS_EXIT(x)	(((x) & 0x1ff) << 0)
+
+/* DSIM_PHYTIMING */
+#define DSIM_PHYTIMING_LPX(x)		((x) << 8)
+#define DSIM_PHYTIMING_HS_EXIT(x)	((x) << 0)
+
+/* DSIM_PHYTIMING1 */
+#define DSIM_PHYTIMING1_CLK_PREPARE(x)	((x) << 24)
+#define DSIM_PHYTIMING1_CLK_ZERO(x)	((x) << 16)
+#define DSIM_PHYTIMING1_CLK_POST(x)	((x) << 8)
+#define DSIM_PHYTIMING1_CLK_TRAIL(x)	((x) << 0)
+
+/* DSIM_PHYTIMING2 */
+#define DSIM_PHYTIMING2_HS_PREPARE(x)	((x) << 16)
+#define DSIM_PHYTIMING2_HS_ZERO(x)	((x) << 8)
+#define DSIM_PHYTIMING2_HS_TRAIL(x)	((x) << 0)
+
 #define DSI_MAX_BUS_WIDTH		4
 #define DSI_NUM_VIRTUAL_CHANNELS	4
 #define DSI_TX_FIFO_SIZE		2048
@@ -236,6 +258,12 @@ struct exynos_dsi_transfer {
 #define DSIM_STATE_INITIALIZED		BIT(1)
 #define DSIM_STATE_CMD_LPM		BIT(2)
 
+struct exynos_dsi_driver_data {
+	unsigned int plltmr_reg;
+
+	unsigned int has_freqband:1;
+};
+
 struct exynos_dsi {
 	struct mipi_dsi_host dsi_host;
 	struct drm_connector connector;
@@ -266,11 +294,39 @@ struct exynos_dsi {
 
 	spinlock_t transfer_lock; /* protects transfer_list */
 	struct list_head transfer_list;
+
+	struct exynos_dsi_driver_data *driver_data;
 };
 
 #define host_to_dsi(host) container_of(host, struct exynos_dsi, dsi_host)
 #define connector_to_dsi(c) container_of(c, struct exynos_dsi, connector)
 
+static struct exynos_dsi_driver_data exynos4_dsi_driver_data = {
+	.plltmr_reg = 0x50,
+	.has_freqband = 1,
+};
+
+static struct exynos_dsi_driver_data exynos5_dsi_driver_data = {
+	.plltmr_reg = 0x58,
+};
+
+static struct of_device_id exynos_dsi_of_match[] = {
+	{ .compatible = "samsung,exynos4210-mipi-dsi",
+	  .data = &exynos4_dsi_driver_data },
+	{ .compatible = "samsung,exynos5410-mipi-dsi",
+	  .data = &exynos5_dsi_driver_data },
+	{ }
+};
+
+static inline struct exynos_dsi_driver_data *exynos_dsi_get_driver_data(
+						struct platform_device *pdev)
+{
+	const struct of_device_id *of_id =
+			of_match_device(exynos_dsi_of_match, &pdev->dev);
+
+	return (struct exynos_dsi_driver_data *)of_id->data;
+}
+
 static void exynos_dsi_wait_for_reset(struct exynos_dsi *dsi)
 {
 	if (wait_for_completion_timeout(&dsi->completed, msecs_to_jiffies(300)))
@@ -344,14 +400,9 @@ static unsigned long exynos_dsi_pll_find_pms(struct exynos_dsi *dsi,
 static unsigned long exynos_dsi_set_pll(struct exynos_dsi *dsi,
 					unsigned long freq)
 {
-	static const unsigned long freq_bands[] = {
-		100 * MHZ, 120 * MHZ, 160 * MHZ, 200 * MHZ,
-		270 * MHZ, 320 * MHZ, 390 * MHZ, 450 * MHZ,
-		510 * MHZ, 560 * MHZ, 640 * MHZ, 690 * MHZ,
-		770 * MHZ, 870 * MHZ, 950 * MHZ,
-	};
+	struct exynos_dsi_driver_data *driver_data = dsi->driver_data;
 	unsigned long fin, fout;
-	int timeout, band;
+	int timeout;
 	u8 p, s;
 	u16 m;
 	u32 reg;
@@ -372,18 +423,30 @@ static unsigned long exynos_dsi_set_pll(struct exynos_dsi *dsi,
 			"failed to find PLL PMS for requested frequency\n");
 		return -EFAULT;
 	}
+	dev_dbg(dsi->dev, "PLL freq %lu, (p %d, m %d, s %d)\n", fout, p, m, s);
 
-	for (band = 0; band < ARRAY_SIZE(freq_bands); ++band)
-		if (fout < freq_bands[band])
-			break;
+	writel(500, dsi->reg_base + driver_data->plltmr_reg);
+
+	reg = DSIM_PLL_EN | DSIM_PLL_P(p) | DSIM_PLL_M(m) | DSIM_PLL_S(s);
+
+	if (driver_data->has_freqband) {
+		static const unsigned long freq_bands[] = {
+			100 * MHZ, 120 * MHZ, 160 * MHZ, 200 * MHZ,
+			270 * MHZ, 320 * MHZ, 390 * MHZ, 450 * MHZ,
+			510 * MHZ, 560 * MHZ, 640 * MHZ, 690 * MHZ,
+			770 * MHZ, 870 * MHZ, 950 * MHZ,
+		};
+		int band;
 
-	dev_dbg(dsi->dev, "PLL freq %lu, (p %d, m %d, s %d), band %d\n", fout,
-		p, m, s, band);
+		for (band = 0; band < ARRAY_SIZE(freq_bands); ++band)
+			if (fout < freq_bands[band])
+				break;
 
-	writel(500, dsi->reg_base + DSIM_PLLTMR_REG);
+		dev_dbg(dsi->dev, "band %d\n", band);
+
+		reg |= DSIM_FREQ_BAND(band);
+	}
 
-	reg = DSIM_FREQ_BAND(band) | DSIM_PLL_EN
-			| DSIM_PLL_P(p) | DSIM_PLL_M(m) | DSIM_PLL_S(s);
 	writel(reg, dsi->reg_base + DSIM_PLLCTRL_REG);
 
 	timeout = 1000;
@@ -437,6 +500,59 @@ static int exynos_dsi_enable_clock(struct exynos_dsi *dsi)
 	return 0;
 }
 
+static void exynos_dsi_set_phy_ctrl(struct exynos_dsi *dsi)
+{
+	struct exynos_dsi_driver_data *driver_data = dsi->driver_data;
+	u32 reg;
+
+	if (driver_data->has_freqband)
+		return;
+
+	/* B D-PHY: D-PHY Master & Slave Analog Block control */
+	reg = DSIM_PHYCTRL_ULPS_EXIT(0x0af);
+	writel(reg, dsi->reg_base + DSIM_PHYCTRL_REG);
+
+	/*
+	 * T LPX: Transmitted length of any Low-Power state period
+	 * T HS-EXIT: Time that the transmitter drives LP-11 following a HS
+	 *	burst
+	 */
+	reg = DSIM_PHYTIMING_LPX(0x06) | DSIM_PHYTIMING_HS_EXIT(0x0b);
+	writel(reg, dsi->reg_base + DSIM_PHYTIMING_REG);
+
+	/*
+	 * T CLK-PREPARE: Time that the transmitter drives the Clock Lane LP-00
+	 *	Line state immediately before the HS-0 Line state starting the
+	 *	HS transmission
+	 * T CLK-ZERO: Time that the transmitter drives the HS-0 state prior to
+	 *	transmitting the Clock.
+	 * T CLK_POST: Time that the transmitter continues to send HS clock
+	 *	after the last associated Data Lane has transitioned to LP Mode
+	 *	Interval is defined as the period from the end of T HS-TRAIL to
+	 *	the beginning of T CLK-TRAIL
+	 * T CLK-TRAIL: Time that the transmitter drives the HS-0 state after
+	 *	the last payload clock bit of a HS transmission burst
+	 */
+	reg = DSIM_PHYTIMING1_CLK_PREPARE(0x07) |
+			DSIM_PHYTIMING1_CLK_ZERO(0x27) |
+			DSIM_PHYTIMING1_CLK_POST(0x0d) |
+			DSIM_PHYTIMING1_CLK_TRAIL(0x08);
+	writel(reg, dsi->reg_base + DSIM_PHYTIMING1_REG);
+
+	/*
+	 * T HS-PREPARE: Time that the transmitter drives the Data Lane LP-00
+	 *	Line state immediately before the HS-0 Line state starting the
+	 *	HS transmission
+	 * T HS-ZERO: Time that the transmitter drives the HS-0 state prior to
+	 *	transmitting the Sync sequence.
+	 * T HS-TRAIL: Time that the transmitter drives the flipped differential
+	 *	state after last payload data bit of a HS transmission burst
+	 */
+	reg = DSIM_PHYTIMING2_HS_PREPARE(0x09) | DSIM_PHYTIMING2_HS_ZERO(0x0d) |
+			DSIM_PHYTIMING2_HS_TRAIL(0x0b);
+	writel(reg, dsi->reg_base + DSIM_PHYTIMING2_REG);
+}
+
 static void exynos_dsi_disable_clock(struct exynos_dsi *dsi)
 {
 	u32 reg;
@@ -987,10 +1103,11 @@ static void exynos_dsi_disable_irq(struct exynos_dsi *dsi)
 
 static int exynos_dsi_init(struct exynos_dsi *dsi)
 {
-	exynos_dsi_enable_clock(dsi);
 	exynos_dsi_reset(dsi);
 	exynos_dsi_enable_irq(dsi);
+	exynos_dsi_enable_clock(dsi);
 	exynos_dsi_wait_for_reset(dsi);
+	exynos_dsi_set_phy_ctrl(dsi);
 	exynos_dsi_init_link(dsi);
 
 	return 0;
@@ -1544,6 +1661,7 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 	dsi->dsi_host.dev = &pdev->dev;
 
 	dsi->dev = &pdev->dev;
+	dsi->driver_data = exynos_dsi_get_driver_data(pdev);
 
 	ret = exynos_dsi_parse_dt(dsi);
 	if (ret)
@@ -1626,11 +1744,6 @@ static int exynos_dsi_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static struct of_device_id exynos_dsi_of_match[] = {
-	{ .compatible = "samsung,exynos4210-mipi-dsi" },
-	{ }
-};
-
 struct platform_driver dsi_driver = {
 	.probe = exynos_dsi_probe,
 	.remove = exynos_dsi_remove,
-- 
1.9.0

[PATCH v6 09/14] ARM: dts: s6e3fa0: add DT bindings

[YoungJun Cho]

This patch adds DT bindings for s6e3fa0 panel.
The bindings describes panel resources and display timings.

Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 .../devicetree/bindings/panel/samsung,s6e3fa0.txt  | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt

diff --git a/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
new file mode 100644
index 0000000..2cd32f5
--- /dev/null
+++ b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
@@ -0,0 +1,46 @@
+Samsung S6E3FA0 AMOLED LCD 5.7 inch panel
+
+Required properties:
+  - compatible: "samsung,s6e3fa0"
+  - reg: the virtual channel number of a DSI peripheral
+  - vdd3-supply: core voltage supply
+  - vci-supply: voltage supply for analog circuits
+  - reset-gpios: a GPIO spec for the reset pin
+  - det-gpios: a GPIO spec for the OLED detection pin
+  - te-gpios: a GPIO spec for the TE pin
+  - display-timings: timings for the connected panel as described by [1]
+
+Optional properties:
+
+The device node can contain one 'port' child node with one child 'endpoint'
+node, according to the bindings defined in [2]. This node should describe
+panel's video bus.
+
+[1]: Documentation/devicetree/bindings/video/display-timing.txt
+[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
+
+Example:
+
+	panel@0 {
+		compatible = "samsung,s6e3fa0";
+		reg = <0>;
+		vdd3-supply = <&vcclcd_reg>;
+		vci-supply = <&vlcd_reg>;
+		reset-gpios = <&gpy7 4 0>;
+		det-gpios = <&gpg0 6 0>;
+		te-gpios = <&gpd1 7 0>;
+
+		display-timings {
+			timings0 {
+				clock-frequency = <0>;
+				hactive = <1080>;
+				vactive = <1920>;
+				hfront-porch = <2>;
+				hback-porch = <2>;
+				hsync-len = <1>;
+				vfront-porch = <1>;
+				vback-porch = <4>;
+				vsync-len = <1>;
+			};
+		};
+	};
-- 
1.9.0

Re: [PATCH v6 09/14] ARM: dts: s6e3fa0: add DT bindings

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1660 bytes --]

On Thu, Jul 17, 2014 at 06:01:24PM +0900, YoungJun Cho wrote:
> This patch adds DT bindings for s6e3fa0 panel.
> The bindings describes panel resources and display timings.

The commit message here should preferably say which platform this is
used on.

> Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
> Acked-by: Inki Dae <inki.dae@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  .../devicetree/bindings/panel/samsung,s6e3fa0.txt  | 46 ++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
> 
> diff --git a/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
> new file mode 100644
> index 0000000..2cd32f5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
> @@ -0,0 +1,46 @@
> +Samsung S6E3FA0 AMOLED LCD 5.7 inch panel
> +
> +Required properties:
> +  - compatible: "samsung,s6e3fa0"
> +  - reg: the virtual channel number of a DSI peripheral
> +  - vdd3-supply: core voltage supply
> +  - vci-supply: voltage supply for analog circuits
> +  - reset-gpios: a GPIO spec for the reset pin
> +  - det-gpios: a GPIO spec for the OLED detection pin
> +  - te-gpios: a GPIO spec for the TE pin
> +  - display-timings: timings for the connected panel as described by [1]

display-timings should be optional. The panel driver should provide a
default mode. And only if you really need to override the default mode
you should provide the option of getting an alternative set of values
from DT.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 09/14] ARM: dts: s6e3fa0: add DT bindings

[YoungJun Cho]

Hi Thierry,

On 07/17/2014 07:38 PM, Thierry Reding wrote:
> On Thu, Jul 17, 2014 at 06:01:24PM +0900, YoungJun Cho wrote:
>> This patch adds DT bindings for s6e3fa0 panel.
>> The bindings describes panel resources and display timings.
>
> The commit message here should preferably say which platform this is
> used on.

Although only exynos drm could use this panel now, I think this panel 
could be used by any platform.
Do you have any good idea for that?

>
>> Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
>> Acked-by: Inki Dae <inki.dae@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>   .../devicetree/bindings/panel/samsung,s6e3fa0.txt  | 46 ++++++++++++++++++++++
>>   1 file changed, 46 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
>>
>> diff --git a/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
>> new file mode 100644
>> index 0000000..2cd32f5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
>> @@ -0,0 +1,46 @@
>> +Samsung S6E3FA0 AMOLED LCD 5.7 inch panel
>> +
>> +Required properties:
>> +  - compatible: "samsung,s6e3fa0"
>> +  - reg: the virtual channel number of a DSI peripheral
>> +  - vdd3-supply: core voltage supply
>> +  - vci-supply: voltage supply for analog circuits
>> +  - reset-gpios: a GPIO spec for the reset pin
>> +  - det-gpios: a GPIO spec for the OLED detection pin
>> +  - te-gpios: a GPIO spec for the TE pin
>> +  - display-timings: timings for the connected panel as described by [1]
>
> display-timings should be optional. The panel driver should provide a
> default mode. And only if you really need to override the default mode
> you should provide the option of getting an alternative set of values
> from DT.

Could you explain why this display-timings should be optional?
Most of DTs regard display-timings as required property.

Thank you.
Best regards YJ

>
> Thierry
>

[PATCH v6 10/14] drm/panel: add S6E3FA0 driver

[YoungJun Cho]

This patch adds MIPI DSI command mode based
S6E3FA0 AMOLED LCD Panel driver.

Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/panel/Kconfig         |   7 +
 drivers/gpu/drm/panel/Makefile        |   1 +
 drivers/gpu/drm/panel/panel-s6e3fa0.c | 541 ++++++++++++++++++++++++++++++++++
 3 files changed, 549 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-s6e3fa0.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 4ec874d..be1392e 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -30,4 +30,11 @@ config DRM_PANEL_S6E8AA0
 	select DRM_MIPI_DSI
 	select VIDEOMODE_HELPERS
 
+config DRM_PANEL_S6E3FA0
+	tristate "S6E3FA0 DSI command mode panel"
+	depends on DRM && DRM_PANEL
+	depends on OF
+	select DRM_MIPI_DSI
+	select VIDEOMODE_HELPERS
+
 endmenu
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 8b92921..85c6738 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
 obj-$(CONFIG_DRM_PANEL_LD9040) += panel-ld9040.o
 obj-$(CONFIG_DRM_PANEL_S6E8AA0) += panel-s6e8aa0.o
+obj-$(CONFIG_DRM_PANEL_S6E3FA0) += panel-s6e3fa0.o
diff --git a/drivers/gpu/drm/panel/panel-s6e3fa0.c b/drivers/gpu/drm/panel/panel-s6e3fa0.c
new file mode 100644
index 0000000..811ec92
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-s6e3fa0.c
@@ -0,0 +1,541 @@
+/*
+ * MIPI DSI command mode based s6e3fa0 AMOLED LCD 5.7 inch drm panel driver.
+ *
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd
+ *
+ * YoungJun Cho <yj44.cho@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+
+#include <linux/gpio/consumer.h>
+#include <linux/of_gpio.h>
+#include <linux/regulator/consumer.h>
+
+#include <video/mipi_display.h>
+#include <video/of_videomode.h>
+#include <video/videomode.h>
+
+/* Manufacturer Command Set */
+#define MCS_GLOBAL_PARAMETER	0xb0
+#define MCS_AID			0xb2
+#define MCS_ELVSSOPT		0xb6
+#define MCS_TEMPERATURE_SET	0xb8
+#define MCS_PENTILE_CTRL	0xc0
+#define MCS_GAMMA_MODE		0xca
+#define MCS_VDDM		0xd7
+#define MCS_ALS			0xe3
+#define MCS_ERR_FG		0xed
+#define MCS_KEY_LEV1		0xf0
+#define MCS_GAMMA_UPDATE	0xf7
+#define MCS_KEY_LEV2		0xfc
+#define MCS_RE			0xfe
+#define MCS_TOUT2_HSYNC		0xff
+
+/* Content Adaptive Brightness Control */
+#define DCS_WRITE_CABC		0x55
+
+#define MTP_ID_LEN		3
+#define GAMMA_LEVEL_NUM		30
+
+#define DEFAULT_VDDM_VAL	0x15
+
+struct s6e3fa0 {
+	struct device			*dev;
+	struct drm_panel		panel;
+
+	struct regulator_bulk_data	supplies[2];
+	struct gpio_desc		*reset_gpio;
+	struct videomode		vm;
+
+	unsigned int			power_on_delay;
+	unsigned int			reset_delay;
+	unsigned int			init_delay;
+	unsigned int			width_mm;
+	unsigned int			height_mm;
+
+	unsigned char			id;
+	unsigned char			vddm;
+	unsigned int			brightness;
+};
+
+#define panel_to_s6e3fa0(p) container_of(p, struct s6e3fa0, panel)
+
+/* VDD Memory Lookup Table contains pairs of {ReadValue, WriteValue} */
+static const unsigned char s6e3fa0_vddm_lut[][2] = {
+	{0x00, 0x0d}, {0x01, 0x0d}, {0x02, 0x0e}, {0x03, 0x0f}, {0x04, 0x10},
+	{0x05, 0x11}, {0x06, 0x12}, {0x07, 0x13}, {0x08, 0x14}, {0x09, 0x15},
+	{0x0a, 0x16}, {0x0b, 0x17}, {0x0c, 0x18}, {0x0d, 0x19}, {0x0e, 0x1a},
+	{0x0f, 0x1b}, {0x10, 0x1c}, {0x11, 0x1d}, {0x12, 0x1e}, {0x13, 0x1f},
+	{0x14, 0x20}, {0x15, 0x21}, {0x16, 0x22}, {0x17, 0x23}, {0x18, 0x24},
+	{0x19, 0x25}, {0x1a, 0x26}, {0x1b, 0x27}, {0x1c, 0x28}, {0x1d, 0x29},
+	{0x1e, 0x2a}, {0x1f, 0x2b}, {0x20, 0x2c}, {0x21, 0x2d}, {0x22, 0x2e},
+	{0x23, 0x2f}, {0x24, 0x30}, {0x25, 0x31}, {0x26, 0x32}, {0x27, 0x33},
+	{0x28, 0x34}, {0x29, 0x35}, {0x2a, 0x36}, {0x2b, 0x37}, {0x2c, 0x38},
+	{0x2d, 0x39}, {0x2e, 0x3a}, {0x2f, 0x3b}, {0x30, 0x3c}, {0x31, 0x3d},
+	{0x32, 0x3e}, {0x33, 0x3f}, {0x34, 0x3f}, {0x35, 0x3f}, {0x36, 0x3f},
+	{0x37, 0x3f}, {0x38, 0x3f}, {0x39, 0x3f}, {0x3a, 0x3f}, {0x3b, 0x3f},
+	{0x3c, 0x3f}, {0x3d, 0x3f}, {0x3e, 0x3f}, {0x3f, 0x3f}, {0x40, 0x0c},
+	{0x41, 0x0b}, {0x42, 0x0a}, {0x43, 0x09}, {0x44, 0x08}, {0x45, 0x07},
+	{0x46, 0x06}, {0x47, 0x05}, {0x48, 0x04}, {0x49, 0x03}, {0x4a, 0x02},
+	{0x4b, 0x01}, {0x4c, 0x40}, {0x4d, 0x41}, {0x4e, 0x42}, {0x4f, 0x43},
+	{0x50, 0x44}, {0x51, 0x45}, {0x52, 0x46}, {0x53, 0x47}, {0x54, 0x48},
+	{0x55, 0x49}, {0x56, 0x4a}, {0x57, 0x4b}, {0x58, 0x4c}, {0x59, 0x4d},
+	{0x5a, 0x4e}, {0x5b, 0x4f}, {0x5c, 0x50}, {0x5d, 0x51}, {0x5e, 0x52},
+	{0x5f, 0x53}, {0x60, 0x54}, {0x61, 0x55}, {0x62, 0x56}, {0x63, 0x57},
+	{0x64, 0x58}, {0x65, 0x59}, {0x66, 0x5a}, {0x67, 0x5b}, {0x68, 0x5c},
+	{0x69, 0x5d}, {0x6a, 0x5e}, {0x6b, 0x5f}, {0x6c, 0x60}, {0x6d, 0x61},
+	{0x6e, 0x62}, {0x6f, 0x63}, {0x70, 0x64}, {0x71, 0x65}, {0x72, 0x66},
+	{0x73, 0x67}, {0x74, 0x68}, {0x75, 0x69}, {0x76, 0x6a}, {0x77, 0x6b},
+	{0x78, 0x6c}, {0x79, 0x6d}, {0x7a, 0x6e}, {0x7b, 0x6f}, {0x7c, 0x70},
+	{0x7d, 0x71}, {0x7e, 0x72}, {0x7f, 0x73},
+};
+
+static int s6e3fa0_dcs_read(struct s6e3fa0 *ctx, unsigned char cmd,
+							void *data, size_t len)
+{
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+
+	return mipi_dsi_dcs_read(dsi, dsi->channel, cmd, data, len);
+}
+
+static void s6e3fa0_dcs_write(struct s6e3fa0 *ctx, const void *data, size_t len)
+{
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+
+	mipi_dsi_dcs_write(dsi, dsi->channel, data, len);
+}
+
+#define s6e3fa0_dcs_write_seq(ctx, seq...)				\
+do {									\
+	const unsigned char d[] = { seq };				\
+	BUILD_BUG_ON_MSG(ARRAY_SIZE(d) > 64, "too big seq for stack");	\
+	s6e3fa0_dcs_write(ctx, d, ARRAY_SIZE(d));			\
+} while (0)
+
+#define s6e3fa0_dcs_write_seq_static(ctx, seq...)			\
+do {									\
+	static const unsigned char d[] = { seq };			\
+	s6e3fa0_dcs_write(ctx, d, ARRAY_SIZE(d));			\
+} while (0)
+
+static void s6e3fa0_apply_level_1_key(struct s6e3fa0 *ctx)
+{
+	s6e3fa0_dcs_write_seq_static(ctx, MCS_KEY_LEV1, 0x5a, 0x5a);
+}
+
+static void s6e3fa0_apply_level_2_key(struct s6e3fa0 *ctx, bool on)
+{
+	if (on)
+		s6e3fa0_dcs_write_seq_static(ctx, MCS_KEY_LEV2, 0x5a, 0x5a);
+	else
+		s6e3fa0_dcs_write_seq_static(ctx, MCS_KEY_LEV2, 0xa5, 0xa5);
+}
+
+static void s6e3fa0_set_maximum_return_packet_size(struct s6e3fa0 *ctx,
+							unsigned int size)
+{
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
+
+	if (ops && ops->transfer) {
+		unsigned char buf[] = {size, 0};
+		struct mipi_dsi_msg msg = {
+			.channel = dsi->channel,
+			.type = MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE,
+			.tx_len = sizeof(buf),
+			.tx_buf = buf
+		};
+
+		ops->transfer(dsi->host, &msg);
+	}
+}
+
+static void s6e3fa0_read_mtp_id(struct s6e3fa0 *ctx)
+{
+	unsigned char id[MTP_ID_LEN];
+	int ret;
+
+	s6e3fa0_set_maximum_return_packet_size(ctx, MTP_ID_LEN);
+	ret = s6e3fa0_dcs_read(ctx, MIPI_DCS_GET_DISPLAY_ID, id, MTP_ID_LEN);
+	if (ret < MTP_ID_LEN || id[0] == 0x00) {
+		dev_err(ctx->dev, "failed to read id\n");
+		return;
+	}
+
+	dev_info(ctx->dev, "ID: 0x%02x, 0x%02x, 0x%02x\n", id[0], id[1], id[2]);
+
+	ctx->id = id[2];
+}
+
+static void s6e3fa0_read_vddm(struct s6e3fa0 *ctx)
+{
+	unsigned char vddm;
+	int ret;
+
+	s6e3fa0_apply_level_2_key(ctx, true);
+	s6e3fa0_dcs_write_seq_static(ctx, MCS_GLOBAL_PARAMETER, 0x16);
+	s6e3fa0_set_maximum_return_packet_size(ctx, 1);
+	ret = s6e3fa0_dcs_read(ctx, MCS_VDDM, &vddm, 1);
+	s6e3fa0_apply_level_2_key(ctx, false);
+
+	if (ret < 1 || vddm > 0x7f) {
+		dev_err(ctx->dev, "failed to read vddm, use default val.\n");
+		vddm = DEFAULT_VDDM_VAL;
+	}
+
+	ctx->vddm = s6e3fa0_vddm_lut[vddm][1];
+}
+
+static void s6e3fa0_set_pentile_control(struct s6e3fa0 *ctx)
+{
+	s6e3fa0_dcs_write_seq_static(ctx, MCS_PENTILE_CTRL, 0x00, 0x02, 0x03,
+					0x32, 0x03, 0x44, 0x44, 0xc0, 0x00,
+					0x1c, 0x20, 0xe8);
+}
+
+static void s6e3fa0_write_ambient_light_sensor(struct s6e3fa0 *ctx)
+{
+	s6e3fa0_dcs_write_seq_static(ctx, MCS_ALS, 0xff, 0xff, 0xff, 0xff);
+}
+
+static void s6e3fa0_set_readability_enhancement(struct s6e3fa0 *ctx)
+{
+	s6e3fa0_dcs_write_seq_static(ctx, MCS_RE, 0x00, 0x03,
+					MCS_GLOBAL_PARAMETER, 0x2b,
+					MCS_RE, 0xe4);
+}
+
+static void s6e3fa0_set_common_control(struct s6e3fa0 *ctx)
+{
+	s6e3fa0_set_pentile_control(ctx);
+	s6e3fa0_write_ambient_light_sensor(ctx);
+	s6e3fa0_set_readability_enhancement(ctx);
+}
+
+static void s6e3fa0_set_gamma(struct s6e3fa0 *ctx)
+{
+	s6e3fa0_dcs_write_seq_static(ctx, MCS_GAMMA_MODE, 0x01, 0x00, 0x01,
+					0x00, 0x01, 0x00, 0x80, 0x80, 0x80,
+					0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
+					0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
+					0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
+					0x80, 0x80, 0x80, 0x00, 0x00, 0x00);
+}
+
+static void s6e3fa0_set_amoled_impulse_driving(struct s6e3fa0 *ctx)
+{
+	s6e3fa0_dcs_write_seq_static(ctx, MCS_AID, 0x00, 0x06, 0x00, 0x06);
+}
+
+static void s6e3fa0_set_elvss(struct s6e3fa0 *ctx)
+{
+	s6e3fa0_dcs_write_seq_static(ctx, MCS_ELVSSOPT, 0x88, 0x0a);
+}
+
+static void s6e3fa0_update_gamma(struct s6e3fa0 *ctx)
+{
+	s6e3fa0_dcs_write_seq_static(ctx, MCS_GAMMA_UPDATE, 0x03);
+}
+
+static void s6e3fa0_write_automatic_current_limit(struct s6e3fa0 *ctx)
+{
+	s6e3fa0_dcs_write_seq_static(ctx, DCS_WRITE_CABC, 0x02);
+}
+
+static void s6e3fa0_set_brightness_control(struct s6e3fa0 *ctx)
+{
+	s6e3fa0_set_gamma(ctx);
+	s6e3fa0_set_amoled_impulse_driving(ctx);
+	s6e3fa0_set_elvss(ctx);
+	s6e3fa0_update_gamma(ctx);
+	s6e3fa0_write_automatic_current_limit(ctx);
+}
+
+static void s6e3fa0_set_temperature(struct s6e3fa0 *ctx)
+{
+	s6e3fa0_dcs_write_seq_static(ctx, MCS_GLOBAL_PARAMETER, 0x05,
+					MCS_TEMPERATURE_SET, 0x19);
+}
+
+static void s6e3fa0_set_elvss_control(struct s6e3fa0 *ctx)
+{
+	s6e3fa0_set_temperature(ctx);
+	s6e3fa0_set_elvss(ctx);
+}
+
+static void s6e3fa0_set_te_on(struct s6e3fa0 *ctx)
+{
+	s6e3fa0_dcs_write_seq_static(ctx, MIPI_DCS_SET_TEAR_ON, 0x00);
+}
+
+static void s6e3fa0_set_etc_and_write_vddm(struct s6e3fa0 *ctx)
+{
+	s6e3fa0_apply_level_2_key(ctx, true);
+	s6e3fa0_dcs_write_seq(ctx, MCS_ERR_FG, 0x0c, 0x04,
+				MCS_TOUT2_HSYNC, 0x01,
+				MCS_GLOBAL_PARAMETER, 0x16,
+				MCS_VDDM, ctx->vddm);
+	s6e3fa0_apply_level_2_key(ctx, false);
+}
+
+static void s6e3fa0_set_etc_condition(struct s6e3fa0 *ctx)
+{
+	s6e3fa0_set_te_on(ctx);
+	s6e3fa0_set_etc_and_write_vddm(ctx);
+}
+
+static void s6e3fa0_panel_init(struct s6e3fa0 *ctx)
+{
+	s6e3fa0_set_common_control(ctx);
+	s6e3fa0_set_brightness_control(ctx);
+	s6e3fa0_set_elvss_control(ctx);
+	s6e3fa0_set_etc_condition(ctx);
+
+	msleep(ctx->init_delay);
+}
+
+static int s6e3fa0_power_off(struct s6e3fa0 *ctx)
+{
+	gpiod_set_value(ctx->reset_gpio, 0);
+
+	return regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+}
+
+static int s6e3fa0_power_on(struct s6e3fa0 *ctx)
+{
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+	if (ret)
+		return ret;
+
+	msleep(ctx->power_on_delay);
+
+	gpiod_set_value(ctx->reset_gpio, 1);
+	gpiod_set_value(ctx->reset_gpio, 0);
+	usleep_range(1000, 2000);
+	gpiod_set_value(ctx->reset_gpio, 1);
+
+	msleep(ctx->reset_delay);
+
+	return 0;
+}
+
+static void s6e3fa0_set_sequence(struct s6e3fa0 *ctx)
+{
+	s6e3fa0_apply_level_1_key(ctx);
+	s6e3fa0_dcs_write_seq_static(ctx, MIPI_DCS_EXIT_SLEEP_MODE);
+	msleep(20);
+
+	s6e3fa0_read_mtp_id(ctx);
+	s6e3fa0_read_vddm(ctx);
+
+	s6e3fa0_panel_init(ctx);
+
+	s6e3fa0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_ON);
+}
+
+static int s6e3fa0_disable(struct drm_panel *panel)
+{
+	struct s6e3fa0 *ctx = panel_to_s6e3fa0(panel);
+
+	s6e3fa0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_OFF);
+	msleep(35);
+	s6e3fa0_dcs_write_seq_static(ctx, MIPI_DCS_ENTER_SLEEP_MODE);
+	msleep(120);
+
+	return s6e3fa0_power_off(ctx);
+}
+
+static int s6e3fa0_enable(struct drm_panel *panel)
+{
+	struct s6e3fa0 *ctx = panel_to_s6e3fa0(panel);
+	int ret;
+
+	ret = s6e3fa0_power_on(ctx);
+	if (ret)
+		return ret;
+
+	s6e3fa0_set_sequence(ctx);
+
+	return ret;
+}
+
+static int s6e3fa0_get_modes(struct drm_panel *panel)
+{
+	struct drm_connector *connector = panel->connector;
+	struct s6e3fa0 *ctx = panel_to_s6e3fa0(panel);
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_create(connector->dev);
+	if (!mode) {
+		DRM_ERROR("failed to create a new display mode\n");
+		return 0;
+	}
+
+	drm_display_mode_from_videomode(&ctx->vm, mode);
+	mode->width_mm = ctx->width_mm;
+	mode->height_mm = ctx->height_mm;
+	connector->display_info.width_mm = mode->width_mm;
+	connector->display_info.height_mm = mode->height_mm;
+
+	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+	drm_mode_probed_add(connector, mode);
+
+	return 1;
+}
+
+static const struct drm_panel_funcs s6e3fa0_drm_funcs = {
+	.disable	= s6e3fa0_disable,
+	.enable		= s6e3fa0_enable,
+	.get_modes	= s6e3fa0_get_modes,
+};
+
+static int s6e3fa0_parse_dt(struct s6e3fa0 *ctx)
+{
+	struct device *dev = ctx->dev;
+	int ret;
+
+	ret = of_get_videomode(dev->of_node, &ctx->vm, 0);
+	if (ret) {
+		dev_err(dev, "failed to get video mode: %d\n", ret);
+		return ret;
+	}
+
+	return ret;
+}
+
+static irqreturn_t s6e3fa0_det_interrupt(int irq, void *dev_id)
+{
+	/* TODO */
+	return IRQ_HANDLED;
+}
+
+static int s6e3fa0_probe(struct mipi_dsi_device *dsi)
+{
+	struct device *dev = &dsi->dev;
+	struct s6e3fa0 *ctx;
+	struct gpio_desc *det_gpio;
+	int ret, te_gpio;
+
+	ctx = devm_kzalloc(dev, sizeof(struct s6e3fa0), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	mipi_dsi_set_drvdata(dsi, ctx);
+
+	ctx->dev = dev;
+
+	dsi->lanes = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+
+	ret = s6e3fa0_parse_dt(ctx);
+	if (ret)
+		return ret;
+
+	ctx->supplies[0].supply = "vdd3";
+	ctx->supplies[1].supply = "vci";
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ctx->supplies),
+					ctx->supplies);
+	if (ret) {
+		dev_err(dev, "failed to get regulators: %d\n", ret);
+		return ret;
+	}
+
+	ctx->reset_gpio = devm_gpiod_get(dev, "reset");
+	if (IS_ERR(ctx->reset_gpio)) {
+		dev_err(dev, "failed to get reset gpio: %ld\n",
+				PTR_ERR(ctx->reset_gpio));
+		return PTR_ERR(ctx->reset_gpio);
+	}
+	ret = gpiod_direction_output(ctx->reset_gpio, 1);
+	if (ret < 0) {
+		dev_err(dev, "failed to configure reset gpio: %d\n", ret);
+		return ret;
+	}
+
+	det_gpio = devm_gpiod_get(dev, "det");
+	if (IS_ERR(det_gpio)) {
+		dev_err(dev, "failed to get det gpio: %ld\n",
+				PTR_ERR(det_gpio));
+		return PTR_ERR(det_gpio);
+	}
+	ret = gpiod_direction_input(det_gpio);
+	if (ret < 0) {
+		dev_err(dev, "failed to configure det gpio: %d\n", ret);
+		return ret;
+	}
+	ret = devm_request_irq(dev, gpiod_to_irq(det_gpio),
+				s6e3fa0_det_interrupt, IRQF_TRIGGER_FALLING,
+				"oled-det", ctx);
+	if (ret) {
+		dev_err(dev, "failed to request det irq: %d\n", ret);
+		return ret;
+	}
+
+	te_gpio = of_get_named_gpio(dev->of_node, "te-gpios", 0);
+	if (!gpio_is_valid(te_gpio)) {
+		dev_err(dev, "failed to get te gpio: %d\n", te_gpio);
+		return te_gpio;
+	}
+
+	ctx->power_on_delay = 10;
+	ctx->reset_delay = 5;
+	ctx->init_delay = 120;
+	ctx->width_mm = 71;
+	ctx->height_mm = 126;
+
+	ctx->brightness = GAMMA_LEVEL_NUM - 1;
+
+	drm_panel_init(&ctx->panel);
+	ctx->panel.dev = dev;
+	ctx->panel.funcs = &s6e3fa0_drm_funcs;
+
+	ret = drm_panel_add(&ctx->panel);
+	if (ret)
+		return ret;
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret)
+		drm_panel_remove(&ctx->panel);
+
+	return ret;
+}
+
+static int s6e3fa0_remove(struct mipi_dsi_device *dsi)
+{
+	struct s6e3fa0 *ctx = mipi_dsi_get_drvdata(dsi);
+
+	mipi_dsi_detach(dsi);
+	drm_panel_remove(&ctx->panel);
+
+	return 0;
+}
+
+static struct of_device_id s6e3fa0_of_match[] = {
+	{ .compatible = "samsung,s6e3fa0" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, s6e3fa0_of_match);
+
+static struct mipi_dsi_driver s6e3fa0_driver = {
+	.probe = s6e3fa0_probe,
+	.remove = s6e3fa0_remove,
+	.driver = {
+		.name = "panel_s6e3fa0",
+		.owner = THIS_MODULE,
+		.of_match_table = s6e3fa0_of_match,
+	},
+};
+module_mipi_dsi_driver(s6e3fa0_driver);
+
+MODULE_AUTHOR("YoungJun Cho <yj44.cho@samsung.com>");
+MODULE_DESCRIPTION("MIPI DSI command mode based s6e3fa0 AMOLED LCD Driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.0

Re: [PATCH v6 10/14] drm/panel: add S6E3FA0 driver

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 8183 bytes --]

On Thu, Jul 17, 2014 at 06:01:25PM +0900, YoungJun Cho wrote:
[...]
> diff --git a/drivers/gpu/drm/panel/panel-s6e3fa0.c b/drivers/gpu/drm/panel/panel-s6e3fa0.c
[...]
> +/* Manufacturer Command Set */
> +#define MCS_GLOBAL_PARAMETER	0xb0
> +#define MCS_AID			0xb2
> +#define MCS_ELVSSOPT		0xb6
> +#define MCS_TEMPERATURE_SET	0xb8
> +#define MCS_PENTILE_CTRL	0xc0
> +#define MCS_GAMMA_MODE		0xca
> +#define MCS_VDDM		0xd7
> +#define MCS_ALS			0xe3
> +#define MCS_ERR_FG		0xed
> +#define MCS_KEY_LEV1		0xf0
> +#define MCS_GAMMA_UPDATE	0xf7
> +#define MCS_KEY_LEV2		0xfc
> +#define MCS_RE			0xfe
> +#define MCS_TOUT2_HSYNC		0xff
> +
> +/* Content Adaptive Brightness Control */
> +#define DCS_WRITE_CABC		0x55

Is this not a manufacturer specific command? I couldn't find it in the
DSI or DCS specifications, but it sounds like something standard (also
indicated by the DCS_ prefix). Can you point out the specification for
this?

> +#define MTP_ID_LEN		3
> +#define GAMMA_LEVEL_NUM		30
> +
> +#define DEFAULT_VDDM_VAL	0x15
> +
> +struct s6e3fa0 {
> +	struct device			*dev;
> +	struct drm_panel		panel;
> +
> +	struct regulator_bulk_data	supplies[2];
> +	struct gpio_desc		*reset_gpio;
> +	struct videomode		vm;
> +
> +	unsigned int			power_on_delay;
> +	unsigned int			reset_delay;
> +	unsigned int			init_delay;
> +	unsigned int			width_mm;
> +	unsigned int			height_mm;
> +
> +	unsigned char			id;
> +	unsigned char			vddm;
> +	unsigned int			brightness;
> +};

Please don't use this kind of artificial padding. A simple space will
do.

> +
> +#define panel_to_s6e3fa0(p) container_of(p, struct s6e3fa0, panel)

Please turn this into a function so we can get proper type checking.

> +
> +/* VDD Memory Lookup Table contains pairs of {ReadValue, WriteValue} */
> +static const unsigned char s6e3fa0_vddm_lut[][2] = {
> +	{0x00, 0x0d}, {0x01, 0x0d}, {0x02, 0x0e}, {0x03, 0x0f}, {0x04, 0x10},
> +	{0x05, 0x11}, {0x06, 0x12}, {0x07, 0x13}, {0x08, 0x14}, {0x09, 0x15},
> +	{0x0a, 0x16}, {0x0b, 0x17}, {0x0c, 0x18}, {0x0d, 0x19}, {0x0e, 0x1a},
> +	{0x0f, 0x1b}, {0x10, 0x1c}, {0x11, 0x1d}, {0x12, 0x1e}, {0x13, 0x1f},
> +	{0x14, 0x20}, {0x15, 0x21}, {0x16, 0x22}, {0x17, 0x23}, {0x18, 0x24},
> +	{0x19, 0x25}, {0x1a, 0x26}, {0x1b, 0x27}, {0x1c, 0x28}, {0x1d, 0x29},
> +	{0x1e, 0x2a}, {0x1f, 0x2b}, {0x20, 0x2c}, {0x21, 0x2d}, {0x22, 0x2e},
> +	{0x23, 0x2f}, {0x24, 0x30}, {0x25, 0x31}, {0x26, 0x32}, {0x27, 0x33},
> +	{0x28, 0x34}, {0x29, 0x35}, {0x2a, 0x36}, {0x2b, 0x37}, {0x2c, 0x38},
> +	{0x2d, 0x39}, {0x2e, 0x3a}, {0x2f, 0x3b}, {0x30, 0x3c}, {0x31, 0x3d},
> +	{0x32, 0x3e}, {0x33, 0x3f}, {0x34, 0x3f}, {0x35, 0x3f}, {0x36, 0x3f},
> +	{0x37, 0x3f}, {0x38, 0x3f}, {0x39, 0x3f}, {0x3a, 0x3f}, {0x3b, 0x3f},
> +	{0x3c, 0x3f}, {0x3d, 0x3f}, {0x3e, 0x3f}, {0x3f, 0x3f}, {0x40, 0x0c},
> +	{0x41, 0x0b}, {0x42, 0x0a}, {0x43, 0x09}, {0x44, 0x08}, {0x45, 0x07},
> +	{0x46, 0x06}, {0x47, 0x05}, {0x48, 0x04}, {0x49, 0x03}, {0x4a, 0x02},
> +	{0x4b, 0x01}, {0x4c, 0x40}, {0x4d, 0x41}, {0x4e, 0x42}, {0x4f, 0x43},
> +	{0x50, 0x44}, {0x51, 0x45}, {0x52, 0x46}, {0x53, 0x47}, {0x54, 0x48},
> +	{0x55, 0x49}, {0x56, 0x4a}, {0x57, 0x4b}, {0x58, 0x4c}, {0x59, 0x4d},
> +	{0x5a, 0x4e}, {0x5b, 0x4f}, {0x5c, 0x50}, {0x5d, 0x51}, {0x5e, 0x52},
> +	{0x5f, 0x53}, {0x60, 0x54}, {0x61, 0x55}, {0x62, 0x56}, {0x63, 0x57},
> +	{0x64, 0x58}, {0x65, 0x59}, {0x66, 0x5a}, {0x67, 0x5b}, {0x68, 0x5c},
> +	{0x69, 0x5d}, {0x6a, 0x5e}, {0x6b, 0x5f}, {0x6c, 0x60}, {0x6d, 0x61},
> +	{0x6e, 0x62}, {0x6f, 0x63}, {0x70, 0x64}, {0x71, 0x65}, {0x72, 0x66},
> +	{0x73, 0x67}, {0x74, 0x68}, {0x75, 0x69}, {0x76, 0x6a}, {0x77, 0x6b},
> +	{0x78, 0x6c}, {0x79, 0x6d}, {0x7a, 0x6e}, {0x7b, 0x6f}, {0x7c, 0x70},
> +	{0x7d, 0x71}, {0x7e, 0x72}, {0x7f, 0x73},
> +};

What's this used for?

> +static int s6e3fa0_dcs_read(struct s6e3fa0 *ctx, unsigned char cmd,
> +							void *data, size_t len)
> +{
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +
> +	return mipi_dsi_dcs_read(dsi, dsi->channel, cmd, data, len);
> +}
> +
> +static void s6e3fa0_dcs_write(struct s6e3fa0 *ctx, const void *data, size_t len)
> +{
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +
> +	mipi_dsi_dcs_write(dsi, dsi->channel, data, len);
> +}

Both mipi_dsi_dcs_read() and mipi_dsi_dcs_write() return error codes on
failure. Why are you silently ignoring them?

> +#define s6e3fa0_dcs_write_seq(ctx, seq...)				\
> +do {									\
> +	const unsigned char d[] = { seq };				\
> +	BUILD_BUG_ON_MSG(ARRAY_SIZE(d) > 64, "too big seq for stack");	\
> +	s6e3fa0_dcs_write(ctx, d, ARRAY_SIZE(d));			\
> +} while (0)
> +
> +#define s6e3fa0_dcs_write_seq_static(ctx, seq...)			\
> +do {									\
> +	static const unsigned char d[] = { seq };			\
> +	s6e3fa0_dcs_write(ctx, d, ARRAY_SIZE(d));			\
> +} while (0)

I've had this discussion with Andrzej before and I'm still not convinced
that this is a useful macro.

At least they should propagate the error code, though.

> +static void s6e3fa0_set_maximum_return_packet_size(struct s6e3fa0 *ctx,
> +							unsigned int size)
> +{
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
> +
> +	if (ops && ops->transfer) {
> +		unsigned char buf[] = {size, 0};
> +		struct mipi_dsi_msg msg = {
> +			.channel = dsi->channel,
> +			.type = MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE,
> +			.tx_len = sizeof(buf),
> +			.tx_buf = buf
> +		};
> +
> +		ops->transfer(dsi->host, &msg);
> +	}
> +}

The Set Maximum Return Packet Size command is a standard command, so
please turn that into a generic function exposed by the DSI core.

> +static void s6e3fa0_read_mtp_id(struct s6e3fa0 *ctx)
> +{
> +	unsigned char id[MTP_ID_LEN];
> +	int ret;
> +
> +	s6e3fa0_set_maximum_return_packet_size(ctx, MTP_ID_LEN);
> +	ret = s6e3fa0_dcs_read(ctx, MIPI_DCS_GET_DISPLAY_ID, id, MTP_ID_LEN);

This also looks like a standard DCS command. I can't find it in either
v1.01 nor v1.02 of the specification, though. Do you know where it's
specified?

> +static void s6e3fa0_set_te_on(struct s6e3fa0 *ctx)
> +{
> +	s6e3fa0_dcs_write_seq_static(ctx, MIPI_DCS_SET_TEAR_ON, 0x00);
> +}

This is also a standard DCS command.

> +static int s6e3fa0_power_off(struct s6e3fa0 *ctx)
> +{
> +	gpiod_set_value(ctx->reset_gpio, 0);

Setting the reset GPIO to 0 for power off? Shouldn't this be 1 and the
polarity be specified in the GPIO specifier?

> +static void s6e3fa0_set_sequence(struct s6e3fa0 *ctx)
> +{
> +	s6e3fa0_apply_level_1_key(ctx);
> +	s6e3fa0_dcs_write_seq_static(ctx, MIPI_DCS_EXIT_SLEEP_MODE);
> +	msleep(20);
> +
> +	s6e3fa0_read_mtp_id(ctx);
> +	s6e3fa0_read_vddm(ctx);
> +
> +	s6e3fa0_panel_init(ctx);
> +
> +	s6e3fa0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_ON);
> +}
> +
> +static int s6e3fa0_disable(struct drm_panel *panel)
> +{
> +	struct s6e3fa0 *ctx = panel_to_s6e3fa0(panel);
> +
> +	s6e3fa0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_OFF);
> +	msleep(35);
> +	s6e3fa0_dcs_write_seq_static(ctx, MIPI_DCS_ENTER_SLEEP_MODE);
> +	msleep(120);
> +
> +	return s6e3fa0_power_off(ctx);
> +}

The SET_DISPLAY_{ON,OFF} and {ENTER,EXIT}_SLEEP_MODE are standard
commands, too.

> +static int s6e3fa0_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device *dev = &dsi->dev;
> +	struct s6e3fa0 *ctx;
> +	struct gpio_desc *det_gpio;
> +	int ret, te_gpio;
> +
> +	ctx = devm_kzalloc(dev, sizeof(struct s6e3fa0), GFP_KERNEL);

sizeof(*ctx)

> +	det_gpio = devm_gpiod_get(dev, "det");
> +	if (IS_ERR(det_gpio)) {
> +		dev_err(dev, "failed to get det gpio: %ld\n",
> +				PTR_ERR(det_gpio));
> +		return PTR_ERR(det_gpio);
> +	}
> +	ret = gpiod_direction_input(det_gpio);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to configure det gpio: %d\n", ret);
> +		return ret;
> +	}
> +	ret = devm_request_irq(dev, gpiod_to_irq(det_gpio),
> +				s6e3fa0_det_interrupt, IRQF_TRIGGER_FALLING,
> +				"oled-det", ctx);
> +	if (ret) {
> +		dev_err(dev, "failed to request det irq: %d\n", ret);
> +		return ret;
> +	}
> +
> +	te_gpio = of_get_named_gpio(dev->of_node, "te-gpios", 0);

Why doesn't this use the gpiod_* API like the other GPIOs?

> +static struct of_device_id s6e3fa0_of_match[] = {

Should be static const.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 10/14] drm/panel: add S6E3FA0 driver

[YoungJun Cho]

Hi Thierry,

Thank you a lot for kind comments.

On 07/17/2014 07:36 PM, Thierry Reding wrote:
> On Thu, Jul 17, 2014 at 06:01:25PM +0900, YoungJun Cho wrote:
> [...]
>> diff --git a/drivers/gpu/drm/panel/panel-s6e3fa0.c b/drivers/gpu/drm/panel/panel-s6e3fa0.c
> [...]
>> +/* Manufacturer Command Set */
>> +#define MCS_GLOBAL_PARAMETER	0xb0
>> +#define MCS_AID			0xb2
>> +#define MCS_ELVSSOPT		0xb6
>> +#define MCS_TEMPERATURE_SET	0xb8
>> +#define MCS_PENTILE_CTRL	0xc0
>> +#define MCS_GAMMA_MODE		0xca
>> +#define MCS_VDDM		0xd7
>> +#define MCS_ALS			0xe3
>> +#define MCS_ERR_FG		0xed
>> +#define MCS_KEY_LEV1		0xf0
>> +#define MCS_GAMMA_UPDATE	0xf7
>> +#define MCS_KEY_LEV2		0xfc
>> +#define MCS_RE			0xfe
>> +#define MCS_TOUT2_HSYNC		0xff
>> +
>> +/* Content Adaptive Brightness Control */
>> +#define DCS_WRITE_CABC		0x55
>
> Is this not a manufacturer specific command? I couldn't find it in the
> DSI or DCS specifications, but it sounds like something standard (also
> indicated by the DCS_ prefix). Can you point out the specification for
> this?
>

Andrzej commented before and decided it as DCS one because if the value 
is less than 0xb0, it is DCS one and the others are MCS one.
But still I'm not sure it is correct.

>> +#define MTP_ID_LEN		3
>> +#define GAMMA_LEVEL_NUM		30
>> +
>> +#define DEFAULT_VDDM_VAL	0x15
>> +
>> +struct s6e3fa0 {
>> +	struct device			*dev;
>> +	struct drm_panel		panel;
>> +
>> +	struct regulator_bulk_data	supplies[2];
>> +	struct gpio_desc		*reset_gpio;
>> +	struct videomode		vm;
>> +
>> +	unsigned int			power_on_delay;
>> +	unsigned int			reset_delay;
>> +	unsigned int			init_delay;
>> +	unsigned int			width_mm;
>> +	unsigned int			height_mm;
>> +
>> +	unsigned char			id;
>> +	unsigned char			vddm;
>> +	unsigned int			brightness;
>> +};
>
> Please don't use this kind of artificial padding. A simple space will
> do.
>
>> +
>> +#define panel_to_s6e3fa0(p) container_of(p, struct s6e3fa0, panel)
>
> Please turn this into a function so we can get proper type checking.
>
>> +
>> +/* VDD Memory Lookup Table contains pairs of {ReadValue, WriteValue} */
>> +static const unsigned char s6e3fa0_vddm_lut[][2] = {
>> +	{0x00, 0x0d}, {0x01, 0x0d}, {0x02, 0x0e}, {0x03, 0x0f}, {0x04, 0x10},
>> +	{0x05, 0x11}, {0x06, 0x12}, {0x07, 0x13}, {0x08, 0x14}, {0x09, 0x15},
>> +	{0x0a, 0x16}, {0x0b, 0x17}, {0x0c, 0x18}, {0x0d, 0x19}, {0x0e, 0x1a},
>> +	{0x0f, 0x1b}, {0x10, 0x1c}, {0x11, 0x1d}, {0x12, 0x1e}, {0x13, 0x1f},
>> +	{0x14, 0x20}, {0x15, 0x21}, {0x16, 0x22}, {0x17, 0x23}, {0x18, 0x24},
>> +	{0x19, 0x25}, {0x1a, 0x26}, {0x1b, 0x27}, {0x1c, 0x28}, {0x1d, 0x29},
>> +	{0x1e, 0x2a}, {0x1f, 0x2b}, {0x20, 0x2c}, {0x21, 0x2d}, {0x22, 0x2e},
>> +	{0x23, 0x2f}, {0x24, 0x30}, {0x25, 0x31}, {0x26, 0x32}, {0x27, 0x33},
>> +	{0x28, 0x34}, {0x29, 0x35}, {0x2a, 0x36}, {0x2b, 0x37}, {0x2c, 0x38},
>> +	{0x2d, 0x39}, {0x2e, 0x3a}, {0x2f, 0x3b}, {0x30, 0x3c}, {0x31, 0x3d},
>> +	{0x32, 0x3e}, {0x33, 0x3f}, {0x34, 0x3f}, {0x35, 0x3f}, {0x36, 0x3f},
>> +	{0x37, 0x3f}, {0x38, 0x3f}, {0x39, 0x3f}, {0x3a, 0x3f}, {0x3b, 0x3f},
>> +	{0x3c, 0x3f}, {0x3d, 0x3f}, {0x3e, 0x3f}, {0x3f, 0x3f}, {0x40, 0x0c},
>> +	{0x41, 0x0b}, {0x42, 0x0a}, {0x43, 0x09}, {0x44, 0x08}, {0x45, 0x07},
>> +	{0x46, 0x06}, {0x47, 0x05}, {0x48, 0x04}, {0x49, 0x03}, {0x4a, 0x02},
>> +	{0x4b, 0x01}, {0x4c, 0x40}, {0x4d, 0x41}, {0x4e, 0x42}, {0x4f, 0x43},
>> +	{0x50, 0x44}, {0x51, 0x45}, {0x52, 0x46}, {0x53, 0x47}, {0x54, 0x48},
>> +	{0x55, 0x49}, {0x56, 0x4a}, {0x57, 0x4b}, {0x58, 0x4c}, {0x59, 0x4d},
>> +	{0x5a, 0x4e}, {0x5b, 0x4f}, {0x5c, 0x50}, {0x5d, 0x51}, {0x5e, 0x52},
>> +	{0x5f, 0x53}, {0x60, 0x54}, {0x61, 0x55}, {0x62, 0x56}, {0x63, 0x57},
>> +	{0x64, 0x58}, {0x65, 0x59}, {0x66, 0x5a}, {0x67, 0x5b}, {0x68, 0x5c},
>> +	{0x69, 0x5d}, {0x6a, 0x5e}, {0x6b, 0x5f}, {0x6c, 0x60}, {0x6d, 0x61},
>> +	{0x6e, 0x62}, {0x6f, 0x63}, {0x70, 0x64}, {0x71, 0x65}, {0x72, 0x66},
>> +	{0x73, 0x67}, {0x74, 0x68}, {0x75, 0x69}, {0x76, 0x6a}, {0x77, 0x6b},
>> +	{0x78, 0x6c}, {0x79, 0x6d}, {0x7a, 0x6e}, {0x7b, 0x6f}, {0x7c, 0x70},
>> +	{0x7d, 0x71}, {0x7e, 0x72}, {0x7f, 0x73},
>> +};
>
> What's this used for?
>

This ldi contains an internal memory and requires an appropriate VDD.
Each panel stores OTP value for this vddm, so reads this value, finds 
matching value with vddm_lut and writes the final value to avoid noise 
issues from an inappropriate VDD.

>> +static int s6e3fa0_dcs_read(struct s6e3fa0 *ctx, unsigned char cmd,
>> +							void *data, size_t len)
>> +{
>> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>> +
>> +	return mipi_dsi_dcs_read(dsi, dsi->channel, cmd, data, len);
>> +}
>> +
>> +static void s6e3fa0_dcs_write(struct s6e3fa0 *ctx, const void *data, size_t len)
>> +{
>> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>> +
>> +	mipi_dsi_dcs_write(dsi, dsi->channel, data, len);
>> +}
>
> Both mipi_dsi_dcs_read() and mipi_dsi_dcs_write() return error codes on
> failure. Why are you silently ignoring them?
>
>> +#define s6e3fa0_dcs_write_seq(ctx, seq...)				\
>> +do {									\
>> +	const unsigned char d[] = { seq };				\
>> +	BUILD_BUG_ON_MSG(ARRAY_SIZE(d) > 64, "too big seq for stack");	\
>> +	s6e3fa0_dcs_write(ctx, d, ARRAY_SIZE(d));			\
>> +} while (0)
>> +
>> +#define s6e3fa0_dcs_write_seq_static(ctx, seq...)			\
>> +do {									\
>> +	static const unsigned char d[] = { seq };			\
>> +	s6e3fa0_dcs_write(ctx, d, ARRAY_SIZE(d));			\
>> +} while (0)
>
> I've had this discussion with Andrzej before and I'm still not convinced
> that this is a useful macro.
>
> At least they should propagate the error code, though.
>
>> +static void s6e3fa0_set_maximum_return_packet_size(struct s6e3fa0 *ctx,
>> +							unsigned int size)
>> +{
>> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>> +	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
>> +
>> +	if (ops && ops->transfer) {
>> +		unsigned char buf[] = {size, 0};
>> +		struct mipi_dsi_msg msg = {
>> +			.channel = dsi->channel,
>> +			.type = MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE,
>> +			.tx_len = sizeof(buf),
>> +			.tx_buf = buf
>> +		};
>> +
>> +		ops->transfer(dsi->host, &msg);
>> +	}
>> +}
>
> The Set Maximum Return Packet Size command is a standard command, so
> please turn that into a generic function exposed by the DSI core.
>

For this and below standard DCS commands, you want to use generic 
functions, but I have no idea for that.
Could you explain more detail?

>> +static void s6e3fa0_read_mtp_id(struct s6e3fa0 *ctx)
>> +{
>> +	unsigned char id[MTP_ID_LEN];
>> +	int ret;
>> +
>> +	s6e3fa0_set_maximum_return_packet_size(ctx, MTP_ID_LEN);
>> +	ret = s6e3fa0_dcs_read(ctx, MIPI_DCS_GET_DISPLAY_ID, id, MTP_ID_LEN);
>
> This also looks like a standard DCS command. I can't find it in either
> v1.01 nor v1.02 of the specification, though. Do you know where it's
> specified?
>

Yes, I also can't find it in DCS specification and there is no special 
description in panel datasheet.
But as it is declared in "include/video/mipi_display.h", so I think it 
is a standard one.

Thank you.
Best regards YJ

>> +static void s6e3fa0_set_te_on(struct s6e3fa0 *ctx)
>> +{
>> +	s6e3fa0_dcs_write_seq_static(ctx, MIPI_DCS_SET_TEAR_ON, 0x00);
>> +}
>
> This is also a standard DCS command.
>
>> +static int s6e3fa0_power_off(struct s6e3fa0 *ctx)
>> +{
>> +	gpiod_set_value(ctx->reset_gpio, 0);
>
> Setting the reset GPIO to 0 for power off? Shouldn't this be 1 and the
> polarity be specified in the GPIO specifier?
>
>> +static void s6e3fa0_set_sequence(struct s6e3fa0 *ctx)
>> +{
>> +	s6e3fa0_apply_level_1_key(ctx);
>> +	s6e3fa0_dcs_write_seq_static(ctx, MIPI_DCS_EXIT_SLEEP_MODE);
>> +	msleep(20);
>> +
>> +	s6e3fa0_read_mtp_id(ctx);
>> +	s6e3fa0_read_vddm(ctx);
>> +
>> +	s6e3fa0_panel_init(ctx);
>> +
>> +	s6e3fa0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_ON);
>> +}
>> +
>> +static int s6e3fa0_disable(struct drm_panel *panel)
>> +{
>> +	struct s6e3fa0 *ctx = panel_to_s6e3fa0(panel);
>> +
>> +	s6e3fa0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_OFF);
>> +	msleep(35);
>> +	s6e3fa0_dcs_write_seq_static(ctx, MIPI_DCS_ENTER_SLEEP_MODE);
>> +	msleep(120);
>> +
>> +	return s6e3fa0_power_off(ctx);
>> +}
>
> The SET_DISPLAY_{ON,OFF} and {ENTER,EXIT}_SLEEP_MODE are standard
> commands, too.
>
>> +static int s6e3fa0_probe(struct mipi_dsi_device *dsi)
>> +{
>> +	struct device *dev = &dsi->dev;
>> +	struct s6e3fa0 *ctx;
>> +	struct gpio_desc *det_gpio;
>> +	int ret, te_gpio;
>> +
>> +	ctx = devm_kzalloc(dev, sizeof(struct s6e3fa0), GFP_KERNEL);
>
> sizeof(*ctx)
>
>> +	det_gpio = devm_gpiod_get(dev, "det");
>> +	if (IS_ERR(det_gpio)) {
>> +		dev_err(dev, "failed to get det gpio: %ld\n",
>> +				PTR_ERR(det_gpio));
>> +		return PTR_ERR(det_gpio);
>> +	}
>> +	ret = gpiod_direction_input(det_gpio);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to configure det gpio: %d\n", ret);
>> +		return ret;
>> +	}
>> +	ret = devm_request_irq(dev, gpiod_to_irq(det_gpio),
>> +				s6e3fa0_det_interrupt, IRQF_TRIGGER_FALLING,
>> +				"oled-det", ctx);
>> +	if (ret) {
>> +		dev_err(dev, "failed to request det irq: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	te_gpio = of_get_named_gpio(dev->of_node, "te-gpios", 0);
>
> Why doesn't this use the gpiod_* API like the other GPIOs?
>
>> +static struct of_device_id s6e3fa0_of_match[] = {
>
> Should be static const.
>
> Thierry
>

Re: [PATCH v6 10/14] drm/panel: add S6E3FA0 driver

[Andrzej Hajda]

On 07/18/2014 03:49 AM, YoungJun Cho wrote:
> Hi Thierry,
>
> Thank you a lot for kind comments.
>
> On 07/17/2014 07:36 PM, Thierry Reding wrote:
>> On Thu, Jul 17, 2014 at 06:01:25PM +0900, YoungJun Cho wrote:
>> [...]
>>> diff --git a/drivers/gpu/drm/panel/panel-s6e3fa0.c b/drivers/gpu/drm/panel/panel-s6e3fa0.c
>> [...]
>>> +/* Manufacturer Command Set */
>>> +#define MCS_GLOBAL_PARAMETER	0xb0
>>> +#define MCS_AID			0xb2
>>> +#define MCS_ELVSSOPT		0xb6
>>> +#define MCS_TEMPERATURE_SET	0xb8
>>> +#define MCS_PENTILE_CTRL	0xc0
>>> +#define MCS_GAMMA_MODE		0xca
>>> +#define MCS_VDDM		0xd7
>>> +#define MCS_ALS			0xe3
>>> +#define MCS_ERR_FG		0xed
>>> +#define MCS_KEY_LEV1		0xf0
>>> +#define MCS_GAMMA_UPDATE	0xf7
>>> +#define MCS_KEY_LEV2		0xfc
>>> +#define MCS_RE			0xfe
>>> +#define MCS_TOUT2_HSYNC		0xff
>>> +
>>> +/* Content Adaptive Brightness Control */
>>> +#define DCS_WRITE_CABC		0x55
>> Is this not a manufacturer specific command? I couldn't find it in the
>> DSI or DCS specifications, but it sounds like something standard (also
>> indicated by the DCS_ prefix). Can you point out the specification for
>> this?
>>
> Andrzej commented before and decided it as DCS one because if the value 
> is less than 0xb0, it is DCS one and the others are MCS one.
> But still I'm not sure it is correct.
I would not say 'decided'. I have just stated that according to DCS
specification
it should be DCS command (below 0xb0), but it is not present in mipi dcs
specs.
On the other side many panels have it [1]:

[1]:
https://www.google.com/search?q=%22Write+Content+Adaptive+Brightness+Control%22

>>> +#define MTP_ID_LEN		3
>>> +#define GAMMA_LEVEL_NUM		30
>>> +
>>> +#define DEFAULT_VDDM_VAL	0x15
>>> +
>>> +struct s6e3fa0 {
>>> +	struct device			*dev;
>>> +	struct drm_panel		panel;
>>> +
>>> +	struct regulator_bulk_data	supplies[2];
>>> +	struct gpio_desc		*reset_gpio;
>>> +	struct videomode		vm;
>>> +
>>> +	unsigned int			power_on_delay;
>>> +	unsigned int			reset_delay;
>>> +	unsigned int			init_delay;
>>> +	unsigned int			width_mm;
>>> +	unsigned int			height_mm;
>>> +
>>> +	unsigned char			id;
>>> +	unsigned char			vddm;
>>> +	unsigned int			brightness;
>>> +};
>> Please don't use this kind of artificial padding. A simple space will
>> do.
>>
>>> +
>>> +#define panel_to_s6e3fa0(p) container_of(p, struct s6e3fa0, panel)
>> Please turn this into a function so we can get proper type checking.
>>
>>> +
>>> +/* VDD Memory Lookup Table contains pairs of {ReadValue, WriteValue} */
>>> +static const unsigned char s6e3fa0_vddm_lut[][2] = {
>>> +	{0x00, 0x0d}, {0x01, 0x0d}, {0x02, 0x0e}, {0x03, 0x0f}, {0x04, 0x10},
>>> +	{0x05, 0x11}, {0x06, 0x12}, {0x07, 0x13}, {0x08, 0x14}, {0x09, 0x15},
>>> +	{0x0a, 0x16}, {0x0b, 0x17}, {0x0c, 0x18}, {0x0d, 0x19}, {0x0e, 0x1a},
>>> +	{0x0f, 0x1b}, {0x10, 0x1c}, {0x11, 0x1d}, {0x12, 0x1e}, {0x13, 0x1f},
>>> +	{0x14, 0x20}, {0x15, 0x21}, {0x16, 0x22}, {0x17, 0x23}, {0x18, 0x24},
>>> +	{0x19, 0x25}, {0x1a, 0x26}, {0x1b, 0x27}, {0x1c, 0x28}, {0x1d, 0x29},
>>> +	{0x1e, 0x2a}, {0x1f, 0x2b}, {0x20, 0x2c}, {0x21, 0x2d}, {0x22, 0x2e},
>>> +	{0x23, 0x2f}, {0x24, 0x30}, {0x25, 0x31}, {0x26, 0x32}, {0x27, 0x33},
>>> +	{0x28, 0x34}, {0x29, 0x35}, {0x2a, 0x36}, {0x2b, 0x37}, {0x2c, 0x38},
>>> +	{0x2d, 0x39}, {0x2e, 0x3a}, {0x2f, 0x3b}, {0x30, 0x3c}, {0x31, 0x3d},
>>> +	{0x32, 0x3e}, {0x33, 0x3f}, {0x34, 0x3f}, {0x35, 0x3f}, {0x36, 0x3f},
>>> +	{0x37, 0x3f}, {0x38, 0x3f}, {0x39, 0x3f}, {0x3a, 0x3f}, {0x3b, 0x3f},
>>> +	{0x3c, 0x3f}, {0x3d, 0x3f}, {0x3e, 0x3f}, {0x3f, 0x3f}, {0x40, 0x0c},
>>> +	{0x41, 0x0b}, {0x42, 0x0a}, {0x43, 0x09}, {0x44, 0x08}, {0x45, 0x07},
>>> +	{0x46, 0x06}, {0x47, 0x05}, {0x48, 0x04}, {0x49, 0x03}, {0x4a, 0x02},
>>> +	{0x4b, 0x01}, {0x4c, 0x40}, {0x4d, 0x41}, {0x4e, 0x42}, {0x4f, 0x43},
>>> +	{0x50, 0x44}, {0x51, 0x45}, {0x52, 0x46}, {0x53, 0x47}, {0x54, 0x48},
>>> +	{0x55, 0x49}, {0x56, 0x4a}, {0x57, 0x4b}, {0x58, 0x4c}, {0x59, 0x4d},
>>> +	{0x5a, 0x4e}, {0x5b, 0x4f}, {0x5c, 0x50}, {0x5d, 0x51}, {0x5e, 0x52},
>>> +	{0x5f, 0x53}, {0x60, 0x54}, {0x61, 0x55}, {0x62, 0x56}, {0x63, 0x57},
>>> +	{0x64, 0x58}, {0x65, 0x59}, {0x66, 0x5a}, {0x67, 0x5b}, {0x68, 0x5c},
>>> +	{0x69, 0x5d}, {0x6a, 0x5e}, {0x6b, 0x5f}, {0x6c, 0x60}, {0x6d, 0x61},
>>> +	{0x6e, 0x62}, {0x6f, 0x63}, {0x70, 0x64}, {0x71, 0x65}, {0x72, 0x66},
>>> +	{0x73, 0x67}, {0x74, 0x68}, {0x75, 0x69}, {0x76, 0x6a}, {0x77, 0x6b},
>>> +	{0x78, 0x6c}, {0x79, 0x6d}, {0x7a, 0x6e}, {0x7b, 0x6f}, {0x7c, 0x70},
>>> +	{0x7d, 0x71}, {0x7e, 0x72}, {0x7f, 0x73},
>>> +};
>> What's this used for?
>>
> This ldi contains an internal memory and requires an appropriate VDD.
> Each panel stores OTP value for this vddm, so reads this value, finds 
> matching value with vddm_lut and writes the final value to avoid noise 
> issues from an inappropriate VDD.
>
>>> +static int s6e3fa0_dcs_read(struct s6e3fa0 *ctx, unsigned char cmd,
>>> +							void *data, size_t len)
>>> +{
>>> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>>> +
>>> +	return mipi_dsi_dcs_read(dsi, dsi->channel, cmd, data, len);
>>> +}
>>> +
>>> +static void s6e3fa0_dcs_write(struct s6e3fa0 *ctx, const void *data, size_t len)
>>> +{
>>> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>>> +
>>> +	mipi_dsi_dcs_write(dsi, dsi->channel, data, len);
>>> +}
>> Both mipi_dsi_dcs_read() and mipi_dsi_dcs_write() return error codes on
>> failure. Why are you silently ignoring them?
>>
>>> +#define s6e3fa0_dcs_write_seq(ctx, seq...)				\
>>> +do {									\
>>> +	const unsigned char d[] = { seq };				\
>>> +	BUILD_BUG_ON_MSG(ARRAY_SIZE(d) > 64, "too big seq for stack");	\
>>> +	s6e3fa0_dcs_write(ctx, d, ARRAY_SIZE(d));			\
>>> +} while (0)
>>> +
>>> +#define s6e3fa0_dcs_write_seq_static(ctx, seq...)			\
>>> +do {									\
>>> +	static const unsigned char d[] = { seq };			\
>>> +	s6e3fa0_dcs_write(ctx, d, ARRAY_SIZE(d));			\
>>> +} while (0)
>> I've had this discussion with Andrzej before and I'm still not convinced
>> that this is a useful macro.
>>
>> At least they should propagate the error code, though.

This is different implementation, my version propagates error code.
Anyway I have also objections about ignoring errors.

>>
>>> +static void s6e3fa0_set_maximum_return_packet_size(struct s6e3fa0 *ctx,
>>> +							unsigned int size)
>>> +{
>>> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>>> +	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
>>> +
>>> +	if (ops && ops->transfer) {
>>> +		unsigned char buf[] = {size, 0};
>>> +		struct mipi_dsi_msg msg = {
>>> +			.channel = dsi->channel,
>>> +			.type = MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE,
>>> +			.tx_len = sizeof(buf),
>>> +			.tx_buf = buf
>>> +		};
>>> +
>>> +		ops->transfer(dsi->host, &msg);
>>> +	}
>>> +}
>> The Set Maximum Return Packet Size command is a standard command, so
>> please turn that into a generic function exposed by the DSI core.
>>
> For this and below standard DCS commands, you want to use generic 
> functions, but I have no idea for that.
> Could you explain more detail?
>
>>> +static void s6e3fa0_read_mtp_id(struct s6e3fa0 *ctx)
>>> +{
>>> +	unsigned char id[MTP_ID_LEN];
>>> +	int ret;
>>> +
>>> +	s6e3fa0_set_maximum_return_packet_size(ctx, MTP_ID_LEN);
>>> +	ret = s6e3fa0_dcs_read(ctx, MIPI_DCS_GET_DISPLAY_ID, id, MTP_ID_LEN);
>> This also looks like a standard DCS command. I can't find it in either
>> v1.01 nor v1.02 of the specification, though. Do you know where it's
>> specified?
>>
> Yes, I also can't find it in DCS specification and there is no special 
> description in panel datasheet.
> But as it is declared in "include/video/mipi_display.h", so I think it 
> is a standard one.

On page 9 of the "Introduction of MIPI by Renesas" [2] there is info
it is a part of "Nokia I/F command list", I guess it is kind of alpha
version of MIPI DCS.

[2]: http://wenku.baidu.com/view/658fab68af1ffc4ffe47acbe.html

Regards
Andrzej
>
> Thank you.
> Best regards YJ
>
>>> +static void s6e3fa0_set_te_on(struct s6e3fa0 *ctx)
>>> +{
>>> +	s6e3fa0_dcs_write_seq_static(ctx, MIPI_DCS_SET_TEAR_ON, 0x00);
>>> +}
>> This is also a standard DCS command.
>>
>>> +static int s6e3fa0_power_off(struct s6e3fa0 *ctx)
>>> +{
>>> +	gpiod_set_value(ctx->reset_gpio, 0);
>> Setting the reset GPIO to 0 for power off? Shouldn't this be 1 and the
>> polarity be specified in the GPIO specifier?
>>
>>> +static void s6e3fa0_set_sequence(struct s6e3fa0 *ctx)
>>> +{
>>> +	s6e3fa0_apply_level_1_key(ctx);
>>> +	s6e3fa0_dcs_write_seq_static(ctx, MIPI_DCS_EXIT_SLEEP_MODE);
>>> +	msleep(20);
>>> +
>>> +	s6e3fa0_read_mtp_id(ctx);
>>> +	s6e3fa0_read_vddm(ctx);
>>> +
>>> +	s6e3fa0_panel_init(ctx);
>>> +
>>> +	s6e3fa0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_ON);
>>> +}
>>> +
>>> +static int s6e3fa0_disable(struct drm_panel *panel)
>>> +{
>>> +	struct s6e3fa0 *ctx = panel_to_s6e3fa0(panel);
>>> +
>>> +	s6e3fa0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_OFF);
>>> +	msleep(35);
>>> +	s6e3fa0_dcs_write_seq_static(ctx, MIPI_DCS_ENTER_SLEEP_MODE);
>>> +	msleep(120);
>>> +
>>> +	return s6e3fa0_power_off(ctx);
>>> +}
>> The SET_DISPLAY_{ON,OFF} and {ENTER,EXIT}_SLEEP_MODE are standard
>> commands, too.
>>
>>> +static int s6e3fa0_probe(struct mipi_dsi_device *dsi)
>>> +{
>>> +	struct device *dev = &dsi->dev;
>>> +	struct s6e3fa0 *ctx;
>>> +	struct gpio_desc *det_gpio;
>>> +	int ret, te_gpio;
>>> +
>>> +	ctx = devm_kzalloc(dev, sizeof(struct s6e3fa0), GFP_KERNEL);
>> sizeof(*ctx)
>>
>>> +	det_gpio = devm_gpiod_get(dev, "det");
>>> +	if (IS_ERR(det_gpio)) {
>>> +		dev_err(dev, "failed to get det gpio: %ld\n",
>>> +				PTR_ERR(det_gpio));
>>> +		return PTR_ERR(det_gpio);
>>> +	}
>>> +	ret = gpiod_direction_input(det_gpio);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "failed to configure det gpio: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +	ret = devm_request_irq(dev, gpiod_to_irq(det_gpio),
>>> +				s6e3fa0_det_interrupt, IRQF_TRIGGER_FALLING,
>>> +				"oled-det", ctx);
>>> +	if (ret) {
>>> +		dev_err(dev, "failed to request det irq: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	te_gpio = of_get_named_gpio(dev->of_node, "te-gpios", 0);
>> Why doesn't this use the gpiod_* API like the other GPIOs?
>>
>>> +static struct of_device_id s6e3fa0_of_match[] = {
>> Should be static const.
>>
>> Thierry
>>
>

Re: [PATCH v6 10/14] drm/panel: add S6E3FA0 driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3433 bytes --]

On Mon, Jul 21, 2014 at 10:56:08AM +0200, Andrzej Hajda wrote:
> On 07/18/2014 03:49 AM, YoungJun Cho wrote:
> > Hi Thierry,
> >
> > Thank you a lot for kind comments.
> >
> > On 07/17/2014 07:36 PM, Thierry Reding wrote:
> >> On Thu, Jul 17, 2014 at 06:01:25PM +0900, YoungJun Cho wrote:
> >> [...]
> >>> diff --git a/drivers/gpu/drm/panel/panel-s6e3fa0.c b/drivers/gpu/drm/panel/panel-s6e3fa0.c
> >> [...]
> >>> +/* Manufacturer Command Set */
> >>> +#define MCS_GLOBAL_PARAMETER	0xb0
> >>> +#define MCS_AID			0xb2
> >>> +#define MCS_ELVSSOPT		0xb6
> >>> +#define MCS_TEMPERATURE_SET	0xb8
> >>> +#define MCS_PENTILE_CTRL	0xc0
> >>> +#define MCS_GAMMA_MODE		0xca
> >>> +#define MCS_VDDM		0xd7
> >>> +#define MCS_ALS			0xe3
> >>> +#define MCS_ERR_FG		0xed
> >>> +#define MCS_KEY_LEV1		0xf0
> >>> +#define MCS_GAMMA_UPDATE	0xf7
> >>> +#define MCS_KEY_LEV2		0xfc
> >>> +#define MCS_RE			0xfe
> >>> +#define MCS_TOUT2_HSYNC		0xff
> >>> +
> >>> +/* Content Adaptive Brightness Control */
> >>> +#define DCS_WRITE_CABC		0x55
> >> Is this not a manufacturer specific command? I couldn't find it in the
> >> DSI or DCS specifications, but it sounds like something standard (also
> >> indicated by the DCS_ prefix). Can you point out the specification for
> >> this?
> >>
> > Andrzej commented before and decided it as DCS one because if the value 
> > is less than 0xb0, it is DCS one and the others are MCS one.
> > But still I'm not sure it is correct.
> I would not say 'decided'. I have just stated that according to DCS
> specification
> it should be DCS command (below 0xb0), but it is not present in mipi dcs
> specs.
> On the other side many panels have it [1]:
> 
> [1]:
> https://www.google.com/search?q=%22Write+Content+Adaptive+Brightness+Control%22

Yeah, my search yielded similar results. I wonder if this is perhaps
part of a draft future specification. I'll try to ask around some more
if anybody knows what the status of this is.

> >>> +static void s6e3fa0_read_mtp_id(struct s6e3fa0 *ctx)
> >>> +{
> >>> +	unsigned char id[MTP_ID_LEN];
> >>> +	int ret;
> >>> +
> >>> +	s6e3fa0_set_maximum_return_packet_size(ctx, MTP_ID_LEN);
> >>> +	ret = s6e3fa0_dcs_read(ctx, MIPI_DCS_GET_DISPLAY_ID, id, MTP_ID_LEN);
> >> This also looks like a standard DCS command. I can't find it in either
> >> v1.01 nor v1.02 of the specification, though. Do you know where it's
> >> specified?
> >>
> > Yes, I also can't find it in DCS specification and there is no special 
> > description in panel datasheet.
> > But as it is declared in "include/video/mipi_display.h", so I think it 
> > is a standard one.
> 
> On page 9 of the "Introduction of MIPI by Renesas" [2] there is info
> it is a part of "Nokia I/F command list", I guess it is kind of alpha
> version of MIPI DCS.
> 
> [2]: http://wenku.baidu.com/view/658fab68af1ffc4ffe47acbe.html

That link is the only one which contains "Nokia I/F command list" on the
internet (that is, according to Google). But since this is already part
of the mipi_display.h header file we may as well use it.

I wonder if perhaps the READ_DDB_START and READ_DDB_CONTINUE commands
could be used as a replacement for this display ID.

Adding Guennadi, Tomi, Paul and Imre on Cc since they were involved with
the original patch that added mipi_display.h. Perhaps they remember what
the origin of this command is.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 10/14] drm/panel: add S6E3FA0 driver

[Andrzej Hajda]

On 07/21/2014 11:19 AM, Thierry Reding wrote:
> On Mon, Jul 21, 2014 at 10:56:08AM +0200, Andrzej Hajda wrote:
>> On 07/18/2014 03:49 AM, YoungJun Cho wrote:
>>> Hi Thierry,
>>>
>>> Thank you a lot for kind comments.
>>>
>>> On 07/17/2014 07:36 PM, Thierry Reding wrote:
>>>> On Thu, Jul 17, 2014 at 06:01:25PM +0900, YoungJun Cho wrote:
>>>> [...]
>>>>> diff --git a/drivers/gpu/drm/panel/panel-s6e3fa0.c b/drivers/gpu/drm/panel/panel-s6e3fa0.c
>>>> [...]
>>>>> +/* Manufacturer Command Set */
>>>>> +#define MCS_GLOBAL_PARAMETER	0xb0
>>>>> +#define MCS_AID			0xb2
>>>>> +#define MCS_ELVSSOPT		0xb6
>>>>> +#define MCS_TEMPERATURE_SET	0xb8
>>>>> +#define MCS_PENTILE_CTRL	0xc0
>>>>> +#define MCS_GAMMA_MODE		0xca
>>>>> +#define MCS_VDDM		0xd7
>>>>> +#define MCS_ALS			0xe3
>>>>> +#define MCS_ERR_FG		0xed
>>>>> +#define MCS_KEY_LEV1		0xf0
>>>>> +#define MCS_GAMMA_UPDATE	0xf7
>>>>> +#define MCS_KEY_LEV2		0xfc
>>>>> +#define MCS_RE			0xfe
>>>>> +#define MCS_TOUT2_HSYNC		0xff
>>>>> +
>>>>> +/* Content Adaptive Brightness Control */
>>>>> +#define DCS_WRITE_CABC		0x55
>>>> Is this not a manufacturer specific command? I couldn't find it in the
>>>> DSI or DCS specifications, but it sounds like something standard (also
>>>> indicated by the DCS_ prefix). Can you point out the specification for
>>>> this?
>>>>
>>> Andrzej commented before and decided it as DCS one because if the value 
>>> is less than 0xb0, it is DCS one and the others are MCS one.
>>> But still I'm not sure it is correct.
>> I would not say 'decided'. I have just stated that according to DCS
>> specification
>> it should be DCS command (below 0xb0), but it is not present in mipi dcs
>> specs.
>> On the other side many panels have it [1]:
>>
>> [1]:
>> https://www.google.com/search?q=%22Write+Content+Adaptive+Brightness+Control%22
> 
> Yeah, my search yielded similar results. I wonder if this is perhaps
> part of a draft future specification. I'll try to ask around some more
> if anybody knows what the status of this is.
> 
>>>>> +static void s6e3fa0_read_mtp_id(struct s6e3fa0 *ctx)
>>>>> +{
>>>>> +	unsigned char id[MTP_ID_LEN];
>>>>> +	int ret;
>>>>> +
>>>>> +	s6e3fa0_set_maximum_return_packet_size(ctx, MTP_ID_LEN);
>>>>> +	ret = s6e3fa0_dcs_read(ctx, MIPI_DCS_GET_DISPLAY_ID, id, MTP_ID_LEN);
>>>> This also looks like a standard DCS command. I can't find it in either
>>>> v1.01 nor v1.02 of the specification, though. Do you know where it's
>>>> specified?
>>>>
>>> Yes, I also can't find it in DCS specification and there is no special 
>>> description in panel datasheet.
>>> But as it is declared in "include/video/mipi_display.h", so I think it 
>>> is a standard one.
>>
>> On page 9 of the "Introduction of MIPI by Renesas" [2] there is info
>> it is a part of "Nokia I/F command list", I guess it is kind of alpha
>> version of MIPI DCS.
>>
>> [2]: http://wenku.baidu.com/view/658fab68af1ffc4ffe47acbe.html
> 
> That link is the only one which contains "Nokia I/F command list" on the
> internet (that is, according to Google). But since this is already part
> of the mipi_display.h header file we may as well use it.
> 
> I wonder if perhaps the READ_DDB_START and READ_DDB_CONTINUE commands
> could be used as a replacement for this display ID.
> 
> Adding Guennadi, Tomi, Paul and Imre on Cc since they were involved with
> the original patch that added mipi_display.h. Perhaps they remember what
> the origin of this command is.


I guess it was PCF8833 used in Nokia 6100 [1][2].

[1]: http://www.vintagecomputercables.com/datasheet/PCF8833_1.pdf
[2]:
http://www.elecfreaks.com/store/download/datasheet/shield/6100_Display_Driver.pdf

Regards
Andrzej

> 
> Thierry
> 

Re: [PATCH v6 10/14] drm/panel: add S6E3FA0 driver

[YoungJun Cho]

Hi,

On 07/21/2014 08:18 PM, Andrzej Hajda wrote:
> On 07/21/2014 11:19 AM, Thierry Reding wrote:
>> On Mon, Jul 21, 2014 at 10:56:08AM +0200, Andrzej Hajda wrote:
>>> On 07/18/2014 03:49 AM, YoungJun Cho wrote:
>>>> Hi Thierry,
>>>>
>>>> Thank you a lot for kind comments.
>>>>
>>>> On 07/17/2014 07:36 PM, Thierry Reding wrote:
>>>>> On Thu, Jul 17, 2014 at 06:01:25PM +0900, YoungJun Cho wrote:
>>>>> [...]
>>>>>> diff --git a/drivers/gpu/drm/panel/panel-s6e3fa0.c b/drivers/gpu/drm/panel/panel-s6e3fa0.c
>>>>> [...]
>>>>>> +/* Manufacturer Command Set */
>>>>>> +#define MCS_GLOBAL_PARAMETER	0xb0
>>>>>> +#define MCS_AID			0xb2
>>>>>> +#define MCS_ELVSSOPT		0xb6
>>>>>> +#define MCS_TEMPERATURE_SET	0xb8
>>>>>> +#define MCS_PENTILE_CTRL	0xc0
>>>>>> +#define MCS_GAMMA_MODE		0xca
>>>>>> +#define MCS_VDDM		0xd7
>>>>>> +#define MCS_ALS			0xe3
>>>>>> +#define MCS_ERR_FG		0xed
>>>>>> +#define MCS_KEY_LEV1		0xf0
>>>>>> +#define MCS_GAMMA_UPDATE	0xf7
>>>>>> +#define MCS_KEY_LEV2		0xfc
>>>>>> +#define MCS_RE			0xfe
>>>>>> +#define MCS_TOUT2_HSYNC		0xff
>>>>>> +
>>>>>> +/* Content Adaptive Brightness Control */
>>>>>> +#define DCS_WRITE_CABC		0x55
>>>>> Is this not a manufacturer specific command? I couldn't find it in the
>>>>> DSI or DCS specifications, but it sounds like something standard (also
>>>>> indicated by the DCS_ prefix). Can you point out the specification for
>>>>> this?
>>>>>
>>>> Andrzej commented before and decided it as DCS one because if the value
>>>> is less than 0xb0, it is DCS one and the others are MCS one.
>>>> But still I'm not sure it is correct.
>>> I would not say 'decided'. I have just stated that according to DCS
>>> specification
>>> it should be DCS command (below 0xb0), but it is not present in mipi dcs
>>> specs.
>>> On the other side many panels have it [1]:
>>>
>>> [1]:
>>> https://www.google.com/search?q=%22Write+Content+Adaptive+Brightness+Control%22
>>
>> Yeah, my search yielded similar results. I wonder if this is perhaps
>> part of a draft future specification. I'll try to ask around some more
>> if anybody knows what the status of this is.
>>
>>>>>> +static void s6e3fa0_read_mtp_id(struct s6e3fa0 *ctx)
>>>>>> +{
>>>>>> +	unsigned char id[MTP_ID_LEN];
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	s6e3fa0_set_maximum_return_packet_size(ctx, MTP_ID_LEN);
>>>>>> +	ret = s6e3fa0_dcs_read(ctx, MIPI_DCS_GET_DISPLAY_ID, id, MTP_ID_LEN);
>>>>> This also looks like a standard DCS command. I can't find it in either
>>>>> v1.01 nor v1.02 of the specification, though. Do you know where it's
>>>>> specified?
>>>>>
>>>> Yes, I also can't find it in DCS specification and there is no special
>>>> description in panel datasheet.
>>>> But as it is declared in "include/video/mipi_display.h", so I think it
>>>> is a standard one.
>>>
>>> On page 9 of the "Introduction of MIPI by Renesas" [2] there is info
>>> it is a part of "Nokia I/F command list", I guess it is kind of alpha
>>> version of MIPI DCS.
>>>
>>> [2]: http://wenku.baidu.com/view/658fab68af1ffc4ffe47acbe.html
>>
>> That link is the only one which contains "Nokia I/F command list" on the
>> internet (that is, according to Google). But since this is already part
>> of the mipi_display.h header file we may as well use it.
>>
>> I wonder if perhaps the READ_DDB_START and READ_DDB_CONTINUE commands
>> could be used as a replacement for this display ID.
>>

There is a simple description for "Read DDB Start(A1H)" like below.
- This command returns supplier identification and display module model 
/ revision information.
- NOTE: This information is not the same what Read IDs(04H) command is 
returning.

For reference, Read IDs(04H) description is like below.
- This read command returns 24-bit display identification information.
- The first read byte identifies the OLED module's manufacturer.
- The Second read byte is used to track the OLED module/driver version.
- The third read byte identifies the OLED module/driver.

>> Adding Guennadi, Tomi, Paul and Imre on Cc since they were involved with
>> the original patch that added mipi_display.h. Perhaps they remember what
>> the origin of this command is.
>
>
> I guess it was PCF8833 used in Nokia 6100 [1][2].
>
> [1]: http://www.vintagecomputercables.com/datasheet/PCF8833_1.pdf
> [2]:
> http://www.elecfreaks.com/store/download/datasheet/shield/6100_Display_Driver.pdf
>

Yes, this command is related with Nokia.

Last Friday, I met panel vendor guy for some issues.
At the break time, I asked him for about this Read IDs(04H), why it is 
not included in DCS specification.
He said that this command was originated by Nokia.
In feature phone times, most of panel vendors wanted their panels to be 
used by Nokia and Nokia wanted this command, so most of panel vendors 
still provide this command traditionally.

Thank you.
Best regards YJ

> Regards
> Andrzej
>
>>
>> Thierry
>>
>
>

Re: [PATCH v6 10/14] drm/panel: add S6E3FA0 driver

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2730 bytes --]

On Tue, Jul 22, 2014 at 12:41:21PM +0900, YoungJun Cho wrote:
> On 07/21/2014 08:18 PM, Andrzej Hajda wrote:
> >On 07/21/2014 11:19 AM, Thierry Reding wrote:
> >>On Mon, Jul 21, 2014 at 10:56:08AM +0200, Andrzej Hajda wrote:
> >>>On 07/18/2014 03:49 AM, YoungJun Cho wrote:
> >>>>On 07/17/2014 07:36 PM, Thierry Reding wrote:
> >>>>>On Thu, Jul 17, 2014 at 06:01:25PM +0900, YoungJun Cho wrote:
[...]
> >>>>>>+static void s6e3fa0_read_mtp_id(struct s6e3fa0 *ctx)
> >>>>>>+{
> >>>>>>+	unsigned char id[MTP_ID_LEN];
> >>>>>>+	int ret;
> >>>>>>+
> >>>>>>+	s6e3fa0_set_maximum_return_packet_size(ctx, MTP_ID_LEN);
> >>>>>>+	ret = s6e3fa0_dcs_read(ctx, MIPI_DCS_GET_DISPLAY_ID, id, MTP_ID_LEN);
> >>>>>This also looks like a standard DCS command. I can't find it in either
> >>>>>v1.01 nor v1.02 of the specification, though. Do you know where it's
> >>>>>specified?
> >>>>>
> >>>>Yes, I also can't find it in DCS specification and there is no special
> >>>>description in panel datasheet.
> >>>>But as it is declared in "include/video/mipi_display.h", so I think it
> >>>>is a standard one.
> >>>
> >>>On page 9 of the "Introduction of MIPI by Renesas" [2] there is info
> >>>it is a part of "Nokia I/F command list", I guess it is kind of alpha
> >>>version of MIPI DCS.
> >>>
> >>>[2]: http://wenku.baidu.com/view/658fab68af1ffc4ffe47acbe.html
> >>
> >>That link is the only one which contains "Nokia I/F command list" on the
> >>internet (that is, according to Google). But since this is already part
> >>of the mipi_display.h header file we may as well use it.
> >>
> >>I wonder if perhaps the READ_DDB_START and READ_DDB_CONTINUE commands
> >>could be used as a replacement for this display ID.
> >>
> 
> There is a simple description for "Read DDB Start(A1H)" like below.
> - This command returns supplier identification and display module model /
> revision information.
> - NOTE: This information is not the same what Read IDs(04H) command is
> returning.
> 
> For reference, Read IDs(04H) description is like below.
> - This read command returns 24-bit display identification information.
> - The first read byte identifies the OLED module's manufacturer.
> - The Second read byte is used to track the OLED module/driver version.
> - The third read byte identifies the OLED module/driver.

Okay, that explains things a little better. Can you point me to the
document that this is from?

But what I was trying to say is that if the Read IDs command isn't an
official DCS command, maybe it would be a better idea to use the DDB
instead. I assume that even if it isn't the same information it would
at least be a superset and therefore a suitable replacement.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 10/14] drm/panel: add S6E3FA0 driver

[YoungJun Cho]

Hi Thierry,

On 07/22/2014 04:49 PM, Thierry Reding wrote:
> On Tue, Jul 22, 2014 at 12:41:21PM +0900, YoungJun Cho wrote:
>> On 07/21/2014 08:18 PM, Andrzej Hajda wrote:
>>> On 07/21/2014 11:19 AM, Thierry Reding wrote:
>>>> On Mon, Jul 21, 2014 at 10:56:08AM +0200, Andrzej Hajda wrote:
>>>>> On 07/18/2014 03:49 AM, YoungJun Cho wrote:
>>>>>> On 07/17/2014 07:36 PM, Thierry Reding wrote:
>>>>>>> On Thu, Jul 17, 2014 at 06:01:25PM +0900, YoungJun Cho wrote:
> [...]
>>>>>>>> +static void s6e3fa0_read_mtp_id(struct s6e3fa0 *ctx)
>>>>>>>> +{
>>>>>>>> +	unsigned char id[MTP_ID_LEN];
>>>>>>>> +	int ret;
>>>>>>>> +
>>>>>>>> +	s6e3fa0_set_maximum_return_packet_size(ctx, MTP_ID_LEN);
>>>>>>>> +	ret = s6e3fa0_dcs_read(ctx, MIPI_DCS_GET_DISPLAY_ID, id, MTP_ID_LEN);
>>>>>>> This also looks like a standard DCS command. I can't find it in either
>>>>>>> v1.01 nor v1.02 of the specification, though. Do you know where it's
>>>>>>> specified?
>>>>>>>
>>>>>> Yes, I also can't find it in DCS specification and there is no special
>>>>>> description in panel datasheet.
>>>>>> But as it is declared in "include/video/mipi_display.h", so I think it
>>>>>> is a standard one.
>>>>>
>>>>> On page 9 of the "Introduction of MIPI by Renesas" [2] there is info
>>>>> it is a part of "Nokia I/F command list", I guess it is kind of alpha
>>>>> version of MIPI DCS.
>>>>>
>>>>> [2]: http://wenku.baidu.com/view/658fab68af1ffc4ffe47acbe.html
>>>>
>>>> That link is the only one which contains "Nokia I/F command list" on the
>>>> internet (that is, according to Google). But since this is already part
>>>> of the mipi_display.h header file we may as well use it.
>>>>
>>>> I wonder if perhaps the READ_DDB_START and READ_DDB_CONTINUE commands
>>>> could be used as a replacement for this display ID.
>>>>
>>
>> There is a simple description for "Read DDB Start(A1H)" like below.
>> - This command returns supplier identification and display module model /
>> revision information.
>> - NOTE: This information is not the same what Read IDs(04H) command is
>> returning.
>>
>> For reference, Read IDs(04H) description is like below.
>> - This read command returns 24-bit display identification information.
>> - The first read byte identifies the OLED module's manufacturer.
>> - The Second read byte is used to track the OLED module/driver version.
>> - The third read byte identifies the OLED module/driver.
>
> Okay, that explains things a little better. Can you point me to the
> document that this is from?

Sorry, I forgot to write specification name.
This specification is s6e3fa0 data sheet and it is confidential.
So I quoted only that portion.

>
> But what I was trying to say is that if the Read IDs command isn't an
> official DCS command, maybe it would be a better idea to use the DDB
> instead. I assume that even if it isn't the same information it would
> at least be a superset and therefore a suitable replacement.

This panel has several versions and each should set specific tuning 
value especially for AID, ELVSS and etc.
(Current is suitable for id[2] == 0x23).

I'll check READ_DDB_START & READ_DDB_CONTINUE result and try to use them 
if it is possible.

Thank you.
Best regards YJ

>
> Thierry
>

Re: [PATCH v6 10/14] drm/panel: add S6E3FA0 driver

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 667 bytes --]

On 22/07/14 10:49, Thierry Reding wrote:

> But what I was trying to say is that if the Read IDs command isn't an
> official DCS command, maybe it would be a better idea to use the DDB
> instead. I assume that even if it isn't the same information it would
> at least be a superset and therefore a suitable replacement.

Only if DDB commands work on that panel =). Even if a panel supports
DCS, it doesn't mean it supports all the commands.

Also, does it really matter which one to use inside a panel driver? I
don't really see any pros nor cons with either option. Except, of
course, if using one of those makes the driver's code simpler.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 10/14] drm/panel: add S6E3FA0 driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]

On Wed, Jul 30, 2014 at 04:44:21PM +0300, Tomi Valkeinen wrote:
> On 22/07/14 10:49, Thierry Reding wrote:
> 
> > But what I was trying to say is that if the Read IDs command isn't an
> > official DCS command, maybe it would be a better idea to use the DDB
> > instead. I assume that even if it isn't the same information it would
> > at least be a superset and therefore a suitable replacement.
> 
> Only if DDB commands work on that panel =). Even if a panel supports
> DCS, it doesn't mean it supports all the commands.

Indeed. I was perhaps a little naïve and assumed this was such a great
standard command that every panel simply had to support it. But so far
I haven't yet come across a single panel that does...

> Also, does it really matter which one to use inside a panel driver? I
> don't really see any pros nor cons with either option. Except, of
> course, if using one of those makes the driver's code simpler.

Yeah, at this point I don't see why the read IDs command shouldn't be
used. It's somewhat unfortunate that it isn't mentioned in the DCS
specification at all, but specifications are only as useful to the
degree that they get implemented...

My hope had been that we would be able to automatically probe for panels
using the DDB, but it seems like that's not a practicable idea given the
almost non-existent support for it.

So as long as we can standardize on common APIs rather than per-driver
implementations, I'm good.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 10/14] drm/panel: add S6E3FA0 driver

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 622 bytes --]

On 22/07/14 06:41, YoungJun Cho wrote:

> Yes, this command is related with Nokia.
> 
> Last Friday, I met panel vendor guy for some issues.
> At the break time, I asked him for about this Read IDs(04H), why it is
> not included in DCS specification.
> He said that this command was originated by Nokia.
> In feature phone times, most of panel vendors wanted their panels to be
> used by Nokia and Nokia wanted this command, so most of panel vendors
> still provide this command traditionally.

This is my understanding also. I think the whole MIPI DCS spec
originated from Nokia's command set.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v6 10/14] drm/panel: add S6E3FA0 driver

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2466 bytes --]

On Fri, Jul 18, 2014 at 10:49:35AM +0900, YoungJun Cho wrote:
> Hi Thierry,
> 
> Thank you a lot for kind comments.
> 
> On 07/17/2014 07:36 PM, Thierry Reding wrote:
> > On Thu, Jul 17, 2014 at 06:01:25PM +0900, YoungJun Cho wrote:
[...]
> > >diff --git a/drivers/gpu/drm/panel/panel-s6e3fa0.c b/drivers/gpu/drm/panel/panel-s6e3fa0.c
[...]
> > >+static void s6e3fa0_set_maximum_return_packet_size(struct s6e3fa0 *ctx,
> > >+							unsigned int size)
> > >+{
> > >+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> > >+	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
> > >+
> > >+	if (ops && ops->transfer) {
> > >+		unsigned char buf[] = {size, 0};
> > >+		struct mipi_dsi_msg msg = {
> > >+			.channel = dsi->channel,
> > >+			.type = MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE,
> > >+			.tx_len = sizeof(buf),
> > >+			.tx_buf = buf
> > >+		};
> > >+
> > >+		ops->transfer(dsi->host, &msg);
> > >+	}
> > >+}
> >
> > The Set Maximum Return Packet Size command is a standard command, so
> > please turn that into a generic function exposed by the DSI core.
> >
> 
> For this and below standard DCS commands, you want to use generic functions,
> but I have no idea for that.
> Could you explain more detail?

The goal should be to make these standard DCS commands available to all
DSI peripherals, so the implementation should look something like this:

static int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
						   u16 size)
{
	struct mipi_dsi_msg msg;

	if (!dsi->ops || !dsi->ops->transfer)
		return -ENOSYS;

	memset(&msg, 0, sizeof(msg));
	msg.channel = dsi->channel;
	msg.type = MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE;
	msg.tx_len = sizeof(size);
	msg.tx_buf = &size;

	return dsi->ops->transfer(dsi->host, &msg);
}

The above is somewhat special, since it isn't DCS. For DCS I'd suggest a
common prefix, like so:

enum mipi_dcs_tear_mode {
	MIPI_DCS_TEAR_VBLANK,
	MIPI_DCS_TEAR_BLANK,
};

static int mipi_dcs_set_tear_on(struct mipi_dsi_device *dsi,
				enum mipi_dcs_tear_mode mode)
{
	u8 data[2] = { MIPI_DSI_DCS_SET_TEAR_ON, mode };
	struct mipi_dsi_msg msg;

	if (!dsi->ops || !dsi->ops->transfer)
		return -ENOSYS;

	memset(&msg, 0, sizeof(msg));
	msg.channel = dsi->channel;
	msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM;
	msg.tx_len = sizeof(data);
	msg.tx_buf = data;

	return dsi->ops->transfer(dsi->host, &msg);
}

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 10/14] drm/panel: add S6E3FA0 driver

[YoungJun Cho]

Hi Thierry,

Now I understand what you mean.

I'll implement common DSI helper functions.

Thank you.
Best regards YJ

On 07/21/2014 06:35 PM, Thierry Reding wrote:
> On Fri, Jul 18, 2014 at 10:49:35AM +0900, YoungJun Cho wrote:
>> Hi Thierry,
>>
>> Thank you a lot for kind comments.
>>
>> On 07/17/2014 07:36 PM, Thierry Reding wrote:
>>> On Thu, Jul 17, 2014 at 06:01:25PM +0900, YoungJun Cho wrote:
> [...]
>>>> diff --git a/drivers/gpu/drm/panel/panel-s6e3fa0.c b/drivers/gpu/drm/panel/panel-s6e3fa0.c
> [...]
>>>> +static void s6e3fa0_set_maximum_return_packet_size(struct s6e3fa0 *ctx,
>>>> +							unsigned int size)
>>>> +{
>>>> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>>>> +	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
>>>> +
>>>> +	if (ops && ops->transfer) {
>>>> +		unsigned char buf[] = {size, 0};
>>>> +		struct mipi_dsi_msg msg = {
>>>> +			.channel = dsi->channel,
>>>> +			.type = MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE,
>>>> +			.tx_len = sizeof(buf),
>>>> +			.tx_buf = buf
>>>> +		};
>>>> +
>>>> +		ops->transfer(dsi->host, &msg);
>>>> +	}
>>>> +}
>>>
>>> The Set Maximum Return Packet Size command is a standard command, so
>>> please turn that into a generic function exposed by the DSI core.
>>>
>>
>> For this and below standard DCS commands, you want to use generic functions,
>> but I have no idea for that.
>> Could you explain more detail?
>
> The goal should be to make these standard DCS commands available to all
> DSI peripherals, so the implementation should look something like this:
>
> static int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
> 						   u16 size)
> {
> 	struct mipi_dsi_msg msg;
>
> 	if (!dsi->ops || !dsi->ops->transfer)
> 		return -ENOSYS;
>
> 	memset(&msg, 0, sizeof(msg));
> 	msg.channel = dsi->channel;
> 	msg.type = MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE;
> 	msg.tx_len = sizeof(size);
> 	msg.tx_buf = &size;
>
> 	return dsi->ops->transfer(dsi->host, &msg);
> }
>
> The above is somewhat special, since it isn't DCS. For DCS I'd suggest a
> common prefix, like so:
>
> enum mipi_dcs_tear_mode {
> 	MIPI_DCS_TEAR_VBLANK,
> 	MIPI_DCS_TEAR_BLANK,
> };
>
> static int mipi_dcs_set_tear_on(struct mipi_dsi_device *dsi,
> 				enum mipi_dcs_tear_mode mode)
> {
> 	u8 data[2] = { MIPI_DSI_DCS_SET_TEAR_ON, mode };
> 	struct mipi_dsi_msg msg;
>
> 	if (!dsi->ops || !dsi->ops->transfer)
> 		return -ENOSYS;
>
> 	memset(&msg, 0, sizeof(msg));
> 	msg.channel = dsi->channel;
> 	msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM;
> 	msg.tx_len = sizeof(data);
> 	msg.tx_buf = data;
>
> 	return dsi->ops->transfer(dsi->host, &msg);
> }
>
> Thierry
>

Re: [PATCH v6 10/14] drm/panel: add S6E3FA0 driver

[YoungJun Cho]

Hi Thierry,

Sorry for late reply.

I implemented common DSI helper functions and tested in s6e3fa0 panel
(I should test with other panels).

The helper function prototypes are like below:

int mipi_dsi_set_maximum_return_packet_size(struct mipi_dsi_device *dsi,
						u16 size);

int mipi_dcs_enter_sleep_mode(struct mipi_dsi_device *dsi);
int mipi_dcs_exit_sleep_mode(struct mipi_dsi_device *dsi);
int mipi_dcs_set_display_off(struct mipi_dsi_device *dsi);
int mipi_dcs_set_display_on(struct mipi_dsi_device *dsi);
int mipi_dcs_set_tear_off(struct mipi_dsi_device *dsi);

enum mipi_dcs_tear_mode {
	MIPI_DCS_TEAR_MODE_VBLANK,
	MIPI_DCS_TEAR_MODE_HVBLANK,
};

int mipi_dcs_set_tear_on(struct mipi_dsi_device *dsi,
				enum mipi_dcs_tear_mode mode);

Last time you recommended to implement mipi_dcs_set_tear_on() unrelated 
with mipi_dsi_dcs_write().
As you know, the only difference between mipi_dcs_xxx() except 
mipi_dcs_set_tear_on() is data(dcs command).
So I think it's better to use mipi_dsi_dcs_write() instead.
Do you agree?

And one more thing.
 From my point of view there is no initialization sequence in simple 
panel driver, so this and s6e8aa0 panel couldn't use that interface.
The s6e3fa0 and s6e8aa0 are very similar so I think it is possible to 
combine together like simple panel driver.
I want to ask you for advice on this.

Thank you.
Best regards YJ

On 07/22/2014 12:56 PM, YoungJun Cho wrote:
> Hi Thierry,
>
> Now I understand what you mean.
>
> I'll implement common DSI helper functions.
>
> Thank you.
> Best regards YJ
>
> On 07/21/2014 06:35 PM, Thierry Reding wrote:
>> On Fri, Jul 18, 2014 at 10:49:35AM +0900, YoungJun Cho wrote:
>>> Hi Thierry,
>>>
>>> Thank you a lot for kind comments.
>>>
>>> On 07/17/2014 07:36 PM, Thierry Reding wrote:
>>>> On Thu, Jul 17, 2014 at 06:01:25PM +0900, YoungJun Cho wrote:
>> [...]
>>>>> diff --git a/drivers/gpu/drm/panel/panel-s6e3fa0.c
>>>>> b/drivers/gpu/drm/panel/panel-s6e3fa0.c
>> [...]
>>>>> +static void s6e3fa0_set_maximum_return_packet_size(struct s6e3fa0
>>>>> *ctx,
>>>>> +                            unsigned int size)
>>>>> +{
>>>>> +    struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>>>>> +    const struct mipi_dsi_host_ops *ops = dsi->host->ops;
>>>>> +
>>>>> +    if (ops && ops->transfer) {
>>>>> +        unsigned char buf[] = {size, 0};
>>>>> +        struct mipi_dsi_msg msg = {
>>>>> +            .channel = dsi->channel,
>>>>> +            .type = MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE,
>>>>> +            .tx_len = sizeof(buf),
>>>>> +            .tx_buf = buf
>>>>> +        };
>>>>> +
>>>>> +        ops->transfer(dsi->host, &msg);
>>>>> +    }
>>>>> +}
>>>>
>>>> The Set Maximum Return Packet Size command is a standard command, so
>>>> please turn that into a generic function exposed by the DSI core.
>>>>
>>>
>>> For this and below standard DCS commands, you want to use generic
>>> functions,
>>> but I have no idea for that.
>>> Could you explain more detail?
>>
>> The goal should be to make these standard DCS commands available to all
>> DSI peripherals, so the implementation should look something like this:
>>
>> static int mipi_dsi_set_maximum_return_packet_size(struct
>> mipi_dsi_device *dsi,
>>                            u16 size)
>> {
>>     struct mipi_dsi_msg msg;
>>
>>     if (!dsi->ops || !dsi->ops->transfer)
>>         return -ENOSYS;
>>
>>     memset(&msg, 0, sizeof(msg));
>>     msg.channel = dsi->channel;
>>     msg.type = MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE;
>>     msg.tx_len = sizeof(size);
>>     msg.tx_buf = &size;
>>
>>     return dsi->ops->transfer(dsi->host, &msg);
>> }
>>
>> The above is somewhat special, since it isn't DCS. For DCS I'd suggest a
>> common prefix, like so:
>>
>> enum mipi_dcs_tear_mode {
>>     MIPI_DCS_TEAR_VBLANK,
>>     MIPI_DCS_TEAR_BLANK,
>> };
>>
>> static int mipi_dcs_set_tear_on(struct mipi_dsi_device *dsi,
>>                 enum mipi_dcs_tear_mode mode)
>> {
>>     u8 data[2] = { MIPI_DSI_DCS_SET_TEAR_ON, mode };
>>     struct mipi_dsi_msg msg;
>>
>>     if (!dsi->ops || !dsi->ops->transfer)
>>         return -ENOSYS;
>>
>>     memset(&msg, 0, sizeof(msg));
>>     msg.channel = dsi->channel;
>>     msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM;
>>     msg.tx_len = sizeof(data);
>>     msg.tx_buf = data;
>>
>>     return dsi->ops->transfer(dsi->host, &msg);
>> }
>>
>> Thierry
>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[PATCH v6 11/14] ARM: dts: exynos4: add system register property

[YoungJun Cho]

This patch adds sysreg property to fimd device node
which is required to use I80 interface.

Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos4.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/exynos4.dtsi b/arch/arm/boot/dts/exynos4.dtsi
index fbaf426..3793881 100644
--- a/arch/arm/boot/dts/exynos4.dtsi
+++ b/arch/arm/boot/dts/exynos4.dtsi
@@ -608,6 +608,7 @@
 		clocks = <&clock CLK_SCLK_FIMD0>, <&clock CLK_FIMD0>;
 		clock-names = "sclk_fimd", "fimd";
 		samsung,power-domain = <&pd_lcd0>;
+		samsung,sysreg = <&sys_reg>;
 		status = "disabled";
 	};
 };
-- 
1.9.0

[PATCH v6 12/14] ARM: dts: exynos5: add system register property

[YoungJun Cho]

This patch adds sysreg property to fimd device node
which is required to use I80 interface.

Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos5.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/exynos5.dtsi b/arch/arm/boot/dts/exynos5.dtsi
index 79d0608..fdead12 100644
--- a/arch/arm/boot/dts/exynos5.dtsi
+++ b/arch/arm/boot/dts/exynos5.dtsi
@@ -87,6 +87,7 @@
 		reg = <0x14400000 0x40000>;
 		interrupt-names = "fifo", "vsync", "lcd_sys";
 		interrupts = <18 4>, <18 5>, <18 6>;
+		samsung,sysreg = <&sysreg_system_controller>;
 		status = "disabled";
 	};
 
-- 
1.9.0

[PATCH v6 13/14] ARM: dts: exynos5420: add mipi-phy node

[YoungJun Cho]

This patch adds mipi-phy node for MIPI DSI device.

Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos5420.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index e385322..0b9d15d 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -517,6 +517,12 @@
 		phy-names = "dp";
 	};
 
+	mipi_phy: video-phy@10040714 {
+		compatible = "samsung,s5pv210-mipi-video-phy";
+		reg = <0x10040714 12>;
+		#phy-cells = <1>;
+	};
+
 	fimd: fimd@14400000 {
 		samsung,power-domain = <&disp_pd>;
 		clocks = <&clock CLK_SCLK_FIMD1>, <&clock CLK_FIMD1>;
-- 
1.9.0

[PATCH v6 14/14] ARM: dts: exynos5420: add dsi node

[YoungJun Cho]

This patch adds common part of dsi node.

Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/boot/dts/exynos5420.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index 0b9d15d..3a7862b 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -523,6 +523,20 @@
 		#phy-cells = <1>;
 	};
 
+	dsi@14500000 {
+		compatible = "samsung,exynos5410-mipi-dsi";
+		reg = <0x14500000 0x10000>;
+		interrupts = <0 82 0>;
+		samsung,power-domain = <&disp_pd>;
+		phys = <&mipi_phy 1>;
+		phy-names = "dsim";
+		clocks = <&clock CLK_DSIM1>, <&clock CLK_SCLK_MIPI1>;
+		clock-names = "bus_clk", "pll_clk";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		status = "disabled";
+	};
+
 	fimd: fimd@14400000 {
 		samsung,power-domain = <&disp_pd>;
 		clocks = <&clock CLK_SCLK_FIMD1>, <&clock CLK_FIMD1>;
-- 
1.9.0

Re: [PATCH v6 00/14] drm/exynos: support LCD I80 interface display

[Inki Dae]

On 2014년 07월 17일 18:01, YoungJun Cho wrote:
> Hi,
> 
> This series adds LCD I80 interface display support for Exynos DRM driver.
> The FIMD(display controller) specification describes it as "LCD I80 interface"
> and the DSI specification describes it as "Command mode interface".
> 
> This is based on exynos-drm-next branch.
> 
> The previous patches,
> RFC: http://www.spinics.net/lists/dri-devel/msg58898.html
> V1: http://www.spinics.net/lists/dri-devel/msg59291.html
> V2: http://www.spinics.net/lists/dri-devel/msg59867.html
> V3: http://www.spinics.net/lists/dri-devel/msg60708.html
> V4: http://www.spinics.net/lists/dri-devel/msg60943.html
> V5: http://www.spinics.net/lists/dri-devel/msg62956.html
> 
> Changelog v2:
> - Fixes typo and removes unnecessary error log. (commented by Andrzej Hazda)
> - Adds missed pendlig_flip flag clear points. (commented by Daniel Kurtz)
> 
> Changelog v3:
> - Removes generic command mode and command mode display timing interface.
> - Moves I80 interface timings from panel DT to the FIMD(display controller) DT.
> 
> Changelog v4:
> - Removes exynos5 sysreg(syscon) DT bindings and node from dtsi because
>   it was already updated by linux-samsung-soc. (commented by Vivek Gautam)
> 
> Changelog v5:
> - Fixes FIMD vidcon0 register relevant code.
> - Fixes panel gamma table, disable sequence.
> - Slitely updates for code cleanup.
> 
> Changelog v6:
> - Removes pass TE host ops in dsi and exynos dsi uses TE irq handler instead,
>   and it is related with the TE GPIO of panel. (commented by Thierry Reding)
> 
> Patches 1 and 2 fix trivial bugs.
> 
> Patches 3, 4, 5 and 6 implement FIMD(display controller) I80 interface.
> The MIPI DSI command mode based panel generates Tearing Effect synchronization
> signal between MCU and FB to display video image, and FIMD should trigger to
> transfer video image at this signal.
> So the panel generates it and the dsi should receive the TE IRQ and call TE
> handler chains to notify it to the FIMD.
> 
> Patches 7 and 8 implement to use Exynos5410 / 5420 / 5440 SoC DSI driver
> which is different from previous Exynos4 SoCs for some registers control.
> 
> Patches 9 and 10 introduce MIPI DSI command mode based Samsung S6E3FA0 AMOLED
> 5.7" LCD drm panel driver.
> 
> The ohters add DT property nodes to support MIPI DSI command mode.
> 
> I welcome any comments.
> 
> Thank you.
> Best regards YJ
> 
> YoungJun Cho (14):
>   drm/exynos: dsi: move the EoT packets configuration point
>   drm/exynos: use wait_event_timeout() for safety usage
>   ARM: dts: samsung-fimd: add LCD I80 interface specific properties
>   drm/exynos: add TE handler to support LCD I80 interface
>   drm/exynos: dsi: add TE interrupt handler to support LCD I80 interface
>   drm/exynos: fimd: support LCD I80 interface
>   ARM: dts: exynos_dsim: add exynos5410 compatible to DT bindings
>   drm/exynos: dsi: add driver data to support Exynos5410/5420/5440 SoCs
>   ARM: dts: s6e3fa0: add DT bindings
>   drm/panel: add S6E3FA0 driver
>   ARM: dts: exynos4: add system register property
>   ARM: dts: exynos5: add system register property
>   ARM: dts: exynos5420: add mipi-phy node
>   ARM: dts: exynos5420: add dsi node

Piked them up except patch 9 and 10. The two patches are needed for more
review and consensus

Thanks,
Inki Dae

> 
>  .../devicetree/bindings/panel/samsung,s6e3fa0.txt  |  46 ++
>  .../devicetree/bindings/video/exynos_dsim.txt      |   4 +-
>  .../devicetree/bindings/video/samsung-fimd.txt     |  28 ++
>  arch/arm/boot/dts/exynos4.dtsi                     |   1 +
>  arch/arm/boot/dts/exynos5.dtsi                     |   1 +
>  arch/arm/boot/dts/exynos5420.dtsi                  |  20 +
>  drivers/gpu/drm/exynos/Kconfig                     |   1 +
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c           |  15 +-
>  drivers/gpu/drm/exynos/exynos_drm_crtc.h           |   7 +
>  drivers/gpu/drm/exynos/exynos_drm_drv.h            |   3 +
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c            | 266 +++++++++-
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c           | 276 +++++++++--
>  drivers/gpu/drm/panel/Kconfig                      |   7 +
>  drivers/gpu/drm/panel/Makefile                     |   1 +
>  drivers/gpu/drm/panel/panel-s6e3fa0.c              | 541 +++++++++++++++++++++
>  include/video/samsung_fimd.h                       |   3 +-
>  16 files changed, 1146 insertions(+), 74 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/panel/samsung,s6e3fa0.txt
>  create mode 100644 drivers/gpu/drm/panel/panel-s6e3fa0.c
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel