Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

2017-10-11 Thread Peter Hutterer 
On Wed, Oct 11, 2017 at 10:43:24PM +0200, Henrik Rydberg wrote:
> > > > , but I'd go for fixing the documentation. And re-reading it, it's not 
> > > > clear that the doc tells us to have [0,90]. It mentions negative values 
> > > > and out of ranges too, so we might just as well simply clarify that we 
> > > > rather have [-90,90], with 0 being "north".
> > > 
> > > ... I'd like the documentation fix to go in together in one go with this 
> > > patch if possible.
> > > 
> > 
> > Sounds like a plan.
> 
> How about this patch?
> 
> Henrik
> 
> ---
> 
> From b14f92066dfab3f8a255ec7b5a30cb1a864dc62f Mon Sep 17 00:00:00 2001
> From: Henrik Rydberg 
> Date: Wed, 11 Oct 2017 22:41:39 +0200
> Subject: [PATCH] Input: docs - clarify the usage of ABS_MT_ORIENTATION
> 
> As more drivers start to support touch orientation, clarify how the
> value range should be set to match the expected behavior in
> userland.
> 
> Signed-off-by: Henrik Rydberg 
> ---
>  Documentation/input/multi-touch-protocol.rst | 14 --
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/input/multi-touch-protocol.rst 
> b/Documentation/input/multi-touch-protocol.rst
> index 8035868..a0c5c03 100644
> --- a/Documentation/input/multi-touch-protocol.rst
> +++ b/Documentation/input/multi-touch-protocol.rst
> @@ -269,15 +269,17 @@ ABS_MT_ORIENTATION
>  The orientation of the touching ellipse. The value should describe a 
> signed
>  quarter of a revolution clockwise around the touch center. The signed 
> value
>  range is arbitrary, but zero should be returned for an ellipse aligned 
> with
> -the Y axis of the surface, a negative value when the ellipse is turned to
> -the left, and a positive value when the ellipse is turned to the
> -right. When completely aligned with the X axis, the range max should be
> -returned.
> +the Y axis of the surface (north). A negative value should be returned 
> when
> +the ellipse is turned to the left (west), 

this bit is good.


> +with the smallest value reported
> +when aligned with the negative X axis. The largest value should be 
> returned
> +when aligned with the positive X axis.

this bit is less precise than before, because 'smallest' doesn't mean
'minimum' but smallest value. so a device announcing -90,90 could think that
returning -120 is a good idea for X alignment. I liked the previous "When
completely aligned with the X axis, the range max should be returned.", it
was less ambiguous.

replacing 'smallest value'/'largest value' with -range_max/range_max should
be sufficient here.


> +
> +The value range should be specified as [-range_max, range_max].

I'd really like this to be [0, max] == can only detect one quarter/half and
[-max, +max] == can detect both directions. It's one extra bit of
information that may come in useful at some point.

>  Touch ellipsis are symmetrical by default. For devices capable of true 
> 360
> -degree orientation, the reported orientation must exceed the range max to
> +degree orientation, the reported orientation will exceed range_max, in 
> order to
>  indicate more than a quarter of a revolution. For an upside-down finger,
> -range max * 2 should be returned.
> ++- 2 * range_max should be returned.

ack to this bit

Cheers,
   Peter

>  
>  Orientation can be omitted if the touch area is circular, or if the
>  information is not available in the kernel driver. Partial orientation
> -- 
> 2.14.1
>

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

2017-10-11 Thread Peter Hutterer 
On Wed, Oct 11, 2017 at 10:43:24PM +0200, Henrik Rydberg wrote:
> > > > , but I'd go for fixing the documentation. And re-reading it, it's not 
> > > > clear that the doc tells us to have [0,90]. It mentions negative values 
> > > > and out of ranges too, so we might just as well simply clarify that we 
> > > > rather have [-90,90], with 0 being "north".
> > > 
> > > ... I'd like the documentation fix to go in together in one go with this 
> > > patch if possible.
> > > 
> > 
> > Sounds like a plan.
> 
> How about this patch?
> 
> Henrik
> 
> ---
> 
> From b14f92066dfab3f8a255ec7b5a30cb1a864dc62f Mon Sep 17 00:00:00 2001
> From: Henrik Rydberg 
> Date: Wed, 11 Oct 2017 22:41:39 +0200
> Subject: [PATCH] Input: docs - clarify the usage of ABS_MT_ORIENTATION
> 
> As more drivers start to support touch orientation, clarify how the
> value range should be set to match the expected behavior in
> userland.
> 
> Signed-off-by: Henrik Rydberg 
> ---
>  Documentation/input/multi-touch-protocol.rst | 14 --
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/input/multi-touch-protocol.rst 
> b/Documentation/input/multi-touch-protocol.rst
> index 8035868..a0c5c03 100644
> --- a/Documentation/input/multi-touch-protocol.rst
> +++ b/Documentation/input/multi-touch-protocol.rst
> @@ -269,15 +269,17 @@ ABS_MT_ORIENTATION
>  The orientation of the touching ellipse. The value should describe a 
> signed
>  quarter of a revolution clockwise around the touch center. The signed 
> value
>  range is arbitrary, but zero should be returned for an ellipse aligned 
> with
> -the Y axis of the surface, a negative value when the ellipse is turned to
> -the left, and a positive value when the ellipse is turned to the
> -right. When completely aligned with the X axis, the range max should be
> -returned.
> +the Y axis of the surface (north). A negative value should be returned 
> when
> +the ellipse is turned to the left (west), 

this bit is good.


> +with the smallest value reported
> +when aligned with the negative X axis. The largest value should be 
> returned
> +when aligned with the positive X axis.

this bit is less precise than before, because 'smallest' doesn't mean
'minimum' but smallest value. so a device announcing -90,90 could think that
returning -120 is a good idea for X alignment. I liked the previous "When
completely aligned with the X axis, the range max should be returned.", it
was less ambiguous.

replacing 'smallest value'/'largest value' with -range_max/range_max should
be sufficient here.


> +
> +The value range should be specified as [-range_max, range_max].

I'd really like this to be [0, max] == can only detect one quarter/half and
[-max, +max] == can detect both directions. It's one extra bit of
information that may come in useful at some point.

>  Touch ellipsis are symmetrical by default. For devices capable of true 
> 360
> -degree orientation, the reported orientation must exceed the range max to
> +degree orientation, the reported orientation will exceed range_max, in 
> order to
>  indicate more than a quarter of a revolution. For an upside-down finger,
> -range max * 2 should be returned.
> ++- 2 * range_max should be returned.

ack to this bit

Cheers,
   Peter

>  
>  Orientation can be omitted if the touch area is circular, or if the
>  information is not available in the kernel driver. Partial orientation
> -- 
> 2.14.1
>

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

2017-10-11 Thread Henrik Rydberg 
> > > , but I'd go for fixing the documentation. And re-reading it, it's not 
> > > clear that the doc tells us to have [0,90]. It mentions negative values 
> > > and out of ranges too, so we might just as well simply clarify that we 
> > > rather have [-90,90], with 0 being "north".
> > 
> > ... I'd like the documentation fix to go in together in one go with this 
> > patch if possible.
> > 
> 
> Sounds like a plan.

How about this patch?

Henrik

---

>From b14f92066dfab3f8a255ec7b5a30cb1a864dc62f Mon Sep 17 00:00:00 2001
From: Henrik Rydberg 
Date: Wed, 11 Oct 2017 22:41:39 +0200
Subject: [PATCH] Input: docs - clarify the usage of ABS_MT_ORIENTATION

As more drivers start to support touch orientation, clarify how the
value range should be set to match the expected behavior in
userland.

Signed-off-by: Henrik Rydberg 
---
 Documentation/input/multi-touch-protocol.rst | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/Documentation/input/multi-touch-protocol.rst 
b/Documentation/input/multi-touch-protocol.rst
index 8035868..a0c5c03 100644
--- a/Documentation/input/multi-touch-protocol.rst
+++ b/Documentation/input/multi-touch-protocol.rst
@@ -269,15 +269,17 @@ ABS_MT_ORIENTATION
 The orientation of the touching ellipse. The value should describe a signed
 quarter of a revolution clockwise around the touch center. The signed value
 range is arbitrary, but zero should be returned for an ellipse aligned with
-the Y axis of the surface, a negative value when the ellipse is turned to
-the left, and a positive value when the ellipse is turned to the
-right. When completely aligned with the X axis, the range max should be
-returned.
+the Y axis of the surface (north). A negative value should be returned when
+the ellipse is turned to the left (west), with the smallest value reported
+when aligned with the negative X axis. The largest value should be returned
+when aligned with the positive X axis.
+
+The value range should be specified as [-range_max, range_max].
 
 Touch ellipsis are symmetrical by default. For devices capable of true 360
-degree orientation, the reported orientation must exceed the range max to
+degree orientation, the reported orientation will exceed range_max, in 
order to
 indicate more than a quarter of a revolution. For an upside-down finger,
-range max * 2 should be returned.
++- 2 * range_max should be returned.
 
 Orientation can be omitted if the touch area is circular, or if the
 information is not available in the kernel driver. Partial orientation
-- 
2.14.1

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

2017-10-11 Thread Henrik Rydberg 
> > > , but I'd go for fixing the documentation. And re-reading it, it's not 
> > > clear that the doc tells us to have [0,90]. It mentions negative values 
> > > and out of ranges too, so we might just as well simply clarify that we 
> > > rather have [-90,90], with 0 being "north".
> > 
> > ... I'd like the documentation fix to go in together in one go with this 
> > patch if possible.
> > 
> 
> Sounds like a plan.

How about this patch?

Henrik

---

>From b14f92066dfab3f8a255ec7b5a30cb1a864dc62f Mon Sep 17 00:00:00 2001
From: Henrik Rydberg 
Date: Wed, 11 Oct 2017 22:41:39 +0200
Subject: [PATCH] Input: docs - clarify the usage of ABS_MT_ORIENTATION

As more drivers start to support touch orientation, clarify how the
value range should be set to match the expected behavior in
userland.

Signed-off-by: Henrik Rydberg 
---
 Documentation/input/multi-touch-protocol.rst | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/Documentation/input/multi-touch-protocol.rst 
b/Documentation/input/multi-touch-protocol.rst
index 8035868..a0c5c03 100644
--- a/Documentation/input/multi-touch-protocol.rst
+++ b/Documentation/input/multi-touch-protocol.rst
@@ -269,15 +269,17 @@ ABS_MT_ORIENTATION
 The orientation of the touching ellipse. The value should describe a signed
 quarter of a revolution clockwise around the touch center. The signed value
 range is arbitrary, but zero should be returned for an ellipse aligned with
-the Y axis of the surface, a negative value when the ellipse is turned to
-the left, and a positive value when the ellipse is turned to the
-right. When completely aligned with the X axis, the range max should be
-returned.
+the Y axis of the surface (north). A negative value should be returned when
+the ellipse is turned to the left (west), with the smallest value reported
+when aligned with the negative X axis. The largest value should be returned
+when aligned with the positive X axis.
+
+The value range should be specified as [-range_max, range_max].
 
 Touch ellipsis are symmetrical by default. For devices capable of true 360
-degree orientation, the reported orientation must exceed the range max to
+degree orientation, the reported orientation will exceed range_max, in 
order to
 indicate more than a quarter of a revolution. For an upside-down finger,
-range max * 2 should be returned.
++- 2 * range_max should be returned.
 
 Orientation can be omitted if the touch area is circular, or if the
 information is not available in the kernel driver. Partial orientation
-- 
2.14.1

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

2017-10-11 Thread Benjamin Tissoires 
On Oct 11 2017 or thereabouts, Jiri Kosina wrote:
> On Wed, 11 Oct 2017, Benjamin Tissoires wrote:
> 
> > I am not sure if libinput even uses ABS_MT_ORIENTATION
> 
> I don't think it does, so that should be okay. However ...

I had a meeting this Peter at noon today. The summary is that libinput
doesn't uses ABS_MT_ORIENTATION, and that the documentation requires
actually 3 things:
- 0 is Y-aligned, up ("north")
- maximum should be aligned with X, pointing toward the right ("east")
- negative and out of range values are allowed

>From this, we can conclude that the minimum doesn't matter, as long as
it is 0 or -max, it is the same from the user space point of view.

One thing that the documentation suggests is that if we report [0, max],
this would indicate that out of ranges values won't be triggered, and
[-max, max] would seem to indicate that the data might be negative and
so out of range values would be acceptable.

Anyway, no changes in any cases from userspace.

> 
> > , but I'd go for fixing the documentation. And re-reading it, it's not 
> > clear that the doc tells us to have [0,90]. It mentions negative values 
> > and out of ranges too, so we might just as well simply clarify that we 
> > rather have [-90,90], with 0 being "north".
> 
> ... I'd like the documentation fix to go in together in one go with this 
> patch if possible.
> 

Sounds like a plan.

Cheers,
Benjamin

> Thanks,
> 
> -- 
> Jiri Kosina
> SUSE Labs
>

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

2017-10-11 Thread Benjamin Tissoires 
On Oct 11 2017 or thereabouts, Jiri Kosina wrote:
> On Wed, 11 Oct 2017, Benjamin Tissoires wrote:
> 
> > I am not sure if libinput even uses ABS_MT_ORIENTATION
> 
> I don't think it does, so that should be okay. However ...

I had a meeting this Peter at noon today. The summary is that libinput
doesn't uses ABS_MT_ORIENTATION, and that the documentation requires
actually 3 things:
- 0 is Y-aligned, up ("north")
- maximum should be aligned with X, pointing toward the right ("east")
- negative and out of range values are allowed

>From this, we can conclude that the minimum doesn't matter, as long as
it is 0 or -max, it is the same from the user space point of view.

One thing that the documentation suggests is that if we report [0, max],
this would indicate that out of ranges values won't be triggered, and
[-max, max] would seem to indicate that the data might be negative and
so out of range values would be acceptable.

Anyway, no changes in any cases from userspace.

> 
> > , but I'd go for fixing the documentation. And re-reading it, it's not 
> > clear that the doc tells us to have [0,90]. It mentions negative values 
> > and out of ranges too, so we might just as well simply clarify that we 
> > rather have [-90,90], with 0 being "north".
> 
> ... I'd like the documentation fix to go in together in one go with this 
> patch if possible.
> 

Sounds like a plan.

Cheers,
Benjamin

> Thanks,
> 
> -- 
> Jiri Kosina
> SUSE Labs
>

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

2017-10-11 Thread Jiri Kosina 
On Wed, 11 Oct 2017, Benjamin Tissoires wrote:

> I am not sure if libinput even uses ABS_MT_ORIENTATION

I don't think it does, so that should be okay. However ...

> , but I'd go for fixing the documentation. And re-reading it, it's not 
> clear that the doc tells us to have [0,90]. It mentions negative values 
> and out of ranges too, so we might just as well simply clarify that we 
> rather have [-90,90], with 0 being "north".

... I'd like the documentation fix to go in together in one go with this 
patch if possible.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

2017-10-11 Thread Jiri Kosina 
On Wed, 11 Oct 2017, Benjamin Tissoires wrote:

