[PATCH 0/3] ASoC: twl4030/tpa6130a2: DB_RANGE mapping fixes

[PATCH 0/3] ASoC: twl4030/tpa6130a2: DB_RANGE mapping fixes

[Peter Ujfalusi]

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(-)

[PATCH 1/3] ASoC: TWL4030: Fix DB_RANGE tlv mapping for digital loopback

[Peter Ujfalusi]

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) */
-- 
1.7.1.1

[PATCH 2/3] ASoC: tpa6130a2: Fix DB_RANGE mapping for tpa6130a2 chip

[Peter Ujfalusi]

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[] = {
-- 
1.7.1.1

[PATCH 3/3] ASoC: tpa6130a2: Fix DB_RANGE mapping for tpa6140a2 chip

[Peter Ujfalusi]

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[] = {
-- 
1.7.1.1

Re: [PATCH 0/3] ASoC: twl4030/tpa6130a2: DB_RANGE mapping fixes

[Mark Brown]

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.

Re: [PATCH 0/3] ASoC: twl4030/tpa6130a2: DB_RANGE mapping fixes

[Peter Ujfalusi]

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

Re: [PATCH 0/3] ASoC: twl4030/tpa6130a2:?DB_RANGE mapping fixes

[Mark Brown]

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.

Re: [PATCH 0/3] ASoC: twl4030/tpa6130a2:?DB_RANGE mapping fixes

[Peter Ujfalusi]

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

Re: [PATCH 0/3] ASoC: twl4030/tpa6130a2:?DB_RANGE mapping fixes

[Mark Brown]

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.

Re: [PATCH 0/3] ASoC: twl4030/tpa6130a2:?DB_RANGE mapping fixes

[Peter Ujfalusi]

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

Re: [PATCH 0/3] ASoC: twl4030/tpa6130a2:?DB_RANGE mapping fixes

[Mark Brown]

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.

Re: [PATCH 0/3] ASoC: twl4030/tpa6130a2:?DB_RANGE mapping fixes

[Peter Ujfalusi]

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.

-- 
Péter

Re: [PATCH 0/3] ASoC: twl4030/tpa6130a2:?DB_RANGE mapping fixes

[Peter Ujfalusi]

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

Re: [PATCH 0/3] ASoC: twl4030/tpa6130a2:?DB_RANGE mapping fixes

[Takashi Iwai]

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

Re: [PATCH 0/3] ASoC: twl4030/tpa6130a2:?DB_RANGE mapping fixes

[Peter Ujfalusi]

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

-- 
Péter

Re: [PATCH 0/3] ASoC: twl4030/tpa6130a2:?DB_RANGE mapping fixes

[Takashi Iwai]

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