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

[Guenter Roeck]

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@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx/
> [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