diff mbox series

[v5,2/3] platform/chrome: Add Wilco EC keyboard backlight LEDs support

Message ID 20190404171007.160878-2-ncrews@chromium.org
State Not Applicable
Headers show
Series [v5,1/3] platform/chrome: wilco_ec: Standardize mailbox interface | expand

Commit Message

Nick Crews April 4, 2019, 5:10 p.m. UTC
The EC is in charge of controlling the keyboard backlight on
the Wilco platform. We expose a standard LED class device at
/sys/class/leds/platform::kbd_backlight. This driver is modeled
after the standard Chrome OS keyboard backlight driver at
drivers/platform/chrome/cros_kbd_led_backlight.c

Some Wilco devices do not support a keyboard backlight. This
is checked via wilco_ec_keyboard_leds_exist() in the core driver,
and a platform_device will only be registered by the core if
a backlight is supported.

After an EC reset the backlight could be in a non-PWM mode.
Earlier in the boot sequence the BIOS should send a command to
the EC to set the brightness, so things **should** be set up,
but we double check in probe() as we query the initial brightness.
If not set up, then set the brightness to 0.

Since the EC will never change the backlight level of its own accord,
we don't need to implement a brightness_get() method.

v5 changes:
-Rename the LED device to "platform::kbd_backlight", to
denote that this is the built-in system keyboard.

v4 changes:
-Call keyboard_led_set_brightness() directly within
 initialize_brightness(), instead of calling the library function.

v3 changes:
-Since this behaves the same as the standard Chrome OS keyboard
 backlight, rename the led device to "chromeos::kbd_backlight"
-Move wilco_ec_keyboard_backlight_exists() into core module, so
 that the core does not depend upon the keyboard backlight driver.
-This required moving some code into wilco-ec.h
-Refactor out some common code in set_brightness() and
 initialize_brightness()

v2 changes:
-Remove and fix uses of led vs LED in kconfig
-Assume BIOS initializes brightness, but double check in probe()
-Remove get_brightness() callback, as EC never changes brightness
 by itself.
-Use a __packed struct as message instead of opaque array
-Add exported wilco_ec_keyboard_leds_exist() so the core driver
 now only creates a platform _device if relevant
-Fix use of keyboard_led_set_brightness() since it can sleep

Signed-off-by: Nick Crews <ncrews@chromium.org>
Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
---
 drivers/platform/chrome/wilco_ec/Kconfig      |   9 +
 drivers/platform/chrome/wilco_ec/Makefile     |   2 +
 drivers/platform/chrome/wilco_ec/core.c       |  58 ++++++
 .../chrome/wilco_ec/kbd_led_backlight.c       | 168 ++++++++++++++++++
 include/linux/platform_data/wilco-ec.h        |  38 ++++
 5 files changed, 275 insertions(+)
 create mode 100644 drivers/platform/chrome/wilco_ec/kbd_led_backlight.c

Comments

Pavel Machek April 4, 2019, 7:52 p.m. UTC | #1
On Thu 2019-04-04 11:10:08, Nick Crews wrote:
> The EC is in charge of controlling the keyboard backlight on
> the Wilco platform. We expose a standard LED class device at
> /sys/class/leds/platform::kbd_backlight. This driver is modeled
> after the standard Chrome OS keyboard backlight driver at
> drivers/platform/chrome/cros_kbd_led_backlight.c
> 
> Some Wilco devices do not support a keyboard backlight. This
> is checked via wilco_ec_keyboard_leds_exist() in the core driver,
> and a platform_device will only be registered by the core if
> a backlight is supported.
> 
> After an EC reset the backlight could be in a non-PWM mode.
> Earlier in the boot sequence the BIOS should send a command to
> the EC to set the brightness, so things **should** be set up,
> but we double check in probe() as we query the initial brightness.
> If not set up, then set the brightness to 0.
> 
> Since the EC will never change the backlight level of its own accord,
> we don't need to implement a brightness_get() method.
...
> Signed-off-by: Nick Crews <ncrews@chromium.org>
> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>

Acked-by: Pavel Machek <pavel@ucw.cz>
Guenter Roeck April 5, 2019, 8:15 p.m. UTC | #2
On Thu, Apr 04, 2019 at 11:10:08AM -0600, Nick Crews wrote:
> The EC is in charge of controlling the keyboard backlight on
> the Wilco platform. We expose a standard LED class device at
> /sys/class/leds/platform::kbd_backlight. This driver is modeled
> after the standard Chrome OS keyboard backlight driver at
> drivers/platform/chrome/cros_kbd_led_backlight.c
> 
> Some Wilco devices do not support a keyboard backlight. This
> is checked via wilco_ec_keyboard_leds_exist() in the core driver,
> and a platform_device will only be registered by the core if
> a backlight is supported.
> 
> After an EC reset the backlight could be in a non-PWM mode.
> Earlier in the boot sequence the BIOS should send a command to
> the EC to set the brightness, so things **should** be set up,
> but we double check in probe() as we query the initial brightness.
> If not set up, then set the brightness to 0.
> 
> Since the EC will never change the backlight level of its own accord,
> we don't need to implement a brightness_get() method.
> 
> v5 changes:
> -Rename the LED device to "platform::kbd_backlight", to
> denote that this is the built-in system keyboard.
> 

NACK.

Per Documentation/leds/leds-class.txt, LED devices are named
	"devicename:colour:function"

This document also states "The naming scheme above leaves scope
for further attributes should they be needed". It does not permit,
however, to redefine one of the fields to mean "location", much less
the declaration that a devicename of "platform" shall refer to an
"internal" backlight, or that there shall be no more than one
"internal" backlight in a given system.

On top of that, with this naming scheme any system with more than
one internal backlight may result in duplicate sysfs attribute names.

If an attribute for "location" is desired or necessary as part of
the LED attribute name, such a change should be well defined, and
it should be documented, and it must not create the risk of
duplicate sysfs attribute names.

Thanks,
Guenter
Pavel Machek April 6, 2019, 8:41 a.m. UTC | #3
On Fri 2019-04-05 13:15:34, Guenter Roeck wrote:
> On Thu, Apr 04, 2019 at 11:10:08AM -0600, Nick Crews wrote:
> > The EC is in charge of controlling the keyboard backlight on
> > the Wilco platform. We expose a standard LED class device at
> > /sys/class/leds/platform::kbd_backlight. This driver is modeled
> > after the standard Chrome OS keyboard backlight driver at
> > drivers/platform/chrome/cros_kbd_led_backlight.c
> > 
> > Some Wilco devices do not support a keyboard backlight. This
> > is checked via wilco_ec_keyboard_leds_exist() in the core driver,
> > and a platform_device will only be registered by the core if
> > a backlight is supported.
> > 
> > After an EC reset the backlight could be in a non-PWM mode.
> > Earlier in the boot sequence the BIOS should send a command to
> > the EC to set the brightness, so things **should** be set up,
> > but we double check in probe() as we query the initial brightness.
> > If not set up, then set the brightness to 0.
> > 
> > Since the EC will never change the backlight level of its own accord,
> > we don't need to implement a brightness_get() method.
> > 
> > v5 changes:
> > -Rename the LED device to "platform::kbd_backlight", to
> > denote that this is the built-in system keyboard.
> > 
> 
> NACK.

Please keep it as it is, it is okay.

> Per Documentation/leds/leds-class.txt, LED devices are named
> 	"devicename:colour:function"

You failed to follow threads explaining this is being changed, even
when I pointed you at them. What you are doing here is not helpful.

> This document also states "The naming scheme above leaves scope
> for further attributes should they be needed". It does not permit,
> however, to redefine one of the fields to mean "location", much less
> the declaration that a devicename of "platform" shall refer to an
> "internal" backlight, or that there shall be no more than one
> "internal" backlight in a given system.

"platform" is as good devicename as "wilco" or "chromeos".
									Pavel
Dmitry Torokhov April 7, 2019, 9:46 p.m. UTC | #4
On Sat, Apr 6, 2019 at 1:41 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> On Fri 2019-04-05 13:15:34, Guenter Roeck wrote:
> > On Thu, Apr 04, 2019 at 11:10:08AM -0600, Nick Crews wrote:
> > > The EC is in charge of controlling the keyboard backlight on
> > > the Wilco platform. We expose a standard LED class device at
> > > /sys/class/leds/platform::kbd_backlight. This driver is modeled
> > > after the standard Chrome OS keyboard backlight driver at
> > > drivers/platform/chrome/cros_kbd_led_backlight.c
> > >
> > > Some Wilco devices do not support a keyboard backlight. This
> > > is checked via wilco_ec_keyboard_leds_exist() in the core driver,
> > > and a platform_device will only be registered by the core if
> > > a backlight is supported.
> > >
> > > After an EC reset the backlight could be in a non-PWM mode.
> > > Earlier in the boot sequence the BIOS should send a command to
> > > the EC to set the brightness, so things **should** be set up,
> > > but we double check in probe() as we query the initial brightness.
> > > If not set up, then set the brightness to 0.
> > >
> > > Since the EC will never change the backlight level of its own accord,
> > > we don't need to implement a brightness_get() method.
> > >
> > > v5 changes:
> > > -Rename the LED device to "platform::kbd_backlight", to
> > > denote that this is the built-in system keyboard.
> > >
> >
> > NACK.
>
> Please keep it as it is, it is okay.
>
> > Per Documentation/leds/leds-class.txt, LED devices are named
> >       "devicename:colour:function"
>
> You failed to follow threads explaining this is being changed, even
> when I pointed you at them. What you are doing here is not helpful.

Pavel, what I find is unhelpful is you requiring to conform to the new
rules that have not been accepted yet and for which there clearly are
objections. You keep ignoring all the issues that we continue to point
out with your proposed scheme.

I will go and try to reply to Jacek's thread, but it is my firm belief
that changing naming scheme is not what we need to do here.

>
> > This document also states "The naming scheme above leaves scope
> > for further attributes should they be needed". It does not permit,
> > however, to redefine one of the fields to mean "location", much less
> > the declaration that a devicename of "platform" shall refer to an
> > "internal" backlight, or that there shall be no more than one
> > "internal" backlight in a given system.
>
> "platform" is as good devicename as "wilco" or "chromeos".

No, because "platform" is not a device, it is something that you are
trying to assign a magic meaning to.

Thanks.
Pavel Machek April 7, 2019, 10:18 p.m. UTC | #5
> > > This document also states "The naming scheme above leaves scope
> > > for further attributes should they be needed". It does not permit,
> > > however, to redefine one of the fields to mean "location", much less
> > > the declaration that a devicename of "platform" shall refer to an
> > > "internal" backlight, or that there shall be no more than one
> > > "internal" backlight in a given system.
> >
> > "platform" is as good devicename as "wilco" or "chromeos".
> 
> No, because "platform" is not a device, it is something that you are
> trying to assign a magic meaning to.

"chromeos" is not a device, either.
									Pavel
Dmitry Torokhov April 7, 2019, 10:26 p.m. UTC | #6
On Sun, Apr 7, 2019 at 3:18 PM Pavel Machek <pavel@ucw.cz> wrote:
>
>
> > > > This document also states "The naming scheme above leaves scope
> > > > for further attributes should they be needed". It does not permit,
> > > > however, to redefine one of the fields to mean "location", much less
> > > > the declaration that a devicename of "platform" shall refer to an
> > > > "internal" backlight, or that there shall be no more than one
> > > > "internal" backlight in a given system.
> > >
> > > "platform" is as good devicename as "wilco" or "chromeos".
> >
> > No, because "platform" is not a device, it is something that you are
> > trying to assign a magic meaning to.
>
> "chromeos" is not a device, either.

I agree, it is not a device name. We do not assign any specific
meaning to it though. We could change it to "cros_ec" if so desired
and nothing should break.

Thanks.
Pavel Machek April 8, 2019, 9:41 a.m. UTC | #7
Hi!

> > > > > This document also states "The naming scheme above leaves scope
> > > > > for further attributes should they be needed". It does not permit,
> > > > > however, to redefine one of the fields to mean "location", much less
> > > > > the declaration that a devicename of "platform" shall refer to an
> > > > > "internal" backlight, or that there shall be no more than one
> > > > > "internal" backlight in a given system.
> > > >
> > > > "platform" is as good devicename as "wilco" or "chromeos".
> > >
> > > No, because "platform" is not a device, it is something that you are
> > > trying to assign a magic meaning to.
> >
> > "chromeos" is not a device, either.
> 
> I agree, it is not a device name. We do not assign any specific
> meaning to it though. We could change it to "cros_ec" if so desired
> and nothing should break.

Yes. And you can also change it to "platform" and nothing will break
:-). Can we end the discussion here?

If not, lets take a look at existing names:

./drivers/platform/x86/asus-laptop.c:		cdev->name = "asus::kbd_backlight";
./drivers/platform/x86/samsung-laptop.c:		samsung->kbd_led.name = "samsung::kbd_backlight";
./drivers/platform/x86/thinkpad_acpi.c:		.name		= "tpacpi::kbd_backlight",
./drivers/platform/x86/toshiba_acpi.c:		dev->kbd_led.name = "toshiba::kbd_backlight";
./drivers/platform/x86/asus-wmi.c:		asus->kbd_led.name = "asus::kbd_backlight";
./drivers/platform/x86/dell-laptop.c:	.name           = "dell::kbd_backlight",
./drivers/platform/chrome/cros_kbd_led_backlight.c:	cdev->name = "chromeos::kbd_backlight";
./drivers/hwmon/applesmc.c:	.name			= "smc::kbd_backlight",
./drivers/hid/hid-asus.c:	drvdata->kbd_backlight->cdev.name = "asus::kbd_backlight";
./drivers/hid/hid-google-hammer.c:	kbd_backlight->cdev.name = "hammer::kbd_backlight";
./drivers/input/misc/ims-pcu.c:		 "pcu%d::kbd_backlight", pcu->device_no);

asus, samsung, toshiba, asus, dell, chromeos... Those are really not
device names, either. But in these cases, LED is probably controlled
by EC, or by ACPI BIOS talking to the EC. People usually call such
devices "platform devices".

(Linux Platform Device Driver - CodeProject
https://www.codeproject.com/tips/1080177/linux-platform-device-driver
A platform device is one which is hardwired on the board and hence not
hot-pluggable. )

You can take a look at discussion around micmute LED.

Thus "platform" is quite suitable name in your case, and incidentaly,
it will be more useful for userspace than "cros_ec".

								Pavel
