[RFC PATCH 0/3] drm/panel: Pull some code out into common helpers

[RFC PATCH 0/3] drm/panel: Pull some code out into common helpers

[Sean Paul]

This series pulls out the power-sequencing code from panel-simple into a
panel-common helper library. This allows drivers that cannot leverage
panel-simple to share some code.

I've converted the 2 sharp mipi drivers, and Chris Zhong's driver on the
list can also be converted. I haven't checked any other drivers, but I
suspect we'll see the same code blocks there too.

I'm sure there's more we can pull out of the various drivers, but this
seems like a good place to start talking about how to share common panel
code across drivers.

Sean

Sean Paul (3):
  drm/panel: Pull common panel code out into helpers
  drm/panel: sharp-lq101r1sx01: Use panel-common helpers
  drm/panel: panel-sharp-ls043t1le01: Use panel-common helpers

 drivers/gpu/drm/panel/Kconfig                   |  22 +++-
 drivers/gpu/drm/panel/Makefile                  |   1 +
 drivers/gpu/drm/panel/panel-common.c            | 149 ++++++++++++++++++++++++
 drivers/gpu/drm/panel/panel-common.h            |  44 +++++++
 drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c |  79 ++++---------
 drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c |  70 +++--------
 drivers/gpu/drm/panel/panel-simple.c            | 112 +++---------------
 7 files changed, 269 insertions(+), 208 deletions(-)
 create mode 100644 drivers/gpu/drm/panel/panel-common.c
 create mode 100644 drivers/gpu/drm/panel/panel-common.h

-- 
2.12.0.367.g23dc2f6d3c-goog

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

[RFC PATCH 1/3] drm/panel: Pull common panel code out into helpers

[Sean Paul]

This patch pulls the regulator/backlight/enable_gpio code out of
panel-simple and creates a new panel-common helper with it. This
helper will be useful to the more complicated drivers which cannot
use panel-simple.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/panel/Kconfig        |  20 +++--
 drivers/gpu/drm/panel/Makefile       |   1 +
 drivers/gpu/drm/panel/panel-common.c | 149 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/panel/panel-common.h |  44 +++++++++++
 drivers/gpu/drm/panel/panel-simple.c | 112 ++++----------------------
 5 files changed, 225 insertions(+), 101 deletions(-)
 create mode 100644 drivers/gpu/drm/panel/panel-common.c
 create mode 100644 drivers/gpu/drm/panel/panel-common.h

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 62aba976e744..be8590724042 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -7,16 +7,26 @@ config DRM_PANEL
 menu "Display Panels"
 	depends on DRM && DRM_PANEL
 
+config DRM_PANEL_COMMON
+	bool
+	depends on OF
+	depends on BACKLIGHT_CLASS_DEVICE
+	select VIDEOMODE_HELPERS
+	help
+	  Common DRM panel helpers for panels using a regulator and a GPIO to
+	  be powered up. Optionally a backlight can be attached so that it can
+	  be automatically turned off when the panel goes into a low power
+	  state.
+
 config DRM_PANEL_SIMPLE
 	tristate "support for simple panels"
 	depends on OF
-	depends on BACKLIGHT_CLASS_DEVICE
+	select DRM_PANEL_COMMON
 	select VIDEOMODE_HELPERS
 	help
-	  DRM panel driver for dumb panels that need at most a regulator and
-	  a GPIO to be powered up. Optionally a backlight can be attached so
-	  that it can be automatically turned off when the panel goes into a
-	  low power state.
+	  DRM panel driver for dumb panels that are not more complicated than
+	  what the common helpers provide for power on/off. Allows for mode
+	  probing via ddc bus, or using fixed modes.
 
 config DRM_PANEL_JDI_LT070ME05000
 	tristate "JDI LT070ME05000 WUXGA DSI panel"
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index a5c7ec0236e0..48c84db88c48 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -1,3 +1,4 @@
+obj-$(CONFIG_DRM_PANEL_COMMON) += panel-common.o
 obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
 obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o
 obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
diff --git a/drivers/gpu/drm/panel/panel-common.c b/drivers/gpu/drm/panel/panel-common.c
new file mode 100644
index 000000000000..2979d7439bdc
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-common.c
@@ -0,0 +1,149 @@
+/*
+ * Copyright (C) 2017 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/backlight.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+
+#include <drm/drm_panel.h>
+
+#include "panel-common.h"
+
+int panel_common_init(struct device *dev, struct panel_common *common,
+		      const char *supply_name, const char *gpio_name,
+		      const char *backlight_name)
+{
+	struct device_node *backlight;
+	int err;
+
+	common->dev = dev;
+	common->enabled = false;
+	common->prepared = false;
+
+	common->supply = devm_regulator_get(dev, supply_name);
+	if (IS_ERR(common->supply))
+		return PTR_ERR(common->supply);
+
+	common->enable_gpio = devm_gpiod_get_optional(dev, gpio_name,
+						     GPIOD_OUT_LOW);
+	if (IS_ERR(common->enable_gpio)) {
+		err = PTR_ERR(common->enable_gpio);
+		dev_err(dev, "failed to request GPIO: %d\n", err);
+		return err;
+	}
+
+	backlight = of_parse_phandle(dev->of_node, backlight_name, 0);
+	if (backlight) {
+		common->backlight = of_find_backlight_by_node(backlight);
+		of_node_put(backlight);
+
+		if (!common->backlight)
+			return -EPROBE_DEFER;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(panel_common_init);
+
+void panel_common_fini(struct panel_common *common)
+{
+	if (common->backlight)
+		put_device(&common->backlight->dev);
+}
+EXPORT_SYMBOL(panel_common_fini);
+
+int panel_common_prepare(struct panel_common *common, unsigned int delay)
+{
+	int err;
+
+	if (common->prepared)
+		return 0;
+
+	err = regulator_enable(common->supply);
+	if (err < 0) {
+		dev_err(common->dev, "failed to enable supply: %d\n", err);
+		return err;
+	}
+
+	if (common->enable_gpio)
+		gpiod_set_value_cansleep(common->enable_gpio, 1);
+
+	if (delay)
+		msleep(delay);
+
+	common->prepared = true;
+
+	return 0;
+}
+EXPORT_SYMBOL(panel_common_prepare);
+
+int panel_common_unprepare(struct panel_common *common, unsigned int delay)
+{
+	if (!common->prepared)
+		return 0;
+
+	if (common->enable_gpio)
+		gpiod_set_value_cansleep(common->enable_gpio, 0);
+
+	regulator_disable(common->supply);
+
+	if (delay)
+		msleep(delay);
+
+	common->prepared = false;
+
+	return 0;
+}
+EXPORT_SYMBOL(panel_common_unprepare);
+
+int panel_common_enable(struct panel_common *common, unsigned int delay)
+{
+	if (common->enabled)
+		return 0;
+
+	if (delay)
+		msleep(delay);
+
+	if (common->backlight) {
+		common->backlight->props.state &= ~BL_CORE_FBBLANK;
+		common->backlight->props.power = FB_BLANK_UNBLANK;
+		backlight_update_status(common->backlight);
+	}
+
+	common->enabled = true;
+
+	return 0;
+}
+EXPORT_SYMBOL(panel_common_enable);
+
+int panel_common_disable(struct panel_common *common, unsigned int delay)
+{
+	if (!common->enabled)
+		return 0;
+
+	if (common->backlight) {
+		common->backlight->props.power = FB_BLANK_POWERDOWN;
+		common->backlight->props.state |= BL_CORE_FBBLANK;
+		backlight_update_status(common->backlight);
+	}
+
+	if (delay)
+		msleep(delay);
+
+	common->enabled = false;
+
+	return 0;
+}
+EXPORT_SYMBOL(panel_common_disable);
diff --git a/drivers/gpu/drm/panel/panel-common.h b/drivers/gpu/drm/panel/panel-common.h
new file mode 100644
index 000000000000..dc3b24ac2938
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-common.h
@@ -0,0 +1,44 @@
+/*
+ * Copyright (C) 2017 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 _PANEL_COMMON_H_
+#define _PANEL_COMMON_H_
+
+struct backlight_device;
+struct regulator;
+struct i2c_adapter;
+struct gpio_desc;
+
+struct panel_common {
+	struct device *dev;
+
+	bool prepared;
+	bool enabled;
+
+	struct backlight_device *backlight;
+	struct regulator *supply;
+
+	struct gpio_desc *enable_gpio;
+};
+
+int panel_common_init(struct device *dev, struct panel_common *common,
+		      const char *supply_name, const char *gpio_name,
+		      const char *backlight_name);
+void panel_common_fini(struct panel_common *common);
+
+int panel_common_prepare(struct panel_common *common, unsigned int delay);
+int panel_common_unprepare(struct panel_common *common, unsigned int delay);
+int panel_common_enable(struct panel_common *common, unsigned int delay);
+int panel_common_disable(struct panel_common *common, unsigned int delay);
+
+#endif // _PANEL_COMMON_H_
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 89eb0422821c..1c21ff2dcd36 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -21,12 +21,9 @@
  * DEALINGS IN THE SOFTWARE.
  */
 
