From mboxrd@z Thu Jan 1 00:00:00 1970 From: Archit Taneja Subject: Re: [PATCH 1/3 v2] drm/i2c/adv7511: Add audio support Date: Tue, 29 Mar 2016 13:35:25 +0530 Message-ID: <56FA3745.3000807@codeaurora.org> References: <15b1936d307912ba12df7fa09ec3a7a80ac5fcef.1459174494.git.joabreu@synopsys.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <15b1936d307912ba12df7fa09ec3a7a80ac5fcef.1459174494.git.joabreu@synopsys.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Jose Abreu , linux-snps-arc@lists.infradead.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, alsa-devel@alsa-project.org, devicetree@vger.kernel.org Cc: tixy@linaro.org, mark.rutland@arm.com, laurent.pinchart+renesas@ideasonboard.com, pawel.moll@arm.com, ijc+devicetree@hellion.org.uk, Vineet.Gupta1@synopsys.com, Alexey.Brodkin@synopsys.com, lgirdwood@gmail.com, robh+dt@kernel.org, nariman@opensource.wolfsonmicro.com, yitian.bu@tangramtek.com, wsa+renesas@sang-engineering.com, broonie@kernel.org, Maruthi.Bayyavarapu@amd.com, galak@codeaurora.org, buyitian@gmail.com, alexander.deucher@amd.com, tiwai@suse.com, perex@perex.cz, CARLOS.PALMINHA@synopsys.com List-Id: alsa-devel@alsa-project.org SGksCgpPbiAwMy8yOC8yMDE2IDA4OjA2IFBNLCBKb3NlIEFicmV1IHdyb3RlOgo+IFRoaXMgcGF0 Y2ggYWRkcyBhdWRpbyBzdXBwb3J0IGZvciB0aGUgQURWNzUxMSBIRE1JIHRyYW5zbWl0dGVyCj4g dXNpbmcgQUxTQSBTb0MuCj4KPiBUaGUgY29kZSB3YXMgcG9ydGVkIGZyb20gQW5hbG9nIERldmlj ZXMgbGludXggdHJlZSBmcm9tCj4gY29tbWl0IDE3NzBjNGExZTMyYiAoIk1lcmdlIHJlbW90ZS10 cmFja2luZyBicmFuY2gKPiAneGlsaW54L21hc3RlcicgaW50byB4Y29tbV96eW5xIiksIHdoaWNo IGlzIGF2YWlsYWJsZSBhdDoKPiAJLSBodHRwczovL2dpdGh1Yi5jb20vYW5hbG9nZGV2aWNlc2lu Yy9saW51eC8KPgo+IFRoZSBtYWluIGNvcmUgZmlsZSB3YXMgcmVuYW1lZCBmcm9tIGFkdjc1MTEu YyB0byBhZHY3NTExX2NvcmUuYwo+IHNvIHRoYXQgYXVkaW8gYW5kIHZpZGVvIGNvbXBpbGUgaW50 byBhIHNpbmdsZSBhZHY3NTExLmtvIG1vZHVsZQo+IGFuZCB0byBrZWVwIHVwIHdpdGggQW5hbG9n IERldmljZXMga2VybmVsIHRyZWUuCj4KPiBUaGUgYXVkaW8gY2FuIGJlIGRpc2FibGVkIHVzaW5n IG1lbnUtY29uZmlnIHNvIGl0IGlzIHBvc3NpYmxlCj4gdG8gdXNlIG9ubHkgdmlkZW8gbW9kZS4K Pgo+IFRoZSBIRE1JIG1vZGUgaXMgYXV0b21hdGljYWxseSBzdGFydGVkIGF0IGJvb3QgYW5kIHRo ZSBhdWRpbwo+ICh3aGVuIGVuYWJsZWQpIHJlZ2lzdGVycyBhcyBhIGNvZGVjIGludG8gQUxTQS4K CklzIHRoZXJlIGEgcmVhc29uIHdoeSB3ZSBzZXQgdGhlIG1vZGUgdG8gSERNSSBhdCBwcm9iZSBp dHNlbGY/ClNob3VsZG4ndCBpdCBiZSBva2F5IHRvIHNldCB0aGUgbW9kZSBsYXRlciBvbmNlIHdl IHJlYWQgdGhlCkVESUQgb2ZmIHRoZSBwYW5lbD8KClNvbWUgbW9yZSBjb21tZW50cyBiZWxvdy4K Cj4KPiBTUERJRiBEQUkgZm9ybWF0IHdhcyBhbHNvIGFkZGVkIHRvIEFTb0MgYXMgaXQgaXMgcmVx dWlyZWQKPiBieSBhZHY3NTExIGF1ZGlvLgo+Cj4gU2lnbmVkLW9mZi1ieTogSm9zZSBBYnJldSA8 am9hYnJldUBzeW5vcHN5cy5jb20+Cj4gLS0tCj4KPiBObyBjaGFuZ2VzIHYxIC0+IHYyLgo+Cj4g ICBkcml2ZXJzL2dwdS9kcm0vaTJjL0tjb25maWcgICAgICAgICB8ICAgMTEgKwo+ICAgZHJpdmVy cy9ncHUvZHJtL2kyYy9NYWtlZmlsZSAgICAgICAgfCAgICAyICsKPiAgIGRyaXZlcnMvZ3B1L2Ry bS9pMmMvYWR2NzUxMS5jICAgICAgIHwgMTAyNCAtLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLQo+ICAgZHJpdmVycy9ncHUvZHJtL2kyYy9hZHY3NTExLmggICAgICAgfCAgIDQxICsr Cj4gICBkcml2ZXJzL2dwdS9kcm0vaTJjL2Fkdjc1MTFfYXVkaW8uYyB8ICAzMTAgKysrKysrKysr KysKPiAgIGRyaXZlcnMvZ3B1L2RybS9pMmMvYWR2NzUxMV9jb3JlLmMgIHwgMTAwNSArKysrKysr KysrKysrKysrKysrKysrKysrKysrKysrKysrCj4gICBpbmNsdWRlL3NvdW5kL3NvYy1kYWkuaCAg ICAgICAgICAgICB8ICAgIDEgKwo+ICAgNyBmaWxlcyBjaGFuZ2VkLCAxMzcwIGluc2VydGlvbnMo KyksIDEwMjQgZGVsZXRpb25zKC0pCj4gICBkZWxldGUgbW9kZSAxMDA2NDQgZHJpdmVycy9ncHUv ZHJtL2kyYy9hZHY3NTExLmMKPiAgIGNyZWF0ZSBtb2RlIDEwMDY0NCBkcml2ZXJzL2dwdS9kcm0v aTJjL2Fkdjc1MTFfYXVkaW8uYwo+ICAgY3JlYXRlIG1vZGUgMTAwNjQ0IGRyaXZlcnMvZ3B1L2Ry bS9pMmMvYWR2NzUxMV9jb3JlLmMKCjxzbmlwPgoKPiArCj4gK3N0YXRpYyBpbnQgYWR2NzUxMV9w cm9iZShzdHJ1Y3QgaTJjX2NsaWVudCAqaTJjLCBjb25zdCBzdHJ1Y3QgaTJjX2RldmljZV9pZCAq aWQpCj4gK3sKPiArCXN0cnVjdCBhZHY3NTExX2xpbmtfY29uZmlnIGxpbmtfY29uZmlnOwo+ICsJ c3RydWN0IGFkdjc1MTEgKmFkdjc1MTE7Cj4gKwlzdHJ1Y3QgZGV2aWNlICpkZXYgPSAmaTJjLT5k ZXY7Cj4gKwl1bnNpZ25lZCBpbnQgdmFsOwo+ICsJaW50IHJldDsKPiArCj4gKwlpZiAoIWRldi0+ b2Zfbm9kZSkKPiArCQlyZXR1cm4gLUVJTlZBTDsKPiArCj4gKwlhZHY3NTExID0gZGV2bV9remFs bG9jKGRldiwgc2l6ZW9mKCphZHY3NTExKSwgR0ZQX0tFUk5FTCk7Cj4gKwlpZiAoIWFkdjc1MTEp Cj4gKwkJcmV0dXJuIC1FTk9NRU07Cj4gKwo+ICsJYWR2NzUxMS0+cG93ZXJlZCA9IGZhbHNlOwo+ ICsJYWR2NzUxMS0+c3RhdHVzID0gY29ubmVjdG9yX3N0YXR1c19kaXNjb25uZWN0ZWQ7Cj4gKwo+ ICsJcmV0ID0gYWR2NzUxMV9wYXJzZV9kdChkZXYtPm9mX25vZGUsICZsaW5rX2NvbmZpZyk7Cj4g KwlpZiAocmV0KQo+ICsJCXJldHVybiByZXQ7Cj4gKwo+ICsJLyoKPiArCSAqIFRoZSBwb3dlciBk b3duIEdQSU8gaXMgb3B0aW9uYWwuIElmIHByZXNlbnQsIHRvZ2dsZSBpdCBmcm9tIGFjdGl2ZSB0 bwo+ICsJICogaW5hY3RpdmUgdG8gd2FrZSB1cCB0aGUgZW5jb2Rlci4KPiArCSAqLwo+ICsJYWR2 NzUxMS0+Z3Bpb19wZCA9IGRldm1fZ3Bpb2RfZ2V0X29wdGlvbmFsKGRldiwgInBkIiwgR1BJT0Rf T1VUX0hJR0gpOwo+ICsJaWYgKElTX0VSUihhZHY3NTExLT5ncGlvX3BkKSkKPiArCQlyZXR1cm4g UFRSX0VSUihhZHY3NTExLT5ncGlvX3BkKTsKPiArCj4gKwlpZiAoYWR2NzUxMS0+Z3Bpb19wZCkg ewo+ICsJCW1kZWxheSg1KTsKPiArCQlncGlvZF9zZXRfdmFsdWVfY2Fuc2xlZXAoYWR2NzUxMS0+ Z3Bpb19wZCwgMCk7Cj4gKwl9Cj4gKwo+ICsJYWR2NzUxMS0+cmVnbWFwID0gZGV2bV9yZWdtYXBf aW5pdF9pMmMoaTJjLCAmYWR2NzUxMV9yZWdtYXBfY29uZmlnKTsKPiArCWlmIChJU19FUlIoYWR2 NzUxMS0+cmVnbWFwKSkKPiArCQlyZXR1cm4gUFRSX0VSUihhZHY3NTExLT5yZWdtYXApOwo+ICsK PiArCXJldCA9IHJlZ21hcF9yZWFkKGFkdjc1MTEtPnJlZ21hcCwgQURWNzUxMV9SRUdfQ0hJUF9S RVZJU0lPTiwgJnZhbCk7Cj4gKwlpZiAocmV0KQo+ICsJCXJldHVybiByZXQ7Cj4gKwlkZXZfZGJn KGRldiwgIlJldi4gJWRcbiIsIHZhbCk7Cj4gKwo+ICsJcmV0ID0gcmVnbWFwX3JlZ2lzdGVyX3Bh dGNoKGFkdjc1MTEtPnJlZ21hcCwgYWR2NzUxMV9maXhlZF9yZWdpc3RlcnMsCj4gKwkJCQkgICAg QVJSQVlfU0laRShhZHY3NTExX2ZpeGVkX3JlZ2lzdGVycykpOwo+ICsJaWYgKHJldCkKPiArCQly ZXR1cm4gcmV0Owo+ICsKPiArCXJlZ21hcF93cml0ZShhZHY3NTExLT5yZWdtYXAsIEFEVjc1MTFf UkVHX0VESURfSTJDX0FERFIsIGVkaWRfaTJjX2FkZHIpOwo+ICsJcmVnbWFwX3dyaXRlKGFkdjc1 MTEtPnJlZ21hcCwgQURWNzUxMV9SRUdfUEFDS0VUX0kyQ19BRERSLAo+ICsJCSAgICAgcGFja2V0 X2kyY19hZGRyKTsKPiArCXJlZ21hcF93cml0ZShhZHY3NTExLT5yZWdtYXAsIEFEVjc1MTFfUkVH X0NFQ19JMkNfQUREUiwgY2VjX2kyY19hZGRyKTsKPiArCWFkdjc1MTFfcGFja2V0X2Rpc2FibGUo YWR2NzUxMSwgMHhmZmZmKTsKPiArCj4gKwlhZHY3NTExLT5pMmNfbWFpbiA9IGkyYzsKPiArCWFk djc1MTEtPmkyY19lZGlkID0gaTJjX25ld19kdW1teShpMmMtPmFkYXB0ZXIsIGVkaWRfaTJjX2Fk ZHIgPj4gMSk7Cj4gKwlpZiAoIWFkdjc1MTEtPmkyY19lZGlkKQo+ICsJCXJldHVybiAtRU5PTUVN Owo+ICsKPiArCWlmIChpMmMtPmlycSkgewo+ICsJCWluaXRfd2FpdHF1ZXVlX2hlYWQoJmFkdjc1 MTEtPndxKTsKPiArCj4gKwkJcmV0ID0gZGV2bV9yZXF1ZXN0X3RocmVhZGVkX2lycShkZXYsIGky Yy0+aXJxLCBOVUxMLAo+ICsJCQkJCQlhZHY3NTExX2lycV9oYW5kbGVyLAo+ICsJCQkJCQlJUlFG X09ORVNIT1QsIGRldl9uYW1lKGRldiksCj4gKwkJCQkJCWFkdjc1MTEpOwo+ICsJCWlmIChyZXQp Cj4gKwkJCWdvdG8gZXJyX2kyY191bnJlZ2lzdGVyX2RldmljZTsKPiArCX0KPiArCj4gKwkvKiBD RUMgaXMgdW51c2VkIGZvciBub3cgKi8KPiArCXJlZ21hcF93cml0ZShhZHY3NTExLT5yZWdtYXAs IEFEVjc1MTFfUkVHX0NFQ19DVFJMLAo+ICsJCSAgICAgQURWNzUxMV9DRUNfQ1RSTF9QT1dFUl9E T1dOKTsKPiArCj4gKwlhZHY3NTExX3Bvd2VyX29mZihhZHY3NTExKTsKPiArCj4gKwlpMmNfc2V0 X2NsaWVudGRhdGEoaTJjLCBhZHY3NTExKTsKPiArCj4gKyNpZmRlZiBDT05GSUdfRFJNX0kyQ19B RFY3NTExX0FVRElPCj4gKwlhZHY3NTExX2F1ZGlvX2luaXQoJmkyYy0+ZGV2KTsKPiArI2VuZGlm CgpJZiB3ZSBpbnRlbmQgdG8gaGF2ZSBtb3JlIGF1ZGlvIGZ1bmNzIGJlaW5nIHVzZWQgYnkgdGhl IGNvcmUgaW4gdGhlCmZ1dHVyZSwgaXQgd291bGQgYmUgbmljZSB0byBoYXZlIE5PUCBhdWRpbyBm dW5jcyByYXRoZXIgdGhhbiBoYXZpbmcKbXVsdGlwbGUgI2lmZGVmIGNoZWNrcyBpbiB0aGUgZHJp dmVyIHdoZW4gQ09ORklHX0RSTV9JMkNfQURWNzUxMV9BVURJTwppc24ndCBzZXQuCgo+ICsKPiAr CWFkdjc1MTFfc2V0X2xpbmtfY29uZmlnKGFkdjc1MTEsICZsaW5rX2NvbmZpZyk7Cj4gKwo+ICsJ LyogRW5hYmxlIEhETUkgbW9kZSAqLwo+ICsJcmVnbWFwX3VwZGF0ZV9iaXRzKGFkdjc1MTEtPnJl Z21hcCwgQURWNzUxMV9SRUdfSERDUF9IRE1JX0NGRywKPiArCQkJQURWNzUxMV9IRE1JX0NGR19N T0RFX01BU0ssCj4gKwkJCUFEVjc1MTFfSERNSV9DRkdfTU9ERV9IRE1JKTsKPiArCj4gKwlyZXR1 cm4gMDsKPiArCj4gK2Vycl9pMmNfdW5yZWdpc3Rlcl9kZXZpY2U6Cj4gKwlpMmNfdW5yZWdpc3Rl cl9kZXZpY2UoYWR2NzUxMS0+aTJjX2VkaWQpOwo+ICsKPiArCXJldHVybiByZXQ7Cj4gK30KPiAr Cj4gK3N0YXRpYyBpbnQgYWR2NzUxMV9yZW1vdmUoc3RydWN0IGkyY19jbGllbnQgKmkyYykKPiAr ewo+ICsJc3RydWN0IGFkdjc1MTEgKmFkdjc1MTEgPSBpMmNfZ2V0X2NsaWVudGRhdGEoaTJjKTsK PiArCgpBcmUgd2UgbWlzc2luZyBhIGNhbGwgdG8gYWR2NzUxMV9hdWRpb19leGl0KCkgaGVyZT8K ClRoYW5rcywKQXJjaGl0CgotLSAKVGhlIFF1YWxjb21tIElubm92YXRpb24gQ2VudGVyLCBJbmMu IGlzIGEgbWVtYmVyIG9mIHRoZSBDb2RlIEF1cm9yYSAKRm9ydW0sIGhvc3RlZCBieSBUaGUgTGlu dXggRm91bmRhdGlvbgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5v cmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2 ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: architt@codeaurora.org (Archit Taneja) Date: Tue, 29 Mar 2016 13:35:25 +0530 Subject: [PATCH 1/3 v2] drm/i2c/adv7511: Add audio support In-Reply-To: <15b1936d307912ba12df7fa09ec3a7a80ac5fcef.1459174494.git.joabreu@synopsys.com> References: <15b1936d307912ba12df7fa09ec3a7a80ac5fcef.1459174494.git.joabreu@synopsys.com> List-ID: Message-ID: <56FA3745.3000807@codeaurora.org> To: linux-snps-arc@lists.infradead.org Hi, On 03/28/2016 08:06 PM, Jose Abreu wrote: > This patch adds audio support for the ADV7511 HDMI transmitter > using ALSA SoC. > > The code was ported from Analog Devices linux tree from > commit 1770c4a1e32b ("Merge remote-tracking branch > 'xilinx/master' into xcomm_zynq"), which is available at: > - https://github.com/analogdevicesinc/linux/ > > The main core file was renamed from adv7511.c to adv7511_core.c > so that audio and video compile into a single adv7511.ko module > and to keep up with Analog Devices kernel tree. > > The audio can be disabled using menu-config so it is possible > to use only video mode. > > The HDMI mode is automatically started at boot and the audio > (when enabled) registers as a codec into ALSA. Is there a reason why we set the mode to HDMI at probe itself? Shouldn't it be okay to set the mode later once we read the EDID off the panel? Some more comments below. > > SPDIF DAI format was also added to ASoC as it is required > by adv7511 audio. > > Signed-off-by: Jose Abreu > --- > > No changes v1 -> v2. > > drivers/gpu/drm/i2c/Kconfig | 11 + > drivers/gpu/drm/i2c/Makefile | 2 + > drivers/gpu/drm/i2c/adv7511.c | 1024 ----------------------------------- > drivers/gpu/drm/i2c/adv7511.h | 41 ++ > drivers/gpu/drm/i2c/adv7511_audio.c | 310 +++++++++++ > drivers/gpu/drm/i2c/adv7511_core.c | 1005 ++++++++++++++++++++++++++++++++++ > include/sound/soc-dai.h | 1 + > 7 files changed, 1370 insertions(+), 1024 deletions(-) > delete mode 100644 drivers/gpu/drm/i2c/adv7511.c > create mode 100644 drivers/gpu/drm/i2c/adv7511_audio.c > create mode 100644 drivers/gpu/drm/i2c/adv7511_core.c > + > +static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) > +{ > + struct adv7511_link_config link_config; > + struct adv7511 *adv7511; > + struct device *dev = &i2c->dev; > + unsigned int val; > + int ret; > + > + if (!dev->of_node) > + return -EINVAL; > + > + adv7511 = devm_kzalloc(dev, sizeof(*adv7511), GFP_KERNEL); > + if (!adv7511) > + return -ENOMEM; > + > + adv7511->powered = false; > + adv7511->status = connector_status_disconnected; > + > + ret = adv7511_parse_dt(dev->of_node, &link_config); > + if (ret) > + return ret; > + > + /* > + * The power down GPIO is optional. If present, toggle it from active to > + * inactive to wake up the encoder. > + */ > + adv7511->gpio_pd = devm_gpiod_get_optional(dev, "pd", GPIOD_OUT_HIGH); > + if (IS_ERR(adv7511->gpio_pd)) > + return PTR_ERR(adv7511->gpio_pd); > + > + if (adv7511->gpio_pd) { > + mdelay(5); > + gpiod_set_value_cansleep(adv7511->gpio_pd, 0); > + } > + > + adv7511->regmap = devm_regmap_init_i2c(i2c, &adv7511_regmap_config); > + if (IS_ERR(adv7511->regmap)) > + return PTR_ERR(adv7511->regmap); > + > + ret = regmap_read(adv7511->regmap, ADV7511_REG_CHIP_REVISION, &val); > + if (ret) > + return ret; > + dev_dbg(dev, "Rev. %d\n", val); > + > + ret = regmap_register_patch(adv7511->regmap, adv7511_fixed_registers, > + ARRAY_SIZE(adv7511_fixed_registers)); > + if (ret) > + return ret; > + > + regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr); > + regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, > + packet_i2c_addr); > + regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, cec_i2c_addr); > + adv7511_packet_disable(adv7511, 0xffff); > + > + adv7511->i2c_main = i2c; > + adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1); > + if (!adv7511->i2c_edid) > + return -ENOMEM; > + > + if (i2c->irq) { > + init_waitqueue_head(&adv7511->wq); > + > + ret = devm_request_threaded_irq(dev, i2c->irq, NULL, > + adv7511_irq_handler, > + IRQF_ONESHOT, dev_name(dev), > + adv7511); > + if (ret) > + goto err_i2c_unregister_device; > + } > + > + /* CEC is unused for now */ > + regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL, > + ADV7511_CEC_CTRL_POWER_DOWN); > + > + adv7511_power_off(adv7511); > + > + i2c_set_clientdata(i2c, adv7511); > + > +#ifdef CONFIG_DRM_I2C_ADV7511_AUDIO > + adv7511_audio_init(&i2c->dev); > +#endif If we intend to have more audio funcs being used by the core in the future, it would be nice to have NOP audio funcs rather than having multiple #ifdef checks in the driver when CONFIG_DRM_I2C_ADV7511_AUDIO isn't set. > + > + adv7511_set_link_config(adv7511, &link_config); > + > + /* Enable HDMI mode */ > + regmap_update_bits(adv7511->regmap, ADV7511_REG_HDCP_HDMI_CFG, > + ADV7511_HDMI_CFG_MODE_MASK, > + ADV7511_HDMI_CFG_MODE_HDMI); > + > + return 0; > + > +err_i2c_unregister_device: > + i2c_unregister_device(adv7511->i2c_edid); > + > + return ret; > +} > + > +static int adv7511_remove(struct i2c_client *i2c) > +{ > + struct adv7511 *adv7511 = i2c_get_clientdata(i2c); > + Are we missing a call to adv7511_audio_exit() here? Thanks, Archit -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751826AbcC2IFn (ORCPT ); Tue, 29 Mar 2016 04:05:43 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:57388 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751217AbcC2IFi (ORCPT ); Tue, 29 Mar 2016 04:05:38 -0400 Subject: Re: [PATCH 1/3 v2] drm/i2c/adv7511: Add audio support To: Jose Abreu , linux-snps-arc@lists.infradead.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, alsa-devel@alsa-project.org, devicetree@vger.kernel.org References: <15b1936d307912ba12df7fa09ec3a7a80ac5fcef.1459174494.git.joabreu@synopsys.com> Cc: tixy@linaro.org, mark.rutland@arm.com, broonie@kernel.org, Alexey.Brodkin@synopsys.com, lgirdwood@gmail.com, yitian.bu@tangramtek.com, wsa+renesas@sang-engineering.com, laurent.pinchart+renesas@ideasonboard.com, nariman@opensource.wolfsonmicro.com, Maruthi.Bayyavarapu@amd.com, pawel.moll@arm.com, ijc+devicetree@hellion.org.uk, Vineet.Gupta1@synopsys.com, buyitian@gmail.com, perex@perex.cz, tiwai@suse.com, robh+dt@kernel.org, galak@codeaurora.org, alexander.deucher@amd.com, CARLOS.PALMINHA@synopsys.com From: Archit Taneja Message-ID: <56FA3745.3000807@codeaurora.org> Date: Tue, 29 Mar 2016 13:35:25 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <15b1936d307912ba12df7fa09ec3a7a80ac5fcef.1459174494.git.joabreu@synopsys.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 03/28/2016 08:06 PM, Jose Abreu wrote: > This patch adds audio support for the ADV7511 HDMI transmitter > using ALSA SoC. > > The code was ported from Analog Devices linux tree from > commit 1770c4a1e32b ("Merge remote-tracking branch > 'xilinx/master' into xcomm_zynq"), which is available at: > - https://github.com/analogdevicesinc/linux/ > > The main core file was renamed from adv7511.c to adv7511_core.c > so that audio and video compile into a single adv7511.ko module > and to keep up with Analog Devices kernel tree. > > The audio can be disabled using menu-config so it is possible > to use only video mode. > > The HDMI mode is automatically started at boot and the audio > (when enabled) registers as a codec into ALSA. Is there a reason why we set the mode to HDMI at probe itself? Shouldn't it be okay to set the mode later once we read the EDID off the panel? Some more comments below. > > SPDIF DAI format was also added to ASoC as it is required > by adv7511 audio. > > Signed-off-by: Jose Abreu > --- > > No changes v1 -> v2. > > drivers/gpu/drm/i2c/Kconfig | 11 + > drivers/gpu/drm/i2c/Makefile | 2 + > drivers/gpu/drm/i2c/adv7511.c | 1024 ----------------------------------- > drivers/gpu/drm/i2c/adv7511.h | 41 ++ > drivers/gpu/drm/i2c/adv7511_audio.c | 310 +++++++++++ > drivers/gpu/drm/i2c/adv7511_core.c | 1005 ++++++++++++++++++++++++++++++++++ > include/sound/soc-dai.h | 1 + > 7 files changed, 1370 insertions(+), 1024 deletions(-) > delete mode 100644 drivers/gpu/drm/i2c/adv7511.c > create mode 100644 drivers/gpu/drm/i2c/adv7511_audio.c > create mode 100644 drivers/gpu/drm/i2c/adv7511_core.c > + > +static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) > +{ > + struct adv7511_link_config link_config; > + struct adv7511 *adv7511; > + struct device *dev = &i2c->dev; > + unsigned int val; > + int ret; > + > + if (!dev->of_node) > + return -EINVAL; > + > + adv7511 = devm_kzalloc(dev, sizeof(*adv7511), GFP_KERNEL); > + if (!adv7511) > + return -ENOMEM; > + > + adv7511->powered = false; > + adv7511->status = connector_status_disconnected; > + > + ret = adv7511_parse_dt(dev->of_node, &link_config); > + if (ret) > + return ret; > + > + /* > + * The power down GPIO is optional. If present, toggle it from active to > + * inactive to wake up the encoder. > + */ > + adv7511->gpio_pd = devm_gpiod_get_optional(dev, "pd", GPIOD_OUT_HIGH); > + if (IS_ERR(adv7511->gpio_pd)) > + return PTR_ERR(adv7511->gpio_pd); > + > + if (adv7511->gpio_pd) { > + mdelay(5); > + gpiod_set_value_cansleep(adv7511->gpio_pd, 0); > + } > + > + adv7511->regmap = devm_regmap_init_i2c(i2c, &adv7511_regmap_config); > + if (IS_ERR(adv7511->regmap)) > + return PTR_ERR(adv7511->regmap); > + > + ret = regmap_read(adv7511->regmap, ADV7511_REG_CHIP_REVISION, &val); > + if (ret) > + return ret; > + dev_dbg(dev, "Rev. %d\n", val); > + > + ret = regmap_register_patch(adv7511->regmap, adv7511_fixed_registers, > + ARRAY_SIZE(adv7511_fixed_registers)); > + if (ret) > + return ret; > + > + regmap_write(adv7511->regmap, ADV7511_REG_EDID_I2C_ADDR, edid_i2c_addr); > + regmap_write(adv7511->regmap, ADV7511_REG_PACKET_I2C_ADDR, > + packet_i2c_addr); > + regmap_write(adv7511->regmap, ADV7511_REG_CEC_I2C_ADDR, cec_i2c_addr); > + adv7511_packet_disable(adv7511, 0xffff); > + > + adv7511->i2c_main = i2c; > + adv7511->i2c_edid = i2c_new_dummy(i2c->adapter, edid_i2c_addr >> 1); > + if (!adv7511->i2c_edid) > + return -ENOMEM; > + > + if (i2c->irq) { > + init_waitqueue_head(&adv7511->wq); > + > + ret = devm_request_threaded_irq(dev, i2c->irq, NULL, > + adv7511_irq_handler, > + IRQF_ONESHOT, dev_name(dev), > + adv7511); > + if (ret) > + goto err_i2c_unregister_device; > + } > + > + /* CEC is unused for now */ > + regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL, > + ADV7511_CEC_CTRL_POWER_DOWN); > + > + adv7511_power_off(adv7511); > + > + i2c_set_clientdata(i2c, adv7511); > + > +#ifdef CONFIG_DRM_I2C_ADV7511_AUDIO > + adv7511_audio_init(&i2c->dev); > +#endif If we intend to have more audio funcs being used by the core in the future, it would be nice to have NOP audio funcs rather than having multiple #ifdef checks in the driver when CONFIG_DRM_I2C_ADV7511_AUDIO isn't set. > + > + adv7511_set_link_config(adv7511, &link_config); > + > + /* Enable HDMI mode */ > + regmap_update_bits(adv7511->regmap, ADV7511_REG_HDCP_HDMI_CFG, > + ADV7511_HDMI_CFG_MODE_MASK, > + ADV7511_HDMI_CFG_MODE_HDMI); > + > + return 0; > + > +err_i2c_unregister_device: > + i2c_unregister_device(adv7511->i2c_edid); > + > + return ret; > +} > + > +static int adv7511_remove(struct i2c_client *i2c) > +{ > + struct adv7511 *adv7511 = i2c_get_clientdata(i2c); > + Are we missing a call to adv7511_audio_exit() here? Thanks, Archit -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation