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
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; }
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
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);
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.
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
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)
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
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
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
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.
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
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
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@lists.freedesktop.org
-
Emil Velikov -
Eric Anholt -
Noralf Trønnes -
Sean Paul