Re: [PATCH] LEDS-One-Shot-Timer-Trigger-implementation

[NeilBrown <neilb@suse.de>]

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

On Wed, 28 Mar 2012 10:09:09 -0600 Shuah Khan <shuahkhan@gmail.com> wrote:

> Neil,
> 
> > 
> > In general I approve of this - thanks for your efforts.
> > 
> > However there are some details that need attention.
> 
> Thanks.
> > 
> > > 
> > > This patch implements the one-shot-timer trigger support by enhancing the
> > > current led-class and ledtrig-timer drivers to support the following
> > > use-cases:
> > > 
> > > use-case 1:
> > > echo one-shot-timer > /sys/class/leds/SOMELED/trigger
> > 
> > I don't like the name "one-shot-timer".  This name describes how you expect
> > the functionality to be used, but not what the actual difference between
> > "timer" and "one-shot-timer" is.
> > That difference is simply that one-shot-timer does not start an initial
> > default blink, while "timer" does.
> > So a name like "timer-no-default" would be a more accurate description and so
> > should be preferred.
> 
> Yes, timer-no-default describes the enhancement I am making, better than
> one-shot-timer does. I will generate patch v2 to address your concerns
> and will change the name to timer-no-default and update the relevant
> code and documentation that goes with the patch to reflect the change.

Thanks.


> 
> > 
> > kstrtoul is currently preferred.  It ensures uniform error codes.
> > 
> 
> Yes. I noticed the checkpatch.pl warnings. This change is better made as
> a separate patch. Do you mind if I deferred it and volunteer to fix it
> in a separate patch shortly. :)

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.

> 
> > 
> > > +	
> > 
> > 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.

NeilBrown

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