[PATCHv5 0/2] INPUT: Route keyboard LEDs through the generic LEDs layer

[PATCHv5 0/2] INPUT: Route keyboard LEDs through the generic LEDs layer

[Samuel Thibault]

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

Samuel

[PATCHv5 1/2] INPUT: Introduce generic trigger/LED pairs between keyboard modifiers and input LEDs

[Samuel Thibault]

This permits to change which modifier triggers which keyboard LEDs, by adding
modifier triggers, and a series of evled LEDs.

This permits to fix #7063 from userland by using a modifier to implement proper
CapsLock behavior and have the keyboard caps lock led show that modifier state.

[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 <samuel.thibault@ens-lyon.org>
Signed-off-by: Evan Broder <evan@ebroder.net>
Acked-by: Peter Korsgaard <jacmet@sunsite.dk>
Signed-off-by: John Crispin <blogic@openwrt.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
Changed in this version:
- separate kbd LED changes from input LED changes
- remove the VT triggers, revert back to emitting EV_LED events

--- a/drivers/tty/Kconfig
+++ b/drivers/tty/Kconfig
@@ -13,6 +13,9 @@ config VT
 	bool "Virtual terminal" if EXPERT
 	depends on !S390 && !UML
 	select INPUT
+	select NEW_LEDS
+	select LEDS_CLASS
+	select LEDS_TRIGGERS
 	default y
 	---help---
 	  If you say Y here, you will get support for terminal devices with
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -33,6 +33,7 @@
 #include <linux/string.h>
 #include <linux/init.h>
 #include <linux/slab.h>
+#include <linux/leds.h>
 
 #include <linux/kbd_kern.h>
 #include <linux/kbd_diacr.h>
@@ -130,6 +131,7 @@ static char rep;					/* flag telling cha
 static int shift_state = 0;
 
 static unsigned char ledstate = 0xff;			/* undefined */
+static unsigned char lockstate = 0xff;			/* undefined */
 static unsigned char ledioctl;
 
 /*
@@ -962,6 +964,146 @@ static void k_brl(struct vc_data *vc, un
 }
 
 /*
+ * Keyboard LEDs are propagated by default like the following example:
+ *
+ * VT keyboard LED state or modifier state, calls kbd_bh()
+ * -> kbd-xxx VT trigger
+ * -> vt::xxx VT LED
+ * -> input EV_DEV events (in vt_led_cb)
+ *
+ * KDSETLED directly triggers vt::xxx LEDs.
+ *
+ * Userland can however choose the trigger for the vt::numl LED, or
+ * independently choose the trigger for any inputx::numl LED.
+ */
+
+/* We route VT keyboard "leds" through triggers */
+static void kbd_ledstate_trigger_activate(struct led_classdev *cdev);
+
+static struct led_trigger ledtrig_ledstate[] = {
+#define DEFINE_LEDSTATE_TRIGGER(kbd_led, nam) \
+	[kbd_led] = { \
+		.name = nam, \
+		.activate = kbd_ledstate_trigger_activate, \
+	}
+	DEFINE_LEDSTATE_TRIGGER(VC_SCROLLOCK, "kbd-scrollock"),
+	DEFINE_LEDSTATE_TRIGGER(VC_NUMLOCK,   "kbd-numlock"),
+	DEFINE_LEDSTATE_TRIGGER(VC_CAPSLOCK,  "kbd-capslock"),
+	DEFINE_LEDSTATE_TRIGGER(VC_KANALOCK,  "kbd-kanalock"),
+#undef DEFINE_LEDSTATE_TRIGGER
+};
+
+static int input_leds[] = {
+	[VC_SCROLLOCK] = LED_SCROLLL,
+	[VC_NUMLOCK] = LED_NUML,
+	[VC_CAPSLOCK] = LED_CAPSL,
+	[VC_KANALOCK] = LED_KANA,
+};
+
+/* Called on trigger connection, to set initial state */
+static void kbd_ledstate_trigger_activate(struct led_classdev *cdev)
+{
+	struct led_trigger *trigger = cdev->trigger;
+	int led = trigger - ledtrig_ledstate;
+
+	tasklet_disable(&keyboard_tasklet);
+	led_trigger_event(trigger, ledstate & (1 << led) ? LED_FULL : LED_OFF);
+	tasklet_enable(&keyboard_tasklet);
+}
+
+/* We route VT keyboard lockstates through triggers */
+static void kbd_lockstate_trigger_activate(struct led_classdev *cdev);
+
+static struct led_trigger ledtrig_lockstate[] = {
+#define DEFINE_LOCKSTATE_TRIGGER(kbd_led, nam) \
+	[kbd_led] = { \
+		.name = nam, \
+		.activate = kbd_lockstate_trigger_activate, \
+	}
+	DEFINE_LOCKSTATE_TRIGGER(VC_SHIFTLOCK,  "kbd-shiftlock"),
+	DEFINE_LOCKSTATE_TRIGGER(VC_ALTGRLOCK,  "kbd-altgrlock"),
+	DEFINE_LOCKSTATE_TRIGGER(VC_CTRLLOCK,   "kbd-ctrllock"),
+	DEFINE_LOCKSTATE_TRIGGER(VC_ALTLOCK,    "kbd-altlock"),
+	DEFINE_LOCKSTATE_TRIGGER(VC_SHIFTLLOCK, "kbd-shiftllock"),
+	DEFINE_LOCKSTATE_TRIGGER(VC_SHIFTRLOCK, "kbd-shiftrlock"),
+	DEFINE_LOCKSTATE_TRIGGER(VC_CTRLLLOCK,  "kbd-ctrlllock"),
+	DEFINE_LOCKSTATE_TRIGGER(VC_CTRLRLOCK,  "kbd-ctrlrlock"),
+#undef DEFINE_LOCKSTATE_TRIGGER
+};
+
+/* Called on trigger connection, to set initial state */
+static void kbd_lockstate_trigger_activate(struct led_classdev *cdev)
+{
+	struct led_trigger *trigger = cdev->trigger;
+	int led = trigger - ledtrig_lockstate;
+
+	tasklet_disable(&keyboard_tasklet);
+	led_trigger_event(trigger, lockstate & (1 << led) ? LED_FULL : LED_OFF);
+	tasklet_enable(&keyboard_tasklet);
+}
+
+/* Handler for VT LEDs, scheduling injecting input events in a worker */
+static void vt_led_set(struct led_classdev *cdev,
+			  enum led_brightness brightness);
+static struct led_classdev vt_leds[LED_CNT] = {
+#define DEFINE_INPUT_LED(vt_led, nam, deftrig) \
+	[vt_led] = { \
+		.name = "vt::"nam, \
+		.max_brightness = 1, \
+		.brightness_set = vt_led_set, \
+		.default_trigger = deftrig, \
+	}
+/* Default triggers for the VT LEDs just correspond to the legacy
+ * usage. */
+	DEFINE_INPUT_LED(LED_NUML, "numl", "kbd-numlock"),
+	DEFINE_INPUT_LED(LED_CAPSL, "capsl", "kbd-capslock"),
+	DEFINE_INPUT_LED(LED_SCROLLL, "scrolll", "kbd-scrollock"),
+	DEFINE_INPUT_LED(LED_COMPOSE, "compose", NULL),
+	DEFINE_INPUT_LED(LED_KANA, "kana", "kbd-kanalock"),
+	DEFINE_INPUT_LED(LED_SLEEP, "sleep", NULL),
+	DEFINE_INPUT_LED(LED_SUSPEND, "suspend", NULL),
+	DEFINE_INPUT_LED(LED_MUTE, "mute", NULL),
+	DEFINE_INPUT_LED(LED_MISC, "misc", NULL),
+	DEFINE_INPUT_LED(LED_MAIL, "mail", NULL),
+	DEFINE_INPUT_LED(LED_CHARGING, "charging", NULL),
+};
+
+static struct work_struct vt_led_work[LED_CNT];
+static char vt_led_state[LED_CNT];
+
+/* Emit input events to actually update one LED */
+static int kbd_update_leds_helper(struct input_handle *handle, void *data)
+{
+	int led = *(int *)data;
+
+	if (test_bit(EV_LED, handle->dev->evbit)) {
+		input_inject_event(handle, EV_LED, led, vt_led_state[led]);
+		input_inject_event(handle, EV_SYN, SYN_REPORT, 0);
+	}
+
+	return 0;
+}
+
+/* Emit input events to actually update one LED on all keyboards */
+static void vt_led_cb(struct work_struct *work)
+{
+	int led = work - vt_led_work;
+
+	input_handler_for_each_handle(&kbd_handler, &led,
+				      kbd_update_leds_helper);
+}
+
+/* VT LED state change, scheduling triggering input events for it */
+static void vt_led_set(struct led_classdev *cdev,
+			  enum led_brightness brightness)
+{
+	int led = cdev - vt_leds;
+
+	vt_led_state[led] = !!brightness;
+	schedule_work(&vt_led_work[led]);
+}
+
+/*
  * The leds display either (i) the status of NumLock, CapsLock, ScrollLock,
  * or (ii) whatever pattern of lights people want to show using KDSETLED,
  * or (iii) specified bits of specified words in kernel memory.
@@ -995,20 +1137,6 @@ static inline unsigned char getleds(void
 	return kb->ledflagstate;
 }
 
-static int kbd_update_leds_helper(struct input_handle *handle, void *data)
-{
-	unsigned char leds = *(unsigned char *)data;
-
-	if (test_bit(EV_LED, handle->dev->evbit)) {
-		input_inject_event(handle, EV_LED, LED_SCROLLL, !!(leds & 0x01));
-		input_inject_event(handle, EV_LED, LED_NUML,    !!(leds & 0x02));
-		input_inject_event(handle, EV_LED, LED_CAPSL,   !!(leds & 0x04));
-		input_inject_event(handle, EV_SYN, SYN_REPORT, 0);
-	}
-
-	return 0;
-}
-
 /**
  *	vt_get_leds	-	helper for braille console
  *	@console: console to read
@@ -1085,26 +1213,39 @@ void vt_kbd_con_stop(int console)
 }
 
 /*
- * This is the tasklet that updates LED state on all keyboards
- * attached to the box. The reason we use tasklet is that we
- * need to handle the scenario when keyboard handler is not
- * registered yet but we already getting updates from the VT to
- * update led state.
+ * This is the tasklet that updates LED state of all LED triggers, which will
+ * thus update the VT LED status, and eventually submit input events.
+ * The reason we use tasklet is that we need to handle the scenario when
+ * keyboard handler is not registered yet but we already getting updates
+ * from the VT to update led state.
  */
 static void kbd_bh(unsigned long dummy)
 {
 	unsigned char leds;
 	unsigned long flags;
-	
+	int i;
+
 	spin_lock_irqsave(&led_lock, flags);
 	leds = getleds();
 	spin_unlock_irqrestore(&led_lock, flags);
 
 	if (leds != ledstate) {
-		input_handler_for_each_handle(&kbd_handler, &leds,
-					      kbd_update_leds_helper);
+		for (i = 0; i < ARRAY_SIZE(ledtrig_ledstate); i++)
+			if ((leds ^ ledstate) & (1 << i))
+				led_trigger_event(&ledtrig_ledstate[i],
+						leds & (1 << i)
+						? LED_FULL : LED_OFF);
 		ledstate = leds;
 	}
+
+	if (kbd->lockstate != lockstate) {
+		for (i = 0; i < ARRAY_SIZE(ledtrig_lockstate); i++)
+			if ((kbd->lockstate ^ lockstate) & (1 << i))
+				led_trigger_event(&ledtrig_lockstate[i],
+						kbd->lockstate & (1 << i)
+						? LED_FULL : LED_OFF);
+		lockstate = kbd->lockstate;
+	}
 }
 
 DECLARE_TASKLET_DISABLED(keyboard_tasklet, kbd_bh, 0);
@@ -1448,10 +1589,13 @@ static void kbd_disconnect(struct input_
  */
 static void kbd_start(struct input_handle *handle)
 {
+	int i;
+
 	tasklet_disable(&keyboard_tasklet);
 
 	if (ledstate != 0xff)
-		kbd_update_leds_helper(handle, &ledstate);
+		for (i = 0; i < ARRAY_SIZE(ledtrig_ledstate); i++)
+			kbd_update_leds_helper(handle, &input_leds[i]);
 
 	tasklet_enable(&keyboard_tasklet);
 }
@@ -1501,6 +1645,26 @@ int __init kbd_init(void)
 	if (error)
 		return error;
 
+	for (i = 0; i < ARRAY_SIZE(ledtrig_ledstate); i++) {
+		error = led_trigger_register(&ledtrig_ledstate[i]);
+		if (error)
+			pr_err("error %d while registering trigger %s\n",
+					error, ledtrig_ledstate[i].name);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(ledtrig_lockstate); i++) {
+		error = led_trigger_register(&ledtrig_lockstate[i]);
+		if (error)
+			pr_err("error %d while registering trigger %s\n",
+					error, ledtrig_lockstate[i].name);
+	}
+
+	for (i = 0; i < LED_CNT; i++)
+		if (vt_leds[i].name) {
+			led_classdev_register(NULL, &vt_leds[i]);
+			INIT_WORK(&vt_led_work[i], vt_led_cb);
+		}
+
 	tasklet_enable(&keyboard_tasklet);
 	tasklet_schedule(&keyboard_tasklet);
 
--- a/Documentation/leds/leds-class.txt
+++ b/Documentation/leds/leds-class.txt
@@ -2,9 +2,6 @@
 LED handling under Linux
 ========================
 
-If you're reading this and thinking about keyboard leds, these are
-handled by the input subsystem and the led class is *not* needed.
-
 In its simplest form, the LED class just allows control of LEDs from
 userspace. LEDs appear in /sys/class/leds/. The maximum brightness of the
 LED is defined in max_brightness file. The brightness file will set the brightness
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -11,9 +11,6 @@ menuconfig NEW_LEDS
 	  Say Y to enable Linux LED support.  This allows control of supported
 	  LEDs from both userspace and optionally, by kernel events (triggers).
 
-	  This is not related to standard keyboard LEDs which are controlled
-	  via the input system.
-
 if NEW_LEDS
 
 config LEDS_CLASS

[PATCHv5 2/2] INPUT: Introduce generic trigger/LED pairs to input LEDs

[Samuel Thibault]

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 <samuel.thibault@ens-lyon.org>
Signed-off-by: Evan Broder <evan@ebroder.net>
Acked-by: Peter Korsgaard <jacmet@sunsite.dk>
Signed-off-by: John Crispin <blogic@openwrt.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
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

--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -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 <vojtech@suse.cz>");
@@ -324,11 +325,20 @@ static int input_get_disposition(struct
 		break;
 
 	case EV_LED:
-		if (is_event_supported(code, dev->ledbit, LED_MAX) &&
-		    !!test_bit(code, dev->led) != !!value) {
-
-			__change_bit(code, dev->led);
-			disposition = INPUT_PASS_TO_ALL;
+		if (is_event_supported(code, dev->ledbit, LED_MAX)) {
+#ifdef CONFIG_INPUT_LED
+			/* 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);
 
--- a/drivers/input/Kconfig
+++ b/drivers/input/Kconfig
@@ -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"
--- a/drivers/input/Makefile
+++ b/drivers/input/Makefile
@@ -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
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -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
--- /dev/null
+++ b/drivers/input/leds.c
@@ -0,0 +1,155 @@
+/*
+ * LED support for the input layer
+ *
+ * Copyright 2010-2014 Samuel Thibault <samuel.thibault@ens-lyon.org>
+ *
+ * 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;
+		}
+	}
+}
+
+/* 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;
+	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);
+	}
+}
+
+/* A new input device with potential LEDs to connect.  */
+int input_led_connect(struct input_dev *dev)
+{
+	int i, error = -ENOMEM;
+	struct led_classdev *leds;
+	struct led_trigger *triggers;
+
+	leds = kcalloc(LED_CNT, sizeof(*leds), GFP_KERNEL);
+	if (!leds)
+		goto err;
+	dev->leds = leds;
+
+	triggers = kcalloc(LED_CNT, sizeof(*triggers), GFP_KERNEL);
+	if (!triggers)
+		goto err;
+	dev->triggers = triggers;
+
+	/* 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
+			 * by default */
+			leds[i].default_trigger = triggers[i].name;
+		}
+
+	/* 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]);
+		}
+
+	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;
+
+	for (i = 0; i < LED_CNT; i++) {
+		if (leds[i].name)
+			led_classdev_unregister(&leds[i]);
+		if (triggers[i].name)
+			led_trigger_unregister(&triggers[i]);
+	}
+
+	input_led_delete(dev);
+}

[PATCHv5 2/2] INPUT: Introduce generic trigger/LED pairs to input LEDs

[Dmitry Torokhov]

Hi Samuel,

On Sat, Dec 27, 2014 at 12:23:10AM +0100, Samuel Thibault wrote:
> 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 <samuel.thibault@ens-lyon.org>
> Signed-off-by: Evan Broder <evan@ebroder.net>
> Acked-by: Peter Korsgaard <jacmet@sunsite.dk>
> Signed-off-by: John Crispin <blogic@openwrt.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 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
> 
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -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 <vojtech@suse.cz>");
> @@ -324,11 +325,20 @@ static int input_get_disposition(struct
>  		break;
>  
>  	case EV_LED:
> -		if (is_event_supported(code, dev->ledbit, LED_MAX) &&
> -		    !!test_bit(code, dev->led) != !!value) {
> -
> -			__change_bit(code, dev->led);
> -			disposition = INPUT_PASS_TO_ALL;
> +		if (is_event_supported(code, dev->ledbit, LED_MAX)) {
> +#ifdef CONFIG_INPUT_LED

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.

> +			/* 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);
>  
> --- a/drivers/input/Kconfig
> +++ b/drivers/input/Kconfig
> @@ -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"
> --- a/drivers/input/Makefile
> +++ b/drivers/input/Makefile
> @@ -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
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -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
> --- /dev/null
> +++ b/drivers/input/leds.c
> @@ -0,0 +1,155 @@
> +/*
> + * LED support for the input layer
> + *
> + * Copyright 2010-2014 Samuel Thibault <samuel.thibault@ens-lyon.org>
> + *
> + * 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;
> +		}
> +	}
> +}
> +
> +/* 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?

> +	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);
> +	}
> +}
> +
> +/* A new input device with potential LEDs to connect.  */
> +int input_led_connect(struct input_dev *dev)
> +{
> +	int i, error = -ENOMEM;
> +	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?

> +	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.

> +
> +	/* 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
> +			 * by default */
> +			leds[i].default_trigger = triggers[i].name;
> +		}
> +
> +	/* 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.

> +		}
> +
> +	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;
> +
> +	for (i = 0; i < LED_CNT; i++) {
> +		if (leds[i].name)
> +			led_classdev_unregister(&leds[i]);
> +		if (triggers[i].name)
> +			led_trigger_unregister(&triggers[i]);
> +	}
> +
> +	input_led_delete(dev);
> +}

Thanks.

-- 
Dmitry

[PATCHv5 2/2] INPUT: Introduce generic trigger/LED pairs to input LEDs

[Samuel Thibault]

Dmitry Torokhov, le Sun 04 Jan 2015 15:28:38 -0800, a ?crit :
> 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.

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

Ok.

> > +/* 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.

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

> > +	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.

> > +	/* 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

[PATCHv5 2/2] INPUT: Introduce generic trigger/LED pairs to input LEDs

[Dmitry Torokhov]

Hi Samuel,

On Mon, Jan 05, 2015 at 12:45:01AM +0100, Samuel Thibault wrote:
> Dmitry Torokhov, le Sun 04 Jan 2015 15:28:38 -0800, a ?crit :
> > 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.
> 
> > I'd rather stub it out instead of providing 2 separate code paths.
> 
> Ok.
> 
> > > +/* 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.
> 
> > > +/* A new input device with potential LEDs to connect.  */
> > > +int input_led_connect(struct input_dev *dev)
> > > +{
> > > +	int i, error = -ENOMEM;
> > > +	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.

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.

Thanks.

> 
> > > +	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.
> 
> > > +	/* 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

-- 
Dmitry

[PATCHv5 2/2] INPUT: Introduce generic trigger/LED pairs to input LEDs

[Samuel Thibault]

Dmitry Torokhov, le Mon 05 Jan 2015 09:42:05 -0800, a ?crit :
> 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.

Ah, I didn't notice the content of uinput_set_bit() there.  It seems to
be making sure that the device hasn't been created yet indeed.  All good
then.

Samuel

[PATCHv5 2/2] INPUT: Introduce generic trigger/LED pairs to input LEDs

[Samuel Thibault]

Dmitry Torokhov, le Sun 04 Jan 2015 15:28:38 -0800, a ?crit :
> > +	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

[PATCHv5 2/2] INPUT: Introduce generic trigger/LED pairs to input LEDs

[Samuel Thibault]

Samuel Thibault, le Fri 23 Jan 2015 01:10:38 +0100, a ?crit :
> Dmitry Torokhov, le Sun 04 Jan 2015 15:28:38 -0800, a ?crit :
> > > +	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.

Samuel

[PATCHv5 2/2] INPUT: Introduce generic trigger/LED pairs to input LEDs

[Dmitry Torokhov]

On Friday, January 23, 2015 01:30:11 AM Samuel Thibault wrote:
> Samuel Thibault, le Fri 23 Jan 2015 01:10:38 +0100, a ?crit :
> > Dmitry Torokhov, le Sun 04 Jan 2015 15:28:38 -0800, a ?crit :
> > > > +	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.

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

Thanks.

-- 
Dmitry

[PATCHv5 2/2] INPUT: Introduce generic trigger/LED pairs to input LEDs

[Samuel Thibault]

Dmitry Torokhov, le Thu 22 Jan 2015 16:37:02 -0800, a ?crit :
> On Friday, January 23, 2015 01:30:11 AM Samuel Thibault wrote:
> > Samuel Thibault, le Fri 23 Jan 2015 01:10:38 +0100, a ?crit :
> > > Dmitry Torokhov, le Sun 04 Jan 2015 15:28:38 -0800, a ?crit :
> > > > > +	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.
> 
> 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.

Samuel

[PATCHv5 2/2] INPUT: Introduce generic trigger/LED pairs to input LEDs

[Dmitry Torokhov]

On Friday, January 23, 2015 01:44:29 AM Samuel Thibault wrote:
> Dmitry Torokhov, le Thu 22 Jan 2015 16:37:02 -0800, a ?crit :
> > On Friday, January 23, 2015 01:30:11 AM Samuel Thibault wrote:
> > > Samuel Thibault, le Fri 23 Jan 2015 01:10:38 +0100, a ?crit :
> > > > Dmitry Torokhov, le Sun 04 Jan 2015 15:28:38 -0800, a ?crit :
> > > > > > +	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.
> > 
> > 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.

Thanks.

-- 
Dmitry

[PATCHv5 2/2] INPUT: Introduce generic trigger/LED pairs to input LEDs

[Samuel Thibault]

Dmitry Torokhov, le Thu 22 Jan 2015 16:51:16 -0800, a ?crit :
> 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.

Ok, well, then we can just use to_input_dev(cdev->dev->parent); to
access the input_dev from the led_classdev cdev.

Samuel

[PATCHv5 0/2] INPUT: Route keyboard LEDs through the generic LEDs layer

[Pavel Machek]

Hi!

> 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.

Confused,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv5 0/2] INPUT: Route keyboard LEDs through the generic LEDs layer

[Samuel Thibault]

Pavel Machek, le Fri 02 Jan 2015 20:53:51 +0100, a ?crit :
> 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).

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

That should have.

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

It should have worked with input4-numl too.

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

It shouldn't.

> 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?

Samuel

[PATCHv5 0/2] INPUT: Route keyboard LEDs through the generic LEDs layer

[Pavel Machek]

On Fri 2015-01-02 21:11:17, Samuel Thibault wrote:
> Pavel Machek, le Fri 02 Jan 2015 20:53:51 +0100, a ?crit :
> > 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).
> 
> > I tried putting kbd-capslock and input4-capsl there, but that did not
> > seem to help.
> 
> That should have.
> 
> > It works with heartbeat trigger, but not with input4-numl
> > trigger.
> 
> It should have worked with input4-numl too.
> 
> > vt::capsl/brightness controls capslock led, even when
> > input4-capsl/trigger is set to input4-numl.
> 
> It shouldn't.
> 
> > 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?

I tested on Thinkpad X60, with internal keyboard. (PS/2, I'd say). I
let Debian boot into graphical login prompt, switched to text console,
and did the testing there.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCHv5 0/2] INPUT: Route keyboard LEDs through the generic LEDs layer

[Samuel Thibault]

Hello,

Pavel Machek, le Fri 02 Jan 2015 20:53:51 +0100, a ?crit :
> > 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:

- depending on the numlock bios setting, the numlock led will be
  driven by the OS (synchronized mode) or handled internally in the BIOS
  (independent mode).
- in synchronized mode, whenever the internal numlock led is lit, the
  right part of the internal keyboard acts as the keypad keys.

Samuel

[PATCHv5 0/2] INPUT: Route keyboard LEDs through the generic LEDs layer

[Samuel Thibault]

Hello,

Samuel Thibault, le Wed 21 Jan 2015 16:21:12 +0100, a ?crit :
> 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:

Actually it's even worse than that: depending on whether it is in
synchronized mode or independent mode, *and* depending on the current
*state* of the internal numlock LED, and probably also whether an
external keyboard is connected, the internal numlock key will produce
*or not* key events!  The actual transition table for this seems quite
clumsy, basically the BIOS seems to be assuming that the the numlock
key toggles the numlock LED, and setting up anything different leads
to all kinds of confusing effects, the "simplest" of which being that
you definitely not want to set e.g. a heartbeat trigger on the numlock
LED, or else the right part of the keyboard will randomly behave as a
keypad...

I guess that may happen with other laptops, so I'm afraid I can only
reject any kind of bug report involving the internal numlock LED or key
of a laptop.

So we're left with these:

Pavel Machek, le Fri 02 Jan 2015 20:53:51 +0100, a ?crit :
> > 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.

Did you check that it was working before?  If you are using
console-setup, it is a known bug that the capslock LED doesn't reflect
the layout capslock state, because console-setup uses another modifier
than the capslock modifier, because that one has unwanted legacy
hardcoded effects.  That's precisely the point of the keyboard part
of my two led patches to provide console-setup with an interface to
properly route the layout capslock state (expressed as another modifier
than capslock) to the capslock LED.

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

That is still not right, and I still don't see how that can happen: my
patch registers the input4::capsl LED with its default trigger name
just before the input4-capsl trigger, and doesn't do anything about
trigger assignment after that, so I don't see how it can be coming from
my patch.

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

Was it at least changing the content of the trigger file?  Again, if you
are using console-setup, it is normal that the capslock modifier does
not change, console-setup uses another modifier, that can be seen in
dumpkeys | grep 58, depending on your precise configuration it will use
various modifiers.  So in that case the kbd-capslock and input*-capsl
are not good tests.

Basically you're left with the kbd-scrollock and input*-scrolll triggers
and scrollock key for easy testing, if you happen to have that key on
your keyboard... (apparently the thinkpad I'm testing doesn't do funky
things with it).

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

That is probably due to the weird numlock behavior of the thinkpad.

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

That is also not right, I can't reproduce it, and I don't see how it can
happen with my patch.  Could you copy/paste the commands you are using?

I have also posted reworked patches according to Dmitry's comments,
probably better try those now.

I'm sorry I forgot to make the second threaded with the first, but they
are independent anyway.

Samuel