From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752183AbaBBSxn (ORCPT ); Sun, 2 Feb 2014 13:53:43 -0500 Received: from smtp1-g21.free.fr ([212.27.42.1]:35476 "EHLO smtp1-g21.free.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751974AbaBBSxm convert rfc822-to-8bit (ORCPT ); Sun, 2 Feb 2014 13:53:42 -0500 Date: Sun, 2 Feb 2014 19:54:00 +0100 From: Jean-Francois Moine To: Russell King - ARM Linux Cc: dri-devel@lists.freedesktop.org, Dave Airlie , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Rob Clark Subject: Re: [PATCH v5 00/23] Message-ID: <20140202195400.073f4eb4@armhf> In-Reply-To: <20140202182349.GJ26684@n2100.arm.linux.org.uk> References: <20140202124358.GD26684@n2100.arm.linux.org.uk> <20140202190606.6fa193ce@armhf> <20140202182349.GJ26684@n2100.arm.linux.org.uk> X-Mailer: Claws Mail 3.9.3 (GTK+ 2.24.22; arm-unknown-linux-gnueabihf) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 2 Feb 2014 18:23:49 +0000 Russell King - ARM Linux wrote: > On Sun, Feb 02, 2014 at 07:06:06PM +0100, Jean-Francois Moine wrote: > > On Sun, 2 Feb 2014 12:43:58 +0000 > > Russell King - ARM Linux wrote: > > > > > On Wed, Jan 29, 2014 at 10:01:22AM +0100, Jean-Francois Moine wrote: > > > > This patch set contains various extensions to the tda998x driver: > > > > > > > > - simplify the i2c read/write > > > > - code cleanup and fix some small errors > > > > - use global constants > > > > - don't read write-only registers > > > > - add DT support > > > > - use IRQ for connection status and EDID read > > > > > > I discussed these patches with Rob Clark recently, and the conclusion > > > we came to is that I'll merge them into a git tree, test them, and > > > once I'm happy I'll send a pull request as appropriate. > > > > > > I'll go through them later today. Those patches which have been re- > > > posted without any change for the last few times (the first few) I'll > > > take into my git tree today so you don't have to keep re-posting them > > > (more importantly, I won't have to keep on looking at them either.) > > > > Thanks. > > > > BTW, I found some problems with the patch 12 'add DT support' you > > already acked: > > > > - the .of_match_table is not needed because the i2c client is created by > > the i2c subsystem from the 'reg' in the DT, > > Okay - may it be a good idea to have someone knowledgable of I2C give it > a review? Sure! There is still a lot of magic in the DT. I used the tda998x in the DT for a long time without .of_match_table and it loaded correctly. I added it in the patch just because my other modules had it. A few days ago, when I moved the tda998x audio codec description in the DT as a subnode of the tda998x i2c, the codec module was not loaded. An of_platform_populate() of the subnodes was needed in the tda998x i2c driver. I could not find why... > > - on encoder_destroy(), the function drm_i2c_encoder_destroy() > > unregisters the i2c client, so, with a DT, a second encoder_init() > > would crash. > > I think this is one of the down-sides of trying to bolt DT into this: > the drm encoder slave support is not designed to cope with an i2c client > device pre-created. > > In fact, I can't see how this stuff comes anywhere close to working in > a DT setup: in such a scenario, you declare that there's a tda998x > device in DT. I2C parses this, and creates an i2c_client itself for > the tda998x. > > When the TDA998x driver initialises, it finds this i2c client and > binds to it, calling tda998x_probe(), which does nothing. > > However, the only way to attach a slave encoder to a DRM device is via > a call to drm_i2c_encoder_init(), which unconditionally calls > i2c_new_device(). This creates a _new_ i2c_client structure, again > unconditionally, for the tda998x. This must be bound by the I2C > subsystem to a driver - hopefully the tda998x driver, which then > calls it's encoder_init function. > > None of this will happen if DT has already created an i2c_client at > the appropriate address, because DRMs i2c_new_device() will fail. > > My hypothesis is that you have other patches to I2C and/or DRM to > sort this out which you haven't been posting with this series. So, > could you please provide some hints as to how this works? I explained how to use the tda998x in a DT context in a message to Jyri Sarha: http://lists.freedesktop.org/archives/dri-devel/2014-January/052936.html -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ From mboxrd@z Thu Jan 1 00:00:00 1970 From: moinejf@free.fr (Jean-Francois Moine) Date: Sun, 2 Feb 2014 19:54:00 +0100 Subject: [PATCH v5 00/23] In-Reply-To: <20140202182349.GJ26684@n2100.arm.linux.org.uk> References: <20140202124358.GD26684@n2100.arm.linux.org.uk> <20140202190606.6fa193ce@armhf> <20140202182349.GJ26684@n2100.arm.linux.org.uk> Message-ID: <20140202195400.073f4eb4@armhf> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Sun, 2 Feb 2014 18:23:49 +0000 Russell King - ARM Linux wrote: > On Sun, Feb 02, 2014 at 07:06:06PM +0100, Jean-Francois Moine wrote: > > On Sun, 2 Feb 2014 12:43:58 +0000 > > Russell King - ARM Linux wrote: > > > > > On Wed, Jan 29, 2014 at 10:01:22AM +0100, Jean-Francois Moine wrote: > > > > This patch set contains various extensions to the tda998x driver: > > > > > > > > - simplify the i2c read/write > > > > - code cleanup and fix some small errors > > > > - use global constants > > > > - don't read write-only registers > > > > - add DT support > > > > - use IRQ for connection status and EDID read > > > > > > I discussed these patches with Rob Clark recently, and the conclusion > > > we came to is that I'll merge them into a git tree, test them, and > > > once I'm happy I'll send a pull request as appropriate. > > > > > > I'll go through them later today. Those patches which have been re- > > > posted without any change for the last few times (the first few) I'll > > > take into my git tree today so you don't have to keep re-posting them > > > (more importantly, I won't have to keep on looking at them either.) > > > > Thanks. > > > > BTW, I found some problems with the patch 12 'add DT support' you > > already acked: > > > > - the .of_match_table is not needed because the i2c client is created by > > the i2c subsystem from the 'reg' in the DT, > > Okay - may it be a good idea to have someone knowledgable of I2C give it > a review? Sure! There is still a lot of magic in the DT. I used the tda998x in the DT for a long time without .of_match_table and it loaded correctly. I added it in the patch just because my other modules had it. A few days ago, when I moved the tda998x audio codec description in the DT as a subnode of the tda998x i2c, the codec module was not loaded. An of_platform_populate() of the subnodes was needed in the tda998x i2c driver. I could not find why... > > - on encoder_destroy(), the function drm_i2c_encoder_destroy() > > unregisters the i2c client, so, with a DT, a second encoder_init() > > would crash. > > I think this is one of the down-sides of trying to bolt DT into this: > the drm encoder slave support is not designed to cope with an i2c client > device pre-created. > > In fact, I can't see how this stuff comes anywhere close to working in > a DT setup: in such a scenario, you declare that there's a tda998x > device in DT. I2C parses this, and creates an i2c_client itself for > the tda998x. > > When the TDA998x driver initialises, it finds this i2c client and > binds to it, calling tda998x_probe(), which does nothing. > > However, the only way to attach a slave encoder to a DRM device is via > a call to drm_i2c_encoder_init(), which unconditionally calls > i2c_new_device(). This creates a _new_ i2c_client structure, again > unconditionally, for the tda998x. This must be bound by the I2C > subsystem to a driver - hopefully the tda998x driver, which then > calls it's encoder_init function. > > None of this will happen if DT has already created an i2c_client at > the appropriate address, because DRMs i2c_new_device() will fail. > > My hypothesis is that you have other patches to I2C and/or DRM to > sort this out which you haven't been posting with this series. So, > could you please provide some hints as to how this works? I explained how to use the tda998x in a DT context in a message to Jyri Sarha: http://lists.freedesktop.org/archives/dri-devel/2014-January/052936.html -- Ken ar c'henta? | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jean-Francois Moine Subject: Re: [PATCH v5 00/23] Date: Sun, 2 Feb 2014 19:54:00 +0100 Message-ID: <20140202195400.073f4eb4@armhf> References: <20140202124358.GD26684@n2100.arm.linux.org.uk> <20140202190606.6fa193ce@armhf> <20140202182349.GJ26684@n2100.arm.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from smtp1-g21.free.fr (smtp1-g21.free.fr [212.27.42.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 90502FCFA1 for ; Sun, 2 Feb 2014 10:53:41 -0800 (PST) In-Reply-To: <20140202182349.GJ26684@n2100.arm.linux.org.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org To: Russell King - ARM Linux Cc: linux-arm-kernel@lists.infradead.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org T24gU3VuLCAyIEZlYiAyMDE0IDE4OjIzOjQ5ICswMDAwClJ1c3NlbGwgS2luZyAtIEFSTSBMaW51 eCA8bGludXhAYXJtLmxpbnV4Lm9yZy51az4gd3JvdGU6Cgo+IE9uIFN1biwgRmViIDAyLCAyMDE0 IGF0IDA3OjA2OjA2UE0gKzAxMDAsIEplYW4tRnJhbmNvaXMgTW9pbmUgd3JvdGU6Cj4gPiBPbiBT dW4sIDIgRmViIDIwMTQgMTI6NDM6NTggKzAwMDAKPiA+IFJ1c3NlbGwgS2luZyAtIEFSTSBMaW51 eCA8bGludXhAYXJtLmxpbnV4Lm9yZy51az4gd3JvdGU6Cj4gPiAKPiA+ID4gT24gV2VkLCBKYW4g MjksIDIwMTQgYXQgMTA6MDE6MjJBTSArMDEwMCwgSmVhbi1GcmFuY29pcyBNb2luZSB3cm90ZToK PiA+ID4gPiBUaGlzIHBhdGNoIHNldCBjb250YWlucyB2YXJpb3VzIGV4dGVuc2lvbnMgdG8gdGhl IHRkYTk5OHggZHJpdmVyOgo+ID4gPiA+IAo+ID4gPiA+IC0gc2ltcGxpZnkgdGhlIGkyYyByZWFk L3dyaXRlCj4gPiA+ID4gLSBjb2RlIGNsZWFudXAgYW5kIGZpeCBzb21lIHNtYWxsIGVycm9ycwo+ ID4gPiA+IC0gdXNlIGdsb2JhbCBjb25zdGFudHMKPiA+ID4gPiAtIGRvbid0IHJlYWQgd3JpdGUt b25seSByZWdpc3RlcnMKPiA+ID4gPiAtIGFkZCBEVCBzdXBwb3J0Cj4gPiA+ID4gLSB1c2UgSVJR IGZvciBjb25uZWN0aW9uIHN0YXR1cyBhbmQgRURJRCByZWFkCj4gPiA+IAo+ID4gPiBJIGRpc2N1 c3NlZCB0aGVzZSBwYXRjaGVzIHdpdGggUm9iIENsYXJrIHJlY2VudGx5LCBhbmQgdGhlIGNvbmNs dXNpb24KPiA+ID4gd2UgY2FtZSB0byBpcyB0aGF0IEknbGwgbWVyZ2UgdGhlbSBpbnRvIGEgZ2l0 IHRyZWUsIHRlc3QgdGhlbSwgYW5kCj4gPiA+IG9uY2UgSSdtIGhhcHB5IEknbGwgc2VuZCBhIHB1 bGwgcmVxdWVzdCBhcyBhcHByb3ByaWF0ZS4KPiA+ID4gCj4gPiA+IEknbGwgZ28gdGhyb3VnaCB0 aGVtIGxhdGVyIHRvZGF5LiAgVGhvc2UgcGF0Y2hlcyB3aGljaCBoYXZlIGJlZW4gcmUtCj4gPiA+ IHBvc3RlZCB3aXRob3V0IGFueSBjaGFuZ2UgZm9yIHRoZSBsYXN0IGZldyB0aW1lcyAodGhlIGZp cnN0IGZldykgSSdsbAo+ID4gPiB0YWtlIGludG8gbXkgZ2l0IHRyZWUgdG9kYXkgc28geW91IGRv bid0IGhhdmUgdG8ga2VlcCByZS1wb3N0aW5nIHRoZW0KPiA+ID4gKG1vcmUgaW1wb3J0YW50bHks IEkgd29uJ3QgaGF2ZSB0byBrZWVwIG9uIGxvb2tpbmcgYXQgdGhlbSBlaXRoZXIuKQo+ID4gCj4g PiBUaGFua3MuCj4gPiAKPiA+IEJUVywgSSBmb3VuZCBzb21lIHByb2JsZW1zIHdpdGggdGhlIHBh dGNoIDEyICdhZGQgRFQgc3VwcG9ydCcgeW91Cj4gPiBhbHJlYWR5IGFja2VkOgo+ID4gCj4gPiAt IHRoZSAub2ZfbWF0Y2hfdGFibGUgaXMgbm90IG5lZWRlZCBiZWNhdXNlIHRoZSBpMmMgY2xpZW50 IGlzIGNyZWF0ZWQgYnkKPiA+ICAgdGhlIGkyYyBzdWJzeXN0ZW0gZnJvbSB0aGUgJ3JlZycgaW4g dGhlIERULAo+IAo+IE9rYXkgLSBtYXkgaXQgYmUgYSBnb29kIGlkZWEgdG8gaGF2ZSBzb21lb25l IGtub3dsZWRnYWJsZSBvZiBJMkMgZ2l2ZSBpdAo+IGEgcmV2aWV3PwoKU3VyZSEgVGhlcmUgaXMg c3RpbGwgYSBsb3Qgb2YgbWFnaWMgaW4gdGhlIERULgoKSSB1c2VkIHRoZSB0ZGE5OTh4IGluIHRo ZSBEVCBmb3IgYSBsb25nIHRpbWUgd2l0aG91dCAub2ZfbWF0Y2hfdGFibGUKYW5kIGl0IGxvYWRl ZCBjb3JyZWN0bHkuIEkgYWRkZWQgaXQgaW4gdGhlIHBhdGNoIGp1c3QgYmVjYXVzZSBteSBvdGhl cgptb2R1bGVzIGhhZCBpdC4KCkEgZmV3IGRheXMgYWdvLCB3aGVuIEkgbW92ZWQgdGhlIHRkYTk5 OHggYXVkaW8gY29kZWMgZGVzY3JpcHRpb24gaW4gdGhlCkRUIGFzIGEgc3Vibm9kZSBvZiB0aGUg dGRhOTk4eCBpMmMsIHRoZSBjb2RlYyBtb2R1bGUgd2FzIG5vdCBsb2FkZWQuIEFuCm9mX3BsYXRm b3JtX3BvcHVsYXRlKCkgb2YgdGhlIHN1Ym5vZGVzIHdhcyBuZWVkZWQgaW4gdGhlIHRkYTk5OHgg aTJjCmRyaXZlci4gSSBjb3VsZCBub3QgZmluZCB3aHkuLi4KCj4gPiAtIG9uIGVuY29kZXJfZGVz dHJveSgpLCB0aGUgZnVuY3Rpb24gZHJtX2kyY19lbmNvZGVyX2Rlc3Ryb3koKQo+ID4gICB1bnJl Z2lzdGVycyB0aGUgaTJjIGNsaWVudCwgc28sIHdpdGggYSBEVCwgYSBzZWNvbmQgZW5jb2Rlcl9p bml0KCkKPiA+ICAgd291bGQgY3Jhc2guCj4gCj4gSSB0aGluayB0aGlzIGlzIG9uZSBvZiB0aGUg ZG93bi1zaWRlcyBvZiB0cnlpbmcgdG8gYm9sdCBEVCBpbnRvIHRoaXM6Cj4gdGhlIGRybSBlbmNv ZGVyIHNsYXZlIHN1cHBvcnQgaXMgbm90IGRlc2lnbmVkIHRvIGNvcGUgd2l0aCBhbiBpMmMgY2xp ZW50Cj4gZGV2aWNlIHByZS1jcmVhdGVkLgo+IAo+IEluIGZhY3QsIEkgY2FuJ3Qgc2VlIGhvdyB0 aGlzIHN0dWZmIGNvbWVzIGFueXdoZXJlIGNsb3NlIHRvIHdvcmtpbmcgaW4KPiBhIERUIHNldHVw OiBpbiBzdWNoIGEgc2NlbmFyaW8sIHlvdSBkZWNsYXJlIHRoYXQgdGhlcmUncyBhIHRkYTk5OHgK PiBkZXZpY2UgaW4gRFQuICBJMkMgcGFyc2VzIHRoaXMsIGFuZCBjcmVhdGVzIGFuIGkyY19jbGll bnQgaXRzZWxmIGZvcgo+IHRoZSB0ZGE5OTh4Lgo+IAo+IFdoZW4gdGhlIFREQTk5OHggZHJpdmVy IGluaXRpYWxpc2VzLCBpdCBmaW5kcyB0aGlzIGkyYyBjbGllbnQgYW5kCj4gYmluZHMgdG8gaXQs IGNhbGxpbmcgdGRhOTk4eF9wcm9iZSgpLCB3aGljaCBkb2VzIG5vdGhpbmcuCj4gCj4gSG93ZXZl ciwgdGhlIG9ubHkgd2F5IHRvIGF0dGFjaCBhIHNsYXZlIGVuY29kZXIgdG8gYSBEUk0gZGV2aWNl IGlzIHZpYQo+IGEgY2FsbCB0byBkcm1faTJjX2VuY29kZXJfaW5pdCgpLCB3aGljaCB1bmNvbmRp dGlvbmFsbHkgY2FsbHMKPiBpMmNfbmV3X2RldmljZSgpLiAgVGhpcyBjcmVhdGVzIGEgX25ld18g aTJjX2NsaWVudCBzdHJ1Y3R1cmUsIGFnYWluCj4gdW5jb25kaXRpb25hbGx5LCBmb3IgdGhlIHRk YTk5OHguICBUaGlzIG11c3QgYmUgYm91bmQgYnkgdGhlIEkyQwo+IHN1YnN5c3RlbSB0byBhIGRy aXZlciAtIGhvcGVmdWxseSB0aGUgdGRhOTk4eCBkcml2ZXIsIHdoaWNoIHRoZW4KPiBjYWxscyBp dCdzIGVuY29kZXJfaW5pdCBmdW5jdGlvbi4KPiAKPiBOb25lIG9mIHRoaXMgd2lsbCBoYXBwZW4g aWYgRFQgaGFzIGFscmVhZHkgY3JlYXRlZCBhbiBpMmNfY2xpZW50IGF0Cj4gdGhlIGFwcHJvcHJp YXRlIGFkZHJlc3MsIGJlY2F1c2UgRFJNcyBpMmNfbmV3X2RldmljZSgpIHdpbGwgZmFpbC4KPiAK PiBNeSBoeXBvdGhlc2lzIGlzIHRoYXQgeW91IGhhdmUgb3RoZXIgcGF0Y2hlcyB0byBJMkMgYW5k L29yIERSTSB0bwo+IHNvcnQgdGhpcyBvdXQgd2hpY2ggeW91IGhhdmVuJ3QgYmVlbiBwb3N0aW5n IHdpdGggdGhpcyBzZXJpZXMuICBTbywKPiBjb3VsZCB5b3UgcGxlYXNlIHByb3ZpZGUgc29tZSBo aW50cyBhcyB0byBob3cgdGhpcyB3b3Jrcz8KCkkgZXhwbGFpbmVkIGhvdyB0byB1c2UgdGhlIHRk YTk5OHggaW4gYSBEVCBjb250ZXh0IGluIGEgbWVzc2FnZSB0byBKeXJpClNhcmhhOgoKaHR0cDov L2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9hcmNoaXZlcy9kcmktZGV2ZWwvMjAxNC1KYW51YXJ5LzA1 MjkzNi5odG1sCgotLSAKS2VuIGFyIGMnaGVudGHDsQl8CSAgICAgICoqIEJyZWl6aCBoYSBMaW51 eCBhdGF2ISAqKgpKZWYJCXwJCWh0dHA6Ly9tb2luZWpmLmZyZWUuZnIvCl9fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QK ZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwOi8vbGlzdHMuZnJlZWRlc2t0b3Au b3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg==