subject:"\[Linuxwacom\-devel\] \[PATCH wdaemon 1\/2\] Cleanup initial values"

Re: [Linuxwacom-devel] [PATCH wdaemon 1/2] Cleanup initial values

2016-01-20 Thread Peter Hutterer

On Wed, Jan 20, 2016 at 01:10:09PM -0800, Ping Cheng wrote:
> On Tue, Jan 19, 2016 at 7:11 PM, Peter Hutterer 
> wrote:
> 
> > On Tue, Jan 19, 2016 at 06:35:24PM -0800, Ping Cheng wrote:
> > > On Tue, Jan 19, 2016 at 6:17 PM, Peter Hutterer <
> > peter.hutte...@who-t.net>
> > > wrote:
> > >
> > > > On Tue, Jan 19, 2016 at 05:47:50PM -0800, Ping Cheng wrote:
> > > > > On Tue, Jan 19, 2016 at 5:03 PM, Peter Hutterer <
> > > > peter.hutte...@who-t.net>
> > > > > wrote:
> > > > >
> > > > > > On Tue, Jan 19, 2016 at 04:29:09PM -0800, Ping Cheng wrote:
> > > > > > > Group inital values by device types.
> > > > > > >
> > > > > > > Signed-off-by: Ping Cheng 
> > > > > > > ---
> > > > > > > v2: rely on device type instead of kernel version, as suggested
> > by
> > > > Peter.
> > > > > > > ---
> > > > > > >  wacom.c | 67
> > > > > > +
> > > > > > >  1 file changed, 13 insertions(+), 54 deletions(-)
> > > > > > >
> > > > > > > diff --git a/wacom.c b/wacom.c
> > > > > > > index 32a98c4..3f5a725 100644
> > > > > > > --- a/wacom.c
> > > > > > > +++ b/wacom.c
> > > > > > > @@ -211,26 +211,21 @@ static int wacom_intuos_events(struct
> > > > uinput_info
> > > > > > *info)
> > > > > > >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_RUBBER);
> > > > > > >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_MOUSE);
> > > > > > >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_LENS);
> > > > > > > - if (features->type != INTUOS5S &&
> > > > > > > - features->type != INTUOS5M &&
> > > > > > > - features->type != INTUOS5L)
> > > > > > > + if (features->type != INTUOS)
> > > > > > >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_FINGER);
> > > > > >
> > > > > > ok, now Im confused. in the previous version you had > INTUOS which
> > > > > > would've
> > > > > > excluded a couple of devices. Now you're only excluding the
> > Intuos1. Is
> > > > > > this
> > > > > > intended?
> > > > > >
> > > > >
> > > > > Yes, it is intentional.
> > > > >
> > > > > "> INTUOS" meant INTUOS is not included. All types after INTUOS were
> > > > > included, which are Intuos 3 and later, as well as all Cintitqs.
> > Now, I
> > > > > don't have those types in order. I can not use > INTUOS any more. In
> > > > fact,
> > > > > "> INTUOS" was unnecessary then. The type enum rearrangement was not
> > for
> > > > > this statement. It was for the if/switch statement you commented
> > below.
> > > >
> > > > I was referring to the devices < INTUOS that would've been dropped
> > with v1
> > > >
> > >
> > > There was no < INTUOS in v1. This was a <= INTUOS3L. But, that was for
> > > another loop...
> >
> > In the current code, the intuos 5 ones don't get the BTN_TOOL_FINGER bit
> > set (at least not here).
> >
> 
> That was a bug in the original code. It should be set.
> 
> 
> > in the first version [1], you removed the intous5 condition and replaced it
> > with an kernel version check and the condition
> >
> 
> I see why I got confused. I didn't count the original code/version.
> 
> > + if (features->type > INTUOS)
> >
> > this would've added the tag to everything except PTU, PL, GRAPHIRE, P4,
> > PENPARTNER and INTUOS.
> >
> 
> We are inside wacom_intuos_events. Except INTUOS, all other types mentioned
> above won't get into the routine. That's why we only need to consider
> INTUOS here.
> 
> In the second version [2], you removed the intuos5 condition and replaced it
> > with
> > > + if (features->type != INTUOS)
> >
> > so PTU, PL, GRAPHIRE, P4 and PENPARTNER now get the bit set.
> >
> 
> No, they still don't get the bit set. They are not in wacom_intuos_events
> at all.
> 
> that's equivalent to the current code, so with v2 you're unsetting
> > BTN_TOOL_FINGER from the Intuos, but leaving it in place for everything
> > else
> > (including Intuos 5 models which were previously excluded)
> 
> 
> > [1] http://sourceforge.net/p/linuxwacom/mailman/message/34776688/
> > [2] http://sourceforge.net/p/linuxwacom/mailman/message/34779774/
> >
> > It looks correct enough, but I'm just making sure this is indeed what you
> > intended (the commit message should've mentioned this change, "Cleanup" is
> > always a bit generic)
> >
> 
> You are right. It is intentional. I should add more in the commit message.
> But, it is just to clean up the code by properly grouping devices.
> 
> I didn't want to point out all the bugs, which were mainly caused by the
> mess...

and in the end it's just wdaemon anyway ;)

pushed now, thanks. together with the other commit still missing (I changed
a whitespace before pushing for nicer alignment)

Cheers,
   Peter

> 
> >
> > > but included in v2. Since this is the same as the current code, I guess
> > this
> > > > was intentational though and maybe an oversight in v1?
> > > >
> > >
> > > I am a bit confused ;-). This is v2 we are looking at. We only had one
> > > version before this one. Do you

Re: [Linuxwacom-devel] [PATCH wdaemon 1/2] Cleanup initial values

2016-01-20 Thread Ping Cheng

On Tue, Jan 19, 2016 at 7:11 PM, Peter Hutterer 
wrote:

> On Tue, Jan 19, 2016 at 06:35:24PM -0800, Ping Cheng wrote:
> > On Tue, Jan 19, 2016 at 6:17 PM, Peter Hutterer <
> peter.hutte...@who-t.net>
> > wrote:
> >
> > > On Tue, Jan 19, 2016 at 05:47:50PM -0800, Ping Cheng wrote:
> > > > On Tue, Jan 19, 2016 at 5:03 PM, Peter Hutterer <
> > > peter.hutte...@who-t.net>
> > > > wrote:
> > > >
> > > > > On Tue, Jan 19, 2016 at 04:29:09PM -0800, Ping Cheng wrote:
> > > > > > Group inital values by device types.
> > > > > >
> > > > > > Signed-off-by: Ping Cheng 
> > > > > > ---
> > > > > > v2: rely on device type instead of kernel version, as suggested
> by
> > > Peter.
> > > > > > ---
> > > > > >  wacom.c | 67
> > > > > +
> > > > > >  1 file changed, 13 insertions(+), 54 deletions(-)
> > > > > >
> > > > > > diff --git a/wacom.c b/wacom.c
> > > > > > index 32a98c4..3f5a725 100644
> > > > > > --- a/wacom.c
> > > > > > +++ b/wacom.c
> > > > > > @@ -211,26 +211,21 @@ static int wacom_intuos_events(struct
> > > uinput_info
> > > > > *info)
> > > > > >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_RUBBER);
> > > > > >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_MOUSE);
> > > > > >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_LENS);
> > > > > > - if (features->type != INTUOS5S &&
> > > > > > - features->type != INTUOS5M &&
> > > > > > - features->type != INTUOS5L)
> > > > > > + if (features->type != INTUOS)
> > > > > >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_FINGER);
> > > > >
> > > > > ok, now Im confused. in the previous version you had > INTUOS which
> > > > > would've
> > > > > excluded a couple of devices. Now you're only excluding the
> Intuos1. Is
> > > > > this
> > > > > intended?
> > > > >
> > > >
> > > > Yes, it is intentional.
> > > >
> > > > "> INTUOS" meant INTUOS is not included. All types after INTUOS were
> > > > included, which are Intuos 3 and later, as well as all Cintitqs.
> Now, I
> > > > don't have those types in order. I can not use > INTUOS any more. In
> > > fact,
> > > > "> INTUOS" was unnecessary then. The type enum rearrangement was not
> for
> > > > this statement. It was for the if/switch statement you commented
> below.
> > >
> > > I was referring to the devices < INTUOS that would've been dropped
> with v1
> > >
> >
> > There was no < INTUOS in v1. This was a <= INTUOS3L. But, that was for
> > another loop...
>
> In the current code, the intuos 5 ones don't get the BTN_TOOL_FINGER bit
> set (at least not here).
>

That was a bug in the original code. It should be set.


> in the first version [1], you removed the intous5 condition and replaced it
> with an kernel version check and the condition
>

I see why I got confused. I didn't count the original code/version.

> + if (features->type > INTUOS)
>
> this would've added the tag to everything except PTU, PL, GRAPHIRE, P4,
> PENPARTNER and INTUOS.
>

We are inside wacom_intuos_events. Except INTUOS, all other types mentioned
above won't get into the routine. That's why we only need to consider
INTUOS here.

In the second version [2], you removed the intuos5 condition and replaced it
> with
> > + if (features->type != INTUOS)
>
> so PTU, PL, GRAPHIRE, P4 and PENPARTNER now get the bit set.
>

No, they still don't get the bit set. They are not in wacom_intuos_events
at all.

that's equivalent to the current code, so with v2 you're unsetting
> BTN_TOOL_FINGER from the Intuos, but leaving it in place for everything
> else
> (including Intuos 5 models which were previously excluded)


> [1] http://sourceforge.net/p/linuxwacom/mailman/message/34776688/
> [2] http://sourceforge.net/p/linuxwacom/mailman/message/34779774/
>
> It looks correct enough, but I'm just making sure this is indeed what you
> intended (the commit message should've mentioned this change, "Cleanup" is
> always a bit generic)
>

You are right. It is intentional. I should add more in the commit message.
But, it is just to clean up the code by properly grouping devices.

I didn't want to point out all the bugs, which were mainly caused by the
mess...

Cheers,

Ping

>
> > but included in v2. Since this is the same as the current code, I guess
> this
> > > was intentational though and maybe an oversight in v1?
> > >
> >
> > I am a bit confused ;-). This is v2 we are looking at. We only had one
> > version before this one. Do you refer to another version?
> >
> > Cheers,
> >
> > Ping
> >
> > > > > - set_event(info, UI_SET_KEYBIT, BTN_TOUCH);
> > > > > >   set_event(info, UI_SET_KEYBIT, BTN_STYLUS2);
> > > > > >   set_event(info, UI_SET_KEYBIT, BTN_RIGHT);
> > > > > >   set_event(info, UI_SET_KEYBIT, BTN_LEFT);
> > > > > >   set_event(info, UI_SET_KEYBIT, BTN_MIDDLE);
> > > > > >   set_event(info, UI_SET_KEYBIT, BTN_SIDE);
> > > > > >   set_event(info, UI_SET_KEYBIT, BTN_EXTRA);
> > > > >

Re: [Linuxwacom-devel] [PATCH wdaemon 1/2] Cleanup initial values

2016-01-19 Thread Ping Cheng

On Tue, Jan 19, 2016 at 5:03 PM, Peter Hutterer 
wrote:

> On Tue, Jan 19, 2016 at 04:29:09PM -0800, Ping Cheng wrote:
> > Group inital values by device types.
> >
> > Signed-off-by: Ping Cheng 
> > ---
> > v2: rely on device type instead of kernel version, as suggested by Peter.
> > ---
> >  wacom.c | 67
> +
> >  1 file changed, 13 insertions(+), 54 deletions(-)
> >
> > diff --git a/wacom.c b/wacom.c
> > index 32a98c4..3f5a725 100644
> > --- a/wacom.c
> > +++ b/wacom.c
> > @@ -211,26 +211,21 @@ static int wacom_intuos_events(struct uinput_info
> *info)
> >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_RUBBER);
> >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_MOUSE);
> >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_LENS);
> > - if (features->type != INTUOS5S &&
> > - features->type != INTUOS5M &&
> > - features->type != INTUOS5L)
> > + if (features->type != INTUOS)
> >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_FINGER);
>
> ok, now Im confused. in the previous version you had > INTUOS which
> would've
> excluded a couple of devices. Now you're only excluding the Intuos1. Is
> this
> intended?
>

Yes, it is intentional.

"> INTUOS" meant INTUOS is not included. All types after INTUOS were
included, which are Intuos 3 and later, as well as all Cintitqs. Now, I
don't have those types in order. I can not use > INTUOS any more. In fact,
"> INTUOS" was unnecessary then. The type enum rearrangement was not for
this statement. It was for the if/switch statement you commented below.


> > - set_event(info, UI_SET_KEYBIT, BTN_TOUCH);
> >   set_event(info, UI_SET_KEYBIT, BTN_STYLUS2);
> >   set_event(info, UI_SET_KEYBIT, BTN_RIGHT);
> >   set_event(info, UI_SET_KEYBIT, BTN_LEFT);
> >   set_event(info, UI_SET_KEYBIT, BTN_MIDDLE);
> >   set_event(info, UI_SET_KEYBIT, BTN_SIDE);
> >   set_event(info, UI_SET_KEYBIT, BTN_EXTRA);
> > - if (features->type != INTUOS5S &&
> > - features->type != INTUOS5M &&
> > - features->type != INTUOS5L)
> > - set_event(info, UI_SET_KEYBIT, BTN_7);
> > - set_event(info, UI_SET_KEYBIT, BTN_MIDDLE);
> >
> > - if (features->type != INTUOS5S &&
> > - features->type != INTUOS5M &&
> > - features->type != INTUOS5L) {
> > + if (features->type == INTUOS3S ||
> > + features->type == INTUOS3 ||
> > + features->type == INTUOS3L ||
> > + features->type == CINTIQ ||
> > + features->type == BEE ||
> > + features->type == WACOM_21UX2) {
>
> I'd prefer a switch for simpler fallthrough, but I'm not going to fight too
> hard for this :)
>

Yeah, I paid attention to your suggestion ;-). But, there are only two
states: with RX/Y, or without. We can definitely go with

switch (features->type) {
case INTUOS3S:
case INTUOS3:
case INTUOS3L:
case CINTIQ:
case BEE:
case WACOM_21UX2:
set_event(info, UI_SET_ABSBIT, ABS_RX);
set_event(info, UI_SET_ABSBIT, ABS_RY);
break;
 }

Feel free to merge it in when you push the patch. You have my
signed-off-by, either way.

Thank you for your review.

Ping


> >   set_event(info, UI_SET_ABSBIT, ABS_RX);
> >   set_event(info, UI_SET_ABSBIT, ABS_RY);
> >   }
> > @@ -352,26 +347,14 @@ static int wacom_set_events(struct uinput_info
> *info, struct uinput_user_dev *de
> >   break;
> >   case INTUOS4:
> >   case INTUOS4L:
> > - set_event(info, UI_SET_ABSBIT, ABS_Z);
> > - set_event(info, UI_SET_KEYBIT, BTN_7);
> > - set_event(info, UI_SET_KEYBIT, BTN_8);
> > - /* fall trhu */
> > - case INTUOS4S:
> > - set_event(info, UI_SET_KEYBIT, BTN_0);
> > - set_event(info, UI_SET_KEYBIT, BTN_1);
> > - set_event(info, UI_SET_KEYBIT, BTN_2);
> > - set_event(info, UI_SET_KEYBIT, BTN_3);
> > - set_event(info, UI_SET_KEYBIT, BTN_4);
> > - set_event(info, UI_SET_KEYBIT, BTN_5);
> > - set_event(info, UI_SET_KEYBIT, BTN_6);
> > - wacom_intuos_events(info);
> > - break;
> >   case INTUOS5M:
> >   case INTUOS5L:
> >   set_event(info, UI_SET_KEYBIT, BTN_7);
> >   set_event(info, UI_SET_KEYBIT, BTN_8);
> >   case INTUOS5S:
> >   set_event(info, UI_SET_ABSBIT, ABS_Z);
> > + /* fall through */
> > + case INTUOS4S:
> >   set_event(info, UI_SET_KEYBIT, BTN_0);
> >   set_event(info, UI_SET_KEYBIT, BTN_1);
> >   set_event(info, UI_SET_KEYBIT, BTN_2);
> > @@ -416,7 +399,6 @@ static int wacom_set_initial_values(struct
> uinput_info *info,
> >   dev->absmax[ABS_WHEEL] = 71;
> >   /* fall through */
> >   case G4:
> > - /* fall through

Re: [Linuxwacom-devel] [PATCH wdaemon 1/2] Cleanup initial values

2016-01-19 Thread Peter Hutterer

On Tue, Jan 19, 2016 at 05:47:50PM -0800, Ping Cheng wrote:
> On Tue, Jan 19, 2016 at 5:03 PM, Peter Hutterer 
> wrote:
> 
> > On Tue, Jan 19, 2016 at 04:29:09PM -0800, Ping Cheng wrote:
> > > Group inital values by device types.
> > >
> > > Signed-off-by: Ping Cheng 
> > > ---
> > > v2: rely on device type instead of kernel version, as suggested by Peter.
> > > ---
> > >  wacom.c | 67
> > +
> > >  1 file changed, 13 insertions(+), 54 deletions(-)
> > >
> > > diff --git a/wacom.c b/wacom.c
> > > index 32a98c4..3f5a725 100644
> > > --- a/wacom.c
> > > +++ b/wacom.c
> > > @@ -211,26 +211,21 @@ static int wacom_intuos_events(struct uinput_info
> > *info)
> > >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_RUBBER);
> > >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_MOUSE);
> > >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_LENS);
> > > - if (features->type != INTUOS5S &&
> > > - features->type != INTUOS5M &&
> > > - features->type != INTUOS5L)
> > > + if (features->type != INTUOS)
> > >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_FINGER);
> >
> > ok, now Im confused. in the previous version you had > INTUOS which
> > would've
> > excluded a couple of devices. Now you're only excluding the Intuos1. Is
> > this
> > intended?
> >
> 
> Yes, it is intentional.
> 
> "> INTUOS" meant INTUOS is not included. All types after INTUOS were
> included, which are Intuos 3 and later, as well as all Cintitqs. Now, I
> don't have those types in order. I can not use > INTUOS any more. In fact,
> "> INTUOS" was unnecessary then. The type enum rearrangement was not for
> this statement. It was for the if/switch statement you commented below.

I was referring to the devices < INTUOS that would've been dropped with v1
but included in v2. Since this is the same as the current code, I guess this
was intentational though and maybe an oversight in v1?

Cheers,
   Peter


> > > - set_event(info, UI_SET_KEYBIT, BTN_TOUCH);
> > >   set_event(info, UI_SET_KEYBIT, BTN_STYLUS2);
> > >   set_event(info, UI_SET_KEYBIT, BTN_RIGHT);
> > >   set_event(info, UI_SET_KEYBIT, BTN_LEFT);
> > >   set_event(info, UI_SET_KEYBIT, BTN_MIDDLE);
> > >   set_event(info, UI_SET_KEYBIT, BTN_SIDE);
> > >   set_event(info, UI_SET_KEYBIT, BTN_EXTRA);
> > > - if (features->type != INTUOS5S &&
> > > - features->type != INTUOS5M &&
> > > - features->type != INTUOS5L)
> > > - set_event(info, UI_SET_KEYBIT, BTN_7);
> > > - set_event(info, UI_SET_KEYBIT, BTN_MIDDLE);
> > >
> > > - if (features->type != INTUOS5S &&
> > > - features->type != INTUOS5M &&
> > > - features->type != INTUOS5L) {
> > > + if (features->type == INTUOS3S ||
> > > + features->type == INTUOS3 ||
> > > + features->type == INTUOS3L ||
> > > + features->type == CINTIQ ||
> > > + features->type == BEE ||
> > > + features->type == WACOM_21UX2) {
> >
> > I'd prefer a switch for simpler fallthrough, but I'm not going to fight too
> > hard for this :)
> >
> 
> Yeah, I paid attention to your suggestion ;-). But, there are only two
> states: with RX/Y, or without. We can definitely go with
> 
> switch (features->type) {
> case INTUOS3S:
> case INTUOS3:
> case INTUOS3L:
> case CINTIQ:
> case BEE:
> case WACOM_21UX2:
> set_event(info, UI_SET_ABSBIT, ABS_RX);
> set_event(info, UI_SET_ABSBIT, ABS_RY);
> break;
>  }
> 
> Feel free to merge it in when you push the patch. You have my
> signed-off-by, either way.
> 
> Thank you for your review.
> 
> Ping
> 
> 
> > >   set_event(info, UI_SET_ABSBIT, ABS_RX);
> > >   set_event(info, UI_SET_ABSBIT, ABS_RY);
> > >   }
> > > @@ -352,26 +347,14 @@ static int wacom_set_events(struct uinput_info
> > *info, struct uinput_user_dev *de
> > >   break;
> > >   case INTUOS4:
> > >   case INTUOS4L:
> > > - set_event(info, UI_SET_ABSBIT, ABS_Z);
> > > - set_event(info, UI_SET_KEYBIT, BTN_7);
> > > - set_event(info, UI_SET_KEYBIT, BTN_8);
> > > - /* fall trhu */
> > > - case INTUOS4S:
> > > - set_event(info, UI_SET_KEYBIT, BTN_0);
> > > - set_event(info, UI_SET_KEYBIT, BTN_1);
> > > - set_event(info, UI_SET_KEYBIT, BTN_2);
> > > - set_event(info, UI_SET_KEYBIT, BTN_3);
> > > - set_event(info, UI_SET_KEYBIT, BTN_4);
> > > - set_event(info, UI_SET_KEYBIT, BTN_5);
> > > - set_event(info, UI_SET_KEYBIT, BTN_6);
> > > - wacom_intuos_events(info);
> > > - break;
> > >   case INTUOS5M:
> > >   case INTUOS5L:
> > >   set_event(info, UI_SET_KEYBIT, BTN_7);
> > >   set_event(info, UI_SET_KEYBIT, BTN_8);
> > >   case

Re: [Linuxwacom-devel] [PATCH wdaemon 1/2] Cleanup initial values

2016-01-19 Thread Peter Hutterer

On Tue, Jan 19, 2016 at 04:29:09PM -0800, Ping Cheng wrote:
> Group inital values by device types.
> 
> Signed-off-by: Ping Cheng 
> ---
> v2: rely on device type instead of kernel version, as suggested by Peter.
> ---
>  wacom.c | 67 
> +
>  1 file changed, 13 insertions(+), 54 deletions(-)
> 
> diff --git a/wacom.c b/wacom.c
> index 32a98c4..3f5a725 100644
> --- a/wacom.c
> +++ b/wacom.c
> @@ -211,26 +211,21 @@ static int wacom_intuos_events(struct uinput_info *info)
>   set_event(info, UI_SET_KEYBIT, BTN_TOOL_RUBBER);
>   set_event(info, UI_SET_KEYBIT, BTN_TOOL_MOUSE);
>   set_event(info, UI_SET_KEYBIT, BTN_TOOL_LENS);
> - if (features->type != INTUOS5S &&
> - features->type != INTUOS5M &&
> - features->type != INTUOS5L)
> + if (features->type != INTUOS)
>   set_event(info, UI_SET_KEYBIT, BTN_TOOL_FINGER);

ok, now Im confused. in the previous version you had > INTUOS which would've
excluded a couple of devices. Now you're only excluding the Intuos1. Is this
intended?

> - set_event(info, UI_SET_KEYBIT, BTN_TOUCH);
>   set_event(info, UI_SET_KEYBIT, BTN_STYLUS2);
>   set_event(info, UI_SET_KEYBIT, BTN_RIGHT);
>   set_event(info, UI_SET_KEYBIT, BTN_LEFT);
>   set_event(info, UI_SET_KEYBIT, BTN_MIDDLE);
>   set_event(info, UI_SET_KEYBIT, BTN_SIDE);
>   set_event(info, UI_SET_KEYBIT, BTN_EXTRA);
> - if (features->type != INTUOS5S &&
> - features->type != INTUOS5M &&
> - features->type != INTUOS5L)
> - set_event(info, UI_SET_KEYBIT, BTN_7);
> - set_event(info, UI_SET_KEYBIT, BTN_MIDDLE);
>  
> - if (features->type != INTUOS5S &&
> - features->type != INTUOS5M &&
> - features->type != INTUOS5L) {
> + if (features->type == INTUOS3S ||
> + features->type == INTUOS3 ||
> + features->type == INTUOS3L ||
> + features->type == CINTIQ ||
> + features->type == BEE ||
> + features->type == WACOM_21UX2) {

I'd prefer a switch for simpler fallthrough, but I'm not going to fight too
hard for this :)

Cheers,
   Peter

>   set_event(info, UI_SET_ABSBIT, ABS_RX);
>   set_event(info, UI_SET_ABSBIT, ABS_RY);
>   }
> @@ -352,26 +347,14 @@ static int wacom_set_events(struct uinput_info *info, 
> struct uinput_user_dev *de
>   break;
>   case INTUOS4:
>   case INTUOS4L:
> - set_event(info, UI_SET_ABSBIT, ABS_Z);
> - set_event(info, UI_SET_KEYBIT, BTN_7);
> - set_event(info, UI_SET_KEYBIT, BTN_8);
> - /* fall trhu */
> - case INTUOS4S:
> - set_event(info, UI_SET_KEYBIT, BTN_0);
> - set_event(info, UI_SET_KEYBIT, BTN_1);
> - set_event(info, UI_SET_KEYBIT, BTN_2);
> - set_event(info, UI_SET_KEYBIT, BTN_3);
> - set_event(info, UI_SET_KEYBIT, BTN_4);
> - set_event(info, UI_SET_KEYBIT, BTN_5);
> - set_event(info, UI_SET_KEYBIT, BTN_6);
> - wacom_intuos_events(info);
> - break;
>   case INTUOS5M:
>   case INTUOS5L:
>   set_event(info, UI_SET_KEYBIT, BTN_7);
>   set_event(info, UI_SET_KEYBIT, BTN_8);
>   case INTUOS5S:
>   set_event(info, UI_SET_ABSBIT, ABS_Z);
> + /* fall through */
> + case INTUOS4S:
>   set_event(info, UI_SET_KEYBIT, BTN_0);
>   set_event(info, UI_SET_KEYBIT, BTN_1);
>   set_event(info, UI_SET_KEYBIT, BTN_2);
> @@ -416,7 +399,6 @@ static int wacom_set_initial_values(struct uinput_info 
> *info,
>   dev->absmax[ABS_WHEEL] = 71;
>   /* fall through */
>   case G4:
> - /* fall through */
>   case GRAPHIRE:
>   break;
>  
> @@ -429,45 +411,22 @@ static int wacom_set_initial_values(struct uinput_info 
> *info,
>   /* fall through */
>   case INTUOS3S:
>   dev->absmax[ABS_RX] = 4096;
> - dev->absmax[ABS_Z] = 899;
> - dev->absmin[ABS_Z] = -900;
>   /* fall through */
> - case INTUOS:
> - dev->absmax[ABS_WHEEL] = 1023;
> - dev->absmax[ABS_TILT_X] = 127;
> - dev->absmax[ABS_TILT_Y] = 127;
> - dev->absmin[ABS_RZ] = -900;
> - dev->absmax[ABS_RZ] = 899;
> - dev->absmin[ABS_THROTTLE] = -1023;
> - dev->absmax[ABS_THROTTLE] = 1023;
> - break;
> - case PL:
> - case PTU:
> - break;
> - case PENPARTNER:
> - break;
> - case TABLETPC:
> - dev->absmax[ABS_RX] = 1023;
> - dev->absmax[ABS_RY] = 1023;
>   case INTUOS4S:
>   case INTUOS4:
>   case INTUOS4L:
> - dev->absmin[ABS_Z] = -900;
> - dev->absmax[ABS_Z] = 899;
> - break;
>

Re: [Linuxwacom-devel] [PATCH wdaemon 1/2] Cleanup initial values

2016-01-19 Thread Ping Cheng

On Tue, Jan 19, 2016 at 6:17 PM, Peter Hutterer 
wrote:

> On Tue, Jan 19, 2016 at 05:47:50PM -0800, Ping Cheng wrote:
> > On Tue, Jan 19, 2016 at 5:03 PM, Peter Hutterer <
> peter.hutte...@who-t.net>
> > wrote:
> >
> > > On Tue, Jan 19, 2016 at 04:29:09PM -0800, Ping Cheng wrote:
> > > > Group inital values by device types.
> > > >
> > > > Signed-off-by: Ping Cheng 
> > > > ---
> > > > v2: rely on device type instead of kernel version, as suggested by
> Peter.
> > > > ---
> > > >  wacom.c | 67
> > > +
> > > >  1 file changed, 13 insertions(+), 54 deletions(-)
> > > >
> > > > diff --git a/wacom.c b/wacom.c
> > > > index 32a98c4..3f5a725 100644
> > > > --- a/wacom.c
> > > > +++ b/wacom.c
> > > > @@ -211,26 +211,21 @@ static int wacom_intuos_events(struct
> uinput_info
> > > *info)
> > > >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_RUBBER);
> > > >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_MOUSE);
> > > >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_LENS);
> > > > - if (features->type != INTUOS5S &&
> > > > - features->type != INTUOS5M &&
> > > > - features->type != INTUOS5L)
> > > > + if (features->type != INTUOS)
> > > >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_FINGER);
> > >
> > > ok, now Im confused. in the previous version you had > INTUOS which
> > > would've
> > > excluded a couple of devices. Now you're only excluding the Intuos1. Is
> > > this
> > > intended?
> > >
> >
> > Yes, it is intentional.
> >
> > "> INTUOS" meant INTUOS is not included. All types after INTUOS were
> > included, which are Intuos 3 and later, as well as all Cintitqs. Now, I
> > don't have those types in order. I can not use > INTUOS any more. In
> fact,
> > "> INTUOS" was unnecessary then. The type enum rearrangement was not for
> > this statement. It was for the if/switch statement you commented below.
>
> I was referring to the devices < INTUOS that would've been dropped with v1
>

There was no < INTUOS in v1. This was a <= INTUOS3L. But, that was for
another loop...

but included in v2. Since this is the same as the current code, I guess this
> was intentational though and maybe an oversight in v1?
>

I am a bit confused ;-). This is v2 we are looking at. We only had one
version before this one. Do you refer to another version?

