| Message ID | 20200325150246.128463-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | [v2] Input: goodix - Fix compilation when ACPI support is disabled | expand |
Hi Hans, On Wed, Mar 25, 2020 at 04:02:46PM +0100, Hans de Goede wrote: > acpi_evaluate_object() and acpi_execute_simple_method() are not part of > the group of ACPI related functions which get stubbed by > include/linux/acpi.h when ACPI support is disabled, so the > IRQ_PIN_ACCESS_ACPI_METHOD handling code must be disabled through > an #ifdef when ACPI support is not enabled. > > For consistency also #ifdef out the IRQ_PIN_ACCESS_ACPI_GPIO code > and use the same #if condition as which is used to replace > goodix_add_acpi_gpio_mappings with a stub. I am not big fun of multiple #ifdefs sprinkled through the code, can we do more straightforward stubs, like below? Thanks!
Hi, On 4/1/20 3:45 AM, Dmitry Torokhov wrote: > Hi Hans, > > On Wed, Mar 25, 2020 at 04:02:46PM +0100, Hans de Goede wrote: >> acpi_evaluate_object() and acpi_execute_simple_method() are not part of >> the group of ACPI related functions which get stubbed by >> include/linux/acpi.h when ACPI support is disabled, so the >> IRQ_PIN_ACCESS_ACPI_METHOD handling code must be disabled through >> an #ifdef when ACPI support is not enabled. >> >> For consistency also #ifdef out the IRQ_PIN_ACCESS_ACPI_GPIO code >> and use the same #if condition as which is used to replace >> goodix_add_acpi_gpio_mappings with a stub. > > I am not big fun of multiple #ifdefs sprinkled through the code, can we > do more straightforward stubs, like below? The solution you suggested is fine with me. Regards, Hans p.s. For reference here is Dmitry's solution once again, manually copied because my email client cut it of as being part of the signature: Input: goodix - fix compilation when ACPI support is disabled From: Hans de Goede <hdegoede@redhat.com> acpi_evaluate_object() and acpi_execute_simple_method() are not part of the group of ACPI related functions which get stubbed by include/linux/acpi.h when ACPI support is disabled, so the IRQ_PIN_ACCESS_ACPI_METHOD handling code must be stubbed out. For consistency use the same #if condition as which is used to replace goodix_add_acpi_gpio_mappings with a stub. Fixes: c5fca485320e ("Input: goodix - add support for controlling the IRQ pin through ACPI methods") Reported-by: kbuild test robot <lkp@intel.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> [dtor: stubbed out the ACPI method accessors] Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/touchscreen/goodix.c | 55 +++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 13 deletions(-) diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c index 47f812b804c8..02c75ea385e0 100644 --- a/drivers/input/touchscreen/goodix.c +++ b/drivers/input/touchscreen/goodix.c @@ -63,6 +63,11 @@ #define MAX_CONTACTS_LOC 5 #define TRIGGER_LOC 6 +/* Our special handling for GPIO accesses through ACPI is x86 specific */ +#if defined CONFIG_X86 && defined CONFIG_ACPI +#define ACPI_GPIO_SUPPORT +#endif + struct goodix_ts_data; enum goodix_irq_pin_access_method { @@ -600,12 +605,42 @@ static int goodix_send_cfg(struct goodix_ts_data *ts, const u8 *cfg, int len) return 0; } -static int goodix_irq_direction_output(struct goodix_ts_data *ts, - int value) +#ifdef ACPI_GPIO_SUPPORT +static int goodix_pin_acpi_direction_input(struct goodix_ts_data *ts) { - struct device *dev = &ts->client->dev; + acpi_handle handle = ACPI_HANDLE(&ts->client->dev); acpi_status status; + status = acpi_evaluate_object(handle, "INTI", NULL, NULL); + return ACPI_SUCCESS(status) ? 0 : -EIO; +} + +static int goodix_pin_acpi_output_method(struct goodix_ts_data *ts, int value) +{ + acpi_handle handle = ACPI_HANDLE(&ts->client->dev); + acpi_status status; + + status = acpi_execute_simple_method(handle, "INTO", value); + return ACPI_SUCCESS(status) ? 0 : -EIO; +} +#else +static int goodix_pin_acpi_direction_input(struct goodix_ts_data *ts) +{ + dev_err(&ts->client->dev, + "%s called on device without ACPI support\n", __func__); + return -EINVAL; +} + +static int goodix_pin_acpi_output_method(struct goodix_ts_data *ts, int value) +{ + dev_err(&ts->client->dev, + "%s called on device without ACPI support\n", __func__); + return -EINVAL; +} +#endif + +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, @@ -621,9 +656,7 @@ static int goodix_irq_direction_output(struct goodix_ts_data *ts, */ 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 goodix_pin_acpi_output_method(ts, value); } return -EINVAL; /* Never reached */ @@ -631,9 +664,6 @@ static int goodix_irq_direction_output(struct goodix_ts_data *ts, static int goodix_irq_direction_input(struct goodix_ts_data *ts) { - 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, @@ -641,12 +671,11 @@ static int goodix_irq_direction_input(struct goodix_ts_data *ts) __func__); return -EINVAL; case IRQ_PIN_ACCESS_GPIO: + return gpiod_direction_input(ts->gpiod_int); case IRQ_PIN_ACCESS_ACPI_GPIO: return gpiod_direction_input(ts->gpiod_int); case IRQ_PIN_ACCESS_ACPI_METHOD: - status = acpi_evaluate_object(ACPI_HANDLE(dev), "INTI", - NULL, NULL); - return ACPI_SUCCESS(status) ? 0 : -EIO; + return goodix_pin_acpi_direction_input(ts); } return -EINVAL; /* Never reached */ @@ -710,7 +739,7 @@ static int goodix_reset(struct goodix_ts_data *ts) return 0; } -#if defined CONFIG_X86 && defined CONFIG_ACPI +#ifdef ACPI_GPIO_SUPPORT #include <asm/cpu_device_id.h> #include <asm/intel-family.h>
On Wed, 2020-04-01 at 11:14 +0200, Hans de Goede wrote: > Hi, > > On 4/1/20 3:45 AM, Dmitry Torokhov wrote: > > Hi Hans, > > > > On Wed, Mar 25, 2020 at 04:02:46PM +0100, Hans de Goede wrote: > > > acpi_evaluate_object() and acpi_execute_simple_method() are not > > > part of > > > the group of ACPI related functions which get stubbed by > > > include/linux/acpi.h when ACPI support is disabled, so the > > > IRQ_PIN_ACCESS_ACPI_METHOD handling code must be disabled through > > > an #ifdef when ACPI support is not enabled. > > > > > > For consistency also #ifdef out the IRQ_PIN_ACCESS_ACPI_GPIO code > > > and use the same #if condition as which is used to replace > > > goodix_add_acpi_gpio_mappings with a stub. > > > > I am not big fun of multiple #ifdefs sprinkled through the code, > > can we > > do more straightforward stubs, like below? > > The solution you suggested is fine with me. The patch itself is slightly confusing (it looks like it's renaming functions), but looks pretty much like I requested, so: Reviewed-by: Bastien Nocera <hadess@hadess.net>
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c index 2c9cd1bfb860..7ccd8e436297 100644 --- a/drivers/input/touchscreen/goodix.c +++ b/drivers/input/touchscreen/goodix.c @@ -63,13 +63,20 @@ #define MAX_CONTACTS_LOC 5 #define TRIGGER_LOC 6 +/* Our special handling for GPIO accesses through ACPI is x86 specific */ +#if defined CONFIG_X86 && defined CONFIG_ACPI +#define ACPI_GPIO_SUPPORT +#endif + struct goodix_ts_data; enum goodix_irq_pin_access_method { IRQ_PIN_ACCESS_NONE, IRQ_PIN_ACCESS_GPIO, +#ifdef ACPI_GPIO_SUPPORT IRQ_PIN_ACCESS_ACPI_GPIO, IRQ_PIN_ACCESS_ACPI_METHOD, +#endif }; struct goodix_chip_data { @@ -572,8 +579,8 @@ static int goodix_send_cfg(struct goodix_ts_data *ts, const u8 *cfg, int len) static int goodix_irq_direction_output(struct goodix_ts_data *ts, int value) { - struct device *dev = &ts->client->dev; - acpi_status status; + struct device *dev __maybe_unused = &ts->client->dev; + acpi_status status __maybe_unused; switch (ts->irq_pin_access_method) { case IRQ_PIN_ACCESS_NONE: @@ -583,6 +590,7 @@ 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); +#ifdef ACPI_GPIO_SUPPORT case IRQ_PIN_ACCESS_ACPI_GPIO: /* * The IRQ pin triggers on a falling edge, so its gets marked @@ -593,6 +601,7 @@ static int goodix_irq_direction_output(struct goodix_ts_data *ts, status = acpi_execute_simple_method(ACPI_HANDLE(dev), "INTO", value); return ACPI_SUCCESS(status) ? 0 : -EIO; +#endif } return -EINVAL; /* Never reached */ @@ -600,8 +609,8 @@ static int goodix_irq_direction_output(struct goodix_ts_data *ts, static int goodix_irq_direction_input(struct goodix_ts_data *ts) { - struct device *dev = &ts->client->dev; - acpi_status status; + struct device *dev __maybe_unused = &ts->client->dev; + acpi_status status __maybe_unused; switch (ts->irq_pin_access_method) { case IRQ_PIN_ACCESS_NONE: @@ -610,12 +619,15 @@ static int goodix_irq_direction_input(struct goodix_ts_data *ts) __func__); return -EINVAL; case IRQ_PIN_ACCESS_GPIO: + return gpiod_direction_input(ts->gpiod_int); +#ifdef ACPI_GPIO_SUPPORT case IRQ_PIN_ACCESS_ACPI_GPIO: return gpiod_direction_input(ts->gpiod_int); case IRQ_PIN_ACCESS_ACPI_METHOD: status = acpi_evaluate_object(ACPI_HANDLE(dev), "INTI", NULL, NULL); return ACPI_SUCCESS(status) ? 0 : -EIO; +#endif } return -EINVAL; /* Never reached */ @@ -679,7 +691,7 @@ static int goodix_reset(struct goodix_ts_data *ts) return 0; } -#if defined CONFIG_X86 && defined CONFIG_ACPI +#ifdef ACPI_GPIO_SUPPORT #include <asm/cpu_device_id.h> #include <asm/intel-family.h> @@ -862,6 +874,7 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts) ts->gpiod_rst = gpiod; switch (ts->irq_pin_access_method) { +#ifdef ACPI_GPIO_SUPPORT case IRQ_PIN_ACCESS_ACPI_GPIO: /* * We end up here if goodix_add_acpi_gpio_mappings() has @@ -878,6 +891,7 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts) if (!ts->gpiod_rst) ts->irq_pin_access_method = IRQ_PIN_ACCESS_NONE; break; +#endif default: if (ts->gpiod_int && ts->gpiod_rst) { ts->reset_controller_at_probe = true;
acpi_evaluate_object() and acpi_execute_simple_method() are not part of the group of ACPI related functions which get stubbed by include/linux/acpi.h when ACPI support is disabled, so the IRQ_PIN_ACCESS_ACPI_METHOD handling code must be disabled through an #ifdef when ACPI support is not enabled. For consistency also #ifdef out the IRQ_PIN_ACCESS_ACPI_GPIO code and use the same #if condition as which is used to replace goodix_add_acpi_gpio_mappings with a stub. Fixes: c5fca485320e ("Input: goodix - add support for controlling the IRQ pin through ACPI methods") Reported-by: kbuild test robot <lkp@intel.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: - Introduce ACPI_GPIO_SUPPORT helper define - Use __maybe_unused for variables which will be unused when ACPI_GPIO_SUPPORT is not set --- drivers/input/touchscreen/goodix.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)