On Thu, Mar 31, 2011 at 04:25:23PM -0700, Ping Cheng wrote:
> On Wed, Mar 30, 2011 at 11:17 PM, Peter Hutterer
> > > + if ((tmpcommon->tablet_id == common->tablet_id) &&
> > > + tmppriv != priv)
> > > + {
> > > + if (IsTouch(tmppriv) && IsPen(priv))
> > > + {
> > > + common->wcmPointerToTouch =
> > tmppriv;
> > > + common->tablet_type |=
> > WCM_PENTOUCH;
> > > + tmpcommon->tablet_type |=
> > WCM_PENTOUCH;
> > > + }
> > > + else if (IsTouch(priv) && IsPen(tmppriv))
> > > + {
> > > + tmpcommon->wcmPointerToTouch =
> > priv;
> > > + common->tablet_type |=
> > WCM_PENTOUCH;
> > > + tmpcommon->tablet_type |=
> > WCM_PENTOUCH;
> > > + }
> >
> >
> > this can be simplified with the if/else if condition setting a tmp variable
> > for the new priv assignment and then a shared block for the three actual
> > assignments.
> >
>
> Not really. Although priv can be set to tmppriv, both common and tmpcommon
> are needed which can not be used with one name. Plus, since it is a if/else
> if, there would be cases outside of those statements that we should not
> assign any values except continue.
for (; device != NULL; device = device->next)
{
WacomCommonPtr tmpcommon = NULL;
WacomDevicePtr tmppriv = NULL;
/* FIXME: why strstr and not strcmp? */
if (!strstr(device->drv->driverName, "wacom"))
continue;
tmppriv = (WacomDevicePtr) device->private;
tmpcommon = tmppriv->common;
if (tmppriv == priv)
continue;
if ((tmpcommon->tablet_id == common->tablet_id))
{
WacomCommonPtr ptr_dev = NULL;
WacomDevicePtr touch_dev = NULL;
if (IsTouch(tmppriv) && IsPen(priv))
{
ptr_dev = common;
touch_dev = tmppriv;
}
else if (IsTouch(priv) && IsPen(tmppriv))
{
ptr_dev = tmpcommon;
touch_dev = priv;
}
if (ptr_dev && touch_dev)
{
ptr_dev->wcmPointerToTouch = touch_dev;
common->tablet_type |= WCM_PENTOUCH;
tmpcommon->tablet_type |= WCM_PENTOUCH;
}
}
}
and while writing this I noticed another thing. IIRC touch and pen are two
kernel devices (this is what you refer to as port, right?). would it make
sense to have them share the common struct once we've linked them?
> > + }
> > > + }
> > > + }
> > > +}
> > > +
> > > /* wcmPreInit - called for each input devices with the driver set to
> > > * "wacom" */
> > > #if GET_ABI_MAJOR(ABI_XINPUT_VERSION) < 12
> > > @@ -518,6 +557,12 @@ static int wcmPreInit(InputDriverPtr drv,
> > InputInfoPtr pInfo, int flags)
> > > pInfo->fd = -1;
> > > }
> > >
> > > + /* only link them once per port. We need to try for both pen and
> > touch
> > > + * since we do not know which tool (touch or pen) will be added
> > first.
> > > + */
> > > + if (IsTouch(priv) || (IsPen(priv) && !common->wcmPointerToTouch))
> > > + wcmLinkTouchAndPen(pInfo);
> > > +
> > > return Success;
> > >
> > > SetupProc_fail:
> > > diff --git a/src/xf86WacomDefs.h b/src/xf86WacomDefs.h
> > > index 43348fc..60d6426 100644
> > > --- a/src/xf86WacomDefs.h
> > > +++ b/src/xf86WacomDefs.h
> > > @@ -173,6 +173,7 @@ struct _WacomModel
> > > #define WCM_TPC (0x00000200 | WCM_LCD) /* TabletPC
> > (special
> > > button handling,
> > > always an LCD) */
> > > +#define WCM_PENTOUCH 0x00000400 /* Tablet supports pen and touch
> > */
> > > #define TabletHasFeature(common, feature) (((common)->tablet_type &
> > (feature)) != 0)
> > >
> > > #define ABSOLUTE_FLAG 0x00000100
> > > @@ -421,6 +422,9 @@ struct _WacomCommonRec
> > > int fd; /* file descriptor to tablet */
> > > int fd_refs; /* number of references to fd; if =0,
> > fd is invalid */
> > > unsigned long wcmKeys[NBITS(KEY_MAX)]; /* supported tool types for
> > the device */
> > > + WacomDevicePtr wcmPointerToTouch; /* The pointer for pen to access
> > the
> > > + touch tool of the same device id
> > */
> >
> > the term pointer in X is so badly overloaded that I can't recommend using
> > it. how about just "wcmTouchDevice"?
> >
> > keeping this pointer gives us a race condition similar to the hotplugging
> > one we had a while ago. if the touch device is removed first and tablet
> > still sends event, the new block in commonDispatchDevice may dereference
> > the
> > already deleted pointer.
> > You need a matching wcmUnlinkTouchAndPen() called during DEVICE_OFF.
> >
>
> I will think about it a bit more and do some testing to be sure.
remember, race conditions you can _only_ trigger reliably by recreating the
exact conditions they may occur in (e.g. with gdb).
Cheers,
Peter
> Thank you for the review. I'll make a v2 after I hear your reply about the
> if/else if statement discussion.
>
> Ping
------------------------------------------------------------------------------
Create and publish websites with WebMatrix
Use the most popular FREE web apps or write code yourself;
WebMatrix provides all the features you need to develop and
publish your website. http://p.sf.net/sfu/ms-webmatrix-sf
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
