[PATCH 2/2] Support LED and tap-on-LED feature

Takashi Iwai tiwai at suse.de
Tue Apr 20 23:26:41 PDT 2010


At Wed, 21 Apr 2010 14:16:29 +1000,
Peter Hutterer wrote:
> 
> On Thu, Apr 15, 2010 at 06:12:11PM +0200, Takashi Iwai wrote:
> > This patch adds the control of the embedded LED on the top-left corner
> > of new Synaptics devices.  For LED control, it requires the patch to
> > Linux synaptics input driver,
> > 	https://patchwork.kernel.org/patch/92434/
> > 
> > When evdev reports the presense of LED and LED_MUTE bits, the driver
> > assumes it supports the embeded LED control.  This corresponds to the
> > touchpad-off state.  When touchpad is disabled, LED is turned on.
> > 
> > For linking between the touchpad state and the LED state, a new callback
> > UpdateHardware is introduced.  Only eventcomm.c supports this (naturally).
> > 
> > A new feature for the LED-equipped device is that user can double-tap
> > on the LED to toggle the touchpad state on the fly.  This is also linked
> > with the touchpad-off state.
> > 
> > There is a new parameter for controlling the LED double-tap behavior, too.
> > It specifies the double-tap time.  Passing zero disables the double-tap
> > feature.
> > 
> > Signed-off-by: Takashi Iwai <tiwai at suse.de>
> > ---
> >  include/synaptics-properties.h |    3 ++
> >  man/synaptics.man              |   13 +++++++
> >  src/eventcomm.c                |   39 +++++++++++++++++++++-
> >  src/properties.c               |   26 ++++++++++++++
> >  src/synaptics.c                |   73 ++++++++++++++++++++++++++++++++++++++-
> >  src/synapticsstr.h             |    6 +++
> >  src/synproto.h                 |    1 +
> >  tools/synclient.c              |    1 +
> >  8 files changed, 159 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/synaptics-properties.h b/include/synaptics-properties.h
> > index c77afd3..56c3e1d 100644
> > --- a/include/synaptics-properties.h
> > +++ b/include/synaptics-properties.h
> > @@ -161,4 +161,7 @@
> >  /* 8 bit (BOOL) */
> >  #define SYNAPTICS_PROP_TOUCH_BUTTON_SENSE "Synaptics Touch Button Sense"
> >  
> > +/* 32 bit */
> > +#define SYNAPTICS_PROP_LED_DOUBLE_TAP "Synaptics LED Double Tap"
> > +
> >  #endif /* _SYNAPTICS_PROPERTIES_H_ */
> > diff --git a/man/synaptics.man b/man/synaptics.man
> > index f92ea0c..baa7e8f 100644
> > --- a/man/synaptics.man
> > +++ b/man/synaptics.man
> > @@ -530,6 +530,19 @@ clicking the button.  On the other hand, this reduces the available
> >  space on the touchpad.  The default is on.
> >  Property: "Synaptics Touch Button Sense"
> >  .
> > +.TP
> > +.BI "Option \*qLEDDoubleTap\*q \*q" integer \*q
> > +.
> > +The double-tap time for toggling the touchpad-control on the top-left
> > +corner LED.
> > +.
> > +Some devices have an LED on the top-left corner to indicate the
> > +touchpad state.  User can double-tap on the LED to toggle the touchpad
> > +state.  This option controls the double-tap time in milli-seconds.
> > +When it's zero, the LED-tapping feature is disabled.  The default
> > +value is 400.
> > +Property: "Synaptics LED Double Tap"
> 
> a few questions to the principle:
> - any specific reason this is a double-tap?

A single-tap is often too sensitive.  User may toggle it mistakenly,
and wonders what happens suddenly.  Also, Windows driver uses the
double-tap, too.

> - the corner might be used for the same feature even if there is no LED
>   present, right?

The check is done only when has_led flag is set, so it's exclusive for
the device with LED control.

> as I read this patch, the LED updating could be split out
>   from the actual gesture of disabling the touchpad by clicking into the
>   corner.

In general yes, the double-tapping to toggle touchpad is an individual
feature.  But this becomes intuitive when coupled with LED, thus
rather somewhat special for LED-equipped device, I suppose.


