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

[H. Nikolaus Schaller]

Hi Sebastian,

> Am 19.02.2017 um 00:44 schrieb Sebastian Reichel <sre@xxxxxxxxxx>:
> 
> 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@xxxxxxxxxx>:
>>> 
>>> 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.

Ok!

> 
>>> $ 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.

That looks like a good strategy.

> 
>> 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

agreed.

> 
>>>> 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 don't really remember when we noticed it first. Maybe it was back in tslib times
some years ago where setting the sensitivity threshold made problems. We then
carried along our patch for a long time in our local repo (and modified it
several times) and only started upstreaming some months ago. So it was included
but we never thought about it being something as important as the pre-calibration
which is really a benefit for setup and immediate useability of the whole system.
Hence we buried it a little in this pre-calibration, flipping and rotation patch.

> 
>>> 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).

AFAIK, GIMP and for example https://sourceforge.net/projects/xournal/ appear
to be able to handle X pressure, but I haven't running and tested either one
on our devices. Pressure is used in such drawing tools to simulate that some
physical pens make wider strokes on higher pressure.

This seems to indicate that X can handle pressure in a non-boolean way, but rarely does.
Especially I think the usual menu, click, drag, scroll gestures are only based
on BTN_TOUCH status and not on ABS_PRESSURE. So it is rarely noticed to make
a difference.

> 
>>> 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.

Ok. I will propose a patch.

BR,
Nikolaus

Attachment: signature.asc
Description: Message signed with OpenPGP using GPGMail