Date:	Thu, 4 Nov 2010 14:25:51 -0400 (EDT)
From:	Jiri Kosina <jkosina@...e.cz>
To:	Stefan Achatz <erazor_de@...rs.sourceforge.net>
Cc:	Randy Dunlap <rdunlap@...otime.net>,
	Bruno Prémont <bonbons@...ux-vserver.org>,
	Stephane Chatty <chatty@...c.fr>,
	Don Prince <dhprince-devel@...oo.co.uk>,
	Dmitry Torokhov <dtor@...l.ru>, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-input@...r.kernel.org
Subject: Re: [PATCH] HID: roccat: Add support for Roccat Kone[+]

On Sun, 24 Oct 2010, Stefan Achatz wrote:

> This patch adds support for Roccat Kone[+] gaming mouse. Kone[+] is an enhanced version
> of the old Kone with more memory for macros, a better sensor and more functionality.
> This driver is conceptual similar to the existing Kone and Pyra drivers.
> Userland tools can soon be found at http://sourceforge.net/projects/roccat

Seems like there is a lot of duplicate code with Roccat Kone.

> +static int koneplus_send_control(struct usb_device *usb_dev, uint value,
> +		enum koneplus_control_requests request)
> +{
> +	int len;
> +	struct koneplus_control *control;
> +
> +	if ((request == KONEPLUS_CONTROL_REQUEST_PROFILE_SETTINGS ||
> +			request == KONEPLUS_CONTROL_REQUEST_PROFILE_BUTTONS) &&
> +			value > 4)
> +		return -EINVAL;
> +
> +	control = kmalloc(sizeof(struct koneplus_control), GFP_KERNEL);
> +	if (!control)
> +		return -ENOMEM;
> +
> +	control->command = KONEPLUS_COMMAND_CONTROL;
> +	control->value = value;
> +	control->request = request;
> +
> +	len = usb_control_msg(usb_dev, usb_sndctrlpipe(usb_dev, 0),
> +			USB_REQ_SET_CONFIGURATION,
> +			USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_OUT,
> +			KONEPLUS_USB_COMMAND_CONTROL, 0, control,
> +			sizeof(struct koneplus_control),
> +			USB_CTRL_SET_TIMEOUT);
> +
> +	kfree(control);
> +
> +	if (len != sizeof(struct koneplus_control))
> +		return len;
> +
> +	return 0;
> +}
> +
> +static int koneplus_receive(struct usb_device *usb_dev, uint usb_command,
> +		void *buf, uint size) {
> +	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,
> +			usb_command, 0, buf, size, USB_CTRL_SET_TIMEOUT);
> +
> +	return (len != size) ? -EIO : 0;
> +}
> +
> +static int koneplus_receive_control_status(struct usb_device *usb_dev)
> +{
> +	int retval;
> +	struct koneplus_control *control;
> +
> +	control = kmalloc(sizeof(struct koneplus_control), GFP_KERNEL);
> +	if (!control)
> +		return -ENOMEM;
> +
> +	do {
> +		retval = koneplus_receive(usb_dev, KONEPLUS_USB_COMMAND_CONTROL,
> +				control, sizeof(struct koneplus_control));
> +
> +		/* check if we get a completely wrong answer */
> +		if (retval)
> +			goto out;
> +
> +		if (control->value == KONEPLUS_CONTROL_REQUEST_STATUS_OK) {
> +			retval = 0;
> +			goto out;
> +		}
> +
> +		/* indicates that hardware needs some more time to complete action */
> +		if (control->value == KONEPLUS_CONTROL_REQUEST_STATUS_WAIT) {
> +			msleep(500); /* windows driver uses 1000 */
> +			continue;
> +		}
> +
> +		/* seems to be critical - replug necessary */
> +		if (control->value == KONEPLUS_CONTROL_REQUEST_STATUS_OVERLOAD) {
> +			retval = -EINVAL;
> +			goto out;
> +		}
> +
> +		dev_err(&usb_dev->dev, "koneplus_receive_control_status: "
> +				"unknown response value 0x%x\n", control->value);
> +		retval = -EINVAL;
> +		goto out;
> +
> +	} while (1);
> +out:
> +	kfree(control);
> +	return retval;
> +}
> +
> +static int koneplus_send(struct usb_device *usb_dev, uint command,
> +		void *buf, uint size) {
> +	int len;
> +
> +	len = usb_control_msg(usb_dev, usb_sndctrlpipe(usb_dev, 0),
> +			USB_REQ_SET_CONFIGURATION,
> +			USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_OUT,
> +			command, 0, buf, size, USB_CTRL_SET_TIMEOUT);
> +
> +	if (len != size)
> +		return -EIO;
> +
> +	if (koneplus_receive_control_status(usb_dev))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int koneplus_select_profile(struct usb_device *usb_dev, uint number,
> +		enum koneplus_control_requests request)
> +{
> +	int retval;
> +
> +	retval = koneplus_send_control(usb_dev, number, request);
> +	if (retval)
> +		return retval;
> +
> +	/* allow time to settle things - windows driver uses 500 */
> +	msleep(100);
> +
> +	retval = koneplus_receive_control_status(usb_dev);
> +	if (retval)
> +		return retval;
> +
> +	return 0;
> +}
> +
> +static int koneplus_get_info(struct usb_device *usb_dev,
> +		struct koneplus_info *buf)
> +{
> +	return koneplus_receive(usb_dev, KONEPLUS_USB_COMMAND_INFO,
> +			buf, sizeof(struct koneplus_info));
> +}
> +
> +static int koneplus_get_profile_settings(struct usb_device *usb_dev,
> +		struct koneplus_profile_settings *buf, uint number)
> +{
> +	int retval;
> +
> +	retval = koneplus_select_profile(usb_dev, number,
> +			KONEPLUS_CONTROL_REQUEST_PROFILE_SETTINGS);
> +	if (retval)
> +		return retval;
> +
> +	return koneplus_receive(usb_dev, KONEPLUS_USB_COMMAND_PROFILE_SETTINGS,
> +			buf, sizeof(struct koneplus_profile_settings));
> +}
> +
> +static int koneplus_set_profile_settings(struct usb_device *usb_dev,
> +		struct koneplus_profile_settings const *settings)
> +{
> +	return koneplus_send(usb_dev, KONEPLUS_USB_COMMAND_PROFILE_SETTINGS,
> +			(void *)settings, sizeof(struct koneplus_profile_settings));
> +}
> +
> +static int koneplus_get_profile_buttons(struct usb_device *usb_dev,
> +		struct koneplus_profile_buttons *buf, int number)
> +{
> +	int retval;
> +
> +	retval = koneplus_select_profile(usb_dev, number,
> +			KONEPLUS_CONTROL_REQUEST_PROFILE_BUTTONS);
> +	if (retval)
> +		return retval;
> +
> +	return koneplus_receive(usb_dev, KONEPLUS_USB_COMMAND_PROFILE_BUTTONS,
> +			buf, sizeof(struct koneplus_profile_buttons));
> +}
> +
> +static int koneplus_set_profile_buttons(struct usb_device *usb_dev,
> +		struct koneplus_profile_buttons const *buttons)
> +{
> +	return koneplus_send(usb_dev, KONEPLUS_USB_COMMAND_PROFILE_BUTTONS,
> +			(void *)buttons, sizeof(struct koneplus_profile_buttons));
> +}
> +
> +/* retval is 0-4 on success, < 0 on error */
> +static int koneplus_get_startup_profile(struct usb_device *usb_dev)
> +{
> +	struct koneplus_startup_profile *buf;
> +	int retval;
> +
> +	buf = kmalloc(sizeof(struct koneplus_startup_profile), GFP_KERNEL);
> +
> +	retval = koneplus_receive(usb_dev, KONEPLUS_USB_COMMAND_STARTUP_PROFILE,
> +			buf, sizeof(struct koneplus_startup_profile));
> +
> +	if (retval)
> +		goto out;
> +
> +	retval = buf->startup_profile;
> +out:
> +	kfree(buf);
> +	return retval;
> +}
> +
> +static int koneplus_set_startup_profile(struct usb_device *usb_dev,
> +		int startup_profile)
> +{
> +	struct koneplus_startup_profile buf;
> +
> +	buf.command = KONEPLUS_COMMAND_STARTUP_PROFILE;
> +	buf.size = sizeof(struct koneplus_startup_profile);
> +	buf.startup_profile = startup_profile;
> +
> +	return koneplus_send(usb_dev, KONEPLUS_USB_COMMAND_STARTUP_PROFILE,
> +			(char *)&buf, sizeof(struct koneplus_profile_buttons));
> +}

Is there any reason this couldn't be merged with the Roccat Kone 
counterparts?

> +static ssize_t koneplus_sysfs_read(struct file *fp, struct kobject *kobj,
> +		char *buf, loff_t off, size_t count,
> +		size_t real_size, uint command)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct koneplus_device *koneplus = hid_get_drvdata(dev_get_drvdata(dev));
> +	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
> +	int retval;
> +
> +	if (off != 0 || count != real_size)
> +		return -EINVAL;
> +
> +	mutex_lock(&koneplus->koneplus_lock);
> +	retval = koneplus_receive(usb_dev, command, buf, real_size);
> +	mutex_unlock(&koneplus->koneplus_lock);
> +
> +	if (retval)
> +		return retval;
> +
> +	return real_size;
> +}
> +
> +static ssize_t koneplus_sysfs_write(struct file *fp, struct kobject *kobj,
> +		void const *buf, loff_t off, size_t count,
> +		size_t real_size, uint command)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct koneplus_device *koneplus = hid_get_drvdata(dev_get_drvdata(dev));
> +	struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev));
> +	int retval;
> +
> +	if (off != 0 || count != real_size)
> +		return -EINVAL;
> +
> +	mutex_lock(&koneplus->koneplus_lock);
> +	retval = koneplus_send(usb_dev, command, (void *)buf, real_size);
> +	mutex_unlock(&koneplus->koneplus_lock);
> +
> +	if (retval)
> +		return retval;
> +
> +	return real_size;
> +}
> +
> +static ssize_t koneplus_sysfs_write_macro(struct file *fp,
> +		struct kobject *kobj, struct bin_attribute *attr, char *buf,
> +		loff_t off, size_t count)
> +{
> +	return koneplus_sysfs_write(fp, kobj, buf, off, count,
> +			sizeof(struct koneplus_macro), KONEPLUS_USB_COMMAND_MACRO);
> +}
> +
> +static ssize_t koneplus_sysfs_read_sensor(struct file *fp,
> +		struct kobject *kobj, struct bin_attribute *attr, char *buf,
> +		loff_t off, size_t count)
> +{
> +	return koneplus_sysfs_read(fp, kobj, buf, off, count,
> +			sizeof(struct koneplus_sensor), KONEPLUS_USB_COMMAND_SENSOR);
> +}
> +
> +static ssize_t koneplus_sysfs_write_sensor(struct file *fp,
> +		struct kobject *kobj, struct bin_attribute *attr, char *buf,
> +		loff_t off, size_t count)
> +{
> +	return koneplus_sysfs_write(fp, kobj, buf, off, count,
> +			sizeof(struct koneplus_sensor), KONEPLUS_USB_COMMAND_SENSOR);
> +}
> +
> +static ssize_t koneplus_sysfs_write_tcu(struct file *fp,
> +		struct kobject *kobj, struct bin_attribute *attr, char *buf,
> +		loff_t off, size_t count)
> +{
> +	return koneplus_sysfs_write(fp, kobj, buf, off, count,
> +			sizeof(struct koneplus_tcu), KONEPLUS_USB_COMMAND_TCU);
> +}
> +
> +static ssize_t koneplus_sysfs_read_tcu_image(struct file *fp,
> +		struct kobject *kobj, struct bin_attribute *attr, char *buf,
> +		loff_t off, size_t count)
> +{
> +	return koneplus_sysfs_read(fp, kobj, buf, off, count,
> +			sizeof(struct koneplus_tcu_image), KONEPLUS_USB_COMMAND_TCU);
> +}
> +
> +static ssize_t koneplus_sysfs_read_profilex_settings(struct file *fp,
> +		struct kobject *kobj, struct bin_attribute *attr, char *buf,
> +		loff_t off, size_t count, uint number)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct koneplus_device *koneplus = hid_get_drvdata(dev_get_drvdata(dev));
> +
> +	if (off >= sizeof(struct koneplus_profile_settings))
> +		return 0;
> +
> +	if (off + count > sizeof(struct koneplus_profile_settings))
> +		count = sizeof(struct koneplus_profile_settings) - off;
> +
> +	mutex_lock(&koneplus->koneplus_lock);
> +	memcpy(buf, ((void const *)&koneplus->profile_settings[number]) + off,
> +			count);
> +	mutex_unlock(&koneplus->koneplus_lock);
> +
> +	return count;
> +}
> +
> +static ssize_t koneplus_sysfs_read_profile1_settings(struct file *fp,
> +		struct kobject *kobj, struct bin_attribute *attr, char *buf,
> +		loff_t off, size_t count)
> +{
> +	return koneplus_sysfs_read_profilex_settings(fp, kobj,
> +			attr, buf, off, count, 0);
> +}
> +
> +static ssize_t koneplus_sysfs_read_profile2_settings(struct file *fp,
> +		struct kobject *kobj, struct bin_attribute *attr, char *buf,
> +		loff_t off, size_t count)
> +{
> +	return koneplus_sysfs_read_profilex_settings(fp, kobj,
> +			attr, buf, off, count, 1);
> +}
> +
> +static ssize_t koneplus_sysfs_read_profile3_settings(struct file *fp,
> +		struct kobject *kobj, struct bin_attribute *attr, char *buf,
> +		loff_t off, size_t count)
> +{
> +	return koneplus_sysfs_read_profilex_settings(fp, kobj,
> +			attr, buf, off, count, 2);
> +}
> +
> +static ssize_t koneplus_sysfs_read_profile4_settings(struct file *fp,
> +		struct kobject *kobj, struct bin_attribute *attr, char *buf,
> +		loff_t off, size_t count)
> +{
> +	return koneplus_sysfs_read_profilex_settings(fp, kobj,
> +			attr, buf, off, count, 3);
> +}
> +
> +static ssize_t koneplus_sysfs_read_profile5_settings(struct file *fp,
> +		struct kobject *kobj, struct bin_attribute *attr, char *buf,
> +		loff_t off, size_t count)
> +{
> +	return koneplus_sysfs_read_profilex_settings(fp, kobj,
> +			attr, buf, off, count, 4);
> +}
> +

This is really ugly. Can't you have just one read() function and 
distuingish according to bin_attribute->private?


> diff --git a/drivers/hid/hid-roccat-koneplus.h b/drivers/hid/hid-roccat-koneplus.h
> new file mode 100644
> index 0000000..bf750fa
> --- /dev/null
> +++ b/drivers/hid/hid-roccat-koneplus.h
> @@ -0,0 +1,229 @@
> +#ifndef __HID_ROCCAT_KONEPLUS_H
> +#define __HID_ROCCAT_KONEPLUS_H
> +
> +/*
> + * Copyright (c) 2010 Stefan Achatz <erazor_de@...rs.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>
> +
> +#pragma pack(push)
> +#pragma pack(1)

Why?

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists