From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: <20160413134947.GA1492@ulmo.ba.sec> References: <1460528887-22915-1-git-send-email-simhavcs@gmail.com> <20160413134947.GA1492@ulmo.ba.sec> Date: Wed, 13 Apr 2016 20:52:38 +0530 Message-ID: Subject: Re: [PATCH] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel From: Vinay Simha Content-Type: multipart/alternative; boundary=001a113ee4bc23692105305f5921 To: Thierry Reding Cc: David Airlie , John Stultz , Rob Herring , "open list:DRM PANEL DRIVERS" , Ian Campbell , Jonathan Cameron , Pawel Moll , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , Kumar Gala , Shawn Guo , Mark Rutland , Sumit Semwal , Ralf Baechle , Archit Taneja , open list List-ID: --001a113ee4bc23692105305f5921 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable In Previous email , forgot to add cc: archit. regards, vinay simha bn On 13-Apr-2016 19:19, "Thierry Reding" wrote: > On Wed, Apr 13, 2016 at 11:58:04AM +0530, Vinay Simha BN wrote: > > 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 > > > > Other fixes folded in were provided > > by Archit Taneja > > > > Signed-off-by: Vinay Simha BN > > [sumit.semwal: Ported to the drm/panel framework] > > Signed-off-by: Sumit Semwal > > [jstultz: Cherry-picked to mainline, folded down other fixes > > from Vinay and Archit] > > Signed-off-by: John Stultz > > --- > > .../bindings/display/panel/jdi,lt070me05000.txt | 27 + > > .../devicetree/bindings/vendor-prefixes.txt | 1 + > > drivers/gpu/drm/panel/Kconfig | 11 + > > drivers/gpu/drm/panel/Makefile | 1 + > > drivers/gpu/drm/panel/panel-jdi-lt070me05000.c | 685 > +++++++++++++++++++++ > > 5 files changed, 725 insertions(+) > > create mode 100644 > Documentation/devicetree/bindings/display/panel/jdi,lt070me05000.txt > > create mode 100644 drivers/gpu/drm/panel/panel-jdi-lt070me05000.c > > What's the difference between this and the patch you sent earlier? I'm > going to assume that the newer one is the correct patch, so I'll ignore > the previous patch. > > > diff --git > a/Documentation/devicetree/bindings/display/panel/jdi,lt070me05000.txt > b/Documentation/devicetree/bindings/display/panel/jdi,lt070me05000.txt > > new file mode 100644 > > index 0000000..35c5ac7 > > --- /dev/null > > +++ > b/Documentation/devicetree/bindings/display/panel/jdi,lt070me05000.txt > > The binding documentation should be a separate patch. > > > @@ -0,0 +1,27 @@ > > +JDI model LT070ME05000 1920x1200 7" DSI Panel > > + > > +Basic data sheet is at: > > + > http://panelone.net/en/7-0-inch/JDI_LT070ME05000_7.0_inch-datasheet > > + > > +This panel has video mode implemented currently in the driver. > > That's information irrelevant to the DT binding, since you're presumably > talking about the Linux drm/panel driver, whereas the DT binding is > supposed to specify the description of the panel hardware in OS-agnostic > terms. > > > +Required properties: > > +- compatible: should be "jdi,lt070me05000" > > + > > +Optional properties: > > +- power-supply: phandle of the regulator that provides the supply > voltage > > +- reset-gpio: phandle of gpio for reset line > > +- backlight: phandle of the backlight device attached to the panel > > + > > +Example: > > + > > + dsi@54300000 { > > + panel: panel@0 { > > + compatible =3D "jdi,lt070me05000"; > > + reg =3D <0>; > > + > > + power-supply =3D <...>; > > + reset-gpio =3D <...>; > > + backlight =3D <...>; > > + }; > > + }; > > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt > b/Documentation/devicetree/bindings/vendor-prefixes.txt > > index a580f3e..ec42bb4 100644 > > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt > > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt > > @@ -130,6 +130,7 @@ invensense InvenSense Inc. > > isee ISEE 2007 S.L. > > isil Intersil > > issi Integrated Silicon Solutions Inc. > > +jdi Japan Display Inc. > > jedec JEDEC Solid State Technology Association > > karo Ka-Ro electronics GmbH > > keymile Keymile GmbH > > This should be a separate patch as well. > > > diff --git a/drivers/gpu/drm/panel/Kconfig > b/drivers/gpu/drm/panel/Kconfig > > index 1500ab9..f41690e 100644 > > --- a/drivers/gpu/drm/panel/Kconfig > > +++ b/drivers/gpu/drm/panel/Kconfig > > @@ -61,6 +61,17 @@ config DRM_PANEL_SHARP_LQ101R1SX01 > > To compile this driver as a module, choose M here: the module > > will be called panel-sharp-lq101r1sx01. > > > > +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 vide= o/ > > + command mode panel as found in Google Nexus 7 (2013) devices. > > + The panel has a 1200(RGB)=C3=971920 (WUXGA) resolution and uses > > + 24 bit RGB per pixel. > > + > > config DRM_PANEL_SHARP_LS043T1LE01 > > tristate "Sharp LS043T1LE01 qHD video mode panel" > > depends on OF > > Please keep these sorted alphabetically. I do realize that the list > isn't sorted quite correctly at the moment, so you may as well leave > this as-is and I'll fix up the order when applying and after fixing > the current ordering. > > > diff --git a/drivers/gpu/drm/panel/Makefile > b/drivers/gpu/drm/panel/Makefile > > index f277eed..e6c6fc8 100644 > > --- a/drivers/gpu/drm/panel/Makefile > > +++ b/drivers/gpu/drm/panel/Makefile > > @@ -5,3 +5,4 @@ obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) +=3D > panel-samsung-ld9040.o > > obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) +=3D panel-samsung-s6e8aa0.o > > obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) +=3D panel-sharp-lq101r1sx01= .o > > obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) +=3D panel-sharp-ls043t1le01= .o > > +obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) +=3D panel-jdi-lt070me05000.o > > Same here. > > > 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..051aa1b > > --- /dev/null > > +++ b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c > > @@ -0,0 +1,685 @@ > > +/* > > + * Copyright (C) 2015 InforceComputing > > + * Author: Vinay Simha BN > > + * > > + * Copyright (C) 2015 Linaro Ltd > > + * Author: Sumit Semwal > > Should either of those copyright lines include 2016? We're a pretty good > way into 2016, and I'd be surprised if this wasn't touched this year at > all. > > > +/* > > + * From internet archives, the panel for Nexus 7 2nd Gen, 2013 model i= s > a > > + * JDI model LT070ME05000, and its data sheet is at: > > + * http://panelone.net/en/7-0-inch/JDI_LT070ME05000_7.0_inch-datashee= t > > + */ > > This comment is oddly placed here above the struct jdi_panel definition. > Perhaps move that information into the header comment? > > > +struct jdi_panel { > > + struct drm_panel base; > > + struct mipi_dsi_device *dsi; > > + > > + /* TODO: Add backilght support */ > > "backlight". Also if this is still TODO, why even have parts of the code > here? Just drop everything that's not used and move that to a follow-up > patch that implements full support. > > > + struct backlight_device *backlight; > > + struct regulator *supply; > > + struct gpio_desc *reset_gpio; > > + struct gpio_desc *enable_gpio; > > + struct gpio_desc *vcc_gpio; > > + > > + struct regulator *backlit; > > + struct regulator *lvs7; > > + struct regulator *avdd; > > + struct regulator *iovdd; > > + struct gpio_desc *pwm_gpio; > > Most of these resources aren't specified in the device tree binding, so > your driver doesn't properly implement the binding. You'll have to fix > the driver or the binding. > > > + > > + 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 char MCAP[2] =3D {0xB0, 0x00}; > > +static char interface_setting_cmd[6] =3D {0xB3, 0x04, 0x08, 0x00, 0x22= , > 0x00}; > > +static char interface_setting[6] =3D {0xB3, 0x26, 0x08, 0x00, 0x20, 0x= 00}; > > Please make all of these (and the ones below) arrays of u8, since that's > the type used in the prototype of the function that receives these as > parameters. Also, please use consistent capitalization and a space after > { and before }. > > > +static char interface_ID_setting[2] =3D {0xB4, 0x0C}; > > +static char DSI_control[3] =3D {0xB6, 0x3A, 0xD3}; > > + > > +static char tear_scan_line[3] =3D {0x44, 0x03, 0x00}; > > This is a standard DCS command, please turn this into a generic helper > such as mipi_dsi_dcs_set_tear_scanline(). > > > +/* for fps control, set fps to 60.32Hz */ > > +static char LTPS_timing_setting[2] =3D {0xC6, 0x78}; > > +static char sequencer_timing_control[2] =3D {0xD6, 0x01}; > > + > > +/* set brightness */ > > +static char write_display_brightness[] =3D {0x51, 0xFF}; > > Same here. > > > +/* enable LEDPWM pin output, turn on LEDPWM output, turn off pwm > dimming */ > > +static char write_control_display[] =3D {0x53, 0x24}; > > And here. > > > +/* > > + * choose cabc mode, 0x00(-0%), 0x01(-15%), 0x02(-40%), 0x03(-54%), > > + * disable SRE(sunlight readability enhancement) > > + */ > > +static char write_cabc[] =3D {0x55, 0x00}; > > And here. > > > +static int jdi_panel_init(struct jdi_panel *jdi) > > +{ > > + struct mipi_dsi_device *dsi =3D jdi->dsi; > > + int ret; > > + > > + dsi->mode_flags |=3D MIPI_DSI_MODE_LPM; > > + > > + if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) { > > + ret =3D mipi_dsi_dcs_soft_reset(dsi); > > + if (ret < 0) > > + return ret; > > + > > + mdelay(10); > > Please don't use mdelay() because it will busy-loop for a very long time > and waste precious CPU cycles. Instead, use usleep_range() for these > cases (or msleep() for durations longer than ~10 ms). Same goes for the > other occurrences below. > > > + > > + ret =3D mipi_dsi_dcs_set_pixel_format(dsi, 0x70); > > + if (ret < 0) > > + return ret; > > There are symbolic constants for the pixel formats, use them to convey > meaning. > > > + > > + ret =3D mipi_dsi_dcs_set_column_address(dsi, 0x0000, 0x04= AF); > > + if (ret < 0) > > + return ret; > > + > > + ret =3D mipi_dsi_dcs_set_page_address(dsi, 0x0000, 0x077F= ); > > + if (ret < 0) > > + return ret; > > These should be parameterized on the panel width and height, to make it > clear where the values come from. > > > + > > + ret =3D mipi_dsi_dcs_set_tear_on(dsi, > > + > MIPI_DSI_DCS_TEAR_MODE_VBLANK); > > + if (ret < 0) > > + return ret; > > + mdelay(5); > > + > > + ret =3D mipi_dsi_generic_write(dsi, tear_scan_line, > > + sizeof(tear_scan_line)); > > + if (ret < 0) > > + return ret; > > Like I mentioned earlier, this should be a generic helper to help make > its intention clearer. > > > + > > + ret =3D mipi_dsi_dcs_write_buffer(dsi, > write_display_brightness, > > + > sizeof(write_display_brightness) > > + ); > > + if (ret < 0) > > + return ret; > > + > > + ret =3D mipi_dsi_dcs_write_buffer(dsi, write_control_disp= lay, > > + > sizeof(write_control_display)); > > + if (ret < 0) > > + return ret; > > + > > + ret =3D mipi_dsi_dcs_write_buffer(dsi, write_cabc, > > + sizeof(write_cabc)); > > + if (ret < 0) > > + return ret; > > Same for these. I don't currently have access to the DCS 1.3 > specification, but I suspect that some of the parameters for these > commands could be defined via symbolic names. > > > + > > + ret =3D mipi_dsi_dcs_exit_sleep_mode(dsi); > > + if (ret < 0) > > + return ret; > > + mdelay(120); > > + > > + ret =3D mipi_dsi_generic_write(dsi, MCAP, sizeof(MCAP)); > > + if (ret < 0) > > + return ret; > > + mdelay(10); > > + > > + ret =3D mipi_dsi_generic_write(dsi, interface_setting, > > + sizeof(interface_setting)); > > + if (ret < 0) > > + return ret; > > + mdelay(20); > > + > > + backlight_control4[18] =3D 0x04; > > + backlight_control4[19] =3D 0x00; > > + > > + ret =3D mipi_dsi_generic_write(dsi, backlight_control4, > > + sizeof(backlight_control4)); > > + if (ret < 0) > > + return ret; > > + mdelay(20); > > + > > + MCAP[1] =3D 0x03; > > + ret =3D mipi_dsi_generic_write(dsi, MCAP, sizeof(MCAP)); > > + if (ret < 0) > > + return ret; > > That's odd. Doesn't this break idempotence of this function? You modify > the global MCAP array by writing 0x03 to MCAP[1], so when this function > is called a second time, MCAP[1] will be 0x03 already in the first call > a few lines above, instead of the 0x00 that it was initially. > > > + } else { > > + /* > > + * TODO : need to verify panel settings when > > + * dsi cmd mode supported for apq8064 - simhavcs > > + */ > > + ret =3D mipi_dsi_dcs_soft_reset(dsi); > > + if (ret < 0) > > + return ret; > > This whole else clause is dead code because the condition for the if > clause is always true. Just drop it and add it back when you properly > support both modes. > > > +static int jdi_panel_on(struct jdi_panel *jdi) > > +{ > > + struct mipi_dsi_device *dsi =3D jdi->dsi; > > + int ret; > > + > > + dsi->mode_flags |=3D MIPI_DSI_MODE_LPM; > > + > > + ret =3D 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 =3D jdi->dsi; > > + int ret; > > + > > + dsi->mode_flags &=3D ~MIPI_DSI_MODE_LPM; > > Should this not be cleared at the end of the function so that the below > commands still get run in LP mode? > > > + > > + ret =3D mipi_dsi_dcs_set_display_off(dsi); > > + if (ret < 0) > > + return ret; > > + > > + ret =3D mipi_dsi_dcs_enter_sleep_mode(dsi); > > + if (ret < 0) > > + return ret; > > + > > + ret =3D 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 =3D to_jdi_panel(panel); > > + > > + if (!jdi->enabled) > > + return 0; > > + > > + DRM_DEBUG("disable\n"); > > This doesn't look very useful debug information. I think you should > remove it. Same for other similar occurrences below. > > > +static const struct drm_panel_funcs jdi_panel_funcs =3D { > > + .disable =3D jdi_panel_disable, > > + .unprepare =3D jdi_panel_unprepare, > > + .prepare =3D jdi_panel_prepare, > > + .enable =3D jdi_panel_enable, > > + .get_modes =3D jdi_panel_get_modes, > > +}; > > + > > +static const struct of_device_id jdi_of_match[] =3D { > > + { .compatible =3D "jdi,lt070me05000", }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(of, jdi_of_match); > > A single tab is enough to indent the above. > > > +static int jdi_panel_add(struct jdi_panel *jdi) > > +{ > > + struct device *dev =3D &jdi->dsi->dev; > > + int ret; > > + > > + jdi->mode =3D &default_mode; > > What do you keep this around for? > > > + > > + /* lvs5 */ > > + jdi->supply =3D devm_regulator_get(dev, "power"); > > + if (IS_ERR(jdi->supply)) > > + return PTR_ERR(jdi->supply); > > + > > + /* l17 */ > > + jdi->backlit =3D devm_regulator_get(dev, "backlit"); > > + if (IS_ERR(jdi->supply)) > > + return PTR_ERR(jdi->supply); > > + > > + jdi->lvs7 =3D devm_regulator_get(dev, "lvs7"); > > + if (IS_ERR(jdi->lvs7)) > > + return PTR_ERR(jdi->lvs7); > > + > > + jdi->avdd =3D devm_regulator_get(dev, "avdd"); > > + if (IS_ERR(jdi->avdd)) > > + return PTR_ERR(jdi->avdd); > > + > > + jdi->iovdd =3D devm_regulator_get(dev, "iovdd"); > > + if (IS_ERR(jdi->iovdd)) > > + return PTR_ERR(jdi->iovdd); > > + > > + jdi->vcc_gpio =3D devm_gpiod_get(dev, "vcc", GPIOD_OUT_LOW); > > + if (IS_ERR(jdi->vcc_gpio)) { > > + dev_err(dev, "cannot get vcc-gpio %ld\n", > > + PTR_ERR(jdi->vcc_gpio)); > > + jdi->vcc_gpio =3D NULL; > > + } else { > > + gpiod_direction_output(jdi->vcc_gpio, 0); > > + } > > + > > + jdi->reset_gpio =3D 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 =3D NULL; > > + } else { > > + gpiod_direction_output(jdi->reset_gpio, 0); > > + } > > + > > + jdi->enable_gpio =3D 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 =3D NULL; > > + } else { > > + gpiod_direction_output(jdi->enable_gpio, 0); > > + } > > + > > + jdi->pwm_gpio =3D devm_gpiod_get(dev, "pwm", GPIOD_OUT_LOW); > > + if (IS_ERR(jdi->pwm_gpio)) { > > + dev_err(dev, "cannot get pwm-gpio %ld\n", > > + PTR_ERR(jdi->pwm_gpio)); > > + jdi->pwm_gpio =3D NULL; > > + } else { > > + gpiod_direction_output(jdi->pwm_gpio, 0); > > + } > > As I said earlier, most of these aren't specified in the binding, fix > either the binding or the code. > > > + /* we don't have backlight right now, proceed further */ > > +#ifdef BACKLIGHT > > + np =3D of_parse_phandle(dev->of_node, "backlight", 0); > > + if (np) { > > + jdi->backlight =3D of_find_backlight_by_node(np); > > + of_node_put(np); > > + > > + if (!jdi->backlight) > > + return -EPROBE_DEFER; > > + } > > +#endif > > Again, if you don't support it, don't submit it. We don't want dead code > in the kernel. > > > +MODULE_AUTHOR("Sumit Semwal "); > > +MODULE_AUTHOR("Vinay Simha BN "); > > +MODULE_DESCRIPTION("JDI WUXGA LT070ME05000 DSI video/command mode pane= l > driver"); > > Technically this doesn't support command mode yet, so you might want to > remove that from the description to avoid confusion. > > Thierry > --001a113ee4bc23692105305f5921 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

In Previous email , forgot to add cc: archit.

regards,
vinay simha bn

On 13-Apr-2016 19:19, "Thierry Reding"= <thierry.reding@gmail.com> wrote:
On Wed= , Apr 13, 2016 at 11:58:04AM +0530, Vinay Simha BN wrote:
> 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:
>=C2=A0 =C2=A0 =C2=A0https://android.googlesource.= com/kernel/msm.git
>
> And video mode setting is from dsi-panel-jdi-dualmipi1-video.dtsi
> file in:
>=C2=A0 =C2=A0 =C2=A0git://codeaurora.org/kernel/msm-3.1= 0.git=C2=A0 LNX.LA.3.6_rb1.27
>
> Other fixes folded in were provided
> by Archit Taneja <archit= .taneja@gmail.com>
>
> Signed-off-by: Vinay Simha BN <simhavcs@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
>=C2=A0 from Vinay and Archit]
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>=C2=A0 .../bindings/display/panel/jdi,lt070me05000.txt=C2=A0 =C2=A0 |= =C2=A0 27 +
>=C2=A0 .../devicetree/bindings/vendor-prefixes.txt=C2=A0 =C2=A0 =C2=A0 = =C2=A0 |=C2=A0 =C2=A01 +
>=C2=A0 drivers/gpu/drm/panel/Kconfig=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 11 +
>=C2=A0 drivers/gpu/drm/panel/Makefile=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 =C2=A01 +
>=C2=A0 drivers/gpu/drm/panel/panel-jdi-lt070me05000.c=C2=A0 =C2=A0 =C2= =A0| 685 +++++++++++++++++++++
>=C2=A0 5 files changed, 725 insertions(+)
>=C2=A0 create mode 100644 Documentation/devicetree/bindings/display/pan= el/jdi,lt070me05000.txt
>=C2=A0 create mode 100644 drivers/gpu/drm/panel/panel-jdi-lt070me05000.= c

What's the difference between this and the patch you sent earlier? I= 9;m
going to assume that the newer one is the correct patch, so I'll ignore=
the previous patch.

> diff --git a/Documentation/devicetree/bindings/display/panel/jdi,lt070= me05000.txt b/Documentation/devicetree/bindings/display/panel/jdi,lt070me05= 000.txt
> new file mode 100644
> index 0000000..35c5ac7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/jdi,lt070me05000= .txt

The binding documentation should be a separate patch.

> @@ -0,0 +1,27 @@
> +JDI model LT070ME05000 1920x1200 7" DSI Panel
> +
> +Basic data sheet is at:
> +=C2=A0 =C2=A0 =C2=A0http://= panelone.net/en/7-0-inch/JDI_LT070ME05000_7.0_inch-datasheet
> +
> +This panel has video mode implemented currently in the driver.

That's information irrelevant to the DT binding, since you're presu= mably
talking about the Linux drm/panel driver, whereas the DT binding is
supposed to specify the description of the panel hardware in OS-agnostic terms.

> +Required properties:
> +- compatible: should be "jdi,lt070me05000"
> +
> +Optional properties:
> +- power-supply: phandle of the regulator that provides the supply vol= tage
> +- reset-gpio: phandle of gpio for reset line
> +- backlight: phandle of the backlight device attached to the panel > +
> +Example:
> +
> +=C2=A0 =C2=A0 =C2=A0dsi@54300000 {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0panel: panel@0 {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0compatible =3D "jdi,lt070me05000";
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0reg =3D <0>;
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0power-supply =3D <...>;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0reset-gpio =3D <...>;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0backlight =3D <...>;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0};
> +=C2=A0 =C2=A0 =C2=A0};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/D= ocumentation/devicetree/bindings/vendor-prefixes.txt
> index a580f3e..ec42bb4 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -130,6 +130,7 @@ invensense=C2=A0 =C2=A0 =C2=A0 =C2=A0 InvenSense I= nc.
>=C2=A0 isee ISEE 2007 S.L.
>=C2=A0 isil Intersil
>=C2=A0 issi Integrated Silicon Solutions Inc.
> +jdi=C2=A0 Japan Display Inc.
>=C2=A0 jedec=C2=A0 =C2=A0 =C2=A0 =C2=A0 JEDEC Solid State Technology As= sociation
>=C2=A0 karo Ka-Ro electronics GmbH
>=C2=A0 keymile=C2=A0 =C2=A0 =C2=A0 Keymile GmbH

