[PATCH 0/4] ALSA: pxa27x: ac97 reset fixes

[PATCH 0/4] ALSA: pxa27x: ac97 reset fixes

[Mike Dunn]

AC97 reset is broken on the pxa27x.  These patches fix it by addressing multiple
problems:
  - cold reset fails because the GCR register is not set correctly
  - warm reset fails because some code used to work around a hardware bug is
    broken
  - the intent of the work-around code is obfuscated by unfortunate function
    naming

This patch set replaces some earlier individual patches that had some problems.
Thanks Robert and Igor.

Mike Dunn (4):
  ALSA: pxa27x: fix ac97 cold reset
  ALSA: pxa27x: fix ac97 warm reset bug work-around code
  ALSA: pxa27x: ac97 controller driver requests gpio
  ALSA: pxa27x: rename pxa27x_assert_ac97()

 arch/arm/mach-pxa/include/mach/mfp-pxa27x.h |    3 ++
 arch/arm/mach-pxa/pxa27x.c                  |   38 +++++++++++++++++++-------
 sound/arm/pxa2xx-ac97-lib.c                 |   26 +++++++++++++-----
 3 files changed, 50 insertions(+), 17 deletions(-)

-- 
1.7.8.6

[PATCH 0/4] ALSA: pxa27x: ac97 reset fixes

[Mike Dunn]

AC97 reset is broken on the pxa27x.  These patches fix it by addressing multiple
problems:
  - cold reset fails because the GCR register is not set correctly
  - warm reset fails because some code used to work around a hardware bug is
    broken
  - the intent of the work-around code is obfuscated by unfortunate function
    naming

This patch set replaces some earlier individual patches that had some problems.
Thanks Robert and Igor.

Mike Dunn (4):
  ALSA: pxa27x: fix ac97 cold reset
  ALSA: pxa27x: fix ac97 warm reset bug work-around code
  ALSA: pxa27x: ac97 controller driver requests gpio
  ALSA: pxa27x: rename pxa27x_assert_ac97()

 arch/arm/mach-pxa/include/mach/mfp-pxa27x.h |    3 ++
 arch/arm/mach-pxa/pxa27x.c                  |   38 +++++++++++++++++++-------
 sound/arm/pxa2xx-ac97-lib.c                 |   26 +++++++++++++-----
 3 files changed, 50 insertions(+), 17 deletions(-)

-- 
1.7.8.6

[PATCH 1/4] ALSA: pxa2xx: fix ac97 cold reset

[Mike Dunn]

Cold reset on the pxa27x currently fails and

     pxa2xx_ac97_try_cold_reset: cold reset timeout (GSR=0x44)

appears in the kernel log.  Through trial-and-error (the pxa270 developer's
manual is mostly incoherent on the topic of ac97 reset), I got cold reset to
complete by setting the WARM_RST bit in the GCR register (and later noticed that
pxa3xx does this for cold reset as well).  Also, a timeout loop is needed to
wait for the reset to complete.

Tested on a palm treo 680 machine.

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
 sound/arm/pxa2xx-ac97-lib.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
index 6fc0ae9..1ecd0a66 100644
--- a/sound/arm/pxa2xx-ac97-lib.c
+++ b/sound/arm/pxa2xx-ac97-lib.c
@@ -148,6 +148,8 @@ static inline void pxa_ac97_warm_pxa27x(void)
 
 static inline void pxa_ac97_cold_pxa27x(void)
 {
+	unsigned int timeout;
+
 	GCR &=  GCR_COLD_RST;  /* clear everything but nCRST */
 	GCR &= ~GCR_COLD_RST;  /* then assert nCRST */
 
@@ -157,8 +159,10 @@ static inline void pxa_ac97_cold_pxa27x(void)
 	clk_enable(ac97conf_clk);
 	udelay(5);
 	clk_disable(ac97conf_clk);
-	GCR = GCR_COLD_RST;
-	udelay(50);
+	GCR = GCR_COLD_RST | GCR_WARM_RST;
+	timeout = 100;     /* wait for the codec-ready bit to be set */
+	while (!((GSR | gsr_bits) & (GSR_PCR | GSR_SCR)) && timeout--)
+		mdelay(1);
 }
 #endif
 
-- 
1.7.8.6

[PATCH 1/4] ALSA: pxa2xx: fix ac97 cold reset

[Mike Dunn]

Cold reset on the pxa27x currently fails and

     pxa2xx_ac97_try_cold_reset: cold reset timeout (GSR=0x44)

appears in the kernel log.  Through trial-and-error (the pxa270 developer's
manual is mostly incoherent on the topic of ac97 reset), I got cold reset to
complete by setting the WARM_RST bit in the GCR register (and later noticed that
pxa3xx does this for cold reset as well).  Also, a timeout loop is needed to
wait for the reset to complete.

Tested on a palm treo 680 machine.

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
 sound/arm/pxa2xx-ac97-lib.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
index 6fc0ae9..1ecd0a66 100644
--- a/sound/arm/pxa2xx-ac97-lib.c
+++ b/sound/arm/pxa2xx-ac97-lib.c
@@ -148,6 +148,8 @@ static inline void pxa_ac97_warm_pxa27x(void)
 
 static inline void pxa_ac97_cold_pxa27x(void)
 {
+	unsigned int timeout;
+
 	GCR &=  GCR_COLD_RST;  /* clear everything but nCRST */
 	GCR &= ~GCR_COLD_RST;  /* then assert nCRST */
 
@@ -157,8 +159,10 @@ static inline void pxa_ac97_cold_pxa27x(void)
 	clk_enable(ac97conf_clk);
 	udelay(5);
 	clk_disable(ac97conf_clk);
-	GCR = GCR_COLD_RST;
-	udelay(50);
+	GCR = GCR_COLD_RST | GCR_WARM_RST;
+	timeout = 100;     /* wait for the codec-ready bit to be set */
+	while (!((GSR | gsr_bits) & (GSR_PCR | GSR_SCR)) && timeout--)
+		mdelay(1);
 }
 #endif
 
-- 
1.7.8.6

[PATCH 2/4] ALSA: pxa27x: fix ac97 warm reset bug work-around code

[Mike Dunn]

