From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932703AbeE2JX1 convert rfc822-to-8bit (ORCPT ); Tue, 29 May 2018 05:23:27 -0400 Received: from mail.bootlin.com ([62.4.15.54]:49409 "EHLO mail.bootlin.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932434AbeE2JXT (ORCPT ); Tue, 29 May 2018 05:23:19 -0400 Date: Tue, 29 May 2018 11:23:16 +0200 From: Maxime Ripard To: Linus Walleij Cc: Thierry Reding , Chen-Yu Tsai , "open list:DRM PANEL DRIVERS" , Gustavo Padovan , Daniel Vetter , Maarten Lankhorst , Sean Paul , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v4 2/3] drm/panel: Add Ilitek ILI9881c panel driver Message-ID: <20180529092316.twbwhjqutvnzzfse@flea> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: 8BIT In-Reply-To: User-Agent: NeoMutt/20180323 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Linus, On Fri, May 11, 2018 at 05:54:27PM +0200, Linus Walleij wrote: > Hi Maxime, > > sorry that noone had much time to look at the driver. > But I can help out, hopefully. Thanks for your feedback :) > On Fri, May 4, 2018 at 5:23 PM, Maxime Ripard wrote: > > > The LHR050H41 panel is the panel shipped with the BananaPi M2-Magic, and is > > based on the Ilitek ILI9881c Controller. Add a driver for it, modelled > > after the other Ilitek controller drivers. > > > > Signed-off-by: Maxime Ripard > > Nice, I have some experience with those. > > > +config DRM_PANEL_ILITEK_ILI9881C > > + tristate "Ilitek ILI9881C-based panels" > > + depends on OF > > + depends on DRM_MIPI_DSI > > + depends on BACKLIGHT_CLASS_DEVICE > > If it absolutely needs DRM_MIPI_DSI and > BACKLIGHT_CLASS_DEVICE it maybe you should > be more helpful to the user to just select it? > > Depending on OF is fine, that is more of a platform > property. All the other panels in this file seems to be using a depends on for these two, so I'd rather remain consistent on this. > > +struct ili9881c { > > + struct drm_panel panel; > > + struct mipi_dsi_device *dsi; > > + > > + struct backlight_device *backlight; > > + struct gpio_desc *power; > > Should this not be modeled using a fixed regulator instead? Probably, yes. On my board it's a GPIO that goes through the connector, but it seems like the controller itself takes a regulator, so there's probably a GPIO-controlled regulator on the display PCB somewhere. I'll change it. > > +static const struct ili9881c_instr ili9881c_init[] = { > > + ILI9881C_SWITCH_PAGE_INSTR(3), > > + ILI9881C_COMMAND_INSTR(0x01, 0x00), > > + ILI9881C_COMMAND_INSTR(0x02, 0x00), > > + ILI9881C_COMMAND_INSTR(0x03, 0x73), > > + ILI9881C_COMMAND_INSTR(0x04, 0x03), > > + ILI9881C_COMMAND_INSTR(0x05, 0x00), > > + ILI9881C_COMMAND_INSTR(0x06, 0x06), > > + ILI9881C_COMMAND_INSTR(0x07, 0x06), > (...) > > This is a bit hard to understand to say the least. I know :/ > If this comes from a datasheet some more elaborate construction of > the commands would be nice, maybe using some #defines? > > If this just comes from some code drop or reverse engineering, > OK bad luck, I can live with it :) This comes from a code drop (or rather, an obscure initialisation sequence coming straight from the vendor without any documentation or comment associated to it). > It looks a bit like you're uploading a small firmware. > > > +static int ili9881c_switch_page(struct ili9881c *ctx, u8 page) > > +{ > > + u8 buf[4] = { 0xff, 0x98, 0x81, page }; > > That is also a bit unclear wrt what is going on. My understanding is that the controller maps some registers (that might be accessible through other buses if the controller is setup to use something other than DCS) to DCS commands. Since there's more commands than DCS would allow, it's setup in several pages, with each pages having its own set of commands, and the page 0 accepting the usual standard DCS commands. It really doesn't look to me like a firmware, but just a poorly documented initialisation sequence.. I'll add a comment > > +static const struct drm_display_mode default_mode = { > > + .clock = 62000, > > + .vrefresh = 60, > > + > > + .hdisplay = 720, > > + .hsync_start = 720 + 10, > > + .hsync_end = 720 + 10 + 20, > > + .htotal = 720 + 10 + 20 + 30, > > + > > + .vdisplay = 1280, > > + .vsync_start = 1280 + 10, > > + .vsync_end = 1280 + 10 + 10, > > + .vtotal = 1280 + 10 + 10 + 20, > > +}; > > Is that the default mode for this Ilitek device or just for the > BananaPi? If it is the latter it should be named > bananapi_default_mode or something so as not to confuse > the next user of this driver. I'll do it, thanks! Maxime -- Maxime Ripard, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxime Ripard Subject: Re: [PATCH v4 2/3] drm/panel: Add Ilitek ILI9881c panel driver Date: Tue, 29 May 2018 11:23:16 +0200 Message-ID: <20180529092316.twbwhjqutvnzzfse@flea> References: Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mail.bootlin.com (mail.bootlin.com [62.4.15.54]) by gabe.freedesktop.org (Postfix) with ESMTP id 0CDA86E38A for ; Tue, 29 May 2018 09:23:18 +0000 (UTC) Content-Disposition: inline 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: Linus Walleij Cc: "linux-kernel@vger.kernel.org" , "open list:DRM PANEL DRIVERS" , Chen-Yu Tsai , Thierry Reding , Daniel Vetter List-Id: dri-devel@lists.freedesktop.org SGkgTGludXMsCgpPbiBGcmksIE1heSAxMSwgMjAxOCBhdCAwNTo1NDoyN1BNICswMjAwLCBMaW51 cyBXYWxsZWlqIHdyb3RlOgo+IEhpIE1heGltZSwKPiAKPiBzb3JyeSB0aGF0IG5vb25lIGhhZCBt dWNoIHRpbWUgdG8gbG9vayBhdCB0aGUgZHJpdmVyLgo+IEJ1dCBJIGNhbiBoZWxwIG91dCwgaG9w ZWZ1bGx5LgoKVGhhbmtzIGZvciB5b3VyIGZlZWRiYWNrIDopCgo+IE9uIEZyaSwgTWF5IDQsIDIw MTggYXQgNToyMyBQTSwgTWF4aW1lIFJpcGFyZCA8bWF4aW1lLnJpcGFyZEBib290bGluLmNvbT4g d3JvdGU6Cj4gCj4gPiBUaGUgTEhSMDUwSDQxIHBhbmVsIGlzIHRoZSBwYW5lbCBzaGlwcGVkIHdp dGggdGhlIEJhbmFuYVBpIE0yLU1hZ2ljLCBhbmQgaXMKPiA+IGJhc2VkIG9uIHRoZSBJbGl0ZWsg SUxJOTg4MWMgQ29udHJvbGxlci4gQWRkIGEgZHJpdmVyIGZvciBpdCwgbW9kZWxsZWQKPiA+IGFm dGVyIHRoZSBvdGhlciBJbGl0ZWsgY29udHJvbGxlciBkcml2ZXJzLgo+ID4KPiA+IFNpZ25lZC1v ZmYtYnk6IE1heGltZSBSaXBhcmQgPG1heGltZS5yaXBhcmRAYm9vdGxpbi5jb20+Cj4gCj4gTmlj ZSwgSSBoYXZlIHNvbWUgZXhwZXJpZW5jZSB3aXRoIHRob3NlLgo+IAo+ID4gK2NvbmZpZyBEUk1f UEFORUxfSUxJVEVLX0lMSTk4ODFDCj4gPiArICAgICAgIHRyaXN0YXRlICJJbGl0ZWsgSUxJOTg4 MUMtYmFzZWQgcGFuZWxzIgo+ID4gKyAgICAgICBkZXBlbmRzIG9uIE9GCj4gPiArICAgICAgIGRl cGVuZHMgb24gRFJNX01JUElfRFNJCj4gPiArICAgICAgIGRlcGVuZHMgb24gQkFDS0xJR0hUX0NM QVNTX0RFVklDRQo+IAo+IElmIGl0IGFic29sdXRlbHkgbmVlZHMgRFJNX01JUElfRFNJIGFuZAo+ IEJBQ0tMSUdIVF9DTEFTU19ERVZJQ0UgaXQgbWF5YmUgeW91IHNob3VsZAo+IGJlIG1vcmUgaGVs cGZ1bCB0byB0aGUgdXNlciB0byBqdXN0IHNlbGVjdCBpdD8KPiAKPiBEZXBlbmRpbmcgb24gT0Yg aXMgZmluZSwgdGhhdCBpcyBtb3JlIG9mIGEgcGxhdGZvcm0KPiBwcm9wZXJ0eS4KCkFsbCB0aGUg b3RoZXIgcGFuZWxzIGluIHRoaXMgZmlsZSBzZWVtcyB0byBiZSB1c2luZyBhIGRlcGVuZHMgb24g Zm9yCnRoZXNlIHR3bywgc28gSSdkIHJhdGhlciByZW1haW4gY29uc2lzdGVudCBvbiB0aGlzLgoK PiA+ICtzdHJ1Y3QgaWxpOTg4MWMgewo+ID4gKyAgICAgICBzdHJ1Y3QgZHJtX3BhbmVsICAgICAg ICBwYW5lbDsKPiA+ICsgICAgICAgc3RydWN0IG1pcGlfZHNpX2RldmljZSAgKmRzaTsKPiA+ICsK PiA+ICsgICAgICAgc3RydWN0IGJhY2tsaWdodF9kZXZpY2UgKmJhY2tsaWdodDsKPiA+ICsgICAg ICAgc3RydWN0IGdwaW9fZGVzYyAgICAgICAgKnBvd2VyOwo+IAo+IFNob3VsZCB0aGlzIG5vdCBi ZSBtb2RlbGVkIHVzaW5nIGEgZml4ZWQgcmVndWxhdG9yIGluc3RlYWQ/CgpQcm9iYWJseSwgeWVz LiBPbiBteSBib2FyZCBpdCdzIGEgR1BJTyB0aGF0IGdvZXMgdGhyb3VnaCB0aGUKY29ubmVjdG9y LCBidXQgaXQgc2VlbXMgbGlrZSB0aGUgY29udHJvbGxlciBpdHNlbGYgdGFrZXMgYSByZWd1bGF0 b3IsCnNvIHRoZXJlJ3MgcHJvYmFibHkgYSBHUElPLWNvbnRyb2xsZWQgcmVndWxhdG9yIG9uIHRo ZSBkaXNwbGF5IFBDQgpzb21ld2hlcmUuIEknbGwgY2hhbmdlIGl0LgoKPiA+ICtzdGF0aWMgY29u c3Qgc3RydWN0IGlsaTk4ODFjX2luc3RyIGlsaTk4ODFjX2luaXRbXSA9IHsKPiA+ICsgICAgICAg SUxJOTg4MUNfU1dJVENIX1BBR0VfSU5TVFIoMyksCj4gPiArICAgICAgIElMSTk4ODFDX0NPTU1B TkRfSU5TVFIoMHgwMSwgMHgwMCksCj4gPiArICAgICAgIElMSTk4ODFDX0NPTU1BTkRfSU5TVFIo MHgwMiwgMHgwMCksCj4gPiArICAgICAgIElMSTk4ODFDX0NPTU1BTkRfSU5TVFIoMHgwMywgMHg3 MyksCj4gPiArICAgICAgIElMSTk4ODFDX0NPTU1BTkRfSU5TVFIoMHgwNCwgMHgwMyksCj4gPiAr ICAgICAgIElMSTk4ODFDX0NPTU1BTkRfSU5TVFIoMHgwNSwgMHgwMCksCj4gPiArICAgICAgIElM STk4ODFDX0NPTU1BTkRfSU5TVFIoMHgwNiwgMHgwNiksCj4gPiArICAgICAgIElMSTk4ODFDX0NP TU1BTkRfSU5TVFIoMHgwNywgMHgwNiksCj4gKC4uLikKPiAKPiBUaGlzIGlzIGEgYml0IGhhcmQg dG8gdW5kZXJzdGFuZCB0byBzYXkgdGhlIGxlYXN0LgoKSSBrbm93IDovCgo+IElmIHRoaXMgY29t ZXMgZnJvbSBhIGRhdGFzaGVldCBzb21lIG1vcmUgZWxhYm9yYXRlIGNvbnN0cnVjdGlvbiBvZgo+ IHRoZSBjb21tYW5kcyB3b3VsZCBiZSBuaWNlLCBtYXliZSB1c2luZyBzb21lICNkZWZpbmVzPwo+ IAo+IElmIHRoaXMganVzdCBjb21lcyBmcm9tIHNvbWUgY29kZSBkcm9wIG9yIHJldmVyc2UgZW5n aW5lZXJpbmcsCj4gT0sgYmFkIGx1Y2ssIEkgY2FuIGxpdmUgd2l0aCBpdCA6KQoKVGhpcyBjb21l cyBmcm9tIGEgY29kZSBkcm9wIChvciByYXRoZXIsIGFuIG9ic2N1cmUgaW5pdGlhbGlzYXRpb24K c2VxdWVuY2UgY29taW5nIHN0cmFpZ2h0IGZyb20gdGhlIHZlbmRvciB3aXRob3V0IGFueSBkb2N1 bWVudGF0aW9uIG9yCmNvbW1lbnQgYXNzb2NpYXRlZCB0byBpdCkuCgo+IEl0IGxvb2tzIGEgYml0 IGxpa2UgeW91J3JlIHVwbG9hZGluZyBhIHNtYWxsIGZpcm13YXJlLgo+IAo+ID4gK3N0YXRpYyBp bnQgaWxpOTg4MWNfc3dpdGNoX3BhZ2Uoc3RydWN0IGlsaTk4ODFjICpjdHgsIHU4IHBhZ2UpCj4g PiArewo+ID4gKyAgICAgICB1OCBidWZbNF0gPSB7IDB4ZmYsIDB4OTgsIDB4ODEsIHBhZ2UgfTsK PiAKPiBUaGF0IGlzIGFsc28gYSBiaXQgdW5jbGVhciB3cnQgd2hhdCBpcyBnb2luZyBvbi4KCk15 IHVuZGVyc3RhbmRpbmcgaXMgdGhhdCB0aGUgY29udHJvbGxlciBtYXBzIHNvbWUgcmVnaXN0ZXJz ICh0aGF0Cm1pZ2h0IGJlIGFjY2Vzc2libGUgdGhyb3VnaCBvdGhlciBidXNlcyBpZiB0aGUgY29u dHJvbGxlciBpcyBzZXR1cCB0bwp1c2Ugc29tZXRoaW5nIG90aGVyIHRoYW4gRENTKSB0byBEQ1Mg Y29tbWFuZHMuIFNpbmNlIHRoZXJlJ3MgbW9yZQpjb21tYW5kcyB0aGFuIERDUyB3b3VsZCBhbGxv dywgaXQncyBzZXR1cCBpbiBzZXZlcmFsIHBhZ2VzLCB3aXRoIGVhY2gKcGFnZXMgaGF2aW5nIGl0 cyBvd24gc2V0IG9mIGNvbW1hbmRzLCBhbmQgdGhlIHBhZ2UgMCBhY2NlcHRpbmcgdGhlCnVzdWFs IHN0YW5kYXJkIERDUyBjb21tYW5kcy4KCkl0IHJlYWxseSBkb2Vzbid0IGxvb2sgdG8gbWUgbGlr ZSBhIGZpcm13YXJlLCBidXQganVzdCBhIHBvb3JseQpkb2N1bWVudGVkIGluaXRpYWxpc2F0aW9u IHNlcXVlbmNlLi4gSSdsbCBhZGQgYSBjb21tZW50Cgo+ID4gK3N0YXRpYyBjb25zdCBzdHJ1Y3Qg ZHJtX2Rpc3BsYXlfbW9kZSBkZWZhdWx0X21vZGUgPSB7Cj4gPiArICAgICAgIC5jbG9jayAgICAg ICAgICA9IDYyMDAwLAo+ID4gKyAgICAgICAudnJlZnJlc2ggICAgICAgPSA2MCwKPiA+ICsKPiA+ ICsgICAgICAgLmhkaXNwbGF5ICAgICAgID0gNzIwLAo+ID4gKyAgICAgICAuaHN5bmNfc3RhcnQg ICAgPSA3MjAgKyAxMCwKPiA+ICsgICAgICAgLmhzeW5jX2VuZCAgICAgID0gNzIwICsgMTAgKyAy MCwKPiA+ICsgICAgICAgLmh0b3RhbCAgICAgICAgID0gNzIwICsgMTAgKyAyMCArIDMwLAo+ID4g Kwo+ID4gKyAgICAgICAudmRpc3BsYXkgICAgICAgPSAxMjgwLAo+ID4gKyAgICAgICAudnN5bmNf c3RhcnQgICAgPSAxMjgwICsgMTAsCj4gPiArICAgICAgIC52c3luY19lbmQgICAgICA9IDEyODAg KyAxMCArIDEwLAo+ID4gKyAgICAgICAudnRvdGFsICAgICAgICAgPSAxMjgwICsgMTAgKyAxMCAr IDIwLAo+ID4gK307Cj4gCj4gSXMgdGhhdCB0aGUgZGVmYXVsdCBtb2RlIGZvciB0aGlzIElsaXRl ayBkZXZpY2Ugb3IganVzdCBmb3IgdGhlCj4gQmFuYW5hUGk/IElmIGl0IGlzIHRoZSBsYXR0ZXIg aXQgc2hvdWxkIGJlIG5hbWVkCj4gYmFuYW5hcGlfZGVmYXVsdF9tb2RlIG9yIHNvbWV0aGluZyBz byBhcyBub3QgdG8gY29uZnVzZQo+IHRoZSBuZXh0IHVzZXIgb2YgdGhpcyBkcml2ZXIuCgpJJ2xs IGRvIGl0LCB0aGFua3MhCk1heGltZQoKLS0gCk1heGltZSBSaXBhcmQsIEJvb3RsaW4gKGZvcm1l cmx5IEZyZWUgRWxlY3Ryb25zKQpFbWJlZGRlZCBMaW51eCBhbmQgS2VybmVsIGVuZ2luZWVyaW5n Cmh0dHBzOi8vYm9vdGxpbi5jb20KX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRl c2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8v ZHJpLWRldmVsCg==