[alsa-devel] [PATCH] ASoC: codecs/wm8978: Only reconfigure GPIO1 when used for PLL output
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);
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...
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.
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.
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.
participants (2)
-
Eric Millbrandt -
Mark Brown