Cheers,

Ping

> > > - set_event(info, UI_SET_KEYBIT, BTN_TOUCH);
> > > >   set_event(info, UI_SET_KEYBIT, BTN_STYLUS2);
> > > >   set_event(info, UI_SET_KEYBIT, BTN_RIGHT);
> > > >   set_event(info, UI_SET_KEYBIT, BTN_LEFT);
> > > >   set_event(info, UI_SET_KEYBIT, BTN_MIDDLE);
> > > >   set_event(info, UI_SET_KEYBIT, BTN_SIDE);
> > > >   set_event(info, UI_SET_KEYBIT, BTN_EXTRA);
> > > > - if (features->type != INTUOS5S &&
> > > > - features->type != INTUOS5M &&
> > > > - features->type != INTUOS5L)
> > > > - set_event(info, UI_SET_KEYBIT, BTN_7);
> > > > - set_event(info, UI_SET_KEYBIT, BTN_MIDDLE);
> > > >
> > > > - if (features->type != INTUOS5S &&
> > > > - features->type != INTUOS5M &&
> > > > - features->type != INTUOS5L) {
> > > > + if (features->type == INTUOS3S ||
> > > > + features->type == INTUOS3 ||
> > > > + features->type == INTUOS3L ||
> > > > + features->type == CINTIQ ||
> > > > + features->type == BEE ||
> > > > + features->type == WACOM_21UX2) {
> > >
> > > I'd prefer a switch for simpler fallthrough, but I'm not going to
> fight too
> > > hard for this :)
> > >
> >
> > Yeah, I paid attention to your suggestion ;-). But, there are only two
> > states: with RX/Y, or without. We can definitely go with
> >
> > switch (features->type) {
> > case INTUOS3S:
> > case INTUOS3:
> > case INTUOS3L:
> > case CINTIQ:
> > case BEE:
> > case WACOM_21UX2:
> > set_event(info, UI_SET_ABSBIT, ABS_RX);
> > set_event(info, UI_SET_ABSBIT, ABS_RY);
> > break;
> >  }
> >
> > Feel free to merge it in when you push the patch. You have my
> > signed-off-by, either way.
> >
> > Thank you for your review.
> >
> > Ping
> >
> >
> > > >   set_event(info, UI_SET_ABSBIT, ABS_RX);
> > > >   set_event(info, UI_SET_ABSBIT, ABS_RY);
> > > >   }
> > > > @@ -352,26 +347,14 @@ static int wacom_set_events(struct uinput_info
> > > *info, struct uinput_user_dev *de
> > > >   break;
> > > >   case INTUOS4:
> > > >   case INTUOS4L:
> > > > - set_event(info, UI_SET_ABSBIT, ABS_Z);
> > > > - set_event(info, UI_SET_KEYBIT, BTN_7);
> > > > - set_event(info, UI_SET_KEYBIT, BTN_8);
> > > > - /* fall trhu */
> > > > - case INTUOS4S:
> > > > - set_event(info, UI_SET_KEYBIT, BTN_0);
> > > > - set_event(info, UI_SET_KEYBIT, BTN_1);
> > > > -

Re: [Linuxwacom-devel] [PATCH wdaemon 1/2] Cleanup initial values

2016-01-19 Thread Peter Hutterer

On Tue, Jan 19, 2016 at 06:35:24PM -0800, Ping Cheng wrote:
> On Tue, Jan 19, 2016 at 6:17 PM, Peter Hutterer 
> wrote:
> 
> > On Tue, Jan 19, 2016 at 05:47:50PM -0800, Ping Cheng wrote:
> > > On Tue, Jan 19, 2016 at 5:03 PM, Peter Hutterer <
> > peter.hutte...@who-t.net>
> > > wrote:
> > >
> > > > On Tue, Jan 19, 2016 at 04:29:09PM -0800, Ping Cheng wrote:
> > > > > Group inital values by device types.
> > > > >
> > > > > Signed-off-by: Ping Cheng 
> > > > > ---
> > > > > v2: rely on device type instead of kernel version, as suggested by
> > Peter.
> > > > > ---
> > > > >  wacom.c | 67
> > > > +
> > > > >  1 file changed, 13 insertions(+), 54 deletions(-)
> > > > >
> > > > > diff --git a/wacom.c b/wacom.c
> > > > > index 32a98c4..3f5a725 100644
> > > > > --- a/wacom.c
> > > > > +++ b/wacom.c
> > > > > @@ -211,26 +211,21 @@ static int wacom_intuos_events(struct
> > uinput_info
> > > > *info)
> > > > >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_RUBBER);
> > > > >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_MOUSE);
> > > > >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_LENS);
> > > > > - if (features->type != INTUOS5S &&
> > > > > - features->type != INTUOS5M &&
> > > > > - features->type != INTUOS5L)
> > > > > + if (features->type != INTUOS)
> > > > >   set_event(info, UI_SET_KEYBIT, BTN_TOOL_FINGER);
> > > >
> > > > ok, now Im confused. in the previous version you had > INTUOS which
> > > > would've
> > > > excluded a couple of devices. Now you're only excluding the Intuos1. Is
> > > > this
> > > > intended?
> > > >
> > >
> > > Yes, it is intentional.
> > >
> > > "> INTUOS" meant INTUOS is not included. All types after INTUOS were
> > > included, which are Intuos 3 and later, as well as all Cintitqs. Now, I
> > > don't have those types in order. I can not use > INTUOS any more. In
> > fact,
> > > "> INTUOS" was unnecessary then. The type enum rearrangement was not for
> > > this statement. It was for the if/switch statement you commented below.
> >
> > I was referring to the devices < INTUOS that would've been dropped with v1
> >
> 
> There was no < INTUOS in v1. This was a <= INTUOS3L. But, that was for
> another loop...

