[PATCH synaptics] conf: ship a quirk for Cypress touchpads

[Hans de Goede]

Hi,

On 03/24/2014 12:45 AM, Peter Hutterer wrote:
> On Fri, Mar 21, 2014 at 11:03:36AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 03/21/2014 10:31 AM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 03/20/2014 11:18 PM, Peter Hutterer wrote:
>>>> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
>>>> ---
>>>>  conf/11-x11-synaptics.fdi |  9 +++++++++
>>>>  conf/50-synaptics.conf    | 12 ++++++++++++
>>>>  2 files changed, 21 insertions(+)
>>>>
>>>> diff --git a/conf/11-x11-synaptics.fdi b/conf/11-x11-synaptics.fdi
>>>> index a898875..44dfce3 100644
>>>> --- a/conf/11-x11-synaptics.fdi
>>>> +++ b/conf/11-x11-synaptics.fdi
>>>> @@ -34,6 +34,15 @@
>>>>          <match key="info.product" contains="Apple|bcm5974">
>>>>              <merge key="input.x11_options.SoftButtonAreas" type="string">0 0 0 0 0 0 0 0</merge>
>>>>          </match>
>>>> +
>>>> +        <!--The Cypress touchpads provide BTN_RIGHT in firmware, together with
>>>> +            clickfinger, and two-finger scrolling. Disable Clickpads, otherwise we
>>>> +            get flaky button behaviour.
>>>> +            https://bugs.freedesktop.org/show_bug.cgi?id=70819
>>>> +            https://bugs.freedesktop.org/show_bug.cgi?id=76341 -->
>>>
>>> I'm not so sure that using a xorg.conf snippet for this is the best way to go about this, I would
>>> prefer to see a fix to the kernel to not report the clickpad flag for non clickpads. Esp. as some
>>> newer or future Cypress touchpads may very well get rid of the firmware mode and do everything in
>>> the drivers as other vendors do.
>>>
>>> I really think that "CyPS/2 Cypress Trackpad" is a way to generic match string and is
>>> going to bite us in the future. At a minimum we should use the still to be merged DMI matching
>>> here to limit this to certain laptop models, but IHMO it would be much better to simply fix
>>> this in the kernel. For starters we could add a dmi quirk in the kernel for the XPS 13, until we
>>> figure out a better way to decide which Cypress Trackpads use this firmware emulation and which
>>> models don't.
>>>
>>> And maybe the one in the Dell XPS 13 even has a command we can send to it to turn of the
>>> emulation too, which would allow us to really fix 70819 too.
>>
>> So I've spend some time reading the kernel cypress_ps2 driver, conclusions:
>>
>> 1) It can only do multi-touch for upto 2 fingers, it can detect tripple / quad
>> fingers being down but will only report coordinates for 2.
>>
>> 2) It seems to do a lot of processing in firmware, so in general at least for
>> the currently supported generation not reporting INPUT_PROP_BUTTONPAD seems to be
>> the right thing to do for all currently supported Cypress Trackpads.
>>
>> I still believe the kernel is the right place to fix this. Adam, can you build a kernel
>> with the attached patch, remove your xorg.conf changes and then test that setup please ?
>>
>> Regards,
>>
>> Hans
> 
>> From 4b0668abbea0068cbdf7f9b0d149ccc245d79fab Mon Sep 17 00:00:00 2001
>> From: Hans de Goede <hdegoede at redhat.com>
>> Date: Fri, 21 Mar 2014 10:55:11 +0100
>> Subject: [PATCH] input: cypress_ps2: Don't report the cypress PS/2 trackpads
>>  as a button pad
>>
>> The cypress PS/2 trackpad models supported by the cypress_ps2 driver do
>> right button emulation in firmware, filtering out a bunch of events userspace
>> needs to do clickpad handling on its own. So reporting them as clickpads
>> actually causes a bunch of issues:
>>
>> https://bugs.freedesktop.org/show_bug.cgi?id=70819
>> https://bugs.freedesktop.org/show_bug.cgi?id=76341
>>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> ---
>>  drivers/input/mouse/cypress_ps2.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/input/mouse/cypress_ps2.c b/drivers/input/mouse/cypress_ps2.c
>> index 87095e2..8af34ff 100644
>> --- a/drivers/input/mouse/cypress_ps2.c
>> +++ b/drivers/input/mouse/cypress_ps2.c
>> @@ -409,7 +409,6 @@ static int cypress_set_input_params(struct input_dev *input,
>>  	__clear_bit(REL_X, input->relbit);
>>  	__clear_bit(REL_Y, input->relbit);
>>  
>> -	__set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
>>  	__set_bit(EV_KEY, input->evbit);
>>  	__set_bit(BTN_LEFT, input->keybit);
>>  	__set_bit(BTN_RIGHT, input->keybit);
>> -- 
>> 1.9.0
> 
> I recommend that you expand the commit message with a blurb on why it causes
> issues before you send it to the lkml, it'll make it easier to review.
> Specifically that BTN_RIGHT is emulated in firmware based on the finger
> position, and that no motion events are sent when the finger is in the
> button area.
> 
> other than that this seems to be the best solution, thanks.
> Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net>

Thanks for the review, I've send this upstream with an amended commit msg.

> though I'll still ship the xorg.conf snippet in fedora until this filters
> down.

Ok.

Regards,

Hans