* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Al Viro @ 2018-03-21 0:05 UTC (permalink / raw)
To: Linus Torvalds
Cc: Kees Cook, Florian Weimer, Andrew Morton, Josh Poimboeuf,
Rasmus Villemoes, Randy Dunlap, Miguel Ojeda, Ingo Molnar,
David Laight, Ian Abbott, linux-input, linux-btrfs,
Network Development, Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <CA+55aFwO8KZD_tZwqwL05FbzpKW4Ucd88C0tcR7LJ1utuy3WGg@mail.gmail.com>
On Tue, Mar 20, 2018 at 04:26:52PM -0700, Linus Torvalds wrote:
> On Tue, Mar 20, 2018 at 4:23 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Hmm. So thanks to the diseased mind of Martin Uecker, there's a better
> > test for "__is_constant()":
> >
> > /* Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de> */
> > #define __is_constant(a) \
> > (sizeof(int) == sizeof(*(1 ? ((void*)((a) * 0l)) : (int*)1)))
> >
> > that is actually *specified* by the C standard to work, and doesn't
> > even depend on any gcc extensions.
>
> Well, it does depend on 'sizeof(*(void *)X)' being 1 and the compiler
> not complaining about it, and that sizeof(int) is not 1.
>
> But since we depend on those things in the kernel anyway, that's fine.
It also depends upon "ICE for null pointer constant purposes" having the
same extensions as "ICE for enum purposes", etc., which is not obvious.
Back in 2007 or so we had a long thread regarding null pointer constants
in sparse; I probably still have notes from back then, but that'll take
some serious digging to find ;-/
What's more, gcc definitely has odd extensions. Example I remember from
back then:
extern unsigned n;
struct {
int x : 1 + n - n;
} y;
is accepted. Used to be quietly accepted with -Wpedantic -std=c99, even,
but that got fixed - with -Wpedantic it does, at least, warn. What is
and what is not recognized is fairly random - 1 + n - n + 1 + n - n
is recognized as "constant", 1 + n + n + 1 - n - n is not. Of course,
neither is an ICE.
^ permalink raw reply
* [PATCH v3] Fix modifier keys for Redragon Asura Keyboard
From: Robert Munteanu @ 2018-03-21 10:43 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-kernel, linux-input, Robert Munteanu
The microdia family of keyboards uses a non-standard way of sending
modifier keys.
The down event always sets the first bit to 0x04 and the second keycode
to a custom value For instance, left shift sends the following bits
04 02 00 00 00 00 00 00
while left control sends
04 01 00 00 00 00 00 00
As a result all modifier keys are mapped to left shift and the keyboard is
non-functional in that respect. To solve the problem, we capture the
raw data in raw_event and manually generate the correct input events.
The keyboard functions mostly as expected now, with only a few minor
issues:
- two USB devices are detected instead of 1
- some key combinations are not triggered - e.g.
left shift + left ctrl + p. However, the same combination is recognized
with the right shift key.
Changelog:
- v2: modifier keys work, some combinations are still troublesome
- v3: style cleanup, rebase on top of 4.14
- v4: remove most debugging calls, make init info useful for user,
rebased on top of 4.15
Signed-off-by: Robert Munteanu <rombert@apache.org>
---
drivers/hid/Kconfig | 9 +++
drivers/hid/Makefile | 2 +-
drivers/hid/hid-core.c | 3 +
drivers/hid/hid-ids.h | 3 +
drivers/hid/hid-microdia.c | 148 +++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 164 insertions(+), 1 deletion(-)
create mode 100644 drivers/hid/hid-microdia.c
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 779c5ae47f36..c3350f2ec4ea 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -548,6 +548,15 @@ config HID_MAYFLASH
Say Y here if you have HJZ Mayflash PS3 game controller adapters
and want to enable force feedback support.
+config HID_MICRODIA
+ tristate "Microdia based keyboards"
+ depends on HID
+ default !EXPERT
+ ---help---
+ Support for Microdia devices that are not compliant with the HID standard.
+
+ One known example if the Redragon Asura Keyboard.
+
config HID_MICROSOFT
tristate "Microsoft non-fully HID-compliant devices"
depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 235bd2a7b333..e66a305876c5 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -62,6 +62,7 @@ obj-$(CONFIG_HID_LOGITECH_DJ) += hid-logitech-dj.o
obj-$(CONFIG_HID_LOGITECH_HIDPP) += hid-logitech-hidpp.o
obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
obj-$(CONFIG_HID_MAYFLASH) += hid-mf.o
+obj-$(CONFIG_HID_MICRODIA) += hid-microdia.o
obj-$(CONFIG_HID_MICROSOFT) += hid-microsoft.o
obj-$(CONFIG_HID_MONTEREY) += hid-monterey.o
obj-$(CONFIG_HID_MULTITOUCH) += hid-multitouch.o
@@ -121,4 +122,3 @@ obj-$(CONFIG_USB_KBD) += usbhid/
obj-$(CONFIG_I2C_HID) += i2c-hid/
-obj-$(CONFIG_INTEL_ISH_HID) += intel-ish-hid/
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 0c3f608131cf..b36c2df4b755 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2393,6 +2393,9 @@ static const struct hid_device_id hid_have_special_driver[] = {
#endif
#if IS_ENABLED(CONFIG_HID_ZYDACRON)
{ HID_USB_DEVICE(USB_VENDOR_ID_ZYDACRON, USB_DEVICE_ID_ZYDACRON_REMOTE_CONTROL) },
+#endif
+#if IS_ENABLED(CONFIG_HID_MICRODIA)
+ { HID_USB_DEVICE(USB_VENDOR_ID_MICRODIA, USB_DEVICE_ID_REDRAGON_ASURA) },
#endif
{ }
};
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 5da3d6256d25..146869e55c5a 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -1171,4 +1171,7 @@
#define USB_VENDOR_ID_UGTIZER 0x2179
#define USB_DEVICE_ID_UGTIZER_TABLET_GP0610 0x0053
+#define USB_VENDOR_ID_MICRODIA 0x0c45
+#define USB_DEVICE_ID_REDRAGON_ASURA 0x760b
+
#endif
diff --git a/drivers/hid/hid-microdia.c b/drivers/hid/hid-microdia.c
new file mode 100644
index 000000000000..f9d8de18a989
--- /dev/null
+++ b/drivers/hid/hid-microdia.c
@@ -0,0 +1,148 @@
+/*
+ * HID driver for Microdia-based keyboards
+ *
+ * Copyright (c) 2017 Robert Munteanu
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/input.h>
+#include <linux/hid.h>
+#include <linux/module.h>
+#include <linux/log2.h>
+#include <linux/input-event-codes.h>
+
+#include "hid-ids.h"
+
+
+// The microdia family of keyboards uses a non-standard way of sending
+// modifier keys
+//
+// The down event always sets the first bit to 0x04 and the second keycode
+// to a custom value. For instance, left shift sends the following bits
+//
+// 04 02 00 00 00 00 00 00
+//
+// while left control sends
+//
+// 04 01 00 00 00 00 00 00
+//
+// Unfortunately these are all mapped to left shift and the keyboard is
+// non-functional in that respect. To solve the problem, we capture the
+// raw data in raw_event and manually generate the correct input events
+//
+// TODO
+//
+// 1. Some modifiers keys still don't work, e.g. Ctrl-Shift-P
+// Interestingly enough, this happens when pressing the physical
+// left shift key, but now when pressing the physical right shift key.
+// hexdump does not show them either.
+// 2. A second USB keyboard is registered, but it should be removed
+// 3. Modifier keys sometimes get stuck, need to re-press to clear
+
+static int microdia_input_configured(struct hid_device *hdev,
+ struct hid_input *hi)
+{
+
+ hid_info(hdev, "Keyboard configured with limited support (modifier keys may get stuck, some modifiers combinations with left-shift not working) ");
+
+ hid_set_drvdata(hdev, hi);
+
+ return 0;
+}
+
+static int microdia_input_mapping(struct hid_device *hdev, struct hid_input *hi,
+ struct hid_field *field, struct hid_usage *usage,
+ unsigned long **bit, int *max)
+{
+ // do not map left shift since we will manually generate input
+ // events in microdia_raw_event
+ if ((usage->hid & HID_USAGE_PAGE) == HID_UP_KEYBOARD)
+ if ((usage->hid & HID_USAGE) == 0xe1)
+ return -1;
+
+ return 0;
+}
+
+// array index of a modifier matches the bit index in the data payload
+// the modifier data is always stored in the second int
+// e.g. for left alt the 1 bit is set - 04 04 ...
+// TODO - second entry should be left shift, but that's not possible
+// since we ignore it in the mapping
+static int microdia_modifiers[7] = {
+ KEY_LEFTCTRL,
+ KEY_RIGHTSHIFT,
+ KEY_LEFTALT,
+ KEY_LEFTMETA,
+ KEY_RIGHTCTRL,
+ KEY_RIGHTSHIFT,
+ KEY_RIGHTALT
+};
+
+static int microdia_last_handled_modifier;
+
+static int microdia_raw_event(struct hid_device *hdev,
+ struct hid_report *report, u8 *data, int size)
+{
+ int i, changed_key, new_value, mapped_key;
+ struct hid_input *hi;
+
+ // 1. validate that we get 8 bits of the for 04 xx 00 00 00 00 00 00
+ if (size != 8)
+ return 0;
+
+ if (data[0] != 4)
+ return 0;
+
+ // TODO - can I somehow use a bit mask? data & 0x00ffffff != 0
+ for (i = 2; i < size; i++)
+ if (data[i] != 0)
+ return 0;
+
+ // TODO - don't process the keyup event 04 00 00 00 00 00 00 00
+ // if we don't expect a modifier key 'up' event
+
+ // 2. detect based on previous data what key was pressed or depressed
+ // a key was pressed or depressed if its bit has a different value
+ // between this and the previous invocation. If the bit is set
+ // it was pressed
+
+ changed_key = data[1] ^ microdia_last_handled_modifier;
+ new_value = (data[1] & changed_key) != 0;
+ // TODO - is ilog2 really needed?
+ mapped_key = microdia_modifiers[ilog2(changed_key)];
+
+ // 3. report the key event and track what was sent
+ hi = hid_get_drvdata(hdev);
+
+ input_report_key(hi->input, mapped_key, new_value);
+
+ microdia_last_handled_modifier = data[1];
+
+ return 1;
+}
+
+static const struct hid_device_id microdia_devices[] = {
+ {HID_USB_DEVICE(USB_VENDOR_ID_MICRODIA, USB_DEVICE_ID_REDRAGON_ASURA)},
+ {}
+};
+
+MODULE_DEVICE_TABLE(hid, microdia_devices);
+
+static struct hid_driver microdia_driver = {
+ .name = "microdia",
+ .input_mapping = microdia_input_mapping,
+ .id_table = microdia_devices,
+ .raw_event = microdia_raw_event,
+ .input_configured = microdia_input_configured,
+};
+
+module_hid_driver(microdia_driver);
+
+MODULE_LICENSE("GPL");
--
2.16.2
^ permalink raw reply related
* Re: [PATCH] HID: ntrig: document sysfs interface
From: Jonathan Corbet @ 2018-03-21 15:28 UTC (permalink / raw)
To: Aishwarya Pant
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
Greg KH, Julia Lawall, linux-doc
In-Reply-To: <20180302053017.GA23123@mordor.localdomain>
On Fri, 2 Mar 2018 11:00:17 +0530
Aishwarya Pant <aishpant@gmail.com> wrote:
> Add sysfs documentation for N-Trig touchscreens under Documentation/ABI.
> Descriptions have been collected from code comments.
Applied to the docs tree, thanks.
jon
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: document sysfs interface
From: Jonathan Corbet @ 2018-03-21 15:28 UTC (permalink / raw)
To: Aishwarya Pant
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
Greg KH, Julia Lawall, linux-doc
In-Reply-To: <20180302131653.GA24934@mordor.localdomain>
On Fri, 2 Mar 2018 18:46:53 +0530
Aishwarya Pant <aishpant@gmail.com> wrote:
> Descriptions have been collected from git commit logs.
Applied to the docs tree, thanks.
jon
^ permalink raw reply
* Re: [PATCH] HID: ntrig: document sysfs interface
From: Jonathan Corbet @ 2018-03-21 15:31 UTC (permalink / raw)
To: Aishwarya Pant
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
Greg KH, Julia Lawall, linux-doc
In-Reply-To: <20180321092805.279a8dde@lwn.net>
On Wed, 21 Mar 2018 09:28:05 -0600
Jonathan Corbet <corbet@lwn.net> wrote:
> > Add sysfs documentation for N-Trig touchscreens under Documentation/ABI.
> > Descriptions have been collected from code comments.
>
> Applied to the docs tree, thanks.
Oops, I thought I'd checked to see whether Jiri had picked these up, but I
evidently haven't had enough coffee yet. Since they're taken care of,
I'll unapply them; sorry for the noise.
jon
^ permalink raw reply
* Re: [PATCH] Input: trackpoint: document sysfs interface
From: Jonathan Corbet @ 2018-03-21 15:35 UTC (permalink / raw)
To: Aishwarya Pant
Cc: Dmitry Torokhov, linux-input, linux-kernel, Greg KH, Julia Lawall,
linux-doc
In-Reply-To: <20180302173019.GA27660@mordor.localdomain>
On Fri, 2 Mar 2018 23:00:19 +0530
Aishwarya Pant <aishpant@gmail.com> wrote:
> Descriptions have been collected from git commit logs, code commits and
> the TrackPoint System Version 4.0 Engineering Specification.
Applied to the docs tree, thanks.
jon
^ permalink raw reply
* Re: [PATCH v5 0/4] new driver for Valve Steam Controller
From: Benjamin Tissoires @ 2018-03-21 15:47 UTC (permalink / raw)
To: Rodrigo Rivas Costa
Cc: Pierre-Loup A. Griffais, Clément VUCHENER, Jiri Kosina,
Cameron Gutman, lkml, linux-input
In-Reply-To: <20180319200813.GA18746@casa>
Hi Rodrigo,
On Mon, Mar 19, 2018 at 9:08 PM, Rodrigo Rivas Costa
<rodrigorivascosta@gmail.com> wrote:
> On Sat, Mar 17, 2018 at 02:54:07PM -0700, Pierre-Loup A. Griffais wrote:
>>
>>
>> On 03/15/2018 02:06 PM, Rodrigo Rivas Costa wrote:
>> > On Wed, Mar 14, 2018 at 05:39:25PM +0100, Benjamin Tissoires wrote:
>> > > On Mon, Mar 12, 2018 at 9:51 PM, Rodrigo Rivas Costa
>> > > <rodrigorivascosta@gmail.com> wrote:
>> > > > On Mon, Mar 12, 2018 at 03:30:43PM +0100, Clément VUCHENER wrote:
>> > > > > 2018-03-11 20:58 GMT+01:00 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>:
>> > > > > > This patchset implements a driver for Valve Steam Controller, based on a
>> > > > > > reverse analysis by myself.
>> > > > > >
>> > > > > > Sorry, I've been out of town for a few weeks and couldn't keep up with this...
>> > > > > >
>> > > > > > @Pierre-Loup and @Clément, could you please have another look at this and
>> > > > > > check if it is worthy? Benjamin will not commit it without an express ACK from
>> > > > > > Valve. Of course he is right to be cautious, but I checked this driver with
>> > > > > > the Steam Client and all seems to go just fine. I think that there is a lot of
>> > > > > > Linux out of the desktop that could use this driver and cannot use the Steam
>> > > > > > Client. Worst case scenario, this driver can now be blacklisted, but I hope
>> > > > > > that will not be needed.
>> > > > >
>> > > > > I tested the driver with my 4.15 fedora kernel (I only built the
>> > > > > module not the whole kernel) and I got double inputs (your driver
>> > > > > input device + steam uinput device) when testing Shovel Knight with
>> > > > > Steam Big Picture. It seems to work fine when the inputs are the same,
>> > > > > but after changing the controller configuration in Steam, the issue
>> > > > > became apparent.
>> > > >
>> > > > I assumed that when several joysticks are available, games would listen
>> > > > to one one of them. It looks like I'm wrong, and some (many?) games will
>> > > > listen to all available joysticks at the same time. Thus having two
>> > > > logical joysticks that represent the same physical one is not good.
>> > >
>> > > Yeah, the general rule of thumb is "think of the worst thing that can
>> > > happen, someone will do something worst".
>> > >
>> > > >
>> > > > An easy solution would be that Steam Client grabs this driver
>> > > > (ioctl(EVIOCGRAB)) when creating the uinput device. Another solution
>> > > > would be that Steam Client blacklists this driver, of course.
>> > >
>> > > This is 2 solutions that rely on a userspace change, and this is not
>> > > acceptable in its current form. What if people do not upgrade Steam
>> > > client but upgrade their kernel? Well, Steam might be able to force
>> > > people to always run the latest shiny available version, but for other
>> > > games, you can't expect people to have a compatible version of the
>> > > userspace stack.
>> >
>> > Well, if you don't have Steam then you don't have the double input in
>> > the first place. Unless you are using a different user-mode driver, of
>> > course.
>> > >
>> > > Also, "blacklisting the driver" from Steam client is something the OS
>> > > can do, but not the client when you run on a different distribution.
>> > > You need root for that, and I don't want to give root permissions to
>> > > Steam (or to any user space client that shouldn't have root privileges
>> > > for what it matters).
>> >
>> > Actually Steam needs a system installation that adds udev rules to grant
>> > uaccess to /dev/uinput and the USB raw device for the controller.
>> > Adding a /etc/modprobe.d/steam.conf should be possible, too. It would be
>> > a bit inconvenient because you'll need a distro update of the steam
>> > package, not just the usual user-mode-only auto-update.
>>
>> It's definitely a bit tricky; we've rolled out an update to our reference
>> package whenever we've added support for new devices (the final Steam
>> Controller, direct PS4 gamepad led/gyro access through HID, HTC Vive and its
>> myriad of onboard devices, bootloaders of all these things for firmware
>> updates, etc). Whenever we have to do that, the rollout never is as smooth
>> as desired: many users aren't using our own package; even on the
>> distributions we support directly. Then downstream distributions adopt these
>> udev changes with various delays (sometimes never), and port them to their
>> own mechanism of doing things, since everyone has their own idea of a robust
>> security model. I wish local sessions always had proper access to HID
>> devices connected to the physical computer the user is sitting at, but I
>> realize that the basic gaming desktop is just one of many usecases distros
>> out there have to support and it'd be unreasonable to expect them to focus
>> exclusively on it.
>
> I was afraid of something like that. Let's forget about that option for
> the moment.
>
>> > > > > And without Steam and your external tool, you get double inputs too. I
>> > > > > tried RetroArch and it was unusable because of the keyboard inputs
>> > > > > from the lizard mode (e.g. pressing B also presses Esc and quits
>> > > > > RetroArch). Having to download and compile an external tool to make
>> > > > > the driver work properly may be too difficult for the user. Your goal
>> > > > > was to provide an alternative to user space drivers but now you
>> > > > > actually depend on (a very simple) one.
>> > > >
>> > > > Yes, I noticed that. TBH, this driver without Steam Client or the
>> > > > user-space tool is not very nice, precisely because you'll get constant
>> > > > Escape and Enter presses, and most games react to those.
>> > > >
>> > > > Frankly speaking, I'm not sure how to proceed. I can think of the
>> > > > following options:
>> > > > 1.Steam Client installation could add a file to blacklist
>> > > > hid-steam, just as it adds a few udev rules.
>> > >
>> > > But what about RetroArch? And what if you install Steam but want to
>> > > play SDL games that could benefit from your driver?
>> >
>> > That is an issue of solution 1. I actually have the module blacklisted
>> > in my PC, and run `sudo modprobe hid-steam` to use SDL.
>> >
>> > > > 2.The default CONFIG_HID_STEAM can be changed to "n". Maybe only
>> > > > on the architectures for which there is a Steam Client available.
>> > > > This way DIY projects will still be able to use it.
>> > >
>> > > But this will make the decision to include or not the driver in
>> > > distributions harder. And if no distribution uses it, you won't have
>> > > free tests, and you will be alone to maintain it. So that's not ideal
>> > > either
>> >
>> > Could we set the default to 'y' in non-PC systems. It would be enabled
>> > in my Raspbian, for example... better than nothing.
>> > >
>> > > > 3.This driver could be abandoned :-(. Just use Steam Client if possible or
>> > > > any of the user-mode drivers available.
>> > >
>> > > This would be a waste for everybody as it's always better when we share.
>> >
>> > Indeed!
>> >
>> > I tried a new option:
>> > 4. The driver detects whether the DEVUSB/HIDRAW device is in use, and
>> > if that is the case it will disable itself. If the DEVUSB/HIDRAW is
>> > not in use, then the driver will work normally. A bit hackish maybe
>> > but it should work.
>> >
>> > I tried doing this option 4, but I'm unable to do it properly. I don't
>> > even know if it is possible...
>> >
>> > > >
>> > > > If we decide for 1 or 2, then the lizard mode could be disabled without
>> > > > ill effects. We could even enable the gyro and all the other gadgets
>> > > > without worring about current compatibility.
>> > >
>> > > To me, 1 is out of the question. The kernel can't expect a user space
>> > > change especially if you are knowingly introducing a bug for the end
>> > > user.
>> > >
>> > > 2 is still on the table IMO, and 3 would be a shame.
>> > >
>> > > I know we already discussed about sysfs and module parameters, but if
>> > > the driver will conflict with a userspace stack, the only way would be
>> > > to have a (dynamic) parameter "enhanced_mode" or
>> > > "kernel_awesome_support" or whatever which would be a boolean, that
>> > > defaults to false that Steam can eventually lookup if they want so in
>> > > the future we can default it to true. When this parameter is set, the
>> > > driver will create the inputs and toggle the various modes, while when
>> > > it's toggled off, it'll clean up itself and keep the device as if it
>> > > were connected to hid-generic. Bonus point, this removes the need for
>> > > the simple user space tool that enables the mode.
>> >
>> > That is doable, but that sysfs/parameter can be changed by a non-root
>> > user? I looked for a udev rule to grant access to those but found
>> > nothing.
>> >
>> > IIUC, when this parameter is false the driver will do nothing, right?
>> > The user will just need to change it to true to be able to use it, but
>> > that will have to be done by root.
>> >
>> > I'll try doing this, but I'd appreciate your advice about what approach
>> > would be better: sysfs? a module parameter? a cdev? or even a EV_MSC?
>> >
>> > > > At the end of the day, I think that it is up to Valve what to do.
>> > >
>> > > Again, Valve is a big player here, but do not underestimate other
>> > > projects (like RetroArch mentioned above) because if you break their
>> > > workflow, they will have the right to request a revert of the commit
>> > > because it breaks some random user playing games in the far end of
>> > > Antarctica (yes, penguins do love to play games :-P )
>> >
>> > And everybody loves penguins! If we take away Steam (say a RaspberryPi
>> > as a canonical example) and disable the lizard mode, then this driver is
>> > just a regular gamepad. RetroArch should be happy with that. Unless they
>> > already have an user mode driver for the steam-controller, of course...
>>
>> Both of these things seem reasonable to me, with a few caveats:
>>
>> - If there's an opt-in mechanism available, it would be good to ensure we
>> have a way to reliably query its state without requiring extra permissions.
>> This way, if we know it's likely to affect Steam client functionality, we'll
>> have the right mechanism to properly message the situation to users.
>>
>> - If you find a way for the client to be able to program an opt out when
>> it's running that is not automatic (eg. by detecting the hidraw device being
>> opened), we'd be happy to participate with that scheme assuming it doesn't
The only concern I have with detecting if hidraw is opened or not is
that there are other tools that will want to open hidraw to do
something entirely different. But I agree, this is the least intrusive
solution as far as I can see.
>> require extra permissions. As soon as the API is figured out, we can include
>> it in the client, just send me a heads-up. The one thing that I'd be
>> cautious of is robust behavior against abnormal client termination. If it's
>> a sysfs entry we have to poke, I'm worried that if the client crashes we
>> might not always be able to opt the driver back out. It'd be nice if it was
>> based on opening an fd instead, this way the kernel would robustly clean up
>> after us and your driver would kick back in.
>
> Ok, I've written the following scheme:
> * I've added a member to struct steam_device: int hid_usage_count.
> * Whenver I call hid_hw_open(), hid_usage_count is incremented.
> * Whenver I call hid_hw_close(), hid_usage_count is decremented.
>
> Now, with this little function:
>
> static bool steam_is_only_client(struct steam_device *steam)
> {
> unsigned int count = steam->hdev->ll_open_count;
> return count == steam->hid_usage_count;
> }
>
> I can check if the hidraw device is opened from user-space. It is nice
> because it works not only for SteamClient, but any other user-space
> hid driver. And it is resistent to crashes, too.
>
> It is a bit hacky because I don't think ll_open_count is intended for
Definitively too hacky for me :)
It seems to be working, but AFAIU, there is still the emulated
keyboard/mouse in place that call hid_hw_open() when they are opened.
So you do not really know if the mismatch comes from extra eyes on the
other hidinput devices or from hidraw.
> that, and it is usually protected by a mutex... The proper way, IMO,
> would be to have a callback for when the hidraw is first opened/last
> closed, but that does not exist, AFAICT.
The callback doesn't exists because no one actually had the need to.
This particular use case could be a reason to implement such system.
However, I think we should think of the design of the 'callback'
really carefully so it is generic enough to be reused for other cases.
One thing that would be of a help here would be if you overwrite the
low-level transport driver (struct hid_ll_driver) in hid-steam
directly. You basically just duplicate the current ll_driver and
overwrite only the functions you are interested in.
If, for instance, you add a hint to hid_hw_open/close() telling who
opened/closed the device, you could create some new entries in
hid_ll_driver .opening and .closed that would be called before
entering/leaving hid_hw_open/close().
You could even snoop the various requests in hid_hw_(raw_)request() in
case you know what to expect from the client.
This is just an idea and not a formal ack, but it might worth
investigating this path.
>
> But hey, it works. The mutexless access is not a big deal, because I
> call this function on every input report, so I will get the right value
> eventually.
ouch. That's a no from me for this implementation then. Testing it for
every input report seems overkill. Also, I do not understand correctly
how you will send your own commands if the hidraw node is opened.
Could you send the patch as an RFC so we have a good idea of what we
are talking here. But from what I understand, it would be wiser to
have the infrastructure to get notified when the hidraw node is used
by Steam or any other client and do your magic here.
[sometime later]
Actually, if there is no magic command involved to "disable" hid-steam
and let Steam handle the touchpad itself it should be fine. But if the
touchpad is configured in a non default mode, will Steam reset the
controller when it opens the hidraw node? (talking about the current
Steam client, not possible future changes)
>
> Then if I am the only user, I can disable/enable the lizard mode when
> opening/closing the input device. Moreover if hidraw is opened, then I
> bypass the input events, and this gamepad goes idle, preventing the
> double input problem. It's a bit tricky in the details, but I think I
> got it right.
Wouldn't it be better to actually kill the extra input nodes instead
of keeping them around? This way users will only see one Steam
controller joystick. Also, this would prevent the use case where you
open Steam, minimize it (so the joystick are disabled), and then start
an SDL game to wait until you download the latest patch for your
favorite game.
Of course Steam could close the hidraw node on minimize, but it'll be
better IMO to be explicit in this particular case.
>
> If you don't think this solution is too hacky, I'll post a reroll with
> this code, in a few days. I still have to do a few more tests.
Please send the reroll, even if not entirely tested. It'll allow
everyone involved to have a better idea of the solution and will help
everyone to find the correct solution.
Cheers,
Benjamin
>
> Now, what I would really want is a review by Valve of my set-lizard function:
>
> static void steam_set_lizard_mode(struct steam_device *steam, bool enabled)
> {
> if (enabled) {
> steam_send_report_byte(steam, 0x8e); //enable mouse
> steam_send_report_byte(steam, 0x85); //enable esc, enter and cursor keys
> } else {
> steam_send_report_byte(steam, 0x81); //disable esc, enter and cursor keys
> steam_write_register(steam, 0x08, 0x07); //disable mouse (cmd: 0x87)
> }
> }
>
> While it works, I find its asymmetry quite uncanny. I'm afraid that some
> of these are there for a side effect, this is not their real purpose.
> Could you give me a hint about this?
>
>> Note that there's a general desire on our side to create a reference
>> userspace implementation that would more or less have the current
>> functionality of the Steam client, but would be easily usable from other
>> platforms where the client doesn't currentl run. Unfortunately it's quite a
>> bit of work, so it's unclear what the timeframe would be, if it ever does
>> happen.
>
> Do you mean the piece of Steam Client that does input-mapping, but as a
> portable program without the full client? That would be awesome! And if
> open-sourced, even awesomer!!
>
> Thank you all.
> Rodrigo
>
>> Thanks,
>> - Pierre-Loup
>>
>> >
>> > Best regards.
>> > Rodrigo
>> >
>> > > Cheers,
>> > > Benjamin
>> > >
>> > > > Best Regards.
>> > > > Rodrigo.
>> > > >
>> > > > > Also the button and axis codes do not match the gamepad API doc
>> > > > > (https://www.kernel.org/doc/Documentation/input/gamepad.txt).
>> > > > >
>> > > > > >
>> > > > > > For full reference, I'm adding a full changelog of this patchset.
>> > > > > >
>> > > > > > Changes in v5:
>> > > > > > * Fix license SPDX to GPL-2.0+.
>> > > > > > * Minor stylistic changes (BIT(3) instead 0x08 and so on).
>> > > > > >
>> > > > > > Changes in v4:
>> > > > > > * Add command to check the wireless connection status on probe, without
>> > > > > > waiting for a message (thanks to Clément Vuchener for the tip).
>> > > > > > * Removed the error code on redundant connection/disconnection messages. That
>> > > > > > was harmless but polluted dmesg.
>> > > > > > * Added buttons for touching the left-pad and right-pad.
>> > > > > > * Fixed a misplaced #include from 2/4 to 1/4.
>> > > > > >
>> > > > > > Changes in v3:
>> > > > > > * Use RCU to do the dynamic connec/disconnect of wireless devices.
>> > > > > > * Remove entries in hid-quirks.c as they are no longer needed. This allows
>> > > > > > this module to be blacklisted without side effects.
>> > > > > > * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
>> > > > > > existing use cases (lizard mode). A user-space tool to do that is
>> > > > > > linked.
>> > > > > > * Fully separated axes for joystick and left-pad. As it happens.
>> > > > > > * Add fuzz values for left/right pad axes, they are a little wiggly.
>> > > > > >
>> > > > > > Changes in v2:
>> > > > > > * Remove references to USB. Now the interesting interfaces are selected by
>> > > > > > looking for the ones with feature reports.
>> > > > > > * Feature reports buffers are allocated with hid_alloc_report_buf().
>> > > > > > * Feature report length is checked, to avoid overflows in case of
>> > > > > > corrupt/malicius USB devices.
>> > > > > > * Resolution added to the ABS axes.
>> > > > > > * A lot of minor cleanups.
>> > > > > >
>> > > > > > Rodrigo Rivas Costa (4):
>> > > > > > HID: add driver for Valve Steam Controller
>> > > > > > HID: steam: add serial number information.
>> > > > > > HID: steam: command to check wireless connection
>> > > > > > HID: steam: add battery device.
>> > > > > >
>> > > > > > drivers/hid/Kconfig | 8 +
>> > > > > > drivers/hid/Makefile | 1 +
>> > > > > > drivers/hid/hid-ids.h | 4 +
>> > > > > > drivers/hid/hid-steam.c | 794 ++++++++++++++++++++++++++++++++++++++++++++++++
>> > > > > > 4 files changed, 807 insertions(+)
>> > > > > > create mode 100644 drivers/hid/hid-steam.c
>> > > > > >
>> > > > > > --
>> > > > > > 2.16.2
>> > > > > >
>> >
>>
^ permalink raw reply
* Re: [PATCH v3] Fix modifier keys for Redragon Asura Keyboard
From: Benjamin Tissoires @ 2018-03-21 16:12 UTC (permalink / raw)
To: Robert Munteanu; +Cc: Jiri Kosina, lkml, open list:HID CORE LAYER
In-Reply-To: <20180321104331.22783-1-rombert@apache.org>
Hi Robert,
First, apologies for not answering to the RFC. I missed it and it fell
down in my INBOX.
On Wed, Mar 21, 2018 at 11:43 AM, Robert Munteanu <rombert@apache.org> wrote:
> The microdia family of keyboards uses a non-standard way of sending
> modifier keys.
>
> The down event always sets the first bit to 0x04 and the second keycode
Pretty sure you are talking about bytes here, not bits.
And in the HID world, the first byte is usually a report ID.
> to a custom value For instance, left shift sends the following bits
>
> 04 02 00 00 00 00 00 00
>
> while left control sends
>
> 04 01 00 00 00 00 00 00
This sounds like bit(0) is mapped to LEFT_SHIFT and bit(1) mapped to
LEFT_CONTROL in the second byte.
Fortunately, LeftControl is designed in HID as 0xe0 in the keyboard
HID usage table, and LeftShift is 0xe1. So that would match the
behavior you are seeing.
If you have 04 04 00 00 00 00 00 00 when pressing LeftAlt, then
definitively you are just facing a bitmask, which is fairly common.
>
> As a result all modifier keys are mapped to left shift and the keyboard is
> non-functional in that respect. To solve the problem, we capture the
> raw data in raw_event and manually generate the correct input events.
I'd like to see the hid-recorder events from these keypresses. The
device might be buggy, but I have a gut feeling your solution is not
the simplest one.
Please grab hid-recorder from http://bentiss.github.io/hid-replay-docs/
>
> The keyboard functions mostly as expected now, with only a few minor
> issues:
>
> - two USB devices are detected instead of 1
Well, that should be easy enough to solve
> - some key combinations are not triggered - e.g.
> left shift + left ctrl + p. However, the same combination is recognized
> with the right shift key.
Could you add this combination in the hid-recorder output too?
more comments inlined:
>
> Changelog:
>
> - v2: modifier keys work, some combinations are still troublesome
> - v3: style cleanup, rebase on top of 4.14
> - v4: remove most debugging calls, make init info useful for user,
> rebased on top of 4.15
>
> Signed-off-by: Robert Munteanu <rombert@apache.org>
> ---
> drivers/hid/Kconfig | 9 +++
> drivers/hid/Makefile | 2 +-
> drivers/hid/hid-core.c | 3 +
> drivers/hid/hid-ids.h | 3 +
> drivers/hid/hid-microdia.c | 148 +++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 164 insertions(+), 1 deletion(-)
> create mode 100644 drivers/hid/hid-microdia.c
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 779c5ae47f36..c3350f2ec4ea 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -548,6 +548,15 @@ config HID_MAYFLASH
> Say Y here if you have HJZ Mayflash PS3 game controller adapters
> and want to enable force feedback support.
>
> +config HID_MICRODIA
> + tristate "Microdia based keyboards"
> + depends on HID
> + default !EXPERT
> + ---help---
> + Support for Microdia devices that are not compliant with the HID standard.
> +
> + One known example if the Redragon Asura Keyboard.
> +
> config HID_MICROSOFT
> tristate "Microsoft non-fully HID-compliant devices"
> depends on HID
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 235bd2a7b333..e66a305876c5 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -62,6 +62,7 @@ obj-$(CONFIG_HID_LOGITECH_DJ) += hid-logitech-dj.o
> obj-$(CONFIG_HID_LOGITECH_HIDPP) += hid-logitech-hidpp.o
> obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
> obj-$(CONFIG_HID_MAYFLASH) += hid-mf.o
> +obj-$(CONFIG_HID_MICRODIA) += hid-microdia.o
> obj-$(CONFIG_HID_MICROSOFT) += hid-microsoft.o
> obj-$(CONFIG_HID_MONTEREY) += hid-monterey.o
> obj-$(CONFIG_HID_MULTITOUCH) += hid-multitouch.o
> @@ -121,4 +122,3 @@ obj-$(CONFIG_USB_KBD) += usbhid/
>
> obj-$(CONFIG_I2C_HID) += i2c-hid/
>
> -obj-$(CONFIG_INTEL_ISH_HID) += intel-ish-hid/
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 0c3f608131cf..b36c2df4b755 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -2393,6 +2393,9 @@ static const struct hid_device_id hid_have_special_driver[] = {
> #endif
> #if IS_ENABLED(CONFIG_HID_ZYDACRON)
> { HID_USB_DEVICE(USB_VENDOR_ID_ZYDACRON, USB_DEVICE_ID_ZYDACRON_REMOTE_CONTROL) },
> +#endif
> +#if IS_ENABLED(CONFIG_HID_MICRODIA)
> + { HID_USB_DEVICE(USB_VENDOR_ID_MICRODIA, USB_DEVICE_ID_REDRAGON_ASURA) },
> #endif
You don't need this hunk anymore since v4.15 IIRC
> { }
> };
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 5da3d6256d25..146869e55c5a 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -1171,4 +1171,7 @@
> #define USB_VENDOR_ID_UGTIZER 0x2179
> #define USB_DEVICE_ID_UGTIZER_TABLET_GP0610 0x0053
>
> +#define USB_VENDOR_ID_MICRODIA 0x0c45
> +#define USB_DEVICE_ID_REDRAGON_ASURA 0x760b
> +
> #endif
> diff --git a/drivers/hid/hid-microdia.c b/drivers/hid/hid-microdia.c
> new file mode 100644
> index 000000000000..f9d8de18a989
> --- /dev/null
> +++ b/drivers/hid/hid-microdia.c
> @@ -0,0 +1,148 @@
missing the SPDX identifier
(see Documentation/process/license-rules.rst)
> +/*
> + * HID driver for Microdia-based keyboards
> + *
> + * Copyright (c) 2017 Robert Munteanu
> + */
> +
> +/*
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the Free
> + * Software Foundation; either version 2 of the License, or (at your option)
> + * any later version.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/input.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +#include <linux/log2.h>
> +#include <linux/input-event-codes.h>
> +
> +#include "hid-ids.h"
> +
> +
> +// The microdia family of keyboards uses a non-standard way of sending
> +// modifier keys
Unless it has changed since last time I checked, we do not use C++ comments
(see Documentation/process/coding-style.rst)
> +//
> +// The down event always sets the first bit to 0x04 and the second keycode
> +// to a custom value. For instance, left shift sends the following bits
> +//
> +// 04 02 00 00 00 00 00 00
> +//
> +// while left control sends
> +//
> +// 04 01 00 00 00 00 00 00
> +//
> +// Unfortunately these are all mapped to left shift and the keyboard is
> +// non-functional in that respect. To solve the problem, we capture the
> +// raw data in raw_event and manually generate the correct input events
> +//
> +// TODO
> +//
> +// 1. Some modifiers keys still don't work, e.g. Ctrl-Shift-P
> +// Interestingly enough, this happens when pressing the physical
> +// left shift key, but now when pressing the physical right shift key.
> +// hexdump does not show them either.
> +// 2. A second USB keyboard is registered, but it should be removed
> +// 3. Modifier keys sometimes get stuck, need to re-press to clear
> +
> +static int microdia_input_configured(struct hid_device *hdev,
> + struct hid_input *hi)
> +{
> +
> + hid_info(hdev, "Keyboard configured with limited support (modifier keys may get stuck, some modifiers combinations with left-shift not working) ");
> +
> + hid_set_drvdata(hdev, hi);
> +
> + return 0;
> +}
> +
> +static int microdia_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> + struct hid_field *field, struct hid_usage *usage,
> + unsigned long **bit, int *max)
> +{
> + // do not map left shift since we will manually generate input
> + // events in microdia_raw_event
> + if ((usage->hid & HID_USAGE_PAGE) == HID_UP_KEYBOARD)
> + if ((usage->hid & HID_USAGE) == 0xe1)
> + return -1;
> +
> + return 0;
> +}
> +
> +// array index of a modifier matches the bit index in the data payload
> +// the modifier data is always stored in the second int
> +// e.g. for left alt the 1 bit is set - 04 04 ...
> +// TODO - second entry should be left shift, but that's not possible
> +// since we ignore it in the mapping
> +static int microdia_modifiers[7] = {
> + KEY_LEFTCTRL,
> + KEY_RIGHTSHIFT,
> + KEY_LEFTALT,
> + KEY_LEFTMETA,
> + KEY_RIGHTCTRL,
> + KEY_RIGHTSHIFT,
> + KEY_RIGHTALT
> +};
> +
> +static int microdia_last_handled_modifier;
> +
> +static int microdia_raw_event(struct hid_device *hdev,
> + struct hid_report *report, u8 *data, int size)
> +{
> + int i, changed_key, new_value, mapped_key;
> + struct hid_input *hi;
> +
> + // 1. validate that we get 8 bits of the for 04 xx 00 00 00 00 00 00
> + if (size != 8)
> + return 0;
> +
> + if (data[0] != 4)
> + return 0;
> +
> + // TODO - can I somehow use a bit mask? data & 0x00ffffff != 0
> + for (i = 2; i < size; i++)
> + if (data[i] != 0)
> + return 0;
> +
> + // TODO - don't process the keyup event 04 00 00 00 00 00 00 00
That's a lot of TODO in such a simple driver :/
> + // if we don't expect a modifier key 'up' event
> +
> + // 2. detect based on previous data what key was pressed or depressed
> + // a key was pressed or depressed if its bit has a different value
> + // between this and the previous invocation. If the bit is set
> + // it was pressed
> +
> + changed_key = data[1] ^ microdia_last_handled_modifier;
> + new_value = (data[1] & changed_key) != 0;
> + // TODO - is ilog2 really needed?
> + mapped_key = microdia_modifiers[ilog2(changed_key)];
What if you get 2 changed keys at a time?
Anyway, before giving you a ACK or NACK on the driver, the behavior
from the keyboard firmware looks sane and pretty common. So my guess
is that there is something wrong in the report descriptors, and I need
the hid-recorder output to have a better idea of what is wrong with
our current handling of this keyboard.
Cheers,
Benjamin
> +
> + // 3. report the key event and track what was sent
> + hi = hid_get_drvdata(hdev);
> +
> + input_report_key(hi->input, mapped_key, new_value);
> +
> + microdia_last_handled_modifier = data[1];
> +
> + return 1;
> +}
> +
> +static const struct hid_device_id microdia_devices[] = {
> + {HID_USB_DEVICE(USB_VENDOR_ID_MICRODIA, USB_DEVICE_ID_REDRAGON_ASURA)},
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(hid, microdia_devices);
> +
> +static struct hid_driver microdia_driver = {
> + .name = "microdia",
> + .input_mapping = microdia_input_mapping,
> + .id_table = microdia_devices,
> + .raw_event = microdia_raw_event,
> + .input_configured = microdia_input_configured,
> +};
> +
> +module_hid_driver(microdia_driver);
> +
> +MODULE_LICENSE("GPL");
> --
> 2.16.2
>
^ permalink raw reply
* Re: [PATCH v3] Fix modifier keys for Redragon Asura Keyboard
From: Robert Munteanu @ 2018-03-21 16:33 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, lkml, open list:HID CORE LAYER
In-Reply-To: <CAO-hwJJRZkpqiFr1j5U_x0pgFP3RPN0w_cfKJvKMz_QznH87cA@mail.gmail.com>
Hi Benjamin,
On Wed, 2018-03-21 at 17:12 +0100, Benjamin Tissoires wrote:
> Hi Robert,
>
> First, apologies for not answering to the RFC. I missed it and it
> fell
> down in my INBOX.
>
> On Wed, Mar 21, 2018 at 11:43 AM, Robert Munteanu <rombert@apache.org
> > wrote:
> > The microdia family of keyboards uses a non-standard way of sending
> > modifier keys.
> >
> > The down event always sets the first bit to 0x04 and the second
> > keycode
>
> Pretty sure you are talking about bytes here, not bits.
> And in the HID world, the first byte is usually a report ID.
Right, thanks for spotting that.
>
> > to a custom value For instance, left shift sends the following bits
> >
> > 04 02 00 00 00 00 00 00
> >
> > while left control sends
> >
> > 04 01 00 00 00 00 00 00
>
> This sounds like bit(0) is mapped to LEFT_SHIFT and bit(1) mapped to
> LEFT_CONTROL in the second byte.
> Fortunately, LeftControl is designed in HID as 0xe0 in the keyboard
> HID usage table, and LeftShift is 0xe1. So that would match the
> behavior you are seeing.
> If you have 04 04 00 00 00 00 00 00 when pressing LeftAlt, then
> definitively you are just facing a bitmask, which is fairly common.
>
> >
> > As a result all modifier keys are mapped to left shift and the
> > keyboard is
> > non-functional in that respect. To solve the problem, we capture
> > the
> > raw data in raw_event and manually generate the correct input
> > events.
>
> I'd like to see the hid-recorder events from these keypresses. The
> device might be buggy, but I have a gut feeling your solution is not
> the simplest one.
> Please grab hid-recorder from http://bentiss.github.io/hid-replay-
> docs/
See below, I recorded the output for pressing left ctrl, left alt, left
shift, right ctrl, right alt, right shift, meta.
D: 0
R: 169 05 0c 09 01 a1 01 85 01 19 00 2a 80 03 15 00 26 80 03 95 01 75
10 81 00 c0 05 01 09 80 a1 01 85 02 19 81 29 83 15 00 25 01 75 01 95 03
81 02 95 05 81 01 c0 06 00 ff 09 01 a1 01 85 03 1a f1 00 2a f8 00 15 00
25 01 75 01 95 08 81 02 c0 05 01 09 06 a1 01 85 04 05 07 19 e0 29 e7 15
00 25 01 75 01 95 08 81 00 95 30 75 01 15 00 25 01 05 07 19 00 29 2f 81
02 c0 05 01 09 06 a1 01 85 05 95 38 75 01 15 00 25 01 05 07 19 30 29 67
81 02 c0 05 01 09 06 a1 01 85 06 95 38 75 01 15 00 25 01 05 07 19 68 29
9f 81 02 c0
N: USB Keyboard
P: usb-0000:00:14.0-6/input1
I: 3 0c45 760b
D: 0
E: 0.000000 8 04 01 00 00 00 00 00 00
E: 0.039920 8 04 00 00 00 00 00 00 00
E: 0.751952 8 04 04 00 00 00 00 00 00
E: 0.823977 8 04 00 00 00 00 00 00 00
E: 2.639887 8 04 02 00 00 00 00 00 00
E: 2.711896 8 04 00 00 00 00 00 00 00
E: 3.583932 8 04 10 00 00 00 00 00 00
E: 3.663919 8 04 00 00 00 00 00 00 00
E: 4.871886 8 04 40 00 00 00 00 00 00
E: 4.935906 8 04 00 00 00 00 00 00 00
E: 6.503872 8 04 20 00 00 00 00 00 00
E: 6.575850 8 04 00 00 00 00 00 00 00
E: 7.383844 8 04 08 00 00 00 00 00 00
E: 7.455861 8 04 00 00 00 00 00 00 00
>
> >
> > The keyboard functions mostly as expected now, with only a few
> > minor
> > issues:
> >
> > - two USB devices are detected instead of 1
>
> Well, that should be easy enough to solve
I think so, but I could not find a way how to do that.
>
> > - some key combinations are not triggered - e.g.
> > left shift + left ctrl + p. However, the same combination is
> > recognized
> > with the right shift key.
>
> Could you add this combination in the hid-recorder output too?
This is what Left Ctrl + Shift + U produces:
E: 24.575686 8 04 01 00 00 00 00 00 00
E: 24.951689 8 04 03 00 00 00 00 00 00
E: 26.719615 8 04 01 00 00 00 00 00 00
E: 26.727611 8 04 00 00 00 00 00 00 00
On the other hand, Right Ctrl + Shift + U produces:
E: 55.911268 8 04 10 00 00 00 00 00 00
E: 56.119280 8 04 30 00 00 00 00 00 00
E: 56.599319 8 04 30 00 00 00 01 00 00
E: 56.703282 8 04 30 00 00 00 00 00 00
E: 56.815278 8 04 20 00 00 00 00 00 00
E: 56.823255 8 04 00 00 00 00 00 00 00
It looks suspicious that the keypress is not registered. Do I need to
boot a kernel without my patch applied?
> more comments inlined:
>
> >
> > Changelog:
> >
> > - v2: modifier keys work, some combinations are still troublesome
> > - v3: style cleanup, rebase on top of 4.14
> > - v4: remove most debugging calls, make init info useful for user,
> > rebased on top of 4.15
> >
> > Signed-off-by: Robert Munteanu <rombert@apache.org>
> > ---
> > drivers/hid/Kconfig | 9 +++
> > drivers/hid/Makefile | 2 +-
> > drivers/hid/hid-core.c | 3 +
> > drivers/hid/hid-ids.h | 3 +
> > drivers/hid/hid-microdia.c | 148
> > +++++++++++++++++++++++++++++++++++++++++++++
> > 5 files changed, 164 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/hid/hid-microdia.c
> >
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index 779c5ae47f36..c3350f2ec4ea 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -548,6 +548,15 @@ config HID_MAYFLASH
> > Say Y here if you have HJZ Mayflash PS3 game controller
> > adapters
> > and want to enable force feedback support.
> >
> > +config HID_MICRODIA
> > + tristate "Microdia based keyboards"
> > + depends on HID
> > + default !EXPERT
> > + ---help---
> > + Support for Microdia devices that are not compliant with the
> > HID standard.
> > +
> > + One known example if the Redragon Asura Keyboard.
> > +
> > config HID_MICROSOFT
> > tristate "Microsoft non-fully HID-compliant devices"
> > depends on HID
> > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > index 235bd2a7b333..e66a305876c5 100644
> > --- a/drivers/hid/Makefile
> > +++ b/drivers/hid/Makefile
> > @@ -62,6 +62,7 @@ obj-$(CONFIG_HID_LOGITECH_DJ) += hid-logitech-
> > dj.o
> > obj-$(CONFIG_HID_LOGITECH_HIDPP) += hid-logitech-hidpp.o
> > obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
> > obj-$(CONFIG_HID_MAYFLASH) += hid-mf.o
> > +obj-$(CONFIG_HID_MICRODIA) += hid-microdia.o
> > obj-$(CONFIG_HID_MICROSOFT) += hid-microsoft.o
> > obj-$(CONFIG_HID_MONTEREY) += hid-monterey.o
> > obj-$(CONFIG_HID_MULTITOUCH) += hid-multitouch.o
> > @@ -121,4 +122,3 @@ obj-$(CONFIG_USB_KBD) += usbhid/
> >
> > obj-$(CONFIG_I2C_HID) += i2c-hid/
> >
> > -obj-$(CONFIG_INTEL_ISH_HID) += intel-ish-hid/
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 0c3f608131cf..b36c2df4b755 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -2393,6 +2393,9 @@ static const struct hid_device_id
> > hid_have_special_driver[] = {
> > #endif
> > #if IS_ENABLED(CONFIG_HID_ZYDACRON)
> > { HID_USB_DEVICE(USB_VENDOR_ID_ZYDACRON,
> > USB_DEVICE_ID_ZYDACRON_REMOTE_CONTROL) },
> > +#endif
> > +#if IS_ENABLED(CONFIG_HID_MICRODIA)
> > + {
> > HID_USB_DEVICE(USB_VENDOR_ID_MICRODIA, USB_DEVICE_ID_REDRAGON_ASUR
> > A) },
> > #endif
>
> You don't need this hunk anymore since v4.15 IIRC
Ack, will update and retest.
>
> > { }
> > };
> > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > index 5da3d6256d25..146869e55c5a 100644
> > --- a/drivers/hid/hid-ids.h
> > +++ b/drivers/hid/hid-ids.h
> > @@ -1171,4 +1171,7 @@
> > #define USB_VENDOR_ID_UGTIZER 0x2179
> > #define USB_DEVICE_ID_UGTIZER_TABLET_GP0610 0x0053
> >
> > +#define USB_VENDOR_ID_MICRODIA 0x0c45
> > +#define USB_DEVICE_ID_REDRAGON_ASURA 0x760b
> > +
> > #endif
> > diff --git a/drivers/hid/hid-microdia.c b/drivers/hid/hid-
> > microdia.c
> > new file mode 100644
> > index 000000000000..f9d8de18a989
> > --- /dev/null
> > +++ b/drivers/hid/hid-microdia.c
> > @@ -0,0 +1,148 @@
>
> missing the SPDX identifier
> (see Documentation/process/license-rules.rst)
Ack, will update.
>
> > +/*
> > + * HID driver for Microdia-based keyboards
> > + *
> > + * Copyright (c) 2017 Robert Munteanu
> > + */
> > +
> > +/*
> > + * This program is free software; you can redistribute it and/or
> > modify it
> > + * under the terms of the GNU General Public License as published
> > by the Free
> > + * Software Foundation; either version 2 of the License, or (at
> > your option)
> > + * any later version.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/input.h>
> > +#include <linux/hid.h>
> > +#include <linux/module.h>
> > +#include <linux/log2.h>
> > +#include <linux/input-event-codes.h>
> > +
> > +#include "hid-ids.h"
> > +
> > +
> > +// The microdia family of keyboards uses a non-standard way of
> > sending
> > +// modifier keys
>
> Unless it has changed since last time I checked, we do not use C++
> comments
> (see Documentation/process/coding-style.rst)
Ack.
>
> > +//
> > +// The down event always sets the first bit to 0x04 and the second
> > keycode
> > +// to a custom value. For instance, left shift sends the following
> > bits
> > +//
> > +// 04 02 00 00 00 00 00 00
> > +//
> > +// while left control sends
> > +//
> > +// 04 01 00 00 00 00 00 00
> > +//
> > +// Unfortunately these are all mapped to left shift and the
> > keyboard is
> > +// non-functional in that respect. To solve the problem, we
> > capture the
> > +// raw data in raw_event and manually generate the correct input
> > events
> > +//
> > +// TODO
> > +//
> > +// 1. Some modifiers keys still don't work, e.g. Ctrl-Shift-P
> > +// Interestingly enough, this happens when pressing the
> > physical
> > +// left shift key, but now when pressing the physical right
> > shift key.
> > +// hexdump does not show them either.
> > +// 2. A second USB keyboard is registered, but it should be
> > removed
> > +// 3. Modifier keys sometimes get stuck, need to re-press to clear
> > +
> > +static int microdia_input_configured(struct hid_device *hdev,
> > + struct hid_input *hi)
> > +{
> > +
> > + hid_info(hdev, "Keyboard configured with limited support
> > (modifier keys may get stuck, some modifiers combinations with
> > left-shift not working) ");
> > +
> > + hid_set_drvdata(hdev, hi);
> > +
> > + return 0;
> > +}
> > +
> > +static int microdia_input_mapping(struct hid_device *hdev, struct
> > hid_input *hi,
> > + struct hid_field *field, struct hid_usage *usage,
> > + unsigned long **bit, int *max)
> > +{
> > + // do not map left shift since we will manually generate
> > input
> > + // events in microdia_raw_event
> > + if ((usage->hid & HID_USAGE_PAGE) == HID_UP_KEYBOARD)
> > + if ((usage->hid & HID_USAGE) == 0xe1)
> > + return -1;
> > +
> > + return 0;
> > +}
> > +
> > +// array index of a modifier matches the bit index in the data
> > payload
> > +// the modifier data is always stored in the second int
> > +// e.g. for left alt the 1 bit is set - 04 04 ...
> > +// TODO - second entry should be left shift, but that's not
> > possible
> > +// since we ignore it in the mapping
> > +static int microdia_modifiers[7] = {
> > + KEY_LEFTCTRL,
> > + KEY_RIGHTSHIFT,
> > + KEY_LEFTALT,
> > + KEY_LEFTMETA,
> > + KEY_RIGHTCTRL,
> > + KEY_RIGHTSHIFT,
> > + KEY_RIGHTALT
> > +};
> > +
> > +static int microdia_last_handled_modifier;
> > +
> > +static int microdia_raw_event(struct hid_device *hdev,
> > + struct hid_report *report, u8 *data, int size)
> > +{
> > + int i, changed_key, new_value, mapped_key;
> > + struct hid_input *hi;
> > +
> > + // 1. validate that we get 8 bits of the for 04 xx 00 00 00
> > 00 00 00
> > + if (size != 8)
> > + return 0;
> > +
> > + if (data[0] != 4)
> > + return 0;
> > +
> > + // TODO - can I somehow use a bit mask? data & 0x00ffffff
> > != 0
> > + for (i = 2; i < size; i++)
> > + if (data[i] != 0)
> > + return 0;
> > +
> > + // TODO - don't process the keyup event 04 00 00 00 00 00
> > 00 00
>
> That's a lot of TODO in such a simple driver :/
Yes, well that's what I get for learning to write hid drivers by
example ... and C isn't my primary programming language either.
>
> > + // if we don't expect a modifier key 'up' event
> > +
> > + // 2. detect based on previous data what key was pressed or
> > depressed
> > + // a key was pressed or depressed if its bit has a
> > different value
> > + // between this and the previous invocation. If the bit is
> > set
> > + // it was pressed
> > +
> > + changed_key = data[1] ^ microdia_last_handled_modifier;
> > + new_value = (data[1] & changed_key) != 0;
> > + // TODO - is ilog2 really needed?
> > + mapped_key = microdia_modifiers[ilog2(changed_key)];
>
> What if you get 2 changed keys at a time?
This could explain the problems that I've been getting with modifier
keys being 'stuck'.
>
> Anyway, before giving you a ACK or NACK on the driver, the behavior
> from the keyboard firmware looks sane and pretty common. So my guess
> is that there is something wrong in the report descriptors, and I
> need
> the hid-recorder output to have a better idea of what is wrong with
> our current handling of this keyboard.
The output is inline. That being said, I'm aware that re-firing the
keypress events myself does not belong in a HID driver, but I'm eager
to work on this being massaged into something proper for inclusion.
Thanks for taking the time to review.
Robert
>
> Cheers,
> Benjamin
>
> > +
> > + // 3. report the key event and track what was sent
> > + hi = hid_get_drvdata(hdev);
> > +
> > + input_report_key(hi->input, mapped_key, new_value);
> > +
> > + microdia_last_handled_modifier = data[1];
> > +
> > + return 1;
> > +}
> > +
> > +static const struct hid_device_id microdia_devices[] = {
> > + {HID_USB_DEVICE(USB_VENDOR_ID_MICRODIA,
> > USB_DEVICE_ID_REDRAGON_ASURA)},
> > + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(hid, microdia_devices);
> > +
> > +static struct hid_driver microdia_driver = {
> > + .name = "microdia",
> > + .input_mapping = microdia_input_mapping,
> > + .id_table = microdia_devices,
> > + .raw_event = microdia_raw_event,
> > + .input_configured = microdia_input_configured,
> > +};
> > +
> > +module_hid_driver(microdia_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > --
> > 2.16.2
> >
^ permalink raw reply
* Re: [PATCH] Input: alps - Demystify trackstick initialization for v3 and v6 protocols
From: Pali Rohár @ 2018-03-21 16:41 UTC (permalink / raw)
To: Masaki Ota, Dmitry Torokhov
Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <TYXPR01MB0719CA528857480EC046DEF6C7D40@TYXPR01MB0719.jpnprd01.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 4135 bytes --]
That is pity, but OK.
Anyway, as wrote patch which I sent in the first email matches this
documentation.
Dmitry, can you review/comment/accept/reject this patch?
On Monday 19 March 2018 08:41:19 Masaki Ota wrote:
> Hi, Pali,
>
> v3/v6 devices are T3 type, and it can use only Method 2.
> P36 (At this time GLIDEPOINT_T3 uses the Method 2 ....)
> T3 has the potential that uses Method2, but I think it needs to change Firmware.
>
> Best Regards,
> Masaki Ota
> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Friday, March 16, 2018 7:58 PM
> To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] Input: alps - Demystify trackstick initialization for v3 and v6 protocols
>
> Great, thank you. Enabling that RAW/extended/SP4 mode is done by sequence E6, E6, E6, F3, C8, F3, 14 (written in page 35) and it matches what function alps_trackstick_enter_extended_mode_v3_v6() in my patch is doing. So is correct.
>
> On page 36 I see that there is described Method 1 for reporting stick data which prevents cursor jumps. Seems that kernel uses Method 2.
> Method 1 depends on some prioritization.
>
> Do you have some information how to activate Method 1? Sometimes I observe that problem with "cursor jumps" and from Method 1 could prevent it. So I would like to try experimenting...
>
> On Wednesday 14 March 2018 23:56:46 Masaki Ota wrote:
> > Hi, Pali,
> >
> > I have added Appendix.
> > According to this spec documents, SP raw mode is SP 4 byte mode.
> > I think Extended mode meaning is almost all the same as Raw mode.
> > The description of how to set is written in Page 35.
> >
> > Best Regards,
> > Masaki Ota
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > Sent: Thursday, March 15, 2018 7:58 AM
> > To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>;
> > linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] Input: alps - Demystify trackstick initialization
> > for v3 and v6 protocols
> >
> > Hi! Thank you for information.
> >
> > Your PS/2 Aux Port Control description seems to really matches. Just there is reverse order of bits. Bit 0 in description is highest, therefore matches BIT(7) macro.
> >
> > Bit 6 in description (BIT(1) in code) describes SP Extended Mode which alps.c enabled. And in that description is written:
> >
> > "If 1 SP is extended packet format (driver must set SP raw mode and GP absolute mode)."
> >
> > Do you have any idea what "SP raw mode" is? How to set it? For me it looks like it could be that extended mode of trackstick itself.
> >
> > "GP absolute mode" I guess is GlidePoint absolute mode, therefore
> > enable
> > 6 byte absolute mode for touchpad.
> >
> > And for Bit 7 (BIT(0)) is written:
> >
> > This bit is used with the PS/2 Aux port to use the Pass-Thru mode ( see appendix A ). Do you have some information about this appendix A?
> >
> > On Wednesday 14 March 2018 10:21:43 Masaki Ota wrote:
> > > Hi, Pali,
> > >
> > > I just picked up the spec which relates with trackstic.
> > >
> > > Best Regards,
> > > Masaki Ota
> > > -----Original Message-----
> > > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > > Sent: Tuesday, March 13, 2018 8:14 AM
> > > To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>; Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com>
> > > Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH] Input: alps - Demystify trackstick
> > > initialization for v3 and v6 protocols
> > >
> > > Masaki, if you have access to the internal ALPS v3 / Rushmore
> > > documentation, I would like to have a review of this patch or
> > > confirmation of those information :-)
> >
> > --
> > Pali Rohár
> > pali.rohar@gmail.com
>
> --
> Pali Rohár
> pali.rohar@gmail.com
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCH v3] Fix modifier keys for Redragon Asura Keyboard
From: Ondrej Zary @ 2018-03-21 16:51 UTC (permalink / raw)
To: Robert Munteanu
Cc: Jiri Kosina, Benjamin Tissoires, linux-kernel, linux-input
In-Reply-To: <20180321104331.22783-1-rombert@apache.org>
On Wednesday 21 March 2018 11:43:31 Robert Munteanu wrote:
> +#define USB_VENDOR_ID_MICRODIA 0x0c45
> +#define USB_DEVICE_ID_REDRAGON_ASURA 0x760b
Microdia is most probably an incorrect name. The 0c45 id probably belongs
to "Sonix Technology Co., Ltd."
--
Ondrej Zary
^ permalink raw reply
* [PATCH] Input: add bu21029 touch driver
From: Mark Jonas @ 2018-03-21 17:04 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Mark Rutland
Cc: linux-input, devicetree, linux-kernel, hs, Zhu Yi, Mark Jonas
From: Zhu Yi <yi.zhu5@cn.bosch.com>
Add the ROHM BU21029 resistive touch panel controller
support with i2c interface.
Signed-off-by: Zhu Yi <yi.zhu5@cn.bosch.com>
Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
Reviewed-by: Heiko Schocher <hs@denx.de>
---
.../bindings/input/touchscreen/bu21029.txt | 30 ++
drivers/input/touchscreen/Kconfig | 12 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/bu21029_ts.c | 456 +++++++++++++++++++++
4 files changed, 499 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/bu21029.txt
create mode 100644 drivers/input/touchscreen/bu21029_ts.c
diff --git a/Documentation/devicetree/bindings/input/touchscreen/bu21029.txt b/Documentation/devicetree/bindings/input/touchscreen/bu21029.txt
new file mode 100644
index 0000000..7b61602
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/bu21029.txt
@@ -0,0 +1,30 @@
+* Rohm BU21029 Touch Screen Controller
+
+Required properties:
+ - compatible : must be "rohm,bu21029"
+ - reg : i2c device address of the chip
+ - interrupt-parent : the phandle for the gpio controller
+ - interrupts : (gpio) interrupt to which the chip is connected
+ - reset-gpios : gpio pin to reset the chip
+ - rohm,x-plate-ohms : x-plate resistance in ohms
+
+Optional properties:
+ - touchscreen-max-pressure: maximum pressure value
+
+Example:
+
+ &i2c1 {
+ /* ... */
+
+ bu21029: bu21029@40 {
+ compatible = "rohm,bu21029";
+ reg = <0x40>;
+ interrupt-parent = <&gpio1>;
+ interrupts = <4 IRQ_TYPE_EDGE_FALLING>;
+ reset-gpios = <&gpio6 16 GPIO_ACTIVE_LOW>;
+ rohm,x-plate-ohms = <600>;
+ touchscreen-max-pressure = <4095>;
+ };
+
+ /* ... */
+ };
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 4f15496..e09fe8f 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -151,6 +151,18 @@ config TOUCHSCREEN_BU21013
To compile this driver as a module, choose M here: the
module will be called bu21013_ts.
+config TOUCHSCREEN_BU21029
+ tristate "Rohm BU21029 based touch panel controllers"
+ depends on I2C
+ help
+ Say Y here if you have a Rohm BU21029 touchscreen controller
+ connected to your system.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called bu21029_ts.
+
config TOUCHSCREEN_CHIPONE_ICN8318
tristate "chipone icn8318 touchscreen controller"
depends on GPIOLIB || COMPILE_TEST
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index dddae79..f50624c 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_TOUCHSCREEN_AR1021_I2C) += ar1021_i2c.o
obj-$(CONFIG_TOUCHSCREEN_ATMEL_MXT) += atmel_mxt_ts.o
obj-$(CONFIG_TOUCHSCREEN_AUO_PIXCIR) += auo-pixcir-ts.o
obj-$(CONFIG_TOUCHSCREEN_BU21013) += bu21013_ts.o
+obj-$(CONFIG_TOUCHSCREEN_BU21029) += bu21029_ts.o
obj-$(CONFIG_TOUCHSCREEN_CHIPONE_ICN8318) += chipone_icn8318.o
obj-$(CONFIG_TOUCHSCREEN_CY8CTMG110) += cy8ctmg110_ts.o
obj-$(CONFIG_TOUCHSCREEN_CYTTSP_CORE) += cyttsp_core.o
diff --git a/drivers/input/touchscreen/bu21029_ts.c b/drivers/input/touchscreen/bu21029_ts.c
new file mode 100644
index 0000000..d5cbf11
--- /dev/null
+++ b/drivers/input/touchscreen/bu21029_ts.c
@@ -0,0 +1,456 @@
+/*
+ * Rohm BU21029 touchscreen controller driver
+ *
+ * Copyright (C) 2015 Bosch Sicherheitssysteme GmbH
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/timer.h>
+
+/* HW_ID1 Register (PAGE=0, ADDR=0x0E, Reset value=0x02, Read only)
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * | D7 | D6 | D5 | D4 | D3 | D2 | D1 | D0 |
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * | HW_IDH |
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * HW_ID2 Register (PAGE=0, ADDR=0x0F, Reset value=0x29, Read only)
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * | D7 | D6 | D5 | D4 | D3 | D2 | D1 | D0 |
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * | HW_IDL |
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * HW_IDH: high 8bits of IC's ID
+ * HW_IDL: low 8bits of IC's ID
+ */
+#define BU21029_HWID_REG (0x0E << 3)
+#define SUPPORTED_HWID 0x0229
+
+/* CFR0 Register (PAGE=0, ADDR=0x00, Reset value=0x20)
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * | D7 | D6 | D5 | D4 | D3 | D2 | D1 | D0 |
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * | 0 | 0 | CALIB | INTRM | 0 | 0 | 0 | 0 |
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * CALIB: 0 = not to use calibration result (*)
+ * 1 = use calibration result
+ * INTRM: 0 = INT output depend on "pen down" (*)
+ * 1 = INT output always "0"
+ */
+#define BU21029_CFR0_REG (0x00 << 3)
+#define CFR0_VALUE 0x00
+
+/* CFR1 Register (PAGE=0, ADDR=0x01, Reset value=0xA6)
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * | D7 | D6 | D5 | D4 | D3 | D2 | D1 | D0 |
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * | MAV | AVE[2:0] | 0 | SMPL[2:0] |
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * MAV: 0 = median average filter off
+ * 1 = median average filter on (*)
+ * AVE: AVE+1 = number of average samples for MAV,
+ * if AVE>SMPL, then AVE=SMPL (=3)
+ * SMPL: SMPL+1 = number of conversion samples for MAV (=7)
+ */
+#define BU21029_CFR1_REG (0x01 << 3)
+#define CFR1_VALUE 0xA6
+
+/* CFR2 Register (PAGE=0, ADDR=0x02, Reset value=0x04)
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * | D7 | D6 | D5 | D4 | D3 | D2 | D1 | D0 |
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * | INTVL_TIME[3:0] | TIME_ST_ADC[3:0] |
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * INTVL_TIME: waiting time between completion of conversion
+ * and start of next conversion, only usable in
+ * autoscan mode (=20.480ms)
+ * TIME_ST_ADC: waiting time between application of voltage
+ * to panel and start of A/D conversion (=100us)
+ */
+#define BU21029_CFR2_REG (0x02 << 3)
+#define CFR2_VALUE 0xC9
+
+/* CFR3 Register (PAGE=0, ADDR=0x0B, Reset value=0x72)
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * | D7 | D6 | D5 | D4 | D3 | D2 | D1 | D0 |
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * | RM8 | STRETCH| PU90K | DUAL | PIDAC_OFS[3:0] |
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * RM8: 0 = coordinate resolution is 12bit (*)
+ * 1 = coordinate resolution is 8bit
+ * STRETCH: 0 = SCL_STRETCH function off
+ * 1 = SCL_STRETCH function on (*)
+ * PU90K: 0 = internal pull-up resistance for touch detection is ~50kohms (*)
+ * 1 = internal pull-up resistance for touch detection is ~90kohms
+ * DUAL: 0 = dual touch detection off (*)
+ * 1 = dual touch detection on
+ * PIDAC_OFS: dual touch detection circuit adjustment, it is not necessary
+ * to change this from initial value
+ */
+#define BU21029_CFR3_REG (0x0B << 3)
+#define CFR3_VALUE 0x42
+
+/* LDO Register (PAGE=0, ADDR=0x0C, Reset value=0x00)
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * | D7 | D6 | D5 | D4 | D3 | D2 | D1 | D0 |
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * | 0 | PVDD[2:0] | 0 | AVDD[2:0] |
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * PVDD: output voltage of panel output regulator (=2.000V)
+ * AVDD: output voltage of analog circuit regulator (=2.000V)
+ */
+#define BU21029_LDO_REG (0x0C << 3)
+#define LDO_VALUE 0x77
+
+/* Serial Interface Command Byte 1 (CID=1)
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * | D7 | D6 | D5 | D4 | D3 | D2 | D1 | D0 |
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * | 1 | CF | CMSK | PDM | STP |
+ * +--------+--------+--------+--------+--------+--------+--------+--------+
+ * CF: conversion function, see table 3 in datasheet p6 (=0000, automatic scan)
+ * CMSK: 0 = executes convert function (*)
+ * 1 = reads the convert result
+ * PDM: 0 = power down after convert function stops (*)
+ * 1 = keep power on after convert function stops
+ * STP: 1 = abort current conversion and power down, set to "0" automatically
+ */
+#define BU21029_AUTOSCAN 0x80
+
+/* The timeout value needs to be larger than INTVL_TIME + tConv4 (sample and
+ * conversion time), where tConv4 is calculated by formula:
+ * tPON + tDLY1 + (tTIME_ST_ADC + (tADC * tSMPL) * 2 + tDLY2) * 3
+ * see figure 8 in datasheet p15 for details of each field.
+ */
+#define PEN_UP_TIMEOUT msecs_to_jiffies(50)
+
+#define STOP_DELAY_US 50L
+#define START_DELAY_MS 2L
+#define BUF_LEN 8L
+#define SCALE_12BIT (1 << 12)
+#define MAX_12BIT ((1 << 12) - 1)
+#define DRIVER_NAME "bu21029"
+
+struct bu21029_ts_data {
+ struct i2c_client *client;
+ struct input_dev *in_dev;
+ struct timer_list timer;
+ u32 reset_gpios;
+ u32 reset_gpios_assert;
+ u32 x_plate_ohms;
+ u32 max_pressure;
+};
+
+static int bu21029_touch_report(struct bu21029_ts_data *bu21029)
+{
+ struct i2c_client *i2c = bu21029->client;
+ u8 buf[BUF_LEN];
+ u16 x, y, z1, z2;
+ u32 rz;
+
+ /* read touch data and deassert INT (by restarting the autoscan mode) */
+ int error = i2c_smbus_read_i2c_block_data(i2c,
+ BU21029_AUTOSCAN,
+ BUF_LEN,
+ buf);
+ if (error < 0)
+ return error;
+
+ /* compose upper 8 and lower 4 bits into a 12bit value:
+ * +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
+ * | ByteH | ByteL |
+ * +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
+ * |b07|b06|b05|b04|b03|b02|b01|b00|b07|b06|b05|b04|b03|b02|b01|b00|
+ * +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
+ * |v11|v10|v09|v08|v07|v06|v05|v04|v03|v02|v01|v00| 0 | 0 | 0 | 0 |
+ * +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+
+ */
+ x = (buf[0] << 4) | (buf[1] >> 4);
+ y = (buf[2] << 4) | (buf[3] >> 4);
+ z1 = (buf[4] << 4) | (buf[5] >> 4);
+ z2 = (buf[6] << 4) | (buf[7] >> 4);
+
+ if (z1 == 0 || z2 == 0)
+ return 0;
+
+ /* calculate Rz (pressure resistance value) by equation:
+ * Rz = Rx * (x/Q) * ((z2/z1) - 1), where
+ * Rx is x-plate resistance,
+ * Q is the touch screen resolution (8bit = 256, 12bit = 4096)
+ * x, z1, z2 are the measured positions.
+ */
+ rz = z2 - z1;
+ rz *= x;
+ rz *= bu21029->x_plate_ohms;
+ rz /= z1;
+ rz = DIV_ROUND_CLOSEST(rz, SCALE_12BIT);
+ if (rz <= bu21029->max_pressure) {
+ input_report_abs(bu21029->in_dev, ABS_X, x);
+ input_report_abs(bu21029->in_dev, ABS_Y, y);
+ input_report_abs(bu21029->in_dev, ABS_PRESSURE, rz);
+ input_report_key(bu21029->in_dev, BTN_TOUCH, 1);
+ input_sync(bu21029->in_dev);
+ }
+
+ return 0;
+}
+
+static void bu21029_touch_release(struct timer_list *t)
+{
+ struct bu21029_ts_data *bu21029 = from_timer(bu21029, t, timer);
+
+ input_report_abs(bu21029->in_dev, ABS_PRESSURE, 0);
+ input_report_key(bu21029->in_dev, BTN_TOUCH, 0);
+ input_sync(bu21029->in_dev);
+}
+
+static irqreturn_t bu21029_touch_soft_irq(int irq, void *data)
+{
+ struct bu21029_ts_data *bu21029 = data;
+ struct i2c_client *i2c = bu21029->client;
+
+ /* report touch and deassert interrupt (will assert again after
+ * INTVL_TIME + tConv4 for continuous touch)
+ */
+ int error = bu21029_touch_report(bu21029);
+
+ if (error) {
+ dev_err(&i2c->dev, "failed to report (error: %d)\n", error);
+ return IRQ_NONE;
+ }
+
+ /* reset timer for pen up detection */
+ mod_timer(&bu21029->timer, jiffies + PEN_UP_TIMEOUT);
+
+ return IRQ_HANDLED;
+}
+
+static void bu21029_reset_chip(struct bu21029_ts_data *bu21029)
+{
+ gpio_set_value(bu21029->reset_gpios,
+ bu21029->reset_gpios_assert);
+ udelay(STOP_DELAY_US);
+ gpio_set_value(bu21029->reset_gpios,
+ !bu21029->reset_gpios_assert);
+ mdelay(START_DELAY_MS);
+}
+
+static int bu21029_init_chip(struct bu21029_ts_data *bu21029)
+{
+ struct i2c_client *i2c = bu21029->client;
+ struct {
+ u8 reg;
+ u8 value;
+ } init_table[] = {
+ {BU21029_CFR0_REG, CFR0_VALUE},
+ {BU21029_CFR1_REG, CFR1_VALUE},
+ {BU21029_CFR2_REG, CFR2_VALUE},
+ {BU21029_CFR3_REG, CFR3_VALUE},
+ {BU21029_LDO_REG, LDO_VALUE}
+ };
+ int error, i;
+ u16 hwid;
+
+ bu21029_reset_chip(bu21029);
+
+ error = i2c_smbus_read_i2c_block_data(i2c,
+ BU21029_HWID_REG,
+ 2,
+ (u8 *)&hwid);
+ if (error < 0) {
+ dev_err(&i2c->dev, "failed to read HW ID\n");
+ return error;
+ }
+
+ if (cpu_to_be16(hwid) != SUPPORTED_HWID) {
+ dev_err(&i2c->dev, "unsupported HW ID 0x%x\n", hwid);
+ return -ENODEV;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(init_table); ++i) {
+ error = i2c_smbus_write_byte_data(i2c,
+ init_table[i].reg,
+ init_table[i].value);
+ if (error < 0) {
+ dev_err(&i2c->dev,
+ "failed to write 0x%x to register 0x%x\n",
+ init_table[i].value,
+ init_table[i].reg);
+ return error;
+ }
+ }
+
+ error = i2c_smbus_write_byte(i2c, BU21029_AUTOSCAN);
+ if (error < 0) {
+ dev_err(&i2c->dev, "failed to start autoscan\n");
+ return error;
+ }
+
+ return 0;
+}
+
+static int bu21029_parse_dt(struct bu21029_ts_data *bu21029)
+{
+ struct device *dev = &bu21029->client->dev;
+ struct device_node *np = dev->of_node;
+ enum of_gpio_flags flags;
+ u32 val32;
+ int gpio;
+
+ if (!np) {
+ dev_err(dev, "no device tree data\n");
+ return -EINVAL;
+ }
+
+ gpio = of_get_named_gpio_flags(np, "reset-gpios", 0, &flags);
+ if (!gpio_is_valid(gpio)) {
+ dev_err(dev, "invalid 'reset-gpios' supplied\n");
+ return -EINVAL;
+ }
+ bu21029->reset_gpios = gpio;
+ bu21029->reset_gpios_assert = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
+
+ if (of_property_read_u32(np, "rohm,x-plate-ohms", &val32)) {
+ dev_err(dev, "invalid 'x-plate-ohms' supplied\n");
+ return -EINVAL;
+ }
+ bu21029->x_plate_ohms = val32;
+
+ if (of_property_read_u32(np, "touchscreen-max-pressure", &val32))
+ bu21029->max_pressure = MAX_12BIT;
+ else
+ bu21029->max_pressure = val32;
+
+ return 0;
+}
+
+static int bu21029_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct bu21029_ts_data *bu21029;
+ struct input_dev *in_dev;
+ int error;
+
+ if (!i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_WRITE_BYTE |
+ I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
+ I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
+ dev_err(&client->dev,
+ "i2c functionality support is not sufficient\n");
+ return -EIO;
+ }
+
+ bu21029 = devm_kzalloc(&client->dev, sizeof(*bu21029), GFP_KERNEL);
+ if (!bu21029)
+ return -ENOMEM;
+
+ in_dev = devm_input_allocate_device(&client->dev);
+ if (!in_dev) {
+ dev_err(&client->dev, "unable to allocate input device\n");
+ return -ENOMEM;
+ }
+
+ bu21029->client = client;
+ bu21029->in_dev = in_dev;
+ timer_setup(&bu21029->timer, bu21029_touch_release, 0);
+
+ error = bu21029_parse_dt(bu21029);
+ if (error)
+ return error;
+
+ error = devm_gpio_request_one(&client->dev,
+ bu21029->reset_gpios,
+ GPIOF_OUT_INIT_HIGH,
+ DRIVER_NAME);
+ if (error) {
+ dev_err(&client->dev, "unable to request reset-gpios\n");
+ return error;
+ }
+
+ error = bu21029_init_chip(bu21029);
+ if (error) {
+ dev_err(&client->dev, "unable to config bu21029\n");
+ return error;
+ }
+
+ in_dev->name = DRIVER_NAME;
+ in_dev->id.bustype = BUS_I2C;
+ in_dev->dev.parent = &client->dev;
+
+ __set_bit(EV_SYN, in_dev->evbit);
+ __set_bit(EV_KEY, in_dev->evbit);
+ __set_bit(EV_ABS, in_dev->evbit);
+ __set_bit(ABS_X, in_dev->absbit);
+ __set_bit(ABS_Y, in_dev->absbit);
+ __set_bit(ABS_PRESSURE, in_dev->absbit);
+ __set_bit(BTN_TOUCH, in_dev->keybit);
+
+ input_set_abs_params(in_dev, ABS_X, 0, MAX_12BIT, 0, 0);
+ input_set_abs_params(in_dev, ABS_Y, 0, MAX_12BIT, 0, 0);
+ input_set_abs_params(in_dev, ABS_PRESSURE,
+ 0, bu21029->max_pressure, 0, 0);
+ input_set_drvdata(in_dev, bu21029);
+
+ error = input_register_device(in_dev);
+ if (error) {
+ dev_err(&client->dev, "unable to register input device\n");
+ return error;
+ }
+
+ i2c_set_clientdata(client, bu21029);
+
+ error = devm_request_threaded_irq(&client->dev,
+ client->irq,
+ NULL,
+ bu21029_touch_soft_irq,
+ IRQF_ONESHOT,
+ DRIVER_NAME,
+ bu21029);
+ if (error) {
+ dev_err(&client->dev, "unable to request touch irq\n");
+ return error;
+ }
+
+ return 0;
+}
+
+static int bu21029_remove(struct i2c_client *client)
+{
+ struct bu21029_ts_data *bu21029 = i2c_get_clientdata(client);
+
+ del_timer_sync(&bu21029->timer);
+
+ return 0;
+}
+
+static const struct i2c_device_id bu21029_ids[] = {
+ {DRIVER_NAME, 0},
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, bu21029_ids);
+
+static struct i2c_driver bu21029_driver = {
+ .driver = {
+ .name = DRIVER_NAME,
+ .owner = THIS_MODULE,
+ },
+ .id_table = bu21029_ids,
+ .probe = bu21029_probe,
+ .remove = bu21029_remove,
+};
+module_i2c_driver(bu21029_driver);
+
+MODULE_AUTHOR("Zhu Yi <yi.zhu5@cn.bosch.com>");
+MODULE_DESCRIPTION("Rohm BU21029 touchscreen controller driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
^ permalink raw reply related
* Re: ALPS Trackpoint & pressure
From: Pali Rohár @ 2018-03-21 17:48 UTC (permalink / raw)
To: Peter Hutterer; +Cc: Dmitry Torokhov, Masaki Ota, linux-input, linux-kernel
In-Reply-To: <20180208235703.GA4300@jelly>
[-- Attachment #1: Type: text/plain, Size: 4107 bytes --]
On Friday 09 February 2018 09:57:03 Peter Hutterer wrote:
> On Fri, Feb 09, 2018 at 12:21:41AM +0100, Pali Rohár wrote:
> > On Tuesday 06 February 2018 10:29:47 Peter Hutterer wrote:
> > > On Mon, Feb 05, 2018 at 02:49:55PM -0800, Dmitry Torokhov wrote:
> > > > On Sun, Feb 04, 2018 at 04:08:39PM +0100, Pali Rohár wrote:
> > > > > Hi! Now playing again with trackpoint connected to ALPS rushmore
> > > > > touchpad and I'm seeking a nice feature. Via ALPS PS/2 protocol it
> > > > > reports pressure of trackpoint. Parser for it is already implemented in
> > > > > alps.c and value is assigned to variable "z". When I just move
> > > > > trackpoint z is zero, when I push trackpoint while moving, then z is
> > > > > number higher, maximally 32. Variable "z" is set, but unused.
> > > > >
> > > > > Do we have some input interface which can be used to report this
> > > > > pressure of trackpoint to userspace? I can use this feature e.g. as
> > > > > additional button...
> > > >
> > > > We could either do the conversion in kernel and emit BTN_LEFT, or
> > > > report ABS_PRESSURE and see if userspace will be able to handle
> > > > REL_X/REL_Y/ABS_PRESSURE device.
> > > >
> > > > Adding Peter.
> > >
> > > judging by trackpoint history, I'd leave the pressure->click conversion to
> > > userspace because every trackpoint may need a different threshold setting.
> > > "easier" to have this in userspace with dmi matches etc. plus, converting to
> > > BTN_LEFT in the kernel means we cannot use it as a separate interaction
> > > anymore.
> >
> > Also BTN_LEFT is already reported when left button under trackpoint is
> > pressed. Therefore it would not be possible to distinguish between
> > trackpoint "press" and real left button press.
>
> yep, that's what I meant with "we cannot use it as separate interaction",
> we'd have no way to know how the click was generated.
>
> >
> > > That aside, I think exporting ABS_PRESSURE is fine, that's what it's there
> > > for. Nothing will use it for now though, tbh not sure yet how that would be
> > > exported from libinput. but worth filing a bug for, please assign it to me.
> >
> > Ok, so ABS_PRESSURE? Also then question is, how to report minimal and
> > maximal (possible) value? If we are going to "standardize" API for it,
> > we should also define min/max values, so userspace would be able to
> > normalize this pressure event. I can imagine that some devices can
> > report 8bit value, but ALPS rushmore reports only 5bit value.
>
> tbh, I'm not putting my hopes on this being an accurate range ever. So
> what's likely going to happen is that you pick a best-guess min/max for the
> kernel and then we have dmi-based overrides for every single trackpoint in
> userspace to tell us what a realistic threshold value for a click is and
> what the actual min/max ranges is.
>
> This is relatively easy to measure in userspace, we can pop it into
> 60-evdev.hwdb to override the min/max and ship the threshold values as
> libinput-specific files. It's awful, but most likely the best we can do.
>
> But hey, at least a min of 0 will be accurate ;)
>
> Cheers,
> Peter
Now I see that alps.c already has following initialization code for
tracksticks:
if (priv->flags & ALPS_DUALPOINT_WITH_PRESSURE) {
input_set_capability(dev2, EV_ABS, ABS_PRESSURE);
input_set_abs_params(dev2, ABS_PRESSURE, 0, 127, 0, 0);
}
And for ALPS SS4 S2 devices alps.c sets that flag. And in function
alps_process_packet_ss4_v2() for SS4_PACKET_ID_STICK it calls:
input_report_rel(dev2, REL_X, SS4_TS_X_V2(packet));
input_report_rel(dev2, REL_Y, SS4_TS_Y_V2(packet));
input_report_abs(dev2, ABS_PRESSURE, SS4_TS_Z_V2(packet));
Therefore ABS_PRESSURE is already used for one trackstick device.
I will prepare patch to export "z" axes also for other ALPS trackpoints
via that ABS_PRESSURE attribute.
Above input_set_abs_params() call already sets minimal and maximal
value, therefore this problem is solved too.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCH v5 0/4] new driver for Valve Steam Controller
From: Rodrigo Rivas Costa @ 2018-03-21 17:56 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Pierre-Loup A. Griffais, Clément VUCHENER, Jiri Kosina,
Cameron Gutman, lkml, linux-input
In-Reply-To: <CAO-hwJK3x-tgzqy_MOSfPCpeyAuJ1+uCcatY+vAf7Q16_P73Mg@mail.gmail.com>
On Wed, Mar 21, 2018 at 04:47:53PM +0100, Benjamin Tissoires wrote:
> Hi Rodrigo,
>
> On Mon, Mar 19, 2018 at 9:08 PM, Rodrigo Rivas Costa
> <rodrigorivascosta@gmail.com> wrote:
> > On Sat, Mar 17, 2018 at 02:54:07PM -0700, Pierre-Loup A. Griffais wrote:
> >>
> >>
> >> On 03/15/2018 02:06 PM, Rodrigo Rivas Costa wrote:
> >> > On Wed, Mar 14, 2018 at 05:39:25PM +0100, Benjamin Tissoires wrote:
> >> > > On Mon, Mar 12, 2018 at 9:51 PM, Rodrigo Rivas Costa
> >> > > <rodrigorivascosta@gmail.com> wrote:
> >> > > > On Mon, Mar 12, 2018 at 03:30:43PM +0100, Clément VUCHENER wrote:
> >> > > > > 2018-03-11 20:58 GMT+01:00 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>:
> >> > > > > > This patchset implements a driver for Valve Steam Controller, based on a
> >> > > > > > reverse analysis by myself.
> >> > > > > >
> >> > > > > > Sorry, I've been out of town for a few weeks and couldn't keep up with this...
> >> > > > > >
> >> > > > > > @Pierre-Loup and @Clément, could you please have another look at this and
> >> > > > > > check if it is worthy? Benjamin will not commit it without an express ACK from
> >> > > > > > Valve. Of course he is right to be cautious, but I checked this driver with
> >> > > > > > the Steam Client and all seems to go just fine. I think that there is a lot of
> >> > > > > > Linux out of the desktop that could use this driver and cannot use the Steam
> >> > > > > > Client. Worst case scenario, this driver can now be blacklisted, but I hope
> >> > > > > > that will not be needed.
> >> > > > >
> >> > > > > I tested the driver with my 4.15 fedora kernel (I only built the
> >> > > > > module not the whole kernel) and I got double inputs (your driver
> >> > > > > input device + steam uinput device) when testing Shovel Knight with
> >> > > > > Steam Big Picture. It seems to work fine when the inputs are the same,
> >> > > > > but after changing the controller configuration in Steam, the issue
> >> > > > > became apparent.
> >> > > >
> >> > > > I assumed that when several joysticks are available, games would listen
> >> > > > to one one of them. It looks like I'm wrong, and some (many?) games will
> >> > > > listen to all available joysticks at the same time. Thus having two
> >> > > > logical joysticks that represent the same physical one is not good.
> >> > >
> >> > > Yeah, the general rule of thumb is "think of the worst thing that can
> >> > > happen, someone will do something worst".
> >> > >
> >> > > >
> >> > > > An easy solution would be that Steam Client grabs this driver
> >> > > > (ioctl(EVIOCGRAB)) when creating the uinput device. Another solution
> >> > > > would be that Steam Client blacklists this driver, of course.
> >> > >
> >> > > This is 2 solutions that rely on a userspace change, and this is not
> >> > > acceptable in its current form. What if people do not upgrade Steam
> >> > > client but upgrade their kernel? Well, Steam might be able to force
> >> > > people to always run the latest shiny available version, but for other
> >> > > games, you can't expect people to have a compatible version of the
> >> > > userspace stack.
> >> >
> >> > Well, if you don't have Steam then you don't have the double input in
> >> > the first place. Unless you are using a different user-mode driver, of
> >> > course.
> >> > >
> >> > > Also, "blacklisting the driver" from Steam client is something the OS
> >> > > can do, but not the client when you run on a different distribution.
> >> > > You need root for that, and I don't want to give root permissions to
> >> > > Steam (or to any user space client that shouldn't have root privileges
> >> > > for what it matters).
> >> >
> >> > Actually Steam needs a system installation that adds udev rules to grant
> >> > uaccess to /dev/uinput and the USB raw device for the controller.
> >> > Adding a /etc/modprobe.d/steam.conf should be possible, too. It would be
> >> > a bit inconvenient because you'll need a distro update of the steam
> >> > package, not just the usual user-mode-only auto-update.
> >>
> >> It's definitely a bit tricky; we've rolled out an update to our reference
> >> package whenever we've added support for new devices (the final Steam
> >> Controller, direct PS4 gamepad led/gyro access through HID, HTC Vive and its
> >> myriad of onboard devices, bootloaders of all these things for firmware
> >> updates, etc). Whenever we have to do that, the rollout never is as smooth
> >> as desired: many users aren't using our own package; even on the
> >> distributions we support directly. Then downstream distributions adopt these
> >> udev changes with various delays (sometimes never), and port them to their
> >> own mechanism of doing things, since everyone has their own idea of a robust
> >> security model. I wish local sessions always had proper access to HID
> >> devices connected to the physical computer the user is sitting at, but I
> >> realize that the basic gaming desktop is just one of many usecases distros
> >> out there have to support and it'd be unreasonable to expect them to focus
> >> exclusively on it.
> >
> > I was afraid of something like that. Let's forget about that option for
> > the moment.
> >
> >> > > > > And without Steam and your external tool, you get double inputs too. I
> >> > > > > tried RetroArch and it was unusable because of the keyboard inputs
> >> > > > > from the lizard mode (e.g. pressing B also presses Esc and quits
> >> > > > > RetroArch). Having to download and compile an external tool to make
> >> > > > > the driver work properly may be too difficult for the user. Your goal
> >> > > > > was to provide an alternative to user space drivers but now you
> >> > > > > actually depend on (a very simple) one.
> >> > > >
> >> > > > Yes, I noticed that. TBH, this driver without Steam Client or the
> >> > > > user-space tool is not very nice, precisely because you'll get constant
> >> > > > Escape and Enter presses, and most games react to those.
> >> > > >
> >> > > > Frankly speaking, I'm not sure how to proceed. I can think of the
> >> > > > following options:
> >> > > > 1.Steam Client installation could add a file to blacklist
> >> > > > hid-steam, just as it adds a few udev rules.
> >> > >
> >> > > But what about RetroArch? And what if you install Steam but want to
> >> > > play SDL games that could benefit from your driver?
> >> >
> >> > That is an issue of solution 1. I actually have the module blacklisted
> >> > in my PC, and run `sudo modprobe hid-steam` to use SDL.
> >> >
> >> > > > 2.The default CONFIG_HID_STEAM can be changed to "n". Maybe only
> >> > > > on the architectures for which there is a Steam Client available.
> >> > > > This way DIY projects will still be able to use it.
> >> > >
> >> > > But this will make the decision to include or not the driver in
> >> > > distributions harder. And if no distribution uses it, you won't have
> >> > > free tests, and you will be alone to maintain it. So that's not ideal
> >> > > either
> >> >
> >> > Could we set the default to 'y' in non-PC systems. It would be enabled
> >> > in my Raspbian, for example... better than nothing.
> >> > >
> >> > > > 3.This driver could be abandoned :-(. Just use Steam Client if possible or
> >> > > > any of the user-mode drivers available.
> >> > >
> >> > > This would be a waste for everybody as it's always better when we share.
> >> >
> >> > Indeed!
> >> >
> >> > I tried a new option:
> >> > 4. The driver detects whether the DEVUSB/HIDRAW device is in use, and
> >> > if that is the case it will disable itself. If the DEVUSB/HIDRAW is
> >> > not in use, then the driver will work normally. A bit hackish maybe
> >> > but it should work.
> >> >
> >> > I tried doing this option 4, but I'm unable to do it properly. I don't
> >> > even know if it is possible...
> >> >
> >> > > >
> >> > > > If we decide for 1 or 2, then the lizard mode could be disabled without
> >> > > > ill effects. We could even enable the gyro and all the other gadgets
> >> > > > without worring about current compatibility.
> >> > >
> >> > > To me, 1 is out of the question. The kernel can't expect a user space
> >> > > change especially if you are knowingly introducing a bug for the end
> >> > > user.
> >> > >
> >> > > 2 is still on the table IMO, and 3 would be a shame.
> >> > >
> >> > > I know we already discussed about sysfs and module parameters, but if
> >> > > the driver will conflict with a userspace stack, the only way would be
> >> > > to have a (dynamic) parameter "enhanced_mode" or
> >> > > "kernel_awesome_support" or whatever which would be a boolean, that
> >> > > defaults to false that Steam can eventually lookup if they want so in
> >> > > the future we can default it to true. When this parameter is set, the
> >> > > driver will create the inputs and toggle the various modes, while when
> >> > > it's toggled off, it'll clean up itself and keep the device as if it
> >> > > were connected to hid-generic. Bonus point, this removes the need for
> >> > > the simple user space tool that enables the mode.
> >> >
> >> > That is doable, but that sysfs/parameter can be changed by a non-root
> >> > user? I looked for a udev rule to grant access to those but found
> >> > nothing.
> >> >
> >> > IIUC, when this parameter is false the driver will do nothing, right?
> >> > The user will just need to change it to true to be able to use it, but
> >> > that will have to be done by root.
> >> >
> >> > I'll try doing this, but I'd appreciate your advice about what approach
> >> > would be better: sysfs? a module parameter? a cdev? or even a EV_MSC?
> >> >
> >> > > > At the end of the day, I think that it is up to Valve what to do.
> >> > >
> >> > > Again, Valve is a big player here, but do not underestimate other
> >> > > projects (like RetroArch mentioned above) because if you break their
> >> > > workflow, they will have the right to request a revert of the commit
> >> > > because it breaks some random user playing games in the far end of
> >> > > Antarctica (yes, penguins do love to play games :-P )
> >> >
> >> > And everybody loves penguins! If we take away Steam (say a RaspberryPi
> >> > as a canonical example) and disable the lizard mode, then this driver is
> >> > just a regular gamepad. RetroArch should be happy with that. Unless they
> >> > already have an user mode driver for the steam-controller, of course...
> >>
> >> Both of these things seem reasonable to me, with a few caveats:
> >>
> >> - If there's an opt-in mechanism available, it would be good to ensure we
> >> have a way to reliably query its state without requiring extra permissions.
> >> This way, if we know it's likely to affect Steam client functionality, we'll
> >> have the right mechanism to properly message the situation to users.
> >>
> >> - If you find a way for the client to be able to program an opt out when
> >> it's running that is not automatic (eg. by detecting the hidraw device being
> >> opened), we'd be happy to participate with that scheme assuming it doesn't
>
> The only concern I have with detecting if hidraw is opened or not is
> that there are other tools that will want to open hidraw to do
> something entirely different. But I agree, this is the least intrusive
> solution as far as I can see.
>
> >> require extra permissions. As soon as the API is figured out, we can include
> >> it in the client, just send me a heads-up. The one thing that I'd be
> >> cautious of is robust behavior against abnormal client termination. If it's
> >> a sysfs entry we have to poke, I'm worried that if the client crashes we
> >> might not always be able to opt the driver back out. It'd be nice if it was
> >> based on opening an fd instead, this way the kernel would robustly clean up
> >> after us and your driver would kick back in.
> >
> > Ok, I've written the following scheme:
> > * I've added a member to struct steam_device: int hid_usage_count.
> > * Whenver I call hid_hw_open(), hid_usage_count is incremented.
> > * Whenver I call hid_hw_close(), hid_usage_count is decremented.
> >
> > Now, with this little function:
> >
> > static bool steam_is_only_client(struct steam_device *steam)
> > {
> > unsigned int count = steam->hdev->ll_open_count;
> > return count == steam->hid_usage_count;
> > }
> >
> > I can check if the hidraw device is opened from user-space. It is nice
> > because it works not only for SteamClient, but any other user-space
> > hid driver. And it is resistent to crashes, too.
> >
> > It is a bit hacky because I don't think ll_open_count is intended for
>
> Definitively too hacky for me :)
> It seems to be working, but AFAIU, there is still the emulated
> keyboard/mouse in place that call hid_hw_open() when they are opened.
> So you do not really know if the mismatch comes from extra eyes on the
> other hidinput devices or from hidraw.
Not really, the emulated keyboard/mouse are different USB interfaces, so
they are separeted hidraw devices, and they do not add to the
ll_open_count.
> > that, and it is usually protected by a mutex... The proper way, IMO,
> > would be to have a callback for when the hidraw is first opened/last
> > closed, but that does not exist, AFAICT.
>
> The callback doesn't exists because no one actually had the need to.
> This particular use case could be a reason to implement such system.
> However, I think we should think of the design of the 'callback'
> really carefully so it is generic enough to be reused for other cases.
>
> One thing that would be of a help here would be if you overwrite the
> low-level transport driver (struct hid_ll_driver) in hid-steam
> directly. You basically just duplicate the current ll_driver and
> overwrite only the functions you are interested in.
> If, for instance, you add a hint to hid_hw_open/close() telling who
> opened/closed the device, you could create some new entries in
> hid_ll_driver .opening and .closed that would be called before
> entering/leaving hid_hw_open/close().
That sounds interesting. I'll try if I can make it work.
> You could even snoop the various requests in hid_hw_(raw_)request() in
> case you know what to expect from the client.
Without a full protocol descrition, it may be too risky... maybe I could
wait until the client sends a feature report, to count it as a
worthy client.
> This is just an idea and not a formal ack, but it might worth
> investigating this path.
>
> >
> > But hey, it works. The mutexless access is not a big deal, because I
> > call this function on every input report, so I will get the right value
> > eventually.
>
> ouch. That's a no from me for this implementation then. Testing it for
> every input report seems overkill. Also, I do not understand correctly
> how you will send your own commands if the hidraw node is opened.
> Could you send the patch as an RFC so we have a good idea of what we
> are talking here. But from what I understand, it would be wiser to
> have the infrastructure to get notified when the hidraw node is used
> by Steam or any other client and do your magic here.
I'm not using the mutex, so it is just a unsigned comparison...
I'll post the code later today. Then, if I manage the hid_ll I will
repost.
> [sometime later]
> Actually, if there is no magic command involved to "disable" hid-steam
> and let Steam handle the touchpad itself it should be fine. But if the
> touchpad is configured in a non default mode, will Steam reset the
> controller when it opens the hidraw node? (talking about the current
> Steam client, not possible future changes)
Right, when I detect another client while hid-steam is in use, I just
sit there doing nothing, and skipping my events. OTHO, when I detect the
client is closed and hid-steam is still in use, then I disable the
lizard mode, just as if hid-steam is closed-reopened.
And I think the Steam Client is prepared for that, and resets the device
upon opening it. I've never seen Steam Client not being able to
configure the controller, and I've done some crazy configurations.
> >
> > Then if I am the only user, I can disable/enable the lizard mode when
> > opening/closing the input device. Moreover if hidraw is opened, then I
> > bypass the input events, and this gamepad goes idle, preventing the
> > double input problem. It's a bit tricky in the details, but I think I
> > got it right.
>
> Wouldn't it be better to actually kill the extra input nodes instead
> of keeping them around? This way users will only see one Steam
> controller joystick. Also, this would prevent the use case where you
> open Steam, minimize it (so the joystick are disabled), and then start
> an SDL game to wait until you download the latest patch for your
> favorite game.
I thought of that, but I see problems there. Imagine a game that
register for changes in the input devices: when anything changes, it
closes and reopens all of them. That is:
* The game opens all the input devices, including the hid-steam.
* The game opens the hidraw, because it has a user-mode driver.
* hid-steam detects a user-mode client so it deletes the mouse/keyboard
input nodes.
* The game detects that input devices have changed, closes all of them
and repeats.
And you have an infinite loop.
Instead, I'm now using feature reports to the controller to
disable/enable the lizard mode, just as the Steam Client does.
> Of course Steam could close the hidraw node on minimize, but it'll be
> better IMO to be explicit in this particular case.
Steam does not close hidraw on minimize. Instead, it changes its uinput
emulation into "desktop mode". When a game is launched it changes too,
but I'm not sure about the details... Anyway, Steam Client closes the
hidraw only when it is fully closed with the "Exit Steam" option.
> > If you don't think this solution is too hacky, I'll post a reroll with
> > this code, in a few days. I still have to do a few more tests.
>
> Please send the reroll, even if not entirely tested. It'll allow
> everyone involved to have a better idea of the solution and will help
> everyone to find the correct solution.
I'll do today, if I have the time.
Best regards.
Rodrigo
> Cheers,
> Benjamin
>
> >
> > Now, what I would really want is a review by Valve of my set-lizard function:
> >
> > static void steam_set_lizard_mode(struct steam_device *steam, bool enabled)
> > {
> > if (enabled) {
> > steam_send_report_byte(steam, 0x8e); //enable mouse
> > steam_send_report_byte(steam, 0x85); //enable esc, enter and cursor keys
> > } else {
> > steam_send_report_byte(steam, 0x81); //disable esc, enter and cursor keys
> > steam_write_register(steam, 0x08, 0x07); //disable mouse (cmd: 0x87)
> > }
> > }
> >
> > While it works, I find its asymmetry quite uncanny. I'm afraid that some
> > of these are there for a side effect, this is not their real purpose.
> > Could you give me a hint about this?
> >
> >> Note that there's a general desire on our side to create a reference
> >> userspace implementation that would more or less have the current
> >> functionality of the Steam client, but would be easily usable from other
> >> platforms where the client doesn't currentl run. Unfortunately it's quite a
> >> bit of work, so it's unclear what the timeframe would be, if it ever does
> >> happen.
> >
> > Do you mean the piece of Steam Client that does input-mapping, but as a
> > portable program without the full client? That would be awesome! And if
> > open-sourced, even awesomer!!
> >
> > Thank you all.
> > Rodrigo
> >
> >> Thanks,
> >> - Pierre-Loup
> >>
> >> >
> >> > Best regards.
> >> > Rodrigo
> >> >
> >> > > Cheers,
> >> > > Benjamin
> >> > >
> >> > > > Best Regards.
> >> > > > Rodrigo.
> >> > > >
> >> > > > > Also the button and axis codes do not match the gamepad API doc
> >> > > > > (https://www.kernel.org/doc/Documentation/input/gamepad.txt).
> >> > > > >
> >> > > > > >
> >> > > > > > For full reference, I'm adding a full changelog of this patchset.
> >> > > > > >
> >> > > > > > Changes in v5:
> >> > > > > > * Fix license SPDX to GPL-2.0+.
> >> > > > > > * Minor stylistic changes (BIT(3) instead 0x08 and so on).
> >> > > > > >
> >> > > > > > Changes in v4:
> >> > > > > > * Add command to check the wireless connection status on probe, without
> >> > > > > > waiting for a message (thanks to Clément Vuchener for the tip).
> >> > > > > > * Removed the error code on redundant connection/disconnection messages. That
> >> > > > > > was harmless but polluted dmesg.
> >> > > > > > * Added buttons for touching the left-pad and right-pad.
> >> > > > > > * Fixed a misplaced #include from 2/4 to 1/4.
> >> > > > > >
> >> > > > > > Changes in v3:
> >> > > > > > * Use RCU to do the dynamic connec/disconnect of wireless devices.
> >> > > > > > * Remove entries in hid-quirks.c as they are no longer needed. This allows
> >> > > > > > this module to be blacklisted without side effects.
> >> > > > > > * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
> >> > > > > > existing use cases (lizard mode). A user-space tool to do that is
> >> > > > > > linked.
> >> > > > > > * Fully separated axes for joystick and left-pad. As it happens.
> >> > > > > > * Add fuzz values for left/right pad axes, they are a little wiggly.
> >> > > > > >
> >> > > > > > Changes in v2:
> >> > > > > > * Remove references to USB. Now the interesting interfaces are selected by
> >> > > > > > looking for the ones with feature reports.
> >> > > > > > * Feature reports buffers are allocated with hid_alloc_report_buf().
> >> > > > > > * Feature report length is checked, to avoid overflows in case of
> >> > > > > > corrupt/malicius USB devices.
> >> > > > > > * Resolution added to the ABS axes.
> >> > > > > > * A lot of minor cleanups.
> >> > > > > >
> >> > > > > > Rodrigo Rivas Costa (4):
> >> > > > > > HID: add driver for Valve Steam Controller
> >> > > > > > HID: steam: add serial number information.
> >> > > > > > HID: steam: command to check wireless connection
> >> > > > > > HID: steam: add battery device.
> >> > > > > >
> >> > > > > > drivers/hid/Kconfig | 8 +
> >> > > > > > drivers/hid/Makefile | 1 +
> >> > > > > > drivers/hid/hid-ids.h | 4 +
> >> > > > > > drivers/hid/hid-steam.c | 794 ++++++++++++++++++++++++++++++++++++++++++++++++
> >> > > > > > 4 files changed, 807 insertions(+)
> >> > > > > > create mode 100644 drivers/hid/hid-steam.c
> >> > > > > >
> >> > > > > > --
> >> > > > > > 2.16.2
> >> > > > > >
> >> >
> >>
^ permalink raw reply
* [PATCH] Input: alps - Update documentation for trackstick v3 format
From: Pali Rohár @ 2018-03-21 20:03 UTC (permalink / raw)
To: Dmitry Torokhov, Jonathan Corbet, Masaki Ota
Cc: linux-input, linux-doc, linux-kernel
Bits for M, R and L buttons are already processed in alps. Other newly
documented bits not yet.
Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
This is based on information which Masaki Ota provided to us.
---
Documentation/input/devices/alps.rst | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/Documentation/input/devices/alps.rst b/Documentation/input/devices/alps.rst
index 6779148e428c..b556d6bde5e1 100644
--- a/Documentation/input/devices/alps.rst
+++ b/Documentation/input/devices/alps.rst
@@ -192,10 +192,13 @@ The final v3 packet type is the trackstick packet::
byte 0: 1 1 x7 y7 1 1 1 1
byte 1: 0 x6 x5 x4 x3 x2 x1 x0
byte 2: 0 y6 y5 y4 y3 y2 y1 y0
- byte 3: 0 1 0 0 1 0 0 0
- byte 4: 0 z4 z3 z2 z1 z0 ? ?
+ byte 3: 0 1 TP SW 1 M R L
+ byte 4: 0 z6 z5 z4 z3 z2 z1 z0
byte 5: 0 0 1 1 1 1 1 1
+TP means Tap SW status when tap processing is enabled or Press status when press
+processing is enabled. SW means scroll up when 4 buttons are available.
+
ALPS Absolute Mode - Protocol Version 4
---------------------------------------
--
2.11.0
^ permalink raw reply related
* [PATCH] Input: alps - Report pressure of v3 and v7 trackstick
From: Pali Rohár @ 2018-03-21 20:07 UTC (permalink / raw)
To: Dmitry Torokhov, Masaki Ota; +Cc: linux-input, linux-kernel
ALPS v3 and v7 packet formats reports trackstick pressure. This information
is already parsed in unused "z" variable.
ALPS SS4 S2 devices already reports trackstick pressure as ABS_PRESSURE
attribute, therefore reports pressure in the same way also for v3 and v7.
This patch also updates parsing v3 pressure information, it is also stored
in 7 bits.
Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
drivers/input/mouse/alps.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 010c1bcdb06d..0a9e6a3a2f9f 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -139,11 +139,11 @@ static const struct alps_model_info alps_model_data[] = {
};
static const struct alps_protocol_info alps_v3_protocol_data = {
- ALPS_PROTO_V3, 0x8f, 0x8f, ALPS_DUALPOINT
+ ALPS_PROTO_V3, 0x8f, 0x8f, ALPS_DUALPOINT | ALPS_DUALPOINT_WITH_PRESSURE
};
static const struct alps_protocol_info alps_v3_rushmore_data = {
- ALPS_PROTO_V3_RUSHMORE, 0x8f, 0x8f, ALPS_DUALPOINT
+ ALPS_PROTO_V3_RUSHMORE, 0x8f, 0x8f, ALPS_DUALPOINT | ALPS_DUALPOINT_WITH_PRESSURE
};
static const struct alps_protocol_info alps_v4_protocol_data = {
@@ -155,7 +155,7 @@ static const struct alps_protocol_info alps_v5_protocol_data = {
};
static const struct alps_protocol_info alps_v7_protocol_data = {
- ALPS_PROTO_V7, 0x48, 0x48, ALPS_DUALPOINT
+ ALPS_PROTO_V7, 0x48, 0x48, ALPS_DUALPOINT | ALPS_DUALPOINT_WITH_PRESSURE
};
static const struct alps_protocol_info alps_v8_protocol_data = {
@@ -583,7 +583,7 @@ static void alps_process_trackstick_packet_v3(struct psmouse *psmouse)
x = (s8)(((packet[0] & 0x20) << 2) | (packet[1] & 0x7f));
y = (s8)(((packet[0] & 0x10) << 3) | (packet[2] & 0x7f));
- z = (packet[4] & 0x7c) >> 2;
+ z = packet[4] & 0x7c;
/*
* The x and y values tend to be quite large, and when used
@@ -595,6 +595,7 @@ static void alps_process_trackstick_packet_v3(struct psmouse *psmouse)
input_report_rel(dev, REL_X, x);
input_report_rel(dev, REL_Y, -y);
+ input_report_abs(dev, ABS_PRESSURE, z);
/*
* Most ALPS models report the trackstick buttons in the touchpad
@@ -1118,6 +1119,7 @@ static void alps_process_trackstick_packet_v7(struct psmouse *psmouse)
input_report_rel(dev2, REL_X, (char)x);
input_report_rel(dev2, REL_Y, -((char)y));
+ input_report_abs(dev2, ABS_PRESSURE, z);
input_report_key(dev2, BTN_LEFT, left);
input_report_key(dev2, BTN_RIGHT, right);
--
2.11.0
^ permalink raw reply related
* [RFC PATCH v6 0/1] hid-steam driver with user mode client dection
From: Rodrigo Rivas Costa @ 2018-03-21 20:49 UTC (permalink / raw)
To: Benjamin Tissoires, Pierre-Loup A. Griffais,
Clément VUCHENER, Jiri Kosina, Cameron Gutman, lkml,
linux-input
Cc: Rodrigo Rivas Costa
This is a RFC for the problem of how to work with the Steam Controller from
a kernel driver without disturbing the user mode drivers.
The main user mode driver is the Steam Controller itself. From now on I will
call any user mode driver a "hidraw client" because that is the mode most of
them use.
Steam Client is able to use a raw libusb user mode driver, but it
looks like it does so only if hidraw is not available, so I'm not currently
concerned with those.
This driver, as is, does the following:
1. It keeps the mouse/keyboard emulation (lizard mode) working while the
hid-steam input device is not in use.
2. When the hid-steam input device is opened, lizard mode is disabled, but
ONLY IF there are no hidraw clients. This is necessary because with the
lizard mode on, a gesture in the gamepad can make several unrelated input
events and games go crazy.
3. When the hid-steam input device is closed, lizard mode is re-enabled, but
ONLY IF there are no hidraw clients.
4. If while the hid-steam input device is opened, a hidraw client starts,
then the input events are disabled for this driver, in order to avoid
double inputs in games that read all available game controllers.
4. If a hid-steam input device and a hidraw client are running, and then the
hidraw client finishes, then lizard mode is disabled, just like in 2.
Other than what to do exactly, IMO, there are three tricky parts that deserve
the RFC:
1. Is the procedure avobe the right way to do it?
2. How to detect whether a hidraw client is in use. Currently I'm looking at
hid_device::ll_open_count and compare with the number of times I called
hid_hw_open() - hid_hw_close(). I'm exploring using a custom hid_ll_driver
instead...
3. How to disable the lizard mode. Currently I'm sending a few feature
reports that prevent the hardware from sending keyboard/mouse events. But
maybe they can be disabled at hid level, so we can avoid messing with the
hardware configuration, and the possible incompatibilities and races that
arise.
PS. I've collapsed the patch-set into a single bigger commit, because my
original split makes sense no longer. Sorry for the inconvenience.
Thank you in advance for any comment.
Rodrigo Rivas Costa (1):
HID: add driver for Valve Steam Controller
drivers/hid/Kconfig | 8 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-ids.h | 4 +
drivers/hid/hid-steam.c | 874 ++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 887 insertions(+)
create mode 100644 drivers/hid/hid-steam.c
--
2.16.2
^ permalink raw reply
* [RFC PATCH v6 1/1] HID: add driver for Valve Steam Controller
From: Rodrigo Rivas Costa @ 2018-03-21 20:49 UTC (permalink / raw)
To: Benjamin Tissoires, Pierre-Loup A. Griffais,
Clément VUCHENER, Jiri Kosina, Cameron Gutman, lkml,
linux-input
Cc: Rodrigo Rivas Costa
In-Reply-To: <20180321204932.12687-1-rodrigorivascosta@gmail.com>
There are two ways to connect the Steam Controller: directly to the USB
or with the USB wireless adapter. Both methods are similar, but the
wireless adapter can connect up to 4 devices at the same time.
The wired device will appear as 3 interfaces: a virtual mouse, a virtual
keyboard and a custom HID device.
The wireless device will appear as 5 interfaces: a virtual keyboard and
4 custom HID devices, that will remain silent until a device is actually
connected.
The custom HID device has a report descriptor with all vendor specific
usages, so the hid-generic is not very useful. In a PC/SteamBox Valve
Steam Client provices a software translation by using direct USB access
and a creates a uinput virtual gamepad.
Signed-off-by: Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
---
drivers/hid/Kconfig | 8 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-ids.h | 4 +
drivers/hid/hid-steam.c | 874 ++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 887 insertions(+)
create mode 100644 drivers/hid/hid-steam.c
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 779c5ae47f36..de5f4849bfe4 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -811,6 +811,14 @@ config HID_SPEEDLINK
---help---
Support for Speedlink Vicious and Divine Cezanne mouse.
+config HID_STEAM
+ tristate "Steam Controller support"
+ depends on HID
+ ---help---
+ Say Y here if you have a Steam Controller if you want to use it
+ without running the Steam Client. It supports both the wired and
+ the wireless adaptor.
+
config HID_STEELSERIES
tristate "Steelseries SRW-S1 steering wheel support"
depends on HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 235bd2a7b333..e146c257285a 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -94,6 +94,7 @@ obj-$(CONFIG_HID_SAMSUNG) += hid-samsung.o
obj-$(CONFIG_HID_SMARTJOYPLUS) += hid-sjoy.o
obj-$(CONFIG_HID_SONY) += hid-sony.o
obj-$(CONFIG_HID_SPEEDLINK) += hid-speedlink.o
+obj-$(CONFIG_HID_STEAM) += hid-steam.o
obj-$(CONFIG_HID_STEELSERIES) += hid-steelseries.o
obj-$(CONFIG_HID_SUNPLUS) += hid-sunplus.o
obj-$(CONFIG_HID_GREENASIA) += hid-gaff.o
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index a0baa5ba5b84..3014991e5d4b 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -987,6 +987,10 @@
#define USB_VENDOR_ID_STANTUM_SITRONIX 0x1403
#define USB_DEVICE_ID_MTP_SITRONIX 0x5001
+#define USB_VENDOR_ID_VALVE 0x28de
+#define USB_DEVICE_ID_STEAM_CONTROLLER 0x1102
+#define USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS 0x1142
+
#define USB_VENDOR_ID_STEELSERIES 0x1038
#define USB_DEVICE_ID_STEELSERIES_SRWS1 0x1410
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
new file mode 100644
index 000000000000..88acd8f7fbf3
--- /dev/null
+++ b/drivers/hid/hid-steam.c
@@ -0,0 +1,874 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * HID driver for Valve Steam Controller
+ *
+ * Copyright (c) 2018 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
+ *
+ * Supports both the wired and wireless interfaces.
+ *
+ * For additional functions, such as disabling the virtual mouse/keyboard or
+ * changing the right-pad margin you can use the user-space tool at:
+ *
+ * https://github.com/rodrigorc/steamctrl
+ */
+
+#include <linux/device.h>
+#include <linux/input.h>
+#include <linux/hid.h>
+#include <linux/module.h>
+#include <linux/workqueue.h>
+#include <linux/mutex.h>
+#include <linux/rcupdate.h>
+#include <linux/delay.h>
+#include <linux/power_supply.h>
+#include "hid-ids.h"
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>");
+
+#define STEAM_QUIRK_WIRELESS BIT(0)
+#define STEAM_IS_ONLY_CLIENT BIT(1)
+
+#define STEAM_SERIAL_LEN 10
+/* Touch pads are 40 mm in diameter and 65535 units */
+#define STEAM_PAD_RESOLUTION 1638
+/* Trigger runs are about 5 mm and 256 units */
+#define STEAM_TRIGGER_RESOLUTION 51
+/* Joystick runs are about 5 mm and 256 units */
+#define STEAM_JOYSTICK_RESOLUTION 51
+
+#define STEAM_PAD_FUZZ 256
+
+struct steam_device {
+ spinlock_t lock;
+ struct mutex mutex;
+ struct hid_device *hdev;
+ int hid_usage_count;
+ struct input_dev __rcu *input;
+ unsigned long quirks;
+ struct work_struct work_connect, work_disable_lizard;
+ bool connected;
+ char serial_no[STEAM_SERIAL_LEN + 1];
+ struct power_supply_desc battery_desc;
+ struct power_supply __rcu *battery;
+ u8 battery_charge;
+ u16 voltage;
+};
+
+static int steam_recv_report(struct steam_device *steam,
+ u8 *data, int size)
+{
+ struct hid_report *r;
+ u8 *buf;
+ int ret;
+
+ r = steam->hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[0];
+ if (hid_report_len(r) < 64)
+ return -EINVAL;
+
+ buf = hid_alloc_report_buf(r, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ /*
+ * The report ID is always 0, so strip the first byte from the output.
+ * hid_report_len() is not counting the report ID, so +1 to the length
+ * or else we get a EOVERFLOW. We are safe from a buffer overflow
+ * because hid_alloc_report_buf() allocates +7 bytes.
+ */
+ ret = hid_hw_raw_request(steam->hdev, 0x00,
+ buf, hid_report_len(r) + 1,
+ HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+ if (ret > 0)
+ memcpy(data, buf + 1, min(size, ret - 1));
+ kfree(buf);
+ return ret;
+}
+
+static int steam_send_report(struct steam_device *steam,
+ u8 *cmd, int size)
+{
+ struct hid_report *r;
+ u8 *buf;
+ unsigned int retries = 10;
+ int ret;
+
+ r = steam->hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[0];
+ if (hid_report_len(r) < 64)
+ return -EINVAL;
+
+ buf = hid_alloc_report_buf(r, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ /* The report ID is always 0 */
+ memcpy(buf + 1, cmd, size);
+
+ /*
+ * Sometimes the wireless controller fails with EPIPE
+ * when sending a feature report.
+ * Doing a HID_REQ_GET_REPORT and waiting for a while
+ * seems to fix that.
+ */
+ do {
+ ret = hid_hw_raw_request(steam->hdev, 0,
+ buf, size + 1,
+ HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+ if (ret != -EPIPE)
+ break;
+ steam_recv_report(steam, NULL, 0);
+ msleep(50);
+ } while (--retries);
+
+ kfree(buf);
+ if (ret < 0)
+ hid_err(steam->hdev, "%s: error %d (%*ph)\n", __func__,
+ ret, size, cmd);
+ return ret;
+}
+
+static inline int steam_send_report_byte(struct steam_device *steam, u8 cmd)
+{
+ return steam_send_report(steam, &cmd, 1);
+}
+
+static inline int steam_write_register(struct steam_device *steam, u8 reg, u16 value)
+{
+ u8 cmd[] = {0x87, 0x03, reg, value & 0xFF, value >> 8};
+ return steam_send_report(steam, cmd, sizeof(cmd));
+}
+
+static int steam_get_serial(struct steam_device *steam)
+{
+ /*
+ * Send: 0xae 0x15 0x01
+ * Recv: 0xae 0x15 0x01 serialnumber (10 chars)
+ */
+ int ret;
+ u8 cmd[] = {0xae, 0x15, 0x01};
+ u8 reply[3 + STEAM_SERIAL_LEN + 1];
+
+ ret = steam_send_report(steam, cmd, sizeof(cmd));
+ if (ret < 0)
+ return ret;
+ ret = steam_recv_report(steam, reply, sizeof(reply));
+ if (ret < 0)
+ return ret;
+ reply[3 + STEAM_SERIAL_LEN] = 0;
+ strlcpy(steam->serial_no, reply + 3, sizeof(steam->serial_no));
+ return 0;
+}
+
+/*
+ * This command requests the wireless adaptor to post an event
+ * with the connection status. Useful if this driver is loaded when
+ * the controller is already connected.
+ */
+static inline int steam_request_conn_status(struct steam_device *steam)
+{
+ return steam_send_report_byte(steam, 0xb4);
+}
+
+static void steam_set_lizard_mode(struct steam_device *steam, bool enabled)
+{
+ if (enabled) {
+ steam_send_report_byte(steam, 0x8e); //enable mouse
+ steam_send_report_byte(steam, 0x85); //enable esc, enter and cursor keys
+ } else {
+ steam_send_report_byte(steam, 0x81); //disable esc, enter and cursor keys
+ steam_write_register(steam, 0x08, 0x07); //disable mouse
+ }
+}
+
+static bool steam_is_only_client(struct steam_device *steam)
+{
+ unsigned int count = steam->hdev->ll_open_count;
+ if (steam->hid_usage_count == ((steam->quirks & STEAM_QUIRK_WIRELESS) ? 1 : 0))
+ return false;
+
+ return count == steam->hid_usage_count;
+}
+
+static void steam_disable_lizard_mode(struct steam_device *steam)
+{
+ mutex_lock(&steam->mutex);
+ if (!(steam->quirks & STEAM_IS_ONLY_CLIENT) &&
+ steam_is_only_client(steam)) {
+ steam_set_lizard_mode(steam, false);
+ steam->quirks |= STEAM_IS_ONLY_CLIENT;
+ }
+ mutex_unlock(&steam->mutex);
+}
+
+static void steam_enable_lizard_mode(struct steam_device *steam)
+{
+ mutex_lock(&steam->mutex);
+ if (steam->quirks & STEAM_IS_ONLY_CLIENT) {
+ if (steam_is_only_client(steam))
+ steam_set_lizard_mode(steam, true);
+ steam->quirks &= ~STEAM_IS_ONLY_CLIENT;
+ }
+ mutex_unlock(&steam->mutex);
+}
+
+static int steam_input_open(struct input_dev *dev)
+{
+ struct steam_device *steam = input_get_drvdata(dev);
+ int res;
+
+ res = hid_hw_open(steam->hdev);
+ if (res)
+ return res;
+ ++steam->hid_usage_count;
+ steam_disable_lizard_mode(steam);
+ return 0;
+}
+
+static void steam_input_close(struct input_dev *dev)
+{
+ struct steam_device *steam = input_get_drvdata(dev);
+
+ steam_enable_lizard_mode(steam);
+ hid_hw_close(steam->hdev);
+ --steam->hid_usage_count;
+}
+
+static enum power_supply_property steam_battery_props[] = {
+ POWER_SUPPLY_PROP_PRESENT,
+ POWER_SUPPLY_PROP_SCOPE,
+ POWER_SUPPLY_PROP_VOLTAGE_NOW,
+ POWER_SUPPLY_PROP_CAPACITY,
+};
+
+static int steam_battery_get_property(struct power_supply *psy,
+ enum power_supply_property psp,
+ union power_supply_propval *val)
+{
+ struct steam_device *steam = power_supply_get_drvdata(psy);
+ unsigned long flags;
+ s16 volts;
+ u8 batt;
+ int ret = 0;
+
+ spin_lock_irqsave(&steam->lock, flags);
+ volts = steam->voltage;
+ batt = steam->battery_charge;
+ spin_unlock_irqrestore(&steam->lock, flags);
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_PRESENT:
+ val->intval = 1;
+ break;
+ case POWER_SUPPLY_PROP_SCOPE:
+ val->intval = POWER_SUPPLY_SCOPE_DEVICE;
+ break;
+ case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+ val->intval = volts * 1000; /* mV -> uV */
+ break;
+ case POWER_SUPPLY_PROP_CAPACITY:
+ val->intval = batt;
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ return ret;
+}
+
+static int steam_battery_register(struct steam_device *steam)
+{
+ struct power_supply *battery;
+ struct power_supply_config battery_cfg = { .drv_data = steam, };
+ unsigned long flags;
+ int ret;
+
+ steam->battery_desc.type = POWER_SUPPLY_TYPE_BATTERY;
+ steam->battery_desc.properties = steam_battery_props;
+ steam->battery_desc.num_properties = ARRAY_SIZE(steam_battery_props);
+ steam->battery_desc.get_property = steam_battery_get_property;
+ steam->battery_desc.name = devm_kasprintf(&steam->hdev->dev,
+ GFP_KERNEL, "steam-controller-%s-battery",
+ steam->serial_no);
+ if (!steam->battery_desc.name)
+ return -ENOMEM;
+
+ /* avoid the warning of 0% battery while waiting for the first info */
+ spin_lock_irqsave(&steam->lock, flags);
+ steam->voltage = 3000;
+ steam->battery_charge = 100;
+ spin_unlock_irqrestore(&steam->lock, flags);
+
+ battery = power_supply_register(&steam->hdev->dev,
+ &steam->battery_desc, &battery_cfg);
+ if (IS_ERR(battery)) {
+ ret = PTR_ERR(battery);
+ hid_err(steam->hdev,
+ "%s:power_supply_register failed with error %d\n",
+ __func__, ret);
+ return ret;
+ }
+ rcu_assign_pointer(steam->battery, battery);
+ power_supply_powers(battery, &steam->hdev->dev);
+ return 0;
+}
+
+static int steam_register(struct steam_device *steam)
+{
+ struct hid_device *hdev = steam->hdev;
+ struct input_dev *input;
+ int ret;
+
+ rcu_read_lock();
+ input = rcu_dereference(steam->input);
+ rcu_read_unlock();
+ if (input) {
+ dbg_hid("%s: already connected\n", __func__);
+ return 0;
+ }
+
+ ret = steam_get_serial(steam);
+ if (ret)
+ return ret;
+
+ hid_info(hdev, "Steam Controller '%s' connected",
+ steam->serial_no);
+
+ input = input_allocate_device();
+ if (!input)
+ return -ENOMEM;
+
+ input_set_drvdata(input, steam);
+ input->dev.parent = &hdev->dev;
+ input->open = steam_input_open;
+ input->close = steam_input_close;
+
+ input->name = (steam->quirks & STEAM_QUIRK_WIRELESS) ?
+ "Wireless Steam Controller" :
+ "Steam Controller";
+ input->phys = hdev->phys;
+ input->uniq = steam->serial_no;
+ input->id.bustype = hdev->bus;
+ input->id.vendor = hdev->vendor;
+ input->id.product = hdev->product;
+ input->id.version = hdev->version;
+
+ input_set_capability(input, EV_KEY, BTN_TR2);
+ input_set_capability(input, EV_KEY, BTN_TL2);
+ input_set_capability(input, EV_KEY, BTN_TR);
+ input_set_capability(input, EV_KEY, BTN_TL);
+ input_set_capability(input, EV_KEY, BTN_Y);
+ input_set_capability(input, EV_KEY, BTN_B);
+ input_set_capability(input, EV_KEY, BTN_X);
+ input_set_capability(input, EV_KEY, BTN_A);
+ input_set_capability(input, EV_KEY, BTN_SELECT);
+ input_set_capability(input, EV_KEY, BTN_MODE);
+ input_set_capability(input, EV_KEY, BTN_START);
+ input_set_capability(input, EV_KEY, BTN_GEAR_DOWN);
+ input_set_capability(input, EV_KEY, BTN_GEAR_UP);
+ input_set_capability(input, EV_KEY, BTN_THUMBR);
+ input_set_capability(input, EV_KEY, BTN_THUMBL);
+ input_set_capability(input, EV_KEY, BTN_THUMB);
+ input_set_capability(input, EV_KEY, BTN_THUMB2);
+
+ input_set_abs_params(input, ABS_Z, 0, 255, 0, 0);
+ input_set_abs_params(input, ABS_RZ, 0, 255, 0, 0);
+ input_set_abs_params(input, ABS_X, -32767, 32767, 0, 0);
+ input_set_abs_params(input, ABS_Y, -32767, 32767, 0, 0);
+ input_set_abs_params(input, ABS_RX, -32767, 32767,
+ STEAM_PAD_FUZZ, 0);
+ input_set_abs_params(input, ABS_RY, -32767, 32767,
+ STEAM_PAD_FUZZ, 0);
+ input_set_abs_params(input, ABS_HAT1X, -32767, 32767,
+ STEAM_PAD_FUZZ, 0);
+ input_set_abs_params(input, ABS_HAT1Y, -32767, 32767,
+ STEAM_PAD_FUZZ, 0);
+ input_set_abs_params(input, ABS_HAT0X, -1, 1, 0, 0);
+ input_set_abs_params(input, ABS_HAT0Y, -1, 1, 0, 0);
+ input_abs_set_res(input, ABS_X, STEAM_JOYSTICK_RESOLUTION);
+ input_abs_set_res(input, ABS_Y, STEAM_JOYSTICK_RESOLUTION);
+ input_abs_set_res(input, ABS_RX, STEAM_PAD_RESOLUTION);
+ input_abs_set_res(input, ABS_RY, STEAM_PAD_RESOLUTION);
+ input_abs_set_res(input, ABS_HAT1X, STEAM_PAD_RESOLUTION);
+ input_abs_set_res(input, ABS_HAT1Y, STEAM_PAD_RESOLUTION);
+ input_abs_set_res(input, ABS_Z, STEAM_TRIGGER_RESOLUTION);
+ input_abs_set_res(input, ABS_RZ, STEAM_TRIGGER_RESOLUTION);
+
+ ret = input_register_device(input);
+ if (ret)
+ goto input_register_fail;
+
+ rcu_assign_pointer(steam->input, input);
+
+ /* ignore battery errors, we can live without it */
+ if (steam->quirks & STEAM_QUIRK_WIRELESS)
+ steam_battery_register(steam);
+
+ return 0;
+
+input_register_fail:
+ input_free_device(input);
+ return ret;
+}
+
+static void steam_unregister(struct steam_device *steam)
+{
+ struct input_dev *input;
+ struct power_supply *battery;
+
+ rcu_read_lock();
+ input = rcu_dereference(steam->input);
+ battery = rcu_dereference(steam->battery);
+ rcu_read_unlock();
+
+ if (battery) {
+ RCU_INIT_POINTER(steam->battery, NULL);
+ synchronize_rcu();
+ power_supply_unregister(battery);
+ }
+ if (input) {
+ RCU_INIT_POINTER(steam->input, NULL);
+ synchronize_rcu();
+ hid_info(steam->hdev, "Steam Controller '%s' disconnected",
+ steam->serial_no);
+ input_unregister_device(input);
+ }
+}
+
+static void steam_work_connect_cb(struct work_struct *work)
+{
+ struct steam_device *steam = container_of(work, struct steam_device,
+ work_connect);
+ unsigned long flags;
+ bool connected;
+ int ret;
+
+ spin_lock_irqsave(&steam->lock, flags);
+ connected = steam->connected;
+ spin_unlock_irqrestore(&steam->lock, flags);
+
+ if (connected) {
+ ret = steam_register(steam);
+ if (ret) {
+ hid_err(steam->hdev,
+ "%s:steam_register failed with error %d\n",
+ __func__, ret);
+ }
+ } else {
+ steam_unregister(steam);
+ }
+}
+
+static void steam_work_disable_lizard_cb(struct work_struct *work)
+{
+ struct steam_device *steam = container_of(work, struct steam_device,
+ work_disable_lizard);
+ steam_disable_lizard_mode(steam);
+}
+
+
+static bool steam_is_valve_interface(struct hid_device *hdev)
+{
+ struct hid_report_enum *rep_enum;
+
+ /*
+ * The wired device creates 3 interfaces:
+ * 0: emulated mouse.
+ * 1: emulated keyboard.
+ * 2: the real game pad.
+ * The wireless device creates 5 interfaces:
+ * 0: emulated keyboard.
+ * 1-4: slots where up to 4 real game pads will be connected to.
+ * We know which one is the real gamepad interface because they are the
+ * only ones with a feature report.
+ */
+ rep_enum = &hdev->report_enum[HID_FEATURE_REPORT];
+ return !list_empty(&rep_enum->report_list);
+}
+
+static int steam_probe(struct hid_device *hdev,
+ const struct hid_device_id *id)
+{
+ struct steam_device *steam;
+ int ret;
+
+ ret = hid_parse(hdev);
+ if (ret) {
+ hid_err(hdev,
+ "%s:parse of hid interface failed\n", __func__);
+ return ret;
+ }
+
+ /*
+ * Connect the HID device so that Steam Controller keeps on working
+ * without changes.
+ */
+ ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+ if (ret) {
+ hid_err(hdev,
+ "%s:hid_hw_start failed with error %d\n",
+ __func__, ret);
+ return ret;
+ }
+
+ if (!steam_is_valve_interface(hdev))
+ return 0;
+
+ /*
+ * From this point on, if anything fails return 0 and ignores
+ * the error, so that the default HID devices are still bound.
+ */
+ steam = devm_kzalloc(&hdev->dev, sizeof(*steam), GFP_KERNEL);
+ if (!steam) {
+ ret = -ENOMEM;
+ goto mem_fail;
+ }
+
+ spin_lock_init(&steam->lock);
+ mutex_init(&steam->mutex);
+ steam->hdev = hdev;
+ hid_set_drvdata(hdev, steam);
+ steam->quirks = id->driver_data;
+ INIT_WORK(&steam->work_connect, steam_work_connect_cb);
+ INIT_WORK(&steam->work_disable_lizard, steam_work_disable_lizard_cb);
+
+ if (steam->quirks & STEAM_QUIRK_WIRELESS) {
+ ret = hid_hw_open(hdev);
+ if (ret) {
+ hid_err(hdev,
+ "%s:hid_hw_open for wireless\n",
+ __func__);
+ goto hid_hw_open_fail;
+ }
+ ++steam->hid_usage_count;
+ hid_info(hdev, "Steam wireless receiver connected");
+ steam_request_conn_status(steam);
+ } else {
+ ret = steam_register(steam);
+ if (ret) {
+ hid_err(hdev,
+ "%s:steam_register failed with error %d\n",
+ __func__, ret);
+ goto input_register_fail;
+ }
+ }
+
+ return 0;
+
+input_register_fail:
+hid_hw_open_fail:
+ cancel_work_sync(&steam->work_disable_lizard);
+ cancel_work_sync(&steam->work_connect);
+ hid_set_drvdata(hdev, NULL);
+mem_fail:
+ hid_err(hdev, "%s: failed with error %d\n",
+ __func__, ret);
+ return 0;
+}
+
+static void steam_remove(struct hid_device *hdev)
+{
+ struct steam_device *steam = hid_get_drvdata(hdev);
+
+ if (!steam) {
+ hid_hw_stop(hdev);
+ return;
+ }
+
+ if (steam->quirks & STEAM_QUIRK_WIRELESS) {
+ hid_info(hdev, "Steam wireless receiver disconnected");
+ hid_hw_close(hdev);
+ --steam->hid_usage_count;
+ }
+ hid_hw_stop(hdev);
+ cancel_work_sync(&steam->work_disable_lizard);
+ cancel_work_sync(&steam->work_connect);
+ steam_unregister(steam);
+}
+
+static void steam_do_connect_event(struct steam_device *steam, bool connected)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&steam->lock, flags);
+ steam->connected = connected;
+ spin_unlock_irqrestore(&steam->lock, flags);
+
+ if (schedule_work(&steam->work_connect) == 0)
+ dbg_hid("%s: connected=%d event already queued\n",
+ __func__, connected);
+}
+
+/*
+ * The size for this message payload is 60.
+ * The known values are:
+ * (* values are not sent through wireless)
+ * (* accelerator/gyro is disabled by default)
+ * Offset| Type | Mapped to |Meaning
+ * -------+-------+-----------+--------------------------
+ * 4-7 | u32 | -- | sequence number
+ * 8-10 | 24bit | see below | buttons
+ * 11 | u8 | ABS_Z | left trigger
+ * 12 | u8 | ABS_RZ | right trigger
+ * 13-15 | -- | -- | always 0
+ * 16-17 | s16 | ABS_X | X value
+ * 18-19 | s16 | ABS_Y | Y value
+ * 20-21 | s16 | ABS_RX | right-pad X value
+ * 22-23 | s16 | ABS_RY | right-pad Y value
+ * 24-25 | s16 | -- | * left trigger
+ * 26-27 | s16 | -- | * right trigger
+ * 28-29 | s16 | -- | * accelerometer X value
+ * 30-31 | s16 | -- | * accelerometer Y value
+ * 32-33 | s16 | -- | * accelerometer Z value
+ * 34-35 | s16 | -- | gyro X value
+ * 36-36 | s16 | -- | gyro Y value
+ * 38-39 | s16 | -- | gyro Z value
+ * 40-41 | s16 | -- | quaternion W value
+ * 42-43 | s16 | -- | quaternion X value
+ * 44-45 | s16 | -- | quaternion Y value
+ * 46-47 | s16 | -- | quaternion Z value
+ * 48-49 | -- | -- | always 0
+ * 50-51 | s16 | -- | * left trigger (uncalibrated)
+ * 52-53 | s16 | -- | * right trigger (uncalibrated)
+ * 54-55 | s16 | -- | * joystick X value (uncalibrated)
+ * 56-57 | s16 | -- | * joystick Y value (uncalibrated)
+ * 58-59 | s16 | -- | * left-pad X value
+ * 60-61 | s16 | -- | * left-pad Y value
+ * 62-63 | u16 | -- | * battery voltage
+ *
+ * The buttons are:
+ * Bit | Mapped to | Description
+ * ------+------------+--------------------------------
+ * 8.0 | BTN_TR2 | right trigger fully pressed
+ * 8.1 | BTN_TL2 | left trigger fully pressed
+ * 8.2 | BTN_TR | right shoulder
+ * 8.3 | BTN_TL | left shoulder
+ * 8.4 | BTN_Y | button Y
+ * 8.5 | BTN_B | button B
+ * 8.6 | BTN_X | button X
+ * 8.7 | BTN_A | button A
+ * 9.0 | -ABS_HAT0Y | lef-pad up
+ * 9.1 | +ABS_HAT0X | lef-pad right
+ * 9.2 | -ABS_HAT0X | lef-pad left
+ * 9.3 | +ABS_HAT0Y | lef-pad down
+ * 9.4 | BTN_SELECT | menu left
+ * 9.5 | BTN_MODE | steam logo
+ * 9.6 | BTN_START | menu right
+ * 9.7 | BTN_GEAR_DOWN | left back lever
+ * 10.0 | BTN_GEAR_UP | right back lever
+ * 10.1 | -- | left-pad clicked
+ * 10.2 | BTN_THUMBR | right-pad clicked
+ * 10.3 | BTN_THUMB | left-pad touched (but see explanation below)
+ * 10.4 | BTN_THUMB2 | right-pad touched
+ * 10.5 | -- | unknown
+ * 10.6 | BTN_THUMBL | joystick clicked
+ * 10.7 | -- | lpad_and_joy
+ */
+
+static void steam_do_input_event(struct steam_device *steam,
+ struct input_dev *input, u8 *data)
+{
+ /* 24 bits of buttons */
+ u8 b8, b9, b10;
+ bool lpad_touched, lpad_and_joy;
+
+ b8 = data[8];
+ b9 = data[9];
+ b10 = data[10];
+
+ input_report_abs(input, ABS_Z, data[11]);
+ input_report_abs(input, ABS_RZ, data[12]);
+
+ /*
+ * These two bits tells how to interpret the values X and Y.
+ * lpad_and_joy tells that the joystick and the lpad are used at the
+ * same time.
+ * lpad_touched tells whether X/Y are to be read as lpad coord or
+ * joystick values.
+ * (lpad_touched || lpad_and_joy) tells if the lpad is really touched.
+ */
+ lpad_touched = b10 & BIT(3);
+ lpad_and_joy = b10 & BIT(7);
+ input_report_abs(input, lpad_touched ? ABS_HAT1X : ABS_X,
+ (s16) le16_to_cpup((__le16 *)(data + 16)));
+ input_report_abs(input, lpad_touched ? ABS_HAT1Y : ABS_Y,
+ -(s16) le16_to_cpup((__le16 *)(data + 18)));
+ /* Check if joystick is centered */
+ if (lpad_touched && !lpad_and_joy) {
+ input_report_abs(input, ABS_X, 0);
+ input_report_abs(input, ABS_Y, 0);
+ }
+ /* Check if lpad is untouched */
+ if (!(lpad_touched || lpad_and_joy)) {
+ input_report_abs(input, ABS_HAT1X, 0);
+ input_report_abs(input, ABS_HAT1Y, 0);
+ }
+
+ input_report_abs(input, ABS_RX,
+ (s16) le16_to_cpup((__le16 *)(data + 20)));
+ input_report_abs(input, ABS_RY,
+ -(s16) le16_to_cpup((__le16 *)(data + 22)));
+
+ input_event(input, EV_KEY, BTN_TR2, !!(b8 & BIT(0)));
+ input_event(input, EV_KEY, BTN_TL2, !!(b8 & BIT(1)));
+ input_event(input, EV_KEY, BTN_TR, !!(b8 & BIT(2)));
+ input_event(input, EV_KEY, BTN_TL, !!(b8 & BIT(3)));
+ input_event(input, EV_KEY, BTN_Y, !!(b8 & BIT(4)));
+ input_event(input, EV_KEY, BTN_B, !!(b8 & BIT(5)));
+ input_event(input, EV_KEY, BTN_X, !!(b8 & BIT(6)));
+ input_event(input, EV_KEY, BTN_A, !!(b8 & BIT(7)));
+ input_event(input, EV_KEY, BTN_SELECT, !!(b9 & BIT(4)));
+ input_event(input, EV_KEY, BTN_MODE, !!(b9 & BIT(5)));
+ input_event(input, EV_KEY, BTN_START, !!(b9 & BIT(6)));
+ input_event(input, EV_KEY, BTN_GEAR_DOWN, !!(b9 & BIT(7)));
+ input_event(input, EV_KEY, BTN_GEAR_UP, !!(b10 & BIT(0)));
+ input_event(input, EV_KEY, BTN_THUMBR, !!(b10 & BIT(2)));
+ input_event(input, EV_KEY, BTN_THUMBL, !!(b10 & BIT(6)));
+ input_event(input, EV_KEY, BTN_THUMB, lpad_touched || lpad_and_joy);
+ input_event(input, EV_KEY, BTN_THUMB2, !!(b10 & BIT(4)));
+
+ input_report_abs(input, ABS_HAT0X,
+ !!(b9 & BIT(1)) - !!(b9 & BIT(2)));
+ input_report_abs(input, ABS_HAT0Y,
+ !!(b9 & BIT(3)) - !!(b9 & BIT(0)));
+
+ input_sync(input);
+}
+
+/*
+ * The size for this message payload is 11.
+ * The known values are:
+ * Offset| Type | Meaning
+ * -------+-------+---------------------------
+ * 4-7 | u32 | sequence number
+ * 8-11 | -- | always 0
+ * 12-13 | u16 | voltage (mV)
+ * 14 | u8 | battery percent
+ */
+static void steam_do_battery_event(struct steam_device *steam,
+ struct power_supply *battery, u8 *data)
+{
+ unsigned long flags;
+
+ s16 volts = le16_to_cpup((__le16 *)(data + 12));
+ u8 batt = data[14];
+
+ /* Creating the battery may have failed */
+ rcu_read_lock();
+ battery = rcu_dereference(steam->battery);
+ if (likely(battery)) {
+ spin_lock_irqsave(&steam->lock, flags);
+ steam->voltage = volts;
+ steam->battery_charge = batt;
+ spin_unlock_irqrestore(&steam->lock, flags);
+ power_supply_changed(battery);
+ }
+ rcu_read_unlock();
+}
+
+static int steam_raw_event(struct hid_device *hdev,
+ struct hid_report *report, u8 *data,
+ int size)
+{
+ struct steam_device *steam = hid_get_drvdata(hdev);
+ struct input_dev *input;
+ struct power_supply *battery;
+
+ if (!steam)
+ return 0;
+
+ /*
+ * All messages are size=64, all values little-endian.
+ * The format is:
+ * Offset| Meaning
+ * -------+--------------------------------------------
+ * 0-1 | always 0x01, 0x00, maybe protocol version?
+ * 2 | type of message
+ * 3 | length of the real payload (not checked)
+ * 4-n | payload data, depends on the type
+ *
+ * There are these known types of message:
+ * 0x01: input data (60 bytes)
+ * 0x03: wireless connect/disconnect (1 byte)
+ * 0x04: battery status (11 bytes)
+ */
+
+ if (size != 64 || data[0] != 1 || data[1] != 0)
+ return 0;
+
+ switch (data[2]) {
+ case 0x01:
+ if (!steam_is_only_client(steam)) {
+ steam->quirks &= ~STEAM_IS_ONLY_CLIENT;
+ return 0;
+ }
+ if (!(steam->quirks & STEAM_IS_ONLY_CLIENT))
+ schedule_work(&steam->work_disable_lizard);
+ rcu_read_lock();
+ input = rcu_dereference(steam->input);
+ if (likely(input)) {
+ steam_do_input_event(steam, input, data);
+ } else {
+ dbg_hid("%s: input data without connect event\n",
+ __func__);
+ steam_do_connect_event(steam, true);
+ }
+ rcu_read_unlock();
+ break;
+ case 0x03:
+ /*
+ * The payload of this event is a single byte:
+ * 0x01: disconnected.
+ * 0x02: connected.
+ */
+ switch (data[4]) {
+ case 0x01:
+ steam_do_connect_event(steam, false);
+ break;
+ case 0x02:
+ steam_do_connect_event(steam, true);
+ break;
+ }
+ break;
+ case 0x04:
+ if (steam->quirks & STEAM_QUIRK_WIRELESS) {
+ rcu_read_lock();
+ battery = rcu_dereference(steam->battery);
+ if (likely(battery)) {
+ steam_do_battery_event(steam, battery, data);
+ } else {
+ dbg_hid(
+ "%s: battery data without connect event\n",
+ __func__);
+ steam_do_connect_event(steam, true);
+ }
+ rcu_read_unlock();
+ }
+ break;
+ }
+ return 0;
+}
+
+static const struct hid_device_id steam_controllers[] = {
+ { /* Wired Steam Controller */
+ HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
+ USB_DEVICE_ID_STEAM_CONTROLLER)
+ },
+ { /* Wireless Steam Controller */
+ HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
+ USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS),
+ .driver_data = STEAM_QUIRK_WIRELESS
+ },
+ {}
+};
+
+MODULE_DEVICE_TABLE(hid, steam_controllers);
+
+static struct hid_driver steam_controller_driver = {
+ .name = "hid-steam",
+ .id_table = steam_controllers,
+ .probe = steam_probe,
+ .remove = steam_remove,
+ .raw_event = steam_raw_event,
+};
+
+module_hid_driver(steam_controller_driver);
--
2.16.2
^ permalink raw reply related
* Re: [PATCH v6 0/6] Add MediaTek PMIC keys support
From: Sean Wang @ 2018-03-22 2:17 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Lee Jones, Alexandre Belloni
Cc: Mark Rutland, a.zummo, devicetree, linus.walleij, jcsing.lee,
linux-kernel, krzk, javier, linux-mediatek, linux-arm-kernel,
linux-input, matthias.bgg, eddie.huang, Chen.Zhong, beomho.seo,
linux-rtc
In-Reply-To: <808b2029da224ab687fb0553cbc1c68e@mtkmbs08n1.mediatek.inc>
Hi, Dmitry and Lee
The series seems not being got merged. Are they good enough to be ready
into the your tree?
Recently I've tested the series with focusing on pwrkey event generated
through interrupt when push and release the key on bpi-r2 board and then
finally it's working fine. but for homekey it cannot be found on the
board and thus I cannot have more tests more about it.
Tested-by: Sean Wang <sean.wang@mediatek.com>
Sean
On Thu, 2018-03-22 at 10:03 +0800, Sean Wang (王志亘) wrote:
>
> > -----Original Message-----
> > From: Linux-mediatek [mailto:linux-mediatek-bounces@lists.infradead.org] On
> > Behalf Of Chen Zhong
> > Sent: Wednesday, October 25, 2017 9:16 PM
> > To: Dmitry Torokhov <dmitry.torokhov@gmail.com>; Rob Herring
> > <robh+dt@kernel.org>; Lee Jones <lee.jones@linaro.org>; Alexandre Belloni
> > <alexandre.belloni@free-electrons.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>; Alessandro Zummo
> > <a.zummo@towertech.it>; devicetree@vger.kernel.org; Linus Walleij
> > <linus.walleij@linaro.org>; Jaechul Lee <jcsing.lee@samsung.com>; linux-
> > kernel@vger.kernel.org; Krzysztof Kozlowski <krzk@kernel.org>; Javier
> > Martinez Canillas <javier@osg.samsung.com>; linux-
> > mediatek@lists.infradead.org; linux-arm-kernel@lists.infradead.org; linux-
> > input@vger.kernel.org; Matthias Brugger <matthias.bgg@gmail.com>; Eddie
> > Huang (S智) <eddie.huang@mediatek.com>; Chen Zhong (钟辰)
> > <Chen.Zhong@mediatek.com>; Beomho Seo <beomho.seo@samsung.com>;
> > linux-rtc@vger.kernel.org
> > Subject: [PATCH v6 0/6] Add MediaTek PMIC keys support
> >
> > MediaTek PMIC are multi-function devices that can handle key interrupts,
> > typically there are two keys attached to PMIC, which called pwrkey and
> > homekey. PWRKEY usually used to wake up system from sleep. Homekey can
> > used as volume down key due to board design. Long press keys can shutdown
> > PMIC, the mode can be choose to be one key only or two keys together.
> > This series add support for key functions for MediaTek PMIC MT6397/MT6323.
> >
> > Changes since v5:
> > - use __maybe_unused annotation instead of #ifdef guard
> > - use of_* API instead of device_* API
> >
> > Changes since v4:
> > - rebase to Linux 4.14-rc1
> > - add a common keyboard binding document
> > - use child device tree node to define each key
> >
> > Changes since v3:
> > - make the naming to be consistent as mtk_pmic or MTK_PMIC
> > - add suspend/resume functions to enable/disable irq
> > - change binding properties to define wakeup sources
> >
> > Changes since v2:
> > - use standard properties for keycodes and debounce time
> > - change to use platform_get_irq in leaf drivers
> > - use better ways to define IRQ resources
> >
> > Changes since v1:
> > - create irq mappings in mfd core driver instead of leaf drivers
> > - remove some unused parts in mtk-pmic-keys driver
> >
> > Chen Zhong (6):
> > mfd: mt6397: create irq mappings in mfd core driver
> > dt-bindings: input: Add common keyboard document bindings
> > dt-bindings: input: Add document bindings for mtk-pmic-keys
> > dt-bindings: mfd: Add bindings for the keys as subnode of PMIC
> > input: Add MediaTek PMIC keys support
> > mfd: mt6397: Add PMIC keys support to MT6397 driver
> >
> > Documentation/devicetree/bindings/input/keys.txt | 8 +
> > .../devicetree/bindings/input/mtk-pmic-keys.txt | 43 +++
> > Documentation/devicetree/bindings/mfd/mt6397.txt | 6 +
> > drivers/input/keyboard/Kconfig | 9 +
> > drivers/input/keyboard/Makefile | 1 +
> > drivers/input/keyboard/mtk-pmic-keys.c | 339
> > +++++++++++++++++++++
> > drivers/mfd/mt6397-core.c | 26 +-
> > drivers/rtc/rtc-mt6397.c | 7 +-
> > 8 files changed, 432 insertions(+), 7 deletions(-) create mode 100644
> > Documentation/devicetree/bindings/input/keys.txt
> > create mode 100644 Documentation/devicetree/bindings/input/mtk-pmic-
> > keys.txt
> > create mode 100644 drivers/input/keyboard/mtk-pmic-keys.c
> >
> > --
> > 1.9.1
> >
> >
> >
> > _______________________________________________
> > Linux-mediatek mailing list
> > Linux-mediatek@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply
* Re: [PATCH 0/7] HID core and multitouch fixups
From: Peter Hutterer @ 2018-03-22 4:42 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Dmitry Torokhov, linux-input, linux-kernel
In-Reply-To: <20180320110451.16582-1-benjamin.tissoires@redhat.com>
On Tue, Mar 20, 2018 at 12:04:44PM +0100, Benjamin Tissoires wrote:
> Hi,
>
> Patches 1 and 2 are related to the Razer Blade Stealth that has some dead zone
> near the edges of the touchpad.
> Patches 3 was previously sent and reviewed by Dmitry and he suggested patch 4
> at the time.
> Patches 5..7 are cleanups I realized while trying to merge hid-multitouch
> into hid-core, so that other drivers could reuse the hid-mt logic (but it's
> not that easy I must confess).
Series: Acked-by: Peter Hutterer <peter.hutterer@who-t.net>
Cheers,
Peter
>
> Cheers,
> Benjamin
>
> Benjamin Tissoires (7):
> HID: multitouch: export a quirk for the button handling of touchpads
> HID: multitouch: remove dead zones of Razer Blade Stealth
> HID: use BIT macro instead of plain integers for flags
> HID: use BIT() macro for quirks too
> HID: core: remove the need for HID_QUIRK_NO_EMPTY_INPUT
> HID: multitouch: do not set HID_QUIRK_NO_INIT_REPORTS
> HID: core: reset the quirks before calling probe again
>
> drivers/hid/hid-asus.c | 3 +-
> drivers/hid/hid-core.c | 2 ++
> drivers/hid/hid-input.c | 10 +++----
> drivers/hid/hid-multitouch.c | 61 ++++++++++++++++-------------------------
> drivers/hid/hid-uclogic.c | 1 -
> include/linux/hid.h | 65 ++++++++++++++++++++++----------------------
> 6 files changed, 65 insertions(+), 77 deletions(-)
>
> --
> 2.14.3
>
> --
> 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
>
^ permalink raw reply
* Re: [PATCH v3] Fix modifier keys for Redragon Asura Keyboard
From: Robert Munteanu @ 2018-03-22 8:17 UTC (permalink / raw)
To: Ondrej Zary; +Cc: Jiri Kosina, Benjamin Tissoires, linux-kernel, linux-input
In-Reply-To: <201803211751.04734.linux@rainbow-software.org>
On Wed, 2018-03-21 at 17:51 +0100, Ondrej Zary wrote:
> On Wednesday 21 March 2018 11:43:31 Robert Munteanu wrote:
> > +#define USB_VENDOR_ID_MICRODIA 0x0c45
> > +#define USB_DEVICE_ID_REDRAGON_ASURA 0x760b
>
> Microdia is most probably an incorrect name. The 0c45 id probably
> belongs
> to "Sonix Technology Co., Ltd."
Right, I was unsure about that. I'll remove the microdia references and
use sonix.
Robert
^ permalink raw reply
* Re: [PATCH v3] Fix modifier keys for Redragon Asura Keyboard
From: Benjamin Tissoires @ 2018-03-22 8:33 UTC (permalink / raw)
To: Robert Munteanu; +Cc: Jiri Kosina, lkml, open list:HID CORE LAYER
In-Reply-To: <c2196c49d44693b5b0950132ceb42b7910c462dd.camel@apache.org>
On Wed, Mar 21, 2018 at 5:33 PM, Robert Munteanu <rombert@apache.org> wrote:
> Hi Benjamin,
>
> On Wed, 2018-03-21 at 17:12 +0100, Benjamin Tissoires wrote:
>> Hi Robert,
>>
>> First, apologies for not answering to the RFC. I missed it and it
>> fell
>> down in my INBOX.
>>
>> On Wed, Mar 21, 2018 at 11:43 AM, Robert Munteanu <rombert@apache.org
>> > wrote:
>> > The microdia family of keyboards uses a non-standard way of sending
>> > modifier keys.
>> >
>> > The down event always sets the first bit to 0x04 and the second
>> > keycode
>>
>> Pretty sure you are talking about bytes here, not bits.
>> And in the HID world, the first byte is usually a report ID.
>
> Right, thanks for spotting that.
>
>>
>> > to a custom value For instance, left shift sends the following bits
>> >
>> > 04 02 00 00 00 00 00 00
>> >
>> > while left control sends
>> >
>> > 04 01 00 00 00 00 00 00
>>
>> This sounds like bit(0) is mapped to LEFT_SHIFT and bit(1) mapped to
>> LEFT_CONTROL in the second byte.
>> Fortunately, LeftControl is designed in HID as 0xe0 in the keyboard
>> HID usage table, and LeftShift is 0xe1. So that would match the
>> behavior you are seeing.
>> If you have 04 04 00 00 00 00 00 00 when pressing LeftAlt, then
>> definitively you are just facing a bitmask, which is fairly common.
>>
>> >
>> > As a result all modifier keys are mapped to left shift and the
>> > keyboard is
>> > non-functional in that respect. To solve the problem, we capture
>> > the
>> > raw data in raw_event and manually generate the correct input
>> > events.
>>
>> I'd like to see the hid-recorder events from these keypresses. The
>> device might be buggy, but I have a gut feeling your solution is not
>> the simplest one.
>> Please grab hid-recorder from http://bentiss.github.io/hid-replay-
>> docs/
>
> See below, I recorded the output for pressing left ctrl, left alt, left
> shift, right ctrl, right alt, right shift, meta.
>
> D: 0
> R: 169 05 0c 09 01 a1 01 85 01 19 00 2a 80 03 15 00 26 80 03 95 01 75
> 10 81 00 c0 05 01 09 80 a1 01 85 02 19 81 29 83 15 00 25 01 75 01 95 03
> 81 02 95 05 81 01 c0 06 00 ff 09 01 a1 01 85 03 1a f1 00 2a f8 00 15 00
> 25 01 75 01 95 08 81 02 c0 05 01 09 06 a1 01 85 04 05 07 19 e0 29 e7 15
> 00 25 01 75 01 95 08 81 00 95 30 75 01 15 00 25 01 05 07 19 00 29 2f 81
> 02 c0 05 01 09 06 a1 01 85 05 95 38 75 01 15 00 25 01 05 07 19 30 29 67
> 81 02 c0 05 01 09 06 a1 01 85 06 95 38 75 01 15 00 25 01 05 07 19 68 29
> 9f 81 02 c0
> N: USB Keyboard
> P: usb-0000:00:14.0-6/input1
> I: 3 0c45 760b
> D: 0
> E: 0.000000 8 04 01 00 00 00 00 00 00
> E: 0.039920 8 04 00 00 00 00 00 00 00
> E: 0.751952 8 04 04 00 00 00 00 00 00
> E: 0.823977 8 04 00 00 00 00 00 00 00
> E: 2.639887 8 04 02 00 00 00 00 00 00
> E: 2.711896 8 04 00 00 00 00 00 00 00
> E: 3.583932 8 04 10 00 00 00 00 00 00
> E: 3.663919 8 04 00 00 00 00 00 00 00
> E: 4.871886 8 04 40 00 00 00 00 00 00
> E: 4.935906 8 04 00 00 00 00 00 00 00
> E: 6.503872 8 04 20 00 00 00 00 00 00
> E: 6.575850 8 04 00 00 00 00 00 00 00
> E: 7.383844 8 04 08 00 00 00 00 00 00
> E: 7.455861 8 04 00 00 00 00 00 00 00
Thanks. So it's indeed an issue with the report descriptor. Not sure
how this can also work under Windows.
FYI, the Report ID 4 translates to:
0x05, 0x01, // Usage Page (Generic Desktop) 78
0x09, 0x06, // Usage (Keyboard) 80
0xa1, 0x01, // Collection (Application) 82
0x85, 0x04, // Report ID (4) 84
0x05, 0x07, // Usage Page (Keyboard) 86
0x19, 0xe0, // Usage Minimum (224) 88
0x29, 0xe7, // Usage Maximum (231) 90
0x15, 0x00, // Logical Minimum (0) 92
0x25, 0x01, // Logical Maximum (1) 94
0x75, 0x01, // Report Size (1) 96
0x95, 0x08, // Report Count (8) 98
0x81, 0x00, // Input (Data,Arr,Abs) 100
0x95, 0x30, // Report Count (48) 102
0x75, 0x01, // Report Size (1) 104
0x15, 0x00, // Logical Minimum (0) 106
0x25, 0x01, // Logical Maximum (1) 108
0x05, 0x07, // Usage Page (Keyboard) 110
0x19, 0x00, // Usage Minimum (0) 112
0x29, 0x2f, // Usage Maximum (47) 114
0x81, 0x02, // Input (Data,Var,Abs) 116
0xc0, // End Collection 118
the problem lies in the byte offset 100. If you use "Input
(Data,Arr,Abs)", the logical min/max should not be 0,1 but 0,8 and the
keyboard should translate the various values based on this offset.
Here, the keyboard is using a plain bitfield, and so it should be
using 'Input (Data,Var,Abs)', which translates to '0x81, 0x02'
(instead of '0x81, 0x00').
The best solution for you is to introduce a report descriptor fixup.
See hid-aureal.c in the kernel tree for an example of a simple report
descriptor fixup.
>
>>
>> >
>> > The keyboard functions mostly as expected now, with only a few
>> > minor
>> > issues:
>> >
>> > - two USB devices are detected instead of 1
>>
>> Well, that should be easy enough to solve
>
> I think so, but I could not find a way how to do that.
>
>>
>> > - some key combinations are not triggered - e.g.
>> > left shift + left ctrl + p. However, the same combination is
>> > recognized
>> > with the right shift key.
>>
>> Could you add this combination in the hid-recorder output too?
>
> This is what Left Ctrl + Shift + U produces:
>
> E: 24.575686 8 04 01 00 00 00 00 00 00
> E: 24.951689 8 04 03 00 00 00 00 00 00
> E: 26.719615 8 04 01 00 00 00 00 00 00
> E: 26.727611 8 04 00 00 00 00 00 00 00
>
> On the other hand, Right Ctrl + Shift + U produces:
>
> E: 55.911268 8 04 10 00 00 00 00 00 00
> E: 56.119280 8 04 30 00 00 00 00 00 00
> E: 56.599319 8 04 30 00 00 00 01 00 00
> E: 56.703282 8 04 30 00 00 00 00 00 00
> E: 56.815278 8 04 20 00 00 00 00 00 00
> E: 56.823255 8 04 00 00 00 00 00 00 00
>
> It looks suspicious that the keypress is not registered. Do I need to
> boot a kernel without my patch applied?
This seems like a firmware issue, and I doubt we will be able to fix
it. Does the keyboard under Windows behave sanely?
Cheers,
Benjamin
>
>> more comments inlined:
>>
>> >
>> > Changelog:
>> >
>> > - v2: modifier keys work, some combinations are still troublesome
>> > - v3: style cleanup, rebase on top of 4.14
>> > - v4: remove most debugging calls, make init info useful for user,
>> > rebased on top of 4.15
>> >
>> > Signed-off-by: Robert Munteanu <rombert@apache.org>
>> > ---
>> > drivers/hid/Kconfig | 9 +++
>> > drivers/hid/Makefile | 2 +-
>> > drivers/hid/hid-core.c | 3 +
>> > drivers/hid/hid-ids.h | 3 +
>> > drivers/hid/hid-microdia.c | 148
>> > +++++++++++++++++++++++++++++++++++++++++++++
>> > 5 files changed, 164 insertions(+), 1 deletion(-)
>> > create mode 100644 drivers/hid/hid-microdia.c
>> >
>> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> > index 779c5ae47f36..c3350f2ec4ea 100644
>> > --- a/drivers/hid/Kconfig
>> > +++ b/drivers/hid/Kconfig
>> > @@ -548,6 +548,15 @@ config HID_MAYFLASH
>> > Say Y here if you have HJZ Mayflash PS3 game controller
>> > adapters
>> > and want to enable force feedback support.
>> >
>> > +config HID_MICRODIA
>> > + tristate "Microdia based keyboards"
>> > + depends on HID
>> > + default !EXPERT
>> > + ---help---
>> > + Support for Microdia devices that are not compliant with the
>> > HID standard.
>> > +
>> > + One known example if the Redragon Asura Keyboard.
>> > +
>> > config HID_MICROSOFT
>> > tristate "Microsoft non-fully HID-compliant devices"
>> > depends on HID
>> > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>> > index 235bd2a7b333..e66a305876c5 100644
>> > --- a/drivers/hid/Makefile
>> > +++ b/drivers/hid/Makefile
>> > @@ -62,6 +62,7 @@ obj-$(CONFIG_HID_LOGITECH_DJ) += hid-logitech-
>> > dj.o
>> > obj-$(CONFIG_HID_LOGITECH_HIDPP) += hid-logitech-hidpp.o
>> > obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
>> > obj-$(CONFIG_HID_MAYFLASH) += hid-mf.o
>> > +obj-$(CONFIG_HID_MICRODIA) += hid-microdia.o
>> > obj-$(CONFIG_HID_MICROSOFT) += hid-microsoft.o
>> > obj-$(CONFIG_HID_MONTEREY) += hid-monterey.o
>> > obj-$(CONFIG_HID_MULTITOUCH) += hid-multitouch.o
>> > @@ -121,4 +122,3 @@ obj-$(CONFIG_USB_KBD) += usbhid/
>> >
>> > obj-$(CONFIG_I2C_HID) += i2c-hid/
>> >
>> > -obj-$(CONFIG_INTEL_ISH_HID) += intel-ish-hid/
>> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> > index 0c3f608131cf..b36c2df4b755 100644
>> > --- a/drivers/hid/hid-core.c
>> > +++ b/drivers/hid/hid-core.c
>> > @@ -2393,6 +2393,9 @@ static const struct hid_device_id
>> > hid_have_special_driver[] = {
>> > #endif
>> > #if IS_ENABLED(CONFIG_HID_ZYDACRON)
>> > { HID_USB_DEVICE(USB_VENDOR_ID_ZYDACRON,
>> > USB_DEVICE_ID_ZYDACRON_REMOTE_CONTROL) },
>> > +#endif
>> > +#if IS_ENABLED(CONFIG_HID_MICRODIA)
>> > + {
>> > HID_USB_DEVICE(USB_VENDOR_ID_MICRODIA, USB_DEVICE_ID_REDRAGON_ASUR
>> > A) },
>> > #endif
>>
>> You don't need this hunk anymore since v4.15 IIRC
>
> Ack, will update and retest.
>
>>
>> > { }
>> > };
>> > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>> > index 5da3d6256d25..146869e55c5a 100644
>> > --- a/drivers/hid/hid-ids.h
>> > +++ b/drivers/hid/hid-ids.h
>> > @@ -1171,4 +1171,7 @@
>> > #define USB_VENDOR_ID_UGTIZER 0x2179
>> > #define USB_DEVICE_ID_UGTIZER_TABLET_GP0610 0x0053
>> >
>> > +#define USB_VENDOR_ID_MICRODIA 0x0c45
>> > +#define USB_DEVICE_ID_REDRAGON_ASURA 0x760b
>> > +
>> > #endif
>> > diff --git a/drivers/hid/hid-microdia.c b/drivers/hid/hid-
>> > microdia.c
>> > new file mode 100644
>> > index 000000000000..f9d8de18a989
>> > --- /dev/null
>> > +++ b/drivers/hid/hid-microdia.c
>> > @@ -0,0 +1,148 @@
>>
>> missing the SPDX identifier
>> (see Documentation/process/license-rules.rst)
>
> Ack, will update.
>
>>
>> > +/*
>> > + * HID driver for Microdia-based keyboards
>> > + *
>> > + * Copyright (c) 2017 Robert Munteanu
>> > + */
>> > +
>> > +/*
>> > + * This program is free software; you can redistribute it and/or
>> > modify it
>> > + * under the terms of the GNU General Public License as published
>> > by the Free
>> > + * Software Foundation; either version 2 of the License, or (at
>> > your option)
>> > + * any later version.
>> > + */
>> > +
>> > +#include <linux/device.h>
>> > +#include <linux/input.h>
>> > +#include <linux/hid.h>
>> > +#include <linux/module.h>
>> > +#include <linux/log2.h>
>> > +#include <linux/input-event-codes.h>
>> > +
>> > +#include "hid-ids.h"
>> > +
>> > +
>> > +// The microdia family of keyboards uses a non-standard way of
>> > sending
>> > +// modifier keys
>>
>> Unless it has changed since last time I checked, we do not use C++
>> comments
>> (see Documentation/process/coding-style.rst)
>
> Ack.
>
>>
>> > +//
>> > +// The down event always sets the first bit to 0x04 and the second
>> > keycode
>> > +// to a custom value. For instance, left shift sends the following
>> > bits
>> > +//
>> > +// 04 02 00 00 00 00 00 00
>> > +//
>> > +// while left control sends
>> > +//
>> > +// 04 01 00 00 00 00 00 00
>> > +//
>> > +// Unfortunately these are all mapped to left shift and the
>> > keyboard is
>> > +// non-functional in that respect. To solve the problem, we
>> > capture the
>> > +// raw data in raw_event and manually generate the correct input
>> > events
>> > +//
>> > +// TODO
>> > +//
>> > +// 1. Some modifiers keys still don't work, e.g. Ctrl-Shift-P
>> > +// Interestingly enough, this happens when pressing the
>> > physical
>> > +// left shift key, but now when pressing the physical right
>> > shift key.
>> > +// hexdump does not show them either.
>> > +// 2. A second USB keyboard is registered, but it should be
>> > removed
>> > +// 3. Modifier keys sometimes get stuck, need to re-press to clear
>> > +
>> > +static int microdia_input_configured(struct hid_device *hdev,
>> > + struct hid_input *hi)
>> > +{
>> > +
>> > + hid_info(hdev, "Keyboard configured with limited support
>> > (modifier keys may get stuck, some modifiers combinations with
>> > left-shift not working) ");
>> > +
>> > + hid_set_drvdata(hdev, hi);
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static int microdia_input_mapping(struct hid_device *hdev, struct
>> > hid_input *hi,
>> > + struct hid_field *field, struct hid_usage *usage,
>> > + unsigned long **bit, int *max)
>> > +{
>> > + // do not map left shift since we will manually generate
>> > input
>> > + // events in microdia_raw_event
>> > + if ((usage->hid & HID_USAGE_PAGE) == HID_UP_KEYBOARD)
>> > + if ((usage->hid & HID_USAGE) == 0xe1)
>> > + return -1;
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +// array index of a modifier matches the bit index in the data
>> > payload
>> > +// the modifier data is always stored in the second int
>> > +// e.g. for left alt the 1 bit is set - 04 04 ...
>> > +// TODO - second entry should be left shift, but that's not
>> > possible
>> > +// since we ignore it in the mapping
>> > +static int microdia_modifiers[7] = {
>> > + KEY_LEFTCTRL,
>> > + KEY_RIGHTSHIFT,
>> > + KEY_LEFTALT,
>> > + KEY_LEFTMETA,
>> > + KEY_RIGHTCTRL,
>> > + KEY_RIGHTSHIFT,
>> > + KEY_RIGHTALT
>> > +};
>> > +
>> > +static int microdia_last_handled_modifier;
>> > +
>> > +static int microdia_raw_event(struct hid_device *hdev,
>> > + struct hid_report *report, u8 *data, int size)
>> > +{
>> > + int i, changed_key, new_value, mapped_key;
>> > + struct hid_input *hi;
>> > +
>> > + // 1. validate that we get 8 bits of the for 04 xx 00 00 00
>> > 00 00 00
>> > + if (size != 8)
>> > + return 0;
>> > +
>> > + if (data[0] != 4)
>> > + return 0;
>> > +
>> > + // TODO - can I somehow use a bit mask? data & 0x00ffffff
>> > != 0
>> > + for (i = 2; i < size; i++)
>> > + if (data[i] != 0)
>> > + return 0;
>> > +
>> > + // TODO - don't process the keyup event 04 00 00 00 00 00
>> > 00 00
>>
>> That's a lot of TODO in such a simple driver :/
>
> Yes, well that's what I get for learning to write hid drivers by
> example ... and C isn't my primary programming language either.
>
>>
>> > + // if we don't expect a modifier key 'up' event
>> > +
>> > + // 2. detect based on previous data what key was pressed or
>> > depressed
>> > + // a key was pressed or depressed if its bit has a
>> > different value
>> > + // between this and the previous invocation. If the bit is
>> > set
>> > + // it was pressed
>> > +
>> > + changed_key = data[1] ^ microdia_last_handled_modifier;
>> > + new_value = (data[1] & changed_key) != 0;
>> > + // TODO - is ilog2 really needed?
>> > + mapped_key = microdia_modifiers[ilog2(changed_key)];
>>
>> What if you get 2 changed keys at a time?
>
> This could explain the problems that I've been getting with modifier
> keys being 'stuck'.
>
>>
>> Anyway, before giving you a ACK or NACK on the driver, the behavior
>> from the keyboard firmware looks sane and pretty common. So my guess
>> is that there is something wrong in the report descriptors, and I
>> need
>> the hid-recorder output to have a better idea of what is wrong with
>> our current handling of this keyboard.
>
> The output is inline. That being said, I'm aware that re-firing the
> keypress events myself does not belong in a HID driver, but I'm eager
> to work on this being massaged into something proper for inclusion.
>
> Thanks for taking the time to review.
>
> Robert
>
>>
>> Cheers,
>> Benjamin
>>
>> > +
>> > + // 3. report the key event and track what was sent
>> > + hi = hid_get_drvdata(hdev);
>> > +
>> > + input_report_key(hi->input, mapped_key, new_value);
>> > +
>> > + microdia_last_handled_modifier = data[1];
>> > +
>> > + return 1;
>> > +}
>> > +
>> > +static const struct hid_device_id microdia_devices[] = {
>> > + {HID_USB_DEVICE(USB_VENDOR_ID_MICRODIA,
>> > USB_DEVICE_ID_REDRAGON_ASURA)},
>> > + {}
>> > +};
>> > +
>> > +MODULE_DEVICE_TABLE(hid, microdia_devices);
>> > +
>> > +static struct hid_driver microdia_driver = {
>> > + .name = "microdia",
>> > + .input_mapping = microdia_input_mapping,
>> > + .id_table = microdia_devices,
>> > + .raw_event = microdia_raw_event,
>> > + .input_configured = microdia_input_configured,
>> > +};
>> > +
>> > +module_hid_driver(microdia_driver);
>> > +
>> > +MODULE_LICENSE("GPL");
>> > --
>> > 2.16.2
>> >
>
^ permalink raw reply
* Re: [PATCH v3] Fix modifier keys for Redragon Asura Keyboard
From: Robert Munteanu @ 2018-03-22 13:39 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, lkml, open list:HID CORE LAYER
In-Reply-To: <CAO-hwJL3RJMOaTT9=Vv3_3+wVSVyTD+A4Qk34+uQzzkcEH=btg@mail.gmail.com>
On Thu, 2018-03-22 at 09:33 +0100, Benjamin Tissoires wrote:
> On Wed, Mar 21, 2018 at 5:33 PM, Robert Munteanu <rombert@apache.org>
> wrote:
> > Hi Benjamin,
> >
> > On Wed, 2018-03-21 at 17:12 +0100, Benjamin Tissoires wrote:
> > > Hi Robert,
> > >
> > > First, apologies for not answering to the RFC. I missed it and it
> > > fell
> > > down in my INBOX.
> > >
> > > On Wed, Mar 21, 2018 at 11:43 AM, Robert Munteanu <rombert@apache
> > > .org
> > > > wrote:
> > > > The microdia family of keyboards uses a non-standard way of
> > > > sending
> > > > modifier keys.
> > > >
> > > > The down event always sets the first bit to 0x04 and the second
> > > > keycode
> > >
> > > Pretty sure you are talking about bytes here, not bits.
> > > And in the HID world, the first byte is usually a report ID.
> >
> > Right, thanks for spotting that.
> >
> > >
> > > > to a custom value For instance, left shift sends the following
> > > > bits
> > > >
> > > > 04 02 00 00 00 00 00 00
> > > >
> > > > while left control sends
> > > >
> > > > 04 01 00 00 00 00 00 00
> > >
> > > This sounds like bit(0) is mapped to LEFT_SHIFT and bit(1) mapped
> > > to
> > > LEFT_CONTROL in the second byte.
> > > Fortunately, LeftControl is designed in HID as 0xe0 in the
> > > keyboard
> > > HID usage table, and LeftShift is 0xe1. So that would match the
> > > behavior you are seeing.
> > > If you have 04 04 00 00 00 00 00 00 when pressing LeftAlt, then
> > > definitively you are just facing a bitmask, which is fairly
> > > common.
> > >
> > > >
> > > > As a result all modifier keys are mapped to left shift and the
> > > > keyboard is
> > > > non-functional in that respect. To solve the problem, we
> > > > capture
> > > > the
> > > > raw data in raw_event and manually generate the correct input
> > > > events.
> > >
> > > I'd like to see the hid-recorder events from these keypresses.
> > > The
> > > device might be buggy, but I have a gut feeling your solution is
> > > not
> > > the simplest one.
> > > Please grab hid-recorder from http://bentiss.github.io/hid-replay
> > > -
> > > docs/
> >
> > See below, I recorded the output for pressing left ctrl, left alt,
> > left
> > shift, right ctrl, right alt, right shift, meta.
> >
> > D: 0
> > R: 169 05 0c 09 01 a1 01 85 01 19 00 2a 80 03 15 00 26 80 03 95 01
> > 75
> > 10 81 00 c0 05 01 09 80 a1 01 85 02 19 81 29 83 15 00 25 01 75 01
> > 95 03
> > 81 02 95 05 81 01 c0 06 00 ff 09 01 a1 01 85 03 1a f1 00 2a f8 00
> > 15 00
> > 25 01 75 01 95 08 81 02 c0 05 01 09 06 a1 01 85 04 05 07 19 e0 29
> > e7 15
> > 00 25 01 75 01 95 08 81 00 95 30 75 01 15 00 25 01 05 07 19 00 29
> > 2f 81
> > 02 c0 05 01 09 06 a1 01 85 05 95 38 75 01 15 00 25 01 05 07 19 30
> > 29 67
> > 81 02 c0 05 01 09 06 a1 01 85 06 95 38 75 01 15 00 25 01 05 07 19
> > 68 29
> > 9f 81 02 c0
> > N: USB Keyboard
> > P: usb-0000:00:14.0-6/input1
> > I: 3 0c45 760b
> > D: 0
> > E: 0.000000 8 04 01 00 00 00 00 00 00
> > E: 0.039920 8 04 00 00 00 00 00 00 00
> > E: 0.751952 8 04 04 00 00 00 00 00 00
> > E: 0.823977 8 04 00 00 00 00 00 00 00
> > E: 2.639887 8 04 02 00 00 00 00 00 00
> > E: 2.711896 8 04 00 00 00 00 00 00 00
> > E: 3.583932 8 04 10 00 00 00 00 00 00
> > E: 3.663919 8 04 00 00 00 00 00 00 00
> > E: 4.871886 8 04 40 00 00 00 00 00 00
> > E: 4.935906 8 04 00 00 00 00 00 00 00
> > E: 6.503872 8 04 20 00 00 00 00 00 00
> > E: 6.575850 8 04 00 00 00 00 00 00 00
> > E: 7.383844 8 04 08 00 00 00 00 00 00
> > E: 7.455861 8 04 00 00 00 00 00 00 00
>
> Thanks. So it's indeed an issue with the report descriptor. Not sure
> how this can also work under Windows.
> FYI, the Report ID 4 translates to:
>
> 0x05, 0x01, // Usage Page (Generic
> Desktop) 78
> 0x09, 0x06, // Usage
> (Keyboard) 80
> 0xa1, 0x01, // Collection
> (Application) 82
> 0x85, 0x04, // Report ID
> (4) 84
> 0x05, 0x07, // Usage Page
> (Keyboard) 86
> 0x19, 0xe0, // Usage Minimum
> (224) 88
> 0x29, 0xe7, // Usage Maximum
> (231) 90
> 0x15, 0x00, // Logical Minimum
> (0) 92
> 0x25, 0x01, // Logical Maximum
> (1) 94
> 0x75, 0x01, // Report Size
> (1) 96
> 0x95, 0x08, // Report Count
> (8) 98
> 0x81, 0x00, // Input
> (Data,Arr,Abs) 100
> 0x95, 0x30, // Report Count
> (48) 102
> 0x75, 0x01, // Report Size
> (1) 104
> 0x15, 0x00, // Logical Minimum
> (0) 106
> 0x25, 0x01, // Logical Maximum
> (1) 108
> 0x05, 0x07, // Usage Page
> (Keyboard) 110
> 0x19, 0x00, // Usage Minimum
> (0) 112
> 0x29, 0x2f, // Usage Maximum
> (47) 114
> 0x81, 0x02, // Input
> (Data,Var,Abs) 116
> 0xc0, // End
> Collection 118
>
> the problem lies in the byte offset 100. If you use "Input
> (Data,Arr,Abs)", the logical min/max should not be 0,1 but 0,8 and
> the
> keyboard should translate the various values based on this offset.
> Here, the keyboard is using a plain bitfield, and so it should be
> using 'Input (Data,Var,Abs)', which translates to '0x81, 0x02'
> (instead of '0x81, 0x00').
>
> The best solution for you is to introduce a report descriptor fixup.
> See hid-aureal.c in the kernel tree for an example of a simple report
> descriptor fixup.
Yes, that works! I applied a descriptor fixup and that is the only
change actually neded - I dropped all my other code.
I had a sense that the code was not the right approach and that the
problem was likely in the HID descriptor or HID parsing, but I was
unable to advance. Thanks for the extensive description.
>
> >
> > >
> > > >
> > > > The keyboard functions mostly as expected now, with only a few
> > > > minor
> > > > issues:
> > > >
> > > > - two USB devices are detected instead of 1
> > >
> > > Well, that should be easy enough to solve
> >
> > I think so, but I could not find a way how to do that.
> >
> > >
> > > > - some key combinations are not triggered - e.g.
> > > > left shift + left ctrl + p. However, the same combination is
> > > > recognized
> > > > with the right shift key.
> > >
> > > Could you add this combination in the hid-recorder output too?
> >
> > This is what Left Ctrl + Shift + U produces:
> >
> > E: 24.575686 8 04 01 00 00 00 00 00 00
> > E: 24.951689 8 04 03 00 00 00 00 00 00
> > E: 26.719615 8 04 01 00 00 00 00 00 00
> > E: 26.727611 8 04 00 00 00 00 00 00 00
> >
> > On the other hand, Right Ctrl + Shift + U produces:
> >
> > E: 55.911268 8 04 10 00 00 00 00 00 00
> > E: 56.119280 8 04 30 00 00 00 00 00 00
> > E: 56.599319 8 04 30 00 00 00 01 00 00
> > E: 56.703282 8 04 30 00 00 00 00 00 00
> > E: 56.815278 8 04 20 00 00 00 00 00 00
> > E: 56.823255 8 04 00 00 00 00 00 00 00
> >
> > It looks suspicious that the keypress is not registered. Do I need
> > to
> > boot a kernel without my patch applied?
>
> This seems like a firmware issue, and I doubt we will be able to fix
> it. Does the keyboard under Windows behave sanely?
I will need to double-check next week, I don't have a Windows machine
at hand right now.
Thanks,
Robert
>
> Cheers,
> Benjamin
>
> >
> > > more comments inlined:
> > >
> > > >
> > > > Changelog:
> > > >
> > > > - v2: modifier keys work, some combinations are still
> > > > troublesome
> > > > - v3: style cleanup, rebase on top of 4.14
> > > > - v4: remove most debugging calls, make init info useful for
> > > > user,
> > > > rebased on top of 4.15
> > > >
> > > > Signed-off-by: Robert Munteanu <rombert@apache.org>
> > > > ---
> > > > drivers/hid/Kconfig | 9 +++
> > > > drivers/hid/Makefile | 2 +-
> > > > drivers/hid/hid-core.c | 3 +
> > > > drivers/hid/hid-ids.h | 3 +
> > > > drivers/hid/hid-microdia.c | 148
> > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > > 5 files changed, 164 insertions(+), 1 deletion(-)
> > > > create mode 100644 drivers/hid/hid-microdia.c
> > > >
> > > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > > > index 779c5ae47f36..c3350f2ec4ea 100644
> > > > --- a/drivers/hid/Kconfig
> > > > +++ b/drivers/hid/Kconfig
> > > > @@ -548,6 +548,15 @@ config HID_MAYFLASH
> > > > Say Y here if you have HJZ Mayflash PS3 game controller
> > > > adapters
> > > > and want to enable force feedback support.
> > > >
> > > > +config HID_MICRODIA
> > > > + tristate "Microdia based keyboards"
> > > > + depends on HID
> > > > + default !EXPERT
> > > > + ---help---
> > > > + Support for Microdia devices that are not compliant with
> > > > the
> > > > HID standard.
> > > > +
> > > > + One known example if the Redragon Asura Keyboard.
> > > > +
> > > > config HID_MICROSOFT
> > > > tristate "Microsoft non-fully HID-compliant devices"
> > > > depends on HID
> > > > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > > > index 235bd2a7b333..e66a305876c5 100644
> > > > --- a/drivers/hid/Makefile
> > > > +++ b/drivers/hid/Makefile
> > > > @@ -62,6 +62,7 @@ obj-$(CONFIG_HID_LOGITECH_DJ) += hid-
> > > > logitech-
> > > > dj.o
> > > > obj-$(CONFIG_HID_LOGITECH_HIDPP) += hid-logitech-hidpp.o
> > > > obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
> > > > obj-$(CONFIG_HID_MAYFLASH) += hid-mf.o
> > > > +obj-$(CONFIG_HID_MICRODIA) += hid-microdia.o
> > > > obj-$(CONFIG_HID_MICROSOFT) += hid-microsoft.o
> > > > obj-$(CONFIG_HID_MONTEREY) += hid-monterey.o
> > > > obj-$(CONFIG_HID_MULTITOUCH) += hid-multitouch.o
> > > > @@ -121,4 +122,3 @@ obj-$(CONFIG_USB_KBD) +=
> > > > usbhid/
> > > >
> > > > obj-$(CONFIG_I2C_HID) += i2c-hid/
> > > >
> > > > -obj-$(CONFIG_INTEL_ISH_HID) += intel-ish-hid/
> > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > > index 0c3f608131cf..b36c2df4b755 100644
> > > > --- a/drivers/hid/hid-core.c
> > > > +++ b/drivers/hid/hid-core.c
> > > > @@ -2393,6 +2393,9 @@ static const struct hid_device_id
> > > > hid_have_special_driver[] = {
> > > > #endif
> > > > #if IS_ENABLED(CONFIG_HID_ZYDACRON)
> > > > { HID_USB_DEVICE(USB_VENDOR_ID_ZYDACRON,
> > > > USB_DEVICE_ID_ZYDACRON_REMOTE_CONTROL) },
> > > > +#endif
> > > > +#if IS_ENABLED(CONFIG_HID_MICRODIA)
> > > > + {
> > > > HID_USB_DEVICE(USB_VENDOR_ID_MICRODIA, USB_DEVICE_ID_REDRAGON_
> > > > ASUR
> > > > A) },
> > > > #endif
> > >
> > > You don't need this hunk anymore since v4.15 IIRC
> >
> > Ack, will update and retest.
> >
> > >
> > > > { }
> > > > };
> > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > > > index 5da3d6256d25..146869e55c5a 100644
> > > > --- a/drivers/hid/hid-ids.h
> > > > +++ b/drivers/hid/hid-ids.h
> > > > @@ -1171,4 +1171,7 @@
> > > > #define USB_VENDOR_ID_UGTIZER 0x2179
> > > > #define USB_DEVICE_ID_UGTIZER_TABLET_GP0610 0x0053
> > > >
> > > > +#define USB_VENDOR_ID_MICRODIA 0x0c45
> > > > +#define USB_DEVICE_ID_REDRAGON_ASURA 0x760b
> > > > +
> > > > #endif
> > > > diff --git a/drivers/hid/hid-microdia.c b/drivers/hid/hid-
> > > > microdia.c
> > > > new file mode 100644
> > > > index 000000000000..f9d8de18a989
> > > > --- /dev/null
> > > > +++ b/drivers/hid/hid-microdia.c
> > > > @@ -0,0 +1,148 @@
> > >
> > > missing the SPDX identifier
> > > (see Documentation/process/license-rules.rst)
> >
> > Ack, will update.
> >
> > >
> > > > +/*
> > > > + * HID driver for Microdia-based keyboards
> > > > + *
> > > > + * Copyright (c) 2017 Robert Munteanu
> > > > + */
> > > > +
> > > > +/*
> > > > + * This program is free software; you can redistribute it
> > > > and/or
> > > > modify it
> > > > + * under the terms of the GNU General Public License as
> > > > published
> > > > by the Free
> > > > + * Software Foundation; either version 2 of the License, or
> > > > (at
> > > > your option)
> > > > + * any later version.
> > > > + */
> > > > +
> > > > +#include <linux/device.h>
> > > > +#include <linux/input.h>
> > > > +#include <linux/hid.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/log2.h>
> > > > +#include <linux/input-event-codes.h>
> > > > +
> > > > +#include "hid-ids.h"
> > > > +
> > > > +
> > > > +// The microdia family of keyboards uses a non-standard way of
> > > > sending
> > > > +// modifier keys
> > >
> > > Unless it has changed since last time I checked, we do not use
> > > C++
> > > comments
> > > (see Documentation/process/coding-style.rst)
> >
> > Ack.
> >
> > >
> > > > +//
> > > > +// The down event always sets the first bit to 0x04 and the
> > > > second
> > > > keycode
> > > > +// to a custom value. For instance, left shift sends the
> > > > following
> > > > bits
> > > > +//
> > > > +// 04 02 00 00 00 00 00 00
> > > > +//
> > > > +// while left control sends
> > > > +//
> > > > +// 04 01 00 00 00 00 00 00
> > > > +//
> > > > +// Unfortunately these are all mapped to left shift and the
> > > > keyboard is
> > > > +// non-functional in that respect. To solve the problem, we
> > > > capture the
> > > > +// raw data in raw_event and manually generate the correct
> > > > input
> > > > events
> > > > +//
> > > > +// TODO
> > > > +//
> > > > +// 1. Some modifiers keys still don't work, e.g. Ctrl-Shift-P
> > > > +// Interestingly enough, this happens when pressing the
> > > > physical
> > > > +// left shift key, but now when pressing the physical right
> > > > shift key.
> > > > +// hexdump does not show them either.
> > > > +// 2. A second USB keyboard is registered, but it should be
> > > > removed
> > > > +// 3. Modifier keys sometimes get stuck, need to re-press to
> > > > clear
> > > > +
> > > > +static int microdia_input_configured(struct hid_device *hdev,
> > > > + struct hid_input *hi)
> > > > +{
> > > > +
> > > > + hid_info(hdev, "Keyboard configured with limited
> > > > support
> > > > (modifier keys may get stuck, some modifiers combinations with
> > > > left-shift not working) ");
> > > > +
> > > > + hid_set_drvdata(hdev, hi);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int microdia_input_mapping(struct hid_device *hdev,
> > > > struct
> > > > hid_input *hi,
> > > > + struct hid_field *field, struct hid_usage
> > > > *usage,
> > > > + unsigned long **bit, int *max)
> > > > +{
> > > > + // do not map left shift since we will manually
> > > > generate
> > > > input
> > > > + // events in microdia_raw_event
> > > > + if ((usage->hid & HID_USAGE_PAGE) == HID_UP_KEYBOARD)
> > > > + if ((usage->hid & HID_USAGE) == 0xe1)
> > > > + return -1;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +// array index of a modifier matches the bit index in the data
> > > > payload
> > > > +// the modifier data is always stored in the second int
> > > > +// e.g. for left alt the 1 bit is set - 04 04 ...
> > > > +// TODO - second entry should be left shift, but that's not
> > > > possible
> > > > +// since we ignore it in the mapping
> > > > +static int microdia_modifiers[7] = {
> > > > + KEY_LEFTCTRL,
> > > > + KEY_RIGHTSHIFT,
> > > > + KEY_LEFTALT,
> > > > + KEY_LEFTMETA,
> > > > + KEY_RIGHTCTRL,
> > > > + KEY_RIGHTSHIFT,
> > > > + KEY_RIGHTALT
> > > > +};
> > > > +
> > > > +static int microdia_last_handled_modifier;
> > > > +
> > > > +static int microdia_raw_event(struct hid_device *hdev,
> > > > + struct hid_report *report, u8 *data, int size)
> > > > +{
> > > > + int i, changed_key, new_value, mapped_key;
> > > > + struct hid_input *hi;
> > > > +
> > > > + // 1. validate that we get 8 bits of the for 04 xx 00
> > > > 00 00
> > > > 00 00 00
> > > > + if (size != 8)
> > > > + return 0;
> > > > +
> > > > + if (data[0] != 4)
> > > > + return 0;
> > > > +
> > > > + // TODO - can I somehow use a bit mask? data &
> > > > 0x00ffffff
> > > > != 0
> > > > + for (i = 2; i < size; i++)
> > > > + if (data[i] != 0)
> > > > + return 0;
> > > > +
> > > > + // TODO - don't process the keyup event 04 00 00 00 00
> > > > 00
> > > > 00 00
> > >
> > > That's a lot of TODO in such a simple driver :/
> >
> > Yes, well that's what I get for learning to write hid drivers by
> > example ... and C isn't my primary programming language either.
> >
> > >
> > > > + // if we don't expect a modifier key 'up' event
> > > > +
> > > > + // 2. detect based on previous data what key was
> > > > pressed or
> > > > depressed
> > > > + // a key was pressed or depressed if its bit has a
> > > > different value
> > > > + // between this and the previous invocation. If the bit
> > > > is
> > > > set
> > > > + // it was pressed
> > > > +
> > > > + changed_key = data[1] ^ microdia_last_handled_modifier;
> > > > + new_value = (data[1] & changed_key) != 0;
> > > > + // TODO - is ilog2 really needed?
> > > > + mapped_key = microdia_modifiers[ilog2(changed_key)];
> > >
> > > What if you get 2 changed keys at a time?
> >
> > This could explain the problems that I've been getting with
> > modifier
> > keys being 'stuck'.
> >
> > >
> > > Anyway, before giving you a ACK or NACK on the driver, the
> > > behavior
> > > from the keyboard firmware looks sane and pretty common. So my
> > > guess
> > > is that there is something wrong in the report descriptors, and I
> > > need
> > > the hid-recorder output to have a better idea of what is wrong
> > > with
> > > our current handling of this keyboard.
> >
> > The output is inline. That being said, I'm aware that re-firing the
> > keypress events myself does not belong in a HID driver, but I'm
> > eager
> > to work on this being massaged into something proper for inclusion.
> >
> > Thanks for taking the time to review.
> >
> > Robert
> >
> > >
> > > Cheers,
> > > Benjamin
> > >
> > > > +
> > > > + // 3. report the key event and track what was sent
> > > > + hi = hid_get_drvdata(hdev);
> > > > +
> > > > + input_report_key(hi->input, mapped_key, new_value);
> > > > +
> > > > + microdia_last_handled_modifier = data[1];
> > > > +
> > > > + return 1;
> > > > +}
> > > > +
> > > > +static const struct hid_device_id microdia_devices[] = {
> > > > + {HID_USB_DEVICE(USB_VENDOR_ID_MICRODIA,
> > > > USB_DEVICE_ID_REDRAGON_ASURA)},
> > > > + {}
> > > > +};
> > > > +
> > > > +MODULE_DEVICE_TABLE(hid, microdia_devices);
> > > > +
> > > > +static struct hid_driver microdia_driver = {
> > > > + .name = "microdia",
> > > > + .input_mapping = microdia_input_mapping,
> > > > + .id_table = microdia_devices,
> > > > + .raw_event = microdia_raw_event,
> > > > + .input_configured = microdia_input_configured,
> > > > +};
> > > > +
> > > > +module_hid_driver(microdia_driver);
> > > > +
> > > > +MODULE_LICENSE("GPL");
> > > > --
> > > > 2.16.2
> > > >
^ permalink raw reply
* Re: [PATCH v3] Fix modifier keys for Redragon Asura Keyboard
From: Robert Munteanu @ 2018-03-22 13:40 UTC (permalink / raw)
To: Ondrej Zary; +Cc: Jiri Kosina, Benjamin Tissoires, linux-kernel, linux-input
In-Reply-To: <1b0047c60fc5dc4be4bbe627ab830e65db1cfae5.camel@apache.org>
On Thu, 2018-03-22 at 09:17 +0100, Robert Munteanu wrote:
> On Wed, 2018-03-21 at 17:51 +0100, Ondrej Zary wrote:
> > On Wednesday 21 March 2018 11:43:31 Robert Munteanu wrote:
> > > +#define USB_VENDOR_ID_MICRODIA 0x0c45
> > > +#define USB_DEVICE_ID_REDRAGON_ASURA 0x760b
> >
> > Microdia is most probably an incorrect name. The 0c45 id probably
> > belongs
> > to "Sonix Technology Co., Ltd."
>
> Right, I was unsure about that. I'll remove the microdia references
> and
> use sonix.
I see the following in hid-ids.h
define USB_VENDOR_ID_JESS 0x0c45
which I guess I should use since it's already defined in there.
Robert
^ permalink raw reply
* Re: [PATCH v5 0/2] Remove false-positive VLAs when using max()
From: Kees Cook @ 2018-03-22 15:01 UTC (permalink / raw)
To: Linus Torvalds
Cc: Al Viro, Florian Weimer, Andrew Morton, Josh Poimboeuf,
Rasmus Villemoes, Randy Dunlap, Miguel Ojeda, Ingo Molnar,
David Laight, Ian Abbott, linux-input, linux-btrfs,
Network Development, Linux Kernel Mailing List, Kernel Hardening
In-Reply-To: <CA+55aFwxk=tUECYQkd4cog08qW4ZT=r2K7FQXzGnc-zuMc7JQA@mail.gmail.com>
On Tue, Mar 20, 2018 at 4:23 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Sat, Mar 17, 2018 at 1:07 PM, Kees Cook <keescook@chromium.org> wrote:
>>
>> No luck! :( gcc 4.4 refuses to play along. And, hilariously, not only
>> does it not change the complaint about __builtin_choose_expr(), it
>> also thinks that's a VLA now.
>
> Hmm. So thanks to the diseased mind of Martin Uecker, there's a better
> test for "__is_constant()":
>
> /* Glory to Martin Uecker <Martin.Uecker@med.uni-goettingen.de> */
> #define __is_constant(a) \
> (sizeof(int) == sizeof(*(1 ? ((void*)((a) * 0l)) : (int*)1)))
>
> that is actually *specified* by the C standard to work, and doesn't
> even depend on any gcc extensions.
I feel we risk awakening Cthulhu with this. :)
> The reason is some really subtle pointer conversion rules, where the
> type of the ternary operator will depend on whether one of the
> pointers is NULL or not.
>
> And the definition of NULL, in turn, very much depends on "integer
> constant expression that has the value 0".
>
> Are you willing to do one final try on a generic min/max? Same as my
> last patch, but using the above __is_constant() test instead of
> __builtin_constant_p?
So, this time it's not a catastrophic failure with gcc 4.4. Instead it
fails in 11 distinct places:
$ grep "first argument to ‘__builtin_choose_expr’ not a constant" log
| cut -d: -f1-2
crypto/ablkcipher.c:71
crypto/blkcipher.c:70
crypto/skcipher.c:95
mm/percpu.c:2453
net/ceph/osdmap.c:1545
net/ceph/osdmap.c:1756
net/ceph/osdmap.c:1763
mm/kmemleak.c:1371
mm/kmemleak.c:1403
drivers/infiniband/hw/hfi1/pio_copy.c:421
drivers/infiniband/hw/hfi1/pio_copy.c:547
Seems like it doesn't like void * arguments:
mm/percpu.c:
void *ptr;
...
base = min(ptr, base);
mm/kmemleak.c:
static void scan_large_block(void *start, void *end)
...
next = min(start + MAX_SCAN_SIZE, end);
I'll poke a bit more...
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox