Thierry Reding, Thanks for the review. Will address your reviews and resend the patches. On Wed, Apr 13, 2016 at 7:19 PM, Thierry Reding <thierry.reding@xxxxxxxxx> 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 <archit.taneja@xxxxxxxxx> >> >> Signed-off-by: Vinay Simha BN <simhavcs@xxxxxxxxx> >> [sumit.semwal: Ported to the drm/panel framework] >> Signed-off-by: Sumit Semwal <sumit.semwal@xxxxxxxxxx> >> [jstultz: Cherry-picked to mainline, folded down other fixes >> from Vinay and Archit] >> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx> >> --- >> .../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 = "jdi,lt070me05000"; >> + reg = <0>; >> + >> + power-supply = <...>; >> + reset-gpio = <...>; >> + backlight = <...>; >> + }; >> + }; >> 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 video/ >> + command 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_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) += panel-samsung-ld9040.o >> obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += panel-samsung-s6e8aa0.o >> obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o >> obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) += panel-sharp-ls043t1le01.o >> +obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += 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 <simhavcs@xxxxxxxxx> >> + * >> + * Copyright (C) 2015 Linaro Ltd >> + * Author: Sumit Semwal <sumit.semwal@xxxxxxxxxx> > > 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 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 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] = {0xB0, 0x00}; >> +static char interface_setting_cmd[6] = {0xB3, 0x04, 0x08, 0x00, 0x22, 0x00}; >> +static char interface_setting[6] = {0xB3, 0x26, 0x08, 0x00, 0x20, 0x00}; > > 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] = {0xB4, 0x0C}; >> +static char DSI_control[3] = {0xB6, 0x3A, 0xD3}; >> + >> +static char tear_scan_line[3] = {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] = {0xC6, 0x78}; >> +static char sequencer_timing_control[2] = {0xD6, 0x01}; >> + >> +/* set brightness */ >> +static char write_display_brightness[] = {0x51, 0xFF}; > > Same here. > >> +/* enable LEDPWM pin output, turn on LEDPWM output, turn off pwm dimming */ >> +static char write_control_display[] = {0x53, 0x24}; > > And here. > >> +/* >> + * choose cabc mode, 0x00(-0%), 0x01(-15%), 0x02(-40%), 0x03(-54%), >> + * disable SRE(sunlight readability enhancement) >> + */ >> +static char write_cabc[] = {0x55, 0x00}; > > And here. > >> +static int jdi_panel_init(struct jdi_panel *jdi) >> +{ >> + struct mipi_dsi_device *dsi = jdi->dsi; >> + int ret; >> + >> + dsi->mode_flags |= MIPI_DSI_MODE_LPM; >> + >> + if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) { >> + ret = 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 = 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 = 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; > > These should be parameterized on the panel width and height, to make it > clear where the values come from. > >> + >> + ret = mipi_dsi_dcs_set_tear_on(dsi, >> + MIPI_DSI_DCS_TEAR_MODE_VBLANK); >> + if (ret < 0) >> + return ret; >> + mdelay(5); >> + >> + ret = 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 = mipi_dsi_dcs_write_buffer(dsi, write_display_brightness, >> + sizeof(write_display_brightness) >> + ); >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_write_buffer(dsi, write_control_display, >> + sizeof(write_control_display)); >> + if (ret < 0) >> + return ret; >> + >> + ret = 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 = mipi_dsi_dcs_exit_sleep_mode(dsi); >> + if (ret < 0) >> + return ret; >> + mdelay(120); >> + >> + ret = mipi_dsi_generic_write(dsi, MCAP, sizeof(MCAP)); >> + if (ret < 0) >> + return ret; >> + mdelay(10); >> + >> + ret = mipi_dsi_generic_write(dsi, interface_setting, >> + sizeof(interface_setting)); >> + if (ret < 0) >> + return ret; >> + mdelay(20); >> + >> + backlight_control4[18] = 0x04; >> + backlight_control4[19] = 0x00; >> + >> + ret = mipi_dsi_generic_write(dsi, backlight_control4, >> + sizeof(backlight_control4)); >> + if (ret < 0) >> + return ret; >> + mdelay(20); >> + >> + MCAP[1] = 0x03; >> + ret = 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 = 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 = jdi->dsi; >> + int ret; >> + >> + dsi->mode_flags |= MIPI_DSI_MODE_LPM; >> + >> + ret = mipi_dsi_dcs_set_display_on(dsi); >> + if (ret < 0) >> + return ret; >> + >> + return 0; >> +} >> + >> +static int jdi_panel_off(struct jdi_panel *jdi) >> +{ >> + struct mipi_dsi_device *dsi = jdi->dsi; >> + int ret; >> + >> + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; > > Should this not be cleared at the end of the function so that the below > commands still get run in LP mode? > >> + >> + ret = mipi_dsi_dcs_set_display_off(dsi); >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_enter_sleep_mode(dsi); >> + if (ret < 0) >> + return ret; >> + >> + ret = mipi_dsi_dcs_set_tear_off(dsi); >> + if (ret < 0) >> + return ret; >> + >> + msleep(100); >> + >> + return 0; >> +} >> + >> +static int jdi_panel_disable(struct drm_panel *panel) >> +{ >> + struct jdi_panel *jdi = to_jdi_panel(panel); >> + >> + if (!jdi->enabled) >> + return 0; >> + >> + DRM_DEBUG("disable\n"); > > 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 = { >> + .disable = jdi_panel_disable, >> + .unprepare = jdi_panel_unprepare, >> + .prepare = jdi_panel_prepare, >> + .enable = jdi_panel_enable, >> + .get_modes = jdi_panel_get_modes, >> +}; >> + >> +static const struct of_device_id jdi_of_match[] = { >> + { .compatible = "jdi,lt070me05000", }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(of, jdi_of_match); > > A single tab is enough to indent the above. > >> +static int jdi_panel_add(struct jdi_panel *jdi) >> +{ >> + struct device *dev = &jdi->dsi->dev; >> + int ret; >> + >> + jdi->mode = &default_mode; > > What do you keep this around for? > >> + >> + /* lvs5 */ >> + jdi->supply = devm_regulator_get(dev, "power"); >> + if (IS_ERR(jdi->supply)) >> + return PTR_ERR(jdi->supply); >> + >> + /* l17 */ >> + jdi->backlit = devm_regulator_get(dev, "backlit"); >> + if (IS_ERR(jdi->supply)) >> + return PTR_ERR(jdi->supply); >> + >> + jdi->lvs7 = devm_regulator_get(dev, "lvs7"); >> + if (IS_ERR(jdi->lvs7)) >> + return PTR_ERR(jdi->lvs7); >> + >> + jdi->avdd = devm_regulator_get(dev, "avdd"); >> + if (IS_ERR(jdi->avdd)) >> + return PTR_ERR(jdi->avdd); >> + >> + jdi->iovdd = devm_regulator_get(dev, "iovdd"); >> + if (IS_ERR(jdi->iovdd)) >> + return PTR_ERR(jdi->iovdd); >> + >> + jdi->vcc_gpio = 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 = NULL; >> + } else { >> + gpiod_direction_output(jdi->vcc_gpio, 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)); >> + jdi->reset_gpio = NULL; >> + } else { >> + gpiod_direction_output(jdi->reset_gpio, 0); >> + } >> + >> + jdi->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW); >> + if (IS_ERR(jdi->enable_gpio)) { >> + dev_err(dev, "cannot get enable-gpio %ld\n", >> + PTR_ERR(jdi->enable_gpio)); >> + jdi->enable_gpio = NULL; >> + } else { >> + gpiod_direction_output(jdi->enable_gpio, 0); >> + } >> + >> + jdi->pwm_gpio = 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 = 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 = of_parse_phandle(dev->of_node, "backlight", 0); >> + if (np) { >> + jdi->backlight = 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 <sumit.semwal@xxxxxxxxxx>"); >> +MODULE_AUTHOR("Vinay Simha BN <simhavcs@xxxxxxxxx>"); >> +MODULE_DESCRIPTION("JDI WUXGA LT070ME05000 DSI video/command mode panel driver"); > > Technically this doesn't support command mode yet, so you might want to > remove that from the description to avoid confusion. > > Thierry -- Regards, Vinay Simha.B.N. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
