From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yakir Yang Subject: Re: [PATCH v4] drm/rockchip: hdmi: add Innosilicon HDMI support Date: Tue, 19 Jan 2016 11:50:29 +0800 Message-ID: <569DB285.1000009@rock-chips.com> References: <1453128123-29155-1-git-send-email-ykk@rock-chips.com> <1453128140-29208-1-git-send-email-ykk@rock-chips.com> <20160118171514.GG19062@n2100.arm.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20160118171514.GG19062@n2100.arm.linux.org.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Russell King - ARM Linux Cc: Mark Rutland , devicetree@vger.kernel.org, Thierry Reding , Pawel Moll , Ian Campbell , Ken Mixte , Zheng Yang , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-rockchip@lists.infradead.org, Rob Herring , Kumar Gala , Ben Chan , linux-arm-kernel@lists.infradead.org List-Id: linux-rockchip.vger.kernel.org SGkgUnVzc2VsbCwKClRoYW5rcyBmb3IgeW91ciBjb21tZW50cyA6LUQKCk9uIDAxLzE5LzIwMTYg MDE6MTUgQU0sIFJ1c3NlbGwgS2luZyAtIEFSTSBMaW51eCB3cm90ZToKPiBIaSwKPgo+IFNvbWUg Y29tbWVudHMgYmVsb3cuLi4KPgo+IE9uIE1vbiwgSmFuIDE4LCAyMDE2IGF0IDEwOjQyOjIwUE0g KzA4MDAsIFlha2lyIFlhbmcgd3JvdGU6Cj4+ICtzdGF0aWMgaW50IGlubm9faGRtaV9jb25maWdf dmlkZW9fYXZpKHN0cnVjdCBpbm5vX2hkbWkgKmhkbWkpCj4+ICt7Cj4+ICsJY2hhciBpbmZvW0hE TUlfU0laRV9BVklfSU5GT0ZSQU1FXSA9IHswfTsKPj4gKwlpbnQgYXZpX2NvbG9yX21vZGU7Cj4+ ICsJaW50IGk7Cj4+ICsKPj4gKwloZG1pX3dyaXRlYihoZG1pLCBIRE1JX0NPTlRST0xfUEFDS0VU X0JVRl9JTkRFWCwgSU5GT0ZSQU1FX0FWSSk7Cj4+ICsKPj4gKwlpbmZvWzBdID0gMHg4MjsKPj4g KwlpbmZvWzFdID0gMHgwMjsKPj4gKwlpbmZvWzJdID0gMHgwRDsKPj4gKwlpbmZvWzNdID0gaW5m b1swXSArIGluZm9bMV0gKyBpbmZvWzJdOwo+PiArCj4+ICsJaWYgKGhkbWktPmhkbWlfZGF0YS5l bmNfb3V0X2Zvcm1hdCA9PSBIRE1JX0NPTE9SU1BBQ0VfUkdCKQo+PiArCQlhdmlfY29sb3JfbW9k ZSA9IEFWSV9DT0xPUl9NT0RFX1JHQjsKPj4gKwllbHNlIGlmIChoZG1pLT5oZG1pX2RhdGEuZW5j X291dF9mb3JtYXQgPT0gSERNSV9DT0xPUlNQQUNFX1lVVjQ0NCkKPj4gKwkJYXZpX2NvbG9yX21v ZGUgPSBBVklfQ09MT1JfTU9ERV9ZQ0JDUjQ0NDsKPj4gKwllbHNlIGlmIChoZG1pLT5oZG1pX2Rh dGEuZW5jX291dF9mb3JtYXQgPT0gSERNSV9DT0xPUlNQQUNFX1lVVjQyMikKPj4gKwkJYXZpX2Nv bG9yX21vZGUgPSBBVklfQ09MT1JfTU9ERV9ZQ0JDUjQyMjsKPj4gKwllbHNlCj4+ICsJCWF2aV9j b2xvcl9tb2RlID0gQVZJX0NPTE9SX01PREVfUkdCOwo+PiArCj4+ICsJaW5mb1s0XSA9IChhdmlf Y29sb3JfbW9kZSA8PCA1KTsKPj4gKwlpbmZvWzVdID0gKEFWSV9DT0xPUklNRVRSWV9OT19EQVRB IDw8IDYpIHwKPj4gKwkJICAoQVZJX0NPREVEX0ZSQU1FX0FTUEVDVF9OT19EQVRBIDw8IDQpIHwK Pj4gKwkJICBBQ1RJVkVfQVNQRUNUX1JBVEVfU0FNRV9BU19DT0RFRF9GUkFNRTsKPj4gKwo+PiAr CWluZm9bNl0gPSAwOwo+PiArCWluZm9bN10gPSBoZG1pLT5oZG1pX2RhdGEudmljOwo+PiArCj4+ ICsJaWYgKGhkbWktPmhkbWlfZGF0YS52aWMgPT0gNiB8fCBoZG1pLT5oZG1pX2RhdGEudmljID09 IDcgfHwKPj4gKwkgICAgaGRtaS0+aGRtaV9kYXRhLnZpYyA9PSAyMSB8fCBoZG1pLT5oZG1pX2Rh dGEudmljID09IDIyKQo+PiArCQlpbmZvWzhdID0gMTsKPj4gKwllbHNlCj4+ICsJCWluZm9bOF0g PSAwOwo+PiArCj4+ICsJLyogQ2FsY3VsYXRlIGF2aSBpbmZvIGZyYW1lIGNoZWNLc3VtICovCj4+ ICsJZm9yIChpID0gNDsgaSA8IEhETUlfU0laRV9BVklfSU5GT0ZSQU1FOyBpKyspCj4+ICsJCWlu Zm9bM10gKz0gaW5mb1tpXTsKPj4gKwlpbmZvWzNdID0gMHgxMDAgLSBpbmZvWzNdOwo+PiArCj4+ ICsJZm9yIChpID0gMDsgaSA8IEhETUlfU0laRV9BVklfSU5GT0ZSQU1FOyBpKyspCj4+ICsJCWhk bWlfd3JpdGViKGhkbWksIEhETUlfQ09OVFJPTF9QQUNLRVRfQUREUiArIGksIGluZm9baV0pOwo+ PiArCj4+ICsJcmV0dXJuIDA7Cj4gSXMgdGhlcmUgYSByZWFzb24gd2h5IHRoZSBoZWxwZXJzIGlu IGRyaXZlcnMvdmlkZW8vaGRtaS5jIGNhbid0IGJlIHVzZWQKPiB0byBnZW5lcmF0ZSB0aGlzPwo+ Cj4gaGRtaV9hdmlfaW5mb2ZyYW1lX2luaXQgLyBoZG1pX2F2aV9pbmZvZnJhbWVfcGFjayBhbmQK PiBkcm1faGRtaV9hdmlfaW5mb2ZyYW1lX2Zyb21fZGlzcGxheV9tb2RlID8KCkdyZWF0LCB0aGFu a3MgZm9yIHBvaW50IG91dCwgaXQgd291bGQgbWFrZSBjb2RlIG11Y2ggY2xlYW4gd2l0aCB0aG9z ZSAKaGVscGVyIGZ1bmN0aW9ucy4KCj4+ICt9Cj4+ICsKPj4gK3N0YXRpYyBpbnQgaW5ub19oZG1p X2NvbmZpZ192aWRlb192c2koc3RydWN0IGlubm9faGRtaSAqaGRtaSkKPj4gK3sKPj4gKwljaGFy IGluZm9bSERNSV9TSVpFX1ZTSV9JTkZPRlJBTUVdID0gezB9Owo+PiArCWludCBpOwo+PiArCj4+ ICsJaGRtaV9tb2RiKGhkbWksIEhETUlfUEFDS0VUX1NFTkRfQVVUTywgbV9QQUNLRVRfVlNJX0VO LAo+PiArCQkgIHZfUEFDS0VUX1ZTSV9FTigwKSk7Cj4+ICsKPj4gKwloZG1pX3dyaXRlYihoZG1p LCBIRE1JX0NPTlRST0xfUEFDS0VUX0JVRl9JTkRFWCwgSU5GT0ZSQU1FX1ZTSSk7Cj4+ICsKPj4g KwkvKiBIZWFkZXIgQnl0ZXMgKi8KPj4gKwlpbmZvWzBdID0gMHg4MTsKPj4gKwlpbmZvWzFdID0g MHgwMTsKPj4gKwo+PiArCS8qIFBCMSAtIFBCMyBjb250YWluIHRoZSAyNGJpdCBJRUVFIFJlZ2lz dHJhdGlvbiBJZGVudGlmaWVyICovCj4+ICsJaW5mb1s0XSA9IDB4MDM7Cj4+ICsJaW5mb1s1XSA9 IDB4MGM7Cj4+ICsJaW5mb1s2XSA9IDB4MDA7Cj4+ICsKPj4gKwkvKiBQQjQgLSBIRE1JX1ZpZGVv X0Zvcm1hdCBpbnRvIGJpdHMgNzo1ICovCj4+ICsJaW5mb1s3XSA9IDA7Cj4+ICsKPj4gKwkvKgo+ PiArCSAqIFBCNSAtIERlcGVuZGluZyBvbiB0aGUgdmlkZW8gZm9ybWF0LCB0aGlzIGJ5dGUgd2ls bCBjb250YWluCj4+ICsJICogZWl0aGVyIHRoZSBIRE1JX1ZJQyBjb2RlIGluIGJ1dHMgNzowLCBP UiB0aGUgM0RfU3RydWN0dXJlIGluCj4+ICsJICogYml0cyA3OjQuCj4+ICsJICovCj4+ICsJaW5m b1syXSA9IDB4MDYgLSAyOwo+PiArCWluZm9bOF0gPSAwOwo+PiArCWluZm9bOV0gPSAwOwo+PiAr Cj4+ICsJaW5mb1szXSA9IGluZm9bMF0gKyBpbmZvWzFdICsgaW5mb1syXTsKPj4gKwo+PiArCS8q IENhbGN1bGF0ZSBpbmZvIGZyYW1lIGNoZWNLc3VtICovCj4+ICsJZm9yIChpID0gNDsgaSA8IEhE TUlfU0laRV9WU0lfSU5GT0ZSQU1FOyBpKyspCj4+ICsJCWluZm9bM10gKz0gaW5mb1tpXTsKPj4g KwlpbmZvWzNdID0gMHgxMDAgLSBpbmZvWzNdOwo+PiArCj4+ICsJZm9yIChpID0gMDsgaSA8IEhE TUlfU0laRV9WU0lfSU5GT0ZSQU1FOyBpKyspCj4+ICsJCWhkbWlfd3JpdGViKGhkbWksIEhETUlf Q09OVFJPTF9QQUNLRVRfQUREUiArIGksIGluZm9baV0pOwo+PiArCj4+ICsJaGRtaV9tb2RiKGhk bWksIEhETUlfUEFDS0VUX1NFTkRfQVVUTywgbV9QQUNLRVRfVlNJX0VOLAo+PiArCQkgIHZfUEFD S0VUX1ZTSV9FTigxKSk7Cj4+ICsKPj4gKwlyZXR1cm4gMDsKPiBoZG1pX3ZlbmRvcl9pbmZvZnJh bWVfaW5pdCAvIGhkbWlfdmVuZG9yX2luZm9mcmFtZV9wYWNrPwo+Cj4gWW91IGNhbiBwcm9iYWJs eSB1c2UgdGhlIHNhbWUgZnVuY3Rpb24gdG8gdXBsb2FkIHRoZSBjb250cm9sIHBhY2tldAo+IHRv byAtIHNvbWV0aGluZyBsaWtlOgo+Cj4gc3RhdGljIGludCBpbm5vX2hkbWlfdXBsb2FkX2ZyYW1l KHN0cnVjdCBpbm5vX2hkbWkgKmhkbWksIGludCBzZXR1cF9yYywKPiAJdW5pb24gaGRtaV9pbmZv ZnJhbWUgKmZyYW1lLCB1MzIgbWFzaywgdTMyIGRpc2FibGUsIHUzMiBlbmFibGUpCj4gewo+IAlp ZiAobWFzaykKPiAJCWhkbWlfbW9kYihoZG1pLCBIRE1JX1BBQ0tFVF9TRU5EX0FVVE8sIG1hc2ss IGRpc2FibGUpOwo+Cj4gCWlmIChzZXR1cF9yYyA+PSAwKSB7Cj4gCQl1OCBwYWNrZWRfZnJhbWVb WU9VUl9NQVhJTVVNX0lORk9fRlJBTUVfU0laRV07Cj4gCQlzc2l6ZV90IHJjLCBpOwo+Cj4gCQly YyA9IGhkbWlfaW5mb2ZyYW1lX3BhY2soZnJhbWUsIHBhY2tlZF9mcmFtZSwKPiAJCQkJCSBzaXpl b2YocGFja2VkX2ZyYW1lKSk7Cj4gCQlpZiAocmMgPCAwKQo+IAkJCXJldHVybiByYzsKPgo+IAkJ Zm9yIChpID0gMDsgaSA8IHJjOyBpKyspCj4gCQkJaGRtaV93cml0ZWIoaGRtaSwgSERNSV9DT05U Uk9MX1BBQ0tFVF9BRERSICsgaSwgYnVmW2ldKTsKPgo+IAkJaWYgKG1hc2spCj4gCQkJaGRtaV9t b2RiKGhkbWksIEhETUlfUEFDS0VUX1NFTkRfQVVUTywgbWFzaywgZW5hYmxlKTsKPiAJfQo+Cj4g CXJldHVybiBzZXR1cF9yYzsKPiB9Cj4KPiBzdGF0aWMgaW50IGlubm9faGRtaV9jb25maWdfdmlk ZW9fdnNpKHN0cnVjdCBpbm5vX2hkbWkgKmhkbWkpCj4gewo+IAl1bmlvbiBoZG1pX2luZm9mcmFt ZSBmcmFtZTsKPgo+IAlyYyA9IGRybV9oZG1pX3ZlbmRvcl9pbmZvZnJhbWVfZnJvbV9kaXNwbGF5 X21vZGUoJmZyYW1lLnZlbmRvci5oZG1pLAo+IAkJCQkJCQkgdGhlX2RybV9kaXNwbGF5X21vZGUp Owo+Cj4gCXJldHVybiBpbm5vX2hkbWlfdXBsb2FkX2ZyYW1lKGhkbWksIHJjLCAmZnJhbWUsIG1f UEFDS0VUX1ZTSV9FTiwKPiAJCQkJICAgICAgdl9QQUNLRVRfVlNJX0VOKDApLCB2X1BBQ0tFVF9W U0lfRU4oMSkpOwo+IH0KPgoKQWguLi4uIGFwcHJlY2lhdGUgZm9yIHRoZSBjb2RlLCAgaXQncyBn cmVhdCBzdWdnZXN0LCB0aGFua3MuCgpBbHNvIEkgZm91bmQgYSBtaXN0YWtlbiB3aXRoIHByZXZp b3VzICJpbm5vX2hkbWlfY29uZmlnX3ZpZGVvX3ZzaSIKZnVuY3Rpb24gdGhhdCB3ZSBkb24ndCBu ZWVkIHRvIHNlbmQgdGhlIEhETUkgdmVuZG9yIGluZm9mcmFtZXMgaWYKd2UgYXJlIG5vdCB1c2lu ZyBhIDRLIG9yIHN0ZXJlb3Njb3BpYyAzRCBtb2RlLiAgOikKCj4+ICtzdGF0aWMgaW50IGlubm9f aGRtaV9pMmNfd2FpdChzdHJ1Y3QgaW5ub19oZG1pICpoZG1pKQo+PiArewo+PiArCXN0cnVjdCBp bm5vX2hkbWlfaTJjICppMmMgPSBoZG1pLT5pMmM7Cj4+ICsJaW50IHN0YXQ7Cj4+ICsKPj4gKwlz dGF0ID0gd2FpdF9mb3JfY29tcGxldGlvbl90aW1lb3V0KCZpMmMtPmNtcCwgSFogLyAxMCk7Cj4+ ICsJaWYgKCFzdGF0KSB7Cj4+ICsJCXN0YXQgPSB3YWl0X2Zvcl9jb21wbGV0aW9uX3RpbWVvdXQo JmkyYy0+Y21wLCBIWiAvIDEwKTsKPj4gKwkJaWYgKCFzdGF0KQo+PiArCQkJcmV0dXJuIC1FQUdB SU47Cj4gV2hhdCdzIHRoZSByZWFzb24gZm9yIHRoaXMgZG91YmxlIHdhaXRfZm9yX2NvbXBsZXRp b24/ICBUaGlzIHByb2JhYmx5Cj4gbmVlZHMgYSBjb21tZW50LCBvdGhlcndpc2UgaXQgbG9va3Mg bGlrZSBzb21lIHdlaXJkIGN1dC1uLXBhc3RlIGVycm9yLgoKWWVwLCB5b3UgJ3JlIHJpZ2h0LCBk b24ndCBuZWVkIGRvdWJsZSB3YWl0IGhlcmUuCgo+PiArc3RhdGljIGludCBpbm5vX2hkbWlfaTJj X3dyaXRlKHN0cnVjdCBpbm5vX2hkbWkgKmhkbWksIHN0cnVjdCBpMmNfbXNnICptc2dzKQo+PiAr ewo+PiArCXN0cnVjdCBpbm5vX2hkbWlfaTJjICppMmMgPSBoZG1pLT5pMmM7Cj4gWW91IHNlZW0g dG8gaGF2ZSBhIGxvY2FsIGkyYyBwb2ludGVyLi4uCkFoLCBnb3QgaXQgIDspCj4+ICsKPj4gKwkv Kgo+PiArCSAqIFRoZSBEREMgbW9kdWxlIG9ubHkgc3VwcG9ydCByZWFkIEVESUQgbWVzc2FnZSwg c28KPj4gKwkgKiB3ZSBhc3N1bWUgdGhhdCBlYWNoIHdvcmQgd3JpdGUgdG8gdGhpcyBpMmMgYWRh cHRlcgo+PiArCSAqIHNob3VsZCBiZSB0aGUgb2Zmc2V0IG9mIEVESUQgd29yZCBhZGRyZXNzLgo+ PiArCSAqLwo+PiArCWlmICgobXNncy0+bGVuICE9IDEpIHx8Cj4+ICsJICAgICgobXNncy0+YWRk ciAhPSBERENfQUREUikgJiYgKG1zZ3MtPmFkZHIgIT0gRERDX1NFR01FTlRfQUREUikpKQo+PiAr CQlyZXR1cm4gLUVJTlZBTDsKPj4gKwo+PiArCXJlaW5pdF9jb21wbGV0aW9uKCZpMmMtPmNtcCk7 Cj4+ICsKPj4gKwlpZiAobXNncy0+YWRkciA9PSBERENfU0VHTUVOVF9BRERSKQo+PiArCQloZG1p LT5pMmMtPnNlZ21lbnRfYWRkciA9IG1zZ3MtPmJ1ZlswXTsKPj4gKwlpZiAobXNncy0+YWRkciA9 PSBERENfQUREUikKPj4gKwkJaGRtaS0+aTJjLT5kZGNfYWRkciA9IG1zZ3MtPmJ1ZlswXTsKPj4g Kwo+PiArCS8qIFNldCBlZGlkIGZpZm8gZmlyc3QgYWRkciAqLwo+PiArCWhkbWlfd3JpdGViKGhk bWksIEhETUlfRURJRF9GSUZPX09GRlNFVCwgMHgwMCk7Cj4+ICsKPj4gKwkvKiBTZXQgZWRpZCB3 b3JkIGFkZHJlc3MgMHgwMC8weDgwICovCj4+ICsJaGRtaV93cml0ZWIoaGRtaSwgSERNSV9FRElE X1dPUkRfQUREUiwgaGRtaS0+aTJjLT5kZGNfYWRkcik7Cj4+ICsKPj4gKwkvKiBTZXQgZWRpZCBz ZWdtZW50IHBvaW50ZXIgKi8KPj4gKwloZG1pX3dyaXRlYihoZG1pLCBIRE1JX0VESURfU0VHTUVO VF9QT0lOVEVSLCBoZG1pLT5pMmMtPnNlZ21lbnRfYWRkcik7Cj4gYnV0IHRoZW4geW91IGRvbid0 IHVzZSBpdCBpbiB0aGUgZm91ciBsb2NhdGlvbnMgYWJvdmUuCj4KPgoKVGhhbmtzLAotIFlha2ly CgoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRl dmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9s aXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: ykk@rock-chips.com (Yakir Yang) Date: Tue, 19 Jan 2016 11:50:29 +0800 Subject: [PATCH v4] drm/rockchip: hdmi: add Innosilicon HDMI support In-Reply-To: <20160118171514.GG19062@n2100.arm.linux.org.uk> References: <1453128123-29155-1-git-send-email-ykk@rock-chips.com> <1453128140-29208-1-git-send-email-ykk@rock-chips.com> <20160118171514.GG19062@n2100.arm.linux.org.uk> Message-ID: <569DB285.1000009@rock-chips.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Russell, Thanks for your comments :-D On 01/19/2016 01:15 AM, Russell King - ARM Linux wrote: > Hi, > > Some comments below... > > On Mon, Jan 18, 2016 at 10:42:20PM +0800, Yakir Yang wrote: >> +static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi) >> +{ >> + char info[HDMI_SIZE_AVI_INFOFRAME] = {0}; >> + int avi_color_mode; >> + int i; >> + >> + hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_BUF_INDEX, INFOFRAME_AVI); >> + >> + info[0] = 0x82; >> + info[1] = 0x02; >> + info[2] = 0x0D; >> + info[3] = info[0] + info[1] + info[2]; >> + >> + if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) >> + avi_color_mode = AVI_COLOR_MODE_RGB; >> + else if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_YUV444) >> + avi_color_mode = AVI_COLOR_MODE_YCBCR444; >> + else if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_YUV422) >> + avi_color_mode = AVI_COLOR_MODE_YCBCR422; >> + else >> + avi_color_mode = AVI_COLOR_MODE_RGB; >> + >> + info[4] = (avi_color_mode << 5); >> + info[5] = (AVI_COLORIMETRY_NO_DATA << 6) | >> + (AVI_CODED_FRAME_ASPECT_NO_DATA << 4) | >> + ACTIVE_ASPECT_RATE_SAME_AS_CODED_FRAME; >> + >> + info[6] = 0; >> + info[7] = hdmi->hdmi_data.vic; >> + >> + if (hdmi->hdmi_data.vic == 6 || hdmi->hdmi_data.vic == 7 || >> + hdmi->hdmi_data.vic == 21 || hdmi->hdmi_data.vic == 22) >> + info[8] = 1; >> + else >> + info[8] = 0; >> + >> + /* Calculate avi info frame checKsum */ >> + for (i = 4; i < HDMI_SIZE_AVI_INFOFRAME; i++) >> + info[3] += info[i]; >> + info[3] = 0x100 - info[3]; >> + >> + for (i = 0; i < HDMI_SIZE_AVI_INFOFRAME; i++) >> + hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_ADDR + i, info[i]); >> + >> + return 0; > Is there a reason why the helpers in drivers/video/hdmi.c can't be used > to generate this? > > hdmi_avi_infoframe_init / hdmi_avi_infoframe_pack and > drm_hdmi_avi_infoframe_from_display_mode ? Great, thanks for point out, it would make code much clean with those helper functions. >> +} >> + >> +static int inno_hdmi_config_video_vsi(struct inno_hdmi *hdmi) >> +{ >> + char info[HDMI_SIZE_VSI_INFOFRAME] = {0}; >> + int i; >> + >> + hdmi_modb(hdmi, HDMI_PACKET_SEND_AUTO, m_PACKET_VSI_EN, >> + v_PACKET_VSI_EN(0)); >> + >> + hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_BUF_INDEX, INFOFRAME_VSI); >> + >> + /* Header Bytes */ >> + info[0] = 0x81; >> + info[1] = 0x01; >> + >> + /* PB1 - PB3 contain the 24bit IEEE Registration Identifier */ >> + info[4] = 0x03; >> + info[5] = 0x0c; >> + info[6] = 0x00; >> + >> + /* PB4 - HDMI_Video_Format into bits 7:5 */ >> + info[7] = 0; >> + >> + /* >> + * PB5 - Depending on the video format, this byte will contain >> + * either the HDMI_VIC code in buts 7:0, OR the 3D_Structure in >> + * bits 7:4. >> + */ >> + info[2] = 0x06 - 2; >> + info[8] = 0; >> + info[9] = 0; >> + >> + info[3] = info[0] + info[1] + info[2]; >> + >> + /* Calculate info frame checKsum */ >> + for (i = 4; i < HDMI_SIZE_VSI_INFOFRAME; i++) >> + info[3] += info[i]; >> + info[3] = 0x100 - info[3]; >> + >> + for (i = 0; i < HDMI_SIZE_VSI_INFOFRAME; i++) >> + hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_ADDR + i, info[i]); >> + >> + hdmi_modb(hdmi, HDMI_PACKET_SEND_AUTO, m_PACKET_VSI_EN, >> + v_PACKET_VSI_EN(1)); >> + >> + return 0; > hdmi_vendor_infoframe_init / hdmi_vendor_infoframe_pack? > > You can probably use the same function to upload the control packet > too - something like: > > static int inno_hdmi_upload_frame(struct inno_hdmi *hdmi, int setup_rc, > union hdmi_infoframe *frame, u32 mask, u32 disable, u32 enable) > { > if (mask) > hdmi_modb(hdmi, HDMI_PACKET_SEND_AUTO, mask, disable); > > if (setup_rc >= 0) { > u8 packed_frame[YOUR_MAXIMUM_INFO_FRAME_SIZE]; > ssize_t rc, i; > > rc = hdmi_infoframe_pack(frame, packed_frame, > sizeof(packed_frame)); > if (rc < 0) > return rc; > > for (i = 0; i < rc; i++) > hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_ADDR + i, buf[i]); > > if (mask) > hdmi_modb(hdmi, HDMI_PACKET_SEND_AUTO, mask, enable); > } > > return setup_rc; > } > > static int inno_hdmi_config_video_vsi(struct inno_hdmi *hdmi) > { > union hdmi_infoframe frame; > > rc = drm_hdmi_vendor_infoframe_from_display_mode(&frame.vendor.hdmi, > the_drm_display_mode); > > return inno_hdmi_upload_frame(hdmi, rc, &frame, m_PACKET_VSI_EN, > v_PACKET_VSI_EN(0), v_PACKET_VSI_EN(1)); > } > Ah.... appreciate for the code, it's great suggest, thanks. Also I found a mistaken with previous "inno_hdmi_config_video_vsi" function that we don't need to send the HDMI vendor infoframes if we are not using a 4K or stereoscopic 3D mode. :) >> +static int inno_hdmi_i2c_wait(struct inno_hdmi *hdmi) >> +{ >> + struct inno_hdmi_i2c *i2c = hdmi->i2c; >> + int stat; >> + >> + stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10); >> + if (!stat) { >> + stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10); >> + if (!stat) >> + return -EAGAIN; > What's the reason for this double wait_for_completion? This probably > needs a comment, otherwise it looks like some weird cut-n-paste error. Yep, you 're right, don't need double wait here. >> +static int inno_hdmi_i2c_write(struct inno_hdmi *hdmi, struct i2c_msg *msgs) >> +{ >> + struct inno_hdmi_i2c *i2c = hdmi->i2c; > You seem to have a local i2c pointer... Ah, got it ;) >> + >> + /* >> + * The DDC module only support read EDID message, so >> + * we assume that each word write to this i2c adapter >> + * should be the offset of EDID word address. >> + */ >> + if ((msgs->len != 1) || >> + ((msgs->addr != DDC_ADDR) && (msgs->addr != DDC_SEGMENT_ADDR))) >> + return -EINVAL; >> + >> + reinit_completion(&i2c->cmp); >> + >> + if (msgs->addr == DDC_SEGMENT_ADDR) >> + hdmi->i2c->segment_addr = msgs->buf[0]; >> + if (msgs->addr == DDC_ADDR) >> + hdmi->i2c->ddc_addr = msgs->buf[0]; >> + >> + /* Set edid fifo first addr */ >> + hdmi_writeb(hdmi, HDMI_EDID_FIFO_OFFSET, 0x00); >> + >> + /* Set edid word address 0x00/0x80 */ >> + hdmi_writeb(hdmi, HDMI_EDID_WORD_ADDR, hdmi->i2c->ddc_addr); >> + >> + /* Set edid segment pointer */ >> + hdmi_writeb(hdmi, HDMI_EDID_SEGMENT_POINTER, hdmi->i2c->segment_addr); > but then you don't use it in the four locations above. > > Thanks, - Yakir From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757224AbcASDuk (ORCPT ); Mon, 18 Jan 2016 22:50:40 -0500 Received: from lucky1.263xmail.com ([211.157.147.132]:51407 "EHLO lucky1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757134AbcASDuh (ORCPT ); Mon, 18 Jan 2016 22:50:37 -0500 X-263anti-spam: KSV:0; X-MAIL-GRAY: 1 X-MAIL-DELIVERY: 0 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED: 0 X-RL-SENDER: ykk@rock-chips.com X-FST-TO: zhengyang@rock-chips.com X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: ykk@rock-chips.com X-UNIQUE-TAG: <0745bf2e2e03e974b5a3c40e4914b86f> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH v4] drm/rockchip: hdmi: add Innosilicon HDMI support To: Russell King - ARM Linux References: <1453128123-29155-1-git-send-email-ykk@rock-chips.com> <1453128140-29208-1-git-send-email-ykk@rock-chips.com> <20160118171514.GG19062@n2100.arm.linux.org.uk> Cc: Mark Yao , Heiko Stuebner , Mark Rutland , devicetree@vger.kernel.org, Pawel Moll , Ian Campbell , David Airlie , Ben Chan , Ken Mixte , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-rockchip@lists.infradead.org, Rob Herring , Kumar Gala , Thierry Reding , linux-arm-kernel@lists.infradead.org, Zheng Yang From: Yakir Yang Message-ID: <569DB285.1000009@rock-chips.com> Date: Tue, 19 Jan 2016 11:50:29 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <20160118171514.GG19062@n2100.arm.linux.org.uk> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Russell, Thanks for your comments :-D On 01/19/2016 01:15 AM, Russell King - ARM Linux wrote: > Hi, > > Some comments below... > > On Mon, Jan 18, 2016 at 10:42:20PM +0800, Yakir Yang wrote: >> +static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi) >> +{ >> + char info[HDMI_SIZE_AVI_INFOFRAME] = {0}; >> + int avi_color_mode; >> + int i; >> + >> + hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_BUF_INDEX, INFOFRAME_AVI); >> + >> + info[0] = 0x82; >> + info[1] = 0x02; >> + info[2] = 0x0D; >> + info[3] = info[0] + info[1] + info[2]; >> + >> + if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) >> + avi_color_mode = AVI_COLOR_MODE_RGB; >> + else if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_YUV444) >> + avi_color_mode = AVI_COLOR_MODE_YCBCR444; >> + else if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_YUV422) >> + avi_color_mode = AVI_COLOR_MODE_YCBCR422; >> + else >> + avi_color_mode = AVI_COLOR_MODE_RGB; >> + >> + info[4] = (avi_color_mode << 5); >> + info[5] = (AVI_COLORIMETRY_NO_DATA << 6) | >> + (AVI_CODED_FRAME_ASPECT_NO_DATA << 4) | >> + ACTIVE_ASPECT_RATE_SAME_AS_CODED_FRAME; >> + >> + info[6] = 0; >> + info[7] = hdmi->hdmi_data.vic; >> + >> + if (hdmi->hdmi_data.vic == 6 || hdmi->hdmi_data.vic == 7 || >> + hdmi->hdmi_data.vic == 21 || hdmi->hdmi_data.vic == 22) >> + info[8] = 1; >> + else >> + info[8] = 0; >> + >> + /* Calculate avi info frame checKsum */ >> + for (i = 4; i < HDMI_SIZE_AVI_INFOFRAME; i++) >> + info[3] += info[i]; >> + info[3] = 0x100 - info[3]; >> + >> + for (i = 0; i < HDMI_SIZE_AVI_INFOFRAME; i++) >> + hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_ADDR + i, info[i]); >> + >> + return 0; > Is there a reason why the helpers in drivers/video/hdmi.c can't be used > to generate this? > > hdmi_avi_infoframe_init / hdmi_avi_infoframe_pack and > drm_hdmi_avi_infoframe_from_display_mode ? Great, thanks for point out, it would make code much clean with those helper functions. >> +} >> + >> +static int inno_hdmi_config_video_vsi(struct inno_hdmi *hdmi) >> +{ >> + char info[HDMI_SIZE_VSI_INFOFRAME] = {0}; >> + int i; >> + >> + hdmi_modb(hdmi, HDMI_PACKET_SEND_AUTO, m_PACKET_VSI_EN, >> + v_PACKET_VSI_EN(0)); >> + >> + hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_BUF_INDEX, INFOFRAME_VSI); >> + >> + /* Header Bytes */ >> + info[0] = 0x81; >> + info[1] = 0x01; >> + >> + /* PB1 - PB3 contain the 24bit IEEE Registration Identifier */ >> + info[4] = 0x03; >> + info[5] = 0x0c; >> + info[6] = 0x00; >> + >> + /* PB4 - HDMI_Video_Format into bits 7:5 */ >> + info[7] = 0; >> + >> + /* >> + * PB5 - Depending on the video format, this byte will contain >> + * either the HDMI_VIC code in buts 7:0, OR the 3D_Structure in >> + * bits 7:4. >> + */ >> + info[2] = 0x06 - 2; >> + info[8] = 0; >> + info[9] = 0; >> + >> + info[3] = info[0] + info[1] + info[2]; >> + >> + /* Calculate info frame checKsum */ >> + for (i = 4; i < HDMI_SIZE_VSI_INFOFRAME; i++) >> + info[3] += info[i]; >> + info[3] = 0x100 - info[3]; >> + >> + for (i = 0; i < HDMI_SIZE_VSI_INFOFRAME; i++) >> + hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_ADDR + i, info[i]); >> + >> + hdmi_modb(hdmi, HDMI_PACKET_SEND_AUTO, m_PACKET_VSI_EN, >> + v_PACKET_VSI_EN(1)); >> + >> + return 0; > hdmi_vendor_infoframe_init / hdmi_vendor_infoframe_pack? > > You can probably use the same function to upload the control packet > too - something like: > > static int inno_hdmi_upload_frame(struct inno_hdmi *hdmi, int setup_rc, > union hdmi_infoframe *frame, u32 mask, u32 disable, u32 enable) > { > if (mask) > hdmi_modb(hdmi, HDMI_PACKET_SEND_AUTO, mask, disable); > > if (setup_rc >= 0) { > u8 packed_frame[YOUR_MAXIMUM_INFO_FRAME_SIZE]; > ssize_t rc, i; > > rc = hdmi_infoframe_pack(frame, packed_frame, > sizeof(packed_frame)); > if (rc < 0) > return rc; > > for (i = 0; i < rc; i++) > hdmi_writeb(hdmi, HDMI_CONTROL_PACKET_ADDR + i, buf[i]); > > if (mask) > hdmi_modb(hdmi, HDMI_PACKET_SEND_AUTO, mask, enable); > } > > return setup_rc; > } > > static int inno_hdmi_config_video_vsi(struct inno_hdmi *hdmi) > { > union hdmi_infoframe frame; > > rc = drm_hdmi_vendor_infoframe_from_display_mode(&frame.vendor.hdmi, > the_drm_display_mode); > > return inno_hdmi_upload_frame(hdmi, rc, &frame, m_PACKET_VSI_EN, > v_PACKET_VSI_EN(0), v_PACKET_VSI_EN(1)); > } > Ah.... appreciate for the code, it's great suggest, thanks. Also I found a mistaken with previous "inno_hdmi_config_video_vsi" function that we don't need to send the HDMI vendor infoframes if we are not using a 4K or stereoscopic 3D mode. :) >> +static int inno_hdmi_i2c_wait(struct inno_hdmi *hdmi) >> +{ >> + struct inno_hdmi_i2c *i2c = hdmi->i2c; >> + int stat; >> + >> + stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10); >> + if (!stat) { >> + stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10); >> + if (!stat) >> + return -EAGAIN; > What's the reason for this double wait_for_completion? This probably > needs a comment, otherwise it looks like some weird cut-n-paste error. Yep, you 're right, don't need double wait here. >> +static int inno_hdmi_i2c_write(struct inno_hdmi *hdmi, struct i2c_msg *msgs) >> +{ >> + struct inno_hdmi_i2c *i2c = hdmi->i2c; > You seem to have a local i2c pointer... Ah, got it ;) >> + >> + /* >> + * The DDC module only support read EDID message, so >> + * we assume that each word write to this i2c adapter >> + * should be the offset of EDID word address. >> + */ >> + if ((msgs->len != 1) || >> + ((msgs->addr != DDC_ADDR) && (msgs->addr != DDC_SEGMENT_ADDR))) >> + return -EINVAL; >> + >> + reinit_completion(&i2c->cmp); >> + >> + if (msgs->addr == DDC_SEGMENT_ADDR) >> + hdmi->i2c->segment_addr = msgs->buf[0]; >> + if (msgs->addr == DDC_ADDR) >> + hdmi->i2c->ddc_addr = msgs->buf[0]; >> + >> + /* Set edid fifo first addr */ >> + hdmi_writeb(hdmi, HDMI_EDID_FIFO_OFFSET, 0x00); >> + >> + /* Set edid word address 0x00/0x80 */ >> + hdmi_writeb(hdmi, HDMI_EDID_WORD_ADDR, hdmi->i2c->ddc_addr); >> + >> + /* Set edid segment pointer */ >> + hdmi_writeb(hdmi, HDMI_EDID_SEGMENT_POINTER, hdmi->i2c->segment_addr); > but then you don't use it in the four locations above. > > Thanks, - Yakir