lkml.org 
[lkml]   [2017]   [Oct]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3] HID: hid-multitouch: support fine-grain orientation reporting
Hi Benjamin,

Thanks for the review. I've fixed according to your comments and sent
the v4 patch:
https://patchwork.kernel.org/patch/9995003/

Wei-Ning

On Fri, Oct 6, 2017 at 4:13 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> Hi,
>
> On Oct 05 2017 or thereabouts, Jiri Kosina wrote:
>>
>> [ adding Benjamin to CC ]
>>
>> On Thu, 28 Sep 2017, Wei-Ning Huang wrote:
>>
>> > 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.
>> >
>> > Signed-off-by: Wei-Ning Huang <wnhuang@google.com>
>> > Reviewed-by: Dmitry Torokhov <dtor@chromium.org>
>> > ---
>> > drivers/hid/hid-multitouch.c | 35 +++++++++++++++++++++++++++++++++--
>> > include/linux/hid.h | 1 +
>> > 2 files changed, 34 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> > index 440b999304a5..f9801d1b1eae 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 {
>> > @@ -591,6 +592,20 @@ 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,
>> > + 0, field->logical_maximum / 4,
>
> Shouldn't this be "-field->logical_maximum / 4, field->logical_maximum / 4,"?
>
> Also, if HID_DG_HEIGHT is seen after HID_DG_AZIMUTH, it'll change the
> ABS_MT_ORIENTATION you just set here.
>
>> > + 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 +698,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 +718,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, EV_ABS, ABS_MT_TOUCH_MAJOR, major);
>> > input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, minor);
>> > @@ -789,6 +809,17 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
>> > break;
>> > case HID_DG_CONTACTCOUNT:
>> > break;
>> > + case HID_DG_AZIMUTH:
>> > + /*
>> > + * Azimuth is counter-clockwise and ranges from [0, MAX)
>> > + * (a full revolution). Convert it to clockwise ranging
>> > + * [-MAX/2, MAX/2].
>
> It took me a while to get this right. The problem is the definition of
> ABS_MT_ORIENTATION says that we report [-MAX/4, MAX/4], *but* we can go
> out of range to [-MAX/2, MAX/2] to report upside down ellipsis.
>
> So this is correct, but I'd like to have some mention of [-MAX/4, MAX/4]
> in this comment, to ease further rereading of the code.
>
> Cheers,
> Benjamin
>
>> > + */
>> > + if (value > field->logical_maximum / 2)
>> > + value -= field->logical_maximum;
>> > + td->curdata.a = -value;
>> > + td->curdata.has_azimuth = true;
>> > + break;
>> > case HID_DG_TOUCH:
>> > /* do nothing */
>> > break;
>> > diff --git a/include/linux/hid.h b/include/linux/hid.h
>> > index ab05a86269dc..d81b9b6fd83a 100644
>> > --- a/include/linux/hid.h
>> > +++ b/include/linux/hid.h
>> > @@ -281,6 +281,7 @@ struct hid_item {
>> >
>> > #define HID_DG_DEVICECONFIG 0x000d000e
>> > #define HID_DG_DEVICESETTINGS 0x000d0023
>> > +#define HID_DG_AZIMUTH 0x000d003f
>> > #define HID_DG_CONFIDENCE 0x000d0047
>> > #define HID_DG_WIDTH 0x000d0048
>> > #define HID_DG_HEIGHT 0x000d0049
>> > --
>> > 2.12.2
>> >
>>
>> --
>> Jiri Kosina
>> SUSE Labs
>>



--
Wei-Ning Huang, 黃偉寧 | Software Engineer, Google Inc., Taiwan |
wnhuang@google.com | Cell: +886 910-380678

\
 
 \ /
  Last update: 2017-10-10 06:22    [W:0.048 / U:0.464 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site