Message-Id: <E8C59C6C-23D4-4417-8A9E-3E9AE5D2D100@goldelico.com>
Date:   Sat, 24 Sep 2016 07:55:27 +0200
From:   "H. Nikolaus Schaller" <hns@...delico.com>
To:     Sebastian Reichel <sre@...nel.org>
Cc:     Rob Herring <robh@...nel.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Mark Rutland <mark.rutland@....com>,
        Benoît Cousson <bcousson@...libre.com>,
        Tony Lindgren <tony@...mide.com>,
        Russell King <linux@...linux.org.uk>,
        Arnd Bergmann <arnd@...db.de>,
        Michael Welling <mwelling@...e.org>,
        Mika Penttilä <mika.penttila@...tfour.com>,
        Javier Martinez Canillas <javier@....samsung.com>,
        Igor Grinberg <grinberg@...pulab.co.il>,
        "Andrew F. Davis" <afd@...com>, linux-input@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-omap@...r.kernel.org, letux-kernel@...nphoenux.org,
        linux-iio@...r.kernel.org
Subject: Re: [PATCH v3 1/8] drivers:input:tsc2007: add new common binding names, pre-calibration, flipping and rotation

Hi,

> Am 24.09.2016 um 02:31 schrieb Sebastian Reichel <sre@...nel.org>:
> 
> On Fri, Sep 23, 2016 at 05:47:26PM -0500, Rob Herring wrote:
>> On Fri, Sep 23, 2016 at 02:41:09PM +0200, H. Nikolaus Schaller wrote:
>>> commit b98abe52fa8e ("Input: add common DT binding for touchscreens")
>>> introduced common DT bindings for touchscreens [1] and a helper function to
>>> parse the DT.
>>> 
>>> commit ed7c9870c9bc ("Input: of_touchscreen - add support for inverted / swapped axes")
>>> added another helper for parsing axis inversion and swapping
>>> and applying them to x and y coordinates.
>>> 
>>> Both helpers have been integrated to accommodate any orientation of the
>>> touch panel in relation to the LCD.
>>> 
>>> A new feature is to introduce scaling the min/max ADC values to the screen
>>> size.
>>> 
>>> This makes it possible to pre-calibrate the touch so that is (almost)
>>> exactly matches the LCD pixel coordinates it is glued onto. This allows to
>>> well enough operate the touch before a user space calibration step can
>>> improve the precision.
>>> 
>>> Finally, calculate_pressure has been renamed to calculate_resistance
>>> because that is what it is doing.
>> 
>> Seems like you are breaking compatibility with old DTs. I can't tell for
>> sure though.
>> 
>>> 
>>> [1]: Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt
>>> 
>>> Signed-off-by: H. Nikolaus Schaller <hns@...delico.com>
>>> ---
>>> .../bindings/input/touchscreen/tsc2007.txt         |  20 ++--
>>> drivers/input/touchscreen/tsc2007.c                | 126 +++++++++++++++++----
>>> include/linux/i2c/tsc2007.h                        |   8 ++
>>> 3 files changed, 123 insertions(+), 31 deletions(-)
>>> 
>>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
>>> index ec365e1..6e9fd55 100644
>>> --- a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
>>> +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
>>> @@ -6,6 +6,7 @@ Required properties:
>>> - ti,x-plate-ohms: X-plate resistance in ohms.
>>> 
>>> Optional properties:
>>> +- generic touch screen properties: see touchscreen binding [2].
>>> - gpios: the interrupt gpio the chip is connected to (trough the penirq pin).
>>>   The penirq pin goes to low when the panel is touched.
>>>   (see GPIO binding[1] for more details).
>>> @@ -13,17 +14,20 @@ Optional properties:
>>>   (see interrupt binding[0]).
>>> - interrupts: (gpio) interrupt to which the chip is connected
>>>   (see interrupt binding[0]).
>>> -- ti,max-rt: maximum pressure.
>>> -- ti,fuzzx: specifies the absolute input fuzz x value.
>>> -  If set, it will permit noise in the data up to +- the value given to the fuzz
>>> -  parameter, that is used to filter noise from the event stream.
>>> -- ti,fuzzy: specifies the absolute input fuzz y value.
>>> -- ti,fuzzz: specifies the absolute input fuzz z value.
>>> +- ti,max-rt: maximum pressure resistance above which samples are ignored
>>> +  (default: 4095).
>>> +- ti,report-resistance: report resistance (no pressure = max_rt) instead
>>> +  of pressure (no pressure = 0).
>>> +- ti,min-x: minimum value reported by X axis ADC (default 0).
>>> +- ti,max-x: maximum value reported by X axis ADC (default 4095).
>>> +- ti,min-y: minimum value reported by Y axis ADC (default 0).
>>> +- ti,max-y: maximum value reported by Y axis ADC (default 4095).
>> 
>> Seems like these could be common too. They make more sense than giving x
>> and y sizes in pixel units which really should come from the panel.
> 
> The generic touchscreen properties are described "(in pixels)" in
> the DT, but they are used in the same way.
> 
> So ti,max-[xy] is basically the same as touchscreen-size-[xy],

No it is not the same and should be kept separate.

> except, that the generic bindings don't support min-[xy] != 0.

What would be the purpose of this? Every user-space I know
about (X11, Replicant) expects coordinates in some range
0..max so setting min in device tree makes no sense to me.

> 
> So maybe change the generic bindings like this:
> 
> touchscreen-min-x: minimum value reported by X axis ADC (default 0)
> touchscreen-max-x: maximum value reported by Y axis ADC
> touchscreen-min-y: minimum value reported by Y axis ADC (default 0)
> touchscreen-max-y: maximum value reported by Y axis ADC
> touchscreen-size-x: deprecated alias for touchscreen-max-x
> touchscreen-size-y: deprecated alias for touchscreen-max-y
> 

Initially I had thought about this but it does not solve the problems
with touch pre-calibration. Since it mixes raw coordinates with
system coordinates.

To achieve the goal of having a roughly precalibrated touch which
should provide (0,0) at the lower left corner and (touchscreen-size-x,touchscreen-size-y)
in pixel coordinates of the panel. Hence it roughly works without
a calibration matrix in user space (e.g. xorg.conf or Replicant).

Why do we need pre-calibration? Because some systems might need
touch interaction before they can offer (force) the user into
a touch calibration step. We use these drivers and approach in
our production kernels for GTA04, OpenPandora and Pyra for a while
and nobody was even missing a user-space calibration tool any more.

The underlaying problem is that you can have the same controller chip
in different board designs and there are different touch panel types.
Each one has certain physical properties but they can differ.
But you certainly want touchscreen-size-x/y to be a constant.

Now if we make touchscreen-max-x/y the same as touchscreen-size-x/y
and change the panel, we have to adjust user space transformation
each time we change the panel. This does not seem to be right
and can be done better by keeping them separately.

This is what this approach does: the roughly correct scaling of
raw values to pixel values.

ti,min-x   ->   0
...
x          ->   some value between 0 and touchscreen-size-x
                calculated by
			touchscreen-size-x * (x - min-x) / (max-x - min-x)
...
ti,max-x   ->   touchscreen-size-x

Hence the ti,min/max values describe the range of expected input
values from the ADC and the touchscreen-size-x describes the touch
in LCD pixels passed as input events.

Example:

ti,min-x = 64
ti,max-x = 4016
touchscreen-size-x = 480

If we change the panel type which presents a slightly different ADC range:

ti,min-x = 100
ti,max-x = 3900
touchscreen-size-x = 480

and we still get a coordinate range (0 .. 480).

Note that this feature can be effectively disabled if ti,min-x=0 and
ti,max-x=4095 and touchscreen-size-x=4095, i.e. reports the full
range of ADC values because then it multiplies by 1.

Our proposed driver does use these values if they are missing from DT
and therefore it should not break old DT files which expect raw values
to be reported.

I hope this clarifies what we need to achieve and you can
agree.

BR and thanks,
Nikolaus

Download attachment "signature.asc" of type "application/pgp-signature" (802 bytes)

Powered by blists - more mailing lists