All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] HID: add driver for Roccat Kone gaming mouse
@ 2010-02-20  8:11 ` Stefan Achatz
  0 siblings, 0 replies; 17+ messages in thread
From: Stefan Achatz @ 2010-02-20  8:11 UTC (permalink / raw)
  To: Jiri Kosina, Jussi Kivilinna, wylda, Pavel Machek,
	Alessandro Guido, Tomas Hanak, Jason Noble, simon.windows,
	Sean Hildebrand, Sid Boyce, Henning Glawe
  Cc: linux-input, linux-kernel

This Patch adds support for Kone gaming mouse from Roccat.
It provides access to profiles, settings, firmware, weight,
actual settings etc. through sysfs files.
Event handling of this mouse differs from standard hid behaviour
in that tilt button press is reported in each move event which
results in strange behaviour if not handled by the driver.
Signed-off-by: Stefan Achatz <erazor_de@users.sourceforge.net>

---
 drivers/hid/Kconfig           |    7 +
 drivers/hid/Makefile          |    1 +
 drivers/hid/hid-ids.h         |    3 +
 drivers/hid/hid-roccat-kone.c |  940 +++++++++++++++++++++++++++++++++++++++++
 drivers/hid/hid-roccat-kone.h |  187 ++++++++
 5 files changed, 1138 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hid/hid-roccat-kone.c
 create mode 100644 drivers/hid/hid-roccat-kone.h

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 24d90ea..ac945a6 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -227,6 +227,13 @@ config HID_PETALYNX
 	---help---
 	Support for Petalynx Maxter remote control.
 
+config HID_ROCCAT_KONE
+	tristate "Roccat Kone" if EMBEDDED
+	depends on USB_HID
+	default !EMBEDDED
+	---help---
+	Support for Roccat Kone mouse.
+
 config HID_SAMSUNG
 	tristate "Samsung" if EMBEDDED
 	depends on USB_HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 0de2dff..295d481 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_HID_MONTEREY)	+= hid-monterey.o
 obj-$(CONFIG_HID_NTRIG)		+= hid-ntrig.o
 obj-$(CONFIG_HID_PANTHERLORD)	+= hid-pl.o
 obj-$(CONFIG_HID_PETALYNX)	+= hid-petalynx.o
+obj-$(CONFIG_HID_ROCCAT_KONE)	+= hid-roccat-kone.o
 obj-$(CONFIG_HID_SAMSUNG)	+= hid-samsung.o
 obj-$(CONFIG_HID_SMARTJOYPLUS)	+= hid-sjoy.o
 obj-$(CONFIG_HID_SONY)		+= hid-sony.o
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 010368e..248cafc 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -382,6 +382,9 @@
 #define USB_VENDOR_ID_POWERCOM		0x0d9f
 #define USB_DEVICE_ID_POWERCOM_UPS	0x0002
 
+#define USB_VENDOR_ID_ROCCAT		0x1e7d
+#define USB_DEVICE_ID_ROCCAT_KONE	0x2ced
+
 #define USB_VENDOR_ID_SAITEK		0x06a3
 #define USB_DEVICE_ID_SAITEK_RUMBLEPAD	0xff17
 
diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
new file mode 100644
index 0000000..94a2fb9
--- /dev/null
+++ b/drivers/hid/hid-roccat-kone.c
@@ -0,0 +1,940 @@
+/*
+ * Roccat Kone driver for Linux
+ *
+ * Copyright (c) 2010 Stefan Achatz <erazor_de@users.sourceforge.net>
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/input.h>
+#include <linux/hid.h>
+#include <linux/usb.h>
+#include <linux/module.h>
+#include "hid-ids.h"
+#include "hid-roccat-kone.h"
+
+static uint16_t kone_calc_profile_checksum(struct kone_profile const *profile)
+{
+	uint16_t checksum = 0;
+	unsigned char *address;
+	int i;
+	for (i = 0, address = (unsigned char *)profile;
+			i < sizeof(struct kone_profile) - 2; ++i, ++address) {
+		checksum += *address;
+	}
+	return checksum;
+}
+
+static uint16_t kone_calc_settings_checksum(
+		struct kone_settings const *settings)
+{
+	uint16_t checksum = 0;
+	unsigned char *address = (unsigned char *)settings;
+	int i;
+
+	for (i = 0; i < sizeof(struct kone_settings) - 2; ++i, ++address)
+		checksum += *address;
+
+	return checksum;
+}
+
+static void kone_set_settings_checksum(struct kone_settings *settings)
+{
+	settings->checksum = cpu_to_le16(kone_calc_settings_checksum(settings));
+}
+
+static void kone_set_profile_checksum(struct kone_profile *profile)
+{
+	profile->checksum = cpu_to_le16(kone_calc_profile_checksum(profile));
+}
+
+static int kone_check_write(struct usb_device *usb_dev)
+{
+	int len;
+	unsigned char *data;
+
+	data = kmalloc(1, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	do {
+		msleep(50);
+
+		len = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
+				USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
+						| USB_RECIP_INTERFACE
+						| USB_DIR_IN,
+				kone_command_confirm_write, 0, data, 1,
+				USB_CTRL_SET_TIMEOUT);
+
+		if (len != 1) {
+			kfree(data);
+			return -EIO;
+		}
+		/*
+		 * value of 3 seems to mean something like
+		 * "not finished yet, but it looks good"
+		 * So check again after a moment.
+		 */
+	} while (*data == 3);
+
+	if (*data == 1) { /* everything alright */
+		kfree(data);
+		return 0;
+	} else { /* unknown answer */
+		dev_err(&usb_dev->dev, "got retval %d when checking write\n",
+				*data);
+		kfree(data);
+		return -EIO;
+	}
+}
+
+static int kone_get_settings(struct usb_device *usb_dev,
+		struct kone_settings *buf)
+{
+	int len;
+
+	len = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
+			USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
+					| USB_RECIP_INTERFACE | USB_DIR_IN,
+			kone_command_settings, 0, buf,
+			sizeof(struct kone_settings), USB_CTRL_SET_TIMEOUT);
+
+	if (len != sizeof(struct kone_settings))
+		return -EIO;
+
+	return 0;
+}
+
+static int kone_set_settings(struct usb_device *usb_dev,
+		struct kone_settings const *buf)
+{
+	struct kone_settings *settings, *original_settings;
+	int len, err;
+
+	settings = kmalloc(sizeof(struct kone_settings), GFP_KERNEL);
+	if (!settings)
+		return -ENOMEM;
+
+	memcpy(settings, buf, sizeof(struct kone_settings));
+
+	kone_set_settings_checksum(settings);
+
+	original_settings = kmalloc(sizeof(struct kone_settings), GFP_KERNEL);
+	if (!original_settings) {
+		kfree(settings);
+		return -ENOMEM;
+	}
+
+	err = kone_get_settings(usb_dev, original_settings);
+	if (err) {
+		kfree(original_settings);
+		kfree(settings);
+		return err;
+	}
+
+	if (memcmp(settings, original_settings, sizeof(struct kone_settings))
+			!= 0) {
+		len = usb_control_msg(usb_dev, usb_sndctrlpipe(usb_dev, 0),
+				USB_REQ_SET_CONFIGURATION, USB_TYPE_CLASS
+						| USB_RECIP_INTERFACE
+						| USB_DIR_OUT,
+				kone_command_settings, 0, settings,
+				sizeof(struct kone_settings),
+				USB_CTRL_SET_TIMEOUT);
+
+		if (len != sizeof(struct kone_settings)) {
+			kfree(original_settings);
+			kfree(settings);
+			return -EIO;
+		}
+
+		if (kone_check_write(usb_dev)) {
+			kfree(original_settings);
+			kfree(settings);
+			return -EIO;
+		}
+	}
+
+	kfree(original_settings);
+	kfree(settings);
+	return 0;
+}
+
+static int kone_get_startup_profile(struct usb_device *usb_dev, int *result)
+{
+	struct kone_settings *buf;
+	int err;
+
+	buf = kmalloc(sizeof(struct kone_settings), GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	err = kone_get_settings(usb_dev, buf);
+	if (err) {
+		kfree(buf);
+		return err;
+	}
+
+	*result = buf->startup_profile;
+	kfree(buf);
+	return 0;
+}
+
+static int kone_get_profile(struct usb_device *usb_dev,
+		struct kone_profile *buf, int number)
+{
+	int len;
+
+	if (number < 1 || number > 5)
+		return -EINVAL;
+
+	len = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
+			USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
+					| USB_RECIP_INTERFACE | USB_DIR_IN,
+			kone_command_profile, number, buf,
+			sizeof(struct kone_profile), USB_CTRL_SET_TIMEOUT);
+
+	if (len != sizeof(struct kone_profile)) {
+		dev_err(&usb_dev->dev, "wrong len %d\n", len);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int kone_set_profile(struct usb_device *usb_dev, char const *buf,
+		int number)
+{
+	struct kone_profile *profile, *original_profile;
+	int len, err, result;
+
+	if (number < 1 || number > 5)
+		return -EINVAL;
+
+	profile = kmalloc(sizeof(struct kone_profile), GFP_KERNEL);
+	if (!profile)
+		return -ENOMEM;
+
+	/* prepare profile to write */
+	memcpy(profile, buf, sizeof(struct kone_profile));
+	profile->profile = number;
+	kone_set_profile_checksum(profile);
+
+	/*
+	 * to reduce unnecessary writes read profile and compare with data
+	 * that should be written
+	 */
+	original_profile = kmalloc(sizeof(struct kone_profile), GFP_KERNEL);
+	if (!original_profile) {
+		kfree(profile);
+		return -ENOMEM;
+	}
+
+	err = kone_get_profile(usb_dev, original_profile, number);
+	if (err) {
+		kfree(original_profile);
+		kfree(profile);
+		return err;
+	}
+
+	result = memcmp(profile, original_profile, sizeof(struct kone_profile));
+	if (result != 0) {
+		len = usb_control_msg(usb_dev, usb_sndctrlpipe(usb_dev, 0),
+				USB_REQ_SET_CONFIGURATION, USB_TYPE_CLASS
+						| USB_RECIP_INTERFACE
+						| USB_DIR_OUT,
+				kone_command_profile, number, profile,
+				sizeof(struct kone_profile),
+				USB_CTRL_SET_TIMEOUT);
+
+		if (len != sizeof(struct kone_profile)) {
+			kfree(original_profile);
+			kfree(profile);
+			return -EIO;
+		}
+
+		if (kone_check_write(usb_dev)) {
+			kfree(original_profile);
+			kfree(profile);
+			return -EIO;
+		}
+	}
+
+	kfree(original_profile);
+	kfree(profile);
+	return 0;
+}
+
+static int kone_get_profile_startup_dpi(struct usb_device *usb_dev, int number,
+		int *result)
+{
+	struct kone_profile *buf;
+	int err;
+
+	buf = kmalloc(sizeof(struct kone_profile), GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	err = kone_get_profile(usb_dev, buf, number);
+	if (err) {
+		kfree(buf);
+		return err;
+	}
+
+	*result = buf->dpi_value;
+	kfree(buf);
+	return 0;
+}
+
+static int kone_get_weight(struct usb_device *usb_dev, int *result)
+{
+	int len;
+	uint8_t *data;
+
+	data = kmalloc(1, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	len = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
+			USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
+					| USB_RECIP_INTERFACE | USB_DIR_IN,
+			kone_command_weight, 0, data, 1, USB_CTRL_SET_TIMEOUT);
+
+	if (len != 1) {
+		kfree(data);
+		return -EIO;
+	}
+	*result = (int)*data;
+	return 0;
+}
+
+static int kone_get_firmware_version(struct usb_device *usb_dev, int *result)
+{
+	int len;
+	unsigned char *data;
+
+	data = kmalloc(2, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	len = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
+			USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
+					| USB_RECIP_INTERFACE | USB_DIR_IN,
+			kone_command_firmware_version, 0, data, 2,
+			USB_CTRL_SET_TIMEOUT);
+
+	if (len != 2) {
+		kfree(data);
+		return -EIO;
+	}
+	*result = (int)*data;
+	return 0;
+}
+
+static ssize_t kone_set_settings_raw(struct device *dev,
+		struct device_attribute *attr, char const *buf, size_t size)
+{
+	struct hid_device *hdev = dev_get_drvdata(dev);
+	struct kone_device *kone = hid_get_drvdata(hdev);
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	int err;
+
+	if (size != sizeof(struct kone_settings))
+		return -EINVAL;
+
+	err = kone_set_settings(usb_dev, (struct kone_settings const *)buf);
+	if (err)
+		return err;
+
+	/*
+	 * If we get here, treat buf as okay (apart from checksum) and use value
+	 * of startup_profile as its at hand
+	 */
+	kone->act_profile = ((struct kone_settings *)buf)->startup_profile;
+	kone->act_profile_valid = 1;
+	kone->act_dpi_valid = 0;
+
+	return sizeof(struct kone_settings);
+}
+
+static ssize_t kone_show_settings_raw(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	int err;
+	err = kone_get_settings(usb_dev, (struct kone_settings *)buf);
+	if (err)
+		return err;
+
+	return sizeof(struct kone_settings);
+}
+
+static ssize_t kone_get_profile_raw(struct device *dev, char *buf, int number)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	int err;
+	err = kone_get_profile(usb_dev, (struct kone_profile *)buf, number);
+	if (err)
+		return err;
+	return sizeof(struct kone_profile);
+}
+
+static ssize_t kone_set_profile_raw(struct device *dev, char const *buf,
+		size_t size, int number)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+
+	int err;
+
+	if (size != sizeof(struct kone_profile))
+		return -EINVAL;
+
+	err = kone_set_profile(usb_dev, buf, number);
+	if (err)
+		return err;
+
+	return sizeof(struct kone_profile);
+}
+
+static ssize_t kone_show_profile_1_raw(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	return kone_get_profile_raw(dev, buf, 1);
+}
+
+static ssize_t kone_set_profile_1_raw(struct device *dev,
+		struct device_attribute *attr, char const *buf, size_t size)
+{
+	return kone_set_profile_raw(dev, buf, size, 1);
+}
+
+static ssize_t kone_show_profile_2_raw(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	return kone_get_profile_raw(dev, buf, 2);
+}
+
+static ssize_t kone_set_profile_2_raw(struct device *dev,
+		struct device_attribute *attr, char const *buf, size_t size)
+{
+	return kone_set_profile_raw(dev, buf, size, 2);
+}
+
+static ssize_t kone_show_profile_3_raw(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	return kone_get_profile_raw(dev, buf, 3);
+}
+
+static ssize_t kone_set_profile_3_raw(struct device *dev,
+		struct device_attribute *attr, char const *buf, size_t size)
+{
+	return kone_set_profile_raw(dev, buf, size, 3);
+}
+
+static ssize_t kone_show_profile_4_raw(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	return kone_get_profile_raw(dev, buf, 4);
+}
+
+static ssize_t kone_set_profile_4_raw(struct device *dev,
+		struct device_attribute *attr, char const *buf, size_t size)
+{
+	return kone_set_profile_raw(dev, buf, size, 4);
+}
+
+static ssize_t kone_show_profile_5_raw(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	return kone_get_profile_raw(dev, buf, 5);
+}
+
+static ssize_t kone_set_profile_5_raw(struct device *dev,
+		struct device_attribute *attr, char const *buf, size_t size)
+{
+	return kone_set_profile_raw(dev, buf, size, 5);
+}
+
+/*
+ * helper to fill kone_device structure with actual values
+ * returns 0 on success or error
+ */
+static int kone_device_set_actual_values(struct kone_device *kone,
+		struct device *dev, int both)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	int err;
+
+	/* first make sure profile is valid */
+	if (!kone->act_profile_valid) {
+		err = kone_get_startup_profile(usb_dev, &kone->act_profile);
+		if (err)
+			return err;
+		kone->act_profile_valid = 1;
+	}
+
+	/* then get startup dpi value */
+	if (!kone->act_dpi_valid && both) {
+		err = kone_get_profile_startup_dpi(usb_dev, kone->act_profile,
+				&kone->act_dpi);
+		if (err)
+			return err;
+		kone->act_dpi_valid = 1;
+	}
+
+	return 0;
+}
+
+static ssize_t kone_show_actual_profile(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct hid_device *hdev = dev_get_drvdata(dev);
+	struct kone_device *kone = hid_get_drvdata(hdev);
+	int err;
+	err = kone_device_set_actual_values(kone, dev, 0);
+	if (err)
+		return err;
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", kone->act_profile);
+}
+
+static ssize_t kone_show_actual_dpi_raw(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct hid_device *hdev = dev_get_drvdata(dev);
+	struct kone_device *kone = hid_get_drvdata(hdev);
+	int err;
+
+	err = kone_device_set_actual_values(kone, dev, 1);
+	if (err)
+		return err;
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", kone->act_dpi);
+}
+
+static ssize_t kone_show_actual_dpi(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct hid_device *hdev = dev_get_drvdata(dev);
+	struct kone_device *kone = hid_get_drvdata(hdev);
+	int err, dpi;
+	err = kone_device_set_actual_values(kone, dev, 1);
+	if (err)
+		return err;
+
+	dpi = kone->act_dpi;
+	switch (dpi) {
+	case 0:
+		break;
+	case 6:
+		dpi = 3200;
+		break;
+	default:
+		dpi = dpi * 400;
+	}
+
+	return snprintf(buf, PAGE_SIZE, "%ddpi\n", dpi);
+}
+
+static ssize_t kone_show_weight_raw(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	int weight;
+	int retval;
+	retval = kone_get_weight(usb_dev, &weight);
+	if (retval)
+		return retval;
+	return snprintf(buf, PAGE_SIZE, "%d\n", weight);
+}
+
+static ssize_t kone_show_firmware_version_raw(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	int firmware_version;
+	int retval;
+	retval = kone_get_firmware_version(usb_dev, &firmware_version);
+	if (retval)
+		return retval;
+	return snprintf(buf, PAGE_SIZE, "%d\n", firmware_version);
+}
+
+static ssize_t kone_show_tcu(struct device *dev, struct device_attribute *attr,
+		char *buf)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	int err, result;
+	struct kone_settings *settings;
+
+	settings = kmalloc(sizeof(struct kone_settings), GFP_KERNEL);
+	if (!settings)
+		return -ENOMEM;
+
+	err = kone_get_settings(usb_dev, settings);
+	if (err) {
+		kfree(settings);
+		return err;
+	}
+
+	result = settings->tcu;
+	kfree(settings);
+	return snprintf(buf, PAGE_SIZE, "%d\n", result);
+}
+
+static int kone_tcu_command(struct usb_device *usb_dev, int number)
+{
+	int len;
+	char *value;
+
+	value = kmalloc(1, GFP_KERNEL);
+	if (!value)
+		return -ENOMEM;
+
+	*value = number;
+
+	len = usb_control_msg(usb_dev, usb_sndctrlpipe(usb_dev, 0),
+			USB_REQ_SET_CONFIGURATION, USB_TYPE_CLASS
+					| USB_RECIP_INTERFACE | USB_DIR_OUT,
+			kone_command_calibrate, 0, value, 1,
+			USB_CTRL_SET_TIMEOUT);
+
+	if (len != 1) {
+		kfree(value);
+		return -EIO;
+	}
+
+	kfree(value);
+	return 0;
+}
+
+/* integer of 0 deactivates tcu, 1 activates it */
+static ssize_t kone_set_tcu(struct device *dev, struct device_attribute *attr,
+		char const *buf, size_t size)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	int err;
+	unsigned long state;
+	struct kone_settings *settings;
+
+	err = strict_strtoul(buf, 10, &state);
+	if (err)
+		return err;
+
+	if (state != 0 && state != 1)
+		return -EINVAL;
+
+	if (state == 1) { /* state activate */
+		err = kone_tcu_command(usb_dev, 1);
+		if (err)
+			return err;
+		err = kone_tcu_command(usb_dev, 2);
+		if (err)
+			return err;
+		ssleep(5); /* tcu needs this time for calibration */
+		err = kone_tcu_command(usb_dev, 3);
+		if (err)
+			return err;
+		err = kone_tcu_command(usb_dev, 0);
+		if (err)
+			return err;
+		err = kone_tcu_command(usb_dev, 4);
+		if (err)
+			return err;
+		/*
+		 * Kone needs this time to settle things.
+		 * Reading settings too early will result in invalid data.
+		 * Roccat's driver waits 1 sec, maybe this time could be
+		 * shortened.
+		 */
+		ssleep(1);
+	}
+
+	settings = kmalloc(sizeof(struct kone_settings), GFP_KERNEL);
+	if (!settings)
+		return -ENOMEM;
+
+	err = kone_get_settings(usb_dev, settings);
+	if (err) {
+		kfree(settings);
+		return err;
+	}
+
+	/* only write settings back if activation state is different */
+	if (settings->tcu != state) {
+		settings->tcu = state;
+
+		err = kone_set_settings(usb_dev, settings);
+		if (err) {
+			kfree(settings);
+			return err;
+		}
+	}
+
+	kfree(settings);
+	return size;
+}
+
+static ssize_t kone_show_startup_profile(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
+	int err, result;
+	err = kone_get_startup_profile(usb_dev, &result);
+	if (err)
+		return err;
+	return snprintf(buf, PAGE_SIZE, "%d\n", result);
+}
+
+static ssize_t kone_set_startup_profile(struct device *dev,
+		struct device_attribute *attr, char const *buf, size_t size)
+{
+	struct hid_device *hdev = dev_get_drvdata(dev);
+	struct kone_device *kone = hid_get_drvdata(hdev);
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	int err;
+	unsigned long new_profile;
+	struct kone_settings *settings;
+
+	err = strict_strtoul(buf, 10, &new_profile);
+	if (err)
+		return err;
+
+	if (new_profile  < 1 || new_profile > 5)
+		return -EINVAL;
+
+	settings = kmalloc(sizeof(struct kone_settings), GFP_KERNEL);
+	if (!settings)
+		return -ENOMEM;
+
+	err = kone_get_settings(usb_dev, settings);
+	if (err) {
+		kfree(settings);
+		return err;
+	}
+
+	settings->startup_profile = new_profile;
+
+	err = kone_set_settings(usb_dev, settings);
+	if (err) {
+		kfree(settings);
+		return err;
+	}
+
+	kone->act_profile = new_profile;
+	kone->act_profile_valid = 1;
+	kone->act_dpi_valid = 0;
+
+	kfree(settings);
+	return size;
+}
+
+static ssize_t kone_show_driver_version(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, DRIVER_VERSION "\n");
+}
+
+static DEVICE_ATTR(actual_dpi_raw, S_IRUGO, kone_show_actual_dpi_raw, NULL);
+static DEVICE_ATTR(actual_dpi, S_IRUGO, kone_show_actual_dpi, NULL);
+static DEVICE_ATTR(actual_profile, S_IRUGO, kone_show_actual_profile, NULL);
+static DEVICE_ATTR(weight_raw, S_IRUGO, kone_show_weight_raw, NULL);
+static DEVICE_ATTR(profile1_raw, S_IRUGO | S_IWUGO,
+		kone_show_profile_1_raw, kone_set_profile_1_raw);
+static DEVICE_ATTR(profile2_raw, S_IRUGO | S_IWUGO,
+		kone_show_profile_2_raw, kone_set_profile_2_raw);
+static DEVICE_ATTR(profile3_raw, S_IRUGO | S_IWUGO,
+		kone_show_profile_3_raw, kone_set_profile_3_raw);
+static DEVICE_ATTR(profile4_raw, S_IRUGO | S_IWUGO,
+		kone_show_profile_4_raw, kone_set_profile_4_raw);
+static DEVICE_ATTR(profile5_raw, S_IRUGO | S_IWUGO,
+		kone_show_profile_5_raw, kone_set_profile_5_raw);
+static DEVICE_ATTR(settings_raw, S_IRUGO | S_IWUGO,
+		kone_show_settings_raw, kone_set_settings_raw);
+static DEVICE_ATTR(firmware_version_raw, S_IRUGO,
+		kone_show_firmware_version_raw, NULL);
+static DEVICE_ATTR(tcu, S_IRUGO | S_IWUGO, kone_show_tcu, kone_set_tcu);
+static DEVICE_ATTR(startup_profile, S_IRUGO | S_IWUGO,
+		kone_show_startup_profile, kone_set_startup_profile);
+static DEVICE_ATTR(kone_driver_version, S_IRUGO,
+		kone_show_driver_version, NULL);
+
+static struct attribute *kone_attributes[] = {
+		&dev_attr_actual_dpi_raw.attr,
+		&dev_attr_actual_dpi.attr,
+		&dev_attr_actual_profile.attr,
+		&dev_attr_weight_raw.attr,
+		&dev_attr_profile1_raw.attr,
+		&dev_attr_profile2_raw.attr,
+		&dev_attr_profile3_raw.attr,
+		&dev_attr_profile4_raw.attr,
+		&dev_attr_profile5_raw.attr,
+		&dev_attr_settings_raw.attr,
+		&dev_attr_firmware_version_raw.attr,
+		&dev_attr_tcu.attr,
+		&dev_attr_startup_profile.attr,
+		&dev_attr_kone_driver_version.attr,
+		NULL
+};
+
+static struct attribute_group kone_attribute_group = {
+		.attrs = kone_attributes
+};
+
+static int kone_create_files(struct usb_interface *intf)
+{
+	if (intf->cur_altsetting->desc.bInterfaceProtocol
+			== USB_INTERFACE_PROTOCOL_MOUSE)
+		return sysfs_create_group(&intf->dev.kobj,
+				&kone_attribute_group);
+	else
+		return 0;
+}
+
+static void kone_remove_files(struct usb_interface *intf)
+{
+	if (intf->cur_altsetting->desc.bInterfaceProtocol
+			== USB_INTERFACE_PROTOCOL_MOUSE)
+		sysfs_remove_group(&intf->dev.kobj, &kone_attribute_group);
+}
+
+static int kone_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+	struct kone_device *kone;
+	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
+	int ret;
+
+	kone = kzalloc(sizeof(*kone), GFP_KERNEL);
+	if (!kone) {
+		dev_err(&hdev->dev, "can't alloc device descriptor\n");
+		return -ENOMEM;
+	}
+
+	hid_set_drvdata(hdev, kone);
+	ret = hid_parse(hdev);
+	if (ret) {
+		dev_err(&hdev->dev, "parse failed\n");
+		goto err_free;
+	}
+
+	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+	if (ret) {
+		dev_err(&hdev->dev, "hw start failed\n");
+		goto err_free;
+	}
+
+	ret = kone_create_files(intf);
+	if (ret) {
+		dev_err(&hdev->dev, "cannot create sysfs files\n");
+		goto err_stop;
+	}
+
+	return 0;
+err_stop:
+	hid_hw_stop(hdev);
+err_free:
+	kfree(kone);
+	return ret;
+}
+
+static void kone_remove(struct hid_device *hdev)
+{
+	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
+	kone_remove_files(intf);
+	hid_hw_stop(hdev);
+	kfree(hid_get_drvdata(hdev));
+}
+
+static int kone_raw_event(struct hid_device *hdev, struct hid_report *report,
+		u8 *data, int size)
+{
+	struct kone_device *kone = hid_get_drvdata(hdev);
+	struct kone_mouse_event *event = (struct kone_mouse_event *)data;
+
+	/* keyboard events should be processed by default handler */
+	if (size != 12)
+		return 0;
+
+	/* Firmware 1.38 introduced new behaviour for tilt buttons.
+	 * Pressed tilt button is reported in each movement event.
+	 * Workaround sends only one event per press.
+	 */
+	if (kone->last_tilt_state == event->tilt)
+		event->tilt = 0;
+	else
+		kone->last_tilt_state = event->tilt;
+
+	switch (event->event) {
+	case kone_mouse_event_osd_dpi:
+		kone->act_dpi = event->value;
+		kone->act_dpi_valid = 1;
+		dev_dbg(&hdev->dev, "osd dpi event. actual dpi %d (and %d)\n",
+				event->value, kone->act_dpi);
+		return 1; /* return 1 if event was handled */
+	case kone_mouse_event_switch_dpi:
+		kone->act_dpi = event->value;
+		kone->act_dpi_valid = 1;
+		dev_dbg(&hdev->dev, "switched dpi to %d\n", event->value);
+		return 1;
+	case kone_mouse_event_osd_profile:
+		kone->act_profile = event->value;
+		kone->act_profile_valid = 1;
+		dev_dbg(&hdev->dev, "osd profile event. actual profile %d\n",
+				event->value);
+		return 1;
+	case kone_mouse_event_switch_profile:
+		kone->act_profile = event->value;
+		kone->act_profile_valid = 1;
+		kone->act_dpi_valid = 0;
+		dev_dbg(&hdev->dev, "switched profile to %d\n", event->value);
+		return 1;
+	case kone_mouse_event_call_overlong_macro:
+		dev_dbg(&hdev->dev, "overlong macro called %d\n", event->macro);
+		return 1;
+	}
+
+	return 0; /* do further processing */
+}
+
+static const struct hid_device_id kone_devices[] = { { HID_USB_DEVICE(
+		USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_KONE) }, { } };
+MODULE_DEVICE_TABLE(hid, kone_devices);
+
+static struct hid_driver kone_driver = { .name = "kone",
+		.id_table = kone_devices, .probe = kone_probe,
+		.remove = kone_remove, .raw_event = kone_raw_event, };
+
+static int kone_init(void)
+{
+	return hid_register_driver(&kone_driver);
+}
+
+static void kone_exit(void)
+{
+	hid_unregister_driver(&kone_driver);
+}
+
+module_init(kone_init);
+module_exit(kone_exit);
+
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE(DRIVER_LICENSE);
diff --git a/drivers/hid/hid-roccat-kone.h b/drivers/hid/hid-roccat-kone.h
new file mode 100644
index 0000000..14f5ebf
--- /dev/null
+++ b/drivers/hid/hid-roccat-kone.h
@@ -0,0 +1,187 @@
+#ifndef __HID_ROCCAT_KONE_H
+#define __HID_ROCCAT_KONE_H
+
+/*
+ * Copyright (c) 2010 Stefan Achatz <erazor_de@users.sourceforge.net>
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+#include <linux/types.h>
+
+#define DRIVER_VERSION "v0.2.4"
+#define DRIVER_AUTHOR "Stefan Achatz"
+#define DRIVER_DESC "USB Roccat Kone driver"
+#define DRIVER_LICENSE "GPL v2"
+
+struct kone_device {
+	/*
+	 * Actual values might not get called that much so I store them when
+	 * they are at hand or get them only when needed.
+	 */
+	int act_profile, act_profile_valid;
+	int act_dpi, act_dpi_valid;
+	int last_tilt_state;
+};
+
+#pragma pack(push)
+#pragma pack(1)
+
+struct kone_keystroke {
+	uint8_t key;
+	uint8_t action;
+	uint16_t period; /* in milliseconds */
+};
+
+enum kone_keystroke_buttons {
+	kone_keystroke_button_1 = 0xf0, /* left mouse button */
+	kone_keystroke_button_2 = 0xf1, /* right mouse button */
+	kone_keystroke_button_3 = 0xf2, /* wheel */
+	kone_keystroke_button_9 = 0xf3, /* side button up  \todo confirm */
+	kone_keystroke_button_8 = 0xf4 /* side button down \todo confirm */
+};
+
+enum kone_keystroke_actions {
+	kone_keystroke_action_press = 0,
+	kone_keystroke_action_release = 1
+};
+
+struct kone_button_info {
+	uint8_t number; /* range 1-8 */
+	uint8_t type;
+	uint8_t macro_type; /* 0 = short, 1 = overlong */
+	uint8_t macro_name[16]; /* can be max 15 chars long */
+	uint8_t set_name[16]; /* can be max 15 chars long */
+	uint8_t count;
+	struct kone_keystroke keystroke[20];
+};
+
+enum kone_button_info_types {
+	/* valid button types until firmware 1.32 */
+	kone_button_info_type_button_1 = 0x1, /* click (left mouse button) */
+	kone_button_info_type_button_2 = 0x2, /* menu (right mouse button)*/
+	kone_button_info_type_button_3 = 0x3, /* scroll (wheel) */
+	kone_button_info_type_double_click = 0x4,
+	kone_button_info_type_key = 0x5,
+	kone_button_info_type_macro = 0x6,
+	kone_button_info_type_off = 0x7,
+	/* TODO clarify function and rename */
+	kone_button_info_type_osd_xy_prescaling = 0x8,
+	kone_button_info_type_osd_dpi = 0x9,
+	kone_button_info_type_osd_profile = 0xa,
+	kone_button_info_type_button_9 = 0xb, /* ie forward */
+	kone_button_info_type_button_8 = 0xc, /* ie backward */
+	kone_button_info_type_dpi_up = 0xd, /* internal */
+	kone_button_info_type_dpi_down = 0xe, /* internal */
+	kone_button_info_type_button_7 = 0xf, /* tilt left */
+	kone_button_info_type_button_6 = 0x10, /* tilt right */
+	kone_button_info_type_profile_up = 0x11, /* internal */
+	kone_button_info_type_profile_down = 0x12, /* internal */
+	kone_button_info_type_overlong_macro = 0x106,
+	/* additional valid button types since firmware 1.38 */
+	kone_button_info_type_multimedia_open_player = 0x20,
+	kone_button_info_type_multimedia_next_track = 0x21,
+	kone_button_info_type_multimedia_prev_track = 0x22,
+	kone_button_info_type_multimedia_play_pause = 0x23,
+	kone_button_info_type_multimedia_stop = 0x24,
+	kone_button_info_type_multimedia_mute = 0x25,
+	kone_button_info_type_multimedia_volume_up = 0x26,
+	kone_button_info_type_multimedia_volume_down = 0x27
+};
+
+struct kone_light_info {
+	uint8_t number; /* number of light 1-5 */
+	uint8_t mod;   /* 1 = on, 2 = off */
+	uint8_t red;   /* range 0x00-0xff */
+	uint8_t green; /* range 0x00-0xff */
+	uint8_t blue;  /* range 0x00-0xff */
+};
+
+struct kone_profile {
+	uint16_t size; /* always 975 */
+	uint16_t unused; /* always 0 */
+	uint8_t profile; /* range 1-5 */
+	uint16_t main_sensitivity; /* range 100-1000 */
+	uint8_t xy_sensitivity_enabled; /* 1 = on, 2 = off */
+	uint16_t x_sensitivity; /* range 100-1000 */
+	uint16_t y_sensitivity; /* range 100-1000 */
+	uint8_t dpi_rate; /* bit 1 = 800, ... */
+	uint8_t dpi_value; /* range 1-6 */
+	uint8_t polling_rate; /* 1 = 125Hz, 2 = 500Hz, 3 = 1000Hz */
+	/* kone has no dcu
+	 * value is always 2 in firmwares <= 1.32 and
+	 * 1 in firmwares > 1.32
+	 */
+	uint8_t dcu_flag;
+	uint8_t light_effect_1; /* range 1-3 */
+	uint8_t light_effect_2; /* range 1-5 */
+	uint8_t light_effect_3; /* range 1-4 */
+	uint8_t light_effect_speed; /* range 0-255 */
+
+	struct kone_light_info light_info[5];
+	struct kone_button_info button_info[8];
+
+	uint16_t checksum; /* \brief holds checksum of struct */
+};
+
+enum kone_polling_rates {
+	kone_polling_rate_125 = 1,
+	kone_polling_rate_500 = 2,
+	kone_polling_rate_1000 = 3
+};
+
+struct kone_settings {
+	uint16_t size; /* always 36 */
+	uint8_t  startup_profile; /* 1-5 */
+	uint8_t	 unknown1;
+	uint8_t  tcu; /* 0 = off, 1 = on */
+	uint8_t  unknown2[23];
+	uint8_t  calibration_data[4];
+	uint8_t  unknown3[2];
+	uint16_t checksum;
+};
+
+/*
+ * 12 byte mouse event read by interrupt_read
+ */
+struct kone_mouse_event {
+	uint8_t report_number; /* always 1 */
+	uint8_t button;
+	uint16_t x;
+	uint16_t y;
+	uint8_t wheel; /* up = 1, down = -1 */
+	uint8_t tilt; /* right = 1, left = -1 */
+	uint8_t unknown;
+	uint8_t event;
+	uint8_t value;
+	uint8_t macro;
+};
+
+enum kone_mouse_events {
+	kone_mouse_event_osd_dpi = 0xa0,
+	kone_mouse_event_osd_profile = 0xb0,
+	/* TODO clarify meaning and occurence of kone_mouse_event_calibration */
+	kone_mouse_event_calibration = 0xc0,
+	kone_mouse_event_call_overlong_macro = 0xe0,
+	kone_mouse_event_switch_dpi = 0xf0,
+	kone_mouse_event_switch_profile = 0xf1
+};
+
+enum kone_commands {
+	kone_command_profile = 0x5a,
+	kone_command_settings = 0x15a,
+	kone_command_firmware_version = 0x25a,
+	kone_command_weight = 0x45a,
+	kone_command_calibrate = 0x55a,
+	kone_command_confirm_write = 0x65a,
+	kone_command_firmware = 0xe5a
+};
+
+#pragma pack(pop)
+
+#endif
-- 
1.6.6


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH] HID: add driver for Roccat Kone gaming mouse
@ 2010-02-20  8:11 ` Stefan Achatz
  0 siblings, 0 replies; 17+ messages in thread
From: Stefan Achatz @ 2010-02-20  8:11 UTC (permalink / raw)
  To: Jiri Kosina, Jussi Kivilinna, wylda, Pavel Machek,
	Alessandro Guido, Tomas Hanak
  Cc: linux-input, linux-kernel

This Patch adds support for Kone gaming mouse from Roccat.
It provides access to profiles, settings, firmware, weight,
actual settings etc. through sysfs files.
Event handling of this mouse differs from standard hid behaviour
in that tilt button press is reported in each move event which
results in strange behaviour if not handled by the driver.
Signed-off-by: Stefan Achatz <erazor_de@users.sourceforge.net>

---
 drivers/hid/Kconfig           |    7 +
 drivers/hid/Makefile          |    1 +
 drivers/hid/hid-ids.h         |    3 +
 drivers/hid/hid-roccat-kone.c |  940 +++++++++++++++++++++++++++++++++++++++++
 drivers/hid/hid-roccat-kone.h |  187 ++++++++
 5 files changed, 1138 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hid/hid-roccat-kone.c
 create mode 100644 drivers/hid/hid-roccat-kone.h

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 24d90ea..ac945a6 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -227,6 +227,13 @@ config HID_PETALYNX
 	---help---
 	Support for Petalynx Maxter remote control.
 
+config HID_ROCCAT_KONE
+	tristate "Roccat Kone" if EMBEDDED
+	depends on USB_HID
+	default !EMBEDDED
+	---help---
+	Support for Roccat Kone mouse.
+
 config HID_SAMSUNG
 	tristate "Samsung" if EMBEDDED
 	depends on USB_HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 0de2dff..295d481 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_HID_MONTEREY)	+= hid-monterey.o
 obj-$(CONFIG_HID_NTRIG)		+= hid-ntrig.o
 obj-$(CONFIG_HID_PANTHERLORD)	+= hid-pl.o
 obj-$(CONFIG_HID_PETALYNX)	+= hid-petalynx.o
+obj-$(CONFIG_HID_ROCCAT_KONE)	+= hid-roccat-kone.o
 obj-$(CONFIG_HID_SAMSUNG)	+= hid-samsung.o
 obj-$(CONFIG_HID_SMARTJOYPLUS)	+= hid-sjoy.o
 obj-$(CONFIG_HID_SONY)		+= hid-sony.o
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 010368e..248cafc 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -382,6 +382,9 @@
 #define USB_VENDOR_ID_POWERCOM		0x0d9f
 #define USB_DEVICE_ID_POWERCOM_UPS	0x0002
 
+#define USB_VENDOR_ID_ROCCAT		0x1e7d
+#define USB_DEVICE_ID_ROCCAT_KONE	0x2ced
+
 #define USB_VENDOR_ID_SAITEK		0x06a3
 #define USB_DEVICE_ID_SAITEK_RUMBLEPAD	0xff17
 
diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
new file mode 100644
index 0000000..94a2fb9
--- /dev/null
+++ b/drivers/hid/hid-roccat-kone.c
@@ -0,0 +1,940 @@
+/*
+ * Roccat Kone driver for Linux
+ *
+ * Copyright (c) 2010 Stefan Achatz <erazor_de@users.sourceforge.net>
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/input.h>
+#include <linux/hid.h>
+#include <linux/usb.h>
+#include <linux/module.h>
+#include "hid-ids.h"
+#include "hid-roccat-kone.h"
+
+static uint16_t kone_calc_profile_checksum(struct kone_profile const *profile)
+{
+	uint16_t checksum = 0;
+	unsigned char *address;
+	int i;
+	for (i = 0, address = (unsigned char *)profile;
+			i < sizeof(struct kone_profile) - 2; ++i, ++address) {
+		checksum += *address;
+	}
+	return checksum;
+}
+
+static uint16_t kone_calc_settings_checksum(
+		struct kone_settings const *settings)
+{
+	uint16_t checksum = 0;
+	unsigned char *address = (unsigned char *)settings;
+	int i;
+
+	for (i = 0; i < sizeof(struct kone_settings) - 2; ++i, ++address)
+		checksum += *address;
+
+	return checksum;
+}
+
+static void kone_set_settings_checksum(struct kone_settings *settings)
+{
+	settings->checksum = cpu_to_le16(kone_calc_settings_checksum(settings));
+}
+
+static void kone_set_profile_checksum(struct kone_profile *profile)
+{
+	profile->checksum = cpu_to_le16(kone_calc_profile_checksum(profile));
+}
+
+static int kone_check_write(struct usb_device *usb_dev)
+{
+	int len;
+	unsigned char *data;
+
+	data = kmalloc(1, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	do {
+		msleep(50);
+
+		len = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
+				USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
+						| USB_RECIP_INTERFACE
+						| USB_DIR_IN,
+				kone_command_confirm_write, 0, data, 1,
+				USB_CTRL_SET_TIMEOUT);
+
+		if (len != 1) {
+			kfree(data);
+			return -EIO;
+		}
+		/*
+		 * value of 3 seems to mean something like
+		 * "not finished yet, but it looks good"
+		 * So check again after a moment.
+		 */
+	} while (*data == 3);
+
+	if (*data == 1) { /* everything alright */
+		kfree(data);
+		return 0;
+	} else { /* unknown answer */
+		dev_err(&usb_dev->dev, "got retval %d when checking write\n",
+				*data);
+		kfree(data);
+		return -EIO;
+	}
+}
+
+static int kone_get_settings(struct usb_device *usb_dev,
+		struct kone_settings *buf)
+{
+	int len;
+
+	len = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
+			USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
+					| USB_RECIP_INTERFACE | USB_DIR_IN,
+			kone_command_settings, 0, buf,
+			sizeof(struct kone_settings), USB_CTRL_SET_TIMEOUT);
+
+	if (len != sizeof(struct kone_settings))
+		return -EIO;
+
+	return 0;
+}
+
+static int kone_set_settings(struct usb_device *usb_dev,
+		struct kone_settings const *buf)
+{
+	struct kone_settings *settings, *original_settings;
+	int len, err;
+
+	settings = kmalloc(sizeof(struct kone_settings), GFP_KERNEL);
+	if (!settings)
+		return -ENOMEM;
+
+	memcpy(settings, buf, sizeof(struct kone_settings));
+
+	kone_set_settings_checksum(settings);
+
+	original_settings = kmalloc(sizeof(struct kone_settings), GFP_KERNEL);
+	if (!original_settings) {
+		kfree(settings);
+		return -ENOMEM;
+	}
+
+	err = kone_get_settings(usb_dev, original_settings);
+	if (err) {
+		kfree(original_settings);
+		kfree(settings);
+		return err;
+	}
+
+	if (memcmp(settings, original_settings, sizeof(struct kone_settings))
+			!= 0) {
+		len = usb_control_msg(usb_dev, usb_sndctrlpipe(usb_dev, 0),
+				USB_REQ_SET_CONFIGURATION, USB_TYPE_CLASS
+						| USB_RECIP_INTERFACE
+						| USB_DIR_OUT,
+				kone_command_settings, 0, settings,
+				sizeof(struct kone_settings),
+				USB_CTRL_SET_TIMEOUT);
+
+		if (len != sizeof(struct kone_settings)) {
+			kfree(original_settings);
+			kfree(settings);
+			return -EIO;
+		}
+
+		if (kone_check_write(usb_dev)) {
+			kfree(original_settings);
+			kfree(settings);
+			return -EIO;
+		}
+	}
+
+	kfree(original_settings);
+	kfree(settings);
+	return 0;
+}
+
+static int kone_get_startup_profile(struct usb_device *usb_dev, int *result)
+{
+	struct kone_settings *buf;
+	int err;
+
+	buf = kmalloc(sizeof(struct kone_settings), GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	err = kone_get_settings(usb_dev, buf);
+	if (err) {
+		kfree(buf);
+		return err;
+	}
+
+	*result = buf->startup_profile;
+	kfree(buf);
+	return 0;
+}
+
+static int kone_get_profile(struct usb_device *usb_dev,
+		struct kone_profile *buf, int number)
+{
+	int len;
+
+	if (number < 1 || number > 5)
+		return -EINVAL;
+
+	len = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
+			USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
+					| USB_RECIP_INTERFACE | USB_DIR_IN,
+			kone_command_profile, number, buf,
+			sizeof(struct kone_profile), USB_CTRL_SET_TIMEOUT);
+
+	if (len != sizeof(struct kone_profile)) {
+		dev_err(&usb_dev->dev, "wrong len %d\n", len);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int kone_set_profile(struct usb_device *usb_dev, char const *buf,
+		int number)
+{
+	struct kone_profile *profile, *original_profile;
+	int len, err, result;
+
+	if (number < 1 || number > 5)
+		return -EINVAL;
+
+	profile = kmalloc(sizeof(struct kone_profile), GFP_KERNEL);
+	if (!profile)
+		return -ENOMEM;
+
+	/* prepare profile to write */
+	memcpy(profile, buf, sizeof(struct kone_profile));
+	profile->profile = number;
+	kone_set_profile_checksum(profile);
+
+	/*
+	 * to reduce unnecessary writes read profile and compare with data
+	 * that should be written
+	 */
+	original_profile = kmalloc(sizeof(struct kone_profile), GFP_KERNEL);
+	if (!original_profile) {
+		kfree(profile);
+		return -ENOMEM;
+	}
+
+	err = kone_get_profile(usb_dev, original_profile, number);
+	if (err) {
+		kfree(original_profile);
+		kfree(profile);
+		return err;
+	}
+
+	result = memcmp(profile, original_profile, sizeof(struct kone_profile));
+	if (result != 0) {
+		len = usb_control_msg(usb_dev, usb_sndctrlpipe(usb_dev, 0),
+				USB_REQ_SET_CONFIGURATION, USB_TYPE_CLASS
+						| USB_RECIP_INTERFACE
+						| USB_DIR_OUT,
+				kone_command_profile, number, profile,
+				sizeof(struct kone_profile),
+				USB_CTRL_SET_TIMEOUT);
+
+		if (len != sizeof(struct kone_profile)) {
+			kfree(original_profile);
+			kfree(profile);
+			return -EIO;
+		}
+
+		if (kone_check_write(usb_dev)) {
+			kfree(original_profile);
+			kfree(profile);
+			return -EIO;
+		}
+	}
+
+	kfree(original_profile);
+	kfree(profile);
+	return 0;
+}
+
+static int kone_get_profile_startup_dpi(struct usb_device *usb_dev, int number,
+		int *result)
+{
+	struct kone_profile *buf;
+	int err;
+
+	buf = kmalloc(sizeof(struct kone_profile), GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	err = kone_get_profile(usb_dev, buf, number);
+	if (err) {
+		kfree(buf);
+		return err;
+	}
+
+	*result = buf->dpi_value;
+	kfree(buf);
+	return 0;
+}
+
+static int kone_get_weight(struct usb_device *usb_dev, int *result)
+{
+	int len;
+	uint8_t *data;
+
+	data = kmalloc(1, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	len = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
+			USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
+					| USB_RECIP_INTERFACE | USB_DIR_IN,
+			kone_command_weight, 0, data, 1, USB_CTRL_SET_TIMEOUT);
+
+	if (len != 1) {
+		kfree(data);
+		return -EIO;
+	}
+	*result = (int)*data;
+	return 0;
+}
+
+static int kone_get_firmware_version(struct usb_device *usb_dev, int *result)
+{
+	int len;
+	unsigned char *data;
+
+	data = kmalloc(2, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	len = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
+			USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
+					| USB_RECIP_INTERFACE | USB_DIR_IN,
+			kone_command_firmware_version, 0, data, 2,
+			USB_CTRL_SET_TIMEOUT);
+
+	if (len != 2) {
+		kfree(data);
+		return -EIO;
+	}
+	*result = (int)*data;
+	return 0;
+}
+
+static ssize_t kone_set_settings_raw(struct device *dev,
+		struct device_attribute *attr, char const *buf, size_t size)
+{
+	struct hid_device *hdev = dev_get_drvdata(dev);
+	struct kone_device *kone = hid_get_drvdata(hdev);
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	int err;
+
+	if (size != sizeof(struct kone_settings))
+		return -EINVAL;
+
+	err = kone_set_settings(usb_dev, (struct kone_settings const *)buf);
+	if (err)
+		return err;
+
+	/*
+	 * If we get here, treat buf as okay (apart from checksum) and use value
+	 * of startup_profile as its at hand
+	 */
+	kone->act_profile = ((struct kone_settings *)buf)->startup_profile;
+	kone->act_profile_valid = 1;
+	kone->act_dpi_valid = 0;
+
+	return sizeof(struct kone_settings);
+}
+
+static ssize_t kone_show_settings_raw(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	int err;
+	err = kone_get_settings(usb_dev, (struct kone_settings *)buf);
+	if (err)
+		return err;
+
+	return sizeof(struct kone_settings);
+}
+
+static ssize_t kone_get_profile_raw(struct device *dev, char *buf, int number)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	int err;
+	err = kone_get_profile(usb_dev, (struct kone_profile *)buf, number);
+	if (err)
+		return err;
+	return sizeof(struct kone_profile);
+}
+
+static ssize_t kone_set_profile_raw(struct device *dev, char const *buf,
+		size_t size, int number)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+
+	int err;
+
+	if (size != sizeof(struct kone_profile))
+		return -EINVAL;
+
+	err = kone_set_profile(usb_dev, buf, number);
+	if (err)
+		return err;
+
+	return sizeof(struct kone_profile);
+}
+
+static ssize_t kone_show_profile_1_raw(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	return kone_get_profile_raw(dev, buf, 1);
+}
+
+static ssize_t kone_set_profile_1_raw(struct device *dev,
+		struct device_attribute *attr, char const *buf, size_t size)
+{
+	return kone_set_profile_raw(dev, buf, size, 1);
+}
+
+static ssize_t kone_show_profile_2_raw(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	return kone_get_profile_raw(dev, buf, 2);
+}
+
+static ssize_t kone_set_profile_2_raw(struct device *dev,
+		struct device_attribute *attr, char const *buf, size_t size)
+{
+	return kone_set_profile_raw(dev, buf, size, 2);
+}
+
+static ssize_t kone_show_profile_3_raw(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	return kone_get_profile_raw(dev, buf, 3);
+}
+
+static ssize_t kone_set_profile_3_raw(struct device *dev,
+		struct device_attribute *attr, char const *buf, size_t size)
+{
+	return kone_set_profile_raw(dev, buf, size, 3);
+}
+
+static ssize_t kone_show_profile_4_raw(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	return kone_get_profile_raw(dev, buf, 4);
+}
+
+static ssize_t kone_set_profile_4_raw(struct device *dev,
+		struct device_attribute *attr, char const *buf, size_t size)
+{
+	return kone_set_profile_raw(dev, buf, size, 4);
+}
+
+static ssize_t kone_show_profile_5_raw(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	return kone_get_profile_raw(dev, buf, 5);
+}
+
+static ssize_t kone_set_profile_5_raw(struct device *dev,
+		struct device_attribute *attr, char const *buf, size_t size)
+{
+	return kone_set_profile_raw(dev, buf, size, 5);
+}
+
+/*
+ * helper to fill kone_device structure with actual values
+ * returns 0 on success or error
+ */
+static int kone_device_set_actual_values(struct kone_device *kone,
+		struct device *dev, int both)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	int err;
+
+	/* first make sure profile is valid */
+	if (!kone->act_profile_valid) {
+		err = kone_get_startup_profile(usb_dev, &kone->act_profile);
+		if (err)
+			return err;
+		kone->act_profile_valid = 1;
+	}
+
+	/* then get startup dpi value */
+	if (!kone->act_dpi_valid && both) {
+		err = kone_get_profile_startup_dpi(usb_dev, kone->act_profile,
+				&kone->act_dpi);
+		if (err)
+			return err;
+		kone->act_dpi_valid = 1;
+	}
+
+	return 0;
+}
+
+static ssize_t kone_show_actual_profile(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct hid_device *hdev = dev_get_drvdata(dev);
+	struct kone_device *kone = hid_get_drvdata(hdev);
+	int err;
+	err = kone_device_set_actual_values(kone, dev, 0);
+	if (err)
+		return err;
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", kone->act_profile);
+}
+
+static ssize_t kone_show_actual_dpi_raw(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct hid_device *hdev = dev_get_drvdata(dev);
+	struct kone_device *kone = hid_get_drvdata(hdev);
+	int err;
+
+	err = kone_device_set_actual_values(kone, dev, 1);
+	if (err)
+		return err;
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", kone->act_dpi);
+}
+
+static ssize_t kone_show_actual_dpi(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct hid_device *hdev = dev_get_drvdata(dev);
+	struct kone_device *kone = hid_get_drvdata(hdev);
+	int err, dpi;
+	err = kone_device_set_actual_values(kone, dev, 1);
+	if (err)
+		return err;
+
+	dpi = kone->act_dpi;
+	switch (dpi) {
+	case 0:
+		break;
+	case 6:
+		dpi = 3200;
+		break;
+	default:
+		dpi = dpi * 400;
+	}
+
+	return snprintf(buf, PAGE_SIZE, "%ddpi\n", dpi);
+}
+
+static ssize_t kone_show_weight_raw(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	int weight;
+	int retval;
+	retval = kone_get_weight(usb_dev, &weight);
+	if (retval)
+		return retval;
+	return snprintf(buf, PAGE_SIZE, "%d\n", weight);
+}
+
+static ssize_t kone_show_firmware_version_raw(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	int firmware_version;
+	int retval;
+	retval = kone_get_firmware_version(usb_dev, &firmware_version);
+	if (retval)
+		return retval;
+	return snprintf(buf, PAGE_SIZE, "%d\n", firmware_version);
+}
+
+static ssize_t kone_show_tcu(struct device *dev, struct device_attribute *attr,
+		char *buf)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	int err, result;
+	struct kone_settings *settings;
+
+	settings = kmalloc(sizeof(struct kone_settings), GFP_KERNEL);
+	if (!settings)
+		return -ENOMEM;
+
+	err = kone_get_settings(usb_dev, settings);
+	if (err) {
+		kfree(settings);
+		return err;
+	}
+
+	result = settings->tcu;
+	kfree(settings);
+	return snprintf(buf, PAGE_SIZE, "%d\n", result);
+}
+
+static int kone_tcu_command(struct usb_device *usb_dev, int number)
+{
+	int len;
+	char *value;
+
+	value = kmalloc(1, GFP_KERNEL);
+	if (!value)
+		return -ENOMEM;
+
+	*value = number;
+
+	len = usb_control_msg(usb_dev, usb_sndctrlpipe(usb_dev, 0),
+			USB_REQ_SET_CONFIGURATION, USB_TYPE_CLASS
+					| USB_RECIP_INTERFACE | USB_DIR_OUT,
+			kone_command_calibrate, 0, value, 1,
+			USB_CTRL_SET_TIMEOUT);
+
+	if (len != 1) {
+		kfree(value);
+		return -EIO;
+	}
+
+	kfree(value);
+	return 0;
+}
+
+/* integer of 0 deactivates tcu, 1 activates it */
+static ssize_t kone_set_tcu(struct device *dev, struct device_attribute *attr,
+		char const *buf, size_t size)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	int err;
+	unsigned long state;
+	struct kone_settings *settings;
+
+	err = strict_strtoul(buf, 10, &state);
+	if (err)
+		return err;
+
+	if (state != 0 && state != 1)
+		return -EINVAL;
+
+	if (state == 1) { /* state activate */
+		err = kone_tcu_command(usb_dev, 1);
+		if (err)
+			return err;
+		err = kone_tcu_command(usb_dev, 2);
+		if (err)
+			return err;
+		ssleep(5); /* tcu needs this time for calibration */
+		err = kone_tcu_command(usb_dev, 3);
+		if (err)
+			return err;
+		err = kone_tcu_command(usb_dev, 0);
+		if (err)
+			return err;
+		err = kone_tcu_command(usb_dev, 4);
+		if (err)
+			return err;
+		/*
+		 * Kone needs this time to settle things.
+		 * Reading settings too early will result in invalid data.
+		 * Roccat's driver waits 1 sec, maybe this time could be
+		 * shortened.
+		 */
+		ssleep(1);
+	}
+
+	settings = kmalloc(sizeof(struct kone_settings), GFP_KERNEL);
+	if (!settings)
+		return -ENOMEM;
+
+	err = kone_get_settings(usb_dev, settings);
+	if (err) {
+		kfree(settings);
+		return err;
+	}
+
+	/* only write settings back if activation state is different */
+	if (settings->tcu != state) {
+		settings->tcu = state;
+
+		err = kone_set_settings(usb_dev, settings);
+		if (err) {
+			kfree(settings);
+			return err;
+		}
+	}
+
+	kfree(settings);
+	return size;
+}
+
+static ssize_t kone_show_startup_profile(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
+	int err, result;
+	err = kone_get_startup_profile(usb_dev, &result);
+	if (err)
+		return err;
+	return snprintf(buf, PAGE_SIZE, "%d\n", result);
+}
+
+static ssize_t kone_set_startup_profile(struct device *dev,
+		struct device_attribute *attr, char const *buf, size_t size)
+{
+	struct hid_device *hdev = dev_get_drvdata(dev);
+	struct kone_device *kone = hid_get_drvdata(hdev);
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	int err;
+	unsigned long new_profile;
+	struct kone_settings *settings;
+
+	err = strict_strtoul(buf, 10, &new_profile);
+	if (err)
+		return err;
+
+	if (new_profile  < 1 || new_profile > 5)
+		return -EINVAL;
+
+	settings = kmalloc(sizeof(struct kone_settings), GFP_KERNEL);
+	if (!settings)
+		return -ENOMEM;
+
+	err = kone_get_settings(usb_dev, settings);
+	if (err) {
+		kfree(settings);
+		return err;
+	}
+
+	settings->startup_profile = new_profile;
+
+	err = kone_set_settings(usb_dev, settings);
+	if (err) {
+		kfree(settings);
+		return err;
+	}
+
+	kone->act_profile = new_profile;
+	kone->act_profile_valid = 1;
+	kone->act_dpi_valid = 0;
+
+	kfree(settings);
+	return size;
+}
+
+static ssize_t kone_show_driver_version(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, DRIVER_VERSION "\n");
+}
+
+static DEVICE_ATTR(actual_dpi_raw, S_IRUGO, kone_show_actual_dpi_raw, NULL);
+static DEVICE_ATTR(actual_dpi, S_IRUGO, kone_show_actual_dpi, NULL);
+static DEVICE_ATTR(actual_profile, S_IRUGO, kone_show_actual_profile, NULL);
+static DEVICE_ATTR(weight_raw, S_IRUGO, kone_show_weight_raw, NULL);
+static DEVICE_ATTR(profile1_raw, S_IRUGO | S_IWUGO,
+		kone_show_profile_1_raw, kone_set_profile_1_raw);
+static DEVICE_ATTR(profile2_raw, S_IRUGO | S_IWUGO,
+		kone_show_profile_2_raw, kone_set_profile_2_raw);
+static DEVICE_ATTR(profile3_raw, S_IRUGO | S_IWUGO,
+		kone_show_profile_3_raw, kone_set_profile_3_raw);
+static DEVICE_ATTR(profile4_raw, S_IRUGO | S_IWUGO,
+		kone_show_profile_4_raw, kone_set_profile_4_raw);
+static DEVICE_ATTR(profile5_raw, S_IRUGO | S_IWUGO,
+		kone_show_profile_5_raw, kone_set_profile_5_raw);
+static DEVICE_ATTR(settings_raw, S_IRUGO | S_IWUGO,
+		kone_show_settings_raw, kone_set_settings_raw);
+static DEVICE_ATTR(firmware_version_raw, S_IRUGO,
+		kone_show_firmware_version_raw, NULL);
+static DEVICE_ATTR(tcu, S_IRUGO | S_IWUGO, kone_show_tcu, kone_set_tcu);
+static DEVICE_ATTR(startup_profile, S_IRUGO | S_IWUGO,
+		kone_show_startup_profile, kone_set_startup_profile);
+static DEVICE_ATTR(kone_driver_version, S_IRUGO,
+		kone_show_driver_version, NULL);
+
+static struct attribute *kone_attributes[] = {
+		&dev_attr_actual_dpi_raw.attr,
+		&dev_attr_actual_dpi.attr,
+		&dev_attr_actual_profile.attr,
+		&dev_attr_weight_raw.attr,
+		&dev_attr_profile1_raw.attr,
+		&dev_attr_profile2_raw.attr,
+		&dev_attr_profile3_raw.attr,
+		&dev_attr_profile4_raw.attr,
+		&dev_attr_profile5_raw.attr,
+		&dev_attr_settings_raw.attr,
+		&dev_attr_firmware_version_raw.attr,
+		&dev_attr_tcu.attr,
+		&dev_attr_startup_profile.attr,
+		&dev_attr_kone_driver_version.attr,
+		NULL
+};
+
+static struct attribute_group kone_attribute_group = {
+		.attrs = kone_attributes
+};
+
+static int kone_create_files(struct usb_interface *intf)
+{
+	if (intf->cur_altsetting->desc.bInterfaceProtocol
+			== USB_INTERFACE_PROTOCOL_MOUSE)
+		return sysfs_create_group(&intf->dev.kobj,
+				&kone_attribute_group);
+	else
+		return 0;
+}
+
+static void kone_remove_files(struct usb_interface *intf)
+{
+	if (intf->cur_altsetting->desc.bInterfaceProtocol
+			== USB_INTERFACE_PROTOCOL_MOUSE)
+		sysfs_remove_group(&intf->dev.kobj, &kone_attribute_group);
+}
+
+static int kone_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+	struct kone_device *kone;
+	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
+	int ret;
+
+	kone = kzalloc(sizeof(*kone), GFP_KERNEL);
+	if (!kone) {
+		dev_err(&hdev->dev, "can't alloc device descriptor\n");
+		return -ENOMEM;
+	}
+
+	hid_set_drvdata(hdev, kone);
+	ret = hid_parse(hdev);
+	if (ret) {
+		dev_err(&hdev->dev, "parse failed\n");
+		goto err_free;
+	}
+
+	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+	if (ret) {
+		dev_err(&hdev->dev, "hw start failed\n");
+		goto err_free;
+	}
+
+	ret = kone_create_files(intf);
+	if (ret) {
+		dev_err(&hdev->dev, "cannot create sysfs files\n");
+		goto err_stop;
+	}
+
+	return 0;
+err_stop:
+	hid_hw_stop(hdev);
+err_free:
+	kfree(kone);
+	return ret;
+}
+
+static void kone_remove(struct hid_device *hdev)
+{
+	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
+	kone_remove_files(intf);
+	hid_hw_stop(hdev);
+	kfree(hid_get_drvdata(hdev));
+}
+
+static int kone_raw_event(struct hid_device *hdev, struct hid_report *report,
+		u8 *data, int size)
+{
+	struct kone_device *kone = hid_get_drvdata(hdev);
+	struct kone_mouse_event *event = (struct kone_mouse_event *)data;
+
+	/* keyboard events should be processed by default handler */
+	if (size != 12)
+		return 0;
+
+	/* Firmware 1.38 introduced new behaviour for tilt buttons.
+	 * Pressed tilt button is reported in each movement event.
+	 * Workaround sends only one event per press.
+	 */
+	if (kone->last_tilt_state == event->tilt)
+		event->tilt = 0;
+	else
+		kone->last_tilt_state = event->tilt;
+
+	switch (event->event) {
+	case kone_mouse_event_osd_dpi:
+		kone->act_dpi = event->value;
+		kone->act_dpi_valid = 1;
+		dev_dbg(&hdev->dev, "osd dpi event. actual dpi %d (and %d)\n",
+				event->value, kone->act_dpi);
+		return 1; /* return 1 if event was handled */
+	case kone_mouse_event_switch_dpi:
+		kone->act_dpi = event->value;
+		kone->act_dpi_valid = 1;
+		dev_dbg(&hdev->dev, "switched dpi to %d\n", event->value);
+		return 1;
+	case kone_mouse_event_osd_profile:
+		kone->act_profile = event->value;
+		kone->act_profile_valid = 1;
+		dev_dbg(&hdev->dev, "osd profile event. actual profile %d\n",
+				event->value);
+		return 1;
+	case kone_mouse_event_switch_profile:
+		kone->act_profile = event->value;
+		kone->act_profile_valid = 1;
+		kone->act_dpi_valid = 0;
+		dev_dbg(&hdev->dev, "switched profile to %d\n", event->value);
+		return 1;
+	case kone_mouse_event_call_overlong_macro:
+		dev_dbg(&hdev->dev, "overlong macro called %d\n", event->macro);
+		return 1;
+	}
+
+	return 0; /* do further processing */
+}
+
+static const struct hid_device_id kone_devices[] = { { HID_USB_DEVICE(
+		USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_KONE) }, { } };
+MODULE_DEVICE_TABLE(hid, kone_devices);
+
+static struct hid_driver kone_driver = { .name = "kone",
+		.id_table = kone_devices, .probe = kone_probe,
+		.remove = kone_remove, .raw_event = kone_raw_event, };
+
+static int kone_init(void)
+{
+	return hid_register_driver(&kone_driver);
+}
+
+static void kone_exit(void)
+{
+	hid_unregister_driver(&kone_driver);
+}
+
+module_init(kone_init);
+module_exit(kone_exit);
+
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE(DRIVER_LICENSE);
diff --git a/drivers/hid/hid-roccat-kone.h b/drivers/hid/hid-roccat-kone.h
new file mode 100644
index 0000000..14f5ebf
--- /dev/null
+++ b/drivers/hid/hid-roccat-kone.h
@@ -0,0 +1,187 @@
+#ifndef __HID_ROCCAT_KONE_H
+#define __HID_ROCCAT_KONE_H
+
+/*
+ * Copyright (c) 2010 Stefan Achatz <erazor_de@users.sourceforge.net>
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+#include <linux/types.h>
+
+#define DRIVER_VERSION "v0.2.4"
+#define DRIVER_AUTHOR "Stefan Achatz"
+#define DRIVER_DESC "USB Roccat Kone driver"
+#define DRIVER_LICENSE "GPL v2"
+
+struct kone_device {
+	/*
+	 * Actual values might not get called that much so I store them when
+	 * they are at hand or get them only when needed.
+	 */
+	int act_profile, act_profile_valid;
+	int act_dpi, act_dpi_valid;
+	int last_tilt_state;
+};
+
+#pragma pack(push)
+#pragma pack(1)
+
+struct kone_keystroke {
+	uint8_t key;
+	uint8_t action;
+	uint16_t period; /* in milliseconds */
+};
+
+enum kone_keystroke_buttons {
+	kone_keystroke_button_1 = 0xf0, /* left mouse button */
+	kone_keystroke_button_2 = 0xf1, /* right mouse button */
+	kone_keystroke_button_3 = 0xf2, /* wheel */
+	kone_keystroke_button_9 = 0xf3, /* side button up  \todo confirm */
+	kone_keystroke_button_8 = 0xf4 /* side button down \todo confirm */
+};
+
+enum kone_keystroke_actions {
+	kone_keystroke_action_press = 0,
+	kone_keystroke_action_release = 1
+};
+
+struct kone_button_info {
+	uint8_t number; /* range 1-8 */
+	uint8_t type;
+	uint8_t macro_type; /* 0 = short, 1 = overlong */
+	uint8_t macro_name[16]; /* can be max 15 chars long */
+	uint8_t set_name[16]; /* can be max 15 chars long */
+	uint8_t count;
+	struct kone_keystroke keystroke[20];
+};
+
+enum kone_button_info_types {
+	/* valid button types until firmware 1.32 */
+	kone_button_info_type_button_1 = 0x1, /* click (left mouse button) */
+	kone_button_info_type_button_2 = 0x2, /* menu (right mouse button)*/
+	kone_button_info_type_button_3 = 0x3, /* scroll (wheel) */
+	kone_button_info_type_double_click = 0x4,
+	kone_button_info_type_key = 0x5,
+	kone_button_info_type_macro = 0x6,
+	kone_button_info_type_off = 0x7,
+	/* TODO clarify function and rename */
+	kone_button_info_type_osd_xy_prescaling = 0x8,
+	kone_button_info_type_osd_dpi = 0x9,
+	kone_button_info_type_osd_profile = 0xa,
+	kone_button_info_type_button_9 = 0xb, /* ie forward */
+	kone_button_info_type_button_8 = 0xc, /* ie backward */
+	kone_button_info_type_dpi_up = 0xd, /* internal */
+	kone_button_info_type_dpi_down = 0xe, /* internal */
+	kone_button_info_type_button_7 = 0xf, /* tilt left */
+	kone_button_info_type_button_6 = 0x10, /* tilt right */
+	kone_button_info_type_profile_up = 0x11, /* internal */
+	kone_button_info_type_profile_down = 0x12, /* internal */
+	kone_button_info_type_overlong_macro = 0x106,
+	/* additional valid button types since firmware 1.38 */
+	kone_button_info_type_multimedia_open_player = 0x20,
+	kone_button_info_type_multimedia_next_track = 0x21,
+	kone_button_info_type_multimedia_prev_track = 0x22,
+	kone_button_info_type_multimedia_play_pause = 0x23,
+	kone_button_info_type_multimedia_stop = 0x24,
+	kone_button_info_type_multimedia_mute = 0x25,
+	kone_button_info_type_multimedia_volume_up = 0x26,
+	kone_button_info_type_multimedia_volume_down = 0x27
+};
+
+struct kone_light_info {
+	uint8_t number; /* number of light 1-5 */
+	uint8_t mod;   /* 1 = on, 2 = off */
+	uint8_t red;   /* range 0x00-0xff */
+	uint8_t green; /* range 0x00-0xff */
+	uint8_t blue;  /* range 0x00-0xff */
+};
+
+struct kone_profile {
+	uint16_t size; /* always 975 */
+	uint16_t unused; /* always 0 */
+	uint8_t profile; /* range 1-5 */
+	uint16_t main_sensitivity; /* range 100-1000 */
+	uint8_t xy_sensitivity_enabled; /* 1 = on, 2 = off */
+	uint16_t x_sensitivity; /* range 100-1000 */
+	uint16_t y_sensitivity; /* range 100-1000 */
+	uint8_t dpi_rate; /* bit 1 = 800, ... */
+	uint8_t dpi_value; /* range 1-6 */
+	uint8_t polling_rate; /* 1 = 125Hz, 2 = 500Hz, 3 = 1000Hz */
+	/* kone has no dcu
+	 * value is always 2 in firmwares <= 1.32 and
+	 * 1 in firmwares > 1.32
+	 */
+	uint8_t dcu_flag;
+	uint8_t light_effect_1; /* range 1-3 */
+	uint8_t light_effect_2; /* range 1-5 */
+	uint8_t light_effect_3; /* range 1-4 */
+	uint8_t light_effect_speed; /* range 0-255 */
+
+	struct kone_light_info light_info[5];
+	struct kone_button_info button_info[8];
+
+	uint16_t checksum; /* \brief holds checksum of struct */
+};
+
+enum kone_polling_rates {
+	kone_polling_rate_125 = 1,
+	kone_polling_rate_500 = 2,
+	kone_polling_rate_1000 = 3
+};
+
+struct kone_settings {
+	uint16_t size; /* always 36 */
+	uint8_t  startup_profile; /* 1-5 */
+	uint8_t	 unknown1;
+	uint8_t  tcu; /* 0 = off, 1 = on */
+	uint8_t  unknown2[23];
+	uint8_t  calibration_data[4];
+	uint8_t  unknown3[2];
+	uint16_t checksum;
+};
+
+/*
+ * 12 byte mouse event read by interrupt_read
+ */
+struct kone_mouse_event {
+	uint8_t report_number; /* always 1 */
+	uint8_t button;
+	uint16_t x;
+	uint16_t y;
+	uint8_t wheel; /* up = 1, down = -1 */
+	uint8_t tilt; /* right = 1, left = -1 */
+	uint8_t unknown;
+	uint8_t event;
+	uint8_t value;
+	uint8_t macro;
+};
+
+enum kone_mouse_events {
+	kone_mouse_event_osd_dpi = 0xa0,
+	kone_mouse_event_osd_profile = 0xb0,
+	/* TODO clarify meaning and occurence of kone_mouse_event_calibration */
+	kone_mouse_event_calibration = 0xc0,
+	kone_mouse_event_call_overlong_macro = 0xe0,
+	kone_mouse_event_switch_dpi = 0xf0,
+	kone_mouse_event_switch_profile = 0xf1
+};
+
+enum kone_commands {
+	kone_command_profile = 0x5a,
+	kone_command_settings = 0x15a,
+	kone_command_firmware_version = 0x25a,
+	kone_command_weight = 0x45a,
+	kone_command_calibrate = 0x55a,
+	kone_command_confirm_write = 0x65a,
+	kone_command_firmware = 0xe5a
+};
+
+#pragma pack(pop)
+
+#endif
-- 
1.6.6


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] HID: add driver for Roccat Kone gaming mouse
  2010-02-20  8:11 ` Stefan Achatz
  (?)
@ 2010-02-21  8:13 ` Dmitry Torokhov
  2010-02-21 16:50   ` [PATCH 2/2] HID: documentation additions and elimination of legacy filenames for Roccat Kone driver Stefan Achatz
  -1 siblings, 1 reply; 17+ messages in thread
From: Dmitry Torokhov @ 2010-02-21  8:13 UTC (permalink / raw)
  To: Stefan Achatz
  Cc: Jiri Kosina, Jussi Kivilinna, wylda, Pavel Machek,
	Alessandro Guido, Tomas Hanak, Jason Noble, simon.windows,
	Sean Hildebrand, Sid Boyce, Henning Glawe, linux-input,
	linux-kernel

Hi Stefan,

On Sat, Feb 20, 2010 at 09:11:53AM +0100, Stefan Achatz wrote:
> This Patch adds support for Kone gaming mouse from Roccat.
> It provides access to profiles, settings, firmware, weight,
> actual settings etc. through sysfs files.

There is no documentation provided for all these new sysfs attributes
so it is really hard to judge their usefullness. For example why would
one care about weight?

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/2] HID: documentation additions and elimination of legacy filenames for Roccat Kone driver
  2010-02-21  8:13 ` Dmitry Torokhov
@ 2010-02-21 16:50   ` Stefan Achatz
  2010-02-22  6:29     ` Dmitry Torokhov
  0 siblings, 1 reply; 17+ messages in thread
From: Stefan Achatz @ 2010-02-21 16:50 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Jiri Kosina, Jussi Kivilinna, wylda, Pavel Machek,
	Alessandro Guido, Tomas Hanak, Jason Noble, simon.windows,
	Sean Hildebrand, Sid Boyce, Henning Glawe, linux-input,
	linux-kernel

 This patch adds more documentation and renames sysfs attributes to eliminate my legacy naming.
 Signed-off-by: Stefan Achatz <erazor_de@users.sourceforge.net>

---
 drivers/hid/hid-roccat-kone.c |  265 +++++++++++++++++++++++++++--------------
 drivers/hid/hid-roccat-kone.h |    7 +-
 2 files changed, 178 insertions(+), 94 deletions(-)

diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
index 94a2fb9..941f5b3 100644
--- a/drivers/hid/hid-roccat-kone.c
+++ b/drivers/hid/hid-roccat-kone.c
@@ -54,6 +54,11 @@ static void kone_set_profile_checksum(struct kone_profile *profile)
 	profile->checksum = cpu_to_le16(kone_calc_profile_checksum(profile));
 }
 
+/*
+ * Checks success after writing data to mouse
+ * On success returns 0
+ * On failure returns errno
+ */
 static int kone_check_write(struct usb_device *usb_dev)
 {
 	int len;
@@ -95,6 +100,11 @@ static int kone_check_write(struct usb_device *usb_dev)
 	}
 }
 
