* [PATCH] evdev: flush ABS_* events during EVIOCGABS
From: David Herrmann @ 2014-04-10 19:09 UTC (permalink / raw)
To: linux-input
Cc: Peter Hutterer, Dmitry Torokhov, Benjamin Tissoires,
David Herrmann
We currently flush input events in the outgoing client queue on most
EVIOCG* ioctls so userspace doesn't get events twice (once during the
ioctl and once during a later read()).
We introduced this in:
commit 483180281f0ac60d1138710eb21f4b9961901294
Author: David Herrmann <dh.herrmann@gmail.com>
Date: Sun Apr 7 21:13:19 2013 -0700
Input: evdev - flush queues during EVIOCGKEY-like ioctls
However, we didn't modify the EVIOCGABS handler as we considered ABS
values to change fast enough that flushing the queue seems irrelevant. But
as it turns out, the ABS SLOT events suffer from the same issues. Hence,
also flush the input queue from ABS values on EVIOCGABS.
Reported-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
Hi
This is untested as I don't have any multitouch hardware right now. Peter, can
you give this a try?
Thanks
David
drivers/input/evdev.c | 63 ++++++++++++++++++++++++++++++++++++---------------
1 file changed, 45 insertions(+), 18 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index a06e125..fc55c19 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -55,8 +55,11 @@ struct evdev_client {
struct input_event buffer[];
};
-/* flush queued events of type @type, caller must hold client->buffer_lock */
-static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
+/* Flush queued events of given type @type and code @code. A negative code
+ * is interpreted as catch-all. Caller must hold client->buffer_lock. */
+static void __evdev_flush_queue(struct evdev_client *client,
+ unsigned int type,
+ int code)
{
unsigned int i, head, num;
unsigned int mask = client->bufsize - 1;
@@ -75,7 +78,7 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
ev = &client->buffer[i];
is_report = ev->type == EV_SYN && ev->code == SYN_REPORT;
- if (ev->type == type) {
+ if (ev->type == type && (code < 0 || ev->code == code)) {
/* drop matched entry */
continue;
} else if (is_report && !num) {
@@ -774,7 +777,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
spin_unlock(&dev->event_lock);
- __evdev_flush_queue(client, type);
+ __evdev_flush_queue(client, type, -1);
spin_unlock_irq(&client->buffer_lock);
@@ -787,6 +790,40 @@ static int evdev_handle_get_val(struct evdev_client *client,
return ret;
}
+static int evdev_handle_get_abs(struct evdev_client *client,
+ struct input_dev *dev,
+ unsigned int code,
+ unsigned int size,
+ void __user *p)
+{
+ struct input_absinfo abs;
+ int ret;
+
+ if (!dev->absinfo)
+ return -EINVAL;
+
+ /* see evdev_handle_get_val() for locking order */
+
+ spin_lock_irq(&dev->event_lock);
+ spin_lock(&client->buffer_lock);
+
+ abs = dev->absinfo[code];
+
+ spin_unlock(&dev->event_lock);
+
+ __evdev_flush_queue(client, EV_ABS, code);
+
+ spin_unlock_irq(&client->buffer_lock);
+
+ ret = copy_to_user(p, &abs, min_t(size_t,
+ size,
+ sizeof(struct input_absinfo)));
+ if (ret < 0)
+ evdev_queue_syn_dropped(client);
+
+ return ret;
+}
+
static int evdev_handle_mt_request(struct input_dev *dev,
unsigned int size,
int __user *ip)
@@ -972,20 +1009,10 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
_IOC_NR(cmd) & EV_MAX, size,
p, compat_mode);
- if ((_IOC_NR(cmd) & ~ABS_MAX) == _IOC_NR(EVIOCGABS(0))) {
-
- if (!dev->absinfo)
- return -EINVAL;
-
- t = _IOC_NR(cmd) & ABS_MAX;
- abs = dev->absinfo[t];
-
- if (copy_to_user(p, &abs, min_t(size_t,
- size, sizeof(struct input_absinfo))))
- return -EFAULT;
-
- return 0;
- }
+ if ((_IOC_NR(cmd) & ~ABS_MAX) == _IOC_NR(EVIOCGABS(0)))
+ return evdev_handle_get_abs(client, dev,
+ _IOC_NR(cmd) & ABS_MAX,
+ size, p);
}
if (_IOC_DIR(cmd) == _IOC_WRITE) {
--
1.9.1
^ permalink raw reply related
* Re: [patch]GPIO button is supposed to wake the system up if the wakeup attribute is set
From: One Thousand Gnomes @ 2014-04-10 10:48 UTC (permalink / raw)
To: Li, Aubrey; +Cc: dmitry.torokhov, sachin.kamat, linux-input
In-Reply-To: <5345FDBD.9090908@linux.intel.com>
On Thu, 10 Apr 2014 10:11:09 +0800
"Li, Aubrey" <aubrey.li@linux.intel.com> wrote:
> When the wakeup attribute is set, GPIO button is supposed to set
> irqflag - IRQF_NO_SUSPEND to request irq. So when the system enters
> the suspend sleep mode, the GPIO irq keeps enabled and is able to
> wake the system up.
>
> The affected/tested machines include Dell Venue 11 Pro and Asus T100TA.
>
> Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
Tested-by: Alan Cox <alan@linux.intel.com>
^ permalink raw reply
* [PATCH] bma150: extend chip detection for bma180
From: Dr. H. Nikolaus Schaller @ 2014-04-10 9:30 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Jingoo Han, Fugang Duan, linux-input, LKML, Marek Belisko
This driver has been used while on the OpenPhoenux GTA04 with
a BMA180.
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
drivers/input/misc/bma150.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/input/misc/bma150.c b/drivers/input/misc/bma150.c
index 52d3a9b..b36831c 100644
--- a/drivers/input/misc/bma150.c
+++ b/drivers/input/misc/bma150.c
@@ -70,6 +70,7 @@
#define BMA150_CFG_5_REG 0x11
#define BMA150_CHIP_ID 2
+#define BMA180_CHIP_ID 3
#define BMA150_CHIP_ID_REG BMA150_DATA_0_REG
#define BMA150_ACC_X_LSB_REG BMA150_DATA_2_REG
@@ -539,7 +540,7 @@ static int bma150_probe(struct i2c_client *client,
}
chip_id = i2c_smbus_read_byte_data(client, BMA150_CHIP_ID_REG);
- if (chip_id != BMA150_CHIP_ID) {
+ if (chip_id != BMA150_CHIP_ID && chip_id != BMA180_CHIP_ID) {
dev_err(&client->dev, "BMA150 chip id error: %d\n", chip_id);
return -EINVAL;
}
@@ -643,6 +644,7 @@ static UNIVERSAL_DEV_PM_OPS(bma150_pm, bma150_suspend, bma150_resume, NULL);
static const struct i2c_device_id bma150_id[] = {
{ "bma150", 0 },
+ { "bma180", 0 },
{ "smb380", 0 },
{ "bma023", 0 },
{ }
--
1.9.1
^ permalink raw reply related
* [PATCH] tca8418: fix loading this driver as a module from a device tree
From: Dr. H. Nikolaus Schaller @ 2014-04-10 9:29 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, LKML, Marek Belisko
Loading the tca8418 driver as a module on a device tree based system needs
a MODULE_ALIAS because the driver name does not match the automatic
name generation rules of a 'compatible' entry on i2c bus.
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
drivers/input/keyboard/tca8418_keypad.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/input/keyboard/tca8418_keypad.c b/drivers/input/keyboard/tca8418_keypad.c
index 55c1530..3556360 100644
--- a/drivers/input/keyboard/tca8418_keypad.c
+++ b/drivers/input/keyboard/tca8418_keypad.c
@@ -392,6 +392,11 @@ static const struct of_device_id tca8418_dt_ids[] = {
{ }
};
MODULE_DEVICE_TABLE(of, tca8418_dt_ids);
+/* the device tree based i2c loader looks for
+ * "i2c:" + second_component_of(property("compatible"))
+ * and therefore we need an alias to be found.
+ */
+MODULE_ALIAS("i2c:tca8418");
#endif
static struct i2c_driver tca8418_keypad_driver = {
--
1.9.1
^ permalink raw reply related
* [PATCH] gpio_keys, twl4030-pwrbutton: stay awake for 1sec on resume
From: Dr. H. Nikolaus Schaller @ 2014-04-10 9:29 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Sachin Kamat, Sebastian Reichel, Aaro Koskinen, Kumar Gala,
linux-input, LKML, Marek Belisko, Lukas Maerdian
This gives the userspace (e.g. Replicant) a chance to fully handle the
pm_wakeup_event, before autosleep suspends the system alltogether
again.
This fixes suspend/resume on the OpenPhoenux GTA04, in combination with
the Replicant 4.2.2 userspace, which needs to execute this to stay
awake: 'echo on > /sys/power/state'
Signed-off-by: Lukas Märdian <lukas@goldelico.com>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
drivers/input/keyboard/gpio_keys.c | 7 ++-----
drivers/input/misc/twl4030-pwrbutton.c | 2 +-
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 2db1324..be8c62e 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -345,9 +345,6 @@ static void gpio_keys_gpio_work_func(struct work_struct *work)
container_of(work, struct gpio_button_data, work);
gpio_keys_gpio_report_event(bdata);
-
- if (bdata->button->wakeup)
- pm_relax(bdata->input->dev.parent);
}
static void gpio_keys_gpio_timer(unsigned long _data)
@@ -364,7 +361,7 @@ static irqreturn_t gpio_keys_gpio_isr(int irq, void *dev_id)
BUG_ON(irq != bdata->irq);
if (bdata->button->wakeup)
- pm_stay_awake(bdata->input->dev.parent);
+ pm_wakeup_event(bdata->input->dev.parent, 1000);
if (bdata->timer_debounce)
mod_timer(&bdata->timer,
jiffies + msecs_to_jiffies(bdata->timer_debounce));
@@ -402,7 +399,7 @@ static irqreturn_t gpio_keys_irq_isr(int irq, void *dev_id)
if (!bdata->key_pressed) {
if (bdata->button->wakeup)
- pm_wakeup_event(bdata->input->dev.parent, 0);
+ pm_wakeup_event(bdata->input->dev.parent, 1000);
input_event(input, EV_KEY, button->code, 1);
input_sync(input);
diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c
index fb3b63b..8622e43 100644
--- a/drivers/input/misc/twl4030-pwrbutton.c
+++ b/drivers/input/misc/twl4030-pwrbutton.c
@@ -41,7 +41,7 @@ static irqreturn_t powerbutton_irq(int irq, void *_pwr)
err = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &value, STS_HW_CONDITIONS);
if (!err) {
- pm_wakeup_event(pwr->dev.parent, 0);
+ pm_wakeup_event(pwr->dev.parent, 1000);
input_report_key(pwr, KEY_POWER, value & PWR_PWRON_IRQ);
input_sync(pwr);
} else {
--
1.9.1
^ permalink raw reply related
* Re: [PATCH resend 2/2] input/serio/8042: Add firmware_id support
From: Hans de Goede @ 2014-04-10 9:02 UTC (permalink / raw)
To: Dmitry Torokhov, Matthew Garrett
Cc: Benjamin Tissoires, Peter Hutterer, linux-input
In-Reply-To: <20140409200923.GA25808@core.coreip.homeip.net>
Hi,
On 04/09/2014 10:09 PM, Dmitry Torokhov wrote:
> On Wed, Apr 09, 2014 at 07:29:26PM +0100, Matthew Garrett wrote:
>> On Wed, Apr 09, 2014 at 11:24:34AM -0700, Dmitry Torokhov wrote:
>>
>>> Do we need all IDs? I'd expect we only interested in HID, not CIDs?
>>
>> I think HID handles the cases we've seen so far, but we could imagine a
>> system vendor providing their own HID, a trackpad vendor's CID and then
>> the generic mouse CID. It seems better to err on the side of including
>> them.
>
> OK, fair enough. Another question - do we want to prefix IDs with "PNP:"
> prefix so that if we add device tree in the future we'll know what kind
> of IDs we are dealing with?
I'm a bit divided on this, adding a "PNP: " prefix will make it a bit harder
to parse, OTOH once we will have other users like devicetree knowing where
the info comes from will be very useful. To me in the end the latter argument
wins. Let me know if you agree and I'll do a v3 adding the PNP: prefix.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH 2/4] serio: Add pnp_id to struct serio
From: Hans de Goede @ 2014-04-10 8:03 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Benjamin Tissoires, Peter Hutterer, linux-input
In-Reply-To: <20140409180652.GA3060@core.coreip.homeip.net>
Hi,
On 04/09/2014 08:06 PM, Dmitry Torokhov wrote:
> Hi Hans,
>
> On Wed, Apr 09, 2014 at 04:02:59PM +0200, Hans de Goede wrote:
>> Serio device drivers need access to the pnp_id of the serio port, windows
>> drivers bind by the pnp-id and some drivers need to know the exact pnp-id
>> used so they know exactly with which hardware model / revision they are
>> dealing with.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> include/linux/serio.h | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/include/linux/serio.h b/include/linux/serio.h
>> index 9f779c7..6532440 100644
>> --- a/include/linux/serio.h
>> +++ b/include/linux/serio.h
>> @@ -16,6 +16,7 @@
>> #include <linux/mutex.h>
>> #include <linux/device.h>
>> #include <linux/mod_devicetable.h>
>> +#include <linux/pnp.h>
>> #include <uapi/linux/serio.h>
>>
>> struct serio {
>> @@ -28,6 +29,7 @@ struct serio {
>> bool manual_bind;
>>
>> struct serio_device_id id;
>> + struct pnp_id *pnp_id;
>
> Why do we need this if we are already adding generic 'firmware_id'
> attribute?
In patch 4/4 we do pnp-id matching in synaptics.c to set the new
INPUT_PROP_TOPBUTTONPAD property on relevant touchpads. Since the kernel
has a special pnp_id type + pnp_id matching code, the correct thing to
do is to use that for in kernel pnp-id matching, which means that we
need to propagate the pnp-id to the serio drivers.
Regards,
Hans
^ permalink raw reply
* [PATCH 3.4 025/134] HID: clean up quirk for Sony RF receivers
From: Greg Kroah-Hartman @ 2014-04-10 3:22 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kroah-Hartman, stable, Fernando Luis Vazquez Cao,
Jiri Kosina, Ben Hutchings, Yijing Wang, linux-input, linux-usb
In-Reply-To: <20140410032259.587501440@linuxfoundation.org>
3.4-stable review patch. If anyone has any objections, please let me know.
------------------
From: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
commit 99d249021abd4341771523ed8dd7946276103432 upstream.
Document what the fix-up is does and make it more robust by ensuring
that it is only applied to the USB interface that corresponds to the
mouse (sony_report_fixup() is called once per interface during probing).
Cc: linux-input@vger.kernel.org
Cc: linux-usb@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Cc: Yijing Wang <wangyijing@huawei.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/hid/hid-sony.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -44,9 +44,19 @@ static __u8 *sony_report_fixup(struct hi
{
struct sony_sc *sc = hid_get_drvdata(hdev);
- if ((sc->quirks & VAIO_RDESC_CONSTANT) &&
- *rsize >= 56 && rdesc[54] == 0x81 && rdesc[55] == 0x07) {
+ /*
+ * Some Sony RF receivers wrongly declare the mouse pointer as a
+ * a constant non-data variable.
+ */
+ if ((sc->quirks & VAIO_RDESC_CONSTANT) && *rsize >= 56 &&
+ /* usage page: generic desktop controls */
+ /* rdesc[0] == 0x05 && rdesc[1] == 0x01 && */
+ /* usage: mouse */
+ rdesc[2] == 0x09 && rdesc[3] == 0x02 &&
+ /* input (usage page for x,y axes): constant, variable, relative */
+ rdesc[54] == 0x81 && rdesc[55] == 0x07) {
hid_info(hdev, "Fixing up Sony RF Receiver report descriptor\n");
+ /* input: data, variable, relative */
rdesc[55] = 0x06;
}
^ permalink raw reply
* [PATCH 3.4 024/134] HID: add support for Sony RF receiver with USB product id 0x0374
From: Greg Kroah-Hartman @ 2014-04-10 3:22 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kroah-Hartman, stable, Fernando Luis Vazquez Cao,
Jiri Kosina, Ben Hutchings, Yijing Wang, linux-input, linux-usb
In-Reply-To: <20140410032259.587501440@linuxfoundation.org>
3.4-stable review patch. If anyone has any objections, please let me know.
------------------
From: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
commit a464918419f94a0043d2f549d6defb4c3f69f68a upstream.
Some Vaio desktop computers, among them the VGC-LN51JGB multimedia PC, have
a RF receiver, multi-interface USB device 054c:0374, that is used to connect
a wireless keyboard and a wireless mouse.
The keyboard works flawlessly, but the mouse (VGP-WMS3 in my case) does not
seem to be generating any pointer events. The problem is that the mouse pointer
is wrongly declared as a constant non-data variable in the report descriptor
(see lsusb and usbhid-dump output below), with the consequence that it is
ignored by the HID code.
Add this device to the have-special-driver list and fix up the report
descriptor in the Sony-specific driver which happens to already have a fixup
for a similar firmware bug.
# lsusb -vd 054C:0374
Bus 003 Device 002: ID 054c:0374 Sony Corp.
Device Descriptor:
bLength 18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass 0 (Defined at Interface level)
bDeviceSubClass 0
bDeviceProtocol 0
bMaxPacketSize0 8
idVendor 0x054c Sony Corp.
idProduct 0x0374
iSerial 0
[...]
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 0
bNumEndpoints 1
bInterfaceClass 3 Human Interface Device
bInterfaceSubClass 1 Boot Interface Subclass
bInterfaceProtocol 2 Mouse
iInterface 2 RF Receiver
[...]
Report Descriptor: (length is 100)
[...]
Item(Global): Usage Page, data= [ 0x01 ] 1
Generic Desktop Controls
Item(Local ): Usage, data= [ 0x30 ] 48
Direction-X
Item(Local ): Usage, data= [ 0x31 ] 49
Direction-Y
Item(Global): Report Count, data= [ 0x02 ] 2
Item(Global): Report Size, data= [ 0x08 ] 8
Item(Global): Logical Minimum, data= [ 0x81 ] 129
Item(Global): Logical Maximum, data= [ 0x7f ] 127
Item(Main ): Input, data= [ 0x07 ] 7
Constant Variable Relative No_Wrap Linear
Preferred_State No_Null_Position Non_Volatile Bitfield
# usbhid-dump
003:002:001:DESCRIPTOR 1357910009.758544
05 01 09 02 A1 01 05 01 09 02 A1 02 85 01 09 01
A1 00 05 09 19 01 29 05 95 05 75 01 15 00 25 01
81 02 75 03 95 01 81 01 05 01 09 30 09 31 95 02
75 08 15 81 25 7F 81 07 A1 02 85 01 09 38 35 00
45 00 15 81 25 7F 95 01 75 08 81 06 C0 A1 02 85
01 05 0C 15 81 25 7F 95 01 75 08 0A 38 02 81 06
C0 C0 C0 C0
Cc: linux-input@vger.kernel.org
Cc: linux-usb@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Fernando Luis Vazquez Cao <fernando@oss.ntt.co.jp>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Cc: Yijing Wang <wangyijing@huawei.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/hid/hid-core.c | 1 +
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-sony.c | 4 +++-
3 files changed, 5 insertions(+), 1 deletion(-)
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1619,6 +1619,7 @@ static const struct hid_device_id hid_ha
{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_NAVIGATION_CONTROLLER) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_PS3_CONTROLLER) },
{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_VAIO_VGX_MOUSE) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_VAIO_VGP_MOUSE) },
{ HID_USB_DEVICE(USB_VENDOR_ID_STANTUM, USB_DEVICE_ID_MTP) },
{ HID_USB_DEVICE(USB_VENDOR_ID_STANTUM_STM, USB_DEVICE_ID_MTP_STM) },
{ HID_USB_DEVICE(USB_VENDOR_ID_STANTUM_SITRONIX, USB_DEVICE_ID_MTP_SITRONIX) },
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -685,6 +685,7 @@
#define USB_VENDOR_ID_SONY 0x054c
#define USB_DEVICE_ID_SONY_VAIO_VGX_MOUSE 0x024b
+#define USB_DEVICE_ID_SONY_VAIO_VGP_MOUSE 0x0374
#define USB_DEVICE_ID_SONY_PS3_CONTROLLER 0x0268
#define USB_DEVICE_ID_SONY_NAVIGATION_CONTROLLER 0x042f
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -46,7 +46,7 @@ static __u8 *sony_report_fixup(struct hi
if ((sc->quirks & VAIO_RDESC_CONSTANT) &&
*rsize >= 56 && rdesc[54] == 0x81 && rdesc[55] == 0x07) {
- hid_info(hdev, "Fixing up Sony Vaio VGX report descriptor\n");
+ hid_info(hdev, "Fixing up Sony RF Receiver report descriptor\n");
rdesc[55] = 0x06;
}
@@ -218,6 +218,8 @@ static const struct hid_device_id sony_d
.driver_data = SIXAXIS_CONTROLLER_BT },
{ HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_VAIO_VGX_MOUSE),
.driver_data = VAIO_RDESC_CONSTANT },
+ { HID_USB_DEVICE(USB_VENDOR_ID_SONY, USB_DEVICE_ID_SONY_VAIO_VGP_MOUSE),
+ .driver_data = VAIO_RDESC_CONSTANT },
{ }
};
MODULE_DEVICE_TABLE(hid, sony_devices);
^ permalink raw reply
* [patch]GPIO button is supposed to wake the system up if the wakeup attribute is set
From: Li, Aubrey @ 2014-04-10 2:11 UTC (permalink / raw)
To: dmitry.torokhov, sachin.kamat, linux-input, LKML
When the wakeup attribute is set, GPIO button is supposed to set
irqflag - IRQF_NO_SUSPEND to request irq. So when the system enters
the suspend sleep mode, the GPIO irq keeps enabled and is able to
wake the system up.
The affected/tested machines include Dell Venue 11 Pro and Asus T100TA.
Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com>
---
drivers/input/keyboard/gpio_keys.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 2db1324..40f3963 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -503,6 +503,9 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
if (!button->can_disable)
irqflags |= IRQF_SHARED;
+ if (button->wakeup)
+ irqflags |= IRQF_NO_SUSPEND;
+
error = request_any_context_irq(bdata->irq, isr, irqflags, desc, bdata);
if (error < 0) {
dev_err(dev, "Unable to claim irq %d; error %d\n",
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH 4/4] HID: microsoft: undefining ms_map_key_clear after usage
From: Tolga Cakir @ 2014-04-10 0:02 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Derya, Jiri Kosina, Reyad Attiyat,
linux-input, linux-kernel@vger.kernel.org
In-Reply-To: <CAN+gG=HFEcsVdmuf6o2GyfSmjWRansS02MVbXdjws7xVNp1QGw@mail.gmail.com>
Am 09.04.2014 22:42, schrieb Benjamin Tissoires:
> On Fri, Apr 4, 2014 at 1:07 PM, Tolga Cakir <tolga@cevel.net> wrote:
>> This is inspired by various other hid drivers.
>>
>> Signed-off-by: Tolga Cakir <tolga@cevel.net>
>> ---
>> drivers/hid/hid-microsoft.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
>> index 5674c0c..5281d2d 100644
>> --- a/drivers/hid/hid-microsoft.c
>> +++ b/drivers/hid/hid-microsoft.c
>> @@ -211,6 +211,8 @@ static int ms_sidewinder_kb_quirk(struct hid_input *hi, struct hid_usage *usage,
>> return 1;
>> }
>>
>> +#undef ms_map_key_clear
>> +
> Not sure this is really needed. The macro is prefixed by ms_, so I
> will simply let it there.
>
> Cheers,
> Benjamin
Ouch, I've just rechecked hid-lenovo-tpkbd.c and it wasn't prefixed
there and therefore
needed. My fault, I'm sorry.
Greetings,
Tolga Cakir
>> static int ms_sidewinder_control(struct hid_device *hdev, __u8 setup)
>> {
>> struct ms_data *sc = hid_get_drvdata(hdev);
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> 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 3/4] HID: microsoft: added Sidewinder X4 / X6 sysfs support
From: Tolga Cakir @ 2014-04-09 23:58 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Derya, Jiri Kosina, Reyad Attiyat,
linux-input, linux-kernel@vger.kernel.org
In-Reply-To: <CAN+gG=HDuDW8H6_fESdHwtJSKQ4CYgXr0UM9qpYv6EPGE7W_KA@mail.gmail.com>
Am 09.04.2014 22:42, schrieb Benjamin Tissoires:
> On Fri, Apr 4, 2014 at 1:07 PM, Tolga Cakir <tolga@cevel.net> wrote:
>> This patch enables us to set the profile, LEDs and read the key mask via sysfs. Documentation is included in this patch.
>>
>> Signed-off-by: Tolga Cakir <tolga@cevel.net>
>> ---
>> .../ABI/testing/sysfs-driver-hid-microsoft | 30 +++
>> drivers/hid/hid-microsoft.c | 231 +++++++++++++++++++++
>> 2 files changed, 261 insertions(+)
>> create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-microsoft
>>
>> diff --git a/Documentation/ABI/testing/sysfs-driver-hid-microsoft b/Documentation/ABI/testing/sysfs-driver-hid-microsoft
>> new file mode 100644
>> index 0000000..9fdfad7
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-driver-hid-microsoft
>> @@ -0,0 +1,30 @@
>> +What: /sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/auto_led
>> +Date: April 2014
>> +Contact: Tolga Cakir <tolga@cevel.net>
>> +Description: This file allows you to set and view the Auto LED status.
>> + Valid values are 0 and 1.
>> +
>> +What: /sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/profile
>> +Date: April 2014
>> +Contact: Tolga Cakir <tolga@cevel.net>
>> +Description: Both, the Sidewinder X4 and the X6 can handle profiles.
>> + They can be switched by writing to this file. Profile LEDs will be
>> + set accordingly.
>> + Valid values are 1 - 3.
>> +
>> +What: /sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/key_mask
>> +Date: April 2014
>> +Contact: Tolga Cakir <tolga@cevel.net>
>> +Description: This file is read-only and outputs an unsigned long integer.
>> + Every bit represents one of the S1 - S6 extra keys on the Sidewinder
>> + X4 and S1 - S30 on the Sidewinder X6. The least significant bit
>> + represents S1 on both gaming keyboards, the most significant bit
>> + represents the Bank switch button on both keyboards. Multiple
>> + special keys can be pressed at the same time.
> So that means that the user space polls the sysfs to trigger the actual macros?
> Or maybe I am missing something.
Nope, you got it right. Originally, I was only working on the Sidewinder
X4 support, featuring 6 macro keys and 3 profiles, resulting in 18
possible outcomes.
So, I hardcoded KEY_F13 - KEY_F24 and some other keys in the hid driver.
That solution was working fine for the Sidewinder X4. However, when I got my
hands on a Sidewinder X6, supporting 30 macro keys and 3 profiles,
resulting in 90 possible outcomes, I had to overthink my original design.
I've searched for similar keyboards and found the Roccat Arvo. As far as
I've understood the code of hid-roccat-arvo, it shows pressed keys as
key_mask via
sysfs and a user land tool (http://sourceforge.net/projects/roccat)
polls sysfs to handle the keypresses. I'm doing essentially the same
here, too.
>> +
>> +What: /sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/record_led
>> +Date: April 2014
>> +Contact: Tolga Cakir <tolga@cevel.net>
>> +Description: This file allows you to set and view the Record LED status.
>> + Valid values are 0 - 2. 0 stands for off, 1 for solid mode and 2
>> + for blinking mode.
>> diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
>> index 5b5d40f..5674c0c 100644
>> --- a/drivers/hid/hid-microsoft.c
>> +++ b/drivers/hid/hid-microsoft.c
>> @@ -19,6 +19,8 @@
>> #include <linux/input.h>
>> #include <linux/hid.h>
>> #include <linux/module.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/usb.h>
> iiiik, I really don't like seeing this in a HID driver. I spent too
> much try fighting to make HID usb agnostic to accept that blindy.
> Fortunately, it doesn't seems to be used in the final patch.
Aww, sorry to make you see such cruel things :(. I will fix it in v2,
thank you for pointing that out; I wasn't aware of it.
>> #include "hid-ids.h"
>>
>> @@ -209,6 +211,219 @@ static int ms_sidewinder_kb_quirk(struct hid_input *hi, struct hid_usage *usage,
>> return 1;
>> }
>>
>> +static int ms_sidewinder_control(struct hid_device *hdev, __u8 setup)
>> +{
>> + struct ms_data *sc = hid_get_drvdata(hdev);
>> + struct ms_sidewinder_extra *sidewinder = sc->extra;
>> + struct hid_report *report =
>> + hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[7];
>> +
>> + /*
>> + * LEDs 1 - 3 should not be set simultaneously, however
>> + * they can be set in any combination with Auto or Record LEDs.
>> + */
>> + report->field[0]->value[0] = (setup & 0x01) ? 0x01 : 0x00; /* X6 only: Macro Pad toggle */
>> + report->field[0]->value[1] = (setup & 0x02) ? 0x01 : 0x00; /* LED Auto */
>> + report->field[0]->value[2] = (setup & 0x04) ? 0x01 : 0x00; /* LED 1 */
>> + report->field[0]->value[3] = (setup & 0x08) ? 0x01 : 0x00; /* LED 2 */
>> + report->field[0]->value[4] = (setup & 0x10) ? 0x01 : 0x00; /* LED 3 */
>> + report->field[1]->value[0] = 0x00; /* Clear Record LED */
>> +
>> + switch (setup & 0x60) {
>> + case 0x40: report->field[1]->value[0] = 0x02; break; /* Record LED Blink */
>> + case 0x20: report->field[1]->value[0] = 0x03; break; /* Record LED Solid */
> do we have any guarantees that setup & 0x60 != 0x60?
I think there is no possibility for that outcome. The only code, which
touches these bits, can be found in
static ms_sidewinder_record_store. Everytime, prior writing to these
bits, they're cleared and the only
possible values are either 0x40, 0x20, 0x00 or 0x10 (if LED 3 is on).
But I think, I get your point. I will create
another case for 0x60 in terms of error handling in v2.
>> + }
>> +
>> + hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
>> + sidewinder->status = setup;
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * Sidewinder sysfs
>> + * @key_mask: show pressed special keys
>> + * @profile: show and set profile count and LED status
>> + * @auto_led: show and set LED Auto
>> + * @record_led: show and set Record LED
>> + */
>> +static ssize_t ms_sidewinder_key_mask_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> + struct ms_data *sc = hid_get_drvdata(hdev);
>> + struct ms_sidewinder_extra *sidewinder = sc->extra;
>> +
>> + return snprintf(buf, PAGE_SIZE, "%lu\n", sidewinder->key_mask);
>> +}
>> +
>> +static struct device_attribute dev_attr_ms_sidewinder_key_mask = {
>> + .attr = { .name = __stringify(key_mask), .mode = S_IRUGO },
>> + .show = ms_sidewinder_key_mask_show
>> +};
>> +
>> +static ssize_t ms_sidewinder_profile_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> + struct ms_data *sc = hid_get_drvdata(hdev);
>> + struct ms_sidewinder_extra *sidewinder = sc->extra;
>> +
>> + return snprintf(buf, PAGE_SIZE, "%1u\n", sidewinder->profile);
>> +}
>> +
>> +static ssize_t ms_sidewinder_profile_store(struct device *dev,
>> + struct device_attribute *attr, char const *buf, size_t count)
>> +{
>> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> + struct ms_data *sc = hid_get_drvdata(hdev);
>> + struct ms_sidewinder_extra *sidewinder = sc->extra;
>> + __u8 setup = sidewinder->status & ~(0x1c); /* Clear Profile LEDs */
>> +
>> + if (sscanf(buf, "%1u", &sidewinder->profile) != 1)
>> + return -EINVAL;
>> +
>> + if (sidewinder->profile < 1 || sidewinder->profile > 3) {
>> + return -EINVAL;
>> + } else {
>> + setup |= 0x02 << sidewinder->profile;
>> + ms_sidewinder_control(hdev, setup);
>> + return strnlen(buf, PAGE_SIZE);
>> + }
>> +}
>> +
>> +static struct device_attribute dev_attr_ms_sidewinder_profile =
>> + __ATTR(profile, S_IWUSR | S_IRUGO,
>> + ms_sidewinder_profile_show,
>> + ms_sidewinder_profile_store);
>> +
>> +static ssize_t ms_sidewinder_macro_pad_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> + struct ms_data *sc = hid_get_drvdata(hdev);
>> + struct ms_sidewinder_extra *sidewinder = sc->extra;
>> +
>> + return snprintf(buf, PAGE_SIZE, "%1u\n", (sidewinder->status & 0x01));
>> +}
>> +
>> +static ssize_t ms_sidewinder_macro_pad_store(struct device *dev,
>> + struct device_attribute *attr, char const *buf, size_t count)
>> +{
>> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> + struct ms_data *sc = hid_get_drvdata(hdev);
>> + struct ms_sidewinder_extra *sidewinder = sc->extra;
>> + unsigned int value;
>> + __u8 setup;
>> +
>> + if (sscanf(buf, "%1u", &value) != 1)
>> + return -EINVAL;
>> +
>> + if (value < 0 || value > 1) {
>> + return -EINVAL;
>> + } else {
>> + setup = sidewinder->status & ~(0x01); /* Clear Macro Pad */
>> + if (value)
>> + setup |= 0x01;
>> + ms_sidewinder_control(hdev, setup);
>> + return strnlen(buf, PAGE_SIZE);
>> + }
>> +}
>> +
>> +static struct device_attribute dev_attr_ms_sidewinder_macro_pad =
>> + __ATTR(macro_pad, S_IWUSR | S_IRUGO,
>> + ms_sidewinder_macro_pad_show,
>> + ms_sidewinder_macro_pad_store);
>> +
>> +static ssize_t ms_sidewinder_record_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> + struct ms_data *sc = hid_get_drvdata(hdev);
>> + struct ms_sidewinder_extra *sidewinder = sc->extra;
>> +
>> + return snprintf(buf, PAGE_SIZE, "%1d\n", (sidewinder->status & 0x60) >> 5);
>> +}
>> +
>> +static ssize_t ms_sidewinder_record_store(struct device *dev,
>> + struct device_attribute *attr, char const *buf, size_t count)
>> +{
>> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> + struct ms_data *sc = hid_get_drvdata(hdev);
>> + struct ms_sidewinder_extra *sidewinder = sc->extra;
>> + unsigned int value;
>> + __u8 setup;
>> +
>> + if (sscanf(buf, "%1u", &value) != 1)
>> + return -EINVAL;
>> +
>> + if (value < 0 || value > 2) {
>> + return -EINVAL;
>> + } else {
>> + setup = sidewinder->status & ~(0xe0); /* Clear Record LED */
>> + if (value)
>> + setup |= 0x10 << value;
>> + ms_sidewinder_control(hdev, setup);
>> + return strnlen(buf, PAGE_SIZE);
>> + }
>> +}
>> +
>> +static struct device_attribute dev_attr_ms_sidewinder_record =
>> + __ATTR(record_led, S_IWUSR | S_IRUGO,
>> + ms_sidewinder_record_show,
>> + ms_sidewinder_record_store);
>> +
>> +static ssize_t ms_sidewinder_auto_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> + struct ms_data *sc = hid_get_drvdata(hdev);
>> + struct ms_sidewinder_extra *sidewinder = sc->extra;
>> +
>> + return snprintf(buf, PAGE_SIZE, "%1d\n", (sidewinder->status & 0x02) >> 1);
>> +}
>> +
>> +static ssize_t ms_sidewinder_auto_store(struct device *dev,
>> + struct device_attribute *attr, char const *buf, size_t count)
>> +{
>> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>> + struct ms_data *sc = hid_get_drvdata(hdev);
>> + struct ms_sidewinder_extra *sidewinder = sc->extra;
>> + unsigned int value;
>> + __u8 setup;
>> +
>> + if (sscanf(buf, "%1d", &value) != 1)
>> + return -EINVAL;
>> +
>> + if (value < 0 || value > 1) {
>> + return -EINVAL;
>> + } else {
>> + setup = sidewinder->status & ~(0x02); /* Clear Auto LED */
>> + if (value)
>> + setup |= 0x02;
>> + ms_sidewinder_control(hdev, setup);
>> + return strnlen(buf, PAGE_SIZE);
>> + }
>> +}
>> +
>> +static struct device_attribute dev_attr_ms_sidewinder_auto =
>> + __ATTR(auto_led, S_IWUSR | S_IRUGO,
>> + ms_sidewinder_auto_show,
>> + ms_sidewinder_auto_store);
>> +
>> +static struct attribute *ms_attributes[] = {
>> + &dev_attr_ms_sidewinder_key_mask.attr,
>> + &dev_attr_ms_sidewinder_profile.attr,
>> + &dev_attr_ms_sidewinder_macro_pad.attr,
>> + &dev_attr_ms_sidewinder_record.attr,
>> + &dev_attr_ms_sidewinder_auto.attr,
>> + NULL
>> +};
>> +
>> +static const struct attribute_group ms_attr_group = {
>> + .attrs = ms_attributes,
>> +};
>> +
>> static int ms_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>> struct hid_field *field, struct hid_usage *usage,
>> unsigned long **bit, int *max)
>> @@ -357,6 +572,13 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> return -ENOMEM;
>> }
>> sc->extra = sidewinder;
>> +
>> + /* Create sysfs files for the Consumer Control Device only */
>> + if (hdev->type == 2) {
> type is an enum, so using HID_TYPE_USBNONE makes more sense.
Nice! Thank you for that hint!
>> + if (sysfs_create_group(&hdev->dev.kobj, &ms_attr_group)) {
>> + hid_warn(hdev, "Could not create sysfs group\n");
>> + }
>> + }
>> }
>>
>> ret = hid_parse(hdev);
>> @@ -377,6 +599,14 @@ err_free:
>> return ret;
>> }
>>
>> +static void ms_remove(struct hid_device *hdev)
>> +{
>> + sysfs_remove_group(&hdev->dev.kobj,
>> + &ms_attr_group);
> I guess this should be protected by some "if".
Yepp, you're right!
> Regarding the rest of the sysfs code. I did not went too deeper, but
> it on overall looks good to me.
>
> However, I am a little bit concerned by the API you are providing. We
> have already a LED API (have a look at hid-sony.c for instance) which
> seems to be more commonly used.
> It may not gives all of the features you need, but having a specific
> API for only two keyboards seems too much for me.
I originally tried to use the common LED API, but couldn't get it to
work. It's just too
difficult for me, I'm very new to C and kernel development. I've studied
hid-sony.c
countless times, but I guess I need more examples to fully understand
the LED API.
I'll keep you updated about my progress.
> Also, If I read correctly, this patch set does not provide a way to
> use the macro keys: they are just stored and can be polled through
> sysfs (which I don't like).
>
> I have no idea how the other keyboards deal with macro keys, but this
> implementation seems not enough for me.
>
> Cheers,
> Benjamin
As I already mentioned, the Roccat Arvo (and several other Roccat gaming
keyboards) handle
the macro keys like this.
Basically, there are two types of gaming keyboards out there; the ones,
which don't depend
on software / drivers and do everything in hardware. For such keyboards,
one usually needs
a user-land tool, which programs the keyboard's internal memory and
that's it. It sends out
the standard USB packets, without the need of any software / driver
afterwards.
The other type of keyboards depend on software. Each macro key sends out
a specific USB
packet, the software catches these packets and handles it.
So, what I did there, is basically splitting up the driver in a kernel
and a user-land component.
The kernel recognizes the key-presses and shows them via sysfs and the
user-land tool
executes events (simple key-presses, scripts etc.), depending on which
bit is set in key_mask.
Atleast, that's what I thought would be the best solution. Hardcoding
keycodes might
work for some keyboards (Logitech G710+, Razer BlackWidow Ultimate 2013,
Cooler Master
Storm Trigger), but will definitely fail on keyboards like the
Sidewinder X6, Corsair K90 / K95
or the Logitech G110 featuring 50+ macro keys.
I'd love to approach another solution, but the problem is, that the
Linux kernel is currently
not prepared for handling so many extra keys. We'd need more keycodes
for such a solution,
like defining KEY_M1 - KEY_M99 for example. This way, we could make the
kernel driver
send out keycodes and eliminate the use of sysfs for that. But I don't
think that would be a
good solution. What are your thoughts Benjamin?
Again, thank you for your patience and for your time Benjamin!
Greetings,
Tolga Cakir
>> +
>> + hid_hw_stop(hdev);
>> +}
>> +
>> static const struct hid_device_id ms_devices[] = {
>> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_GV),
>> .driver_data = MS_HIDINPUT },
>> @@ -419,6 +649,7 @@ static struct hid_driver ms_driver = {
>> .input_mapped = ms_input_mapped,
>> .event = ms_event,
>> .probe = ms_probe,
>> + .remove = ms_remove,
>> };
>> module_hid_driver(ms_driver);
>>
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
> --
> 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 2/4] HID: microsoft: initial support for Microsoft Sidewinder X4 / X6 keyboards
From: Tolga Cakir @ 2014-04-09 22:30 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Derya, Jiri Kosina, Reyad Attiyat, linux-input,
linux-kernel@vger.kernel.org
In-Reply-To: <CAN+gG=HpAXhME5YiDoX=7R30QHTKQ3Xjf2DB0jAh49h5KGpwxA@mail.gmail.com>
Am 09.04.2014 22:41, schrieb Benjamin Tissoires:
> On Fri, Apr 4, 2014 at 1:06 PM, Tolga Cakir <tolga@cevel.net> wrote:
>> This patch will let hid-microsoft handle the Microsoft Sidewinder X4 and X6 keyboards.
> I think this commit message should be a little bit more explicit,
> especially because the current patch:
> - supports 3 keys which were not supported before
> - prepares the support of the S1-S30 keys, but without actually
> delivering events (which is a little bit awkward way of splitting the
> series IMO)
Yepp, I think I will put patch 2 and 3 together. It will make more sense
then.
>> Signed-off-by: Tolga Cakir <tolga@cevel.net>
>> ---
>> drivers/hid/hid-core.c | 2 +
>> drivers/hid/hid-ids.h | 2 +
>> drivers/hid/hid-microsoft.c | 114 ++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 118 insertions(+)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index dbe548b..5de5ba1 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1815,6 +1815,8 @@ static const struct hid_device_id hid_have_special_driver[] = {
>> { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_PICOLCD_BOOTLOADER) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_COMFORT_MOUSE_4500) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_GV) },
>> + { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_X6) },
>> + { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_X4) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_NE4K) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_NE4K_JP) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_LK6K) },
>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>> index 548c1a5..21be65d 100644
>> --- a/drivers/hid/hid-ids.h
>> +++ b/drivers/hid/hid-ids.h
>> @@ -626,6 +626,8 @@
>> #define USB_DEVICE_ID_MS_PRESENTER_8K_BT 0x0701
>> #define USB_DEVICE_ID_MS_PRESENTER_8K_USB 0x0713
>> #define USB_DEVICE_ID_MS_DIGITAL_MEDIA_3K 0x0730
>> +#define USB_DEVICE_ID_SIDEWINDER_X6 0x074b
>> +#define USB_DEVICE_ID_SIDEWINDER_X4 0x0768
>> #define USB_DEVICE_ID_MS_COMFORT_MOUSE_4500 0x076c
>> #define USB_DEVICE_ID_MS_TOUCH_COVER_2 0x07a7
>> #define USB_DEVICE_ID_MS_TYPE_COVER_2 0x07a9
>> diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
>> index 0a61403..5b5d40f 100644
>> --- a/drivers/hid/hid-microsoft.c
>> +++ b/drivers/hid/hid-microsoft.c
>> @@ -29,12 +29,28 @@
>> #define MS_NOGET 0x10
>> #define MS_DUPLICATE_USAGES 0x20
>> #define MS_RDESC_3K 0x40
>> +#define MS_SIDEWINDER 0x80
>>
>> struct ms_data {
>> unsigned long quirks;
>> void *extra;
> ok, so now extra is used, but with a struct ms_sidewinder_extra.
> I do not think having a void pointer is a right choice here given that
> there is only one possibility. So I'd rather have a direct struct
> ms_sidewinder_extra pointer and the future will tell us if we need to
> have a void pointer or not.
>
> In fact, I'd rather not having to allocate twice during probe, so I
> would even consider integrating struct ms_sidewinder_extra directly in
> struct ms_data.
Originally, I had thought about the same, but discarded the idea of
integrating struct ms_sidewinder_extra into struct ms_data, because
other Microsoft devices would allocate space, which wouldn't be used.
I think having a struct ms_sidewinder_extra pointer in struct ms_data is
the better idea. But if you want me to integrate ms_sidewinder_extra
into ms_data, I will do it ;)!
>
>> };
>>
>> +/*
>> + * For Sidewinder X4 / X6 devices.
>> + * @profile: for storing profile status.
>> + * @status: holds information about LED states and numpad mode (X6
>> + * only). The 1st bit is for numpad mode, bits 2 - 7 are reserved for
>> + * LED configuration and the last bit is currently unused.
>> + * @key_mask: holds information about pressed special keys. It's
>> + * readable via sysfs, so user-space tools can handle keys.
>> + */
>> +struct ms_sidewinder_extra {
>> + unsigned profile;
>> + __u8 status;
> Both profile and status are not used in this patch.
>
> Splitting a big patch into some smaller ones is good, but keep in mind
> that we want to have patches that are self consistent and that do only
> what they say.
> Let's assume we revert in the future 3/4, what will happens with those
> two fields which will not be used anymore?
Thank you for clearing that up, didn't thought about that. I will put
patch 2 and patch 3 into one big patch, cause else it doesn't make any
sense.
>> + unsigned long key_mask;
>> +};
>> +
>> static __u8 *ms_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>> unsigned int *rsize)
>> {
>> @@ -143,6 +159,56 @@ static int ms_presenter_8k_quirk(struct hid_input *hi, struct hid_usage *usage,
>> return 1;
>> }
>>
>> +static int ms_sidewinder_kb_quirk(struct hid_input *hi, struct hid_usage *usage,
>> + unsigned long **bit, int *max)
>> +{
>> + set_bit(EV_REP, hi->input->evbit);
> I think you should also test against the usage page. It looks like the
> usages are specific enough, but we never know.
Okay, I'll come up with a solution in v2, thanks!
>> + switch (usage->hid & HID_USAGE) {
>> + /*
>> + * Registering Sidewinder X4 / X6 special keys. S1 - S6 macro keys
>> + * are shared between Sidewinder X4 & X6 and are programmable.
>> + */
>> + case 0xfb01: ms_map_key_clear(KEY_UNKNOWN); break; /* S1 */
>> + case 0xfb02: ms_map_key_clear(KEY_UNKNOWN); break; /* S2 */
>> + case 0xfb03: ms_map_key_clear(KEY_UNKNOWN); break; /* S3 */
>> + case 0xfb04: ms_map_key_clear(KEY_UNKNOWN); break; /* S4 */
>> + case 0xfb05: ms_map_key_clear(KEY_UNKNOWN); break; /* S5 */
>> + case 0xfb06: ms_map_key_clear(KEY_UNKNOWN); break; /* S6 */
>> + /* S7 - S30 macro keys are only present on the Sidewinder X6 */
>> + case 0xfb07: ms_map_key_clear(KEY_UNKNOWN); break; /* S7 */
>> + case 0xfb08: ms_map_key_clear(KEY_UNKNOWN); break; /* S8 */
>> + case 0xfb09: ms_map_key_clear(KEY_UNKNOWN); break; /* S9 */
>> + case 0xfb0a: ms_map_key_clear(KEY_UNKNOWN); break; /* S10 */
>> + case 0xfb0b: ms_map_key_clear(KEY_UNKNOWN); break; /* S11 */
>> + case 0xfb0c: ms_map_key_clear(KEY_UNKNOWN); break; /* S12 */
>> + case 0xfb0d: ms_map_key_clear(KEY_UNKNOWN); break; /* S13 */
>> + case 0xfb0e: ms_map_key_clear(KEY_UNKNOWN); break; /* S14 */
>> + case 0xfb0f: ms_map_key_clear(KEY_UNKNOWN); break; /* S15 */
>> + case 0xfb10: ms_map_key_clear(KEY_UNKNOWN); break; /* S16 */
>> + case 0xfb11: ms_map_key_clear(KEY_UNKNOWN); break; /* S17 */
>> + case 0xfb12: ms_map_key_clear(KEY_UNKNOWN); break; /* S18 */
>> + case 0xfb13: ms_map_key_clear(KEY_UNKNOWN); break; /* S19 */
>> + case 0xfb14: ms_map_key_clear(KEY_UNKNOWN); break; /* S20 */
>> + case 0xfb15: ms_map_key_clear(KEY_UNKNOWN); break; /* S21 */
>> + case 0xfb16: ms_map_key_clear(KEY_UNKNOWN); break; /* S22 */
>> + case 0xfb17: ms_map_key_clear(KEY_UNKNOWN); break; /* S23 */
>> + case 0xfb18: ms_map_key_clear(KEY_UNKNOWN); break; /* S24 */
>> + case 0xfb19: ms_map_key_clear(KEY_UNKNOWN); break; /* S25 */
>> + case 0xfb1a: ms_map_key_clear(KEY_UNKNOWN); break; /* S26 */
>> + case 0xfb1b: ms_map_key_clear(KEY_UNKNOWN); break; /* S27 */
>> + case 0xfb1c: ms_map_key_clear(KEY_UNKNOWN); break; /* S28 */
>> + case 0xfb1d: ms_map_key_clear(KEY_UNKNOWN); break; /* S29 */
>> + case 0xfb1e: ms_map_key_clear(KEY_UNKNOWN); break; /* S30 */
>> + /* Not programmable keys: Profile, Game Center (X6 only) and Macro */
>> + case 0xfd11: ms_map_key_clear(KEY_GAMES); break; /* X6: Game Center*/
>> + case 0xfd12: ms_map_key_clear(KEY_MACRO); break; /* Macro */
>> + case 0xfd15: ms_map_key_clear(KEY_UNKNOWN); break; /* Profile */
> Arg, this is a little bit ugly.
>
> you should either consider using fall-through between the each case or
> a for loop like you did in ms_event.
>
Hehe, I'll use a loop in v2 to keep things clean.
>> + default:
>> + return 0;
>> + }
>> + return 1;
>> +}
>> +
>> static int ms_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>> struct hid_field *field, struct hid_usage *usage,
>> unsigned long **bit, int *max)
>> @@ -159,6 +225,10 @@ static int ms_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>> ms_presenter_8k_quirk(hi, usage, bit, max))
>> return 1;
>>
>> + if ((sc->quirks & MS_SIDEWINDER) &&
>> + ms_sidewinder_kb_quirk(hi, usage, bit, max))
>> + return 1;
>> +
>> return 0;
>> }
>>
>> @@ -229,6 +299,34 @@ static int ms_event(struct hid_device *hdev, struct hid_field *field,
>> return 1;
>> }
>>
>> + /*
>> + * Sidewinder special button handling & profile switching
>> + *
>> + * Pressing S1 - S30 macro keys will not send out any keycodes, but
>> + * set bits on key_mask (readable via sysfs). It's possible to press
>> + * multiple special keys at the same time.
>> + */
>> + if (sc->quirks & MS_SIDEWINDER) {
>> + struct input_dev *input = field->hidinput->input;
>> + struct ms_sidewinder_extra *sidewinder = sc->extra;
>> + int i;
>> +
>> + for (i = 0; i <= 29; i++) { /* Run through S1 - S30 keys */
>> + if ((usage->hid & HID_USAGE) == (0xfb01 + i)) {
>> + value ? set_bit(i, &sidewinder->key_mask) : clear_bit(i, &sidewinder->key_mask);
>> + break; /* Exit loop, when correct hid usage has been found */
>> + }
>> + }
>> +
>> + switch (usage->hid & HID_USAGE) {
>> + case 0xfd11: input_event(input, usage->type, KEY_GAMES, value); break;
>> + case 0xfd12: input_event(input, usage->type, KEY_MACRO, value); break;
>> + case 0xfd15: value ? set_bit(30, &sidewinder->key_mask) : clear_bit(30, &sidewinder->key_mask); break;
> I don't really like the "30" here. But I do not see an elegant way of
> getting it.
Hmm, I'll think about another solution. I might use something like
MACRO_KEYS_MAX or something like that and put that in there.
>> + }
>> +
>> + return 1;
>> + }
>> +
>> return 0;
>> }
>>
>> @@ -249,6 +347,18 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> if (sc->quirks & MS_NOGET)
>> hdev->quirks |= HID_QUIRK_NOGET;
>>
>> + if (sc->quirks & MS_SIDEWINDER) {
>> + struct ms_sidewinder_extra *sidewinder;
>> +
>> + sidewinder = devm_kzalloc(&hdev->dev, sizeof(struct ms_sidewinder_extra),
>> + GFP_KERNEL);
>> + if (!sidewinder) {
>> + hid_err(hdev, "can't alloc microsoft descriptor\n");
> Hmm, this err message does not reflect the reality (it's the same as
> the one used in 1/4).
>
> Cheers,
> Benjamin
Ouch, that hurts. I will fix it in v2!
Thank you Benjamin for your time!
Greetings,
Tolga Cakir
>> + return -ENOMEM;
>> + }
>> + sc->extra = sidewinder;
>> + }
>> +
>> ret = hid_parse(hdev);
>> if (ret) {
>> hid_err(hdev, "parse failed\n");
>> @@ -270,6 +380,10 @@ err_free:
>> static const struct hid_device_id ms_devices[] = {
>> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_GV),
>> .driver_data = MS_HIDINPUT },
>> + { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_X6),
>> + .driver_data = MS_SIDEWINDER },
>> + { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_X4),
>> + .driver_data = MS_SIDEWINDER },
>> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_OFFICE_KB),
>> .driver_data = MS_ERGONOMY },
>> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_NE4K),
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
> --
> 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 1/4] HID: microsoft: moving quirks to struct
From: Tolga Cakir @ 2014-04-09 22:07 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Derya, Jiri Kosina, Reyad Attiyat, linux-input,
linux-kernel@vger.kernel.org
In-Reply-To: <CAN+gG=FFnK7NwiGN1oLsBA6euyDHOxrY6s7-yRo553jcLbzuYQ@mail.gmail.com>
Am 09.04.2014 22:40, schrieb Benjamin Tissoires:
> On Fri, Apr 4, 2014 at 1:06 PM, Tolga Cakir <tolga@cevel.net> wrote:
>> This will give us the opportunity to easily implement extra features
>> for new devices.
>>
>> Signed-off-by: Tolga Cakir <tolga@cevel.net>
>> ---
>> drivers/hid/hid-microsoft.c | 44 ++++++++++++++++++++++++++++----------------
>> 1 file changed, 28 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
>> index 6fd5817..0a61403 100644
>> --- a/drivers/hid/hid-microsoft.c
>> +++ b/drivers/hid/hid-microsoft.c
>> @@ -30,23 +30,28 @@
>> #define MS_DUPLICATE_USAGES 0x20
>> #define MS_RDESC_3K 0x40
>>
>> +struct ms_data {
>> + unsigned long quirks;
>> + void *extra;
> I do not like having a void * pointer in this kind of struct,
> especially when it is not used in the current patch.
> Please remove it in v2
Hi Benjamin, thank you for the review!
Yes you're right, I will change that in v2.
>> +};
>> +
>> static __u8 *ms_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>> unsigned int *rsize)
>> {
>> - unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
>> + struct ms_data *sc = hid_get_drvdata(hdev);
>>
>> /*
>> * Microsoft Wireless Desktop Receiver (Model 1028) has
>> * 'Usage Min/Max' where it ought to have 'Physical Min/Max'
>> */
>> - if ((quirks & MS_RDESC) && *rsize == 571 && rdesc[557] == 0x19 &&
>> + if ((sc->quirks & MS_RDESC) && *rsize == 571 && rdesc[557] == 0x19 &&
>> rdesc[559] == 0x29) {
>> hid_info(hdev, "fixing up Microsoft Wireless Receiver Model 1028 report descriptor\n");
>> rdesc[557] = 0x35;
>> rdesc[559] = 0x45;
>> }
>> /* the same as above (s/usage/physical/) */
>> - if ((quirks & MS_RDESC_3K) && *rsize == 106 && rdesc[94] == 0x19 &&
>> + if ((sc->quirks & MS_RDESC_3K) && *rsize == 106 && rdesc[94] == 0x19 &&
>> rdesc[95] == 0x00 && rdesc[96] == 0x29 &&
>> rdesc[97] == 0xff) {
>> rdesc[94] = 0x35;
>> @@ -142,15 +147,15 @@ static int ms_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>> struct hid_field *field, struct hid_usage *usage,
>> unsigned long **bit, int *max)
>> {
>> - unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
>> + struct ms_data *sc = hid_get_drvdata(hdev);
>>
>> - if (quirks & MS_ERGONOMY) {
>> + if (sc->quirks & MS_ERGONOMY) {
>> int ret = ms_ergonomy_kb_quirk(hi, usage, bit, max);
>> if (ret)
>> return ret;
>> }
>>
>> - if ((quirks & MS_PRESENTER) &&
>> + if ((sc->quirks & MS_PRESENTER) &&
>> ms_presenter_8k_quirk(hi, usage, bit, max))
>> return 1;
>>
>> @@ -161,9 +166,9 @@ static int ms_input_mapped(struct hid_device *hdev, struct hid_input *hi,
>> struct hid_field *field, struct hid_usage *usage,
>> unsigned long **bit, int *max)
>> {
>> - unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
>> + struct ms_data *sc = hid_get_drvdata(hdev);
>>
>> - if (quirks & MS_DUPLICATE_USAGES)
>> + if (sc->quirks & MS_DUPLICATE_USAGES)
>> clear_bit(usage->code, *bit);
>>
>> return 0;
>> @@ -172,7 +177,7 @@ static int ms_input_mapped(struct hid_device *hdev, struct hid_input *hi,
>> static int ms_event(struct hid_device *hdev, struct hid_field *field,
>> struct hid_usage *usage, __s32 value)
>> {
>> - unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
>> + struct ms_data *sc = hid_get_drvdata(hdev);
>> struct input_dev *input;
>>
>> if (!(hdev->claimed & HID_CLAIMED_INPUT) || !field->hidinput ||
>> @@ -182,7 +187,7 @@ static int ms_event(struct hid_device *hdev, struct hid_field *field,
>> input = field->hidinput->input;
>>
>> /* Handling MS keyboards special buttons */
>> - if (quirks & MS_ERGONOMY && usage->hid == (HID_UP_MSVENDOR | 0xff00)) {
>> + if (sc->quirks & MS_ERGONOMY && usage->hid == (HID_UP_MSVENDOR | 0xff00)) {
>> /* Special keypad keys */
>> input_report_key(input, KEY_KPEQUAL, value & 0x01);
>> input_report_key(input, KEY_KPLEFTPAREN, value & 0x02);
>> @@ -190,7 +195,7 @@ static int ms_event(struct hid_device *hdev, struct hid_field *field,
>> return 1;
>> }
>>
>> - if (quirks & MS_ERGONOMY && usage->hid == (HID_UP_MSVENDOR | 0xff01)) {
>> + if (sc->quirks & MS_ERGONOMY && usage->hid == (HID_UP_MSVENDOR | 0xff01)) {
>> /* Scroll wheel */
>> int step = ((value & 0x60) >> 5) + 1;
>>
>> @@ -205,7 +210,7 @@ static int ms_event(struct hid_device *hdev, struct hid_field *field,
>> return 1;
>> }
>>
>> - if (quirks & MS_ERGONOMY && usage->hid == (HID_UP_MSVENDOR | 0xff05)) {
>> + if (sc->quirks & MS_ERGONOMY && usage->hid == (HID_UP_MSVENDOR | 0xff05)) {
>> static unsigned int last_key = 0;
>> unsigned int key = 0;
>> switch (value) {
>> @@ -229,12 +234,19 @@ static int ms_event(struct hid_device *hdev, struct hid_field *field,
>>
>> static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> {
>> - unsigned long quirks = id->driver_data;
>> + struct ms_data *sc;
>> int ret;
>>
>> - hid_set_drvdata(hdev, (void *)quirks);
>> + sc = devm_kzalloc(&hdev->dev, sizeof(struct ms_data), GFP_KERNEL);
>> + if (!sc) {
>> + hid_err(hdev, "can't alloc microsoft descriptor\n");
> How about: "Can not allocate microsoft data"?
>
> I have no idea where this "descriptor" comes from.
>
> Besides these two nitpicks, the rest of the patch is:
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> Cheers,
> Benjamin
That sounds better, yeah :)! It was quite some time ago, when I came up
with "descriptor".
I remember, that it was due to the "sc" I chose for ms_data.
Unfortunately, I don't remember what "sc" is standing for, but it's also
used in hid-sony. But I'm fine using your suggestion ;)!
>> + return -ENOMEM;
>> + }
>> +
>> + sc->quirks = id->driver_data;
>> + hid_set_drvdata(hdev, sc);
>>
>> - if (quirks & MS_NOGET)
>> + if (sc->quirks & MS_NOGET)
>> hdev->quirks |= HID_QUIRK_NOGET;
>>
>> ret = hid_parse(hdev);
>> @@ -243,7 +255,7 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> goto err_free;
>> }
>>
>> - ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT | ((quirks & MS_HIDINPUT) ?
>> + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT | ((sc->quirks & MS_HIDINPUT) ?
>> HID_CONNECT_HIDINPUT_FORCE : 0));
>> if (ret) {
>> hid_err(hdev, "hw start failed\n");
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
> --
> 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 4/4] HID: microsoft: undefining ms_map_key_clear after usage
From: Benjamin Tissoires @ 2014-04-09 20:42 UTC (permalink / raw)
To: Tolga Cakir
Cc: Benjamin Tissoires, Derya, Jiri Kosina, Reyad Attiyat,
linux-input, linux-kernel@vger.kernel.org
In-Reply-To: <1396631237-11813-1-git-send-email-tolga@cevel.net>
On Fri, Apr 4, 2014 at 1:07 PM, Tolga Cakir <tolga@cevel.net> wrote:
> This is inspired by various other hid drivers.
>
> Signed-off-by: Tolga Cakir <tolga@cevel.net>
> ---
> drivers/hid/hid-microsoft.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
> index 5674c0c..5281d2d 100644
> --- a/drivers/hid/hid-microsoft.c
> +++ b/drivers/hid/hid-microsoft.c
> @@ -211,6 +211,8 @@ static int ms_sidewinder_kb_quirk(struct hid_input *hi, struct hid_usage *usage,
> return 1;
> }
>
> +#undef ms_map_key_clear
> +
Not sure this is really needed. The macro is prefixed by ms_, so I
will simply let it there.
Cheers,
Benjamin
> static int ms_sidewinder_control(struct hid_device *hdev, __u8 setup)
> {
> struct ms_data *sc = hid_get_drvdata(hdev);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 3/4] HID: microsoft: added Sidewinder X4 / X6 sysfs support
From: Benjamin Tissoires @ 2014-04-09 20:42 UTC (permalink / raw)
To: Tolga Cakir
Cc: Benjamin Tissoires, Derya, Jiri Kosina, Reyad Attiyat,
linux-input, linux-kernel@vger.kernel.org
In-Reply-To: <1396631227-11770-1-git-send-email-tolga@cevel.net>
On Fri, Apr 4, 2014 at 1:07 PM, Tolga Cakir <tolga@cevel.net> wrote:
> This patch enables us to set the profile, LEDs and read the key mask via sysfs. Documentation is included in this patch.
>
> Signed-off-by: Tolga Cakir <tolga@cevel.net>
> ---
> .../ABI/testing/sysfs-driver-hid-microsoft | 30 +++
> drivers/hid/hid-microsoft.c | 231 +++++++++++++++++++++
> 2 files changed, 261 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-microsoft
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-hid-microsoft b/Documentation/ABI/testing/sysfs-driver-hid-microsoft
> new file mode 100644
> index 0000000..9fdfad7
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-hid-microsoft
> @@ -0,0 +1,30 @@
> +What: /sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/auto_led
> +Date: April 2014
> +Contact: Tolga Cakir <tolga@cevel.net>
> +Description: This file allows you to set and view the Auto LED status.
> + Valid values are 0 and 1.
> +
> +What: /sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/profile
> +Date: April 2014
> +Contact: Tolga Cakir <tolga@cevel.net>
> +Description: Both, the Sidewinder X4 and the X6 can handle profiles.
> + They can be switched by writing to this file. Profile LEDs will be
> + set accordingly.
> + Valid values are 1 - 3.
> +
> +What: /sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/key_mask
> +Date: April 2014
> +Contact: Tolga Cakir <tolga@cevel.net>
> +Description: This file is read-only and outputs an unsigned long integer.
> + Every bit represents one of the S1 - S6 extra keys on the Sidewinder
> + X4 and S1 - S30 on the Sidewinder X6. The least significant bit
> + represents S1 on both gaming keyboards, the most significant bit
> + represents the Bank switch button on both keyboards. Multiple
> + special keys can be pressed at the same time.
So that means that the user space polls the sysfs to trigger the actual macros?
Or maybe I am missing something.
> +
> +What: /sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/<hid-bus>:<vendor-id>:<product-id>.<num>/record_led
> +Date: April 2014
> +Contact: Tolga Cakir <tolga@cevel.net>
> +Description: This file allows you to set and view the Record LED status.
> + Valid values are 0 - 2. 0 stands for off, 1 for solid mode and 2
> + for blinking mode.
> diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
> index 5b5d40f..5674c0c 100644
> --- a/drivers/hid/hid-microsoft.c
> +++ b/drivers/hid/hid-microsoft.c
> @@ -19,6 +19,8 @@
> #include <linux/input.h>
> #include <linux/hid.h>
> #include <linux/module.h>
> +#include <linux/sysfs.h>
> +#include <linux/usb.h>
iiiik, I really don't like seeing this in a HID driver. I spent too
much try fighting to make HID usb agnostic to accept that blindy.
Fortunately, it doesn't seems to be used in the final patch.
>
> #include "hid-ids.h"
>
> @@ -209,6 +211,219 @@ static int ms_sidewinder_kb_quirk(struct hid_input *hi, struct hid_usage *usage,
> return 1;
> }
>
> +static int ms_sidewinder_control(struct hid_device *hdev, __u8 setup)
> +{
> + struct ms_data *sc = hid_get_drvdata(hdev);
> + struct ms_sidewinder_extra *sidewinder = sc->extra;
> + struct hid_report *report =
> + hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[7];
> +
> + /*
> + * LEDs 1 - 3 should not be set simultaneously, however
> + * they can be set in any combination with Auto or Record LEDs.
> + */
> + report->field[0]->value[0] = (setup & 0x01) ? 0x01 : 0x00; /* X6 only: Macro Pad toggle */
> + report->field[0]->value[1] = (setup & 0x02) ? 0x01 : 0x00; /* LED Auto */
> + report->field[0]->value[2] = (setup & 0x04) ? 0x01 : 0x00; /* LED 1 */
> + report->field[0]->value[3] = (setup & 0x08) ? 0x01 : 0x00; /* LED 2 */
> + report->field[0]->value[4] = (setup & 0x10) ? 0x01 : 0x00; /* LED 3 */
> + report->field[1]->value[0] = 0x00; /* Clear Record LED */
> +
> + switch (setup & 0x60) {
> + case 0x40: report->field[1]->value[0] = 0x02; break; /* Record LED Blink */
> + case 0x20: report->field[1]->value[0] = 0x03; break; /* Record LED Solid */
do we have any guarantees that setup & 0x60 != 0x60?
> + }
> +
> + hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> + sidewinder->status = setup;
> +
> + return 0;
> +}
> +
> +/*
> + * Sidewinder sysfs
> + * @key_mask: show pressed special keys
> + * @profile: show and set profile count and LED status
> + * @auto_led: show and set LED Auto
> + * @record_led: show and set Record LED
> + */
> +static ssize_t ms_sidewinder_key_mask_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> + struct ms_data *sc = hid_get_drvdata(hdev);
> + struct ms_sidewinder_extra *sidewinder = sc->extra;
> +
> + return snprintf(buf, PAGE_SIZE, "%lu\n", sidewinder->key_mask);
> +}
> +
> +static struct device_attribute dev_attr_ms_sidewinder_key_mask = {
> + .attr = { .name = __stringify(key_mask), .mode = S_IRUGO },
> + .show = ms_sidewinder_key_mask_show
> +};
> +
> +static ssize_t ms_sidewinder_profile_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> + struct ms_data *sc = hid_get_drvdata(hdev);
> + struct ms_sidewinder_extra *sidewinder = sc->extra;
> +
> + return snprintf(buf, PAGE_SIZE, "%1u\n", sidewinder->profile);
> +}
> +
> +static ssize_t ms_sidewinder_profile_store(struct device *dev,
> + struct device_attribute *attr, char const *buf, size_t count)
> +{
> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> + struct ms_data *sc = hid_get_drvdata(hdev);
> + struct ms_sidewinder_extra *sidewinder = sc->extra;
> + __u8 setup = sidewinder->status & ~(0x1c); /* Clear Profile LEDs */
> +
> + if (sscanf(buf, "%1u", &sidewinder->profile) != 1)
> + return -EINVAL;
> +
> + if (sidewinder->profile < 1 || sidewinder->profile > 3) {
> + return -EINVAL;
> + } else {
> + setup |= 0x02 << sidewinder->profile;
> + ms_sidewinder_control(hdev, setup);
> + return strnlen(buf, PAGE_SIZE);
> + }
> +}
> +
> +static struct device_attribute dev_attr_ms_sidewinder_profile =
> + __ATTR(profile, S_IWUSR | S_IRUGO,
> + ms_sidewinder_profile_show,
> + ms_sidewinder_profile_store);
> +
> +static ssize_t ms_sidewinder_macro_pad_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> + struct ms_data *sc = hid_get_drvdata(hdev);
> + struct ms_sidewinder_extra *sidewinder = sc->extra;
> +
> + return snprintf(buf, PAGE_SIZE, "%1u\n", (sidewinder->status & 0x01));
> +}
> +
> +static ssize_t ms_sidewinder_macro_pad_store(struct device *dev,
> + struct device_attribute *attr, char const *buf, size_t count)
> +{
> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> + struct ms_data *sc = hid_get_drvdata(hdev);
> + struct ms_sidewinder_extra *sidewinder = sc->extra;
> + unsigned int value;
> + __u8 setup;
> +
> + if (sscanf(buf, "%1u", &value) != 1)
> + return -EINVAL;
> +
> + if (value < 0 || value > 1) {
> + return -EINVAL;
> + } else {
> + setup = sidewinder->status & ~(0x01); /* Clear Macro Pad */
> + if (value)
> + setup |= 0x01;
> + ms_sidewinder_control(hdev, setup);
> + return strnlen(buf, PAGE_SIZE);
> + }
> +}
> +
> +static struct device_attribute dev_attr_ms_sidewinder_macro_pad =
> + __ATTR(macro_pad, S_IWUSR | S_IRUGO,
> + ms_sidewinder_macro_pad_show,
> + ms_sidewinder_macro_pad_store);
> +
> +static ssize_t ms_sidewinder_record_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> + struct ms_data *sc = hid_get_drvdata(hdev);
> + struct ms_sidewinder_extra *sidewinder = sc->extra;
> +
> + return snprintf(buf, PAGE_SIZE, "%1d\n", (sidewinder->status & 0x60) >> 5);
> +}
> +
> +static ssize_t ms_sidewinder_record_store(struct device *dev,
> + struct device_attribute *attr, char const *buf, size_t count)
> +{
> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> + struct ms_data *sc = hid_get_drvdata(hdev);
> + struct ms_sidewinder_extra *sidewinder = sc->extra;
> + unsigned int value;
> + __u8 setup;
> +
> + if (sscanf(buf, "%1u", &value) != 1)
> + return -EINVAL;
> +
> + if (value < 0 || value > 2) {
> + return -EINVAL;
> + } else {
> + setup = sidewinder->status & ~(0xe0); /* Clear Record LED */
> + if (value)
> + setup |= 0x10 << value;
> + ms_sidewinder_control(hdev, setup);
> + return strnlen(buf, PAGE_SIZE);
> + }
> +}
> +
> +static struct device_attribute dev_attr_ms_sidewinder_record =
> + __ATTR(record_led, S_IWUSR | S_IRUGO,
> + ms_sidewinder_record_show,
> + ms_sidewinder_record_store);
> +
> +static ssize_t ms_sidewinder_auto_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> + struct ms_data *sc = hid_get_drvdata(hdev);
> + struct ms_sidewinder_extra *sidewinder = sc->extra;
> +
> + return snprintf(buf, PAGE_SIZE, "%1d\n", (sidewinder->status & 0x02) >> 1);
> +}
> +
> +static ssize_t ms_sidewinder_auto_store(struct device *dev,
> + struct device_attribute *attr, char const *buf, size_t count)
> +{
> + struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> + struct ms_data *sc = hid_get_drvdata(hdev);
> + struct ms_sidewinder_extra *sidewinder = sc->extra;
> + unsigned int value;
> + __u8 setup;
> +
> + if (sscanf(buf, "%1d", &value) != 1)
> + return -EINVAL;
> +
> + if (value < 0 || value > 1) {
> + return -EINVAL;
> + } else {
> + setup = sidewinder->status & ~(0x02); /* Clear Auto LED */
> + if (value)
> + setup |= 0x02;
> + ms_sidewinder_control(hdev, setup);
> + return strnlen(buf, PAGE_SIZE);
> + }
> +}
> +
> +static struct device_attribute dev_attr_ms_sidewinder_auto =
> + __ATTR(auto_led, S_IWUSR | S_IRUGO,
> + ms_sidewinder_auto_show,
> + ms_sidewinder_auto_store);
> +
> +static struct attribute *ms_attributes[] = {
> + &dev_attr_ms_sidewinder_key_mask.attr,
> + &dev_attr_ms_sidewinder_profile.attr,
> + &dev_attr_ms_sidewinder_macro_pad.attr,
> + &dev_attr_ms_sidewinder_record.attr,
> + &dev_attr_ms_sidewinder_auto.attr,
> + NULL
> +};
> +
> +static const struct attribute_group ms_attr_group = {
> + .attrs = ms_attributes,
> +};
> +
> static int ms_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> struct hid_field *field, struct hid_usage *usage,
> unsigned long **bit, int *max)
> @@ -357,6 +572,13 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id)
> return -ENOMEM;
> }
> sc->extra = sidewinder;
> +
> + /* Create sysfs files for the Consumer Control Device only */
> + if (hdev->type == 2) {
type is an enum, so using HID_TYPE_USBNONE makes more sense.
> + if (sysfs_create_group(&hdev->dev.kobj, &ms_attr_group)) {
> + hid_warn(hdev, "Could not create sysfs group\n");
> + }
> + }
> }
>
> ret = hid_parse(hdev);
> @@ -377,6 +599,14 @@ err_free:
> return ret;
> }
>
> +static void ms_remove(struct hid_device *hdev)
> +{
> + sysfs_remove_group(&hdev->dev.kobj,
> + &ms_attr_group);
I guess this should be protected by some "if".
Regarding the rest of the sysfs code. I did not went too deeper, but
it on overall looks good to me.
However, I am a little bit concerned by the API you are providing. We
have already a LED API (have a look at hid-sony.c for instance) which
seems to be more commonly used.
It may not gives all of the features you need, but having a specific
API for only two keyboards seems too much for me.
Also, If I read correctly, this patch set does not provide a way to
use the macro keys: they are just stored and can be polled through
sysfs (which I don't like).
I have no idea how the other keyboards deal with macro keys, but this
implementation seems not enough for me.
Cheers,
Benjamin
> +
> + hid_hw_stop(hdev);
> +}
> +
> static const struct hid_device_id ms_devices[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_GV),
> .driver_data = MS_HIDINPUT },
> @@ -419,6 +649,7 @@ static struct hid_driver ms_driver = {
> .input_mapped = ms_input_mapped,
> .event = ms_event,
> .probe = ms_probe,
> + .remove = ms_remove,
> };
> module_hid_driver(ms_driver);
>
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH 2/4] HID: microsoft: initial support for Microsoft Sidewinder X4 / X6 keyboards
From: Benjamin Tissoires @ 2014-04-09 20:41 UTC (permalink / raw)
To: Tolga Cakir
Cc: Benjamin Tissoires, Derya, Jiri Kosina, Reyad Attiyat,
linux-input, linux-kernel@vger.kernel.org
In-Reply-To: <1396631216-11726-1-git-send-email-tolga@cevel.net>
On Fri, Apr 4, 2014 at 1:06 PM, Tolga Cakir <tolga@cevel.net> wrote:
> This patch will let hid-microsoft handle the Microsoft Sidewinder X4 and X6 keyboards.
I think this commit message should be a little bit more explicit,
especially because the current patch:
- supports 3 keys which were not supported before
- prepares the support of the S1-S30 keys, but without actually
delivering events (which is a little bit awkward way of splitting the
series IMO)
>
> Signed-off-by: Tolga Cakir <tolga@cevel.net>
> ---
> drivers/hid/hid-core.c | 2 +
> drivers/hid/hid-ids.h | 2 +
> drivers/hid/hid-microsoft.c | 114 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 118 insertions(+)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index dbe548b..5de5ba1 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1815,6 +1815,8 @@ static const struct hid_device_id hid_have_special_driver[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_PICOLCD_BOOTLOADER) },
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_COMFORT_MOUSE_4500) },
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_GV) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_X6) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_X4) },
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_NE4K) },
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_NE4K_JP) },
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_LK6K) },
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 548c1a5..21be65d 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -626,6 +626,8 @@
> #define USB_DEVICE_ID_MS_PRESENTER_8K_BT 0x0701
> #define USB_DEVICE_ID_MS_PRESENTER_8K_USB 0x0713
> #define USB_DEVICE_ID_MS_DIGITAL_MEDIA_3K 0x0730
> +#define USB_DEVICE_ID_SIDEWINDER_X6 0x074b
> +#define USB_DEVICE_ID_SIDEWINDER_X4 0x0768
> #define USB_DEVICE_ID_MS_COMFORT_MOUSE_4500 0x076c
> #define USB_DEVICE_ID_MS_TOUCH_COVER_2 0x07a7
> #define USB_DEVICE_ID_MS_TYPE_COVER_2 0x07a9
> diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
> index 0a61403..5b5d40f 100644
> --- a/drivers/hid/hid-microsoft.c
> +++ b/drivers/hid/hid-microsoft.c
> @@ -29,12 +29,28 @@
> #define MS_NOGET 0x10
> #define MS_DUPLICATE_USAGES 0x20
> #define MS_RDESC_3K 0x40
> +#define MS_SIDEWINDER 0x80
>
> struct ms_data {
> unsigned long quirks;
> void *extra;
ok, so now extra is used, but with a struct ms_sidewinder_extra.
I do not think having a void pointer is a right choice here given that
there is only one possibility. So I'd rather have a direct struct
ms_sidewinder_extra pointer and the future will tell us if we need to
have a void pointer or not.
In fact, I'd rather not having to allocate twice during probe, so I
would even consider integrating struct ms_sidewinder_extra directly in
struct ms_data.
> };
>
> +/*
> + * For Sidewinder X4 / X6 devices.
> + * @profile: for storing profile status.
> + * @status: holds information about LED states and numpad mode (X6
> + * only). The 1st bit is for numpad mode, bits 2 - 7 are reserved for
> + * LED configuration and the last bit is currently unused.
> + * @key_mask: holds information about pressed special keys. It's
> + * readable via sysfs, so user-space tools can handle keys.
> + */
> +struct ms_sidewinder_extra {
> + unsigned profile;
> + __u8 status;
Both profile and status are not used in this patch.
Splitting a big patch into some smaller ones is good, but keep in mind
that we want to have patches that are self consistent and that do only
what they say.
Let's assume we revert in the future 3/4, what will happens with those
two fields which will not be used anymore?
> + unsigned long key_mask;
> +};
> +
> static __u8 *ms_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> unsigned int *rsize)
> {
> @@ -143,6 +159,56 @@ static int ms_presenter_8k_quirk(struct hid_input *hi, struct hid_usage *usage,
> return 1;
> }
>
> +static int ms_sidewinder_kb_quirk(struct hid_input *hi, struct hid_usage *usage,
> + unsigned long **bit, int *max)
> +{
> + set_bit(EV_REP, hi->input->evbit);
I think you should also test against the usage page. It looks like the
usages are specific enough, but we never know.
> + switch (usage->hid & HID_USAGE) {
> + /*
> + * Registering Sidewinder X4 / X6 special keys. S1 - S6 macro keys
> + * are shared between Sidewinder X4 & X6 and are programmable.
> + */
> + case 0xfb01: ms_map_key_clear(KEY_UNKNOWN); break; /* S1 */
> + case 0xfb02: ms_map_key_clear(KEY_UNKNOWN); break; /* S2 */
> + case 0xfb03: ms_map_key_clear(KEY_UNKNOWN); break; /* S3 */
> + case 0xfb04: ms_map_key_clear(KEY_UNKNOWN); break; /* S4 */
> + case 0xfb05: ms_map_key_clear(KEY_UNKNOWN); break; /* S5 */
> + case 0xfb06: ms_map_key_clear(KEY_UNKNOWN); break; /* S6 */
> + /* S7 - S30 macro keys are only present on the Sidewinder X6 */
> + case 0xfb07: ms_map_key_clear(KEY_UNKNOWN); break; /* S7 */
> + case 0xfb08: ms_map_key_clear(KEY_UNKNOWN); break; /* S8 */
> + case 0xfb09: ms_map_key_clear(KEY_UNKNOWN); break; /* S9 */
> + case 0xfb0a: ms_map_key_clear(KEY_UNKNOWN); break; /* S10 */
> + case 0xfb0b: ms_map_key_clear(KEY_UNKNOWN); break; /* S11 */
> + case 0xfb0c: ms_map_key_clear(KEY_UNKNOWN); break; /* S12 */
> + case 0xfb0d: ms_map_key_clear(KEY_UNKNOWN); break; /* S13 */
> + case 0xfb0e: ms_map_key_clear(KEY_UNKNOWN); break; /* S14 */
> + case 0xfb0f: ms_map_key_clear(KEY_UNKNOWN); break; /* S15 */
> + case 0xfb10: ms_map_key_clear(KEY_UNKNOWN); break; /* S16 */
> + case 0xfb11: ms_map_key_clear(KEY_UNKNOWN); break; /* S17 */
> + case 0xfb12: ms_map_key_clear(KEY_UNKNOWN); break; /* S18 */
> + case 0xfb13: ms_map_key_clear(KEY_UNKNOWN); break; /* S19 */
> + case 0xfb14: ms_map_key_clear(KEY_UNKNOWN); break; /* S20 */
> + case 0xfb15: ms_map_key_clear(KEY_UNKNOWN); break; /* S21 */
> + case 0xfb16: ms_map_key_clear(KEY_UNKNOWN); break; /* S22 */
> + case 0xfb17: ms_map_key_clear(KEY_UNKNOWN); break; /* S23 */
> + case 0xfb18: ms_map_key_clear(KEY_UNKNOWN); break; /* S24 */
> + case 0xfb19: ms_map_key_clear(KEY_UNKNOWN); break; /* S25 */
> + case 0xfb1a: ms_map_key_clear(KEY_UNKNOWN); break; /* S26 */
> + case 0xfb1b: ms_map_key_clear(KEY_UNKNOWN); break; /* S27 */
> + case 0xfb1c: ms_map_key_clear(KEY_UNKNOWN); break; /* S28 */
> + case 0xfb1d: ms_map_key_clear(KEY_UNKNOWN); break; /* S29 */
> + case 0xfb1e: ms_map_key_clear(KEY_UNKNOWN); break; /* S30 */
> + /* Not programmable keys: Profile, Game Center (X6 only) and Macro */
> + case 0xfd11: ms_map_key_clear(KEY_GAMES); break; /* X6: Game Center*/
> + case 0xfd12: ms_map_key_clear(KEY_MACRO); break; /* Macro */
> + case 0xfd15: ms_map_key_clear(KEY_UNKNOWN); break; /* Profile */
Arg, this is a little bit ugly.
you should either consider using fall-through between the each case or
a for loop like you did in ms_event.
> + default:
> + return 0;
> + }
> + return 1;
> +}
> +
> static int ms_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> struct hid_field *field, struct hid_usage *usage,
> unsigned long **bit, int *max)
> @@ -159,6 +225,10 @@ static int ms_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> ms_presenter_8k_quirk(hi, usage, bit, max))
> return 1;
>
> + if ((sc->quirks & MS_SIDEWINDER) &&
> + ms_sidewinder_kb_quirk(hi, usage, bit, max))
> + return 1;
> +
> return 0;
> }
>
> @@ -229,6 +299,34 @@ static int ms_event(struct hid_device *hdev, struct hid_field *field,
> return 1;
> }
>
> + /*
> + * Sidewinder special button handling & profile switching
> + *
> + * Pressing S1 - S30 macro keys will not send out any keycodes, but
> + * set bits on key_mask (readable via sysfs). It's possible to press
> + * multiple special keys at the same time.
> + */
> + if (sc->quirks & MS_SIDEWINDER) {
> + struct input_dev *input = field->hidinput->input;
> + struct ms_sidewinder_extra *sidewinder = sc->extra;
> + int i;
> +
> + for (i = 0; i <= 29; i++) { /* Run through S1 - S30 keys */
> + if ((usage->hid & HID_USAGE) == (0xfb01 + i)) {
> + value ? set_bit(i, &sidewinder->key_mask) : clear_bit(i, &sidewinder->key_mask);
> + break; /* Exit loop, when correct hid usage has been found */
> + }
> + }
> +
> + switch (usage->hid & HID_USAGE) {
> + case 0xfd11: input_event(input, usage->type, KEY_GAMES, value); break;
> + case 0xfd12: input_event(input, usage->type, KEY_MACRO, value); break;
> + case 0xfd15: value ? set_bit(30, &sidewinder->key_mask) : clear_bit(30, &sidewinder->key_mask); break;
I don't really like the "30" here. But I do not see an elegant way of
getting it.
> + }
> +
> + return 1;
> + }
> +
> return 0;
> }
>
> @@ -249,6 +347,18 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id)
> if (sc->quirks & MS_NOGET)
> hdev->quirks |= HID_QUIRK_NOGET;
>
> + if (sc->quirks & MS_SIDEWINDER) {
> + struct ms_sidewinder_extra *sidewinder;
> +
> + sidewinder = devm_kzalloc(&hdev->dev, sizeof(struct ms_sidewinder_extra),
> + GFP_KERNEL);
> + if (!sidewinder) {
> + hid_err(hdev, "can't alloc microsoft descriptor\n");
Hmm, this err message does not reflect the reality (it's the same as
the one used in 1/4).
Cheers,
Benjamin
> + return -ENOMEM;
> + }
> + sc->extra = sidewinder;
> + }
> +
> ret = hid_parse(hdev);
> if (ret) {
> hid_err(hdev, "parse failed\n");
> @@ -270,6 +380,10 @@ err_free:
> static const struct hid_device_id ms_devices[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_GV),
> .driver_data = MS_HIDINPUT },
> + { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_X6),
> + .driver_data = MS_SIDEWINDER },
> + { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_X4),
> + .driver_data = MS_SIDEWINDER },
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_OFFICE_KB),
> .driver_data = MS_ERGONOMY },
> { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_NE4K),
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH 1/4] HID: microsoft: moving quirks to struct
From: Benjamin Tissoires @ 2014-04-09 20:40 UTC (permalink / raw)
To: Tolga Cakir
Cc: Benjamin Tissoires, Derya, Jiri Kosina, Reyad Attiyat,
linux-input, linux-kernel@vger.kernel.org
In-Reply-To: <1396631173-11668-1-git-send-email-tolga@cevel.net>
On Fri, Apr 4, 2014 at 1:06 PM, Tolga Cakir <tolga@cevel.net> wrote:
> This will give us the opportunity to easily implement extra features
> for new devices.
>
> Signed-off-by: Tolga Cakir <tolga@cevel.net>
> ---
> drivers/hid/hid-microsoft.c | 44 ++++++++++++++++++++++++++++----------------
> 1 file changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
> index 6fd5817..0a61403 100644
> --- a/drivers/hid/hid-microsoft.c
> +++ b/drivers/hid/hid-microsoft.c
> @@ -30,23 +30,28 @@
> #define MS_DUPLICATE_USAGES 0x20
> #define MS_RDESC_3K 0x40
>
> +struct ms_data {
> + unsigned long quirks;
> + void *extra;
I do not like having a void * pointer in this kind of struct,
especially when it is not used in the current patch.
Please remove it in v2
> +};
> +
> static __u8 *ms_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> unsigned int *rsize)
> {
> - unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
> + struct ms_data *sc = hid_get_drvdata(hdev);
>
> /*
> * Microsoft Wireless Desktop Receiver (Model 1028) has
> * 'Usage Min/Max' where it ought to have 'Physical Min/Max'
> */
> - if ((quirks & MS_RDESC) && *rsize == 571 && rdesc[557] == 0x19 &&
> + if ((sc->quirks & MS_RDESC) && *rsize == 571 && rdesc[557] == 0x19 &&
> rdesc[559] == 0x29) {
> hid_info(hdev, "fixing up Microsoft Wireless Receiver Model 1028 report descriptor\n");
> rdesc[557] = 0x35;
> rdesc[559] = 0x45;
> }
> /* the same as above (s/usage/physical/) */
> - if ((quirks & MS_RDESC_3K) && *rsize == 106 && rdesc[94] == 0x19 &&
> + if ((sc->quirks & MS_RDESC_3K) && *rsize == 106 && rdesc[94] == 0x19 &&
> rdesc[95] == 0x00 && rdesc[96] == 0x29 &&
> rdesc[97] == 0xff) {
> rdesc[94] = 0x35;
> @@ -142,15 +147,15 @@ static int ms_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> struct hid_field *field, struct hid_usage *usage,
> unsigned long **bit, int *max)
> {
> - unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
> + struct ms_data *sc = hid_get_drvdata(hdev);
>
> - if (quirks & MS_ERGONOMY) {
> + if (sc->quirks & MS_ERGONOMY) {
> int ret = ms_ergonomy_kb_quirk(hi, usage, bit, max);
> if (ret)
> return ret;
> }
>
> - if ((quirks & MS_PRESENTER) &&
> + if ((sc->quirks & MS_PRESENTER) &&
> ms_presenter_8k_quirk(hi, usage, bit, max))
> return 1;
>
> @@ -161,9 +166,9 @@ static int ms_input_mapped(struct hid_device *hdev, struct hid_input *hi,
> struct hid_field *field, struct hid_usage *usage,
> unsigned long **bit, int *max)
> {
> - unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
> + struct ms_data *sc = hid_get_drvdata(hdev);
>
> - if (quirks & MS_DUPLICATE_USAGES)
> + if (sc->quirks & MS_DUPLICATE_USAGES)
> clear_bit(usage->code, *bit);
>
> return 0;
> @@ -172,7 +177,7 @@ static int ms_input_mapped(struct hid_device *hdev, struct hid_input *hi,
> static int ms_event(struct hid_device *hdev, struct hid_field *field,
> struct hid_usage *usage, __s32 value)
> {
> - unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
> + struct ms_data *sc = hid_get_drvdata(hdev);
> struct input_dev *input;
>
> if (!(hdev->claimed & HID_CLAIMED_INPUT) || !field->hidinput ||
> @@ -182,7 +187,7 @@ static int ms_event(struct hid_device *hdev, struct hid_field *field,
> input = field->hidinput->input;
>
> /* Handling MS keyboards special buttons */
> - if (quirks & MS_ERGONOMY && usage->hid == (HID_UP_MSVENDOR | 0xff00)) {
> + if (sc->quirks & MS_ERGONOMY && usage->hid == (HID_UP_MSVENDOR | 0xff00)) {
> /* Special keypad keys */
> input_report_key(input, KEY_KPEQUAL, value & 0x01);
> input_report_key(input, KEY_KPLEFTPAREN, value & 0x02);
> @@ -190,7 +195,7 @@ static int ms_event(struct hid_device *hdev, struct hid_field *field,
> return 1;
> }
>
> - if (quirks & MS_ERGONOMY && usage->hid == (HID_UP_MSVENDOR | 0xff01)) {
> + if (sc->quirks & MS_ERGONOMY && usage->hid == (HID_UP_MSVENDOR | 0xff01)) {
> /* Scroll wheel */
> int step = ((value & 0x60) >> 5) + 1;
>
> @@ -205,7 +210,7 @@ static int ms_event(struct hid_device *hdev, struct hid_field *field,
> return 1;
> }
>
> - if (quirks & MS_ERGONOMY && usage->hid == (HID_UP_MSVENDOR | 0xff05)) {
> + if (sc->quirks & MS_ERGONOMY && usage->hid == (HID_UP_MSVENDOR | 0xff05)) {
> static unsigned int last_key = 0;
> unsigned int key = 0;
> switch (value) {
> @@ -229,12 +234,19 @@ static int ms_event(struct hid_device *hdev, struct hid_field *field,
>
> static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id)
> {
> - unsigned long quirks = id->driver_data;
> + struct ms_data *sc;
> int ret;
>
> - hid_set_drvdata(hdev, (void *)quirks);
> + sc = devm_kzalloc(&hdev->dev, sizeof(struct ms_data), GFP_KERNEL);
> + if (!sc) {
> + hid_err(hdev, "can't alloc microsoft descriptor\n");
How about: "Can not allocate microsoft data"?
I have no idea where this "descriptor" comes from.
Besides these two nitpicks, the rest of the patch is:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
> + return -ENOMEM;
> + }
> +
> + sc->quirks = id->driver_data;
> + hid_set_drvdata(hdev, sc);
>
> - if (quirks & MS_NOGET)
> + if (sc->quirks & MS_NOGET)
> hdev->quirks |= HID_QUIRK_NOGET;
>
> ret = hid_parse(hdev);
> @@ -243,7 +255,7 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id)
> goto err_free;
> }
>
> - ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT | ((quirks & MS_HIDINPUT) ?
> + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT | ((sc->quirks & MS_HIDINPUT) ?
> HID_CONNECT_HIDINPUT_FORCE : 0));
> if (ret) {
> hid_err(hdev, "hw start failed\n");
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH resend 2/2] input/serio/8042: Add firmware_id support
From: Dmitry Torokhov @ 2014-04-09 20:09 UTC (permalink / raw)
To: Matthew Garrett
Cc: Hans de Goede, Benjamin Tissoires, Peter Hutterer, linux-input
In-Reply-To: <20140409182926.GB3196@srcf.ucam.org>
On Wed, Apr 09, 2014 at 07:29:26PM +0100, Matthew Garrett wrote:
> On Wed, Apr 09, 2014 at 11:24:34AM -0700, Dmitry Torokhov wrote:
>
> > Do we need all IDs? I'd expect we only interested in HID, not CIDs?
>
> I think HID handles the cases we've seen so far, but we could imagine a
> system vendor providing their own HID, a trackpad vendor's CID and then
> the generic mouse CID. It seems better to err on the side of including
> them.
OK, fair enough. Another question - do we want to prefix IDs with "PNP:"
prefix so that if we add device tree in the future we'll know what kind
of IDs we are dealing with?
--
Dmitry
^ permalink raw reply
* Re: [PATCH resend 2/2] input/serio/8042: Add firmware_id support
From: Matthew Garrett @ 2014-04-09 18:29 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Hans de Goede, Benjamin Tissoires, Peter Hutterer, linux-input
In-Reply-To: <20140409182434.GA25308@core.coreip.homeip.net>
On Wed, Apr 09, 2014 at 11:24:34AM -0700, Dmitry Torokhov wrote:
> Do we need all IDs? I'd expect we only interested in HID, not CIDs?
I think HID handles the cases we've seen so far, but we could imagine a
system vendor providing their own HID, a trackpad vendor's CID and then
the generic mouse CID. It seems better to err on the side of including
them.
--
Matthew Garrett | mjg59@srcf.ucam.org
^ permalink raw reply
* Re: [PATCH resend 2/2] input/serio/8042: Add firmware_id support
From: Dmitry Torokhov @ 2014-04-09 18:24 UTC (permalink / raw)
To: Hans de Goede
Cc: Matthew Garrett, Benjamin Tissoires, Peter Hutterer, linux-input
In-Reply-To: <1397033270-29597-3-git-send-email-hdegoede@redhat.com>
On Wed, Apr 09, 2014 at 10:47:50AM +0200, Hans de Goede wrote:
> Fill in the new serio firmware_id sysfs attribute for pnp instantiated
> 8042 serio ports.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Peter Hutterer <peter.hutterer@who-t.net>
> ---
> drivers/input/serio/i8042-x86ia64io.h | 26 ++++++++++++++++++++++++++
> drivers/input/serio/i8042.c | 6 ++++++
> 2 files changed, 32 insertions(+)
>
> diff --git a/drivers/input/serio/i8042-x86ia64io.h b/drivers/input/serio/i8042-x86ia64io.h
> index 0ec9abb..3f9da83 100644
> --- a/drivers/input/serio/i8042-x86ia64io.h
> +++ b/drivers/input/serio/i8042-x86ia64io.h
> @@ -704,6 +704,8 @@ static char i8042_pnp_aux_name[32];
>
> static int i8042_pnp_kbd_probe(struct pnp_dev *dev, const struct pnp_device_id *did)
> {
> + struct pnp_id *id = dev->id;
> +
> if (pnp_port_valid(dev, 0) && pnp_port_len(dev, 0) == 1)
> i8042_pnp_data_reg = pnp_port_start(dev,0);
>
> @@ -719,6 +721,17 @@ static int i8042_pnp_kbd_probe(struct pnp_dev *dev, const struct pnp_device_id *
> strlcat(i8042_pnp_kbd_name, pnp_dev_name(dev), sizeof(i8042_pnp_kbd_name));
> }
>
> + if (id) {
> + strlcpy(i8042_kbd_firmware_id, id->id,
> + sizeof(i8042_kbd_firmware_id));
> + for (id = id->next; id; id = id->next) {
> + strlcat(i8042_kbd_firmware_id, " ",
> + sizeof(i8042_kbd_firmware_id));
> + strlcat(i8042_kbd_firmware_id, id->id,
> + sizeof(i8042_kbd_firmware_id));
Do we need all IDs? I'd expect we only interested in HID, not CIDs?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 3/3] Input: synaptics-rmi4 - report sensor resolution
From: Benjamin Tissoires @ 2014-04-09 18:21 UTC (permalink / raw)
To: Christopher Heiny
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Linus Walleij, Benjamin Tissoires, David Herrmann,
Jiri Kosina
In-Reply-To: <1396300267-5108-3-git-send-email-cheiny@synaptics.com>
On Mon, Mar 31, 2014 at 5:11 PM, Christopher Heiny <cheiny@synaptics.com> wrote:
> Reports the sensor resolution by reading the size of the sensor
> from F11 query registers or from the platform data if the firmware
> does not contain the appropriate query registers.
>
> Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
> Acked-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Linux Walleij <linus.walleij@linaro.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
>
> ---
Looks fine to me:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
> drivers/input/rmi4/rmi_f11.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
> include/linux/rmi.h | 2 ++
> 2 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/rmi4/rmi_f11.c b/drivers/input/rmi4/rmi_f11.c
> index ee47b7e..9c682f0 100644
> --- a/drivers/input/rmi4/rmi_f11.c
> +++ b/drivers/input/rmi4/rmi_f11.c
> @@ -408,6 +408,10 @@ struct f11_2d_sensor_queries {
> u8 clickpad_props;
> u8 mouse_buttons;
> bool has_advanced_gestures;
> +
> + /* Query 15 - 18 */
> + u16 x_sensor_size_mm;
> + u16 y_sensor_size_mm;
> };
>
> /* Defs for Ctrl0. */
> @@ -518,6 +522,8 @@ struct f11_2d_sensor {
> char input_phys[NAME_BUFFER_SIZE];
> u8 report_abs;
> u8 report_rel;
> + u8 x_mm;
> + u8 y_mm;
> };
>
> /** Data pertaining to F11 in general. For per-sensor data, see struct
> @@ -1064,7 +1070,7 @@ static int rmi_f11_get_query_parameters(struct rmi_device *rmi_dev,
> query_size++;
> }
>
> - if (f11->has_query12 && sensor_query->has_info2) {
> + if (sensor_query->has_info2) {
> rc = rmi_read(rmi_dev, query_base_addr + query_size, query_buf);
> if (rc < 0)
> return rc;
> @@ -1085,6 +1091,20 @@ static int rmi_f11_get_query_parameters(struct rmi_device *rmi_dev,
> query_size++;
> }
>
> + if (sensor_query->has_physical_props) {
> + rc = rmi_read_block(rmi_dev, query_base_addr
> + + query_size, query_buf, 4);
> + if (rc < 0)
> + return rc;
> +
> + sensor_query->x_sensor_size_mm =
> + (query_buf[0] | (query_buf[1] << 8)) / 10;
> + sensor_query->y_sensor_size_mm =
> + (query_buf[2] | (query_buf[3] << 8)) / 10;
> +
> + query_size += 4;
> + }
> +
> return query_size;
> }
>
> @@ -1106,6 +1126,7 @@ static void f11_set_abs_params(struct rmi_function *fn, struct f11_data *f11)
> ((f11->dev_controls.ctrl0_9[9] & 0x0F) << 8);
> u16 x_min, x_max, y_min, y_max;
> unsigned int input_flags;
> + int res_x, res_y;
>
> /* We assume touchscreen unless demonstrably a touchpad or specified
> * as a touchpad in the platform data
> @@ -1156,6 +1177,18 @@ static void f11_set_abs_params(struct rmi_function *fn, struct f11_data *f11)
> x_min, x_max, 0, 0);
> input_set_abs_params(input, ABS_MT_POSITION_Y,
> y_min, y_max, 0, 0);
> +
> + if (sensor->x_mm && sensor->y_mm) {
> + res_x = (x_max - x_min) / sensor->x_mm;
> + res_y = (y_max - y_min) / sensor->y_mm;
> +
> + input_abs_set_res(input, ABS_X, res_x);
> + input_abs_set_res(input, ABS_Y, res_y);
> +
> + input_abs_set_res(input, ABS_MT_POSITION_X, res_x);
> + input_abs_set_res(input, ABS_MT_POSITION_Y, res_y);
> + }
> +
> if (!sensor->type_a)
> input_mt_init_slots(input, sensor->nbr_fingers, input_flags);
> if (IS_ENABLED(CONFIG_RMI4_F11_PEN) && sensor->sens_query.has_pen)
> @@ -1252,6 +1285,14 @@ static int rmi_f11_initialize(struct rmi_function *fn)
> pdata->f11_sensor_data->axis_align;
> sensor->type_a = pdata->f11_sensor_data->type_a;
>
> + if (sensor->sens_query.has_physical_props) {
> + sensor->x_mm = sensor->sens_query.x_sensor_size_mm;
> + sensor->y_mm = sensor->sens_query.y_sensor_size_mm;
> + } else if (pdata->f11_sensor_data) {
> + sensor->x_mm = pdata->f11_sensor_data->x_mm;
> + sensor->y_mm = pdata->f11_sensor_data->y_mm;
> + }
> +
> if (sensor->sensor_type == rmi_f11_sensor_default)
> sensor->sensor_type =
> pdata->f11_sensor_data->sensor_type;
> diff --git a/include/linux/rmi.h b/include/linux/rmi.h
> index 164b813..ca35b2f 100644
> --- a/include/linux/rmi.h
> +++ b/include/linux/rmi.h
> @@ -95,6 +95,8 @@ struct rmi_f11_sensor_data {
> struct rmi_f11_2d_axis_alignment axis_align;
> bool type_a;
> enum rmi_f11_sensor_type sensor_type;
> + int x_mm;
> + int y_mm;
> int disable_report_mask;
> };
>
> --
> 1.8.3.2
>
> --
> 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 v2 2/3] Input: synaptics-rmi4 - F11 abs or rel reporting
From: Benjamin Tissoires @ 2014-04-09 18:19 UTC (permalink / raw)
To: Christopher Heiny
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Linus Walleij, Benjamin Tissoires, David Herrmann,
Jiri Kosina
In-Reply-To: <1396300267-5108-2-git-send-email-cheiny@synaptics.com>
On Mon, Mar 31, 2014 at 5:11 PM, Christopher Heiny <cheiny@synaptics.com> wrote:
> If the firmware provides reporting both relative and absolute
> coordinates, reporting both can cause userspace confusion, so
> default to reporting only the absolute coordinates. In certain
> cases in may be desirable to force only reporting relative
> coordinates so a mask is provided in the platform data for
> disabling absolute reporting and falling back to reporting
> relative coordinates.
>
>
> Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
> Acked-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Linux Walleij <linus.walleij@linaro.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
>
> ---
> drivers/input/rmi4/rmi_f11.c | 122 +++++++++++++++++++++----------------------
> include/linux/rmi.h | 5 ++
> 2 files changed, 64 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_f11.c b/drivers/input/rmi4/rmi_f11.c
> index e98fa75..ee47b7e 100644
> --- a/drivers/input/rmi4/rmi_f11.c
> +++ b/drivers/input/rmi4/rmi_f11.c
> @@ -499,9 +499,7 @@ struct f11_2d_data {
> * assume we have one of those sensors and report events appropriately.
> * @sensor_type - indicates whether we're touchscreen or touchpad.
> * @input - input device for absolute pointing stream
> - * @mouse_input - input device for relative pointing stream.
> * @input_phys - buffer for the absolute phys name for this sensor.
> - * @input_phys_mouse - buffer for the relative phys name for this sensor.
> */
> struct f11_2d_sensor {
> struct rmi_f11_2d_axis_alignment axis_align;
> @@ -516,10 +514,10 @@ struct f11_2d_sensor {
> u32 type_a; /* boolean but debugfs API requires u32 */
> enum rmi_f11_sensor_type sensor_type;
> struct input_dev *input;
> - struct input_dev *mouse_input;
> struct rmi_function *fn;
> char input_phys[NAME_BUFFER_SIZE];
> - char input_phys_mouse[NAME_BUFFER_SIZE];
> + u8 report_abs;
> + u8 report_rel;
> };
>
> /** Data pertaining to F11 in general. For per-sensor data, see struct
> @@ -544,6 +542,9 @@ struct f11_data {
> struct mutex dev_controls_mutex;
> u16 rezero_wait_ms;
> struct f11_2d_sensor sensor;
> + unsigned long *abs_mask;
> + unsigned long *rel_mask;
> + unsigned long *result_bits;
> };
>
> enum finger_state_values {
> @@ -591,10 +592,7 @@ static void rmi_f11_rel_pos_report(struct f11_2d_sensor *sensor, u8 n_finger)
> if (x || y) {
> input_report_rel(sensor->input, REL_X, x);
> input_report_rel(sensor->input, REL_Y, y);
> - input_report_rel(sensor->mouse_input, REL_X, x);
> - input_report_rel(sensor->mouse_input, REL_Y, y);
> }
> - input_sync(sensor->mouse_input);
> }
>
> static void rmi_f11_abs_pos_report(struct f11_data *f11,
> @@ -691,13 +689,17 @@ static void rmi_f11_abs_pos_report(struct f11_data *f11,
> }
>
> static void rmi_f11_finger_handler(struct f11_data *f11,
> - struct f11_2d_sensor *sensor)
> + struct f11_2d_sensor *sensor,
> + unsigned long *irq_bits, int num_irq_regs)
> {
> const u8 *f_state = sensor->data.f_state;
> u8 finger_state;
> u8 finger_pressed_count;
> u8 i;
>
> + int rel_bits;
> + int abs_bits;
> +
> for (i = 0, finger_pressed_count = 0; i < sensor->nbr_fingers; i++) {
> /* Possible of having 4 fingers per f_statet register */
> finger_state = (f_state[i / 4] >> (2 * (i % 4))) &
> @@ -711,10 +713,14 @@ static void rmi_f11_finger_handler(struct f11_data *f11,
> finger_pressed_count++;
> }
>
> - if (sensor->data.abs_pos)
> + abs_bits = bitmap_and(f11->result_bits, irq_bits, f11->abs_mask,
> + num_irq_regs);
> + if (abs_bits)
I am finding this bitmask gymnastic quite difficult to follow.
I am also wondering if the following test will give the same result:
"if (sensor->data.abs_pos && sensor->report_abs)"
> rmi_f11_abs_pos_report(f11, sensor, finger_state, i);
>
> - if (sensor->data.rel_pos)
> + rel_bits = bitmap_and(f11->result_bits, irq_bits, f11->rel_mask,
> + num_irq_regs);
> + if (rel_bits)
Same for rel bits here
The rest of the patch looks fine to me (except that if my previous
assumptions are right, ew could remove the whole bitmask alloc and
handling).
Cheers,
Benjamin
> rmi_f11_rel_pos_report(sensor, i);
> }
> input_mt_sync_frame(sensor->input);
> @@ -1171,21 +1177,36 @@ static int rmi_f11_initialize(struct rmi_function *fn)
> u16 max_x_pos, max_y_pos, temp;
> int rc;
> const struct rmi_device_platform_data *pdata = rmi_get_platform_data(rmi_dev);
> + struct rmi_driver_data *drvdata = dev_get_drvdata(&rmi_dev->dev);
> struct f11_2d_sensor *sensor;
> u8 buf;
> + int mask_size;
>
> dev_dbg(&fn->dev, "Initializing F11 values for %s.\n",
> pdata->sensor_name);
>
> + mask_size = BITS_TO_LONGS(drvdata->irq_count) * sizeof(unsigned long);
> +
> /*
> ** init instance data, fill in values and create any sysfs files
> */
> - f11 = devm_kzalloc(&fn->dev, sizeof(struct f11_data), GFP_KERNEL);
> + f11 = devm_kzalloc(&fn->dev, sizeof(struct f11_data) + mask_size * 3,
> + GFP_KERNEL);
> if (!f11)
> return -ENOMEM;
>
> f11->rezero_wait_ms = pdata->f11_rezero_wait;
>
> + f11->abs_mask = (unsigned long*)((char *)f11
> + + sizeof(struct f11_data));
> + f11->rel_mask = (unsigned long*)((char *)f11
> + + sizeof(struct f11_data) + mask_size);
> + f11->result_bits = (unsigned long*)((char *)f11
> + + sizeof(struct f11_data) + mask_size * 2);
> +
> + set_bit(fn->irq_pos, f11->abs_mask);
> + set_bit(fn->irq_pos + 1, f11->rel_mask);
> +
> query_base_addr = fn->fd.query_base_addr;
> control_base_addr = fn->fd.control_base_addr;
>
> @@ -1224,6 +1245,8 @@ static int rmi_f11_initialize(struct rmi_function *fn)
> sensor->sensor_type = rmi_f11_sensor_touchpad;
> }
>
> + sensor->report_abs = sensor->sens_query.has_abs;
> +
> if (pdata->f11_sensor_data) {
> sensor->axis_align =
> pdata->f11_sensor_data->axis_align;
> @@ -1232,8 +1255,19 @@ static int rmi_f11_initialize(struct rmi_function *fn)
> if (sensor->sensor_type == rmi_f11_sensor_default)
> sensor->sensor_type =
> pdata->f11_sensor_data->sensor_type;
> +
> + sensor->report_abs = sensor->report_abs
> + && !(pdata->f11_sensor_data->disable_report_mask
> + & RMI_F11_DISABLE_ABS_REPORT);
> }
>
> + if (!sensor->report_abs)
> + /*
> + * If device doesn't have abs or if it has been disables
> + * fallback to reporting rel data.
> + */
> + sensor->report_rel = sensor->sens_query.has_rel;
> +
> rc = rmi_read_block(rmi_dev,
> control_base_addr + F11_CTRL_SENSOR_MAX_X_POS_OFFSET,
> (u8 *)&max_x_pos, sizeof(max_x_pos));
> @@ -1293,7 +1327,6 @@ static int rmi_f11_register_devices(struct rmi_function *fn)
> struct rmi_device *rmi_dev = fn->rmi_dev;
> struct f11_data *f11 = dev_get_drvdata(&fn->dev);
> struct input_dev *input_dev;
> - struct input_dev *input_dev_mouse;
> struct rmi_driver *driver = rmi_dev->driver;
> struct f11_2d_sensor *sensor = &f11->sensor;
> int rc;
> @@ -1324,9 +1357,10 @@ static int rmi_f11_register_devices(struct rmi_function *fn)
> set_bit(EV_ABS, input_dev->evbit);
> input_set_capability(input_dev, EV_KEY, BTN_TOUCH);
>
> - f11_set_abs_params(fn, f11);
> + if (sensor->report_abs)
> + f11_set_abs_params(fn, f11);
>
> - if (sensor->sens_query.has_rel) {
> + if (sensor->report_rel) {
> set_bit(EV_REL, input_dev->evbit);
> set_bit(REL_X, input_dev->relbit);
> set_bit(REL_Y, input_dev->relbit);
> @@ -1338,56 +1372,10 @@ static int rmi_f11_register_devices(struct rmi_function *fn)
> goto error_unregister;
> }
>
> - if (sensor->sens_query.has_rel) {
> - /*create input device for mouse events */
> - input_dev_mouse = input_allocate_device();
> - if (!input_dev_mouse) {
> - rc = -ENOMEM;
> - goto error_unregister;
> - }
> -
> - sensor->mouse_input = input_dev_mouse;
> - if (driver->set_input_params) {
> - rc = driver->set_input_params(rmi_dev,
> - input_dev_mouse);
> - if (rc < 0) {
> - dev_err(&fn->dev,
> - "%s: Error in setting input device.\n",
> - __func__);
> - goto error_unregister;
> - }
> - }
> - sprintf(sensor->input_phys_mouse, "%s.rel/input0",
> - dev_name(&fn->dev));
> - set_bit(EV_REL, input_dev_mouse->evbit);
> - set_bit(REL_X, input_dev_mouse->relbit);
> - set_bit(REL_Y, input_dev_mouse->relbit);
> -
> - set_bit(BTN_MOUSE, input_dev_mouse->evbit);
> - /* Register device's buttons and keys */
> - set_bit(EV_KEY, input_dev_mouse->evbit);
> - set_bit(BTN_LEFT, input_dev_mouse->keybit);
> - set_bit(BTN_MIDDLE, input_dev_mouse->keybit);
> - set_bit(BTN_RIGHT, input_dev_mouse->keybit);
> -
> - rc = input_register_device(input_dev_mouse);
> - if (rc) {
> - input_free_device(input_dev_mouse);
> - sensor->mouse_input = NULL;
> - goto error_unregister;
> - }
> -
> - set_bit(BTN_RIGHT, input_dev_mouse->keybit);
> - }
> -
> return 0;
>
> error_unregister:
> if (f11->sensor.input) {
> - if (f11->sensor.mouse_input) {
> - input_unregister_device(f11->sensor.mouse_input);
> - f11->sensor.mouse_input = NULL;
> - }
> input_unregister_device(f11->sensor.input);
> f11->sensor.input = NULL;
> }
> @@ -1398,8 +1386,16 @@ error_unregister:
> static int rmi_f11_config(struct rmi_function *fn)
> {
> struct f11_data *f11 = dev_get_drvdata(&fn->dev);
> + struct rmi_driver *drv = fn->rmi_dev->driver;
> + struct f11_2d_sensor *sensor = &f11->sensor;
> int rc;
>
> + if (!sensor->report_abs)
> + drv->clear_irq_bits(fn->rmi_dev, f11->abs_mask);
> +
> + if (!sensor->report_rel)
> + drv->clear_irq_bits(fn->rmi_dev, f11->rel_mask);
> +
> rc = f11_write_control_regs(fn, &f11->sensor.sens_query,
> &f11->dev_controls, fn->fd.query_base_addr);
> if (rc < 0)
> @@ -1411,6 +1407,7 @@ static int rmi_f11_config(struct rmi_function *fn)
> static int rmi_f11_attention(struct rmi_function *fn, unsigned long *irq_bits)
> {
> struct rmi_device *rmi_dev = fn->rmi_dev;
> + struct rmi_driver_data *drvdata = dev_get_drvdata(&rmi_dev->dev);
> struct f11_data *f11 = dev_get_drvdata(&fn->dev);
> u16 data_base_addr = fn->fd.data_base_addr;
> u16 data_base_addr_offset = 0;
> @@ -1423,7 +1420,8 @@ static int rmi_f11_attention(struct rmi_function *fn, unsigned long *irq_bits)
> if (error)
> return error;
>
> - rmi_f11_finger_handler(f11, &f11->sensor);
> + rmi_f11_finger_handler(f11, &f11->sensor, irq_bits,
> + drvdata->num_of_irq_regs);
> data_base_addr_offset += f11->sensor.pkt_size;
>
> return 0;
> @@ -1477,8 +1475,6 @@ static void rmi_f11_remove(struct rmi_function *fn)
>
> if (f11->sensor.input)
> input_unregister_device(f11->sensor.input);
> - if (f11->sensor.mouse_input)
> - input_unregister_device(f11->sensor.mouse_input);
> }
>
> static struct rmi_function_handler rmi_f11_handler = {
> diff --git a/include/linux/rmi.h b/include/linux/rmi.h
> index 735e978..164b813 100644
> --- a/include/linux/rmi.h
> +++ b/include/linux/rmi.h
> @@ -76,6 +76,8 @@ enum rmi_f11_sensor_type {
> rmi_f11_sensor_touchpad
> };
>
> +#define RMI_F11_DISABLE_ABS_REPORT BIT(0)
> +
> /**
> * struct rmi_f11_sensor_data - overrides defaults for a single F11 2D sensor.
> * @axis_align - provides axis alignment overrides (see above).
> @@ -86,11 +88,14 @@ enum rmi_f11_sensor_type {
> * pointing device (touchpad) rather than a direct pointing device
> * (touchscreen). This is useful when F11_2D_QUERY14 register is not
> * available.
> + * @disable_report_mask - Force data to not be reported even if it is supported
> + * by the firware.
> */
> struct rmi_f11_sensor_data {
> struct rmi_f11_2d_axis_alignment axis_align;
> bool type_a;
> enum rmi_f11_sensor_type sensor_type;
> + int disable_report_mask;
> };
>
> /**
> --
> 1.8.3.2
>
> --
> 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 v2 1/3] Input: synaptics-rmi4 - add F11 capabilities for touchpads
From: Benjamin Tissoires @ 2014-04-09 18:13 UTC (permalink / raw)
To: Christopher Heiny
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Linus Walleij, Benjamin Tissoires, David Herrmann,
Jiri Kosina
In-Reply-To: <1396300267-5108-1-git-send-email-cheiny@synaptics.com>
On Mon, Mar 31, 2014 at 5:11 PM, Christopher Heiny <cheiny@synaptics.com> wrote:
> When the device is a touchpad additional capabilities need to
> be set and reported.
>
> Signed-off-by: Andrew Duggan <aduggan@synaptics.com>
> Acked-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Linux Walleij <linus.walleij@linaro.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
>
> ---
> drivers/input/rmi4/rmi_f11.c | 23 +++++++++++++++--------
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_f11.c b/drivers/input/rmi4/rmi_f11.c
> index 8709abe..92eac8a 100644
> --- a/drivers/input/rmi4/rmi_f11.c
> +++ b/drivers/input/rmi4/rmi_f11.c
> @@ -717,7 +717,7 @@ static void rmi_f11_finger_handler(struct f11_data *f11,
> if (sensor->data.rel_pos)
> rmi_f11_rel_pos_report(sensor, i);
> }
> - input_mt_sync(sensor->input);
> + input_mt_sync_frame(sensor->input);
Small nitpicking here: I think we should add an if (!sensor->type_a)
before this statement.
However, it should still be harmless for protocol A device because
input->mt is null.
So Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> for this one.
Also I wanted to apologize regarding the rebase I asked for in the v1,
it looks like I hallucinated because the synaptics-rmi4 branch is
plugged to a v3.7-rc+ and contains the multitouch changes I said that
were not in.
Cheers,
Benjamin
> input_sync(sensor->input);
> }
>
> @@ -1104,13 +1104,10 @@ static void f11_set_abs_params(struct rmi_function *fn, struct f11_data *f11)
> /* We assume touchscreen unless demonstrably a touchpad or specified
> * as a touchpad in the platform data
> */
> - if (sensor->sensor_type == rmi_f11_sensor_touchpad ||
> - (sensor->sens_query.has_info2 &&
> - !sensor->sens_query.is_clear))
> - input_flags = INPUT_PROP_POINTER;
> + if (sensor->sensor_type == rmi_f11_sensor_touchpad)
> + input_flags = INPUT_MT_POINTER;
> else
> - input_flags = INPUT_PROP_DIRECT;
> - set_bit(input_flags, input->propbit);
> + input_flags = INPUT_MT_DIRECT;
>
> if (sensor->axis_align.swap_axes) {
> int temp = device_x_max;
> @@ -1220,11 +1217,20 @@ static int rmi_f11_initialize(struct rmi_function *fn)
> return rc;
> }
>
> + if (sensor->sens_query.has_info2) {
> + if (sensor->sens_query.is_clear)
> + sensor->sensor_type = rmi_f11_sensor_touchscreen;
> + else
> + sensor->sensor_type = rmi_f11_sensor_touchpad;
> + }
> +
> if (pdata->f11_sensor_data) {
> sensor->axis_align =
> pdata->f11_sensor_data->axis_align;
> sensor->type_a = pdata->f11_sensor_data->type_a;
> - sensor->sensor_type =
> +
> + if (sensor->sensor_type == rmi_f11_sensor_default)
> + sensor->sensor_type =
> pdata->f11_sensor_data->sensor_type;
> }
>
> @@ -1490,6 +1496,7 @@ static struct rmi_function_handler rmi_f11_handler = {
> module_rmi_driver(rmi_f11_handler);
>
> MODULE_AUTHOR("Christopher Heiny <cheiny@synaptics.com");
> +MODULE_AUTHOR("Andrew Duggan <aduggan@synaptics.com");
> MODULE_DESCRIPTION("RMI F11 module");
> MODULE_LICENSE("GPL");
> MODULE_VERSION(RMI_DRIVER_VERSION);
> --
> 1.8.3.2
>
> --
> 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 2/4] serio: Add pnp_id to struct serio
From: Dmitry Torokhov @ 2014-04-09 18:06 UTC (permalink / raw)
To: Hans de Goede; +Cc: Benjamin Tissoires, Peter Hutterer, linux-input
In-Reply-To: <1397052181-22768-2-git-send-email-hdegoede@redhat.com>
Hi Hans,
On Wed, Apr 09, 2014 at 04:02:59PM +0200, Hans de Goede wrote:
> Serio device drivers need access to the pnp_id of the serio port, windows
> drivers bind by the pnp-id and some drivers need to know the exact pnp-id
> used so they know exactly with which hardware model / revision they are
> dealing with.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> include/linux/serio.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/linux/serio.h b/include/linux/serio.h
> index 9f779c7..6532440 100644
> --- a/include/linux/serio.h
> +++ b/include/linux/serio.h
> @@ -16,6 +16,7 @@
> #include <linux/mutex.h>
> #include <linux/device.h>
> #include <linux/mod_devicetable.h>
> +#include <linux/pnp.h>
> #include <uapi/linux/serio.h>
>
> struct serio {
> @@ -28,6 +29,7 @@ struct serio {
> bool manual_bind;
>
> struct serio_device_id id;
> + struct pnp_id *pnp_id;
Why do we need this if we are already adding generic 'firmware_id'
attribute?
Thanks.
--
Dmitry
^ 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