From: Martin Kepplinger <martink@posteo.de>
To: Oliver Neukum <oneukum@suse.com>
Cc: dmitry.torokhov@gmail.com, kernel-testers@vger.kernel.org,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-usb@vger.kernel.org
Subject: Re: [PATCH v3] input: tablet: add Pegasus Notetaker tablet driver
Date: Mon, 23 May 2016 19:19:01 +0200 [thread overview]
Message-ID: <57433B85.1050301@posteo.de> (raw)
In-Reply-To: <1464008447.12181.58.camel@suse.com>
Am 2016-05-23 um 15:00 schrieb Oliver Neukum:
> On Mon, 2016-05-23 at 14:43 +0200, Martin Kepplinger wrote:
>> Am 2016-05-23 um 14:26 schrieb Oliver Neukum:
>>> On Mon, 2016-05-23 at 13:39 +0200, Martin Kepplinger wrote:
>>>
>>>> It's *really* fun to use as an input tablet though! So let's support this
>>>> for everybody.
>>>
>>> Hi,
>>>
>>> I am afraid there are a few issues.
>>
>> Thanks for having a look, comments below.
>>
>>>
>>> 1. Why the kernel thread?
>>
>> I provide switching to the xy tablet mode by pressing a button during
>> runtime. So usb_control_msg() is called from the thread, not the
>> interrupt routine.
>>
>> We don't get in the way of any user app. If we leave that functionality
>> out, users may have to un- and replug in the device after using
>> userspace apps that transfer data or the like.
>
> The functionality is OK, but the implementation, though creative,
> is buggy.
>
> a - multiple devices will fail
> b - you have a race condition that will fail to wake the thread
> if it is still running when the button is pressed again
>
> Kernel threads are wasteful. Using the normal system work queue might be
> much easier
You're right. I'll definitely change this.
>
>>> 2. This driver has questionable power management.
>>
>> I think I do it how others do it. Suspend/resume works. Anything
>> specific required here?
>
> Runtime PM is missing.
There is no way to turn the device off. So like any other mouse or input
tablet we don't use runtime PM.
In case the Pen is out of range or off, we just don't handle URBs and do
nothing.
... but I may miss something, so in this case please get back to me.
thanks
martin
>
> Regards
> Oliver
>
>
prev parent reply other threads:[~2016-05-23 17:19 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-23 11:39 [PATCH v3] input: tablet: add Pegasus Notetaker tablet driver Martin Kepplinger
[not found] ` <1464003592-24043-1-git-send-email-martink-1KBjaw7Xf1+zQB+pC5nmwQ@public.gmane.org>
2016-05-23 12:26 ` Oliver Neukum
2016-05-23 12:43 ` Martin Kepplinger
[not found] ` <5742FB0F.5070508-1KBjaw7Xf1+zQB+pC5nmwQ@public.gmane.org>
2016-05-23 13:00 ` Oliver Neukum
2016-05-23 17:19 ` Martin Kepplinger [this message]
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=57433B85.1050301@posteo.de \
--to=martink@posteo.de \
--cc=dmitry.torokhov@gmail.com \
--cc=kernel-testers@vger.kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=oneukum@suse.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).