From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755467AbcKVIQ6 (ORCPT ); Tue, 22 Nov 2016 03:16:58 -0500 Received: from mail-oi0-f46.google.com ([209.85.218.46]:33309 "EHLO mail-oi0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755267AbcKVIQ4 (ORCPT ); Tue, 22 Nov 2016 03:16:56 -0500 MIME-Version: 1.0 In-Reply-To: <1961666.AZMl081OYz@avalon> References: <1479775052-28194-1-git-send-email-john.stultz@linaro.org> <1479775052-28194-2-git-send-email-john.stultz@linaro.org> <1961666.AZMl081OYz@avalon> From: John Stultz Date: Tue, 22 Nov 2016 00:16:55 -0800 Message-ID: Subject: Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally To: Laurent Pinchart Cc: lkml , David Airlie , Archit Taneja , Wolfram Sang , Lars-Peter Clausen , "dri-devel@lists.freedesktop.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 On Tue, Nov 22, 2016 at 12:14 AM, Laurent Pinchart wrote: > Hi John, > > Thank you for the patch. > > On Monday 21 Nov 2016 16:37:30 John Stultz wrote: >> In chasing down issues with EDID probing, I found some >> duplicated but incomplete logic used to power the chip on and >> off. >> >> This patch refactors the adv7511_power_on/off functions, so >> they can be used for internal needs, and replaces duplicative >> logic that powers the chip on and off around the EDID probing >> with the common logic. >> >> Cc: David Airlie >> Cc: Archit Taneja >> Cc: Wolfram Sang >> Cc: Lars-Peter Clausen >> Cc: Laurent Pinchart >> Cc: dri-devel@lists.freedesktop.org >> Signed-off-by: John Stultz >> --- >> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 30 +++++++++++-------------- >> 1 file changed, 14 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 8dba729..b240e05 >> 100644 >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> @@ -325,7 +325,7 @@ static void adv7511_set_link_config(struct adv7511 >> *adv7511, adv7511->rgb = config->input_colorspace == HDMI_COLORSPACE_RGB; >> } >> >> -static void adv7511_power_on(struct adv7511 *adv7511) >> +static void __adv7511_power_on(struct adv7511 *adv7511) >> { >> adv7511->current_edid_segment = -1; >> >> @@ -343,6 +343,7 @@ static void adv7511_power_on(struct adv7511 *adv7511) >> ADV7511_INT1_DDC_ERROR); >> } >> >> + > > This isn't needed. Apologies. I saw this right after I sent it! >> /* >> * Per spec it is allowed to pulse the HPD signal to indicate that the >> * EDID information has changed. Some monitors do this when they > wakeup >> @@ -362,11 +363,15 @@ static void adv7511_power_on(struct adv7511 *adv7511) >> >> if (adv7511->type == ADV7533) >> adv7533_dsi_power_on(adv7511); >> +} >> >> +static void adv7511_power_on(struct adv7511 *adv7511) >> +{ >> + __adv7511_power_on(adv7511); >> adv7511->powered = true; >> } >> >> -static void adv7511_power_off(struct adv7511 *adv7511) >> +static void __adv7511_power_off(struct adv7511 *adv7511) >> { >> /* TODO: setup additional power down modes */ >> regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER, >> @@ -376,7 +381,11 @@ static void adv7511_power_off(struct adv7511 *adv7511) >> >> if (adv7511->type == ADV7533) >> adv7533_dsi_power_off(adv7511); >> +} >> >> +static void adv7511_power_off(struct adv7511 *adv7511) >> +{ >> + __adv7511_power_off(adv7511); >> adv7511->powered = false; >> } >> >> @@ -545,24 +554,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511, >> unsigned int count; >> >> /* Reading the EDID only works if the device is powered */ >> - if (!adv7511->powered) { >> - regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER, >> - ADV7511_POWER_POWER_DOWN, 0); >> - if (adv7511->i2c_main->irq) { >> - regmap_write(adv7511->regmap, > ADV7511_REG_INT_ENABLE(0), >> - ADV7511_INT0_EDID_READY); >> - regmap_write(adv7511->regmap, > ADV7511_REG_INT_ENABLE(1), >> - ADV7511_INT1_DDC_ERROR); >> - } >> - adv7511->current_edid_segment = -1; >> - } >> + if (!adv7511->powered) >> + __adv7511_power_on(adv7511); > > The __adv7511_power_on() function does more than the above, in particular it > performs an expensive regcache_sync() and calls adv7533_dsi_power_on() for the > ADV7533. Don't those operations have side effects that are either not wanted > or not needed here ? In any case this patch modifies the behaviour of the > driver, which needs to be documented in the kernel message. Sorry, what do you mean by kernel message? Commit message, maybe? Fair point, I'll review the adv7533_dsi_power_on bits and see if they should move out to the external function rather then the internal one. Similarly for the regcache_sync. Thanks so much for the review! thanks -john From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Stultz Subject: Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally Date: Tue, 22 Nov 2016 00:16:55 -0800 Message-ID: References: <1479775052-28194-1-git-send-email-john.stultz@linaro.org> <1479775052-28194-2-git-send-email-john.stultz@linaro.org> <1961666.AZMl081OYz@avalon> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mail-oi0-x22c.google.com (mail-oi0-x22c.google.com [IPv6:2607:f8b0:4003:c06::22c]) by gabe.freedesktop.org (Postfix) with ESMTPS id D5CE46E122 for ; Tue, 22 Nov 2016 08:16:56 +0000 (UTC) Received: by mail-oi0-x22c.google.com with SMTP id v84so10655956oie.3 for ; Tue, 22 Nov 2016 00:16:56 -0800 (PST) In-Reply-To: <1961666.AZMl081OYz@avalon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Laurent Pinchart Cc: lkml , "dri-devel@lists.freedesktop.org" , Wolfram Sang List-Id: dri-devel@lists.freedesktop.org T24gVHVlLCBOb3YgMjIsIDIwMTYgYXQgMTI6MTQgQU0sIExhdXJlbnQgUGluY2hhcnQKPGxhdXJl bnQucGluY2hhcnRAaWRlYXNvbmJvYXJkLmNvbT4gd3JvdGU6Cj4gSGkgSm9obiwKPgo+IFRoYW5r IHlvdSBmb3IgdGhlIHBhdGNoLgo+Cj4gT24gTW9uZGF5IDIxIE5vdiAyMDE2IDE2OjM3OjMwIEpv aG4gU3R1bHR6IHdyb3RlOgo+PiBJbiBjaGFzaW5nIGRvd24gaXNzdWVzIHdpdGggRURJRCBwcm9i aW5nLCBJIGZvdW5kIHNvbWUKPj4gZHVwbGljYXRlZCBidXQgaW5jb21wbGV0ZSBsb2dpYyB1c2Vk IHRvIHBvd2VyIHRoZSBjaGlwIG9uIGFuZAo+PiBvZmYuCj4+Cj4+IFRoaXMgcGF0Y2ggcmVmYWN0 b3JzIHRoZSBhZHY3NTExX3Bvd2VyX29uL29mZiBmdW5jdGlvbnMsIHNvCj4+IHRoZXkgY2FuIGJl IHVzZWQgZm9yIGludGVybmFsIG5lZWRzLCBhbmQgcmVwbGFjZXMgZHVwbGljYXRpdmUKPj4gbG9n aWMgdGhhdCBwb3dlcnMgdGhlIGNoaXAgb24gYW5kIG9mZiBhcm91bmQgdGhlIEVESUQgcHJvYmlu Zwo+PiB3aXRoIHRoZSBjb21tb24gbG9naWMuCj4+Cj4+IENjOiBEYXZpZCBBaXJsaWUgPGFpcmxp ZWRAbGludXguaWU+Cj4+IENjOiBBcmNoaXQgVGFuZWphIDxhcmNoaXR0QGNvZGVhdXJvcmEub3Jn Pgo+PiBDYzogV29sZnJhbSBTYW5nIDx3c2ErcmVuZXNhc0BzYW5nLWVuZ2luZWVyaW5nLmNvbT4K Pj4gQ2M6IExhcnMtUGV0ZXIgQ2xhdXNlbiA8bGFyc0BtZXRhZm9vLmRlPgo+PiBDYzogTGF1cmVu dCBQaW5jaGFydCA8bGF1cmVudC5waW5jaGFydEBpZGVhc29uYm9hcmQuY29tPgo+PiBDYzogZHJp LWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwo+PiBTaWduZWQtb2ZmLWJ5OiBKb2huIFN0dWx0 eiA8am9obi5zdHVsdHpAbGluYXJvLm9yZz4KPj4gLS0tCj4+ICBkcml2ZXJzL2dwdS9kcm0vYnJp ZGdlL2Fkdjc1MTEvYWR2NzUxMV9kcnYuYyB8IDMwICsrKysrKysrKysrLS0tLS0tLS0tLS0tLS0K Pj4gIDEgZmlsZSBjaGFuZ2VkLCAxNCBpbnNlcnRpb25zKCspLCAxNiBkZWxldGlvbnMoLSkKPj4K Pj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9icmlkZ2UvYWR2NzUxMS9hZHY3NTExX2Ry di5jCj4+IGIvZHJpdmVycy9ncHUvZHJtL2JyaWRnZS9hZHY3NTExL2Fkdjc1MTFfZHJ2LmMgaW5k ZXggOGRiYTcyOS4uYjI0MGUwNQo+PiAxMDA2NDQKPj4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL2Jy aWRnZS9hZHY3NTExL2Fkdjc1MTFfZHJ2LmMKPj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL2JyaWRn ZS9hZHY3NTExL2Fkdjc1MTFfZHJ2LmMKPj4gQEAgLTMyNSw3ICszMjUsNyBAQCBzdGF0aWMgdm9p ZCBhZHY3NTExX3NldF9saW5rX2NvbmZpZyhzdHJ1Y3QgYWR2NzUxMQo+PiAqYWR2NzUxMSwgYWR2 NzUxMS0+cmdiID0gY29uZmlnLT5pbnB1dF9jb2xvcnNwYWNlID09IEhETUlfQ09MT1JTUEFDRV9S R0I7Cj4+ICB9Cj4+Cj4+IC1zdGF0aWMgdm9pZCBhZHY3NTExX3Bvd2VyX29uKHN0cnVjdCBhZHY3 NTExICphZHY3NTExKQo+PiArc3RhdGljIHZvaWQgX19hZHY3NTExX3Bvd2VyX29uKHN0cnVjdCBh ZHY3NTExICphZHY3NTExKQo+PiAgewo+PiAgICAgICBhZHY3NTExLT5jdXJyZW50X2VkaWRfc2Vn bWVudCA9IC0xOwo+Pgo+PiBAQCAtMzQzLDYgKzM0Myw3IEBAIHN0YXRpYyB2b2lkIGFkdjc1MTFf cG93ZXJfb24oc3RydWN0IGFkdjc1MTEgKmFkdjc1MTEpCj4+ICAgICAgICAgICAgICAgICAgICAg ICAgICAgIEFEVjc1MTFfSU5UMV9ERENfRVJST1IpOwo+PiAgICAgICB9Cj4+Cj4+ICsKPgo+IFRo aXMgaXNuJ3QgbmVlZGVkLgoKQXBvbG9naWVzLiBJIHNhdyB0aGlzIHJpZ2h0IGFmdGVyIEkgc2Vu dCBpdCEKCj4+ICAgICAgIC8qCj4+ICAgICAgICAqIFBlciBzcGVjIGl0IGlzIGFsbG93ZWQgdG8g cHVsc2UgdGhlIEhQRCBzaWduYWwgdG8gaW5kaWNhdGUgdGhhdCB0aGUKPj4gICAgICAgICogRURJ RCBpbmZvcm1hdGlvbiBoYXMgY2hhbmdlZC4gU29tZSBtb25pdG9ycyBkbyB0aGlzIHdoZW4gdGhl eQo+IHdha2V1cAo+PiBAQCAtMzYyLDExICszNjMsMTUgQEAgc3RhdGljIHZvaWQgYWR2NzUxMV9w b3dlcl9vbihzdHJ1Y3QgYWR2NzUxMSAqYWR2NzUxMSkKPj4KPj4gICAgICAgaWYgKGFkdjc1MTEt PnR5cGUgPT0gQURWNzUzMykKPj4gICAgICAgICAgICAgICBhZHY3NTMzX2RzaV9wb3dlcl9vbihh ZHY3NTExKTsKPj4gK30KPj4KPj4gK3N0YXRpYyB2b2lkIGFkdjc1MTFfcG93ZXJfb24oc3RydWN0 IGFkdjc1MTEgKmFkdjc1MTEpCj4+ICt7Cj4+ICsgICAgIF9fYWR2NzUxMV9wb3dlcl9vbihhZHY3 NTExKTsKPj4gICAgICAgYWR2NzUxMS0+cG93ZXJlZCA9IHRydWU7Cj4+ICB9Cj4+Cj4+IC1zdGF0 aWMgdm9pZCBhZHY3NTExX3Bvd2VyX29mZihzdHJ1Y3QgYWR2NzUxMSAqYWR2NzUxMSkKPj4gK3N0 YXRpYyB2b2lkIF9fYWR2NzUxMV9wb3dlcl9vZmYoc3RydWN0IGFkdjc1MTEgKmFkdjc1MTEpCj4+ ICB7Cj4+ICAgICAgIC8qIFRPRE86IHNldHVwIGFkZGl0aW9uYWwgcG93ZXIgZG93biBtb2RlcyAq Lwo+PiAgICAgICByZWdtYXBfdXBkYXRlX2JpdHMoYWR2NzUxMS0+cmVnbWFwLCBBRFY3NTExX1JF R19QT1dFUiwKPj4gQEAgLTM3Niw3ICszODEsMTEgQEAgc3RhdGljIHZvaWQgYWR2NzUxMV9wb3dl cl9vZmYoc3RydWN0IGFkdjc1MTEgKmFkdjc1MTEpCj4+Cj4+ICAgICAgIGlmIChhZHY3NTExLT50 eXBlID09IEFEVjc1MzMpCj4+ICAgICAgICAgICAgICAgYWR2NzUzM19kc2lfcG93ZXJfb2ZmKGFk djc1MTEpOwo+PiArfQo+Pgo+PiArc3RhdGljIHZvaWQgYWR2NzUxMV9wb3dlcl9vZmYoc3RydWN0 IGFkdjc1MTEgKmFkdjc1MTEpCj4+ICt7Cj4+ICsgICAgIF9fYWR2NzUxMV9wb3dlcl9vZmYoYWR2 NzUxMSk7Cj4+ICAgICAgIGFkdjc1MTEtPnBvd2VyZWQgPSBmYWxzZTsKPj4gIH0KPj4KPj4gQEAg LTU0NSwyNCArNTU0LDEzIEBAIHN0YXRpYyBpbnQgYWR2NzUxMV9nZXRfbW9kZXMoc3RydWN0IGFk djc1MTEgKmFkdjc1MTEsCj4+ICAgICAgIHVuc2lnbmVkIGludCBjb3VudDsKPj4KPj4gICAgICAg LyogUmVhZGluZyB0aGUgRURJRCBvbmx5IHdvcmtzIGlmIHRoZSBkZXZpY2UgaXMgcG93ZXJlZCAq Lwo+PiAtICAgICBpZiAoIWFkdjc1MTEtPnBvd2VyZWQpIHsKPj4gLSAgICAgICAgICAgICByZWdt YXBfdXBkYXRlX2JpdHMoYWR2NzUxMS0+cmVnbWFwLCBBRFY3NTExX1JFR19QT1dFUiwKPj4gLSAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgQURWNzUxMV9QT1dFUl9QT1dFUl9ET1dOLCAw KTsKPj4gLSAgICAgICAgICAgICBpZiAoYWR2NzUxMS0+aTJjX21haW4tPmlycSkgewo+PiAtICAg ICAgICAgICAgICAgICAgICAgcmVnbWFwX3dyaXRlKGFkdjc1MTEtPnJlZ21hcCwKPiBBRFY3NTEx X1JFR19JTlRfRU5BQkxFKDApLAo+PiAtICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg IEFEVjc1MTFfSU5UMF9FRElEX1JFQURZKTsKPj4gLSAgICAgICAgICAgICAgICAgICAgIHJlZ21h cF93cml0ZShhZHY3NTExLT5yZWdtYXAsCj4gQURWNzUxMV9SRUdfSU5UX0VOQUJMRSgxKSwKPj4g LSAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBBRFY3NTExX0lOVDFfRERDX0VSUk9S KTsKPj4gLSAgICAgICAgICAgICB9Cj4+IC0gICAgICAgICAgICAgYWR2NzUxMS0+Y3VycmVudF9l ZGlkX3NlZ21lbnQgPSAtMTsKPj4gLSAgICAgfQo+PiArICAgICBpZiAoIWFkdjc1MTEtPnBvd2Vy ZWQpCj4+ICsgICAgICAgICAgICAgX19hZHY3NTExX3Bvd2VyX29uKGFkdjc1MTEpOwo+Cj4gVGhl IF9fYWR2NzUxMV9wb3dlcl9vbigpIGZ1bmN0aW9uIGRvZXMgbW9yZSB0aGFuIHRoZSBhYm92ZSwg aW4gcGFydGljdWxhciBpdAo+IHBlcmZvcm1zIGFuIGV4cGVuc2l2ZSByZWdjYWNoZV9zeW5jKCkg YW5kIGNhbGxzIGFkdjc1MzNfZHNpX3Bvd2VyX29uKCkgZm9yIHRoZQo+IEFEVjc1MzMuIERvbid0 IHRob3NlIG9wZXJhdGlvbnMgaGF2ZSBzaWRlIGVmZmVjdHMgdGhhdCBhcmUgZWl0aGVyIG5vdCB3 YW50ZWQKPiBvciBub3QgbmVlZGVkIGhlcmUgPyBJbiBhbnkgY2FzZSB0aGlzIHBhdGNoIG1vZGlm aWVzIHRoZSBiZWhhdmlvdXIgb2YgdGhlCj4gZHJpdmVyLCB3aGljaCBuZWVkcyB0byBiZSBkb2N1 bWVudGVkIGluIHRoZSBrZXJuZWwgbWVzc2FnZS4KClNvcnJ5LCB3aGF0IGRvIHlvdSBtZWFuIGJ5 IGtlcm5lbCBtZXNzYWdlPyBDb21taXQgbWVzc2FnZSwgbWF5YmU/CgpGYWlyIHBvaW50LCBJJ2xs IHJldmlldyB0aGUgYWR2NzUzM19kc2lfcG93ZXJfb24gYml0cyBhbmQgc2VlIGlmIHRoZXkKc2hv dWxkIG1vdmUgb3V0IHRvIHRoZSBleHRlcm5hbCBmdW5jdGlvbiByYXRoZXIgdGhlbiB0aGUgaW50 ZXJuYWwgb25lLgpTaW1pbGFybHkgZm9yIHRoZSByZWdjYWNoZV9zeW5jLgoKVGhhbmtzIHNvIG11 Y2ggZm9yIHRoZSByZXZpZXchCgp0aGFua3MKLWpvaG4KX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxA bGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxt YW4vbGlzdGluZm8vZHJpLWRldmVsCg==