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

[Emil Velikov]

On 18 June 2016 at 07:59, Vinay Simha  wrote:
> On Sat, Jun 18, 2016 at 5:58 AM, Emil Velikov  
> wrote:
>> Hi Vinay,
>>
>> On 17 June 2016 at 19:23, Vinay Simha BN  wrote:
>>
>>> v6:
>>>  * emil review comments incorporated
>>>PANEL_NUM_REGULATORS dropped, return ret added at necessary
>>>places, if checks dropped for backlight and gpios
>>
>> Looks like some of my suggestions went below the radar. Did you miss
>> them or I've I lost the plot somewhere ? In case of the latter don't
>> be shy to point out :-)
>>
>>
>>> +struct jdi_panel {
>>> +   struct drm_panel base;
>>> +   struct mipi_dsi_device *dsi;
>>> +
>>> +   struct regulator_bulk_data supplies[3];
>>> +
>> struct regulator_bulk_data supplies[ARRAY_SIZE(regulator_names)];
>>
>>
>>> +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;
>>> +
>> IMHO neither this function nor its caller jdi_panel_unprepare() should
>> stop in the middle/return prematurely.
>>
>> Or in other words - one should change the function return type to void
>> and drop the returns.
>>
>>
>>> +static int jdi_panel_unprepare(struct drm_panel *panel)
>>> +{
>>> +   struct jdi_panel *jdi = to_jdi_panel(panel);
>>> +   struct device *dev = >dsi->dev;
>>> +   int ret;
>>> +
>>> +   if (!jdi->prepared)
>>> +   return 0;
>>> +
>>> +   ret = jdi_panel_off(jdi);
>>> +   if (ret) {
>>> +   dev_err(panel->dev, "failed to set panel off: %d\n", ret);
>>> +   return ret;
>> As suggested above, drop this return.
>>
> i can make the function void for jdi_panel_off  and drop the return,
> but i cannot make void for jdi_panel_unprepare,
> since drm_panel_prepare requires 0 or negative value.
>
Seems like I wasn't clear enough - all you want here is to drop the
spurious return.

Regards,
Emil

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

[Vinay Simha]

On Sat, Jun 18, 2016 at 5:58 AM, Emil Velikov  
wrote:
> Hi Vinay,
>
> On 17 June 2016 at 19:23, Vinay Simha BN  wrote:
>
>> v6:
>>  * emil review comments incorporated
>>PANEL_NUM_REGULATORS dropped, return ret added at necessary
>>places, if checks dropped for backlight and gpios
>
> Looks like some of my suggestions went below the radar. Did you miss
> them or I've I lost the plot somewhere ? In case of the latter don't
> be shy to point out :-)
>
>
>> +struct jdi_panel {
>> +   struct drm_panel base;
>> +   struct mipi_dsi_device *dsi;
>> +
>> +   struct regulator_bulk_data supplies[3];
>> +
> struct regulator_bulk_data supplies[ARRAY_SIZE(regulator_names)];
>
>
>> +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;
>> +
> IMHO neither this function nor its caller jdi_panel_unprepare() should
> stop in the middle/return prematurely.
>
> Or in other words - one should change the function return type to void
> and drop the returns.
>
>
>> +static int jdi_panel_unprepare(struct drm_panel *panel)
>> +{
>> +   struct jdi_panel *jdi = to_jdi_panel(panel);
>> +   struct device *dev = >dsi->dev;
>> +   int ret;
>> +
>> +   if (!jdi->prepared)
>> +   return 0;
>> +
>> +   ret = jdi_panel_off(jdi);
>> +   if (ret) {
>> +   dev_err(panel->dev, "failed to set panel off: %d\n", ret);
>> +   return ret;
> As suggested above, drop this return.
>
i can make the function void for jdi_panel_off  and drop the return,
but i cannot make void for jdi_panel_unprepare,
since drm_panel_prepare requires 0 or negative value.


* call to drm_panel_prepare().
 *
 * Return: 0 on success or a negative error code on failure.
 */
static inline int drm_panel_unprepare(struct drm_panel *panel)
{
if (panel && panel->funcs && panel->funcs->unprepare)
return panel->funcs->unprepare(panel);

return panel ? -ENOSYS : -EINVAL;
}

>
>
>> +static int jdi_panel_prepare(struct drm_panel *panel)
>> +{
>> +   struct jdi_panel *jdi = to_jdi_panel(panel);
>> +   struct device *dev = >dsi->dev;
>> +   int ret;
>> +
>> +   if (jdi->prepared)
>> +   return 0;
>> +
>> +   ret = regulator_bulk_enable(ARRAY_SIZE(jdi->supplies), 
>> jdi->supplies);
>> +   if (ret < 0) {
>> +   dev_err(dev, "regulator enable failed, %d\n", ret);
>> +   return ret;
>> +   }
>> +
>> +   msleep(20);
>> +
>> +   if (jdi->reset_gpio) {
> You can drop the check.
>
>> +   gpiod_set_value(jdi->reset_gpio, 1);
>> +   usleep_range(10, 20);
>> +   }
>> +
>> +   if (jdi->enable_gpio) {
> Ditto.
>
>
>> +
>> +poweroff:
>> +   gpiod_set_value(jdi->enable_gpio, 0);
>> +   gpiod_set_value(jdi->reset_gpio, 0);
>> +
> Just noticed that you're missing regulator_bulk_disable() here.
>
>
> Regards,
> Emil



-- 
Regards,

Vinay Simha.B.N.

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

[Emil Velikov]

Hi Vinay,

On 17 June 2016 at 19:23, Vinay Simha BN  wrote:

> v6:
>  * emil review comments incorporated
>PANEL_NUM_REGULATORS dropped, return ret added at necessary
>places, if checks dropped for backlight and gpios

Looks like some of my suggestions went below the radar. Did you miss
them or I've I lost the plot somewhere ? In case of the latter don't
be shy to point out :-)


> +struct jdi_panel {
> +   struct drm_panel base;
> +   struct mipi_dsi_device *dsi;
> +
> +   struct regulator_bulk_data supplies[3];
> +
struct regulator_bulk_data supplies[ARRAY_SIZE(regulator_names)];


> +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;
> +
IMHO neither this function nor its caller jdi_panel_unprepare() should
stop in the middle/return prematurely.

Or in other words - one should change the function return type to void
and drop the returns.


> +static int jdi_panel_unprepare(struct drm_panel *panel)
> +{
> +   struct jdi_panel *jdi = to_jdi_panel(panel);
> +   struct device *dev = >dsi->dev;
> +   int ret;
> +
> +   if (!jdi->prepared)
> +   return 0;
> +
> +   ret = jdi_panel_off(jdi);
> +   if (ret) {
> +   dev_err(panel->dev, "failed to set panel off: %d\n", ret);
> +   return ret;
As suggested above, drop this return.



> +static int jdi_panel_prepare(struct drm_panel *panel)
> +{
> +   struct jdi_panel *jdi = to_jdi_panel(panel);
> +   struct device *dev = >dsi->dev;
> +   int ret;
> +
> +   if (jdi->prepared)
> +   return 0;
> +
> +   ret = regulator_bulk_enable(ARRAY_SIZE(jdi->supplies), jdi->supplies);
> +   if (ret < 0) {
> +   dev_err(dev, "regulator enable failed, %d\n", ret);
> +   return ret;
> +   }
> +
> +   msleep(20);
> +
> +   if (jdi->reset_gpio) {
You can drop the check.

> +   gpiod_set_value(jdi->reset_gpio, 1);
> +   usleep_range(10, 20);
> +   }
> +
> +   if (jdi->enable_gpio) {
Ditto.


> +
> +poweroff:
> +   gpiod_set_value(jdi->enable_gpio, 0);
> +   gpiod_set_value(jdi->reset_gpio, 0);
> +
Just noticed that you're missing regulator_bulk_disable() here.


Regards,
Emil

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

[Vinay Simha BN]

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 
Cc: Rob Clark 
Cc: Sumit Semwal 
Cc: John Stultz 
Cc: Emil Velikov 
Cc: Thierry Reding 
Cc: David Airlie 
Signed-off-by: Sumit Semwal 
Signed-off-by: John Stultz 
Signed-off-by: Vinay Simha BN 

---
v1:
 * sumit ported to drm/panel framework, john cherry-picked to mainline,
   folded down other fixes from Vinay and Archit, vinay removed interface
   setting cmd mode, video mode panel selected
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

v5:
 * theirry review comments incorporated
   panel model naming consistent, alphabetical soring in Kconfig
   Makefile, MAX_BRIGHTNESS dropped, regulator_names, parameterize
   panel width and height, descprition for control display, cabc
   and interface setting, temporary variable removed, consistent
   error reporting and commit message
 * removed tear on/off, scanline, since these are required only
   for command mode panels

v6:
 * emil review comments incorporated
   PANEL_NUM_REGULATORS dropped, return ret added at necessary
   places, if checks dropped for backlight and gpios
---
 drivers/gpu/drm/panel/Kconfig  |  11 +
 drivers/gpu/drm/panel/Makefile |   1 +
 drivers/gpu/drm/panel/panel-jdi-lt070me05000.c | 494 +
 3 files changed, 506 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..62aba97 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -18,6 +18,17 @@ config DRM_PANEL_SIMPLE
  that it can be automatically turned off when the panel goes into a
  low power state.

+config DRM_PANEL_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 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 per pixel.
+
 config DRM_PANEL_SAMSUNG_LD9040
tristate "Samsung LD9040 RGB/SPI panel"
depends on OF && SPI
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index f277eed..a5c7ec0 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
+obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o
 obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
 obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += 
panel-panasonic-vvx10f034n00.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.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 000..3213025
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
@@ -0,0 +1,494 @@
+/*
+ * Copyright (C) 2016 InforceComputing
+ * Author: Vinay Simha BN 
+ *
+ * Copyright (C) 2016 Linaro Ltd
+ * Author: Sumit Semwal 
+ *
+ * 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 .
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+static const char * const regulator_names[] = {
+   "vddp",
+   "dcdc_en",
+   "vcc"
+};
+
+struct jdi_panel {
+   struct drm_panel base;
+   struct mipi_dsi_device *dsi;
+
+   struct