[lm-sensors] hwmon: (w83795) Exclude fan control feature by default

Guenter Roeck guenter.roeck at ericsson.com
Thu Oct 28 20:00:05 CEST 2010


Hi Jean,

On Thu, 2010-10-28 at 13:52 -0400, Jean Delvare wrote:
> 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 at linux-fr.org>
> > > Cc: Guenter Roeck <guenter.roeck at ericsson.com>
> > 
> > Minor comment below. Otherwise
> > 	Acked-by: Guenter Roeck <guenter.roeck at ericsson.com>
> > 
> > > ---
> > > 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.
> 
That is much worse. I agree, better stick with the hardcoded value.

Guenter

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






More information about the lm-sensors mailing list