[PATCH v3 0/4] hwmon: (adt7475) small enhancements

[PATCH v3 0/4] hwmon: (adt7475) small enhancements

[Chris Packham]

This is a series of small enhancements to the adt7475 driver. We're using an
adt7476 chip in several network switches and generally speaking the
requirement for this type of equipment is to keep the fan moving so that we
know it is working when we need it.

I think the only contentious one is 2/4. The desire to keep the fans running is
understood but the attribute/implementation are being debated.

I think the others stand on their own merits.

Chris Packham (4):
  hwmon: (adt7475) replace find_nearest() with find_closest()
  hwmon: (adt7475) fan stall prevention
  hwmon: (adt7475) temperature smoothing
  hwmon: (adt7475) add high frequency support

 Documentation/hwmon/adt7475 |   9 +++
 drivers/hwmon/adt7475.c     | 186 +++++++++++++++++++++++++++++++++++---------
 2 files changed, 160 insertions(+), 35 deletions(-)

-- 
2.11.0.24.ge6920cf

[PATCH v3 1/4] hwmon: (adt7475) replace find_nearest() with find_closest()

[Chris Packham]

The adt7475 has had find_nearest() since it's creation in 2009. Since
then find_closest() has been introduced and several drivers have been
updated to use it. Update the adt7475 to use find_closest() and remove
the now unused find_nearest().

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
Changes in v3:
- None

 drivers/hwmon/adt7475.c | 34 +++-------------------------------
 1 file changed, 3 insertions(+), 31 deletions(-)

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index c803e3c5fcd4..ec0c43fbcdce 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -22,6 +22,7 @@
 #include <linux/hwmon-vid.h>
 #include <linux/err.h>
 #include <linux/jiffies.h>
+#include <linux/util_macros.h>
 
 /* Indexes for the sysfs hooks */
 
@@ -314,35 +315,6 @@ static void adt7475_write_word(struct i2c_client *client, int reg, u16 val)
 	i2c_smbus_write_byte_data(client, reg, val & 0xFF);
 }
 
-/*
- * Find the nearest value in a table - used for pwm frequency and
- * auto temp range
- */
-static int find_nearest(long val, const int *array, int size)
-{
-	int i;
-
-	if (val < array[0])
-		return 0;
-
-	if (val > array[size - 1])
-		return size - 1;
-
-	for (i = 0; i < size - 1; i++) {
-		int a, b;
-
-		if (val > array[i + 1])
-			continue;
-
-		a = val - array[i];
-		b = array[i + 1] - val;
-
-		return (a <= b) ? i : i + 1;
-	}
-
-	return 0;
-}
-
 static ssize_t show_voltage(struct device *dev, struct device_attribute *attr,
 			    char *buf)
 {
@@ -606,7 +578,7 @@ static ssize_t set_point2(struct device *dev, struct device_attribute *attr,
 	val -= temp;
 
 	/* Find the nearest table entry to what the user wrote */
-	val = find_nearest(val, autorange_table, ARRAY_SIZE(autorange_table));
+	val = find_closest(val, autorange_table, ARRAY_SIZE(autorange_table));
 
 	data->range[sattr->index] &= ~0xF0;
 	data->range[sattr->index] |= val << 4;
@@ -864,7 +836,7 @@ static ssize_t set_pwmfreq(struct device *dev, struct device_attribute *attr,
 	if (kstrtol(buf, 10, &val))
 		return -EINVAL;
 
-	out = find_nearest(val, pwmfreq_table, ARRAY_SIZE(pwmfreq_table));
+	out = find_closest(val, pwmfreq_table, ARRAY_SIZE(pwmfreq_table));
 
 	mutex_lock(&data->lock);
 
-- 
2.11.0.24.ge6920cf

[PATCH v3 2/4] hwmon: (adt7475) fan stall prevention

[Chris Packham]

By default adt7475 will stop the fans (pwm duty cycle 0%) when the
temperature drops past Tmin - hysteresis. Some systems want to keep the
fans moving even when the temperature drops so add new sysfs attributes
that configure the enhanced acoustics min 1-3 which allows the fans to
run at the minimum configure pwm duty cycle.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Changes in v2:
- use pwmN_stall_dis as the attribute name. I think this describes the purpose
  pretty well. I went with a new attribute instead of overloading
  pwmN_auto_point1_pwm so this doesn't affect existing users.
Changes in v3:
- Fix grammar.
- change enh_acou to enh_acoustics

 Documentation/hwmon/adt7475 |  5 +++++
 drivers/hwmon/adt7475.c     | 50 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/Documentation/hwmon/adt7475 b/Documentation/hwmon/adt7475
index 0502f2b464e1..3990bae60e78 100644
--- a/Documentation/hwmon/adt7475
+++ b/Documentation/hwmon/adt7475
@@ -109,6 +109,11 @@ fan speed) is applied. PWM values range from 0 (off) to 255 (full speed).
 Fan speed may be set to maximum when the temperature sensor associated with
 the PWM control exceeds temp#_max.
 
+At Tmin - hysteresis the PWM output can either be off (0% duty cycle) or at the
+minimum (i.e. auto_point1_pwm). This behaviour can be configured using the
+pwm[1-*]_stall_dis sysfs attribute. A value of 0 means the fans will shut off.
+A value of 1 means the fans will run at auto_point1_pwm.
+
 Notes
 -----
 
diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index ec0c43fbcdce..4d6c625fec70 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -79,6 +79,9 @@
 
 #define REG_TEMP_TRANGE_BASE	0x5F
 
+#define REG_ENHANCE_ACOUSTICS1	0x62
+#define REG_ENHANCE_ACOUSTICS2	0x63
+
 #define REG_PWM_MIN_BASE	0x64
 
 #define REG_TEMP_TMIN_BASE	0x67
@@ -209,6 +212,7 @@ struct adt7475_data {
 	u8 range[3];
 	u8 pwmctl[3];
 	u8 pwmchan[3];
+	u8 enh_acoustics[2];
 
 	u8 vid;
 	u8 vrm;
@@ -700,6 +704,43 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
 	data->pwm[sattr->nr][sattr->index] = clamp_val(val, 0, 0xFF);
 	i2c_smbus_write_byte_data(client, reg,
 				  data->pwm[sattr->nr][sattr->index]);
+	mutex_unlock(&data->lock);
+
+	return count;
+}
+
+
+static ssize_t show_stall_dis(struct device *dev, struct device_attribute *attr,
+			      char *buf)
+{
+	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+	struct i2c_client *client = to_i2c_client(dev);
+	struct adt7475_data *data = i2c_get_clientdata(client);
+	u8 mask = BIT(5 + sattr->index);
+
+	return sprintf(buf, "%d\n", !!(data->enh_acoustics[0] & mask));
+}
+
+static ssize_t set_stall_dis(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t count)
+{
+	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+	struct i2c_client *client = to_i2c_client(dev);
+	struct adt7475_data *data = i2c_get_clientdata(client);
+	long val;
+	u8 mask = BIT(5 + sattr->index);
+
+	if (kstrtol(buf, 10, &val))
+		return -EINVAL;
+
+	mutex_lock(&data->lock);
+
+	data->enh_acoustics[0] &= ~mask;
+	if (val)
+		data->enh_acoustics[0] |= mask;
+
+	i2c_smbus_write_byte_data(client, REG_ENHANCE_ACOUSTICS1,
+				  data->enh_acoustics[0]);
 
 	mutex_unlock(&data->lock);
 
@@ -1028,6 +1069,8 @@ static SENSOR_DEVICE_ATTR_2(pwm1_auto_point1_pwm, S_IRUGO | S_IWUSR, show_pwm,
 			    set_pwm, MIN, 0);
 static SENSOR_DEVICE_ATTR_2(pwm1_auto_point2_pwm, S_IRUGO | S_IWUSR, show_pwm,
 			    set_pwm, MAX, 0);
+static SENSOR_DEVICE_ATTR_2(pwm1_stall_dis, S_IRUGO | S_IWUSR, show_stall_dis,
+			    set_stall_dis, 0, 0);
 static SENSOR_DEVICE_ATTR_2(pwm2, S_IRUGO | S_IWUSR, show_pwm, set_pwm, INPUT,
 			    1);
 static SENSOR_DEVICE_ATTR_2(pwm2_freq, S_IRUGO | S_IWUSR, show_pwmfreq,
@@ -1040,6 +1083,8 @@ static SENSOR_DEVICE_ATTR_2(pwm2_auto_point1_pwm, S_IRUGO | S_IWUSR, show_pwm,
 			    set_pwm, MIN, 1);
 static SENSOR_DEVICE_ATTR_2(pwm2_auto_point2_pwm, S_IRUGO | S_IWUSR, show_pwm,
 			    set_pwm, MAX, 1);
+static SENSOR_DEVICE_ATTR_2(pwm2_stall_dis, S_IRUGO | S_IWUSR, show_stall_dis,
+			    set_stall_dis, 0, 1);
 static SENSOR_DEVICE_ATTR_2(pwm3, S_IRUGO | S_IWUSR, show_pwm, set_pwm, INPUT,
 			    2);
 static SENSOR_DEVICE_ATTR_2(pwm3_freq, S_IRUGO | S_IWUSR, show_pwmfreq,
@@ -1052,6 +1097,8 @@ static SENSOR_DEVICE_ATTR_2(pwm3_auto_point1_pwm, S_IRUGO | S_IWUSR, show_pwm,
 			    set_pwm, MIN, 2);
 static SENSOR_DEVICE_ATTR_2(pwm3_auto_point2_pwm, S_IRUGO | S_IWUSR, show_pwm,
 			    set_pwm, MAX, 2);
+static SENSOR_DEVICE_ATTR_2(pwm3_stall_dis, S_IRUGO | S_IWUSR, show_stall_dis,
+			    set_stall_dis, 0, 2);
 
 /* Non-standard name, might need revisiting */
 static DEVICE_ATTR_RW(pwm_use_point2_pwm_at_crit);
@@ -1112,12 +1159,14 @@ static struct attribute *adt7475_attrs[] = {
 	&sensor_dev_attr_pwm1_auto_channels_temp.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
 	&sensor_dev_attr_pwm1_auto_point2_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm1_stall_dis.dev_attr.attr,
 	&sensor_dev_attr_pwm3.dev_attr.attr,
 	&sensor_dev_attr_pwm3_freq.dev_attr.attr,
 	&sensor_dev_attr_pwm3_enable.dev_attr.attr,
 	&sensor_dev_attr_pwm3_auto_channels_temp.dev_attr.attr,
 	&sensor_dev_attr_pwm3_auto_point1_pwm.dev_attr.attr,
 	&sensor_dev_attr_pwm3_auto_point2_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm3_stall_dis.dev_attr.attr,
 	&dev_attr_pwm_use_point2_pwm_at_crit.attr,
 	NULL,
 };
@@ -1136,6 +1185,7 @@ static struct attribute *pwm2_attrs[] = {
 	&sensor_dev_attr_pwm2_auto_channels_temp.dev_attr.attr,
 	&sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr,
 	&sensor_dev_attr_pwm2_auto_point2_pwm.dev_attr.attr,
+	&sensor_dev_attr_pwm2_stall_dis.dev_attr.attr,
 	NULL
 };
 
-- 
2.11.0.24.ge6920cf

[PATCH v3 3/4] hwmon: (adt7475) temperature smoothing

[Chris Packham]

When enabled temperature smoothing allows ramping the fan speed over a
configurable period of time instead of jumping to the new speed
instantaneously.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Changes in v2:
- use a single tempN_smoothing attribute
Changes in v3:
- change enh_acou to enh_acoustics
- simplify show_temp_st()

 Documentation/hwmon/adt7475 |  4 ++
 drivers/hwmon/adt7475.c     | 93 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+)

diff --git a/Documentation/hwmon/adt7475 b/Documentation/hwmon/adt7475
index 3990bae60e78..e82b24ec4b07 100644
--- a/Documentation/hwmon/adt7475
+++ b/Documentation/hwmon/adt7475
@@ -114,6 +114,10 @@ minimum (i.e. auto_point1_pwm). This behaviour can be configured using the
 pwm[1-*]_stall_dis sysfs attribute. A value of 0 means the fans will shut off.
 A value of 1 means the fans will run at auto_point1_pwm.
 
+The responsiveness of the ADT747x to temperature changes can be configured.
+This allows smoothing of the fan speed transition. To set the transition time
+set the value in ms in the temp[1-*]_smoothing sysfs attribute.
+
 Notes
 -----
 
diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index 4d6c625fec70..f7322330789c 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -526,6 +526,90 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
+/* Assuming CONFIG6[SLOW] is 0 */
+static const int ad7475_st_map[] = {
+	37500, 18800, 12500, 7500, 4700, 3100, 1600, 800,
+};
+
+static ssize_t show_temp_st(struct device *dev, struct device_attribute *attr,
+				  char *buf)
+{
+	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+	struct i2c_client *client = to_i2c_client(dev);
+	struct adt7475_data *data = i2c_get_clientdata(client);
+	long val;
+
+	switch (sattr->index) {
+	case 0:
+		val = data->enh_acoustics[0] & 0xf;
+		break;
+	case 1:
+		val = (data->enh_acoustics[1] >> 4) & 0xf;
+		break;
+	case 2:
+		val = data->enh_acoustics[1] & 0xf;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (val & 0x8)
+		return sprintf(buf, "%d\n", ad7475_st_map[val & 0x7]);
+	else
+		return sprintf(buf, "0\n");
+}
+
+static ssize_t set_temp_st(struct device *dev, struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+	struct i2c_client *client = to_i2c_client(dev);
+	struct adt7475_data *data = i2c_get_clientdata(client);
+	unsigned char reg;
+	int shift, idx;
+	ulong val;
+
+	if (kstrtoul(buf, 10, &val))
+		return -EINVAL;
+
+	switch (sattr->index) {
+	case 0:
+		reg = REG_ENHANCE_ACOUSTICS1;
+		shift = 0;
+		idx = 0;
+		break;
+	case 1:
+		reg = REG_ENHANCE_ACOUSTICS2;
+		shift = 4;
+		idx = 1;
+		break;
+	case 2:
+		reg = REG_ENHANCE_ACOUSTICS2;
+		shift = 0;
+		idx = 1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (val > 0) {
+		val = find_closest_descending(val, ad7475_st_map,
+					      ARRAY_SIZE(ad7475_st_map));
+		val |= 0x8;
+	}
+
+	mutex_lock(&data->lock);
+
+	data->enh_acoustics[idx] &= ~(0xf << shift);
+	data->enh_acoustics[idx] |= (val << shift);
+
+	i2c_smbus_write_byte_data(client, reg, data->enh_acoustics[idx]);
+
+	mutex_unlock(&data->lock);
+
+	return count;
+}
+
 /*
  * Table of autorange values - the user will write the value in millidegrees,
  * and we'll convert it
@@ -1008,6 +1092,8 @@ static SENSOR_DEVICE_ATTR_2(temp1_crit, S_IRUGO | S_IWUSR, show_temp, set_temp,
 			    THERM, 0);
 static SENSOR_DEVICE_ATTR_2(temp1_crit_hyst, S_IRUGO | S_IWUSR, show_temp,
 			    set_temp, HYSTERSIS, 0);
+static SENSOR_DEVICE_ATTR_2(temp1_smoothing, S_IRUGO | S_IWUSR, show_temp_st,
+			    set_temp_st, 0, 0);
 static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp, NULL, INPUT, 1);
 static SENSOR_DEVICE_ATTR_2(temp2_alarm, S_IRUGO, show_temp, NULL, ALARM, 1);
 static SENSOR_DEVICE_ATTR_2(temp2_max, S_IRUGO | S_IWUSR, show_temp, set_temp,
@@ -1024,6 +1110,8 @@ static SENSOR_DEVICE_ATTR_2(temp2_crit, S_IRUGO | S_IWUSR, show_temp, set_temp,
 			    THERM, 1);
 static SENSOR_DEVICE_ATTR_2(temp2_crit_hyst, S_IRUGO | S_IWUSR, show_temp,
 			    set_temp, HYSTERSIS, 1);
+static SENSOR_DEVICE_ATTR_2(temp2_smoothing, S_IRUGO | S_IWUSR, show_temp_st,
+			    set_temp_st, 0, 1);
 static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp, NULL, INPUT, 2);
 static SENSOR_DEVICE_ATTR_2(temp3_alarm, S_IRUGO, show_temp, NULL, ALARM, 2);
 static SENSOR_DEVICE_ATTR_2(temp3_fault, S_IRUGO, show_temp, NULL, FAULT, 2);
@@ -1041,6 +1129,8 @@ static SENSOR_DEVICE_ATTR_2(temp3_crit, S_IRUGO | S_IWUSR, show_temp, set_temp,
 			    THERM, 2);
 static SENSOR_DEVICE_ATTR_2(temp3_crit_hyst, S_IRUGO | S_IWUSR, show_temp,
 			    set_temp, HYSTERSIS, 2);
+static SENSOR_DEVICE_ATTR_2(temp3_smoothing, S_IRUGO | S_IWUSR, show_temp_st,
+			    set_temp_st, 0, 2);
 static SENSOR_DEVICE_ATTR_2(fan1_input, S_IRUGO, show_tach, NULL, INPUT, 0);
 static SENSOR_DEVICE_ATTR_2(fan1_min, S_IRUGO | S_IWUSR, show_tach, set_tach,
 			    MIN, 0);
@@ -1125,6 +1215,7 @@ static struct attribute *adt7475_attrs[] = {
 	&sensor_dev_attr_temp1_auto_point2_temp.dev_attr.attr,
 	&sensor_dev_attr_temp1_crit.dev_attr.attr,
 	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
+	&sensor_dev_attr_temp1_smoothing.dev_attr.attr,
 	&sensor_dev_attr_temp2_input.dev_attr.attr,
 	&sensor_dev_attr_temp2_alarm.dev_attr.attr,
 	&sensor_dev_attr_temp2_max.dev_attr.attr,
@@ -1134,6 +1225,7 @@ static struct attribute *adt7475_attrs[] = {
 	&sensor_dev_attr_temp2_auto_point2_temp.dev_attr.attr,
 	&sensor_dev_attr_temp2_crit.dev_attr.attr,
 	&sensor_dev_attr_temp2_crit_hyst.dev_attr.attr,
+	&sensor_dev_attr_temp2_smoothing.dev_attr.attr,
 	&sensor_dev_attr_temp3_input.dev_attr.attr,
 	&sensor_dev_attr_temp3_fault.dev_attr.attr,
 	&sensor_dev_attr_temp3_alarm.dev_attr.attr,
@@ -1144,6 +1236,7 @@ static struct attribute *adt7475_attrs[] = {
 	&sensor_dev_attr_temp3_auto_point2_temp.dev_attr.attr,
 	&sensor_dev_attr_temp3_crit.dev_attr.attr,
 	&sensor_dev_attr_temp3_crit_hyst.dev_attr.attr,
+	&sensor_dev_attr_temp3_smoothing.dev_attr.attr,
 	&sensor_dev_attr_fan1_input.dev_attr.attr,
 	&sensor_dev_attr_fan1_min.dev_attr.attr,
 	&sensor_dev_attr_fan1_alarm.dev_attr.attr,
-- 
2.11.0.24.ge6920cf

[PATCH v3 4/4] hwmon: (adt7475) add high frequency support

[Chris Packham]

Systems using 4-wire fans usually require high frequency (22.5kHz)
output on the pwm. Add 22500 as a valid option in the pwmfreq_table. In
high frequency mode the low-order bit are ignored so they can safely be
set to 0.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
Changes in v3:
- New

 drivers/hwmon/adt7475.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index f7322330789c..f5a65d1166cd 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -936,7 +936,7 @@ static ssize_t set_pwmctrl(struct device *dev, struct device_attribute *attr,
 
 /* List of frequencies for the PWM */
 static const int pwmfreq_table[] = {
-	11, 14, 22, 29, 35, 44, 58, 88
+	11, 14, 22, 29, 35, 44, 58, 88, 22500
 };
 
 static ssize_t show_pwmfreq(struct device *dev, struct device_attribute *attr,
@@ -944,9 +944,10 @@ static ssize_t show_pwmfreq(struct device *dev, struct device_attribute *attr,
 {
 	struct adt7475_data *data = adt7475_update_device(dev);
 	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+	int i = clamp_val(data->range[sattr->index] & 0xf, 0,
+			  sizeof(pwmfreq_table));
 
-	return sprintf(buf, "%d\n",
-		       pwmfreq_table[data->range[sattr->index] & 7]);
+	return sprintf(buf, "%d\n", pwmfreq_table[i]);
 }
 
 static ssize_t set_pwmfreq(struct device *dev, struct device_attribute *attr,
@@ -967,7 +968,7 @@ static ssize_t set_pwmfreq(struct device *dev, struct device_attribute *attr,
 
 	data->range[sattr->index] =
 		adt7475_read(TEMP_TRANGE_REG(sattr->index));
-	data->range[sattr->index] &= ~7;
+	data->range[sattr->index] &= ~0xf;
 	data->range[sattr->index] |= out;
 
 	i2c_smbus_write_byte_data(client, TEMP_TRANGE_REG(sattr->index),
-- 
2.11.0.24.ge6920cf

Re: [PATCH v3 4/4] hwmon: (adt7475) add high frequency support

[Chris Packham]

On 11/05/17 15:45, Chris Packham wrote:
> Systems using 4-wire fans usually require high frequency (22.5kHz)
> output on the pwm. Add 22500 as a valid option in the pwmfreq_table. In
> high frequency mode the low-order bit are ignored so they can safely be
> set to 0.
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> Changes in v3:
> - New
>
>  drivers/hwmon/adt7475.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
> index f7322330789c..f5a65d1166cd 100644
> --- a/drivers/hwmon/adt7475.c
> +++ b/drivers/hwmon/adt7475.c
> @@ -936,7 +936,7 @@ static ssize_t set_pwmctrl(struct device *dev, struct device_attribute *attr,
>
>  /* List of frequencies for the PWM */
>  static const int pwmfreq_table[] = {
> -	11, 14, 22, 29, 35, 44, 58, 88
> +	11, 14, 22, 29, 35, 44, 58, 88, 22500
>  };
>
>  static ssize_t show_pwmfreq(struct device *dev, struct device_attribute *attr,
> @@ -944,9 +944,10 @@ static ssize_t show_pwmfreq(struct device *dev, struct device_attribute *attr,
>  {
>  	struct adt7475_data *data = adt7475_update_device(dev);
>  	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
> +	int i = clamp_val(data->range[sattr->index] & 0xf, 0,
> +			  sizeof(pwmfreq_table));

Oops that should probably be

+			  ARRAY_SIZE(pwmfreq_table) - 1);

Guenter, do you want me to send a v4 or can you fix it up at your end?

>
> -	return sprintf(buf, "%d\n",
> -		       pwmfreq_table[data->range[sattr->index] & 7]);
> +	return sprintf(buf, "%d\n", pwmfreq_table[i]);
>  }
>
>  static ssize_t set_pwmfreq(struct device *dev, struct device_attribute *attr,
> @@ -967,7 +968,7 @@ static ssize_t set_pwmfreq(struct device *dev, struct device_attribute *attr,
>
>  	data->range[sattr->index] =
>  		adt7475_read(TEMP_TRANGE_REG(sattr->index));
> -	data->range[sattr->index] &= ~7;
> +	data->range[sattr->index] &= ~0xf;
>  	data->range[sattr->index] |= out;
>
>  	i2c_smbus_write_byte_data(client, TEMP_TRANGE_REG(sattr->index),
>

Re: [PATCH v3 1/4] hwmon: (adt7475) replace find_nearest() with find_closest()

[Guenter Roeck]

On 05/10/2017 08:45 PM, Chris Packham wrote:
> The adt7475 has had find_nearest() since it's creation in 2009. Since
> then find_closest() has been introduced and several drivers have been
> updated to use it. Update the adt7475 to use find_closest() and remove
> the now unused find_nearest().
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

Applied to hwmon-next.

Thanks,
Guenter

> ---
> Changes in v3:
> - None
>
>  drivers/hwmon/adt7475.c | 34 +++-------------------------------
>  1 file changed, 3 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
> index c803e3c5fcd4..ec0c43fbcdce 100644
> --- a/drivers/hwmon/adt7475.c
> +++ b/drivers/hwmon/adt7475.c
> @@ -22,6 +22,7 @@
>  #include <linux/hwmon-vid.h>
>  #include <linux/err.h>
>  #include <linux/jiffies.h>
> +#include <linux/util_macros.h>
>
>  /* Indexes for the sysfs hooks */
>
> @@ -314,35 +315,6 @@ static void adt7475_write_word(struct i2c_client *client, int reg, u16 val)
>  	i2c_smbus_write_byte_data(client, reg, val & 0xFF);
>  }
>
> -/*
> - * Find the nearest value in a table - used for pwm frequency and
> - * auto temp range
> - */
> -static int find_nearest(long val, const int *array, int size)
> -{
> -	int i;
> -
> -	if (val < array[0])
> -		return 0;
> -
> -	if (val > array[size - 1])
> -		return size - 1;
> -
> -	for (i = 0; i < size - 1; i++) {
> -		int a, b;
> -
> -		if (val > array[i + 1])
> -			continue;
> -
> -		a = val - array[i];
> -		b = array[i + 1] - val;
> -
> -		return (a <= b) ? i : i + 1;
> -	}
> -
> -	return 0;
> -}
> -
>  static ssize_t show_voltage(struct device *dev, struct device_attribute *attr,
>  			    char *buf)
>  {
> @@ -606,7 +578,7 @@ static ssize_t set_point2(struct device *dev, struct device_attribute *attr,
>  	val -= temp;
>
>  	/* Find the nearest table entry to what the user wrote */
> -	val = find_nearest(val, autorange_table, ARRAY_SIZE(autorange_table));
> +	val = find_closest(val, autorange_table, ARRAY_SIZE(autorange_table));
>
>  	data->range[sattr->index] &= ~0xF0;
>  	data->range[sattr->index] |= val << 4;
> @@ -864,7 +836,7 @@ static ssize_t set_pwmfreq(struct device *dev, struct device_attribute *attr,
>  	if (kstrtol(buf, 10, &val))
>  		return -EINVAL;
>
> -	out = find_nearest(val, pwmfreq_table, ARRAY_SIZE(pwmfreq_table));
> +	out = find_closest(val, pwmfreq_table, ARRAY_SIZE(pwmfreq_table));
>
>  	mutex_lock(&data->lock);
>
>

Re: [PATCH v3 2/4] hwmon: (adt7475) fan stall prevention

[Guenter Roeck]

On 05/10/2017 08:45 PM, Chris Packham wrote:
> By default adt7475 will stop the fans (pwm duty cycle 0%) when the
> temperature drops past Tmin - hysteresis. Some systems want to keep the
> fans moving even when the temperature drops so add new sysfs attributes
> that configure the enhanced acoustics min 1-3 which allows the fans to
> run at the minimum configure pwm duty cycle.
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>
> Changes in v2:
> - use pwmN_stall_dis as the attribute name. I think this describes the purpose
>   pretty well. I went with a new attribute instead of overloading

Almost agree. Can we use pwmN_stall_disable ?

Thanks,
Guenter


>   pwmN_auto_point1_pwm so this doesn't affect existing users.
> Changes in v3:
> - Fix grammar.
> - change enh_acou to enh_acoustics
>
>  Documentation/hwmon/adt7475 |  5 +++++
>  drivers/hwmon/adt7475.c     | 50 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 55 insertions(+)
>
> diff --git a/Documentation/hwmon/adt7475 b/Documentation/hwmon/adt7475
> index 0502f2b464e1..3990bae60e78 100644
> --- a/Documentation/hwmon/adt7475
> +++ b/Documentation/hwmon/adt7475
> @@ -109,6 +109,11 @@ fan speed) is applied. PWM values range from 0 (off) to 255 (full speed).
>  Fan speed may be set to maximum when the temperature sensor associated with
>  the PWM control exceeds temp#_max.
>
> +At Tmin - hysteresis the PWM output can either be off (0% duty cycle) or at the
> +minimum (i.e. auto_point1_pwm). This behaviour can be configured using the
> +pwm[1-*]_stall_dis sysfs attribute. A value of 0 means the fans will shut off.
> +A value of 1 means the fans will run at auto_point1_pwm.
> +
>  Notes
>  -----
>
> diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
> index ec0c43fbcdce..4d6c625fec70 100644
> --- a/drivers/hwmon/adt7475.c
> +++ b/drivers/hwmon/adt7475.c
> @@ -79,6 +79,9 @@
>
>  #define REG_TEMP_TRANGE_BASE	0x5F
>
> +#define REG_ENHANCE_ACOUSTICS1	0x62
> +#define REG_ENHANCE_ACOUSTICS2	0x63
> +
>  #define REG_PWM_MIN_BASE	0x64
>
>  #define REG_TEMP_TMIN_BASE	0x67
> @@ -209,6 +212,7 @@ struct adt7475_data {
>  	u8 range[3];
>  	u8 pwmctl[3];
>  	u8 pwmchan[3];
> +	u8 enh_acoustics[2];
>
>  	u8 vid;
>  	u8 vrm;
> @@ -700,6 +704,43 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
>  	data->pwm[sattr->nr][sattr->index] = clamp_val(val, 0, 0xFF);
>  	i2c_smbus_write_byte_data(client, reg,
>  				  data->pwm[sattr->nr][sattr->index]);
> +	mutex_unlock(&data->lock);
> +
> +	return count;
> +}
> +
> +
> +static ssize_t show_stall_dis(struct device *dev, struct device_attribute *attr,
> +			      char *buf)
> +{
> +	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct adt7475_data *data = i2c_get_clientdata(client);
> +	u8 mask = BIT(5 + sattr->index);
> +
> +	return sprintf(buf, "%d\n", !!(data->enh_acoustics[0] & mask));
> +}
> +
> +static ssize_t set_stall_dis(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct adt7475_data *data = i2c_get_clientdata(client);
> +	long val;
> +	u8 mask = BIT(5 + sattr->index);
> +
> +	if (kstrtol(buf, 10, &val))
> +		return -EINVAL;
> +
> +	mutex_lock(&data->lock);
> +
> +	data->enh_acoustics[0] &= ~mask;
> +	if (val)
> +		data->enh_acoustics[0] |= mask;
> +
> +	i2c_smbus_write_byte_data(client, REG_ENHANCE_ACOUSTICS1,
> +				  data->enh_acoustics[0]);
>
>  	mutex_unlock(&data->lock);
>
> @@ -1028,6 +1069,8 @@ static SENSOR_DEVICE_ATTR_2(pwm1_auto_point1_pwm, S_IRUGO | S_IWUSR, show_pwm,
>  			    set_pwm, MIN, 0);
>  static SENSOR_DEVICE_ATTR_2(pwm1_auto_point2_pwm, S_IRUGO | S_IWUSR, show_pwm,
>  			    set_pwm, MAX, 0);
> +static SENSOR_DEVICE_ATTR_2(pwm1_stall_dis, S_IRUGO | S_IWUSR, show_stall_dis,
> +			    set_stall_dis, 0, 0);
>  static SENSOR_DEVICE_ATTR_2(pwm2, S_IRUGO | S_IWUSR, show_pwm, set_pwm, INPUT,
>  			    1);
>  static SENSOR_DEVICE_ATTR_2(pwm2_freq, S_IRUGO | S_IWUSR, show_pwmfreq,
> @@ -1040,6 +1083,8 @@ static SENSOR_DEVICE_ATTR_2(pwm2_auto_point1_pwm, S_IRUGO | S_IWUSR, show_pwm,
>  			    set_pwm, MIN, 1);
>  static SENSOR_DEVICE_ATTR_2(pwm2_auto_point2_pwm, S_IRUGO | S_IWUSR, show_pwm,
>  			    set_pwm, MAX, 1);
> +static SENSOR_DEVICE_ATTR_2(pwm2_stall_dis, S_IRUGO | S_IWUSR, show_stall_dis,
> +			    set_stall_dis, 0, 1);
>  static SENSOR_DEVICE_ATTR_2(pwm3, S_IRUGO | S_IWUSR, show_pwm, set_pwm, INPUT,
>  			    2);
>  static SENSOR_DEVICE_ATTR_2(pwm3_freq, S_IRUGO | S_IWUSR, show_pwmfreq,
> @@ -1052,6 +1097,8 @@ static SENSOR_DEVICE_ATTR_2(pwm3_auto_point1_pwm, S_IRUGO | S_IWUSR, show_pwm,
>  			    set_pwm, MIN, 2);
>  static SENSOR_DEVICE_ATTR_2(pwm3_auto_point2_pwm, S_IRUGO | S_IWUSR, show_pwm,
>  			    set_pwm, MAX, 2);
> +static SENSOR_DEVICE_ATTR_2(pwm3_stall_dis, S_IRUGO | S_IWUSR, show_stall_dis,
> +			    set_stall_dis, 0, 2);
>
>  /* Non-standard name, might need revisiting */
>  static DEVICE_ATTR_RW(pwm_use_point2_pwm_at_crit);
> @@ -1112,12 +1159,14 @@ static struct attribute *adt7475_attrs[] = {
>  	&sensor_dev_attr_pwm1_auto_channels_temp.dev_attr.attr,
>  	&sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr,
>  	&sensor_dev_attr_pwm1_auto_point2_pwm.dev_attr.attr,
> +	&sensor_dev_attr_pwm1_stall_dis.dev_attr.attr,
>  	&sensor_dev_attr_pwm3.dev_attr.attr,
>  	&sensor_dev_attr_pwm3_freq.dev_attr.attr,
>  	&sensor_dev_attr_pwm3_enable.dev_attr.attr,
>  	&sensor_dev_attr_pwm3_auto_channels_temp.dev_attr.attr,
>  	&sensor_dev_attr_pwm3_auto_point1_pwm.dev_attr.attr,
>  	&sensor_dev_attr_pwm3_auto_point2_pwm.dev_attr.attr,
> +	&sensor_dev_attr_pwm3_stall_dis.dev_attr.attr,
>  	&dev_attr_pwm_use_point2_pwm_at_crit.attr,
>  	NULL,
>  };
> @@ -1136,6 +1185,7 @@ static struct attribute *pwm2_attrs[] = {
>  	&sensor_dev_attr_pwm2_auto_channels_temp.dev_attr.attr,
>  	&sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr,
>  	&sensor_dev_attr_pwm2_auto_point2_pwm.dev_attr.attr,
> +	&sensor_dev_attr_pwm2_stall_dis.dev_attr.attr,
>  	NULL
>  };
>
>

Re: [PATCH v3 3/4] hwmon: (adt7475) temperature smoothing

[Guenter Roeck]

On 05/10/2017 08:45 PM, Chris Packham wrote:
> When enabled temperature smoothing allows ramping the fan speed over a
> configurable period of time instead of jumping to the new speed
> instantaneously.
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>
> Changes in v2:
> - use a single tempN_smoothing attribute

This is a bit confusing. tempN suggests that the attribute would be associated
with a given temperature, not with fan control. Not that I have a better idea
for an attribute name, though, so unless you find a better name I am ok with it.

> Changes in v3:
> - change enh_acou to enh_acoustics
> - simplify show_temp_st()
>
>  Documentation/hwmon/adt7475 |  4 ++
>  drivers/hwmon/adt7475.c     | 93 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 97 insertions(+)
>
> diff --git a/Documentation/hwmon/adt7475 b/Documentation/hwmon/adt7475
> index 3990bae60e78..e82b24ec4b07 100644
> --- a/Documentation/hwmon/adt7475
> +++ b/Documentation/hwmon/adt7475
> @@ -114,6 +114,10 @@ minimum (i.e. auto_point1_pwm). This behaviour can be configured using the
>  pwm[1-*]_stall_dis sysfs attribute. A value of 0 means the fans will shut off.
>  A value of 1 means the fans will run at auto_point1_pwm.
>
> +The responsiveness of the ADT747x to temperature changes can be configured.
> +This allows smoothing of the fan speed transition. To set the transition time
> +set the value in ms in the temp[1-*]_smoothing sysfs attribute.
> +
>  Notes
>  -----
>
> diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
> index 4d6c625fec70..f7322330789c 100644
> --- a/drivers/hwmon/adt7475.c
> +++ b/drivers/hwmon/adt7475.c
> @@ -526,6 +526,90 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *attr,
>  	return count;
>  }
>
> +/* Assuming CONFIG6[SLOW] is 0 */
> +static const int ad7475_st_map[] = {
> +	37500, 18800, 12500, 7500, 4700, 3100, 1600, 800,
> +};
> +
> +static ssize_t show_temp_st(struct device *dev, struct device_attribute *attr,
> +				  char *buf)
> +{
> +	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct adt7475_data *data = i2c_get_clientdata(client);
> +	long val;
> +
> +	switch (sattr->index) {
> +	case 0:
> +		val = data->enh_acoustics[0] & 0xf;
> +		break;
> +	case 1:
> +		val = (data->enh_acoustics[1] >> 4) & 0xf;
> +		break;
> +	case 2:
> +		val = data->enh_acoustics[1] & 0xf;
> +		break;
> +	default:
> +		return -EINVAL;

This will never happen and, if it does, would indicate a bug, not invalid input.
I kind of dislike dead code; it just bloats the kernel. Please either use
default: for or together with case 2:, or make it if/else.

> +	}
> +
> +	if (val & 0x8)
> +		return sprintf(buf, "%d\n", ad7475_st_map[val & 0x7]);
> +	else
> +		return sprintf(buf, "0\n");
> +}
> +
> +static ssize_t set_temp_st(struct device *dev, struct device_attribute *attr,
> +				 const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct adt7475_data *data = i2c_get_clientdata(client);
> +	unsigned char reg;
> +	int shift, idx;
> +	ulong val;
> +
> +	if (kstrtoul(buf, 10, &val))
> +		return -EINVAL;
> +
> +	switch (sattr->index) {
> +	case 0:
> +		reg = REG_ENHANCE_ACOUSTICS1;
> +		shift = 0;
> +		idx = 0;
> +		break;
> +	case 1:
> +		reg = REG_ENHANCE_ACOUSTICS2;
> +		shift = 4;
> +		idx = 1;
> +		break;
> +	case 2:
> +		reg = REG_ENHANCE_ACOUSTICS2;
> +		shift = 0;
> +		idx = 1;
> +		break;

Is this correct ? It associates
	temp1_smoothing -> remote 1
	temp2_smoothing -> remote 2
	temp3_smoothing -> local

which, unless I am missing something, doesn't match temp1/2/3.

> +	default:
> +		return -EINVAL;

Same as above.

> +	}
> +
> +	if (val > 0) {
> +		val = find_closest_descending(val, ad7475_st_map,
> +					      ARRAY_SIZE(ad7475_st_map));
> +		val |= 0x8;
> +	}
> +
> +	mutex_lock(&data->lock);
> +
> +	data->enh_acoustics[idx] &= ~(0xf << shift);
> +	data->enh_acoustics[idx] |= (val << shift);
> +
> +	i2c_smbus_write_byte_data(client, reg, data->enh_acoustics[idx]);
> +
> +	mutex_unlock(&data->lock);
> +
> +	return count;
> +}
> +
>  /*
>   * Table of autorange values - the user will write the value in millidegrees,
>   * and we'll convert it
> @@ -1008,6 +1092,8 @@ static SENSOR_DEVICE_ATTR_2(temp1_crit, S_IRUGO | S_IWUSR, show_temp, set_temp,
>  			    THERM, 0);
>  static SENSOR_DEVICE_ATTR_2(temp1_crit_hyst, S_IRUGO | S_IWUSR, show_temp,
>  			    set_temp, HYSTERSIS, 0);
> +static SENSOR_DEVICE_ATTR_2(temp1_smoothing, S_IRUGO | S_IWUSR, show_temp_st,
> +			    set_temp_st, 0, 0);
>  static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp, NULL, INPUT, 1);
>  static SENSOR_DEVICE_ATTR_2(temp2_alarm, S_IRUGO, show_temp, NULL, ALARM, 1);
>  static SENSOR_DEVICE_ATTR_2(temp2_max, S_IRUGO | S_IWUSR, show_temp, set_temp,
> @@ -1024,6 +1110,8 @@ static SENSOR_DEVICE_ATTR_2(temp2_crit, S_IRUGO | S_IWUSR, show_temp, set_temp,
>  			    THERM, 1);
>  static SENSOR_DEVICE_ATTR_2(temp2_crit_hyst, S_IRUGO | S_IWUSR, show_temp,
>  			    set_temp, HYSTERSIS, 1);
> +static SENSOR_DEVICE_ATTR_2(temp2_smoothing, S_IRUGO | S_IWUSR, show_temp_st,
> +			    set_temp_st, 0, 1);
>  static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp, NULL, INPUT, 2);
>  static SENSOR_DEVICE_ATTR_2(temp3_alarm, S_IRUGO, show_temp, NULL, ALARM, 2);
>  static SENSOR_DEVICE_ATTR_2(temp3_fault, S_IRUGO, show_temp, NULL, FAULT, 2);
> @@ -1041,6 +1129,8 @@ static SENSOR_DEVICE_ATTR_2(temp3_crit, S_IRUGO | S_IWUSR, show_temp, set_temp,
>  			    THERM, 2);
>  static SENSOR_DEVICE_ATTR_2(temp3_crit_hyst, S_IRUGO | S_IWUSR, show_temp,
>  			    set_temp, HYSTERSIS, 2);
> +static SENSOR_DEVICE_ATTR_2(temp3_smoothing, S_IRUGO | S_IWUSR, show_temp_st,
> +			    set_temp_st, 0, 2);
>  static SENSOR_DEVICE_ATTR_2(fan1_input, S_IRUGO, show_tach, NULL, INPUT, 0);
>  static SENSOR_DEVICE_ATTR_2(fan1_min, S_IRUGO | S_IWUSR, show_tach, set_tach,
>  			    MIN, 0);
> @@ -1125,6 +1215,7 @@ static struct attribute *adt7475_attrs[] = {
>  	&sensor_dev_attr_temp1_auto_point2_temp.dev_attr.attr,
>  	&sensor_dev_attr_temp1_crit.dev_attr.attr,
>  	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
> +	&sensor_dev_attr_temp1_smoothing.dev_attr.attr,
>  	&sensor_dev_attr_temp2_input.dev_attr.attr,
>  	&sensor_dev_attr_temp2_alarm.dev_attr.attr,
>  	&sensor_dev_attr_temp2_max.dev_attr.attr,
> @@ -1134,6 +1225,7 @@ static struct attribute *adt7475_attrs[] = {
>  	&sensor_dev_attr_temp2_auto_point2_temp.dev_attr.attr,
>  	&sensor_dev_attr_temp2_crit.dev_attr.attr,
>  	&sensor_dev_attr_temp2_crit_hyst.dev_attr.attr,
> +	&sensor_dev_attr_temp2_smoothing.dev_attr.attr,
>  	&sensor_dev_attr_temp3_input.dev_attr.attr,
>  	&sensor_dev_attr_temp3_fault.dev_attr.attr,
>  	&sensor_dev_attr_temp3_alarm.dev_attr.attr,
> @@ -1144,6 +1236,7 @@ static struct attribute *adt7475_attrs[] = {
>  	&sensor_dev_attr_temp3_auto_point2_temp.dev_attr.attr,
>  	&sensor_dev_attr_temp3_crit.dev_attr.attr,
>  	&sensor_dev_attr_temp3_crit_hyst.dev_attr.attr,
> +	&sensor_dev_attr_temp3_smoothing.dev_attr.attr,
>  	&sensor_dev_attr_fan1_input.dev_attr.attr,
>  	&sensor_dev_attr_fan1_min.dev_attr.attr,
>  	&sensor_dev_attr_fan1_alarm.dev_attr.attr,
>

Re: [PATCH v3 4/4] hwmon: (adt7475) add high frequency support

[Guenter Roeck]

On 05/11/2017 02:31 PM, Chris Packham wrote:
> On 11/05/17 15:45, Chris Packham wrote:
>> Systems using 4-wire fans usually require high frequency (22.5kHz)
>> output on the pwm. Add 22500 as a valid option in the pwmfreq_table. In
>> high frequency mode the low-order bit are ignored so they can safely be
>> set to 0.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>> Changes in v3:
>> - New
>>
>>  drivers/hwmon/adt7475.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
>> index f7322330789c..f5a65d1166cd 100644
>> --- a/drivers/hwmon/adt7475.c
>> +++ b/drivers/hwmon/adt7475.c
>> @@ -936,7 +936,7 @@ static ssize_t set_pwmctrl(struct device *dev, struct device_attribute *attr,
>>
>>  /* List of frequencies for the PWM */
>>  static const int pwmfreq_table[] = {
>> -	11, 14, 22, 29, 35, 44, 58, 88
>> +	11, 14, 22, 29, 35, 44, 58, 88, 22500
>>  };
>>
>>  static ssize_t show_pwmfreq(struct device *dev, struct device_attribute *attr,
>> @@ -944,9 +944,10 @@ static ssize_t show_pwmfreq(struct device *dev, struct device_attribute *attr,
>>  {
>>  	struct adt7475_data *data = adt7475_update_device(dev);
>>  	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
>> +	int i = clamp_val(data->range[sattr->index] & 0xf, 0,
>> +			  sizeof(pwmfreq_table));
>
> Oops that should probably be
>
> +			  ARRAY_SIZE(pwmfreq_table) - 1);
>
> Guenter, do you want me to send a v4 or can you fix it up at your end?
>
Please resend together with patches 2 and 3.

Thanks,
Guenter

>>
>> -	return sprintf(buf, "%d\n",
>> -		       pwmfreq_table[data->range[sattr->index] & 7]);
>> +	return sprintf(buf, "%d\n", pwmfreq_table[i]);
>>  }
>>
>>  static ssize_t set_pwmfreq(struct device *dev, struct device_attribute *attr,
>> @@ -967,7 +968,7 @@ static ssize_t set_pwmfreq(struct device *dev, struct device_attribute *attr,
>>
>>  	data->range[sattr->index] =
>>  		adt7475_read(TEMP_TRANGE_REG(sattr->index));
>> -	data->range[sattr->index] &= ~7;
>> +	data->range[sattr->index] &= ~0xf;
>>  	data->range[sattr->index] |= out;
>>
>>  	i2c_smbus_write_byte_data(client, TEMP_TRANGE_REG(sattr->index),
>>
>
>

Re: [PATCH v3 2/4] hwmon: (adt7475) fan stall prevention

[Chris Packham]

On 15/05/17 02:54, Guenter Roeck wrote:
> On 05/10/2017 08:45 PM, Chris Packham wrote:
>> By default adt7475 will stop the fans (pwm duty cycle 0%) when the
>> temperature drops past Tmin - hysteresis. Some systems want to keep the
>> fans moving even when the temperature drops so add new sysfs attributes
>> that configure the enhanced acoustics min 1-3 which allows the fans to
>> run at the minimum configure pwm duty cycle.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>
>> Changes in v2:
>> - use pwmN_stall_dis as the attribute name. I think this describes the purpose
>>   pretty well. I went with a new attribute instead of overloading
>
> Almost agree. Can we use pwmN_stall_disable ?
>

Sold. I'll send a v4 of this patch with the name changed.

> Thanks,
> Guenter
>
>
>>   pwmN_auto_point1_pwm so this doesn't affect existing users.
>> Changes in v3:
>> - Fix grammar.
>> - change enh_acou to enh_acoustics

Re: [PATCH v3 3/4] hwmon: (adt7475) temperature smoothing

[Chris Packham]

On 15/05/17 03:40, Guenter Roeck wrote:
> On 05/10/2017 08:45 PM, Chris Packham wrote:
>> When enabled temperature smoothing allows ramping the fan speed over a
>> configurable period of time instead of jumping to the new speed
>> instantaneously.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>
>> Changes in v2:
>> - use a single tempN_smoothing attribute
>
> This is a bit confusing. tempN suggests that the attribute would be associated
> with a given temperature, not with fan control. Not that I have a better idea
> for an attribute name, though, so unless you find a better name I am ok with it.
>

The datasheet is a bit confusing in this respect.

 From the description of register 0x62:

"Assuming that PWMx is associated with the Remote 1 temperature channel, 
these bits define the maximum rate of change of the PWMx output for 
Remote 1 temperature related changes. Instead of the fan speed jumping 
instantaneously to its newly determined speed, it ramps
gracefully at the rate determined by these bits. This feature ultimately 
enhances the acoustics of the fan."

Based on my reading it's a property of the temperature input not of the 
PWM. If you changed pwmN_auto_channels_temp this setting would stay with 
the temperature sensor not the PWM.

>> Changes in v3:
>> - change enh_acou to enh_acoustics
>> - simplify show_temp_st()
>>
>>  Documentation/hwmon/adt7475 |  4 ++
>>  drivers/hwmon/adt7475.c     | 93 +++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 97 insertions(+)
>>
>> diff --git a/Documentation/hwmon/adt7475 b/Documentation/hwmon/adt7475
>> index 3990bae60e78..e82b24ec4b07 100644
>> --- a/Documentation/hwmon/adt7475
>> +++ b/Documentation/hwmon/adt7475
>> @@ -114,6 +114,10 @@ minimum (i.e. auto_point1_pwm). This behaviour can be configured using the
>>  pwm[1-*]_stall_dis sysfs attribute. A value of 0 means the fans will shut off.
>>  A value of 1 means the fans will run at auto_point1_pwm.
>>
>> +The responsiveness of the ADT747x to temperature changes can be configured.
>> +This allows smoothing of the fan speed transition. To set the transition time
>> +set the value in ms in the temp[1-*]_smoothing sysfs attribute.
>> +
>>  Notes
>>  -----
>>
>> diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
>> index 4d6c625fec70..f7322330789c 100644
>> --- a/drivers/hwmon/adt7475.c
>> +++ b/drivers/hwmon/adt7475.c
>> @@ -526,6 +526,90 @@ static ssize_t set_temp(struct device *dev, struct device_attribute *attr,
>>  	return count;
>>  }
>>
>> +/* Assuming CONFIG6[SLOW] is 0 */
>> +static const int ad7475_st_map[] = {
>> +	37500, 18800, 12500, 7500, 4700, 3100, 1600, 800,
>> +};
>> +
>> +static ssize_t show_temp_st(struct device *dev, struct device_attribute *attr,
>> +				  char *buf)
>> +{
>> +	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	struct adt7475_data *data = i2c_get_clientdata(client);
>> +	long val;
>> +
>> +	switch (sattr->index) {
>> +	case 0:
>> +		val = data->enh_acoustics[0] & 0xf;
>> +		break;
>> +	case 1:
>> +		val = (data->enh_acoustics[1] >> 4) & 0xf;
>> +		break;
>> +	case 2:
>> +		val = data->enh_acoustics[1] & 0xf;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>
> This will never happen and, if it does, would indicate a bug, not invalid input.
> I kind of dislike dead code; it just bloats the kernel. Please either use
> default: for or together with case 2:, or make it if/else.
>

Will combine default and case 2.

>> +	}
>> +
>> +	if (val & 0x8)
>> +		return sprintf(buf, "%d\n", ad7475_st_map[val & 0x7]);
>> +	else
>> +		return sprintf(buf, "0\n");
>> +}
>> +
>> +static ssize_t set_temp_st(struct device *dev, struct device_attribute *attr,
>> +				 const char *buf, size_t count)
>> +{
>> +	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	struct adt7475_data *data = i2c_get_clientdata(client);
>> +	unsigned char reg;
>> +	int shift, idx;
>> +	ulong val;
>> +
>> +	if (kstrtoul(buf, 10, &val))
>> +		return -EINVAL;
>> +
>> +	switch (sattr->index) {
>> +	case 0:
>> +		reg = REG_ENHANCE_ACOUSTICS1;
>> +		shift = 0;
>> +		idx = 0;
>> +		break;
>> +	case 1:
>> +		reg = REG_ENHANCE_ACOUSTICS2;
>> +		shift = 4;
>> +		idx = 1;
>> +		break;
>> +	case 2:
>> +		reg = REG_ENHANCE_ACOUSTICS2;
>> +		shift = 0;
>> +		idx = 1;
>> +		break;
>
> Is this correct ? It associates
> 	temp1_smoothing -> remote 1
> 	temp2_smoothing -> remote 2
> 	temp3_smoothing -> local

Yes

> which, unless I am missing something, doesn't match temp1/2/3.
>

So it should be

temp1 -> Remote 1
temp2 -> Local
temp3 -> Remote 2

I'll rework accordingly.

>> +	default:
>> +		return -EINVAL;
>
> Same as above.
>

Done.

>> +	}
>> +
>> +	if (val > 0) {
>> +		val = find_closest_descending(val, ad7475_st_map,
>> +					      ARRAY_SIZE(ad7475_st_map));
>> +		val |= 0x8;
>> +	}
>> +
>> +	mutex_lock(&data->lock);
>> +
>> +	data->enh_acoustics[idx] &= ~(0xf << shift);
>> +	data->enh_acoustics[idx] |= (val << shift);
>> +
>> +	i2c_smbus_write_byte_data(client, reg, data->enh_acoustics[idx]);
>> +
>> +	mutex_unlock(&data->lock);
>> +
>> +	return count;
>> +}
>> +
>>  /*
>>   * Table of autorange values - the user will write the value in millidegrees,
>>   * and we'll convert it
>> @@ -1008,6 +1092,8 @@ static SENSOR_DEVICE_ATTR_2(temp1_crit, S_IRUGO | S_IWUSR, show_temp, set_temp,
>>  			    THERM, 0);
>>  static SENSOR_DEVICE_ATTR_2(temp1_crit_hyst, S_IRUGO | S_IWUSR, show_temp,
>>  			    set_temp, HYSTERSIS, 0);
>> +static SENSOR_DEVICE_ATTR_2(temp1_smoothing, S_IRUGO | S_IWUSR, show_temp_st,
>> +			    set_temp_st, 0, 0);
>>  static SENSOR_DEVICE_ATTR_2(temp2_input, S_IRUGO, show_temp, NULL, INPUT, 1);
>>  static SENSOR_DEVICE_ATTR_2(temp2_alarm, S_IRUGO, show_temp, NULL, ALARM, 1);
>>  static SENSOR_DEVICE_ATTR_2(temp2_max, S_IRUGO | S_IWUSR, show_temp, set_temp,
>> @@ -1024,6 +1110,8 @@ static SENSOR_DEVICE_ATTR_2(temp2_crit, S_IRUGO | S_IWUSR, show_temp, set_temp,
>>  			    THERM, 1);
>>  static SENSOR_DEVICE_ATTR_2(temp2_crit_hyst, S_IRUGO | S_IWUSR, show_temp,
>>  			    set_temp, HYSTERSIS, 1);
>> +static SENSOR_DEVICE_ATTR_2(temp2_smoothing, S_IRUGO | S_IWUSR, show_temp_st,
>> +			    set_temp_st, 0, 1);
>>  static SENSOR_DEVICE_ATTR_2(temp3_input, S_IRUGO, show_temp, NULL, INPUT, 2);
>>  static SENSOR_DEVICE_ATTR_2(temp3_alarm, S_IRUGO, show_temp, NULL, ALARM, 2);
>>  static SENSOR_DEVICE_ATTR_2(temp3_fault, S_IRUGO, show_temp, NULL, FAULT, 2);
>> @@ -1041,6 +1129,8 @@ static SENSOR_DEVICE_ATTR_2(temp3_crit, S_IRUGO | S_IWUSR, show_temp, set_temp,
>>  			    THERM, 2);
>>  static SENSOR_DEVICE_ATTR_2(temp3_crit_hyst, S_IRUGO | S_IWUSR, show_temp,
>>  			    set_temp, HYSTERSIS, 2);
>> +static SENSOR_DEVICE_ATTR_2(temp3_smoothing, S_IRUGO | S_IWUSR, show_temp_st,
>> +			    set_temp_st, 0, 2);
>>  static SENSOR_DEVICE_ATTR_2(fan1_input, S_IRUGO, show_tach, NULL, INPUT, 0);
>>  static SENSOR_DEVICE_ATTR_2(fan1_min, S_IRUGO | S_IWUSR, show_tach, set_tach,
>>  			    MIN, 0);
>> @@ -1125,6 +1215,7 @@ static struct attribute *adt7475_attrs[] = {
>>  	&sensor_dev_attr_temp1_auto_point2_temp.dev_attr.attr,
>>  	&sensor_dev_attr_temp1_crit.dev_attr.attr,
>>  	&sensor_dev_attr_temp1_crit_hyst.dev_attr.attr,
>> +	&sensor_dev_attr_temp1_smoothing.dev_attr.attr,
>>  	&sensor_dev_attr_temp2_input.dev_attr.attr,
>>  	&sensor_dev_attr_temp2_alarm.dev_attr.attr,
>>  	&sensor_dev_attr_temp2_max.dev_attr.attr,
>> @@ -1134,6 +1225,7 @@ static struct attribute *adt7475_attrs[] = {
>>  	&sensor_dev_attr_temp2_auto_point2_temp.dev_attr.attr,
>>  	&sensor_dev_attr_temp2_crit.dev_attr.attr,
>>  	&sensor_dev_attr_temp2_crit_hyst.dev_attr.attr,
>> +	&sensor_dev_attr_temp2_smoothing.dev_attr.attr,
>>  	&sensor_dev_attr_temp3_input.dev_attr.attr,
>>  	&sensor_dev_attr_temp3_fault.dev_attr.attr,
>>  	&sensor_dev_attr_temp3_alarm.dev_attr.attr,
>> @@ -1144,6 +1236,7 @@ static struct attribute *adt7475_attrs[] = {
>>  	&sensor_dev_attr_temp3_auto_point2_temp.dev_attr.attr,
>>  	&sensor_dev_attr_temp3_crit.dev_attr.attr,
>>  	&sensor_dev_attr_temp3_crit_hyst.dev_attr.attr,
>> +	&sensor_dev_attr_temp3_smoothing.dev_attr.attr,
>>  	&sensor_dev_attr_fan1_input.dev_attr.attr,
>>  	&sensor_dev_attr_fan1_min.dev_attr.attr,
>>  	&sensor_dev_attr_fan1_alarm.dev_attr.attr,
>>
>
>

Re: [PATCH v3 3/4] hwmon: (adt7475) temperature smoothing

[Guenter Roeck]

On 05/14/2017 02:23 PM, Chris Packham wrote:
> On 15/05/17 03:40, Guenter Roeck wrote:
>> On 05/10/2017 08:45 PM, Chris Packham wrote:
>>> When enabled temperature smoothing allows ramping the fan speed over a
>>> configurable period of time instead of jumping to the new speed
>>> instantaneously.
>>>
>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>> ---
>>>
>>> Changes in v2:
>>> - use a single tempN_smoothing attribute
>>
>> This is a bit confusing. tempN suggests that the attribute would be associated
>> with a given temperature, not with fan control. Not that I have a better idea
>> for an attribute name, though, so unless you find a better name I am ok with it.
>>
>
> The datasheet is a bit confusing in this respect.
>
>  From the description of register 0x62:
>
> "Assuming that PWMx is associated with the Remote 1 temperature channel,
> these bits define the maximum rate of change of the PWMx output for
> Remote 1 temperature related changes. Instead of the fan speed jumping
> instantaneously to its newly determined speed, it ramps
> gracefully at the rate determined by these bits. This feature ultimately
> enhances the acoustics of the fan."
>
> Based on my reading it's a property of the temperature input not of the
> PWM. If you changed pwmN_auto_channels_temp this setting would stay with
> the temperature sensor not the PWM.
>

Agreed, that is why I said that I don't have a better idea ...

Guenter