linux-kernel - Re: [PATCH 2/3] input: touchscreen: ad7879: fix default x/y axis assignment

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <53cf5cb2a96a236925bb2901e3c417f8@agner.ch>
Date:	Tue, 26 Jan 2016 09:04:20 -0800
From:	Stefan Agner <stefan@...er.ch>
To:	michael.hennerich@...log.com
Cc:	dmitry.torokhov@...il.com, robh+dt@...nel.org,
	mark.rutland@....com, ijc+devicetree@...lion.org.uk,
	galak@...eaurora.org, realmz6@...il.com, broonie@...nel.org,
	jic23@...nel.org, linux-input@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] input: touchscreen: ad7879: fix default x/y axis
 assignment

On 2016-01-25 23:58, Michael Hennerich wrote:
> On 01/26/2016 04:04 AM, Stefan Agner wrote:
>> The measurements read from the controller which are temporary stored
>> in conversion_data, are interpreted wrong. The first measurement X+
>> contains the Y position, and the second measurement Y+ the X position
>> (see also Table 11 Register Table in the data sheet).
>>
>> The problem is already known and a swap option has been introduced:
>> commit 6680884a4420 ("Input: ad7879 - add option to correct xy axis")
>>
>> However, with that the meaning of the new boolean is inverted since
>> the underlying values are already swapped. With this change, a true
>> in swap_xy actually swaps the two axis.
>>
>> Signed-off-by: Stefan Agner <stefan@...er.ch>
>> ---
>> Hi Michael,
>>
>> It seems that swap_xy is not used in any board which is in mainline,
>> hence swap_xy is always false. Therefore, up until now all boards
>> actually used swapped axis. However, I doubt that the blackfin boards
>> really have those axis swapped, it is probably more likely that the
>> userspace calibration took care of it.
>>
>> However, if they are really swapped, we should set the swap_xy flag
>> to 1 for those board...
>>
>> Do you happen to now what is the case with those boards?
>>
> 
> 
> Hi Stefan,
> 
> I would be hesitant to invert the default behaviour of the driver.
> Too many people in the field already using it as it is.

Afaik, we should be able to change in-kernel API's (especially if they
are wrong) since we do not guarantee any API...

> 
> A XY swap can have multiple reasons.
> 
> Lot's of small VGA/QVGA TFTs have the option to switch the scan
> direction from Landscape to Portrait. In addition you can also rotate
> and flip or mirror using VDMA options. So it really depends on the use
> case, how the touch panel is mounted to the screen or how it is wired.

Ok, I see the reason for that functionality.

I am mainly concerned about the new DT bindings. The touchscreen binding
documents specify touchscreen-swapped-x-y, see:
https://www.kernel.org/doc/Documentation/devicetree/bindings/input/touchscreen/touchscreen.txt

I would like to make sure that this property is really swapping axis
(and not necessary if the hardware is implemented according to the
datasheet...)

We could also implement a workaround to keep the platform data behavior
as is (invert the swap_xy flag)...

--
Stefan


>> --
>> Stefan
>>
>>   drivers/input/touchscreen/ad7879.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/ad7879.c b/drivers/input/touchscreen/ad7879.c
>> index a73934b..e290e7b 100644
>> --- a/drivers/input/touchscreen/ad7879.c
>> +++ b/drivers/input/touchscreen/ad7879.c
>> @@ -94,8 +94,8 @@
>>   #define AD7879_TEMP_BIT			(1<<1)
>>
>>   enum {
>> -	AD7879_SEQ_XPOS  = 0,
>> -	AD7879_SEQ_YPOS  = 1,
>> +	AD7879_SEQ_YPOS  = 0,
>> +	AD7879_SEQ_XPOS  = 1,
>>   	AD7879_SEQ_Z1    = 2,
>>   	AD7879_SEQ_Z2    = 3,
>>   	AD7879_NR_SENSE  = 4,
>>