I tried to add support for the K40 some time ago, but the vendor specific USB protocol became over-complicated because of a lot of small differences between the K90 and the K40. Also, since I wrote the first version of this driver, I learned that USB control transfers could be done from user-space without the need to detach the kernel driver (please tell me if I am wrong).
So, I decided to move all USB related features in user-space (as far as I know, I was the only user, but if someone is looking for a replacement, I wrote a small tool available here: https://github.com/cvuchener/corsair-usb-config). This simplification only leaves the usage code remapping part and the driver no longer depends on USB and LED subsystems. This should make the driver easier to maintain or to add new supported devices.
After the removal of USB related functions in first patch, the addition of K40 support in the second patch is simply a matter of adding the device in the id list.
Cl?ment Vuchener (2):
HID: corsair: Remove all features using the USB protocol
HID: corsair: Add K40 support
Documentation/ABI/testing/sysfs-driver-hid-corsair | 15 -
drivers/hid/Kconfig | 2 +-
drivers/hid/hid-core.c | 1 +
drivers/hid/hid-corsair.c | 498 +--------------------
drivers/hid/hid-ids.h | 1 +
5 files changed, 5 insertions(+), 512 deletions(-)
delete mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair
--
2.5.5
Remove every use of USB control requests since it can be more easily done in user space. This removes the dependency on USB and LED subsystems. The simplyfied driver now only remaps Corsair usage codes.
Signed-off-by: Cl?ment Vuchener <[email protected]>
---
Documentation/ABI/testing/sysfs-driver-hid-corsair | 15 -
drivers/hid/Kconfig | 2 +-
drivers/hid/hid-corsair.c | 497 +--------------------
3 files changed, 2 insertions(+), 512 deletions(-)
delete mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair
diff --git a/Documentation/ABI/testing/sysfs-driver-hid-corsair b/Documentation/ABI/testing/sysfs-driver-hid-corsair
deleted file mode 100644
index b8827f0..0000000
--- a/Documentation/ABI/testing/sysfs-driver-hid-corsair
+++ /dev/null
@@ -1,15 +0,0 @@
-What: /sys/bus/drivers/corsair/<dev>/macro_mode
-Date: August 2015
-KernelVersion: 4.2
-Contact: Clement Vuchener <[email protected]>
-Description: Get/set the current playback mode. "SW" for software mode
- where G-keys triggers their regular key codes. "HW" for
- hardware playback mode where the G-keys play their macro
- from the on-board memory.
-
-
-What: /sys/bus/drivers/corsair/<dev>/current_profile
-Date: August 2015
-KernelVersion: 4.2
-Contact: Clement Vuchener <[email protected]>
-Description: Get/set the current selected profile. Values are from 1 to 3.
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 4117225..43b018f 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -173,7 +173,7 @@ config HID_CHICONY
config HID_CORSAIR
tristate "Corsair devices"
- depends on HID && USB && LEDS_CLASS
+ depends on HID
---help---
Support for Corsair devices that are not fully compliant with the
HID standard.
diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
index 717704e..98f40aa 100644
--- a/drivers/hid/hid-corsair.c
+++ b/drivers/hid/hid-corsair.c
@@ -16,31 +16,9 @@
#include <linux/hid.h>
#include <linux/module.h>
-#include <linux/usb.h>
-#include <linux/leds.h>
#include "hid-ids.h"
-#define CORSAIR_USE_K90_MACRO (1<<0)
-#define CORSAIR_USE_K90_BACKLIGHT (1<<1)
-
-struct k90_led {
- struct led_classdev cdev;
- int brightness;
- struct work_struct work;
- bool removed;
-};
-
-struct k90_drvdata {
- struct k90_led record_led;
-};
-
-struct corsair_drvdata {
- unsigned long quirks;
- struct k90_drvdata *k90;
- struct k90_led *backlight;
-};
-
#define K90_GKEY_COUNT 18
static int corsair_usage_to_gkey(unsigned int usage)
@@ -119,474 +97,6 @@ MODULE_PARM_DESC(profilekey_codes, "Key codes for the profile buttons");
#define CORSAIR_USAGE_LIGHT_BRIGHT 0xfd
#define CORSAIR_USAGE_LIGHT_MAX 0xfd
-/* USB control protocol */
-
-#define K90_REQUEST_BRIGHTNESS 49
-#define K90_REQUEST_MACRO_MODE 2
-#define K90_REQUEST_STATUS 4
-#define K90_REQUEST_GET_MODE 5
-#define K90_REQUEST_PROFILE 20
-
-#define K90_MACRO_MODE_SW 0x0030
-#define K90_MACRO_MODE_HW 0x0001
-
-#define K90_MACRO_LED_ON 0x0020
-#define K90_MACRO_LED_OFF 0x0040
-
-/*
- * LED class devices
- */
-
-#define K90_BACKLIGHT_LED_SUFFIX "::backlight"
-#define K90_RECORD_LED_SUFFIX "::record"
-
-static enum led_brightness k90_backlight_get(struct led_classdev *led_cdev)
-{
- int ret;
- struct k90_led *led = container_of(led_cdev, struct k90_led, cdev);
- struct device *dev = led->cdev.dev->parent;
- struct usb_interface *usbif = to_usb_interface(dev->parent);
- struct usb_device *usbdev = interface_to_usbdev(usbif);
- int brightness;
- char data[8];
-
- ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0),
- K90_REQUEST_STATUS,
- USB_DIR_IN | USB_TYPE_VENDOR |
- USB_RECIP_DEVICE, 0, 0, data, 8,
- USB_CTRL_SET_TIMEOUT);
- if (ret < 0) {
- dev_warn(dev, "Failed to get K90 initial state (error %d).\n",
- ret);
- return -EIO;
- }
- brightness = data[4];
- if (brightness < 0 || brightness > 3) {
- dev_warn(dev,
- "Read invalid backlight brightness: %02hhx.\n",
- data[4]);
- return -EIO;
- }
- return brightness;
-}
-
-static enum led_brightness k90_record_led_get(struct led_classdev *led_cdev)
-{
- struct k90_led *led = container_of(led_cdev, struct k90_led, cdev);
-
- return led->brightness;
-}
-
-static void k90_brightness_set(struct led_classdev *led_cdev,
- enum led_brightness brightness)
-{
- struct k90_led *led = container_of(led_cdev, struct k90_led, cdev);
-
- led->brightness = brightness;
- schedule_work(&led->work);
-}
-
-static void k90_backlight_work(struct work_struct *work)
-{
- int ret;
- struct k90_led *led = container_of(work, struct k90_led, work);
- struct device *dev;
- struct usb_interface *usbif;
- struct usb_device *usbdev;
-
- if (led->removed)
- return;
-
- dev = led->cdev.dev->parent;
- usbif = to_usb_interface(dev->parent);
- usbdev = interface_to_usbdev(usbif);
-
- ret = usb_control_msg(usbdev, usb_sndctrlpipe(usbdev, 0),
- K90_REQUEST_BRIGHTNESS,
- USB_DIR_OUT | USB_TYPE_VENDOR |
- USB_RECIP_DEVICE, led->brightness, 0,
- NULL, 0, USB_CTRL_SET_TIMEOUT);
- if (ret != 0)
- dev_warn(dev, "Failed to set backlight brightness (error: %d).\n",
- ret);
-}
-
-static void k90_record_led_work(struct work_struct *work)
-{
- int ret;
- struct k90_led *led = container_of(work, struct k90_led, work);
- struct device *dev;
- struct usb_interface *usbif;
- struct usb_device *usbdev;
- int value;
-
- if (led->removed)
- return;
-
- dev = led->cdev.dev->parent;
- usbif = to_usb_interface(dev->parent);
- usbdev = interface_to_usbdev(usbif);
-
- if (led->brightness > 0)
- value = K90_MACRO_LED_ON;
- else
- value = K90_MACRO_LED_OFF;
-
- ret = usb_control_msg(usbdev, usb_sndctrlpipe(usbdev, 0),
- K90_REQUEST_MACRO_MODE,
- USB_DIR_OUT | USB_TYPE_VENDOR |
- USB_RECIP_DEVICE, value, 0, NULL, 0,
- USB_CTRL_SET_TIMEOUT);
- if (ret != 0)
- dev_warn(dev, "Failed to set record LED state (error: %d).\n",
- ret);
-}
-
-/*
- * Keyboard attributes
- */
-
-static ssize_t k90_show_macro_mode(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- int ret;
- struct usb_interface *usbif = to_usb_interface(dev->parent);
- struct usb_device *usbdev = interface_to_usbdev(usbif);
- const char *macro_mode;
- char data[8];
-
- ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0),
- K90_REQUEST_GET_MODE,
- USB_DIR_IN | USB_TYPE_VENDOR |
- USB_RECIP_DEVICE, 0, 0, data, 2,
- USB_CTRL_SET_TIMEOUT);
- if (ret < 0) {
- dev_warn(dev, "Failed to get K90 initial mode (error %d).\n",
- ret);
- return -EIO;
- }
-
- switch (data[0]) {
- case K90_MACRO_MODE_HW:
- macro_mode = "HW";
- break;
-
- case K90_MACRO_MODE_SW:
- macro_mode = "SW";
- break;
- default:
- dev_warn(dev, "K90 in unknown mode: %02hhx.\n",
- data[0]);
- return -EIO;
- }
-
- return snprintf(buf, PAGE_SIZE, "%s\n", macro_mode);
-}
-
-static ssize_t k90_store_macro_mode(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t count)
-{
- int ret;
- struct usb_interface *usbif = to_usb_interface(dev->parent);
- struct usb_device *usbdev = interface_to_usbdev(usbif);
- __u16 value;
-
- if (strncmp(buf, "SW", 2) == 0)
- value = K90_MACRO_MODE_SW;
- else if (strncmp(buf, "HW", 2) == 0)
- value = K90_MACRO_MODE_HW;
- else
- return -EINVAL;
-
- ret = usb_control_msg(usbdev, usb_sndctrlpipe(usbdev, 0),
- K90_REQUEST_MACRO_MODE,
- USB_DIR_OUT | USB_TYPE_VENDOR |
- USB_RECIP_DEVICE, value, 0, NULL, 0,
- USB_CTRL_SET_TIMEOUT);
- if (ret != 0) {
- dev_warn(dev, "Failed to set macro mode.\n");
- return ret;
- }
-
- return count;
-}
-
-static ssize_t k90_show_current_profile(struct device *dev,
- struct device_attribute *attr,
- char *buf)
-{
- int ret;
- struct usb_interface *usbif = to_usb_interface(dev->parent);
- struct usb_device *usbdev = interface_to_usbdev(usbif);
- int current_profile;
- char data[8];
-
- ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0),
- K90_REQUEST_STATUS,
- USB_DIR_IN | USB_TYPE_VENDOR |
- USB_RECIP_DEVICE, 0, 0, data, 8,
- USB_CTRL_SET_TIMEOUT);
- if (ret < 0) {
- dev_warn(dev, "Failed to get K90 initial state (error %d).\n",
- ret);
- return -EIO;
- }
- current_profile = data[7];
- if (current_profile < 1 || current_profile > 3) {
- dev_warn(dev, "Read invalid current profile: %02hhx.\n",
- data[7]);
- return -EIO;
- }
-
- return snprintf(buf, PAGE_SIZE, "%d\n", current_profile);
-}
-
-static ssize_t k90_store_current_profile(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t count)
-{
- int ret;
- struct usb_interface *usbif = to_usb_interface(dev->parent);
- struct usb_device *usbdev = interface_to_usbdev(usbif);
- int profile;
-
- if (kstrtoint(buf, 10, &profile))
- return -EINVAL;
- if (profile < 1 || profile > 3)
- return -EINVAL;
-
- ret = usb_control_msg(usbdev, usb_sndctrlpipe(usbdev, 0),
- K90_REQUEST_PROFILE,
- USB_DIR_OUT | USB_TYPE_VENDOR |
- USB_RECIP_DEVICE, profile, 0, NULL, 0,
- USB_CTRL_SET_TIMEOUT);
- if (ret != 0) {
- dev_warn(dev, "Failed to change current profile (error %d).\n",
- ret);
- return ret;
- }
-
- return count;
-}
-
-static DEVICE_ATTR(macro_mode, 0644, k90_show_macro_mode, k90_store_macro_mode);
-static DEVICE_ATTR(current_profile, 0644, k90_show_current_profile,
- k90_store_current_profile);
-
-static struct attribute *k90_attrs[] = {
- &dev_attr_macro_mode.attr,
- &dev_attr_current_profile.attr,
- NULL
-};
-
-static const struct attribute_group k90_attr_group = {
- .attrs = k90_attrs,
-};
-
-/*
- * Driver functions
- */
-
-static int k90_init_backlight(struct hid_device *dev)
-{
- int ret;
- struct corsair_drvdata *drvdata = hid_get_drvdata(dev);
- size_t name_sz;
- char *name;
-
- drvdata->backlight = kzalloc(sizeof(struct k90_led), GFP_KERNEL);
- if (!drvdata->backlight) {
- ret = -ENOMEM;
- goto fail_backlight_alloc;
- }
-
- name_sz =
- strlen(dev_name(&dev->dev)) + sizeof(K90_BACKLIGHT_LED_SUFFIX);
- name = kzalloc(name_sz, GFP_KERNEL);
- if (!name) {
- ret = -ENOMEM;
- goto fail_name_alloc;
- }
- snprintf(name, name_sz, "%s" K90_BACKLIGHT_LED_SUFFIX,
- dev_name(&dev->dev));
- drvdata->backlight->removed = false;
- drvdata->backlight->cdev.name = name;
- drvdata->backlight->cdev.max_brightness = 3;
- drvdata->backlight->cdev.brightness_set = k90_brightness_set;
- drvdata->backlight->cdev.brightness_get = k90_backlight_get;
- INIT_WORK(&drvdata->backlight->work, k90_backlight_work);
- ret = led_classdev_register(&dev->dev, &drvdata->backlight->cdev);
- if (ret != 0)
- goto fail_register_cdev;
-
- return 0;
-
-fail_register_cdev:
- kfree(drvdata->backlight->cdev.name);
-fail_name_alloc:
- kfree(drvdata->backlight);
- drvdata->backlight = NULL;
-fail_backlight_alloc:
- return ret;
-}
-
-static int k90_init_macro_functions(struct hid_device *dev)
-{
- int ret;
- struct corsair_drvdata *drvdata = hid_get_drvdata(dev);
- struct k90_drvdata *k90;
- size_t name_sz;
- char *name;
-
- k90 = kzalloc(sizeof(struct k90_drvdata), GFP_KERNEL);
- if (!k90) {
- ret = -ENOMEM;
- goto fail_drvdata;
- }
- drvdata->k90 = k90;
-
- /* Init LED device for record LED */
- name_sz = strlen(dev_name(&dev->dev)) + sizeof(K90_RECORD_LED_SUFFIX);
- name = kzalloc(name_sz, GFP_KERNEL);
- if (!name) {
- ret = -ENOMEM;
- goto fail_record_led_alloc;
- }
- snprintf(name, name_sz, "%s" K90_RECORD_LED_SUFFIX,
- dev_name(&dev->dev));
- k90->record_led.removed = false;
- k90->record_led.cdev.name = name;
- k90->record_led.cdev.max_brightness = 1;
- k90->record_led.cdev.brightness_set = k90_brightness_set;
- k90->record_led.cdev.brightness_get = k90_record_led_get;
- INIT_WORK(&k90->record_led.work, k90_record_led_work);
- k90->record_led.brightness = 0;
- ret = led_classdev_register(&dev->dev, &k90->record_led.cdev);
- if (ret != 0)
- goto fail_record_led;
-
- /* Init attributes */
- ret = sysfs_create_group(&dev->dev.kobj, &k90_attr_group);
- if (ret != 0)
- goto fail_sysfs;
-
- return 0;
-
-fail_sysfs:
- k90->record_led.removed = true;
- led_classdev_unregister(&k90->record_led.cdev);
- cancel_work_sync(&k90->record_led.work);
-fail_record_led:
- kfree(k90->record_led.cdev.name);
-fail_record_led_alloc:
- kfree(k90);
-fail_drvdata:
- drvdata->k90 = NULL;
- return ret;
-}
-
-static void k90_cleanup_backlight(struct hid_device *dev)
-{
- struct corsair_drvdata *drvdata = hid_get_drvdata(dev);
-
- if (drvdata->backlight) {
- drvdata->backlight->removed = true;
- led_classdev_unregister(&drvdata->backlight->cdev);
- cancel_work_sync(&drvdata->backlight->work);
- kfree(drvdata->backlight->cdev.name);
- kfree(drvdata->backlight);
- }
-}
-
-static void k90_cleanup_macro_functions(struct hid_device *dev)
-{
- struct corsair_drvdata *drvdata = hid_get_drvdata(dev);
- struct k90_drvdata *k90 = drvdata->k90;
-
- if (k90) {
- sysfs_remove_group(&dev->dev.kobj, &k90_attr_group);
-
- k90->record_led.removed = true;
- led_classdev_unregister(&k90->record_led.cdev);
- cancel_work_sync(&k90->record_led.work);
- kfree(k90->record_led.cdev.name);
-
- kfree(k90);
- }
-}
-
-static int corsair_probe(struct hid_device *dev, const struct hid_device_id *id)
-{
- int ret;
- unsigned long quirks = id->driver_data;
- struct corsair_drvdata *drvdata;
- struct usb_interface *usbif = to_usb_interface(dev->dev.parent);
-
- drvdata = devm_kzalloc(&dev->dev, sizeof(struct corsair_drvdata),
- GFP_KERNEL);
- if (drvdata == NULL)
- return -ENOMEM;
- drvdata->quirks = quirks;
- hid_set_drvdata(dev, drvdata);
-
- ret = hid_parse(dev);
- if (ret != 0) {
- hid_err(dev, "parse failed\n");
- return ret;
- }
- ret = hid_hw_start(dev, HID_CONNECT_DEFAULT);
- if (ret != 0) {
- hid_err(dev, "hw start failed\n");
- return ret;
- }
-
- if (usbif->cur_altsetting->desc.bInterfaceNumber == 0) {
- if (quirks & CORSAIR_USE_K90_MACRO) {
- ret = k90_init_macro_functions(dev);
- if (ret != 0)
- hid_warn(dev, "Failed to initialize K90 macro functions.\n");
- }
- if (quirks & CORSAIR_USE_K90_BACKLIGHT) {
- ret = k90_init_backlight(dev);
- if (ret != 0)
- hid_warn(dev, "Failed to initialize K90 backlight.\n");
- }
- }
-
- return 0;
-}
-
-static void corsair_remove(struct hid_device *dev)
-{
- k90_cleanup_macro_functions(dev);
- k90_cleanup_backlight(dev);
-
- hid_hw_stop(dev);
-}
-
-static int corsair_event(struct hid_device *dev, struct hid_field *field,
- struct hid_usage *usage, __s32 value)
-{
- struct corsair_drvdata *drvdata = hid_get_drvdata(dev);
-
- if (!drvdata->k90)
- return 0;
-
- switch (usage->hid & HID_USAGE) {
- case CORSAIR_USAGE_MACRO_RECORD_START:
- drvdata->k90->record_led.brightness = 1;
- break;
- case CORSAIR_USAGE_MACRO_RECORD_STOP:
- drvdata->k90->record_led.brightness = 0;
- break;
- default:
- break;
- }
-
- return 0;
-}
-
static int corsair_input_mapping(struct hid_device *dev,
struct hid_input *input,
struct hid_field *field,
@@ -641,9 +151,7 @@ static int corsair_input_mapping(struct hid_device *dev,
}
static const struct hid_device_id corsair_devices[] = {
- { HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_DEVICE_ID_CORSAIR_K90),
- .driver_data = CORSAIR_USE_K90_MACRO |
- CORSAIR_USE_K90_BACKLIGHT },
+ { HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_DEVICE_ID_CORSAIR_K90) },
{}
};
@@ -652,9 +160,6 @@ MODULE_DEVICE_TABLE(hid, corsair_devices);
static struct hid_driver corsair_driver = {
.name = "corsair",
.id_table = corsair_devices,
- .probe = corsair_probe,
- .event = corsair_event,
- .remove = corsair_remove,
.input_mapping = corsair_input_mapping,
};
--
2.5.5
The Corsair K40 uses the same usage codes as the K90 for its special keys (although it has only 6 G-keys).
Signed-off-by: Cl?ment Vuchener <[email protected]>
---
drivers/hid/hid-core.c | 1 +
drivers/hid/hid-corsair.c | 1 +
drivers/hid/hid-ids.h | 1 +
3 files changed, 3 insertions(+)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index bdb8cc8..73860b9 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1871,6 +1871,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_WIRELESS2) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_AK1D) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_ACER_SWITCH12) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_DEVICE_ID_CORSAIR_K40) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_DEVICE_ID_CORSAIR_K90) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CREATIVELABS, USB_DEVICE_ID_PRODIKEYS_PCMIDI) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, USB_DEVICE_ID_CYGNAL_CP2112) },
diff --git a/drivers/hid/hid-corsair.c b/drivers/hid/hid-corsair.c
index 98f40aa..85b5168 100644
--- a/drivers/hid/hid-corsair.c
+++ b/drivers/hid/hid-corsair.c
@@ -151,6 +151,7 @@ static int corsair_input_mapping(struct hid_device *dev,
}
static const struct hid_device_id corsair_devices[] = {
+ { HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_DEVICE_ID_CORSAIR_K40) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_DEVICE_ID_CORSAIR_K90) },
{}
};
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 5c0e43e..ea9fef9 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -256,6 +256,7 @@
#define USB_DEVICE_ID_CODEMERCS_IOW_LAST 0x15ff
#define USB_VENDOR_ID_CORSAIR 0x1b1c
+#define USB_DEVICE_ID_CORSAIR_K40 0x1b0e
#define USB_DEVICE_ID_CORSAIR_K90 0x1b02
#define USB_VENDOR_ID_CREATIVELABS 0x041e
--
2.5.5
On Wed, 23 Mar 2016, =?UTF-8?q?Cl=C3=A9ment=20Vuchener?= wrote:
> So, I decided to move all USB related features in user-space (as far as
> I know, I was the only user, but if someone is looking for a
> replacement, I wrote a small tool available here:
> https://github.com/cvuchener/corsair-usb-config). This simplification
> only leaves the usage code remapping part and the driver no longer
> depends on USB and LED subsystems. This should make the driver easier to
> maintain or to add new supported devices.
While you are performing this move, is there anything that's actually
preventing you from doing the remapping from userspace as well?
HID subsystem has for long time been providing the setkeycode() hook for
remapping usages, and udev (well, more precisely, that s*****d thing) is
actually shipping a lot of hw-specific remap data these days.
Thanks,
--
Jiri Kosina
SUSE Labs
Hi Clément,
On Mar 23 2016 or thereabouts, =?UTF-8?q?Cl=C3=A9ment=20Vuchener?= wrote:
> I tried to add support for the K40 some time ago, but the vendor specific USB protocol became over-complicated because of a lot of small differences between the K90 and the K40. Also, since I wrote the first version of this driver, I learned that USB control transfers could be done from user-space without the need to detach the kernel driver (please tell me if I am wrong).
>
> So, I decided to move all USB related features in user-space (as far as I know, I was the only user, but if someone is looking for a replacement, I wrote a small tool available here: https://github.com/cvuchener/corsair-usb-config). This simplification only leaves the usage code remapping part and the driver no longer depends on USB and LED subsystems. This should make the driver easier to maintain or to add new supported devices.
>
> After the removal of USB related functions in first patch, the addition of K40 support in the second patch is simply a matter of adding the device in the id list.
I would say you can not do this this way. Even if you believe you are the
only user of the API, there might be someone who uses it, and you will end
up breaking his keyboard.
Jiri will correct me, but the proper way to follow is to mark the API as
deprecated, make sure your driver uses the deprecated API only for the
K40, and then add the K90 in the driver, without implementing the API.
After a few months (years?) with your API marked as deprecated, you then
will be able to remove it. This is one of the many reasons we wrote
libratbag in pure user-space, to avoid having to maintain complex API in
the kernel forever.
Cheers,
Benjamin
>
> Clément Vuchener (2):
> HID: corsair: Remove all features using the USB protocol
> HID: corsair: Add K40 support
>
> Documentation/ABI/testing/sysfs-driver-hid-corsair | 15 -
> drivers/hid/Kconfig | 2 +-
> drivers/hid/hid-core.c | 1 +
> drivers/hid/hid-corsair.c | 498 +--------------------
> drivers/hid/hid-ids.h | 1 +
> 5 files changed, 5 insertions(+), 512 deletions(-)
> delete mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair
>
> --
> 2.5.5
>
On Thu, 24 Mar 2016, Benjamin Tissoires wrote:
> I would say you can not do this this way. Even if you believe you are the
> only user of the API, there might be someone who uses it, and you will end
> up breaking his keyboard.
>
> Jiri will correct me, but the proper way to follow is to mark the API as
> deprecated, make sure your driver uses the deprecated API only for the
> K40, and then add the K90 in the driver, without implementing the API.
>
> After a few months (years?) with your API marked as deprecated, you then
> will be able to remove it. This is one of the many reasons we wrote
> libratbag in pure user-space, to avoid having to maintain complex API in
> the kernel forever.
You are right that this is the right way to deprecate the API.
Fortunately this one is "officially" marked as testing, so we might be a
little bit more relaxed, but still we'd really need to take care not to
break users left and right.
That's why I first asked whether also the remapping shouldn't be moved to
userspace, to make sure that we eventuall start the depreciation of as
many features as possible at the same time.
Thanks,
--
Jiri Kosina
SUSE Labs
2016-03-24 15:30 GMT+01:00 Jiri Kosina <[email protected]>:
> On Wed, 23 Mar 2016, =?UTF-8?q?Cl=C3=A9ment=20Vuchener?= wrote:
>
>> So, I decided to move all USB related features in user-space (as far as
>> I know, I was the only user, but if someone is looking for a
>> replacement, I wrote a small tool available here:
>> https://github.com/cvuchener/corsair-usb-config). This simplification
>> only leaves the usage code remapping part and the driver no longer
>> depends on USB and LED subsystems. This should make the driver easier to
>> maintain or to add new supported devices.
>
> While you are performing this move, is there anything that's actually
> preventing you from doing the remapping from userspace as well?
>
> HID subsystem has for long time been providing the setkeycode() hook for
> remapping usages, and udev (well, more precisely, that s*****d thing) is
> actually shipping a lot of hw-specific remap data these days.
Thanks for the tip, is it possible to ignore some usages with this
method (done in the default case)?
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
>
On Thu, 24 Mar 2016, Clément VUCHENER wrote:
> Thanks for the tip, is it possible to ignore some usages with this
> method (done in the default case)?
mapping to 0xff is a bit hackish way to achieve this.
--
Jiri Kosina
SUSE Labs