From: Bastien Nocera <hadess@hadess.net>
To: Hans de Goede <hdegoede@redhat.com>,
Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-input@vger.kernel.org, Dmitry Mastykin <mastichi@gmail.com>
Subject: Re: [PATCH resend 01/10] Input: goodix - Refactor IRQ pin GPIO accesses
Date: Mon, 02 Mar 2020 12:09:55 +0100 [thread overview]
Message-ID: <1c57e9d567af8041fb8aff94d4095af0ff8b4484.camel@hadess.net> (raw)
In-Reply-To: <20200221164735.508324-1-hdegoede@redhat.com>
On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
> Suspending Goodix touchscreens requires changing the interrupt pin to
> output before sending them a power-down command. Followed by wiggling
> the interrupt pin to wake the device up, after which it is put back
> in input mode.
>
> So far we have only effectively supported this on devices which use
> devicetree. On X86 ACPI platforms both looking up the pins; and using
> a
> pin as both IRQ and GPIO is a bit more complicated. E.g. on some
> devices
> we cannot directly access the IRQ pin as GPIO and we need to call
> ACPI
> methods to control it instead.
>
> This commit adds a new irq_pin_access_method field to the
> goodix_chip_data
> struct and adds goodix_irq_direction_output and
> goodix_irq_direction_input
> helpers which together abstract the GPIO accesses to the IRQ pin.
>
> This is a preparation patch for adding support for properly
> suspending the
> touchscreen on X86 ACPI platforms.
>
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317
> BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207
> Cc: Dmitry Mastykin <mastichi@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/input/touchscreen/goodix.c | 62 ++++++++++++++++++++++++--
> ----
> 1 file changed, 51 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 0403102e807e..08806a00a9b9 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -31,6 +31,11 @@
>
> struct goodix_ts_data;
>
> +enum goodix_irq_pin_access_method {
> + irq_pin_access_none,
> + irq_pin_access_gpio,
I don't know if that matches the kernel coding style, but I'd rather
the enum member names were upper-case, and typedef'ed.
> +};
> +
> struct goodix_chip_data {
> u16 config_addr;
> int config_len;
> @@ -53,6 +58,7 @@ struct goodix_ts_data {
> const char *cfg_name;
> struct completion firmware_loading_complete;
> unsigned long irq_flags;
> + enum goodix_irq_pin_access_method irq_pin_access_method;
> unsigned int contact_size;
> };
>
> @@ -502,17 +508,48 @@ static int goodix_send_cfg(struct
> goodix_ts_data *ts,
> return 0;
> }
>
> +static int goodix_irq_direction_output(struct goodix_ts_data *ts,
> + int value)
> +{
> + switch (ts->irq_pin_access_method) {
> + case irq_pin_access_none:
> + dev_err(&ts->client->dev,
> + "%s called without an irq_pin_access_method
> set\n",
> + __func__);
> + return -EINVAL;
> + case irq_pin_access_gpio:
> + return gpiod_direction_output(ts->gpiod_int, value);
Is that going to complain about default not being handled? If so, an if
conditional might be enough.
> + }
> +
> + return -EINVAL; /* Never reached */
> +}
> +
> +static int goodix_irq_direction_input(struct goodix_ts_data *ts)
> +{
> + switch (ts->irq_pin_access_method) {
> + case irq_pin_access_none:
> + dev_err(&ts->client->dev,
> + "%s called without an irq_pin_access_method
> set\n",
> + __func__);
> + return -EINVAL;
> + case irq_pin_access_gpio:
> + return gpiod_direction_input(ts->gpiod_int);
Ditto.
Looks fine otherwise.
> + }
> +
> + return -EINVAL; /* Never reached */
> +}
> +
> static int goodix_int_sync(struct goodix_ts_data *ts)
> {
> int error;
>
> - error = gpiod_direction_output(ts->gpiod_int, 0);
> + error = goodix_irq_direction_output(ts, 0);
> if (error)
> return error;
>
> msleep(50); /* T5: 50ms */
>
> - error = gpiod_direction_input(ts->gpiod_int);
> + error = goodix_irq_direction_input(ts);
> if (error)
> return error;
>
> @@ -536,7 +573,7 @@ static int goodix_reset(struct goodix_ts_data
> *ts)
> msleep(20); /* T2: > 10ms */
>
> /* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
> - error = gpiod_direction_output(ts->gpiod_int, ts->client->addr
> == 0x14);
> + error = goodix_irq_direction_output(ts, ts->client->addr ==
> 0x14);
> if (error)
> return error;
>
> @@ -617,6 +654,9 @@ static int goodix_get_gpio_config(struct
> goodix_ts_data *ts)
>
> ts->gpiod_rst = gpiod;
>
> + if (ts->gpiod_int && ts->gpiod_rst)
> + ts->irq_pin_access_method = irq_pin_access_gpio;
> +
> return 0;
> }
>
> @@ -889,7 +929,7 @@ static int goodix_ts_probe(struct i2c_client
> *client,
> if (error)
> return error;
>
> - if (ts->gpiod_int && ts->gpiod_rst) {
> + if (ts->irq_pin_access_method == irq_pin_access_gpio) {
> /* reset the controller */
> error = goodix_reset(ts);
> if (error) {
> @@ -912,7 +952,7 @@ static int goodix_ts_probe(struct i2c_client
> *client,
>
> ts->chip = goodix_get_chip_data(ts->id);
>
> - if (ts->gpiod_int && ts->gpiod_rst) {
> + if (ts->irq_pin_access_method == irq_pin_access_gpio) {
> /* update device config */
> ts->cfg_name = devm_kasprintf(&client->dev, GFP_KERNEL,
> "goodix_%d_cfg.bin", ts-
> >id);
> @@ -943,7 +983,7 @@ static int goodix_ts_remove(struct i2c_client
> *client)
> {
> struct goodix_ts_data *ts = i2c_get_clientdata(client);
>
> - if (ts->gpiod_int && ts->gpiod_rst)
> + if (ts->irq_pin_access_method == irq_pin_access_gpio)
> wait_for_completion(&ts->firmware_loading_complete);
>
> return 0;
> @@ -956,7 +996,7 @@ static int __maybe_unused goodix_suspend(struct
> device *dev)
> int error;
>
> /* We need gpio pins to suspend/resume */
> - if (!ts->gpiod_int || !ts->gpiod_rst) {
> + if (ts->irq_pin_access_method == irq_pin_access_none) {
> disable_irq(client->irq);
> return 0;
> }
> @@ -967,7 +1007,7 @@ static int __maybe_unused goodix_suspend(struct
> device *dev)
> goodix_free_irq(ts);
>
> /* Output LOW on the INT pin for 5 ms */
> - error = gpiod_direction_output(ts->gpiod_int, 0);
> + error = goodix_irq_direction_output(ts, 0);
> if (error) {
> goodix_request_irq(ts);
> return error;
> @@ -979,7 +1019,7 @@ static int __maybe_unused goodix_suspend(struct
> device *dev)
> GOODIX_CMD_SCREEN_OFF);
> if (error) {
> dev_err(&ts->client->dev, "Screen off command
> failed\n");
> - gpiod_direction_input(ts->gpiod_int);
> + goodix_irq_direction_input(ts);
> goodix_request_irq(ts);
> return -EAGAIN;
> }
> @@ -999,7 +1039,7 @@ static int __maybe_unused goodix_resume(struct
> device *dev)
> struct goodix_ts_data *ts = i2c_get_clientdata(client);
> int error;
>
> - if (!ts->gpiod_int || !ts->gpiod_rst) {
> + if (ts->irq_pin_access_method == irq_pin_access_none) {
> enable_irq(client->irq);
> return 0;
> }
> @@ -1008,7 +1048,7 @@ static int __maybe_unused goodix_resume(struct
> device *dev)
> * Exit sleep mode by outputting HIGH level to INT pin
> * for 2ms~5ms.
> */
> - error = gpiod_direction_output(ts->gpiod_int, 1);
> + error = goodix_irq_direction_output(ts, 1);
> if (error)
> return error;
>
next prev parent reply other threads:[~2020-03-02 11:10 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-21 16:47 [PATCH resend 01/10] Input: goodix - Refactor IRQ pin GPIO accesses Hans de Goede
2020-02-21 16:47 ` [PATCH resend 02/10] Input: goodix - Make loading the config from disk independent from the GPIO setup Hans de Goede
2020-03-02 11:12 ` Bastien Nocera
2020-02-21 16:47 ` [PATCH resend 03/10] Input: goodix - Make resetting the controller at probe " Hans de Goede
2020-03-02 11:14 ` Bastien Nocera
2020-02-21 16:47 ` [PATCH resend 04/10] Input: goodix - Add support for getting IRQ + reset GPIOs on Cherry Trail devices Hans de Goede
2020-03-02 11:23 ` Bastien Nocera
2020-03-02 15:40 ` Hans de Goede
2020-03-02 15:44 ` Bastien Nocera
2020-03-02 16:23 ` Hans de Goede
2020-02-21 16:47 ` [PATCH resend 05/10] Input: goodix - Add support for getting IRQ + reset GPIOs on Bay " Hans de Goede
2020-03-02 11:24 ` Bastien Nocera
2020-02-21 16:47 ` [PATCH resend 06/10] Input: goodix - Add support for controlling the IRQ pin through ACPI methods Hans de Goede
2020-03-02 11:25 ` Bastien Nocera
2020-02-21 16:47 ` [PATCH resend 07/10] Input: goodix - Move defines to above struct goodix_ts_data declaration Hans de Goede
2020-03-02 11:25 ` Bastien Nocera
2020-02-21 16:47 ` [PATCH resend 08/10] Input: goodix - Save a copy of the config from goodix_read_config() Hans de Goede
2020-03-02 11:30 ` Bastien Nocera
2020-03-02 18:36 ` Hans de Goede
2020-02-21 16:47 ` [PATCH resend 09/10] Input: goodix - Make goodix_send_cfg() take a raw buffer as argument Hans de Goede
2020-03-02 11:33 ` Bastien Nocera
2020-03-02 18:53 ` Hans de Goede
2020-02-21 16:47 ` [PATCH resend 10/10] Input: goodix - Restore config on resume if necessary Hans de Goede
2020-03-02 11:35 ` Bastien Nocera
2020-03-02 19:08 ` Hans de Goede
2020-03-02 11:09 ` Bastien Nocera [this message]
2020-03-02 13:23 ` [PATCH resend 01/10] Input: goodix - Refactor IRQ pin GPIO accesses Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1c57e9d567af8041fb8aff94d4095af0ff8b4484.camel@hadess.net \
--to=hadess@hadess.net \
--cc=dmitry.torokhov@gmail.com \
--cc=hdegoede@redhat.com \
--cc=linux-input@vger.kernel.org \
--cc=mastichi@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).