Messages in this thread | | | Date | Sun, 4 Jan 2015 15:28:38 -0800 | From | Dmitry Torokhov <> | Subject | Re: [PATCHv5 2/2] INPUT: Introduce generic trigger/LED pairs to input LEDs |
| |
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@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> > 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
| |