Re: [PATCH 1/1] Corsair Vengeance K90 driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, 29 Aug 2015, Clément Vuchener wrote:


This is missing changelog and Signed-off-by: line. I know that you have 
provided that in your cover letter, but cover letter never make it to the 
actual GIT commits.

So please fix that and resend the patch, thanks.

> ---
>  Documentation/ABI/testing/sysfs-class-k90_profile  |  55 ++
>  .../ABI/testing/sysfs-driver-hid-corsair-k90       |  15 +
>  drivers/hid/Kconfig                                |  10 +
>  drivers/hid/Makefile                               |   1 +
>  drivers/hid/hid-core.c                             |   1 +
>  drivers/hid/hid-corsair-k90.c                      | 690
> +++++++++++++++++++++
>  drivers/hid/hid-ids.h                              |   3 +
>  7 files changed, 775 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-k90_profile
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair-k90
>  create mode 100644 drivers/hid/hid-corsair-k90.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-k90_profile
> b/Documentation/ABI/testing/sysfs-class-k90_profile
> new file mode 100644
> index 0000000..3275c16
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-k90_profile
> @@ -0,0 +1,55 @@
> +What:		/sys/class/k90_profile/<profile>/profile_number
> +Date:		August 2015
> +KernelVersion:	4.2
> +Contact:	Clement Vuchener <clement.vuchener@xxxxxxxxx>
> +Description:	Get the number of the profile.
> +
> +What:		/sys/class/k90_profile/<profile>/bindings
> +Date:		August 2015
> +KernelVersion:	4.2
> +Contact:	Clement Vuchener <clement.vuchener@xxxxxxxxx>
> +Description:	Write bindings to the keyboard on-board profile.
> +		The data structure is:
> +		 - number of bindings in this structure (1 byte)
> +		 - size of this data structure (2 bytes big endian)
> +		 - size of the macro data written to
> +		   /sys/class/k90_profile/<profile>/data (2 bytes big endian)
> +		 - array of bindings referencing the data from
> +		   /sys/class/k90_profile/<profile>/data, each containing:
> +		   * 0x10 for a key usage, 0x20 for a macro (1 byte)
> +		   * offset of the key usage/macro data (2 bytes big endian)
> +		   * length of the key usage/macro data (2 bytes big endian)
> +

This looks like violation of one-value-per-attribute rule for sysfs ABI. 
Could you please think about it once again with this aspect on mind?

[ ... snip ... ]
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index e6fce23..f0d9125 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1807,6 +1807,7 @@ static const struct hid_device_id
> hid_have_special_driver[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY,
> USB_DEVICE_ID_CHICONY_WIRELESS) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY,
> USB_DEVICE_ID_CHICONY_WIRELESS2) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_AK1D) },
> +	{ HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_DEVICE_ID_CORSAIR_K90) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_CREATIVELABS,
> USB_DEVICE_ID_PRODIKEYS_PCMIDI) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, USB_DEVICE_ID_CYGNAL_CP2112) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS,
> USB_DEVICE_ID_CYPRESS_BARCODE_1) },

Your mail client is corrupting long lines, making tha patch application 
impossible. Please fix that for your following submissions.

> diff --git a/drivers/hid/hid-corsair-k90.c b/drivers/hid/hid-corsair-k90.c
> new file mode 100644
> index 0000000..67c1095
> --- /dev/null
> +++ b/drivers/hid/hid-corsair-k90.c
> @@ -0,0 +1,690 @@
> +/*
> + * HID driver for Corsair Vengeance K90 Keyboard

Usually we try to be a little bit more generic, and name the driver 
according to the vendor (and fold all the vedor-specific stuff into the 
one driver).

So my suggestion would be to name the driver hid-corsair, keeping the 
possibility for adding more devices later open.

[ ... snip ... ]
> +static int k90_init_special_functions(struct hid_device *dev)
> +{
> +	int ret, i;
> +	struct usb_interface *usbif = to_usb_interface(dev->dev.parent);
> +	struct usb_device *usbdev = interface_to_usbdev(usbif);
> +	char data[8];
> +	struct k90_drvdata *drvdata =
> +	    kzalloc(sizeof(struct k90_drvdata), GFP_KERNEL);
> +	size_t name_sz;
> +	char *name;
> +	struct k90_led *led;
> +
> +	if (!drvdata) {
> +		ret = -ENOMEM;
> +		goto fail_drvdata;
> +	}
> +	hid_set_drvdata(dev, drvdata);
> +
> +	/* Get current status */
> +	ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0),
> +			      K90_REQUEST_STATUS,
> +			      USB_DIR_IN | USB_TYPE_VENDOR |
> +			      USB_RECIP_DEVICE, 0, 0, data, 8,
> +			      USB_CTRL_SET_TIMEOUT);

So you apparently also depend on USB ...

> +	if (ret < 0) {
> +		hid_warn(dev, "Failed to get K90 initial state (error %d).\n",
> +			 ret);
> +		drvdata->backlight.brightness = 0;
> +		drvdata->current_profile = 1;
> +	} else {
> +		drvdata->backlight.brightness = data[4];
> +		drvdata->current_profile = data[7];
> +	}
> +	/* Get current mode */
> +	ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0),
> +			      K90_REQUEST_GET_MODE,
> +			      USB_DIR_IN | USB_TYPE_VENDOR |
> +			      USB_RECIP_DEVICE, 0, 0, data, 2,
> +			      USB_CTRL_SET_TIMEOUT);
> +	if (ret < 0)
> +		hid_warn(dev, "Failed to get K90 initial mode (error %d).\n",
> +			 ret);
> +	else {
> +		switch (data[0]) {
> +		case K90_MACRO_MODE_HW:
> +			drvdata->macro_mode = 1;
> +			break;
> +		case K90_MACRO_MODE_SW:
> +			drvdata->macro_mode = 0;
> +			break;
> +		default:
> +			hid_warn(dev, "K90 in unknown mode: %02x.\n",
> +				 data[0]);
> +		}
> +	}
> +
> +	/* Init LED device for backlight */
> +	name_sz =
> +	    strlen(dev_name(&dev->dev)) + sizeof(K90_BACKLIGHT_LED_SUFFIX);
> +	name = devm_kzalloc(&dev->dev, name_sz, GFP_KERNEL);
> +	if (!name) {
> +		ret = -ENOMEM;
> +		goto fail_backlight;
> +	}
> +	snprintf(name, name_sz, "%s" K90_BACKLIGHT_LED_SUFFIX,
> +		 dev_name(&dev->dev));
> +	led = &drvdata->backlight;
> +	led->cdev.name = name;
> +	led->cdev.max_brightness = 3;
> +	led->cdev.brightness_set = k90_brightness_set;
> +	led->cdev.brightness_get = k90_brightness_get;
> +	INIT_WORK(&led->work, k90_backlight_work);
> +	ret = led_classdev_register(&dev->dev, &led->cdev);

... and also on LED subsystem, but this dependency is not expressed in 
Kconfig.

Thanks,

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux