[PATCH 1/2] dt-bindings: hwmon: pwm-fan: Document start from dead stop properties

[PATCH 1/2] dt-bindings: hwmon: pwm-fan: Document start from dead stop properties

[Marek Vasut]

Delta AFC0612DB-F00 fan has to be set to at least 30% PWM duty cycle
to spin up from a dead stop, and can be afterward throttled down to
lower PWM duty cycle. Introduce support for operating such fans which
need to start at higher PWM duty cycle first and can slow down next.

Document two new DT properties, "fan-dead-stop-start-percent" and
"fan-dead-stop-start-usec". The former describes the minimum percent
of fan RPM at which it will surely spin up from dead stop. This value
can be found in the fan datasheet and can be converted to PWM duty
cycle easily. The "fan-dead-stop-start-usec" describes the minimum
time in microseconds for which the fan has to be set to dead stop
start RPM for the fan to surely spin up.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Jean Delvare <jdelvare@suse.com>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-hwmon@vger.kernel.org
---
 Documentation/devicetree/bindings/hwmon/pwm-fan.yaml | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
index 4e5abf7580cc6..f1042471b5176 100644
--- a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
+++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
@@ -31,6 +31,17 @@ properties:
       it must be self resetting edge interrupts.
     maxItems: 1
 
+  fan-dead-stop-start-percent:
+    description:
+      Minimum fan RPM in percent to start from dead stop.
+    minimum: 0
+    maximum: 100
+
+  fan-dead-stop-start-usec:
+    description:
+      Time to wait in microseconds after start from dead stop.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
   pulses-per-revolution:
     description:
       Define the number of pulses per fan revolution for each tachometer
-- 
2.45.2

[PATCH 2/2] hwmon: (pwm-fan) Introduce start from dead stop handling

[Marek Vasut]

Delta AFC0612DB-F00 fan has to be set to at least 30% PWM duty cycle
to spin up from a dead stop, and can be afterward throttled down to
lower PWM duty cycle. Introduce support for operating such fans which
need to start at higher PWM duty cycle first and can slow down next.

Introduce two new DT properties, "fan-dead-stop-start-percent" and
"fan-dead-stop-start-usec". The former describes the minimum percent
of fan RPM at which it will surely spin up from dead stop. This value
can be found in the fan datasheet and can be converted to PWM duty
cycle easily. The "fan-dead-stop-start-usec" describes the minimum
time in microseconds for which the fan has to be set to dead stop
start RPM for the fan to surely spin up.

Adjust the PWM setting code such that if the PWM duty cycle is below
the minimum duty cycle needed by the fan to spin up from dead stop,
then first set the PWM duty cycle to the minimum duty cycle needed
by the fan to spin up from dead stop, then wait the time necessary
for the fan to spin up from dead stop, and finally set the PWM duty
cycle to the one desired by user.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Jean Delvare <jdelvare@suse.com>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-hwmon@vger.kernel.org
---
 drivers/hwmon/pwm-fan.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index c434db4656e7d..264b7ddf8bb40 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -7,6 +7,7 @@
  * Author: Kamil Debski <k.debski@samsung.com>
  */
 
+#include <linux/delay.h>
 #include <linux/hwmon.h>
 #include <linux/interrupt.h>
 #include <linux/mod_devicetable.h>
@@ -60,6 +61,9 @@ struct pwm_fan_ctx {
 
 	struct hwmon_chip_info info;
 	struct hwmon_channel_info fan_channel;
+
+	u64 pwm_duty_cycle_from_dead_stop;
+	u32 pwm_usec_from_dead_stop;
 };
 
 /* This handler assumes self resetting edge triggered interrupt. */
