Re: [PATCH v9 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

[Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 5043 bytes --]

Hi,

On Sat, Feb 18, 2017 at 12:33:34PM +0100, H. Nikolaus Schaller wrote:
> Hi Sebastian,
> 
> > Am 18.02.2017 um 04:22 schrieb Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
> > 
> > Hi,
> > 
> > On Fri, Feb 17, 2017 at 12:40:41PM -0800, Dmitry Torokhov wrote:
> >>> AFAIK there is no mainline board using the DT except ours (and the upcoming
> >>> OMAP5-Pyra), so we shouldn't care too much. If you prefer, you can remove this
> >>> compatibility property. We don't need it for our devices.
> > 
> > $ cd linux.git/arch
> > $ git grep -l tsc2004
> > arm/boot/dts/imx6qdl-nit6xlite.dtsi
> > arm/boot/dts/imx7d-nitrogen7.dts
> > arm/boot/dts/logicpd-torpedo-37xx-devkit.dts
> > arm/boot/dts/omap4-var-som-om44.dtsi
> > $ git grep -l tsc2005
> > arm/boot/dts/omap3-n900.dts
> 
> Those are not relevant since tsc2004/5 and tsc2007 are independent drivers and don't
> share code.

Yes, I'm aware.

> Hence the N900 is not influenced by this patch series.
> If it has a similar issue, it should be fixed of course.

Right. I added them to see every board affect by the patch suggested
by me in my last paragraph.

> > $ git grep -l tsc2007
> > arm/boot/dts/imx28-tx28.dts
> > arm/boot/dts/imx35-eukrea-cpuimx35.dtsi
> > arm/boot/dts/imx51-eukrea-cpuimx51.dtsi
> > arm/boot/dts/imx53-tx53-x03x.dts
> > arm/boot/dts/imx6qdl-tx6.dtsi
> > arm/boot/dts/imx6ul-tx6ul.dtsi
> > arm/boot/dts/omap3-gta04.dtsi
> > sh/boards/mach-ecovec24/setup.c
> 
> Sorry, I was a little imprecise here, because I looked for the min/max properties.
> Of course, the imx devices use the tsc2007 as well.
>
> Maybe we should edit all these DTS and set the "ti,report-resistance" property
> by default. Then, no user should notice a difference.

I suggest to create a patch without the report-reistance stuff and
add it early after the merge window and see what happens. If no
users notices anything the change is not an ABI break from kernel's
PoV.

> Is any user/maintainer of these devices following this discussion and can comment?
>
> > 
> >> You seem to be treating DT data as something very fluid, which is wrong.
> >> You need to treat it as a firmware, unlikely to change once device is
> >> shipped. Unlike legacy platform data, the fact that DTS files are not
> >> present in mainline does not mean that we can ignore such users and
> >> change behavior at will.
> >> 
> >> That said, if driver behavior is out of line from the subsystem
> >> expectations, we need to fix it.
> >> 
> >> 
> >>> That the function name is wrong is a second issue and this double negation might
> >>> confuse a litte.
> >>> 
> >>> Please test on a real device if the patched driver reports pressure now (unless
> >>> ti,report-resistance is specified).
> >> 
> >> I unfortunately can not test this driver as I do not have the hardware.
> >> So all my observations are from code and data sheets.
> >> 
> >> That said, what is the values emitted as ABS_PRESSURE when finger is not
> >> touching the device, barely touching the device, or pressing firmly?
> >> It seems that between TSC2007, TSC2004, TSC2005, and ADS7846, we have
> >> confusion as to what is being reported.
> > 
> > As far as I can see all calculate Rtouch and ADS7846 reports
> > (Rmax - Rtouch), which looks sensible.
> 
> I don't see where this subtraction from Rmax takes place for the tsc2007:
> 
> http://lxr.free-electrons.com/source/drivers/input/touchscreen/tsc2007.c#L131

sorry if I wrote this ambiguous, let me split my sentence

1. tsc200x & ads7846 calculate Rtouch
2. ads7846 reports Rmax - Rtouch
(3. tsc200x does not, it reports Rtouch instead)
4. ads7846 behaviour looks sensible to me

> >> I am adding a few more folks to the CC so we can try and soft this out.
> >> Sebastian, Pali, Pavel, any input here?
> > 
> > I think tsc200x works, since usually userspace is Xorg and I think
> > it only cares for x/y coordinates + boolean pressure. Since
> > no-pressure is correctly reported as 0, everything works as
> > expected.
> 
> No pressure is usually treated as a special case in these drivers,
> so reduction to "boolean" in user-space works well by accident and
> might still hide a bug.

That's what I assumed.

Btw. how did you notice that tsc2007 sends "inverted" pressure values?
Just in evtest or in some non-development application? (Just asking because
the behavour obviously changes at least for that usecase)

> > I currently don't have X running on my N900 due some
> > omapdrm bug, so I can't test this, sorry.
> 
> I usually look with evtest if ABS_PRESSURE is monotonic.

That would not have helped to check if X handles the touchscreen in
a boolean way. I can provide some N900 evtest data, though (tomorrow,
I don't have my dev N900 with me at the moment).

> > I suggest to put the resistance vs pressure thing in its own patch,
> > that also fixes tsc200x-core and merge it to linux-next after the
> > merge window.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]