[PATCH 0/5] gpio-fan: regulator support

[PATCH 0/5] gpio-fan: regulator support

[Alexander Stein]

Hi,

this series adds support for regulator support for the use-case a GPIO
controlled fan also has a switchable power supply.
Patch 1 is the change for the binding, which is based on pwm-fan.
Patch 2-4 is some small cleanup in preparation for regulator support.

Note: Patch 4 can also be suqash into patch 3

Patch 5 is the regulator support by adding RPM support which enabled/disables
the regulator accordingly.

Alexander Stein (5):
  dt-bindings: hwmon: gpio-fan: Add optional regulator support
  hwmon: (gpio-fan) Add missing mutex locks
  hwmon: (gpio-fan) Ensure lock is held during set_fan_speed()
  hwmon: (gpio-fan) Remove unnecessary comment
  hwmon: (gpio-fan) Add regulator support

 .../devicetree/bindings/hwmon/gpio-fan.yaml   |   3 +
 drivers/hwmon/gpio-fan.c                      | 106 ++++++++++++++++--
 2 files changed, 102 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH 1/5] dt-bindings: hwmon: gpio-fan: Add optional regulator support

[Alexander Stein]

This adds an optional regulator support (e.g. switchable supply) to the
GPIO fan binding.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 Documentation/devicetree/bindings/hwmon/gpio-fan.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/gpio-fan.yaml b/Documentation/devicetree/bindings/hwmon/gpio-fan.yaml
index 7f30cfc873506..d5386990ab153 100644
--- a/Documentation/devicetree/bindings/hwmon/gpio-fan.yaml
+++ b/Documentation/devicetree/bindings/hwmon/gpio-fan.yaml
@@ -23,6 +23,9 @@ properties:
   alarm-gpios:
     maxItems: 1
 
+  fan-supply:
+    description: Phandle to the regulator that provides power to the fan.
+
   gpio-fan,speed-map:
     $ref: /schemas/types.yaml#/definitions/uint32-matrix
     minItems: 2
-- 
2.34.1

[PATCH 2/5] hwmon: (gpio-fan) Add missing mutex locks

[Alexander Stein]

set_fan_speed() is expected to be called with fan_data->lock being locked.
Add locking for proper synchronization.

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

diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
index d92c536be9af7..b779240328d59 100644
--- a/drivers/hwmon/gpio-fan.c
+++ b/drivers/hwmon/gpio-fan.c
@@ -393,7 +393,12 @@ static int gpio_fan_set_cur_state(struct thermal_cooling_device *cdev,
 	if (state >= fan_data->num_speed)
 		return -EINVAL;
 
+	mutex_lock(&fan_data->lock);
+
 	set_fan_speed(fan_data, state);
+
+	mutex_unlock(&fan_data->lock);
+
 	return 0;
 }
 
@@ -489,7 +494,11 @@ MODULE_DEVICE_TABLE(of, of_gpio_fan_match);
 
 static void gpio_fan_stop(void *data)
 {
+	struct gpio_fan_data *fan_data = data;
+
+	mutex_lock(&fan_data->lock);
 	set_fan_speed(data, 0);
+	mutex_unlock(&fan_data->lock);
 }
 
 static int gpio_fan_probe(struct platform_device *pdev)
@@ -562,7 +571,9 @@ static int gpio_fan_suspend(struct device *dev)
 
 	if (fan_data->gpios) {
 		fan_data->resume_speed = fan_data->speed_index;
+		mutex_lock(&fan_data->lock);
 		set_fan_speed(fan_data, 0);
+		mutex_unlock(&fan_data->lock);
 	}
 
 	return 0;
@@ -572,8 +583,11 @@ static int gpio_fan_resume(struct device *dev)
 {
 	struct gpio_fan_data *fan_data = dev_get_drvdata(dev);
 
-	if (fan_data->gpios)
+	if (fan_data->gpios) {
+		mutex_lock(&fan_data->lock);
 		set_fan_speed(fan_data, fan_data->resume_speed);
+		mutex_unlock(&fan_data->lock);
+	}
 
 	return 0;
 }
-- 
2.34.1

[PATCH 3/5] hwmon: (gpio-fan) Ensure lock is held during set_fan_speed()

[Alexander Stein]

