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

[Andrew Morton <akpm@linux-foundation.org>]

On Wed, 25 Apr 2012 11:42:38 -0600
Shuah Khan <shuahkhan@gmail.com> wrote:

> Second version of the patch to add new transient trigger to support
> one shot timer activation. This trigger has two properties, activate
> and duration. duration allows setting timer value in msecs. activate
> allows activating and deactivating the timer specified by duration as
> needed.
> 
> There was a suggestion to change units to seconds similar to "safe_delay_show" 
> and "safe_delay_store" in drivers/md/md.c. safe_delay_show shows time in
> seconds, however safe_delay_store still takes the value in msecs. This
> sounded like an asymmetric interface.
> 
> I decided to leave duration units as msecs to be consistent with the
> rest of the triggers in the leds sub-system for now.
> 
> I also found couple of bugs in activate store causing it to cancel the 
> timer when it shouldn't and extending the timer when a second activate
> comes in while timer is running.
> 
> Hope I addressed all of the review comments and didn't miss anything.
> Thanks again for a good discussion and ideas.

First rule of changelogs: think about how your words will appear to
others in a year's time.  Text which refers to an earlier version of
the patch is uninteresting.  Text which refers to some review
discussion is uninteresting.  There is a convention that this short-term
information is presented after the separating "^---" in the changelog,
after the signoffs, cc's, etc.

When I stripped out all the short-term fluff all I was left with was
part of the first paragraph, and it's a poor changelog.  It doesn't
describe what a "transient trigger" is, it doesn't define "one shot
timer activation".  Quite importantly, it doesn't describe the user
interface to this feature, which is the most important part of the
patch, because it is the part we can never change.

So, please fully describe the feature and fully describe the interface
which it exposes to users.  This would include a description of dynamic
behaviour such as cancelation, what happens if a LED driver module is
removed when the trigger is in its pending and active states, what
happens if the trigger is re-acticated when it is pending, what happens
if the trigger is re-activated when the LED is active, etc.

This stuff matters!  We want to hear you describe it in your own words,
so we can understand (and hopefully agree with) your design intent and
then check the implementation agaisnt that intent.

Can I use this feature to have my LED temporarily blink *off*?

Another thing which is missing here is a decent description of why the
kernel needs this feature.  I can implement a one-shot LED trigger in
userspace, using complex things like "sleep".  Where is the
justification for adding kernel code to do this?