[PATCH] arm64: dts: qcom: sc7180: Add sleep state for alc5682 codec

[PATCH] arm64: dts: qcom: sc7180: Add sleep state for alc5682 codec

[Joseph S. Barrera III]

Add sleep state to acl5682. In default, gpio28 (HP_IRQ) is bias-pull-up.
To save power, in the new sleep state, gpio28 is bias-disable.

sleeping, /sys/kernel/debug/gpio shows gpio28 as "no pull". When codec
is awake (microphone plugged in and in use), it shows gpio28 as "pull up".

Signed-off-by: Joseph S. Barrera III <joebar@chromium.org>
---

 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
index b5f534db135a..94dd6c34d997 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
@@ -755,8 +755,9 @@ hp_i2c: &i2c9 {
 	alc5682: codec@1a {
 		compatible = "realtek,rt5682i";
 		reg = <0x1a>;
-		pinctrl-names = "default";
+		pinctrl-names = "default", "sleep";
 		pinctrl-0 = <&hp_irq>;
+		pinctrl-1 = <&hp_sleep>;
 
 		#sound-dai-cells = <1>;
 
@@ -1336,6 +1337,18 @@ pinconf {
 		};
 	};
 
+	hp_sleep: hp-sleep {
+		pinmux {
+			pins = "gpio28";
+			function = "gpio";
+		};
+
+		pinconf {
+			pins = "gpio28";
+			bias-disable;
+		};
+	};
+
 	pen_irq_l: pen-irq-l {
 		pinmux {
 			pins = "gpio21";
-- 
2.31.0

Re: [PATCH] arm64: dts: qcom: sc7180: Add sleep state for alc5682 codec

[Alexandru M Stan]

On Thu, Aug 18, 2022 at 8:42 AM Joseph S. Barrera III
<joebar@chromium.org> wrote:
>
> Add sleep state to acl5682. In default, gpio28 (HP_IRQ) is bias-pull-up.
> To save power, in the new sleep state, gpio28 is bias-disable.
>
> sleeping, /sys/kernel/debug/gpio shows gpio28 as "no pull". When codec
> is awake (microphone plugged in and in use), it shows gpio28 as "pull up".
>
> Signed-off-by: Joseph S. Barrera III <joebar@chromium.org>
> ---
>
>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> index b5f534db135a..94dd6c34d997 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> @@ -755,8 +755,9 @@ hp_i2c: &i2c9 {
>         alc5682: codec@1a {
>                 compatible = "realtek,rt5682i";
>                 reg = <0x1a>;
> -               pinctrl-names = "default";
> +               pinctrl-names = "default", "sleep";
>                 pinctrl-0 = <&hp_irq>;
> +               pinctrl-1 = <&hp_sleep>;
>
>                 #sound-dai-cells = <1>;
>
> @@ -1336,6 +1337,18 @@ pinconf {
>                 };
>         };
>
> +       hp_sleep: hp-sleep {
> +               pinmux {
> +                       pins = "gpio28";
> +                       function = "gpio";
> +               };
> +
> +               pinconf {
> +                       pins = "gpio28";
> +                       bias-disable;
> +               };
> +       };
> +
>         pen_irq_l: pen-irq-l {
>                 pinmux {
>                         pins = "gpio21";
> --
> 2.31.0
>

Thanks, this should save us a current leak.

Reviewed-by: Alexandru Stan <amstan@chromium.org>

Re: [PATCH] arm64: dts: qcom: sc7180: Add sleep state for alc5682 codec

[Stephen Boyd]

Quoting Joseph S. Barrera III (2022-08-18 08:42:19)
> Add sleep state to acl5682. In default, gpio28 (HP_IRQ) is bias-pull-up.
> To save power, in the new sleep state, gpio28 is bias-disable.
>
> sleeping, /sys/kernel/debug/gpio shows gpio28 as "no pull". When codec

Is something missing? The sentence starts with 'sleeping'.

> is awake (microphone plugged in and in use), it shows gpio28 as "pull up".
>
> Signed-off-by: Joseph S. Barrera III <joebar@chromium.org>
> ---
>
>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> index b5f534db135a..94dd6c34d997 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> @@ -755,8 +755,9 @@ hp_i2c: &i2c9 {
>         alc5682: codec@1a {
>                 compatible = "realtek,rt5682i";
>                 reg = <0x1a>;
> -               pinctrl-names = "default";
> +               pinctrl-names = "default", "sleep";
>                 pinctrl-0 = <&hp_irq>;
> +               pinctrl-1 = <&hp_sleep>;
>
>                 #sound-dai-cells = <1>;
>
> @@ -1336,6 +1337,18 @@ pinconf {
>                 };
>         };
>
> +       hp_sleep: hp-sleep {
> +               pinmux {
> +                       pins = "gpio28";
> +                       function = "gpio";
> +               };
> +
> +               pinconf {
> +                       pins = "gpio28";
> +                       bias-disable;
> +               };

Does removing the bias cause an irq to trigger? I'm worried that this
change may cause a spurious irq upon entering or exiting sleep, maybe
both actually. The irq is double edged so we really want it to stay
stable at one level whenever the gpio interrupt hardware is sensing the
line.

From what I can tell the pin is powered by AVDD-supply and I can't tell
if that is ever powered off while the driver is probed. Probably not? If
the power to the pin on the codec is never turned off then there isn't a
power leak from what I can tell.

Re: [PATCH] arm64: dts: qcom: sc7180: Add sleep state for alc5682 codec

[Doug Anderson]

Hi,

On Thu, Aug 18, 2022 at 11:46 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Joseph S. Barrera III (2022-08-18 08:42:19)
> > Add sleep state to acl5682. In default, gpio28 (HP_IRQ) is bias-pull-up.
> > To save power, in the new sleep state, gpio28 is bias-disable.
> >
> > sleeping, /sys/kernel/debug/gpio shows gpio28 as "no pull". When codec
>
> Is something missing? The sentence starts with 'sleeping'.
>
> > is awake (microphone plugged in and in use), it shows gpio28 as "pull up".
> >
> > Signed-off-by: Joseph S. Barrera III <joebar@chromium.org>
> > ---
> >
> >  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > index b5f534db135a..94dd6c34d997 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > @@ -755,8 +755,9 @@ hp_i2c: &i2c9 {
> >         alc5682: codec@1a {
> >                 compatible = "realtek,rt5682i";
> >                 reg = <0x1a>;
> > -               pinctrl-names = "default";
> > +               pinctrl-names = "default", "sleep";
> >                 pinctrl-0 = <&hp_irq>;
> > +               pinctrl-1 = <&hp_sleep>;
> >
> >                 #sound-dai-cells = <1>;
> >
> > @@ -1336,6 +1337,18 @@ pinconf {
> >                 };
> >         };
> >
> > +       hp_sleep: hp-sleep {
> > +               pinmux {
> > +                       pins = "gpio28";
> > +                       function = "gpio";
> > +               };
> > +
> > +               pinconf {
> > +                       pins = "gpio28";
> > +                       bias-disable;
> > +               };
>
> Does removing the bias cause an irq to trigger? I'm worried that this
> change may cause a spurious irq upon entering or exiting sleep, maybe
> both actually. The irq is double edged so we really want it to stay
> stable at one level whenever the gpio interrupt hardware is sensing the
> line.
>
> From what I can tell the pin is powered by AVDD-supply

Officially DBVDD I think, but (at least on the trogdor hardware) they
are the same rail.

> and I can't tell
> if that is ever powered off while the driver is probed. Probably not?

It doesn't seem to be. The driver I'm looking at turns on all the
regulators at probe time and never turns them off.

> If
> the power to the pin on the codec is never turned off then there isn't a
> power leak from what I can tell.

I tend to agree with Stephen's analysis. We actually need to keep the
pullup enabled unless we are actually turning power off to the codec,
which we don't seem to be doing.

I guess I'm a little surprised that we don't even seem to turn any of
this codec's regulators off in S3. That seems like it would be drawing
power that we don't want. Maybe the "low power" mode of the codec is
low enough and we need to avoid powering it off to avoid pops / hisses
in S3 or something? If that's true, this might be one of those places
where the "LPM" of the regulators might actually be useful...


-Doug

Re: [PATCH] arm64: dts: qcom: sc7180: Add sleep state for alc5682 codec

[Joseph S. Barrera III]

On 8/18/22 1:34 PM, Doug Anderson wrote:
> I tend to agree with Stephen's analysis. We actually need to keep the
> pullup enabled unless we are actually turning power off to the codec,
> which we don't seem to be doing.
> 
> I guess I'm a little surprised that we don't even seem to turn any of
> this codec's regulators off in S3. That seems like it would be drawing
> power that we don't want. Maybe the "low power" mode of the codec is
> low enough and we need to avoid powering it off to avoid pops / hisses
> in S3 or something? If that's true, this might be one of those places
> where the "LPM" of the regulators might actually be useful...

OK, fair enough, so suggestions on what I should do instead? Should I
look at why or how to turn the regulators off? Should I look into LPM?
Are there existing bugs for such work?

Re: [PATCH] arm64: dts: qcom: sc7180: Add sleep state for alc5682 codec

[Doug Anderson]

Hi,

On Thu, Aug 18, 2022 at 3:19 PM Joseph S. Barrera III
<joebar@chromium.org> wrote:
>
> On 8/18/22 1:34 PM, Doug Anderson wrote:
> > I tend to agree with Stephen's analysis. We actually need to keep the
> > pullup enabled unless we are actually turning power off to the codec,
> > which we don't seem to be doing.
> >
> > I guess I'm a little surprised that we don't even seem to turn any of
> > this codec's regulators off in S3. That seems like it would be drawing
> > power that we don't want. Maybe the "low power" mode of the codec is
> > low enough and we need to avoid powering it off to avoid pops / hisses
> > in S3 or something? If that's true, this might be one of those places
> > where the "LPM" of the regulators might actually be useful...
>
> OK, fair enough, so suggestions on what I should do instead? Should I
> look at why or how to turn the regulators off? Should I look into LPM?
> Are there existing bugs for such work?

It would be interesting to know from Judy if there's a reason we never
turn the audio codec rails off. Maybe there's history that I'm not
aware of? Matthias or Mengqi might also have ideas of how much power
is at stake here?

Ah, searching through the ChromeOS bug tracker finds some hits. At
this point we should probably move the discussion off the lists and
bring it back to the lists when we have some results. Unfortunately
most of the bugs are not public and so having a discussion here is
just noise for most people CCed.

-Doug

Re: [PATCH] arm64: dts: qcom: sc7180: Add sleep state for alc5682 codec

[Joseph S. Barrera III]

On 8/18/22 3:39 PM, Doug Anderson wrote:
> Ah, searching through the ChromeOS bug tracker finds some hits. At
> this point we should probably move the discussion off the lists and
> bring it back to the lists when we have some results. Unfortunately
> most of the bugs are not public and so having a discussion here is
> just noise for most people CCed.

+1, thanks Doug