[PATCH 2/2] touchpad: Add edge-scrolling support

[Hans de Goede]

Hi,

On 11/20/2014 07:19 AM, Peter Hutterer wrote:
> On Wed, Nov 19, 2014 at 11:50:13AM +0100, Hans de Goede wrote:
> [...]
>>>
>>>> +
>>>> +	switch (tp->model) {
>>>> +	case MODEL_SYNAPTICS:
>>>> +		edge_width = width * .07;
>>>> +		edge_height = height * .07;
>>>> +		break;
>>>> +	case MODEL_ALPS:
>>>> +		edge_width = width * .15;
>>>> +		edge_height = height * .15;
>>>> +		break;
>>>> +	case MODEL_APPLETOUCH:
>>>> +	case MODEL_UNIBODY_MACBOOK:
>>>
>>> the unibody macbooks already had a clickpad, so this isn't needed
>>>
>>>> +		edge_width = width * .085;
>>>> +		edge_height = height * .085;
>>>> +		break;
>>>> +	default:
>>>> +		edge_width = width * .04;
>>>> +		edge_height = height * .054;
>>>> +	}
>>>
>>> fwiw, there is a bit of history there and many oddities are based on a
>>> the edges in synaptics being 'wrong'. the kernel exports the synaptics
>>> dimensions as the coordinates produced by 'an average finger', but it's not
>>> hard to go beyond min/max. iirc for alps and other devices the edges are the
>>> actual edges (and for the T440-style synaptics pads that we manually fixed
>>> up). Hence the need for different edge zones on synaptics.
>>>
>>> And the default numbers come from the synaptics user interface guide.
>>>      Typical bezel limits: x 1472–5472  y 1408–4448
>>>      Typical edge margins: x 1632–5312  y 1568–4288
>>> i.e. 4% and 5.4% are interpolated from those
>>>
>>> so in short, we don't need MODEL_SYNAPTICS, it's covered by the defaults.
>>>
>>> that just leaves APPLETOUCH and ALPS, I wonder if these have resolutions
>>> set (the synaptics code pre-dates resolution support in the kernel). if so,
>>> just defining a sensible size for the edge is enough (10mm?). or based on
>>> the diagonal.
>>
>> As also mentioned in a private mail not all alps devices have resolution set,
>> so at least for some devices we will need to take a percentage of width / height
>> (no idea why you mention diagonal here).
>
> pls see comments in the other email
>
>>>>   static int
>>>> @@ -1096,6 +1133,9 @@ tp_init(struct tp_dispatch *tp,
>>>>   	if (tp_init_scroll(tp) != 0)
>>>>   		return -1;
>>>>
>>>> +	if (tp_edge_scroll_init(tp, device) != 0)
>>>> +		return -1;
>>>
>>> tp_scroll_init() -> tp_edge_scroll_init()
>>>
>>>> +
>>>>   	device->seat_caps |= EVDEV_DEVICE_POINTER;
>>>>
>>>>   	return 0;
>>>> @@ -1239,6 +1279,7 @@ evdev_mt_touchpad_create(struct evdev_device *device)
>>>>
>>>>   	tp->model = tp_get_model(device);
>>>>
>>>> +	device->dispatch = &tp->base;
>>>
>>> I guess this is needed for some check or another? if so, can we either pass
>>> in the data that's needed, or alternatively make it consistent that the
>>> create method sets device->dispatch for the fallback interface as well.
>>
>> This is needed because the initial value for tp->scroll.mode is set
>> by calling tp_scroll_config_scroll_mode_get_default_mode(device).
>
> yeah, I think we need to wrap this differently. The current approach works,
> but it's a bit of a cardhouse. specifically, in a failure case the order of
> events is:
>    evdev_mt_touchpad_create() sets device->dispatch but returns NULL
>    evdev_configure_device() takes the NULL and overwrites device->dispatch
>    evdev_configure_device() calls evdev_device_destroy() which frees
>        device->dispatch
>
> so again, it works right now but if we ever rearrange
> evdev_configure_device() we'll be dealing with a freed pointer here. and the
> failure case isn't easy to test for.
>
> simple way to split this is to have an inline that takes the tp_dispatch and
> is called from get_default_mode() and tp_init_touch().

Ok, fixed as suggested for V3.

Regards,

Hans