Re: [PATCH v3 1/5] Input: goodix - reset device at init

[Bastien Nocera <hadess@hadess.net>]

On Thu, 2015-09-10 at 14:04 +0000, Tirdea, Irina wrote:
> 
> > -----Original Message-----
> > From: Bastien Nocera [mailto:hadess@hadess.net]
> > Sent: 09 September, 2015 20:03
> > To: Tirdea, Irina; linux-input@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org; Rob Herring; Pawel Moll; Ian
> > Campbell; Kumar Gala; Purdila, Octavian; Dmitry Torokhov; Mark
> > Rutland; devicetree@vger.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.

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?

Cheers