On Fri, Mar 06, 2020 at 09:54:26AM +0100, Hans de Goede wrote: > Hi, > > On 3/6/20 5:03 AM, Dmitry Torokhov wrote: > > Hi Hans, > > > > On Thu, Mar 05, 2020 at 11:01:22PM +0100, Hans de Goede wrote: > > > Suspending Goodix touchscreens requires changing the interrupt pin to > > > output before sending them a power-down command. Followed by wiggling > > > the interrupt pin to wake the device up, after which it is put back > > > in input mode. > > > > > > So far we have only effectively supported this on devices which use > > > devicetree. On X86 ACPI platforms both looking up the pins; and using a > > > pin as both IRQ and GPIO is a bit more complicated. E.g. on some devices > > > we cannot directly access the IRQ pin as GPIO and we need to call ACPI > > > methods to control it instead. > > > > > > This commit adds a new irq_pin_access_method field to the goodix_chip_data > > > struct and adds goodix_irq_direction_output and goodix_irq_direction_input > > > helpers which together abstract the GPIO accesses to the IRQ pin. > > > > > > This is a preparation patch for adding support for properly suspending the > > > touchscreen on X86 ACPI platforms. > > > > > > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1786317 > > > BugLink: https://github.com/nexus511/gpd-ubuntu-packages/issues/10 > > > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=199207 > > > Cc: Dmitry Mastykin <mastichi@xxxxxxxxx> > > > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > > > --- > > > Changes in v2: > > > - Make enum member names upper-case > > > --- > > > drivers/input/touchscreen/goodix.c | 62 ++++++++++++++++++++++++------ > > > 1 file changed, 51 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c > > > index 0403102e807e..9cfbcf3cbca8 100644 > > > --- a/drivers/input/touchscreen/goodix.c > > > +++ b/drivers/input/touchscreen/goodix.c > > > @@ -31,6 +31,11 @@ > > > struct goodix_ts_data; > > > +enum goodix_irq_pin_access_method { > > > + IRQ_PIN_ACCESS_NONE, > > > + IRQ_PIN_ACCESS_GPIO, > > > +}; > > > + > > > struct goodix_chip_data { > > > u16 config_addr; > > > int config_len; > > > @@ -53,6 +58,7 @@ struct goodix_ts_data { > > > const char *cfg_name; > > > struct completion firmware_loading_complete; > > > unsigned long irq_flags; > > > + enum goodix_irq_pin_access_method irq_pin_access_method; > > > unsigned int contact_size; > > > }; > > > @@ -502,17 +508,48 @@ static int goodix_send_cfg(struct goodix_ts_data *ts, > > > return 0; > > > } > > > +static int goodix_irq_direction_output(struct goodix_ts_data *ts, > > > + int value) > > > +{ > > > + 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_output(ts->gpiod_int, value); > > > + } > > > + > > > + return -EINVAL; /* Never reached */ > > > > I do not like these "never reached" comments. We can either let compiler > > issue a warning that we did not cover all switch cases, or stick > > "default:" alongside "case IRQ_PIN_ACCESS_NONE:". > > I just tried removing this line, this results in: > > CC [M] drivers/input/touchscreen/goodix.o > drivers/input/touchscreen/goodix.c: In function ‘goodix_irq_direction_output’: > drivers/input/touchscreen/goodix.c:593:1: warning: control reaches end of non-void function [-Wreturn-type] > 593 | } > | ^ Ah, right, we use C, not C++ ;) > > And I do not want to add a default label, the switch-case is on an enum type and > if I do that I loose the useful warnings for one of the enum values not being > handled in the switch-case. OK, fair enough. Thanks. -- Dmitry
