On Wed, Dec 28, 2011 at 1:48 PM, Chris Bagwell <chris@xxxxxxxxxxxxxx> wrote:
> On Tue, Dec 27, 2011 at 6:48 PM, Ping Cheng <pinglinux@xxxxxxxxx> wrote:
>> On Wed, Dec 21, 2011 at 8:14 PM, Chris Bagwell <chris@xxxxxxxxxxxxxx> wrote:
>>>
>>> On Wed, Dec 21, 2011 at 12:43 PM, Ping Cheng <pinglinux@xxxxxxxxx> wrote:
>>> > On Sun, Dec 18, 2011 at 6:07 PM, Chris Bagwell <chris@xxxxxxxxxxxxxx> wrote:
>>> >>
>>> >> On Fri, Dec 16, 2011 at 6:38 PM, Ping Cheng <pinglinux@xxxxxxxxx> wrote:
>>> >> > Signed-off-by: Ping Cheng <pingc@xxxxxxxxx>
>>> >> > ---
>>> >> > drivers/input/tablet/wacom_sys.c | 26 ++++++------
>>> >> > drivers/input/tablet/wacom_wac.c | 82
>>> >> > +++++++++++++++++++++++++++++++++++++-
>>> >> > drivers/input/tablet/wacom_wac.h | 8 ++++
>>> >> > 3 files changed, 101 insertions(+), 15 deletions(-)
>>> >> >
>>> >> > diff --git a/drivers/input/tablet/wacom_sys.c
>>> >> > b/drivers/input/tablet/wacom_sys.c
>>> >> > index b9736fb..eee06ef 100644
>>> >> > --- a/drivers/input/tablet/wacom_sys.c
>>> >> > +++ b/drivers/input/tablet/wacom_sys.c
>>> >> > @@ -297,6 +297,10 @@ static int wacom_parse_hid(struct usb_interface
>>> >> > *intf,
>>> >> > features->pktlen
>>> >> > = WACOM_PKGLEN_TPC2FG;
>>> >> >
>>> >> > features->touch_max = 2;
>>> >> > }
>>> >> > +
>>> >> > + if (features->type ==
>>> >> > MTSCREEN)
>>> >> > + features->pktlen
>>> >> > = WACOM_PKGLEN_MTOUCH;
>>> >> > +
>>> >> > if (features->type ==
>>> >> > BAMBOO_PT) {
>>> >> > /* need to reset
>>> >> > back */
>>> >> > features->pktlen
>>> >> > = WACOM_PKGLEN_BBTOUCH;
>>> >> > @@ -331,18 +335,15 @@ static int wacom_parse_hid(struct usb_interface
>>> >> > *intf,
>>> >> > case HID_USAGE_Y:
>>> >> > if (usage == WCM_DESKTOP) {
>>> >> > if (finger) {
>>> >> > - features->device_type =
>>> >> > BTN_TOOL_FINGER;
>>> >> > - if (features->type ==
>>> >> > TABLETPC2FG) {
>>> >> > - /* need to reset
>>> >> > back */
>>> >> > - features->pktlen
>>> >> > = WACOM_PKGLEN_TPC2FG;
>>> >> > + int type =
>>> >> > features->type;
>>> >> > +
>>> >> > + if (type == TABLETPC2FG
>>> >> > || type == MTSCREEN) {
>>> >> > features->y_max =
>>> >> >
>>> >> > get_unaligned_le16(&report[i + 3]);
>>> >> > features->y_phy =
>>> >> >
>>> >> > get_unaligned_le16(&report[i + 6]);
>>> >> > i += 7;
>>> >> > - } else if
>>> >> > (features->type == BAMBOO_PT) {
>>> >> > - /* need to reset
>>> >> > back */
>>> >> > - features->pktlen
>>> >> > = WACOM_PKGLEN_BBTOUCH;
>>> >> > + } else if (type ==
>>> >> > BAMBOO_PT) {
>>> >> > features->y_phy =
>>> >> >
>>> >> > get_unaligned_le16(&report[i + 3]);distracted me.
>>> >> > features->y_max =
>>> >> > @@ -356,10 +357,6 @@ static int wacom_parse_hid(struct usb_interface
>>> >> > *intf,
>>> >> > i += 4;
>>> >> > }
>>> >> > } else if (pen) {
>>> >> > - /* penabled only accepts
>>> >> > exact bytes of data */
>>> >> > - if (features->type ==
>>> >> > TABLETPC2FG)
>>> >> > - features->pktlen
>>> >> > = WACOM_PKGLEN_GRAPHIRE;
>>> >> > - features->device_type =
>>> >> > BTN_TOOL_PEN;
>>> >>
>>> >> All makes sense. Getting rid of duplicate code since X/Y will always
>>> >> be present. Maybe worth a 1 line comment now that its cleaned up so
>>> >> someone doesn't let it creep back in.
>>> >
>>> >
>>> > I'll add some comments in v2.
>>> >
>>> >>
>>> >> > features->y_max =
>>> >> >
>>> >> > get_unaligned_le16(&report[i + 3]);
>>> >> > i += 4;
>>> >> > @@ -432,7 +429,8 @@ static int wacom_query_tablet_data(struct
>>> >> > usb_interface *intf, struct wacom_feat
>>> >> >
>>> >> > /* ask to report tablet data if it is MT Tablet PC or
>>> >> > * not a Tablet PC */
>>> >> > - if (features->type == TABLETPC2FG) {
>>> >> > + if (((features->type == TABLETPC2FG) || (features->type ==
>>> >> > MTSCREEN))
>>> >> > + && features->device_type != BTN_TOOL_PEN) {
>>> >>
>>> >> Glad to see this device_type check. I thought it was probably needed.
>>> >>
>>> >> I'll have to trust you on this one but its different than Bamboo's.
>>> >> The set on Bamboo is on Pen and causes it to start sending pen
>>> >> packets. IIR, the touch packets are always sending. So this is
>>> >> opposite and enables something related to touch?
>>> >
>>> >
>>> > There are two types' of devices: tabletPC or non-tabletPC. For all tabletPC
>>> > devices that support more than one finger/contact, a set call is needed. For
>>> > all non-tabletPC devices, a set call is needed. Bamboo is a non-tabletPC
>>> > device.
>>> >
>>> >>
>>> >> Assuming you say yes, do you mind changing that to an affirmative
>>> >> check for TOUCH so it reads:
>>> >
>>> >
>>> > Well, it does not depend on whether it is touch or pen. It depends on the
>>> > type of the device as explained above. Do you still want me to use your
>>> > below logic?
>>>
>>> Let me ask this way. On Bamboo there are 2 interfaces, 1 pen and 1
>>> touch. I need to set feature report 2 to a specific value on the pen
>>> interface to get useful packets. If I set feature report 2 to
>>> something on touch interface, I get an error response back from
>>> device. This aligns with HID descriptor because only the pen
>>> interface declares that feature report #2.
>>>
>>> I'm sure its similar for Tablet PC's. Its also writing to feature
>>> report #2 but unique data values. What ever the values are, it
>>> probably also doesn't make sense to write to both pen interface and
>>> touch interface on Tablet PC.
>>>
>>> If the feature report is only on one of two interfaces I do suggest
>>> changing the if() to something like below to make it more obvious ("if
>>> (TOUCH)" is more obvious that your looking for specific interface then
>>> "if (!PEN)"). If its on both and only 1 needs to be written to then
>>> that would probably be a good comment as well.
>>
>> I see your point. I'll use TOUCH/PEN for the if-statement and add a comment.
>>
>
> Thanks.
>
>>> >>
>>> >> if (TOUCH)
>>> >> /* do TOUCH set's */
>>> >> if (PEN)
>>> >> /* do PEN set's */
>>> >>
>>> >>
>>> >> > do {
>>> >> > rep_data[0] = 3;
>>> >> > rep_data[1] = 4;
>>> >> > @@ -479,9 +477,9 @@ static int wacom_retrieve_hid_descriptor(struct
>>> >> > usb_interface *intf,
>>> >> > features->pressure_fuzz = 0;
>>> >> > features->distance_fuzz = 0;
>>> >> >
>>> >> > - /* only Tablet PCs and Bamboo P&T need to retrieve the info */
>>> >> > + /* only devices that support touch need to retrieve the info */
>>> >> > if ((features->type != TABLETPC) && (features->type !=
>>> >> > TABLETPC2FG) &&
>>> >> > - (features->type != BAMBOO_PT))
>>> >> > + (features->type != BAMBOO_PT) && (features->type !=
>>> >> > MTSCREEN))
>>> >> > goto out;
>>> >> >
>>> >> > if (usb_get_extra_descriptor(interface, HID_DEVICET_HID,
>>> >> > &hid_desc)) {
>>> >> > diff --git a/drivers/input/tablet/wacom_wac.c
>>> >> > b/drivers/input/tablet/wacom_wac.c
>>> >> > index 8dc185d..1a887a1 100644
>>> >> > --- a/drivers/input/tablet/wacom_wac.c
>>> >> > +++ b/drivers/input/tablet/wacom_wac.c
>>> >> > @@ -729,6 +729,73 @@ static int wacom_intuos_irq(struct wacom_wac
>>> >> > *wacom)
>>> >> > return 1;
>>> >> > }
>>> >> >
>>> >> > +static int wacom_mt_touch(struct wacom_wac *wacom)
>>> >> > +{
>>> >> > + struct wacom_features *features = &wacom->features;
>>> >> > + struct input_dev *input = wacom->input;
>>> >> > + char *data = wacom->data;
>>> >> > + struct input_mt_slot *mt;
>>> >> > + int i, id = -1, j = 0, k = 4;
>>> >> > + int x = 0, y = 0;
>>> >> > + int current_num_contacts = data[2];
>>> >> > + int contacts_to_send = 0;
>>> >> > + bool touch = false;
>>> >> > +
>>> >> > + /* reset the counter by the first packet */
>>> >> > + if (current_num_contacts) {
>>> >> > + features->num_contacts = current_num_contacts;
>>> >> > + features->num_contacts_left = current_num_contacts;
>>> >> > + }
>>> >>
>>> >> It seems that the saved contact values are only processed in case were
>>> >> data[2] indicates no contact counts in packet. I would think that
>>> >> packets with no contacts could be ignored (return) unless there is
>>> >> some data that must be implied. Why is it not ignored?
>>> >
>>> >
>>> > current_num_contacts is only reported, hence updated, by the first set of
>>> > data. Later touch data will be processed as long as number of contacts does
>>> > not exceed current_num_contacts.
>>> >
>>> >>
>>> >>
>>> >> > +
>>> >> > + /* There are at most 5 contacts per packet */
>>> >> > + contacts_to_send = min(5, (int)features->num_contacts_left);
>>> >>
>>> >> Its not obvious to me if data[2] contains touches in packet or total
>>> >> touches on screen.
>>> >
>>> >
>>> > Total touches on the screen.
>>>
>>> OK. I understand now. It would be nice to add that to above comment
>>> (the fact that num_contacts_left is total on screen).
>>
>> num_contacts_left is normally not the total on the screen. It is the
>> number of contacts that need to be processed. num_contacts is the
>> total on the screen. contacts_to_send is the number of contacts we are
>> going to send.
>
> OK. With understanding that data[2] is # of touches in packet and not
> total touches on screen then my questions change slightly.
>
> I assume if user put 6 touches a screen and moves around you'd get 2
> packets with movement; 1 with data[2] == 5 and 1 with data[2] == 1.
> There is this code then:
>
> int current_num_contacts = data[2];
> if (current_num_contacts) {
> features->num_contacts = current_num_contacts;
> features->num_contacts_left = current_num_contacts;
> }
>
> So it seems to me that num_contacts can never reach 6 (total touches
> on screen). Maybe that needs to be a += or something?
I get it now. In first touch packet received, data[2] will contain
the total touches to be reported. There will (data[2] / 5) additional
packets following this packet and they will all have data[2] set to 0.
So I understand how your logic is meant to work with that firmware
behaviour. If you could throw in something about data[2] == 0 being a
continuation packet then I think it would help future readers.
So that wraps up all the concerns/questions/comments I had on the patches.
Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
