From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752786AbcFNRDI (ORCPT ); Tue, 14 Jun 2016 13:03:08 -0400 Received: from mail-it0-f42.google.com ([209.85.214.42]:35252 "EHLO mail-it0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751371AbcFNRDG (ORCPT ); Tue, 14 Jun 2016 13:03:06 -0400 MIME-Version: 1.0 In-Reply-To: <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> <20160614112527.GA30021@ulmo.ba.sec> From: Vinay Simha Date: Tue, 14 Jun 2016 22:33:04 +0530 Message-ID: Subject: Re: [PATCH v4 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel To: Thierry Reding Cc: Archit Taneja , Sumit Semwal , John Stultz , Rob Clark , David Airlie , open list , "open list:DRM PANEL DRIVERS" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jun 14, 2016 at 4:55 PM, Thierry Reding wrote: > 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. 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, > >> >> + 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 -- Regards, Vinay Simha.B.N.