On Fri, Jan 17, 2014 at 05:54:33PM -0800, Ping Cheng wrote: > On Thu, Jan 16, 2014 at 8:49 PM, Peter Hutterer > <peter.hutte...@who-t.net>wrote: > > > On Tue, Jan 14, 2014 at 04:27:01PM -0800, Ping Cheng wrote: > > > On Mon, Jan 13, 2014 at 1:45 AM, Peter Hutterer < > > peter.hutte...@who-t.net>wrote: > > > > > > > On Wed, Jan 08, 2014 at 02:50:11PM -0800, Ping Cheng wrote: > > > > > wcmLinkTouchAndPen only assigned touch to tablet (pen) interface. > > > > > New Intuos series requires another tool (pad in this case) on the > > > > > same interface as touch to access touch tool as well. That is, touch > > > > > tool should be assigned to wcmTouchDevice for its own common struct. > > > > > > > > I tried to review this patch but I had a really hard time parsing the > > above > > > > and my head is spinning from trying to grasp from the diff. Can you > > > > rephrase > > > > what this does, it'll get easer to review then. sorry > > > > > > > > > > Before rephrasing the commit comment, I'd like to explain why we need the > > > patch. > > > > > > This patch is to support patch 2/2 v2. It addresses your suggestion of > > > removing the for-loop in wcmUpdateHWTouchProperty, to my initial "Support > > > hardware touch switch" patch. > > > > > > For those who have relatively short memories, here is the history. For > > USB > > > devices that support both touch and pen, we have two interfaces, one for > > > touch and the other for pen. So, for each dual input device, we have two > > > WacomCommon structs. These two stucts do not share tools. In order for > > pen > > > tools to tell touch to ignore its events when pen is in prox, we added > > > routine wcmLinkTouchAndPen to assign touch as wcmTouchDevice to the > > > WacomCommonPtr of pen. That was good. > > > > > > However, we now want to replace priv with something like > > > common->wcmTouchDevice in wcmUpdateHWTouchProperty. Without assigning > > touch > > > to touch interface's common->wcmTouchDevice, driver crashes since > > > wcmTouchDevice is NULL (unassigned). > > > > thanks for the explanation. so if I understand this correctly, you want to > > have a bi-directional link between the two 'common' devices instead of a > > single-directional link from tablet to touch. If that's correct please > > rename wcmTouchDevice to wcmLinkedDevice or something, with a bunch of > > comments explaining what it links to. > > > > Yes, you get this part right. > > Updating function name is not a big deal once we resolve other issues below. > > > > > If you wonder which priv in usbDispatchEvents is receiving SW_MUTE_DEVICE > > > event, it was PAD tool. Newer Intuos series has PAD and touch tools on > > the > > > same interface, same as those Bamboos. > > > > hmm, so if the event is received on the pad device, which already has a > > link > > to the touch device's common struct why do we need to update this? I must > > be > > missing something else here. > > > > Even we know touch is on the same interface with pad (which is not always > true), the struct common does not give us the pointer to access the touch > tool. We have to go through the whole list of tools associated with common > to find touch every time when we need to update TouchSwitchState. So, the > repeated for-loop can not be avoided unless we find and KEEP it during > initialization steps in wcmConfig.c.
sorry, what I mean't wasn't the "common" struct but the common->wcmTouchDevice. That should point to the actual touch device, right? I don't quite understand yet why something like this isn't sufficient in the event handling code: case SW_MUTE: WacomDevicePtr touch; if (IsTouch(device)) touch = device; else touch = device->common->wcmTouchDevice; wcmUpdateHWSWitchProperty(touch); > > > Does this information put your head to a proper position? If not, please > > > give me your exact position, I'll try my best to locate it ;-). > > > > there are more questions: > > first - there are two loops in wcmLinkTouchAndPen() but you only update the > > second loop, is that intentional? > > > Yes. > > There are no pen and touch devices with different product ids that support > touch switch yet. > > > > if so, why, and please add a comment to > > that extent. and if it's not too much hassle imo it's better to update both > > loops, just because it'll make it easier to understand. > > > > Maybe we want to be proactive for this support? yes, definitely. it's not something that's obvious when reading it and it doesn't cost us much to run a quick loop here. > > second - that second loops is never executed. Either we link and return, or > > we don't link in which case "device" is NULL and the loop is never entered. > > do we still need that second for loop? > > > > That is a bug, unrelated to the patch we are discussing. "device = > xf86FirstLocalDevice();" is needed for the second loop. > > > if the answer is that both need to be updated/fixed, then I'd like you to > > move the loops into a helper function with a boolean for compare_id. > > they're > > identical except for the tablet_id check and so you could do something like > > > > if (!wcmLinkDevices(pInfo, TRUE)) /* compare tablet_ids */ > > wcmLinkDevices(pInfo, FALSE); /* ok, try to link without comparing */ > > > > it'll reduce the code and make it easier to spot bugs like the one > > mentioned > > above. > > > > I can do that. The question is, should we consider different product ids > now? > > I think it is a better choice to consider it now. Just need your > confirmation. yes, I agree. > > (also, I'm not a big fan of loop bodies containing a single if statement, > > it's usually better to have, in this case, a if (not wacom driver) > > continue; > > to enhance readability. > > > > I have no preferences. So, either way, I can update it. yes please. Cheers, Peter > > > > > > > > > > The use of IsTablet is necessary to guarantee we link pen and touch > > > > > tools. > > > > > > > > > > Signed-off-by: Ping Cheng <pi...@wacom.com> > > > > > --- > > > > > src/wcmConfig.c | 21 ++++++++++++++++----- > > > > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/src/wcmConfig.c b/src/wcmConfig.c > > > > > index 2d19944..6891ca6 100644 > > > > > --- a/src/wcmConfig.c > > > > > +++ b/src/wcmConfig.c > > > > > @@ -388,6 +388,11 @@ wcmInitModel(InputInfoPtr pInfo) > > > > > /** > > > > > * Link the touch tool to the pen of the same device > > > > > * so we can arbitrate the events when posting them. > > > > > + * As a by-product, we also assign the touch tool to > > > > > + * its own common wcmTouchDevice so other tool on the > > > > > + * same interface can access it, such as those devices > > > > > + * that have expresskeys (pad) and touch on the same > > > > > + * interface like Bamboo and new Intuos series. > > > > > */ > > > > > static void wcmLinkTouchAndPen(InputInfoPtr pInfo) > > > > > { > > > > > @@ -406,18 +411,24 @@ static void wcmLinkTouchAndPen(InputInfoPtr > > pInfo) > > > > > tmpcommon = tmppriv->common; > > > > > > > > > > /* skip the same tool or already linked > > devices */ > > > > > - if ((tmppriv == priv) || > > tmpcommon->wcmTouchDevice) > > > > > + if ((tmppriv == priv) || > > > > > + (tmpcommon->wcmTouchDevice && > > > > IsTablet(tmppriv))) > > > > > continue; > > > > > > > > > > if (tmpcommon->tablet_id == common->tablet_id) > > > > > { > > > > > - 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))) > > > > > { > > > > > TabletSetFeature(common, > > > > WCM_PENTOUCH); > > > > > TabletSetFeature(tmpcommon, > > > > WCM_PENTOUCH); > > > > > -- > > > > > 1.8.3.2 > > > > > > > > > > > ------------------------------------------------------------------------------ CenturyLink Cloud: The Leader in Enterprise Cloud Services. Learn Why More Businesses Are Choosing CenturyLink Cloud For Critical Workloads, Development Environments & Everything In Between. Get a Quote or Start a Free Trial Today. http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk _______________________________________________ Linuxwacom-devel mailing list Linuxwacom-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel