Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220818084216.1.I5c2b6fea19c4c0dec67fd4931f03df8e5ccaca95@changeid>
 <CAE-0n52GzxXEsToWzfU1TMuASuC6TKK7NXxYbBQWxNmM74FxZA@mail.gmail.com>
In-Reply-To: <CAE-0n52GzxXEsToWzfU1TMuASuC6TKK7NXxYbBQWxNmM74FxZA@mail.gmail.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Thu, 18 Aug 2022 13:34:39 -0700
Message-ID: <CAD=FV=WNuqtpnCr2Zn0z_L1OCiwD8dNzhDxvhfHYuYVmciPbuQ@mail.gmail.com>
Subject: Re: [PATCH] arm64: dts: qcom: sc7180: Add sleep state for alc5682 codec
To:     Stephen Boyd <swboyd@chromium.org>
Cc:     "Joseph S. Barrera III" <joebar@chromium.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        Alexandru Stan <amstan@chromium.org>,
        Judy Hsiao <judyhsiao@chromium.org>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Konrad Dybcio <konrad.dybcio@somainline.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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