[alsa-devel] [PATCH v2] ASoC: tlv320aic3x: Add support for tlv320aic3104

[Benoît Thébaudeau]

On Wed, Feb 4, 2015 at 1:35 PM, Peter Ujfalusi <peter.ujfalusi at ti.com> wrote:
> On 02/04/2015 01:11 PM, Benoît Thébaudeau wrote:
>> On Wed, Feb 4, 2015 at 11:02 AM, Peter Ujfalusi <peter.ujfalusi at ti.com> wrote:
>>> On 02/04/2015 11:33 AM, Benoît Thébaudeau wrote:
>>>> What I meant was to do the following instead:
>>>>
>>>> /*
>>>>  * ADC PGA mix input volumes. From -12 to 0 dB in 1.5 dB steps. Disconnected
>>>>  * below -12 dB
>>>>  */
>>>> static const DECLARE_TLV_DB_SCALE(mix_tlv, -1350, 150, 1);
>>>>
>>>> /* Left PGA Mixer for tlv320aic3104 */
>>>> static const struct snd_kcontrol_new aic3104_left_pga_mixer_controls[] = {
>>>>     SOC_DAPM_SINGLE_MUT_TLV("Line1L Volume",
>>>>                 LINE1L_2_LADC_CTRL, 3, 7, 15, 1, mix_tlv),
>>>
>>> You mean SOC_DAPM_SINGLE_TLV() ?
>>
>> No. It would not fit our needs here.
>>
>>> There is no _MUT_ variant AFAIK.
>>
>> Indeed, but support could be added, for it, something like (might need
>> updating and some other changes in DAPM):
>>
>> #define SOC_DAPM_SINGLE_MUT_TLV(xname, reg, shift, min, max, invert,
>> tlv_array) \
>> {    .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \
>>     .info = snd_soc_info_volsw_mut, \
>>     .access = SNDRV_CTL_ELEM_ACCESS_TLV_READ | SNDRV_CTL_ELEM_ACCESS_READWRITE,\
>>     .tlv.p = (tlv_array), \
>>     .get = snd_soc_dapm_get_volsw_mut, .put = snd_soc_dapm_put_volsw_mut, \
>>     .private_value = SOC_SINGLE_MUT_VALUE(reg, shift, min, max, invert) }
>
> We would need more than this for aic3x driver. Something which handles:
> valid field values: 0 - 8 (inverted or not)
> Plus when mute is asked we should write 0xf (mute value)
> 0b1001 - 0b1110 is reserved value on all codecs.
>
> #define SOC_DAPM_SINGLE_MUT_TLV(xname, reg, shift, min, max, mute_val,
>                                 invert, tlv_array)

mute_val could be deduced from max thanks to invert. This is what I
did with SOC_DAPM_SINGLE_MUT_TLV(). See the value 15 passed to it.

>>> According to the datasheets of aic3106, aic3104:
>>> 0b0000 - 0b1000 is valid (0 - -12 dB)
>>> 0b1001 - 0b1110 is reserved, do not write these sequences to the register
>>> 0b1111 is disconnected.
>>>
>>> And for fun the aic3007's reg20 for example:
>>> 0b0000 - 0dB
>>> 0b0001 - 0b0011 - reserved
>>> 0b0100 - -6dB
>>> 0b0101 - 0b0111 - reserved
>>> 0b1000 - -12dB
>>> 0b1001 - 0b1110 - reserved
>>> 0b1111 - disconnected
>>>
>>> Note that the driver never had control for these gains and the aic3104 support
>>> is following this behavior, but if we do it for aic3104 we should do it for
>>> all other support codecs as well as a followup patch or series. IMHO.
>>
>> I had not checked the other CODECs. So for all these CODECs, toggling
>> either as 0b0000/0b0001 or as 0b1110/0b1111 (like the driver currently
>> does) is wrong.
>
> The driver toggles between 0b0000 and 0b1111 currently, which is correct:
> # amixer sset 'Left PGA Mixer Line1L' off
> snd_soc_dapm_put_volsw_aic3x 1: reg: 0x13, val: 0x00
> snd_soc_dapm_put_volsw_aic3x 2: val: 0x78, mask: 0x78
>
> # amixer sset 'Left PGA Mixer Line1L' on
> snd_soc_dapm_put_volsw_aic3x 1: reg: 0x13, val: 0x01
> snd_soc_dapm_put_volsw_aic3x 2: val: 0x00, mask: 0x78

So the current behavior is indeed correct, but incomplete.

>> I agree that they should be fixed too. But for them,
>> it's even more complicated because of the multiple reserved value
>> ranges. Or maybe we could just ignore these reserved ranges and handle
>> the mute value, but that would mean relying on the user not to use
>> reserved values. So maybe use TLV_DB_RANGE_HEAD() and
>> TLV_DB_SCALE_ITEM() to define mix_tlv, then use
>> SOC_(DAPM_)SINGLE_TLV().
>
> I myself also wondered time to time why the driver was written the way it has
> been regarding to these gain controls.

Yes. I think that TLV_DB_RANGE_HEAD() and TLV_DB_SCALE_ITEM() would be
a good solution to get rid of SOC_DAPM_SINGLE_AIC3X() /
snd_soc_dapm_put_volsw_aic3x() while fully implementing these controls
as volumes. That would be worth checking.

Best regards,
Benoît