This should be a separate patch as well.

> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kco= nfig
> index 1500ab9..f41690e 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -61,6 +61,17 @@ config DRM_PANEL_SHARP_LQ101R1SX01
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0To compile this driver as a module, c= hoose M here: the module
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0will be called panel-sharp-lq101r1sx0= 1.
>
> +config DRM_PANEL_JDI_LT070ME05000
> +=C2=A0 =C2=A0 =C2=A0tristate "JDI LT070ME05000 WUXGA DSI panel&q= uot;
> +=C2=A0 =C2=A0 =C2=A0depends on OF
> +=C2=A0 =C2=A0 =C2=A0depends on DRM_MIPI_DSI
> +=C2=A0 =C2=A0 =C2=A0depends on BACKLIGHT_CLASS_DEVICE
> +=C2=A0 =C2=A0 =C2=A0help
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0Say Y here if you want to enable support f= or JDI WUXGA DSI video/
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0command mode panel as found in Google Nexu= s 7 (2013) devices.
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0The panel has a 1200(RGB)=C3=971920 (WUXGA= ) resolution and uses
> +=C2=A0 =C2=A0 =C2=A0 =C2=A024 bit RGB per pixel.
> +
>=C2=A0 config DRM_PANEL_SHARP_LS043T1LE01
>=C2=A0 =C2=A0 =C2=A0 =C2=A0tristate "Sharp LS043T1LE01 qHD video m= ode panel"
>=C2=A0 =C2=A0 =C2=A0 =C2=A0depends on OF