> I am not sure if libinput even uses ABS_MT_ORIENTATION

I don't think it does, so that should be okay. However ...

> , but I'd go for fixing the documentation. And re-reading it, it's not 
> clear that the doc tells us to have [0,90]. It mentions negative values 
> and out of ranges too, so we might just as well simply clarify that we 
> rather have [-90,90], with 0 being "north".

... I'd like the documentation fix to go in together in one go with this 
patch if possible.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

2017-10-11 Thread Benjamin Tissoires 
On Oct 10 2017 or thereabouts, Dmitry Torokhov wrote:
> On Mon, Oct 9, 2017 at 11:57 PM, Henrik Rydberg  wrote:
> > Hi Wei-Ning,
> >
> >> The current hid-multitouch driver only allow the report of two
> >> orientations, vertical and horizontal. We use the Azimuth orientation
> >> usage 0x3F under the Digitizer usage page to report orientation if the
> >> device supports it.
> >>
> >> Changelog:
> >>   v1 -> v2:
> >>- Fix commit message.
> >>- Remove resolution reporting for ABS_MT_ORIENTATION.
> >>   v2 -> v3:
> >>- Fix commit message.
> >>   v3 -> v4:
> >>- Fix ABS_MT_ORIENTATION ABS param range.
> >>- Don't set ABS_MT_ORIENTATION in ABS_DG_HEIGHT when it is already
> >>  set by ABS_DG_AZIMUTH.
> >>
> >> Signed-off-by: Wei-Ning Huang 
> >> Reviewed-by: Dmitry Torokhov 
> >> ---
> >>  drivers/hid/hid-multitouch.c | 52 
> >> 
> >>  include/linux/hid.h  |  1 +
> >>  2 files changed, 49 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> >> index 440b999304a5..3317dae64ef7 100644
> >> --- a/drivers/hid/hid-multitouch.c
> >> +++ b/drivers/hid/hid-multitouch.c
> >> @@ -84,11 +84,12 @@ MODULE_LICENSE("GPL");
> >>  #define MT_IO_FLAGS_PENDING_SLOTS2
> >>
> >>  struct mt_slot {
> >> - __s32 x, y, cx, cy, p, w, h;
> >> + __s32 x, y, cx, cy, p, w, h, a;
> >>   __s32 contactid;/* the device ContactID assigned to this 
> >> slot */
> >>   bool touch_state;   /* is the touch valid? */
> >>   bool inrange_state; /* is the finger in proximity of the sensor? 
> >> */
> >>   bool confidence_state;  /* is the touch made by a finger? */
> >> + bool has_azimuth;   /* the contact reports azimuth */
> >>  };
> >>
> >>  struct mt_class {
> >> @@ -571,8 +572,15 @@ static int mt_touch_input_mapping(struct hid_device 
> >> *hdev, struct hid_input *hi,
> >>   if (!(cls->quirks & MT_QUIRK_NO_AREA)) {
> >>   set_abs(hi->input, ABS_MT_TOUCH_MINOR, field,
> >>   cls->sn_height);
> >> - input_set_abs_params(hi->input,
> >> - ABS_MT_ORIENTATION, 0, 1, 0, 0);
> >> +
> >> + /*
> >> +  * Only set ABS_MT_ORIENTATION if it is not
> >> +  * already set by the HID_DG_AZIMUTH usage.
> >> +  */
> >> + if (!test_bit(ABS_MT_ORIENTATION,
> >> + hi->input->absbit))
> >> + input_set_abs_params(hi->input,
> >> + ABS_MT_ORIENTATION, 0, 1, 0, 
> >> 0);
> >>   }
> >>   mt_store_field(usage, td, hi);
> >>   return 1;
> >> @@ -591,6 +599,21 @@ static int mt_touch_input_mapping(struct hid_device 
> >> *hdev, struct hid_input *hi,
> >>   td->cc_index = field->index;
> >>   td->cc_value_index = usage->usage_index;
> >>   return 1;
> >> + case HID_DG_AZIMUTH:
> >> + hid_map_usage(hi, usage, bit, max,
> >> + EV_ABS, ABS_MT_ORIENTATION);
> >> + /*
> >> +  * Azimuth has the range of [0, MAX) representing a 
> >> full
> >> +  * revolution. Set ABS_MT_ORIENTATION to a quarter of
> >> +  * MAX according the definition of ABS_MT_ORIENTATION
> >> +  */
> >> + input_set_abs_params(hi->input, ABS_MT_ORIENTATION,
> >> + -field->logical_maximum / 4,
> >> + field->logical_maximum / 4,
> 
> 
> So do we expect userspace to have special handling for the range when
> "min" is negative? I.e. when range is [0,1] it is understood that we
> are reporting the first quadrant only, with 0 reported when finger is
> roughly vertical and 1 is when it is horizontal. Similarly, the range
> [0-90] would describe the 1st quadrant as well. This matches the
> in-kernel documentation. However here we have [-90, 90] that describes
> 2 quadrants. Do we want to keep it as is and have userspace adapt (we
> probably will need a patch to multi-touch-protocol.txt), or should

Actually, I requested the [-90, 90] change because the only other user
in the kernel I could find that support ABS_MT_ORIENTATION with a
different range than [0,1] was hid-magicmouse and it was not following
the documentation to the letter:
https://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git/tree/drivers/hid/hid-magicmouse.c?h=for-next#n411


After a deeper search, we have (reporting ABS_MT_ORIENTATION different
from

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

2017-10-11 Thread Benjamin Tissoires 
On Oct 10 2017 or thereabouts, Dmitry Torokhov wrote:
> On Mon, Oct 9, 2017 at 11:57 PM, Henrik Rydberg  wrote:
> > Hi Wei-Ning,
> >
> >> The current hid-multitouch driver only allow the report of two
> >> orientations, vertical and horizontal. We use the Azimuth orientation
> >> usage 0x3F under the Digitizer usage page to report orientation if the
> >> device supports it.
> >>
> >> Changelog:
> >>   v1 -> v2:
> >>- Fix commit message.
> >>- Remove resolution reporting for ABS_MT_ORIENTATION.
> >>   v2 -> v3:
> >>- Fix commit message.
> >>   v3 -> v4:
> >>- Fix ABS_MT_ORIENTATION ABS param range.
> >>- Don't set ABS_MT_ORIENTATION in ABS_DG_HEIGHT when it is already
> >>  set by ABS_DG_AZIMUTH.
> >>
> >> Signed-off-by: Wei-Ning Huang 
> >> Reviewed-by: Dmitry Torokhov 
> >> ---
> >>  drivers/hid/hid-multitouch.c | 52 
> >> 
> >>  include/linux/hid.h  |  1 +
> >>  2 files changed, 49 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> >> index 440b999304a5..3317dae64ef7 100644
> >> --- a/drivers/hid/hid-multitouch.c
> >> +++ b/drivers/hid/hid-multitouch.c
> >> @@ -84,11 +84,12 @@ MODULE_LICENSE("GPL");
> >>  #define MT_IO_FLAGS_PENDING_SLOTS2
> >>
> >>  struct mt_slot {
> >> - __s32 x, y, cx, cy, p, w, h;
> >> + __s32 x, y, cx, cy, p, w, h, a;
> >>   __s32 contactid;/* the device ContactID assigned to this 
> >> slot */
> >>   bool touch_state;   /* is the touch valid? */
> >>   bool inrange_state; /* is the finger in proximity of the sensor? 
> >> */
> >>   bool confidence_state;  /* is the touch made by a finger? */
> >> + bool has_azimuth;   /* the contact reports azimuth */
> >>  };
> >>
> >>  struct mt_class {
> >> @@ -571,8 +572,15 @@ static int mt_touch_input_mapping(struct hid_device 
> >> *hdev, struct hid_input *hi,
> >>   if (!(cls->quirks & MT_QUIRK_NO_AREA)) {
> >>   set_abs(hi->input, ABS_MT_TOUCH_MINOR, field,
> >>   cls->sn_height);
> >> - input_set_abs_params(hi->input,
> >> - ABS_MT_ORIENTATION, 0, 1, 0, 0);
> >> +
> >> + /*
> >> +  * Only set ABS_MT_ORIENTATION if it is not
> >> +  * already set by the HID_DG_AZIMUTH usage.
> >> +  */
> >> + if (!test_bit(ABS_MT_ORIENTATION,
> >> + hi->input->absbit))
> >> + input_set_abs_params(hi->input,
> >> + ABS_MT_ORIENTATION, 0, 1, 0, 
> >> 0);
> >>   }
> >>   mt_store_field(usage, td, hi);
> >>   return 1;
> >> @@ -591,6 +599,21 @@ static int mt_touch_input_mapping(struct hid_device 
> >> *hdev, struct hid_input *hi,
> >>   td->cc_index = field->index;
> >>   td->cc_value_index = usage->usage_index;
> >>   return 1;
> >> + case HID_DG_AZIMUTH:
> >> + hid_map_usage(hi, usage, bit, max,
> >> + EV_ABS, ABS_MT_ORIENTATION);
> >> + /*
> >> +  * Azimuth has the range of [0, MAX) representing a 
> >> full
> >> +  * revolution. Set ABS_MT_ORIENTATION to a quarter of
> >> +  * MAX according the definition of ABS_MT_ORIENTATION
> >> +  */
> >> + input_set_abs_params(hi->input, ABS_MT_ORIENTATION,
> >> + -field->logical_maximum / 4,
> >> + field->logical_maximum / 4,
> 
> 
> So do we expect userspace to have special handling for the range when
> "min" is negative? I.e. when range is [0,1] it is understood that we
> are reporting the first quadrant only, with 0 reported when finger is
> roughly vertical and 1 is when it is horizontal. Similarly, the range
> [0-90] would describe the 1st quadrant as well. This matches the
> in-kernel documentation. However here we have [-90, 90] that describes
> 2 quadrants. Do we want to keep it as is and have userspace adapt (we
> probably will need a patch to multi-touch-protocol.txt), or should

Actually, I requested the [-90, 90] change because the only other user
in the kernel I could find that support ABS_MT_ORIENTATION with a
different range than [0,1] was hid-magicmouse and it was not following
the documentation to the letter:
https://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git/tree/drivers/hid/hid-magicmouse.c?h=for-next#n411


After a deeper search, we have (reporting ABS_MT_ORIENTATION different
from [0,1]):
drivers/hid/hid-magicmouse.c -> [-32,32]

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

2017-10-10 Thread Dmitry Torokhov 
On Mon, Oct 9, 2017 at 11:57 PM, Henrik Rydberg  wrote:
> Hi Wei-Ning,
>
>> The current hid-multitouch driver only allow the report of two
>> orientations, vertical and horizontal. We use the Azimuth orientation
>> usage 0x3F under the Digitizer usage page to report orientation if the
>> device supports it.
>>
>> Changelog:
>>   v1 -> v2:
>>- Fix commit message.
>>- Remove resolution reporting for ABS_MT_ORIENTATION.
>>   v2 -> v3:
>>- Fix commit message.
>>   v3 -> v4:
>>- Fix ABS_MT_ORIENTATION ABS param range.
>>- Don't set ABS_MT_ORIENTATION in ABS_DG_HEIGHT when it is already
>>  set by ABS_DG_AZIMUTH.
>>
>> Signed-off-by: Wei-Ning Huang 
>> Reviewed-by: Dmitry Torokhov 
>> ---
>>  drivers/hid/hid-multitouch.c | 52 
>> 
>>  include/linux/hid.h  |  1 +
>>  2 files changed, 49 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 440b999304a5..3317dae64ef7 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -84,11 +84,12 @@ MODULE_LICENSE("GPL");
>>  #define MT_IO_FLAGS_PENDING_SLOTS2
>>
>>  struct mt_slot {
>> - __s32 x, y, cx, cy, p, w, h;
>> + __s32 x, y, cx, cy, p, w, h, a;
>>   __s32 contactid;/* the device ContactID assigned to this slot 
>> */
>>   bool touch_state;   /* is the touch valid? */
>>   bool inrange_state; /* is the finger in proximity of the sensor? */
>>   bool confidence_state;  /* is the touch made by a finger? */
>> + bool has_azimuth;   /* the contact reports azimuth */
>>  };
>>
>>  struct mt_class {
>> @@ -571,8 +572,15 @@ static int mt_touch_input_mapping(struct hid_device 
>> *hdev, struct hid_input *hi,
>>   if (!(cls->quirks & MT_QUIRK_NO_AREA)) {
>>   set_abs(hi->input, ABS_MT_TOUCH_MINOR, field,
>>   cls->sn_height);
>> - input_set_abs_params(hi->input,
>> - ABS_MT_ORIENTATION, 0, 1, 0, 0);
>> +
>> + /*
>> +  * Only set ABS_MT_ORIENTATION if it is not
>> +  * already set by the HID_DG_AZIMUTH usage.
>> +  */
>> + if (!test_bit(ABS_MT_ORIENTATION,
>> + hi->input->absbit))
>> + input_set_abs_params(hi->input,
>> + ABS_MT_ORIENTATION, 0, 1, 0, 
>> 0);
>>   }
>>   mt_store_field(usage, td, hi);
>>   return 1;
>> @@ -591,6 +599,21 @@ static int mt_touch_input_mapping(struct hid_device 
>> *hdev, struct hid_input *hi,
>>   td->cc_index = field->index;
>>   td->cc_value_index = usage->usage_index;
>>   return 1;
>> + case HID_DG_AZIMUTH:
>> + hid_map_usage(hi, usage, bit, max,
>> + EV_ABS, ABS_MT_ORIENTATION);
>> + /*
>> +  * Azimuth has the range of [0, MAX) representing a 
>> full
>> +  * revolution. Set ABS_MT_ORIENTATION to a quarter of
>> +  * MAX according the definition of ABS_MT_ORIENTATION
>> +  */
>> + input_set_abs_params(hi->input, ABS_MT_ORIENTATION,
>> + -field->logical_maximum / 4,
>> + field->logical_maximum / 4,


So do we expect userspace to have special handling for the range when
"min" is negative? I.e. when range is [0,1] it is understood that we
are reporting the first quadrant only, with 0 reported when finger is
roughly vertical and 1 is when it is horizontal. Similarly, the range
[0-90] would describe the 1st quadrant as well. This matches the
in-kernel documentation. However here we have [-90, 90] that describes
2 quadrants. Do we want to keep it as is and have userspace adapt (we
probably will need a patch to multi-touch-protocol.txt), or should
this be:

input_set_abs_params(hi->input, ABS_MT_ORIENTATION, 0,
field->logical_maximum / 4, ...);

and userspace should be able to handle reported negative events and
have them being understood as going counter-clockwise into the 4th and
then 3rd quadrant?

Thanks,
Dmitry

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

2017-10-10 Thread Dmitry Torokhov 
On Mon, Oct 9, 2017 at 11:57 PM, Henrik Rydberg  wrote:
> Hi Wei-Ning,
>
>> The current hid-multitouch driver only allow the report of two
>> orientations, vertical and horizontal. We use the Azimuth orientation
>> usage 0x3F under the Digitizer usage page to report orientation if the
>> device supports it.
>>
>> Changelog:
>>   v1 -> v2:
>>- Fix commit message.
>>- Remove resolution reporting for ABS_MT_ORIENTATION.
>>   v2 -> v3:
>>- Fix commit message.
>>   v3 -> v4:
>>- Fix ABS_MT_ORIENTATION ABS param range.
>>- Don't set ABS_MT_ORIENTATION in ABS_DG_HEIGHT when it is already
>>  set by ABS_DG_AZIMUTH.
>>
>> Signed-off-by: Wei-Ning Huang 
>> Reviewed-by: Dmitry Torokhov 
>> ---
>>  drivers/hid/hid-multitouch.c | 52 
>> 
>>  include/linux/hid.h  |  1 +
>>  2 files changed, 49 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 440b999304a5..3317dae64ef7 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -84,11 +84,12 @@ MODULE_LICENSE("GPL");
>>  #define MT_IO_FLAGS_PENDING_SLOTS2
>>
>>  struct mt_slot {
>> - __s32 x, y, cx, cy, p, w, h;
>> + __s32 x, y, cx, cy, p, w, h, a;
>>   __s32 contactid;/* the device ContactID assigned to this slot 
>> */
>>   bool touch_state;   /* is the touch valid? */
>>   bool inrange_state; /* is the finger in proximity of the sensor? */
>>   bool confidence_state;  /* is the touch made by a finger? */
>> + bool has_azimuth;   /* the contact reports azimuth */
>>  };
>>
>>  struct mt_class {
>> @@ -571,8 +572,15 @@ static int mt_touch_input_mapping(struct hid_device 
>> *hdev, struct hid_input *hi,
>>   if (!(cls->quirks & MT_QUIRK_NO_AREA)) {
>>   set_abs(hi->input, ABS_MT_TOUCH_MINOR, field,
>>   cls->sn_height);
>> - input_set_abs_params(hi->input,
>> - ABS_MT_ORIENTATION, 0, 1, 0, 0);
>> +
>> + /*
>> +  * Only set ABS_MT_ORIENTATION if it is not
>> +  * already set by the HID_DG_AZIMUTH usage.
>> +  */
>> + if (!test_bit(ABS_MT_ORIENTATION,
>> + hi->input->absbit))
>> + input_set_abs_params(hi->input,
>> + ABS_MT_ORIENTATION, 0, 1, 0, 
>> 0);
>>   }
>>   mt_store_field(usage, td, hi);
>>   return 1;
>> @@ -591,6 +599,21 @@ static int mt_touch_input_mapping(struct hid_device 
>> *hdev, struct hid_input *hi,
>>   td->cc_index = field->index;
>>   td->cc_value_index = usage->usage_index;
>>   return 1;
>> + case HID_DG_AZIMUTH:
>> + hid_map_usage(hi, usage, bit, max,
>> + EV_ABS, ABS_MT_ORIENTATION);
>> + /*
>> +  * Azimuth has the range of [0, MAX) representing a 
>> full
>> +  * revolution. Set ABS_MT_ORIENTATION to a quarter of
>> +  * MAX according the definition of ABS_MT_ORIENTATION
>> +  */
>> + input_set_abs_params(hi->input, ABS_MT_ORIENTATION,
>> + -field->logical_maximum / 4,
>> + field->logical_maximum / 4,


So do we expect userspace to have special handling for the range when
"min" is negative? I.e. when range is [0,1] it is understood that we
are reporting the first quadrant only, with 0 reported when finger is
roughly vertical and 1 is when it is horizontal. Similarly, the range
[0-90] would describe the 1st quadrant as well. This matches the
in-kernel documentation. However here we have [-90, 90] that describes
2 quadrants. Do we want to keep it as is and have userspace adapt (we
probably will need a patch to multi-touch-protocol.txt), or should
this be:

input_set_abs_params(hi->input, ABS_MT_ORIENTATION, 0,
field->logical_maximum / 4, ...);

and userspace should be able to handle reported negative events and
have them being understood as going counter-clockwise into the 4th and
then 3rd quadrant?

Thanks,
Dmitry

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

2017-10-10 Thread Benjamin Tissoires 
On Oct 10 2017 or thereabouts, Wei-Ning Huang wrote:
> From: Wei-Ning Huang 
> 
> The current hid-multitouch driver only allow the report of two
> orientations, vertical and horizontal. We use the Azimuth orientation
> usage 0x3F under the Digitizer usage page to report orientation if the
> device supports it.
> 
> Changelog:
>   v1 -> v2:
>- Fix commit message.
>- Remove resolution reporting for ABS_MT_ORIENTATION.
>   v2 -> v3:
>- Fix commit message.
>   v3 -> v4:
>- Fix ABS_MT_ORIENTATION ABS param range.
>- Don't set ABS_MT_ORIENTATION in ABS_DG_HEIGHT when it is already
>  set by ABS_DG_AZIMUTH.
> 
> Signed-off-by: Wei-Ning Huang 
> Reviewed-by: Dmitry Torokhov 

Looks good now:
Reviewed-by: Benjamin Tissoires 

Cheers,
Benjamin

> ---
>  drivers/hid/hid-multitouch.c | 52 
> 
>  include/linux/hid.h  |  1 +
>  2 files changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 440b999304a5..3317dae64ef7 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -84,11 +84,12 @@ MODULE_LICENSE("GPL");
>  #define MT_IO_FLAGS_PENDING_SLOTS2
>  
>  struct mt_slot {
> - __s32 x, y, cx, cy, p, w, h;
> + __s32 x, y, cx, cy, p, w, h, a;
>   __s32 contactid;/* the device ContactID assigned to this slot */
>   bool touch_state;   /* is the touch valid? */
>   bool inrange_state; /* is the finger in proximity of the sensor? */
>   bool confidence_state;  /* is the touch made by a finger? */
> + bool has_azimuth;   /* the contact reports azimuth */
>  };
>  
>  struct mt_class {
> @@ -571,8 +572,15 @@ static int mt_touch_input_mapping(struct hid_device 
> *hdev, struct hid_input *hi,
>   if (!(cls->quirks & MT_QUIRK_NO_AREA)) {
>   set_abs(hi->input, ABS_MT_TOUCH_MINOR, field,
>   cls->sn_height);
> - input_set_abs_params(hi->input,
> - ABS_MT_ORIENTATION, 0, 1, 0, 0);
> +
> + /*
> +  * Only set ABS_MT_ORIENTATION if it is not
> +  * already set by the HID_DG_AZIMUTH usage.
> +  */
> + if (!test_bit(ABS_MT_ORIENTATION,
> + hi->input->absbit))
> + input_set_abs_params(hi->input,
> + ABS_MT_ORIENTATION, 0, 1, 0, 0);
>   }
>   mt_store_field(usage, td, hi);
>   return 1;
> @@ -591,6 +599,21 @@ static int mt_touch_input_mapping(struct hid_device 
> *hdev, struct hid_input *hi,
>   td->cc_index = field->index;
>   td->cc_value_index = usage->usage_index;
>   return 1;
> + case HID_DG_AZIMUTH:
> + hid_map_usage(hi, usage, bit, max,
> + EV_ABS, ABS_MT_ORIENTATION);
> + /*
> +  * Azimuth has the range of [0, MAX) representing a full
> +  * revolution. Set ABS_MT_ORIENTATION to a quarter of
> +  * MAX according the definition of ABS_MT_ORIENTATION
> +  */
> + input_set_abs_params(hi->input, ABS_MT_ORIENTATION,
> + -field->logical_maximum / 4,
> + field->logical_maximum / 4,
> + cls->sn_move ?
> + field->logical_maximum / cls->sn_move : 0, 0);
> + mt_store_field(usage, td, hi);
> + return 1;
>   case HID_DG_CONTACTMAX:
>   /* we don't set td->last_slot_field as contactcount and
>* contact max are global to the report */
> @@ -683,6 +706,10 @@ static void mt_complete_slot(struct mt_device *td, 
> struct input_dev *input)
>   int wide = (s->w > s->h);
>   int major = max(s->w, s->h);
>   int minor = min(s->w, s->h);
> + int orientation = wide;
> +
> + if (s->has_azimuth)
> + orientation = s->a;
>  
>   /*
>* divided by two to match visual scale of touch
> @@ -699,7 +726,8 @@ static void mt_complete_slot(struct mt_device *td, struct 
> input_dev *input)
>   input_event(input, EV_ABS, ABS_MT_TOOL_Y, s->cy);
>   input_event(input, EV_ABS, ABS_MT_DISTANCE,
>   !s->touch_state);
>

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

2017-10-10 Thread Benjamin Tissoires 
On Oct 10 2017 or thereabouts, Wei-Ning Huang wrote:
> From: Wei-Ning Huang 
> 
> The current hid-multitouch driver only allow the report of two
> orientations, vertical and horizontal. We use the Azimuth orientation
> usage 0x3F under the Digitizer usage page to report orientation if the
> device supports it.
> 
> Changelog:
>   v1 -> v2:
>- Fix commit message.
>- Remove resolution reporting for ABS_MT_ORIENTATION.
>   v2 -> v3:
>- Fix commit message.
>   v3 -> v4:
>- Fix ABS_MT_ORIENTATION ABS param range.
>- Don't set ABS_MT_ORIENTATION in ABS_DG_HEIGHT when it is already
>  set by ABS_DG_AZIMUTH.
> 
> Signed-off-by: Wei-Ning Huang 
> Reviewed-by: Dmitry Torokhov 

Looks good now:
Reviewed-by: Benjamin Tissoires 

Cheers,
Benjamin

> ---
>  drivers/hid/hid-multitouch.c | 52 
> 
>  include/linux/hid.h  |  1 +
>  2 files changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 440b999304a5..3317dae64ef7 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -84,11 +84,12 @@ MODULE_LICENSE("GPL");
>  #define MT_IO_FLAGS_PENDING_SLOTS2
>  
>  struct mt_slot {
> - __s32 x, y, cx, cy, p, w, h;
> + __s32 x, y, cx, cy, p, w, h, a;
>   __s32 contactid;/* the device ContactID assigned to this slot */
>   bool touch_state;   /* is the touch valid? */
>   bool inrange_state; /* is the finger in proximity of the sensor? */
>   bool confidence_state;  /* is the touch made by a finger? */
> + bool has_azimuth;   /* the contact reports azimuth */
>  };
>  
>  struct mt_class {
> @@ -571,8 +572,15 @@ static int mt_touch_input_mapping(struct hid_device 
> *hdev, struct hid_input *hi,
>   if (!(cls->quirks & MT_QUIRK_NO_AREA)) {
>   set_abs(hi->input, ABS_MT_TOUCH_MINOR, field,
>   cls->sn_height);
> - input_set_abs_params(hi->input,
> - ABS_MT_ORIENTATION, 0, 1, 0, 0);
> +
> + /*
> +  * Only set ABS_MT_ORIENTATION if it is not
> +  * already set by the HID_DG_AZIMUTH usage.
> +  */
> + if (!test_bit(ABS_MT_ORIENTATION,
> + hi->input->absbit))
> + input_set_abs_params(hi->input,
> + ABS_MT_ORIENTATION, 0, 1, 0, 0);
>   }
>   mt_store_field(usage, td, hi);
>   return 1;
> @@ -591,6 +599,21 @@ static int mt_touch_input_mapping(struct hid_device 
> *hdev, struct hid_input *hi,
>   td->cc_index = field->index;
>   td->cc_value_index = usage->usage_index;
>   return 1;
> + case HID_DG_AZIMUTH:
> + hid_map_usage(hi, usage, bit, max,
> + EV_ABS, ABS_MT_ORIENTATION);
> + /*
> +  * Azimuth has the range of [0, MAX) representing a full
> +  * revolution. Set ABS_MT_ORIENTATION to a quarter of
> +  * MAX according the definition of ABS_MT_ORIENTATION
> +  */
> + input_set_abs_params(hi->input, ABS_MT_ORIENTATION,
> + -field->logical_maximum / 4,
> + field->logical_maximum / 4,
> + cls->sn_move ?
> + field->logical_maximum / cls->sn_move : 0, 0);
> + mt_store_field(usage, td, hi);
> + return 1;
>   case HID_DG_CONTACTMAX:
>   /* we don't set td->last_slot_field as contactcount and
>* contact max are global to the report */
> @@ -683,6 +706,10 @@ static void mt_complete_slot(struct mt_device *td, 
> struct input_dev *input)
>   int wide = (s->w > s->h);
>   int major = max(s->w, s->h);
>   int minor = min(s->w, s->h);
> + int orientation = wide;
> +
> + if (s->has_azimuth)
> + orientation = s->a;
>  
>   /*
>* divided by two to match visual scale of touch
> @@ -699,7 +726,8 @@ static void mt_complete_slot(struct mt_device *td, struct 
> input_dev *input)
>   input_event(input, EV_ABS, ABS_MT_TOOL_Y, s->cy);
>   input_event(input, EV_ABS, ABS_MT_DISTANCE,
>   !s->touch_state);
> - input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide);
> +

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

2017-10-10 Thread Henrik Rydberg 
Hi Wei-Ning,

> The current hid-multitouch driver only allow the report of two
> orientations, vertical and horizontal. We use the Azimuth orientation
> usage 0x3F under the Digitizer usage page to report orientation if the
> device supports it.
> 
> Changelog:
>   v1 -> v2:
>- Fix commit message.
>- Remove resolution reporting for ABS_MT_ORIENTATION.
>   v2 -> v3:
>- Fix commit message.
>   v3 -> v4:
>- Fix ABS_MT_ORIENTATION ABS param range.
>- Don't set ABS_MT_ORIENTATION in ABS_DG_HEIGHT when it is already
>  set by ABS_DG_AZIMUTH.
> 
> Signed-off-by: Wei-Ning Huang 
> Reviewed-by: Dmitry Torokhov 
> ---
>  drivers/hid/hid-multitouch.c | 52 
> 
>  include/linux/hid.h  |  1 +
>  2 files changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 440b999304a5..3317dae64ef7 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -84,11 +84,12 @@ MODULE_LICENSE("GPL");
>  #define MT_IO_FLAGS_PENDING_SLOTS2
>  
>  struct mt_slot {
> - __s32 x, y, cx, cy, p, w, h;
> + __s32 x, y, cx, cy, p, w, h, a;
>   __s32 contactid;/* the device ContactID assigned to this slot */
>   bool touch_state;   /* is the touch valid? */
>   bool inrange_state; /* is the finger in proximity of the sensor? */
>   bool confidence_state;  /* is the touch made by a finger? */
> + bool has_azimuth;   /* the contact reports azimuth */
>  };
>  
>  struct mt_class {
> @@ -571,8 +572,15 @@ static int mt_touch_input_mapping(struct hid_device 
> *hdev, struct hid_input *hi,
>   if (!(cls->quirks & MT_QUIRK_NO_AREA)) {
>   set_abs(hi->input, ABS_MT_TOUCH_MINOR, field,
>   cls->sn_height);
> - input_set_abs_params(hi->input,
> - ABS_MT_ORIENTATION, 0, 1, 0, 0);
> +
> + /*
> +  * Only set ABS_MT_ORIENTATION if it is not
> +  * already set by the HID_DG_AZIMUTH usage.
> +  */
> + if (!test_bit(ABS_MT_ORIENTATION,
> + hi->input->absbit))
> + input_set_abs_params(hi->input,
> + ABS_MT_ORIENTATION, 0, 1, 0, 0);
>   }
>   mt_store_field(usage, td, hi);
>   return 1;
> @@ -591,6 +599,21 @@ static int mt_touch_input_mapping(struct hid_device 
> *hdev, struct hid_input *hi,
>   td->cc_index = field->index;
>   td->cc_value_index = usage->usage_index;
>   return 1;
> + case HID_DG_AZIMUTH:
> + hid_map_usage(hi, usage, bit, max,
> + EV_ABS, ABS_MT_ORIENTATION);
> + /*
> +  * Azimuth has the range of [0, MAX) representing a full
> +  * revolution. Set ABS_MT_ORIENTATION to a quarter of
> +  * MAX according the definition of ABS_MT_ORIENTATION
> +  */
> + input_set_abs_params(hi->input, ABS_MT_ORIENTATION,
> + -field->logical_maximum / 4,
> + field->logical_maximum / 4,
> + cls->sn_move ?
> + field->logical_maximum / cls->sn_move : 0, 0);
> + mt_store_field(usage, td, hi);
> + return 1;
>   case HID_DG_CONTACTMAX:
>   /* we don't set td->last_slot_field as contactcount and
>* contact max are global to the report */
> @@ -683,6 +706,10 @@ static void mt_complete_slot(struct mt_device *td, 
> struct input_dev *input)
>   int wide = (s->w > s->h);
>   int major = max(s->w, s->h);
>   int minor = min(s->w, s->h);
> + int orientation = wide;
> +
> + if (s->has_azimuth)
> + orientation = s->a;
>  
>   /*
>* divided by two to match visual scale of touch
> @@ -699,7 +726,8 @@ static void mt_complete_slot(struct mt_device *td, struct 
> input_dev *input)
>   input_event(input, EV_ABS, ABS_MT_TOOL_Y, s->cy);
>   input_event(input, EV_ABS, ABS_MT_DISTANCE,
>   !s->touch_state);
> - input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide);
> + input_event(input, EV_ABS, ABS_MT_ORIENTATION,
> +

Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

2017-10-10 Thread Henrik Rydberg 
Hi Wei-Ning,

> The current hid-multitouch driver only allow the report of two
> orientations, vertical and horizontal. We use the Azimuth orientation
> usage 0x3F under the Digitizer usage page to report orientation if the
> device supports it.
> 
> Changelog:
>   v1 -> v2:
>- Fix commit message.
>- Remove resolution reporting for ABS_MT_ORIENTATION.
>   v2 -> v3:
>- Fix commit message.
>   v3 -> v4:
>- Fix ABS_MT_ORIENTATION ABS param range.
>- Don't set ABS_MT_ORIENTATION in ABS_DG_HEIGHT when it is already
>  set by ABS_DG_AZIMUTH.
> 
> Signed-off-by: Wei-Ning Huang 
> Reviewed-by: Dmitry Torokhov 
> ---
>  drivers/hid/hid-multitouch.c | 52 
> 
>  include/linux/hid.h  |  1 +
>  2 files changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 440b999304a5..3317dae64ef7 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -84,11 +84,12 @@ MODULE_LICENSE("GPL");
>  #define MT_IO_FLAGS_PENDING_SLOTS2
>  
>  struct mt_slot {
> - __s32 x, y, cx, cy, p, w, h;
> + __s32 x, y, cx, cy, p, w, h, a;
>   __s32 contactid;/* the device ContactID assigned to this slot */
>   bool touch_state;   /* is the touch valid? */
>   bool inrange_state; /* is the finger in proximity of the sensor? */
>   bool confidence_state;  /* is the touch made by a finger? */
> + bool has_azimuth;   /* the contact reports azimuth */
>  };
>  
>  struct mt_class {
> @@ -571,8 +572,15 @@ static int mt_touch_input_mapping(struct hid_device 
> *hdev, struct hid_input *hi,
>   if (!(cls->quirks & MT_QUIRK_NO_AREA)) {
>   set_abs(hi->input, ABS_MT_TOUCH_MINOR, field,
>   cls->sn_height);
> - input_set_abs_params(hi->input,
> - ABS_MT_ORIENTATION, 0, 1, 0, 0);
> +
> + /*
> +  * Only set ABS_MT_ORIENTATION if it is not
> +  * already set by the HID_DG_AZIMUTH usage.
> +  */
> + if (!test_bit(ABS_MT_ORIENTATION,
> + hi->input->absbit))
> + input_set_abs_params(hi->input,
> + ABS_MT_ORIENTATION, 0, 1, 0, 0);
>   }
>   mt_store_field(usage, td, hi);
>   return 1;
> @@ -591,6 +599,21 @@ static int mt_touch_input_mapping(struct hid_device 
> *hdev, struct hid_input *hi,
>   td->cc_index = field->index;
>   td->cc_value_index = usage->usage_index;
>   return 1;
> + case HID_DG_AZIMUTH:
> + hid_map_usage(hi, usage, bit, max,
> + EV_ABS, ABS_MT_ORIENTATION);
> + /*
> +  * Azimuth has the range of [0, MAX) representing a full
> +  * revolution. Set ABS_MT_ORIENTATION to a quarter of
> +  * MAX according the definition of ABS_MT_ORIENTATION
> +  */
> + input_set_abs_params(hi->input, ABS_MT_ORIENTATION,
> + -field->logical_maximum / 4,
> + field->logical_maximum / 4,
> + cls->sn_move ?
> + field->logical_maximum / cls->sn_move : 0, 0);
> + mt_store_field(usage, td, hi);
> + return 1;
>   case HID_DG_CONTACTMAX:
>   /* we don't set td->last_slot_field as contactcount and
>* contact max are global to the report */
> @@ -683,6 +706,10 @@ static void mt_complete_slot(struct mt_device *td, 
> struct input_dev *input)
>   int wide = (s->w > s->h);
>   int major = max(s->w, s->h);
>   int minor = min(s->w, s->h);
> + int orientation = wide;
> +
> + if (s->has_azimuth)
> + orientation = s->a;
>  
>   /*
>* divided by two to match visual scale of touch
> @@ -699,7 +726,8 @@ static void mt_complete_slot(struct mt_device *td, struct 
> input_dev *input)
>   input_event(input, EV_ABS, ABS_MT_TOOL_Y, s->cy);
>   input_event(input, EV_ABS, ABS_MT_DISTANCE,
>   !s->touch_state);
> - input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide);
> + input_event(input, EV_ABS, ABS_MT_ORIENTATION,
> + orientation);
>

[PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

2017-10-09 Thread Wei-Ning Huang 
From: Wei-Ning Huang 

The current hid-multitouch driver only allow the report of two
orientations, vertical and horizontal. We use the Azimuth orientation
usage 0x3F under the Digitizer usage page to report orientation if the
device supports it.

Changelog:
  v1 -> v2:
   - Fix commit message.
   - Remove resolution reporting for ABS_MT_ORIENTATION.
  v2 -> v3:
   - Fix commit message.
  v3 -> v4:
   - Fix ABS_MT_ORIENTATION ABS param range.
   - Don't set ABS_MT_ORIENTATION in ABS_DG_HEIGHT when it is already
 set by ABS_DG_AZIMUTH.

Signed-off-by: Wei-Ning Huang 
Reviewed-by: Dmitry Torokhov 
---
 drivers/hid/hid-multitouch.c | 52 
 include/linux/hid.h  |  1 +
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 440b999304a5..3317dae64ef7 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -84,11 +84,12 @@ MODULE_LICENSE("GPL");
 #define MT_IO_FLAGS_PENDING_SLOTS  2
 
 struct mt_slot {
-   __s32 x, y, cx, cy, p, w, h;
+   __s32 x, y, cx, cy, p, w, h, a;
__s32 contactid;/* the device ContactID assigned to this slot */
bool touch_state;   /* is the touch valid? */
bool inrange_state; /* is the finger in proximity of the sensor? */
bool confidence_state;  /* is the touch made by a finger? */
+   bool has_azimuth;   /* the contact reports azimuth */
 };
 
 struct mt_class {
@@ -571,8 +572,15 @@ static int mt_touch_input_mapping(struct hid_device *hdev, 
struct hid_input *hi,
if (!(cls->quirks & MT_QUIRK_NO_AREA)) {
set_abs(hi->input, ABS_MT_TOUCH_MINOR, field,
cls->sn_height);
-   input_set_abs_params(hi->input,
-   ABS_MT_ORIENTATION, 0, 1, 0, 0);
+
+   /*
+* Only set ABS_MT_ORIENTATION if it is not
+* already set by the HID_DG_AZIMUTH usage.
+*/
+   if (!test_bit(ABS_MT_ORIENTATION,
+   hi->input->absbit))
+   input_set_abs_params(hi->input,
+   ABS_MT_ORIENTATION, 0, 1, 0, 0);
}
mt_store_field(usage, td, hi);
return 1;
@@ -591,6 +599,21 @@ static int mt_touch_input_mapping(struct hid_device *hdev, 
struct hid_input *hi,
td->cc_index = field->index;
td->cc_value_index = usage->usage_index;
return 1;
+   case HID_DG_AZIMUTH:
+   hid_map_usage(hi, usage, bit, max,
+   EV_ABS, ABS_MT_ORIENTATION);
+   /*
+* Azimuth has the range of [0, MAX) representing a full
+* revolution. Set ABS_MT_ORIENTATION to a quarter of
+* MAX according the definition of ABS_MT_ORIENTATION
+*/
+   input_set_abs_params(hi->input, ABS_MT_ORIENTATION,
+   -field->logical_maximum / 4,
+   field->logical_maximum / 4,
+   cls->sn_move ?
+   field->logical_maximum / cls->sn_move : 0, 0);
+   mt_store_field(usage, td, hi);
+   return 1;
case HID_DG_CONTACTMAX:
/* we don't set td->last_slot_field as contactcount and
 * contact max are global to the report */
@@ -683,6 +706,10 @@ static void mt_complete_slot(struct mt_device *td, struct 
input_dev *input)
int wide = (s->w > s->h);
int major = max(s->w, s->h);
int minor = min(s->w, s->h);
+   int orientation = wide;
+
+   if (s->has_azimuth)
+   orientation = s->a;
 
/*
 * divided by two to match visual scale of touch
@@ -699,7 +726,8 @@ static void mt_complete_slot(struct mt_device *td, struct 
input_dev *input)
input_event(input, EV_ABS, ABS_MT_TOOL_Y, s->cy);
input_event(input, EV_ABS, ABS_MT_DISTANCE,
!s->touch_state);
-   input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide);
+   input_event(input, EV_ABS, ABS_MT_ORIENTATION,
+   orientation);
input_event(input, EV_ABS,

[PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting

2017-10-09 Thread Wei-Ning Huang 
From: Wei-Ning Huang 

The current hid-multitouch driver only allow the report of two
orientations, vertical and horizontal. We use the Azimuth orientation
usage 0x3F under the Digitizer usage page to report orientation if the
device supports it.

Changelog:
  v1 -> v2:
   - Fix commit message.
   - Remove resolution reporting for ABS_MT_ORIENTATION.
  v2 -> v3:
   - Fix commit message.
  v3 -> v4:
   - Fix ABS_MT_ORIENTATION ABS param range.
   - Don't set ABS_MT_ORIENTATION in ABS_DG_HEIGHT when it is already
 set by ABS_DG_AZIMUTH.

Signed-off-by: Wei-Ning Huang 
Reviewed-by: Dmitry Torokhov 
---
 drivers/hid/hid-multitouch.c | 52 
 include/linux/hid.h  |  1 +
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 440b999304a5..3317dae64ef7 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -84,11 +84,12 @@ MODULE_LICENSE("GPL");
 #define MT_IO_FLAGS_PENDING_SLOTS  2
 
 struct mt_slot {
-   __s32 x, y, cx, cy, p, w, h;
+   __s32 x, y, cx, cy, p, w, h, a;
__s32 contactid;/* the device ContactID assigned to this slot */
bool touch_state;   /* is the touch valid? */
bool inrange_state; /* is the finger in proximity of the sensor? */
bool confidence_state;  /* is the touch made by a finger? */
+   bool has_azimuth;   /* the contact reports azimuth */
 };
 
 struct mt_class {
@@ -571,8 +572,15 @@ static int mt_touch_input_mapping(struct hid_device *hdev, 
struct hid_input *hi,
if (!(cls->quirks & MT_QUIRK_NO_AREA)) {
set_abs(hi->input, ABS_MT_TOUCH_MINOR, field,
cls->sn_height);
-   input_set_abs_params(hi->input,
-   ABS_MT_ORIENTATION, 0, 1, 0, 0);
+
+   /*
+* Only set ABS_MT_ORIENTATION if it is not
+* already set by the HID_DG_AZIMUTH usage.
+*/
+   if (!test_bit(ABS_MT_ORIENTATION,
+   hi->input->absbit))
+   input_set_abs_params(hi->input,
+   ABS_MT_ORIENTATION, 0, 1, 0, 0);
}
mt_store_field(usage, td, hi);
return 1;
@@ -591,6 +599,21 @@ static int mt_touch_input_mapping(struct hid_device *hdev, 
struct hid_input *hi,
td->cc_index = field->index;
td->cc_value_index = usage->usage_index;
return 1;
+   case HID_DG_AZIMUTH:
+   hid_map_usage(hi, usage, bit, max,
+   EV_ABS, ABS_MT_ORIENTATION);
+   /*
+* Azimuth has the range of [0, MAX) representing a full
+* revolution. Set ABS_MT_ORIENTATION to a quarter of
+* MAX according the definition of ABS_MT_ORIENTATION
+*/
+   input_set_abs_params(hi->input, ABS_MT_ORIENTATION,
+   -field->logical_maximum / 4,
+   field->logical_maximum / 4,
+   cls->sn_move ?
+   field->logical_maximum / cls->sn_move : 0, 0);
+   mt_store_field(usage, td, hi);
+   return 1;
case HID_DG_CONTACTMAX:
/* we don't set td->last_slot_field as contactcount and
 * contact max are global to the report */
@@ -683,6 +706,10 @@ static void mt_complete_slot(struct mt_device *td, struct 
input_dev *input)
int wide = (s->w > s->h);
int major = max(s->w, s->h);
int minor = min(s->w, s->h);
+   int orientation = wide;
+
+   if (s->has_azimuth)
+   orientation = s->a;
 
/*
 * divided by two to match visual scale of touch
@@ -699,7 +726,8 @@ static void mt_complete_slot(struct mt_device *td, struct 
input_dev *input)
input_event(input, EV_ABS, ABS_MT_TOOL_Y, s->cy);
input_event(input, EV_ABS, ABS_MT_DISTANCE,
!s->touch_state);
-   input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide);
+   input_event(input, EV_ABS, ABS_MT_ORIENTATION,
+   orientation);
input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p);
input_event(input,