lkml.org 
[lkml]   [2015]   [Sep]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/2] ASoC: atmel-classd: add the Audio Class D Amplifier code
From
Date


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().


\
 
 \ /
  Last update: 2015-09-08 12:01    [W:1.572 / U:0.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site