Re: [PATCH v2 00/30] HID: wacom: cleanup/EKR/LED

[Ping Cheng]

Hi Jiri,

This patchset has gone through two rounds of testing/review. It is
also a necessary set to support future userland LED configuration
features.

Do you see any issues with the patches?

Cheers,

Ping


On Wed, Jul 13, 2016 at 2:36 PM, Aaron Armstrong Skomra
 wrote:
> On Wed, Jul 13, 2016 at 9:05 AM, Benjamin Tissoires
>  wrote:
>> Hi,
>>
>> So this is a v2 of my summer cleanup of the wacom driver.
>> I fixed the remarks from everybody I think, and it should be in a better 
>> shape
>> now.
>> I removed the patch that changed the LED banks ordering as libwacom exports 
>> it
>> that way. I also added 3 extra patches for the power_supply to be a little 
>> bit
>> more user friendly in gnome-control-center (well, upowerd).
>>
>> Thanks for double testing on the Cintiq 21UX2 and the 24HD as I could only
>
> Retested on the 21UX2 and the 24HD.
>
> Tested-by Aaron Armstrong Skomra 
>
> Best,
> Aaron
>
>> compare the raw events to what was expected, and nothing is better than 
>> actual
>> testing with real hardware.
>>
>> Cheers,
>> Benjamin
>>
>> Benjamin Tissoires (30):
>>   HID: wacom: actually report the battery level for wireless connected
>>   HID: wacom: store the type in wacom->shared for INTUOSHT and INTUOSHT2
>>   HID: wacom: remove cleanup of wacom->remote_dir from
>> wacom_clean_inputs()
>>   HID: wacom: untie leds from inputs
>>   HID: wacom: use one work queue per task
>>   HID: wacom: switch battery to devres
>>   HID: wacom: switch inputs to devres
>>   HID: wacom: put the managed resources in a group
>>   HID: wacom: convert LEDs to devres
>>   HID: wacom: use devm_kasprintf for allocating the name of the remote
>>   HID: wacom: use devres to allocate driver data
>>   HID: wacom: devres manage the shared data too
>>   HID: wacom: leds: dynamically allocate LED groups
>>   HID: wacom: EKR: add a worker to add/remove resources on
>> addition/removal
>>   HID: wacom: EKR: have the wacom resources dynamically allocated
>>   HID: wacom: rework fail path in probe() and parse_and_register()
>>   HID: wacom: EKR: have proper allocator and destructor
>>   HID: wacom: EKR: use devres groups to manage resources
>>   HID: wacom: EKR: have one array of struct remotes instead of many
>> arrays
>>   HID: wacom: EKR: allocate one input node per remote
>>   HID: wacom: EKR: have one power_supply per remote
>>   HID: wacom: EKR: attach the power_supply on first connection
>>   HID: wacom: leds: use the ledclass instead of custom made sysfs files
>>   HID: wacom: leds: actually release the LEDs on disconnect
>>   HID: wacom: leds: handle the switch of the LEDs directly in the kernel
>>   HID: wacom: leds: make sure Cintiq 21UX2 and 24HD control the right
>> LEDs
>>   HID: wacom: leds: handle Cintiq 24HD leds buttons
>>   HID: wacom: power_supply: mark the type as USB
>>   HID: wacom: power_supply: remove ac information
>>   HID: wacom: power_supply: provide the actual model_name
>>
>>  Documentation/ABI/testing/sysfs-driver-wacom |5 +
>>  drivers/hid/Kconfig  |1 +
>>  drivers/hid/wacom.h  |   96 ++-
>>  drivers/hid/wacom_sys.c  | 1104 
>> ++
>>  drivers/hid/wacom_wac.c  |  254 --
>>  drivers/hid/wacom_wac.h  |   19 +-
>>  6 files changed, 1058 insertions(+), 421 deletions(-)
>>
>> --
>> 2.5.5
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/30] HID: wacom: cleanup/EKR/LED

[Ping Cheng]

Hi Jiri,

This patchset has gone through two rounds of testing/review. It is
also a necessary set to support future userland LED configuration
features.

Do you see any issues with the patches?

Cheers,

Ping


On Wed, Jul 13, 2016 at 2:36 PM, Aaron Armstrong Skomra
 wrote:
> On Wed, Jul 13, 2016 at 9:05 AM, Benjamin Tissoires
>  wrote:
>> Hi,
>>
>> So this is a v2 of my summer cleanup of the wacom driver.
>> I fixed the remarks from everybody I think, and it should be in a better 
>> shape
>> now.
>> I removed the patch that changed the LED banks ordering as libwacom exports 
>> it
>> that way. I also added 3 extra patches for the power_supply to be a little 
>> bit
>> more user friendly in gnome-control-center (well, upowerd).
>>
>> Thanks for double testing on the Cintiq 21UX2 and the 24HD as I could only
>
> Retested on the 21UX2 and the 24HD.
>
> Tested-by Aaron Armstrong Skomra 
>
> Best,
> Aaron
>
>> compare the raw events to what was expected, and nothing is better than 
>> actual
>> testing with real hardware.
>>
>> Cheers,
>> Benjamin
>>
>> Benjamin Tissoires (30):
>>   HID: wacom: actually report the battery level for wireless connected
>>   HID: wacom: store the type in wacom->shared for INTUOSHT and INTUOSHT2
>>   HID: wacom: remove cleanup of wacom->remote_dir from
>> wacom_clean_inputs()
>>   HID: wacom: untie leds from inputs
>>   HID: wacom: use one work queue per task
>>   HID: wacom: switch battery to devres
>>   HID: wacom: switch inputs to devres
>>   HID: wacom: put the managed resources in a group
>>   HID: wacom: convert LEDs to devres
>>   HID: wacom: use devm_kasprintf for allocating the name of the remote
>>   HID: wacom: use devres to allocate driver data
>>   HID: wacom: devres manage the shared data too
>>   HID: wacom: leds: dynamically allocate LED groups
>>   HID: wacom: EKR: add a worker to add/remove resources on
>> addition/removal
>>   HID: wacom: EKR: have the wacom resources dynamically allocated
>>   HID: wacom: rework fail path in probe() and parse_and_register()
>>   HID: wacom: EKR: have proper allocator and destructor
>>   HID: wacom: EKR: use devres groups to manage resources
>>   HID: wacom: EKR: have one array of struct remotes instead of many
>> arrays
>>   HID: wacom: EKR: allocate one input node per remote
>>   HID: wacom: EKR: have one power_supply per remote
>>   HID: wacom: EKR: attach the power_supply on first connection
>>   HID: wacom: leds: use the ledclass instead of custom made sysfs files
>>   HID: wacom: leds: actually release the LEDs on disconnect
>>   HID: wacom: leds: handle the switch of the LEDs directly in the kernel
>>   HID: wacom: leds: make sure Cintiq 21UX2 and 24HD control the right
>> LEDs
>>   HID: wacom: leds: handle Cintiq 24HD leds buttons
>>   HID: wacom: power_supply: mark the type as USB
>>   HID: wacom: power_supply: remove ac information
>>   HID: wacom: power_supply: provide the actual model_name
>>
>>  Documentation/ABI/testing/sysfs-driver-wacom |5 +
>>  drivers/hid/Kconfig  |1 +
>>  drivers/hid/wacom.h  |   96 ++-
>>  drivers/hid/wacom_sys.c  | 1104 
>> ++
>>  drivers/hid/wacom_wac.c  |  254 --
>>  drivers/hid/wacom_wac.h  |   19 +-
>>  6 files changed, 1058 insertions(+), 421 deletions(-)
>>
>> --
>> 2.5.5
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 02/30] HID: wacom: store the type in wacom->shared for INTUOSHT and INTUOSHT2

[Ping Cheng]

On Wed, Jul 13, 2016 at 9:05 AM, Benjamin Tissoires
<benjamin.tissoi...@redhat.com> wrote:
> The type is never set but we check for it in wacom_wireless_irq().

Type was assigned in wacom_wireless_work [1] before we moved code
around. It somehow failed to get into wacom_parse_and_register. The
value was only assigned once on stylus interface. It was unnecessary
to assign it again on touch interface since it is a shared value.
Maybe that was the reason it missed its "flight"?

[1] 
https://git.kernel.org/cgit/linux/kernel/git/dtor/input.git/commit/drivers/hid/wacom_sys.c?id=2546dacd3e0e48c40bbb99caf01455f1ade9bb24

> It looks like this is a big hack from the beginning, so fill in the gap
> only.

Yeah, wireless is a dynamic connection. We can not tell which tablet
model is going to be connected before it is paired. But, the dongle is
always the same. That's the tricky part.

> Untested.

Aaron tested it. So, your code is safe ;).

> Signed-off-by: Benjamin Tissoires <benjamin.tissoi...@redhat.com>

The whole set is

Acked-by: Ping Cheng <pi...@wacom.com>

Thanks for your effort!

Ping

> ---
>
> No changes in v2
> ---
>  drivers/hid/wacom_sys.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 499cc82..9e283aa 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -1720,9 +1720,10 @@ static int wacom_parse_and_register(struct wacom 
> *wacom, bool wireless)
> error = hid_hw_open(hdev);
>
> if ((wacom_wac->features.type == INTUOSHT ||
> -   wacom_wac->features.type == INTUOSHT2) &&
> +wacom_wac->features.type == INTUOSHT2) &&
> (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH)) {
> -   wacom_wac->shared->touch_input = 
> wacom_wac->touch_input;
> +   wacom_wac->shared->type = wacom_wac->features.type;
> +   wacom_wac->shared->touch_input = wacom_wac->touch_input;
> }
>
> return 0;
> --
> 2.5.5
>

Re: [PATCH v2 02/30] HID: wacom: store the type in wacom->shared for INTUOSHT and INTUOSHT2

[Ping Cheng]

On Wed, Jul 13, 2016 at 9:05 AM, Benjamin Tissoires
 wrote:
> The type is never set but we check for it in wacom_wireless_irq().

Type was assigned in wacom_wireless_work [1] before we moved code
around. It somehow failed to get into wacom_parse_and_register. The
value was only assigned once on stylus interface. It was unnecessary
to assign it again on touch interface since it is a shared value.
Maybe that was the reason it missed its "flight"?

[1] 
https://git.kernel.org/cgit/linux/kernel/git/dtor/input.git/commit/drivers/hid/wacom_sys.c?id=2546dacd3e0e48c40bbb99caf01455f1ade9bb24

> It looks like this is a big hack from the beginning, so fill in the gap
> only.

Yeah, wireless is a dynamic connection. We can not tell which tablet
model is going to be connected before it is paired. But, the dongle is
always the same. That's the tricky part.

> Untested.

Aaron tested it. So, your code is safe ;).

> Signed-off-by: Benjamin Tissoires 

The whole set is

Acked-by: Ping Cheng 

Thanks for your effort!

Ping

> ---
>
> No changes in v2
> ---
>  drivers/hid/wacom_sys.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 499cc82..9e283aa 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -1720,9 +1720,10 @@ static int wacom_parse_and_register(struct wacom 
> *wacom, bool wireless)
> error = hid_hw_open(hdev);
>
> if ((wacom_wac->features.type == INTUOSHT ||
> -   wacom_wac->features.type == INTUOSHT2) &&
> +wacom_wac->features.type == INTUOSHT2) &&
> (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH)) {
> -   wacom_wac->shared->touch_input = 
> wacom_wac->touch_input;
> +   wacom_wac->shared->type = wacom_wac->features.type;
> +   wacom_wac->shared->touch_input = wacom_wac->touch_input;
> }
>
> return 0;
> --
> 2.5.5
>

Re: [PATCH] Input: wacom_w8001 - Ignore bogus idx values in interrupt

[Ping Cheng]

On Mon, May 23, 2016 at 9:52 AM, Dmitry Torokhov
<dmitry.torok...@gmail.com> wrote:
> On Sun, May 22, 2016 at 10:21:45PM -0700, Ping Cheng wrote:
>> Hi Chris,
>>
>> On Sun, May 22, 2016 at 6:42 PM, Chris J Arges
>> <christopherar...@gmail.com> wrote:
>> > I've noticed crashes when using my x60t using a coreboot bios. When using
>> > the pen I can produce a crash simply by tapping a few times. This
>> > generates an event which has an idx of 0xc. This in turn crashes the
>> > machine because the array access is greater than W8001_MAX_LENGTH. This
>> > patch checks for bogus values and filters them in order to prevent crashes.
>>
>> Thank you for submitting a patch in addition to reporting the issue.
>>
>> > Signed-off-by: Chris J Arges <christopherar...@gmail.com>
>> > ---
>> >  drivers/input/touchscreen/wacom_w8001.c | 9 +
>> >  1 file changed, 9 insertions(+)
>> >
>> > diff --git a/drivers/input/touchscreen/wacom_w8001.c 
>> > b/drivers/input/touchscreen/wacom_w8001.c
>> > index bab3c6a..c858200 100644
>> > --- a/drivers/input/touchscreen/wacom_w8001.c
>> > +++ b/drivers/input/touchscreen/wacom_w8001.c
>> > @@ -283,6 +283,15 @@ static irqreturn_t w8001_interrupt(struct serio 
>> > *serio,
>> > unsigned char tmp;
>> >
>> > w8001->data[w8001->idx] = data;
>> > +
>> > +   /* ignore bogus idx values */
>> > +   if (w8001->idx >= W8001_MAX_LENGTH) {
>> > +   pr_info("w8001: ignored interrupt: data 0x%02x idx %d\n", 
>> > data,
>> > +   w8001->idx);
>> > +   w8001->idx = 0;
>> > +   return IRQ_HANDLED;
>> > +   }
>> > +
>>
>> I don't have an x60t system to test with. I wonder if your system
>> supports two finger touch or not. We at least have a bug in the code
>> since W8001_MAX_LENGTH should be 13 instead of 11. How come no one had
>> encountered that issue before?
>>
>> I'm going to email a patch to the list. Please test it and let us know
>> your result. Maybe we still need your patch if your device doesn't
>> support two finger touch or the idx=0xc can't be fixed by
>> W8001_MAX_LENGTH=13.
>
> Just so we are clear this version of the patch is buggy as we check the
> index only after [potentially] writing past the array bounds of
> w8001->data[].

Thanks for the heads up. I noticed that last night. Since it breaks
two-finger touch, we won't use it anyway.

My other patch is still necessary though. You'll need to change:

From: wacom <wacom@localhost.localdomain>

to

From: Ping Cheng <pi...@wacom.com>

I made it on a brand new system, which I didn't setup the environment
properly. I can update the patch if that's what you like...

Ping

Re: [PATCH] Input: wacom_w8001 - Ignore bogus idx values in interrupt

[Ping Cheng]

On Mon, May 23, 2016 at 9:52 AM, Dmitry Torokhov
 wrote:
> On Sun, May 22, 2016 at 10:21:45PM -0700, Ping Cheng wrote:
>> Hi Chris,
>>
>> On Sun, May 22, 2016 at 6:42 PM, Chris J Arges
>>  wrote:
>> > I've noticed crashes when using my x60t using a coreboot bios. When using
>> > the pen I can produce a crash simply by tapping a few times. This
>> > generates an event which has an idx of 0xc. This in turn crashes the
>> > machine because the array access is greater than W8001_MAX_LENGTH. This
>> > patch checks for bogus values and filters them in order to prevent crashes.
>>
>> Thank you for submitting a patch in addition to reporting the issue.
>>
>> > Signed-off-by: Chris J Arges 
>> > ---
>> >  drivers/input/touchscreen/wacom_w8001.c | 9 +
>> >  1 file changed, 9 insertions(+)
>> >
>> > diff --git a/drivers/input/touchscreen/wacom_w8001.c 
>> > b/drivers/input/touchscreen/wacom_w8001.c
>> > index bab3c6a..c858200 100644
>> > --- a/drivers/input/touchscreen/wacom_w8001.c
>> > +++ b/drivers/input/touchscreen/wacom_w8001.c
>> > @@ -283,6 +283,15 @@ static irqreturn_t w8001_interrupt(struct serio 
>> > *serio,
>> > unsigned char tmp;
>> >
>> > w8001->data[w8001->idx] = data;
>> > +
>> > +   /* ignore bogus idx values */
>> > +   if (w8001->idx >= W8001_MAX_LENGTH) {
>> > +   pr_info("w8001: ignored interrupt: data 0x%02x idx %d\n", 
>> > data,
>> > +   w8001->idx);
>> > +   w8001->idx = 0;
>> > +   return IRQ_HANDLED;
>> > +   }
>> > +
>>
>> I don't have an x60t system to test with. I wonder if your system
>> supports two finger touch or not. We at least have a bug in the code
>> since W8001_MAX_LENGTH should be 13 instead of 11. How come no one had
>> encountered that issue before?
>>
>> I'm going to email a patch to the list. Please test it and let us know
>> your result. Maybe we still need your patch if your device doesn't
>> support two finger touch or the idx=0xc can't be fixed by
>> W8001_MAX_LENGTH=13.
>
> Just so we are clear this version of the patch is buggy as we check the
> index only after [potentially] writing past the array bounds of
> w8001->data[].

Thanks for the heads up. I noticed that last night. Since it breaks
two-finger touch, we won't use it anyway.

My other patch is still necessary though. You'll need to change:

From: wacom 

to

From: Ping Cheng 

I made it on a brand new system, which I didn't setup the environment
properly. I can update the patch if that's what you like...

Ping

Re: [PATCH] Input: wacom_w8001 - Ignore bogus idx values in interrupt

[Ping Cheng]

Hi Chris,

On Sun, May 22, 2016 at 6:42 PM, Chris J Arges
 wrote:
> I've noticed crashes when using my x60t using a coreboot bios. When using
> the pen I can produce a crash simply by tapping a few times. This
> generates an event which has an idx of 0xc. This in turn crashes the
> machine because the array access is greater than W8001_MAX_LENGTH. This
> patch checks for bogus values and filters them in order to prevent crashes.

Thank you for submitting a patch in addition to reporting the issue.

> Signed-off-by: Chris J Arges 
> ---
>  drivers/input/touchscreen/wacom_w8001.c | 9 +
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/input/touchscreen/wacom_w8001.c 
> b/drivers/input/touchscreen/wacom_w8001.c
> index bab3c6a..c858200 100644
> --- a/drivers/input/touchscreen/wacom_w8001.c
> +++ b/drivers/input/touchscreen/wacom_w8001.c
> @@ -283,6 +283,15 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
> unsigned char tmp;
>
> w8001->data[w8001->idx] = data;
> +
> +   /* ignore bogus idx values */
> +   if (w8001->idx >= W8001_MAX_LENGTH) {
> +   pr_info("w8001: ignored interrupt: data 0x%02x idx %d\n", 
> data,
> +   w8001->idx);
> +   w8001->idx = 0;
> +   return IRQ_HANDLED;
> +   }
> +

I don't have an x60t system to test with. I wonder if your system
supports two finger touch or not. We at least have a bug in the code
since W8001_MAX_LENGTH should be 13 instead of 11. How come no one had
encountered that issue before?

I'm going to email a patch to the list. Please test it and let us know
your result. Maybe we still need your patch if your device doesn't
support two finger touch or the idx=0xc can't be fixed by
W8001_MAX_LENGTH=13.

Thanks,

Ping

> switch (w8001->idx++) {
> case 0:
> if ((data & W8001_LEAD_MASK) != W8001_LEAD_BYTE) {
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Input: wacom_w8001 - Ignore bogus idx values in interrupt

[Ping Cheng]

Hi Chris,

On Sun, May 22, 2016 at 6:42 PM, Chris J Arges
 wrote:
> I've noticed crashes when using my x60t using a coreboot bios. When using
> the pen I can produce a crash simply by tapping a few times. This
> generates an event which has an idx of 0xc. This in turn crashes the
> machine because the array access is greater than W8001_MAX_LENGTH. This
> patch checks for bogus values and filters them in order to prevent crashes.

Thank you for submitting a patch in addition to reporting the issue.

> Signed-off-by: Chris J Arges 
> ---
>  drivers/input/touchscreen/wacom_w8001.c | 9 +
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/input/touchscreen/wacom_w8001.c 
> b/drivers/input/touchscreen/wacom_w8001.c
> index bab3c6a..c858200 100644
> --- a/drivers/input/touchscreen/wacom_w8001.c
> +++ b/drivers/input/touchscreen/wacom_w8001.c
> @@ -283,6 +283,15 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
> unsigned char tmp;
>
> w8001->data[w8001->idx] = data;
> +
> +   /* ignore bogus idx values */
> +   if (w8001->idx >= W8001_MAX_LENGTH) {
> +   pr_info("w8001: ignored interrupt: data 0x%02x idx %d\n", 
> data,
> +   w8001->idx);
> +   w8001->idx = 0;
> +   return IRQ_HANDLED;
> +   }
> +

I don't have an x60t system to test with. I wonder if your system
supports two finger touch or not. We at least have a bug in the code
since W8001_MAX_LENGTH should be 13 instead of 11. How come no one had
encountered that issue before?

I'm going to email a patch to the list. Please test it and let us know
your result. Maybe we still need your patch if your device doesn't
support two finger touch or the idx=0xc can't be fixed by
W8001_MAX_LENGTH=13.

Thanks,

Ping

> switch (w8001->idx++) {
> case 0:
> if ((data & W8001_LEAD_MASK) != W8001_LEAD_BYTE) {
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] HID: wacom: fix Bamboo ONE oops

[Ping Cheng]

On Fri, Mar 25, 2016 at 7:26 AM, Benjamin Tissoires
<benjamin.tissoi...@redhat.com> wrote:
>
> Looks like recent changes in the Wacom driver made the Bamboo ONE crashes.
> The tablet behaves as if it was a regular Bamboo device with pen, touch
> and pad, but there is no physical pad connected to it.
> The weird part is that the pad is still sending events and given that
> there is no input node connected to it, we get  anull pointer exception.
>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1317116
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoi...@redhat.com>

Acked-by: Ping Cheng <pi...@wacom.com>

Thank you Benjamin for the fix.

> ---
> Hi Jiri,

As Benjamin mentioned, both users tested the patch. Since their
testing was on kernel 4.4, we need to backport the patch to 4.4. Do
you need us to make a patch for 4.4 or will you take care of stable as
well?

Thanks,

Ping

>
> This fixes the oopses we saw on the Bamboo ONE.
> The fedora user is fine, and it appears the sourceforge user too, as the
> problems were due to misconfigurations on his end.
>
> Cheers,
> Benjamin
>
>  drivers/hid/wacom_wac.c | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index bd198bb..02c4efe 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -2426,6 +2426,17 @@ void wacom_setup_device_quirks(struct wacom *wacom)
> }
>
> /*
> +* Hack for the Bamboo One:
> +* the device presents a PAD/Touch interface as most Bamboos and even
> +* sends ghosts PAD data on it. However, later, we must disable this
> +* ghost interface, and we can not detect it unless we set it here
> +* to WACOM_DEVICETYPE_PAD or WACOM_DEVICETYPE_TOUCH.
> +*/
> +   if (features->type == BAMBOO_PEN &&
> +   features->pktlen == WACOM_PKGLEN_BBTOUCH3)
> +   features->device_type |= WACOM_DEVICETYPE_PAD;
> +
> +   /*
>  * Raw Wacom-mode pen and touch events both come from interface
>  * 0, whose HID descriptor has an application usage of 0xFF0D
>  * (i.e., WACOM_VENDORDEFINED_PEN). We route pen packets back
> --
> 2.5.0
>

Re: [PATCH] HID: wacom: fix Bamboo ONE oops

[Ping Cheng]

On Fri, Mar 25, 2016 at 7:26 AM, Benjamin Tissoires
 wrote:
>
> Looks like recent changes in the Wacom driver made the Bamboo ONE crashes.
> The tablet behaves as if it was a regular Bamboo device with pen, touch
> and pad, but there is no physical pad connected to it.
> The weird part is that the pad is still sending events and given that
> there is no input node connected to it, we get  anull pointer exception.
>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1317116
>
> Signed-off-by: Benjamin Tissoires 

Acked-by: Ping Cheng 

Thank you Benjamin for the fix.

> ---
> Hi Jiri,

As Benjamin mentioned, both users tested the patch. Since their
testing was on kernel 4.4, we need to backport the patch to 4.4. Do
you need us to make a patch for 4.4 or will you take care of stable as
well?

Thanks,

Ping

>
> This fixes the oopses we saw on the Bamboo ONE.
> The fedora user is fine, and it appears the sourceforge user too, as the
> problems were due to misconfigurations on his end.
>
> Cheers,
> Benjamin
>
>  drivers/hid/wacom_wac.c | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index bd198bb..02c4efe 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -2426,6 +2426,17 @@ void wacom_setup_device_quirks(struct wacom *wacom)
> }
>
> /*
> +* Hack for the Bamboo One:
> +* the device presents a PAD/Touch interface as most Bamboos and even
> +* sends ghosts PAD data on it. However, later, we must disable this
> +* ghost interface, and we can not detect it unless we set it here
> +* to WACOM_DEVICETYPE_PAD or WACOM_DEVICETYPE_TOUCH.
> +*/
> +   if (features->type == BAMBOO_PEN &&
> +   features->pktlen == WACOM_PKGLEN_BBTOUCH3)
> +   features->device_type |= WACOM_DEVICETYPE_PAD;
> +
> +   /*
>  * Raw Wacom-mode pen and touch events both come from interface
>  * 0, whose HID descriptor has an application usage of 0xFF0D
>  * (i.e., WACOM_VENDORDEFINED_PEN). We route pen packets back
> --
> 2.5.0
>

Re: [PATCH v2] hid: usbhid: hid-core: fix recursive deadlock

[Ping Cheng]

Hi Jiri,

On Tue, Dec 1, 2015 at 8:36 AM, Jiri Kosina  wrote:
> On Fri, 20 Nov 2015, Ioan-Adrian Ratiu wrote:
>
>> The critical section protected by usbhid->lock in hid_ctrl() is too
>> big and because of this it causes a recursive deadlock. "Too big" means
>> the case statement and the call to hid_input_report() do not need to be
>> protected by the spinlock (no URB operations are done inside them).
>>
>> The deadlock happens because in certain rare cases drivers try to grab
>> the lock while handling the ctrl irq which grabs the lock before them
>> as described above. For example newer wacom tablets like 056a:033c try
>> to reschedule proximity reads from wacom_intuos_schedule_prox_event()
>> calling hid_hw_request() -> usbhid_request() -> usbhid_submit_report()
>> which tries to grab the usbhid lock already held by hid_ctrl().
>>
>> There are two ways to get out of this deadlock:
>> 1. Make the drivers work "around" the ctrl critical region, in the
>> wacom case for ex. by delaying the scheduling of the proximity read
>> request itself to a workqueue.
>> 2. Shrink the critical region so the usbhid lock protects only the
>> instructions which modify usbhid state, calling hid_input_report()
>> with the spinlock unlocked, allowing the device driver to grab the
>> lock first, finish and then grab the lock afterwards in hid_ctrl().
>>
>> This patch implements the 2nd solution.
>>
>> Signed-off-by: Ioan-Adrian Ratiu 
>
> Applied to for-4.5/core branch. Thanks,

Since wacom_intuos_schedule_prox_event() was introduced on March 5,
2015, the call was first used in kernel 3.19. Shouldn't we back-port
this patch to other stable versions?

Thank you.

Ping
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] hid: usbhid: hid-core: fix recursive deadlock

[Ping Cheng]

Hi Jiri,

On Tue, Dec 1, 2015 at 8:36 AM, Jiri Kosina  wrote:
> On Fri, 20 Nov 2015, Ioan-Adrian Ratiu wrote:
>
>> The critical section protected by usbhid->lock in hid_ctrl() is too
>> big and because of this it causes a recursive deadlock. "Too big" means
>> the case statement and the call to hid_input_report() do not need to be
>> protected by the spinlock (no URB operations are done inside them).
>>
>> The deadlock happens because in certain rare cases drivers try to grab
>> the lock while handling the ctrl irq which grabs the lock before them
>> as described above. For example newer wacom tablets like 056a:033c try
>> to reschedule proximity reads from wacom_intuos_schedule_prox_event()
>> calling hid_hw_request() -> usbhid_request() -> usbhid_submit_report()
>> which tries to grab the usbhid lock already held by hid_ctrl().
>>
>> There are two ways to get out of this deadlock:
>> 1. Make the drivers work "around" the ctrl critical region, in the
>> wacom case for ex. by delaying the scheduling of the proximity read
>> request itself to a workqueue.
>> 2. Shrink the critical region so the usbhid lock protects only the
>> instructions which modify usbhid state, calling hid_input_report()
>> with the spinlock unlocked, allowing the device driver to grab the
>> lock first, finish and then grab the lock afterwards in hid_ctrl().
>>
>> This patch implements the 2nd solution.
>>
>> Signed-off-by: Ioan-Adrian Ratiu 
>
> Applied to for-4.5/core branch. Thanks,

Since wacom_intuos_schedule_prox_event() was introduced on March 5,
2015, the call was first used in kernel 3.19. Shouldn't we back-port
this patch to other stable versions?

Thank you.

Ping
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] HID: wacom: Do not add suffix to name of devices with an unknown type

[Ping Cheng]

On Thu, Apr 30, 2015 at 5:51 PM, Jason Gerecke  wrote:
> The naming logic currently assumes that all devices will be a pen, finger,
> or pad. Though this has historically been the case, the new HID_GENERIC
> catch-all may cause us to probe devices with Wacom's 056A VID which aren't
> any of these types (e.g. the "Cintiq 24HDT Monitor Control"). This patch
> updates the logic so that no suffix will be added to the device name if
> the device type is unknown.
>
> Signed-off-by: Jason Gerecke 

Reviewed-by: Ping Cheng  for the whole set.

Cheers,

Ping

> ---
>  drivers/hid/wacom_sys.c | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 9c57ac0..222baf5 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -1440,12 +1440,15 @@ static void wacom_update_name(struct wacom *wacom)
> snprintf(wacom_wac->pad_name, sizeof(wacom_wac->pad_name),
> "%s Pad", wacom_wac->name);
>
> -   if (features->device_type != BTN_TOOL_FINGER)
> +   if (features->device_type == BTN_TOOL_PEN) {
> strlcat(wacom_wac->name, " Pen", WACOM_NAME_MAX);
> -   else if (features->touch_max)
> -   strlcat(wacom_wac->name, " Finger", WACOM_NAME_MAX);
> -   else
> -   strlcat(wacom_wac->name, " Pad", WACOM_NAME_MAX);
> +   }
> +   else if (features->device_type == BTN_TOOL_FINGER) {
> +   if (features->touch_max)
> +   strlcat(wacom_wac->name, " Finger", WACOM_NAME_MAX);
> +   else
> +   strlcat(wacom_wac->name, " Pad", WACOM_NAME_MAX);
> +   }
>  }
>
>  static int wacom_probe(struct hid_device *hdev,
> --
> 2.3.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] HID: wacom: Do not add suffix to name of devices with an unknown type

[Ping Cheng]

On Thu, Apr 30, 2015 at 5:51 PM, Jason Gerecke killert...@gmail.com wrote:
 The naming logic currently assumes that all devices will be a pen, finger,
 or pad. Though this has historically been the case, the new HID_GENERIC
 catch-all may cause us to probe devices with Wacom's 056A VID which aren't
 any of these types (e.g. the Cintiq 24HDT Monitor Control). This patch
 updates the logic so that no suffix will be added to the device name if
 the device type is unknown.

 Signed-off-by: Jason Gerecke jason.gere...@wacom.com

Reviewed-by: Ping Cheng pi...@wacom.com for the whole set.

Cheers,

Ping

 ---
  drivers/hid/wacom_sys.c | 13 -
  1 file changed, 8 insertions(+), 5 deletions(-)

 diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
 index 9c57ac0..222baf5 100644
 --- a/drivers/hid/wacom_sys.c
 +++ b/drivers/hid/wacom_sys.c
 @@ -1440,12 +1440,15 @@ static void wacom_update_name(struct wacom *wacom)
 snprintf(wacom_wac-pad_name, sizeof(wacom_wac-pad_name),
 %s Pad, wacom_wac-name);

 -   if (features-device_type != BTN_TOOL_FINGER)
 +   if (features-device_type == BTN_TOOL_PEN) {
 strlcat(wacom_wac-name,  Pen, WACOM_NAME_MAX);
 -   else if (features-touch_max)
 -   strlcat(wacom_wac-name,  Finger, WACOM_NAME_MAX);
 -   else
 -   strlcat(wacom_wac-name,  Pad, WACOM_NAME_MAX);
 +   }
 +   else if (features-device_type == BTN_TOOL_FINGER) {
 +   if (features-touch_max)
 +   strlcat(wacom_wac-name,  Finger, WACOM_NAME_MAX);
 +   else
 +   strlcat(wacom_wac-name,  Pad, WACOM_NAME_MAX);
 +   }
  }

  static int wacom_probe(struct hid_device *hdev,
 --
 2.3.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] HID: wacom: check for wacom->shared before following the pointer

[Ping Cheng]

On Thu, Mar 5, 2015 at 2:36 PM, Benjamin Tissoires
 wrote:
> 486b908 (HID: wacom: do not send pen events before touch is up/forced out)
> introduces a kernel oops when plugging a tablet without touch.

Thank you for the catch. You must have tried an Intuos 4 or earlier
model. WACOM_QUIRK_MULTI_INPUT is only set for I5 and later.

> wacom->shared is null for these devices so this leads to a null pointer
> exception.

The root cause is "if (wacom->shared->touch_down)" was added outside
of "if (features->quirks & WACOM_QUIRK_MULTI_INPUT)". A consistent fix
would be:

@@ -555,8 +555,11 @@ static int wacom_intuos_inout(struct wacom_wac *wacom)
   (features->type == CINTIQ && !(data[1] & 0x40)))
return 1;

-   if (features->quirks & WACOM_QUIRK_MULTI_INPUT)
+   if (features->quirks & WACOM_QUIRK_MULTI_INPUT) {
wacom->shared->stylus_in_proximity = true;
+   if (wacom->shared->touch_down)
+   return 1;
+   }

/* in Range while exiting */
if (((data[1] & 0xfe) == 0x20) && wacom->reporting_data) {


Cheers,

Ping

> Change the condition to make it clear that what we need is wacom->shared
> not NULL.
>
> Signed-off-by: Benjamin Tissoires 
> ---
>
> Hi Jiri,
>
> this one should be pulled in 4.0. Or we will have surprised users :)
>
> Cheers,
> Benjamin
>
>  drivers/hid/wacom_wac.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index ff9a7ab..9739a4c 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -564,11 +564,12 @@ static int wacom_intuos_inout(struct wacom_wac *wacom)
>(features->type == CINTIQ && !(data[1] & 0x40)))
> return 1;
>
> -   if (features->quirks & WACOM_QUIRK_MULTI_INPUT)
> +   if (wacom->shared) {
> wacom->shared->stylus_in_proximity = true;
>
> -   if (wacom->shared->touch_down)
> -   return 1;
> +   if (wacom->shared->touch_down)
> +   return 1;
> +   }
>
> /* in Range while exiting */
> if (((data[1] & 0xfe) == 0x20) && wacom->reporting_data) {
> --
> 2.1.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] HID: wacom: check for wacom-shared before following the pointer

[Ping Cheng]

On Thu, Mar 5, 2015 at 2:36 PM, Benjamin Tissoires
benjamin.tissoi...@redhat.com wrote:
 486b908 (HID: wacom: do not send pen events before touch is up/forced out)
 introduces a kernel oops when plugging a tablet without touch.

Thank you for the catch. You must have tried an Intuos 4 or earlier
model. WACOM_QUIRK_MULTI_INPUT is only set for I5 and later.

 wacom-shared is null for these devices so this leads to a null pointer
 exception.

The root cause is if (wacom-shared-touch_down) was added outside
of if (features-quirks  WACOM_QUIRK_MULTI_INPUT). A consistent fix
would be:

@@ -555,8 +555,11 @@ static int wacom_intuos_inout(struct wacom_wac *wacom)
   (features-type == CINTIQ  !(data[1]  0x40)))
return 1;

-   if (features-quirks  WACOM_QUIRK_MULTI_INPUT)
+   if (features-quirks  WACOM_QUIRK_MULTI_INPUT) {
wacom-shared-stylus_in_proximity = true;
+   if (wacom-shared-touch_down)
+   return 1;
+   }

/* in Range while exiting */
if (((data[1]  0xfe) == 0x20)  wacom-reporting_data) {


Cheers,

Ping

 Change the condition to make it clear that what we need is wacom-shared
 not NULL.

 Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com
 ---

 Hi Jiri,

 this one should be pulled in 4.0. Or we will have surprised users :)

 Cheers,
 Benjamin

  drivers/hid/wacom_wac.c | 7 ---
  1 file changed, 4 insertions(+), 3 deletions(-)

 diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
 index ff9a7ab..9739a4c 100644
 --- a/drivers/hid/wacom_wac.c
 +++ b/drivers/hid/wacom_wac.c
 @@ -564,11 +564,12 @@ static int wacom_intuos_inout(struct wacom_wac *wacom)
(features-type == CINTIQ  !(data[1]  0x40)))
 return 1;

 -   if (features-quirks  WACOM_QUIRK_MULTI_INPUT)
 +   if (wacom-shared) {
 wacom-shared-stylus_in_proximity = true;

 -   if (wacom-shared-touch_down)
 -   return 1;
 +   if (wacom-shared-touch_down)
 +   return 1;
 +   }

 /* in Range while exiting */
 if (((data[1]  0xfe) == 0x20)  wacom-reporting_data) {
 --
 2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 2/2] HID: wacom: add full support of the Wacom Bamboo PAD

[Ping Cheng]

On Thu, Feb 26, 2015 at 8:28 AM, Benjamin Tissoires
 wrote:
> The stylus of this device works just fine out of the box.
> The touch is seen by default as a mouse with relative events and some
> gestures.
> The wireless and the wired version have slightly different firmwares, but
> the debug mode 2 on the feature 2 is common to the 2 devices. In this mode,
> all the reports are emitted through the debug interface (pen, raw touch
> and mouse emulation), so we have to re-route manually the events.
>
> We keep the Pen interface as a HID_GENERIC one because it works, and only
> parse the raw touches while discarding the mouse emulation & gestures.
>
> Switching the default in raw mode allows us to have a consistent user
> experience accross all the multitouch touchpads (and enable the touch part
> of the devices).
>
> Note that the buttons of this devices are reported through the touch
> interface. There is no 'Pad' interface. It seemed more natural to have
> the BTN_LEFT and BTN_RIGHT reported with the touch because they are
> placed under the touch interface and it looks like they belong to the
> touch part.
>
> Tested-by: Josep Sanchez Ferreres 
> Signed-off-by: Benjamin Tissoires 

Acked-by: Ping Cheng  for the series.

Cheers,

Ping

> ---
> changes in v3:
> - store touch_down information to not send pen events before the touch has 
> been
>   released
>
> changes in v2:
> - re-route the pen events in the pen HID interface
>
>  drivers/hid/wacom_sys.c |  24 +++
>  drivers/hid/wacom_wac.c | 104 
> 
>  drivers/hid/wacom_wac.h |   5 +++
>  3 files changed, 133 insertions(+)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index b3c2395..957699f 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -406,6 +406,9 @@ static int wacom_query_tablet_data(struct hid_device 
> *hdev,
> else if (features->type == WACOM_27QHDT) {
> return wacom_set_device_mode(hdev, 131, 3, 2);
> }
> +   else if (features->type == BAMBOO_PAD) {
> +   return wacom_set_device_mode(hdev, 2, 2, 2);
> +   }
> } else if (features->device_type == BTN_TOOL_PEN) {
> if (features->type <= BAMBOO_PT && features->type != 
> WIRELESS) {
> return wacom_set_device_mode(hdev, 2, 2, 2);
> @@ -1425,6 +1428,21 @@ static int wacom_probe(struct hid_device *hdev,
> goto fail_allocate_inputs;
> }
>
> +   /*
> +* Bamboo Pad has a generic hid handling for the Pen, and we switch it
> +* into debug mode for the touch part.
> +* We ignore the other interfaces.
> +*/
> +   if (features->type == BAMBOO_PAD) {
> +   if (features->pktlen == WACOM_PKGLEN_PENABLED) {
> +   features->type = HID_GENERIC;
> +   } else if ((features->pktlen != WACOM_PKGLEN_BPAD_TOUCH) &&
> +  (features->pktlen != WACOM_PKGLEN_BPAD_TOUCH_USB)) 
> {
> +   error = -ENODEV;
> +   goto fail_shared_data;
> +   }
> +   }
> +
> /* set the default size in case we do not get them from hid */
> wacom_set_default_phy(features);
>
> @@ -1459,6 +1477,12 @@ static int wacom_probe(struct hid_device *hdev,
> features->y_max = 4096;
> }
>
> +   /*
> +* Same thing for Bamboo PAD
> +*/
> +   if (features->type == BAMBOO_PAD)
> +   features->device_type = BTN_TOOL_FINGER;
> +
> if (hdev->bus == BUS_BLUETOOTH)
> features->quirks |= WACOM_QUIRK_BATTERY;
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index 16e8d28..bbf72f9 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -1826,6 +1826,91 @@ static int wacom_bpt_irq(struct wacom_wac *wacom, 
> size_t len)
> return 0;
>  }
>
> +static void wacom_bamboo_pad_pen_event(struct wacom_wac *wacom,
> +   unsigned char *data)
> +{
> +   unsigned char prefix;
> +
> +   /*
> +* We need to reroute the event from the debug interface to the
> +* pen interface.
> +* We need to add the report ID to the actual pen report, so we
> +* temporary overwrite the first byte to prevent having to 
> kzalloc/kfree
> +* and memcpy the report.
> +*/
> +   prefix = data[0];
> +   data[0] = WACOM_REPORT_BPAD_PEN;
> +

Re: [PATCH v3 2/2] HID: wacom: add full support of the Wacom Bamboo PAD

[Ping Cheng]

On Thu, Feb 26, 2015 at 8:28 AM, Benjamin Tissoires
benjamin.tissoi...@redhat.com wrote:
 The stylus of this device works just fine out of the box.
 The touch is seen by default as a mouse with relative events and some
 gestures.
 The wireless and the wired version have slightly different firmwares, but
 the debug mode 2 on the feature 2 is common to the 2 devices. In this mode,
 all the reports are emitted through the debug interface (pen, raw touch
 and mouse emulation), so we have to re-route manually the events.

 We keep the Pen interface as a HID_GENERIC one because it works, and only
 parse the raw touches while discarding the mouse emulation  gestures.

 Switching the default in raw mode allows us to have a consistent user
 experience accross all the multitouch touchpads (and enable the touch part
 of the devices).

 Note that the buttons of this devices are reported through the touch
 interface. There is no 'Pad' interface. It seemed more natural to have
 the BTN_LEFT and BTN_RIGHT reported with the touch because they are
 placed under the touch interface and it looks like they belong to the
 touch part.

 Tested-by: Josep Sanchez Ferreres josep.sanchez.ferre...@est.fib.upc.edu
 Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com

Acked-by: Ping Cheng pi...@wacom.com for the series.

Cheers,

Ping

 ---
 changes in v3:
 - store touch_down information to not send pen events before the touch has 
 been
   released

 changes in v2:
 - re-route the pen events in the pen HID interface

  drivers/hid/wacom_sys.c |  24 +++
  drivers/hid/wacom_wac.c | 104 
 
  drivers/hid/wacom_wac.h |   5 +++
  3 files changed, 133 insertions(+)

 diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
 index b3c2395..957699f 100644
 --- a/drivers/hid/wacom_sys.c
 +++ b/drivers/hid/wacom_sys.c
 @@ -406,6 +406,9 @@ static int wacom_query_tablet_data(struct hid_device 
 *hdev,
 else if (features-type == WACOM_27QHDT) {
 return wacom_set_device_mode(hdev, 131, 3, 2);
 }
 +   else if (features-type == BAMBOO_PAD) {
 +   return wacom_set_device_mode(hdev, 2, 2, 2);
 +   }
 } else if (features-device_type == BTN_TOOL_PEN) {
 if (features-type = BAMBOO_PT  features-type != 
 WIRELESS) {
 return wacom_set_device_mode(hdev, 2, 2, 2);
 @@ -1425,6 +1428,21 @@ static int wacom_probe(struct hid_device *hdev,
 goto fail_allocate_inputs;
 }

 +   /*
 +* Bamboo Pad has a generic hid handling for the Pen, and we switch it
 +* into debug mode for the touch part.
 +* We ignore the other interfaces.
 +*/
 +   if (features-type == BAMBOO_PAD) {
 +   if (features-pktlen == WACOM_PKGLEN_PENABLED) {
 +   features-type = HID_GENERIC;
 +   } else if ((features-pktlen != WACOM_PKGLEN_BPAD_TOUCH) 
 +  (features-pktlen != WACOM_PKGLEN_BPAD_TOUCH_USB)) 
 {
 +   error = -ENODEV;
 +   goto fail_shared_data;
 +   }
 +   }
 +
 /* set the default size in case we do not get them from hid */
 wacom_set_default_phy(features);

 @@ -1459,6 +1477,12 @@ static int wacom_probe(struct hid_device *hdev,
 features-y_max = 4096;
 }

 +   /*
 +* Same thing for Bamboo PAD
 +*/
 +   if (features-type == BAMBOO_PAD)
 +   features-device_type = BTN_TOOL_FINGER;
 +
 if (hdev-bus == BUS_BLUETOOTH)
 features-quirks |= WACOM_QUIRK_BATTERY;

 diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
 index 16e8d28..bbf72f9 100644
 --- a/drivers/hid/wacom_wac.c
 +++ b/drivers/hid/wacom_wac.c
 @@ -1826,6 +1826,91 @@ static int wacom_bpt_irq(struct wacom_wac *wacom, 
 size_t len)
 return 0;
  }

 +static void wacom_bamboo_pad_pen_event(struct wacom_wac *wacom,
 +   unsigned char *data)
 +{
 +   unsigned char prefix;
 +
 +   /*
 +* We need to reroute the event from the debug interface to the
 +* pen interface.
 +* We need to add the report ID to the actual pen report, so we
 +* temporary overwrite the first byte to prevent having to 
 kzalloc/kfree
 +* and memcpy the report.
 +*/
 +   prefix = data[0];
 +   data[0] = WACOM_REPORT_BPAD_PEN;
 +
 +   /*
 +* actually reroute the event.
 +* No need to check if wacom-shared-pen is valid, hid_input_report()
 +* will check for us.
 +*/
 +   hid_input_report(wacom-shared-pen, HID_INPUT_REPORT, data,
 +WACOM_PKGLEN_PENABLED, 1);
 +
 +   data[0] = prefix;
 +}
 +
 +static int wacom_bamboo_pad_touch_event(struct wacom_wac *wacom,
 +   unsigned char *data

Re: [PATCH v2 2/2] HID: wacom: add full support of the Wacom Bamboo PAD

[Ping Cheng]

On Wed, Feb 25, 2015 at 8:43 AM, Benjamin Tissoires
 wrote:
> The stylus of this device works just fine out of the box.
> The touch is seen by default as a mouse with relative events and some
> gestures.
> The wireless and the wired version have slightly different firmwares, but
> the debug mode 2 on the feature 2 is common to the 2 devices. In this mode,
> all the reports are emitted through the debug interface (pen, raw touch
> and mouse emulation), so we have to re-route manually the events.
>
> We keep the Pen interface as a HID_GENERIC one because it works, and only
> parse the raw touches while discarding the mouse emulation & gestures.
>
> Switching the default in raw mode allows us to have a consistent user
> experience accross all the multitouch touchpads (and enable the touch part
> of the devices).
>
> Note that the buttons of this devices are reported through the touch
> interface. There is no 'Pad' interface. It seemed more natural to have
> the BTN_LEFT and BTN_RIGHT reported with the touch because they are
> placed under the touch interface and it looks like they belong to the
> touch part.
>
> Tested-by: Josep Sanchez Ferreres 
> Signed-off-by: Benjamin Tissoires 
> ---
>  drivers/hid/wacom_sys.c |  24 
>  drivers/hid/wacom_wac.c | 100 
> 
>  drivers/hid/wacom_wac.h |   5 +++
>  3 files changed, 129 insertions(+)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index b3c2395..957699f 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -406,6 +406,9 @@ static int wacom_query_tablet_data(struct hid_device 
> *hdev,
> else if (features->type == WACOM_27QHDT) {
> return wacom_set_device_mode(hdev, 131, 3, 2);
> }
> +   else if (features->type == BAMBOO_PAD) {
> +   return wacom_set_device_mode(hdev, 2, 2, 2);
> +   }
> } else if (features->device_type == BTN_TOOL_PEN) {
> if (features->type <= BAMBOO_PT && features->type != 
> WIRELESS) {
> return wacom_set_device_mode(hdev, 2, 2, 2);
> @@ -1425,6 +1428,21 @@ static int wacom_probe(struct hid_device *hdev,
> goto fail_allocate_inputs;
> }
>
> +   /*
> +* Bamboo Pad has a generic hid handling for the Pen, and we switch it
> +* into debug mode for the touch part.
> +* We ignore the other interfaces.
> +*/
> +   if (features->type == BAMBOO_PAD) {
> +   if (features->pktlen == WACOM_PKGLEN_PENABLED) {
> +   features->type = HID_GENERIC;
> +   } else if ((features->pktlen != WACOM_PKGLEN_BPAD_TOUCH) &&
> +  (features->pktlen != WACOM_PKGLEN_BPAD_TOUCH_USB)) 
> {
> +   error = -ENODEV;
> +   goto fail_shared_data;
> +   }
> +   }
> +
> /* set the default size in case we do not get them from hid */
> wacom_set_default_phy(features);
>
> @@ -1459,6 +1477,12 @@ static int wacom_probe(struct hid_device *hdev,
> features->y_max = 4096;
> }
>
> +   /*
> +* Same thing for Bamboo PAD
> +*/
> +   if (features->type == BAMBOO_PAD)
> +   features->device_type = BTN_TOOL_FINGER;
> +
> if (hdev->bus == BUS_BLUETOOTH)
> features->quirks |= WACOM_QUIRK_BATTERY;
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index 16e8d28..ff693a6 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -1826,6 +1826,87 @@ static int wacom_bpt_irq(struct wacom_wac *wacom, 
> size_t len)
> return 0;
>  }
>
> +static void wacom_bamboo_pad_pen_event(struct wacom_wac *wacom,
> +   unsigned char *data)
> +{
> +   unsigned char prefix;
> +
> +   /*
> +* We need to reroute the event from the debug interface to the
> +* pen interface.
> +* We need to add the report ID to the actual pen report, so we
> +* temporary overwrite the first byte to prevent having to 
> kzalloc/kfree
> +* and memcpy the report.
> +*/
> +   prefix = data[0];
> +   data[0] = WACOM_REPORT_BPAD_PEN;
> +
> +   /*
> +* actually reroute the event.
> +* No need to check if wacom->shared->pen is valid, hid_input_report()
> +* will check for us.
> +*/
> +   hid_input_report(wacom->shared->pen, HID_INPUT_REPORT, data,
> +WACOM_PKGLEN_PENABLED, 1);
> +
> +   data[0] = prefix;
> +}
> +
> +static int wacom_bamboo_pad_touch_event(struct wacom_wac *wacom,
> +   unsigned char *data)
> +{
> +   struct input_dev *input = wacom->input;
> +   unsigned char *finger_data, prefix;
> +   unsigned id;
> +   int x, y;
> +   bool valid;
> +
> +   prefix = data[0];
> +
> +   

Re: [PATCH v2 2/2] HID: wacom: add full support of the Wacom Bamboo PAD

[Ping Cheng]

On Wed, Feb 25, 2015 at 8:43 AM, Benjamin Tissoires
benjamin.tissoi...@redhat.com wrote:
 The stylus of this device works just fine out of the box.
 The touch is seen by default as a mouse with relative events and some
 gestures.
 The wireless and the wired version have slightly different firmwares, but
 the debug mode 2 on the feature 2 is common to the 2 devices. In this mode,
 all the reports are emitted through the debug interface (pen, raw touch
 and mouse emulation), so we have to re-route manually the events.

 We keep the Pen interface as a HID_GENERIC one because it works, and only
 parse the raw touches while discarding the mouse emulation  gestures.

 Switching the default in raw mode allows us to have a consistent user
 experience accross all the multitouch touchpads (and enable the touch part
 of the devices).

 Note that the buttons of this devices are reported through the touch
 interface. There is no 'Pad' interface. It seemed more natural to have
 the BTN_LEFT and BTN_RIGHT reported with the touch because they are
 placed under the touch interface and it looks like they belong to the
 touch part.

 Tested-by: Josep Sanchez Ferreres josep.sanchez.ferre...@est.fib.upc.edu
 Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com
 ---
  drivers/hid/wacom_sys.c |  24 
  drivers/hid/wacom_wac.c | 100 
 
  drivers/hid/wacom_wac.h |   5 +++
  3 files changed, 129 insertions(+)

 diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
 index b3c2395..957699f 100644
 --- a/drivers/hid/wacom_sys.c
 +++ b/drivers/hid/wacom_sys.c
 @@ -406,6 +406,9 @@ static int wacom_query_tablet_data(struct hid_device 
 *hdev,
 else if (features-type == WACOM_27QHDT) {
 return wacom_set_device_mode(hdev, 131, 3, 2);
 }
 +   else if (features-type == BAMBOO_PAD) {
 +   return wacom_set_device_mode(hdev, 2, 2, 2);
 +   }
 } else if (features-device_type == BTN_TOOL_PEN) {
 if (features-type = BAMBOO_PT  features-type != 
 WIRELESS) {
 return wacom_set_device_mode(hdev, 2, 2, 2);
 @@ -1425,6 +1428,21 @@ static int wacom_probe(struct hid_device *hdev,
 goto fail_allocate_inputs;
 }

 +   /*
 +* Bamboo Pad has a generic hid handling for the Pen, and we switch it
 +* into debug mode for the touch part.
 +* We ignore the other interfaces.
 +*/
 +   if (features-type == BAMBOO_PAD) {
 +   if (features-pktlen == WACOM_PKGLEN_PENABLED) {
 +   features-type = HID_GENERIC;
 +   } else if ((features-pktlen != WACOM_PKGLEN_BPAD_TOUCH) 
 +  (features-pktlen != WACOM_PKGLEN_BPAD_TOUCH_USB)) 
 {
 +   error = -ENODEV;
 +   goto fail_shared_data;
 +   }
 +   }
 +
 /* set the default size in case we do not get them from hid */
 wacom_set_default_phy(features);

 @@ -1459,6 +1477,12 @@ static int wacom_probe(struct hid_device *hdev,
 features-y_max = 4096;
 }

 +   /*
 +* Same thing for Bamboo PAD
 +*/
 +   if (features-type == BAMBOO_PAD)
 +   features-device_type = BTN_TOOL_FINGER;
 +
 if (hdev-bus == BUS_BLUETOOTH)
 features-quirks |= WACOM_QUIRK_BATTERY;

 diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
 index 16e8d28..ff693a6 100644
 --- a/drivers/hid/wacom_wac.c
 +++ b/drivers/hid/wacom_wac.c
 @@ -1826,6 +1826,87 @@ static int wacom_bpt_irq(struct wacom_wac *wacom, 
 size_t len)
 return 0;
  }

 +static void wacom_bamboo_pad_pen_event(struct wacom_wac *wacom,
 +   unsigned char *data)
 +{
 +   unsigned char prefix;
 +
 +   /*
 +* We need to reroute the event from the debug interface to the
 +* pen interface.
 +* We need to add the report ID to the actual pen report, so we
 +* temporary overwrite the first byte to prevent having to 
 kzalloc/kfree
 +* and memcpy the report.
 +*/
 +   prefix = data[0];
 +   data[0] = WACOM_REPORT_BPAD_PEN;
 +
 +   /*
 +* actually reroute the event.
 +* No need to check if wacom-shared-pen is valid, hid_input_report()
 +* will check for us.
 +*/
 +   hid_input_report(wacom-shared-pen, HID_INPUT_REPORT, data,
 +WACOM_PKGLEN_PENABLED, 1);
 +
 +   data[0] = prefix;
 +}
 +
 +static int wacom_bamboo_pad_touch_event(struct wacom_wac *wacom,
 +   unsigned char *data)
 +{
 +   struct input_dev *input = wacom-input;
 +   unsigned char *finger_data, prefix;
 +   unsigned id;
 +   int x, y;
 +   bool valid;
 +
 +   prefix = data[0];
 +
 +   for (id = 0; id  wacom-features.touch_max; id++) {
 +

Re: [PATCH] HID: wacom: do not directly use input_mt_report_pointer_emulation

[Ping Cheng]

On Tue, Feb 17, 2015 at 11:19 AM, Benjamin Tissoires
 wrote:
> input_mt_sync_frame() calls input_mt_report_pointer_emulation() and do
> some internal steps required to keep in sync the state of the touch within
> the various reports.
>
> Given that we use input_mt_assign_slot() in this driver, it is better to
> use input_mt_sync_frame().

It is sensible to me, except I think you meant input_mt_slot() instead
of input_mt_assign_slot().

> Signed-off-by: Benjamin Tissoires 

Reviewed-by: Ping Cheng 

Ping

> ---
>  drivers/hid/wacom_wac.c | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index 8627581..de93968 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -1093,7 +1093,7 @@ static int wacom_24hdt_irq(struct wacom_wac *wacom)
> }
> }
> }
> -   input_mt_report_pointer_emulation(input, true);
> +   input_mt_sync_frame(input);
>
> wacom->num_contacts_left -= contacts_to_send;
> if (wacom->num_contacts_left <= 0)
> @@ -1144,7 +1144,7 @@ static int wacom_mt_touch(struct wacom_wac *wacom)
> input_report_abs(input, ABS_MT_POSITION_Y, y);
> }
> }
> -   input_mt_report_pointer_emulation(input, true);
> +   input_mt_sync_frame(input);
>
> wacom->num_contacts_left -= contacts_to_send;
> if (wacom->num_contacts_left < 0)
> @@ -1176,7 +1176,7 @@ static int wacom_tpc_mt_touch(struct wacom_wac *wacom)
> contact_with_no_pen_down_count++;
> }
> }
> -   input_mt_report_pointer_emulation(input, true);
> +   input_mt_sync_frame(input);
>
> /* keep touch state for pen event */
> wacom->shared->touch_down = (contact_with_no_pen_down_count > 0);
> @@ -1638,7 +1638,7 @@ static int wacom_bpt_touch(struct wacom_wac *wacom)
> }
> }
>
> -   input_mt_report_pointer_emulation(input, true);
> +   input_mt_sync_frame(input);
>
> input_report_key(pad_input, BTN_LEFT, (data[1] & 0x08) != 0);
> input_report_key(pad_input, BTN_FORWARD, (data[1] & 0x04) != 0);
> @@ -1728,7 +1728,7 @@ static int wacom_bpt3_touch(struct wacom_wac *wacom)
> wacom_bpt3_button_msg(wacom, data + offset);
>
> }
> -   input_mt_report_pointer_emulation(input, true);
> +   input_mt_sync_frame(input);
>
> return 1;
>  }
> --
> 2.1.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] HID: wacom: do not directly use input_mt_report_pointer_emulation

[Ping Cheng]

On Tue, Feb 17, 2015 at 11:19 AM, Benjamin Tissoires
benjamin.tissoi...@redhat.com wrote:
 input_mt_sync_frame() calls input_mt_report_pointer_emulation() and do
 some internal steps required to keep in sync the state of the touch within
 the various reports.

 Given that we use input_mt_assign_slot() in this driver, it is better to
 use input_mt_sync_frame().

It is sensible to me, except I think you meant input_mt_slot() instead
of input_mt_assign_slot().

 Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com

Reviewed-by: Ping Cheng pi...@wacom.com

Ping

 ---
  drivers/hid/wacom_wac.c | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)

 diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
 index 8627581..de93968 100644
 --- a/drivers/hid/wacom_wac.c
 +++ b/drivers/hid/wacom_wac.c
 @@ -1093,7 +1093,7 @@ static int wacom_24hdt_irq(struct wacom_wac *wacom)
 }
 }
 }
 -   input_mt_report_pointer_emulation(input, true);
 +   input_mt_sync_frame(input);

 wacom-num_contacts_left -= contacts_to_send;
 if (wacom-num_contacts_left = 0)
 @@ -1144,7 +1144,7 @@ static int wacom_mt_touch(struct wacom_wac *wacom)
 input_report_abs(input, ABS_MT_POSITION_Y, y);
 }
 }
 -   input_mt_report_pointer_emulation(input, true);
 +   input_mt_sync_frame(input);

 wacom-num_contacts_left -= contacts_to_send;
 if (wacom-num_contacts_left  0)
 @@ -1176,7 +1176,7 @@ static int wacom_tpc_mt_touch(struct wacom_wac *wacom)
 contact_with_no_pen_down_count++;
 }
 }
 -   input_mt_report_pointer_emulation(input, true);
 +   input_mt_sync_frame(input);

 /* keep touch state for pen event */
 wacom-shared-touch_down = (contact_with_no_pen_down_count  0);
 @@ -1638,7 +1638,7 @@ static int wacom_bpt_touch(struct wacom_wac *wacom)
 }
 }

 -   input_mt_report_pointer_emulation(input, true);
 +   input_mt_sync_frame(input);

 input_report_key(pad_input, BTN_LEFT, (data[1]  0x08) != 0);
 input_report_key(pad_input, BTN_FORWARD, (data[1]  0x04) != 0);
 @@ -1728,7 +1728,7 @@ static int wacom_bpt3_touch(struct wacom_wac *wacom)
 wacom_bpt3_button_msg(wacom, data + offset);

 }
 -   input_mt_report_pointer_emulation(input, true);
 +   input_mt_sync_frame(input);

 return 1;
  }
 --
 2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] hid: Initialize battery_no to -1 & correct its format string

[Ping Cheng]

Hi Benjamin,

The relevant code was introduced by
d70420b914c98a3758674c6e9858810e0ab4ea30. Can you take a look and let
us know if Aniroop's patch fits your original thought or not?

Thanks,

Ping

On Tue, Jan 6, 2015 at 6:32 AM, Aniroop Mathur  wrote:
> Dear Mr. Jason and Mr. Ping,
>
> Please update about below patch.
> Except avoiding subtraction, there is also a need to avoid negative
> battery name which may come due to %ld, so need to change to %lu.
>
> Thanks,
> Aniroop
>
> On Mon, Dec 29, 2014 at 5:33 PM, Jiri Kosina  wrote:
>> On Sun, 28 Dec 2014, Aniroop Mathur wrote:
>>
>>> From: Aniroop Mathur 
>>>
>>> This patch initializes battery_no to -1 to avoid extra subtraction
>>> operation performed everytime wacom battery is initialized
>>
>> This is so femto-optimization, that I don't really care deeply. Adding
>> Jason and Ping to CC. If they want this, I'll apply the patch.
>>
>>> and correct format string of unsigned long from %ld to %lu.
>>>
>>> Signed-off-by: Aniroop Mathur 
>>> ---
>>>  drivers/hid/wacom_sys.c | 8 
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
>>> index 129fd33..4b5ff84 100644
>>> --- a/drivers/hid/wacom_sys.c
>>> +++ b/drivers/hid/wacom_sys.c
>>> @@ -919,17 +919,17 @@ static int wacom_ac_get_property(struct power_supply 
>>> *psy,
>>>
>>>  static int wacom_initialize_battery(struct wacom *wacom)
>>>  {
>>> - static atomic_t battery_no = ATOMIC_INIT(0);
>>> + static atomic_t battery_no = ATOMIC_INIT(-1);
>>>   int error;
>>>   unsigned long n;
>>>
>>>   if (wacom->wacom_wac.features.quirks & WACOM_QUIRK_BATTERY) {
>>> - n = atomic_inc_return(_no) - 1;
>>> + n = atomic_inc_return(_no);
>>>
>>>   wacom->battery.properties = wacom_battery_props;
>>>   wacom->battery.num_properties = 
>>> ARRAY_SIZE(wacom_battery_props);
>>>   wacom->battery.get_property = wacom_battery_get_property;
>>> - sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%ld", n);
>>> + sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%lu", n);
>>>   wacom->battery.name = wacom->wacom_wac.bat_name;
>>>   wacom->battery.type = POWER_SUPPLY_TYPE_BATTERY;
>>>   wacom->battery.use_for_apm = 0;
>>> @@ -937,7 +937,7 @@ static int wacom_initialize_battery(struct wacom *wacom)
>>>   wacom->ac.properties = wacom_ac_props;
>>>   wacom->ac.num_properties = ARRAY_SIZE(wacom_ac_props);
>>>   wacom->ac.get_property = wacom_ac_get_property;
>>> - sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%ld", n);
>>> + sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%lu", n);
>>>   wacom->ac.name = wacom->wacom_wac.ac_name;
>>>   wacom->ac.type = POWER_SUPPLY_TYPE_MAINS;
>>>   wacom->ac.use_for_apm = 0;
>>> --
>>> 1.9.1
>>>
>>
>> --
>> Jiri Kosina
>> SUSE Labs
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] hid: Initialize battery_no to -1 correct its format string

[Ping Cheng]

Hi Benjamin,

The relevant code was introduced by
d70420b914c98a3758674c6e9858810e0ab4ea30. Can you take a look and let
us know if Aniroop's patch fits your original thought or not?

Thanks,

Ping

On Tue, Jan 6, 2015 at 6:32 AM, Aniroop Mathur aniroop.mat...@gmail.com wrote:
 Dear Mr. Jason and Mr. Ping,

 Please update about below patch.
 Except avoiding subtraction, there is also a need to avoid negative
 battery name which may come due to %ld, so need to change to %lu.

 Thanks,
 Aniroop

 On Mon, Dec 29, 2014 at 5:33 PM, Jiri Kosina jkos...@suse.cz wrote:
 On Sun, 28 Dec 2014, Aniroop Mathur wrote:

 From: Aniroop Mathur a.mat...@samsung.com

 This patch initializes battery_no to -1 to avoid extra subtraction
 operation performed everytime wacom battery is initialized

 This is so femto-optimization, that I don't really care deeply. Adding
 Jason and Ping to CC. If they want this, I'll apply the patch.

 and correct format string of unsigned long from %ld to %lu.

 Signed-off-by: Aniroop Mathur a.mat...@samsung.com
 ---
  drivers/hid/wacom_sys.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)

 diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
 index 129fd33..4b5ff84 100644
 --- a/drivers/hid/wacom_sys.c
 +++ b/drivers/hid/wacom_sys.c
 @@ -919,17 +919,17 @@ static int wacom_ac_get_property(struct power_supply 
 *psy,

  static int wacom_initialize_battery(struct wacom *wacom)
  {
 - static atomic_t battery_no = ATOMIC_INIT(0);
 + static atomic_t battery_no = ATOMIC_INIT(-1);
   int error;
   unsigned long n;

   if (wacom-wacom_wac.features.quirks  WACOM_QUIRK_BATTERY) {
 - n = atomic_inc_return(battery_no) - 1;
 + n = atomic_inc_return(battery_no);

   wacom-battery.properties = wacom_battery_props;
   wacom-battery.num_properties = 
 ARRAY_SIZE(wacom_battery_props);
   wacom-battery.get_property = wacom_battery_get_property;
 - sprintf(wacom-wacom_wac.bat_name, wacom_battery_%ld, n);
 + sprintf(wacom-wacom_wac.bat_name, wacom_battery_%lu, n);
   wacom-battery.name = wacom-wacom_wac.bat_name;
   wacom-battery.type = POWER_SUPPLY_TYPE_BATTERY;
   wacom-battery.use_for_apm = 0;
 @@ -937,7 +937,7 @@ static int wacom_initialize_battery(struct wacom *wacom)
   wacom-ac.properties = wacom_ac_props;
   wacom-ac.num_properties = ARRAY_SIZE(wacom_ac_props);
   wacom-ac.get_property = wacom_ac_get_property;
 - sprintf(wacom-wacom_wac.ac_name, wacom_ac_%ld, n);
 + sprintf(wacom-wacom_wac.ac_name, wacom_ac_%lu, n);
   wacom-ac.name = wacom-wacom_wac.ac_name;
   wacom-ac.type = POWER_SUPPLY_TYPE_MAINS;
   wacom-ac.use_for_apm = 0;
 --
 1.9.1


 --
 Jiri Kosina
 SUSE Labs
 --
 To unsubscribe from this list: send the line unsubscribe linux-input in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Fwd: [PATCH] HID: wacom: make the WL connection friendly for the desktop

[Ping Cheng]

On Thu, Sep 11, 2014 at 10:14 AM, Benjamin Tissoires
 wrote:
>
> Currently, tablets connected to the WL receiver all share the same
> VID/PID. There is no way for the user space to know which one is which
> besides parsing the name. We can force the PID to be set to the
> actual hardware. This way, the input device will have the correct PID
> which can be match in libwacom.


Nice job, Benjamin! Thank you, on behalf of wireless tablet users ;-).

>
> With only this trick, the pad input does not inherit the ID_INPUT_TABLET
> udev property from its parent. We can force udev to accept it by declaring
> a BTN_STYLUS which is never used.


BTN_STYLUS can be confusing to userland clients that retrieve it to
decide tool types. But, without it, the client is already confused
with joystick. So, unless we introduce a new tool type, which I
proposed a few years ago and was rejected, we may have to keep clients
confused. With that said, the patch is

>
> This way, tablets connected through WL can be used from the user point of
> view in the same way they are used while connected through wire.
>
> Signed-off-by: Benjamin Tissoires 


Reviewed-by: Ping Cheng 

Cheers,

Ping

> ---
>
> Hi guys,
>
> to continue on Ping's work regarding the Wireless reciever, here is a patch
> which allows to have the same user experience regardless the transport layer.
> Unfortunately, a libwacom patch is also required, but once both are applied,
> there is no differences between the wired and wireless connections. Yeah!
>
> Cheers,
> Benjamin
>
>  drivers/hid/wacom_sys.c | 2 +-
>  drivers/hid/wacom_wac.c | 3 +++
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 9e4e188..a8b7f16 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -1035,7 +1035,7 @@ static struct input_dev *wacom_allocate_input(struct 
> wacom *wacom)
> input_dev->uniq = hdev->uniq;
> input_dev->id.bustype = hdev->bus;
> input_dev->id.vendor  = hdev->vendor;
> -   input_dev->id.product = hdev->product;
> +   input_dev->id.product = wacom_wac->pid ? wacom_wac->pid : 
> hdev->product;
> input_dev->id.version = hdev->version;
> input_set_drvdata(input_dev, wacom);
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index c3cbbfb..b8180e4 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -1990,6 +1990,9 @@ int wacom_setup_pad_input_capabilities(struct input_dev 
> *input_dev,
> input_set_abs_params(input_dev, ABS_X, 0, 1, 0, 0);
> input_set_abs_params(input_dev, ABS_Y, 0, 1, 0, 0);
>
> +   /* kept for making udev and libwacom accepting the pad */
> +   __set_bit(BTN_STYLUS, input_dev->keybit);
> +
> switch (features->type) {
> case GRAPHIRE_BT:
> __set_bit(BTN_0, input_dev->keybit);
> --
> 2.1.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Fwd: [PATCH] HID: wacom: make the WL connection friendly for the desktop

[Ping Cheng]

On Thu, Sep 11, 2014 at 10:14 AM, Benjamin Tissoires
benjamin.tissoi...@redhat.com wrote:

 Currently, tablets connected to the WL receiver all share the same
 VID/PID. There is no way for the user space to know which one is which
 besides parsing the name. We can force the PID to be set to the
 actual hardware. This way, the input device will have the correct PID
 which can be match in libwacom.


Nice job, Benjamin! Thank you, on behalf of wireless tablet users ;-).


 With only this trick, the pad input does not inherit the ID_INPUT_TABLET
 udev property from its parent. We can force udev to accept it by declaring
 a BTN_STYLUS which is never used.


BTN_STYLUS can be confusing to userland clients that retrieve it to
decide tool types. But, without it, the client is already confused
with joystick. So, unless we introduce a new tool type, which I
proposed a few years ago and was rejected, we may have to keep clients
confused. With that said, the patch is


 This way, tablets connected through WL can be used from the user point of
 view in the same way they are used while connected through wire.

 Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com


Reviewed-by: Ping Cheng pi...@wacom.com

Cheers,

Ping

 ---

 Hi guys,

 to continue on Ping's work regarding the Wireless reciever, here is a patch
 which allows to have the same user experience regardless the transport layer.
 Unfortunately, a libwacom patch is also required, but once both are applied,
 there is no differences between the wired and wireless connections. Yeah!

 Cheers,
 Benjamin

  drivers/hid/wacom_sys.c | 2 +-
  drivers/hid/wacom_wac.c | 3 +++
  2 files changed, 4 insertions(+), 1 deletion(-)

 diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
 index 9e4e188..a8b7f16 100644
 --- a/drivers/hid/wacom_sys.c
 +++ b/drivers/hid/wacom_sys.c
 @@ -1035,7 +1035,7 @@ static struct input_dev *wacom_allocate_input(struct 
 wacom *wacom)
 input_dev-uniq = hdev-uniq;
 input_dev-id.bustype = hdev-bus;
 input_dev-id.vendor  = hdev-vendor;
 -   input_dev-id.product = hdev-product;
 +   input_dev-id.product = wacom_wac-pid ? wacom_wac-pid : 
 hdev-product;
 input_dev-id.version = hdev-version;
 input_set_drvdata(input_dev, wacom);

 diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
 index c3cbbfb..b8180e4 100644
 --- a/drivers/hid/wacom_wac.c
 +++ b/drivers/hid/wacom_wac.c
 @@ -1990,6 +1990,9 @@ int wacom_setup_pad_input_capabilities(struct input_dev 
 *input_dev,
 input_set_abs_params(input_dev, ABS_X, 0, 1, 0, 0);
 input_set_abs_params(input_dev, ABS_Y, 0, 1, 0, 0);

 +   /* kept for making udev and libwacom accepting the pad */
 +   __set_bit(BTN_STYLUS, input_dev-keybit);
 +
 switch (features-type) {
 case GRAPHIRE_BT:
 __set_bit(BTN_0, input_dev-keybit);
 --
 2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] Input - wacom: update the ABI doc according to latest changes

[Ping Cheng]

On Thu, Aug 7, 2014 at 1:43 PM, Benjamin Tissoires
 wrote:
> Now the devices show up under hid no matter the connection (for USB
> and Bluetooth, not serial nor i2c).
>
> The USB devices can now be easily found under
> /sys/bus/hid/devices/::.
>
> The Bluetooth devices could also be found under this path since their
> inclusion (April 2010), so this patch fixes the unprecise "hidraw*" path
> for them.
>
> The ABI has been unified while setting the LEDs and OLEDs. So Bluetooth
> devices lost their own LED selector but use the USB sysfs attribute.
> For OLEDs, Bluetooth devices handle only 1-bit images instead of 4 for USB.
> The documentation has been updated to match this.
>
> Signed-off-by: Benjamin Tissoires 

Reviewed-by: Ping Cheng 

Ping

>  Documentation/ABI/testing/sysfs-driver-wacom | 70 
> +++-
>  1 file changed, 27 insertions(+), 43 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-wacom 
> b/Documentation/ABI/testing/sysfs-driver-wacom
> index 7fc7810..c4f0fed 100644
> --- a/Documentation/ABI/testing/sysfs-driver-wacom
> +++ b/Documentation/ABI/testing/sysfs-driver-wacom
> @@ -1,48 +1,27 @@
> -WWhat: /sys/class/hidraw/hidraw*/device/oled*_img
> -Date:  June 2012
> -Contact:   linux-blueto...@vger.kernel.org
> -Description:
> -   The /sys/class/hidraw/hidraw*/device/oled*_img files control
> -   OLED mocro displays on Intuos4 Wireless tablet. Accepted image
> -   has to contain 256 bytes (64x32 px 1 bit colour). The format
> -   is the same as PBM image 62x32px without header (64 bits per
> -   horizontal line, 32 lines). An example of setting OLED No. 0:
> -   dd bs=256 count=1 if=img_file of=[path to oled0_img]/oled0_img
> -   The attribute is read only and no local copy of the image is
> -   stored.
> -
> -What:  /sys/class/hidraw/hidraw*/device/speed
> +What:  /sys/bus/hid/devices/::./speed
>  Date:  April 2010
>  Kernel Version:2.6.35
>  Contact:   linux-blueto...@vger.kernel.org
>  Description:
> -   The /sys/class/hidraw/hidraw*/device/speed file controls
> -   reporting speed of Wacom bluetooth tablet. Reading from
> -   this file returns 1 if tablet reports in high speed mode
> +   The /sys/bus/hid/devices/::./speed file
> +   controls reporting speed of Wacom bluetooth tablet. Reading
> +   from this file returns 1 if tablet reports in high speed mode
> or 0 otherwise. Writing to this file one of these values
> switches reporting speed.
>
> -What:  /sys/class/leds/0005\:056A\:00BD.0001\:selector\:*/
> -Date:  May 2012
> -Kernel Version:3.5
> -Contact:   linux-blueto...@vger.kernel.org
> -Description:
> -   LED selector for Intuos4 WL. There are 4 leds, but only one 
> LED
> -   can be lit at a time. Max brightness is 127.
> -
> -What:  
> /sys/bus/usb/devices/-:./wacom_led/led
> -Date:  August 2011
> +What:  /sys/bus/hid/devices/::./wacom_led/led
> +Date:  August 2014
>  Contact:   linux-in...@vger.kernel.org
>  Description:
> Attribute group for control of the status LEDs and the OLEDs.
> This attribute group is only available for Intuos 4 M, L,
> -   and XL (with LEDs and OLEDs), Intuos 5 (LEDs only), and Cintiq
> -   21UX2 and Cintiq 24HD (LEDs only). Therefore its presence
> -   implicitly signifies the presence of said LEDs and OLEDs on 
> the
> -   tablet device.
> +   and XL (with LEDs and OLEDs), Intuos 4 WL, Intuos 5 (LEDs 
> only),
> +   Intuos Pro (LEDs only) and Cintiq 21UX2 and Cintiq 24HD
> +   (LEDs only). Therefore its presence implicitly signifies the
> +   presence of said LEDs and OLEDs on the tablet device.
>
> -What:  
> /sys/bus/usb/devices/-:./wacom_led/status0_luminance
> -Date:  August 2011
> +What:  
> /sys/bus/hid/devices/::./wacom_led/status0_luminance
> +Date:  August 2014
>  Contact:   linux-in...@vger.kernel.org
>  Description:
> Writing to this file sets the status LED luminance (1..127)
> @@ -50,16 +29,16 @@ Description:
> button is pressed on the stylus. This luminance level is
> normally lower than the level when a button is pressed.
>
> -What:  
> /sys/bus/usb/devices/-:./wacom_led/status1_luminance
> -Date:  August 2011
> +What: 

Re: [PATCH v2] Input - wacom: update the ABI doc according to latest changes

[Ping Cheng]

On Thu, Aug 7, 2014 at 1:43 PM, Benjamin Tissoires
benjamin.tissoi...@redhat.com wrote:
 Now the devices show up under hid no matter the connection (for USB
 and Bluetooth, not serial nor i2c).

 The USB devices can now be easily found under
 /sys/bus/hid/devices/bus:vid:pid.n

 The Bluetooth devices could also be found under this path since their
 inclusion (April 2010), so this patch fixes the unprecise hidraw* path
 for them.

 The ABI has been unified while setting the LEDs and OLEDs. So Bluetooth
 devices lost their own LED selector but use the USB sysfs attribute.
 For OLEDs, Bluetooth devices handle only 1-bit images instead of 4 for USB.
 The documentation has been updated to match this.

 Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com

Reviewed-by: Ping Cheng pi...@wacom.com

Ping

  Documentation/ABI/testing/sysfs-driver-wacom | 70 
 +++-
  1 file changed, 27 insertions(+), 43 deletions(-)

 diff --git a/Documentation/ABI/testing/sysfs-driver-wacom 
 b/Documentation/ABI/testing/sysfs-driver-wacom
 index 7fc7810..c4f0fed 100644
 --- a/Documentation/ABI/testing/sysfs-driver-wacom
 +++ b/Documentation/ABI/testing/sysfs-driver-wacom
 @@ -1,48 +1,27 @@
 -WWhat: /sys/class/hidraw/hidraw*/device/oled*_img
 -Date:  June 2012
 -Contact:   linux-blueto...@vger.kernel.org
 -Description:
 -   The /sys/class/hidraw/hidraw*/device/oled*_img files control
 -   OLED mocro displays on Intuos4 Wireless tablet. Accepted image
 -   has to contain 256 bytes (64x32 px 1 bit colour). The format
 -   is the same as PBM image 62x32px without header (64 bits per
 -   horizontal line, 32 lines). An example of setting OLED No. 0:
 -   dd bs=256 count=1 if=img_file of=[path to oled0_img]/oled0_img
 -   The attribute is read only and no local copy of the image is
 -   stored.
 -
 -What:  /sys/class/hidraw/hidraw*/device/speed
 +What:  /sys/bus/hid/devices/bus:vid:pid.n/speed
  Date:  April 2010
  Kernel Version:2.6.35
  Contact:   linux-blueto...@vger.kernel.org
  Description:
 -   The /sys/class/hidraw/hidraw*/device/speed file controls
 -   reporting speed of Wacom bluetooth tablet. Reading from
 -   this file returns 1 if tablet reports in high speed mode
 +   The /sys/bus/hid/devices/bus:vid:pid.n/speed file
 +   controls reporting speed of Wacom bluetooth tablet. Reading
 +   from this file returns 1 if tablet reports in high speed mode
 or 0 otherwise. Writing to this file one of these values
 switches reporting speed.

 -What:  /sys/class/leds/0005\:056A\:00BD.0001\:selector\:*/
 -Date:  May 2012
 -Kernel Version:3.5
 -Contact:   linux-blueto...@vger.kernel.org
 -Description:
 -   LED selector for Intuos4 WL. There are 4 leds, but only one 
 LED
 -   can be lit at a time. Max brightness is 127.
 -
 -What:  
 /sys/bus/usb/devices/busnum-devnum:cfg.intf/wacom_led/led
 -Date:  August 2011
 +What:  /sys/bus/hid/devices/bus:vid:pid.n/wacom_led/led
 +Date:  August 2014
  Contact:   linux-in...@vger.kernel.org
  Description:
 Attribute group for control of the status LEDs and the OLEDs.
 This attribute group is only available for Intuos 4 M, L,
 -   and XL (with LEDs and OLEDs), Intuos 5 (LEDs only), and Cintiq
 -   21UX2 and Cintiq 24HD (LEDs only). Therefore its presence
 -   implicitly signifies the presence of said LEDs and OLEDs on 
 the
 -   tablet device.
 +   and XL (with LEDs and OLEDs), Intuos 4 WL, Intuos 5 (LEDs 
 only),
 +   Intuos Pro (LEDs only) and Cintiq 21UX2 and Cintiq 24HD
 +   (LEDs only). Therefore its presence implicitly signifies the
 +   presence of said LEDs and OLEDs on the tablet device.

 -What:  
 /sys/bus/usb/devices/busnum-devnum:cfg.intf/wacom_led/status0_luminance
 -Date:  August 2011
 +What:  
 /sys/bus/hid/devices/bus:vid:pid.n/wacom_led/status0_luminance
 +Date:  August 2014
  Contact:   linux-in...@vger.kernel.org
  Description:
 Writing to this file sets the status LED luminance (1..127)
 @@ -50,16 +29,16 @@ Description:
 button is pressed on the stylus. This luminance level is
 normally lower than the level when a button is pressed.

 -What:  
 /sys/bus/usb/devices/busnum-devnum:cfg.intf/wacom_led/status1_luminance
 -Date:  August 2011
 +What:  
 /sys/bus/hid/devices/bus:vid:pid.n/wacom_led/status1_luminance
 +Date:  August 2014
  Contact:   linux-in...@vger.kernel.org
  Description:
 Writing to this file sets the status LED luminance (1..127

Re: [PATCH] Input - wacom: update the ABI doc according to latest changes

[Ping Cheng]

On Tue, Aug 5, 2014 at 2:34 PM, Benjamin Tissoires
 wrote:
> Hi Ping,
>
> On Aug 05 2014 or thereabouts, Ping Cheng wrote:
>> Thank you Benjamin for updating the ABI docs. My comments are inline.
>>
>> With those lines updated, this patch is:
>>
>> Reviewed-by: Ping Cheng 
>
> Thanks!
>
>>
>> Cheers,
>>
>> Ping
>>
>> On Tue, Aug 5, 2014 at 8:34 AM, Benjamin Tissoires
>>  wrote:
>> > Now the devices show up under hid no matter the connection. So update the
>> > sysfs path to the shortest and common one.
>> > The ABI has also been unified regarding the OLEDs, but Bluetooth handle
>> > only 1-bit images instead of 4.
>> >
>> > Signed-off-by: Benjamin Tissoires 
>> > ---
>> >
>> > Guys,
>> >
>> > by updating this and working on the g-s-d client side for OLEDs, I think 
>> > now
>> > that we should also keep the oled*_img for Bluetooth and implement this 
>> > file
>> > for USB too. The user space code gets messy quite fast due to the need to
>> > support old and new kernels, whereas if we handle the scrambling in the 
>> > kernel,
>> > it should be quite straightforward.
>> > Of course, we have to keep the current button_rawimg for backward
>> > compatibility.
>> >
>> > Any thoughts?
>> >
>> > Cheers,
>> > Benjamin
>> >
>> >  Documentation/ABI/testing/sysfs-driver-wacom | 56 
>> > ++--
>> >  1 file changed, 20 insertions(+), 36 deletions(-)
>> >
>> > diff --git a/Documentation/ABI/testing/sysfs-driver-wacom 
>> > b/Documentation/ABI/testing/sysfs-driver-wacom
>> > index 7fc7810..c75ed4c 100644
>> > --- a/Documentation/ABI/testing/sysfs-driver-wacom
>> > +++ b/Documentation/ABI/testing/sysfs-driver-wacom
>> > @@ -1,47 +1,26 @@
>> > -WWhat: /sys/class/hidraw/hidraw*/device/oled*_img
>> > -Date:  June 2012
>> > -Contact:   linux-blueto...@vger.kernel.org
>> > -Description:
>> > -   The /sys/class/hidraw/hidraw*/device/oled*_img files 
>> > control
>> > -   OLED mocro displays on Intuos4 Wireless tablet. Accepted 
>> > image
>> > -   has to contain 256 bytes (64x32 px 1 bit colour). The 
>> > format
>> > -   is the same as PBM image 62x32px without header (64 bits 
>> > per
>> > -   horizontal line, 32 lines). An example of setting OLED No. 
>> > 0:
>> > -   dd bs=256 count=1 if=img_file of=[path to 
>> > oled0_img]/oled0_img
>> > -   The attribute is read only and no local copy of the image 
>> > is
>> > -   stored.
>> > -
>> > -What:  /sys/class/hidraw/hidraw*/device/speed
>> > +What:  /sys/bus/hid/devices/::./speed
>> >  Date:  April 2010
>>
>> Should the Date be updated?
>
> I don't think that this one should. The ABI is the same since April
> 2010, and the "new" path works also for older kernels. So basically,
> here, for Bluetooth, there has been no changes.

Oh, I see your point. But, don't we consider path part of the interface?

Cheers,

Ping

>>
>> >  Kernel Version:2.6.35
>>
>> This patch is planned for 3.16, right?
>
> Same comment than before (about not touching it).
> This will be planned for 3.17, 3.16 has been out last Sunday.
>
>>
>> >  Contact:   linux-blueto...@vger.kernel.org
>> >  Description:
>> > -   The /sys/class/hidraw/hidraw*/device/speed file controls
>> > -   reporting speed of Wacom bluetooth tablet. Reading from
>> > -   this file returns 1 if tablet reports in high speed mode
>> > +   The /sys/bus/hid/devices/::./speed file
>> > +   controls reporting speed of Wacom bluetooth tablet. Reading
>> > +   from this file returns 1 if tablet reports in high speed 
>> > mode
>> > or 0 otherwise. Writing to this file one of these values
>> > switches reporting speed.
>> >
>> > -What:  /sys/class/leds/0005\:056A\:00BD.0001\:selector\:*/
>> > -Date:  May 2012
>> > -Kernel Version:3.5
>> > -Contact:   linux-blueto...@vger.kernel.org
>> > -Description:
>> > -   LED selector for Intuos4 WL. There are 4 leds, but only 
>> > one 

Re: [PATCH] Input - wacom: update the ABI doc according to latest changes

[Ping Cheng]

Thank you Benjamin for updating the ABI docs. My comments are inline.

With those lines updated, this patch is:

Reviewed-by: Ping Cheng 

Cheers,

Ping

On Tue, Aug 5, 2014 at 8:34 AM, Benjamin Tissoires
 wrote:
> Now the devices show up under hid no matter the connection. So update the
> sysfs path to the shortest and common one.
> The ABI has also been unified regarding the OLEDs, but Bluetooth handle
> only 1-bit images instead of 4.
>
> Signed-off-by: Benjamin Tissoires 
> ---
>
> Guys,
>
> by updating this and working on the g-s-d client side for OLEDs, I think now
> that we should also keep the oled*_img for Bluetooth and implement this file
> for USB too. The user space code gets messy quite fast due to the need to
> support old and new kernels, whereas if we handle the scrambling in the 
> kernel,
> it should be quite straightforward.
> Of course, we have to keep the current button_rawimg for backward
> compatibility.
>
> Any thoughts?
>
> Cheers,
> Benjamin
>
>  Documentation/ABI/testing/sysfs-driver-wacom | 56 
> ++--
>  1 file changed, 20 insertions(+), 36 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-wacom 
> b/Documentation/ABI/testing/sysfs-driver-wacom
> index 7fc7810..c75ed4c 100644
> --- a/Documentation/ABI/testing/sysfs-driver-wacom
> +++ b/Documentation/ABI/testing/sysfs-driver-wacom
> @@ -1,47 +1,26 @@
> -WWhat: /sys/class/hidraw/hidraw*/device/oled*_img
> -Date:  June 2012
> -Contact:   linux-blueto...@vger.kernel.org
> -Description:
> -   The /sys/class/hidraw/hidraw*/device/oled*_img files control
> -   OLED mocro displays on Intuos4 Wireless tablet. Accepted image
> -   has to contain 256 bytes (64x32 px 1 bit colour). The format
> -   is the same as PBM image 62x32px without header (64 bits per
> -   horizontal line, 32 lines). An example of setting OLED No. 0:
> -   dd bs=256 count=1 if=img_file of=[path to oled0_img]/oled0_img
> -   The attribute is read only and no local copy of the image is
> -   stored.
> -
> -What:  /sys/class/hidraw/hidraw*/device/speed
> +What:  /sys/bus/hid/devices/::./speed
>  Date:  April 2010

Should the Date be updated?

>  Kernel Version:2.6.35

This patch is planned for 3.16, right?

>  Contact:   linux-blueto...@vger.kernel.org
>  Description:
> -   The /sys/class/hidraw/hidraw*/device/speed file controls
> -   reporting speed of Wacom bluetooth tablet. Reading from
> -   this file returns 1 if tablet reports in high speed mode
> +   The /sys/bus/hid/devices/::./speed file
> +   controls reporting speed of Wacom bluetooth tablet. Reading
> +   from this file returns 1 if tablet reports in high speed mode
> or 0 otherwise. Writing to this file one of these values
> switches reporting speed.
>
> -What:  /sys/class/leds/0005\:056A\:00BD.0001\:selector\:*/
> -Date:  May 2012
> -Kernel Version:3.5
> -Contact:   linux-blueto...@vger.kernel.org
> -Description:
> -   LED selector for Intuos4 WL. There are 4 leds, but only one 
> LED
> -   can be lit at a time. Max brightness is 127.
> -
> -What:  
> /sys/bus/usb/devices/-:./wacom_led/led
> +What:  /sys/bus/hid/devices/::./wacom_led/led
>  Date:  August 2011

Same here and other 6 dates below

>  Contact:   linux-in...@vger.kernel.org
>  Description:
> Attribute group for control of the status LEDs and the OLEDs.
> This attribute group is only available for Intuos 4 M, L,
> -   and XL (with LEDs and OLEDs), Intuos 5 (LEDs only), and Cintiq
> -   21UX2 and Cintiq 24HD (LEDs only). Therefore its presence
> -   implicitly signifies the presence of said LEDs and OLEDs on 
> the
> -   tablet device.
> +   and XL (with LEDs and OLEDs), Intuos 4 WL, Intuos 5 (LEDs 
> only),
> +   Intuos Pro (LEDs only) and Cintiq 21UX2 and Cintiq 24HD
> +   (LEDs only). Therefore its presence implicitly signifies the
> +   presence of said LEDs and OLEDs on the tablet device.
>
> -What:  
> /sys/bus/usb/devices/-:./wacom_led/status0_luminance
> +What:  
> /sys/bus/hid/devices/::./wacom_led/status0_luminance
>  Date:  August 2011
>  Contact:   linux-in...@vger.kernel.org
>  Description:
> @@ -50,7 +29,7 @@ Description:
> button is pressed on the stylus. This luminance level is
> 

Re: [PATCH] Input - wacom: update the ABI doc according to latest changes

[Ping Cheng]

Thank you Benjamin for updating the ABI docs. My comments are inline.

With those lines updated, this patch is:

Reviewed-by: Ping Cheng pi...@wacom.com

Cheers,

Ping

On Tue, Aug 5, 2014 at 8:34 AM, Benjamin Tissoires
benjamin.tissoi...@redhat.com wrote:
 Now the devices show up under hid no matter the connection. So update the
 sysfs path to the shortest and common one.
 The ABI has also been unified regarding the OLEDs, but Bluetooth handle
 only 1-bit images instead of 4.

 Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com
 ---

 Guys,

 by updating this and working on the g-s-d client side for OLEDs, I think now
 that we should also keep the oled*_img for Bluetooth and implement this file
 for USB too. The user space code gets messy quite fast due to the need to
 support old and new kernels, whereas if we handle the scrambling in the 
 kernel,
 it should be quite straightforward.
 Of course, we have to keep the current buttonn_rawimg for backward
 compatibility.

 Any thoughts?

 Cheers,
 Benjamin

  Documentation/ABI/testing/sysfs-driver-wacom | 56 
 ++--
  1 file changed, 20 insertions(+), 36 deletions(-)

 diff --git a/Documentation/ABI/testing/sysfs-driver-wacom 
 b/Documentation/ABI/testing/sysfs-driver-wacom
 index 7fc7810..c75ed4c 100644
 --- a/Documentation/ABI/testing/sysfs-driver-wacom
 +++ b/Documentation/ABI/testing/sysfs-driver-wacom
 @@ -1,47 +1,26 @@
 -WWhat: /sys/class/hidraw/hidraw*/device/oled*_img
 -Date:  June 2012
 -Contact:   linux-blueto...@vger.kernel.org
 -Description:
 -   The /sys/class/hidraw/hidraw*/device/oled*_img files control
 -   OLED mocro displays on Intuos4 Wireless tablet. Accepted image
 -   has to contain 256 bytes (64x32 px 1 bit colour). The format
 -   is the same as PBM image 62x32px without header (64 bits per
 -   horizontal line, 32 lines). An example of setting OLED No. 0:
 -   dd bs=256 count=1 if=img_file of=[path to oled0_img]/oled0_img
 -   The attribute is read only and no local copy of the image is
 -   stored.
 -
 -What:  /sys/class/hidraw/hidraw*/device/speed
 +What:  /sys/bus/hid/devices/bus:vid:pid.n/speed
  Date:  April 2010

Should the Date be updated?

  Kernel Version:2.6.35

This patch is planned for 3.16, right?

  Contact:   linux-blueto...@vger.kernel.org
  Description:
 -   The /sys/class/hidraw/hidraw*/device/speed file controls
 -   reporting speed of Wacom bluetooth tablet. Reading from
 -   this file returns 1 if tablet reports in high speed mode
 +   The /sys/bus/hid/devices/bus:vid:pid.n/speed file
 +   controls reporting speed of Wacom bluetooth tablet. Reading
 +   from this file returns 1 if tablet reports in high speed mode
 or 0 otherwise. Writing to this file one of these values
 switches reporting speed.

 -What:  /sys/class/leds/0005\:056A\:00BD.0001\:selector\:*/
 -Date:  May 2012
 -Kernel Version:3.5
 -Contact:   linux-blueto...@vger.kernel.org
 -Description:
 -   LED selector for Intuos4 WL. There are 4 leds, but only one 
 LED
 -   can be lit at a time. Max brightness is 127.
 -
 -What:  
 /sys/bus/usb/devices/busnum-devnum:cfg.intf/wacom_led/led
 +What:  /sys/bus/hid/devices/bus:vid:pid.n/wacom_led/led
  Date:  August 2011

Same here and other 6 dates below

  Contact:   linux-in...@vger.kernel.org
  Description:
 Attribute group for control of the status LEDs and the OLEDs.
 This attribute group is only available for Intuos 4 M, L,
 -   and XL (with LEDs and OLEDs), Intuos 5 (LEDs only), and Cintiq
 -   21UX2 and Cintiq 24HD (LEDs only). Therefore its presence
 -   implicitly signifies the presence of said LEDs and OLEDs on 
 the
 -   tablet device.
 +   and XL (with LEDs and OLEDs), Intuos 4 WL, Intuos 5 (LEDs 
 only),
 +   Intuos Pro (LEDs only) and Cintiq 21UX2 and Cintiq 24HD
 +   (LEDs only). Therefore its presence implicitly signifies the
 +   presence of said LEDs and OLEDs on the tablet device.

 -What:  
 /sys/bus/usb/devices/busnum-devnum:cfg.intf/wacom_led/status0_luminance
 +What:  
 /sys/bus/hid/devices/bus:vid:pid.n/wacom_led/status0_luminance
  Date:  August 2011
  Contact:   linux-in...@vger.kernel.org
  Description:
 @@ -50,7 +29,7 @@ Description:
 button is pressed on the stylus. This luminance level is
 normally lower than the level when a button is pressed.

 -What:  
 /sys/bus/usb/devices/busnum-devnum:cfg.intf/wacom_led/status1_luminance
 +What:  
 /sys/bus/hid/devices/bus:vid:pid.n/wacom_led/status1_luminance
  Date

Re: [PATCH] Input - wacom: update the ABI doc according to latest changes

[Ping Cheng]

On Tue, Aug 5, 2014 at 2:34 PM, Benjamin Tissoires
benjamin.tissoi...@redhat.com wrote:
 Hi Ping,

 On Aug 05 2014 or thereabouts, Ping Cheng wrote:
 Thank you Benjamin for updating the ABI docs. My comments are inline.

 With those lines updated, this patch is:

 Reviewed-by: Ping Cheng pi...@wacom.com

 Thanks!


 Cheers,

 Ping

 On Tue, Aug 5, 2014 at 8:34 AM, Benjamin Tissoires
 benjamin.tissoi...@redhat.com wrote:
  Now the devices show up under hid no matter the connection. So update the
  sysfs path to the shortest and common one.
  The ABI has also been unified regarding the OLEDs, but Bluetooth handle
  only 1-bit images instead of 4.
 
  Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com
  ---
 
  Guys,
 
  by updating this and working on the g-s-d client side for OLEDs, I think 
  now
  that we should also keep the oled*_img for Bluetooth and implement this 
  file
  for USB too. The user space code gets messy quite fast due to the need to
  support old and new kernels, whereas if we handle the scrambling in the 
  kernel,
  it should be quite straightforward.
  Of course, we have to keep the current buttonn_rawimg for backward
  compatibility.
 
  Any thoughts?
 
  Cheers,
  Benjamin
 
   Documentation/ABI/testing/sysfs-driver-wacom | 56 
  ++--
   1 file changed, 20 insertions(+), 36 deletions(-)
 
  diff --git a/Documentation/ABI/testing/sysfs-driver-wacom 
  b/Documentation/ABI/testing/sysfs-driver-wacom
  index 7fc7810..c75ed4c 100644
  --- a/Documentation/ABI/testing/sysfs-driver-wacom
  +++ b/Documentation/ABI/testing/sysfs-driver-wacom
  @@ -1,47 +1,26 @@
  -WWhat: /sys/class/hidraw/hidraw*/device/oled*_img
  -Date:  June 2012
  -Contact:   linux-blueto...@vger.kernel.org
  -Description:
  -   The /sys/class/hidraw/hidraw*/device/oled*_img files 
  control
  -   OLED mocro displays on Intuos4 Wireless tablet. Accepted 
  image
  -   has to contain 256 bytes (64x32 px 1 bit colour). The 
  format
  -   is the same as PBM image 62x32px without header (64 bits 
  per
  -   horizontal line, 32 lines). An example of setting OLED No. 
  0:
  -   dd bs=256 count=1 if=img_file of=[path to 
  oled0_img]/oled0_img
  -   The attribute is read only and no local copy of the image 
  is
  -   stored.
  -
  -What:  /sys/class/hidraw/hidraw*/device/speed
  +What:  /sys/bus/hid/devices/bus:vid:pid.n/speed
   Date:  April 2010

 Should the Date be updated?

 I don't think that this one should. The ABI is the same since April
 2010, and the new path works also for older kernels. So basically,
 here, for Bluetooth, there has been no changes.

Oh, I see your point. But, don't we consider path part of the interface?

Cheers,

Ping


   Kernel Version:2.6.35

 This patch is planned for 3.16, right?

 Same comment than before (about not touching it).
 This will be planned for 3.17, 3.16 has been out last Sunday.


   Contact:   linux-blueto...@vger.kernel.org
   Description:
  -   The /sys/class/hidraw/hidraw*/device/speed file controls
  -   reporting speed of Wacom bluetooth tablet. Reading from
  -   this file returns 1 if tablet reports in high speed mode
  +   The /sys/bus/hid/devices/bus:vid:pid.n/speed file
  +   controls reporting speed of Wacom bluetooth tablet. Reading
  +   from this file returns 1 if tablet reports in high speed 
  mode
  or 0 otherwise. Writing to this file one of these values
  switches reporting speed.
 
  -What:  /sys/class/leds/0005\:056A\:00BD.0001\:selector\:*/
  -Date:  May 2012
  -Kernel Version:3.5
  -Contact:   linux-blueto...@vger.kernel.org
  -Description:
  -   LED selector for Intuos4 WL. There are 4 leds, but only 
  one LED
  -   can be lit at a time. Max brightness is 127.
  -
  -What:  
  /sys/bus/usb/devices/busnum-devnum:cfg.intf/wacom_led/led
  +What:  /sys/bus/hid/devices/bus:vid:pid.n/wacom_led/led
   Date:  August 2011

 Same here and other 6 dates below

 We can probably update these dates. I let them untouched because the
 ABI did not changed but only the path did.


   Contact:   linux-in...@vger.kernel.org
   Description:
  Attribute group for control of the status LEDs and the 
  OLEDs.
  This attribute group is only available for Intuos 4 M, L,
  -   and XL (with LEDs and OLEDs), Intuos 5 (LEDs only), and 
  Cintiq
  -   21UX2 and Cintiq 24HD (LEDs only). Therefore its presence
  -   implicitly signifies the presence of said LEDs and OLEDs 
  on the
  -   tablet device.
  +   and XL (with LEDs and OLEDs), Intuos 4 WL, Intuos 5 (LEDs 
  only),
  +   Intuos Pro (LEDs only) and Cintiq

Re: [PATCH v3 4/7] Input - wacom: Check for bluetooth protocol while setting OLEDs

[Ping Cheng]

On Tue, Jul 29, 2014 at 3:53 PM, Przemo Firszt  wrote:
> Dnia 2014-07-29, wto o godzinie 14:43 -0400, Benjamin Tissoires pisze:
>> Bluetooth Intuos 4 use 1-bit definition while the USB ones use a 4-bits
>> definition. This changes the size of the raw image we receive, and thus
>> the kernel will only accept 1-bit images for Bluetooth and 4-bits for
>> USB.
>>
>> Signed-off-by: Benjamin Tissoires 
>> ---
>>  drivers/hid/wacom_sys.c | 29 +
>>  1 file changed, 21 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
>> index f5c9c56..a12cd9c 100644
>> --- a/drivers/hid/wacom_sys.c
>> +++ b/drivers/hid/wacom_sys.c
>> @@ -20,6 +20,7 @@
>>  #define WAC_CMD_LED_CONTROL  0x20
>>  #define WAC_CMD_ICON_START   0x21
>>  #define WAC_CMD_ICON_XFER0x23
>> +#define WAC_CMD_ICON_BT_XFER 0x26
>>  #define WAC_CMD_RETRIES  10
>>
>>  static int wacom_get_report(struct hid_device *hdev, u8 type, u8 id,
>> @@ -526,12 +527,14 @@ static int wacom_led_control(struct wacom *wacom)
>>   return retval;
>>  }
>>
>> -static int wacom_led_putimage(struct wacom *wacom, int button_id, const 
>> void *img)
>> +static int wacom_led_putimage(struct wacom *wacom, int button_id, u8 
>> xfer_id,
>> + const unsigned len, const void *img)
>>  {
>>   unsigned char *buf;
>>   int i, retval;
>> + const unsigned chunk_len = len / 4; /* 4 chunks are needed to be sent 
>> */
>>
>> - buf = kzalloc(259, GFP_KERNEL);
>> + buf = kzalloc(chunk_len + 3 , GFP_KERNEL);
>>   if (!buf)
>>   return -ENOMEM;
>>
>> @@ -543,15 +546,15 @@ static int wacom_led_putimage(struct wacom *wacom, int 
>> button_id, const void *im
>>   if (retval < 0)
>>   goto out;
>>
>> - buf[0] = WAC_CMD_ICON_XFER;
>> + buf[0] = xfer_id;
>>   buf[1] = button_id & 0x07;
>>   for (i = 0; i < 4; i++) {
>>   buf[2] = i;
>> - memcpy(buf + 3, img + i * 256, 256);
>> + memcpy(buf + 3, img + i * chunk_len, chunk_len);
>>
>>   retval = wacom_set_report(wacom->hdev, HID_FEATURE_REPORT,
>> -   WAC_CMD_ICON_XFER,
>> -   buf, 259, WAC_CMD_RETRIES);
>> +   xfer_id, buf, chunk_len + 3,
>> +   WAC_CMD_RETRIES);
>>   if (retval < 0)
>>   break;
>>   }
>> @@ -652,13 +655,23 @@ static ssize_t wacom_button_image_store(struct device 
>> *dev, int button_id,
>>   struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>>   struct wacom *wacom = hid_get_drvdata(hdev);
>>   int err;
>> + unsigned len;
>> + u8 xfer_id;
>>
>> - if (count != 1024)
>> + if (hdev->bus == BUS_BLUETOOTH) {
>> + len = 256;
>> + xfer_id = WAC_CMD_ICON_BT_XFER;
>> + } else {
>> + len = 1024;
>> + xfer_id = WAC_CMD_ICON_XFER;
>> + }
>> +
>> + if (count != len)
>>   return -EINVAL;
>>
>>   mutex_lock(>lock);
>>
>> - err = wacom_led_putimage(wacom, button_id, buf);
>> + err = wacom_led_putimage(wacom, button_id, xfer_id, len, buf);
>>
>>   mutex_unlock(>lock);
>
> Signed-off-by: Przemo Firszt 

Reviewed-by: Ping Cheng 

> I'll test the whole series tomorrow.

I'd like to see Przemo's Tested-by tag here as well. Przemo, are you
done with your testing?

Thank you Przemo and Benjamin for your effort in making this
transition smooth and successful. We, at Wacom, are very grateful to
be part of this great community.

Cheers,

Ping
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 4/7] Input - wacom: Check for bluetooth protocol while setting OLEDs

[Ping Cheng]

On Tue, Jul 29, 2014 at 3:53 PM, Przemo Firszt prz...@firszt.eu wrote:
 Dnia 2014-07-29, wto o godzinie 14:43 -0400, Benjamin Tissoires pisze:
 Bluetooth Intuos 4 use 1-bit definition while the USB ones use a 4-bits
 definition. This changes the size of the raw image we receive, and thus
 the kernel will only accept 1-bit images for Bluetooth and 4-bits for
 USB.

 Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com
 ---
  drivers/hid/wacom_sys.c | 29 +
  1 file changed, 21 insertions(+), 8 deletions(-)

 diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
 index f5c9c56..a12cd9c 100644
 --- a/drivers/hid/wacom_sys.c
 +++ b/drivers/hid/wacom_sys.c
 @@ -20,6 +20,7 @@
  #define WAC_CMD_LED_CONTROL  0x20
  #define WAC_CMD_ICON_START   0x21
  #define WAC_CMD_ICON_XFER0x23
 +#define WAC_CMD_ICON_BT_XFER 0x26
  #define WAC_CMD_RETRIES  10

  static int wacom_get_report(struct hid_device *hdev, u8 type, u8 id,
 @@ -526,12 +527,14 @@ static int wacom_led_control(struct wacom *wacom)
   return retval;
  }

 -static int wacom_led_putimage(struct wacom *wacom, int button_id, const 
 void *img)
 +static int wacom_led_putimage(struct wacom *wacom, int button_id, u8 
 xfer_id,
 + const unsigned len, const void *img)
  {
   unsigned char *buf;
   int i, retval;
 + const unsigned chunk_len = len / 4; /* 4 chunks are needed to be sent 
 */

 - buf = kzalloc(259, GFP_KERNEL);
 + buf = kzalloc(chunk_len + 3 , GFP_KERNEL);
   if (!buf)
   return -ENOMEM;

 @@ -543,15 +546,15 @@ static int wacom_led_putimage(struct wacom *wacom, int 
 button_id, const void *im
   if (retval  0)
   goto out;

 - buf[0] = WAC_CMD_ICON_XFER;
 + buf[0] = xfer_id;
   buf[1] = button_id  0x07;
   for (i = 0; i  4; i++) {
   buf[2] = i;
 - memcpy(buf + 3, img + i * 256, 256);
 + memcpy(buf + 3, img + i * chunk_len, chunk_len);

   retval = wacom_set_report(wacom-hdev, HID_FEATURE_REPORT,
 -   WAC_CMD_ICON_XFER,
 -   buf, 259, WAC_CMD_RETRIES);
 +   xfer_id, buf, chunk_len + 3,
 +   WAC_CMD_RETRIES);
   if (retval  0)
   break;
   }
 @@ -652,13 +655,23 @@ static ssize_t wacom_button_image_store(struct device 
 *dev, int button_id,
   struct hid_device *hdev = container_of(dev, struct hid_device, dev);
   struct wacom *wacom = hid_get_drvdata(hdev);
   int err;
 + unsigned len;
 + u8 xfer_id;

 - if (count != 1024)
 + if (hdev-bus == BUS_BLUETOOTH) {
 + len = 256;
 + xfer_id = WAC_CMD_ICON_BT_XFER;
 + } else {
 + len = 1024;
 + xfer_id = WAC_CMD_ICON_XFER;
 + }
 +
 + if (count != len)
   return -EINVAL;

   mutex_lock(wacom-lock);

 - err = wacom_led_putimage(wacom, button_id, buf);
 + err = wacom_led_putimage(wacom, button_id, xfer_id, len, buf);

   mutex_unlock(wacom-lock);

 Signed-off-by: Przemo Firszt prz...@firszt.eu

Reviewed-by: Ping Cheng pi...@wacom.com

 I'll test the whole series tomorrow.

I'd like to see Przemo's Tested-by tag here as well. Przemo, are you
done with your testing?

Thank you Przemo and Benjamin for your effort in making this
transition smooth and successful. We, at Wacom, are very grateful to
be part of this great community.

Cheers,

Ping
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 07/10] Input - wacom: handle Graphire BT tablets in wacom.ko

[Ping Cheng]

On Thu, Jul 24, 2014 at 12:58 PM, Dmitry Torokhov
 wrote:
>
> On Thu, Jul 24, 2014 at 03:43:52PM -0400, Benjamin Tissoires wrote:
> > On Jul 24 2014 or thereabouts, Dmitry Torokhov wrote:
> > > Hi Benjamin,
> > >
> > > On Thu, Jul 24, 2014 at 02:14:02PM -0400, Benjamin Tissoires wrote:
> > > > + } else if (features->type == GRAPHIRE_BT) {
> > > > + /* Compute distance between mouse and tablet 
> > > > */
> > > > + rw = 44 - (data[6] >> 2);
> > > > + if (rw < 0)
> > > > + rw = 0;
> > > > + else if (rw > 31)
> > > > + rw = 31;
> > >
> > > rw = clamp_val(rw, 0, 31);
> > >
> > > ?
> >
> > could be...
> >
> > I'll do the tests and resubmit later the v3. I'll wait a little in case
> > you have other comments on the other patches :)
>
> How about doing it incrementally - the patch is still sound as far as I
> am concerned. I will wait a bit for other to chime in with comments or
> acks before applying this series.


Thank you Benjamin and Dmitry for your support. The whole series is:

Acked-by: Ping Cheng 

Cheers,

Ping
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 07/10] Input - wacom: handle Graphire BT tablets in wacom.ko

[Ping Cheng]

On Thu, Jul 24, 2014 at 12:58 PM, Dmitry Torokhov
dmitry.torok...@gmail.com wrote:

 On Thu, Jul 24, 2014 at 03:43:52PM -0400, Benjamin Tissoires wrote:
  On Jul 24 2014 or thereabouts, Dmitry Torokhov wrote:
   Hi Benjamin,
  
   On Thu, Jul 24, 2014 at 02:14:02PM -0400, Benjamin Tissoires wrote:
+ } else if (features-type == GRAPHIRE_BT) {
+ /* Compute distance between mouse and tablet 
*/
+ rw = 44 - (data[6]  2);
+ if (rw  0)
+ rw = 0;
+ else if (rw  31)
+ rw = 31;
  
   rw = clamp_val(rw, 0, 31);
  
   ?
 
  could be...
 
  I'll do the tests and resubmit later the v3. I'll wait a little in case
  you have other comments on the other patches :)

 How about doing it incrementally - the patch is still sound as far as I
 am concerned. I will wait a bit for other to chime in with comments or
 acks before applying this series.


Thank you Benjamin and Dmitry for your support. The whole series is:

Acked-by: Ping Cheng pi...@wacom.com

Cheers,

Ping
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Input - wacom: split out the pad device for the wireless receiver

[Ping Cheng]

On Wed, Jul 2, 2014 at 2:37 PM, Benjamin Tissoires
 wrote:
> The Wireless Receiver should also behave in the same way than regular
> USB devices.
>
> To simplify the unregistering of the different devices,
> wacom_unregister_inputs() is introduced.
> For consistency, the function wacom_register_input() is renamed into
> wacom_register_input().

wacom_register_inputs().

>
> Signed-off-by: Benjamin Tissoires 

Reviewed-by: Ping Cheng .

Ping

> ---
>
> Hi,
>
> I noticed this afternoon that the "pad-in-a-separate-device" was missing the
> conversion for the Wireless Receiver too :(
> So here is a fix.
>
> I also have a patch for hid-wacom to behave in the same way regarding the
> bluetooth devices. I'll send it once we know a little bit more about how will 
> be
> handled wacom.ko in 3.17.
>
> Cheers,
> Benjamin
>
>  drivers/input/tablet/wacom_sys.c | 46 
> 
>  1 file changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/input/tablet/wacom_sys.c 
> b/drivers/input/tablet/wacom_sys.c
> index 598efd4..a70aa01 100644
> --- a/drivers/input/tablet/wacom_sys.c
> +++ b/drivers/input/tablet/wacom_sys.c
> @@ -1148,7 +1148,17 @@ static struct input_dev *wacom_allocate_input(struct 
> wacom *wacom)
> return input_dev;
>  }
>
> -static int wacom_register_input(struct wacom *wacom)
> +static void wacom_unregister_inputs(struct wacom *wacom)
> +{
> +   if (wacom->wacom_wac.input)
> +   input_unregister_device(wacom->wacom_wac.input);
> +   if (wacom->wacom_wac.pad_input)
> +   input_unregister_device(wacom->wacom_wac.pad_input);
> +   wacom->wacom_wac.input = NULL;
> +   wacom->wacom_wac.pad_input = NULL;
> +}
> +
> +static int wacom_register_inputs(struct wacom *wacom)
>  {
> struct input_dev *input_dev, *pad_input_dev;
> struct wacom_wac *wacom_wac = &(wacom->wacom_wac);
> @@ -1220,16 +1230,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_inputs(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_inputs(wacom2);
>
> if (wacom_wac->pid == 0) {
> dev_info(>intf->dev, "wireless tablet disconnected\n");
> @@ -1259,9 +1265,11 @@ static void wacom_wireless_work(struct work_struct 
> *work)
> wacom_wac1->features.device_type = BTN_TOOL_PEN;
> snprintf(wacom_wac1->name, WACOM_NAME_MAX, "%s (WL) Pen",
>  wacom_wac1->features.name);
> +   snprintf(wacom_wac1->pad_name, WACOM_NAME_MAX, "%s (WL) Pad",
> +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);
> +   error = wacom_register_inputs(wacom1);
> if (error)
> goto fail;
>
> @@ -1279,7 +1287,9 @@ 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);
> +   snprintf(wacom_wac2->pad_name, WACOM_NAME_MAX,
> +"%s (WL) Pad", wacom_wac2->features.name);
> +   error = wacom_register_inputs(wacom2);
> if (error)
> goto fail;
>
> @@ -1296,15 +1306,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) {
>

Re: [PATCH] Input - wacom: split out the pad device for the wireless receiver

[Ping Cheng]

On Wed, Jul 2, 2014 at 2:37 PM, Benjamin Tissoires
benjamin.tissoi...@redhat.com wrote:
 The Wireless Receiver should also behave in the same way than regular
 USB devices.

 To simplify the unregistering of the different devices,
 wacom_unregister_inputs() is introduced.
 For consistency, the function wacom_register_input() is renamed into
 wacom_register_input().

wacom_register_inputs().


 Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com

Reviewed-by: Ping Cheng pi...@wacom.com.

Ping

 ---

 Hi,

 I noticed this afternoon that the pad-in-a-separate-device was missing the
 conversion for the Wireless Receiver too :(
 So here is a fix.

 I also have a patch for hid-wacom to behave in the same way regarding the
 bluetooth devices. I'll send it once we know a little bit more about how will 
 be
 handled wacom.ko in 3.17.

 Cheers,
 Benjamin

  drivers/input/tablet/wacom_sys.c | 46 
 
  1 file changed, 23 insertions(+), 23 deletions(-)

 diff --git a/drivers/input/tablet/wacom_sys.c 
 b/drivers/input/tablet/wacom_sys.c
 index 598efd4..a70aa01 100644
 --- a/drivers/input/tablet/wacom_sys.c
 +++ b/drivers/input/tablet/wacom_sys.c
 @@ -1148,7 +1148,17 @@ static struct input_dev *wacom_allocate_input(struct 
 wacom *wacom)
 return input_dev;
  }

 -static int wacom_register_input(struct wacom *wacom)
 +static void wacom_unregister_inputs(struct wacom *wacom)
 +{
 +   if (wacom-wacom_wac.input)
 +   input_unregister_device(wacom-wacom_wac.input);
 +   if (wacom-wacom_wac.pad_input)
 +   input_unregister_device(wacom-wacom_wac.pad_input);
 +   wacom-wacom_wac.input = NULL;
 +   wacom-wacom_wac.pad_input = NULL;
 +}
 +
 +static int wacom_register_inputs(struct wacom *wacom)
  {
 struct input_dev *input_dev, *pad_input_dev;
 struct wacom_wac *wacom_wac = (wacom-wacom_wac);
 @@ -1220,16 +1230,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_inputs(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_inputs(wacom2);

 if (wacom_wac-pid == 0) {
 dev_info(wacom-intf-dev, wireless tablet disconnected\n);
 @@ -1259,9 +1265,11 @@ static void wacom_wireless_work(struct work_struct 
 *work)
 wacom_wac1-features.device_type = BTN_TOOL_PEN;
 snprintf(wacom_wac1-name, WACOM_NAME_MAX, %s (WL) Pen,
  wacom_wac1-features.name);
 +   snprintf(wacom_wac1-pad_name, WACOM_NAME_MAX, %s (WL) Pad,
 +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);
 +   error = wacom_register_inputs(wacom1);
 if (error)
 goto fail;

 @@ -1279,7 +1287,9 @@ 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);
 +   snprintf(wacom_wac2-pad_name, WACOM_NAME_MAX,
 +%s (WL) Pad, wacom_wac2-features.name);
 +   error = wacom_register_inputs(wacom2);
 if (error)
 goto fail;

 @@ -1296,15 +1306,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_inputs(wacom1);
 +   wacom_unregister_inputs(wacom2);
 return;
  }

 @@ -1450,7 +1453,7 @@ static int wacom_probe(struct usb_interface *intf, 
 const struct usb_device_id *i
 goto fail4;

 if (!(features-quirks  WACOM_QUIRK_NO_INPUT)) {
 -   error = wacom_register_input(wacom);
 +   error = wacom_register_inputs(wacom);
 if (error)
 goto fail5;
 }
 @@ -1490,10 +1493,7 @@ static void

Re: [PATCH 1/5] Input - wacom: create a separate input device for pads

[Ping Cheng]

Hi Benjamin,

On Mon, Jun 23, 2014 at 1:57 PM, Benjamin Tissoires
 wrote:
> Currently, the pad events are sent through the stylus input device
> for the Intuos/Cintiqs, and through the touch input device for the
> Bamboos.
>
> To differentiate the buttons pressed on the pad from the ones pressed
> on the stylus, the Intuos/Cintiq uses MISC_SERIAL and ABS_MISC. This
> lead to a multiplexing of the events into one device, which are then
> splitted out in xf86-input-wacom. Bamboos are not using MISC events
> because the pad is attached to the touch interface, and only BTN_TOUCH
> is used for the finger (and DOUBLE_TAP, etc...). However, the user space
> driver still splits out the pad from the touch interface in the same
> way it does for the pro line devices.
>
> The other problem we can see with this fact is that some of the Intuos
> and Cintiq have a wheel, and the effective range of the reported values
> is [0..71]. Unfortunately, the airbrush stylus also sends wheel events
> (there is a small wheel on it), but in the range [0..1023]. From the user
> space point of view it is kind of difficult to understand that because
> the wheel on the pad are quite common, while the airbrush tool is not.
>
> A solution to fix all of these problems is to split out the pad device
> from the stylus/touch. This decision makes more sense because the pad is
> not linked to the absolute position of the finger or pen, and usually, the
> events from the pad are filtered out by the compositor, which then convert
> them into actions or keyboard shortcuts.

This is a very good solution. I like it.

> For backward compatibility with current xf86-input-wacom, the pad devices
> still present the ABS_X, ABS_Y and ABS_MISC events, but they can be
> completely ignored in the new implementation.

I do not think we need to keep ABS_X and ABS_Y for pad. We've already
supported a tablet (Intuos pen small, a pen only tablet with buttons
reported on touch interface) that does not set ABS_X and ABS_Y for
pad.

Unless you plan to use other means to tell userland those events are
from PAD tool, ABS_MISC is necessary and is a reasonable way to group
PAD events. So, I do not think it is for backward compatibility. It is
there to stay. With that said, the whole patchset is

> Signed-off-by: Benjamin Tissoires 

Reviewed-by: Ping Cheng 

Thank you, Benjamin, for your support.

Ping

> ---
>  drivers/input/tablet/wacom.h |  2 ++
>  drivers/input/tablet/wacom_sys.c | 63 
> +++-
>  drivers/input/tablet/wacom_wac.c | 27 -
>  drivers/input/tablet/wacom_wac.h |  2 ++
>  4 files changed, 85 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/input/tablet/wacom.h b/drivers/input/tablet/wacom.h
> index 9ebf0ed..caa59ca 100644
> --- a/drivers/input/tablet/wacom.h
> +++ b/drivers/input/tablet/wacom.h
> @@ -136,4 +136,6 @@ void wacom_wac_irq(struct wacom_wac *wacom_wac, size_t 
> len);
>  void wacom_setup_device_quirks(struct wacom_features *features);
>  int wacom_setup_input_capabilities(struct input_dev *input_dev,
>struct wacom_wac *wacom_wac);
> +int wacom_setup_pad_input_capabilities(struct input_dev *input_dev,
> +  struct wacom_wac *wacom_wac);
>  #endif
> diff --git a/drivers/input/tablet/wacom_sys.c 
> b/drivers/input/tablet/wacom_sys.c
> index c993eee..b9bf37e 100644
> --- a/drivers/input/tablet/wacom_sys.c
> +++ b/drivers/input/tablet/wacom_sys.c
> @@ -135,6 +135,9 @@ static int wacom_open(struct input_dev *dev)
>
> mutex_lock(>lock);
>
> +   if (wacom->open)
> +   goto out;
> +
> if (usb_submit_urb(wacom->irq, GFP_KERNEL)) {
> retval = -EIO;
> goto out;
> @@ -157,9 +160,14 @@ static void wacom_close(struct input_dev *dev)
> autopm_error = usb_autopm_get_interface(wacom->intf);
>
> mutex_lock(>lock);
> +   if (!wacom->open)
> +   goto out;
> +
> usb_kill_urb(wacom->irq);
> wacom->open = false;
> wacom->intf->needs_remote_wakeup = 0;
> +
> +out:
> mutex_unlock(>lock);
>
> if (!autopm_error)
> @@ -1109,19 +1117,16 @@ static void wacom_destroy_battery(struct wacom *wacom)
> }
>  }
>
> -static int wacom_register_input(struct wacom *wacom)
> +static struct input_dev *wacom_allocate_input(struct wacom *wacom)
>  {
> struct input_dev *input_dev;
> struct usb_interface *intf = wacom->intf;
> struct usb_device *dev = interface_to_usbdev(intf);
> struct wacom_wac *wacom_wac = &(wacom->wacom_wac);
> -   int error;
>
> i

Re: [PATCH 1/5] Input - wacom: create a separate input device for pads

[Ping Cheng]

Hi Benjamin,

On Mon, Jun 23, 2014 at 1:57 PM, Benjamin Tissoires
benjamin.tissoi...@redhat.com wrote:
 Currently, the pad events are sent through the stylus input device
 for the Intuos/Cintiqs, and through the touch input device for the
 Bamboos.

 To differentiate the buttons pressed on the pad from the ones pressed
 on the stylus, the Intuos/Cintiq uses MISC_SERIAL and ABS_MISC. This
 lead to a multiplexing of the events into one device, which are then
 splitted out in xf86-input-wacom. Bamboos are not using MISC events
 because the pad is attached to the touch interface, and only BTN_TOUCH
 is used for the finger (and DOUBLE_TAP, etc...). However, the user space
 driver still splits out the pad from the touch interface in the same
 way it does for the pro line devices.

 The other problem we can see with this fact is that some of the Intuos
 and Cintiq have a wheel, and the effective range of the reported values
 is [0..71]. Unfortunately, the airbrush stylus also sends wheel events
 (there is a small wheel on it), but in the range [0..1023]. From the user
 space point of view it is kind of difficult to understand that because
 the wheel on the pad are quite common, while the airbrush tool is not.

 A solution to fix all of these problems is to split out the pad device
 from the stylus/touch. This decision makes more sense because the pad is
 not linked to the absolute position of the finger or pen, and usually, the
 events from the pad are filtered out by the compositor, which then convert
 them into actions or keyboard shortcuts.

This is a very good solution. I like it.

 For backward compatibility with current xf86-input-wacom, the pad devices
 still present the ABS_X, ABS_Y and ABS_MISC events, but they can be
 completely ignored in the new implementation.

I do not think we need to keep ABS_X and ABS_Y for pad. We've already
supported a tablet (Intuos pen small, a pen only tablet with buttons
reported on touch interface) that does not set ABS_X and ABS_Y for
pad.

Unless you plan to use other means to tell userland those events are
from PAD tool, ABS_MISC is necessary and is a reasonable way to group
PAD events. So, I do not think it is for backward compatibility. It is
there to stay. With that said, the whole patchset is

 Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com

Reviewed-by: Ping Cheng pi...@wacom.com

Thank you, Benjamin, for your support.

Ping

 ---
  drivers/input/tablet/wacom.h |  2 ++
  drivers/input/tablet/wacom_sys.c | 63 
 +++-
  drivers/input/tablet/wacom_wac.c | 27 -
  drivers/input/tablet/wacom_wac.h |  2 ++
  4 files changed, 85 insertions(+), 9 deletions(-)

 diff --git a/drivers/input/tablet/wacom.h b/drivers/input/tablet/wacom.h
 index 9ebf0ed..caa59ca 100644
 --- a/drivers/input/tablet/wacom.h
 +++ b/drivers/input/tablet/wacom.h
 @@ -136,4 +136,6 @@ void wacom_wac_irq(struct wacom_wac *wacom_wac, size_t 
 len);
  void wacom_setup_device_quirks(struct wacom_features *features);
  int wacom_setup_input_capabilities(struct input_dev *input_dev,
struct wacom_wac *wacom_wac);
 +int wacom_setup_pad_input_capabilities(struct input_dev *input_dev,
 +  struct wacom_wac *wacom_wac);
  #endif
 diff --git a/drivers/input/tablet/wacom_sys.c 
 b/drivers/input/tablet/wacom_sys.c
 index c993eee..b9bf37e 100644
 --- a/drivers/input/tablet/wacom_sys.c
 +++ b/drivers/input/tablet/wacom_sys.c
 @@ -135,6 +135,9 @@ static int wacom_open(struct input_dev *dev)

 mutex_lock(wacom-lock);

 +   if (wacom-open)
 +   goto out;
 +
 if (usb_submit_urb(wacom-irq, GFP_KERNEL)) {
 retval = -EIO;
 goto out;
 @@ -157,9 +160,14 @@ static void wacom_close(struct input_dev *dev)
 autopm_error = usb_autopm_get_interface(wacom-intf);

 mutex_lock(wacom-lock);
 +   if (!wacom-open)
 +   goto out;
 +
 usb_kill_urb(wacom-irq);
 wacom-open = false;
 wacom-intf-needs_remote_wakeup = 0;
 +
 +out:
 mutex_unlock(wacom-lock);

 if (!autopm_error)
 @@ -1109,19 +1117,16 @@ static void wacom_destroy_battery(struct wacom *wacom)
 }
  }

 -static int wacom_register_input(struct wacom *wacom)
 +static struct input_dev *wacom_allocate_input(struct wacom *wacom)
  {
 struct input_dev *input_dev;
 struct usb_interface *intf = wacom-intf;
 struct usb_device *dev = interface_to_usbdev(intf);
 struct wacom_wac *wacom_wac = (wacom-wacom_wac);
 -   int error;

 input_dev = input_allocate_device();
 -   if (!input_dev) {
 -   error = -ENOMEM;
 -   goto fail1;
 -   }
 +   if (!input_dev)
 +   return NULL;

 input_dev-name = wacom_wac-name;
 input_dev-phys = wacom-phys;
 @@ -1131,21 +1136,59 @@ static int wacom_register_input(struct wacom

Re: [PATCH] Input - wacom: put a flag when the led are initialized

[Ping Cheng]

Hi Benjamin,

On Monday, June 16, 2014, Benjamin Tissoires
 wrote:
>
> Hi Ping,
>
> On Jun 13 2014 or thereabouts, Ping Cheng wrote:
> > Hi Benjamin,
> >
> > On Fri, Jun 13, 2014 at 1:29 PM, Benjamin Tissoires
> >  wrote:
> > > This solves a bug with the wireless receiver:
> >
> > Your patch does get rid of the crash. But, it does not fix it at the
> > root cause.
>
> True, it fixes the crash, does not get rid of the root cause, but it is
> still required. See later.
>
> >
> > > - at plug, the wireless receiver does not know which Wacom device it is
> > >   connected to, so it does not actually creates all the LEDs
> >
> > This is the root cause - LEDs are not created for wireless devices.
> > Neither here, nor later when a real device is connected.
>
> Yep
>
> >
> > > - when the tablet connects, wacom->wacom_wac.features.type is set to the
> > >   proper device so that wacom_wac can understand the packets
> >
> > LEDs are not created for any wireless devices since we don't call
> > wacom_initialize_leds() when real tablets are connected.
>
> Yep
>
> >
> > > - when the receiver is unplugged, it detects that a LED should have been
> > >   created (based on wacom->wacom_wac.features.type) and tries to remove
> > >   it: crash when removing the sysfs group.
> >
> > When receiver is unplugged, it remembers the last tablet that
> > connected to it. If that tablet supports LEDs, wacom_destroy_leds() is
> > called. But, no LEDs were initialized. That's why it crashes.
>
> Yep
>
> >
> > > Side effect, we can now safely call several times wacom_destroy_leds().
> >
> > led_initialized will never be true if we keep wacom_initialize_leds()
> > inside probe().
>
> If you are talking about wireless devices only, yes, this value will
> never be true. That's the purpose of this patch actually :)
>
> >
> > To make initialize_leds() and desctroy_leds() work for wireless
> > devices, we need to move them to wacom_wireless_work() where we know
> > what type of tablet is connected/disconnected.
>
> Unfortunately, this does not work:
> - we *can* create the LEDs sysfs in the wireless work
> - this will prevent the crash
> - the user will think it can control the LEDs
> - actually these LEDs control will do nothing because LEDs handling for
>   wireless devices goes through the WL interface, and not the PEN
>   interface of the WL receiver.
> - we need to implement a specific led_handling for the wireless receiver
>   (which will need to know which type of tablet is connected to it)
> - we still need a way to say that the pen intf which is declared as the
>   connected device does not has a LED sysfs.
>
> We could add a quirk to the wacom_wac->features saying that the
> connection is wireless, so there is no LED attached to the interface.
>
> Still, there will be some work to do to properly handle the LED
> configuration from the WL receiver. This work has to be done in the
> kernel, but also in the user space (g-s-d) because now, the led control
> will not be on the pen device, but on a plain usb device without input.
>
> If you prefer, I can add such a quirk. But my only concern is here to
> fix the kernel oops, not to add features which would require more
> testing across different hardware and development on the user space
> side.
>
> >
> > > Signed-off-by: Benjamin Tissoires 
> >
> > Thank you for your support. But, sorry
> >
> > NAKed-by: Ping Cheng 
>
> Please reconsider it or validate the quirk approach I mentioned.


I see your point. My concern was once we fixed the crash here, we
would forget to fix the actual problem.

For the time being, let's make sure we can move on. So,

Acked-by: Ping Cheng .

Cheers,

Ping
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Input - wacom: put a flag when the led are initialized

[Ping Cheng]

Hi Benjamin,

On Monday, June 16, 2014, Benjamin Tissoires
benjamin.tissoi...@redhat.com wrote:

 Hi Ping,

 On Jun 13 2014 or thereabouts, Ping Cheng wrote:
  Hi Benjamin,
 
  On Fri, Jun 13, 2014 at 1:29 PM, Benjamin Tissoires
  benjamin.tissoi...@redhat.com wrote:
   This solves a bug with the wireless receiver:
 
  Your patch does get rid of the crash. But, it does not fix it at the
  root cause.

 True, it fixes the crash, does not get rid of the root cause, but it is
 still required. See later.

 
   - at plug, the wireless receiver does not know which Wacom device it is
 connected to, so it does not actually creates all the LEDs
 
  This is the root cause - LEDs are not created for wireless devices.
  Neither here, nor later when a real device is connected.

 Yep

 
   - when the tablet connects, wacom-wacom_wac.features.type is set to the
 proper device so that wacom_wac can understand the packets
 
  LEDs are not created for any wireless devices since we don't call
  wacom_initialize_leds() when real tablets are connected.

 Yep

 
   - when the receiver is unplugged, it detects that a LED should have been
 created (based on wacom-wacom_wac.features.type) and tries to remove
 it: crash when removing the sysfs group.
 
  When receiver is unplugged, it remembers the last tablet that
  connected to it. If that tablet supports LEDs, wacom_destroy_leds() is
  called. But, no LEDs were initialized. That's why it crashes.

 Yep

 
   Side effect, we can now safely call several times wacom_destroy_leds().
 
  led_initialized will never be true if we keep wacom_initialize_leds()
  inside probe().

 If you are talking about wireless devices only, yes, this value will
 never be true. That's the purpose of this patch actually :)

 
  To make initialize_leds() and desctroy_leds() work for wireless
  devices, we need to move them to wacom_wireless_work() where we know
  what type of tablet is connected/disconnected.

 Unfortunately, this does not work:
 - we *can* create the LEDs sysfs in the wireless work
 - this will prevent the crash
 - the user will think it can control the LEDs
 - actually these LEDs control will do nothing because LEDs handling for
   wireless devices goes through the WL interface, and not the PEN
   interface of the WL receiver.
 - we need to implement a specific led_handling for the wireless receiver
   (which will need to know which type of tablet is connected to it)
 - we still need a way to say that the pen intf which is declared as the
   connected device does not has a LED sysfs.

 We could add a quirk to the wacom_wac-features saying that the
 connection is wireless, so there is no LED attached to the interface.

 Still, there will be some work to do to properly handle the LED
 configuration from the WL receiver. This work has to be done in the
 kernel, but also in the user space (g-s-d) because now, the led control
 will not be on the pen device, but on a plain usb device without input.

 If you prefer, I can add such a quirk. But my only concern is here to
 fix the kernel oops, not to add features which would require more
 testing across different hardware and development on the user space
 side.

 
   Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com
 
  Thank you for your support. But, sorry
 
  NAKed-by: Ping Cheng pi...@wacom.com

 Please reconsider it or validate the quirk approach I mentioned.


I see your point. My concern was once we fixed the crash here, we
would forget to fix the actual problem.

For the time being, let's make sure we can move on. So,

Acked-by: Ping Cheng pi...@wacom.com.

Cheers,

Ping
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Input - wacom: put a flag when the led are initialized

[Ping Cheng]

Hi Benjamin,

On Fri, Jun 13, 2014 at 1:29 PM, Benjamin Tissoires
 wrote:
> This solves a bug with the wireless receiver:

Your patch does get rid of the crash. But, it does not fix it at the
root cause.

> - at plug, the wireless receiver does not know which Wacom device it is
>   connected to, so it does not actually creates all the LEDs

This is the root cause - LEDs are not created for wireless devices.
Neither here, nor later when a real device is connected.

> - when the tablet connects, wacom->wacom_wac.features.type is set to the
>   proper device so that wacom_wac can understand the packets

LEDs are not created for any wireless devices since we don't call
wacom_initialize_leds() when real tablets are connected.

> - when the receiver is unplugged, it detects that a LED should have been
>   created (based on wacom->wacom_wac.features.type) and tries to remove
>   it: crash when removing the sysfs group.

When receiver is unplugged, it remembers the last tablet that
connected to it. If that tablet supports LEDs, wacom_destroy_leds() is
called. But, no LEDs were initialized. That's why it crashes.

> Side effect, we can now safely call several times wacom_destroy_leds().

led_initialized will never be true if we keep wacom_initialize_leds()
inside probe().

To make initialize_leds() and desctroy_leds() work for wireless
devices, we need to move them to wacom_wireless_work() where we know
what type of tablet is connected/disconnected.

> Signed-off-by: Benjamin Tissoires 

Thank you for your support. But, sorry

NAKed-by: Ping Cheng 

Ping

> ---
>  drivers/input/tablet/wacom.h | 1 +
>  drivers/input/tablet/wacom_sys.c | 6 ++
>  2 files changed, 7 insertions(+)
>
> diff --git a/drivers/input/tablet/wacom.h b/drivers/input/tablet/wacom.h
> index 70b1e71..f13ad31 100644
> --- a/drivers/input/tablet/wacom.h
> +++ b/drivers/input/tablet/wacom.h
> @@ -120,6 +120,7 @@ struct wacom {
> u8 hlv;   /* status led brightness button pressed 
> (1..127) */
> u8 img_lum;   /* OLED matrix display brightness */
> } led;
> +   bool led_initialized;
> struct power_supply battery;
>  };
>
> diff --git a/drivers/input/tablet/wacom_sys.c 
> b/drivers/input/tablet/wacom_sys.c
> index 94096fd..7087b33 100644
> --- a/drivers/input/tablet/wacom_sys.c
> +++ b/drivers/input/tablet/wacom_sys.c
> @@ -1016,12 +1016,18 @@ static int wacom_initialize_leds(struct wacom *wacom)
> return error;
> }
> wacom_led_control(wacom);
> +   wacom->led_initialized = true;
>
> return 0;
>  }
>
>  static void wacom_destroy_leds(struct wacom *wacom)
>  {
> +   if (!wacom->led_initialized)
> +   return;
> +
> +   wacom->led_initialized = false;
> +
> switch (wacom->wacom_wac.features.type) {
> case INTUOS4S:
> case INTUOS4:
> --
> 1.9.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Input - wacom: put a flag when the led are initialized

[Ping Cheng]

Hi Benjamin,

On Fri, Jun 13, 2014 at 1:29 PM, Benjamin Tissoires
benjamin.tissoi...@redhat.com wrote:
 This solves a bug with the wireless receiver:

Your patch does get rid of the crash. But, it does not fix it at the
root cause.

 - at plug, the wireless receiver does not know which Wacom device it is
   connected to, so it does not actually creates all the LEDs

This is the root cause - LEDs are not created for wireless devices.
Neither here, nor later when a real device is connected.

 - when the tablet connects, wacom-wacom_wac.features.type is set to the
   proper device so that wacom_wac can understand the packets

LEDs are not created for any wireless devices since we don't call
wacom_initialize_leds() when real tablets are connected.

 - when the receiver is unplugged, it detects that a LED should have been
   created (based on wacom-wacom_wac.features.type) and tries to remove
   it: crash when removing the sysfs group.

When receiver is unplugged, it remembers the last tablet that
connected to it. If that tablet supports LEDs, wacom_destroy_leds() is
called. But, no LEDs were initialized. That's why it crashes.

 Side effect, we can now safely call several times wacom_destroy_leds().

led_initialized will never be true if we keep wacom_initialize_leds()
inside probe().

To make initialize_leds() and desctroy_leds() work for wireless
devices, we need to move them to wacom_wireless_work() where we know
what type of tablet is connected/disconnected.

 Signed-off-by: Benjamin Tissoires benjamin.tissoi...@redhat.com

Thank you for your support. But, sorry

NAKed-by: Ping Cheng pi...@wacom.com

Ping

 ---
  drivers/input/tablet/wacom.h | 1 +
  drivers/input/tablet/wacom_sys.c | 6 ++
  2 files changed, 7 insertions(+)

 diff --git a/drivers/input/tablet/wacom.h b/drivers/input/tablet/wacom.h
 index 70b1e71..f13ad31 100644
 --- a/drivers/input/tablet/wacom.h
 +++ b/drivers/input/tablet/wacom.h
 @@ -120,6 +120,7 @@ struct wacom {
 u8 hlv;   /* status led brightness button pressed 
 (1..127) */
 u8 img_lum;   /* OLED matrix display brightness */
 } led;
 +   bool led_initialized;
 struct power_supply battery;
  };

 diff --git a/drivers/input/tablet/wacom_sys.c 
 b/drivers/input/tablet/wacom_sys.c
 index 94096fd..7087b33 100644
 --- a/drivers/input/tablet/wacom_sys.c
 +++ b/drivers/input/tablet/wacom_sys.c
 @@ -1016,12 +1016,18 @@ static int wacom_initialize_leds(struct wacom *wacom)
 return error;
 }
 wacom_led_control(wacom);
 +   wacom-led_initialized = true;

 return 0;
  }

  static void wacom_destroy_leds(struct wacom *wacom)
  {
 +   if (!wacom-led_initialized)
 +   return;
 +
 +   wacom-led_initialized = false;
 +
 switch (wacom-wacom_wac.features.type) {
 case INTUOS4S:
 case INTUOS4:
 --
 1.9.0

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: wacom: Fixes for stylus pressure values for Thinkpad Yoga

[Ping Cheng]

Hi Carl,

Thank you for the heads up. I believe Jason's patchset 4 of 4
(http://www.spinics.net/lists/linux-input/msg29435.html) fixed the
issue for your device and for other's. The patch was submitted last
month. If you can test the set on your device and give us a Tested-by
here, it will help Dmitry to merge the patch upstream.

Thank you for your effort.

Ping

On Wed, Feb 26, 2014 at 2:38 PM, Carl Worth  wrote:
> This series of patches fixes the pressure values reported for the
> wacom tablet built-in to a Lenovo ThinkPad Yoga laptop. Prior to this
> patch series, if I slowly increased stylus pressure, (expecting a
> gradual increase of values from 0 to 1023), I instead received values
> that increased slowly to 255, then reset to 0 and increased slowly
> again, etc.
>
> The buggy arithmetic that is updated here appears to exist in
> identical forms for other drivers. I did not update any code that I
> was not able to test directly. But it looks like wacom_pl_irq and
> wacom_dtu_irq potentially have similar bugs, (depending on the actual
> pressure_max values encountered in practice).
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: wacom: Fixes for stylus pressure values for Thinkpad Yoga

[Ping Cheng]

Hi Carl,

Thank you for the heads up. I believe Jason's patchset 4 of 4
(http://www.spinics.net/lists/linux-input/msg29435.html) fixed the
issue for your device and for other's. The patch was submitted last
month. If you can test the set on your device and give us a Tested-by
here, it will help Dmitry to merge the patch upstream.

Thank you for your effort.

Ping

On Wed, Feb 26, 2014 at 2:38 PM, Carl Worth cwo...@cworth.org wrote:
 This series of patches fixes the pressure values reported for the
 wacom tablet built-in to a Lenovo ThinkPad Yoga laptop. Prior to this
 patch series, if I slowly increased stylus pressure, (expecting a
 gradual increase of values from 0 to 1023), I instead received values
 that increased slowly to 255, then reset to 0 and increased slowly
 again, etc.

 The buggy arithmetic that is updated here appears to exist in
 identical forms for other drivers. I did not update any code that I
 was not able to test directly. But it looks like wacom_pl_irq and
 wacom_dtu_irq potentially have similar bugs, (depending on the actual
 pressure_max values encountered in practice).

 --
 To unsubscribe from this list: send the line unsubscribe linux-input in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] input - input.h: Add a new switch event

[Ping Cheng]

One of Wacom's pen and touch capable models added a switch for
users to turn on/off touch events. We need to report the state of
this switch to userland. But, there is no existing switch event
defined for this purpose. Luckily enough, there is a room for a
new switch event.

Signed-off-by: Ping Cheng 
---
 include/uapi/linux/input.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index d08abf9..d4097b0 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -855,6 +855,7 @@ struct input_keymap_entry {
 #define SW_FRONT_PROXIMITY 0x0b  /* set = front proximity sensor active */
 #define SW_ROTATE_LOCK 0x0c  /* set = rotate locked/disabled */
 #define SW_LINEIN_INSERT   0x0d  /* set = inserted */
+#define SW_TOUCH_ENABLED   0x0e  /* set = touch switch turned on (touch 
events off) */
 #define SW_MAX 0x0f
 #define SW_CNT (SW_MAX+1)
 
-- 
1.8.1.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] input - input.h: Add a new switch event

[Ping Cheng]

One of Wacom's pen and touch capable models added a switch for
users to turn on/off touch events. We need to report the state of
this switch to userland. But, there is no existing switch event
defined for this purpose. Luckily enough, there is a room for a
new switch event.

Signed-off-by: Ping Cheng pi...@wacom.com
---
 include/uapi/linux/input.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index d08abf9..d4097b0 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -855,6 +855,7 @@ struct input_keymap_entry {
 #define SW_FRONT_PROXIMITY 0x0b  /* set = front proximity sensor active */
 #define SW_ROTATE_LOCK 0x0c  /* set = rotate locked/disabled */
 #define SW_LINEIN_INSERT   0x0d  /* set = inserted */
+#define SW_TOUCH_ENABLED   0x0e  /* set = touch switch turned on (touch 
events off) */
 #define SW_MAX 0x0f
 #define SW_CNT (SW_MAX+1)
 
-- 
1.8.1.2

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 16/20] HID: Only dump input if someone is listening

[Ping Cheng]

On Thu, Sep 6, 2012 at 1:57 PM, Henrik Rydberg  wrote:
>> Let's wait once Dmitry is finished with the input changes, and I can
>> either take the set through my tree with his Signed-off-by: on the Input
>> patches, or vice-versa.
>
> Ok. Meanwhile, the current set is now in a temporary branch on github,
> see below. Given there are no more change requests, I can add the
> right SOBs and push it to my next tree for a while, before sending a
> proper pull request to either of you.
>
> Thanks,
> Henrik
>
> The following changes since commit 4cbe5a555fa58a79b6ecbb6c531b8bab0650778d:
>
>   Linux 3.6-rc4 (2012-09-01 10:39:58 -0700)
>
> are available in the git repository at:
>
>   git://github.com/rydberg/linux.git maybe

This branch is tested with a Wacom Bamboo3 and an Intuos 5 M.

Tested-by: Ping Cheng 

Ping

> for you to fetch changes up to 631ea780695fdc0d1efa91b6714bd85369daab06:
>
>   HID: hid-multitouch: Add Flatfrog support (2012-09-06 22:21:02 +0200)
>
> 
> Henrik Rydberg (21):
>   Input: Break out MT data
>   Input: Improve the events-per-packet estimate
>   Input: Make sure we follow all EV_KEY events
>   Input: Move autorepeat to the event-passing phase
>   Input: Send events one packet at a time
>   Input: evdev - Add the events() callback
>   Input: MT - Add flags to input_mt_init_slots()
>   Input: MT - Handle frame synchronization in core
>   Input: MT - Add in-kernel tracking
>   Input: MT - Get slot by key
>   Input: bcm5974 - only setup button urb for TYPE1 devices
>   Input: bcm5974 - Preparatory renames
>   Input: bcm5974 - Drop pressure and width emulation
>   Input: bcm5974 - Drop the logical dimensions
>   Input: bcm5974 - Convert to MT-B
>   HID: Add an input configured notification callback
>   HID: hid-multitouch: Simplify setup and frame synchronization
>   HID: hid-multitouch: Remove the redundant touch state
>   HID: hid-multitouch: Fix contact count on 3M panels
>   HID: Allow more fields in the hid report
>   HID: hid-multitouch: Add Flatfrog support
>
>  drivers/hid/hid-ids.h|   3 +
>  drivers/hid/hid-input.c  |  11 +-
>  drivers/hid/hid-magicmouse.c |   2 +-
>  drivers/hid/hid-multitouch.c | 192 
> +++-
>  drivers/input/evdev.c|  78 -
>  drivers/input/input-mt.c | 294 
> +
>  drivers/input/input.c| 245 
> +++--
>  drivers/input/misc/uinput.c  |   2 +-
>  drivers/input/mouse/alps.c   |   2 +-
>  drivers/input/mouse/bcm5974.c| 317 
> -
>  drivers/input/mouse/elantech.c   |   4 +-
>  drivers/input/mouse/sentelic.c   |   2 +-
>  drivers/input/mouse/synaptics.c  |   4 +-
>  drivers/input/tablet/wacom_wac.c |   6 +-
>  drivers/input/touchscreen/atmel_mxt_ts.c |   2 +-
>  drivers/input/touchscreen/cyttsp_core.c  |   2 +-
>  drivers/input/touchscreen/edt-ft5x06.c   |   2 +-
>  drivers/input/touchscreen/egalax_ts.c|   2 +-
>  drivers/input/touchscreen/ili210x.c  |   2 +-
>  drivers/input/touchscreen/mms114.c   |   2 +-
>  drivers/input/touchscreen/penmount.c |   2 +-
>  drivers/input/touchscreen/wacom_w8001.c  |   2 +-
>  include/linux/hid.h  |   5 +-
>  include/linux/input.h|  33 --
>  include/linux/input/mt.h |  55 +-
>  25 files changed, 807 insertions(+), 464 deletions(-)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 16/20] HID: Only dump input if someone is listening

[Ping Cheng]

On Thu, Sep 6, 2012 at 1:57 PM, Henrik Rydberg rydb...@euromail.se wrote:
 Let's wait once Dmitry is finished with the input changes, and I can
 either take the set through my tree with his Signed-off-by: on the Input
 patches, or vice-versa.

 Ok. Meanwhile, the current set is now in a temporary branch on github,
 see below. Given there are no more change requests, I can add the
 right SOBs and push it to my next tree for a while, before sending a
 proper pull request to either of you.

 Thanks,
 Henrik

 The following changes since commit 4cbe5a555fa58a79b6ecbb6c531b8bab0650778d:

   Linux 3.6-rc4 (2012-09-01 10:39:58 -0700)

 are available in the git repository at:

   git://github.com/rydberg/linux.git maybe

This branch is tested with a Wacom Bamboo3 and an Intuos 5 M.

Tested-by: Ping Cheng pi...@wacom.com

Ping

 for you to fetch changes up to 631ea780695fdc0d1efa91b6714bd85369daab06:

   HID: hid-multitouch: Add Flatfrog support (2012-09-06 22:21:02 +0200)

 
 Henrik Rydberg (21):
   Input: Break out MT data
   Input: Improve the events-per-packet estimate
   Input: Make sure we follow all EV_KEY events
   Input: Move autorepeat to the event-passing phase
   Input: Send events one packet at a time
   Input: evdev - Add the events() callback
   Input: MT - Add flags to input_mt_init_slots()
   Input: MT - Handle frame synchronization in core
   Input: MT - Add in-kernel tracking
   Input: MT - Get slot by key
   Input: bcm5974 - only setup button urb for TYPE1 devices
   Input: bcm5974 - Preparatory renames
   Input: bcm5974 - Drop pressure and width emulation
   Input: bcm5974 - Drop the logical dimensions
   Input: bcm5974 - Convert to MT-B
   HID: Add an input configured notification callback
   HID: hid-multitouch: Simplify setup and frame synchronization
   HID: hid-multitouch: Remove the redundant touch state
   HID: hid-multitouch: Fix contact count on 3M panels
   HID: Allow more fields in the hid report
   HID: hid-multitouch: Add Flatfrog support

  drivers/hid/hid-ids.h|   3 +
  drivers/hid/hid-input.c  |  11 +-
  drivers/hid/hid-magicmouse.c |   2 +-
  drivers/hid/hid-multitouch.c | 192 
 +++-
  drivers/input/evdev.c|  78 -
  drivers/input/input-mt.c | 294 
 +
  drivers/input/input.c| 245 
 +++--
  drivers/input/misc/uinput.c  |   2 +-
  drivers/input/mouse/alps.c   |   2 +-
  drivers/input/mouse/bcm5974.c| 317 
 -
  drivers/input/mouse/elantech.c   |   4 +-
  drivers/input/mouse/sentelic.c   |   2 +-
  drivers/input/mouse/synaptics.c  |   4 +-
  drivers/input/tablet/wacom_wac.c |   6 +-
  drivers/input/touchscreen/atmel_mxt_ts.c |   2 +-
  drivers/input/touchscreen/cyttsp_core.c  |   2 +-
  drivers/input/touchscreen/edt-ft5x06.c   |   2 +-
  drivers/input/touchscreen/egalax_ts.c|   2 +-
  drivers/input/touchscreen/ili210x.c  |   2 +-
  drivers/input/touchscreen/mms114.c   |   2 +-
  drivers/input/touchscreen/penmount.c |   2 +-
  drivers/input/touchscreen/wacom_w8001.c  |   2 +-
  include/linux/hid.h  |   5 +-
  include/linux/input.h|  33 --
  include/linux/input/mt.h |  55 +-
  25 files changed, 807 insertions(+), 464 deletions(-)
 --
 To unsubscribe from this list: send the line unsubscribe linux-input in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 09/19] Input: MT - Handle frame synchronization in core

[Ping Cheng]

On Thu, Aug 16, 2012 at 11:07 AM, Henrik Rydberg  wrote:
> On Wed, Aug 15, 2012 at 04:28:17PM -0700, Ping Cheng wrote:
>> On Sun, Aug 12, 2012 at 2:42 PM, Henrik Rydberg  wrote:
>> > Collect common frame synchronization tasks in a new function,
>> > input_mt_sync_frame(). Depending on the flags set, it drops
>> > unseen contacts and performs pointer emulation.
>> >
>> > Signed-off-by: Henrik Rydberg 
>>
>> I went through the patchset except those for bcm5974. Since there are
>> changes that affect other drivers, do you plan to update the affected
>> drivers as well?
>
> I am not sure what you mean here? There is a patch with in-kernel api
> changes, which also changes all drivers using the api. Some of those
> drivers will benefit from further changes, but that is a different
> story.

Just to clarify a little bit more. Patch 08/19 was only a generic
update. A complete update should introduce INPUT_MT_POINTER and
INPUT_MT_DIRECT so the drivers can fully utilize the new feature.

Now I understand you expect us to update the "different story".

Thanks.

Ping

Ping
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 09/19] Input: MT - Handle frame synchronization in core

[Ping Cheng]

On Thu, Aug 16, 2012 at 11:07 AM, Henrik Rydberg  wrote:
> On Wed, Aug 15, 2012 at 04:28:17PM -0700, Ping Cheng wrote:
>> On Sun, Aug 12, 2012 at 2:42 PM, Henrik Rydberg  wrote:
>> > Collect common frame synchronization tasks in a new function,
>> > input_mt_sync_frame(). Depending on the flags set, it drops
>> > unseen contacts and performs pointer emulation.
>> >
>> > Signed-off-by: Henrik Rydberg 
>>
>> I went through the patchset except those for bcm5974. Since there are
>> changes that affect other drivers, do you plan to update the affected
>> drivers as well?
>
> I am not sure what you mean here? There is a patch with in-kernel api
> changes, which also changes all drivers using the api. Some of those
> drivers will benefit from further changes, but that is a different
> story.

I meant that some new routines require individual MT drivers to be
updated to adapt to the new implementation.

For example, the new input_mt_init_slots() takes care of the
__set_bit() functions in one place. That is great. But, it requires
wacom_wac.c to be updated since it has an interface change. I guess
there are other drivers calling input_mt_init_slots as well.

I was wondering if you plan to update all drivers after this patchset
is accepted or if you need us to chime in.


>> > +void input_mt_sync_frame(struct input_dev *dev)
>> > +{
>> > +   struct input_mt *mt = dev->mt;
>> > +   struct input_mt_slot *s;
>> > +
>> > +   if (!mt)
>> > +   return;
>> > +
>> > +   if (mt->flags & INPUT_MT_DROP_UNUSED) {
>> > +   for (s = mt->slots; s != mt->slots + mt->num_slots; s++) {
>> > +   if (s->frame == mt->frame)
>> > +   continue;
>> > +   input_mt_slot(dev, s - mt->slots);
>> > +   input_event(dev, EV_ABS, ABS_MT_TRACKING_ID, -1);
>> > +   }
>> > +   }
>> > +
>> > +   if (mt->flags & INPUT_MT_POINTER)
>> > +   input_mt_report_pointer_emulation(dev, true);
>> > +
>> > +   if (mt->flags & INPUT_MT_DIRECT)
>> > +   input_mt_report_pointer_emulation(dev, false);
>> > +
>> > +   mt->frame++;
>>
>> Where do we reset this frame counter?
>
> Why would we reset it? It is used in the core to keep track of changes
> per frame, and may wrap around without issues.

>From what I see, frame/mt is only initialized when driver starts.
Frame will be increased by MT events while the driver running. If this
is true, won't it be possible the value gets too large?

I might have missed a detail about frame somewhere in the patchset.

Ping
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 09/19] Input: MT - Handle frame synchronization in core

[Ping Cheng]

On Thu, Aug 16, 2012 at 11:07 AM, Henrik Rydberg rydb...@euromail.se wrote:
 On Wed, Aug 15, 2012 at 04:28:17PM -0700, Ping Cheng wrote:
 On Sun, Aug 12, 2012 at 2:42 PM, Henrik Rydberg rydb...@euromail.se wrote:
  Collect common frame synchronization tasks in a new function,
  input_mt_sync_frame(). Depending on the flags set, it drops
  unseen contacts and performs pointer emulation.
 
  Signed-off-by: Henrik Rydberg rydb...@euromail.se

 I went through the patchset except those for bcm5974. Since there are
 changes that affect other drivers, do you plan to update the affected
 drivers as well?

 I am not sure what you mean here? There is a patch with in-kernel api
 changes, which also changes all drivers using the api. Some of those
 drivers will benefit from further changes, but that is a different
 story.

I meant that some new routines require individual MT drivers to be
updated to adapt to the new implementation.

For example, the new input_mt_init_slots() takes care of the
__set_bit() functions in one place. That is great. But, it requires
wacom_wac.c to be updated since it has an interface change. I guess
there are other drivers calling input_mt_init_slots as well.

I was wondering if you plan to update all drivers after this patchset
is accepted or if you need us to chime in.


  +void input_mt_sync_frame(struct input_dev *dev)
  +{
  +   struct input_mt *mt = dev-mt;
  +   struct input_mt_slot *s;
  +
  +   if (!mt)
  +   return;
  +
  +   if (mt-flags  INPUT_MT_DROP_UNUSED) {
  +   for (s = mt-slots; s != mt-slots + mt-num_slots; s++) {
  +   if (s-frame == mt-frame)
  +   continue;
  +   input_mt_slot(dev, s - mt-slots);
  +   input_event(dev, EV_ABS, ABS_MT_TRACKING_ID, -1);
  +   }
  +   }
  +
  +   if (mt-flags  INPUT_MT_POINTER)
  +   input_mt_report_pointer_emulation(dev, true);
  +
  +   if (mt-flags  INPUT_MT_DIRECT)
  +   input_mt_report_pointer_emulation(dev, false);
  +
  +   mt-frame++;

 Where do we reset this frame counter?

 Why would we reset it? It is used in the core to keep track of changes
 per frame, and may wrap around without issues.

From what I see, frame/mt is only initialized when driver starts.
Frame will be increased by MT events while the driver running. If this
is true, won't it be possible the value gets too large?

I might have missed a detail about frame somewhere in the patchset.

Ping
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 09/19] Input: MT - Handle frame synchronization in core

[Ping Cheng]

On Thu, Aug 16, 2012 at 11:07 AM, Henrik Rydberg rydb...@euromail.se wrote:
 On Wed, Aug 15, 2012 at 04:28:17PM -0700, Ping Cheng wrote:
 On Sun, Aug 12, 2012 at 2:42 PM, Henrik Rydberg rydb...@euromail.se wrote:
  Collect common frame synchronization tasks in a new function,
  input_mt_sync_frame(). Depending on the flags set, it drops
  unseen contacts and performs pointer emulation.
 
  Signed-off-by: Henrik Rydberg rydb...@euromail.se

 I went through the patchset except those for bcm5974. Since there are
 changes that affect other drivers, do you plan to update the affected
 drivers as well?

 I am not sure what you mean here? There is a patch with in-kernel api
 changes, which also changes all drivers using the api. Some of those
 drivers will benefit from further changes, but that is a different
 story.

Just to clarify a little bit more. Patch 08/19 was only a generic
update. A complete update should introduce INPUT_MT_POINTER and
INPUT_MT_DIRECT so the drivers can fully utilize the new feature.

Now I understand you expect us to update the different story.

Thanks.

Ping

Ping
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 09/19] Input: MT - Handle frame synchronization in core

[Ping Cheng]

On Sun, Aug 12, 2012 at 2:42 PM, Henrik Rydberg  wrote:
> Collect common frame synchronization tasks in a new function,
> input_mt_sync_frame(). Depending on the flags set, it drops
> unseen contacts and performs pointer emulation.
>
> Signed-off-by: Henrik Rydberg 

I went through the patchset except those for bcm5974. Since there are
changes that affect other drivers, do you plan to update the affected
drivers as well?

I have a minor question below inline. You can add my Reviewed-by tag
after updating commit comments for 02/19.

Thank you for your effort.

Ping

> ---
>  drivers/input/input-mt.c | 74 
> ++--
>  include/linux/input/mt.h |  9 ++
>  2 files changed, 81 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
> index bbb3304..21b105e 100644
> --- a/drivers/input/input-mt.c
> +++ b/drivers/input/input-mt.c
> @@ -14,6 +14,14 @@
>
>  #define TRKID_SGN  ((TRKID_MAX + 1) >> 1)
>
> +static void copy_abs(struct input_dev *dev, unsigned int dst, unsigned int 
> src)
> +{
> +   if (dev->absinfo && test_bit(src, dev->absbit)) {
> +   dev->absinfo[dst] = dev->absinfo[src];
> +   dev->absbit[BIT_WORD(dst)] |= BIT_MASK(dst);
> +   }
> +}
> +
>  /**
>   * input_mt_init_slots() - initialize MT input slots
>   * @dev: input device supporting MT events and finger tracking
> @@ -45,6 +53,28 @@ int input_mt_init_slots(struct input_dev *dev, unsigned 
> int num_slots,
> input_set_abs_params(dev, ABS_MT_SLOT, 0, num_slots - 1, 0, 0);
> input_set_abs_params(dev, ABS_MT_TRACKING_ID, 0, TRKID_MAX, 0, 0);
>
> +   if (flags & (INPUT_MT_POINTER | INPUT_MT_DIRECT)) {
> +   __set_bit(EV_KEY, dev->evbit);
> +   __set_bit(BTN_TOUCH, dev->keybit);
> +
> +   copy_abs(dev, ABS_X, ABS_MT_POSITION_X);
> +   copy_abs(dev, ABS_Y, ABS_MT_POSITION_Y);
> +   copy_abs(dev, ABS_PRESSURE, ABS_MT_PRESSURE);
> +   }
> +   if (flags & INPUT_MT_POINTER) {
> +   __set_bit(BTN_TOOL_FINGER, dev->keybit);
> +   __set_bit(BTN_TOOL_DOUBLETAP, dev->keybit);
> +   if (num_slots >= 3)
> +   __set_bit(BTN_TOOL_TRIPLETAP, dev->keybit);
> +   if (num_slots >= 4)
> +   __set_bit(BTN_TOOL_QUADTAP, dev->keybit);
> +   if (num_slots >= 5)
> +   __set_bit(BTN_TOOL_QUINTTAP, dev->keybit);
> +   __set_bit(INPUT_PROP_POINTER, dev->propbit);
> +   }
> +   if (flags & INPUT_MT_DIRECT)
> +   __set_bit(INPUT_PROP_DIRECT, dev->propbit);
> +
> /* Mark slots as 'unused' */
> for (i = 0; i < num_slots; i++)
> input_mt_set_value(>slots[i], ABS_MT_TRACKING_ID, -1);
> @@ -87,12 +117,17 @@ void input_mt_report_slot_state(struct input_dev *dev,
> struct input_mt_slot *slot;
> int id;
>
> -   if (!mt || !active) {
> +   if (!mt)
> +   return;
> +
> +   slot = >slots[input_abs_get_val(dev, ABS_MT_SLOT)];
> +   slot->frame = mt->frame;
> +
> +   if (!active) {
> input_event(dev, EV_ABS, ABS_MT_TRACKING_ID, -1);
> return;
> }
>
> -   slot = >slots[input_abs_get_val(dev, ABS_MT_SLOT)];
> id = input_mt_get_value(slot, ABS_MT_TRACKING_ID);
> if (id < 0 || input_mt_get_value(slot, ABS_MT_TOOL_TYPE) != tool_type)
> id = input_mt_new_trkid(mt);
> @@ -177,3 +212,38 @@ void input_mt_report_pointer_emulation(struct input_dev 
> *dev, bool use_count)
> }
>  }
>  EXPORT_SYMBOL(input_mt_report_pointer_emulation);
> +
> +/**
> + * input_mt_sync_frame() - synchronize mt frame
> + * @dev: input device with allocated MT slots
> + *
> + * Close the frame and prepare the internal state for a new one.
> + * Depending on the flags, marks unused slots as inactive and performs
> + * pointer emulation.
> + */
> +void input_mt_sync_frame(struct input_dev *dev)
> +{
> +   struct input_mt *mt = dev->mt;
> +   struct input_mt_slot *s;
> +
> +   if (!mt)
> +   return;
> +
> +   if (mt->flags & INPUT_MT_DROP_UNUSED) {
> +   for (s = mt->slots; s != mt->slots + mt->num_slots; s++) {
> +   if (s->frame == mt->frame)
> +   continue;
> +   input_mt_slot(dev, s - mt->slots);
> +   input_event(dev, EV_ABS, ABS_MT_TRACKING_ID, -1);
> +   }
> +   }
> +
> +   if (mt->flags & INPUT_MT_POINTER)
> +   input_mt_report_pointer_emulation(dev, true);
> +
> +   if (mt->flags & INPUT_MT_DIRECT)
> +   input_mt_report_pointer_emulation(dev, false);
> +
> +   mt->frame++;

Where do we reset this frame counter?

> +}
> +EXPORT_SYMBOL(input_mt_sync_frame);
> diff --git a/include/linux/input/mt.h b/include/linux/input/mt.h
> 

Re: [PATCH 09/19] Input: MT - Handle frame synchronization in core

[Ping Cheng]

On Sun, Aug 12, 2012 at 2:42 PM, Henrik Rydberg rydb...@euromail.se wrote:
 Collect common frame synchronization tasks in a new function,
 input_mt_sync_frame(). Depending on the flags set, it drops
 unseen contacts and performs pointer emulation.

 Signed-off-by: Henrik Rydberg rydb...@euromail.se

I went through the patchset except those for bcm5974. Since there are
changes that affect other drivers, do you plan to update the affected
drivers as well?

I have a minor question below inline. You can add my Reviewed-by tag
after updating commit comments for 02/19.

Thank you for your effort.

Ping

 ---
  drivers/input/input-mt.c | 74 
 ++--
  include/linux/input/mt.h |  9 ++
  2 files changed, 81 insertions(+), 2 deletions(-)

 diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
 index bbb3304..21b105e 100644
 --- a/drivers/input/input-mt.c
 +++ b/drivers/input/input-mt.c
 @@ -14,6 +14,14 @@

  #define TRKID_SGN  ((TRKID_MAX + 1)  1)

 +static void copy_abs(struct input_dev *dev, unsigned int dst, unsigned int 
 src)
 +{
 +   if (dev-absinfo  test_bit(src, dev-absbit)) {
 +   dev-absinfo[dst] = dev-absinfo[src];
 +   dev-absbit[BIT_WORD(dst)] |= BIT_MASK(dst);
 +   }
 +}
 +
  /**
   * input_mt_init_slots() - initialize MT input slots
   * @dev: input device supporting MT events and finger tracking
 @@ -45,6 +53,28 @@ int input_mt_init_slots(struct input_dev *dev, unsigned 
 int num_slots,
 input_set_abs_params(dev, ABS_MT_SLOT, 0, num_slots - 1, 0, 0);
 input_set_abs_params(dev, ABS_MT_TRACKING_ID, 0, TRKID_MAX, 0, 0);

 +   if (flags  (INPUT_MT_POINTER | INPUT_MT_DIRECT)) {
 +   __set_bit(EV_KEY, dev-evbit);
 +   __set_bit(BTN_TOUCH, dev-keybit);
 +
 +   copy_abs(dev, ABS_X, ABS_MT_POSITION_X);
 +   copy_abs(dev, ABS_Y, ABS_MT_POSITION_Y);
 +   copy_abs(dev, ABS_PRESSURE, ABS_MT_PRESSURE);
 +   }
 +   if (flags  INPUT_MT_POINTER) {
 +   __set_bit(BTN_TOOL_FINGER, dev-keybit);
 +   __set_bit(BTN_TOOL_DOUBLETAP, dev-keybit);
 +   if (num_slots = 3)
 +   __set_bit(BTN_TOOL_TRIPLETAP, dev-keybit);
 +   if (num_slots = 4)
 +   __set_bit(BTN_TOOL_QUADTAP, dev-keybit);
 +   if (num_slots = 5)
 +   __set_bit(BTN_TOOL_QUINTTAP, dev-keybit);
 +   __set_bit(INPUT_PROP_POINTER, dev-propbit);
 +   }
 +   if (flags  INPUT_MT_DIRECT)
 +   __set_bit(INPUT_PROP_DIRECT, dev-propbit);
 +
 /* Mark slots as 'unused' */
 for (i = 0; i  num_slots; i++)
 input_mt_set_value(mt-slots[i], ABS_MT_TRACKING_ID, -1);
 @@ -87,12 +117,17 @@ void input_mt_report_slot_state(struct input_dev *dev,
 struct input_mt_slot *slot;
 int id;

 -   if (!mt || !active) {
 +   if (!mt)
 +   return;
 +
 +   slot = mt-slots[input_abs_get_val(dev, ABS_MT_SLOT)];
 +   slot-frame = mt-frame;
 +
 +   if (!active) {
 input_event(dev, EV_ABS, ABS_MT_TRACKING_ID, -1);
 return;
 }

 -   slot = mt-slots[input_abs_get_val(dev, ABS_MT_SLOT)];
 id = input_mt_get_value(slot, ABS_MT_TRACKING_ID);
 if (id  0 || input_mt_get_value(slot, ABS_MT_TOOL_TYPE) != tool_type)
 id = input_mt_new_trkid(mt);
 @@ -177,3 +212,38 @@ void input_mt_report_pointer_emulation(struct input_dev 
 *dev, bool use_count)
 }
  }
  EXPORT_SYMBOL(input_mt_report_pointer_emulation);
 +
 +/**
 + * input_mt_sync_frame() - synchronize mt frame
 + * @dev: input device with allocated MT slots
 + *
 + * Close the frame and prepare the internal state for a new one.
 + * Depending on the flags, marks unused slots as inactive and performs
 + * pointer emulation.
 + */
 +void input_mt_sync_frame(struct input_dev *dev)
 +{
 +   struct input_mt *mt = dev-mt;
 +   struct input_mt_slot *s;
 +
 +   if (!mt)
 +   return;
 +
 +   if (mt-flags  INPUT_MT_DROP_UNUSED) {
 +   for (s = mt-slots; s != mt-slots + mt-num_slots; s++) {
 +   if (s-frame == mt-frame)
 +   continue;
 +   input_mt_slot(dev, s - mt-slots);
 +   input_event(dev, EV_ABS, ABS_MT_TRACKING_ID, -1);
 +   }
 +   }
 +
 +   if (mt-flags  INPUT_MT_POINTER)
 +   input_mt_report_pointer_emulation(dev, true);
 +
 +   if (mt-flags  INPUT_MT_DIRECT)
 +   input_mt_report_pointer_emulation(dev, false);
 +
 +   mt-frame++;

Where do we reset this frame counter?

 +}
 +EXPORT_SYMBOL(input_mt_sync_frame);
 diff --git a/include/linux/input/mt.h b/include/linux/input/mt.h
 index 03c52ce..a11d485 100644
 --- a/include/linux/input/mt.h
 +++ b/include/linux/input/mt.h
 @@ -15,12 +15,17 @@

  

Re: [PATCH 02/19] Input: Improve the events-per-packet estimate

[Ping Cheng]

On Tue, Aug 14, 2012 at 2:12 PM, Dmitry Torokhov
 wrote:
> >> On Sun, Aug 12, 2012 at 2:42 PM, Henrik Rydberg  
> >> wrote:
> >> > Many MT devices send a number of keys along with the mt information.
> >> > This patch makes sure that there is room for them in the packet
> >> > buffer.
>>
>> So, what device are we talking about here? I thought it is a touch
>> device with a few extra buttons, which are reported as key events. Am
>> I missing something?
>
> I was talking about a bog-standard computer keyboard here.
>
>>
>> If it is a touch device, we won't have too many buttons. So,
>> test_bit(i, dev->keybit) won't be true for more than the number of
>> buttons that declared by __set_bit().
>
> input_estimate_events_per_packet() is a generic routine that is used for
> all devices, not only [multi]touch.

I understand you are talking about standard keyboard. And I know this
routine is for all devices.

However, from the commit comments, the patch is to address an MT
issue. If it is not just for MT, we need either to make it clear in
the comments or to verify the type of the device in the code.

>> I would think we could play a keyboard (this keyboard does not have
>> letters on it ;-) with ten fingers.
>
> But even that keyboard would have more than 10 keys, right? So even
> though max_events should be 10 + 10 + 1 (10 keys, 10 msc, syn) your loop
> would produce what 88 + 88 + 1 for full size music keyboard?

No, I was not talking about implementing full music keyboard functions
in the kernel. My point was: why do we take 7 instead of 10, or
another number?

In fact, 7 works for me as long as we explain the rationale behind the
decision. I do not have a device that needs to post 10 button events
simultaneously, yet ;-).

Ping
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 02/19] Input: Improve the events-per-packet estimate

[Ping Cheng]

On Tue, Aug 14, 2012 at 1:01 PM, Henrik Rydberg  wrote:
> Hi Ping,
>
> Long time no see. :-)
>
>> > +   /* Make room for KEY and MSC events */
>> > +   events += 7;
>>
>> It is nice to get rid of the redundant pieces and to incorporate
>> common functions. Thank you.
>>
>> I have a question about the code above though.  Why do we use 7
>> instead of going through the keys like:
>>
>>   for (i = 0; i < KEY_MAX; i++)
>>   if (test_bit(i, dev->keybit))
>>   events++;
>
> Keyboards register a large amount of different keys, but seldom send
> more than one or two at a time. The value 7 is ad hoc, admittedly, but
> it makes the default buffer 8 bytes, which happens to precisely match
> the default buffer in evdev.