-#include <linux/backlight.h>
-#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 <drm/drmP.h>
 #include <drm/drm_crtc.h>
@@ -36,6 +33,8 @@
 #include <video/display_timing.h>
 #include <video/videomode.h>
 
+#include "panel-common.h"
+
 struct panel_desc {
 	const struct drm_display_mode *modes;
 	unsigned int num_modes;
@@ -77,13 +76,13 @@ struct panel_desc {
 
 struct panel_simple {
 	struct drm_panel base;
+	struct panel_common common;
+
 	bool prepared;
 	bool enabled;
 
 	const struct panel_desc *desc;
 
-	struct backlight_device *backlight;
-	struct regulator *supply;
 	struct i2c_adapter *ddc;
 
 	struct gpio_desc *enable_gpio;
@@ -163,87 +162,28 @@ static int panel_simple_disable(struct drm_panel *panel)
 {
 	struct panel_simple *p = to_panel_simple(panel);
 
-	if (!p->enabled)
-		return 0;
-
-	if (p->backlight) {
-		p->backlight->props.power = FB_BLANK_POWERDOWN;
-		p->backlight->props.state |= BL_CORE_FBBLANK;
-		backlight_update_status(p->backlight);
-	}
-
-	if (p->desc->delay.disable)
-		msleep(p->desc->delay.disable);
-
-	p->enabled = false;
-
-	return 0;
+	return panel_common_disable(&p->common, p->desc->delay.disable);
 }
 
 static int panel_simple_unprepare(struct drm_panel *panel)
 {
 	struct panel_simple *p = to_panel_simple(panel);
 
-	if (!p->prepared)
-		return 0;
-
-	if (p->enable_gpio)
-		gpiod_set_value_cansleep(p->enable_gpio, 0);
-
-	regulator_disable(p->supply);
-
-	if (p->desc->delay.unprepare)
-		msleep(p->desc->delay.unprepare);
-
-	p->prepared = false;
-
-	return 0;
+	return panel_common_unprepare(&p->common, p->desc->delay.unprepare);
 }
 
 static int panel_simple_prepare(struct drm_panel *panel)
 {
 	struct panel_simple *p = to_panel_simple(panel);
-	int err;
-
-	if (p->prepared)
-		return 0;
 
-	err = regulator_enable(p->supply);
-	if (err < 0) {
-		dev_err(panel->dev, "failed to enable supply: %d\n", err);
-		return err;
-	}
-
-	if (p->enable_gpio)
-		gpiod_set_value_cansleep(p->enable_gpio, 1);
-
-	if (p->desc->delay.prepare)
-		msleep(p->desc->delay.prepare);
-
-	p->prepared = true;
-
-	return 0;
+	return panel_common_prepare(&p->common, p->desc->delay.prepare);
 }
 
 static int panel_simple_enable(struct drm_panel *panel)
 {
 	struct panel_simple *p = to_panel_simple(panel);
 
-	if (p->enabled)
-		return 0;
-
-	if (p->desc->delay.enable)
-		msleep(p->desc->delay.enable);
-
-	if (p->backlight) {
-		p->backlight->props.state &= ~BL_CORE_FBBLANK;
-		p->backlight->props.power = FB_BLANK_UNBLANK;
-		backlight_update_status(p->backlight);
-	}
-
-	p->enabled = true;
-
-	return 0;
+	return panel_common_enable(&p->common, p->desc->delay.enable);
 }
 
 static int panel_simple_get_modes(struct drm_panel *panel)
@@ -295,7 +235,7 @@ static const struct drm_panel_funcs panel_simple_funcs = {
 
 static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 {
-	struct device_node *backlight, *ddc;
+	struct device_node *ddc;
 	struct panel_simple *panel;
 	int err;
 
@@ -303,30 +243,12 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 	if (!panel)
 		return -ENOMEM;
 
-	panel->enabled = false;
-	panel->prepared = false;
 	panel->desc = desc;
 
-	panel->supply = devm_regulator_get(dev, "power");
-	if (IS_ERR(panel->supply))
-		return PTR_ERR(panel->supply);
-
-	panel->enable_gpio = devm_gpiod_get_optional(dev, "enable",
-						     GPIOD_OUT_LOW);
-	if (IS_ERR(panel->enable_gpio)) {
-		err = PTR_ERR(panel->enable_gpio);
-		dev_err(dev, "failed to request GPIO: %d\n", err);
+	err = panel_common_init(dev, &panel->common, "supply", "gpio",
+				"backlight");
+	if (err)
 		return err;
-	}
-
-	backlight = of_parse_phandle(dev->of_node, "backlight", 0);
-	if (backlight) {
-		panel->backlight = of_find_backlight_by_node(backlight);
-		of_node_put(backlight);
-
-		if (!panel->backlight)
-			return -EPROBE_DEFER;
-	}
 
 	ddc = of_parse_phandle(dev->of_node, "ddc-i2c-bus", 0);
 	if (ddc) {
@@ -335,7 +257,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 
 		if (!panel->ddc) {
 			err = -EPROBE_DEFER;
-			goto free_backlight;
+			goto free_common;
 		}
 	}
 
@@ -354,9 +276,8 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 free_ddc:
 	if (panel->ddc)
 		put_device(&panel->ddc->dev);
-free_backlight:
-	if (panel->backlight)
-		put_device(&panel->backlight->dev);
+free_common:
+	panel_common_fini(&panel->common);
 
 	return err;
 }
@@ -373,8 +294,7 @@ static int panel_simple_remove(struct device *dev)
 	if (panel->ddc)
 		put_device(&panel->ddc->dev);
 
-	if (panel->backlight)
-		put_device(&panel->backlight->dev);
+	panel_common_fini(&panel->common);
 
 	return 0;
 }
-- 
2.12.0.367.g23dc2f6d3c-goog

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

[RFC PATCH 2/3] drm/panel: sharp-lq101r1sx01: Use panel-common helpers

[Sean Paul]

Instead of duplicating common code from panel-simple, use the panel-common helpers.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/panel/Kconfig                   |  1 +
 drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c | 79 +++++++------------------
 2 files changed, 24 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index be8590724042..98db78d22a13 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -73,6 +73,7 @@ config DRM_PANEL_SHARP_LQ101R1SX01
 	depends on OF
 	depends on DRM_MIPI_DSI
 	depends on BACKLIGHT_CLASS_DEVICE
+	select DRM_PANEL_COMMON
 	help
 	  Say Y here if you want to enable support for Sharp LQ101R1SX01
 	  TFT-LCD modules. The panel has a 2560x1600 resolution and uses
diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
index 3cce3ca19601..fb2bf67449ab 100644
--- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
@@ -6,11 +6,8 @@
  * published by the Free Software Foundation.
  */
 
-#include <linux/backlight.h>
-#include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/of.h>
-#include <linux/regulator/consumer.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
@@ -19,17 +16,17 @@
 
 #include <video/mipi_display.h>
 
+#include "panel-common.h"
+
 struct sharp_panel {
 	struct drm_panel base;
+	struct panel_common common;
+
 	/* the datasheet refers to them as DSI-LINK1 and DSI-LINK2 */
 	struct mipi_dsi_device *link1;
 	struct mipi_dsi_device *link2;
 
-	struct backlight_device *backlight;
-	struct regulator *supply;
-
 	bool prepared;
-	bool enabled;
 
 	const struct drm_display_mode *mode;
 };
@@ -93,17 +90,7 @@ static int sharp_panel_disable(struct drm_panel *panel)
 {
 	struct sharp_panel *sharp = to_sharp_panel(panel);
 
-	if (!sharp->enabled)
-		return 0;
-
-	if (sharp->backlight) {
-		sharp->backlight->props.power = FB_BLANK_POWERDOWN;
-		backlight_update_status(sharp->backlight);
-	}
-
-	sharp->enabled = false;
-
-	return 0;
+	return panel_common_disable(&sharp->common, 0);
 }
 
 static int sharp_panel_unprepare(struct drm_panel *panel)
@@ -126,11 +113,13 @@ static int sharp_panel_unprepare(struct drm_panel *panel)
 
 	msleep(120);
 
-	regulator_disable(sharp->supply);
+	err = panel_common_unprepare(&sharp->common, 0);
+	if (err < 0)
+		dev_err(panel->dev, "failed common unprepare: %d\n", err);
 
 	sharp->prepared = false;
 
-	return 0;
+	return err;
 }
 
 static int sharp_setup_symmetrical_split(struct mipi_dsi_device *left,
@@ -176,17 +165,15 @@ static int sharp_panel_prepare(struct drm_panel *panel)
 	if (sharp->prepared)
 		return 0;
 
-	err = regulator_enable(sharp->supply);
-	if (err < 0)
-		return err;
-
 	/*
 	 * According to the datasheet, the panel needs around 10 ms to fully
 	 * power up. At least another 120 ms is required before exiting sleep
 	 * mode to make sure the panel is ready. Throw in another 20 ms for
 	 * good measure.
 	 */
-	msleep(150);
+	err = panel_common_prepare(&sharp->common, 150);
+	if (err < 0)
+		return err;
 
 	err = mipi_dsi_dcs_exit_sleep_mode(sharp->link1);
 	if (err < 0) {
@@ -252,7 +239,7 @@ static int sharp_panel_prepare(struct drm_panel *panel)
 	return 0;
 
 poweroff:
-	regulator_disable(sharp->supply);
+	panel_common_unprepare(&sharp->common, 0);
 	return err;
 }
 
@@ -260,17 +247,7 @@ static int sharp_panel_enable(struct drm_panel *panel)
 {
 	struct sharp_panel *sharp = to_sharp_panel(panel);
 
-	if (sharp->enabled)
-		return 0;
-
-	if (sharp->backlight) {
-		sharp->backlight->props.power = FB_BLANK_UNBLANK;
-		backlight_update_status(sharp->backlight);
-	}
-
-	sharp->enabled = true;
-
-	return 0;
+	return panel_common_enable(&sharp->common, 0);
 }
 
 static const struct drm_display_mode default_mode = {
@@ -324,23 +301,15 @@ MODULE_DEVICE_TABLE(of, sharp_of_match);
 
 static int sharp_panel_add(struct sharp_panel *sharp)
 {
-	struct device_node *np;
 	int err;
 
 	sharp->mode = &default_mode;
+	sharp->prepared = false;
 
-	sharp->supply = devm_regulator_get(&sharp->link1->dev, "power");
-	if (IS_ERR(sharp->supply))
-		return PTR_ERR(sharp->supply);
-
-	np = of_parse_phandle(sharp->link1->dev.of_node, "backlight", 0);
-	if (np) {
-		sharp->backlight = of_find_backlight_by_node(np);
-		of_node_put(np);
-
-		if (!sharp->backlight)
-			return -EPROBE_DEFER;
-	}
+	err = panel_common_init(&sharp->link1->dev, &sharp->common, "supply",
+				"gpio", "backlight");
+	if (err < 0)
+		return err;
 
 	drm_panel_init(&sharp->base);
 	sharp->base.funcs = &sharp_panel_funcs;
@@ -348,13 +317,12 @@ static int sharp_panel_add(struct sharp_panel *sharp)
 
 	err = drm_panel_add(&sharp->base);
 	if (err < 0)
-		goto put_backlight;
+		goto err_panel;
 
 	return 0;
 
-put_backlight:
-	if (sharp->backlight)
-		put_device(&sharp->backlight->dev);
+err_panel:
+	panel_common_fini(&sharp->common);
 
 	return err;
 }
@@ -364,8 +332,7 @@ static void sharp_panel_del(struct sharp_panel *sharp)
 	if (sharp->base.dev)
 		drm_panel_remove(&sharp->base);
 
-	if (sharp->backlight)
-		put_device(&sharp->backlight->dev);
+	panel_common_fini(&sharp->common);
 
 	if (sharp->link2)
 		put_device(&sharp->link2->dev);
-- 
2.12.0.367.g23dc2f6d3c-goog

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

[RFC PATCH 3/3] drm/panel: panel-sharp-ls043t1le01: Use panel-common helpers

[Sean Paul]

Instead of duplicating common code from panel-simple, use the panel-common helpers.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/panel/Kconfig                   |  1 +
 drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c | 70 +++++++------------------
 2 files changed, 20 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 98db78d22a13..22f374f414cd 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -88,6 +88,7 @@ config DRM_PANEL_SHARP_LS043T1LE01
 	depends on OF
 	depends on DRM_MIPI_DSI
 	depends on BACKLIGHT_CLASS_DEVICE
+	select DRM_PANEL_COMMON
 	help
 	  Say Y here if you want to enable support for Sharp LS043T1LE01 qHD
 	  (540x960) DSI panel as found on the Qualcomm APQ8074 Dragonboard
diff --git a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
index 3aeb0bda4947..d89e45aa4912 100644
--- a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
@@ -18,11 +18,9 @@
  * this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <linux/backlight.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/of.h>
-#include <linux/regulator/consumer.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
@@ -31,16 +29,16 @@
 
 #include <video/mipi_display.h>
 
+#include "panel-common.h"
+
 struct sharp_nt_panel {
 	struct drm_panel base;
+	struct panel_common common;
 	struct mipi_dsi_device *dsi;
 
-	struct backlight_device *backlight;
-	struct regulator *supply;
 	struct gpio_desc *reset_gpio;
 
 	bool prepared;
-	bool enabled;
 
 	const struct drm_display_mode *mode;
 };
@@ -114,17 +112,7 @@ static int sharp_nt_panel_disable(struct drm_panel *panel)
 {
 	struct sharp_nt_panel *sharp_nt = to_sharp_nt_panel(panel);
 
-	if (!sharp_nt->enabled)
-		return 0;
-
-	if (sharp_nt->backlight) {
-		sharp_nt->backlight->props.power = FB_BLANK_POWERDOWN;
-		backlight_update_status(sharp_nt->backlight);
-	}
-
-	sharp_nt->enabled = false;
-
-	return 0;
+	return panel_common_disable(&sharp_nt->common, 0);
 }
 
 static int sharp_nt_panel_unprepare(struct drm_panel *panel)
@@ -141,7 +129,10 @@ static int sharp_nt_panel_unprepare(struct drm_panel *panel)
 		return ret;
 	}
 
-	regulator_disable(sharp_nt->supply);
+	ret = panel_common_unprepare(&sharp_nt->common, 0);
+	if (ret < 0)
+		dev_err(panel->dev, "failed common unprepare: %d\n", ret);
+
 	if (sharp_nt->reset_gpio)
 		gpiod_set_value(sharp_nt->reset_gpio, 0);
 
@@ -158,12 +149,10 @@ static int sharp_nt_panel_prepare(struct drm_panel *panel)
 	if (sharp_nt->prepared)
 		return 0;
 
-	ret = regulator_enable(sharp_nt->supply);
+	ret = panel_common_prepare(&sharp_nt->common, 20);
 	if (ret < 0)
 		return ret;
 
-	msleep(20);
-
 	if (sharp_nt->reset_gpio) {
 		gpiod_set_value(sharp_nt->reset_gpio, 1);
 		msleep(1);
@@ -190,7 +179,7 @@ static int sharp_nt_panel_prepare(struct drm_panel *panel)
 	return 0;
 
 poweroff:
-	regulator_disable(sharp_nt->supply);
+	panel_common_unprepare(&sharp_nt->common, 0);
 	if (sharp_nt->reset_gpio)
 		gpiod_set_value(sharp_nt->reset_gpio, 0);
 	return ret;
@@ -200,17 +189,7 @@ static int sharp_nt_panel_enable(struct drm_panel *panel)
 {
 	struct sharp_nt_panel *sharp_nt = to_sharp_nt_panel(panel);
 
-	if (sharp_nt->enabled)
-		return 0;
-
-	if (sharp_nt->backlight) {
-		sharp_nt->backlight->props.power = FB_BLANK_UNBLANK;
-		backlight_update_status(sharp_nt->backlight);
-	}
-
-	sharp_nt->enabled = true;
-
-	return 0;
+	return panel_common_enable(&sharp_nt->common, 0);
 }
 
 static const struct drm_display_mode default_mode = {
@@ -259,14 +238,14 @@ static const struct drm_panel_funcs sharp_nt_panel_funcs = {
 static int sharp_nt_panel_add(struct sharp_nt_panel *sharp_nt)
 {
 	struct device *dev = &sharp_nt->dsi->dev;
-	struct device_node *np;
 	int ret;
 
 	sharp_nt->mode = &default_mode;
 
-	sharp_nt->supply = devm_regulator_get(dev, "avdd");
-	if (IS_ERR(sharp_nt->supply))
-		return PTR_ERR(sharp_nt->supply);
+	ret = panel_common_init(dev, &sharp_nt->common, "avdd", "",
+				"backlight");
+	if (ret < 0)
+		return ret;
 
 	sharp_nt->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(sharp_nt->reset_gpio)) {
@@ -277,28 +256,18 @@ static int sharp_nt_panel_add(struct sharp_nt_panel *sharp_nt)
 		gpiod_set_value(sharp_nt->reset_gpio, 0);
 	}
 
-	np = of_parse_phandle(dev->of_node, "backlight", 0);
-	if (np) {
-		sharp_nt->backlight = of_find_backlight_by_node(np);
-		of_node_put(np);
-
-		if (!sharp_nt->backlight)
-			return -EPROBE_DEFER;
-	}
-
 	drm_panel_init(&sharp_nt->base);
 	sharp_nt->base.funcs = &sharp_nt_panel_funcs;
 	sharp_nt->base.dev = &sharp_nt->dsi->dev;
 
 	ret = drm_panel_add(&sharp_nt->base);
 	if (ret < 0)
-		goto put_backlight;
+		goto err_common;
 
 	return 0;
 
-put_backlight:
-	if (sharp_nt->backlight)
-		put_device(&sharp_nt->backlight->dev);
+err_common:
+	panel_common_fini(&sharp_nt->common);
 
 	return ret;
 }
@@ -308,8 +277,7 @@ static void sharp_nt_panel_del(struct sharp_nt_panel *sharp_nt)
 	if (sharp_nt->base.dev)
 		drm_panel_remove(&sharp_nt->base);
 
-	if (sharp_nt->backlight)
-		put_device(&sharp_nt->backlight->dev);
+	panel_common_fini(&sharp_nt->common);
 }
 
 static int sharp_nt_panel_probe(struct mipi_dsi_device *dsi)
-- 
2.12.0.367.g23dc2f6d3c-goog

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

Re: [RFC PATCH 0/3] drm/panel: Pull some code out into common helpers

[Sean Paul]

On Thu, Mar 16, 2017 at 6:08 PM, Sean Paul <seanpaul@chromium.org> wrote:
> This series pulls out the power-sequencing code from panel-simple into a
> panel-common helper library. This allows drivers that cannot leverage
> panel-simple to share some code.
>
> I've converted the 2 sharp mipi drivers, and Chris Zhong's driver on the
> list can also be converted. I haven't checked any other drivers, but I
> suspect we'll see the same code blocks there too.
>
> I'm sure there's more we can pull out of the various drivers, but this
> seems like a good place to start talking about how to share common panel
> code across drivers.
>

Cc: Noralf Trønnes <noralf@tronnes.org>
Cc: Daniel Vetter <daniel@ffwll.ch>

Adding Noralf to see if this might be useful for tinydrm.

Sean


> Sean
>
> Sean Paul (3):
>   drm/panel: Pull common panel code out into helpers
>   drm/panel: sharp-lq101r1sx01: Use panel-common helpers
>   drm/panel: panel-sharp-ls043t1le01: Use panel-common helpers
>
>  drivers/gpu/drm/panel/Kconfig                   |  22 +++-
>  drivers/gpu/drm/panel/Makefile                  |   1 +
>  drivers/gpu/drm/panel/panel-common.c            | 149 ++++++++++++++++++++++++
>  drivers/gpu/drm/panel/panel-common.h            |  44 +++++++
>  drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c |  79 ++++---------
>  drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c |  70 +++--------
>  drivers/gpu/drm/panel/panel-simple.c            | 112 +++---------------
>  7 files changed, 269 insertions(+), 208 deletions(-)
>  create mode 100644 drivers/gpu/drm/panel/panel-common.c
>  create mode 100644 drivers/gpu/drm/panel/panel-common.h
>
> --
> 2.12.0.367.g23dc2f6d3c-goog
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 2/3] drm/panel: sharp-lq101r1sx01: Use panel-common helpers

[Eric Anholt]

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

Sean Paul <seanpaul@chromium.org> writes:

> Instead of duplicating common code from panel-simple, use the panel-common helpers.
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/panel/Kconfig                   |  1 +
>  drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c | 79 +++++++------------------
>  2 files changed, 24 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index be8590724042..98db78d22a13 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -73,6 +73,7 @@ config DRM_PANEL_SHARP_LQ101R1SX01
>  	depends on OF
>  	depends on DRM_MIPI_DSI
>  	depends on BACKLIGHT_CLASS_DEVICE
> +	select DRM_PANEL_COMMON
>  	help
>  	  Say Y here if you want to enable support for Sharp LQ101R1SX01
>  	  TFT-LCD modules. The panel has a 2560x1600 resolution and uses
> diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
> index 3cce3ca19601..fb2bf67449ab 100644
> --- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
> +++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
> @@ -6,11 +6,8 @@
>   * published by the Free Software Foundation.
>   */
>  
> -#include <linux/backlight.h>
> -#include <linux/gpio/consumer.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> -#include <linux/regulator/consumer.h>
>  
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc.h>
> @@ -19,17 +16,17 @@
>  
>  #include <video/mipi_display.h>
>  
> +#include "panel-common.h"
> +
>  struct sharp_panel {
>  	struct drm_panel base;
> +	struct panel_common common;
> +
>  	/* the datasheet refers to them as DSI-LINK1 and DSI-LINK2 */
>  	struct mipi_dsi_device *link1;
>  	struct mipi_dsi_device *link2;
>  
> -	struct backlight_device *backlight;
> -	struct regulator *supply;
> -
>  	bool prepared;
> -	bool enabled;
>  
>  	const struct drm_display_mode *mode;
>  };
> @@ -93,17 +90,7 @@ static int sharp_panel_disable(struct drm_panel *panel)
>  {
>  	struct sharp_panel *sharp = to_sharp_panel(panel);
>  
> -	if (!sharp->enabled)
> -		return 0;
> -
> -	if (sharp->backlight) {
> -		sharp->backlight->props.power = FB_BLANK_POWERDOWN;
> -		backlight_update_status(sharp->backlight);
> -	}
> -
> -	sharp->enabled = false;
> -
> -	return 0;
> +	return panel_common_disable(&sharp->common, 0);
>  }
>  
>  static int sharp_panel_unprepare(struct drm_panel *panel)
> @@ -126,11 +113,13 @@ static int sharp_panel_unprepare(struct drm_panel *panel)
>  
>  	msleep(120);
>  
> -	regulator_disable(sharp->supply);
> +	err = panel_common_unprepare(&sharp->common, 0);
> +	if (err < 0)
> +		dev_err(panel->dev, "failed common unprepare: %d\n", err);
>  
>  	sharp->prepared = false;
>  
> -	return 0;
> +	return err;
>  }
>  
>  static int sharp_setup_symmetrical_split(struct mipi_dsi_device *left,
> @@ -176,17 +165,15 @@ static int sharp_panel_prepare(struct drm_panel *panel)
>  	if (sharp->prepared)
>  		return 0;
>  
> -	err = regulator_enable(sharp->supply);
> -	if (err < 0)
> -		return err;
> -
>  	/*
>  	 * According to the datasheet, the panel needs around 10 ms to fully
>  	 * power up. At least another 120 ms is required before exiting sleep
>  	 * mode to make sure the panel is ready. Throw in another 20 ms for
>  	 * good measure.
>  	 */
> -	msleep(150);
> +	err = panel_common_prepare(&sharp->common, 150);
> +	if (err < 0)
> +		return err;

Aside: I like how 120 + 20 = 150, because always round up your delays :)

>  
>  	err = mipi_dsi_dcs_exit_sleep_mode(sharp->link1);
>  	if (err < 0) {
> @@ -252,7 +239,7 @@ static int sharp_panel_prepare(struct drm_panel *panel)
>  	return 0;
>  
>  poweroff:
> -	regulator_disable(sharp->supply);
> +	panel_common_unprepare(&sharp->common, 0);
>  	return err;
>  }
>  
> @@ -260,17 +247,7 @@ static int sharp_panel_enable(struct drm_panel *panel)
>  {
>  	struct sharp_panel *sharp = to_sharp_panel(panel);
>  
> -	if (sharp->enabled)
> -		return 0;
> -
> -	if (sharp->backlight) {
> -		sharp->backlight->props.power = FB_BLANK_UNBLANK;
> -		backlight_update_status(sharp->backlight);
> -	}
> -
> -	sharp->enabled = true;
> -
> -	return 0;
> +	return panel_common_enable(&sharp->common, 0);
>  }
>  
>  static const struct drm_display_mode default_mode = {
> @@ -324,23 +301,15 @@ MODULE_DEVICE_TABLE(of, sharp_of_match);
>  
>  static int sharp_panel_add(struct sharp_panel *sharp)
>  {
> -	struct device_node *np;
>  	int err;
>  
>  	sharp->mode = &default_mode;
> +	sharp->prepared = false;
>  
> -	sharp->supply = devm_regulator_get(&sharp->link1->dev, "power");
> -	if (IS_ERR(sharp->supply))
> -		return PTR_ERR(sharp->supply);
> -
> -	np = of_parse_phandle(sharp->link1->dev.of_node, "backlight", 0);
> -	if (np) {
> -		sharp->backlight = of_find_backlight_by_node(np);
> -		of_node_put(np);
> -
> -		if (!sharp->backlight)
> -			return -EPROBE_DEFER;
> -	}
> +	err = panel_common_init(&sharp->link1->dev, &sharp->common, "supply",
> +				"gpio", "backlight");
> +	if (err < 0)
> +		return err;

Looks like you've incidentally changed the regulator name from "power"
to "supply".  Seems like it should be a called out in the commit
message, if intentional.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

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

Re: [RFC PATCH 0/3] drm/panel: Pull some code out into common helpers

[Eric Anholt]

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

Sean Paul <seanpaul@chromium.org> writes:

> This series pulls out the power-sequencing code from panel-simple into a
> panel-common helper library. This allows drivers that cannot leverage
> panel-simple to share some code.
>
> I've converted the 2 sharp mipi drivers, and Chris Zhong's driver on the
> list can also be converted. I haven't checked any other drivers, but I
> suspect we'll see the same code blocks there too.
>
> I'm sure there's more we can pull out of the various drivers, but this
> seems like a good place to start talking about how to share common panel
> code across drivers.

I've been looking at doing another panel driver for one with a little
SPI sequence at bringup, and this series seems like it would be nice to
have for that.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

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

Re: [RFC PATCH 2/3] drm/panel: sharp-lq101r1sx01: Use panel-common helpers

[Sean Paul]

On Tue, Mar 21, 2017 at 02:06:26PM -0700, Eric Anholt wrote:
> Sean Paul <seanpaul@chromium.org> writes:
> 
> > Instead of duplicating common code from panel-simple, use the panel-common helpers.
> >
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  drivers/gpu/drm/panel/Kconfig                   |  1 +
> >  drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c | 79 +++++++------------------
> >  2 files changed, 24 insertions(+), 56 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> > index be8590724042..98db78d22a13 100644
> > --- a/drivers/gpu/drm/panel/Kconfig
> > +++ b/drivers/gpu/drm/panel/Kconfig
> > @@ -73,6 +73,7 @@ config DRM_PANEL_SHARP_LQ101R1SX01
> >  	depends on OF
> >  	depends on DRM_MIPI_DSI
> >  	depends on BACKLIGHT_CLASS_DEVICE
> > +	select DRM_PANEL_COMMON
> >  	help
> >  	  Say Y here if you want to enable support for Sharp LQ101R1SX01
> >  	  TFT-LCD modules. The panel has a 2560x1600 resolution and uses
> > diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
> > index 3cce3ca19601..fb2bf67449ab 100644
> > --- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
> > +++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
> > @@ -6,11 +6,8 @@
> >   * published by the Free Software Foundation.
> >   */
> >  
> > -#include <linux/backlight.h>
> > -#include <linux/gpio/consumer.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > -#include <linux/regulator/consumer.h>
> >  
> >  #include <drm/drmP.h>
> >  #include <drm/drm_crtc.h>
> > @@ -19,17 +16,17 @@
> >  
> >  #include <video/mipi_display.h>
> >  
> > +#include "panel-common.h"
> > +
> >  struct sharp_panel {
> >  	struct drm_panel base;
> > +	struct panel_common common;
> > +
> >  	/* the datasheet refers to them as DSI-LINK1 and DSI-LINK2 */
> >  	struct mipi_dsi_device *link1;
> >  	struct mipi_dsi_device *link2;
> >  
> > -	struct backlight_device *backlight;
> > -	struct regulator *supply;
> > -
> >  	bool prepared;
> > -	bool enabled;
> >  
> >  	const struct drm_display_mode *mode;
> >  };
> > @@ -93,17 +90,7 @@ static int sharp_panel_disable(struct drm_panel *panel)
> >  {
> >  	struct sharp_panel *sharp = to_sharp_panel(panel);
> >  
> > -	if (!sharp->enabled)
> > -		return 0;
> > -
> > -	if (sharp->backlight) {
> > -		sharp->backlight->props.power = FB_BLANK_POWERDOWN;
> > -		backlight_update_status(sharp->backlight);
> > -	}
> > -
> > -	sharp->enabled = false;
> > -
> > -	return 0;
> > +	return panel_common_disable(&sharp->common, 0);
> >  }
> >  
> >  static int sharp_panel_unprepare(struct drm_panel *panel)
> > @@ -126,11 +113,13 @@ static int sharp_panel_unprepare(struct drm_panel *panel)
> >  
> >  	msleep(120);
> >  
> > -	regulator_disable(sharp->supply);
> > +	err = panel_common_unprepare(&sharp->common, 0);
> > +	if (err < 0)
> > +		dev_err(panel->dev, "failed common unprepare: %d\n", err);
> >  
> >  	sharp->prepared = false;
> >  
> > -	return 0;
> > +	return err;
> >  }
> >  
> >  static int sharp_setup_symmetrical_split(struct mipi_dsi_device *left,
> > @@ -176,17 +165,15 @@ static int sharp_panel_prepare(struct drm_panel *panel)
> >  	if (sharp->prepared)
> >  		return 0;
> >  
> > -	err = regulator_enable(sharp->supply);
> > -	if (err < 0)
> > -		return err;
> > -
> >  	/*
> >  	 * According to the datasheet, the panel needs around 10 ms to fully
> >  	 * power up. At least another 120 ms is required before exiting sleep
> >  	 * mode to make sure the panel is ready. Throw in another 20 ms for
> >  	 * good measure.
> >  	 */
> > -	msleep(150);
> > +	err = panel_common_prepare(&sharp->common, 150);
> > +	if (err < 0)
> > +		return err;
> 
> Aside: I like how 120 + 20 = 150, because always round up your delays :)

You missed the initial 10ms to "fully power up", so it's 10 + 120 + 20 = 150
That said, you can never have enough fudge in your delays :-)

> 
> >  
> >  	err = mipi_dsi_dcs_exit_sleep_mode(sharp->link1);
> >  	if (err < 0) {
> > @@ -252,7 +239,7 @@ static int sharp_panel_prepare(struct drm_panel *panel)
> >  	return 0;
> >  
> >  poweroff:
> > -	regulator_disable(sharp->supply);
> > +	panel_common_unprepare(&sharp->common, 0);
> >  	return err;
> >  }
> >  
> > @@ -260,17 +247,7 @@ static int sharp_panel_enable(struct drm_panel *panel)
> >  {
> >  	struct sharp_panel *sharp = to_sharp_panel(panel);
> >  
> > -	if (sharp->enabled)
> > -		return 0;
> > -
> > -	if (sharp->backlight) {
> > -		sharp->backlight->props.power = FB_BLANK_UNBLANK;
> > -		backlight_update_status(sharp->backlight);
> > -	}
> > -
> > -	sharp->enabled = true;
> > -
> > -	return 0;
> > +	return panel_common_enable(&sharp->common, 0);
> >  }
> >  
> >  static const struct drm_display_mode default_mode = {
> > @@ -324,23 +301,15 @@ MODULE_DEVICE_TABLE(of, sharp_of_match);
> >  
> >  static int sharp_panel_add(struct sharp_panel *sharp)
> >  {
> > -	struct device_node *np;
> >  	int err;
> >  
> >  	sharp->mode = &default_mode;
> > +	sharp->prepared = false;
> >  
> > -	sharp->supply = devm_regulator_get(&sharp->link1->dev, "power");
> > -	if (IS_ERR(sharp->supply))
> > -		return PTR_ERR(sharp->supply);
> > -
> > -	np = of_parse_phandle(sharp->link1->dev.of_node, "backlight", 0);
> > -	if (np) {
> > -		sharp->backlight = of_find_backlight_by_node(np);
> > -		of_node_put(np);
> > -
> > -		if (!sharp->backlight)
> > -			return -EPROBE_DEFER;
> > -	}
> > +	err = panel_common_init(&sharp->link1->dev, &sharp->common, "supply",
> > +				"gpio", "backlight");
> > +	if (err < 0)
> > +		return err;
> 
> Looks like you've incidentally changed the regulator name from "power"
> to "supply".  Seems like it should be a called out in the commit
> message, if intentional.

Oh my, yes I did, thanks for catching that. I'll fix this up locally and wait
for Thierry to weigh in before posting a v2.

Sean

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 0/3] drm/panel: Pull some code out into common helpers

[Emil Velikov]

Hi Sean,

On 16 March 2017 at 22:08, Sean Paul <seanpaul@chromium.org> wrote:
> This series pulls out the power-sequencing code from panel-simple into a
> panel-common helper library. This allows drivers that cannot leverage
> panel-simple to share some code.
>
> I've converted the 2 sharp mipi drivers, and Chris Zhong's driver on the
> list can also be converted. I haven't checked any other drivers, but I
> suspect we'll see the same code blocks there too.
>
> I'm sure there's more we can pull out of the various drivers, but this
> seems like a good place to start talking about how to share common panel
> code across drivers.
>
Fwiw I think that the idea is good, but I'm wondering on the following
architectural questions:
 - Shouldn't prepared and enabled be part of struct drm_panel ?
 - Would it be better to subclass struct panel_common around struct drm_panel ?

I might be threading the thin line of "midlayer vs helpers" here, so
please let me know if I've got it wrong.

Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 0/3] drm/panel: Pull some code out into common helpers

[Sean Paul]

On Wed, Mar 22, 2017 at 02:36:27PM +0000, Emil Velikov wrote:
> Hi Sean,
> 
> On 16 March 2017 at 22:08, Sean Paul <seanpaul@chromium.org> wrote:
> > This series pulls out the power-sequencing code from panel-simple into a
> > panel-common helper library. This allows drivers that cannot leverage
> > panel-simple to share some code.
> >
> > I've converted the 2 sharp mipi drivers, and Chris Zhong's driver on the
> > list can also be converted. I haven't checked any other drivers, but I
> > suspect we'll see the same code blocks there too.
> >
> > I'm sure there's more we can pull out of the various drivers, but this
> > seems like a good place to start talking about how to share common panel
> > code across drivers.
> >
> Fwiw I think that the idea is good, but I'm wondering on the following
> architectural questions:

Hey Emil,
Thanks for your feedback!


>  - Shouldn't prepared and enabled be part of struct drm_panel ?

I don't think so. Not all panels need to worry about keeping track of the
prepared/enabled state.

>  - Would it be better to subclass struct panel_common around struct drm_panel ?
> 
> I might be threading the thin line of "midlayer vs helpers" here, so
> please let me know if I've got it wrong.

Yeah, you could do either. I was going for something more akin to the helpers we
already have. If you went the subclass route, the drivers would need to subclass
panel_common, which would subclass drm_panel. I figured it was too much
unraveling to get at the important bits in the hooks that provide a drm_panel
pointer. Nothing that a few to_* helpers couldn't solve, though.

I was also thinking we could subclass panel_common if there's an opportunity to
get something like mipi_dcs_panel_common to reduce the copypasta amongst mipi
panels which execute a common dcs recipe.

Sean

> 
> Thanks
> Emil

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 0/3] drm/panel: Pull some code out into common helpers

[Noralf Trønnes]

Den 17.03.2017 15.17, skrev Sean Paul:
> On Thu, Mar 16, 2017 at 6:08 PM, Sean Paul <seanpaul@chromium.org> wrote:
>> This series pulls out the power-sequencing code from panel-simple into a
>> panel-common helper library. This allows drivers that cannot leverage
>> panel-simple to share some code.
>>
>> I've converted the 2 sharp mipi drivers, and Chris Zhong's driver on the
>> list can also be converted. I haven't checked any other drivers, but I
>> suspect we'll see the same code blocks there too.
>>
>> I'm sure there's more we can pull out of the various drivers, but this
>> seems like a good place to start talking about how to share common panel
>> code across drivers.
>>
> Cc: Noralf Trønnes <noralf@tronnes.org>
> Cc: Daniel Vetter <daniel@ffwll.ch>
>
> Adding Noralf to see if this might be useful for tinydrm.

tinydrm _can_ use panel_common, but it can also use drm_panel which we
decided against, so I don't know where the architectural lines go.

Most controllers/panels supported by tinydrm will have these fields:

struct mipi_dbi {
...
     bool enabled;
...
     struct gpio_desc *reset;
...
     struct backlight_device *backlight;
     struct regulator *regulator;
};

Noralf.

> Sean
>
>
>> Sean
>>
>> Sean Paul (3):
>>    drm/panel: Pull common panel code out into helpers
>>    drm/panel: sharp-lq101r1sx01: Use panel-common helpers
>>    drm/panel: panel-sharp-ls043t1le01: Use panel-common helpers
>>
>>   drivers/gpu/drm/panel/Kconfig                   |  22 +++-
>>   drivers/gpu/drm/panel/Makefile                  |   1 +
>>   drivers/gpu/drm/panel/panel-common.c            | 149 ++++++++++++++++++++++++
>>   drivers/gpu/drm/panel/panel-common.h            |  44 +++++++
>>   drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c |  79 ++++---------
>>   drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c |  70 +++--------
>>   drivers/gpu/drm/panel/panel-simple.c            | 112 +++---------------
>>   7 files changed, 269 insertions(+), 208 deletions(-)
>>   create mode 100644 drivers/gpu/drm/panel/panel-common.c
>>   create mode 100644 drivers/gpu/drm/panel/panel-common.h
>>
>> --
>> 2.12.0.367.g23dc2f6d3c-goog
>>

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

Re: [RFC PATCH 0/3] drm/panel: Pull some code out into common helpers

[Emil Velikov]

On 22 March 2017 at 15:06, Sean Paul <seanpaul@chromium.org> wrote:
> On Wed, Mar 22, 2017 at 02:36:27PM +0000, Emil Velikov wrote:
>> Hi Sean,
>>
>> On 16 March 2017 at 22:08, Sean Paul <seanpaul@chromium.org> wrote:
>> > This series pulls out the power-sequencing code from panel-simple into a
>> > panel-common helper library. This allows drivers that cannot leverage
>> > panel-simple to share some code.
>> >
>> > I've converted the 2 sharp mipi drivers, and Chris Zhong's driver on the
>> > list can also be converted. I haven't checked any other drivers, but I
>> > suspect we'll see the same code blocks there too.
>> >
>> > I'm sure there's more we can pull out of the various drivers, but this
>> > seems like a good place to start talking about how to share common panel
>> > code across drivers.
>> >
>> Fwiw I think that the idea is good, but I'm wondering on the following
>> architectural questions:
>
> Hey Emil,
> Thanks for your feedback!
>
>
>>  - Shouldn't prepared and enabled be part of struct drm_panel ?
>
> I don't think so. Not all panels need to worry about keeping track of the
> prepared/enabled state.
>
You're correct - I did not notice those before. Yet, seems like adding
the trivial prepared/enabled check will be beneficial to them ?

Unrelated tidbits/ideas, while skimming through:
 - lg-lg4573
struct lg4573::vm - never set/used.
Might want a prepare/unprepare hook to manage power on/off
 - panasonic-vvx10f034n00
struct wuxga_nt_panel::mode - set but unused
 - sharp-ls043t1le01
struct sharp_nt_panel::mode - set but unused


>>  - Would it be better to subclass struct panel_common around struct drm_panel ?
>>
>> I might be threading the thin line of "midlayer vs helpers" here, so
>> please let me know if I've got it wrong.
>
> Yeah, you could do either. I was going for something more akin to the helpers we
> already have. If you went the subclass route, the drivers would need to subclass
> panel_common, which would subclass drm_panel. I figured it was too much
> unraveling to get at the important bits in the hooks that provide a drm_panel
> pointer. Nothing that a few to_* helpers couldn't solve, though.
>
> I was also thinking we could subclass panel_common if there's an opportunity to
> get something like mipi_dcs_panel_common to reduce the copypasta amongst mipi
> panels which execute a common dcs recipe.
>
Indeed - wrapping panel_common around drm_panel might not be that good of idea.

Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 1/3] drm/panel: Pull common panel code out into helpers

[Emil Velikov]

Hi Sean,

Something small that stood out while skimming through the design.

On 16 March 2017 at 22:08, Sean Paul <seanpaul@chromium.org> wrote:

>  struct panel_simple {
>         struct drm_panel base;
> +       struct panel_common common;
> +
>         bool prepared;
>         bool enabled;
>
There two should go ?

>         const struct panel_desc *desc;
>
> -       struct backlight_device *backlight;
> -       struct regulator *supply;
>         struct i2c_adapter *ddc;
>
>         struct gpio_desc *enable_gpio;
This should go as well ?

Regards,
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 3/3] drm/panel: panel-sharp-ls043t1le01: Use panel-common helpers

[Emil Velikov]

On 16 March 2017 at 22:08, Sean Paul <seanpaul@chromium.org> wrote:

>  static const struct drm_display_mode default_mode = {
> @@ -259,14 +238,14 @@ static const struct drm_panel_funcs sharp_nt_panel_funcs = {
>  static int sharp_nt_panel_add(struct sharp_nt_panel *sharp_nt)
>  {
>         struct device *dev = &sharp_nt->dsi->dev;
> -       struct device_node *np;
>         int ret;
>
>         sharp_nt->mode = &default_mode;
>
> -       sharp_nt->supply = devm_regulator_get(dev, "avdd");
> -       if (IS_ERR(sharp_nt->supply))
> -               return PTR_ERR(sharp_nt->supply);
> +       ret = panel_common_init(dev, &sharp_nt->common, "avdd", "",
> +                               "backlight");
Skimming through the GPIO APi documentation does not say what will
happen if we have an empty string for the con_id.
Might be better to pass NULL [for either supply, gpio or backlight
name] and omit the setup of the component ?

Regards,
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel