Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen

[Bjorn Andersson]

On Fri, Jan 26, 2024 at 01:02:32PM +0000, Daniel Thompson wrote:
> On Fri, Jan 26, 2024 at 09:12:37AM +0100, Johan Hovold wrote:
> > On Thu, Jan 25, 2024 at 07:55:14PM -0800, Bjorn Andersson wrote:
> > > The failing read-test in __i2c_hid_core_probe() determines that there's
> > > nothing connected at the documented address of the touchscreen.
> > >
> > > Introduce the 5ms after-power and 200ms after-reset delays found in the
> > > ACPI tables. Also wire up the reset-gpio, for good measure.
> >
> > As the supplies for the touchscreen are always on (and left on by the
> > bootloader) it would seem that it is really the addition of the reset
> > gpio which makes things work here. Unless the delay is needed for some
> > other reason.
> >
> > (The power-on delay also looks a bit short compared to what is used for
> > other devices.)
> >
> > Reset support was only recently added with commit 2be404486c05 ("HID:
> > i2c-hid-of: Add reset GPIO support to i2c-hid-of") so we should not
> > backport this one before first determining that.
> 
> This comment attracted my attention so I tried booting with each of the
> three lines individually.
> 
> 
> On Thu, Jan 25, 2024 at 07:55:14PM -0800, Bjorn Andersson wrote:
> > +             reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
> 
> This is not enough, on it's own, to get the touch screen running.
> 

No, because pinctrl already brings the chip out of reset without this.

> I guess that's not so much of a surprise since the rebind-the-driver
> from userspace trick wouldn't have been touching this reset.
> 

Right, it would just have been left deasserted from the first attempt.

That said, the addition of the reset means that we're now asserting
reset in such rebind attempts. And as such the
post-reset-deassert-delay-ms is now needed between the explicit deassert
from the driver and the i2c read.

> 
> > +             post-power-on-delay-ms = <5>;
> 
> This line alone is enough (in v6.7.1).
> 

So the delay taken through really_probe() until we reach that i2c read
is almost the entire delay needed, on your specific device.

> 
> > +             post-reset-deassert-delay-ms = <200>;
> 
> This line alone is also enough!
> 
> In short it looks like the delays make the difference and, even a short
> delay, can fix the problem.
> 
> Of course, regardless of the line-by-line results I also ran with all
> the changes so, FWIW:
> Tested-by: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
> 

Thanks,
Bjorn

> 
> Daniel.