Re: Re: [PATCH] Input: hanwang - add support for Art Master II tablet

["weixing" <weixing@hanwang.com.cn>]

Hi Dmitry Torokhov,

On 2012-06-13 14:38:19,you wrote:
>
>Hi,
>
>On Mon, Jun 11, 2012 at 10:10:39PM +0800, weixing wrote:
>> this adds support for old hanwang Art master II tablet
>> 
>> Signed-off-by: weixing <weixing@hanwang.com.cn>
>> ---
>>  drivers/input/tablet/hanwang.c |   63 ++++++++++++++++++++++++++++++++++++++-
>>  1 files changed, 61 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/input/tablet/hanwang.c b/drivers/input/tablet/hanwang.c
>> index b2db3cf..4154c1d 100644
>> --- a/drivers/input/tablet/hanwang.c
>> +++ b/drivers/input/tablet/hanwang.c
>> @@ -63,6 +63,7 @@ MODULE_LICENSE(DRIVER_LICENSE);
>>  enum hanwang_tablet_type {
>>  	HANWANG_ART_MASTER_III,
>>  	HANWANG_ART_MASTER_HD,
>> +	HANWANG_ART_MASTER_II,
>>  };
>>  
>>  struct hanwang {
>> @@ -99,6 +100,8 @@ static const struct hanwang_features features_array[] = {
>>  	  ART_MASTER_PKGLEN_MAX, 0x7f00, 0x4f60, 0x3f, 0x7f, 2048 },
>>  	{ 0x8401, "Hanwang Art Master HD 5012", HANWANG_ART_MASTER_HD,
>>  	  ART_MASTER_PKGLEN_MAX, 0x678e, 0x4150, 0x3f, 0x7f, 1024 },
>> +	{ 0x8503, "Hanwang Art Master II", HANWANG_ART_MASTER_II,
>> +	  ART_MASTER_PKGLEN_MAX, 0x27de, 0x1cfe, 0x3f, 0x7f, 1024 },
>>  };
>>  
>>  static const int hw_eventtypes[] = {
>> @@ -120,6 +123,55 @@ static const int hw_mscevents[] = {
>>  	MSC_SERIAL,
>>  };
>>  
>> +static void hanwang_parse_ArtmasterII_packet(struct hanwang *hanwang)
>
>Caps are disliked. Why don't you call it
>hanwang_parse_artmaster2_packet()?
>

Ok,I'll fix it.

>> +{
>> +	unsigned char *data = hanwang->data;
>> +	struct input_dev *input_dev = hanwang->dev;
>> +	struct usb_device *dev = hanwang->usbdev;
>> +	u16 x, y, p;
>> +
>> +	hanwang->current_tool = BTN_TOOL_PEN;
>> +
>i> +	switch (data[0]) {
>> +	case 0x02:	/* correct report id */
>> +		switch (data[1]) {
>> +		case 0x00:	/* pen leave */
>> +			hanwang->current_id = 0;
>> +			input_report_key(input_dev, hanwang->current_tool, 0);
>> +			break;
>> +		default:
>> +			hanwang->current_id = STYLUS_DEVICE_ID;
>> +			x = (data[2] << 8) | data[3];
>> +			y = (data[4] << 8) | data[5];
>> +			p = (data[7] >> 6) | (data[6] << 2);
>> +
>> +			input_report_key(input_dev, BTN_TOOL_PEN, 1);
>> +			input_report_abs(input_dev, ABS_X,
>> +						le16_to_cpup((__le16 *)&x));
>> +			input_report_abs(input_dev, ABS_Y,
>> +						le16_to_cpup((__le16 *)&y));
>> +			input_report_abs(input_dev, ABS_PRESSURE,
>> +						le16_to_cpup((__le16 *)&p));
>
>This does not make any sense. You first manually conver BE data to CPU
>format and then you do "le16_to_cpup((__le16 *)&x" for no reason...
>
>It looks what you need is:
>
>			input_report_abs(input_dev, ABS_X,
>					 be16_to_cpup((__be16 *)&data[2]);
>			input_report_abs(input_dev, ABS_Y,
>					 be16_to_cpup((__be16 *)&data[3]);
>			input_report_abs(input_dev, ABS_PRESSURE,
>					 (data[7] >> 6) | (data[6] << 2));
>
>I see that the original driver has the same issue.. My bad, we need to
>fix it as well.
>

Sorry,I'll fix it.A little formalism here,my bad.Really appreciate ur advice and comment. 

>> +			input_report_abs(input_dev, ABS_TILT_X, data[7] & 0x3f);
>> +			input_report_abs(input_dev, ABS_TILT_Y, data[8] & 0x7f);
>> +			input_report_key(input_dev, BTN_STYLUS2,
>> +						data[1] & 0x02);
>> +			break;
>> +		}
>> +
>> +		input_report_abs(input_dev, ABS_MISC, hanwang->current_id);
>> +		input_event(input_dev, EV_MSC, MSC_SERIAL,
>> +				hanwang->features->pid);
>
>Hmm, so the difference between ArtmasterII and others is that you it
>does not have BTN_STYLUS, only BTN_STYLUS2 (why, BTW? I'd expect if a
>device had only one button on a stylus it woudl report BTN_STYLUS, not
>BTN_STYLUS2); and it sends hanwang->features->pid instead of 0xffffffff
>in MSC_SERIAL. Does it have to be a separate fucntion that is 99% the
>same as the original one?
>
ok, i'll fix that.

>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
>



Thanks.