Provide a small convenience wrapper that set/get the display brightness value
Cc: John Stultz john.stultz@linaro.org Cc: Sumit Semwal sumit.semwal@linaro.org Cc: Archit Taneja archit.taneja@gmail.com Cc: Rob Clark robdclark@gmail.com Cc: Jani Nikula jani.nikula@linux.intel.com Signed-off-by: Vinay Simha BN simhavcs@gmail.com
-- v1: *tested in nexus7 2nd gen.
v2: * implemented jani review comments -functions name mapped accordingly -bl value increased from 0xff to 0xffff -backlight interface will be handled in panel driver, so it is moved from the mipi_dsi helper function --- drivers/gpu/drm/drm_mipi_dsi.c | 49 ++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_mipi_dsi.h | 4 ++++ 2 files changed, 53 insertions(+)
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index 7938ce7..5661a00 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -1025,6 +1025,55 @@ int mipi_dsi_dcs_set_pixel_format(struct mipi_dsi_device *dsi, u8 format) } EXPORT_SYMBOL(mipi_dsi_dcs_set_pixel_format);
+/** + * mipi_dsi_dcs_get_display_brightness() - gets the current brightness value + * of the display + * @dsi: DSI peripheral device + * @brightness: brightness value + * + * Return: 0 on success or a negative error code on failure. + */ +int mipi_dsi_dcs_get_display_brightness(struct mipi_dsi_device *dsi, + u16 *brightness) +{ + ssize_t err; + + err = mipi_dsi_dcs_read(dsi, MIPI_DCS_GET_DISPLAY_BRIGHTNESS, + brightness, sizeof(*brightness)); + if (err < 0) { + if (err == 0) + err = -ENODATA; + + return err; + } + + return 0; +} +EXPORT_SYMBOL(mipi_dsi_dcs_get_display_brightness); + +/** + * mipi_dsi_dcs_set_display_brightness() - sets the brightness value of + * the display + * @dsi: DSI peripheral device + * @brightness: brightness value + * + * Return: 0 on success or a negative error code on failure. + */ +int mipi_dsi_dcs_set_display_brightness(struct mipi_dsi_device *dsi, + u16 brightness) +{ + ssize_t err; + u8 bl_value[2] = { brightness & 0xff, brightness >> 8 }; + + err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, + bl_value, sizeof(bl_value)); + if (err < 0) + return err; + + return 0; +} +EXPORT_SYMBOL(mipi_dsi_dcs_set_display_brightness); + static int mipi_dsi_drv_probe(struct device *dev) { struct mipi_dsi_driver *drv = to_mipi_dsi_driver(dev->driver); diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h index ec55285..4494641 100644 --- a/include/drm/drm_mipi_dsi.h +++ b/include/drm/drm_mipi_dsi.h @@ -268,6 +268,10 @@ int mipi_dsi_dcs_set_tear_off(struct mipi_dsi_device *dsi); int mipi_dsi_dcs_set_tear_on(struct mipi_dsi_device *dsi, enum mipi_dsi_dcs_tear_mode mode); int mipi_dsi_dcs_set_pixel_format(struct mipi_dsi_device *dsi, u8 format); +int mipi_dsi_dcs_get_display_brightness(struct mipi_dsi_device *dsi, + u16 *brightness); +int mipi_dsi_dcs_set_display_brightness(struct mipi_dsi_device *dsi, + u16 brightness);
/** * struct mipi_dsi_driver - DSI driver
Add support for the JDI lt070me05000 WUXGA DSI panel used in Nexus 7 2013 devices.
Programming sequence for the panel is was originally found in the android-msm-flo-3.4-lollipop-release branch from: https://android.googlesource.com/kernel/msm.git
And video mode setting is from dsi-panel-jdi-dualmipi1-video.dtsi file in: git://codeaurora.org/kernel/msm-3.10.git LNX.LA.3.6_rb1.27
Cc: Archit Taneja archit.taneja@gmail.com [sumit.semwal: Ported to the drm/panel framework] Signed-off-by: Sumit Semwal sumit.semwal@linaro.org [jstultz: Cherry-picked to mainline, folded down other fixes from Vinay and Archit] Signed-off-by: John Stultz john.stultz@linaro.org [vinay simha bn: removed interface setting cmd mode, video mode panel setting selection] Cc: Rob Clark robdclark@gmail.com Signed-off-by: Vinay Simha BN simhavcs@gmail.com -- v2: * incorporated code reviews from theiry, archit code style, alphabetical soring in Makefile, Kconfig, regulator_bulk, arrays of u8, generic helper function, documentation bindings,
v3: * dcs backlight support added * tested this panel driver in nexus7 2013 device
v4: * backlight interface added in the panel driver * incorporated width_mm and height_mm suggested by rob herring --- drivers/gpu/drm/panel/Kconfig | 11 + drivers/gpu/drm/panel/Makefile | 1 + drivers/gpu/drm/panel/panel-jdi-lt070me05000.c | 539 +++++++++++++++++++++++++ 3 files changed, 551 insertions(+) create mode 100644 drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index 1500ab9..83e89e7 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -7,6 +7,17 @@ config DRM_PANEL menu "Display Panels" depends on DRM && DRM_PANEL
+config DRM_PANEL_JDI_LT070ME05000 + tristate "JDI LT070ME05000 WUXGA DSI 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 JDI WUXGA DSI video + mode panel as found in Google Nexus 7 (2013) devices. + The panel has a 1200(RGB)×1920 (WUXGA) resolution and uses + 24 bit RGB per pixel. + config DRM_PANEL_SIMPLE tristate "support for simple panels" depends on OF diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile index f277eed..5d74ac2 100644 --- a/drivers/gpu/drm/panel/Makefile +++ b/drivers/gpu/drm/panel/Makefile @@ -1,3 +1,4 @@ +obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.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-jdi-lt070me05000.c b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file mode 100644 index 0000000..8657e5f --- /dev/null +++ b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c @@ -0,0 +1,539 @@ +/* + * Copyright (C) 2016 InforceComputing + * Author: Vinay Simha BN simhavcs@gmail.com + * + * Copyright (C) 2016 Linaro Ltd + * Author: Sumit Semwal sumit.semwal@linaro.org + * + * From internet archives, the panel for Nexus 7 2nd Gen, 2013 model is a + * JDI model LT070ME05000, and its data sheet is at: + * http://panelone.net/en/7-0-inch/JDI_LT070ME05000_7.0_inch-datasheet + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published by + * the Free Software Foundation. + * + * 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. + * + * You should have received a copy of the GNU General Public License along with + * 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> +#include <drm/drm_mipi_dsi.h> +#include <drm/drm_panel.h> + +#include <video/mipi_display.h> + +#define PANEL_DEV_REGULATOR_MAX 3 +#define PANEL_MAX_BRIGHTNESS 0xFF + +const char *regs[] = { + "vddp", + "dcdc_en", + "vcc" +}; + +struct jdi_panel { + struct drm_panel base; + struct mipi_dsi_device *dsi; + + struct regulator_bulk_data supplies[PANEL_DEV_REGULATOR_MAX]; + + struct gpio_desc *reset_gpio; + struct gpio_desc *enable_gpio; + struct backlight_device *backlight; + + bool prepared; + bool enabled; + + const struct drm_display_mode *mode; +}; + +static inline struct jdi_panel *to_jdi_panel(struct drm_panel *panel) +{ + return container_of(panel, struct jdi_panel, base); +} + +static int jdi_panel_init(struct jdi_panel *jdi) +{ + struct mipi_dsi_device *dsi = jdi->dsi; + int ret; + + dsi->mode_flags |= MIPI_DSI_MODE_LPM; + + ret = mipi_dsi_dcs_soft_reset(dsi); + if (ret < 0) + return ret; + + usleep_range(10000, 20000); + + ret = mipi_dsi_dcs_set_pixel_format(dsi, 0x70); + if (ret < 0) + return ret; + + ret = mipi_dsi_dcs_set_column_address(dsi, 0x0000, 0x04AF); + if (ret < 0) + return ret; + + ret = mipi_dsi_dcs_set_page_address(dsi, 0x0000, 0x077F); + if (ret < 0) + return ret; + + ret = mipi_dsi_dcs_set_tear_on(dsi, + MIPI_DSI_DCS_TEAR_MODE_VBLANK); + if (ret < 0) + return ret; + usleep_range(5000, 10000); + + ret = mipi_dsi_set_tear_scanline(dsi, 0x0300); + if (ret < 0) + return ret; + + ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, + (u8[]){ 0xFF }, 1); + if (ret < 0) + return ret; + + ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, + (u8[]){ 0x24 }, 1); + if (ret < 0) + return ret; + + ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_POWER_SAVE, + (u8[]){ 0x00 }, 1); + if (ret < 0) + return ret; + + ret = mipi_dsi_dcs_exit_sleep_mode(dsi); + if (ret < 0) + return ret; + mdelay(120); + + ret = mipi_dsi_generic_write(dsi, (u8[]){0xB0, 0x00}, 2); + if (ret < 0) + return ret; + mdelay(10); + + ret = mipi_dsi_generic_write(dsi, (u8[]) + {0xB3, 0x26, 0x08, 0x00, 0x20, 0x00}, 6); + if (ret < 0) + return ret; + mdelay(20); + + ret = mipi_dsi_generic_write(dsi, (u8[]){0xB0, 0x03}, 2); + if (ret < 0) + return ret; + + return 0; +} + +static int jdi_panel_on(struct jdi_panel *jdi) +{ + struct mipi_dsi_device *dsi = jdi->dsi; + int ret; + + dsi->mode_flags |= MIPI_DSI_MODE_LPM; + + ret = mipi_dsi_dcs_set_display_on(dsi); + if (ret < 0) + return ret; + + return 0; +} + +static int jdi_panel_off(struct jdi_panel *jdi) +{ + struct mipi_dsi_device *dsi = jdi->dsi; + int ret; + + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; + + ret = mipi_dsi_dcs_set_display_off(dsi); + if (ret < 0) + return ret; + + ret = mipi_dsi_dcs_enter_sleep_mode(dsi); + if (ret < 0) + return ret; + + ret = mipi_dsi_dcs_set_tear_off(dsi); + if (ret < 0) + return ret; + + msleep(100); + + return 0; +} + +static int jdi_panel_disable(struct drm_panel *panel) +{ + struct jdi_panel *jdi = to_jdi_panel(panel); + + if (!jdi->enabled) + return 0; + + DRM_DEBUG("disable\n"); + + if (jdi->backlight) { + jdi->backlight->props.power = FB_BLANK_POWERDOWN; + backlight_update_status(jdi->backlight); + } + + jdi->enabled = false; + + return 0; +} + +static int jdi_panel_unprepare(struct drm_panel *panel) +{ + struct jdi_panel *jdi = to_jdi_panel(panel); + struct regulator_bulk_data *s = jdi->supplies; + int num = PANEL_DEV_REGULATOR_MAX; + struct device *dev = &jdi->dsi->dev; + int ret; + + if (!jdi->prepared) + return 0; + + DRM_DEBUG("unprepare\n"); + + ret = jdi_panel_off(jdi); + if (ret) { + dev_err(panel->dev, "failed to set panel off: %d\n", ret); + return ret; + } + + ret = regulator_bulk_disable(num, s); + if (ret < 0) { + dev_err(dev, "regulator disable failed, %d\n", ret); + return ret; + } + + if (jdi->reset_gpio) + gpiod_set_value(jdi->reset_gpio, 0); + + if (jdi->enable_gpio) + gpiod_set_value(jdi->enable_gpio, 0); + + jdi->prepared = false; + + return 0; +} + +static int jdi_panel_prepare(struct drm_panel *panel) +{ + struct jdi_panel *jdi = to_jdi_panel(panel); + struct regulator_bulk_data *s = jdi->supplies; + int num = PANEL_DEV_REGULATOR_MAX; + struct device *dev = &jdi->dsi->dev; + int ret; + + if (jdi->prepared) + return 0; + + DRM_DEBUG("prepare\n"); + + ret = regulator_bulk_enable(num, s); + if (ret < 0) { + dev_err(dev, "regulator enable failed, %d\n", ret); + return ret; + } + + msleep(20); + + if (jdi->reset_gpio) { + gpiod_set_value(jdi->reset_gpio, 1); + usleep_range(10, 20); + } + + if (jdi->enable_gpio) { + gpiod_set_value(jdi->enable_gpio, 1); + usleep_range(10, 20); + } + + ret = jdi_panel_init(jdi); + if (ret) { + dev_err(panel->dev, "failed to init panel: %d\n", ret); + goto poweroff; + } + + ret = jdi_panel_on(jdi); + if (ret) { + dev_err(panel->dev, "failed to set panel on: %d\n", ret); + goto poweroff; + } + + jdi->prepared = true; + + return 0; + +poweroff: + if (jdi->reset_gpio) + gpiod_set_value(jdi->reset_gpio, 0); + if (jdi->enable_gpio) + gpiod_set_value(jdi->enable_gpio, 0); + + return ret; +} + +static int jdi_panel_enable(struct drm_panel *panel) +{ + struct jdi_panel *jdi = to_jdi_panel(panel); + + if (jdi->enabled) + return 0; + + DRM_DEBUG("enable\n"); + + if (jdi->backlight) { + jdi->backlight->props.power = FB_BLANK_UNBLANK; + backlight_update_status(jdi->backlight); + } + + jdi->enabled = true; + + return 0; +} + +static const struct drm_display_mode default_mode = { + .clock = 155493, + .hdisplay = 1200, + .hsync_start = 1200 + 48, + .hsync_end = 1200 + 48 + 32, + .htotal = 1200 + 48 + 32 + 60, + .vdisplay = 1920, + .vsync_start = 1920 + 3, + .vsync_end = 1920 + 3 + 5, + .vtotal = 1920 + 3 + 5 + 6, + .vrefresh = 60, + .flags = 0, + .width_mm = 95, + .height_mm = 151, +}; + +static int jdi_panel_get_modes(struct drm_panel *panel) +{ + struct drm_display_mode *mode; + struct jdi_panel *jdi = to_jdi_panel(panel); + struct device *dev = &jdi->dsi->dev; + + mode = drm_mode_duplicate(panel->drm, &default_mode); + if (!mode) { + dev_err(dev, "failed to add mode %ux%ux@%u\n", + default_mode.hdisplay, default_mode.vdisplay, + default_mode.vrefresh); + return -ENOMEM; + } + + drm_mode_set_name(mode); + + drm_mode_probed_add(panel->connector, mode); + + return 1; +} + +static int dsi_dcs_bl_get_brightness(struct backlight_device *bl) +{ + struct mipi_dsi_device *dsi = bl_get_data(bl); + int ret; + u16 brightness; + u8 bl_value[2]; + + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; + + ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness); + if (ret < 0) + return ret; + + dsi->mode_flags |= MIPI_DSI_MODE_LPM; + + bl_value[0] = brightness & 0xff; + bl_value[1] = brightness >> 8; + + return bl_value[0]; +} + +static int dsi_dcs_bl_update_status(struct backlight_device *bl) +{ + struct mipi_dsi_device *dsi = bl_get_data(bl); + int ret; + u16 brightness = bl->props.brightness; + + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; + + ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness); + if (ret < 0) + return ret; + + dsi->mode_flags |= MIPI_DSI_MODE_LPM; + + return 0; +} + +static const struct backlight_ops dsi_bl_ops = { + .update_status = dsi_dcs_bl_update_status, + .get_brightness = dsi_dcs_bl_get_brightness, +}; + +struct backlight_device + *drm_panel_create_dsi_backlight(struct mipi_dsi_device *dsi) +{ + struct device *dev = &dsi->dev; + struct backlight_properties props; + + memset(&props, 0, sizeof(props)); + props.type = BACKLIGHT_RAW; + props.brightness = PANEL_MAX_BRIGHTNESS; + props.max_brightness = PANEL_MAX_BRIGHTNESS; + + return devm_backlight_device_register(dev, dev_name(dev), dev, dsi, + &dsi_bl_ops, &props); +} + +static const struct drm_panel_funcs jdi_panel_funcs = { + .disable = jdi_panel_disable, + .unprepare = jdi_panel_unprepare, + .prepare = jdi_panel_prepare, + .enable = jdi_panel_enable, + .get_modes = jdi_panel_get_modes, +}; + +static const struct of_device_id jdi_of_match[] = { + { .compatible = "jdi,lt070me05000", }, + { } +}; +MODULE_DEVICE_TABLE(of, jdi_of_match); + +static int jdi_panel_add(struct jdi_panel *jdi) +{ + struct device *dev = &jdi->dsi->dev; + struct regulator_bulk_data *s = jdi->supplies; + int num = PANEL_DEV_REGULATOR_MAX; + int i, ret; + + jdi->mode = &default_mode; + + for (i = 0; i < num; i++) + s[i].supply = regs[i]; + + ret = devm_regulator_bulk_get(dev, num, s); + if (ret < 0) { + dev_err(dev, "%s: failed to init regulator, ret=%d\n", + __func__, ret); + return ret; + } + + jdi->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); + if (IS_ERR(jdi->reset_gpio)) { + dev_err(dev, "cannot get reset-gpios %ld\n", + PTR_ERR(jdi->reset_gpio)); + jdi->reset_gpio = NULL; + } else { + gpiod_direction_output(jdi->reset_gpio, 0); + } + + jdi->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW); + if (IS_ERR(jdi->enable_gpio)) { + dev_err(dev, "cannot get enable-gpio %ld\n", + PTR_ERR(jdi->enable_gpio)); + jdi->enable_gpio = NULL; + } else { + gpiod_direction_output(jdi->enable_gpio, 0); + } + + jdi->backlight = drm_panel_create_dsi_backlight(jdi->dsi); + + drm_panel_init(&jdi->base); + jdi->base.funcs = &jdi_panel_funcs; + jdi->base.dev = &jdi->dsi->dev; + + ret = drm_panel_add(&jdi->base); + if (ret < 0) + return ret; + + return 0; +} + +static void jdi_panel_del(struct jdi_panel *jdi) +{ + if (jdi->base.dev) + drm_panel_remove(&jdi->base); +} + +static int jdi_panel_probe(struct mipi_dsi_device *dsi) +{ + struct jdi_panel *jdi; + int ret; + + dsi->lanes = 4; + dsi->format = MIPI_DSI_FMT_RGB888; + dsi->mode_flags = MIPI_DSI_MODE_VIDEO_HSE | MIPI_DSI_MODE_VIDEO | + MIPI_DSI_CLOCK_NON_CONTINUOUS; + + jdi = devm_kzalloc(&dsi->dev, sizeof(*jdi), GFP_KERNEL); + if (!jdi) + return -ENOMEM; + + mipi_dsi_set_drvdata(dsi, jdi); + + jdi->dsi = dsi; + + ret = jdi_panel_add(jdi); + if (ret < 0) + return ret; + + return mipi_dsi_attach(dsi); +} + +static int jdi_panel_remove(struct mipi_dsi_device *dsi) +{ + struct jdi_panel *jdi = mipi_dsi_get_drvdata(dsi); + int ret; + + ret = jdi_panel_disable(&jdi->base); + if (ret < 0) + dev_err(&dsi->dev, "failed to disable panel: %d\n", ret); + + ret = mipi_dsi_detach(dsi); + if (ret < 0) + dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", + ret); + + drm_panel_detach(&jdi->base); + jdi_panel_del(jdi); + + return 0; +} + +static void jdi_panel_shutdown(struct mipi_dsi_device *dsi) +{ + struct jdi_panel *jdi = mipi_dsi_get_drvdata(dsi); + + jdi_panel_disable(&jdi->base); +} + +static struct mipi_dsi_driver jdi_panel_driver = { + .driver = { + .name = "panel-jdi-lt070me05000", + .of_match_table = jdi_of_match, + }, + .probe = jdi_panel_probe, + .remove = jdi_panel_remove, + .shutdown = jdi_panel_shutdown, +}; +module_mipi_dsi_driver(jdi_panel_driver); + +MODULE_AUTHOR("Sumit Semwal sumit.semwal@linaro.org"); +MODULE_AUTHOR("Vinay Simha BN simhavcs@gmail.com"); +MODULE_DESCRIPTION("JDI WUXGA LT070ME05000 DSI video mode panel driver"); +MODULE_LICENSE("GPL v2");
On Mon, Jun 13, 2016 at 03:55:28PM +0530, Vinay Simha BN wrote:
Add support for the JDI lt070me05000 WUXGA DSI panel used in
Can you please make the names consistent? Use the all-uppercase spelling for the panel model.
Nexus 7 2013 devices.
Programming sequence for the panel is was originally found in the android-msm-flo-3.4-lollipop-release branch from: https://android.googlesource.com/kernel/msm.git
And video mode setting is from dsi-panel-jdi-dualmipi1-video.dtsi file in: git://codeaurora.org/kernel/msm-3.10.git LNX.LA.3.6_rb1.27
Cc: Archit Taneja archit.taneja@gmail.com [sumit.semwal: Ported to the drm/panel framework] Signed-off-by: Sumit Semwal sumit.semwal@linaro.org [jstultz: Cherry-picked to mainline, folded down other fixes from Vinay and Archit] Signed-off-by: John Stultz john.stultz@linaro.org [vinay simha bn: removed interface setting cmd mode, video mode panel setting selection] Cc: Rob Clark robdclark@gmail.com Signed-off-by: Vinay Simha BN simhavcs@gmail.com -- v2:
- incorporated code reviews from theiry, archit code style, alphabetical soring in Makefile, Kconfig, regulator_bulk, arrays of u8, generic helper function, documentation bindings,
v3:
- dcs backlight support added
- tested this panel driver in nexus7 2013 device
v4:
- backlight interface added in the panel driver
- incorporated width_mm and height_mm suggested by rob herring
drivers/gpu/drm/panel/Kconfig | 11 + drivers/gpu/drm/panel/Makefile | 1 + drivers/gpu/drm/panel/panel-jdi-lt070me05000.c | 539 +++++++++++++++++++++++++ 3 files changed, 551 insertions(+) create mode 100644 drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index 1500ab9..83e89e7 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -7,6 +7,17 @@ config DRM_PANEL menu "Display Panels" depends on DRM && DRM_PANEL
+config DRM_PANEL_JDI_LT070ME05000
tristate "JDI LT070ME05000 WUXGA DSI panel"depends on OFdepends on DRM_MIPI_DSIdepends on BACKLIGHT_CLASS_DEVICEhelp
Please use consistent indentation here. Also, this is badly sorted. The DRM_PANEL_SIMPLE is special in that it doesn't have a vendor prefix, all others should be sorted after panel-simple and in alphabetical order by vendor, then model.
Say Y here if you want to enable support for JDI WUXGA DSI videomode panel as found in Google Nexus 7 (2013) devices.The panel has a 1200(RGB)×1920 (WUXGA) resolution and uses24 bit RGB per pixel.
It's kind of redundant to say 1200(RGB) when you say that it's 24 bit RGB afterwards. You also repeat WUXGA twice, so you can drop either of the occurrences as well.
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile index f277eed..5d74ac2 100644 --- a/drivers/gpu/drm/panel/Makefile +++ b/drivers/gpu/drm/panel/Makefile @@ -1,3 +1,4 @@ +obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o
Please sort this the same way as the Kconfig entries.
obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.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-jdi-lt070me05000.c b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file mode 100644 index 0000000..8657e5f --- /dev/null +++ b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c @@ -0,0 +1,539 @@ +/*
- Copyright (C) 2016 InforceComputing
- Author: Vinay Simha BN simhavcs@gmail.com
- Copyright (C) 2016 Linaro Ltd
- Author: Sumit Semwal sumit.semwal@linaro.org
- From internet archives, the panel for Nexus 7 2nd Gen, 2013 model is a
- JDI model LT070ME05000, and its data sheet is at:
- This program is free software; you can redistribute it and/or modify it
- under the terms of the GNU General Public License version 2 as published by
- the Free Software Foundation.
- 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.
- You should have received a copy of the GNU General Public License along with
- 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> +#include <drm/drm_mipi_dsi.h> +#include <drm/drm_panel.h>
+#include <video/mipi_display.h>
+#define PANEL_DEV_REGULATOR_MAX 3
_MAX suggests that the number of regulators is actually variable, but it's not. PANEL_NUM_REGULATORS, perhaps?
+#define PANEL_MAX_BRIGHTNESS 0xFF
There isn't really a need for this macro, you only use the constant in one location. Also, a brightness is usually represented in decimal.
+const char *regs[] = {
- "vddp",
- "dcdc_en",
- "vcc"
+};
This should be static. Also use a more sensible name, such as regulator_names, please.
+static int jdi_panel_init(struct jdi_panel *jdi) +{
[...]
- struct mipi_dsi_device *dsi = jdi->dsi;
- int ret;
- dsi->mode_flags |= MIPI_DSI_MODE_LPM;
- ret = mipi_dsi_dcs_soft_reset(dsi);
- if (ret < 0)
return ret;- usleep_range(10000, 20000);
- ret = mipi_dsi_dcs_set_pixel_format(dsi, 0x70);
- if (ret < 0)
return ret;
Please use the existing symbolic constants for this.
- ret = mipi_dsi_dcs_set_column_address(dsi, 0x0000, 0x04AF);
- if (ret < 0)
return ret;- ret = mipi_dsi_dcs_set_page_address(dsi, 0x0000, 0x077F);
- if (ret < 0)
return ret;
Please parameterize these using the panel width and height.
- ret = mipi_dsi_dcs_set_tear_on(dsi,
MIPI_DSI_DCS_TEAR_MODE_VBLANK);- if (ret < 0)
return ret;- usleep_range(5000, 10000);
Use a blank line after conditional blocks, please.
- ret = mipi_dsi_set_tear_scanline(dsi, 0x0300);
- if (ret < 0)
return ret;
Might be worth making this a decimal. But then the question begs: why is 768 a good value for the tear scanline? Also, should you not set the tear scanline before enabling tear mode?
- ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
(u8[]){ 0xFF }, 1);- if (ret < 0)
return ret;
Is this necessary? Shouldn't the backlight implementation set this upon registration already?
- ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,
(u8[]){ 0x24 }, 1);- if (ret < 0)
return ret;- ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_POWER_SAVE,
(u8[]){ 0x00 }, 1);- if (ret < 0)
return ret;
What do these mean?
- ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
- if (ret < 0)
return ret;- mdelay(120);
Again, could use an extra blank line for readability. Also, don't ever use mdelay() for such large delays! Use msleep() instead.
- ret = mipi_dsi_generic_write(dsi, (u8[]){0xB0, 0x00}, 2);
- if (ret < 0)
return ret;- mdelay(10);
Same here. This also needs at least a comment, though ideally you'd use symbolic names for those magic numbers.
- ret = mipi_dsi_generic_write(dsi, (u8[])
{0xB3, 0x26, 0x08, 0x00, 0x20, 0x00}, 6);- if (ret < 0)
return ret;- mdelay(20);
And here.
+static int jdi_panel_unprepare(struct drm_panel *panel) +{
- struct jdi_panel *jdi = to_jdi_panel(panel);
- struct regulator_bulk_data *s = jdi->supplies;
- int num = PANEL_DEV_REGULATOR_MAX;
- struct device *dev = &jdi->dsi->dev;
- int ret;
- if (!jdi->prepared)
return 0;- DRM_DEBUG("unprepare\n");
- ret = jdi_panel_off(jdi);
- if (ret) {
dev_err(panel->dev, "failed to set panel off: %d\n", ret);return ret;- }
- ret = regulator_bulk_disable(num, s);
- if (ret < 0) {
dev_err(dev, "regulator disable failed, %d\n", ret);return ret;- }
- if (jdi->reset_gpio)
gpiod_set_value(jdi->reset_gpio, 0);
That's odd. Wouldn't you typically assert the reset in ->unprepare()? Perhaps you need to mark the GPIO as GPIO_ACTIVE_LOW?
- if (jdi->enable_gpio)
gpiod_set_value(jdi->enable_gpio, 0);- jdi->prepared = false;
- return 0;
+}
+static int jdi_panel_prepare(struct drm_panel *panel) +{
- struct jdi_panel *jdi = to_jdi_panel(panel);
- struct regulator_bulk_data *s = jdi->supplies;
There's no need for this temporary variable, jdi->supplies isn't terrible long.
- int num = PANEL_DEV_REGULATOR_MAX;
This should be unsigned int. Also, might be worth to simply drop this usage and do...
- struct device *dev = &jdi->dsi->dev;
- int ret;
- if (jdi->prepared)
return 0;- DRM_DEBUG("prepare\n");
- ret = regulator_bulk_enable(num, s);
ret = regulator_bulk_enable(ARRAY_SIZE(jdi->supplies), jdi->supplies);
here.
- if (ret < 0) {
dev_err(dev, "regulator enable failed, %d\n", ret);
Can you please be more consistent with how you report errors? Other drivers use "...: %d\n", ret and you do so yourself in other parts of this same driver, so make them all consistent, please.
+static int jdi_panel_enable(struct drm_panel *panel) +{
- struct jdi_panel *jdi = to_jdi_panel(panel);
- if (jdi->enabled)
return 0;- DRM_DEBUG("enable\n");
Do you really need these? You can use the kernel's built-in function tracing for the same purpose.
+static const struct drm_display_mode default_mode = {
.clock = 155493,.hdisplay = 1200,.hsync_start = 1200 + 48,.hsync_end = 1200 + 48 + 32,.htotal = 1200 + 48 + 32 + 60,.vdisplay = 1920,.vsync_start = 1920 + 3,.vsync_end = 1920 + 3 + 5,.vtotal = 1920 + 3 + 5 + 6,.vrefresh = 60,.flags = 0,.width_mm = 95,.height_mm = 151,
It's slightly unusual to set this in the mode. Typically you'd set the connector->display_info.{width,height}_mm directly.
+static int dsi_dcs_bl_get_brightness(struct backlight_device *bl) +{
- struct mipi_dsi_device *dsi = bl_get_data(bl);
- int ret;
- u16 brightness;
- u8 bl_value[2];
- dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
- ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
- if (ret < 0)
return ret;- dsi->mode_flags |= MIPI_DSI_MODE_LPM;
- bl_value[0] = brightness & 0xff;
- bl_value[1] = brightness >> 8;
- return bl_value[0];
+}
Why not simply return "brightness & 0xff" here?
+static int dsi_dcs_bl_update_status(struct backlight_device *bl) +{
- struct mipi_dsi_device *dsi = bl_get_data(bl);
- int ret;
- u16 brightness = bl->props.brightness;
There's no need for this temporary variable.
- dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
- ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
- if (ret < 0)
return ret;- dsi->mode_flags |= MIPI_DSI_MODE_LPM;
- return 0;
+}
[...]
+struct backlight_device
*drm_panel_create_dsi_backlight(struct mipi_dsi_device *dsi)
Your indentation is wrong here. The * should go on the same line as the type, and the function name should start in column 1.
+{
- struct device *dev = &dsi->dev;
- struct backlight_properties props;
- memset(&props, 0, sizeof(props));
- props.type = BACKLIGHT_RAW;
- props.brightness = PANEL_MAX_BRIGHTNESS;
- props.max_brightness = PANEL_MAX_BRIGHTNESS;
Just use 255 for the maximum and default brightness here.
+static int jdi_panel_add(struct jdi_panel *jdi) +{
- struct device *dev = &jdi->dsi->dev;
- struct regulator_bulk_data *s = jdi->supplies;
- int num = PANEL_DEV_REGULATOR_MAX;
Again, no need for these two temporary variables.
- int i, ret;
i should be unsigned int.
- jdi->mode = &default_mode;
- for (i = 0; i < num; i++)
s[i].supply = regs[i];- ret = devm_regulator_bulk_get(dev, num, s);
- if (ret < 0) {
dev_err(dev, "%s: failed to init regulator, ret=%d\n",__func__, ret);return ret;- }
- jdi->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
- if (IS_ERR(jdi->reset_gpio)) {
dev_err(dev, "cannot get reset-gpios %ld\n",PTR_ERR(jdi->reset_gpio));
This is a third variant of error reporting. Please stick to one.
jdi->reset_gpio = NULL;- } else {
gpiod_direction_output(jdi->reset_gpio, 0);- }
- jdi->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
- if (IS_ERR(jdi->enable_gpio)) {
dev_err(dev, "cannot get enable-gpio %ld\n",PTR_ERR(jdi->enable_gpio));jdi->enable_gpio = NULL;- } else {
gpiod_direction_output(jdi->enable_gpio, 0);- }
- jdi->backlight = drm_panel_create_dsi_backlight(jdi->dsi);
You should check for errors here.
+MODULE_AUTHOR("Vinay Simha BN simhavcs@gmail.com"); +MODULE_DESCRIPTION("JDI WUXGA LT070ME05000 DSI video mode panel driver");
The commit message names this "JDI LT070ME05000 WUXGA", please use the same here.
Thierry
thierry,
reviews are incorportated in v4, need some clarification on few reviews.
On Mon, Jun 13, 2016 at 6:05 PM, Thierry Reding thierry.reding@gmail.com wrote:
On Mon, Jun 13, 2016 at 03:55:28PM +0530, Vinay Simha BN wrote:
Add support for the JDI lt070me05000 WUXGA DSI panel used in
Can you please make the names consistent? Use the all-uppercase spelling for the panel model.
Nexus 7 2013 devices.
Programming sequence for the panel is was originally found in the android-msm-flo-3.4-lollipop-release branch from: https://android.googlesource.com/kernel/msm.git
And video mode setting is from dsi-panel-jdi-dualmipi1-video.dtsi file in: git://codeaurora.org/kernel/msm-3.10.git LNX.LA.3.6_rb1.27
Cc: Archit Taneja archit.taneja@gmail.com [sumit.semwal: Ported to the drm/panel framework] Signed-off-by: Sumit Semwal sumit.semwal@linaro.org [jstultz: Cherry-picked to mainline, folded down other fixes from Vinay and Archit] Signed-off-by: John Stultz john.stultz@linaro.org [vinay simha bn: removed interface setting cmd mode, video mode panel setting selection] Cc: Rob Clark robdclark@gmail.com Signed-off-by: Vinay Simha BN simhavcs@gmail.com -- v2:
- incorporated code reviews from theiry, archit code style, alphabetical soring in Makefile, Kconfig, regulator_bulk, arrays of u8, generic helper function, documentation bindings,
v3:
- dcs backlight support added
- tested this panel driver in nexus7 2013 device
v4:
- backlight interface added in the panel driver
- incorporated width_mm and height_mm suggested by rob herring
drivers/gpu/drm/panel/Kconfig | 11 + drivers/gpu/drm/panel/Makefile | 1 + drivers/gpu/drm/panel/panel-jdi-lt070me05000.c | 539 +++++++++++++++++++++++++ 3 files changed, 551 insertions(+) create mode 100644 drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index 1500ab9..83e89e7 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -7,6 +7,17 @@ config DRM_PANEL menu "Display Panels" depends on DRM && DRM_PANEL
+config DRM_PANEL_JDI_LT070ME05000
tristate "JDI LT070ME05000 WUXGA DSI panel"depends on OFdepends on DRM_MIPI_DSIdepends on BACKLIGHT_CLASS_DEVICEhelpPlease use consistent indentation here. Also, this is badly sorted. The DRM_PANEL_SIMPLE is special in that it doesn't have a vendor prefix, all others should be sorted after panel-simple and in alphabetical order by vendor, then model.
Say Y here if you want to enable support for JDI WUXGA DSI videomode panel as found in Google Nexus 7 (2013) devices.The panel has a 1200(RGB)×1920 (WUXGA) resolution and uses24 bit RGB per pixel.It's kind of redundant to say 1200(RGB) when you say that it's 24 bit RGB afterwards. You also repeat WUXGA twice, so you can drop either of the occurrences as well.
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile index f277eed..5d74ac2 100644 --- a/drivers/gpu/drm/panel/Makefile +++ b/drivers/gpu/drm/panel/Makefile @@ -1,3 +1,4 @@ +obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o
Please sort this the same way as the Kconfig entries.
obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.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-jdi-lt070me05000.c b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file mode 100644 index 0000000..8657e5f --- /dev/null +++ b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c @@ -0,0 +1,539 @@ +/*
- Copyright (C) 2016 InforceComputing
- Author: Vinay Simha BN simhavcs@gmail.com
- Copyright (C) 2016 Linaro Ltd
- Author: Sumit Semwal sumit.semwal@linaro.org
- From internet archives, the panel for Nexus 7 2nd Gen, 2013 model is a
- JDI model LT070ME05000, and its data sheet is at:
- This program is free software; you can redistribute it and/or modify it
- under the terms of the GNU General Public License version 2 as published by
- the Free Software Foundation.
- 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.
- You should have received a copy of the GNU General Public License along with
- 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> +#include <drm/drm_mipi_dsi.h> +#include <drm/drm_panel.h>
+#include <video/mipi_display.h>
+#define PANEL_DEV_REGULATOR_MAX 3
_MAX suggests that the number of regulators is actually variable, but it's not. PANEL_NUM_REGULATORS, perhaps?
+#define PANEL_MAX_BRIGHTNESS 0xFF
There isn't really a need for this macro, you only use the constant in one location. Also, a brightness is usually represented in decimal.
+const char *regs[] = {
"vddp","dcdc_en","vcc"+};
This should be static. Also use a more sensible name, such as regulator_names, please.
it kept as regs, to keep constant names as used in the dsi_cfg file drivers/gpu/drm/msm/dsi/dsi_cfg.c
+static int jdi_panel_init(struct jdi_panel *jdi) +{
[...]
struct mipi_dsi_device *dsi = jdi->dsi;int ret;dsi->mode_flags |= MIPI_DSI_MODE_LPM;ret = mipi_dsi_dcs_soft_reset(dsi);if (ret < 0)return ret;usleep_range(10000, 20000);ret = mipi_dsi_dcs_set_pixel_format(dsi, 0x70);if (ret < 0)return ret;Please use the existing symbolic constants for this.
i am not clear on the symbolic constants for pixel_format ?
ret = mipi_dsi_dcs_set_column_address(dsi, 0x0000, 0x04AF);if (ret < 0)return ret;ret = mipi_dsi_dcs_set_page_address(dsi, 0x0000, 0x077F);if (ret < 0)return ret;Please parameterize these using the panel width and height.
ret = mipi_dsi_dcs_set_tear_on(dsi,MIPI_DSI_DCS_TEAR_MODE_VBLANK);if (ret < 0)return ret;usleep_range(5000, 10000);Use a blank line after conditional blocks, please.
ret = mipi_dsi_set_tear_scanline(dsi, 0x0300);if (ret < 0)return ret;Might be worth making this a decimal. But then the question begs: why is 768 a good value for the tear scanline? Also, should you not set the tear scanline before enabling tear mode?
will check on this.
ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,(u8[]){ 0xFF }, 1);if (ret < 0)return ret;Is this necessary? Shouldn't the backlight implementation set this upon registration already?
ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,(u8[]){ 0x24 }, 1);if (ret < 0)return ret;
brightness control setting, to enable pwm/backlight
ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_POWER_SAVE,(u8[]){ 0x00 }, 1);if (ret < 0)return ret;
this is to set cabc off/on
What do these mean?
ret = mipi_dsi_dcs_exit_sleep_mode(dsi);if (ret < 0)return ret;mdelay(120);Again, could use an extra blank line for readability. Also, don't ever use mdelay() for such large delays! Use msleep() instead.
ret = mipi_dsi_generic_write(dsi, (u8[]){0xB0, 0x00}, 2);if (ret < 0)return ret;mdelay(10);Same here. This also needs at least a comment, though ideally you'd use symbolic names for those magic numbers.
i do not have the datasheet to give more description. this is for interface setting, either command mode/video mode
ret = mipi_dsi_generic_write(dsi, (u8[]){0xB3, 0x26, 0x08, 0x00, 0x20, 0x00}, 6);if (ret < 0)return ret;mdelay(20);And here.
+static int jdi_panel_unprepare(struct drm_panel *panel) +{
struct jdi_panel *jdi = to_jdi_panel(panel);struct regulator_bulk_data *s = jdi->supplies;int num = PANEL_DEV_REGULATOR_MAX;struct device *dev = &jdi->dsi->dev;int ret;if (!jdi->prepared)return 0;DRM_DEBUG("unprepare\n");ret = jdi_panel_off(jdi);if (ret) {dev_err(panel->dev, "failed to set panel off: %d\n", ret);return ret;}ret = regulator_bulk_disable(num, s);if (ret < 0) {dev_err(dev, "regulator disable failed, %d\n", ret);return ret;}if (jdi->reset_gpio)gpiod_set_value(jdi->reset_gpio, 0);That's odd. Wouldn't you typically assert the reset in ->unprepare()? Perhaps you need to mark the GPIO as GPIO_ACTIVE_LOW?
if (jdi->enable_gpio)gpiod_set_value(jdi->enable_gpio, 0);jdi->prepared = false;return 0;+}
+static int jdi_panel_prepare(struct drm_panel *panel) +{
struct jdi_panel *jdi = to_jdi_panel(panel);struct regulator_bulk_data *s = jdi->supplies;There's no need for this temporary variable, jdi->supplies isn't terrible long.
int num = PANEL_DEV_REGULATOR_MAX;This should be unsigned int. Also, might be worth to simply drop this usage and do...
struct device *dev = &jdi->dsi->dev;int ret;if (jdi->prepared)return 0;DRM_DEBUG("prepare\n");ret = regulator_bulk_enable(num, s);ret = regulator_bulk_enable(ARRAY_SIZE(jdi->supplies), jdi->supplies);here.
if (ret < 0) {dev_err(dev, "regulator enable failed, %d\n", ret);Can you please be more consistent with how you report errors? Other drivers use "...: %d\n", ret and you do so yourself in other parts of this same driver, so make them all consistent, please.
+static int jdi_panel_enable(struct drm_panel *panel) +{
struct jdi_panel *jdi = to_jdi_panel(panel);if (jdi->enabled)return 0;DRM_DEBUG("enable\n");Do you really need these? You can use the kernel's built-in function tracing for the same purpose.
+static const struct drm_display_mode default_mode = {
.clock = 155493,.hdisplay = 1200,.hsync_start = 1200 + 48,.hsync_end = 1200 + 48 + 32,.htotal = 1200 + 48 + 32 + 60,.vdisplay = 1920,.vsync_start = 1920 + 3,.vsync_end = 1920 + 3 + 5,.vtotal = 1920 + 3 + 5 + 6,.vrefresh = 60,.flags = 0,.width_mm = 95,.height_mm = 151,It's slightly unusual to set this in the mode. Typically you'd set the connector->display_info.{width,height}_mm directly.
+static int dsi_dcs_bl_get_brightness(struct backlight_device *bl) +{
struct mipi_dsi_device *dsi = bl_get_data(bl);int ret;u16 brightness;u8 bl_value[2];dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);if (ret < 0)return ret;dsi->mode_flags |= MIPI_DSI_MODE_LPM;bl_value[0] = brightness & 0xff;bl_value[1] = brightness >> 8;return bl_value[0];+}
Why not simply return "brightness & 0xff" here?
+static int dsi_dcs_bl_update_status(struct backlight_device *bl) +{
struct mipi_dsi_device *dsi = bl_get_data(bl);int ret;u16 brightness = bl->props.brightness;There's no need for this temporary variable.
dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);if (ret < 0)return ret;dsi->mode_flags |= MIPI_DSI_MODE_LPM;return 0;+}
[...]
+struct backlight_device
*drm_panel_create_dsi_backlight(struct mipi_dsi_device *dsi)Your indentation is wrong here. The * should go on the same line as the type, and the function name should start in column 1.
+{
struct device *dev = &dsi->dev;struct backlight_properties props;memset(&props, 0, sizeof(props));props.type = BACKLIGHT_RAW;props.brightness = PANEL_MAX_BRIGHTNESS;props.max_brightness = PANEL_MAX_BRIGHTNESS;Just use 255 for the maximum and default brightness here.
+static int jdi_panel_add(struct jdi_panel *jdi) +{
struct device *dev = &jdi->dsi->dev;struct regulator_bulk_data *s = jdi->supplies;int num = PANEL_DEV_REGULATOR_MAX;Again, no need for these two temporary variables.
int i, ret;i should be unsigned int.
jdi->mode = &default_mode;for (i = 0; i < num; i++)s[i].supply = regs[i];ret = devm_regulator_bulk_get(dev, num, s);if (ret < 0) {dev_err(dev, "%s: failed to init regulator, ret=%d\n",__func__, ret);return ret;}jdi->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);if (IS_ERR(jdi->reset_gpio)) {dev_err(dev, "cannot get reset-gpios %ld\n",PTR_ERR(jdi->reset_gpio));This is a third variant of error reporting. Please stick to one.
for PTR_ERR(jdi->reset_gpio) returns unsigned long, so this error reporting cannot be changed to ret, others error reporting incorporated consistently.
jdi->reset_gpio = NULL;} else {gpiod_direction_output(jdi->reset_gpio, 0);}jdi->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);if (IS_ERR(jdi->enable_gpio)) {dev_err(dev, "cannot get enable-gpio %ld\n",PTR_ERR(jdi->enable_gpio));jdi->enable_gpio = NULL;} else {gpiod_direction_output(jdi->enable_gpio, 0);}jdi->backlight = drm_panel_create_dsi_backlight(jdi->dsi);You should check for errors here.
+MODULE_AUTHOR("Vinay Simha BN simhavcs@gmail.com"); +MODULE_DESCRIPTION("JDI WUXGA LT070ME05000 DSI video mode panel driver");
The commit message names this "JDI LT070ME05000 WUXGA", please use the same here.
Thierry
On Tue, Jun 14, 2016 at 04:27:53PM +0530, Vinay Simha wrote: [...]
On Mon, Jun 13, 2016 at 6:05 PM, Thierry Reding thierry.reding@gmail.com wrote:
On Mon, Jun 13, 2016 at 03:55:28PM +0530, Vinay Simha BN wrote:
[...]
+const char *regs[] = {
"vddp","dcdc_en","vcc"+};
This should be static. Also use a more sensible name, such as regulator_names, please.
it kept as regs, to keep constant names as used in the dsi_cfg file drivers/gpu/drm/msm/dsi/dsi_cfg.c
That's a completely different driver, no need to be consistent.
+static int jdi_panel_init(struct jdi_panel *jdi) +{
[...]
struct mipi_dsi_device *dsi = jdi->dsi;int ret;dsi->mode_flags |= MIPI_DSI_MODE_LPM;ret = mipi_dsi_dcs_soft_reset(dsi);if (ret < 0)return ret;usleep_range(10000, 20000);ret = mipi_dsi_dcs_set_pixel_format(dsi, 0x70);if (ret < 0)return ret;Please use the existing symbolic constants for this.
i am not clear on the symbolic constants for pixel_format ?
See include/video/mipi_display.h
ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,(u8[]){ 0x24 }, 1);if (ret < 0)return ret;brightness control setting, to enable pwm/backlight
ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_POWER_SAVE,(u8[]){ 0x00 }, 1);if (ret < 0)return ret;this is to set cabc off/on
Can you point me to the specification of these? I'd like to investigate whether or not we can turn these into more sensible commands. As-is, it is completely obfuscated and I'd like to avoid that where possible.
ret = mipi_dsi_generic_write(dsi, (u8[]){0xB0, 0x00}, 2);if (ret < 0)return ret;mdelay(10);Same here. This also needs at least a comment, though ideally you'd use symbolic names for those magic numbers.
i do not have the datasheet to give more description. this is for interface setting, either command mode/video mode
Okay, please add a comment on what this is supposed to do, then.
jdi->mode = &default_mode;for (i = 0; i < num; i++)s[i].supply = regs[i];ret = devm_regulator_bulk_get(dev, num, s);if (ret < 0) {dev_err(dev, "%s: failed to init regulator, ret=%d\n",__func__, ret);return ret;}jdi->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);if (IS_ERR(jdi->reset_gpio)) {dev_err(dev, "cannot get reset-gpios %ld\n",PTR_ERR(jdi->reset_gpio));This is a third variant of error reporting. Please stick to one.
for PTR_ERR(jdi->reset_gpio) returns unsigned long, so this error reporting cannot be changed to ret, others error reporting incorporated consistently.
PTR_ERR() returns signed long, not unsigned.
You can still use the same format for the message and substitute the %ld printk specifier to match the type.
Thierry
On Tue, Jun 14, 2016 at 4:55 PM, Thierry Reding thierry.reding@gmail.com wrote:
On Tue, Jun 14, 2016 at 04:27:53PM +0530, Vinay Simha wrote: [...]
On Mon, Jun 13, 2016 at 6:05 PM, Thierry Reding thierry.reding@gmail.com wrote:
On Mon, Jun 13, 2016 at 03:55:28PM +0530, Vinay Simha BN wrote:
[...]
+const char *regs[] = {
"vddp","dcdc_en","vcc"+};
This should be static. Also use a more sensible name, such as regulator_names, please.
it kept as regs, to keep constant names as used in the dsi_cfg file drivers/gpu/drm/msm/dsi/dsi_cfg.c
That's a completely different driver, no need to be consistent.
+static int jdi_panel_init(struct jdi_panel *jdi) +{
[...]
struct mipi_dsi_device *dsi = jdi->dsi;int ret;dsi->mode_flags |= MIPI_DSI_MODE_LPM;ret = mipi_dsi_dcs_soft_reset(dsi);if (ret < 0)return ret;usleep_range(10000, 20000);ret = mipi_dsi_dcs_set_pixel_format(dsi, 0x70);if (ret < 0)return ret;Please use the existing symbolic constants for this.
i am not clear on the symbolic constants for pixel_format ?
See include/video/mipi_display.h
ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,(u8[]){ 0x24 }, 1);if (ret < 0)return ret;brightness control setting, to enable pwm/backlight
ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_POWER_SAVE,(u8[]){ 0x00 }, 1);if (ret < 0)return ret;this is to set cabc off/on
Can you point me to the specification of these? I'd like to investigate whether or not we can turn these into more sensible commands. As-is, it is completely obfuscated and I'd like to avoid that where possible.
https://cdn-shop.adafruit.com/datasheets/HX8357-D_DS_April2012.pdf
MIPI_DCS_WRITE_CONTROL_DISPLAY This is one of the panel i got in google search, refer 6.2.44 Write control display (53h) (page 171)
MIPI_DCS_WRITE_POWER_SAVE 6.2.46 Write content adaptive brightness control (55h) (page 173)
fyi, nx7 2013 panel/schematic datasheet is not available in opensource,
ret = mipi_dsi_generic_write(dsi, (u8[]){0xB0, 0x00}, 2);if (ret < 0)return ret;mdelay(10);Same here. This also needs at least a comment, though ideally you'd use symbolic names for those magic numbers.
i do not have the datasheet to give more description. this is for interface setting, either command mode/video mode
Okay, please add a comment on what this is supposed to do, then.
jdi->mode = &default_mode;for (i = 0; i < num; i++)s[i].supply = regs[i];ret = devm_regulator_bulk_get(dev, num, s);if (ret < 0) {dev_err(dev, "%s: failed to init regulator, ret=%d\n",__func__, ret);return ret;}jdi->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);if (IS_ERR(jdi->reset_gpio)) {dev_err(dev, "cannot get reset-gpios %ld\n",PTR_ERR(jdi->reset_gpio));This is a third variant of error reporting. Please stick to one.
for PTR_ERR(jdi->reset_gpio) returns unsigned long, so this error reporting cannot be changed to ret, others error reporting incorporated consistently.
PTR_ERR() returns signed long, not unsigned.
You can still use the same format for the message and substitute the %ld printk specifier to match the type.
Thierry
dri-devel@lists.freedesktop.org
-
Thierry Reding -
Vinay Simha -
Vinay Simha BN