From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH v4 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel Date: Tue, 14 Jun 2016 13:25:27 +0200 Message-ID: <20160614112527.GA30021@ulmo.ba.sec> References: <1465813529-30621-1-git-send-email-simhavcs@gmail.com> <1465813529-30621-2-git-send-email-simhavcs@gmail.com> <20160613123514.GF27930@ulmo.ba.sec> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0224576460==" Return-path: Received: from mail-wm0-x243.google.com (mail-wm0-x243.google.com [IPv6:2a00:1450:400c:c09::243]) by gabe.freedesktop.org (Postfix) with ESMTPS id E43136E720 for ; Tue, 14 Jun 2016 11:25:30 +0000 (UTC) Received: by mail-wm0-x243.google.com with SMTP id n184so21644217wmn.1 for ; Tue, 14 Jun 2016 04:25:30 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Vinay Simha Cc: open list , "open list:DRM PANEL DRIVERS" , Archit Taneja List-Id: dri-devel@lists.freedesktop.org --===============0224576460== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="wRRV7LY7NUeQGEoC" Content-Disposition: inline --wRRV7LY7NUeQGEoC Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Jun 14, 2016 at 04:27:53PM +0530, Vinay Simha wrote: [...] > On Mon, Jun 13, 2016 at 6:05 PM, Thierry Reding > wrote: > > On Mon, Jun 13, 2016 at 03:55:28PM +0530, Vinay Simha BN wrote: [...] > >> +const char *regs[] = { > >> + "vddp", > >> + "dcdc_en", > >> + "vcc" > >> +}; > > > > This should be static. Also use a more sensible name, such as > > regulator_names, please. > it kept as regs, to keep constant names as used in > the dsi_cfg file > drivers/gpu/drm/msm/dsi/dsi_cfg.c That's a completely different driver, no need to be consistent. > >> +static int jdi_panel_init(struct jdi_panel *jdi) > >> +{ > > [...] > >> + struct mipi_dsi_device *dsi = jdi->dsi; > >> + int ret; > >> + > >> + dsi->mode_flags |= MIPI_DSI_MODE_LPM; > >> + > >> + ret = mipi_dsi_dcs_soft_reset(dsi); > >> + if (ret < 0) > >> + return ret; > >> + > >> + usleep_range(10000, 20000); > >> + > >> + ret = mipi_dsi_dcs_set_pixel_format(dsi, 0x70); > >> + if (ret < 0) > >> + return ret; > > > > Please use the existing symbolic constants for this. > i am not clear on the symbolic constants for pixel_format ? See include/video/mipi_display.h > >> + > >> + ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, > >> + (u8[]){ 0x24 }, 1); > >> + if (ret < 0) > >> + return ret; > brightness control setting, to enable pwm/backlight > >> + > >> + ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_POWER_SAVE, > >> + (u8[]){ 0x00 }, 1); > >> + if (ret < 0) > >> + return ret; > > > this is to set cabc off/on Can you point me to the specification of these? I'd like to investigate whether or not we can turn these into more sensible commands. As-is, it is completely obfuscated and I'd like to avoid that where possible. > >> + ret = mipi_dsi_generic_write(dsi, (u8[]){0xB0, 0x00}, 2); > >> + if (ret < 0) > >> + return ret; > >> + mdelay(10); > > > > Same here. This also needs at least a comment, though ideally you'd use > > symbolic names for those magic numbers. > i do not have the datasheet to give more description. > this is for interface setting, either command mode/video mode Okay, please add a comment on what this is supposed to do, then. > >> + > >> + jdi->mode = &default_mode; > >> + > >> + for (i = 0; i < num; i++) > >> + s[i].supply = regs[i]; > >> + > >> + ret = devm_regulator_bulk_get(dev, num, s); > >> + if (ret < 0) { > >> + dev_err(dev, "%s: failed to init regulator, ret=%d\n", > >> + __func__, ret); > >> + return ret; > >> + } > >> + > >> + jdi->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > >> + if (IS_ERR(jdi->reset_gpio)) { > >> + dev_err(dev, "cannot get reset-gpios %ld\n", > >> + PTR_ERR(jdi->reset_gpio)); > > > > This is a third variant of error reporting. Please stick to one. > for PTR_ERR(jdi->reset_gpio) returns unsigned long, so this error reporting > cannot be changed to ret, > others error reporting incorporated consistently. PTR_ERR() returns signed long, not unsigned. You can still use the same format for the message and substitute the %ld printk specifier to match the type. Thierry --wRRV7LY7NUeQGEoC Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJXX+mjAAoJEN0jrNd/PrOhpNEP/iQ0KnobrO8vJpHthJ2V9djL 7QU/SBGtd377j4Vok0ERierQpi2QzkAdl1OHEI2kParaSsfjVYKhCXxZX7JHbAUu OB6u7kDadgoHjEjkNRo7GJXoimR7EXFYiDaUVI01IIHP5owF4LjfzoY5XpR4F6H8 7J3JgZI5cf6K+hMAENfFF9TQ1nBSF7MMhpTZOwA7Kwa14X+sXJ9rB77552eBnRP8 4XcSbBMPi7wU9tU7+rKoRxpdX966dhHcxZT/ZTqrDIZlbUakz5xv827T1r7c28tP PSAfyE1E6Kw8wjH+eqcxVwMv5N/URP5V32RGRh6qSCbAV2gsw42SguVfcwXcpfMD o7CutxU0hGpK+l5Dp7DECRGcOleNvOV4h5LpNcz2CLhB3PpE+35qthmirl5eWVEu frAv1bfWlhHrKfBRBETdcQisTbsmKZUXg+EqrQlGkYyyr83Hs5BnHrckzj+NE14p yMBG2liBlIDrgpUNrTacRz7Rk3aAU2O48+3g2r4bYtQjcI5dCgF6wP6ZZRjzmCJB DZjtPSrkimy/lh6cDSvATVgZKZ7vKhM6pC/xkXnu4KaST/b6FKs9gotFzSPTk2QL qbm3kGr1mXsokziBwSn8JERWW256LUXOtHZX+suV2xWbE4Qqr6dpmPIFqGZrnjvb qia/QQOSZqVCJjZVQxJH =lhG9 -----END PGP SIGNATURE----- --wRRV7LY7NUeQGEoC-- --===============0224576460== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0224576460==-- From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751613AbcFNLZc (ORCPT ); Tue, 14 Jun 2016 07:25:32 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:36399 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750703AbcFNLZa (ORCPT ); Tue, 14 Jun 2016 07:25:30 -0400 Date: Tue, 14 Jun 2016 13:25:27 +0200 From: Thierry Reding To: Vinay Simha Cc: Archit Taneja , Sumit Semwal , John Stultz , Rob Clark , David Airlie , open list , "open list:DRM PANEL DRIVERS" Subject: Re: [PATCH v4 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel Message-ID: <20160614112527.GA30021@ulmo.ba.sec> References: <1465813529-30621-1-git-send-email-simhavcs@gmail.com> <1465813529-30621-2-git-send-email-simhavcs@gmail.com> <20160613123514.GF27930@ulmo.ba.sec> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="wRRV7LY7NUeQGEoC" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.1 (2016-04-27) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --wRRV7LY7NUeQGEoC Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Jun 14, 2016 at 04:27:53PM +0530, Vinay Simha wrote: [...] > On Mon, Jun 13, 2016 at 6:05 PM, Thierry Reding > wrote: > > On Mon, Jun 13, 2016 at 03:55:28PM +0530, Vinay Simha BN wrote: [...] > >> +const char *regs[] = { > >> + "vddp", > >> + "dcdc_en", > >> + "vcc" > >> +}; > > > > This should be static. Also use a more sensible name, such as > > regulator_names, please. > it kept as regs, to keep constant names as used in > the dsi_cfg file > drivers/gpu/drm/msm/dsi/dsi_cfg.c That's a completely different driver, no need to be consistent. > >> +static int jdi_panel_init(struct jdi_panel *jdi) > >> +{ > > [...] > >> + struct mipi_dsi_device *dsi = jdi->dsi; > >> + int ret; > >> + > >> + dsi->mode_flags |= MIPI_DSI_MODE_LPM; > >> + > >> + ret = mipi_dsi_dcs_soft_reset(dsi); > >> + if (ret < 0) > >> + return ret; > >> + > >> + usleep_range(10000, 20000); > >> + > >> + ret = mipi_dsi_dcs_set_pixel_format(dsi, 0x70); > >> + if (ret < 0) > >> + return ret; > > > > Please use the existing symbolic constants for this. > i am not clear on the symbolic constants for pixel_format ? See include/video/mipi_display.h > >> + > >> + ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, > >> + (u8[]){ 0x24 }, 1); > >> + if (ret < 0) > >> + return ret; > brightness control setting, to enable pwm/backlight > >> + > >> + ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_POWER_SAVE, > >> + (u8[]){ 0x00 }, 1); > >> + if (ret < 0) > >> + return ret; > > > this is to set cabc off/on Can you point me to the specification of these? I'd like to investigate whether or not we can turn these into more sensible commands. As-is, it is completely obfuscated and I'd like to avoid that where possible. > >> + ret = mipi_dsi_generic_write(dsi, (u8[]){0xB0, 0x00}, 2); > >> + if (ret < 0) > >> + return ret; > >> + mdelay(10); > > > > Same here. This also needs at least a comment, though ideally you'd use > > symbolic names for those magic numbers. > i do not have the datasheet to give more description. > this is for interface setting, either command mode/video mode Okay, please add a comment on what this is supposed to do, then. > >> + > >> + jdi->mode = &default_mode; > >> + > >> + for (i = 0; i < num; i++) > >> + s[i].supply = regs[i]; > >> + > >> + ret = devm_regulator_bulk_get(dev, num, s); > >> + if (ret < 0) { > >> + dev_err(dev, "%s: failed to init regulator, ret=%d\n", > >> + __func__, ret); > >> + return ret; > >> + } > >> + > >> + jdi->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > >> + if (IS_ERR(jdi->reset_gpio)) { > >> + dev_err(dev, "cannot get reset-gpios %ld\n", > >> + PTR_ERR(jdi->reset_gpio)); > > > > This is a third variant of error reporting. Please stick to one. > for PTR_ERR(jdi->reset_gpio) returns unsigned long, so this error reporting > cannot be changed to ret, > others error reporting incorporated consistently. PTR_ERR() returns signed long, not unsigned. You can still use the same format for the message and substitute the %ld printk specifier to match the type. Thierry --wRRV7LY7NUeQGEoC Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJXX+mjAAoJEN0jrNd/PrOhpNEP/iQ0KnobrO8vJpHthJ2V9djL 7QU/SBGtd377j4Vok0ERierQpi2QzkAdl1OHEI2kParaSsfjVYKhCXxZX7JHbAUu OB6u7kDadgoHjEjkNRo7GJXoimR7EXFYiDaUVI01IIHP5owF4LjfzoY5XpR4F6H8 7J3JgZI5cf6K+hMAENfFF9TQ1nBSF7MMhpTZOwA7Kwa14X+sXJ9rB77552eBnRP8 4XcSbBMPi7wU9tU7+rKoRxpdX966dhHcxZT/ZTqrDIZlbUakz5xv827T1r7c28tP PSAfyE1E6Kw8wjH+eqcxVwMv5N/URP5V32RGRh6qSCbAV2gsw42SguVfcwXcpfMD o7CutxU0hGpK+l5Dp7DECRGcOleNvOV4h5LpNcz2CLhB3PpE+35qthmirl5eWVEu frAv1bfWlhHrKfBRBETdcQisTbsmKZUXg+EqrQlGkYyyr83Hs5BnHrckzj+NE14p yMBG2liBlIDrgpUNrTacRz7Rk3aAU2O48+3g2r4bYtQjcI5dCgF6wP6ZZRjzmCJB DZjtPSrkimy/lh6cDSvATVgZKZ7vKhM6pC/xkXnu4KaST/b6FKs9gotFzSPTk2QL qbm3kGr1mXsokziBwSn8JERWW256LUXOtHZX+suV2xWbE4Qqr6dpmPIFqGZrnjvb qia/QQOSZqVCJjZVQxJH =lhG9 -----END PGP SIGNATURE----- --wRRV7LY7NUeQGEoC--