| Message ID | 20200221164735.508324-4-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [resend,01/10] Input: goodix - Refactor IRQ pin GPIO accesses | expand |
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? > > 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 | 113 > ++++++++++++++++++++++++++++- > 1 file changed, 109 insertions(+), 4 deletions(-) > > diff --git a/drivers/input/touchscreen/goodix.c > b/drivers/input/touchscreen/goodix.c > index dd5a8f9e8ada..9de2f325ac6e 100644 > --- a/drivers/input/touchscreen/goodix.c > +++ b/drivers/input/touchscreen/goodix.c > @@ -34,6 +34,7 @@ struct goodix_ts_data; > enum goodix_irq_pin_access_method { > irq_pin_access_none, > irq_pin_access_gpio, > + irq_pin_access_acpi_gpio, > }; > > struct goodix_chip_data { > @@ -53,6 +54,8 @@ struct goodix_ts_data { > struct regulator *vddio; > struct gpio_desc *gpiod_int; > struct gpio_desc *gpiod_rst; > + int gpio_count; > + int gpio_int_idx; > u16 id; > u16 version; > const char *cfg_name; > @@ -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 > *ts) > return 0; > } > > +#if defined CONFIG_X86 && defined CONFIG_ACPI > +static const struct acpi_gpio_params first_gpio = { 0, 0, false }; > +static const struct acpi_gpio_params second_gpio = { 1, 0, false }; > + > +static const struct acpi_gpio_mapping acpi_goodix_int_first_gpios[] > = { > + { GOODIX_GPIO_INT_NAME "-gpios", &first_gpio, 1 }, > + { GOODIX_GPIO_RST_NAME "-gpios", &second_gpio, 1 }, > + { }, > +}; > + > +static const struct acpi_gpio_mapping acpi_goodix_int_last_gpios[] = > { > + { GOODIX_GPIO_RST_NAME "-gpios", &first_gpio, 1 }, > + { GOODIX_GPIO_INT_NAME "-gpios", &second_gpio, 1 }, > + { }, > +}; > + > +static int goodix_resource(struct acpi_resource *ares, void *data) > +{ > + struct goodix_ts_data *ts = data; > + struct device *dev = &ts->client->dev; > + struct acpi_resource_gpio *gpio; > + > + switch (ares->type) { > + case ACPI_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; > + } > + > + return 0; > +} > + > +static int goodix_add_acpi_gpio_mappings(struct goodix_ts_data *ts) 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. > +{ > + const struct acpi_gpio_mapping *gpio_mapping = NULL; > + struct device *dev = &ts->client->dev; > + LIST_HEAD(resources); > + int ret; > + > + 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); > + return ret; > + } > + > + 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; > + } else if (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; > + } > + > + return devm_acpi_dev_add_driver_gpios(dev, gpio_mapping); > +} > +#else > +static int goodix_add_acpi_gpio_mappings(struct goodix_ts_data *ts) > +{ > + return -EINVAL; > +} > +#endif /* CONFIG_X86 && CONFIG_ACPI */ > + > /** > * goodix_get_gpio_config - Get GPIO config from ACPI/DT > * > @@ -609,6 +700,7 @@ static int goodix_get_gpio_config(struct > goodix_ts_data *ts) > int error; > struct device *dev; > struct gpio_desc *gpiod; > + bool added_acpi_mappings = false; > > if (!ts->client) > return -EINVAL; > @@ -632,6 +724,7 @@ static int goodix_get_gpio_config(struct > goodix_ts_data *ts) > return error; > } > > +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); > return error; > } > + if (!gpiod && has_acpi_companion(dev) && !added_acpi_mappings) > { > + added_acpi_mappings = true; Does this mean we retry at most once? > + if (goodix_add_acpi_gpio_mappings(ts) == 0) > + goto retry_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) { Can't say I like switch statements with only 2 cases... > + case irq_pin_access_acpi_gpio: Can you add a comment that explains that this is to fallback in case we didn't manage to get the ACPI mappings? > + 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; > + } > } > > return 0;
Hi, On 3/2/20 12:23 PM, Bastien Nocera wrote: > 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? Will do for v2 of this series. >> 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 | 113 >> ++++++++++++++++++++++++++++- >> 1 file changed, 109 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/input/touchscreen/goodix.c >> b/drivers/input/touchscreen/goodix.c >> index dd5a8f9e8ada..9de2f325ac6e 100644 >> --- a/drivers/input/touchscreen/goodix.c >> +++ b/drivers/input/touchscreen/goodix.c >> @@ -34,6 +34,7 @@ struct goodix_ts_data; >> enum goodix_irq_pin_access_method { >> irq_pin_access_none, >> irq_pin_access_gpio, >> + irq_pin_access_acpi_gpio, >> }; >> >> struct goodix_chip_data { >> @@ -53,6 +54,8 @@ struct goodix_ts_data { >> struct regulator *vddio; >> struct gpio_desc *gpiod_int; >> struct gpio_desc *gpiod_rst; >> + int gpio_count; >> + int gpio_int_idx; >> u16 id; >> u16 version; >> const char *cfg_name; >> @@ -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 >> *ts) >> return 0; >> } >> >> +#if defined CONFIG_X86 && defined CONFIG_ACPI >> +static const struct acpi_gpio_params first_gpio = { 0, 0, false }; >> +static const struct acpi_gpio_params second_gpio = { 1, 0, false }; >> + >> +static const struct acpi_gpio_mapping acpi_goodix_int_first_gpios[] >> = { >> + { GOODIX_GPIO_INT_NAME "-gpios", &first_gpio, 1 }, >> + { GOODIX_GPIO_RST_NAME "-gpios", &second_gpio, 1 }, >> + { }, >> +}; >> + >> +static const struct acpi_gpio_mapping acpi_goodix_int_last_gpios[] = >> { >> + { GOODIX_GPIO_RST_NAME "-gpios", &first_gpio, 1 }, >> + { GOODIX_GPIO_INT_NAME "-gpios", &second_gpio, 1 }, >> + { }, >> +}; >> + >> +static int goodix_resource(struct acpi_resource *ares, void *data) >> +{ >> + struct goodix_ts_data *ts = data; >> + struct device *dev = &ts->client->dev; >> + struct acpi_resource_gpio *gpio; >> + >> + switch (ares->type) { >> + case ACPI_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; >> + } >> + >> + return 0; >> +} >> + >> +static int goodix_add_acpi_gpio_mappings(struct goodix_ts_data *ts) > > 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. > >> +{ >> + const struct acpi_gpio_mapping *gpio_mapping = NULL; >> + struct device *dev = &ts->client->dev; >> + LIST_HEAD(resources); >> + int ret; >> + >> + 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); >> + return ret; >> + } >> + >> + 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; >> + } else if (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; >> + } >> + >> + return devm_acpi_dev_add_driver_gpios(dev, gpio_mapping); >> +} >> +#else >> +static int goodix_add_acpi_gpio_mappings(struct goodix_ts_data *ts) >> +{ >> + return -EINVAL; >> +} >> +#endif /* CONFIG_X86 && CONFIG_ACPI */ >> + >> /** >> * goodix_get_gpio_config - Get GPIO config from ACPI/DT >> * >> @@ -609,6 +700,7 @@ static int goodix_get_gpio_config(struct >> goodix_ts_data *ts) >> int error; >> struct device *dev; >> struct gpio_desc *gpiod; >> + bool added_acpi_mappings = false; >> >> if (!ts->client) >> return -EINVAL; >> @@ -632,6 +724,7 @@ static int goodix_get_gpio_config(struct >> goodix_ts_data *ts) >> return error; >> } >> >> +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); >> return error; >> } >> + if (!gpiod && has_acpi_companion(dev) && !added_acpi_mappings) >> { >> + added_acpi_mappings = true; > > 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. >> + if (goodix_add_acpi_gpio_mappings(ts) == 0) >> + goto retry_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) { > > Can't say I like switch statements with only 2 cases... More cases are added in later patches. > >> + case irq_pin_access_acpi_gpio: > > Can you add a comment that explains that this is to fallback in case we > didn't manage to get the ACPI mappings? Will do for v2 if this patch series. > >> + 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; >> + } >> } >> >> return 0; > Regards, Hans
On Mon, 2020-03-02 at 16:40 +0100, Hans de Goede wrote: > > 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.
Hi, On 3/2/20 4:44 PM, Bastien Nocera wrote: > On Mon, 2020-03-02 at 16:40 +0100, Hans de Goede wrote: >>> 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
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c index dd5a8f9e8ada..9de2f325ac6e 100644 --- a/drivers/input/touchscreen/goodix.c +++ b/drivers/input/touchscreen/goodix.c @@ -34,6 +34,7 @@ struct goodix_ts_data; enum goodix_irq_pin_access_method { irq_pin_access_none, irq_pin_access_gpio, + irq_pin_access_acpi_gpio, }; struct goodix_chip_data { @@ -53,6 +54,8 @@ struct goodix_ts_data { struct regulator *vddio; struct gpio_desc *gpiod_int; struct gpio_desc *gpiod_rst; + int gpio_count; + int gpio_int_idx; u16 id; u16 version; const char *cfg_name; @@ -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 *ts) return 0; } +#if defined CONFIG_X86 && defined CONFIG_ACPI +static const struct acpi_gpio_params first_gpio = { 0, 0, false }; +static const struct acpi_gpio_params second_gpio = { 1, 0, false }; + +static const struct acpi_gpio_mapping acpi_goodix_int_first_gpios[] = { + { GOODIX_GPIO_INT_NAME "-gpios", &first_gpio, 1 }, + { GOODIX_GPIO_RST_NAME "-gpios", &second_gpio, 1 }, + { }, +}; + +static const struct acpi_gpio_mapping acpi_goodix_int_last_gpios[] = { + { GOODIX_GPIO_RST_NAME "-gpios", &first_gpio, 1 }, + { GOODIX_GPIO_INT_NAME "-gpios", &second_gpio, 1 }, + { }, +}; + +static int goodix_resource(struct acpi_resource *ares, void *data) +{ + struct goodix_ts_data *ts = data; + struct device *dev = &ts->client->dev; + struct acpi_resource_gpio *gpio; + + switch (ares->type) { + case ACPI_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; + } + + return 0; +} + +static int goodix_add_acpi_gpio_mappings(struct goodix_ts_data *ts) +{ + const struct acpi_gpio_mapping *gpio_mapping = NULL; + struct device *dev = &ts->client->dev; + LIST_HEAD(resources); + int ret; + + 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); + return ret; + } + + 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; + } else if (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; + } + + return devm_acpi_dev_add_driver_gpios(dev, gpio_mapping); +} +#else +static int goodix_add_acpi_gpio_mappings(struct goodix_ts_data *ts) +{ + return -EINVAL; +} +#endif /* CONFIG_X86 && CONFIG_ACPI */ + /** * goodix_get_gpio_config - Get GPIO config from ACPI/DT * @@ -609,6 +700,7 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts) int error; struct device *dev; struct gpio_desc *gpiod; + bool added_acpi_mappings = false; if (!ts->client) return -EINVAL; @@ -632,6 +724,7 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts) return error; } +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); return error; } + if (!gpiod && has_acpi_companion(dev) && !added_acpi_mappings) { + added_acpi_mappings = true; + if (goodix_add_acpi_gpio_mappings(ts) == 0) + goto retry_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) { + case irq_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; + } } return 0;
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 <mastichi@gmail.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/input/touchscreen/goodix.c | 113 ++++++++++++++++++++++++++++- 1 file changed, 109 insertions(+), 4 deletions(-)