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

[Peter Hutterer]

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

Cheers,
   Peter