From mboxrd@z Thu Jan 1 00:00:00 1970 From: laurent.pinchart@ideasonboard.com (Laurent Pinchart) Date: Wed, 02 Aug 2017 17:22:54 +0300 Subject: [PATCH v2 3/4] drm/bridge: dw-hdmi: add cec driver In-Reply-To: References: <20170731142906.GX31807@n2100.armlinux.org.uk> <20170802132730.GZ31807@n2100.armlinux.org.uk> Message-ID: <2270313.kliK1kl0Jl@avalon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Hans, On Wednesday 02 Aug 2017 15:34:33 Hans Verkuil wrote: > On 08/02/17 15:27, Russell King - ARM Linux wrote: > > On Wed, Aug 02, 2017 at 04:14:34PM +0300, Laurent Pinchart wrote: > >> On Wednesday 02 Aug 2017 08:47:23 Hans Verkuil wrote: > >>> On 08/02/2017 12:32 AM, Laurent Pinchart wrote: > >>>>> + > >>>>> + cec_register_cec_notifier(cec->adap, cec->notify); > >>>>> + > >>>>> + return 0; > >>>>> +} > >>>>> + > >>>>> +static int dw_hdmi_cec_remove(struct platform_device *pdev) > >>>>> +{ > >>>>> + struct dw_hdmi_cec *cec = platform_get_drvdata(pdev); > >>>>> + > >>>>> + cec_unregister_adapter(cec->adap); > >>>>> + cec_notifier_put(cec->notify); > >>>>> + > >>>>> + return 0; > >>>>> +} > >>>>> + > >>>>> +static struct platform_driver dw_hdmi_cec_driver = { > >>>>> + .probe = dw_hdmi_cec_probe, > >>>>> + .remove = dw_hdmi_cec_remove, > >>>>> + .driver = { > >>>>> + .name = "dw-hdmi-cec", > >>>>> + }, > >>>>> +}; > >>>>> +module_platform_driver(dw_hdmi_cec_driver); > >>>> > >>>> Is there a particular reason why this has to be a separate module > >>>> instead of simply calling the CEC init/cleanup functions directly from > >>>> the main dw-hdmi driver ? > >>> > >>> Not all SoCs that use dw-hdmi also use the dw-hdmi CEC implementation. > >>> Some use their own implementation (amlogic). > >> > >> Lovely. Of course we need to reinvent the wheel every time, where would > >> the fun be otherwise ? > >> > >>> So by implementing the cec-notifier in the dw-hdmi driver and keeping > >>> dw-hdmi CEC separate you can easily choose whether or not you want to > >>> use this CEC driver or another SoC CEC driver. > >> > >> I'm certainly fine with such a split, but I don't think it requires a > >> separate platform_driver. We could use a similar approach as with the > >> HDMI PHY that can also differ between SoCs. The PHY is identified at > >> runtime when possible, and the SoC-specific glue code can override that > >> with a few data fields and function pointers. > > > > Excuse me if I completely lose interest in reworking the driver at this > > point, as it's enough of an effort to follow the churn in CEC from one > > kernel version to another. I'm not about to rewrite the driver and > > restart the review cycle from scratch and then have several iterations > > of having to update it as CEC continues to evolve. > > > > Let's get this driver in mainline, and then if we want further changes > > we can do that later. > > +1 > > I also don't think the phy idea applies here. CEC implementations range from > tightly coupled to the HDMI implementation to a completely independent i2c > device that is unrelated to any HDMI receiver/transmitter. And various > shades in between those extremes. Then it should be even simpler. Instead of creating a separate platform device for the DW HDMI CEC, we can just call the DW HDMI CEC driver init/cleanup functions directly when the IP core implements CEC. When it doesn't, a separate device will be described in DT (at least to my understanding, otherwise where would it come from ?) and a separate driver would handle it. > The only thing that is needed is that the CEC device needs to be informed > when an EDID is read and when the hotplug disappears. The CEC notifier does > just that and it is already in use, so it is nothing new. No need to rework > this. The notifier implementation in patch 1/4 looks good to me (except for the missing cec_notifier_put() call that I mentioned in my review). -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH v2 3/4] drm/bridge: dw-hdmi: add cec driver Date: Wed, 02 Aug 2017 17:22:54 +0300 Message-ID: <2270313.kliK1kl0Jl@avalon> References: <20170731142906.GX31807@n2100.armlinux.org.uk> <20170802132730.GZ31807@n2100.armlinux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from galahad.ideasonboard.com (galahad.ideasonboard.com [185.26.127.97]) by gabe.freedesktop.org (Postfix) with ESMTPS id 65C8E6F16F for ; Wed, 2 Aug 2017 14:22:41 +0000 (UTC) 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: dri-devel@lists.freedesktop.org Cc: Hans Verkuil , Russell King - ARM Linux , linux-arm-kernel@lists.infradead.org List-Id: dri-devel@lists.freedesktop.org SGkgSGFucywKCk9uIFdlZG5lc2RheSAwMiBBdWcgMjAxNyAxNTozNDozMyBIYW5zIFZlcmt1aWwg d3JvdGU6Cj4gT24gMDgvMDIvMTcgMTU6MjcsIFJ1c3NlbGwgS2luZyAtIEFSTSBMaW51eCB3cm90 ZToKPiA+IE9uIFdlZCwgQXVnIDAyLCAyMDE3IGF0IDA0OjE0OjM0UE0gKzAzMDAsIExhdXJlbnQg UGluY2hhcnQgd3JvdGU6Cj4gPj4gT24gV2VkbmVzZGF5IDAyIEF1ZyAyMDE3IDA4OjQ3OjIzIEhh bnMgVmVya3VpbCB3cm90ZToKPiA+Pj4gT24gMDgvMDIvMjAxNyAxMjozMiBBTSwgTGF1cmVudCBQ aW5jaGFydCB3cm90ZToKPiA+Pj4+PiArCj4gPj4+Pj4gKwljZWNfcmVnaXN0ZXJfY2VjX25vdGlm aWVyKGNlYy0+YWRhcCwgY2VjLT5ub3RpZnkpOwo+ID4+Pj4+ICsKPiA+Pj4+PiArCXJldHVybiAw Owo+ID4+Pj4+ICt9Cj4gPj4+Pj4gKwo+ID4+Pj4+ICtzdGF0aWMgaW50IGR3X2hkbWlfY2VjX3Jl bW92ZShzdHJ1Y3QgcGxhdGZvcm1fZGV2aWNlICpwZGV2KQo+ID4+Pj4+ICt7Cj4gPj4+Pj4gKwlz dHJ1Y3QgZHdfaGRtaV9jZWMgKmNlYyA9IHBsYXRmb3JtX2dldF9kcnZkYXRhKHBkZXYpOwo+ID4+ Pj4+ICsKPiA+Pj4+PiArCWNlY191bnJlZ2lzdGVyX2FkYXB0ZXIoY2VjLT5hZGFwKTsKPiA+Pj4+ PiArCWNlY19ub3RpZmllcl9wdXQoY2VjLT5ub3RpZnkpOwo+ID4+Pj4+ICsKPiA+Pj4+PiArCXJl dHVybiAwOwo+ID4+Pj4+ICt9Cj4gPj4+Pj4gKwo+ID4+Pj4+ICtzdGF0aWMgc3RydWN0IHBsYXRm b3JtX2RyaXZlciBkd19oZG1pX2NlY19kcml2ZXIgPSB7Cj4gPj4+Pj4gKwkucHJvYmUJPSBkd19o ZG1pX2NlY19wcm9iZSwKPiA+Pj4+PiArCS5yZW1vdmUJPSBkd19oZG1pX2NlY19yZW1vdmUsCj4g Pj4+Pj4gKwkuZHJpdmVyID0gewo+ID4+Pj4+ICsJCS5uYW1lID0gImR3LWhkbWktY2VjIiwKPiA+ Pj4+PiArCX0sCj4gPj4+Pj4gK307Cj4gPj4+Pj4gK21vZHVsZV9wbGF0Zm9ybV9kcml2ZXIoZHdf aGRtaV9jZWNfZHJpdmVyKTsKPiA+Pj4+IAo+ID4+Pj4gSXMgdGhlcmUgYSBwYXJ0aWN1bGFyIHJl YXNvbiB3aHkgdGhpcyBoYXMgdG8gYmUgYSBzZXBhcmF0ZSBtb2R1bGUKPiA+Pj4+IGluc3RlYWQg b2Ygc2ltcGx5IGNhbGxpbmcgdGhlIENFQyBpbml0L2NsZWFudXAgZnVuY3Rpb25zIGRpcmVjdGx5 IGZyb20KPiA+Pj4+IHRoZSBtYWluIGR3LWhkbWkgZHJpdmVyID8KPiA+Pj4gCj4gPj4+IE5vdCBh bGwgU29DcyB0aGF0IHVzZSBkdy1oZG1pIGFsc28gdXNlIHRoZSBkdy1oZG1pIENFQyBpbXBsZW1l bnRhdGlvbi4KPiA+Pj4gU29tZSB1c2UgdGhlaXIgb3duIGltcGxlbWVudGF0aW9uIChhbWxvZ2lj KS4KPiA+PiAKPiA+PiBMb3ZlbHkuIE9mIGNvdXJzZSB3ZSBuZWVkIHRvIHJlaW52ZW50IHRoZSB3 aGVlbCBldmVyeSB0aW1lLCB3aGVyZSB3b3VsZAo+ID4+IHRoZSBmdW4gYmUgb3RoZXJ3aXNlID8K PiA+PiAKPiA+Pj4gU28gYnkgaW1wbGVtZW50aW5nIHRoZSBjZWMtbm90aWZpZXIgaW4gdGhlIGR3 LWhkbWkgZHJpdmVyIGFuZCBrZWVwaW5nCj4gPj4+IGR3LWhkbWkgQ0VDIHNlcGFyYXRlIHlvdSBj YW4gZWFzaWx5IGNob29zZSB3aGV0aGVyIG9yIG5vdCB5b3Ugd2FudCB0bwo+ID4+PiB1c2UgdGhp cyBDRUMgZHJpdmVyIG9yIGFub3RoZXIgU29DIENFQyBkcml2ZXIuCj4gPj4gCj4gPj4gSSdtIGNl cnRhaW5seSBmaW5lIHdpdGggc3VjaCBhIHNwbGl0LCBidXQgSSBkb24ndCB0aGluayBpdCByZXF1 aXJlcyBhCj4gPj4gc2VwYXJhdGUgcGxhdGZvcm1fZHJpdmVyLiBXZSBjb3VsZCB1c2UgYSBzaW1p bGFyIGFwcHJvYWNoIGFzIHdpdGggdGhlCj4gPj4gSERNSSBQSFkgdGhhdCBjYW4gYWxzbyBkaWZm ZXIgYmV0d2VlbiBTb0NzLiBUaGUgUEhZIGlzIGlkZW50aWZpZWQgYXQKPiA+PiBydW50aW1lIHdo ZW4gcG9zc2libGUsIGFuZCB0aGUgU29DLXNwZWNpZmljIGdsdWUgY29kZSBjYW4gb3ZlcnJpZGUg dGhhdAo+ID4+IHdpdGggYSBmZXcgZGF0YSBmaWVsZHMgYW5kIGZ1bmN0aW9uIHBvaW50ZXJzLgo+ ID4gCj4gPiBFeGN1c2UgbWUgaWYgSSBjb21wbGV0ZWx5IGxvc2UgaW50ZXJlc3QgaW4gcmV3b3Jr aW5nIHRoZSBkcml2ZXIgYXQgdGhpcwo+ID4gcG9pbnQsIGFzIGl0J3MgZW5vdWdoIG9mIGFuIGVm Zm9ydCB0byBmb2xsb3cgdGhlIGNodXJuIGluIENFQyBmcm9tIG9uZQo+ID4ga2VybmVsIHZlcnNp b24gdG8gYW5vdGhlci4gIEknbSBub3QgYWJvdXQgdG8gcmV3cml0ZSB0aGUgZHJpdmVyIGFuZAo+ ID4gcmVzdGFydCB0aGUgcmV2aWV3IGN5Y2xlIGZyb20gc2NyYXRjaCBhbmQgdGhlbiBoYXZlIHNl dmVyYWwgaXRlcmF0aW9ucwo+ID4gb2YgaGF2aW5nIHRvIHVwZGF0ZSBpdCBhcyBDRUMgY29udGlu dWVzIHRvIGV2b2x2ZS4KPiA+IAo+ID4gTGV0J3MgZ2V0IHRoaXMgZHJpdmVyIGluIG1haW5saW5l LCBhbmQgdGhlbiBpZiB3ZSB3YW50IGZ1cnRoZXIgY2hhbmdlcwo+ID4gd2UgY2FuIGRvIHRoYXQg bGF0ZXIuCj4gCj4gKzEKPiAKPiBJIGFsc28gZG9uJ3QgdGhpbmsgdGhlIHBoeSBpZGVhIGFwcGxp ZXMgaGVyZS4gQ0VDIGltcGxlbWVudGF0aW9ucyByYW5nZSBmcm9tCj4gdGlnaHRseSBjb3VwbGVk IHRvIHRoZSBIRE1JIGltcGxlbWVudGF0aW9uIHRvIGEgY29tcGxldGVseSBpbmRlcGVuZGVudCBp MmMKPiBkZXZpY2UgdGhhdCBpcyB1bnJlbGF0ZWQgdG8gYW55IEhETUkgcmVjZWl2ZXIvdHJhbnNt aXR0ZXIuIEFuZCB2YXJpb3VzCj4gc2hhZGVzIGluIGJldHdlZW4gdGhvc2UgZXh0cmVtZXMuCgpU aGVuIGl0IHNob3VsZCBiZSBldmVuIHNpbXBsZXIuIEluc3RlYWQgb2YgY3JlYXRpbmcgYSBzZXBh cmF0ZSBwbGF0Zm9ybSBkZXZpY2UgCmZvciB0aGUgRFcgSERNSSBDRUMsIHdlIGNhbiBqdXN0IGNh bGwgdGhlIERXIEhETUkgQ0VDIGRyaXZlciBpbml0L2NsZWFudXAgCmZ1bmN0aW9ucyBkaXJlY3Rs eSB3aGVuIHRoZSBJUCBjb3JlIGltcGxlbWVudHMgQ0VDLiBXaGVuIGl0IGRvZXNuJ3QsIGEgCnNl cGFyYXRlIGRldmljZSB3aWxsIGJlIGRlc2NyaWJlZCBpbiBEVCAoYXQgbGVhc3QgdG8gbXkgdW5k ZXJzdGFuZGluZywgCm90aGVyd2lzZSB3aGVyZSB3b3VsZCBpdCBjb21lIGZyb20gPykgYW5kIGEg c2VwYXJhdGUgZHJpdmVyIHdvdWxkIGhhbmRsZSBpdC4KCj4gVGhlIG9ubHkgdGhpbmcgdGhhdCBp cyBuZWVkZWQgaXMgdGhhdCB0aGUgQ0VDIGRldmljZSBuZWVkcyB0byBiZSBpbmZvcm1lZAo+IHdo ZW4gYW4gRURJRCBpcyByZWFkIGFuZCB3aGVuIHRoZSBob3RwbHVnIGRpc2FwcGVhcnMuIFRoZSBD RUMgbm90aWZpZXIgZG9lcwo+IGp1c3QgdGhhdCBhbmQgaXQgaXMgYWxyZWFkeSBpbiB1c2UsIHNv IGl0IGlzIG5vdGhpbmcgbmV3LiBObyBuZWVkIHRvIHJld29yawo+IHRoaXMuCgpUaGUgbm90aWZp ZXIgaW1wbGVtZW50YXRpb24gaW4gcGF0Y2ggMS80IGxvb2tzIGdvb2QgdG8gbWUgKGV4Y2VwdCBm b3IgdGhlIAptaXNzaW5nIGNlY19ub3RpZmllcl9wdXQoKSBjYWxsIHRoYXQgSSBtZW50aW9uZWQg aW4gbXkgcmV2aWV3KS4KCi0tIApSZWdhcmRzLAoKTGF1cmVudCBQaW5jaGFydAoKX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxpbmcg bGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRl c2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg==