Re: [RFC] led-class: always implement blinking

[Richard Purdie <rpurdie@rpsys.net>]

On Tue, 2010-09-21 at 21:48 +0200, Johannes Berg wrote:
> On Tue, 2010-09-21 at 12:44 -0700, Andrew Morton wrote:
> > On Fri, 20 Aug 2010 11:21:42 +0200
> > Johannes Berg <johannes@sipsolutions.net> wrote:
> > 
> > > +static int led_blink_set(struct led_classdev *led_cdev,
> > > +			 unsigned long *delay_on, unsigned long *delay_off)
> 
> > > +	if (*delay_on == led_cdev->blink_delay_on &&
> > > +	    *delay_off == led_cdev->blink_delay_off)
> > > +		return 0;
> > > +
> > > +	/* deactivate previous settings */
> > > +	del_timer_sync(&led_cdev->blink_timer);
> > > +
> > > +	led_cdev->blink_delay_on = *delay_on;
> > > +	led_cdev->blink_delay_off = *delay_off;
> 
> > delay_on and delay_off could have been pass-by-value rather than
> > pass-by-reference?  That would clean up some gunk in callers, too.
> > 
> > If there was some reason for doing it with pass-by-reference then that
> > reason should have been documented!
> 
> Well, this function gets assigned to led_cdev->blink_set(), which is a
> function pointer that takes pass-by-reference arguments. The comment
> there says:
> 
>         /* Activate hardware accelerated blink, delays are in
>          * miliseconds and if none is provided then a sensible default
>          * should be chosen. The call can adjust the timings if it can't
>          * match the values specified exactly. */
>         int             (*blink_set)(struct led_classdev *led_cdev,
>                                      unsigned long *delay_on,
>                                      unsigned long *delay_off);
> 
> but the software implementation doesn't adjust the timings, of course. I
> suppose the "adjust the timings" was also meant to update the values.

The idea was that hardware fallbacks would let the caller know what
values it had actually fallen back to. The software fallback using
timers is generic so doesn't need to change the values.

I've been meaning to look more closely at the patch but I haven't got to
it yet, sorry :(.

Richard