* [lm-sensors] [PATCH] hwmon: (w83795) Unconditionally support manual fan speed control
@ 2012-01-22 17:36 Jean Delvare
2012-01-22 17:52 ` Guenter Roeck
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Jean Delvare @ 2012-01-22 17:36 UTC (permalink / raw)
To: lm-sensors
Manual fan speed control uses a standard interface and has received
sufficient testing and review by now, it can be enabled
unconditionally. This includes attributes pwm[1-8], pwm[1-8]_enable,
pwm[1-8]_mode and pwm[1-8]_freq.
We only let the user switch from automatic mode to manual mode, but
not the other way around, as automatic control settings may not have
been set properly by the BIOS.
Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
drivers/hwmon/Kconfig | 9 +++++----
drivers/hwmon/w83795.c | 26 ++++++++++++++++++--------
2 files changed, 23 insertions(+), 12 deletions(-)
--- linux-3.2.orig/drivers/hwmon/w83795.c 2011-07-22 04:17:23.000000000 +0200
+++ linux-3.2/drivers/hwmon/w83795.c 2012-01-22 14:50:25.309726525 +0100
@@ -929,6 +929,14 @@ store_pwm_enable(struct device *dev, str
if (val < 1 || val > 2)
return -EINVAL;
+#ifndef CONFIG_SENSORS_W83795_FANCTRL
+ if (val > 1) {
+ dev_warn(dev, "Automatic fan speed control support disabled\n");
+ dev_warn(dev, "Build with CONFIG_SENSORS_W83795_FANCTRL=y if you want it\n");
+ return -EOPNOTSUPP;
+ }
+#endif
+
mutex_lock(&data->update_lock);
switch (val) {
case 1:
@@ -1625,18 +1633,18 @@ store_sf_setup(struct device *dev, struc
#define SENSOR_ATTR_PWM(index) { \
SENSOR_ATTR_2(pwm##index, S_IWUSR | S_IRUGO, show_pwm, \
store_pwm, PWM_OUTPUT, index - 1), \
+ SENSOR_ATTR_2(pwm##index##_enable, S_IWUSR | S_IRUGO, \
+ show_pwm_enable, store_pwm_enable, NOT_USED, index - 1), \
+ SENSOR_ATTR_2(pwm##index##_mode, S_IRUGO, \
+ show_pwm_mode, NULL, NOT_USED, index - 1), \
+ SENSOR_ATTR_2(pwm##index##_freq, S_IWUSR | S_IRUGO, \
+ show_pwm, store_pwm, PWM_FREQ, index - 1), \
SENSOR_ATTR_2(pwm##index##_nonstop, S_IWUSR | S_IRUGO, \
show_pwm, store_pwm, PWM_NONSTOP, index - 1), \
SENSOR_ATTR_2(pwm##index##_start, S_IWUSR | S_IRUGO, \
show_pwm, store_pwm, PWM_START, index - 1), \
SENSOR_ATTR_2(pwm##index##_stop_time, S_IWUSR | S_IRUGO, \
show_pwm, store_pwm, PWM_STOP_TIME, index - 1), \
- SENSOR_ATTR_2(pwm##index##_freq, S_IWUSR | S_IRUGO, \
- show_pwm, store_pwm, PWM_FREQ, index - 1), \
- SENSOR_ATTR_2(pwm##index##_enable, S_IWUSR | S_IRUGO, \
- show_pwm_enable, store_pwm_enable, NOT_USED, index - 1), \
- SENSOR_ATTR_2(pwm##index##_mode, S_IRUGO, \
- show_pwm_mode, NULL, NOT_USED, index - 1), \
SENSOR_ATTR_2(fan##index##_target, S_IWUSR | S_IRUGO, \
show_fanin, store_fanin, FANIN_TARGET, index - 1) }
@@ -2006,15 +2014,17 @@ static int w83795_handle_files(struct de
}
}
-#ifdef CONFIG_SENSORS_W83795_FANCTRL
for (i = 0; i < data->has_pwm; i++) {
+#ifdef CONFIG_SENSORS_W83795_FANCTRL
for (j = 0; j < ARRAY_SIZE(w83795_pwm[0]); j++) {
+#else
+ for (j = 0; j < 4; j++) {
+#endif
err = fn(dev, &w83795_pwm[i][j].dev_attr);
if (err)
return err;
}
}
-#endif
for (i = 0; i < ARRAY_SIZE(w83795_temp); i++) {
if (!(data->has_temp & (1 << i)))
--- linux-3.2.orig/drivers/hwmon/Kconfig 2012-01-06 11:21:10.000000000 +0100
+++ linux-3.2/drivers/hwmon/Kconfig 2012-01-21 17:37:49.102223035 +0100
@@ -1226,18 +1226,19 @@ config SENSORS_W83795
depends on I2C && EXPERIMENTAL
help
If you say yes here you get support for the Winbond W83795G and
- W83795ADG hardware monitoring chip.
+ W83795ADG hardware monitoring chip, including manual fan speed
+ control.
This driver can also be built as a module. If so, the module
will be called w83795.
config SENSORS_W83795_FANCTRL
- boolean "Include fan control support (DANGEROUS)"
+ boolean "Include automatic fan control support (DANGEROUS)"
depends on SENSORS_W83795 && EXPERIMENTAL
default n
help
- If you say yes here, support for the both manual and automatic
- fan control features will be included in the driver.
+ If you say yes here, support for automatic fan speed control
+ will be included in the driver.
This part of the code wasn't carefully reviewed and tested yet,
so enabling this option is strongly discouraged on production
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [lm-sensors] [PATCH] hwmon: (w83795) Unconditionally support manual fan speed control
2012-01-22 17:36 [lm-sensors] [PATCH] hwmon: (w83795) Unconditionally support manual fan speed control Jean Delvare
@ 2012-01-22 17:52 ` Guenter Roeck
2012-01-22 19:10 ` Jean Delvare
2012-01-22 20:04 ` Guenter Roeck
2 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2012-01-22 17:52 UTC (permalink / raw)
To: lm-sensors
Hi Jean,
On Sun, Jan 22, 2012 at 12:36:13PM -0500, Jean Delvare wrote:
> Manual fan speed control uses a standard interface and has received
> sufficient testing and review by now, it can be enabled
> unconditionally. This includes attributes pwm[1-8], pwm[1-8]_enable,
> pwm[1-8]_mode and pwm[1-8]_freq.
>
> We only let the user switch from automatic mode to manual mode, but
> not the other way around, as automatic control settings may not have
> been set properly by the BIOS.
>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> ---
> drivers/hwmon/Kconfig | 9 +++++----
> drivers/hwmon/w83795.c | 26 ++++++++++++++++++--------
> 2 files changed, 23 insertions(+), 12 deletions(-)
>
> --- linux-3.2.orig/drivers/hwmon/w83795.c 2011-07-22 04:17:23.000000000 +0200
> +++ linux-3.2/drivers/hwmon/w83795.c 2012-01-22 14:50:25.309726525 +0100
> @@ -929,6 +929,14 @@ store_pwm_enable(struct device *dev, str
> if (val < 1 || val > 2)
> return -EINVAL;
>
> +#ifndef CONFIG_SENSORS_W83795_FANCTRL
> + if (val > 1) {
> + dev_warn(dev, "Automatic fan speed control support disabled\n");
> + dev_warn(dev, "Build with CONFIG_SENSORS_W83795_FANCTRL=y if you want it\n");
> + return -EOPNOTSUPP;
> + }
> +#endif
> +
> mutex_lock(&data->update_lock);
> switch (val) {
> case 1:
> @@ -1625,18 +1633,18 @@ store_sf_setup(struct device *dev, struc
> #define SENSOR_ATTR_PWM(index) { \
> SENSOR_ATTR_2(pwm##index, S_IWUSR | S_IRUGO, show_pwm, \
> store_pwm, PWM_OUTPUT, index - 1), \
> + SENSOR_ATTR_2(pwm##index##_enable, S_IWUSR | S_IRUGO, \
> + show_pwm_enable, store_pwm_enable, NOT_USED, index - 1), \
> + SENSOR_ATTR_2(pwm##index##_mode, S_IRUGO, \
> + show_pwm_mode, NULL, NOT_USED, index - 1), \
> + SENSOR_ATTR_2(pwm##index##_freq, S_IWUSR | S_IRUGO, \
> + show_pwm, store_pwm, PWM_FREQ, index - 1), \
> SENSOR_ATTR_2(pwm##index##_nonstop, S_IWUSR | S_IRUGO, \
> show_pwm, store_pwm, PWM_NONSTOP, index - 1), \
> SENSOR_ATTR_2(pwm##index##_start, S_IWUSR | S_IRUGO, \
> show_pwm, store_pwm, PWM_START, index - 1), \
> SENSOR_ATTR_2(pwm##index##_stop_time, S_IWUSR | S_IRUGO, \
> show_pwm, store_pwm, PWM_STOP_TIME, index - 1), \
> - SENSOR_ATTR_2(pwm##index##_freq, S_IWUSR | S_IRUGO, \
> - show_pwm, store_pwm, PWM_FREQ, index - 1), \
> - SENSOR_ATTR_2(pwm##index##_enable, S_IWUSR | S_IRUGO, \
> - show_pwm_enable, store_pwm_enable, NOT_USED, index - 1), \
> - SENSOR_ATTR_2(pwm##index##_mode, S_IRUGO, \
> - show_pwm_mode, NULL, NOT_USED, index - 1), \
> SENSOR_ATTR_2(fan##index##_target, S_IWUSR | S_IRUGO, \
> show_fanin, store_fanin, FANIN_TARGET, index - 1) }
>
> @@ -2006,15 +2014,17 @@ static int w83795_handle_files(struct de
> }
> }
>
> -#ifdef CONFIG_SENSORS_W83795_FANCTRL
> for (i = 0; i < data->has_pwm; i++) {
> +#ifdef CONFIG_SENSORS_W83795_FANCTRL
> for (j = 0; j < ARRAY_SIZE(w83795_pwm[0]); j++) {
> +#else
> + for (j = 0; j < 4; j++) {
> +#endif
In situations like this I came to prefer something like
#ifdef CONFIG_SENSORS_W83795_FANCTRL
#define NUM_PWM_ATTRIBUTES ARRAY_SIZE(w83795_pwm[0])
#else
#define NUM_PWM_ATTRIBUTES 4
#endif
...
for (j = 0; j < NUM_PWM_ATTRIBUTES; j++)
to avoid the ifdef in the code, especially in situations like this where opening
and closing brackets no longer match due to the ifdef.
Other than that,
Acked-by: Guenter Roeck <guenter.roeck@ericsson.com>
I guess this should be independent of the multiline comment patch I sent out earlier.
Would be good, though, if both are in the same tree. Do you want me to take both patches,
or do you want to take them ? Let me know.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [lm-sensors] [PATCH] hwmon: (w83795) Unconditionally support manual fan speed control
2012-01-22 17:36 [lm-sensors] [PATCH] hwmon: (w83795) Unconditionally support manual fan speed control Jean Delvare
2012-01-22 17:52 ` Guenter Roeck
@ 2012-01-22 19:10 ` Jean Delvare
2012-01-22 20:04 ` Guenter Roeck
2 siblings, 0 replies; 4+ messages in thread
From: Jean Delvare @ 2012-01-22 19:10 UTC (permalink / raw)
To: lm-sensors
On Sun, 22 Jan 2012 09:52:10 -0800, Guenter Roeck wrote:
> Hi Jean,
>
> On Sun, Jan 22, 2012 at 12:36:13PM -0500, Jean Delvare wrote:
> > Manual fan speed control uses a standard interface and has received
> > sufficient testing and review by now, it can be enabled
> > unconditionally. This includes attributes pwm[1-8], pwm[1-8]_enable,
> > pwm[1-8]_mode and pwm[1-8]_freq.
> >
> > We only let the user switch from automatic mode to manual mode, but
> > not the other way around, as automatic control settings may not have
> > been set properly by the BIOS.
> >
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > ---
> > drivers/hwmon/Kconfig | 9 +++++----
> > drivers/hwmon/w83795.c | 26 ++++++++++++++++++--------
> > 2 files changed, 23 insertions(+), 12 deletions(-)
> > (...)
> > @@ -2006,15 +2014,17 @@ static int w83795_handle_files(struct de
> > }
> > }
> >
> > -#ifdef CONFIG_SENSORS_W83795_FANCTRL
> > for (i = 0; i < data->has_pwm; i++) {
> > +#ifdef CONFIG_SENSORS_W83795_FANCTRL
> > for (j = 0; j < ARRAY_SIZE(w83795_pwm[0]); j++) {
> > +#else
> > + for (j = 0; j < 4; j++) {
> > +#endif
>
> In situations like this I came to prefer something like
>
> #ifdef CONFIG_SENSORS_W83795_FANCTRL
> #define NUM_PWM_ATTRIBUTES ARRAY_SIZE(w83795_pwm[0])
> #else
> #define NUM_PWM_ATTRIBUTES 4
> #endif
> ...
> for (j = 0; j < NUM_PWM_ATTRIBUTES; j++)
>
> to avoid the ifdef in the code, especially in situations like this where opening
> and closing brackets no longer match due to the ifdef.
Agreed, I'll update the patch that way and resend.
> Other than that,
>
> Acked-by: Guenter Roeck <guenter.roeck@ericsson.com>
>
> I guess this should be independent of the multiline comment patch I sent out earlier.
> Would be good, though, if both are in the same tree. Do you want me to take both patches,
> or do you want to take them ? Let me know.
They are independent indeed, but I agree with your reasoning still. As
I maintain the w83795 driver I propose that I pick both patches in my
tree.
Thanks,
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [lm-sensors] [PATCH] hwmon: (w83795) Unconditionally support manual fan speed control
2012-01-22 17:36 [lm-sensors] [PATCH] hwmon: (w83795) Unconditionally support manual fan speed control Jean Delvare
2012-01-22 17:52 ` Guenter Roeck
2012-01-22 19:10 ` Jean Delvare
@ 2012-01-22 20:04 ` Guenter Roeck
2 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2012-01-22 20:04 UTC (permalink / raw)
To: lm-sensors
On Sun, Jan 22, 2012 at 02:10:31PM -0500, Jean Delvare wrote:
> On Sun, 22 Jan 2012 09:52:10 -0800, Guenter Roeck wrote:
> > Hi Jean,
> >
> > On Sun, Jan 22, 2012 at 12:36:13PM -0500, Jean Delvare wrote:
> > > Manual fan speed control uses a standard interface and has received
> > > sufficient testing and review by now, it can be enabled
> > > unconditionally. This includes attributes pwm[1-8], pwm[1-8]_enable,
> > > pwm[1-8]_mode and pwm[1-8]_freq.
> > >
> > > We only let the user switch from automatic mode to manual mode, but
> > > not the other way around, as automatic control settings may not have
> > > been set properly by the BIOS.
> > >
> > > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > > ---
> > > drivers/hwmon/Kconfig | 9 +++++----
> > > drivers/hwmon/w83795.c | 26 ++++++++++++++++++--------
> > > 2 files changed, 23 insertions(+), 12 deletions(-)
> > > (...)
> > > @@ -2006,15 +2014,17 @@ static int w83795_handle_files(struct de
> > > }
> > > }
> > >
> > > -#ifdef CONFIG_SENSORS_W83795_FANCTRL
> > > for (i = 0; i < data->has_pwm; i++) {
> > > +#ifdef CONFIG_SENSORS_W83795_FANCTRL
> > > for (j = 0; j < ARRAY_SIZE(w83795_pwm[0]); j++) {
> > > +#else
> > > + for (j = 0; j < 4; j++) {
> > > +#endif
> >
> > In situations like this I came to prefer something like
> >
> > #ifdef CONFIG_SENSORS_W83795_FANCTRL
> > #define NUM_PWM_ATTRIBUTES ARRAY_SIZE(w83795_pwm[0])
> > #else
> > #define NUM_PWM_ATTRIBUTES 4
> > #endif
> > ...
> > for (j = 0; j < NUM_PWM_ATTRIBUTES; j++)
> >
> > to avoid the ifdef in the code, especially in situations like this where opening
> > and closing brackets no longer match due to the ifdef.
>
> Agreed, I'll update the patch that way and resend.
>
> > Other than that,
> >
> > Acked-by: Guenter Roeck <guenter.roeck@ericsson.com>
> >
> > I guess this should be independent of the multiline comment patch I sent out earlier.
> > Would be good, though, if both are in the same tree. Do you want me to take both patches,
> > or do you want to take them ? Let me know.
>
> They are independent indeed, but I agree with your reasoning still. As
> I maintain the w83795 driver I propose that I pick both patches in my
> tree.
>
Ok, I'll drop it from mine.
Thanks,
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-01-22 20:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-22 17:36 [lm-sensors] [PATCH] hwmon: (w83795) Unconditionally support manual fan speed control Jean Delvare
2012-01-22 17:52 ` Guenter Roeck
2012-01-22 19:10 ` Jean Delvare
2012-01-22 20:04 ` Guenter Roeck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.