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

[Hans de Goede <hdegoede@redhat.com>]

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@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:".

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 | }
       | ^

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.

Regards,

Hans