[PATCHv4] INPUT: Route keyboard LEDs through the generic LEDs layer.

[PATCHv4] INPUT: Route keyboard LEDs through the generic LEDs layer.

[Samuel Thibault]

This permits to reassign keyboard LEDs to something else than keyboard "leds"
state, by adding keyboard led and modifier triggers connected to a series
of VT input LEDs, themselves connected to VT input triggers, which
per-input device LEDs use by default.  Userland can thus easily change the LED
behavior of (a priori) all input devices, or of particular input devices.

This also 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@mokafive.com: Rebased to 3.2-rc1 or so, cleaned up some includes, and fixed some constants]
[blogic@openwrt.org: CONFIG_INPUT_LEDS stubs should be static inline]
[akpm@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>
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Tested-by: Pavel Machek <pavel@ucw.cz>
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:
- Use kcalloc instead of kzalloc
- to avoid any mutex order violation, defer LED update into a work callback.

This has been in mm for some months now.

--- 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/input/input.c
+++ b/drivers/input/input.c
@@ -711,6 +711,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 +2140,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);
 
@@ -2422,6 +2428,8 @@ static int __init input_init(void)
 		goto fail2;
 	}
 
+	input_led_init();
+
 	return 0;
 
  fail2:	input_proc_exit();
@@ -2431,6 +2439,7 @@ static int __init input_init(void)
 
 static void __exit input_exit(void)
 {
+	input_led_exit();
 	input_proc_exit();
 	unregister_chrdev_region(MKDEV(INPUT_MAJOR, 0),
 				 INPUT_MAX_CHAR_DEVICES);
--- 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/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
--- a/drivers/tty/Kconfig
+++ b/drivers/tty/Kconfig
@@ -13,6 +13,10 @@ config VT
 	bool "Virtual terminal" if EXPERT
 	depends on !S390 && !UML
 	select INPUT
+	select NEW_LEDS
+	select LEDS_CLASS
+	select LEDS_TRIGGERS
+	select INPUT_LEDS
 	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;
 
 /*
@@ -961,6 +963,41 @@ static void k_brl(struct vc_data *vc, un
 	}
 }
 
+/* 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 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
+};
+
 /*
  * The leds display either (i) the status of NumLock, CapsLock, ScrollLock,
  * or (ii) whatever pattern of lights people want to show using KDSETLED,
@@ -995,18 +1032,25 @@ static inline unsigned char getleds(void
 	return kb->ledflagstate;
 }
 
-static int kbd_update_leds_helper(struct input_handle *handle, void *data)
+/* Called on trigger connection, to set initial state */
+static void kbd_ledstate_trigger_activate(struct led_classdev *cdev)
 {
-	unsigned char leds = *(unsigned char *)data;
+	struct led_trigger *trigger = cdev->trigger;
+	int led = trigger - ledtrig_ledstate;
 
-	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);
-	}
+	tasklet_disable(&keyboard_tasklet);
+	led_trigger_event(trigger, ledstate & (1 << led) ? LED_FULL : LED_OFF);
+	tasklet_enable(&keyboard_tasklet);
+}
 
-	return 0;
+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);
 }
 
 /**
@@ -1095,16 +1139,29 @@ 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);
@@ -1442,20 +1499,6 @@ static void kbd_disconnect(struct input_
 	kfree(handle);
 }
 
-/*
- * Start keyboard handler on the new keyboard by refreshing LED state to
- * match the rest of the system.
- */
-static void kbd_start(struct input_handle *handle)
-{
-	tasklet_disable(&keyboard_tasklet);
-
-	if (ledstate != 0xff)
-		kbd_update_leds_helper(handle, &ledstate);
-
-	tasklet_enable(&keyboard_tasklet);
-}
-
 static const struct input_device_id kbd_ids[] = {
 	{
 		.flags = INPUT_DEVICE_ID_MATCH_EVBIT,
@@ -1477,7 +1520,6 @@ static struct input_handler kbd_handler
 	.match		= kbd_match,
 	.connect	= kbd_connect,
 	.disconnect	= kbd_disconnect,
-	.start		= kbd_start,
 	.name		= "kbd",
 	.id_table	= kbd_ids,
 };
@@ -1501,6 +1543,20 @@ 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);
+	}
+
 	tasklet_enable(&keyboard_tasklet);
 	tasklet_schedule(&keyboard_tasklet);
 
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -79,6 +79,7 @@ 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: leds 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 +165,8 @@ struct input_dev {
 	unsigned long snd[BITS_TO_LONGS(SND_CNT)];
 	unsigned long sw[BITS_TO_LONGS(SW_CNT)];
 
+	struct led_classdev *leds;
+
 	int (*open)(struct input_dev *dev);
 	void (*close)(struct input_dev *dev);
 	int (*flush)(struct input_dev *dev, struct file *file);
@@ -531,4 +534,29 @@ 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
+
+void input_led_init(void);
+void input_led_exit(void);
+
+int input_led_connect(struct input_dev *dev);
+void input_led_disconnect(struct input_dev *dev);
+
+#else
+
+static inline void input_led_init(void) { }
+
+static inline void input_led_exit(void) { }
+
+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,272 @@
+/*
+ * 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.
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/leds.h>
+#include <linux/input.h>
+
+/*
+ * Keyboard LEDs are propagated by default like the following example:
+ *
+ * VT keyboard numlock trigger
+ * -> vt::numl VT LED
+ * -> vt-numl VT trigger
+ * -> per-device inputX::numl LED
+ *
+ * Userland can however choose the trigger for the vt::numl LED, or
+ * independently choose the trigger for any inputx::numl LED.
+ *
+ *
+ * VT LED classes and triggers are registered on-demand according to
+ * existing LED devices
+ */
+
+/* Handler for VT LEDs, just triggers the corresponding VT trigger. */
+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 const char *const vt_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",
+};
+/* Handler for hotplug initialization */
+static void vt_led_trigger_activate(struct led_classdev *cdev);
+/* VT triggers */
+static struct led_trigger vt_led_triggers[LED_CNT] = {
+#define DEFINE_INPUT_LED_TRIGGER(vt_led, nam) \
+	[vt_led] = { \
+		.name = "vt-"nam, \
+		.activate = vt_led_trigger_activate, \
+	}
+	DEFINE_INPUT_LED_TRIGGER(LED_NUML, "numl"),
+	DEFINE_INPUT_LED_TRIGGER(LED_CAPSL, "capsl"),
+	DEFINE_INPUT_LED_TRIGGER(LED_SCROLLL, "scrolll"),
+	DEFINE_INPUT_LED_TRIGGER(LED_COMPOSE, "compose"),
+	DEFINE_INPUT_LED_TRIGGER(LED_KANA, "kana"),
+	DEFINE_INPUT_LED_TRIGGER(LED_SLEEP, "sleep"),
+	DEFINE_INPUT_LED_TRIGGER(LED_SUSPEND, "suspend"),
+	DEFINE_INPUT_LED_TRIGGER(LED_MUTE, "mute"),
+	DEFINE_INPUT_LED_TRIGGER(LED_MISC, "misc"),
+	DEFINE_INPUT_LED_TRIGGER(LED_MAIL, "mail"),
+	DEFINE_INPUT_LED_TRIGGER(LED_CHARGING, "charging"),
+};
+
+/* Lock for registration coherency */
+static DEFINE_MUTEX(vt_led_registered_lock);
+
+/* Which VT LED classes and triggers are registered */
+static unsigned long vt_led_registered[BITS_TO_LONGS(LED_CNT)];
+
+/* Number of input devices having each LED */
+static int vt_led_references[LED_CNT];
+
+static int vt_led_state[LED_CNT];
+static struct work_struct vt_led_work[LED_CNT];
+
+static void vt_led_cb(struct work_struct *work)
+{
+	int led = work - vt_led_work;
+
+	led_trigger_event(&vt_led_triggers[led], vt_led_state[led]);
+}
+
+/* VT LED state change, tell the VT trigger.  */
+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]);
+}
+
+/* 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;
+
+	input_event(dev, EV_LED, led, !!brightness);
+	input_event(dev, EV_SYN, SYN_REPORT, 0);
+}
+
+/* Keyboard hotplug, initialize its LED status */
+static void vt_led_trigger_activate(struct led_classdev *cdev)
+{
+	struct led_trigger *trigger = cdev->trigger;
+	int led = trigger - vt_led_triggers;
+
+	if (cdev->brightness_set)
+		cdev->brightness_set(cdev, vt_leds[led].brightness);
+}
+
+/* Free led stuff 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;
+		if (leds) {
+			int i;
+			for (i = 0; i < LED_CNT; i++)
+				kfree(leds[i].name);
+			kfree(leds);
+			dev->leds = NULL;
+		}
+	}
+}
+
+/* A new input device with potential LEDs to connect.  */
+int input_led_connect(struct input_dev *dev)
+{
+	int i, error = 0;
+	struct led_classdev *leds;
+
+	dev->leds = leds = kcalloc(LED_CNT, sizeof(*leds), GFP_KERNEL);
+	if (!dev->leds)
+		return -ENOMEM;
+
+	/* lazily register missing VT LEDs */
+	mutex_lock(&vt_led_registered_lock);
+	for (i = 0; i < LED_CNT; i++)
+		if (vt_leds[i].name && test_bit(i, dev->ledbit)) {
+			if (!vt_led_references[i]) {
+				led_trigger_register(&vt_led_triggers[i]);
+				/* This keyboard is first to have led i,
+				 * try to register it */
+				if (!led_classdev_register(NULL, &vt_leds[i]))
+					vt_led_references[i] = 1;
+				else
+					led_trigger_unregister(&vt_led_triggers[i]);
+			} else
+				vt_led_references[i]++;
+		}
+	mutex_unlock(&vt_led_registered_lock);
+
+	/* and register this device's LEDs */
+	for (i = 0; i < LED_CNT; i++)
+		if (vt_leds[i].name && test_bit(i, dev->ledbit)) {
+			leds[i].name = kasprintf(GFP_KERNEL, "%s::%s",
+						dev_name(&dev->dev),
+						vt_led_names[i]);
+			if (!leds[i].name) {
+				error = -ENOMEM;
+				goto err;
+			}
+			leds[i].max_brightness = 1;
+			leds[i].brightness_set = perdevice_input_led_set;
+			leds[i].default_trigger = vt_led_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;
+			perdevice_input_led_set(&leds[i],
+					vt_leds[i].brightness);
+		}
+
+	return 0;
+
+err:
+	input_led_delete(dev);
+	return error;
+}
+
+/*
+ * Disconnected input device. Clean it, and deregister now-useless VT LEDs
+ * and triggers.
+ */
+void input_led_disconnect(struct input_dev *dev)
+{
+	int i;
+	struct led_classdev *leds = dev->leds;
+
+	for (i = 0; i < LED_CNT; i++)
+		if (leds[i].name)
+			led_classdev_unregister(&leds[i]);
+
+	input_led_delete(dev);
+
+	mutex_lock(&vt_led_registered_lock);
+	for (i = 0; i < LED_CNT; i++) {
+		if (!vt_leds[i].name || !test_bit(i, dev->ledbit))
+			continue;
+
+		vt_led_references[i]--;
+		if (vt_led_references[i]) {
+			/* Still some devices needing it */
+			continue;
+		}
+
+		led_classdev_unregister(&vt_leds[i]);
+		led_trigger_unregister(&vt_led_triggers[i]);
+		clear_bit(i, vt_led_registered);
+	}
+	mutex_unlock(&vt_led_registered_lock);
+}
+
+void __init input_led_init(void)
+{
+	unsigned i;
+
+	for (i = 0; i < LED_CNT; i++)
+		INIT_WORK(&vt_led_work[i], vt_led_cb);
+}
+
+void __exit input_led_exit(void)
+{
+	unsigned i;
+
+	for (i = 0; i < LED_CNT; i++)
+		cancel_work_sync(&vt_led_work[i]);
+}

Re: [PATCHv4] INPUT: Route keyboard LEDs through the generic LEDs layer.

[John Crispin]

Hi

On 10/12/2014 02:02, Samuel Thibault wrote:
> This permits to reassign keyboard LEDs to something else than keyboard "leds"
> state, by adding keyboard led and modifier triggers connected to a series
> of VT input LEDs, themselves connected to VT input triggers, which
> per-input device LEDs use by default.  Userland can thus easily change the LED
> behavior of (a priori) all input devices, or of particular input devices.
> 
> This also 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@mokafive.com: Rebased to 3.2-rc1 or so, cleaned up some includes, and fixed some constants]
> [blogic@openwrt.org: CONFIG_INPUT_LEDS stubs should be static inline]
> [akpm@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>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
> Tested-by: Pavel Machek <pavel@ucw.cz>
> Acked-by: Peter Korsgaard <jacmet@sunsite.dk>
> Signed-off-by: John Crispin <blogic@openwrt.org>

I am not sure why my SoB was added. I originally sent a trivial fix up
for a header file as linux-next was not building (this was a year or
more ago). I never reviewed this patch nor have I tested it and I
certainly was not involved in the development. the patch simply broke
the compile of the Mips based Wifi and DSL SoCs that i maitain.

the code I touched is marked below

[snip]

> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -79,6 +79,7 @@ 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: leds 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 +165,8 @@ struct input_dev {
>  	unsigned long snd[BITS_TO_LONGS(SND_CNT)];
>  	unsigned long sw[BITS_TO_LONGS(SW_CNT)];
>  
> +	struct led_classdev *leds;
> +
>  	int (*open)(struct input_dev *dev);
>  	void (*close)(struct input_dev *dev);
>  	int (*flush)(struct input_dev *dev, struct file *file);
> @@ -531,4 +534,29 @@ 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
> +
> +void input_led_init(void);
> +void input_led_exit(void);
> +
> +int input_led_connect(struct input_dev *dev);
> +void input_led_disconnect(struct input_dev *dev);
> +
> +#else
> +
> +static inline void input_led_init(void) { }
> +
> +static inline void input_led_exit(void) { }
> +
> +static inline int input_led_connect(struct input_dev *dev)
> +{
> +	return 0;
> +}
> +
> +static inline void input_led_disconnect(struct input_dev *dev)
> +{
> +}
> +
> +#endif
> +

this #else part was added by me to make sure that linux-next was
building again. this really does not qualify my SoB being added.

	John















>  #endif
> --- /dev/null
> +++ b/drivers/input/leds.c
> @@ -0,0 +1,272 @@
> +/*
> + * 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.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/leds.h>
> +#include <linux/input.h>
> +
> +/*
> + * Keyboard LEDs are propagated by default like the following example:
> + *
> + * VT keyboard numlock trigger
> + * -> vt::numl VT LED
> + * -> vt-numl VT trigger
> + * -> per-device inputX::numl LED
> + *
> + * Userland can however choose the trigger for the vt::numl LED, or
> + * independently choose the trigger for any inputx::numl LED.
> + *
> + *
> + * VT LED classes and triggers are registered on-demand according to
> + * existing LED devices
> + */
> +
> +/* Handler for VT LEDs, just triggers the corresponding VT trigger. */
> +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 const char *const vt_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",
> +};
> +/* Handler for hotplug initialization */
> +static void vt_led_trigger_activate(struct led_classdev *cdev);
> +/* VT triggers */
> +static struct led_trigger vt_led_triggers[LED_CNT] = {
> +#define DEFINE_INPUT_LED_TRIGGER(vt_led, nam) \
> +	[vt_led] = { \
> +		.name = "vt-"nam, \
> +		.activate = vt_led_trigger_activate, \
> +	}
> +	DEFINE_INPUT_LED_TRIGGER(LED_NUML, "numl"),
> +	DEFINE_INPUT_LED_TRIGGER(LED_CAPSL, "capsl"),
> +	DEFINE_INPUT_LED_TRIGGER(LED_SCROLLL, "scrolll"),
> +	DEFINE_INPUT_LED_TRIGGER(LED_COMPOSE, "compose"),
> +	DEFINE_INPUT_LED_TRIGGER(LED_KANA, "kana"),
> +	DEFINE_INPUT_LED_TRIGGER(LED_SLEEP, "sleep"),
> +	DEFINE_INPUT_LED_TRIGGER(LED_SUSPEND, "suspend"),
> +	DEFINE_INPUT_LED_TRIGGER(LED_MUTE, "mute"),
> +	DEFINE_INPUT_LED_TRIGGER(LED_MISC, "misc"),
> +	DEFINE_INPUT_LED_TRIGGER(LED_MAIL, "mail"),
> +	DEFINE_INPUT_LED_TRIGGER(LED_CHARGING, "charging"),
> +};
> +
> +/* Lock for registration coherency */
> +static DEFINE_MUTEX(vt_led_registered_lock);
> +
> +/* Which VT LED classes and triggers are registered */
> +static unsigned long vt_led_registered[BITS_TO_LONGS(LED_CNT)];
> +
> +/* Number of input devices having each LED */
> +static int vt_led_references[LED_CNT];
> +
> +static int vt_led_state[LED_CNT];
> +static struct work_struct vt_led_work[LED_CNT];
> +
> +static void vt_led_cb(struct work_struct *work)
> +{
> +	int led = work - vt_led_work;
> +
> +	led_trigger_event(&vt_led_triggers[led], vt_led_state[led]);
> +}
> +
> +/* VT LED state change, tell the VT trigger.  */
> +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]);
> +}
> +
> +/* 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;
> +
> +	input_event(dev, EV_LED, led, !!brightness);
> +	input_event(dev, EV_SYN, SYN_REPORT, 0);
> +}
> +
> +/* Keyboard hotplug, initialize its LED status */
> +static void vt_led_trigger_activate(struct led_classdev *cdev)
> +{
> +	struct led_trigger *trigger = cdev->trigger;
> +	int led = trigger - vt_led_triggers;
> +
> +	if (cdev->brightness_set)
> +		cdev->brightness_set(cdev, vt_leds[led].brightness);
> +}
> +
> +/* Free led stuff 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;
> +		if (leds) {
> +			int i;
> +			for (i = 0; i < LED_CNT; i++)
> +				kfree(leds[i].name);
> +			kfree(leds);
> +			dev->leds = NULL;
> +		}
> +	}
> +}
> +
> +/* A new input device with potential LEDs to connect.  */
> +int input_led_connect(struct input_dev *dev)
> +{
> +	int i, error = 0;
> +	struct led_classdev *leds;
> +
> +	dev->leds = leds = kcalloc(LED_CNT, sizeof(*leds), GFP_KERNEL);
> +	if (!dev->leds)
> +		return -ENOMEM;
> +
> +	/* lazily register missing VT LEDs */
> +	mutex_lock(&vt_led_registered_lock);
> +	for (i = 0; i < LED_CNT; i++)
> +		if (vt_leds[i].name && test_bit(i, dev->ledbit)) {
> +			if (!vt_led_references[i]) {
> +				led_trigger_register(&vt_led_triggers[i]);
> +				/* This keyboard is first to have led i,
> +				 * try to register it */
> +				if (!led_classdev_register(NULL, &vt_leds[i]))
> +					vt_led_references[i] = 1;
> +				else
> +					led_trigger_unregister(&vt_led_triggers[i]);
> +			} else
> +				vt_led_references[i]++;
> +		}
> +	mutex_unlock(&vt_led_registered_lock);
> +
> +	/* and register this device's LEDs */
> +	for (i = 0; i < LED_CNT; i++)
> +		if (vt_leds[i].name && test_bit(i, dev->ledbit)) {
> +			leds[i].name = kasprintf(GFP_KERNEL, "%s::%s",
> +						dev_name(&dev->dev),
> +						vt_led_names[i]);
> +			if (!leds[i].name) {
> +				error = -ENOMEM;
> +				goto err;
> +			}
> +			leds[i].max_brightness = 1;
> +			leds[i].brightness_set = perdevice_input_led_set;
> +			leds[i].default_trigger = vt_led_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;
> +			perdevice_input_led_set(&leds[i],
> +					vt_leds[i].brightness);
> +		}
> +
> +	return 0;
> +
> +err:
> +	input_led_delete(dev);
> +	return error;
> +}
> +
> +/*
> + * Disconnected input device. Clean it, and deregister now-useless VT LEDs
> + * and triggers.
> + */
> +void input_led_disconnect(struct input_dev *dev)
> +{
> +	int i;
> +	struct led_classdev *leds = dev->leds;
> +
> +	for (i = 0; i < LED_CNT; i++)
> +		if (leds[i].name)
> +			led_classdev_unregister(&leds[i]);
> +
> +	input_led_delete(dev);
> +
> +	mutex_lock(&vt_led_registered_lock);
> +	for (i = 0; i < LED_CNT; i++) {
> +		if (!vt_leds[i].name || !test_bit(i, dev->ledbit))
> +			continue;
> +
> +		vt_led_references[i]--;
> +		if (vt_led_references[i]) {
> +			/* Still some devices needing it */
> +			continue;
> +		}
> +
> +		led_classdev_unregister(&vt_leds[i]);
> +		led_trigger_unregister(&vt_led_triggers[i]);
> +		clear_bit(i, vt_led_registered);
> +	}
> +	mutex_unlock(&vt_led_registered_lock);
> +}
> +
> +void __init input_led_init(void)
> +{
> +	unsigned i;
> +
> +	for (i = 0; i < LED_CNT; i++)
> +		INIT_WORK(&vt_led_work[i], vt_led_cb);
> +}
> +
> +void __exit input_led_exit(void)
> +{
> +	unsigned i;
> +
> +	for (i = 0; i < LED_CNT; i++)
> +		cancel_work_sync(&vt_led_work[i]);
> +}
> 

Re: [PATCHv4] INPUT: Route keyboard LEDs through the generic LEDs layer.

[Andrew Morton]

On Wed, 10 Dec 2014 08:01:00 +0100 John Crispin <blogic@openwrt.org> wrote:

> Hi
> 
> On 10/12/2014 02:02, Samuel Thibault wrote:
> > This permits to reassign keyboard LEDs to something else than keyboard "leds"
> > state, by adding keyboard led and modifier triggers connected to a series
> > of VT input LEDs, themselves connected to VT input triggers, which
> > per-input device LEDs use by default.  Userland can thus easily change the LED
> > behavior of (a priori) all input devices, or of particular input devices.
> > 
> > This also 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@mokafive.com: Rebased to 3.2-rc1 or so, cleaned up some includes, and fixed some constants]
> > [blogic@openwrt.org: CONFIG_INPUT_LEDS stubs should be static inline]
> > [akpm@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>
> > Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
> > Tested-by: Pavel Machek <pavel@ucw.cz>
> > Acked-by: Peter Korsgaard <jacmet@sunsite.dk>
> > Signed-off-by: John Crispin <blogic@openwrt.org>
> 
> I am not sure why my SoB was added. I originally sent a trivial fix up
> for a header file as linux-next was not building (this was a year or
> more ago). I never reviewed this patch nor have I tested it and I
> certainly was not involved in the development. the patch simply broke
> the compile of the Mips based Wifi and DSL SoCs that i maitain.
> 
> ...
>
> this #else part was added by me to make sure that linux-next was
> building again. this really does not qualify my SoB being added.
> 

The SOB is appropriate - you made a change to the code and (presumably)
attached your SOB to that.  The change is briefly described there:

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

Re: [PATCHv4] INPUT: Route keyboard LEDs through the generic LEDs layer.

[Andrew Morton]

On Wed, 10 Dec 2014 02:02:14 +0100 Samuel Thibault <samuel.thibault@ens-lyon.org> wrote:

> This permits to reassign keyboard LEDs to something else than keyboard "leds"
> state, by adding keyboard led and modifier triggers connected to a series
> of VT input LEDs, themselves connected to VT input triggers, which
> per-input device LEDs use by default.  Userland can thus easily change the LED
> behavior of (a priori) all input devices, or of particular input devices.
> 
> This also 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@mokafive.com: Rebased to 3.2-rc1 or so, cleaned up some includes, and fixed some constants]
> [blogic@openwrt.org: CONFIG_INPUT_LEDS stubs should be static inline]
> [akpm@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>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
> Tested-by: Pavel Machek <pavel@ucw.cz>
> 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:
> - Use kcalloc instead of kzalloc
> - to avoid any mutex order violation, defer LED update into a work callback.

Confused.  This patch is identical to the one that's presently in -mm.

Re: [PATCHv4] INPUT: Route keyboard LEDs through the generic LEDs layer.

[Samuel Thibault]

Andrew Morton, le Fri 19 Dec 2014 14:46:41 -0800, a écrit :
> > Changed in this version:
> > - Use kcalloc instead of kzalloc
> > - to avoid any mutex order violation, defer LED update into a work callback.
> 
> Confused.  This patch is identical to the one that's presently in -mm.

Well, yes: I'm submitting the latest version of this patch to Dmitry,
i.e. including the fixes that happened in the -mm tree.

Samuel

Re: [PATCHv4] INPUT: Route keyboard LEDs through the generic LEDs layer.

[Pavel Machek]

On Fri 2014-12-19 23:59:33, Samuel Thibault wrote:
> Andrew Morton, le Fri 19 Dec 2014 14:46:41 -0800, a écrit :
> > > Changed in this version:
> > > - Use kcalloc instead of kzalloc
> > > - to avoid any mutex order violation, defer LED update into a work callback.
> > 
> > Confused.  This patch is identical to the one that's presently in -mm.
> 
> Well, yes: I'm submitting the latest version of this patch to Dmitry,
> i.e. including the fixes that happened in the -mm tree.

Andrew, do you think you could send the patch to Dmitry? He has been
ignoring it for way too long... and it should be slightly harder for
him to ignore patch from you...

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

Re: [PATCHv4] INPUT: Route keyboard LEDs through the generic LEDs layer.

[Dmitry Torokhov]

On Sat, Dec 20, 2014 at 12:02:53AM +0100, Pavel Machek wrote:
> On Fri 2014-12-19 23:59:33, Samuel Thibault wrote:
> > Andrew Morton, le Fri 19 Dec 2014 14:46:41 -0800, a écrit :
> > > > Changed in this version:
> > > > - Use kcalloc instead of kzalloc
> > > > - to avoid any mutex order violation, defer LED update into a work callback.
> > > 
> > > Confused.  This patch is identical to the one that's presently in -mm.
> > 
> > Well, yes: I'm submitting the latest version of this patch to Dmitry,
> > i.e. including the fixes that happened in the -mm tree.
> 
> Andrew, do you think you could send the patch to Dmitry? He has been
> ignoring it for way too long... and it should be slightly harder for
> him to ignore patch from you...

I'll keep ignoring the patch until someone will figure out how to make new way
of handling LEDs play well with the current way (via writing to evdev). That
means not treating access to evdev as "raw" and ignoring LED trigger remapping
done in LED layer. So far I just heard that it is not a problem and nobody uses
evdev interface...

Thanks.

-- 
Dmitry

Re: [PATCHv4] INPUT: Route keyboard LEDs through the generic LEDs layer.

[Samuel Thibault]

Dmitry Torokhov, le Fri 19 Dec 2014 15:07:21 -0800, a écrit :
> On Sat, Dec 20, 2014 at 12:02:53AM +0100, Pavel Machek wrote:
> > On Fri 2014-12-19 23:59:33, Samuel Thibault wrote:
> > > Andrew Morton, le Fri 19 Dec 2014 14:46:41 -0800, a écrit :
> > > > > Changed in this version:
> > > > > - Use kcalloc instead of kzalloc
> > > > > - to avoid any mutex order violation, defer LED update into a work callback.
> > > > 
> > > > Confused.  This patch is identical to the one that's presently in -mm.
> > > 
> > > Well, yes: I'm submitting the latest version of this patch to Dmitry,
> > > i.e. including the fixes that happened in the -mm tree.
> > 
> > Andrew, do you think you could send the patch to Dmitry? He has been
> > ignoring it for way too long... and it should be slightly harder for
> > him to ignore patch from you...
> 
> I'll keep ignoring the patch until someone will figure out how to make new way
> of handling LEDs play well with the current way (via writing to evdev). That
> means not treating access to evdev as "raw" and ignoring LED trigger remapping
> done in LED layer. So far I just heard that it is not a problem and nobody uses
> evdev interface...

?! There must have been a misunderstanding, and the absence of response
to my last mail (April 17th) didn't help to understand what you were
expecting.  I'm sorry, but things can't work if there are not efforts to
discuss on both sides.

I never said that nobody uses the evdev interface.

I just said that I wasn't sure to understand the scenario you were
proposing.  I was asking for details, not barring discussion...  I'm
fine with reworking my patch yet again, but I can't do that if I don't
understand precisely the use-case you are expecting.

So, to recap what we apparently want:

(1) console-setup wants to be able to change which keyboard modifier is
shown on the capslock LED
(2) embedded people want to be able to plug the keyboard LED state to misc
LEDs
- people will quickly want to be able to repurpose their keyboard LEDs,
for instance:
  (3) make the numlock LED show disk activity
  (4) exchange capslock and numlock LEDs for some reason.

And the question is: how all of that should be mixed?

ATM, we have this:

kbd state -> kbd led -> input led
  ^^            ^^          ^^
KDSKBLED     KDSETLED     EV_LED

What I had proposed and implemented in my current patch is the following
pipeline:

kbd state -> kbd led -> kbd trigger -> vt led -> vt trigger -> input led
  ^^           ^^                   ^^                      ^^        ^^
KDSKBLED   KDSETLED   vt::capsl/trigger   input*::capsl/trigger   EV_LED

Which leaves the evdev users keeping raw control, unmodified.  In
your mail from April 12th, you seemed to want that (4) have effect on
evdev, I asked for confirmation, but never got an answer.  Your above
paragraph seems to clearly show that you want evdev users to get at
least the (4) remapping.  That wasn't obvious to me, because I had
always considered evdev as a way to drive physical hardware, without any
software abstraction.  But OK, let's go with that.

AIUI, what you would be OK with is the following:

kbd state -> kbd trigger -> kbd led -> input led -> input rawled
   ^^                    ^^   ^^         ^^      ^^
KDSKBLED  vt::capsl/trigger  KDSETLED  EV_LED  input*::capsl/trigger

Which, instead of introducing a "VT led", separates the input led into
the part exposed to other systems (kbd, evdev) and the lowlevel part.  I
have here also fixed the behavior of KDSETLED: in my patch proposal it
would get redirected by console-setup, which I believe we don't want.

So for (1) console-setup would use vt::capsl/trigger to make it use
another modifier, and this would only have effect for actual capslock
keypresses and KDSKBLED ioctls, which I believe is what we want
(again, correct me if I'm wrong, but please detail, otherwise I can't
know how I'm wrong); for (2) people would use input*::capsl/trigger*,
which would now indeed have the nice effect of making e.g. Xorg
getting the good behavior along the way; for (3) people would also use
input*::capsl/trigger*, and that would now indeed prevent spurious
changes from e.g. Xorg.

Now, there is (4).  I don't know why people would want that, but well,
you mentioned it in some mail, so why not.  Ideally it would go through
input*::capsl/trigger*, but what to put there?  In my earlier proposal,
that would have been vt-capsl, but you want the exchange to work on
evdev access too.  A possibility would be to introduce a LED trigger for
each input LED, like this:

kbd state -> kbd trigger -> kbd led -> input led -> input led trigger -> input rawled
   ^^                    ^^   ^^          ^^                          ^^        
KDSKBLED  vt::capsl/trigger  KDSETLED   EV_LED              input*::capsl/trigger

Which seems quite hairy: for each input LED we'd create both a trigger
(gathering both the kbd led events and the evdev led events) and a LED
(the actual physical LED), but why not.

To exchange capsl and numl, one would then have to do something like
this:

for i in input*
do
	echo $i-numl > /sys/class/leds/$i::capsl/trigger
	echo $i-capsl > /sys/class/leds/$i::numl/trigger
done

That also opens the possibility of odd things like this:

echo input0-numl > /sys/class/leds/input1::capsl/trigger

i.e. all evdev numl events on input0 will actually also drive input1's
capsl.  Why not.

Is it looking more like what you want?

Samuel

Re: [PATCHv4] INPUT: Route keyboard LEDs through the generic LEDs layer.

[Dmitry Torokhov]

On Sat, Dec 20, 2014 at 03:16:18AM +0100, Samuel Thibault wrote:
> Dmitry Torokhov, le Fri 19 Dec 2014 15:07:21 -0800, a écrit :
> > On Sat, Dec 20, 2014 at 12:02:53AM +0100, Pavel Machek wrote:
> > > On Fri 2014-12-19 23:59:33, Samuel Thibault wrote:
> > > > Andrew Morton, le Fri 19 Dec 2014 14:46:41 -0800, a écrit :
> > > > > > Changed in this version:
> > > > > > - Use kcalloc instead of kzalloc
> > > > > > - to avoid any mutex order violation, defer LED update into a work callback.
> > > > > 
> > > > > Confused.  This patch is identical to the one that's presently in -mm.
> > > > 
> > > > Well, yes: I'm submitting the latest version of this patch to Dmitry,
> > > > i.e. including the fixes that happened in the -mm tree.
> > > 
> > > Andrew, do you think you could send the patch to Dmitry? He has been
> > > ignoring it for way too long... and it should be slightly harder for
> > > him to ignore patch from you...
> > 
> > I'll keep ignoring the patch until someone will figure out how to make new way
> > of handling LEDs play well with the current way (via writing to evdev). That
> > means not treating access to evdev as "raw" and ignoring LED trigger remapping
> > done in LED layer. So far I just heard that it is not a problem and nobody uses
> > evdev interface...
> 
> ?! There must have been a misunderstanding, and the absence of response
> to my last mail (April 17th) didn't help to understand what you were
> expecting.  I'm sorry, but things can't work if there are not efforts to
> discuss on both sides.
> 
> I never said that nobody uses the evdev interface.

OK, them I must have misunderstood you indeed.

> 
> I just said that I wasn't sure to understand the scenario you were
> proposing.  I was asking for details, not barring discussion...  I'm
> fine with reworking my patch yet again, but I can't do that if I don't
> understand precisely the use-case you are expecting.
> 
> So, to recap what we apparently want:
> 
> (1) console-setup wants to be able to change which keyboard modifier is
> shown on the capslock LED
> (2) embedded people want to be able to plug the keyboard LED state to misc
> LEDs
> - people will quickly want to be able to repurpose their keyboard LEDs,
> for instance:
>   (3) make the numlock LED show disk activity
>   (4) exchange capslock and numlock LEDs for some reason.
> 
> And the question is: how all of that should be mixed?
> 
> ATM, we have this:
> 
> kbd state -> kbd led -> input led
>   ^^            ^^          ^^
> KDSKBLED     KDSETLED     EV_LED
> 
> What I had proposed and implemented in my current patch is the following
> pipeline:
> 
> kbd state -> kbd led -> kbd trigger -> vt led -> vt trigger -> input led
>   ^^           ^^                   ^^                      ^^        ^^
> KDSKBLED   KDSETLED   vt::capsl/trigger   input*::capsl/trigger   EV_LED
> 
> Which leaves the evdev users keeping raw control, unmodified.  In
> your mail from April 12th, you seemed to want that (4) have effect on
> evdev, I asked for confirmation, but never got an answer.  Your above
> paragraph seems to clearly show that you want evdev users to get at
> least the (4) remapping.  That wasn't obvious to me, because I had
> always considered evdev as a way to drive physical hardware, without any
> software abstraction.  But OK, let's go with that.
> 
> AIUI, what you would be OK with is the following:
> 
> kbd state -> kbd trigger -> kbd led -> input led -> input rawled
>    ^^                    ^^   ^^         ^^      ^^
> KDSKBLED  vt::capsl/trigger  KDSETLED  EV_LED  input*::capsl/trigger
> 
> Which, instead of introducing a "VT led", separates the input led into
> the part exposed to other systems (kbd, evdev) and the lowlevel part.  I
> have here also fixed the behavior of KDSETLED: in my patch proposal it
> would get redirected by console-setup, which I believe we don't want.
> 
> So for (1) console-setup would use vt::capsl/trigger to make it use
> another modifier, and this would only have effect for actual capslock
> keypresses and KDSKBLED ioctls, which I believe is what we want
> (again, correct me if I'm wrong, but please detail, otherwise I can't
> know how I'm wrong); for (2) people would use input*::capsl/trigger*,
> which would now indeed have the nice effect of making e.g. Xorg
> getting the good behavior along the way; for (3) people would also use
> input*::capsl/trigger*, and that would now indeed prevent spurious
> changes from e.g. Xorg.
> 
> Now, there is (4).  I don't know why people would want that, but well,
> you mentioned it in some mail, so why not.

Reasoning is simple: your keyboard might lack a certain LED but you
might want to know the state. So you'd repurpose one led for another. It
is not really any different from case (2).

> Ideally it would go through
> input*::capsl/trigger*, but what to put there?  In my earlier proposal,
> that would have been vt-capsl, but you want the exchange to work on
> evdev access too.  A possibility would be to introduce a LED trigger for
> each input LED, like this:
> 
> kbd state -> kbd trigger -> kbd led -> input led -> input led trigger -> input rawled
>    ^^                    ^^   ^^          ^^                          ^^        
> KDSKBLED  vt::capsl/trigger  KDSETLED   EV_LED              input*::capsl/trigger
> 
> Which seems quite hairy: for each input LED we'd create both a trigger
> (gathering both the kbd led events and the evdev led events) and a LED
> (the actual physical LED), but why not.
> 
> To exchange capsl and numl, one would then have to do something like
> this:
> 
> for i in input*
> do
> 	echo $i-numl > /sys/class/leds/$i::capsl/trigger
> 	echo $i-capsl > /sys/class/leds/$i::numl/trigger
> done
> 
> That also opens the possibility of odd things like this:
> 
> echo input0-numl > /sys/class/leds/input1::capsl/trigger
> 
> i.e. all evdev numl events on input0 will actually also drive input1's
> capsl.  Why not.
> 
> Is it looking more like what you want?

Yes, I believe it is.

Also, can you please split off input core changes from tty changes in
your patch? I think it will be easier for me to digest it that way,
especially since LED state propagation between keyboards seems to be
broken even in console at the moment.

Thank you.

-- 
Dmitry

Re: [PATCHv4] INPUT: Route keyboard LEDs through the generic LEDs layer.

[Samuel Thibault]

Dmitry Torokhov, le Fri 19 Dec 2014 23:27:15 -0800, a écrit :
> Yes, I believe it is.

Good :)

> Also, can you please split off input core changes from tty changes in
> your patch?

Yes.  I this new formulation they become really completely separate
matters.

Samuel