hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enable
PATCH v2 3/4 hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enable
From: Nikolaus Schulz <[email protected]>
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.
--- drivers/hwmon/f75375s.c | 42 ++++++++++++++++++++++++++++++++++-------- 1 files changed, 34 insertions(+), 8 deletions(-)
drivers/hwmon/f75375s.c +33 -8
@@ -263,6 +263,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) {
@@ -336,11 +351,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));
@@ -354,12 +373,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 */
@@ -377,6 +398,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 -EINVAL ; } }
@@ -734,14 +757,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))))
Signature · 7 lines
1.7.9.1 _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Re: [lm-sensors] [PATCH v2 3/4] hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enab
Quoted 12 lines · Nikolaus Schulz
From: Nikolaus Schulz <[email protected]>
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_enableattribute 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.
Quoted 6 lines
Toggling the duty mode via
pwm_enableis 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 <[email protected]>
Nikolaus,
I don't feel comfortable adding this patch to 3.3 at this point; it is not a pure bug fix but a functionality enhancement. We can either wait for 3.4, or split it into two patches (if possible), one to fix <automatic, speed mode> for 3.3 and one to add <automatic, pwm> for 3.4.
Any preference ?
Thanks,
Guenter
Quoted 98 lines
--- drivers/hwmon/f75375s.c | 42 ++++++++++++++++++++++++++++++++++-------- 1 files changed, 34 insertions(+), 8 deletions(-)drivers/hwmon/f75375s.c +33
-8@@ -263,6 +263,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) {@@ -336,11 +351,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));@@ -354,12 +373,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 */ @@ -377,6 +398,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 -EINVAL ; } }@@ -734,14 +757,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))))Signature · 1 line
1.7.9.1
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Guenter Roeck
AppliedRe: [lm-sensors] [PATCH v2 3/4] hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enab
In reply to Nikolaus SchulzQuoted 12 lines · Nikolaus Schulz
From: Nikolaus Schulz <[email protected]>
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_enableattribute 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.
Quoted 6 lines
Toggling the duty mode via
pwm_enableis 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 <[email protected]>
Applied to -next.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Re: [lm-sensors] [PATCH v2 3/4] hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enab
In reply to Nikolaus SchulzQuoted 18 lines · Guenter Roeck
Quoted 17 lines · Nikolaus Schulz
From: Nikolaus Schulz <[email protected]>
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_enableattribute 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_enableis 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.
Quoted 6 lines
Quoted 1 line
Signed-off-by Nikolaus Schulz <[email protected]>Nikolaus,
I don't feel comfortable adding this patch to 3.3 at this point; it is not a pure bug fix but a functionality enhancement.
The patch is indeed quite invasive for v3.3, but I think it is necessary in this form in order to fix the current code. Let me explain. (Be warned, this is going to be verbose.)
The F75373 & F75375 chips support three modes:
a) Closed loop, user specifies RPM.
Let's call that "manual speed mode".
b) Open loop, user specifies PWM aka duty cycle. ("manual duty mode")
c) Automatic temperature mode, user may configure RPM depending on temperature, this is again a closed loop mode. ("automatic speed mode")
One can distinguish these modes by two boolean switches: duty mode vs speed mode (aka open loop vs closed loop), and auto vs. manual (temperature-drive or not).
Automatic duty mode is not supported by the F75373 & F75375, and in the driver, the three supported modes map to the values 1, 2 and 3 for pwm*_enable.
But the F75387 is different as it supports all four possible modes. Thus, to express these in sysfs, we need to extend the range of valid values for pwm*_enable. Note that any mode might have been enabled by the BIOS at system startup, so the driver really should detect and report all four modes correctly.
Currently, for the F75387, f75375_init does not initialize pwm*_enable
correctly: it identifies manual duty mode as manual speed mode, and identifies both manual speed mode and auto speed mode as manual mode. I think this should be fixed in v3.3 to identify all four modes correctly. - This is the first change in my patch.
Now, for the F75387 the matter is slightly complicated by the fact that the same registers are used to configure the open and closed loop modes. So, at any point in time, these registers can only hold the configuration for one of these modes - and they make sense only for that mode.
Thus, switching from an open loop mode to a closed loop mode and vice versa is generally dangerous for this chip, because the PWM values in the configuration register(s) are then simply reinterpreted as RPM and vice versa. Which is of course bogus. Moreover, changing the configuration registers for auto mode is not (yet) supported by the driver, so we're stuck here with the values provided by the BIOS. And the BIOS might have configured any auto mode, either duty or speed mode.
The current code lets the user switch to automatic duty mode only, and does not provide a way to activate auto speed mode. This might enable a bogus configuration, with uncertain consequences.
For these reasons, until a safe way to support switching between open loop mode and closed loop mode is implemented in the driver, my patch inhibits such a mode switch. - This is the second change in my patch.
Moreover, when setting modes via writes to pwm*_enable, the current mapping of values to modes is inconsistent: for the F75387, 2 maps to automatic duty mode, while for the F75373 & F75375, it maps to automatic speed mode. IMO this should be fixed in v3.3 to avoid breaking the API later. - This is the third change in my patch (touching the same area as the first).
Quoted 5 lines
We can either wait for 3.4, or split it into
two patches (if possible), one to fix <automatic, speed mode> for 3.3 and one to add <automatic, pwm> for 3.4.Any preference ?
Given my reasoning above, I consider this more a fix of seriously broken functionality, rather than an addition of new functionality. We simply cannot ignore the automatic pwm mode, because it might already be enabled by the BIOS. The board I use for testing does so, for example.
So I really think this should go into v3.3.
However, it might make sense to split out the inhibiting of switches between open and closed loop modes.
-- Nikolaus
Quoted 90 lines
Quoted 90 lines
--- drivers/hwmon/f75375s.c | 42 ++++++++++++++++++++++++++++++++++-------- 1 files changed, 34 insertions(+), 8 deletions(-)drivers/hwmon/f75375s.c +32
-7@@ -263,6 +263,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) {@@ -336,11 +351,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));@@ -354,12 +373,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 */ @@ -377,6 +398,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 -EINVAL ; } }@@ -734,14 +757,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
Quoted 9 lines
Quoted 8 lines
drivers/hwmon/f75375s.c +1
-1-/* manual */ +/* manual, pwm */ data->pwm_enable[nr] =1 ; }else {if (!(conf & (1 <<F75375_FAN_CTRL_LINEAR (nr))))Signature · 1 line
1.7.9.1
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Re: [lm-sensors] [PATCH v2 3/4] hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enab
In reply to Nikolaus SchulzNikolaus,
Quoted 101 lines · Nikolaus Schulz
Quoted 18 lines · Guenter Roeck
Quoted 17 lines · Nikolaus Schulz
From: Nikolaus Schulz <[email protected]>
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_enableattribute 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_enableis 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.Quoted 6 lines
Quoted 1 line
Signed-off-by Nikolaus Schulz <[email protected]>Nikolaus,
I don't feel comfortable adding this patch to 3.3 at this point; it is not a pure bug fix but a functionality enhancement.
The patch is indeed quite invasive for v3.3, but I think it is necessary in this form in order to fix the current code. Let me explain. (Be warned, this is going to be verbose.)
The F75373 & F75375 chips support three modes:
a) Closed loop, user specifies RPM.
Let's call that "manual speed mode".
b) Open loop, user specifies PWM aka duty cycle. ("manual duty mode")
c) Automatic temperature mode, user may configure RPM depending on temperature, this is again a closed loop mode. ("automatic speed mode")One can distinguish these modes by two boolean switches: duty mode vs speed mode (aka open loop vs closed loop), and auto vs. manual (temperature-drive or not).
Automatic duty mode is not supported by the F75373 & F75375, and in the driver, the three supported modes map to the values 1, 2 and 3 for pwm*_enable.
But the F75387 is different as it supports all four possible modes. Thus, to express these in sysfs, we need to extend the range of valid values for pwm*_enable. Note that any mode might have been enabled by the BIOS at system startup, so the driver really should detect and report all four modes correctly.
Currently, for the F75387,
f75375_initdoes not initialize pwm*_enable
correctly: it identifies manual duty mode as manual speed mode, and identifies both manual speed mode and auto speed mode as manual mode. I think this should be fixed in v3.3 to identify all four modes correctly. - This is the first change in my patch.Now, for the F75387 the matter is slightly complicated by the fact that the same registers are used to configure the open and closed loop modes. So, at any point in time, these registers can only hold the configuration for one of these modes - and they make sense only for that mode.
Thus, switching from an open loop mode to a closed loop mode and vice versa is generally dangerous for this chip, because the PWM values in the configuration register(s) are then simply reinterpreted as RPM and vice versa. Which is of course bogus. Moreover, changing the configuration registers for auto mode is not (yet) supported by the driver, so we're stuck here with the values provided by the BIOS. And the BIOS might have configured any auto mode, either duty or speed mode.
The current code lets the user switch to automatic duty mode only, and does not provide a way to activate auto speed mode. This might enable a bogus configuration, with uncertain consequences.
For these reasons, until a safe way to support switching between open loop mode and closed loop mode is implemented in the driver, my patch inhibits such a mode switch. - This is the second change in my patch.
Moreover, when setting modes via writes to pwm*_enable, the current mapping of values to modes is inconsistent: for the F75387, 2 maps to automatic duty mode, while for the F75373 & F75375, it maps to automatic speed mode. IMO this should be fixed in v3.3 to avoid breaking the API later. - This is the third change in my patch (touching the same area as the first).
Quoted 5 lines
We can either wait for 3.4, or split it into
two patches (if possible), one to fix <automatic, speed mode> for 3.3 and one to add <automatic, pwm> for 3.4.Any preference ?
Given my reasoning above, I consider this more a fix of seriously broken functionality, rather than an addition of new functionality. We simply cannot ignore the automatic pwm mode, because it might already be enabled by the BIOS. The board I use for testing does so, for example.
Quoted 5 lines
So I really think this should go into v3.3.
However, it might make sense to split out the inhibiting of switches between open and closed loop modes.
That is why I suggested to split the patch into two parts. I don't mind a fix for pwm_mode=3, limiting its scope. My concern is that pwm_mode=4 adds functionality which I would prefer to delay until 3.4.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Re: [lm-sensors] [PATCH v2 3/4] hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enab
In reply to Nikolaus SchulzQuoted 104 lines · Guenter Roeck
Nikolaus,
Quoted 101 lines · Nikolaus Schulz
Quoted 18 lines · Guenter Roeck
Quoted 17 lines · Nikolaus Schulz
From: Nikolaus Schulz <[email protected]>
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_enableattribute 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_enableis 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.Quoted 6 lines
Quoted 1 line
Signed-off-by Nikolaus Schulz <[email protected]>Nikolaus,
I don't feel comfortable adding this patch to 3.3 at this point; it is not a pure bug fix but a functionality enhancement.
The patch is indeed quite invasive for v3.3, but I think it is necessary in this form in order to fix the current code. Let me explain. (Be warned, this is going to be verbose.)
The F75373 & F75375 chips support three modes:
a) Closed loop, user specifies RPM.
Let's call that "manual speed mode".
b) Open loop, user specifies PWM aka duty cycle. ("manual duty mode")
c) Automatic temperature mode, user may configure RPM depending on temperature, this is again a closed loop mode. ("automatic speed mode")One can distinguish these modes by two boolean switches: duty mode vs speed mode (aka open loop vs closed loop), and auto vs. manual (temperature-drive or not).
Automatic duty mode is not supported by the F75373 & F75375, and in the driver, the three supported modes map to the values 1, 2 and 3 for pwm*_enable.
But the F75387 is different as it supports all four possible modes. Thus, to express these in sysfs, we need to extend the range of valid values for pwm*_enable. Note that any mode might have been enabled by the BIOS at system startup, so the driver really should detect and report all four modes correctly.
Currently, for the F75387,
f75375_initdoes not initialize pwm*_enable
correctly: it identifies manual duty mode as manual speed mode, and identifies both manual speed mode and auto speed mode as manual mode. I think this should be fixed in v3.3 to identify all four modes correctly. - This is the first change in my patch.Now, for the F75387 the matter is slightly complicated by the fact that the same registers are used to configure the open and closed loop modes. So, at any point in time, these registers can only hold the configuration for one of these modes - and they make sense only for that mode.
Thus, switching from an open loop mode to a closed loop mode and vice versa is generally dangerous for this chip, because the PWM values in the configuration register(s) are then simply reinterpreted as RPM and vice versa. Which is of course bogus. Moreover, changing the configuration registers for auto mode is not (yet) supported by the driver, so we're stuck here with the values provided by the BIOS. And the BIOS might have configured any auto mode, either duty or speed mode.
The current code lets the user switch to automatic duty mode only, and does not provide a way to activate auto speed mode. This might enable a bogus configuration, with uncertain consequences.
For these reasons, until a safe way to support switching between open loop mode and closed loop mode is implemented in the driver, my patch inhibits such a mode switch. - This is the second change in my patch.
Moreover, when setting modes via writes to pwm*_enable, the current mapping of values to modes is inconsistent: for the F75387, 2 maps to automatic duty mode, while for the F75373 & F75375, it maps to automatic speed mode. IMO this should be fixed in v3.3 to avoid breaking the API later. - This is the third change in my patch (touching the same area as the first).
Quoted 5 lines
We can either wait for 3.4, or split it into
two patches (if possible), one to fix <automatic, speed mode> for 3.3 and one to add <automatic, pwm> for 3.4.Any preference ?
Given my reasoning above, I consider this more a fix of seriously broken functionality, rather than an addition of new functionality. We simply cannot ignore the automatic pwm mode, because it might already be enabled by the BIOS. The board I use for testing does so, for example.
Quoted 8 lines
Quoted 5 lines
So I really think this should go into v3.3.
However, it might make sense to split out the inhibiting of switches between open and closed loop modes.
That is why I suggested to split the patch into two parts. I don't mind a fix for pwm_mode=3, limiting its scope. My concern is that pwm_mode=4 adds functionality which I would prefer to delay until 3.4.
Well, as I have said, this functionality may already be activated by the BIOS. The driver has to deal with it, right? It's not so much a matter of supporting something, but a matter of not messing it up.
Nikolaus
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Re: [lm-sensors] [PATCH v2 3/4] hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enab
In reply to Nikolaus SchulzOn Fri, 2012-03-02 at 14:47 -0500, Nikolaus Schulz wrote:
[ ... ]
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Given my reasoning above, I consider this more a fix of seriously broken functionality, rather than an addition of new functionality. We simply cannot ignore the automatic pwm mode, because it might already be enabled by the BIOS. The board I use for testing does so, for example.
So I really think this should go into v3.3.
However, it might make sense to split out the inhibiting of switches between open and closed loop modes.
Quoted 8 lines
Quoted 3 lines
That is why I suggested to split the patch into two parts. I don't mind a fix for pwm_mode=3, limiting its scope. My concern is that pwm_mode=4 adds functionality which I would prefer to delay until 3.4.
Well, as I have said, this functionality may already be activated by the BIOS. The driver has to deal with it, right? It's not so much a matter of supporting something, but a matter of not messing it up.
Hmm - good point. I'll accept that. I'll add your reasoning to the patch description and apply it to 3.3.
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Re: [lm-sensors] [PATCH v2 3/4] hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enab
In reply to Nikolaus SchulzQuoted 11 lines · Guenter Roeck
Quoted 8 lines · Nikolaus Schulz
Quoted 3 lines
That is why I suggested to split the patch into two parts. I don't mind a fix for pwm_mode=3, limiting its scope. My concern is that pwm_mode=4 adds functionality which I would prefer to delay until 3.4.
Well, as I have said, this functionality may already be activated by the BIOS. The driver has to deal with it, right? It's not so much a matter of supporting something, but a matter of not messing it up.
Hmm - good point. I'll accept that. I'll add your reasoning to the patch description and apply it to 3.3.
Thank you. And yes, adding an explicit note about that to the commit message is a good idea. I should have been more clear there.
Sorry for bugging you some more, do you consider 331255d35d6f517020485aee38dbb8b8dfaa1642 "hwmon: (f75375s) Fix writes to the pwm* attribute for the F75387" for v3.3? I think it should go in.
Thanks,
Nikolaus
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Re: [lm-sensors] [PATCH v2 3/4] hwmon: (f75375s) Properly map the F75387 automatic modes to pwm_enab
In reply to Nikolaus SchulzQuoted 13 lines · Nikolaus Schulz
Quoted 11 lines · Guenter Roeck
Quoted 8 lines · Nikolaus Schulz
Quoted 3 lines
That is why I suggested to split the patch into two parts. I don't mind a fix for pwm_mode=3, limiting its scope. My concern is that pwm_mode=4 adds functionality which I would prefer to delay until 3.4.
Well, as I have said, this functionality may already be activated by the BIOS. The driver has to deal with it, right? It's not so much a matter of supporting something, but a matter of not messing it up.
Hmm - good point. I'll accept that. I'll add your reasoning to the patch description and apply it to 3.3.
Quoted 7 lines
Thank you. And yes, adding an explicit note about that to the commit message is a good idea. I should have been more clear there.
Sorry for bugging you some more, do you consider
331255d35d6f517020485aee38dbb8b8dfaa1642"hwmon: (f75375s) Fix writes to the pwm* attribute for the F75387" for v3.3? I think it should go in.
Yes.
Guenter
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors