Goodix: Obviously wrong reset logic

Goodix: Obviously wrong reset logic

[Marco Felsch]

Hi all,

since linux v4.5 the goodix touch driver support the reset by commit
ec6e1b4082d9 ("Input: goodix - reset device at init"). I was a bit
confused while I read the goodix_reset() function:
8<----------------------------------------------------------------------
static int goodix_reset(struct goodix_ts_data *ts)                                                                                      
{              
	int error;

	/* begin select I2C slave addr */
	error = gpiod_direction_output(ts->gpiod_rst, 0);
	if (error)
		return error;

	msleep(20);				/* T2: > 10ms */                                                                        

	/* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */                                                                                           
	error = goodix_irq_direction_output(ts, ts->client->addr == 0x14);
	(error)
		return error;

	usleep_range(100, 2000);		/* T3: > 100us */

	error = gpiod_direction_output(ts->gpiod_rst, 1);
	if (error)
		return error;
	
	...
}
8<----------------------------------------------------------------------
because the gpiod_direction_output() takes the logical level and and not
the physical level. Also it is not intuitive to read. Releasing the reset
line first and set it after?

As pointed out by the commit message, the reset logic is based on the
datasheet GT911 [1] and GT9271[2]. Those datasheets says that the reset
pin is active low. Setting this in my DT-based board will break
everything. I checked the DT's already using a goodix device and all of
them are specifying the pin as active-high. IMHO this is wrong.

Now the question is: Does the ACPI tables specify it as active high too
and was this the reason to miss-use the gpiod_ API? If this is the case
fixing the driver would break everything and in that case we should at
least mention it within the driver and within the DT-Bindings.

Regards,
  Marco

Re: Goodix: Obviously wrong reset logic

[Hans de Goede]

Hi,

On 8/19/20 11:43 AM, Marco Felsch wrote:
> Hi all,
> 
> since linux v4.5 the goodix touch driver support the reset by commit
> ec6e1b4082d9 ("Input: goodix - reset device at init"). I was a bit
> confused while I read the goodix_reset() function:
> 8<----------------------------------------------------------------------
> static int goodix_reset(struct goodix_ts_data *ts)
> {
> 	int error;
> 
> 	/* begin select I2C slave addr */
> 	error = gpiod_direction_output(ts->gpiod_rst, 0);
> 	if (error)
> 		return error;
> 
> 	msleep(20);				/* T2: > 10ms */
> 
> 	/* HIGH: 0x28/0x29, LOW: 0xBA/0xBB */
> 	error = goodix_irq_direction_output(ts, ts->client->addr == 0x14);
> 	(error)
> 		return error;
> 
> 	usleep_range(100, 2000);		/* T3: > 100us */
> 
> 	error = gpiod_direction_output(ts->gpiod_rst, 1);
> 	if (error)
> 		return error;
> 	
> 	...
> }
> 8<----------------------------------------------------------------------
> because the gpiod_direction_output() takes the logical level and and not
> the physical level. Also it is not intuitive to read. Releasing the reset
> line first and set it after?
> 
> As pointed out by the commit message, the reset logic is based on the
> datasheet GT911 [1] and GT9271[2]. Those datasheets says that the reset
> pin is active low. Setting this in my DT-based board will break
> everything. I checked the DT's already using a goodix device and all of
> them are specifying the pin as active-high. IMHO this is wrong.
> 
> Now the question is: Does the ACPI tables specify it as active high too
> and was this the reason to miss-use the gpiod_ API? If this is the case
> fixing the driver would break everything and in that case we should at
> least mention it within the driver and within the DT-Bindings.

Until recently the goodix code did not use the GPIOs in the ACPI case at
all.

I've recently fixed this and now there is a helper for setting the
direction + output value of the GPIO. This is done though a helper
since on some ACPI platforms the IRQ is modeled as a "direct" IRQ not a
GpioInt, with ACPI methods for setting the direction + value.

This new helper looks like this:

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);
         case IRQ_PIN_ACCESS_ACPI_GPIO:
                 /*
                  * The IRQ pin triggers on a falling edge, so its gets marked
                  * as active-low, use output_raw to avoid the value inversion.
                  */
                 return gpiod_direction_output_raw(ts->gpiod_int, value);
         case IRQ_PIN_ACCESS_ACPI_METHOD:
                 return goodix_pin_acpi_output_method(ts, value);
         }

         return -EINVAL; /* Never reached */
}

Note how in the ACPI case gpiod_direction_output_raw is used to avoid the
problem you mention. The ACPI table correctly marks the GpioInt (in case
where a GpioInt resource is used) as active low, so I made
goodix_irq_direction_output() use gpiod_direction_output_raw() when the GPIOs
come from ACPI to avoid the exact problem you describe.

If all current DTS users wrongly specify the pin as active-high, as the code
expects, then indeed if you mark it active-low then things will break.

The problem is you cannot just change this, as that will break existing
dts/dtb files some of which may not be part of the kernel...

I guess one option would be to do the same thing as I'm doing in the
devicetree case, and use gpiod_direction_output_raw() ignoring the
active high/low specified in dt/acpi. I'm not sure if that is a good idea
but it is an option.

FWIW I very carefully modeled my patch-series for adding support for the
GPIOs under ACPI to not make any functional changes to the DT case to
avoid regressions. It might be best for you to also play it safe here
and just make the pin active-high in the dts file, maybe with a comment
that that is wrong but it is what the driver expects. Then you don't
run the risk of breaking other peoples working setups.

Regards,

Hans