From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sam Ravnborg Subject: Re: [PATCH V7 2/2] drm/panel: Add Sitronix ST7701 panel driver Date: Sat, 12 Jan 2019 11:14:09 +0100 Message-ID: <20190112101409.GA14273@ravnborg.org> References: <20190111124714.19566-1-jagan@amarulasolutions.com> <20190111124714.19566-2-jagan@amarulasolutions.com> <20190111211931.GA29735@ravnborg.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from asavdk4.altibox.net (asavdk4.altibox.net [109.247.116.15]) by gabe.freedesktop.org (Postfix) with ESMTPS id A46016E2F8 for ; Sat, 12 Jan 2019 10:14:13 +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: Jagan Teki Cc: David Airlie , Sean Paul , linux-kernel , dri-devel , Thierry Reding , Michael Trimarchi , linux-amarula@amarulasolutions.com List-Id: dri-devel@lists.freedesktop.org SGkgSmFnYW4uCgo+IE9uIFNhdCwgSmFuIDEyLCAyMDE5IGF0IDI6NDkgQU0gU2FtIFJhdm5ib3Jn IDxzYW1AcmF2bmJvcmcub3JnPiB3cm90ZToKPiA+Cj4gPiBIaSBKYWdhbi4KPiA+Cj4gPiBHYXZl IHRoaXMgYW5vdGhlciBtb3JlIGRldGFpbGVkIHJlYWQgLSB0cmlnZ2VyZWQgc29tZSBhZGRpdGlv bmFsIGNvbW1lbnRzLgo+ID4gRGVwaXRlIHRoZSBjb21tZW50cyBpdCBsb29rcyBnb29kLCBhbmQg dGhpcyBpcyBhbGwgbW9yZSBvcgo+ID4gbGVzcyBjb3NtZXRpYyBpbXByb3ZlbWVudHMuCj4gCj4g VGhhbmtzIGZvciB0aGUgcmV2aWV3Lgo+IAo+ID4KPiA+ICAgICAgICAgU2FtCj4gPgo+ID4gPiAr c3RydWN0IHN0NzcwMV9wYW5lbF9kZXNjIHsKPiA+ID4gKyAgICAgY29uc3Qgc3RydWN0IGRybV9k aXNwbGF5X21vZGUgKm1vZGU7Cj4gPiA+ICsgICAgIHVuc2lnbmVkIGludCBsYW5lczsKPiA+ID4g KyAgICAgdW5zaWduZWQgbG9uZyBmbGFnczsKPiA+ID4gKyAgICAgZW51bSBtaXBpX2RzaV9waXhl bF9mb3JtYXQgZm9ybWF0Owo+ID4gPiArICAgICBjb25zdCBjaGFyICpjb25zdCAqc3VwcGx5X25h bWVzOwo+ID4gPiArICAgICB1bnNpZ25lZCBpbnQgbnVtX3N1cHBsaWVzOwo+ID4gPiArICAgICB1 bnNpZ25lZCBpbnQgcGFuZWxfc2xlZXBfZGVsYXk7Cj4gPiA+ICt9Owo+ID4gPiArCj4gPiA+ICtz dHJ1Y3Qgc3Q3NzAxIHsKPiA+ID4gKyAgICAgc3RydWN0IGRybV9wYW5lbCBwYW5lbDsKPiA+ID4g KyAgICAgc3RydWN0IG1pcGlfZHNpX2RldmljZSAqZHNpOwo+ID4gPiArICAgICBjb25zdCBzdHJ1 Y3Qgc3Q3NzAxX3BhbmVsX2Rlc2MgKmRlc2M7Cj4gPiA+ICsKPiA+ID4gKyAgICAgc3RydWN0IGJh Y2tsaWdodF9kZXZpY2UgKmJhY2tsaWdodDsKPiA+ID4gKyAgICAgc3RydWN0IHJlZ3VsYXRvcl9i dWxrX2RhdGEgKnN1cHBsaWVzOwo+ID4gPiArICAgICB1bnNpZ25lZCBpbnQgbnVtX3N1cHBsaWVz Owo+ID4gSSBjYW5ub3Qgc2VlIHRoYXQgbnVtX3N1cHBsaWVzIGluIHRoaXMgc3RydWN0IGFyZSB1 c2VkPwo+IAo+IFllcyBpdCBpcyB1c2VkIGluIHRoZSBjb2RlLCBwbGVhc2UgY2hlY2sgaW4gc3Ry dWN0IHN0NzcwMV9wYW5lbF9kZXNjLgpJIGhhdmUgYXBwbGllZCB0aGUgcGF0Y2ggYW5kIGRlbGV0 ZWQgbnVtX3N1cHBsaWVzIC0gbm93IGJ1aWxkIGVycm9ycy4KU28gbnVtX3N1cHBsaWVzIGluIHN0 cnVjdCBzdDc3MDEgaXMgbm90IHVzZWQgYW5kIHNob3VsZCBiZSBkZWxldGVkLgoKPiA+ID4gK3N0 YXRpYyBpbnQgc3Q3NzAxX3ByZXBhcmUoc3RydWN0IGRybV9wYW5lbCAqcGFuZWwpCj4gPiA+ICt7 Cj4gPiA+ICsgICAgIHN0cnVjdCBzdDc3MDEgKnN0NzcwMSA9IHBhbmVsX3RvX3N0NzcwMShwYW5l bCk7Cj4gPiA+ICsgICAgIGludCByZXQ7Cj4gPiA+ICsKPiA+ID4gKyAgICAgZ3Bpb2Rfc2V0X3Zh bHVlKHN0NzcwMS0+cmVzZXQsIDApOwo+ID4gPiArICAgICBtc2xlZXAoMjApOwo+ID4gPiArCj4g PiA+ICsgICAgIHJldCA9IHJlZ3VsYXRvcl9idWxrX2VuYWJsZShzdDc3MDEtPmRlc2MtPm51bV9z dXBwbGllcywKPiA+ID4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIHN0NzcwMS0+ c3VwcGxpZXMpOwo+ID4gPiArICAgICBpZiAocmV0IDwgMCkKPiA+ID4gKyAgICAgICAgICAgICBy ZXR1cm4gcmV0Owo+ID4gPiArICAgICBtc2xlZXAoMjApOwo+ID4gPiArCj4gPiA+ICsgICAgIGdw aW9kX3NldF92YWx1ZShzdDc3MDEtPnJlc2V0LCAxKTsKPiA+ID4gKyAgICAgbXNsZWVwKDIwKTsK PiA+ID4gKwo+ID4gPiArICAgICBncGlvZF9zZXRfdmFsdWUoc3Q3NzAxLT5yZXNldCwgMCk7Cj4g PiA+ICsgICAgIG1zbGVlcCgzMCk7Cj4gPiA+ICsKPiA+ID4gKyAgICAgZ3Bpb2Rfc2V0X3ZhbHVl KHN0NzcwMS0+cmVzZXQsIDEpOwo+ID4gPiArICAgICBtc2xlZXAoMTUwKTsKPiA+ID4gKwo+ID4g PiArICAgICBzdDc3MDFfaW5pdF9zZXF1ZW5jZShzdDc3MDEpOwo+ID4gPiArCj4gPiA+ICsgICAg IHJldHVybiAwOwo+ID4gPiArfQo+ID4gPiArCj4gPgo+ID4gPiArc3RhdGljIGludCBzdDc3MDFf dW5wcmVwYXJlKHN0cnVjdCBkcm1fcGFuZWwgKnBhbmVsKQo+ID4gPiArewo+ID4gPiArICAgICBz dHJ1Y3Qgc3Q3NzAxICpzdDc3MDEgPSBwYW5lbF90b19zdDc3MDEocGFuZWwpOwo+ID4gPiArCj4g PiA+ICsgICAgIFNUNzcwMV9EU0koc3Q3NzAxLCBNSVBJX0RDU19FWElUX1NMRUVQX01PREUsIDB4 MDApOwoKU2hvdWxkIHRoaXMgYmUgTUlQSV9EQ1NfRU5URVJfU0xFRVBfTU9ERT8KCk5vdGUgLSB5 b3UgaGF2ZSBzaG9ydGN1dHMgZm90IHRoZSBzdGFuZGFyZCBjb21tYW5kcyBsaWtlCmluIHRoaXMg Y2FzZToKCgltaXBpX2RzaV9kY3NfZW50ZXJfc2xlZXBfbW9kZShzdDc3MDEtPmRzaSk7CgpUaGF5 IGNvdWxkIGJlIGludHJvZHVjZWQgaW4gbWFueSBwYWxjZXMsIGJ1dCBJIGFsc28gbGlrZSBob3cg YWxsIHRoZQpjb21tYW5kcyBmb2xsb3dzIGEgY29uc2lzdGVudCB3YXkgdG8gYmUgaXNzdWVkLgpT byBjb25zaWRlciB0aGlzIG9ubHkgaWYgdGhpcyB3YXMgbmV3IGZvciB5b3UuCgoKPiA+ID4gKwo+ ID4gPiArICAgICBtc2xlZXAoc3Q3NzAxLT5zbGVlcF9kZWxheSk7Cj4gPiA+ICsKPiA+ID4gKyAg ICAgZ3Bpb2Rfc2V0X3ZhbHVlKHN0NzcwMS0+cmVzZXQsIDApOwo+ID4gPiArCj4gPiA+ICsgICAg IGdwaW9kX3NldF92YWx1ZShzdDc3MDEtPnJlc2V0LCAxKTsKPiA+ID4gKwo+ID4gPiArICAgICBn cGlvZF9zZXRfdmFsdWUoc3Q3NzAxLT5yZXNldCwgMCk7Cj4gPiBObyB0aW1pbmcgY29uc3RyYWlu cyBoZXJlPyBJbiBwcmVwYXJlIHRoZXJlIGFyZSBzbGVlcHMgaW50ZXJtaXhlZC4KPiAKPiBEZWxh eSB3aGlsZSBkb2luZyB1bnByYXJlIGlzIG5vdCBuZWVkZWQgSSBzdXBwb3NlLgoKSWYgdGhlIHB1 cnBvc2UgaXMgYWxvbmUgdG8gcmVzZXQgdGhlIGRpc3BsYXkgdGhlbiBhIHNpbmdsZSB3cml0ZSAn MCcKc2hvdWxkIGRvIGl0IEkgdGhpbmsKQW5kIHRoZXJlIGlzIGEgcmVxdWlyZW1lbnQgdGhhdCBp dCBtdXN0IGJlIGxvdyBmb3IgYSBtaW5pbXVtIG9mIDEwIHVzCndoaWNoIHdvdWxkIGJlIGdvb2Qg dG8gaGF2ZSBoZXJlLgoKSSBhc2xvIGZvdW5kIGluIGNoYXB0ZXIgOS4gKHBhZ2UgMTYzIC0gc2Vj b25kIGxpbmUpIHRoaXMgc3RhdGVtZW50OgoiVkREQSBhbmQgVkRESSBtdXN0IGJlIHBvd2VyZWQg ZG93biB3aXRoIG1pbmltdW0gMTIwbXNlYy4iCgpUaGlzIGlzIHNpbWlsYXIgdG8gdGhlIHVucHJl cGFyZSBkZWxheSB0byBiZSBmb3VuZCBpbiBzaW1wbGUtcGFuZWwuYwpTbyBhbiB1bnByZXBhcmUg ZGVsYXkgc2VlbXMgaW4gb3JkZXIgaGVyZS4KCgo+ID4gPiArc3RhdGljIGNvbnN0IHN0cnVjdCBz dDc3MDFfcGFuZWxfZGVzYyB0czg1NTBiX2Rlc2MgPSB7Cj4gPiA+ICsgICAgIC5tb2RlID0gJnRz ODU1MGJfbW9kZSwKPiA+ID4gKyAgICAgLmxhbmVzID0gMiwKPiA+ID4gKyAgICAgLmZsYWdzID0g TUlQSV9EU0lfTU9ERV9WSURFTywKPiA+ID4gKyAgICAgLmZvcm1hdCA9IE1JUElfRFNJX0ZNVF9S R0I4ODgsCj4gPiA+ICsgICAgIC5zdXBwbHlfbmFtZXMgPSB0czg1NTBiX3N1cHBseV9uYW1lcywK PiA+ID4gKyAgICAgLm51bV9zdXBwbGllcyA9IEFSUkFZX1NJWkUodHM4NTUwYl9zdXBwbHlfbmFt ZXMpLAo+ID4gPiArICAgICAucGFuZWxfc2xlZXBfZGVsYXkgPSA4MCwgLyogcGFuZWwgbmVlZCBl eHRyYSA4MG1zIGZvciBzbGVlcCBvdXQgY21kICovCj4gPiBJbiB0aGUgb25seSBwbGFjZSB0aGlz IGlzIHVzZWQgdGhlcmUgaXMgYWRkZWQgMTIwIG1zIHRvby4KPiA+IExvb2tzIGluY29uc2lzdGVu dCAtIGRvIHdlIGhhdmUgdGhlIHNhbWUgZGVsYXkgdHdpY2U/Cj4gCj4gMTIwbXMgaXMgdGhlIG9u ZSByZWNvbW1lbmRlZCBieSBzdDc3MDEgY29udHJvbGxlciBkZWxheSBhZnRlciBhIHNsZWVwCj4g b3V0IGNvbW1hbmQsIHBsZWFzZSBjaGVjayBpdCBpbiBkYXRhc2hlZXQgWzFdLCBwYWdlIDE4OC4g QW5kIHRoaXMgODBtcwo+IGlzIHNwZWNpZmljIHRvIFRTODU1MEIgcGFuZWwgYXMgcGVyIEJTUCBj b2RlIHRoaXMgZG9lc24ndCBoYXZlIHByb3Blcgo+IGRvY3VtZW50YXRpb24gYnV0IHRoZSBCU1Ag Y29kZSBkb2luZyB0aGlzIGV4dHJhIDgwbXMuCgpPSywgdGhhbmtzIGZvciB0aGUgcG9pbnRlciB0 byB0aGUgZGF0YXNoZWV0LgoKPiBbMV0gaHR0cDovL3d3dy5zdGFydGVrLWxjZC5jb20vcmVzL3N0 YXJ0ZWtsY2QvcGRyZXMvMjAxNzA1LzIwMTcwNTEyMTQ0MjQyOTA0LnBkZgoKCglTYW0KX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxp bmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJl ZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 60F67C43387 for ; Sat, 12 Jan 2019 10:14:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 348DA20870 for ; Sat, 12 Jan 2019 10:14:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725857AbfALKOR (ORCPT ); Sat, 12 Jan 2019 05:14:17 -0500 Received: from asavdk4.altibox.net ([109.247.116.15]:56088 "EHLO asavdk4.altibox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725816AbfALKOQ (ORCPT ); Sat, 12 Jan 2019 05:14:16 -0500 Received: from ravnborg.org (unknown [158.248.194.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by asavdk4.altibox.net (Postfix) with ESMTPS id 2550F804D2; Sat, 12 Jan 2019 11:14:10 +0100 (CET) Date: Sat, 12 Jan 2019 11:14:09 +0100 From: Sam Ravnborg To: Jagan Teki Cc: David Airlie , linux-amarula@amarulasolutions.com, linux-kernel , dri-devel , Thierry Reding , Michael Trimarchi , Sean Paul Subject: Re: [PATCH V7 2/2] drm/panel: Add Sitronix ST7701 panel driver Message-ID: <20190112101409.GA14273@ravnborg.org> References: <20190111124714.19566-1-jagan@amarulasolutions.com> <20190111124714.19566-2-jagan@amarulasolutions.com> <20190111211931.GA29735@ravnborg.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-CMAE-Score: 0 X-CMAE-Analysis: v=2.3 cv=UpRNyd4B c=1 sm=1 tr=0 a=UWs3HLbX/2nnQ3s7vZ42gw==:117 a=UWs3HLbX/2nnQ3s7vZ42gw==:17 a=kj9zAlcOel0A:10 a=7gkXJVJtAAAA:8 a=6-oEOvViAAAA:8 a=LGALzHtsx367PvwkiA0A:9 a=XysytHMBpD90YD8v:21 a=fZ7kpXaeLR5VPvBn:21 a=CjuIK1q_8ugA:10 a=E9Po1WZjFZOl8hwRPBS3:22 a=XKUzlloKlYS27tEbwpHE:22 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Jagan. > On Sat, Jan 12, 2019 at 2:49 AM Sam Ravnborg wrote: > > > > Hi Jagan. > > > > Gave this another more detailed read - triggered some additional comments. > > Depite the comments it looks good, and this is all more or > > less cosmetic improvements. > > Thanks for the review. > > > > > Sam > > > > > +struct st7701_panel_desc { > > > + const struct drm_display_mode *mode; > > > + unsigned int lanes; > > > + unsigned long flags; > > > + enum mipi_dsi_pixel_format format; > > > + const char *const *supply_names; > > > + unsigned int num_supplies; > > > + unsigned int panel_sleep_delay; > > > +}; > > > + > > > +struct st7701 { > > > + struct drm_panel panel; > > > + struct mipi_dsi_device *dsi; > > > + const struct st7701_panel_desc *desc; > > > + > > > + struct backlight_device *backlight; > > > + struct regulator_bulk_data *supplies; > > > + unsigned int num_supplies; > > I cannot see that num_supplies in this struct are used? > > Yes it is used in the code, please check in struct st7701_panel_desc. I have applied the patch and deleted num_supplies - now build errors. So num_supplies in struct st7701 is not used and should be deleted. > > > +static int st7701_prepare(struct drm_panel *panel) > > > +{ > > > + struct st7701 *st7701 = panel_to_st7701(panel); > > > + int ret; > > > + > > > + gpiod_set_value(st7701->reset, 0); > > > + msleep(20); > > > + > > > + ret = regulator_bulk_enable(st7701->desc->num_supplies, > > > + st7701->supplies); > > > + if (ret < 0) > > > + return ret; > > > + msleep(20); > > > + > > > + gpiod_set_value(st7701->reset, 1); > > > + msleep(20); > > > + > > > + gpiod_set_value(st7701->reset, 0); > > > + msleep(30); > > > + > > > + gpiod_set_value(st7701->reset, 1); > > > + msleep(150); > > > + > > > + st7701_init_sequence(st7701); > > > + > > > + return 0; > > > +} > > > + > > > > > +static int st7701_unprepare(struct drm_panel *panel) > > > +{ > > > + struct st7701 *st7701 = panel_to_st7701(panel); > > > + > > > + ST7701_DSI(st7701, MIPI_DCS_EXIT_SLEEP_MODE, 0x00); Should this be MIPI_DCS_ENTER_SLEEP_MODE? Note - you have shortcuts fot the standard commands like in this case: mipi_dsi_dcs_enter_sleep_mode(st7701->dsi); Thay could be introduced in many palces, but I also like how all the commands follows a consistent way to be issued. So consider this only if this was new for you. > > > + > > > + msleep(st7701->sleep_delay); > > > + > > > + gpiod_set_value(st7701->reset, 0); > > > + > > > + gpiod_set_value(st7701->reset, 1); > > > + > > > + gpiod_set_value(st7701->reset, 0); > > No timing constrains here? In prepare there are sleeps intermixed. > > Delay while doing unprare is not needed I suppose. If the purpose is alone to reset the display then a single write '0' should do it I think And there is a requirement that it must be low for a minimum of 10 us which would be good to have here. I aslo found in chapter 9. (page 163 - second line) this statement: "VDDA and VDDI must be powered down with minimum 120msec." This is similar to the unprepare delay to be found in simple-panel.c So an unprepare delay seems in order here. > > > +static const struct st7701_panel_desc ts8550b_desc = { > > > + .mode = &ts8550b_mode, > > > + .lanes = 2, > > > + .flags = MIPI_DSI_MODE_VIDEO, > > > + .format = MIPI_DSI_FMT_RGB888, > > > + .supply_names = ts8550b_supply_names, > > > + .num_supplies = ARRAY_SIZE(ts8550b_supply_names), > > > + .panel_sleep_delay = 80, /* panel need extra 80ms for sleep out cmd */ > > In the only place this is used there is added 120 ms too. > > Looks inconsistent - do we have the same delay twice? > > 120ms is the one recommended by st7701 controller delay after a sleep > out command, please check it in datasheet [1], page 188. And this 80ms > is specific to TS8550B panel as per BSP code this doesn't have proper > documentation but the BSP code doing this extra 80ms. OK, thanks for the pointer to the datasheet. > [1] http://www.startek-lcd.com/res/starteklcd/pdres/201705/20170512144242904.pdf Sam