[alsa-devel] [PATCH] alsa-lib/tlv: fix handling of raw value ranges

[Clemens Ladisch]

Benoît Thébaudeau wrote:
> Clemens Ladisch wrote:
>> Benoît Thébaudeau wrote:
>>> In case of a TLV dB range with all items having raw value ranges strictly within
>>> the main raw value range reported by the driver, snd_tlv_convert_from_dB()
>>> returned one of the main raw range boundaries, which was outside
>>> all dB range items.
>>
>> But the main raw range boundaries _are_ valid values.
>
> Not always. For instance, if a codec register has the following volume values:
> 0x00 to 0x20: prohibited
> 0x21 to 0x40: some dB values
> 0x41 to 0x60: prohibited
> 0x61 to 0xff: some dB values
>
> then the kernel driver will use the existing SOC_SINGLE_TLV macro, which, like
> most TLV macros, automatically sets min to 0. The driver then relies on the
> associated TLV range description to filter out invalid raw values.

All raw values described by the .info callback, returned by the .get
callback, and especially accepted by the .put callback, are valid.

The TLV information just _describes_ values, it cannot _restrict_ them.

If a register has invalid values, the driver must somehow map between
raw control values and valid register values.

>>> static const unsigned int bd3753x_vol_fader_gain_att_tlv[] = {
>>> 	TLV_DB_RANGE_HEAD(2),
>>> 	0, 0, TLV_DB_SCALE_ITEM(TLV_DB_GAIN_MUTE, 0, 1),
>>> 	48, 142, TLV_DB_SCALE_ITEM(-7900, 100, 0),
>>> };
>>> 	SOC_SINGLE_TLV(... 0, 255, 1, bd3753x_vol_fader_gain_att_tlv),
>>>
>>> snd_tlv_convert_from_dB(tlv, 0, 255, 1500, &value, 1) returned 255
>>> instead of the expected 142.
>>
>> But there is no guarantee that this mixer control will have only one of
>> those values in the TLV's range.
>
> What do you mean?

It _is_ possible to have raw values 1..47 and 143..255 (and
convert_to_dB will fail for those).

>> As for the patch, while I'm not sure whether it is needed at all, I also
>> don't quite understand the reason for all the changes.
>>
>>> --- alsa-lib/src/control/tlv.c
>>> +++ alsa-lib/src/control/tlv.c
>>> @@ -291,41 +291,37 @@ int snd_tlv_convert_from_dB(unsigned int
>>>  {
>>>  	switch (tlv[0]) {
>>>  	case SND_CTL_TLVT_DB_RANGE: {
>>> +		long dbmin, dbmax, prev_submax;
>>>  		unsigned int pos, len;
>>>  		len = int_index(tlv[1]);
>>> +		if (len < 6 || len > MAX_TLV_RANGE_SIZE)
>>>  			return -EINVAL;
>>>  		pos = 2;
>>> +		prev_submax = 0;
>>> +		do {
>>
>> Why is this not a while loop?
>
> Because the added "if (len < 6" makes it pointless.

Then why did you add the len<6 check?  This is, in effect, just the same
as the old while condition.


Regards,
Clemens