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

[PATCH v4 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.
pwm1_enable was reworked to supported the modes Guneter suggested, while
keeping the previous behavior. This also resulted in reordering and refactoring
the commits even more.

Changes in v4:
* Reordered commits so current behavior is the default all the time
* Fixed pwm state bug in Patch 1, this affects the patch context in Patch 2
* Refactor even more code in Patch 3 & 4 for smaller further patches
* Squashed the dynamic regulator switch patch and the sysfs attribute patch
  into one, keeping default behavior.
  Overhaul the patch to support different modes altogether
* Fixed bugs in module removal in pwm1_enable=1
* Moved internal PWM state removal to the end to keep the patch smaller

Best regards,
Alexander

Alexander Stein (6):
  hwmon: pwm-fan: Refactor fan power on/off
  hwmon: pwm-fan: Simplify enable/disable check
  hwmon: pwm-fan: Add dedicated power switch function
  hwmon: pwm-fan: split __set_pwm into locked/unlocked functions
  hwmon: pwm-fan: Switch regulator dynamically
  hwmon: pwm-fan: Remove internal duplicated pwm_state

 Documentation/hwmon/pwm-fan.rst |  12 ++
 drivers/hwmon/pwm-fan.c         | 330 +++++++++++++++++++++++---------
 2 files changed, 255 insertions(+), 87 deletions(-)

-- 
2.25.1

[PATCH v4 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..831878daffe6 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 = 0;
+	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 = &ctx->pwm_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;
+
 	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 v4 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 831878daffe6..96b10d422828 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 = &ctx->pwm_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 v4 3/6] hwmon: pwm-fan: Add dedicated power switch function

[Alexander Stein]

This handles enabling/disabling the regulator in a single function, while
keeping the enables/disabled balanced. This is a preparation when
regulator is switched from different code paths.

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

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 96b10d422828..04af24268963 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 pwm_state pwm_state;
 	struct regulator *reg_en;
+	bool regulator_enabled;
 	bool enabled;
 
 	int tach_count;
@@ -85,6 +86,29 @@ static void sample_timer(struct timer_list *t)
 	mod_timer(&ctx->rpm_timer, jiffies + HZ);
 }
 
+static int pwm_fan_switch_power(struct pwm_fan_ctx *ctx, bool on)
+{
+	int ret = 0;
+
+	if (!ctx->reg_en)
+		return ret;
+
+	if (ctx->regulator_enabled && on) {
+		ret = 0;
+	} else if (!ctx->regulator_enabled && on) {
+		ret = regulator_enable(ctx->reg_en);
+		if (ret == 0)
+			ctx->regulator_enabled = true;
+	} else if (ctx->regulator_enabled && !on) {
+		ret = regulator_disable(ctx->reg_en);
+		if (ret == 0)
+			ctx->regulator_enabled = false;
+	} else if (!ctx->regulator_enabled && !on) {
+		ret = 0;
+	}
+	return ret;
+}
+
 static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
 {
 	struct pwm_state *state = &ctx->pwm_state;
@@ -316,7 +340,9 @@ static int pwm_fan_of_get_cooling_data(struct device *dev,
 
 static void pwm_fan_regulator_disable(void *data)
 {
-	regulator_disable(data);
+	struct pwm_fan_ctx *ctx = data;
+
+	pwm_fan_switch_power(ctx, false);
 }
 
 static void pwm_fan_pwm_disable(void *__ctx)
@@ -360,13 +386,13 @@ static int pwm_fan_probe(struct platform_device *pdev)
 
 		ctx->reg_en = NULL;
 	} else {
-		ret = regulator_enable(ctx->reg_en);
+		ret = pwm_fan_switch_power(ctx, true);
 		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);
+					       ctx);
 		if (ret)
 			return ret;
 	}
@@ -512,12 +538,10 @@ static int pwm_fan_disable(struct device *dev)
 			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;
-		}
+	ret = pwm_fan_switch_power(ctx, false);
+	if (ret) {
+		dev_err(dev, "Failed to disable fan supply: %d\n", ret);
+		return ret;
 	}
 
 	return 0;
@@ -539,12 +563,10 @@ 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;
-		}
+	ret = pwm_fan_switch_power(ctx, true);
+	if (ret) {
+		dev_err(dev, "Failed to enable fan supply: %d\n", ret);
+		return ret;
 	}
 
 	if (ctx->pwm_value == 0)
-- 
2.25.1

[PATCH v4 4/6] hwmon: pwm-fan: split __set_pwm into locked/unlocked functions

[Alexander Stein]

Regular calls to set_pwm don't hold the mutex, but the upcoming
update_enable support needs to call set_pwm with the mutex being held.
So provide the previous behavior in set_pwm (handling the lock), while
adding __set_pwm which assumes the lock being held.

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

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 04af24268963..fcc1b7b55a65 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -152,14 +152,12 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
 	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);
 		if (ret)
-			goto exit_set_pwm_err;
+			return ret;
 		ret = pwm_fan_power_on(ctx);
 	} else {
 		ret = pwm_fan_power_off(ctx);
@@ -167,8 +165,17 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
 	if (!ret)
 		ctx->pwm_value = pwm;
 
-exit_set_pwm_err:
+	return ret;
+}
+
+static int set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
+{
+	int ret;
+
+	mutex_lock(&ctx->lock);
+	ret = __set_pwm(ctx, pwm);
 	mutex_unlock(&ctx->lock);
+
 	return ret;
 }
 
@@ -192,7 +199,7 @@ static int pwm_fan_write(struct device *dev, enum hwmon_sensor_types type,
 	if (val < 0 || val > MAX_PWM)
 		return -EINVAL;
 
-	ret = __set_pwm(ctx, val);
+	ret = set_pwm(ctx, val);
 	if (ret)
 		return ret;
 
@@ -280,7 +287,7 @@ pwm_fan_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
 	if (state == ctx->pwm_fan_state)
 		return 0;
 
-	ret = __set_pwm(ctx, ctx->pwm_fan_cooling_levels[state]);
+	ret = set_pwm(ctx, ctx->pwm_fan_cooling_levels[state]);
 	if (ret) {
 		dev_err(&cdev->device, "Cannot set pwm!\n");
 		return ret;
@@ -400,7 +407,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
 	pwm_init_state(ctx->pwm, &ctx->pwm_state);
 
 	/*
-	 * __set_pwm assumes that MAX_PWM * (period - 1) fits into an unsigned
+	 * 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.
 	 */
@@ -410,7 +417,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
 	}
 
 	/* Set duty cycle to maximum allowed and enable PWM output */
-	ret = __set_pwm(ctx, MAX_PWM);
+	ret = set_pwm(ctx, MAX_PWM);
 	if (ret) {
 		dev_err(dev, "Failed to configure PWM: %d\n", ret);
 		return ret;
-- 
2.25.1

[PATCH v4 5/6] hwmon: pwm-fan: Switch regulator dynamically

[Alexander Stein]

This adds the enable attribute which is used to select if zero PWM duty
means to switch off regulator and PWM or to keep them enabled but
at inactive PWM output level.
Depending on the select enable mode, turn off the regulator and PWM if
the PWM duty is zero, or keep them enabled.
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>
---
 Documentation/hwmon/pwm-fan.rst |  12 ++
 drivers/hwmon/pwm-fan.c         | 213 +++++++++++++++++++++-----------
 2 files changed, 154 insertions(+), 71 deletions(-)

diff --git a/Documentation/hwmon/pwm-fan.rst b/Documentation/hwmon/pwm-fan.rst
index 82fe96742fee..f77998b204ef 100644
--- a/Documentation/hwmon/pwm-fan.rst
+++ b/Documentation/hwmon/pwm-fan.rst
@@ -18,3 +18,15 @@ 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 -> 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
+pwm1		rw	relative speed (0-255), 255=max. speed.
+=============== ======= =======================================================
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index fcc1b7b55a65..e5d4b3b1cc49 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -28,6 +28,13 @@ struct pwm_fan_tach {
 	u8 pulses_per_revolution;
 };
 
+enum pwm_fan_enable_mode {
+	pwm_off_reg_off,
+	pwm_disable_reg_enable,
+	pwm_enable_reg_enable,
+	pwm_disable_reg_disable,
+};
+
 struct pwm_fan_ctx {
 	struct device *dev;
 
@@ -35,6 +42,7 @@ struct pwm_fan_ctx {
 	struct pwm_device *pwm;
 	struct pwm_state pwm_state;
 	struct regulator *reg_en;
+	enum pwm_fan_enable_mode enable_mode;
 	bool regulator_enabled;
 	bool enabled;
 
@@ -86,6 +94,29 @@ static void sample_timer(struct timer_list *t)
 	mod_timer(&ctx->rpm_timer, jiffies + HZ);
 }
 
+static void pwm_fan_enable_mode_2_state(int enable_mode,
+					struct pwm_state *state,
+					bool *enable_regulator)
+{
+	switch (enable_mode) {
+	case pwm_disable_reg_enable:
+		/* disable pwm, keep regulator enabled */
+		state->enabled = false;
+		*enable_regulator = true;
+		break;
+	case pwm_enable_reg_enable:
+		/* keep pwm and regulator enabled */
+		state->enabled = true;
+		*enable_regulator = true;
+		break;
+	case pwm_off_reg_off:
+	case pwm_disable_reg_disable:
+		/* disable pwm and regulator */
+		state->enabled = false;
+		*enable_regulator = false;
+	}
+}
+
 static int pwm_fan_switch_power(struct pwm_fan_ctx *ctx, bool on)
 {
 	int ret = 0;
@@ -117,30 +148,46 @@ static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
 	if (ctx->enabled)
 		return 0;
 
+	ret = pwm_fan_switch_power(ctx, true);
+	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:
+	pwm_fan_switch_power(ctx, false);
 	return ret;
 }
 
 static int pwm_fan_power_off(struct pwm_fan_ctx *ctx)
 {
 	struct pwm_state *state = &ctx->pwm_state;
+	bool enable_regulator = false;
 
 	if (!ctx->enabled)
 		return 0;
 
+	pwm_fan_enable_mode_2_state(ctx->enable_mode,
+				    state,
+				    &enable_regulator);
+
 	state->enabled = false;
 	state->duty_cycle = 0;
 	pwm_apply_state(ctx->pwm, state);
 
+	pwm_fan_switch_power(ctx, enable_regulator);
+
 	ctx->enabled = false;
 
 	return 0;
@@ -153,6 +200,10 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
 	int ret = 0;
 
 	if (pwm > 0) {
+		if (ctx->enable_mode == pwm_off_reg_off)
+			/* pwm-fan hard disabled */
+			return 0;
+
 		period = state->period;
 		state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
 		ret = pwm_apply_state(ctx->pwm, state);
@@ -190,20 +241,76 @@ 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)
+{
+	int ret = 0;
+	int old_val;
+
+	mutex_lock(&ctx->lock);
+
+	if (ctx->enable_mode == val)
+		goto out;
+
+	old_val = ctx->enable_mode;
+	ctx->enable_mode = val;
+
+	if (val == 0) {
+		/* Disable pwm-fan unconditionally */
+		ret = __set_pwm(ctx, 0);
+		if (ret)
+			ctx->enable_mode = old_val;
+		pwm_fan_update_state(ctx, 0);
+	} else {
+		/*
+		 * Change PWM and/or regulator state if currently disabled
+		 * Nothing to do if currently enabled
+		 */
+		if (!ctx->enabled) {
+			struct pwm_state *state = &ctx->pwm_state;
+			bool enable_regulator = false;
+
+			state->duty_cycle = 0;
+			pwm_fan_enable_mode_2_state(val,
+						    state,
+						    &enable_regulator);
+
+			pwm_apply_state(ctx->pwm, state);
+			pwm_fan_switch_power(ctx, enable_regulator);
+			pwm_fan_update_state(ctx, 0);
+		}
+	}
+out:
+	mutex_unlock(&ctx->lock);
+
+	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:
+		if (val < 0 || val > 3)
+			ret = -EINVAL;
+		else
+			ret = pwm_fan_update_enable(ctx, val);
 
-	ret = set_pwm(ctx, val);
-	if (ret)
 		return ret;
+	default:
+		return -EOPNOTSUPP;
+	}
 
-	pwm_fan_update_state(ctx, val);
 	return 0;
 }
 
@@ -214,9 +321,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->enable_mode;
+			return 0;
+		}
+		return -EOPNOTSUPP;
 	case hwmon_fan:
 		*val = ctx->tachs[channel].rpm;
 		return 0;
@@ -345,20 +458,14 @@ static int pwm_fan_of_get_cooling_data(struct device *dev,
 	return 0;
 }
 
-static void pwm_fan_regulator_disable(void *data)
-{
-	struct pwm_fan_ctx *ctx = data;
-
-	pwm_fan_switch_power(ctx, false);
-}
-
-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);
+	/* Switch off everything */
+	ctx->enable_mode = pwm_disable_reg_disable;
+	pwm_fan_power_off(ctx);
 }
 
 static int pwm_fan_probe(struct platform_device *pdev)
