Re: [PATCH 1/1] leds: add "kickable" LED trigger

[Shuah Khan]

On Tue, 2012-04-17 at 00:33 +0200, Jonas Bonn wrote:
> Hi Shuah,
> 
> Nice work!  If I may comment on this briefly:
> 
> i)  The semantics of this patch make me cringe.  It's bad enough that
> this is a "LED" trigger; having a "vibrating LED" is even worse.
> Really, this is a GPIO trigger (which is reasonably logical); secondly,
> this trigger is about maintaining a transient state given some criteria.
> 
> ii)  Why force the trigger to a fixed duration?  The point, if I
> understand correctly, is to return to the "rest state" after a given
> time in case the triggering entity dies; but if the triggering entity
> (userspace application) is still alive, why not let it prolong the
> "active state" by triggering again.  This is along the lines of the
> 'kickable' trigger patch I posted earlier.
> 
> iii)  I don't see the point of maintaining the state.  The state is
> maintained by the fact that you have running timer; if the timer is not
> running, the state is the "rest state".
> 
> I'd suggest:
> 
> i)  Calling this ledtrig-transient
> ii)  Having a property 'duration' that determines how long the
> GPIO/LED/signal is 'active'
> iii) Having a property 'activate' (or 'tickle'!) that just sets the
> 'active' state if it isn't already active; writing to this properly
> while active just resets the timer to 0 so that another full 'duration'
> can pass before the LED deactivates.
> iv)  Maybe having a property 'active-state' to decide whether the LED is
> on or off when active; this isn't strictly necessary as the LED class
> already has the active-low property to invert the meaning of the ON
> state.
> 
> This would allow this trigger to be used for a bunch of different use
> cases:
> 
> i)   Control of vibrator by userspace app.
> ii)  Use of LED by userspace app as activity indicator.
> iii) 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.
> iv)  Use by any userspace app that needs a transient GPIO output.
> 
> I hope that makes sense.  It mostly comes down to semantics... the
> implementation is mostly fine, with the exception of maintaining the
> illumination state that I don't see the need for.
> 
> /Jonas

Jonas,

Thanks for the feedback. I better change the name before too long :)
Thanks for summarizing the use-cases. I like your duration and activate
idea. I am testing the patch now and will incorporate your ideas. I
would like to include you in the thread when I send it out if you don't
mind. Thanks again,

-- Shuah

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