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

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

[Hans de Goede]

acpi_execute_simple_method() is 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>
---
 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;
-- 
2.26.0.rc2

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

[Bastien Nocera]

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?

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@intel.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  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;

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

[Hans de Goede]

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@intel.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   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;
> 

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

[Bastien Nocera]

On Wed, 2020-03-25 at 12:49 +0100, Hans de Goede wrote:
> 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.

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
}


> 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@intel.com>
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > >   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;

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

[Hans de Goede]

Hi,

On 3/25/20 1:11 PM, Bastien Nocera wrote:
> On Wed, 2020-03-25 at 12:49 +0100, Hans de Goede wrote:
>> 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.
> 
> 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



> 
> 
>> 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@intel.com>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>    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;
> 

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

[Bastien Nocera]

On Wed, 2020-03-25 at 14:55 +0100, Hans de Goede wrote:
> 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:

It's not only the fact that there's extra #ifdef's, it's that the
ifdef's need to be just "that". It's not "#ifdef FOO", it's "#if
defined CONFIG_X86 && defined CONFIG_ACPI".

I'd really prefer a separate function(s) that would be the only
place(s) where the conditions would be, and with one-liner bodies, to
work-around the fact that those ACPI calls are only really half-
stubbed.

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

[Hans de Goede]

Hi,

On 3/25/20 3:02 PM, Bastien Nocera wrote:
> On Wed, 2020-03-25 at 14:55 +0100, Hans de Goede wrote:
>> 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:
> 
> It's not only the fact that there's extra #ifdef's, it's that the
> ifdef's need to be just "that". It's not "#ifdef FOO", it's "#if
> defined CONFIG_X86 && defined CONFIG_ACPI".

If that is the problem I would prefer adding:

/* Our special handling for GPIO accesses through ACPI is x86 specific */
#if defined CONFIG_X86 && defined CONFIG_ACPI
#define ACPI_GPIO_SUPPORT
#endif

And use:

#ifdef ACPI_GPIO_SUPPORT

Elsewhere.

Would that work for you?

Regards,

Hans

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

[Bastien Nocera]

On Wed, 2020-03-25 at 15:05 +0100, Hans de Goede wrote:
> Hi,
> 
> On 3/25/20 3:02 PM, Bastien Nocera wrote:
> > On Wed, 2020-03-25 at 14:55 +0100, Hans de Goede wrote:
> > > 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:
> > 
> > It's not only the fact that there's extra #ifdef's, it's that the
> > ifdef's need to be just "that". It's not "#ifdef FOO", it's "#if
> > defined CONFIG_X86 && defined CONFIG_ACPI".
> 
> If that is the problem I would prefer adding:
> 
> /* Our special handling for GPIO accesses through ACPI is x86
> specific */
> #if defined CONFIG_X86 && defined CONFIG_ACPI
> #define ACPI_GPIO_SUPPORT
> #endif
> 
> And use:
> 
> #ifdef ACPI_GPIO_SUPPORT
> 
> Elsewhere.
> 
> Would that work for you?

That's slightly better, but I would still have preferred stubbing out
those ACPI calls directly. Right now, the fact that we expect half of
the commands to be stubbed out and the other half to not be called is
just weird.

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

[Hans de Goede]

Hi,

On 3/25/20 3:10 PM, Bastien Nocera wrote:
> On Wed, 2020-03-25 at 15:05 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 3/25/20 3:02 PM, Bastien Nocera wrote:
>>> On Wed, 2020-03-25 at 14:55 +0100, Hans de Goede wrote:
>>>> 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:
>>>
>>> It's not only the fact that there's extra #ifdef's, it's that the
>>> ifdef's need to be just "that". It's not "#ifdef FOO", it's "#if
>>> defined CONFIG_X86 && defined CONFIG_ACPI".
>>
>> If that is the problem I would prefer adding:
>>
>> /* Our special handling for GPIO accesses through ACPI is x86
>> specific */
>> #if defined CONFIG_X86 && defined CONFIG_ACPI
>> #define ACPI_GPIO_SUPPORT
>> #endif
>>
>> And use:
>>
>> #ifdef ACPI_GPIO_SUPPORT
>>
>> Elsewhere.
>>
>> Would that work for you?
> 
> That's slightly better, but I would still have preferred stubbing out
> those ACPI calls directly. Right now, the fact that we expect half of
> the commands to be stubbed out and the other half to not be called is
> just weird.

I agree that ideally we would have stubs for this in the include/linux/...
headers, but unfortunately that is not the case here; and fixing this
is tricky for the acpi case as explained before.

I'm not a fan of adding local stubs for this, so I'm going to go with
adding:

#if defined CONFIG_X86 && defined CONFIG_ACPI
#define ACPI_GPIO_SUPPORT
#endif

+ using __maybe_unused to avoid the extra braces for v2, and then lets
see if Dmitry has anything to add to this discussion.

Regards,

Hans