From: Hans de Goede <hdegoede@redhat.com>
To: Hans de Goede <hdegoede@redhat.com>, Pavel Machek <pavel@ucw.cz>,
Lee Jones <lee@kernel.org>
Cc: Kate Hsuan <hpa@redhat.com>, linux-leds@vger.kernel.org
Subject: Re: [PATCH 1/1] leds: trigger: Add new LED Input events trigger
Date: Tue, 7 May 2024 11:16:04 +0200 [thread overview]
Message-ID: <5261419e-d788-4a38-86e6-d7094fa1a242@redhat.com> (raw)
In-Reply-To: <20240505123240.14955-2-hdegoede@redhat.com>
Hi All,
On 5/5/24 2:32 PM, Hans de Goede wrote:
> Add a new trigger which turns LEDs on when there is input
> (/dev/input/event*) activity and turns them back off again after there has
> been no activity for 5 seconds.
>
> This is primarily intended to control LED devices which are a backlight for
> capacitive touch-buttons, such as e.g. the menu / home / back buttons found
> on the bottom bezel of many somewhat older smartphones and tablets.
>
> This can also be used to turn on the keyboard backlight LED on input
> events and turn the keyboard backlight off again when idle.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Self-nack for this patch. I still believe the basic idea of having such
a trigger is good. But the way how this implementation uses
the trigger->led_cdevs list is racy and that list is better left as
a private implementation detail of drivers/leds/led-trigger.c and not
used directly by triggers.
So I'm going to rework this for v2 so that it no longer loops over
trigger->led_cdevs list.
Regards,
Hans
> ---
> drivers/leds/trigger/Kconfig | 16 ++
> drivers/leds/trigger/Makefile | 1 +
> drivers/leds/trigger/ledtrig-input-events.c | 252 ++++++++++++++++++++
> 3 files changed, 269 insertions(+)
> create mode 100644 drivers/leds/trigger/ledtrig-input-events.c
>
> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index d11d80176fc0..809ffba0cd6a 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -152,4 +152,20 @@ config LEDS_TRIGGER_TTY
>
> When build as a module this driver will be called ledtrig-tty.
>
> +config LEDS_TRIGGER_INPUT_EVENTS
> + tristate "LED Input events trigger"
> + depends on INPUT
> + help
> + Turn LEDs on when there is input (/dev/input/event*) activity and turn
> + them back off again after there has been no activity for 5 seconds.
> +
> + This is primarily intended to control LEDs which are a backlight for
> + capacitive touch-buttons, such as e.g. the menu / home / back buttons
> + found on the bottom bezel of many older smartphones and tablets.
> +
> + This can also be used to turn on the keyboard backlight LED on
> + input events and turn the keyboard backlight off again when idle.
> +
> + When build as a module this driver will be called ledtrig-input-events.
> +
> endif # LEDS_TRIGGERS
> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
> index 25c4db97cdd4..f78a3077efc7 100644
> --- a/drivers/leds/trigger/Makefile
> +++ b/drivers/leds/trigger/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_LEDS_TRIGGER_NETDEV) += ledtrig-netdev.o
> obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o
> obj-$(CONFIG_LEDS_TRIGGER_AUDIO) += ledtrig-audio.o
> obj-$(CONFIG_LEDS_TRIGGER_TTY) += ledtrig-tty.o
> +obj-$(CONFIG_LEDS_TRIGGER_INPUT_EVENTS) += ledtrig-input-events.o
> diff --git a/drivers/leds/trigger/ledtrig-input-events.c b/drivers/leds/trigger/ledtrig-input-events.c
> new file mode 100644
> index 000000000000..cb86d1afeab4
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-input-events.c
> @@ -0,0 +1,252 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Input Events LED trigger
> + *
> + * Copyright (C) 2024 Hans de Goede <hansg@kernel.org>
> + * Partially based on Atsushi Nemoto's ledtrig-heartbeat.c.
> + */
> +
> +#include <linux/input.h>
> +#include <linux/jiffies.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/workqueue.h>
> +#include "../leds.h"
> +
> +#define DEFAULT_LED_OFF_DELAY_MS 5000
> +
> +struct input_events_data {
> + struct delayed_work work;
> + spinlock_t lock;
> + struct led_classdev *led_cdev;
> + int led_cdev_saved_flags;
> + /* To avoid repeatedly setting the brightness while there is events */
> + bool led_on;
> + unsigned long led_off_time;
> + unsigned long led_off_delay;
> +};
> +
> +static void led_input_events_work(struct work_struct *work)
> +{
> + struct input_events_data *data =
> + container_of(work, struct input_events_data, work.work);
> +
> + spin_lock_irq(&data->lock);
> +
> + /*
> + * This time_after_eq() check avoids a race where this work starts
> + * running before a new event pushed led_off_time back.
> + */
> + if (time_after_eq(jiffies, data->led_off_time)) {
> + led_set_brightness_nosleep(data->led_cdev, LED_OFF);
> + data->led_on = false;
> + }
> +
> + spin_unlock_irq(&data->lock);
> +}
> +
> +static ssize_t delay_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + struct input_events_data *input_events_data = led_trigger_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%lu\n", input_events_data->led_off_delay);
> +}
> +
> +static ssize_t delay_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t size)
> +{
> + struct input_events_data *input_events_data = led_trigger_get_drvdata(dev);
> + unsigned long delay;
> + int ret;
> +
> + ret = kstrtoul(buf, 0, &delay);
> + if (ret)
> + return ret;
> +
> + /* Clamp between 0.5 and 1000 seconds */
> + delay = clamp_val(delay, 500UL, 1000000UL);
> + input_events_data->led_off_delay = msecs_to_jiffies(delay);
> +
> + return size;
> +}
> +
> +static DEVICE_ATTR_RW(delay);
> +
> +static struct attribute *input_events_led_attrs[] = {
> + &dev_attr_delay.attr,
> + NULL
> +};
> +ATTRIBUTE_GROUPS(input_events_led);
> +
> +static int input_events_activate(struct led_classdev *led_cdev)
> +{
> + struct input_events_data *data;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + INIT_DELAYED_WORK(&data->work, led_input_events_work);
> + spin_lock_init(&data->lock);
> + data->led_cdev = led_cdev;
> + data->led_cdev_saved_flags = led_cdev->flags;
> + data->led_off_delay = msecs_to_jiffies(DEFAULT_LED_OFF_DELAY_MS);
> +
> + led_set_trigger_data(led_cdev, data);
> +
> + /*
> + * Use led_cdev->blink_brightness + LED_BLINK_SW flag so that sysfs
> + * brightness writes will change led_cdev->new_blink_brightness for
> + * configuring the on state brightness (like ledtrig-heartbeat).
> + */
> + if (!led_cdev->blink_brightness)
> + led_cdev->blink_brightness = led_cdev->max_brightness;
> +
> + set_bit(LED_BLINK_SW, &led_cdev->work_flags);
> + /* Turn LED off during suspend, original flags are restored on deactivate() */
> + led_cdev->flags |= LED_CORE_SUSPENDRESUME;
> +
> + /* Start with LED off */
> + led_set_brightness_nosleep(data->led_cdev, LED_OFF);
> + return 0;
> +}
> +
> +static void input_events_deactivate(struct led_classdev *led_cdev)
> +{
> + struct input_events_data *data = led_get_trigger_data(led_cdev);
> +
> + led_cdev->flags = data->led_cdev_saved_flags;
> + clear_bit(LED_BLINK_SW, &led_cdev->work_flags);
> + cancel_delayed_work_sync(&data->work);
> + kfree(data);
> +}
> +
> +static struct led_trigger input_events_led_trigger = {
> + .name = "input-events",
> + .activate = input_events_activate,
> + .deactivate = input_events_deactivate,
> + .groups = input_events_led_groups,
> +};
> +
> +static void input_events_event(struct input_handle *handle, unsigned int type,
> + unsigned int code, int data)
> +{
> + struct led_classdev *led_cdev;
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(led_cdev, &input_events_led_trigger.led_cdevs, trig_list) {
> + struct input_events_data *data = led_get_trigger_data(led_cdev);
> + unsigned long led_off_delay = READ_ONCE(data->led_off_delay);
> + unsigned long flags;
> +
> + if (test_and_clear_bit(LED_BLINK_BRIGHTNESS_CHANGE, &led_cdev->work_flags))
> + led_cdev->blink_brightness = led_cdev->new_blink_brightness;
> +
> + spin_lock_irqsave(&data->lock, flags);
> +
> + if (!data->led_on) {
> + led_set_brightness_nosleep(led_cdev, led_cdev->blink_brightness);
> + data->led_on = true;
> + }
> + data->led_off_time = jiffies + led_off_delay;
> +
> + spin_unlock_irqrestore(&data->lock, flags);
> +
> + mod_delayed_work(system_wq, &data->work, led_off_delay);
> + }
> + rcu_read_unlock();
> +}
> +
> +static int input_events_connect(struct input_handler *handler, struct input_dev *dev,
> + const struct input_device_id *id)
> +{
> + struct input_handle *handle;
> + int ret;
> +
> + handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL);
> + if (!handle)
> + return -ENOMEM;
> +
> + handle->dev = dev;
> + handle->handler = handler;
> + handle->name = "input-events";
> +
> + ret = input_register_handle(handle);
> + if (ret)
> + goto err_free_handle;
> +
> + ret = input_open_device(handle);
> + if (ret)
> + goto err_unregister_handle;
> +
> + return 0;
> +
> +err_unregister_handle:
> + input_unregister_handle(handle);
> +err_free_handle:
> + kfree(handle);
> + return ret;
> +}
> +
> +static void input_events_disconnect(struct input_handle *handle)
> +{
> + input_close_device(handle);
> + input_unregister_handle(handle);
> + kfree(handle);
> +}
> +
> +static const struct input_device_id input_events_ids[] = {
> + {
> + .flags = INPUT_DEVICE_ID_MATCH_EVBIT,
> + .evbit = { BIT_MASK(EV_KEY) },
> + },
> + {
> + .flags = INPUT_DEVICE_ID_MATCH_EVBIT,
> + .evbit = { BIT_MASK(EV_REL) },
> + },
> + {
> + .flags = INPUT_DEVICE_ID_MATCH_EVBIT,
> + .evbit = { BIT_MASK(EV_ABS) },
> + },
> + { }
> +};
> +
> +static struct input_handler input_events_handler = {
> + .name = "input-events",
> + .event = input_events_event,
> + .connect = input_events_connect,
> + .disconnect = input_events_disconnect,
> + .id_table = input_events_ids,
> +};
> +
> +static int __init input_events_init(void)
> +{
> + int ret;
> +
> + ret = led_trigger_register(&input_events_led_trigger);
> + if (ret)
> + return ret;
> +
> + ret = input_register_handler(&input_events_handler);
> + if (ret) {
> + led_trigger_unregister(&input_events_led_trigger);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void __exit input_events_exit(void)
> +{
> + input_unregister_handler(&input_events_handler);
> + led_trigger_unregister(&input_events_led_trigger);
> +}
> +
> +module_init(input_events_init);
> +module_exit(input_events_exit);
> +
> +MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
> +MODULE_DESCRIPTION("Input Events LED trigger");
> +MODULE_LICENSE("GPL");
prev parent reply other threads:[~2024-05-07 9:16 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-05 12:32 [PATCH 0/1] leds: trigger: Add new LED Input events trigger Hans de Goede
2024-05-05 12:32 ` [PATCH 1/1] " Hans de Goede
2024-05-07 9:16 ` Hans de Goede [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5261419e-d788-4a38-86e6-d7094fa1a242@redhat.com \
--to=hdegoede@redhat.com \
--cc=hpa@redhat.com \
--cc=lee@kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=pavel@ucw.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).