Message-ID: <1334983269.2435.90.camel@jerome.southpole.se>
Date:	Sat, 21 Apr 2012 06:41:09 +0200
From:	Jonas Bonn <jonas@...thpole.se>
To:	shuahkhan@...il.com
Cc:	Andrew Morton <akpm@...ux-foundation.org>, neilb@...e.de,
	LKML <linux-kernel@...r.kernel.org>,
	Richard Purdie <richard.purdie@...uxfoundation.org>
Subject: Re: [PATCH ] leds: add new transient trigger for one shot timer
 support


Hi Shuah,

A couple of questions:

i)  Why is it important for you to maintain the LED state?

ii)  Why do you need to be able to cancel the timer?

As I understand this trigger (and your use-case), this is about being
able to turn on a LED (signal) and forget about it, knowing that it will
be extinguished in due time without any intervention.  So I would leave
it at that; don't even present an option to intervene.

That said, there is already a way to intervene:  I can always write 0 to
'brightness' to extinguish the led immediately.

A couple more comments inline below.

/Jonas

On Thu, 2012-04-19 at 22:04 -0600, Shuah Khan wrote:
> This patch adds a new transient trigger for one shot timer support and is
> to be used for the following example use cases:
> 
> - Control of vibrate (phones, tablets etc.) hardware by userspace app.
> - Use of LED by userspace app as activity indicator.
> - Use of LED by userspace app as a kind of watchdog indicator -- as
>        long as the app is alive, it can keep the LED illuminated, if it dies
>        the LED will be extinguished automatically.
> - Use by any userspace app that needs a transient GPIO output
> 
> Transient trigger exports two attributes:
>        transient_enabled -     one shot timer enable mechanism.
>                                1 when enabled, 0 when disabled.
>                                enabled state indicates a timer
>                                with a value of transient_time running.
>                                disabled state indicates no active timer
>                                running.

You can already read the state of the LED... it's in the 'brightness'
property.  You don't need to be able to read it here, too.

>        transient_time -        one shot timer value. When transient_enabled
>                                is set, transient_time value is used to start
>                                a timer that runs once.

'duration' might be a better name.  What are the units?  I like to see
the units explicity in the property name so that I don't need to think:
'duration_msec' might be appropriate.