This patch fixes some code that implements a work-around to a hardware bug in
the ac97 controller on the pxa27x.  A bug in the controller's warm reset
functionality requires that the mfp used by the controller as the AC97_nRESET
line be temporarily reconfigured as a generic output gpio (AF0) and manually
held high for the duration of the warm reset cycle.  This is what was done in
the original code, but it was broken long ago by commit
fb1bf8cd13bfa7ed0364ab0d82f717fc020d35f6 
([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset())
which changed the mfp to a GPIO input instead of a high output.

Tested on a palm treo 680 machine.  Reportedly, this broken code only prevents a
warm reset on hardware that lacks a pull-up on the line, which appears to be the
case for me.

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---

An earlier version of this patch failed to drive the output high as intended.
Thanks again Igor!  Warm reset now works and need not be skipped.

 arch/arm/mach-pxa/include/mach/mfp-pxa27x.h |    3 +++
 arch/arm/mach-pxa/pxa27x.c                  |   13 +++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h b/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
index a611ad3..8281e17 100644
--- a/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
+++ b/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
@@ -463,6 +463,9 @@
 	GPIO76_LCD_PCLK,	\
 	GPIO77_LCD_BIAS
 
+/* these enable a work-around for a hw bug in pxa27x during ac97 warm reset */
+#define GPIO113_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO113, AF0, DRIVE_HIGH)
+#define GPIO95_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO95, AF0, DRIVE_HIGH)
 
 extern int keypad_set_wake(unsigned int on);
 #endif /* __ASM_ARCH_MFP_PXA27X_H */
diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
index 8047ee0..eea90fe 100644
--- a/arch/arm/mach-pxa/pxa27x.c
+++ b/arch/arm/mach-pxa/pxa27x.c
@@ -47,14 +47,23 @@ void pxa27x_clear_otgph(void)
 EXPORT_SYMBOL(pxa27x_clear_otgph);
 
 static unsigned long ac97_reset_config[] = {
-	GPIO113_GPIO,
+	GPIO113_AC97_nRESET_GPIO_HIGH,
 	GPIO113_AC97_nRESET,
-	GPIO95_GPIO,
+	GPIO95_AC97_nRESET_GPIO_HIGH,
 	GPIO95_AC97_nRESET,
 };
 
 void pxa27x_assert_ac97reset(int reset_gpio, int on)
 {
+	/* set direction and level before switching from ac97 alt fn to gpio */
+	if (on) {
+		int ret = gpio_direction_output(reset_gpio, 1);
+		if (ret) {
+			pr_err("%s: gpio_direction_output failed: %d\n",
+				__func__, ret);
+			return;
+		}
+	}
 	if (reset_gpio == 113)
 		pxa2xx_mfp_config(on ? &ac97_reset_config[0] :
 				       &ac97_reset_config[1], 1);
-- 
1.7.8.6

[PATCH 2/4] ALSA: pxa27x: fix ac97 warm reset bug work-around code

[Mike Dunn]

This patch fixes some code that implements a work-around to a hardware bug in
the ac97 controller on the pxa27x.  A bug in the controller's warm reset
functionality requires that the mfp used by the controller as the AC97_nRESET
line be temporarily reconfigured as a generic output gpio (AF0) and manually
held high for the duration of the warm reset cycle.  This is what was done in
the original code, but it was broken long ago by commit
fb1bf8cd13bfa7ed0364ab0d82f717fc020d35f6 
([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset())
which changed the mfp to a GPIO input instead of a high output.

Tested on a palm treo 680 machine.  Reportedly, this broken code only prevents a
warm reset on hardware that lacks a pull-up on the line, which appears to be the
case for me.

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---

An earlier version of this patch failed to drive the output high as intended.
Thanks again Igor!  Warm reset now works and need not be skipped.

 arch/arm/mach-pxa/include/mach/mfp-pxa27x.h |    3 +++
 arch/arm/mach-pxa/pxa27x.c                  |   13 +++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h b/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
index a611ad3..8281e17 100644
--- a/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
+++ b/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
@@ -463,6 +463,9 @@
 	GPIO76_LCD_PCLK,	\
 	GPIO77_LCD_BIAS
 
+/* these enable a work-around for a hw bug in pxa27x during ac97 warm reset */
+#define GPIO113_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO113, AF0, DRIVE_HIGH)
+#define GPIO95_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO95, AF0, DRIVE_HIGH)
 
 extern int keypad_set_wake(unsigned int on);
 #endif /* __ASM_ARCH_MFP_PXA27X_H */
diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
index 8047ee0..eea90fe 100644
--- a/arch/arm/mach-pxa/pxa27x.c
+++ b/arch/arm/mach-pxa/pxa27x.c
@@ -47,14 +47,23 @@ void pxa27x_clear_otgph(void)
 EXPORT_SYMBOL(pxa27x_clear_otgph);
 
 static unsigned long ac97_reset_config[] = {
-	GPIO113_GPIO,
+	GPIO113_AC97_nRESET_GPIO_HIGH,
 	GPIO113_AC97_nRESET,
-	GPIO95_GPIO,
+	GPIO95_AC97_nRESET_GPIO_HIGH,
 	GPIO95_AC97_nRESET,
 };
 
 void pxa27x_assert_ac97reset(int reset_gpio, int on)
 {
+	/* set direction and level before switching from ac97 alt fn to gpio */
+	if (on) {
+		int ret = gpio_direction_output(reset_gpio, 1);
+		if (ret) {
+			pr_err("%s: gpio_direction_output failed: %d\n",
+				__func__, ret);
+			return;
+		}
+	}
 	if (reset_gpio == 113)
 		pxa2xx_mfp_config(on ? &ac97_reset_config[0] :
 				       &ac97_reset_config[1], 1);
-- 
1.7.8.6

[PATCH 3/4] ALSA: pxa27x: ac97 controller driver requests gpio

[Mike Dunn]

During initialization, the ac97 driver must obtain the gpio for the mfp that is
used by the ac97 controller as the AC97_nRESET line, or else gpiolib will issue
a warning and stack dump.  Gpiolib may change this to a hard error soon.  The
line is used during an ac97 warm reset for working around a hardware bug in the
controller.

Tested on a palm treo 680 machine.

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
 sound/arm/pxa2xx-ac97-lib.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
index 1ecd0a66..416d2e3 100644
--- a/sound/arm/pxa2xx-ac97-lib.c
+++ b/sound/arm/pxa2xx-ac97-lib.c
@@ -18,6 +18,7 @@
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/io.h>
+#include <linux/gpio.h>
 
 #include <sound/ac97_codec.h>
 #include <sound/pxa2xx-lib.h>
@@ -344,7 +345,12 @@ int pxa2xx_ac97_hw_probe(struct platform_device *dev)
 	}
 
 	if (cpu_is_pxa27x()) {
-		/* Use GPIO 113 as AC97 Reset on Bulverde */
+		ret = gpio_request(reset_gpio, "pxa27x ac97 reset");
+		if (ret < 0) {
+			pr_err("%s: gpio_request() failed: %d\n",
+			       __func__, ret);
+			goto err_conf;
+		}
 		pxa27x_assert_ac97reset(reset_gpio, 0);
 		ac97conf_clk = clk_get(&dev->dev, "AC97CONFCLK");
 		if (IS_ERR(ac97conf_clk)) {
@@ -388,6 +394,8 @@ EXPORT_SYMBOL_GPL(pxa2xx_ac97_hw_probe);
 
 void pxa2xx_ac97_hw_remove(struct platform_device *dev)
 {
+	if (cpu_is_pxa27x())
+		gpio_free(reset_gpio);
 	GCR |= GCR_ACLINK_OFF;
 	free_irq(IRQ_AC97, NULL);
 	if (ac97conf_clk) {
-- 
1.7.8.6

[PATCH 3/4] ALSA: pxa27x: ac97 controller driver requests gpio

[Mike Dunn]

During initialization, the ac97 driver must obtain the gpio for the mfp that is
used by the ac97 controller as the AC97_nRESET line, or else gpiolib will issue
a warning and stack dump.  Gpiolib may change this to a hard error soon.  The
line is used during an ac97 warm reset for working around a hardware bug in the
controller.

Tested on a palm treo 680 machine.

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
 sound/arm/pxa2xx-ac97-lib.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
index 1ecd0a66..416d2e3 100644
--- a/sound/arm/pxa2xx-ac97-lib.c
+++ b/sound/arm/pxa2xx-ac97-lib.c
@@ -18,6 +18,7 @@
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/io.h>
+#include <linux/gpio.h>
 
 #include <sound/ac97_codec.h>
 #include <sound/pxa2xx-lib.h>
@@ -344,7 +345,12 @@ int pxa2xx_ac97_hw_probe(struct platform_device *dev)
 	}
 
 	if (cpu_is_pxa27x()) {
-		/* Use GPIO 113 as AC97 Reset on Bulverde */
+		ret = gpio_request(reset_gpio, "pxa27x ac97 reset");
+		if (ret < 0) {
+			pr_err("%s: gpio_request() failed: %d\n",
+			       __func__, ret);
+			goto err_conf;
+		}
 		pxa27x_assert_ac97reset(reset_gpio, 0);
 		ac97conf_clk = clk_get(&dev->dev, "AC97CONFCLK");
 		if (IS_ERR(ac97conf_clk)) {
@@ -388,6 +394,8 @@ EXPORT_SYMBOL_GPL(pxa2xx_ac97_hw_probe);
 
 void pxa2xx_ac97_hw_remove(struct platform_device *dev)
 {
+	if (cpu_is_pxa27x())
+		gpio_free(reset_gpio);
 	GCR |= GCR_ACLINK_OFF;
 	free_irq(IRQ_AC97, NULL);
 	if (ac97conf_clk) {
-- 
1.7.8.6

[PATCH 4/4] ALSA: pxa27x: rename pxa27x_assert_ac97()

[Mike Dunn]

This patch does nothing functionally, it just gives the function a new name and
modifies the prototype slightly in order to clarify what the function is doing
(which is not necessarily asserting the reset).
Some commentary also added.

Tested on a palm treo 680 machine.

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---

checkpatch.pl issues a warning for this patch:
	      WARNING: externs should be avoided in .c files
but I just want to rename the function.

 arch/arm/mach-pxa/pxa27x.c  |   29 +++++++++++++++++++----------
 sound/arm/pxa2xx-ac97-lib.c |    8 ++++----
 2 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
index eea90fe..896bf94 100644
--- a/arch/arm/mach-pxa/pxa27x.c
+++ b/arch/arm/mach-pxa/pxa27x.c
@@ -53,26 +53,35 @@ static unsigned long ac97_reset_config[] = {
 	GPIO95_AC97_nRESET,
 };
 
-void pxa27x_assert_ac97reset(int reset_gpio, int on)
+void pxa27x_configure_ac97reset(int reset_mfp, bool to_gpio)
 {
+	/*
+	 * This helper function is used to work around a bug in the pxa27x's
+	 * ac97 controller during a warm reset.  The configuration of the
+	 * reset_mfp is changed as follows:
+	 * to_gpio == true: configured to generic output gpio and driven high
+	 * to_gpio == false: configured to ac97 controller alt fn AC97_nRESET
+	 */
+
 	/* set direction and level before switching from ac97 alt fn to gpio */
-	if (on) {
-		int ret = gpio_direction_output(reset_gpio, 1);
+	if (to_gpio == true) {
+		int ret = gpio_direction_output(reset_mfp, 1);
 		if (ret) {
 			pr_err("%s: gpio_direction_output failed: %d\n",
 				__func__, ret);
 			return;
 		}
 	}
-	if (reset_gpio == 113)
-		pxa2xx_mfp_config(on ? &ac97_reset_config[0] :
-				       &ac97_reset_config[1], 1);
 
-	if (reset_gpio == 95)
-		pxa2xx_mfp_config(on ? &ac97_reset_config[2] :
-				       &ac97_reset_config[3], 1);
+	if (reset_mfp == 113)
+		pxa2xx_mfp_config(to_gpio ? &ac97_reset_config[0] :
+				  &ac97_reset_config[1], 1);
+
+	if (reset_mfp == 95)
+		pxa2xx_mfp_config(to_gpio ? &ac97_reset_config[2] :
+				  &ac97_reset_config[3], 1);
 }
-EXPORT_SYMBOL_GPL(pxa27x_assert_ac97reset);
+EXPORT_SYMBOL_GPL(pxa27x_configure_ac97reset);
 
 /* Crystal clock: 13MHz */
 #define BASE_CLK	13000000
diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
index 416d2e3..fbac476 100644
--- a/sound/arm/pxa2xx-ac97-lib.c
+++ b/sound/arm/pxa2xx-ac97-lib.c
@@ -34,7 +34,7 @@ static struct clk *ac97_clk;
 static struct clk *ac97conf_clk;
 static int reset_gpio;
 
-extern void pxa27x_assert_ac97reset(int reset_gpio, int on);
+extern void pxa27x_configure_ac97reset(int reset_mfp, bool to_gpio);
 
 /*
  * Beware PXA27x bugs:
@@ -140,10 +140,10 @@ static inline void pxa_ac97_warm_pxa27x(void)
 	gsr_bits = 0;
 
 	/* warm reset broken on Bulverde, so manually keep AC97 reset high */
-	pxa27x_assert_ac97reset(reset_gpio, 1);
+	pxa27x_configure_ac97reset(reset_gpio, true);
 	udelay(10);
 	GCR |= GCR_WARM_RST;
-	pxa27x_assert_ac97reset(reset_gpio, 0);
+	pxa27x_configure_ac97reset(reset_gpio, false);
 	udelay(500);
 }
 
@@ -351,7 +351,7 @@ int pxa2xx_ac97_hw_probe(struct platform_device *dev)
 			       __func__, ret);
 			goto err_conf;
 		}
-		pxa27x_assert_ac97reset(reset_gpio, 0);
+		pxa27x_configure_ac97reset(reset_gpio, false);
 		ac97conf_clk = clk_get(&dev->dev, "AC97CONFCLK");
 		if (IS_ERR(ac97conf_clk)) {
 			ret = PTR_ERR(ac97conf_clk);
-- 
1.7.8.6

[PATCH 4/4] ALSA: pxa27x: rename pxa27x_assert_ac97()

[Mike Dunn]

This patch does nothing functionally, it just gives the function a new name and
modifies the prototype slightly in order to clarify what the function is doing
(which is not necessarily asserting the reset).
Some commentary also added.

Tested on a palm treo 680 machine.

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---

checkpatch.pl issues a warning for this patch:
	      WARNING: externs should be avoided in .c files
but I just want to rename the function.

 arch/arm/mach-pxa/pxa27x.c  |   29 +++++++++++++++++++----------
 sound/arm/pxa2xx-ac97-lib.c |    8 ++++----
 2 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
index eea90fe..896bf94 100644
--- a/arch/arm/mach-pxa/pxa27x.c
+++ b/arch/arm/mach-pxa/pxa27x.c
@@ -53,26 +53,35 @@ static unsigned long ac97_reset_config[] = {
 	GPIO95_AC97_nRESET,
 };
 
-void pxa27x_assert_ac97reset(int reset_gpio, int on)
+void pxa27x_configure_ac97reset(int reset_mfp, bool to_gpio)
 {
+	/*
+	 * This helper function is used to work around a bug in the pxa27x's
+	 * ac97 controller during a warm reset.  The configuration of the
+	 * reset_mfp is changed as follows:
+	 * to_gpio == true: configured to generic output gpio and driven high
+	 * to_gpio == false: configured to ac97 controller alt fn AC97_nRESET
+	 */
+
 	/* set direction and level before switching from ac97 alt fn to gpio */
-	if (on) {
-		int ret = gpio_direction_output(reset_gpio, 1);
+	if (to_gpio == true) {
+		int ret = gpio_direction_output(reset_mfp, 1);
 		if (ret) {
 			pr_err("%s: gpio_direction_output failed: %d\n",
 				__func__, ret);
 			return;
 		}
 	}
-	if (reset_gpio == 113)
-		pxa2xx_mfp_config(on ? &ac97_reset_config[0] :
-				       &ac97_reset_config[1], 1);
 
-	if (reset_gpio == 95)
-		pxa2xx_mfp_config(on ? &ac97_reset_config[2] :
-				       &ac97_reset_config[3], 1);
+	if (reset_mfp == 113)
+		pxa2xx_mfp_config(to_gpio ? &ac97_reset_config[0] :
+				  &ac97_reset_config[1], 1);
+
+	if (reset_mfp == 95)
+		pxa2xx_mfp_config(to_gpio ? &ac97_reset_config[2] :
+				  &ac97_reset_config[3], 1);
 }
-EXPORT_SYMBOL_GPL(pxa27x_assert_ac97reset);
+EXPORT_SYMBOL_GPL(pxa27x_configure_ac97reset);
 
 /* Crystal clock: 13MHz */
 #define BASE_CLK	13000000
diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
index 416d2e3..fbac476 100644
--- a/sound/arm/pxa2xx-ac97-lib.c
+++ b/sound/arm/pxa2xx-ac97-lib.c
@@ -34,7 +34,7 @@ static struct clk *ac97_clk;
 static struct clk *ac97conf_clk;
 static int reset_gpio;
 
-extern void pxa27x_assert_ac97reset(int reset_gpio, int on);
+extern void pxa27x_configure_ac97reset(int reset_mfp, bool to_gpio);
 
 /*
  * Beware PXA27x bugs:
@@ -140,10 +140,10 @@ static inline void pxa_ac97_warm_pxa27x(void)
 	gsr_bits = 0;
 
 	/* warm reset broken on Bulverde, so manually keep AC97 reset high */
-	pxa27x_assert_ac97reset(reset_gpio, 1);
+	pxa27x_configure_ac97reset(reset_gpio, true);
 	udelay(10);
 	GCR |= GCR_WARM_RST;
-	pxa27x_assert_ac97reset(reset_gpio, 0);
+	pxa27x_configure_ac97reset(reset_gpio, false);
 	udelay(500);
 }
 
@@ -351,7 +351,7 @@ int pxa2xx_ac97_hw_probe(struct platform_device *dev)
 			       __func__, ret);
 			goto err_conf;
 		}
-		pxa27x_assert_ac97reset(reset_gpio, 0);
+		pxa27x_configure_ac97reset(reset_gpio, false);
 		ac97conf_clk = clk_get(&dev->dev, "AC97CONFCLK");
 		if (IS_ERR(ac97conf_clk)) {
 			ret = PTR_ERR(ac97conf_clk);
-- 
1.7.8.6

Re: [PATCH 0/4] ALSA: pxa27x: ac97 reset fixes

[Robert Jarzmik]

Mike Dunn <mikedunn@newsguy.com> writes:

> AC97 reset is broken on the pxa27x.  These patches fix it by addressing multiple
> problems:
>   - cold reset fails because the GCR register is not set correctly
>   - warm reset fails because some code used to work around a hardware bug is
>     broken
>   - the intent of the work-around code is obfuscated by unfortunate function
>     naming
>
> This patch set replaces some earlier individual patches that had some problems.
> Thanks Robert and Igor.

Yes, this looks good. I'll test that one, but it does indeed look promising.
Igor, your opinion here ?

Cheers.

--
Robert

[PATCH 0/4] ALSA: pxa27x: ac97 reset fixes

[Robert Jarzmik]

Mike Dunn <mikedunn@newsguy.com> writes:

> AC97 reset is broken on the pxa27x.  These patches fix it by addressing multiple
> problems:
>   - cold reset fails because the GCR register is not set correctly
>   - warm reset fails because some code used to work around a hardware bug is
>     broken
>   - the intent of the work-around code is obfuscated by unfortunate function
>     naming
>
> This patch set replaces some earlier individual patches that had some problems.
> Thanks Robert and Igor.

Yes, this looks good. I'll test that one, but it does indeed look promising.
Igor, your opinion here ?

Cheers.

--
Robert

Re: [PATCH 1/4] ALSA: pxa2xx: fix ac97 cold reset

[Igor Grinberg]

On 01/06/13 21:13, Mike Dunn wrote:
> Cold reset on the pxa27x currently fails and
> 
>      pxa2xx_ac97_try_cold_reset: cold reset timeout (GSR=0x44)
> 
> appears in the kernel log.  Through trial-and-error (the pxa270 developer's
> manual is mostly incoherent on the topic of ac97 reset), I got cold reset to
> complete by setting the WARM_RST bit in the GCR register (and later noticed that
> pxa3xx does this for cold reset as well).  Also, a timeout loop is needed to
> wait for the reset to complete.
> 
> Tested on a palm treo 680 machine.
> 
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> ---
>  sound/arm/pxa2xx-ac97-lib.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
> index 6fc0ae9..1ecd0a66 100644
> --- a/sound/arm/pxa2xx-ac97-lib.c
> +++ b/sound/arm/pxa2xx-ac97-lib.c
> @@ -148,6 +148,8 @@ static inline void pxa_ac97_warm_pxa27x(void)
>  
>  static inline void pxa_ac97_cold_pxa27x(void)
>  {
> +	unsigned int timeout;
> +
>  	GCR &=  GCR_COLD_RST;  /* clear everything but nCRST */
>  	GCR &= ~GCR_COLD_RST;  /* then assert nCRST */
>  
> @@ -157,8 +159,10 @@ static inline void pxa_ac97_cold_pxa27x(void)
>  	clk_enable(ac97conf_clk);
>  	udelay(5);
>  	clk_disable(ac97conf_clk);
> -	GCR = GCR_COLD_RST;
> -	udelay(50);
> +	GCR = GCR_COLD_RST | GCR_WARM_RST;
> +	timeout = 100;     /* wait for the codec-ready bit to be set */
> +	while (!((GSR | gsr_bits) & (GSR_PCR | GSR_SCR)) && timeout--)
> +		mdelay(1);

Can we use msleep() instead?
May be this will require to change the granularity to 10...

>  }
>  #endif
>  

-- 
Regards,
Igor.

[PATCH 1/4] ALSA: pxa2xx: fix ac97 cold reset

[Igor Grinberg]

On 01/06/13 21:13, Mike Dunn wrote:
> Cold reset on the pxa27x currently fails and
> 
>      pxa2xx_ac97_try_cold_reset: cold reset timeout (GSR=0x44)
> 
> appears in the kernel log.  Through trial-and-error (the pxa270 developer's
> manual is mostly incoherent on the topic of ac97 reset), I got cold reset to
> complete by setting the WARM_RST bit in the GCR register (and later noticed that
> pxa3xx does this for cold reset as well).  Also, a timeout loop is needed to
> wait for the reset to complete.
> 
> Tested on a palm treo 680 machine.
> 
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> ---
>  sound/arm/pxa2xx-ac97-lib.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
> index 6fc0ae9..1ecd0a66 100644
> --- a/sound/arm/pxa2xx-ac97-lib.c
> +++ b/sound/arm/pxa2xx-ac97-lib.c
> @@ -148,6 +148,8 @@ static inline void pxa_ac97_warm_pxa27x(void)
>  
>  static inline void pxa_ac97_cold_pxa27x(void)
>  {
> +	unsigned int timeout;
> +
>  	GCR &=  GCR_COLD_RST;  /* clear everything but nCRST */
>  	GCR &= ~GCR_COLD_RST;  /* then assert nCRST */
>  
> @@ -157,8 +159,10 @@ static inline void pxa_ac97_cold_pxa27x(void)
>  	clk_enable(ac97conf_clk);
>  	udelay(5);
>  	clk_disable(ac97conf_clk);
> -	GCR = GCR_COLD_RST;
> -	udelay(50);
> +	GCR = GCR_COLD_RST | GCR_WARM_RST;
> +	timeout = 100;     /* wait for the codec-ready bit to be set */
> +	while (!((GSR | gsr_bits) & (GSR_PCR | GSR_SCR)) && timeout--)
> +		mdelay(1);

Can we use msleep() instead?
May be this will require to change the granularity to 10...

>  }
>  #endif
>  

-- 
Regards,
Igor.

Re: [PATCH 2/4] ALSA: pxa27x: fix ac97 warm reset bug work-around code

[Igor Grinberg]

On 01/06/13 21:13, Mike Dunn wrote:
> This patch fixes some code that implements a work-around to a hardware bug in
> the ac97 controller on the pxa27x.  A bug in the controller's warm reset
> functionality requires that the mfp used by the controller as the AC97_nRESET
> line be temporarily reconfigured as a generic output gpio (AF0) and manually
> held high for the duration of the warm reset cycle.  This is what was done in
> the original code, but it was broken long ago by commit
> fb1bf8cd13bfa7ed0364ab0d82f717fc020d35f6 
> ([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset())
> which changed the mfp to a GPIO input instead of a high output.
> 
> Tested on a palm treo 680 machine.  Reportedly, this broken code only prevents a
> warm reset on hardware that lacks a pull-up on the line, which appears to be the
> case for me.
> 
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> ---
> 
> An earlier version of this patch failed to drive the output high as intended.
> Thanks again Igor!  Warm reset now works and need not be skipped.
> 
>  arch/arm/mach-pxa/include/mach/mfp-pxa27x.h |    3 +++
>  arch/arm/mach-pxa/pxa27x.c                  |   13 +++++++++++--
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h b/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
> index a611ad3..8281e17 100644
> --- a/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
> +++ b/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
> @@ -463,6 +463,9 @@
>  	GPIO76_LCD_PCLK,	\
>  	GPIO77_LCD_BIAS
>  
> +/* these enable a work-around for a hw bug in pxa27x during ac97 warm reset */
> +#define GPIO113_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO113, AF0, DRIVE_HIGH)
> +#define GPIO95_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO95, AF0, DRIVE_HIGH)

Do we really need to change the LPM? Because DRIVE_* only affect LPM.

>  
>  extern int keypad_set_wake(unsigned int on);
>  #endif /* __ASM_ARCH_MFP_PXA27X_H */
> diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
> index 8047ee0..eea90fe 100644
> --- a/arch/arm/mach-pxa/pxa27x.c
> +++ b/arch/arm/mach-pxa/pxa27x.c
> @@ -47,14 +47,23 @@ void pxa27x_clear_otgph(void)
>  EXPORT_SYMBOL(pxa27x_clear_otgph);
>  
>  static unsigned long ac97_reset_config[] = {
> -	GPIO113_GPIO,
> +	GPIO113_AC97_nRESET_GPIO_HIGH,
>  	GPIO113_AC97_nRESET,
> -	GPIO95_GPIO,
> +	GPIO95_AC97_nRESET_GPIO_HIGH,
>  	GPIO95_AC97_nRESET,
>  };
>  
>  void pxa27x_assert_ac97reset(int reset_gpio, int on)
>  {
> +	/* set direction and level before switching from ac97 alt fn to gpio */
> +	if (on) {
> +		int ret = gpio_direction_output(reset_gpio, 1);
> +		if (ret) {
> +			pr_err("%s: gpio_direction_output failed: %d\n",
> +				__func__, ret);
> +			return;
> +		}
> +	}

Same concern, as you raised in reply to my patch.
As pxa2xx_mfp_config() changes GPDR for input,
the above should be done after the MFP AF0 switch.

>  	if (reset_gpio == 113)
>  		pxa2xx_mfp_config(on ? &ac97_reset_config[0] :
>  				       &ac97_reset_config[1], 1);

-- 
Regards,
Igor.

[PATCH 2/4] ALSA: pxa27x: fix ac97 warm reset bug work-around code

[Igor Grinberg]

On 01/06/13 21:13, Mike Dunn wrote:
> This patch fixes some code that implements a work-around to a hardware bug in
> the ac97 controller on the pxa27x.  A bug in the controller's warm reset
> functionality requires that the mfp used by the controller as the AC97_nRESET
> line be temporarily reconfigured as a generic output gpio (AF0) and manually
> held high for the duration of the warm reset cycle.  This is what was done in
> the original code, but it was broken long ago by commit
> fb1bf8cd13bfa7ed0364ab0d82f717fc020d35f6 
> ([ARM] pxa: introduce processor specific pxa27x_assert_ac97reset())
> which changed the mfp to a GPIO input instead of a high output.
> 
> Tested on a palm treo 680 machine.  Reportedly, this broken code only prevents a
> warm reset on hardware that lacks a pull-up on the line, which appears to be the
> case for me.
> 
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> ---
> 
> An earlier version of this patch failed to drive the output high as intended.
> Thanks again Igor!  Warm reset now works and need not be skipped.
> 
>  arch/arm/mach-pxa/include/mach/mfp-pxa27x.h |    3 +++
>  arch/arm/mach-pxa/pxa27x.c                  |   13 +++++++++++--
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h b/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
> index a611ad3..8281e17 100644
> --- a/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
> +++ b/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
> @@ -463,6 +463,9 @@
>  	GPIO76_LCD_PCLK,	\
>  	GPIO77_LCD_BIAS
>  
> +/* these enable a work-around for a hw bug in pxa27x during ac97 warm reset */
> +#define GPIO113_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO113, AF0, DRIVE_HIGH)
> +#define GPIO95_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO95, AF0, DRIVE_HIGH)

Do we really need to change the LPM? Because DRIVE_* only affect LPM.

>  
>  extern int keypad_set_wake(unsigned int on);
>  #endif /* __ASM_ARCH_MFP_PXA27X_H */
> diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
> index 8047ee0..eea90fe 100644
> --- a/arch/arm/mach-pxa/pxa27x.c
> +++ b/arch/arm/mach-pxa/pxa27x.c
> @@ -47,14 +47,23 @@ void pxa27x_clear_otgph(void)
>  EXPORT_SYMBOL(pxa27x_clear_otgph);
>  
>  static unsigned long ac97_reset_config[] = {
> -	GPIO113_GPIO,
> +	GPIO113_AC97_nRESET_GPIO_HIGH,
>  	GPIO113_AC97_nRESET,
> -	GPIO95_GPIO,
> +	GPIO95_AC97_nRESET_GPIO_HIGH,
>  	GPIO95_AC97_nRESET,
>  };
>  
>  void pxa27x_assert_ac97reset(int reset_gpio, int on)
>  {
> +	/* set direction and level before switching from ac97 alt fn to gpio */
> +	if (on) {
> +		int ret = gpio_direction_output(reset_gpio, 1);
> +		if (ret) {
> +			pr_err("%s: gpio_direction_output failed: %d\n",
> +				__func__, ret);
> +			return;
> +		}
> +	}

Same concern, as you raised in reply to my patch.
As pxa2xx_mfp_config() changes GPDR for input,
the above should be done after the MFP AF0 switch.

>  	if (reset_gpio == 113)
>  		pxa2xx_mfp_config(on ? &ac97_reset_config[0] :
>  				       &ac97_reset_config[1], 1);

-- 
Regards,
Igor.

Re: [PATCH 3/4] ALSA: pxa27x: ac97 controller driver requests gpio

[Igor Grinberg]

On 01/06/13 21:13, Mike Dunn wrote:
> During initialization, the ac97 driver must obtain the gpio for the mfp that is
> used by the ac97 controller as the AC97_nRESET line, or else gpiolib will issue
> a warning and stack dump.  Gpiolib may change this to a hard error soon.  The
> line is used during an ac97 warm reset for working around a hardware bug in the
> controller.
> 
> Tested on a palm treo 680 machine.
> 
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> ---
>  sound/arm/pxa2xx-ac97-lib.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
> index 1ecd0a66..416d2e3 100644
> --- a/sound/arm/pxa2xx-ac97-lib.c
> +++ b/sound/arm/pxa2xx-ac97-lib.c
> @@ -18,6 +18,7 @@
>  #include <linux/delay.h>
>  #include <linux/module.h>
>  #include <linux/io.h>
> +#include <linux/gpio.h>
>  
>  #include <sound/ac97_codec.h>
>  #include <sound/pxa2xx-lib.h>
> @@ -344,7 +345,12 @@ int pxa2xx_ac97_hw_probe(struct platform_device *dev)
>  	}
>  
>  	if (cpu_is_pxa27x()) {
> -		/* Use GPIO 113 as AC97 Reset on Bulverde */
> +		ret = gpio_request(reset_gpio, "pxa27x ac97 reset");
> +		if (ret < 0) {
> +			pr_err("%s: gpio_request() failed: %d\n",
> +			       __func__, ret);
> +			goto err_conf;
> +		}

I think if we are fine with the request in the driver,
then we should move the direction and value configuration also
to the driver, but let the pxa27x_assert_ac97reset() only switch
the AF.

>  		pxa27x_assert_ac97reset(reset_gpio, 0);
>  		ac97conf_clk = clk_get(&dev->dev, "AC97CONFCLK");
>  		if (IS_ERR(ac97conf_clk)) {
> @@ -388,6 +394,8 @@ EXPORT_SYMBOL_GPL(pxa2xx_ac97_hw_probe);
>  
>  void pxa2xx_ac97_hw_remove(struct platform_device *dev)
>  {
> +	if (cpu_is_pxa27x())
> +		gpio_free(reset_gpio);
>  	GCR |= GCR_ACLINK_OFF;
>  	free_irq(IRQ_AC97, NULL);
>  	if (ac97conf_clk) {

-- 
Regards,
Igor.

[PATCH 3/4] ALSA: pxa27x: ac97 controller driver requests gpio

[Igor Grinberg]

On 01/06/13 21:13, Mike Dunn wrote:
> During initialization, the ac97 driver must obtain the gpio for the mfp that is
> used by the ac97 controller as the AC97_nRESET line, or else gpiolib will issue
> a warning and stack dump.  Gpiolib may change this to a hard error soon.  The
> line is used during an ac97 warm reset for working around a hardware bug in the
> controller.
> 
> Tested on a palm treo 680 machine.
> 
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> ---
>  sound/arm/pxa2xx-ac97-lib.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
> index 1ecd0a66..416d2e3 100644
> --- a/sound/arm/pxa2xx-ac97-lib.c
> +++ b/sound/arm/pxa2xx-ac97-lib.c
> @@ -18,6 +18,7 @@
>  #include <linux/delay.h>
>  #include <linux/module.h>
>  #include <linux/io.h>
> +#include <linux/gpio.h>
>  
>  #include <sound/ac97_codec.h>
>  #include <sound/pxa2xx-lib.h>
> @@ -344,7 +345,12 @@ int pxa2xx_ac97_hw_probe(struct platform_device *dev)
>  	}
>  
>  	if (cpu_is_pxa27x()) {
> -		/* Use GPIO 113 as AC97 Reset on Bulverde */
> +		ret = gpio_request(reset_gpio, "pxa27x ac97 reset");
> +		if (ret < 0) {
> +			pr_err("%s: gpio_request() failed: %d\n",
> +			       __func__, ret);
> +			goto err_conf;
> +		}

I think if we are fine with the request in the driver,
then we should move the direction and value configuration also
to the driver, but let the pxa27x_assert_ac97reset() only switch
the AF.

>  		pxa27x_assert_ac97reset(reset_gpio, 0);
>  		ac97conf_clk = clk_get(&dev->dev, "AC97CONFCLK");
>  		if (IS_ERR(ac97conf_clk)) {
> @@ -388,6 +394,8 @@ EXPORT_SYMBOL_GPL(pxa2xx_ac97_hw_probe);
>  
>  void pxa2xx_ac97_hw_remove(struct platform_device *dev)
>  {
> +	if (cpu_is_pxa27x())
> +		gpio_free(reset_gpio);
>  	GCR |= GCR_ACLINK_OFF;
>  	free_irq(IRQ_AC97, NULL);
>  	if (ac97conf_clk) {

-- 
Regards,
Igor.

Re: [PATCH 4/4] ALSA: pxa27x: rename pxa27x_assert_ac97()

[Igor Grinberg]

On 01/06/13 21:14, Mike Dunn wrote:
> This patch does nothing functionally, it just gives the function a new name and
> modifies the prototype slightly in order to clarify what the function is doing
> (which is not necessarily asserting the reset).
> Some commentary also added.
> 
> Tested on a palm treo 680 machine.
> 
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>

This patch makes sense, but again raising your concern...
The gpio_direction_output(reset_gpio, 1) should be called after
the change to AF0.

> ---
> 
> checkpatch.pl issues a warning for this patch:
> 	      WARNING: externs should be avoided in .c files
> but I just want to rename the function.
> 
>  arch/arm/mach-pxa/pxa27x.c  |   29 +++++++++++++++++++----------
>  sound/arm/pxa2xx-ac97-lib.c |    8 ++++----
>  2 files changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
> index eea90fe..896bf94 100644
> --- a/arch/arm/mach-pxa/pxa27x.c
> +++ b/arch/arm/mach-pxa/pxa27x.c
> @@ -53,26 +53,35 @@ static unsigned long ac97_reset_config[] = {
>  	GPIO95_AC97_nRESET,
>  };
>  
> -void pxa27x_assert_ac97reset(int reset_gpio, int on)
> +void pxa27x_configure_ac97reset(int reset_mfp, bool to_gpio)
>  {
> +	/*
> +	 * This helper function is used to work around a bug in the pxa27x's
> +	 * ac97 controller during a warm reset.  The configuration of the
> +	 * reset_mfp is changed as follows:
> +	 * to_gpio == true: configured to generic output gpio and driven high
> +	 * to_gpio == false: configured to ac97 controller alt fn AC97_nRESET
> +	 */
> +
>  	/* set direction and level before switching from ac97 alt fn to gpio */
> -	if (on) {
> -		int ret = gpio_direction_output(reset_gpio, 1);
> +	if (to_gpio == true) {
> +		int ret = gpio_direction_output(reset_mfp, 1);
>  		if (ret) {
>  			pr_err("%s: gpio_direction_output failed: %d\n",
>  				__func__, ret);
>  			return;
>  		}
>  	}
> -	if (reset_gpio == 113)
> -		pxa2xx_mfp_config(on ? &ac97_reset_config[0] :
> -				       &ac97_reset_config[1], 1);
>  
> -	if (reset_gpio == 95)
> -		pxa2xx_mfp_config(on ? &ac97_reset_config[2] :
> -				       &ac97_reset_config[3], 1);
> +	if (reset_mfp == 113)
> +		pxa2xx_mfp_config(to_gpio ? &ac97_reset_config[0] :
> +				  &ac97_reset_config[1], 1);
> +
> +	if (reset_mfp == 95)
> +		pxa2xx_mfp_config(to_gpio ? &ac97_reset_config[2] :
> +				  &ac97_reset_config[3], 1);
>  }
> -EXPORT_SYMBOL_GPL(pxa27x_assert_ac97reset);
> +EXPORT_SYMBOL_GPL(pxa27x_configure_ac97reset);
>  
>  /* Crystal clock: 13MHz */
>  #define BASE_CLK	13000000
> diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
> index 416d2e3..fbac476 100644
> --- a/sound/arm/pxa2xx-ac97-lib.c
> +++ b/sound/arm/pxa2xx-ac97-lib.c
> @@ -34,7 +34,7 @@ static struct clk *ac97_clk;
>  static struct clk *ac97conf_clk;
>  static int reset_gpio;
>  
> -extern void pxa27x_assert_ac97reset(int reset_gpio, int on);
> +extern void pxa27x_configure_ac97reset(int reset_mfp, bool to_gpio);
>  
>  /*
>   * Beware PXA27x bugs:
> @@ -140,10 +140,10 @@ static inline void pxa_ac97_warm_pxa27x(void)
>  	gsr_bits = 0;
>  
>  	/* warm reset broken on Bulverde, so manually keep AC97 reset high */
> -	pxa27x_assert_ac97reset(reset_gpio, 1);
> +	pxa27x_configure_ac97reset(reset_gpio, true);
>  	udelay(10);
>  	GCR |= GCR_WARM_RST;
> -	pxa27x_assert_ac97reset(reset_gpio, 0);
> +	pxa27x_configure_ac97reset(reset_gpio, false);
>  	udelay(500);
>  }
>  
> @@ -351,7 +351,7 @@ int pxa2xx_ac97_hw_probe(struct platform_device *dev)
>  			       __func__, ret);
>  			goto err_conf;
>  		}
> -		pxa27x_assert_ac97reset(reset_gpio, 0);
> +		pxa27x_configure_ac97reset(reset_gpio, false);
>  		ac97conf_clk = clk_get(&dev->dev, "AC97CONFCLK");
>  		if (IS_ERR(ac97conf_clk)) {
>  			ret = PTR_ERR(ac97conf_clk);

-- 
Regards,
Igor.

[PATCH 4/4] ALSA: pxa27x: rename pxa27x_assert_ac97()

[Igor Grinberg]

On 01/06/13 21:14, Mike Dunn wrote:
> This patch does nothing functionally, it just gives the function a new name and
> modifies the prototype slightly in order to clarify what the function is doing
> (which is not necessarily asserting the reset).
> Some commentary also added.
> 
> Tested on a palm treo 680 machine.
> 
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>

This patch makes sense, but again raising your concern...
The gpio_direction_output(reset_gpio, 1) should be called after
the change to AF0.

> ---
> 
> checkpatch.pl issues a warning for this patch:
> 	      WARNING: externs should be avoided in .c files
> but I just want to rename the function.
> 
>  arch/arm/mach-pxa/pxa27x.c  |   29 +++++++++++++++++++----------
>  sound/arm/pxa2xx-ac97-lib.c |    8 ++++----
>  2 files changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/mach-pxa/pxa27x.c b/arch/arm/mach-pxa/pxa27x.c
> index eea90fe..896bf94 100644
> --- a/arch/arm/mach-pxa/pxa27x.c
> +++ b/arch/arm/mach-pxa/pxa27x.c
> @@ -53,26 +53,35 @@ static unsigned long ac97_reset_config[] = {
>  	GPIO95_AC97_nRESET,
>  };
>  
> -void pxa27x_assert_ac97reset(int reset_gpio, int on)
> +void pxa27x_configure_ac97reset(int reset_mfp, bool to_gpio)
>  {
> +	/*
> +	 * This helper function is used to work around a bug in the pxa27x's
> +	 * ac97 controller during a warm reset.  The configuration of the
> +	 * reset_mfp is changed as follows:
> +	 * to_gpio == true: configured to generic output gpio and driven high
> +	 * to_gpio == false: configured to ac97 controller alt fn AC97_nRESET
> +	 */
> +
>  	/* set direction and level before switching from ac97 alt fn to gpio */
> -	if (on) {
> -		int ret = gpio_direction_output(reset_gpio, 1);
> +	if (to_gpio == true) {
> +		int ret = gpio_direction_output(reset_mfp, 1);
>  		if (ret) {
>  			pr_err("%s: gpio_direction_output failed: %d\n",
>  				__func__, ret);
>  			return;
>  		}
>  	}
> -	if (reset_gpio == 113)
> -		pxa2xx_mfp_config(on ? &ac97_reset_config[0] :
> -				       &ac97_reset_config[1], 1);
>  
> -	if (reset_gpio == 95)
> -		pxa2xx_mfp_config(on ? &ac97_reset_config[2] :
> -				       &ac97_reset_config[3], 1);
> +	if (reset_mfp == 113)
> +		pxa2xx_mfp_config(to_gpio ? &ac97_reset_config[0] :
> +				  &ac97_reset_config[1], 1);
> +
> +	if (reset_mfp == 95)
> +		pxa2xx_mfp_config(to_gpio ? &ac97_reset_config[2] :
> +				  &ac97_reset_config[3], 1);
>  }
> -EXPORT_SYMBOL_GPL(pxa27x_assert_ac97reset);
> +EXPORT_SYMBOL_GPL(pxa27x_configure_ac97reset);
>  
>  /* Crystal clock: 13MHz */
>  #define BASE_CLK	13000000
> diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
> index 416d2e3..fbac476 100644
> --- a/sound/arm/pxa2xx-ac97-lib.c
> +++ b/sound/arm/pxa2xx-ac97-lib.c
> @@ -34,7 +34,7 @@ static struct clk *ac97_clk;
>  static struct clk *ac97conf_clk;
>  static int reset_gpio;
>  
> -extern void pxa27x_assert_ac97reset(int reset_gpio, int on);
> +extern void pxa27x_configure_ac97reset(int reset_mfp, bool to_gpio);
>  
>  /*
>   * Beware PXA27x bugs:
> @@ -140,10 +140,10 @@ static inline void pxa_ac97_warm_pxa27x(void)
>  	gsr_bits = 0;
>  
>  	/* warm reset broken on Bulverde, so manually keep AC97 reset high */
> -	pxa27x_assert_ac97reset(reset_gpio, 1);
> +	pxa27x_configure_ac97reset(reset_gpio, true);
>  	udelay(10);
>  	GCR |= GCR_WARM_RST;
> -	pxa27x_assert_ac97reset(reset_gpio, 0);
> +	pxa27x_configure_ac97reset(reset_gpio, false);
>  	udelay(500);
>  }
>  
> @@ -351,7 +351,7 @@ int pxa2xx_ac97_hw_probe(struct platform_device *dev)
>  			       __func__, ret);
>  			goto err_conf;
>  		}
> -		pxa27x_assert_ac97reset(reset_gpio, 0);
> +		pxa27x_configure_ac97reset(reset_gpio, false);
>  		ac97conf_clk = clk_get(&dev->dev, "AC97CONFCLK");
>  		if (IS_ERR(ac97conf_clk)) {
>  			ret = PTR_ERR(ac97conf_clk);

-- 
Regards,
Igor.

Re: [PATCH 3/4] ALSA: pxa27x: ac97 controller driver requests gpio

[Igor Grinberg]

On 01/07/13 11:36, Igor Grinberg wrote:
> On 01/06/13 21:13, Mike Dunn wrote:
>> During initialization, the ac97 driver must obtain the gpio for the mfp that is
>> used by the ac97 controller as the AC97_nRESET line, or else gpiolib will issue
>> a warning and stack dump.  Gpiolib may change this to a hard error soon.  The
>> line is used during an ac97 warm reset for working around a hardware bug in the
>> controller.
>>
>> Tested on a palm treo 680 machine.
>>
>> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
>> ---
>>  sound/arm/pxa2xx-ac97-lib.c |   10 +++++++++-
>>  1 files changed, 9 insertions(+), 1 deletions(-)
>>
>> diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
>> index 1ecd0a66..416d2e3 100644
>> --- a/sound/arm/pxa2xx-ac97-lib.c
>> +++ b/sound/arm/pxa2xx-ac97-lib.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/delay.h>
>>  #include <linux/module.h>
>>  #include <linux/io.h>
>> +#include <linux/gpio.h>
>>  
>>  #include <sound/ac97_codec.h>
>>  #include <sound/pxa2xx-lib.h>
>> @@ -344,7 +345,12 @@ int pxa2xx_ac97_hw_probe(struct platform_device *dev)
>>  	}
>>  
>>  	if (cpu_is_pxa27x()) {
>> -		/* Use GPIO 113 as AC97 Reset on Bulverde */
>> +		ret = gpio_request(reset_gpio, "pxa27x ac97 reset");
>> +		if (ret < 0) {
>> +			pr_err("%s: gpio_request() failed: %d\n",
>> +			       __func__, ret);
>> +			goto err_conf;
>> +		}
> 
> I think if we are fine with the request in the driver,
> then we should move the direction and value configuration also
> to the driver, but let the pxa27x_assert_ac97reset() only switch
> the AF.

Also this patch should be combined with 2/4.

> 
>>  		pxa27x_assert_ac97reset(reset_gpio, 0);
>>  		ac97conf_clk = clk_get(&dev->dev, "AC97CONFCLK");
>>  		if (IS_ERR(ac97conf_clk)) {
>> @@ -388,6 +394,8 @@ EXPORT_SYMBOL_GPL(pxa2xx_ac97_hw_probe);
>>  
>>  void pxa2xx_ac97_hw_remove(struct platform_device *dev)
>>  {
>> +	if (cpu_is_pxa27x())
>> +		gpio_free(reset_gpio);
>>  	GCR |= GCR_ACLINK_OFF;
>>  	free_irq(IRQ_AC97, NULL);
>>  	if (ac97conf_clk) {
> 

-- 
Regards,
Igor.

[PATCH 3/4] ALSA: pxa27x: ac97 controller driver requests gpio

[Igor Grinberg]

On 01/07/13 11:36, Igor Grinberg wrote:
> On 01/06/13 21:13, Mike Dunn wrote:
>> During initialization, the ac97 driver must obtain the gpio for the mfp that is
>> used by the ac97 controller as the AC97_nRESET line, or else gpiolib will issue
>> a warning and stack dump.  Gpiolib may change this to a hard error soon.  The
>> line is used during an ac97 warm reset for working around a hardware bug in the
>> controller.
>>
>> Tested on a palm treo 680 machine.
>>
>> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
>> ---
>>  sound/arm/pxa2xx-ac97-lib.c |   10 +++++++++-
>>  1 files changed, 9 insertions(+), 1 deletions(-)
>>
>> diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
>> index 1ecd0a66..416d2e3 100644
>> --- a/sound/arm/pxa2xx-ac97-lib.c
>> +++ b/sound/arm/pxa2xx-ac97-lib.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/delay.h>
>>  #include <linux/module.h>
>>  #include <linux/io.h>
>> +#include <linux/gpio.h>
>>  
>>  #include <sound/ac97_codec.h>
>>  #include <sound/pxa2xx-lib.h>
>> @@ -344,7 +345,12 @@ int pxa2xx_ac97_hw_probe(struct platform_device *dev)
>>  	}
>>  
>>  	if (cpu_is_pxa27x()) {
>> -		/* Use GPIO 113 as AC97 Reset on Bulverde */
>> +		ret = gpio_request(reset_gpio, "pxa27x ac97 reset");
>> +		if (ret < 0) {
>> +			pr_err("%s: gpio_request() failed: %d\n",
>> +			       __func__, ret);
>> +			goto err_conf;
>> +		}
> 
> I think if we are fine with the request in the driver,
> then we should move the direction and value configuration also
> to the driver, but let the pxa27x_assert_ac97reset() only switch
> the AF.

Also this patch should be combined with 2/4.

> 
>>  		pxa27x_assert_ac97reset(reset_gpio, 0);
>>  		ac97conf_clk = clk_get(&dev->dev, "AC97CONFCLK");
>>  		if (IS_ERR(ac97conf_clk)) {
>> @@ -388,6 +394,8 @@ EXPORT_SYMBOL_GPL(pxa2xx_ac97_hw_probe);
>>  
>>  void pxa2xx_ac97_hw_remove(struct platform_device *dev)
>>  {
>> +	if (cpu_is_pxa27x())
>> +		gpio_free(reset_gpio);
>>  	GCR |= GCR_ACLINK_OFF;
>>  	free_irq(IRQ_AC97, NULL);
>>  	if (ac97conf_clk) {
> 

-- 
Regards,
Igor.

Re: [PATCH 2/4] ALSA: pxa27x: fix ac97 warm reset bug work-around code

[Russell King - ARM Linux]

On Mon, Jan 07, 2013 at 11:31:51AM +0200, Igor Grinberg wrote:
> On 01/06/13 21:13, Mike Dunn wrote:
> > +/* these enable a work-around for a hw bug in pxa27x during ac97 warm reset */
> > +#define GPIO113_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO113, AF0, DRIVE_HIGH)
> > +#define GPIO95_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO95, AF0, DRIVE_HIGH)
> 
> Do we really need to change the LPM? Because DRIVE_* only affect LPM.

Also, be very careful about driving output pins to peripherals high
during sleep modes, especially if the peripherals are powered down.
If they are powered down, you will drain power through the output
into the peripheral to the extent that you _can_ cause it to remain
partially powered up.

[PATCH 2/4] ALSA: pxa27x: fix ac97 warm reset bug work-around code

[Russell King - ARM Linux]

On Mon, Jan 07, 2013 at 11:31:51AM +0200, Igor Grinberg wrote:
> On 01/06/13 21:13, Mike Dunn wrote:
> > +/* these enable a work-around for a hw bug in pxa27x during ac97 warm reset */
> > +#define GPIO113_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO113, AF0, DRIVE_HIGH)
> > +#define GPIO95_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO95, AF0, DRIVE_HIGH)
> 
> Do we really need to change the LPM? Because DRIVE_* only affect LPM.

Also, be very careful about driving output pins to peripherals high
during sleep modes, especially if the peripherals are powered down.
If they are powered down, you will drain power through the output
into the peripheral to the extent that you _can_ cause it to remain
partially powered up.

Re: [PATCH 1/4] ALSA: pxa2xx: fix ac97 cold reset

[Mike Dunn]

On 01/07/2013 01:16 AM, Igor Grinberg wrote:
> On 01/06/13 21:13, Mike Dunn wrote:


[..]


>>  static inline void pxa_ac97_cold_pxa27x(void)
>>  {
>> +	unsigned int timeout;
>> +
>>  	GCR &=  GCR_COLD_RST;  /* clear everything but nCRST */
>>  	GCR &= ~GCR_COLD_RST;  /* then assert nCRST */
>>  
>> @@ -157,8 +159,10 @@ static inline void pxa_ac97_cold_pxa27x(void)
>>  	clk_enable(ac97conf_clk);
>>  	udelay(5);
>>  	clk_disable(ac97conf_clk);
>> -	GCR = GCR_COLD_RST;
>> -	udelay(50);
>> +	GCR = GCR_COLD_RST | GCR_WARM_RST;
>> +	timeout = 100;     /* wait for the codec-ready bit to be set */
>> +	while (!((GSR | gsr_bits) & (GSR_PCR | GSR_SCR)) && timeout--)
>> +		mdelay(1);
> 
> Can we use msleep() instead?
> May be this will require to change the granularity to 10...


Probably.  mdelay() is used by similiar code in the same file, so I just stayed
consistent.  The code runs very infrequently, so I didn't worry about it.

Thanks,
Mike

[PATCH 1/4] ALSA: pxa2xx: fix ac97 cold reset

[Mike Dunn]

On 01/07/2013 01:16 AM, Igor Grinberg wrote:
> On 01/06/13 21:13, Mike Dunn wrote:


[..]


>>  static inline void pxa_ac97_cold_pxa27x(void)
>>  {
>> +	unsigned int timeout;
>> +
>>  	GCR &=  GCR_COLD_RST;  /* clear everything but nCRST */
>>  	GCR &= ~GCR_COLD_RST;  /* then assert nCRST */
>>  
>> @@ -157,8 +159,10 @@ static inline void pxa_ac97_cold_pxa27x(void)
>>  	clk_enable(ac97conf_clk);
>>  	udelay(5);
>>  	clk_disable(ac97conf_clk);
>> -	GCR = GCR_COLD_RST;
>> -	udelay(50);
>> +	GCR = GCR_COLD_RST | GCR_WARM_RST;
>> +	timeout = 100;     /* wait for the codec-ready bit to be set */
>> +	while (!((GSR | gsr_bits) & (GSR_PCR | GSR_SCR)) && timeout--)
>> +		mdelay(1);
> 
> Can we use msleep() instead?
> May be this will require to change the granularity to 10...


Probably.  mdelay() is used by similiar code in the same file, so I just stayed
consistent.  The code runs very infrequently, so I didn't worry about it.

Thanks,
Mike

Re: [PATCH 2/4] ALSA: pxa27x: fix ac97 warm reset bug work-around code

[Mike Dunn]

On 01/07/2013 01:31 AM, Igor Grinberg wrote:
> On 01/06/13 21:13, Mike Dunn wrote:


[..]


>> diff --git a/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h b/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
>> index a611ad3..8281e17 100644
>> --- a/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
>> +++ b/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
>> @@ -463,6 +463,9 @@
>>  	GPIO76_LCD_PCLK,	\
>>  	GPIO77_LCD_BIAS
>>  
>> +/* these enable a work-around for a hw bug in pxa27x during ac97 warm reset */
>> +#define GPIO113_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO113, AF0, DRIVE_HIGH)
>> +#define GPIO95_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO95, AF0, DRIVE_HIGH)
> 
> Do we really need to change the LPM? Because DRIVE_* only affect LPM.


Oops, no, we don't.  In fact, since you pointed this out to me in your review of
the earlier patch, I meant to remove it.  It does no harm, but should be removed
for clarity.

Thanks,
Mike

[PATCH 2/4] ALSA: pxa27x: fix ac97 warm reset bug work-around code

[Mike Dunn]

On 01/07/2013 01:31 AM, Igor Grinberg wrote:
> On 01/06/13 21:13, Mike Dunn wrote:


[..]


>> diff --git a/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h b/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
>> index a611ad3..8281e17 100644
>> --- a/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
>> +++ b/arch/arm/mach-pxa/include/mach/mfp-pxa27x.h
>> @@ -463,6 +463,9 @@
>>  	GPIO76_LCD_PCLK,	\
>>  	GPIO77_LCD_BIAS
>>  
>> +/* these enable a work-around for a hw bug in pxa27x during ac97 warm reset */
>> +#define GPIO113_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO113, AF0, DRIVE_HIGH)
>> +#define GPIO95_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(GPIO95, AF0, DRIVE_HIGH)
> 
> Do we really need to change the LPM? Because DRIVE_* only affect LPM.


Oops, no, we don't.  In fact, since you pointed this out to me in your review of
the earlier patch, I meant to remove it.  It does no harm, but should be removed
for clarity.

Thanks,
Mike

Re: [PATCH 1/4] ALSA: pxa2xx: fix ac97 cold reset

[Igor Grinberg]

On 01/07/13 15:36, Mike Dunn wrote:
> On 01/07/2013 01:16 AM, Igor Grinberg wrote:
>> On 01/06/13 21:13, Mike Dunn wrote:
> 
> 
> [..]
> 
> 
>>>  static inline void pxa_ac97_cold_pxa27x(void)
>>>  {
>>> +	unsigned int timeout;
>>> +
>>>  	GCR &=  GCR_COLD_RST;  /* clear everything but nCRST */
>>>  	GCR &= ~GCR_COLD_RST;  /* then assert nCRST */
>>>  
>>> @@ -157,8 +159,10 @@ static inline void pxa_ac97_cold_pxa27x(void)
>>>  	clk_enable(ac97conf_clk);
>>>  	udelay(5);
>>>  	clk_disable(ac97conf_clk);
>>> -	GCR = GCR_COLD_RST;
>>> -	udelay(50);
>>> +	GCR = GCR_COLD_RST | GCR_WARM_RST;
>>> +	timeout = 100;     /* wait for the codec-ready bit to be set */
>>> +	while (!((GSR | gsr_bits) & (GSR_PCR | GSR_SCR)) && timeout--)
>>> +		mdelay(1);
>>
>> Can we use msleep() instead?
>> May be this will require to change the granularity to 10...
> 
> 
> Probably.  mdelay() is used by similiar code in the same file, so I just stayed
> consistent.  The code runs very infrequently, so I didn't worry about it.

Well, if we can, I think we'd better do, no?


-- 
Regards,
Igor.

[PATCH 1/4] ALSA: pxa2xx: fix ac97 cold reset

[Igor Grinberg]

On 01/07/13 15:36, Mike Dunn wrote:
> On 01/07/2013 01:16 AM, Igor Grinberg wrote:
>> On 01/06/13 21:13, Mike Dunn wrote:
> 
> 
> [..]
> 
> 
>>>  static inline void pxa_ac97_cold_pxa27x(void)
>>>  {
>>> +	unsigned int timeout;
>>> +
>>>  	GCR &=  GCR_COLD_RST;  /* clear everything but nCRST */
>>>  	GCR &= ~GCR_COLD_RST;  /* then assert nCRST */
>>>  
>>> @@ -157,8 +159,10 @@ static inline void pxa_ac97_cold_pxa27x(void)
>>>  	clk_enable(ac97conf_clk);
>>>  	udelay(5);
>>>  	clk_disable(ac97conf_clk);
>>> -	GCR = GCR_COLD_RST;
>>> -	udelay(50);
>>> +	GCR = GCR_COLD_RST | GCR_WARM_RST;
>>> +	timeout = 100;     /* wait for the codec-ready bit to be set */
>>> +	while (!((GSR | gsr_bits) & (GSR_PCR | GSR_SCR)) && timeout--)
>>> +		mdelay(1);
>>
>> Can we use msleep() instead?
>> May be this will require to change the granularity to 10...
> 
> 
> Probably.  mdelay() is used by similiar code in the same file, so I just stayed
> consistent.  The code runs very infrequently, so I didn't worry about it.

Well, if we can, I think we'd better do, no?


-- 
Regards,
Igor.

Re: [PATCH 3/4] ALSA: pxa27x: ac97 controller driver requests gpio

[Mike Dunn]

On 01/07/2013 01:36 AM, Igor Grinberg wrote:
> On 01/06/13 21:13, Mike Dunn wrote:
>> During initialization, the ac97 driver must obtain the gpio for the mfp that is
>> used by the ac97 controller as the AC97_nRESET line, or else gpiolib will issue
>> a warning and stack dump.  Gpiolib may change this to a hard error soon.  The
>> line is used during an ac97 warm reset for working around a hardware bug in the
>> controller.
>>
>> Tested on a palm treo 680 machine.
>>
>> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
>> ---
>>  sound/arm/pxa2xx-ac97-lib.c |   10 +++++++++-
>>  1 files changed, 9 insertions(+), 1 deletions(-)
>>
>> diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
>> index 1ecd0a66..416d2e3 100644
>> --- a/sound/arm/pxa2xx-ac97-lib.c
>> +++ b/sound/arm/pxa2xx-ac97-lib.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/delay.h>
>>  #include <linux/module.h>
>>  #include <linux/io.h>
>> +#include <linux/gpio.h>
>>  
>>  #include <sound/ac97_codec.h>
>>  #include <sound/pxa2xx-lib.h>
>> @@ -344,7 +345,12 @@ int pxa2xx_ac97_hw_probe(struct platform_device *dev)
>>  	}
>>  
>>  	if (cpu_is_pxa27x()) {
>> -		/* Use GPIO 113 as AC97 Reset on Bulverde */
>> +		ret = gpio_request(reset_gpio, "pxa27x ac97 reset");
>> +		if (ret < 0) {
>> +			pr_err("%s: gpio_request() failed: %d\n",
>> +			       __func__, ret);
>> +			goto err_conf;
>> +		}
> 
> I think if we are fine with the request in the driver,
> then we should move the direction and value configuration also
> to the driver, but let the pxa27x_assert_ac97reset() only switch
> the AF.


Well, I won't make a fuss on this point, but with code that runs very
infrequently, I thought that the cost of a call to gpio_direction_output() was
worth the clarity and insurance.

Thanks,
Mike

[PATCH 3/4] ALSA: pxa27x: ac97 controller driver requests gpio

[Mike Dunn]

On 01/07/2013 01:36 AM, Igor Grinberg wrote:
> On 01/06/13 21:13, Mike Dunn wrote:
>> During initialization, the ac97 driver must obtain the gpio for the mfp that is
>> used by the ac97 controller as the AC97_nRESET line, or else gpiolib will issue
>> a warning and stack dump.  Gpiolib may change this to a hard error soon.  The
>> line is used during an ac97 warm reset for working around a hardware bug in the
>> controller.
>>
>> Tested on a palm treo 680 machine.
>>
>> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
>> ---
>>  sound/arm/pxa2xx-ac97-lib.c |   10 +++++++++-
>>  1 files changed, 9 insertions(+), 1 deletions(-)
>>
>> diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
>> index 1ecd0a66..416d2e3 100644
>> --- a/sound/arm/pxa2xx-ac97-lib.c
>> +++ b/sound/arm/pxa2xx-ac97-lib.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/delay.h>
>>  #include <linux/module.h>
>>  #include <linux/io.h>
>> +#include <linux/gpio.h>
>>  
>>  #include <sound/ac97_codec.h>
>>  #include <sound/pxa2xx-lib.h>
>> @@ -344,7 +345,12 @@ int pxa2xx_ac97_hw_probe(struct platform_device *dev)
>>  	}
>>  
>>  	if (cpu_is_pxa27x()) {
>> -		/* Use GPIO 113 as AC97 Reset on Bulverde */
>> +		ret = gpio_request(reset_gpio, "pxa27x ac97 reset");
>> +		if (ret < 0) {
>> +			pr_err("%s: gpio_request() failed: %d\n",
>> +			       __func__, ret);
>> +			goto err_conf;
>> +		}
> 
> I think if we are fine with the request in the driver,
> then we should move the direction and value configuration also
> to the driver, but let the pxa27x_assert_ac97reset() only switch
> the AF.


Well, I won't make a fuss on this point, but with code that runs very
infrequently, I thought that the cost of a call to gpio_direction_output() was
worth the clarity and insurance.

Thanks,
Mike

Re: [PATCH 1/4] ALSA: pxa2xx: fix ac97 cold reset

[Mike Dunn]

On 01/07/2013 05:57 AM, Igor Grinberg wrote:
> On 01/07/13 15:36, Mike Dunn wrote:
>> On 01/07/2013 01:16 AM, Igor Grinberg wrote:
>>> On 01/06/13 21:13, Mike Dunn wrote:
>>
>>
>> [..]
>>
>>
>>>>  static inline void pxa_ac97_cold_pxa27x(void)
>>>>  {
>>>> +	unsigned int timeout;
>>>> +
>>>>  	GCR &=  GCR_COLD_RST;  /* clear everything but nCRST */
>>>>  	GCR &= ~GCR_COLD_RST;  /* then assert nCRST */
>>>>  
>>>> @@ -157,8 +159,10 @@ static inline void pxa_ac97_cold_pxa27x(void)
>>>>  	clk_enable(ac97conf_clk);
>>>>  	udelay(5);
>>>>  	clk_disable(ac97conf_clk);
>>>> -	GCR = GCR_COLD_RST;
>>>> -	udelay(50);
>>>> +	GCR = GCR_COLD_RST | GCR_WARM_RST;
>>>> +	timeout = 100;     /* wait for the codec-ready bit to be set */
>>>> +	while (!((GSR | gsr_bits) & (GSR_PCR | GSR_SCR)) && timeout--)
>>>> +		mdelay(1);
>>>
>>> Can we use msleep() instead?
>>> May be this will require to change the granularity to 10...
>>
>>
>> Probably.  mdelay() is used by similiar code in the same file, so I just stayed
>> consistent.  The code runs very infrequently, so I didn't worry about it.
> 
> Well, if we can, I think we'd better do, no?


If you feel strongly about it, I'll defer to you.  But again my arguments are
(1) the code runs very infrequently, and (2) very similar code for the other pxa
family siblings uses mdelay().

Thanks,
Mike

[PATCH 1/4] ALSA: pxa2xx: fix ac97 cold reset

[Mike Dunn]

On 01/07/2013 05:57 AM, Igor Grinberg wrote:
> On 01/07/13 15:36, Mike Dunn wrote:
>> On 01/07/2013 01:16 AM, Igor Grinberg wrote:
>>> On 01/06/13 21:13, Mike Dunn wrote:
>>
>>
>> [..]
>>
>>
>>>>  static inline void pxa_ac97_cold_pxa27x(void)
>>>>  {
>>>> +	unsigned int timeout;
>>>> +
>>>>  	GCR &=  GCR_COLD_RST;  /* clear everything but nCRST */
>>>>  	GCR &= ~GCR_COLD_RST;  /* then assert nCRST */
>>>>  
>>>> @@ -157,8 +159,10 @@ static inline void pxa_ac97_cold_pxa27x(void)
>>>>  	clk_enable(ac97conf_clk);
>>>>  	udelay(5);
>>>>  	clk_disable(ac97conf_clk);
>>>> -	GCR = GCR_COLD_RST;
>>>> -	udelay(50);
>>>> +	GCR = GCR_COLD_RST | GCR_WARM_RST;
>>>> +	timeout = 100;     /* wait for the codec-ready bit to be set */
>>>> +	while (!((GSR | gsr_bits) & (GSR_PCR | GSR_SCR)) && timeout--)
>>>> +		mdelay(1);
>>>
>>> Can we use msleep() instead?
>>> May be this will require to change the granularity to 10...
>>
>>
>> Probably.  mdelay() is used by similiar code in the same file, so I just stayed
>> consistent.  The code runs very infrequently, so I didn't worry about it.
> 
> Well, if we can, I think we'd better do, no?


If you feel strongly about it, I'll defer to you.  But again my arguments are
(1) the code runs very infrequently, and (2) very similar code for the other pxa
family siblings uses mdelay().

Thanks,
Mike

Re: [PATCH 4/4] ALSA: pxa27x: rename pxa27x_assert_ac97()

[Mike Dunn]

On 01/07/2013 01:39 AM, Igor Grinberg wrote:
> 
> 
> On 01/06/13 21:14, Mike Dunn wrote:
>> This patch does nothing functionally, it just gives the function a new name and
>> modifies the prototype slightly in order to clarify what the function is doing
>> (which is not necessarily asserting the reset).
>> Some commentary also added.
>>
>> Tested on a palm treo 680 machine.
>>
>> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> 
> This patch makes sense, but again raising your concern...
> The gpio_direction_output(reset_gpio, 1) should be called after
> the change to AF0.


Can you clarify please?  Don't you mean *before* the change to AF0?  The proper
values must be set in the GPDR and GPSR/GPCR before changing to AF0, no?  This
not only makes intuitive sense, it is also consistent with the information in
the pxa270 developer's manual.  Once AF0 is set, the last values that were set
for direction and level take immediate effect, no?

Thanks,
Mike

[PATCH 4/4] ALSA: pxa27x: rename pxa27x_assert_ac97()

[Mike Dunn]

On 01/07/2013 01:39 AM, Igor Grinberg wrote:
> 
> 
> On 01/06/13 21:14, Mike Dunn wrote:
>> This patch does nothing functionally, it just gives the function a new name and
>> modifies the prototype slightly in order to clarify what the function is doing
>> (which is not necessarily asserting the reset).
>> Some commentary also added.
>>
>> Tested on a palm treo 680 machine.
>>
>> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> 
> This patch makes sense, but again raising your concern...
> The gpio_direction_output(reset_gpio, 1) should be called after
> the change to AF0.


Can you clarify please?  Don't you mean *before* the change to AF0?  The proper
values must be set in the GPDR and GPSR/GPCR before changing to AF0, no?  This
not only makes intuitive sense, it is also consistent with the information in
the pxa270 developer's manual.  Once AF0 is set, the last values that were set
for direction and level take immediate effect, no?

Thanks,
Mike

Re: [PATCH 1/4] ALSA: pxa2xx: fix ac97 cold reset

[Igor Grinberg]

On 01/07/13 16:19, Mike Dunn wrote:
> On 01/07/2013 05:57 AM, Igor Grinberg wrote:
>> On 01/07/13 15:36, Mike Dunn wrote:
>>> On 01/07/2013 01:16 AM, Igor Grinberg wrote:
>>>> On 01/06/13 21:13, Mike Dunn wrote:
>>>
>>>
>>> [..]
>>>
>>>
>>>>>  static inline void pxa_ac97_cold_pxa27x(void)
>>>>>  {
>>>>> +	unsigned int timeout;
>>>>> +
>>>>>  	GCR &=  GCR_COLD_RST;  /* clear everything but nCRST */
>>>>>  	GCR &= ~GCR_COLD_RST;  /* then assert nCRST */
>>>>>  
>>>>> @@ -157,8 +159,10 @@ static inline void pxa_ac97_cold_pxa27x(void)
>>>>>  	clk_enable(ac97conf_clk);
>>>>>  	udelay(5);
>>>>>  	clk_disable(ac97conf_clk);
>>>>> -	GCR = GCR_COLD_RST;
>>>>> -	udelay(50);
>>>>> +	GCR = GCR_COLD_RST | GCR_WARM_RST;
>>>>> +	timeout = 100;     /* wait for the codec-ready bit to be set */
>>>>> +	while (!((GSR | gsr_bits) & (GSR_PCR | GSR_SCR)) && timeout--)
>>>>> +		mdelay(1);
>>>>
>>>> Can we use msleep() instead?
>>>> May be this will require to change the granularity to 10...
>>>
>>>
>>> Probably.  mdelay() is used by similiar code in the same file, so I just stayed
>>> consistent.  The code runs very infrequently, so I didn't worry about it.
>>
>> Well, if we can, I think we'd better do, no?
> 
> 
> If you feel strongly about it, I'll defer to you.  But again my arguments are
> (1) the code runs very infrequently, and (2) very similar code for the other pxa
> family siblings uses mdelay().

No, not too strong, but please, don't argument your code with something like:
"there are other places where this is done", because it might be done
improperly and just go in unnoticed.


-- 
Regards,
Igor.

[PATCH 1/4] ALSA: pxa2xx: fix ac97 cold reset

[Igor Grinberg]

On 01/07/13 16:19, Mike Dunn wrote:
> On 01/07/2013 05:57 AM, Igor Grinberg wrote:
>> On 01/07/13 15:36, Mike Dunn wrote:
>>> On 01/07/2013 01:16 AM, Igor Grinberg wrote:
>>>> On 01/06/13 21:13, Mike Dunn wrote:
>>>
>>>
>>> [..]
>>>
>>>
>>>>>  static inline void pxa_ac97_cold_pxa27x(void)
>>>>>  {
>>>>> +	unsigned int timeout;
>>>>> +
>>>>>  	GCR &=  GCR_COLD_RST;  /* clear everything but nCRST */
>>>>>  	GCR &= ~GCR_COLD_RST;  /* then assert nCRST */
>>>>>  
>>>>> @@ -157,8 +159,10 @@ static inline void pxa_ac97_cold_pxa27x(void)
>>>>>  	clk_enable(ac97conf_clk);
>>>>>  	udelay(5);
>>>>>  	clk_disable(ac97conf_clk);
>>>>> -	GCR = GCR_COLD_RST;
>>>>> -	udelay(50);
>>>>> +	GCR = GCR_COLD_RST | GCR_WARM_RST;
>>>>> +	timeout = 100;     /* wait for the codec-ready bit to be set */
>>>>> +	while (!((GSR | gsr_bits) & (GSR_PCR | GSR_SCR)) && timeout--)
>>>>> +		mdelay(1);
>>>>
>>>> Can we use msleep() instead?
>>>> May be this will require to change the granularity to 10...
>>>
>>>
>>> Probably.  mdelay() is used by similiar code in the same file, so I just stayed
>>> consistent.  The code runs very infrequently, so I didn't worry about it.
>>
>> Well, if we can, I think we'd better do, no?
> 
> 
> If you feel strongly about it, I'll defer to you.  But again my arguments are
> (1) the code runs very infrequently, and (2) very similar code for the other pxa
> family siblings uses mdelay().

No, not too strong, but please, don't argument your code with something like:
"there are other places where this is done", because it might be done
improperly and just go in unnoticed.


-- 
Regards,
Igor.

Re: [PATCH 3/4] ALSA: pxa27x: ac97 controller driver requests gpio

[Igor Grinberg]

On 01/07/13 16:10, Mike Dunn wrote:
> On 01/07/2013 01:36 AM, Igor Grinberg wrote:
>> On 01/06/13 21:13, Mike Dunn wrote:
>>> During initialization, the ac97 driver must obtain the gpio for the mfp that is
>>> used by the ac97 controller as the AC97_nRESET line, or else gpiolib will issue
>>> a warning and stack dump.  Gpiolib may change this to a hard error soon.  The
>>> line is used during an ac97 warm reset for working around a hardware bug in the
>>> controller.
>>>
>>> Tested on a palm treo 680 machine.
>>>
>>> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
>>> ---
>>>  sound/arm/pxa2xx-ac97-lib.c |   10 +++++++++-
>>>  1 files changed, 9 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
>>> index 1ecd0a66..416d2e3 100644
>>> --- a/sound/arm/pxa2xx-ac97-lib.c
>>> +++ b/sound/arm/pxa2xx-ac97-lib.c
>>> @@ -18,6 +18,7 @@
>>>  #include <linux/delay.h>
>>>  #include <linux/module.h>
>>>  #include <linux/io.h>
>>> +#include <linux/gpio.h>
>>>  
>>>  #include <sound/ac97_codec.h>
>>>  #include <sound/pxa2xx-lib.h>
>>> @@ -344,7 +345,12 @@ int pxa2xx_ac97_hw_probe(struct platform_device *dev)
>>>  	}
>>>  
>>>  	if (cpu_is_pxa27x()) {
>>> -		/* Use GPIO 113 as AC97 Reset on Bulverde */
>>> +		ret = gpio_request(reset_gpio, "pxa27x ac97 reset");
>>> +		if (ret < 0) {
>>> +			pr_err("%s: gpio_request() failed: %d\n",
>>> +			       __func__, ret);
>>> +			goto err_conf;
>>> +		}
>>
>> I think if we are fine with the request in the driver,
>> then we should move the direction and value configuration also
>> to the driver, but let the pxa27x_assert_ac97reset() only switch
>> the AF.
> 
> 
> Well, I won't make a fuss on this point, but with code that runs very
> infrequently, I thought that the cost of a call to gpio_direction_output() was
> worth the clarity and insurance.

This is not about how frequent the code runs, but the fact that the arch code
relies on sound code (outside of arch) to request the GPIO for it.
I would recommend to place the GPIO request call along with the
direction/value calls - in the same subsystem.

Also, the patch ordering is not good:
patch 2 drives the GPIO, and patch 3 requests it,
whereas it should be the other way around, so I would recommend to
combine both into one.

-- 
Regards,
Igor.

[PATCH 3/4] ALSA: pxa27x: ac97 controller driver requests gpio

[Igor Grinberg]

On 01/07/13 16:10, Mike Dunn wrote:
> On 01/07/2013 01:36 AM, Igor Grinberg wrote:
>> On 01/06/13 21:13, Mike Dunn wrote:
>>> During initialization, the ac97 driver must obtain the gpio for the mfp that is
>>> used by the ac97 controller as the AC97_nRESET line, or else gpiolib will issue
>>> a warning and stack dump.  Gpiolib may change this to a hard error soon.  The
>>> line is used during an ac97 warm reset for working around a hardware bug in the
>>> controller.
>>>
>>> Tested on a palm treo 680 machine.
>>>
>>> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
>>> ---
>>>  sound/arm/pxa2xx-ac97-lib.c |   10 +++++++++-
>>>  1 files changed, 9 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/sound/arm/pxa2xx-ac97-lib.c b/sound/arm/pxa2xx-ac97-lib.c
>>> index 1ecd0a66..416d2e3 100644
>>> --- a/sound/arm/pxa2xx-ac97-lib.c
>>> +++ b/sound/arm/pxa2xx-ac97-lib.c
>>> @@ -18,6 +18,7 @@
>>>  #include <linux/delay.h>
>>>  #include <linux/module.h>
>>>  #include <linux/io.h>
>>> +#include <linux/gpio.h>
>>>  
>>>  #include <sound/ac97_codec.h>
>>>  #include <sound/pxa2xx-lib.h>
>>> @@ -344,7 +345,12 @@ int pxa2xx_ac97_hw_probe(struct platform_device *dev)
>>>  	}
>>>  
>>>  	if (cpu_is_pxa27x()) {
>>> -		/* Use GPIO 113 as AC97 Reset on Bulverde */
>>> +		ret = gpio_request(reset_gpio, "pxa27x ac97 reset");
>>> +		if (ret < 0) {
>>> +			pr_err("%s: gpio_request() failed: %d\n",
>>> +			       __func__, ret);
>>> +			goto err_conf;
>>> +		}
>>
>> I think if we are fine with the request in the driver,
>> then we should move the direction and value configuration also
>> to the driver, but let the pxa27x_assert_ac97reset() only switch
>> the AF.
> 
> 
> Well, I won't make a fuss on this point, but with code that runs very
> infrequently, I thought that the cost of a call to gpio_direction_output() was
> worth the clarity and insurance.

This is not about how frequent the code runs, but the fact that the arch code
relies on sound code (outside of arch) to request the GPIO for it.
I would recommend to place the GPIO request call along with the
direction/value calls - in the same subsystem.

Also, the patch ordering is not good:
patch 2 drives the GPIO, and patch 3 requests it,
whereas it should be the other way around, so I would recommend to
combine both into one.

-- 
Regards,
Igor.

Re: [PATCH 4/4] ALSA: pxa27x: rename pxa27x_assert_ac97()

[Igor Grinberg]

On 01/07/13 16:34, Mike Dunn wrote:
> On 01/07/2013 01:39 AM, Igor Grinberg wrote:
>>
>>
>> On 01/06/13 21:14, Mike Dunn wrote:
>>> This patch does nothing functionally, it just gives the function a new name and
>>> modifies the prototype slightly in order to clarify what the function is doing
>>> (which is not necessarily asserting the reset).
>>> Some commentary also added.
>>>
>>> Tested on a palm treo 680 machine.
>>>
>>> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
>>
>> This patch makes sense, but again raising your concern...
>> The gpio_direction_output(reset_gpio, 1) should be called after
>> the change to AF0.
> 
> 
> Can you clarify please?  Don't you mean *before* the change to AF0?  The proper
> values must be set in the GPDR and GPSR/GPCR before changing to AF0, no?  This
> not only makes intuitive sense, it is also consistent with the information in
> the pxa270 developer's manual.  Once AF0 is set, the last values that were set
> for direction and level take immediate effect, no?

Yes, you are right, I've just forgot about the
#define GPIOx_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(...)
you introduced in patch 2, so no concern here. Sorry...


-- 
Regards,
Igor.

[PATCH 4/4] ALSA: pxa27x: rename pxa27x_assert_ac97()

[Igor Grinberg]

On 01/07/13 16:34, Mike Dunn wrote:
> On 01/07/2013 01:39 AM, Igor Grinberg wrote:
>>
>>
>> On 01/06/13 21:14, Mike Dunn wrote:
>>> This patch does nothing functionally, it just gives the function a new name and
>>> modifies the prototype slightly in order to clarify what the function is doing
>>> (which is not necessarily asserting the reset).
>>> Some commentary also added.
>>>
>>> Tested on a palm treo 680 machine.
>>>
>>> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
>>
>> This patch makes sense, but again raising your concern...
>> The gpio_direction_output(reset_gpio, 1) should be called after
>> the change to AF0.
> 
> 
> Can you clarify please?  Don't you mean *before* the change to AF0?  The proper
> values must be set in the GPDR and GPSR/GPCR before changing to AF0, no?  This
> not only makes intuitive sense, it is also consistent with the information in
> the pxa270 developer's manual.  Once AF0 is set, the last values that were set
> for direction and level take immediate effect, no?

Yes, you are right, I've just forgot about the
#define GPIOx_AC97_nRESET_GPIO_HIGH MFP_CFG_OUT(...)
you introduced in patch 2, so no concern here. Sorry...


-- 
Regards,
Igor.

Re: [PATCH 3/4] ALSA: pxa27x: ac97 controller driver requests gpio

[Mike Dunn]

On 01/07/2013 07:38 AM, Igor Grinberg wrote:
> On 01/07/13 16:10, Mike Dunn wrote:


[..]


>>
>> Well, I won't make a fuss on this point, but with code that runs very
>> infrequently, I thought that the cost of a call to gpio_direction_output() was
>> worth the clarity and insurance.
> 
> This is not about how frequent the code runs, but the fact that the arch code
> relies on sound code (outside of arch) to request the GPIO for it.
> I would recommend to place the GPIO request call along with the
> direction/value calls - in the same subsystem.


OK.  This is in some shared pxa code called by the ac97 driver during
initialization, where I earlier placed the gpio_request() calls only.


> 
> Also, the patch ordering is not good:
> patch 2 drives the GPIO, and patch 3 requests it,
> whereas it should be the other way around, so I would recommend to
> combine both into one.


Agreed.

Thanks,
Mike

[PATCH 3/4] ALSA: pxa27x: ac97 controller driver requests gpio

[Mike Dunn]

On 01/07/2013 07:38 AM, Igor Grinberg wrote:
> On 01/07/13 16:10, Mike Dunn wrote:


[..]


>>
>> Well, I won't make a fuss on this point, but with code that runs very
>> infrequently, I thought that the cost of a call to gpio_direction_output() was
>> worth the clarity and insurance.
> 
> This is not about how frequent the code runs, but the fact that the arch code
> relies on sound code (outside of arch) to request the GPIO for it.
> I would recommend to place the GPIO request call along with the
> direction/value calls - in the same subsystem.


OK.  This is in some shared pxa code called by the ac97 driver during
initialization, where I earlier placed the gpio_request() calls only.


> 
> Also, the patch ordering is not good:
> patch 2 drives the GPIO, and patch 3 requests it,
> whereas it should be the other way around, so I would recommend to
> combine both into one.


Agreed.

Thanks,
Mike