On Fri, Jan 24, 2014 at 05:23:47PM -0800, Ping Cheng wrote: > Tools defined for devices that support both pen and touch need > to access the touch tool for arbitration and touch switch state > updates. Initially searching for the touch tool and storing it > to common->wcmTouchDevice saves time later when we need to access it. > > TabletFeature is updated with WCM_PENTOUCH for devices that > support both pen and touch too. > > Reviewed-by: Aaron Armstrong Skomra <sko...@gmail.com> > Signed-off-by: Ping Chdeng <pi...@wacom.com> > --- > src/wcmConfig.c | 101 > +++++++++++++++++++++++++++++--------------------------- > 1 file changed, 52 insertions(+), 49 deletions(-) > > diff --git a/src/wcmConfig.c b/src/wcmConfig.c > index 2d19944..a6d4fdc 100644 > --- a/src/wcmConfig.c > +++ b/src/wcmConfig.c > @@ -385,11 +385,17 @@ wcmInitModel(InputInfoPtr pInfo) > return TRUE; > } > > + > /** > - * Link the touch tool to the pen of the same device > - * so we can arbitrate the events when posting them. > + * Lookup to find the associated pen and touch for the same device. > + * Store touch tool in wcmTouchDevice for pen and touch, respectively, > + * of the same device. Update TabletFeature to indicate it is a hybrid > + * of touch and pen. > + * > + * @return True if found a touch tool for hybrid device. > + * false otherwise. > */ > -static void wcmLinkTouchAndPen(InputInfoPtr pInfo) > +static Bool wcmPenAndTouch(InputInfoPtr pInfo, Bool different_id)
Please document the "different_id" parameter as well, so it's obvious from the outset what it does. Maybe rename it to "allow_different_ids". > { > WacomDevicePtr priv = pInfo->private; > WacomCommonPtr common = priv->common; > @@ -397,65 +403,62 @@ static void wcmLinkTouchAndPen(InputInfoPtr pInfo) > WacomCommonPtr tmpcommon = NULL; > WacomDevicePtr tmppriv = NULL; > > - /* Lookup to find the associated pen and touch with same product id */ > for (; device != NULL; device = device->next) > { > - if (!strcmp(device->drv->driverName, "wacom")) > - { > - tmppriv = (WacomDevicePtr) device->private; > - tmpcommon = tmppriv->common; > + if (strcmp(device->drv->driverName, "wacom")) > + continue; > > - /* skip the same tool or already linked devices */ > - if ((tmppriv == priv) || tmpcommon->wcmTouchDevice) > - continue; > + tmppriv = (WacomDevicePtr) device->private; > + tmpcommon = tmppriv->common; > > - if (tmpcommon->tablet_id == common->tablet_id) > - { > - if (IsTouch(tmppriv) && IsTablet(priv)) > - common->wcmTouchDevice = tmppriv; > - else if (IsTouch(priv) && IsTablet(tmppriv)) > - tmpcommon->wcmTouchDevice = priv; > - > - if (common->wcmTouchDevice || > - tmpcommon->wcmTouchDevice) > - { > - TabletSetFeature(common, WCM_PENTOUCH); > - TabletSetFeature(tmpcommon, > WCM_PENTOUCH); > - } > - } > + /* skip the same tool or already linked devices */ > + if ((tmppriv == priv) || > + (tmpcommon->wcmTouchDevice && IsTablet(tmppriv))) > + continue; > > - if (common->wcmTouchDevice) > - return; > - } > - } > - > - /* Lookup for pen and touch devices with different product ids */ > - for (; device != NULL; device = device->next) > - { > - if (!strcmp(device->drv->driverName, "wacom")) > + if (((tmpcommon->tablet_id == common->tablet_id) && > !different_id) > + || different_id) if (((tmpcommon->tablet_id == common->tablet_id) || different_id) would be enough here for our purposes. > { > - tmppriv = (WacomDevicePtr) device->private; > - tmpcommon = tmppriv->common; > - > - /* skip the same tool or already linked devices */ > - if ((tmppriv == priv) || tmpcommon->wcmTouchDevice) > - continue; > - > - if (IsTouch(tmppriv) && IsTablet(priv)) > + if (IsTouch(tmppriv)) > + { > common->wcmTouchDevice = tmppriv; > - else if (IsTouch(priv) && IsTablet(tmppriv)) > + tmpcommon->wcmTouchDevice = tmppriv; > + } else if (IsTouch(priv)) > + { > tmpcommon->wcmTouchDevice = priv; > + common->wcmTouchDevice = priv; > + } > > - if (common->wcmTouchDevice || tmpcommon->wcmTouchDevice) > + if ((common->wcmTouchDevice && IsTablet(priv)) || > + (tmpcommon->wcmTouchDevice && IsTablet(tmppriv))) NAK to this. Please split this patch into the cleanup patch and a separate patch where the actual changes are made. Burying minor changes like this inside a cleanup patch is a bad idea, it makes it really hard to find what goes wrong in the future (or even figure out what's different). What I see here is that there are some checks for IsTablet removed and others added, but it's not clear if that's a need for this patch or something else. > { > TabletSetFeature(common, WCM_PENTOUCH); > TabletSetFeature(tmpcommon, WCM_PENTOUCH); > } > - > - if (common->wcmTouchDevice) > - return; > } > + > + if (common->wcmTouchDevice) > + return TRUE; > } > + return FALSE; > +} > + > +/** > + * Update festures for tablets that support both pen and touch. > + * Pen and touch of the same device can have same product id, > + * such as Intuos and most Cintiq series; or different product > + * ids, such as Cintiq 24HD. We look for pen and touch with same > + * product id first. If we do not find a touch tool, devices > + * with different product ids will be searched. > + */ > +static void wcmUpdatePenAndTouchInfo(InputInfoPtr pInfo) > +{ > + /* pen and touch with same product id */ > + if (wcmPenAndTouch(pInfo, FALSE)) > + return; > + else > + /* pen and touch devices with different product ids */ > + wcmPenAndTouch(pInfo, TRUE); why not the folowing? > + if (!wcmPenAndTouch(pInfo, FALSE)) > + wcmPenAndTouch(pInfo, TRUE); /* pen and touch devices with > different product ids */ Cheers, Peter > } > > /** > @@ -606,11 +609,11 @@ static int wcmPreInit(InputDriverPtr drv, InputInfoPtr > pInfo, int flags) > pInfo->fd = -1; > } > > - /* only link them once per port. We need to try for both tablet tool > + /* only update once per port. We need to try for both tablet tool > * and touch since we do not know which tool will be added first. > */ > if (IsTouch(priv) || (IsTablet(priv) && !common->wcmTouchDevice)) > - wcmLinkTouchAndPen(pInfo); > + wcmUpdatePenAndTouchInfo(pInfo); > > free(type); > free(oldname); > -- > 1.8.3.2 > ------------------------------------------------------------------------------ WatchGuard Dimension instantly turns raw network data into actionable security intelligence. It gives you real-time visual feedback on key security issues and trends. Skip the complicated setup - simply import a virtual appliance and go from zero to informed in seconds. http://pubads.g.doubleclick.net/gampad/clk?id=123612991&iu=/4140/ostg.clktrk _______________________________________________ Linuxwacom-devel mailing list Linuxwacom-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel