[PATCH synaptics] Add synaptics orientation support

[Peter Hutterer]

On Sat, Mar 19, 2011 at 09:42:38PM -0400, zt.tmzt at gmail.com wrote:
> Why does this need to be a SYNAPTICS property, synclient does not respect
> XInput properties? Are they changing the same thing on the backend and using
> the same semantics? (On the last, no, I believe the min/max have to be
> changed and axes swapped on evdev devices)

it doesn't have to be a synaptics property but I'm beginning to think the
approach of a server-defined property with a driver-specific implementation
may be best.

i.e. the server defines the property name and value range, the
implementation is done in the driver.

Cheers,
  Peter

> --
> Timothy Meade
> tmzt on freenode
> On Mar 18, 2011 2:42 AM, "Peter Hutterer" <peter.hutterer at who-t.net> wrote:
> > Hi Aapo,
> >
> > I noticed this patch in the synaptics repo today. Unfortunately, it needs
> > a bit more work, so I've reverted it for now. Please find my comments
> > inline.
> >
> > fwiw, patches to synatics should go to the xorg-devel list first for
> public
> > review.
> >
> > On Fri, Mar 18, 2011 at 04:26:46PM +1000, Peter Hutterer wrote:
> >> This patch allows usage of "synclient Orientation=0" (values from 0 to
> >> 3). It will rotate the touchpad similar to "xrandr -o". Original patch
> >> was extended for alps and ps2.
> >>
> >> Signed-off-by: Christoph Brill <egore911 at egore911.de>
> >> ---
> >> include/synaptics-properties.h | 3 +++
> >> man/synaptics.man | 6 ++++++
> >> src/alpscomm.c | 29 +++++++++++++++++++++++++----
> >> src/eventcomm.c | 22 ++++++++++++++++++++--
> >> src/properties.c | 8 ++++++++
> >> src/ps2comm.c | 36 +++++++++++++++++++++++++++++-------
> >> src/synaptics.c | 2 ++
> >> src/synapticsstr.h | 1 +
> >> tools/synclient.c | 1 +
> >> 9 files changed, 95 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/include/synaptics-properties.h
> b/include/synaptics-properties.h
> >> index bdb2112..0b4e570 100644
> >> --- a/include/synaptics-properties.h
> >> +++ b/include/synaptics-properties.h
> >> @@ -158,4 +158,7 @@
> >> /* 32 Bit Integer, 2 values, horizontal hysteresis, vertical hysteresis
> */
> >> #define SYNAPTICS_PROP_NOISE_CANCELLATION "Synaptics Noise Cancellation"
> >>
> >> +/* 32 bit, 4 values, normal, inverted, left, right */
> >> +#define SYNAPTICS_ORIENTATION "Synaptics Orientation"
> >
> > why not use degrees here?
> > this opens the way for a unified rotation property for devices that need a
> > rotation outside of 90 degree values.
> >
> > "left" doesn't have a clear meaning. "90 degrees clockwise" is less
> > ambiguous
> >
> > "orientation" vs "rotation"?
> > I'm more of a fan of the latter, but can be convinced otherwise.
> >
> >
> >> +
> >> #endif /* _SYNAPTICS_PROPERTIES_H_ */
> >> diff --git a/man/synaptics.man b/man/synaptics.man
> >> index 0a35883..44d1c27 100644
> >> --- a/man/synaptics.man
> >> +++ b/man/synaptics.man
> >> @@ -510,6 +510,12 @@ AreaBottomEdge option to any integer value other
> than zero. If supported by the
> >> server (version 1.9 and later), the edge may be specified in percent of
> >> the total height of the touchpad. Property: "Synaptics Area"
> >> .
> >> +.TP
> >> +.BI "Option \*qOrientation\*q \*q" integer \*q
> >> +Rotate the touchpad similar to xrandr -o.
> >> +.
> >> +The orientation can be 0 (normal), 1(left), 2 (inverted) or 3(right).
> >> +.
> >
> > same here, just because xrandr uses 0-3 doesn't make it a good idea ;)
> >
> >>
> >> .SH CONFIGURATION DETAILS
> >> .SS Area handling
> >> diff --git a/src/alpscomm.c b/src/alpscomm.c
> >> index dc76655..7d5cda2 100644
> >> --- a/src/alpscomm.c
> >> +++ b/src/alpscomm.c
> >> @@ -149,11 +149,12 @@ ALPS_get_packet(struct CommData *comm, InputInfoPtr
> pInfo)
> >> * reflects left,right,down,up in lef1,rig1,mid1,up1.
> >> */
> >> static void
> >> -ALPS_process_packet(unsigned char *packet, struct SynapticsHwState *hw)
> >> +ALPS_process_packet(SynapticsPrivate *priv, unsigned char *packet,
> struct SynapticsHwState *hw)
> >> {
> >> int x = 0, y = 0, z = 0;
> >> int left = 0, right = 0, middle = 0;
> >> int i;
> >> + SynapticsParameters *para = &priv->synpara;
> >>
> >> x = (packet[1] & 0x7f) | ((packet[2] & 0x78) << (7-3));
> >> y = (packet[4] & 0x7f) | ((packet[3] & 0x70) << (7-4));
> >> @@ -172,8 +173,27 @@ ALPS_process_packet(unsigned char *packet, struct
> SynapticsHwState *hw)
> >> hw->multi[i] = FALSE;
> >>
> >> if (z > 0) {
> >> - hw->x = x;
> >> - hw->y = y;
> >> + if (para->orientation==0)
> >> + hw->x = x;
> >> + else if (para->orientation==2)
> >
> > self-explanatory enums please, not magic numbers.
> > else if (para->orientation == ROTATION_90CW)
> > is much easier to read.
> >
> > (also, spaces before/after ==)
> >
> >> + hw->x = priv->maxx + priv->minx - x;
> >> + else if (para->orientation==3)
> >> + hw->y = (priv->maxx - x) * (priv->maxy - priv->miny) / (priv->maxx -
> priv->minx) + priv->miny;
> >> + else if (para->orientation==1)
> >> + hw->y = (x - priv->minx) * (priv->maxy - priv->miny) / (priv->maxx -
> priv->minx) + priv->miny;
> >> + else
> >> + hw->x = x;
> >> +
> >> + if (para->orientation==0)
> >> + hw->y = y;
> >> + else if (para->orientation==2)
> >> + hw->y = priv->maxy + priv->miny - y;
> >> + else if (para->orientation==3)
> >> + hw->x = (y - priv->miny) * (priv->maxx - priv->minx) / (priv->maxy -
> priv->miny) + priv->minx;
> >> + else if (para->orientation==1)
> >> + hw->x = (priv->maxy - y) * (priv->maxx - priv->minx) / (priv->maxy -
> priv->miny) + priv->minx;
> >> + else
> >> + hw->y = y;
> >
> > this needs to be done in a function to avoid duplicating the code.
> >
> >> }
> >> hw->z = z;
> >> hw->numFingers = (z > 0) ? 1 : 0;
> >> @@ -210,11 +230,12 @@ ALPSReadHwState(InputInfoPtr pInfo,
> >> {
> >> unsigned char *buf = comm->protoBuf;
> >> struct SynapticsHwState *hw = &(comm->hwState);
> >> + SynapticsPrivate *priv = (SynapticsPrivate *)pInfo->private;
> >>
> >> if (!ALPS_get_packet(comm, pInfo))
> >> return FALSE;
> >>
> >> - ALPS_process_packet(buf, hw);
> >> + ALPS_process_packet(priv, buf, hw);
> >>
> >> *hwRet = *hw;
> >> return TRUE;
> >> diff --git a/src/eventcomm.c b/src/eventcomm.c
> >> index d394d3f..3d550f1 100644
> >> --- a/src/eventcomm.c
> >> +++ b/src/eventcomm.c
> >> @@ -400,10 +400,28 @@ EventReadHwState(InputInfoPtr pInfo,
> >> case EV_ABS:
> >> switch (ev.code) {
> >> case ABS_X:
> >> - hw->x = ev.value;
> >> + if (para->orientation==0)
> >> + hw->x = ev.value;
> >> + else if (para->orientation==2)
> >> + hw->x = priv->maxx + priv->minx - ev.value;
> >> + else if (para->orientation==3)
> >> + hw->y = (priv->maxx - ev.value) * (priv->maxy - priv->miny) /
> (priv->maxx - priv->minx) + priv->miny;
> >> + else if (para->orientation==1)
> >> + hw->y = (ev.value - priv->minx) * (priv->maxy - priv->miny) /
> (priv->maxx - priv->minx) + priv->miny;
> >> + else
> >> + hw->x = ev.value;
> >> break;
> >> case ABS_Y:
> >> - hw->y = ev.value;
> >> + if (para->orientation==0)
> >> + hw->y = ev.value;
> >> + else if (para->orientation==2)
> >> + hw->y = priv->maxy + priv->miny - ev.value;
> >> + else if (para->orientation==3)
> >> + hw->x = (ev.value - priv->miny) * (priv->maxx - priv->minx) /
> (priv->maxy - priv->miny) + priv->minx;
> >> + else if (para->orientation==1)
> >> + hw->x = (priv->maxy - ev.value) * (priv->maxx - priv->minx) /
> (priv->maxy - priv->miny) + priv->minx;
> >> + else
> >> + hw->y = ev.value;
> >> break;
> >> case ABS_PRESSURE:
> >> hw->z = ev.value;
> >> diff --git a/src/properties.c b/src/properties.c
> >> index 23b5a6a..06909ed 100644
> >> --- a/src/properties.c
> >> +++ b/src/properties.c
> >> @@ -83,6 +83,7 @@ Atom prop_capabilities = 0;
> >> Atom prop_resolution = 0;
> >> Atom prop_area = 0;
> >> Atom prop_noise_cancellation = 0;
> >> +Atom prop_orientation = 0;
> >>
> >> static Atom
> >> InitAtom(DeviceIntPtr dev, char *name, int format, int nvalues, int
> *values)
> >> @@ -285,6 +286,8 @@ InitDeviceProperties(InputInfoPtr pInfo)
> >> prop_noise_cancellation = InitAtom(pInfo->dev,
> >> SYNAPTICS_PROP_NOISE_CANCELLATION, 32, 2, values);
> >>
> >> + prop_orientation = InitAtom(pInfo->dev, SYNAPTICS_ORIENTATION, 32, 1,
> &para->orientation);
> >> +
> >> }
> >>
> >> int
> >> @@ -666,6 +669,11 @@ SetProperty(DeviceIntPtr dev, Atom property,
> XIPropertyValuePtr prop,
> >> return BadValue;
> >> para->hyst_x = hyst[0];
> >> para->hyst_y = hyst[1];
> >> + } else if (property == prop_orientation)
> >> + {
> >> + if (prop->size != 1 || prop->format != 32 || prop->type != XA_INTEGER)
> >> + return BadMatch;
> >> + para->orientation = *(INT32*)prop->data;
> >> }
> >>
> >> return Success;
> >> diff --git a/src/ps2comm.c b/src/ps2comm.c
> >> index 0e9b861..1c2bbc3 100644
> >> --- a/src/ps2comm.c
> >> +++ b/src/ps2comm.c
> >> @@ -524,7 +524,7 @@ PS2ReadHwStateProto(InputInfoPtr pInfo,
> >> SynapticsParameters *para = &priv->synpara;
> >> struct PS2SynapticsHwInfo *synhw;
> >> int newabs;
> >> - int w, i;
> >> + int w, i, x, y;
> >>
> >> synhw = (struct PS2SynapticsHwInfo*)priv->proto_data;
> >> if (!synhw)
> >> @@ -541,17 +541,17 @@ PS2ReadHwStateProto(InputInfoPtr pInfo,
> >> return FALSE;
> >>
> >> /* Handle normal packets */
> >> - hw->x = hw->y = hw->z = hw->numFingers = hw->fingerWidth = 0;
> >> + hw->x = hw->y = hw->z = hw->numFingers = hw->fingerWidth = x = y = 0;
> >
> > urgh, just add a new line.
> >
> >> hw->left = hw->right = hw->up = hw->down = hw->middle = FALSE;
> >> for (i = 0; i < 8; i++)
> >> hw->multi[i] = FALSE;
> >>
> >> if (newabs) { /* newer protos...*/
> >> DBG(7, "using new protocols\n");
> >> - hw->x = (((buf[3] & 0x10) << 8) |
> >> + x = (((buf[3] & 0x10) << 8) |
> >> ((buf[1] & 0x0f) << 8) |
> >> buf[4]);
> >> - hw->y = (((buf[3] & 0x20) << 7) |
> >> + y = (((buf[3] & 0x20) << 7) |
> >> ((buf[1] & 0xf0) << 4) |
> >> buf[5]);
> >>
> >> @@ -598,9 +598,9 @@ PS2ReadHwStateProto(InputInfoPtr pInfo,
> >> }
> >> } else { /* old proto...*/
> >> DBG(7, "using old protocol\n");
> >> - hw->x = (((buf[1] & 0x1F) << 8) |
> >> + x = (((buf[1] & 0x1F) << 8) |
> >> buf[2]);
> >> - hw->y = (((buf[4] & 0x1F) << 8) |
> >> + y = (((buf[4] & 0x1F) << 8) |
> >> buf[5]);
> >>
> >> hw->z = (((buf[0] & 0x30) << 2) |
> >> @@ -612,7 +612,29 @@ PS2ReadHwStateProto(InputInfoPtr pInfo,
> >> hw->right = (buf[0] & 0x02) ? 1 : 0;
> >> }
> >>
> >> - hw->y = YMAX_NOMINAL + YMIN_NOMINAL - hw->y;
> >> + y = YMAX_NOMINAL + YMIN_NOMINAL - y;
> >> +
> >> + if (para->orientation==0)
> >> + hw->x = x;
> >> + else if (para->orientation==2)
> >> + hw->x = priv->maxx + priv->minx - x;
> >> + else if (para->orientation==3)
> >> + hw->y = (priv->maxx - x) * (priv->maxy - priv->miny) / (priv->maxx -
> priv->minx) + priv->miny;
> >> + else if (para->orientation==1)
> >> + hw->y = (x - priv->minx) * (priv->maxy - priv->miny) / (priv->maxx -
> priv->minx) + priv->miny;
> >> + else
> >> + hw->x = x;
> >> +
> >> + if (para->orientation==0)
> >> + hw->y = y;
> >> + else if (para->orientation==2)
> >> + hw->y = priv->maxy + priv->miny - y;
> >> + else if (para->orientation==3)
> >> + hw->x = (y - priv->miny) * (priv->maxx - priv->minx) / (priv->maxy -
> priv->miny) + priv->minx;
> >> + else if (para->orientation==1)
> >> + hw->x = (priv->maxy - y) * (priv->maxx - priv->minx) / (priv->maxy -
> priv->miny) + priv->minx;
> >> + else
> >> + hw->y = y;
> >
> > duplication again, needs abstraction.
> >
> >>
> >> if (hw->z >= para->finger_high) {
> >> int w_ok = 0;
> >> diff --git a/src/synaptics.c b/src/synaptics.c
> >> index 1233917..03a9f60 100644
> >> --- a/src/synaptics.c
> >> +++ b/src/synaptics.c
> >> @@ -574,6 +574,8 @@ static void set_default_parameters(InputInfoPtr
> pInfo)
> >> pars->resolution_horiz = xf86SetIntOption(opts, "HorizResolution",
> horizResolution);
> >> pars->resolution_vert = xf86SetIntOption(opts, "VertResolution",
> vertResolution);
> >>
> >> + pars->orientation = xf86SetIntOption(opts, "Orientation", 0);
> >> +
> >> /* Warn about (and fix) incorrectly configured TopEdge/BottomEdge
> parameters */
> >> if (pars->top_edge > pars->bottom_edge) {
> >> int tmp = pars->top_edge;
> >> diff --git a/src/synapticsstr.h b/src/synapticsstr.h
> >> index 8f6593e..90640f7 100644
> >> --- a/src/synapticsstr.h
> >> +++ b/src/synapticsstr.h
> >> @@ -161,6 +161,7 @@ typedef struct _SynapticsParameters
> >> 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 hyst_x, hyst_y; /* x and y width of hysteresis box */
> >> + int orientation; /* orientation of the touchpad */
> >> } SynapticsParameters;
> >>
> >>
> >> diff --git a/tools/synclient.c b/tools/synclient.c
> >> index 9776d23..1ac5502 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},
> >> + {"Orientation", PT_INT, 0, 3, SYNAPTICS_ORIENTATION, 32, 0},
> > ^ tab missing?
> >
> > Cheers,
> > Peter
> >
> >> { NULL, 0, 0, 0, 0 }
> >> };
> >>
> >> --
> >> 1.7.4
> > _______________________________________________
> > xorg-devel at lists.x.org: X.Org development
> > Archives: http://lists.x.org/archives/xorg-devel
> > Info: http://lists.x.org/mailman/listinfo/xorg-devel