Re: [PATCH v3 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: 4909 bytes --]

Hi,

On Sat, Sep 24, 2016 at 07:55:27AM +0200, H. Nikolaus Schaller wrote:
> > 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.

touchscreen-size-x was actually refering to your definition of
touchscreen-max-x and not system coordinates. For that it would
make much more sense to use a phandle to the screen IMHO.

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

well I did not mean to use touchscreen-size-x/y for describing the
size of screen, as visible in n900.dts (first implementation of the
common binding), which sets the value to 4096.

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

I have nothing against the feature. OTOH I'm quite in of kernel
based TS calibration. Note, that you can only add it for hardware
without pre-existing touchscreen support, since you break peoples
systems otherwise (We have that problem for N900).

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

so basically you use touchscreen-size-x to describe the screen and
not the touchscreen. When I added it, I did mean the max ADC value.
Actually I was under the impression, that X drivers would scale this
to screen size automatically. Since all my touchscreen HW required
calibration I did never test this, though.

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

I did understand what you want, but I disagreed about
using touchscreen-size-x/y for system coordinates. I
now see, that it's too late for that, as other people
already did so.

I do agree with Rob, that the ti,min/max-x/y should become common,
though. Also I would do s/minimum value/minimum raw value/g.
Additionally touchscreen-size-x/y should mention, that it's used to
scale the raw values.

-- Sebastian

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