[PATCH 04/18] Add tap-on-LED feature support

[Takashi Iwai]

At Wed, 13 Oct 2010 13:29:48 +1000,
Peter Hutterer wrote:
> 
> On Fri, Oct 08, 2010 at 07:22:28PM +0200, Takashi Iwai wrote:
> > Signed-off-by: Takashi Iwai <tiwai at suse.de>
> > ---
> >  include/synaptics-properties.h |    3 +
> >  man/synaptics.man              |   17 +++++++
> >  src/properties.c               |   27 +++++++++++
> >  src/synaptics.c                |   97 +++++++++++++++++++++++++++++++++++++++-
> >  src/synapticsstr.h             |    6 +++
> >  tools/synclient.c              |    1 +
> >  6 files changed, 149 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/synaptics-properties.h b/include/synaptics-properties.h
> > index dd7e259..1343e6d 100644
> > --- a/include/synaptics-properties.h
> > +++ b/include/synaptics-properties.h
> > @@ -161,4 +161,7 @@
> >  /* 8 bit (BOOL), led_status (on/off) */
> >  #define SYNAPTICS_PROP_LED_STATUS "Synaptics LED Status"
> >  
> > +/* 8 bit (BOOL), double-tap action on LED corner (on/off) */
> > +#define SYNAPTICS_PROP_LED_DOUBLE_TAP "Synaptics LED Dobule Tap"
> > +
> 
> SYNAPTICS_PROP_GESTURES is essentially there for things like this, no need
> for a new property, just append it there.

OK.

> > +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);
> > +}
> 
> last argument must be TRUE, we need to notify the clients about this change. 

Good catch.

> > +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:
> 
> 
> typo

Maybe this word should be removed since it's no longer specific to
clickpad. 


> > +    } else
> > +        priv->led_tapped = TRUE;
> > +    priv->led_tap_millis = hw->millis;
> > +    return 0; /* already processed; ignore this finger event */
> > +}
> > +
> 
> two tab indentations in this hunk.

Sorry, this was my default setup of emacs (I'm a kernel programmer, so
tab is preferred).  Will de-tabify later.

> > +    if (para->has_led && para->led_double_tap) {
> > +	if (inside_active_area)
> > +		finger = handle_toggle_led(local, hw, finger);
> > +        if (para->touchpad_off == 1) {
> > +            priv->finger_state = finger;
> > +            return delay;
> > +        }
> > +    }
> > +
> 
> tab/space indentation mixed up. also, are you sure this is what you want
> here? the led only toggled if it's inside the user's active area? Given that
> this is a hardware feature more than a software feature, it should likely be
> triggerable regardless of the area defined, isn't it?

I'm not quite sure why I put it.  It wasn't in my original patch for
older xorg version.  IIRC, it was needed since finger variable is
updated only when inside_active_area is true.


> >      /* tap and drag detection. Needs to be performed even if the finger is in
> >       * the dead area to reset the state. */
> >      timeleft = HandleTapProcessing(priv, hw, edge, finger, inside_active_area);
> > diff --git a/src/synapticsstr.h b/src/synapticsstr.h
> > index 88ca9de..05308c1 100644
> > --- a/src/synapticsstr.h
> > +++ b/src/synapticsstr.h
> > @@ -162,6 +162,7 @@ typedef struct _SynapticsParameters
> >      int area_left_edge, area_right_edge, area_top_edge, area_bottom_edge; /* area coordinates absolute */
> >      Bool has_led;                           /* has an embedded LED */
> >      Bool led_status;                        /* Current status of LED (1=on) */
> > +    Bool led_double_tap;		    /* double-tap period in ms for touchpad LED control */
> 
> 
> spaces only please
> 
> meat of the patch looks good, bar the few comments above, you have my ack.

OK, thanks.


Takashi