[RFC 0/4] Use the Common Display Framework in tegra-drm

This series leverages the (still work-in-progress) Common Display Framework to
add panel support to the tegra-drm driver. It also adds a driver for the
CLAA101WA01A panel used on the Ventana board.

The CDF is a moving target but Tegra needs some sort of display framework and
even in its current state the CDF seems to be the best candidate. Besides, by
using the CDF from now on we hope to provide useful feedback to Laurent and the
other CDF designers.

The changes to tegra-drm are rather minimal. Panels are referenced from Tegra DC
output nodes through the "nvidia,panel" property. This property is looked up
when a display connect notification is received in order to see if it points to
the connected display entity. If it does, the entity is then used for output.

The DPMS states are then propagated to the output entity, which is then supposed
to call back into the set_stream() hook in order to enable/disable the output
stream as needed.

Although the overall design seems to work ok, a few specific issues need to be
addressed:

1) The CDF has a get_modes() hook, but this is already implemented by
tegra_connector_get_modes(). Ideally everything should be moved to the CDF hook,
but Tegra's implementation uses DRM functions to retrieve the EDID and CDF
should, AFAIK, remain DRM-agnostic.

2) There is currently no panel/backlight interaction, e.g. backlight status is
controlled through FB events, independantly from the panel state. It could make
sense to have the panel DT node reference the backlight and control it as part
of its own power on/off sequence. Right now however, a backlight device cannot
ignore FB events.

3) Probably related to 2), now the backlight's power controls are part of the
panel driver, because the pwm-backlight driver cannot control the power
regulator and enable GPIO. This means that the backlight power is not turned off
when its brightness is set to 0 through sysfs. Once again this speaks in favor
of having stronger panel/backlight interaction: for instance, the panel driver
could reference the backlight and hijack its update_status() op to replace it by
one that does the correct power sequencing before calling the original function.
This would require some extra infrastructure though. Another possibility would
be to have a dedicated backlight driver for each panel, with its own
"compatible" string.

The code is based on 3.8rc5 + Steffen's videomode patches and the CDF v2.

Alexandre Courbot (4):
  video: panel: add CLAA101WA01A panel support
  tegra: ventana: add display and backlight DT nodes
  drm: tegra: use the Common Display Framework
  tegra: enable CDF and claa101 panel

 .../bindings/gpu/nvidia,tegra20-host1x.txt         |   2 +
 .../video/display/chunghwa,claa101wa01a.txt        |   8 +
 arch/arm/boot/dts/tegra20-ventana.dts              |  34 +++-
 arch/arm/configs/tegra_defconfig                   |   2 +
 drivers/gpu/drm/tegra/drm.h                        |  16 ++
 drivers/gpu/drm/tegra/output.c                     | 118 +++++++++++-
 drivers/video/display/Kconfig                      |   8 +
 drivers/video/display/Makefile                     |   1 +
 drivers/video/display/panel-claa101wa01a.c         | 209 +++++++++++++++++++++
 9 files changed, 393 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/video/display/chunghwa,claa101wa01a.txt
 create mode 100644 drivers/video/display/panel-claa101wa01a.c

-- 
1.8.1.1

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 arch/arm/boot/dts/tegra20-ventana.dts | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index adc4754..48f4e6d 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts

@@ -10,6 +10,16 @@
 		reg = <0x00000000 0x40000000>;
 	};
 
+	host1x {
+		dc@54200000 {
+			rgb {
+				status = "okay";
+				nvidia,ddc-i2c-bus = <&lcd_ddc>;
+				nvidia,panel = <&panel>;
+			};
+		};
+	};
+
 	pinmux {
 		pinctrl-names = "default";
 		pinctrl-0 = <&state_default>;

@@ -341,7 +351,7 @@
 			#size-cells = <0>;
 		};
 
-		i2c@1 {
+		lcd_ddc: i2c@1 {
 			reg = <1>;
 			#address-cells = <1>;
 			#size-cells = <0>;

@@ -516,6 +526,24 @@
 		bus-width = <8>;
 	};
 
+	backlight: backlight {
+		compatible = "pwm-backlight";
+		pwms = <&pwm 2 5000000>;
+
+		brightness-levels = <0 16 32 48 64 80 96 112 128 144 160 176 192 208 224 240 255>;
+		default-brightness-level = <10>;
+	};
+
+	panel: panel {
+		compatible = "chunghwa,claa101wa01a";
+
+		pnl-supply = <&vdd_pnl_reg>;
+		pnl-enable-gpios = <&gpio 10 0>;
+
+		bl-supply = <&vdd_bl_reg>;
+		bl-enable-gpios = <&gpio 28 0>;
+	};
+
 	regulators {
 		compatible = "simple-bus";
 		#address-cells = <1>;

@@ -549,7 +577,7 @@
 			enable-active-high;
 		};
 
-		regulator@3 {
+		vdd_pnl_reg: regulator@3 {
 			compatible = "regulator-fixed";
 			reg = <3>;
 			regulator-name = "vdd_pnl";

@@ -559,7 +587,7 @@
 			enable-active-high;
 		};
 
-		regulator@4 {
+		vdd_bl_reg: regulator@4 {
 			compatible = "regulator-fixed";
 			reg = <4>;
 			regulator-name = "vdd_bl";

-- 
1.8.1.1

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 arch/arm/configs/tegra_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
index a7827fd..6da0de2 100644
--- a/arch/arm/configs/tegra_defconfig
+++ b/arch/arm/configs/tegra_defconfig

@@ -150,6 +150,8 @@ CONFIG_BACKLIGHT_LCD_SUPPORT=y
 CONFIG_BACKLIGHT_CLASS_DEVICE=y
 # CONFIG_BACKLIGHT_GENERIC is not set
 CONFIG_BACKLIGHT_PWM=y
+CONFIG_DISPLAY_CORE=y
+CONFIG_DISPLAY_PANEL_CLAA101WA01A=y
 CONFIG_FRAMEBUFFER_CONSOLE=y
 CONFIG_LOGO=y
 CONFIG_SOUND=y

-- 
1.8.1.1

Make the tegra-drm driver use the Common Display Framework, letting it
control the panel state according to the DPMS status.

A "nvidia,panel" property is added to the output node of the Tegra DC
that references the panel connected to a given output.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 .../bindings/gpu/nvidia,tegra20-host1x.txt         |   2 +
 drivers/gpu/drm/tegra/drm.h                        |  16 +++
 drivers/gpu/drm/tegra/output.c                     | 118 ++++++++++++++++++++-
 3 files changed, 134 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
index b4fa934..9c65e8e 100644
--- a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
+++ b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt

@@ -67,6 +67,7 @@ of the following host1x client modules:
   - nvidia,ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
   - nvidia,hpd-gpio: specifies a GPIO used for hotplug detection
   - nvidia,edid: supplies a binary EDID blob
+  - nvidia,panel: phandle of a display entity connected to this output
 
 - hdmi: High Definition Multimedia Interface
 

@@ -81,6 +82,7 @@ of the following host1x client modules:
   - nvidia,ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
   - nvidia,hpd-gpio: specifies a GPIO used for hotplug detection
   - nvidia,edid: supplies a binary EDID blob
+  - nvidia,panel: phandle of a display entity connected to this output
 
 - tvo: TV encoder output
 

diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index 741b5dc..5e63c56 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h

@@ -17,6 +17,7 @@
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_fb_cma_helper.h>
 #include <drm/drm_fixed.h>
+#include <video/display.h>
 
 struct tegra_framebuffer {
 	struct drm_framebuffer base;

@@ -147,6 +148,9 @@ struct tegra_output {
 
 	struct drm_encoder encoder;
 	struct drm_connector connector;
+	struct display_entity this;
+	struct display_entity *output;
+	struct display_entity_notifier display_notifier;
 };
 
 static inline struct tegra_output *encoder_to_output(struct drm_encoder *e)

@@ -159,6 +163,18 @@ static inline struct tegra_output *connector_to_output(struct drm_connector *c)
 	return container_of(c, struct tegra_output, connector);
 }
 
+static inline
+struct tegra_output *display_entity_to_output(struct display_entity *e)
+{
+	return container_of(e, struct tegra_output, this);
+}
+
+static inline struct tegra_output *
+display_notifier_to_output(struct display_entity_notifier *n)
+{
+	return container_of(n, struct tegra_output, display_notifier);
+}
+
 static inline int tegra_output_enable(struct tegra_output *output)
 {
 	if (output && output->ops && output->ops->enable)

diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
index 8140fc6..d5bf37a 100644
--- a/drivers/gpu/drm/tegra/output.c
+++ b/drivers/gpu/drm/tegra/output.c

@@ -105,6 +105,29 @@ static const struct drm_encoder_funcs encoder_funcs = {
 
 static void tegra_encoder_dpms(struct drm_encoder *encoder, int mode)
 {
+	struct tegra_output *output = encoder_to_output(encoder);
+	enum display_entity_state state;
+
+	if (!output->output)
+		return;
+
+	switch (mode) {
+	case DRM_MODE_DPMS_ON:
+		state = DISPLAY_ENTITY_STATE_ON;
+		break;
+	case DRM_MODE_DPMS_STANDBY:
+	case DRM_MODE_DPMS_SUSPEND:
+		state = DISPLAY_ENTITY_STATE_STANDBY;
+		break;
+	case DRM_MODE_DPMS_OFF:
+		state = DISPLAY_ENTITY_STATE_OFF;
+		break;
+	default:
+		dev_warn(output->dev, "unknown DPMS state, ignoring\n");
+		return;
+	}
+
+	display_entity_set_state(output->output, state);
 }
 
 static bool tegra_encoder_mode_fixup(struct drm_encoder *encoder,

@@ -129,9 +152,14 @@ static void tegra_encoder_mode_set(struct drm_encoder *encoder,
 	struct tegra_output *output = encoder_to_output(encoder);
 	int err;
 
-	err = tegra_output_enable(output);
+	if (output->output)
+		err = display_entity_set_state(output->output,
+					       DISPLAY_ENTITY_STATE_ON);
+	else
+		err = tegra_output_enable(output);
+
 	if (err < 0)
-		dev_err(encoder->dev->dev, "tegra_output_enable(): %d\n", err);
+		dev_err(encoder->dev->dev, "cannot enable output: %d\n", err);
 }
 
 static const struct drm_encoder_helper_funcs encoder_helper_funcs = {

@@ -185,6 +213,69 @@ int tegra_output_parse_dt(struct tegra_output *output)
 	return 0;
 }
 
+static int display_notify_callback(struct display_entity_notifier *notifier,
+				   struct display_entity *entity, int event)
+{
+	struct tegra_output *output = display_notifier_to_output(notifier);
+	struct device_node *pnode;
+
+	switch (event) {
+	case DISPLAY_ENTITY_NOTIFIER_CONNECT:
+		if (output->output)
+			break;
+
+		pnode = of_parse_phandle(output->of_node, "nvidia,panel", 0);
+		if (!pnode)
+			break;
+
+		if (entity->dev && entity->dev->of_node = pnode) {
+			dev_dbg(output->dev, "connecting panel\n");
+			output->output = display_entity_get(entity);
+			display_entity_connect(&output->this, output->output);
+		}
+		of_node_put(pnode);
+
+		break;
+
+	case DISPLAY_ENTITY_NOTIFIER_DISCONNECT:
+		if (!output->output || output->output != entity)
+			break;
+
+		dev_dbg(output->dev, "disconnecting panel\n");
+		display_entity_disconnect(&output->this, output->output);
+		output->output = NULL;
+		display_entity_put(&output->this);
+
+		break;
+
+	default:
+		dev_dbg(output->dev, "unhandled display event\n");
+		break;
+	}
+
+	return 0;
+}
+
+static int tegra_output_set_stream(struct display_entity *entity,
+				   enum display_entity_stream_state state)
+{
+	struct tegra_output *output = display_entity_to_output(entity);
+
+	switch (state) {
+	case DISPLAY_ENTITY_STATE_OFF:
+	case DISPLAY_ENTITY_STATE_STANDBY:
+		return output->ops->disable(output);
+	case DISPLAY_ENTITY_STATE_ON:
+		return output->ops->enable(output);
+	}
+
+	return 0;
+}
+
+static struct display_entity_video_ops tegra_output_video_ops = {
+	.set_stream = tegra_output_set_stream,
+};
+
 int tegra_output_init(struct drm_device *drm, struct tegra_output *output)
 {
 	int connector, encoder, err;

@@ -250,6 +341,23 @@ int tegra_output_init(struct drm_device *drm, struct tegra_output *output)
 
 	output->encoder.possible_crtcs = 0x3;
 
+	/* register display entity */
+	memset(&output->this, 0, sizeof(output->this));
+	output->this.dev = drm->dev;
+	output->this.ops.video = &tegra_output_video_ops;
+	err = display_entity_register(&output->this);
+	if (err) {
+		dev_err(output->dev, "cannot register display entity\n");
+		return err;
+	}
+
+	/* register display notifier */
+	output->display_notifier.dev = NULL;
+	output->display_notifier.notify = display_notify_callback;
+	err = display_entity_register_notifier(&output->display_notifier);
+	if (err)
+		return err;
+
 	return 0;
 
 free_hpd:

@@ -260,6 +368,12 @@ free_hpd:
 
 int tegra_output_exit(struct tegra_output *output)
 {
+	if (output->output)
+		display_entity_put(output->output);
+
+	display_entity_unregister_notifier(&output->display_notifier);
+	display_entity_unregister(&output->this);
+
 	if (gpio_is_valid(output->hpd_gpio)) {
 		free_irq(output->hpd_irq, output);
 		gpio_free(output->hpd_gpio);

-- 
1.8.1.1

On 01/30/2013 11:02 AM, Alexandre Courbot wrote:

Make the tegra-drm driver use the Common Display Framework, letting it
control the panel state according to the DPMS status.

A "nvidia,panel" property is added to the output node of the Tegra DC
that references the panel connected to a given output.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---

[...]

Could you pick up a somewhat meaningful name? You know, there are too
many variables with name "drm/connector/output/encoder"... :)

+	struct display_entity_notifier display_notifier;
 };
 

[...]

+static int display_notify_callback(struct display_entity_notifier *notifier,
+				   struct display_entity *entity, int event)
+{
+	struct tegra_output *output = display_notifier_to_output(notifier);
+	struct device_node *pnode;
+
+	switch (event) {
+	case DISPLAY_ENTITY_NOTIFIER_CONNECT:
+		if (output->output)
+			break;
+
+		pnode = of_parse_phandle(output->of_node, "nvidia,panel", 0);
+		if (!pnode)
+			break;
+
+		if (entity->dev && entity->dev->of_node = pnode) {
+			dev_dbg(output->dev, "connecting panel\n");
+			output->output = display_entity_get(entity);
+			display_entity_connect(&output->this, output->output);
+		}
+		of_node_put(pnode);
+
+		break;
+
+	case DISPLAY_ENTITY_NOTIFIER_DISCONNECT:
+		if (!output->output || output->output != entity)
+			break;
+
+		dev_dbg(output->dev, "disconnecting panel\n");
+		display_entity_disconnect(&output->this, output->output);
+		output->output = NULL;
+		display_entity_put(&output->this);

No "display_entity_get" for "output->this", so I don't think we need
"display_entity_put" here. If you register this entity with "release"
callback and you wanna release "output->this", call the "release"
function manually.

Only when you have "display_entity_get", use "display_entity_put" to
release.

+
+		break;
+
+	default:
+		dev_dbg(output->dev, "unhandled display event\n");
+		break;
+	}
+
+	return 0;
+}
+

[...]

Use "output->dev" here. Actually the device you wanna register it to
display entity is the "encoder"(in drm terms), not "drm->dev". If we use
"drm->dev" here, we will have all same device for all encoders(HDMI,
DSI...).

+	output->this.ops.video = &tegra_output_video_ops;
+	err = display_entity_register(&output->this);
+	if (err) {
+		dev_err(output->dev, "cannot register display entity\n");
+		return err;
+	}
+
+	/* register display notifier */
+	output->display_notifier.dev = NULL;

Set "display_notifier.dev" to NULL makes we have to compare with every
display entity, just like what you do in "display_notify_callback":

entity->dev && entity->dev->of_node = pnode

So can we get the "struct device *" of panel here? Seems currently the
"of" framework doesn't allow "device_node -> device".

On 01/30/2013 03:50 PM, Mark Zhang wrote:

quoted

@@ -147,6 +148,9 @@ struct tegra_output {

  	struct drm_encoder encoder;
  	struct drm_connector connector;
+	struct display_entity this;
+	struct display_entity *output;

Could you pick up a somewhat meaningful name? You know, there are too
many variables with name "drm/connector/output/encoder"... :)

Well, it's supposed to be abstract. From the CDF point of view it could 
be anything besides a panel. I know this makes it an output of an 
output, but I can't think of anything better right now.

quoted

+		if (entity->dev && entity->dev->of_node = pnode) {
+			dev_dbg(output->dev, "connecting panel\n");
+			output->output = display_entity_get(entity);
+			display_entity_connect(&output->this, output->output);
+		}
+		of_node_put(pnode);
+
+		break;
+
+	case DISPLAY_ENTITY_NOTIFIER_DISCONNECT:
+		if (!output->output || output->output != entity)
+			break;
+
+		dev_dbg(output->dev, "disconnecting panel\n");
+		display_entity_disconnect(&output->this, output->output);
+		output->output = NULL;
+		display_entity_put(&output->this);

No "display_entity_get" for "output->this", so I don't think we need
"display_entity_put" here. If you register this entity with "release"
callback and you wanna release "output->this", call the "release"
function manually.

Oh, this was supposed to be called on output->output actually, to 
balance the display_entity_get() of the connect event. Thanks for 
catching this.

quoted

+	/* register display entity */
+	memset(&output->this, 0, sizeof(output->this));
+	output->this.dev = drm->dev;

Use "output->dev" here. Actually the device you wanna register it to
display entity is the "encoder"(in drm terms), not "drm->dev". If we use
"drm->dev" here, we will have all same device for all encoders(HDMI,
DSI...).

Yes, that's absolutely right.

quoted

+	/* register display notifier */
+	output->display_notifier.dev = NULL;

Set "display_notifier.dev" to NULL makes we have to compare with every
display entity, just like what you do in "display_notify_callback":

entity->dev && entity->dev->of_node = pnode

So can we get the "struct device *" of panel here? Seems currently the
"of" framework doesn't allow "device_node -> device".

Nope. AFAICT the device might not be instanciated at this point. We 
become aware of it for the first time in the callback function. We also 
don't want to defer probing until the panel is parsed first, since the 
panel might also depend on resources of the display device.

Thanks,
Alex.

On Wed, Jan 30, 2013 at 04:01:48PM +0900, Alex Courbot wrote:

On 01/30/2013 03:50 PM, Mark Zhang wrote:

quoted

quoted

@@ -147,6 +148,9 @@ struct tegra_output {

 	struct drm_encoder encoder;
 	struct drm_connector connector;
+	struct display_entity this;
+	struct display_entity *output;

Could you pick up a somewhat meaningful name? You know, there are too
many variables with name "drm/connector/output/encoder"... :)

Well, it's supposed to be abstract. From the CDF point of view it
could be anything besides a panel. I know this makes it an output of
an output, but I can't think of anything better right now.

How about renaming "this" to stream to match with what the output is in
CDF speak. And the output's output is the panel, right? Why not just
call it that? Even if it isn't directly connected to a panel entity but
has indeed a whole pipeline in between, for tegra-drm it is still a
panel.

Thierry

On 01/30/2013 04:24 PM, Thierry Reding wrote:

quoted

quoted

Could you pick up a somewhat meaningful name? You know, there are too
many variables with name "drm/connector/output/encoder"... :)

Well, it's supposed to be abstract. From the CDF point of view it
could be anything besides a panel. I know this makes it an output of
an output, but I can't think of anything better right now.

How about renaming "this" to stream to match with what the output is in
CDF speak.

Good idea.

And the output's output is the panel, right? Why not just
call it that? Even if it isn't directly connected to a panel entity but
has indeed a whole pipeline in between, for tegra-drm it is still a
panel.

Makes sense indeed. Besides the DT refer it as "panel" already.

Thanks,
Alex.

On 01/30/2013 03:01 PM, Alex Courbot wrote:

On 01/30/2013 03:50 PM, Mark Zhang wrote:

[...]

quoted

quoted

+    /* register display notifier */
+    output->display_notifier.dev = NULL;

Set "display_notifier.dev" to NULL makes we have to compare with every
display entity, just like what you do in "display_notify_callback":

entity->dev && entity->dev->of_node = pnode

So can we get the "struct device *" of panel here? Seems currently the
"of" framework doesn't allow "device_node -> device".

Nope. AFAICT the device might not be instanciated at this point. We
become aware of it for the first time in the callback function. We also
don't want to defer probing until the panel is parsed first, since the
panel might also depend on resources of the display device.

Agree.

Thanks,
Alex.

Add support for the Chunghwa CLAA101WA01A display panel.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 .../video/display/chunghwa,claa101wa01a.txt        |   8 +
 drivers/video/display/Kconfig                      |   8 +
 drivers/video/display/Makefile                     |   1 +
 drivers/video/display/panel-claa101wa01a.c         | 209 +++++++++++++++++++++
 4 files changed, 226 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/display/chunghwa,claa101wa01a.txt
 create mode 100644 drivers/video/display/panel-claa101wa01a.c

diff --git a/Documentation/devicetree/bindings/video/display/chunghwa,claa101wa01a.txt b/Documentation/devicetree/bindings/video/display/chunghwa,claa101wa01a.txt
new file mode 100644
index 0000000..cfdc7fd
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/display/chunghwa,claa101wa01a.txt

@@ -0,0 +1,8 @@
+Chunghwa CLAA101WA01A Display Panel
+
+Required properties:
+- compatible: "chunghwa,claa101wa01a"
+- pnl-supply: regulator controlling power supply to the panel
+- bl-supply: regulator controlling power supply to the backlight
+- pnl-enable-gpios: GPIO that enables the panel
+- bl-enable-gpios: GPIO that enables the backlight

diff --git a/drivers/video/display/Kconfig b/drivers/video/display/Kconfig
index 9ca2e60..6902abb 100644
--- a/drivers/video/display/Kconfig
+++ b/drivers/video/display/Kconfig

@@ -32,4 +32,12 @@ config DISPLAY_PANEL_R61517
 
 	  If you are in doubt, say N.
 
+config DISPLAY_PANEL_CLAA101WA01A
+	tristate "Chunghwa CLAA101WA01A Display Panel"
+	select BACKLIGHT_PWM
+	---help---
+	  Support for the Chunghwa CLAA101WA01A Display Panel.
+
+	  If you are in doubt, say N.
+
 endif # DISPLAY_CORE

diff --git a/drivers/video/display/Makefile b/drivers/video/display/Makefile
index ec557a1..19084a2 100644
--- a/drivers/video/display/Makefile
+++ b/drivers/video/display/Makefile

@@ -2,3 +2,4 @@ obj-$(CONFIG_DISPLAY_CORE) += display-core.o
 obj-$(CONFIG_DISPLAY_PANEL_DPI) += panel-dpi.o
 obj-$(CONFIG_DISPLAY_PANEL_R61505) += panel-r61505.o
 obj-$(CONFIG_DISPLAY_PANEL_R61517) += panel-r61517.o
+obj-$(CONFIG_DISPLAY_PANEL_CLAA101WA01A) += panel-claa101wa01a.o

diff --git a/drivers/video/display/panel-claa101wa01a.c b/drivers/video/display/panel-claa101wa01a.c
new file mode 100644
index 0000000..93ae86b
--- /dev/null
+++ b/drivers/video/display/panel-claa101wa01a.c

@@ -0,0 +1,209 @@
+/*
+ * CLAA101WA01A Display Panel
+ *
+ * Copyright (C) 2013 NVIDIA CORPORATION.  All rights reserved.
+ *
+ * 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/kernel.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/regulator/consumer.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+
+#include <video/display.h>
+
+#define CLAA101WA01A_WIDTH	223
+#define CLAA101WA01A_HEIGHT	125
+
+struct panel_claa101 {
+	struct display_entity entity;
+	struct regulator *vdd_pnl;
+	struct regulator *vdd_bl;
+	/* Enable GPIOs */
+	int pnl_enable;
+	int bl_enable;
+};
+
+#define to_panel_claa101(p)	container_of(p, struct panel_claa101, entity)
+
+static int panel_claa101_set_state(struct display_entity *entity,
+				   enum display_entity_state state)
+{
+	struct panel_claa101 *panel = to_panel_claa101(entity);
+
+	/* OFF and STANDBY are equivalent to us */
+	if (state = DISPLAY_ENTITY_STATE_STANDBY)
+		state = DISPLAY_ENTITY_STATE_OFF;
+
+	switch (state) {
+	case DISPLAY_ENTITY_STATE_OFF:
+	case DISPLAY_ENTITY_STATE_STANDBY:
+		if (entity->source)
+			display_entity_set_stream(entity->source,
+						 DISPLAY_ENTITY_STREAM_STOPPED);
+
+		/* TODO error checking? */
+		gpio_set_value_cansleep(panel->bl_enable, 0);
+		usleep_range(10000, 10000);
+		regulator_disable(panel->vdd_bl);
+		usleep_range(200000, 200000);
+		gpio_set_value_cansleep(panel->pnl_enable, 0);
+		regulator_disable(panel->vdd_pnl);
+		break;
+
+	case DISPLAY_ENTITY_STATE_ON:
+		regulator_enable(panel->vdd_pnl);
+		gpio_set_value_cansleep(panel->pnl_enable, 1);
+		usleep_range(200000, 200000);
+		regulator_enable(panel->vdd_bl);
+		usleep_range(10000, 10000);
+		gpio_set_value_cansleep(panel->bl_enable, 1);
+
+		if (entity->source)
+			display_entity_set_stream(entity->source,
+					      DISPLAY_ENTITY_STREAM_CONTINUOUS);
+		break;
+	}
+
+	return 0;
+}
+
+static int panel_claa101_get_modes(struct display_entity *entity,
+				   const struct videomode **modes)
+{
+	/* TODO get modes from EDID? */
+	return 0;
+}
+
+static int panel_claa101_get_size(struct display_entity *entity,
+				  unsigned int *width, unsigned int *height)
+{
+	*width = CLAA101WA01A_WIDTH;
+	*height = CLAA101WA01A_HEIGHT;
+
+	return 0;
+}
+
+static int panel_claa101_get_params(struct display_entity *entity,
+				 struct display_entity_interface_params *params)
+{
+	return 0;
+}
+
+static const struct display_entity_control_ops panel_claa101_control_ops = {
+	.set_state = panel_claa101_set_state,
+	.get_modes = panel_claa101_get_modes,
+	.get_size = panel_claa101_get_size,
+	.get_params = panel_claa101_get_params,
+};
+
+static int __init panel_claa101_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct panel_claa101 *panel;
+	int err;
+
+	panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
+	if (!panel)
+		return -ENOMEM;
+
+	panel->vdd_pnl = devm_regulator_get(dev, "pnl");
+	if (IS_ERR(panel->vdd_pnl)) {
+		dev_err(dev, "cannot get vdd regulator\n");
+		return PTR_ERR(panel->vdd_pnl);
+	}
+
+	panel->vdd_bl = devm_regulator_get(dev, "bl");
+	if (IS_ERR(panel->vdd_bl)) {
+		dev_err(dev, "cannot get bl regulator\n");
+		return PTR_ERR(panel->vdd_bl);
+	}
+
+	err = of_get_named_gpio(dev->of_node, "pnl-enable-gpios", 0);
+	if (err < 0) {
+		dev_err(dev, "cannot find panel enable GPIO!\n");
+		return err;
+	}
+	panel->pnl_enable = err;
+	err = devm_gpio_request_one(dev, panel->pnl_enable,
+				    GPIOF_DIR_OUT | GPIOF_INIT_LOW, "panel");
+	if (err < 0) {
+		dev_err(dev, "cannot acquire panel enable GPIO!\n");
+		return err;
+	}
+
+	err = of_get_named_gpio(dev->of_node, "bl-enable-gpios", 0);
+	if (err < 0) {
+		dev_err(dev, "cannot find backlight enable GPIO!\n");
+		return err;
+	}
+	panel->bl_enable = err;
+	err = devm_gpio_request_one(dev, panel->bl_enable,
+				   GPIOF_DIR_OUT | GPIOF_INIT_LOW, "backlight");
+	if (err < 0) {
+		dev_err(dev, "cannot acquire backlight enable GPIO!\n");
+		return err;
+	}
+
+	panel->entity.dev = dev;
+	panel->entity.ops.ctrl = &panel_claa101_control_ops;
+	err = display_entity_register(&panel->entity);
+	if (err < 0)
+		return err;
+
+	platform_set_drvdata(pdev, panel);
+
+	dev_info(dev, "%s successful\n", __func__);
+
+	return 0;
+}
+
+static int __exit panel_claa101_remove(struct platform_device *pdev)
+{
+	struct panel_claa101 *panel = platform_get_drvdata(pdev);
+
+	display_entity_unregister(&panel->entity);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static struct of_device_id panel_claa101_of_match[] = {
+	{ .compatible = "chunghwa,claa101wa01a", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, pwm_backlight_of_match);
+#else
+#endif
+
+static const struct dev_pm_ops panel_claa101_dev_pm_ops = {
+};
+
+static struct platform_driver panel_claa101_driver = {
+	.probe = panel_claa101_probe,
+	.remove = panel_claa101_remove,
+	.driver = {
+		.name = "panel_claa101wa01a",
+		.owner = THIS_MODULE,
+#ifdef CONFIG_PM
+		.pm = &panel_claa101_dev_pm_ops,
+#endif
+#ifdef CONFIG_OF
+		.of_match_table	= of_match_ptr(panel_claa101_of_match),
+#endif
+	},
+};
+
+module_platform_driver(panel_claa101_driver);
+
+MODULE_AUTHOR("Alexandre Courbot <acourbot@nvidia.com>");
+MODULE_DESCRIPTION("Chunghwa CLAA101WA01A Display Panel");
+MODULE_LICENSE("GPL");

-- 
1.8.1.1

On 01/30/2013 11:02 AM, Alexandre Courbot wrote:

Add support for the Chunghwa CLAA101WA01A display panel.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---

[...]

+
+#include <video/display.h>
+
+#define CLAA101WA01A_WIDTH	223
+#define CLAA101WA01A_HEIGHT	125

If I remember correct, the physical size of the panel can be fetched in
EDID. If this is correct, we don't have to hard-code here.

+
+struct panel_claa101 {
+	struct display_entity entity;
+	struct regulator *vdd_pnl;
+	struct regulator *vdd_bl;
+	/* Enable GPIOs */
+	int pnl_enable;
+	int bl_enable;
+};
+
+#define to_panel_claa101(p)	container_of(p, struct panel_claa101, entity)
+
+static int panel_claa101_set_state(struct display_entity *entity,
+				   enum display_entity_state state)
+{
+	struct panel_claa101 *panel = to_panel_claa101(entity);
+
+	/* OFF and STANDBY are equivalent to us */
+	if (state = DISPLAY_ENTITY_STATE_STANDBY)
+		state = DISPLAY_ENTITY_STATE_OFF;

Do we need this? The "switch" below handles the same thing already.

+
+	switch (state) {
+	case DISPLAY_ENTITY_STATE_OFF:
+	case DISPLAY_ENTITY_STATE_STANDBY:
+		if (entity->source)
+			display_entity_set_stream(entity->source,
+						 DISPLAY_ENTITY_STREAM_STOPPED);
+
+		/* TODO error checking? */
+		gpio_set_value_cansleep(panel->bl_enable, 0);
+		usleep_range(10000, 10000);
+		regulator_disable(panel->vdd_bl);
+		usleep_range(200000, 200000);
+		gpio_set_value_cansleep(panel->pnl_enable, 0);
+		regulator_disable(panel->vdd_pnl);
+		break;
+
+	case DISPLAY_ENTITY_STATE_ON:
+		regulator_enable(panel->vdd_pnl);
+		gpio_set_value_cansleep(panel->pnl_enable, 1);
+		usleep_range(200000, 200000);
+		regulator_enable(panel->vdd_bl);
+		usleep_range(10000, 10000);
+		gpio_set_value_cansleep(panel->bl_enable, 1);
+
+		if (entity->source)
+			display_entity_set_stream(entity->source,
+					      DISPLAY_ENTITY_STREAM_CONTINUOUS);
+		break;
+	}
+
+	return 0;
+}
+
+static int panel_claa101_get_modes(struct display_entity *entity,
+				   const struct videomode **modes)
+{
+	/* TODO get modes from EDID? */

Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that
case, you can get EDID here. I know drm has some helpers to fetch EDID
but I recall there are some other functions which has no drm
dependencies which may be suitable for you.

+	return 0;
+}

[...]

+
+static int __exit panel_claa101_remove(struct platform_device *pdev)
+{
+	struct panel_claa101 *panel = platform_get_drvdata(pdev);
+
+	display_entity_unregister(&panel->entity);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF

We don't need this kind of "ifdef" in upstream kernel.

+static struct of_device_id panel_claa101_of_match[] = {
+	{ .compatible = "chunghwa,claa101wa01a", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, pwm_backlight_of_match);

What does this mean? Why we need this?

+#else
+#endif
+
+static const struct dev_pm_ops panel_claa101_dev_pm_ops = {
+};
+
+static struct platform_driver panel_claa101_driver = {
+	.probe = panel_claa101_probe,
+	.remove = panel_claa101_remove,
+	.driver = {
+		.name = "panel_claa101wa01a",
+		.owner = THIS_MODULE,
+#ifdef CONFIG_PM
+		.pm = &panel_claa101_dev_pm_ops,

If you haven't implemented this in this patch set, I suggest removing this.

+#endif
+#ifdef CONFIG_OF
+		.of_match_table	= of_match_ptr(panel_claa101_of_match),
+#endif
+	},
+};
+
+module_platform_driver(panel_claa101_driver);
+
+MODULE_AUTHOR("Alexandre Courbot [off-list ref]");
+MODULE_DESCRIPTION("Chunghwa CLAA101WA01A Display Panel");
+MODULE_LICENSE("GPL");

On 01/30/2013 04:20 PM, Mark Zhang wrote:

quoted

+	/* OFF and STANDBY are equivalent to us */
+	if (state = DISPLAY_ENTITY_STATE_STANDBY)
+		state = DISPLAY_ENTITY_STATE_OFF;

Do we need this? The "switch" below handles the same thing already.

Indeed, I have rewritten this part actually.

quoted

+static int panel_claa101_get_modes(struct display_entity *entity,
+				   const struct videomode **modes)
+{
+	/* TODO get modes from EDID? */

Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that
case, you can get EDID here. I know drm has some helpers to fetch EDID
but I recall there are some other functions which has no drm
dependencies which may be suitable for you.

I explained this in the cover letter - I'm not sure we want to have a 
dependency on DRM here, as CDF entities could also be connected to other 
subsystems. That's something we need to figure out. But yes, ultimately 
this should be the place where EDID is retrieved.

quoted

+static struct of_device_id panel_claa101_of_match[] = {
+	{ .compatible = "chunghwa,claa101wa01a", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, pwm_backlight_of_match);

What does this mean? Why we need this?

Well, now you know where I copy my code from. :)


Thanks,
Alex.

On Wed, Jan 30, 2013 at 04:27:11PM +0900, Alex Courbot wrote:

On 01/30/2013 04:20 PM, Mark Zhang wrote:

[...]

quoted

quoted

+static int panel_claa101_get_modes(struct display_entity *entity,
+				   const struct videomode **modes)
+{
+	/* TODO get modes from EDID? */

Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that
case, you can get EDID here. I know drm has some helpers to fetch EDID
but I recall there are some other functions which has no drm
dependencies which may be suitable for you.

I explained this in the cover letter - I'm not sure we want to have
a dependency on DRM here, as CDF entities could also be connected to
other subsystems. That's something we need to figure out. But yes,
ultimately this should be the place where EDID is retrieved.

I already said this in another thread. I think we should only be using
the CDF .get_modes() for static modes that cannot be obtained from EDID.
Thinking about it, I'm not quite sure why EDID would be needed for this
kind of panel anyway. Ventana probably has it because it keeps us from
having to hardcode things, but if we provide drivers for the panel
anyway, we can just as well hard-code the supported mode(s) in those
drivers.

What I'm trying to say is that the existence of EDID is board-specific,
so boards other than Ventana may not have an EDID EEPROM. Or perhaps
this particular display has the EEPROM integrated? Even in that case,
some boards may use this panel and simply not connect the EEPROM to an
I2C controller.

Thierry

On 01/30/2013 03:48 PM, Thierry Reding wrote:

* PGP Signed by an unknown key

On Wed, Jan 30, 2013 at 04:27:11PM +0900, Alex Courbot wrote:

quoted

On 01/30/2013 04:20 PM, Mark Zhang wrote:

[...]

quoted

quoted

quoted

+static int panel_claa101_get_modes(struct display_entity *entity,
+				   const struct videomode **modes)
+{
+	/* TODO get modes from EDID? */

Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that
case, you can get EDID here. I know drm has some helpers to fetch EDID
but I recall there are some other functions which has no drm
dependencies which may be suitable for you.

I explained this in the cover letter - I'm not sure we want to have
a dependency on DRM here, as CDF entities could also be connected to
other subsystems. That's something we need to figure out. But yes,
ultimately this should be the place where EDID is retrieved.

I already said this in another thread. I think we should only be using
the CDF .get_modes() for static modes that cannot be obtained from EDID.
Thinking about it, I'm not quite sure why EDID would be needed for this
kind of panel anyway. Ventana probably has it because it keeps us from
having to hardcode things, but if we provide drivers for the panel
anyway, we can just as well hard-code the supported mode(s) in those
drivers.

I don't think so. I think it's better that we only have one entry for
getting video modes. Since CDF already provides ".get_modes" for us, we
can rely on that. And according to my understanding, get video modes in
panel driver makes sense than getting it in crtc.

In this way, panel driver may get video modes from EDID or from
hard-coded display timing infos, but other modules such as crtc don't
need to care about that.

Mark

What I'm trying to say is that the existence of EDID is board-specific,
so boards other than Ventana may not have an EDID EEPROM. Or perhaps
this particular display has the EEPROM integrated? Even in that case,
some boards may use this panel and simply not connect the EEPROM to an
I2C controller.

Thierry

* Unknown Key
* 0x7F3EB3A1

On 01/30/2013 04:48 PM, Thierry Reding wrote:

I already said this in another thread. I think we should only be using
the CDF .get_modes() for static modes that cannot be obtained from EDID.
Thinking about it, I'm not quite sure why EDID would be needed for this
kind of panel anyway. Ventana probably has it because it keeps us from
having to hardcode things, but if we provide drivers for the panel
anyway, we can just as well hard-code the supported mode(s) in those
drivers.

What I'm trying to say is that the existence of EDID is board-specific,
so boards other than Ventana may not have an EDID EEPROM. Or perhaps
this particular display has the EEPROM integrated? Even in that case,
some boards may use this panel and simply not connect the EEPROM to an
I2C controller.

Actually this display has its own EEPROM and pins for the I2C bus. 
That's why it would seems out-of-place to have EDID taking place outside 
its driver.

But you are right that we should also handle the case where the I2C bus 
is not connected and provide a static list of videomodes (that could be 
an "offline" dump of the EDID data). However, the driver could take the 
decision to use it if the EDID bus is not specified in the DT or if 
transfer failed for some reason.

But then as you mention we might as well save time and directly serve 
that offline version, since we know the content of the EEPROM anyway.

Alex.

On 01/30/2013 12:20 AM, Mark Zhang wrote:

On 01/30/2013 11:02 AM, Alexandre Courbot wrote:

quoted

Add support for the Chunghwa CLAA101WA01A display panel.

quoted

+static int panel_claa101_get_modes(struct display_entity *entity,
+				   const struct videomode **modes)
+{
+	/* TODO get modes from EDID? */

Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that
case, you can get EDID here. I know drm has some helpers to fetch EDID
but I recall there are some other functions which has no drm
dependencies which may be suitable for you.

DDC access is a property of the display controller, not the panel
itself. The panel might be hooked up to a display controller's DDC/I2C
channel as the target, but it isn't the host/controller of the DDC/I2C
channel. As such, placing the nvidia,ddc property into the display
controller node makes sense.

Re: the other discussion in this thread: Probably the simplest is if
tegradrm/other-CDF-users do something like:

1) If DDC I2C channel available for this display channel, query DDC.
2) If not, or perhaps also if that fails, query the panel driver for the
mode list.

That would cover all bases very simply.

On 01/31/2013 04:19 AM, Stephen Warren wrote:

On 01/30/2013 12:20 AM, Mark Zhang wrote:

quoted

On 01/30/2013 11:02 AM, Alexandre Courbot wrote:

quoted

Add support for the Chunghwa CLAA101WA01A display panel.

quoted

quoted

+static int panel_claa101_get_modes(struct display_entity *entity,
+				   const struct videomode **modes)
+{
+	/* TODO get modes from EDID? */

Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that
case, you can get EDID here. I know drm has some helpers to fetch EDID
but I recall there are some other functions which has no drm
dependencies which may be suitable for you.

DDC access is a property of the display controller, not the panel
itself. The panel might be hooked up to a display controller's DDC/I2C
channel as the target, but it isn't the host/controller of the DDC/I2C
channel. As such, placing the nvidia,ddc property into the display
controller node makes sense.

Yes, DC triggers the DDC access and is the host of the DDC/I2C channel.
So I think it's reasonable to put nvidia,ddc property into the display
controller node. But the video mode info in EDID which be fetched via
DDC is the property of the panel, so this info should be provided by
panel driver. Actually display controller cares about the video modes,
not the way to get these info. So I think it's better to do the whole
work like this:

1) display controller relied on CDF to call "display_entity_get_modes"
2) panel driver calls the function which is hooked to display controller
to get the video modes via DDC.
3) if the video modes can't be fetched via DDC, panel driver provides
the info(either by hard-coded or defined in DT) anyway

Re: the other discussion in this thread: Probably the simplest is if
tegradrm/other-CDF-users do something like:

1) If DDC I2C channel available for this display channel, query DDC.

Just like I said above, display controller should not query DDC
directly. Since CDF already defines these for us, display controller
should call CDF's functions, like: display_entity_get_modes,
display_entity_get_size... I think this is the key difference I wanna
talk about. And also in this way, display controller doesn't need to
care about this 2 steps logics, just call "display_entity_get_modes" is OK.

Mark

2) If not, or perhaps also if that fails, query the panel driver for the
mode list.

That would cover all bases very simply.

On Thu, Jan 31, 2013 at 12:51 PM, Mark Zhang [off-list ref] wrote:

quoted

DDC access is a property of the display controller, not the panel
itself. The panel might be hooked up to a display controller's DDC/I2C
channel as the target, but it isn't the host/controller of the DDC/I2C
channel. As such, placing the nvidia,ddc property into the display
controller node makes sense.

Yes, DC triggers the DDC access and is the host of the DDC/I2C channel.
So I think it's reasonable to put nvidia,ddc property into the display
controller node. But the video mode info in EDID which be fetched via
DDC is the property of the panel, so this info should be provided by
panel driver. Actually display controller cares about the video modes,
not the way to get these info. So I think it's better to do the whole
work like this:

1) display controller relied on CDF to call "display_entity_get_modes"
2) panel driver calls the function which is hooked to display controller
to get the video modes via DDC.
3) if the video modes can't be fetched via DDC, panel driver provides
the info(either by hard-coded or defined in DT) anyway

This would require a callback dedicated to this in display_entity and
would break its simple design.

Just like I said above, display controller should not query DDC
directly. Since CDF already defines these for us, display controller
should call CDF's functions, like: display_entity_get_modes,
display_entity_get_size... I think this is the key difference I wanna
talk about. And also in this way, display controller doesn't need to
care about this 2 steps logics, just call "display_entity_get_modes" is OK.

Well the fact is that it *is* the display controller that queries DDC
directly. The panel is just a bus here, and the EDID EEPROM could
perfectly work without it. If you model what you described with DT
nodes, I'm afraid you will end up with something both complex (with DC
node referencing panel node and back again for the EDID bus) and that
does not picture reality accurately.

Alex.

On 01/31/2013 12:24 PM, Alexandre Courbot wrote:

On Thu, Jan 31, 2013 at 12:51 PM, Mark Zhang [off-list ref] wrote:

quoted

quoted

DDC access is a property of the display controller, not the panel
itself. The panel might be hooked up to a display controller's DDC/I2C
channel as the target, but it isn't the host/controller of the DDC/I2C
channel. As such, placing the nvidia,ddc property into the display
controller node makes sense.

Yes, DC triggers the DDC access and is the host of the DDC/I2C channel.
So I think it's reasonable to put nvidia,ddc property into the display
controller node. But the video mode info in EDID which be fetched via
DDC is the property of the panel, so this info should be provided by
panel driver. Actually display controller cares about the video modes,
not the way to get these info. So I think it's better to do the whole
work like this:

1) display controller relied on CDF to call "display_entity_get_modes"
2) panel driver calls the function which is hooked to display controller
to get the video modes via DDC.
3) if the video modes can't be fetched via DDC, panel driver provides
the info(either by hard-coded or defined in DT) anyway

This would require a callback dedicated to this in display_entity and
would break its simple design.

We just need to add a function pointer param which can be used by panel
driver in "display_entity_get_modes", don't we?

quoted

Just like I said above, display controller should not query DDC
directly. Since CDF already defines these for us, display controller
should call CDF's functions, like: display_entity_get_modes,
display_entity_get_size... I think this is the key difference I wanna
talk about. And also in this way, display controller doesn't need to
care about this 2 steps logics, just call "display_entity_get_modes" is OK.

Well the fact is that it *is* the display controller that queries DDC
directly. The panel is just a bus here, and the EDID EEPROM could
perfectly work without it. If you model what you described with DT
nodes, I'm afraid you will end up with something both complex (with DC
node referencing panel node and back again for the EDID bus) and that
does not picture reality accurately.

Display controller don't know whether the panel has EDID EEPROM but the
panel driver knows. So why we need to make display controller queries
EDID blindly? Since panel driver knows about all "video modes/panel
size" stuffs, why not we let it handle all these things?

Alex.

On Thu, Jan 31, 2013 at 1:54 PM, Mark Zhang [off-list ref] wrote:

Display controller don't know whether the panel has EDID EEPROM but the
panel driver knows. So why we need to make display controller queries
EDID blindly? Since panel driver knows about all "video modes/panel
size" stuffs, why not we let it handle all these things?

The DC actually does know whether the connected panel supports EDID -
in this case, the output node will have a property for the DDC bus,
e.g.

        nvidia,ddc-i2c-bus = <&lcd_ddc>;

If no DDC bus is specified or if transfer fails, the DC driver can
still query the panel driver for hardcoded modes.

Also passing a function pointer to get_modes() does not relief the DC
driver from probing for the DDC bus. You will still have to handle
both conditions (DDC bus present or not), the check will just be moved
into your callback function.

Alex.

On 01/31/2013 02:36 PM, Alexandre Courbot wrote:

On Thu, Jan 31, 2013 at 1:54 PM, Mark Zhang [off-list ref] wrote:

quoted

Display controller don't know whether the panel has EDID EEPROM but the
panel driver knows. So why we need to make display controller queries
EDID blindly? Since panel driver knows about all "video modes/panel
size" stuffs, why not we let it handle all these things?

The DC actually does know whether the connected panel supports EDID -
in this case, the output node will have a property for the DDC bus,
e.g.

        nvidia,ddc-i2c-bus = <&lcd_ddc>;

If no DDC bus is specified or if transfer fails, the DC driver can
still query the panel driver for hardcoded modes.

Strictly speaking, according to my understanding, "nvidia,ddc-i2c-bus"
means tegra has a hardware ddc channel, this doesn't mean we can get the
EDID from this DDC surely.

