[PATCH] hwmon: (max6650) Allow fan shutdown and a more intuitive mode interface

[PATCH] hwmon: (max6650) Allow fan shutdown and a more intuitive mode interface

[Mike Looijmans]

The fan can be stopped by writing "0" to fan1_target in sysfs.

Writing non-zero values to fan1_target or pwm1 in sysfs automatically
selects the corresponding control mode (closed or open loop).

This allows userspace applications to control the fan speed without
the need to know specific details of the controller (like the fact
that fan1_target does not take effect when pwm1_enable is set to
anything but "2"). Early initialization of the fan controller prevents
overheating, for example when resetting the board while the fan was
completely turned off.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
This patch requires the devicetree support patch for the max6650.

Changes the functionality and interface of the driver, to be able to
initialize the chip at boot, and allows userspace control without
requiring hardware knowledge.

 .../devicetree/bindings/hwmon/max6650.txt          |   5 +
 Documentation/hwmon/max6650                        |   5 +-
 drivers/hwmon/max6650.c                            | 153 +++++++++++++--------
 3 files changed, 106 insertions(+), 57 deletions(-)

diff --git a/Documentation/devicetree/bindings/hwmon/max6650.txt b/Documentation/devicetree/bindings/hwmon/max6650.txt
index 2e46e69..724ab3a 100644
--- a/Documentation/devicetree/bindings/hwmon/max6650.txt
+++ b/Documentation/devicetree/bindings/hwmon/max6650.txt
@@ -13,6 +13,10 @@ Optional properties, default is to retain the chip's current setting:
 - maxim,fan-prescale  : Pre-scaling value, as per datasheet [1]. Lower values
 			allow more fine-grained control of slower fans.
 			Valid: 1, 2, 4, 8, 16.
+- maxim,fan-target-rpm: Initial requested fan rotation speed. If specified, the
+			driver selects closed-loop mode and the requested speed.
+			This ensures the fan is already running before userspace
+			takes over.
 
 Example:
 	fan-max6650: max6650@1b {
@@ -20,4 +24,5 @@ Example:
 		compatible = "maxim,max6650";
 		maxim,fan-microvolt = <12000000>;
 		maxim,fan-prescale = <4>;
+		maxim,fan-target-rpm = <1200>;
 	};
diff --git a/Documentation/hwmon/max6650 b/Documentation/hwmon/max6650
index 58d9644..53e308b 100644
--- a/Documentation/hwmon/max6650
+++ b/Documentation/hwmon/max6650
@@ -33,9 +33,12 @@ fan2_input	ro	"
 fan3_input	ro	"
 fan4_input	ro	"
 fan1_target	rw	desired fan speed in RPM (closed loop mode only)
+			Set to 0 to stop the fan. Writing any other value sets
+			the regulator mode to "closed loop".
 pwm1_enable	rw	regulator mode, 0=full on, 1=open loop, 2=closed loop
 pwm1		rw	relative speed (0-255), 255=max. speed.
-			Used in open loop mode only.
+			Set to 0 to stop the fan. Writing any other value sets
+			the regulator mode to "open loop".
 fan1_div	rw	sets the speed range the inputs can handle. Legal
 			values are 1, 2, 4, and 8. Use lower values for
 			faster fans.
diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index 6beb62c..d40756a 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -185,6 +185,35 @@ static struct max6650_data *max6650_update_device(struct device *dev)
 	return data;
 }
 
+static bool max6650_is_powerdown(const struct max6650_data *data)
+{
+	return (data->config & MAX6650_CFG_MODE_MASK) == MAX6650_CFG_MODE_OFF;
+}
+
+/*
+ * Change the operating mode of the chip (if needed).
+ * mode is one of the MAX6650_CFG_MODE_* values.
+ */
+static int max6650_set_operating_mode(struct max6650_data *data, u8 mode)
+{
+	int result;
+	u8 config = data->config;
+
+	if (mode == (config & MAX6650_CFG_MODE_MASK))
+		return 0;
+
+	config = (config & ~MAX6650_CFG_MODE_MASK) | mode;
+
+	result = i2c_smbus_write_byte_data(data->client, MAX6650_REG_CONFIG,
+					   config);
+	if (result < 0)
+		return result;
+
+	data->config = config;
+
+	return 0;
+}
+
 static ssize_t get_fan(struct device *dev, struct device_attribute *devattr,
 		       char *buf)
 {
@@ -258,26 +287,26 @@ static ssize_t get_target(struct device *dev, struct device_attribute *devattr,
 	 *    FanSpeed = KSCALE x fCLK / [256 x (KTACH + 1)]
 	 *
 	 * then multiply by 60 to give rpm.
+	 *
+	 * When not running, report target to be "0".
 	 */
 
-	kscale = DIV_FROM_REG(data->config);
-	ktach = data->speed;
-	rpm = 60 * kscale * clock / (256 * (ktach + 1));
+	if (max6650_is_powerdown(data))
+		rpm = 0;
+	else {
+		kscale = DIV_FROM_REG(data->config);
+		ktach = data->speed;
+		rpm = 60 * kscale * clock / (256 * (ktach + 1));
+	}
 	return sprintf(buf, "%d\n", rpm);
 }
 
-static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
-			 const char *buf, size_t count)
+static int max6650_set_target(struct max6650_data *data, unsigned long rpm)
 {
-	struct max6650_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	int kscale, ktach;
-	unsigned long rpm;
-	int err;
 
-	err = kstrtoul(buf, 10, &rpm);
-	if (err)
-		return err;
+	if (rpm == 0)
+		return max6650_set_operating_mode(data, MAX6650_CFG_MODE_OFF);
 
 	rpm = clamp_val(rpm, FAN_RPM_MIN, FAN_RPM_MAX);
 
@@ -288,8 +317,6 @@ static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
 	 *     KTACH = [(fCLK x KSCALE) / (256 x FanSpeed)] - 1
 	 */
 
-	mutex_lock(&data->update_lock);
-
 	kscale = DIV_FROM_REG(data->config);
 	ktach = ((clock * kscale) / (256 * rpm / 60)) - 1;
 	if (ktach < 0)
@@ -298,7 +325,25 @@ static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
 		ktach = 255;
 	data->speed = ktach;
 
-	i2c_smbus_write_byte_data(client, MAX6650_REG_SPEED, data->speed);
+	i2c_smbus_write_byte_data(data->client, MAX6650_REG_SPEED, data->speed);
+
+	return max6650_set_operating_mode(data, MAX6650_CFG_MODE_CLOSED_LOOP);
+}
+
+static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
+			 const char *buf, size_t count)
+{
+	struct max6650_data *data = dev_get_drvdata(dev);
+	unsigned long rpm;
+	int err;
+
+	err = kstrtoul(buf, 10, &rpm);
+	if (err)
+		return err;
+
+	mutex_lock(&data->update_lock);
+
+	err = max6650_set_target(data, rpm);
 
 	mutex_unlock(&data->update_lock);
 
@@ -320,17 +365,21 @@ static ssize_t get_pwm(struct device *dev, struct device_attribute *devattr,
 	int pwm;
 	struct max6650_data *data = max6650_update_device(dev);
 
-	/*
-	 * Useful range for dac is 0-180 for 12V fans and 0-76 for 5V fans.
-	 * Lower DAC values mean higher speeds.
-	 */
-	if (data->config & MAX6650_CFG_V12)
-		pwm = 255 - (255 * (int)data->dac)/180;
-	else
-		pwm = 255 - (255 * (int)data->dac)/76;
-
-	if (pwm < 0)
+	if (max6650_is_powerdown(data))
 		pwm = 0;
+	else {
+		/*
+		 * Useful range for dac is 0-180 for 12V fans and 0-76 for 5V
+		 * fans. Lower DAC values mean higher speeds.
+		 */
+		if (data->config & MAX6650_CFG_V12)
+			pwm = 255 - (255 * (int)data->dac)/180;
+		else
+			pwm = 255 - (255 * (int)data->dac)/76;
+
+		if (pwm < 0)
+			pwm = 0;
+	}
 
 	return sprintf(buf, "%d\n", pwm);
 }
@@ -351,16 +400,20 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *devattr,
 
 	mutex_lock(&data->update_lock);
 
-	if (data->config & MAX6650_CFG_V12)
-		data->dac = 180 - (180 * pwm)/255;
-	else
-		data->dac = 76 - (76 * pwm)/255;
-
-	i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, data->dac);
+	if (pwm == 0)
+		err = max6650_set_operating_mode(data, MAX6650_CFG_MODE_OFF);
+	else {
+		if (data->config & MAX6650_CFG_V12)
+			data->dac = 180 - (180 * pwm)/255;
+		else
+			data->dac = 76 - (76 * pwm)/255;
+		i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, data->dac);
+		err = max6650_set_operating_mode(data, MAX6650_CFG_MODE_OPEN_LOOP);
+	}
 
 	mutex_unlock(&data->update_lock);
 
-	return count;
+	return err < 0 ? err : count;
 }
 
 /*
@@ -385,25 +438,24 @@ static ssize_t set_enable(struct device *dev, struct device_attribute *devattr,
 			  const char *buf, size_t count)
 {
 	struct max6650_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	int max6650_modes[3] = {0, 3, 2};
 	unsigned long mode;
 	int err;
+	const u8 max6650_modes[3] = {
+		MAX6650_CFG_MODE_ON,
+		MAX6650_CFG_MODE_OPEN_LOOP,
+		MAX6650_CFG_MODE_CLOSED_LOOP
+		};
 
 	err = kstrtoul(buf, 10, &mode);
 	if (err)
 		return err;
 
-	if (mode > 2)
+	if (mode >= ARRAY_SIZE(max6650_modes))
 		return -EINVAL;
 
 	mutex_lock(&data->update_lock);
 
-	data->config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
-	data->config = (data->config & ~MAX6650_CFG_MODE_MASK)
-		       | (max6650_modes[mode] << 4);
-
-	i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, data->config);
+	max6650_set_operating_mode(data, max6650_modes[mode]);
 
 	mutex_unlock(&data->update_lock);
 
@@ -582,6 +634,7 @@ static int max6650_init_client(struct max6650_data *data,
 	int err = -EIO;
 	u32 voltage;
 	u32 prescale;
+	u32 target_rpm;
 
 	if (of_property_read_u32(dev->of_node, "maxim,fan-microvolt",
 				 &voltage))
@@ -642,22 +695,6 @@ static int max6650_init_client(struct max6650_data *data,
 		 (config & MAX6650_CFG_V12) ? 12 : 5,
 		 1 << (config & MAX6650_CFG_PRESCALER_MASK));
 
-	/*
-	 * If mode is set to "full off", we change it to "open loop" and
-	 * set DAC to 255, which has the same effect. We do this because
-	 * there's no "full off" mode defined in hwmon specifications.
-	 */
-
-	if ((config & MAX6650_CFG_MODE_MASK) == MAX6650_CFG_MODE_OFF) {
-		dev_dbg(dev, "Change mode to open loop, full off.\n");
-		config = (config & ~MAX6650_CFG_MODE_MASK)
-			 | MAX6650_CFG_MODE_OPEN_LOOP;
-		if (i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, 255)) {
-			dev_err(dev, "DAC write error, aborting.\n");
-			return err;
-		}
-	}
-
 	if (i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, config)) {
 		dev_err(dev, "Config write error, aborting.\n");
 		return err;
@@ -666,6 +703,10 @@ static int max6650_init_client(struct max6650_data *data,
 	data->config = config;
 	data->count = i2c_smbus_read_byte_data(client, MAX6650_REG_COUNT);
 
+	if (!of_property_read_u32(client->dev.of_node, "maxim,fan-target-rpm",
+				 &target_rpm))
+		max6650_set_target(data, target_rpm);
+
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH] hwmon: (max6650) Allow fan shutdown and a more intuitive mode interface

[Guenter Roeck]

On 08/16/2016 12:53 AM, Mike Looijmans wrote:
> The fan can be stopped by writing "0" to fan1_target in sysfs.
>
> Writing non-zero values to fan1_target or pwm1 in sysfs automatically
> selects the corresponding control mode (closed or open loop).
>
> This allows userspace applications to control the fan speed without
> the need to know specific details of the controller (like the fact
> that fan1_target does not take effect when pwm1_enable is set to
> anything but "2"). Early initialization of the fan controller prevents
> overheating, for example when resetting the board while the fan was
> completely turned off.
>

But this is the normal hwmon ABI. It should not be a surprise.
On the other side, changing the mode automatically as side effect
_is_ a surprise.

> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
> This patch requires the devicetree support patch for the max6650.
>
> Changes the functionality and interface of the driver, to be able to
> initialize the chip at boot, and allows userspace control without
> requiring hardware knowledge.
>
>  .../devicetree/bindings/hwmon/max6650.txt          |   5 +
>  Documentation/hwmon/max6650                        |   5 +-
>  drivers/hwmon/max6650.c                            | 153 +++++++++++++--------
>  3 files changed, 106 insertions(+), 57 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/hwmon/max6650.txt b/Documentation/devicetree/bindings/hwmon/max6650.txt
> index 2e46e69..724ab3a 100644
> --- a/Documentation/devicetree/bindings/hwmon/max6650.txt
> +++ b/Documentation/devicetree/bindings/hwmon/max6650.txt
> @@ -13,6 +13,10 @@ Optional properties, default is to retain the chip's current setting:
>  - maxim,fan-prescale  : Pre-scaling value, as per datasheet [1]. Lower values
>  			allow more fine-grained control of slower fans.
>  			Valid: 1, 2, 4, 8, 16.
> +- maxim,fan-target-rpm: Initial requested fan rotation speed. If specified, the
> +			driver selects closed-loop mode and the requested speed.
> +			This ensures the fan is already running before userspace
> +			takes over.
>

Needs to be separate patch and be approved by devicetree maintainers. Overall,
it would be better to make have the devicetree changces as single patch.

>  Example:
>  	fan-max6650: max6650@1b {
> @@ -20,4 +24,5 @@ Example:
>  		compatible = "maxim,max6650";
>  		maxim,fan-microvolt = <12000000>;
>  		maxim,fan-prescale = <4>;
> +		maxim,fan-target-rpm = <1200>;
>  	};
> diff --git a/Documentation/hwmon/max6650 b/Documentation/hwmon/max6650
> index 58d9644..53e308b 100644
> --- a/Documentation/hwmon/max6650
> +++ b/Documentation/hwmon/max6650
> @@ -33,9 +33,12 @@ fan2_input	ro	"
>  fan3_input	ro	"
>  fan4_input	ro	"
>  fan1_target	rw	desired fan speed in RPM (closed loop mode only)
> +			Set to 0 to stop the fan. Writing any other value sets
> +			the regulator mode to "closed loop".
>  pwm1_enable	rw	regulator mode, 0=full on, 1=open loop, 2=closed loop
>  pwm1		rw	relative speed (0-255), 255=max. speed.
> -			Used in open loop mode only.
> +			Set to 0 to stop the fan. Writing any other value sets
> +			the regulator mode to "open loop".

The mode should really be selected using pwm1_enable. I don't like the idea
of automatically changing it as side effect.

>  fan1_div	rw	sets the speed range the inputs can handle. Legal
>  			values are 1, 2, 4, and 8. Use lower values for
>  			faster fans.
> diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
> index 6beb62c..d40756a 100644
> --- a/drivers/hwmon/max6650.c
> +++ b/drivers/hwmon/max6650.c
> @@ -185,6 +185,35 @@ static struct max6650_data *max6650_update_device(struct device *dev)
>  	return data;
>  }
>
> +static bool max6650_is_powerdown(const struct max6650_data *data)
> +{
> +	return (data->config & MAX6650_CFG_MODE_MASK) == MAX6650_CFG_MODE_OFF;
> +}
> +
> +/*
> + * Change the operating mode of the chip (if needed).
> + * mode is one of the MAX6650_CFG_MODE_* values.
> + */
> +static int max6650_set_operating_mode(struct max6650_data *data, u8 mode)
> +{
> +	int result;
> +	u8 config = data->config;
> +
> +	if (mode == (config & MAX6650_CFG_MODE_MASK))
> +		return 0;
> +
> +	config = (config & ~MAX6650_CFG_MODE_MASK) | mode;
> +
> +	result = i2c_smbus_write_byte_data(data->client, MAX6650_REG_CONFIG,
> +					   config);
> +	if (result < 0)
> +		return result;
> +
> +	data->config = config;
> +
> +	return 0;
> +}
> +
>  static ssize_t get_fan(struct device *dev, struct device_attribute *devattr,
>  		       char *buf)
>  {
> @@ -258,26 +287,26 @@ static ssize_t get_target(struct device *dev, struct device_attribute *devattr,
>  	 *    FanSpeed = KSCALE x fCLK / [256 x (KTACH + 1)]
>  	 *
>  	 * then multiply by 60 to give rpm.
> +	 *
> +	 * When not running, report target to be "0".
>  	 */
>
> -	kscale = DIV_FROM_REG(data->config);
> -	ktach = data->speed;
> -	rpm = 60 * kscale * clock / (256 * (ktach + 1));
> +	if (max6650_is_powerdown(data))
> +		rpm = 0;
> +	else {
> +		kscale = DIV_FROM_REG(data->config);
> +		ktach = data->speed;
> +		rpm = 60 * kscale * clock / (256 * (ktach + 1));
> +	}
>  	return sprintf(buf, "%d\n", rpm);
>  }
>
> -static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
> -			 const char *buf, size_t count)
> +static int max6650_set_target(struct max6650_data *data, unsigned long rpm)
>  {
> -	struct max6650_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
>  	int kscale, ktach;
> -	unsigned long rpm;
> -	int err;
>
> -	err = kstrtoul(buf, 10, &rpm);
> -	if (err)
> -		return err;
> +	if (rpm == 0)
> +		return max6650_set_operating_mode(data, MAX6650_CFG_MODE_OFF);
>
>  	rpm = clamp_val(rpm, FAN_RPM_MIN, FAN_RPM_MAX);
>
> @@ -288,8 +317,6 @@ static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
>  	 *     KTACH = [(fCLK x KSCALE) / (256 x FanSpeed)] - 1
>  	 */
>
> -	mutex_lock(&data->update_lock);
> -
>  	kscale = DIV_FROM_REG(data->config);
>  	ktach = ((clock * kscale) / (256 * rpm / 60)) - 1;
>  	if (ktach < 0)
> @@ -298,7 +325,25 @@ static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
>  		ktach = 255;
>  	data->speed = ktach;
>
> -	i2c_smbus_write_byte_data(client, MAX6650_REG_SPEED, data->speed);
> +	i2c_smbus_write_byte_data(data->client, MAX6650_REG_SPEED, data->speed);
> +
> +	return max6650_set_operating_mode(data, MAX6650_CFG_MODE_CLOSED_LOOP);
> +}
> +
> +static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
> +			 const char *buf, size_t count)
> +{
> +	struct max6650_data *data = dev_get_drvdata(dev);
> +	unsigned long rpm;
> +	int err;
> +
> +	err = kstrtoul(buf, 10, &rpm);
> +	if (err)
> +		return err;
> +
> +	mutex_lock(&data->update_lock);
> +
> +	err = max6650_set_target(data, rpm);
>
>  	mutex_unlock(&data->update_lock);
>
> @@ -320,17 +365,21 @@ static ssize_t get_pwm(struct device *dev, struct device_attribute *devattr,
>  	int pwm;
>  	struct max6650_data *data = max6650_update_device(dev);
>
> -	/*
> -	 * Useful range for dac is 0-180 for 12V fans and 0-76 for 5V fans.
> -	 * Lower DAC values mean higher speeds.
> -	 */
> -	if (data->config & MAX6650_CFG_V12)
> -		pwm = 255 - (255 * (int)data->dac)/180;
> -	else
> -		pwm = 255 - (255 * (int)data->dac)/76;
> -
> -	if (pwm < 0)
> +	if (max6650_is_powerdown(data))
>  		pwm = 0;
> +	else {
> +		/*
> +		 * Useful range for dac is 0-180 for 12V fans and 0-76 for 5V
> +		 * fans. Lower DAC values mean higher speeds.
> +		 */
> +		if (data->config & MAX6650_CFG_V12)
> +			pwm = 255 - (255 * (int)data->dac)/180;
> +		else
> +			pwm = 255 - (255 * (int)data->dac)/76;
> +
> +		if (pwm < 0)
> +			pwm = 0;
> +	}
>
>  	return sprintf(buf, "%d\n", pwm);
>  }
> @@ -351,16 +400,20 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *devattr,
>
>  	mutex_lock(&data->update_lock);
>
> -	if (data->config & MAX6650_CFG_V12)
> -		data->dac = 180 - (180 * pwm)/255;
> -	else
> -		data->dac = 76 - (76 * pwm)/255;
> -
> -	i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, data->dac);
> +	if (pwm == 0)
> +		err = max6650_set_operating_mode(data, MAX6650_CFG_MODE_OFF);
> +	else {
> +		if (data->config & MAX6650_CFG_V12)
> +			data->dac = 180 - (180 * pwm)/255;
> +		else
> +			data->dac = 76 - (76 * pwm)/255;
> +		i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, data->dac);
> +		err = max6650_set_operating_mode(data, MAX6650_CFG_MODE_OPEN_LOOP);
> +	}
>
>  	mutex_unlock(&data->update_lock);
>
> -	return count;
> +	return err < 0 ? err : count;
>  }
>
>  /*
> @@ -385,25 +438,24 @@ static ssize_t set_enable(struct device *dev, struct device_attribute *devattr,
>  			  const char *buf, size_t count)
>  {
>  	struct max6650_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
> -	int max6650_modes[3] = {0, 3, 2};
>  	unsigned long mode;
>  	int err;
> +	const u8 max6650_modes[3] = {
> +		MAX6650_CFG_MODE_ON,
> +		MAX6650_CFG_MODE_OPEN_LOOP,
> +		MAX6650_CFG_MODE_CLOSED_LOOP
> +		};
>
>  	err = kstrtoul(buf, 10, &mode);
>  	if (err)
>  		return err;
>
> -	if (mode > 2)
> +	if (mode >= ARRAY_SIZE(max6650_modes))
>  		return -EINVAL;
>
>  	mutex_lock(&data->update_lock);
>
> -	data->config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
> -	data->config = (data->config & ~MAX6650_CFG_MODE_MASK)
> -		       | (max6650_modes[mode] << 4);
> -
> -	i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, data->config);
> +	max6650_set_operating_mode(data, max6650_modes[mode]);
>
>  	mutex_unlock(&data->update_lock);
>
> @@ -582,6 +634,7 @@ static int max6650_init_client(struct max6650_data *data,
>  	int err = -EIO;
>  	u32 voltage;
>  	u32 prescale;
> +	u32 target_rpm;
>
>  	if (of_property_read_u32(dev->of_node, "maxim,fan-microvolt",
>  				 &voltage))
> @@ -642,22 +695,6 @@ static int max6650_init_client(struct max6650_data *data,
>  		 (config & MAX6650_CFG_V12) ? 12 : 5,
>  		 1 << (config & MAX6650_CFG_PRESCALER_MASK));
>
> -	/*
> -	 * If mode is set to "full off", we change it to "open loop" and
> -	 * set DAC to 255, which has the same effect. We do this because
> -	 * there's no "full off" mode defined in hwmon specifications.
> -	 */
> -
> -	if ((config & MAX6650_CFG_MODE_MASK) == MAX6650_CFG_MODE_OFF) {
> -		dev_dbg(dev, "Change mode to open loop, full off.\n");
> -		config = (config & ~MAX6650_CFG_MODE_MASK)
> -			 | MAX6650_CFG_MODE_OPEN_LOOP;
> -		if (i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, 255)) {
> -			dev_err(dev, "DAC write error, aborting.\n");
> -			return err;
> -		}
> -	}
> -
>  	if (i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, config)) {
>  		dev_err(dev, "Config write error, aborting.\n");
>  		return err;
> @@ -666,6 +703,10 @@ static int max6650_init_client(struct max6650_data *data,
>  	data->config = config;
>  	data->count = i2c_smbus_read_byte_data(client, MAX6650_REG_COUNT);
>
> +	if (!of_property_read_u32(client->dev.of_node, "maxim,fan-target-rpm",
> +				 &target_rpm))
> +		max6650_set_target(data, target_rpm);
> +
>  	return 0;
>  }
>
>

Re: [PATCH] hwmon: (max6650) Allow fan shutdown and a more intuitive mode interface

[Guenter Roeck]

On 08/18/2016 11:35 PM, Mike Looijmans wrote:
> On 19-08-16 04:54, Guenter Roeck wrote:
>> On 08/16/2016 12:53 AM, Mike Looijmans wrote:
>>> The fan can be stopped by writing "0" to fan1_target in sysfs.
>>>
>>> Writing non-zero values to fan1_target or pwm1 in sysfs automatically
>>> selects the corresponding control mode (closed or open loop).
>>>
>>> This allows userspace applications to control the fan speed without
>>> the need to know specific details of the controller (like the fact
>>> that fan1_target does not take effect when pwm1_enable is set to
>>> anything but "2"). Early initialization of the fan controller prevents
>>> overheating, for example when resetting the board while the fan was
>>> completely turned off.
>>>
>>
>> But this is the normal hwmon ABI. It should not be a surprise.
>> On the other side, changing the mode automatically as side effect
>> _is_ a surprise.
>
> I was under the impression that the pwm*_enable=2 mode was intended for
> temperature controlled fans. Reading the docs for a bunch of drivers reveals
> that apart from "0=full on" and "1=manual pwm" there is no standard whatsoever
> for the meaning of this value, each driver assigns its own, forcing userspace
> to know all the details of a particular model. So indeed, I'll have to change
> that. I'll have to write software specific for this fan then, it cannot be
> generic with the current ABI.
>
> I was actually aiming for supporting "off" mode, so I could later (when the
> new hardware arrives) also control a related power supply from this driver.
>
> Guess I should just add a mode "3" to implement "off".
>

What is wrong with "pwm/target 0 equals off" ?

Not sure what you mean with "control a related power supply". Please explain.

>>
>>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>>> ---
>>> This patch requires the devicetree support patch for the max6650.
>>>
>>> Changes the functionality and interface of the driver, to be able to
>>> initialize the chip at boot, and allows userspace control without
>>> requiring hardware knowledge.
>>>
>>>  .../devicetree/bindings/hwmon/max6650.txt          |   5 +
>>>  Documentation/hwmon/max6650                        |   5 +-
>>>  drivers/hwmon/max6650.c                            | 153 +++++++++++++--------
>>>  3 files changed, 106 insertions(+), 57 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/hwmon/max6650.txt
>>> b/Documentation/devicetree/bindings/hwmon/max6650.txt
>>> index 2e46e69..724ab3a 100644
>>> --- a/Documentation/devicetree/bindings/hwmon/max6650.txt
>>> +++ b/Documentation/devicetree/bindings/hwmon/max6650.txt
>>> @@ -13,6 +13,10 @@ Optional properties, default is to retain the chip's
>>> current setting:
>>>  - maxim,fan-prescale  : Pre-scaling value, as per datasheet [1]. Lower values
>>>              allow more fine-grained control of slower fans.
>>>              Valid: 1, 2, 4, 8, 16.
>>> +- maxim,fan-target-rpm: Initial requested fan rotation speed. If specified,
>>> the
>>> +            driver selects closed-loop mode and the requested speed.
>>> +            This ensures the fan is already running before userspace
>>> +            takes over.
>>>
>>
>> Needs to be separate patch and be approved by devicetree maintainers. Overall,
>> it would be better to make have the devicetree changces as single patch.
>
> Okay.
>
> How do they get synchronized then, i.e. what if the documentation gets
> approved but the actual implementation does not?
>

You would normally submit a series of patches, so the overall status would be
easy to follow.

Problem with multiple small patches is that it adds load to the devicetree
maintainers, and it never gives them (nor us) a complete picture.

Guenter

[PATCH 1/2] hwmon: (max6650) Allow fan shutdown and initial rpm target

[Mike Looijmans]

The fan can be stopped by writing "3" to pwm1_enable in sysfs.

Add devicetree property for early initialization of the fan controller
to prevent overheating, for example when resetting the board while the
fan was completely turned off.

Also improve error reporting, I2C failures were ignored while writing
new values.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
 Documentation/hwmon/max6650 |   1 +
 drivers/hwmon/max6650.c     | 108 +++++++++++++++++++++++++++-----------------
 2 files changed, 68 insertions(+), 41 deletions(-)

diff --git a/Documentation/hwmon/max6650 b/Documentation/hwmon/max6650
index 58d9644..dff1d29 100644
--- a/Documentation/hwmon/max6650
+++ b/Documentation/hwmon/max6650
@@ -34,6 +34,7 @@ fan3_input	ro	"
 fan4_input	ro	"
 fan1_target	rw	desired fan speed in RPM (closed loop mode only)
 pwm1_enable	rw	regulator mode, 0=full on, 1=open loop, 2=closed loop
+			3=off
 pwm1		rw	relative speed (0-255), 255=max. speed.
 			Used in open loop mode only.
 fan1_div	rw	sets the speed range the inputs can handle. Legal
diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index c87517a..a993b44 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -185,6 +185,30 @@ static struct max6650_data *max6650_update_device(struct device *dev)
 	return data;
 }
 
+/*
+ * Change the operating mode of the chip (if needed).
+ * mode is one of the MAX6650_CFG_MODE_* values.
+ */
+static int max6650_set_operating_mode(struct max6650_data *data, u8 mode)
+{
+	int result;
+	u8 config = data->config;
+
+	if (mode == (config & MAX6650_CFG_MODE_MASK))
+		return 0;
+
+	config = (config & ~MAX6650_CFG_MODE_MASK) | mode;
+
+	result = i2c_smbus_write_byte_data(data->client, MAX6650_REG_CONFIG,
+					   config);
+	if (result < 0)
+		return result;
+
+	data->config = config;
+
+	return 0;
+}
+
 static ssize_t get_fan(struct device *dev, struct device_attribute *devattr,
 		       char *buf)
 {
@@ -266,18 +290,12 @@ static ssize_t get_target(struct device *dev, struct device_attribute *devattr,
 	return sprintf(buf, "%d\n", rpm);
 }
 
-static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
-			 const char *buf, size_t count)
+static int max6650_set_target(struct max6650_data *data, unsigned long rpm)
 {
-	struct max6650_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
 	int kscale, ktach;
-	unsigned long rpm;
-	int err;
 
-	err = kstrtoul(buf, 10, &rpm);
-	if (err)
-		return err;
+	if (rpm == 0)
+		return max6650_set_operating_mode(data, MAX6650_CFG_MODE_OFF);
 
 	rpm = clamp_val(rpm, FAN_RPM_MIN, FAN_RPM_MAX);
 
@@ -288,8 +306,6 @@ static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
 	 *     KTACH = [(fCLK x KSCALE) / (256 x FanSpeed)] - 1
 	 */
 
-	mutex_lock(&data->update_lock);
-
 	kscale = DIV_FROM_REG(data->config);
 	ktach = ((clock * kscale) / (256 * rpm / 60)) - 1;
 	if (ktach < 0)
@@ -298,10 +314,30 @@ static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
 		ktach = 255;
 	data->speed = ktach;
 
-	i2c_smbus_write_byte_data(client, MAX6650_REG_SPEED, data->speed);
+	return i2c_smbus_write_byte_data(data->client, MAX6650_REG_SPEED,
+					 data->speed);
+}
+
+static ssize_t set_target(struct device *dev, struct device_attribute *devattr,
+			 const char *buf, size_t count)
+{
+	struct max6650_data *data = dev_get_drvdata(dev);
+	unsigned long rpm;
+	int err;
+
+	err = kstrtoul(buf, 10, &rpm);
+	if (err)
+		return err;
+
+	mutex_lock(&data->update_lock);
+
+	err = max6650_set_target(data, rpm);
 
 	mutex_unlock(&data->update_lock);
 
+	if (err < 0)
+		return err;
+
 	return count;
 }
 
@@ -355,12 +391,11 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *devattr,
 		data->dac = 180 - (180 * pwm)/255;
 	else
 		data->dac = 76 - (76 * pwm)/255;
-
-	i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, data->dac);
+	err = i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, data->dac);
 
 	mutex_unlock(&data->update_lock);
 
-	return count;
+	return err < 0 ? err : count;
 }
 
 /*
@@ -369,14 +404,14 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *devattr,
  * 0 = Fan always on
  * 1 = Open loop, Voltage is set according to speed, not regulated.
  * 2 = Closed loop, RPM for all fans regulated by fan1 tachometer
+ * 3 = Fan off
  */
-
 static ssize_t get_enable(struct device *dev, struct device_attribute *devattr,
 			  char *buf)
 {
 	struct max6650_data *data = max6650_update_device(dev);
 	int mode = (data->config & MAX6650_CFG_MODE_MASK) >> 4;
-	int sysfs_modes[4] = {0, 1, 2, 1};
+	int sysfs_modes[4] = {0, 3, 2, 1};
 
 	return sprintf(buf, "%d\n", sysfs_modes[mode]);
 }
@@ -385,25 +420,25 @@ static ssize_t set_enable(struct device *dev, struct device_attribute *devattr,
 			  const char *buf, size_t count)
 {
 	struct max6650_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	int max6650_modes[3] = {0, 3, 2};
 	unsigned long mode;
 	int err;
+	const u8 max6650_modes[] = {
+		MAX6650_CFG_MODE_ON,
+		MAX6650_CFG_MODE_OPEN_LOOP,
+		MAX6650_CFG_MODE_CLOSED_LOOP,
+		MAX6650_CFG_MODE_OFF,
+		};
 
 	err = kstrtoul(buf, 10, &mode);
 	if (err)
 		return err;
 
-	if (mode > 2)
+	if (mode >= ARRAY_SIZE(max6650_modes))
 		return -EINVAL;
 
 	mutex_lock(&data->update_lock);
 
-	data->config = i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG);
-	data->config = (data->config & ~MAX6650_CFG_MODE_MASK)
-		       | (max6650_modes[mode] << 4);
-
-	i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, data->config);
+	max6650_set_operating_mode(data, max6650_modes[mode]);
 
 	mutex_unlock(&data->update_lock);
 
@@ -582,6 +617,7 @@ static int max6650_init_client(struct max6650_data *data,
 	int err = -EIO;
 	u32 voltage;
 	u32 prescale;
+	u32 target_rpm;
 
 	if (of_property_read_u32(dev->of_node, "maxim,fan-microvolt",
 				 &voltage))
@@ -642,22 +678,6 @@ static int max6650_init_client(struct max6650_data *data,
 		 (config & MAX6650_CFG_V12) ? 12 : 5,
 		 1 << (config & MAX6650_CFG_PRESCALER_MASK));
 
-	/*
-	 * If mode is set to "full off", we change it to "open loop" and
-	 * set DAC to 255, which has the same effect. We do this because
-	 * there's no "full off" mode defined in hwmon specifications.
-	 */
-
-	if ((config & MAX6650_CFG_MODE_MASK) == MAX6650_CFG_MODE_OFF) {
-		dev_dbg(dev, "Change mode to open loop, full off.\n");
-		config = (config & ~MAX6650_CFG_MODE_MASK)
-			 | MAX6650_CFG_MODE_OPEN_LOOP;
-		if (i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, 255)) {
-			dev_err(dev, "DAC write error, aborting.\n");
-			return err;
-		}
-	}
-
 	if (i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, config)) {
 		dev_err(dev, "Config write error, aborting.\n");
 		return err;
@@ -666,6 +686,12 @@ static int max6650_init_client(struct max6650_data *data,
 	data->config = config;
 	data->count = i2c_smbus_read_byte_data(client, MAX6650_REG_COUNT);
 
+	if (!of_property_read_u32(client->dev.of_node, "maxim,fan-target-rpm",
+				  &target_rpm)) {
+		max6650_set_target(data, target_rpm);
+		max6650_set_operating_mode(data, MAX6650_CFG_MODE_CLOSED_LOOP);
+	}
+
 	return 0;
 }
 
-- 
1.9.1

[PATCH 2/2] hwmon: (max6650) Add initial rpm target devicetree documentation

[Mike Looijmans]

Add devicetree property for early initialization of the fan controller
to prevent overheating, for example when resetting the board while the
fan was completely turned off.

Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
---
 Documentation/devicetree/bindings/hwmon/max6650.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/max6650.txt b/Documentation/devicetree/bindings/hwmon/max6650.txt
index d6c10e3..f6bd87d 100644
--- a/Documentation/devicetree/bindings/hwmon/max6650.txt
+++ b/Documentation/devicetree/bindings/hwmon/max6650.txt
@@ -13,6 +13,10 @@ Optional properties, default is to retain the chip's current setting:
 - maxim,fan-prescale  : Pre-scaling value, as per datasheet [1]. Lower values
 			allow more fine-grained control of slower fans.
 			Valid: 1, 2, 4, 8, 16.
+- maxim,fan-target-rpm: Initial requested fan rotation speed. If specified, the
+			driver selects closed-loop mode and the requested speed.
+			This ensures the fan is already running before userspace
+			takes over.
 
 Example:
 	fan-max6650: max6650@1b {
@@ -20,4 +24,5 @@ Example:
 		compatible = "maxim,max6650";
 		maxim,fan-microvolt = <12000000>;
 		maxim,fan-prescale = <4>;
+		maxim,fan-target-rpm = <1200>;
 	};
-- 
1.9.1

Re: [PATCH 2/2] hwmon: (max6650) Add initial rpm target devicetree documentation

[Rob Herring]

On Wed, Aug 24, 2016 at 10:13:27AM +0200, Mike Looijmans wrote:
> Add devicetree property for early initialization of the fan controller
> to prevent overheating, for example when resetting the board while the
> fan was completely turned off.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
>  Documentation/devicetree/bindings/hwmon/max6650.txt | 5 +++++
>  1 file changed, 5 insertions(+)

The kernel boot seems kind of late to ensure your fan is running, but

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 2/2] hwmon: (max6650) Add initial rpm target devicetree documentation

[Mike Looijmans]

On 30-08-16 18:38, Rob Herring wrote:
> On Wed, Aug 24, 2016 at 10:13:27AM +0200, Mike Looijmans wrote:
>> Add devicetree property for early initialization of the fan controller
>> to prevent overheating, for example when resetting the board while the
>> fan was completely turned off.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>> ---
>>   Documentation/devicetree/bindings/hwmon/max6650.txt | 5 +++++
>>   1 file changed, 5 insertions(+)
>
> The kernel boot seems kind of late to ensure your fan is running, but
>
> Acked-by: Rob Herring <robh@kernel.org>

Thanks!

Well, "late's better than never," my grandma used to say when we visited :)

To put your mind at ease, the particular platform I'm using this for is 
an xc7z045 (Zynq) where the ARMs running the Linux kernel will only 
account for 0.5W, but once the logic awakens the chip can dissipate up 
to 30W. The logic bitstream is usually on the rootfs, so kernel boot is 
early enough for the fan to start spinning. And with the logic asleep, 
it need not spin at all.


-- 
Mike Looijmans

Re: [PATCH 1/2] hwmon: (max6650) Allow fan shutdown and initial rpm target

[Guenter Roeck]

On 08/24/2016 01:13 AM, Mike Looijmans wrote:
> The fan can be stopped by writing "3" to pwm1_enable in sysfs.
>
> Add devicetree property for early initialization of the fan controller
> to prevent overheating, for example when resetting the board while the
> fan was completely turned off.
>
> Also improve error reporting, I2C failures were ignored while writing
> new values.
>
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>


Applied to hwmon-next.

Thanks,
Guenter