Instead of just documenting by comment, ensure locking per code.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/hwmon/gpio-fan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
index b779240328d59..322b161d5ca1f 100644
--- a/drivers/hwmon/gpio-fan.c
+++ b/drivers/hwmon/gpio-fan.c
@@ -124,9 +124,10 @@ static int __get_fan_ctrl(struct gpio_fan_data *fan_data)
 	return ctrl_val;
 }
 
-/* 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)
 {
+	WARN_ON_ONCE(!mutex_is_locked(&fan_data->lock));
+
 	if (fan_data->speed_index == speed_index)
 		return;
 
-- 
2.34.1

[PATCH 4/5] hwmon: (gpio-fan) Remove unnecessary comment

[Alexander Stein]

__set_fan_ctrl() is only called by set_fan_speed(), which ensures
fan_data->lock is held already.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/hwmon/gpio-fan.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
index 322b161d5ca1f..4614d0ad86b3a 100644
--- a/drivers/hwmon/gpio-fan.c
+++ b/drivers/hwmon/gpio-fan.c
@@ -100,7 +100,6 @@ static int fan_alarm_init(struct gpio_fan_data *fan_data)
  * Control GPIOs.
  */
 
-/* Must be called with fan_data->lock held, except during initialization. */
 static void __set_fan_ctrl(struct gpio_fan_data *fan_data, int ctrl_val)
 {
 	int i;
-- 
2.34.1

[PATCH 5/5] hwmon: (gpio-fan) Add regulator support

[Alexander Stein]

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>
---
 drivers/hwmon/gpio-fan.c | 86 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 82 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
index 4614d0ad86b3a..3d4719391ea90 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;
 };
 
 /*
@@ -123,15 +127,40 @@ static int __get_fan_ctrl(struct gpio_fan_data *fan_data)
 	return ctrl_val;
 }
 
-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)
 {
 	WARN_ON_ONCE(!mutex_is_locked(&fan_data->lock));
 
 	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) {
+			dev_err(fan_data->dev,
+				"Failed to runtime_get device: %d\n", ret);
+			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) {
+			dev_err(fan_data->dev,
+				"Failed to runtime_put device: %d\n", ret);
+			return ret;
+		}
+	}
+
 	fan_data->speed_index = speed_index;
+
+	return 0;
 }
 
 static int get_fan_speed_index(struct gpio_fan_data *fan_data)
@@ -499,6 +528,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 +552,16 @@ 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_optional(dev, "fan");
+	if (IS_ERR(fan_data->supply)) {
+		err = PTR_ERR(fan_data->supply);
+		if (err != -ENODEV)
+			return dev_err_probe(dev, err,
+					     "Failed to get fan-supply");
+
+		fan_data->supply = NULL;
+	}
+
 	/* 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 +589,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);
@@ -565,6 +617,28 @@ static void gpio_fan_shutdown(struct platform_device *pdev)
 		set_fan_speed(fan_data, 0);
 }
 
+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);
@@ -592,14 +666,18 @@ static int gpio_fan_resume(struct device *dev)
 	return 0;
 }
 
-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,
 	},
 };
-- 
2.34.1

Re: [PATCH 5/5] hwmon: (gpio-fan) Add regulator support

[Mark Brown]

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

On Mon, Feb 10, 2025 at 03:59:33PM +0100, Alexander Stein wrote:

> +	fan_data->supply = devm_regulator_get_optional(dev, "fan");
> +	if (IS_ERR(fan_data->supply)) {
> +		err = PTR_ERR(fan_data->supply);

Unless fans are expected to operate without power (which does seem
unlikely) the supply should really be mandatory.

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

Re: [PATCH 1/5] dt-bindings: hwmon: gpio-fan: Add optional regulator support

[Krzysztof Kozlowski]

On Mon, Feb 10, 2025 at 03:59:29PM +0100, Alexander Stein wrote:
> This adds an optional regulator support (e.g. switchable supply) to the

Add an...

> GPIO fan binding.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
>  Documentation/devicetree/bindings/hwmon/gpio-fan.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/gpio-fan.yaml b/Documentation/devicetree/bindings/hwmon/gpio-fan.yaml
> index 7f30cfc873506..d5386990ab153 100644
> --- a/Documentation/devicetree/bindings/hwmon/gpio-fan.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/gpio-fan.yaml
> @@ -23,6 +23,9 @@ properties:
>    alarm-gpios:
>      maxItems: 1
>  
> +  fan-supply:
> +    description: Phandle to the regulator that provides power to the fan.

Look how fan-common.yaml writes it. It is really not necessary to say
that phandle is a phandle.

"Power supply for fan."

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 1/5] dt-bindings: hwmon: gpio-fan: Add optional regulator support

[Guenter Roeck]

On Mon, Feb 10, 2025 at 03:59:29PM +0100, Alexander Stein wrote:
> This adds an optional regulator support (e.g. switchable supply) to the
> GPIO fan binding.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Applied, after making the change suggested by Krzysztof.

> ---
>  Documentation/devicetree/bindings/hwmon/gpio-fan.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/gpio-fan.yaml b/Documentation/devicetree/bindings/hwmon/gpio-fan.yaml
> index 7f30cfc873506..d5386990ab153 100644
> --- a/Documentation/devicetree/bindings/hwmon/gpio-fan.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/gpio-fan.yaml
> @@ -23,6 +23,9 @@ properties:
>    alarm-gpios:
>      maxItems: 1
>  
> +  fan-supply:
> +    description: Phandle to the regulator that provides power to the fan.
> +
>    gpio-fan,speed-map:
>      $ref: /schemas/types.yaml#/definitions/uint32-matrix
>      minItems: 2

Re: [PATCH 2/5] hwmon: (gpio-fan) Add missing mutex locks

[Guenter Roeck]

On Mon, Feb 10, 2025 at 03:59:30PM +0100, Alexander Stein wrote:
> set_fan_speed() is expected to be called with fan_data->lock being locked.
> Add locking for proper synchronization.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>

Applied.

Thanks,
Guenter

Re: [PATCH 3/5] hwmon: (gpio-fan) Ensure lock is held during set_fan_speed()

[Guenter Roeck]

On Mon, Feb 10, 2025 at 03:59:31PM +0100, Alexander Stein wrote:
> Instead of just documenting by comment, ensure locking per code.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
>  drivers/hwmon/gpio-fan.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
> index b779240328d59..322b161d5ca1f 100644
> --- a/drivers/hwmon/gpio-fan.c
> +++ b/drivers/hwmon/gpio-fan.c
> @@ -124,9 +124,10 @@ static int __get_fan_ctrl(struct gpio_fan_data *fan_data)
>  	return ctrl_val;
>  }
>  
> -/* 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)
>  {
> +	WARN_ON_ONCE(!mutex_is_locked(&fan_data->lock));
> +

No, this just increases code size for no good reason. The comment is
perfectly fine and sufficient.

Guenter

Re: [PATCH 4/5] hwmon: (gpio-fan) Remove unnecessary comment

[Guenter Roeck]

On Mon, Feb 10, 2025 at 03:59:32PM +0100, Alexander Stein wrote:
> __set_fan_ctrl() is only called by set_fan_speed(), which ensures
> fan_data->lock is held already.
> 

The comment is still valid, and it does not hurt to have it.

Guenter

> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
>  drivers/hwmon/gpio-fan.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
> index 322b161d5ca1f..4614d0ad86b3a 100644
> --- a/drivers/hwmon/gpio-fan.c
> +++ b/drivers/hwmon/gpio-fan.c
> @@ -100,7 +100,6 @@ static int fan_alarm_init(struct gpio_fan_data *fan_data)
>   * Control GPIOs.
>   */
>  
> -/* Must be called with fan_data->lock held, except during initialization. */
>  static void __set_fan_ctrl(struct gpio_fan_data *fan_data, int ctrl_val)
>  {
>  	int i;

Re: [PATCH 5/5] hwmon: (gpio-fan) Add regulator support

[Guenter Roeck]

On 2/10/25 07:26, Mark Brown wrote:
> On Mon, Feb 10, 2025 at 03:59:33PM +0100, Alexander Stein wrote:
> 
>> +	fan_data->supply = devm_regulator_get_optional(dev, "fan");
>> +	if (IS_ERR(fan_data->supply)) {
>> +		err = PTR_ERR(fan_data->supply);
> 
> Unless fans are expected to operate without power (which does seem
> unlikely) the supply should really be mandatory.

Agreed. devm_regulator_get() returns a dummy regulator of none is
configured in devicetree, so this should be fine. Worse,
regulator_get_optional() returns an error pointer if regulator
support is disabled, while devm_regulator_get() returns NULL.

Guenter