Re: [PATCH ] leds: add new transient trigger for one shot timer support

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 2248 bytes --]

On Sun, 22 Apr 2012 17:51:27 -0600 Shuah Khan <shuahkhan@gmail.com> wrote:

> Hi Jonas,
> 
> activate does sound better and having units in the property name is a
> good idea. Will make the changes including the 0200 change for activate.
> Will change the transient_time to transient_time_msec (no problem using
> duration, just that time is shorter :)

I'm not sure I agree.

I think there are two possible correct choices:

 1/ follow the established pattern:  use "delay_off" and assume milliseconds.
    Consistency is good.

 2/ Do the "right" thing which is to use seconds.  "seconds" are the SI 
    unit for time and we should always use them (says Linus - I could
    probably hunt up a reference if you were interested).
    so "0.2" is 200ms.  In that case don't use "delay_off" - probably
    "duration" would be good.
    This doesn't mean you need to use floating point.  See
    "safe_delay_show" and "safe_delay_store" in drivers/md/md.c

But that is just my opinion.


> 
> > A couple of questions:
> > 
> > i)  Why is it important for you to maintain the LED state?
> One use-case that would be easier for user to comprehend would be to
> have a constant timer that runs periodically. time value doesn't change
> and by simply writing 1 or 0, timer with a value specified by the time
> file can be started. The following use-case is a good example for what I
> am talking about:

I agree that "delay" and "activate" should be separate interfaces.

I wonder if we should allow control of the brightness during the "on" time as
well.
You could set the brightness after enabling the timer, but awkward pauses or
races could then leave the "led" permanently on.

Possibly we could hook into led_set_brightness() and restart the timer
whenever the brightness was set - and remember the setting.


So
  echo 100 > brightness

set the current brightness, sets the future brightness, starts the timer.
Or maybe if we have to hook into led_set_brightness anyway, then we could
cause "echo 100 > brightness" *not* to set the current brightness but just
to record a value for later usage...

But these are just minor things.  I think you are close to something useful.

NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]