From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Wu, Songjun" Subject: Re: [PATCH 1/2] ASoC: atmel-classd: add the Audio Class D Amplifier code Date: Tue, 8 Sep 2015 17:36:01 +0800 Message-ID: <55EEAC01.3080409@atmel.com> References: <1441086101-15303-1-git-send-email-songjun.wu@atmel.com> <1441086101-15303-2-git-send-email-songjun.wu@atmel.com> <20150903113716.GU12027@sirena.org.uk> <55EC0AF5.8060403@atmel.com> <20150907162350.GV5313@sirena.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: Received: from DVREDG02.corp.atmel.com (nasmtp01.atmel.com [192.199.1.246]) by alsa0.perex.cz (Postfix) with ESMTP id 87700260666 for ; Tue, 8 Sep 2015 11:36:12 +0200 (CEST) In-Reply-To: <20150907162350.GV5313@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: mark.rutland@arm.com, devicetree@vger.kernel.org, alsa-devel@alsa-project.org, pawel.moll@arm.com, lgirdwood@gmail.com, ijc+devicetree@hellion.org.uk, nicolas.ferre@atmel.com, linux-kernel@vger.kernel.org, tiwai@suse.com, robh+dt@kernel.org, galak@codeaurora.org, linux-arm-kernel@lists.infradead.org List-Id: alsa-devel@alsa-project.org CgpPbiA5LzgvMjAxNSAwMDoyMywgTWFyayBCcm93biB3cm90ZToKPiBPbiBTdW4sIFNlcCAwNiwg MjAxNSBhdCAwNTo0NDoyMVBNICswODAwLCBXdSwgU29uZ2p1biB3cm90ZToKPj4gT24gOS8zLzIw MTUgMTk6MzcsIE1hcmsgQnJvd24gd3JvdGU6Cj4+PiBPbiBUdWUsIFNlcCAwMSwgMjAxNSBhdCAw MTo0MTo0MFBNICswODAwLCBTb25nanVuIFd1IHdyb3RlOgo+Cj4+Pj4gK3N0YXRpYyBjb25zdCBj aGFyICogY29uc3QgZXFjZmdfYmFzc190ZXh0W10gPSB7Cj4+Pj4gKwkiLTEyIGRCIiwgIi02IGRC IiwgIjAgZEIiLCAiKzYgZEIiLCAiKzEyIGRCIgo+Pj4+ICt9Owo+Pj4KPj4+PiArc3RhdGljIGNv bnN0IHVuc2lnbmVkIGludCBlcWNmZ19iYXNzX3ZhbHVlW10gPSB7Cj4+Pj4gKwlDTEFTU0RfSU5U UE1SX0VRQ0ZHX0JfQ1VUXzEyLAo+Pj4+ICsJQ0xBU1NEX0lOVFBNUl9FUUNGR19CX0NVVF82LCBD TEFTU0RfSU5UUE1SX0VRQ0ZHX0ZMQVQsCj4+Pj4gKwlDTEFTU0RfSU5UUE1SX0VRQ0ZHX0JfQk9P U1RfNiwgQ0xBU1NEX0lOVFBNUl9FUUNGR19CX0JPT1NUXzEyCj4+Pj4gK307Cj4KPj4+IFRoaXMg c2hvdWxkIGJlIGEgVm9sdW1lIGNvbnRyb2wgd2l0aCBUTFYgaW5mb3JtYXRpb24sIGFzIHNob3Vs ZCB0aGUKPj4+IGZvbGxvd2luZyBmZXcgY29udHJvbHMuCj4KPj4gVGhlIFZvbHVtZSBjb250cm9s IHdpdGggVExWIGluZm9ybWF0aW9uIGlzIG5vdCBzdWl0YWJsZSBmb3IgdGhpcyBjYXNlLgo+PiBC YXNzLCBNZWRpdW0sIGFuZCB0cmVibGUgYXJlIG11dHVhbGx5IGV4Y2x1c2l2ZS4KPj4gU28gSSB0 aGluayB0aGUgU09DX0VOVU0gY29udHJvbCBpcyBzdWl0YWJsZSBmb3IgdGhpcyBjYXNlLgo+PiBU aGUgcmVnaXN0ZXIgbGF5b3V0IGlzIG5vdCB2ZXJ5IGdvb2QsCj4+IFRoZSByZWdpc3RlciBpcyBk ZWZpbmVkIGFzIGJlbG93Lgo+PiDigKIgIEVRQ0ZHOiBFcXVhbGl6YXRpb24gU2VsZWN0aW9uCj4+ IFZhbHVlIE5hbWUgICAgICAgRGVzY3JpcHRpb24KPj4gMCAgICAgRkxBVCAgICAgICBGbGF0IFJl c3BvbnNlCj4+IDEgICAgIEJCT09TVDEyICAgQmFzcyBib29zdCArMTIgZEIKPj4gMiAgICAgQkJP T1NUNiAgICBCYXNzIGJvb3N0ICs2IGRCCj4+IDMgICAgIEJDVVQxMiAgICAgQmFzcyBjdXQgLTEy IGRCCj4+IDQgICAgIEJDVVQ2ICAgICAgQmFzcyBjdXQgLTYgZEIKPj4gNSAgICAgTUJPT1NUMyAg ICBNZWRpdW0gYm9vc3QgKzMgZEIKPj4gNiAgICAgTUJPT1NUOCAgICBNZWRpdW0gYm9vc3QgKzgg ZEIKPj4gNyAgICAgTUNVVDMgICAgICBNZWRpdW0gY3V0IC0zIGRCCj4+IDggICAgIE1DVVQ4ICAg ICAgTWVkaXVtIGN1dCAtOCBkQgo+PiA5ICAgICBUQk9PU1QxMiAgIFRyZWJsZSBib29zdCArMTIg ZEIKPj4gMTAgICAgVEJPT1NUNiAgICBUcmVibGUgYm9vc3QgKzYgZEIKPj4gMTEgICAgVENVVDEy ICAgICBUcmVibGUgY3V0IC0xMiBkQgo+PiAxMiAgICBUQ1VUNiAgICAgIFRyZWJsZSBjdXQgLTYg ZEIKPgo+IE9LLCBzbyB0aGF0J3Mgbm90IGFjdHVhbGx5IHdoYXQgdGhlIGNvZGUgd2FzIGRvaW5n IC0gaXQgaGFkIHNlcGFyYXRlCj4gZW51bXMgZm9yIGJhc3MsIG1pZCBhbmQgdHJlYmxlLiAgSWYg eW91IG1ha2UgdGhpcyBhIHNpbmdsZSBlbnVtIHdpdGggYWxsCj4gdGhlIGFib3ZlIG9wdGlvbnMg aW4gaXQgdGhhdCBzZWVtcyBsaWtlIHRoZSBiZXN0IHdheSBvZiBoYW5kbGluZyB0aGluZ3MuCj4K QSBzaW5nbGUgZW51bSBzZWVtcyBub3QgdmVyeSBmcmllbmRseSB0byB1c2VyLCB0aGVyZSBhcmUg dHJlZSBFUXMsIGJhc3MsIAptZWRpdW0gYW5kIHRyZWJsZS4KU28gSSBjcmVhdGUgdHJlZSBlbnVt IGNvbnRyb2xzIHRvIGNvbnRyb2wgdGhyZWUgRVFzLgpUaGUgJ2dldCcgZnVuY3Rpb24gaXMgcmVw bGFjZWQgYnkgJ2NsYXNzZF9nZXRfZXFfZW51bScsIGlmIHVzZXIgb3BlcmF0ZXMgCm9uZSBvZiB0 aGUgdHJlZSBFUSBjb250cm9scywgdGhlIG90aGVyIHR3byBFUXMgd2lsbCBzaG93IDAgZEIuCgo+ Pj4+ICtzdGF0aWMgY29uc3Qgc3RydWN0IHNuZF9rY29udHJvbF9uZXcgYXRtZWxfY2xhc3NkX3Nu ZF9jb250cm9sc1tdID0gewo+Pj4+ICtTT0NfU0lOR0xFX1RMVigiTGVmdCBWb2x1bWUiLCBDTEFT U0RfSU5UUE1SLAo+Pj4+ICsJCUNMQVNTRF9JTlRQTVJfQVRUTF9TSElGVCwgNzgsIDEsIGNsYXNz ZF9kaWdpdGFsX3RsdiksCj4+Pj4gKwo+Pj4+ICtTT0NfU0lOR0xFX1RMVigiUmlnaHQgVm9sdW1l IiwgQ0xBU1NEX0lOVFBNUiwKPj4+PiArCQlDTEFTU0RfSU5UUE1SX0FUVFJfU0hJRlQsIDc4LCAx LCBjbGFzc2RfZGlnaXRhbF90bHYpLAo+Pj4KPj4+IFRoaXMgc2hvdWxkIGJlIGEgc2luZ2xlIHN0 ZXJlbyBjb250cm9sIHJhdGhlciB0aGFuIHNlcGFyYXRlIGxlZnQgYW5kCj4+PiByaWdodCBjb250 cm9scy4KPgo+PiBTaW5jZSB0aGUgY2xhc3NEIElQIGRlZmluZXMgdHdvIHJlZ2lzdGVyIGZpZWxk cyB0byBjb250cm9sIGxlZnQgdm9sdW1lIGFuZAo+PiByaWdodCB2b2x1bWUgcmVzcGVjdGl2ZWx5 LCBJIHRoaW5rIGl0J3MgYmV0dGVyIHRvIHByb3ZpZGUgdHdvIGNvbnRyb2xzIHRvCj4+IHVzZXIu Cj4KPiBObywgdGhpcyBpcyByZWFsbHkgY29tbW9uLCB3ZSBjb21iaW5lIHRoZW0gaW4gTGludXgg dG8gcHJlc2VudCBhCj4gY29uc2lzdGVudCBpbnRlcmZhY2UgdG8gdXNlcnNwYWNlLgo+CkkgdGhp bmsgY2FyZWZ1bGx5LCB5b3VyIHN1Z2dlc3Rpb24gaXMgcmVhc29uYWJsZS4KVGhlIGNvZGUgd2ls bCBiZSBtb2RpZmllZCwgY29tYmluZSB0aGUgbGVmdCBhbmQgcmlnaHQgdG8gYSBzaW5nbGUgc3Rl cmVvIApjb250cm9sLgpUaGFuayB5b3UuCgo+Pj4+ICsJZGV2X2luZm8oZGV2LAo+Pj4+ICsJCSJB dG1lbCBDbGFzcyBEIEFtcGxpZmllciAoQ0xBU1NEKSBkZXZpY2UgYXQgMHglcCAoaXJxICVkKVxu IiwKPj4+PiArCQlpb19iYXNlLCBkZC0+aXJxKTsKPgo+Pj4gVGhpcyBpcyBhIGJpdCBub2lzeSBh bmQgbm90IHJlYWxseSBiYXNlZCBvbiBpbnRlcmFjdGlvbiB3aXRoIHRoZQo+Pj4gaGFyZHdhcmUu Li4gIGRldl9kYmcoKSBzZWVtcyBiZXR0ZXIuCj4KPj4gVGhpcyBpbmZvcm1hdGlvbiB3aWxsIG9j Y3VyIG9ubHkgb25jZSB3aGVuIGxpbnV4IGtlcm5lbCBzdGFydHMuCj4+IEl0IHNob3dzIHRoZSBj bGFzc0QgaXMgbG9hZGVkIHRvIGxpbnV4IGtlcm5lbC4KPj4gSSB0aGluayBpdCdzIGJldHRlciB0 byBwcm92aWRlIG1vcmUgaW5mb3JtYXRpb24gdG8gdXNlci4KPgo+IFRoaXMgc3R1ZmYgYWxsIGFk ZHMgdXAgYW5kIHNpbmNlIGl0J2xsIGdvIG91dCBvbiB0aGUgY29uc29sZSBieSBkZWZhdWx0Cj4g aXQgYm90aCBtYWtlcyB0aGluZ3MgbW9yZSBub2lzeSBhbmQgc2xvd3MgZG93biBib290IC0gcHJp bnRpbmcgb24gdGhlCj4gc2VyaWFsIHBvcnQgaXNuJ3QgZnJlZS4gIElmIHdlIHdhbnQgdG8gaGF2 ZSB0aGlzIHNvcnQgb2YgaW5mb3JtYXRpb24gd2UKPiBwcmludGVkIHdlIHNob3VsZCByZWFsbHkg ZG8gaXQgaW4gdGhlIGRyaXZlciBjb3JlIHNvIGl0IGFwcGVhcnMKPiBjb25zaXN0ZW50bHkgZm9y IGFsbCBkZXZpY2VzIHJhdGhlciB0aGFuIGhhdmluZyBpbmRpdmlkdWFsIGNvZGUgaW4gZWFjaAo+ IGRyaXZlci4KPgpBY2NlcHQsIHRoZSBjb2RlIHdpbGwgYmUgbW9kaWZpZWQgdG8gZGV2X2RiZygp LgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpBbHNhLWRl dmVsIG1haWxpbmcgbGlzdApBbHNhLWRldmVsQGFsc2EtcHJvamVjdC5vcmcKaHR0cDovL21haWxt YW4uYWxzYS1wcm9qZWN0Lm9yZy9tYWlsbWFuL2xpc3RpbmZvL2Fsc2EtZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: songjun.wu@atmel.com (Wu, Songjun) Date: Tue, 8 Sep 2015 17:36:01 +0800 Subject: [PATCH 1/2] ASoC: atmel-classd: add the Audio Class D Amplifier code In-Reply-To: <20150907162350.GV5313@sirena.org.uk> References: <1441086101-15303-1-git-send-email-songjun.wu@atmel.com> <1441086101-15303-2-git-send-email-songjun.wu@atmel.com> <20150903113716.GU12027@sirena.org.uk> <55EC0AF5.8060403@atmel.com> <20150907162350.GV5313@sirena.org.uk> Message-ID: <55EEAC01.3080409@atmel.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 9/8/2015 00:23, Mark Brown wrote: > On Sun, Sep 06, 2015 at 05:44:21PM +0800, Wu, Songjun wrote: >> On 9/3/2015 19:37, Mark Brown wrote: >>> On Tue, Sep 01, 2015 at 01:41:40PM +0800, Songjun Wu wrote: > >>>> +static const char * const eqcfg_bass_text[] = { >>>> + "-12 dB", "-6 dB", "0 dB", "+6 dB", "+12 dB" >>>> +}; >>> >>>> +static const unsigned int eqcfg_bass_value[] = { >>>> + CLASSD_INTPMR_EQCFG_B_CUT_12, >>>> + CLASSD_INTPMR_EQCFG_B_CUT_6, CLASSD_INTPMR_EQCFG_FLAT, >>>> + CLASSD_INTPMR_EQCFG_B_BOOST_6, CLASSD_INTPMR_EQCFG_B_BOOST_12 >>>> +}; > >>> This should be a Volume control with TLV information, as should the >>> following few controls. > >> The Volume control with TLV information is not suitable for this case. >> Bass, Medium, and treble are mutually exclusive. >> So I think the SOC_ENUM control is suitable for this case. >> The register layout is not very good, >> The register is defined as below. >> ? EQCFG: Equalization Selection >> Value Name Description >> 0 FLAT Flat Response >> 1 BBOOST12 Bass boost +12 dB >> 2 BBOOST6 Bass boost +6 dB >> 3 BCUT12 Bass cut -12 dB >> 4 BCUT6 Bass cut -6 dB >> 5 MBOOST3 Medium boost +3 dB >> 6 MBOOST8 Medium boost +8 dB >> 7 MCUT3 Medium cut -3 dB >> 8 MCUT8 Medium cut -8 dB >> 9 TBOOST12 Treble boost +12 dB >> 10 TBOOST6 Treble boost +6 dB >> 11 TCUT12 Treble cut -12 dB >> 12 TCUT6 Treble cut -6 dB > > OK, so that's not actually what the code was doing - it had separate > enums for bass, mid and treble. If you make this a single enum with all > the above options in it that seems like the best way of handling things. > A single enum seems not very friendly to user, there are tree EQs, bass, medium and treble. So I create tree enum controls to control three EQs. The 'get' function is replaced by 'classd_get_eq_enum', if user operates one of the tree EQ controls, the other two EQs will show 0 dB. >>>> +static const struct snd_kcontrol_new atmel_classd_snd_controls[] = { >>>> +SOC_SINGLE_TLV("Left Volume", CLASSD_INTPMR, >>>> + CLASSD_INTPMR_ATTL_SHIFT, 78, 1, classd_digital_tlv), >>>> + >>>> +SOC_SINGLE_TLV("Right Volume", CLASSD_INTPMR, >>>> + CLASSD_INTPMR_ATTR_SHIFT, 78, 1, classd_digital_tlv), >>> >>> This should be a single stereo control rather than separate left and >>> right controls. > >> Since the classD IP defines two register fields to control left volume and >> right volume respectively, I think it's better to provide two controls to >> user. > > No, this is really common, we combine them in Linux to present a > consistent interface to userspace. > I think carefully, your suggestion is reasonable. The code will be modified, combine the left and right to a single stereo control. Thank you. >>>> + dev_info(dev, >>>> + "Atmel Class D Amplifier (CLASSD) device at 0x%p (irq %d)\n", >>>> + io_base, dd->irq); > >>> This is a bit noisy and not really based on interaction with the >>> hardware... dev_dbg() seems better. > >> This information will occur only once when linux kernel starts. >> It shows the classD is loaded to linux kernel. >> I think it's better to provide more information to user. > > This stuff all adds up and since it'll go out on the console by default > it both makes things more noisy and slows down boot - printing on the > serial port isn't free. If we want to have this sort of information we > printed we should really do it in the driver core so it appears > consistently for all devices rather than having individual code in each > driver. > Accept, the code will be modified to dev_dbg(). From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754369AbbIHJgO (ORCPT ); Tue, 8 Sep 2015 05:36:14 -0400 Received: from nasmtp01.atmel.com ([192.199.1.246]:17524 "EHLO DVREDG02.corp.atmel.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754323AbbIHJgL (ORCPT ); Tue, 8 Sep 2015 05:36:11 -0400 Subject: Re: [PATCH 1/2] ASoC: atmel-classd: add the Audio Class D Amplifier code To: Mark Brown References: <1441086101-15303-1-git-send-email-songjun.wu@atmel.com> <1441086101-15303-2-git-send-email-songjun.wu@atmel.com> <20150903113716.GU12027@sirena.org.uk> <55EC0AF5.8060403@atmel.com> <20150907162350.GV5313@sirena.org.uk> CC: , , , , , , , , , , , , From: "Wu, Songjun" Organization: ATMEL Message-ID: <55EEAC01.3080409@atmel.com> Date: Tue, 8 Sep 2015 17:36:01 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <20150907162350.GV5313@sirena.org.uk> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 9/8/2015 00:23, Mark Brown wrote: > On Sun, Sep 06, 2015 at 05:44:21PM +0800, Wu, Songjun wrote: >> On 9/3/2015 19:37, Mark Brown wrote: >>> On Tue, Sep 01, 2015 at 01:41:40PM +0800, Songjun Wu wrote: > >>>> +static const char * const eqcfg_bass_text[] = { >>>> + "-12 dB", "-6 dB", "0 dB", "+6 dB", "+12 dB" >>>> +}; >>> >>>> +static const unsigned int eqcfg_bass_value[] = { >>>> + CLASSD_INTPMR_EQCFG_B_CUT_12, >>>> + CLASSD_INTPMR_EQCFG_B_CUT_6, CLASSD_INTPMR_EQCFG_FLAT, >>>> + CLASSD_INTPMR_EQCFG_B_BOOST_6, CLASSD_INTPMR_EQCFG_B_BOOST_12 >>>> +}; > >>> This should be a Volume control with TLV information, as should the >>> following few controls. > >> The Volume control with TLV information is not suitable for this case. >> Bass, Medium, and treble are mutually exclusive. >> So I think the SOC_ENUM control is suitable for this case. >> The register layout is not very good, >> The register is defined as below. >> • EQCFG: Equalization Selection >> Value Name Description >> 0 FLAT Flat Response >> 1 BBOOST12 Bass boost +12 dB >> 2 BBOOST6 Bass boost +6 dB >> 3 BCUT12 Bass cut -12 dB >> 4 BCUT6 Bass cut -6 dB >> 5 MBOOST3 Medium boost +3 dB >> 6 MBOOST8 Medium boost +8 dB >> 7 MCUT3 Medium cut -3 dB >> 8 MCUT8 Medium cut -8 dB >> 9 TBOOST12 Treble boost +12 dB >> 10 TBOOST6 Treble boost +6 dB >> 11 TCUT12 Treble cut -12 dB >> 12 TCUT6 Treble cut -6 dB > > OK, so that's not actually what the code was doing - it had separate > enums for bass, mid and treble. If you make this a single enum with all > the above options in it that seems like the best way of handling things. > A single enum seems not very friendly to user, there are tree EQs, bass, medium and treble. So I create tree enum controls to control three EQs. The 'get' function is replaced by 'classd_get_eq_enum', if user operates one of the tree EQ controls, the other two EQs will show 0 dB. >>>> +static const struct snd_kcontrol_new atmel_classd_snd_controls[] = { >>>> +SOC_SINGLE_TLV("Left Volume", CLASSD_INTPMR, >>>> + CLASSD_INTPMR_ATTL_SHIFT, 78, 1, classd_digital_tlv), >>>> + >>>> +SOC_SINGLE_TLV("Right Volume", CLASSD_INTPMR, >>>> + CLASSD_INTPMR_ATTR_SHIFT, 78, 1, classd_digital_tlv), >>> >>> This should be a single stereo control rather than separate left and >>> right controls. > >> Since the classD IP defines two register fields to control left volume and >> right volume respectively, I think it's better to provide two controls to >> user. > > No, this is really common, we combine them in Linux to present a > consistent interface to userspace. > I think carefully, your suggestion is reasonable. The code will be modified, combine the left and right to a single stereo control. Thank you. >>>> + dev_info(dev, >>>> + "Atmel Class D Amplifier (CLASSD) device at 0x%p (irq %d)\n", >>>> + io_base, dd->irq); > >>> This is a bit noisy and not really based on interaction with the >>> hardware... dev_dbg() seems better. > >> This information will occur only once when linux kernel starts. >> It shows the classD is loaded to linux kernel. >> I think it's better to provide more information to user. > > This stuff all adds up and since it'll go out on the console by default > it both makes things more noisy and slows down boot - printing on the > serial port isn't free. If we want to have this sort of information we > printed we should really do it in the driver core so it appears > consistently for all devices rather than having individual code in each > driver. > Accept, the code will be modified to dev_dbg().