Skip to contentSkip to conversation

drm/dsi: Implement dcs set/get display brightness

PATCH Superseded Vinay Simha BN 2 participants 7 messages Jun 13, 2016 – Jun 15, 2016

Subsystems (3)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

PATCH v2 1/2 drm/dsi: Implement dcs set/get display brightness

Provide a small convenience wrapper that set/get the display brightness value

Cc John Stultz <[email protected]>
Cc Sumit Semwal <[email protected]>
Cc Archit Taneja <[email protected]>
Cc Rob Clark <[email protected]>
Cc Jani Nikula <[email protected]>

--
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(+)
drivers/gpu/drm/drm_mipi_dsi.c +49 -0
index 7938ce7..5661a00 100644
@@ -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);
include/drm/drm_mipi_dsi.h +4 -0
index ec55285..4494641 100644
@@ -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
Signature · 1 line
2.1.2
#1 lore raw

PATCH v4 2/2 drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel

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 <[email protected]>
[sumit.semwal: Ported to the drm/panel framework]

[jstultz: Cherry-picked to mainline, folded down other fixes
from Vinay and Archit]

[vinay simha bn: removed interface setting cmd mode, video mode panel setting selection]

--
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
drivers/gpu/drm/panel/Kconfig +11 -0
index 1500ab9..83e89e7 100644
@@ -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
drivers/gpu/drm/panel/Makefile +1 -0
index f277eed..5d74ac2 100644
@@ -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
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +539 -0

539 lines not on this page · raw message

Signature · 1 line
2.1.2
#2 lore raw