Please keep these sorted alphabetically. I do realize that the list
isn't sorted quite correctly at the moment, so you may as well leave this as-is and I'll fix up the order when applying and after fixing
the current ordering.

> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Ma= kefile
> index f277eed..e6c6fc8 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -5,3 +5,4 @@ obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) +=3D panel-sams= ung-ld9040.o
>=C2=A0 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) +=3D panel-samsung-s6e8a= a0.o
>=C2=A0 obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) +=3D panel-sharp-lq101= r1sx01.o
>=C2=A0 obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) +=3D panel-sharp-ls043= t1le01.o
> +obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) +=3D panel-jdi-lt070me05000.= o

Same here.

> 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..051aa1b
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
> @@ -0,0 +1,685 @@
> +/*
> + * Copyright (C) 2015 InforceComputing
> + * Author: Vinay Simha BN <s= imhavcs@gmail.com>
> + *
> + * Copyright (C) 2015 Linaro Ltd
> + * Author: Sumit Semwal <sumit.semwal@linaro.org>

Should either of those copyright lines include 2016? We're a pretty goo= d
way into 2016, and I'd be surprised if this wasn't touched this yea= r at
all.

> +/*
> + * From internet archives, the panel for Nexus 7 2nd Gen, 2013 model = is a
> + * JDI model LT070ME05000, and its data sheet is at:
> + *=C2=A0 http://panelone.ne= t/en/7-0-inch/JDI_LT070ME05000_7.0_inch-datasheet
> + */