+/*
+ * Reads settings data from mouse and stores it in @buf
+ * On success returns 0
+ * On failure returns errno
+ */
 static int kone_get_settings(struct usb_device *usb_dev,
 		struct kone_settings *buf)
 {
@@ -112,6 +122,13 @@ static int kone_get_settings(struct usb_device *usb_dev,
 	return 0;
 }
 
+/*
+ * Write settings to mouse
+ * Reads and compares stored data with new data to prevent needless write
+ * Sets checksum of supplied data
+ * On success returns 0
+ * On failure returns errno
+ */
 static int kone_set_settings(struct usb_device *usb_dev,
 		struct kone_settings const *buf)
 {
@@ -167,6 +184,11 @@ static int kone_set_settings(struct usb_device *usb_dev,
 	return 0;
 }
 
+/*
+ * Stores number of startup profile in @result
+ * On success returns 0
+ * On failure returns errno
+ */
 static int kone_get_startup_profile(struct usb_device *usb_dev, int *result)
 {
 	struct kone_settings *buf;
@@ -187,6 +209,12 @@ static int kone_get_startup_profile(struct usb_device *usb_dev, int *result)
 	return 0;
 }
 
+/*
+ * Reads profile data from mouse and stores it in @buf
+ * @number: profile number to read
+ * On success returns 0
+ * On failure returns errno
+ */
 static int kone_get_profile(struct usb_device *usb_dev,
 		struct kone_profile *buf, int number)
 {
@@ -195,6 +223,11 @@ static int kone_get_profile(struct usb_device *usb_dev,
 	if (number < 1 || number > 5)
 		return -EINVAL;
 
+	/*
+	 * The manufacturer uses a control message of type class with interface
+	 * as recipient and provides the profile number as index parameter.
+	 * This is prevented in userspace by function check_ctrlrecip.
+	 */
 	len = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
 			USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
 					| USB_RECIP_INTERFACE | USB_DIR_IN,
@@ -209,6 +242,15 @@ static int kone_get_profile(struct usb_device *usb_dev,
 	return 0;
 }
 
+/*
+ * Writes profile data to mouse
+ * Reads and compares stored data with new one to reduce unnecessary writes.
+ * Sets profile number and checksum of data, so one can copy profile data
+ * from one profile to another without usage of a userland tool.
+ * @number: profile number to write
+ * On success returns 0
+ * On failure returns errno
+ */
 static int kone_set_profile(struct usb_device *usb_dev, char const *buf,
 		int number)
 {
@@ -272,6 +314,11 @@ static int kone_set_profile(struct usb_device *usb_dev, char const *buf,
 	return 0;
 }
 
+/*
+ * Reads dpi setting from startup profile and stores it in @result
+ * On success returns 0
+ * On failure returns errno
+ */
 static int kone_get_profile_startup_dpi(struct usb_device *usb_dev, int number,
 		int *result)
 {
@@ -293,6 +340,11 @@ static int kone_get_profile_startup_dpi(struct usb_device *usb_dev, int number,
 	return 0;
 }
 
+/*
+ * Reads value of "fast-clip-weight" and stores it in @result
+ * On success returns 0
+ * On failure returns errno
+ */
 static int kone_get_weight(struct usb_device *usb_dev, int *result)
 {
 	int len;
@@ -315,6 +367,11 @@ static int kone_get_weight(struct usb_device *usb_dev, int *result)
 	return 0;
 }
 
+/*
+ * Reads firmware_version of mouse and stores it in @result
+ * On success returns 0
+ * On failure returns errno
+ */
 static int kone_get_firmware_version(struct usb_device *usb_dev, int *result)
 {
 	int len;
@@ -338,7 +395,7 @@ static int kone_get_firmware_version(struct usb_device *usb_dev, int *result)
 	return 0;
 }
 
-static ssize_t kone_set_settings_raw(struct device *dev,
+static ssize_t kone_sysfs_set_settings(struct device *dev,
 		struct device_attribute *attr, char const *buf, size_t size)
 {
 	struct hid_device *hdev = dev_get_drvdata(dev);
@@ -365,7 +422,7 @@ static ssize_t kone_set_settings_raw(struct device *dev,
 	return sizeof(struct kone_settings);
 }
 
-static ssize_t kone_show_settings_raw(struct device *dev,
+static ssize_t kone_sysfs_show_settings(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct usb_interface *intf = to_usb_interface(dev);
@@ -378,7 +435,7 @@ static ssize_t kone_show_settings_raw(struct device *dev,
 	return sizeof(struct kone_settings);
 }
 
-static ssize_t kone_get_profile_raw(struct device *dev, char *buf, int number)
+static ssize_t kone_sysfs_get_profile(struct device *dev, char *buf, int number)
 {
 	struct usb_interface *intf = to_usb_interface(dev);
 	struct usb_device *usb_dev = interface_to_usbdev(intf);
@@ -389,7 +446,7 @@ static ssize_t kone_get_profile_raw(struct device *dev, char *buf, int number)
 	return sizeof(struct kone_profile);
 }
 
-static ssize_t kone_set_profile_raw(struct device *dev, char const *buf,
+static ssize_t kone_sysfs_set_profile(struct device *dev, char const *buf,
 		size_t size, int number)
 {
 	struct usb_interface *intf = to_usb_interface(dev);
@@ -407,69 +464,70 @@ static ssize_t kone_set_profile_raw(struct device *dev, char const *buf,
 	return sizeof(struct kone_profile);
 }
 
-static ssize_t kone_show_profile_1_raw(struct device *dev,
+static ssize_t kone_sysfs_show_profile_1(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	return kone_get_profile_raw(dev, buf, 1);
+	return kone_sysfs_get_profile(dev, buf, 1);
 }
 
-static ssize_t kone_set_profile_1_raw(struct device *dev,
+static ssize_t kone_sysfs_set_profile_1(struct device *dev,
 		struct device_attribute *attr, char const *buf, size_t size)
 {
-	return kone_set_profile_raw(dev, buf, size, 1);
+	return kone_sysfs_set_profile(dev, buf, size, 1);
 }
 
-static ssize_t kone_show_profile_2_raw(struct device *dev,
+static ssize_t kone_sysfs_show_profile_2(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	return kone_get_profile_raw(dev, buf, 2);
+	return kone_sysfs_get_profile(dev, buf, 2);
 }
 
-static ssize_t kone_set_profile_2_raw(struct device *dev,
+static ssize_t kone_sysfs_set_profile_2(struct device *dev,
 		struct device_attribute *attr, char const *buf, size_t size)
 {
-	return kone_set_profile_raw(dev, buf, size, 2);
+	return kone_sysfs_set_profile(dev, buf, size, 2);
 }
 
-static ssize_t kone_show_profile_3_raw(struct device *dev,
+static ssize_t kone_sysfs_show_profile_3(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	return kone_get_profile_raw(dev, buf, 3);
+	return kone_sysfs_get_profile(dev, buf, 3);
 }
 
-static ssize_t kone_set_profile_3_raw(struct device *dev,
+static ssize_t kone_sysfs_set_profile_3(struct device *dev,
 		struct device_attribute *attr, char const *buf, size_t size)
 {
-	return kone_set_profile_raw(dev, buf, size, 3);
+	return kone_sysfs_set_profile(dev, buf, size, 3);
 }
 
-static ssize_t kone_show_profile_4_raw(struct device *dev,
+static ssize_t kone_sysfs_show_profile_4(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	return kone_get_profile_raw(dev, buf, 4);
+	return kone_sysfs_get_profile(dev, buf, 4);
 }
 
-static ssize_t kone_set_profile_4_raw(struct device *dev,
+static ssize_t kone_sysfs_set_profile_4(struct device *dev,
 		struct device_attribute *attr, char const *buf, size_t size)
 {
-	return kone_set_profile_raw(dev, buf, size, 4);
+	return kone_sysfs_set_profile(dev, buf, size, 4);
 }
 
-static ssize_t kone_show_profile_5_raw(struct device *dev,
+static ssize_t kone_sysfs_show_profile_5(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	return kone_get_profile_raw(dev, buf, 5);
+	return kone_sysfs_get_profile(dev, buf, 5);
 }
 
-static ssize_t kone_set_profile_5_raw(struct device *dev,
+static ssize_t kone_sysfs_set_profile_5(struct device *dev,
 		struct device_attribute *attr, char const *buf, size_t size)
 {
-	return kone_set_profile_raw(dev, buf, size, 5);
+	return kone_sysfs_set_profile(dev, buf, size, 5);
 }
 
 /*
- * helper to fill kone_device structure with actual values
- * returns 0 on success or error
+ * Helper to fill kone_device structure with actual values
+ * On success returns 0
+ * On failure returns errno
  */
 static int kone_device_set_actual_values(struct kone_device *kone,
 		struct device *dev, int both)
@@ -498,7 +556,7 @@ static int kone_device_set_actual_values(struct kone_device *kone,
 	return 0;
 }
 
-static ssize_t kone_show_actual_profile(struct device *dev,
+static ssize_t kone_sysfs_show_actual_profile(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct hid_device *hdev = dev_get_drvdata(dev);
@@ -511,7 +569,7 @@ static ssize_t kone_show_actual_profile(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", kone->act_profile);
 }
 
-static ssize_t kone_show_actual_dpi_raw(struct device *dev,
+static ssize_t kone_sysfs_show_actual_dpi(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct hid_device *hdev = dev_get_drvdata(dev);
@@ -525,31 +583,7 @@ static ssize_t kone_show_actual_dpi_raw(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", kone->act_dpi);
 }
 
-static ssize_t kone_show_actual_dpi(struct device *dev,
-		struct device_attribute *attr, char *buf)
-{
-	struct hid_device *hdev = dev_get_drvdata(dev);
-	struct kone_device *kone = hid_get_drvdata(hdev);
-	int err, dpi;
-	err = kone_device_set_actual_values(kone, dev, 1);
-	if (err)
-		return err;
-
-	dpi = kone->act_dpi;
-	switch (dpi) {
-	case 0:
-		break;
-	case 6:
-		dpi = 3200;
-		break;
-	default:
-		dpi = dpi * 400;
-	}
-
-	return snprintf(buf, PAGE_SIZE, "%ddpi\n", dpi);
-}
-
-static ssize_t kone_show_weight_raw(struct device *dev,
+static ssize_t kone_sysfs_show_weight(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct usb_interface *intf = to_usb_interface(dev);
@@ -562,7 +596,7 @@ static ssize_t kone_show_weight_raw(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", weight);
 }
 
-static ssize_t kone_show_firmware_version_raw(struct device *dev,
+static ssize_t kone_sysfs_show_firmware_version(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct usb_interface *intf = to_usb_interface(dev);
@@ -575,8 +609,8 @@ static ssize_t kone_show_firmware_version_raw(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", firmware_version);
 }
 
-static ssize_t kone_show_tcu(struct device *dev, struct device_attribute *attr,
-		char *buf)
+static ssize_t kone_sysfs_show_tcu(struct device *dev,
+		struct device_attribute *attr, char *buf)
 {
 	struct usb_interface *intf = to_usb_interface(dev);
 	struct usb_device *usb_dev = interface_to_usbdev(intf);
@@ -624,9 +658,8 @@ static int kone_tcu_command(struct usb_device *usb_dev, int number)
 	return 0;
 }
 
-/* integer of 0 deactivates tcu, 1 activates it */
-static ssize_t kone_set_tcu(struct device *dev, struct device_attribute *attr,
-		char const *buf, size_t size)
+static ssize_t kone_sysfs_set_tcu(struct device *dev,
+		struct device_attribute *attr, char const *buf, size_t size)
 {
 	struct usb_interface *intf = to_usb_interface(dev);
 	struct usb_device *usb_dev = interface_to_usbdev(intf);
@@ -692,7 +725,7 @@ static ssize_t kone_set_tcu(struct device *dev, struct device_attribute *attr,
 	return size;
 }
 
-static ssize_t kone_show_startup_profile(struct device *dev,
+static ssize_t kone_sysfs_show_startup_profile(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
@@ -703,7 +736,7 @@ static ssize_t kone_show_startup_profile(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%d\n", result);
 }
 
-static ssize_t kone_set_startup_profile(struct device *dev,
+static ssize_t kone_sysfs_set_startup_profile(struct device *dev,
 		struct device_attribute *attr, char const *buf, size_t size)
 {
 	struct hid_device *hdev = dev_get_drvdata(dev);
@@ -747,48 +780,85 @@ static ssize_t kone_set_startup_profile(struct device *dev,
 	return size;
 }
 
-static ssize_t kone_show_driver_version(struct device *dev,
+/*
+ * This file is used by userland software to find devices that are handled by
+ * this driver. This provides a consistent way for actual and older kernels
+ * where this driver replaced usbhid instead of generic-usb.
+ * Driver capabilities are determined by version number.
+ */
+static ssize_t kone_sysfs_show_driver_version(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	return snprintf(buf, PAGE_SIZE, DRIVER_VERSION "\n");
 }
 
-static DEVICE_ATTR(actual_dpi_raw, S_IRUGO, kone_show_actual_dpi_raw, NULL);
-static DEVICE_ATTR(actual_dpi, S_IRUGO, kone_show_actual_dpi, NULL);
-static DEVICE_ATTR(actual_profile, S_IRUGO, kone_show_actual_profile, NULL);
-static DEVICE_ATTR(weight_raw, S_IRUGO, kone_show_weight_raw, NULL);
-static DEVICE_ATTR(profile1_raw, S_IRUGO | S_IWUGO,
-		kone_show_profile_1_raw, kone_set_profile_1_raw);
-static DEVICE_ATTR(profile2_raw, S_IRUGO | S_IWUGO,
-		kone_show_profile_2_raw, kone_set_profile_2_raw);
-static DEVICE_ATTR(profile3_raw, S_IRUGO | S_IWUGO,
-		kone_show_profile_3_raw, kone_set_profile_3_raw);
-static DEVICE_ATTR(profile4_raw, S_IRUGO | S_IWUGO,
-		kone_show_profile_4_raw, kone_set_profile_4_raw);
-static DEVICE_ATTR(profile5_raw, S_IRUGO | S_IWUGO,
-		kone_show_profile_5_raw, kone_set_profile_5_raw);
-static DEVICE_ATTR(settings_raw, S_IRUGO | S_IWUGO,
-		kone_show_settings_raw, kone_set_settings_raw);
-static DEVICE_ATTR(firmware_version_raw, S_IRUGO,
-		kone_show_firmware_version_raw, NULL);
-static DEVICE_ATTR(tcu, S_IRUGO | S_IWUGO, kone_show_tcu, kone_set_tcu);
+/*
+ * Read actual dpi settings.
+ * Returns raw value for further processing. Refer to enum kone_polling_rates to
+ * get real value.
+ */
+static DEVICE_ATTR(actual_dpi, S_IRUGO, kone_sysfs_show_actual_dpi, NULL);
+
+static DEVICE_ATTR(actual_profile, S_IRUGO, kone_sysfs_show_actual_profile,
+		NULL);
+
+/*
+ * The mouse can be equipped with one of four supplied weights from 5 to 20
+ * grams which are recognized and its value can be read out.
+ * This returns the raw value reported by the mouse for easy evaluation by
+ * software. Refer to enum kone_weights to get corresponding real weight.
+ */
+static DEVICE_ATTR(weight, S_IRUGO, kone_sysfs_show_weight, NULL);
+
+static DEVICE_ATTR(profile1, S_IRUGO | S_IWUGO,
+		kone_sysfs_show_profile_1, kone_sysfs_set_profile_1);
+static DEVICE_ATTR(profile2, S_IRUGO | S_IWUGO,
+		kone_sysfs_show_profile_2, kone_sysfs_set_profile_2);
+static DEVICE_ATTR(profile3, S_IRUGO | S_IWUGO,
+		kone_sysfs_show_profile_3, kone_sysfs_set_profile_3);
+static DEVICE_ATTR(profile4, S_IRUGO | S_IWUGO,
+		kone_sysfs_show_profile_4, kone_sysfs_set_profile_4);
+static DEVICE_ATTR(profile5, S_IRUGO | S_IWUGO,
+		kone_sysfs_show_profile_5, kone_sysfs_set_profile_5);
+
+static DEVICE_ATTR(settings, S_IRUGO | S_IWUGO,
+		kone_sysfs_show_settings, kone_sysfs_set_settings);
+
+/*
+ * Prints firmware version stored in mouse as integer.
+ * The raw value reported by the mouse is returned for easy evaluation, to get
+ * the real version number the decimal point has to be shifted 2 positions to
+ * the left. E.g. a value of 138 means 1.38.
+ */
+static DEVICE_ATTR(firmware_version, S_IRUGO,
+		kone_sysfs_show_firmware_version, NULL);
+
+/*
+ * Prints state of Tracking Control Unit as number where 0 = off and 1 = on
+ * Writing 0 deactivates tcu and writing 1 calibrates and activates the tcu
+ */
+static DEVICE_ATTR(tcu, S_IRUGO | S_IWUGO, kone_sysfs_show_tcu,
+		kone_sysfs_set_tcu);
+
+/* Prints and takes the number of the profile the mouse starts with */
 static DEVICE_ATTR(startup_profile, S_IRUGO | S_IWUGO,
-		kone_show_startup_profile, kone_set_startup_profile);
+		kone_sysfs_show_startup_profile,
+		kone_sysfs_set_startup_profile);
+
 static DEVICE_ATTR(kone_driver_version, S_IRUGO,
-		kone_show_driver_version, NULL);
+		kone_sysfs_show_driver_version, NULL);
 
 static struct attribute *kone_attributes[] = {
-		&dev_attr_actual_dpi_raw.attr,
 		&dev_attr_actual_dpi.attr,
 		&dev_attr_actual_profile.attr,
-		&dev_attr_weight_raw.attr,
-		&dev_attr_profile1_raw.attr,
-		&dev_attr_profile2_raw.attr,
-		&dev_attr_profile3_raw.attr,
-		&dev_attr_profile4_raw.attr,
-		&dev_attr_profile5_raw.attr,
-		&dev_attr_settings_raw.attr,
-		&dev_attr_firmware_version_raw.attr,
+		&dev_attr_weight.attr,
+		&dev_attr_profile1.attr,
+		&dev_attr_profile2.attr,
+		&dev_attr_profile3.attr,
+		&dev_attr_profile4.attr,
+		&dev_attr_profile5.attr,
+		&dev_attr_settings.attr,
+		&dev_attr_firmware_version.attr,
 		&dev_attr_tcu.attr,
 		&dev_attr_startup_profile.attr,
 		&dev_attr_kone_driver_version.attr,
@@ -801,6 +871,13 @@ static struct attribute_group kone_attribute_group = {
 
 static int kone_create_files(struct usb_interface *intf)
 {
+	/*
+	 * Kone consists of a mouse and a keyboard part.
+	 * Adding sysfs files only to mousepart as information about
+	 * profile and dpi change is reported only in mouseevent.
+	 * There is no way to bind only to mousepart since IGNORE_MOUSE quirk
+	 * moved to hid-apple
+	 */
 	if (intf->cur_altsetting->desc.bInterfaceProtocol
 			== USB_INTERFACE_PROTOCOL_MOUSE)
 		return sysfs_create_group(&intf->dev.kobj,
@@ -882,6 +959,10 @@ static int kone_raw_event(struct hid_device *hdev, struct hid_report *report,
 	else
 		kone->last_tilt_state = event->tilt;
 
+	/*
+	 * handle special events and keep actual profile and dpi values
+	 * up to date
+	 */
 	switch (event->event) {
 	case kone_mouse_event_osd_dpi:
 		kone->act_dpi = event->value;
diff --git a/drivers/hid/hid-roccat-kone.h b/drivers/hid/hid-roccat-kone.h
index 14f5ebf..35a5806 100644
--- a/drivers/hid/hid-roccat-kone.h
+++ b/drivers/hid/hid-roccat-kone.h
@@ -21,11 +21,12 @@
 
 struct kone_device {
 	/*
-	 * Actual values might not get called that much so I store them when
-	 * they are at hand or get them only when needed.
+	 * Storing actual values since there is no way of getting this
+	 * information from the device.
 	 */
 	int act_profile, act_profile_valid;
 	int act_dpi, act_dpi_valid;
+	/* used for neutralizing abnormal tilt button behaviour */
 	int last_tilt_state;
 };
 
@@ -163,11 +164,13 @@ struct kone_mouse_event {
 };
 
 enum kone_mouse_events {
+	/* osd events are thought to be display on screen */
 	kone_mouse_event_osd_dpi = 0xa0,
 	kone_mouse_event_osd_profile = 0xb0,
 	/* TODO clarify meaning and occurence of kone_mouse_event_calibration */
 	kone_mouse_event_calibration = 0xc0,
 	kone_mouse_event_call_overlong_macro = 0xe0,
+	/* switch events notify if user changed values wiht mousebutton click */
 	kone_mouse_event_switch_dpi = 0xf0,
 	kone_mouse_event_switch_profile = 0xf1
 };
-- 
1.6.6


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/2] HID: documentation additions and elimination of legacy filenames for Roccat Kone driver
  2010-02-21 16:50   ` [PATCH 2/2] HID: documentation additions and elimination of legacy filenames for Roccat Kone driver Stefan Achatz
@ 2010-02-22  6:29     ` Dmitry Torokhov
  2010-02-22 10:01       ` Jiri Kosina
  0 siblings, 1 reply; 17+ messages in thread
From: Dmitry Torokhov @ 2010-02-22  6:29 UTC (permalink / raw)
  To: Stefan Achatz
  Cc: Jiri Kosina, Jussi Kivilinna, wylda, Pavel Machek,
	Alessandro Guido, Tomas Hanak, Jason Noble, simon.windows,
	Sean Hildebrand, Sid Boyce, Henning Glawe, linux-input,
	linux-kernel

Hi Stefan,

On Sun, Feb 21, 2010 at 05:50:45PM +0100, Stefan Achatz wrote:
>  This patch adds more documentation and renames sysfs attributes to eliminate my legacy naming.

There are still questions as to the intended use of the attributes...

>  Signed-off-by: Stefan Achatz <erazor_de@users.sourceforge.net>
> 
> ---
>  drivers/hid/hid-roccat-kone.c |  265 +++++++++++++++++++++++++++--------------
>  drivers/hid/hid-roccat-kone.h |    7 +-
>  2 files changed, 178 insertions(+), 94 deletions(-)
> 
> diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
> index 94a2fb9..941f5b3 100644
> --- a/drivers/hid/hid-roccat-kone.c
> +++ b/drivers/hid/hid-roccat-kone.c
> @@ -54,6 +54,11 @@ static void kone_set_profile_checksum(struct kone_profile *profile)
>  	profile->checksum = cpu_to_le16(kone_calc_profile_checksum(profile));
>  }
>  
> +/*
> + * Checks success after writing data to mouse
> + * On success returns 0
> + * On failure returns errno
> + */
>  static int kone_check_write(struct usb_device *usb_dev)
>  {
>  	int len;
> @@ -95,6 +100,11 @@ static int kone_check_write(struct usb_device *usb_dev)
>  	}
>  }
>  
> +/*
> + * Reads settings data from mouse and stores it in @buf
> + * On success returns 0
> + * On failure returns errno
> + */
>  static int kone_get_settings(struct usb_device *usb_dev,
>  		struct kone_settings *buf)
>  {
> @@ -112,6 +122,13 @@ static int kone_get_settings(struct usb_device *usb_dev,
>  	return 0;
>  }
>  
> +/*
> + * Write settings to mouse
> + * Reads and compares stored data with new data to prevent needless write
> + * Sets checksum of supplied data
> + * On success returns 0
> + * On failure returns errno
> + */
>  static int kone_set_settings(struct usb_device *usb_dev,
>  		struct kone_settings const *buf)
>  {
> @@ -167,6 +184,11 @@ static int kone_set_settings(struct usb_device *usb_dev,
>  	return 0;
>  }
>  
> +/*
> + * Stores number of startup profile in @result
> + * On success returns 0
> + * On failure returns errno
> + */
>  static int kone_get_startup_profile(struct usb_device *usb_dev, int *result)
>  {
>  	struct kone_settings *buf;
> @@ -187,6 +209,12 @@ static int kone_get_startup_profile(struct usb_device *usb_dev, int *result)
>  	return 0;
>  }
>  
> +/*
> + * Reads profile data from mouse and stores it in @buf
> + * @number: profile number to read
> + * On success returns 0
> + * On failure returns errno
> + */
>  static int kone_get_profile(struct usb_device *usb_dev,
>  		struct kone_profile *buf, int number)
>  {
> @@ -195,6 +223,11 @@ static int kone_get_profile(struct usb_device *usb_dev,
>  	if (number < 1 || number > 5)
>  		return -EINVAL;
>  
> +	/*
> +	 * The manufacturer uses a control message of type class with interface
> +	 * as recipient and provides the profile number as index parameter.
> +	 * This is prevented in userspace by function check_ctrlrecip.
> +	 */
>  	len = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
>  			USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
>  					| USB_RECIP_INTERFACE | USB_DIR_IN,
> @@ -209,6 +242,15 @@ static int kone_get_profile(struct usb_device *usb_dev,
>  	return 0;
>  }
>  
> +/*
> + * Writes profile data to mouse
> + * Reads and compares stored data with new one to reduce unnecessary writes.

Do we care?

> + * Sets profile number and checksum of data, so one can copy profile data
> + * from one profile to another without usage of a userland tool.

What is in a profile?

> + * @number: profile number to write
> + * On success returns 0
> + * On failure returns errno
> + */
>  static int kone_set_profile(struct usb_device *usb_dev, char const *buf,
>  		int number)
>  {
> @@ -272,6 +314,11 @@ static int kone_set_profile(struct usb_device *usb_dev, char const *buf,
>  	return 0;
>  }
>  
> +/*
> + * Reads dpi setting from startup profile and stores it in @result
> + * On success returns 0
> + * On failure returns errno
> + */
>  static int kone_get_profile_startup_dpi(struct usb_device *usb_dev, int number,
>  		int *result)
>  {
> @@ -293,6 +340,11 @@ static int kone_get_profile_startup_dpi(struct usb_device *usb_dev, int number,
>  	return 0;
>  }
>  
> +/*
> + * Reads value of "fast-clip-weight" and stores it in @result
> + * On success returns 0
> + * On failure returns errno
> + */
>  static int kone_get_weight(struct usb_device *usb_dev, int *result)
>  {
>  	int len;
> @@ -315,6 +367,11 @@ static int kone_get_weight(struct usb_device *usb_dev, int *result)
>  	return 0;
>  }
>  
> +/*
> + * Reads firmware_version of mouse and stores it in @result
> + * On success returns 0
> + * On failure returns errno
> + */
>  static int kone_get_firmware_version(struct usb_device *usb_dev, int *result)
>  {
>  	int len;
> @@ -338,7 +395,7 @@ static int kone_get_firmware_version(struct usb_device *usb_dev, int *result)
>  	return 0;
>  }
>  
> -static ssize_t kone_set_settings_raw(struct device *dev,
> +static ssize_t kone_sysfs_set_settings(struct device *dev,
>  		struct device_attribute *attr, char const *buf, size_t size)
>  {
>  	struct hid_device *hdev = dev_get_drvdata(dev);
> @@ -365,7 +422,7 @@ static ssize_t kone_set_settings_raw(struct device *dev,
>  	return sizeof(struct kone_settings);
>  }
>  
> -static ssize_t kone_show_settings_raw(struct device *dev,
> +static ssize_t kone_sysfs_show_settings(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
>  	struct usb_interface *intf = to_usb_interface(dev);
> @@ -378,7 +435,7 @@ static ssize_t kone_show_settings_raw(struct device *dev,
>  	return sizeof(struct kone_settings);
>  }
>  
> -static ssize_t kone_get_profile_raw(struct device *dev, char *buf, int number)
> +static ssize_t kone_sysfs_get_profile(struct device *dev, char *buf, int number)
>  {
>  	struct usb_interface *intf = to_usb_interface(dev);
>  	struct usb_device *usb_dev = interface_to_usbdev(intf);
> @@ -389,7 +446,7 @@ static ssize_t kone_get_profile_raw(struct device *dev, char *buf, int number)
>  	return sizeof(struct kone_profile);
>  }
>  
> -static ssize_t kone_set_profile_raw(struct device *dev, char const *buf,
> +static ssize_t kone_sysfs_set_profile(struct device *dev, char const *buf,
>  		size_t size, int number)
>  {
>  	struct usb_interface *intf = to_usb_interface(dev);
> @@ -407,69 +464,70 @@ static ssize_t kone_set_profile_raw(struct device *dev, char const *buf,
>  	return sizeof(struct kone_profile);
>  }
>  
> -static ssize_t kone_show_profile_1_raw(struct device *dev,
> +static ssize_t kone_sysfs_show_profile_1(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
> -	return kone_get_profile_raw(dev, buf, 1);
> +	return kone_sysfs_get_profile(dev, buf, 1);
>  }
>  
> -static ssize_t kone_set_profile_1_raw(struct device *dev,
> +static ssize_t kone_sysfs_set_profile_1(struct device *dev,
>  		struct device_attribute *attr, char const *buf, size_t size)
>  {
> -	return kone_set_profile_raw(dev, buf, size, 1);
> +	return kone_sysfs_set_profile(dev, buf, size, 1);
>  }
>  
> -static ssize_t kone_show_profile_2_raw(struct device *dev,
> +static ssize_t kone_sysfs_show_profile_2(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
> -	return kone_get_profile_raw(dev, buf, 2);
> +	return kone_sysfs_get_profile(dev, buf, 2);
>  }
>  
> -static ssize_t kone_set_profile_2_raw(struct device *dev,
> +static ssize_t kone_sysfs_set_profile_2(struct device *dev,
>  		struct device_attribute *attr, char const *buf, size_t size)
>  {
> -	return kone_set_profile_raw(dev, buf, size, 2);
> +	return kone_sysfs_set_profile(dev, buf, size, 2);
>  }
>  
> -static ssize_t kone_show_profile_3_raw(struct device *dev,
> +static ssize_t kone_sysfs_show_profile_3(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
> -	return kone_get_profile_raw(dev, buf, 3);
> +	return kone_sysfs_get_profile(dev, buf, 3);
>  }
>  
> -static ssize_t kone_set_profile_3_raw(struct device *dev,
> +static ssize_t kone_sysfs_set_profile_3(struct device *dev,
>  		struct device_attribute *attr, char const *buf, size_t size)
>  {
> -	return kone_set_profile_raw(dev, buf, size, 3);
> +	return kone_sysfs_set_profile(dev, buf, size, 3);
>  }
>  
> -static ssize_t kone_show_profile_4_raw(struct device *dev,
> +static ssize_t kone_sysfs_show_profile_4(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
> -	return kone_get_profile_raw(dev, buf, 4);
> +	return kone_sysfs_get_profile(dev, buf, 4);
>  }
>  
> -static ssize_t kone_set_profile_4_raw(struct device *dev,
> +static ssize_t kone_sysfs_set_profile_4(struct device *dev,
>  		struct device_attribute *attr, char const *buf, size_t size)
>  {
> -	return kone_set_profile_raw(dev, buf, size, 4);
> +	return kone_sysfs_set_profile(dev, buf, size, 4);
>  }
>  
> -static ssize_t kone_show_profile_5_raw(struct device *dev,
> +static ssize_t kone_sysfs_show_profile_5(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
> -	return kone_get_profile_raw(dev, buf, 5);
> +	return kone_sysfs_get_profile(dev, buf, 5);
>  }
>  
> -static ssize_t kone_set_profile_5_raw(struct device *dev,
> +static ssize_t kone_sysfs_set_profile_5(struct device *dev,
>  		struct device_attribute *attr, char const *buf, size_t size)
>  {
> -	return kone_set_profile_raw(dev, buf, size, 5);
> +	return kone_sysfs_set_profile(dev, buf, size, 5);
>  }
>  
>  /*
> - * helper to fill kone_device structure with actual values
> - * returns 0 on success or error
> + * Helper to fill kone_device structure with actual values
> + * On success returns 0
> + * On failure returns errno
>   */
>  static int kone_device_set_actual_values(struct kone_device *kone,
>  		struct device *dev, int both)
> @@ -498,7 +556,7 @@ static int kone_device_set_actual_values(struct kone_device *kone,
>  	return 0;
>  }
>  
> -static ssize_t kone_show_actual_profile(struct device *dev,
> +static ssize_t kone_sysfs_show_actual_profile(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
>  	struct hid_device *hdev = dev_get_drvdata(dev);
> @@ -511,7 +569,7 @@ static ssize_t kone_show_actual_profile(struct device *dev,
>  	return snprintf(buf, PAGE_SIZE, "%d\n", kone->act_profile);
>  }
>  
> -static ssize_t kone_show_actual_dpi_raw(struct device *dev,
> +static ssize_t kone_sysfs_show_actual_dpi(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
>  	struct hid_device *hdev = dev_get_drvdata(dev);
> @@ -525,31 +583,7 @@ static ssize_t kone_show_actual_dpi_raw(struct device *dev,
>  	return snprintf(buf, PAGE_SIZE, "%d\n", kone->act_dpi);
>  }
>  
> -static ssize_t kone_show_actual_dpi(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct hid_device *hdev = dev_get_drvdata(dev);
> -	struct kone_device *kone = hid_get_drvdata(hdev);
> -	int err, dpi;
> -	err = kone_device_set_actual_values(kone, dev, 1);
> -	if (err)
> -		return err;
> -
> -	dpi = kone->act_dpi;
> -	switch (dpi) {
> -	case 0:
> -		break;
> -	case 6:
> -		dpi = 3200;
> -		break;
> -	default:
> -		dpi = dpi * 400;
> -	}
> -
> -	return snprintf(buf, PAGE_SIZE, "%ddpi\n", dpi);
> -}
> -
> -static ssize_t kone_show_weight_raw(struct device *dev,
> +static ssize_t kone_sysfs_show_weight(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
>  	struct usb_interface *intf = to_usb_interface(dev);
> @@ -562,7 +596,7 @@ static ssize_t kone_show_weight_raw(struct device *dev,
>  	return snprintf(buf, PAGE_SIZE, "%d\n", weight);
>  }
>  
> -static ssize_t kone_show_firmware_version_raw(struct device *dev,
> +static ssize_t kone_sysfs_show_firmware_version(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
>  	struct usb_interface *intf = to_usb_interface(dev);
> @@ -575,8 +609,8 @@ static ssize_t kone_show_firmware_version_raw(struct device *dev,
>  	return snprintf(buf, PAGE_SIZE, "%d\n", firmware_version);
>  }
>  
> -static ssize_t kone_show_tcu(struct device *dev, struct device_attribute *attr,
> -		char *buf)
> +static ssize_t kone_sysfs_show_tcu(struct device *dev,
> +		struct device_attribute *attr, char *buf)
>  {
>  	struct usb_interface *intf = to_usb_interface(dev);
>  	struct usb_device *usb_dev = interface_to_usbdev(intf);
> @@ -624,9 +658,8 @@ static int kone_tcu_command(struct usb_device *usb_dev, int number)
>  	return 0;
>  }
>  
> -/* integer of 0 deactivates tcu, 1 activates it */
> -static ssize_t kone_set_tcu(struct device *dev, struct device_attribute *attr,
> -		char const *buf, size_t size)
> +static ssize_t kone_sysfs_set_tcu(struct device *dev,
> +		struct device_attribute *attr, char const *buf, size_t size)
>  {
>  	struct usb_interface *intf = to_usb_interface(dev);
>  	struct usb_device *usb_dev = interface_to_usbdev(intf);
> @@ -692,7 +725,7 @@ static ssize_t kone_set_tcu(struct device *dev, struct device_attribute *attr,
>  	return size;
>  }
>  
> -static ssize_t kone_show_startup_profile(struct device *dev,
> +static ssize_t kone_sysfs_show_startup_profile(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
>  	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
> @@ -703,7 +736,7 @@ static ssize_t kone_show_startup_profile(struct device *dev,
>  	return snprintf(buf, PAGE_SIZE, "%d\n", result);
>  }
>  
> -static ssize_t kone_set_startup_profile(struct device *dev,
> +static ssize_t kone_sysfs_set_startup_profile(struct device *dev,
>  		struct device_attribute *attr, char const *buf, size_t size)
>  {
>  	struct hid_device *hdev = dev_get_drvdata(dev);
> @@ -747,48 +780,85 @@ static ssize_t kone_set_startup_profile(struct device *dev,
>  	return size;
>  }
>  
> -static ssize_t kone_show_driver_version(struct device *dev,
> +/*
> + * This file is used by userland software to find devices that are handled by
> + * this driver. This provides a consistent way for actual and older kernels
> + * where this driver replaced usbhid instead of generic-usb.
> + * Driver capabilities are determined by version number.
> + */
> +static ssize_t kone_sysfs_show_driver_version(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
>  	return snprintf(buf, PAGE_SIZE, DRIVER_VERSION "\n");
>  }
>  

This should not be a per-device but belong to the module, if it is
really needed. Normally you query device's capabilities to determine
what's there. Also, why the need for special driver, can't evdev be
used/extended?

> -static DEVICE_ATTR(actual_dpi_raw, S_IRUGO, kone_show_actual_dpi_raw, NULL);
> -static DEVICE_ATTR(actual_dpi, S_IRUGO, kone_show_actual_dpi, NULL);
> -static DEVICE_ATTR(actual_profile, S_IRUGO, kone_show_actual_profile, NULL);
> -static DEVICE_ATTR(weight_raw, S_IRUGO, kone_show_weight_raw, NULL);
> -static DEVICE_ATTR(profile1_raw, S_IRUGO | S_IWUGO,
> -		kone_show_profile_1_raw, kone_set_profile_1_raw);
> -static DEVICE_ATTR(profile2_raw, S_IRUGO | S_IWUGO,
> -		kone_show_profile_2_raw, kone_set_profile_2_raw);
> -static DEVICE_ATTR(profile3_raw, S_IRUGO | S_IWUGO,
> -		kone_show_profile_3_raw, kone_set_profile_3_raw);
> -static DEVICE_ATTR(profile4_raw, S_IRUGO | S_IWUGO,
> -		kone_show_profile_4_raw, kone_set_profile_4_raw);
> -static DEVICE_ATTR(profile5_raw, S_IRUGO | S_IWUGO,
> -		kone_show_profile_5_raw, kone_set_profile_5_raw);
> -static DEVICE_ATTR(settings_raw, S_IRUGO | S_IWUGO,
> -		kone_show_settings_raw, kone_set_settings_raw);
> -static DEVICE_ATTR(firmware_version_raw, S_IRUGO,
> -		kone_show_firmware_version_raw, NULL);
> -static DEVICE_ATTR(tcu, S_IRUGO | S_IWUGO, kone_show_tcu, kone_set_tcu);
> +/*
> + * Read actual dpi settings.
> + * Returns raw value for further processing. Refer to enum kone_polling_rates to
> + * get real value.
> + */
> +static DEVICE_ATTR(actual_dpi, S_IRUGO, kone_sysfs_show_actual_dpi, NULL);
> +
> +static DEVICE_ATTR(actual_profile, S_IRUGO, kone_sysfs_show_actual_profile,
> +		NULL);
> +
> +/*
> + * The mouse can be equipped with one of four supplied weights from 5 to 20
> + * grams which are recognized and its value can be read out.
> + * This returns the raw value reported by the mouse for easy evaluation by
> + * software. Refer to enum kone_weights to get corresponding real weight.
> + */
> +static DEVICE_ATTR(weight, S_IRUGO, kone_sysfs_show_weight, NULL);

Why is this useful?

> +
> +static DEVICE_ATTR(profile1, S_IRUGO | S_IWUGO,
> +		kone_sysfs_show_profile_1, kone_sysfs_set_profile_1);
> +static DEVICE_ATTR(profile2, S_IRUGO | S_IWUGO,
> +		kone_sysfs_show_profile_2, kone_sysfs_set_profile_2);
> +static DEVICE_ATTR(profile3, S_IRUGO | S_IWUGO,
> +		kone_sysfs_show_profile_3, kone_sysfs_set_profile_3);
> +static DEVICE_ATTR(profile4, S_IRUGO | S_IWUGO,
> +		kone_sysfs_show_profile_4, kone_sysfs_set_profile_4);
> +static DEVICE_ATTR(profile5, S_IRUGO | S_IWUGO,
> +		kone_sysfs_show_profile_5, kone_sysfs_set_profile_5);
> +
> +static DEVICE_ATTR(settings, S_IRUGO | S_IWUGO,
> +		kone_sysfs_show_settings, kone_sysfs_set_settings);
> +
> +/*
> + * Prints firmware version stored in mouse as integer.
> + * The raw value reported by the mouse is returned for easy evaluation, to get
> + * the real version number the decimal point has to be shifted 2 positions to
> + * the left. E.g. a value of 138 means 1.38.
> + */
> +static DEVICE_ATTR(firmware_version, S_IRUGO,
> +		kone_sysfs_show_firmware_version, NULL);

Why is this useful?

> +
> +/*
> + * Prints state of Tracking Control Unit as number where 0 = off and 1 = on
> + * Writing 0 deactivates tcu and writing 1 calibrates and activates the tcu
> + */
> +static DEVICE_ATTR(tcu, S_IRUGO | S_IWUGO, kone_sysfs_show_tcu,
> +		kone_sysfs_set_tcu);
> +

What does TCU do?

> +/* Prints and takes the number of the profile the mouse starts with */
>  static DEVICE_ATTR(startup_profile, S_IRUGO | S_IWUGO,
> -		kone_show_startup_profile, kone_set_startup_profile);
> +		kone_sysfs_show_startup_profile,
> +		kone_sysfs_set_startup_profile);
> +
>  static DEVICE_ATTR(kone_driver_version, S_IRUGO,
> -		kone_show_driver_version, NULL);
> +		kone_sysfs_show_driver_version, NULL);
>  
>  static struct attribute *kone_attributes[] = {
> -		&dev_attr_actual_dpi_raw.attr,
>  		&dev_attr_actual_dpi.attr,
>  		&dev_attr_actual_profile.attr,
> -		&dev_attr_weight_raw.attr,
> -		&dev_attr_profile1_raw.attr,
> -		&dev_attr_profile2_raw.attr,
> -		&dev_attr_profile3_raw.attr,
> -		&dev_attr_profile4_raw.attr,
> -		&dev_attr_profile5_raw.attr,
> -		&dev_attr_settings_raw.attr,
> -		&dev_attr_firmware_version_raw.attr,
> +		&dev_attr_weight.attr,
> +		&dev_attr_profile1.attr,
> +		&dev_attr_profile2.attr,
> +		&dev_attr_profile3.attr,
> +		&dev_attr_profile4.attr,
> +		&dev_attr_profile5.attr,
> +		&dev_attr_settings.attr,
> +		&dev_attr_firmware_version.attr,
>  		&dev_attr_tcu.attr,
>  		&dev_attr_startup_profile.attr,
>  		&dev_attr_kone_driver_version.attr,
> @@ -801,6 +871,13 @@ static struct attribute_group kone_attribute_group = {
>  
>  static int kone_create_files(struct usb_interface *intf)
>  {
> +	/*
> +	 * Kone consists of a mouse and a keyboard part.
> +	 * Adding sysfs files only to mousepart as information about
> +	 * profile and dpi change is reported only in mouseevent.
> +	 * There is no way to bind only to mousepart since IGNORE_MOUSE quirk
> +	 * moved to hid-apple
> +	 */
>  	if (intf->cur_altsetting->desc.bInterfaceProtocol
>  			== USB_INTERFACE_PROTOCOL_MOUSE)
>  		return sysfs_create_group(&intf->dev.kobj,
> @@ -882,6 +959,10 @@ static int kone_raw_event(struct hid_device *hdev, struct hid_report *report,
>  	else
>  		kone->last_tilt_state = event->tilt;
>  
> +	/*
> +	 * handle special events and keep actual profile and dpi values
> +	 * up to date
> +	 */
>  	switch (event->event) {
>  	case kone_mouse_event_osd_dpi:
>  		kone->act_dpi = event->value;
> diff --git a/drivers/hid/hid-roccat-kone.h b/drivers/hid/hid-roccat-kone.h
> index 14f5ebf..35a5806 100644
> --- a/drivers/hid/hid-roccat-kone.h
> +++ b/drivers/hid/hid-roccat-kone.h
> @@ -21,11 +21,12 @@
>  
>  struct kone_device {
>  	/*
> -	 * Actual values might not get called that much so I store them when
> -	 * they are at hand or get them only when needed.
> +	 * Storing actual values since there is no way of getting this
> +	 * information from the device.
>  	 */
>  	int act_profile, act_profile_valid;
>  	int act_dpi, act_dpi_valid;
> +	/* used for neutralizing abnormal tilt button behaviour */
>  	int last_tilt_state;
>  };
>  
> @@ -163,11 +164,13 @@ struct kone_mouse_event {
>  };
>  
>  enum kone_mouse_events {
> +	/* osd events are thought to be display on screen */
>  	kone_mouse_event_osd_dpi = 0xa0,
>  	kone_mouse_event_osd_profile = 0xb0,
>  	/* TODO clarify meaning and occurence of kone_mouse_event_calibration */
>  	kone_mouse_event_calibration = 0xc0,
>  	kone_mouse_event_call_overlong_macro = 0xe0,
> +	/* switch events notify if user changed values wiht mousebutton click */
>  	kone_mouse_event_switch_dpi = 0xf0,
>  	kone_mouse_event_switch_profile = 0xf1
>  };
> -- 
> 1.6.6
> 

-- 
Dmitry

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/2] HID: documentation additions and elimination of legacy filenames for Roccat Kone driver
  2010-02-22  6:29     ` Dmitry Torokhov
@ 2010-02-22 10:01       ` Jiri Kosina
  2010-02-22 18:42         ` [PATCH 3/3] Adding documentation to sysfs attributes of roccat kone driver Stefan Achatz
  0 siblings, 1 reply; 17+ messages in thread
From: Jiri Kosina @ 2010-02-22 10:01 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Stefan Achatz, Jussi Kivilinna, wylda, Pavel Machek,
	Alessandro Guido, Tomas Hanak, Jason Noble, simon.windows,
	Sean Hildebrand, Sid Boyce, Henning Glawe, linux-input,
	linux-kernel

On Sun, 21 Feb 2010, Dmitry Torokhov wrote:

> On Sun, Feb 21, 2010 at 05:50:45PM +0100, Stefan Achatz wrote:
> >  This patch adds more documentation and renames sysfs attributes to eliminate my legacy naming.
> 
> There are still questions as to the intended use of the attributes...

Indeed.

Originally there was a plan to have everything under sysfs documented in 
Documentation/ABI.

Many developers (including me) have been too lazy to do that (or forgot to 
do so), but it might still be worthwile to resurrect this.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 3/3] Adding documentation to sysfs attributes of roccat kone driver
  2010-02-22 10:01       ` Jiri Kosina
@ 2010-02-22 18:42         ` Stefan Achatz
  2010-02-22 23:10           ` Dmitry Torokhov
  2010-02-23  8:03             ` Stefan Achatz
  0 siblings, 2 replies; 17+ messages in thread
From: Stefan Achatz @ 2010-02-22 18:42 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Dmitry Torokhov, Jussi Kivilinna, wylda, Pavel Machek,
	Alessandro Guido, Tomas Hanak, Jason Noble, simon.windows,
	Sean Hildebrand, Sid Boyce, Henning Glawe, linux-input,
	linux-kernel

 This patch adds documentation to sysfs attributes introduced by
 roccat kone mouse driver. The attributes are used by userland
 software already released by me under
 http://sourceforge.net/projects/roccat/

Didn't know if sysfs-driver-hid-roccat-kone or sysfs-driver-usb-roccat-kone
is the right filename, since the sysfs files are in bus/usb path.

I refused to split the What-lines to keep lines under 80 chars.

Signed-off-by: Stefan Achatz <erazor_de@users.sourceforge.net>
---
 .../ABI/testing/sysfs-driver-hid-roccat-kone       |  108 ++++++++++++++++++++
 1 files changed, 108 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-roccat-kone

diff --git a/Documentation/ABI/testing/sysfs-driver-hid-roccat-kone 
b/Documentation/ABI/testing/sysfs-driver-hid-roccat-kone
new file mode 100644
index 0000000..0bf1589
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-hid-roccat-kone
@@ -0,0 +1,108 @@
+What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/actual_dpi
+Date:		February 2010
+Contact:	Stefan Achatz <erazor_de@users.sourceforge.net>
+Description:	It is possible to switch the dpi setting of the mouse with the
+		press of a button.
+		When read, this file returns the raw number of the actual dpi
+		setting of the mouse. This number has to be further processed
+		to receive the real dpi value.
+
+		VALUE DPI
+		1     800
+		2     1200
+		3     1600
+		4     2000
+		5     2400
+		6     3200
+
+		This file is readonly.
+
+What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/actual_profile
+Date:		February 2010
+Contact:	Stefan Achatz <erazor_de@users.sourceforge.net>
+Description:	When read, this file returns the number of the actual profile.
+		This file is readonly.
+
+What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface 
num>/firmware_version
+Date:		February 2010
+Contact:	Stefan Achatz <erazor_de@users.sourceforge.net>
+Description:	When read, this file returns the raw version number of the
+		firmware stored in the mouse. Using the integer value eases
+		further usage in other programs. To receive the real version
+		number the decimal point has to be shifted 2 positions to the
+		left. E.g. a returned value of 138 means 1.38
+		This file is readonly.
+	
+What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface 
num>/kone_driver_version
+Date:		February 2010
+Contact:	Stefan Achatz <erazor_de@users.sourceforge.net>
+Description:	When read, this file returns the driver version.
+		The format of the string is "v<major>.<minor>.<patchlevel>".
+		This filename is used by the userland tools to find the
+		sys-paths of installed kone-mice. Versions of this driver for
+		old kernels replace usbhid instead of generic-usb. The way to
+		scan for this file has been chosen to provide a consistent way
+		for all supported kernel versions.
+		The version number is used to determine the drivers abilities.
+		This file is readonly.
+
+What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/profile[1-5]
+Date:		February 2010
+Contact:	Stefan Achatz <erazor_de@users.sourceforge.net>
+Description:	The mouse can store 5 profiles which can be switched by the
+		press of a button. A profile holds informations like button
+		mappings, sensitivity, the colors of the 5 leds and light
+		effects.
+		When read, these files return the respective profile. The
+		returned data is 975 bytes in size.
+		When written, this file lets one write the respective profile
+		data back to the mouse. The data has to be 975 bytes long. The
+		checksum and correct profile number will be set by the driver.
+		The mouse will reject invalid data.
+
+What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/settings
+Date:		February 2010
+Contact:	Stefan Achatz <erazor_de@users.sourceforge.net>
+Description:	When read, this file returns the settings stored in the mouse.
+		The size of the data is 36 bytes and holds information like the
+		startup_profile, tcu state and calibration_data.
+		When written, this file lets write settings back to the mouse.
+		The data has to be 36 bytes long, the checksum will be
+		calculated by the driver. The mouse will reject invalid data.
+
+What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/startup_profile
+Date:		February 2010
+Contact:	Stefan Achatz <erazor_de@users.sourceforge.net>
+Description:	When read, this file returns the number of the profile thats
+		active when the mouse is powered on.
+		This file is readonly.
+
+What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/tcu
+Date:		February 2010
+Contact:	Stefan Achatz <erazor_de@users.sourceforge.net>
+Description:	The mouse has a "Tracking Control Unit" which lets the user
+		calibrate the laser power to the mousepad surface.
+		When read, this file returns the current state of the TCU,
+		where 0 means off and 1 means on.
+		Writing 0 in this file will switch the TCU off.
+		Writing 1 in this file will start the calibration which takes
+		around 6 seconds to complete and activates the TCU.
+
+What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/weight
+Date:		February 2010
+Contact:	Stefan Achatz <erazor_de@users.sourceforge.net>
+Description:	The mouse can be equipped with one of four supplied weights
+		ranging from 5 to 20 grams which are recognized by the mouse
+		and its value can be read out. When read, this file returns the
+		raw value returned by the mouse which eases further processing
+		in other software.
+		The values map to the weights as follows:
+
+		VALUE WEIGHT
+		0     none
+		1     5g
+		2     10g
+		3     15g
+		4     20g
+
+		This file is readonly.
-- 
1.6.6


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] Adding documentation to sysfs attributes of roccat kone driver
  2010-02-22 18:42         ` [PATCH 3/3] Adding documentation to sysfs attributes of roccat kone driver Stefan Achatz
@ 2010-02-22 23:10           ` Dmitry Torokhov
  2010-02-23  8:03             ` Stefan Achatz
  1 sibling, 0 replies; 17+ messages in thread
From: Dmitry Torokhov @ 2010-02-22 23:10 UTC (permalink / raw)
  To: Stefan Achatz
  Cc: Jiri Kosina, Jussi Kivilinna, wylda, Pavel Machek,
	Alessandro Guido, Tomas Hanak, Jason Noble, simon.windows,
	Sean Hildebrand, Sid Boyce, Henning Glawe, linux-input,
	linux-kernel

Hi Stefan,

On Mon, Feb 22, 2010 at 07:42:04PM +0100, Stefan Achatz wrote:
>  This patch adds documentation to sysfs attributes introduced by
>  roccat kone mouse driver. The attributes are used by userland
>  software already released by me under
>  http://sourceforge.net/projects/roccat/
> 

Thank you for adding documentation for the sysfs attributes. I have one
more request though - the documentation adds a lot of anformation about
whta the fields mean but unfortunately it is silent on _why_ all these
fields are needed.

For example, why weight is interesting? Is it just shown just because we can?
What woudl yuser do with this information? Why would I need to
manipulate several profiles instead of letting udev configure desired
state for me every time mouse is plugged into the box? Should not TCU
calibration be simply performed during driver binding instead of
on-demand from userspace? And so forth.

Thanks.

> Didn't know if sysfs-driver-hid-roccat-kone or sysfs-driver-usb-roccat-kone
> is the right filename, since the sysfs files are in bus/usb path.
> 
> I refused to split the What-lines to keep lines under 80 chars.
> 
> Signed-off-by: Stefan Achatz <erazor_de@users.sourceforge.net>
> ---
>  .../ABI/testing/sysfs-driver-hid-roccat-kone       |  108 ++++++++++++++++++++
>  1 files changed, 108 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-roccat-kone
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-hid-roccat-kone 
> b/Documentation/ABI/testing/sysfs-driver-hid-roccat-kone
> new file mode 100644
> index 0000000..0bf1589
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-hid-roccat-kone
> @@ -0,0 +1,108 @@
> +What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/actual_dpi
> +Date:		February 2010
> +Contact:	Stefan Achatz <erazor_de@users.sourceforge.net>
> +Description:	It is possible to switch the dpi setting of the mouse with the
> +		press of a button.
> +		When read, this file returns the raw number of the actual dpi
> +		setting of the mouse. This number has to be further processed
> +		to receive the real dpi value.
> +
> +		VALUE DPI
> +		1     800
> +		2     1200
> +		3     1600
> +		4     2000
> +		5     2400
> +		6     3200
> +
> +		This file is readonly.
> +
> +What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/actual_profile
> +Date:		February 2010
> +Contact:	Stefan Achatz <erazor_de@users.sourceforge.net>
> +Description:	When read, this file returns the number of the actual profile.
> +		This file is readonly.
> +
> +What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface 
> num>/firmware_version
> +Date:		February 2010
> +Contact:	Stefan Achatz <erazor_de@users.sourceforge.net>
> +Description:	When read, this file returns the raw version number of the
> +		firmware stored in the mouse. Using the integer value eases
> +		further usage in other programs. To receive the real version
> +		number the decimal point has to be shifted 2 positions to the
> +		left. E.g. a returned value of 138 means 1.38
> +		This file is readonly.
> +	
> +What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface 
> num>/kone_driver_version
> +Date:		February 2010
> +Contact:	Stefan Achatz <erazor_de@users.sourceforge.net>
> +Description:	When read, this file returns the driver version.
> +		The format of the string is "v<major>.<minor>.<patchlevel>".
> +		This filename is used by the userland tools to find the
> +		sys-paths of installed kone-mice. Versions of this driver for
> +		old kernels replace usbhid instead of generic-usb. The way to
> +		scan for this file has been chosen to provide a consistent way
> +		for all supported kernel versions.
> +		The version number is used to determine the drivers abilities.
> +		This file is readonly.
> +
> +What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/profile[1-5]
> +Date:		February 2010
> +Contact:	Stefan Achatz <erazor_de@users.sourceforge.net>
> +Description:	The mouse can store 5 profiles which can be switched by the
> +		press of a button. A profile holds informations like button
> +		mappings, sensitivity, the colors of the 5 leds and light
> +		effects.
> +		When read, these files return the respective profile. The
> +		returned data is 975 bytes in size.
> +		When written, this file lets one write the respective profile
> +		data back to the mouse. The data has to be 975 bytes long. The
> +		checksum and correct profile number will be set by the driver.
> +		The mouse will reject invalid data.
> +
> +What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/settings
> +Date:		February 2010
> +Contact:	Stefan Achatz <erazor_de@users.sourceforge.net>
> +Description:	When read, this file returns the settings stored in the mouse.
> +		The size of the data is 36 bytes and holds information like the
> +		startup_profile, tcu state and calibration_data.
> +		When written, this file lets write settings back to the mouse.
> +		The data has to be 36 bytes long, the checksum will be
> +		calculated by the driver. The mouse will reject invalid data.
> +
> +What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/startup_profile
> +Date:		February 2010
> +Contact:	Stefan Achatz <erazor_de@users.sourceforge.net>
> +Description:	When read, this file returns the number of the profile thats
> +		active when the mouse is powered on.
> +		This file is readonly.
> +
> +What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/tcu
> +Date:		February 2010
> +Contact:	Stefan Achatz <erazor_de@users.sourceforge.net>
> +Description:	The mouse has a "Tracking Control Unit" which lets the user
> +		calibrate the laser power to the mousepad surface.
> +		When read, this file returns the current state of the TCU,
> +		where 0 means off and 1 means on.
> +		Writing 0 in this file will switch the TCU off.
> +		Writing 1 in this file will start the calibration which takes
> +		around 6 seconds to complete and activates the TCU.
> +
> +What:		/sys/bus/usb/devices/<busnum>-<devnum>:<config num>.<interface num>/weight
> +Date:		February 2010
> +Contact:	Stefan Achatz <erazor_de@users.sourceforge.net>
> +Description:	The mouse can be equipped with one of four supplied weights
> +		ranging from 5 to 20 grams which are recognized by the mouse
> +		and its value can be read out. When read, this file returns the
> +		raw value returned by the mouse which eases further processing
> +		in other software.
> +		The values map to the weights as follows:
> +
> +		VALUE WEIGHT
> +		0     none
> +		1     5g
> +		2     10g
> +		3     15g
> +		4     20g
> +
> +		This file is readonly.
> -- 
> 1.6.6
> 
> --
> 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

-- 
Dmitry

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] Adding documentation to sysfs attributes of roccat kone driver
  2010-02-22 18:42         ` [PATCH 3/3] Adding documentation to sysfs attributes of roccat kone driver Stefan Achatz
@ 2010-02-23  8:03             ` Stefan Achatz
  2010-02-23  8:03             ` Stefan Achatz
  1 sibling, 0 replies; 17+ messages in thread
From: Stefan Achatz @ 2010-02-23  8:03 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Jiri Kosina, Jussi Kivilinna, wylda, Pavel Machek,
	Alessandro Guido, Tomas Hanak, Jason Noble, simon.windows,
	Sean Hildebrand, Sid Boyce, Henning Glawe, linux-input,
	linux-kernel

> Thank you for adding documentation for the sysfs attributes. I have one
> more request though - the documentation adds a lot of anformation about
> whta the fields mean but unfortunately it is silent on _why_ all these
> fields are needed.

> For example, why weight is interesting? Is it just shown just because we can?
> What woudl yuser do with this information? Why would I need to
> manipulate several profiles instead of letting udev configure desired
> state for me every time mouse is plugged into the box? Should not TCU
> calibration be simply performed during driver binding instead of
> on-demand from userspace? And so forth.

I would have done all with libusb in userspace if I had not some problems with it:
- devio wouldn't let me access the mouseprofiles because the manufacturer uses a way to access them check_ctrlrecip() forbids.
- removing usbhid temporarily would render the mouse not responding for the time the action takes.
- readding usbhid is quirky and one needs to replug the mouse if the software fails in some way.

I'm unsure if all this functionality is wanted inside the kernel. This device is Hardware for gamers which i find interesting to be supported by linux also.
In my attempt to raise acceptance for kernel integration i moved all unnecessary functionality like calculating real weight and dpi values into my userland tools to keep only the basic functions in the module.
This driver is available as externally compiled module for some months now, in my eyes its the most elegant (and the only workable) solution and these patches are also an attempt to find out if this project generally has a chance to be integrated in the kernel (but there is also a module for sonys ps3 controller).
There are more devices from this manufacturer which have similar functions like a keyboard with a whole bunch of macro keys where the macros are stored in the keyboard which I find useful as (amateur) programmer and admin.

The weight is really just a "because I can" value.

Switching the profiles is done via software or pressing a button on the mouse and is useful if you wan't to play games with high sensitivity and use a low sensitivity profile for detail work in a graphics application. Also different button mappings (the mouse can play macros with can contain keyboard and mouse button events) are useful for different applications. As example some applications (viewers) do pagescroling with "page up/down" some with arrow keys. You are free to change profiles in the mouse or load new profiles as you like.
You can use a script to backup profiles and load a whole set of specific profiles before starting maybe a game and restore the old profiles on exit.

TCU calibration is only needed if you switch the surface the mouse is being used on. Also you can save and restore already aquired calibration data with the settings attribute without the need of recalibration.

Maybe this answers some of the questions. I'm new to this kernel patching stuff and I might benefit from verbatim questions and statements.
Thanks
Stefan Achatz
___________________________________________________________
GRATIS für alle WEB.DE-Nutzer: Die maxdome Movie-FLAT!
Jetzt freischalten unter http://movieflat.web.de

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] Adding documentation to sysfs attributes of roccat kone driver
@ 2010-02-23  8:03             ` Stefan Achatz
  0 siblings, 0 replies; 17+ messages in thread
From: Stefan Achatz @ 2010-02-23  8:03 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Jiri Kosina, Jussi Kivilinna, wylda, Pavel Machek,
	Alessandro Guido, Tomas Hanak, Jason Noble, simon.windows,
	Sean Hildebrand, Sid Boyce, Henning Glawe, linux-input,
	linux-kernel

> Thank you for adding documentation for the sysfs attributes. I have one
> more request though - the documentation adds a lot of anformation about
> whta the fields mean but unfortunately it is silent on _why_ all these
> fields are needed.

> For example, why weight is interesting? Is it just shown just because we can?
> What woudl yuser do with this information? Why would I need to
> manipulate several profiles instead of letting udev configure desired
> state for me every time mouse is plugged into the box? Should not TCU
> calibration be simply performed during driver binding instead of
> on-demand from userspace? And so forth.

I would have done all with libusb in userspace if I had not some problems with it:
- devio wouldn't let me access the mouseprofiles because the manufacturer uses a way to access them check_ctrlrecip() forbids.
- removing usbhid temporarily would render the mouse not responding for the time the action takes.
- readding usbhid is quirky and one needs to replug the mouse if the software fails in some way.

I'm unsure if all this functionality is wanted inside the kernel. This device is Hardware for gamers which i find interesting to be supported by linux also.
In my attempt to raise acceptance for kernel integration i moved all unnecessary functionality like calculating real weight and dpi values into my userland tools to keep only the basic functions in the module.
This driver is available as externally compiled module for some months now, in my eyes its the most elegant (and the only workable) solution and these patches are also an attempt to find out if this project generally has a chance to be integrated in the kernel (but there is also a module for sonys ps3 controller).
There are more devices from this manufacturer which have similar functions like a keyboard with a whole bunch of macro keys where the macros are stored in the keyboard which I find useful as (amateur) programmer and admin.

The weight is really just a "because I can" value.

Switching the profiles is done via software or pressing a button on the mouse and is useful if you wan't to play games with high sensitivity and use a low sensitivity profile for detail work in a graphics application. Also different button mappings (the mouse can play macros with can contain keyboard and mouse button events) are useful for different applications. As example some applications (viewers) do pagescroling with "page up/down" some with arrow keys. You are free to change profiles in the mouse or load new profiles as you like.
You can use a script to backup profiles and load a whole set of specific profiles before starting maybe a game and restore the old profiles on exit.

TCU calibration is only needed if you switch the surface the mouse is being used on. Also you can save and restore already aquired calibration data with the settings attribute without the need of recalibration.

Maybe this answers some of the questions. I'm new to this kernel patching stuff and I might benefit from verbatim questions and statements.
Thanks
Stefan Achatz
___________________________________________________________
GRATIS für alle WEB.DE-Nutzer: Die maxdome Movie-FLAT!
Jetzt freischalten unter http://movieflat.web.de
--
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	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/3] Adding documentation to sysfs attributes of roccat kone driver
  2010-02-23  8:03             ` Stefan Achatz
  (?)
@ 2010-02-26  7:44             ` Dmitry Torokhov
  2010-03-08 16:04               ` [PATCH 4/4] Added locks for sysfs attributes and internal data Stefan Achatz
  -1 siblings, 1 reply; 17+ messages in thread
From: Dmitry Torokhov @ 2010-02-26  7:44 UTC (permalink / raw)
  To: Stefan Achatz
  Cc: Jiri Kosina, Jussi Kivilinna, wylda, Pavel Machek,
	Alessandro Guido, Tomas Hanak, Jason Noble, simon.windows,
	Sean Hildebrand, Sid Boyce, Henning Glawe, linux-input,
	linux-kernel

On Tue, Feb 23, 2010 at 09:03:08AM +0100, Stefan Achatz wrote:
> > Thank you for adding documentation for the sysfs attributes. I have one
> > more request though - the documentation adds a lot of anformation about
> > whta the fields mean but unfortunately it is silent on _why_ all these
> > fields are needed.
> 
> > For example, why weight is interesting? Is it just shown just because we can?
> > What woudl yuser do with this information? Why would I need to
> > manipulate several profiles instead of letting udev configure desired
> > state for me every time mouse is plugged into the box? Should not TCU
> > calibration be simply performed during driver binding instead of
> > on-demand from userspace? And so forth.
> 

> I would have done all with libusb in userspace if I had not some
> problems with it:
> - devio wouldn't let me access the mouseprofiles because the
> manufacturer uses a way to access them check_ctrlrecip() forbids.
> - removing usbhid temporarily would render the mouse not responding
> for the time the action takes.
> - readding usbhid is quirky and one needs to replug the mouse if the
> software fails in some way.
> 
> I'm unsure if all this functionality is wanted inside the kernel. This
> device is Hardware for gamers which i find interesting to be supported
> by linux also.  In my attempt to raise acceptance for kernel
> integration i moved all unnecessary functionality like calculating
> real weight and dpi values into my userland tools to keep only the
> basic functions in the module.  This driver is available as externally
> compiled module for some months now, in my eyes its the most elegant
> (and the only workable) solution and these patches are also an attempt
> to find out if this project generally has a chance to be integrated in
> the kernel (but there is also a module for sonys ps3 controller).
> There are more devices from this manufacturer which have similar
> functions like a keyboard with a whole bunch of macro keys where the
> macros are stored in the keyboard which I find useful as (amateur)
> programmer and admin.
> 
> The weight is really just a "because I can" value.
> 
> Switching the profiles is done via software or pressing a button on
> the mouse and is useful if you wan't to play games with high
> sensitivity and use a low sensitivity profile for detail work in a
> graphics application. Also different button mappings (the mouse can
> play macros with can contain keyboard and mouse button events) are
> useful for different applications. As example some applications
> (viewers) do pagescroling with "page up/down" some with arrow keys.
> You are free to change profiles in the mouse or load new profiles as
> you like.  You can use a script to backup profiles and load a whole
> set of specific profiles before starting maybe a game and restore the
> old profiles on exit.
> 
> TCU calibration is only needed if you switch the surface the mouse is
> being used on. Also you can save and restore already aquired
> calibration data with the settings attribute without the need of
> recalibration.
> 
> Maybe this answers some of the questions. I'm new to this kernel
> patching stuff and I might benefit from verbatim questions and
> statements.

OK, I see. In this case you need to separate "because I can" attributes
that are not really useful to the users from the ones that actually
provide useful data and resubmit.

Version, firmware and weight should go, maybe DPI as well. You should
use VID/PID already exported in the kernel to locate the devices you are
interested in.

TCU - you probaly want it write only and block until device is
calibrated.

Sysfs attributes altering state of the device should not be user
writable (so 0644 or 0600).

There should be some locking around sysfs methods - will the device
survive sumiltaneous access from different processes at the same time?

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 4/4] Added locks for sysfs attributes and internal data.
  2010-02-26  7:44             ` Dmitry Torokhov
@ 2010-03-08 16:04               ` Stefan Achatz
  2010-03-09  0:05                 ` Jiri Kosina
  2010-03-09  7:41                 ` Dmitry Torokhov
  0 siblings, 2 replies; 17+ messages in thread
From: Stefan Achatz @ 2010-03-08 16:04 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Jiri Kosina, Jussi Kivilinna, wylda, Pavel Machek,
	Alessandro Guido, Tomas Hanak, Jason Noble, simon.windows,
	Sean Hildebrand, Sid Boyce, Henning Glawe, linux-input,
	linux-kernel

>From 47678162da8e374d6f132db21d89b718ee5cfbd1 Mon Sep 17 00:00:00 2001
From: Stefan Achatz <erazor_de@users.sourceforge.net>
Date: Mon, 8 Mar 2010 16:54:19 +0100
Subject: [PATCH 4/4] Added locks for sysfs attributes and internal data.

Added mutex lock to prevent different processes from accessing
sysfs attributes at the same time.
Added spin lock for internal data.
---
 drivers/hid/hid-roccat-kone.c |  246 ++++++++++++++++++++++++++++------------
 drivers/hid/hid-roccat-kone.h |    9 +-
 2 files changed, 179 insertions(+), 76 deletions(-)

diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
index 941f5b3..eded7d4 100644
--- a/drivers/hid/hid-roccat-kone.c
+++ b/drivers/hid/hid-roccat-kone.c
@@ -223,11 +223,6 @@ static int kone_get_profile(struct usb_device *usb_dev,
 	if (number < 1 || number > 5)
 		return -EINVAL;
 
-	/*
-	 * The manufacturer uses a control message of type class with interface
-	 * as recipient and provides the profile number as index parameter.
-	 * This is prevented in userspace by function check_ctrlrecip.
-	 */
 	len = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
 			USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
 					| USB_RECIP_INTERFACE | USB_DIR_IN,
@@ -398,16 +393,18 @@ static int kone_get_firmware_version(struct usb_device *usb_dev, int *result)
 static ssize_t kone_sysfs_set_settings(struct device *dev,
 		struct device_attribute *attr, char const *buf, size_t size)
 {
-	struct hid_device *hdev = dev_get_drvdata(dev);
-	struct kone_device *kone = hid_get_drvdata(hdev);
-	struct usb_interface *intf = to_usb_interface(dev);
-	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
+	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
 	int err;
+	unsigned long flags;
 
 	if (size != sizeof(struct kone_settings))
 		return -EINVAL;
 
+	mutex_lock(&kone->kone_lock);
 	err = kone_set_settings(usb_dev, (struct kone_settings const *)buf);
+	mutex_unlock(&kone->kone_lock);
+
 	if (err)
 		return err;
 
@@ -415,9 +412,10 @@ static ssize_t kone_sysfs_set_settings(struct device *dev,
 	 * If we get here, treat buf as okay (apart from checksum) and use value
 	 * of startup_profile as its at hand
 	 */
+	spin_lock_irqsave(&kone->actual_lock, flags);
 	kone->act_profile = ((struct kone_settings *)buf)->startup_profile;
-	kone->act_profile_valid = 1;
-	kone->act_dpi_valid = 0;
+	kone->act_dpi = -1;
+	spin_unlock_irqrestore(&kone->actual_lock, flags);
 
 	return sizeof(struct kone_settings);
 }
@@ -425,10 +423,14 @@ static ssize_t kone_sysfs_set_settings(struct device *dev,
 static ssize_t kone_sysfs_show_settings(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	struct usb_interface *intf = to_usb_interface(dev);
-	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
+	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
 	int err;
+
+	mutex_lock(&kone->kone_lock);
 	err = kone_get_settings(usb_dev, (struct kone_settings *)buf);
+	mutex_unlock(&kone->kone_lock);
+
 	if (err)
 		return err;
 
@@ -437,10 +439,14 @@ static ssize_t kone_sysfs_show_settings(struct device *dev,
 
 static ssize_t kone_sysfs_get_profile(struct device *dev, char *buf, int number)
 {
-	struct usb_interface *intf = to_usb_interface(dev);
-	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
+	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
 	int err;
+
+	mutex_lock(&kone->kone_lock);
 	err = kone_get_profile(usb_dev, (struct kone_profile *)buf, number);
+	mutex_unlock(&kone->kone_lock);
+
 	if (err)
 		return err;
 	return sizeof(struct kone_profile);
@@ -449,15 +455,17 @@ static ssize_t kone_sysfs_get_profile(struct device *dev, char *buf, int number)
 static ssize_t kone_sysfs_set_profile(struct device *dev, char const *buf,
 		size_t size, int number)
 {
-	struct usb_interface *intf = to_usb_interface(dev);
-	struct usb_device *usb_dev = interface_to_usbdev(intf);
-
+	struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
+	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
 	int err;
 
 	if (size != sizeof(struct kone_profile))
 		return -EINVAL;
 
+	mutex_lock(&kone->kone_lock);
 	err = kone_set_profile(usb_dev, buf, number);
+	mutex_unlock(&kone->kone_lock);
+
 	if (err)
 		return err;
 
@@ -525,32 +533,68 @@ static ssize_t kone_sysfs_set_profile_5(struct device *dev,
 }
 
 /*
- * Helper to fill kone_device structure with actual values
- * On success returns 0
- * On failure returns errno
+ * Fills act_profile in kone_device.
+ * Also writes result in @result.
+ * Once act_profile is valid, its not getting invalid any more.
+ * Returns on success 0, on failure errno
  */
-static int kone_device_set_actual_values(struct kone_device *kone,
-		struct device *dev, int both)
+static int kone_device_set_actual_profile(struct kone_device *kone,
+		struct device *dev, int *result)
 {
-	struct usb_interface *intf = to_usb_interface(dev);
-	struct usb_device *usb_dev = interface_to_usbdev(intf);
-	int err;
+	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
+	int err, temp;
+	unsigned long flags;
 
-	/* first make sure profile is valid */
-	if (!kone->act_profile_valid) {
-		err = kone_get_startup_profile(usb_dev, &kone->act_profile);
+	spin_lock_irqsave(&kone->actual_lock, flags);
+	if (kone->act_profile == -1) {
+		spin_unlock_irqrestore(&kone->actual_lock, flags);
+		err = kone_get_startup_profile(usb_dev, &temp);
 		if (err)
 			return err;
-		kone->act_profile_valid = 1;
+		spin_lock_irq(&kone->actual_lock);
+		kone->act_profile = temp;
+		spin_unlock_irqrestore(&kone->actual_lock, flags);
+		if (result)
+			*result = temp;
+	} else {
+		if (result)
+			*result = kone->act_profile;
+		spin_unlock_irqrestore(&kone->actual_lock, flags);
 	}
+	return 0;
+}
+
+/*
+ * Fills act_dpi in kone_device.
+ * Also writes result in @result.
+ * On success returns 0
+ * On failure returns errno
+ */
+static int kone_device_set_actual_dpi(struct kone_device *kone,
+		struct device *dev, int *result)
+{
+	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
+	int err, temp;
+	unsigned long flags;
+
+	kone_device_set_actual_profile(kone, dev, NULL);
 
-	/* then get startup dpi value */
-	if (!kone->act_dpi_valid && both) {
+	spin_lock_irqsave(&kone->actual_lock, flags);
+	if (kone->act_dpi == -1) {
+		spin_unlock_irqrestore(&kone->actual_lock, flags);
 		err = kone_get_profile_startup_dpi(usb_dev, kone->act_profile,
-				&kone->act_dpi);
+				&temp);
 		if (err)
 			return err;
-		kone->act_dpi_valid = 1;
+		spin_lock_irqsave(&kone->actual_lock, flags);
+		kone->act_dpi = temp;
+		spin_unlock_irqrestore(&kone->actual_lock, flags);
+		if (result)
+			*result = temp;
+	} else {
+		if (result)
+			*result = kone->act_dpi;
+		spin_unlock_irqrestore(&kone->actual_lock, flags);
 	}
 
 	return 0;
@@ -559,38 +603,47 @@ static int kone_device_set_actual_values(struct kone_device *kone,
 static ssize_t kone_sysfs_show_actual_profile(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	struct hid_device *hdev = dev_get_drvdata(dev);
-	struct kone_device *kone = hid_get_drvdata(hdev);
-	int err;
-	err = kone_device_set_actual_values(kone, dev, 0);
+	struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
+	int err, temp = 0;
+
+	mutex_lock(&kone->kone_lock);
+	err = kone_device_set_actual_profile(kone, dev, &temp);
+	mutex_unlock(&kone->kone_lock);
+
 	if (err)
 		return err;
 
-	return snprintf(buf, PAGE_SIZE, "%d\n", kone->act_profile);
+	return snprintf(buf, PAGE_SIZE, "%d\n", temp);
 }
 
 static ssize_t kone_sysfs_show_actual_dpi(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	struct hid_device *hdev = dev_get_drvdata(dev);
-	struct kone_device *kone = hid_get_drvdata(hdev);
-	int err;
+	struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
+	int err, temp = 0;
+
+	mutex_lock(&kone->kone_lock);
+	err = kone_device_set_actual_dpi(kone, dev, &temp);
+	mutex_unlock(&kone->kone_lock);
 
-	err = kone_device_set_actual_values(kone, dev, 1);
 	if (err)
 		return err;
 
-	return snprintf(buf, PAGE_SIZE, "%d\n", kone->act_dpi);
+	return snprintf(buf, PAGE_SIZE, "%d\n", temp);
 }
 
 static ssize_t kone_sysfs_show_weight(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	struct usb_interface *intf = to_usb_interface(dev);
-	struct usb_device *usb_dev = interface_to_usbdev(intf);
-	int weight;
+	struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
+	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
+	int weight = 0;
 	int retval;
+
+	mutex_lock(&kone->kone_lock);
 	retval = kone_get_weight(usb_dev, &weight);
+	mutex_unlock(&kone->kone_lock);
+
 	if (retval)
 		return retval;
 	return snprintf(buf, PAGE_SIZE, "%d\n", weight);
@@ -599,11 +652,15 @@ static ssize_t kone_sysfs_show_weight(struct device *dev,
 static ssize_t kone_sysfs_show_firmware_version(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	struct usb_interface *intf = to_usb_interface(dev);
-	struct usb_device *usb_dev = interface_to_usbdev(intf);
-	int firmware_version;
+	struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
+	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
+	int firmware_version = 0;
 	int retval;
+
+	mutex_lock(&kone->kone_lock);
 	retval = kone_get_firmware_version(usb_dev, &firmware_version);
+	mutex_unlock(&kone->kone_lock);
+
 	if (retval)
 		return retval;
 	return snprintf(buf, PAGE_SIZE, "%d\n", firmware_version);
@@ -612,8 +669,8 @@ static ssize_t kone_sysfs_show_firmware_version(struct device *dev,
 static ssize_t kone_sysfs_show_tcu(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	struct usb_interface *intf = to_usb_interface(dev);
-	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
+	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
 	int err, result;
 	struct kone_settings *settings;
 
@@ -621,7 +678,10 @@ static ssize_t kone_sysfs_show_tcu(struct device *dev,
 	if (!settings)
 		return -ENOMEM;
 
+	mutex_lock(&kone->kone_lock);
 	err = kone_get_settings(usb_dev, settings);
+	mutex_unlock(&kone->kone_lock);
+
 	if (err) {
 		kfree(settings);
 		return err;
@@ -661,8 +721,8 @@ static int kone_tcu_command(struct usb_device *usb_dev, int number)
 static ssize_t kone_sysfs_set_tcu(struct device *dev,
 		struct device_attribute *attr, char const *buf, size_t size)
 {
-	struct usb_interface *intf = to_usb_interface(dev);
-	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
+	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
 	int err;
 	unsigned long state;
 	struct kone_settings *settings;
@@ -674,23 +734,35 @@ static ssize_t kone_sysfs_set_tcu(struct device *dev,
 	if (state != 0 && state != 1)
 		return -EINVAL;
 
+	mutex_lock(&kone->kone_lock);
+
 	if (state == 1) { /* state activate */
 		err = kone_tcu_command(usb_dev, 1);
-		if (err)
+		if (err) {
+			mutex_unlock(&kone->kone_lock);
 			return err;
+		}
 		err = kone_tcu_command(usb_dev, 2);
-		if (err)
+		if (err) {
+			mutex_unlock(&kone->kone_lock);
 			return err;
+		}
 		ssleep(5); /* tcu needs this time for calibration */
 		err = kone_tcu_command(usb_dev, 3);
-		if (err)
+		if (err) {
+			mutex_unlock(&kone->kone_lock);
 			return err;
+		}
 		err = kone_tcu_command(usb_dev, 0);
-		if (err)
+		if (err) {
+			mutex_unlock(&kone->kone_lock);
 			return err;
+		}
 		err = kone_tcu_command(usb_dev, 4);
-		if (err)
+		if (err) {
+			mutex_unlock(&kone->kone_lock);
 			return err;
+		}
 		/*
 		 * Kone needs this time to settle things.
 		 * Reading settings too early will result in invalid data.
@@ -701,11 +773,14 @@ static ssize_t kone_sysfs_set_tcu(struct device *dev,
 	}
 
 	settings = kmalloc(sizeof(struct kone_settings), GFP_KERNEL);
-	if (!settings)
+	if (!settings) {
+		mutex_unlock(&kone->kone_lock);
 		return -ENOMEM;
+	}
 
 	err = kone_get_settings(usb_dev, settings);
 	if (err) {
+		mutex_unlock(&kone->kone_lock);
 		kfree(settings);
 		return err;
 	}
@@ -716,11 +791,14 @@ static ssize_t kone_sysfs_set_tcu(struct device *dev,
 
 		err = kone_set_settings(usb_dev, settings);
 		if (err) {
+			mutex_unlock(&kone->kone_lock);
 			kfree(settings);
 			return err;
 		}
 	}
 
+	mutex_unlock(&kone->kone_lock);
+
 	kfree(settings);
 	return size;
 }
@@ -728,23 +806,27 @@ static ssize_t kone_sysfs_set_tcu(struct device *dev,
 static ssize_t kone_sysfs_show_startup_profile(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
+	struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
 	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
 	int err, result;
+
+	mutex_lock(&kone->kone_lock);
 	err = kone_get_startup_profile(usb_dev, &result);
+	mutex_unlock(&kone->kone_lock);
+
 	if (err)
 		return err;
+
 	return snprintf(buf, PAGE_SIZE, "%d\n", result);
 }
 
 static ssize_t kone_sysfs_set_startup_profile(struct device *dev,
 		struct device_attribute *attr, char const *buf, size_t size)
 {
-	struct hid_device *hdev = dev_get_drvdata(dev);
-	struct kone_device *kone = hid_get_drvdata(hdev);
-	struct usb_interface *intf = to_usb_interface(dev);
-	struct usb_device *usb_dev = interface_to_usbdev(intf);
+	struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
+	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
 	int err;
-	unsigned long new_profile;
+	unsigned long new_profile, flags;
 	struct kone_settings *settings;
 
 	err = strict_strtoul(buf, 10, &new_profile);
@@ -758,8 +840,11 @@ static ssize_t kone_sysfs_set_startup_profile(struct device *dev,
 	if (!settings)
 		return -ENOMEM;
 
+	mutex_lock(&kone->kone_lock);
+
 	err = kone_get_settings(usb_dev, settings);
 	if (err) {
+		mutex_unlock(&kone->kone_lock);
 		kfree(settings);
 		return err;
 	}
@@ -767,16 +852,21 @@ static ssize_t kone_sysfs_set_startup_profile(struct device *dev,
 	settings->startup_profile = new_profile;
 
 	err = kone_set_settings(usb_dev, settings);
+
+	mutex_unlock(&kone->kone_lock);
+
 	if (err) {
 		kfree(settings);
 		return err;
 	}
 
+	spin_lock_irqsave(&kone->actual_lock, flags);
 	kone->act_profile = new_profile;
-	kone->act_profile_valid = 1;
-	kone->act_dpi_valid = 0;
+	kone->act_dpi = -1;
+	spin_unlock_irqrestore(&kone->actual_lock, flags);
 
 	kfree(settings);
+
 	return size;
 }
 
@@ -904,6 +994,10 @@ static int kone_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		dev_err(&hdev->dev, "can't alloc device descriptor\n");
 		return -ENOMEM;
 	}
+	mutex_init(&kone->kone_lock);
+	spin_lock_init(&kone->actual_lock);
+	kone->act_dpi = -1;
+	kone->act_profile = -1;
 
 	hid_set_drvdata(hdev, kone);
 	ret = hid_parse(hdev);
@@ -965,26 +1059,30 @@ static int kone_raw_event(struct hid_device *hdev, struct hid_report *report,
 	 */
 	switch (event->event) {
 	case kone_mouse_event_osd_dpi:
+		spin_lock(&kone->actual_lock);
 		kone->act_dpi = event->value;
-		kone->act_dpi_valid = 1;
-		dev_dbg(&hdev->dev, "osd dpi event. actual dpi %d (and %d)\n",
-				event->value, kone->act_dpi);
+		spin_unlock(&kone->actual_lock);
+		dev_dbg(&hdev->dev, "osd dpi event. actual dpi %d\n",
+				event->value);
 		return 1; /* return 1 if event was handled */
 	case kone_mouse_event_switch_dpi:
+		spin_lock(&kone->actual_lock);
 		kone->act_dpi = event->value;
-		kone->act_dpi_valid = 1;
+		spin_unlock(&kone->actual_lock);
 		dev_dbg(&hdev->dev, "switched dpi to %d\n", event->value);
 		return 1;
 	case kone_mouse_event_osd_profile:
+		spin_lock(&kone->actual_lock);
 		kone->act_profile = event->value;
-		kone->act_profile_valid = 1;
+		spin_unlock(&kone->actual_lock);
 		dev_dbg(&hdev->dev, "osd profile event. actual profile %d\n",
 				event->value);
 		return 1;
 	case kone_mouse_event_switch_profile:
+		spin_lock(&kone->actual_lock);
 		kone->act_profile = event->value;
-		kone->act_profile_valid = 1;
-		kone->act_dpi_valid = 0;
+		kone->act_dpi = -1;
+		spin_unlock(&kone->actual_lock);
 		dev_dbg(&hdev->dev, "switched profile to %d\n", event->value);
 		return 1;
 	case kone_mouse_event_call_overlong_macro:
diff --git a/drivers/hid/hid-roccat-kone.h b/drivers/hid/hid-roccat-kone.h
index 35a5806..9878f05 100644
--- a/drivers/hid/hid-roccat-kone.h
+++ b/drivers/hid/hid-roccat-kone.h
@@ -24,10 +24,15 @@ struct kone_device {
 	 * Storing actual values since there is no way of getting this
 	 * information from the device.
 	 */
-	int act_profile, act_profile_valid;
-	int act_dpi, act_dpi_valid;
+	int act_profile, act_dpi;
+	/* ensuring actual values are only accessed by one task at a time */
+	spinlock_t actual_lock;
+
 	/* used for neutralizing abnormal tilt button behaviour */
 	int last_tilt_state;
+
+	/* ensuring only one sysfs task accesses hardware at a time */
+	struct mutex kone_lock;
 };
 
 #pragma pack(push)
-- 
1.6.6.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 4/4] Added locks for sysfs attributes and internal data.
  2010-03-08 16:04               ` [PATCH 4/4] Added locks for sysfs attributes and internal data Stefan Achatz
@ 2010-03-09  0:05                 ` Jiri Kosina
  2010-03-09  7:41                 ` Dmitry Torokhov
  1 sibling, 0 replies; 17+ messages in thread
From: Jiri Kosina @ 2010-03-09  0:05 UTC (permalink / raw)
  To: Stefan Achatz
  Cc: Dmitry Torokhov, Jussi Kivilinna, wylda, Pavel Machek,
	Alessandro Guido, Tomas Hanak, Jason Noble, simon.windows,
	Sean Hildebrand, Sid Boyce, Henning Glawe, linux-input,
	linux-kernel

On Mon, 8 Mar 2010, Stefan Achatz wrote:

> From 47678162da8e374d6f132db21d89b718ee5cfbd1 Mon Sep 17 00:00:00 2001
> From: Stefan Achatz <erazor_de@users.sourceforge.net>
> Date: Mon, 8 Mar 2010 16:54:19 +0100
> Subject: [PATCH 4/4] Added locks for sysfs attributes and internal data.
> 
> Added mutex lock to prevent different processes from accessing
> sysfs attributes at the same time.
> Added spin lock for internal data.
> ---
>  drivers/hid/hid-roccat-kone.c |  246 ++++++++++++++++++++++++++++------------
>  drivers/hid/hid-roccat-kone.h |    9 +-
>  2 files changed, 179 insertions(+), 76 deletions(-)

Stefan,

could you please submit the rest of the patch series as well? I seem to 
have got only "4/4" patch of your series, but not the rest of the roccat 
driver (I recall there were some review comments raised after your initial 
submission of the driver, so full respin would be appreciated).

Also, please don't forget to put proper Signed-off-by: lines to the 
patches you submit with intention for upstream merge.

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 4/4] Added locks for sysfs attributes and internal data.
  2010-03-08 16:04               ` [PATCH 4/4] Added locks for sysfs attributes and internal data Stefan Achatz
  2010-03-09  0:05                 ` Jiri Kosina
@ 2010-03-09  7:41                 ` Dmitry Torokhov
  2010-03-13 11:19                   ` Stefan Achatz
  1 sibling, 1 reply; 17+ messages in thread
From: Dmitry Torokhov @ 2010-03-09  7:41 UTC (permalink / raw)
  To: Stefan Achatz
  Cc: Jiri Kosina, Jussi Kivilinna, wylda, Pavel Machek,
	Alessandro Guido, Tomas Hanak, Jason Noble, simon.windows,
	Sean Hildebrand, Sid Boyce, Henning Glawe, linux-input,
	linux-kernel

Hi Stefan,

On Mon, Mar 08, 2010 at 05:04:29PM +0100, Stefan Achatz wrote:
> From 47678162da8e374d6f132db21d89b718ee5cfbd1 Mon Sep 17 00:00:00 2001
> From: Stefan Achatz <erazor_de@users.sourceforge.net>
> Date: Mon, 8 Mar 2010 16:54:19 +0100
> Subject: [PATCH 4/4] Added locks for sysfs attributes and internal data.
> 
> Added mutex lock to prevent different processes from accessing
> sysfs attributes at the same time.
> Added spin lock for internal data.
> ---
>  drivers/hid/hid-roccat-kone.c |  246 ++++++++++++++++++++++++++++------------
>  drivers/hid/hid-roccat-kone.h |    9 +-
>  2 files changed, 179 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
> index 941f5b3..eded7d4 100644
> --- a/drivers/hid/hid-roccat-kone.c
> +++ b/drivers/hid/hid-roccat-kone.c
> @@ -223,11 +223,6 @@ static int kone_get_profile(struct usb_device *usb_dev,
>  	if (number < 1 || number > 5)
>  		return -EINVAL;
>  
> -	/*
> -	 * The manufacturer uses a control message of type class with interface
> -	 * as recipient and provides the profile number as index parameter.
> -	 * This is prevented in userspace by function check_ctrlrecip.
> -	 */
>  	len = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
>  			USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
>  					| USB_RECIP_INTERFACE | USB_DIR_IN,
> @@ -398,16 +393,18 @@ static int kone_get_firmware_version(struct usb_device *usb_dev, int *result)
>  static ssize_t kone_sysfs_set_settings(struct device *dev,
>  		struct device_attribute *attr, char const *buf, size_t size)
>  {
> -	struct hid_device *hdev = dev_get_drvdata(dev);
> -	struct kone_device *kone = hid_get_drvdata(hdev);
> -	struct usb_interface *intf = to_usb_interface(dev);
> -	struct usb_device *usb_dev = interface_to_usbdev(intf);
> +	struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
> +	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
>  	int err;
> +	unsigned long flags;
>  
>  	if (size != sizeof(struct kone_settings))
>  		return -EINVAL;
>  
> +	mutex_lock(&kone->kone_lock);
>  	err = kone_set_settings(usb_dev, (struct kone_settings const *)buf);

Hmm, this kind of casting tells me that this is binary, not text data
and thus binary sysfs attribute shoudl be used.

> +	mutex_unlock(&kone->kone_lock);
> +
>  	if (err)
>  		return err;
>  
> @@ -415,9 +412,10 @@ static ssize_t kone_sysfs_set_settings(struct device *dev,
>  	 * If we get here, treat buf as okay (apart from checksum) and use value
>  	 * of startup_profile as its at hand
>  	 */
> +	spin_lock_irqsave(&kone->actual_lock, flags);
>  	kone->act_profile = ((struct kone_settings *)buf)->startup_profile;
> -	kone->act_profile_valid = 1;
> -	kone->act_dpi_valid = 0;
> +	kone->act_dpi = -1;
> +	spin_unlock_irqrestore(&kone->actual_lock, flags);
>  

You don't really need a spinlock to set one integer.

>  	return sizeof(struct kone_settings);
>  }
> @@ -425,10 +423,14 @@ static ssize_t kone_sysfs_set_settings(struct device *dev,
>  static ssize_t kone_sysfs_show_settings(struct device *dev,
>  		struct device_attribute *attr, char *buf)
>  {
> -	struct usb_interface *intf = to_usb_interface(dev);
> -	struct usb_device *usb_dev = interface_to_usbdev(intf);
> +	struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
> +	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
>  	int err;
> +
> +	mutex_lock(&kone->kone_lock);
>  	err = kone_get_settings(usb_dev, (struct kone_settings *)buf);
> +	mutex_unlock(&kone->kone_lock);
> +
>  	if (err)
>  		return err;
>  
> @@ -437,10 +439,14 @@ static ssize_t kone_sysfs_show_settings(struct device *dev,
>  
>  static ssize_t kone_sysfs_get_profile(struct device *dev, char *buf, int number)
>  {
> -	struct usb_interface *intf = to_usb_interface(dev);
> -	struct usb_device *usb_dev = interface_to_usbdev(intf);
> +	struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
> +	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
>  	int err;
> +
> +	mutex_lock(&kone->kone_lock);
>  	err = kone_get_profile(usb_dev, (struct kone_profile *)buf, number);
> +	mutex_unlock(&kone->kone_lock);
> +
>  	if (err)
>  		return err;
>  	return sizeof(struct kone_profile);
> @@ -449,15 +455,17 @@ static ssize_t kone_sysfs_get_profile(struct device *dev, char *buf, int number)
>  static ssize_t kone_sysfs_set_profile(struct device *dev, char const *buf,
>  		size_t size, int number)
>  {
> -	struct usb_interface *intf = to_usb_interface(dev);
> -	struct usb_device *usb_dev = interface_to_usbdev(intf);
> -
> +	struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
> +	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
>  	int err;
>  
>  	if (size != sizeof(struct kone_profile))
>  		return -EINVAL;
>  
> +	mutex_lock(&kone->kone_lock);
>  	err = kone_set_profile(usb_dev, buf, number);
> +	mutex_unlock(&kone->kone_lock);
> +
>  	if (err)
>  		return err;
>  
> @@ -525,32 +533,68 @@ static ssize_t kone_sysfs_set_profile_5(struct device *dev,
>  }
>  
>  /*
> - * Helper to fill kone_device structure with actual values
> - * On success returns 0
> - * On failure returns errno
> + * Fills act_profile in kone_device.
> + * Also writes result in @result.
> + * Once act_profile is valid, its not getting invalid any more.
> + * Returns on success 0, on failure errno
>   */
> -static int kone_device_set_actual_values(struct kone_device *kone,
> -		struct device *dev, int both)
> +static int kone_device_set_actual_profile(struct kone_device *kone,
> +		struct device *dev, int *result)
>  {
> -	struct usb_interface *intf = to_usb_interface(dev);
> -	struct usb_device *usb_dev = interface_to_usbdev(intf);
> -	int err;
> +	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
> +	int err, temp;
> +	unsigned long flags;
>  
> -	/* first make sure profile is valid */
> -	if (!kone->act_profile_valid) {
> -		err = kone_get_startup_profile(usb_dev, &kone->act_profile);
> +	spin_lock_irqsave(&kone->actual_lock, flags);
> +	if (kone->act_profile == -1) {
> +		spin_unlock_irqrestore(&kone->actual_lock, flags);
> +		err = kone_get_startup_profile(usb_dev, &temp);
>  		if (err)
>  			return err;
> -		kone->act_profile_valid = 1;
> +		spin_lock_irq(&kone->actual_lock);
> +		kone->act_profile = temp;
> +		spin_unlock_irqrestore(&kone->actual_lock, flags);

You shoudl not be mixing _irq/_irqrestore. Also it is probably not
needed at all. If it is needed then the common style to acquire and
release locks only once in a function - this makes checking whether
lock/unlock is balanced easier.

> +		if (result)
> +			*result = temp;
> +	} else {
> +		if (result)
> +			*result = kone->act_profile;
> +		spin_unlock_irqrestore(&kone->actual_lock, flags);
>  	}
> +	return 0;
> +}
> +
> +/*
> + * Fills act_dpi in kone_device.
> + * Also writes result in @result.
> + * On success returns 0
> + * On failure returns errno
> + */
> +static int kone_device_set_actual_dpi(struct kone_device *kone,
> +		struct device *dev, int *result)
> +{
> +	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
> +	int err, temp;
> +	unsigned long flags;
> +
> +	kone_device_set_actual_profile(kone, dev, NULL);
>  
> -	/* then get startup dpi value */
> -	if (!kone->act_dpi_valid && both) {
> +	spin_lock_irqsave(&kone->actual_lock, flags);
> +	if (kone->act_dpi == -1) {
> +		spin_unlock_irqrestore(&kone->actual_lock, flags);

So what are we protecting exactly? What if act_dpi will become -1 here
(after you released the spinlock?

>  		err = kone_get_profile_startup_dpi(usb_dev, kone->act_profile,
> -				&kone->act_dpi);
> +				&temp);

Stopped looking - locking obviously is bogus and needs to be redone. You
need to decide what exactly needs protection and what critical sections
are.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 4/4] Added locks for sysfs attributes and internal data.
  2010-03-09  7:41                 ` Dmitry Torokhov
@ 2010-03-13 11:19                   ` Stefan Achatz
  2010-03-14  8:42                     ` Dmitry Torokhov
  0 siblings, 1 reply; 17+ messages in thread
From: Stefan Achatz @ 2010-03-13 11:19 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Jiri Kosina, Jussi Kivilinna, wylda, Pavel Machek,
	Alessandro Guido, Tomas Hanak, Jason Noble, simon.windows,
	Sean Hildebrand, Sid Boyce, Henning Glawe, linux-input,
	linux-kernel

Hello,
I have some more questions regarding the work on my driver:

1. binary vs text sysfs attributes
As I found little information about binary sysfs attributes I used the normal 
sysfs attributes to transfer binary data because they are also easier to use. 
The transfered data is far less than the 4096 bytes minimum page size.
So, is there a significant difference in these two variants that should force 
me to use the binary variant?

2. memory footprint
I'm thinking about a redesign of my sysfs attributes to reduce io and simplify 
the use of binary sysfs attributes (if needed, see above). This would 
increase the memory footprint of hid_drvdata to around 5kbytes.
Is it allowed for a driver to constantly alloc such an amount of data or 
should I stick with more complicated code and more io?

3. grow up of driver
I see my work is not quite ready for kernel inclusion. Maybe I'm wrong in this 
list and should bug other people. Would the linuxdriverproject.org be a 
better place to raise my work to adulthood?

Thanks in advance
Stefan Achatz

> Hi Stefan,
>
> On Mon, Mar 08, 2010 at 05:04:29PM +0100, Stefan Achatz wrote:
> > From 47678162da8e374d6f132db21d89b718ee5cfbd1 Mon Sep 17 00:00:00 2001
> > From: Stefan Achatz <erazor_de@users.sourceforge.net>
> > Date: Mon, 8 Mar 2010 16:54:19 +0100
> > Subject: [PATCH 4/4] Added locks for sysfs attributes and internal data.
> >
> > Added mutex lock to prevent different processes from accessing
> > sysfs attributes at the same time.
> > Added spin lock for internal data.
> > ---
> >  drivers/hid/hid-roccat-kone.c |  246
> > ++++++++++++++++++++++++++++------------ drivers/hid/hid-roccat-kone.h | 
> >   9 +-
> >  2 files changed, 179 insertions(+), 76 deletions(-)
> >
> > diff --git a/drivers/hid/hid-roccat-kone.c
> > b/drivers/hid/hid-roccat-kone.c index 941f5b3..eded7d4 100644
> > --- a/drivers/hid/hid-roccat-kone.c
> > +++ b/drivers/hid/hid-roccat-kone.c
> > @@ -223,11 +223,6 @@ static int kone_get_profile(struct usb_device
> > *usb_dev, if (number < 1 || number > 5)
> >  		return -EINVAL;
> >
> > -	/*
> > -	 * The manufacturer uses a control message of type class with interface
> > -	 * as recipient and provides the profile number as index parameter.
> > -	 * This is prevented in userspace by function check_ctrlrecip.
> > -	 */
> >  	len = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
> >  			USB_REQ_CLEAR_FEATURE, USB_TYPE_CLASS
> >
> >  					| USB_RECIP_INTERFACE | USB_DIR_IN,
> >
> > @@ -398,16 +393,18 @@ static int kone_get_firmware_version(struct
> > usb_device *usb_dev, int *result) static ssize_t
> > kone_sysfs_set_settings(struct device *dev,
> >  		struct device_attribute *attr, char const *buf, size_t size)
> >  {
> > -	struct hid_device *hdev = dev_get_drvdata(dev);
> > -	struct kone_device *kone = hid_get_drvdata(hdev);
> > -	struct usb_interface *intf = to_usb_interface(dev);
> > -	struct usb_device *usb_dev = interface_to_usbdev(intf);
> > +	struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
> > +	struct usb_device *usb_dev =
> > interface_to_usbdev(to_usb_interface(dev)); int err;
> > +	unsigned long flags;
> >
> >  	if (size != sizeof(struct kone_settings))
> >  		return -EINVAL;
> >
> > +	mutex_lock(&kone->kone_lock);
> >  	err = kone_set_settings(usb_dev, (struct kone_settings const *)buf);
>
> Hmm, this kind of casting tells me that this is binary, not text data
> and thus binary sysfs attribute shoudl be used.
>
> > +	mutex_unlock(&kone->kone_lock);
> > +
> >  	if (err)
> >  		return err;
> >
> > @@ -415,9 +412,10 @@ static ssize_t kone_sysfs_set_settings(struct device
> > *dev, * If we get here, treat buf as okay (apart from checksum) and use
> > value * of startup_profile as its at hand
> >  	 */
> > +	spin_lock_irqsave(&kone->actual_lock, flags);
> >  	kone->act_profile = ((struct kone_settings *)buf)->startup_profile;
> > -	kone->act_profile_valid = 1;
> > -	kone->act_dpi_valid = 0;
> > +	kone->act_dpi = -1;
> > +	spin_unlock_irqrestore(&kone->actual_lock, flags);
>
> You don't really need a spinlock to set one integer.
>
> >  	return sizeof(struct kone_settings);
> >  }
> > @@ -425,10 +423,14 @@ static ssize_t kone_sysfs_set_settings(struct
> > device *dev, static ssize_t kone_sysfs_show_settings(struct device *dev,
> >  		struct device_attribute *attr, char *buf)
> >  {
> > -	struct usb_interface *intf = to_usb_interface(dev);
> > -	struct usb_device *usb_dev = interface_to_usbdev(intf);
> > +	struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
> > +	struct usb_device *usb_dev =
> > interface_to_usbdev(to_usb_interface(dev)); int err;
> > +
> > +	mutex_lock(&kone->kone_lock);
> >  	err = kone_get_settings(usb_dev, (struct kone_settings *)buf);
> > +	mutex_unlock(&kone->kone_lock);
> > +
> >  	if (err)
> >  		return err;
> >
> > @@ -437,10 +439,14 @@ static ssize_t kone_sysfs_show_settings(struct
> > device *dev,
> >
> >  static ssize_t kone_sysfs_get_profile(struct device *dev, char *buf, int
> > number) {
> > -	struct usb_interface *intf = to_usb_interface(dev);
> > -	struct usb_device *usb_dev = interface_to_usbdev(intf);
> > +	struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
> > +	struct usb_device *usb_dev =
> > interface_to_usbdev(to_usb_interface(dev)); int err;
> > +
> > +	mutex_lock(&kone->kone_lock);
> >  	err = kone_get_profile(usb_dev, (struct kone_profile *)buf, number);
> > +	mutex_unlock(&kone->kone_lock);
> > +
> >  	if (err)
> >  		return err;
> >  	return sizeof(struct kone_profile);
> > @@ -449,15 +455,17 @@ static ssize_t kone_sysfs_get_profile(struct device
> > *dev, char *buf, int number) static ssize_t kone_sysfs_set_profile(struct
> > device *dev, char const *buf, size_t size, int number)
> >  {
> > -	struct usb_interface *intf = to_usb_interface(dev);
> > -	struct usb_device *usb_dev = interface_to_usbdev(intf);
> > -
> > +	struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev));
> > +	struct usb_device *usb_dev =
> > interface_to_usbdev(to_usb_interface(dev)); int err;
> >
> >  	if (size != sizeof(struct kone_profile))
> >  		return -EINVAL;
> >
> > +	mutex_lock(&kone->kone_lock);
> >  	err = kone_set_profile(usb_dev, buf, number);
> > +	mutex_unlock(&kone->kone_lock);
> > +
> >  	if (err)
> >  		return err;
> >
> > @@ -525,32 +533,68 @@ static ssize_t kone_sysfs_set_profile_5(struct
> > device *dev, }
> >
> >  /*
> > - * Helper to fill kone_device structure with actual values
> > - * On success returns 0
> > - * On failure returns errno
> > + * Fills act_profile in kone_device.
> > + * Also writes result in @result.
> > + * Once act_profile is valid, its not getting invalid any more.
> > + * Returns on success 0, on failure errno
> >   */
> > -static int kone_device_set_actual_values(struct kone_device *kone,
> > -		struct device *dev, int both)
> > +static int kone_device_set_actual_profile(struct kone_device *kone,
> > +		struct device *dev, int *result)
> >  {
> > -	struct usb_interface *intf = to_usb_interface(dev);
> > -	struct usb_device *usb_dev = interface_to_usbdev(intf);
> > -	int err;
> > +	struct usb_device *usb_dev =
> > interface_to_usbdev(to_usb_interface(dev)); +	int err, temp;
> > +	unsigned long flags;
> >
> > -	/* first make sure profile is valid */
> > -	if (!kone->act_profile_valid) {
> > -		err = kone_get_startup_profile(usb_dev, &kone->act_profile);
> > +	spin_lock_irqsave(&kone->actual_lock, flags);
> > +	if (kone->act_profile == -1) {
> > +		spin_unlock_irqrestore(&kone->actual_lock, flags);
> > +		err = kone_get_startup_profile(usb_dev, &temp);
> >  		if (err)
> >  			return err;
> > -		kone->act_profile_valid = 1;
> > +		spin_lock_irq(&kone->actual_lock);
> > +		kone->act_profile = temp;
> > +		spin_unlock_irqrestore(&kone->actual_lock, flags);
>
> You shoudl not be mixing _irq/_irqrestore. Also it is probably not
> needed at all. If it is needed then the common style to acquire and
> release locks only once in a function - this makes checking whether
> lock/unlock is balanced easier.
>
> > +		if (result)
> > +			*result = temp;
> > +	} else {
> > +		if (result)
> > +			*result = kone->act_profile;
> > +		spin_unlock_irqrestore(&kone->actual_lock, flags);
> >  	}
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Fills act_dpi in kone_device.
> > + * Also writes result in @result.
> > + * On success returns 0
> > + * On failure returns errno
> > + */
> > +static int kone_device_set_actual_dpi(struct kone_device *kone,
> > +		struct device *dev, int *result)
> > +{
> > +	struct usb_device *usb_dev =
> > interface_to_usbdev(to_usb_interface(dev)); +	int err, temp;
> > +	unsigned long flags;
> > +
> > +	kone_device_set_actual_profile(kone, dev, NULL);
> >
> > -	/* then get startup dpi value */
> > -	if (!kone->act_dpi_valid && both) {
> > +	spin_lock_irqsave(&kone->actual_lock, flags);
> > +	if (kone->act_dpi == -1) {
> > +		spin_unlock_irqrestore(&kone->actual_lock, flags);
>
> So what are we protecting exactly? What if act_dpi will become -1 here
> (after you released the spinlock?
>
> >  		err = kone_get_profile_startup_dpi(usb_dev, kone->act_profile,
> > -				&kone->act_dpi);
> > +				&temp);
>
> Stopped looking - locking obviously is bogus and needs to be redone. You
> need to decide what exactly needs protection and what critical sections
> are.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 4/4] Added locks for sysfs attributes and internal data.
  2010-03-13 11:19                   ` Stefan Achatz
@ 2010-03-14  8:42                     ` Dmitry Torokhov
  2010-03-15 13:51                       ` Jiri Kosina
  0 siblings, 1 reply; 17+ messages in thread
From: Dmitry Torokhov @ 2010-03-14  8:42 UTC (permalink / raw)
  To: Stefan Achatz
  Cc: Jiri Kosina, Jussi Kivilinna, wylda, Pavel Machek,
	Alessandro Guido, Tomas Hanak, Jason Noble, simon.windows,
	Sean Hildebrand, Sid Boyce, Henning Glawe, linux-input,
	linux-kernel

Hi Stefan,

On Sat, Mar 13, 2010 at 12:19:27PM +0100, Stefan Achatz wrote:
> Hello,
> I have some more questions regarding the work on my driver:
> 
> 1. binary vs text sysfs attributes
> As I found little information about binary sysfs attributes I used the normal 
> sysfs attributes to transfer binary data because they are also easier to use. 
> The transfered data is far less than the 4096 bytes minimum page size.
> So, is there a significant difference in these two variants that should force 
> me to use the binary variant?
> 

I'd say in this case it just sets expectations for the interface -
normal sysfs attributes are supposed to pass data in human-readable
form, one value per attribute.

> 2. memory footprint
> I'm thinking about a redesign of my sysfs attributes to reduce io and simplify 
> the use of binary sysfs attributes (if needed, see above). This would 
> increase the memory footprint of hid_drvdata to around 5kbytes.
> Is it allowed for a driver to constantly alloc such an amount of data or 
> should I stick with more complicated code and more io?

5K is not that much but why do you think binary attributes will require
such increase?

> 
> 3. grow up of driver
> I see my work is not quite ready for kernel inclusion. Maybe I'm wrong in this 
> list and should bug other people. Would the linuxdriverproject.org be a 
> better place to raise my work to adulthood?
> 

Rarely a driver is ready for inclusion right away ;) Normally that
happens after someone submitted a few of them to the same subsystem so
he knows maintainers perferences, etc. I'd say you are doing well as is
here, there is no sense to move away somewhere else at this point.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 4/4] Added locks for sysfs attributes and internal data.
  2010-03-14  8:42                     ` Dmitry Torokhov
@ 2010-03-15 13:51                       ` Jiri Kosina
  0 siblings, 0 replies; 17+ messages in thread
From: Jiri Kosina @ 2010-03-15 13:51 UTC (permalink / raw)
  To: Dmitry Torokhov, Stefan Achatz
  Cc: Jussi Kivilinna, wylda, Pavel Machek, Alessandro Guido,
	Tomas Hanak, Jason Noble, simon.windows, Sean Hildebrand,
	Sid Boyce, Henning Glawe, linux-input, linux-kernel

On Sun, 14 Mar 2010, Dmitry Torokhov wrote:

> > 3. grow up of driver
> > I see my work is not quite ready for kernel inclusion. Maybe I'm wrong in this 
> > list and should bug other people. Would the linuxdriverproject.org be a 
> > better place to raise my work to adulthood?
> > 
> Rarely a driver is ready for inclusion right away ;) Normally that 
> happens after someone submitted a few of them to the same subsystem so 
> he knows maintainers perferences, etc. I'd say you are doing well as is 
> here, there is no sense to move away somewhere else at this point.

Absolutely.

Stefan, you are handling the review feedback in an excellent way, and the 
driver is very quickly getting into shape in which it would be merge-able 
right away.

So please do another round of submission with all the concerns that have 
been raised previously addressed, and I believe I'll just apply the driver 
to my tree.

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2010-03-15 13:51 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-02-20  8:11 [PATCH] HID: add driver for Roccat Kone gaming mouse Stefan Achatz
2010-02-20  8:11 ` Stefan Achatz
2010-02-21  8:13 ` Dmitry Torokhov
2010-02-21 16:50   ` [PATCH 2/2] HID: documentation additions and elimination of legacy filenames for Roccat Kone driver Stefan Achatz
2010-02-22  6:29     ` Dmitry Torokhov
2010-02-22 10:01       ` Jiri Kosina
2010-02-22 18:42         ` [PATCH 3/3] Adding documentation to sysfs attributes of roccat kone driver Stefan Achatz
2010-02-22 23:10           ` Dmitry Torokhov
2010-02-23  8:03           ` Stefan Achatz
2010-02-23  8:03             ` Stefan Achatz
2010-02-26  7:44             ` Dmitry Torokhov
2010-03-08 16:04               ` [PATCH 4/4] Added locks for sysfs attributes and internal data Stefan Achatz
2010-03-09  0:05                 ` Jiri Kosina
2010-03-09  7:41                 ` Dmitry Torokhov
2010-03-13 11:19                   ` Stefan Achatz
2010-03-14  8:42                     ` Dmitry Torokhov
2010-03-15 13:51                       ` Jiri Kosina

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.