Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 16 Feb 2022 15:45:51 +0800
From:   Tzung-Bi Shih <tzungbi@google.com>
To:     Prashant Malani <pmalani@chromium.org>
Cc:     bleung@chromium.org, groeck@chromium.org,
        chrome-platform@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 3/5] platform/chrome: cros_ec: initialize
 `wake_enabled` in cros_ec_register()
Message-ID: <Ygyrr1U9JbqHDSCK@google.com>
References: <20220216043639.3839185-1-tzungbi@google.com>
 <20220216043639.3839185-4-tzungbi@google.com>
 <CACeCKacKtJ50b2TuuhcVYPq0Fd0unVJdMuNDk1PLNK_r2JHZ+A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CACeCKacKtJ50b2TuuhcVYPq0Fd0unVJdMuNDk1PLNK_r2JHZ+A@mail.gmail.com>
Precedence: bulk

On Tue, Feb 15, 2022 at 09:47:09PM -0800, Prashant Malani wrote:
> On Tue, Feb 15, 2022 at 8:37 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
> >
> > `wake_enabled` indicates cros_ec_resume() needs to call
> > disable_irq_wake() to undo enable_irq_wake() in cros_ec_suspend().
> >
> > Initialize `wake_enabled` in cros_ec_register() and determine the flag
> > in cros_ec_suspend() instead of reset-after-used in cros_ec_resume().

After reconsidering the 2 options in [1], I feel the flag needs to be set in
cros_ec_suspend() just in case if someone changes the wakeup capability per
[2].

[1]: https://patchwork.kernel.org/project/chrome-platform/patch/20220209045035.380615-4-tzungbi@google.com/#24739778
[2]: https://patchwork.kernel.org/project/chrome-platform/patch/20220209045035.380615-4-tzungbi@google.com/#24740205

Will change it back in the next version, pardon me.

> > @@ -383,10 +384,9 @@ int cros_ec_resume(struct cros_ec_device *ec_dev)
> >                 dev_dbg(ec_dev->dev, "Error %d sending resume event to ec",
> >                         ret);
> >
> > -       if (ec_dev->wake_enabled) {
> > +       if (ec_dev->wake_enabled)
> >                 disable_irq_wake(ec_dev->irq);
> > -               ec_dev->wake_enabled = 0;
> > -       }
> > +
> 
> Better to leave it as is, and ensure "wake_enabled" is cleared after resume?
> Will result in a smaller diff.

No, cros_ec_suspend() uses the flag to tell cros_ec_resume(): don't forget to
call disable_irq_wake().  It shouldn't be reset after used by
cros_ec_resume().