From mboxrd@z Thu Jan 1 00:00:00 1970 From: jassi brar Subject: Re: [PATCH 2/3] ASoC: AC97: S3C: Add controller driver Date: Tue, 26 Jan 2010 20:17:25 +0900 Message-ID: <1b68c6791001260317k44893521vc0310ea35bac8ee1@mail.gmail.com> References: <1264485101-13782-1-git-send-email-jassisinghbrar@gmail.com> <1264485101-13782-2-git-send-email-jassisinghbrar@gmail.com> <20100126104723.GF15759@rakim.wolfsonmicro.main> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20100126104723.GF15759@rakim.wolfsonmicro.main> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: alsa-devel-bounces@alsa-project.org Errors-To: alsa-devel-bounces@alsa-project.org To: Mark Brown Cc: alsa-devel@alsa-project.org, linux-samsung-soc@vger.kernel.org, Jassi Brar , ben-linux@fluff.org, linux-arm-kernel@lists.infradead.org, lrg@slimlogic.co.uk List-Id: linux-samsung-soc@vger.kernel.org T24gVHVlLCBKYW4gMjYsIDIwMTAgYXQgNzo0NyBQTSwgTWFyayBCcm93bgo8YnJvb25pZUBvcGVu c291cmNlLndvbGZzb25taWNyby5jb20+IHdyb3RlOgo+IE9uIFR1ZSwgSmFuIDI2LCAyMDEwIGF0 IDAyOjUxOjQwUE0gKzA5MDAsIGphc3Npc2luZ2hicmFyQGdtYWlsLmNvbSB3cm90ZToKPgo+IFRo aXMgbG9va3MgZ29vZCBvdmVyYWxsLCBqdXN0IGEgZmV3IHNtYWxsaXNoIGlzc3VlczoKPgo+PiAr c3RhdGljIHZvaWQgczNjX2FjOTdfYWN0aXZhdGUoc3RydWN0IHNuZF9hYzk3ICphYzk3KQo+PiAr ewo+PiArIMKgIMKgIHUzMiBhY19nbGJjdHJsLCBzdGF0Owo+PiArCj4+ICsgwqAgwqAgc3RhdCA9 IHJlYWRsKHMzY19hYzk3LnJlZ3MgKyBTM0NfQUM5N19HTEJTVEFUKSAmIDB4NzsKPj4gKyDCoCDC oCBzd2l0Y2ggKHN0YXQpIHsKPj4gKyDCoCDCoCBjYXNlIFMzQ19BQzk3X0dMQlNUQVRfTUFJTlNU QVRFX0FDVElWRToKPj4gKyDCoCDCoCDCoCDCoCDCoCDCoCByZXR1cm47Cj4+ICsgwqAgwqAgY2Fz ZSBTM0NfQUM5N19HTEJTVEFUX01BSU5TVEFURV9SRUFEWToKPj4gKyDCoCDCoCBjYXNlIFMzQ19B Qzk3X0dMQlNUQVRfTUFJTlNUQVRFX0lOSVQ6Cj4+ICsgwqAgwqAgwqAgwqAgwqAgwqAgYnJlYWs7 Cj4+ICsgwqAgwqAgZGVmYXVsdDoKPj4gKyDCoCDCoCDCoCDCoCDCoCDCoCBzM2NfYWM5N19jb2xk X3Jlc2V0KGFjOTcpOwo+PiArIMKgIMKgIMKgIMKgIMKgIMKgIHMzY19hYzk3X3dhcm1fcmVzZXQo YWM5Nyk7Cj4+ICsgwqAgwqAgwqAgwqAgwqAgwqAgYnJlYWs7Cj4+ICsgwqAgwqAgfQo+Cj4gVGhp cyBhdXRvbWF0aWMgY29sZCBhbmQgd2FybSByZXNldCBsb29rcyBhIGJpdCBmaXNoeSAtIG9idmlv dXNseSBpZiB0aGlzCj4gY29kZSBwYXRoIGV2ZXIgZ2V0cyBoaXQgaW4gbm9ybWFsIG9wZXJhdGlv biB0aGVuIGl0J3MgZ29pbmcgdG8gc2VyaW91c2x5Cj4gZGlzcnVwdCB0aGluZ3Mgc2luY2UgaXQn bGwgcmVzZXQgdGhlIENPREVDIHJlZ2lzdGVycy4gwqBBIHdhcm0gcmVzZXQgYnkKPiBpdHNlbGYg d291bGRuJ3QgYmUgYSBwcm9ibGVtIGJ1dCBJJ2QgcmF0aGVyIHNlZSBleHBsaWNpdCBjb2xkIHJl c2V0cyBpbgo+IHRoZSBjYWxsZXJzIHdoZXJlIHRoYXQncyByZXF1aXJlZC4KQmVmb3JlIHJlYWQv d3JpdGUgd2UgbmVlZCB0byBlbnN1cmUgdGhlIGxpbmsgaXMgYWN0aXZlLiBBbmQgdG8gcmVhY2gg dGhlCmFjdGl2ZSBzdGF0ZSB3ZSBoYXZlIHRvIGRvIHRoYXQgYXMgc3VnZ2VzdGVkIGJ5IHRoZSBG U00gc2hvd24gaW4gU29DcycgTWFudWFsLgpBbHNvLCB0aGlzIGhlbHBzIG5vdCByZWx5aW5nIG9u IGNvZGVjL2NvcmUgdG8gcGVyZm9ybSBwYXJ0aWN1bGFyIHN0ZXBzCm9mIGluaXRpYWxpemF0aW9u cy4KCj4+ICsgwqAgwqAgYWNfZ2xiY3RybCA9IHJlYWRsKHMzY19hYzk3LnJlZ3MgKyBTM0NfQUM5 N19HTEJDVFJMKTsKPj4gKyDCoCDCoCBhY19nbGJjdHJsID0gUzNDX0FDOTdfR0xCQ1RSTF9BQ0xJ TktPTjsKPj4gKyDCoCDCoCB3cml0ZWwoYWNfZ2xiY3RybCwgczNjX2FjOTcucmVncyArIFMzQ19B Qzk3X0dMQkNUUkwpOwo+PiArIMKgIMKgIG1zbGVlcCgxKTsKPgo+IFRoaXMgYWxzbyBsb29rcyBh IGJpdCBvZGQsIEFDTElOS09OIHNvdW5kcyBsaWtlIGJyaW5naW5nIHVwIHRoZSBsaW5rCj4gd2hp Y2ggaXMgd2hhdCBhIHdhcm0gcmVzZXQgZG9lcy4KQXMgc2hvd24gaW4gRlNNIG9mIFNvQ3MgbWFu dWFsLCB0aGlzIHNldHMgdGhlIGNvbnRyb2xsZXIgc3RhdGUgdG8gUkVBRFkuClBsZWFzZSBoYXZl IGEgbG9vayBhdCBhbnkgbWFudWFsJ3MgQUM5NyBjaGFwdGVyLgoKPj4gKyDCoCDCoCBJTklUX0NP TVBMRVRJT04oczNjX2FjOTcuZG9uZSk7Cj4+ICsKPj4gKyDCoCDCoCBpZiAoIXdhaXRfZm9yX2Nv bXBsZXRpb25fdGltZW91dCgmczNjX2FjOTcuZG9uZSwgSFopKQo+PiArIMKgIMKgIMKgIMKgIMKg IMKgIHByaW50ayhLRVJOX0VSUiAiQUM5NzogVW5hYmxlIHRvIGFjdGl2YXRlISIpOwo+Cj4gVGhp cyBsb29rcyByYWN5IC0gdGhlIElOSVRfQ09NUExFVElPTigpIGhhcHBlbnMgYWZ0ZXIgYWxsIHRo ZSBoYXJkd2FyZQo+IGNvbmZpZ3VyYXRpb24gd2hpY2ggc3VnZ2VzdHMgdGhhdCBpZiB5b3UncmUg dW5sdWNreSB0aGUgZXZlbnQgd2hpY2gKPiBzaG91bGQgdHJpZ2dlciB0aGUgY29tcGxldGlvbiB3 aWxsIGhhdmUgaGFwcGVuZWQgYmVmb3JlIHRoZSBpbml0LiDCoEEKPiBidW5jaCBvZiBpbnRlcnJ1 cHRzIGFycml2aW5nIGF0IGFuIGluY29udmVuaWVudCB0aW1lIGNvdWxkIHRyaWdnZXIgdGhpcywK PiBmb3IgZXhhbXBsZS4KWWVzLCBpbml0IG5lZWRzIHRvIGJlIG1vdmVkIGVhcmx5LgoKPiBUaGUg c2FtZSBpZGlvbSBhcHBlYXJzIGluIHRoZSByZWdpc3RlciByZWFkcyBhbmQgd3JpdGVzLgo+Cj4+ ICsgwqAgwqAgaWYgKGFkZHIgIT0gcmVnKQo+PiArIMKgIMKgIMKgIMKgIMKgIMKgIHByaW50ayhL RVJOX0VSUiAiczNjLWFjOTc6IHJlcSBhZGRyID0gJTAyeCwiCj4+ICsgwqAgwqAgwqAgwqAgwqAg wqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgIiByZXAgYWRkciA9ICUwMnhcbiIsIHJlZywgYWRk cik7Cj4KPiBQbGVhc2UgZG9uJ3Qgc3BsaXQgZXJyb3IgbWVzc2FnZXMgb3ZlciBtdWx0aXBsZSBs aW5lcywgaXQgbWFrZXMgZ3JlcHBpbmcKPiBmb3IgdGhlbSBoYXJkZXIuCm9rLgoKPj4gK3N0YXRp YyBpcnFyZXR1cm5fdCBzM2NfYWM5N19pcnEoaW50IGlycSwgdm9pZCAqZGV2X2lkKQo+PiArewo+ PiArIMKgIMKgIHUzMiBhY19nbGJjdHJsLCBhY19nbGJzdGF0Owo+PiArCj4+ICsgwqAgwqAgYWNf Z2xic3RhdCA9IHJlYWRsKHMzY19hYzk3LnJlZ3MgKyBTM0NfQUM5N19HTEJTVEFUKTsKPj4gKwo+ PiArIMKgIMKgIGlmIChhY19nbGJzdGF0ICYgUzNDX0FDOTdfR0xCU1RBVF9DT0RFQ1JFQURZKSB7 Cj4+ICsKPj4gKyDCoCDCoCDCoCDCoCDCoCDCoCBhY19nbGJjdHJsID0gcmVhZGwoczNjX2FjOTcu cmVncyArIFMzQ19BQzk3X0dMQkNUUkwpOwo+PiArIMKgIMKgIMKgIMKgIMKgIMKgIGFjX2dsYmN0 cmwgJj0gflMzQ19BQzk3X0dMQkNUUkxfQ09ERUNSRUFEWUlFOwo+PiArIMKgIMKgIMKgIMKgIMKg IMKgIHdyaXRlbChhY19nbGJjdHJsLCBzM2NfYWM5Ny5yZWdzICsgUzNDX0FDOTdfR0xCQ1RSTCk7 Cj4+ICsKPj4gKyDCoCDCoCDCoCDCoCDCoCDCoCBhY19nbGJjdHJsID0gcmVhZGwoczNjX2FjOTcu cmVncyArIFMzQ19BQzk3X0dMQkNUUkwpOwo+PiArIMKgIMKgIMKgIMKgIMKgIMKgIGFjX2dsYmN0 cmwgfD0gKDE8PDMwKTsgLyogQ2xlYXIgaW50ZXJydXB0ICovCj4+ICsgwqAgwqAgwqAgwqAgwqAg wqAgd3JpdGVsKGFjX2dsYmN0cmwsIHMzY19hYzk3LnJlZ3MgKyBTM0NfQUM5N19HTEJDVFJMKTsK Pj4gKwo+PiArIMKgIMKgIMKgIMKgIMKgIMKgIGNvbXBsZXRlKCZzM2NfYWM5Ny5kb25lKTsKPj4g KyDCoCDCoCB9Cj4+ICsKPj4gKyDCoCDCoCByZXR1cm4gSVJRX0hBTkRMRUQ7Cj4+ICt9Cj4KPiBZ b3Ugc2hvdWxkIG9ubHkgYmUgcmV0dXJuaW5nIElSUV9IQU5ETEVEIGlmIHlvdSBhY3R1YWxseSBo YW5kbGVkIGFuCj4gaW50ZXJydXB0IGhlcmUuCkknZCByYXRoZXIgbW92ZSB0aGUgaW50ci1jbGVh cmluZyBvdXQgb2YgdGhlIGJsb2NrLgoKPj4gKyNkZWZpbmUgUzNDX0FDOTdfUkFURVMgKFNORFJW X1BDTV9SQVRFXzgwMDAgfCBTTkRSVl9QQ01fUkFURV8xMTAyNSB8XAo+PiArIMKgIMKgIMKgIMKg IMKgIMKgIFNORFJWX1BDTV9SQVRFXzE2MDAwIHwgU05EUlZfUENNX1JBVEVfMjIwNTAgfCBcCj4+ ICsgwqAgwqAgwqAgwqAgwqAgwqAgU05EUlZfUENNX1JBVEVfMzIwMDAgfCBTTkRSVl9QQ01fUkFU RV80NDEwMCB8IFwKPj4gKyDCoCDCoCDCoCDCoCDCoCDCoCBTTkRSVl9QQ01fUkFURV80ODAwMCkK Pgo+IFNORFJWX1BDTV9SQVRFXzgwMDBfNDgwMDAuCnllcy4KCj4+ICsgwqAgwqAgwqAgwqAgwqAg wqAgLmNhcHR1cmUgPSB7Cj4+ICsgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgLnN0cmVh bV9uYW1lID0gIkFDOTcgQ2FwdHVyZSIsCj4+ICsgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAg wqAgLyogTk9URTogSWYgdGhlIGNvZGVjIG91cHV0cyBqdXN0IG9uZSBzbG90LAo+PiArIMKgIMKg IMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgKiBpdCAqc2VlbXMqIG91ciBBQzk3IGNvbnRyb2xs ZXIgcmVhZHMgdGhlIG9ubHkKPj4gKyDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCog dmFsaWQgc2xvdChpZiBlaXRoZXIgMyBvciA0KSBmb3IgUENNLUluLgo+PiArIMKgIMKgIMKgIMKg IMKgIMKgIMKgIMKgIMKgIMKgIMKgKiBGb3Igc3VjaCBjYXNlcywgd2UgcmVjb3JkIE1vbm8uCj4+ ICsgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAqLwo+Cj4gVGhpcyBzZWVtcyBsaWtl IHVuc3VycHJpc2luZyBiZWhhdmlvdXIgZm9yIGFuIEFDOTcgY29udHJvbGxlciAtIHRoZSBzbG90 Cj4gdmFsaWRpdHkgaW5mb3JtYXRpb24gaXMgdGhlcmUgZm9yIGp1c3QgdGhpcyBwdXJwb3NlLiDC oEknZCBqdXN0IHJlbW92ZQo+IHRoZSBjb21tZW50IGFzIGEgcmVzdWx0Lgo+Cj4+ICsgwqAgwqAg wqAgwqAgwqAgwqAgLmNhcHR1cmUgPSB7Cj4+ICsgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAg wqAgLnN0cmVhbV9uYW1lID0gIkFDOTcgTWljIENhcHR1cmUiLAo+PiArIMKgIMKgIMKgIMKgIMKg IMKgIMKgIMKgIMKgIMKgIC5jaGFubmVsc19taW4gPSAxLAo+PiArIMKgIMKgIMKgIMKgIMKgIMKg IMKgIMKgIMKgIMKgIC8qIE5PVEU6IElmIHRoZSBjb2RlYyhsaWtlIFdNOTcxMykgY2FuJ3Qgb3Vw dXQganVzdAo+PiArIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgIMKgKiBvbmUgc2xvdCwg aXQgKnNlZW1zKiBvdXIgQUM5NyBjb250cm9sbGVyIHJlYWRzCj4+ICsgwqAgwqAgwqAgwqAgwqAg wqAgwqAgwqAgwqAgwqAgwqAqIHR3byBzbG90cyhpZiBvbmUgb2YgdGhlbSBpcyBTbG90LTYpIGZv ciBNSUMgYWxzby4KPj4gKyDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCDCoCogRm9yIHN1 Y2ggY2FzZXMsIHdlIHJlY29yZCBTdGVyZW8uCj4+ICsgwqAgwqAgwqAgwqAgwqAgwqAgwqAgwqAg wqAgwqAgwqAqLwo+Cj4gU2ltaWxhcmx5IGhlcmUuCj4KPj4gKyDCoCDCoCBpZiAoYWM5N19wZGF0 YS0+Y2ZnX2dwaW8ocGRldikpIHsKPj4gKyDCoCDCoCDCoCDCoCDCoCDCoCBkZXZfZXJyKCZwZGV2 LT5kZXYsICJVbmFibGUgdG8gY29uZmlndXJlIGdwaW9cbiIpOwo+PiArIMKgIMKgIMKgIMKgIMKg IMKgIHJldCA9IC1FSU5WQUw7Cj4+ICsgwqAgwqAgwqAgwqAgwqAgwqAgZ290byBsYjM7Cj4+ICsg wqAgwqAgfQo+Cj4gU2hvdWxkIHJlYWxseSBjaGVjayBmb3IgdGhlIGZ1bmN0aW9uIGJlaW5nIG5v bi1OVUxMIGhlcmUuCmFscmVhZHkgY2hlY2tlZCBhdCB0aGUgc3RhcnQgYW5kIHByb2JlIGZhaWxz IGlmIGl0cyBOVUxMCgo+PiArbGI1Ogo+PiArIMKgIMKgIGZyZWVfaXJxKGlycV9yZXMtPnN0YXJ0 LCBOVUxMKTsKPgo+IFBlcmhhcHMgYSBiZXR0ZXIgbmFtZSB0aGFuICdsYicgLSBlcnIsIG9yIGZh aWwgb3Igc29tZXRoaW5nLiDCoGxiIG1lYW5zCj4gbm90aGluZyB0byBtZSBhdCBsZWFzdC4KbWVh bnMgbGFiZWwgdG8gbWUgOikKCj4+ICsgwqAgwqAgaXJxX3JlcyA9IHBsYXRmb3JtX2dldF9yZXNv dXJjZShwZGV2LCBJT1JFU09VUkNFX0lSUSwgMCk7Cj4+ICsgwqAgwqAgaWYgKGlycV9yZXMpCj4+ ICsgwqAgwqAgwqAgwqAgwqAgwqAgZnJlZV9pcnEoaXJxX3Jlcy0+c3RhcnQsIE5VTEwpOwo+Cj4g VGhpcyBzaG91bGQgbmV2ZXIgZ2V0IGNhbGxlZCBpZiB0aGUgcmVzb3VyY2VzIGFyZW4ndCBhbGxv Y2F0ZWQuCmJ1dCB0aGUgc3RhdGljIGNvZGUgYW5hbHlzZXIgZG9lc24ndCB1bmRlcnN0YW5kIHRo YXQuCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCkFsc2Et ZGV2ZWwgbWFpbGluZyBsaXN0CkFsc2EtZGV2ZWxAYWxzYS1wcm9qZWN0Lm9yZwpodHRwOi8vbWFp bG1hbi5hbHNhLXByb2plY3Qub3JnL21haWxtYW4vbGlzdGluZm8vYWxzYS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: jassisinghbrar@gmail.com (jassi brar) Date: Tue, 26 Jan 2010 20:17:25 +0900 Subject: [PATCH 2/3] ASoC: AC97: S3C: Add controller driver In-Reply-To: <20100126104723.GF15759@rakim.wolfsonmicro.main> References: <1264485101-13782-1-git-send-email-jassisinghbrar@gmail.com> <1264485101-13782-2-git-send-email-jassisinghbrar@gmail.com> <20100126104723.GF15759@rakim.wolfsonmicro.main> Message-ID: <1b68c6791001260317k44893521vc0310ea35bac8ee1@mail.gmail.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, Jan 26, 2010 at 7:47 PM, Mark Brown wrote: > On Tue, Jan 26, 2010 at 02:51:40PM +0900, jassisinghbrar at gmail.com wrote: > > This looks good overall, just a few smallish issues: > >> +static void s3c_ac97_activate(struct snd_ac97 *ac97) >> +{ >> + ? ? u32 ac_glbctrl, stat; >> + >> + ? ? stat = readl(s3c_ac97.regs + S3C_AC97_GLBSTAT) & 0x7; >> + ? ? switch (stat) { >> + ? ? case S3C_AC97_GLBSTAT_MAINSTATE_ACTIVE: >> + ? ? ? ? ? ? return; >> + ? ? case S3C_AC97_GLBSTAT_MAINSTATE_READY: >> + ? ? case S3C_AC97_GLBSTAT_MAINSTATE_INIT: >> + ? ? ? ? ? ? break; >> + ? ? default: >> + ? ? ? ? ? ? s3c_ac97_cold_reset(ac97); >> + ? ? ? ? ? ? s3c_ac97_warm_reset(ac97); >> + ? ? ? ? ? ? break; >> + ? ? } > > This automatic cold and warm reset looks a bit fishy - obviously if this > code path ever gets hit in normal operation then it's going to seriously > disrupt things since it'll reset the CODEC registers. ?A warm reset by > itself wouldn't be a problem but I'd rather see explicit cold resets in > the callers where that's required. Before read/write we need to ensure the link is active. And to reach the active state we have to do that as suggested by the FSM shown in SoCs' Manual. Also, this helps not relying on codec/core to perform particular steps of initializations. >> + ? ? ac_glbctrl = readl(s3c_ac97.regs + S3C_AC97_GLBCTRL); >> + ? ? ac_glbctrl = S3C_AC97_GLBCTRL_ACLINKON; >> + ? ? writel(ac_glbctrl, s3c_ac97.regs + S3C_AC97_GLBCTRL); >> + ? ? msleep(1); > > This also looks a bit odd, ACLINKON sounds like bringing up the link > which is what a warm reset does. As shown in FSM of SoCs manual, this sets the controller state to READY. Please have a look at any manual's AC97 chapter. >> + ? ? INIT_COMPLETION(s3c_ac97.done); >> + >> + ? ? if (!wait_for_completion_timeout(&s3c_ac97.done, HZ)) >> + ? ? ? ? ? ? printk(KERN_ERR "AC97: Unable to activate!"); > > This looks racy - the INIT_COMPLETION() happens after all the hardware > configuration which suggests that if you're unlucky the event which > should trigger the completion will have happened before the init. ?A > bunch of interrupts arriving at an inconvenient time could trigger this, > for example. Yes, init needs to be moved early. > The same idiom appears in the register reads and writes. > >> + ? ? if (addr != reg) >> + ? ? ? ? ? ? printk(KERN_ERR "s3c-ac97: req addr = %02x," >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? " rep addr = %02x\n", reg, addr); > > Please don't split error messages over multiple lines, it makes grepping > for them harder. ok. >> +static irqreturn_t s3c_ac97_irq(int irq, void *dev_id) >> +{ >> + ? ? u32 ac_glbctrl, ac_glbstat; >> + >> + ? ? ac_glbstat = readl(s3c_ac97.regs + S3C_AC97_GLBSTAT); >> + >> + ? ? if (ac_glbstat & S3C_AC97_GLBSTAT_CODECREADY) { >> + >> + ? ? ? ? ? ? ac_glbctrl = readl(s3c_ac97.regs + S3C_AC97_GLBCTRL); >> + ? ? ? ? ? ? ac_glbctrl &= ~S3C_AC97_GLBCTRL_CODECREADYIE; >> + ? ? ? ? ? ? writel(ac_glbctrl, s3c_ac97.regs + S3C_AC97_GLBCTRL); >> + >> + ? ? ? ? ? ? ac_glbctrl = readl(s3c_ac97.regs + S3C_AC97_GLBCTRL); >> + ? ? ? ? ? ? ac_glbctrl |= (1<<30); /* Clear interrupt */ >> + ? ? ? ? ? ? writel(ac_glbctrl, s3c_ac97.regs + S3C_AC97_GLBCTRL); >> + >> + ? ? ? ? ? ? complete(&s3c_ac97.done); >> + ? ? } >> + >> + ? ? return IRQ_HANDLED; >> +} > > You should only be returning IRQ_HANDLED if you actually handled an > interrupt here. I'd rather move the intr-clearing out of the block. >> +#define S3C_AC97_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 |\ >> + ? ? ? ? ? ? SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 | \ >> + ? ? ? ? ? ? SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 | \ >> + ? ? ? ? ? ? SNDRV_PCM_RATE_48000) > > SNDRV_PCM_RATE_8000_48000. yes. >> + ? ? ? ? ? ? .capture = { >> + ? ? ? ? ? ? ? ? ? ? .stream_name = "AC97 Capture", >> + ? ? ? ? ? ? ? ? ? ? /* NOTE: If the codec ouputs just one slot, >> + ? ? ? ? ? ? ? ? ? ? ?* it *seems* our AC97 controller reads the only >> + ? ? ? ? ? ? ? ? ? ? ?* valid slot(if either 3 or 4) for PCM-In. >> + ? ? ? ? ? ? ? ? ? ? ?* For such cases, we record Mono. >> + ? ? ? ? ? ? ? ? ? ? ?*/ > > This seems like unsurprising behaviour for an AC97 controller - the slot > validity information is there for just this purpose. ?I'd just remove > the comment as a result. > >> + ? ? ? ? ? ? .capture = { >> + ? ? ? ? ? ? ? ? ? ? .stream_name = "AC97 Mic Capture", >> + ? ? ? ? ? ? ? ? ? ? .channels_min = 1, >> + ? ? ? ? ? ? ? ? ? ? /* NOTE: If the codec(like WM9713) can't ouput just >> + ? ? ? ? ? ? ? ? ? ? ?* one slot, it *seems* our AC97 controller reads >> + ? ? ? ? ? ? ? ? ? ? ?* two slots(if one of them is Slot-6) for MIC also. >> + ? ? ? ? ? ? ? ? ? ? ?* For such cases, we record Stereo. >> + ? ? ? ? ? ? ? ? ? ? ?*/ > > Similarly here. > >> + ? ? if (ac97_pdata->cfg_gpio(pdev)) { >> + ? ? ? ? ? ? dev_err(&pdev->dev, "Unable to configure gpio\n"); >> + ? ? ? ? ? ? ret = -EINVAL; >> + ? ? ? ? ? ? goto lb3; >> + ? ? } > > Should really check for the function being non-NULL here. already checked at the start and probe fails if its NULL >> +lb5: >> + ? ? free_irq(irq_res->start, NULL); > > Perhaps a better name than 'lb' - err, or fail or something. ?lb means > nothing to me at least. means label to me :) >> + ? ? irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >> + ? ? if (irq_res) >> + ? ? ? ? ? ? free_irq(irq_res->start, NULL); > > This should never get called if the resources aren't allocated. but the static code analyser doesn't understand that.