[PATCH 1/2] ASoC: uda134x: replace a macro with a value in platform struct.

[PATCH 1/2] ASoC: uda134x: replace a macro with a value in platform struct.

[Vladimir Zapolskiy]

This change wipes out a hardcoded macro, which enables codec bias
level control. Now is_powered_on_standby value shall be used instead.

Signed-off-by: Vladimir Zapolskiy <vzapolskiy@gmail.com>
---
 include/sound/uda134x.h    |   12 ++++++++++++
 sound/soc/codecs/uda134x.c |   21 +++++----------------
 2 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/include/sound/uda134x.h b/include/sound/uda134x.h
index 509efb0..e475659 100644
--- a/include/sound/uda134x.h
+++ b/include/sound/uda134x.h
@@ -18,6 +18,18 @@ struct uda134x_platform_data {
 	struct l3_pins l3;
 	void (*power) (int);
 	int model;
+	/*
+	  ALSA SOC usually puts the device in standby mode when it's not used
+	  for sometime. If you unset is_powered_on_standby the driver will
+	  turn off the ADC/DAC when this callback is invoked and turn it back
+	  on when needed. Unfortunately this will result in a very light bump
+	  (it can be audible only with good earphones). If this bothers you
+	  set is_powered_on_standby, you will have slightly higher power
+	  consumption. Please note that sending the L3 command for ADC is
+	  enough to make the bump, so it doesn't make difference if you
+	  completely take off power from the codec.
+	*/
+	int is_powered_on_standby;
 #define UDA134X_UDA1340 1
 #define UDA134X_UDA1341 2
 #define UDA134X_UDA1344 3
diff --git a/sound/soc/codecs/uda134x.c b/sound/soc/codecs/uda134x.c
index e8066b6..5c7b054 100644
--- a/sound/soc/codecs/uda134x.c
+++ b/sound/soc/codecs/uda134x.c
@@ -27,19 +27,6 @@
 #include "uda134x.h"
 
 
-#define POWER_OFF_ON_STANDBY 1
-/*
-  ALSA SOC usually puts the device in standby mode when it's not used
-  for sometime. If you define POWER_OFF_ON_STANDBY the driver will
-  turn off the ADC/DAC when this callback is invoked and turn it back
-  on when needed. Unfortunately this will result in a very light bump
-  (it can be audible only with good earphones). If this bothers you
-  just comment this line, you will have slightly higher power
-  consumption . Please note that sending the L3 command for ADC is
-  enough to make the bump, so it doesn't make difference if you
-  completely take off power from the codec.
- */
-
 #define UDA134X_RATES SNDRV_PCM_RATE_8000_48000
 #define UDA134X_FORMATS (SNDRV_PCM_FMTBIT_S8 | SNDRV_PCM_FMTBIT_S16_LE | \
 		SNDRV_PCM_FMTBIT_S18_3LE | SNDRV_PCM_FMTBIT_S20_3LE)
@@ -559,9 +546,11 @@ static int uda134x_soc_probe(struct platform_device *pdev)
 	codec->num_dai = 1;
 	codec->read = uda134x_read_reg_cache;
 	codec->write = uda134x_write;
-#ifdef POWER_OFF_ON_STANDBY
-	codec->set_bias_level = uda134x_set_bias_level;
-#endif
+
+	if (!pd->is_powered_on_standby) {
+		codec->set_bias_level = uda134x_set_bias_level;
+	}
+
 	INIT_LIST_HEAD(&codec->dapm_widgets);
 	INIT_LIST_HEAD(&codec->dapm_paths);
 
-- 
1.7.0.4

[PATCH 2/2] ASoC: uda134x: fix bias level setup on initialization

[Vladimir Zapolskiy]

On initialization ADC/DAC are enabled only for UDA1341, that's why
bias_level shall be set to off explicitly, otherwise dapm is
misinformed about bias_level on startup.

Signed-off-by: Vladimir Zapolskiy <vzapolskiy@gmail.com>
---
 sound/soc/codecs/uda134x.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/uda134x.c b/sound/soc/codecs/uda134x.c
index 5c7b054..ed13515 100644
--- a/sound/soc/codecs/uda134x.c
+++ b/sound/soc/codecs/uda134x.c
@@ -547,10 +547,6 @@ static int uda134x_soc_probe(struct platform_device *pdev)
 	codec->read = uda134x_read_reg_cache;
 	codec->write = uda134x_write;
 
-	if (!pd->is_powered_on_standby) {
-		codec->set_bias_level = uda134x_set_bias_level;
-	}
-
 	INIT_LIST_HEAD(&codec->dapm_widgets);
 	INIT_LIST_HEAD(&codec->dapm_paths);
 
@@ -561,6 +557,14 @@ static int uda134x_soc_probe(struct platform_device *pdev)
 
 	uda134x_reset(codec);
 
+	if (pd->is_powered_on_standby) {
+		codec->set_bias_level = NULL;
+		uda134x_set_bias_level(codec, SND_SOC_BIAS_ON);
+	} else {
+		codec->set_bias_level = uda134x_set_bias_level;
+		uda134x_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
+	}
+
 	/* register pcms */
 	ret = snd_soc_new_pcms(socdev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1);
 	if (ret < 0) {
-- 
1.7.0.4

Re: [PATCH 1/2] ASoC: uda134x: replace a macro with a value in platform struct.

[Liam Girdwood]

On Thu, 2010-06-24 at 17:38 +0400, Vladimir Zapolskiy wrote:
> This change wipes out a hardcoded macro, which enables codec bias
> level control. Now is_powered_on_standby value shall be used instead.
> 
> Signed-off-by: Vladimir Zapolskiy <vzapolskiy@gmail.com>
> ---
>  include/sound/uda134x.h    |   12 ++++++++++++
>  sound/soc/codecs/uda134x.c |   21 +++++----------------
>  2 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/include/sound/uda134x.h b/include/sound/uda134x.h
> index 509efb0..e475659 100644
> --- a/include/sound/uda134x.h
> +++ b/include/sound/uda134x.h
> @@ -18,6 +18,18 @@ struct uda134x_platform_data {
>  	struct l3_pins l3;
>  	void (*power) (int);
>  	int model;
> +	/*
> +	  ALSA SOC usually puts the device in standby mode when it's not used
> +	  for sometime. If you unset is_powered_on_standby the driver will
> +	  turn off the ADC/DAC when this callback is invoked and turn it back
> +	  on when needed. Unfortunately this will result in a very light bump
> +	  (it can be audible only with good earphones). If this bothers you
> +	  set is_powered_on_standby, you will have slightly higher power
> +	  consumption. Please note that sending the L3 command for ADC is
> +	  enough to make the bump, so it doesn't make difference if you
> +	  completely take off power from the codec.
> +	*/
> +	int is_powered_on_standby;
>  #define UDA134X_UDA1340 1
>  #define UDA134X_UDA1341 2
>  #define UDA134X_UDA1344 3
> diff --git a/sound/soc/codecs/uda134x.c b/sound/soc/codecs/uda134x.c
> index e8066b6..5c7b054 100644
> --- a/sound/soc/codecs/uda134x.c
> +++ b/sound/soc/codecs/uda134x.c
> @@ -27,19 +27,6 @@
>  #include "uda134x.h"
>  
> 
> -#define POWER_OFF_ON_STANDBY 1
> -/*
> -  ALSA SOC usually puts the device in standby mode when it's not used
> -  for sometime. If you define POWER_OFF_ON_STANDBY the driver will
> -  turn off the ADC/DAC when this callback is invoked and turn it back
> -  on when needed. Unfortunately this will result in a very light bump
> -  (it can be audible only with good earphones). If this bothers you
> -  just comment this line, you will have slightly higher power
> -  consumption . Please note that sending the L3 command for ADC is
> -  enough to make the bump, so it doesn't make difference if you
> -  completely take off power from the codec.
> - */
> -
>  #define UDA134X_RATES SNDRV_PCM_RATE_8000_48000
>  #define UDA134X_FORMATS (SNDRV_PCM_FMTBIT_S8 | SNDRV_PCM_FMTBIT_S16_LE | \
>  		SNDRV_PCM_FMTBIT_S18_3LE | SNDRV_PCM_FMTBIT_S20_3LE)
> @@ -559,9 +546,11 @@ static int uda134x_soc_probe(struct platform_device *pdev)
>  	codec->num_dai = 1;
>  	codec->read = uda134x_read_reg_cache;
>  	codec->write = uda134x_write;
> -#ifdef POWER_OFF_ON_STANDBY
> -	codec->set_bias_level = uda134x_set_bias_level;
> -#endif
> +
> +	if (!pd->is_powered_on_standby) {
> +		codec->set_bias_level = uda134x_set_bias_level;

The codec ops pointers have moved in multi-component. Can you put this
check in your uda134x_set_bias_level() instead.

Thanks

Liam
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH 1/2] ASoC: uda134x: replace a macro with a value in platform struct.

[Vladimir Zapolskiy]

Hi,

On Thu, Jun 24, 2010 at 5:49 PM, Liam Girdwood <lrg@slimlogic.co.uk> wrote:
> On Thu, 2010-06-24 at 17:38 +0400, Vladimir Zapolskiy wrote:
>> This change wipes out a hardcoded macro, which enables codec bias
>> level control. Now is_powered_on_standby value shall be used instead.
>>
>> Signed-off-by: Vladimir Zapolskiy <vzapolskiy@gmail.com>
>> ---
>>  include/sound/uda134x.h    |   12 ++++++++++++
>>  sound/soc/codecs/uda134x.c |   21 +++++----------------
>>  2 files changed, 17 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/sound/uda134x.h b/include/sound/uda134x.h
>> index 509efb0..e475659 100644
>> --- a/include/sound/uda134x.h
>> +++ b/include/sound/uda134x.h
>> @@ -18,6 +18,18 @@ struct uda134x_platform_data {
>>       struct l3_pins l3;
>>       void (*power) (int);
>>       int model;
>> +     /*
>> +       ALSA SOC usually puts the device in standby mode when it's not used
>> +       for sometime. If you unset is_powered_on_standby the driver will
>> +       turn off the ADC/DAC when this callback is invoked and turn it back
>> +       on when needed. Unfortunately this will result in a very light bump
>> +       (it can be audible only with good earphones). If this bothers you
>> +       set is_powered_on_standby, you will have slightly higher power
>> +       consumption. Please note that sending the L3 command for ADC is
>> +       enough to make the bump, so it doesn't make difference if you
>> +       completely take off power from the codec.
>> +     */
>> +     int is_powered_on_standby;
>>  #define UDA134X_UDA1340 1
>>  #define UDA134X_UDA1341 2
>>  #define UDA134X_UDA1344 3
>> diff --git a/sound/soc/codecs/uda134x.c b/sound/soc/codecs/uda134x.c
>> index e8066b6..5c7b054 100644
>> --- a/sound/soc/codecs/uda134x.c
>> +++ b/sound/soc/codecs/uda134x.c
>> @@ -27,19 +27,6 @@
>>  #include "uda134x.h"
>>
>>
>> -#define POWER_OFF_ON_STANDBY 1
>> -/*
>> -  ALSA SOC usually puts the device in standby mode when it's not used
>> -  for sometime. If you define POWER_OFF_ON_STANDBY the driver will
>> -  turn off the ADC/DAC when this callback is invoked and turn it back
>> -  on when needed. Unfortunately this will result in a very light bump
>> -  (it can be audible only with good earphones). If this bothers you
>> -  just comment this line, you will have slightly higher power
>> -  consumption . Please note that sending the L3 command for ADC is
>> -  enough to make the bump, so it doesn't make difference if you
>> -  completely take off power from the codec.
>> - */
>> -
>>  #define UDA134X_RATES SNDRV_PCM_RATE_8000_48000
>>  #define UDA134X_FORMATS (SNDRV_PCM_FMTBIT_S8 | SNDRV_PCM_FMTBIT_S16_LE | \
>>               SNDRV_PCM_FMTBIT_S18_3LE | SNDRV_PCM_FMTBIT_S20_3LE)
>> @@ -559,9 +546,11 @@ static int uda134x_soc_probe(struct platform_device *pdev)
>>       codec->num_dai = 1;
>>       codec->read = uda134x_read_reg_cache;
>>       codec->write = uda134x_write;
>> -#ifdef POWER_OFF_ON_STANDBY
>> -     codec->set_bias_level = uda134x_set_bias_level;
>> -#endif
>> +
>> +     if (!pd->is_powered_on_standby) {
>> +             codec->set_bias_level = uda134x_set_bias_level;
>
> The codec ops pointers have moved in multi-component. Can you put this
> check in your uda134x_set_bias_level() instead.
>
Not sure I understand the problem, and I do not see a simple solution
for actual bugfix in patch 2/2, if this check is placed in
uda134x_set_bias_level(). Please could you provide me with a hint or
link to some codec source file?

Thank you,
Vladimir
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/2] ASoC: uda134x: replace a macro with a value in platform struct.

[Liam Girdwood]

On Thu, 2010-06-24 at 19:54 +0400, Vladimir Zapolskiy wrote:
> Hi,
> 
> On Thu, Jun 24, 2010 at 5:49 PM, Liam Girdwood <lrg@slimlogic.co.uk> wrote:
> > On Thu, 2010-06-24 at 17:38 +0400, Vladimir Zapolskiy wrote:

> >> @@ -559,9 +546,11 @@ static int uda134x_soc_probe(struct platform_device *pdev)
> >>       codec->num_dai = 1;
> >>       codec->read = uda134x_read_reg_cache;
> >>       codec->write = uda134x_write;
> >> -#ifdef POWER_OFF_ON_STANDBY
> >> -     codec->set_bias_level = uda134x_set_bias_level;
> >> -#endif
> >> +
> >> +     if (!pd->is_powered_on_standby) {
> >> +             codec->set_bias_level = uda134x_set_bias_level;
> >
> > The codec ops pointers have moved in multi-component. Can you put this
> > check in your uda134x_set_bias_level() instead.
> >
> Not sure I understand the problem, and I do not see a simple solution
> for actual bugfix in patch 2/2, if this check is placed in
> uda134x_set_bias_level(). Please could you provide me with a hint or
> link to some codec source file?

I was meaning something like :-

uda134x_set_bias_level()
{
	if (pd->is_powered_on_standby)
		return

	/* rest of func */

}

The multi-component branch makes codec->set_bias_level a driver level
operation (that can be shared amongst other uda134x codec devices so we
cant change this at runtime)

Thanks

Liam
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH 1/2] ASoC: uda134x: replace a macro with a value in platform struct.

[Vladimir Zapolskiy]

On Thu, Jun 24, 2010 at 8:05 PM, Liam Girdwood <lrg@slimlogic.co.uk> wrote:
> On Thu, 2010-06-24 at 19:54 +0400, Vladimir Zapolskiy wrote:
>> Hi,
>>
>> On Thu, Jun 24, 2010 at 5:49 PM, Liam Girdwood <lrg@slimlogic.co.uk> wrote:
>> > On Thu, 2010-06-24 at 17:38 +0400, Vladimir Zapolskiy wrote:
>
>> >> @@ -559,9 +546,11 @@ static int uda134x_soc_probe(struct platform_device *pdev)
>> >>       codec->num_dai = 1;
>> >>       codec->read = uda134x_read_reg_cache;
>> >>       codec->write = uda134x_write;
>> >> -#ifdef POWER_OFF_ON_STANDBY
>> >> -     codec->set_bias_level = uda134x_set_bias_level;
>> >> -#endif
>> >> +
>> >> +     if (!pd->is_powered_on_standby) {
>> >> +             codec->set_bias_level = uda134x_set_bias_level;
>> >
>> > The codec ops pointers have moved in multi-component. Can you put this
>> > check in your uda134x_set_bias_level() instead.
>> >
>> Not sure I understand the problem, and I do not see a simple solution
>> for actual bugfix in patch 2/2, if this check is placed in
>> uda134x_set_bias_level(). Please could you provide me with a hint or
>> link to some codec source file?
>
> I was meaning something like :-
>
> uda134x_set_bias_level()
> {
>        if (pd->is_powered_on_standby)
>                return
>
>        /* rest of func */
>
> }
>
Thank you, good that I meant the same. But that makes quite impossible
to set a mandatory bias level on initialization without additional code
complication.

> The multi-component branch makes codec->set_bias_level a driver level
> operation (that can be shared amongst other uda134x codec devices so we
> cant change this at runtime)
>
Looks like I miss something, because in codec drivers I observe
codec->set_bias_level is set in probe function, like it is in uda134x.

Would it be better just to drop a condition of pd->is_powered_on_standby?
What do you think?

Vladimir
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/2] ASoC: uda134x: replace a macro with a value in platform struct.

[Liam Girdwood]

On Thu, 2010-06-24 at 20:21 +0400, Vladimir Zapolskiy wrote:
> On Thu, Jun 24, 2010 at 8:05 PM, Liam Girdwood <lrg@slimlogic.co.uk> wrote:
> > On Thu, 2010-06-24 at 19:54 +0400, Vladimir Zapolskiy wrote:
> >> Hi,
> >>
> >> On Thu, Jun 24, 2010 at 5:49 PM, Liam Girdwood <lrg@slimlogic.co.uk> wrote:
> >> > On Thu, 2010-06-24 at 17:38 +0400, Vladimir Zapolskiy wrote:
> >
> >> >> @@ -559,9 +546,11 @@ static int uda134x_soc_probe(struct platform_device *pdev)
> >> >>       codec->num_dai = 1;
> >> >>       codec->read = uda134x_read_reg_cache;
> >> >>       codec->write = uda134x_write;
> >> >> -#ifdef POWER_OFF_ON_STANDBY
> >> >> -     codec->set_bias_level = uda134x_set_bias_level;
> >> >> -#endif
> >> >> +
> >> >> +     if (!pd->is_powered_on_standby) {
> >> >> +             codec->set_bias_level = uda134x_set_bias_level;
> >> >
> >> > The codec ops pointers have moved in multi-component. Can you put this
> >> > check in your uda134x_set_bias_level() instead.
> >> >
> >> Not sure I understand the problem, and I do not see a simple solution
> >> for actual bugfix in patch 2/2, if this check is placed in
> >> uda134x_set_bias_level(). Please could you provide me with a hint or
> >> link to some codec source file?
> >
> > I was meaning something like :-
> >
> > uda134x_set_bias_level()
> > {
> >        if (pd->is_powered_on_standby)
> >                return
> >
> >        /* rest of func */
> >
> > }
> >
> Thank you, good that I meant the same. But that makes quite impossible
> to set a mandatory bias level on initialization without additional code
> complication.
> 

Sorry, I'm just trying to make the multi-component merge simpler here.

The multi-component branch is here :-

http://git.kernel.org/?p=linux/kernel/git/lrg/asoc-2.6.git;a=shortlog;h=refs/heads/topic/multi-component

I'll merge this myself later.

> > The multi-component branch makes codec->set_bias_level a driver level
> > operation (that can be shared amongst other uda134x codec devices so we
> > cant change this at runtime)
> >
> Looks like I miss something, because in codec drivers I observe
> codec->set_bias_level is set in probe function, like it is in uda134x.
> 
> Would it be better just to drop a condition of pd->is_powered_on_standby?
> What do you think?

Both looks fine.

Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH 1/2] ASoC: uda134x: replace a macro with a value in platform struct.

[Mark Brown]

On Thu, Jun 24, 2010 at 05:38:50PM +0400, Vladimir Zapolskiy wrote:
> This change wipes out a hardcoded macro, which enables codec bias
> level control. Now is_powered_on_standby value shall be used instead.
> 
> Signed-off-by: Vladimir Zapolskiy <vzapolskiy@gmail.com>

Applied both, thanks.  Like I say, the first patch really should be done
by using DAPM rather than this custom code but it's still an improvement
on the existing code.