>        When timer expires transient_enabled goes back to disabled state,
>        transient_time is left at the set value to be used when transient
>        is enabled at a future time. This will allow user app to set the
>        time once and enable it to run it once for the specified value as
>        needed.
> 
> From 80a15b18f6758d2890dfa6b266f2ea4e30338e3c Mon Sep 17 00:00:00 2001
> From: Shuah Khan <shuahkhan@...il.com>
> Date: Thu, 19 Apr 2012 21:05:36 -0600
> Subject: [PATCH 2/2] leds: add new transient trigger for one shot timer
>  support
> 
> 
> Signed-off-by: Shuah Khan <shuahkhan@...il.com>
> ---
>  drivers/leds/Kconfig             |    8 ++
>  drivers/leds/Makefile            |    1 +
>  drivers/leds/ledtrig-transient.c |  246 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 255 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/leds/ledtrig-transient.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 589ba02..c6d8006 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -478,4 +478,12 @@ config LEDS_TRIGGER_DEFAULT_ON
>  comment "iptables trigger is under Netfilter config (LED target)"
>  	depends on LEDS_TRIGGERS
>  
> +config LEDS_TRIGGER_TRANSIENT
> +	tristate "LED Transient Trigger"
> +	depends on LEDS_TRIGGERS
> +	help
> +	  This allows one time enable of a transient state on GPIO/PWM based
> +	  hadrware.
> +	  If unsure, say Y.
> +
>  endif # NEW_LEDS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index fa0f428..48d5af7 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -57,3 +57,4 @@ obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT)	+= ledtrig-heartbeat.o
>  obj-$(CONFIG_LEDS_TRIGGER_BACKLIGHT)	+= ledtrig-backlight.o
>  obj-$(CONFIG_LEDS_TRIGGER_GPIO)		+= ledtrig-gpio.o
>  obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)	+= ledtrig-default-on.o
> +obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)	+= ledtrig-transient.o
> diff --git a/drivers/leds/ledtrig-transient.c b/drivers/leds/ledtrig-transient.c
> new file mode 100644
> index 0000000..e829124
> --- /dev/null
> +++ b/drivers/leds/ledtrig-transient.c
> @@ -0,0 +1,246 @@
> +/*
> + * LED Kernel Transient Trigger
> + *
> + * Copyright (C) 2012 Shuah Khan <shuahkhan@...il.com>
> + *
> + * Based on Richard Purdie's ledtrig-timer.c and Atsushi Nemoto's
> + * ledtrig-heartbeat.c
> + * Design and use-case input from Jonas Bonn <jonas@...thpole.se>
> + *
> + * 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 trigger is intended to be used for the following example use cases:
> +
> +* - Control of vibrate (phones, tablets etc.) hardware by userspace app.
> +* - Use of LED by userspace app as activity indicator.
> +* - Use of LED by userspace app as a kind of watchdog indicator -- as
> +*	long as the app is alive, it can keep the LED illuminated, if it dies
> +*	the LED will be extinguished automatically.
> +* - Use by any userspace app that needs a transient GPIO output.
> +*
> +* Transient trigger exports two attributes:
> +*	transient_enabled -	one shot timer enable mechanism.
> +*				1 when enabled, 0 when disabled.
> +*				enabled state indicates a timer
> +*				with a value of transient_time running.
> +*				disabled state indicates no active timer
> +*				running.
> +*	transient_time -	one shot timer value. When transient_enabled
> +*				is set, transient_time value is used to start
> +*				a timer that runs once.
> +*	When timer expires transient_enabled goes back to disabled state,
> +*	transient_time is left at the set value to be used when transient
> +*	is enabled at a future time. This will allow user app to set the
> +*	time once and enable it to run it once for the specified value as
> +*	needed.
> +*
> +*	echo 1 > transient_enabled - starts timer = transient_time when
> +*				     transient_time is not 0.
> +*	echo 0 > transient_enabled - cancels currently running timer.
> +*	echo n > transient_time	   - stores timer value to be used upon next
> +*				     enable. Currently active timer if any,
> +*				     continues to run for the specified time.
> +*	echo 0 > transient_time	   - stores timer value to be used upon next
> +*				     enable. Currently active timer if any,
> +*				     continues to run for the specified time.
> +*
> +*	Future enhancements:
> +*	1. extending and shortening the timer could be supported either by
> +*	   simply modifying the running timer transient_timer is set when
> +*	   transient_enabled is in enabled state. For example:
> +*	   echo n > transient_time will extend/shorten timer, however this
> +*	   could lead race condtions with timer expiry.
> +*	2. The same could be done using a third attribute to control
> +*	   extending and shortening the timer.
> +*
> +*	Example use-cases:
> +*	echo transient > trigger
> +*	echo n > transient_time
> +*	repeat the following step as needed:
> +*	echo 1 > transient_enabled - start timer = transient_time to run once
> +*	echo 1 > transient_enabled - start timer = transient_time to run once
> +*	echo none > trigger
> +*/
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +#include <linux/timer.h>
> +#include <linux/leds.h>
> +#include "leds.h"
> +
> +struct transient_trig_data {
> +	int transient_enabled;

As mentioned earlier, I don't think you need to track this.

> +	unsigned long transient_time;
> +	struct timer_list timer;
> +};
> +
> +static void transient_timer_function(unsigned long data)
> +{
> +	struct led_classdev *led_cdev = (struct led_classdev *) data;
> +	struct transient_trig_data *transient_data = led_cdev->trigger_data;
> +
> +	if (transient_data->transient_enabled) {
> +		transient_data->transient_enabled = 0;
> +		led_cdev->brightness_set(led_cdev, LED_OFF);
> +		del_timer(&transient_data->timer);
> +	}
> +}
> +
> +static ssize_t led_transient_enabled_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct transient_trig_data *transient_data = led_cdev->trigger_data;
> +
> +	return sprintf(buf, "%d\n", transient_data->transient_enabled);
> +}
> +
> +static ssize_t led_transient_enabled_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct transient_trig_data *transient_data = led_cdev->trigger_data;
> +	unsigned long state;
> +	ssize_t ret = -EINVAL;
> +
> +	ret = kstrtoul(buf, 10, &state);
> +	if (ret)
> +		return ret;
> +
> +	if (state != 1 && state != 0)
> +		return ret;
> +
> +	/* cancel the running timer */
> +	if (state == 0) {
> +		transient_timer_function((unsigned long) led_cdev);
> +		return size;
> +	}
> +
> +	transient_data->transient_enabled = (int) state;
> +
> +	/* start timer with transient_time value */
> +	if (state == 1 && transient_data->transient_time != 0) {
> +		led_cdev->brightness_set(led_cdev, LED_FULL);
> +		mod_timer(&transient_data->timer,
> +			  jiffies + transient_data->transient_time);
> +	}
> +
> +	return size;
> +}
> +
> +static ssize_t led_transient_time_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct transient_trig_data *transient_data = led_cdev->trigger_data;
> +
> +	return sprintf(buf, "%lu\n", transient_data->transient_time);
> +}
> +
> +static ssize_t led_transient_time_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t size)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct transient_trig_data *transient_data = led_cdev->trigger_data;
> +	unsigned long state;
> +	ssize_t ret = -EINVAL;
> +
> +	ret = kstrtoul(buf, 10, &state);
> +	if (ret)
> +		return ret;
> +
> +	transient_data->transient_time = state;
> +
> +	return size;
> +}
> +
> +static DEVICE_ATTR(transient_enabled, 0644, led_transient_enabled_show,
> +		   led_transient_enabled_store);

I would make this a verb, 'enable', and make it 0200.  Even 'enable'
isn't a great choice; something along the lines of 'activate' seems more
appropriate. 

> +static DEVICE_ATTR(transient_time, 0644, led_transient_time_show,
> +		   led_transient_time_store);
> +
> +static void transient_trig_activate(struct led_classdev *led_cdev)
> +{
> +	int rc;
> +	struct transient_trig_data *tdata;
> +
> +	tdata = kzalloc(sizeof(struct transient_trig_data), GFP_KERNEL);
> +	if (!tdata) {
> +		dev_err(led_cdev->dev,
> +			"unable to allocate transient trigger\n");
> +		return;
> +	}
> +	led_cdev->trigger_data = tdata;
> +
> +	rc = device_create_file(led_cdev->dev, &dev_attr_transient_enabled);
> +	if (rc)
> +		goto err_out;
> +
> +	rc = device_create_file(led_cdev->dev, &dev_attr_transient_time);
> +	if (rc)
> +		goto err_out_time;
> +
> +	setup_timer(&tdata->timer, transient_timer_function,
> +		    (unsigned long) led_cdev);
> +	led_cdev->activated = true;
> +
> +	printk(KERN_DEBUG "Ativated led transient trigger %s\n",
> +		led_cdev->name);
> +
> +	return;
> +
> +err_out_time:
> +	device_remove_file(led_cdev->dev, &dev_attr_transient_enabled);
> +err_out:
> +	dev_err(led_cdev->dev, "unable to register transient trigger\n");
> +	led_cdev->trigger_data = NULL;
> +	kfree(tdata);
> +}
> +
> +static void transient_trig_deactivate(struct led_classdev *led_cdev)
> +{
> +	struct transient_trig_data *transient_data = led_cdev->trigger_data;
> +
> +	if (led_cdev->activated) {
> +		device_remove_file(led_cdev->dev, &dev_attr_transient_enabled);
> +		device_remove_file(led_cdev->dev, &dev_attr_transient_time);
> +		del_timer_sync(&transient_data->timer);
> +		led_cdev->trigger_data = NULL;
> +		led_cdev->activated = false;
> +		kfree(transient_data);
> +	}
> +	printk(KERN_DEBUG "Deativated led transient trigger %s\n",
> +		led_cdev->name);
> +}
> +
> +static struct led_trigger transient_trigger = {
> +	.name     = "transient",
> +	.activate = transient_trig_activate,
> +	.deactivate = transient_trig_deactivate,
> +};
> +
> +static int __init transient_trig_init(void)
> +{
> +	printk(KERN_DEBUG "Registered led transient trigger\n");
> +	return led_trigger_register(&transient_trigger);
> +}
> +
> +static void __exit transient_trig_exit(void)
> +{
> +	led_trigger_unregister(&transient_trigger);
> +	printk(KERN_DEBUG "Unregistered led transient trigger\n");
> +}
> +
> +module_init(transient_trig_init);
> +module_exit(transient_trig_exit);
> +
> +MODULE_AUTHOR("Shuah Khan <shuahkhan@...il.com>");
> +MODULE_DESCRIPTION("Transient LED trigger");
> +MODULE_LICENSE("GPL");


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists