Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
In-Reply-To: <YLhbpYq8/+JUlP27@google.com>
References: <20210510220012.2003285-1-swboyd@chromium.org> <YJnllh7GfuVlL3ze@google.com>
 <CAE-0n539o_DWqHbPuarWozk4Rev_d++2Da=AvOYALwvB1j3dVA@mail.gmail.com>
 <YLgt2ZJ6GZwUNL8T@google.com> <CAE-0n52S72vWZkzwva2_uqsMMdgdKbX7-MKtNE5PdaetyeqN2Q@mail.gmail.com>
 <YLhbpYq8/+JUlP27@google.com>
From:   Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.9.1
Date:   Wed, 2 Jun 2021 23:13:40 -0700
Message-ID: <CAE-0n50PZ+vUH_dSQnunViWrkEdQNHyLNHjKbrsbEbO-YL8DRA@mail.gmail.com>
Subject: Re: [PATCH] Input: elan_i2c: Disable irq on shutdown
To:     Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc:     linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
        Jingle Wu <jingle.wu@emc.com.tw>, Wolfram Sang <wsa@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Quoting Dmitry Torokhov (2021-06-02 21:33:41)
> On Wed, Jun 02, 2021 at 06:33:49PM -0700, Stephen Boyd wrote:
> > Quoting Dmitry Torokhov (2021-06-02 18:18:17)
> > >
> > > I do not think keeping counter balanced would be important here, as we
> > > are shutting down, and upon reboot everything will be reinitialized from
> > > scratch. Also, we are lucky in that there is just a handful of I2C
> > > drivers defining shutdown() methods.
> > >
> > > > Please don't make me shave this yak.
> > >
> > > I'm afraid someone has to... I'm adding Wolfram to CC to get his take on
> > > this.
> > >
> >
> > I suppose another option would be to introduce some common function that
> > i2c drivers can use for their shutdown op, like i2c_generic_shutdown()
> > that would disable the irq? I would guess that it isn't a great idea to
> > blanket disable the irq in case some i2c driver wants to do something
> > that may require that irq to come in once more during shutdown to signal
> > that things are off or something like that.
> >
> > Would having this common function that this driver opts into work for
> > you?
>
> Opting in in this fashion will still require changes in the majority
> of drivers (any I2C touchscreen or touchpad may be touched while system
> is being shut down, so all of them will need to have interrupt freed or
> disabled, or they may initiate I2C transfer). How about something like
> this;

Yes, this approach should work. I assume a device that doesn't have a
shutdown function will be happy to let the i2c core disable the irq for
it so it looks low risk.

Will you send a proper patch to Wolfram or would you like me to wrap it
up and resend?

>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 38107c0c318c..c835e7bb71de 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -603,9 +603,12 @@ static void i2c_device_shutdown(struct device *dev)
>
>         if (!client || !dev->driver)
>                 return;
> +
>         driver = to_i2c_driver(dev->driver);
>         if (driver->shutdown)
>                 driver->shutdown(client);
> +       else if (client->irq > 0)
> +               disable_irq(client->irq);
>  }
>
>  static void i2c_client_dev_release(struct device *dev)
>