Also passing a function pointer to get_modes() does not relief the DC
driver from probing for the DDC bus. You will still have to handle
both conditions (DDC bus present or not), the check will just be moved
into your callback function.

Yes. But panel driver decides whether the DDC probing is needed. If the
panel has an EEPROM, it can call the function which display controller
provides to do a DDC probing, it not, panel driver just ignores this
function pointer and return the hardcoded modes directly.

Alex.

On 01/31/2013 12:30 AM, Mark Zhang wrote:

On 01/31/2013 02:36 PM, Alexandre Courbot wrote:

quoted

On Thu, Jan 31, 2013 at 1:54 PM, Mark Zhang [off-list ref] wrote:

quoted

Display controller don't know whether the panel has EDID EEPROM but the
panel driver knows. So why we need to make display controller queries
EDID blindly? Since panel driver knows about all "video modes/panel
size" stuffs, why not we let it handle all these things?

The DC actually does know whether the connected panel supports EDID -
in this case, the output node will have a property for the DDC bus,
e.g.

        nvidia,ddc-i2c-bus = <&lcd_ddc>;

If no DDC bus is specified or if transfer fails, the DC driver can
still query the panel driver for hardcoded modes.

Strictly speaking, according to my understanding, "nvidia,ddc-i2c-bus"
means tegra has a hardware ddc channel, this doesn't mean we can get the
EDID from this DDC surely.

