Re: [PATCH v4 3/5] platform/chrome: cros_ec: initialize `wake_enabled` in cros_ec_register()

[Prashant Malani]

On Tue, Feb 15, 2022 at 8:37 PM Tzung-Bi Shih <tzungbi@xxxxxxxxxx> 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().
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@xxxxxxxxxx>

One minor thing to consider, but regardless:

Reviewed-by: Prashant Malani <pmalani@xxxxxxxxxxxx>

> ---
> Changes from v3:
> (https://patchwork.kernel.org/project/chrome-platform/patch/20220209095703.517608-4-tzungbi@xxxxxxxxxx/)
> - Change the patch title.
> - Simplify by initializing wake_enabled in cros_ec_register().
>
> No changes from v2.
>
> Changes from v1:
> (https://lore.kernel.org/lkml/20220125101527.1812887-1-tzungbi@xxxxxxxxxx/T/#u)
> - Use imperative mood in commit message.
>
>  drivers/platform/chrome/cros_ec.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
> index 25cd8df6e7b0..b6604a9ab315 100644
> --- a/drivers/platform/chrome/cros_ec.c
> +++ b/drivers/platform/chrome/cros_ec.c
> @@ -191,6 +191,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
>         ec_dev->max_passthru = 0;
>         ec_dev->ec = NULL;
>         ec_dev->pd = NULL;
> +       ec_dev->wake_enabled = false;
>
>         ec_dev->din = devm_kzalloc(dev, ec_dev->din_size, GFP_KERNEL);
>         if (!ec_dev->din)
> @@ -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.

I'll leave it up to you.

-Prashant