Re: [PATCH resend 01/10] Input: goodix - Refactor IRQ pin GPIO accesses

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

Hi,

On 3/2/20 12:09 PM, Bastien Nocera wrote:
> On Fri, 2020-02-21 at 17:47 +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>
>> ---
>>   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..08806a00a9b9 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,
> 
> I don't know if that matches the kernel coding style, but I'd rather
> the enum member names were upper-case, and typedef'ed.

I checked and most of the kernel also uses upper-case for
enum member names, I'll fix this (for the entire series) for
v2 of the series.

As for using typedef-s that is typically not done in the kernel
for enums / structs so I'm going to keep that as is.


> 
>> +};
>> +
>>   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);
> 
> Is that going to complain about default not being handled? If so, an if
> conditional might be enough.

All the values in the enum are handled so there is no need for a default
label. As for changing this to an if, later patches add more values
to the enum and to the switch-case, having this as a switch-case from
the start makes the diff in later patches smaller.

> 
>> +	}
>> +
>> +	return -EINVAL; /* Never reached */
>> +}
>> +
>> +static int goodix_irq_direction_input(struct goodix_ts_data *ts)
>> +{
>> +	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);
> 
> Ditto.

Ditto.

> Looks fine otherwise.

Thank you for the review.

Regards,

Hans



> 
> 
>> +	}
>> +
>> +	return -EINVAL; /* Never reached */
>> +}
>> +
>>   static int goodix_int_sync(struct goodix_ts_data *ts)
>>   {
>>   	int error;
>>   
>> -	error = gpiod_direction_output(ts->gpiod_int, 0);
>> +	error = goodix_irq_direction_output(ts, 0);
>>   	if (error)
>>   		return error;
>>   
>>   	msleep(50);				/* T5: 50ms */
>>   
>> -	error = gpiod_direction_input(ts->gpiod_int);
>> +	error = goodix_irq_direction_input(ts);
>>   	if (error)
>>   		return error;
>>   
>> @@ -536,7 +573,7 @@ static int goodix_reset(struct goodix_ts_data
>> *ts)
>>   	msleep(20);				/* T2: > 10ms */
>>   
>>   	/* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
>> -	error = gpiod_direction_output(ts->gpiod_int, ts->client->addr
>> == 0x14);
>> +	error = goodix_irq_direction_output(ts, ts->client->addr ==
>> 0x14);
>>   	if (error)
>>   		return error;
>>   
>> @@ -617,6 +654,9 @@ static int goodix_get_gpio_config(struct
>> goodix_ts_data *ts)
>>   
>>   	ts->gpiod_rst = gpiod;
>>   
>> +	if (ts->gpiod_int && ts->gpiod_rst)
>> +		ts->irq_pin_access_method = irq_pin_access_gpio;
>> +
>>   	return 0;
>>   }
>>   
>> @@ -889,7 +929,7 @@ static int goodix_ts_probe(struct i2c_client
>> *client,
>>   	if (error)
>>   		return error;
>>   
>> -	if (ts->gpiod_int && ts->gpiod_rst) {
>> +	if (ts->irq_pin_access_method == irq_pin_access_gpio) {
>>   		/* reset the controller */
>>   		error = goodix_reset(ts);
>>   		if (error) {
>> @@ -912,7 +952,7 @@ static int goodix_ts_probe(struct i2c_client
>> *client,
>>   
>>   	ts->chip = goodix_get_chip_data(ts->id);
>>   
>> -	if (ts->gpiod_int && ts->gpiod_rst) {
>> +	if (ts->irq_pin_access_method == irq_pin_access_gpio) {
>>   		/* update device config */
>>   		ts->cfg_name = devm_kasprintf(&client->dev, GFP_KERNEL,
>>   					      "goodix_%d_cfg.bin", ts-
>>> id);
>> @@ -943,7 +983,7 @@ static int goodix_ts_remove(struct i2c_client
>> *client)
>>   {
>>   	struct goodix_ts_data *ts = i2c_get_clientdata(client);
>>   
>> -	if (ts->gpiod_int && ts->gpiod_rst)
>> +	if (ts->irq_pin_access_method == irq_pin_access_gpio)
>>   		wait_for_completion(&ts->firmware_loading_complete);
>>   
>>   	return 0;
>> @@ -956,7 +996,7 @@ static int __maybe_unused goodix_suspend(struct
>> device *dev)
>>   	int error;
>>   
>>   	/* We need gpio pins to suspend/resume */
>> -	if (!ts->gpiod_int || !ts->gpiod_rst) {
>> +	if (ts->irq_pin_access_method == irq_pin_access_none) {
>>   		disable_irq(client->irq);
>>   		return 0;
>>   	}
>> @@ -967,7 +1007,7 @@ static int __maybe_unused goodix_suspend(struct
>> device *dev)
>>   	goodix_free_irq(ts);
>>   
>>   	/* Output LOW on the INT pin for 5 ms */
>> -	error = gpiod_direction_output(ts->gpiod_int, 0);
>> +	error = goodix_irq_direction_output(ts, 0);
>>   	if (error) {
>>   		goodix_request_irq(ts);
>>   		return error;
>> @@ -979,7 +1019,7 @@ static int __maybe_unused goodix_suspend(struct
>> device *dev)
>>   				    GOODIX_CMD_SCREEN_OFF);
>>   	if (error) {
>>   		dev_err(&ts->client->dev, "Screen off command
>> failed\n");
>> -		gpiod_direction_input(ts->gpiod_int);
>> +		goodix_irq_direction_input(ts);
>>   		goodix_request_irq(ts);
>>   		return -EAGAIN;
>>   	}
>> @@ -999,7 +1039,7 @@ static int __maybe_unused goodix_resume(struct
>> device *dev)
>>   	struct goodix_ts_data *ts = i2c_get_clientdata(client);
>>   	int error;
>>   
>> -	if (!ts->gpiod_int || !ts->gpiod_rst) {
>> +	if (ts->irq_pin_access_method == irq_pin_access_none) {
>>   		enable_irq(client->irq);
>>   		return 0;
>>   	}
>> @@ -1008,7 +1048,7 @@ static int __maybe_unused goodix_resume(struct
>> device *dev)
>>   	 * Exit sleep mode by outputting HIGH level to INT pin
>>   	 * for 2ms~5ms.
>>   	 */
>> -	error = gpiod_direction_output(ts->gpiod_int, 1);
>> +	error = goodix_irq_direction_output(ts, 1);
>>   	if (error)
>>   		return error;
>>   
>