diff mbox series

[v3,1/1] hwmon: (gpio-fan) Add regulator support

Message ID 20250324124550.989292-1-alexander.stein@ew.tq-group.com (mailing list archive)
State Changes Requested
Headers show
Series [v3,1/1] hwmon: (gpio-fan) Add regulator support | expand

Commit Message

Alexander Stein March 24, 2025, 12:45 p.m. UTC
FANs might be supplied by a regulator which needs to be enabled as well.
This is implemented using runtime PM. Every time speed_index changes from
0 to non-zero and vise versa RPM is resumed or suspended.
Intitial RPM state is determined by initial value of speed_index.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Patch 1 & 2 from v1 [1] have already been applied, although number 2 [2] is not
yet showing in next-20250305. Patches 3 & 4 (just removing comments) from v1
have been dropped, so only this patch remains.

Changes in v3:
* Remove noisy dev_err calls related to runtime pm
* Properly propagate return codes from set_fan_speed

Changes in v2:
* Make regulator non-optional

[1] https://lore.kernel.org/all/20250210145934.761280-1-alexander.stein@ew.tq-group.com/
[2] https://web.git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git/commit/?h=hwmon-next&id=9fee7d19bab635f89223cc40dfd2c8797fdc4988
---
 drivers/hwmon/gpio-fan.c | 104 +++++++++++++++++++++++++++++++++------
 1 file changed, 90 insertions(+), 14 deletions(-)

Comments

Guenter Roeck March 24, 2025, 2:23 p.m. UTC | #1
On 3/24/25 05:45, Alexander Stein wrote:
> FANs might be supplied by a regulator which needs to be enabled as well.
> This is implemented using runtime PM. Every time speed_index changes from
> 0 to non-zero and vise versa RPM is resumed or suspended.
> Intitial RPM state is determined by initial value of speed_index.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> Patch 1 & 2 from v1 [1] have already been applied, although number 2 [2] is not
> yet showing in next-20250305. Patches 3 & 4 (just removing comments) from v1
> have been dropped, so only this patch remains.
> 
> Changes in v3:
> * Remove noisy dev_err calls related to runtime pm
> * Properly propagate return codes from set_fan_speed
> 
> Changes in v2:
> * Make regulator non-optional
> 
> [1] https://lore.kernel.org/all/20250210145934.761280-1-alexander.stein@ew.tq-group.com/
> [2] https://web.git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git/commit/?h=hwmon-next&id=9fee7d19bab635f89223cc40dfd2c8797fdc4988
> ---
>   drivers/hwmon/gpio-fan.c | 104 +++++++++++++++++++++++++++++++++------
>   1 file changed, 90 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
> index cee3fa146d69a..4c736b7eb5473 100644
> --- a/drivers/hwmon/gpio-fan.c
> +++ b/drivers/hwmon/gpio-fan.c
> @@ -20,6 +20,9 @@
>   #include <linux/gpio/consumer.h>
>   #include <linux/of.h>
>   #include <linux/of_platform.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
>   #include <linux/thermal.h>
>   
>   struct gpio_fan_speed {
> @@ -42,6 +45,7 @@ struct gpio_fan_data {
>   	bool			pwm_enable;
>   	struct gpio_desc	*alarm_gpio;
>   	struct work_struct	alarm_work;
> +	struct regulator	*supply;
>   };
>   
>   /*
> @@ -125,13 +129,32 @@ static int __get_fan_ctrl(struct gpio_fan_data *fan_data)
>   }
>   
>   /* Must be called with fan_data->lock held, except during initialization. */
> -static void set_fan_speed(struct gpio_fan_data *fan_data, int speed_index)
> +static int set_fan_speed(struct gpio_fan_data *fan_data, int speed_index)
>   {
>   	if (fan_data->speed_index == speed_index)
> -		return;
> +		return 0;
> +
> +	if (fan_data->speed_index == 0 && speed_index > 0) {
> +		int ret;
> +
> +		ret = pm_runtime_resume_and_get(fan_data->dev);
> +		if (ret < 0)
> +			return ret;
> +	}
>   
>   	__set_fan_ctrl(fan_data, fan_data->speed[speed_index].ctrl_val);
> +
> +	if (fan_data->speed_index > 0 && speed_index == 0) {
> +		int ret;
> +
> +		ret = pm_runtime_put_sync(fan_data->dev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>   	fan_data->speed_index = speed_index;
> +
> +	return 0;
>   }
>   
>   static int get_fan_speed_index(struct gpio_fan_data *fan_data)
> @@ -189,7 +212,9 @@ static ssize_t pwm1_store(struct device *dev, struct device_attribute *attr,
>   	}
>   
>   	speed_index = DIV_ROUND_UP(pwm * (fan_data->num_speed - 1), 255);
> -	set_fan_speed(fan_data, speed_index);
> +	ret = set_fan_speed(fan_data, speed_index);

That means pre-initializing ret is no longer necessary.

> +	if (!ret)
> +		ret = count;
>   

This is confusing. Please use the same error handling everywhere.
I would suggest
	return ret ? : count;
or
	return ret ? ret : count;

>   exit_unlock:
>   	mutex_unlock(&fan_data->lock);
> @@ -211,6 +236,7 @@ static ssize_t pwm1_enable_store(struct device *dev,
>   {
>   	struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
>   	unsigned long val;
> +	int ret;
>   
>   	if (kstrtoul(buf, 10, &val) || val > 1)
>   		return -EINVAL;
> @@ -224,11 +250,14 @@ static ssize_t pwm1_enable_store(struct device *dev,
>   
>   	/* Disable manual control mode: set fan at full speed. */
>   	if (val == 0)
> -		set_fan_speed(fan_data, fan_data->num_speed - 1);
> +		ret = set_fan_speed(fan_data, fan_data->num_speed - 1);
>   
>   	mutex_unlock(&fan_data->lock);
>   
> -	return count;
> +	if (ret)
> +		return ret;
> +	else
> +		return count;

Static analyzers will rightfully complain that else after return is pointless.
Try
	return ret ? : count;
or at least
	return ret ? ret : count;

Thanks,
Guenter
diff mbox series

Patch

diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
index cee3fa146d69a..4c736b7eb5473 100644
--- a/drivers/hwmon/gpio-fan.c
+++ b/drivers/hwmon/gpio-fan.c
@@ -20,6 +20,9 @@ 
 #include <linux/gpio/consumer.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 #include <linux/thermal.h>
 
 struct gpio_fan_speed {
@@ -42,6 +45,7 @@  struct gpio_fan_data {
 	bool			pwm_enable;
 	struct gpio_desc	*alarm_gpio;
 	struct work_struct	alarm_work;
+	struct regulator	*supply;
 };
 
 /*
@@ -125,13 +129,32 @@  static int __get_fan_ctrl(struct gpio_fan_data *fan_data)
 }
 
 /* Must be called with fan_data->lock held, except during initialization. */
-static void set_fan_speed(struct gpio_fan_data *fan_data, int speed_index)
+static int set_fan_speed(struct gpio_fan_data *fan_data, int speed_index)
 {
 	if (fan_data->speed_index == speed_index)
-		return;
+		return 0;
+
+	if (fan_data->speed_index == 0 && speed_index > 0) {
+		int ret;
+
+		ret = pm_runtime_resume_and_get(fan_data->dev);
+		if (ret < 0)
+			return ret;
+	}
 
 	__set_fan_ctrl(fan_data, fan_data->speed[speed_index].ctrl_val);
+
+	if (fan_data->speed_index > 0 && speed_index == 0) {
+		int ret;
+
+		ret = pm_runtime_put_sync(fan_data->dev);
+		if (ret < 0)
+			return ret;
+	}
+
 	fan_data->speed_index = speed_index;
+
+	return 0;
 }
 
 static int get_fan_speed_index(struct gpio_fan_data *fan_data)
@@ -189,7 +212,9 @@  static ssize_t pwm1_store(struct device *dev, struct device_attribute *attr,
 	}
 
 	speed_index = DIV_ROUND_UP(pwm * (fan_data->num_speed - 1), 255);
-	set_fan_speed(fan_data, speed_index);
+	ret = set_fan_speed(fan_data, speed_index);
+	if (!ret)
+		ret = count;
 
 exit_unlock:
 	mutex_unlock(&fan_data->lock);
@@ -211,6 +236,7 @@  static ssize_t pwm1_enable_store(struct device *dev,
 {
 	struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
 	unsigned long val;
+	int ret;
 
 	if (kstrtoul(buf, 10, &val) || val > 1)
 		return -EINVAL;
@@ -224,11 +250,14 @@  static ssize_t pwm1_enable_store(struct device *dev,
 
 	/* Disable manual control mode: set fan at full speed. */
 	if (val == 0)
-		set_fan_speed(fan_data, fan_data->num_speed - 1);
+		ret = set_fan_speed(fan_data, fan_data->num_speed - 1);
 
 	mutex_unlock(&fan_data->lock);
 
-	return count;
+	if (ret)
+		return ret;
+	else
+		return count;
 }
 
 static ssize_t pwm1_mode_show(struct device *dev,
@@ -279,7 +308,7 @@  static ssize_t set_rpm(struct device *dev, struct device_attribute *attr,
 		goto exit_unlock;
 	}
 
-	set_fan_speed(fan_data, rpm_to_speed_index(fan_data, rpm));
+	ret = set_fan_speed(fan_data, rpm_to_speed_index(fan_data, rpm));
 
 exit_unlock:
 	mutex_unlock(&fan_data->lock);
@@ -386,6 +415,7 @@  static int gpio_fan_set_cur_state(struct thermal_cooling_device *cdev,
 				  unsigned long state)
 {
 	struct gpio_fan_data *fan_data = cdev->devdata;
+	int ret;
 
 	if (!fan_data)
 		return -EINVAL;
@@ -395,11 +425,11 @@  static int gpio_fan_set_cur_state(struct thermal_cooling_device *cdev,
 
 	mutex_lock(&fan_data->lock);
 
-	set_fan_speed(fan_data, state);
+	ret = set_fan_speed(fan_data, state);
 
 	mutex_unlock(&fan_data->lock);
 
-	return 0;
+	return ret;
 }
 
 static const struct thermal_cooling_device_ops gpio_fan_cool_ops = {
@@ -499,6 +529,8 @@  static void gpio_fan_stop(void *data)
 	mutex_lock(&fan_data->lock);
 	set_fan_speed(data, 0);
 	mutex_unlock(&fan_data->lock);
+
+	pm_runtime_disable(fan_data->dev);
 }
 
 static int gpio_fan_probe(struct platform_device *pdev)
@@ -521,6 +553,11 @@  static int gpio_fan_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, fan_data);
 	mutex_init(&fan_data->lock);
 
+	fan_data->supply = devm_regulator_get(dev, "fan");
+	if (IS_ERR(fan_data->supply))
+		return dev_err_probe(dev, PTR_ERR(fan_data->supply),
+				     "Failed to get fan-supply");
+
 	/* Configure control GPIOs if available. */
 	if (fan_data->gpios && fan_data->num_gpios > 0) {
 		if (!fan_data->speed || fan_data->num_speed <= 1)
@@ -548,6 +585,17 @@  static int gpio_fan_probe(struct platform_device *pdev)
 			return err;
 	}
 
+	pm_runtime_set_suspended(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	/* If current GPIO state is active, mark RPM as active as well */
+	if (fan_data->speed_index > 0) {
+		int ret;
+
+		ret = pm_runtime_resume_and_get(&pdev->dev);
+		if (ret)
+			return ret;
+	}
+
 	/* Optional cooling device register for Device tree platforms */
 	fan_data->cdev = devm_thermal_of_cooling_device_register(dev, np,
 				"gpio-fan", fan_data, &gpio_fan_cool_ops);
@@ -568,41 +616,69 @@  static void gpio_fan_shutdown(struct platform_device *pdev)
 	}
 }
 
+static int gpio_fan_runtime_suspend(struct device *dev)
+{
+	struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
+	int ret = 0;
+
+	if (fan_data->supply)
+		ret = regulator_disable(fan_data->supply);
+
+	return ret;
+}
+
+static int gpio_fan_runtime_resume(struct device *dev)
+{
+	struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
+	int ret = 0;
+
+	if (fan_data->supply)
+		ret = regulator_enable(fan_data->supply);
+
+	return ret;
+}
+
 static int gpio_fan_suspend(struct device *dev)
 {
 	struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
+	int ret = 0;
 
 	if (fan_data->gpios) {
 		fan_data->resume_speed = fan_data->speed_index;
 		mutex_lock(&fan_data->lock);
-		set_fan_speed(fan_data, 0);
+		ret = set_fan_speed(fan_data, 0);
 		mutex_unlock(&fan_data->lock);
 	}
 
-	return 0;
+	return ret;
 }
 
 static int gpio_fan_resume(struct device *dev)
 {
 	struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
+	int ret = 0;
 
 	if (fan_data->gpios) {
 		mutex_lock(&fan_data->lock);
-		set_fan_speed(fan_data, fan_data->resume_speed);
+		ret = set_fan_speed(fan_data, fan_data->resume_speed);
 		mutex_unlock(&fan_data->lock);
 	}
 
-	return 0;
+	return ret;
 }
 
-static DEFINE_SIMPLE_DEV_PM_OPS(gpio_fan_pm, gpio_fan_suspend, gpio_fan_resume);
+static const struct dev_pm_ops gpio_fan_pm = {
+	RUNTIME_PM_OPS(gpio_fan_runtime_suspend,
+		       gpio_fan_runtime_resume, NULL)
+	SYSTEM_SLEEP_PM_OPS(gpio_fan_suspend, gpio_fan_resume)
+};
 
 static struct platform_driver gpio_fan_driver = {
 	.probe		= gpio_fan_probe,
 	.shutdown	= gpio_fan_shutdown,
 	.driver	= {
 		.name	= "gpio-fan",
-		.pm	= pm_sleep_ptr(&gpio_fan_pm),
+		.pm	= pm_ptr(&gpio_fan_pm),
 		.of_match_table = of_gpio_fan_match,
 	},
 };