[lm-sensors] [PATCH 5/6] hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enable

[lm-sensors] [PATCH 5/6] hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enable

[Nikolaus Schulz]

From: Nikolaus Schulz <schulz@macnetix.de>

The F75387 supports automatic fan control using either PWM duty cycle or
RPM speed values.  Make the driver detect the latter mode, and expose the
different modes in sysfs as per pwm_enable, so that the user can switch
between them.

The interpretation of the pwm_enable attribute for the F75387 is adjusted
to be a superset of those values used for similar Fintek chips which do
not support automatic duty mode, with 2 mapping to automatic speed mode,
and moving automatic duty mode to the new value 4.

Toggling the duty mode via pwm_enable is currently denied for the F75387,
as the chip then simply reinterprets the fan configuration register values
according to the new mode, switching between RPM and PWM units, which
makes this a dangerous operation.

Signed-off-by: Nikolaus Schulz <mail@microschulz.de>
---
 drivers/hwmon/f75375s.c |   42 ++++++++++++++++++++++++++++++++++--------
 1 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/f75375s.c b/drivers/hwmon/f75375s.c
index 3b06abf..29b11c6 100644
--- a/drivers/hwmon/f75375s.c
+++ b/drivers/hwmon/f75375s.c
@@ -266,6 +266,21 @@ static inline u16 rpm_to_reg(int rpm)
 	return 1500000 / rpm;
 }
 
+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();
+	}
+}
+
 static ssize_t set_fan_min(struct device *dev, struct device_attribute *attr,
 		const char *buf, size_t count)
 {
@@ -345,11 +360,15 @@ static int set_pwm_enable_direct(struct i2c_client *client, int nr, int val)
 	struct f75375_data *data = i2c_get_clientdata(client);
 	u8 fanmode;
 
-	if (val < 0 || val > 3)
+	if (val < 0 || val > 4)
 		return -EINVAL;
 
 	fanmode = f75375_read8(client, F75375_REG_FAN_TIMER);
 	if (data->kind = f75387) {
+		/* For now, deny dangerous toggling of duty mode */
+		if (duty_mode_enabled(data->pwm_enable[nr]) !+				duty_mode_enabled(val))
+			return -EOPNOTSUPP;
 		/* clear each fanX_mode bit before setting them properly */
 		fanmode &= ~(1 << F75387_FAN_DUTY_MODE(nr));
 		fanmode &= ~(1 << F75387_FAN_MANU_MODE(nr));
@@ -363,12 +382,14 @@ static int set_pwm_enable_direct(struct i2c_client *client, int nr, int val)
 			fanmode  |= (1 << F75387_FAN_MANU_MODE(nr));
 			fanmode  |= (1 << F75387_FAN_DUTY_MODE(nr));
 			break;
-		case 2: /* AUTOMATIC*/
-			fanmode  |=  (1 << F75387_FAN_DUTY_MODE(nr));
+		case 2: /* Automatic, speed mode */
 			break;
 		case 3: /* fan speed */
 			fanmode |= (1 << F75387_FAN_MANU_MODE(nr));
 			break;
+		case 4: /* Automatic, pwm */
+			fanmode  |=  (1 << F75387_FAN_DUTY_MODE(nr));
+			break;
 		}
 	} else {
 		/* clear each fanX_mode bit before setting them properly */
@@ -386,6 +407,8 @@ static int set_pwm_enable_direct(struct i2c_client *client, int nr, int val)
 			break;
 		case 3: /* fan speed */
 			break;
+		case 4: /* Automatic pwm */
+			return -EOPNOTSUPP;
 		}
 	}
 
@@ -761,14 +784,17 @@ static void f75375_init(struct i2c_client *client, struct f75375_data *data,
 
 				manu = ((mode >> F75387_FAN_MANU_MODE(nr)) & 1);
 				duty = ((mode >> F75387_FAN_DUTY_MODE(nr)) & 1);
-				if (manu && duty)
-					/* speed */
+				if (!manu && duty)
+					/* auto, pwm */
+					data->pwm_enable[nr] = 4;
+				else if (manu && !duty)
+					/* manual, speed */
 					data->pwm_enable[nr] = 3;
-				else if (!manu && duty)
-					/* automatic */
+				else if (!manu && !duty)
+					/* automatic, speed */
 					data->pwm_enable[nr] = 2;
 				else
-					/* manual */
+					/* manual, pwm */
 					data->pwm_enable[nr] = 1;
 			} else {
 				if (!(conf & (1 << F75375_FAN_CTRL_LINEAR(nr))))
-- 
1.7.9.1


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 5/6] hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enable

[Guenter Roeck]

On Wed, Feb 22, 2012 at 05:18:48PM -0500, Nikolaus Schulz wrote:
> From: Nikolaus Schulz <schulz@macnetix.de>
> 
> The F75387 supports automatic fan control using either PWM duty cycle or
> RPM speed values.  Make the driver detect the latter mode, and expose the
> different modes in sysfs as per pwm_enable, so that the user can switch
> between them.
> 
> The interpretation of the pwm_enable attribute for the F75387 is adjusted
> to be a superset of those values used for similar Fintek chips which do
> not support automatic duty mode, with 2 mapping to automatic speed mode,
> and moving automatic duty mode to the new value 4.
> 
> Toggling the duty mode via pwm_enable is currently denied for the F75387,
> as the chip then simply reinterprets the fan configuration register values
> according to the new mode, switching between RPM and PWM units, which
> makes this a dangerous operation.
> 
> Signed-off-by: Nikolaus Schulz <mail@microschulz.de>
> ---
[ ... ]

> @@ -363,12 +382,14 @@ static int set_pwm_enable_direct(struct i2c_client *client, int nr, int val)
>  			fanmode  |= (1 << F75387_FAN_MANU_MODE(nr));
>  			fanmode  |= (1 << F75387_FAN_DUTY_MODE(nr));
>  			break;
> -		case 2: /* AUTOMATIC*/
> -			fanmode  |=  (1 << F75387_FAN_DUTY_MODE(nr));
> +		case 2: /* Automatic, speed mode */
>  			break;
>  		case 3: /* fan speed */
>  			fanmode |= (1 << F75387_FAN_MANU_MODE(nr));
>  			break;
> +		case 4: /* Automatic, pwm */
> +			fanmode  |=  (1 << F75387_FAN_DUTY_MODE(nr));

Might be time to get rid of those extra spaces.

> +			break;
>  		}
>  	} else {
>  		/* clear each fanX_mode bit before setting them properly */
> @@ -386,6 +407,8 @@ static int set_pwm_enable_direct(struct i2c_client *client, int nr, int val)
>  			break;
>  		case 3: /* fan speed */
>  			break;
> +		case 4: /* Automatic pwm */
> +			return -EOPNOTSUPP;

	Should be -EINVAL.

Thanks,
Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 5/6] hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enable

[Guenter Roeck]

On Wed, Feb 22, 2012 at 05:18:48PM -0500, Nikolaus Schulz wrote:
> From: Nikolaus Schulz <schulz@macnetix.de>
> 
> The F75387 supports automatic fan control using either PWM duty cycle or
> RPM speed values.  Make the driver detect the latter mode, and expose the
> different modes in sysfs as per pwm_enable, so that the user can switch
> between them.
> 
> The interpretation of the pwm_enable attribute for the F75387 is adjusted
> to be a superset of those values used for similar Fintek chips which do
> not support automatic duty mode, with 2 mapping to automatic speed mode,
> and moving automatic duty mode to the new value 4.
> 
> Toggling the duty mode via pwm_enable is currently denied for the F75387,
> as the chip then simply reinterprets the fan configuration register values
> according to the new mode, switching between RPM and PWM units, which
> makes this a dangerous operation.
> 
> Signed-off-by: Nikolaus Schulz <mail@microschulz.de>
> ---
>  drivers/hwmon/f75375s.c |   42 ++++++++++++++++++++++++++++++++++--------
>  1 files changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwmon/f75375s.c b/drivers/hwmon/f75375s.c
> index 3b06abf..29b11c6 100644
> --- a/drivers/hwmon/f75375s.c
> +++ b/drivers/hwmon/f75375s.c
> @@ -266,6 +266,21 @@ static inline u16 rpm_to_reg(int rpm)
>  	return 1500000 / rpm;
>  }
>  
> +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;

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 5/6] hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enable

[Nikolaus Schulz]

On Wed, Feb 22, 2012 at 05:32:34PM -0800, Guenter Roeck wrote:
> On Wed, Feb 22, 2012 at 05:18:48PM -0500, Nikolaus Schulz wrote:
> > From: Nikolaus Schulz <schulz@macnetix.de>
> > 
> > The F75387 supports automatic fan control using either PWM duty cycle or
> > RPM speed values.  Make the driver detect the latter mode, and expose the
> > different modes in sysfs as per pwm_enable, so that the user can switch
> > between them.
> > 
> > The interpretation of the pwm_enable attribute for the F75387 is adjusted
> > to be a superset of those values used for similar Fintek chips which do
> > not support automatic duty mode, with 2 mapping to automatic speed mode,
> > and moving automatic duty mode to the new value 4.
> > 
> > Toggling the duty mode via pwm_enable is currently denied for the F75387,
> > as the chip then simply reinterprets the fan configuration register values
> > according to the new mode, switching between RPM and PWM units, which
> > makes this a dangerous operation.
> > 
> > Signed-off-by: Nikolaus Schulz <mail@microschulz.de>
> > ---
> [ ... ]
> 
> > @@ -363,12 +382,14 @@ static int set_pwm_enable_direct(struct i2c_client *client, int nr, int val)
> >  			fanmode  |= (1 << F75387_FAN_MANU_MODE(nr));
> >  			fanmode  |= (1 << F75387_FAN_DUTY_MODE(nr));
> >  			break;
> > -		case 2: /* AUTOMATIC*/
> > -			fanmode  |=  (1 << F75387_FAN_DUTY_MODE(nr));
> > +		case 2: /* Automatic, speed mode */
> >  			break;
> >  		case 3: /* fan speed */
> >  			fanmode |= (1 << F75387_FAN_MANU_MODE(nr));
> >  			break;
> > +		case 4: /* Automatic, pwm */
> > +			fanmode  |=  (1 << F75387_FAN_DUTY_MODE(nr));
> 
> Might be time to get rid of those extra spaces.

I see no extra spaces.  You mean the extra parentheses?  I can update
the patch to drop them.

> > +			break;
> >  		}
> >  	} else {
> >  		/* clear each fanX_mode bit before setting them properly */
> > @@ -386,6 +407,8 @@ static int set_pwm_enable_direct(struct i2c_client *client, int nr, int val)
> >  			break;
> >  		case 3: /* fan speed */
> >  			break;
> > +		case 4: /* Automatic pwm */
> > +			return -EOPNOTSUPP;
> 
> 	Should be -EINVAL.

The value 4 is valid as it encodes an existing mode, it's just that this
code path handles hardware that does not support that mode.  Why should
the code return EINVAL?

Thanks for your review,
Nikolaus

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 5/6] hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enable

[Guenter Roeck]

On Thu, 2012-02-23 at 14:12 -0500, Nikolaus Schulz wrote:
> On Wed, Feb 22, 2012 at 05:32:34PM -0800, Guenter Roeck wrote:
> > On Wed, Feb 22, 2012 at 05:18:48PM -0500, Nikolaus Schulz wrote:
> > > From: Nikolaus Schulz <schulz@macnetix.de>
> > > 
> > > The F75387 supports automatic fan control using either PWM duty cycle or
> > > RPM speed values.  Make the driver detect the latter mode, and expose the
> > > different modes in sysfs as per pwm_enable, so that the user can switch
> > > between them.
> > > 
> > > The interpretation of the pwm_enable attribute for the F75387 is adjusted
> > > to be a superset of those values used for similar Fintek chips which do
> > > not support automatic duty mode, with 2 mapping to automatic speed mode,
> > > and moving automatic duty mode to the new value 4.
> > > 
> > > Toggling the duty mode via pwm_enable is currently denied for the F75387,
> > > as the chip then simply reinterprets the fan configuration register values
> > > according to the new mode, switching between RPM and PWM units, which
> > > makes this a dangerous operation.
> > > 
> > > Signed-off-by: Nikolaus Schulz <mail@microschulz.de>
> > > ---
> > [ ... ]
> > 
> > > @@ -363,12 +382,14 @@ static int set_pwm_enable_direct(struct i2c_client *client, int nr, int val)
> > >  			fanmode  |= (1 << F75387_FAN_MANU_MODE(nr));
> > >  			fanmode  |= (1 << F75387_FAN_DUTY_MODE(nr));
> > >  			break;
> > > -		case 2: /* AUTOMATIC*/
> > > -			fanmode  |=  (1 << F75387_FAN_DUTY_MODE(nr));
> > > +		case 2: /* Automatic, speed mode */
> > >  			break;
> > >  		case 3: /* fan speed */
> > >  			fanmode |= (1 << F75387_FAN_MANU_MODE(nr));
> > >  			break;
> > > +		case 4: /* Automatic, pwm */
> > > +			fanmode  |=  (1 << F75387_FAN_DUTY_MODE(nr));
> > 
> > Might be time to get rid of those extra spaces.
> 
> I see no extra spaces.  You mean the extra parentheses?  I can update
> the patch to drop them.
> 
There seem to be two spaces before and after |=.

> > > +			break;
> > >  		}
> > >  	} else {
> > >  		/* clear each fanX_mode bit before setting them properly */
> > > @@ -386,6 +407,8 @@ static int set_pwm_enable_direct(struct i2c_client *client, int nr, int val)
> > >  			break;
> > >  		case 3: /* fan speed */
> > >  			break;
> > > +		case 4: /* Automatic pwm */
> > > +			return -EOPNOTSUPP;
> > 
> > 	Should be -EINVAL.
> 
> The value 4 is valid as it encodes an existing mode, it's just that this
> code path handles hardware that does not support that mode.  Why should
> the code return EINVAL?
> 
val is invalid for this hardware. Also it returned -EINVAL before you
made your changes.

Thanks,
Guenter



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 5/6] hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enable

[Nikolaus Schulz]

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

b) a simple logical expression like you suggested may lead to hidden
   bugs once the range of valid values changes.

But if you insist, I can change that.

Nikolaus

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 5/6] hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enable

[Nikolaus Schulz]

On Thu, Feb 23, 2012 at 11:21:09AM -0800, Guenter Roeck wrote:
> On Thu, 2012-02-23 at 14:12 -0500, Nikolaus Schulz wrote:
> > On Wed, Feb 22, 2012 at 05:32:34PM -0800, Guenter Roeck wrote:
> > > On Wed, Feb 22, 2012 at 05:18:48PM -0500, Nikolaus Schulz wrote:
> > > > From: Nikolaus Schulz <schulz@macnetix.de>
[...]
> > > > @@ -363,12 +382,14 @@ static int set_pwm_enable_direct(struct i2c_client *client, int nr, int val)
> > > >  			fanmode  |= (1 << F75387_FAN_MANU_MODE(nr));
> > > >  			fanmode  |= (1 << F75387_FAN_DUTY_MODE(nr));
> > > >  			break;
> > > > -		case 2: /* AUTOMATIC*/
> > > > -			fanmode  |=  (1 << F75387_FAN_DUTY_MODE(nr));
> > > > +		case 2: /* Automatic, speed mode */
> > > >  			break;
> > > >  		case 3: /* fan speed */
> > > >  			fanmode |= (1 << F75387_FAN_MANU_MODE(nr));
> > > >  			break;
> > > > +		case 4: /* Automatic, pwm */
> > > > +			fanmode  |=  (1 << F75387_FAN_DUTY_MODE(nr));
> > > 
> > > Might be time to get rid of those extra spaces.
> > 
> > I see no extra spaces.  You mean the extra parentheses?  I can update
> > the patch to drop them.
> > 
> There seem to be two spaces before and after |=.

Ah, got it :)

> > > > +			break;
> > > >  		}
> > > >  	} else {
> > > >  		/* clear each fanX_mode bit before setting them properly */
> > > > @@ -386,6 +407,8 @@ static int set_pwm_enable_direct(struct i2c_client *client, int nr, int val)
> > > >  			break;
> > > >  		case 3: /* fan speed */
> > > >  			break;
> > > > +		case 4: /* Automatic pwm */
> > > > +			return -EOPNOTSUPP;
> > > 
> > > 	Should be -EINVAL.
> > 
> > The value 4 is valid as it encodes an existing mode, it's just that this
> > code path handles hardware that does not support that mode.  Why should
> > the code return EINVAL?
> > 
> val is invalid for this hardware.

Ok, I consider that a valid argument :)

> Also it returned -EINVAL before you
> made your changes.

That one less so, since before, the value 4 was never valid for any
hardware, but it is now for the F75387.

But I'm okay making it return EINVAL.

Thanks for your review,
Nikolaus

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 5/6] hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enable

[Guenter Roeck]

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.

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

Guenter



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 5/6] hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enable

[Nikolaus Schulz]

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@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 5/6] hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enable

[Guenter Roeck]

On Thu, 2012-02-23 at 15:16 -0500, Nikolaus Schulz wrote:
> 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?
> 
Just keep the switch statement. Not worth the argument, and if someone
objected to the enum earlier I'd rather not revive that discussion.

Guenter



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors