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 21:11:55 +0900 Message-ID: <1b68c6791001260411u2b39d8actf35a0f3541fda92d@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> <1b68c6791001260317k44893521vc0310ea35bac8ee1@mail.gmail.com> <20100126115258.GM15759@rakim.wolfsonmicro.main> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20100126115258.GM15759@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 T24gVHVlLCBKYW4gMjYsIDIwMTAgYXQgODo1MiBQTSwgTWFyayBCcm93bgo8YnJvb25pZUBvcGVu c291cmNlLndvbGZzb25taWNyby5jb20+IHdyb3RlOgo+IE9uIFR1ZSwgSmFuIDI2LCAyMDEwIGF0 IDA4OjE3OjI1UE0gKzA5MDAsIGphc3NpIGJyYXIgd3JvdGU6Cj4+IE9uIFR1ZSwgSmFuIDI2LCAy MDEwIGF0IDc6NDcgUE0sIE1hcmsgQnJvd24KPgo+PiA+PiArIMKgIMKgIGRlZmF1bHQ6Cj4+ID4+ ICsgwqAgwqAgwqAgwqAgwqAgwqAgczNjX2FjOTdfY29sZF9yZXNldChhYzk3KTsKPj4gPj4gKyDC oCDCoCDCoCDCoCDCoCDCoCBzM2NfYWM5N193YXJtX3Jlc2V0KGFjOTcpOwo+PiA+PiArIMKgIMKg IMKgIMKgIMKgIMKgIGJyZWFrOwo+PiA+PiArIMKgIMKgIH0KPgo+PiA+IFRoaXMgYXV0b21hdGlj IGNvbGQgYW5kIHdhcm0gcmVzZXQgbG9va3MgYSBiaXQgZmlzaHkgLSBvYnZpb3VzbHkgaWYgdGhp cwo+PiA+IGNvZGUgcGF0aCBldmVyIGdldHMgaGl0IGluIG5vcm1hbCBvcGVyYXRpb24gdGhlbiBp dCdzIGdvaW5nIHRvIHNlcmlvdXNseQo+PiA+IGRpc3J1cHQgdGhpbmdzIHNpbmNlIGl0J2xsIHJl c2V0IHRoZSBDT0RFQyByZWdpc3RlcnMuIMKgQSB3YXJtIHJlc2V0IGJ5Cj4+ID4gaXRzZWxmIHdv dWxkbid0IGJlIGEgcHJvYmxlbSBidXQgSSdkIHJhdGhlciBzZWUgZXhwbGljaXQgY29sZCByZXNl dHMgaW4KPj4gPiB0aGUgY2FsbGVycyB3aGVyZSB0aGF0J3MgcmVxdWlyZWQuCj4KPj4gQmVmb3Jl IHJlYWQvd3JpdGUgd2UgbmVlZCB0byBlbnN1cmUgdGhlIGxpbmsgaXMgYWN0aXZlLiBBbmQgdG8g cmVhY2ggdGhlCj4+IGFjdGl2ZSBzdGF0ZSB3ZSBoYXZlIHRvIGRvIHRoYXQgYXMgc3VnZ2VzdGVk IGJ5IHRoZSBGU00gc2hvd24gaW4gU29DcycgTWFudWFsLgo+Cj4gVGhhdCdzIG5vdCBhZGRyZXNz aW5nIHRoZSBwcm9ibGVtLCB0aG91Z2ggLSB0aGUgYmlnIGlzc3VlIGlzIG5vdAo+IGJyaW5naW5n IHVwIHRoZSBBQzk3IGxpbmssIGl0J3MgdGhlIGZhY3QgdGhhdCB5b3UncmUgZG9pbmcgYW4KPiB1 bmNvbnRyb2xsZWQgY29sZCByZXNldC4gwqBJZiB3ZSBoaXQgdGhpcyBjb2RlIHBhdGggaXQnbGwg cmV2ZXJ0IHRoZQo+IGRldmljZSByZWdpc3RlcnMgdG8gZGVmYXVsdCB3aGljaCB3aWxsIHVwc2V0 IHRoZSBkcml2ZXJzIGZvciB0aGUgQ09ERUMKPiByYXRoZXIgYmFkbHkuIMKgVGhlcmUgc2hvdWxk IGJlIG5vIHBvc3NpYmlsaXR5IG9mIHRoYXQgaGFwcGVuaW5nLCBpZiBpdAo+IGlzIGhhcHBlbmlu ZyBpdCdzIHNvbWV0aGluZyB0aGF0IGNhbGxlcnMgcmVhbGx5IHNob3VsZCBrbm93IGFib3V0LgpJ dCBzaG91bGQgbmV2ZXIgYmUgcmVhY2hlZCBhZnRlciB0aGUgbGluayBpcyB1cCBhbmQgcnVubmlu Zy4gT25seSBpZiBzb21ldGhpbmcKZ29lcyB3cm9uZyBhdCBydW50aW1lLCB3b3VsZCB0aGUgY29u dHJvbGxlciBzdGF0ZSBiZSBjaGFuZ2VkLiBBbmQKdGhpcyBpcyBhbiBhdHRlbXB0IHRvIHJlY292 ZXIgZnJvbSB0aGF0IGZhaWx1cmUuIElmIHRoZSB1cHBlciBsYXllciBzaG91bGQKa25vdyBvZiBz dWNoIGZhaWx1cmUsIHRoZW4gbWF5YmUgd2Ugc2hvdWxkIG5vdCBkbyBpdC4KSSBoYXZlIG5ldmVy IHNlZW4gc3VjaCBydW50aW1lIGVycm9yIHRob3VnaC4KCj4+IEFsc28sIHRoaXMgaGVscHMgbm90 IHJlbHlpbmcgb24gY29kZWMvY29yZSB0byBwZXJmb3JtIHBhcnRpY3VsYXIgc3RlcHMKPj4gb2Yg aW5pdGlhbGl6YXRpb25zLgo+Cj4gRXF1YWxseSB3ZWxsIGlmIHRoZSBjb250cm9sbGVyIGRyaXZl ciBkaXZlcmdlcyBmcm9tIHdoYXQgb3RoZXIgZHJpdmVycwo+IGRvIGl0J3MgZ29pbmcgdG8gbGVh ZCB0byBpbnRlcm9wZXJhYmlsaXR5IHNrZXcgZm9yIGRyaXZlcnMuCj4KPiBGb3IgdGhlIHdhcm0g cmVzZXQgZnVuY3Rpb24gSSdkIHN1Z2dlc3QgYWRkaW5nIHNvbWV0aGluZyB3aGljaCBjaGVja3Mg dG8KPiBzZWUgaWYgdGhlIGxpbmsgaXMgYWxyZWFkeSBhY3RpdmUgYW5kIHN1cHByZXNzZXMgdGhl IHdhcm0gcmVzZXQgaW4gdGhhdAo+IGNhc2UuIMKgVGhhdCB3aWxsIGF2b2lkIHNsb3dpbmcgZXZl cnl0aGluZyBkb3duIHdpdGggc3B1cmlvdXMgd2FybSByZXNldHMKPiB3aGVuIHRoZSBsaW5rIGlz IGFscmVhZHkgcnVubm5pbmcgYW5kIG90aGVyIGRyaXZlcnMgYXNzdW1lIHRoZXkgbmVlZCB0bwo+ IGJyaW5nIGl0IHVwIC0gd2l0aCB0aGUgd2F5IHRoaW5ncyBhcmUgc3RydWN0dXJlZCBhdCBwcmVz ZW50IG1hbnkgd2FybQo+IHJlc2V0cyByZXF1ZXN0ZWQgYnkgb3RoZXIgZHJpdmVycyBhcmUgbGlr ZWx5IHRvIGJlIHNwdXJpb3VzLgpvaywgaSB3aWxsIGFkZCB0aGUgY2hlY2sgaW4gd2FybSByZXNl dC4KCj4gSWRlYWxseSB0aGUgY29yZSB3b3VsZCBrZWVwIHRyYWNrIG9mIHRoaXMgYW5kIHRyYW5z cGFyZW50bHkgdHJpZ2dlciB0aGUKPiB3YXJtIHJlc2V0IHdoZW4gcmVxdWlyZWQsIGJ1dCBkcml2 ZXIgbG9jYWwgaXMgZmluZSBzaW5jZSB0aGF0J3Mgbm90Cj4gdGhlcmUgeWV0Lgo+Cj4+ID4+ICsg wqAgwqAgYWNfZ2xiY3RybCA9IFMzQ19BQzk3X0dMQkNUUkxfQUNMSU5LT047Cj4+ID4+ICsgwqAg wqAgd3JpdGVsKGFjX2dsYmN0cmwsIHMzY19hYzk3LnJlZ3MgKyBTM0NfQUM5N19HTEJDVFJMKTsK Pj4gPj4gKyDCoCDCoCBtc2xlZXAoMSk7Cj4KPj4gPiBUaGlzIGFsc28gbG9va3MgYSBiaXQgb2Rk LCBBQ0xJTktPTiBzb3VuZHMgbGlrZSBicmluZ2luZyB1cCB0aGUgbGluawo+PiA+IHdoaWNoIGlz IHdoYXQgYSB3YXJtIHJlc2V0IGRvZXMuCj4KPj4gQXMgc2hvd24gaW4gRlNNIG9mIFNvQ3MgbWFu dWFsLCB0aGlzIHNldHMgdGhlIGNvbnRyb2xsZXIgc3RhdGUgdG8gUkVBRFkuCj4+IFBsZWFzZSBo YXZlIGEgbG9vayBhdCBhbnkgbWFudWFsJ3MgQUM5NyBjaGFwdGVyLgo+Cj4gUmlnaHQsIGJ1dCB3 aHkgaXMgdGhpcyBiZWluZyBkb25lIGJ5IHRoaXMgZnVuY3Rpb24gYW5kIG5vdCwgZm9yIGV4YW1w bGUsCj4gYnkgdGhlIHdhcm0gcmVzZXQ/IMKgSXQncyB0aGUgc3RydWN0dXJlIG9mIHRoZSBjb2Rl IEknbSBjb21tZW50aW5nIG9uCj4gcmF0aGVyIHRoYW4gdGhlIG9wZXJhdGlvbnMgdGhhdCBnZXQg cGVyZm9ybWVkIG9uIHRoZSBoYXJkd2FyZS4KSSBhc3N1bWUgdGhlIEFMU0EgZGVmaW5pdGlvbiBv ZiBjb2xkL3dhcm0gcmVzZXQgaXMgc2FtZSBhcyB0aGF0IG9mCnRoZSBBQzk3IGNvbnRyb2xsZXIu IEFueXRoaW5nIHJlbWFpbmluZyBzaG91bGQgYmUgZG9uZSBvbmx5IHdoZW4gbmVjZXNzYXJ5LgpG b3IgZXgsIHRoZSBBTFNBIG1pZ2h0IHdhbnQgdG8gbGVhdmUgY29udHJvbGxlciBhZnRlciBqdXN0 IGNvbGQvd2FybQpyZXNldCAuLi4gYXMKdGhlIFNvQyBtYW51YWwgc2F5cyBpdCdzIGluIGxvdy1w b3dlciBtb2RlIHdpdGhvdXQgdGhlIEFDLWxpbmsgb24uCgo+PiA+PiArbGI1Ogo+PiA+PiArIMKg IMKgIGZyZWVfaXJxKGlycV9yZXMtPnN0YXJ0LCBOVUxMKTsKPgo+PiA+IFBlcmhhcHMgYSBiZXR0 ZXIgbmFtZSB0aGFuICdsYicgLSBlcnIsIG9yIGZhaWwgb3Igc29tZXRoaW5nLiDCoGxiIG1lYW5z Cj4+ID4gbm90aGluZyB0byBtZSBhdCBsZWFzdC4KPgo+PiBtZWFucyBsYWJlbCB0byBtZSA6KQo+ Cj4gSWYgeW91J2Qgc3BlbHQgb3V0IGxhYmVsIEknZCBwcm9iYWJseSBoYXZlIGJlZW4gT0ssIGJ1 dCBpbiBFbmdsaXNoIGxiIGlzCj4gbm9ybWFsbHkgb25seSBhbiBhYmJyZXZpYXRpb24gZm9yIHBv dW5kcyBhcyBhIHVuaXQgb2Ygd2VpZ2h0Lgpvaywgd2lsbCByZW5hbWUgdGhlIGxhYmVscy4KX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KQWxzYS1kZXZlbCBt YWlsaW5nIGxpc3QKQWxzYS1kZXZlbEBhbHNhLXByb2plY3Qub3JnCmh0dHA6Ly9tYWlsbWFuLmFs c2EtcHJvamVjdC5vcmcvbWFpbG1hbi9saXN0aW5mby9hbHNhLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: jassisinghbrar@gmail.com (jassi brar) Date: Tue, 26 Jan 2010 21:11:55 +0900 Subject: [PATCH 2/3] ASoC: AC97: S3C: Add controller driver In-Reply-To: <20100126115258.GM15759@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> <1b68c6791001260317k44893521vc0310ea35bac8ee1@mail.gmail.com> <20100126115258.GM15759@rakim.wolfsonmicro.main> Message-ID: <1b68c6791001260411u2b39d8actf35a0f3541fda92d@mail.gmail.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, Jan 26, 2010 at 8:52 PM, Mark Brown wrote: > On Tue, Jan 26, 2010 at 08:17:25PM +0900, jassi brar wrote: >> On Tue, Jan 26, 2010 at 7:47 PM, Mark Brown > >> >> + ? ? 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. > > That's not addressing the problem, though - the big issue is not > bringing up the AC97 link, it's the fact that you're doing an > uncontrolled cold reset. ?If we hit this code path it'll revert the > device registers to default which will upset the drivers for the CODEC > rather badly. ?There should be no possibility of that happening, if it > is happening it's something that callers really should know about. It should never be reached after the link is up and running. Only if something goes wrong at runtime, would the controller state be changed. And this is an attempt to recover from that failure. If the upper layer should know of such failure, then maybe we should not do it. I have never seen such runtime error though. >> Also, this helps not relying on codec/core to perform particular steps >> of initializations. > > Equally well if the controller driver diverges from what other drivers > do it's going to lead to interoperability skew for drivers. > > For the warm reset function I'd suggest adding something which checks to > see if the link is already active and suppresses the warm reset in that > case. ?That will avoid slowing everything down with spurious warm resets > when the link is already runnning and other drivers assume they need to > bring it up - with the way things are structured at present many warm > resets requested by other drivers are likely to be spurious. ok, i will add the check in warm reset. > Ideally the core would keep track of this and transparently trigger the > warm reset when required, but driver local is fine since that's not > there yet. > >> >> + ? ? 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. > > Right, but why is this being done by this function and not, for example, > by the warm reset? ?It's the structure of the code I'm commenting on > rather than the operations that get performed on the hardware. I assume the ALSA definition of cold/warm reset is same as that of the AC97 controller. Anything remaining should be done only when necessary. For ex, the ALSA might want to leave controller after just cold/warm reset ... as the SoC manual says it's in low-power mode without the AC-link on. >> >> +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 :) > > If you'd spelt out label I'd probably have been OK, but in English lb is > normally only an abbreviation for pounds as a unit of weight. ok, will rename the labels.