Hi Guenter,
On Thu, 28 Oct 2010 09:01:42 -0700, Guenter Roeck wrote:
> On Thu, Oct 28, 2010 at 11:52:39AM -0400, Jean Delvare wrote:
> > The fan control feature of the w83795 driver is insufficiently
> > reviewed and tested for public consumption at this time, so make it
> > optional and disabled by default. We will change the default when
> > review and testing is deemed sufficient. Ultimately the option will
> > go away.
> >
> > Signed-off-by: Jean Delvare <khali@xxxxxxxxxxxx>
> > Cc: Guenter Roeck <guenter.roeck@xxxxxxxxxxxx>
>
> Minor comment below. Otherwise
> Acked-by: Guenter Roeck <guenter.roeck@xxxxxxxxxxxx>
>
> > ---
> > With this, the only non-standard attribute showing by default is "chassis". I'll
> > fix it now and send a patch immediately.
> >
> > drivers/hwmon/Kconfig | 17 +++++++++++++++++
> > drivers/hwmon/w83795.c | 10 ++++++++++
> > 2 files changed, 27 insertions(+)
> >
> > --- linux-2.6.37-rc0.orig/drivers/hwmon/Kconfig 2010-10-28 16:26:10.000000000 +0200
> > +++ linux-2.6.37-rc0/drivers/hwmon/Kconfig 2010-10-28 17:24:25.000000000 +0200
> > @@ -1041,6 +1041,23 @@ config SENSORS_W83795
> > 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)"
> > + 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.
> > +
> > + This part of the code wasn't carefully reviewed and tested yet,
> > + so enabling this option is strongly discouraged on production
> > + servers. Only developers and testers should enable it for the
> > + time being.
> > +
> > + Please also note that this option will create sysfs attribute
> > + files which may change in the future, so you shouldn't rely
> > + on them being stable.
> > +
> > config SENSORS_W83L785TS
> > tristate "Winbond W83L785TS-S"
> > depends on I2C && EXPERIMENTAL
> > --- linux-2.6.37-rc0.orig/drivers/hwmon/w83795.c 2010-10-28 16:41:32.000000000 +0200
> > +++ linux-2.6.37-rc0/drivers/hwmon/w83795.c 2010-10-28 17:39:12.000000000 +0200
> > @@ -1455,6 +1455,7 @@ store_in(struct device *dev, struct devi
> > }
> >
> >
> > +#ifdef CONFIG_SENSORS_W83795_FANCTRL
> > static ssize_t
> > show_sf_setup(struct device *dev, struct device_attribute *attr, char *buf)
> > {
> > @@ -1506,6 +1507,7 @@ store_sf_setup(struct device *dev, struc
> > mutex_unlock(&data->update_lock);
> > return count;
> > }
> > +#endif
> >
> >
> > #define NOT_USED -1
> > @@ -1711,6 +1713,7 @@ static const struct sensor_device_attrib
> > store_chassis_clear, ALARM_STATUS, 46),
> > SENSOR_ATTR_2(beep_enable, S_IWUSR | S_IRUGO, show_alarm_beep,
> > store_beep, BEEP_ENABLE, 47),
> > +#ifdef CONFIG_SENSORS_W83795_FANCTRL
> > SENSOR_ATTR_2(speed_cruise_tolerance, S_IWUSR | S_IRUGO, show_fanin,
> > store_fanin, FANIN_TOL, NOT_USED),
> > SENSOR_ATTR_2(pwm_default, S_IWUSR | S_IRUGO, show_sf_setup,
> > @@ -1719,6 +1722,7 @@ static const struct sensor_device_attrib
> > store_sf_setup, SETUP_PWM_UPTIME, NOT_USED),
> > SENSOR_ATTR_2(pwm_downtime, S_IWUSR | S_IRUGO, show_sf_setup,
> > store_sf_setup, SETUP_PWM_DOWNTIME, NOT_USED),
> > +#endif
> > };
> >
> > /*
> > @@ -1872,6 +1876,7 @@ static int w83795_handle_files(struct de
> > return err;
> > }
> >
> > +#ifdef CONFIG_SENSORS_W83795_FANCTRL
> > for (i = 0; i < data->has_pwm; i++) {
> > for (j = 0; j < ARRAY_SIZE(w83795_pwm[0]); j++) {
> > err = fn(dev, &w83795_pwm[i][j].dev_attr);
> > @@ -1879,11 +1884,16 @@ static int w83795_handle_files(struct de
> > return err;
> > }
> > }
> > +#endif
> >
> > for (i = 0; i < ARRAY_SIZE(w83795_temp); i++) {
> > if (!(data->has_temp & (1 << i)))
> > continue;
> > +#ifdef CONFIG_SENSORS_W83795_FANCTRL
> > for (j = 0; j < ARRAY_SIZE(w83795_temp[0]); j++) {
> > +#else
> > + for (j = 0; j < 8; j++) {
> > +#endif
>
> Can you keep using ARRAY_SIZE here and if necessary make array elements conditional ?
Not easily. The array elements are filled in through preprocessing, and
you can't put #ifdefs inside #defines. So we'd have to either duplicate
part of the macro (which I don't want to do) or split the macro
generating the elements.
I'm not particularly happy with the hard-coded 8 either, but the
alternative isn't very appealing. After splitting the macro, I also
have to comment out many callback functions to avoid build-time
warnings. Thus clutters the code further...
I end up with the following. Do you really thing this is preferable
over my first version? I don't, especially for an option we will get
rid of after some (hopefully short) time.
---
drivers/hwmon/Kconfig | 17 +++++++++++++++++
drivers/hwmon/w83795.c | 47 +++++++++++++++++++++++++++++++++++++----------
2 files changed, 54 insertions(+), 10 deletions(-)
--- linux-2.6.37-rc0.orig/drivers/hwmon/Kconfig 2010-10-28 16:26:10.000000000 +0200
+++ linux-2.6.37-rc0/drivers/hwmon/Kconfig 2010-10-28 17:24:25.000000000 +0200
@@ -1041,6 +1041,23 @@ config SENSORS_W83795
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)"
+ 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.
+
+ This part of the code wasn't carefully reviewed and tested yet,
+ so enabling this option is strongly discouraged on production
+ servers. Only developers and testers should enable it for the
+ time being.
+
+ Please also note that this option will create sysfs attribute
+ files which may change in the future, so you shouldn't rely
+ on them being stable.
+
config SENSORS_W83L785TS
tristate "Winbond W83L785TS-S"
depends on I2C && EXPERIMENTAL
--- linux-2.6.37-rc0.orig/drivers/hwmon/w83795.c 2010-10-28 16:41:32.000000000 +0200
+++ linux-2.6.37-rc0/drivers/hwmon/w83795.c 2010-10-28 19:44:01.000000000 +0200
@@ -914,6 +914,7 @@ store_pwm_enable(struct device *dev, str
return count;
}
+#ifdef CONFIG_SENSORS_W83795_FANCTRL
static ssize_t
show_temp_src(struct device *dev, struct device_attribute *attr, char *buf)
{
@@ -1029,6 +1030,7 @@ store_temp_pwm_enable(struct device *dev
}
return count;
}
+#endif
#define FANIN_TARGET 0
#define FANIN_TOL 1
@@ -1089,6 +1091,7 @@ store_fanin(struct device *dev, struct d
}
+#ifdef CONFIG_SENSORS_W83795_FANCTRL
static ssize_t
show_temp_pwm(struct device *dev, struct device_attribute *attr, char *buf)
{
@@ -1221,6 +1224,7 @@ store_sf4_temp(struct device *dev, struc
return count;
}
+#endif
static ssize_t
@@ -1455,6 +1459,7 @@ store_in(struct device *dev, struct devi
}
+#ifdef CONFIG_SENSORS_W83795_FANCTRL
static ssize_t
show_sf_setup(struct device *dev, struct device_attribute *attr, char *buf)
{
@@ -1506,6 +1511,7 @@ store_sf_setup(struct device *dev, struc
mutex_unlock(&data->update_lock);
return count;
}
+#endif
#define NOT_USED -1
@@ -1569,7 +1575,7 @@ store_sf_setup(struct device *dev, struc
SENSOR_ATTR_2(temp##index##_beep, S_IWUSR | S_IRUGO, \
show_alarm_beep, store_beep, BEEP_ENABLE, index + 17) }
-#define SENSOR_ATTR_TEMP(index) { \
+#define SENSOR_ATTR_TEMP(index) \
SENSOR_ATTR_2(temp##index##_type, S_IRUGO | (index < 4 ? S_IWUSR : 0), \
show_temp_mode, store_temp_mode, NOT_USED, index - 1), \
SENSOR_ATTR_2(temp##index##_input, S_IRUGO, show_temp, \
@@ -1587,7 +1593,10 @@ store_sf_setup(struct device *dev, struc
index + (index > 4 ? 11 : 17)), \
SENSOR_ATTR_2(temp##index##_beep, S_IWUSR | S_IRUGO, \
show_alarm_beep, store_beep, BEEP_ENABLE, \
- index + (index > 4 ? 11 : 17)), \
+ index + (index > 4 ? 11 : 17))
+
+#ifdef CONFIG_SENSORS_W83795_FANCTRL
+#define SENSOR_ATTR_TEMP_FANCTRL(index) \
SENSOR_ATTR_2(temp##index##_source_sel, S_IWUSR | S_IRUGO, \
show_temp_src, store_temp_src, NOT_USED, index - 1), \
SENSOR_ATTR_2(temp##index##_pwm_enable, S_IWUSR | S_IRUGO, \
@@ -1631,7 +1640,10 @@ store_sf_setup(struct device *dev, struc
SENSOR_ATTR_2(temp##index##_auto_point6_temp, S_IRUGO | S_IWUSR,\
show_sf4_temp, store_sf4_temp, 5, index - 1), \
SENSOR_ATTR_2(temp##index##_auto_point7_temp, S_IRUGO | S_IWUSR,\
- show_sf4_temp, store_sf4_temp, 6, index - 1) }
+ show_sf4_temp, store_sf4_temp, 6, index - 1)
+#else
+#define SENSOR_ATTR_TEMP_FANCTRL(index)
+#endif
static struct sensor_device_attribute_2 w83795_in[][5] = {
@@ -1675,13 +1687,24 @@ static const struct sensor_device_attrib
SENSOR_ATTR_FAN(14),
};
-static const struct sensor_device_attribute_2 w83795_temp[][29] = {
- SENSOR_ATTR_TEMP(1),
- SENSOR_ATTR_TEMP(2),
- SENSOR_ATTR_TEMP(3),
- SENSOR_ATTR_TEMP(4),
- SENSOR_ATTR_TEMP(5),
- SENSOR_ATTR_TEMP(6),
+static const
+#ifdef CONFIG_SENSORS_W83795_FANCTRL
+struct sensor_device_attribute_2 w83795_temp[][29] = {
+#else
+struct sensor_device_attribute_2 w83795_temp[][8] = {
+#endif
+ { SENSOR_ATTR_TEMP(1),
+ SENSOR_ATTR_TEMP_FANCTRL(1) },
+ { SENSOR_ATTR_TEMP(2),
+ SENSOR_ATTR_TEMP_FANCTRL(2) },
+ { SENSOR_ATTR_TEMP(3),
+ SENSOR_ATTR_TEMP_FANCTRL(3) },
+ { SENSOR_ATTR_TEMP(4),
+ SENSOR_ATTR_TEMP_FANCTRL(4) },
+ { SENSOR_ATTR_TEMP(5),
+ SENSOR_ATTR_TEMP_FANCTRL(5) },
+ { SENSOR_ATTR_TEMP(6),
+ SENSOR_ATTR_TEMP_FANCTRL(6) },
};
static const struct sensor_device_attribute_2 w83795_dts[][8] = {
@@ -1711,6 +1734,7 @@ static const struct sensor_device_attrib
store_chassis_clear, ALARM_STATUS, 46),
SENSOR_ATTR_2(beep_enable, S_IWUSR | S_IRUGO, show_alarm_beep,
store_beep, BEEP_ENABLE, 47),
+#ifdef CONFIG_SENSORS_W83795_FANCTRL
SENSOR_ATTR_2(speed_cruise_tolerance, S_IWUSR | S_IRUGO, show_fanin,
store_fanin, FANIN_TOL, NOT_USED),
SENSOR_ATTR_2(pwm_default, S_IWUSR | S_IRUGO, show_sf_setup,
@@ -1719,6 +1743,7 @@ static const struct sensor_device_attrib
store_sf_setup, SETUP_PWM_UPTIME, NOT_USED),
SENSOR_ATTR_2(pwm_downtime, S_IWUSR | S_IRUGO, show_sf_setup,
store_sf_setup, SETUP_PWM_DOWNTIME, NOT_USED),
+#endif
};
/*
@@ -1872,6 +1897,7 @@ static int w83795_handle_files(struct de
return err;
}
+#ifdef CONFIG_SENSORS_W83795_FANCTRL
for (i = 0; i < data->has_pwm; i++) {
for (j = 0; j < ARRAY_SIZE(w83795_pwm[0]); j++) {
err = fn(dev, &w83795_pwm[i][j].dev_attr);
@@ -1879,6 +1905,7 @@ static int w83795_handle_files(struct de
return err;
}
}
+#endif
for (i = 0; i < ARRAY_SIZE(w83795_temp); i++) {
if (!(data->has_temp & (1 << i)))
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