@@ -392,16 +499,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
 			return PTR_ERR(ctx->reg_en);
 
 		ctx->reg_en = NULL;
-	} else {
-		ret = pwm_fan_switch_power(ctx, true);
-		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);
-		if (ret)
-			return ret;
 	}
 
 	pwm_init_state(ctx->pwm, &ctx->pwm_state);
@@ -416,14 +513,19 @@ static int pwm_fan_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	/* Set duty cycle to maximum allowed and enable PWM output */
+	ctx->enable_mode = pwm_disable_reg_enable;
+
+	/*
+	 * 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;
 
@@ -455,7 +557,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];
@@ -529,57 +631,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;
-	}
-
-	ret = pwm_fan_switch_power(ctx, false);
-	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;
-
-	ret = pwm_fan_switch_power(ctx, true);
-	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 v4 6/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 | 49 +++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index e5d4b3b1cc49..e0ce81cdf5e0 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -40,7 +40,6 @@ struct pwm_fan_ctx {
 
 	struct mutex lock;
 	struct pwm_device *pwm;
-	struct pwm_state pwm_state;
 	struct regulator *reg_en;
 	enum pwm_fan_enable_mode enable_mode;
 	bool regulator_enabled;
@@ -142,7 +141,7 @@ static int pwm_fan_switch_power(struct pwm_fan_ctx *ctx, bool on)
 
 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)
@@ -154,8 +153,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;
@@ -172,19 +172,20 @@ static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
 
 static int pwm_fan_power_off(struct pwm_fan_ctx *ctx)
 {
-	struct pwm_state *state = &ctx->pwm_state;
 	bool enable_regulator = false;
+	struct pwm_state state;
 
 	if (!ctx->enabled)
 		return 0;
 
 	pwm_fan_enable_mode_2_state(ctx->enable_mode,
-				    state,
+				    &state,
 				    &enable_regulator);
 
-	state->enabled = false;
-	state->duty_cycle = 0;
-	pwm_apply_state(ctx->pwm, state);
+	pwm_get_state(ctx->pwm, &state);
+	state.enabled = false;
+	state.duty_cycle = 0;
+	pwm_apply_state(ctx->pwm, &state);
 
 	pwm_fan_switch_power(ctx, enable_regulator);
 
@@ -195,7 +196,7 @@ 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;
 
@@ -204,9 +205,10 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
 			/* pwm-fan hard disabled */
 			return 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)
 			return ret;
 		ret = pwm_fan_power_on(ctx);
@@ -266,15 +268,16 @@ static int pwm_fan_update_enable(struct pwm_fan_ctx *ctx, long val)
 		 * Nothing to do if currently enabled
 		 */
 		if (!ctx->enabled) {
-			struct pwm_state *state = &ctx->pwm_state;
 			bool enable_regulator = false;
+			struct pwm_state state;
 
-			state->duty_cycle = 0;
+			pwm_get_state(ctx->pwm, &state);
+			state.duty_cycle = 0;
 			pwm_fan_enable_mode_2_state(val,
-						    state,
+						    &state,
 						    &enable_regulator);
 
-			pwm_apply_state(ctx->pwm, state);
+			pwm_apply_state(ctx->pwm, &state);
 			pwm_fan_switch_power(ctx, enable_regulator);
 			pwm_fan_update_state(ctx, 0);
 		}
@@ -473,6 +476,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;
@@ -501,18 +505,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;
+	}
+
 	ctx->enable_mode = pwm_disable_reg_enable;
 
 	/*
-- 
2.25.1

Re: [PATCH v4 6/6] hwmon: pwm-fan: Remove internal duplicated pwm_state

[Uwe Kleine-König]

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

Hello,

On Mon, May 23, 2022 at 01:05:13PM +0200, Alexander Stein wrote:
> 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 | 49 +++++++++++++++++++++++++----------------
>  1 file changed, 30 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index e5d4b3b1cc49..e0ce81cdf5e0 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -40,7 +40,6 @@ struct pwm_fan_ctx {
>  
>  	struct mutex lock;
>  	struct pwm_device *pwm;
> -	struct pwm_state pwm_state;
>  	struct regulator *reg_en;
>  	enum pwm_fan_enable_mode enable_mode;
>  	bool regulator_enabled;
> @@ -142,7 +141,7 @@ static int pwm_fan_switch_power(struct pwm_fan_ctx *ctx, bool on)
>  
>  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)
> @@ -154,8 +153,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;

IMHO this isn't a net win. You trade the overhead of pwm_get_state
against some memory savings. I personally am not a big fan of the
get_state + modify + apply codeflow. The PWM framework does internal
caching of the last applied state, but the details are a bit ugly. (i.e.
pwm_get_state returns the last applied state, unless there was no state
applied before. In that case it returns what .get_state returned during
request time, unless there is no .get_state callback ... not sure if the
device tree stuff somehow goes into that, didn't find it on a quick
glance)

Also there is a (small) danger, that pwm_state contains something that
isn't intended by the driver, e.g. a wrong polarity. So I like the
consumer to fully specify what they intend and not use pwm_get_state().

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 v4 6/6] hwmon: pwm-fan: Remove internal duplicated pwm_state

[Alexander Stein]

Hi Uwe,

Am Montag, 23. Mai 2022, 14:46:14 CEST schrieb Uwe Kleine-König:
> * PGP Signed by an unknown key
> 
> Hello,
> 
> On Mon, May 23, 2022 at 01:05:13PM +0200, Alexander Stein wrote:
> > 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 | 49 +++++++++++++++++++++++++----------------
> >  1 file changed, 30 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> > index e5d4b3b1cc49..e0ce81cdf5e0 100644
> > --- a/drivers/hwmon/pwm-fan.c
> > +++ b/drivers/hwmon/pwm-fan.c
> > @@ -40,7 +40,6 @@ struct pwm_fan_ctx {
> > 
> >  	struct mutex lock;
> >  	struct pwm_device *pwm;
> > 
> > -	struct pwm_state pwm_state;
> > 
> >  	struct regulator *reg_en;
> >  	enum pwm_fan_enable_mode enable_mode;
> >  	bool regulator_enabled;
> > 
> > @@ -142,7 +141,7 @@ static int pwm_fan_switch_power(struct pwm_fan_ctx
> > *ctx, bool on)> 
> >  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)
> > 
> > @@ -154,8 +153,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;
> 
> IMHO this isn't a net win. You trade the overhead of pwm_get_state
> against some memory savings. I personally am not a big fan of the
> get_state + modify + apply codeflow. The PWM framework does internal
> caching of the last applied state, but the details are a bit ugly. (i.e.
> pwm_get_state returns the last applied state, unless there was no state
> applied before. In that case it returns what .get_state returned during
> request time, unless there is no .get_state callback ... not sure if the
> device tree stuff somehow goes into that, didn't find it on a quick
> glance)
> 
> Also there is a (small) danger, that pwm_state contains something that
> isn't intended by the driver, e.g. a wrong polarity. So I like the
> consumer to fully specify what they intend and not use pwm_get_state().

Ah, I see. I have no hard feelings for this patch. I just wondered why the PWM 
state is duplicated. and wanted to get rid of it. If there is a specific 
reason for this, I'm ok with that.

Best regards,
Alexander

Re: [PATCH v4 6/6] hwmon: pwm-fan: Remove internal duplicated pwm_state

[Guenter Roeck]

On 5/23/22 06:55, Alexander Stein wrote:
> Hi Uwe,
> 
> Am Montag, 23. Mai 2022, 14:46:14 CEST schrieb Uwe Kleine-König:
>> * PGP Signed by an unknown key
>>
>> Hello,
>>
>> On Mon, May 23, 2022 at 01:05:13PM +0200, Alexander Stein wrote:
>>> 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 | 49 +++++++++++++++++++++++++----------------
>>>   1 file changed, 30 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
>>> index e5d4b3b1cc49..e0ce81cdf5e0 100644
>>> --- a/drivers/hwmon/pwm-fan.c
>>> +++ b/drivers/hwmon/pwm-fan.c
>>> @@ -40,7 +40,6 @@ struct pwm_fan_ctx {
>>>
>>>   	struct mutex lock;
>>>   	struct pwm_device *pwm;
>>>
>>> -	struct pwm_state pwm_state;
>>>
>>>   	struct regulator *reg_en;
>>>   	enum pwm_fan_enable_mode enable_mode;
>>>   	bool regulator_enabled;
>>>
>>> @@ -142,7 +141,7 @@ static int pwm_fan_switch_power(struct pwm_fan_ctx
>>> *ctx, bool on)>
>>>   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)
>>>
>>> @@ -154,8 +153,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;
>>
>> IMHO this isn't a net win. You trade the overhead of pwm_get_state
>> against some memory savings. I personally am not a big fan of the
>> get_state + modify + apply codeflow. The PWM framework does internal
>> caching of the last applied state, but the details are a bit ugly. (i.e.
>> pwm_get_state returns the last applied state, unless there was no state
>> applied before. In that case it returns what .get_state returned during
>> request time, unless there is no .get_state callback ... not sure if the
>> device tree stuff somehow goes into that, didn't find it on a quick
>> glance)
>>
>> Also there is a (small) danger, that pwm_state contains something that
>> isn't intended by the driver, e.g. a wrong polarity. So I like the
>> consumer to fully specify what they intend and not use pwm_get_state().
> 
> Ah, I see. I have no hard feelings for this patch. I just wondered why the PWM
> state is duplicated. and wanted to get rid of it. If there is a specific
> reason for this, I'm ok with that.
> 

I don't see the value of continuous runtime overhead to save a few bytes of data,
so I don't see a reason to _not_ cache the state locally. This is similar to
caching a clock frequency locally instead of calling the clock subsystem again
and again to read it. Sure, nowadays CPUs are more powerful than they used to be,
but I don't see that as reason or argument for wasting their power.

Guenter

Re: [PATCH v4 6/6] hwmon: pwm-fan: Remove internal duplicated pwm_state

[Alexander Stein]

Hello Guenter, Uwe,

Am Montag, 23. Mai 2022, 16:18:57 CEST schrieb Guenter Roeck:
> On 5/23/22 06:55, Alexander Stein wrote:
> > Hi Uwe,
> > 
> > Am Montag, 23. Mai 2022, 14:46:14 CEST schrieb Uwe Kleine-König:
> >> * PGP Signed by an unknown key
> >> 
> >> Hello,
> >> 
> >> On Mon, May 23, 2022 at 01:05:13PM +0200, Alexander Stein wrote:
> >>> 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 | 49 +++++++++++++++++++++++++----------------
> >>>   1 file changed, 30 insertions(+), 19 deletions(-)
> >>> 
> >>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> >>> index e5d4b3b1cc49..e0ce81cdf5e0 100644
> >>> --- a/drivers/hwmon/pwm-fan.c
> >>> +++ b/drivers/hwmon/pwm-fan.c
> >>> @@ -40,7 +40,6 @@ struct pwm_fan_ctx {
> >>> 
> >>>   	struct mutex lock;
> >>>   	struct pwm_device *pwm;
> >>> 
> >>> -	struct pwm_state pwm_state;
> >>> 
> >>>   	struct regulator *reg_en;
> >>>   	enum pwm_fan_enable_mode enable_mode;
> >>>   	bool regulator_enabled;
> >>> 
> >>> @@ -142,7 +141,7 @@ static int pwm_fan_switch_power(struct pwm_fan_ctx
> >>> *ctx, bool on)>
> >>> 
> >>>   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)
> >>> 
> >>> @@ -154,8 +153,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;
> >> 
> >> IMHO this isn't a net win. You trade the overhead of pwm_get_state
> >> against some memory savings. I personally am not a big fan of the
> >> get_state + modify + apply codeflow. The PWM framework does internal
> >> caching of the last applied state, but the details are a bit ugly. (i.e.
> >> pwm_get_state returns the last applied state, unless there was no state
> >> applied before. In that case it returns what .get_state returned during
> >> request time, unless there is no .get_state callback ... not sure if the
> >> device tree stuff somehow goes into that, didn't find it on a quick
> >> glance)
> >> 
> >> Also there is a (small) danger, that pwm_state contains something that
> >> isn't intended by the driver, e.g. a wrong polarity. So I like the
> >> consumer to fully specify what they intend and not use pwm_get_state().
> > 
> > Ah, I see. I have no hard feelings for this patch. I just wondered why the
> > PWM state is duplicated. and wanted to get rid of it. If there is a
> > specific reason for this, I'm ok with that.
> 
> I don't see the value of continuous runtime overhead to save a few bytes of
> data, so I don't see a reason to _not_ cache the state locally. This is
> similar to caching a clock frequency locally instead of calling the clock
> subsystem again and again to read it. Sure, nowadays CPUs are more powerful
> than they used to be, but I don't see that as reason or argument for
> wasting their power.

Ok, seems reasonable. I'm fully fine with patch 6 being dropped. What about 
the other patches?

Best regards,
Alexander

Re: [PATCH v4 6/6] hwmon: pwm-fan: Remove internal duplicated pwm_state

[Uwe Kleine-König]

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

Hello Alexander,

On Tue, Jun 21, 2022 at 08:41:37AM +0200, Alexander Stein wrote:
> Am Montag, 23. Mai 2022, 16:18:57 CEST schrieb Guenter Roeck:
> > On 5/23/22 06:55, Alexander Stein wrote:
> > > Hi Uwe,
> > > 
> > > Am Montag, 23. Mai 2022, 14:46:14 CEST schrieb Uwe Kleine-König:
> > >> * PGP Signed by an unknown key
> > >> 
> > >> Hello,
> > >> 
> > >> On Mon, May 23, 2022 at 01:05:13PM +0200, Alexander Stein wrote:
> > >>> 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 | 49 +++++++++++++++++++++++++----------------
> > >>>   1 file changed, 30 insertions(+), 19 deletions(-)
> > >>> 
> > >>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> > >>> index e5d4b3b1cc49..e0ce81cdf5e0 100644
> > >>> --- a/drivers/hwmon/pwm-fan.c
> > >>> +++ b/drivers/hwmon/pwm-fan.c
> > >>> @@ -40,7 +40,6 @@ struct pwm_fan_ctx {
> > >>> 
> > >>>   	struct mutex lock;
> > >>>   	struct pwm_device *pwm;
> > >>> 
> > >>> -	struct pwm_state pwm_state;
> > >>> 
> > >>>   	struct regulator *reg_en;
> > >>>   	enum pwm_fan_enable_mode enable_mode;
> > >>>   	bool regulator_enabled;
> > >>> 
> > >>> @@ -142,7 +141,7 @@ static int pwm_fan_switch_power(struct pwm_fan_ctx
> > >>> *ctx, bool on)>
> > >>> 
> > >>>   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)
> > >>> 
> > >>> @@ -154,8 +153,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;
> > >> 
> > >> IMHO this isn't a net win. You trade the overhead of pwm_get_state
> > >> against some memory savings. I personally am not a big fan of the
> > >> get_state + modify + apply codeflow. The PWM framework does internal
> > >> caching of the last applied state, but the details are a bit ugly. (i.e.
> > >> pwm_get_state returns the last applied state, unless there was no state
> > >> applied before. In that case it returns what .get_state returned during
> > >> request time, unless there is no .get_state callback ... not sure if the
> > >> device tree stuff somehow goes into that, didn't find it on a quick
> > >> glance)
> > >> 
> > >> Also there is a (small) danger, that pwm_state contains something that
> > >> isn't intended by the driver, e.g. a wrong polarity. So I like the
> > >> consumer to fully specify what they intend and not use pwm_get_state().
> > > 
> > > Ah, I see. I have no hard feelings for this patch. I just wondered why the
> > > PWM state is duplicated. and wanted to get rid of it. If there is a
> > > specific reason for this, I'm ok with that.
> > 
> > I don't see the value of continuous runtime overhead to save a few bytes of
> > data, so I don't see a reason to _not_ cache the state locally. This is
> > similar to caching a clock frequency locally instead of calling the clock
> > subsystem again and again to read it. Sure, nowadays CPUs are more powerful
> > than they used to be, but I don't see that as reason or argument for
> > wasting their power.
> 
> Ok, seems reasonable. I'm fully fine with patch 6 being dropped. What about 
> the other patches?

+1 for dropping patch #6. Otherwise (with my PWM expert hat on) I have
no further criticism. But I didn't look deep enough into the patches for
an Ack, I guess I'm also missing some hwmon foo to objectively review
further.

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 v4 2/6] hwmon: pwm-fan: Simplify enable/disable check

[Guenter Roeck]

On Mon, May 23, 2022 at 01:05:09PM +0200, Alexander Stein wrote:
> 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 831878daffe6..96b10d422828 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;

There is no reason for this goto. Just return directly.

> +	}
>  
> +	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 = &ctx->pwm_state;
>  
> +	if (!ctx->enabled)
> +		return 0;
> +

ctx->enabled will initially be false. How is it known that pwm is
disabled when the driver is loaded ? At the very least that warrants
an explanation.

>  	state->enabled = false;
>  	state->duty_cycle = 0;
>  	pwm_apply_state(ctx->pwm, state);

This code is a bit inconsistent with pwm_fan_power_on(). Why check for
error there, but not here ?

>  
> +	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");

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

[Guenter Roeck]

On Mon, May 23, 2022 at 01:05:08PM +0200, Alexander Stein wrote:
> 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>

For my reference:

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  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..831878daffe6 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 = 0;
> +	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 = &ctx->pwm_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;
> +
>  	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;

Re: [PATCH v4 3/6] hwmon: pwm-fan: Add dedicated power switch function

[Guenter Roeck]

On Mon, May 23, 2022 at 01:05:10PM +0200, Alexander Stein wrote:
> This handles enabling/disabling the regulator in a single function, while
> keeping the enables/disabled balanced. This is a preparation when
> regulator is switched from different code paths.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
>  drivers/hwmon/pwm-fan.c | 52 +++++++++++++++++++++++++++++------------
>  1 file changed, 37 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index 96b10d422828..04af24268963 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 pwm_state pwm_state;
>  	struct regulator *reg_en;
> +	bool regulator_enabled;
>  	bool enabled;
>  
>  	int tach_count;
> @@ -85,6 +86,29 @@ static void sample_timer(struct timer_list *t)
>  	mod_timer(&ctx->rpm_timer, jiffies + HZ);
>  }
>  
> +static int pwm_fan_switch_power(struct pwm_fan_ctx *ctx, bool on)
> +{
> +	int ret = 0;
> +
> +	if (!ctx->reg_en)
> +		return ret;
> +
> +	if (ctx->regulator_enabled && on) {
> +		ret = 0;

ret is already 0 here.

> +	} else if (!ctx->regulator_enabled && on) {
> +		ret = regulator_enable(ctx->reg_en);
> +		if (ret == 0)
> +			ctx->regulator_enabled = true;
> +	} else if (ctx->regulator_enabled && !on) {
> +		ret = regulator_disable(ctx->reg_en);
> +		if (ret == 0)
> +			ctx->regulator_enabled = false;
> +	} else if (!ctx->regulator_enabled && !on) {
> +		ret = 0;

ret is already 0 here.

> +	}
> +	return ret;
> +}
> +
>  static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
>  {
>  	struct pwm_state *state = &ctx->pwm_state;
> @@ -316,7 +340,9 @@ static int pwm_fan_of_get_cooling_data(struct device *dev,
>  
>  static void pwm_fan_regulator_disable(void *data)
>  {
> -	regulator_disable(data);
> +	struct pwm_fan_ctx *ctx = data;
> +
> +	pwm_fan_switch_power(ctx, false);

You can directly pass 'data' as argument here; there is no need
for the extra variable.

>  }
>  
>  static void pwm_fan_pwm_disable(void *__ctx)
> @@ -360,13 +386,13 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  
>  		ctx->reg_en = NULL;
>  	} else {
> -		ret = regulator_enable(ctx->reg_en);
> +		ret = pwm_fan_switch_power(ctx, true);
>  		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);
> +					       ctx);
>  		if (ret)
>  			return ret;
>  	}
> @@ -512,12 +538,10 @@ static int pwm_fan_disable(struct device *dev)
>  			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;
> -		}
> +	ret = pwm_fan_switch_power(ctx, false);
> +	if (ret) {
> +		dev_err(dev, "Failed to disable fan supply: %d\n", ret);
> +		return ret;
>  	}
>  
>  	return 0;
> @@ -539,12 +563,10 @@ 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;
> -		}
> +	ret = pwm_fan_switch_power(ctx, true);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable fan supply: %d\n", ret);
> +		return ret;
>  	}
>  
>  	if (ctx->pwm_value == 0)

Re: [PATCH v4 4/6] hwmon: pwm-fan: split __set_pwm into locked/unlocked functions

[Guenter Roeck]

On Mon, May 23, 2022 at 01:05:11PM +0200, Alexander Stein wrote:
> Regular calls to set_pwm don't hold the mutex, but the upcoming
> update_enable support needs to call set_pwm with the mutex being held.
> So provide the previous behavior in set_pwm (handling the lock), while
> adding __set_pwm which assumes the lock being held.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>

For my reference:

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/hwmon/pwm-fan.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index 04af24268963..fcc1b7b55a65 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -152,14 +152,12 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
>  	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);
>  		if (ret)
> -			goto exit_set_pwm_err;
> +			return ret;
>  		ret = pwm_fan_power_on(ctx);
>  	} else {
>  		ret = pwm_fan_power_off(ctx);
> @@ -167,8 +165,17 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
>  	if (!ret)
>  		ctx->pwm_value = pwm;
>  
> -exit_set_pwm_err:
> +	return ret;
> +}
> +
> +static int set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
> +{
> +	int ret;
> +
> +	mutex_lock(&ctx->lock);
> +	ret = __set_pwm(ctx, pwm);
>  	mutex_unlock(&ctx->lock);
> +
>  	return ret;
>  }
>  
> @@ -192,7 +199,7 @@ static int pwm_fan_write(struct device *dev, enum hwmon_sensor_types type,
>  	if (val < 0 || val > MAX_PWM)
>  		return -EINVAL;
>  
> -	ret = __set_pwm(ctx, val);
> +	ret = set_pwm(ctx, val);
>  	if (ret)
>  		return ret;
>  
> @@ -280,7 +287,7 @@ pwm_fan_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
>  	if (state == ctx->pwm_fan_state)
>  		return 0;
>  
> -	ret = __set_pwm(ctx, ctx->pwm_fan_cooling_levels[state]);
> +	ret = set_pwm(ctx, ctx->pwm_fan_cooling_levels[state]);
>  	if (ret) {
>  		dev_err(&cdev->device, "Cannot set pwm!\n");
>  		return ret;
> @@ -400,7 +407,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  	pwm_init_state(ctx->pwm, &ctx->pwm_state);
>  
>  	/*
> -	 * __set_pwm assumes that MAX_PWM * (period - 1) fits into an unsigned
> +	 * 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.
>  	 */
> @@ -410,7 +417,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Set duty cycle to maximum allowed and enable PWM output */
> -	ret = __set_pwm(ctx, MAX_PWM);
> +	ret = set_pwm(ctx, MAX_PWM);
>  	if (ret) {
>  		dev_err(dev, "Failed to configure PWM: %d\n", ret);
>  		return ret;

Re: [PATCH v4 5/6] hwmon: pwm-fan: Switch regulator dynamically

[Guenter Roeck]

On Mon, May 23, 2022 at 01:05:12PM +0200, Alexander Stein wrote:
> This adds the enable attribute which is used to select if zero PWM duty
> means to switch off regulator and PWM or to keep them enabled but
> at inactive PWM output level.
> Depending on the select enable mode, turn off the regulator and PWM if
> the PWM duty is zero, or keep them enabled.
> 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>

This patch is too complex to review as diff. Please resend the series with
the changes requested so I can apply and review the result.

Thanks,
Guenter

> ---
>  Documentation/hwmon/pwm-fan.rst |  12 ++
>  drivers/hwmon/pwm-fan.c         | 213 +++++++++++++++++++++-----------
>  2 files changed, 154 insertions(+), 71 deletions(-)
> 
> diff --git a/Documentation/hwmon/pwm-fan.rst b/Documentation/hwmon/pwm-fan.rst
> index 82fe96742fee..f77998b204ef 100644
> --- a/Documentation/hwmon/pwm-fan.rst
> +++ b/Documentation/hwmon/pwm-fan.rst
> @@ -18,3 +18,15 @@ 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 -> 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
> +pwm1		rw	relative speed (0-255), 255=max. speed.
> +=============== ======= =======================================================
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index fcc1b7b55a65..e5d4b3b1cc49 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -28,6 +28,13 @@ struct pwm_fan_tach {
>  	u8 pulses_per_revolution;
>  };
>  
> +enum pwm_fan_enable_mode {
> +	pwm_off_reg_off,
> +	pwm_disable_reg_enable,
> +	pwm_enable_reg_enable,
> +	pwm_disable_reg_disable,
> +};
> +
>  struct pwm_fan_ctx {
>  	struct device *dev;
>  
> @@ -35,6 +42,7 @@ struct pwm_fan_ctx {
>  	struct pwm_device *pwm;
>  	struct pwm_state pwm_state;
>  	struct regulator *reg_en;
> +	enum pwm_fan_enable_mode enable_mode;
>  	bool regulator_enabled;
>  	bool enabled;
>  
> @@ -86,6 +94,29 @@ static void sample_timer(struct timer_list *t)
>  	mod_timer(&ctx->rpm_timer, jiffies + HZ);
>  }
>  
> +static void pwm_fan_enable_mode_2_state(int enable_mode,
> +					struct pwm_state *state,
> +					bool *enable_regulator)
> +{
> +	switch (enable_mode) {
> +	case pwm_disable_reg_enable:
> +		/* disable pwm, keep regulator enabled */
> +		state->enabled = false;
> +		*enable_regulator = true;
> +		break;
> +	case pwm_enable_reg_enable:
> +		/* keep pwm and regulator enabled */
> +		state->enabled = true;
> +		*enable_regulator = true;
> +		break;
> +	case pwm_off_reg_off:
> +	case pwm_disable_reg_disable:
> +		/* disable pwm and regulator */
> +		state->enabled = false;
> +		*enable_regulator = false;
> +	}
> +}
> +
>  static int pwm_fan_switch_power(struct pwm_fan_ctx *ctx, bool on)
>  {
>  	int ret = 0;
> @@ -117,30 +148,46 @@ static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
>  	if (ctx->enabled)
>  		return 0;
>  
> +	ret = pwm_fan_switch_power(ctx, true);
> +	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:
> +	pwm_fan_switch_power(ctx, false);
>  	return ret;
>  }
>  
>  static int pwm_fan_power_off(struct pwm_fan_ctx *ctx)
>  {
>  	struct pwm_state *state = &ctx->pwm_state;
> +	bool enable_regulator = false;
>  
>  	if (!ctx->enabled)
>  		return 0;
>  
> +	pwm_fan_enable_mode_2_state(ctx->enable_mode,
> +				    state,
> +				    &enable_regulator);
> +
>  	state->enabled = false;
>  	state->duty_cycle = 0;
>  	pwm_apply_state(ctx->pwm, state);
>  
> +	pwm_fan_switch_power(ctx, enable_regulator);
> +
>  	ctx->enabled = false;
>  
>  	return 0;
> @@ -153,6 +200,10 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
>  	int ret = 0;
>  
>  	if (pwm > 0) {
> +		if (ctx->enable_mode == pwm_off_reg_off)
> +			/* pwm-fan hard disabled */
> +			return 0;
> +
>  		period = state->period;
>  		state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
>  		ret = pwm_apply_state(ctx->pwm, state);
> @@ -190,20 +241,76 @@ 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)
> +{
> +	int ret = 0;
> +	int old_val;
> +
> +	mutex_lock(&ctx->lock);
> +
> +	if (ctx->enable_mode == val)
> +		goto out;
> +
> +	old_val = ctx->enable_mode;
> +	ctx->enable_mode = val;
> +
> +	if (val == 0) {
> +		/* Disable pwm-fan unconditionally */
> +		ret = __set_pwm(ctx, 0);
> +		if (ret)
> +			ctx->enable_mode = old_val;
> +		pwm_fan_update_state(ctx, 0);
> +	} else {
> +		/*
> +		 * Change PWM and/or regulator state if currently disabled
> +		 * Nothing to do if currently enabled
> +		 */
> +		if (!ctx->enabled) {
> +			struct pwm_state *state = &ctx->pwm_state;
> +			bool enable_regulator = false;
> +
> +			state->duty_cycle = 0;
> +			pwm_fan_enable_mode_2_state(val,
> +						    state,
> +						    &enable_regulator);
> +
> +			pwm_apply_state(ctx->pwm, state);
> +			pwm_fan_switch_power(ctx, enable_regulator);
> +			pwm_fan_update_state(ctx, 0);
> +		}
> +	}
> +out:
> +	mutex_unlock(&ctx->lock);
> +
> +	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:
> +		if (val < 0 || val > 3)
> +			ret = -EINVAL;
> +		else
> +			ret = pwm_fan_update_enable(ctx, val);
>  
> -	ret = set_pwm(ctx, val);
> -	if (ret)
>  		return ret;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
>  
> -	pwm_fan_update_state(ctx, val);
>  	return 0;
>  }
>  
> @@ -214,9 +321,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->enable_mode;
> +			return 0;
> +		}
> +		return -EOPNOTSUPP;
>  	case hwmon_fan:
>  		*val = ctx->tachs[channel].rpm;
>  		return 0;
> @@ -345,20 +458,14 @@ static int pwm_fan_of_get_cooling_data(struct device *dev,
>  	return 0;
>  }
>  
> -static void pwm_fan_regulator_disable(void *data)
> -{
> -	struct pwm_fan_ctx *ctx = data;
> -
> -	pwm_fan_switch_power(ctx, false);
> -}
> -
> -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);
> +	/* Switch off everything */
> +	ctx->enable_mode = pwm_disable_reg_disable;
> +	pwm_fan_power_off(ctx);
>  }
>  
>  static int pwm_fan_probe(struct platform_device *pdev)
> @@ -392,16 +499,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  			return PTR_ERR(ctx->reg_en);
>  
>  		ctx->reg_en = NULL;
> -	} else {
> -		ret = pwm_fan_switch_power(ctx, true);
> -		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);
> -		if (ret)
> -			return ret;
>  	}
>  
>  	pwm_init_state(ctx->pwm, &ctx->pwm_state);
> @@ -416,14 +513,19 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	/* Set duty cycle to maximum allowed and enable PWM output */
> +	ctx->enable_mode = pwm_disable_reg_enable;
> +
> +	/*
> +	 * 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;
>  
> @@ -455,7 +557,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];
> @@ -529,57 +631,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;
> -	}
> -
> -	ret = pwm_fan_switch_power(ctx, false);
> -	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;
> -
> -	ret = pwm_fan_switch_power(ctx, true);
> -	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
>  

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

[Alexander Stein]

Hello Guenter,

thanks for your feedback.

Am Dienstag, 30. August 2022, 15:43:38 CEST schrieb Guenter Roeck:
> On Mon, May 23, 2022 at 01:05:09PM +0200, Alexander Stein wrote:
> > 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 831878daffe6..96b10d422828 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;
> 
> There is no reason for this goto. Just return directly.

Sure, will do so.

> > +	}
> > 
> > +	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 = &ctx->pwm_state;
> > 
> > +	if (!ctx->enabled)
> > +		return 0;
> > +
> 
> ctx->enabled will initially be false. How is it known that pwm is
> disabled when the driver is loaded ? At the very least that warrants
> an explanation.

I'm not sure what you are concerned about. The PWM is enabled in probe 
unconditionally, calling __set_pwm(ctx, MAX_PWM).

> >  	state->enabled = false;
> >  	state->duty_cycle = 0;
> >  	pwm_apply_state(ctx->pwm, state);
> 
> This code is a bit inconsistent with pwm_fan_power_on(). Why check for
> error there, but not here ?

You are right, make sense to check in both functions.

Thanks and best regards
Alexander

> > +	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");

Re: Re: [PATCH v4 3/6] hwmon: pwm-fan: Add dedicated power switch function

[Alexander Stein]

Hello Guenter,

thanks for your review.

Am Dienstag, 30. August 2022, 15:50:13 CEST schrieb Guenter Roeck:
> On Mon, May 23, 2022 at 01:05:10PM +0200, Alexander Stein wrote:
> > This handles enabling/disabling the regulator in a single function, while
> > keeping the enables/disabled balanced. This is a preparation when
> > regulator is switched from different code paths.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > 
> >  drivers/hwmon/pwm-fan.c | 52 +++++++++++++++++++++++++++++------------
> >  1 file changed, 37 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> > index 96b10d422828..04af24268963 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 pwm_state pwm_state;
> >  	struct regulator *reg_en;
> > 
> > +	bool regulator_enabled;
> > 
> >  	bool enabled;
> >  	
> >  	int tach_count;
> > 
> > @@ -85,6 +86,29 @@ static void sample_timer(struct timer_list *t)
> > 
> >  	mod_timer(&ctx->rpm_timer, jiffies + HZ);
> >  
> >  }
> > 
> > +static int pwm_fan_switch_power(struct pwm_fan_ctx *ctx, bool on)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!ctx->reg_en)
> > +		return ret;
> > +
> > +	if (ctx->regulator_enabled && on) {
> > +		ret = 0;
> 
> ret is already 0 here.
> 
> > +	} else if (!ctx->regulator_enabled && on) {
> > +		ret = regulator_enable(ctx->reg_en);
> > +		if (ret == 0)
> > +			ctx->regulator_enabled = true;
> > +	} else if (ctx->regulator_enabled && !on) {
> > +		ret = regulator_disable(ctx->reg_en);
> > +		if (ret == 0)
> > +			ctx->regulator_enabled = false;
> > +	} else if (!ctx->regulator_enabled && !on) {
> > +		ret = 0;
> 
> ret is already 0 here.

Ok, I'll remove both branches, setting ret to 0 again. I just wanted to keep 
the branches for all possibilities, but no heard feelings here.

> > +	}
> > +	return ret;
> > +}
> > +
> > 
> >  static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
> >  {
> >  
> >  	struct pwm_state *state = &ctx->pwm_state;
> > 
> > @@ -316,7 +340,9 @@ static int pwm_fan_of_get_cooling_data(struct device
> > *dev,> 
> >  static void pwm_fan_regulator_disable(void *data)
> >  {
> > 
> > -	regulator_disable(data);
> > +	struct pwm_fan_ctx *ctx = data;
> > +
> > +	pwm_fan_switch_power(ctx, false);
> 
> You can directly pass 'data' as argument here; there is no need
> for the extra variable.

Nice, thanks. Will do so.

Best regards,
Alexander

> >  }
> >  
> >  static void pwm_fan_pwm_disable(void *__ctx)
> > 
> > @@ -360,13 +386,13 @@ static int pwm_fan_probe(struct platform_device
> > *pdev)> 
> >  		ctx->reg_en = NULL;
> >  	
> >  	} else {
> > 
> > -		ret = regulator_enable(ctx->reg_en);
> > +		ret = pwm_fan_switch_power(ctx, true);
> > 
> >  		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);
> > +					       ctx);
> > 
> >  		if (ret)
> >  		
> >  			return ret;
> >  	
> >  	}
> > 
> > @@ -512,12 +538,10 @@ static int pwm_fan_disable(struct device *dev)
> > 
> >  			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;
> > -		}
> > +	ret = pwm_fan_switch_power(ctx, false);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to disable fan supply: %d\n", ret);
> > +		return ret;
> > 
> >  	}
> >  	
> >  	return 0;
> > 
> > @@ -539,12 +563,10 @@ 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;
> > -		}
> > +	ret = pwm_fan_switch_power(ctx, true);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to enable fan supply: %d\n", ret);
> > +		return ret;
> > 
> >  	}
> >  	
> >  	if (ctx->pwm_value == 0)