Thanks for the patch! I've added my comments in-line below :)
Firstly though, it looks like your patch was mangled along the way, so
I haven't been able to give it a whirl... In the future I'd suggest
either attaching the patches as files, or using `git send-email`.
On Mon, Mar 24, 2014 at 6:13 PM, Paul A. Tessier <phern...@gmail.com> wrote:
> This adds support for the Wacom Wireless Kit that comes with the Intuos Pro
> tablets. There are a two main pieces, one is in input-wacom, and the other
> is in libwacom. xf86-input-wacom still needs to be patched to get the touch
> sensitivity on the buttons to work ...
>
> Changes to input-wacom:
>
> added read support for all the relevant sysfs attributes
> added support for crop mark brightness in sysfs
> corrected led sysfs attributes to work when connected to wireless
> added the vendor and product ids to the uniq field of the input devices
You should probably split these into multiple patches: One for the
fix, one for crop brightness, one for sleep (and powersave), and one
to export the vid:pid.
>
> Changes to libwacom:
>
> added support for reading uniq field when device is wireless
>
>
> The changes to libwacom were so that the tablet will show up as the same
> device whether plugged in or wireless, removing the need to create a custom
> wireless.tablet file and duplicate config settings in the
> gnome-control-center. KDE "should" behave similarly (didn't test).
>
>
> input-wacom:
>
> {3.7-0 => 3.7-1}/wacom.h | 2 +
> {3.7-0 => 3.7-1}/wacom_sys.c | 241
> ++++++++++++++++++++++++++++++++++++-------
> {3.7-0 => 3.7-1}/wacom_wac.h | 3 +
> 3 files changed, 208 insertions(+), 38 deletions(-)
Are you planning on submitting these patches upstream to Dmitry on the
linux-input mailinglist? We generally try to get code accepted
upstream before submitting input-wacom code, but its fine to hash out
the details here first.
Also, do you have plans to backport these patches to the 2.6.x
directories as well? They all seem to have basic LED support, though I
haven't looked all that closely.
>
> diff --git a/3.7-0/wacom.h b/3.7-1/wacom.h
> index b79d451..cb49f84 100644
> --- a/3.7-0/wacom.h
> +++ b/3.7-1/wacom.h
> @@ -121,8 +121,10 @@ struct wacom {
> u8 llv; /* status led brightness no button (1..127) */
> u8 hlv; /* status led brightness button pressed (1..127) */
> u8 img_lum; /* OLED matrix display brightness */
> + u8 crop_lum; /* crop marks led brightness (1..127) */
> } led;
> struct power_supply battery;
> + u8 sleep_timer; /* wireless tablet sleeps after X minutes idle (1..20)
> */
> };
>
> static inline void wacom_schedule_work(struct wacom_wac *wacom_wac)
> diff --git a/3.7-0/wacom_sys.c b/3.7-1/wacom_sys.c
> index 8de1cc3..0da44a7 100644
> --- a/3.7-0/wacom_sys.c
> +++ b/3.7-1/wacom_sys.c
> @@ -713,9 +713,19 @@ static void wacom_remove_shared_data(struct wacom_wac
> *wacom)
> static int wacom_led_control(struct wacom *wacom)
> {
> unsigned char *buf;
> + int buf_size;
> + int command;
> int retval;
> +
> + if (wacom->wacom_wac.features.quirks & WACOM_QUIRK_WIRELESS_KIT) {
> + command = 0x03;
You should introduce an e.g. WAC_CMD_WIRELESS_CONTROL macro rather
than use a magic constant.
> + buf_size = 13;
> + } else {
> + command = WAC_CMD_LED_CONTROL;
> + buf_size = 9;
> + }
>
> - buf = kzalloc(9, GFP_KERNEL);
> + buf = kzalloc(buf_size, GFP_KERNEL);
> if (!buf)
> return -ENOMEM;
>
> @@ -728,10 +738,19 @@ static int wacom_led_control(struct wacom *wacom)
> */
> int ring_led = wacom->led.select[0] & 0x03;
> int ring_lum = (((wacom->led.llv & 0x60) >> 5) - 1) & 0x03;
> - int crop_lum = 0;
> + int crop_lum = (((wacom->led.crop_lum & 0x60) >> 5) - 1) & 0x03;
Way more complex than it needs to be. `int crop_lum =
wacom->led.crop_lum & 0x03` should be all you need.
> + int bits = (crop_lum << 4) | (ring_lum << 2) | (ring_led);
>
> - buf[0] = WAC_CMD_LED_CONTROL;
> - buf[1] = (crop_lum << 4) | (ring_lum << 2) | (ring_led);
> + if (wacom->wacom_wac.features.quirks & WACOM_QUIRK_WIRELESS_KIT) {
> + buf[2] = wacom->sleep_timer;
This doesn't quite jibe with the protocol docs that I have. buf[2]
contains a "powersave" (EMR low-power mode) timer. The "sleep"
(EMR/MFT/wireless minimum power) timer is controlled by buf[1]. If
you'd like to export both timers through sysfs, that'd be awesome :)
> + buf[3] = 0x04;
> + buf[4] = 0x40 | bits;
> + } else {
> + buf[1] = bits;
> +
> + if (wacom->wacom_wac.features.type >= INTUOSPS)
> + buf[2] = ring_lum;
> + }
> }
> else {
> int led = wacom->led.select[0] | 0x4;
> @@ -740,15 +759,15 @@ static int wacom_led_control(struct wacom *wacom)
> wacom->wacom_wac.features.type == WACOM_24HD)
> led |= (wacom->led.select[1] << 4) | 0x40;
>
> - buf[0] = WAC_CMD_LED_CONTROL;
> buf[1] = led;
> buf[2] = wacom->led.llv;
> buf[3] = wacom->led.hlv;
> buf[4] = wacom->led.img_lum;
> }
>
> - retval = wacom_set_report(wacom->intf, 0x03, WAC_CMD_LED_CONTROL,
> - buf, 9, WAC_CMD_RETRIES);
> + buf[0] = command;
> + retval = wacom_set_report(wacom->intf, 0x03, command, buf, buf_size,
> + WAC_CMD_RETRIES);
> kfree(buf);
>
> return retval;
> @@ -825,7 +844,7 @@ static ssize_t wacom_led##SET_ID##_select_show(struct
> device *dev, \
> struct device_attribute *attr, char *buf) \
> { \
> struct wacom *wacom = dev_get_drvdata(dev); \
> - return snprintf(buf, 2, "%d\n", wacom->led.select[SET_ID]); \
> + return snprintf(buf, 3, "%d\n", wacom->led.select[SET_ID]); \
Keen eye.
> } \
> static DEVICE_ATTR(status_led##SET_ID##_select, S_IWUSR | S_IRUSR, \
> wacom_led##SET_ID##_select_show, \
> @@ -863,11 +882,19 @@ static ssize_t wacom_##name##_luminance_store(struct
> device *dev, \
> return wacom_luminance_store(wacom, &wacom->led.field, \
> buf, count); \
> } \
> -static DEVICE_ATTR(name##_luminance, S_IWUSR, \
> - NULL, wacom_##name##_luminance_store)
> +static ssize_t wacom_##name##_luminance_show(struct device *dev, \
> + struct device_attribute *attr, char *buf) \
> +{ \
> + struct wacom *wacom = dev_get_drvdata(dev); \
> + return snprintf(buf, 5, "%d\n", wacom->led.field); \
> +} \
> +static DEVICE_ATTR(name##_luminance, S_IWUSR | S_IRUSR, \
> + wacom_##name##_luminance_show, \
> + wacom_##name##_luminance_store)
>
> DEVICE_LUMINANCE_ATTR(status0, llv);
> DEVICE_LUMINANCE_ATTR(status1, hlv);
> +DEVICE_LUMINANCE_ATTR(crop_marks, crop_lum);
> DEVICE_LUMINANCE_ATTR(buttons, img_lum);
>
> static ssize_t wacom_button_image_store(struct device *dev, int button_id,
> @@ -906,6 +933,48 @@ DEVICE_BTNIMG_ATTR(5);
> DEVICE_BTNIMG_ATTR(6);
> DEVICE_BTNIMG_ATTR(7);
>
> +static ssize_t wacom_sleep_timer_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct wacom *wacom = dev_get_drvdata(dev);
> + unsigned int time;
> + int err;
> +
> + err = kstrtouint(buf, 10, &time);
> + if (err)
> + return err;
> +
> + mutex_lock(&wacom->lock);
> +
> + wacom->sleep_timer = clamp_val(time, 1, 20);
Valid values are from 5 to 60 (below the tablet defaults to 15 minute
sleep; above it disables sleep). For the powersave timer, they're 1 to
60 (below, 1 minute default; above disabled)
We might want to have seperate attributes that allow the user to
explicitely disable/default the timers, or possibly allow these to
take special values (0 = disabled, -1 = default?). Whatever the
decision, our sysfs documentaiton should be updated to reflect it.
> + err = wacom_led_control(wacom);
> +
> + mutex_unlock(&wacom->lock);
> +
> + return err < 0 ? err : count;
> +}
> +
> +static ssize_t wacom_sleep_timer_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct wacom *wacom = dev_get_drvdata(dev);
> + return snprintf(buf, 5, "%d\n", wacom->sleep_timer);
Only need 4, not 5.
> +}
> +
> +static DEVICE_ATTR(sleep_timer, S_IWUSR | S_IRUSR, wacom_sleep_timer_show,
> + wacom_sleep_timer_store);
> +
> +static struct attribute *wireless_attrs[] = {
> + &dev_attr_sleep_timer.attr,
> + NULL
> +};
> +
> +static struct attribute_group wireless_attr_group = {
> + .name = "wacom_wireless",
> + .attrs = wireless_attrs,
> +};
> +
> static struct attribute *cintiq_led_attrs[] = {
> &dev_attr_status_led0_select.attr,
> &dev_attr_status_led1_select.attr,
> @@ -940,6 +1009,7 @@ static struct attribute_group intuos4_led_attr_group =
> {
>
> static struct attribute *intuos5_led_attrs[] = {
> &dev_attr_status0_luminance.attr,
> + &dev_attr_crop_marks_luminance.attr,
> &dev_attr_status_led0_select.attr,
> NULL
> };
> @@ -990,6 +1060,7 @@ static int wacom_initialize_leds(struct wacom *wacom)
> wacom->led.select[1] = 0;
> wacom->led.llv = 32;
> wacom->led.hlv = 0;
> + wacom->led.crop_lum = 32;
Should be `wacom->led.crop_lum = 2`
> wacom->led.img_lum = 0;
>
> error = sysfs_create_group(&wacom->intf->dev.kobj,
> @@ -1004,7 +1075,7 @@ static int wacom_initialize_leds(struct wacom *wacom)
>
> if (error) {
> dev_err(&wacom->intf->dev,
> - "cannot create sysfs group err: %d\n", error);
> + "cannot create 'wacom_led' sysfs group: err %d\n", error);
> return error;
> }
> wacom_led_control(wacom);
> @@ -1111,7 +1182,17 @@ static int wacom_register_input(struct wacom *wacom)
> goto fail1;
> }
>
> + if (wacom_wac->uniq[0] == 0) {
> + if (wacom_wac->features.quirks & WACOM_QUIRK_WIRELESS_KIT)
> + snprintf(wacom_wac->uniq, WACOM_UNIQ_MAX, "056a:%04x",
> + wacom_wac->pid);
Instead of hardcoding "056a" into the string, use USB_VENDOR_ID_WACOM.
> + else
> + snprintf(wacom_wac->uniq, WACOM_UNIQ_MAX, "%04x:%04x",
> + dev->descriptor.idVendor, dev->descriptor.idProduct);
> + }
> +
> input_dev->name = wacom_wac->name;
> + input_dev->uniq = wacom_wac->uniq;
The tablet VID:PID isn't a unique enough identifier to be stored in
the `uniq` field. You should add a sysfs attribute to the wireless
group which exports the information.
> input_dev->dev.parent = &intf->dev;
> input_dev->open = wacom_open;
> input_dev->close = wacom_close;
> @@ -1136,6 +1217,102 @@ fail1:
> return error;
> }
>
> +static void wacom_unregister_wireless(struct wacom *wacom)
> +{
> + struct wacom_features *wacom_features = &wacom->wacom_wac.features;
> +
> + if (!(wacom_features->quirks & WACOM_QUIRK_WIRELESS_KIT))
> + return;
> +
> + switch (wacom_features->type) {
> + case INTUOSPS:
> + case INTUOSPM:
> + case INTUOSPL:
All wireless tablets should have this particular group, IIRC. Bamboo
PIDs 0xDC-0xDF, Intuos, Intuos5, and Intuos Pro.
> + if (wacom_features->device_type == BTN_TOOL_PEN) {
> + sysfs_remove_group(&wacom->intf->dev.kobj,
> + &wireless_attr_group);
> + }
> + break;
> + }
> +}
> +
> +static int wacom_register_wireless(struct wacom *wacom)
> +{
> + struct wacom_wac *wacom_wac = &wacom->wacom_wac;
> + struct wacom_features *wacom_features = &wacom_wac->features;
> + int error;
> +
> + if (!(wacom_features->quirks & WACOM_QUIRK_WIRELESS_KIT))
> + return 0;
> +
> + /* Initialize default values */
> + switch (wacom_features->type) {
> + case INTUOSPS:
> + case INTUOSPM:
> + case INTUOSPL:
See above comment.
> + if (wacom_features->device_type == BTN_TOOL_PEN) {
> + wacom->sleep_timer = 2;
The default value for this and the powersave timer are 15 minutes and
1 minute respectively. You should set them to zero to use the hardware
default.
> +
> + error = sysfs_create_group(&wacom->intf->dev.kobj,
> + &wireless_attr_group);
> + } else
> + return 0;
> + break;
> +
> + default:
> + return 0;
> + }
> +
> + if (error) {
> + dev_err(&wacom->intf->dev,
> + "cannot create 'wacom_wireless' sysfs group: err %d\n",
> + error);
> + return error;
> + }
> + wacom_led_control(wacom);
> +
> + return 0;
> +}
> +
> +static void wacom_unregister(struct wacom *wacom)
> +{
> + struct wacom_wac *wacom_wac = &wacom->wacom_wac;
> +
> + if (wacom_wac->input) {
> + input_unregister_device(wacom_wac->input);
> + wacom_unregister_wireless(wacom);
> + wacom_destroy_leds(wacom);
> + }
> +
> + wacom_wac->input = NULL;
> + wacom_wac->uniq[0] = 0;
> +}
> +
> +static int wacom_register(struct wacom *wacom)
> +{
> + int error;
> +
> + error = wacom_initialize_leds(wacom);
> + if (error)
> + goto fail1;
> +
> +
> + error = wacom_register_wireless(wacom);
> + if (error)
> + goto fail2;
> +
> + error = wacom_register_input(wacom);
> + if (error)
> + goto fail3;
> +
> + return 0;
> +
> +fail3: wacom_unregister_wireless(wacom);
> +fail2: wacom_destroy_leds(wacom);
> +fail1:
> + return error;
> +}
> +
> static void wacom_wireless_work(struct work_struct *work)
> {
> struct wacom *wacom = container_of(work, struct wacom, work);
> @@ -1155,16 +1332,12 @@ static void wacom_wireless_work(struct work_struct
> *work)
> /* Stylus interface */
> wacom1 = usb_get_intfdata(usbdev->config->interface[1]);
> wacom_wac1 = &(wacom1->wacom_wac);
> - if (wacom_wac1->input)
> - input_unregister_device(wacom_wac1->input);
> - wacom_wac1->input = NULL;
> + wacom_unregister(wacom1);
>
> /* Touch interface */
> wacom2 = usb_get_intfdata(usbdev->config->interface[2]);
> wacom_wac2 = &(wacom2->wacom_wac);
> - if (wacom_wac2->input)
> - input_unregister_device(wacom_wac2->input);
> - wacom_wac2->input = NULL;
> + wacom_unregister(wacom2);
>
> if (wacom_wac->pid == 0) {
> dev_info(&wacom->intf->dev, "wireless tablet disconnected\n");
> @@ -1187,6 +1360,9 @@ static void wacom_wireless_work(struct work_struct
> *work)
> "ignoring unknown PID.\n");
> return;
> }
> +
> + wacom_wac1->pid = wacom_wac->pid;
> + wacom_wac2->pid = wacom_wac->pid;
>
> /* Stylus interface */
> wacom_wac1->features =
> @@ -1196,7 +1372,8 @@ static void wacom_wireless_work(struct work_struct
> *work)
> wacom_wac1->features.name);
> wacom_wac1->shared->touch_max = wacom_wac1->features.touch_max;
> wacom_wac1->shared->type = wacom_wac1->features.type;
> - error = wacom_register_input(wacom1);
> + wacom_wac1->features.quirks |= WACOM_QUIRK_WIRELESS_KIT;
> + error = wacom_register(wacom1);
> if (error)
> goto fail;
>
> @@ -1214,7 +1391,8 @@ static void wacom_wireless_work(struct work_struct
> *work)
> else
> snprintf(wacom_wac2->name, WACOM_NAME_MAX,
> "%s (WL) Pad",wacom_wac2->features.name);
> - error = wacom_register_input(wacom2);
> + wacom_wac2->features.quirks |= WACOM_QUIRK_WIRELESS_KIT;
> + error = wacom_register(wacom2);
> if (error)
> goto fail;
>
> @@ -1231,15 +1409,8 @@ static void wacom_wireless_work(struct work_struct
> *work)
> return;
>
> fail:
> - if (wacom_wac2->input) {
> - input_unregister_device(wacom_wac2->input);
> - wacom_wac2->input = NULL;
> - }
> -
> - if (wacom_wac1->input) {
> - input_unregister_device(wacom_wac1->input);
> - wacom_wac1->input = NULL;
> - }
> + wacom_unregister(wacom2);
> + wacom_unregister(wacom1);
> return;
> }
>
> @@ -1374,14 +1545,10 @@ static int wacom_probe(struct usb_interface *intf,
> const struct usb_device_id *i
> wacom->irq->transfer_dma = wacom->data_dma;
> wacom->irq->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
>
> - error = wacom_initialize_leds(wacom);
> - if (error)
> - goto fail4;
> -
> if (!(features->quirks & WACOM_QUIRK_NO_INPUT)) {
> - error = wacom_register_input(wacom);
> + error = wacom_register(wacom);
> if (error)
> - goto fail5;
> + goto fail4;
> }
>
> /* Note that if query fails it is not a hard failure */
> @@ -1403,7 +1570,7 @@ static int wacom_probe(struct usb_interface *intf,
> const struct usb_device_id *i
>
> return 0;
>
> - fail5: wacom_destroy_leds(wacom);
> + fail5: wacom_unregister(wacom);
> fail4: wacom_remove_shared_data(wacom_wac);
> fail3: usb_free_urb(wacom->irq);
> fail2: usb_free_coherent(dev, WACOM_PKGLEN_MAX, wacom_wac->data,
> wacom->data_dma);
> @@ -1419,10 +1586,8 @@ static void wacom_disconnect(struct usb_interface
> *intf)
>
> usb_kill_urb(wacom->irq);
> cancel_work_sync(&wacom->work);
> - if (wacom->wacom_wac.input)
> - input_unregister_device(wacom->wacom_wac.input);
> + wacom_unregister(wacom);
> wacom_destroy_battery(wacom);
> - wacom_destroy_leds(wacom);
> usb_free_urb(wacom->irq);
> usb_free_coherent(interface_to_usbdev(intf), WACOM_PKGLEN_MAX,
> wacom->wacom_wac.data, wacom->data_dma);
> diff --git a/3.7-0/wacom_wac.h b/3.7-1/wacom_wac.h
> index f69c0eb..1cd54df 100644
> --- a/3.7-0/wacom_wac.h
> +++ b/3.7-1/wacom_wac.h
> @@ -15,6 +15,7 @@
> #define WACOM_PKGLEN_MAX 68
>
> #define WACOM_NAME_MAX 64
> +#define WACOM_UNIQ_MAX 16
>
> /* packet length for individual models */
> #define WACOM_PKGLEN_PENPRTN 7
> @@ -65,6 +66,7 @@
> #define WACOM_QUIRK_BBTOUCH_LOWRES 0x0002
> #define WACOM_QUIRK_NO_INPUT 0x0004
> #define WACOM_QUIRK_MONITOR 0x0008
> +#define WACOM_QUIRK_WIRELESS_KIT 0x0016
>
> enum {
> PENPARTNER = 0,
> @@ -144,6 +146,7 @@ struct wacom_shared {
>
> struct wacom_wac {
> char name[WACOM_NAME_MAX];
> + char uniq[WACOM_UNIQ_MAX];
> unsigned char *data;
> int tool[2];
> int id[2];
>
>
> libwacom:
>
> {libwacom-0 => libwacom-1}/libwacom.c | 33
> ++++++++++++++++++++++++++++++---
> 1 file changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/libwacom-0/libwacom.c b/libwacom-1/libwacom.c
> index 574e6f3..2967846 100644
> --- a/libwacom-0/libwacom.c
> +++ b/libwacom-1/libwacom.c
> @@ -47,6 +47,18 @@
> #define INPUT_PROP_DIRECT 0x01 /* direct input devices */
> #endif
>
> +
> +#define WACOM_WIRELESS_KIT_VENDOR_ID 0x056a
> +#define WACOM_WIRELESS_KIT_PRODUCT_ID 0x0084
> +
> +static gboolean
> +is_wireless(int vendor_id, int product_id)
> +{
> + return vendor_id == WACOM_WIRELESS_KIT_VENDOR_ID
> + && product_id == WACOM_WIRELESS_KIT_PRODUCT_ID;
> +}
> +
> +
> static const WacomDevice *
> libwacom_get_device(const WacomDeviceDatabase *db, const char *match)
> {
> @@ -207,7 +219,7 @@ get_device_info (const char *path,
>
> *bus = bus_from_str (bus_str);
> if (*bus == WBUSTYPE_USB) {
> - const char *vendor_str, *product_str;
> + const char *vendor_str, *product_str, *uniq_str;
> GUdevDevice *parent;
>
> vendor_str = g_udev_device_get_property (device, "ID_VENDOR_ID");
> @@ -221,14 +233,29 @@ get_device_info (const char *path,
> if (parent) {
> vendor_str = g_udev_device_get_property (parent,
> "ID_VENDOR_ID");
> product_str = g_udev_device_get_property (parent,
> "ID_MODEL_ID");
> + g_object_unref (parent);
> }
> }
>
> *vendor_id = strtol (vendor_str, NULL, 16);
> *product_id = strtol (product_str, NULL, 16);
> +
> + /* check for wireless kit enabled tablet */
> + if (is_wireless (*vendor_id, *product_id)) {
> + char *sysfs_path, *contents;
> + sysfs_path = g_build_filename ("/sys/class/input", devname,
> "device/uniq", NULL);
> + if (g_file_get_contents (sysfs_path, &contents, NULL, NULL)) {
> + if (sscanf(contents, "%x:%x", vendor_id, product_id) != 2) {
> + libwacom_error_set(error, WERROR_UNKNOWN_MODEL, "Unable to
> parse wireless tablet model");
> + g_free (contents);
> + g_free (sysfs_path);
> + goto bail;
> + }
> + g_free (contents);
> + }
> + g_free (sysfs_path);
> + }
>
> - if (parent)
> - g_object_unref (parent);
> } else if (*bus == WBUSTYPE_BLUETOOTH || *bus == WBUSTYPE_SERIAL) {
> GUdevDevice *parent;
> const char *product_str;
>
Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....
------------------------------------------------------------------------------
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
