Custom Search

Re: [PATCH 4/4] input: wacom - add 0xE5 (MT device) support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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?

>
>> >>
>> >> > +
>> >> > +       for (i = 0; i < contacts_to_send; i++) {
>> >> > +               id = le16_to_cpup((__le16 *)&data[k]);
>> >> > +
>> >> > +               /* is there an existing slot for this contact? */
>> >> > +               for (j = 0; j < features->touch_max; j++) {
>> >> > +                       mt = &input->mt[j];
>> >> > +                       if (input_mt_get_value(mt, ABS_MT_TRACKING_ID)
>> >> > == id )
>> >> > +                               break;
>> >> > +               }
>> >> > +
>> >> > +               /* no. then find an unused slot to fill */
>> >> > +               if (j >= features->touch_max) {
>> >> > +                       for (j = 0; j < features->touch_max; j++) {
>> >> > +                               mt = &input->mt[j];
>> >> > +                               if (input_mt_get_value(mt,
>> >> > ABS_MT_TRACKING_ID) == -1 )
>> >> > +                                       break;
>> >> > +                       }
>> >> > +               }
>> >>
>> >> This may need a little more thought since hid-multitouch doesn't query
>> >> old value.  Is there a case were a touch is removed and new touch
>> >> added in same packet?  Probably that would never send the -1 to user
>> >> land.
>> >
>> >
>> > Once a touch leaves the screen, a -1 will always be sent to indicate the
>> > touch is out. When a new touch is added, it may use any available packet.
>> > Hence the one used by the just removed touch can also be used. Do I get your
>> > question properly?
>>
>> I'm thinking of this sequence inside 1 packet (probably not easy to
>> produce in the wild):
>>
>>  * 1st location in packet shows Finger with ID of 100 as not touching any more.
>>  * Loops threw and finds matching ID and sets to -1.
>>  * 2nd location in packet shows Finger with ID of 101 as touching.  Its new.
>>  * Loops threw to find 1st slot with -1 and uses one we just cleared.
>>  * Sync window occurs.  User only sees Contact ID change from 100 and 101.
>
> Oh, I know where you are. You are in the real world, not in the
> firmware ;). The firmware can not tell if the second finger (101) is
> the same finger as the first one (100) or not. All it can do is:
> report a finger out then a finger in. So, in any moment if there is
> one finger on the screen, it report one finger. If none, it reports
> none. If, by any chance, the second one (101) touches before the first
> one out, we get two finger data before one if out.
>
> Your case won't happen, at least in theory.

OK.  I only wanted to bring up the possibility.  Depending on firmware
design its possible that never could happen.

>
>> >> > +
>> >> > +               touch = data[k - 1] & 0x1;
>> >> > +               input_mt_slot(input, j);
>> >> > +               if (touch) {
>> >> > +                       x = le16_to_cpup((__le16 *)&data[k + 6]);
>> >> > +                       y = le16_to_cpup((__le16 *)&data[k + 8]);
>> >> > +
>> >> > +                       input_report_abs(input, ABS_MT_POSITION_X, x);
>> >> > +                       input_report_abs(input, ABS_MT_POSITION_Y, y);
>> >> > +               } else
>> >> > +                       id = -1;
>> >> > +
>> >> > +               /* keep the id from firmware since we need
>> >> > +                * it to process future events
>> >> > +                */
>> >> > +               input_report_abs(input, ABS_MT_TRACKING_ID, id);
>> >> > +
>> >> > +               k += WACOM_BYTES_PER_MT_PACKET;
>> >> > +       }
>> >> > +
>> >> > +       input_mt_report_pointer_emulation(input, true);
>> >> > +
>> >> > +       features->num_contacts_left -= contacts_to_send;
>> >> > +       if (features->num_contacts_left < 0)
>> >> > +               features->num_contacts_left = 0;
>> >>
>> >> Same comment as above.  Could use a comment on why storing this would be
>> >> needed.
>> >
>> >
>> > If you understood the value of current_num_contacts, it would be easier to
>> > see the need for num_contacts_left. I'll add a comment for
>> > current_num_contacts.
>>
>> I guess I still don't see why its needed or how its being used.
>>
>> Now that I understand that current_num_contacts is total touches, I do
>> understand you need to know when a packet contains 5 full touches and
>> when it contains less than 5.  For example if 6 touches then 1st
>> packet could have 5 and next packet could have 1.  So you need to know
>> to stop looping after 1.  But I do not see how this num_contacts_left
>> helps that.  It looks like it would set contacts_to_send to 5 in both
>> packets.
>
> Why do you think contacts_to_send could be 5? If num_contacts_left is
> 1, min(5, (int)features->num_contacts_left) should return 1, which is
> the contacts_to_send. Do I miss something?

I think it stemmed from my misunderstanding of what data[2] contained.
 If it contains 1 for 2nd packet with 6th finger then
num_contacts_left would be 1 as well and it would work.

With my new understanding that data[2] is touches in packet (1 to 5)
then it seems all this num_contacts and num_contacts_left is trying to
compute total touches on the screen but then its never used by
anything.  I'd get rid of it and just keep the camp of data[2] to <=
5/contacts_to_send part.

Chris

>
> Ping
--
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


[Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]    [Yosemite Photos]    [Free Online Dating]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux