On Thu, Feb 23, 2012 at 12:00:52PM -0800, Guenter Roeck wrote:
> On Thu, 2012-02-23 at 14:50 -0500, Nikolaus Schulz wrote:
> > On Wed, Feb 22, 2012 at 06:26:00PM -0800, Guenter Roeck wrote:
> > > On Wed, Feb 22, 2012 at 05:18:48PM -0500, Nikolaus Schulz wrote:
> > > > +static bool duty_mode_enabled(u8 pwm_enable)
> > > > +{
> > > > + switch (pwm_enable) {
> > > > + case 0: /* Manual, duty mode (full speed) */
> > > > + case 1: /* Manual, duty mode */
> > > > + case 4: /* Auto, duty mode */
> > > > + return true;
> > > > + case 2: /* Auto, speed mode */
> > > > + case 3: /* Manual, speed mode */
> > > > + return false;
> > > > + default:
> > > > + BUG();
> > > > + }
> > > > +}
> > >
> > > This keeps bugging me ... I think it would make sense to simplify it to
> > >
> > > return pwm_enable != 2 && pwm_enable != 3;
> >
> > Well, I used that verbose switch statement because
> >
> > a) I feel uncomfortable using these magic numbers instead of a
> > well-defined enum - by the way, is there a reason for not using an enum for
> > that? - and
> >
> I don't think there is a reason other than being lazy.
If I recall correctly, early versions of this driver posted to this list
did in fact have an enum here, but for some reason that was dropped.
> > b) a simple logical expression like you suggested may lead to hidden
> > bugs once the range of valid values changes.
> >
> Good point. Maybe start using an enum ?
I can prepare another patch for that.
But using an enum does not address b), so I still tend to keep the
switch statement(s). Are you okay with that?
Nikolaus
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
