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");