Re: [PATCH] Input: goodix - Fix compilation when ACPI support is disabled

Hans de Goede <hdegoede@xxxxxxxxxx> · Wed, 25 Mar 2020 12:49:14 +0100

Hi,

On 3/25/20 11:47 AM, Bastien Nocera wrote:
On Wed, 2020-03-25 at 11:33 +0100, Hans de Goede wrote:
acpi_execute_simple_method() is not part of the group of ACPI

Could we not stub acpi_execute_simple_method() either in acpi.h or in
this driver, and make it return -EINVAL?

We have 2 troublesome calls:

acpi_execute_simple_method() in goodix_irq_direction_output()
acpi_evaluate_object() in goodix_irq_direction_input()

The second one was not in the build-bot output because the
build-bot never got around to linking and its prototype is
brought in even when CONFIG_ACPI is not set.

This also immediately introduces a problem with adding
a stub for this one. We cannot have a static stub in
goodix.c for it because that will trigger warnings after
include/acpi/acpixf.h first having it declared public.

We could add a non static stub, but that just feels wrong.

Doing a static inline stub in include/linux/acpi.h also is
not possible for the same reason.  That would leave adding
#ifdef with a stub to include/acpi/acpixf.h, but that is
not going to fly either because the headers under include/acpi
are part of the ACPICA project:
https://github.com/acpica/acpica

Which is OS independent and the kernel syncs the files from
it once each cycle, so we cannot make Linux specific changes
to those headers.

So all in all I believe that #ifdef is the best solution.

Also note that all the #ifdef-s are in switch-case and cover
whole cases, so they are pretty clean IMHO.

As for the braces, alternatively we could keep the variables
at the top of the goodix_irq_direction_[in|out]put functions
and mark the as __maybe_unused, then the extra braces this
change introduces goes away.

Regards,

Hans





There's already a stub to avoid those 2 access methods from being used,
and I'd prefer a little more code to more ifdef-spaghetti, or awkwardly
placed curly braces.

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@xxxxxxxxx>
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
  drivers/input/touchscreen/goodix.c | 27 +++++++++++++++++++--------
  1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/input/touchscreen/goodix.c
b/drivers/input/touchscreen/goodix.c
index 2c9cd1bfb860..a593ca38b35b 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -68,8 +68,10 @@ struct goodix_ts_data;
  enum goodix_irq_pin_access_method {
  	IRQ_PIN_ACCESS_NONE,
  	IRQ_PIN_ACCESS_GPIO,
+#if defined CONFIG_X86 && defined CONFIG_ACPI
  	IRQ_PIN_ACCESS_ACPI_GPIO,
  	IRQ_PIN_ACCESS_ACPI_METHOD,
+#endif
  };
  
  struct goodix_chip_data {
@@ -572,9 +574,6 @@ 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;
-
  	switch (ts->irq_pin_access_method) {
  	case IRQ_PIN_ACCESS_NONE:
  		dev_err(&ts->client->dev,
@@ -583,26 +582,29 @@ 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);
+#if defined CONFIG_X86 && defined CONFIG_ACPI
  	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);
-	case IRQ_PIN_ACCESS_ACPI_METHOD:
+	case IRQ_PIN_ACCESS_ACPI_METHOD: {
+		struct device *dev = &ts->client->dev;
+		acpi_status status;
+
  		status = acpi_execute_simple_method(ACPI_HANDLE(dev),
  						    "INTO", value);
  		return ACPI_SUCCESS(status) ? 0 : -EIO;
  	}
+#endif
+	}
  
  	return -EINVAL; /* Never reached */
  }
  
  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,
@@ -610,13 +612,20 @@ 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);
+#if defined CONFIG_X86 && defined CONFIG_ACPI
  	case IRQ_PIN_ACCESS_ACPI_GPIO:
  		return gpiod_direction_input(ts->gpiod_int);
-	case IRQ_PIN_ACCESS_ACPI_METHOD:
+	case IRQ_PIN_ACCESS_ACPI_METHOD: {
+		struct device *dev = &ts->client->dev;
+		acpi_status status;
+
  		status = acpi_evaluate_object(ACPI_HANDLE(dev), "INTI",
  					      NULL, NULL);
  		return ACPI_SUCCESS(status) ? 0 : -EIO;
  	}
+#endif
+	}
  
  	return -EINVAL; /* Never reached */
  }
@@ -862,6 +871,7 @@ static int goodix_get_gpio_config(struct
goodix_ts_data *ts)
  	ts->gpiod_rst = gpiod;
  
  	switch (ts->irq_pin_access_method) {
+#if defined CONFIG_X86 && defined CONFIG_ACPI
  	case IRQ_PIN_ACCESS_ACPI_GPIO:
  		/*
  		 * We end up here if goodix_add_acpi_gpio_mappings()
has
@@ -878,6 +888,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;








