[PATCH] ASoC: codecs/wm8978: Only reconfigure GPIO1 when used for PLL output

[PATCH] ASoC: codecs/wm8978: Only reconfigure GPIO1 when used for PLL output

[Eric Millbrandt]

When GPIO1 is used for other purposes than PLL output (GPIO, Amute, etc) the
driver incorrectly reconfigures the pin to input when the PLL is recalculated.
Only reconfigure the pin for input when reconfiguring the PLL and GPIO1 is
used for PLL output.

Signed-off-by: Eric Millbrandt <emillbrandt@dekaresearch.com>
---
 sound/soc/codecs/wm8978.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/sound/soc/codecs/wm8978.c b/sound/soc/codecs/wm8978.c
index a706262..0659b41 100644
--- a/sound/soc/codecs/wm8978.c
+++ b/sound/soc/codecs/wm8978.c
@@ -509,7 +509,11 @@ static int wm8978_configure_pll(struct snd_soc_codec *codec)
 
 		dev_dbg(codec->dev, "%s: OPCLKDIV=%d\n", __func__, opclk_div);
 
-		snd_soc_update_bits(codec, WM8978_GPIO_CONTROL, 0x30,
+		/*
+		 * GPIO1 is used for OPCLK, reconfigure into default
+		 * mode as input - before configuring OPCLKDIV and PLL
+		 */
+		snd_soc_update_bits(codec, WM8978_GPIO_CONTROL, 0x37,
 				    (opclk_div - 1) << 4);
 
 		wm8978->f_pllout = f_opclk * opclk_div;
@@ -529,9 +533,6 @@ static int wm8978_configure_pll(struct snd_soc_codec *codec)
 			return idx;
 
 		wm8978->mclk_idx = idx;
-
-		/* GPIO1 into default mode as input - before configuring PLL */
-		snd_soc_update_bits(codec, WM8978_GPIO_CONTROL, 7, 0);
 	} else {
 		return -EINVAL;
 	}
@@ -638,8 +639,12 @@ static int wm8978_set_dai_sysclk(struct snd_soc_dai *codec_dai, int clk_id,
 		/* Clock CODEC directly from MCLK */
 		snd_soc_update_bits(codec, WM8978_CLOCKING, 0x100, 0);
 
-		/* GPIO1 into default mode as input - before configuring PLL */
-		snd_soc_update_bits(codec, WM8978_GPIO_CONTROL, 7, 0);
+		/*
+		 * If GPIO1 is used for OPCLK, reconfigure GPIO1 into default
+		 * mode as input - before configuring PLL
+		 */
+		if (f_opclk)
+			snd_soc_update_bits(codec, WM8978_GPIO_CONTROL, 7, 0);
 
 		/* Turn off PLL */
 		snd_soc_update_bits(codec, WM8978_POWER_MANAGEMENT_1, 0x20, 0);
-- 
1.7.2.5

Re: [PATCH] ASoC: codecs/wm8978: Only reconfigure GPIO1 when used for PLL output

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1217 bytes --]

On Fri, Nov 09, 2012 at 12:33:38PM -0500, Eric Millbrandt wrote:

> When GPIO1 is used for other purposes than PLL output (GPIO, Amute, etc) the
> driver incorrectly reconfigures the pin to input when the PLL is recalculated.
> Only reconfigure the pin for input when reconfiguring the PLL and GPIO1 is
> used for PLL output.

I'm really having a hard time understanding what this change improves.

