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

[Hans de Goede <hdegoede@xxxxxxxxxx>]

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@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;