From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bjorn Andersson Subject: Re: [PATCH] drm/panel: auo novatek 1080p video mode panel Date: Mon, 10 Aug 2015 12:54:20 -0700 Message-ID: <20150810195420.GK6519@usrtlx11787.corpusers.net> References: <1437507362-20162-1-git-send-email-robdclark@gmail.com> <20150807131931.GA27639@ulmo> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Return-path: Received: from seldrel01.sonyericsson.com ([37.139.156.2]:16719 "EHLO seldrel01.sonyericsson.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752498AbbHJTyb (ORCPT ); Mon, 10 Aug 2015 15:54:31 -0400 Content-Disposition: inline In-Reply-To: Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Rob Clark Cc: Thierry Reding , linux-arm-msm , "dri-devel@lists.freedesktop.org" On Fri 07 Aug 09:11 PDT 2015, Rob Clark wrote: > On Fri, Aug 7, 2015 at 9:19 AM, Thierry Reding wrote: > > On Tue, Jul 21, 2015 at 03:36:02PM -0400, Rob Clark wrote: [..] > >> +- compatible: should be "auo,novatek-1080p-vid" > > > > This looks a little generic for a compatible string. Can't we get at the > > specific panel model number that's been used? What if AUO ever produced > > some other Novatek panel with a 1080p resolution? > > Maybe Sony or someone else can chime in? That somewhat generic name > was all I could get from downstream android kernel. I'm sure there is > a better possible name, although I have no means to find that out > myself. > We're working on it. > > Also, what's the -vid suffix for? > > the same panel seems to also work in cmd mode.. so idea was to have > -vid and -cmd compat strings to choose which mode to operate in. > An alternative would be to make it a bool property, to indicate video mode - following how the framework is implemented. [..] > >> diff --git a/drivers/gpu/drm/panel/panel-auo-novatek-1080p.c b/drivers/gpu/drm/panel/panel-auo-novatek-1080p.c > >> +static int auo_panel_init(struct auo_panel *auo) > >> +{ > >> + struct mipi_dsi_device *dsi = auo->dsi; > >> + int ret; > >> + > >> + dsi->mode_flags |= MIPI_DSI_MODE_LPM; > >> + > >> + ret = mipi_dsi_generic_write(dsi, (u8[]){ 0xb0, 0x04 }, 2); > > > > I find this notation hard to read. Have you considered moving this into > > some sort of table that you can loop through? Or perhaps add some > > helpers, say, mipi_dsi_generic_writeb() and mipi_dsi_dcs_writeb() to > > help make this more readable? > > > > Yeah, helper macro thing might be a reasonable idea. The table option > makes it hard to use the helpers for things that are not non-standard, > or when you need delays, etc.. > I agree with you here, we don't want lists of data that the driver has to interpret into writes (of various types) and delays. > > >> + if (ret < 0) > >> + return ret; > >> + Regards, Bjorn