Hi,

On 3/25/20 1:11 PM, Bastien Nocera wrote:

On Wed, 2020-03-25 at 12:49 +0100, Hans de Goede wrote:

quoted

Hi,

On 3/25/20 11:47 AM, Bastien Nocera wrote:

quoted

On Wed, 2020-03-25 at 11:33 +0100, Hans de Goede wrote:

quoted

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.

And we can't do something like that?

static acpi_status
goodix_acpi_execute_simple_method (...)
{
#ifdef whatever
   return acpi_execute_simple_method(....);
#else
   return -EINVAL;
#endif
}

We could do something like that, but TBH I'm not a fan of that
adding extra wrappers makes it harder to see what the code is
actually doing.

I understand your dislike for the extra braces I added and
I'm fine with fixing that by adding __maybe_unused to the
variable declarations at the top. I don't really see what
the problem with the #ifdef-s is given how clean they are,
with the braces thing fixed by using __maybe_unused things
would look like e.g. this:

	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);
#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:
		status = acpi_evaluate_object(ACPI_HANDLE(dev), "INTI",
					      NULL, NULL);
		return ACPI_SUCCESS(status) ? 0 : -EIO;
#endif
	}

IMHO that looks just fine, it is not as if the #if makes it really
hard to read things and / or if there are all sort of weird
constructs necessary to make things work with the #if.

Regards,

Hans

quoted

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

quoted

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.

quoted

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 <redacted>
Signed-off-by: Hans de Goede <redacted>
---
   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-

quoted

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;