On Wed, Feb 4, 2015 at 1:35 PM, Peter Ujfalusi peter.ujfalusi@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@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