Message-ID: <1443492265.17930.9.camel@hadess.net>
Date:	Tue, 29 Sep 2015 04:04:25 +0200
From:	Bastien Nocera <hadess@...ess.net>
To:	"Tirdea, Irina" <irina.tirdea@...el.com>,
	"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	"Purdila, Octavian" <octavian.purdila@...el.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Mark Rutland <mark.rutland@....com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH v3 1/5] Input: goodix - reset device at init

On Fri, 2015-09-25 at 21:04 +0000, Tirdea, Irina wrote:
> 
> > -----Original Message-----
> > From: Bastien Nocera [mailto:hadess@...ess.net]
> > Sent: 25 September, 2015 17:44
> > To: Tirdea, Irina; linux-input@...r.kernel.org
> > Cc: linux-kernel@...r.kernel.org; Rob Herring; Pawel Moll; Ian
> > Campbell; Kumar Gala; Purdila, Octavian; Dmitry Torokhov; Mark
> > Rutland; devicetree@...r.kernel.org
> > Subject: Re: [PATCH v3 1/5] Input: goodix - reset device at init
> > 
> > On Thu, 2015-09-10 at 14:04 +0000, Tirdea, Irina wrote:
> > > 
> > > > -----Original Message-----
> > > > From: Bastien Nocera [mailto:hadess@...ess.net]
> > > > Sent: 09 September, 2015 20:03
> > > > To: Tirdea, Irina; linux-input@...r.kernel.org
> > > > Cc: linux-kernel@...r.kernel.org; Rob Herring; Pawel Moll; Ian
> > > > Campbell; Kumar Gala; Purdila, Octavian; Dmitry Torokhov; Mark
> > > > Rutland; devicetree@...r.kernel.org
> > > > Subject: Re: [PATCH v3 1/5] Input: goodix - reset device at
> > > > init
> > > > 
> > > > On Thu, 2015-07-30 at 11:27 +0000, Tirdea, Irina wrote:
> > > > > I can send some additional patches that will simplify testing
> > > > > the
> > > > > configuration update to the Goodix device. I think this
> > > > > feature
> > > > > is
> > > > > the easiest
> > > > > to test so we can determine if writing to the interrupt pin
> > > > > actually
> > > > > works.
> > > > > However, even if it is a BIOS problem and the code will work,
> > > > > the
> > > > > warning
> > > > > will still be printed in dmesg.
> > > > 
> > > > 
> > > > Somehow missed this mail before replying to the current
> > > > patchset.
> > > > I'd
> > > > be fine with that, though it's still not clear to me whether
> > > > the
> > > > BIOS/hardware is at fault, or the code that's being added to
> > > > the
> > > > driver
> > > > ;)
> > > > 
> > > 
> > > The reset procedure is described in the Goodix GT911 datasheet
> > > [1]
> > > and is
> > > used for power-on reset and power management. The power-on
> > > sequence
> > > is described in chapter 6.1. I2C Timing, in the Power-on Timing
> > > diagram.
> > > The sequence for putting the device to sleep is described in
> > > chapter
> > > 7.1. Operating Modes, c) Sleep mode. These sequences use the
> > > interrupt
> > > pin as output.
> > > 
> > > The warning you mentioned comes from the following code in the
> > > goodix
> > > driver, which sets the interrupt to be used as output:
> > > 
> > > +       error = gpiod_direction_output(ts->gpiod_int, ts->client-
> > > > addr == 0x14);
> > > 
> > > The gpiod_direction_output() call ends up in
> > > drivers/pinctrl/intel/pinctrl-baytrail.c:
> > > /*
> > >  * Before making any direction modifications, do a check if gpio
> > >  * is set for direct IRQ.  On baytrail, setting GPIO to output
> > > does
> > >  * not make sense, so let's at least warn the caller before they
> > > shoot
> > >  * themselves in the foot.
> > >  */
> > > WARN(readl(conf_reg) & BYT_DIRECT_IRQ_EN,
> > > 	"Potential Error: Setting GPIO with direct_irq_en to output");
> > > 
> > > So the problem comes from using the gpio interrupt pin as output,
> > > which
> > > should not work on Baytrail if BYT_DIRECT_IRQ_EN is set by BIOS.
> > > The above warning is introduced and discussed in [2] and [3]. As
> > > I
> > > mentioned,
> > > this could be a real HW issue or the BIOS sets BYT_DIRECT_IRQ_EN
> > > when
> > > it should not. I have also tested these patches on a Baytrail
> > > plarform
> > > (that uses the same pinctrl driver), but I did not see any issues
> > > since
> > > BYT_DIRECT_IRQ_EN is not set in my case for the interrupt gpio
> > > pin.
> > 
> > Do we have a way to work-around this in the GPIO driver?
> > 
> > > To determine if using the interrupt pin as output works, you can
> > > test
> > > updating
> > > the goodix configuration [4].
> > 
> > Right, the problem being that it's a later patch in the branch, and
> > that the driver will fail to probe if there's an error from the
> > GPIO
> > call.
> > 
> 
> The warning from your dmesg output will not cause probe to fail.
> If you look at the code for byt_gpio_direction_output, it will just
> print
> a warning and continue [1]:
> 	WARN(readl(conf_reg) & BYT_DIRECT_IRQ_EN,
> 		"Potential Error: Setting GPIO with direct_irq_en to
> output");
> I thought probe finishes successfully, but due to the warning in
> dmesg you
> are not sure whether the IRQ GPIO pin can be used as output.
> If probe fails, it must be for another reason than the direct_irq_en
> warning.
> 
> > Would you have a patch for me to test that would bypass this error,
> > or
> > at least fallback gracefully to not resetting, not probing GPIOs if
> > they're badly setup?
> 
> If the driver fails to initialize the GPIOs, it will at least print
> some
> "Failed to get GPIO" warnings in dmesg. Do you have such messages in
> dmesg or any additional information on why probe fails?
> 
> The current code will ignore GPIOs if they are not defined in ACPI
> (see the check for -ENOENT), but does not ignore other error codes.
> If you want to bypass all GPIO errors, you can use the code below.

The failure isn't there, it's when running goodix_i2c_test():
Sep 25 16:39:20 winbook kernel: Goodix-TS i2c-GDIX1001:00: i2c test failed attempt 1: -121
Sep 25 16:39:20 winbook kernel: Goodix-TS i2c-GDIX1001:00: i2c test failed attempt 2: -121
Sep 25 16:39:20 winbook kernel: Goodix-TS i2c-GDIX1001:00: I2C communication failure: -121
Sep 25 16:39:20 winbook kernel: Goodix-TS: probe of i2c-GDIX1001:00 failed with error -121

The GPIO setup seems to work (bar the warnings), and the reset as well,
but then the device fails to communicate. Likely a fallout from the
reset actually failing.

Swapping around the RST and INT pins leads to the same problem. Either
this device's GPIO PINs aren't actually functional, and the firmware
contains garbage, or something else is wrong.

I'm not sure how we can detect, and blacklist, those devices. At least
my original device, the Onda v975w, and the WinBook TW100 would have
those problems.

Cheers
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists