| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7b7fe0a5-22bf-45bd-8ad9-ada80714228c@roeck-us.net>
Date: Wed, 5 Mar 2025 06:24:55 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Alexander Stein <alexander.stein@...tq-group.com>,
Jean Delvare <jdelvare@...e.com>
Cc: linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] hwmon: (gpio-fan) Add regulator support
On 3/5/25 02:53, 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@...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 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 | 81 ++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 77 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
> index cee3fa146d69a..db918d6858325 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,38 @@ 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)
What is the point of changing this function to return an error,
but not checking for that error in the calling code ?
> {
> 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);
Please drop that noise. If you think you need the messages,
make it debug messages.
> + 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,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 +584,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,6 +615,28 @@ 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);
> @@ -595,14 +664,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,
> },
> };
Powered by blists - more mailing lists