[PATCH RFC 0/3] asoc: uda1380 cleanup

[PATCH RFC 0/3] asoc: uda1380 cleanup

[Vasily Khoruzhick]

This patch series cleanups uda1380 codec driver a bit:
- it introduces callbacks instead of direct gpiolib usage, as some
machines requires more actions than just plain gpio toggle to
enable/disable/reset codec;
- it makes driver more powersave-friendly and fixes suspend/resume

[PATCH RFC 1/3] ASoC: uda1380: use callbacks instead of gpiolib

[Vasily Khoruzhick]

Some machines require some tricks to enable/disable
codec, i.e. disable or enable i2s clock before enabling/disabling
codec, and just configuring gpio is not enough; some machines
have no reset pin (reset is performed on codec power on automatically).
Fix that issue by using machine-specific callback to enable codec power.

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 include/sound/uda1380.h    |    3 +--
 sound/soc/codecs/uda1380.c |   25 +++----------------------
 2 files changed, 4 insertions(+), 24 deletions(-)

diff --git a/include/sound/uda1380.h b/include/sound/uda1380.h
index 381319c..d2171dc 100644
--- a/include/sound/uda1380.h
+++ b/include/sound/uda1380.h
@@ -12,8 +12,7 @@
 #define __UDA1380_H
 
 struct uda1380_platform_data {
-	int gpio_power;
-	int gpio_reset;
+	void (*set_power)(int);
 	int dac_clk;
 #define UDA1380_DAC_CLK_SYSCLK 0
 #define UDA1380_DAC_CLK_WSPLL  1
diff --git a/sound/soc/codecs/uda1380.c b/sound/soc/codecs/uda1380.c
index 2f925a2..2b7f200 100644
--- a/sound/soc/codecs/uda1380.c
+++ b/sound/soc/codecs/uda1380.c
@@ -19,7 +19,6 @@
 #include <linux/types.h>
 #include <linux/slab.h>
 #include <linux/errno.h>
-#include <linux/gpio.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/workqueue.h>
@@ -752,22 +751,11 @@ static int uda1380_register(struct uda1380_priv *uda1380)
 		return -EINVAL;
 	}
 
-	if (!pdata || !pdata->gpio_power || !pdata->gpio_reset)
+	if (!pdata || !pdata->set_power)
 		return -EINVAL;
 
-	ret = gpio_request(pdata->gpio_power, "uda1380 power");
-	if (ret)
-		goto err_out;
-	ret = gpio_request(pdata->gpio_reset, "uda1380 reset");
-	if (ret)
-		goto err_gpio;
-
-	gpio_direction_output(pdata->gpio_power, 1);
-
 	/* we may need to have the clock running here - pH5 */
-	gpio_direction_output(pdata->gpio_reset, 1);
-	udelay(5);
-	gpio_set_value(pdata->gpio_reset, 0);
+	pdata->set_power(1);
 
 	mutex_init(&codec->mutex);
 	INIT_LIST_HEAD(&codec->dapm_widgets);
@@ -818,11 +806,6 @@ static int uda1380_register(struct uda1380_priv *uda1380)
 err_dai:
 	snd_soc_unregister_codec(codec);
 err_reset:
-	gpio_set_value(pdata->gpio_power, 0);
-	gpio_free(pdata->gpio_reset);
-err_gpio:
-	gpio_free(pdata->gpio_power);
-err_out:
 	return ret;
 }
 
@@ -834,9 +817,7 @@ static void uda1380_unregister(struct uda1380_priv *uda1380)
 	snd_soc_unregister_dais(uda1380_dai, ARRAY_SIZE(uda1380_dai));
 	snd_soc_unregister_codec(&uda1380->codec);
 
-	gpio_set_value(pdata->gpio_power, 0);
-	gpio_free(pdata->gpio_reset);
-	gpio_free(pdata->gpio_power);
+	pdata->set_power(0);
 
 	kfree(uda1380);
 	uda1380_codec = NULL;
-- 
1.7.1

Re: [PATCH RFC 1/3] ASoC: uda1380: use callbacks instead of gpiolib

[Mark Brown]

On 26 Jun 2010, at 16:14, Vasily Khoruzhick <anarsoul@gmail.com> wrote:

> Some machines require some tricks to enable/disable
> codec, i.e. disable or enable i2s clock before enabling/disabling
> codec, and just configuring gpio is not enough; some machines
> have no reset pin (reset is performed on codec power on automatically).
> Fix that issue by using machine-specific callback to enable codec power.
> 
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>

This is fine but it'd be really nice to preserve the use of GPIOs since
that will cover the majority of machines - for example, by providing a
default callback if none is provided and GPIOs are. This will also
avoid the need to update existing machine drivers (which needs to be
done otherwise).

However, I do wonder if the more complex set_power() callbacks might
not just end up as regulator API consumers? 

> ---
> include/sound/uda1380.h    |    3 +--
> sound/soc/codecs/uda1380.c |   25 +++---------------------
> 2 files changed, 4 insertions(+), 24 deletions(-)
> 
> diff --git a/include/sound/uda1380.h b/include/sound/uda1380.h
> index 381319c..d2171dc 100644
> --- a/include/sound/uda1380.h
> +++ b/include/sound/uda1380.h
> @@ -12,8 +12,7 @@
> #define __UDA1380_H
> 
> struct uda1380_platform_data {
> -	int gpio_power;
> -	int gpio_reset;
> +	void (*set_power)(int);
>  	int dac_clk;
> #define UDA1380_DAC_CLK_SYSCLK 0
> #define UDA1380_DAC_CLK_WSPLL  1
> diff --git a/sound/soc/codecs/uda1380.c b/sound/soc/codecs/uda1380.c
> index 2f925a2..2b7f200 100644
> --- a/sound/soc/codecs/uda1380.c
> +++ b/sound/soc/codecs/uda1380.c
> @@ -19,7 +19,6 @@
> #include <linux/types.h>
> #include <linux/slab.h>
> #include <linux/errno.h>
> -#include <linux/gpio.h>
> #include <linux/delay.h>
> #include <linux/i2c.h>
> #include <linux/workqueue.h>
> @@ -752,22 +751,11 @@ static int uda1380_register(struct uda1380_priv *uda1380)
>  		return -EINVAL;
>  	}
> 
> -	if (!pdata || !pdata->gpio_power || !pdata->gpio_reset)
> +	if (!pdata || !pdata->set_power)
>  		return -EINVAL;
> 
> -	ret = gpio_request(pdata->gpio_power, "uda1380 power");
> -	if (ret)
> -		goto err_out;
> -	ret = gpio_request(pdata->gpio_reset, "uda1380 reset");
> -	if (ret)
> -		goto err_gpio;
> -
> -	gpio_direction_output(pdata->gpio_power, 1);
> -
>  	/* we may need to have the clock running here - pH5 */
> -	gpio_direction_output(pdata->gpio_reset, 1);
> -	udelay(5);
> -	gpio_set_value(pdata->gpio_reset, 0);
> +	pdata->set_power(1);
> 
>  	mutex_init(&codec->mutex);
>  	INIT_LIST_HEAD(&codec->dapm_widgets);
> @@ -818,11 +806,6 @@ static int uda1380_register(struct uda1380_priv *uda1380)
> err_dai:
>  	snd_soc_unregister_codec(codec);
> err_reset:
> -	gpio_set_value(pdata->gpio_power, 0);
> -	gpio_free(pdata->gpio_reset);
> -err_gpio:
> -	gpio_free(pdata->gpio_power);
> -err_out:
>  	return ret;
> }
> 
> @@ -834,9 +817,7 @@ static void uda1380_unregister(struct uda1380_priv *uda1380)
>  	snd_soc_unregister_dais(uda1380_dai, ARRAY_SIZE(uda1380_dai));
>  	snd_soc_unregister_codec(&uda1380->codec);
> 
> -	gpio_set_value(pdata->gpio_power, 0);
> -	gpio_free(pdata->gpio_reset);
> -	gpio_free(pdata->gpio_power);
> +	pdata->set_power(0);
> 
>  	kfree(uda1380);
>  	uda1380_codec = NULL;
> -- 
> 1.7.1
> 

Re: [PATCH RFC 1/3] ASoC: uda1380: use callbacks instead of gpiolib

[Vasily Khoruzhick]

[-- Attachment #1.1: Type: Text/Plain, Size: 1718 bytes --]

В сообщении от 26 июня 2010 19:40:37 автор Mark Brown написал:
> On 26 Jun 2010, at 16:14, Vasily Khoruzhick <anarsoul@gmail.com> wrote:
> > Some machines require some tricks to enable/disable
> > codec, i.e. disable or enable i2s clock before enabling/disabling
> > codec, and just configuring gpio is not enough; some machines
> > have no reset pin (reset is performed on codec power on automatically).
> > Fix that issue by using machine-specific callback to enable codec power.
> > 
> > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> 
> This is fine but it'd be really nice to preserve the use of GPIOs since
> that will cover the majority of machines - for example, by providing a
> default callback if none is provided and GPIOs are. This will also
> avoid the need to update existing machine drivers (which needs to be
> done otherwise).

The only machine that uses uda1380 and supported by mainline kernel is 
magician, rx1950 and h1940 sound support is not merged yet, so that's not a 
problem to perform that change.
 
> However, I do wonder if the more complex set_power() callbacks might
> not just end up as regulator API consumers?

Is it really necessary? Plain callback perfectly fits here, and same approach 
is used for s3cmci driver. For example, rx1950_uda1380_set_power is not 
complex and looks like this:

static void rx1950_uda1380_set_power(int enable)
{
	clk_disable(i2c_clk);
	gpio_direction_output(S3C2410_GPD(0), 0);
	gpio_direction_output(S3C2410_GPJ(0), enable);
	if (enable) {
		gpio_set_value(S3C2410_GPD(0), 1);
		mdelay(1);
		gpio_set_value(S3C2410_GPD(0), 0);
	}
	clk_enable(i2c_clk);
}

Regards
Vasily

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH RFC 1/3] ASoC: uda1380: use callbacks instead of gpiolib

[Mark Brown]

On 26 Jun 2010, at 17:53, Vasily Khoruzhick wrote:

> В сообщении от 26 июня 2010 19:40:37 автор Mark Brown написал:

>> This is fine but it'd be really nice to preserve the use of GPIOs since
>> that will cover the majority of machines - for example, by providing a
>> default callback if none is provided and GPIOs are. This will also
>> avoid the need to update existing machine drivers (which needs to be
>> done otherwise).

> The only machine that uses uda1380 and supported by mainline kernel is 
> magician, rx1950 and h1940 sound support is not merged yet, so that's not a 
> problem to perform that change.

It's not just the hassle, it's also the fact that all these machine drivers will end
up duplicating the code. This feels like a step back for machines other than
yours.

>> However, I do wonder if the more complex set_power() callbacks might
>> not just end up as regulator API consumers?

> Is it really necessary? Plain callback perfectly fits here, and same approach 
> is used for s3cmci driver. For example, rx1950_uda1380_set_power is not 
> complex and looks like this:

The regulator API isn't massively complex either and it does provide a standard
abstraction for controlling power which seems like it might be useful here,
though given the (frankly rather odd) fiddling with clocks your callback does
perhaps it won't help.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH RFC 1/3] ASoC: uda1380: use callbacks instead of gpiolib

[Vasily Khoruzhick]

[-- Attachment #1.1: Type: Text/Plain, Size: 789 bytes --]

В сообщении от 26 июня 2010 23:09:39 автор Mark Brown написал:

> The regulator API isn't massively complex either and it does provide a
> standard abstraction for controlling power which seems like it might be
> useful here, though given the (frankly rather odd) fiddling with clocks
> your callback does perhaps it won't help.

Ok, but I still don't like idea about keeping both regulator and gpio stuff. 
Btw, this weird clock stuff can be merged to i2c driver, i.e. keep i2c module 
clock enabled only on i2c transfer (it's not scl line, it's module clock) 
(I've sent patch for i2c-s3c2410 ~month ago, can resend it), and we can avoid 
modyfing power/reset controlling part of uda1380 driver. What do you think 
about that?

Regards
Vasily

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH RFC 1/3] ASoC: uda1380: use callbacks instead of gpiolib

[Mark Brown]

On 26 Jun 2010, at 21:53, Vasily Khoruzhick wrote:

> В сообщении от 26 июня 2010 23:09:39 автор Mark Brown написал:
> 
>> The regulator API isn't massively complex either and it does provide a
>> standard abstraction for controlling power which seems like it might be
>> useful here, though given the (frankly rather odd) fiddling with clocks
>> your callback does perhaps it won't help.
> 
> Ok, but I still don't like idea about keeping both regulator and gpio stuff. 

If you're going to the regulator API it's probably OK to remove the GPIO stuff
since the regulator API supports GPIO controlled regulators already. Does
mean transitioning the existing drivers but it means everyone shares the same
code.

> Btw, this weird clock stuff can be merged to i2c driver, i.e. keep i2c module 
> clock enabled only on i2c transfer (it's not scl line, it's module clock) 
> (I've sent patch for i2c-s3c2410 ~month ago, can resend it), and we can avoid 
> modyfing power/reset controlling part of uda1380 driver. What do you think 
> about that?

You definitely should be doing this in the I2C driver not the CODEC driver,
you're peering inside the internals of the I2C driver as things stand and are
likely to cause problems if someone comes along and implements this in the
driver. There's also the fact that all users of the I2C controller will benefit if
the code is implemented there, not just this one audio driver.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH RFC 1/3] ASoC: uda1380: use callbacks instead of gpiolib

[Vasily Khoruzhick]

[-- Attachment #1.1: Type: Text/Plain, Size: 649 bytes --]

В сообщении от 26 июня 2010 23:57:42 автор Mark Brown написал:

> You definitely should be doing this in the I2C driver not the CODEC driver,
> you're peering inside the internals of the I2C driver as things stand and
> are likely to cause problems if someone comes along and implements this in
> the driver. There's also the fact that all users of the I2C controller
> will benefit if the code is implemented there, not just this one audio
> driver.

I'm afraid that it can cause problems with other i2c-controlled devices, and I 
can test i2c modification only on h1940 and rx1950 with same codec (uda1380).

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH RFC 1/3] ASoC: uda1380: use callbacks instead of gpiolib

[Mark Brown]

On Sun, Jun 27, 2010 at 12:12:05AM +0300, Vasily Khoruzhick wrote:
> В сообщении от 26 июня 2010 23:57:42 автор Mark Brown написал:

> > You definitely should be doing this in the I2C driver not the CODEC driver,
> > you're peering inside the internals of the I2C driver as things stand and
> > are likely to cause problems if someone comes along and implements this in
> > the driver. There's also the fact that all users of the I2C controller
> > will benefit if the code is implemented there, not just this one audio
> > driver.

> I'm afraid that it can cause problems with other i2c-controlled devices, and I 
That's still not dealing with the problems that will arise if someone
does implement this in the core.  To be honest I'm having a hard time
seeing why devices would care - the I2C controller only clocks the bus
during a transfer and otherwise produces no output - so it should just
be an issue for the controller IP.

> can test i2c modification only on h1940 and rx1950 with same codec (uda1380).

Nobody is ever going to be able to test on all possible hardware,
testing is a community effort.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH RFC 1/3] ASoC: uda1380: use callbacks instead of gpiolib

[Vasily Khoruzhick]

[-- Attachment #1.1: Type: Text/Plain, Size: 1371 bytes --]

В сообщении от 26 июня 2010 23:57:42 автор Mark Brown написал:
> On 26 Jun 2010, at 21:53, Vasily Khoruzhick wrote:
> > В сообщении от 26 июня 2010 23:09:39 автор Mark Brown написал:
> >> The regulator API isn't massively complex either and it does provide a
> >> standard abstraction for controlling power which seems like it might be
> >> useful here, though given the (frankly rather odd) fiddling with clocks
> >> your callback does perhaps it won't help.
> > 
> > Ok, but I still don't like idea about keeping both regulator and gpio
> > stuff.
> 
> If you're going to the regulator API it's probably OK to remove the GPIO
> stuff since the regulator API supports GPIO controlled regulators already.
> Does mean transitioning the existing drivers but it means everyone shares
> the same code.

Actually, power on sequence on rx1950 (and on magician) looks like following:

1. set power pin to 1
2. set reset pin to 1
3. wait a bit (1ms or so)
4. set reset pin to 0

Reset right after power on is necessary at least on rx1950 (otherwise codec 
doesn't respond on i2c).

Fixed regulator doesn't support that logic now, and I don't want to mix 
gpio/regulator stuff in uda1380 driver. Does it makes sense to implement
another fixed-like regulator just for uda1380?

Regards
Vasily

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH RFC 1/3] ASoC: uda1380: use callbacks instead of gpiolib

[Mark Brown]

On Mon, Jun 28, 2010 at 03:00:13PM +0300, Vasily Khoruzhick wrote:

> Actually, power on sequence on rx1950 (and on magician) looks like following:

> 1. set power pin to 1

Is this a feature of the chip or an external power control?

Re: [PATCH RFC 1/3] ASoC: uda1380: use callbacks instead of gpiolib

[Vasily Khoruzhick]

[-- Attachment #1.1: Type: Text/Plain, Size: 442 bytes --]

В сообщении от 28 июня 2010 16:41:45 автор Mark Brown написал:
> On Mon, Jun 28, 2010 at 03:00:13PM +0300, Vasily Khoruzhick wrote:
> > Actually, power on sequence on rx1950 (and on magician) looks like
> > following:
> > 
> > 1. set power pin to 1
> 
> Is this a feature of the chip or an external power control?

External power control, power and reset pins are routed to s3c24xx gpio pins 
on rx1950

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH RFC 1/3] ASoC: uda1380: use callbacks instead of gpiolib

[Mark Brown]

On Mon, Jun 28, 2010 at 04:49:33PM +0300, Vasily Khoruzhick wrote:
> В сообщении от 28 июня 2010 16:41:45 автор Mark Brown написал:

> > Is this a feature of the chip or an external power control?

> External power control, power and reset pins are routed to s3c24xx gpio pins 
> on rx1950

So this is an on-board power source that's being controlled by the GPIO
rather than an external device?
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH RFC 1/3] ASoC: uda1380: use callbacks instead of gpiolib

[Vasily Khoruzhick]

[-- Attachment #1.1: Type: Text/Plain, Size: 360 bytes --]

В сообщении от 28 июня 2010 16:50:46 автор Mark Brown написал:
> So this is an on-board power source that's being controlled by the GPIO
> rather than an external device?

That's just gpio-based power control for uda1380 (not sure about schematics, 
it's not available). Same approach is used in h1940 and magician.

Regards
Vasily

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH RFC 1/3] ASoC: uda1380: use callbacks instead of gpiolib

[Mark Brown]

On Mon, Jun 28, 2010 at 05:05:51PM +0300, Vasily Khoruzhick wrote:
> В сообщении от 28 июня 2010 16:50:46 автор Mark Brown написал:
> > So this is an on-board power source that's being controlled by the GPIO
> > rather than an external device?

> That's just gpio-based power control for uda1380 (not sure about schematics, 
> it's not available). Same approach is used in h1940 and magician.

What I'm trying to figure out is if the GPIO is wired directly to the
UDA1380 or if this is controlling something external to the UDA1380
which these designs just happen to have copied.  If it's a direct
connection to the UDA1380 then using GPIOs directly is perfectly
sensible.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH RFC 1/3] ASoC: uda1380: use callbacks instead of gpiolib

[Vasily Khoruzhick]

[-- Attachment #1.1: Type: Text/Plain, Size: 807 bytes --]

В сообщении от 28 июня 2010 17:15:40 автор Mark Brown написал:

> What I'm trying to figure out is if the GPIO is wired directly to the
> UDA1380 or if this is controlling something external to the UDA1380
> which these designs just happen to have copied.  If it's a direct
> connection to the UDA1380 then using GPIOs directly is perfectly
> sensible.

I'm not sure if s3c24xx GPIOs can drive UDA1380 directly. I suspect it's wired 
to the gate of some transistor (FET - is it right term to use here? :)) to 
handle higher current (sorry, English is not my native language, and it's hard 
to explain schematic-specific things, that's not my field :)). Btw, I'm sure 
that there's no any external power-control chip, maybe just single transistor.

Regards
Vasily

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH RFC 1/3] ASoC: uda1380: use callbacks instead of gpiolib

[Mark Brown]

On Mon, Jun 28, 2010 at 04:20:43PM +0200, pHilipp Zabel wrote:

> The reset GPIO is connected directly to the UDA1380 RESET pin. The
> power source is external.

OK, the power source is definitely a regulator then.

[PATCH RFC 2/3] magician: pass .set_power callback to uda1380 pdata

[Vasily Khoruzhick]

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 sound/soc/pxa/magician.c |   25 +++++++++++++++++++++++--
 1 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/sound/soc/pxa/magician.c b/sound/soc/pxa/magician.c
index 4c8d99a..f2ad1b6 100644
--- a/sound/soc/pxa/magician.c
+++ b/sound/soc/pxa/magician.c
@@ -457,12 +457,21 @@ static struct snd_soc_device magician_snd_devdata = {
 
 static struct platform_device *magician_snd_device;
 
+static void set_power(int enable)
+{
+	gpio_direction_output(EGPIO_MAGICIAN_CODEC_POWER, enable);
+	if (enable) {
+		gpio_direction_output(EGPIO_MAGICIAN_CODEC_RESET, 1);
+		udelay(5);
+		gpio_set_value(EGPIO_MAGICIAN_CODEC_RESET, 0);
+	}
+}
+
 /*
  * FIXME: move into magician board file once merged into the pxa tree
  */
 static struct uda1380_platform_data uda1380_info = {
-	.gpio_power = EGPIO_MAGICIAN_CODEC_POWER,
-	.gpio_reset = EGPIO_MAGICIAN_CODEC_RESET,
+	.set_power = set_power,
 	.dac_clk    = UDA1380_DAC_CLK_WSPLL,
 };
 
@@ -490,6 +499,12 @@ static int __init magician_init(void)
 	if (!client)
 		return -ENODEV;
 
+	ret = gpio_request(EGPIO_MAGICIAN_CODEC_POWER, "CODEC_POWER");
+	if (ret)
+		goto err_request_codec_pwr;
+	ret = gpio_request(EGPIO_MAGICIAN_CODEC_RESET, "CODEC_RESET");
+	if (ret)
+		goto err_request_codec_reset;
 	ret = gpio_request(EGPIO_MAGICIAN_SPK_POWER, "SPK_POWER");
 	if (ret)
 		goto err_request_spk;
@@ -535,6 +550,10 @@ err_request_mic:
 err_request_ep:
 	gpio_free(EGPIO_MAGICIAN_SPK_POWER);
 err_request_spk:
+	gpio_free(EGPIO_MAGICIAN_CODEC_RESET);
+err_request_codec_reset:
+	gpio_free(EGPIO_MAGICIAN_CODEC_POWER);
+err_request_codec_pwr:
 	return ret;
 }
 
@@ -551,6 +570,8 @@ static void __exit magician_exit(void)
 	gpio_free(EGPIO_MAGICIAN_MIC_POWER);
 	gpio_free(EGPIO_MAGICIAN_EP_POWER);
 	gpio_free(EGPIO_MAGICIAN_SPK_POWER);
+	gpio_free(EGPIO_MAGICIAN_CODEC_RESET);
+	gpio_free(EGPIO_MAGICIAN_CODEC_POWER);
 }
 
 module_init(magician_init);
-- 
1.7.1

[PATCH RFC 3/3] uda1380: make driver more powersave-friendly

[Vasily Khoruzhick]

Disable some codec modules in standby mode, completely disable
codec in off mode to save some power.
Fix suspend/resume: mark mixer regs as dirty on resume to
restore mixer values, otherwise driver produces no sound
(master is muted by default).

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 sound/soc/codecs/uda1380.c |   92 ++++++++++++++++++++++++++++---------------
 1 files changed, 60 insertions(+), 32 deletions(-)

diff --git a/sound/soc/codecs/uda1380.c b/sound/soc/codecs/uda1380.c
index 2b7f200..a30f809 100644
--- a/sound/soc/codecs/uda1380.c
+++ b/sound/soc/codecs/uda1380.c
@@ -130,7 +130,41 @@ static int uda1380_write(struct snd_soc_codec *codec, unsigned int reg,
 		return -EIO;
 }
 
-#define uda1380_reset(c)	uda1380_write(c, UDA1380_RESET, 0)
+static void uda1380_sync_cache(struct snd_soc_codec *codec)
+{
+	int reg;
+	u8 data[3];
+	u16 *cache = codec->reg_cache;
+
+	/* Sync reg_cache with the hardware */
+	for (reg = 0; reg < UDA1380_MVOL; reg++) {
+		data[0] = reg;
+		data[1] = (cache[reg] & 0xff00) >> 8;
+		data[2] = cache[reg] & 0x00ff;
+		if (codec->hw_write(codec->control_data, data, 3) != 3)
+			dev_err(codec->dev, "%s: write to reg 0x%x failed\n",
+				__func__, reg);
+	}
+
+	for (reg = UDA1380_MVOL; reg < UDA1380_CACHEREGNUM; reg++)
+		set_bit(reg - 0x10, &uda1380_cache_dirty);
+}
+
+static int uda1380_reset(struct snd_soc_codec *codec)
+{
+	u8 data[3];
+
+	data[0] = UDA1380_RESET;
+	data[1] = 0;
+	data[2] = 0;
+
+	if (codec->hw_write(codec->control_data, data, 3) != 3) {
+		dev_err(codec->dev, "%s: failed\n", __func__);
+		return -EIO;
+	}
+
+	return 0;
+}
 
 static void uda1380_flush_work(struct work_struct *work)
 {
@@ -481,13 +515,13 @@ static int uda1380_trigger(struct snd_pcm_substream *substream, int cmd,
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		uda1380_write_reg_cache(codec, UDA1380_MIXER,
+		uda1380_write(codec, UDA1380_MIXER,
 					mixer & ~R14_SILENCE);
 		schedule_work(&uda1380->work);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
-		uda1380_write_reg_cache(codec, UDA1380_MIXER,
+		uda1380_write(codec, UDA1380_MIXER,
 					mixer | R14_SILENCE);
 		schedule_work(&uda1380->work);
 		break;
@@ -561,17 +595,38 @@ static int uda1380_set_bias_level(struct snd_soc_codec *codec,
 	enum snd_soc_bias_level level)
 {
 	int pm = uda1380_read_reg_cache(codec, UDA1380_PM);
+	struct uda1380_platform_data *pdata = codec->dev->platform_data;
+
+	if (codec->bias_level == level)
+		return 0;
 
 	switch (level) {
 	case SND_SOC_BIAS_ON:
 	case SND_SOC_BIAS_PREPARE:
+		/* ADC, DAC on */
 		uda1380_write(codec, UDA1380_PM, R02_PON_BIAS | pm);
 		break;
 	case SND_SOC_BIAS_STANDBY:
-		uda1380_write(codec, UDA1380_PM, R02_PON_BIAS);
+		if (codec->bias_level == SND_SOC_BIAS_OFF) {
+			pdata->set_power(1);
+			uda1380_reset(codec);
+
+			switch (pdata->dac_clk) {
+			case UDA1380_DAC_CLK_SYSCLK:
+				uda1380_write_reg_cache(codec, UDA1380_CLK, 0);
+				break;
+			case UDA1380_DAC_CLK_WSPLL:
+				uda1380_write_reg_cache(codec, UDA1380_CLK,
+					R00_DAC_CLK);
+				break;
+			}
+
+			uda1380_sync_cache(codec);
+		}
+		uda1380_write(codec, UDA1380_PM, 0x0);
 		break;
 	case SND_SOC_BIAS_OFF:
-		uda1380_write(codec, UDA1380_PM, 0x0);
+		pdata->set_power(0);
 		break;
 	}
 	codec->bias_level = level;
@@ -658,16 +713,7 @@ static int uda1380_resume(struct platform_device *pdev)
 {
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
 	struct snd_soc_codec *codec = socdev->card->codec;
-	int i;
-	u8 data[2];
-	u16 *cache = codec->reg_cache;
 
-	/* Sync reg_cache with the hardware */
-	for (i = 0; i < ARRAY_SIZE(uda1380_reg); i++) {
-		data[0] = (i << 1) | ((cache[i] >> 8) & 0x0001);
-		data[1] = cache[i] & 0x00ff;
-		codec->hw_write(codec->control_data, data, 2);
-	}
 	uda1380_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
 	return 0;
 }
@@ -697,15 +743,6 @@ static int uda1380_probe(struct platform_device *pdev)
 
 	/* power on device */
 	uda1380_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
-	/* set clock input */
-	switch (pdata->dac_clk) {
-	case UDA1380_DAC_CLK_SYSCLK:
-		uda1380_write(codec, UDA1380_CLK, 0);
-		break;
-	case UDA1380_DAC_CLK_WSPLL:
-		uda1380_write(codec, UDA1380_CLK, R00_DAC_CLK);
-		break;
-	}
 
 	snd_soc_add_controls(codec, uda1380_snd_controls,
 				ARRAY_SIZE(uda1380_snd_controls));
@@ -754,9 +791,6 @@ static int uda1380_register(struct uda1380_priv *uda1380)
 	if (!pdata || !pdata->set_power)
 		return -EINVAL;
 
-	/* we may need to have the clock running here - pH5 */
-	pdata->set_power(1);
-
 	mutex_init(&codec->mutex);
 	INIT_LIST_HEAD(&codec->dapm_widgets);
 	INIT_LIST_HEAD(&codec->dapm_paths);
@@ -776,12 +810,6 @@ static int uda1380_register(struct uda1380_priv *uda1380)
 
 	memcpy(codec->reg_cache, uda1380_reg, sizeof(uda1380_reg));
 
-	ret = uda1380_reset(codec);
-	if (ret < 0) {
-		dev_err(codec->dev, "Failed to issue reset\n");
-		goto err_reset;
-	}
-
 	INIT_WORK(&uda1380->work, uda1380_flush_work);
 
 	for (i = 0; i < ARRAY_SIZE(uda1380_dai); i++)
-- 
1.7.1

Re: [PATCH RFC 3/3] uda1380: make driver more powersave-friendly

[Mark Brown]

On 26 Jun 2010, at 16:14, Vasily Khoruzhick wrote:

> Disable some codec modules in standby mode, completely disable
> codec in off mode to save some power.
> Fix suspend/resume: mark mixer regs as dirty on resume to
> restore mixer values, otherwise driver produces no sound
> (master is muted by default).

Please remember to CC Liam on ASoC patches.

> +	for (reg = UDA1380_MVOL; reg < UDA1380_CACHEREGNUM; reg++)
> +		set_bit(reg - 0x10, &uda1380_cache_dirty);

This seems odd, I'd expect the cache to be being marked as clean immediately
after sync?

> @@ -561,17 +595,38 @@ static int uda1380_set_bias_level(struct snd_soc_codec *codec,
> 	enum snd_soc_bias_level level)
> {
> 	int pm = uda1380_read_reg_cache(codec, UDA1380_PM);
> +	struct uda1380_platform_data *pdata = codec->dev->platform_data;
> +
> +	if (codec->bias_level == level)
> +		return 0;
> 
> 	switch (level) {
> 	case SND_SOC_BIAS_ON:
> 	case SND_SOC_BIAS_PREPARE:
> +		/* ADC, DAC on */
> 		uda1380_write(codec, UDA1380_PM, R02_PON_BIAS | pm);

Like I said previously you really should look at using DAPM here, this should
make the code simpler and will let you 

You might also want to consider snd_soc_update_bits().

> 		break;
> 	case SND_SOC_BIAS_STANDBY:
> -		uda1380_write(codec, UDA1380_PM, R02_PON_BIAS);
> +		if (codec->bias_level == SND_SOC_BIAS_OFF) {
> +			pdata->set_power(1);
> +			uda1380_reset(codec);

The reset here seems unneeded and possibly wasteful if there is no power
control on the system. It'd seem better to do something like just power up,
flagging the cache as dirty if there was a callback. I'd strongly expect that
if you are actually controlling power the device will be in the default state
anyway.

> +			switch (pdata->dac_clk) {
> +			case UDA1380_DAC_CLK_SYSCLK:
> +				uda1380_write_reg_cache(codec, UDA1380_CLK, 0);
> +				break;
> +			case UDA1380_DAC_CLK_WSPLL:
> +				uda1380_write_reg_cache(codec, UDA1380_CLK,
> +					R00_DAC_CLK);
> +				break;
> +			}

Why is this being managed every time the device is enabled? Surely the
setting can be done once at startup.

> -	ret = uda1380_reset(codec);
> -	if (ret < 0) {
> -		dev_err(codec->dev, "Failed to issue reset\n");
> -		goto err_reset;
> -	}
> -

The reason for the reset at startup is that we don't know what state the device
is in when Linux gets control.

Re: [PATCH RFC 3/3] uda1380: make driver more powersave-friendly

[Vasily Khoruzhick]

[-- Attachment #1.1: Type: Text/Plain, Size: 2502 bytes --]

В сообщении от 26 июня 2010 23:45:12 автор Mark Brown написал:
> Please remember to CC Liam on ASoC patches.

Ok, I'll do
> 
> > +	for (reg = UDA1380_MVOL; reg < UDA1380_CACHEREGNUM; reg++)
> > +		set_bit(reg - 0x10, &uda1380_cache_dirty);
> 
> This seems odd, I'd expect the cache to be being marked as clean
> immediately after sync?

Nope, it is not. Only i2c and clock related regs of uda1380 can be modified 
when there's no i2s clock, i.e. mixer regs should be updated right after i2s 
clock was enabled, so we marking mixer-related regs cache as dirty, to make 
sure they'll be updated when possible.
 
> > 		uda1380_write(codec, UDA1380_PM, R02_PON_BIAS | pm);
> 
> Like I said previously you really should look at using DAPM here, this
> should make the code simpler and will let you
> 
> You might also want to consider snd_soc_update_bits().

Hmm, uda134x driver does pretty same things as in my patch... And it seems 
part of your sentence is lost :(
 
> > 		break;
> > 	
> > 	case SND_SOC_BIAS_STANDBY:
> > -		uda1380_write(codec, UDA1380_PM, R02_PON_BIAS);
> > +		if (codec->bias_level == SND_SOC_BIAS_OFF) {
> > +			pdata->set_power(1);
> > +			uda1380_reset(codec);
> 
> The reset here seems unneeded and possibly wasteful if there is no power
> control on the system. It'd seem better to do something like just power up,
> flagging the cache as dirty if there was a callback. I'd strongly expect
> that if you are actually controlling power the device will be in the
> default state anyway.

Ok

> > +			switch (pdata->dac_clk) {
> > +			case UDA1380_DAC_CLK_SYSCLK:
> > +				uda1380_write_reg_cache(codec, UDA1380_CLK, 0);
> > +				break;
> > +			case UDA1380_DAC_CLK_WSPLL:
> > +				uda1380_write_reg_cache(codec, UDA1380_CLK,
> > +					R00_DAC_CLK);
> > +				break;
> > +			}
> 
> Why is this being managed every time the device is enabled? Surely the
> setting can be done once at startup.

Yep, just need to mark cache of this reg as dirty here...
 
> > -	ret = uda1380_reset(codec);
> > -	if (ret < 0) {
> > -		dev_err(codec->dev, "Failed to issue reset\n");
> > -		goto err_reset;
> > -	}
> > -
> 
> The reason for the reset at startup is that we don't know what state the
> device is in when Linux gets control.

It's softreset and it's performed by writing some value to some reg. i2c xfers 
is not possible when codec is not enabled (it is not at this point)

Regards
Vasily

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH RFC 3/3] uda1380: make driver more powersave-friendly

[Mark Brown]

On Sun, Jun 27, 2010 at 12:07:26AM +0300, Vasily Khoruzhick wrote:
> В сообщении от 26 июня 2010 23:45:12 автор Mark Brown написал:

> > This seems odd, I'd expect the cache to be being marked as clean
> > immediately after sync?

> Nope, it is not. Only i2c and clock related regs of uda1380 can be modified 
> when there's no i2s clock, i.e. mixer regs should be updated right after i2s 
> clock was enabled, so we marking mixer-related regs cache as dirty, to make 
> sure they'll be updated when possible.

This is very much non-obvious from your code - it really needs comments
explaining why the cache sync function didn't actually manage to sync
the cache.  I'd also really expect that the dirtying of the cache would
be done when the operation that invalidates it happens, not later on
when restoring the cache.  Otherwise there's a window where the cache is
flagged as valid but is not actually so which doesn't seem robust.

> > > 		uda1380_write(codec, UDA1380_PM, R02_PON_BIAS | pm);

> > Like I said previously you really should look at using DAPM here, this
> > should make the code simpler and will let you

> Hmm, uda134x driver does pretty same things as in my patch... And it seems 
> part of your sentence is lost :(

Old drivers for fairly obscure chips aren't always a good guide to best
practices for things.

> > > -	ret = uda1380_reset(codec);
> > > -	if (ret < 0) {
> > > -		dev_err(codec->dev, "Failed to issue reset\n");
> > > -		goto err_reset;
> > > -	}

> > The reason for the reset at startup is that we don't know what state the
> > device is in when Linux gets control.

> It's softreset and it's performed by writing some value to some reg. i2c xfers 
> is not possible when codec is not enabled (it is not at this point)

It needs to happen at some point.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH RFC 3/3] uda1380: make driver more powersave-friendly

[Vasily Khoruzhick]

[-- Attachment #1.1: Type: Text/Plain, Size: 1436 bytes --]

В сообщении от 27 июня 2010 13:10:54 автор Mark Brown написал:
> > Nope, it is not. Only i2c and clock related regs of uda1380 can be
> > modified when there's no i2s clock, i.e. mixer regs should be updated
> > right after i2s clock was enabled, so we marking mixer-related regs
> > cache as dirty, to make sure they'll be updated when possible.
> 
> This is very much non-obvious from your code - it really needs comments
> explaining why the cache sync function didn't actually manage to sync
> the cache.  I'd also really expect that the dirtying of the cache would
> be done when the operation that invalidates it happens, not later on
> when restoring the cache.  Otherwise there's a window where the cache is
> flagged as valid but is not actually so which doesn't seem robust.

Ok I'll add comments, but there's no window where cache is marked as valid but 
is no consistent with hardware - resume callback marks cache as dirty 
immediately :)

> Old drivers for fairly obscure chips aren't always a good guide to best
> practices for things.

Ok, what driver should I use as reference?

> > It's softreset and it's performed by writing some value to some reg. i2c
> > xfers is not possible when codec is not enabled (it is not at this
> > point)
> 
> It needs to happen at some point.

Softreset is performed right after codec power on, it can't be performed 
earlier.

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH RFC 3/3] uda1380: make driver more powersave-friendly

[Mark Brown]

On 27 Jun 2010, at 11:43, Vasily Khoruzhick wrote:

> В сообщении от 27 июня 2010 13:10:54 автор Mark Brown написал:
>> Old drivers for fairly obscure chips aren't always a good guide to best
>> practices for things.

> Ok, what driver should I use as reference?

Off the top of my head WM8904 does regulator based power management.

>>> It's softreset and it's performed by writing some value to some reg. i2c
>>> xfers is not possible when codec is not enabled (it is not at this
>>> point)
>> 
>> It needs to happen at some point.
> 
> Softreset is performed right after codec power on, it can't be performed 
> earlier.

After power on it's not needed, the device should be in the default state
already. It's only required on startup when there's no power control for the
device.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH RFC 3/3] uda1380: make driver more powersave-friendly

[Vasily Khoruzhick]

[-- Attachment #1.1: Type: Text/Plain, Size: 324 bytes --]

В сообщении от 27 июня 2010 23:55:29 автор Mark Brown написал:

> After power on it's not needed, the device should be in the default state
> already. It's only required on startup when there's no power control for
> the device.

But that's not possible to reset codec if it is not powered on :(

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH RFC 3/3] uda1380: make driver more powersave-friendly

[Mark Brown]

On 27 Jun 2010, at 22:15, Vasily Khoruzhick <anarsoul@gmail.com> wrote:

> В сообщении от 27 июня 2010 23:55:29 автор Mark Brown написал:
> 
>> After power on it's not needed, the device should be in the default state
>> already. It's only required on startup when there's no power control for
>> the device.
> 
> But that's not possible to reset codec if it is not powered on :(

So don't reset it if you have control of the power? Remember, not all
systems will have soft control of the power for the device. 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel