From: Hans de Goede <hidden> Date: 2020-02-21 16:47:53
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 <redacted>
Signed-off-by: Hans de Goede <redacted>
---
drivers/input/touchscreen/goodix.c | 62 ++++++++++++++++++++++++------
1 file changed, 51 insertions(+), 11 deletions(-)
@@ -502,17 +508,48 @@ static int goodix_send_cfg(struct goodix_ts_data *ts,return0;}+staticintgoodix_irq_direction_output(structgoodix_ts_data*ts,+intvalue)+{+switch(ts->irq_pin_access_method){+caseirq_pin_access_none:+dev_err(&ts->client->dev,+"%s called without an irq_pin_access_method set\n",+__func__);+return-EINVAL;+caseirq_pin_access_gpio:+returngpiod_direction_output(ts->gpiod_int,value);+}++return-EINVAL;/* Never reached */+}++staticintgoodix_irq_direction_input(structgoodix_ts_data*ts)+{+switch(ts->irq_pin_access_method){+caseirq_pin_access_none:+dev_err(&ts->client->dev,+"%s called without an irq_pin_access_method set\n",+__func__);+return-EINVAL;+caseirq_pin_access_gpio:+returngpiod_direction_input(ts->gpiod_int);+}++return-EINVAL;/* Never reached */+}+staticintgoodix_int_sync(structgoodix_ts_data*ts){interror;-error=gpiod_direction_output(ts->gpiod_int,0);+error=goodix_irq_direction_output(ts,0);if(error)returnerror;msleep(50);/* T5: 50ms */-error=gpiod_direction_input(ts->gpiod_int);+error=goodix_irq_direction_input(ts);if(error)returnerror;
@@ -943,7 +983,7 @@ static int goodix_ts_remove(struct i2c_client *client){structgoodix_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);return0;
@@ -956,7 +996,7 @@ static int __maybe_unused goodix_suspend(struct device *dev)interror;/* 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);return0;}
@@ -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);returnerror;
@@ -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)structgoodix_ts_data*ts=i2c_get_clientdata(client);interror;-if(!ts->gpiod_int||!ts->gpiod_rst){+if(ts->irq_pin_access_method==irq_pin_access_none){enable_irq(client->irq);return0;}
@@ -1008,7 +1048,7 @@ static int __maybe_unused goodix_resume(struct device *dev)*ExitsleepmodebyoutputtingHIGHleveltoINTpin*for2ms~5ms.*/-error=gpiod_direction_output(ts->gpiod_int,1);+error=goodix_irq_direction_output(ts,1);if(error)returnerror;
From: Hans de Goede <hidden> Date: 2020-02-21 16:47:53
At least on X86 ACPI platforms it is not necessary to load the touchscreen
controller config from disk, if it needs to be loaded this has already been
done by the BIOS / UEFI firmware.
Even on other (e.g. devicetree) platforms the config-loading as currently
done has the issue that the loaded cfg file is based on the controller
model, but the actual cfg is device specific, so the cfg files are not
part of linux-firmware and this can only work with a device specific OS
image which includes the cfg file.
And we do not need access to the GPIOs at all to load the config, if we
do not have access we can still load the config.
So all in all tying the decision to try to load the config from disk to
being able to access the GPIOs is not desirable. This commit adds a new
load_cfg_from_disk boolean to control the firmware loading instead.
This commits sets the new bool to true when we set irq_pin_access_method
to irq_pin_access_gpio, so there are no functional changes.
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 <redacted>
Signed-off-by: Hans de Goede <redacted>
---
drivers/input/touchscreen/goodix.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
@@ -983,7 +986,7 @@ static int goodix_ts_remove(struct i2c_client *client){structgoodix_ts_data*ts=i2c_get_clientdata(client);-if(ts->irq_pin_access_method==irq_pin_access_gpio)+if(ts->load_cfg_from_disk)wait_for_completion(&ts->firmware_loading_complete);return0;
@@ -1001,7 +1004,8 @@ static int __maybe_unused goodix_suspend(struct device *dev)return0;}-wait_for_completion(&ts->firmware_loading_complete);+if(ts->load_cfg_from_disk)+wait_for_completion(&ts->firmware_loading_complete);/* Free IRQ as IRQ pin is used as output in the suspend sequence */goodix_free_irq(ts);
From: Hans de Goede <hidden> Date: 2020-02-21 16:47:55
On most Cherry Trail (x86, UEFI + ACPI) devices the ACPI tables do not have
a _DSD with a "daffd814-6eba-4d8c-8a91-bc9bbf4aa301" UUID, adding
"irq-gpios" and "reset-gpios" mappings, so we cannot get the GPIOS by name
without first manually adding mappings ourselves.
These devices contain 1 GpioInt and 1 GpioIo resource in their _CRS table.
There is no fixed order for these 2. This commit adds code to check that
there is 1 of each as expected and then registers a mapping matching their
order using devm_acpi_dev_add_driver_gpios().
This gives us access to both GPIOs allowing us to properly suspend the
controller during suspend, and making it possible to reset the controller
if necessary.
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 <redacted>
Signed-off-by: Hans de Goede <redacted>
---
drivers/input/touchscreen/goodix.c | 113 ++++++++++++++++++++++++++++-
1 file changed, 109 insertions(+), 4 deletions(-)
@@ -521,6 +524,12 @@ static int goodix_irq_direction_output(struct goodix_ts_data *ts,return-EINVAL;caseirq_pin_access_gpio:returngpiod_direction_output(ts->gpiod_int,value);+caseirq_pin_access_acpi_gpio:+/*+*TheIRQpintriggersonafallingedge,soitsgetsmarked+*asactive-low,useoutput_rawtoavoidthevalueinversion.+*/+returngpiod_direction_output_raw(ts->gpiod_int,value);}return-EINVAL;/* Never reached */
@@ -535,6 +544,7 @@ static int goodix_irq_direction_input(struct goodix_ts_data *ts)__func__);return-EINVAL;caseirq_pin_access_gpio:+caseirq_pin_access_acpi_gpio:returngpiod_direction_input(ts->gpiod_int);}
@@ -599,6 +609,87 @@ static int goodix_reset(struct goodix_ts_data *ts)return0;}+#if defined CONFIG_X86 && defined CONFIG_ACPI+staticconststructacpi_gpio_paramsfirst_gpio={0,0,false};+staticconststructacpi_gpio_paramssecond_gpio={1,0,false};++staticconststructacpi_gpio_mappingacpi_goodix_int_first_gpios[]={+{GOODIX_GPIO_INT_NAME"-gpios",&first_gpio,1},+{GOODIX_GPIO_RST_NAME"-gpios",&second_gpio,1},+{},+};++staticconststructacpi_gpio_mappingacpi_goodix_int_last_gpios[]={+{GOODIX_GPIO_RST_NAME"-gpios",&first_gpio,1},+{GOODIX_GPIO_INT_NAME"-gpios",&second_gpio,1},+{},+};++staticintgoodix_resource(structacpi_resource*ares,void*data)+{+structgoodix_ts_data*ts=data;+structdevice*dev=&ts->client->dev;+structacpi_resource_gpio*gpio;++switch(ares->type){+caseACPI_RESOURCE_TYPE_GPIO:+gpio=&ares->data.gpio;+if(gpio->connection_type==ACPI_RESOURCE_GPIO_TYPE_INT){+if(ts->gpio_int_idx==-1){+ts->gpio_int_idx=ts->gpio_count;+}else{+dev_err(dev,"More then one GpioInt resource, ignoring ACPI GPIO resources\n");+ts->gpio_int_idx=-2;+}+}+ts->gpio_count++;+break;+default:+break;+}++return0;+}++staticintgoodix_add_acpi_gpio_mappings(structgoodix_ts_data*ts)+{+conststructacpi_gpio_mapping*gpio_mapping=NULL;+structdevice*dev=&ts->client->dev;+LIST_HEAD(resources);+intret;++ts->gpio_count=0;+ts->gpio_int_idx=-1;+ret=acpi_dev_get_resources(ACPI_COMPANION(dev),&resources,+goodix_resource,ts);+if(ret<0){+dev_err(dev,"Error getting ACPI resources: %d\n",ret);+returnret;+}++acpi_dev_free_resource_list(&resources);++if(ts->gpio_count==2&&ts->gpio_int_idx==0){+ts->irq_pin_access_method=irq_pin_access_acpi_gpio;+gpio_mapping=acpi_goodix_int_first_gpios;+}elseif(ts->gpio_count==2&&ts->gpio_int_idx==1){+ts->irq_pin_access_method=irq_pin_access_acpi_gpio;+gpio_mapping=acpi_goodix_int_last_gpios;+}else{+dev_warn(dev,"Unexpected ACPI resources: gpio_count %d, gpio_int_idx %d\n",+ts->gpio_count,ts->gpio_int_idx);+return-EINVAL;+}++returndevm_acpi_dev_add_driver_gpios(dev,gpio_mapping);+}+#else+staticintgoodix_add_acpi_gpio_mappings(structgoodix_ts_data*ts)+{+return-EINVAL;+}+#endif /* CONFIG_X86 && CONFIG_ACPI */+/***goodix_get_gpio_config-GetGPIOconfigfromACPI/DT*
@@ -609,6 +700,7 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)interror;structdevice*dev;structgpio_desc*gpiod;+booladded_acpi_mappings=false;if(!ts->client)return-EINVAL;
@@ -632,6 +724,7 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)returnerror;}+retry_get_irq_gpio:/* Get the interrupt GPIO pin number */gpiod=devm_gpiod_get_optional(dev,GOODIX_GPIO_INT_NAME,GPIOD_IN);if(IS_ERR(gpiod)){
@@ -641,6 +734,11 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)GOODIX_GPIO_INT_NAME,error);returnerror;}+if(!gpiod&&has_acpi_companion(dev)&&!added_acpi_mappings){+added_acpi_mappings=true;+if(goodix_add_acpi_gpio_mappings(ts)==0)+gotoretry_get_irq_gpio;+}ts->gpiod_int=gpiod;
@@ -656,10 +754,17 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)ts->gpiod_rst=gpiod;-if(ts->gpiod_int&&ts->gpiod_rst){-ts->reset_controller_at_probe=true;-ts->load_cfg_from_disk=true;-ts->irq_pin_access_method=irq_pin_access_gpio;+switch(ts->irq_pin_access_method){+caseirq_pin_access_acpi_gpio:+if(!ts->gpiod_int||!ts->gpiod_rst)+ts->irq_pin_access_method=irq_pin_access_none;+break;+default:+if(ts->gpiod_int&&ts->gpiod_rst){+ts->reset_controller_at_probe=true;+ts->load_cfg_from_disk=true;+ts->irq_pin_access_method=irq_pin_access_gpio;+}}return0;
From: Hans de Goede <hidden> Date: 2020-02-21 16:47:58
Before this commit we would always reset the controller at probe when we
have access to the GPIOs which are necessary to do a reset.
Doing the reset requires access to the GPIOs, but just because we have
access to the GPIOs does not mean that we should always reset the
controller at probe. On X86 ACPI platforms the BIOS / UEFI firmware will
already have reset the controller and it will have loaded the device
specific config into the controller. Doing the reset sometimes causes the
controller to loose its configuration, so on X86 ACPI platforms this is not
a good idea.
This commit adds a new reset_controller_at_probe boolean to control the
reset at probe behavior.
This commits sets the new bool to true when we set irq_pin_access_method
to irq_pin_access_gpio, so there are no functional changes.
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 <redacted>
Signed-off-by: Hans de Goede <redacted>
---
drivers/input/touchscreen/goodix.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Hans de Goede <hidden> Date: 2020-02-21 16:48:00
On most Bay Trail (x86, UEFI + ACPI) devices the ACPI tables do not have
a _DSD with a "daffd814-6eba-4d8c-8a91-bc9bbf4aa301" UUID, adding
"irq-gpios" and "reset-gpios" mappings, so we cannot get the GPIOS by name
without first manually adding mappings ourselves.
These devices contain 2 GpioIo resource in their _CRS table, on all 4 such
devices which I have access to, the order of the 2 GPIOs is reset, int.
Note that the GPIO to which the touchscreen controller irq pin is connected
is configured in direct-irq mode on these Bay Trail devices, the
pinctrl-baytrail.c driver still allows controlling the pin as a GPIO in
this case, but this is not necessarily the case on other X86 ACPI
platforms, nor do we have a guarantee that the GPIO order is the same
elsewhere, so we limit the use of a _CRS table with 2 GpioIo resources
to Bay Trail devices only.
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 <redacted>
Signed-off-by: Hans de Goede <redacted>
---
drivers/input/touchscreen/goodix.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
@@ -610,6 +610,21 @@ static int goodix_reset(struct goodix_ts_data *ts)}#if defined CONFIG_X86 && defined CONFIG_ACPI+#include<asm/cpu_device_id.h>+#include<asm/intel-family.h>++staticconststructx86_cpu_idbaytrail_cpu_ids[]={+{X86_VENDOR_INTEL,6,INTEL_FAM6_ATOM_SILVERMONT,X86_FEATURE_ANY,},+{}+};++staticinlineboolis_byt(void)+{+conststructx86_cpu_id*id=x86_match_cpu(baytrail_cpu_ids);++return!!id;+}+staticconststructacpi_gpio_paramsfirst_gpio={0,0,false};staticconststructacpi_gpio_paramssecond_gpio={1,0,false};
@@ -675,6 +690,10 @@ static int goodix_add_acpi_gpio_mappings(struct goodix_ts_data *ts)}elseif(ts->gpio_count==2&&ts->gpio_int_idx==1){ts->irq_pin_access_method=irq_pin_access_acpi_gpio;gpio_mapping=acpi_goodix_int_last_gpios;+}elseif(is_byt()&&ts->gpio_count==2&&ts->gpio_int_idx==-1){+dev_info(dev,"No ACPI GpioInt resource, assuming that the GPIO order is reset, int\n");+ts->irq_pin_access_method=irq_pin_access_acpi_gpio;+gpio_mapping=acpi_goodix_int_last_gpios;}else{dev_warn(dev,"Unexpected ACPI resources: gpio_count %d, gpio_int_idx %d\n",ts->gpio_count,ts->gpio_int_idx);
From: Hans de Goede <hidden> Date: 2020-02-21 16:48:01
Some Apollo Lake (x86, UEFI + ACPI) devices only list the reset GPIO
in their _CRS table and the bit-banging of the IRQ line necessary to
wake-up the controller from suspend can be done by calling 2 Goodix
custom / specific ACPI methods.
This commit adds support for controlling the IRQ line in this matter,
allowing us to properly suspend the touchscreen controller on such
devices.
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 <redacted>
Signed-off-by: Hans de Goede <redacted>
---
drivers/input/touchscreen/goodix.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
@@ -690,6 +710,12 @@ static int goodix_add_acpi_gpio_mappings(struct goodix_ts_data *ts)}elseif(ts->gpio_count==2&&ts->gpio_int_idx==1){ts->irq_pin_access_method=irq_pin_access_acpi_gpio;gpio_mapping=acpi_goodix_int_last_gpios;+}elseif(ts->gpio_count==1&&ts->gpio_int_idx==-1&&+acpi_has_method(ACPI_HANDLE(dev),"INTI")&&+acpi_has_method(ACPI_HANDLE(dev),"INTO")){+dev_info(dev,"Using ACPI INTI and INTO methods for IRQ pin access\n");+ts->irq_pin_access_method=irq_pin_access_acpi_method;+gpio_mapping=acpi_goodix_reset_only_gpios;}elseif(is_byt()&&ts->gpio_count==2&&ts->gpio_int_idx==-1){dev_info(dev,"No ACPI GpioInt resource, assuming that the GPIO order is reset, int\n");ts->irq_pin_access_method=irq_pin_access_acpi_gpio;
@@ -778,6 +804,10 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)if(!ts->gpiod_int||!ts->gpiod_rst)ts->irq_pin_access_method=irq_pin_access_none;break;+caseirq_pin_access_acpi_method:+if(!ts->gpiod_rst)+ts->irq_pin_access_method=irq_pin_access_none;+break;default:if(ts->gpiod_int&&ts->gpiod_rst){ts->reset_controller_at_probe=true;
@@ -426,22 +426,21 @@ static int goodix_request_irq(struct goodix_ts_data *ts)ts->irq_flags,ts->client->name,ts);}-staticintgoodix_check_cfg_8(structgoodix_ts_data*ts,-conststructfirmware*cfg)+staticintgoodix_check_cfg_8(structgoodix_ts_data*ts,constu8*cfg,intlen){-inti,raw_cfg_len=cfg->size-2;+inti,raw_cfg_len=len-2;u8check_sum=0;for(i=0;i<raw_cfg_len;i++)-check_sum+=cfg->data[i];+check_sum+=cfg[i];check_sum=(~check_sum)+1;-if(check_sum!=cfg->data[raw_cfg_len]){+if(check_sum!=cfg[raw_cfg_len]){dev_err(&ts->client->dev,"The checksum of the config fw is not correct");return-EINVAL;}-if(cfg->data[raw_cfg_len+1]!=1){+if(cfg[raw_cfg_len+1]!=1){dev_err(&ts->client->dev,"Config fw must have Config_Fresh register set");return-EINVAL;
@@ -463,22 +462,22 @@ static void goodix_fix_cfg_8(struct goodix_ts_data *ts)ts->config[raw_cfg_len+1]=1;}-staticintgoodix_check_cfg_16(structgoodix_ts_data*ts,-conststructfirmware*cfg)+staticintgoodix_check_cfg_16(structgoodix_ts_data*ts,constu8*cfg,+intlen){-inti,raw_cfg_len=cfg->size-3;+inti,raw_cfg_len=len-3;u16check_sum=0;for(i=0;i<raw_cfg_len;i+=2)-check_sum+=get_unaligned_be16(&cfg->data[i]);+check_sum+=get_unaligned_be16(&cfg[i]);check_sum=(~check_sum)+1;-if(check_sum!=get_unaligned_be16(&cfg->data[raw_cfg_len])){+if(check_sum!=get_unaligned_be16(&cfg[raw_cfg_len])){dev_err(&ts->client->dev,"The checksum of the config fw is not correct");return-EINVAL;}-if(cfg->data[raw_cfg_len+2]!=1){+if(cfg[raw_cfg_len+2]!=1){dev_err(&ts->client->dev,"Config fw must have Config_Fresh register set");return-EINVAL;
@@ -506,16 +505,15 @@ static void goodix_fix_cfg_16(struct goodix_ts_data *ts)*@ts:goodix_ts_datapointer*@cfg:firmwareconfigdata*/-staticintgoodix_check_cfg(structgoodix_ts_data*ts,-conststructfirmware*cfg)+staticintgoodix_check_cfg(structgoodix_ts_data*ts,constu8*cfg,intlen){-if(cfg->size>GOODIX_CONFIG_MAX_LENGTH){+if(len>GOODIX_CONFIG_MAX_LENGTH){dev_err(&ts->client->dev,"The length of the config fw is not correct");return-EINVAL;}-returnts->chip->check_config(ts,cfg);+returnts->chip->check_config(ts,cfg,len);}/**
@@ -524,17 +522,15 @@ static int goodix_check_cfg(struct goodix_ts_data *ts,*@ts:goodix_ts_datapointer*@cfg:configfirmwaretowritetodevice*/-staticintgoodix_send_cfg(structgoodix_ts_data*ts,-conststructfirmware*cfg)+staticintgoodix_send_cfg(structgoodix_ts_data*ts,constu8*cfg,intlen){interror;-error=goodix_check_cfg(ts,cfg);+error=goodix_check_cfg(ts,cfg,len);if(error)returnerror;-error=goodix_i2c_write(ts->client,ts->chip->config_addr,cfg->data,-cfg->size);+error=goodix_i2c_write(ts->client,ts->chip->config_addr,cfg,len);if(error){dev_err(&ts->client->dev,"Failed to write config data: %d",error);
@@ -1058,7 +1054,7 @@ static void goodix_config_cb(const struct firmware *cfg, void *ctx)if(cfg){/* send device configuration to the firmware */-error=goodix_send_cfg(ts,cfg);+error=goodix_send_cfg(ts,cfg->data,cfg->size);if(error)gotoerr_release_cfg;}
From: Hans de Goede <hidden> Date: 2020-02-21 16:48:14
Some devices, e.g the Trekstor Primetab S11B, loose there config over
a suspend/resume cycle (likely the controller looses power during suspend).
This commit reads back the config version on resume and if matches the
expected config version it resets the controller and resends the config
we read back and saved at probe time.
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 <redacted>
Signed-off-by: Hans de Goede <redacted>
---
drivers/input/touchscreen/goodix.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
quoted hunk
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 <redacted>
Signed-off-by: Hans de Goede <redacted>
---
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
@@ -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;
On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
At least on X86 ACPI platforms it is not necessary to load the
touchscreen
controller config from disk, if it needs to be loaded this has
already been
done by the BIOS / UEFI firmware.
Even on other (e.g. devicetree) platforms the config-loading as
currently
done has the issue that the loaded cfg file is based on the
controller
model, but the actual cfg is device specific, so the cfg files are
not
part of linux-firmware and this can only work with a device specific
OS
image which includes the cfg file.
And we do not need access to the GPIOs at all to load the config, if
we
do not have access we can still load the config.
So all in all tying the decision to try to load the config from disk
to
being able to access the GPIOs is not desirable. This commit adds a
new
load_cfg_from_disk boolean to control the firmware loading instead.
This commits sets the new bool to true when we set
irq_pin_access_method
to irq_pin_access_gpio, so there are no functional changes.
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 <redacted>
Signed-off-by: Hans de Goede <redacted>
@@ -983,7 +986,7 @@ static int goodix_ts_remove(struct i2c_client
*client)
{
struct goodix_ts_data *ts = i2c_get_clientdata(client);
- if (ts->irq_pin_access_method == irq_pin_access_gpio)
+ if (ts->load_cfg_from_disk)
wait_for_completion(&ts->firmware_loading_complete);
return 0;
@@ -1001,7 +1004,8 @@ static int __maybe_unused goodix_suspend(struct
device *dev)
return 0;
}
- wait_for_completion(&ts->firmware_loading_complete);
+ if (ts->load_cfg_from_disk)
+ wait_for_completion(&ts->firmware_loading_complete);
/* Free IRQ as IRQ pin is used as output in the suspend
sequence */
goodix_free_irq(ts);
On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
Before this commit we would always reset the controller at probe when
we
have access to the GPIOs which are necessary to do a reset.
Doing the reset requires access to the GPIOs, but just because we
have
access to the GPIOs does not mean that we should always reset the
controller at probe. On X86 ACPI platforms the BIOS / UEFI firmware
will
already have reset the controller and it will have loaded the device
specific config into the controller. Doing the reset sometimes causes
the
controller to loose its configuration, so on X86 ACPI platforms this
On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
On most Cherry Trail (x86, UEFI + ACPI) devices the ACPI tables do
not have
a _DSD with a "daffd814-6eba-4d8c-8a91-bc9bbf4aa301" UUID, adding
"irq-gpios" and "reset-gpios" mappings, so we cannot get the GPIOS by
name
without first manually adding mappings ourselves.
These devices contain 1 GpioInt and 1 GpioIo resource in their _CRS
table.
There is no fixed order for these 2. This commit adds code to check
that
there is 1 of each as expected and then registers a mapping matching
their
order using devm_acpi_dev_add_driver_gpios().
This gives us access to both GPIOs allowing us to properly suspend
the
controller during suspend, and making it possible to reset the
controller
if necessary.
Can you include the DSDT snippet that defines those GPIOs in the commit
message?
@@ -521,6 +524,12 @@ static int goodix_irq_direction_output(struct
goodix_ts_data *ts,
return -EINVAL;
case irq_pin_access_gpio:
return gpiod_direction_output(ts->gpiod_int, value);
+ case irq_pin_access_acpi_gpio:
+ /*
+ * The IRQ pin triggers on a falling edge, so its gets
marked
+ * as active-low, use output_raw to avoid the value
inversion.
+ */
+ return gpiod_direction_output_raw(ts->gpiod_int,
value);
}
return -EINVAL; /* Never reached */
@@ -535,6 +544,7 @@ static int goodix_irq_direction_input(struct
goodix_ts_data *ts)
__func__);
return -EINVAL;
case irq_pin_access_gpio:
+ case irq_pin_access_acpi_gpio:
return gpiod_direction_input(ts->gpiod_int);
}
@@ -599,6 +609,87 @@ static int goodix_reset(struct goodix_ts_data
Is there no way to implement this in a more generic way? Is goodix the
only driver that needs this sort of handling of GPIOs for ACPI?
This portion could do with being split off, if we were ever to get that
more generic solution.
On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
On most Bay Trail (x86, UEFI + ACPI) devices the ACPI tables do not
have
a _DSD with a "daffd814-6eba-4d8c-8a91-bc9bbf4aa301" UUID, adding
"irq-gpios" and "reset-gpios" mappings, so we cannot get the GPIOS by
name
without first manually adding mappings ourselves.
These devices contain 2 GpioIo resource in their _CRS table, on all 4
such
devices which I have access to, the order of the 2 GPIOs is reset,
int.
Note that the GPIO to which the touchscreen controller irq pin is
connected
is configured in direct-irq mode on these Bay Trail devices, the
pinctrl-baytrail.c driver still allows controlling the pin as a GPIO
in
this case, but this is not necessarily the case on other X86 ACPI
platforms, nor do we have a guarantee that the GPIO order is the same
elsewhere, so we limit the use of a _CRS table with 2 GpioIo
resources
to Bay Trail devices only.
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 <redacted>
Signed-off-by: Hans de Goede <redacted>
This is gross, but...
Reviewed-by: Bastien Nocera <hadess@hadess.net>
On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
Some Apollo Lake (x86, UEFI + ACPI) devices only list the reset GPIO
in their _CRS table and the bit-banging of the IRQ line necessary to
wake-up the controller from suspend can be done by calling 2 Goodix
custom / specific ACPI methods.
This commit adds support for controlling the IRQ line in this matter,
allowing us to properly suspend the touchscreen controller on such
devices.
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 <redacted>
Signed-off-by: Hans de Goede <redacted>
@@ -516,6 +517,9 @@ static int goodix_send_cfg(struct goodix_ts_data
*ts,
static int goodix_irq_direction_output(struct goodix_ts_data *ts,
int value)
{
+ struct device *dev = &ts->client->dev;
+ acpi_status status;
+
switch (ts->irq_pin_access_method) {
case irq_pin_access_none:
dev_err(&ts->client->dev,
@@ -530,6 +534,10 @@ static int goodix_irq_direction_output(struct
goodix_ts_data *ts,
* as active-low, use output_raw to avoid the value
inversion.
*/
return gpiod_direction_output_raw(ts->gpiod_int,
value);
+ case irq_pin_access_acpi_method:
+ status = acpi_execute_simple_method(ACPI_HANDLE(dev),
+ "INTO", value);
+ return ACPI_SUCCESS(status) ? 0 : -EIO;
}
return -EINVAL; /* Never reached */
@@ -537,6 +545,9 @@ static int goodix_irq_direction_output(struct
firmware *);
+ int (*check_config)(struct goodix_ts_data *ts, const u8 *cfg,
int len);
Any way to make the length a uint instead of an int? That way, we
wouldn't need to add < 0 guards, and the "len > MAX_LENGTH" check would
be enough.
Looks good otherwise:
Reviewed-by: Bastien Nocera <hadess@hadess.net>
From: Hans de Goede <hidden> Date: 2020-03-02 13:23:41
Hi,
On 3/2/20 12:09 PM, Bastien Nocera wrote:
On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
quoted
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 <redacted>
Signed-off-by: Hans de Goede <redacted>
---
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
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.
I checked and most of the kernel also uses upper-case for
enum member names, I'll fix this (for the entire series) for
v2 of the series.
As for using typedef-s that is typically not done in the kernel
for enums / structs so I'm going to keep that as is.
quoted
+};
+
struct goodix_chip_data {
u16 config_addr;
int config_len;
@@ -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.
All the values in the enum are handled so there is no need for a default
label. As for changing this to an if, later patches add more values
to the enum and to the switch-case, having this as a switch-case from
the start makes the diff in later patches smaller.
quoted
+ }
+
+ 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);
@@ -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;
From: Hans de Goede <hidden> Date: 2020-03-02 15:40:19
Hi,
On 3/2/20 12:23 PM, Bastien Nocera wrote:
On Fri, 2020-02-21 at 17:47 +0100, Hans de Goede wrote:
quoted
On most Cherry Trail (x86, UEFI + ACPI) devices the ACPI tables do
not have
a _DSD with a "daffd814-6eba-4d8c-8a91-bc9bbf4aa301" UUID, adding
"irq-gpios" and "reset-gpios" mappings, so we cannot get the GPIOS by
name
without first manually adding mappings ourselves.
These devices contain 1 GpioInt and 1 GpioIo resource in their _CRS
table.
There is no fixed order for these 2. This commit adds code to check
that
there is 1 of each as expected and then registers a mapping matching
their
order using devm_acpi_dev_add_driver_gpios().
This gives us access to both GPIOs allowing us to properly suspend
the
controller during suspend, and making it possible to reset the
controller
if necessary.
Can you include the DSDT snippet that defines those GPIOs in the commit
message?
@@ -521,6 +524,12 @@ static int goodix_irq_direction_output(struct
goodix_ts_data *ts,
return -EINVAL;
case irq_pin_access_gpio:
return gpiod_direction_output(ts->gpiod_int, value);
+ case irq_pin_access_acpi_gpio:
+ /*
+ * The IRQ pin triggers on a falling edge, so its gets
marked
+ * as active-low, use output_raw to avoid the value
inversion.
+ */
+ return gpiod_direction_output_raw(ts->gpiod_int,
value);
}
return -EINVAL; /* Never reached */
@@ -535,6 +544,7 @@ static int goodix_irq_direction_input(struct
goodix_ts_data *ts)
__func__);
return -EINVAL;
case irq_pin_access_gpio:
+ case irq_pin_access_acpi_gpio:
return gpiod_direction_input(ts->gpiod_int);
}
@@ -599,6 +609,87 @@ static int goodix_reset(struct goodix_ts_data
Is there no way to implement this in a more generic way? Is goodix the
only driver that needs this sort of handling of GPIOs for ACPI?
The Linux GPIO subsystem expects drivers to request GPIOs by name, but
in most ACPI tables there is only a list of resources, so we have an
index, but not a name. ACPI tables can define extra GPIO info using
a method with the special ACPI "daffd814-6eba-4d8c-8a91-bc9bbf4aa301"
UUID which is reserved for this, but I'm not aware of any devices where
the ACPI tables actually use this.
So all x86 drivers which lookup GPIOs from ACPI tables need to manually
add a mapping by calling devm_acpi_dev_add_driver_gpios(). Ideally the
Windows driver mandates a fixed order in which the GPIOs must be put
in the _CRS method and all we need a single acpi_gpio_mapping in the
driver.
But in some cases, like this case the order is not fixed and we need
some heuristics to figure out the right order and we have multiple
acpi_gpio_mapping-s and select one to pass to devm_acpi_dev_add_driver_gpios()
based on heuristics. That is what happening here. These heuristics
are tyically different per driver, so this is not really something
which we can share between drivers. The only other case which I'm aware
of which is doing something similar (but not identical) is the bcm
bluetooth code in drivers/bluetooth/hci_bcm.c, starting around line 880.
TL;DR: at this point in time I do not see a more generic way to do this.
This portion could do with being split off, if we were ever to get that
more generic solution.
Yes, we are not really "retrying", we are doing a 2 step
probe:
1) First try to get the GPIOs without having done our heuristics and
without having called devm_acpi_dev_add_driver_gpios(). This is for
ACPI platforms extra GPIO info (including names) using the special
ACPI "daffd814-6eba-4d8c-8a91-bc9bbf4aa301" UUID method.
2) If this fails then we add our own name to index mappings and
get the GPIOs using those.
On Mon, 2020-03-02 at 16:40 +0100, Hans de Goede wrote:
quoted
Does this mean we retry at most once?
Yes, we are not really "retrying", we are doing a 2 step
probe:
1) First try to get the GPIOs without having done our heuristics and
without having called devm_acpi_dev_add_driver_gpios(). This is for
ACPI platforms extra GPIO info (including names) using the special
ACPI "daffd814-6eba-4d8c-8a91-bc9bbf4aa301" UUID method.
2) If this fails then we add our own name to index mappings and
get the GPIOs using those.
Is there a better way to communicate that? Using a separate function
for that piece of code, and maybe some comments to clarify what it's
doing.
Thanks for the other explanations.
From: Hans de Goede <hidden> Date: 2020-03-02 16:23:27
Hi,
On 3/2/20 4:44 PM, Bastien Nocera wrote:
On Mon, 2020-03-02 at 16:40 +0100, Hans de Goede wrote:
quoted
quoted
Does this mean we retry at most once?
Yes, we are not really "retrying", we are doing a 2 step
probe:
1) First try to get the GPIOs without having done our heuristics and
without having called devm_acpi_dev_add_driver_gpios(). This is for
ACPI platforms extra GPIO info (including names) using the special
ACPI "daffd814-6eba-4d8c-8a91-bc9bbf4aa301" UUID method.
2) If this fails then we add our own name to index mappings and
get the GPIOs using those.
Is there a better way to communicate that? Using a separate function
for that piece of code,
The code adding our own mappings already is in a separate function, that
is what the goodix_add_acpi_gpio_mappings function is for.
and maybe some comments to clarify what it's
doing.
I will add a comment above the goodix_add_acpi_gpio_mappings function
explaining that it is used to add our own mappings if the ACPI
tables do not contain GPIO-name to ACPI resource index mappings.
Regards,
Hans
firmware *);
+ int (*check_config)(struct goodix_ts_data *ts, const u8 *cfg,
int len);
Any way to make the length a uint instead of an int? That way, we
wouldn't need to add < 0 guards, and the "len > MAX_LENGTH" check would
be enough.
Actually the code does things like:
int raw_cfg_len = len - 3;
for (i = 0; i < raw_cfg_len; i += 2)
check_sum += get_unaligned_be16(&ts->config[i]);
check_sum = (~check_sum) + 1;
if (check_sum != get_unaligned_be16(&cfg[raw_cfg_len])) {
Which is bad without a minimum check regardless of us using signed
or unsigned ints here. unsigned ints will short-circuit / skip the for
loop when len < 3, but then we end up with a negative array index when
doing: &cfg[raw_cfg_len]. And when going unsigned then both the loop
and the array index will be out of bounds when len < 3. So the proper
fix here is to add a minimum check.
I will add an extra patch to the patch-set adding a minimum bounds check.
Regards,
Hans
Looks good otherwise:
Reviewed-by: Bastien Nocera <hadess@hadess.net>