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@xxxxxx> 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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
