diff mbox

[8/8] ASoC: rsnd: Add Volume Ramp support

Message ID 87tx2w6h2r.wl%kuninori.morimoto.gx@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kuninori Morimoto Oct. 22, 2014, 1:14 a.m. UTC
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

This patch adds Volume Ramp to
Renesas sound driver.

This sample indicates Mute -> Volume 100% -> Mute

amixer set "DVC Out Ramp" 100%		// Mute as default
amixer set "DVC Out Ramp Period" 80%
amixer set "DVC Out Ramp Enable" on
aplay xxx.wav &
amixer set "DVC Out Ramp" 0%		// to Volume 100%
amixer set "DVC Out Ramp" 100%		// to Mute

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/sh/rcar/dvc.c  |   34 ++++++++++++++++++++++++++++++++++
 sound/soc/sh/rcar/gen.c  |    3 +++
 sound/soc/sh/rcar/rsnd.h |    6 ++++++
 3 files changed, 43 insertions(+)

Comments

Mark Brown Oct. 28, 2014, 10:38 p.m. UTC | #1
On Tue, Oct 21, 2014 at 06:14:55PM -0700, Kuninori Morimoto wrote:

> This sample indicates Mute -> Volume 100% -> Mute

> amixer set "DVC Out Ramp" 100%		// Mute as default
> amixer set "DVC Out Ramp Period" 80%

Normally this would be expressed as an enum with some sort of time
units.  We did discuss at the mini-summit making a standard way of
expressing this but nobody did that yet.

> amixer set "DVC Out Ramp Enable" on

That "Enable" should be a Switch control.

> aplay xxx.wav &
> amixer set "DVC Out Ramp" 0%		// to Volume 100%
> amixer set "DVC Out Ramp" 100%		// to Mute

I'm not sure I quite understand this control - what exactly is this
setting?  It appears that the ramp value is the opposite of the volume
we end up with which is a bit odd and the comments suggest that this is
actually triggering a ramp when normally ramps would be done as part of
a volume set, mute or power up/down operation.
Kuninori Morimoto Oct. 29, 2014, 12:26 a.m. UTC | #2
Hi Mark

Thank you for your feedback

> > amixer set "DVC Out Ramp" 100%		// Mute as default
> > amixer set "DVC Out Ramp Period" 80%
> 
> Normally this would be expressed as an enum with some sort of time
> units.  We did discuss at the mini-summit making a standard way of
> expressing this but nobody did that yet.

OK, I can be a 1st guy :)
The HW setting is like below

    value: mean
    00000: 128 dB/1 step
    00001:  64 dB/1 step
    00010:  32 dB/1 step
    ...
    10101: 0.125 dB/2048 steps
    10110: 0.125 dB/4096 steps
    10111: 0.125 dB/8192 steps

Current "DVC Out Ramp Period" wants to have direct value of register.
But, upstream want to have time like this ?

     amixer set "DVC Out Ramp Period" 15000  // 15sec

> > aplay xxx.wav &
> > amixer set "DVC Out Ramp" 0%		// to Volume 100%
> > amixer set "DVC Out Ramp" 100%		// to Mute
> 
> I'm not sure I quite understand this control - what exactly is this
> setting?  It appears that the ramp value is the opposite of the volume
> we end up with which is a bit odd and the comments suggest that this is
> actually triggering a ramp when normally ramps would be done as part of
> a volume set, mute or power up/down operation.

Current "DVC Out Ramp" want to have direct value of register.
It is...

   000:  1 time
   ...
   031:  0.5 time
   ...
   3FE:  4.1 x 10^-7 time
   3FF:  Mute

Maybe I can used inverted value for it ?

      amixer set "DVC Out Ramp" 100%	// to Volume 100%
      amixer set "DVC Out Ramp" 0%	// to Mute
Mark Brown Oct. 29, 2014, 10:56 p.m. UTC | #3
On Tue, Oct 28, 2014 at 05:26:06PM -0700, Kuninori Morimoto wrote:

> > > amixer set "DVC Out Ramp" 100%		// Mute as default
> > > amixer set "DVC Out Ramp Period" 80%

> > Normally this would be expressed as an enum with some sort of time
> > units.  We did discuss at the mini-summit making a standard way of
> > expressing this but nobody did that yet.

> OK, I can be a 1st guy :)
> The HW setting is like below
> 
>     value: mean
>     00000: 128 dB/1 step

> Current "DVC Out Ramp Period" wants to have direct value of register.
> But, upstream want to have time like this ?

Yes, please - much easier for users.

> > > amixer set "DVC Out Ramp" 0%		// to Volume 100%
> > > amixer set "DVC Out Ramp" 100%		// to Mute

> > I'm not sure I quite understand this control - what exactly is this
> > setting?  It appears that the ramp value is the opposite of the volume
> > we end up with which is a bit odd and the comments suggest that this is
> > actually triggering a ramp when normally ramps would be done as part of
> > a volume set, mute or power up/down operation.

> Current "DVC Out Ramp" want to have direct value of register.
> It is...

>    000:  1 time
>    ...
>    031:  0.5 time
>    ...
>    3FE:  4.1 x 10^-7 time
>    3FF:  Mute

> Maybe I can used inverted value for it ?

That sounds like it'd address part of it, though I'm still not 100% sure
what the effect of this control is - is it the final value or something
(0.5*programmed volume for example)?
Kuninori Morimoto Oct. 30, 2014, 12:28 a.m. UTC | #4
Hi Mark

> > Current "DVC Out Ramp Period" wants to have direct value of register.
> > But, upstream want to have time like this ?
> 
> Yes, please - much easier for users.

OK, will try again

> >    000:  1 time
> >    ...
> >    031:  0.5 time
> >    ...
> >    3FE:  4.1 x 10^-7 time
> >    3FF:  Mute
> 
> > Maybe I can used inverted value for it ?
> 
> That sounds like it'd address part of it, though I'm still not 100% sure
> what the effect of this control is - is it the final value or something
> (0.5*programmed volume for example)?

In easy explain,
we can set "Volume Ramp Period" (= it will be "time" instead),
and "Volume scale". see below...

/*
 * we already have default Volume here.
 * I use it as "volume X"
 */

/*
 * This sample want to use "Volume Up"
 * Mute -> volume X
 */

/*
 * target volume for "start" is Mute.
 * volume X x 0 time = volume 0
 */
amixer set "DVC Out Ramp" 0%		// Mute as default

/*
 * We want to Volume up in 15sec
 */
amixer set "DVC Out Ramp Period" 15000

/*
 * Start sound playback with Volume Ramp
 */
amixer set "DVC Out Ramp Enable" on
aplay xxx.wav &

/*
 * Volume up. Mute -> volume X
 * Current Volume = Mute =
 *    volume X x 0 time = volume 0
 * Set next target volume
 *    volume X x 1 time = volume X
 */
amixer set "DVC Out Ramp" 100%		// to Volume 100%

/*
 * Back to Mute again
 * Current Volume =
 *    volume X x 1 time = volume X
 * Set next target volume
 *    volume X x 0 time = volume 0
 */
amixer set "DVC Out Ramp" 0%		// to Mute


Best regards
---
Kuninori Morimoto
Kuninori Morimoto Oct. 30, 2014, 6:11 a.m. UTC | #5
Hi Mark again

> > OK, I can be a 1st guy :)
> > The HW setting is like below
> > 
> >     value: mean
> >     00000: 128 dB/1 step
> 
> > Current "DVC Out Ramp Period" wants to have direct value of register.
> > But, upstream want to have time like this ?
> 
> Yes, please - much easier for users.

I tried to use "time" for Volume Ramp, but it seems difficult.

 1) datasheet indicates Volume Ramp (= x dB / y step)
    but, there is no document "what is step" here.
    (even though HW guy...)

 2) Volume UP/Down time depends on "Volume Ramp"
    and Start/Stop Volume
    If 0% -> 100% takes X   sec,
      50% -> 100% takes X/2 sec.
    But,we don't know final Volume (= 5, 6) when
    user sets "Volume Ramp" (= 1).

        1. amixer set "DVC Out Ramp Period" xxx	// set Period
        2. amixer set "DVC Out Ramp" 0%		// Mute as default
        3. amixer set "DVC Out Ramp" on		// enable Volume Ramp
        4. aplay xxx.wav &
        5. amixer set "DVC Out Ramp" 100%	// to Volume 100%
        6. amixer set "DVC Out Ramp" 0%		// to Mute

  3) If we can calculate it somehow,
     Volume UP/Down speed is not same.
     
     amixer set "DVC Out Ramp Period" 10000	// Volume UP/Down in 10 sec
      0% -> 100%
     50% -> 100%    are happen in 10 sec.
     90% -> 100%
     This is confusable ?

Best regards
---
Kuninori Morimoto
Mark Brown Oct. 31, 2014, 11:51 a.m. UTC | #6
On Wed, Oct 29, 2014 at 05:28:19PM -0700, Kuninori Morimoto wrote:

> > >    000:  1 time
> > >    ...
> > >    031:  0.5 time
> > >    ...
> > >    3FE:  4.1 x 10^-7 time
> > >    3FF:  Mute

> > > Maybe I can used inverted value for it ?

> > That sounds like it'd address part of it, though I'm still not 100% sure
> > what the effect of this control is - is it the final value or something
> > (0.5*programmed volume for example)?

> In easy explain,
> we can set "Volume Ramp Period" (= it will be "time" instead),
> and "Volume scale". see below...

> /*
>  * target volume for "start" is Mute.
>  * volume X x 0 time = volume 0
>  */
> amixer set "DVC Out Ramp" 0%		// Mute as default

I'm sorry, this explanation still isn't at all clear to me.  Can you
explain it in words please?
Mark Brown Oct. 31, 2014, 11:53 a.m. UTC | #7
On Wed, Oct 29, 2014 at 11:11:23PM -0700, Kuninori Morimoto wrote:
> > > OK, I can be a 1st guy :)
> > > The HW setting is like below

> > >     value: mean
> > >     00000: 128 dB/1 step

> > > Current "DVC Out Ramp Period" wants to have direct value of register.
> > > But, upstream want to have time like this ?

> > Yes, please - much easier for users.

> I tried to use "time" for Volume Ramp, but it seems difficult.

No, just quote it as you've listed it above rather than with the
absolute time.
Kuninori Morimoto Nov. 4, 2014, 12:07 a.m. UTC | #8
Hi Mark

> > > >     value: mean
> > > >     00000: 128 dB/1 step
> 
> > > > Current "DVC Out Ramp Period" wants to have direct value of register.
> > > > But, upstream want to have time like this ?
> 
> > > Yes, please - much easier for users.
> 
> > I tried to use "time" for Volume Ramp, but it seems difficult.
> 
> No, just quote it as you've listed it above rather than with the
> absolute time.

Sorry, do you mean list it in source code as comment ?
or, do you mean like this ?

amixer "DVC Ramp time" "128db/1step"
Kuninori Morimoto Nov. 4, 2014, 12:30 a.m. UTC | #9
Hi Mark

> > /*
> >  * target volume for "start" is Mute.
> >  * volume X x 0 time = volume 0
> >  */
> > amixer set "DVC Out Ramp" 0%		// Mute as default
> 
> I'm sorry, this explanation still isn't at all clear to me.  Can you
> explain it in words please?

Our DVC Volume Ramp is doing "Normal volume" x "Volume scale".
"Volume scale" exchange Volume in "X db / Y step" (= speed).
Our "Volume Ramp" exchange this "Volume scale" in "X db / Y step" speed.

"Normal Volume" x 0 => "Normal Volume" x 1

Does this good answer for you ?

Best regards
---
Kuninori Morimoto
Mark Brown Nov. 4, 2014, 11:45 a.m. UTC | #10
On Mon, Nov 03, 2014 at 04:07:44PM -0800, Kuninori Morimoto wrote:

> > No, just quote it as you've listed it above rather than with the
> > absolute time.

> Sorry, do you mean list it in source code as comment ?
> or, do you mean like this ?

> amixer "DVC Ramp time" "128db/1step"

Like this second one (just say 128dB/step if it's only one step).
Kuninori Morimoto Nov. 5, 2014, 12:10 a.m. UTC | #11
Hi Mark

> > > No, just quote it as you've listed it above rather than with the
> > > absolute time.
> 
> > Sorry, do you mean list it in source code as comment ?
> > or, do you mean like this ?
> 
> > amixer "DVC Ramp time" "128db/1step"
> 
> Like this second one (just say 128dB/step if it's only one step).

Maybe this is  bikeshed, but I want to use same as "datasheet words".
Some of them is not "1 step".

	"128 dB/1 step",
	"64 dB/1 step",
	"32 dB/1 step",
	"16 dB/1 step",
	"8 dB/1 step",
	"4 dB/1 step",
	"2 dB/1 step",
	"1 dB/1 step",
	"0.5 dB/1 step",
	"0.25 dB/1 step",
	"0.125 dB/1 step",
	"0.125 dB/2 steps",
	"0.125 dB/4 steps",
	"0.125 dB/8 steps",
	"0.125 dB/16 steps",
	"0.125 dB/32 steps",
	"0.125 dB/64 steps",
	"0.125 dB/128 steps",
	"0.125 dB/256 steps",
	"0.125 dB/512 steps",
	"0.125 dB/1024 steps",
	"0.125 dB/2048 steps",
	"0.125 dB/4096 steps",
	"0.125 dB/8192 steps",
Mark Brown Nov. 5, 2014, 5:24 p.m. UTC | #12
On Tue, Nov 04, 2014 at 04:10:39PM -0800, Kuninori Morimoto wrote:

> > Like this second one (just say 128dB/step if it's only one step).

> Maybe this is  bikeshed, but I want to use same as "datasheet words".
> Some of them is not "1 step".

> 	"0.125 dB/1 step",
> 	"0.125 dB/2 steps",
> 	"0.125 dB/4 steps",

That's absolutely fine - I didn't realise they weren't all a single
step.
Mark Brown Nov. 6, 2014, 4:11 p.m. UTC | #13
On Mon, Nov 03, 2014 at 04:30:00PM -0800, Kuninori Morimoto wrote:
> > > /*
> > >  * target volume for "start" is Mute.
> > >  * volume X x 0 time = volume 0
> > >  */
> > > amixer set "DVC Out Ramp" 0%		// Mute as default

> > I'm sorry, this explanation still isn't at all clear to me.  Can you
> > explain it in words please?

> Our DVC Volume Ramp is doing "Normal volume" x "Volume scale".
> "Volume scale" exchange Volume in "X db / Y step" (= speed).
> Our "Volume Ramp" exchange this "Volume scale" in "X db / Y step" speed.

> "Normal Volume" x 0 => "Normal Volume" x 1

> Does this good answer for you ?

OK, that's what I'd thought that the control should be doing based on
the naming but the examples you were showing were confusing.  Let me
take a look at the new version you posted...
diff mbox

Patch

diff --git a/sound/soc/sh/rcar/dvc.c b/sound/soc/sh/rcar/dvc.c
index 2654ab0..f353f7d 100644
--- a/sound/soc/sh/rcar/dvc.c
+++ b/sound/soc/sh/rcar/dvc.c
@@ -37,6 +37,9 @@  struct rsnd_dvc {
 	struct clk *clk;
 	struct rsnd_dvc_cfg_m volume;
 	struct rsnd_dvc_cfg_m mute;
+	struct rsnd_dvc_cfg_s ren;	/* Ramp Enable */
+	struct rsnd_dvc_cfg_s rperiod;	/* Ramp Period */
+	struct rsnd_dvc_cfg_s rvol;	/* Ramp Volume */
 };
 
 #define rsnd_mod_to_dvc(_mod)	\
@@ -66,6 +69,15 @@  static void rsnd_dvc_volume_update(struct rsnd_mod *mod)
 	rsnd_mod_write(mod, DVC_VOL0R, dvc->volume.val[0]);
 	rsnd_mod_write(mod, DVC_VOL1R, dvc->volume.val[1]);
 
+	/* Enable Ramp */
+	if (dvc->ren.val) {
+		dvucr |= 0x10;
+		rsnd_mod_write(mod, DVC_VRCTR, 0xff);
+		rsnd_mod_write(mod, DVC_VRPDR, dvc->rperiod.val << 8 |
+					       dvc->rperiod.val);
+		rsnd_mod_write(mod, DVC_VRDBR, dvc->rvol.val);
+	}
+
 	/*  Enable Mute */
 	if (mute) {
 		dvucr |= 0x1;
@@ -290,6 +302,28 @@  static int rsnd_dvc_pcm_new(struct rsnd_mod *mod,
 	if (ret < 0)
 		return ret;
 
+	/* Ramp */
+	ret = _rsnd_dvc_pcm_new_s(mod, rdai, rtd,
+			rsnd_dai_is_play(rdai, io) ?
+			"DVC Out Ramp Enable" : "DVC In Ramp Enable",
+			&dvc->ren, 1);
+	if (ret < 0)
+		return ret;
+
+	ret = _rsnd_dvc_pcm_new_s(mod, rdai, rtd,
+			rsnd_dai_is_play(rdai, io) ?
+			"DVC Out Ramp Period" : "DVC In Ramp Period",
+			&dvc->rperiod, 0x17); /* 10111 */
+	if (ret < 0)
+		return ret;
+
+	ret = _rsnd_dvc_pcm_new_s(mod, rdai, rtd,
+			rsnd_dai_is_play(rdai, io) ?
+			"DVC Out Ramp Volume" : "DVC In Ramp Volume",
+			&dvc->rvol, 0x3ff);
+	if (ret < 0)
+		return ret;
+
 	return 0;
 }
 
diff --git a/sound/soc/sh/rcar/gen.c b/sound/soc/sh/rcar/gen.c
index 61dee68..4cb3202 100644
--- a/sound/soc/sh/rcar/gen.c
+++ b/sound/soc/sh/rcar/gen.c
@@ -324,6 +324,9 @@  static int rsnd_gen2_probe(struct platform_device *pdev,
 		RSND_GEN_M_REG(DVC_ADINR,	0xe08,	0x100),
 		RSND_GEN_M_REG(DVC_DVUCR,	0xe10,	0x100),
 		RSND_GEN_M_REG(DVC_ZCMCR,	0xe14,	0x100),
+		RSND_GEN_M_REG(DVC_VRCTR,	0xe18,	0x100),
+		RSND_GEN_M_REG(DVC_VRPDR,	0xe1c,	0x100),
+		RSND_GEN_M_REG(DVC_VRDBR,	0xe20,	0x100),
 		RSND_GEN_M_REG(DVC_VOL0R,	0xe28,	0x100),
 		RSND_GEN_M_REG(DVC_VOL1R,	0xe2c,	0x100),
 		RSND_GEN_M_REG(DVC_DVUER,	0xe48,	0x100),
diff --git a/sound/soc/sh/rcar/rsnd.h b/sound/soc/sh/rcar/rsnd.h
index d119adf..ed44ca8 100644
--- a/sound/soc/sh/rcar/rsnd.h
+++ b/sound/soc/sh/rcar/rsnd.h
@@ -91,6 +91,9 @@  enum rsnd_reg {
 	RSND_REG_SHARE20,
 	RSND_REG_SHARE21,
 	RSND_REG_SHARE22,
+	RSND_REG_SHARE23,
+	RSND_REG_SHARE24,
+	RSND_REG_SHARE25,
 
 	RSND_REG_MAX,
 };
@@ -129,6 +132,9 @@  enum rsnd_reg {
 #define RSND_REG_CMD_CTRL		RSND_REG_SHARE20
 #define RSND_REG_CMDOUT_TIMSEL		RSND_REG_SHARE21
 #define RSND_REG_BUSIF_DALIGN		RSND_REG_SHARE22
+#define RSND_REG_DVC_VRCTR		RSND_REG_SHARE23
+#define RSND_REG_DVC_VRPDR		RSND_REG_SHARE24
+#define RSND_REG_DVC_VRDBR		RSND_REG_SHARE25
 
 struct rsnd_of_data;
 struct rsnd_priv;