This comment is oddly placed here above the struct jdi_panel definition. Perhaps move that information into the header comment?

> +struct jdi_panel {
> +=C2=A0 =C2=A0 =C2=A0struct drm_panel base;
> +=C2=A0 =C2=A0 =C2=A0struct mipi_dsi_device *dsi;
> +
> +=C2=A0 =C2=A0 =C2=A0/* TODO: Add backilght support */

"backlight". Also if this is still TODO, why even have parts of t= he code
here? Just drop everything that's not used and move that to a follow-up=
patch that implements full support.

> +=C2=A0 =C2=A0 =C2=A0struct backlight_device *backlight;
> +=C2=A0 =C2=A0 =C2=A0struct regulator *supply;
> +=C2=A0 =C2=A0 =C2=A0struct gpio_desc *reset_gpio;
> +=C2=A0 =C2=A0 =C2=A0struct gpio_desc *enable_gpio;
> +=C2=A0 =C2=A0 =C2=A0struct gpio_desc *vcc_gpio;
> +
> +=C2=A0 =C2=A0 =C2=A0struct regulator *backlit;
> +=C2=A0 =C2=A0 =C2=A0struct regulator *lvs7;
> +=C2=A0 =C2=A0 =C2=A0struct regulator *avdd;
> +=C2=A0 =C2=A0 =C2=A0struct regulator *iovdd;
> +=C2=A0 =C2=A0 =C2=A0struct gpio_desc *pwm_gpio;

Most of these resources aren't specified in the device tree binding, so=
your driver doesn't properly implement the binding. You'll have to = fix
the driver or the binding.

> +
> +=C2=A0 =C2=A0 =C2=A0bool prepared;
> +=C2=A0 =C2=A0 =C2=A0bool enabled;
> +
> +=C2=A0 =C2=A0 =C2=A0const struct drm_display_mode *mode;
> +};
> +
> +static inline struct jdi_panel *to_jdi_panel(struct drm_panel *panel)=
> +{
> +=C2=A0 =C2=A0 =C2=A0return container_of(panel, struct jdi_panel, base= );
> +}
> +
> +static char MCAP[2] =3D {0xB0, 0x00};
> +static char interface_setting_cmd[6] =3D {0xB3, 0x04, 0x08, 0x00, 0x2= 2, 0x00};
> +static char interface_setting[6] =3D {0xB3, 0x26, 0x08, 0x00, 0x20, 0= x00};