In the current code, the intuos 5 ones don't get the BTN_TOOL_FINGER bit
set (at least not here).

in the first version [1], you removed the intous5 condition and replaced it
with an kernel version check and the condition

> + if (features->type > INTUOS)

this would've added the tag to everything except PTU, PL, GRAPHIRE, P4,
PENPARTNER and INTUOS.

In the second version [2], you removed the intuos5 condition and replaced it
with 
> + if (features->type != INTUOS)

so PTU, PL, GRAPHIRE, P4 and PENPARTNER now get the bit set. 
that's equivalent to the current code, so with v2 you're unsetting
BTN_TOOL_FINGER from the Intuos, but leaving it in place for everything else
(including Intuos 5 models which were previously excluded)

[1] http://sourceforge.net/p/linuxwacom/mailman/message/34776688/
[2] http://sourceforge.net/p/linuxwacom/mailman/message/34779774/

It looks correct enough, but I'm just making sure this is indeed what you
intended (the commit message should've mentioned this change, "Cleanup" is
always a bit generic)

Cheers,
   Peter

> 
> but included in v2. Since this is the same as the current code, I guess this
> > was intentational though and maybe an oversight in v1?
> >
> 
> I am a bit confused ;-). This is v2 we are looking at. We only had one
> version before this one. Do you refer to another version?
> 
> Cheers,
> 
> Ping
> 
> > > > - set_event(info, UI_SET_KEYBIT, BTN_TOUCH);
> > > > >   set_event(info, UI_SET_KEYBIT, BTN_STYLUS2);
> > > > >   set_event(info, UI_SET_KEYBIT, BTN_RIGHT);
> > > > >   set_event(info, UI_SET_KEYBIT, BTN_LEFT);
> > > > >   set_event(info, UI_SET_KEYBIT, BTN_MIDDLE);
> > > > >   set_event(info, UI_SET_KEYBIT, BTN_SIDE);
> > > > >   set_event(info, UI_SET_KEYBIT, BTN_EXTRA);
> > > > > - if (features->type != INTUOS5S &&
> > > > > - features->type != INTUOS5M &&
> > > > > - features->type != INTUOS5L)
> > > > > - set_event(info, UI_SET_KEYBIT, BTN_7);
> > > > > - set_event(info, UI_SET_KEYBIT, BTN_MIDDLE);
> > > > >
> > > > > - if (features->type != INTUOS5S &&
> > > > > - features->type != INTUOS5M &&
> > > > > - features->type != INTUOS5L) {
> > > > > + if (features->type == INTUOS3S ||
> > > > > + features->type == INTUOS3 ||
> > > > > + features->type == INTUOS3L ||
> > > > > + features->type == CINTIQ ||
> > > > > + features->type == BEE ||
> > > > > + features->type == WACOM_21UX2) {
> > > >
> > > > I'd prefer a switch for simpler fallthrough, but I'm not going to
> > fight too
> > > > hard for this :)
> > > >
> > >
> > > Yeah, I

[Linuxwacom-devel] [PATCH wdaemon 1/2] Cleanup initial values

2016-01-19 Thread Ping Cheng

Group inital values by device types.

Signed-off-by: Ping Cheng 
---
v2: rely on device type instead of kernel version, as suggested by Peter.
---
 wacom.c | 67 +
 1 file changed, 13 insertions(+), 54 deletions(-)

diff --git a/wacom.c b/wacom.c
index 32a98c4..3f5a725 100644
--- a/wacom.c
+++ b/wacom.c
@@ -211,26 +211,21 @@ static int wacom_intuos_events(struct uinput_info *info)
set_event(info, UI_SET_KEYBIT, BTN_TOOL_RUBBER);
set_event(info, UI_SET_KEYBIT, BTN_TOOL_MOUSE);
set_event(info, UI_SET_KEYBIT, BTN_TOOL_LENS);
-   if (features->type != INTUOS5S &&
-   features->type != INTUOS5M &&
-   features->type != INTUOS5L)
+   if (features->type != INTUOS)
set_event(info, UI_SET_KEYBIT, BTN_TOOL_FINGER);
-   set_event(info, UI_SET_KEYBIT, BTN_TOUCH);
set_event(info, UI_SET_KEYBIT, BTN_STYLUS2);
set_event(info, UI_SET_KEYBIT, BTN_RIGHT);
set_event(info, UI_SET_KEYBIT, BTN_LEFT);
set_event(info, UI_SET_KEYBIT, BTN_MIDDLE);
set_event(info, UI_SET_KEYBIT, BTN_SIDE);
set_event(info, UI_SET_KEYBIT, BTN_EXTRA);
-   if (features->type != INTUOS5S &&
-   features->type != INTUOS5M &&
-   features->type != INTUOS5L)
-   set_event(info, UI_SET_KEYBIT, BTN_7);
-   set_event(info, UI_SET_KEYBIT, BTN_MIDDLE);
 
-   if (features->type != INTUOS5S &&
-   features->type != INTUOS5M &&
-   features->type != INTUOS5L) {
+   if (features->type == INTUOS3S ||
+   features->type == INTUOS3 ||
+   features->type == INTUOS3L ||
+   features->type == CINTIQ ||
+   features->type == BEE ||
+   features->type == WACOM_21UX2) {
set_event(info, UI_SET_ABSBIT, ABS_RX);
set_event(info, UI_SET_ABSBIT, ABS_RY);
}
@@ -352,26 +347,14 @@ static int wacom_set_events(struct uinput_info *info, 
struct uinput_user_dev *de
break;
case INTUOS4:
case INTUOS4L:
-   set_event(info, UI_SET_ABSBIT, ABS_Z);
-   set_event(info, UI_SET_KEYBIT, BTN_7);
-   set_event(info, UI_SET_KEYBIT, BTN_8);
-   /* fall trhu */
-   case INTUOS4S:
-   set_event(info, UI_SET_KEYBIT, BTN_0);
-   set_event(info, UI_SET_KEYBIT, BTN_1);
-   set_event(info, UI_SET_KEYBIT, BTN_2);
-   set_event(info, UI_SET_KEYBIT, BTN_3);
-   set_event(info, UI_SET_KEYBIT, BTN_4);
-   set_event(info, UI_SET_KEYBIT, BTN_5);
-   set_event(info, UI_SET_KEYBIT, BTN_6);
-   wacom_intuos_events(info);
-   break;
case INTUOS5M:
case INTUOS5L:
set_event(info, UI_SET_KEYBIT, BTN_7);
set_event(info, UI_SET_KEYBIT, BTN_8);
case INTUOS5S:
set_event(info, UI_SET_ABSBIT, ABS_Z);
+   /* fall through */
+   case INTUOS4S:
set_event(info, UI_SET_KEYBIT, BTN_0);
set_event(info, UI_SET_KEYBIT, BTN_1);
set_event(info, UI_SET_KEYBIT, BTN_2);
@@ -416,7 +399,6 @@ static int wacom_set_initial_values(struct uinput_info 
*info,
dev->absmax[ABS_WHEEL] = 71;
/* fall through */
case G4:
-   /* fall through */
case GRAPHIRE:
break;
 
@@ -429,45 +411,22 @@ static int wacom_set_initial_values(struct uinput_info 
*info,
/* fall through */
case INTUOS3S:
dev->absmax[ABS_RX] = 4096;
-   dev->absmax[ABS_Z] = 899;
-   dev->absmin[ABS_Z] = -900;
/* fall through */
-   case INTUOS:
-   dev->absmax[ABS_WHEEL] = 1023;
-   dev->absmax[ABS_TILT_X] = 127;
-   dev->absmax[ABS_TILT_Y] = 127;
-   dev->absmin[ABS_RZ] = -900;
-   dev->absmax[ABS_RZ] = 899;
-   dev->absmin[ABS_THROTTLE] = -1023;
-   dev->absmax[ABS_THROTTLE] = 1023;
-   break;
-   case PL:
-   case PTU:
-   break;
-   case PENPARTNER:
-   break;
-   case TABLETPC:
-   dev->absmax[ABS_RX] = 1023;
-   dev->absmax[ABS_RY] = 1023;
case INTUOS4S:
case INTUOS4:
case INTUOS4L:
-   dev->absmin[ABS_Z] = -900;
-   dev->absmax[ABS_Z] = 899;
-   break;
case INTUOS5S:
case INTUOS5M:
case INTUOS5L:
-   dev->absfuzz[ABS_X] = 4;
-   dev->absfuzz[ABS_Y] = 4;
-   dev->absfuzz[ABS_Y] = 4;
dev->absmin[ABS_Z] = -900;
dev->absmax[ABS_Z] = 899;
-   dev->absmin[ABS_RZ] = -900;
-   dev->absmax[ABS_RZ] = 899;
+   /* fall

Re: [Linuxwacom-devel] [PATCH wdaemon 1/2] Cleanup initial values

Re: [Linuxwacom-devel] [PATCH wdaemon 1/2] Cleanup initial values

Re: [Linuxwacom-devel] [PATCH wdaemon 1/2] Cleanup initial values

Re: [Linuxwacom-devel] [PATCH wdaemon 1/2] Cleanup initial values

Re: [Linuxwacom-devel] [PATCH wdaemon 1/2] Cleanup initial values

Re: [Linuxwacom-devel] [PATCH wdaemon 1/2] Cleanup initial values

Re: [Linuxwacom-devel] [PATCH wdaemon 1/2] Cleanup initial values

[Linuxwacom-devel] [PATCH wdaemon 1/2] Cleanup initial values

8 matches

Site Navigation

Mail list logo

Footer information