From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751865AbeEKPyc (ORCPT ); Fri, 11 May 2018 11:54:32 -0400 Received: from mail-io0-f196.google.com ([209.85.223.196]:37767 "EHLO mail-io0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751634AbeEKPy3 (ORCPT ); Fri, 11 May 2018 11:54:29 -0400 X-Google-Smtp-Source: AB8JxZq5jKCYbH4qkT+Nk52TIxQrLPZF8DekkVkfd7n4/rp8koAOuLYgAYSEmRxfuesDiGYmp2v/IgqUg3xz5MwH91U= MIME-Version: 1.0 In-Reply-To: References: From: Linus Walleij Date: Fri, 11 May 2018 17:54:27 +0200 Message-ID: Subject: Re: [PATCH v4 2/3] drm/panel: Add Ilitek ILI9881c panel driver To: Maxime Ripard Cc: Thierry Reding , Chen-Yu Tsai , "open list:DRM PANEL DRIVERS" , Gustavo Padovan , Daniel Vetter , Maarten Lankhorst , Sean Paul , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Maxime, sorry that noone had much time to look at the driver. But I can help out, hopefully. 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. > +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? > + struct gpio_desc *reset; > +}; > +enum ili9881c_op { > + ILI9881C_SWITCH_PAGE, > + ILI9881C_COMMAND, > +}; > + > +struct ili9881c_instr { > + enum ili9881c_op op; > + > + union arg { > + struct cmd { > + u8 cmd; > + u8 data; > + } cmd; > + u8 page; > + } arg; > +}; > + > +#define ILI9881C_SWITCH_PAGE_INSTR(_page) \ > + { \ > + .op = ILI9881C_SWITCH_PAGE, \ > + .arg = { \ > + .page = (_page), \ > + }, \ > + } > + > +#define ILI9881C_COMMAND_INSTR(_cmd, _data) \ > + { \ > + .op = ILI9881C_COMMAND, \ > + .arg = { \ > + .cmd = { \ > + .cmd = (_cmd), \ > + .data = (_data), \ > + }, \ > + }, \ > + } That looks clever. > +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. 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 :) 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. > +static int ili9881c_prepare(struct drm_panel *panel) > +{ > + struct ili9881c *ctx = panel_to_ili9881c(panel); > + unsigned int i; > + int ret; > + > + /* Power the panel */ > + gpiod_set_value(ctx->power, 1); > + msleep(5); So isn't this a fixed regulator using a GPIO? > +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. > + ctx->power = devm_gpiod_get(&dsi->dev, "power", GPIOD_OUT_LOW); > + if (IS_ERR(ctx->power)) { > + dev_err(&dsi->dev, "Couldn't get our power GPIO\n"); > + return PTR_ERR(ctx->power); > + } So I'm a bit sceptical about this one. Yours, Linus Walleij From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linus Walleij Subject: Re: [PATCH v4 2/3] drm/panel: Add Ilitek ILI9881c panel driver Date: Fri, 11 May 2018 17:54:27 +0200 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mail-io0-x242.google.com (mail-io0-x242.google.com [IPv6:2607:f8b0:4001:c06::242]) by gabe.freedesktop.org (Postfix) with ESMTPS id 88DD36F1E0 for ; Fri, 11 May 2018 15:54:29 +0000 (UTC) Received: by mail-io0-x242.google.com with SMTP id f21-v6so7520364iob.13 for ; Fri, 11 May 2018 08:54:29 -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: Maxime Ripard 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 SGkgTWF4aW1lLAoKc29ycnkgdGhhdCBub29uZSBoYWQgbXVjaCB0aW1lIHRvIGxvb2sgYXQgdGhl IGRyaXZlci4KQnV0IEkgY2FuIGhlbHAgb3V0LCBob3BlZnVsbHkuCgpPbiBGcmksIE1heSA0LCAy MDE4IGF0IDU6MjMgUE0sIE1heGltZSBSaXBhcmQgPG1heGltZS5yaXBhcmRAYm9vdGxpbi5jb20+ IHdyb3RlOgoKPiBUaGUgTEhSMDUwSDQxIHBhbmVsIGlzIHRoZSBwYW5lbCBzaGlwcGVkIHdpdGgg dGhlIEJhbmFuYVBpIE0yLU1hZ2ljLCBhbmQgaXMKPiBiYXNlZCBvbiB0aGUgSWxpdGVrIElMSTk4 ODFjIENvbnRyb2xsZXIuIEFkZCBhIGRyaXZlciBmb3IgaXQsIG1vZGVsbGVkCj4gYWZ0ZXIgdGhl IG90aGVyIElsaXRlayBjb250cm9sbGVyIGRyaXZlcnMuCj4KPiBTaWduZWQtb2ZmLWJ5OiBNYXhp bWUgUmlwYXJkIDxtYXhpbWUucmlwYXJkQGJvb3RsaW4uY29tPgoKTmljZSwgSSBoYXZlIHNvbWUg ZXhwZXJpZW5jZSB3aXRoIHRob3NlLgoKPiArY29uZmlnIERSTV9QQU5FTF9JTElURUtfSUxJOTg4 MUMKPiArICAgICAgIHRyaXN0YXRlICJJbGl0ZWsgSUxJOTg4MUMtYmFzZWQgcGFuZWxzIgo+ICsg ICAgICAgZGVwZW5kcyBvbiBPRgo+ICsgICAgICAgZGVwZW5kcyBvbiBEUk1fTUlQSV9EU0kKPiAr ICAgICAgIGRlcGVuZHMgb24gQkFDS0xJR0hUX0NMQVNTX0RFVklDRQoKSWYgaXQgYWJzb2x1dGVs eSBuZWVkcyBEUk1fTUlQSV9EU0kgYW5kCkJBQ0tMSUdIVF9DTEFTU19ERVZJQ0UgaXQgbWF5YmUg eW91IHNob3VsZApiZSBtb3JlIGhlbHBmdWwgdG8gdGhlIHVzZXIgdG8ganVzdCBzZWxlY3QgaXQ/ CgpEZXBlbmRpbmcgb24gT0YgaXMgZmluZSwgdGhhdCBpcyBtb3JlIG9mIGEgcGxhdGZvcm0KcHJv cGVydHkuCgo+ICtzdHJ1Y3QgaWxpOTg4MWMgewo+ICsgICAgICAgc3RydWN0IGRybV9wYW5lbCAg ICAgICAgcGFuZWw7Cj4gKyAgICAgICBzdHJ1Y3QgbWlwaV9kc2lfZGV2aWNlICAqZHNpOwo+ICsK PiArICAgICAgIHN0cnVjdCBiYWNrbGlnaHRfZGV2aWNlICpiYWNrbGlnaHQ7Cj4gKyAgICAgICBz dHJ1Y3QgZ3Bpb19kZXNjICAgICAgICAqcG93ZXI7CgpTaG91bGQgdGhpcyBub3QgYmUgbW9kZWxl ZCB1c2luZyBhIGZpeGVkIHJlZ3VsYXRvciBpbnN0ZWFkPwoKPiArICAgICAgIHN0cnVjdCBncGlv X2Rlc2MgICAgICAgICpyZXNldDsKPiArfTsKCj4gK2VudW0gaWxpOTg4MWNfb3Agewo+ICsgICAg ICAgSUxJOTg4MUNfU1dJVENIX1BBR0UsCj4gKyAgICAgICBJTEk5ODgxQ19DT01NQU5ELAo+ICt9 Owo+ICsKPiArc3RydWN0IGlsaTk4ODFjX2luc3RyIHsKPiArICAgICAgIGVudW0gaWxpOTg4MWNf b3AgICAgICAgIG9wOwo+ICsKPiArICAgICAgIHVuaW9uIGFyZyB7Cj4gKyAgICAgICAgICAgICAg IHN0cnVjdCBjbWQgewo+ICsgICAgICAgICAgICAgICAgICAgICAgIHU4ICAgICAgY21kOwo+ICsg ICAgICAgICAgICAgICAgICAgICAgIHU4ICAgICAgZGF0YTsKPiArICAgICAgICAgICAgICAgfSBj bWQ7Cj4gKyAgICAgICAgICAgICAgIHU4ICAgICAgcGFnZTsKPiArICAgICAgIH0gYXJnOwo+ICt9 Owo+ICsKPiArI2RlZmluZSBJTEk5ODgxQ19TV0lUQ0hfUEFHRV9JTlNUUihfcGFnZSkgICAgICBc Cj4gKyAgICAgICB7ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgXAo+ICsg ICAgICAgICAgICAgICAub3AgPSBJTEk5ODgxQ19TV0lUQ0hfUEFHRSwgICAgIFwKPiArICAgICAg ICAgICAgICAgLmFyZyA9IHsgICAgICAgICAgICAgICAgICAgICAgICBcCj4gKyAgICAgICAgICAg ICAgICAgICAgICAgLnBhZ2UgPSAoX3BhZ2UpLCAgICAgICAgXAo+ICsgICAgICAgICAgICAgICB9 LCAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIFwKPiArICAgICAgIH0KPiArCj4gKyNkZWZp bmUgSUxJOTg4MUNfQ09NTUFORF9JTlNUUihfY21kLCBfZGF0YSkgICAgICAgICAgICBcCj4gKyAg ICAgICB7ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBcCj4g KyAgICAgICAgICAgICAgIC5vcCA9IElMSTk4ODFDX0NPTU1BTkQsICAgICAgICAgXAo+ICsgICAg ICAgICAgICAgICAuYXJnID0geyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgXAo+ICsg ICAgICAgICAgICAgICAgICAgICAgIC5jbWQgPSB7ICAgICAgICAgICAgICAgICAgICAgICAgXAo+ ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgLmNtZCA9IChfY21kKSwgICAgICAgICAg XAo+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgLmRhdGEgPSAoX2RhdGEpLCAgICAg ICAgXAo+ICsgICAgICAgICAgICAgICAgICAgICAgIH0sICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgXAo+ICsgICAgICAgICAgICAgICB9LCAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgXAo+ICsgICAgICAgfQoKVGhhdCBsb29rcyBjbGV2ZXIuCgo+ICtzdGF0aWMgY29u c3Qgc3RydWN0IGlsaTk4ODFjX2luc3RyIGlsaTk4ODFjX2luaXRbXSA9IHsKPiArICAgICAgIElM STk4ODFDX1NXSVRDSF9QQUdFX0lOU1RSKDMpLAo+ICsgICAgICAgSUxJOTg4MUNfQ09NTUFORF9J TlNUUigweDAxLCAweDAwKSwKPiArICAgICAgIElMSTk4ODFDX0NPTU1BTkRfSU5TVFIoMHgwMiwg MHgwMCksCj4gKyAgICAgICBJTEk5ODgxQ19DT01NQU5EX0lOU1RSKDB4MDMsIDB4NzMpLAo+ICsg ICAgICAgSUxJOTg4MUNfQ09NTUFORF9JTlNUUigweDA0LCAweDAzKSwKPiArICAgICAgIElMSTk4 ODFDX0NPTU1BTkRfSU5TVFIoMHgwNSwgMHgwMCksCj4gKyAgICAgICBJTEk5ODgxQ19DT01NQU5E X0lOU1RSKDB4MDYsIDB4MDYpLAo+ICsgICAgICAgSUxJOTg4MUNfQ09NTUFORF9JTlNUUigweDA3 LCAweDA2KSwKKC4uLikKClRoaXMgaXMgYSBiaXQgaGFyZCB0byB1bmRlcnN0YW5kIHRvIHNheSB0 aGUgbGVhc3QuIElmIHRoaXMgY29tZXMgZnJvbQphIGRhdGFzaGVldCBzb21lIG1vcmUgZWxhYm9y YXRlIGNvbnN0cnVjdGlvbiBvZiB0aGUgY29tbWFuZHMKd291bGQgYmUgbmljZSwgbWF5YmUgdXNp bmcgc29tZSAjZGVmaW5lcz8KCklmIHRoaXMganVzdCBjb21lcyBmcm9tIHNvbWUgY29kZSBkcm9w IG9yIHJldmVyc2UgZW5naW5lZXJpbmcsCk9LIGJhZCBsdWNrLCBJIGNhbiBsaXZlIHdpdGggaXQg OikKCkl0IGxvb2tzIGEgYml0IGxpa2UgeW91J3JlIHVwbG9hZGluZyBhIHNtYWxsIGZpcm13YXJl LgoKPiArc3RhdGljIGludCBpbGk5ODgxY19zd2l0Y2hfcGFnZShzdHJ1Y3QgaWxpOTg4MWMgKmN0 eCwgdTggcGFnZSkKPiArewo+ICsgICAgICAgdTggYnVmWzRdID0geyAweGZmLCAweDk4LCAweDgx LCBwYWdlIH07CgpUaGF0IGlzIGFsc28gYSBiaXQgdW5jbGVhciB3cnQgd2hhdCBpcyBnb2luZyBv bi4KCj4gK3N0YXRpYyBpbnQgaWxpOTg4MWNfcHJlcGFyZShzdHJ1Y3QgZHJtX3BhbmVsICpwYW5l bCkKPiArewo+ICsgICAgICAgc3RydWN0IGlsaTk4ODFjICpjdHggPSBwYW5lbF90b19pbGk5ODgx YyhwYW5lbCk7Cj4gKyAgICAgICB1bnNpZ25lZCBpbnQgaTsKPiArICAgICAgIGludCByZXQ7Cj4g Kwo+ICsgICAgICAgLyogUG93ZXIgdGhlIHBhbmVsICovCj4gKyAgICAgICBncGlvZF9zZXRfdmFs dWUoY3R4LT5wb3dlciwgMSk7Cj4gKyAgICAgICBtc2xlZXAoNSk7CgpTbyBpc24ndCB0aGlzIGEg Zml4ZWQgcmVndWxhdG9yIHVzaW5nIGEgR1BJTz8KCj4gK3N0YXRpYyBjb25zdCBzdHJ1Y3QgZHJt X2Rpc3BsYXlfbW9kZSBkZWZhdWx0X21vZGUgPSB7Cj4gKyAgICAgICAuY2xvY2sgICAgICAgICAg PSA2MjAwMCwKPiArICAgICAgIC52cmVmcmVzaCAgICAgICA9IDYwLAo+ICsKPiArICAgICAgIC5o ZGlzcGxheSAgICAgICA9IDcyMCwKPiArICAgICAgIC5oc3luY19zdGFydCAgICA9IDcyMCArIDEw LAo+ICsgICAgICAgLmhzeW5jX2VuZCAgICAgID0gNzIwICsgMTAgKyAyMCwKPiArICAgICAgIC5o dG90YWwgICAgICAgICA9IDcyMCArIDEwICsgMjAgKyAzMCwKPiArCj4gKyAgICAgICAudmRpc3Bs YXkgICAgICAgPSAxMjgwLAo+ICsgICAgICAgLnZzeW5jX3N0YXJ0ICAgID0gMTI4MCArIDEwLAo+ ICsgICAgICAgLnZzeW5jX2VuZCAgICAgID0gMTI4MCArIDEwICsgMTAsCj4gKyAgICAgICAudnRv dGFsICAgICAgICAgPSAxMjgwICsgMTAgKyAxMCArIDIwLAo+ICt9OwoKSXMgdGhhdCB0aGUgZGVm YXVsdCBtb2RlIGZvciB0aGlzIElsaXRlayBkZXZpY2Ugb3IganVzdCBmb3IgdGhlCkJhbmFuYVBp PyBJZiBpdCBpcyB0aGUgbGF0dGVyIGl0IHNob3VsZCBiZSBuYW1lZApiYW5hbmFwaV9kZWZhdWx0 X21vZGUgb3Igc29tZXRoaW5nIHNvIGFzIG5vdCB0byBjb25mdXNlCnRoZSBuZXh0IHVzZXIgb2Yg dGhpcyBkcml2ZXIuCgo+ICsgICAgICAgY3R4LT5wb3dlciA9IGRldm1fZ3Bpb2RfZ2V0KCZkc2kt PmRldiwgInBvd2VyIiwgR1BJT0RfT1VUX0xPVyk7Cj4gKyAgICAgICBpZiAoSVNfRVJSKGN0eC0+ cG93ZXIpKSB7Cj4gKyAgICAgICAgICAgICAgIGRldl9lcnIoJmRzaS0+ZGV2LCAiQ291bGRuJ3Qg Z2V0IG91ciBwb3dlciBHUElPXG4iKTsKPiArICAgICAgICAgICAgICAgcmV0dXJuIFBUUl9FUlIo Y3R4LT5wb3dlcik7Cj4gKyAgICAgICB9CgpTbyBJJ20gYSBiaXQgc2NlcHRpY2FsIGFib3V0IHRo aXMgb25lLgoKWW91cnMsCkxpbnVzIFdhbGxlaWoKX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlz dHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4v bGlzdGluZm8vZHJpLWRldmVsCg==