> 
> > +.
> >  .LP
> >  A tap event happens when the finger is touched and released in a time
> >  interval shorter than MaxTapTime, and the touch and release
> > diff --git a/src/eventcomm.c b/src/eventcomm.c
> > index ca99f3a..292e64e 100644
> > --- a/src/eventcomm.c
> > +++ b/src/eventcomm.c
> > @@ -179,6 +179,39 @@ static void event_query_clickpad(LocalDevicePtr local)
> >      }
> >  }
> >  
> > +/* LED support: the kernel driver should give EV_LED and LED_MUTE bits */
> > +static void event_query_led(LocalDevicePtr local)
> > +{
> > +    SynapticsPrivate *priv = (SynapticsPrivate *)local->private;
> > +    unsigned long evbits[NBITS(EV_MAX)] = {0};
> > +    int rc;
> > +
> > +    priv->has_led = FALSE;
> > +
> > +    SYSCALL(rc = ioctl(local->fd, EVIOCGBIT(0, sizeof(evbits)), evbits));
> > +    if (TEST_BIT(EV_LED, evbits)) {
> > +	unsigned long ledbits[NBITS(LED_MAX)] = {0};
> > +	SYSCALL(rc = ioctl(local->fd, EVIOCGBIT(EV_LED, sizeof(ledbits)), ledbits));
> > +	if (TEST_BIT(LED_MUTE, ledbits)) {
> > +	    xf86Msg(X_INFO, "%s: has LED control\n", local->name);
> > +	    priv->has_led = TRUE;
> > +	}
> > +    }
> > +}
> > +
> > +static void EventUpdateHardware(LocalDevicePtr local)
> > +{
> > +    SynapticsPrivate *priv = (SynapticsPrivate *)local->private;
> > +    if (priv->has_led) {
> > +	struct input_event ev;
> > +	gettimeofday(&ev.time, NULL);
> > +	ev.type = EV_LED;
> > +	ev.code = LED_MUTE;
> > +	ev.value = priv->synpara.touchpad_off != 0;
> > +	(void)write(local->fd, &ev, sizeof(ev));
> 
> skip the (void) here please. a return value check would be nice in case of
> error.

Heh, I'm a lazy guy :)


> > +    }
> > +}
> > +
> >  /* Query device for axis ranges */
> >  static void
> >  event_query_axis_ranges(LocalDevicePtr local)
> > @@ -268,6 +301,7 @@ event_query_axis_ranges(LocalDevicePtr local)
> >      }
> >  
> >      event_query_clickpad(local);
> > +    event_query_led(local);
> >  }
> >  
> >  static Bool
> > @@ -421,6 +455,8 @@ EventReadHwState(LocalDevicePtr local,
> >  		break;
> >  	    }
> >  	    break;
> > +	case EV_LED:
> > +	    return TRUE;
> >  	}
> >      }
> >      return FALSE;
> > @@ -502,5 +538,6 @@ struct SynapticsProtocolOperations event_proto_operations = {
> >      EventQueryHardware,
> >      EventReadHwState,
> >      EventAutoDevProbe,
> > -    EventReadDevDimensions
> > +    EventReadDevDimensions,
> > +    EventUpdateHardware,
> >  };
> > diff --git a/src/properties.c b/src/properties.c
> > index a579479..57f11f0 100644
> > --- a/src/properties.c
> > +++ b/src/properties.c
> > @@ -86,6 +86,7 @@ Atom prop_resolution            = 0;
> >  Atom prop_area                  = 0;
> >  Atom prop_touch_button_area     = 0;
> >  Atom prop_touch_button_sense    = 0;
> > +Atom prop_led_double_tap        = 0;
> >  
> >  static Atom
> >  InitAtom(DeviceIntPtr dev, char *name, int format, int nvalues, int *values)
> > @@ -280,6 +281,9 @@ InitDeviceProperties(LocalDevicePtr local)
> >      prop_touch_button_area = InitAtom(local->dev, SYNAPTICS_PROP_TOUCH_BUTTON_AREA, 8, 1, &para->touch_button_area);
> >  
> >      prop_touch_button_sense = InitAtom(local->dev, SYNAPTICS_PROP_TOUCH_BUTTON_SENSE, 8, 1, &para->touch_button_sense);
> > +
> > +    prop_led_double_tap = InitAtom(local->dev, SYNAPTICS_PROP_LED_DOUBLE_TAP, 32, 1, &para->led_double_tap);
> > +
> >  }
> >  
> >  int
> > @@ -496,6 +500,11 @@ SetProperty(DeviceIntPtr dev, Atom property, XIPropertyValuePtr prop,
> >              return BadValue;
> >  
> >          para->touchpad_off = off;
> > +	if (!checkonly) {
> > +	    if (priv->proto_ops && priv->proto_ops->UpdateHardware)
> > +		priv->proto_ops->UpdateHardware(local);
> > +	}
> > +
> 
> indentation.

What is wrong here?


> >      } else if (property == prop_guestmouse)
> >      {
> > @@ -661,10 +670,27 @@ SetProperty(DeviceIntPtr dev, Atom property, XIPropertyValuePtr prop,
> >              return BadMatch;
> >  
> >          para->touch_button_sense = *(BOOL*)prop->data;
> > +    } else if (property == prop_led_double_tap)
> > +    {
> > +        if (prop->size != 1 || prop->format != 32 || prop->type != XA_INTEGER)
> > +            return BadMatch;
> > +
> > +        para->led_double_tap = *(INT32*)prop->data;
> >      }
> >  
> >      return Success;
> >  }
> >  
> > +void SynapticsToggleOffProperty(DeviceIntPtr dev, Bool off)
> > +{
> > +	uint8_t val;
> 
> please use CARD8 here for consistency with the rest of the code.

OK.


> > +
> > +	if (!prop_off)
> > +		return;
> > +	val = off;
> > +	XIChangeDeviceProperty(dev, prop_off, XA_INTEGER, 8,
> > +			       PropModeReplace, 1, &val, FALSE);
> > +}
> > +
> >  #endif
> >  
> > diff --git a/src/synaptics.c b/src/synaptics.c
> > index f701074..86001e2 100644
> > --- a/src/synaptics.c
> > +++ b/src/synaptics.c
> > @@ -134,6 +134,7 @@ static void CalculateScalingCoeffs(SynapticsPrivate *priv);
> >  void InitDeviceProperties(LocalDevicePtr local);
> >  int SetProperty(DeviceIntPtr dev, Atom property, XIPropertyValuePtr prop,
> >                  BOOL checkonly);
> > +void SynapticsToggleOffProperty(DeviceIntPtr dev, Bool off);
> >  #endif
> >  
> >  InputDriverRec SYNAPTICS = {
> > @@ -534,6 +535,7 @@ static void set_default_parameters(LocalDevicePtr local)
> >      pars->resolution_vert = xf86SetIntOption(opts, "VertResolution", vertResolution);
> >      pars->touch_button_area = xf86SetIntOption(opts, "TouchButtonArea", 20);
> >      pars->touch_button_sense = xf86SetBoolOption(opts, "TouchButtonSense", FALSE);
> > +    pars->led_double_tap = xf86SetIntOption(opts, "LEDDoubleTap", 400);
> >  
> >      /* Warn about (and fix) incorrectly configured TopEdge/BottomEdge parameters */
> >      if (pars->top_edge > pars->bottom_edge) {
> > @@ -766,6 +768,11 @@ DeviceOn(DeviceIntPtr dev)
> >      }
> >  
> >      xf86AddEnabledDevice(local);
> > +
> > +    /* update LED */
> > +    if (priv->proto_ops && priv->proto_ops->UpdateHardware)
> > +	priv->proto_ops->UpdateHardware(local);
> > +
> >      dev->public.on = TRUE;
> >  
> >      return Success;
> > @@ -2054,6 +2061,60 @@ HandleClickWithFingers(SynapticsParameters *para, struct SynapticsHwState *hw)
> >      }
> >  }
> >  
> > +/* clicpad button toggle point:
> > + * some devices have a LED at the upper-left corner, and double-tapping it
> > + * toggles the touchpad enable/disable
> > + */
> > +static int
> > +HandleToggleLED(LocalDevicePtr local, struct SynapticsHwState *hw, int finger)
> > +{
> > +    SynapticsPrivate *priv = (SynapticsPrivate *) (local->private);
> > +    SynapticsParameters *para = &priv->synpara;
> > +    int click_led_x = (priv->maxx - priv->minx) * 1 / 10 + priv->minx;
> > +    int click_led_y = priv->maxy / 8 + priv->miny;
> 
> please use defines for the 1/10 and 1/8 instead of the numbers being hidden
> in the code.

OK.

>  I guess this doesn't change that much so we might get by
> without having configuration options for this but it should at least be a
> define.

Yeah, that's why I didn't give options or properties, too.


thanks,

Takashi


More information about the xorg-devel mailing list