Re: [PATCH v2] ASoC: tlv320aic3x: Add support for tlv320aic3104

[Peter Ujfalusi <peter.ujfalusi@ti.com>]

On 02/04/2015 01:11 PM, Benoît Thébaudeau wrote:
> Dear Peter Ujfalusi,
> 
> On Wed, Feb 4, 2015 at 11:02 AM, Peter Ujfalusi <peter.ujfalusi@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)

> /**
>  * 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.

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

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

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html