* [PATCH] leds: Add Chrome OS keyboard backlight LEDs driver
@ 2016-03-03 23:46 Dmitry Torokhov
2016-03-04 8:38 ` Evan McClain
2016-03-04 9:38 ` Jacek Anaszewski
0 siblings, 2 replies; 13+ messages in thread
From: Dmitry Torokhov @ 2016-03-03 23:46 UTC (permalink / raw)
To: Richard Purdie, Jacek Anaszewski
Cc: Bryan Wu, Simon Que, Olof Johansson, Duncan Laurie,
Guenter Roeck, linux-kernel, linux-leds
From: Simon Que <sque@chromium.org>
This is a driver for ACPI-based keyboard backlight LEDs found on
Chromebooks. The driver locates \\_SB.KBLT ACPI device and exports
backlight as "chromeos::kbd_backlight" LED class device in sysfs.
Signed-off-by: Simon Que <sque@chromium.org>
Signed-off-by: Duncan Laurie <dlaurie@chromium.org>
Signed-off-by: Dmitry Torokhov <dtor@chromium.org>
---
drivers/leds/Kconfig | 11 ++++
drivers/leds/Makefile | 1 +
drivers/leds/leds-chromeos-keyboard.c | 106 ++++++++++++++++++++++++++++++++++
3 files changed, 118 insertions(+)
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index f86527cf..02aaaf1 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -581,6 +581,17 @@ config LEDS_SN3218
This driver can also be built as a module. If so the module
will be called leds-sn3218.
+config LEDS_CHROMEOS_KEYBOARD
+ tristate "LED backlight support for Chrome OS keyboards"
+ depends on LEDS_CLASS && ACPI
+ depends on CHROME_PLATFORMS || COMPILE_TEST
+ help
+ This option enables support for the keyboard backlight LEDs on
+ select Chrome OS systems.
+
+ This driver can also be built as a module. If so the module
+ will be called leds-chromeos-keyboard.
+
comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"
config LEDS_BLINKM
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 89c9b6f..5b96d15 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_LEDS_KTD2692) += leds-ktd2692.o
obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o
obj-$(CONFIG_LEDS_SEAD3) += leds-sead3.o
obj-$(CONFIG_LEDS_SN3218) += leds-sn3218.o
+obj-$(CONFIG_LEDS_CHROMEOS_KEYBOARD) += leds-chromeos-keyboard.o
# LED SPI Drivers
obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
diff --git a/drivers/leds/leds-chromeos-keyboard.c b/drivers/leds/leds-chromeos-keyboard.c
new file mode 100644
index 0000000..3592f09
--- /dev/null
+++ b/drivers/leds/leds-chromeos-keyboard.c
@@ -0,0 +1,106 @@
+/*
+ * Keyboard backlight LED driver for Chrome OS.
+ *
+ * Copyright (C) 2012 Google, Inc.
+ *
+ * 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/acpi.h>
+#include <linux/leds.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+/* Keyboard LED ACPI Device must be defined in firmware */
+#define ACPI_KEYBOARD_BACKLIGHT_DEVICE "\\_SB.KBLT"
+#define ACPI_KEYBOARD_BACKLIGHT_READ ACPI_KEYBOARD_BACKLIGHT_DEVICE ".KBQC"
+#define ACPI_KEYBOARD_BACKLIGHT_WRITE ACPI_KEYBOARD_BACKLIGHT_DEVICE ".KBCM"
+
+#define ACPI_KEYBOARD_BACKLIGHT_MAX 100
+
+static void keyboard_led_set_brightness(struct led_classdev *cdev,
+ enum led_brightness brightness)
+{
+ union acpi_object param;
+ struct acpi_object_list input;
+ acpi_status status;
+
+ param.type = ACPI_TYPE_INTEGER;
+ param.integer.value = brightness;
+ input.count = 1;
+ input.pointer = ¶m;
+
+ status = acpi_evaluate_object(NULL, ACPI_KEYBOARD_BACKLIGHT_WRITE,
+ &input, NULL);
+ if (ACPI_FAILURE(status))
+ dev_err(cdev->dev, "Error setting keyboard LED value: %d\n",
+ status);
+}
+
+static int keyboard_led_probe(struct platform_device *pdev)
+{
+ struct led_classdev *cdev;
+ acpi_handle handle;
+ acpi_status status;
+ int error;
+
+ /* Look for the keyboard LED ACPI Device */
+ status = acpi_get_handle(ACPI_ROOT_OBJECT,
+ ACPI_KEYBOARD_BACKLIGHT_DEVICE,
+ &handle);
+ if (ACPI_FAILURE(status)) {
+ dev_err(&pdev->dev, "Unable to find ACPI device %s: %d\n",
+ ACPI_KEYBOARD_BACKLIGHT_DEVICE, status);
+ return -ENXIO;
+ }
+
+ cdev = devm_kzalloc(&pdev->dev, sizeof(*cdev), GFP_KERNEL);
+ if (!cdev)
+ return -ENOMEM;
+
+ cdev->name = "chromeos::kbd_backlight";
+ cdev->brightness_set = keyboard_led_set_brightness;
+ cdev->max_brightness = ACPI_KEYBOARD_BACKLIGHT_MAX;
+ cdev->brightness = cdev->max_brightness;
+ cdev->flags |= LED_CORE_SUSPENDRESUME;
+
+ error = devm_led_classdev_register(&pdev->dev, cdev);
+ if (error)
+ return error;
+
+ platform_set_drvdata(pdev, cdev);
+ return 0;
+}
+
+static const struct acpi_device_id keyboard_led_id[] = {
+ { "GOOG0002", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, keyboard_led_id);
+
+static struct platform_driver keyboard_led_driver = {
+ .driver = {
+ .name = "chromeos-keyboard-leds",
+ .acpi_match_table = ACPI_PTR(keyboard_led_id),
+ },
+ .probe = keyboard_led_probe,
+};
+module_platform_driver(keyboard_led_driver);
+
+MODULE_AUTHOR("Simon Que <sque@chromium.org>");
+MODULE_DESCRIPTION("ChromeOS Keyboard backlight LED Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:chromeos-keyboard-leds");
--
2.7.0.rc3.207.g0ac5344
--
Dmitry
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] leds: Add Chrome OS keyboard backlight LEDs driver
2016-03-03 23:46 [PATCH] leds: Add Chrome OS keyboard backlight LEDs driver Dmitry Torokhov
@ 2016-03-04 8:38 ` Evan McClain
2016-03-04 9:38 ` Jacek Anaszewski
2016-03-04 9:38 ` Jacek Anaszewski
1 sibling, 1 reply; 13+ messages in thread
From: Evan McClain @ 2016-03-04 8:38 UTC (permalink / raw)
To: Dmitry Torokhov, Richard Purdie, Jacek Anaszewski
Cc: Bryan Wu, Simon Que, Olof Johansson, Duncan Laurie,
Guenter Roeck, linux-kernel, linux-leds
[-- Attachment #1: Type: text/plain, Size: 554 bytes --]
On Thu, 2016-03-03 at 15:46 -0800, Dmitry Torokhov wrote:
> From: Simon Que <sque@chromium.org>
>
> This is a driver for ACPI-based keyboard backlight LEDs found on
> Chromebooks. The driver locates \\_SB.KBLT ACPI device and exports
> backlight as "chromeos::kbd_backlight" LED class device in sysfs.
Was it ever decided where this driver should live? I was planning on
submitting to platform/chrome since most keyboard backlights seem to
live over there but I don't think I got a response.
--
Evan McClain
https://keybase.io/aeroevan
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] leds: Add Chrome OS keyboard backlight LEDs driver
2016-03-03 23:46 [PATCH] leds: Add Chrome OS keyboard backlight LEDs driver Dmitry Torokhov
2016-03-04 8:38 ` Evan McClain
@ 2016-03-04 9:38 ` Jacek Anaszewski
2016-03-04 19:19 ` Dmitry Torokhov
1 sibling, 1 reply; 13+ messages in thread
From: Jacek Anaszewski @ 2016-03-04 9:38 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Richard Purdie, Bryan Wu, Simon Que, Olof Johansson,
Duncan Laurie, Guenter Roeck, linux-kernel, linux-leds
Hi Dmitry,
Thanks for the patch. There's already other pending version [1],
still to be decided where it should live.
[1] http://comments.gmane.org/gmane.linux.leds/4523
Best regards,
Jacek Anaszewski
On 03/04/2016 12:46 AM, Dmitry Torokhov wrote:
> From: Simon Que <sque@chromium.org>
>
> This is a driver for ACPI-based keyboard backlight LEDs found on
> Chromebooks. The driver locates \\_SB.KBLT ACPI device and exports
> backlight as "chromeos::kbd_backlight" LED class device in sysfs.
>
> Signed-off-by: Simon Que <sque@chromium.org>
> Signed-off-by: Duncan Laurie <dlaurie@chromium.org>
> Signed-off-by: Dmitry Torokhov <dtor@chromium.org>
> ---
> drivers/leds/Kconfig | 11 ++++
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-chromeos-keyboard.c | 106 ++++++++++++++++++++++++++++++++++
> 3 files changed, 118 insertions(+)
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index f86527cf..02aaaf1 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -581,6 +581,17 @@ config LEDS_SN3218
> This driver can also be built as a module. If so the module
> will be called leds-sn3218.
>
> +config LEDS_CHROMEOS_KEYBOARD
> + tristate "LED backlight support for Chrome OS keyboards"
> + depends on LEDS_CLASS && ACPI
> + depends on CHROME_PLATFORMS || COMPILE_TEST
> + help
> + This option enables support for the keyboard backlight LEDs on
> + select Chrome OS systems.
> +
> + This driver can also be built as a module. If so the module
> + will be called leds-chromeos-keyboard.
> +
> comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"
>
> config LEDS_BLINKM
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 89c9b6f..5b96d15 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -67,6 +67,7 @@ obj-$(CONFIG_LEDS_KTD2692) += leds-ktd2692.o
> obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o
> obj-$(CONFIG_LEDS_SEAD3) += leds-sead3.o
> obj-$(CONFIG_LEDS_SN3218) += leds-sn3218.o
> +obj-$(CONFIG_LEDS_CHROMEOS_KEYBOARD) += leds-chromeos-keyboard.o
>
> # LED SPI Drivers
> obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
> diff --git a/drivers/leds/leds-chromeos-keyboard.c b/drivers/leds/leds-chromeos-keyboard.c
> new file mode 100644
> index 0000000..3592f09
> --- /dev/null
> +++ b/drivers/leds/leds-chromeos-keyboard.c
> @@ -0,0 +1,106 @@
> +/*
> + * Keyboard backlight LED driver for Chrome OS.
> + *
> + * Copyright (C) 2012 Google, Inc.
> + *
> + * 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/acpi.h>
> +#include <linux/leds.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +/* Keyboard LED ACPI Device must be defined in firmware */
> +#define ACPI_KEYBOARD_BACKLIGHT_DEVICE "\\_SB.KBLT"
> +#define ACPI_KEYBOARD_BACKLIGHT_READ ACPI_KEYBOARD_BACKLIGHT_DEVICE ".KBQC"
> +#define ACPI_KEYBOARD_BACKLIGHT_WRITE ACPI_KEYBOARD_BACKLIGHT_DEVICE ".KBCM"
> +
> +#define ACPI_KEYBOARD_BACKLIGHT_MAX 100
> +
> +static void keyboard_led_set_brightness(struct led_classdev *cdev,
> + enum led_brightness brightness)
> +{
> + union acpi_object param;
> + struct acpi_object_list input;
> + acpi_status status;
> +
> + param.type = ACPI_TYPE_INTEGER;
> + param.integer.value = brightness;
> + input.count = 1;
> + input.pointer = ¶m;
> +
> + status = acpi_evaluate_object(NULL, ACPI_KEYBOARD_BACKLIGHT_WRITE,
> + &input, NULL);
> + if (ACPI_FAILURE(status))
> + dev_err(cdev->dev, "Error setting keyboard LED value: %d\n",
> + status);
> +}
> +
> +static int keyboard_led_probe(struct platform_device *pdev)
> +{
> + struct led_classdev *cdev;
> + acpi_handle handle;
> + acpi_status status;
> + int error;
> +
> + /* Look for the keyboard LED ACPI Device */
> + status = acpi_get_handle(ACPI_ROOT_OBJECT,
> + ACPI_KEYBOARD_BACKLIGHT_DEVICE,
> + &handle);
> + if (ACPI_FAILURE(status)) {
> + dev_err(&pdev->dev, "Unable to find ACPI device %s: %d\n",
> + ACPI_KEYBOARD_BACKLIGHT_DEVICE, status);
> + return -ENXIO;
> + }
> +
> + cdev = devm_kzalloc(&pdev->dev, sizeof(*cdev), GFP_KERNEL);
> + if (!cdev)
> + return -ENOMEM;
> +
> + cdev->name = "chromeos::kbd_backlight";
> + cdev->brightness_set = keyboard_led_set_brightness;
> + cdev->max_brightness = ACPI_KEYBOARD_BACKLIGHT_MAX;
> + cdev->brightness = cdev->max_brightness;
> + cdev->flags |= LED_CORE_SUSPENDRESUME;
> +
> + error = devm_led_classdev_register(&pdev->dev, cdev);
> + if (error)
> + return error;
> +
> + platform_set_drvdata(pdev, cdev);
> + return 0;
> +}
> +
> +static const struct acpi_device_id keyboard_led_id[] = {
> + { "GOOG0002", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(acpi, keyboard_led_id);
> +
> +static struct platform_driver keyboard_led_driver = {
> + .driver = {
> + .name = "chromeos-keyboard-leds",
> + .acpi_match_table = ACPI_PTR(keyboard_led_id),
> + },
> + .probe = keyboard_led_probe,
> +};
> +module_platform_driver(keyboard_led_driver);
> +
> +MODULE_AUTHOR("Simon Que <sque@chromium.org>");
> +MODULE_DESCRIPTION("ChromeOS Keyboard backlight LED Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:chromeos-keyboard-leds");
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] leds: Add Chrome OS keyboard backlight LEDs driver
2016-03-04 8:38 ` Evan McClain
@ 2016-03-04 9:38 ` Jacek Anaszewski
2016-03-04 19:13 ` Dmitry Torokhov
0 siblings, 1 reply; 13+ messages in thread
From: Jacek Anaszewski @ 2016-03-04 9:38 UTC (permalink / raw)
To: Evan McClain
Cc: Dmitry Torokhov, Richard Purdie, Bryan Wu, Simon Que,
Olof Johansson, Duncan Laurie, Guenter Roeck, linux-kernel,
linux-leds
Hi Evan,
On 03/04/2016 09:38 AM, Evan McClain wrote:
> On Thu, 2016-03-03 at 15:46 -0800, Dmitry Torokhov wrote:
>> From: Simon Que <sque@chromium.org>
>>
>> This is a driver for ACPI-based keyboard backlight LEDs found on
>> Chromebooks. The driver locates \\_SB.KBLT ACPI device and exports
>> backlight as "chromeos::kbd_backlight" LED class device in sysfs.
>
> Was it ever decided where this driver should live? I was planning on
> submitting to platform/chrome since most keyboard backlights seem to
> live over there but I don't think I got a response.
>
It hasn't been decided yet. I can take it, but could you submit one more
version, without
'owner = THIS_MODULE' in struct platform_driver keyboard_led_driver ?
It is redundant, because the core will do it.
Also the line with devm_kzalloc has over 80 characters.
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] leds: Add Chrome OS keyboard backlight LEDs driver
2016-03-04 9:38 ` Jacek Anaszewski
@ 2016-03-04 19:13 ` Dmitry Torokhov
2016-03-04 20:41 ` Evan McClain
2016-03-04 20:55 ` Jacek Anaszewski
0 siblings, 2 replies; 13+ messages in thread
From: Dmitry Torokhov @ 2016-03-04 19:13 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Evan McClain, Richard Purdie, Bryan Wu, Simon Que,
Olof Johansson, Duncan Laurie, Guenter Roeck, linux-kernel,
linux-leds
On Fri, Mar 04, 2016 at 10:38:40AM +0100, Jacek Anaszewski wrote:
> Hi Evan,
>
> On 03/04/2016 09:38 AM, Evan McClain wrote:
> >On Thu, 2016-03-03 at 15:46 -0800, Dmitry Torokhov wrote:
> >>From: Simon Que <sque@chromium.org>
> >>
> >>This is a driver for ACPI-based keyboard backlight LEDs found on
> >>Chromebooks. The driver locates \\_SB.KBLT ACPI device and exports
> >>backlight as "chromeos::kbd_backlight" LED class device in sysfs.
> >
> >Was it ever decided where this driver should live? I was planning on
> >submitting to platform/chrome since most keyboard backlights seem to
> >live over there but I don't think I got a response.
> >
>
> It hasn't been decided yet. I can take it, but could you submit one more
> version, without
>
> 'owner = THIS_MODULE' in struct platform_driver keyboard_led_driver ?
>
> It is redundant, because the core will do it.
>
> Also the line with devm_kzalloc has over 80 characters.
Also:
- preferably use sizeof(*cdev) instead of sizeof(struct ...)
- do not check cdev->flags & LED_SUSPENDED in
keyboard_led_set_brightness() as it is not going to be called when led
device is suspended anyway
- change the MODULE_LICENSE from "GPL v2" to "GPL" as to match the
actual license notice
- report ACPI errors in error messages (since we clobber them)
- preferably use ENXIO instead of ENODEV
- maybe add "depends on CHROME_PLATFORMS || COMPILE_TEST" so that we do
not prompt for it on non-Chrome platforms
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] leds: Add Chrome OS keyboard backlight LEDs driver
2016-03-04 9:38 ` Jacek Anaszewski
@ 2016-03-04 19:19 ` Dmitry Torokhov
2016-03-04 19:22 ` Olof Johansson
0 siblings, 1 reply; 13+ messages in thread
From: Dmitry Torokhov @ 2016-03-04 19:19 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Richard Purdie, Bryan Wu, Simon Que, Olof Johansson,
Duncan Laurie, Guenter Roeck, linux-kernel, linux-leds
Hi Jacek,
On Fri, Mar 04, 2016 at 10:38:24AM +0100, Jacek Anaszewski wrote:
> Hi Dmitry,
>
> Thanks for the patch. There's already other pending version [1],
> still to be decided where it should live.
>
> [1] http://comments.gmane.org/gmane.linux.leds/4523
Yes, sorry, have not noticed it already being submitted while doing some
spring cleaning.
I mentioned in the other message what I think Evan needs to change in
his version to make it match this one.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] leds: Add Chrome OS keyboard backlight LEDs driver
2016-03-04 19:19 ` Dmitry Torokhov
@ 2016-03-04 19:22 ` Olof Johansson
0 siblings, 0 replies; 13+ messages in thread
From: Olof Johansson @ 2016-03-04 19:22 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jacek Anaszewski, Richard Purdie, Bryan Wu, Simon Que,
Duncan Laurie, Guenter Roeck, linux-kernel, linux-leds
2016-03-04 11:19 GMT-08:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
>
> Hi Jacek,
>
> On Fri, Mar 04, 2016 at 10:38:24AM +0100, Jacek Anaszewski wrote:
> > Hi Dmitry,
> >
> > Thanks for the patch. There's already other pending version [1],
> > still to be decided where it should live.
> >
> > [1] http://comments.gmane.org/gmane.linux.leds/4523
>
> Yes, sorry, have not noticed it already being submitted while doing some
> spring cleaning.
Part of the reason for this is that discoverability of the patch was
low -- it was only ever sent on linux-leds and no other list.
Cc:ing lkml is useful since most of us are subscribed to it and a
quick search for patches will find it. In addition to the subsystem
list of course, since subsystem maintainers don't want to have to
monitor all of LKML for patches to their code.
-Olof
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] leds: Add Chrome OS keyboard backlight LEDs driver
2016-03-04 19:13 ` Dmitry Torokhov
@ 2016-03-04 20:41 ` Evan McClain
2016-03-04 20:56 ` Dmitry Torokhov
2016-03-04 20:55 ` Jacek Anaszewski
1 sibling, 1 reply; 13+ messages in thread
From: Evan McClain @ 2016-03-04 20:41 UTC (permalink / raw)
To: Dmitry Torokhov, Jacek Anaszewski
Cc: Richard Purdie, Bryan Wu, Simon Que, Olof Johansson,
Duncan Laurie, Guenter Roeck, linux-kernel, linux-leds
[-- Attachment #1: Type: text/plain, Size: 2605 bytes --]
On Fri, 2016-03-04 at 11:13 -0800, Dmitry Torokhov wrote:
> On Fri, Mar 04, 2016 at 10:38:40AM +0100, Jacek Anaszewski wrote:
> >
> > Hi Evan,
> >
> > On 03/04/2016 09:38 AM, Evan McClain wrote:
> > >
> > > On Thu, 2016-03-03 at 15:46 -0800, Dmitry Torokhov wrote:
> > > >
> > > > From: Simon Que <sque@chromium.org>
> > > >
> > > > This is a driver for ACPI-based keyboard backlight LEDs found
> > > > on
> > > > Chromebooks. The driver locates \\_SB.KBLT ACPI device and
> > > > exports
> > > > backlight as "chromeos::kbd_backlight" LED class device in
> > > > sysfs.
> > > Was it ever decided where this driver should live? I was planning
> > > on
> > > submitting to platform/chrome since most keyboard backlights seem
> > > to
> > > live over there but I don't think I got a response.
> > >
> > It hasn't been decided yet. I can take it, but could you submit one
> > more
> > version, without
> >
> > 'owner = THIS_MODULE' in struct platform_driver
> > keyboard_led_driver ?
> >
> > It is redundant, because the core will do it.
> >
> > Also the line with devm_kzalloc has over 80 characters.
> Also:
>
> - preferably use sizeof(*cdev) instead of sizeof(struct ...)
> - do not check cdev->flags & LED_SUSPENDED in
> keyboard_led_set_brightness() as it is not going to be called when
> led
> device is suspended anyway
Your patch is definitely better, I was only taking Simon's original
submission and doing the minimal cleanup to help get it submitted (as
someone using mainline linux on a pixel 2/samus).
> - change the MODULE_LICENSE from "GPL v2" to "GPL" as to match the
> actual license notice
I think this got changed in one of the revisions in error.
> - report ACPI errors in error messages (since we clobber them)
> - preferably use ENXIO instead of ENODEV
Most other drivers seem to use ENODEV on probe, but I'm in the 'learn
through grep' level of understanding for parts of linux.
> - maybe add "depends on CHROME_PLATFORMS || COMPILE_TEST" so that we
> do
> not prompt for it on non-Chrome platforms
Adding depends on CHROME_PLATFORMS definitely makes sense, but also
might support putting this driver in platform/chrome. Either way I just
selfishly want better mainline support for this laptop.
The only change I made (other than the changes suggested by Jacek) was
to remove the line setting brightness to max_brightness on probe. I can
resubmit a cleaned up patch incorporating your changes unless you would
like to take over.
Thanks
--
Evan McClain
https://keybase.io/aeroevan
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] leds: Add Chrome OS keyboard backlight LEDs driver
2016-03-04 19:13 ` Dmitry Torokhov
2016-03-04 20:41 ` Evan McClain
@ 2016-03-04 20:55 ` Jacek Anaszewski
2016-03-04 20:59 ` Dmitry Torokhov
1 sibling, 1 reply; 13+ messages in thread
From: Jacek Anaszewski @ 2016-03-04 20:55 UTC (permalink / raw)
To: Dmitry Torokhov, Jacek Anaszewski
Cc: Evan McClain, Richard Purdie, Bryan Wu, Simon Que,
Olof Johansson, Duncan Laurie, Guenter Roeck, linux-kernel,
linux-leds
On 03/04/2016 08:13 PM, Dmitry Torokhov wrote:
> On Fri, Mar 04, 2016 at 10:38:40AM +0100, Jacek Anaszewski wrote:
>> Hi Evan,
>>
>> On 03/04/2016 09:38 AM, Evan McClain wrote:
>>> On Thu, 2016-03-03 at 15:46 -0800, Dmitry Torokhov wrote:
>>>> From: Simon Que <sque@chromium.org>
>>>>
>>>> This is a driver for ACPI-based keyboard backlight LEDs found on
>>>> Chromebooks. The driver locates \\_SB.KBLT ACPI device and exports
>>>> backlight as "chromeos::kbd_backlight" LED class device in sysfs.
>>>
>>> Was it ever decided where this driver should live? I was planning on
>>> submitting to platform/chrome since most keyboard backlights seem to
>>> live over there but I don't think I got a response.
>>>
>>
>> It hasn't been decided yet. I can take it, but could you submit one more
>> version, without
>>
>> 'owner = THIS_MODULE' in struct platform_driver keyboard_led_driver ?
>>
>> It is redundant, because the core will do it.
>>
>> Also the line with devm_kzalloc has over 80 characters.
>
> Also:
>
> - preferably use sizeof(*cdev) instead of sizeof(struct ...)
> - do not check cdev->flags & LED_SUSPENDED in
> keyboard_led_set_brightness() as it is not going to be called when led
> device is suspended anyway
> - change the MODULE_LICENSE from "GPL v2" to "GPL" as to match the
I can see "either version 2 of the License" in the license notice.
> actual license notice
> - report ACPI errors in error messages (since we clobber them)
> - preferably use ENXIO instead of ENODEV
> - maybe add "depends on CHROME_PLATFORMS || COMPILE_TEST" so that we do
> not prompt for it on non-Chrome platforms
I agree with the remaining items.
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] leds: Add Chrome OS keyboard backlight LEDs driver
2016-03-04 20:41 ` Evan McClain
@ 2016-03-04 20:56 ` Dmitry Torokhov
2016-03-04 22:09 ` Olof Johansson
0 siblings, 1 reply; 13+ messages in thread
From: Dmitry Torokhov @ 2016-03-04 20:56 UTC (permalink / raw)
To: Evan McClain
Cc: Jacek Anaszewski, Richard Purdie, Bryan Wu, Simon Que,
Olof Johansson, Duncan Laurie, Guenter Roeck, linux-kernel,
linux-leds
On Fri, Mar 04, 2016 at 03:41:36PM -0500, Evan McClain wrote:
> On Fri, 2016-03-04 at 11:13 -0800, Dmitry Torokhov wrote:
> > On Fri, Mar 04, 2016 at 10:38:40AM +0100, Jacek Anaszewski wrote:
> > >
> > > Hi Evan,
> > >
> > > On 03/04/2016 09:38 AM, Evan McClain wrote:
> > > >
> > > > On Thu, 2016-03-03 at 15:46 -0800, Dmitry Torokhov wrote:
> > > > >
> > > > > From: Simon Que <sque@chromium.org>
> > > > >
> > > > > This is a driver for ACPI-based keyboard backlight LEDs found
> > > > > on
> > > > > Chromebooks. The driver locates \\_SB.KBLT ACPI device and
> > > > > exports
> > > > > backlight as "chromeos::kbd_backlight" LED class device in
> > > > > sysfs.
> > > > Was it ever decided where this driver should live? I was planning
> > > > on
> > > > submitting to platform/chrome since most keyboard backlights seem
> > > > to
> > > > live over there but I don't think I got a response.
> > > >
> > > It hasn't been decided yet. I can take it, but could you submit one
> > > more
> > > version, without
> > >
> > > 'owner = THIS_MODULE' in struct platform_driver
> > > keyboard_led_driver ?
> > >
> > > It is redundant, because the core will do it.
> > >
> > > Also the line with devm_kzalloc has over 80 characters.
> > Also:
> >
> > - preferably use sizeof(*cdev) instead of sizeof(struct ...)
> > - do not check cdev->flags & LED_SUSPENDED in
> > keyboard_led_set_brightness() as it is not going to be called when
> > led
> > device is suspended anyway
>
> Your patch is definitely better, I was only taking Simon's original
> submission and doing the minimal cleanup to help get it submitted (as
> someone using mainline linux on a pixel 2/samus).
>
> > - change the MODULE_LICENSE from "GPL v2" to "GPL" as to match the
> > actual license notice
>
> I think this got changed in one of the revisions in error.
>
> > - report ACPI errors in error messages (since we clobber them)
> > - preferably use ENXIO instead of ENODEV
>
> Most other drivers seem to use ENODEV on probe, but I'm in the 'learn
> through grep' level of understanding for parts of linux.
>
> > - maybe add "depends on CHROME_PLATFORMS || COMPILE_TEST" so that we
> > do
> > not prompt for it on non-Chrome platforms
>
> Adding depends on CHROME_PLATFORMS definitely makes sense, but also
> might support putting this driver in platform/chrome. Either way I just
Olof, do you want it in platform/chrome?
> selfishly want better mainline support for this laptop.
>
> The only change I made (other than the changes suggested by Jacek) was
> to remove the line setting brightness to max_brightness on probe. I can
I wonder if we should actually read the current brightness before
registering the led device.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] leds: Add Chrome OS keyboard backlight LEDs driver
2016-03-04 20:55 ` Jacek Anaszewski
@ 2016-03-04 20:59 ` Dmitry Torokhov
2016-03-04 21:48 ` Jacek Anaszewski
0 siblings, 1 reply; 13+ messages in thread
From: Dmitry Torokhov @ 2016-03-04 20:59 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Jacek Anaszewski, Evan McClain, Richard Purdie, Bryan Wu,
Simon Que, Olof Johansson, Duncan Laurie, Guenter Roeck,
linux-kernel, linux-leds
On Fri, Mar 04, 2016 at 09:55:24PM +0100, Jacek Anaszewski wrote:
> On 03/04/2016 08:13 PM, Dmitry Torokhov wrote:
> >On Fri, Mar 04, 2016 at 10:38:40AM +0100, Jacek Anaszewski wrote:
> >>Hi Evan,
> >>
> >>On 03/04/2016 09:38 AM, Evan McClain wrote:
> >>>On Thu, 2016-03-03 at 15:46 -0800, Dmitry Torokhov wrote:
> >>>>From: Simon Que <sque@chromium.org>
> >>>>
> >>>>This is a driver for ACPI-based keyboard backlight LEDs found on
> >>>>Chromebooks. The driver locates \\_SB.KBLT ACPI device and exports
> >>>>backlight as "chromeos::kbd_backlight" LED class device in sysfs.
> >>>
> >>>Was it ever decided where this driver should live? I was planning on
> >>>submitting to platform/chrome since most keyboard backlights seem to
> >>>live over there but I don't think I got a response.
> >>>
> >>
> >>It hasn't been decided yet. I can take it, but could you submit one more
> >>version, without
> >>
> >>'owner = THIS_MODULE' in struct platform_driver keyboard_led_driver ?
> >>
> >>It is redundant, because the core will do it.
> >>
> >>Also the line with devm_kzalloc has over 80 characters.
> >
> >Also:
> >
> >- preferably use sizeof(*cdev) instead of sizeof(struct ...)
> >- do not check cdev->flags & LED_SUSPENDED in
> > keyboard_led_set_brightness() as it is not going to be called when led
> > device is suspended anyway
> >- change the MODULE_LICENSE from "GPL v2" to "GPL" as to match the
>
> I can see "either version 2 of the License" in the license notice.
>From module.h:
* "GPL" [GNU Public License v2 or later]
* "GPL v2" [GNU Public License v2]
leds-chromeos-keyboard is GPL v2+ so module license should be "GPL".
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] leds: Add Chrome OS keyboard backlight LEDs driver
2016-03-04 20:59 ` Dmitry Torokhov
@ 2016-03-04 21:48 ` Jacek Anaszewski
0 siblings, 0 replies; 13+ messages in thread
From: Jacek Anaszewski @ 2016-03-04 21:48 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jacek Anaszewski, Evan McClain, Richard Purdie, Bryan Wu,
Simon Que, Olof Johansson, Duncan Laurie, Guenter Roeck,
linux-kernel, linux-leds
On 03/04/2016 09:59 PM, Dmitry Torokhov wrote:
> On Fri, Mar 04, 2016 at 09:55:24PM +0100, Jacek Anaszewski wrote:
>> On 03/04/2016 08:13 PM, Dmitry Torokhov wrote:
>>> On Fri, Mar 04, 2016 at 10:38:40AM +0100, Jacek Anaszewski wrote:
>>>> Hi Evan,
>>>>
>>>> On 03/04/2016 09:38 AM, Evan McClain wrote:
>>>>> On Thu, 2016-03-03 at 15:46 -0800, Dmitry Torokhov wrote:
>>>>>> From: Simon Que <sque@chromium.org>
>>>>>>
>>>>>> This is a driver for ACPI-based keyboard backlight LEDs found on
>>>>>> Chromebooks. The driver locates \\_SB.KBLT ACPI device and exports
>>>>>> backlight as "chromeos::kbd_backlight" LED class device in sysfs.
>>>>>
>>>>> Was it ever decided where this driver should live? I was planning on
>>>>> submitting to platform/chrome since most keyboard backlights seem to
>>>>> live over there but I don't think I got a response.
>>>>>
>>>>
>>>> It hasn't been decided yet. I can take it, but could you submit one more
>>>> version, without
>>>>
>>>> 'owner = THIS_MODULE' in struct platform_driver keyboard_led_driver ?
>>>>
>>>> It is redundant, because the core will do it.
>>>>
>>>> Also the line with devm_kzalloc has over 80 characters.
>>>
>>> Also:
>>>
>>> - preferably use sizeof(*cdev) instead of sizeof(struct ...)
>>> - do not check cdev->flags & LED_SUSPENDED in
>>> keyboard_led_set_brightness() as it is not going to be called when led
>>> device is suspended anyway
>>> - change the MODULE_LICENSE from "GPL v2" to "GPL" as to match the
>>
>> I can see "either version 2 of the License" in the license notice.
>
>>From module.h:
>
> * "GPL" [GNU Public License v2 or later]
> * "GPL v2" [GNU Public License v2]
>
> leds-chromeos-keyboard is GPL v2+ so module license should be "GPL".
That's surprising. Thanks for spotting this.
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] leds: Add Chrome OS keyboard backlight LEDs driver
2016-03-04 20:56 ` Dmitry Torokhov
@ 2016-03-04 22:09 ` Olof Johansson
0 siblings, 0 replies; 13+ messages in thread
From: Olof Johansson @ 2016-03-04 22:09 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Evan McClain, Jacek Anaszewski, Richard Purdie, Bryan Wu,
Simon Que, Olof Johansson, Duncan Laurie, Guenter Roeck,
linux-kernel, Linux LED Subsystem
On Fri, Mar 4, 2016 at 12:56 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Mar 04, 2016 at 03:41:36PM -0500, Evan McClain wrote:
>> On Fri, 2016-03-04 at 11:13 -0800, Dmitry Torokhov wrote:
>> > On Fri, Mar 04, 2016 at 10:38:40AM +0100, Jacek Anaszewski wrote:
>> > >
>> > > Hi Evan,
>> > >
>> > > On 03/04/2016 09:38 AM, Evan McClain wrote:
>> > > >
>> > > > On Thu, 2016-03-03 at 15:46 -0800, Dmitry Torokhov wrote:
>> > > > >
>> > > > > From: Simon Que <sque@chromium.org>
>> > > > >
>> > > > > This is a driver for ACPI-based keyboard backlight LEDs found
>> > > > > on
>> > > > > Chromebooks. The driver locates \\_SB.KBLT ACPI device and
>> > > > > exports
>> > > > > backlight as "chromeos::kbd_backlight" LED class device in
>> > > > > sysfs.
>> > > > Was it ever decided where this driver should live? I was planning
>> > > > on
>> > > > submitting to platform/chrome since most keyboard backlights seem
>> > > > to
>> > > > live over there but I don't think I got a response.
>> > > >
>> > > It hasn't been decided yet. I can take it, but could you submit one
>> > > more
>> > > version, without
>> > >
>> > > 'owner = THIS_MODULE' in struct platform_driver
>> > > keyboard_led_driver ?
>> > >
>> > > It is redundant, because the core will do it.
>> > >
>> > > Also the line with devm_kzalloc has over 80 characters.
>> > Also:
>> >
>> > - preferably use sizeof(*cdev) instead of sizeof(struct ...)
>> > - do not check cdev->flags & LED_SUSPENDED in
>> > keyboard_led_set_brightness() as it is not going to be called when
>> > led
>> > device is suspended anyway
>>
>> Your patch is definitely better, I was only taking Simon's original
>> submission and doing the minimal cleanup to help get it submitted (as
>> someone using mainline linux on a pixel 2/samus).
>>
>> > - change the MODULE_LICENSE from "GPL v2" to "GPL" as to match the
>> > actual license notice
>>
>> I think this got changed in one of the revisions in error.
>>
>> > - report ACPI errors in error messages (since we clobber them)
>> > - preferably use ENXIO instead of ENODEV
>>
>> Most other drivers seem to use ENODEV on probe, but I'm in the 'learn
>> through grep' level of understanding for parts of linux.
>>
>> > - maybe add "depends on CHROME_PLATFORMS || COMPILE_TEST" so that we
>> > do
>> > not prompt for it on non-Chrome platforms
>>
>> Adding depends on CHROME_PLATFORMS definitely makes sense, but also
>> might support putting this driver in platform/chrome. Either way I just
>
> Olof, do you want it in platform/chrome?
Based on in-person discussion, I can take them through there, sure.
-Olof
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-03-04 22:09 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-03 23:46 [PATCH] leds: Add Chrome OS keyboard backlight LEDs driver Dmitry Torokhov
2016-03-04 8:38 ` Evan McClain
2016-03-04 9:38 ` Jacek Anaszewski
2016-03-04 19:13 ` Dmitry Torokhov
2016-03-04 20:41 ` Evan McClain
2016-03-04 20:56 ` Dmitry Torokhov
2016-03-04 22:09 ` Olof Johansson
2016-03-04 20:55 ` Jacek Anaszewski
2016-03-04 20:59 ` Dmitry Torokhov
2016-03-04 21:48 ` Jacek Anaszewski
2016-03-04 9:38 ` Jacek Anaszewski
2016-03-04 19:19 ` Dmitry Torokhov
2016-03-04 19:22 ` Olof Johansson
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.