Linux Input Archive on lore.kernel.org
 help / color / Atom feed
From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
To: Jiri Kosina <jikos@kernel.org>
Cc: наб <nabijaczleweli@nabijaczleweli.xyz>,
	"Peter Hutterer" <peter.hutterer@who-t.net>,
	"open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
	lkml <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 0/4] Stylus-on-touchscreen device support
Date: Mon, 3 May 2021 11:52:43 +0200
Message-ID: <CAO-hwJKLYTqQ9qZ8LZGabze_NjNfwAq2_V-28LFewjOC=EK8rw@mail.gmail.com> (raw)
In-Reply-To: <CAO-hwJ+HhKU7EzPZGOtPePT_h7OUaJ=QfWi7eAcxsfRaDtQuvg@mail.gmail.com>

On Mon, May 3, 2021 at 11:39 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Mon, May 3, 2021 at 11:11 AM Jiri Kosina <jikos@kernel.org> wrote:
> >
> > On Tue, 20 Apr 2021, наб wrote:
> >
> > > > This patchset adds support for stylus-on-touchscreen devices as found on
> > > > the OneMix 3 Pro and Dell Inspiron 15 7000 2-in-1 (7591), among others;
> > > > with it, they properly behave like a drawing tablet.
> > > >
> > > > Patches 2 and 4 funxionally depend on patch 1.
> > > > Patch 4 needs patch 3 to apply.
> > > >
> > > > The output of this patchset and the need for a kernel, rather than
> > > > userspace, patch was previously discussed here:
> > > >   https://gitlab.freedesktop.org/libinput/libinput/-/merge_requests/558#note_792834
> > > >
> > > > Changes in v2:
> > > > Patch 4 now ANDs the secondary button with the tip switch,
> > > > since it's otherwise borderline useless to the user.
> > > >
> > > > Ahelenia Ziemiańska (4):
> > > >   HID: multitouch: require Finger field to mark Win8 reports as MT
> > > >   HID: multitouch: set Stylus suffix for Stylus-application devices, too
> > > >   HID: input: replace outdated HID numbers+comments with macros
> > > >   HID: input: work around Win8 stylus-on-touchscreen reporting
> > > >
> > > >  drivers/hid/hid-input.c      | 54 ++++++++++++++++++++++++++++++++++--
> > > >  drivers/hid/hid-multitouch.c | 18 +++++++-----
> > > >  2 files changed, 62 insertions(+), 10 deletions(-)
> > > >
> > > > --
> > > > 2.20.1
> > >
> > > Bumping this after a monthish ‒ is it missing something? Am I?
> >
> > Benjamin had concerns about regressions and wanted to run a full battery
> > of testing on it.
> >
> > Benjamin, is there any outcome of that, please?
> >
>
> Sorry, no real outcome here.
>
> I ran the test suite, and there were no errors, until I realized that
> there are no tests regarding tablets, so it can't detect any
> regressions here.
> And then, the usual happens, no time to actually work on the test suite... :(
>
> I'll do a "normal" review soon (i.e. today)
>

So I did a quick pass at the patches:
- 1/4 -> I think this one is safe and could go as it is, maybe with
CC: stable on it. Any regressions should have been caught by the
testsuite, so that's a good one.
- 2/4 and 3/4 -> Ack on those 2 too, note stable material, but not
necessary v5.13 material
- 4/4 -> I honestly have no idea if the patch is correct or not. I
would hold on this one until we have proper tests for those.

Jiri, would you be ok to split the series as this?

наб, would you be OK to work on the test suite at
https://gitlab.freedesktop.org/libevdev/hid-tools so we can move
forward for your last patch?

The problem I see on the last patch is that it is touching a generic
path and is not trivial. So adding tests would have 3 benefits:
- we ensure we are doing the correct thing
- we ensure we are not breaking existing devices (to some extent,
given that the tests are non written for the tablets)
- we ensure we are not breaking that in the future.

Cheers,
Benjamin


  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-08 17:41 наб
2021-03-08 17:42 ` [PATCH v2 1/4] HID: multitouch: require Finger field to mark Win8 reports as MT Ahelenia Ziemiańska
2021-03-08 17:42 ` [PATCH v2 2/4] HID: multitouch: set Stylus suffix for Stylus-application devices, too Ahelenia Ziemiańska
2021-03-08 17:42 ` [PATCH v2 3/4] HID: input: replace outdated HID numbers+comments with macros Ahelenia Ziemiańska
2021-03-08 17:42 ` [PATCH v2 4/4] HID: input: work around Win8 stylus-on-touchscreen reporting Ahelenia Ziemiańska
2021-04-20 13:17 ` [PATCH v2 0/4] Stylus-on-touchscreen device support наб
2021-05-03  9:11   ` Jiri Kosina
2021-05-03  9:39     ` Benjamin Tissoires
2021-05-03  9:52       ` Benjamin Tissoires [this message]
2021-05-03 13:00         ` наб
2021-05-04 13:35           ` Benjamin Tissoires
2021-05-05 13:07         ` Jiri Kosina

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAO-hwJKLYTqQ9qZ8LZGabze_NjNfwAq2_V-28LFewjOC=EK8rw@mail.gmail.com' \
    --to=benjamin.tissoires@redhat.com \
    --cc=jikos@kernel.org \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nabijaczleweli@nabijaczleweli.xyz \
    --cc=peter.hutterer@who-t.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux Input Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-input/0 linux-input/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-input linux-input/ https://lore.kernel.org/linux-input \
		linux-input@vger.kernel.org
	public-inbox-index linux-input

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-input


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git