Re: [PATCH 1/2]input - wacom_w8001: Support pen or touch only devices

[Ping Cheng <pinglinux@gmail.com>]

On Wed, Dec 8, 2010 at 10:42 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Ping,
>
> On Wed, Dec 08, 2010 at 05:17:13PM -0800, Ping Cheng wrote:
>> Not all penabled devices support touch. The same idea is true for
>> touch devices. Setting up the devices according to the specific
>> querying results.
>>
>> Signed-off-by: Ping Cheng <pingc@wacom.com>
>> ---
>>  drivers/input/touchscreen/wacom_w8001.c |   39 ++++++++++++++++++++++++------
>>  1 files changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
>> index 361d00a..90b92e8 100644
>> --- a/drivers/input/touchscreen/wacom_w8001.c
>> +++ b/drivers/input/touchscreen/wacom_w8001.c
>> @@ -37,6 +37,7 @@ MODULE_LICENSE("GPL");
>>
>>  #define W8001_QUERY_PACKET   0x20
>>
>> +#define W8001_CMD_STOP               '0'
>>  #define W8001_CMD_START              '1'
>>  #define W8001_CMD_QUERY              '*'
>>  #define W8001_CMD_TOUCHQUERY '%'
>> @@ -279,24 +280,46 @@ static int w8001_setup(struct w8001 *w8001)
>>       struct w8001_coord coord;
>>       int error;
>>
>> -     error = w8001_command(w8001, W8001_CMD_QUERY, true);
>> +     error = w8001_command(w8001, W8001_CMD_STOP, false);
>
> Isn't there any response from the device to W8001_CMD_STOP?

No, there is no response. We just tell the device to set ready for
QUERY command.

>>       if (error)
>>               return error;
>>
>> -     parse_data(w8001->response, &coord);
>> +     mdelay(250);    /* wait 250ms before querying the device */
>>
>> -     input_set_abs_params(dev, ABS_X, 0, coord.x, 0, 0);
>> -     input_set_abs_params(dev, ABS_Y, 0, coord.y, 0, 0);
>> -     input_set_abs_params(dev, ABS_PRESSURE, 0, coord.pen_pressure, 0, 0);
>> -     input_set_abs_params(dev, ABS_TILT_X, 0, coord.tilt_x, 0, 0);
>> -     input_set_abs_params(dev, ABS_TILT_Y, 0, coord.tilt_y, 0, 0);
>> +     /* penabled? */
>> +     error = w8001_command(w8001, W8001_CMD_QUERY, true);
>> +     if (!error) {
>> +             dev->keybit[BIT_WORD(BTN_TOOL_PEN)] |= BIT_MASK(BTN_TOOL_PEN);
>> +             dev->keybit[BIT_WORD(BTN_TOOL_RUBBER)] |= BIT_MASK(BTN_TOOL_RUBBER);
>> +             dev->keybit[BIT_WORD(BTN_STYLUS)] |= BIT_MASK(BTN_STYLUS);
>> +             dev->keybit[BIT_WORD(BTN_STYLUS2)] |= BIT_MASK(BTN_STYLUS2);
>
> Could we start switching to __set_bit(BTN_TOOL_PEN, dev->keybit) style?

Sure, I'll make a new patch. I was following the existing code style
for consistency ......

> The original way was somewhat OK when setting several buttons at once
> but I feel __set_bit() form is much safer.
>
>> +             parse_data(w8001->response, &coord);
>> +
>> +             input_set_abs_params(dev, ABS_X, 0, coord.x, 0, 0);
>> +             input_set_abs_params(dev, ABS_Y, 0, coord.y, 0, 0);
>> +             input_set_abs_params(dev, ABS_PRESSURE, 0, coord.pen_pressure, 0, 0);
>> +             if (coord.tilt_x && coord.tilt_y)
>> +             {
>
> Formatting: opening brace goes on the same as the statement.

Oops. I thought I've taken care of this already.

Thanks.

Ping

>> +                     input_set_abs_params(dev, ABS_TILT_X, 0, coord.tilt_x, 0, 0);
>> +                     input_set_abs_params(dev, ABS_TILT_Y, 0, coord.tilt_y, 0, 0);
>> +             }
>> +     }
>>
>> +     /* touch enabled? */
>>       error = w8001_command(w8001, W8001_CMD_TOUCHQUERY, true);
>> -     if (!error) {
>> +
>> +     /* Some non-touch devices may reply to the touch query. But their
>> +      * second byte is empty, which indicates touch is not supported.
>> +      */
>> +     if (!error && w8001->response[1]) {
>>               struct w8001_touch_query touch;
>>
>>               parse_touchquery(w8001->response, &touch);
>>
>> +             input_set_abs_params(dev, ABS_X, 0, touch.x, 0, 0);
>> +             input_set_abs_params(dev, ABS_Y, 0, touch.y, 0, 0);
>> +             dev->keybit[BIT_WORD(BTN_TOOL_FINGER)] |= BIT_MASK(BTN_TOOL_FINGER);
>> +
>>               switch (touch.sensor_id) {
>>               case 0:
>>               case 2:
>> --
>> 1.7.2.3
>>
>
> Thanks.
>
> --
> Dmitry
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html