Guenter Roeck April 8, 2019, 1:31 p.m. UTC | #8
On Mon, Apr 8, 2019 at 2:41 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > > > > > This document also states "The naming scheme above leaves scope
> > > > > > for further attributes should they be needed". It does not permit,
> > > > > > however, to redefine one of the fields to mean "location", much less
> > > > > > the declaration that a devicename of "platform" shall refer to an
> > > > > > "internal" backlight, or that there shall be no more than one
> > > > > > "internal" backlight in a given system.
> > > > >
> > > > > "platform" is as good devicename as "wilco" or "chromeos".
> > > >
> > > > No, because "platform" is not a device, it is something that you are
> > > > trying to assign a magic meaning to.
> > >
> > > "chromeos" is not a device, either.
> >
> > I agree, it is not a device name. We do not assign any specific
> > meaning to it though. We could change it to "cros_ec" if so desired
> > and nothing should break.
>
> Yes. And you can also change it to "platform" and nothing will break
> :-). Can we end the discussion here?
>
> If not, lets take a look at existing names:
>
> ./drivers/platform/x86/asus-laptop.c:           cdev->name = "asus::kbd_backlight";
> ./drivers/platform/x86/samsung-laptop.c:                samsung->kbd_led.name = "samsung::kbd_backlight";
> ./drivers/platform/x86/thinkpad_acpi.c:         .name           = "tpacpi::kbd_backlight",
> ./drivers/platform/x86/toshiba_acpi.c:          dev->kbd_led.name = "toshiba::kbd_backlight";
> ./drivers/platform/x86/asus-wmi.c:              asus->kbd_led.name = "asus::kbd_backlight";
> ./drivers/platform/x86/dell-laptop.c:   .name           = "dell::kbd_backlight",
> ./drivers/platform/chrome/cros_kbd_led_backlight.c:     cdev->name = "chromeos::kbd_backlight";
> ./drivers/hwmon/applesmc.c:     .name                   = "smc::kbd_backlight",
> ./drivers/hid/hid-asus.c:       drvdata->kbd_backlight->cdev.name = "asus::kbd_backlight";
> ./drivers/hid/hid-google-hammer.c:      kbd_backlight->cdev.name = "hammer::kbd_backlight";
> ./drivers/input/misc/ims-pcu.c:          "pcu%d::kbd_backlight", pcu->device_no);
>
> asus, samsung, toshiba, asus, dell, chromeos... Those are really not
> device names, either. But in these cases, LED is probably controlled
> by EC, or by ACPI BIOS talking to the EC. People usually call such
> devices "platform devices".
>
> (Linux Platform Device Driver - CodeProject
> https://www.codeproject.com/tips/1080177/linux-platform-device-driver
> A platform device is one which is hardwired on the board and hence not
> hot-pluggable. )
>
> You can take a look at discussion around micmute LED.
>
> Thus "platform" is quite suitable name in your case, and incidentaly,
> it will be more useful for userspace than "cros_ec".
>
No, it isn't. All those name are at least roughly associated with the
driver name, and they all do reflect the abbreviated driver name.
"platform" isn't, and is prone to duplicates.

If you want the location reflected in the led sysfs atttribute name,
say so. "platform" is neither a driver name nor a location. On a side
note, appending a number to the attribute name is not a good idea. In
addition to not being backward compatible, it would be prone to
renumbering at each reboot.

Guenter

>                                                                 Pavel
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
diff mbox series

Patch

diff --git a/drivers/platform/chrome/wilco_ec/Kconfig b/drivers/platform/chrome/wilco_ec/Kconfig
index e09e4cebe9b4..d8cf4a60d1b5 100644
--- a/drivers/platform/chrome/wilco_ec/Kconfig
+++ b/drivers/platform/chrome/wilco_ec/Kconfig
@@ -18,3 +18,12 @@  config WILCO_EC_DEBUGFS
 	  manipulation and allow for testing arbitrary commands.  This
 	  interface is intended for debug only and will not be present
 	  on production devices.
+
+config WILCO_EC_KBD_BACKLIGHT
+	tristate "Enable keyboard backlight control"
+	depends on WILCO_EC
+	help
+	  If you say Y here, you get support to set the keyboard backlight
+	  brightness. This happens via a standard LED driver that uses the
+	  Wilco EC mailbox interface. A standard LED class device will
+	  appear under /sys/class/leds/chromeos::kbd_backlight
diff --git a/drivers/platform/chrome/wilco_ec/Makefile b/drivers/platform/chrome/wilco_ec/Makefile
index 063e7fb4ea17..8436539813cd 100644
--- a/drivers/platform/chrome/wilco_ec/Makefile
+++ b/drivers/platform/chrome/wilco_ec/Makefile
@@ -4,3 +4,5 @@  wilco_ec-objs				:= core.o mailbox.o
 obj-$(CONFIG_WILCO_EC)			+= wilco_ec.o
 wilco_ec_debugfs-objs			:= debugfs.o
 obj-$(CONFIG_WILCO_EC_DEBUGFS)		+= wilco_ec_debugfs.o
+wilco_kbd_backlight-objs		:= kbd_led_backlight.o
+obj-$(CONFIG_WILCO_EC_KBD_BACKLIGHT)	+= wilco_kbd_backlight.o
diff --git a/drivers/platform/chrome/wilco_ec/core.c b/drivers/platform/chrome/wilco_ec/core.c
index 05e1e2be1c91..3c45c157b7da 100644
--- a/drivers/platform/chrome/wilco_ec/core.c
+++ b/drivers/platform/chrome/wilco_ec/core.c
@@ -38,11 +38,47 @@  static struct resource *wilco_get_resource(struct platform_device *pdev,
 				   dev_name(dev));
 }
 
+/**
+ * wilco_ec_keyboard_backlight_exists() - Is the keyboad backlight supported?
+ * @ec: EC device to query.
+ * @exists: Return value to fill in.
+ *
+ * Return: 0 on success, negative error code on failure.
+ */
+static int wilco_ec_keyboard_backlight_exists(struct wilco_ec_device *ec,
+					      bool *exists)
+{
+	struct wilco_ec_kbbl_msg request;
+	struct wilco_ec_kbbl_msg response;
+	struct wilco_ec_message msg;
+	int ret;
+
+	memset(&request, 0, sizeof(request));
+	request.command = WILCO_EC_COMMAND_KBBL;
+	request.subcmd = WILCO_KBBL_SUBCMD_GET_FEATURES;
+
+	memset(&msg, 0, sizeof(msg));
+	msg.type = WILCO_EC_MSG_LEGACY;
+	msg.request_data = &request;
+	msg.request_size = sizeof(request);
+	msg.response_data = &response;
+	msg.response_size = sizeof(response);
+
+	ret = wilco_ec_mailbox(ec, &msg);
+	if (ret < 0)
+		return ret;
+
+	*exists = response.status != 0xFF;
+
+	return 0;
+}
+
 static int wilco_ec_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct wilco_ec_device *ec;
 	int ret;
+	bool kbbl_exists;
 
 	ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
 	if (!ec)
@@ -89,8 +125,29 @@  static int wilco_ec_probe(struct platform_device *pdev)
 		goto unregister_debugfs;
 	}
 
+	/* Register child dev to be found by the keyboard backlight driver. */
+	ret = wilco_ec_keyboard_backlight_exists(ec, &kbbl_exists);
+	if (ret) {
+		dev_err(ec->dev,
+			"Failed checking keyboard backlight support: %d", ret);
+		goto unregister_rtc;
+	}
+	if (kbbl_exists) {
+		ec->kbbl_pdev = platform_device_register_data(dev,
+						"wilco-kbd-backlight",
+						PLATFORM_DEVID_AUTO, NULL, 0);
+		if (IS_ERR(ec->kbbl_pdev)) {
+			dev_err(dev,
+				"Failed to create keyboard backlight pdev\n");
+			ret = PTR_ERR(ec->kbbl_pdev);
+			goto unregister_rtc;
+		}
+	}
+
 	return 0;
 
+unregister_rtc:
+	platform_device_unregister(ec->rtc_pdev);
 unregister_debugfs:
 	if (ec->debugfs_pdev)
 		platform_device_unregister(ec->debugfs_pdev);
@@ -102,6 +159,7 @@  static int wilco_ec_remove(struct platform_device *pdev)
 {
 	struct wilco_ec_device *ec = platform_get_drvdata(pdev);
 
+	platform_device_unregister(ec->kbbl_pdev);
 	platform_device_unregister(ec->rtc_pdev);
 	if (ec->debugfs_pdev)
 		platform_device_unregister(ec->debugfs_pdev);
diff --git a/drivers/platform/chrome/wilco_ec/kbd_led_backlight.c b/drivers/platform/chrome/wilco_ec/kbd_led_backlight.c
new file mode 100644
index 000000000000..8fa4b10b6392
--- /dev/null
+++ b/drivers/platform/chrome/wilco_ec/kbd_led_backlight.c
@@ -0,0 +1,168 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Keyboard backlight LED driver for the Wilco Embedded Controller
+ *
+ * Copyright 2019 Google LLC
+ *
+ * The EC is in charge of controlling the keyboard backlight on
+ * the Wilco platform. We expose a standard LED class device at
+ * /sys/class/leds/platform::kbd_backlight. Power Manager normally
+ * controls the backlight by writing a percentage in range [0, 100]
+ * to the brightness property. This driver is modeled after the
+ * standard Chrome OS keyboard backlight driver at
+ * drivers/platform/chrome/cros_kbd_led_backlight.c
+ *
+ * Some Wilco devices do not support a keyboard backlight. This
+ * is checked via wilco_ec_keyboard_backlight_exists() in the core driver,
+ * and a platform_device will only be registered by the core if
+ * a backlight is supported.
+ *
+ * After an EC reset the backlight could be in a non-PWM mode.
+ * Earlier in the boot sequence the BIOS should send a command to
+ * the EC to set the brightness, so things **should** be set up,
+ * but we double check in probe() as we query the initial brightness.
+ * If not set up, then we set the brightness to KBBL_DEFAULT_BRIGHTNESS.
+ *
+ * Since the EC will never change the backlight level of its own accord,
+ * we don't need to implement a brightness_get() method.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/platform_data/wilco-ec.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define DRV_NAME		"wilco-kbd-backlight"
+
+#define KBBL_DEFAULT_BRIGHTNESS	0
+
+struct wilco_keyboard_led_data {
+	struct wilco_ec_device *ec;
+	struct led_classdev keyboard;
+};
+
+/* Send a request, get a response, and check that the response is good. */
+static int send_kbbl_msg(struct wilco_ec_device *ec,
+			 const struct wilco_ec_kbbl_msg *request,
+			 struct wilco_ec_kbbl_msg *response)
+{
+	struct wilco_ec_message msg;
+	int ret;
+
+	memset(&msg, 0, sizeof(msg));
+	msg.type = WILCO_EC_MSG_LEGACY;
+	msg.request_data = &request;
+	msg.request_size = sizeof(request);
+	msg.response_data = &response;
+	msg.response_size = sizeof(response);
+
+	ret = wilco_ec_mailbox(ec, &msg);
+	if (ret < 0)
+		dev_err(ec->dev, "Failed sending brightness command: %d", ret);
+
+	if (response->status) {
+		dev_err(ec->dev,
+			"EC reported failure sending brightness command: %d",
+			response->status);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+/* This may sleep because it uses wilco_ec_mailbox() */
+static int keyboard_led_set_brightness(struct led_classdev *cdev,
+				       enum led_brightness brightness)
+{
+	struct wilco_ec_kbbl_msg request;
+	struct wilco_ec_kbbl_msg response;
+	struct wilco_keyboard_led_data *data;
+
+	memset(&request, 0, sizeof(request));
+	request.command = WILCO_EC_COMMAND_KBBL;
+	request.subcmd = WILCO_KBBL_SUBCMD_GET_STATE;
+	request.mode = WILCO_KBBL_MODE_FLAG_PWM;
+	request.percent = brightness;
+
+	data = container_of(cdev, struct wilco_keyboard_led_data, keyboard);
+	return send_kbbl_msg(data->ec, &request, &response);
+}
+
+/*
+ * Get the current brightness, ensuring that we are in PWM mode. If not
+ * in PWM mode, then the current brightness is meaningless, so set the
+ * brightness to KBBL_DEFAULT_BRIGHTNESS.
+ *
+ * Return: Final brightness of the keyboard, or negative error code on failure.
+ */
+static int initialize_brightness(struct wilco_keyboard_led_data *data)
+{
+	struct wilco_ec_kbbl_msg request;
+	struct wilco_ec_kbbl_msg response;
+	int ret;
+
+	memset(&request, 0, sizeof(request));
+	request.command = WILCO_EC_COMMAND_KBBL;
+	request.subcmd = WILCO_KBBL_SUBCMD_GET_STATE;
+
+	ret = send_kbbl_msg(data->ec, &request, &response);
+	if (ret < 0)
+		return ret;
+
+	if (response.mode & WILCO_KBBL_MODE_FLAG_PWM)
+		return response.percent;
+
+	dev_warn(data->ec->dev, "Keyboard brightness not initialized by BIOS");
+	ret = keyboard_led_set_brightness(&data->keyboard,
+					  KBBL_DEFAULT_BRIGHTNESS);
+	if (ret < 0)
+		return ret;
+
+	return KBBL_DEFAULT_BRIGHTNESS;
+}
+
+static int keyboard_led_probe(struct platform_device *pdev)
+{
+	struct wilco_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
+	struct wilco_keyboard_led_data *data;
+	int ret;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->ec = ec;
+	/* This acts the same as the CrOS backlight, so use the same name */
+	data->keyboard.name = "platform::kbd_backlight";
+	data->keyboard.max_brightness = 100;
+	data->keyboard.flags = LED_CORE_SUSPENDRESUME;
+	data->keyboard.brightness_set_blocking = keyboard_led_set_brightness;
+	ret = initialize_brightness(data);
+	if (ret < 0)
+		return ret;
+	data->keyboard.brightness = ret;
+
+	ret = devm_led_classdev_register(&pdev->dev, &data->keyboard);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static struct platform_driver keyboard_led_driver = {
+	.driver = {
+		.name = DRV_NAME,
+	},
+	.probe = keyboard_led_probe,
+};
+module_platform_driver(keyboard_led_driver);
+
+MODULE_AUTHOR("Nick Crews <ncrews@chromium.org>");
+MODULE_DESCRIPTION("Wilco keyboard backlight LED driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" DRV_NAME);
diff --git a/include/linux/platform_data/wilco-ec.h b/include/linux/platform_data/wilco-ec.h
index 1ff224793c99..c3965b7f397d 100644
--- a/include/linux/platform_data/wilco-ec.h
+++ b/include/linux/platform_data/wilco-ec.h
@@ -32,6 +32,7 @@ 
  * @data_size: Size of the data buffer used for EC communication.
  * @debugfs_pdev: The child platform_device used by the debugfs sub-driver.
  * @rtc_pdev: The child platform_device used by the RTC sub-driver.
+ * @kbbl_pdev: The child pdev used by the keyboard backlight sub-driver.
  */
 struct wilco_ec_device {
 	struct device *dev;
@@ -43,6 +44,7 @@  struct wilco_ec_device {
 	size_t data_size;
 	struct platform_device *debugfs_pdev;
 	struct platform_device *rtc_pdev;
+	struct platform_device *kbbl_pdev;
 };
 
 /**
@@ -114,6 +116,42 @@  struct wilco_ec_message {
 	void *response_data;
 };
 
+/* Constants and structs useful for keyboard backlight (KBBL) control */
+
+#define WILCO_EC_COMMAND_KBBL		0x75
+#define WILCO_KBBL_MODE_FLAG_PWM	BIT(1)	/* Set brightness by percent. */
+
+/**
+ * enum kbbl_subcommand - What action does the EC perform?
+ * @WILCO_KBBL_SUBCMD_GET_FEATURES: Request available functionality from EC.
+ * @WILCO_KBBL_SUBCMD_GET_STATE: Request current mode and brightness from EC.
+ * @WILCO_KBBL_SUBCMD_SET_STATE: Write mode and brightness to EC.
+ */
+enum kbbl_subcommand {
+	WILCO_KBBL_SUBCMD_GET_FEATURES = 0x00,
+	WILCO_KBBL_SUBCMD_GET_STATE = 0x01,
+	WILCO_KBBL_SUBCMD_SET_STATE = 0x02,
+};
+
+/**
+ * struct wilco_ec_kbbl_msg - Message to/from EC for keyboard backlight control.
+ * @command: Always WILCO_EC_COMMAND_KBBL.
+ * @status: Set by EC to 0 on success, 0xFF on failure.
+ * @subcmd: One of enum kbbl_subcommand.
+ * @mode: Bit flags for used mode, we want to use WILCO_KBBL_MODE_FLAG_PWM.
+ * @percent: Brightness in 0-100. Only meaningful in PWM mode.
+ */
+struct wilco_ec_kbbl_msg {
+	u8 command;
+	u8 status;
+	u8 subcmd;
+	u8 reserved3;
+	u8 mode;
+	u8 reserved5to8[4];
+	u8 percent;
+	u8 reserved10to15[6];
+} __packed;
+
 /**
  * wilco_ec_mailbox() - Send request to the EC and receive the response.
  * @ec: Wilco EC device.