From: Johan Hovold <johan@kernel.org>
To: Janne Kanniainen <janne.kanniainen@gmail.com>
Cc: cooloney@gmail.com, rpurdie@rpsys.net,
linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org,
linux-usb@vger.kernel.org, Jiri Kosina <jkosina@suse.cz>,
linux-input@vger.kernel.org, Johan Hovold <johan@kernel.org>
Subject: Re: [PATCH v2] leds: USB: Add support for MSI GT683R led panels
Date: Fri, 6 Jun 2014 11:47:52 +0200 [thread overview]
Message-ID: <20140606094752.GB9307@localhost> (raw)
In-Reply-To: <1402003746-6354-1-git-send-email-janne.kanniainen@gmail.com>
[ +CC: Jiri, linux-input, linux-usb (again) ]
First of all, please reply to the original thread and make sure to not
drop people or lists from CC.
On Fri, Jun 06, 2014 at 12:29:06AM +0300, Janne Kanniainen wrote:
> This driver adds support for USB controlled led panels that exist in
> MSI GT683R laptop.
I had a change to look at bit closer at this today and it turns out this
is a HID device and as such is already claimed by the HID driver.
There was a quirk added for this particular VID/PID by commit
620ae90ed8ca ("HID: usbhid: quirk for MSI GX680R led panel") in order to
avoid a 10s delay at boot due to missing report descriptors:
https://bugzilla.redhat.com/show_bug.cgi?id=907221
http://www.spinics.net/lists/linux-usb/msg54756.html
Since this is a HID device (and the control requests you're doing are
Set_Report requests) you should probably implement this as a specific
HID driver if there's no generic support you can use directly.
I've added Jiri Kosina (and linux-input) who knows a lot more about the
HID layer and how this is supposed to be implemented as CC.
> 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
There are still some issues in the code below, but I'll only point out
the more problematic ones for now.
> Signed-off-by: Janne Kanniainen <janne.kanniainen@gmail.com>
> ---
> drivers/leds/Kconfig | 6 ++
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-gt683r.c | 158 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 165 insertions(+)
> create mode 100644 drivers/leds/leds-gt683r.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 6de9dfb..2cffa0c 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -487,6 +487,12 @@ config LEDS_BLINKM
> This option enables support for the BlinkM RGB LED connected
> through I2C. Say Y to enable support for the BlinkM LED.
>
> +config LEDS_GT683R
> + tristate "LED support for the MSI GT683R"
> + depends on LEDS_CLASS && USB
> + help
> + This option enables support for the MSI GT683R LEDS
> +
> comment "LED Triggers"
> source "drivers/leds/trigger/Kconfig"
>
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 3cd76db..af5fb4e 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -54,6 +54,7 @@ obj-$(CONFIG_LEDS_ASIC3) += leds-asic3.o
> obj-$(CONFIG_LEDS_MAX8997) += leds-max8997.o
> obj-$(CONFIG_LEDS_LM355x) += leds-lm355x.o
> obj-$(CONFIG_LEDS_BLINKM) += leds-blinkm.o
> +obj-$(CONFIG_LEDS_GT683R) += leds-gt683r.o
>
> # LED SPI Drivers
> obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
> diff --git a/drivers/leds/leds-gt683r.c b/drivers/leds/leds-gt683r.c
> new file mode 100644
> index 0000000..5a204ea
> --- /dev/null
> +++ b/drivers/leds/leds-gt683r.c
> @@ -0,0 +1,158 @@
> +/*
> + * 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/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +
> +#define USB_DEVICE_ID_MSI_GT683R_LED_PANEL 0xff00
> +#define USB_VENDOR_ID_MSI 0x1770
> +
> +#define GT683R_LED_BACK BIT(0)
> +#define GT683R_LED_SIDE BIT(1)
> +#define GT683R_LED_FRONT BIT(2)
> +#define GT683R_LED_ALL (GT683R_LED_BACK | GT683R_LED_SIDE | GT683R_LED_FRONT)
> +
> +#define GT683R_LED_OFF 0
> +#define GT683R_LED_AUDIO 2
> +#define GT683R_LED_BREATHING 3
> +#define GT683R_LED_NORMAL 5
> +
> +struct gt683r_led {
> + struct usb_device *usb_dev;
> + struct usb_endpoint_descriptor *ep;
> + struct led_classdev led_dev;
> + struct mutex lock;
> + struct work_struct work;
> + enum led_brightness brightness;
> +};
> +
> +static const struct usb_device_id gt683r_led_id[] = {
> + { USB_DEVICE(USB_VENDOR_ID_MSI, USB_DEVICE_ID_MSI_GT683R_LED_PANEL) },
> + { }
> +};
> +
> +static const u64 gt683r_led_select_leds = 0x300201ULL;
> +static const u64 gt683r_led_select_type = 0x100200201ULL;
No, you'd still want to use byte arrays for this in order to handle
endianess (if you are to keep these at all).
For arrays you can use the ARRAY_SIZE() macro if that was the reason for
this change. I should have mentioned that when I pointed out that you
cannot use strlen().
Where did you get these (HID report) values from by the way?
> +
> +static void gt683r_brightness_set(struct led_classdev *led_cdev,
> + enum led_brightness brightness)
> +{
> + struct gt683r_led *led =
> + container_of(led_cdev, struct gt683r_led, led_dev);
> +
> + mutex_lock(&led->lock);
You cannot grab a mutex here since this function can be called from
interrupt context (if I remember correctly). Either way, you shouldn't
be holding the lock until the work task has finished...
> +
> + led->brightness = brightness;
> +
> + schedule_work(&led->work);
> +}
> +
> +static int gt683r_led_snd_msg(struct gt683r_led *led, u64 msg)
> +{
> + int result = usb_control_msg(led->usb_dev,
> + usb_sndctrlpipe(led->usb_dev,
> + led->ep->bEndpointAddress),
No need to store the endpoint in the led struct as the address of
endpoint 0 is 0.
> + 0x09, 0x22, 0x0301, 0x0000,
> + &msg, sizeof(u64), USB_CTRL_SET_TIMEOUT);
You cannot use a stack allocated buffer (msg) for the data transfer as
some platforms can't do DMA from the stack.
> + if (result)
> + dev_err(&led->usb_dev->dev,
> + "Failed to send control message: %i\n", result);
> +
> + return result;
> +}
> +
> +static void gt683r_led_set(struct gt683r_led *led, char type)
> +{
> + gt683r_led_snd_msg(led, gt683r_led_select_leds |
> + (GT683R_LED_ALL << 24));
> + gt683r_led_snd_msg(led, gt683r_led_select_type |
> + (type << 24));
> +}
> +
> +static void gt683r_led_work(struct work_struct *work)
> +{
> + struct gt683r_led *led =
> + container_of(work, struct gt683r_led, work);
> +
> + if (led->brightness)
> + gt683r_led_set(led, GT683R_LED_NORMAL);
> + else
> + gt683r_led_set(led, GT683R_LED_OFF);
> +
> + mutex_unlock(&led->lock);
> +}
> +
> +static struct led_classdev gt683r_led_dev = {
> + .name = "gt683r-led",
> + .brightness_set = gt683r_brightness_set,
> + .max_brightness = 1,
> + .flags = LED_CORE_SUSPENDRESUME,
> +};
> +
> +static int gt683r_led_probe(struct usb_interface *intf,
> + const struct usb_device_id *id)
> +{
> + int ret = 0;
> + struct gt683r_led *led =
> + devm_kzalloc(&intf->dev, sizeof(struct gt683r_led), GFP_KERNEL);
Again, please split definition and allocation.
> + if (!led)
> + return -ENOMEM;
> +
> + led->usb_dev = interface_to_usbdev(intf);
> + usb_set_intfdata(intf, led);
> +
> + led->ep = &led->usb_dev->ep0.desc;
> + if (!usb_endpoint_xfer_control(led->ep) ||
> + usb_endpoint_maxp(led->ep) != sizeof(u64))
> + return -EINVAL;
EP0 is a control endpoint, and you probably don't need the
max-packet-size test either.
> +
> + led->led_dev = gt683r_led_dev;
> + ret = led_classdev_register(&led->usb_dev->dev, &led->led_dev);
> + if (ret) {
> + dev_err(&led->usb_dev->dev, "Could not register led_classdev\n");
> + return ret;
> + }
> +
> + mutex_init(&led->lock);
> + INIT_WORK(&led->work, gt683r_led_work);
> +
> + return 0;
> +}
> +
> +static void gt683r_led_disconnect(struct usb_interface *intf)
> +{
> + struct gt683r_led *led = usb_get_intfdata(intf);
> +
> + led_classdev_unregister(&led->led_dev);
> + cancel_work_sync(&led->work);
> +}
> +
> +static struct usb_driver gt683r_led_driver = {
> + .probe = gt683r_led_probe,
> + .disconnect = gt683r_led_disconnect,
> + .name = "GT683R Led Driver",
> + .id_table = gt683r_led_id,
> +};
> +
> +module_usb_driver(gt683r_led_driver);
> +
> +MODULE_AUTHOR("Janne Kanniainen");
> +MODULE_DESCRIPTION("MSI GT683R led driver");
> +MODULE_LICENSE("GPL");
Johan
next prev parent reply other threads:[~2014-06-06 9:48 UTC|newest]
Thread overview: 74+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-06-05 21:29 [PATCH v2] leds: USB: Add support for MSI GT683R led panels Janne Kanniainen
2014-06-06 9:47 ` Johan Hovold [this message]
2014-06-07 10:12 ` Janne Kanniainen
2014-06-09 11:42 ` Johan Hovold
2014-06-10 21:10 ` Janne Kanniainen
2014-06-10 21:21 ` [PATCH v3] leds: USB: HID: " Janne Kanniainen
2014-06-11 11:25 ` Jiri Kosina
2014-06-11 14:06 ` Johan Hovold
2014-06-11 14:05 ` Johan Hovold
2014-06-11 15:30 ` Johan Hovold
2014-06-11 17:34 ` Johan Hovold
2014-06-11 22:48 ` [PATCH v4] " Janne Kanniainen
2014-06-12 9:06 ` Johan Hovold
2014-06-12 20:34 ` [PATCH v5] " Janne Kanniainen
2014-06-13 7:54 ` Johan Hovold
2014-06-13 17:19 ` Janne Kanniainen
2014-06-15 14:59 ` Janne Kanniainen
2014-06-16 7:39 ` Johan Hovold
2014-06-16 17:23 ` [PATCH v6] " Janne Kanniainen
2014-06-16 22:01 ` Janne Kanniainen
2014-06-17 13:46 ` Johan Hovold
2014-06-17 16:41 ` [PATCH v8] " Janne Kanniainen
2014-06-18 7:39 ` Johan Hovold
2014-06-18 16:05 ` [PATCH v9] " Janne Kanniainen
2014-06-18 16:11 ` Johan Hovold
2014-06-18 18:41 ` Janne Kanniainen
2014-06-18 18:46 ` Johan Hovold
2014-06-18 22:10 ` Jiri Kosina
2014-06-23 14:35 ` Oliver Neukum
2014-06-23 14:42 ` Johan Hovold
2014-06-23 16:17 ` Greg KH
2014-06-23 17:16 ` [PATCH v10] " Janne Kanniainen
2014-06-23 17:27 ` Johan Hovold
2014-06-23 18:23 ` Greg KH
2014-06-23 18:24 ` Greg KH
2014-06-23 19:31 ` Johan Hovold
2014-06-23 19:40 ` Greg KH
2014-06-23 19:52 ` Johan Hovold
2014-06-23 20:24 ` Greg KH
2014-06-23 20:44 ` Johan Hovold
2014-06-24 13:10 ` Bjørn Mork
2014-06-24 14:50 ` Johan Hovold
2014-06-24 19:38 ` [PATCH 1/2] HID: leds: fix race condition in MSI GT683R driver Janne Kanniainen
2014-06-24 19:38 ` [PATCH 2/2] HID: leds: move led_mode attribute to led-class devices " Janne Kanniainen
2014-06-24 19:56 ` Greg KH
2014-06-25 11:55 ` Johan Hovold
2014-06-25 15:59 ` [PATCH 2/2 v2] HID: leds: Use attribute-groups " Janne Kanniainen
2014-06-25 17:41 ` Johan Hovold
2014-06-25 18:13 ` [PATCH 2/2 v3] HID: leds: move led_mode attribute to led-class devices " Janne Kanniainen
2014-06-30 10:39 ` Johan Hovold
2014-07-01 17:50 ` [PATCH 2/2 v4] " Janne Kanniainen
2014-07-01 20:16 ` Johan Hovold
2014-07-02 17:37 ` [PATCH 2/2 v5] " Janne Kanniainen
2014-07-03 8:28 ` Johan Hovold
2014-07-03 17:17 ` [PATCH 1/2 v6] HID: gt683r: fix race condition Janne Kanniainen
2014-07-03 17:17 ` [PATCH 2/2 v6] HID: gt683r: move mode attribute to led-class devices Janne Kanniainen
2014-07-03 17:40 ` Johan Hovold
2014-07-03 18:17 ` Bryan Wu
2014-07-03 18:28 ` Janne Kanniainen
2014-07-03 17:34 ` [PATCH 1/2 v6] HID: gt683r: fix race condition Johan Hovold
2014-07-03 18:13 ` Bryan Wu
2014-06-25 19:09 ` [PATCH 2/2 v2] HID: leds: Use attribute-groups in MSI GT683R driver Jiri Kosina
2014-06-25 22:55 ` Bryan Wu
2014-06-30 10:47 ` Johan Hovold
2014-06-30 11:33 ` Jiri Kosina
2014-06-30 23:17 ` Bryan Wu
2014-07-01 8:48 ` Johan Hovold
2014-07-01 15:48 ` Bryan Wu
2014-07-01 17:53 ` Janne Kanniainen
2014-07-02 8:56 ` Jiri Kosina
2014-06-23 16:20 ` [PATCH v9] leds: USB: HID: Add support for MSI GT683R led panels Janne Kanniainen
2014-06-14 22:42 ` [PATCH v5] " Pavel Machek
2014-06-14 23:23 ` Janne Kanniainen
2014-06-16 7:45 ` Johan Hovold
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140606094752.GB9307@localhost \
--to=johan@kernel.org \
--cc=cooloney@gmail.com \
--cc=janne.kanniainen@gmail.com \
--cc=jkosina@suse.cz \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=rpurdie@rpsys.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).