Re: [PATCH 4/11] LED: Add LED Timer Trigger

From: Richard Purdie <rpurdie@rpsys.net>
To: Jan-Benedict Glaw <jbglaw@lug-owl.de>
Cc: LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 4/11] LED: Add LED Timer Trigger
Date: Tue, 31 Jan 2006 17:36:56 +0000	[thread overview]
Message-ID: <1138729017.6869.219.camel@localhost.localdomain> (raw)
In-Reply-To: <20060131150101.GX18336@lug-owl.de>

On Tue, 2006-01-31 at 16:01 +0100, Jan-Benedict Glaw wrote:
> On Tue, 2006-01-31 13:41:37 +0000, Richard Purdie <rpurdie@rpsys.net> wrote:
> > +static void led_timer_setdata(struct led_device *led_dev, unsigned long duty, unsigned long frequency)
> > +{
> > +	struct timer_trig_data *timer_data = led_dev->trigger_data;
> > +	signed long duty1;
> > +
> > +	if (frequency > 500)
> > +		frequency = 500;
> 
> Why? 

We're dealing with msec delays. Any frequency > 1000 will just cause
problems. There was a reason for using half that but it escapes me and
might be unneeded now. 500Hz/1000Hz is above the frequency the human eye
can see so is unlikely to present a problem.

> ...and especially: why, without complaining?

This is the important bit. It should return an -EINVAL back to
userspace.

> > +	if (duty > 100)
> > +		duty = 100;
> 
> Dito.

Duty cycles > 100 make no sense and would break the subsequent
calculation. Same problem/solution as above.

Thanks,

Richard