| Message ID | 20200325103348.108792-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Input: goodix - Fix compilation when ACPI support is disabled | expand |
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;
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; >
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;
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; >
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.
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
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.
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
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;
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(-)