diff mbox

[v10] leds: USB: HID: Add support for MSI GT683R led panels

Message ID 1403543808-8228-1-git-send-email-janne.kanniainen@gmail.com (mailing list archive)
State New, archived
Delegated to: Jiri Kosina
Headers show

Commit Message

Janne Kanniainen June 23, 2014, 5:16 p.m. UTC
This driver adds support for USB controlled led panels that exists in
MSI GT683R laptop

Signed-off-by: Janne Kanniainen <janne.kanniainen@gmail.com>
---
Changes in v2:
	- sorted headers to alphabetic order
	- using devm_kzalloc
	- using BIT(n)
	- using usb_control_msg instead of usb_submit_urb
	- removing unneeded code
Changes in v3:
	- implemented as HID device
	- some cleanups and bug fixes
Changes in v4:
	- more cleanups
	- support for selecting leds
	- suppport for selecting status

Changes in v5:
	- mode attribute documented under Documentation/ABI
	- made array for led_classdev
	- led devices uses now recommended naming scheme

Changes in v6:
	- flush_work added
	- using hid device name instead of hard coded gt683r
	- allocating name buffers with devm_kzalloc

Changes in v7:
	- buf with for fixed

Changes in v8:
	- some cleanups and bugs fixed

Changes in v9:
	- few style issues fixed

Changes in v10:
	- race condition fixed
	- using proper attribute group

 .../ABI/testing/sysfs-class-hid-driver-gt683r      |  14 +
 drivers/hid/Kconfig                                |  14 +
 drivers/hid/Makefile                               |   1 +
 drivers/hid/hid-core.c                             |   1 +
 drivers/hid/hid-gt683r.c                           | 318 +++++++++++++++++++++
 drivers/hid/hid-ids.h                              |   2 +-
 drivers/hid/usbhid/hid-quirks.c                    |   2 +-
 7 files changed, 350 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-hid-driver-gt683r
 create mode 100644 drivers/hid/hid-gt683r.c

Comments

Johan Hovold June 23, 2014, 5:27 p.m. UTC | #1
On Mon, Jun 23, 2014 at 08:16:48PM +0300, Janne Kanniainen wrote:
> This driver adds support for USB controlled led panels that exists in
> MSI GT683R laptop
> 
> Signed-off-by: Janne Kanniainen <janne.kanniainen@gmail.com>
> ---
> Changes in v2:
> 	- sorted headers to alphabetic order
> 	- using devm_kzalloc
> 	- using BIT(n)
> 	- using usb_control_msg instead of usb_submit_urb
> 	- removing unneeded code
> Changes in v3:
> 	- implemented as HID device
> 	- some cleanups and bug fixes
> Changes in v4:
> 	- more cleanups
> 	- support for selecting leds
> 	- suppport for selecting status
> 
> Changes in v5:
> 	- mode attribute documented under Documentation/ABI
> 	- made array for led_classdev
> 	- led devices uses now recommended naming scheme
> 
> Changes in v6:
> 	- flush_work added
> 	- using hid device name instead of hard coded gt683r
> 	- allocating name buffers with devm_kzalloc
> 
> Changes in v7:
> 	- buf with for fixed
> 
> Changes in v8:
> 	- some cleanups and bugs fixed
> 
> Changes in v9:
> 	- few style issues fixed
> 
> Changes in v10:
> 	- race condition fixed
> 	- using proper attribute group

You need to send a separate patch on top of v9, which Jiri has already
applied to his tree.

<snip>

> +static DEVICE_ATTR_RW(leds_mode);
> +
> +static struct attribute *gt683r_attributes[] = {
> +	&dev_attr_leds_mode.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group gt683r_attribute_group = {
> +	.attrs = gt683r_attributes
> +};
> +
> +static int gt683r_led_probe(struct hid_device *hdev,
> +			const struct hid_device_id *id)
> +{
> +	int i;
> +	int ret;
> +	int name_sz;
> +	char *name;
> +	struct gt683r_led *led;
> +
> +	led = devm_kzalloc(&hdev->dev, sizeof(*led), GFP_KERNEL);
> +	if (!led)
> +		return -ENOMEM;
> +
> +	mutex_init(&led->lock);
> +	INIT_WORK(&led->work, gt683r_led_work);
> +
> +	led->mode = GT683R_LED_NORMAL;
> +	led->hdev = hdev;
> +	hid_set_drvdata(hdev, led);
> +
> +	ret = hid_parse(hdev);
> +	if (ret) {
> +		hid_err(hdev, "hid parsing failed\n");
> +		return ret;
> +	}
> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> +	if (ret) {
> +		hid_err(hdev, "hw start failed\n");
> +		return ret;
> +	}
> +
> +	for (i = 0; i < GT683R_LED_COUNT; i++) {
> +		name_sz = strlen(dev_name(&hdev->dev)) +
> +				strlen(gt683r_panel_names[i]) + 3;
> +
> +		name = devm_kzalloc(&hdev->dev, name_sz, GFP_KERNEL);
> +		if (!name) {
> +			ret = -ENOMEM;
> +			goto fail;
> +		}
> +
> +		snprintf(name, name_sz, "%s::%s",
> +				dev_name(&hdev->dev), gt683r_panel_names[i]);
> +		led->led_devs[i].name = name;
> +		led->led_devs[i].max_brightness = 1;
> +		led->led_devs[i].brightness_set = gt683r_brightness_set;
> +		ret = led_classdev_register(&hdev->dev, &led->led_devs[i]);
> +		if (ret) {
> +			hid_err(hdev, "could not register led device\n");
> +			goto fail;
> +		}
> +	}
> +
> +	ret = sysfs_create_group(&led->hdev->dev.kobj, &gt683r_attribute_group);
> +	if (ret) {
> +		hid_err(hdev, "failed to create sysfs attributes\n");
> +		goto fail;
> +	}

This does not solve the race Greg is referring to. There's some more
info here:

	http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/

but I'm not sure exactly how you'd apply that to an HID driver. Perhaps
you could use the struct device_driver in struct hid_driver (although it
is currently marked as "private").

Johan

> +
> +	return 0;
> +
> +fail:
> +	for (i = i - 1; i >= 0; i--)
> +		led_classdev_unregister(&led->led_devs[i]);
> +	hid_hw_stop(hdev);
> +	return ret;
> +}
> +
> +static void gt683r_led_remove(struct hid_device *hdev)
> +{
> +	int i;
> +	struct gt683r_led *led = hid_get_drvdata(hdev);
> +
> +	sysfs_remove_group(&led->hdev->dev.kobj, &gt683r_attribute_group);
> +	for (i = 0; i < GT683R_LED_COUNT; i++)
> +		led_classdev_unregister(&led->led_devs[i]);
> +	flush_work(&led->work);
> +	hid_hw_stop(hdev);
> +}
> +
> +static struct hid_driver gt683r_led_driver = {
> +	.probe = gt683r_led_probe,
> +	.remove = gt683r_led_remove,
> +	.name = "gt683r_led",
> +	.id_table = gt683r_led_id,
> +};
> +
> +module_hid_driver(gt683r_led_driver);
> +
> +MODULE_AUTHOR("Janne Kanniainen");
> +MODULE_DESCRIPTION("MSI GT683R led driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 34bb220..3692d37 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -641,7 +641,7 @@
>  #define USB_DEVICE_ID_GENIUS_KB29E	0x3004
>  
>  #define USB_VENDOR_ID_MSI		0x1770
> -#define USB_DEVICE_ID_MSI_GX680R_LED_PANEL	0xff00
> +#define USB_DEVICE_ID_MSI_GT683R_LED_PANEL 0xff00
>  
>  #define USB_VENDOR_ID_NATIONAL_SEMICONDUCTOR 0x0400
>  #define USB_DEVICE_ID_N_S_HARMONY	0xc359
> diff --git a/drivers/hid/usbhid/hid-quirks.c b/drivers/hid/usbhid/hid-quirks.c
> index 8e4ddb3..c640e1d 100644
> --- a/drivers/hid/usbhid/hid-quirks.c
> +++ b/drivers/hid/usbhid/hid-quirks.c
> @@ -73,7 +73,7 @@ static const struct hid_blacklist {
>  	{ USB_VENDOR_ID_FORMOSA, USB_DEVICE_ID_FORMOSA_IR_RECEIVER, HID_QUIRK_NO_INIT_REPORTS },
>  	{ USB_VENDOR_ID_FREESCALE, USB_DEVICE_ID_FREESCALE_MX28, HID_QUIRK_NOGET },
>  	{ USB_VENDOR_ID_MGE, USB_DEVICE_ID_MGE_UPS, HID_QUIRK_NOGET },
> -	{ USB_VENDOR_ID_MSI, USB_DEVICE_ID_MSI_GX680R_LED_PANEL, HID_QUIRK_NO_INIT_REPORTS },
> +	{ USB_VENDOR_ID_MSI, USB_DEVICE_ID_MSI_GT683R_LED_PANEL, HID_QUIRK_NO_INIT_REPORTS },
>  	{ USB_VENDOR_ID_NEXIO, USB_DEVICE_ID_NEXIO_MULTITOUCH_PTI0750, HID_QUIRK_NO_INIT_REPORTS },
>  	{ USB_VENDOR_ID_NOVATEK, USB_DEVICE_ID_NOVATEK_MOUSE, HID_QUIRK_NO_INIT_REPORTS },
>  	{ USB_VENDOR_ID_PIXART, USB_DEVICE_ID_PIXART_OPTICAL_TOUCH_SCREEN, HID_QUIRK_NO_INIT_REPORTS },
--
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
Greg KH June 23, 2014, 6:23 p.m. UTC | #2
On Mon, Jun 23, 2014 at 08:16:48PM +0300, Janne Kanniainen wrote:
> +	ret = sysfs_create_group(&led->hdev->dev.kobj, &gt683r_attribute_group);
> +	if (ret) {
> +		hid_err(hdev, "failed to create sysfs attributes\n");
> +		goto fail;
> +	}

No, you need to set the attribute group _before_ you call
led_classdev_register, as that is where the device will be created in
sysfs.  Surely the other led drivers already do this?  I'm almost afraid
to go look...

You also have to document your sysfs file in Documentation/ABI/

thanks,

greg k-h
--
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
Greg KH June 23, 2014, 6:24 p.m. UTC | #3
On Mon, Jun 23, 2014 at 02:23:24PM -0400, Greg KH wrote:
> On Mon, Jun 23, 2014 at 08:16:48PM +0300, Janne Kanniainen wrote:
> > +	ret = sysfs_create_group(&led->hdev->dev.kobj, &gt683r_attribute_group);
> > +	if (ret) {
> > +		hid_err(hdev, "failed to create sysfs attributes\n");
> > +		goto fail;
> > +	}
> 
> No, you need to set the attribute group _before_ you call
> led_classdev_register, as that is where the device will be created in
> sysfs.  Surely the other led drivers already do this?  I'm almost afraid
> to go look...

Yes, they do it already, set .dev_attr_group and you should be fine.

thanks,

greg k-h
--
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
Johan Hovold June 23, 2014, 7:31 p.m. UTC | #4
On Mon, Jun 23, 2014 at 02:24:32PM -0400, Greg KH wrote:
> On Mon, Jun 23, 2014 at 02:23:24PM -0400, Greg KH wrote:
> > On Mon, Jun 23, 2014 at 08:16:48PM +0300, Janne Kanniainen wrote:
> > > +	ret = sysfs_create_group(&led->hdev->dev.kobj, &gt683r_attribute_group);
> > > +	if (ret) {
> > > +		hid_err(hdev, "failed to create sysfs attributes\n");
> > > +		goto fail;
> > > +	}
> > 
> > No, you need to set the attribute group _before_ you call
> > led_classdev_register, as that is where the device will be created in
> > sysfs.  Surely the other led drivers already do this?  I'm almost afraid
> > to go look...
> 
> Yes, they do it already, set .dev_attr_group and you should be fine.

But this isn't an attribute of the LEDs but rather of the parent HID
device that is being probed (the led_mode is common to all three LEDs
and thus belongs in the parent device, right?).

Johan
--
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
Greg KH June 23, 2014, 7:40 p.m. UTC | #5
On Mon, Jun 23, 2014 at 09:31:34PM +0200, Johan Hovold wrote:
> On Mon, Jun 23, 2014 at 02:24:32PM -0400, Greg KH wrote:
> > On Mon, Jun 23, 2014 at 02:23:24PM -0400, Greg KH wrote:
> > > On Mon, Jun 23, 2014 at 08:16:48PM +0300, Janne Kanniainen wrote:
> > > > +	ret = sysfs_create_group(&led->hdev->dev.kobj, &gt683r_attribute_group);
> > > > +	if (ret) {
> > > > +		hid_err(hdev, "failed to create sysfs attributes\n");
> > > > +		goto fail;
> > > > +	}
> > > 
> > > No, you need to set the attribute group _before_ you call
> > > led_classdev_register, as that is where the device will be created in
> > > sysfs.  Surely the other led drivers already do this?  I'm almost afraid
> > > to go look...
> > 
> > Yes, they do it already, set .dev_attr_group and you should be fine.
> 
> But this isn't an attribute of the LEDs but rather of the parent HID
> device that is being probed (the led_mode is common to all three LEDs
> and thus belongs in the parent device, right?).

Then that's even worse :(

The sysfs attribute should be on the class device here for the LED, you
should not put an attribute on a device you are not the driver for.

greg k-h
--
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
Johan Hovold June 23, 2014, 7:52 p.m. UTC | #6
On Mon, Jun 23, 2014 at 03:40:59PM -0400, Greg KH wrote:
> On Mon, Jun 23, 2014 at 09:31:34PM +0200, Johan Hovold wrote:
> > On Mon, Jun 23, 2014 at 02:24:32PM -0400, Greg KH wrote:
> > > On Mon, Jun 23, 2014 at 02:23:24PM -0400, Greg KH wrote:
> > > > On Mon, Jun 23, 2014 at 08:16:48PM +0300, Janne Kanniainen wrote:
> > > > > +	ret = sysfs_create_group(&led->hdev->dev.kobj, &gt683r_attribute_group);
> > > > > +	if (ret) {
> > > > > +		hid_err(hdev, "failed to create sysfs attributes\n");
> > > > > +		goto fail;
> > > > > +	}
> > > > 
> > > > No, you need to set the attribute group _before_ you call
> > > > led_classdev_register, as that is where the device will be created in
> > > > sysfs.  Surely the other led drivers already do this?  I'm almost afraid
> > > > to go look...
> > > 
> > > Yes, they do it already, set .dev_attr_group and you should be fine.
> > 
> > But this isn't an attribute of the LEDs but rather of the parent HID
> > device that is being probed (the led_mode is common to all three LEDs
> > and thus belongs in the parent device, right?).
> 
> Then that's even worse :(
> 
> The sysfs attribute should be on the class device here for the LED, you
> should not put an attribute on a device you are not the driver for.

But this is the driver for the HID device, which then in turn has three
individual LEDs. This particular device isn't really an input device
(the actual keyboard in this case appears to be connected over PS2), but
there are several HID drivers which are primarily input devices with
LEDs as sub-devices (e.g. drivers/hid/hid-lg4ff.c).

Johan
--
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
Greg KH June 23, 2014, 8:24 p.m. UTC | #7
On Mon, Jun 23, 2014 at 09:52:12PM +0200, Johan Hovold wrote:
> On Mon, Jun 23, 2014 at 03:40:59PM -0400, Greg KH wrote:
> > On Mon, Jun 23, 2014 at 09:31:34PM +0200, Johan Hovold wrote:
> > > On Mon, Jun 23, 2014 at 02:24:32PM -0400, Greg KH wrote:
> > > > On Mon, Jun 23, 2014 at 02:23:24PM -0400, Greg KH wrote:
> > > > > On Mon, Jun 23, 2014 at 08:16:48PM +0300, Janne Kanniainen wrote:
> > > > > > +	ret = sysfs_create_group(&led->hdev->dev.kobj, &gt683r_attribute_group);
> > > > > > +	if (ret) {
> > > > > > +		hid_err(hdev, "failed to create sysfs attributes\n");
> > > > > > +		goto fail;
> > > > > > +	}
> > > > > 
> > > > > No, you need to set the attribute group _before_ you call
> > > > > led_classdev_register, as that is where the device will be created in
> > > > > sysfs.  Surely the other led drivers already do this?  I'm almost afraid
> > > > > to go look...
> > > > 
> > > > Yes, they do it already, set .dev_attr_group and you should be fine.
> > > 
> > > But this isn't an attribute of the LEDs but rather of the parent HID
> > > device that is being probed (the led_mode is common to all three LEDs
> > > and thus belongs in the parent device, right?).
> > 
> > Then that's even worse :(
> > 
> > The sysfs attribute should be on the class device here for the LED, you
> > should not put an attribute on a device you are not the driver for.
> 
> But this is the driver for the HID device, which then in turn has three
> individual LEDs. This particular device isn't really an input device
> (the actual keyboard in this case appears to be connected over PS2), but
> there are several HID drivers which are primarily input devices with
> LEDs as sub-devices (e.g. drivers/hid/hid-lg4ff.c).

I don't know the specifics here, but as you just created a class device,
why aren't the attributes on that class device?  Shouldn't that be the
logical place for it, instead of having them on some "random" other type
of device?  Userspace will never be notified that the attribute is on
that device due to the file being created "later", and tools using
libudev and the like will be looking at the LED class device, not the
"parent" device for any specific LED stuff.

but if this really is the way that all LED devices work, and userspace
programs are expecting this, that's seems odd.

greg k-h
--
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
Johan Hovold June 23, 2014, 8:44 p.m. UTC | #8
On Mon, Jun 23, 2014 at 04:24:48PM -0400, Greg KH wrote:
> On Mon, Jun 23, 2014 at 09:52:12PM +0200, Johan Hovold wrote:
> > On Mon, Jun 23, 2014 at 03:40:59PM -0400, Greg KH wrote:
> > > On Mon, Jun 23, 2014 at 09:31:34PM +0200, Johan Hovold wrote:
> > > > On Mon, Jun 23, 2014 at 02:24:32PM -0400, Greg KH wrote:
> > > > > On Mon, Jun 23, 2014 at 02:23:24PM -0400, Greg KH wrote:
> > > > > > On Mon, Jun 23, 2014 at 08:16:48PM +0300, Janne Kanniainen wrote:
> > > > > > > +	ret = sysfs_create_group(&led->hdev->dev.kobj, &gt683r_attribute_group);
> > > > > > > +	if (ret) {
> > > > > > > +		hid_err(hdev, "failed to create sysfs attributes\n");
> > > > > > > +		goto fail;
> > > > > > > +	}
> > > > > > 
> > > > > > No, you need to set the attribute group _before_ you call
> > > > > > led_classdev_register, as that is where the device will be created in
> > > > > > sysfs.  Surely the other led drivers already do this?  I'm almost afraid
> > > > > > to go look...
> > > > > 
> > > > > Yes, they do it already, set .dev_attr_group and you should be fine.
> > > > 
> > > > But this isn't an attribute of the LEDs but rather of the parent HID
> > > > device that is being probed (the led_mode is common to all three LEDs
> > > > and thus belongs in the parent device, right?).
> > > 
> > > Then that's even worse :(
> > > 
> > > The sysfs attribute should be on the class device here for the LED, you
> > > should not put an attribute on a device you are not the driver for.
> > 
> > But this is the driver for the HID device, which then in turn has three
> > individual LEDs. This particular device isn't really an input device
> > (the actual keyboard in this case appears to be connected over PS2), but
> > there are several HID drivers which are primarily input devices with
> > LEDs as sub-devices (e.g. drivers/hid/hid-lg4ff.c).
> 
> I don't know the specifics here, but as you just created a class device,
> why aren't the attributes on that class device?  Shouldn't that be the
> logical place for it, instead of having them on some "random" other type
> of device?

This is a non-standard attribute of this particular laptop. It has three
individual LEDs that can be enabled separately (using standard LED class
attributes), but they will all three be in the same "mode" (which here
apparently means that they can be fully on, vary with the volume(?!), or
pulse synchronously when enabled).

If we were to implement this mode attribute as a class attribute,
changing the mode of of one LED would also change the mode of the other
two devices (LEDs). Therefore I think it has to be an attribute of the
parent HID device (that the driver is for).

> Userspace will never be notified that the attribute is on
> that device due to the file being created "later", and tools using
> libudev and the like will be looking at the LED class device, not the
> "parent" device for any specific LED stuff.

Yeah, that seems to be the case. I doubt anyone will care much about
this particular custom attribute, but sure, setting the led_mode of the
HID device from an udev rule could be problematic.

> but if this really is the way that all LED devices work, and userspace
> programs are expecting this, that's seems odd.

Johan
--
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
Bjørn Mork June 24, 2014, 1:10 p.m. UTC | #9
Johan Hovold <johan@kernel.org> writes:

> This is a non-standard attribute of this particular laptop. It has three
> individual LEDs that can be enabled separately (using standard LED class
> attributes), but they will all three be in the same "mode" (which here
> apparently means that they can be fully on, vary with the volume(?!), or
> pulse synchronously when enabled).
>
> If we were to implement this mode attribute as a class attribute,
> changing the mode of of one LED would also change the mode of the other
> two devices (LEDs).

Document this behaviour and it becomes a feature.


Bjørn
--
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
Johan Hovold June 24, 2014, 2:50 p.m. UTC | #10
On Tue, Jun 24, 2014 at 03:10:43PM +0200, Bjørn Mork wrote:
> Johan Hovold <johan@kernel.org> writes:
> 
> > This is a non-standard attribute of this particular laptop. It has three
> > individual LEDs that can be enabled separately (using standard LED class
> > attributes), but they will all three be in the same "mode" (which here
> > apparently means that they can be fully on, vary with the volume(?!), or
> > pulse synchronously when enabled).
> >
> > If we were to implement this mode attribute as a class attribute,
> > changing the mode of of one LED would also change the mode of the other
> > two devices (LEDs).
> 
> Document this behaviour and it becomes a feature.

Yeah, you're right. That is probably the best way to handle this.

Janne, could you send two separate patches on top of v9 that fixes the
use-before-initialisation (reported by Oliver) and moves the led_mode
attribute from the parent HID device to the led-class devices (thereby
fixing the race reported by Greg)?

As the "led_"-prefix will then be redundant, you should probably rename
the attribute as "msi_mode" or similar (just "mode" might be too
generic).

Remember to update the ABI documentation as well, including mentioning
the fact that changing the mode of one LED will update the mode of its
two sibling devices as well (as suggested by Bjørn).

Thanks,
Johan
--
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
diff mbox

Patch

diff --git a/Documentation/ABI/testing/sysfs-class-hid-driver-gt683r b/Documentation/ABI/testing/sysfs-class-hid-driver-gt683r
new file mode 100644
index 0000000..317e9d5
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-hid-driver-gt683r
@@ -0,0 +1,14 @@ 
+What:		/sys/class/hidraw/<hidraw>/device/leds_mode
+Date:		Jun 2014
+KernelVersion:	3.17
+Contact:	Janne Kanniainen <janne.kanniainen@gmail.com>
+Description:
+		Set the mode of LEDs
+
+		0 - normal
+		1 - audio
+		2 - breathing
+
+		Normal: LEDs are fully on when enabled
+		Audio:  LEDs brightness depends on sound level
+		Breathing: LEDs brightness varies at human breathing rate
\ No newline at end of file
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 7af9d0b..e2f4590 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -261,6 +261,20 @@  config HOLTEK_FF
 	  Say Y here if you have a Holtek On Line Grip based game controller
 	  and want to have force feedback support for it.
 
+config HID_GT683R
+	tristate "MSI GT68xR LED support"
+	depends on LEDS_CLASS && USB_HID
+	---help---
+	Say Y here if you want to enable support for the three MSI GT68xR LEDs
+
+	This driver support following modes:
+	  - Normal: LEDs are fully on when enabled
+	  - Audio:  LEDs brightness depends on sound level
+	  - Breathing: LEDs brightness varies at human breathing rate
+
+	Currently the following devices are know to be supported:
+	  - MSI GT683R
+
 config HID_HUION
 	tristate "Huion tablets"
 	depends on USB_HID
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index fc712dd..7129311 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -48,6 +48,7 @@  obj-$(CONFIG_HID_EMS_FF)	+= hid-emsff.o
 obj-$(CONFIG_HID_ELECOM)	+= hid-elecom.o
 obj-$(CONFIG_HID_ELO)		+= hid-elo.o
 obj-$(CONFIG_HID_EZKEY)		+= hid-ezkey.o
+obj-$(CONFIG_HID_GT683R)	+= hid-gt683r.o
 obj-$(CONFIG_HID_GYRATION)	+= hid-gyration.o
 obj-$(CONFIG_HID_HOLTEK)	+= hid-holtek-kbd.o
 obj-$(CONFIG_HID_HOLTEK)	+= hid-holtek-mouse.o
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index da52279..ec88fdb 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1827,6 +1827,7 @@  static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_WIRELESS_OPTICAL_DESKTOP_3_0) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_OFFICE_KB) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_MONTEREY, USB_DEVICE_ID_GENIUS_KB29E) },
+	{ HID_USB_DEVICE(USB_VENDOR_ID_MSI, USB_DEVICE_ID_MSI_GT683R_LED_PANEL) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_NTRIG, USB_DEVICE_ID_NTRIG_TOUCH_SCREEN) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_NTRIG, USB_DEVICE_ID_NTRIG_TOUCH_SCREEN_1) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_NTRIG, USB_DEVICE_ID_NTRIG_TOUCH_SCREEN_2) },
diff --git a/drivers/hid/hid-gt683r.c b/drivers/hid/hid-gt683r.c
new file mode 100644
index 0000000..5bd0113
--- /dev/null
+++ b/drivers/hid/hid-gt683r.c
@@ -0,0 +1,318 @@ 
+/*
+ * MSI GT683R led driver
+ *
+ * Copyright (c) 2014 Janne Kanniainen <janne.kanniainen@gmail.com>
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/hid.h>
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+
+#include "hid-ids.h"
+
+#define GT683R_BUFFER_SIZE			8
+
+/*
+ * GT683R_LED_OFF: all LEDs are off
+ * GT683R_LED_AUDIO: LEDs brightness depends on sound level
+ * GT683R_LED_BREATHING: LEDs brightness varies at human breathing rate
+ * GT683R_LED_NORMAL: LEDs are fully on when enabled
+ */
+enum gt683r_led_mode {
+	GT683R_LED_OFF = 0,
+	GT683R_LED_AUDIO = 2,
+	GT683R_LED_BREATHING = 3,
+	GT683R_LED_NORMAL = 5
+};
+
+enum gt683r_panels {
+	GT683R_LED_BACK = 0,
+	GT683R_LED_SIDE = 1,
+	GT683R_LED_FRONT = 2,
+	GT683R_LED_COUNT,
+};
+
+static const char * const gt683r_panel_names[] = {
+	"back",
+	"side",
+	"front",
+};
+
+struct gt683r_led {
+	struct hid_device *hdev;
+	struct led_classdev led_devs[GT683R_LED_COUNT];
+	struct mutex lock;
+	struct work_struct work;
+	enum led_brightness brightnesses[GT683R_LED_COUNT];
+	enum gt683r_led_mode mode;
+};
+
+static const struct hid_device_id gt683r_led_id[] = {
+	{ HID_USB_DEVICE(USB_VENDOR_ID_MSI, USB_DEVICE_ID_MSI_GT683R_LED_PANEL) },
+	{ }
+};
+
+static void gt683r_brightness_set(struct led_classdev *led_cdev,
+				enum led_brightness brightness)
+{
+	int i;
+	struct device *dev = led_cdev->dev->parent;
+	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
+	struct gt683r_led *led = hid_get_drvdata(hdev);
+
+	for (i = 0; i < GT683R_LED_COUNT; i++) {
+		if (led_cdev == &led->led_devs[i])
+			break;
+	}
+
+	if (i < GT683R_LED_COUNT) {
+		led->brightnesses[i] = brightness;
+		schedule_work(&led->work);
+	}
+}
+
+static ssize_t leds_mode_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	u8 sysfs_mode;
+	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
+	struct gt683r_led *led = hid_get_drvdata(hdev);
+
+	if (led->mode == GT683R_LED_NORMAL)
+		sysfs_mode = 0;
+	else if (led->mode == GT683R_LED_AUDIO)
+		sysfs_mode = 1;
+	else
+		sysfs_mode = 2;
+
+	return scnprintf(buf, PAGE_SIZE, "%u\n", sysfs_mode);
+}
+
+static ssize_t leds_mode_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	u8 sysfs_mode;
+	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
+	struct gt683r_led *led = hid_get_drvdata(hdev);
+
+
+	if (kstrtou8(buf, 10, &sysfs_mode) || sysfs_mode > 2)
+		return -EINVAL;
+
+	mutex_lock(&led->lock);
+
+	if (sysfs_mode == 0)
+		led->mode = GT683R_LED_NORMAL;
+	else if (sysfs_mode == 1)
+		led->mode = GT683R_LED_AUDIO;
+	else
+		led->mode = GT683R_LED_BREATHING;
+
+	mutex_unlock(&led->lock);
+	schedule_work(&led->work);
+
+	return count;
+}
+
+static int gt683r_led_snd_msg(struct gt683r_led *led, u8 *msg)
+{
+	int ret;
+
+	ret = hid_hw_raw_request(led->hdev, msg[0], msg, GT683R_BUFFER_SIZE,
+				HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+	if (ret != GT683R_BUFFER_SIZE) {
+		hid_err(led->hdev,
+			"failed to send set report request: %i\n", ret);
+		if (ret < 0)
+			return ret;
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int gt683r_leds_set(struct gt683r_led *led, u8 leds)
+{
+	int ret;
+	u8 *buffer;
+
+	buffer = kzalloc(GT683R_BUFFER_SIZE, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	buffer[0] = 0x01;
+	buffer[1] = 0x02;
+	buffer[2] = 0x30;
+	buffer[3] = leds;
+	ret = gt683r_led_snd_msg(led, buffer);
+
+	kfree(buffer);
+	return ret;
+}
+
+static int gt683r_mode_set(struct gt683r_led *led, u8 mode)
+{
+	int ret;
+	u8 *buffer;
+
+	buffer = kzalloc(GT683R_BUFFER_SIZE, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	buffer[0] = 0x01;
+	buffer[1] = 0x02;
+	buffer[2] = 0x20;
+	buffer[3] = mode;
+	buffer[4] = 0x01;
+	ret = gt683r_led_snd_msg(led, buffer);
+
+	kfree(buffer);
+	return ret;
+}
+
+static void gt683r_led_work(struct work_struct *work)
+{
+	int i;
+	u8 leds = 0;
+	u8 mode;
+	struct gt683r_led *led = container_of(work, struct gt683r_led, work);
+
+	mutex_lock(&led->lock);
+
+	for (i = 0; i < GT683R_LED_COUNT; i++) {
+		if (led->brightnesses[i])
+			leds |= BIT(i);
+	}
+
+	if (gt683r_leds_set(led, leds))
+		goto fail;
+
+	if (leds)
+		mode = led->mode;
+	else
+		mode = GT683R_LED_OFF;
+
+	gt683r_mode_set(led, mode);
+fail:
+	mutex_unlock(&led->lock);
+}
+
+static DEVICE_ATTR_RW(leds_mode);
+
+static struct attribute *gt683r_attributes[] = {
+	&dev_attr_leds_mode.attr,
+	NULL,
+};
+
+static struct attribute_group gt683r_attribute_group = {
+	.attrs = gt683r_attributes
+};
+
+static int gt683r_led_probe(struct hid_device *hdev,
+			const struct hid_device_id *id)
+{
+	int i;
+	int ret;
+	int name_sz;
+	char *name;
+	struct gt683r_led *led;
+
+	led = devm_kzalloc(&hdev->dev, sizeof(*led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	mutex_init(&led->lock);
+	INIT_WORK(&led->work, gt683r_led_work);
+
+	led->mode = GT683R_LED_NORMAL;
+	led->hdev = hdev;
+	hid_set_drvdata(hdev, led);
+
+	ret = hid_parse(hdev);
+	if (ret) {
+		hid_err(hdev, "hid parsing failed\n");
+		return ret;
+	}
+
+	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+	if (ret) {
+		hid_err(hdev, "hw start failed\n");
+		return ret;
+	}
+
+	for (i = 0; i < GT683R_LED_COUNT; i++) {
+		name_sz = strlen(dev_name(&hdev->dev)) +
+				strlen(gt683r_panel_names[i]) + 3;
+
+		name = devm_kzalloc(&hdev->dev, name_sz, GFP_KERNEL);
+		if (!name) {
+			ret = -ENOMEM;
+			goto fail;
+		}
+
+		snprintf(name, name_sz, "%s::%s",
+				dev_name(&hdev->dev), gt683r_panel_names[i]);
+		led->led_devs[i].name = name;
+		led->led_devs[i].max_brightness = 1;
+		led->led_devs[i].brightness_set = gt683r_brightness_set;
+		ret = led_classdev_register(&hdev->dev, &led->led_devs[i]);
+		if (ret) {
+			hid_err(hdev, "could not register led device\n");
+			goto fail;
+		}
+	}
+
+	ret = sysfs_create_group(&led->hdev->dev.kobj, &gt683r_attribute_group);
+	if (ret) {
+		hid_err(hdev, "failed to create sysfs attributes\n");
+		goto fail;
+	}
+
+	return 0;
+
+fail:
+	for (i = i - 1; i >= 0; i--)
+		led_classdev_unregister(&led->led_devs[i]);
+	hid_hw_stop(hdev);
+	return ret;
+}
+
+static void gt683r_led_remove(struct hid_device *hdev)
+{
+	int i;
+	struct gt683r_led *led = hid_get_drvdata(hdev);
+
+	sysfs_remove_group(&led->hdev->dev.kobj, &gt683r_attribute_group);
+	for (i = 0; i < GT683R_LED_COUNT; i++)
+		led_classdev_unregister(&led->led_devs[i]);
+	flush_work(&led->work);
+	hid_hw_stop(hdev);
+}
+
+static struct hid_driver gt683r_led_driver = {
+	.probe = gt683r_led_probe,
+	.remove = gt683r_led_remove,
+	.name = "gt683r_led",
+	.id_table = gt683r_led_id,
+};
+
+module_hid_driver(gt683r_led_driver);
+
+MODULE_AUTHOR("Janne Kanniainen");
+MODULE_DESCRIPTION("MSI GT683R led driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 34bb220..3692d37 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -641,7 +641,7 @@ 
 #define USB_DEVICE_ID_GENIUS_KB29E	0x3004
 
 #define USB_VENDOR_ID_MSI		0x1770
-#define USB_DEVICE_ID_MSI_GX680R_LED_PANEL	0xff00
+#define USB_DEVICE_ID_MSI_GT683R_LED_PANEL 0xff00
 
 #define USB_VENDOR_ID_NATIONAL_SEMICONDUCTOR 0x0400
 #define USB_DEVICE_ID_N_S_HARMONY	0xc359
diff --git a/drivers/hid/usbhid/hid-quirks.c b/drivers/hid/usbhid/hid-quirks.c
index 8e4ddb3..c640e1d 100644
--- a/drivers/hid/usbhid/hid-quirks.c
+++ b/drivers/hid/usbhid/hid-quirks.c
@@ -73,7 +73,7 @@  static const struct hid_blacklist {
 	{ USB_VENDOR_ID_FORMOSA, USB_DEVICE_ID_FORMOSA_IR_RECEIVER, HID_QUIRK_NO_INIT_REPORTS },
 	{ USB_VENDOR_ID_FREESCALE, USB_DEVICE_ID_FREESCALE_MX28, HID_QUIRK_NOGET },
 	{ USB_VENDOR_ID_MGE, USB_DEVICE_ID_MGE_UPS, HID_QUIRK_NOGET },
-	{ USB_VENDOR_ID_MSI, USB_DEVICE_ID_MSI_GX680R_LED_PANEL, HID_QUIRK_NO_INIT_REPORTS },
+	{ USB_VENDOR_ID_MSI, USB_DEVICE_ID_MSI_GT683R_LED_PANEL, HID_QUIRK_NO_INIT_REPORTS },
 	{ USB_VENDOR_ID_NEXIO, USB_DEVICE_ID_NEXIO_MULTITOUCH_PTI0750, HID_QUIRK_NO_INIT_REPORTS },
 	{ USB_VENDOR_ID_NOVATEK, USB_DEVICE_ID_NOVATEK_MOUSE, HID_QUIRK_NO_INIT_REPORTS },
 	{ USB_VENDOR_ID_PIXART, USB_DEVICE_ID_PIXART_OPTICAL_TOUCH_SCREEN, HID_QUIRK_NO_INIT_REPORTS },