[PATCH V5 00/12] drm/exynos: few patches to enhance bridge chip support

[PATCH V5 00/12] drm/exynos: few patches to enhance bridge chip support

[Ajay Kumar]

This series is based on exynos-drm-next branch of Inki Dae's tree at:
git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git

This patchset also consolidates various inputs from the drm community
regarding the bridge chaining concept:
(1) [RFC V2 0/3] drm/bridge: panel and chaining
	http://www.spinics.net/lists/linux-samsung-soc/msg30160.html
(2) [RFC V3 0/3] drm/bridge: panel and chaining
	http://www.spinics.net/lists/linux-samsung-soc/msg30507.html

I have tested this after adding few DT changes for exynos5250-snow,
exynos5420-peach-pit and exynos5800-peach-pi boards.

The V4 series of this particular patchset was also tested by:
Rahul Sharma <rahul.sharma@samsung.com>
Javier Martinez Canillas <javier@dowhile0.org>

Changes since V2:
	-- Address comments from Jingoo Han for ps8622 driver
	-- Address comments from Daniel, Rob and Thierry regarding
	   bridge chaining
	-- Address comments from Thierry regarding the names for
	   new drm_panel functions

Changes since V3:
	-- Remove hotplug based initialization of exynos_dp
	-- Make exynos_dp work directly with drm_panel, remove
	   dependency on panel_binder
	-- Minor cleanups in panel_binder and panel_lvds driver

Changes since V4:
	-- Use gpiod interface for panel-lvds and ps8622 drivers.
	-- Address comments from Javier.
	-- Fix compilation issues when PANEL_BINDER is selected as module.
	-- Split Documentation patches from driver patches.
	-- Rebase on top of the tree.

Ajay Kumar (9):
  [RESEND PATCH V5 01/12] drm/exynos: Move DP setup out of hotplug workqueue
  [RESEND PATCH V5 02/12] drm/panel: add prepare and unprepare routines
  [RESEND PATCH V5 03/12] drm/exynos: dp: modify driver to support drm_panel
  [PATCH V5 04/12] drm/panel: Add driver for lvds/edp based panels
  [PATCH V5 05/12] Documentation: Add DT bindings for panel-lvds driver
  [RESEND PATCH V5 06/12] drm/bridge: add helper functions to support bridge chain
  [PATCH V5 07/12] drm/bridge: Add a driver which binds drm_bridge with drm_panel
  [RESEND PATCH V5 08/12] drm/bridge: ptn3460: Support bridge chaining
  [RESEND PATCH V5 09/12] drm/exynos: dp: create bridge chain using ptn3460 and panel_binder

Vincent Palatin (2):
  [PATCH V5 10/12] Documentation: Add DT bindings for ps8622/ps8625 bridge driver
  [PATCH V5 11/12] drm/bridge: Add ps8622/ps8625 bridge driver

Rahul Sharma (1):
  [RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver

 .../devicetree/bindings/drm/bridge/ps8622.txt      |   21 +
 .../devicetree/bindings/panel/panel-lvds.txt       |   50 ++
 .../devicetree/bindings/video/exynos_dp.txt        |    2 +
 drivers/gpu/drm/bridge/Kconfig                     |   15 +
 drivers/gpu/drm/bridge/Makefile                    |    2 +
 drivers/gpu/drm/bridge/panel_binder.c              |  193 ++++++++
 drivers/gpu/drm/bridge/ps8622.c                    |  476 ++++++++++++++++++++
 drivers/gpu/drm/bridge/ptn3460.c                   |  137 +-----
 drivers/gpu/drm/exynos/Kconfig                     |    1 +
 drivers/gpu/drm/exynos/exynos_dp_core.c            |   87 +++-
 drivers/gpu/drm/exynos/exynos_dp_core.h            |    2 +
 drivers/gpu/drm/panel/Kconfig                      |   10 +
 drivers/gpu/drm/panel/Makefile                     |    1 +
 drivers/gpu/drm/panel/panel-lvds.c                 |  268 +++++++++++
 include/drm/bridge/panel_binder.h                  |   44 ++
 include/drm/bridge/ps8622.h                        |   41 ++
 include/drm/bridge/ptn3460.h                       |   15 +-
 include/drm/drm_crtc.h                             |   72 +++
 include/drm/drm_panel.h                            |   18 +
 19 files changed, 1316 insertions(+), 139 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/drm/bridge/ps8622.txt
 create mode 100644 Documentation/devicetree/bindings/panel/panel-lvds.txt
 create mode 100644 drivers/gpu/drm/bridge/panel_binder.c
 create mode 100644 drivers/gpu/drm/bridge/ps8622.c
 create mode 100644 drivers/gpu/drm/panel/panel-lvds.c
 create mode 100644 include/drm/bridge/panel_binder.h
 create mode 100644 include/drm/bridge/ps8622.h

-- 
1.7.9.5

[RESEND PATCH V5 01/12] drm/exynos: Move DP setup out of hotplug workqueue

[Ajay Kumar]

Move the DP training and video enable from the hotplug handler into
a seperate function and call the same during dpms ON.

With existing code, DP HPD should be generated just few ms before
calling enable_irq in dp_poweron.

This patch removes that stringent time constraint.

Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_dp_core.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
index 845d766..a94b114 100644
--- a/drivers/gpu/drm/exynos/exynos_dp_core.c
+++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
@@ -875,10 +875,18 @@ static irqreturn_t exynos_dp_irq_handler(int irq, void *arg)
 static void exynos_dp_hotplug(struct work_struct *work)
 {
 	struct exynos_dp_device *dp;
-	int ret;
 
 	dp = container_of(work, struct exynos_dp_device, hotplug_work);
 
+	if (dp->drm_dev)
+		drm_helper_hpd_irq_event(dp->drm_dev);
+}
+
+static void exynos_dp_setup(void *in_ctx)
+{
+	struct exynos_dp_device *dp = in_ctx;
+	int ret;
+
 	ret = exynos_dp_detect_hpd(dp);
 	if (ret) {
 		/* Cable has been disconnected, we're done */
@@ -1059,6 +1067,7 @@ static void exynos_dp_poweron(struct exynos_dp_device *dp)
 	exynos_dp_phy_init(dp);
 	exynos_dp_init_dp(dp);
 	enable_irq(dp->irq);
+	exynos_dp_setup(dp);
 }
 
 static void exynos_dp_poweroff(struct exynos_dp_device *dp)
-- 
1.7.9.5

[RESEND PATCH V5 02/12] drm/panel: add prepare and unprepare routines

[Ajay Kumar]

Most of the panels need an init sequence as mentioned below:
	-- poweron LCD unit/LCD_EN
	-- start video data
	-- poweron LED unit/BACKLIGHT_EN
And, a de-init sequence as mentioned below:
	-- poweroff LED unit/BACKLIGHT_EN
	-- stop video data
	-- poweroff LCD unit/LCD_EN
With existing callbacks for drm panel, we cannot accomodate such panels,
since only two callbacks, i.e "panel_enable" and panel_disable are supported.

This patch adds:
	-- "prepare" callback which can be called before
	the actual video data is on, and then call the "enable"
	callback after the video data is available.

	-- "unprepare" callback which can be called after
	the video data is off, and use "disable" callback
	to do something before switching off the video data.

Now, we can easily map the above scenario as shown below:
	poweron LCD unit/LCD_EN = "prepare" callback
	poweron LED unit/BACKLIGHT_EN = "enable" callback
	poweroff LED unit/BACKLIGHT_EN = "disable" callback
	poweroff LCD unit/LCD_EN = "unprepare" callback

Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 include/drm/drm_panel.h |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index c2ab77a..9addc69 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -31,7 +31,9 @@ struct drm_device;
 struct drm_panel;
 
 struct drm_panel_funcs {
+	int (*unprepare)(struct drm_panel *panel);
 	int (*disable)(struct drm_panel *panel);
+	int (*prepare)(struct drm_panel *panel);
 	int (*enable)(struct drm_panel *panel);
 	int (*get_modes)(struct drm_panel *panel);
 };
@@ -46,6 +48,14 @@ struct drm_panel {
 	struct list_head list;
 };
 
+static inline int drm_panel_unprepare(struct drm_panel *panel)
+{
+	if (panel && panel->funcs && panel->funcs->unprepare)
+		return panel->funcs->unprepare(panel);
+
+	return panel ? -ENOSYS : -EINVAL;
+}
+
 static inline int drm_panel_disable(struct drm_panel *panel)
 {
 	if (panel && panel->funcs && panel->funcs->disable)
@@ -54,6 +64,14 @@ static inline int drm_panel_disable(struct drm_panel *panel)
 	return panel ? -ENOSYS : -EINVAL;
 }
 
+static inline int drm_panel_prepare(struct drm_panel *panel)
+{
+	if (panel && panel->funcs && panel->funcs->prepare)
+		return panel->funcs->prepare(panel);
+
+	return panel ? -ENOSYS : -EINVAL;
+}
+
 static inline int drm_panel_enable(struct drm_panel *panel)
 {
 	if (panel && panel->funcs && panel->funcs->enable)
-- 
1.7.9.5

[RESEND PATCH V5 03/12] drm/exynos: dp: modify driver to support drm_panel

[Ajay Kumar]

Add drm_panel controls to support powerup/down of the
eDP panel, if one is present at the sink side.

Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 .../devicetree/bindings/video/exynos_dp.txt        |    2 +
 drivers/gpu/drm/exynos/Kconfig                     |    1 +
 drivers/gpu/drm/exynos/exynos_dp_core.c            |   45 ++++++++++++++++----
 drivers/gpu/drm/exynos/exynos_dp_core.h            |    2 +
 4 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/exynos_dp.txt b/Documentation/devicetree/bindings/video/exynos_dp.txt
index 53dbccf..c029a09 100644
--- a/Documentation/devicetree/bindings/video/exynos_dp.txt
+++ b/Documentation/devicetree/bindings/video/exynos_dp.txt
@@ -51,6 +51,8 @@ Required properties for dp-controller:
 			LANE_COUNT1 = 1, LANE_COUNT2 = 2, LANE_COUNT4 = 4
 	- display-timings: timings for the connected panel as described by
 		Documentation/devicetree/bindings/video/display-timing.txt
+	-edp-panel:
+		phandle for the edp/lvds drm_panel node.
 
 Optional properties for dp-controller:
 	-interlaced:
diff --git a/drivers/gpu/drm/exynos/Kconfig b/drivers/gpu/drm/exynos/Kconfig
index 178d2a9..fd1c070 100644
--- a/drivers/gpu/drm/exynos/Kconfig
+++ b/drivers/gpu/drm/exynos/Kconfig
@@ -52,6 +52,7 @@ config DRM_EXYNOS_DP
 	bool "EXYNOS DRM DP driver support"
 	depends on DRM_EXYNOS_FIMD && ARCH_EXYNOS && (DRM_PTN3460=n || DRM_PTN3460=y || DRM_PTN3460=DRM_EXYNOS)
 	default DRM_EXYNOS
+	select DRM_PANEL
 	help
 	  This enables support for DP device.
 
diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
index a94b114..b3d0d9b 100644
--- a/drivers/gpu/drm/exynos/exynos_dp_core.c
+++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
@@ -28,6 +28,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_panel.h>
 #include <drm/bridge/ptn3460.h>
 
 #include "exynos_drm_drv.h"
@@ -1029,6 +1030,9 @@ static int exynos_dp_create_connector(struct exynos_drm_display *display,
 	drm_connector_register(connector);
 	drm_mode_connector_attach_encoder(connector, encoder);
 
+	if (dp->edp_panel)
+		drm_panel_attach(dp->edp_panel, &dp->connector);
+
 	return 0;
 }
 
@@ -1063,11 +1067,13 @@ static void exynos_dp_poweron(struct exynos_dp_device *dp)
 	if (dp->dpms_mode == DRM_MODE_DPMS_ON)
 		return;
 
+	drm_panel_prepare(dp->edp_panel);
 	clk_prepare_enable(dp->clock);
 	exynos_dp_phy_init(dp);
 	exynos_dp_init_dp(dp);
 	enable_irq(dp->irq);
 	exynos_dp_setup(dp);
+	drm_panel_enable(dp->edp_panel);
 }
 
 static void exynos_dp_poweroff(struct exynos_dp_device *dp)
@@ -1075,10 +1081,12 @@ static void exynos_dp_poweroff(struct exynos_dp_device *dp)
 	if (dp->dpms_mode != DRM_MODE_DPMS_ON)
 		return;
 
+	drm_panel_disable(dp->edp_panel);
 	disable_irq(dp->irq);
 	flush_work(&dp->hotplug_work);
 	exynos_dp_phy_exit(dp);
 	clk_disable_unprepare(dp->clock);
+	drm_panel_unprepare(dp->edp_panel);
 }
 
 static void exynos_dp_dpms(struct exynos_drm_display *display, int mode)
@@ -1209,8 +1217,17 @@ err:
 static int exynos_dp_dt_parse_panel(struct exynos_dp_device *dp)
 {
 	int ret;
+	struct device_node *videomode_parent;
+
+	/* Receive video timing info from panel node
+	 * if there is a panel node
+	 */
+	if (dp->panel_node)
+		videomode_parent = dp->panel_node;
+	else
+		videomode_parent = dp->dev->of_node;
 
-	ret = of_get_videomode(dp->dev->of_node, &dp->panel.vm,
+	ret = of_get_videomode(videomode_parent, &dp->panel.vm,
 			OF_USE_NATIVE_MODE);
 	if (ret) {
 		DRM_ERROR("failed: of_get_videomode() : %d\n", ret);
@@ -1224,16 +1241,10 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct drm_device *drm_dev = data;
 	struct resource *res;
-	struct exynos_dp_device *dp;
+	struct exynos_dp_device *dp = exynos_dp_display.ctx;
 	unsigned int irq_flags;
-
 	int ret = 0;
 
-	dp = devm_kzalloc(&pdev->dev, sizeof(struct exynos_dp_device),
-				GFP_KERNEL);
-	if (!dp)
-		return -ENOMEM;
-
 	dp->dev = &pdev->dev;
 	dp->dpms_mode = DRM_MODE_DPMS_OFF;
 
@@ -1307,7 +1318,6 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data)
 	disable_irq(dp->irq);
 
 	dp->drm_dev = drm_dev;
-	exynos_dp_display.ctx = dp;
 
 	platform_set_drvdata(pdev, &exynos_dp_display);
 
@@ -1334,6 +1344,8 @@ static const struct component_ops exynos_dp_ops = {
 
 static int exynos_dp_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
+	struct exynos_dp_device *dp;
 	int ret;
 
 	ret = exynos_drm_component_add(&pdev->dev, EXYNOS_DEVICE_TYPE_CONNECTOR,
@@ -1341,6 +1353,21 @@ static int exynos_dp_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	dp = devm_kzalloc(&pdev->dev, sizeof(struct exynos_dp_device),
+				GFP_KERNEL);
+	if (!dp)
+		return -ENOMEM;
+
+	dp->panel_node = of_parse_phandle(dev->of_node, "edp-panel", 0);
+	if (dp->panel_node) {
+		dp->edp_panel = of_drm_find_panel(dp->panel_node);
+		of_node_put(dp->panel_node);
+		if (!dp->edp_panel)
+			return -EPROBE_DEFER;
+	}
+
+	exynos_dp_display.ctx = dp;
+
 	ret = component_add(&pdev->dev, &exynos_dp_ops);
 	if (ret)
 		exynos_drm_component_del(&pdev->dev,
diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.h b/drivers/gpu/drm/exynos/exynos_dp_core.h
index 02cc4f9..3c29e4e 100644
--- a/drivers/gpu/drm/exynos/exynos_dp_core.h
+++ b/drivers/gpu/drm/exynos/exynos_dp_core.h
@@ -146,9 +146,11 @@ struct link_train {
 
 struct exynos_dp_device {
 	struct device		*dev;
+	struct device_node	*panel_node;
 	struct drm_device	*drm_dev;
 	struct drm_connector	connector;
 	struct drm_encoder	*encoder;
+	struct drm_panel	*edp_panel;
 	struct clk		*clock;
 	unsigned int		irq;
 	void __iomem		*reg_base;
-- 
1.7.9.5

[PATCH V5 04/12] drm/panel: Add driver for lvds/edp based panels

[Ajay Kumar]

This patch adds a simple driver to handle all the LCD and LED
powerup/down routines needed to support eDP/LVDS panels.

The LCD and LED units are usually powered up via regulators,
and almost on all boards, we will have a BACKLIGHT_EN pin to
enable/ disable the backlight.
Sometimes, we can have LCD_EN switches as well.

The routines in this driver can be used to control
panel power sequence on such boards.

Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>
---
 drivers/gpu/drm/panel/Kconfig      |   10 ++
 drivers/gpu/drm/panel/Makefile     |    1 +
 drivers/gpu/drm/panel/panel-lvds.c |  268 ++++++++++++++++++++++++++++++++++++
 3 files changed, 279 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-lvds.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 4ec874d..8fe7ee5 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -30,4 +30,14 @@ config DRM_PANEL_S6E8AA0
 	select DRM_MIPI_DSI
 	select VIDEOMODE_HELPERS
 
+config DRM_PANEL_EDP_LVDS
+	tristate "support for eDP/LVDS panels"
+	depends on OF && DRM_PANEL
+	select VIDEOMODE_HELPERS
+	help
+	  DRM panel driver for direct eDP panels or LVDS connected
+	  via DP bridges, that need at most a regulator for LCD unit,
+	  a regulator for LED unit and/or enable GPIOs for LCD or LED units.
+	  Delay values can also be specified to support powerup and
+	  powerdown process.
 endmenu
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 8b92921..eaafa01 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_EDP_LVDS) += panel-lvds.o
diff --git a/drivers/gpu/drm/panel/panel-lvds.c b/drivers/gpu/drm/panel/panel-lvds.c
new file mode 100644
index 0000000..ce20587
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-lvds.c
@@ -0,0 +1,268 @@
+/*
+ * panel driver for lvds and eDP panels
+ *
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd
+ *
+ * Ajay Kumar <ajaykumar.rs@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 <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+
+#include <video/of_videomode.h>
+#include <video/videomode.h>
+
+#include <drm/drmP.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_panel.h>
+
+struct panel_lvds {
+	struct drm_panel	base;
+	struct regulator	*backlight_fet;
+	struct regulator	*lcd_fet;
+	struct gpio_desc	*lcd_en_gpio;
+	struct gpio_desc	*led_en_gpio;
+	struct videomode	vm;
+	int			width_mm;
+	int			height_mm;
+	bool			backlight_fet_enabled;
+	bool			lcd_fet_enabled;
+	int			panel_prepare_delay;
+	int			panel_enable_delay;
+	int			panel_disable_delay;
+	int			panel_unprepare_delay;
+};
+
+static inline struct panel_lvds *to_panel(struct drm_panel *panel)
+{
+	return container_of(panel, struct panel_lvds, base);
+}
+
+static int panel_lvds_prepare(struct drm_panel *panel)
+{
+	struct panel_lvds *lvds_panel = to_panel(panel);
+
+	if (!lvds_panel->lcd_fet_enabled) {
+		if (regulator_enable(lvds_panel->lcd_fet)) {
+			DRM_ERROR("failed to enable LCD fet\n");
+			return -EAGAIN;
+		}
+		lvds_panel->lcd_fet_enabled = true;
+	}
+
+	if (lvds_panel->lcd_en_gpio)
+		gpiod_set_value(lvds_panel->lcd_en_gpio, 1);
+
+	msleep(lvds_panel->panel_prepare_delay);
+
+	return 0;
+}
+
+static int panel_lvds_enable(struct drm_panel *panel)
+{
+	struct panel_lvds *lvds_panel = to_panel(panel);
+
+	if (!lvds_panel->backlight_fet_enabled) {
+		if (regulator_enable(lvds_panel->backlight_fet)) {
+			DRM_ERROR("failed to enable LED fet\n");
+			return -EAGAIN;
+		}
+		lvds_panel->backlight_fet_enabled = true;
+	}
+
+	msleep(lvds_panel->panel_enable_delay);
+
+	if (lvds_panel->led_en_gpio)
+		gpiod_set_value(lvds_panel->led_en_gpio, 1);
+
+	return 0;
+}
+
+static int panel_lvds_disable(struct drm_panel *panel)
+{
+	struct panel_lvds *lvds_panel = to_panel(panel);
+
+	if (lvds_panel->led_en_gpio)
+		gpiod_set_value(lvds_panel->led_en_gpio, 0);
+
+	if (lvds_panel->backlight_fet_enabled) {
+		regulator_disable(lvds_panel->backlight_fet);
+		lvds_panel->backlight_fet_enabled = false;
+	}
+
+	msleep(lvds_panel->panel_disable_delay);
+
+	return 0;
+}
+
+static int panel_lvds_unprepare(struct drm_panel *panel)
+{
+	struct panel_lvds *lvds_panel = to_panel(panel);
+
+	if (lvds_panel->lcd_en_gpio)
+		gpiod_set_value(lvds_panel->lcd_en_gpio, 0);
+
+	if (lvds_panel->lcd_fet_enabled) {
+		regulator_disable(lvds_panel->lcd_fet);
+		lvds_panel->lcd_fet_enabled = false;
+	}
+
+	msleep(lvds_panel->panel_unprepare_delay);
+
+	return 0;
+}
+
+static int panel_lvds_get_modes(struct drm_panel *panel)
+{
+	struct drm_connector *connector = panel->connector;
+	struct panel_lvds *lvds_panel = to_panel(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(&lvds_panel->vm, mode);
+	mode->width_mm = lvds_panel->width_mm;
+	mode->height_mm = lvds_panel->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_set_name(mode);
+	drm_mode_probed_add(connector, mode);
+
+	return 1;
+}
+
+static const struct drm_panel_funcs panel_lvds_funcs = {
+	.unprepare = panel_lvds_unprepare,
+	.disable = panel_lvds_disable,
+	.prepare = panel_lvds_prepare,
+	.enable = panel_lvds_enable,
+	.get_modes = panel_lvds_get_modes,
+};
+
+static int panel_lvds_probe(struct platform_device *pdev)
+{
+	struct panel_lvds *panel;
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	int ret;
+
+	panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
+	if (!panel)
+		return -ENOMEM;
+
+	ret = of_get_videomode(node, &panel->vm, 0);
+	if (ret) {
+		DRM_ERROR("failed: of_get_videomode() : %d\n", ret);
+		return ret;
+	}
+
+	panel->lcd_fet = devm_regulator_get(dev, "lcd");
+	if (IS_ERR(panel->lcd_fet))
+		return -EPROBE_DEFER;
+
+	panel->backlight_fet = devm_regulator_get(dev, "backlight");
+	if (IS_ERR(panel->backlight_fet))
+		return -EPROBE_DEFER;
+
+	of_property_read_u32(node, "panel-width-mm", &panel->width_mm);
+	of_property_read_u32(node, "panel-height-mm", &panel->height_mm);
+
+	of_property_read_u32(node, "panel-prepare-delay",
+					&panel->panel_prepare_delay);
+	of_property_read_u32(node, "panel-enable-delay",
+					&panel->panel_enable_delay);
+	of_property_read_u32(node, "panel-disable-delay",
+					&panel->panel_disable_delay);
+	of_property_read_u32(node, "panel-unprepare-delay",
+					&panel->panel_unprepare_delay);
+
+	panel->lcd_en_gpio = devm_gpiod_get(dev, "lcd-enable");
+	if (IS_ERR(panel->lcd_en_gpio)) {
+		ret = PTR_ERR(panel->lcd_en_gpio);
+		if (ret != -ENOENT) {
+			dev_err(dev, "failed to request GPIO: %d\n", ret);
+			return ret;
+		}
+		panel->lcd_en_gpio = NULL;
+	} else {
+		ret = gpiod_direction_output(panel->lcd_en_gpio, 0);
+		if (ret < 0) {
+			dev_err(dev, "failed to setup GPIO: %d\n", ret);
+			return ret;
+		}
+	}
+
+	panel->led_en_gpio = devm_gpiod_get(dev, "backlight-enable");
+	if (IS_ERR(panel->led_en_gpio)) {
+		ret = PTR_ERR(panel->led_en_gpio);
+		if (ret != -ENOENT) {
+			dev_err(dev, "failed to request GPIO: %d\n", ret);
+			return ret;
+		}
+		panel->led_en_gpio = NULL;
+	} else {
+		ret = gpiod_direction_output(panel->led_en_gpio, 0);
+		if (ret < 0) {
+			dev_err(dev, "failed to setup GPIO: %d\n", ret);
+			return ret;
+		}
+	}
+
+	drm_panel_init(&panel->base);
+	panel->base.dev = dev;
+	panel->base.funcs = &panel_lvds_funcs;
+
+	ret = drm_panel_add(&panel->base);
+	if (ret < 0)
+		return ret;
+
+	dev_set_drvdata(dev, panel);
+
+	return 0;
+}
+
+static int panel_lvds_remove(struct platform_device *pdev)
+{
+	struct panel_lvds *panel = dev_get_drvdata(&pdev->dev);
+
+	panel_lvds_disable(&panel->base);
+	panel_lvds_unprepare(&panel->base);
+
+	drm_panel_remove(&panel->base);
+
+	return 0;
+}
+
+static const struct of_device_id lvds_panel_dt_match[] = {
+	{ .compatible = "panel-lvds" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, lvds_panel_dt_match);
+
+struct platform_driver lvds_panel_driver = {
+	.driver = {
+		.name = "panel_lvds",
+		.owner = THIS_MODULE,
+		.of_match_table = lvds_panel_dt_match,
+	},
+	.probe = panel_lvds_probe,
+	.remove = panel_lvds_remove,
+};
+module_platform_driver(lvds_panel_driver);
+
+MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
+MODULE_DESCRIPTION("lvds/eDP panel driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

[PATCH V5 05/12] Documentation: Add DT bindings for panel-lvds driver

[Ajay Kumar]

Add DT binding documentation for panel-lvds driver.

Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 .../devicetree/bindings/panel/panel-lvds.txt       |   50 ++++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/panel/panel-lvds.txt

diff --git a/Documentation/devicetree/bindings/panel/panel-lvds.txt b/Documentation/devicetree/bindings/panel/panel-lvds.txt
new file mode 100644
index 0000000..fdf91da2
--- /dev/null
+++ b/Documentation/devicetree/bindings/panel/panel-lvds.txt
@@ -0,0 +1,50 @@
+panel interface for eDP/lvds panels
+
+Required properties:
+  - compatible: "panel-lvds"
+
+Optional properties:
+	-lcd-enable-gpios:
+		panel LCD poweron GPIO.
+			Indicates which GPIO needs to be powered up as output
+			to powerup/enable the switch to the LCD panel.
+	-backlight-enable-gpios:
+		panel LED enable GPIO.
+			Indicates which GPIO needs to be powered up as output
+			to enable the backlight.
+	-panel-prepare-delay:
+		delay value in ms required for panel_prepare process
+			Delay in ms needed for the panel LCD unit to
+			powerup completely.
+			ex: delay needed till eDP panel throws HPD.
+			    delay needed so that we cans tart reading edid.
+	-panel-enable-delay:
+		delay value in ms required for panel_enable process
+			Delay in ms needed for the panel backlight/LED unit
+			to powerup, and delay needed between video_enable and
+			backlight_enable.
+	-panel-disable-delay:
+		delay value in ms required for panel_disable process
+			Delay in ms needed for the panel backlight/LED unit
+			powerdown, and delay needed between backlight_disable
+			and video_disable.
+	-panel-unprepare-delay:
+		delay value in ms required for panel_post_disable process
+			Delay in ms needed for the panel LCD unit to
+			to powerdown completely, and the minimum delay needed
+			before powering it on again.
+	-panel-width-mm: physical panel width [mm]
+	-panel-height-mm: physical panel height [mm]
+
+Example:
+
+	panel-lvds {
+		compatible = "panel-lvds";
+		backlight-enable-gpios = <&gpx3 0 0>;
+		panel-prepare-delay = <40>;
+		panel-enable-delay = <20>;
+		panel-disable-delay = <20>;
+		panel-unprepare-delay = <30>;
+		panel-width-mm = <256>;
+		panel-height-mm = <144>;
+	};
-- 
1.7.9.5

[RESEND PATCH V5 06/12] drm/bridge: add helper functions to support bridge chain

[Ajay Kumar]

Add helper functions to create bridge chain and to call the
corresponding next_bridge functions.

Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
Suggested-by: Rob Clark <robdclark@gmail.com>
---
 include/drm/drm_crtc.h |   72 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index e529b68..0a6950a 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -652,6 +652,7 @@ struct drm_bridge_funcs {
 struct drm_bridge {
 	struct drm_device *dev;
 	struct list_head head;
+	struct drm_bridge *next_bridge;
 
 	struct drm_mode_object base;
 
@@ -1166,6 +1167,77 @@ drm_property_blob_find(struct drm_device *dev, uint32_t id)
 	return mo ? obj_to_blob(mo) : NULL;
 }
 
+static inline int drm_bridge_add_to_chain(struct drm_bridge *head,
+					  struct drm_bridge *last)
+{
+	struct drm_bridge *temp = head;
+
+	if (head && last) {
+		while (temp->next_bridge)
+			temp = temp->next_bridge;
+
+		temp->next_bridge = last;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static inline void drm_next_bridge_mode_fixup(struct drm_bridge *bridge,
+					const struct drm_display_mode *mode,
+					struct drm_display_mode *adjusted_mode)
+{
+	if (bridge && bridge->next_bridge && bridge->next_bridge->funcs &&
+	    bridge->next_bridge->funcs->mode_fixup)
+		bridge->next_bridge->funcs->mode_fixup(bridge->next_bridge,
+							mode, adjusted_mode);
+}
+
+static inline void drm_next_bridge_disable(struct drm_bridge *bridge)
+{
+	if (bridge && bridge->next_bridge && bridge->next_bridge->funcs &&
+	    bridge->next_bridge->funcs->disable)
+		bridge->next_bridge->funcs->disable(bridge->next_bridge);
+}
+
+static inline void drm_next_bridge_post_disable(struct drm_bridge *bridge)
+{
+	if (bridge && bridge->next_bridge && bridge->next_bridge->funcs &&
+	    bridge->next_bridge->funcs->post_disable)
+		bridge->next_bridge->funcs->post_disable(bridge->next_bridge);
+}
+
+static inline void drm_next_bridge_mode_set(struct drm_bridge *bridge,
+					struct drm_display_mode *mode,
+					struct drm_display_mode *adjusted_mode)
+{
+	if (bridge && bridge->next_bridge && bridge->next_bridge->funcs &&
+	    bridge->next_bridge->funcs->mode_set)
+		bridge->next_bridge->funcs->mode_set(bridge->next_bridge,
+							mode, adjusted_mode);
+}
+
+static inline void drm_next_bridge_pre_enable(struct drm_bridge *bridge)
+{
+	if (bridge && bridge->next_bridge && bridge->next_bridge->funcs &&
+	    bridge->next_bridge->funcs->pre_enable)
+		bridge->next_bridge->funcs->pre_enable(bridge->next_bridge);
+}
+
+static inline void drm_next_bridge_enable(struct drm_bridge *bridge)
+{
+	if (bridge && bridge->next_bridge && bridge->next_bridge->funcs &&
+	    bridge->next_bridge->funcs->enable)
+		bridge->next_bridge->funcs->enable(bridge->next_bridge);
+}
+
+static inline void drm_next_bridge_destroy(struct drm_bridge *bridge)
+{
+	if (bridge && bridge->next_bridge && bridge->next_bridge->funcs &&
+	    bridge->next_bridge->funcs->destroy)
+		bridge->next_bridge->funcs->destroy(bridge->next_bridge);
+}
+
 /* Plane list iterator for legacy (overlay only) planes. */
 #define drm_for_each_legacy_plane(plane, planelist) \
 	list_for_each_entry(plane, planelist, head) \
-- 
1.7.9.5

[PATCH V5 07/12] drm/bridge: Add a driver which binds drm_bridge with drm_panel

[Ajay Kumar]

Add a dummy bridge which binds all of the drm_bridge callbacks
to corresponding drm_panel callbacks.

In theory, this is just a glue layer for the last bridge and
the panel attached to it.

This driver also implements the required drm_connector ops
for the encoder(on which the entire bridge chain is hanging off).

Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/gpu/drm/bridge/Kconfig        |    7 ++
 drivers/gpu/drm/bridge/Makefile       |    1 +
 drivers/gpu/drm/bridge/panel_binder.c |  193 +++++++++++++++++++++++++++++++++
 include/drm/bridge/panel_binder.h     |   44 ++++++++
 4 files changed, 245 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/panel_binder.c
 create mode 100644 include/drm/bridge/panel_binder.h

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 884923f..e3fb487 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -3,3 +3,10 @@ config DRM_PTN3460
 	depends on DRM
 	select DRM_KMS_HELPER
 	---help---
+
+config DRM_PANEL_BINDER
+	tristate "bridge panel binder"
+	depends on DRM
+	select DRM_KMS_HELPER
+	select DRM_PANEL
+	---help---
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index b4733e1..ba8b5b8 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -1,3 +1,4 @@
 ccflags-y := -Iinclude/drm
 
 obj-$(CONFIG_DRM_PTN3460) += ptn3460.o
+obj-$(CONFIG_DRM_PANEL_BINDER) += panel_binder.o
diff --git a/drivers/gpu/drm/bridge/panel_binder.c b/drivers/gpu/drm/bridge/panel_binder.c
new file mode 100644
index 0000000..6160ed5
--- /dev/null
+++ b/drivers/gpu/drm/bridge/panel_binder.c
@@ -0,0 +1,193 @@
+/*
+ * Copyright (C) 2014 Samsung Electronics Co., Ltd.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+
+#include <drm/drm_panel.h>
+
+#include "drmP.h"
+#include "drm_crtc.h"
+#include "drm_crtc_helper.h"
+
+#include "bridge/panel_binder.h"
+
+struct panel_binder {
+	struct drm_connector	connector;
+	struct i2c_client	*client;
+	struct drm_encoder	*encoder;
+	struct drm_bridge	*bridge;
+	struct drm_panel	*panel;
+};
+
+static void panel_binder_pre_enable(struct drm_bridge *bridge)
+{
+	struct panel_binder *panel_binder = bridge->driver_private;
+
+	drm_panel_prepare(panel_binder->panel);
+}
+
+static void panel_binder_enable(struct drm_bridge *bridge)
+{
+	struct panel_binder *panel_binder = bridge->driver_private;
+
+	drm_panel_enable(panel_binder->panel);
+}
+
+static void panel_binder_disable(struct drm_bridge *bridge)
+{
+	struct panel_binder *panel_binder = bridge->driver_private;
+
+	drm_panel_disable(panel_binder->panel);
+}
+
+static void panel_binder_post_disable(struct drm_bridge *bridge)
+{
+	struct panel_binder *panel_binder = bridge->driver_private;
+
+	drm_panel_unprepare(panel_binder->panel);
+}
+
+void panel_binder_destroy(struct drm_bridge *bridge)
+{
+	struct panel_binder *panel_binder = bridge->driver_private;
+
+	drm_panel_detach(panel_binder->panel);
+	drm_bridge_cleanup(bridge);
+}
+
+struct drm_bridge_funcs panel_binder_funcs = {
+	.pre_enable = panel_binder_pre_enable,
+	.enable = panel_binder_enable,
+	.disable = panel_binder_disable,
+	.post_disable = panel_binder_post_disable,
+	.destroy = panel_binder_destroy,
+};
+
+static int panel_binder_mode_valid(struct drm_connector *connector,
+				 struct drm_display_mode *mode)
+{
+	return MODE_OK;
+}
+
+static int panel_binder_get_modes(struct drm_connector *connector)
+{
+	struct panel_binder *panel_binder;
+
+	panel_binder = container_of(connector, struct panel_binder, connector);
+
+	return panel_binder->panel->funcs->get_modes(panel_binder->panel);
+}
+
+static struct drm_encoder *panel_binder_best_encoder(struct drm_connector
+								*connector)
+{
+	struct panel_binder *panel_binder;
+
+	panel_binder = container_of(connector, struct panel_binder, connector);
+
+	return panel_binder->encoder;
+}
+
+static const struct drm_connector_helper_funcs
+					panel_binder_connector_helper_funcs = {
+	.get_modes = panel_binder_get_modes,
+	.mode_valid = panel_binder_mode_valid,
+	.best_encoder = panel_binder_best_encoder,
+};
+
+static enum drm_connector_status panel_binder_detect(struct drm_connector
+							*connector, bool force)
+{
+	return connector_status_connected;
+}
+
+static void panel_binder_connector_destroy(struct drm_connector *connector)
+{
+	drm_connector_cleanup(connector);
+}
+
+static const struct drm_connector_funcs panel_binder_connector_funcs = {
+	.dpms = drm_helper_connector_dpms,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.detect = panel_binder_detect,
+	.destroy = panel_binder_connector_destroy,
+};
+
+struct drm_bridge *panel_binder_init(struct drm_device *dev,
+					struct drm_encoder *encoder,
+					struct i2c_client *client,
+					struct device_node *node,
+					struct drm_panel *panel,
+					int connector_type,
+					uint8_t polled)
+{
+	int ret;
+	struct drm_bridge *bridge;
+	struct panel_binder *panel_binder;
+
+	if (IS_ERR_OR_NULL(panel)) {
+		DRM_ERROR("invalid drm_panel pointer\n");
+		return NULL;
+	}
+
+	bridge = devm_kzalloc(dev->dev, sizeof(*bridge), GFP_KERNEL);
+	if (!bridge) {
+		DRM_ERROR("failed to allocate drm bridge\n");
+		return NULL;
+	}
+
+	panel_binder = devm_kzalloc(dev->dev, sizeof(*panel_binder),
+								GFP_KERNEL);
+	if (!panel_binder) {
+		DRM_ERROR("failed to allocate bridge panel_binder\n");
+		return NULL;
+	}
+
+	panel_binder->client = client;
+	panel_binder->encoder = encoder;
+	panel_binder->bridge = bridge;
+	panel_binder->panel = panel;
+
+	ret = drm_bridge_init(dev, bridge, &panel_binder_funcs);
+	if (ret) {
+		DRM_ERROR("failed to initialize bridge with drm\n");
+		goto err;
+	}
+
+	bridge->driver_private = panel_binder;
+
+	drm_panel_attach(panel_binder->panel, &panel_binder->connector);
+
+	if (!encoder->bridge)
+		/* First entry in the bridge chain */
+		encoder->bridge = bridge;
+
+	panel_binder->connector.polled = polled;
+	ret = drm_connector_init(dev, &panel_binder->connector,
+			&panel_binder_connector_funcs, connector_type);
+	if (ret) {
+		DRM_ERROR("failed to initialize connector with drm\n");
+		goto err;
+	}
+	drm_connector_helper_add(&panel_binder->connector,
+			&panel_binder_connector_helper_funcs);
+	drm_connector_register(&panel_binder->connector);
+	drm_mode_connector_attach_encoder(&panel_binder->connector, encoder);
+
+	return bridge;
+
+err:
+	return NULL;
+}
+EXPORT_SYMBOL(panel_binder_init);
diff --git a/include/drm/bridge/panel_binder.h b/include/drm/bridge/panel_binder.h
new file mode 100644
index 0000000..5bed382
--- /dev/null
+++ b/include/drm/bridge/panel_binder.h
@@ -0,0 +1,44 @@
+/*
+ * Copyright (C) 2014 Samsung Electronics Co., Ltd.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _DRM_BRIDGE_PANEL_H_
+#define _DRM_BRIDGE_PANEL_H_
+
+struct drm_device;
+struct drm_encoder;
+struct i2c_client;
+struct device_node;
+struct drm_panel;
+
+#if defined(CONFIG_DRM_PANEL_BINDER) || defined(CONFIG_DRM_PANEL_BINDER_MODULE)
+struct drm_bridge *panel_binder_init(struct drm_device *dev,
+					struct drm_encoder *encoder,
+					struct i2c_client *client,
+					struct device_node *node,
+					struct drm_panel *panel,
+					int connector_type,
+					uint8_t polled);
+#else
+static inline struct drm_bridge *panel_binder_init(struct drm_device *dev,
+						struct drm_encoder *encoder,
+						struct i2c_client *client,
+						struct device_node *node,
+						struct drm_panel *panel,
+						int connector_type,
+						uint8_t polled)
+{
+	return 0;
+}
+#endif
+
+#endif
-- 
1.7.9.5

[RESEND PATCH V5 08/12] drm/bridge: ptn3460: Support bridge chaining

[Ajay Kumar]

Modify the driver to invoke callbacks for the next bridge
in the bridge chain.
Also, remove the drm_connector implementation from ptn3460,
since the same is implemented using panel_binder.

Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/gpu/drm/bridge/ptn3460.c        |  137 +++++--------------------------
 drivers/gpu/drm/exynos/exynos_dp_core.c |   16 ++--
 include/drm/bridge/ptn3460.h            |   15 ++--
 3 files changed, 39 insertions(+), 129 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c
index d466696..5fe16c6 100644
--- a/drivers/gpu/drm/bridge/ptn3460.c
+++ b/drivers/gpu/drm/bridge/ptn3460.c
@@ -34,37 +34,15 @@
 #define PTN3460_EDID_SRAM_LOAD_ADDR		0x85
 
 struct ptn3460_bridge {
-	struct drm_connector connector;
 	struct i2c_client *client;
 	struct drm_encoder *encoder;
 	struct drm_bridge *bridge;
-	struct edid *edid;
 	int gpio_pd_n;
 	int gpio_rst_n;
 	u32 edid_emulation;
 	bool enabled;
 };
 
-static int ptn3460_read_bytes(struct ptn3460_bridge *ptn_bridge, char addr,
-		u8 *buf, int len)
-{
-	int ret;
-
-	ret = i2c_master_send(ptn_bridge->client, &addr, 1);
-	if (ret <= 0) {
-		DRM_ERROR("Failed to send i2c command, ret=%d\n", ret);
-		return ret;
-	}
-
-	ret = i2c_master_recv(ptn_bridge->client, buf, len);
-	if (ret <= 0) {
-		DRM_ERROR("Failed to recv i2c data, ret=%d\n", ret);
-		return ret;
-	}
-
-	return 0;
-}
-
 static int ptn3460_write_byte(struct ptn3460_bridge *ptn_bridge, char addr,
 		char val)
 {
@@ -126,6 +104,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
 		gpio_set_value(ptn_bridge->gpio_rst_n, 1);
 	}
 
+	drm_next_bridge_pre_enable(bridge);
+
 	/*
 	 * There's a bug in the PTN chip where it falsely asserts hotplug before
 	 * it is fully functional. We're forced to wait for the maximum start up
@@ -142,6 +122,7 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
 
 static void ptn3460_enable(struct drm_bridge *bridge)
 {
+	drm_next_bridge_enable(bridge);
 }
 
 static void ptn3460_disable(struct drm_bridge *bridge)
@@ -153,6 +134,8 @@ static void ptn3460_disable(struct drm_bridge *bridge)
 
 	ptn_bridge->enabled = false;
 
+	drm_next_bridge_disable(bridge);
+
 	if (gpio_is_valid(ptn_bridge->gpio_rst_n))
 		gpio_set_value(ptn_bridge->gpio_rst_n, 1);
 
@@ -162,6 +145,7 @@ static void ptn3460_disable(struct drm_bridge *bridge)
 
 static void ptn3460_post_disable(struct drm_bridge *bridge)
 {
+	drm_next_bridge_post_disable(bridge);
 }
 
 void ptn3460_bridge_destroy(struct drm_bridge *bridge)
@@ -173,6 +157,9 @@ void ptn3460_bridge_destroy(struct drm_bridge *bridge)
 		gpio_free(ptn_bridge->gpio_pd_n);
 	if (gpio_is_valid(ptn_bridge->gpio_rst_n))
 		gpio_free(ptn_bridge->gpio_rst_n);
+
+	drm_next_bridge_destroy(bridge);
+
 	/* Nothing else to free, we've got devm allocated memory */
 }
 
@@ -184,81 +171,10 @@ struct drm_bridge_funcs ptn3460_bridge_funcs = {
 	.destroy = ptn3460_bridge_destroy,
 };
 
-int ptn3460_get_modes(struct drm_connector *connector)
-{
-	struct ptn3460_bridge *ptn_bridge;
-	u8 *edid;
-	int ret, num_modes;
-	bool power_off;
-
-	ptn_bridge = container_of(connector, struct ptn3460_bridge, connector);
-
-	if (ptn_bridge->edid)
-		return drm_add_edid_modes(connector, ptn_bridge->edid);
-
-	power_off = !ptn_bridge->enabled;
-	ptn3460_pre_enable(ptn_bridge->bridge);
-
-	edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
-	if (!edid) {
-		DRM_ERROR("Failed to allocate edid\n");
-		return 0;
-	}
-
-	ret = ptn3460_read_bytes(ptn_bridge, PTN3460_EDID_ADDR, edid,
-			EDID_LENGTH);
-	if (ret) {
-		kfree(edid);
-		num_modes = 0;
-		goto out;
-	}
-
-	ptn_bridge->edid = (struct edid *)edid;
-	drm_mode_connector_update_edid_property(connector, ptn_bridge->edid);
-
-	num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
-
-out:
-	if (power_off)
-		ptn3460_disable(ptn_bridge->bridge);
-
-	return num_modes;
-}
-
-struct drm_encoder *ptn3460_best_encoder(struct drm_connector *connector)
-{
-	struct ptn3460_bridge *ptn_bridge;
-
-	ptn_bridge = container_of(connector, struct ptn3460_bridge, connector);
-
-	return ptn_bridge->encoder;
-}
-
-struct drm_connector_helper_funcs ptn3460_connector_helper_funcs = {
-	.get_modes = ptn3460_get_modes,
-	.best_encoder = ptn3460_best_encoder,
-};
-
-enum drm_connector_status ptn3460_detect(struct drm_connector *connector,
-		bool force)
-{
-	return connector_status_connected;
-}
-
-void ptn3460_connector_destroy(struct drm_connector *connector)
-{
-	drm_connector_cleanup(connector);
-}
-
-struct drm_connector_funcs ptn3460_connector_funcs = {
-	.dpms = drm_helper_connector_dpms,
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.detect = ptn3460_detect,
-	.destroy = ptn3460_connector_destroy,
-};
-
-int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
-		struct i2c_client *client, struct device_node *node)
+struct drm_bridge *ptn3460_init(struct drm_device *dev,
+				struct drm_encoder *encoder,
+				struct i2c_client *client,
+				struct device_node *node)
 {
 	int ret;
 	struct drm_bridge *bridge;
@@ -267,13 +183,13 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
 	bridge = devm_kzalloc(dev->dev, sizeof(*bridge), GFP_KERNEL);
 	if (!bridge) {
 		DRM_ERROR("Failed to allocate drm bridge\n");
-		return -ENOMEM;
+		return NULL;
 	}
 
 	ptn_bridge = devm_kzalloc(dev->dev, sizeof(*ptn_bridge), GFP_KERNEL);
 	if (!ptn_bridge) {
 		DRM_ERROR("Failed to allocate ptn bridge\n");
-		return -ENOMEM;
+		return NULL;
 	}
 
 	ptn_bridge->client = client;
@@ -285,7 +201,7 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
 				GPIOF_OUT_INIT_HIGH, "PTN3460_PD_N");
 		if (ret) {
 			DRM_ERROR("Request powerdown-gpio failed (%d)\n", ret);
-			return ret;
+			return NULL;
 		}
 	}
 
@@ -300,7 +216,7 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
 		if (ret) {
 			DRM_ERROR("Request reset-gpio failed (%d)\n", ret);
 			gpio_free(ptn_bridge->gpio_pd_n);
-			return ret;
+			return NULL;
 		}
 	}
 
@@ -318,26 +234,17 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
 	}
 
 	bridge->driver_private = ptn_bridge;
-	encoder->bridge = bridge;
+	if (!encoder->bridge)
+		/* First entry in the bridge chain */
+		encoder->bridge = bridge;
 
-	ret = drm_connector_init(dev, &ptn_bridge->connector,
-			&ptn3460_connector_funcs, DRM_MODE_CONNECTOR_LVDS);
-	if (ret) {
-		DRM_ERROR("Failed to initialize connector with drm\n");
-		goto err;
-	}
-	drm_connector_helper_add(&ptn_bridge->connector,
-			&ptn3460_connector_helper_funcs);
-	drm_connector_register(&ptn_bridge->connector);
-	drm_mode_connector_attach_encoder(&ptn_bridge->connector, encoder);
-
-	return 0;
+	return bridge;
 
 err:
 	if (gpio_is_valid(ptn_bridge->gpio_pd_n))
 		gpio_free(ptn_bridge->gpio_pd_n);
 	if (gpio_is_valid(ptn_bridge->gpio_rst_n))
 		gpio_free(ptn_bridge->gpio_rst_n);
-	return ret;
+	return NULL;
 }
 EXPORT_SYMBOL(ptn3460_init);
diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
index b3d0d9b..9d31296 100644
--- a/drivers/gpu/drm/exynos/exynos_dp_core.c
+++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
@@ -988,19 +988,19 @@ static bool find_bridge(const char *compat, struct bridge_init *bridge)
 	return true;
 }
 
-/* returns the number of bridges attached */
-static int exynos_drm_attach_lcd_bridge(struct drm_device *dev,
+static int exynos_drm_attach_lcd_bridge(struct exynos_dp_device *dp,
 		struct drm_encoder *encoder)
 {
 	struct bridge_init bridge;
-	int ret;
+	struct drm_bridge *bridge_chain = NULL;
+	bool connector_created = false;
 
 	if (find_bridge("nxp,ptn3460", &bridge)) {
-		ret = ptn3460_init(dev, encoder, bridge.client, bridge.node);
-		if (!ret)
-			return 1;
+		bridge_chain = ptn3460_init(dp->drm_dev, encoder, bridge.client,
+								bridge.node);
 	}
-	return 0;
+
+	return connector_created;
 }
 
 static int exynos_dp_create_connector(struct exynos_drm_display *display,
@@ -1013,7 +1013,7 @@ static int exynos_dp_create_connector(struct exynos_drm_display *display,
 	dp->encoder = encoder;
 
 	/* Pre-empt DP connector creation if there's a bridge */
-	ret = exynos_drm_attach_lcd_bridge(dp->drm_dev, encoder);
+	ret = exynos_drm_attach_lcd_bridge(dp, encoder);
 	if (ret)
 		return 0;
 
diff --git a/include/drm/bridge/ptn3460.h b/include/drm/bridge/ptn3460.h
index ff62344..f612b9b 100644
--- a/include/drm/bridge/ptn3460.h
+++ b/include/drm/bridge/ptn3460.h
@@ -21,15 +21,18 @@ struct device_node;
 
 #if defined(CONFIG_DRM_PTN3460) || defined(CONFIG_DRM_PTN3460_MODULE)
 
-int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
-		struct i2c_client *client, struct device_node *node);
+struct drm_bridge *ptn3460_init(struct drm_device *dev,
+				struct drm_encoder *encoder,
+				struct i2c_client *client,
+				struct device_node *node);
 #else
 
-static inline int ptn3460_init(struct drm_device *dev,
-		struct drm_encoder *encoder, struct i2c_client *client,
-		struct device_node *node)
+static inline struct drm_bridge *ptn3460_init(struct drm_device *dev,
+						struct drm_encoder *encoder,
+						struct i2c_client *client,
+						struct device_node *node)
 {
-	return 0;
+	return NULL;
 }
 
 #endif
-- 
1.7.9.5

[RESEND PATCH V5 09/12] drm/exynos: dp: create bridge chain using ptn3460 and panel_binder

[Ajay Kumar]

exynos_dp supports a simple bridge chain with ptn3460 bridge
and an LVDS panel attached to it.
This patch creates the bridge chain with ptn3460 as the head
of the list and panel_binder being the tail.

Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_dp_core.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
index 9d31296..0ca6256 100644
--- a/drivers/gpu/drm/exynos/exynos_dp_core.c
+++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
@@ -30,6 +30,7 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_panel.h>
 #include <drm/bridge/ptn3460.h>
+#include <drm/bridge/panel_binder.h>
 
 #include "exynos_drm_drv.h"
 #include "exynos_dp_core.h"
@@ -992,7 +993,7 @@ static int exynos_drm_attach_lcd_bridge(struct exynos_dp_device *dp,
 		struct drm_encoder *encoder)
 {
 	struct bridge_init bridge;
-	struct drm_bridge *bridge_chain = NULL;
+	struct drm_bridge *bridge_chain = NULL, *next = NULL;
 	bool connector_created = false;
 
 	if (find_bridge("nxp,ptn3460", &bridge)) {
@@ -1000,6 +1001,15 @@ static int exynos_drm_attach_lcd_bridge(struct exynos_dp_device *dp,
 								bridge.node);
 	}
 
+	if (bridge_chain && dp->edp_panel) {
+		next = panel_binder_init(dp->drm_dev, encoder, bridge.client,
+			bridge.node, dp->edp_panel, DRM_MODE_CONNECTOR_LVDS,
+			DRM_CONNECTOR_POLL_HPD);
+		if (next)
+			connector_created = true;
+		drm_bridge_add_to_chain(bridge_chain, next);
+	}
+
 	return connector_created;
 }
 
-- 
1.7.9.5

[PATCH V5 10/12] drm/bridge: Add ps8622/ps8625 bridge driver

[Ajay Kumar]

From: Vincent Palatin <vpalatin@chromium.org>

This patch adds drm_bridge driver for parade DisplayPort
to LVDS bridge chip.

Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/gpu/drm/bridge/Kconfig  |    8 +
 drivers/gpu/drm/bridge/Makefile |    1 +
 drivers/gpu/drm/bridge/ps8622.c |  476 +++++++++++++++++++++++++++++++++++++++
 include/drm/bridge/ps8622.h     |   41 ++++
 4 files changed, 526 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/ps8622.c
 create mode 100644 include/drm/bridge/ps8622.h

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index e3fb487..7b843c8 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -10,3 +10,11 @@ config DRM_PANEL_BINDER
 	select DRM_KMS_HELPER
 	select DRM_PANEL
 	---help---
+
+config DRM_PS8622
+	tristate "Parade eDP/LVDS bridge"
+	depends on DRM
+	select DRM_KMS_HELPER
+	select BACKLIGHT_LCD_SUPPORT
+	select BACKLIGHT_CLASS_DEVICE
+	---help---
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index ba8b5b8..b494d4b 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -2,3 +2,4 @@ ccflags-y := -Iinclude/drm
 
 obj-$(CONFIG_DRM_PTN3460) += ptn3460.o
 obj-$(CONFIG_DRM_PANEL_BINDER) += panel_binder.o
+obj-$(CONFIG_DRM_PS8622) += ps8622.o
diff --git a/drivers/gpu/drm/bridge/ps8622.c b/drivers/gpu/drm/bridge/ps8622.c
new file mode 100644
index 0000000..b5b7c31
--- /dev/null
+++ b/drivers/gpu/drm/bridge/ps8622.c
@@ -0,0 +1,476 @@
+/*
+ * Parade PS8622 eDP/LVDS bridge driver
+ *
+ * Copyright (C) 2014 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/fb.h>
+#include <linux/gpio.h>
+#include <linux/i2c.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/pm.h>
+#include <linux/regulator/consumer.h>
+
+#include "drmP.h"
+#include "drm_crtc.h"
+#include "drm_crtc_helper.h"
+
+struct ps8622_bridge {
+	struct drm_bridge *bridge;
+	struct drm_encoder *encoder;
+	struct i2c_client *client;
+	struct regulator *v12;
+	struct backlight_device *bl;
+	struct mutex enable_mutex;
+
+	struct gpio_desc *gpio_slp_n;
+	struct gpio_desc *gpio_rst_n;
+
+	u8 max_lane_count;
+	u8 lane_count;
+
+	bool enabled;
+};
+
+struct ps8622_device_data {
+	u8 max_lane_count;
+};
+
+static const struct ps8622_device_data ps8622_data = {
+	.max_lane_count = 1,
+};
+
+static const struct ps8622_device_data ps8625_data = {
+	.max_lane_count = 2,
+};
+
+/* Brightness scale on the Parade chip */
+#define PS8622_MAX_BRIGHTNESS 0xff
+
+/* Timings taken from the version 1.7 datasheet for the PS8622/PS8625 */
+#define PS8622_POWER_RISE_T1_MIN_US 10
+#define PS8622_POWER_RISE_T1_MAX_US 10000
+#define PS8622_RST_HIGH_T2_MIN_US 3000
+#define PS8622_RST_HIGH_T2_MAX_US 30000
+#define PS8622_PWMO_END_T12_MS 200
+#define PS8622_POWER_FALL_T16_MAX_US 10000
+#define PS8622_POWER_OFF_T17_MS 500
+
+#if ((PS8622_RST_HIGH_T2_MIN_US + PS8622_POWER_RISE_T1_MAX_US) > \
+	(PS8622_RST_HIGH_T2_MAX_US + PS8622_POWER_RISE_T1_MIN_US))
+#error "T2.min + T1.max must be less than T2.max + T1.min"
+#endif
+
+static int ps8622_set(struct i2c_client *client, u8 page, u8 reg, u8 val)
+{
+	int ret;
+	struct i2c_adapter *adap = client->adapter;
+	struct i2c_msg msg;
+	u8 data[] = {reg, val};
+
+	msg.addr = client->addr + page;
+	msg.flags = 0;
+	msg.len = sizeof(data);
+	msg.buf = data;
+
+	ret = i2c_transfer(adap, &msg, 1);
+	if (ret != 1)
+		pr_warn("PS8622 I2C write (0x%02x,0x%02x,0x%02x) failed: %d\n",
+			client->addr + page, reg, val, ret);
+	return !(ret == 1);
+}
+
+static int ps8622_send_config(struct ps8622_bridge *ps_bridge)
+{
+	struct i2c_client *cl = ps_bridge->client;
+	int err = 0;
+
+	/* wait 20ms after power ON */
+	usleep_range(20000, 30000);
+
+	err |= ps8622_set(cl, 0x02, 0xa1, 0x01); /* HPD low */
+	/* SW setting */
+	err |= ps8622_set(cl, 0x04, 0x14, 0x01); /* [1:0] SW output 1.2V voltage
+						  * is lower to 96% */
+	/* RCO SS setting */
+	err |= ps8622_set(cl, 0x04, 0xe3, 0x20); /* [5:4] = b01 0.5%, b10 1%,
+						  * b11 1.5% */
+	err |= ps8622_set(cl, 0x04, 0xe2, 0x80); /* [7] RCO SS enable */
+	/* RPHY Setting */
+	err |= ps8622_set(cl, 0x04, 0x8a, 0x0c); /* [3:2] CDR tune wait cycle
+						  * before measure for fine tune
+						  * b00: 1us b01: 0.5us b10:2us
+						  * b11: 4us */
+	err |= ps8622_set(cl, 0x04, 0x89, 0x08); /* [3] RFD always on */
+	err |= ps8622_set(cl, 0x04, 0x71, 0x2d); /* CTN lock in/out:
+						  * 20000ppm/80000ppm.
+						  * Lock out 2 times. */
+	/* 2.7G CDR settings */
+	err |= ps8622_set(cl, 0x04, 0x7d, 0x07); /* NOF=40LSB for HBR CDR
+						  * setting */
+	err |= ps8622_set(cl, 0x04, 0x7b, 0x00); /* [1:0] Fmin=+4bands */
+	err |= ps8622_set(cl, 0x04, 0x7a, 0xfd); /* [7:5] DCO_FTRNG=+-40% */
+	/* 1.62G CDR settings */
+	err |= ps8622_set(cl, 0x04, 0xc0, 0x12); /* [5:2]NOF=64LSB [1:0]DCO
+						  * scale is 2/5 */
+	err |= ps8622_set(cl, 0x04, 0xc1, 0x92); /* Gitune=-37% */
+	err |= ps8622_set(cl, 0x04, 0xc2, 0x1c); /* Fbstep=100% */
+	err |= ps8622_set(cl, 0x04, 0x32, 0x80); /* [7] LOS signal disable */
+	/* RPIO Setting */
+	err |= ps8622_set(cl, 0x04, 0x00, 0xb0); /* [7:4] LVDS driver bias
+						  * current : 75% (250mV swing)
+						  * */
+	err |= ps8622_set(cl, 0x04, 0x15, 0x40); /* [7:6] Right-bar GPIO output
+						  * strength is 8mA */
+	/* EQ Training State Machine Setting */
+	err |= ps8622_set(cl, 0x04, 0x54, 0x10); /* RCO calibration start */
+	/* Logic, needs more than 10 I2C command */
+	err |= ps8622_set(cl, 0x01, 0x02, 0x80 | ps_bridge->max_lane_count);
+						 /* [4:0] MAX_LANE_COUNT set to
+						  * max supported lanes */
+	err |= ps8622_set(cl, 0x01, 0x21, 0x80 | ps_bridge->lane_count);
+						 /* [4:0] LANE_COUNT_SET set to
+						  * chosen lane count */
+	err |= ps8622_set(cl, 0x00, 0x52, 0x20);
+	err |= ps8622_set(cl, 0x00, 0xf1, 0x03); /* HPD CP toggle enable */
+	err |= ps8622_set(cl, 0x00, 0x62, 0x41);
+	err |= ps8622_set(cl, 0x00, 0xf6, 0x01); /* Counter number, add 1ms
+						  * counter delay */
+	err |= ps8622_set(cl, 0x00, 0x77, 0x06); /* [6]PWM function control by
+						  * DPCD0040f[7], default is PWM
+						  * block always works. */
+	err |= ps8622_set(cl, 0x00, 0x4c, 0x04); /* 04h Adjust VTotal tolerance
+						  * to fix the 30Hz no display
+						  * issue */
+	err |= ps8622_set(cl, 0x01, 0xc0, 0x00); /* DPCD00400='h00, Parade OUI =
+						  * 'h001cf8 */
+	err |= ps8622_set(cl, 0x01, 0xc1, 0x1c); /* DPCD00401='h1c */
+	err |= ps8622_set(cl, 0x01, 0xc2, 0xf8); /* DPCD00402='hf8 */
+	err |= ps8622_set(cl, 0x01, 0xc3, 0x44); /* DPCD403~408 = ASCII code
+						  * D2SLV5='h4432534c5635 */
+	err |= ps8622_set(cl, 0x01, 0xc4, 0x32); /* DPCD404 */
+	err |= ps8622_set(cl, 0x01, 0xc5, 0x53); /* DPCD405 */
+	err |= ps8622_set(cl, 0x01, 0xc6, 0x4c); /* DPCD406 */
+	err |= ps8622_set(cl, 0x01, 0xc7, 0x56); /* DPCD407 */
+	err |= ps8622_set(cl, 0x01, 0xc8, 0x35); /* DPCD408 */
+	err |= ps8622_set(cl, 0x01, 0xca, 0x01); /* DPCD40A, Initial Code major
+						  * revision '01' */
+	err |= ps8622_set(cl, 0x01, 0xcb, 0x05); /* DPCD40B, Initial Code minor
+						  * revision '05' */
+	if (ps_bridge->bl) {
+		err |= ps8622_set(cl, 0x01, 0xa5, 0xa0);
+						/* DPCD720, internal PWM */
+		err |= ps8622_set(cl, 0x01, 0xa7,
+				ps_bridge->bl->props.brightness);
+						 /* FFh for 100% brightness,
+						  *  0h for 0% brightness */
+	} else {
+		err |= ps8622_set(cl, 0x01, 0xa5, 0x80);
+						/* DPCD720, external PWM */
+	}
+	err |= ps8622_set(cl, 0x01, 0xcc, 0x13); /* Set LVDS output as 6bit-VESA
+						  * mapping, single LVDS channel
+						  * */
+	err |= ps8622_set(cl, 0x02, 0xb1, 0x20); /* Enable SSC set by register
+						  * */
+	err |= ps8622_set(cl, 0x04, 0x10, 0x16); /* Set SSC enabled and +/-1%
+						  * central spreading */
+	/* Logic end */
+	err |= ps8622_set(cl, 0x04, 0x59, 0x60); /* MPU Clock source: LC => RCO
+						  * */
+	err |= ps8622_set(cl, 0x04, 0x54, 0x14); /* LC -> RCO */
+	err |= ps8622_set(cl, 0x02, 0xa1, 0x91); /* HPD high */
+
+	return err ? -EIO : 0;
+}
+
+static int ps8622_backlight_update(struct backlight_device *bl)
+{
+	struct ps8622_bridge *ps_bridge = dev_get_drvdata(&bl->dev);
+	int ret, brightness = bl->props.brightness;
+
+	if (bl->props.power != FB_BLANK_UNBLANK ||
+	    bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
+		brightness = 0;
+
+	mutex_lock(&ps_bridge->enable_mutex);
+
+	if (!ps_bridge->enabled) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = ps8622_set(ps_bridge->client, 0x01, 0xa7, brightness);
+
+out:
+	mutex_unlock(&ps_bridge->enable_mutex);
+	return ret;
+}
+
+static int ps8622_backlight_get(struct backlight_device *bl)
+{
+	return bl->props.brightness;
+}
+
+static const struct backlight_ops ps8622_backlight_ops = {
+	.update_status	= ps8622_backlight_update,
+	.get_brightness	= ps8622_backlight_get,
+};
+
+static void ps8622_pre_enable(struct drm_bridge *bridge)
+{
+	struct ps8622_bridge *ps_bridge = bridge->driver_private;
+	int ret;
+
+	mutex_lock(&ps_bridge->enable_mutex);
+	if (ps_bridge->enabled)
+		goto out;
+
+	gpiod_set_value(ps_bridge->gpio_rst_n, 0);
+
+	if (ps_bridge->v12) {
+		ret = regulator_enable(ps_bridge->v12);
+		if (ret)
+			DRM_ERROR("fails to enable ps_bridge->v12");
+	}
+
+	drm_next_bridge_pre_enable(bridge);
+
+	gpiod_set_value(ps_bridge->gpio_slp_n, 1);
+
+	/*
+	 * T1 is the range of time that it takes for the power to rise after we
+	 * enable the lcd fet. T2 is the range of time in which the data sheet
+	 * specifies we should deassert the reset pin.
+	 *
+	 * If it takes T1.max for the power to rise, we need to wait atleast
+	 * T2.min before deasserting the reset pin. If it takes T1.min for the
+	 * power to rise, we need to wait at most T2.max before deasserting the
+	 * reset pin.
+	 */
+	usleep_range(PS8622_RST_HIGH_T2_MIN_US + PS8622_POWER_RISE_T1_MAX_US,
+		     PS8622_RST_HIGH_T2_MAX_US + PS8622_POWER_RISE_T1_MIN_US);
+
+	gpiod_set_value(ps_bridge->gpio_rst_n, 1);
+
+	ret = ps8622_send_config(ps_bridge);
+	if (ret)
+		DRM_ERROR("Failed to send config to bridge (%d)\n", ret);
+
+	ps_bridge->enabled = true;
+
+out:
+	mutex_unlock(&ps_bridge->enable_mutex);
+}
+
+static void ps8622_enable(struct drm_bridge *bridge)
+{
+	struct ps8622_bridge *ps_bridge = bridge->driver_private;
+
+	mutex_lock(&ps_bridge->enable_mutex);
+	drm_next_bridge_enable(bridge);
+	mutex_unlock(&ps_bridge->enable_mutex);
+}
+
+static void ps8622_disable(struct drm_bridge *bridge)
+{
+	struct ps8622_bridge *ps_bridge = bridge->driver_private;
+
+	mutex_lock(&ps_bridge->enable_mutex);
+
+	if (!ps_bridge->enabled)
+		goto out;
+
+	ps_bridge->enabled = false;
+
+	drm_next_bridge_disable(bridge);
+	msleep(PS8622_PWMO_END_T12_MS);
+
+	/*
+	 * This doesn't matter if the regulators are turned off, but something
+	 * else might keep them on. In that case, we want to assert the slp gpio
+	 * to lower power.
+	 */
+	gpiod_set_value(ps_bridge->gpio_slp_n, 0);
+
+	if (ps_bridge->v12)
+		regulator_disable(ps_bridge->v12);
+
+	/*
+	 * Sleep for at least the amount of time that it takes the power rail to
+	 * fall to prevent asserting the rst gpio from doing anything.
+	 */
+	usleep_range(PS8622_POWER_FALL_T16_MAX_US,
+		     2 * PS8622_POWER_FALL_T16_MAX_US);
+	gpiod_set_value(ps_bridge->gpio_rst_n, 0);
+
+	msleep(PS8622_POWER_OFF_T17_MS);
+
+out:
+	mutex_unlock(&ps_bridge->enable_mutex);
+}
+
+static void ps8622_post_disable(struct drm_bridge *bridge)
+{
+	drm_next_bridge_post_disable(bridge);
+}
+
+static void ps8622_destroy(struct drm_bridge *bridge)
+{
+	struct ps8622_bridge *ps_bridge = bridge->driver_private;
+
+	drm_bridge_cleanup(bridge);
+	if (ps_bridge->bl)
+		backlight_device_unregister(ps_bridge->bl);
+
+	put_device(&ps_bridge->client->dev);
+}
+
+static const struct drm_bridge_funcs ps8622_bridge_funcs = {
+	.pre_enable = ps8622_pre_enable,
+	.enable = ps8622_enable,
+	.disable = ps8622_disable,
+	.post_disable = ps8622_post_disable,
+	.destroy = ps8622_destroy,
+};
+
+static const struct of_device_id ps8622_devices[] = {
+	{
+		.compatible = "parade,ps8622",
+		.data	= &ps8622_data,
+	}, {
+		.compatible = "parade,ps8625",
+		.data	= &ps8625_data,
+	}, {
+		/* end node */
+	}
+};
+
+struct drm_bridge *ps8622_init(struct drm_device *dev,
+				struct drm_encoder *encoder,
+				struct i2c_client *client,
+				struct device_node *node)
+{
+	int ret;
+	const struct of_device_id *match;
+	const struct ps8622_device_data *device_data;
+	struct drm_bridge *bridge;
+	struct ps8622_bridge *ps_bridge;
+
+	node = of_find_matching_node_and_match(NULL, ps8622_devices, &match);
+	if (!node)
+		return NULL;
+
+	bridge = devm_kzalloc(dev->dev, sizeof(*bridge), GFP_KERNEL);
+	if (!bridge) {
+		DRM_ERROR("Failed to allocate drm bridge\n");
+		return NULL;
+	}
+
+	ps_bridge = devm_kzalloc(dev->dev, sizeof(*ps_bridge), GFP_KERNEL);
+	if (!ps_bridge)
+		DRM_ERROR("could not allocate ps bridge\n");
+
+	mutex_init(&ps_bridge->enable_mutex);
+
+	device_data = match->data;
+	ps_bridge->client = client;
+	ps_bridge->encoder = encoder;
+	ps_bridge->bridge = bridge;
+	ps_bridge->v12 = devm_regulator_get(&client->dev, "vdd_bridge");
+	if (IS_ERR(ps_bridge->v12)) {
+		DRM_INFO("no 1.2v regulator found for PS8622\n");
+		ps_bridge->v12 = NULL;
+	}
+
+	ps_bridge->gpio_slp_n = devm_gpiod_get(&client->dev, "sleep");
+	if (IS_ERR(ps_bridge->gpio_slp_n)) {
+		ret = PTR_ERR(ps_bridge->gpio_slp_n);
+		DRM_ERROR("cannot get gpio_slp_n %d\n", ret);
+		goto err_client;
+	} else {
+		ret = gpiod_direction_output(ps_bridge->gpio_slp_n, 1);
+		if (ret) {
+			DRM_ERROR("cannot configure gpio_slp_n\n");
+			goto err_client;
+		}
+	}
+
+	ps_bridge->gpio_rst_n = devm_gpiod_get(&client->dev, "reset");
+	if (IS_ERR(ps_bridge->gpio_rst_n)) {
+		ret = PTR_ERR(ps_bridge->gpio_rst_n);
+		DRM_ERROR("cannot get gpio_rst_n %d\n", ret);
+		goto err_client;
+	} else {
+		/*
+		 * Assert the reset pin high to avoid the bridge being
+		 * initialized prematurely
+		 */
+		ret = gpiod_direction_output(ps_bridge->gpio_rst_n, 1);
+		if (ret) {
+			DRM_ERROR("cannot configure gpio_slp_n\n");
+			goto err_client;
+		}
+	}
+
+	ps_bridge->max_lane_count = device_data->max_lane_count;
+
+	if (of_property_read_u8(node, "lane-count", &ps_bridge->lane_count))
+		ps_bridge->lane_count = ps_bridge->max_lane_count;
+	else if (ps_bridge->lane_count > ps_bridge->max_lane_count) {
+		DRM_INFO("lane-count property is too high for DP bridge\n");
+		ps_bridge->lane_count = ps_bridge->max_lane_count;
+	}
+
+	if (!of_find_property(node, "use-external-pwm", NULL)) {
+		ps_bridge->bl = backlight_device_register("ps8622-backlight",
+				dev->dev, ps_bridge, &ps8622_backlight_ops,
+				NULL);
+		if (IS_ERR(ps_bridge->bl)) {
+			DRM_ERROR("failed to register backlight\n");
+			ret = PTR_ERR(ps_bridge->bl);
+			ps_bridge->bl = NULL;
+			goto err_client;
+		}
+		ps_bridge->bl->props.max_brightness = PS8622_MAX_BRIGHTNESS;
+		ps_bridge->bl->props.brightness = PS8622_MAX_BRIGHTNESS;
+	}
+
+	ret = drm_bridge_init(dev, ps_bridge->bridge, &ps8622_bridge_funcs);
+	if (ret) {
+		DRM_ERROR("Failed to initialize bridge with drm\n");
+		goto err_backlight;
+	}
+
+	ps_bridge->bridge->driver_private = ps_bridge;
+	ps_bridge->enabled = false;
+
+	if (!encoder->bridge)
+		/* First entry in the bridge chain */
+		encoder->bridge = bridge;
+
+	return bridge;
+
+err_backlight:
+	if (ps_bridge->bl)
+		backlight_device_unregister(ps_bridge->bl);
+err_client:
+	put_device(&client->dev);
+	DRM_ERROR("device probe failed : %d\n", ret);
+	return NULL;
+}
+EXPORT_SYMBOL(ps8622_init);
diff --git a/include/drm/bridge/ps8622.h b/include/drm/bridge/ps8622.h
new file mode 100644
index 0000000..c3914b6
--- /dev/null
+++ b/include/drm/bridge/ps8622.h
@@ -0,0 +1,41 @@
+/*
+ * Copyright (C) 2014 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _DRM_BRIDGE_PS8622_H_
+#define _DRM_BRIDGE_PS8622_H_
+
+struct drm_device;
+struct drm_encoder;
+struct i2c_client;
+struct device_node;
+
+#if defined(CONFIG_DRM_PS8622) || defined(CONFIG_DRM_PS8622_MODULE)
+
+struct drm_bridge *ps8622_init(struct drm_device *dev,
+				struct drm_encoder *encoder,
+				struct i2c_client *client,
+				struct device_node *node);
+
+#else
+
+static inline struct drm_bridge *ps8622_init(struct drm_device *dev,
+				struct drm_encoder *encoder,
+				struct i2c_client *client,
+				struct device_node *node)
+{
+	return -ENODEV;
+}
+
+#endif
+
+#endif
-- 
1.7.9.5

[PATCH V5 11/12] Documentation: Add DT bindings for ps8622/ps8625 bridge driver

[Ajay Kumar]

From: Vincent Palatin <vpalatin@chromium.org>

Add DT binding documentation for ps8622/ps8625 bridge driver.

Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 .../devicetree/bindings/drm/bridge/ps8622.txt      |   21 ++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/drm/bridge/ps8622.txt

diff --git a/Documentation/devicetree/bindings/drm/bridge/ps8622.txt b/Documentation/devicetree/bindings/drm/bridge/ps8622.txt
new file mode 100644
index 0000000..1d154ac
--- /dev/null
+++ b/Documentation/devicetree/bindings/drm/bridge/ps8622.txt
@@ -0,0 +1,21 @@
+ps8622-bridge bindings
+
+Required properties:
+	- compatible: "parade,ps8622" or "parade,ps8625"
+	- reg: first i2c address of the bridge
+	- sleep-gpios: OF device-tree gpio specification
+	- reset-gpios: OF device-tree gpio specification
+
+Optional properties:
+	- lane-count: number of DP lanes to use
+	- use-external-pwm: backlight will be controlled by an external PWM
+
+Example:
+	ps8622-bridge@48 {
+		compatible = "parade,ps8622";
+		reg = <0x48>;
+		sleep-gpios = <&gpc3 6 1 0 0>;
+		reset-gpios = <&gpc3 1 1 0 0>;
+		display-timings = <&lcd_display_timings>;
+		lane-count = <1>
+	};
-- 
1.7.9.5

[RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver

[Ajay Kumar]

From: Rahul Sharma <Rahul.Sharma@samsung.com>

This patch adds ps8622 lvds bridge discovery code to the dp driver.

Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>
Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_dp_core.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
index 0ca6256..82e2942 100644
--- a/drivers/gpu/drm/exynos/exynos_dp_core.c
+++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
@@ -31,6 +31,7 @@
 #include <drm/drm_panel.h>
 #include <drm/bridge/ptn3460.h>
 #include <drm/bridge/panel_binder.h>
+#include <drm/bridge/ps8622.h>
 
 #include "exynos_drm_drv.h"
 #include "exynos_dp_core.h"
@@ -999,6 +1000,10 @@ static int exynos_drm_attach_lcd_bridge(struct exynos_dp_device *dp,
 	if (find_bridge("nxp,ptn3460", &bridge)) {
 		bridge_chain = ptn3460_init(dp->drm_dev, encoder, bridge.client,
 								bridge.node);
+	} else if (find_bridge("parade,ps8622", &bridge) ||
+				find_bridge("parade,ps8625", &bridge)) {
+		bridge_chain = ps8622_init(dp->drm_dev, encoder, bridge.client,
+								bridge.node);
 	}
 
 	if (bridge_chain && dp->edp_panel) {
-- 
1.7.9.5

Re: [PATCH V5 05/12] Documentation: Add DT bindings for panel-lvds driver

[Ajay kumar]

+devicetree@vger.kernel.org

On Fri, Jul 18, 2014 at 2:13 AM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
> Add DT binding documentation for panel-lvds driver.
>
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> ---
>  .../devicetree/bindings/panel/panel-lvds.txt       |   50 ++++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/panel/panel-lvds.txt
>
> diff --git a/Documentation/devicetree/bindings/panel/panel-lvds.txt b/Documentation/devicetree/bindings/panel/panel-lvds.txt
> new file mode 100644
> index 0000000..fdf91da2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/panel/panel-lvds.txt
> @@ -0,0 +1,50 @@
> +panel interface for eDP/lvds panels
> +
> +Required properties:
> +  - compatible: "panel-lvds"
> +
> +Optional properties:
> +       -lcd-enable-gpios:
> +               panel LCD poweron GPIO.
> +                       Indicates which GPIO needs to be powered up as output
> +                       to powerup/enable the switch to the LCD panel.
> +       -backlight-enable-gpios:
> +               panel LED enable GPIO.
> +                       Indicates which GPIO needs to be powered up as output
> +                       to enable the backlight.
> +       -panel-prepare-delay:
> +               delay value in ms required for panel_prepare process
> +                       Delay in ms needed for the panel LCD unit to
> +                       powerup completely.
> +                       ex: delay needed till eDP panel throws HPD.
> +                           delay needed so that we cans tart reading edid.
> +       -panel-enable-delay:
> +               delay value in ms required for panel_enable process
> +                       Delay in ms needed for the panel backlight/LED unit
> +                       to powerup, and delay needed between video_enable and
> +                       backlight_enable.
> +       -panel-disable-delay:
> +               delay value in ms required for panel_disable process
> +                       Delay in ms needed for the panel backlight/LED unit
> +                       powerdown, and delay needed between backlight_disable
> +                       and video_disable.
> +       -panel-unprepare-delay:
> +               delay value in ms required for panel_post_disable process
> +                       Delay in ms needed for the panel LCD unit to
> +                       to powerdown completely, and the minimum delay needed
> +                       before powering it on again.
> +       -panel-width-mm: physical panel width [mm]
> +       -panel-height-mm: physical panel height [mm]
> +
> +Example:
> +
> +       panel-lvds {
> +               compatible = "panel-lvds";
> +               backlight-enable-gpios = <&gpx3 0 0>;
> +               panel-prepare-delay = <40>;
> +               panel-enable-delay = <20>;
> +               panel-disable-delay = <20>;
> +               panel-unprepare-delay = <30>;
> +               panel-width-mm = <256>;
> +               panel-height-mm = <144>;
> +       };
> --
> 1.7.9.5
>

Re: [PATCH V5 11/12] Documentation: Add DT bindings for ps8622/ps8625 bridge driver

[Ajay kumar]

+devicetree@vger.kernel.org

On Fri, Jul 18, 2014 at 2:13 AM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
> From: Vincent Palatin <vpalatin@chromium.org>
>
> Add DT binding documentation for ps8622/ps8625 bridge driver.
>
> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> ---
>  .../devicetree/bindings/drm/bridge/ps8622.txt      |   21 ++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/ps8622.txt
>
> diff --git a/Documentation/devicetree/bindings/drm/bridge/ps8622.txt b/Documentation/devicetree/bindings/drm/bridge/ps8622.txt
> new file mode 100644
> index 0000000..1d154ac
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/bridge/ps8622.txt
> @@ -0,0 +1,21 @@
> +ps8622-bridge bindings
> +
> +Required properties:
> +       - compatible: "parade,ps8622" or "parade,ps8625"
> +       - reg: first i2c address of the bridge
> +       - sleep-gpios: OF device-tree gpio specification
> +       - reset-gpios: OF device-tree gpio specification
> +
> +Optional properties:
> +       - lane-count: number of DP lanes to use
> +       - use-external-pwm: backlight will be controlled by an external PWM
> +
> +Example:
> +       ps8622-bridge@48 {
> +               compatible = "parade,ps8622";
> +               reg = <0x48>;
> +               sleep-gpios = <&gpc3 6 1 0 0>;
> +               reset-gpios = <&gpc3 1 1 0 0>;
> +               display-timings = <&lcd_display_timings>;
> +               lane-count = <1>
> +       };
> --
> 1.7.9.5
>

Re: [PATCH V5 05/12] Documentation: Add DT bindings for panel-lvds driver

[Thierry Reding]

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

On Fri, Jul 18, 2014 at 02:20:39AM +0530, Ajay kumar wrote:
> +devicetree@vger.kernel.org
> 
> On Fri, Jul 18, 2014 at 2:13 AM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
> > Add DT binding documentation for panel-lvds driver.
> >
> > Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> > ---
> >  .../devicetree/bindings/panel/panel-lvds.txt       |   50 ++++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/panel/panel-lvds.txt
> >
> > diff --git a/Documentation/devicetree/bindings/panel/panel-lvds.txt b/Documentation/devicetree/bindings/panel/panel-lvds.txt
> > new file mode 100644
> > index 0000000..fdf91da2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/panel/panel-lvds.txt
> > @@ -0,0 +1,50 @@
> > +panel interface for eDP/lvds panels
> > +
> > +Required properties:
> > +  - compatible: "panel-lvds"

I think I've said this before. I oppose the addition of this binding. We
need to list a device-specific compatible value here, wildcards like
this aren't a good choice. And then if we have that compatible value we
can move most of the optional properties below into a driver.

> > +Optional properties:

If all these properties are optional, then what happens if a device tree
node doesn't contain any of them? Doesn't that turn the driver into one
big no-op?

> > +       -lcd-enable-gpios:
> > +               panel LCD poweron GPIO.
> > +                       Indicates which GPIO needs to be powered up as output
> > +                       to powerup/enable the switch to the LCD panel.
> > +       -backlight-enable-gpios:
> > +               panel LED enable GPIO.
> > +                       Indicates which GPIO needs to be powered up as output
> > +                       to enable the backlight.

I've also said before that this really belongs in a backlight driver.
Chances are that you'll want to have a way to set the brightness of the
backlight as well, so simply an enable GPIO won't be good enough.

> > +       -panel-prepare-delay:
> > +               delay value in ms required for panel_prepare process
> > +                       Delay in ms needed for the panel LCD unit to
> > +                       powerup completely.
> > +                       ex: delay needed till eDP panel throws HPD.
> > +                           delay needed so that we cans tart reading edid.

If the panel signals HPD then we don't need this delay at all and we
should just wait for HPD instead.

> > +       -panel-enable-delay:
> > +               delay value in ms required for panel_enable process
> > +                       Delay in ms needed for the panel backlight/LED unit
> > +                       to powerup, and delay needed between video_enable and
> > +                       backlight_enable.
> > +       -panel-disable-delay:
> > +               delay value in ms required for panel_disable process
> > +                       Delay in ms needed for the panel backlight/LED unit
> > +                       powerdown, and delay needed between backlight_disable
> > +                       and video_disable.
> > +       -panel-unprepare-delay:
> > +               delay value in ms required for panel_post_disable process
> > +                       Delay in ms needed for the panel LCD unit to
> > +                       to powerdown completely, and the minimum delay needed
> > +                       before powering it on again.

These delays are all panel specific and they don't vary per board, so
they shouldn't go into the device tree at all.

> > +       -panel-width-mm: physical panel width [mm]
> > +       -panel-height-mm: physical panel height [mm]

Same here.

Thierry

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

Re: [PATCH V5 05/12] Documentation: Add DT bindings for panel-lvds driver

[Ajay kumar]

Hi Thierry,

Thanks for your comments.

On Fri, Jul 18, 2014 at 4:18 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Fri, Jul 18, 2014 at 02:20:39AM +0530, Ajay kumar wrote:
>> +devicetree@vger.kernel.org
>>
>> On Fri, Jul 18, 2014 at 2:13 AM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
>> > Add DT binding documentation for panel-lvds driver.
>> >
>> > Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>> > ---
>> >  .../devicetree/bindings/panel/panel-lvds.txt       |   50 ++++++++++++++++++++
>> >  1 file changed, 50 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/panel/panel-lvds.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/panel/panel-lvds.txt b/Documentation/devicetree/bindings/panel/panel-lvds.txt
>> > new file mode 100644
>> > index 0000000..fdf91da2
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/panel/panel-lvds.txt
>> > @@ -0,0 +1,50 @@
>> > +panel interface for eDP/lvds panels
>> > +
>> > +Required properties:
>> > +  - compatible: "panel-lvds"
>
> I think I've said this before. I oppose the addition of this binding. We
> need to list a device-specific compatible value here, wildcards like
> this aren't a good choice. And then if we have that compatible value we
> can move most of the optional properties below into a driver.
Yes, "panel-lvds" is a wildcard for all lvds panels.
And since lvds panels from different vendors have different values
for power_up_delay, delay from video_to_backlight etc, I thought its
better to pick them up from device tree.

>> > +Optional properties:
>
> If all these properties are optional, then what happens if a device tree
> node doesn't contain any of them? Doesn't that turn the driver into one
> big no-op?
No! We need to provide lcd-supply and backlight-supply.

>> > +       -lcd-enable-gpios:
>> > +               panel LCD poweron GPIO.
>> > +                       Indicates which GPIO needs to be powered up as output
>> > +                       to powerup/enable the switch to the LCD panel.
>> > +       -backlight-enable-gpios:
>> > +               panel LED enable GPIO.
>> > +                       Indicates which GPIO needs to be powered up as output
>> > +                       to enable the backlight.
>
> I've also said before that this really belongs in a backlight driver.
> Chances are that you'll want to have a way to set the brightness of the
> backlight as well, so simply an enable GPIO won't be good enough.
Ok. I can handle this in backlight driver itself (with some minor glitches).
But, how do I map bridge functions to panel functions now?
The bridge supports (pre_enable, enable, disable and post_disable) which I map
with (prepare, enable, disable and unprepare) of the panel, using a sample layer
called bridge to panel_binder.
Moving out the backlight control from panel means I really don't need
those extra
panel calls(prepare and unprepare)!
Then how to distribute 2 panel calls(enable and disable) across 4 bridge calls?

>> > +       -panel-prepare-delay:
>> > +               delay value in ms required for panel_prepare process
>> > +                       Delay in ms needed for the panel LCD unit to
>> > +                       powerup completely.
>> > +                       ex: delay needed till eDP panel throws HPD.
>> > +                           delay needed so that we cans tart reading edid.
>
> If the panel signals HPD then we don't need this delay at all and we
> should just wait for HPD instead.
Not always for HPD, we need to wait for EDID module as well.

>> > +       -panel-enable-delay:
>> > +               delay value in ms required for panel_enable process
>> > +                       Delay in ms needed for the panel backlight/LED unit
>> > +                       to powerup, and delay needed between video_enable and
>> > +                       backlight_enable.
>> > +       -panel-disable-delay:
>> > +               delay value in ms required for panel_disable process
>> > +                       Delay in ms needed for the panel backlight/LED unit
>> > +                       powerdown, and delay needed between backlight_disable
>> > +                       and video_disable.
>> > +       -panel-unprepare-delay:
>> > +               delay value in ms required for panel_post_disable process
>> > +                       Delay in ms needed for the panel LCD unit to
>> > +                       to powerdown completely, and the minimum delay needed
>> > +                       before powering it on again.
>
> These delays are all panel specific and they don't vary per board, so
> they shouldn't go into the device tree at all.
But, fetching them from device tree would allow us to support all lvds
panels in this single driver.

Thanks and Regards,
Ajay Kumar

Re: [PATCH V5 11/12] Documentation: Add DT bindings for ps8622/ps8625 bridge driver

[Thierry Reding]

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

On Fri, Jul 18, 2014 at 02:13:57AM +0530, Ajay Kumar wrote:
> From: Vincent Palatin <vpalatin@chromium.org>
> 
> Add DT binding documentation for ps8622/ps8625 bridge driver.
> 
> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> ---
>  .../devicetree/bindings/drm/bridge/ps8622.txt      |   21 ++++++++++++++++++++

This is the wrong directory. Bindings are supposed to be OS agnostic,
but DRM is (mostly) Linux-specific. video/bridge would be a better
subdirectory for this. Somebody really ought to be moving out the
existing bindings in the drm subdirectory to a more proper location.

>  1 file changed, 21 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/ps8622.txt
> 
> diff --git a/Documentation/devicetree/bindings/drm/bridge/ps8622.txt b/Documentation/devicetree/bindings/drm/bridge/ps8622.txt
> new file mode 100644
> index 0000000..1d154ac
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/bridge/ps8622.txt
> @@ -0,0 +1,21 @@
> +ps8622-bridge bindings
> +
> +Required properties:
> +	- compatible: "parade,ps8622" or "parade,ps8625"

Documentation/devicetree/bindings/vendor-prefixes.txt doesn't contain an
entry for parade yet.

> +	- reg: first i2c address of the bridge
> +	- sleep-gpios: OF device-tree gpio specification
> +	- reset-gpios: OF device-tree gpio specification
> +
> +Optional properties:
> +	- lane-count: number of DP lanes to use
> +	- use-external-pwm: backlight will be controlled by an external PWM

In case of an external backlight, don't you still need a way to control
it? Perhaps instead of using this boolean property you should make this
take a phandle to the real backlight? Like so:

	backlight {
		compatible = "pwm-backlight";
		...
	}

	bridge@48 {
		compatible = "parade,ps8622";
		...
		backlight = <&/backlight>;
	}

Then you can simply look up the backlight device when that property
exists and instantiate the built-in backlight otherwise.

> +
> +Example:
> +	ps8622-bridge@48 {
> +		compatible = "parade,ps8622";
> +		reg = <0x48>;
> +		sleep-gpios = <&gpc3 6 1 0 0>;
> +		reset-gpios = <&gpc3 1 1 0 0>;
> +		display-timings = <&lcd_display_timings>;

Why is this specifying display-timings? It's not mentioned in the
binding above and I don't see why the bridge would need to provide
one since it's really a property of the panel.

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: [RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver

[Thierry Reding]

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

On Fri, Jul 18, 2014 at 02:13:58AM +0530, Ajay Kumar wrote:
> From: Rahul Sharma <Rahul.Sharma@samsung.com>
> 
> This patch adds ps8622 lvds bridge discovery code to the dp driver.
> 
> Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_dp_core.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
> index 0ca6256..82e2942 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
> @@ -31,6 +31,7 @@
>  #include <drm/drm_panel.h>
>  #include <drm/bridge/ptn3460.h>
>  #include <drm/bridge/panel_binder.h>
> +#include <drm/bridge/ps8622.h>
>  
>  #include "exynos_drm_drv.h"
>  #include "exynos_dp_core.h"
> @@ -999,6 +1000,10 @@ static int exynos_drm_attach_lcd_bridge(struct exynos_dp_device *dp,
>  	if (find_bridge("nxp,ptn3460", &bridge)) {
>  		bridge_chain = ptn3460_init(dp->drm_dev, encoder, bridge.client,
>  								bridge.node);
> +	} else if (find_bridge("parade,ps8622", &bridge) ||
> +				find_bridge("parade,ps8625", &bridge)) {
> +		bridge_chain = ps8622_init(dp->drm_dev, encoder, bridge.client,
> +								bridge.node);
>  	}

We really ought to be adding some sort of registry at some point.
Otherwise every driver that wants to use bridges needs to come up with a
similar set of helpers to instantiate them.

Also you're making this driver depend on (now) two bridges, whereas it
really shouldn't matter which exact types it supports. Bridges should be
exposed via a generic interface.

Thierry

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

Re: [PATCH V5 00/12] drm/exynos: few patches to enhance bridge chip support

[Inki Dae]

On 2014년 07월 18일 05:43, Ajay Kumar wrote:
> This series is based on exynos-drm-next branch of Inki Dae's tree at:
> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
> 
> This patchset also consolidates various inputs from the drm community
> regarding the bridge chaining concept:
> (1) [RFC V2 0/3] drm/bridge: panel and chaining
> 	http://www.spinics.net/lists/linux-samsung-soc/msg30160.html
> (2) [RFC V3 0/3] drm/bridge: panel and chaining
> 	http://www.spinics.net/lists/linux-samsung-soc/msg30507.html
> 
> I have tested this after adding few DT changes for exynos5250-snow,
> exynos5420-peach-pit and exynos5800-peach-pi boards.
> 
> The V4 series of this particular patchset was also tested by:
> Rahul Sharma <rahul.sharma@samsung.com>
> Javier Martinez Canillas <javier@dowhile0.org>
> 
> Changes since V2:
> 	-- Address comments from Jingoo Han for ps8622 driver
> 	-- Address comments from Daniel, Rob and Thierry regarding
> 	   bridge chaining
> 	-- Address comments from Thierry regarding the names for
> 	   new drm_panel functions
> 
> Changes since V3:
> 	-- Remove hotplug based initialization of exynos_dp
> 	-- Make exynos_dp work directly with drm_panel, remove
> 	   dependency on panel_binder
> 	-- Minor cleanups in panel_binder and panel_lvds driver
> 
> Changes since V4:
> 	-- Use gpiod interface for panel-lvds and ps8622 drivers.
> 	-- Address comments from Javier.
> 	-- Fix compilation issues when PANEL_BINDER is selected as module.
> 	-- Split Documentation patches from driver patches.
> 	-- Rebase on top of the tree.

Hi Ajay,

Thanks for your contribution.

I am reviewing your patch series. Sorry for late. Below is my comment.

How about using graph concept to bind eDP, LVDS bridge, and Panel? Your
patch tries to bind bridge driver using find_bridge function, and this
function tries to find bridge device node directly using
of_find_compatible_node() again, which in turn make eDP driver to add
all codes related to all bridge devices including all relevant bridge
headers: i.e., if there are five bridge devices then eDP driver should
try to find all of them. That is really not good.

So my opinion is to define the relationship between eDP, LVDS, and Panel
using graph concept: eDP context would have panel and bridge object
according to graph definition of device tree.

As a result, eDP context has already bridge_chain object for lvds bridge
device and panel_binder->bridge object in exynos_drm_attach_lcd_bridge
function context so it can add bridge_chain object  to
panel_binder->bridge as is there. I think lvds bridge device drivers
could follow Linux driver model with this approach.

Rob, it seems to need at least your ACK so that I can merge this patch
series to exynos-drm-next.

Thanks,
Inki Dae

> 
> Ajay Kumar (9):
>   [RESEND PATCH V5 01/12] drm/exynos: Move DP setup out of hotplug workqueue
>   [RESEND PATCH V5 02/12] drm/panel: add prepare and unprepare routines
>   [RESEND PATCH V5 03/12] drm/exynos: dp: modify driver to support drm_panel
>   [PATCH V5 04/12] drm/panel: Add driver for lvds/edp based panels
>   [PATCH V5 05/12] Documentation: Add DT bindings for panel-lvds driver
>   [RESEND PATCH V5 06/12] drm/bridge: add helper functions to support bridge chain
>   [PATCH V5 07/12] drm/bridge: Add a driver which binds drm_bridge with drm_panel
>   [RESEND PATCH V5 08/12] drm/bridge: ptn3460: Support bridge chaining
>   [RESEND PATCH V5 09/12] drm/exynos: dp: create bridge chain using ptn3460 and panel_binder
> 
> Vincent Palatin (2):
>   [PATCH V5 10/12] Documentation: Add DT bindings for ps8622/ps8625 bridge driver
>   [PATCH V5 11/12] drm/bridge: Add ps8622/ps8625 bridge driver
> 
> Rahul Sharma (1):
>   [RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver
> 
>  .../devicetree/bindings/drm/bridge/ps8622.txt      |   21 +
>  .../devicetree/bindings/panel/panel-lvds.txt       |   50 ++
>  .../devicetree/bindings/video/exynos_dp.txt        |    2 +
>  drivers/gpu/drm/bridge/Kconfig                     |   15 +
>  drivers/gpu/drm/bridge/Makefile                    |    2 +
>  drivers/gpu/drm/bridge/panel_binder.c              |  193 ++++++++
>  drivers/gpu/drm/bridge/ps8622.c                    |  476 ++++++++++++++++++++
>  drivers/gpu/drm/bridge/ptn3460.c                   |  137 +-----
>  drivers/gpu/drm/exynos/Kconfig                     |    1 +
>  drivers/gpu/drm/exynos/exynos_dp_core.c            |   87 +++-
>  drivers/gpu/drm/exynos/exynos_dp_core.h            |    2 +
>  drivers/gpu/drm/panel/Kconfig                      |   10 +
>  drivers/gpu/drm/panel/Makefile                     |    1 +
>  drivers/gpu/drm/panel/panel-lvds.c                 |  268 +++++++++++
>  include/drm/bridge/panel_binder.h                  |   44 ++
>  include/drm/bridge/ps8622.h                        |   41 ++
>  include/drm/bridge/ptn3460.h                       |   15 +-
>  include/drm/drm_crtc.h                             |   72 +++
>  include/drm/drm_panel.h                            |   18 +
>  19 files changed, 1316 insertions(+), 139 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/ps8622.txt
>  create mode 100644 Documentation/devicetree/bindings/panel/panel-lvds.txt
>  create mode 100644 drivers/gpu/drm/bridge/panel_binder.c
>  create mode 100644 drivers/gpu/drm/bridge/ps8622.c
>  create mode 100644 drivers/gpu/drm/panel/panel-lvds.c
>  create mode 100644 include/drm/bridge/panel_binder.h
>  create mode 100644 include/drm/bridge/ps8622.h
> 

Re: [PATCH V5 05/12] Documentation: Add DT bindings for panel-lvds driver

[Thierry Reding]

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

On Fri, Jul 18, 2014 at 12:18:05PM +0530, Ajay kumar wrote:
> Hi Thierry,
> 
> Thanks for your comments.
> 
> On Fri, Jul 18, 2014 at 4:18 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Fri, Jul 18, 2014 at 02:20:39AM +0530, Ajay kumar wrote:
> >> +devicetree@vger.kernel.org
> >>
> >> On Fri, Jul 18, 2014 at 2:13 AM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
> >> > Add DT binding documentation for panel-lvds driver.
> >> >
> >> > Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> >> > ---
> >> >  .../devicetree/bindings/panel/panel-lvds.txt       |   50 ++++++++++++++++++++
> >> >  1 file changed, 50 insertions(+)
> >> >  create mode 100644 Documentation/devicetree/bindings/panel/panel-lvds.txt
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/panel/panel-lvds.txt b/Documentation/devicetree/bindings/panel/panel-lvds.txt
> >> > new file mode 100644
> >> > index 0000000..fdf91da2
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/panel/panel-lvds.txt
> >> > @@ -0,0 +1,50 @@
> >> > +panel interface for eDP/lvds panels
> >> > +
> >> > +Required properties:
> >> > +  - compatible: "panel-lvds"
> >
> > I think I've said this before. I oppose the addition of this binding. We
> > need to list a device-specific compatible value here, wildcards like
> > this aren't a good choice. And then if we have that compatible value we
> > can move most of the optional properties below into a driver.
> Yes, "panel-lvds" is a wildcard for all lvds panels.
> And since lvds panels from different vendors have different values
> for power_up_delay, delay from video_to_backlight etc, I thought its
> better to pick them up from device tree.

What I'm saying is that we shouldn't be adding this type of wildcard.
Instead the compatible property needs to list the specific type of panel
and since the driver will match on that specific compatible, the values
for the delays etc. are all implicit and don't have to be listed in
device tree.

> >> > +Optional properties:
> >
> > If all these properties are optional, then what happens if a device tree
> > node doesn't contain any of them? Doesn't that turn the driver into one
> > big no-op?
> No! We need to provide lcd-supply and backlight-supply.

What are those? I don't see them described anywhere.

> >> > +       -lcd-enable-gpios:
> >> > +               panel LCD poweron GPIO.
> >> > +                       Indicates which GPIO needs to be powered up as output
> >> > +                       to powerup/enable the switch to the LCD panel.
> >> > +       -backlight-enable-gpios:
> >> > +               panel LED enable GPIO.
> >> > +                       Indicates which GPIO needs to be powered up as output
> >> > +                       to enable the backlight.
> >
> > I've also said before that this really belongs in a backlight driver.
> > Chances are that you'll want to have a way to set the brightness of the
> > backlight as well, so simply an enable GPIO won't be good enough.
> Ok. I can handle this in backlight driver itself (with some minor glitches).

You can make it work without glitches as well and we've discussed this
before.

> But, how do I map bridge functions to panel functions now?
> The bridge supports (pre_enable, enable, disable and post_disable) which I map
> with (prepare, enable, disable and unprepare) of the panel, using a sample layer
> called bridge to panel_binder.
> Moving out the backlight control from panel means I really don't need
> those extra
> panel calls(prepare and unprepare)!
> Then how to distribute 2 panel calls(enable and disable) across 4 bridge calls?

The .prepare() and .unprepare() functions are useful irrespective of
backlight control. What you want to separate is powering up the panel
from enabling the backlight. So .prepare() could be what's doing the
power up of the panel (and possibly other initialization required to
make it work) and .enable() could be as simple as turning on the
backlight.

What I'm saying is rather than use a backlight-enable-gpios property in
the panel, that property should be part of the backlight device and the
GPIO can then be toggled by the backlight driver when the backlight is
enabled.

> >> > +       -panel-prepare-delay:
> >> > +               delay value in ms required for panel_prepare process
> >> > +                       Delay in ms needed for the panel LCD unit to
> >> > +                       powerup completely.
> >> > +                       ex: delay needed till eDP panel throws HPD.
> >> > +                           delay needed so that we cans tart reading edid.
> >
> > If the panel signals HPD then we don't need this delay at all and we
> > should just wait for HPD instead.
> Not always for HPD, we need to wait for EDID module as well.

I always thought that HPD signalled readiness from the panel to provide
EDID, too. Are there really panels that need additional delays after HPD
has been signalled until the EDID can be read? Are there maybe other
ways to detect that EDID can't be read?

> >> > +       -panel-enable-delay:
> >> > +               delay value in ms required for panel_enable process
> >> > +                       Delay in ms needed for the panel backlight/LED unit
> >> > +                       to powerup, and delay needed between video_enable and
> >> > +                       backlight_enable.
> >> > +       -panel-disable-delay:
> >> > +               delay value in ms required for panel_disable process
> >> > +                       Delay in ms needed for the panel backlight/LED unit
> >> > +                       powerdown, and delay needed between backlight_disable
> >> > +                       and video_disable.
> >> > +       -panel-unprepare-delay:
> >> > +               delay value in ms required for panel_post_disable process
> >> > +                       Delay in ms needed for the panel LCD unit to
> >> > +                       to powerdown completely, and the minimum delay needed
> >> > +                       before powering it on again.
> >
> > These delays are all panel specific and they don't vary per board, so
> > they shouldn't go into the device tree at all.
> But, fetching them from device tree would allow us to support all lvds
> panels in this single driver.

You can do that even without having this data in device tree. My point
is that this is redundant information once you know the panel's specific
compatible value. Therefore there shouldn't be a need to list this in
device tree again.

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: [RESEND PATCH V5 08/12] drm/bridge: ptn3460: Support bridge chaining

[Inki Dae]

On 2014년 07월 18일 05:43, Ajay Kumar wrote:
> Modify the driver to invoke callbacks for the next bridge
> in the bridge chain.
> Also, remove the drm_connector implementation from ptn3460,
> since the same is implemented using panel_binder.
> 
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> ---
>  drivers/gpu/drm/bridge/ptn3460.c        |  137 +++++--------------------------
>  drivers/gpu/drm/exynos/exynos_dp_core.c |   16 ++--
>  include/drm/bridge/ptn3460.h            |   15 ++--
>  3 files changed, 39 insertions(+), 129 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c
> index d466696..5fe16c6 100644
> --- a/drivers/gpu/drm/bridge/ptn3460.c
> +++ b/drivers/gpu/drm/bridge/ptn3460.c
> @@ -34,37 +34,15 @@
>  #define PTN3460_EDID_SRAM_LOAD_ADDR		0x85
>  
>  struct ptn3460_bridge {
> -	struct drm_connector connector;
>  	struct i2c_client *client;
>  	struct drm_encoder *encoder;
>  	struct drm_bridge *bridge;
> -	struct edid *edid;
>  	int gpio_pd_n;
>  	int gpio_rst_n;
>  	u32 edid_emulation;
>  	bool enabled;
>  };
>  
> -static int ptn3460_read_bytes(struct ptn3460_bridge *ptn_bridge, char addr,
> -		u8 *buf, int len)
> -{
> -	int ret;
> -
> -	ret = i2c_master_send(ptn_bridge->client, &addr, 1);
> -	if (ret <= 0) {
> -		DRM_ERROR("Failed to send i2c command, ret=%d\n", ret);
> -		return ret;
> -	}
> -
> -	ret = i2c_master_recv(ptn_bridge->client, buf, len);
> -	if (ret <= 0) {
> -		DRM_ERROR("Failed to recv i2c data, ret=%d\n", ret);
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  static int ptn3460_write_byte(struct ptn3460_bridge *ptn_bridge, char addr,
>  		char val)
>  {
> @@ -126,6 +104,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
>  		gpio_set_value(ptn_bridge->gpio_rst_n, 1);
>  	}
>  
> +	drm_next_bridge_pre_enable(bridge);
> +
>  	/*
>  	 * There's a bug in the PTN chip where it falsely asserts hotplug before
>  	 * it is fully functional. We're forced to wait for the maximum start up
> @@ -142,6 +122,7 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
>  
>  static void ptn3460_enable(struct drm_bridge *bridge)
>  {
> +	drm_next_bridge_enable(bridge);
>  }
>  
>  static void ptn3460_disable(struct drm_bridge *bridge)
> @@ -153,6 +134,8 @@ static void ptn3460_disable(struct drm_bridge *bridge)
>  
>  	ptn_bridge->enabled = false;
>  
> +	drm_next_bridge_disable(bridge);
> +
>  	if (gpio_is_valid(ptn_bridge->gpio_rst_n))
>  		gpio_set_value(ptn_bridge->gpio_rst_n, 1);
>  
> @@ -162,6 +145,7 @@ static void ptn3460_disable(struct drm_bridge *bridge)
>  
>  static void ptn3460_post_disable(struct drm_bridge *bridge)
>  {
> +	drm_next_bridge_post_disable(bridge);
>  }
>  
>  void ptn3460_bridge_destroy(struct drm_bridge *bridge)
> @@ -173,6 +157,9 @@ void ptn3460_bridge_destroy(struct drm_bridge *bridge)
>  		gpio_free(ptn_bridge->gpio_pd_n);
>  	if (gpio_is_valid(ptn_bridge->gpio_rst_n))
>  		gpio_free(ptn_bridge->gpio_rst_n);
> +
> +	drm_next_bridge_destroy(bridge);
> +
>  	/* Nothing else to free, we've got devm allocated memory */
>  }
>  
> @@ -184,81 +171,10 @@ struct drm_bridge_funcs ptn3460_bridge_funcs = {
>  	.destroy = ptn3460_bridge_destroy,
>  };
>  
> -int ptn3460_get_modes(struct drm_connector *connector)
> -{
> -	struct ptn3460_bridge *ptn_bridge;
> -	u8 *edid;
> -	int ret, num_modes;
> -	bool power_off;
> -
> -	ptn_bridge = container_of(connector, struct ptn3460_bridge, connector);
> -
> -	if (ptn_bridge->edid)
> -		return drm_add_edid_modes(connector, ptn_bridge->edid);
> -
> -	power_off = !ptn_bridge->enabled;
> -	ptn3460_pre_enable(ptn_bridge->bridge);
> -
> -	edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
> -	if (!edid) {
> -		DRM_ERROR("Failed to allocate edid\n");
> -		return 0;
> -	}
> -
> -	ret = ptn3460_read_bytes(ptn_bridge, PTN3460_EDID_ADDR, edid,
> -			EDID_LENGTH);
> -	if (ret) {
> -		kfree(edid);
> -		num_modes = 0;
> -		goto out;
> -	}
> -
> -	ptn_bridge->edid = (struct edid *)edid;
> -	drm_mode_connector_update_edid_property(connector, ptn_bridge->edid);
> -
> -	num_modes = drm_add_edid_modes(connector, ptn_bridge->edid);
> -
> -out:
> -	if (power_off)
> -		ptn3460_disable(ptn_bridge->bridge);
> -
> -	return num_modes;
> -}
> -
> -struct drm_encoder *ptn3460_best_encoder(struct drm_connector *connector)
> -{
> -	struct ptn3460_bridge *ptn_bridge;
> -
> -	ptn_bridge = container_of(connector, struct ptn3460_bridge, connector);
> -
> -	return ptn_bridge->encoder;
> -}
> -
> -struct drm_connector_helper_funcs ptn3460_connector_helper_funcs = {
> -	.get_modes = ptn3460_get_modes,
> -	.best_encoder = ptn3460_best_encoder,
> -};
> -
> -enum drm_connector_status ptn3460_detect(struct drm_connector *connector,
> -		bool force)
> -{
> -	return connector_status_connected;
> -}
> -
> -void ptn3460_connector_destroy(struct drm_connector *connector)
> -{
> -	drm_connector_cleanup(connector);
> -}
> -
> -struct drm_connector_funcs ptn3460_connector_funcs = {
> -	.dpms = drm_helper_connector_dpms,
> -	.fill_modes = drm_helper_probe_single_connector_modes,
> -	.detect = ptn3460_detect,
> -	.destroy = ptn3460_connector_destroy,
> -};
> -
> -int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
> -		struct i2c_client *client, struct device_node *node)
> +struct drm_bridge *ptn3460_init(struct drm_device *dev,
> +				struct drm_encoder *encoder,
> +				struct i2c_client *client,
> +				struct device_node *node)
>  {
>  	int ret;
>  	struct drm_bridge *bridge;
> @@ -267,13 +183,13 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
>  	bridge = devm_kzalloc(dev->dev, sizeof(*bridge), GFP_KERNEL);
>  	if (!bridge) {
>  		DRM_ERROR("Failed to allocate drm bridge\n");
> -		return -ENOMEM;
> +		return NULL;

I think you could handle error case correctly. Please return
ERR_PTR(-ENOMEM) instead of NULL, and below codes also.

Thanks,
Inki Dae

>  	}
>  
>  	ptn_bridge = devm_kzalloc(dev->dev, sizeof(*ptn_bridge), GFP_KERNEL);
>  	if (!ptn_bridge) {
>  		DRM_ERROR("Failed to allocate ptn bridge\n");
> -		return -ENOMEM;
> +		return NULL;
>  	}
>  
>  	ptn_bridge->client = client;
> @@ -285,7 +201,7 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
>  				GPIOF_OUT_INIT_HIGH, "PTN3460_PD_N");
>  		if (ret) {
>  			DRM_ERROR("Request powerdown-gpio failed (%d)\n", ret);
> -			return ret;
> +			return NULL;
>  		}
>  	}
>  
> @@ -300,7 +216,7 @@ int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
>  		if (ret) {
>  			DRM_ERROR("Request reset-gpio failed (%d)\n", ret);
>  			gpio_free(ptn_bridge->gpio_pd_n);
> -			return ret;
> +			return NULL;
>  		}
>  	}
>  

