* [PATCH] input: goodix: add poll mode for goodix touchscreen
@ 2025-05-19 8:57 Joseph Guo
2025-05-19 16:15 ` Hans de Goede
2025-05-19 16:23 ` Dmitry Torokhov
0 siblings, 2 replies; 6+ messages in thread
From: Joseph Guo @ 2025-05-19 8:57 UTC (permalink / raw)
To: Bastien Nocera, Hans de Goede, Dmitry Torokhov,
open list:GOODIX TOUCHSCREEN, open list
Cc: qijian.guo
goodix touchscreen only support interrupt mode by default.
Some panels like waveshare panel which is widely used on raspeberry pi
don't have interrupt pins and only work on i2c poll mode.
The waveshare panel 7inch panel use goodix gt911 touchscreen chip.
Update goodix touchscreen to support both interrupt and poll mode.
Signed-off-by: Joseph Guo <qijian.guo@nxp.com>
---
drivers/input/touchscreen/goodix.c | 69 +++++++++++++++++++++++++++---
drivers/input/touchscreen/goodix.h | 4 ++
2 files changed, 67 insertions(+), 6 deletions(-)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index aaf79ac50004..87991b56494d 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -47,6 +47,7 @@
#define RESOLUTION_LOC 1
#define MAX_CONTACTS_LOC 5
#define TRIGGER_LOC 6
+#define POLL_INTERVAL_MS 17 /* 17ms = 60fps */
/* Our special handling for GPIO accesses through ACPI is x86 specific */
#if defined CONFIG_X86 && defined CONFIG_ACPI
@@ -497,6 +498,23 @@ static void goodix_process_events(struct goodix_ts_data *ts)
input_sync(ts->input_dev);
}
+static void goodix_ts_irq_poll_timer(struct timer_list *t)
+{
+ struct goodix_ts_data *ts = from_timer(ts, t, timer);
+
+ schedule_work(&ts->work_i2c_poll);
+ mod_timer(&ts->timer, jiffies + msecs_to_jiffies(POLL_INTERVAL_MS));
+}
+
+static void goodix_ts_work_i2c_poll(struct work_struct *work)
+{
+ struct goodix_ts_data *ts = container_of(work,
+ struct goodix_ts_data, work_i2c_poll);
+
+ goodix_process_events(ts);
+ goodix_i2c_write_u8(ts->client, GOODIX_READ_COOR_ADDR, 0);
+}
+
/**
* goodix_ts_irq_handler - The IRQ handler
*
@@ -523,16 +541,50 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
return IRQ_HANDLED;
}
+static void goodix_enable_irq(struct goodix_ts_data *ts)
+{
+ if (ts->client->irq) {
+ enable_irq(ts->client->irq);
+ } else {
+ ts->timer.expires = jiffies + msecs_to_jiffies(POLL_INTERVAL_MS);
+ add_timer(&ts->timer);
+ }
+}
+
+static void goodix_disable_irq(struct goodix_ts_data *ts)
+{
+ if (ts->client->irq) {
+ disable_irq(ts->client->irq);
+ } else {
+ del_timer(&ts->timer);
+ cancel_work_sync(&ts->work_i2c_poll);
+ }
+}
+
static void goodix_free_irq(struct goodix_ts_data *ts)
{
- devm_free_irq(&ts->client->dev, ts->client->irq, ts);
+ if (ts->client->irq) {
+ devm_free_irq(&ts->client->dev, ts->client->irq, ts);
+ } else {
+ del_timer(&ts->timer);
+ cancel_work_sync(&ts->work_i2c_poll);
+ }
}
static int goodix_request_irq(struct goodix_ts_data *ts)
{
- return devm_request_threaded_irq(&ts->client->dev, ts->client->irq,
- NULL, goodix_ts_irq_handler,
- ts->irq_flags, ts->client->name, ts);
+ if (ts->client->irq) {
+ return devm_request_threaded_irq(&ts->client->dev, ts->client->irq,
+ NULL, goodix_ts_irq_handler,
+ ts->irq_flags, ts->client->name, ts);
+ } else {
+ INIT_WORK(&ts->work_i2c_poll,
+ goodix_ts_work_i2c_poll);
+ timer_setup(&ts->timer, goodix_ts_irq_poll_timer, 0);
+ if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE)
+ goodix_enable_irq(ts);
+ return 0;
+ }
}
static int goodix_check_cfg_8(struct goodix_ts_data *ts, const u8 *cfg, int len)
@@ -1420,6 +1472,11 @@ static void goodix_ts_remove(struct i2c_client *client)
{
struct goodix_ts_data *ts = i2c_get_clientdata(client);
+ if (!client->irq) {
+ del_timer(&ts->timer);
+ cancel_work_sync(&ts->work_i2c_poll);
+ }
+
if (ts->load_cfg_from_disk)
wait_for_completion(&ts->firmware_loading_complete);
}
@@ -1435,7 +1492,7 @@ static int goodix_suspend(struct device *dev)
/* We need gpio pins to suspend/resume */
if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
- disable_irq(client->irq);
+ goodix_disable_irq(ts);
return 0;
}
@@ -1479,7 +1536,7 @@ static int goodix_resume(struct device *dev)
int error;
if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
- enable_irq(client->irq);
+ goodix_enable_irq(ts);
return 0;
}
diff --git a/drivers/input/touchscreen/goodix.h b/drivers/input/touchscreen/goodix.h
index 87797cc88b32..132e49d66324 100644
--- a/drivers/input/touchscreen/goodix.h
+++ b/drivers/input/touchscreen/goodix.h
@@ -56,6 +56,7 @@
#define GOODIX_ID_MAX_LEN 4
#define GOODIX_CONFIG_MAX_LENGTH 240
#define GOODIX_MAX_KEYS 7
+#define GOODIX_NAME_MAX_LEN 38
enum goodix_irq_pin_access_method {
IRQ_PIN_ACCESS_NONE,
@@ -91,6 +92,7 @@ struct goodix_ts_data {
enum gpiod_flags gpiod_rst_flags;
char id[GOODIX_ID_MAX_LEN + 1];
char cfg_name[64];
+ char name[GOODIX_NAME_MAX_LEN];
u16 version;
bool reset_controller_at_probe;
bool load_cfg_from_disk;
@@ -104,6 +106,8 @@ struct goodix_ts_data {
u8 main_clk[GOODIX_MAIN_CLK_LEN];
int bak_ref_len;
u8 *bak_ref;
+ struct timer_list timer;
+ struct work_struct work_i2c_poll;
};
int goodix_i2c_read(struct i2c_client *client, u16 reg, u8 *buf, int len);
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] input: goodix: add poll mode for goodix touchscreen
2025-05-19 8:57 Joseph Guo
@ 2025-05-19 16:15 ` Hans de Goede
2025-05-19 16:23 ` Dmitry Torokhov
1 sibling, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2025-05-19 16:15 UTC (permalink / raw)
To: Joseph Guo, Bastien Nocera, Dmitry Torokhov,
open list:GOODIX TOUCHSCREEN, open list
Hi,
On 19-May-25 10:57 AM, Joseph Guo wrote:
> goodix touchscreen only support interrupt mode by default.
> Some panels like waveshare panel which is widely used on raspeberry pi
> don't have interrupt pins and only work on i2c poll mode.
> The waveshare panel 7inch panel use goodix gt911 touchscreen chip.
>
> Update goodix touchscreen to support both interrupt and poll mode.
>
> Signed-off-by: Joseph Guo <qijian.guo@nxp.com>
> ---
> drivers/input/touchscreen/goodix.c | 69 +++++++++++++++++++++++++++---
> drivers/input/touchscreen/goodix.h | 4 ++
> 2 files changed, 67 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index aaf79ac50004..87991b56494d 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -47,6 +47,7 @@
> #define RESOLUTION_LOC 1
> #define MAX_CONTACTS_LOC 5
> #define TRIGGER_LOC 6
> +#define POLL_INTERVAL_MS 17 /* 17ms = 60fps */
>
> /* Our special handling for GPIO accesses through ACPI is x86 specific */
> #if defined CONFIG_X86 && defined CONFIG_ACPI
> @@ -497,6 +498,23 @@ static void goodix_process_events(struct goodix_ts_data *ts)
> input_sync(ts->input_dev);
> }
>
> +static void goodix_ts_irq_poll_timer(struct timer_list *t)
> +{
> + struct goodix_ts_data *ts = from_timer(ts, t, timer);
> +
> + schedule_work(&ts->work_i2c_poll);
> + mod_timer(&ts->timer, jiffies + msecs_to_jiffies(POLL_INTERVAL_MS));
> +}
> +
> +static void goodix_ts_work_i2c_poll(struct work_struct *work)
> +{
> + struct goodix_ts_data *ts = container_of(work,
> + struct goodix_ts_data, work_i2c_poll);
> +
> + goodix_process_events(ts);
> + goodix_i2c_write_u8(ts->client, GOODIX_READ_COOR_ADDR, 0);
> +}
> +
> /**
> * goodix_ts_irq_handler - The IRQ handler
> *
> @@ -523,16 +541,50 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> +static void goodix_enable_irq(struct goodix_ts_data *ts)
> +{
> + if (ts->client->irq) {
> + enable_irq(ts->client->irq);
> + } else {
> + ts->timer.expires = jiffies + msecs_to_jiffies(POLL_INTERVAL_MS);
> + add_timer(&ts->timer);
> + }
> +}
> +
> +static void goodix_disable_irq(struct goodix_ts_data *ts)
> +{
> + if (ts->client->irq) {
> + disable_irq(ts->client->irq);
> + } else {
> + del_timer(&ts->timer);
> + cancel_work_sync(&ts->work_i2c_poll);
> + }
> +}
> +
> static void goodix_free_irq(struct goodix_ts_data *ts)
> {
> - devm_free_irq(&ts->client->dev, ts->client->irq, ts);
> + if (ts->client->irq) {
> + devm_free_irq(&ts->client->dev, ts->client->irq, ts);
> + } else {
> + del_timer(&ts->timer);
> + cancel_work_sync(&ts->work_i2c_poll);
> + }
> }
>
> static int goodix_request_irq(struct goodix_ts_data *ts)
> {
> - return devm_request_threaded_irq(&ts->client->dev, ts->client->irq,
> - NULL, goodix_ts_irq_handler,
> - ts->irq_flags, ts->client->name, ts);
> + if (ts->client->irq) {
> + return devm_request_threaded_irq(&ts->client->dev, ts->client->irq,
> + NULL, goodix_ts_irq_handler,
> + ts->irq_flags, ts->client->name, ts);
> + } else {
> + INIT_WORK(&ts->work_i2c_poll,
> + goodix_ts_work_i2c_poll);
> + timer_setup(&ts->timer, goodix_ts_irq_poll_timer, 0);
> + if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE)
> + goodix_enable_irq(ts);
> + return 0;
> + }
> }
>
> static int goodix_check_cfg_8(struct goodix_ts_data *ts, const u8 *cfg, int len)
> @@ -1420,6 +1472,11 @@ static void goodix_ts_remove(struct i2c_client *client)
> {
> struct goodix_ts_data *ts = i2c_get_clientdata(client);
>
> + if (!client->irq) {
> + del_timer(&ts->timer);
> + cancel_work_sync(&ts->work_i2c_poll);
> + }
> +
> if (ts->load_cfg_from_disk)
> wait_for_completion(&ts->firmware_loading_complete);
> }
> @@ -1435,7 +1492,7 @@ static int goodix_suspend(struct device *dev)
>
> /* We need gpio pins to suspend/resume */
> if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
> - disable_irq(client->irq);
> + goodix_disable_irq(ts);
> return 0;
> }
>
> @@ -1479,7 +1536,7 @@ static int goodix_resume(struct device *dev)
> int error;
>
> if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
> - enable_irq(client->irq);
> + goodix_enable_irq(ts);
> return 0;
> }
>
> diff --git a/drivers/input/touchscreen/goodix.h b/drivers/input/touchscreen/goodix.h
> index 87797cc88b32..132e49d66324 100644
> --- a/drivers/input/touchscreen/goodix.h
> +++ b/drivers/input/touchscreen/goodix.h
> @@ -56,6 +56,7 @@
> #define GOODIX_ID_MAX_LEN 4
> #define GOODIX_CONFIG_MAX_LENGTH 240
> #define GOODIX_MAX_KEYS 7
> +#define GOODIX_NAME_MAX_LEN 38
>
> enum goodix_irq_pin_access_method {
> IRQ_PIN_ACCESS_NONE,
> @@ -91,6 +92,7 @@ struct goodix_ts_data {
> enum gpiod_flags gpiod_rst_flags;
> char id[GOODIX_ID_MAX_LEN + 1];
> char cfg_name[64];
> + char name[GOODIX_NAME_MAX_LEN];
This adding of the name variable, seems to be some leftover change from another patch?
Please drop this for v2.
Regards,
Hans
> u16 version;
> bool reset_controller_at_probe;
> bool load_cfg_from_disk;
> @@ -104,6 +106,8 @@ struct goodix_ts_data {
> u8 main_clk[GOODIX_MAIN_CLK_LEN];
> int bak_ref_len;
> u8 *bak_ref;
> + struct timer_list timer;
> + struct work_struct work_i2c_poll;
> };
>
> int goodix_i2c_read(struct i2c_client *client, u16 reg, u8 *buf, int len);
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] input: goodix: add poll mode for goodix touchscreen
2025-05-19 8:57 Joseph Guo
2025-05-19 16:15 ` Hans de Goede
@ 2025-05-19 16:23 ` Dmitry Torokhov
1 sibling, 0 replies; 6+ messages in thread
From: Dmitry Torokhov @ 2025-05-19 16:23 UTC (permalink / raw)
To: Joseph Guo
Cc: Bastien Nocera, Hans de Goede, open list:GOODIX TOUCHSCREEN,
open list
Hi Joseph,
On Mon, May 19, 2025 at 05:57:43PM +0900, Joseph Guo wrote:
> goodix touchscreen only support interrupt mode by default.
> Some panels like waveshare panel which is widely used on raspeberry pi
> don't have interrupt pins and only work on i2c poll mode.
> The waveshare panel 7inch panel use goodix gt911 touchscreen chip.
>
> Update goodix touchscreen to support both interrupt and poll mode.
>
> Signed-off-by: Joseph Guo <qijian.guo@nxp.com>
> ---
> drivers/input/touchscreen/goodix.c | 69 +++++++++++++++++++++++++++---
> drivers/input/touchscreen/goodix.h | 4 ++
> 2 files changed, 67 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index aaf79ac50004..87991b56494d 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -47,6 +47,7 @@
> #define RESOLUTION_LOC 1
> #define MAX_CONTACTS_LOC 5
> #define TRIGGER_LOC 6
> +#define POLL_INTERVAL_MS 17 /* 17ms = 60fps */
>
> /* Our special handling for GPIO accesses through ACPI is x86 specific */
> #if defined CONFIG_X86 && defined CONFIG_ACPI
> @@ -497,6 +498,23 @@ static void goodix_process_events(struct goodix_ts_data *ts)
> input_sync(ts->input_dev);
> }
>
> +static void goodix_ts_irq_poll_timer(struct timer_list *t)
> +{
> + struct goodix_ts_data *ts = from_timer(ts, t, timer);
> +
> + schedule_work(&ts->work_i2c_poll);
> + mod_timer(&ts->timer, jiffies + msecs_to_jiffies(POLL_INTERVAL_MS));
> +}
Why are you not suing the existing polling infrastructure
(input_setup_polling())?
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] input: goodix: add poll mode for goodix touchscreen
@ 2025-05-21 2:50 Joseph Guo
2025-05-21 9:20 ` Hans de Goede
0 siblings, 1 reply; 6+ messages in thread
From: Joseph Guo @ 2025-05-21 2:50 UTC (permalink / raw)
To: Bastien Nocera, Hans de Goede, Dmitry Torokhov,
open list:GOODIX TOUCHSCREEN, open list
Cc: qijian.guo, haibo.chen, justin.jiang
goodix touchscreen only support interrupt mode by default.
Some panels like waveshare panel which is widely used on raspeberry pi
don't have interrupt pins and only work on i2c poll mode.
The waveshare panel 7inch panel use goodix gt911 touchscreen chip.
Signed-off-by: Joseph Guo <qijian.guo@nxp.com>
Reviewed-by: Haibo Chen <haibo.chen@nxp.com>
---
Change from v1 to v2
- Remove unused variable in goodix_ts_data struct
- Use polling infrastructure
---
drivers/input/touchscreen/goodix.c | 50 ++++++++++++++++++++++++++----
1 file changed, 44 insertions(+), 6 deletions(-)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index aaf79ac50004..58e49e5cf148 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -47,6 +47,7 @@
#define RESOLUTION_LOC 1
#define MAX_CONTACTS_LOC 5
#define TRIGGER_LOC 6
+#define GOODIX_POLL_INTERVAL_MS 17 /* 17ms = 60fps */
/* Our special handling for GPIO accesses through ACPI is x86 specific */
#if defined CONFIG_X86 && defined CONFIG_ACPI
@@ -497,6 +498,14 @@ static void goodix_process_events(struct goodix_ts_data *ts)
input_sync(ts->input_dev);
}
+static void goodix_ts_work_i2c_poll(struct input_dev *input)
+{
+ struct goodix_ts_data *ts = input_get_drvdata(input);
+
+ goodix_process_events(ts);
+ goodix_i2c_write_u8(ts->client, GOODIX_READ_COOR_ADDR, 0);
+}
+
/**
* goodix_ts_irq_handler - The IRQ handler
*
@@ -523,16 +532,33 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
return IRQ_HANDLED;
}
+static void goodix_enable_irq(struct goodix_ts_data *ts)
+{
+ if (ts->client->irq)
+ enable_irq(ts->client->irq);
+}
+
+static void goodix_disable_irq(struct goodix_ts_data *ts)
+{
+ if (ts->client->irq)
+ disable_irq(ts->client->irq);
+}
+
static void goodix_free_irq(struct goodix_ts_data *ts)
{
- devm_free_irq(&ts->client->dev, ts->client->irq, ts);
+ if (ts->client->irq)
+ devm_free_irq(&ts->client->dev, ts->client->irq, ts);
}
static int goodix_request_irq(struct goodix_ts_data *ts)
{
- return devm_request_threaded_irq(&ts->client->dev, ts->client->irq,
- NULL, goodix_ts_irq_handler,
- ts->irq_flags, ts->client->name, ts);
+ if (ts->client->irq) {
+ return devm_request_threaded_irq(&ts->client->dev, ts->client->irq,
+ NULL, goodix_ts_irq_handler,
+ ts->irq_flags, ts->client->name, ts);
+ }
+ else
+ return 0;
}
static int goodix_check_cfg_8(struct goodix_ts_data *ts, const u8 *cfg, int len)
@@ -1229,6 +1255,18 @@ static int goodix_configure_dev(struct goodix_ts_data *ts)
return error;
}
+ input_set_drvdata(ts->input_dev, ts);
+
+ if (!ts->client->irq) {
+ error = input_setup_polling(ts->input_dev, goodix_ts_work_i2c_poll);
+ if (error) {
+ dev_err(&ts->client->dev,
+ "could not set up polling mode, %d\n", error);
+ return error;
+ }
+ input_set_poll_interval(ts->input_dev, GOODIX_POLL_INTERVAL_MS);
+ }
+
error = input_register_device(ts->input_dev);
if (error) {
dev_err(&ts->client->dev,
@@ -1435,7 +1473,7 @@ static int goodix_suspend(struct device *dev)
/* We need gpio pins to suspend/resume */
if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
- disable_irq(client->irq);
+ goodix_disable_irq(ts);
return 0;
}
@@ -1479,7 +1517,7 @@ static int goodix_resume(struct device *dev)
int error;
if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
- enable_irq(client->irq);
+ goodix_enable_irq(ts);
return 0;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] input: goodix: add poll mode for goodix touchscreen
2025-05-21 2:50 [PATCH] input: goodix: add poll mode for goodix touchscreen Joseph Guo
@ 2025-05-21 9:20 ` Hans de Goede
2025-05-22 2:05 ` 回复: [EXT] " Joseph Guo
0 siblings, 1 reply; 6+ messages in thread
From: Hans de Goede @ 2025-05-21 9:20 UTC (permalink / raw)
To: Joseph Guo, Bastien Nocera, Dmitry Torokhov,
open list:GOODIX TOUCHSCREEN, open list
Cc: haibo.chen, justin.jiang
Hi Joseph,
Thank you for the new version.
For the next version, the subject here should have been "[PATCH v2] input: ...".
You can do this by doing e.g. :
git format-patch -v2 HEAD~
git send-email v2-0001-....patch
On 21-May-25 4:50 AM, Joseph Guo wrote:
> goodix touchscreen only support interrupt mode by default.
> Some panels like waveshare panel which is widely used on raspeberry pi
> don't have interrupt pins and only work on i2c poll mode.
> The waveshare panel 7inch panel use goodix gt911 touchscreen chip.
>
> Signed-off-by: Joseph Guo <qijian.guo@nxp.com>
> Reviewed-by: Haibo Chen <haibo.chen@nxp.com>
> ---
> Change from v1 to v2
> - Remove unused variable in goodix_ts_data struct
> - Use polling infrastructure
> ---
> drivers/input/touchscreen/goodix.c | 50 ++++++++++++++++++++++++++----
> 1 file changed, 44 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index aaf79ac50004..58e49e5cf148 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -47,6 +47,7 @@
> #define RESOLUTION_LOC 1
> #define MAX_CONTACTS_LOC 5
> #define TRIGGER_LOC 6
> +#define GOODIX_POLL_INTERVAL_MS 17 /* 17ms = 60fps */
>
> /* Our special handling for GPIO accesses through ACPI is x86 specific */
> #if defined CONFIG_X86 && defined CONFIG_ACPI
> @@ -497,6 +498,14 @@ static void goodix_process_events(struct goodix_ts_data *ts)
> input_sync(ts->input_dev);
> }
>
> +static void goodix_ts_work_i2c_poll(struct input_dev *input)
> +{
> + struct goodix_ts_data *ts = input_get_drvdata(input);
> +
> + goodix_process_events(ts);
> + goodix_i2c_write_u8(ts->client, GOODIX_READ_COOR_ADDR, 0);
> +}
> +
> /**
> * goodix_ts_irq_handler - The IRQ handler
> *
> @@ -523,16 +532,33 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> +static void goodix_enable_irq(struct goodix_ts_data *ts)
> +{
> + if (ts->client->irq)
> + enable_irq(ts->client->irq);
> +}
> +
> +static void goodix_disable_irq(struct goodix_ts_data *ts)
> +{
> + if (ts->client->irq)
> + disable_irq(ts->client->irq);
> +}
> +
> static void goodix_free_irq(struct goodix_ts_data *ts)
> {
> - devm_free_irq(&ts->client->dev, ts->client->irq, ts);
> + if (ts->client->irq)
> + devm_free_irq(&ts->client->dev, ts->client->irq, ts);
> }
>
> static int goodix_request_irq(struct goodix_ts_data *ts)
> {
> - return devm_request_threaded_irq(&ts->client->dev, ts->client->irq,
> - NULL, goodix_ts_irq_handler,
> - ts->irq_flags, ts->client->name, ts);
> + if (ts->client->irq) {
> + return devm_request_threaded_irq(&ts->client->dev, ts->client->irq,
> + NULL, goodix_ts_irq_handler,
> + ts->irq_flags, ts->client->name, ts);
> + }
> + else
> + return 0;
> }
The '}' here is wrongly indented, also if you use {}, which is not strictly
necessary here, please use them in both branches of the if.
Since in this function we're dealing with a multi-line statement I think it
might be better to write this as:
static int goodix_request_irq(struct goodix_ts_data *ts)
{
if (!ts->client->irq)
return 0;
return devm_request_threaded_irq(&ts->client->dev, ts->client->irq,
NULL, goodix_ts_irq_handler,
ts->irq_flags, ts->client->name, ts);
}
This will also make the diff smaller.
Or you can keep what you have and drop the {}.
Regards,
Hans
> static int goodix_check_cfg_8(struct goodix_ts_data *ts, const u8 *cfg, int len)
> @@ -1229,6 +1255,18 @@ static int goodix_configure_dev(struct goodix_ts_data *ts)
> return error;
> }
>
> + input_set_drvdata(ts->input_dev, ts);
> +
> + if (!ts->client->irq) {
> + error = input_setup_polling(ts->input_dev, goodix_ts_work_i2c_poll);
> + if (error) {
> + dev_err(&ts->client->dev,
> + "could not set up polling mode, %d\n", error);
> + return error;
> + }
> + input_set_poll_interval(ts->input_dev, GOODIX_POLL_INTERVAL_MS);
> + }
> +
> error = input_register_device(ts->input_dev);
> if (error) {
> dev_err(&ts->client->dev,
> @@ -1435,7 +1473,7 @@ static int goodix_suspend(struct device *dev)
>
> /* We need gpio pins to suspend/resume */
> if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
> - disable_irq(client->irq);
> + goodix_disable_irq(ts);
> return 0;
> }
>
> @@ -1479,7 +1517,7 @@ static int goodix_resume(struct device *dev)
> int error;
>
> if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
> - enable_irq(client->irq);
> + goodix_enable_irq(ts);
> return 0;
> }
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* 回复: [EXT] Re: [PATCH] input: goodix: add poll mode for goodix touchscreen
2025-05-21 9:20 ` Hans de Goede
@ 2025-05-22 2:05 ` Joseph Guo
0 siblings, 0 replies; 6+ messages in thread
From: Joseph Guo @ 2025-05-22 2:05 UTC (permalink / raw)
To: Hans de Goede, Bastien Nocera, Dmitry Torokhov,
open list:GOODIX TOUCHSCREEN, open list
Cc: Bough Chen, Justin Jiang
Hi Hans,
Thanks for your great comment. I already made the change in v3.
Best Regards.
Joseph
-----邮件原件-----
发件人: Hans de Goede <hdegoede@redhat.com>
发送时间: Wednesday, May 21, 2025 5:20 PM
收件人: Joseph Guo <qijian.guo@nxp.com>; Bastien Nocera <hadess@hadess.net>; Dmitry Torokhov <dmitry.torokhov@gmail.com>; open list:GOODIX TOUCHSCREEN <linux-input@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>
抄送: Bough Chen <haibo.chen@nxp.com>; Justin Jiang <justin.jiang@nxp.com>
主题: [EXT] Re: [PATCH] input: goodix: add poll mode for goodix touchscreen
Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
Hi Joseph,
Thank you for the new version.
For the next version, the subject here should have been "[PATCH v2] input: ...".
You can do this by doing e.g. :
git format-patch -v2 HEAD~
git send-email v2-0001-....patch
On 21-May-25 4:50 AM, Joseph Guo wrote:
> goodix touchscreen only support interrupt mode by default.
> Some panels like waveshare panel which is widely used on raspeberry pi
> don't have interrupt pins and only work on i2c poll mode.
> The waveshare panel 7inch panel use goodix gt911 touchscreen chip.
>
> Signed-off-by: Joseph Guo <qijian.guo@nxp.com>
> Reviewed-by: Haibo Chen <haibo.chen@nxp.com>
> ---
> Change from v1 to v2
> - Remove unused variable in goodix_ts_data struct
> - Use polling infrastructure
> ---
> drivers/input/touchscreen/goodix.c | 50
> ++++++++++++++++++++++++++----
> 1 file changed, 44 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index aaf79ac50004..58e49e5cf148 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -47,6 +47,7 @@
> #define RESOLUTION_LOC 1
> #define MAX_CONTACTS_LOC 5
> #define TRIGGER_LOC 6
> +#define GOODIX_POLL_INTERVAL_MS 17 /* 17ms = 60fps */
>
> /* Our special handling for GPIO accesses through ACPI is x86
> specific */ #if defined CONFIG_X86 && defined CONFIG_ACPI @@ -497,6
> +498,14 @@ static void goodix_process_events(struct goodix_ts_data *ts)
> input_sync(ts->input_dev);
> }
>
> +static void goodix_ts_work_i2c_poll(struct input_dev *input) {
> + struct goodix_ts_data *ts = input_get_drvdata(input);
> +
> + goodix_process_events(ts);
> + goodix_i2c_write_u8(ts->client, GOODIX_READ_COOR_ADDR, 0); }
> +
> /**
> * goodix_ts_irq_handler - The IRQ handler
> *
> @@ -523,16 +532,33 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> +static void goodix_enable_irq(struct goodix_ts_data *ts) {
> + if (ts->client->irq)
> + enable_irq(ts->client->irq); }
> +
> +static void goodix_disable_irq(struct goodix_ts_data *ts) {
> + if (ts->client->irq)
> + disable_irq(ts->client->irq); }
> +
> static void goodix_free_irq(struct goodix_ts_data *ts) {
> - devm_free_irq(&ts->client->dev, ts->client->irq, ts);
> + if (ts->client->irq)
> + devm_free_irq(&ts->client->dev, ts->client->irq, ts);
> }
>
> static int goodix_request_irq(struct goodix_ts_data *ts) {
> - return devm_request_threaded_irq(&ts->client->dev, ts->client->irq,
> - NULL, goodix_ts_irq_handler,
> - ts->irq_flags, ts->client->name, ts);
> + if (ts->client->irq) {
> + return devm_request_threaded_irq(&ts->client->dev, ts->client->irq,
> + NULL, goodix_ts_irq_handler,
> + ts->irq_flags, ts->client->name, ts);
> + }
> + else
> + return 0;
> }
The '}' here is wrongly indented, also if you use {}, which is not strictly necessary here, please use them in both branches of the if.
Since in this function we're dealing with a multi-line statement I think it might be better to write this as:
static int goodix_request_irq(struct goodix_ts_data *ts) {
if (!ts->client->irq)
return 0;
return devm_request_threaded_irq(&ts->client->dev, ts->client->irq,
NULL, goodix_ts_irq_handler,
ts->irq_flags, ts->client->name, ts); }
This will also make the diff smaller.
Or you can keep what you have and drop the {}.
Regards,
Hans
> static int goodix_check_cfg_8(struct goodix_ts_data *ts, const u8
> *cfg, int len) @@ -1229,6 +1255,18 @@ static int goodix_configure_dev(struct goodix_ts_data *ts)
> return error;
> }
>
> + input_set_drvdata(ts->input_dev, ts);
> +
> + if (!ts->client->irq) {
> + error = input_setup_polling(ts->input_dev, goodix_ts_work_i2c_poll);
> + if (error) {
> + dev_err(&ts->client->dev,
> + "could not set up polling mode, %d\n", error);
> + return error;
> + }
> + input_set_poll_interval(ts->input_dev, GOODIX_POLL_INTERVAL_MS);
> + }
> +
> error = input_register_device(ts->input_dev);
> if (error) {
> dev_err(&ts->client->dev, @@ -1435,7 +1473,7 @@ static
> int goodix_suspend(struct device *dev)
>
> /* We need gpio pins to suspend/resume */
> if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
> - disable_irq(client->irq);
> + goodix_disable_irq(ts);
> return 0;
> }
>
> @@ -1479,7 +1517,7 @@ static int goodix_resume(struct device *dev)
> int error;
>
> if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
> - enable_irq(client->irq);
> + goodix_enable_irq(ts);
> return 0;
> }
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-05-22 2:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-21 2:50 [PATCH] input: goodix: add poll mode for goodix touchscreen Joseph Guo
2025-05-21 9:20 ` Hans de Goede
2025-05-22 2:05 ` 回复: [EXT] " Joseph Guo
-- strict thread matches above, loose matches on Subject: below --
2025-05-19 8:57 Joseph Guo
2025-05-19 16:15 ` Hans de Goede
2025-05-19 16:23 ` Dmitry Torokhov
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).