From mboxrd@z Thu Jan 1 00:00:00 1970 From: laurent.pinchart@ideasonboard.com (Laurent Pinchart) Date: Sat, 21 Apr 2018 11:38:00 +0300 Subject: [PATCH v3 0/7] Add tda998x (HDMI) support to atmel-hlcdc In-Reply-To: <840625e9-42ff-49b7-605a-202b4d980beb@axentia.se> References: <20180419162751.25223-1-peda@axentia.se> <20180420113859.GL4235@w540> <840625e9-42ff-49b7-605a-202b4d980beb@axentia.se> Message-ID: <1549346.R82tLOL7eo@avalon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Peter, On Friday, 20 April 2018 15:55:50 EEST Peter Rosin wrote: > On 2018-04-20 13:38, jacopo mondi wrote: > > On Fri, Apr 20, 2018 at 01:05:21PM +0200, Peter Rosin wrote: > >> On 2018-04-20 12:18, Laurent Pinchart wrote: > >>> 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 > >> > >> That should be Brezillon, sorry for being sloppy with the spelling. > >> > >>>>> (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. > >> > >> In addition to that, my view of this binding > >> > >> endpoint { > >> bus-type = <0>; > > > > bus-type is used by v4l2_fwnode helpers to decide which type of bus > > they're dealing with iirc. Here it seems to me it has no added > > value, also because in your bindings description "it shall be 0" and > > it is not parsed anywhere in you code, but no big deal.... > > bus-type is indeed parsed and verified to be zero which means auto-detect > according to the video-interfaces binding. An auto-detect bus-type with > a given bus-width means a parallel interface IIUC. I believe you could leave it out though. Your display controller only supports parallel buses, so there's no need to specify the bus type explicitly. > From patch 3/7: > > if (of_property_read_u32(node, "bus-type", &bus_type)) > return 0; > if (bus_type != 0) > return -EINVAL; > > >> bus-widht = <16>; > >> }; > >> > >> is that it always means rgb565. See further below. > >> > >>>> 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. > >> > >> My thinking was that the binding with bus-type = <0> and bus-width = > >> would mean a parallel bus (type 0 means auto-detect and with a bus- > >> width that auto-detect means a parallel bus) and the most natural/common > >> interpretation of that bus-width. For bus widths of 12, 16, 18, 24, 30 > >> etc I think that would be rgb444, rgb565, rgb666, rgb888, rgb101010 (or, > >> I'm first so I get to define the default). If you have some other > >> interpretation of a bus with that width, you'd need to extend the > >> video-interface binding with some way of saying what you need, perhaps > >> using some kind of data mapping or something to say e.g. bgr666. And > >> you'd need some kind of indicator if you have YUV signals instead of > >> RGB, and LVDS isn't a completely parallel bus, so you'd need something > >> for that. Etc. > > > > The fundamental issue here is that you're tying the bus with to an > > image format. Why <16> can't be, say, MEDIA_BUS_FMT_YVYU8_1X16? it > > spans to 16 bits, doesn't it? And I'm sorry but the 'I'm first so I > > get to define defaults' argument doesn't apply here. > > I never said that <16> could not be something YUV. I said that <16> without > any further information is RGB. But you are right, the fundamental issue > is that I want to specify the full format in the endpoint node, while you > do not for some reason I don't understand. And maybe saying that "<16> > without more info is RGB" is too presumptuous, but then I think that the > right fix is adding something clearly indicating RGB is the way to go, so > that there is a way for endpoints to specify RGB565 (or whatever is needed). I think that would lack genericity though. I could easily imagine a setup similar to yours where the bridge can accept different types of parallel formats (RGB, BGR, YUV, ...). While the bus width is a property of the PCB routing and is thus fixed, the format wouldn't be a property of the platform but would be dynamic. I believe that combining the bus-width DT property that carries static platform information with the dynamic format reported by the bridge (through the API proposed by Jacopo) would then be a more generic approach. You don't have to depend on Jacopo's patch series though, I would be totally fine with assuming that a 16-bit width means RGB565 in the atmel-hlcdc driver. What I'm not comfortable with is hardcoding that assumption in the helper defined in patch 3/7. I would thus propose moving that code to the atmel-hlcdc driver for now, and creating a generic helper that uses both bus-width and the format queried from the bridge when Jacopo's series makes it to mainline. Would that be an acceptable approach for you ? > > So, it is my opinion you need to expose an image format somewhere. And > > it is my opinion again, which can be very wrong ofc, that this place > > is the bridge driver. > > I don't see why the input side is better than the output side when it > comes to specifying these details? The input side is as likely to have > different options as the output side. DRM/KMS is built upon a sink to source model, where userspace specifies the format on the connector at the output of a pipeline, and the formats along the chain of bridges are then computed automatically. That's why we usually query formats supported by sinks and configure sources accordingly. > > You need to adjust the output to accommodate wirings? You can use the > > bus_width on the hlcd side, as Laurent suggested, but the bus width > > does not tie to any image format at all, even more if you're making > > that decision in a drm-wide function. > > > >> Because the word from Rob was that there should be one common binding > >> that describes video interfaces. I started an implementation that > >> interprets that binding in a drm context in > >> [PATCH 3/7] drm: of: introduce drm_of_media_bus_fmt > > > > As usual, one should try to reuse as much as possible of the existing > > bindings. That's a totally different thing compared to assign to a bus > > width property an associated image format for the whole DRM subsystem > > > > ot: those properties should be moved outside of > > media/video_interfaces.txt sooner or later, to a more generic place... > > > >> With that view, any input format specification of the bridge is not > >> helpful for me since what the bridge specifies (without help) is going to > >> be wrong > > > > No it's not useless. I can have an encoder that can provide both YUV > > and RGB formats. If your bridge accepts RGB I want to know that, and > > the bus_width is unrelated imho. > > I didn't say useless *period*, I said not helpful *for me*. What I mean is > that since I have an encoder that can only output RGB formats, asking the > bridge if it expects RGB is rather useless. It adds nothing whatsoever. > > >> anyway. End result, I need to specify the format manually on either the > >> bridge or the atmel-hlcdc side, and I happen to think that the correct > >> side > >> is with the atmel-hlcdc, because that is where my issue originates. In > >> short, the "drm: bridge: Add support for static image formats" series is > >> unrelated as far as I can tell. > > > > I may be biased on this, so I let other to judge and provide more > > suggestions. -- 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: Sat, 21 Apr 2018 11:38:00 +0300 Message-ID: <1549346.R82tLOL7eo@avalon> References: <20180419162751.25223-1-peda@axentia.se> <20180420113859.GL4235@w540> <840625e9-42ff-49b7-605a-202b4d980beb@axentia.se> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <840625e9-42ff-49b7-605a-202b4d980beb@axentia.se> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Peter Rosin Cc: Mark Rutland , Boris Brezillon , jacopo mondi , Alexandre Belloni , devicetree@vger.kernel.org, David Airlie , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Nicolas Ferre , Rob Herring , Jacopo Mondi , Daniel Vetter , Russell King , linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org SGkgUGV0ZXIsCgpPbiBGcmlkYXksIDIwIEFwcmlsIDIwMTggMTU6NTU6NTAgRUVTVCBQZXRlciBS b3NpbiB3cm90ZToKPiBPbiAyMDE4LTA0LTIwIDEzOjM4LCBqYWNvcG8gbW9uZGkgd3JvdGU6Cj4g PiBPbiBGcmksIEFwciAyMCwgMjAxOCBhdCAwMTowNToyMVBNICswMjAwLCBQZXRlciBSb3NpbiB3 cm90ZToKPiA+PiBPbiAyMDE4LTA0LTIwIDEyOjE4LCBMYXVyZW50IFBpbmNoYXJ0IHdyb3RlOgo+ ID4+PiBPbiBGcmlkYXksIDIwIEFwcmlsIDIwMTggMTE6NTI6MzUgRUVTVCBqYWNvcG8gbW9uZGkg d3JvdGU6Cj4gPj4+PiBIaSBQZXRlciwKPiA+Pj4+IAo+ID4+Pj4gSSd2ZSBiZWVuIGEgYml0IGEg cGFpbiBpbiB0aGUgYXJzZSBmb3IgeW91IHJlY2VudGx5LCBidXQgcGxlYXNlCj4gPj4+PiBiZWFy IHdpdGggbWUgYSBiaXQgbW9yZSwgYW5kIHNvcnJ5IGZvciBqdW1waW5nIGxhdGUgb24gdGhlIGJh bmQgd2Fnb24uCj4gPj4+PiAKPiA+Pj4+IE9uIFRodSwgQXByIDE5LCAyMDE4IGF0IDA2OjI3OjQ0 UE0gKzAyMDAsIFBldGVyIFJvc2luIHdyb3RlOgo+ID4+Pj4+IEhpIQo+ID4+Pj4+IAo+ID4+Pj4+ IEkgbmFpdmVseSB0aG91Z2h0IHRoYXQgc2luY2UgdGhlcmUgd2FzIHN1cHBvcnQgZm9yIGJvdGgg bnhwLHRkYTE5OTg4Cj4gPj4+Pj4gKGluIHRoZSB0ZGE5OTh4IGRyaXZlcikgYW5kIHRoZSBhdG1l bC1obGNkYywgdGhpbmdzIHdvdWxkIGJlIGEgc21vb3RoCj4gPj4+Pj4gcmlkZS4gQnV0IGl0IHdh c24ndCwgc28gSSBzdGFydGVkIGxvb2tpbmcgYXJvdW5kIGFuZCByZWFsaXplZCBJIGhhZCB0bwo+ ID4+Pj4+IGZpeCB0aGluZ3MuCj4gPj4+Pj4gCj4gPj4+Pj4gSW4gdjEgYW5kIHYyIEkgZml4ZWQg dGhpbmdzIGJ5IG1ha2luZyB0aGUgYXRtZWwtaGxjZGMgZHJpdmVyIGEgbWFzdGVyCj4gPj4+Pj4g Y29tcG9uZW50LCBidXQgbm93IGluIHYzIEkgZml4IHRoaW5ncyBieSBtYWtpbmcgdGhlIHRkYTk5 OHggZHJpdmVyCj4gPj4+Pj4gYSBicmlkZ2UgaW5zdGVhZC4gVGhpcyB3YXMgYWZ0ZXIgYSBzdWdn ZXN0aW9uIGZyb20gQm9yaXMgQnJlemlsbGlvbgo+ID4+IAo+ID4+IFRoYXQgc2hvdWxkIGJlIEJy ZXppbGxvbiwgc29ycnkgZm9yIGJlaW5nIHNsb3BweSB3aXRoIHRoZSBzcGVsbGluZy4KPiA+PiAK PiA+Pj4+PiAodGhlIGF0bWVsLWhsY2RjIG1haW50YWluZXIpLCBzbyB0aGVyZSB3YXMgc29tZSBy aXNrIG9mIGJpYXMgLi4uIGJ1dAo+ID4+Pj4+IGFmdGVyIGNvbXBhcmluZyB3aGF0IHdhcyBuZWVk ZWQsIEkgdG9vIGZpbmQgdGhlIGJyaWRnZSBhcHByb2FjaAo+ID4+Pj4+IGJldHRlci4KPiA+Pj4+ PiAKPiA+Pj4+PiBJbiBhZGRpdGlvbiB0byB0aGUgYWJvdmUsIG91ciBQQ0IgaW50ZXJmYWNlIGJl dHdlZW4gdGhlIFNBTUE1RDMgYW5kCj4gPj4+Pj4gdGhlIEhETUkgZW5jb2RlciBpcyBvbmx5IHVz aW5nIDE2IGJpdHMsIGFuZCB0aGlzIGhhcyB0byBiZSBkZXNjcmliZWQKPiA+Pj4+PiBzb21ld2hl cmUsIG9yIHRoZSBhdG1lbC1obGNkYyBkcml2ZXIgaGF2ZSBubyBjaGFuY2Ugb2Ygc2VsZWN0aW5n IHRoZQo+ID4+Pj4+IGNvcnJlY3Qgb3V0cHV0IG1vZGUuIFNpbmNlIEkgaGF2ZSBzaW1pbGFyIHBy b2JsZW1zIHdpdGggYSBkczkwYzE4NQo+ID4+Pj4+IGx2ZHMgZW5jb2RlciBJIGFkZGVkIHBhdGNo ZXMgdG8gb3ZlcnJpZGUgdGhlIGF0bWVsLWhsY2RjIG91dHB1dCBmb3JtYXQKPiA+Pj4+PiB2aWEg RFQgcHJvcGVydGllcyBjb21wYXRpYmxlIHdpdGggdGhlIG1lZGlhIHZpZGVvLWludGVyZmFjZSBi aW5kaW5nCj4gPj4+Pj4gYW5kIHRoaW5ncyBzdGFydCB0byBwbGF5IHRvZ2V0aGVyLgo+ID4+Pj4+ IAo+ID4+Pj4+IFNpbmNlIHRoaXMgc2VyaWVzIHN1cGVyc2VlZHMgdGhlIGJyaWRnZSBzZXJpZXMg WzFdLCBJIGhhdmUgaW5jbHVkZWQKPiA+Pj4+PiB0aGUgbGVmdG92ZXIgYmluZGluZ3MgcGF0Y2gg Zm9yIHRoZSB0aSxkczkwYzE4NSBoZXJlLgo+ID4+Pj4gCj4gPj4+PiBJIGZlZWwgbGlrZSB0aGlz IHNlcmllcyB3b3VsZCBsb29rIGJldHRlciBpZiBpdCB3b3VsZCBtYWtlIHVzZSBvZiB0aGUKPiA+ Pj4+IHByb3Bvc2VkIGJyaWRnZSBtZWRpYSBidXMgZm9ybWF0IHN1cHBvcnQgSSBoYXZlIHJlY2Vu dGx5IHNlbnQgb3V0IFsxXQo+ID4+Pj4gKGFuZCB3aGljaCB3YXMgbm90IHRoZXJlIHdoZW4geW91 IGZpcnN0IHNlbnQgdjEpLgo+ID4+Pj4gCj4gPj4+PiBJIHVuZGVyc3RhbmQgeW91ciBmdW5kYW1l bnRhbCBwcm9ibGVtIGhlcmUgaXMgdGhhdCB0aGUgYnVzIGZvcm1hdAo+ID4+Pj4gdGhhdCBzaG91 bGQgYmUgcmVwb3J0ZWQgYnkgeW91ciBicmlkZ2UgaXMgZGlmZmVyZW50IGZyb20gdGhlIG9uZXMK PiA+Pj4+IGFjdHVhbGx5IHN1cHBvcnRlZCBieSB0aGUgVERBMTk5ODggY2hpcCwgYXMgdGhlIHdp cmluZ3MgZ3JvdW5kIHNvbWUKPiA+Pj4+IG9mIHRoZSBpbnB1dCBwaW5zLgo+ID4+Pj4gCj4gPj4+ PiBBbHRob3VnaCB0aGlzIGlzIGRlZmludGVseSBzb21ldGhpbmcgdGhhdCBjb3VsZCBiZSBkZXNj cmliZWQgaW4gdGhlCj4gPj4+PiBicmlkZ2UncyBvd24gT0Ygbm9kZSB3aXRoIHRoZSAnYnVzX3dp ZHRoJyBwcm9wZXJ0eSwgYW5kIHdoYXQgSSB3b3VsZAo+ID4+Pj4gZG8sIG5vdyB0aGF0IHlvdSBo YXZlIG1hZGUgYSBicmlkZ2Ugb3V0IGZyb20gdGhlIHRkYTE5OTg4IGRyaXZlciwgaXM6Cj4gPj4+ PiAKPiA+Pj4+IDEpIFNldCB0aGUgYnJpZGdlIGFjY2VwdGVkIGlucHV0IGJ1c19mb3JtYXQgcGFy c2luZyBpdHMgcGluCj4gPj4+PiBjb25maWd1cmF0aW9uLCBvciBkZWZhdWx0IGl0IGlmIHRoYXQn cyBub3QgaW1wbGVtZW50ZWQgeWV0Lgo+ID4+Pj4gVGhpcyB3aWxsIGxpa2VseSBiZSByZ2I4ODgu IFlvdSBjYW4gZG8gdGhhdCB1c2luZyB0aGUgdHJpdmlhbAo+ID4+Pj4gc3VwcG9ydCBmb3IgYnJp ZGdlIGlucHV0IGltYWdlIGZvcm1hdHMgaW1wbGVtZW50ZWQgYnkgbXkgc2VyaWVzLgo+ID4+Pj4g MikgU3BlY2lmeSBpbiB0aGUgYnJpZGdlIGVuZHBvaW50IGEgJ2J1c193aWR0aCcgb2YgPDE2Pgo+ ID4+Pj4gMykgSW4geW91ciBhdG1lbC1obGNkIGdldCBib3RoIHRoZSBpbWFnZSBmb3JtYXQgb2Yg dGhlIGJyaWRnZSAocmdiODg4KQo+ID4+Pj4gYW5kIHBhcnNlIHRoZSByZW1vdGUgZW5kcG9pbnQg cHJvcGVydHkgJ2J1c193aWR0aCcgYW5kIGdldCB0aGUgPDE2Pgo+ID4+Pj4gdmFsdWUgYmFjay4K PiA+Pj4gCj4gPj4+IFBhcnNpbmcgcHJvcGVydGllcyBvZiByZW1vdGUgbm9kZXMgc2hvdWxkIGJl IGF2b2lkZWQgYXMgbXVjaCBhcwo+ID4+PiBwb3NzaWJsZSwgYXMgdGhleSBuZWVkIHRvIGJlIGlu dGVycHJldGVkIGluIHRoZSBjb250ZXh0IG9mIHRoZSBEVAo+ID4+PiBiaW5kaW5ncyByZWxhdGVk IHRvIHRoZSBjb21wYXRpYmxlIHN0cmluZyBhcHBsaWNhYmxlIHRvIHRoYXQgbm9kZS4gSSdkCj4g Pj4+IHJhdGhlciBoYXZlIHRoZSBidXNfd2lkdGggcHJvcGVydHkgaW4gdGhlIGxvY2FsIGVuZHBv aW50IG5vZGUuCj4gPj4gCj4gPj4gSW4gYWRkaXRpb24gdG8gdGhhdCwgbXkgdmlldyBvZiB0aGlz IGJpbmRpbmcKPiA+PiAKPiA+PiAJZW5kcG9pbnQgewo+ID4+IAkJYnVzLXR5cGUgPSA8MD47Cj4g PiAKPiA+IGJ1cy10eXBlIGlzIHVzZWQgYnkgdjRsMl9md25vZGUgaGVscGVycyB0byBkZWNpZGUg d2hpY2ggdHlwZSBvZiBidXMKPiA+IHRoZXkncmUgZGVhbGluZyB3aXRoIGlpcmMuIEhlcmUgaXQg c2VlbXMgdG8gbWUgaXQgaGFzIG5vIGFkZGVkCj4gPiB2YWx1ZSwgYWxzbyBiZWNhdXNlIGluIHlv dXIgYmluZGluZ3MgZGVzY3JpcHRpb24gIml0IHNoYWxsIGJlIDAiIGFuZAo+ID4gaXQgaXMgbm90 IHBhcnNlZCBhbnl3aGVyZSBpbiB5b3UgY29kZSwgYnV0IG5vIGJpZyBkZWFsLi4uLgo+IAo+IGJ1 cy10eXBlIGlzIGluZGVlZCBwYXJzZWQgYW5kIHZlcmlmaWVkIHRvIGJlIHplcm8gd2hpY2ggbWVh bnMgYXV0by1kZXRlY3QKPiBhY2NvcmRpbmcgdG8gdGhlIHZpZGVvLWludGVyZmFjZXMgYmluZGlu Zy4gQW4gYXV0by1kZXRlY3QgYnVzLXR5cGUgd2l0aAo+IGEgZ2l2ZW4gYnVzLXdpZHRoIG1lYW5z IGEgcGFyYWxsZWwgaW50ZXJmYWNlIElJVUMuCgpJIGJlbGlldmUgeW91IGNvdWxkIGxlYXZlIGl0 IG91dCB0aG91Z2guIFlvdXIgZGlzcGxheSBjb250cm9sbGVyIG9ubHkgc3VwcG9ydHMgCnBhcmFs bGVsIGJ1c2VzLCBzbyB0aGVyZSdzIG5vIG5lZWQgdG8gc3BlY2lmeSB0aGUgYnVzIHR5cGUgZXhw bGljaXRseS4KCj4gRnJvbSBwYXRjaCAzLzc6Cj4gCj4gCWlmIChvZl9wcm9wZXJ0eV9yZWFkX3Uz Mihub2RlLCAiYnVzLXR5cGUiLCAmYnVzX3R5cGUpKQo+IAkJcmV0dXJuIDA7Cj4gCWlmIChidXNf dHlwZSAhPSAwKQo+IAkJcmV0dXJuIC1FSU5WQUw7Cj4gCj4gPj4gCQlidXMtd2lkaHQgPSA8MTY+ Owo+ID4+IAl9Owo+ID4+IAo+ID4+IGlzIHRoYXQgaXQgYWx3YXlzIG1lYW5zIHJnYjU2NS4gU2Vl IGZ1cnRoZXIgYmVsb3cuCj4gPj4gCj4gPj4+PiA0KSBTZXQgdGhlIGNvcnJlY3QgZm9ybWF0IGlu IHRoZSBhdG1lbCBobGNkIGRyaXZlciB0byBhY2NvbW1vZGF0ZSBhCj4gPj4+PiBicmlkZ2UgdGhh dCB3YW50cyByZ2I4ODggb24gYSAxNiBiaXQgd2lkZSBidXMgKHRoYXQgd291bGQgYmUgcmdiNTY1 LAo+ID4+Pj4gb3IgYXJlIHRoZXJlIG90aGVyIHBvc3NpYmxlIGNvbWJpbmF0aW9ucyBJIGFtIG1p c3Npbmc/KQo+ID4+Pj4gCj4gPj4+PiBJIHdvdWxkIGNvbnNpZGVyIHRoaXMgYmV0dGVyIG1vc3Rs eSBiZWNhdXNlIGluIHRoaXMgc2VyaWVzIHlvdSBhcmUKPiA+Pj4+IGNyZWF0aW5nIGEgc2VtYW50 aWMgZm9yIHRoZSB3aG9sZSBEUk0gc3Vic3lzdGVtIG9uIHRoZSAnYnVzX3dpZHRoJwo+ID4+Pj4g cHJvcGVydHksIHdoZXJlIG51bWVyaWNhbCB2YWx1ZXMgYXMgJzEyJywgJzE2JyBldGMgYXJlIGFy Yml0cmFyeSB0aWVkCj4gPj4+PiB0byB0aGUgc2VsZWN0aW9uIG9mIGEgbWVkaWEgYnVzIGZvcm1h dC4gQXQgbGVhc3QgeW91IHNob3VsZCB1c2UgYQo+ID4+Pj4gY29tbW9uIHNldCBvZiBkZWZpbmVz IFsxXSBiZXR3ZWVuIHRoZSBkZXZpY2UgdHJlZSBhbmQgdGhlIGRyaXZlciwKPiA+Pj4+IGJ1dCB0 aGlzIGxvb2tzIGFueXdheSBmcmFnaWxlIGltaG8uCj4gPj4+IAo+ID4+PiBUaGlzIEkgYWdyZWUg d2l0aCB0aG91Z2guIENvbWJpbmluZyB0aGUgcmVtb3RlIGJ1cyBmb3JtYXQgd2l0aCB0aGUgbG9j YWwKPiA+Pj4gYnVzIHdpZHRoIHNob3VsZCBmaXggdGhlIHByb2JsZW0gd2l0aG91dCBoYXZpbmcg dG8gcGFyc2UgcmVtb3RlCj4gPj4+IHByb3BlcnRpZXMuCj4gPj4gCj4gPj4gTXkgdGhpbmtpbmcg d2FzIHRoYXQgdGhlIGJpbmRpbmcgd2l0aCBidXMtdHlwZSA9IDwwPiBhbmQgYnVzLXdpZHRoID0K PiA+PiA8YnBwPiB3b3VsZCBtZWFuIGEgcGFyYWxsZWwgYnVzICh0eXBlIDAgbWVhbnMgYXV0by1k ZXRlY3QgYW5kIHdpdGggYSBidXMtCj4gPj4gd2lkdGggdGhhdCBhdXRvLWRldGVjdCBtZWFucyBh IHBhcmFsbGVsIGJ1cykgYW5kIHRoZSBtb3N0IG5hdHVyYWwvY29tbW9uCj4gPj4gaW50ZXJwcmV0 YXRpb24gb2YgdGhhdCBidXMtd2lkdGguIEZvciBidXMgd2lkdGhzIG9mIDEyLCAxNiwgMTgsIDI0 LCAzMAo+ID4+IGV0YyBJIHRoaW5rIHRoYXQgd291bGQgYmUgcmdiNDQ0LCByZ2I1NjUsIHJnYjY2 NiwgcmdiODg4LCByZ2IxMDEwMTAgKG9yLAo+ID4+IEknbSBmaXJzdCBzbyBJIGdldCB0byBkZWZp bmUgdGhlIGRlZmF1bHQpLiBJZiB5b3UgaGF2ZSBzb21lIG90aGVyCj4gPj4gaW50ZXJwcmV0YXRp b24gb2YgYSBidXMgd2l0aCB0aGF0IHdpZHRoLCB5b3UnZCBuZWVkIHRvIGV4dGVuZCB0aGUKPiA+ PiB2aWRlby1pbnRlcmZhY2UgYmluZGluZyB3aXRoIHNvbWUgd2F5IG9mIHNheWluZyB3aGF0IHlv dSBuZWVkLCBwZXJoYXBzCj4gPj4gdXNpbmcgc29tZSBraW5kIG9mIGRhdGEgbWFwcGluZyBvciBz b21ldGhpbmcgdG8gc2F5IGUuZy4gYmdyNjY2LiBBbmQKPiA+PiB5b3UnZCBuZWVkIHNvbWUga2lu ZCBvZiBpbmRpY2F0b3IgaWYgeW91IGhhdmUgWVVWIHNpZ25hbHMgaW5zdGVhZCBvZgo+ID4+IFJH QiwgYW5kIExWRFMgaXNuJ3QgYSBjb21wbGV0ZWx5IHBhcmFsbGVsIGJ1cywgc28geW91J2QgbmVl ZCBzb21ldGhpbmcKPiA+PiBmb3IgdGhhdC4gRXRjLgo+ID4gCj4gPiBUaGUgZnVuZGFtZW50YWwg aXNzdWUgaGVyZSBpcyB0aGF0IHlvdSdyZSB0eWluZyB0aGUgYnVzIHdpdGggdG8gYW4KPiA+IGlt YWdlIGZvcm1hdC4gV2h5IDwxNj4gY2FuJ3QgYmUsIHNheSwgTUVESUFfQlVTX0ZNVF9ZVllVOF8x WDE2PyBpdAo+ID4gc3BhbnMgdG8gMTYgYml0cywgZG9lc24ndCBpdD8gQW5kIEknbSBzb3JyeSBi dXQgdGhlICdJJ20gZmlyc3Qgc28gSQo+ID4gZ2V0IHRvIGRlZmluZSBkZWZhdWx0cycgYXJndW1l bnQgZG9lc24ndCBhcHBseSBoZXJlLgo+IAo+IEkgbmV2ZXIgc2FpZCB0aGF0IDwxNj4gY291bGQg bm90IGJlIHNvbWV0aGluZyBZVVYuIEkgc2FpZCB0aGF0IDwxNj4gd2l0aG91dAo+IGFueSBmdXJ0 aGVyIGluZm9ybWF0aW9uIGlzIFJHQi4gQnV0IHlvdSBhcmUgcmlnaHQsIHRoZSBmdW5kYW1lbnRh bCBpc3N1ZQo+IGlzIHRoYXQgSSB3YW50IHRvIHNwZWNpZnkgdGhlIGZ1bGwgZm9ybWF0IGluIHRo ZSBlbmRwb2ludCBub2RlLCB3aGlsZSB5b3UKPiBkbyBub3QgZm9yIHNvbWUgcmVhc29uIEkgZG9u J3QgdW5kZXJzdGFuZC4gQW5kIG1heWJlIHNheWluZyB0aGF0ICI8MTY+Cj4gd2l0aG91dCBtb3Jl IGluZm8gaXMgUkdCIiBpcyB0b28gcHJlc3VtcHR1b3VzLCBidXQgdGhlbiBJIHRoaW5rIHRoYXQg dGhlCj4gcmlnaHQgZml4IGlzIGFkZGluZyBzb21ldGhpbmcgY2xlYXJseSBpbmRpY2F0aW5nIFJH QiBpcyB0aGUgd2F5IHRvIGdvLCBzbwo+IHRoYXQgdGhlcmUgaXMgYSB3YXkgZm9yIGVuZHBvaW50 cyB0byBzcGVjaWZ5IFJHQjU2NSAob3Igd2hhdGV2ZXIgaXMgbmVlZGVkKS4KCkkgdGhpbmsgdGhh dCB3b3VsZCBsYWNrIGdlbmVyaWNpdHkgdGhvdWdoLiBJIGNvdWxkIGVhc2lseSBpbWFnaW5lIGEg c2V0dXAgCnNpbWlsYXIgdG8geW91cnMgd2hlcmUgdGhlIGJyaWRnZSBjYW4gYWNjZXB0IGRpZmZl cmVudCB0eXBlcyBvZiBwYXJhbGxlbCAKZm9ybWF0cyAoUkdCLCBCR1IsIFlVViwgLi4uKS4gV2hp bGUgdGhlIGJ1cyB3aWR0aCBpcyBhIHByb3BlcnR5IG9mIHRoZSBQQ0IgCnJvdXRpbmcgYW5kIGlz IHRodXMgZml4ZWQsIHRoZSBmb3JtYXQgd291bGRuJ3QgYmUgYSBwcm9wZXJ0eSBvZiB0aGUgcGxh dGZvcm0gCmJ1dCB3b3VsZCBiZSBkeW5hbWljLiBJIGJlbGlldmUgdGhhdCBjb21iaW5pbmcgdGhl IGJ1cy13aWR0aCBEVCBwcm9wZXJ0eSB0aGF0IApjYXJyaWVzIHN0YXRpYyBwbGF0Zm9ybSBpbmZv cm1hdGlvbiB3aXRoIHRoZSBkeW5hbWljIGZvcm1hdCByZXBvcnRlZCBieSB0aGUgCmJyaWRnZSAo dGhyb3VnaCB0aGUgQVBJIHByb3Bvc2VkIGJ5IEphY29wbykgd291bGQgdGhlbiBiZSBhIG1vcmUg Z2VuZXJpYyAKYXBwcm9hY2guCgpZb3UgZG9uJ3QgaGF2ZSB0byBkZXBlbmQgb24gSmFjb3BvJ3Mg cGF0Y2ggc2VyaWVzIHRob3VnaCwgSSB3b3VsZCBiZSB0b3RhbGx5IApmaW5lIHdpdGggYXNzdW1p bmcgdGhhdCBhIDE2LWJpdCB3aWR0aCBtZWFucyBSR0I1NjUgaW4gdGhlIGF0bWVsLWhsY2RjIGRy aXZlci4gCldoYXQgSSdtIG5vdCBjb21mb3J0YWJsZSB3aXRoIGlzIGhhcmRjb2RpbmcgdGhhdCBh c3N1bXB0aW9uIGluIHRoZSBoZWxwZXIgCmRlZmluZWQgaW4gcGF0Y2ggMy83LiBJIHdvdWxkIHRo dXMgcHJvcG9zZSBtb3ZpbmcgdGhhdCBjb2RlIHRvIHRoZSBhdG1lbC1obGNkYyAKZHJpdmVyIGZv ciBub3csIGFuZCBjcmVhdGluZyBhIGdlbmVyaWMgaGVscGVyIHRoYXQgdXNlcyBib3RoIGJ1cy13 aWR0aCBhbmQgdGhlIApmb3JtYXQgcXVlcmllZCBmcm9tIHRoZSBicmlkZ2Ugd2hlbiBKYWNvcG8n cyBzZXJpZXMgbWFrZXMgaXQgdG8gbWFpbmxpbmUuIApXb3VsZCB0aGF0IGJlIGFuIGFjY2VwdGFi bGUgYXBwcm9hY2ggZm9yIHlvdSA/Cgo+ID4gU28sIGl0IGlzIG15IG9waW5pb24geW91IG5lZWQg dG8gZXhwb3NlIGFuIGltYWdlIGZvcm1hdCBzb21ld2hlcmUuIEFuZAo+ID4gaXQgaXMgbXkgb3Bp bmlvbiBhZ2Fpbiwgd2hpY2ggY2FuIGJlIHZlcnkgd3Jvbmcgb2ZjLCB0aGF0IHRoaXMgcGxhY2UK PiA+IGlzIHRoZSBicmlkZ2UgZHJpdmVyLgo+IAo+IEkgZG9uJ3Qgc2VlIHdoeSB0aGUgaW5wdXQg c2lkZSBpcyBiZXR0ZXIgdGhhbiB0aGUgb3V0cHV0IHNpZGUgd2hlbiBpdAo+IGNvbWVzIHRvIHNw ZWNpZnlpbmcgdGhlc2UgZGV0YWlscz8gVGhlIGlucHV0IHNpZGUgaXMgYXMgbGlrZWx5IHRvIGhh dmUKPiBkaWZmZXJlbnQgb3B0aW9ucyBhcyB0aGUgb3V0cHV0IHNpZGUuCgpEUk0vS01TIGlzIGJ1 aWx0IHVwb24gYSBzaW5rIHRvIHNvdXJjZSBtb2RlbCwgd2hlcmUgdXNlcnNwYWNlIHNwZWNpZmll cyB0aGUgCmZvcm1hdCBvbiB0aGUgY29ubmVjdG9yIGF0IHRoZSBvdXRwdXQgb2YgYSBwaXBlbGlu ZSwgYW5kIHRoZSBmb3JtYXRzIGFsb25nIHRoZSAKY2hhaW4gb2YgYnJpZGdlcyBhcmUgdGhlbiBj b21wdXRlZCBhdXRvbWF0aWNhbGx5LiBUaGF0J3Mgd2h5IHdlIHVzdWFsbHkgcXVlcnkgCmZvcm1h dHMgc3VwcG9ydGVkIGJ5IHNpbmtzIGFuZCBjb25maWd1cmUgc291cmNlcyBhY2NvcmRpbmdseS4K Cj4gPiBZb3UgbmVlZCB0byBhZGp1c3QgdGhlIG91dHB1dCB0byBhY2NvbW1vZGF0ZSB3aXJpbmdz PyBZb3UgY2FuIHVzZSB0aGUKPiA+IGJ1c193aWR0aCBvbiB0aGUgaGxjZCBzaWRlLCBhcyBMYXVy ZW50IHN1Z2dlc3RlZCwgYnV0IHRoZSBidXMgd2lkdGgKPiA+IGRvZXMgbm90IHRpZSB0byBhbnkg aW1hZ2UgZm9ybWF0IGF0IGFsbCwgZXZlbiBtb3JlIGlmIHlvdSdyZSBtYWtpbmcKPiA+IHRoYXQg ZGVjaXNpb24gaW4gYSBkcm0td2lkZSBmdW5jdGlvbi4KPiA+IAo+ID4+IEJlY2F1c2UgdGhlIHdv cmQgZnJvbSBSb2Igd2FzIHRoYXQgdGhlcmUgc2hvdWxkIGJlIG9uZSBjb21tb24gYmluZGluZwo+ ID4+IHRoYXQgZGVzY3JpYmVzIHZpZGVvIGludGVyZmFjZXMuIEkgc3RhcnRlZCBhbiBpbXBsZW1l bnRhdGlvbiB0aGF0Cj4gPj4gaW50ZXJwcmV0cyB0aGF0IGJpbmRpbmcgaW4gYSBkcm0gY29udGV4 dCBpbgo+ID4+IFtQQVRDSCAzLzddIGRybTogb2Y6IGludHJvZHVjZSBkcm1fb2ZfbWVkaWFfYnVz X2ZtdAo+ID4gCj4gPiBBcyB1c3VhbCwgb25lIHNob3VsZCB0cnkgdG8gcmV1c2UgYXMgbXVjaCBh cyBwb3NzaWJsZSBvZiB0aGUgZXhpc3RpbmcKPiA+IGJpbmRpbmdzLiBUaGF0J3MgYSB0b3RhbGx5 IGRpZmZlcmVudCB0aGluZyBjb21wYXJlZCB0byBhc3NpZ24gdG8gYSBidXMKPiA+IHdpZHRoIHBy b3BlcnR5IGFuIGFzc29jaWF0ZWQgaW1hZ2UgZm9ybWF0IGZvciB0aGUgd2hvbGUgRFJNIHN1YnN5 c3RlbQo+ID4gCj4gPiBvdDogdGhvc2UgcHJvcGVydGllcyBzaG91bGQgYmUgbW92ZWQgb3V0c2lk ZSBvZgo+ID4gbWVkaWEvdmlkZW9faW50ZXJmYWNlcy50eHQgc29vbmVyIG9yIGxhdGVyLCB0byBh IG1vcmUgZ2VuZXJpYyBwbGFjZS4uLgo+ID4gCj4gPj4gV2l0aCB0aGF0IHZpZXcsIGFueSBpbnB1 dCBmb3JtYXQgc3BlY2lmaWNhdGlvbiBvZiB0aGUgYnJpZGdlIGlzIG5vdAo+ID4+IGhlbHBmdWwg Zm9yIG1lIHNpbmNlIHdoYXQgdGhlIGJyaWRnZSBzcGVjaWZpZXMgKHdpdGhvdXQgaGVscCkgaXMg Z29pbmcgdG8KPiA+PiBiZSB3cm9uZwo+ID4gCj4gPiBObyBpdCdzIG5vdCB1c2VsZXNzLiBJIGNh biBoYXZlIGFuIGVuY29kZXIgdGhhdCBjYW4gcHJvdmlkZSBib3RoIFlVVgo+ID4gYW5kIFJHQiBm b3JtYXRzLiBJZiB5b3VyIGJyaWRnZSBhY2NlcHRzIFJHQiBJIHdhbnQgdG8ga25vdyB0aGF0LCBh bmQKPiA+IHRoZSBidXNfd2lkdGggaXMgdW5yZWxhdGVkIGltaG8uCj4gCj4gSSBkaWRuJ3Qgc2F5 IHVzZWxlc3MgKnBlcmlvZCosIEkgc2FpZCBub3QgaGVscGZ1bCAqZm9yIG1lKi4gV2hhdCBJIG1l YW4gaXMKPiB0aGF0IHNpbmNlIEkgaGF2ZSBhbiBlbmNvZGVyIHRoYXQgY2FuIG9ubHkgb3V0cHV0 IFJHQiBmb3JtYXRzLCBhc2tpbmcgdGhlCj4gYnJpZGdlIGlmIGl0IGV4cGVjdHMgUkdCIGlzIHJh dGhlciB1c2VsZXNzLiBJdCBhZGRzIG5vdGhpbmcgd2hhdHNvZXZlci4KPiAKPiA+PiBhbnl3YXku IEVuZCByZXN1bHQsIEkgbmVlZCB0byBzcGVjaWZ5IHRoZSBmb3JtYXQgbWFudWFsbHkgb24gZWl0 aGVyIHRoZQo+ID4+IGJyaWRnZSBvciB0aGUgYXRtZWwtaGxjZGMgc2lkZSwgYW5kIEkgaGFwcGVu IHRvIHRoaW5rIHRoYXQgdGhlIGNvcnJlY3QKPiA+PiBzaWRlCj4gPj4gaXMgd2l0aCB0aGUgYXRt ZWwtaGxjZGMsIGJlY2F1c2UgdGhhdCBpcyB3aGVyZSBteSBpc3N1ZSBvcmlnaW5hdGVzLiBJbgo+ ID4+IHNob3J0LCB0aGUgImRybTogYnJpZGdlOiBBZGQgc3VwcG9ydCBmb3Igc3RhdGljIGltYWdl IGZvcm1hdHMiIHNlcmllcyBpcwo+ID4+IHVucmVsYXRlZCBhcyBmYXIgYXMgSSBjYW4gdGVsbC4K PiA+IAo+ID4gSSBtYXkgYmUgYmlhc2VkIG9uIHRoaXMsIHNvIEkgbGV0IG90aGVyIHRvIGp1ZGdl IGFuZCBwcm92aWRlIG1vcmUKPiA+IHN1Z2dlc3Rpb25zLgoKLS0gClJlZ2FyZHMsCgpMYXVyZW50 IFBpbmNoYXJ0CgoKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9y ZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZl bAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751904AbeDUIh4 (ORCPT ); Sat, 21 Apr 2018 04:37:56 -0400 Received: from perceval.ideasonboard.com ([213.167.242.64]:59660 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751052AbeDUIhv (ORCPT ); Sat, 21 Apr 2018 04:37:51 -0400 From: Laurent Pinchart To: Peter Rosin Cc: jacopo mondi , 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: Sat, 21 Apr 2018 11:38:00 +0300 Message-ID: <1549346.R82tLOL7eo@avalon> Organization: Ideas on Board Oy In-Reply-To: <840625e9-42ff-49b7-605a-202b4d980beb@axentia.se> References: <20180419162751.25223-1-peda@axentia.se> <20180420113859.GL4235@w540> <840625e9-42ff-49b7-605a-202b4d980beb@axentia.se> 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 Hi Peter, On Friday, 20 April 2018 15:55:50 EEST Peter Rosin wrote: > On 2018-04-20 13:38, jacopo mondi wrote: > > On Fri, Apr 20, 2018 at 01:05:21PM +0200, Peter Rosin wrote: > >> On 2018-04-20 12:18, Laurent Pinchart wrote: > >>> 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 > >> > >> That should be Brezillon, sorry for being sloppy with the spelling. > >> > >>>>> (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. > >> > >> In addition to that, my view of this binding > >> > >> endpoint { > >> bus-type = <0>; > > > > bus-type is used by v4l2_fwnode helpers to decide which type of bus > > they're dealing with iirc. Here it seems to me it has no added > > value, also because in your bindings description "it shall be 0" and > > it is not parsed anywhere in you code, but no big deal.... > > bus-type is indeed parsed and verified to be zero which means auto-detect > according to the video-interfaces binding. An auto-detect bus-type with > a given bus-width means a parallel interface IIUC. I believe you could leave it out though. Your display controller only supports parallel buses, so there's no need to specify the bus type explicitly. > From patch 3/7: > > if (of_property_read_u32(node, "bus-type", &bus_type)) > return 0; > if (bus_type != 0) > return -EINVAL; > > >> bus-widht = <16>; > >> }; > >> > >> is that it always means rgb565. See further below. > >> > >>>> 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. > >> > >> My thinking was that the binding with bus-type = <0> and bus-width = > >> would mean a parallel bus (type 0 means auto-detect and with a bus- > >> width that auto-detect means a parallel bus) and the most natural/common > >> interpretation of that bus-width. For bus widths of 12, 16, 18, 24, 30 > >> etc I think that would be rgb444, rgb565, rgb666, rgb888, rgb101010 (or, > >> I'm first so I get to define the default). If you have some other > >> interpretation of a bus with that width, you'd need to extend the > >> video-interface binding with some way of saying what you need, perhaps > >> using some kind of data mapping or something to say e.g. bgr666. And > >> you'd need some kind of indicator if you have YUV signals instead of > >> RGB, and LVDS isn't a completely parallel bus, so you'd need something > >> for that. Etc. > > > > The fundamental issue here is that you're tying the bus with to an > > image format. Why <16> can't be, say, MEDIA_BUS_FMT_YVYU8_1X16? it > > spans to 16 bits, doesn't it? And I'm sorry but the 'I'm first so I > > get to define defaults' argument doesn't apply here. > > I never said that <16> could not be something YUV. I said that <16> without > any further information is RGB. But you are right, the fundamental issue > is that I want to specify the full format in the endpoint node, while you > do not for some reason I don't understand. And maybe saying that "<16> > without more info is RGB" is too presumptuous, but then I think that the > right fix is adding something clearly indicating RGB is the way to go, so > that there is a way for endpoints to specify RGB565 (or whatever is needed). I think that would lack genericity though. I could easily imagine a setup similar to yours where the bridge can accept different types of parallel formats (RGB, BGR, YUV, ...). While the bus width is a property of the PCB routing and is thus fixed, the format wouldn't be a property of the platform but would be dynamic. I believe that combining the bus-width DT property that carries static platform information with the dynamic format reported by the bridge (through the API proposed by Jacopo) would then be a more generic approach. You don't have to depend on Jacopo's patch series though, I would be totally fine with assuming that a 16-bit width means RGB565 in the atmel-hlcdc driver. What I'm not comfortable with is hardcoding that assumption in the helper defined in patch 3/7. I would thus propose moving that code to the atmel-hlcdc driver for now, and creating a generic helper that uses both bus-width and the format queried from the bridge when Jacopo's series makes it to mainline. Would that be an acceptable approach for you ? > > So, it is my opinion you need to expose an image format somewhere. And > > it is my opinion again, which can be very wrong ofc, that this place > > is the bridge driver. > > I don't see why the input side is better than the output side when it > comes to specifying these details? The input side is as likely to have > different options as the output side. DRM/KMS is built upon a sink to source model, where userspace specifies the format on the connector at the output of a pipeline, and the formats along the chain of bridges are then computed automatically. That's why we usually query formats supported by sinks and configure sources accordingly. > > You need to adjust the output to accommodate wirings? You can use the > > bus_width on the hlcd side, as Laurent suggested, but the bus width > > does not tie to any image format at all, even more if you're making > > that decision in a drm-wide function. > > > >> Because the word from Rob was that there should be one common binding > >> that describes video interfaces. I started an implementation that > >> interprets that binding in a drm context in > >> [PATCH 3/7] drm: of: introduce drm_of_media_bus_fmt > > > > As usual, one should try to reuse as much as possible of the existing > > bindings. That's a totally different thing compared to assign to a bus > > width property an associated image format for the whole DRM subsystem > > > > ot: those properties should be moved outside of > > media/video_interfaces.txt sooner or later, to a more generic place... > > > >> With that view, any input format specification of the bridge is not > >> helpful for me since what the bridge specifies (without help) is going to > >> be wrong > > > > No it's not useless. I can have an encoder that can provide both YUV > > and RGB formats. If your bridge accepts RGB I want to know that, and > > the bus_width is unrelated imho. > > I didn't say useless *period*, I said not helpful *for me*. What I mean is > that since I have an encoder that can only output RGB formats, asking the > bridge if it expects RGB is rather useless. It adds nothing whatsoever. > > >> anyway. End result, I need to specify the format manually on either the > >> bridge or the atmel-hlcdc side, and I happen to think that the correct > >> side > >> is with the atmel-hlcdc, because that is where my issue originates. In > >> short, the "drm: bridge: Add support for static image formats" series is > >> unrelated as far as I can tell. > > > > I may be biased on this, so I let other to judge and provide more > > suggestions. -- Regards, Laurent Pinchart