[alsa-devel] [PATCH RFT] ASoC: cs42l56: Fix new value argument in snd_soc_update_bits calls

[Axel Lin]

2014-05-23 1:56 GMT+08:00 Brian Austin <brian.austin at cirrus.com>:
> On Tue, 13 May 2014, Axel Lin wrote:
>
>>
>> The new value argument needs proper shift to match the mask bit fields.
>>
>> Signed-off-by: Axel Lin <axel.lin at ingics.com>
>> ---
>> Hi Brian,
>> This patch replaces
>> [PATCH RFT] ASoC: cs42l56: Fix update mute register bits in
>> cs42l56_digital_mute.
>> I just found more places of snd_soc_update_bits calls needs fix.
>> Regards,
>> Axel
>> sound/soc/codecs/cs42l56.c | 71
>> ++++++++++++++++++----------------------------
>> sound/soc/codecs/cs42l56.h | 14 ++++-----
>> 2 files changed, 35 insertions(+), 50 deletions(-)
>>
>> diff --git a/sound/soc/codecs/cs42l56.c b/sound/soc/codecs/cs42l56.c
>> index 5bb134b..c0be526 100644
>> --- a/sound/soc/codecs/cs42l56.c
>> +++ b/sound/soc/codecs/cs42l56.c
>> {
>>         struct snd_soc_codec *codec = dai->codec;
>> +       unsigned int mask;
>>
>>         if (mute) {
>>                 /* Hit the DSP Mixer first */
>> -               snd_soc_update_bits(codec, CS42L56_DSP_MUTE_CTL,
>> -                                   CS42L56_ADCAMIX_MUTE_MASK |
>> -                               CS42L56_ADCBMIX_MUTE_MASK |
>> -                               CS42L56_PCMAMIX_MUTE_MASK |
>> -                               CS42L56_PCMBMIX_MUTE_MASK |
>> -                               CS42L56_MSTB_MUTE_MASK |
>> -                               CS42L56_MSTA_MUTE_MASK,
>> -                               CS42L56_MUTE);
>> +               mask = CS42L56_ADCAMIX_MUTE_MASK |
>> CS42L56_ADCBMIX_MUTE_MASK |
>> +                      CS42L56_PCMAMIX_MUTE_MASK |
>> CS42L56_PCMBMIX_MUTE_MASK |
>> +                      CS42L56_MSTB_MUTE_MASK | CS42L56_MSTA_MUTE_MASK;
>> +               snd_soc_update_bits(codec, CS42L56_DSP_MUTE_CTL, mask,
>> mask);
>> +
>
>
> I see where my MUTE is not actually doing what I want and since this is for
> just .digital_mute, I would rather just declare a MUTE_ALL define instead of
> adding variables to the function
>
>
>>                 snd_soc_update_bits(codec, CS42L56_LOB_VOLUME,
>> -                               CS42L56_LO_MUTE_MASK,
>> -                               CS42L56_MUTE);
>> +                                   CS42L56_LO_MUTE_MASK,
>> CS42L56_LO_MUTE_MASK);
>
>
> Yes, this is my fault for not understanding this correctly. I don't like it
> and think odd you have to do it like this BTW.
It's to set CS42L56_LO_MUTE_MASK bit (0x80).

>
>
>>
>>
>>         } else {
>> -               snd_soc_update_bits(codec, CS42L56_DSP_MUTE_CTL,
>> -                                   CS42L56_ADCAMIX_MUTE_MASK |
>> -                               CS42L56_ADCBMIX_MUTE_MASK |
>> -                               CS42L56_PCMAMIX_MUTE_MASK |
>> -                               CS42L56_PCMBMIX_MUTE_MASK |
>> -                               CS42L56_MSTB_MUTE_MASK |
>> -                               CS42L56_MSTA_MUTE_MASK,
>> -                               CS42L56_UNMUTE);
>> -               snd_soc_update_bits(codec, CS42L56_MISC_ADC_CTL,
>> -                               CS42L56_ADCA_MUTE_MASK |
>> -                               CS42L56_ADCB_MUTE_MASK,
>> -                               CS42L56_UNMUTE);
>
>
> This should work fine.
>
>
>> -                               CS42L56_UNMUTE);
>> +                                   CS42L56_LO_MUTE_MASK, 0);
>
> What is the difference here? Consistency?

>From my point of view, pass 0 here means to clear the MASK bits.
Note, the mask bit is different in various snd_soc_update_bits calls here,
so it only make sense if CS42L56_UNMUTE is 0.
In this case, CS42L56_UNMUTE actually is defined as 0. so either is ok.

>
>
>>         }
>>         return 0;
>> }
>> diff --git a/sound/soc/codecs/cs42l56.h b/sound/soc/codecs/cs42l56.h
>> index ad2b50a..d2f68c6 100644
>> --- a/sound/soc/codecs/cs42l56.h
>> +++ b/sound/soc/codecs/cs42l56.h
>> @@ -80,19 +80,21 @@
>> #define CS42L56_PDN_HPB_MASK            0xc0
>>
>> /* serial port and clk masks */
>> -#define CS42L56_MASTER_MODE            1
>> -#define CS42L56_SLAVE_MODE             0
>> +#define CS42L56_MASTER_MODE            (1 << 6)
>> +#define CS42L56_SLAVE_MODE             (0 << 6)
>> #define CS42L56_MS_MODE_MASK            0x40
>> -#define CS42L56_SCLK_INV               1
>> +#define CS42L56_SCLK_INV               (1 << 5)
>> #define CS42L56_SCLK_INV_MASK           0x20
>> #define CS42L56_SCLK_MCLK_MASK          0x18
>> +#define CS42L56_MCLK_PREDIV            (1 << 6)
>
> Your way off on this one. It is the third bit in the register

Fixed in v2.

>
> In general for myself I am trying to get away from the "shift" defines and
> use something more meaningful with just a value. I have not tested yet as I
It's nothing wrong using the "shift" in defines.
I personal think using "shift" in the defines makes it easier to
understand the meaning which is mapping
to the bits documented in datasheet.
But since you prefer not using the "shift", I updated the defines in v2.

Thanks for the review,
Axel