* [PATCH] HID: wacom - Bamboo pen only tablet does not support PAD @ 2014-11-17 21:45 Ping Cheng 2014-11-17 22:38 ` Benjamin Tissoires 0 siblings, 1 reply; 3+ messages in thread From: Ping Cheng @ 2014-11-17 21:45 UTC (permalink / raw) To: jkosina; +Cc: linux-input, Ping Cheng Bamboo models do not support HID_DG_CONTACTMAX. We should not rely on that description to decide touch_max for them. Plus, no Bamboo device supports single touch. Signed-off-by: Ping Cheng <pingc@wacom.com> --- drivers/hid/wacom_sys.c | 4 ++-- drivers/hid/wacom_wac.c | 30 ++++++++++++++++++++++-------- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c index 8593047..46147b4 100644 --- a/drivers/hid/wacom_sys.c +++ b/drivers/hid/wacom_sys.c @@ -192,8 +192,8 @@ static void wacom_usage_mapping(struct hid_device *hdev, if (!pen && !finger) return; - if (finger && !features->touch_max) - /* touch device at least supports one touch point */ + if (finger && !features->touch_max && (features->type != BAMBOO_PT)) + /* ISDv4 touch devices at least supports one touch point */ features->touch_max = 1; switch (usage->hid) { diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c index 586b240..fc9b492 100644 --- a/drivers/hid/wacom_wac.c +++ b/drivers/hid/wacom_wac.c @@ -2402,7 +2402,7 @@ int wacom_setup_pad_input_capabilities(struct input_dev *input_dev, case INTUOSPS: /* touch interface does not have the pad device */ if (features->device_type != BTN_TOOL_PEN) - return 1; + goto no_pad; for (i = 0; i < 7; i++) __set_bit(BTN_0 + i, input_dev->keybit); @@ -2447,22 +2447,36 @@ int wacom_setup_pad_input_capabilities(struct input_dev *input_dev, case BAMBOO_PT: /* pad device is on the touch interface */ if (features->device_type != BTN_TOOL_FINGER) - return 1; + goto no_pad; __clear_bit(ABS_MISC, input_dev->absbit); - __set_bit(BTN_LEFT, input_dev->keybit); - __set_bit(BTN_FORWARD, input_dev->keybit); - __set_bit(BTN_BACK, input_dev->keybit); - __set_bit(BTN_RIGHT, input_dev->keybit); + /* Bamboo Pen only tablet does not have pad */ + if ((features->type != BAMBOO_PT) || + ((features->type == BAMBOO_PT) && features->touch_max)) { + __set_bit(BTN_LEFT, input_dev->keybit); + __set_bit(BTN_FORWARD, input_dev->keybit); + __set_bit(BTN_BACK, input_dev->keybit); + __set_bit(BTN_RIGHT, input_dev->keybit); + } else + goto no_pad; break; default: - /* no pad supported */ - return 1; + goto no_pad; } return 0; + +no_pad: + input_dev->evbit[0] &= ~BIT_MASK(EV_KEY) & ~BIT_MASK(EV_ABS); + __clear_bit(ABS_MISC, input_dev->absbit); + __clear_bit(ABS_X, input_dev->absbit); + __clear_bit(ABS_Y, input_dev->absbit); + __clear_bit(BTN_STYLUS, input_dev->keybit); + + return 1; + } static const struct wacom_features wacom_features_0x00 = -- 1.9.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] HID: wacom - Bamboo pen only tablet does not support PAD 2014-11-17 21:45 [PATCH] HID: wacom - Bamboo pen only tablet does not support PAD Ping Cheng @ 2014-11-17 22:38 ` Benjamin Tissoires 2014-11-18 0:13 ` Ping Cheng 0 siblings, 1 reply; 3+ messages in thread From: Benjamin Tissoires @ 2014-11-17 22:38 UTC (permalink / raw) To: Ping Cheng; +Cc: Jiri Kosina, linux-input, Ping Cheng Hi Ping, On Mon, Nov 17, 2014 at 4:45 PM, Ping Cheng <pinglinux@gmail.com> wrote: > Bamboo models do not support HID_DG_CONTACTMAX. We should not rely > on that description to decide touch_max for them. Plus, no Bamboo > device supports single touch. > > Signed-off-by: Ping Cheng <pingc@wacom.com> > --- > drivers/hid/wacom_sys.c | 4 ++-- > drivers/hid/wacom_wac.c | 30 ++++++++++++++++++++++-------- > 2 files changed, 24 insertions(+), 10 deletions(-) > > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c > index 8593047..46147b4 100644 > --- a/drivers/hid/wacom_sys.c > +++ b/drivers/hid/wacom_sys.c > @@ -192,8 +192,8 @@ static void wacom_usage_mapping(struct hid_device *hdev, > if (!pen && !finger) > return; > > - if (finger && !features->touch_max) > - /* touch device at least supports one touch point */ > + if (finger && !features->touch_max && (features->type != BAMBOO_PT)) > + /* ISDv4 touch devices at least supports one touch point */ Can you also explain here that the report descriptor of the Bamboos contains the touch even if the device itself has not the capability? IMO, it will help further readings when we will try to understand this particular case. > features->touch_max = 1; > > switch (usage->hid) { > diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c > index 586b240..fc9b492 100644 > --- a/drivers/hid/wacom_wac.c > +++ b/drivers/hid/wacom_wac.c > @@ -2402,7 +2402,7 @@ int wacom_setup_pad_input_capabilities(struct input_dev *input_dev, > case INTUOSPS: > /* touch interface does not have the pad device */ > if (features->device_type != BTN_TOOL_PEN) > - return 1; > + goto no_pad; There is actually no need to clean up the pad on error. When an error is raised, the associated pad input device is simply freed, so the cleanup add superfluous steps. Maybe a cleaner way to handle that would be to return -ENODEV. This will show that this is an abort and that the pad input device will not be used anymore. > > for (i = 0; i < 7; i++) > __set_bit(BTN_0 + i, input_dev->keybit); > @@ -2447,22 +2447,36 @@ int wacom_setup_pad_input_capabilities(struct input_dev *input_dev, > case BAMBOO_PT: > /* pad device is on the touch interface */ > if (features->device_type != BTN_TOOL_FINGER) > - return 1; > + goto no_pad; > > __clear_bit(ABS_MISC, input_dev->absbit); > > - __set_bit(BTN_LEFT, input_dev->keybit); > - __set_bit(BTN_FORWARD, input_dev->keybit); > - __set_bit(BTN_BACK, input_dev->keybit); > - __set_bit(BTN_RIGHT, input_dev->keybit); > + /* Bamboo Pen only tablet does not have pad */ > + if ((features->type != BAMBOO_PT) || > + ((features->type == BAMBOO_PT) && features->touch_max)) { I would prefer keeping the standard path as it is, and add a special test above where you bail out. We will be able to add further special cases as needed, and not stack the parenthesis :) [looks like it is bikeshedding day for me... sorry] > + __set_bit(BTN_LEFT, input_dev->keybit); > + __set_bit(BTN_FORWARD, input_dev->keybit); > + __set_bit(BTN_BACK, input_dev->keybit); > + __set_bit(BTN_RIGHT, input_dev->keybit); > + } else > + goto no_pad; > > break; > > default: > - /* no pad supported */ > - return 1; > + goto no_pad; > } > return 0; > + > +no_pad: > + input_dev->evbit[0] &= ~BIT_MASK(EV_KEY) & ~BIT_MASK(EV_ABS); > + __clear_bit(ABS_MISC, input_dev->absbit); > + __clear_bit(ABS_X, input_dev->absbit); > + __clear_bit(ABS_Y, input_dev->absbit); > + __clear_bit(BTN_STYLUS, input_dev->keybit); > + > + return 1; > + > } > > static const struct wacom_features wacom_features_0x00 = > -- > 1.9.1 > FWIW, there is no regressions with this patch regarding the devices we have at Red Hat :) /me just updated hid-replay and played with it... Cheers, Benjamin ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] HID: wacom - Bamboo pen only tablet does not support PAD 2014-11-17 22:38 ` Benjamin Tissoires @ 2014-11-18 0:13 ` Ping Cheng 0 siblings, 0 replies; 3+ messages in thread From: Ping Cheng @ 2014-11-18 0:13 UTC (permalink / raw) To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input, Ping Cheng Hi Benjamin, Thank you for the review. An updated version will be posted soon. Let me know if you need us to capture data from more devices for your testing. Cheers, Ping On Mon, Nov 17, 2014 at 2:38 PM, Benjamin Tissoires <benjamin.tissoires@gmail.com> wrote: > Hi Ping, > > On Mon, Nov 17, 2014 at 4:45 PM, Ping Cheng <pinglinux@gmail.com> wrote: >> Bamboo models do not support HID_DG_CONTACTMAX. We should not rely >> on that description to decide touch_max for them. Plus, no Bamboo >> device supports single touch. >> >> Signed-off-by: Ping Cheng <pingc@wacom.com> >> --- >> drivers/hid/wacom_sys.c | 4 ++-- >> drivers/hid/wacom_wac.c | 30 ++++++++++++++++++++++-------- >> 2 files changed, 24 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c >> index 8593047..46147b4 100644 >> --- a/drivers/hid/wacom_sys.c >> +++ b/drivers/hid/wacom_sys.c >> @@ -192,8 +192,8 @@ static void wacom_usage_mapping(struct hid_device *hdev, >> if (!pen && !finger) >> return; >> >> - if (finger && !features->touch_max) >> - /* touch device at least supports one touch point */ >> + if (finger && !features->touch_max && (features->type != BAMBOO_PT)) >> + /* ISDv4 touch devices at least supports one touch point */ > > Can you also explain here that the report descriptor of the Bamboos > contains the touch even if the device itself has not the capability? > IMO, it will help further readings when we will try to understand this > particular case. > >> features->touch_max = 1; >> >> switch (usage->hid) { >> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c >> index 586b240..fc9b492 100644 >> --- a/drivers/hid/wacom_wac.c >> +++ b/drivers/hid/wacom_wac.c >> @@ -2402,7 +2402,7 @@ int wacom_setup_pad_input_capabilities(struct input_dev *input_dev, >> case INTUOSPS: >> /* touch interface does not have the pad device */ >> if (features->device_type != BTN_TOOL_PEN) >> - return 1; >> + goto no_pad; > > There is actually no need to clean up the pad on error. > When an error is raised, the associated pad input device is simply > freed, so the cleanup add superfluous steps. > > Maybe a cleaner way to handle that would be to return -ENODEV. This > will show that this is an abort and that the pad input device will not > be used anymore. > >> >> for (i = 0; i < 7; i++) >> __set_bit(BTN_0 + i, input_dev->keybit); >> @@ -2447,22 +2447,36 @@ int wacom_setup_pad_input_capabilities(struct input_dev *input_dev, >> case BAMBOO_PT: >> /* pad device is on the touch interface */ >> if (features->device_type != BTN_TOOL_FINGER) >> - return 1; >> + goto no_pad; >> >> __clear_bit(ABS_MISC, input_dev->absbit); >> >> - __set_bit(BTN_LEFT, input_dev->keybit); >> - __set_bit(BTN_FORWARD, input_dev->keybit); >> - __set_bit(BTN_BACK, input_dev->keybit); >> - __set_bit(BTN_RIGHT, input_dev->keybit); >> + /* Bamboo Pen only tablet does not have pad */ >> + if ((features->type != BAMBOO_PT) || >> + ((features->type == BAMBOO_PT) && features->touch_max)) { > > I would prefer keeping the standard path as it is, and add a special > test above where you bail out. We will be able to add further special > cases as needed, and not stack the parenthesis :) > > [looks like it is bikeshedding day for me... sorry] > >> + __set_bit(BTN_LEFT, input_dev->keybit); >> + __set_bit(BTN_FORWARD, input_dev->keybit); >> + __set_bit(BTN_BACK, input_dev->keybit); >> + __set_bit(BTN_RIGHT, input_dev->keybit); >> + } else >> + goto no_pad; >> >> break; >> >> default: >> - /* no pad supported */ >> - return 1; >> + goto no_pad; >> } >> return 0; >> + >> +no_pad: >> + input_dev->evbit[0] &= ~BIT_MASK(EV_KEY) & ~BIT_MASK(EV_ABS); >> + __clear_bit(ABS_MISC, input_dev->absbit); >> + __clear_bit(ABS_X, input_dev->absbit); >> + __clear_bit(ABS_Y, input_dev->absbit); >> + __clear_bit(BTN_STYLUS, input_dev->keybit); >> + >> + return 1; >> + >> } >> >> static const struct wacom_features wacom_features_0x00 = >> -- >> 1.9.1 >> > > FWIW, there is no regressions with this patch regarding the devices we > have at Red Hat :) > /me just updated hid-replay and played with it... > > Cheers, > Benjamin ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-11-18 0:13 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-17 21:45 [PATCH] HID: wacom - Bamboo pen only tablet does not support PAD Ping Cheng 2014-11-17 22:38 ` Benjamin Tissoires 2014-11-18 0:13 ` Ping Cheng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox