Ping's out for a little while, so I'll be taking over the patch. I've got other stuff on my plate though, so it may take some time to fold in the changes.
On Thu, Jan 19, 2012 at 8:13 PM, Peter Hutterer <[email protected]> wrote: > Thank you, I CC'd Eduard here to, he may have some comments related to his > code. > > On Wed, Jan 18, 2012 at 04:25:42PM -0800, Ping Cheng wrote: >> Signed-off-by: Ping Cheng <[email protected]> >> --- >> Changes to v1: >> Updated test_parameter_number; >> Added DBG to report sysfs node. >> --- >> include/wacom-properties.h | 3 ++ >> man/xsetwacom.man | 7 ++++ >> src/wcmCommon.c | 4 ++- >> src/wcmConfig.c | 67 >> +++++++++++++++++++++++++++++++++++++++++++- >> src/wcmXCommand.c | 39 +++++++++++++++++++++++++- >> src/xf86Wacom.c | 14 ++++++++- >> src/xf86WacomDefs.h | 6 +++- >> tools/xsetwacom.c | 18 +++++++++++- >> 8 files changed, 152 insertions(+), 6 deletions(-) >> >> diff --git a/include/wacom-properties.h b/include/wacom-properties.h >> index 0bb84b1..b2a3523 100644 >> --- a/include/wacom-properties.h >> +++ b/include/wacom-properties.h >> @@ -46,6 +46,9 @@ >> */ >> #define WACOM_PROP_STRIPBUTTONS "Wacom Strip Buttons" >> >> +/* 8 bit, 2 values, LED0 (right side) and LED1 (left side) */ >> +#define WACOM_PROP_LED "Wacom LEDs" > > I'd prefer "Wacom LED Luminosity" or something that hints at _what_ the > property affects on the LEDs. In the future we may need a "Wacom LED > colors", so we should differentiate from the start. > That sounds like a reasonable name, especially with the possibility of multiple LED brightness levels (which I also think I remember seeing something about) >> + >> /* 8 bit, 6 values, rel wheel up, rel wheel down, abs wheel up, abs wheel >> down, abs wheel 2 up, abs wheel 2 down >> OR >> Atom, 6 values , rel wheel up, rel wheel down, abs wheel up, abs wheel >> down, abs wheel 2 up, abs wheel 2 down >> diff --git a/man/xsetwacom.man b/man/xsetwacom.man >> index dc0995f..97da416 100644 >> --- a/man/xsetwacom.man >> +++ b/man/xsetwacom.man >> @@ -201,6 +201,13 @@ sets the max distance from tablet to stop reporting >> movement for cursor in >> relative mode. Default for Intuos series is 10, for Graphire series >> (including >> Volitos) is 42. Only available for the cursor/puck device. >> .TP >> +\fBLED0\fR status >> +Set the right LED status of an Intuos4/Cintiq 21UX/Cintiq 24HD. Default: 0, >> +range of 0 to 3. > > what do those numbers refer to? These numbers refer to the LED number that should be lit. At most one LED in a bank may be lit at a time (both a hardware and sysfs interface limitation). I haven't run the code myself, but it looks like running "xsetwacom set <id> LED0 2" on an Intuos4 would result in the third LED lighting up. Running "xsetwacom set <id> LED1 2" should have no effect since the Intuos4 has only one bank of LEDs. It should be noted that the 24HD is an odd case. All other tablets have 4 LEDs per bank and one of them will always be lit. The 24HD has three LEDs, and trying to set the 4th LED actually results in all the LEDs in that bank turning off since the "lit" LED is non-existent. > We'll likely have more LEDs in the future, so an interface of > xsetwacom set ... LED 1 <status> seems better, similar to the Button > interface. > I'd actually argue for "xsetwacom set <id> LED <bank> <led number>" rather than a linear mapping. > also, IIRC at some point we talked about 255 levels for LEDs? > The kernel appears to support 128 levels through sysfs (see "status0_luminance" and "status1_luminance"), so that could be added as well. This would have to be done in a patch of its own though since I don't have enough time to do the development and testing... >> +.TP >> +\fBLED1\fR status >> +Set the left LED status of a Cintiq 21UX/24HD. Default: 0, range of 0 to 3. >> +.TP >> \fBThreshold\fR level >> Set the minimum pressure necessary to generate a Button event for the stylus >> tip, eraser, or touch. The pressure levels of all tablets are normalized to >> diff --git a/src/wcmCommon.c b/src/wcmCommon.c >> index 0f041e3..e4a8630 100644 >> --- a/src/wcmCommon.c >> +++ b/src/wcmCommon.c >> @@ -1,6 +1,6 @@ >> /* >> * Copyright 1995-2002 by Frederic Lepied, France. <[email protected]> >> - * Copyright 2002-2010 by Ping Cheng, Wacom. <[email protected]> >> + * Copyright 2002-2012 by Ping Cheng, Wacom. <[email protected]> >> * >> * This program is free software; you can redistribute it and/or >> * modify it under the terms of the GNU General Public License >> @@ -1472,6 +1472,8 @@ WacomCommonPtr wcmNewCommon(void) >> common->wcmMaxStripY = 4096; /* Max fingerstrip Y */ >> common->wcmMaxtiltX = 128; /* Max tilt in X directory */ >> common->wcmMaxtiltY = 128; /* Max tilt in Y directory */ >> + common->fd_sysfs0 = -1; /* file descriptor to sysfs led0 */ >> + common->fd_sysfs1 = -1; /* file descriptor to sysfs led1 */ > > array please > >> common->wcmCursorProxoutDistDefault = PROXOUT_INTUOS_DISTANCE; >> /* default to Intuos */ >> common->wcmSuppress = DEFAULT_SUPPRESS; >> diff --git a/src/wcmConfig.c b/src/wcmConfig.c >> index 5920e11..aff8b0f 100644 >> --- a/src/wcmConfig.c >> +++ b/src/wcmConfig.c >> @@ -1,6 +1,6 @@ >> /* >> * Copyright 1995-2002 by Frederic Lepied, France. <[email protected]> >> - * Copyright 2002-2010 by Ping Cheng, Wacom. <[email protected]> >> + * Copyright 2002-2012 by Ping Cheng, Wacom. <[email protected]> >> * >> * This program is free software; you can redistribute it and/or >> * modify it under the terms of the GNU General Public License >> @@ -21,6 +21,7 @@ >> #include <config.h> >> #endif >> >> +#include <libudev.h> >> #include "xf86Wacom.h" >> #include "wcmFilter.h" >> #include <sys/stat.h> >> @@ -441,6 +442,65 @@ static int wcmIsHotpluggedDevice(InputInfoPtr pInfo) >> return !strcmp(source, "_driver/wacom"); >> } >> >> +static void wcmStoreLEDsysfsInfo(InputInfoPtr pInfo) >> +{ >> + WacomDevicePtr priv = (WacomDevicePtr)pInfo->private; >> + WacomCommonPtr common = priv->common; >> + struct stat st; >> + struct udev *udev = udev_new(); >> + struct udev_device *parent; >> + struct udev_device *device; >> + char fs_path[128], buf[10]; >> + int err = -1; >> + >> + stat(common->device_path, &st); >> + device = udev_device_new_from_devnum(udev, 'c', st.st_rdev); >> + if (!device) >> + return; >> + >> + parent = udev_device_get_parent_with_subsystem_devtype(device, >> + "usb", NULL); >> + >> + if (!parent) >> + return; > > leaking udev device, please use a goto out system like in model_from_sysfs > >> + >> + sprintf(fs_path, "%s/wacom_led/status_led0_select", >> + udev_device_get_syspath(parent)); >> + common->fd_sysfs0 = open(fs_path, O_RDWR); >> + if (common->fd_sysfs0 >= 0) >> + { >> + SYSCALL(err = read(common->fd_sysfs0, buf, 1)); >> + if (err < -1) >> + { >> + xf86Msg(X_WARNING, "%s: failed to get led0 status in " >> + "wcmStoreLEDsysfsInfo.\n", pInfo->name); > > __func__, not harcoded function numbers. Plus, function names are for > debugging, there is no need for this here. we only have one place where we > extract LED status. > >> + } >> + else >> + common->led0_status = buf[0] - '0'; >> + } >> + else >> + DBG(2, common, "No LED0 sysfs on %s for %s\n", >> + fs_path, pInfo->name); >> + >> + sprintf(fs_path, "%s/wacom_led/status_led1_select", >> + udev_device_get_syspath(parent)); >> + common->fd_sysfs1 = open(fs_path, O_RDWR); >> + if (common->fd_sysfs1 >= 0) >> + { >> + SYSCALL(err = read(common->fd_sysfs1, buf, 1)); >> + if (err < -1) >> + { >> + xf86Msg(X_WARNING, "%s: failed to get led1 status in " >> + "wcmStoreLEDsysfsInfo.\n", pInfo->name); >> + } >> + else >> + common->led1_status = buf[0] - '0'; >> + } >> + else >> + DBG(2, common, "No LED1 sysfs on %s for %s\n", >> + fs_path, pInfo->name); >> +} >> + >> /* wcmPreInit - called for each input devices with the driver set to >> * "wacom" */ >> #if GET_ABI_MAJOR(ABI_XINPUT_VERSION) < 12 >> @@ -524,9 +584,14 @@ static int wcmPreInit(InputDriverPtr drv, InputInfoPtr >> pInfo, int flags) >> >> /* check if this is the first tool on the port */ >> if (!wcmMatchDevice(pInfo, &common)) >> + { >> /* initialize supported keys with the first tool on the port */ >> wcmDeviceTypeKeys(pInfo); >> >> + /* store lED sysfs file descriptor and initial status */ >> + wcmStoreLEDsysfsInfo(pInfo); >> + } >> + >> common->debugLevel = xf86SetIntOption(pInfo->options, >> "CommonDBG", common->debugLevel); >> oldname = pInfo->name; >> diff --git a/src/wcmXCommand.c b/src/wcmXCommand.c >> index 40393dc..aec3fee 100644 >> --- a/src/wcmXCommand.c >> +++ b/src/wcmXCommand.c >> @@ -1,5 +1,5 @@ >> /* >> - * Copyright 2007-2010 by Ping Cheng, Wacom. <[email protected]> >> + * Copyright 2007-2012 by Ping Cheng, Wacom. <[email protected]> >> * >> * This program is free software; you can redistribute it and/or >> * modify it under the terms of the GNU General Public License >> @@ -91,6 +91,7 @@ Atom prop_tv_resolutions; >> Atom prop_cursorprox; >> Atom prop_threshold; >> Atom prop_suppress; >> +Atom prop_led; >> Atom prop_touch; >> Atom prop_gesture; >> Atom prop_gesture_param; >> @@ -206,6 +207,10 @@ void InitWcmDeviceProperties(InputInfoPtr pInfo) >> values[1] = common->wcmRawSample; >> prop_suppress = InitWcmAtom(pInfo->dev, WACOM_PROP_SAMPLE, XA_INTEGER, >> 32, 2, values); >> >> + values[0] = common->led0_status; >> + values[1] = common->led1_status; >> + prop_led = InitWcmAtom(pInfo->dev, WACOM_PROP_LED, XA_INTEGER, 8, 2, >> values); >> + >> values[0] = common->wcmTouch; >> prop_touch = InitWcmAtom(pInfo->dev, WACOM_PROP_TOUCH, XA_INTEGER, 8, >> 1, values); >> >> @@ -695,6 +700,38 @@ int wcmSetProperty(DeviceIntPtr dev, Atom property, >> XIPropertyValuePtr prop, >> common->wcmSuppress = values[0]; >> common->wcmRawSample = values[1]; >> } >> + } else if (property == prop_led) >> + { >> + CARD8 *values; >> + >> + if (prop->size != 2 || prop->format != 8) >> + return BadValue; >> + >> + values = (CARD8*)prop->data; >> + >> + if ((values[0] < 0) || (values[0] > 3)) >> + return BadValue; >> + else if ((common->fd_sysfs0 >= 0) && !checkonly) >> + { >> + char buf[10]; >> + int err = -1; >> + sprintf(buf, "%d", values[0]); >> + err = xf86WriteSerial(common->fd_sysfs0, buf, >> strlen(buf)); >> + if (err == strlen(buf)) >> + common->led0_status = values[0]; >> + } >> + >> + if ((values[1] < 0) || (values[1] > 3)) >> + return BadValue; >> + else if ((common->fd_sysfs1 >= 0) && !checkonly) >> + { >> + char buf[10]; >> + int err = -1; >> + sprintf(buf, "%d", values[1]); >> + err = xf86WriteSerial(common->fd_sysfs1, buf, >> strlen(buf)); >> + if (err == strlen(buf)) >> + common->led1_status = values[1]; > > deduplicate, helper function please. > >> + } >> } else if (property == prop_rotation) >> { >> CARD8 value; >> diff --git a/src/xf86Wacom.c b/src/xf86Wacom.c >> index d1c149f..de7135c 100644 >> --- a/src/xf86Wacom.c >> +++ b/src/xf86Wacom.c >> @@ -1,6 +1,6 @@ >> /* >> * Copyright 1995-2002 by Frederic Lepied, France. <[email protected]> >> - * Copyright 2002-2010 by Ping Cheng, Wacom. <[email protected]> >> + * Copyright 2002-2012 by Ping Cheng, Wacom. <[email protected]> >> * >> * This program is free software; you can redistribute it and/or >> * modify it under the terms of the GNU General Public License >> @@ -770,6 +770,18 @@ static void wcmDevClose(InputInfoPtr pInfo) >> xf86CloseSerial (common->fd); >> } >> } >> + >> + if (common->fd_sysfs0 >= 0) >> + { >> + xf86CloseSerial(common->fd_sysfs0); >> + common->fd_sysfs0 = -1; >> + } >> + >> + if (common->fd_sysfs1 >= 0) >> + { >> + xf86CloseSerial(common->fd_sysfs1); >> + common->fd_sysfs1 = -1; >> + } > > deduplicate, helper function please. > > >> } >> >> static void wcmEnableDisableTool(DeviceIntPtr dev, Bool enable) >> diff --git a/src/xf86WacomDefs.h b/src/xf86WacomDefs.h >> index 2f3f7b4..22dea41 100644 >> --- a/src/xf86WacomDefs.h >> +++ b/src/xf86WacomDefs.h >> @@ -1,6 +1,6 @@ >> /* >> * Copyright 1995-2002 by Frederic Lepied, France. <[email protected]> >> - * Copyright 2002-2010 by Ping Cheng, Wacom. <[email protected]> >> + * Copyright 2002-2012 by Ping Cheng, Wacom. <[email protected]> >> * >> * This program is free software; you can redistribute it and/or >> * modify it under the terms of the GNU General Public License >> @@ -420,6 +420,10 @@ struct _WacomCommonRec >> int tablet_type; /* bitmask of tablet features (WCM_LCD, >> WCM_PEN, etc) */ >> int fd; /* file descriptor to tablet */ >> int fd_refs; /* number of references to fd; if =0, fd >> is invalid */ >> + int fd_sysfs0; /* file descriptor to sysfs led0 */ >> + int fd_sysfs1; /* file descriptor to sysfs led1 */ >> + unsigned char led0_status; /* Right LED status */ >> + unsigned char led1_status; /* Left LED status */ > > arrays for both please. > >> unsigned long wcmKeys[NBITS(KEY_MAX)]; /* supported tool types for the >> device */ >> WacomDevicePtr wcmTouchDevice; /* The pointer for pen to access the >> touch tool of the same device id */ >> diff --git a/tools/xsetwacom.c b/tools/xsetwacom.c >> index 9ab285b..fa22352 100644 >> --- a/tools/xsetwacom.c >> +++ b/tools/xsetwacom.c >> @@ -361,6 +361,22 @@ static param_t parameters[] = >> .get_func = get_map, >> }, >> { >> + .name = "LED0", >> + .desc = "Sets right LED status ", > > do all tablets (now and in the future) have 2 LEDs and is it always on the > right? If so, this should simply state LED 1, 2 etc, not the physical > location on one tablet. > As mentioned above, LEDs in banks. Some only have one bank (e.g. Intuos4), while others have two (e.g. 21UX2, 24HD). I don't see any problem with removing references to the physical location of the bank (and, actually, I think Ping may have got it backwards in this case). > Cheers, > Peter > >> + .prop_name = WACOM_PROP_LED, >> + .prop_format = 8, >> + .prop_offset = 0, >> + .arg_count = 1, >> + }, >> + { >> + .name = "LED1", >> + .desc = "Sets left LED status ", >> + .prop_name = WACOM_PROP_LED, >> + .prop_format = 8, >> + .prop_offset = 1, >> + .arg_count = 1, >> + }, >> + { >> .name = "Threshold", >> .desc = "Sets tip/eraser pressure threshold " >> "(default is 27). ", >> @@ -2689,7 +2705,7 @@ static void test_parameter_number(void) >> * deprecated them. >> * Numbers include trailing NULL entry. >> */ >> - assert(ARRAY_SIZE(parameters) == 36); >> + assert(ARRAY_SIZE(parameters) == 38); >> assert(ARRAY_SIZE(deprecated_parameters) == 17); >> } >> >> -- >> 1.7.6.4 > > ------------------------------------------------------------------------------ > Keep Your Developer Skills Current with LearnDevNow! > The most comprehensive online learning library for Microsoft developers > is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3, > Metro Style Apps, more. Free future releases when you subscribe now! > http://p.sf.net/sfu/learndevnow-d2d > _______________________________________________ > Linuxwacom-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel Jason --- Day xee-nee-svsh duu-'ushtlh-ts'it; nuu-wee-ya' duu-xan' 'vm-nvshtlh-ts'it. Huu-chan xuu naa~-gha. ------------------------------------------------------------------------------ Keep Your Developer Skills Current with LearnDevNow! The most comprehensive online learning library for Microsoft developers is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3, Metro Style Apps, more. Free future releases when you subscribe now! http://p.sf.net/sfu/learndevnow-d2d _______________________________________________ Linuxwacom-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
