On Mon, Jan 05, 2015 at 05:04:55PM -0500, Benjamin Tissoires wrote:

On Mon, Jan 5, 2015 at 5:00 PM, Gabriele Mazzotta
[off-list ref] wrote:

quoted

On Monday 05 January 2015 14:24:30 Benjamin Tissoires wrote:

quoted

Hi Gabriele,

[Adding Peter and Hans as this change will impact both
xf86-input-synaptics and libinput]

On Sat, Dec 27, 2014 at 6:31 AM, Gabriele Mazzotta
[off-list ref] wrote:

quoted

Despite claiming to be able to report ABS_TOOL_WIDTH, image sensors
were not doing it. Make them report widths and use ABS_MT_TOUCH_MAJOR
instead ABS_TOOL_WIDTH.

It looks like the current xorg-synaptics code already handles
ABS_MT_TOUCH_MAJOR as finger_width. So I think we should be good in
replacing the ABS_TOOL_WIDTH event. However, I'd prefer having Peter
confirm this because xorg-synaptics still relies a lot on the single
touch emulation.

quoted

Since the 'w' slot is used to report the finger count when two or more
fingers are on the touchpad, make sure that only meaningful values are
emitted, i.e. values greater than or equal to 4, and assign the correct
range to ABS_MT_TOUCH_MAJOR.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=77161
Signed-off-by: Gabriele Mazzotta <redacted>
---
 drivers/input/mouse/synaptics.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index f947292..ea0563e 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c

@@ -814,6 +814,8 @@ static void synaptics_report_slot(struct input_dev *dev, int slot,

Just FYI, this does not apply anymore on top of Dmitry's tree.
synaptics_report_slot() has been removed, and you should now report
the slot state in synaptics_report_mt_data().

quoted

        input_report_abs(dev, ABS_MT_POSITION_X, hw->x);
        input_report_abs(dev, ABS_MT_POSITION_Y, synaptics_invert_y(hw->y));
        input_report_abs(dev, ABS_MT_PRESSURE, hw->z);
+       if (hw->w >= 4)

That I don't like. IMO, at this point, .w should only contain the
finger width, unconditionally.
Also, with 2/2, .w is computed accurately for the second finger, but
not for the first.

I tried to figure out a way to properly extract the actual width
information from the sgm packet when the w is 0 or 1, and the only way
I found was to do the fix in synaptics_image_sensor_process(). I would
have preferred dealing with that in synaptics_parse_hw_state()
directly, but I think the final code would be more and more ugly.
Dealing with the true finger width in synaptics_image_sensor_process()
is not a problem for cr48 sensors, because they will not have the
ABS_MT_TOUCH_MAJOR event exported.

Regarding the last part on cr48 sensors.
Currently these sensors are not reporting widths through ABS_TOOL_WIDTH
and I don't see what could go wrong if they start reporting
ABS_MT_TOUCH_MAJOR. If I understood correctly, they can report widths
only when one finger is on the touchpad. This means that they will
report widths through slot 0, but they won't through slot 1. Nothing
bad should happen.

I am not entirely sure. The entire purpose of having widht for palm
detection is to filter palm from true finger events. So if we only
have the width info on the first slot, it would be useless IMO.
Still I agree with "nothing bad should happen" :)

fwiw, I found finger width _very_ unreliable for palm detection. from what I
recorded, both finger width and palm width ranges overlap to a large amount,
making width useless for anything that's not the whole hand on the
touchpad.

Cheers,
   Peter

quoted

I've just rebased my patches and included the support for cr48 sensors.
The only change I made was to change the default width value from
5 to 4 since this is the minimum values these sensors can report
(according to the existing code).

Anyway, should I simply include the changes you suggested to handle
widths of sgm packets in what will be the new 2/2?

Go ahead and include my changes in your 2/2. It's better to have a
consistent commit.

Cheers,
Benjamin