[alsa-devel] [PATCH 0/3] ASoC: twl4030/tpa6130a2: DB_RANGE mapping fixes
Hello,
There were 'holes' in the DB_RANGE and SCALE_ITEM mapping, which results failure, when user space requests dB value in these holes, since alsa-lib will fail to find a range, and can not round the requested dB to raw value.
Fixing the drivers I'm maintaining to correctly fill the tlv struct.
Peter
--- Peter Ujfalusi (3): ASoC: TWL4030: Fix DB_RANGE tlv mapping for digital loopback ASoC: tpa6130a2: Fix DB_RANGE mapping for tpa6130a2 chip ASoC: tpa6130a2: Fix DB_RANGE mapping for tpa6140a2 chip
sound/soc/codecs/tpa6130a2.c | 22 ++++++++++++++-------- sound/soc/codecs/twl4030.c | 4 ++-- 2 files changed, 16 insertions(+), 10 deletions(-)
The TLV_DB_SCALE_ITEMs within the DB_RANGE has to cover the whole range without holes.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com --- sound/soc/codecs/twl4030.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c index 6fd6d0b..7686457 100644 --- a/sound/soc/codecs/twl4030.c +++ b/sound/soc/codecs/twl4030.c @@ -544,8 +544,8 @@ static const struct snd_kcontrol_new twl4030_dapm_abypassv_control = static const unsigned int twl4030_dapm_dbypass_tlv[] = { TLV_DB_RANGE_HEAD(3), 0, 1, TLV_DB_SCALE_ITEM(-3000, 600, 1), - 2, 3, TLV_DB_SCALE_ITEM(-2400, 0, 0), - 4, 7, TLV_DB_SCALE_ITEM(-1800, 600, 0), + 1, 3, TLV_DB_SCALE_ITEM(-2400, 0, 0), + 3, 7, TLV_DB_SCALE_ITEM(-2400, 600, 0), };
/* Digital bypass left (TX1L -> RX2L) */
The TLV_DB_SCALE_ITEMs within the DB_RANGE has to cover the whole range without holes. While fixing the mapping, also change the ranges covered by SCALE_ITEM a bit.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com --- sound/soc/codecs/tpa6130a2.c | 18 ++++++++++++------ 1 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/sound/soc/codecs/tpa6130a2.c b/sound/soc/codecs/tpa6130a2.c index 99b70e5..b15fbdb 100644 --- a/sound/soc/codecs/tpa6130a2.c +++ b/sound/soc/codecs/tpa6130a2.c @@ -234,17 +234,23 @@ static int tpa6130a2_put_volsw(struct snd_kcontrol *kcontrol, * down in gain. */ static const unsigned int tpa6130_tlv[] = { - TLV_DB_RANGE_HEAD(10), + TLV_DB_RANGE_HEAD(16), 0, 1, TLV_DB_SCALE_ITEM(-5950, 600, 0), + 1, 2, TLV_DB_SCALE_ITEM(-5350, 350, 0), 2, 3, TLV_DB_SCALE_ITEM(-5000, 250, 0), + 3, 4, TLV_DB_SCALE_ITEM(-4750, 200, 0), 4, 5, TLV_DB_SCALE_ITEM(-4550, 160, 0), + 5, 6, TLV_DB_SCALE_ITEM(-4390, 250, 0), 6, 7, TLV_DB_SCALE_ITEM(-4140, 190, 0), + 7, 8, TLV_DB_SCALE_ITEM(-3950, 300, 0), 8, 9, TLV_DB_SCALE_ITEM(-3650, 120, 0), - 10, 11, TLV_DB_SCALE_ITEM(-3330, 160, 0), - 12, 13, TLV_DB_SCALE_ITEM(-3040, 180, 0), - 14, 20, TLV_DB_SCALE_ITEM(-2710, 110, 0), - 21, 37, TLV_DB_SCALE_ITEM(-1960, 74, 0), - 38, 63, TLV_DB_SCALE_ITEM(-720, 45, 0), + 9, 10, TLV_DB_SCALE_ITEM(-3530, 200, 0), + 10, 16, TLV_DB_SCALE_ITEM(-3330, 143, 0), + 16, 21, TLV_DB_SCALE_ITEM(-2470, 102, 0), + 21, 29, TLV_DB_SCALE_ITEM(-1960, 83, 0), + 29, 38, TLV_DB_SCALE_ITEM(-1300, 64, 0), + 38, 45, TLV_DB_SCALE_ITEM(-720, 53, 0), + 45, 63, TLV_DB_SCALE_ITEM(-350, 42, 0), };
static const struct snd_kcontrol_new tpa6130a2_controls[] = {
The TLV_DB_SCALE_ITEMs within the DB_RANGE has to cover the whole range without holes.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@nokia.com --- sound/soc/codecs/tpa6130a2.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/tpa6130a2.c b/sound/soc/codecs/tpa6130a2.c index b15fbdb..5df03f8 100644 --- a/sound/soc/codecs/tpa6130a2.c +++ b/sound/soc/codecs/tpa6130a2.c @@ -263,8 +263,8 @@ static const struct snd_kcontrol_new tpa6130a2_controls[] = { static const unsigned int tpa6140_tlv[] = { TLV_DB_RANGE_HEAD(3), 0, 8, TLV_DB_SCALE_ITEM(-5900, 400, 0), - 9, 16, TLV_DB_SCALE_ITEM(-2500, 200, 0), - 17, 31, TLV_DB_SCALE_ITEM(-1000, 100, 0), + 8, 16, TLV_DB_SCALE_ITEM(-2700, 200, 0), + 16, 31, TLV_DB_SCALE_ITEM(-1100, 100, 0), };
static const struct snd_kcontrol_new tpa6140a2_controls[] = {
On Fri, Jul 16, 2010 at 01:17:08PM +0300, Peter Ujfalusi wrote:
There were 'holes' in the DB_RANGE and SCALE_ITEM mapping, which results failure, when user space requests dB value in these holes, since alsa-lib will fail to find a range, and can not round the requested dB to raw value.
Fixing the drivers I'm maintaining to correctly fill the tlv struct.
This seems like a bug in alsa-lib to be honest - it doesn't seem right to specify two values for the indexes at the edge of each range, especially when one of those values is going to be inaccurate.
Hi,
On Friday 16 July 2010 13:24:05 ext Mark Brown wrote:
On Fri, Jul 16, 2010 at 01:17:08PM +0300, Peter Ujfalusi wrote:
There were 'holes' in the DB_RANGE and SCALE_ITEM mapping, which results failure, when user space requests dB value in these holes, since alsa-lib will fail to find a range, and can not round the requested dB to raw value.
Fixing the drivers I'm maintaining to correctly fill the tlv struct.
This seems like a bug in alsa-lib to be honest
I did considered to improve alsa-lib as well, but fixing the snd_tlv_convert_from_dB is a bit harder IMHO.
- it doesn't seem right to specify two values for the indexes at the edge of
each range,
Sort of. The tlv struct is a lookup table for the dB mapping. Query the raw value, and it will return the associated dB. If you ask for dB value, it will go through the table, and try to find the best matching raw value (with up or down rounding).
There are drivers, which using overlapping table: pci/es1938.c soc/codecs/wm8753.c and now the twl, and tpa driver,
The rest is using non overlapping mapping for dB ranges.
especially when one of those values is going to be inaccurate.
The dB values are matching at the important points (raw values).
Let's see... We have the following gain control: 0: -10dB 1: -5dB 2: 0dB 3: 2dB 4: 4dB
in non overlapping (A): static const unsigned int nonoverlapping_tlv[] = { TLV_DB_RANGE_HEAD(2), 0, 2, TLV_DB_SCALE_ITEM(-1000, 500, 0), 3, 4, TLV_DB_SCALE_ITEM(200, 200, 0), };
in overlapping (B): static const unsigned int overlapping_tlv[] = { TLV_DB_RANGE_HEAD(2), 0, 2, TLV_DB_SCALE_ITEM(-1000, 500, 0), 2, 4, TLV_DB_SCALE_ITEM(0, 200, 0), };
In both cases, if you query based on raw value, you get the same dB. In both cases, if you ask for -10, -5, 0, 2, or 4 dB, you get the same raw. In both case, if you ask for less than -10dB, than you get 0 as raw In both case, if you ask for bigger than 4dB, than it is not changing the volume (I think this is really a bug, it should pick 4).
But right now if you ask for dB in the range 0 .. 2 dB: A: will not find it, since none of the two SCALE range covers it. B: will find it and rounds it either up or down. the 0 - 2 dB range is in the second SCALE.
I do think that these patches are carrying correct fix...
Probably alsa-lib can be fixed somehow to look in between the ranges, when it is looking for certain dB value, but at first look it is not that straight forward.
Thanks, Péter
On Fri, Jul 16, 2010 at 02:05:40PM +0300, Peter Ujfalusi wrote:
On Friday 16 July 2010 13:24:05 ext Mark Brown wrote:
- it doesn't seem right to specify two values for the indexes at the edge of
each range,
Sort of. The tlv struct is a lookup table for the dB mapping. Query the raw value, and it will return the associated dB. If you ask for dB value, it will go through the table, and try to find the best matching raw value (with up or down rounding).
Sure, and the forward mapping in particular seems to make it very clear what should be happening here. The way I'd parse this the ability to specify ranges is just a way of saving on typing and we should always end up with single mapping entries between raw values and dB values. The API is inherently getting a list of specific point values, it's just using a compact encoding for some of them.
In both cases, if you query based on raw value, you get the same dB. In both cases, if you ask for -10, -5, 0, 2, or 4 dB, you get the same raw. In both case, if you ask for less than -10dB, than you get 0 as raw In both case, if you ask for bigger than 4dB, than it is not changing the volume (I think this is really a bug, it should pick 4).
Isn't this going to get messy when the increments don't line up so that it's easy to overlap the start of one range with the beginning of the next?
Probably alsa-lib can be fixed somehow to look in between the ranges, when it is looking for certain dB value, but at first look it is not that straight forward.
Well, I'd expect it to do something like go through all the ranges available and look for the closest match which doesn't seem terribly complex. I haven't looked at the code at all, though.
Hi Mark,
On Friday 16 July 2010 15:33:25 ext Mark Brown wrote:
Sure, and the forward mapping in particular seems to make it very clear what should be happening here. The way I'd parse this the ability to specify ranges is just a way of saving on typing and we should always end up with single mapping entries between raw values and dB values. The API is inherently getting a list of specific point values, it's just using a compact encoding for some of them.
If all ranges are are covered (even with overlaps, but with consistent mapping), than alsa-lib needs less fixing. The whole thing boiled down on how the _DB_RANGE is handled in alsa-lib. It goes through the sub _DB_* ranges, and tries to find the correct mapping. If we have 'holes' in the array, than it will fail to find the item. In case of non overlapping array, we can have several 'holes' (in between two DB_SCALE), so alsa-lib seemingly will fail to find the requested dB 'randomly'.
Isn't this going to get messy when the increments don't line up so that it's easy to overlap the start of one range with the beginning of the next?
Well, I assume however writes the driver knows how to build up a lookup table which is consistent, so I don't really see a problem.
Probably alsa-lib can be fixed somehow to look in between the ranges, when it is looking for certain dB value, but at first look it is not that straight forward.
Well, I'd expect it to do something like go through all the ranges available and look for the closest match which doesn't seem terribly complex. I haven't looked at the code at all, though.
I don't share your concern regarding to overlapping array for TLV, but I can fix alsa-lib instead of the drivers to behave correctly. I'll post patches against alsa-lib to fix the _DB_RANGE handling. It will take some time to understand, and fix the code + testing it.
Thanks, Péter
On Mon, Jul 19, 2010 at 08:57:05AM +0300, Peter Ujfalusi wrote:
On Friday 16 July 2010 15:33:25 ext Mark Brown wrote:
Isn't this going to get messy when the increments don't line up so that it's easy to overlap the start of one range with the beginning of the next?
Well, I assume however writes the driver knows how to build up a lookup table which is consistent, so I don't really see a problem.
The problem I see is if the hardware step sizes result in discontinuties in the scale so you can't easily join the top end of one set of values with the bottom end of another you're pretty stuck - joining the ranges up will only work if they can be made to overlap which isn't always going to be the case.
For example, something like:
1: 0 2: 2 3: 4 4: 5 5: 7 6: 9
(a bit artificial but you get the idea) is going to be a bit tricky.
I'll post patches against alsa-lib to fix the _DB_RANGE handling. It will take some time to understand, and fix the code + testing it.
Thanks.
Hi,
I'm waiting for comments for the series against alsa-lib regarding this issue...
On Monday 19 July 2010 11:51:41 ext Mark Brown wrote:
On Mon, Jul 19, 2010 at 08:57:05AM +0300, Peter Ujfalusi wrote:
On Friday 16 July 2010 15:33:25 ext Mark Brown wrote:
Isn't this going to get messy when the increments don't line up so that it's easy to overlap the start of one range with the beginning of the next?
Well, I assume however writes the driver knows how to build up a lookup table which is consistent, so I don't really see a problem.
The problem I see is if the hardware step sizes result in discontinuties in the scale so you can't easily join the top end of one set of values with the bottom end of another you're pretty stuck - joining the ranges up will only work if they can be made to overlap which isn't always going to be the case.
Yeah, and the tpa6130a2 is even worst than most of the things that I have ever seen: http://focus.ti.com/lit/ds/symlink/tpa6130a2.pdf, page 20 Basically there are no continuous ranges at all (sometimes it feels, that the steps are chosen via lottery).
For example, something like:
1: 0 2: 2 3: 4 4: 5 5: 7 6: 9
(a bit artificial but you get the idea) is going to be a bit tricky.
I see, but I still don't see problem with this: non-overlapping (A): static const unsigned int nonoverlapping_tlv[] = { TLV_DB_RANGE_HEAD(2), 1, 3, TLV_DB_SCALE_ITEM(0, 200, 0), 4, 6, TLV_DB_SCALE_ITEM(500, 200, 0), };
overlapping (B): static const unsigned int overlapping_tlv[] = { TLV_DB_RANGE_HEAD(3), 1, 3, TLV_DB_SCALE_ITEM(0, 200, 0), /* at raw 3 the dB is 4 */ 3, 4, TLV_DB_SCALE_ITEM(400, 100, 0), /* at raw 4 the dB is 5 */ 4, 6, TLV_DB_SCALE_ITEM(500, 200, 0), };
B covers all ranges, with 3 sub-range. A covers the ranges, which has 2dB steps, and leaves a hole for the 1dB step between raw 3, and 4.
I'll post patches against alsa-lib to fix the _DB_RANGE handling. It will take some time to understand, and fix the code + testing it.
Thanks.
No problem.
With the patches I have sent for alsa-lib both A and B style of array going to be handled correctly.
If you have time, can you take a look at them?
Thanks, Péter
On Mon, Jul 19, 2010 at 12:14:49PM +0300, Peter Ujfalusi wrote:
I see, but I still don't see problem with this:
Meh, that works but I guess my brain can't imagine concepts that ugly :)
With the patches I have sent for alsa-lib both A and B style of array going to be handled correctly.
If you have time, can you take a look at them?
I had a look, I don't really have much to say - they look like they do the job.
On Monday 19 July 2010 13:30:38 ext Mark Brown wrote:
On Mon, Jul 19, 2010 at 12:14:49PM +0300, Peter Ujfalusi wrote:
I see, but I still don't see problem with this:
Meh, that works but I guess my brain can't imagine concepts that ugly :)
Fair enough ;)
With the patches I have sent for alsa-lib both A and B style of array going to be handled correctly.
If you have time, can you take a look at them?
I had a look, I don't really have much to say - they look like they do the job.
Thanks.
On Monday 19 July 2010 13:30:38 ext Mark Brown wrote:
Meh, that works but I guess my brain can't imagine concepts that ugly :)
Just out of curiosity: you don't like the overlapping method I suppose. Having spent now two days with this, I tend to favor that, since it clearly shows how the raw <-> dB ranges are mapped. Granted, it handles some raw values twice. But, if the alsa-lib series is accepted, than the issue goes away, and I'm going to be happy with that as well.
Thanks again, Péter
At Mon, 19 Jul 2010 11:30:38 +0100, Mark Brown wrote:
On Mon, Jul 19, 2010 at 12:14:49PM +0300, Peter Ujfalusi wrote:
I see, but I still don't see problem with this:
Meh, that works but I guess my brain can't imagine concepts that ugly :)
TLV_DB_MINMAX would be slightly easier to parse:
static const unsigned int tlv[] = { TLV_DB_RANGE_HEAD(3), 1, 3, TLV_DB_MINMAX_ITEM(0, 400), 3, 4, TLV_DB_MINMAX_ITEM(400, 500) 4, 6, TLV_DB_MINMAX_ITEM(500, 900), };
Takashi
Hi,
On Monday 19 July 2010 19:25:55 ext Takashi Iwai wrote:
At Mon, 19 Jul 2010 11:30:38 +0100,
Mark Brown wrote:
On Mon, Jul 19, 2010 at 12:14:49PM +0300, Peter Ujfalusi wrote:
I see, but I still don't see problem with this:
Meh, that works but I guess my brain can't imagine concepts that ugly :)
TLV_DB_MINMAX would be slightly easier to parse:
static const unsigned int tlv[] = { TLV_DB_RANGE_HEAD(3), 1, 3, TLV_DB_MINMAX_ITEM(0, 400), 3, 4, TLV_DB_MINMAX_ITEM(400, 500) 4, 6, TLV_DB_MINMAX_ITEM(500, 900), };
I agree, that the TLV_DB_MINMAX might be easier to digest, but I want to keep the TLV_DB_SCALE in these drivers (tpa6130a2, and tlw4030). I might need to limit the volume range, and that is not really possible with the MINMAX (the ASoC volume limiting only works with TLV_DB_SCALE type).
At Tue, 20 Jul 2010 08:39:16 +0300, Peter Ujfalusi wrote:
Hi,
On Monday 19 July 2010 19:25:55 ext Takashi Iwai wrote:
At Mon, 19 Jul 2010 11:30:38 +0100,
Mark Brown wrote:
On Mon, Jul 19, 2010 at 12:14:49PM +0300, Peter Ujfalusi wrote:
I see, but I still don't see problem with this:
Meh, that works but I guess my brain can't imagine concepts that ugly :)
TLV_DB_MINMAX would be slightly easier to parse:
static const unsigned int tlv[] = { TLV_DB_RANGE_HEAD(3), 1, 3, TLV_DB_MINMAX_ITEM(0, 400), 3, 4, TLV_DB_MINMAX_ITEM(400, 500) 4, 6, TLV_DB_MINMAX_ITEM(500, 900), };
I agree, that the TLV_DB_MINMAX might be easier to digest, but I want to keep the TLV_DB_SCALE in these drivers (tpa6130a2, and tlw4030). I might need to limit the volume range, and that is not really possible with the MINMAX (the ASoC volume limiting only works with TLV_DB_SCALE type).
OK, it sounds reasonable.
thanks,
Takashi
participants (3)
-
Mark Brown -
Peter Ujfalusi -
Takashi Iwai