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

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

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

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