Message-ID: <20120331070743.526de859@notabene.brown>
Date:	Sat, 31 Mar 2012 07:07:43 +1100
From:	NeilBrown <neilb@...e.de>
To:	shuahkhan@...il.com
Cc:	rpurdie@...ys.net, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv2] LEDS-One-Shot-Timer-Trigger-implementation

On Fri, 30 Mar 2012 09:00:56 -0600 Shuah Khan <shuahkhan@...il.com> wrote:

> Neil, Richard,
> 
> How about this patch? Does this good now? Anything else that 
> needs attention?

Looks good, thanks.

My only concern is that the first sentence of the doc file reads strangely:

     LED infrastructure lacks support for timer-no-default trigger and
     activation.

It looks like it used to say "timer-one-shot" and you did a search/replace on
it.
I think this sentence should mention "one-shot".  "timer-no-default" and
"forever" are how you implement "one-shot" functionality.

Most leds patches seem to be going through Andrew Morton these days.
So if you don't hear anything from anyone else in a couple of day, I suggest
you add
   Reviewed-by: NeilBrown <neilb@...e.de>
   Cc: Richard Purdie <rpurdie@...ux.intel.com>

to the 'Signed-off-by: block and mail it to akpm@...ux-foundation.org (please
lkml etc).

NeilBrown


> 
> Thanks,
> -- Shuah 
> 
> On Wed, 2012-03-28 at 17:28 -0600, Shuah Khan wrote:
> > Neil,
> > 
> > I also added documentation file in this second version. Thanks again for
> > brainstorming ideas and reviews. Leaving the subject line unchanged to
> > avoid confusion, and the documentation and code are updated to change
> > one-shot-timer to timer-no-default.
> > 
> > > 
> > > A separate patch is certainly a good idea.
> > > I would do the clean-up patches first, and then have the big change at the
> > > end of the series.  That way checkpatch won't complain about anything in your
> > > change.
> > > But you should do what you are most comfortable with.
> > 
> > Decided to do the simple_strtoul() cleanup patch later - after this one
> > goes through.
> > > 
> > > > 
> > > > > 
> > > > > > +	
> > > > > 
> > > > > Do you need to led_trigger_unregister(&timer_led_trigger) if the registration
> > > > > of one_shot_timer_led_trigger fails?
> > > > 
> > > > Yes, considered that while I was changing this routine, and thought
> > > > might be better to leave the registered timer alone when the second
> > > > registration fails. I can go either way.
> > > 
> > > The important point is that if timer_trig_init fails, then timer_trig_exit
> > > will never be called.
> > > So when timer_trig_init fails, it must ensure that it has un-done any bits
> > > that it successfully did.
> > 
> > Fixed.
> > 
> > -- Shuah
> > 
> > LED infrastructure lacks support for timer-no-default trigger and activation.
> > The current support allows for setting two timers, one for specifying how 
> > long a state to be on, and the second for how long the state to be off. For
> > example, delay_on value specifies the time period an LED should stay in on
> > state, followed by a delay_off value that specifies how long the LED should
> > stay in off state. The on and off cycle repeats until the trigger gets 
> > deactivated. There is no provision for one time activation to implement
> > features that require an on or off state to be held just once and then stay
> > in the original state forever.
> > 
> > This feature will help implement vibrate functionality which requires one
> > time activation of vibrate mode without a continuous vibrate on/off cycles.
> > 
> > This patch implements the timer-no-default trigger support by enhancing the
> > current led-class, led-core, and ledtrig-timer drivers to
> > 
> > 1. Add support for forever timer case. forever tag can be written to delay_on
> > or delay_off files. Internally forever is mapped to ULONG_MAX with no timer
> > associated with it.
> > 
> > 2. The led_blink_set() which takes two pointers to times one each for delay_on
> > and delay_off has been extended so that a NULL instead of a pointer means 
> > "forever".
> > 
> > 3. Add a new timer-no-default trigger to ledtrig-timer
> > 
> > 
> > The above enhancements support the following use-cases:
> > 
> > use-case 1:
> > echo timer-no-default > /sys/class/leds/SOMELED/trigger
> > echo forever > /sys/class/leds/SOMELED/delay_off
> > echo 2000 > /sys/class/leds/SOMELED/delay_on
> > 
> > When timer-no-default is activated in step1, unlike the timer trigger case,
> > timer-no-default activate routine activates the trigger without starting
> > any timers. The default 1 HZ delay_on and delay_off timers won't be started
> > like in the case of timer trigger activation. Not starting timers ensures 
> > that the one time state isn't stuck if some error occurs before actual timer
> > periods are specified. delay_on and delay_off files get created with 0
> > values. Please note that it is important to set delay_off to forever prior
> > to setting delay_on value. If the order is reversed, the LED will be turned
> > on, with no timer set to turn it off.
> > 
> > When delay_off value is specified in step 2, delay_off_store recognizes the
> > special forever tag and records it and returns without starting any timer.
> > Internally forever maps to ULONG_MAX. The led_blink_set() which takes
> > two pointers to times one each for delay_on and delay_off has been extended so
> > that a NULL instead of a pointer means "forever".
> > 
> > When delay_on value is specified in step 3, a timer gets started for delay_on
> > period, and delay_off stays at ULONG_MAX with no timer associated with it. 
> > 
> > use-case 2:
> > echo timer-no-default > /sys/class/leds/SOMELED/trigger
> > echo forever > /sys/class/leds/SOMELED/delay_on
> > echo 2000 > /sys/class/leds/SOMELED/delay_off
> > 
> > When timer-no-default is activated in step1, unlike the timer trigger case,
> > timer-no-default activate routine activates the trigger without starting
> > any timers. The default 1 HZ delay_on and delay_off timers won't be started
> > like in the case of timer trigger activation. Not starting timers ensures 
> > that the one time state isn't stuck if some error occurs before actual timer
> > periods are specified. delay_on and delay_off files get created with 0
> > values. Please note that it is important to set delay_on to forever prior
> > to setting delay_off value. If the order is reversed, the LED will be turned
> > off, with no timer set to turn it back on.
> > 
> > When delay_on value is specified in step 2, delay_on_store recognizes the
> > special forever tag and records it and returns without starting any timer. 
> > Internally forever maps to ULONG_MAX.
> > 
> > When delay_off value is specified in step 3, a timer gets started for delay_off
> > period, and delay_on stays at ULONG_MAX with no timer associated with it. 
> > 
> > From f7e8a55fb9ffa10daad1bbb80f2e9e0d81ad359f Mon Sep 17 00:00:00 2001
> > From: Shuah Khan <shuahkhan@...il.com>
> > Date: Wed, 28 Mar 2012 15:40:33 -0600
> > Subject: [PATCH] LEDS-timer-no-default-implementation
> > 
> > 
> > Signed-off-by: Shuah Khan <shuahkhan@...il.com>
> > ---
> >  .../leds/leds-timer_no_default_trigger.txt         |   79 +++++++++++++++
> >  drivers/leds/led-class.c                           |    4 +-
> >  drivers/leds/led-core.c                            |   26 ++++-
> >  drivers/leds/leds.h                                |    2 +
> >  drivers/leds/ledtrig-timer.c                       |  104 ++++++++++++++------
> >  5 files changed, 180 insertions(+), 35 deletions(-)
> >  create mode 100644 Documentation/leds/leds-timer_no_default_trigger.txt
> > 
> > diff --git a/Documentation/leds/leds-timer_no_default_trigger.txt b/Documentation/leds/leds-timer_no_default_trigger.txt
> > new file mode 100644
> > index 0000000..ff3fdad
> > --- /dev/null
> > +++ b/Documentation/leds/leds-timer_no_default_trigger.txt
> > @@ -0,0 +1,79 @@
> > +
> > +LED timer no default trigger feature (one shot timer)
> > +=====================================================
> > +
> > +LED infrastructure lacks support for timer-no-default trigger and activation.
> > +The current support allows for setting two timers, one for specifying how
> > +long a state to be on, and the second for how long the state to be off. For
> > +example, delay_on value specifies the time period an LED should stay in on
> > +state, followed by a delay_off value that specifies how long the LED should
> > +stay in off state. The on and off cycle repeats until the trigger gets
> > +deactivated. There is no provision for one time activation to implement
> > +features that require an on or off state to be held just once and then stay
> > +in the original state forever.
> > +
> > +This feature will help implement vibrate functionality which requires one
> > +time activation of vibrate mode without a continuous vibrate on/off cycles.
> > +
> > +This patch implements the timer-no-default trigger support by enhancing the
> > +current led-class, led-core, and ledtrig-timer drivers to:
> > +
> > +- Add support for forever timer case. forever tag can be written to delay_on
> > +  or delay_off files. Internally forever is mapped to ULONG_MAX with no timer
> > +  associated with it.
> > +
> > +- The led_blink_set() which takes two pointers to times one each for delay_on
> > +  and delay_off has been extended so that a NULL instead of a pointer means
> > +  "forever".
> > +
> > +- Add a new timer-no-default trigger to ledtrig-timer
> > +
> > +The above enhancements support the following use-cases:
> > +
> > +use-case 1:
> > +echo timer-no-default > /sys/class/leds/SOMELED/trigger
> > +echo forever > /sys/class/leds/SOMELED/delay_off
> > +echo 2000 > /sys/class/leds/SOMELED/delay_on
> > +
> > +When timer-no-default is activated in step1, unlike the timer trigger case,
> > +timer-no-default activate routine activates the trigger without starting
> > +any timers. The default 1 HZ delay_on and delay_off timers won't be started
> > +like in the case of timer trigger activation. Not starting timers ensures
> > +that the one time state isn't stuck if some error occurs before actual timer
> > +periods are specified. delay_on and delay_off files get created with 0
> > +values. Please note that it is important to set delay_off to forever prior
> > +to setting delay_on value. If the order is reversed, the LED will be turned
> > +on, with no timer set to turn it off.
> > +
> > +When delay_off value is specified in step 2, delay_off_store recognizes the
> > +special forever tag and records it and returns without starting any timer.
> > +Internally forever maps to ULONG_MAX. The led_blink_set() which takes
> > +two pointers to times one each for delay_on and delay_off has been extended
> > +so that a NULL instead of a pointer means "forever".
> > +
> > +When delay_on value is specified in step 3, a timer gets started for
> > +delay_on period, and delay_off stays at ULONG_MAX with no timer associated
> > +with it.
> > +
> > +use-case 2:
> > +echo timer-no-default > /sys/class/leds/SOMELED/trigger
> > +echo forever > /sys/class/leds/SOMELED/delay_on
> > +echo 2000 > /sys/class/leds/SOMELED/delay_off
> > +
> > +When timer-no-default is activated in step1, unlike the timer trigger case,
> > +timer-no-default activate routine activates the trigger without starting
> > +any timers. The default 1 HZ delay_on and delay_off timers won't be started
> > +like in the case of timer trigger activation. Not starting timers ensures
> > +that the one time state isn't stuck if some error occurs before actual timer
> > +periods are specified. delay_on and delay_off files get created with 0
> > +values. Please note that it is important to set delay_on to forever prior
> > +to setting delay_off value. If the order is reversed, the LED will be turned
> > +off, with no timer set to turn it back on.
> > +
> > +When delay_on value is specified in step 2, delay_on_store recognizes the
> > +special forever tag and records it and returns without starting any timer.
> > +Internally forever maps to ULONG_MAX.
> > +
> > +When delay_off value is specified in step 3, a timer gets started for
> > +delay_off period, and delay_on stays at ULONG_MAX with no timer associated
> > +with it.
> > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> > index 5bff843..ed123ba 100644
> > --- a/drivers/leds/led-class.c
> > +++ b/drivers/leds/led-class.c
> > @@ -107,7 +107,9 @@ static void led_timer_function(unsigned long data)
> >  
> >  	led_set_brightness(led_cdev, brightness);
> >  
> > -	mod_timer(&led_cdev->blink_timer, jiffies + msecs_to_jiffies(delay));
> > +	if (delay != LED_TIMER_FOREVER)
> > +		mod_timer(&led_cdev->blink_timer,
> > +			jiffies + msecs_to_jiffies(delay));
> >  }
> >  
> >  /**
> > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> > index d686004..419b0bc 100644
> > --- a/drivers/leds/led-core.c
> > +++ b/drivers/leds/led-core.c
> > @@ -72,17 +72,35 @@ void led_blink_set(struct led_classdev *led_cdev,
> >  		   unsigned long *delay_on,
> >  		   unsigned long *delay_off)
> >  {
> > +	unsigned long val_on;
> > +	unsigned long val_off;
> > +
> >  	del_timer_sync(&led_cdev->blink_timer);
> >  
> > -	if (led_cdev->blink_set &&
> > +	if (delay_on && delay_off && led_cdev->blink_set &&
> >  	    !led_cdev->blink_set(led_cdev, delay_on, delay_off))
> >  		return;
> >  
> > +	/* if delay_on is null, leave it on forever after delay_off period
> > +	   if delay_off is null, leave it off forever after delay on period */
> > +	if (!delay_on)
> > +		val_on = LED_TIMER_FOREVER;
> > +	else
> > +		val_on = *delay_on;
> > +
> > +	if (!delay_off)
> > +		val_off = LED_TIMER_FOREVER;
> > +	else
> > +		val_off = *delay_off;
> > +
> >  	/* blink with 1 Hz as default if nothing specified */
> > -	if (!*delay_on && !*delay_off)
> > -		*delay_on = *delay_off = 500;
> > +	if (!val_on && !val_off) {
> > +		val_on = val_off = 500;
> > +		*delay_on = 500;
> > +		*delay_off = 500;
> > +	}
> >  
> > -	led_set_software_blink(led_cdev, *delay_on, *delay_off);
> > +	led_set_software_blink(led_cdev, val_on, val_off);
> >  }
> >  EXPORT_SYMBOL(led_blink_set);
> >  
> > diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> > index e77c7f8..b2cda9f 100644
> > --- a/drivers/leds/leds.h
> > +++ b/drivers/leds/leds.h
> > @@ -17,6 +17,8 @@
> >  #include <linux/rwsem.h>
> >  #include <linux/leds.h>
> >  
> > +#define LED_TIMER_FOREVER ULONG_MAX
> > +
> >  static inline void led_set_brightness(struct led_classdev *led_cdev,
> >  					enum led_brightness value)
> >  {
> > diff --git a/drivers/leds/ledtrig-timer.c b/drivers/leds/ledtrig-timer.c
> > index 328c64c..e323cf2 100644
> > --- a/drivers/leds/ledtrig-timer.c
> > +++ b/drivers/leds/ledtrig-timer.c
> > @@ -24,6 +24,9 @@ static ssize_t led_delay_on_show(struct device *dev,
> >  {
> >  	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> >  
> > +	if (led_cdev->blink_delay_on == LED_TIMER_FOREVER)
> > +		return sprintf(buf, "forever\n");
> > +
> >  	return sprintf(buf, "%lu\n", led_cdev->blink_delay_on);
> >  }
> >  
> > @@ -32,17 +35,25 @@ static ssize_t led_delay_on_store(struct device *dev,
> >  {
> >  	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> >  	int ret = -EINVAL;
> > -	char *after;
> > -	unsigned long state = simple_strtoul(buf, &after, 10);
> > -	size_t count = after - buf;
> > -
> > -	if (isspace(*after))
> > -		count++;
> >  
> > -	if (count == size) {
> > -		led_blink_set(led_cdev, &state, &led_cdev->blink_delay_off);
> > -		led_cdev->blink_delay_on = state;
> > -		ret = count;
> > +	if (strncmp(buf, "forever", 7) == 0) {
> > +		led_blink_set(led_cdev, NULL, &led_cdev->blink_delay_off);
> > +		led_cdev->blink_delay_on = LED_TIMER_FOREVER;
> > +		ret = size;
> > +	} else {
> > +		char *after;
> > +		unsigned long state = simple_strtoul(buf, &after, 10);
> > +		size_t count = after - buf;
> > +
> > +		if (isspace(*after))
> > +			count++;
> > +
> > +		if (count == size) {
> > +			led_blink_set(led_cdev, &state,
> > +				&led_cdev->blink_delay_off);
> > +			led_cdev->blink_delay_on = state;
> > +			ret = count;
> > +		}
> >  	}
> >  
> >  	return ret;
> > @@ -53,6 +64,9 @@ static ssize_t led_delay_off_show(struct device *dev,
> >  {
> >  	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> >  
> > +	if (led_cdev->blink_delay_off == LED_TIMER_FOREVER)
> > +		return sprintf(buf, "forever\n");
> > +
> >  	return sprintf(buf, "%lu\n", led_cdev->blink_delay_off);
> >  }
> >  
> > @@ -61,17 +75,24 @@ static ssize_t led_delay_off_store(struct device *dev,
> >  {
> >  	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> >  	int ret = -EINVAL;
> > -	char *after;
> > -	unsigned long state = simple_strtoul(buf, &after, 10);
> > -	size_t count = after - buf;
> >  
> > -	if (isspace(*after))
> > -		count++;
> > -
> > -	if (count == size) {
> > -		led_blink_set(led_cdev, &led_cdev->blink_delay_on, &state);
> > -		led_cdev->blink_delay_off = state;
> > -		ret = count;
> > +	if (strncmp(buf, "forever", 7) == 0) {
> > +		led_blink_set(led_cdev, &led_cdev->blink_delay_on, NULL);
> > +		led_cdev->blink_delay_off = LED_TIMER_FOREVER;
> > +	} else {
> > +		char *after;
> > +		unsigned long state = simple_strtoul(buf, &after, 10);
> > +		size_t count = after - buf;
> > +
> > +		if (isspace(*after))
> > +			count++;
> > +
> > +		if (count == size) {
> > +			led_blink_set(led_cdev, &led_cdev->blink_delay_on,
> > +				&state);
> > +			led_cdev->blink_delay_off = state;
> > +			ret = count;
> > +		}
> >  	}
> >  
> >  	return ret;
> > @@ -80,7 +101,7 @@ static ssize_t led_delay_off_store(struct device *dev,
> >  static DEVICE_ATTR(delay_on, 0644, led_delay_on_show, led_delay_on_store);
> >  static DEVICE_ATTR(delay_off, 0644, led_delay_off_show, led_delay_off_store);
> >  
> > -static void timer_trig_activate(struct led_classdev *led_cdev)
> > +static void timer_trig_activate_common(struct led_classdev *led_cdev)
> >  {
> >  	int rc;
> >  
> > @@ -91,17 +112,24 @@ static void timer_trig_activate(struct led_classdev *led_cdev)
> >  		return;
> >  	rc = device_create_file(led_cdev->dev, &dev_attr_delay_off);
> >  	if (rc)
> > -		goto err_out_delayon;
> > -
> > -	led_blink_set(led_cdev, &led_cdev->blink_delay_on,
> > -		      &led_cdev->blink_delay_off);
> > +		device_remove_file(led_cdev->dev, &dev_attr_delay_on);
> >  
> > -	led_cdev->trigger_data = (void *)1;
> > +	else
> > +		led_cdev->trigger_data = (void *)1;
> > +}
> >  
> > -	return;
> > +static void timer_trig_activate_timer_no_default(struct led_classdev *led_cdev)
> > +{
> > +	timer_trig_activate_common(led_cdev);
> > +}
> >  
> > -err_out_delayon:
> > -	device_remove_file(led_cdev->dev, &dev_attr_delay_on);
> > +static void timer_trig_activate(struct led_classdev *led_cdev)
> > +{
> > +	timer_trig_activate_common(led_cdev);
> > +	if (led_cdev->trigger_data) {
> > +		led_blink_set(led_cdev, &led_cdev->blink_delay_on,
> > +		      &led_cdev->blink_delay_off);
> > +	}
> >  }
> >  
> >  static void timer_trig_deactivate(struct led_classdev *led_cdev)
> > @@ -121,14 +149,30 @@ static struct led_trigger timer_led_trigger = {
> >  	.deactivate = timer_trig_deactivate,
> >  };
> >  
> > +static struct led_trigger timer_no_default_led_trigger = {
> > +	.name     = "timer-no-default",
> > +	.activate = timer_trig_activate_timer_no_default,
> > +	.deactivate = timer_trig_deactivate,
> > +};
> > +
> >  static int __init timer_trig_init(void)
> >  {
> > -	return led_trigger_register(&timer_led_trigger);
> > +	int rc = 0;
> > +
> > +	rc = led_trigger_register(&timer_led_trigger);
> > +	if (!rc) {
> > +		rc = led_trigger_register(&timer_no_default_led_trigger);
> > +		if (rc)
> > +			led_trigger_unregister(&timer_led_trigger);
> > +	}
> > +
> > +	return rc;
> >  }
> >  
> >  static void __exit timer_trig_exit(void)
> >  {
> >  	led_trigger_unregister(&timer_led_trigger);
> > +	led_trigger_unregister(&timer_no_default_led_trigger);
> >  }
> >  
> >  module_init(timer_trig_init);
> 


Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)

Powered by blists - more mailing lists