| Message ID | 1533688303-2912-1-git-send-email-tobita.tatsunosuke@wacom.co.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Input: Set INPUT_PROP_-property for HID_UP_DIGITIZERS | expand |
Hi, Would anyone like to review this patch? If there's any problem inserting the changes in the lines, I would like to discuss about implementing INPUT_PROP_TABLET and INPUT_PROP_DIRECT more proper way. Thanks, Tats -----Original Message----- From: linux-input-owner@vger.kernel.org <linux-input-owner@vger.kernel.org> On Behalf Of Tatsunosuke Tobita Sent: Wednesday, August 8, 2018 9:32 AM To: linux-input@vger.kernel.org Cc: Tobita, Tatsunosuke <tatsunosuke.tobita@wacom.com> Subject: [PATCH] Input: Set INPUT_PROP_-property for HID_UP_DIGITIZERS Some system may want to know if a detected digitizer device is either an integrated or an external device. In order to distinguish such condition, setting either INPUT_PROP_DIRECT or INPUT_PROP_POINTER is required, checking the member, "application", in "hid_field" structure. Signed-off-by: Tatsunosuke Tobita <tobita.tatsunosuke@wacom.co.jp> --- drivers/hid/hid-input.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index 930652c..3e98537 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -758,6 +758,11 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel break; case HID_UP_DIGITIZER: + if ((field->application & 0xff) == 0x01) /* Digitizer */ + __set_bit(INPUT_PROP_POINTER, input->propbit); + else if ((field->application & 0xff) == 0x02) /* Pen */ + __set_bit(INPUT_PROP_DIRECT, input->propbit); + switch (usage->hid & 0xff) { case 0x00: /* Undefined */ goto ignore; -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 8 Aug 2018, Tatsunosuke Tobita wrote: > Some system may want to know if a detected digitizer > device is either an integrated or an external device. > In order to distinguish such condition, setting either > INPUT_PROP_DIRECT or INPUT_PROP_POINTER is required, checking > the member, "application", in "hid_field" structure. > > Signed-off-by: Tatsunosuke Tobita <tobita.tatsunosuke@wacom.co.jp> > --- > drivers/hid/hid-input.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c > index 930652c..3e98537 100644 > --- a/drivers/hid/hid-input.c > +++ b/drivers/hid/hid-input.c > @@ -758,6 +758,11 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel > break; > > case HID_UP_DIGITIZER: > + if ((field->application & 0xff) == 0x01) /* Digitizer */ > + __set_bit(INPUT_PROP_POINTER, input->propbit); > + else if ((field->application & 0xff) == 0x02) /* Pen */ > + __set_bit(INPUT_PROP_DIRECT, input->propbit); > + > switch (usage->hid & 0xff) { > case 0x00: /* Undefined */ > goto ignore; Makes sense. Queued for 4.20, thanks.
On Wed, Sep 5, 2018 at 5:01 PM Jiri Kosina <jikos@kernel.org> wrote: > > On Wed, 8 Aug 2018, Tatsunosuke Tobita wrote: > > > Some system may want to know if a detected digitizer > > device is either an integrated or an external device. > > In order to distinguish such condition, setting either > > INPUT_PROP_DIRECT or INPUT_PROP_POINTER is required, checking > > the member, "application", in "hid_field" structure. > > > > Signed-off-by: Tatsunosuke Tobita <tobita.tatsunosuke@wacom.co.jp> > > --- > > drivers/hid/hid-input.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c > > index 930652c..3e98537 100644 > > --- a/drivers/hid/hid-input.c > > +++ b/drivers/hid/hid-input.c > > @@ -758,6 +758,11 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel > > break; > > > > case HID_UP_DIGITIZER: > > + if ((field->application & 0xff) == 0x01) /* Digitizer */ > > + __set_bit(INPUT_PROP_POINTER, input->propbit); > > + else if ((field->application & 0xff) == 0x02) /* Pen */ > > + __set_bit(INPUT_PROP_DIRECT, input->propbit); The one thing I am worried here is that the specification for "pen" usage states: "A digitizer with an integrated display that allows use of a stylus. The system must ensure that the sensed stylus position and the display representations of that position are the same. A pen digitizer has enough time and space resolution for handwriting input. " This is good, its corresponds to the patch. However, it then says in the same paragraph: "A digitizer that *may or may not* be in an integrated display application should use the more generic Digitizer collection usage. " So as I read it, "Pen" implies PROP_DIRECT, but "Digitizer" doesn't imply INPUT_PROP_POINTER. This might be the usage, so I don't know if am I just bikeshedding or if I am raising a valid point... Cheers, Benjamin > > + > > switch (usage->hid & 0xff) { > > case 0x00: /* Undefined */ > > goto ignore; > > Makes sense. > > Queued for 4.20, thanks. > > -- > Jiri Kosina > SUSE Labs >
Hi Benjamin, Thanks I got your point. I agree with you that it is unnecessary to set anything on a digitizer which is not distinguished neither integrated nor external. I will make another patch without setting "INPUT_PROP_POINTER". Thanks, Tats -----Original Message----- From: Benjamin Tissoires <benjamin.tissoires@gmail.com> Sent: Thursday, September 6, 2018 1:24 AM To: Jiri Kosina <jikos@kernel.org> Cc: junkpainting@gmail.com; linux-input <linux-input@vger.kernel.org>; Tobita, Tatsunosuke <tatsunosuke.tobita@wacom.com>; Dmitry Torokhov <dmitry.torokhov@gmail.com>; Jason Gerecke <killertofu@gmail.com>; Ping Cheng <pinglinux@gmail.com> Subject: Re: [PATCH] Input: Set INPUT_PROP_-property for HID_UP_DIGITIZERS On Wed, Sep 5, 2018 at 5:01 PM Jiri Kosina <jikos@kernel.org> wrote: > > On Wed, 8 Aug 2018, Tatsunosuke Tobita wrote: > > > Some system may want to know if a detected digitizer device is > > either an integrated or an external device. > > In order to distinguish such condition, setting either > > INPUT_PROP_DIRECT or INPUT_PROP_POINTER is required, checking the > > member, "application", in "hid_field" structure. > > > > Signed-off-by: Tatsunosuke Tobita <tobita.tatsunosuke@wacom.co.jp> > > --- > > drivers/hid/hid-input.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index > > 930652c..3e98537 100644 > > --- a/drivers/hid/hid-input.c > > +++ b/drivers/hid/hid-input.c > > @@ -758,6 +758,11 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel > > break; > > > > case HID_UP_DIGITIZER: > > + if ((field->application & 0xff) == 0x01) /* Digitizer */ > > + __set_bit(INPUT_PROP_POINTER, input->propbit); > > + else if ((field->application & 0xff) == 0x02) /* Pen */ > > + __set_bit(INPUT_PROP_DIRECT, input->propbit); The one thing I am worried here is that the specification for "pen" usage states: "A digitizer with an integrated display that allows use of a stylus. The system must ensure that the sensed stylus position and the display representations of that position are the same. A pen digitizer has enough time and space resolution for handwriting input. " This is good, its corresponds to the patch. However, it then says in the same paragraph: "A digitizer that *may or may not* be in an integrated display application should use the more generic Digitizer collection usage. " So as I read it, "Pen" implies PROP_DIRECT, but "Digitizer" doesn't imply INPUT_PROP_POINTER. This might be the usage, so I don't know if am I just bikeshedding or if I am raising a valid point... Cheers, Benjamin > > + > > switch (usage->hid & 0xff) { > > case 0x00: /* Undefined */ > > goto ignore; > > Makes sense. > > Queued for 4.20, thanks. > > -- > Jiri Kosina > SUSE Labs >
On Thu, Sep 6, 2018 at 12:12 AM Tobita, Tatsunosuke <tatsunosuke.tobita@wacom.com> wrote: > > Hi Benjamin, > > Thanks I got your point. I agree with you that it is unnecessary to set anything on a digitizer which is not > distinguished neither integrated nor external. > > I will make another patch without setting "INPUT_PROP_POINTER". > > Thanks, > > Tats > > -----Original Message----- > From: Benjamin Tissoires <benjamin.tissoires@gmail.com> > Sent: Thursday, September 6, 2018 1:24 AM > To: Jiri Kosina <jikos@kernel.org> > Cc: junkpainting@gmail.com; linux-input <linux-input@vger.kernel.org>; Tobita, Tatsunosuke <tatsunosuke.tobita@wacom.com>; Dmitry Torokhov <dmitry.torokhov@gmail.com>; Jason Gerecke <killertofu@gmail.com>; Ping Cheng <pinglinux@gmail.com> > Subject: Re: [PATCH] Input: Set INPUT_PROP_-property for HID_UP_DIGITIZERS > > On Wed, Sep 5, 2018 at 5:01 PM Jiri Kosina <jikos@kernel.org> wrote: > > > > On Wed, 8 Aug 2018, Tatsunosuke Tobita wrote: > > > > > Some system may want to know if a detected digitizer device is > > > either an integrated or an external device. > > > In order to distinguish such condition, setting either > > > INPUT_PROP_DIRECT or INPUT_PROP_POINTER is required, checking the > > > member, "application", in "hid_field" structure. > > > > > > Signed-off-by: Tatsunosuke Tobita <tobita.tatsunosuke@wacom.co.jp> > > > --- > > > drivers/hid/hid-input.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index > > > 930652c..3e98537 100644 > > > --- a/drivers/hid/hid-input.c > > > +++ b/drivers/hid/hid-input.c > > > @@ -758,6 +758,11 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel > > > break; > > > > > > case HID_UP_DIGITIZER: > > > + if ((field->application & 0xff) == 0x01) /* Digitizer */ > > > + __set_bit(INPUT_PROP_POINTER, input->propbit); > > > + else if ((field->application & 0xff) == 0x02) /* Pen */ > > > + __set_bit(INPUT_PROP_DIRECT, input->propbit); > > The one thing I am worried here is that the specification for "pen" > usage states: > "A digitizer with an integrated display that allows use of a stylus. > The system must ensure that the sensed stylus position and the display representations of that position are the same. A pen digitizer has enough time and space resolution for handwriting input. " > This is good, its corresponds to the patch. > > However, it then says in the same paragraph: "A digitizer that *may or may not* be in an integrated display application should use the more generic Digitizer collection usage. " > > So as I read it, "Pen" implies PROP_DIRECT, but "Digitizer" doesn't imply INPUT_PROP_POINTER. > > This might be the usage, so I don't know if am I just bikeshedding or if I am raising a valid point... > > Cheers, > Benjamin > That seems like a valid observation... My initial reaction was that not setting INPUT_PROP_POINTER would be problematic for non-display tablets: there's little harm in showing a pointer if a "Digitizer" is actually integrated into a display, but serious usability issues if a pointer is not shown for a non-display tablet. As long as *no* properties are set, however, that's not really an issue -- that just means it's userspace's job to figure out properties, and userspace should err on the side of showing a pointer unless something else trips it up. I think I'm happy with putting my Reviewed-by stamp on it either way. Jason --- Now instead of four in the eights place / you’ve got three, ‘Cause you added one / (That is to say, eight) to the two, / But you can’t take seven from three, / So you look at the sixty-fours.... > > > + > > > switch (usage->hid & 0xff) { > > > case 0x00: /* Undefined */ > > > goto ignore; > > > > Makes sense. > > > > Queued for 4.20, thanks. > > > > -- > > Jiri Kosina > > SUSE Labs > >
On Fri, Sep 7, 2018 at 1:18 PM Tobita, Tatsunosuke <tatsunosuke.tobita@wacom.com> wrote: > > Hi Benjamin, > > Sory to say, but I've changed my mind. > After thingking again about with/without setting INPUT_PROP_POINTER. > I ended to leave it. I want to apply my patch as it was submitted. > The reason is because as Jason told, it is helpful for the external digitizers with the pointer showing on screen when they are plugged. > This helps users visually recognize if their devices are working; it's > > easy to see the reactions of the devices for them. On the other hand, reaction to an integrated digitizer in a display can be seen with/without a pointer. > > What do you think? OK. We'll see in the long run if we need to blacklist devices from the userspace or if the cheap tablets manufacturers play nice with us on tihs front. > > I'd like to apply this patch if you have no critical concern about the definition of "Digitizer" usage. I have my concerns, I exposed them, but they are not critical :) So we can keep the already scheduled for 4.20 patch in its current state and refer to this discussion later when (if) problems arise. Cheers, Benjamin > > Thanks, > > Tats > > > > ________________________________ > 差出人: Jason Gerecke <killertofu@gmail.com> > 送信日時: 2018年9月7日 0:02:32 > 宛先: Tobita, Tatsunosuke > CC: Benjamin Tissoires; Jiri Kosina; Tatsunosuke Tobita; Linux Input; Dmitry Torokhov; Ping Cheng > 件名: Re: [PATCH] Input: Set INPUT_PROP_-property for HID_UP_DIGITIZERS > > On Thu, Sep 6, 2018 at 12:12 AM Tobita, Tatsunosuke > <tatsunosuke.tobita@wacom.com> wrote: > > > > Hi Benjamin, > > > > Thanks I got your point. I agree with you that it is unnecessary to set anything on a digitizer which is not > > distinguished neither integrated nor external. > > > > I will make another patch without setting "INPUT_PROP_POINTER". > > > > Thanks, > > > > Tats > > > > -----Original Message----- > > From: Benjamin Tissoires <benjamin.tissoires@gmail.com> > > Sent: Thursday, September 6, 2018 1:24 AM > > To: Jiri Kosina <jikos@kernel.org> > > Cc: junkpainting@gmail.com; linux-input <linux-input@vger.kernel.org>; Tobita, Tatsunosuke <tatsunosuke.tobita@wacom.com>; Dmitry Torokhov <dmitry.torokhov@gmail.com>; Jason Gerecke <killertofu@gmail.com>; Ping Cheng <pinglinux@gmail.com> > > Subject: Re: [PATCH] Input: Set INPUT_PROP_-property for HID_UP_DIGITIZERS > > > > On Wed, Sep 5, 2018 at 5:01 PM Jiri Kosina <jikos@kernel.org> wrote: > > > > > > On Wed, 8 Aug 2018, Tatsunosuke Tobita wrote: > > > > > > > Some system may want to know if a detected digitizer device is > > > > either an integrated or an external device. > > > > In order to distinguish such condition, setting either > > > > INPUT_PROP_DIRECT or INPUT_PROP_POINTER is required, checking the > > > > member, "application", in "hid_field" structure. > > > > > > > > Signed-off-by: Tatsunosuke Tobita <tobita.tatsunosuke@wacom.co.jp> > > > > --- > > > > drivers/hid/hid-input.c | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index > > > > 930652c..3e98537 100644 > > > > --- a/drivers/hid/hid-input.c > > > > +++ b/drivers/hid/hid-input.c > > > > @@ -758,6 +758,11 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel > > > > break; > > > > > > > > case HID_UP_DIGITIZER: > > > > + if ((field->application & 0xff) == 0x01) /* Digitizer */ > > > > + __set_bit(INPUT_PROP_POINTER, input->propbit); > > > > + else if ((field->application & 0xff) == 0x02) /* Pen */ > > > > + __set_bit(INPUT_PROP_DIRECT, input->propbit); > > > > The one thing I am worried here is that the specification for "pen" > > usage states: > > "A digitizer with an integrated display that allows use of a stylus. > > The system must ensure that the sensed stylus position and the display representations of that position are the same. A pen digitizer has enough time and space resolution for handwriting input. " > > This is good, its corresponds to the patch. > > > > However, it then says in the same paragraph: "A digitizer that *may or may not* be in an integrated display application should use the more generic Digitizer collection usage. " > > > > So as I read it, "Pen" implies PROP_DIRECT, but "Digitizer" doesn't imply INPUT_PROP_POINTER. > > > > This might be the usage, so I don't know if am I just bikeshedding or if I am raising a valid point... > > > > Cheers, > > Benjamin > > > > That seems like a valid observation... My initial reaction was that > not setting INPUT_PROP_POINTER would be problematic for non-display > tablets: there's little harm in showing a pointer if a "Digitizer" is > actually integrated into a display, but serious usability issues if a > pointer is not shown for a non-display tablet. As long as *no* > properties are set, however, that's not really an issue -- that just > means it's userspace's job to figure out properties, and userspace > should err on the side of showing a pointer unless something else > trips it up. > > I think I'm happy with putting my Reviewed-by stamp on it either way. > > Jason > --- > Now instead of four in the eights place / > you’ve got three, ‘Cause you added one / > (That is to say, eight) to the two, / > But you can’t take seven from three, / > So you look at the sixty-fours.... > > > > > + > > > > switch (usage->hid & 0xff) { > > > > case 0x00: /* Undefined */ > > > > goto ignore; > > > > > > Makes sense. > > > > > > Queued for 4.20, thanks. > > > > > > -- > > > Jiri Kosina > > > SUSE Labs > > >
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index 930652c..3e98537 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -758,6 +758,11 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel break; case HID_UP_DIGITIZER: + if ((field->application & 0xff) == 0x01) /* Digitizer */ + __set_bit(INPUT_PROP_POINTER, input->propbit); + else if ((field->application & 0xff) == 0x02) /* Pen */ + __set_bit(INPUT_PROP_DIRECT, input->propbit); + switch (usage->hid & 0xff) { case 0x00: /* Undefined */ goto ignore;
Some system may want to know if a detected digitizer device is either an integrated or an external device. In order to distinguish such condition, setting either INPUT_PROP_DIRECT or INPUT_PROP_POINTER is required, checking the member, "application", in "hid_field" structure. Signed-off-by: Tatsunosuke Tobita <tobita.tatsunosuke@wacom.co.jp> --- drivers/hid/hid-input.c | 5 +++++ 1 file changed, 5 insertions(+)