Re: [PATCH v2 01/11] Input: goodix - Refactor IRQ pin GPIO accesses

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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

Thanks.

-- 
Dmitry