Re: [RESEND PATCH V5 03/12] drm/exynos: dp: modify driver to support drm_panel

[Thierry Reding]

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

On Fri, Jul 18, 2014 at 02:13:49AM +0530, Ajay Kumar wrote:
[...]
> diff --git a/drivers/gpu/drm/exynos/Kconfig b/drivers/gpu/drm/exynos/Kconfig
[...]
>  	depends on DRM_EXYNOS_FIMD && ARCH_EXYNOS && (DRM_PTN3460=n || DRM_PTN3460=y || DRM_PTN3460=DRM_EXYNOS)

One of the reasons the current way of implementing bridges is that it
doesn't scale, as shown by this ridiculous dependency. In patch

	[RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver

you add support for another type of bridge but it doesn't update this
dependency. I suspect that in order for this to work properly you'll
need to extend the dependency like so (rewritten to make it somewhat
cleaner):

	depends on DRM_EXYNOS_FIMD && ARCH_EXYNOS
	depends on DRM_PTN3460=n || DRM_PTN3460=y || DRM_PTN3460=DRM_EXYNOS
	depends on DRM_PS8622=n || DRM_PS8622=y || DRM_PS8622=DRM_EXYNOS

And you'll need one more of these lines for each new bridge chip that
you want to support.

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: [RESEND PATCH V5 03/12] drm/exynos: dp: modify driver to support drm_panel

[Thierry Reding]

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

On Fri, Jul 18, 2014 at 02:13:49AM +0530, Ajay Kumar wrote:
> Add drm_panel controls to support powerup/down of the
> eDP panel, if one is present at the sink side.
> 
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> ---
>  .../devicetree/bindings/video/exynos_dp.txt        |    2 +
>  drivers/gpu/drm/exynos/Kconfig                     |    1 +
>  drivers/gpu/drm/exynos/exynos_dp_core.c            |   45 ++++++++++++++++----
>  drivers/gpu/drm/exynos/exynos_dp_core.h            |    2 +
>  4 files changed, 41 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/video/exynos_dp.txt b/Documentation/devicetree/bindings/video/exynos_dp.txt
> index 53dbccf..c029a09 100644
> --- a/Documentation/devicetree/bindings/video/exynos_dp.txt
> +++ b/Documentation/devicetree/bindings/video/exynos_dp.txt
> @@ -51,6 +51,8 @@ Required properties for dp-controller:
>  			LANE_COUNT1 = 1, LANE_COUNT2 = 2, LANE_COUNT4 = 4
>  	- display-timings: timings for the connected panel as described by
>  		Documentation/devicetree/bindings/video/display-timing.txt
> +	-edp-panel:
> +		phandle for the edp/lvds drm_panel node.

There's really no need for the edp- prefix here. Also please don't use
drm_panel in the binding description since it makes no sense outside of
DRM (and bindings need to be OS agnostic).

Also: "eDP" and "LVDS"

> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
> index a94b114..b3d0d9b 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
> @@ -28,6 +28,7 @@
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_panel.h>
>  #include <drm/bridge/ptn3460.h>
>  
>  #include "exynos_drm_drv.h"
> @@ -1029,6 +1030,9 @@ static int exynos_dp_create_connector(struct exynos_drm_display *display,
>  	drm_connector_register(connector);
>  	drm_mode_connector_attach_encoder(connector, encoder);
>  
> +	if (dp->edp_panel)
> +		drm_panel_attach(dp->edp_panel, &dp->connector);

This function can fail, so you really need to do some error-checking
here.

> @@ -1063,11 +1067,13 @@ static void exynos_dp_poweron(struct exynos_dp_device *dp)
>  	if (dp->dpms_mode == DRM_MODE_DPMS_ON)
>  		return;
>  
> +	drm_panel_prepare(dp->edp_panel);
>  	clk_prepare_enable(dp->clock);
>  	exynos_dp_phy_init(dp);
>  	exynos_dp_init_dp(dp);
>  	enable_irq(dp->irq);
>  	exynos_dp_setup(dp);
> +	drm_panel_enable(dp->edp_panel);

These two as well. clk_prepare_enable() can also fail by the way.

exynos_dp_init_dp() can also fail theoretically (it returns int) but
never returns anything other than 0, so it could just as well return
void.

> @@ -1075,10 +1081,12 @@ static void exynos_dp_poweroff(struct exynos_dp_device *dp)
>  	if (dp->dpms_mode != DRM_MODE_DPMS_ON)
>  		return;
>  
> +	drm_panel_disable(dp->edp_panel);
>  	disable_irq(dp->irq);
>  	flush_work(&dp->hotplug_work);
>  	exynos_dp_phy_exit(dp);
>  	clk_disable_unprepare(dp->clock);
> +	drm_panel_unprepare(dp->edp_panel);
>  }

And these two also.

>  static void exynos_dp_dpms(struct exynos_drm_display *display, int mode)
> @@ -1209,8 +1217,17 @@ err:
>  static int exynos_dp_dt_parse_panel(struct exynos_dp_device *dp)
>  {
>  	int ret;
> +	struct device_node *videomode_parent;
> +
> +	/* Receive video timing info from panel node
> +	 * if there is a panel node
> +	 */
> +	if (dp->panel_node)
> +		videomode_parent = dp->panel_node;
> +	else
> +		videomode_parent = dp->dev->of_node;
>  
> -	ret = of_get_videomode(dp->dev->of_node, &dp->panel.vm,
> +	ret = of_get_videomode(videomode_parent, &dp->panel.vm,

You shouldn't be grabbing the video timings from some random node like
this. Instead you should use the DRM panel's .get_modes() function to
query the supported modes. The panel may not (in fact, should not, at
least under most circumstances) define video timings in the device tree.

> @@ -1341,6 +1353,21 @@ static int exynos_dp_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	dp = devm_kzalloc(&pdev->dev, sizeof(struct exynos_dp_device),
> +				GFP_KERNEL);
> +	if (!dp)
> +		return -ENOMEM;
> +
> +	dp->panel_node = of_parse_phandle(dev->of_node, "edp-panel", 0);

There should be no need to store the struct device_node * obtained here
in the context like this.

> +	if (dp->panel_node) {
> +		dp->edp_panel = of_drm_find_panel(dp->panel_node);
> +		of_node_put(dp->panel_node);

Especially since after this that pointer may become invalid.

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: [RESEND PATCH V5 08/12] drm/bridge: ptn3460: Support bridge chaining

[Thierry Reding]

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

On Fri, Jul 18, 2014 at 02:13:54AM +0530, Ajay Kumar wrote:
[...]
> Also, remove the drm_connector implementation from ptn3460,
> since the same is implemented using panel_binder.

I think that's a step backwards. In fact I think the panel-bridge binder
driver shouldn't be needed at all. At least not for now. We have a very
limited number of bridge drivers, so it shouldn't hurt at this stage to
implement the connector instantiation within each driver. Once we have
more bridge drivers, and therefore a better understanding of what they
need, we can always add something like the panel-binder (though I think
it should be library code, similar to the drm_encoder_helper_add() API,
rather than this kind of self-contained object).

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 V5 11/12] Documentation: Add DT bindings for ps8622/ps8625 bridge driver

[Ajay kumar]

Hi Thierry,

On Mon, Jul 21, 2014 at 12:36 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Fri, Jul 18, 2014 at 02:13:57AM +0530, Ajay Kumar wrote:
>> From: Vincent Palatin <vpalatin@chromium.org>
>>
>> Add DT binding documentation for ps8622/ps8625 bridge driver.
>>
>> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>> ---
>>  .../devicetree/bindings/drm/bridge/ps8622.txt      |   21 ++++++++++++++++++++
>
> This is the wrong directory. Bindings are supposed to be OS agnostic,
> but DRM is (mostly) Linux-specific. video/bridge would be a better
> subdirectory for this. Somebody really ought to be moving out the
> existing bindings in the drm subdirectory to a more proper location.
Ok. I will move them out into video/bridge.

>>  1 file changed, 21 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/ps8622.txt
>>
>> diff --git a/Documentation/devicetree/bindings/drm/bridge/ps8622.txt b/Documentation/devicetree/bindings/drm/bridge/ps8622.txt
>> new file mode 100644
>> index 0000000..1d154ac
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/drm/bridge/ps8622.txt
>> @@ -0,0 +1,21 @@
>> +ps8622-bridge bindings
>> +
>> +Required properties:
>> +     - compatible: "parade,ps8622" or "parade,ps8625"
>
> Documentation/devicetree/bindings/vendor-prefixes.txt doesn't contain an
> entry for parade yet.
I will fix this.

>> +     - reg: first i2c address of the bridge
>> +     - sleep-gpios: OF device-tree gpio specification
>> +     - reset-gpios: OF device-tree gpio specification
>> +
>> +Optional properties:
>> +     - lane-count: number of DP lanes to use
>> +     - use-external-pwm: backlight will be controlled by an external PWM
>
> In case of an external backlight, don't you still need a way to control
> it? Perhaps instead of using this boolean property you should make this
> take a phandle to the real backlight? Like so:
>
>         backlight {
>                 compatible = "pwm-backlight";
>                 ...
>         }
>
>         bridge@48 {
>                 compatible = "parade,ps8622";
>                 ...
>                 backlight = <&/backlight>;
>         }
>
> Then you can simply look up the backlight device when that property
> exists and instantiate the built-in backlight otherwise.
Thanks for the pointer, this approach seems perfect!

>> +
>> +Example:
>> +     ps8622-bridge@48 {
>> +             compatible = "parade,ps8622";
>> +             reg = <0x48>;
>> +             sleep-gpios = <&gpc3 6 1 0 0>;
>> +             reset-gpios = <&gpc3 1 1 0 0>;
>> +             display-timings = <&lcd_display_timings>;
>
> Why is this specifying display-timings? It's not mentioned in the
> binding above and I don't see why the bridge would need to provide
> one since it's really a property of the panel.
Ahh. I was trying to copy bindings from previous patchset and I messed it up.
Will fix it.

Ajay

Re: [RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver

[Ajay kumar]

On Mon, Jul 21, 2014 at 12:40 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Fri, Jul 18, 2014 at 02:13:58AM +0530, Ajay Kumar wrote:
>> From: Rahul Sharma <Rahul.Sharma@samsung.com>
>>
>> This patch adds ps8622 lvds bridge discovery code to the dp driver.
>>
>> Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>
>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_dp_core.c |    5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
>> index 0ca6256..82e2942 100644
>> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
>> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
>> @@ -31,6 +31,7 @@
>>  #include <drm/drm_panel.h>
>>  #include <drm/bridge/ptn3460.h>
>>  #include <drm/bridge/panel_binder.h>
>> +#include <drm/bridge/ps8622.h>
>>
>>  #include "exynos_drm_drv.h"
>>  #include "exynos_dp_core.h"
>> @@ -999,6 +1000,10 @@ static int exynos_drm_attach_lcd_bridge(struct exynos_dp_device *dp,
>>       if (find_bridge("nxp,ptn3460", &bridge)) {
>>               bridge_chain = ptn3460_init(dp->drm_dev, encoder, bridge.client,
>>                                                               bridge.node);
>> +     } else if (find_bridge("parade,ps8622", &bridge) ||
>> +                             find_bridge("parade,ps8625", &bridge)) {
>> +             bridge_chain = ps8622_init(dp->drm_dev, encoder, bridge.client,
>> +                                                             bridge.node);
>>       }
>
> We really ought to be adding some sort of registry at some point.
> Otherwise every driver that wants to use bridges needs to come up with a
> similar set of helpers to instantiate them.
>
> Also you're making this driver depend on (now) two bridges, whereas it
> really shouldn't matter which exact types it supports. Bridges should be
> exposed via a generic interface.

How about moving out the find_bridge() function into a common header file,
and also supporting the list of bridge_init declarations in the same file?

We get bridge chip node from phandle, and then pass the same node
to find_bridge() which searches the list using of_device_is_compatible()
to call the appropriate bridge_init function?

Ajay

Re: [PATCH V5 00/12] drm/exynos: few patches to enhance bridge chip support

[Ajay kumar]

Hi Inki,

On Mon, Jul 21, 2014 at 1:21 PM, Inki Dae <inki.dae@samsung.com> wrote:
> On 2014년 07월 18일 05:43, Ajay Kumar wrote:
>> This series is based on exynos-drm-next branch of Inki Dae's tree at:
>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>
>> This patchset also consolidates various inputs from the drm community
>> regarding the bridge chaining concept:
>> (1) [RFC V2 0/3] drm/bridge: panel and chaining
>>       http://www.spinics.net/lists/linux-samsung-soc/msg30160.html
>> (2) [RFC V3 0/3] drm/bridge: panel and chaining
>>       http://www.spinics.net/lists/linux-samsung-soc/msg30507.html
>>
>> I have tested this after adding few DT changes for exynos5250-snow,
>> exynos5420-peach-pit and exynos5800-peach-pi boards.
>>
>> The V4 series of this particular patchset was also tested by:
>> Rahul Sharma <rahul.sharma@samsung.com>
>> Javier Martinez Canillas <javier@dowhile0.org>
>>
>> Changes since V2:
>>       -- Address comments from Jingoo Han for ps8622 driver
>>       -- Address comments from Daniel, Rob and Thierry regarding
>>          bridge chaining
>>       -- Address comments from Thierry regarding the names for
>>          new drm_panel functions
>>
>> Changes since V3:
>>       -- Remove hotplug based initialization of exynos_dp
>>       -- Make exynos_dp work directly with drm_panel, remove
>>          dependency on panel_binder
>>       -- Minor cleanups in panel_binder and panel_lvds driver
>>
>> Changes since V4:
>>       -- Use gpiod interface for panel-lvds and ps8622 drivers.
>>       -- Address comments from Javier.
>>       -- Fix compilation issues when PANEL_BINDER is selected as module.
>>       -- Split Documentation patches from driver patches.
>>       -- Rebase on top of the tree.
>
> Hi Ajay,
>
> Thanks for your contribution.
>
> I am reviewing your patch series. Sorry for late. Below is my comment.
>
> How about using graph concept to bind eDP, LVDS bridge, and Panel? Your
> patch tries to bind bridge driver using find_bridge function, and this
> function tries to find bridge device node directly using
> of_find_compatible_node() again, which in turn make eDP driver to add
> all codes related to all bridge devices including all relevant bridge
> headers: i.e., if there are five bridge devices then eDP driver should
> try to find all of them. That is really not good.
I agree.

> So my opinion is to define the relationship between eDP, LVDS, and Panel
> using graph concept: eDP context would have panel and bridge object
> according to graph definition of device tree.
>
> As a result, eDP context has already bridge_chain object for lvds bridge
> device and panel_binder->bridge object in exynos_drm_attach_lcd_bridge
> function context so it can add bridge_chain object  to
> panel_binder->bridge as is there. I think lvds bridge device drivers
> could follow Linux driver model with this approach.
Sorry, I didn't quite understand the graph concept. Already Thierry suggested
of having a common repository for list of supported bridges. That way, we can
move out bridge detection stuff from exynos_dp to a common file.

Ajay

> Rob, it seems to need at least your ACK so that I can merge this patch
> series to exynos-drm-next.
>
> Thanks,
> Inki Dae
>
>>
>> Ajay Kumar (9):
>>   [RESEND PATCH V5 01/12] drm/exynos: Move DP setup out of hotplug workqueue
>>   [RESEND PATCH V5 02/12] drm/panel: add prepare and unprepare routines
>>   [RESEND PATCH V5 03/12] drm/exynos: dp: modify driver to support drm_panel
>>   [PATCH V5 04/12] drm/panel: Add driver for lvds/edp based panels
>>   [PATCH V5 05/12] Documentation: Add DT bindings for panel-lvds driver
>>   [RESEND PATCH V5 06/12] drm/bridge: add helper functions to support bridge chain
>>   [PATCH V5 07/12] drm/bridge: Add a driver which binds drm_bridge with drm_panel
>>   [RESEND PATCH V5 08/12] drm/bridge: ptn3460: Support bridge chaining
>>   [RESEND PATCH V5 09/12] drm/exynos: dp: create bridge chain using ptn3460 and panel_binder
>>
>> Vincent Palatin (2):
>>   [PATCH V5 10/12] Documentation: Add DT bindings for ps8622/ps8625 bridge driver
>>   [PATCH V5 11/12] drm/bridge: Add ps8622/ps8625 bridge driver
>>
>> Rahul Sharma (1):
>>   [RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver
>>
>>  .../devicetree/bindings/drm/bridge/ps8622.txt      |   21 +
>>  .../devicetree/bindings/panel/panel-lvds.txt       |   50 ++
>>  .../devicetree/bindings/video/exynos_dp.txt        |    2 +
>>  drivers/gpu/drm/bridge/Kconfig                     |   15 +
>>  drivers/gpu/drm/bridge/Makefile                    |    2 +
>>  drivers/gpu/drm/bridge/panel_binder.c              |  193 ++++++++
>>  drivers/gpu/drm/bridge/ps8622.c                    |  476 ++++++++++++++++++++
>>  drivers/gpu/drm/bridge/ptn3460.c                   |  137 +-----
>>  drivers/gpu/drm/exynos/Kconfig                     |    1 +
>>  drivers/gpu/drm/exynos/exynos_dp_core.c            |   87 +++-
>>  drivers/gpu/drm/exynos/exynos_dp_core.h            |    2 +
>>  drivers/gpu/drm/panel/Kconfig                      |   10 +
>>  drivers/gpu/drm/panel/Makefile                     |    1 +
>>  drivers/gpu/drm/panel/panel-lvds.c                 |  268 +++++++++++
>>  include/drm/bridge/panel_binder.h                  |   44 ++
>>  include/drm/bridge/ps8622.h                        |   41 ++
>>  include/drm/bridge/ptn3460.h                       |   15 +-
>>  include/drm/drm_crtc.h                             |   72 +++
>>  include/drm/drm_panel.h                            |   18 +
>>  19 files changed, 1316 insertions(+), 139 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/drm/bridge/ps8622.txt
>>  create mode 100644 Documentation/devicetree/bindings/panel/panel-lvds.txt
>>  create mode 100644 drivers/gpu/drm/bridge/panel_binder.c
>>  create mode 100644 drivers/gpu/drm/bridge/ps8622.c
>>  create mode 100644 drivers/gpu/drm/panel/panel-lvds.c
>>  create mode 100644 include/drm/bridge/panel_binder.h
>>  create mode 100644 include/drm/bridge/ps8622.h
>>
>

Re: [RESEND PATCH V5 08/12] drm/bridge: ptn3460: Support bridge chaining

[Ajay kumar]

Hi Thierry,

On Mon, Jul 21, 2014 at 1:52 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Fri, Jul 18, 2014 at 02:13:54AM +0530, Ajay Kumar wrote:
> [...]
>> Also, remove the drm_connector implementation from ptn3460,
>> since the same is implemented using panel_binder.
>
> I think that's a step backwards. In fact I think the panel-bridge binder
> driver shouldn't be needed at all. At least not for now. We have a very
> limited number of bridge drivers, so it shouldn't hurt at this stage to
> implement the connector instantiation within each driver. Once we have
> more bridge drivers, and therefore a better understanding of what they
> need, we can always add something like the panel-binder (though I think
> it should be library code, similar to the drm_encoder_helper_add() API,
> rather than this kind of self-contained object).
panel_binder was needed to wrap around panel as a bridge, and this was in turn
needed because people wanted to represent a bridge+panel combo as a chain
of bridges.
So, if we choose to drop panel_binder, we choose to drop the idea of chaining
the bridges, and end up calling drm_panel functions directly from
individual bridges.
And, the bridge will implement the connector functions as you suggested.
I am okay with this, if Daniel/Rob don't have an issue with this.

Ajay

Re: [RESEND PATCH V5 03/12] drm/exynos: dp: modify driver to support drm_panel

[Ajay kumar]

Hi Thierry,

On Mon, Jul 21, 2014 at 1:44 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Fri, Jul 18, 2014 at 02:13:49AM +0530, Ajay Kumar wrote:
>> Add drm_panel controls to support powerup/down of the
>> eDP panel, if one is present at the sink side.
>>
>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>> ---
>>  .../devicetree/bindings/video/exynos_dp.txt        |    2 +
>>  drivers/gpu/drm/exynos/Kconfig                     |    1 +
>>  drivers/gpu/drm/exynos/exynos_dp_core.c            |   45 ++++++++++++++++----
>>  drivers/gpu/drm/exynos/exynos_dp_core.h            |    2 +
>>  4 files changed, 41 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/exynos_dp.txt b/Documentation/devicetree/bindings/video/exynos_dp.txt
>> index 53dbccf..c029a09 100644
>> --- a/Documentation/devicetree/bindings/video/exynos_dp.txt
>> +++ b/Documentation/devicetree/bindings/video/exynos_dp.txt
>> @@ -51,6 +51,8 @@ Required properties for dp-controller:
>>                       LANE_COUNT1 = 1, LANE_COUNT2 = 2, LANE_COUNT4 = 4
>>       - display-timings: timings for the connected panel as described by
>>               Documentation/devicetree/bindings/video/display-timing.txt
>> +     -edp-panel:
>> +             phandle for the edp/lvds drm_panel node.
>
> There's really no need for the edp- prefix here. Also please don't use
> drm_panel in the binding description since it makes no sense outside of
> DRM (and bindings need to be OS agnostic).
>
> Also: "eDP" and "LVDS"
Ok. Will fix this.

>> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
>> index a94b114..b3d0d9b 100644
>> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
>> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
>> @@ -28,6 +28,7 @@
>>  #include <drm/drmP.h>
>>  #include <drm/drm_crtc.h>
>>  #include <drm/drm_crtc_helper.h>
>> +#include <drm/drm_panel.h>
>>  #include <drm/bridge/ptn3460.h>
>>
>>  #include "exynos_drm_drv.h"
>> @@ -1029,6 +1030,9 @@ static int exynos_dp_create_connector(struct exynos_drm_display *display,
>>       drm_connector_register(connector);
>>       drm_mode_connector_attach_encoder(connector, encoder);
>>
>> +     if (dp->edp_panel)
>> +             drm_panel_attach(dp->edp_panel, &dp->connector);
>
> This function can fail, so you really need to do some error-checking
> here.
Ok.

>> @@ -1063,11 +1067,13 @@ static void exynos_dp_poweron(struct exynos_dp_device *dp)
>>       if (dp->dpms_mode == DRM_MODE_DPMS_ON)
>>               return;
>>
>> +     drm_panel_prepare(dp->edp_panel);
>>       clk_prepare_enable(dp->clock);
>>       exynos_dp_phy_init(dp);
>>       exynos_dp_init_dp(dp);
>>       enable_irq(dp->irq);
>>       exynos_dp_setup(dp);
>> +     drm_panel_enable(dp->edp_panel);
>
> These two as well. clk_prepare_enable() can also fail by the way.
>
> exynos_dp_init_dp() can also fail theoretically (it returns int) but
> never returns anything other than 0, so it could just as well return
> void.
Ok. Will fix this.

>> @@ -1075,10 +1081,12 @@ static void exynos_dp_poweroff(struct exynos_dp_device *dp)
>>       if (dp->dpms_mode != DRM_MODE_DPMS_ON)
>>               return;
>>
>> +     drm_panel_disable(dp->edp_panel);
>>       disable_irq(dp->irq);
>>       flush_work(&dp->hotplug_work);
>>       exynos_dp_phy_exit(dp);
>>       clk_disable_unprepare(dp->clock);
>> +     drm_panel_unprepare(dp->edp_panel);
>>  }
>
> And these two also.
Ok.

>>  static void exynos_dp_dpms(struct exynos_drm_display *display, int mode)
>> @@ -1209,8 +1217,17 @@ err:
>>  static int exynos_dp_dt_parse_panel(struct exynos_dp_device *dp)
>>  {
>>       int ret;
>> +     struct device_node *videomode_parent;
>> +
>> +     /* Receive video timing info from panel node
>> +      * if there is a panel node
>> +      */
>> +     if (dp->panel_node)
>> +             videomode_parent = dp->panel_node;
>> +     else
>> +             videomode_parent = dp->dev->of_node;
>>
>> -     ret = of_get_videomode(dp->dev->of_node, &dp->panel.vm,
>> +     ret = of_get_videomode(videomode_parent, &dp->panel.vm,
>
> You shouldn't be grabbing the video timings from some random node like
> this. Instead you should use the DRM panel's .get_modes() function to
> query the supported modes. The panel may not (in fact, should not, at
> least under most circumstances) define video timings in the device tree.
Right, I am planning to use panel-simple driver by adding drm_display_mode
structure for the lvds panels. So, I would not need this change.

>> @@ -1341,6 +1353,21 @@ static int exynos_dp_probe(struct platform_device *pdev)
>>       if (ret)
>>               return ret;
>>
>> +     dp = devm_kzalloc(&pdev->dev, sizeof(struct exynos_dp_device),
>> +                             GFP_KERNEL);
>> +     if (!dp)
>> +             return -ENOMEM;
>> +
>> +     dp->panel_node = of_parse_phandle(dev->of_node, "edp-panel", 0);
>
> There should be no need to store the struct device_node * obtained here
> in the context like this.
>
>> +     if (dp->panel_node) {
>> +             dp->edp_panel = of_drm_find_panel(dp->panel_node);
>> +             of_node_put(dp->panel_node);
>
> Especially since after this that pointer may become invalid.
Same explanation as above. Need not store panel_node inside private context.

Ajay

Re: [PATCH V5 05/12] Documentation: Add DT bindings for panel-lvds driver

[Ajay kumar]

Hi Thierry,

On Mon, Jul 21, 2014 at 1:22 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Fri, Jul 18, 2014 at 12:18:05PM +0530, Ajay kumar wrote:
>> Hi Thierry,
>>
>> Thanks for your comments.
>>
>> On Fri, Jul 18, 2014 at 4:18 AM, Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>> > On Fri, Jul 18, 2014 at 02:20:39AM +0530, Ajay kumar wrote:
>> >> +devicetree@vger.kernel.org
>> >>
>> >> On Fri, Jul 18, 2014 at 2:13 AM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
>> >> > Add DT binding documentation for panel-lvds driver.
>> >> >
>> >> > Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>> >> > ---
>> >> >  .../devicetree/bindings/panel/panel-lvds.txt       |   50 ++++++++++++++++++++
>> >> >  1 file changed, 50 insertions(+)
>> >> >  create mode 100644 Documentation/devicetree/bindings/panel/panel-lvds.txt
>> >> >
>> >> > diff --git a/Documentation/devicetree/bindings/panel/panel-lvds.txt b/Documentation/devicetree/bindings/panel/panel-lvds.txt
>> >> > new file mode 100644
>> >> > index 0000000..fdf91da2
>> >> > --- /dev/null
>> >> > +++ b/Documentation/devicetree/bindings/panel/panel-lvds.txt
>> >> > @@ -0,0 +1,50 @@
>> >> > +panel interface for eDP/lvds panels
>> >> > +
>> >> > +Required properties:
>> >> > +  - compatible: "panel-lvds"
>> >
>> > I think I've said this before. I oppose the addition of this binding. We
>> > need to list a device-specific compatible value here, wildcards like
>> > this aren't a good choice. And then if we have that compatible value we
>> > can move most of the optional properties below into a driver.
>> Yes, "panel-lvds" is a wildcard for all lvds panels.
>> And since lvds panels from different vendors have different values
>> for power_up_delay, delay from video_to_backlight etc, I thought its
>> better to pick them up from device tree.
>
> What I'm saying is that we shouldn't be adding this type of wildcard.
> Instead the compatible property needs to list the specific type of panel
> and since the driver will match on that specific compatible, the values
> for the delays etc. are all implicit and don't have to be listed in
> device tree.
>
>> >> > +Optional properties:
>> >
>> > If all these properties are optional, then what happens if a device tree
>> > node doesn't contain any of them? Doesn't that turn the driver into one
>> > big no-op?
>> No! We need to provide lcd-supply and backlight-supply.
>
> What are those? I don't see them described anywhere.
>
>> >> > +       -lcd-enable-gpios:
>> >> > +               panel LCD poweron GPIO.
>> >> > +                       Indicates which GPIO needs to be powered up as output
>> >> > +                       to powerup/enable the switch to the LCD panel.
>> >> > +       -backlight-enable-gpios:
>> >> > +               panel LED enable GPIO.
>> >> > +                       Indicates which GPIO needs to be powered up as output
>> >> > +                       to enable the backlight.
>> >
>> > I've also said before that this really belongs in a backlight driver.
>> > Chances are that you'll want to have a way to set the brightness of the
>> > backlight as well, so simply an enable GPIO won't be good enough.
>> Ok. I can handle this in backlight driver itself (with some minor glitches).
>
> You can make it work without glitches as well and we've discussed this
> before.
>
>> But, how do I map bridge functions to panel functions now?
>> The bridge supports (pre_enable, enable, disable and post_disable) which I map
>> with (prepare, enable, disable and unprepare) of the panel, using a sample layer
>> called bridge to panel_binder.
>> Moving out the backlight control from panel means I really don't need
>> those extra
>> panel calls(prepare and unprepare)!
>> Then how to distribute 2 panel calls(enable and disable) across 4 bridge calls?
>
> The .prepare() and .unprepare() functions are useful irrespective of
> backlight control. What you want to separate is powering up the panel
> from enabling the backlight. So .prepare() could be what's doing the
> power up of the panel (and possibly other initialization required to
> make it work) and .enable() could be as simple as turning on the
> backlight.
>
> What I'm saying is rather than use a backlight-enable-gpios property in
> the panel, that property should be part of the backlight device and the
> GPIO can then be toggled by the backlight driver when the backlight is
> enabled.
>
>> >> > +       -panel-prepare-delay:
>> >> > +               delay value in ms required for panel_prepare process
>> >> > +                       Delay in ms needed for the panel LCD unit to
>> >> > +                       powerup completely.
>> >> > +                       ex: delay needed till eDP panel throws HPD.
>> >> > +                           delay needed so that we cans tart reading edid.
>> >
>> > If the panel signals HPD then we don't need this delay at all and we
>> > should just wait for HPD instead.
>> Not always for HPD, we need to wait for EDID module as well.
>
> I always thought that HPD signalled readiness from the panel to provide
> EDID, too. Are there really panels that need additional delays after HPD
> has been signalled until the EDID can be read? Are there maybe other
> ways to detect that EDID can't be read?
>
>> >> > +       -panel-enable-delay:
>> >> > +               delay value in ms required for panel_enable process
>> >> > +                       Delay in ms needed for the panel backlight/LED unit
>> >> > +                       to powerup, and delay needed between video_enable and
>> >> > +                       backlight_enable.
>> >> > +       -panel-disable-delay:
>> >> > +               delay value in ms required for panel_disable process
>> >> > +                       Delay in ms needed for the panel backlight/LED unit
>> >> > +                       powerdown, and delay needed between backlight_disable
>> >> > +                       and video_disable.
>> >> > +       -panel-unprepare-delay:
>> >> > +               delay value in ms required for panel_post_disable process
>> >> > +                       Delay in ms needed for the panel LCD unit to
>> >> > +                       to powerdown completely, and the minimum delay needed
>> >> > +                       before powering it on again.
>> >
>> > These delays are all panel specific and they don't vary per board, so
>> > they shouldn't go into the device tree at all.
>> But, fetching them from device tree would allow us to support all lvds
>> panels in this single driver.
>
> You can do that even without having this data in device tree. My point
> is that this is redundant information once you know the panel's specific
> compatible value. Therefore there shouldn't be a need to list this in
> device tree again.
>
> Thierry
I am planning to use panel-simple driver for the lvds panels. Let me test
using that and if things workout well, then will drop this entire driver.

Ajay

Re: [RESEND PATCH V5 08/12] drm/bridge: ptn3460: Support bridge chaining

[Thierry Reding]

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

On Mon, Jul 21, 2014 at 05:28:13PM +0530, Ajay kumar wrote:
> Hi Thierry,
> 
> On Mon, Jul 21, 2014 at 1:52 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Fri, Jul 18, 2014 at 02:13:54AM +0530, Ajay Kumar wrote:
> > [...]
> >> Also, remove the drm_connector implementation from ptn3460,
> >> since the same is implemented using panel_binder.
> >
> > I think that's a step backwards. In fact I think the panel-bridge binder
> > driver shouldn't be needed at all. At least not for now. We have a very
> > limited number of bridge drivers, so it shouldn't hurt at this stage to
> > implement the connector instantiation within each driver. Once we have
> > more bridge drivers, and therefore a better understanding of what they
> > need, we can always add something like the panel-binder (though I think
> > it should be library code, similar to the drm_encoder_helper_add() API,
> > rather than this kind of self-contained object).
> panel_binder was needed to wrap around panel as a bridge, and this was in turn
> needed because people wanted to represent a bridge+panel combo as a chain
> of bridges.
> So, if we choose to drop panel_binder, we choose to drop the idea of chaining
> the bridges, and end up calling drm_panel functions directly from
> individual bridges.
> And, the bridge will implement the connector functions as you suggested.
> I am okay with this, if Daniel/Rob don't have an issue with this.

I think bridge chaining and panel handling are separate issues. That's
why I think we shouldn't mix them like this. From earlier discussion[0]
the conclusion was that the final element in the chain should implement
a connector (with the appropriate type). Often that last element would
be an encoder (when there are no bridges). Sometimes the last element
would be a bridge. It's logical for that same element to also implement
the panel integration since it's closely tied to the connector.

Thierry

[0]: http://lists.freedesktop.org/archives/dri-devel/2014-May/059685.html

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

Re: [RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver

[Thierry Reding]

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

On Mon, Jul 21, 2014 at 04:58:25PM +0530, Ajay kumar wrote:
> On Mon, Jul 21, 2014 at 12:40 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Fri, Jul 18, 2014 at 02:13:58AM +0530, Ajay Kumar wrote:
> >> From: Rahul Sharma <Rahul.Sharma@samsung.com>
> >>
> >> This patch adds ps8622 lvds bridge discovery code to the dp driver.
> >>
> >> Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>
> >> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> >> ---
> >>  drivers/gpu/drm/exynos/exynos_dp_core.c |    5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
> >> index 0ca6256..82e2942 100644
> >> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
> >> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
> >> @@ -31,6 +31,7 @@
> >>  #include <drm/drm_panel.h>
> >>  #include <drm/bridge/ptn3460.h>
> >>  #include <drm/bridge/panel_binder.h>
> >> +#include <drm/bridge/ps8622.h>
> >>
> >>  #include "exynos_drm_drv.h"
> >>  #include "exynos_dp_core.h"
> >> @@ -999,6 +1000,10 @@ static int exynos_drm_attach_lcd_bridge(struct exynos_dp_device *dp,
> >>       if (find_bridge("nxp,ptn3460", &bridge)) {
> >>               bridge_chain = ptn3460_init(dp->drm_dev, encoder, bridge.client,
> >>                                                               bridge.node);
> >> +     } else if (find_bridge("parade,ps8622", &bridge) ||
> >> +                             find_bridge("parade,ps8625", &bridge)) {
> >> +             bridge_chain = ps8622_init(dp->drm_dev, encoder, bridge.client,
> >> +                                                             bridge.node);
> >>       }
> >
> > We really ought to be adding some sort of registry at some point.
> > Otherwise every driver that wants to use bridges needs to come up with a
> > similar set of helpers to instantiate them.
> >
> > Also you're making this driver depend on (now) two bridges, whereas it
> > really shouldn't matter which exact types it supports. Bridges should be
> > exposed via a generic interface.
> 
> How about moving out the find_bridge() function into a common header file,
> and also supporting the list of bridge_init declarations in the same file?
> 
> We get bridge chip node from phandle, and then pass the same node
> to find_bridge() which searches the list using of_device_is_compatible()
> to call the appropriate bridge_init function?

That could work, but it's still somewhat unusual and shouldn't be
required. I think we'd be better of with some sort of registry like we
have for panels. That would mean that a driver that wants to use a
bridge would do something like this:

	struct drm_bridge *bridge;
	struct device_node *np;

	np = of_parse_phandle(dev->of_node, "bridge", 0);
	if (np) {
		bridge = of_drm_find_bridge(np);
		of_node_put(np);

		if (!bridge)
			return -EPROBE_DEFER;
	}

An alternative way would be to add a non-OF wrapper around this, like
this for example:

	bridge = drm_bridge_get(dev, NULL);

Which would be conceptually the same as clk_get() or regulator_get() and
could be easily extended to support non-DT setups as well.

As for bridge drivers I think we may have to rework things a little, so
that a driver can call some sequence like this:

	struct foo_bridge {
		struct drm_bridge base;
		...
	};

	static const struct drm_bridge_funcs foo_bridge_funcs = {
		...
	};

	static int foo_probe(...)
	{
		struct foo_bridge *bridge;
		int err;

		bridge = devm_kzalloc(dev, sizeof(*bridge), GFP_KERNEL);
		if (!bridge)
			return -ENOMEM;

		/* setup bridge (return -EPROBE_DEFER if necessary, ...) */

		/* register bridge with DRM */
		drm_bridge_init(&bridge->base);
		bridge->base.dev = dev;
		bridge->base.funcs = &foo_bridge_funcs;

		err = drm_bridge_add(&bridge->base);
		if (err < 0)
			return err;

		dev_set_drvdata(dev, bridge);
		...
	}

drm_bridge_add() would add the bridge to a global list of bridge devices
which drm_bridge_get()/of_drm_find_bridge() can use to find the one that
it needs. The above has the big advantage that it's completely
independent of the underlying bus that the bridge is on. It could be I2C
or SPI, platform device or PCI device.

Thierry

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

Re: [RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver

[Ajay kumar]

Hi Thierry,

On Mon, Jul 21, 2014 at 6:24 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Jul 21, 2014 at 04:58:25PM +0530, Ajay kumar wrote:
>> On Mon, Jul 21, 2014 at 12:40 PM, Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>> > On Fri, Jul 18, 2014 at 02:13:58AM +0530, Ajay Kumar wrote:
>> >> From: Rahul Sharma <Rahul.Sharma@samsung.com>
>> >>
>> >> This patch adds ps8622 lvds bridge discovery code to the dp driver.
>> >>
>> >> Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>
>> >> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>> >> ---
>> >>  drivers/gpu/drm/exynos/exynos_dp_core.c |    5 +++++
>> >>  1 file changed, 5 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
>> >> index 0ca6256..82e2942 100644
>> >> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
>> >> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
>> >> @@ -31,6 +31,7 @@
>> >>  #include <drm/drm_panel.h>
>> >>  #include <drm/bridge/ptn3460.h>
>> >>  #include <drm/bridge/panel_binder.h>
>> >> +#include <drm/bridge/ps8622.h>
>> >>
>> >>  #include "exynos_drm_drv.h"
>> >>  #include "exynos_dp_core.h"
>> >> @@ -999,6 +1000,10 @@ static int exynos_drm_attach_lcd_bridge(struct exynos_dp_device *dp,
>> >>       if (find_bridge("nxp,ptn3460", &bridge)) {
>> >>               bridge_chain = ptn3460_init(dp->drm_dev, encoder, bridge.client,
>> >>                                                               bridge.node);
>> >> +     } else if (find_bridge("parade,ps8622", &bridge) ||
>> >> +                             find_bridge("parade,ps8625", &bridge)) {
>> >> +             bridge_chain = ps8622_init(dp->drm_dev, encoder, bridge.client,
>> >> +                                                             bridge.node);
>> >>       }
>> >
>> > We really ought to be adding some sort of registry at some point.
>> > Otherwise every driver that wants to use bridges needs to come up with a
>> > similar set of helpers to instantiate them.
>> >
>> > Also you're making this driver depend on (now) two bridges, whereas it
>> > really shouldn't matter which exact types it supports. Bridges should be
>> > exposed via a generic interface.
>>
>> How about moving out the find_bridge() function into a common header file,
>> and also supporting the list of bridge_init declarations in the same file?
>>
>> We get bridge chip node from phandle, and then pass the same node
>> to find_bridge() which searches the list using of_device_is_compatible()
>> to call the appropriate bridge_init function?
>
> That could work, but it's still somewhat unusual and shouldn't be
> required. I think we'd be better of with some sort of registry like we
> have for panels. That would mean that a driver that wants to use a
> bridge would do something like this:
>
>         struct drm_bridge *bridge;
>         struct device_node *np;
>
>         np = of_parse_phandle(dev->of_node, "bridge", 0);
>         if (np) {
>                 bridge = of_drm_find_bridge(np);
>                 of_node_put(np);
>
>                 if (!bridge)
>                         return -EPROBE_DEFER;
>         }
>
> An alternative way would be to add a non-OF wrapper around this, like
> this for example:
Let me try the DT version first :)

>         bridge = drm_bridge_get(dev, NULL);
>
> Which would be conceptually the same as clk_get() or regulator_get() and
> could be easily extended to support non-DT setups as well.
>
> As for bridge drivers I think we may have to rework things a little, so
> that a driver can call some sequence like this:
>
>         struct foo_bridge {
>                 struct drm_bridge base;
>                 ...
>         };
>
>         static const struct drm_bridge_funcs foo_bridge_funcs = {
>                 ...
>         };
>
>         static int foo_probe(...)
>         {
>                 struct foo_bridge *bridge;
>                 int err;
>
>                 bridge = devm_kzalloc(dev, sizeof(*bridge), GFP_KERNEL);
>                 if (!bridge)
>                         return -ENOMEM;
>
>                 /* setup bridge (return -EPROBE_DEFER if necessary, ...) */
>
>                 /* register bridge with DRM */
>                 drm_bridge_init(&bridge->base);
>                 bridge->base.dev = dev;
>                 bridge->base.funcs = &foo_bridge_funcs;
>
>                 err = drm_bridge_add(&bridge->base);
>                 if (err < 0)
>                         return err;
>
>                 dev_set_drvdata(dev, bridge);
>                 ...
>         }
>
> drm_bridge_add() would add the bridge to a global list of bridge devices
> which drm_bridge_get()/of_drm_find_bridge() can use to find the one that
> it needs. The above has the big advantage that it'sdev->mode_config.bridge_list completely
> independent of the underlying bus that the bridge is on. It could be I2C
> or SPI, platform device or PCI device.
>
> Thierry
Ok. This is all about making the bridge driver confine to the driver model.
But, I would need a drm_device pointer to register the bridge to DRM core.
How do I get it?

Ajay

Re: [RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver

[Thierry Reding]

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

On Mon, Jul 21, 2014 at 08:06:01PM +0530, Ajay kumar wrote:
> Hi Thierry,
> 
> On Mon, Jul 21, 2014 at 6:24 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Mon, Jul 21, 2014 at 04:58:25PM +0530, Ajay kumar wrote:
> >> On Mon, Jul 21, 2014 at 12:40 PM, Thierry Reding
> >> <thierry.reding@gmail.com> wrote:
> >> > On Fri, Jul 18, 2014 at 02:13:58AM +0530, Ajay Kumar wrote:
> >> >> From: Rahul Sharma <Rahul.Sharma@samsung.com>
> >> >>
> >> >> This patch adds ps8622 lvds bridge discovery code to the dp driver.
> >> >>
> >> >> Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>
> >> >> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> >> >> ---
> >> >>  drivers/gpu/drm/exynos/exynos_dp_core.c |    5 +++++
> >> >>  1 file changed, 5 insertions(+)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
> >> >> index 0ca6256..82e2942 100644
> >> >> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
> >> >> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
> >> >> @@ -31,6 +31,7 @@
> >> >>  #include <drm/drm_panel.h>
> >> >>  #include <drm/bridge/ptn3460.h>
> >> >>  #include <drm/bridge/panel_binder.h>
> >> >> +#include <drm/bridge/ps8622.h>
> >> >>
> >> >>  #include "exynos_drm_drv.h"
> >> >>  #include "exynos_dp_core.h"
> >> >> @@ -999,6 +1000,10 @@ static int exynos_drm_attach_lcd_bridge(struct exynos_dp_device *dp,
> >> >>       if (find_bridge("nxp,ptn3460", &bridge)) {
> >> >>               bridge_chain = ptn3460_init(dp->drm_dev, encoder, bridge.client,
> >> >>                                                               bridge.node);
> >> >> +     } else if (find_bridge("parade,ps8622", &bridge) ||
> >> >> +                             find_bridge("parade,ps8625", &bridge)) {
> >> >> +             bridge_chain = ps8622_init(dp->drm_dev, encoder, bridge.client,
> >> >> +                                                             bridge.node);
> >> >>       }
> >> >
> >> > We really ought to be adding some sort of registry at some point.
> >> > Otherwise every driver that wants to use bridges needs to come up with a
> >> > similar set of helpers to instantiate them.
> >> >
> >> > Also you're making this driver depend on (now) two bridges, whereas it
> >> > really shouldn't matter which exact types it supports. Bridges should be
> >> > exposed via a generic interface.
> >>
> >> How about moving out the find_bridge() function into a common header file,
> >> and also supporting the list of bridge_init declarations in the same file?
> >>
> >> We get bridge chip node from phandle, and then pass the same node
> >> to find_bridge() which searches the list using of_device_is_compatible()
> >> to call the appropriate bridge_init function?
> >
> > That could work, but it's still somewhat unusual and shouldn't be
> > required. I think we'd be better of with some sort of registry like we
> > have for panels. That would mean that a driver that wants to use a
> > bridge would do something like this:
> >
> >         struct drm_bridge *bridge;
> >         struct device_node *np;
> >
> >         np = of_parse_phandle(dev->of_node, "bridge", 0);
> >         if (np) {
> >                 bridge = of_drm_find_bridge(np);
> >                 of_node_put(np);
> >
> >                 if (!bridge)
> >                         return -EPROBE_DEFER;
> >         }
> >
> > An alternative way would be to add a non-OF wrapper around this, like
> > this for example:
> Let me try the DT version first :)
> 
> >         bridge = drm_bridge_get(dev, NULL);
> >
> > Which would be conceptually the same as clk_get() or regulator_get() and
> > could be easily extended to support non-DT setups as well.
> >
> > As for bridge drivers I think we may have to rework things a little, so
> > that a driver can call some sequence like this:
> >
> >         struct foo_bridge {
> >                 struct drm_bridge base;
> >                 ...
> >         };
> >
> >         static const struct drm_bridge_funcs foo_bridge_funcs = {
> >                 ...
> >         };
> >
> >         static int foo_probe(...)
> >         {
> >                 struct foo_bridge *bridge;
> >                 int err;
> >
> >                 bridge = devm_kzalloc(dev, sizeof(*bridge), GFP_KERNEL);
> >                 if (!bridge)
> >                         return -ENOMEM;
> >
> >                 /* setup bridge (return -EPROBE_DEFER if necessary, ...) */
> >
> >                 /* register bridge with DRM */
> >                 drm_bridge_init(&bridge->base);
> >                 bridge->base.dev = dev;
> >                 bridge->base.funcs = &foo_bridge_funcs;
> >
> >                 err = drm_bridge_add(&bridge->base);
> >                 if (err < 0)
> >                         return err;
> >
> >                 dev_set_drvdata(dev, bridge);
> >                 ...
> >         }
> >
> > drm_bridge_add() would add the bridge to a global list of bridge devices
> > which drm_bridge_get()/of_drm_find_bridge() can use to find the one that
> > it needs. The above has the big advantage that it'sdev->mode_config.bridge_list completely
> > independent of the underlying bus that the bridge is on. It could be I2C
> > or SPI, platform device or PCI device.
> >
> > Thierry
> Ok. This is all about making the bridge driver confine to the driver model.
> But, I would need a drm_device pointer to register the bridge to DRM core.
> How do I get it?

Once you've obtained a reference to the DRM bridge from your driver you
should carry it around until you've set up the DRM device. Then you can
hook it up, which possibly requires a new function (since it's what the
drm_bridge_init() used to do).

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: [RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver

[Ajay kumar]

On Mon, Jul 21, 2014 at 8:14 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Jul 21, 2014 at 08:06:01PM +0530, Ajay kumar wrote:
>> Hi Thierry,
>>
>> On Mon, Jul 21, 2014 at 6:24 PM, Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>> > On Mon, Jul 21, 2014 at 04:58:25PM +0530, Ajay kumar wrote:
>> >> On Mon, Jul 21, 2014 at 12:40 PM, Thierry Reding
>> >> <thierry.reding@gmail.com> wrote:
>> >> > On Fri, Jul 18, 2014 at 02:13:58AM +0530, Ajay Kumar wrote:
>> >> >> From: Rahul Sharma <Rahul.Sharma@samsung.com>
>> >> >>
>> >> >> This patch adds ps8622 lvds bridge discovery code to the dp driver.
>> >> >>
>> >> >> Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>
>> >> >> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>> >> >> ---
>> >> >>  drivers/gpu/drm/exynos/exynos_dp_core.c |    5 +++++
>> >> >>  1 file changed, 5 insertions(+)
>> >> >>
>> >> >> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
>> >> >> index 0ca6256..82e2942 100644
>> >> >> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
>> >> >> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
>> >> >> @@ -31,6 +31,7 @@
>> >> >>  #include <drm/drm_panel.h>
>> >> >>  #include <drm/bridge/ptn3460.h>
>> >> >>  #include <drm/bridge/panel_binder.h>
>> >> >> +#include <drm/bridge/ps8622.h>
>> >> >>
>> >> >>  #include "exynos_drm_drv.h"
>> >> >>  #include "exynos_dp_core.h"
>> >> >> @@ -999,6 +1000,10 @@ static int exynos_drm_attach_lcd_bridge(struct exynos_dp_device *dp,
>> >> >>       if (find_bridge("nxp,ptn3460", &bridge)) {
>> >> >>               bridge_chain = ptn3460_init(dp->drm_dev, encoder, bridge.client,
>> >> >>                                                               bridge.node);
>> >> >> +     } else if (find_bridge("parade,ps8622", &bridge) ||
>> >> >> +                             find_bridge("parade,ps8625", &bridge)) {
>> >> >> +             bridge_chain = ps8622_init(dp->drm_dev, encoder, bridge.client,
>> >> >> +                                                             bridge.node);
>> >> >>       }
>> >> >
>> >> > We really ought to be adding some sort of registry at some point.
>> >> > Otherwise every driver that wants to use bridges needs to come up with a
>> >> > similar set of helpers to instantiate them.
>> >> >
>> >> > Also you're making this driver depend on (now) two bridges, whereas it
>> >> > really shouldn't matter which exact types it supports. Bridges should be
>> >> > exposed via a generic interface.
>> >>
>> >> How about moving out the find_bridge() function into a common header file,
>> >> and also supporting the list of bridge_init declarations in the same file?
>> >>
>> >> We get bridge chip node from phandle, and then pass the same node
>> >> to find_bridge() which searches the list using of_device_is_compatible()
>> >> to call the appropriate bridge_init function?
>> >
>> > That could work, but it's still somewhat unusual and shouldn't be
>> > required. I think we'd be better of with some sort of registry like we
>> > have for panels. That would mean that a driver that wants to use a
>> > bridge would do something like this:
>> >
>> >         struct drm_bridge *bridge;
>> >         struct device_node *np;
>> >
>> >         np = of_parse_phandle(dev->of_node, "bridge", 0);
>> >         if (np) {
>> >                 bridge = of_drm_find_bridge(np);
>> >                 of_node_put(np);
>> >
>> >                 if (!bridge)
>> >                         return -EPROBE_DEFER;
>> >         }
>> >
>> > An alternative way would be to add a non-OF wrapper around this, like
>> > this for example:
>> Let me try the DT version first :)
>>
>> >         bridge = drm_bridge_get(dev, NULL);
>> >
>> > Which would be conceptually the same as clk_get() or regulator_get() and
>> > could be easily extended to support non-DT setups as well.
>> >
>> > As for bridge drivers I think we may have to rework things a little, so
>> > that a driver can call some sequence like this:
>> >
>> >         struct foo_bridge {
>> >                 struct drm_bridge base;
>> >                 ...
>> >         };
>> >
>> >         static const struct drm_bridge_funcs foo_bridge_funcs = {
>> >                 ...
>> >         };
>> >
>> >         static int foo_probe(...)
>> >         {
>> >                 struct foo_bridge *bridge;
>> >                 int err;
>> >
>> >                 bridge = devm_kzalloc(dev, sizeof(*bridge), GFP_KERNEL);
>> >                 if (!bridge)
>> >                         return -ENOMEM;
>> >
>> >                 /* setup bridge (return -EPROBE_DEFER if necessary, ...) */
>> >
>> >                 /* register bridge with DRM */
>> >                 drm_bridge_init(&bridge->base);
>> >                 bridge->base.dev = dev;
>> >                 bridge->base.funcs = &foo_bridge_funcs;
>> >
>> >                 err = drm_bridge_add(&bridge->base);
>> >                 if (err < 0)
>> >                         return err;
>> >
>> >                 dev_set_drvdata(dev, bridge);
>> >                 ...
>> >         }
>> >
>> > drm_bridge_add() would add the bridge to a global list of bridge devices
>> > which drm_bridge_get()/of_drm_find_bridge() can use to find the one that
>> > it needs. The above has the big advantage that it'sdev->mode_config.bridge_list completely
>> > independent of the underlying bus that the bridge is on. It could be I2C
>> > or SPI, platform device or PCI device.
>> >
>> > Thierry
>> Ok. This is all about making the bridge driver confine to the driver model.
>> But, I would need a drm_device pointer to register the bridge to DRM core.
>> How do I get it?
>
> Once you've obtained a reference to the DRM bridge from your driver you
> should carry it around until you've set up the DRM device. Then you can
> hook it up, which possibly requires a new function (since it's what the
> drm_bridge_init() used to do).

Ok. We can have 2 parts:

Non-DRM part:
In order to get a reference for drm_bridge in my encoder driver, I
should be using
of_drm_find_bridge(). In order to do that, I should have already added
the bridge
into a static list of bridges(using something like drm_bridge_add).
Also, bridge_funcs pointer is assigned to the bridge object in the
bridge driver itself.

DRM part:
Assuming that the bridge driver probe will take care of calling
drm_bridge_add(),
I can then call drm_bridge_init from encoder driver, with a drm_bridge pointer
and a drm_device pointer, which will actually register the bridge into DRM core.

Ajay

Re: [RESEND PATCH V5 08/12] drm/bridge: ptn3460: Support bridge chaining

[Ajay kumar]

On Mon, Jul 21, 2014 at 6:10 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Jul 21, 2014 at 05:28:13PM +0530, Ajay kumar wrote:
>> Hi Thierry,
>>
>> On Mon, Jul 21, 2014 at 1:52 PM, Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>> > On Fri, Jul 18, 2014 at 02:13:54AM +0530, Ajay Kumar wrote:
>> > [...]
>> >> Also, remove the drm_connector implementation from ptn3460,
>> >> since the same is implemented using panel_binder.
>> >
>> > I think that's a step backwards. In fact I think the panel-bridge binder
>> > driver shouldn't be needed at all. At least not for now. We have a very
>> > limited number of bridge drivers, so it shouldn't hurt at this stage to
>> > implement the connector instantiation within each driver. Once we have
>> > more bridge drivers, and therefore a better understanding of what they
>> > need, we can always add something like the panel-binder (though I think
>> > it should be library code, similar to the drm_encoder_helper_add() API,
>> > rather than this kind of self-contained object).
>> panel_binder was needed to wrap around panel as a bridge, and this was in turn
>> needed because people wanted to represent a bridge+panel combo as a chain
>> of bridges.
>> So, if we choose to drop panel_binder, we choose to drop the idea of chaining
>> the bridges, and end up calling drm_panel functions directly from
>> individual bridges.
>> And, the bridge will implement the connector functions as you suggested.
>> I am okay with this, if Daniel/Rob don't have an issue with this.
>
> I think bridge chaining and panel handling are separate issues. That's
> why I think we shouldn't mix them like this. From earlier discussion[0]
> the conclusion was that the final element in the chain should implement
> a connector (with the appropriate type). Often that last element would
> be an encoder (when there are no bridges). Sometimes the last element
> would be a bridge. It's logical for that same element to also implement
> the panel integration since it's closely tied to the connector.
>
> Thierry
>
> [0]: http://lists.freedesktop.org/archives/dri-devel/2014-May/059685.html
Going with Thierry's opinion, if the bridge is allowed to do panel integration,
there is no need for a bridge_chain. I mean a bridge_chain doesn't exist in
my case at all. I have just one bridge which integrates a panel.
Is it really necessary to keep next_bridge pointer and other helpers?

Ajay

Re: [RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver

[Thierry Reding]

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

On Tue, Jul 22, 2014 at 11:35:52AM +0530, Ajay kumar wrote:
> On Mon, Jul 21, 2014 at 8:14 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Mon, Jul 21, 2014 at 08:06:01PM +0530, Ajay kumar wrote:
> >> Hi Thierry,
> >>
> >> On Mon, Jul 21, 2014 at 6:24 PM, Thierry Reding
> >> <thierry.reding@gmail.com> wrote:
> >> > On Mon, Jul 21, 2014 at 04:58:25PM +0530, Ajay kumar wrote:
> >> >> On Mon, Jul 21, 2014 at 12:40 PM, Thierry Reding
> >> >> <thierry.reding@gmail.com> wrote:
> >> >> > On Fri, Jul 18, 2014 at 02:13:58AM +0530, Ajay Kumar wrote:
> >> >> >> From: Rahul Sharma <Rahul.Sharma@samsung.com>
> >> >> >>
> >> >> >> This patch adds ps8622 lvds bridge discovery code to the dp driver.
> >> >> >>
> >> >> >> Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>
> >> >> >> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> >> >> >> ---
> >> >> >>  drivers/gpu/drm/exynos/exynos_dp_core.c |    5 +++++
> >> >> >>  1 file changed, 5 insertions(+)
> >> >> >>
> >> >> >> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
> >> >> >> index 0ca6256..82e2942 100644
> >> >> >> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
> >> >> >> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
> >> >> >> @@ -31,6 +31,7 @@
> >> >> >>  #include <drm/drm_panel.h>
> >> >> >>  #include <drm/bridge/ptn3460.h>
> >> >> >>  #include <drm/bridge/panel_binder.h>
> >> >> >> +#include <drm/bridge/ps8622.h>
> >> >> >>
> >> >> >>  #include "exynos_drm_drv.h"
> >> >> >>  #include "exynos_dp_core.h"
> >> >> >> @@ -999,6 +1000,10 @@ static int exynos_drm_attach_lcd_bridge(struct exynos_dp_device *dp,
> >> >> >>       if (find_bridge("nxp,ptn3460", &bridge)) {
> >> >> >>               bridge_chain = ptn3460_init(dp->drm_dev, encoder, bridge.client,
> >> >> >>                                                               bridge.node);
> >> >> >> +     } else if (find_bridge("parade,ps8622", &bridge) ||
> >> >> >> +                             find_bridge("parade,ps8625", &bridge)) {
> >> >> >> +             bridge_chain = ps8622_init(dp->drm_dev, encoder, bridge.client,
> >> >> >> +                                                             bridge.node);
> >> >> >>       }
> >> >> >
> >> >> > We really ought to be adding some sort of registry at some point.
> >> >> > Otherwise every driver that wants to use bridges needs to come up with a
> >> >> > similar set of helpers to instantiate them.
> >> >> >
> >> >> > Also you're making this driver depend on (now) two bridges, whereas it
> >> >> > really shouldn't matter which exact types it supports. Bridges should be
> >> >> > exposed via a generic interface.
> >> >>
> >> >> How about moving out the find_bridge() function into a common header file,
> >> >> and also supporting the list of bridge_init declarations in the same file?
> >> >>
> >> >> We get bridge chip node from phandle, and then pass the same node
> >> >> to find_bridge() which searches the list using of_device_is_compatible()
> >> >> to call the appropriate bridge_init function?
> >> >
> >> > That could work, but it's still somewhat unusual and shouldn't be
> >> > required. I think we'd be better of with some sort of registry like we
> >> > have for panels. That would mean that a driver that wants to use a
> >> > bridge would do something like this:
> >> >
> >> >         struct drm_bridge *bridge;
> >> >         struct device_node *np;
> >> >
> >> >         np = of_parse_phandle(dev->of_node, "bridge", 0);
> >> >         if (np) {
> >> >                 bridge = of_drm_find_bridge(np);
> >> >                 of_node_put(np);
> >> >
> >> >                 if (!bridge)
> >> >                         return -EPROBE_DEFER;
> >> >         }
> >> >
> >> > An alternative way would be to add a non-OF wrapper around this, like
> >> > this for example:
> >> Let me try the DT version first :)
> >>
> >> >         bridge = drm_bridge_get(dev, NULL);
> >> >
> >> > Which would be conceptually the same as clk_get() or regulator_get() and
> >> > could be easily extended to support non-DT setups as well.
> >> >
> >> > As for bridge drivers I think we may have to rework things a little, so
> >> > that a driver can call some sequence like this:
> >> >
> >> >         struct foo_bridge {
> >> >                 struct drm_bridge base;
> >> >                 ...
> >> >         };
> >> >
> >> >         static const struct drm_bridge_funcs foo_bridge_funcs = {
> >> >                 ...
> >> >         };
> >> >
> >> >         static int foo_probe(...)
> >> >         {
> >> >                 struct foo_bridge *bridge;
> >> >                 int err;
> >> >
> >> >                 bridge = devm_kzalloc(dev, sizeof(*bridge), GFP_KERNEL);
> >> >                 if (!bridge)
> >> >                         return -ENOMEM;
> >> >
> >> >                 /* setup bridge (return -EPROBE_DEFER if necessary, ...) */
> >> >
> >> >                 /* register bridge with DRM */
> >> >                 drm_bridge_init(&bridge->base);
> >> >                 bridge->base.dev = dev;
> >> >                 bridge->base.funcs = &foo_bridge_funcs;
> >> >
> >> >                 err = drm_bridge_add(&bridge->base);
> >> >                 if (err < 0)
> >> >                         return err;
> >> >
> >> >                 dev_set_drvdata(dev, bridge);
> >> >                 ...
> >> >         }
> >> >
> >> > drm_bridge_add() would add the bridge to a global list of bridge devices
> >> > which drm_bridge_get()/of_drm_find_bridge() can use to find the one that
> >> > it needs. The above has the big advantage that it'sdev->mode_config.bridge_list completely
> >> > independent of the underlying bus that the bridge is on. It could be I2C
> >> > or SPI, platform device or PCI device.
> >> >
> >> > Thierry
> >> Ok. This is all about making the bridge driver confine to the driver model.
> >> But, I would need a drm_device pointer to register the bridge to DRM core.
> >> How do I get it?
> >
> > Once you've obtained a reference to the DRM bridge from your driver you
> > should carry it around until you've set up the DRM device. Then you can
> > hook it up, which possibly requires a new function (since it's what the
> > drm_bridge_init() used to do).
> 
> Ok. We can have 2 parts:
> 
> Non-DRM part:
> In order to get a reference for drm_bridge in my encoder driver, I
> should be using
> of_drm_find_bridge(). In order to do that, I should have already added
> the bridge
> into a static list of bridges(using something like drm_bridge_add).
> Also, bridge_funcs pointer is assigned to the bridge object in the
> bridge driver itself.
> 
> DRM part:
> Assuming that the bridge driver probe will take care of calling
> drm_bridge_add(),
> I can then call drm_bridge_init from encoder driver, with a drm_bridge pointer
> and a drm_device pointer, which will actually register the bridge into DRM core.

Yes, exactly.

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: [RESEND PATCH V5 01/12] drm/exynos: Move DP setup out of hotplug workqueue

[Sean Paul]

On Thu, Jul 17, 2014 at 4:43 PM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
> Move the DP training and video enable from the hotplug handler into
> a seperate function and call the same during dpms ON.
>
> With existing code, DP HPD should be generated just few ms before
> calling enable_irq in dp_poweron.
>
> This patch removes that stringent time constraint.
>
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>


This looks eerily familiar to:
https://chromium-review.googlesource.com/#/c/65782/

In fact, I think it's probably better to do this in commit, rather than poweron.

Sean


> ---
>  drivers/gpu/drm/exynos/exynos_dp_core.c |   11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
> index 845d766..a94b114 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
> @@ -875,10 +875,18 @@ static irqreturn_t exynos_dp_irq_handler(int irq, void *arg)
>  static void exynos_dp_hotplug(struct work_struct *work)
>  {
>         struct exynos_dp_device *dp;
> -       int ret;
>
>         dp = container_of(work, struct exynos_dp_device, hotplug_work);
>
> +       if (dp->drm_dev)
> +               drm_helper_hpd_irq_event(dp->drm_dev);
> +}
> +
> +static void exynos_dp_setup(void *in_ctx)
> +{
> +       struct exynos_dp_device *dp = in_ctx;
> +       int ret;
> +
>         ret = exynos_dp_detect_hpd(dp);
>         if (ret) {
>                 /* Cable has been disconnected, we're done */
> @@ -1059,6 +1067,7 @@ static void exynos_dp_poweron(struct exynos_dp_device *dp)
>         exynos_dp_phy_init(dp);
>         exynos_dp_init_dp(dp);
>         enable_irq(dp->irq);
> +       exynos_dp_setup(dp);
>  }
>
>  static void exynos_dp_poweroff(struct exynos_dp_device *dp)
> --
> 1.7.9.5
>

Re: [RESEND PATCH V5 01/12] drm/exynos: Move DP setup out of hotplug workqueue

[Ajay kumar]

Sean,

On Tue, Jul 22, 2014 at 8:29 PM, Sean Paul <seanpaul@google.com> wrote:
> On Thu, Jul 17, 2014 at 4:43 PM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
>> Move the DP training and video enable from the hotplug handler into
>> a seperate function and call the same during dpms ON.
>>
>> With existing code, DP HPD should be generated just few ms before
>> calling enable_irq in dp_poweron.
>>
>> This patch removes that stringent time constraint.
>>
>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>
>
> This looks eerily familiar to:
> https://chromium-review.googlesource.com/#/c/65782/
>
> In fact, I think it's probably better to do this in commit, rather than poweron.
>
> Sean
Your are right. This patch contains a subset of changes from the patch
you have mentioned.
But, If I provide commit calback to dp, then it would cause dp to
reinitialize twice
in quick successions - during dpms_on and during commit.
For the user, it would look like a glitch. So, I chose not to provide
a commit callback.

Ajay
>
>> ---
>>  drivers/gpu/drm/exynos/exynos_dp_core.c |   11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
>> index 845d766..a94b114 100644
>> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
>> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
>> @@ -875,10 +875,18 @@ static irqreturn_t exynos_dp_irq_handler(int irq, void *arg)
>>  static void exynos_dp_hotplug(struct work_struct *work)
>>  {
>>         struct exynos_dp_device *dp;
>> -       int ret;
>>
>>         dp = container_of(work, struct exynos_dp_device, hotplug_work);
>>
>> +       if (dp->drm_dev)
>> +               drm_helper_hpd_irq_event(dp->drm_dev);
>> +}
>> +
>> +static void exynos_dp_setup(void *in_ctx)
>> +{
>> +       struct exynos_dp_device *dp = in_ctx;
>> +       int ret;
>> +
>>         ret = exynos_dp_detect_hpd(dp);
>>         if (ret) {
>>                 /* Cable has been disconnected, we're done */
>> @@ -1059,6 +1067,7 @@ static void exynos_dp_poweron(struct exynos_dp_device *dp)
>>         exynos_dp_phy_init(dp);
>>         exynos_dp_init_dp(dp);
>>         enable_irq(dp->irq);
>> +       exynos_dp_setup(dp);
>>  }
>>
>>  static void exynos_dp_poweroff(struct exynos_dp_device *dp)
>> --
>> 1.7.9.5
>>

Re: [RESEND PATCH V5 01/12] drm/exynos: Move DP setup out of hotplug workqueue

[Sean Paul]

On Wed, Jul 23, 2014 at 7:22 AM, Ajay kumar <ajaynumb@gmail.com> wrote:
> Sean,
>
> On Tue, Jul 22, 2014 at 8:29 PM, Sean Paul <seanpaul@google.com> wrote:
>> On Thu, Jul 17, 2014 at 4:43 PM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
>>> Move the DP training and video enable from the hotplug handler into
>>> a seperate function and call the same during dpms ON.
>>>
>>> With existing code, DP HPD should be generated just few ms before
>>> calling enable_irq in dp_poweron.
>>>
>>> This patch removes that stringent time constraint.
>>>
>>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>>
>>
>> This looks eerily familiar to:
>> https://chromium-review.googlesource.com/#/c/65782/
>>
>> In fact, I think it's probably better to do this in commit, rather than poweron.
>>
>> Sean
> Your are right. This patch contains a subset of changes from the patch
> you have mentioned.
> But, If I provide commit calback to dp, then it would cause dp to
> reinitialize twice
> in quick successions - during dpms_on and during commit.
> For the user, it would look like a glitch. So, I chose not to provide
> a commit callback.
>

Interesting. At least in my testing, the absence of the commit
callback causes my simple test app (which just does drmModeSetCrtc) to
fail. In this case, by fail, I mean that the screen just shows black.

Sean

> Ajay
>>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_dp_core.c |   11 ++++++++++-
>>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
>>> index 845d766..a94b114 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
>>> @@ -875,10 +875,18 @@ static irqreturn_t exynos_dp_irq_handler(int irq, void *arg)
>>>  static void exynos_dp_hotplug(struct work_struct *work)
>>>  {
>>>         struct exynos_dp_device *dp;
>>> -       int ret;
>>>
>>>         dp = container_of(work, struct exynos_dp_device, hotplug_work);
>>>
>>> +       if (dp->drm_dev)
>>> +               drm_helper_hpd_irq_event(dp->drm_dev);
>>> +}
>>> +
>>> +static void exynos_dp_setup(void *in_ctx)
>>> +{
>>> +       struct exynos_dp_device *dp = in_ctx;
>>> +       int ret;
>>> +
>>>         ret = exynos_dp_detect_hpd(dp);
>>>         if (ret) {
>>>                 /* Cable has been disconnected, we're done */
>>> @@ -1059,6 +1067,7 @@ static void exynos_dp_poweron(struct exynos_dp_device *dp)
>>>         exynos_dp_phy_init(dp);
>>>         exynos_dp_init_dp(dp);
>>>         enable_irq(dp->irq);
>>> +       exynos_dp_setup(dp);
>>>  }
>>>
>>>  static void exynos_dp_poweroff(struct exynos_dp_device *dp)
>>> --
>>> 1.7.9.5
>>>

Re: [RESEND PATCH V5 01/12] drm/exynos: Move DP setup out of hotplug workqueue

[Ajay kumar]

On Wed, Jul 23, 2014 at 8:12 PM, Sean Paul <seanpaul@google.com> wrote:
> On Wed, Jul 23, 2014 at 7:22 AM, Ajay kumar <ajaynumb@gmail.com> wrote:
>> Sean,
>>
>> On Tue, Jul 22, 2014 at 8:29 PM, Sean Paul <seanpaul@google.com> wrote:
>>> On Thu, Jul 17, 2014 at 4:43 PM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
>>>> Move the DP training and video enable from the hotplug handler into
>>>> a seperate function and call the same during dpms ON.
>>>>
>>>> With existing code, DP HPD should be generated just few ms before
>>>> calling enable_irq in dp_poweron.
>>>>
>>>> This patch removes that stringent time constraint.
>>>>
>>>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>>>
>>>
>>> This looks eerily familiar to:
>>> https://chromium-review.googlesource.com/#/c/65782/
>>>
>>> In fact, I think it's probably better to do this in commit, rather than poweron.
>>>
>>> Sean
>> Your are right. This patch contains a subset of changes from the patch
>> you have mentioned.
>> But, If I provide commit calback to dp, then it would cause dp to
>> reinitialize twice
>> in quick successions - during dpms_on and during commit.
>> For the user, it would look like a glitch. So, I chose not to provide
>> a commit callback.
>>
>
> Interesting. At least in my testing, the absence of the commit
> callback causes my simple test app (which just does drmModeSetCrtc) to
> fail. In this case, by fail, I mean that the screen just shows black.
>
> Sean
I tried running modetest. It is running fine.
Also, exynos_drm_encoder_commit calls both dpms_on and
commit for the encoder. So, there should be no problem I guess!

Your testapp stops working only after adding this patch?

Ajay

>>>
>>>> ---
>>>>  drivers/gpu/drm/exynos/exynos_dp_core.c |   11 ++++++++++-
>>>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
>>>> index 845d766..a94b114 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
>>>> @@ -875,10 +875,18 @@ static irqreturn_t exynos_dp_irq_handler(int irq, void *arg)
>>>>  static void exynos_dp_hotplug(struct work_struct *work)
>>>>  {
>>>>         struct exynos_dp_device *dp;
>>>> -       int ret;
>>>>
>>>>         dp = container_of(work, struct exynos_dp_device, hotplug_work);
>>>>
>>>> +       if (dp->drm_dev)
>>>> +               drm_helper_hpd_irq_event(dp->drm_dev);
>>>> +}
>>>> +
>>>> +static void exynos_dp_setup(void *in_ctx)
>>>> +{
>>>> +       struct exynos_dp_device *dp = in_ctx;
>>>> +       int ret;
>>>> +
>>>>         ret = exynos_dp_detect_hpd(dp);
>>>>         if (ret) {
>>>>                 /* Cable has been disconnected, we're done */
>>>> @@ -1059,6 +1067,7 @@ static void exynos_dp_poweron(struct exynos_dp_device *dp)
>>>>         exynos_dp_phy_init(dp);
>>>>         exynos_dp_init_dp(dp);
>>>>         enable_irq(dp->irq);
>>>> +       exynos_dp_setup(dp);
>>>>  }
>>>>
>>>>  static void exynos_dp_poweroff(struct exynos_dp_device *dp)
>>>> --
>>>> 1.7.9.5
>>>>

Re: [RESEND PATCH V5 01/12] drm/exynos: Move DP setup out of hotplug workqueue

[Sean Paul]

On Wed, Jul 23, 2014 at 11:18 AM, Ajay kumar <ajaynumb@gmail.com> wrote:
> On Wed, Jul 23, 2014 at 8:12 PM, Sean Paul <seanpaul@google.com> wrote:
>> On Wed, Jul 23, 2014 at 7:22 AM, Ajay kumar <ajaynumb@gmail.com> wrote:
>>> Sean,
>>>
>>> On Tue, Jul 22, 2014 at 8:29 PM, Sean Paul <seanpaul@google.com> wrote:
>>>> On Thu, Jul 17, 2014 at 4:43 PM, Ajay Kumar <ajaykumar.rs@samsung.com> wrote:
>>>>> Move the DP training and video enable from the hotplug handler into
>>>>> a seperate function and call the same during dpms ON.
>>>>>
>>>>> With existing code, DP HPD should be generated just few ms before
>>>>> calling enable_irq in dp_poweron.
>>>>>
>>>>> This patch removes that stringent time constraint.
>>>>>
>>>>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>>>>
>>>>
>>>> This looks eerily familiar to:
>>>> https://chromium-review.googlesource.com/#/c/65782/
>>>>
>>>> In fact, I think it's probably better to do this in commit, rather than poweron.
>>>>
>>>> Sean
>>> Your are right. This patch contains a subset of changes from the patch
>>> you have mentioned.
>>> But, If I provide commit calback to dp, then it would cause dp to
>>> reinitialize twice
>>> in quick successions - during dpms_on and during commit.
>>> For the user, it would look like a glitch. So, I chose not to provide
>>> a commit callback.
>>>
>>
>> Interesting. At least in my testing, the absence of the commit
>> callback causes my simple test app (which just does drmModeSetCrtc) to
>> fail. In this case, by fail, I mean that the screen just shows black.
>>
>> Sean
> I tried running modetest. It is running fine.
> Also, exynos_drm_encoder_commit calls both dpms_on and
> commit for the encoder. So, there should be no problem I guess!
>
> Your testapp stops working only after adding this patch?


Sorry for taking a while to get back to you, I didn't get the chance
to test this until now.

Yes, my testapp stops working after adding this patch (but works with
the patch which does it in commit).

Sean



>
> Ajay
>
>>>>
>>>>> ---
>>>>>  drivers/gpu/drm/exynos/exynos_dp_core.c |   11 ++++++++++-
>>>>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
>>>>> index 845d766..a94b114 100644
>>>>> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
>>>>> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
>>>>> @@ -875,10 +875,18 @@ static irqreturn_t exynos_dp_irq_handler(int irq, void *arg)
>>>>>  static void exynos_dp_hotplug(struct work_struct *work)
>>>>>  {
>>>>>         struct exynos_dp_device *dp;
>>>>> -       int ret;
>>>>>
>>>>>         dp = container_of(work, struct exynos_dp_device, hotplug_work);
>>>>>
>>>>> +       if (dp->drm_dev)
>>>>> +               drm_helper_hpd_irq_event(dp->drm_dev);
>>>>> +}
>>>>> +
>>>>> +static void exynos_dp_setup(void *in_ctx)
>>>>> +{
>>>>> +       struct exynos_dp_device *dp = in_ctx;
>>>>> +       int ret;
>>>>> +
>>>>>         ret = exynos_dp_detect_hpd(dp);
>>>>>         if (ret) {
>>>>>                 /* Cable has been disconnected, we're done */
>>>>> @@ -1059,6 +1067,7 @@ static void exynos_dp_poweron(struct exynos_dp_device *dp)
>>>>>         exynos_dp_phy_init(dp);
>>>>>         exynos_dp_init_dp(dp);
>>>>>         enable_irq(dp->irq);
>>>>> +       exynos_dp_setup(dp);
>>>>>  }
>>>>>
>>>>>  static void exynos_dp_poweroff(struct exynos_dp_device *dp)
>>>>> --
>>>>> 1.7.9.5
>>>>>