[PATCH v3 0/6] hwmon: pwm-fan: switch regulator dynamically

[PATCH v3 0/6] hwmon: pwm-fan: switch regulator dynamically

[Alexander Stein]

Hello everyone,

this is a new version for supporting switching off the pwm-fan regulator.
This time it is split into several smaller patches for ease of review.
Some organizational changes are inspired/copied by/from pwm_bl.

One big drawback form v2 was that there was no distinction bewteen when
PWM duty == 0:
* keep PWM on inactive level enabled (regulator as well)
* disable PWM and regulator

This is accomplished by using HWMON_PWM_ENABLE attribute. Documentation is
added accordingly.

Best regards,
Alexander

Alexander Stein (6):
  hwmon: pwm-fan: Refactor fan power on/off
  hwmon: pwm-fan: Simplify enable/disable check
  hwmon: pwm-fan: Dynamically switch off regulator if PWM duty is 0
  hwmon: pwm-fan: Remove internal duplicated pwm_state
  hwmon: pwm-fan: Move PWM enable into separate function
  hwmon: pwm-fan: Add hwmon_pwm_enable attribute

 Documentation/hwmon/pwm-fan.rst |  10 ++
 drivers/hwmon/pwm-fan.c         | 263 ++++++++++++++++++++++----------
 2 files changed, 191 insertions(+), 82 deletions(-)

-- 
2.25.1

[PATCH v3 1/6] hwmon: pwm-fan: Refactor fan power on/off

[Alexander Stein]

In preparation for dynamically switching regulator, split the power on
and power off sequence into separate functions.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/hwmon/pwm-fan.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 6c08551d8d14..458914d01155 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -82,23 +82,47 @@ static void sample_timer(struct timer_list *t)
 	mod_timer(&ctx->rpm_timer, jiffies + HZ);
 }
 
-static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
+static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
 {
+	struct pwm_state *state = &ctx->pwm_state;
 	unsigned long period;
+	int ret;
+
+	period = state->period;
+	state->duty_cycle = DIV_ROUND_UP(ctx->pwm_value * (period - 1), MAX_PWM);
+	state->enabled = true;
+	ret = pwm_apply_state(ctx->pwm, state);
+
+	return ret;
+}
+
+static int pwm_fan_power_off(struct pwm_fan_ctx *ctx)
+{
+	struct pwm_state state;
+
+	state.enabled = false;
+	state.duty_cycle = 0;
+	pwm_apply_state(ctx->pwm, &state);
+
+	return 0;
+}
+
+static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
+{
 	int ret = 0;
-	struct pwm_state *state = &ctx->pwm_state;
 
 	mutex_lock(&ctx->lock);
 	if (ctx->pwm_value == pwm)
 		goto exit_set_pwm_err;
 
-	period = state->period;
-	state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
-	state->enabled = pwm ? true : false;
+	if (pwm > 0)
+		ret = pwm_fan_power_on(ctx);
+	else
+		ret = pwm_fan_power_off(ctx);
 
-	ret = pwm_apply_state(ctx->pwm, state);
 	if (!ret)
 		ctx->pwm_value = pwm;
+
 exit_set_pwm_err:
 	mutex_unlock(&ctx->lock);
 	return ret;
-- 
2.25.1

[PATCH v3 2/6] hwmon: pwm-fan: Simplify enable/disable check

[Alexander Stein]

Instead of comparing the current to the new pwm duty to decide whether to
enable the PWM, use a dedicated flag. Also apply the new PWM duty in any
case. This is a preparation to enable/disable the regulator dynamically.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/hwmon/pwm-fan.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 458914d01155..2b7b26ab9b88 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -29,10 +29,13 @@ struct pwm_fan_tach {
 };
 
 struct pwm_fan_ctx {
+	struct device *dev;
+
 	struct mutex lock;
 	struct pwm_device *pwm;
 	struct pwm_state pwm_state;
 	struct regulator *reg_en;
+	bool enabled;
 
 	int tach_count;
 	struct pwm_fan_tach *tachs;
@@ -85,14 +88,21 @@ static void sample_timer(struct timer_list *t)
 static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
 {
 	struct pwm_state *state = &ctx->pwm_state;
-	unsigned long period;
 	int ret;
 
-	period = state->period;
-	state->duty_cycle = DIV_ROUND_UP(ctx->pwm_value * (period - 1), MAX_PWM);
+	if (ctx->enabled)
+		return 0;
+
 	state->enabled = true;
 	ret = pwm_apply_state(ctx->pwm, state);
+	if (ret) {
+		dev_err(ctx->dev, "failed to enable PWM\n");
+		goto err;
+	}
 
+	ctx->enabled = true;
+
+err:
 	return ret;
 }
 
@@ -100,26 +110,36 @@ static int pwm_fan_power_off(struct pwm_fan_ctx *ctx)
 {
 	struct pwm_state state;
 
+	if (!ctx->enabled)
+		return 0;
+
 	state.enabled = false;
 	state.duty_cycle = 0;
 	pwm_apply_state(ctx->pwm, &state);
 
+	ctx->enabled = false;
+
 	return 0;
 }
 
 static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
 {
+	struct pwm_state *state = &ctx->pwm_state;
+	unsigned long period;
 	int ret = 0;
 
 	mutex_lock(&ctx->lock);
-	if (ctx->pwm_value == pwm)
-		goto exit_set_pwm_err;
 
-	if (pwm > 0)
+	if (pwm > 0) {
+		period = state->period;
+		state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
+		ret = pwm_apply_state(ctx->pwm, state);
+		if (ret)
+			goto exit_set_pwm_err;
 		ret = pwm_fan_power_on(ctx);
-	else
+	} else {
 		ret = pwm_fan_power_off(ctx);
-
+	}
 	if (!ret)
 		ctx->pwm_value = pwm;
 
@@ -326,6 +346,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
 
 	mutex_init(&ctx->lock);
 
+	ctx->dev = &pdev->dev;
 	ctx->pwm = devm_of_pwm_get(dev, dev->of_node, NULL);
 	if (IS_ERR(ctx->pwm))
 		return dev_err_probe(dev, PTR_ERR(ctx->pwm), "Could not get PWM\n");
-- 
2.25.1

[PATCH v3 3/6] hwmon: pwm-fan: Dynamically switch off regulator if PWM duty is 0

[Alexander Stein]

Instead of powering the PWM fan all the time, turn off the regulator if
the PWM is disabled. This is especially important for fan using inverted
PWM signal polarity. Having regulator supplied and PWM disabled, some
PWM controllers provide the active, rather than inactive signal.
With this change the shutdown as well as suspend/resume paths require
modifcations as well.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/hwmon/pwm-fan.c | 91 ++++++++++++-----------------------------
 1 file changed, 27 insertions(+), 64 deletions(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 2b7b26ab9b88..c757af514ede 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -93,16 +93,25 @@ static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
 	if (ctx->enabled)
 		return 0;
 
+	ret = regulator_enable(ctx->reg_en);
+	if (ret < 0) {
+		dev_err(ctx->dev, "failed to enable power supply\n");
+		return ret;
+	}
+
 	state->enabled = true;
 	ret = pwm_apply_state(ctx->pwm, state);
 	if (ret) {
 		dev_err(ctx->dev, "failed to enable PWM\n");
-		goto err;
+		goto disable_regulator;
 	}
 
 	ctx->enabled = true;
 
-err:
+	return 0;
+
+disable_regulator:
+	regulator_disable(ctx->reg_en);
 	return ret;
 }
 
@@ -117,6 +126,8 @@ static int pwm_fan_power_off(struct pwm_fan_ctx *ctx)
 	state.duty_cycle = 0;
 	pwm_apply_state(ctx->pwm, &state);
 
+	regulator_disable(ctx->reg_en);
+
 	ctx->enabled = false;
 
 	return 0;
@@ -314,18 +325,12 @@ static int pwm_fan_of_get_cooling_data(struct device *dev,
 	return 0;
 }
 
-static void pwm_fan_regulator_disable(void *data)
-{
-	regulator_disable(data);
-}
-
-static void pwm_fan_pwm_disable(void *__ctx)
+static void pwm_fan_cleanup(void *__ctx)
 {
 	struct pwm_fan_ctx *ctx = __ctx;
 
-	ctx->pwm_state.enabled = false;
-	pwm_apply_state(ctx->pwm, &ctx->pwm_state);
 	del_timer_sync(&ctx->rpm_timer);
+	pwm_fan_power_off(ctx);
 }
 
 static int pwm_fan_probe(struct platform_device *pdev)
@@ -359,16 +364,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
 			return PTR_ERR(ctx->reg_en);
 
 		ctx->reg_en = NULL;
-	} else {
-		ret = regulator_enable(ctx->reg_en);
-		if (ret) {
-			dev_err(dev, "Failed to enable fan supply: %d\n", ret);
-			return ret;
-		}
-		ret = devm_add_action_or_reset(dev, pwm_fan_regulator_disable,
-					       ctx->reg_en);
-		if (ret)
-			return ret;
 	}
 
 	pwm_init_state(ctx->pwm, &ctx->pwm_state);
@@ -383,14 +378,17 @@ static int pwm_fan_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	/* Set duty cycle to maximum allowed and enable PWM output */
+	/*
+	 * Set duty cycle to maximum allowed and enable PWM output as well as
+	 * the regulator. In case of error nothing is changed
+	 */
 	ret = __set_pwm(ctx, MAX_PWM);
 	if (ret) {
 		dev_err(dev, "Failed to configure PWM: %d\n", ret);
 		return ret;
 	}
 	timer_setup(&ctx->rpm_timer, sample_timer, 0);
-	ret = devm_add_action_or_reset(dev, pwm_fan_pwm_disable, ctx);
+	ret = devm_add_action_or_reset(dev, pwm_fan_cleanup, ctx);
 	if (ret)
 		return ret;
 
@@ -496,61 +494,26 @@ static int pwm_fan_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int pwm_fan_disable(struct device *dev)
-{
-	struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
-	int ret;
-
-	if (ctx->pwm_value) {
-		/* keep ctx->pwm_state unmodified for pwm_fan_resume() */
-		struct pwm_state state = ctx->pwm_state;
-
-		state.duty_cycle = 0;
-		state.enabled = false;
-		ret = pwm_apply_state(ctx->pwm, &state);
-		if (ret < 0)
-			return ret;
-	}
-
-	if (ctx->reg_en) {
-		ret = regulator_disable(ctx->reg_en);
-		if (ret) {
-			dev_err(dev, "Failed to disable fan supply: %d\n", ret);
-			return ret;
-		}
-	}
-
-	return 0;
-}
-
 static void pwm_fan_shutdown(struct platform_device *pdev)
 {
-	pwm_fan_disable(&pdev->dev);
+	struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev);
+
+	pwm_fan_cleanup(ctx);
 }
 
 #ifdef CONFIG_PM_SLEEP
 static int pwm_fan_suspend(struct device *dev)
 {
-	return pwm_fan_disable(dev);
+	struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
+
+	return pwm_fan_power_off(ctx);
 }
 
 static int pwm_fan_resume(struct device *dev)
 {
 	struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
-	int ret;
-
-	if (ctx->reg_en) {
-		ret = regulator_enable(ctx->reg_en);
-		if (ret) {
-			dev_err(dev, "Failed to enable fan supply: %d\n", ret);
-			return ret;
-		}
-	}
-
-	if (ctx->pwm_value == 0)
-		return 0;
 
-	return pwm_apply_state(ctx->pwm, &ctx->pwm_state);
+	return __set_pwm(ctx, ctx->pwm_value);
 }
 #endif
 
-- 
2.25.1

[PATCH v3 4/6] hwmon: pwm-fan: Remove internal duplicated pwm_state

[Alexander Stein]

Each pwm device has already a pwm_state. Use this one instead of
managing an own copy of it.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/hwmon/pwm-fan.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index c757af514ede..21bfd0e931ba 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -33,7 +33,6 @@ struct pwm_fan_ctx {
 
 	struct mutex lock;
 	struct pwm_device *pwm;
-	struct pwm_state pwm_state;
 	struct regulator *reg_en;
 	bool enabled;
 
@@ -87,7 +86,7 @@ static void sample_timer(struct timer_list *t)
 
 static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
 {
-	struct pwm_state *state = &ctx->pwm_state;
+	struct pwm_state state;
 	int ret;
 
 	if (ctx->enabled)
@@ -99,8 +98,9 @@ static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
 		return ret;
 	}
 
-	state->enabled = true;
-	ret = pwm_apply_state(ctx->pwm, state);
+	pwm_get_state(ctx->pwm, &state);
+	state.enabled = true;
+	ret = pwm_apply_state(ctx->pwm, &state);
 	if (ret) {
 		dev_err(ctx->dev, "failed to enable PWM\n");
 		goto disable_regulator;
@@ -122,6 +122,7 @@ static int pwm_fan_power_off(struct pwm_fan_ctx *ctx)
 	if (!ctx->enabled)
 		return 0;
 
+	pwm_get_state(ctx->pwm, &state);
 	state.enabled = false;
 	state.duty_cycle = 0;
 	pwm_apply_state(ctx->pwm, &state);
@@ -135,16 +136,17 @@ static int pwm_fan_power_off(struct pwm_fan_ctx *ctx)
 
 static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
 {
-	struct pwm_state *state = &ctx->pwm_state;
+	struct pwm_state state;
 	unsigned long period;
 	int ret = 0;
 
 	mutex_lock(&ctx->lock);
 
 	if (pwm > 0) {
-		period = state->period;
-		state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
-		ret = pwm_apply_state(ctx->pwm, state);
+		pwm_get_state(ctx->pwm, &state);
+		period = state.period;
+		state.duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
+		ret = pwm_apply_state(ctx->pwm, &state);
 		if (ret)
 			goto exit_set_pwm_err;
 		ret = pwm_fan_power_on(ctx);
@@ -338,6 +340,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
 	struct thermal_cooling_device *cdev;
 	struct device *dev = &pdev->dev;
 	struct pwm_fan_ctx *ctx;
+	struct pwm_state state;
 	struct device *hwmon;
 	int ret;
 	const struct hwmon_channel_info **channels;
@@ -366,18 +369,25 @@ static int pwm_fan_probe(struct platform_device *pdev)
 		ctx->reg_en = NULL;
 	}
 
-	pwm_init_state(ctx->pwm, &ctx->pwm_state);
+	pwm_init_state(ctx->pwm, &state);
 
 	/*
 	 * __set_pwm assumes that MAX_PWM * (period - 1) fits into an unsigned
 	 * long. Check this here to prevent the fan running at a too low
 	 * frequency.
 	 */
-	if (ctx->pwm_state.period > ULONG_MAX / MAX_PWM + 1) {
+	if (state.period > ULONG_MAX / MAX_PWM + 1) {
 		dev_err(dev, "Configured period too big\n");
 		return -EINVAL;
 	}
 
+	/* Apply modified PWM default state */
+	ret = pwm_apply_state(ctx->pwm, &state);
+	if (ret) {
+		dev_err(dev, "failed to apply initial PWM state: %d\n", ret);
+		return -EINVAL;
+	}
+
 	/*
 	 * Set duty cycle to maximum allowed and enable PWM output as well as
 	 * the regulator. In case of error nothing is changed
-- 
2.25.1

[PATCH v3 5/6] hwmon: pwm-fan: Move PWM enable into separate function

[Alexander Stein]

This allows reusage in other code paths.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/hwmon/pwm-fan.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 21bfd0e931ba..9ebe958cc908 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -84,9 +84,17 @@ static void sample_timer(struct timer_list *t)
 	mod_timer(&ctx->rpm_timer, jiffies + HZ);
 }
 
-static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
+static int pwm_fan_enable_pwm(struct pwm_fan_ctx *ctx)
 {
 	struct pwm_state state;
+
+	pwm_get_state(ctx->pwm, &state);
+	state.enabled = true;
+	return pwm_apply_state(ctx->pwm, &state);
+}
+
+static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
+{
 	int ret;
 
 	if (ctx->enabled)
@@ -98,9 +106,7 @@ static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
 		return ret;
 	}
 
-	pwm_get_state(ctx->pwm, &state);
-	state.enabled = true;
-	ret = pwm_apply_state(ctx->pwm, &state);
+	ret = pwm_fan_enable_pwm(ctx);
 	if (ret) {
 		dev_err(ctx->dev, "failed to enable PWM\n");
 		goto disable_regulator;
-- 
2.25.1

[PATCH v3 6/6] hwmon: pwm-fan: Add hwmon_pwm_enable attribute

[Alexander Stein]

This adds the enable attribute which is used to differentiate if PWM duty
means to switch off regulator and PWM or to keep them enabled but
at inactive PWM output level.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 Documentation/hwmon/pwm-fan.rst | 10 ++++
 drivers/hwmon/pwm-fan.c         | 95 +++++++++++++++++++++++++++++----
 2 files changed, 95 insertions(+), 10 deletions(-)

diff --git a/Documentation/hwmon/pwm-fan.rst b/Documentation/hwmon/pwm-fan.rst
index 82fe96742fee..0083480068d1 100644
--- a/Documentation/hwmon/pwm-fan.rst
+++ b/Documentation/hwmon/pwm-fan.rst
@@ -18,3 +18,13 @@ the hwmon's sysfs interface.
 
 The fan rotation speed returned via the optional 'fan1_input' is extrapolated
 from the sampled interrupts from the tachometer signal within 1 second.
+
+The driver provides the following sensor accesses in sysfs:
+
+=============== ======= =======================================================
+fan1_input	ro	fan tachometer speed in RPM
+pwm1_enable	rw	keep enable mode, defines behaviour when pwm1=0
+			0=switch off regulator and disable PWM
+			1=keep regulator enabled and set PWM to inactive level
+pwm1		rw	relative speed (0-255), 255=max. speed.
+=============== ======= =======================================================
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 9ebe958cc908..cb29206ddcdc 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -35,6 +35,7 @@ struct pwm_fan_ctx {
 	struct pwm_device *pwm;
 	struct regulator *reg_en;
 	bool enabled;
+	bool keep_enabled;
 
 	int tach_count;
 	struct pwm_fan_tach *tachs;
@@ -129,7 +130,8 @@ static int pwm_fan_power_off(struct pwm_fan_ctx *ctx)
 		return 0;
 
 	pwm_get_state(ctx->pwm, &state);
-	state.enabled = false;
+	if (!ctx->keep_enabled)
+		state.enabled = false;
 	state.duty_cycle = 0;
 	pwm_apply_state(ctx->pwm, &state);
 
@@ -178,20 +180,87 @@ static void pwm_fan_update_state(struct pwm_fan_ctx *ctx, unsigned long pwm)
 	ctx->pwm_fan_state = i;
 }
 
+static int pwm_fan_update_enable(struct pwm_fan_ctx *ctx, long val)
+{
+	struct pwm_state old_state;
+	int ret;
+
+	pwm_get_state(ctx->pwm, &old_state);
+
+	if (val) {
+		/*
+		 * If PWM is already enabled, nothing will change
+		 * If PWM is disabled, it will enable with inactive level (duty == 0)
+		 */
+		ret = pwm_fan_enable_pwm(ctx);
+		if (ret) {
+			dev_err(ctx->dev, "failed to apply PWM state\n");
+			return ret;
+		}
+
+		/* Increase regulator reference counter to prevent switching off */
+		ret = regulator_enable(ctx->reg_en);
+		if (ret < 0) {
+			dev_err(ctx->dev, "failed to enable regulator\n");
+			/* Restore old state */
+			pwm_apply_state(ctx->pwm, &old_state);
+		}
+	} else {
+		/* Only disable PWM if currently on inactive state */
+		if (!ctx->pwm_value) {
+			struct pwm_state disable_state;
+
+			pwm_get_state(ctx->pwm, &disable_state);
+			disable_state.duty_cycle = 0;
+			disable_state.enabled = false;
+			ret = pwm_apply_state(ctx->pwm, &disable_state);
+			if (ret) {
+				dev_err(ctx->dev, "failed to apply PWM state\n");
+				return ret;
+			}
+		}
+		/* Decrease regulator reference counter */
+		ret = regulator_disable(ctx->reg_en);
+		if (ret < 0) {
+			dev_err(ctx->dev, "failed to decrease power supply usage\n");
+			/* Restore old state */
+			pwm_apply_state(ctx->pwm, &old_state);
+		}
+	}
+	if (ret == 0)
+		ctx->keep_enabled = val;
+
+	return ret;
+}
+
 static int pwm_fan_write(struct device *dev, enum hwmon_sensor_types type,
 			 u32 attr, int channel, long val)
 {
 	struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
 	int ret;
 
-	if (val < 0 || val > MAX_PWM)
-		return -EINVAL;
+	switch (attr) {
+	case hwmon_pwm_input:
+		if (val < 0 || val > MAX_PWM)
+			return -EINVAL;
+		ret = __set_pwm(ctx, val);
+		if (ret)
+			return ret;
+		pwm_fan_update_state(ctx, val);
+		break;
+	case hwmon_pwm_enable:
+		mutex_lock(&ctx->lock);
+		if (val < 0 || val > 1)
+			ret = -EINVAL;
+		else if (ctx->keep_enabled != val)
+			ret = pwm_fan_update_enable(ctx, val);
+		mutex_unlock(&ctx->lock);
 
-	ret = __set_pwm(ctx, val);
-	if (ret)
 		return ret;
+	default:
+		return -EOPNOTSUPP;
+	}
 
-	pwm_fan_update_state(ctx, val);
 	return 0;
 }
 
@@ -202,9 +271,15 @@ static int pwm_fan_read(struct device *dev, enum hwmon_sensor_types type,
 
 	switch (type) {
 	case hwmon_pwm:
-		*val = ctx->pwm_value;
-		return 0;
-
+		switch (attr) {
+		case hwmon_pwm_input:
+			*val = ctx->pwm_value;
+			return 0;
+		case hwmon_pwm_enable:
+			*val = ctx->keep_enabled;
+			return 0;
+		}
+		return -EOPNOTSUPP;
 	case hwmon_fan:
 		*val = ctx->tachs[channel].rpm;
 		return 0;
@@ -436,7 +511,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
 	if (!channels)
 		return -ENOMEM;
 
-	channels[0] = HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT);
+	channels[0] = HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE);
 
 	for (i = 0; i < ctx->tach_count; i++) {
 		struct pwm_fan_tach *tach = &ctx->tachs[i];
-- 
2.25.1

Re: [PATCH v3 6/6] hwmon: pwm-fan: Add hwmon_pwm_enable attribute

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1797 bytes --]

Hello,

On Tue, May 17, 2022 at 04:26:20PM +0200, Alexander Stein wrote:
> This adds the enable attribute which is used to differentiate if PWM duty
> means to switch off regulator and PWM or to keep them enabled but
> at inactive PWM output level.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
>  Documentation/hwmon/pwm-fan.rst | 10 ++++
>  drivers/hwmon/pwm-fan.c         | 95 +++++++++++++++++++++++++++++----
>  2 files changed, 95 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/hwmon/pwm-fan.rst b/Documentation/hwmon/pwm-fan.rst
> index 82fe96742fee..0083480068d1 100644
> --- a/Documentation/hwmon/pwm-fan.rst
> +++ b/Documentation/hwmon/pwm-fan.rst
> @@ -18,3 +18,13 @@ the hwmon's sysfs interface.
>  
>  The fan rotation speed returned via the optional 'fan1_input' is extrapolated
>  from the sampled interrupts from the tachometer signal within 1 second.
> +
> +The driver provides the following sensor accesses in sysfs:
> +
> +=============== ======= =======================================================
> +fan1_input	ro	fan tachometer speed in RPM
> +pwm1_enable	rw	keep enable mode, defines behaviour when pwm1=0
> +			0=switch off regulator and disable PWM
> +			1=keep regulator enabled and set PWM to inactive level

Is the pwm1_enable supposed to be set to 0 if that only does the right
thing if the PWM emits low after pwm_disable()? The question I raised in
v2 about "what is the meaning of disable?" hasn't evolved, has it?

I still think it's unfortunate, that "pwm1_enable" has an effect on the
regulator.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 6/6] hwmon: pwm-fan: Add hwmon_pwm_enable attribute

[Guenter Roeck]

On 5/17/22 07:53, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, May 17, 2022 at 04:26:20PM +0200, Alexander Stein wrote:
>> This adds the enable attribute which is used to differentiate if PWM duty
>> means to switch off regulator and PWM or to keep them enabled but
>> at inactive PWM output level.
>>
>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>> ---
>>   Documentation/hwmon/pwm-fan.rst | 10 ++++
>>   drivers/hwmon/pwm-fan.c         | 95 +++++++++++++++++++++++++++++----
>>   2 files changed, 95 insertions(+), 10 deletions(-)
>>
>> diff --git a/Documentation/hwmon/pwm-fan.rst b/Documentation/hwmon/pwm-fan.rst
>> index 82fe96742fee..0083480068d1 100644
>> --- a/Documentation/hwmon/pwm-fan.rst
>> +++ b/Documentation/hwmon/pwm-fan.rst
>> @@ -18,3 +18,13 @@ the hwmon's sysfs interface.
>>   
>>   The fan rotation speed returned via the optional 'fan1_input' is extrapolated
>>   from the sampled interrupts from the tachometer signal within 1 second.
>> +
>> +The driver provides the following sensor accesses in sysfs:
>> +
>> +=============== ======= =======================================================
>> +fan1_input	ro	fan tachometer speed in RPM
>> +pwm1_enable	rw	keep enable mode, defines behaviour when pwm1=0
>> +			0=switch off regulator and disable PWM
>> +			1=keep regulator enabled and set PWM to inactive level
> 
> Is the pwm1_enable supposed to be set to 0 if that only does the right
> thing if the PWM emits low after pwm_disable()? The question I raised in
> v2 about "what is the meaning of disable?" hasn't evolved, has it?
> 
> I still think it's unfortunate, that "pwm1_enable" has an effect on the
> regulator.
> 

Trying to understand. Are you saying that you are ok with affecting the
regulator when setting pwm := 0 (even though that doesn't really mean
"disable pwm output"), but not with making the behavior explicit by
using pwm1_enable ?

Thanks,
Guenter

Re: [PATCH v3 6/6] hwmon: pwm-fan: Add hwmon_pwm_enable attribute

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3278 bytes --]

Hello,

[dropping Bartlomiej Zolnierkiewicz from Cc as the address bounces]

On Tue, May 17, 2022 at 09:32:24AM -0700, Guenter Roeck wrote:
> On 5/17/22 07:53, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Tue, May 17, 2022 at 04:26:20PM +0200, Alexander Stein wrote:
> > > This adds the enable attribute which is used to differentiate if PWM duty
> > > means to switch off regulator and PWM or to keep them enabled but
> > > at inactive PWM output level.
> > > 
> > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > ---
> > >   Documentation/hwmon/pwm-fan.rst | 10 ++++
> > >   drivers/hwmon/pwm-fan.c         | 95 +++++++++++++++++++++++++++++----
> > >   2 files changed, 95 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/Documentation/hwmon/pwm-fan.rst b/Documentation/hwmon/pwm-fan.rst
> > > index 82fe96742fee..0083480068d1 100644
> > > --- a/Documentation/hwmon/pwm-fan.rst
> > > +++ b/Documentation/hwmon/pwm-fan.rst
> > > @@ -18,3 +18,13 @@ the hwmon's sysfs interface.
> > >   The fan rotation speed returned via the optional 'fan1_input' is extrapolated
> > >   from the sampled interrupts from the tachometer signal within 1 second.
> > > +
> > > +The driver provides the following sensor accesses in sysfs:
> > > +
> > > +=============== ======= =======================================================
> > > +fan1_input	ro	fan tachometer speed in RPM
> > > +pwm1_enable	rw	keep enable mode, defines behaviour when pwm1=0
> > > +			0=switch off regulator and disable PWM
> > > +			1=keep regulator enabled and set PWM to inactive level
> > 
> > Is the pwm1_enable supposed to be set to 0 if that only does the right
> > thing if the PWM emits low after pwm_disable()? The question I raised in
> > v2 about "what is the meaning of disable?" hasn't evolved, has it?
> > 
> > I still think it's unfortunate, that "pwm1_enable" has an effect on the
> > regulator.
> > 
> 
> Trying to understand. Are you saying that you are ok with affecting the
> regulator when setting pwm := 0 (even though that doesn't really mean
> "disable pwm output"), but not with making the behavior explicit by
> using pwm1_enable ?

Not sure about being ok with affecting the regulator when setting
pwm := 0. I don't know enough about pwm-fans to have a strong opinion
for that.

Some questions to refine the semantics and my opinion:

There are fans without a regulator? (I think: yes)

A fan with a regulator always stops if the regulator is off?
(Hmm, there might be problems with shared regulators that only go off
when all consumers are off? What about always-on regulators, these don't
go off on the last consumer calling disable, right?)

Having said that I think the sane behaviour is:

The intention of pwm := 0 is to stop the fan. So disabling the regulator
(if available) sounds right.

I'm unsure what to reasonably expect from a disabled PWM. I think "stops
to oscillate" can be assumed. So I'd say: If a fan continues to rotate
when the PWM input is constantly active, don't call pwm_disable().

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 6/6] hwmon: pwm-fan: Add hwmon_pwm_enable attribute

[Guenter Roeck]

On 5/17/22 09:57, Uwe Kleine-König wrote:
> Hello,
> 
> [dropping Bartlomiej Zolnierkiewicz from Cc as the address bounces]
> 
> On Tue, May 17, 2022 at 09:32:24AM -0700, Guenter Roeck wrote:
>> On 5/17/22 07:53, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> On Tue, May 17, 2022 at 04:26:20PM +0200, Alexander Stein wrote:
>>>> This adds the enable attribute which is used to differentiate if PWM duty
>>>> means to switch off regulator and PWM or to keep them enabled but
>>>> at inactive PWM output level.
>>>>
>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>>>> ---
>>>>    Documentation/hwmon/pwm-fan.rst | 10 ++++
>>>>    drivers/hwmon/pwm-fan.c         | 95 +++++++++++++++++++++++++++++----
>>>>    2 files changed, 95 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/Documentation/hwmon/pwm-fan.rst b/Documentation/hwmon/pwm-fan.rst
>>>> index 82fe96742fee..0083480068d1 100644
>>>> --- a/Documentation/hwmon/pwm-fan.rst
>>>> +++ b/Documentation/hwmon/pwm-fan.rst
>>>> @@ -18,3 +18,13 @@ the hwmon's sysfs interface.
>>>>    The fan rotation speed returned via the optional 'fan1_input' is extrapolated
>>>>    from the sampled interrupts from the tachometer signal within 1 second.
>>>> +
>>>> +The driver provides the following sensor accesses in sysfs:
>>>> +
>>>> +=============== ======= =======================================================
>>>> +fan1_input	ro	fan tachometer speed in RPM
>>>> +pwm1_enable	rw	keep enable mode, defines behaviour when pwm1=0
>>>> +			0=switch off regulator and disable PWM
>>>> +			1=keep regulator enabled and set PWM to inactive level
>>>
>>> Is the pwm1_enable supposed to be set to 0 if that only does the right
>>> thing if the PWM emits low after pwm_disable()? The question I raised in
>>> v2 about "what is the meaning of disable?" hasn't evolved, has it?
>>>
>>> I still think it's unfortunate, that "pwm1_enable" has an effect on the
>>> regulator.
>>>
>>
>> Trying to understand. Are you saying that you are ok with affecting the
>> regulator when setting pwm := 0 (even though that doesn't really mean
>> "disable pwm output"), but not with making the behavior explicit by
>> using pwm1_enable ?
> 
> Not sure about being ok with affecting the regulator when setting
> pwm := 0. I don't know enough about pwm-fans to have a strong opinion
> for that.
> 
> Some questions to refine the semantics and my opinion:
> 
> There are fans without a regulator? (I think: yes)
> 
> A fan with a regulator always stops if the regulator is off?
> (Hmm, there might be problems with shared regulators that only go off
> when all consumers are off? What about always-on regulators, these don't
> go off on the last consumer calling disable, right?)
> 
> Having said that I think the sane behaviour is:
> 
> The intention of pwm := 0 is to stop the fan. So disabling the regulator
> (if available) sounds right.
> 

There are fans (eg at least some CPU fans) which never stop, even with
pwm=0. How do you suggest to handle those ?

Guenter

> I'm unsure what to reasonably expect from a disabled PWM. I think "stops
> to oscillate" can be assumed. So I'd say: If a fan continues to rotate
> when the PWM input is constantly active, don't call pwm_disable().
> 
> Best regards
> Uwe
> 

Re: [PATCH v3 6/6] hwmon: pwm-fan: Add hwmon_pwm_enable attribute

[Alexander Stein]

Am Dienstag, 17. Mai 2022, 19:32:11 CEST schrieb Guenter Roeck:
> On 5/17/22 09:57, Uwe Kleine-König wrote:
> > Hello,
> > 
> > [dropping Bartlomiej Zolnierkiewicz from Cc as the address bounces]
> > 
> > On Tue, May 17, 2022 at 09:32:24AM -0700, Guenter Roeck wrote:
> >> On 5/17/22 07:53, Uwe Kleine-König wrote:
> >>> Hello,
> >>> 
> >>> On Tue, May 17, 2022 at 04:26:20PM +0200, Alexander Stein wrote:
> >>>> This adds the enable attribute which is used to differentiate if PWM
> >>>> duty
> >>>> means to switch off regulator and PWM or to keep them enabled but
> >>>> at inactive PWM output level.
> >>>> 
> >>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> >>>> ---
> >>>> 
> >>>>    Documentation/hwmon/pwm-fan.rst | 10 ++++
> >>>>    drivers/hwmon/pwm-fan.c         | 95
> >>>>    +++++++++++++++++++++++++++++----
> >>>>    2 files changed, 95 insertions(+), 10 deletions(-)
> >>>> 
> >>>> diff --git a/Documentation/hwmon/pwm-fan.rst
> >>>> b/Documentation/hwmon/pwm-fan.rst index 82fe96742fee..0083480068d1
> >>>> 100644
> >>>> --- a/Documentation/hwmon/pwm-fan.rst
> >>>> +++ b/Documentation/hwmon/pwm-fan.rst
> >>>> @@ -18,3 +18,13 @@ the hwmon's sysfs interface.
> >>>> 
> >>>>    The fan rotation speed returned via the optional 'fan1_input' is
> >>>>    extrapolated from the sampled interrupts from the tachometer signal
> >>>>    within 1 second.>>>> 
> >>>> +
> >>>> +The driver provides the following sensor accesses in sysfs:
> >>>> +
> >>>> +=============== =======
> >>>> =======================================================
> >>>> +fan1_input	ro	fan tachometer speed in RPM
> >>>> +pwm1_enable	rw	keep enable mode, defines behaviour when 
pwm1=0
> >>>> +			0=switch off regulator and disable PWM
> >>>> +			1=keep regulator enabled and set PWM to 
inactive level
> >>> 
> >>> Is the pwm1_enable supposed to be set to 0 if that only does the right
> >>> thing if the PWM emits low after pwm_disable()? The question I raised in
> >>> v2 about "what is the meaning of disable?" hasn't evolved, has it?
> >>> 
> >>> I still think it's unfortunate, that "pwm1_enable" has an effect on the
> >>> regulator.
> >> 
> >> Trying to understand. Are you saying that you are ok with affecting the
> >> regulator when setting pwm := 0 (even though that doesn't really mean
> >> "disable pwm output"), but not with making the behavior explicit by
> >> using pwm1_enable ?
> > 
> > Not sure about being ok with affecting the regulator when setting
> > pwm := 0. I don't know enough about pwm-fans to have a strong opinion
> > for that.

In my case (422J fan) just supplying voltage with inactive PWM results in a 
minimum rotation speed. So these two settings are coupled here.

> > Some questions to refine the semantics and my opinion:
> > 
> > There are fans without a regulator? (I think: yes)
> > 
> > A fan with a regulator always stops if the regulator is off?
> > (Hmm, there might be problems with shared regulators that only go off
> > when all consumers are off? What about always-on regulators, these don't
> > go off on the last consumer calling disable, right?)

IMHO this is something the system integrator shall manage. Is it possible to 
disable the regulator? No for shared/always-on ones. In this case stopping the 
fan needs to be done by setting PWM to inactive level and keep regulator on 
(pwm1_enable=1). If this results in a minimum rotation speed as it would using 
a 422J, it's pretty much impossible to actually stop the fan in such a system.

> > Having said that I think the sane behaviour is:
> > 
> > The intention of pwm := 0 is to stop the fan. So disabling the regulator
> > (if available) sounds right.
> 
> There are fans (eg at least some CPU fans) which never stop, even with
> pwm=0. How do you suggest to handle those ?

If it's impossible to stop the fan from the hardware side (e.g. always-on 
regulators), then software only can do this much.
Maybe adding those 4 states Uwe mentioned in [1] is not a bad idea. This way 
it's possible to configure the exact behavior one would want/need. This also 
can handle the cases where a disabled PWM has no/wrong defined state.

Best regards
Alexander

[1] https://lore.kernel.org/all/
20220517170658.u3dpe6gglsihh6n6@pengutronix.de/

Re: [PATCH v3 6/6] hwmon: pwm-fan: Add hwmon_pwm_enable attribute

[Guenter Roeck]

On 5/17/22 07:26, Alexander Stein wrote:
> This adds the enable attribute which is used to differentiate if PWM duty
> means to switch off regulator and PWM or to keep them enabled but
> at inactive PWM output level.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
>   Documentation/hwmon/pwm-fan.rst | 10 ++++
>   drivers/hwmon/pwm-fan.c         | 95 +++++++++++++++++++++++++++++----
>   2 files changed, 95 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/hwmon/pwm-fan.rst b/Documentation/hwmon/pwm-fan.rst
> index 82fe96742fee..0083480068d1 100644
> --- a/Documentation/hwmon/pwm-fan.rst
> +++ b/Documentation/hwmon/pwm-fan.rst
> @@ -18,3 +18,13 @@ the hwmon's sysfs interface.
>   
>   The fan rotation speed returned via the optional 'fan1_input' is extrapolated
>   from the sampled interrupts from the tachometer signal within 1 second.
> +
> +The driver provides the following sensor accesses in sysfs:
> +
> +=============== ======= =======================================================
> +fan1_input	ro	fan tachometer speed in RPM
> +pwm1_enable	rw	keep enable mode, defines behaviour when pwm1=0
> +			0=switch off regulator and disable PWM
> +			1=keep regulator enabled and set PWM to inactive level

Unless I am missing something, I think we have (at least) three
conditions to handle:

- regulator disabled (independent of pwm value)
- regulator enabled, pwm output disabled if pwm=0
- regulator enabled, pwm output enabled and set to 0 (or, if inverted,
   to maximum) if pwm=0

plus possibly:
- regulator disabled, pwm output disabled if pwm=0

Guenter

Re: [PATCH v3 6/6] hwmon: pwm-fan: Add hwmon_pwm_enable attribute

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2604 bytes --]

Hello,

[dropped Bartlomiej Zolnierkiewicz from Cc:]

On Tue, May 17, 2022 at 09:38:56AM -0700, Guenter Roeck wrote:
> On 5/17/22 07:26, Alexander Stein wrote:
> > This adds the enable attribute which is used to differentiate if PWM duty
> > means to switch off regulator and PWM or to keep them enabled but
> > at inactive PWM output level.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> >   Documentation/hwmon/pwm-fan.rst | 10 ++++
> >   drivers/hwmon/pwm-fan.c         | 95 +++++++++++++++++++++++++++++----
> >   2 files changed, 95 insertions(+), 10 deletions(-)
> > 
> > diff --git a/Documentation/hwmon/pwm-fan.rst b/Documentation/hwmon/pwm-fan.rst
> > index 82fe96742fee..0083480068d1 100644
> > --- a/Documentation/hwmon/pwm-fan.rst
> > +++ b/Documentation/hwmon/pwm-fan.rst
> > @@ -18,3 +18,13 @@ the hwmon's sysfs interface.
> >   The fan rotation speed returned via the optional 'fan1_input' is extrapolated
> >   from the sampled interrupts from the tachometer signal within 1 second.
> > +
> > +The driver provides the following sensor accesses in sysfs:
> > +
> > +=============== ======= =======================================================
> > +fan1_input	ro	fan tachometer speed in RPM
> > +pwm1_enable	rw	keep enable mode, defines behaviour when pwm1=0
> > +			0=switch off regulator and disable PWM
> > +			1=keep regulator enabled and set PWM to inactive level
> 
> Unless I am missing something, I think we have (at least) three
> conditions to handle:
> 
> - regulator disabled (independent of pwm value)
> - regulator enabled, pwm output disabled if pwm=0
> - regulator enabled, pwm output enabled and set to 0 (or, if inverted,
>   to maximum) if pwm=0

What is your expectation for a disabled PWM? 
https://lore.kernel.org/linux-pwm/20220517150555.404363-1-u.kleine-koenig@pengutronix.de
might be relevant. If you assume that a pwm might output the active
level after disabling, the case "regulator enabled, pwm output disabled
if pwm=0" sounds wrong.

Would "pwm1_disable_on_zero" be a better name than "pwm1_enable"? The
latter is completely unintuitive to me.

Maybe go for

 0 -> keep pwm and regulator on
 1 -> disable pwm, keep regulator on
 2 -> keep pwm on, disable regulator
 3 -> disable pwm and regulator

(so one bit for pwm and one for regulator), even if 1 is
wrong/unusual/dangerous?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: (EXT) Re: [PATCH v3 6/6] hwmon: pwm-fan: Add hwmon_pwm_enable attribute

[Alexander Stein]

Am Dienstag, 17. Mai 2022, 19:06:58 CEST schrieb Uwe Kleine-König:
> * PGP Signed by an unknown key
> 
> Hello,
> 
> [dropped Bartlomiej Zolnierkiewicz from Cc:]
> 
> On Tue, May 17, 2022 at 09:38:56AM -0700, Guenter Roeck wrote:
> > On 5/17/22 07:26, Alexander Stein wrote:
> > > This adds the enable attribute which is used to differentiate if PWM
> > > duty
> > > means to switch off regulator and PWM or to keep them enabled but
> > > at inactive PWM output level.
> > > 
> > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > ---
> > > 
> > >   Documentation/hwmon/pwm-fan.rst | 10 ++++
> > >   drivers/hwmon/pwm-fan.c         | 95 +++++++++++++++++++++++++++++----
> > >   2 files changed, 95 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/Documentation/hwmon/pwm-fan.rst
> > > b/Documentation/hwmon/pwm-fan.rst index 82fe96742fee..0083480068d1
> > > 100644
> > > --- a/Documentation/hwmon/pwm-fan.rst
> > > +++ b/Documentation/hwmon/pwm-fan.rst
> > > @@ -18,3 +18,13 @@ the hwmon's sysfs interface.
> > > 
> > >   The fan rotation speed returned via the optional 'fan1_input' is
> > >   extrapolated from the sampled interrupts from the tachometer signal
> > >   within 1 second.> > 
> > > +
> > > +The driver provides the following sensor accesses in sysfs:
> > > +
> > > +=============== =======
> > > =======================================================
> > > +fan1_input	ro	fan tachometer speed in RPM
> > > +pwm1_enable	rw	keep enable mode, defines behaviour when 
pwm1=0
> > > +			0=switch off regulator and disable PWM
> > > +			1=keep regulator enabled and set PWM to 
inactive level
> > 
> > Unless I am missing something, I think we have (at least) three
> > conditions to handle:
> > 
> > - regulator disabled (independent of pwm value)
> > - regulator enabled, pwm output disabled if pwm=0
> > - regulator enabled, pwm output enabled and set to 0 (or, if inverted,
> > 
> >   to maximum) if pwm=0
> 
> What is your expectation for a disabled PWM?
> https://lore.kernel.org/linux-pwm/20220517150555.404363-1-u.kleine-koenig@pe
> ngutronix.de might be relevant. If you assume that a pwm might output the
> active level after disabling, the case "regulator enabled, pwm output
> disabled if pwm=0" sounds wrong.
> 
> Would "pwm1_disable_on_zero" be a better name than "pwm1_enable"? The
> latter is completely unintuitive to me.

I guess Guenter suggested 'pwm1_enable' as it already exists as a predefined, 
optional attribute, avoiding adding a new custom attribute.
Reading Documentation/hwmon/w83627ehf.rst or Documentation/hwmon/nzxt-
smart2.rst I get the impression their meaning is pretty unrestricted.
If you are concerned by using 'pwm1_enable', what about 'pwm1_mode'?

> Maybe go for
> 
>  0 -> keep pwm and regulator on
>  1 -> disable pwm, keep regulator on
>  2 -> keep pwm on, disable regulator
>  3 -> disable pwm and regulator
> 
> (so one bit for pwm and one for regulator), even if 1 is
> wrong/unusual/dangerous?

I tend to like this approach, as it can handle all combinations. You can 
decide whether you want to actually shutdown the PWM fan, or keep it enabled 
but without providing any PWM. This can mean the fan still runs at the lowest 
speed. It also addresses the scenarios where regulator cannot be disabled at 
all.

Best regards,
Alexander

Re: (EXT) Re: [PATCH v3 6/6] hwmon: pwm-fan: Add hwmon_pwm_enable attribute

[Guenter Roeck]

On 5/18/22 00:06, Alexander Stein wrote:
> Am Dienstag, 17. Mai 2022, 19:06:58 CEST schrieb Uwe Kleine-König:
>> * PGP Signed by an unknown key
>>
>> Hello,
>>
>> [dropped Bartlomiej Zolnierkiewicz from Cc:]
>>
>> On Tue, May 17, 2022 at 09:38:56AM -0700, Guenter Roeck wrote:
>>> On 5/17/22 07:26, Alexander Stein wrote:
>>>> This adds the enable attribute which is used to differentiate if PWM
>>>> duty
>>>> means to switch off regulator and PWM or to keep them enabled but
>>>> at inactive PWM output level.
>>>>
>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>>>> ---
>>>>
>>>>    Documentation/hwmon/pwm-fan.rst | 10 ++++
>>>>    drivers/hwmon/pwm-fan.c         | 95 +++++++++++++++++++++++++++++----
>>>>    2 files changed, 95 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/Documentation/hwmon/pwm-fan.rst
>>>> b/Documentation/hwmon/pwm-fan.rst index 82fe96742fee..0083480068d1
>>>> 100644
>>>> --- a/Documentation/hwmon/pwm-fan.rst
>>>> +++ b/Documentation/hwmon/pwm-fan.rst
>>>> @@ -18,3 +18,13 @@ the hwmon's sysfs interface.
>>>>
>>>>    The fan rotation speed returned via the optional 'fan1_input' is
>>>>    extrapolated from the sampled interrupts from the tachometer signal
>>>>    within 1 second.> >
>>>> +
>>>> +The driver provides the following sensor accesses in sysfs:
>>>> +
>>>> +=============== =======
>>>> =======================================================
>>>> +fan1_input	ro	fan tachometer speed in RPM
>>>> +pwm1_enable	rw	keep enable mode, defines behaviour when
> pwm1=0
>>>> +			0=switch off regulator and disable PWM
>>>> +			1=keep regulator enabled and set PWM to
> inactive level
>>>
>>> Unless I am missing something, I think we have (at least) three
>>> conditions to handle:
>>>
>>> - regulator disabled (independent of pwm value)
>>> - regulator enabled, pwm output disabled if pwm=0
>>> - regulator enabled, pwm output enabled and set to 0 (or, if inverted,
>>>
>>>    to maximum) if pwm=0
>>
>> What is your expectation for a disabled PWM?
>> https://lore.kernel.org/linux-pwm/20220517150555.404363-1-u.kleine-koenig@pe
>> ngutronix.de might be relevant. If you assume that a pwm might output the
>> active level after disabling, the case "regulator enabled, pwm output
>> disabled if pwm=0" sounds wrong.
>>
>> Would "pwm1_disable_on_zero" be a better name than "pwm1_enable"? The
>> latter is completely unintuitive to me.
> 
> I guess Guenter suggested 'pwm1_enable' as it already exists as a predefined,

Correct.

> optional attribute, avoiding adding a new custom attribute.
> Reading Documentation/hwmon/w83627ehf.rst or Documentation/hwmon/nzxt-
> smart2.rst I get the impression their meaning is pretty unrestricted.
> If you are concerned by using 'pwm1_enable', what about 'pwm1_mode'?
> 

No. pwmX_mode sets the direct current vs. pulse width.

>> Maybe go for
>>
>>   0 -> keep pwm and regulator on
>>   1 -> disable pwm, keep regulator on
>>   2 -> keep pwm on, disable regulator
>>   3 -> disable pwm and regulator
>>
>> (so one bit for pwm and one for regulator), even if 1 is
>> wrong/unusual/dangerous?
> 

0 is for disable, not enable, and 1 should match the current implementation
for compatibility reasons. Something like

0 -> disable pwm and regulator
1 -> enable pwm; if pwm==0, disable pwm, keep regulator enabled
2 -> enable pwm; if pwm==0, keep pwm and regulator enabled
3 -> enable pwm; if pwm==0, disable pwm and regulator

should work.

Guenter

> I tend to like this approach, as it can handle all combinations. You can
> decide whether you want to actually shutdown the PWM fan, or keep it enabled
> but without providing any PWM. This can mean the fan still runs at the lowest
> speed. It also addresses the scenarios where regulator cannot be disabled at
> all.
> 
> Best regards,
> Alexander
> 
>