From: Simon Que <[email protected]>
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 <[email protected]>
Signed-off-by: Duncan Laurie <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
---
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 <[email protected]>");
+MODULE_DESCRIPTION("ChromeOS Keyboard backlight LED Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:chromeos-keyboard-leds");
--
2.7.0.rc3.207.g0ac5344
--
Dmitry
On Thu, 2016-03-03 at 15:46 -0800, Dmitry Torokhov wrote:
> From: Simon Que <[email protected]>
>
> 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
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 <[email protected]>
>>
>> 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
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 <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Duncan Laurie <[email protected]>
> Signed-off-by: Dmitry Torokhov <[email protected]>
> ---
> 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 <[email protected]>");
> +MODULE_DESCRIPTION("ChromeOS Keyboard backlight LED Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:chromeos-keyboard-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 <[email protected]>
> >>
> >>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
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
2016-03-04 11:19 GMT-08:00 Dmitry Torokhov <[email protected]>:
>
> 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
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 <[email protected]>
> > > >
> > > > 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
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 <[email protected]>
>>>>
>>>> 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
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 <[email protected]>
> > > > >
> > > > > 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
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 <[email protected]>
> >>>>
> >>>>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
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 <[email protected]>
>>>>>>
>>>>>> 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
On Fri, Mar 4, 2016 at 12:56 PM, Dmitry Torokhov
<[email protected]> 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 <[email protected]>
>> > > > >
>> > > > > 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