[PATCH v3] Support LED and tap-on-LED feature
Peter Hutterer
peter.hutterer at who-t.net
Mon May 3 21:25:19 PDT 2010
On Fri, Apr 23, 2010 at 05:40:41PM +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 a sysfs file /sys/class/leds/psmouse::synaptics exists, 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>
> ---
>
> v2->v3: Fix a fd leak in write error path in eventcomm.c
thanks for the update. as I said in my last email, I think these patches
should be split out into multiple features. I had a look at the synaptics
code and its current features, so here's my summary of the whole lot. I'm
aware that you might not like this since it's a lot more work than
the current patch.
We have two distinct capabilities that are implemented here. First, we have
a LED on the touchpad. Second, clicking onto this LED disables the touchpad
on Windows (note that I do _not_ believe that this should be the only
action, just because Windows does exactly this).
So just tacking on this like it is means that any other use of the LED
requires adding other properties and then you run into conflicts when both
are enabled, etc. So a nice fix for now, but painful in the not-too-distant
future. (Remember that properties are not strictly API but close enough. We
can't change them too willy-nilly since clients depend on them)
Here's what I think the full patch set should do:
LED handling:
- has_led should be exposed in the synaptics capabilities so that clients
can query it.
- the LED brightness should be its own property. This way clients can use it
for other things as well, e.g. imagine the LED going on when you have new
mail (though for most of us that'd probably just mean the LED is
permanently on).
if has_led is false, the property doesn't need to be initialized.
(It might be worth making this a 0 - 1024 or so range that scales into
whatever the kernel might provide in the future. Since you wrote the
kernel patches, maybe you can comment on whether 255 will be enough for
the X driver?)
Tap handling:
- We have an existing infrastructure to detect corner presses and rather
than in_led_toggle_area we _should_ be able to use the left-top corner
area for triggering the action. SelectTapButton() does this detection for
us, so we only need to pick a tap action to happen.
(The existing corner buttons can be extended to also support a double-tap
feature. So instead of Option "RTCornerButton" "3", it can allow
"RTCornerButton" "3 2", for single and double-tap)
- We already have MaxDoubleTapTime in the driver, this should be reused here.
- The tapping in the corner is close enough to a gesture, so you can add
that toggle into the Synaptics Gestures property.
Now, those were the easy bits. They give us control over the LED and
integrate the tap handling into the existing code.
The main problem we have now though:
- If the LED is hooked directly to the touchpad on/off state, this means it
will flash all the time if syndaemon is running. This has the potential to
be quite annoying.
That's all I came up with so far. I don't have any comments about the code
at the moment, because the patch itself is fine. It's just the integration
I'm worried about.
Cheers,
Peter
> include/synaptics-properties.h | 3 +
> man/synaptics.man | 13 ++++++
> src/eventcomm.c | 32 ++++++++++++++-
> src/properties.c | 26 ++++++++++++
> src/synaptics.c | 85 +++++++++++++++++++++++++++++++++++++++-
> src/synapticsstr.h | 6 +++
> src/synproto.h | 1 +
> tools/synclient.c | 1 +
> 8 files changed, 164 insertions(+), 3 deletions(-)
>
> diff --git a/include/synaptics-properties.h b/include/synaptics-properties.h
> index cf330d8..d24ed9e 100644
> --- a/include/synaptics-properties.h
> +++ b/include/synaptics-properties.h
> @@ -155,4 +155,7 @@
> /* 32 bit, 4 values, left, right, top, bottom */
> #define SYNAPTICS_PROP_AREA "Synaptics Area"
>
> +/* 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 59fbaac..0c79721 100644
> --- a/man/synaptics.man
> +++ b/man/synaptics.man
> @@ -509,6 +509,19 @@ Ignore movements, scrolling and tapping which take place below this edge.
> The option is disabled by default and can be enabled by setting the
> AreaBottomEdge option to any integer value other than zero. Property: "Synaptics Area"
> .
> +.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"
> +.
> .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 6b44778..3948d9a 100644
> --- a/src/eventcomm.c
> +++ b/src/eventcomm.c
> @@ -51,6 +51,8 @@
> #define LONG(x) ((x) / LONG_BITS)
> #define TEST_BIT(bit, array) (array[LONG(bit)] & (1 << OFF(bit)))
>
> +#define SYNAPTICS_LED_SYS_FILE "/sys/class/leds/psmouse::synaptics/brightness"
> +
> /*****************************************************************************
> * Function Definitions
> ****************************************************************************/
> @@ -166,6 +168,32 @@ event_query_info(LocalDevicePtr local)
> }
> }
>
> +static void
> +event_query_led(LocalDevicePtr local)
> +{
> + SynapticsPrivate *priv = (SynapticsPrivate *)local->private;
> +
> + priv->has_led = !access(SYNAPTICS_LED_SYS_FILE, W_OK);
> +}
> +
> +static void EventUpdateHardware(LocalDevicePtr local)
> +{
> + SynapticsPrivate *priv = (SynapticsPrivate *)local->private;
> +
> + if (priv->has_led) {
> + char *val = priv->synpara.touchpad_off ? "255" : "0";
> + int fd = open(SYNAPTICS_LED_SYS_FILE, O_WRONLY);
> + int err;
> +
> + if (fd < 0)
> + return;
> + err = write(fd, val, strlen(val));
> + close(fd);
> + if (err < 0)
> + xf86Msg(X_WARNING, "%s can't write LED value %s\n", val);
> + }
> +}
> +
> /* Query device for axis ranges */
> static void
> event_query_axis_ranges(LocalDevicePtr local)
> @@ -434,6 +462,7 @@ EventReadDevDimensions(LocalDevicePtr local)
> if (event_query_is_touchpad(local->fd, (need_grab) ? *need_grab : TRUE))
> event_query_axis_ranges(local);
> event_query_info(local);
> + event_query_led(local);
> }
>
> static Bool
> @@ -493,5 +522,6 @@ struct SynapticsProtocolOperations event_proto_operations = {
> EventQueryHardware,
> EventReadHwState,
> EventAutoDevProbe,
> - EventReadDevDimensions
> + EventReadDevDimensions,
> + EventUpdateHardware,
> };
> diff --git a/src/properties.c b/src/properties.c
> index 4366034..d7bc3bd 100644
> --- a/src/properties.c
> +++ b/src/properties.c
> @@ -84,6 +84,7 @@ Atom prop_gestures = 0;
> Atom prop_capabilities = 0;
> Atom prop_resolution = 0;
> Atom prop_area = 0;
> +Atom prop_led_double_tap = 0;
>
> static Atom
> InitAtom(DeviceIntPtr dev, char *name, int format, int nvalues, int *values)
> @@ -274,6 +275,9 @@ InitDeviceProperties(LocalDevicePtr local)
> values[2] = para->area_top_edge;
> values[3] = para->area_bottom_edge;
> prop_area = InitAtom(local->dev, SYNAPTICS_PROP_AREA, 32, 4, values);
> +
> + prop_led_double_tap = InitAtom(local->dev, SYNAPTICS_PROP_LED_DOUBLE_TAP, 32, 1, ¶->led_double_tap);
> +
> }
>
> int
> @@ -490,6 +494,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);
> + }
> +
> } else if (property == prop_guestmouse)
> {
> if (prop->size != 1 || prop->format != 8 || prop->type != XA_INTEGER)
> @@ -642,10 +651,27 @@ SetProperty(DeviceIntPtr dev, Atom property, XIPropertyValuePtr prop,
> para->area_right_edge = area[1];
> para->area_top_edge = area[2];
> para->area_bottom_edge = area[3];
> + } 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;
> +
> + 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 554f7a3..a910db6 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 = {
> @@ -544,6 +545,7 @@ static void set_default_parameters(LocalDevicePtr local)
> pars->tap_and_drag_gesture = xf86SetBoolOption(opts, "TapAndDragGesture", TRUE);
> pars->resolution_horiz = xf86SetIntOption(opts, "HorizResolution", horizResolution);
> pars->resolution_vert = xf86SetIntOption(opts, "VertResolution", vertResolution);
> + pars->led_double_tap = xf86SetIntOption(opts, "LEDDoubleTap", 400);
>
> /* Warn about (and fix) incorrectly configured TopEdge/BottomEdge parameters */
> if (pars->top_edge > pars->bottom_edge) {
> @@ -776,6 +778,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;
> @@ -2064,6 +2071,72 @@ HandleClickWithFingers(SynapticsParameters *para, struct SynapticsHwState *hw)
> }
> }
>
> +#define LED_TOGGLE_X_AREA 0.10
> +#define LED_TOGGLE_Y_AREA 0.08
> +
> +static int
> +in_led_toggle_area(LocalDevicePtr local, struct SynapticsHwState *hw)
> +{
> + SynapticsPrivate *priv = (SynapticsPrivate *) (local->private);
> + int click_led_x, click_led_y;
> +
> + click_led_x = (priv->maxx - priv->minx) * LED_TOGGLE_X_AREA + priv->minx;
> + click_led_y = (priv->maxy - priv->miny) * LED_TOGGLE_Y_AREA + priv->miny;
> + return (hw->x < click_led_x && hw->y < click_led_y);
> +}
> +
> +/* 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 diff;
> +
> + if (finger) {
> + if (!in_led_toggle_area(local, hw)) {
> + /* outside the toggle area */
> + priv->led_touch_state = FALSE;
> + priv->led_tapped = FALSE;
> + return finger;
> + }
> + if (!priv->led_touch_state) {
> + /* touch start */
> + priv->led_touch_millis = hw->millis;
> + priv->led_touch_state = TRUE;
> + }
> + return 0; /* already processed; ignore this finger event */
> + }
> +
> + if (!priv->led_touch_state)
> + return finger; /* nothing happened */
> +
> + /* touch-released */
> + priv->led_touch_state = FALSE;
> + diff = TIME_DIFF(priv->led_touch_millis + para->tap_time, hw->millis);
> + if (diff < 0) /* non-tap? */
> + return finger;
> + if (priv->led_tapped) {
> + /* double-tapped? */
> + diff = TIME_DIFF(priv->led_tap_millis + para->led_double_tap, hw->millis);
> + if (diff >= 0) {
> + para->touchpad_off = !para->touchpad_off;
> + if (priv->proto_ops && priv->proto_ops->UpdateHardware)
> + priv->proto_ops->UpdateHardware(local); /* update LED */
> +#if GET_ABI_MAJOR(ABI_XINPUT_VERSION) >= 3
> + SynapticsToggleOffProperty(local->dev, para->touchpad_off);
> +#endif
> + priv->led_tapped = FALSE;
> + }
> + } else
> + priv->led_tapped = TRUE;
> + priv->led_tap_millis = hw->millis;
> + return 0; /* already processed; ignore this finger event */
> +}
> +
> /* clickpad event handling */
> static void
> HandleClickpad(LocalDevicePtr local, struct SynapticsHwState *hw, edge_type edge)
> @@ -2164,13 +2237,13 @@ HandleState(LocalDevicePtr local, struct SynapticsHwState *hw)
> }
>
> /* If touchpad is switched off, we skip the whole thing and return delay */
> - if (para->touchpad_off == 1)
> + if (para->touchpad_off == 1 && !(priv->has_led && para->led_double_tap))
> return delay;
>
> edge = edge_detection(priv, hw->x, hw->y);
>
> /* Clickpad handling for button area */
> - if (priv->is_clickpad)
> + if (para->touchpad_off != 1 && priv->is_clickpad)
> HandleClickpad(local, hw, edge);
>
> /* Treat the first two multi buttons as up/down for now. */
> @@ -2227,6 +2300,14 @@ HandleState(LocalDevicePtr local, struct SynapticsHwState *hw)
>
> finger = SynapticsDetectFinger(priv, hw);
>
> + if (priv->has_led && para->led_double_tap) {
> + finger = HandleToggleLED(local, hw, finger);
> + if (para->touchpad_off == 1) {
> + priv->finger_state = finger;
> + return delay;
> + }
> + }
> +
> /* tap and drag detection */
> timeleft = HandleTapProcessing(priv, hw, edge, finger, inside_active_area);
> if (timeleft > 0)
> diff --git a/src/synapticsstr.h b/src/synapticsstr.h
> index 05e43d3..54c0edf 100644
> --- a/src/synapticsstr.h
> +++ b/src/synapticsstr.h
> @@ -160,6 +160,7 @@ typedef struct _SynapticsParameters
> unsigned int resolution_horiz; /* horizontal resolution of touchpad in units/mm */
> unsigned int resolution_vert; /* vertical resolution of touchpad in units/mm */
> int area_left_edge, area_right_edge, area_top_edge, area_bottom_edge; /* area coordinates absolute */
> + int led_double_tap; /* double-tap period in ms for touchpad LED control */
> } SynapticsParameters;
>
>
> @@ -234,6 +235,11 @@ typedef struct _SynapticsPrivateRec
> Bool has_pressure; /* device reports pressure */
> Bool is_clickpad; /* is Clickpad device (one-button) */
> struct SynapticsHwState prev_hw; /* previous h/w state (for clickpad) */
> + Bool has_led; /* has LED indicator */
> + Bool led_touch_state;
> + Bool led_tapped;
> + int led_touch_millis;
> + int led_tap_millis;
>
> enum TouchpadModel model; /* The detected model */
> } SynapticsPrivate;
> diff --git a/src/synproto.h b/src/synproto.h
> index f7be819..1611a12 100644
> --- a/src/synproto.h
> +++ b/src/synproto.h
> @@ -97,6 +97,7 @@ struct SynapticsProtocolOperations {
> struct CommData *comm, struct SynapticsHwState *hwRet);
> Bool (*AutoDevProbe)(LocalDevicePtr local);
> void (*ReadDevDimensions)(LocalDevicePtr local);
> + void (*UpdateHardware)(LocalDevicePtr local);
> };
>
> extern struct SynapticsProtocolOperations psaux_proto_operations;
> diff --git a/tools/synclient.c b/tools/synclient.c
> index 316ae2c..700bcea 100644
> --- a/tools/synclient.c
> +++ b/tools/synclient.c
> @@ -143,6 +143,7 @@ static struct Parameter params[] = {
> {"AreaRightEdge", PT_INT, 0, 10000, SYNAPTICS_PROP_AREA, 32, 1},
> {"AreaTopEdge", PT_INT, 0, 10000, SYNAPTICS_PROP_AREA, 32, 2},
> {"AreaBottomEdge", PT_INT, 0, 10000, SYNAPTICS_PROP_AREA, 32, 3},
> + {"LEDDoubleTap", PT_INT, 0, 1000, SYNAPTICS_PROP_LED_DOUBLE_TAP, 32, 0},
> { NULL, 0, 0, 0, 0 }
> };
>
> --
> 1.7.0.4
More information about the xorg-devel
mailing list