INPUT: Route keyboard LEDs through the generic LEDs layer

This permits to reassign input LEDs to something else than keyboard "leds" state, by adding a trigger and a led for each input leds, the former being triggered by EV_LED events, and the latter being by default triggered by the former. The user can then make the LED use another trigger, including other LED triggers of the same keyboard.

The hardware LEDs are now not actioned from the ED_LED event any more, but from the per-device LED layer.

[ebroder at mokafive.com: Rebased to 3.2-rc1 or so, cleaned up some includes, and fixed some constants]

[blogic at openwrt.org: CONFIG_INPUT_LEDS stubs should be static inline]

[akpm at linux-foundation.org: remove unneeded `extern', fix comment layout]

Signed-off-by Samuel Thibault <[email protected]>

Signed-off-by Evan Broder <[email protected]>

Acked-by Peter Korsgaard <[email protected]>

Signed-off-by John Crispin <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---
Changed in this version:
- separate kbd LED changes from input LED changes
- add a per-device trigger, triggered by EV_LED events
- make the per-device LED triggered by default by the per-device trigger, so that evdev users get the modified behavior too
- make the hardware driven by the LED instead of EV_LED events

drivers/input/input.c +1 -0

@@ -27,6 +27,7 @@
 #include <linux/device.h>
 #include <linux/mutex.h>
 #include <linux/rcupdate.h>
+#include <linux/leds.h>
 #include "input-compat.h"
 
 MODULE_AUTHOR("Vojtech Pavlik <[email protected]>");

drivers/input/input.c +18 -0

+			/* Redirect through trigger/LED pair */
+			if (dev->triggers && dev->triggers[code].name)
+				led_trigger_event(&dev->triggers[code],
+						  value ? LED_FULL : LED_OFF);
+			disposition = INPUT_PASS_TO_HANDLERS;
+#else /* !CONFIG_INPUT_LED */
+			/* Directly pass to device */
+			if (!!test_bit(code, dev->led) != !!value) {
+				__change_bit(code, dev->led);
+				disposition = INPUT_PASS_TO_ALL;
+			}
+#endif /* !CONFIG_INPUT_LED */
 		}
 		break;
 

@@ -711,6 +721,9 @@ static void input_disconnect_device(stru
 		handle->open = 0;
 
 	spin_unlock_irq(&dev->event_lock);
+
+	if (is_event_supported(EV_LED, dev->evbit, EV_MAX))
+		input_led_disconnect(dev);
 }
 
 /**

@@ -2137,6 +2150,9 @@ int input_register_device(struct input_d
 
 	list_add_tail(&dev->node, &input_dev_list);
 
+	if (is_event_supported(EV_LED, dev->evbit, EV_MAX))
+		input_led_connect(dev);
+
 	list_for_each_entry(handler, &input_handler_list, node)
 		input_attach_handler(dev, handler);
 

drivers/input/Kconfig +9 -0

@@ -178,6 +178,15 @@ comment "Input Device Drivers"
 
 source "drivers/input/keyboard/Kconfig"
 
+config INPUT_LEDS
+	bool "LED Support"
+	depends on LEDS_CLASS = INPUT || LEDS_CLASS = y
+	select LEDS_TRIGGERS
+	default y
+	help
+	  This option enables support for LEDs on keyboards managed
+	  by the input layer.
+
 source "drivers/input/mouse/Kconfig"
 
 source "drivers/input/joystick/Kconfig"

drivers/input/Makefile +3 -0

@@ -6,6 +6,9 @@
 
 obj-$(CONFIG_INPUT)		+= input-core.o
 input-core-y := input.o input-compat.o input-mt.o ff-core.o
+ifeq ($(CONFIG_INPUT_LEDS),y)
+input-core-y += leds.o
+endif
 
 obj-$(CONFIG_INPUT_FF_MEMLESS)	+= ff-memless.o
 obj-$(CONFIG_INPUT_POLLDEV)	+= input-polldev.o

include/linux/input.h +23 -0

@@ -79,6 +79,8 @@ struct input_value {
  * @led: reflects current state of device's LEDs
  * @snd: reflects current state of sound effects
  * @sw: reflects current state of device's switches
+ * @leds: led objects for the device's LEDs
+ * @triggers: trigger objects for the device's LEDs
  * @open: this method is called when the very first user calls
  *	input_open_device(). The driver must prepare the device
  *	to start generating events (start polling thread,

@@ -164,6 +166,9 @@ struct input_dev {
 	unsigned long snd[BITS_TO_LONGS(SND_CNT)];
 	unsigned long sw[BITS_TO_LONGS(SW_CNT)];
 
+	struct led_classdev *leds;
+	struct led_trigger *triggers;
+
 	int (*open)(struct input_dev *dev);
 	void (*close)(struct input_dev *dev);
 	int (*flush)(struct input_dev *dev, struct file *file);

@@ -531,4 +536,22 @@ int input_ff_erase(struct input_dev *dev
 int input_ff_create_memless(struct input_dev *dev, void *data,
 		int (*play_effect)(struct input_dev *, void *, struct ff_effect *));
 
+#ifdef CONFIG_INPUT_LEDS
+
+int input_led_connect(struct input_dev *dev);
+void input_led_disconnect(struct input_dev *dev);
+
+#else
+
+static inline int input_led_connect(struct input_dev *dev)
+{
+	return 0;
+}
+
+static inline void input_led_disconnect(struct input_dev *dev)
+{
+}
+
+#endif
+
 #endif

drivers/input/leds.c new file +61 -0

@@ -0,0 +1,155 @@
+/*
+ * LED support for the input layer
+ *
+ * Copyright 2010-2014 Samuel Thibault <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+/*
+ * This creates a trigger/LED pair per device:
+ * - the trigger is actioned from the core's input_get_disposition,
+ * - the LED is by default triggered by that trigger
+ * - the LED actuates the hardware.
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/leds.h>
+#include <linux/input.h>
+
+static const char *const input_led_names[LED_CNT] = {
+	[LED_NUML] = "numl",
+	[LED_CAPSL] = "capsl",
+	[LED_SCROLLL] = "scrolll",
+	[LED_COMPOSE] = "compose",
+	[LED_KANA] = "kana",
+	[LED_SLEEP] = "sleep",
+	[LED_SUSPEND] = "suspend",
+	[LED_MUTE] = "mute",
+	[LED_MISC] = "misc",
+	[LED_MAIL] = "mail",
+	[LED_CHARGING] = "charging",
+};
+
+/* Free LED data from input device, used at abortion and disconnection.  */
+static void input_led_delete(struct input_dev *dev)
+{
+	if (dev) {
+		struct led_classdev *leds = dev->leds;
+		struct led_trigger *triggers = dev->triggers;
+		int i;
+
+		if (leds) {
+			for (i = 0; i < LED_CNT; i++)
+				kfree(leds[i].name);
+			kfree(leds);
+			dev->leds = NULL;
+		}
+
+		if (triggers) {
+			for (i = 0; i < LED_CNT; i++)
+				kfree(triggers[i].name);
+			kfree(triggers);
+			dev->triggers = NULL;
+		}
+	}
+}

drivers/input/leds.c new file +15 -0

+	if (!dev)
+		/* Still initializing */
+		return;
+
+	leds = dev->leds;
+	led = cdev - leds;
+
+	if (test_bit(EV_LED, dev->evbit) &&
+		led <= LED_MAX && test_bit(led, dev->ledbit) &&
+		!!test_bit(led, dev->led) != !!brightness) {
+		__change_bit(led, dev->led);
+		dev->event(dev, EV_LED, led, !!brightness);
+	}
+}
+

drivers/input/leds.c new file +19 -0

+
+	/* Register this device's LEDs and triggers */
+	for (i = 0; i < LED_CNT; i++)
+		if (input_led_names[i] && test_bit(i, dev->ledbit)) {
+			leds[i].name = kasprintf(GFP_KERNEL, "%s::%s",
+						dev_name(&dev->dev),
+						input_led_names[i]);
+			if (!leds[i].name)
+				goto err;
+			leds[i].max_brightness = 1;
+			leds[i].brightness_set = perdevice_input_led_set;
+
+			triggers[i].name = kasprintf(GFP_KERNEL, "%s-%s",
+						dev_name(&dev->dev),
+						input_led_names[i]);
+			if (!triggers[i].name)
+				goto err;
+
+			/* make the LED triggered by the corresponding trigger

drivers/input/leds.c new file +14 -0

+		}
+
+	return 0;
+
+err:
+	input_led_delete(dev);
+	return error;
+}
+
+void input_led_disconnect(struct input_dev *dev)
+{
+	int i;
+	struct led_classdev *leds = dev->leds;
+	struct led_trigger *triggers = dev->triggers;

I'd rather we did not have a separate config option for this. Do we really need to support case where LEDs are disabled?

I'd rather stub it out instead of providing 2 separate code paths.

drivers/input/leds.c line 71

Quoted 9 lines

drivers/input/leds.c new file +9 -0

+/* LED state change for some keyboard, notify that keyboard.  */
+static void perdevice_input_led_set(struct led_classdev *cdev,
+			  enum led_brightness brightness)
+{
+	struct input_dev *dev;
+	struct led_classdev *leds;
+	int led;
+
+	dev = cdev->dev->platform_data;

Umm, platform data is not the best place for storing this. Why not drvdata?

Quoted 4 lines

drivers/input/leds.c new file +4 -0

+/* A new input device with potential LEDs to connect.  */
+int input_led_connect(struct input_dev *dev)
+{
+	int i, error = -ENOMEM;

drivers/input/leds.c line 96

Quoted 6 lines

drivers/input/leds.c new file +6 -0

+	struct led_classdev *leds;
+	struct led_trigger *triggers;
+
+	leds = kcalloc(LED_CNT, sizeof(*leds), GFP_KERNEL);
+	if (!leds)
+		goto err;

Why do we allocate all possible led's for every device?

drivers/input/leds.c line 102

Quoted 6 lines

drivers/input/leds.c new file +6 -0

+	dev->leds = leds;
+
+	triggers = kcalloc(LED_CNT, sizeof(*triggers), GFP_KERNEL);
+	if (!triggers)
+		goto err;
+	dev->triggers = triggers;

Hmm, maybe having per-device triggers is a bit of overkill and we could have just "input-numl", "input-capsl", etc.

drivers/input/leds.c line 131

Quoted 6 lines

drivers/input/leds.c new file +6 -0

+	/* No issue so far, we can register for real.  */
+	for (i = 0; i < LED_CNT; i++)
+		if (leds[i].name) {
+			led_classdev_register(&dev->dev, &leds[i]);
+			leds[i].dev->platform_data = dev;
+			led_trigger_register(&triggers[i]);

We need error handling here.

Dmitry Torokhov, le Sun 04 Jan 2015 15:28:38 -0800, a ?crit :

Quoted 2 lines

I'd rather we did not have a separate config option for this. Do we really need to support case where LEDs are disabled?

I don't really mind.

Quoted 1 line

I'd rather stub it out instead of providing 2 separate code paths.

Ok.

Quoted 11 lines

drivers/input/leds.c line 71

Quoted 9 lines

drivers/input/leds.c new file +9 -0

+/* LED state change for some keyboard, notify that keyboard.  */
+static void perdevice_input_led_set(struct led_classdev *cdev,
+			  enum led_brightness brightness)
+{
+	struct input_dev *dev;
+	struct led_classdev *leds;
+	int led;
+
+	dev = cdev->dev->platform_data;

Umm, platform data is not the best place for storing this. Why not drvdata?

Just because it didn't exist when I wrote the code :) Ok.

Quoted 4 lines

Quoted 4 lines

drivers/input/leds.c new file +4 -0

+/* A new input device with potential LEDs to connect.  */
+int input_led_connect(struct input_dev *dev)
+{
+	int i, error = -ENOMEM;

Quoted 8 lines

drivers/input/leds.c line 96

Quoted 6 lines

drivers/input/leds.c new file +6 -0

+	struct led_classdev *leds;
+	struct led_trigger *triggers;
+
+	leds = kcalloc(LED_CNT, sizeof(*leds), GFP_KERNEL);
+	if (!leds)
+		goto err;

Why do we allocate all possible led's for every device?

Ah, right, that was making things simpler, but it could be squeezed. I'm just afraid of one thing: may dev->ledbit change after input_register_device? It seems that at least uinput somehow permits this. It then means having to store the number of actually created LEDs and triggers alongside.

Quoted 10 lines

drivers/input/leds.c line 102

Quoted 7 lines

drivers/input/leds.c new file +6 -0

+	dev->leds = leds;
+
+	triggers = kcalloc(LED_CNT, sizeof(*triggers), GFP_KERNEL);
+	if (!triggers)
+		goto err;
+	dev->triggers = triggers;

Hmm, maybe having per-device triggers is a bit of overkill and we could have just "input-numl", "input-capsl", etc.

No, that won't work, notably for evdev access: we have to respect the per-device semantic.

Quoted 8 lines

drivers/input/leds.c line 131

Quoted 6 lines

drivers/input/leds.c new file +6 -0

+	/* No issue so far, we can register for real.  */
+	for (i = 0; i < LED_CNT; i++)
+		if (leds[i].name) {
+			led_classdev_register(&dev->dev, &leds[i]);
+			leds[i].dev->platform_data = dev;
+			led_trigger_register(&triggers[i]);

We need error handling here.

Right.

Samuel

The input device's capabilities (with the exception of keymap) should not change after device registration. If uinput allows that we should fix it there.

drivers/input/leds.c line 71

Quoted 1 line

drivers/input/leds.c new file +1 -0

+	dev = cdev->dev->platform_data;

Umm, platform data is not the best place for storing this. Why not drvdata?

Dmitry Torokhov, le Sun 04 Jan 2015 15:28:38 -0800, a ?crit :

Quoted 3 lines

drivers/input/leds.c line 71

Quoted 1 line

drivers/input/leds.c new file +1 -0

+	dev = cdev->dev->platform_data;

Umm, platform data is not the best place for storing this. Why not drvdata?

Ah, actually led_classdev already makes use of it, see the device_create_with_groups call in led_classdev_register.

Samuel Thibault, le Fri 23 Jan 2015 01:10:38 +0100, a ?crit :

Quoted 6 lines

Dmitry Torokhov, le Sun 04 Jan 2015 15:28:38 -0800, a ?crit :

Quoted 4 lines

drivers/input/leds.c line 71

Quoted 1 line

drivers/input/leds.c new file +1 -0

+	dev = cdev->dev->platform_data;

Umm, platform data is not the best place for storing this. Why not drvdata?

Quoted 2 lines

Ah, actually led_classdev already makes use of it, see the device_create_with_groups call in led_classdev_register.

Actually I'd say it makes sense to be using the platform_data field:
from the point of view of the led object, the input object is indeed something like a platform.

Quoted 10 lines

Samuel Thibault, le Fri 23 Jan 2015 01:10:38 +0100, a ?crit :

Quoted 8 lines

Dmitry Torokhov, le Sun 04 Jan 2015 15:28:38 -0800, a ?crit :

Quoted 4 lines

drivers/input/leds.c line 71

Quoted 1 line

drivers/input/leds.c new file +1 -0

+	dev = cdev->dev->platform_data;

Umm, platform data is not the best place for storing this. Why not drvdata?

Ah, actually led_classdev already makes use of it, see the device_create_with_groups call in led_classdev_register.

Quoted 3 lines

Actually I'd say it makes sense to be using the platform_data field:
from the point of view of the led object, the input object is indeed something like a platform.

No, platform data is what difefrentiates an arm board from another arm board, or an x86 or mips one.

From the point of view of a device connected to that board, yes. But

Dmitry Torokhov, le Thu 22 Jan 2015 16:37:02 -0800, a ?crit :

Quoted 15 lines · Samuel Thibault

Quoted 13 lines

Samuel Thibault, le Fri 23 Jan 2015 01:10:38 +0100, a ?crit :

Quoted 8 lines

Dmitry Torokhov, le Sun 04 Jan 2015 15:28:38 -0800, a ?crit :

Quoted 4 lines

drivers/input/leds.c line 71

Quoted 1 line

drivers/input/leds.c new file +1 -0

+	dev = cdev->dev->platform_data;

Umm, platform data is not the best place for storing this. Why not drvdata?

Ah, actually led_classdev already makes use of it, see the device_create_with_groups call in led_classdev_register.

Actually I'd say it makes sense to be using the platform_data field:
from the point of view of the led object, the input object is indeed something like a platform.

Quoted 2 lines

No, platform data is what difefrentiates an arm board from another arm board, or an x86 or mips one.

From the point of view of a device connected to that board, yes. But from the point of view of LEDs connected to a keyboard, the platform is the keyboard.

No. Please take a look at other users of platform data. Platform is the box we are running on, not parent device. Platform data is supposed to be constant, not changing between driver runs.

Here is v5 coming, with separate patches for the kbd and the input parts.

input4::capsl/trigger was none by default, that can't be right, right?

I tried putting kbd-capslock and input4-capsl there, but that did not seem to help.

It works with heartbeat trigger, but not with input4-numl trigger.

vt::capsl/brightness controls capslock led, even when input4-capsl/trigger is set to input4-numl.

Confused,

Pavel Machek, le Fri 02 Jan 2015 20:53:51 +0100, a ?crit :

Quoted 1 line

input4::capsl/trigger was none by default, that can't be right, right?

Indeed. And I don't see how it can be that way, since the input4::capsl LED and the input4-capsl trigger get initialized at the same time in input_led_connect... (and the order does not matter since both will try to connect to the other on registration).

Quoted 2 lines

I tried putting kbd-capslock and input4-capsl there, but that did not seem to help.

That should have.

Quoted 2 lines

It works with heartbeat trigger, but not with input4-numl trigger.

It should have worked with input4-numl too.

Quoted 2 lines

vt::capsl/brightness controls capslock led, even when input4-capsl/trigger is set to input4-numl.

It shouldn't.

Quoted 1 line

Confused,

I guess I'm even more. I had tested everything that you have described, without any issue, on both an internal laptop keyboard and an external USB keyboard, and have tested again just now, with the same success.

Which hardware setup do you have, more precisely?

Quoted 2 lines

Here is v5 coming, with separate patches for the kbd and the input parts.

After booting with this, capslock led does not seem to work on text console.

input4::capsl/trigger was none by default, that can't be right, right?

I tried putting kbd-capslock and input4-capsl there, but that did not seem to help.

It works with heartbeat trigger, but not with input4-numl trigger. (But numlock led works ok with that trigger, weird).

vt::capsl/brightness controls capslock led, even when input4-capsl/trigger is set to input4-numl.

I have just tried what you described on a thinkpad lenovo T500, without any problem, except that the thinkpad BIOS does a couple of funky things:

Quoted 2 lines

Here is v5 coming, with separate patches for the kbd and the input parts.

After booting with this, capslock led does not seem to work on text console.

input4::capsl/trigger was none by default, that can't be right, right?

I tried putting kbd-capslock and input4-capsl there, but that did not seem to help.

It works with heartbeat trigger, but not with input4-numl trigger. (But numlock led works ok with that trigger, weird).

vt::capsl/brightness controls capslock led, even when input4-capsl/trigger is set to input4-numl.