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

[Johannes Berg]

On Tue, 2010-09-21 at 12:44 -0700, Andrew Morton wrote:
> On Fri, 20 Aug 2010 11:21:42 +0200
> Johannes Berg <johannes@xxxxxxxxxxxxxxxx> 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.

johannes

--
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/