> -		snd_soc_update_bits(codec, WM8978_GPIO_CONTROL, 0x30,
> +		/*
> +		 * GPIO1 is used for OPCLK, reconfigure into default
> +		 * mode as input - before configuring OPCLKDIV and PLL
> +		 */
> +		snd_soc_update_bits(codec, WM8978_GPIO_CONTROL, 0x37,
>  				    (opclk_div - 1) << 4);

Previously we weren't changing the pin mode at all, we were only
updating the bitfield that contains OPCLK.  With your change we will
also change the pin mode to input, but if the pin is doing any form of
output (or anything other than input mode) I'd expect this to at least
glitch things which seems actively harmful.  Why is it being set up as
an input?  Though I have to say that all this handling of the pin in the
existing driver looks at best odd.

Your changelog describes what you're doing but not what is being
fixed...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

Re: [PATCH] ASoC: codecs/wm8978: Only reconfigure GPIO1 when used for PLL output

[Eric Millbrandt]

Mark,

On 2012-11-13 Mark Brown wrote:

>> -            snd_soc_update_bits(codec, WM8978_GPIO_CONTROL, 0x30,
>> +            /*
>> +             * GPIO1 is used for OPCLK, reconfigure into default
>> +             * mode as input - before configuring OPCLKDIV and PLL
>> +             */
>> +            snd_soc_update_bits(codec, WM8978_GPIO_CONTROL, 0x37,
>>                                  (opclk_div - 1) << 4);
>
> Previously we weren't changing the pin mode at all, we were only
> updating the bitfield that contains OPCLK.  With your change we will
> also change the pin mode to input, but if the pin is doing any form of
> output (or anything other than input mode) I'd expect this to at least
> glitch things which seems actively harmful.  Why is it being set up as
> an input?  Though I have to say that all this handling of the pin in the
> existing driver looks at best odd.
>
> Your changelog describes what you're doing but not what is being
> fixed...

The issue was in the following snippet:

@@ -529,9 +533,6 @@ static int wm8978_configure_pll(struct snd_soc_codec *codec)
                        return idx;

                wm8978->mclk_idx = idx;
-
-               /* GPIO1 into default mode as input - before configuring PLL */
-               snd_soc_update_bits(codec, WM8978_GPIO_CONTROL, 7, 0);
        } else {
                return -EINVAL;
        }

This bit of code was always reconfiguring the pin to input when it was known
that OPCLK was not needed.  I am using GPIO1 to enable an amplifier, so I do
not want it reconfigured.  I moved the reconfiguring of GPIO1 to the block of
code under "if (f_opclk)", so we can safely assume that that pin is not being
used for something else.

However, I agree that the handling of the pin is odd.  Does OPCLK really need
to be reconfigured as input whenever the PLL values are updated?  I didn't
see anything in the user manual to suggest that that needed to be done.

Eric



This e-mail and the information, including any attachments, it contains are intended to be a confidential communication only to the person or entity to whom it is addressed and may contain information that is privileged. If the reader of this message is not the intended recipient, you are hereby notified that any dissemination, distribution or copying of this communication is strictly prohibited. If you have received this communication in error, please immediately notify the sender and destroy the original message.

Thank you.

Please consider the environment before printing this email.

Re: [PATCH] ASoC: codecs/wm8978: Only reconfigure GPIO1 when used for PLL output

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 579 bytes --]

On Tue, Nov 13, 2012 at 09:43:08AM -0500, Eric Millbrandt wrote:

> However, I agree that the handling of the pin is odd.  Does OPCLK really need
> to be reconfigured as input whenever the PLL values are updated?  I didn't
> see anything in the user manual to suggest that that needed to be done.

It should not be a requirement of the device itself.  It might be a
requirement of something using the clock but the system doing that
should take care of remuxing the pin as needed.  What I'd suggest doing
here is removing the management of the pin completely from this
function.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

Re: [PATCH] ASoC: codecs/wm8978: Only reconfigure GPIO1 when used for PLL output

[Eric Millbrandt]

On 2012-11-15 Mark Brown wrote:
> On Tue, Nov 13, 2012 at 09:43:08AM -0500, Eric Millbrandt wrote:
>
>> However, I agree that the handling of the pin is odd.  Does OPCLK
>> really need to be reconfigured as input whenever the PLL values are
>> updated?  I didn't see anything in the user manual to suggest that that
>> needed to be
> done.
>
> It should not be a requirement of the device itself.  It might be a
> requirement of something using the clock but the system doing that
> should take care of remuxing the pin as needed.  What I'd suggest doing
> here is removing the management of the pin completely from this
> function.

That sounds reasonable.  I'll make those changes and resubmit.

Eric




This e-mail and the information, including any attachments, it contains are intended to be a confidential communication only to the person or entity to whom it is addressed and may contain information that is privileged. If the reader of this message is not the intended recipient, you are hereby notified that any dissemination, distribution or copying of this communication is strictly prohibited. If you have received this communication in error, please immediately notify the sender and destroy the original message.

Thank you.

Please consider the environment before printing this email.