| Message ID | 20190830000024.20384-2-peter@typeblog.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] gpio: acpi: add quirk to override GpioInt polarity | expand |
On Fri, Aug 30, 2019 at 08:00:24AM +0800, Peter Cai wrote: > The firmware of GPD P2 Max could not handle panel resets although code > is present in DSDT. The kernel needs to take on this job instead, but > the DSDT does not provide _DSD, rendering kernel helpless when trying to > find the respective GPIO pins. > > Fortunately, this time GPD has proper DMI vendor / product strings that > we could match against. We simply apply an acpi_gpio_mapping table when > GPD P2 Max is matched. > > Additionally, the DSDT definition of the irq pin specifies a wrong > polarity. The new quirk introduced in the previous patch > (ACPI_GPIO_QUIRK_OVERRIDE_POLARITY) is applied to correct this. > +#ifdef CONFIG_ACPI I guess most of these #ifdef:s makes code less readable for exchange of saving few bytes in the module footprint. > + { "irq-gpios", &irq_gpios_default, 1, > + ACPI_GPIO_QUIRK_OVERRIDE_POLARITY }, One line? > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "GPD"), > + DMI_MATCH(DMI_PRODUCT_NAME, "P2 MAX") Comma at the end? > + }, > + .driver_data = &gpio_mapping_force_irq_active_high Ditto.
On August 30, 2019 7:55:05 PM GMT+08:00, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: >On Fri, Aug 30, 2019 at 08:00:24AM +0800, Peter Cai wrote: >> The firmware of GPD P2 Max could not handle panel resets although >code >> is present in DSDT. The kernel needs to take on this job instead, but >> the DSDT does not provide _DSD, rendering kernel helpless when trying >to >> find the respective GPIO pins. >> >> Fortunately, this time GPD has proper DMI vendor / product strings >that >> we could match against. We simply apply an acpi_gpio_mapping table >when >> GPD P2 Max is matched. >> >> Additionally, the DSDT definition of the irq pin specifies a wrong >> polarity. The new quirk introduced in the previous patch >> (ACPI_GPIO_QUIRK_OVERRIDE_POLARITY) is applied to correct this. > >> +#ifdef CONFIG_ACPI > >I guess most of these #ifdef:s makes code less readable for exchange of >saving >few bytes in the module footprint. > >> + { "irq-gpios", &irq_gpios_default, 1, >> + ACPI_GPIO_QUIRK_OVERRIDE_POLARITY }, > >One line? > >> + .matches = { >> + DMI_MATCH(DMI_SYS_VENDOR, "GPD"), >> + DMI_MATCH(DMI_PRODUCT_NAME, "P2 MAX") > >Comma at the end? > >> + }, >> + .driver_data = &gpio_mapping_force_irq_active_high > >Ditto. > I guess most of these #ifdef:s makes code less readable for exchange of saving few bytes in the module footprint. Since they can only be used when ACPI is supported (devm_acpi_dev_add_driver_gpios does not exist without ACPI defined, thus the last guard must exist), if they were not guarded then we would be left with a bunch of unused variables warnings when building without ACPI which doesn't seem good. Should we use __maybe_unused here instead of #ifdef guards? > Comma at the end? I was trying to follow the style of this driver but it doesn't seem to be really consistent within itself. Another dmi_system_id definition in the same file mixed both styles so I was kind of confused.
On Fri, Aug 30, 2019 at 11:15:27PM +0800, Peter Cai wrote: > On Fri, Aug 30, 2019, 7:55 PM Andy Shevchenko < > andriy.shevchenko@linux.intel.com> wrote: > > I guess most of these #ifdef:s makes code less readable for exchange of > saving > few bytes in the module footprint. > > Well since they can only be used when ACPI is supported > (devm_acpi_dev_add_driver_gpios does not exist without ACPI defined, thus > the last guard must exist), This is not correct. > if they were not guarded then we would be left > with a bunch of unused variables warnings when building without ACPI which > doesn't seem good. Good / no-good is only matter of few dozens of bytes here and there to be saved. > Should we use __maybe_unused here instead of #ifdef guards? No, it won't make sense, because the structures will be part of _add_driver_gpio() call, due to which compiler likely can't recognize unused structures. However, you may try with warnings enabled `make W=1`. > > Comma at the end? > > I was trying to follow the style of this driver but it doesn't seem to be > really consistent within itself. Another dmi_system_id definition in the > same file mixed both styles so I was kind of confused. I see. So, this is for Dmitry's preferences.
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c index 5178ea8b5f30..65b8d04b6dcf 100644 --- a/drivers/input/touchscreen/goodix.c +++ b/drivers/input/touchscreen/goodix.c @@ -144,6 +144,34 @@ static const struct dmi_system_id rotated_screen[] = { {} }; +#ifdef CONFIG_ACPI +static const struct acpi_gpio_params irq_gpios_default = { 0, 0, false }; +static const struct acpi_gpio_params reset_gpios_default = { 1, 0, false }; +static const struct acpi_gpio_mapping gpio_mapping_force_irq_active_high[] = { + { "irq-gpios", &irq_gpios_default, 1, + ACPI_GPIO_QUIRK_OVERRIDE_POLARITY }, + { "reset-gpios", &reset_gpios_default, 1 }, + {} +}; + +/* + * Devices that need acpi_gpio_mapping to function correctly + */ +static const struct dmi_system_id need_gpio_mapping[] = { +#if defined(CONFIG_DMI) && defined(CONFIG_X86) + { + .ident = "GPD P2 Max", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "GPD"), + DMI_MATCH(DMI_PRODUCT_NAME, "P2 MAX") + }, + .driver_data = &gpio_mapping_force_irq_active_high + }, +#endif + {} +}; +#endif + /** * goodix_i2c_read - read data from a register of the i2c slave device. * @@ -796,6 +824,15 @@ static int goodix_ts_probe(struct i2c_client *client, struct goodix_ts_data *ts; int error; +#ifdef CONFIG_ACPI + struct dmi_system_id *dmi_match; + + dmi_match = dmi_first_match(need_gpio_mapping); + if (dmi_match) + devm_acpi_dev_add_driver_gpios(&client->dev, + dmi_match->driver_data); +#endif + dev_dbg(&client->dev, "I2C Address: 0x%02x\n", client->addr); if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
The firmware of GPD P2 Max could not handle panel resets although code is present in DSDT. The kernel needs to take on this job instead, but the DSDT does not provide _DSD, rendering kernel helpless when trying to find the respective GPIO pins. Fortunately, this time GPD has proper DMI vendor / product strings that we could match against. We simply apply an acpi_gpio_mapping table when GPD P2 Max is matched. Additionally, the DSDT definition of the irq pin specifies a wrong polarity. The new quirk introduced in the previous patch (ACPI_GPIO_QUIRK_OVERRIDE_POLARITY) is applied to correct this. Signed-off-by: Peter Cai <peter@typeblog.net> --- drivers/input/touchscreen/goodix.c | 37 ++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)