@@ -199,7 +203,9 @@ static int pwm_fan_power_off(struct pwm_fan_ctx *ctx, bool force_disable)
 static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
 {
 	struct pwm_state *state = &ctx->pwm_state;
+	unsigned long final_pwm = pwm;
 	unsigned long period;
+	bool update = false;
 	int ret = 0;
 
 	if (pwm > 0) {
@@ -208,11 +214,22 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
 			return 0;
 
 		period = state->period;
-		state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
+		update = state->duty_cycle < ctx->pwm_duty_cycle_from_dead_stop;
+		if (update)
+			state->duty_cycle = ctx->pwm_duty_cycle_from_dead_stop;
+		else
+			state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
 		ret = pwm_apply_might_sleep(ctx->pwm, state);
 		if (ret)
 			return ret;
 		ret = pwm_fan_power_on(ctx);
+		if (!ret && update) {
+			pwm = final_pwm;
+			state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
+			usleep_range(ctx->pwm_usec_from_dead_stop,
+				     ctx->pwm_usec_from_dead_stop * 2);
+			ret = pwm_apply_might_sleep(ctx->pwm, state);
+		}
 	} else {
 		ret = pwm_fan_power_off(ctx, false);
 	}
@@ -480,6 +497,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
 	struct device *hwmon;
 	int ret;
 	const struct hwmon_channel_info **channels;
+	u32 pwm_min_from_dead_stop = 0;
 	u32 *fan_channel_config;
 	int channel_count = 1;	/* We always have a PWM channel. */
 	int i;
@@ -620,6 +638,19 @@ static int pwm_fan_probe(struct platform_device *pdev)
 		channels[1] = &ctx->fan_channel;
 	}
 
+	ret = of_property_read_u32(dev->of_node, "fan-dead-stop-start-percent",
+				   &pwm_min_from_dead_stop);
+	if (!ret && pwm_min_from_dead_stop) {
+		ctx->pwm_duty_cycle_from_dead_stop =
+			DIV_ROUND_UP(pwm_min_from_dead_stop *
+				     (ctx->pwm_state.period - 1),
+				     100);
+	}
+	ret = of_property_read_u32(dev->of_node, "fan-dead-stop-start-usec",
+				   &ctx->pwm_usec_from_dead_stop);
+	if (ret)
+		ctx->pwm_usec_from_dead_stop = 250000;
+
 	ctx->info.ops = &pwm_fan_hwmon_ops;
 	ctx->info.info = channels;
 
-- 
2.45.2

Re: [PATCH 2/2] hwmon: (pwm-fan) Introduce start from dead stop handling

[Guenter Roeck]

On 11/5/24 05:52, Marek Vasut wrote:
> Delta AFC0612DB-F00 fan has to be set to at least 30% PWM duty cycle
> to spin up from a dead stop, and can be afterward throttled down to
> lower PWM duty cycle. Introduce support for operating such fans which
> need to start at higher PWM duty cycle first and can slow down next.
> 
> Introduce two new DT properties, "fan-dead-stop-start-percent" and
> "fan-dead-stop-start-usec". The former describes the minimum percent
> of fan RPM at which it will surely spin up from dead stop. This value
> can be found in the fan datasheet and can be converted to PWM duty
> cycle easily. The "fan-dead-stop-start-usec" describes the minimum
> time in microseconds for which the fan has to be set to dead stop
> start RPM for the fan to surely spin up.
> 
> Adjust the PWM setting code such that if the PWM duty cycle is below
> the minimum duty cycle needed by the fan to spin up from dead stop,
> then first set the PWM duty cycle to the minimum duty cycle needed
> by the fan to spin up from dead stop, then wait the time necessary
> for the fan to spin up from dead stop, and finally set the PWM duty
> cycle to the one desired by user.
> 

As with the other patch, I don't think "dead" adds any value anywhere.

Guenter

> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-hwmon@vger.kernel.org
> ---
>   drivers/hwmon/pwm-fan.c | 33 ++++++++++++++++++++++++++++++++-
>   1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index c434db4656e7d..264b7ddf8bb40 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -7,6 +7,7 @@
>    * Author: Kamil Debski <k.debski@samsung.com>
>    */
>   
> +#include <linux/delay.h>
>   #include <linux/hwmon.h>
>   #include <linux/interrupt.h>
>   #include <linux/mod_devicetable.h>
> @@ -60,6 +61,9 @@ struct pwm_fan_ctx {
>   
>   	struct hwmon_chip_info info;
>   	struct hwmon_channel_info fan_channel;
> +
> +	u64 pwm_duty_cycle_from_dead_stop;
> +	u32 pwm_usec_from_dead_stop;
>   };
>   
>   /* This handler assumes self resetting edge triggered interrupt. */
> @@ -199,7 +203,9 @@ static int pwm_fan_power_off(struct pwm_fan_ctx *ctx, bool force_disable)
>   static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
>   {
>   	struct pwm_state *state = &ctx->pwm_state;
> +	unsigned long final_pwm = pwm;
>   	unsigned long period;
> +	bool update = false;
>   	int ret = 0;
>   
>   	if (pwm > 0) {
> @@ -208,11 +214,22 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
>   			return 0;
>   
>   		period = state->period;
> -		state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
> +		update = state->duty_cycle < ctx->pwm_duty_cycle_from_dead_stop;
> +		if (update)
> +			state->duty_cycle = ctx->pwm_duty_cycle_from_dead_stop;
> +		else
> +			state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
>   		ret = pwm_apply_might_sleep(ctx->pwm, state);
>   		if (ret)
>   			return ret;
>   		ret = pwm_fan_power_on(ctx);
> +		if (!ret && update) {
> +			pwm = final_pwm;
> +			state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
> +			usleep_range(ctx->pwm_usec_from_dead_stop,
> +				     ctx->pwm_usec_from_dead_stop * 2);
> +			ret = pwm_apply_might_sleep(ctx->pwm, state);
> +		}
>   	} else {
>   		ret = pwm_fan_power_off(ctx, false);
>   	}
> @@ -480,6 +497,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
>   	struct device *hwmon;
>   	int ret;
>   	const struct hwmon_channel_info **channels;
> +	u32 pwm_min_from_dead_stop = 0;
>   	u32 *fan_channel_config;
>   	int channel_count = 1;	/* We always have a PWM channel. */
>   	int i;
> @@ -620,6 +638,19 @@ static int pwm_fan_probe(struct platform_device *pdev)
>   		channels[1] = &ctx->fan_channel;
>   	}
>   
> +	ret = of_property_read_u32(dev->of_node, "fan-dead-stop-start-percent",
> +				   &pwm_min_from_dead_stop);
> +	if (!ret && pwm_min_from_dead_stop) {
> +		ctx->pwm_duty_cycle_from_dead_stop =
> +			DIV_ROUND_UP(pwm_min_from_dead_stop *
> +				     (ctx->pwm_state.period - 1),
> +				     100);
> +	}
> +	ret = of_property_read_u32(dev->of_node, "fan-dead-stop-start-usec",
> +				   &ctx->pwm_usec_from_dead_stop);
> +	if (ret)
> +		ctx->pwm_usec_from_dead_stop = 250000;
> +
>   	ctx->info.ops = &pwm_fan_hwmon_ops;
>   	ctx->info.info = channels;
>   

Re: [PATCH 2/2] hwmon: (pwm-fan) Introduce start from dead stop handling

[kernel test robot]

Hi Marek,

kernel test robot noticed the following build errors:

[auto build test ERROR on groeck-staging/hwmon-next]
[also build test ERROR on linus/master v6.12-rc6 next-20241105]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Marek-Vasut/hwmon-pwm-fan-Introduce-start-from-dead-stop-handling/20241105-215454
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20241105135259.101126-2-marex%40denx.de
patch subject: [PATCH 2/2] hwmon: (pwm-fan) Introduce start from dead stop handling
config: arm-defconfig (https://download.01.org/0day-ci/archive/20241106/202411061031.GHHHxm19-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241106/202411061031.GHHHxm19-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411061031.GHHHxm19-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "__aeabi_uldivmod" [drivers/hwmon/pwm-fan.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/2] hwmon: (pwm-fan) Introduce start from dead stop handling

[kernel test robot]

Hi Marek,

kernel test robot noticed the following build errors:

[auto build test ERROR on groeck-staging/hwmon-next]
[also build test ERROR on linus/master v6.12-rc6 next-20241106]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Marek-Vasut/hwmon-pwm-fan-Introduce-start-from-dead-stop-handling/20241105-215454
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20241105135259.101126-2-marex%40denx.de
patch subject: [PATCH 2/2] hwmon: (pwm-fan) Introduce start from dead stop handling
config: i386-randconfig-016-20241106 (https://download.01.org/0day-ci/archive/20241107/202411070251.mEqY5ErJ-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241107/202411070251.mEqY5ErJ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411070251.mEqY5ErJ-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "__udivdi3" [drivers/hwmon/pwm-fan.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] dt-bindings: hwmon: pwm-fan: Document start from dead stop properties

[Guenter Roeck]

On 11/5/24 05:52, Marek Vasut wrote:
> Delta AFC0612DB-F00 fan has to be set to at least 30% PWM duty cycle
> to spin up from a dead stop, and can be afterward throttled down to
> lower PWM duty cycle. Introduce support for operating such fans which

Doesn't this imply that a minimum pwm value is needed as well ?
Super-IO chips such as the NCT67xx series typically have two separate
registers, one for the pwm start value and one for the minimum pwm value.

> need to start at higher PWM duty cycle first and can slow down next.
> 
> Document two new DT properties, "fan-dead-stop-start-percent" and
> "fan-dead-stop-start-usec". The former describes the minimum percent
> of fan RPM at which it will surely spin up from dead stop. This value
> can be found in the fan datasheet and can be converted to PWM duty
> cycle easily. The "fan-dead-stop-start-usec" describes the minimum
> time in microseconds for which the fan has to be set to dead stop
> start RPM for the fan to surely spin up.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-hwmon@vger.kernel.org
> ---
>   Documentation/devicetree/bindings/hwmon/pwm-fan.yaml | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
> index 4e5abf7580cc6..f1042471b5176 100644
> --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
> @@ -31,6 +31,17 @@ properties:
>         it must be self resetting edge interrupts.
>       maxItems: 1
>   
> +  fan-dead-stop-start-percent:

Personally I don't think that "dead-stop" in the property name adds any value.
On the contrary, I think it leads to confusion. I head to read the description
to understand.


> +    description:
> +      Minimum fan RPM in percent to start from dead stop.

"to start when stopped" should be sufficient.

> +    minimum: 0
> +    maximum: 100
> +
> +  fan-dead-stop-start-usec:
> +    description:
> +      Time to wait in microseconds after start from dead stop.

Same as above for both name and description.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
>     pulses-per-revolution:
>       description:
>         Define the number of pulses per fan revolution for each tachometer

Re: [PATCH 1/2] dt-bindings: hwmon: pwm-fan: Document start from dead stop properties

[Marek Vasut]

On 11/5/24 3:11 PM, Guenter Roeck wrote:
> On 11/5/24 05:52, Marek Vasut wrote:
>> Delta AFC0612DB-F00 fan has to be set to at least 30% PWM duty cycle
>> to spin up from a dead stop, and can be afterward throttled down to
>> lower PWM duty cycle. Introduce support for operating such fans which
> 
> Doesn't this imply that a minimum pwm value is needed as well ?

It depends. For this fan, yes, it does stop at around 8% PWM duty cycle.

> Super-IO chips such as the NCT67xx series typically have two separate
> registers, one for the pwm start value and one for the minimum pwm value.

I use plain SoC PWM output to operate the fan. This one needs to be set 
to higher PWM duty cycle first, to spin up, and can be reduced to lower 
PWM duty cycle afterward without stopping.

>> need to start at higher PWM duty cycle first and can slow down next.
>>
>> Document two new DT properties, "fan-dead-stop-start-percent" and
>> "fan-dead-stop-start-usec". The former describes the minimum percent
>> of fan RPM at which it will surely spin up from dead stop. This value
>> can be found in the fan datasheet and can be converted to PWM duty
>> cycle easily. The "fan-dead-stop-start-usec" describes the minimum
>> time in microseconds for which the fan has to be set to dead stop
>> start RPM for the fan to surely spin up.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: Conor Dooley <conor+dt@kernel.org>
>> Cc: Guenter Roeck <linux@roeck-us.net>
>> Cc: Jean Delvare <jdelvare@suse.com>
>> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: devicetree@vger.kernel.org
>> Cc: linux-hwmon@vger.kernel.org
>> ---
>>   Documentation/devicetree/bindings/hwmon/pwm-fan.yaml | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml b/ 
>> Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>> index 4e5abf7580cc6..f1042471b5176 100644
>> --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>> +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>> @@ -31,6 +31,17 @@ properties:
>>         it must be self resetting edge interrupts.
>>       maxItems: 1
>> +  fan-dead-stop-start-percent:
> 
> Personally I don't think that "dead-stop" in the property name adds any 
> value.
> On the contrary, I think it leads to confusion. I head to read the 
> description
> to understand.

The documentation refers to this behavior as a "dead stop" , hence the 
property name. I can change it to fan-stop-to-start-percent ?

Re: [PATCH 1/2] dt-bindings: hwmon: pwm-fan: Document start from dead stop properties

[Guenter Roeck]

On 11/5/24 10:53, Marek Vasut wrote:
> On 11/5/24 3:11 PM, Guenter Roeck wrote:
>> On 11/5/24 05:52, Marek Vasut wrote:
>>> Delta AFC0612DB-F00 fan has to be set to at least 30% PWM duty cycle
>>> to spin up from a dead stop, and can be afterward throttled down to
>>> lower PWM duty cycle. Introduce support for operating such fans which
>>
>> Doesn't this imply that a minimum pwm value is needed as well ?
> 
> It depends. For this fan, yes, it does stop at around 8% PWM duty cycle.
> 
>> Super-IO chips such as the NCT67xx series typically have two separate
>> registers, one for the pwm start value and one for the minimum pwm value.
> 
> I use plain SoC PWM output to operate the fan. This one needs to be set to higher PWM duty cycle first, to spin up, and can be reduced to lower PWM duty cycle afterward without stopping.
> 

Yes, exactly. That is what many fans require.

>>> need to start at higher PWM duty cycle first and can slow down next.
>>>
>>> Document two new DT properties, "fan-dead-stop-start-percent" and
>>> "fan-dead-stop-start-usec". The former describes the minimum percent
>>> of fan RPM at which it will surely spin up from dead stop. This value
>>> can be found in the fan datasheet and can be converted to PWM duty
>>> cycle easily. The "fan-dead-stop-start-usec" describes the minimum
>>> time in microseconds for which the fan has to be set to dead stop
>>> start RPM for the fan to surely spin up.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> ---
>>> Cc: Conor Dooley <conor+dt@kernel.org>
>>> Cc: Guenter Roeck <linux@roeck-us.net>
>>> Cc: Jean Delvare <jdelvare@suse.com>
>>> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
>>> Cc: Rob Herring <robh@kernel.org>
>>> Cc: devicetree@vger.kernel.org
>>> Cc: linux-hwmon@vger.kernel.org
>>> ---
>>>   Documentation/devicetree/bindings/hwmon/pwm-fan.yaml | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml b/ Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>>> index 4e5abf7580cc6..f1042471b5176 100644
>>> --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>>> +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>>> @@ -31,6 +31,17 @@ properties:
>>>         it must be self resetting edge interrupts.
>>>       maxItems: 1
>>> +  fan-dead-stop-start-percent:
>>
>> Personally I don't think that "dead-stop" in the property name adds any value.
>> On the contrary, I think it leads to confusion. I head to read the description
>> to understand.
> 
> The documentation refers to this behavior as a "dead stop" , hence the property name. I can change it to fan-stop-to-start-percent ?

I do not understand the need for that much complexity in the property name,
and I don't think it makes sense to name a property based on a specific
chip documentation. I have seen that before, where different vendors use
different names for the same functionality. That doesn't mean that the
vendor-determined name has to make it into the property name.

As an example, Nuvoton calls the values "Start-Up Value" and "Stop Value".
ITE calls the start value "start PWM value" (and as far as I can see doesn't
have a separate stop value). I am sure pretty much every vendor uses a
different description.

I am personally not a friend of long property names. Having said that,
I'll let you use whatever DT maintainers accept. They may have a different
opinion.

Guenter

Re: [PATCH 1/2] dt-bindings: hwmon: pwm-fan: Document start from dead stop properties

[Marek Vasut]

On 11/6/24 1:34 AM, Guenter Roeck wrote:
> On 11/5/24 10:53, Marek Vasut wrote:
>> On 11/5/24 3:11 PM, Guenter Roeck wrote:
>>> On 11/5/24 05:52, Marek Vasut wrote:
>>>> Delta AFC0612DB-F00 fan has to be set to at least 30% PWM duty cycle
>>>> to spin up from a dead stop, and can be afterward throttled down to
>>>> lower PWM duty cycle. Introduce support for operating such fans which
>>>
>>> Doesn't this imply that a minimum pwm value is needed as well ?
>>
>> It depends. For this fan, yes, it does stop at around 8% PWM duty cycle.
>>
>>> Super-IO chips such as the NCT67xx series typically have two separate
>>> registers, one for the pwm start value and one for the minimum pwm 
>>> value.
>>
>> I use plain SoC PWM output to operate the fan. This one needs to be 
>> set to higher PWM duty cycle first, to spin up, and can be reduced to 
>> lower PWM duty cycle afterward without stopping.
>>
> 
> Yes, exactly. That is what many fans require.
> 
>>>> need to start at higher PWM duty cycle first and can slow down next.
>>>>
>>>> Document two new DT properties, "fan-dead-stop-start-percent" and
>>>> "fan-dead-stop-start-usec". The former describes the minimum percent
>>>> of fan RPM at which it will surely spin up from dead stop. This value
>>>> can be found in the fan datasheet and can be converted to PWM duty
>>>> cycle easily. The "fan-dead-stop-start-usec" describes the minimum
>>>> time in microseconds for which the fan has to be set to dead stop
>>>> start RPM for the fan to surely spin up.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> ---
>>>> Cc: Conor Dooley <conor+dt@kernel.org>
>>>> Cc: Guenter Roeck <linux@roeck-us.net>
>>>> Cc: Jean Delvare <jdelvare@suse.com>
>>>> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
>>>> Cc: Rob Herring <robh@kernel.org>
>>>> Cc: devicetree@vger.kernel.org
>>>> Cc: linux-hwmon@vger.kernel.org
>>>> ---
>>>>   Documentation/devicetree/bindings/hwmon/pwm-fan.yaml | 11 +++++++++++
>>>>   1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml b/ 
>>>> Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>>>> index 4e5abf7580cc6..f1042471b5176 100644
>>>> --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>>>> +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>>>> @@ -31,6 +31,17 @@ properties:
>>>>         it must be self resetting edge interrupts.
>>>>       maxItems: 1
>>>> +  fan-dead-stop-start-percent:
>>>
>>> Personally I don't think that "dead-stop" in the property name adds 
>>> any value.
>>> On the contrary, I think it leads to confusion. I head to read the 
>>> description
>>> to understand.
>>
>> The documentation refers to this behavior as a "dead stop" , hence the 
>> property name. I can change it to fan-stop-to-start-percent ?
> 
> I do not understand the need for that much complexity in the property name,
> and I don't think it makes sense to name a property based on a specific
> chip documentation. I have seen that before, where different vendors use
> different names for the same functionality. That doesn't mean that the
> vendor-determined name has to make it into the property name.
> 
> As an example, Nuvoton calls the values "Start-Up Value" and "Stop Value".
> ITE calls the start value "start PWM value" (and as far as I can see 
> doesn't
> have a separate stop value). I am sure pretty much every vendor uses a
> different description.
> 
> I am personally not a friend of long property names. Having said that,
> I'll let you use whatever DT maintainers accept. They may have a different
> opinion.
Do you have a different suggestion for the property name ? Else I'll 
just send a V2 .

Re: [PATCH 1/2] dt-bindings: hwmon: pwm-fan: Document start from dead stop properties

[Guenter Roeck]

On 11/5/24 17:28, Marek Vasut wrote:
> On 11/6/24 1:34 AM, Guenter Roeck wrote:
>> On 11/5/24 10:53, Marek Vasut wrote:
>>> On 11/5/24 3:11 PM, Guenter Roeck wrote:
>>>> On 11/5/24 05:52, Marek Vasut wrote:
>>>>> Delta AFC0612DB-F00 fan has to be set to at least 30% PWM duty cycle
>>>>> to spin up from a dead stop, and can be afterward throttled down to
>>>>> lower PWM duty cycle. Introduce support for operating such fans which
>>>>
>>>> Doesn't this imply that a minimum pwm value is needed as well ?
>>>
>>> It depends. For this fan, yes, it does stop at around 8% PWM duty cycle.
>>>
>>>> Super-IO chips such as the NCT67xx series typically have two separate
>>>> registers, one for the pwm start value and one for the minimum pwm value.
>>>
>>> I use plain SoC PWM output to operate the fan. This one needs to be set to higher PWM duty cycle first, to spin up, and can be reduced to lower PWM duty cycle afterward without stopping.
>>>
>>
>> Yes, exactly. That is what many fans require.
>>
>>>>> need to start at higher PWM duty cycle first and can slow down next.
>>>>>
>>>>> Document two new DT properties, "fan-dead-stop-start-percent" and
>>>>> "fan-dead-stop-start-usec". The former describes the minimum percent
>>>>> of fan RPM at which it will surely spin up from dead stop. This value
>>>>> can be found in the fan datasheet and can be converted to PWM duty
>>>>> cycle easily. The "fan-dead-stop-start-usec" describes the minimum
>>>>> time in microseconds for which the fan has to be set to dead stop
>>>>> start RPM for the fan to surely spin up.
>>>>>
>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>> ---
>>>>> Cc: Conor Dooley <conor+dt@kernel.org>
>>>>> Cc: Guenter Roeck <linux@roeck-us.net>
>>>>> Cc: Jean Delvare <jdelvare@suse.com>
>>>>> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
>>>>> Cc: Rob Herring <robh@kernel.org>
>>>>> Cc: devicetree@vger.kernel.org
>>>>> Cc: linux-hwmon@vger.kernel.org
>>>>> ---
>>>>>   Documentation/devicetree/bindings/hwmon/pwm-fan.yaml | 11 +++++++++++
>>>>>   1 file changed, 11 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml b/ Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>>>>> index 4e5abf7580cc6..f1042471b5176 100644
>>>>> --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>>>>> +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>>>>> @@ -31,6 +31,17 @@ properties:
>>>>>         it must be self resetting edge interrupts.
>>>>>       maxItems: 1
>>>>> +  fan-dead-stop-start-percent:
>>>>
>>>> Personally I don't think that "dead-stop" in the property name adds any value.
>>>> On the contrary, I think it leads to confusion. I head to read the description
>>>> to understand.
>>>
>>> The documentation refers to this behavior as a "dead stop" , hence the property name. I can change it to fan-stop-to-start-percent ?
>>
>> I do not understand the need for that much complexity in the property name,
>> and I don't think it makes sense to name a property based on a specific
>> chip documentation. I have seen that before, where different vendors use
>> different names for the same functionality. That doesn't mean that the
>> vendor-determined name has to make it into the property name.
>>
>> As an example, Nuvoton calls the values "Start-Up Value" and "Stop Value".
>> ITE calls the start value "start PWM value" (and as far as I can see doesn't
>> have a separate stop value). I am sure pretty much every vendor uses a
>> different description.
>>
>> I am personally not a friend of long property names. Having said that,
>> I'll let you use whatever DT maintainers accept. They may have a different
>> opinion.
> Do you have a different suggestion for the property name ? Else I'll just send a V2 .


fan-start-percent and fan-stop-percent would be good enough for me.
However, the existing cooling-levels property uses duty cycle values
from 0 ..255. Using % for the new properties will create an inconsistency.
It will be up to DT maintainers to decide how the properties should be
defined.

Guenter

Re: [PATCH 1/2] dt-bindings: hwmon: pwm-fan: Document start from dead stop properties

[Marek Vasut]

On 11/6/24 2:55 AM, Guenter Roeck wrote:
> On 11/5/24 17:28, Marek Vasut wrote:
>> On 11/6/24 1:34 AM, Guenter Roeck wrote:
>>> On 11/5/24 10:53, Marek Vasut wrote:
>>>> On 11/5/24 3:11 PM, Guenter Roeck wrote:
>>>>> On 11/5/24 05:52, Marek Vasut wrote:
>>>>>> Delta AFC0612DB-F00 fan has to be set to at least 30% PWM duty cycle
>>>>>> to spin up from a dead stop, and can be afterward throttled down to
>>>>>> lower PWM duty cycle. Introduce support for operating such fans which
>>>>>
>>>>> Doesn't this imply that a minimum pwm value is needed as well ?
>>>>
>>>> It depends. For this fan, yes, it does stop at around 8% PWM duty 
>>>> cycle.
>>>>
>>>>> Super-IO chips such as the NCT67xx series typically have two separate
>>>>> registers, one for the pwm start value and one for the minimum pwm 
>>>>> value.
>>>>
>>>> I use plain SoC PWM output to operate the fan. This one needs to be 
>>>> set to higher PWM duty cycle first, to spin up, and can be reduced 
>>>> to lower PWM duty cycle afterward without stopping.
>>>>
>>>
>>> Yes, exactly. That is what many fans require.
>>>
>>>>>> need to start at higher PWM duty cycle first and can slow down next.
>>>>>>
>>>>>> Document two new DT properties, "fan-dead-stop-start-percent" and
>>>>>> "fan-dead-stop-start-usec". The former describes the minimum percent
>>>>>> of fan RPM at which it will surely spin up from dead stop. This value
>>>>>> can be found in the fan datasheet and can be converted to PWM duty
>>>>>> cycle easily. The "fan-dead-stop-start-usec" describes the minimum
>>>>>> time in microseconds for which the fan has to be set to dead stop
>>>>>> start RPM for the fan to surely spin up.
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>> ---
>>>>>> Cc: Conor Dooley <conor+dt@kernel.org>
>>>>>> Cc: Guenter Roeck <linux@roeck-us.net>
>>>>>> Cc: Jean Delvare <jdelvare@suse.com>
>>>>>> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
>>>>>> Cc: Rob Herring <robh@kernel.org>
>>>>>> Cc: devicetree@vger.kernel.org
>>>>>> Cc: linux-hwmon@vger.kernel.org
>>>>>> ---
>>>>>>   Documentation/devicetree/bindings/hwmon/pwm-fan.yaml | 11 ++++++ 
>>>>>> +++++
>>>>>>   1 file changed, 11 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml 
>>>>>> b/ Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>>>>>> index 4e5abf7580cc6..f1042471b5176 100644
>>>>>> --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
>>>>>> @@ -31,6 +31,17 @@ properties:
>>>>>>         it must be self resetting edge interrupts.
>>>>>>       maxItems: 1
>>>>>> +  fan-dead-stop-start-percent:
>>>>>
>>>>> Personally I don't think that "dead-stop" in the property name adds 
>>>>> any value.
>>>>> On the contrary, I think it leads to confusion. I head to read the 
>>>>> description
>>>>> to understand.
>>>>
>>>> The documentation refers to this behavior as a "dead stop" , hence 
>>>> the property name. I can change it to fan-stop-to-start-percent ?
>>>
>>> I do not understand the need for that much complexity in the property 
>>> name,
>>> and I don't think it makes sense to name a property based on a specific
>>> chip documentation. I have seen that before, where different vendors use
>>> different names for the same functionality. That doesn't mean that the
>>> vendor-determined name has to make it into the property name.
>>>
>>> As an example, Nuvoton calls the values "Start-Up Value" and "Stop 
>>> Value".
>>> ITE calls the start value "start PWM value" (and as far as I can see 
>>> doesn't
>>> have a separate stop value). I am sure pretty much every vendor uses a
>>> different description.
>>>
>>> I am personally not a friend of long property names. Having said that,
>>> I'll let you use whatever DT maintainers accept. They may have a 
>>> different
>>> opinion.
>> Do you have a different suggestion for the property name ? Else I'll 
>> just send a V2 .
> 
> 
> fan-start-percent and fan-stop-percent would be good enough for me.
> However, the existing cooling-levels property uses duty cycle values
> from 0 ..255. Using % for the new properties will create an inconsistency.
> It will be up to DT maintainers to decide how the properties should be
> defined.
All right, well ... I sent V2 with what I have in tree now, and let's 
see what happens.

Re: [PATCH 1/2] dt-bindings: hwmon: pwm-fan: Document start from dead stop properties

[Conor Dooley]

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

On Tue, Nov 05, 2024 at 02:52:15PM +0100, Marek Vasut wrote:
> Delta AFC0612DB-F00 fan has to be set to at least 30% PWM duty cycle
> to spin up from a dead stop, and can be afterward throttled down to
> lower PWM duty cycle. Introduce support for operating such fans which
> need to start at higher PWM duty cycle first and can slow down next.
> 
> Document two new DT properties, "fan-dead-stop-start-percent" and
> "fan-dead-stop-start-usec". The former describes the minimum percent
> of fan RPM at which it will surely spin up from dead stop. This value
> can be found in the fan datasheet and can be converted to PWM duty
> cycle easily. The "fan-dead-stop-start-usec" describes the minimum
> time in microseconds for which the fan has to be set to dead stop
> start RPM for the fan to surely spin up.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-hwmon@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/hwmon/pwm-fan.yaml | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
> index 4e5abf7580cc6..f1042471b5176 100644
> --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml
> @@ -31,6 +31,17 @@ properties:
>        it must be self resetting edge interrupts.
>      maxItems: 1
>  
> +  fan-dead-stop-start-percent:
> +    description:
> +      Minimum fan RPM in percent to start from dead stop.
> +    minimum: 0
> +    maximum: 100
> +
> +  fan-dead-stop-start-usec:

s/usec/us/ and you get the type from property-units.yaml automagically.

> +    description:
> +      Time to wait in microseconds after start from dead stop.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
>    pulses-per-revolution:
>      description:
>        Define the number of pulses per fan revolution for each tachometer
> -- 
> 2.45.2
> 

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