Please make all of these (and the ones below) arrays of u8, since that'= s
the type used in the prototype of the function that receives these as
parameters. Also, please use consistent capitalization and a space after { and before }.

> +static char interface_ID_setting[2] =3D {0xB4, 0x0C};
> +static char DSI_control[3] =3D {0xB6, 0x3A, 0xD3};
> +
> +static char tear_scan_line[3] =3D {0x44, 0x03, 0x00};

This is a standard DCS command, please turn this into a generic helper
such as mipi_dsi_dcs_set_tear_scanline().

> +/* for fps control, set fps to 60.32Hz */
> +static char LTPS_timing_setting[2] =3D {0xC6, 0x78};
> +static char sequencer_timing_control[2] =3D {0xD6, 0x01};
> +
> +/* set brightness */
> +static char write_display_brightness[] =3D {0x51, 0xFF};

Same here.

> +/* enable LEDPWM pin output, turn on LEDPWM output, turn off pwm dimm= ing */
> +static char write_control_display[] =3D {0x53, 0x24};

And here.

> +/*
> + * choose cabc mode, 0x00(-0%), 0x01(-15%), 0x02(-40%), 0x03(-54%), > + * disable SRE(sunlight readability enhancement)
> + */
> +static char write_cabc[] =3D {0x55, 0x00};

And here.

> +static int jdi_panel_init(struct jdi_panel *jdi)
> +{
> +=C2=A0 =C2=A0 =C2=A0struct mipi_dsi_device *dsi =3D jdi->dsi;
> +=C2=A0 =C2=A0 =C2=A0int ret;
> +
> +=C2=A0 =C2=A0 =C2=A0dsi->mode_flags |=3D MIPI_DSI_MODE_LPM;
> +
> +=C2=A0 =C2=A0 =C2=A0if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO)= {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D mipi_dsi_dcs_= soft_reset(dsi);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return ret;
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0mdelay(10);

Please don't use mdelay() because it will busy-loop for a very long tim= e
and waste precious CPU cycles. Instead, use usleep_range() for these
cases (or msleep() for durations longer than ~10 ms). Same goes for the
other occurrences below.

> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D mipi_dsi_dcs_= set_pixel_format(dsi, 0x70);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return ret;

There are symbolic constants for the pixel formats, use them to convey
meaning.

> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D mipi_dsi_dcs_= set_column_address(dsi, 0x0000, 0x04AF);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return ret;
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D mipi_dsi_dcs_= set_page_address(dsi, 0x0000, 0x077F);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return ret;

These should be parameterized on the panel width and height, to make it
clear where the values come from.

> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D mipi_dsi_dcs_= set_tear_on(dsi,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return ret;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0mdelay(5);
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D mipi_dsi_gene= ric_write(dsi, tear_scan_line,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 sizeof(tear_scan_line));
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return ret;

Like I mentioned earlier, this should be a generic helper to help make
its intention clearer.

> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D mipi_dsi_dcs_= write_buffer(dsi, write_display_brightness,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0sizeof(write_display_brightness)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return ret;
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D mipi_dsi_dcs_= write_buffer(dsi, write_control_display,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0sizeof(write_control_display));
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return ret;
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D mipi_dsi_dcs_= write_buffer(dsi, write_cabc,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0sizeof(write_cabc));
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return ret;

Same for these. I don't currently have access to the DCS 1.3
specification, but I suspect that some of the parameters for these
commands could be defined via symbolic names.

> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D mipi_dsi_dcs_= exit_sleep_mode(dsi);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return ret;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0mdelay(120);
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D mipi_dsi_gene= ric_write(dsi, MCAP, sizeof(MCAP));
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return ret;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0mdelay(10);
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D mipi_dsi_gene= ric_write(dsi, interface_setting,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 sizeof(interface_setting));
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return ret;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0mdelay(20);
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0backlight_control4[18= ] =3D 0x04;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0backlight_control4[19= ] =3D 0x00;
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D mipi_dsi_gene= ric_write(dsi, backlight_control4,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 sizeof(backlight_control4));
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return ret;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0mdelay(20);
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0MCAP[1] =3D 0x03;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D mipi_dsi_gene= ric_write(dsi, MCAP, sizeof(MCAP));
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return ret;

That's odd. Doesn't this break idempotence of this function? You mo= dify
the global MCAP array by writing 0x03 to MCAP[1], so when this function
is called a second time, MCAP[1] will be 0x03 already in the first call
a few lines above, instead of the 0x00 that it was initially.

> +=C2=A0 =C2=A0 =C2=A0} else {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0/*
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * TODO : need to ver= ify panel settings when
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 * dsi cmd mode suppo= rted for apq8064 - simhavcs
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0ret =3D mipi_dsi_dcs_= soft_reset(dsi);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return ret;

This whole else clause is dead code because the condition for the if
clause is always true. Just drop it and add it back when you properly
support both modes.

> +static int jdi_panel_on(struct jdi_panel *jdi)
> +{
> +=C2=A0 =C2=A0 =C2=A0struct mipi_dsi_device *dsi =3D jdi->dsi;
> +=C2=A0 =C2=A0 =C2=A0int ret;
> +
> +=C2=A0 =C2=A0 =C2=A0dsi->mode_flags |=3D MIPI_DSI_MODE_LPM;
> +
> +=C2=A0 =C2=A0 =C2=A0ret =3D mipi_dsi_dcs_set_display_on(dsi);
> +=C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return ret;
> +
> +=C2=A0 =C2=A0 =C2=A0return 0;
> +}
> +
> +static int jdi_panel_off(struct jdi_panel *jdi)
> +{
> +=C2=A0 =C2=A0 =C2=A0struct mipi_dsi_device *dsi =3D jdi->dsi;
> +=C2=A0 =C2=A0 =C2=A0int ret;
> +
> +=C2=A0 =C2=A0 =C2=A0dsi->mode_flags &=3D ~MIPI_DSI_MODE_LPM;
Should this not be cleared at the end of the function so that the below
commands still get run in LP mode?

> +
> +=C2=A0 =C2=A0 =C2=A0ret =3D mipi_dsi_dcs_set_display_off(dsi);
> +=C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return ret;
> +
> +=C2=A0 =C2=A0 =C2=A0ret =3D mipi_dsi_dcs_enter_sleep_mode(dsi);
> +=C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return ret;
> +
> +=C2=A0 =C2=A0 =C2=A0ret =3D mipi_dsi_dcs_set_tear_off(dsi);
> +=C2=A0 =C2=A0 =C2=A0if (ret < 0)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return ret;
> +
> +=C2=A0 =C2=A0 =C2=A0msleep(100);
> +
> +=C2=A0 =C2=A0 =C2=A0return 0;
> +}
> +
> +static int jdi_panel_disable(struct drm_panel *panel)
> +{
> +=C2=A0 =C2=A0 =C2=A0struct jdi_panel *jdi =3D to_jdi_panel(panel); > +
> +=C2=A0 =C2=A0 =C2=A0if (!jdi->enabled)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
> +
> +=C2=A0 =C2=A0 =C2=A0DRM_DEBUG("disable\n");

This doesn't look very useful debug information. I think you should
remove it. Same for other similar occurrences below.

> +static const struct drm_panel_funcs jdi_panel_funcs =3D {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.disable =3D jdi_pane= l_disable,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.unprepare =3D jdi_pa= nel_unprepare,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.prepare =3D jdi_pane= l_prepare,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.enable =3D jdi_panel= _enable,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.get_modes =3D jdi_pa= nel_get_modes,
> +};
> +
> +static const struct of_device_id jdi_of_match[] =3D {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{ .compatible =3D &qu= ot;jdi,lt070me05000", },
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{ }
> +};
> +MODULE_DEVICE_TABLE(of, jdi_of_match);

A single tab is enough to indent the above.

> +static int jdi_panel_add(struct jdi_panel *jdi)
> +{
> +=C2=A0 =C2=A0 =C2=A0struct device *dev =3D &jdi->dsi->dev;<= br> > +=C2=A0 =C2=A0 =C2=A0int ret;
> +
> +=C2=A0 =C2=A0 =C2=A0jdi->mode =3D &default_mode;

What do you keep this around for?

> +
> +=C2=A0 =C2=A0 =C2=A0/* lvs5 */
> +=C2=A0 =C2=A0 =C2=A0jdi->supply =3D devm_regulator_get(dev, "= power");
> +=C2=A0 =C2=A0 =C2=A0if (IS_ERR(jdi->supply))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return PTR_ERR(jdi-&g= t;supply);
> +
> +=C2=A0 =C2=A0 =C2=A0/* l17 */
> +=C2=A0 =C2=A0 =C2=A0jdi->backlit =3D devm_regulator_get(dev, "= ;backlit");
> +=C2=A0 =C2=A0 =C2=A0if (IS_ERR(jdi->supply))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return PTR_ERR(jdi-&g= t;supply);
> +
> +=C2=A0 =C2=A0 =C2=A0jdi->lvs7 =3D devm_regulator_get(dev, "lv= s7");
> +=C2=A0 =C2=A0 =C2=A0if (IS_ERR(jdi->lvs7))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return PTR_ERR(jdi-&g= t;lvs7);
> +
> +=C2=A0 =C2=A0 =C2=A0jdi->avdd =3D devm_regulator_get(dev, "av= dd");
> +=C2=A0 =C2=A0 =C2=A0if (IS_ERR(jdi->avdd))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return PTR_ERR(jdi-&g= t;avdd);
> +
> +=C2=A0 =C2=A0 =C2=A0jdi->iovdd =3D devm_regulator_get(dev, "i= ovdd");
> +=C2=A0 =C2=A0 =C2=A0if (IS_ERR(jdi->iovdd))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return PTR_ERR(jdi-&g= t;iovdd);
> +
> +=C2=A0 =C2=A0 =C2=A0jdi->vcc_gpio =3D devm_gpiod_get(dev, "vc= c", GPIOD_OUT_LOW);
> +=C2=A0 =C2=A0 =C2=A0if (IS_ERR(jdi->vcc_gpio)) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dev_err(dev, "ca= nnot get vcc-gpio %ld\n",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0PTR_ERR(jdi->vcc_gpio));
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0jdi->vcc_gpio =3D = NULL;
> +=C2=A0 =C2=A0 =C2=A0} else {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0gpiod_direction_outpu= t(jdi->vcc_gpio, 0);
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0jdi->reset_gpio =3D devm_gpiod_get(dev, "= reset", GPIOD_OUT_LOW);
> +=C2=A0 =C2=A0 =C2=A0if (IS_ERR(jdi->reset_gpio)) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dev_err(dev, "ca= nnot get reset-gpios %ld\n",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0PTR_ERR(jdi->reset_gpio));
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0jdi->reset_gpio = =3D NULL;
> +=C2=A0 =C2=A0 =C2=A0} else {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0gpiod_direction_outpu= t(jdi->reset_gpio, 0);
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0jdi->enable_gpio =3D devm_gpiod_get(dev, "= ;enable", GPIOD_OUT_LOW);
> +=C2=A0 =C2=A0 =C2=A0if (IS_ERR(jdi->enable_gpio)) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dev_err(dev, "ca= nnot get enable-gpio %ld\n",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0PTR_ERR(jdi->enable_gpio));
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0jdi->enable_gpio = =3D NULL;
> +=C2=A0 =C2=A0 =C2=A0} else {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0gpiod_direction_outpu= t(jdi->enable_gpio, 0);
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0jdi->pwm_gpio =3D devm_gpiod_get(dev, "pw= m", GPIOD_OUT_LOW);
> +=C2=A0 =C2=A0 =C2=A0if (IS_ERR(jdi->pwm_gpio)) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dev_err(dev, "ca= nnot get pwm-gpio %ld\n",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0PTR_ERR(jdi->pwm_gpio));
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0jdi->pwm_gpio =3D = NULL;
> +=C2=A0 =C2=A0 =C2=A0} else {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0gpiod_direction_outpu= t(jdi->pwm_gpio, 0);
> +=C2=A0 =C2=A0 =C2=A0}

As I said earlier, most of these aren't specified in the binding, fix either the binding or the code.

> +=C2=A0 =C2=A0 =C2=A0/* we don't have backlight right now, proceed= further */
> +#ifdef BACKLIGHT
> +=C2=A0 =C2=A0 =C2=A0np =3D of_parse_phandle(dev->of_node, "ba= cklight", 0);
> +=C2=A0 =C2=A0 =C2=A0if (np) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0jdi->backlight =3D= of_find_backlight_by_node(np);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0of_node_put(np);
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!jdi->backligh= t)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return -EPROBE_DEFER;
> +=C2=A0 =C2=A0 =C2=A0}
> +#endif

Again, if you don't support it, don't submit it. We don't want = dead code
in the kernel.

> +MODULE_AUTHOR("Sumit Semwal <sumit.semwal@linaro.org>");
> +MODULE_AUTHOR("Vinay Simha BN <simhavcs@gmail.com>");
> +MODULE_DESCRIPTION("JDI WUXGA LT070ME05000 DSI video/command mod= e panel driver");

Technically this doesn't support command mode yet, so you might want to=
remove that from the description to avoid confusion.

Thierry
--001a113ee4bc23692105305f5921--