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

[Benoît Thébaudeau]

Dear Peter Ujfalusi,

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) }

/**
 * snd_soc_dapm_get_volsw_mut - dapm mixer get callback
 * @kcontrol: mixer control
 * @ucontrol: control element information
 *
 * Callback to get the value of a dapm mixer control with a hole separating the
 * mute value from the other valid values.
 *
 * Returns 0 for success.
 */
int snd_soc_dapm_get_volsw_mut(struct snd_kcontrol *kcontrol,
    struct snd_ctl_elem_value *ucontrol)
{
    struct snd_soc_dapm_widget_list *wlist = snd_kcontrol_chip(kcontrol);
    struct snd_soc_dapm_widget *widget = wlist->widgets[0];
    struct soc_mixer_control *mc =
        (struct soc_mixer_control *)kcontrol->private_value;
    unsigned int reg = mc->reg;
    unsigned int shift = mc->shift;
    unsigned int rshift = mc->rshift;
    int min = mc->min;
    int max = mc->max;
    unsigned int invert = mc->invert;
    unsigned int mask = (1 << fls(max)) - 1;

    ucontrol->value.integer.value[0] =
        (snd_soc_read(widget->codec, reg) >> shift) & mask;
    if (shift != rshift)
        ucontrol->value.integer.value[1] =
            (snd_soc_read(widget->codec, reg) >> rshift) & mask;
    if (invert) {
        ucontrol->value.integer.value[0] =
            mask - ucontrol->value.integer.value[0];
        if (shift != rshift)
            ucontrol->value.integer.value[1] =
                mask - ucontrol->value.integer.value[1];
    }
    if (ucontrol->value.integer.value[0] > 0)
        ucontrol->value.integer.value[0] -= min - 1;
    if (shift != rshift)
        if (ucontrol->value.integer.value[1] > 0)
            ucontrol->value.integer.value[1] -= min - 1;

    return 0;
}
EXPORT_SYMBOL_GPL(snd_soc_dapm_get_volsw_mut);

/**
 * snd_soc_dapm_put_volsw_mut - dapm mixer set callback
 * @kcontrol: mixer control
 * @ucontrol: control element information
 *
 * Callback to set the value of a dapm mixer control with a hole separating the
 * mute value from the other valid values.
 *
 * Returns 0 for success.
 */
int snd_soc_dapm_put_volsw_mut(struct snd_kcontrol *kcontrol,
    struct snd_ctl_elem_value *ucontrol)
{
    struct snd_soc_dapm_widget_list *wlist = snd_kcontrol_chip(kcontrol);
    struct snd_soc_dapm_widget *widget = wlist->widgets[0];
    struct snd_soc_codec *codec = widget->codec;
    struct soc_mixer_control *mc =
        (struct soc_mixer_control *)kcontrol->private_value;
    unsigned int reg = mc->reg;
    unsigned int shift = mc->shift;
    int min = mc->min;
    int max = mc->max;
    unsigned int mask = (1 << fls(max)) - 1;
    unsigned int invert = mc->invert;
    unsigned int val;
    int connect, change;
    struct snd_soc_dapm_update update;
    int wi;

    val = (ucontrol->value.integer.value[0] & mask);
    connect = !!val;

    if (val)
        val += min - 1;
    if (invert)
        val = mask - val;
    mask = mask << shift;
    val = val << shift;

    mutex_lock(&codec->mutex);

    change = snd_soc_test_bits(widget->codec, reg, mask, val);
    if (change) {
        for (wi = 0; wi < wlist->num_widgets; wi++) {
            widget = wlist->widgets[wi];

            widget->value = val;

            update.kcontrol = kcontrol;
            update.widget = widget;
            update.reg = reg;
            update.mask = mask;
            update.val = val;
            widget->dapm->update = &update;

            dapm_mixer_update_power(widget, kcontrol, connect);

            widget->dapm->update = NULL;
        }
    }

    mutex_unlock(&codec->mutex);
    return 0;
}
EXPORT_SYMBOL_GPL(snd_soc_dapm_put_volsw_mut);

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

Best regards,
Benoît