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

[Benoît Thébaudeau]

Takashi Iwai wrote:
> At Fri, 30 Mar 2012 13:44:43 +0200,
> Takashi Iwai wrote:
> >
> > At Fri, 30 Mar 2012 12:41:44 +0200 (CEST),
> > 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
> >
> > You can't prohibit the values that are exposed as accessible.
> > The driver may return an error -EINVAL or such, but it doesn't mean
> > that it's allowed to give a hole in the TLV dB definition.
>
> But, don't get me wrong with the statement above.  Your fix can be an
> improvement indeed.  The fact that the function relies on
> range_min/max blindly isn't described anywhere, and it'd be better to
> parse the range properly, of course, especially it's an exported
> function, not only used as an internal function.
>
> There may be minor coding issues as Clemens suggested, but they can
> be
> easily fixed.
>
> So, the motivation here is to make the code more robust and
> consistent

Yes, that's what this patch aims at.

> although the examples you raised seem like rather buggy
> driver implementations.

Why? And you say that there shouldn't be holes in TLV ranges. I don't see why
either. I have not read anywhere that the ALSA APIs forbid this, and by the way
it works. Moreover, the example above with some prohibited ranges would be a
hardware example, not a driver example. It should be possible to handle all
hardware cases.

Regards,
Benoît