Re: [PATCH v2 2/2] drm/panel: add innolux,p079zca panel driver
+ Thierry for real On Tue, Mar 21, 2017 at 03:26:35PM -0400, Sean Paul wrote: > On Wed, Mar 15, 2017 at 03:19:13PM +0800, Chris Zhong wrote: > > Support Innolux P079ZCA 7.85" 768x1024 TFT LCD panel, it is a MIPI DSI > > panel. > > > > Signed-off-by: Chris Zhong> > --- > > > > Changes in v2: > > - add some error check > > - always use Low power mode to send commend > > - add comments for all the sleep > > - use DRM_DEV_ERROR instead of dev_err > > > > Minor suggestion below. Once that's cleared up, you can add: > Reviewed-by: Sean Paul > > Also, please add Thierry directly to your patches so he sees them. > > > > drivers/gpu/drm/panel/Kconfig | 11 + > > drivers/gpu/drm/panel/Makefile| 1 + > > drivers/gpu/drm/panel/panel-innolux-p079zca.c | 344 > > ++ > > 3 files changed, 356 insertions(+) > > create mode 100644 drivers/gpu/drm/panel/panel-innolux-p079zca.c > > > > > > +static int innolux_panel_prepare(struct drm_panel *panel) > > +{ > > + struct innolux_panel *innolux = to_innolux_panel(panel); > > + int err, ret; > > + > > + if (innolux->prepared) > > + return 0; > > + > > + gpiod_set_value_cansleep(innolux->enable_gpio, 0); > > + > > + err = regulator_enable(innolux->supply); > > + if (err < 0) > > + return err; > > + > > + /* T2: 15ms - 1000ms */ > > + usleep_range(15000, 16000); > > + > > + gpiod_set_value_cansleep(innolux->enable_gpio, 1); > > + > > + /* T4: 15ms - 1000ms */ > > + usleep_range(15000, 16000); > > + > > + err = mipi_dsi_dcs_exit_sleep_mode(innolux->link); > > + if (err < 0) { > > + DRM_DEV_ERROR(panel->dev, "failed to exit sleep mode: %d\n", > > + err); > > + goto poweroff; > > + } > > + > > + /* T6: 120ms - 1000ms*/ > > + msleep(120); > > + > > + err = mipi_dsi_dcs_set_display_on(innolux->link); > > + if (err < 0) { > > + DRM_DEV_ERROR(panel->dev, "failed to set display on: %d\n", > > + err); > > + goto poweroff; > > + } > > + > > + /* T7: 5ms */ > > + usleep_range(5000, 6000); > > + > > + innolux->prepared = true; > > + > > + return 0; > > + > > +poweroff: > > + ret = regulator_disable(innolux->supply); > > + if (ret) > > + return ret; > > I don't think we should be returning this error code. If we're here, it's > because something else triggered err, and we should return that. Change this > to: > > if (regulator_disable(innolux->supply)) > DRM_DEV_ERROR(panel->dev, "failed to disable regulator after > error\n"); And maybe include the error code in that message still? ret = regulator_disable(innolux->supply); if (ret) DRM_DEV_ERROR(panel->dev, "failed to disable regulator after error (%d)\n", ret); Brian > > + > > + gpiod_set_value_cansleep(innolux->enable_gpio, 0); > > + return err; > > +} > > > > > -- > > 2.6.3 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 2/2] drm/panel: add innolux,p079zca panel driver
On Tue, Mar 21, 2017 at 01:38:31PM -0700, Brian Norris wrote: > + Thierry for real > > On Tue, Mar 21, 2017 at 03:26:35PM -0400, Sean Paul wrote: > > On Wed, Mar 15, 2017 at 03:19:13PM +0800, Chris Zhong wrote: > > > Support Innolux P079ZCA 7.85" 768x1024 TFT LCD panel, it is a MIPI DSI > > > panel. > > > > > > Signed-off-by: Chris Zhong> > > --- > > > > > > Changes in v2: > > > - add some error check > > > - always use Low power mode to send commend > > > - add comments for all the sleep > > > - use DRM_DEV_ERROR instead of dev_err > > > > > > > Minor suggestion below. Once that's cleared up, you can add: > > Reviewed-by: Sean Paul > > > > Also, please add Thierry directly to your patches so he sees them. > > > > > > > drivers/gpu/drm/panel/Kconfig | 11 + > > > drivers/gpu/drm/panel/Makefile| 1 + > > > drivers/gpu/drm/panel/panel-innolux-p079zca.c | 344 > > > ++ > > > 3 files changed, 356 insertions(+) > > > create mode 100644 drivers/gpu/drm/panel/panel-innolux-p079zca.c > > > > > > > > > > +static int innolux_panel_prepare(struct drm_panel *panel) > > > +{ > > > + struct innolux_panel *innolux = to_innolux_panel(panel); > > > + int err, ret; > > > + > > > + if (innolux->prepared) > > > + return 0; > > > + > > > + gpiod_set_value_cansleep(innolux->enable_gpio, 0); > > > + > > > + err = regulator_enable(innolux->supply); > > > + if (err < 0) > > > + return err; > > > + > > > + /* T2: 15ms - 1000ms */ > > > + usleep_range(15000, 16000); > > > + > > > + gpiod_set_value_cansleep(innolux->enable_gpio, 1); > > > + > > > + /* T4: 15ms - 1000ms */ > > > + usleep_range(15000, 16000); > > > + > > > + err = mipi_dsi_dcs_exit_sleep_mode(innolux->link); > > > + if (err < 0) { > > > + DRM_DEV_ERROR(panel->dev, "failed to exit sleep mode: %d\n", > > > + err); > > > + goto poweroff; > > > + } > > > + > > > + /* T6: 120ms - 1000ms*/ > > > + msleep(120); > > > + > > > + err = mipi_dsi_dcs_set_display_on(innolux->link); > > > + if (err < 0) { > > > + DRM_DEV_ERROR(panel->dev, "failed to set display on: %d\n", > > > + err); > > > + goto poweroff; > > > + } > > > + > > > + /* T7: 5ms */ > > > + usleep_range(5000, 6000); > > > + > > > + innolux->prepared = true; > > > + > > > + return 0; > > > + > > > +poweroff: > > > + ret = regulator_disable(innolux->supply); > > > + if (ret) > > > + return ret; > > > > I don't think we should be returning this error code. If we're here, it's > > because something else triggered err, and we should return that. Change > > this to: > > > > if (regulator_disable(innolux->supply)) > > DRM_DEV_ERROR(panel->dev, "failed to disable regulator after > > error\n"); > > And maybe include the error code in that message still? > > ret = regulator_disable(innolux->supply); > if (ret) > DRM_DEV_ERROR(panel->dev, "failed to disable regulator after > error (%d)\n", > ret); Sure, but let's not have ret beside err, each function should have one or the other. So, perhaps s/ret/regulator_err/ or similar. Sean > > Brian > > > > + > > > + gpiod_set_value_cansleep(innolux->enable_gpio, 0); > > > + return err; > > > +} > > > > > > > > > -- > > > 2.6.3 -- 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: [PATCH v2 2/2] drm/panel: add innolux,p079zca panel driver
On Wed, Mar 15, 2017 at 03:19:13PM +0800, Chris Zhong wrote: > Support Innolux P079ZCA 7.85" 768x1024 TFT LCD panel, it is a MIPI DSI > panel. > > Signed-off-by: Chris Zhong> --- > > Changes in v2: > - add some error check > - always use Low power mode to send commend > - add comments for all the sleep > - use DRM_DEV_ERROR instead of dev_err > Minor suggestion below. Once that's cleared up, you can add: Reviewed-by: Sean Paul Also, please add Thierry directly to your patches so he sees them. > drivers/gpu/drm/panel/Kconfig | 11 + > drivers/gpu/drm/panel/Makefile| 1 + > drivers/gpu/drm/panel/panel-innolux-p079zca.c | 344 > ++ > 3 files changed, 356 insertions(+) > create mode 100644 drivers/gpu/drm/panel/panel-innolux-p079zca.c > > +static int innolux_panel_prepare(struct drm_panel *panel) > +{ > + struct innolux_panel *innolux = to_innolux_panel(panel); > + int err, ret; > + > + if (innolux->prepared) > + return 0; > + > + gpiod_set_value_cansleep(innolux->enable_gpio, 0); > + > + err = regulator_enable(innolux->supply); > + if (err < 0) > + return err; > + > + /* T2: 15ms - 1000ms */ > + usleep_range(15000, 16000); > + > + gpiod_set_value_cansleep(innolux->enable_gpio, 1); > + > + /* T4: 15ms - 1000ms */ > + usleep_range(15000, 16000); > + > + err = mipi_dsi_dcs_exit_sleep_mode(innolux->link); > + if (err < 0) { > + DRM_DEV_ERROR(panel->dev, "failed to exit sleep mode: %d\n", > + err); > + goto poweroff; > + } > + > + /* T6: 120ms - 1000ms*/ > + msleep(120); > + > + err = mipi_dsi_dcs_set_display_on(innolux->link); > + if (err < 0) { > + DRM_DEV_ERROR(panel->dev, "failed to set display on: %d\n", > + err); > + goto poweroff; > + } > + > + /* T7: 5ms */ > + usleep_range(5000, 6000); > + > + innolux->prepared = true; > + > + return 0; > + > +poweroff: > + ret = regulator_disable(innolux->supply); > + if (ret) > + return ret; I don't think we should be returning this error code. If we're here, it's because something else triggered err, and we should return that. Change this to: if (regulator_disable(innolux->supply)) DRM_DEV_ERROR(panel->dev, "failed to disable regulator after error\n"); Sean > + > + gpiod_set_value_cansleep(innolux->enable_gpio, 0); > + return err; > +} > -- > 2.6.3 > > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 2/2] drm/panel: add innolux,p079zca panel driver
On Wed, Mar 15, 2017 at 03:19:13PM +0800, Chris Zhong wrote: > Support Innolux P079ZCA 7.85" 768x1024 TFT LCD panel, it is a MIPI DSI > panel. > > Signed-off-by: Chris Zhong> --- > > Changes in v2: > - add some error check > - always use Low power mode to send commend > - add comments for all the sleep > - use DRM_DEV_ERROR instead of dev_err > > drivers/gpu/drm/panel/Kconfig | 11 + > drivers/gpu/drm/panel/Makefile| 1 + > drivers/gpu/drm/panel/panel-innolux-p079zca.c | 344 > ++ > 3 files changed, 356 insertions(+) > create mode 100644 drivers/gpu/drm/panel/panel-innolux-p079zca.c FWIW: Tested-by: Brian Norris ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 2/2] drm/panel: add innolux,p079zca panel driver
Support Innolux P079ZCA 7.85" 768x1024 TFT LCD panel, it is a MIPI DSI panel. Signed-off-by: Chris Zhong--- Changes in v2: - add some error check - always use Low power mode to send commend - add comments for all the sleep - use DRM_DEV_ERROR instead of dev_err drivers/gpu/drm/panel/Kconfig | 11 + drivers/gpu/drm/panel/Makefile| 1 + drivers/gpu/drm/panel/panel-innolux-p079zca.c | 344 ++ 3 files changed, 356 insertions(+) create mode 100644 drivers/gpu/drm/panel/panel-innolux-p079zca.c diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index 62aba97..99dd010 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -18,6 +18,17 @@ config DRM_PANEL_SIMPLE that it can be automatically turned off when the panel goes into a low power state. +config DRM_PANEL_INNOLUX_P079ZCA + tristate "Innolux P079ZCA panel" + depends on OF + depends on DRM_MIPI_DSI + depends on BACKLIGHT_CLASS_DEVICE + help + Say Y here if you want to enable support for Innolux P079ZCA + TFT-LCD modules. The panel has a 1024x768 resolution and uses + 24 bit RGB per pixel. It provides a MIPI DSI interface to + the host and has a built-in LED backlight. + config DRM_PANEL_JDI_LT070ME05000 tristate "JDI LT070ME05000 WUXGA DSI panel" depends on OF diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile index a5c7ec0..bda2aa4 100644 --- a/drivers/gpu/drm/panel/Makefile +++ b/drivers/gpu/drm/panel/Makefile @@ -1,4 +1,5 @@ obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o +obj-$(CONFIG_DRM_PANEL_INNOLUX_P079ZCA) += panel-innolux-p079zca.o obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += panel-panasonic-vvx10f034n00.o diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c b/drivers/gpu/drm/panel/panel-innolux-p079zca.c new file mode 100644 index 000..b8c34e0 --- /dev/null +++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c @@ -0,0 +1,344 @@ +/* + * Copyright (c) 2017, Fuzhou Rockchip Electronics Co., Ltd + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include +#include +#include +#include +#include + +#include +#include +#include +#include + +#include + +struct innolux_panel { + struct drm_panel base; + struct mipi_dsi_device *link; + + struct backlight_device *backlight; + struct regulator *supply; + struct gpio_desc *enable_gpio; + + bool prepared; + bool enabled; +}; + +static inline struct innolux_panel *to_innolux_panel(struct drm_panel *panel) +{ + return container_of(panel, struct innolux_panel, base); +} + +static int innolux_panel_disable(struct drm_panel *panel) +{ + struct innolux_panel *innolux = to_innolux_panel(panel); + int err; + + if (!innolux->enabled) + return 0; + + if (innolux->backlight) { + innolux->backlight->props.power = FB_BLANK_POWERDOWN; + backlight_update_status(innolux->backlight); + } + + err = mipi_dsi_dcs_set_display_off(innolux->link); + if (err < 0) + DRM_DEV_ERROR(panel->dev, "failed to set display off: %d\n", + err); + + innolux->enabled = false; + + return 0; +} + +static int innolux_panel_unprepare(struct drm_panel *panel) +{ + struct innolux_panel *innolux = to_innolux_panel(panel); + int err; + + if (!innolux->prepared) + return 0; + + err = mipi_dsi_dcs_enter_sleep_mode(innolux->link); + if (err < 0) { + DRM_DEV_ERROR(panel->dev, "failed to enter sleep mode: %d\n", + err); + return err; + } + + gpiod_set_value_cansleep(innolux->enable_gpio, 0); + + /* T8: 80ms - 1000ms */ + msleep(80); + + err = regulator_disable(innolux->supply); + if (err < 0) + return err; + + innolux->prepared = false; + + return 0; +} + +static int innolux_panel_prepare(struct drm_panel *panel) +{ + struct innolux_panel *innolux = to_innolux_panel(panel); + int err, ret; + + if (innolux->prepared) + return 0; + + gpiod_set_value_cansleep(innolux->enable_gpio, 0); + + err = regulator_enable(innolux->supply); + if (err < 0) + return err; + + /* T2: 15ms - 1000ms */ + usleep_range(15000, 16000); + + gpiod_set_value_cansleep(innolux->enable_gpio, 1); + + /* T4: 15ms - 1000ms */ +