From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jean-Francois Moine Subject: Re: [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x Date: Tue, 4 Feb 2014 18:16:05 +0100 Message-ID: <20140204181605.5b837a70@armhf> References: <20140204133014.GA22609@sirena.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from smtp2-g21.free.fr (smtp2-g21.free.fr [212.27.42.2]) by alsa0.perex.cz (Postfix) with ESMTP id 142572619E1 for ; Tue, 4 Feb 2014 18:15:55 +0100 (CET) In-Reply-To: <20140204133014.GA22609@sirena.org.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Mark Brown Cc: alsa-devel@alsa-project.org, Russell King - ARM Linux , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Rob Clark , Dave Airlie , linux-arm-kernel@lists.infradead.org List-Id: alsa-devel@alsa-project.org T24gVHVlLCA0IEZlYiAyMDE0IDEzOjMwOjE0ICswMDAwCk1hcmsgQnJvd24gPGJyb29uaWVAa2Vy bmVsLm9yZz4gd3JvdGU6Cgo+IE9uIFN1biwgSmFuIDI2LCAyMDE0IGF0IDA3OjQ1OjM2UE0gKzAx MDAsIEplYW4tRnJhbmNvaXMgTW9pbmUgd3JvdGU6Cj4gCj4gPiArCS8qIGxvYWQgdGhlIG9wdGlv bmFsIENPREVDICovCj4gPiArCW9mX3BsYXRmb3JtX3BvcHVsYXRlKG5wLCBOVUxMLCBOVUxMLCAm Y2xpZW50LT5kZXYpOwo+ID4gKwo+IAo+IFdoeSBpcyB0aGlzIHVzaW5nIG9mX3BsYXRmb3JtX3Bv cHVsYXRlKCk/ICBUaGF0J3MgYSB2ZXJ5IG9kZCB3YXkgb2YKPiBkb2luZyB0aGluZ3MuCgpUaGUg aTJjIGRvZXMgbm90IHBvcHVsYXRlIHRoZSBzdWJub2RlcyBpbiB0aGUgRFQuIEkgZGlkIG5vdCBm aW5kIHdoeSwKYnV0LCB3aGF0IGlzIHN1cmUgaXMgdGhhdCBpZiBvZl9wbGF0Zm9ybV9wb3B1bGF0 ZSgpIGlzIG5vdCBjYWxsZWQsIHRoZQp0ZGEgQ09ERUMgbW9kdWxlIGlzIG5vdCBsb2FkZWQuCgpZ b3UgbWF5IGZpbmQgYW4gb3RoZXIgZXhhbXBsZSBpbiBkcml2ZXJzL21mZC90d2wtY29yZS5jLgoK PiA+ICtjb25maWcgU05EX1NPQ19UREE5OThYCj4gPiArCXRyaXN0YXRlCj4gPiArCWRlcGVuZHMg b24gT0YKPiA+ICsJZGVmYXVsdCB5IGlmIERSTV9JMkNfTlhQX1REQTk5OFg9eQo+ID4gKwlkZWZh dWx0IG0gaWYgRFJNX0kyQ19OWFBfVERBOTk4WD1tCj4gPiArCj4gCj4gTWFrZSB0aGlzIHZpc2li bGUgaWYgaXQgY2FuIGJlIHNlbGVjdGVkIGZyb20gRFQgc28gaXQgY2FuIGJlIHVzZWQgd2l0aAo+ IGdlbmVyaWMgY2FyZHMuCgpJIGRvbid0IHVuZGVyc3RhbmQuIFRoZSB0ZGEgQ09ERUMgY2FuIG9u bHkgYmUgdXNlZCB3aXRoIHRoZSBUREE5OTh4IEkyQwpkcml2ZXIuIEl0IG1pZ2h0IGhhdmUgYmVl biBpbmNsdWRlZCBpbiB0aGUgdGRhOTk4eCBzb3VyY2UgYXMgd2VsbC4KCj4gPiArc3RhdGljIGlu dCB0ZGFfZ2V0X2VuY29kZXIoc3RydWN0IHRkYV9wcml2ICpwcml2KQo+ID4gK3sKPiA+ICsJc3Ry dWN0IHNuZF9zb2NfY29kZWMgKmNvZGVjID0gcHJpdi0+Y29kZWM7Cj4gPiArCXN0cnVjdCBkZXZp Y2Vfbm9kZSAqbnA7Cj4gPiArCj4gPiArCS8qIGdldCB0aGUgcGFyZW50IHRkYTk5OHggZGV2aWNl ICovCj4gPiArCW5wID0gb2ZfZ2V0X3BhcmVudChjb2RlYy0+ZGV2LT5vZl9ub2RlKTsKPiA+ICsJ aWYgKCFucCB8fCAhb2ZfZGV2aWNlX2lzX2NvbXBhdGlibGUobnAsICJueHAsdGRhOTk4eCIpKSB7 Cj4gPiArCQlkZXZfZXJyKGNvZGVjLT5kZXYsICJubyBvciBiYWQgcGFyZW50IVxuIik7Cj4gPiAr CQlyZXR1cm4gLUVJTlZBTDsKPiA+ICsJfQo+ID4gKwlwcml2LT5pMmNfY2xpZW50ID0gb2ZfZmlu ZF9pMmNfZGV2aWNlX2J5X25vZGUobnApOwo+ID4gKwlvZl9ub2RlX3B1dChucCk7Cj4gPiArCXJl dHVybiAwOwo+ID4gK30KPiAKPiBXaHkgZG9lcyB0aGlzIG5lZWQgdG8gYmUgY2hlY2tlZCBsaWtl IHRoaXM/ICBXZSBkb24ndCBub3JtYWxseSBoYXZlIHRoaXMKPiBzb3J0IG9mIGNvZGUgdG8gY2hl Y2sgdGhhdCB0aGUgcGFyZW50IGlzIGNvcnJlY3QuCgpJbiBteSBwcmV2aW91cyBzdWJtaXQsIHRo ZSB0ZGEgQ09ERUMgd2FzIG5vdCBkZWNsYXJlZCBpbnNpZGUgdGhlCnRkYTk5OHggSTJjIGRldmlj ZSwgc28sIGl0cyBsb2NhdGlvbiB3YXMgc2VhcmNoZWQgZnJvbSAgcGhhbmRsZS4KCk5vdywgdGhl IENPREVDIGlzIGRlY2xhcmVkIGluc2lkZSB0aGUgdGRhOTk4eCBhcyBhIG5vZGUgY2hpbGQuIEJ1 dCwgaW4KYSBiYWQgRFQsIHRoZSB0ZGEgQ09ERUMgY291bGQgYmUgZGVjbGFyZWQgYW55d2hlcmUs IGV2ZW4gaW5zaWRlIGEgb3RoZXIKRFJNIEkyQyBzbGF2ZSBlbmNvZGVyLCBpbiB3aGljaCBjYXNl LCBiYWQgdGhpbmdzIHdvdWxkIGhhcHBlbi4uLgoKPiA+ICtzdGF0aWMgaW50IHRkYV9zdGFydF9z dG9wKHN0cnVjdCB0ZGFfcHJpdiAqcHJpdikKPiA+ICt7Cj4gPiArCWludCBwb3J0Owo+ID4gKwo+ ID4gKwkvKiBnaXZlIHRoZSBhdWRpbyBwYXJhbWV0ZXJzIHRvIHRoZSBIRE1JIGVuY29kZXIgKi8K PiA+ICsJaWYgKHByaXYtPmRhaV9pZCA9PSBBRk1UX0kyUykKPiA+ICsJCXBvcnQgPSBwcml2LT5w b3J0c1swXTsKPiA+ICsJZWxzZQo+ID4gKwkJcG9ydCA9IHByaXYtPnBvcnRzWzFdOwo+ID4gKwl0 ZGE5OTh4X2F1ZGlvX3VwZGF0ZShwcml2LT5pMmNfY2xpZW50LCBwcml2LT5kYWlfaWQsIHBvcnQp Owo+ID4gKwlyZXR1cm4gMDsKPiA+ICt9Cj4gCj4gV2hhdCBkb2VzIHRoaXMgYWN0dWFsbHkgZG8/ ICBObyBpbmZvcm1hdGlvbiBpcyBiZWluZyBwYXNzZWQgaW4gdG8gdGhlCj4gY29yZSBmdW5jdGlv biBoZXJlLCBub3QgZXZlbiBhbnkgaW5mb3JtYXRpb24gb24gaWYgaXQncyBzdGFydGluZyBvcgo+ IHN0b3BwaW5nLiAgTG9va2luZyBhdCB0aGUgcmVzdCBvZiB0aGUgY29kZSBJIGNhbid0IGhlbHAg dGhpbmtpbmcgaXQKPiBtaWdodCBiZSBjbGVhcmVyIHRvIGlubGluZSB0aGlzIHBvc3NpYmx5IHdp dGggYSBsb29rdXAgaGVscGVyLCB0aGUgY29kZQo+IGlzIHZlcnkgc21hbGwgYW5kIHRoZSBsYWNr IG9mIHBhcmFtZXRlcnMgbWFrZXMgaXQgaGFyZCB0byBmb2xsb3cuCgpJIHRob3VnaHQgaXQgd2Fz IHNpbXBsZSBlbm91Z2guIFRoZSBmdW5jdGlvbiB0ZGFfc3RhcnRfc3RvcCgpIGlzIGNhbGxlZApm cm9tIDIgcGxhY2VzOgoKLSBvbiBhdWRpbyBzdGFydCBpbiB0ZGFfc3RhcnR1cCB3aXRoIHRoZSBh dWRpbyB0eXBlIChEQUkgaWQpCglwcml2LT5kYWlfaWQgPSBkYWktPmlkOwoKLSBvbiBhdWRpbyBz dG9wIHdpdGggYSBudWxsIGF1ZGlvIHR5cGUKCXByaXYtPmRhaV9pZCA9IDA7CQkvKiBzdHJlYW1p bmcgc3RvcCAqLwoKT24gc3RyZWFtIHN0YXJ0LCB0aGUgREFJIGlkIGlzIG5ldmVyIG51bGwsIGFz IGV4cGxhaW5lZCBpbiB0aGUgcGF0Y2ggMToKCglUaGUgYXVkaW8gZm9ybWF0IHZhbHVlcyBpbiB0 aGUgZW5jb2RlciBjb25maWd1cmF0aW9uIGludGVyZmFjZSAgYXJlCgljaGFuZ2VkIHRvIG5vbiBu dWxsIHZhbHVlcyBzbyB0aGF0IHRoZSB2YWx1ZSAwIGlzIHVzZWQgaW4gdGhlIGF1ZGlvCglmdW5j dGlvbiB0byBpbmRpY2F0ZSB0aGF0IGF1ZGlvIHN0cmVhbWluZyBpcyBzdG9wcGVkLgoKYW5kIG9u IHN0cmVhbWluZyBzdG9wIHRoZSBwb3J0IGlzIG5vdCBtZWFuaW5nZnVsLgoKSSB3aWxsIGFkZCBh IG51bGwgaXRlbSBpbiB0aGUgZW51bSAoQUZNVF9OT19BVURJTykuCgo+ID4gK3N0YXRpYyBjb25z dCBzdHJ1Y3Qgc25kX3NvY19kYXBtX3JvdXRlIHRkYV9yb3V0ZXNbXSA9IHsKPiA+ICsJeyAiaGRt aS1vdXQiLCBOVUxMLCAiSERNSSBJMlMgUGxheWJhY2siIH0sCj4gPiArCXsgImhkbWktb3V0Iiwg TlVMTCwgIkhETUkgU1BESUYgUGxheWJhY2siIH0sCj4gPiArfTsKPiAKPiBTL1BESUYuCgpEaWQg eW91IGV2ZXIgdHJ5IHRoYXQgd2l0aCBkZWJ1Z2ZzPwoKQlRXLCB0aGlzIHBhdGNoIHNlcmllcyBt YXkgYmUgZGVsYXllZCBmb3Igc29tZSB0aW1lOiB0aGUgdGRhOTk4eCBkcml2ZXIKaGFzIHRvIGJl IHJld29ya2VkIGZvciBEVCBzdXBwb3J0LgoKLS0gCktlbiBhciBjJ2hlbnRhw7EJfAkgICAgICAq KiBCcmVpemggaGEgTGludXggYXRhdiEgKioKSmVmCQl8CQlodHRwOi8vbW9pbmVqZi5mcmVlLmZy LwpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpBbHNhLWRl dmVsIG1haWxpbmcgbGlzdApBbHNhLWRldmVsQGFsc2EtcHJvamVjdC5vcmcKaHR0cDovL21haWxt YW4uYWxzYS1wcm9qZWN0Lm9yZy9tYWlsbWFuL2xpc3RpbmZvL2Fsc2EtZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: moinejf@free.fr (Jean-Francois Moine) Date: Tue, 4 Feb 2014 18:16:05 +0100 Subject: [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x In-Reply-To: <20140204133014.GA22609@sirena.org.uk> References: <20140204133014.GA22609@sirena.org.uk> Message-ID: <20140204181605.5b837a70@armhf> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, 4 Feb 2014 13:30:14 +0000 Mark Brown wrote: > On Sun, Jan 26, 2014 at 07:45:36PM +0100, Jean-Francois Moine wrote: > > > + /* load the optional CODEC */ > > + of_platform_populate(np, NULL, NULL, &client->dev); > > + > > Why is this using of_platform_populate()? That's a very odd way of > doing things. The i2c does not populate the subnodes in the DT. I did not find why, but, what is sure is that if of_platform_populate() is not called, the tda CODEC module is not loaded. You may find an other example in drivers/mfd/twl-core.c. > > +config SND_SOC_TDA998X > > + tristate > > + depends on OF > > + default y if DRM_I2C_NXP_TDA998X=y > > + default m if DRM_I2C_NXP_TDA998X=m > > + > > Make this visible if it can be selected from DT so it can be used with > generic cards. I don't understand. The tda CODEC can only be used with the TDA998x I2C driver. It might have been included in the tda998x source as well. > > +static int tda_get_encoder(struct tda_priv *priv) > > +{ > > + struct snd_soc_codec *codec = priv->codec; > > + struct device_node *np; > > + > > + /* get the parent tda998x device */ > > + np = of_get_parent(codec->dev->of_node); > > + if (!np || !of_device_is_compatible(np, "nxp,tda998x")) { > > + dev_err(codec->dev, "no or bad parent!\n"); > > + return -EINVAL; > > + } > > + priv->i2c_client = of_find_i2c_device_by_node(np); > > + of_node_put(np); > > + return 0; > > +} > > Why does this need to be checked like this? We don't normally have this > sort of code to check that the parent is correct. In my previous submit, the tda CODEC was not declared inside the tda998x I2c device, so, its location was searched from phandle. Now, the CODEC is declared inside the tda998x as a node child. But, in a bad DT, the tda CODEC could be declared anywhere, even inside a other DRM I2C slave encoder, in which case, bad things would happen... > > +static int tda_start_stop(struct tda_priv *priv) > > +{ > > + int port; > > + > > + /* give the audio parameters to the HDMI encoder */ > > + if (priv->dai_id == AFMT_I2S) > > + port = priv->ports[0]; > > + else > > + port = priv->ports[1]; > > + tda998x_audio_update(priv->i2c_client, priv->dai_id, port); > > + return 0; > > +} > > What does this actually do? No information is being passed in to the > core function here, not even any information on if it's starting or > stopping. Looking at the rest of the code I can't help thinking it > might be clearer to inline this possibly with a lookup helper, the code > is very small and the lack of parameters makes it hard to follow. I thought it was simple enough. The function tda_start_stop() is called from 2 places: - on audio start in tda_startup with the audio type (DAI id) priv->dai_id = dai->id; - on audio stop with a null audio type priv->dai_id = 0; /* streaming stop */ On stream start, the DAI id is never null, as explained in the patch 1: The audio format values in the encoder configuration interface are changed to non null values so that the value 0 is used in the audio function to indicate that audio streaming is stopped. and on streaming stop the port is not meaningful. I will add a null item in the enum (AFMT_NO_AUDIO). > > +static const struct snd_soc_dapm_route tda_routes[] = { > > + { "hdmi-out", NULL, "HDMI I2S Playback" }, > > + { "hdmi-out", NULL, "HDMI SPDIF Playback" }, > > +}; > > S/PDIF. Did you ever try that with debugfs? BTW, this patch series may be delayed for some time: the tda998x driver has to be reworked for DT support. -- Ken ar c'henta? | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932364AbaBDRQA (ORCPT ); Tue, 4 Feb 2014 12:16:00 -0500 Received: from smtp2-g21.free.fr ([212.27.42.2]:54920 "EHLO smtp2-g21.free.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932132AbaBDRP6 convert rfc822-to-8bit (ORCPT ); Tue, 4 Feb 2014 12:15:58 -0500 Date: Tue, 4 Feb 2014 18:16:05 +0100 From: Jean-Francois Moine To: Mark Brown Cc: alsa-devel@alsa-project.org, Dave Airlie , dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Rob Clark , Russell King - ARM Linux Subject: Re: [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x Message-ID: <20140204181605.5b837a70@armhf> In-Reply-To: <20140204133014.GA22609@sirena.org.uk> References: <20140204133014.GA22609@sirena.org.uk> X-Mailer: Claws Mail 3.9.3 (GTK+ 2.24.22; arm-unknown-linux-gnueabihf) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 4 Feb 2014 13:30:14 +0000 Mark Brown wrote: > On Sun, Jan 26, 2014 at 07:45:36PM +0100, Jean-Francois Moine wrote: > > > + /* load the optional CODEC */ > > + of_platform_populate(np, NULL, NULL, &client->dev); > > + > > Why is this using of_platform_populate()? That's a very odd way of > doing things. The i2c does not populate the subnodes in the DT. I did not find why, but, what is sure is that if of_platform_populate() is not called, the tda CODEC module is not loaded. You may find an other example in drivers/mfd/twl-core.c. > > +config SND_SOC_TDA998X > > + tristate > > + depends on OF > > + default y if DRM_I2C_NXP_TDA998X=y > > + default m if DRM_I2C_NXP_TDA998X=m > > + > > Make this visible if it can be selected from DT so it can be used with > generic cards. I don't understand. The tda CODEC can only be used with the TDA998x I2C driver. It might have been included in the tda998x source as well. > > +static int tda_get_encoder(struct tda_priv *priv) > > +{ > > + struct snd_soc_codec *codec = priv->codec; > > + struct device_node *np; > > + > > + /* get the parent tda998x device */ > > + np = of_get_parent(codec->dev->of_node); > > + if (!np || !of_device_is_compatible(np, "nxp,tda998x")) { > > + dev_err(codec->dev, "no or bad parent!\n"); > > + return -EINVAL; > > + } > > + priv->i2c_client = of_find_i2c_device_by_node(np); > > + of_node_put(np); > > + return 0; > > +} > > Why does this need to be checked like this? We don't normally have this > sort of code to check that the parent is correct. In my previous submit, the tda CODEC was not declared inside the tda998x I2c device, so, its location was searched from phandle. Now, the CODEC is declared inside the tda998x as a node child. But, in a bad DT, the tda CODEC could be declared anywhere, even inside a other DRM I2C slave encoder, in which case, bad things would happen... > > +static int tda_start_stop(struct tda_priv *priv) > > +{ > > + int port; > > + > > + /* give the audio parameters to the HDMI encoder */ > > + if (priv->dai_id == AFMT_I2S) > > + port = priv->ports[0]; > > + else > > + port = priv->ports[1]; > > + tda998x_audio_update(priv->i2c_client, priv->dai_id, port); > > + return 0; > > +} > > What does this actually do? No information is being passed in to the > core function here, not even any information on if it's starting or > stopping. Looking at the rest of the code I can't help thinking it > might be clearer to inline this possibly with a lookup helper, the code > is very small and the lack of parameters makes it hard to follow. I thought it was simple enough. The function tda_start_stop() is called from 2 places: - on audio start in tda_startup with the audio type (DAI id) priv->dai_id = dai->id; - on audio stop with a null audio type priv->dai_id = 0; /* streaming stop */ On stream start, the DAI id is never null, as explained in the patch 1: The audio format values in the encoder configuration interface are changed to non null values so that the value 0 is used in the audio function to indicate that audio streaming is stopped. and on streaming stop the port is not meaningful. I will add a null item in the enum (AFMT_NO_AUDIO). > > +static const struct snd_soc_dapm_route tda_routes[] = { > > + { "hdmi-out", NULL, "HDMI I2S Playback" }, > > + { "hdmi-out", NULL, "HDMI SPDIF Playback" }, > > +}; > > S/PDIF. Did you ever try that with debugfs? BTW, this patch series may be delayed for some time: the tda998x driver has to be reworked for DT support. -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/