ASoC: rt5677: handle reset_pin logical state correctly
diff mbox

Message ID 1437154736-15320-1-git-send-email-anatol.pomozov@gmail.com
State New
Headers show

Commit Message

anatol.pomozov@gmail.com July 17, 2015, 5:38 p.m. UTC
According to the datasheet RESET is active low pin, i.e. system goes to reset
state when pin is low.

Handle logic state correctly - set reset_pin to logical 0 at boot time, and
set it to logical 1 when we need to reset the chip.

Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
---
 Documentation/devicetree/bindings/sound/rt5677.txt | 2 +-
 sound/soc/codecs/rt5677.c                          | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

Comments

Mark Brown Aug. 14, 2015, 7:01 p.m. UTC | #1
On Fri, Jul 17, 2015 at 10:38:56AM -0700, Anatol Pomozov wrote:
> According to the datasheet RESET is active low pin, i.e. system goes to reset
> state when pin is low.
> 
> Handle logic state correctly - set reset_pin to logical 0 at boot time, and
> set it to logical 1 when we need to reset the chip.

I'm a bit confused here - how was the original code (written by you it
seems...) tested?  This looks like it's fixing the device being held in
reset when it should be operation which seems like something that'd get
noticed.  Are there existing systems that the current code works for
which we need to handle here?
anatol.pomozov@gmail.com Oct. 28, 2015, 7:55 p.m. UTC | #2
Hi

Returning back to this discussion that slipped away from my mailbox.

On Fri, Aug 14, 2015 at 12:01 PM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Jul 17, 2015 at 10:38:56AM -0700, Anatol Pomozov wrote:
>> According to the datasheet RESET is active low pin, i.e. system goes to reset
>> state when pin is low.
>>
>> Handle logic state correctly - set reset_pin to logical 0 at boot time, and
>> set it to logical 1 when we need to reset the chip.
>
> I'm a bit confused here - how was the original code (written by you it
> seems...) tested?  This looks like it's fixing the device being held in
> reset when it should be operation which seems like something that'd get
> noticed.  Are there existing systems that the current code works for
> which we need to handle here?

While working with the original patch you mentioned I missed the
active low description at RESET pin. I configured gpio in the dts as
ACTIVE_HIGH and set the logic state to 0 at suspend mode. This puts
the chip into reset state correctly.

Later I discovered correct polarity and given change modifies gpio
properties accordingly. I also changed our dts gpio to ACTIVE_LOW.
Chip reset state still works as expected. The change does not modify
anything at the signalling level, it just adds double negative to the
code. Compare:

pin configured as ACTIVE_HIGH - to move codec out of reset we need to
set gpio logic level to 1. Physical signal is 1.
pin configured as ACTIVE_LOW - to move codec out of reset we need to
set gpio logic level to 0. Gpio becomes non-active that is represented
by physical signal level 1.
Mark Brown Oct. 29, 2015, 12:02 a.m. UTC | #3
On Wed, Oct 28, 2015 at 12:55:14PM -0700, Anatol Pomozov wrote:
> Returning back to this discussion that slipped away from my mailbox.

I'm afraid I no longer remember what this is about, sorry - if the patch
is needed please resend it.

Patch
diff mbox

diff --git a/Documentation/devicetree/bindings/sound/rt5677.txt b/Documentation/devicetree/bindings/sound/rt5677.txt
index f070789..1b3c13d 100644
--- a/Documentation/devicetree/bindings/sound/rt5677.txt
+++ b/Documentation/devicetree/bindings/sound/rt5677.txt
@@ -18,7 +18,7 @@  Required properties:
 Optional properties:
 
 - realtek,pow-ldo2-gpio : The GPIO that controls the CODEC's POW_LDO2 pin.
-- realtek,reset-gpio : The GPIO that controls the CODEC's RESET pin.
+- realtek,reset-gpio : The GPIO that controls the CODEC's RESET pin. Active low.
 
 - realtek,in1-differential
 - realtek,in2-differential
diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
index 3b46b35..433652f 100644
--- a/sound/soc/codecs/rt5677.c
+++ b/sound/soc/codecs/rt5677.c
@@ -4766,7 +4766,7 @@  static int rt5677_remove(struct snd_soc_codec *codec)
 	if (rt5677->pow_ldo2)
 		gpiod_set_value_cansleep(rt5677->pow_ldo2, 0);
 	if (rt5677->reset_pin)
-		gpiod_set_value_cansleep(rt5677->reset_pin, 0);
+		gpiod_set_value_cansleep(rt5677->reset_pin, 1);
 
 	return 0;
 }
@@ -4783,7 +4783,7 @@  static int rt5677_suspend(struct snd_soc_codec *codec)
 		if (rt5677->pow_ldo2)
 			gpiod_set_value_cansleep(rt5677->pow_ldo2, 0);
 		if (rt5677->reset_pin)
-			gpiod_set_value_cansleep(rt5677->reset_pin, 0);
+			gpiod_set_value_cansleep(rt5677->reset_pin, 1);
 	}
 
 	return 0;
@@ -4797,7 +4797,7 @@  static int rt5677_resume(struct snd_soc_codec *codec)
 		if (rt5677->pow_ldo2)
 			gpiod_set_value_cansleep(rt5677->pow_ldo2, 1);
 		if (rt5677->reset_pin)
-			gpiod_set_value_cansleep(rt5677->reset_pin, 1);
+			gpiod_set_value_cansleep(rt5677->reset_pin, 0);
 		if (rt5677->pow_ldo2 || rt5677->reset_pin)
 			msleep(10);
 
@@ -5142,7 +5142,7 @@  static int rt5677_i2c_probe(struct i2c_client *i2c,
 		rt5677->pow_ldo2 = 0;
 	}
 	rt5677->reset_pin = devm_gpiod_get_optional(&i2c->dev,
-			"realtek,reset", GPIOD_OUT_HIGH);
+			"realtek,reset", GPIOD_OUT_LOW);
 	if (IS_ERR(rt5677->reset_pin)) {
 		ret = PTR_ERR(rt5677->reset_pin);
 		dev_err(&i2c->dev, "Failed to request RESET: %d\n", ret);