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

[Benoît Thébaudeau]

Clemens Ladisch wrote:
> 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.

Now I understand how you see things. But is it an official and absolute ALSA
rule, known by everybody, that may cause issues if not applied?

Here is my point of view. Let's take a real example, which corresponds to the
volume table of the ROHM BD37534 on page 26:
http://www.rohm.com/products/databook/audio/pdf/bd37531fv-e.pdf

This is what gave the 1st example of my patch comment:
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("Main Volume",
		       BD3753X_VOL_GAIN,
		       0, 255, 1, bd3753x_vol_fader_gain_att_tlv),

This solution is the only possible one using existing ALSA SOC TLV macros. This
indeed breaks the rule that you gave above, but is it really an issue? See
below.

The hardware values are:
 0x00 to 0x70: prohibited
 0x71 to 0xcf: +15 dB to -79 dB by -1 dB steps
 0xd0 to 0xfe: prohibited
 0xff: mute

To respect your rule, the driver would have to implement its own .info/.get/.put
callbacks, giving the following mapping:
  SW 0x00: HW 0xff: mute
  SW 0x01 to 0x5f: HW 0x71 to 0xcf: +15 dB to -79 dB by -1 dB steps

But why compel the driver to complicate things this way if it can work with
standard macros and holes in the TLV ranges?

Do you think that it is not a so common case, and hence that it does not deserve
to be handled by the general macros?

It is also the case for the TLV320AIC3104, page 0/register 19/field D6-D3 on
page 51:
http://www.ti.com/lit/ds/symlink/tlv320aic3104.pdf

It is also probably the case for many other existing hardware.

Do you think that the solution would be rather to add common kernel ALSA stuff
to handle such mappings?


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

Because it's useless to wait until the end of the loop to return -EINVAL, and
with my patch, the after-loop code becomes the fallback case for beyond-final-
range values, so it avoids adding a weird extra check there.

Regards,
Benoît