Re: [PATCH v4 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel

Quoted 1 line · Vinay Simha BN

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.

Quoted 45 lines

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 <[email protected]>
[sumit.semwal: Ported to the drm/panel framework]

[jstultz: Cherry-picked to mainline, folded down other fixes
from Vinay and Archit]

[vinay simha bn: removed interface setting cmd mode, video mode panel setting selection]

--
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
drivers/gpu/drm/panel/Kconfig +0 -0
index 1500ab9..83e89e7 100644

drivers/gpu/drm/panel/Kconfig config DRM_PANEL · line 15

Quoted 9 lines
drivers/gpu/drm/panel/Kconfig +6 -0
@@ -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

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.

drivers/gpu/drm/panel/Kconfig config DRM_PANEL · line 19

Quoted 4 lines
drivers/gpu/drm/panel/Kconfig +4 -0
+	   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.

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.

drivers/gpu/drm/panel/Makefile line 1

Quoted 6 lines
drivers/gpu/drm/panel/Makefile +1 -0
index f277eed..5d74ac2 100644
@@ -1,3 +1,4 @@
+obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o

Please sort this the same way as the Kconfig entries.

Quoted 36 lines
drivers/gpu/drm/panel/Makefile +0 -0
 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
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +27 -0
new file mode 100644
index 0000000..8657e5f
@@ -0,0 +1,539 @@
+/*
+ * Copyright (C) 2016 InforceComputing
+ * Author: Vinay Simha BN <[email protected]>
+ *
+ * Copyright (C) 2016 Linaro Ltd
+ * Author: Sumit Semwal <[email protected]>
+ *
+ * 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>

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 37

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+#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?

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 38

Quoted 1 line
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +1 -0
+#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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 44

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+const char *regs[] = {
+	"vddp",
+	"dcdc_en",
+	"vcc"
+};

This should be static. Also use a more sensible name, such as regulator_names, please.

Quoted 2 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +2 -0
+static int jdi_panel_init(struct jdi_panel *jdi)
+{

[...]

Quoted 4 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +4 -0
+	struct mipi_dsi_device *dsi = jdi->dsi;
+	int ret;
+
+	dsi->mode_flags |= MIPI_DSI_MODE_LPM;

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 82

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+
+	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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 90

Quoted 8 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +8 -0
+
+	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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 96

Quoted 6 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +6 -0
+
+	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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 100

Quoted 4 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +4 -0
+
+	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?

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 105

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+
+	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?

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 115

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+
+	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?

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 120

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+
+	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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 125

Quoted 4 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +4 -0
+	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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 131

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+	ret = mipi_dsi_generic_write(dsi, (u8[])
+				     {0xB3, 0x26, 0x08, 0x00, 0x20, 0x00}, 6);
+	if (ret < 0)
+		return ret;
+	mdelay(20);

And here.

Quoted 17 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +17 -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;

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 223

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+	}
+
+	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?

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 236

Quoted 12 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +12 -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;

There's no need for this temporary variable, jdi->supplies isn't terrible long.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c

Quoted 1 line
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +1 -0
+	int num = PANEL_DEV_REGULATOR_MAX;

This should be unsigned int. Also, might be worth to simply drop this usage and do...

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 246

Quoted 9 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +9 -0
+	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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c

Quoted 2 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +2 -0
+	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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 296

Quoted 8 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +8 -0
+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.

Quoted 4 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +4 -0
+static const struct drm_display_mode default_mode = {
+		.clock = 155493,
+		.hdisplay = 1200,
+		.hsync_start = 1200 + 48,

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 321

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+		.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.

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+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);

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 364

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+	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?

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 370

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -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;

There's no need for this temporary variable.

Quoted 11 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +11 -0
+
+	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;
+}

[...]

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 389

Quoted 2 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +2 -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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 397

Quoted 8 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +8 -0
+{
+	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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 421

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 422

Quoted 1 line
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +1 -0
+	int i, ret;

i should be unsigned int.

Quoted 7 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +7 -0
+
+	jdi->mode = &default_mode;
+
+	for (i = 0; i < num; i++)
+		s[i].supply = regs[i];
+
+	ret = devm_regulator_bulk_get(dev, num, s);

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 439

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+	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.

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+		jdi->reset_gpio = NULL;
+	} else {
+		gpiod_direction_output(jdi->reset_gpio, 0);
+	}
+

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 454

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 538

Quoted 2 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +2 -0
+MODULE_AUTHOR("Vinay Simha BN <[email protected]>");
+MODULE_DESCRIPTION("JDI WUXGA LT070ME05000 DSI video mode panel driver");

The commit message names this "JDI LT070ME05000 WUXGA", please use the same here.

Thierry

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

#3 lore raw

Re: [PATCH v4 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel

thierry,

reviews are incorportated in v4, need some clarification on few reviews.

Quoted 139 lines · Thierry Reding
Quoted 1 line · Vinay Simha BN

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.

Quoted 45 lines

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 <[email protected]>
[sumit.semwal: Ported to the drm/panel framework]

[jstultz: Cherry-picked to mainline, folded down other fixes
from Vinay and Archit]

[vinay simha bn: removed interface setting cmd mode, video mode panel setting selection]

--
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
drivers/gpu/drm/panel/Kconfig +0 -0
index 1500ab9..83e89e7 100644

drivers/gpu/drm/panel/Kconfig config DRM_PANEL · line 15

Quoted 9 lines
drivers/gpu/drm/panel/Kconfig +6 -0
@@ -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

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.

drivers/gpu/drm/panel/Kconfig config DRM_PANEL · line 19

Quoted 4 lines
drivers/gpu/drm/panel/Kconfig +4 -0
+        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.

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.

drivers/gpu/drm/panel/Makefile line 1

Quoted 6 lines
drivers/gpu/drm/panel/Makefile +1 -0
index f277eed..5d74ac2 100644
@@ -1,3 +1,4 @@
+obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o

Please sort this the same way as the Kconfig entries.

Quoted 36 lines
drivers/gpu/drm/panel/Makefile +0 -0
 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
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +27 -0
new file mode 100644
index 0000000..8657e5f
@@ -0,0 +1,539 @@
+/*
+ * Copyright (C) 2016 InforceComputing
+ * Author: Vinay Simha BN <[email protected]>
+ *
+ * Copyright (C) 2016 Linaro Ltd
+ * Author: Sumit Semwal <[email protected]>
+ *
+ * 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>

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 37

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+#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?

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 38

Quoted 1 line
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +1 -0
+#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.

Quoted 8 lines

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 44

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+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

Quoted 4 lines
Quoted 3 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +2 -0
+static int jdi_panel_init(struct jdi_panel *jdi)
+{

[...]

Quoted 16 lines
Quoted 4 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +4 -0
+     struct mipi_dsi_device *dsi = jdi->dsi;
+     int ret;
+
+     dsi->mode_flags |= MIPI_DSI_MODE_LPM;

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 82

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+
+     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 ?

Quoted 21 lines

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 90

Quoted 9 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +8 -0
+
+     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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 96

Quoted 6 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +6 -0
+
+     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.

Quoted 9 lines

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 100

Quoted 4 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +4 -0
+
+     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.

Quoted 6 lines

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 105

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+
+     ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
+                              (u8[]){ 0xFF }, 1);
+     if (ret < 0)
+             return ret;
Quoted 8 lines

Is this necessary? Shouldn't the backlight implementation set this upon registration already?

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+
+     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

Quoted 6 lines

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 115

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+
+     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

Quoted 11 lines

What do these mean?

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 120

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+
+     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.

Quoted 7 lines

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 125

Quoted 4 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +4 -0
+     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

Quoted 195 lines

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 131

Quoted 6 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+     ret = mipi_dsi_generic_write(dsi, (u8[])
+                                  {0xB3, 0x26, 0x08, 0x00, 0x20, 0x00}, 6);
+     if (ret < 0)
+             return ret;
+     mdelay(20);

And here.

Quoted 17 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +17 -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;

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 223

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+     }
+
+     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?

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 236

Quoted 12 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +12 -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;

There's no need for this temporary variable, jdi->supplies isn't terrible long.

Quoted 1 line

+ int num = PANEL_DEV_REGULATOR_MAX;

This should be unsigned int. Also, might be worth to simply drop this usage and do...

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 246

Quoted 9 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +9 -0
+     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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c

Quoted 2 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +2 -0
+     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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 296

Quoted 8 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +8 -0
+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.

Quoted 4 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +4 -0
+static const struct drm_display_mode default_mode = {
+             .clock = 155493,
+             .hdisplay = 1200,
+             .hsync_start = 1200 + 48,

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 321

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+             .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.

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+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);

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 364

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+     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?

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 370

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -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;

There's no need for this temporary variable.

Quoted 11 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +11 -0
+
+     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;
+}

[...]

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 389

Quoted 2 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +2 -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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 397

Quoted 8 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +8 -0
+{
+     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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 421

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+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.

Quoted 1 line

+ int i, ret;

i should be unsigned int.

Quoted 11 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +11 -0
+
+     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;
Quoted 8 lines

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 439

Quoted 6 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +6 -0
+     }
+
+     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.

Quoted 26 lines
Quoted 6 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+             jdi->reset_gpio = NULL;
+     } else {
+             gpiod_direction_output(jdi->reset_gpio, 0);
+     }
+

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 454

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 538

Quoted 2 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +2 -0
+MODULE_AUTHOR("Vinay Simha BN <[email protected]>");
+MODULE_DESCRIPTION("JDI WUXGA LT070ME05000 DSI video mode panel driver");

The commit message names this "JDI LT070ME05000 WUXGA", please use the same here.

Thierry

Signature · 3 lines
Regards,

Vinay Simha.B.N.
#4 lore raw

Re: [PATCH v4 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel

On Tue, Jun 14, 2016 at 04:27:53PM +0530, Vinay Simha wrote:
[...]

Quoted 3 lines
Quoted 1 line · Thierry Reding

On Mon, Jun 13, 2016 at 03:55:28PM +0530, Vinay Simha BN wrote:

[...]

Quoted 11 lines
Quoted 8 lines

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 44

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+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.

Quoted 19 lines
Quoted 19 lines
Quoted 2 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +2 -0
+static int jdi_panel_init(struct jdi_panel *jdi)
+{

[...]

Quoted 4 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +4 -0
+     struct mipi_dsi_device *dsi = jdi->dsi;
+     int ret;
+
+     dsi->mode_flags |= MIPI_DSI_MODE_LPM;

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 82

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+
+     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.

Quoted 1 line

i am not clear on the symbolic constants for pixel_format ?

See include/video/mipi_display.h

Quoted 5 lines
Quoted 5 lines

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 110

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+
+     ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,
+                              (u8[]){ 0x24 }, 1);
+     if (ret < 0)
+             return ret;
Quoted 8 lines

brightness control setting, to enable pwm/backlight

Quoted 6 lines

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 115

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+
+     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.

Quoted 9 lines
Quoted 7 lines

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 125

Quoted 4 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +4 -0
+     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.

Quoted 19 lines
Quoted 11 lines
Quoted 11 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +11 -0
+
+     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;
Quoted 8 lines

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 439

Quoted 6 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +6 -0
+     }
+
+     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.

Quoted 3 lines

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 mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

#5 lore raw

Re: [PATCH v4 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel

Quoted 55 lines · Thierry Reding

On Tue, Jun 14, 2016 at 04:27:53PM +0530, Vinay Simha wrote:
[...]

Quoted 3 lines
Quoted 1 line · Thierry Reding

On Mon, Jun 13, 2016 at 03:55:28PM +0530, Vinay Simha BN wrote:

[...]

Quoted 11 lines
Quoted 8 lines

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 44

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+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.

Quoted 19 lines
Quoted 19 lines
Quoted 2 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +2 -0
+static int jdi_panel_init(struct jdi_panel *jdi)
+{

[...]

Quoted 4 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +4 -0
+     struct mipi_dsi_device *dsi = jdi->dsi;
+     int ret;
+
+     dsi->mode_flags |= MIPI_DSI_MODE_LPM;

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 82

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+
+     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.

Quoted 1 line

i am not clear on the symbolic constants for pixel_format ?

See include/video/mipi_display.h

Quoted 12 lines
Quoted 5 lines

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 110

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+
+     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

Quoted 6 lines
Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+
+     ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_POWER_SAVE,
+                              (u8[]){ 0x00 }, 1);
+     if (ret < 0)
+             return ret;
Quoted 5 lines
Quoted 1 line

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,

Quoted 42 lines
Quoted 10 lines
Quoted 8 lines

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 125

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +4 -0
+     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.

Quoted 19 lines
Quoted 11 lines
Quoted 11 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +11 -0
+
+     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;
Quoted 8 lines

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 439

Quoted 6 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +6 -0
+     }
+
+     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.

Quoted 3 lines

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

Signature · 3 lines
Regards,

Vinay Simha.B.N.
#6 lore raw

Re: [PATCH v4 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel

In reply to Thierry Reding
Quoted 266 lines · Thierry Reding
Quoted 1 line · Vinay Simha BN

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.

Quoted 45 lines

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 <[email protected]>
[sumit.semwal: Ported to the drm/panel framework]

[jstultz: Cherry-picked to mainline, folded down other fixes
from Vinay and Archit]

[vinay simha bn: removed interface setting cmd mode, video mode panel setting selection]

--
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
drivers/gpu/drm/panel/Kconfig +0 -0
index 1500ab9..83e89e7 100644

drivers/gpu/drm/panel/Kconfig config DRM_PANEL · line 15

Quoted 9 lines
drivers/gpu/drm/panel/Kconfig +6 -0
@@ -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

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.

drivers/gpu/drm/panel/Kconfig config DRM_PANEL · line 19

Quoted 4 lines
drivers/gpu/drm/panel/Kconfig +4 -0
+        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.

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.

drivers/gpu/drm/panel/Makefile line 1

Quoted 6 lines
drivers/gpu/drm/panel/Makefile +1 -0
index f277eed..5d74ac2 100644
@@ -1,3 +1,4 @@
+obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o

Please sort this the same way as the Kconfig entries.

Quoted 36 lines
drivers/gpu/drm/panel/Makefile +0 -0
 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
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +27 -0
new file mode 100644
index 0000000..8657e5f
@@ -0,0 +1,539 @@
+/*
+ * Copyright (C) 2016 InforceComputing
+ * Author: Vinay Simha BN <[email protected]>
+ *
+ * Copyright (C) 2016 Linaro Ltd
+ * Author: Sumit Semwal <[email protected]>
+ *
+ * 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>

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 37

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+#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?

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 38

Quoted 1 line
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +1 -0
+#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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 44

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+const char *regs[] = {
+     "vddp",
+     "dcdc_en",
+     "vcc"
+};

This should be static. Also use a more sensible name, such as regulator_names, please.

Quoted 2 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +2 -0
+static int jdi_panel_init(struct jdi_panel *jdi)
+{

[...]

Quoted 4 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +4 -0
+     struct mipi_dsi_device *dsi = jdi->dsi;
+     int ret;
+
+     dsi->mode_flags |= MIPI_DSI_MODE_LPM;

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 82

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+
+     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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 90

Quoted 8 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +8 -0
+
+     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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 96

Quoted 6 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +6 -0
+
+     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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 100

Quoted 4 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +4 -0
+
+     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?

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 105

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+
+     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?

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 115

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+
+     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?

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 120

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+
+     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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 125

Quoted 4 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +4 -0
+     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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 131

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+     ret = mipi_dsi_generic_write(dsi, (u8[])
+                                  {0xB3, 0x26, 0x08, 0x00, 0x20, 0x00}, 6);
+     if (ret < 0)
+             return ret;
+     mdelay(20);

And here.

Quoted 22 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +22 -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);
Quoted 8 lines

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 223

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+             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?

in prepare() reset_gpio and enable_gpio are active high
unprepare() reset_gpio and enable_gpio are active low
do i am missing any flag settings?

Quoted 190 lines

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 236

Quoted 13 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +12 -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;

There's no need for this temporary variable, jdi->supplies isn't terrible long.

Quoted 1 line

+ int num = PANEL_DEV_REGULATOR_MAX;

This should be unsigned int. Also, might be worth to simply drop this usage and do...

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 246

Quoted 9 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +9 -0
+     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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c

Quoted 2 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +2 -0
+     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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 296

Quoted 8 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +8 -0
+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.

Quoted 4 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +4 -0
+static const struct drm_display_mode default_mode = {
+             .clock = 155493,
+             .hdisplay = 1200,
+             .hsync_start = 1200 + 48,

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 321

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+             .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.

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+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);

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 364

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+     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?

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 370

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -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;

There's no need for this temporary variable.

Quoted 11 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +11 -0
+
+     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;
+}

[...]

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 389

Quoted 2 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +2 -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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 397

Quoted 8 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +8 -0
+{
+     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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 421

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+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.

Quoted 1 line

+ int i, ret;

i should be unsigned int.

Quoted 7 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +7 -0
+
+     jdi->mode = &default_mode;
+
+     for (i = 0; i < num; i++)
+             s[i].supply = regs[i];
+
+     ret = devm_regulator_bulk_get(dev, num, s);

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 439

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -0
+     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.

Quoted 5 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +5 -0
+             jdi->reset_gpio = NULL;
+     } else {
+             gpiod_direction_output(jdi->reset_gpio, 0);
+     }
+

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 454

Quoted 10 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +10 -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.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c line 538

Quoted 2 lines
drivers/gpu/drm/panel/panel-jdi-lt070me05000.c new file +2 -0
+MODULE_AUTHOR("Vinay Simha BN <[email protected]>");
+MODULE_DESCRIPTION("JDI WUXGA LT070ME05000 DSI video mode panel driver");

The commit message names this "JDI LT070ME05000 WUXGA", please use the same here.

Thierry

Signature · 3 lines
Regards,

Vinay Simha.B.N.