From mboxrd@z Thu Jan 1 00:00:00 1970 From: laurent.pinchart@ideasonboard.com (Laurent Pinchart) Date: Fri, 20 Apr 2018 13:18:13 +0300 Subject: [PATCH v3 0/7] Add tda998x (HDMI) support to atmel-hlcdc In-Reply-To: <20180420085235.GI4235@w540> References: <20180419162751.25223-1-peda@axentia.se> <20180420085235.GI4235@w540> Message-ID: <6354350.l8DgUyNhLT@avalon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hello, On Friday, 20 April 2018 11:52:35 EEST jacopo mondi wrote: > Hi Peter, > I've been a bit a pain in the arse for you recently, but please > bear with me a bit more, and sorry for jumping late on the band wagon. > > On Thu, Apr 19, 2018 at 06:27:44PM +0200, Peter Rosin wrote: > > Hi! > > > > I naively thought that since there was support for both nxp,tda19988 (in > > the tda998x driver) and the atmel-hlcdc, things would be a smooth ride. > > But it wasn't, so I started looking around and realized I had to fix > > things. > > > > In v1 and v2 I fixed things by making the atmel-hlcdc driver a master > > component, but now in v3 I fix things by making the tda998x driver > > a bridge instead. This was after a suggestion from Boris Brezillion > > (the atmel-hlcdc maintainer), so there was some risk of bias ... but > > after comparing what was needed, I too find the bridge approach better. > > > > In addition to the above, our PCB interface between the SAMA5D3 and the > > HDMI encoder is only using 16 bits, and this has to be described > > somewhere, or the atmel-hlcdc driver have no chance of selecting the > > correct output mode. Since I have similar problems with a ds90c185 lvds > > encoder I added patches to override the atmel-hlcdc output format via > > DT properties compatible with the media video-interface binding and > > things start to play together. > > > > Since this series superseeds the bridge series [1], I have included the > > leftover bindings patch for the ti,ds90c185 here. > > I feel like this series would look better if it would make use of the > proposed bridge media bus format support I have recently sent out [1] > (and which was not there when you first sent v1). > > I understand your fundamental problem here is that the bus format > that should be reported by your bridge is different from the ones > actually supported by the TDA19988 chip, as the wirings ground some > of the input pins. > > Although this is defintely something that could be described in the > bridge's own OF node with the 'bus_width' property, and what I would do, > now that you have made a bridge out from the tda19988 driver, is: > > 1) Set the bridge accepted input bus_format parsing its pin > configuration, or default it if that's not implemented yet. > This will likely be rgb888. You can do that using the trivial > support for bridge input image formats implemented by my series. > 2) Specify in the bridge endpoint a 'bus_width' of <16> > 3) In your atmel-hlcd get both the image format of the bridge (rgb888) > and parse the remote endpoint property 'bus_width' and get the <16> > value back. Parsing properties of remote nodes should be avoided as much as possible, as they need to be interpreted in the context of the DT bindings related to the compatible string applicable to that node. I'd rather have the bus_width property in the local endpoint node. > 4) Set the correct format in the atmel hlcd driver to accommodate a > bridge that wants rgb888 on a 16 bit wide bus (that would be rgb565, > or are there other possible combinations I am missing?) > > I would consider this better mostly because in this series you are > creating a semantic for the whole DRM subsystem on the 'bus_width' > property, where numerical values as '12', '16' etc are arbitrary tied > to the selection of a media bus format. At least you should use a > common set of defines [1] between the device tree and the driver, > but this looks anyway fragile imho. This I agree with though. Combining the remote bus format with the local bus width should fix the problem without having to parse remote properties. > Have I maybe missed some parts of the problem you are trying to solve > here? > > Thank you > j > > [1] drm: bridge: Add support for static image formats > https://lwn.net/Articles/752296/ > [2] include/uapi/linux/media-bus-format.h > > > Anyway, this series solves some real issues for my HW. > > > > Cheers, > > Peter > > > > Changes since v2 https://lkml.org/lkml/2018/4/17/385 > > - patch 2/7 fixed spelling and added an example > > - patch 4/7 parse the DT up front and store the result indexed by encoder > > - old patch 5/6 and 6/6 dropped > > - patch 5-7/7 are new and makes the tda998x driver a drm_bridge > > > > Changes since v1 https://lkml.org/lkml/2018/4/9/294 > > - added reviewed-by from Rob to patch 1/6 > > - patch 2/6 changed so that the bus format override is in the endpoint > > DT node, and follows the binding of media video-interfaces. > > - patch 3/6 is new, it adds drm_of_media_bus_fmt which parses above > > media video-interface binding (partially). > > - patch 4/6 now makes use of the above helper (and also fixes problems > > with the 3/5 patch from v1 when no override was specified). > > - do not mention unrelated connector display_info details in the cover > > letter and commit messages. > > > > [1] > > "Bridge" series v2 https://lkml.org/lkml/2018/3/26/610 > > "Bridge" series v1 https://lkml.org/lkml/2018/3/17/221 > > > > Peter Rosin (7): > > dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185 > > dt-bindings: display: atmel: optional video-interface of endpoints > > drm: of: introduce drm_of_media_bus_fmt > > drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes > > drm/i2c: tda998x: find the drm_device via the drm_connector > > drm/i2c: tda998x: split encoder and component functions from the work > > drm/i2c: tda998x: register as a drm bridge > > > > .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 +++ > > .../bindings/display/bridge/lvds-transmitter.txt | 8 +- > > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 71 ++++++-- > > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 2 + > > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 40 +++- > > drivers/gpu/drm/drm_of.c | 38 ++++ > > drivers/gpu/drm/i2c/tda998x_drv.c | 201 ++++++++++++++-- > > include/drm/drm_of.h | 7 + > > 8 files changed, 342 insertions(+), 51 deletions(-) -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH v3 0/7] Add tda998x (HDMI) support to atmel-hlcdc Date: Fri, 20 Apr 2018 13:18:13 +0300 Message-ID: <6354350.l8DgUyNhLT@avalon> References: <20180419162751.25223-1-peda@axentia.se> <20180420085235.GI4235@w540> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20180420085235.GI4235@w540> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: jacopo mondi Cc: Mark Rutland , Boris Brezillon , Alexandre Belloni , devicetree@vger.kernel.org, David Airlie , Nicolas Ferre , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Rob Herring , Jacopo Mondi , Daniel Vetter , Peter Rosin , Russell King , linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org SGVsbG8sCgpPbiBGcmlkYXksIDIwIEFwcmlsIDIwMTggMTE6NTI6MzUgRUVTVCBqYWNvcG8gbW9u ZGkgd3JvdGU6Cj4gSGkgUGV0ZXIsCj4gICAgIEkndmUgYmVlbiBhIGJpdCBhIHBhaW4gaW4gdGhl IGFyc2UgZm9yIHlvdSByZWNlbnRseSwgYnV0IHBsZWFzZQo+IGJlYXIgd2l0aCBtZSBhIGJpdCBt b3JlLCBhbmQgc29ycnkgZm9yIGp1bXBpbmcgbGF0ZSBvbiB0aGUgYmFuZCB3YWdvbi4KPiAKPiBP biBUaHUsIEFwciAxOSwgMjAxOCBhdCAwNjoyNzo0NFBNICswMjAwLCBQZXRlciBSb3NpbiB3cm90 ZToKPiA+IEhpIQo+ID4gCj4gPiBJIG5haXZlbHkgdGhvdWdodCB0aGF0IHNpbmNlIHRoZXJlIHdh cyBzdXBwb3J0IGZvciBib3RoIG54cCx0ZGExOTk4OCAoaW4KPiA+IHRoZSB0ZGE5OTh4IGRyaXZl cikgYW5kIHRoZSBhdG1lbC1obGNkYywgdGhpbmdzIHdvdWxkIGJlIGEgc21vb3RoIHJpZGUuCj4g PiBCdXQgaXQgd2Fzbid0LCBzbyBJIHN0YXJ0ZWQgbG9va2luZyBhcm91bmQgYW5kIHJlYWxpemVk IEkgaGFkIHRvIGZpeAo+ID4gdGhpbmdzLgo+ID4gCj4gPiBJbiB2MSBhbmQgdjIgSSBmaXhlZCB0 aGluZ3MgYnkgbWFraW5nIHRoZSBhdG1lbC1obGNkYyBkcml2ZXIgYSBtYXN0ZXIKPiA+IGNvbXBv bmVudCwgYnV0IG5vdyBpbiB2MyBJIGZpeCB0aGluZ3MgYnkgbWFraW5nIHRoZSB0ZGE5OTh4IGRy aXZlcgo+ID4gYSBicmlkZ2UgaW5zdGVhZC4gVGhpcyB3YXMgYWZ0ZXIgYSBzdWdnZXN0aW9uIGZy b20gQm9yaXMgQnJlemlsbGlvbgo+ID4gKHRoZSBhdG1lbC1obGNkYyBtYWludGFpbmVyKSwgc28g dGhlcmUgd2FzIHNvbWUgcmlzayBvZiBiaWFzIC4uLiBidXQKPiA+IGFmdGVyIGNvbXBhcmluZyB3 aGF0IHdhcyBuZWVkZWQsIEkgdG9vIGZpbmQgdGhlIGJyaWRnZSBhcHByb2FjaCBiZXR0ZXIuCj4g PiAKPiA+IEluIGFkZGl0aW9uIHRvIHRoZSBhYm92ZSwgb3VyIFBDQiBpbnRlcmZhY2UgYmV0d2Vl biB0aGUgU0FNQTVEMyBhbmQgdGhlCj4gPiBIRE1JIGVuY29kZXIgaXMgb25seSB1c2luZyAxNiBi aXRzLCBhbmQgdGhpcyBoYXMgdG8gYmUgZGVzY3JpYmVkCj4gPiBzb21ld2hlcmUsIG9yIHRoZSBh dG1lbC1obGNkYyBkcml2ZXIgaGF2ZSBubyBjaGFuY2Ugb2Ygc2VsZWN0aW5nIHRoZQo+ID4gY29y cmVjdCBvdXRwdXQgbW9kZS4gU2luY2UgSSBoYXZlIHNpbWlsYXIgcHJvYmxlbXMgd2l0aCBhIGRz OTBjMTg1IGx2ZHMKPiA+IGVuY29kZXIgSSBhZGRlZCBwYXRjaGVzIHRvIG92ZXJyaWRlIHRoZSBh dG1lbC1obGNkYyBvdXRwdXQgZm9ybWF0IHZpYQo+ID4gRFQgcHJvcGVydGllcyBjb21wYXRpYmxl IHdpdGggdGhlIG1lZGlhIHZpZGVvLWludGVyZmFjZSBiaW5kaW5nIGFuZAo+ID4gdGhpbmdzIHN0 YXJ0IHRvIHBsYXkgdG9nZXRoZXIuCj4gPiAKPiA+IFNpbmNlIHRoaXMgc2VyaWVzIHN1cGVyc2Vl ZHMgdGhlIGJyaWRnZSBzZXJpZXMgWzFdLCBJIGhhdmUgaW5jbHVkZWQgdGhlCj4gPiBsZWZ0b3Zl ciBiaW5kaW5ncyBwYXRjaCBmb3IgdGhlIHRpLGRzOTBjMTg1IGhlcmUuCj4gCj4gSSBmZWVsIGxp a2UgdGhpcyBzZXJpZXMgd291bGQgbG9vayBiZXR0ZXIgaWYgaXQgd291bGQgbWFrZSB1c2Ugb2Yg dGhlCj4gcHJvcG9zZWQgYnJpZGdlIG1lZGlhIGJ1cyBmb3JtYXQgc3VwcG9ydCBJIGhhdmUgcmVj ZW50bHkgc2VudCBvdXQgWzFdCj4gKGFuZCB3aGljaCB3YXMgbm90IHRoZXJlIHdoZW4geW91IGZp cnN0IHNlbnQgdjEpLgo+IAo+IEkgdW5kZXJzdGFuZCB5b3VyIGZ1bmRhbWVudGFsIHByb2JsZW0g aGVyZSBpcyB0aGF0IHRoZSBidXMgZm9ybWF0Cj4gdGhhdCBzaG91bGQgYmUgcmVwb3J0ZWQgYnkg eW91ciBicmlkZ2UgaXMgZGlmZmVyZW50IGZyb20gdGhlIG9uZXMKPiBhY3R1YWxseSBzdXBwb3J0 ZWQgYnkgdGhlIFREQTE5OTg4IGNoaXAsIGFzIHRoZSB3aXJpbmdzIGdyb3VuZCBzb21lCj4gb2Yg dGhlIGlucHV0IHBpbnMuCj4gCj4gQWx0aG91Z2ggdGhpcyBpcyBkZWZpbnRlbHkgc29tZXRoaW5n IHRoYXQgY291bGQgYmUgZGVzY3JpYmVkIGluIHRoZQo+IGJyaWRnZSdzIG93biBPRiBub2RlIHdp dGggdGhlICdidXNfd2lkdGgnIHByb3BlcnR5LCBhbmQgd2hhdCBJIHdvdWxkIGRvLAo+IG5vdyB0 aGF0IHlvdSBoYXZlIG1hZGUgYSBicmlkZ2Ugb3V0IGZyb20gdGhlIHRkYTE5OTg4IGRyaXZlciwg aXM6Cj4gCj4gMSkgU2V0IHRoZSBicmlkZ2UgYWNjZXB0ZWQgaW5wdXQgYnVzX2Zvcm1hdCBwYXJz aW5nIGl0cyBwaW4KPiBjb25maWd1cmF0aW9uLCBvciBkZWZhdWx0IGl0IGlmIHRoYXQncyBub3Qg aW1wbGVtZW50ZWQgeWV0Lgo+IFRoaXMgd2lsbCBsaWtlbHkgYmUgcmdiODg4LiBZb3UgY2FuIGRv IHRoYXQgdXNpbmcgdGhlIHRyaXZpYWwKPiBzdXBwb3J0IGZvciBicmlkZ2UgaW5wdXQgaW1hZ2Ug Zm9ybWF0cyBpbXBsZW1lbnRlZCBieSBteSBzZXJpZXMuCj4gMikgU3BlY2lmeSBpbiB0aGUgYnJp ZGdlIGVuZHBvaW50IGEgJ2J1c193aWR0aCcgb2YgPDE2Pgo+IDMpIEluIHlvdXIgYXRtZWwtaGxj ZCBnZXQgYm90aCB0aGUgaW1hZ2UgZm9ybWF0IG9mIHRoZSBicmlkZ2UgKHJnYjg4OCkKPiBhbmQg cGFyc2UgdGhlIHJlbW90ZSBlbmRwb2ludCBwcm9wZXJ0eSAnYnVzX3dpZHRoJyBhbmQgZ2V0IHRo ZSA8MTY+Cj4gdmFsdWUgYmFjay4KClBhcnNpbmcgcHJvcGVydGllcyBvZiByZW1vdGUgbm9kZXMg c2hvdWxkIGJlIGF2b2lkZWQgYXMgbXVjaCBhcyBwb3NzaWJsZSwgYXMgCnRoZXkgbmVlZCB0byBi ZSBpbnRlcnByZXRlZCBpbiB0aGUgY29udGV4dCBvZiB0aGUgRFQgYmluZGluZ3MgcmVsYXRlZCB0 byB0aGUgCmNvbXBhdGlibGUgc3RyaW5nIGFwcGxpY2FibGUgdG8gdGhhdCBub2RlLiBJJ2QgcmF0 aGVyIGhhdmUgdGhlIGJ1c193aWR0aCAKcHJvcGVydHkgaW4gdGhlIGxvY2FsIGVuZHBvaW50IG5v ZGUuCgo+IDQpIFNldCB0aGUgY29ycmVjdCBmb3JtYXQgaW4gdGhlIGF0bWVsIGhsY2QgZHJpdmVy IHRvIGFjY29tbW9kYXRlIGEKPiBicmlkZ2UgdGhhdCB3YW50cyByZ2I4ODggb24gYSAxNiBiaXQg d2lkZSBidXMgKHRoYXQgd291bGQgYmUgcmdiNTY1LAo+IG9yIGFyZSB0aGVyZSBvdGhlciBwb3Nz aWJsZSBjb21iaW5hdGlvbnMgSSBhbSBtaXNzaW5nPykKPiAKPiBJIHdvdWxkIGNvbnNpZGVyIHRo aXMgYmV0dGVyIG1vc3RseSBiZWNhdXNlIGluIHRoaXMgc2VyaWVzIHlvdSBhcmUKPiBjcmVhdGlu ZyBhIHNlbWFudGljIGZvciB0aGUgd2hvbGUgRFJNIHN1YnN5c3RlbSBvbiB0aGUgJ2J1c193aWR0 aCcKPiBwcm9wZXJ0eSwgd2hlcmUgbnVtZXJpY2FsIHZhbHVlcyBhcyAnMTInLCAnMTYnIGV0YyBh cmUgYXJiaXRyYXJ5IHRpZWQKPiB0byB0aGUgc2VsZWN0aW9uIG9mIGEgbWVkaWEgYnVzIGZvcm1h dC4gQXQgbGVhc3QgeW91IHNob3VsZCB1c2UgYQo+IGNvbW1vbiBzZXQgb2YgZGVmaW5lcyBbMV0g YmV0d2VlbiB0aGUgZGV2aWNlIHRyZWUgYW5kIHRoZSBkcml2ZXIsCj4gYnV0IHRoaXMgbG9va3Mg YW55d2F5IGZyYWdpbGUgaW1oby4KClRoaXMgSSBhZ3JlZSB3aXRoIHRob3VnaC4gQ29tYmluaW5n IHRoZSByZW1vdGUgYnVzIGZvcm1hdCB3aXRoIHRoZSBsb2NhbCBidXMgCndpZHRoIHNob3VsZCBm aXggdGhlIHByb2JsZW0gd2l0aG91dCBoYXZpbmcgdG8gcGFyc2UgcmVtb3RlIHByb3BlcnRpZXMu Cgo+IEhhdmUgSSBtYXliZSBtaXNzZWQgc29tZSBwYXJ0cyBvZiB0aGUgcHJvYmxlbSB5b3UgYXJl IHRyeWluZyB0byBzb2x2ZQo+IGhlcmU/Cj4gCj4gVGhhbmsgeW91Cj4gICAgago+IAo+IFsxXSBk cm06IGJyaWRnZTogQWRkIHN1cHBvcnQgZm9yIHN0YXRpYyBpbWFnZSBmb3JtYXRzCj4gICAgIGh0 dHBzOi8vbHduLm5ldC9BcnRpY2xlcy83NTIyOTYvCj4gWzJdIGluY2x1ZGUvdWFwaS9saW51eC9t ZWRpYS1idXMtZm9ybWF0LmgKPiAKPiA+IEFueXdheSwgdGhpcyBzZXJpZXMgc29sdmVzIHNvbWUg cmVhbCBpc3N1ZXMgZm9yIG15IEhXLgo+ID4gCj4gPiBDaGVlcnMsCj4gPiBQZXRlcgo+ID4gCj4g PiBDaGFuZ2VzIHNpbmNlIHYyICAgaHR0cHM6Ly9sa21sLm9yZy9sa21sLzIwMTgvNC8xNy8zODUK PiA+IC0gcGF0Y2ggMi83IGZpeGVkIHNwZWxsaW5nIGFuZCBhZGRlZCBhbiBleGFtcGxlCj4gPiAt IHBhdGNoIDQvNyBwYXJzZSB0aGUgRFQgdXAgZnJvbnQgYW5kIHN0b3JlIHRoZSByZXN1bHQgaW5k ZXhlZCBieSBlbmNvZGVyCj4gPiAtIG9sZCBwYXRjaCA1LzYgYW5kIDYvNiBkcm9wcGVkCj4gPiAt IHBhdGNoIDUtNy83IGFyZSBuZXcgYW5kIG1ha2VzIHRoZSB0ZGE5OTh4IGRyaXZlciBhIGRybV9i cmlkZ2UKPiA+IAo+ID4gQ2hhbmdlcyBzaW5jZSB2MSAgIGh0dHBzOi8vbGttbC5vcmcvbGttbC8y MDE4LzQvOS8yOTQKPiA+IC0gYWRkZWQgcmV2aWV3ZWQtYnkgZnJvbSBSb2IgdG8gcGF0Y2ggMS82 Cj4gPiAtIHBhdGNoIDIvNiBjaGFuZ2VkIHNvIHRoYXQgdGhlIGJ1cyBmb3JtYXQgb3ZlcnJpZGUg aXMgaW4gdGhlIGVuZHBvaW50Cj4gPiAgIERUIG5vZGUsIGFuZCBmb2xsb3dzIHRoZSBiaW5kaW5n IG9mIG1lZGlhIHZpZGVvLWludGVyZmFjZXMuCj4gPiAtIHBhdGNoIDMvNiBpcyBuZXcsIGl0IGFk ZHMgZHJtX29mX21lZGlhX2J1c19mbXQgd2hpY2ggcGFyc2VzIGFib3ZlCj4gPiAgIG1lZGlhIHZp ZGVvLWludGVyZmFjZSBiaW5kaW5nIChwYXJ0aWFsbHkpLgo+ID4gLSBwYXRjaCA0LzYgbm93IG1h a2VzIHVzZSBvZiB0aGUgYWJvdmUgaGVscGVyIChhbmQgYWxzbyBmaXhlcyBwcm9ibGVtcwo+ID4g ICB3aXRoIHRoZSAzLzUgcGF0Y2ggZnJvbSB2MSB3aGVuIG5vIG92ZXJyaWRlIHdhcyBzcGVjaWZp ZWQpLgo+ID4gLSBkbyBub3QgbWVudGlvbiB1bnJlbGF0ZWQgY29ubmVjdG9yIGRpc3BsYXlfaW5m byBkZXRhaWxzIGluIHRoZSBjb3Zlcgo+ID4gICBsZXR0ZXIgYW5kIGNvbW1pdCBtZXNzYWdlcy4K PiA+IAo+ID4gWzFdCj4gPiAiQnJpZGdlIiBzZXJpZXMgdjIgICBodHRwczovL2xrbWwub3JnL2xr bWwvMjAxOC8zLzI2LzYxMAo+ID4gIkJyaWRnZSIgc2VyaWVzIHYxICAgaHR0cHM6Ly9sa21sLm9y Zy9sa21sLzIwMTgvMy8xNy8yMjEKPiA+IAo+ID4gUGV0ZXIgUm9zaW4gKDcpOgo+ID4gICBkdC1i aW5kaW5nczogZGlzcGxheTogYnJpZGdlOiBsdmRzLXRyYW5zbWl0dGVyOiBhZGQgdGksZHM5MGMx ODUKPiA+ICAgZHQtYmluZGluZ3M6IGRpc3BsYXk6IGF0bWVsOiBvcHRpb25hbCB2aWRlby1pbnRl cmZhY2Ugb2YgZW5kcG9pbnRzCj4gPiAgIGRybTogb2Y6IGludHJvZHVjZSBkcm1fb2ZfbWVkaWFf YnVzX2ZtdAo+ID4gICBkcm0vYXRtZWwtaGxjZGM6IHN1cHBvcnQgYnVzLXdpZHRoICgxMi8xNi8x OC8yNCkgaW4gZW5kcG9pbnQgbm9kZXMKPiA+ICAgZHJtL2kyYzogdGRhOTk4eDogZmluZCB0aGUg ZHJtX2RldmljZSB2aWEgdGhlIGRybV9jb25uZWN0b3IKPiA+ICAgZHJtL2kyYzogdGRhOTk4eDog c3BsaXQgZW5jb2RlciBhbmQgY29tcG9uZW50IGZ1bmN0aW9ucyBmcm9tIHRoZSB3b3JrCj4gPiAg IGRybS9pMmM6IHRkYTk5OHg6IHJlZ2lzdGVyIGFzIGEgZHJtIGJyaWRnZQo+ID4gIAo+ID4gIC4u Li9kZXZpY2V0cmVlL2JpbmRpbmdzL2Rpc3BsYXkvYXRtZWwvaGxjZGMtZGMudHh0IHwgIDI2ICsr Kwo+ID4gIC4uLi9iaW5kaW5ncy9kaXNwbGF5L2JyaWRnZS9sdmRzLXRyYW5zbWl0dGVyLnR4dCAg IHwgICA4ICstCj4gPiAgZHJpdmVycy9ncHUvZHJtL2F0bWVsLWhsY2RjL2F0bWVsX2hsY2RjX2Ny dGMuYyAgICAgfCAgNzEgKysrKysrLS0KPiA+ICBkcml2ZXJzL2dwdS9kcm0vYXRtZWwtaGxjZGMv YXRtZWxfaGxjZGNfZGMuaCAgICAgICB8ICAgMiArCj4gPiAgZHJpdmVycy9ncHUvZHJtL2F0bWVs LWhsY2RjL2F0bWVsX2hsY2RjX291dHB1dC5jICAgfCAgNDAgKysrLQo+ID4gIGRyaXZlcnMvZ3B1 L2RybS9kcm1fb2YuYyAgICAgICAgICAgICAgICAgICAgICAgICAgIHwgIDM4ICsrKysKPiA+ICBk cml2ZXJzL2dwdS9kcm0vaTJjL3RkYTk5OHhfZHJ2LmMgICAgICAgICAgICAgICAgICB8IDIwMSAr KysrKysrKysrKysrKy0tCj4gPiAgaW5jbHVkZS9kcm0vZHJtX29mLmggICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgfCAgIDcgKwo+ID4gIDggZmlsZXMgY2hhbmdlZCwgMzQyIGluc2VydGlv bnMoKyksIDUxIGRlbGV0aW9ucygtKQoKLS0gClJlZ2FyZHMsCgpMYXVyZW50IFBpbmNoYXJ0CgoK Cl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZl bCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xp c3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754587AbeDTKSI (ORCPT ); Fri, 20 Apr 2018 06:18:08 -0400 Received: from perceval.ideasonboard.com ([213.167.242.64]:39772 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754469AbeDTKSG (ORCPT ); Fri, 20 Apr 2018 06:18:06 -0400 From: Laurent Pinchart To: jacopo mondi Cc: Peter Rosin , linux-kernel@vger.kernel.org, David Airlie , Rob Herring , Mark Rutland , Nicolas Ferre , Alexandre Belloni , Boris Brezillon , Daniel Vetter , Gustavo Padovan , Sean Paul , Russell King , Jacopo Mondi , dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v3 0/7] Add tda998x (HDMI) support to atmel-hlcdc Date: Fri, 20 Apr 2018 13:18:13 +0300 Message-ID: <6354350.l8DgUyNhLT@avalon> Organization: Ideas on Board Oy In-Reply-To: <20180420085235.GI4235@w540> References: <20180419162751.25223-1-peda@axentia.se> <20180420085235.GI4235@w540> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello, On Friday, 20 April 2018 11:52:35 EEST jacopo mondi wrote: > Hi Peter, > I've been a bit a pain in the arse for you recently, but please > bear with me a bit more, and sorry for jumping late on the band wagon. > > On Thu, Apr 19, 2018 at 06:27:44PM +0200, Peter Rosin wrote: > > Hi! > > > > I naively thought that since there was support for both nxp,tda19988 (in > > the tda998x driver) and the atmel-hlcdc, things would be a smooth ride. > > But it wasn't, so I started looking around and realized I had to fix > > things. > > > > In v1 and v2 I fixed things by making the atmel-hlcdc driver a master > > component, but now in v3 I fix things by making the tda998x driver > > a bridge instead. This was after a suggestion from Boris Brezillion > > (the atmel-hlcdc maintainer), so there was some risk of bias ... but > > after comparing what was needed, I too find the bridge approach better. > > > > In addition to the above, our PCB interface between the SAMA5D3 and the > > HDMI encoder is only using 16 bits, and this has to be described > > somewhere, or the atmel-hlcdc driver have no chance of selecting the > > correct output mode. Since I have similar problems with a ds90c185 lvds > > encoder I added patches to override the atmel-hlcdc output format via > > DT properties compatible with the media video-interface binding and > > things start to play together. > > > > Since this series superseeds the bridge series [1], I have included the > > leftover bindings patch for the ti,ds90c185 here. > > I feel like this series would look better if it would make use of the > proposed bridge media bus format support I have recently sent out [1] > (and which was not there when you first sent v1). > > I understand your fundamental problem here is that the bus format > that should be reported by your bridge is different from the ones > actually supported by the TDA19988 chip, as the wirings ground some > of the input pins. > > Although this is defintely something that could be described in the > bridge's own OF node with the 'bus_width' property, and what I would do, > now that you have made a bridge out from the tda19988 driver, is: > > 1) Set the bridge accepted input bus_format parsing its pin > configuration, or default it if that's not implemented yet. > This will likely be rgb888. You can do that using the trivial > support for bridge input image formats implemented by my series. > 2) Specify in the bridge endpoint a 'bus_width' of <16> > 3) In your atmel-hlcd get both the image format of the bridge (rgb888) > and parse the remote endpoint property 'bus_width' and get the <16> > value back. Parsing properties of remote nodes should be avoided as much as possible, as they need to be interpreted in the context of the DT bindings related to the compatible string applicable to that node. I'd rather have the bus_width property in the local endpoint node. > 4) Set the correct format in the atmel hlcd driver to accommodate a > bridge that wants rgb888 on a 16 bit wide bus (that would be rgb565, > or are there other possible combinations I am missing?) > > I would consider this better mostly because in this series you are > creating a semantic for the whole DRM subsystem on the 'bus_width' > property, where numerical values as '12', '16' etc are arbitrary tied > to the selection of a media bus format. At least you should use a > common set of defines [1] between the device tree and the driver, > but this looks anyway fragile imho. This I agree with though. Combining the remote bus format with the local bus width should fix the problem without having to parse remote properties. > Have I maybe missed some parts of the problem you are trying to solve > here? > > Thank you > j > > [1] drm: bridge: Add support for static image formats > https://lwn.net/Articles/752296/ > [2] include/uapi/linux/media-bus-format.h > > > Anyway, this series solves some real issues for my HW. > > > > Cheers, > > Peter > > > > Changes since v2 https://lkml.org/lkml/2018/4/17/385 > > - patch 2/7 fixed spelling and added an example > > - patch 4/7 parse the DT up front and store the result indexed by encoder > > - old patch 5/6 and 6/6 dropped > > - patch 5-7/7 are new and makes the tda998x driver a drm_bridge > > > > Changes since v1 https://lkml.org/lkml/2018/4/9/294 > > - added reviewed-by from Rob to patch 1/6 > > - patch 2/6 changed so that the bus format override is in the endpoint > > DT node, and follows the binding of media video-interfaces. > > - patch 3/6 is new, it adds drm_of_media_bus_fmt which parses above > > media video-interface binding (partially). > > - patch 4/6 now makes use of the above helper (and also fixes problems > > with the 3/5 patch from v1 when no override was specified). > > - do not mention unrelated connector display_info details in the cover > > letter and commit messages. > > > > [1] > > "Bridge" series v2 https://lkml.org/lkml/2018/3/26/610 > > "Bridge" series v1 https://lkml.org/lkml/2018/3/17/221 > > > > Peter Rosin (7): > > dt-bindings: display: bridge: lvds-transmitter: add ti,ds90c185 > > dt-bindings: display: atmel: optional video-interface of endpoints > > drm: of: introduce drm_of_media_bus_fmt > > drm/atmel-hlcdc: support bus-width (12/16/18/24) in endpoint nodes > > drm/i2c: tda998x: find the drm_device via the drm_connector > > drm/i2c: tda998x: split encoder and component functions from the work > > drm/i2c: tda998x: register as a drm bridge > > > > .../devicetree/bindings/display/atmel/hlcdc-dc.txt | 26 +++ > > .../bindings/display/bridge/lvds-transmitter.txt | 8 +- > > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 71 ++++++-- > > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 2 + > > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 40 +++- > > drivers/gpu/drm/drm_of.c | 38 ++++ > > drivers/gpu/drm/i2c/tda998x_drv.c | 201 ++++++++++++++-- > > include/drm/drm_of.h | 7 + > > 8 files changed, 342 insertions(+), 51 deletions(-) -- Regards, Laurent Pinchart