All of lore.kernel.org
 help / color / mirror / Atom feed
* [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 = &param;
+
+	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 = &param;
> +
> +	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.