Having a DDC bus by definition means that the EDID can be queried
through it. If you can't query the EDID, there is no DDC bus, and hence
that property is not present in the DT.

On 01/30/2013 08:51 PM, Mark Zhang wrote:

On 01/31/2013 04:19 AM, Stephen Warren wrote:

quoted

On 01/30/2013 12:20 AM, Mark Zhang wrote:

quoted

On 01/30/2013 11:02 AM, Alexandre Courbot wrote:

quoted

Add support for the Chunghwa CLAA101WA01A display panel.

quoted

quoted

+static int panel_claa101_get_modes(struct display_entity *entity,
+				   const struct videomode **modes)
+{
+	/* TODO get modes from EDID? */

Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that
case, you can get EDID here. I know drm has some helpers to fetch EDID
but I recall there are some other functions which has no drm
dependencies which may be suitable for you.

DDC access is a property of the display controller, not the panel
itself. The panel might be hooked up to a display controller's DDC/I2C
channel as the target, but it isn't the host/controller of the DDC/I2C
channel. As such, placing the nvidia,ddc property into the display
controller node makes sense.

Yes, DC triggers the DDC access and is the host of the DDC/I2C channel.
So I think it's reasonable to put nvidia,ddc property into the display
controller node. But the video mode info in EDID which be fetched via
DDC is the property of the panel, so this info should be provided by
panel driver.

No, that makes absolutely no sense at all in the EDID case.

By the same argument, we'd need a panel driver for every external
monitor which implemented EDID, just to transfer the EDID results from
the display controller's DDC channel into the panel driver and back into
the display controller code, which wants the mode list.

Again, if the mode list is coming from DDC, the display controller
should retrieve it in exactly the same way it retrieves it for any
external monitor - by direct control of the DDC channel to read the
EDID. The only time it makes sense for the panel driver to get involved
in supplying the mode list is when there's no EDID, so the list must be
hard-coded into the driver.

On 02/01/2013 01:20 AM, Stephen Warren wrote:

On 01/30/2013 08:51 PM, Mark Zhang wrote:

quoted

On 01/31/2013 04:19 AM, Stephen Warren wrote:

quoted

On 01/30/2013 12:20 AM, Mark Zhang wrote:

quoted

On 01/30/2013 11:02 AM, Alexandre Courbot wrote:

quoted

Add support for the Chunghwa CLAA101WA01A display panel.

quoted

quoted

+static int panel_claa101_get_modes(struct display_entity *entity,
+				   const struct videomode **modes)
+{
+	/* TODO get modes from EDID? */

Why not move the "nvidia,ddc" from encoder's DT to panel's DT? In that
case, you can get EDID here. I know drm has some helpers to fetch EDID
but I recall there are some other functions which has no drm
dependencies which may be suitable for you.

DDC access is a property of the display controller, not the panel
itself. The panel might be hooked up to a display controller's DDC/I2C
channel as the target, but it isn't the host/controller of the DDC/I2C
channel. As such, placing the nvidia,ddc property into the display
controller node makes sense.

Yes, DC triggers the DDC access and is the host of the DDC/I2C channel.
So I think it's reasonable to put nvidia,ddc property into the display
controller node. But the video mode info in EDID which be fetched via
DDC is the property of the panel, so this info should be provided by
panel driver.

No, that makes absolutely no sense at all in the EDID case.

By the same argument, we'd need a panel driver for every external
monitor which implemented EDID, just to transfer the EDID results from
the display controller's DDC channel into the panel driver and back into
the display controller code, which wants the mode list.

Ah, yes, this is right. We can't write driver for every external monitor
which implements EDID. Although I think it's more reasonable that panel
decides whether the DDC probe is necessary.

Again, if the mode list is coming from DDC, the display controller
should retrieve it in exactly the same way it retrieves it for any
external monitor - by direct control of the DDC channel to read the
EDID. The only time it makes sense for the panel driver to get involved
in supplying the mode list is when there's no EDID, so the list must be
hard-coded into the driver.

On 01/29/2013 08:02 PM, Alexandre Courbot wrote:

Add support for the Chunghwa CLAA101WA01A display panel.

+Chunghwa CLAA101WA01A Display Panel
+
+Required properties:
+- compatible: "chunghwa,claa101wa01a"
+- pnl-supply: regulator controlling power supply to the panel
+- bl-supply: regulator controlling power supply to the backlight
+- pnl-enable-gpios: GPIO that enables the panel
+- bl-enable-gpios: GPIO that enables the backlight

So this looks like a reasonable binding to me. The one issue is that
it's very generic, and if we go this route, we'll probably end up with
tens or hundreds of identical or extremely similar simple bindings, and
associated drivers.

We can avoid this instead by defining something like a "simple-lcd"
binding, and associated driver, that will work for perhaps 90%, 95%,
99%, even 100%(?) of panels. Just like the above, but with:

compatible="simple-panel", "chunghwa,claa101wa01a"

instead, and the driver binding to "simple-panel" rather than
"chunghwa,claa101wa01a".

The driver can assume that a specific set of supplies (and perhaps
GPIOs) is always present, and that the /sequence/ of manipulating those
is fixed. This will avoid the need for anything like the power sequences
code. If a particular panel doesn't fit those assumptions, including the
exact sequence of manipulations for each state transition (which should
be documented in the binding) then it can get a custom driver, this also
avoiding having to define custom sequences in DT.

Things that might be parameterized/optional:

* Perhaps some GPIOs aren't always present.
* If some regulators aren't SW-controllable, DT should still provide a
fixed/dummy regulator so the driver doesn't care.
* Wait times between regulator/GPIO/... manipulation could be specified
in DT.
* For panels without EDID, CDF DT bindings can provide the list of
supported modes, otherwise we assume that the display controller that
drives the panel has been told how to access the EDID, just like it
would for an "external" display.

On Thu, Jan 31, 2013 at 5:27 AM, Stephen Warren [off-list ref] wrote:

So this looks like a reasonable binding to me. The one issue is that
it's very generic, and if we go this route, we'll probably end up with
tens or hundreds of identical or extremely similar simple bindings, and
associated drivers.

We can avoid this instead by defining something like a "simple-lcd"
binding, and associated driver, that will work for perhaps 90%, 95%,
99%, even 100%(?) of panels.

That seems totally doable indeed. Actually the right way to do this
might be by extending the simple DPI panel driver Laurent included in
his patchset.

Just like the above, but with:

compatible="simple-panel", "chunghwa,claa101wa01a"

instead, and the driver binding to "simple-panel" rather than
"chunghwa,claa101wa01a".

Just out of curiosity, why don't we rather define

compatible="chunghwa,claa101wa01a", "simple-panel"

in that order? I thought DT compatible strings should go from more to
less specific. The device would still bind to "simple-panel" if no
more specific driver exists.

The driver can assume that a specific set of supplies (and perhaps
GPIOs) is always present, and that the /sequence/ of manipulating those
is fixed. This will avoid the need for anything like the power sequences
code. If a particular panel doesn't fit those assumptions, including the
exact sequence of manipulations for each state transition (which should
be documented in the binding) then it can get a custom driver, this also
avoiding having to define custom sequences in DT.

Things that might be parameterized/optional:

* Perhaps some GPIOs aren't always present.
* If some regulators aren't SW-controllable, DT should still provide a
fixed/dummy regulator so the driver doesn't care.

How about making all regulators and GPIO optional in the driver?

* Wait times between regulator/GPIO/... manipulation could be specified
in DT.
* For panels without EDID, CDF DT bindings can provide the list of
supported modes, otherwise we assume that the display controller that
drives the panel has been told how to access the EDID, just like it
would for an "external" display.

Excellent. Thanks for the feedback.

Alex.

On 01/30/2013 09:14 PM, Alexandre Courbot wrote:

On Thu, Jan 31, 2013 at 5:27 AM, Stephen Warren [off-list ref] wrote:

quoted

So this looks like a reasonable binding to me. The one issue is that
it's very generic, and if we go this route, we'll probably end up with
tens or hundreds of identical or extremely similar simple bindings, and
associated drivers.

We can avoid this instead by defining something like a "simple-lcd"
binding, and associated driver, that will work for perhaps 90%, 95%,
99%, even 100%(?) of panels.

That seems totally doable indeed. Actually the right way to do this
might be by extending the simple DPI panel driver Laurent included in
his patchset.

quoted

Just like the above, but with:

compatible="simple-panel", "chunghwa,claa101wa01a"

instead, and the driver binding to "simple-panel" rather than
"chunghwa,claa101wa01a".

Just out of curiosity, why don't we rather define

compatible="chunghwa,claa101wa01a", "simple-panel"

in that order? I thought DT compatible strings should go from more to
less specific. The device would still bind to "simple-panel" if no
more specific driver exists.

Yes, that's correct; I "typo"d the example I gave.

quoted

The driver can assume that a specific set of supplies (and perhaps
GPIOs) is always present, and that the /sequence/ of manipulating those
is fixed. This will avoid the need for anything like the power sequences
code. If a particular panel doesn't fit those assumptions, including the
exact sequence of manipulations for each state transition (which should
be documented in the binding) then it can get a custom driver, this also
avoiding having to define custom sequences in DT.

Things that might be parameterized/optional:

* Perhaps some GPIOs aren't always present.
* If some regulators aren't SW-controllable, DT should still provide a
fixed/dummy regulator so the driver doesn't care.

How about making all regulators and GPIO optional in the driver?

The general feedback I've received is that regulators should always be
present, and simply implemented by fixed/dummy regulators if missing in
a particular board design, rather than having the driver code handle
them being optional. This certainly does simplify the driver, since it
can always unconditionally program the regulator irrespective of whether
it's fixed/dummy or not.

On 01/29/2013 08:02 PM, Alexandre Courbot wrote:

Add support for the Chunghwa CLAA101WA01A display panel.

Since this defines a new DT binding, especially since it's for a new
class of device, this patch should be Cc:
devicetree-discuss@lists.ozlabs.org too, next time you post.

On Wed, Jan 30, 2013 at 12:02:15PM +0900, Alexandre Courbot wrote:

This series leverages the (still work-in-progress) Common Display Framework to
add panel support to the tegra-drm driver. It also adds a driver for the
CLAA101WA01A panel used on the Ventana board.

The CDF is a moving target but Tegra needs some sort of display framework and
even in its current state the CDF seems to be the best candidate. Besides, by
using the CDF from now on we hope to provide useful feedback to Laurent and the
other CDF designers.

The changes to tegra-drm are rather minimal. Panels are referenced from Tegra DC
output nodes through the "nvidia,panel" property. This property is looked up
when a display connect notification is received in order to see if it points to
the connected display entity. If it does, the entity is then used for output.

The DPMS states are then propagated to the output entity, which is then supposed
to call back into the set_stream() hook in order to enable/disable the output
stream as needed.

Thanks *a lot* for taking care of this Alexandre! From a quick look at
the patches they seem generally fine. I'll go over them in a bit more
detail though.

Although the overall design seems to work ok, a few specific issues need to be
addressed:

1) The CDF has a get_modes() hook, but this is already implemented by
tegra_connector_get_modes(). Ideally everything should be moved to the CDF hook,
but Tegra's implementation uses DRM functions to retrieve the EDID and CDF
should, AFAIK, remain DRM-agnostic.

Maybe a good option would be to just not implement get_modes() if the
same information can be retrieved via EDID. That is, the DRM driver
could just go and fetch EDID when the nvidia,ddc-i2c-bus is available
(or parse the nvidia,edid blob) and only rely on CDF otherwise.

So for Ventana the only reason why we need CDF is basically the power
sequencing, right?

2) There is currently no panel/backlight interaction, e.g. backlight status is
controlled through FB events, independantly from the panel state. It could make
sense to have the panel DT node reference the backlight and control it as part
of its own power on/off sequence. Right now however, a backlight device cannot
ignore FB events.

I definitely think that we should aim for correct panel and backlight
interaction. Perhaps this could work by looking up the real backlight
via it's phandle and have the CDF driver use the backlight API to
disable or enable the backlight as part of the power sequencing.

I'm not sure what you mean by "cannot ignore FB events"? Can you provide
a concrete problematic use-case?

3) Probably related to 2), now the backlight's power controls are part of the
panel driver, because the pwm-backlight driver cannot control the power
regulator and enable GPIO. This means that the backlight power is not turned off
when its brightness is set to 0 through sysfs. Once again this speaks in favor
of having stronger panel/backlight interaction: for instance, the panel driver
could reference the backlight and hijack its update_status() op to replace it by
one that does the correct power sequencing before calling the original function.
This would require some extra infrastructure though. Another possibility would
be to have a dedicated backlight driver for each panel, with its own
"compatible" string.

Hijacking .update_status() sounds a bit risky. But perhaps you could
wrap the real backlight in a CDF backlight to receive notifications.
Obviously you'd get two backlight devices in sysfs, but that turn out
not to be a problem.

Thierry

On 01/30/2013 04:40 PM, Thierry Reding wrote:

Thanks *a lot* for taking care of this Alexandre! From a quick look at
the patches they seem generally fine. I'll go over them in a bit more
detail though.

Glad you like it better than my previous attempts at controlling Tegra's 
panels and backlights. ;)

quoted

1) The CDF has a get_modes() hook, but this is already implemented by
tegra_connector_get_modes(). Ideally everything should be moved to the CDF hook,
but Tegra's implementation uses DRM functions to retrieve the EDID and CDF
should, AFAIK, remain DRM-agnostic.

Maybe a good option would be to just not implement get_modes() if the
same information can be retrieved via EDID. That is, the DRM driver
could just go and fetch EDID when the nvidia,ddc-i2c-bus is available
(or parse the nvidia,edid blob) and only rely on CDF otherwise.

Since EDID information is per-panel I'd intuitively say it should be 
provided by the panel driver.

So for Ventana the only reason why we need CDF is basically the power
sequencing, right?

As of now, yes.

I definitely think that we should aim for correct panel and backlight
interaction. Perhaps this could work by looking up the real backlight
via it's phandle and have the CDF driver use the backlight API to
disable or enable the backlight as part of the power sequencing.

I have just written a bit of code that does that. It works well and 
seems like a natural way to operate the backlight. However...

I'm not sure what you mean by "cannot ignore FB events"? Can you provide
a concrete problematic use-case?

... that's where thing stop looking nice. The backlight framework 
forcibly registers a framebuffer notifier callback and switches the 
backlight on and off on blank/unblank events, effectively duplicating 
what the panel driver does. There is no way to disable this behavior at 
the moment.

This could be solved by introducing a new function for controlling the 
"ownership" of the backlight that would unregister the notifier when the 
panel driver takes its reference to the backlight.

Hijacking .update_status() sounds a bit risky. But perhaps you could
wrap the real backlight in a CDF backlight to receive notifications.
Obviously you'd get two backlight devices in sysfs, but that turn out
not to be a problem.

For cosmetic reasons I'd prefer to avoid having two backlight devices 
(which ended up in that terrible PWM backlight subdriver thing). Maybe 
we can engineer the backlight framework to make such customizations easier?

Alex.

On Wed, Jan 30, 2013 at 05:23:01PM +0900, Alex Courbot wrote:

On 01/30/2013 04:40 PM, Thierry Reding wrote:

quoted

Thanks *a lot* for taking care of this Alexandre! From a quick look at
the patches they seem generally fine. I'll go over them in a bit more
detail though.

Glad you like it better than my previous attempts at controlling
Tegra's panels and backlights. ;)

quoted

quoted

1) The CDF has a get_modes() hook, but this is already implemented by
tegra_connector_get_modes(). Ideally everything should be moved to the CDF hook,
but Tegra's implementation uses DRM functions to retrieve the EDID and CDF
should, AFAIK, remain DRM-agnostic.

Maybe a good option would be to just not implement get_modes() if the
same information can be retrieved via EDID. That is, the DRM driver
could just go and fetch EDID when the nvidia,ddc-i2c-bus is available
(or parse the nvidia,edid blob) and only rely on CDF otherwise.

Since EDID information is per-panel I'd intuitively say it should be
provided by the panel driver.

Hm. If you know from the devicetree that you have a CLAA101WA01A then
you won't need EDID data at all. If instead you have EDID data you don't
have to put the information that this is a CLAA101WA01A into devicetree.

I think the next thing that will happen is that you want to use the EDID
data to 1) pick display timings 2) pick the right power sequence that
fits the panel found in EDID. You can't archieve 2) by hardcoding the
panel in the devicetree.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |