Re: [Linuxwacom-devel] [PATCH] Input: wacom - add support for theDTI-520 graphics tablet

From: Ping Cheng
Date: Tue May 22 2012 - 18:12:36 EST

Next message: Tejun Heo: "Re: [Bcache v13 11/16] bcache: Core btree code"
Previous message: Joe Perches: "Re: [GIT PATCH] driver core patches for 3.5-rc1 - try 2"
In reply to: Adam Nielsen: "[PATCH] Input: wacom - add support for the DTI-520 graphics tablet"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sat, May 19, 2012 at 8:58 PM, Adam Nielsen <a.nielsen@xxxxxxxxxxx> wrote:
> Add support for the two interfaces provided by the Wacom DTI-520 tablet. This
> allows both the tablet itself as well as the hardware buttons to be seen by the
> kernel.
>
> Signed-off-by: Adam Nielsen <a.nielsen@xxxxxxxxxxx>
> ---
> Hi all,
>
> I'm resending this and CC'ing the linuxwacom-devel list as suggested. There
> are no changes since the previous post. This is the kernel code that makes the
> tablet appear as an input device.

I don't have the device to test this patch. Just to share my comments
(inline) based on the code itself. I am cc'ing linux-input, where this
patch normally goes.

Ping

>
> Cheers,
> Adam.
>
> drivers/input/tablet/wacom_sys.c | 13 +++++
> drivers/input/tablet/wacom_wac.c | 113 +++++++++++++++++++++++++++++++++++++-
> drivers/input/tablet/wacom_wac.h | 3 +
> 3 files changed, 128 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/tablet/wacom_sys.c b/drivers/input/tablet/wacom_sys.c
> index 0d26921..5d1e35d 100644
> --- a/drivers/input/tablet/wacom_sys.c
> +++ b/drivers/input/tablet/wacom_sys.c
> @@ -459,6 +459,19 @@ static int wacom_retrieve_hid_descriptor(struct usb_interface *intf,
> features->y_fuzz = 4;
> features->pressure_fuzz = 0;
> features->distance_fuzz = 0;
> + features->x_phy = 0;
> + features->y_phy = 0;
> +
> + /* DTI devices have two interfaces */
> + if (features->type == WACOM_DTI) {
> + if (interface->desc.bInterfaceNumber == 0) {
> + /* digitizer */
> + } else {
> + /* buttons */
> + features->device_type = 0;
> + features->pktlen = WACOM_PKGLEN_DTIBTN;
> + }
> + }
>
> /*
> * The wireless device HID is basic and layout conflicts with
> diff --git a/drivers/input/tablet/wacom_wac.c b/drivers/input/tablet/wacom_wac.c
> index cecd35c..f0ada18 100644
> --- a/drivers/input/tablet/wacom_wac.c
> +++ b/drivers/input/tablet/wacom_wac.c
> @@ -1073,6 +1073,80 @@ static int wacom_wireless_irq(struct wacom_wac *wacom, size_t len)
> return 0;
> }
>
> +static int wacom_dti_pen(struct wacom_wac *wacom)
> +{
> + struct wacom_features *features = &wacom->features;
> + char *data = wacom->data;
> + struct input_dev *input = wacom->input;
> + int pressure;
> + bool prox = data[1] & 0x40;
> +
> + if (data[0] != WACOM_REPORT_PENABLED) {
> + dbg("wacom_dti_pen: received unknown report #%d", data[0]);
> + return 0;
> + }
> +
> + input_report_key(input, wacom->tool[0], prox);

The line above should be after the if statement below. Otherwise
wacom->tool can be uninitialized.

> +
> + if (prox) {
> + wacom->tool[0] = BTN_TOOL_PEN;
> + wacom->id[0] = STYLUS_DEVICE_ID;
> + }
> + if (wacom->id[0]) {
> + input_report_key(input, BTN_STYLUS, data[7] & 0x01);
> + input_report_key(input, BTN_STYLUS2, data[7] & 0x02);
> + input_report_abs(input, ABS_X, be16_to_cpup((__be16 *)&data[2]));
> + input_report_abs(input, ABS_Y, be16_to_cpup((__be16 *)&data[4]));
> + pressure = (data[6] << 1) | ((data[7] & 0x80) >> 7);
> + if (pressure < 0)
> + pressure = features->pressure_max + pressure + 1;
> + input_report_abs(input, ABS_PRESSURE, pressure);

Need tp post wacom->id[0] through ABS_MISC here.

> +
> + return 1;
> + }
> +
> + if (!prox)
> + wacom->id[0] = 0;

This short if block should be inside the long if block above.

> +
> + return 0;
> +}
> +
> +static int wacom_dti_pad(struct wacom_wac *wacom)
> +{
> + char *data = wacom->data;
> + struct input_dev *input = wacom->input;
> +
> + if (data[0] == WACOM_REPORT_DTIBTN) {
> + input_report_key(input, BTN_3, data[1] & 0x01); /* up */
> + input_report_key(input, BTN_4, data[1] & 0x02); /* down */
> + input_report_key(input, BTN_0, data[1] & 0x04); /* L */
> + input_report_key(input, BTN_2, data[1] & 0x08); /* R */
> + input_report_key(input, BTN_1, data[1] & 0x10); /* both Ctrl */

Looks like we could define default vaules for those buttons.

> +
> + /* Buttons along the top of the display */
> + input_report_key(input, BTN_7, data[2] & 0x01);
> + input_report_key(input, BTN_5, data[2] & 0x02);
> + input_report_key(input, BTN_6, data[2] & 0x04);
> + input_report_key(input, BTN_8, data[2] & 0x08);
> + input_report_key(input, BTN_9, data[2] & 0x10);
> +
> + return 1;
> + }
> +
> + dbg("wacom_dti_pad: received unknown report #%d", data[0]);
> + return 0;
> +}
> +
> +static int wacom_dti_irq(struct wacom_wac *wacom, size_t len)
> +{
> + if (len == WACOM_PKGLEN_GRAPHIRE)
> + return wacom_dti_pen(wacom);
> + else if (len == WACOM_PKGLEN_DTIBTN)
> + return wacom_dti_pad(wacom);
> +
> + return 0;
> +}
> +
> void wacom_wac_irq(struct wacom_wac *wacom_wac, size_t len)
> {
> bool sync;
> @@ -1127,6 +1201,10 @@ void wacom_wac_irq(struct wacom_wac *wacom_wac, size_t len)
> sync = wacom_wireless_irq(wacom_wac, len);
> break;
>
> + case WACOM_DTI:
> + sync = wacom_dti_irq(wacom_wac, len);
> + break;
> +
> default:
> sync = false;
> break;
> @@ -1241,7 +1319,7 @@ void wacom_setup_input_capabilities(struct input_dev *input_dev,
> /* penabled devices have fixed resolution for each model */
> input_abs_set_res(input_dev, ABS_X, features->x_resolution);
> input_abs_set_res(input_dev, ABS_Y, features->y_resolution);
> - } else {
> + } else if ((features->x_phy > 0) && (features->y_phy > 0)) {
> input_abs_set_res(input_dev, ABS_X,
> wacom_calculate_touch_res(features->x_max,
> features->x_phy));
> @@ -1404,6 +1482,35 @@ void wacom_setup_input_capabilities(struct input_dev *input_dev,
> __set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
> break;
>
> + case WACOM_DTI:
> + __clear_bit(ABS_MISC, input_dev->absbit);

Why do we clear ABS_MISC for pen tool?

> + switch (features->device_type) {
> + case BTN_TOOL_PEN:
> + __set_bit(BTN_TOOL_PEN, input_dev->keybit);
> + __set_bit(BTN_STYLUS, input_dev->keybit);
> + __set_bit(BTN_STYLUS2, input_dev->keybit);
> +
> + __set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
> + break;
> +
> + case 0:
> + __set_bit(BTN_0, input_dev->keybit);
> + __set_bit(BTN_1, input_dev->keybit);
> + __set_bit(BTN_2, input_dev->keybit);
> + __set_bit(BTN_3, input_dev->keybit);
> + __set_bit(BTN_4, input_dev->keybit);
> +
> + __set_bit(BTN_5, input_dev->keybit);
> + __set_bit(BTN_6, input_dev->keybit);
> + __set_bit(BTN_7, input_dev->keybit);
> + __set_bit(BTN_8, input_dev->keybit);
> + __set_bit(BTN_9, input_dev->keybit);
> +
> + __clear_bit(BTN_TOUCH, input_dev->keybit);

This interface does not support ABS_X and ABS_Y either, right? Do we
need a tool type for this button box?

> + break;
> + }
> + break;
> +
> case PTU:
> __set_bit(BTN_STYLUS2, input_dev->keybit);
> /* fall through */
> @@ -1566,6 +1673,9 @@ static const struct wacom_features wacom_features_0x38 =
> static const struct wacom_features wacom_features_0x39 =
> { "Wacom DTU710", WACOM_PKGLEN_GRAPHIRE, 34080, 27660, 511,
> 0, PL, WACOM_PL_RES, WACOM_PL_RES };
> +static const struct wacom_features wacom_features_0x3A =
> + { "Wacom DTI520UB/L", WACOM_PKGLEN_GRAPHIRE, 6282, 4762, 511,
> + 0, WACOM_DTI, WACOM_PL_RES, WACOM_PL_RES };
> static const struct wacom_features wacom_features_0xC4 =
> { "Wacom DTF521", WACOM_PKGLEN_GRAPHIRE, 6282, 4762, 511,
> 0, PL, WACOM_PL_RES, WACOM_PL_RES };
> @@ -1780,6 +1890,7 @@ const struct usb_device_id wacom_ids[] = {
> { USB_DEVICE_WACOM(0x37) },
> { USB_DEVICE_WACOM(0x38) },
> { USB_DEVICE_WACOM(0x39) },
> + { USB_DEVICE_WACOM(0x3A) },
> { USB_DEVICE_WACOM(0xC4) },
> { USB_DEVICE_WACOM(0xC0) },
> { USB_DEVICE_WACOM(0xC2) },
> diff --git a/drivers/input/tablet/wacom_wac.h b/drivers/input/tablet/wacom_wac.h
> index ba5a334..008aa12 100644
> --- a/drivers/input/tablet/wacom_wac.h
> +++ b/drivers/input/tablet/wacom_wac.h
> @@ -15,6 +15,7 @@
> #define WACOM_PKGLEN_MAX 64
>
> /* packet length for individual models */
> +#define WACOM_PKGLEN_DTIBTN 3
> #define WACOM_PKGLEN_PENPRTN 7
> #define WACOM_PKGLEN_GRAPHIRE 8
> #define WACOM_PKGLEN_BBFUN 9
> @@ -35,6 +36,7 @@
>
> /* wacom data packet report IDs */
> #define WACOM_REPORT_PENABLED 2
> +#define WACOM_REPORT_DTIBTN 4
> #define WACOM_REPORT_INTUOSREAD 5
> #define WACOM_REPORT_INTUOSWRITE 6
> #define WACOM_REPORT_INTUOSPAD 12
> @@ -72,6 +74,7 @@ enum {
> WACOM_MO,
> TABLETPC,
> TABLETPC2FG,
> + WACOM_DTI,
> MAX_TYPE
> };
>
> -- 1.7.10
>
>
> ------------------------------------------------------------------------------
> Live Security Virtual Conference
> Exclusive live event will cover all the ways today's security and
> threat landscape has changed and how IT managers can respond. Discussions
> will include endpoint security, mobile security and the latest in malware
> threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
> _______________________________________________
> Linuxwacom-devel mailing list
> Linuxwacom-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Tejun Heo: "Re: [Bcache v13 11/16] bcache: Core btree code"
Previous message: Joe Perches: "Re: [GIT PATCH] driver core patches for 3.5-rc1 - try 2"
In reply to: Adam Nielsen: "[PATCH] Input: wacom - add support for the DTI-520 graphics tablet"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]