That can be a valid reason until we need to report more keys
simultaneously. Please update the comments so we know why we end up
with 7.

Thank you.

Ping
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 02/19] Input: Improve the events-per-packet estimate

[Ping Cheng]

On Tue, Aug 14, 2012 at 12:53 PM, Dmitry Torokhov
 wrote:
> On Tuesday, August 14, 2012 12:32:21 PM Ping Cheng wrote:
>> On Sun, Aug 12, 2012 at 2:42 PM, Henrik Rydberg  wrote:
>> > Many MT devices send a number of keys along with the mt information.
>> > This patch makes sure that there is room for them in the packet
>> > buffer.
>> >
>> > Signed-off-by: Henrik Rydberg 
>> > ---
>> >
>> >  drivers/input/input.c | 10 +++---
>> >  1 file changed, 7 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/input/input.c b/drivers/input/input.c
>> > index 6e90705..8ebf116 100644
>> > --- a/drivers/input/input.c
>> > +++ b/drivers/input/input.c
>> > @@ -1777,6 +1777,9 @@ static unsigned int
>> > input_estimate_events_per_packet(struct input_dev *dev)>
>> > if (test_bit(i, dev->relbit))
>> >
>> > events++;
>> >
>> > +   /* Make room for KEY and MSC events */
>> > +   events += 7;
>>
>> Hi Henrik,
>>
>> It is nice to get rid of the redundant pieces and to incorporate
>> common functions. Thank you.
>>
>> I have a question about the code above though.  Why do we use 7
>> instead of going through the keys like:
>>
>>   for (i = 0; i < KEY_MAX; i++)
>>   if (test_bit(i, dev->keybit))
>>   events++;
>
> Because that would result in gross over-estimation for many devices -
> my keyboard has 100+ keys but it never sends all of them in one event
> frame, not even if I can get a cat to lay on it ;)

Thanks for the prompt reply. I thought you were on vacation ;-).

So, what device are we talking about here? I thought it is a touch
device with a few extra buttons, which are reported as key events. Am
I missing something?

If it is a touch device, we won't have too many buttons. So,
test_bit(i, dev->keybit) won't be true for more than the number of
buttons that declared by __set_bit().

I would think we could play a keyboard (this keyboard does not have
letters on it ;-) with ten fingers.

Ping
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 02/19] Input: Improve the events-per-packet estimate

[Ping Cheng]

On Sun, Aug 12, 2012 at 2:42 PM, Henrik Rydberg  wrote:
> Many MT devices send a number of keys along with the mt information.
> This patch makes sure that there is room for them in the packet
> buffer.
>
> Signed-off-by: Henrik Rydberg 
> ---
>  drivers/input/input.c | 10 +++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 6e90705..8ebf116 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -1777,6 +1777,9 @@ static unsigned int 
> input_estimate_events_per_packet(struct input_dev *dev)
> if (test_bit(i, dev->relbit))
> events++;
>
> +   /* Make room for KEY and MSC events */
> +   events += 7;

Hi Henrik,

It is nice to get rid of the redundant pieces and to incorporate
common functions. Thank you.

I have a question about the code above though.  Why do we use 7
instead of going through the keys like:

for (i = 0; i < KEY_MAX; i++)
if (test_bit(i, dev->keybit))
events++;

Ping

> +
> return events;
>  }
>
> @@ -1815,6 +1818,7 @@ int input_register_device(struct input_dev *dev)
>  {
> static atomic_t input_no = ATOMIC_INIT(0);
> struct input_handler *handler;
> +   unsigned int packet_size;
> const char *path;
> int error;
>
> @@ -1827,9 +1831,9 @@ int input_register_device(struct input_dev *dev)
> /* Make sure that bitmasks not mentioned in dev->evbit are clean. */
> input_cleanse_bitmasks(dev);
>
> -   if (!dev->hint_events_per_packet)
> -   dev->hint_events_per_packet =
> -   input_estimate_events_per_packet(dev);
> +   packet_size = input_estimate_events_per_packet(dev);
> +   if (dev->hint_events_per_packet < packet_size)
> +   dev->hint_events_per_packet = packet_size;
>
> /*
>  * If delay and period are pre-set by the driver, then autorepeating
> --
> 1.7.11.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 02/19] Input: Improve the events-per-packet estimate

[Ping Cheng]

On Sun, Aug 12, 2012 at 2:42 PM, Henrik Rydberg rydb...@euromail.se wrote:
 Many MT devices send a number of keys along with the mt information.
 This patch makes sure that there is room for them in the packet
 buffer.

 Signed-off-by: Henrik Rydberg rydb...@euromail.se
 ---
  drivers/input/input.c | 10 +++---
  1 file changed, 7 insertions(+), 3 deletions(-)

 diff --git a/drivers/input/input.c b/drivers/input/input.c
 index 6e90705..8ebf116 100644
 --- a/drivers/input/input.c
 +++ b/drivers/input/input.c
 @@ -1777,6 +1777,9 @@ static unsigned int 
 input_estimate_events_per_packet(struct input_dev *dev)
 if (test_bit(i, dev-relbit))
 events++;

 +   /* Make room for KEY and MSC events */
 +   events += 7;

Hi Henrik,

It is nice to get rid of the redundant pieces and to incorporate
common functions. Thank you.

I have a question about the code above though.  Why do we use 7
instead of going through the keys like:

for (i = 0; i  KEY_MAX; i++)
if (test_bit(i, dev-keybit))
events++;

Ping

 +
 return events;
  }

 @@ -1815,6 +1818,7 @@ int input_register_device(struct input_dev *dev)
  {
 static atomic_t input_no = ATOMIC_INIT(0);
 struct input_handler *handler;
 +   unsigned int packet_size;
 const char *path;
 int error;

 @@ -1827,9 +1831,9 @@ int input_register_device(struct input_dev *dev)
 /* Make sure that bitmasks not mentioned in dev-evbit are clean. */
 input_cleanse_bitmasks(dev);

 -   if (!dev-hint_events_per_packet)
 -   dev-hint_events_per_packet =
 -   input_estimate_events_per_packet(dev);
 +   packet_size = input_estimate_events_per_packet(dev);
 +   if (dev-hint_events_per_packet  packet_size)
 +   dev-hint_events_per_packet = packet_size;

 /*
  * If delay and period are pre-set by the driver, then autorepeating
 --
 1.7.11.4

 --
 To unsubscribe from this list: send the line unsubscribe linux-input in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 02/19] Input: Improve the events-per-packet estimate

[Ping Cheng]

On Tue, Aug 14, 2012 at 12:53 PM, Dmitry Torokhov
dmitry.torok...@gmail.com wrote:
 On Tuesday, August 14, 2012 12:32:21 PM Ping Cheng wrote:
 On Sun, Aug 12, 2012 at 2:42 PM, Henrik Rydberg rydb...@euromail.se wrote:
  Many MT devices send a number of keys along with the mt information.
  This patch makes sure that there is room for them in the packet
  buffer.
 
  Signed-off-by: Henrik Rydberg rydb...@euromail.se
  ---
 
   drivers/input/input.c | 10 +++---
   1 file changed, 7 insertions(+), 3 deletions(-)
 
  diff --git a/drivers/input/input.c b/drivers/input/input.c
  index 6e90705..8ebf116 100644
  --- a/drivers/input/input.c
  +++ b/drivers/input/input.c
  @@ -1777,6 +1777,9 @@ static unsigned int
  input_estimate_events_per_packet(struct input_dev *dev)
  if (test_bit(i, dev-relbit))
 
  events++;
 
  +   /* Make room for KEY and MSC events */
  +   events += 7;

 Hi Henrik,

 It is nice to get rid of the redundant pieces and to incorporate
 common functions. Thank you.

 I have a question about the code above though.  Why do we use 7
 instead of going through the keys like:

   for (i = 0; i  KEY_MAX; i++)
   if (test_bit(i, dev-keybit))
   events++;

 Because that would result in gross over-estimation for many devices -
 my keyboard has 100+ keys but it never sends all of them in one event
 frame, not even if I can get a cat to lay on it ;)

Thanks for the prompt reply. I thought you were on vacation ;-).

So, what device are we talking about here? I thought it is a touch
device with a few extra buttons, which are reported as key events. Am
I missing something?

If it is a touch device, we won't have too many buttons. So,
test_bit(i, dev-keybit) won't be true for more than the number of
buttons that declared by __set_bit().

I would think we could play a keyboard (this keyboard does not have
letters on it ;-) with ten fingers.

Ping
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 02/19] Input: Improve the events-per-packet estimate

[Ping Cheng]

On Tue, Aug 14, 2012 at 1:01 PM, Henrik Rydberg rydb...@euromail.se wrote:
 Hi Ping,

 Long time no see. :-)

  +   /* Make room for KEY and MSC events */
  +   events += 7;

 It is nice to get rid of the redundant pieces and to incorporate
 common functions. Thank you.

 I have a question about the code above though.  Why do we use 7
 instead of going through the keys like:

   for (i = 0; i  KEY_MAX; i++)
   if (test_bit(i, dev-keybit))
   events++;

 Keyboards register a large amount of different keys, but seldom send
 more than one or two at a time. The value 7 is ad hoc, admittedly, but
 it makes the default buffer 8 bytes, which happens to precisely match
 the default buffer in evdev.

That can be a valid reason until we need to report more keys
simultaneously. Please update the comments so we know why we end up
with 7.

Thank you.

Ping
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 02/19] Input: Improve the events-per-packet estimate

[Ping Cheng]

On Tue, Aug 14, 2012 at 2:12 PM, Dmitry Torokhov
dmitry.torok...@gmail.com wrote:
  On Sun, Aug 12, 2012 at 2:42 PM, Henrik Rydberg rydb...@euromail.se 
  wrote:
   Many MT devices send a number of keys along with the mt information.
   This patch makes sure that there is room for them in the packet
   buffer.

 So, what device are we talking about here? I thought it is a touch
 device with a few extra buttons, which are reported as key events. Am
 I missing something?

 I was talking about a bog-standard computer keyboard here.


 If it is a touch device, we won't have too many buttons. So,
 test_bit(i, dev-keybit) won't be true for more than the number of
 buttons that declared by __set_bit().

 input_estimate_events_per_packet() is a generic routine that is used for
 all devices, not only [multi]touch.

I understand you are talking about standard keyboard. And I know this
routine is for all devices.

However, from the commit comments, the patch is to address an MT
issue. If it is not just for MT, we need either to make it clear in
the comments or to verify the type of the device in the code.

 I would think we could play a keyboard (this keyboard does not have
 letters on it ;-) with ten fingers.

 But even that keyboard would have more than 10 keys, right? So even
 though max_events should be 10 + 10 + 1 (10 keys, 10 msc, syn) your loop
 would produce what 88 + 88 + 1 for full size music keyboard?

No, I was not talking about implementing full music keyboard functions
in the kernel. My point was: why do we take 7 instead of 10, or
another number?

In fact, 7 works for me as long as we explain the rationale behind the
decision. I do not have a device that needs to post 10 button events
simultaneously, yet ;-).

Ping
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/