[PATCH 0/2] hwmon: Add new pwmX_fan_channel attribute

[PATCH 0/2] hwmon: Add new pwmX_fan_channel attribute

[Armin Wolf]

Userspace software which needs to know which PWM channel is
associated with which fan channel (like fancontrol/pwmconfig)
currently has to probe each PWM channel and watch for fan speed
changes.

This process has multiple issues:
1. Hardware might overhead during probing of PWM channels.
2. Not all fans react the same to PWM changes, making
detection unreliable.

However some hwmon chips, especially firmware-based ones, already
know which PWM channels are associated to which fan channel,
making probing of them unnecessary.

Add a new sysfs attribute to allow such chips to export
this knowlege to userspace in a standardized manner.

The first patch adds support for the new attribute to the hwmon core,
while the second patch adds support for this new attribute to the
dell-smm-hwmon driver.

All changes have been tested on a Dell Inspiron 3505.

Armin Wolf (2):
  hwmon: Add pwmX_fan_channel attribute
  hwmon: (dell-smm) Add support for pwmX_fan_channel attribute

 Documentation/ABI/testing/sysfs-class-hwmon |  8 ++++++++
 Documentation/hwmon/dell-smm-hwmon.rst      |  1 +
 Documentation/hwmon/sysfs-interface.rst     |  3 +++
 drivers/hwmon/dell-smm-hwmon.c              | 15 ++++++++++++---
 drivers/hwmon/hwmon.c                       |  1 +
 include/linux/hwmon.h                       |  2 ++
 6 files changed, 27 insertions(+), 3 deletions(-)

--
2.30.2

[PATCH 1/2] hwmon: Add pwmX_fan_channel attribute

[Armin Wolf]

Until now, userspace software needs to guess which
PWM channel is associated with which fan channel by
probing each PWM output and watch for fan speed changes.
This proccess is error-prone and unreliable.

Some hwmon chips, especially firmware-based ones, already
know which PWM output is associated with which fan channel.

Allow such chips to export this knowledge to userspace.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 Documentation/ABI/testing/sysfs-class-hwmon | 8 ++++++++
 Documentation/hwmon/sysfs-interface.rst     | 3 +++
 drivers/hwmon/hwmon.c                       | 1 +
 include/linux/hwmon.h                       | 2 ++
 4 files changed, 14 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-hwmon b/Documentation/ABI/testing/sysfs-class-hwmon
index 7271781a23b2..f3d653bcf736 100644
--- a/Documentation/ABI/testing/sysfs-class-hwmon
+++ b/Documentation/ABI/testing/sysfs-class-hwmon
@@ -315,6 +315,14 @@ Description:

 		RW

+What:		/sys/class/hwmon/hwmonX/pwmY_fan_channel
+Description:
+		Select which fan channel is controlled by this PWM output.
+
+		Valid fan channel/PWM output combinations are chip-dependent.
+
+		RW
+
 What:		/sys/class/hwmon/hwmonX/pwmY_auto_channels_temp
 Description:
 		Select which temperature channels affect this PWM output in
diff --git a/Documentation/hwmon/sysfs-interface.rst b/Documentation/hwmon/sysfs-interface.rst
index 209626fb2405..17fcec03d3c5 100644
--- a/Documentation/hwmon/sysfs-interface.rst
+++ b/Documentation/hwmon/sysfs-interface.rst
@@ -209,6 +209,9 @@ PWM
 `pwm[1-*]_freq`
 		Base PWM frequency in Hz.

+`pwm[1-*]_fan_channel`
+                Select which fan channel is controlled by this PWM output.
+
 `pwm[1-*]_auto_channels_temp`
 		Select which temperature channels affect this PWM output in
 		auto mode.
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 2e2cd79d89eb..8c2d7574c461 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -604,6 +604,7 @@ static const char * const hwmon_pwm_attr_templates[] = {
 	[hwmon_pwm_enable] = "pwm%d_enable",
 	[hwmon_pwm_mode] = "pwm%d_mode",
 	[hwmon_pwm_freq] = "pwm%d_freq",
+	[hwmon_pwm_fan_channel] = "pwm%d_fan_channel",
 	[hwmon_pwm_auto_channels_temp] = "pwm%d_auto_channels_temp",
 };

diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 14325f93c6b2..9d40cc1e520f 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -332,6 +332,7 @@ enum hwmon_pwm_attributes {
 	hwmon_pwm_enable,
 	hwmon_pwm_mode,
 	hwmon_pwm_freq,
+	hwmon_pwm_fan_channel,
 	hwmon_pwm_auto_channels_temp,
 };

@@ -339,6 +340,7 @@ enum hwmon_pwm_attributes {
 #define HWMON_PWM_ENABLE		BIT(hwmon_pwm_enable)
 #define HWMON_PWM_MODE			BIT(hwmon_pwm_mode)
 #define HWMON_PWM_FREQ			BIT(hwmon_pwm_freq)
+#define HWMON_PWM_FAN_CHANNEL		BIT(hwmon_pwm_fan_channel)
 #define HWMON_PWM_AUTO_CHANNELS_TEMP	BIT(hwmon_pwm_auto_channels_temp)

 enum hwmon_intrusion_attributes {
--
2.30.2

[PATCH 2/2] hwmon: (dell-smm) Add support for pwmX_fan_channel attribute

[Armin Wolf]

The SMM interface does not differentiate between fan channels
and pwm channels, so each pwmX is associated with fanX_*.

Inform userspace programs of this with the pwmX_fan_channel
attribute.

Tested on a Dell Inspiron 3505.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 Documentation/hwmon/dell-smm-hwmon.rst |  1 +
 drivers/hwmon/dell-smm-hwmon.c         | 15 ++++++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
index e5d85e40972c..2b0651c3f7c5 100644
--- a/Documentation/hwmon/dell-smm-hwmon.rst
+++ b/Documentation/hwmon/dell-smm-hwmon.rst
@@ -38,6 +38,7 @@ fan[1-3]_min                    RO      Minimal Fan speed in RPM
 fan[1-3]_max                    RO      Maximal Fan speed in RPM
 fan[1-3]_target                 RO      Expected Fan speed in RPM
 pwm[1-3]                        RW      Control the fan PWM duty-cycle.
+pwm[1-3]_fan_channel            RO      Hints which PWM is connected to which fan
 pwm1_enable                     WO      Enable or disable automatic BIOS fan
                                         control (not supported on all laptops,
                                         see below for details).
diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 7f8d95dd2717..e6bf65d2ffc6 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -713,6 +713,11 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
 			if (data->fan[channel])
 				return 0644;

+			break;
+		case hwmon_pwm_fan_channel:
+			if (data->fan[channel])
+				return 0444;
+
 			break;
 		case hwmon_pwm_enable:
 			if (data->auto_fan)
@@ -800,6 +805,10 @@ static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 a

 			*val = clamp_val(ret * data->i8k_pwm_mult, 0, 255);

+			return 0;
+		case hwmon_pwm_fan_channel:
+			*val = channel + 1;
+
 			return 0;
 		default:
 			break;
@@ -943,9 +952,9 @@ static const struct hwmon_channel_info *dell_smm_info[] = {
 			   HWMON_F_TARGET
 			   ),
 	HWMON_CHANNEL_INFO(pwm,
-			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
-			   HWMON_PWM_INPUT,
-			   HWMON_PWM_INPUT
+			   HWMON_PWM_INPUT | HWMON_PWM_FAN_CHANNEL | HWMON_PWM_ENABLE,
+			   HWMON_PWM_INPUT | HWMON_PWM_FAN_CHANNEL,
+			   HWMON_PWM_INPUT | HWMON_PWM_FAN_CHANNEL
 			   ),
 	NULL
 };
--
2.30.2

Re: [PATCH 2/2] hwmon: (dell-smm) Add support for pwmX_fan_channel attribute

[Guenter Roeck]

On Sat, Jul 23, 2022 at 05:38:20AM +0200, Armin Wolf wrote:
> The SMM interface does not differentiate between fan channels
> and pwm channels, so each pwmX is associated with fanX_*.
> 
> Inform userspace programs of this with the pwmX_fan_channel
> attribute.
> 
> Tested on a Dell Inspiron 3505.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  Documentation/hwmon/dell-smm-hwmon.rst |  1 +
>  drivers/hwmon/dell-smm-hwmon.c         | 15 ++++++++++++---
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/hwmon/dell-smm-hwmon.rst b/Documentation/hwmon/dell-smm-hwmon.rst
> index e5d85e40972c..2b0651c3f7c5 100644
> --- a/Documentation/hwmon/dell-smm-hwmon.rst
> +++ b/Documentation/hwmon/dell-smm-hwmon.rst
> @@ -38,6 +38,7 @@ fan[1-3]_min                    RO      Minimal Fan speed in RPM
>  fan[1-3]_max                    RO      Maximal Fan speed in RPM
>  fan[1-3]_target                 RO      Expected Fan speed in RPM
>  pwm[1-3]                        RW      Control the fan PWM duty-cycle.
> +pwm[1-3]_fan_channel            RO      Hints which PWM is connected to which fan
>  pwm1_enable                     WO      Enable or disable automatic BIOS fan
>                                          control (not supported on all laptops,
>                                          see below for details).
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 7f8d95dd2717..e6bf65d2ffc6 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -713,6 +713,11 @@ static umode_t dell_smm_is_visible(const void *drvdata, enum hwmon_sensor_types
>  			if (data->fan[channel])
>  				return 0644;
> 
> +			break;
> +		case hwmon_pwm_fan_channel:
> +			if (data->fan[channel])
> +				return 0444;
> +
>  			break;
>  		case hwmon_pwm_enable:
>  			if (data->auto_fan)
> @@ -800,6 +805,10 @@ static int dell_smm_read(struct device *dev, enum hwmon_sensor_types type, u32 a
> 
>  			*val = clamp_val(ret * data->i8k_pwm_mult, 0, 255);
> 
> +			return 0;
> +		case hwmon_pwm_fan_channel:
> +			*val = channel + 1;
> +

This effectively just returns the channel number and
thus adds adds zero value.

Guenter

>  			return 0;
>  		default:
>  			break;
> @@ -943,9 +952,9 @@ static const struct hwmon_channel_info *dell_smm_info[] = {
>  			   HWMON_F_TARGET
>  			   ),
>  	HWMON_CHANNEL_INFO(pwm,
> -			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> -			   HWMON_PWM_INPUT,
> -			   HWMON_PWM_INPUT
> +			   HWMON_PWM_INPUT | HWMON_PWM_FAN_CHANNEL | HWMON_PWM_ENABLE,
> +			   HWMON_PWM_INPUT | HWMON_PWM_FAN_CHANNEL,
> +			   HWMON_PWM_INPUT | HWMON_PWM_FAN_CHANNEL
>  			   ),
>  	NULL
>  };
> --
> 2.30.2
> 

Re: [PATCH 1/2] hwmon: Add pwmX_fan_channel attribute

[Guenter Roeck]

On Sat, Jul 23, 2022 at 05:38:19AM +0200, Armin Wolf wrote:
> Until now, userspace software needs to guess which
> PWM channel is associated with which fan channel by
> probing each PWM output and watch for fan speed changes.
> This proccess is error-prone and unreliable.
> 
> Some hwmon chips, especially firmware-based ones, already
> know which PWM output is associated with which fan channel.
> 
> Allow such chips to export this knowledge to userspace.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

All of the chips I am aware of have a fixed association from pwm channel
output to fan input. None I am aware of make this association configurable.
I do not see the value of this attribute.

Guenter

> ---
>  Documentation/ABI/testing/sysfs-class-hwmon | 8 ++++++++
>  Documentation/hwmon/sysfs-interface.rst     | 3 +++
>  drivers/hwmon/hwmon.c                       | 1 +
>  include/linux/hwmon.h                       | 2 ++
>  4 files changed, 14 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-hwmon b/Documentation/ABI/testing/sysfs-class-hwmon
> index 7271781a23b2..f3d653bcf736 100644
> --- a/Documentation/ABI/testing/sysfs-class-hwmon
> +++ b/Documentation/ABI/testing/sysfs-class-hwmon
> @@ -315,6 +315,14 @@ Description:
> 
>  		RW
> 
> +What:		/sys/class/hwmon/hwmonX/pwmY_fan_channel
> +Description:
> +		Select which fan channel is controlled by this PWM output.
> +
> +		Valid fan channel/PWM output combinations are chip-dependent.
> +
> +		RW
> +
>  What:		/sys/class/hwmon/hwmonX/pwmY_auto_channels_temp
>  Description:
>  		Select which temperature channels affect this PWM output in
> diff --git a/Documentation/hwmon/sysfs-interface.rst b/Documentation/hwmon/sysfs-interface.rst
> index 209626fb2405..17fcec03d3c5 100644
> --- a/Documentation/hwmon/sysfs-interface.rst
> +++ b/Documentation/hwmon/sysfs-interface.rst
> @@ -209,6 +209,9 @@ PWM
>  `pwm[1-*]_freq`
>  		Base PWM frequency in Hz.
> 
> +`pwm[1-*]_fan_channel`
> +                Select which fan channel is controlled by this PWM output.
> +
>  `pwm[1-*]_auto_channels_temp`
>  		Select which temperature channels affect this PWM output in
>  		auto mode.
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 2e2cd79d89eb..8c2d7574c461 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -604,6 +604,7 @@ static const char * const hwmon_pwm_attr_templates[] = {
>  	[hwmon_pwm_enable] = "pwm%d_enable",
>  	[hwmon_pwm_mode] = "pwm%d_mode",
>  	[hwmon_pwm_freq] = "pwm%d_freq",
> +	[hwmon_pwm_fan_channel] = "pwm%d_fan_channel",
>  	[hwmon_pwm_auto_channels_temp] = "pwm%d_auto_channels_temp",
>  };
> 
> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
> index 14325f93c6b2..9d40cc1e520f 100644
> --- a/include/linux/hwmon.h
> +++ b/include/linux/hwmon.h
> @@ -332,6 +332,7 @@ enum hwmon_pwm_attributes {
>  	hwmon_pwm_enable,
>  	hwmon_pwm_mode,
>  	hwmon_pwm_freq,
> +	hwmon_pwm_fan_channel,
>  	hwmon_pwm_auto_channels_temp,
>  };
> 
> @@ -339,6 +340,7 @@ enum hwmon_pwm_attributes {
>  #define HWMON_PWM_ENABLE		BIT(hwmon_pwm_enable)
>  #define HWMON_PWM_MODE			BIT(hwmon_pwm_mode)
>  #define HWMON_PWM_FREQ			BIT(hwmon_pwm_freq)
> +#define HWMON_PWM_FAN_CHANNEL		BIT(hwmon_pwm_fan_channel)
>  #define HWMON_PWM_AUTO_CHANNELS_TEMP	BIT(hwmon_pwm_auto_channels_temp)
> 
>  enum hwmon_intrusion_attributes {
> --
> 2.30.2
> 

Re: [PATCH 1/2] hwmon: Add pwmX_fan_channel attribute

[Armin Wolf]

Am 23.07.22 um 16:17 schrieb Guenter Roeck:

> On Sat, Jul 23, 2022 at 05:38:19AM +0200, Armin Wolf wrote:
>> Until now, userspace software needs to guess which
>> PWM channel is associated with which fan channel by
>> probing each PWM output and watch for fan speed changes.
>> This proccess is error-prone and unreliable.
>>
>> Some hwmon chips, especially firmware-based ones, already
>> know which PWM output is associated with which fan channel.
>>
>> Allow such chips to export this knowledge to userspace.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> All of the chips I am aware of have a fixed association from pwm channel
> output to fan input. None I am aware of make this association configurable.
> I do not see the value of this attribute.
>
> Guenter

That is true, the association from pwm channel output to fan input is usually fixed.
However not all chips are able to discover which pwm channel output is associated with
which fan channel. For example most superio-based chips cannot know how the motherboard
manufacturer wired the fans, and thus userspace relies on pwmconfig for manually probing
each pwm channel.
In contrast, many firmware-based chips do know which pwm channel output controls which
fan channel. One example might be the dell-smm-hwmon driver and the gpio-fan driver.

In this case, making the attribute RO would indeed make sense.

Armin Wolf

>> ---
>>   Documentation/ABI/testing/sysfs-class-hwmon | 8 ++++++++
>>   Documentation/hwmon/sysfs-interface.rst     | 3 +++
>>   drivers/hwmon/hwmon.c                       | 1 +
>>   include/linux/hwmon.h                       | 2 ++
>>   4 files changed, 14 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-hwmon b/Documentation/ABI/testing/sysfs-class-hwmon
>> index 7271781a23b2..f3d653bcf736 100644
>> --- a/Documentation/ABI/testing/sysfs-class-hwmon
>> +++ b/Documentation/ABI/testing/sysfs-class-hwmon
>> @@ -315,6 +315,14 @@ Description:
>>
>>   		RW
>>
>> +What:		/sys/class/hwmon/hwmonX/pwmY_fan_channel
>> +Description:
>> +		Select which fan channel is controlled by this PWM output.
>> +
>> +		Valid fan channel/PWM output combinations are chip-dependent.
>> +
>> +		RW
>> +
>>   What:		/sys/class/hwmon/hwmonX/pwmY_auto_channels_temp
>>   Description:
>>   		Select which temperature channels affect this PWM output in
>> diff --git a/Documentation/hwmon/sysfs-interface.rst b/Documentation/hwmon/sysfs-interface.rst
>> index 209626fb2405..17fcec03d3c5 100644
>> --- a/Documentation/hwmon/sysfs-interface.rst
>> +++ b/Documentation/hwmon/sysfs-interface.rst
>> @@ -209,6 +209,9 @@ PWM
>>   `pwm[1-*]_freq`
>>   		Base PWM frequency in Hz.
>>
>> +`pwm[1-*]_fan_channel`
>> +                Select which fan channel is controlled by this PWM output.
>> +
>>   `pwm[1-*]_auto_channels_temp`
>>   		Select which temperature channels affect this PWM output in
>>   		auto mode.
>> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
>> index 2e2cd79d89eb..8c2d7574c461 100644
>> --- a/drivers/hwmon/hwmon.c
>> +++ b/drivers/hwmon/hwmon.c
>> @@ -604,6 +604,7 @@ static const char * const hwmon_pwm_attr_templates[] = {
>>   	[hwmon_pwm_enable] = "pwm%d_enable",
>>   	[hwmon_pwm_mode] = "pwm%d_mode",
>>   	[hwmon_pwm_freq] = "pwm%d_freq",
>> +	[hwmon_pwm_fan_channel] = "pwm%d_fan_channel",
>>   	[hwmon_pwm_auto_channels_temp] = "pwm%d_auto_channels_temp",
>>   };
>>
>> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
>> index 14325f93c6b2..9d40cc1e520f 100644
>> --- a/include/linux/hwmon.h
>> +++ b/include/linux/hwmon.h
>> @@ -332,6 +332,7 @@ enum hwmon_pwm_attributes {
>>   	hwmon_pwm_enable,
>>   	hwmon_pwm_mode,
>>   	hwmon_pwm_freq,
>> +	hwmon_pwm_fan_channel,
>>   	hwmon_pwm_auto_channels_temp,
>>   };
>>
>> @@ -339,6 +340,7 @@ enum hwmon_pwm_attributes {
>>   #define HWMON_PWM_ENABLE		BIT(hwmon_pwm_enable)
>>   #define HWMON_PWM_MODE			BIT(hwmon_pwm_mode)
>>   #define HWMON_PWM_FREQ			BIT(hwmon_pwm_freq)
>> +#define HWMON_PWM_FAN_CHANNEL		BIT(hwmon_pwm_fan_channel)
>>   #define HWMON_PWM_AUTO_CHANNELS_TEMP	BIT(hwmon_pwm_auto_channels_temp)
>>
>>   enum hwmon_intrusion_attributes {
>> --
>> 2.30.2
>>

Re: [PATCH 1/2] hwmon: Add pwmX_fan_channel attribute

[Guenter Roeck]

On 7/23/22 18:20, Armin Wolf wrote:
> Am 23.07.22 um 16:17 schrieb Guenter Roeck:
> 
>> On Sat, Jul 23, 2022 at 05:38:19AM +0200, Armin Wolf wrote:
>>> Until now, userspace software needs to guess which
>>> PWM channel is associated with which fan channel by
>>> probing each PWM output and watch for fan speed changes.
>>> This proccess is error-prone and unreliable.
>>>
>>> Some hwmon chips, especially firmware-based ones, already
>>> know which PWM output is associated with which fan channel.
>>>
>>> Allow such chips to export this knowledge to userspace.
>>>
>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> All of the chips I am aware of have a fixed association from pwm channel
>> output to fan input. None I am aware of make this association configurable.
>> I do not see the value of this attribute.
>>
>> Guenter
> 
> That is true, the association from pwm channel output to fan input is usually fixed.
> However not all chips are able to discover which pwm channel output is associated with
> which fan channel. For example most superio-based chips cannot know how the motherboard
> manufacturer wired the fans, and thus userspace relies on pwmconfig for manually probing
> each pwm channel.

Alternatively, the user can figure it out based on board documentation
and configure it directly. That is how it is. That doesn't mean that it makes
sense to 'store' that information in a sysfs attribute. That is not what hwmon
sysfs attributes are supposed to be used for.

> In contrast, many firmware-based chips do know which pwm channel output controls which
> fan channel. One example might be the dell-smm-hwmon driver and the gpio-fan driver.
> 
> In this case, making the attribute RO would indeed make sense.
> 

Unless the attribute is used to configure the chip, it does not make sense
in the first place. Also note that gpio-fan is usually configured using
devicetree properties, _and_ it only has a single set of fan/pwm properties,
so a sysfs attribute would always return 1 and make make even less sense there.

Guenter

> Armin Wolf
> 
>>> ---
>>>   Documentation/ABI/testing/sysfs-class-hwmon | 8 ++++++++
>>>   Documentation/hwmon/sysfs-interface.rst     | 3 +++
>>>   drivers/hwmon/hwmon.c                       | 1 +
>>>   include/linux/hwmon.h                       | 2 ++
>>>   4 files changed, 14 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-hwmon b/Documentation/ABI/testing/sysfs-class-hwmon
>>> index 7271781a23b2..f3d653bcf736 100644
>>> --- a/Documentation/ABI/testing/sysfs-class-hwmon
>>> +++ b/Documentation/ABI/testing/sysfs-class-hwmon
>>> @@ -315,6 +315,14 @@ Description:
>>>
>>>           RW
>>>
>>> +What:        /sys/class/hwmon/hwmonX/pwmY_fan_channel
>>> +Description:
>>> +        Select which fan channel is controlled by this PWM output.
>>> +
>>> +        Valid fan channel/PWM output combinations are chip-dependent.
>>> +
>>> +        RW
>>> +
>>>   What:        /sys/class/hwmon/hwmonX/pwmY_auto_channels_temp
>>>   Description:
>>>           Select which temperature channels affect this PWM output in
>>> diff --git a/Documentation/hwmon/sysfs-interface.rst b/Documentation/hwmon/sysfs-interface.rst
>>> index 209626fb2405..17fcec03d3c5 100644
>>> --- a/Documentation/hwmon/sysfs-interface.rst
>>> +++ b/Documentation/hwmon/sysfs-interface.rst
>>> @@ -209,6 +209,9 @@ PWM
>>>   `pwm[1-*]_freq`
>>>           Base PWM frequency in Hz.
>>>
>>> +`pwm[1-*]_fan_channel`
>>> +                Select which fan channel is controlled by this PWM output.
>>> +
>>>   `pwm[1-*]_auto_channels_temp`
>>>           Select which temperature channels affect this PWM output in
>>>           auto mode.
>>> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
>>> index 2e2cd79d89eb..8c2d7574c461 100644
>>> --- a/drivers/hwmon/hwmon.c
>>> +++ b/drivers/hwmon/hwmon.c
>>> @@ -604,6 +604,7 @@ static const char * const hwmon_pwm_attr_templates[] = {
>>>       [hwmon_pwm_enable] = "pwm%d_enable",
>>>       [hwmon_pwm_mode] = "pwm%d_mode",
>>>       [hwmon_pwm_freq] = "pwm%d_freq",
>>> +    [hwmon_pwm_fan_channel] = "pwm%d_fan_channel",
>>>       [hwmon_pwm_auto_channels_temp] = "pwm%d_auto_channels_temp",
>>>   };
>>>
>>> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
>>> index 14325f93c6b2..9d40cc1e520f 100644
>>> --- a/include/linux/hwmon.h
>>> +++ b/include/linux/hwmon.h
>>> @@ -332,6 +332,7 @@ enum hwmon_pwm_attributes {
>>>       hwmon_pwm_enable,
>>>       hwmon_pwm_mode,
>>>       hwmon_pwm_freq,
>>> +    hwmon_pwm_fan_channel,
>>>       hwmon_pwm_auto_channels_temp,
>>>   };
>>>
>>> @@ -339,6 +340,7 @@ enum hwmon_pwm_attributes {
>>>   #define HWMON_PWM_ENABLE        BIT(hwmon_pwm_enable)
>>>   #define HWMON_PWM_MODE            BIT(hwmon_pwm_mode)
>>>   #define HWMON_PWM_FREQ            BIT(hwmon_pwm_freq)
>>> +#define HWMON_PWM_FAN_CHANNEL        BIT(hwmon_pwm_fan_channel)
>>>   #define HWMON_PWM_AUTO_CHANNELS_TEMP    BIT(hwmon_pwm_auto_channels_temp)
>>>
>>>   enum hwmon_intrusion_attributes {
>>> -- 
>>> 2.30.2
>>>

Re: [PATCH 1/2] hwmon: Add pwmX_fan_channel attribute

[Armin Wolf]

Am 24.07.22 um 16:44 schrieb Guenter Roeck:

> On 7/23/22 18:20, Armin Wolf wrote:
>> Am 23.07.22 um 16:17 schrieb Guenter Roeck:
>>
>>> On Sat, Jul 23, 2022 at 05:38:19AM +0200, Armin Wolf wrote:
>>>> Until now, userspace software needs to guess which
>>>> PWM channel is associated with which fan channel by
>>>> probing each PWM output and watch for fan speed changes.
>>>> This proccess is error-prone and unreliable.
>>>>
>>>> Some hwmon chips, especially firmware-based ones, already
>>>> know which PWM output is associated with which fan channel.
>>>>
>>>> Allow such chips to export this knowledge to userspace.
>>>>
>>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> All of the chips I am aware of have a fixed association from pwm
>>> channel
>>> output to fan input. None I am aware of make this association
>>> configurable.
>>> I do not see the value of this attribute.
>>>
>>> Guenter
>>
>> That is true, the association from pwm channel output to fan input is
>> usually fixed.
>> However not all chips are able to discover which pwm channel output
>> is associated with
>> which fan channel. For example most superio-based chips cannot know
>> how the motherboard
>> manufacturer wired the fans, and thus userspace relies on pwmconfig
>> for manually probing
>> each pwm channel.
>
> Alternatively, the user can figure it out based on board documentation
> and configure it directly. That is how it is. That doesn't mean that
> it makes
> sense to 'store' that information in a sysfs attribute. That is not
> what hwmon
> sysfs attributes are supposed to be used for.
>
Good point.
It would be indeed better if userspace software like pwmconfig would
use an internal list containing the names of all hwmon chips for which
the pwm to fan mappings are known.
I will add a note to the documentation of dell-smm-hwmon about the
pwm to fan mapping so userspace software knows about this.
Sorry for bothering you.

Armin Wolf

>> In contrast, many firmware-based chips do know which pwm channel
>> output controls which
>> fan channel. One example might be the dell-smm-hwmon driver and the
>> gpio-fan driver.
>>
>> In this case, making the attribute RO would indeed make sense.
>>
>
> Unless the attribute is used to configure the chip, it does not make
> sense
> in the first place. Also note that gpio-fan is usually configured using
> devicetree properties, _and_ it only has a single set of fan/pwm
> properties,
> so a sysfs attribute would always return 1 and make make even less
> sense there.
>
> Guenter
>
>> Armin Wolf
>>
>>>> ---
>>>>   Documentation/ABI/testing/sysfs-class-hwmon | 8 ++++++++
>>>>   Documentation/hwmon/sysfs-interface.rst     | 3 +++
>>>>   drivers/hwmon/hwmon.c                       | 1 +
>>>>   include/linux/hwmon.h                       | 2 ++
>>>>   4 files changed, 14 insertions(+)
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-class-hwmon
>>>> b/Documentation/ABI/testing/sysfs-class-hwmon
>>>> index 7271781a23b2..f3d653bcf736 100644
>>>> --- a/Documentation/ABI/testing/sysfs-class-hwmon
>>>> +++ b/Documentation/ABI/testing/sysfs-class-hwmon
>>>> @@ -315,6 +315,14 @@ Description:
>>>>
>>>>           RW
>>>>
>>>> +What:        /sys/class/hwmon/hwmonX/pwmY_fan_channel
>>>> +Description:
>>>> +        Select which fan channel is controlled by this PWM output.
>>>> +
>>>> +        Valid fan channel/PWM output combinations are chip-dependent.
>>>> +
>>>> +        RW
>>>> +
>>>>   What: /sys/class/hwmon/hwmonX/pwmY_auto_channels_temp
>>>>   Description:
>>>>           Select which temperature channels affect this PWM output in
>>>> diff --git a/Documentation/hwmon/sysfs-interface.rst
>>>> b/Documentation/hwmon/sysfs-interface.rst
>>>> index 209626fb2405..17fcec03d3c5 100644
>>>> --- a/Documentation/hwmon/sysfs-interface.rst
>>>> +++ b/Documentation/hwmon/sysfs-interface.rst
>>>> @@ -209,6 +209,9 @@ PWM
>>>>   `pwm[1-*]_freq`
>>>>           Base PWM frequency in Hz.
>>>>
>>>> +`pwm[1-*]_fan_channel`
>>>> +                Select which fan channel is controlled by this PWM
>>>> output.
>>>> +
>>>>   `pwm[1-*]_auto_channels_temp`
>>>>           Select which temperature channels affect this PWM output in
>>>>           auto mode.
>>>> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
>>>> index 2e2cd79d89eb..8c2d7574c461 100644
>>>> --- a/drivers/hwmon/hwmon.c
>>>> +++ b/drivers/hwmon/hwmon.c
>>>> @@ -604,6 +604,7 @@ static const char * const
>>>> hwmon_pwm_attr_templates[] = {
>>>>       [hwmon_pwm_enable] = "pwm%d_enable",
>>>>       [hwmon_pwm_mode] = "pwm%d_mode",
>>>>       [hwmon_pwm_freq] = "pwm%d_freq",
>>>> +    [hwmon_pwm_fan_channel] = "pwm%d_fan_channel",
>>>>       [hwmon_pwm_auto_channels_temp] = "pwm%d_auto_channels_temp",
>>>>   };
>>>>
>>>> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
>>>> index 14325f93c6b2..9d40cc1e520f 100644
>>>> --- a/include/linux/hwmon.h
>>>> +++ b/include/linux/hwmon.h
>>>> @@ -332,6 +332,7 @@ enum hwmon_pwm_attributes {
>>>>       hwmon_pwm_enable,
>>>>       hwmon_pwm_mode,
>>>>       hwmon_pwm_freq,
>>>> +    hwmon_pwm_fan_channel,
>>>>       hwmon_pwm_auto_channels_temp,
>>>>   };
>>>>
>>>> @@ -339,6 +340,7 @@ enum hwmon_pwm_attributes {
>>>>   #define HWMON_PWM_ENABLE        BIT(hwmon_pwm_enable)
>>>>   #define HWMON_PWM_MODE            BIT(hwmon_pwm_mode)
>>>>   #define HWMON_PWM_FREQ            BIT(hwmon_pwm_freq)
>>>> +#define HWMON_PWM_FAN_CHANNEL BIT(hwmon_pwm_fan_channel)
>>>>   #define HWMON_PWM_AUTO_CHANNELS_TEMP
>>>> BIT(hwmon_pwm_auto_channels_temp)
>>>>
>>>>   enum hwmon_intrusion_attributes {
>>>> --
>>>> 2.30.2
>>>>
>

Re: [PATCH 1/2] hwmon: Add pwmX_fan_channel attribute

[Guenter Roeck]

On 7/24/22 09:06, Armin Wolf wrote:
[ ... ]
> It would be indeed better if userspace software like pwmconfig would
> use an internal list containing the names of all hwmon chips for which
> the pwm to fan mappings are known.
> I will add a note to the documentation of dell-smm-hwmon about the
> pwm to fan mapping so userspace software knows about this.

That is effectively the fancontrol configuration file. pwmconfig is
supposed to assist in determining how that configuration file should
look like. Having some file added to pwmcontrol to determine how the
fancontrol configuration file should look like seems a bit off-track.

What you are talking about is really the idea of providing a number of
sample configuration files with the fancontrol (or lm-sensors) package,
similar to the various sensors.conf files. That doesn't belong into
the hwmon kernel documentation. It should be part of the lm-sensors
package.

Guenter

> Sorry for bothering you.
> 
> Armin Wolf
> 
>>> In contrast, many firmware-based chips do know which pwm channel
>>> output controls which
>>> fan channel. One example might be the dell-smm-hwmon driver and the
>>> gpio-fan driver.
>>>
>>> In this case, making the attribute RO would indeed make sense.
>>>
>>
>> Unless the attribute is used to configure the chip, it does not make
>> sense
>> in the first place. Also note that gpio-fan is usually configured using
>> devicetree properties, _and_ it only has a single set of fan/pwm
>> properties,
>> so a sysfs attribute would always return 1 and make make even less
>> sense there.
>>
>> Guenter
>>
>>> Armin Wolf
>>>
>>>>> ---
>>>>>   Documentation/ABI/testing/sysfs-class-hwmon | 8 ++++++++
>>>>>   Documentation/hwmon/sysfs-interface.rst     | 3 +++
>>>>>   drivers/hwmon/hwmon.c                       | 1 +
>>>>>   include/linux/hwmon.h                       | 2 ++
>>>>>   4 files changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/ABI/testing/sysfs-class-hwmon
>>>>> b/Documentation/ABI/testing/sysfs-class-hwmon
>>>>> index 7271781a23b2..f3d653bcf736 100644
>>>>> --- a/Documentation/ABI/testing/sysfs-class-hwmon
>>>>> +++ b/Documentation/ABI/testing/sysfs-class-hwmon
>>>>> @@ -315,6 +315,14 @@ Description:
>>>>>
>>>>>           RW
>>>>>
>>>>> +What:        /sys/class/hwmon/hwmonX/pwmY_fan_channel
>>>>> +Description:
>>>>> +        Select which fan channel is controlled by this PWM output.
>>>>> +
>>>>> +        Valid fan channel/PWM output combinations are chip-dependent.
>>>>> +
>>>>> +        RW
>>>>> +
>>>>>   What: /sys/class/hwmon/hwmonX/pwmY_auto_channels_temp
>>>>>   Description:
>>>>>           Select which temperature channels affect this PWM output in
>>>>> diff --git a/Documentation/hwmon/sysfs-interface.rst
>>>>> b/Documentation/hwmon/sysfs-interface.rst
>>>>> index 209626fb2405..17fcec03d3c5 100644
>>>>> --- a/Documentation/hwmon/sysfs-interface.rst
>>>>> +++ b/Documentation/hwmon/sysfs-interface.rst
>>>>> @@ -209,6 +209,9 @@ PWM
>>>>>   `pwm[1-*]_freq`
>>>>>           Base PWM frequency in Hz.
>>>>>
>>>>> +`pwm[1-*]_fan_channel`
>>>>> +                Select which fan channel is controlled by this PWM
>>>>> output.
>>>>> +
>>>>>   `pwm[1-*]_auto_channels_temp`
>>>>>           Select which temperature channels affect this PWM output in
>>>>>           auto mode.
>>>>> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
>>>>> index 2e2cd79d89eb..8c2d7574c461 100644
>>>>> --- a/drivers/hwmon/hwmon.c
>>>>> +++ b/drivers/hwmon/hwmon.c
>>>>> @@ -604,6 +604,7 @@ static const char * const
>>>>> hwmon_pwm_attr_templates[] = {
>>>>>       [hwmon_pwm_enable] = "pwm%d_enable",
>>>>>       [hwmon_pwm_mode] = "pwm%d_mode",
>>>>>       [hwmon_pwm_freq] = "pwm%d_freq",
>>>>> +    [hwmon_pwm_fan_channel] = "pwm%d_fan_channel",
>>>>>       [hwmon_pwm_auto_channels_temp] = "pwm%d_auto_channels_temp",
>>>>>   };
>>>>>
>>>>> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
>>>>> index 14325f93c6b2..9d40cc1e520f 100644
>>>>> --- a/include/linux/hwmon.h
>>>>> +++ b/include/linux/hwmon.h
>>>>> @@ -332,6 +332,7 @@ enum hwmon_pwm_attributes {
>>>>>       hwmon_pwm_enable,
>>>>>       hwmon_pwm_mode,
>>>>>       hwmon_pwm_freq,
>>>>> +    hwmon_pwm_fan_channel,
>>>>>       hwmon_pwm_auto_channels_temp,
>>>>>   };
>>>>>
>>>>> @@ -339,6 +340,7 @@ enum hwmon_pwm_attributes {
>>>>>   #define HWMON_PWM_ENABLE        BIT(hwmon_pwm_enable)
>>>>>   #define HWMON_PWM_MODE            BIT(hwmon_pwm_mode)
>>>>>   #define HWMON_PWM_FREQ            BIT(hwmon_pwm_freq)
>>>>> +#define HWMON_PWM_FAN_CHANNEL BIT(hwmon_pwm_fan_channel)
>>>>>   #define HWMON_PWM_AUTO_CHANNELS_TEMP
>>>>> BIT(hwmon_pwm_auto_channels_temp)
>>>>>
>>>>>   enum hwmon_intrusion_attributes {
>>>>> -- 
>>>>> 2.30.2
>>>>>
>>

Re: [PATCH 1/2] hwmon: Add pwmX_fan_channel attribute

[Armin Wolf]

Am 24.07.22 um 18:30 schrieb Guenter Roeck:

> On 7/24/22 09:06, Armin Wolf wrote:
> [ ... ]
>> It would be indeed better if userspace software like pwmconfig would
>> use an internal list containing the names of all hwmon chips for which
>> the pwm to fan mappings are known.
>> I will add a note to the documentation of dell-smm-hwmon about the
>> pwm to fan mapping so userspace software knows about this.
>
> That is effectively the fancontrol configuration file. pwmconfig is
> supposed to assist in determining how that configuration file should
> look like. Having some file added to pwmcontrol to determine how the
> fancontrol configuration file should look like seems a bit off-track.
>
> What you are talking about is really the idea of providing a number of
> sample configuration files with the fancontrol (or lm-sensors) package,
> similar to the various sensors.conf files. That doesn't belong into
> the hwmon kernel documentation. It should be part of the lm-sensors
> package.
>
> Guenter
>
With "note" i meant some small text for users to read,
not a config file for pwmconfig.

Armin Wolf

>> Sorry for bothering you.
>>
>> Armin Wolf
>>
>>>> In contrast, many firmware-based chips do know which pwm channel
>>>> output controls which
>>>> fan channel. One example might be the dell-smm-hwmon driver and the
>>>> gpio-fan driver.
>>>>
>>>> In this case, making the attribute RO would indeed make sense.
>>>>
>>>
>>> Unless the attribute is used to configure the chip, it does not make
>>> sense
>>> in the first place. Also note that gpio-fan is usually configured using
>>> devicetree properties, _and_ it only has a single set of fan/pwm
>>> properties,
>>> so a sysfs attribute would always return 1 and make make even less
>>> sense there.
>>>
>>> Guenter
>>>
>>>> Armin Wolf
>>>>
>>>>>> ---
>>>>>>   Documentation/ABI/testing/sysfs-class-hwmon | 8 ++++++++
>>>>>>   Documentation/hwmon/sysfs-interface.rst     | 3 +++
>>>>>>   drivers/hwmon/hwmon.c                       | 1 +
>>>>>>   include/linux/hwmon.h                       | 2 ++
>>>>>>   4 files changed, 14 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/ABI/testing/sysfs-class-hwmon
>>>>>> b/Documentation/ABI/testing/sysfs-class-hwmon
>>>>>> index 7271781a23b2..f3d653bcf736 100644
>>>>>> --- a/Documentation/ABI/testing/sysfs-class-hwmon
>>>>>> +++ b/Documentation/ABI/testing/sysfs-class-hwmon
>>>>>> @@ -315,6 +315,14 @@ Description:
>>>>>>
>>>>>>           RW
>>>>>>
>>>>>> +What:        /sys/class/hwmon/hwmonX/pwmY_fan_channel
>>>>>> +Description:
>>>>>> +        Select which fan channel is controlled by this PWM output.
>>>>>> +
>>>>>> +        Valid fan channel/PWM output combinations are
>>>>>> chip-dependent.
>>>>>> +
>>>>>> +        RW
>>>>>> +
>>>>>>   What: /sys/class/hwmon/hwmonX/pwmY_auto_channels_temp
>>>>>>   Description:
>>>>>>           Select which temperature channels affect this PWM
>>>>>> output in
>>>>>> diff --git a/Documentation/hwmon/sysfs-interface.rst
>>>>>> b/Documentation/hwmon/sysfs-interface.rst
>>>>>> index 209626fb2405..17fcec03d3c5 100644
>>>>>> --- a/Documentation/hwmon/sysfs-interface.rst
>>>>>> +++ b/Documentation/hwmon/sysfs-interface.rst
>>>>>> @@ -209,6 +209,9 @@ PWM
>>>>>>   `pwm[1-*]_freq`
>>>>>>           Base PWM frequency in Hz.
>>>>>>
>>>>>> +`pwm[1-*]_fan_channel`
>>>>>> +                Select which fan channel is controlled by this PWM
>>>>>> output.
>>>>>> +
>>>>>>   `pwm[1-*]_auto_channels_temp`
>>>>>>           Select which temperature channels affect this PWM
>>>>>> output in
>>>>>>           auto mode.
>>>>>> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
>>>>>> index 2e2cd79d89eb..8c2d7574c461 100644
>>>>>> --- a/drivers/hwmon/hwmon.c
>>>>>> +++ b/drivers/hwmon/hwmon.c
>>>>>> @@ -604,6 +604,7 @@ static const char * const
>>>>>> hwmon_pwm_attr_templates[] = {
>>>>>>       [hwmon_pwm_enable] = "pwm%d_enable",
>>>>>>       [hwmon_pwm_mode] = "pwm%d_mode",
>>>>>>       [hwmon_pwm_freq] = "pwm%d_freq",
>>>>>> +    [hwmon_pwm_fan_channel] = "pwm%d_fan_channel",
>>>>>>       [hwmon_pwm_auto_channels_temp] = "pwm%d_auto_channels_temp",
>>>>>>   };
>>>>>>
>>>>>> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
>>>>>> index 14325f93c6b2..9d40cc1e520f 100644
>>>>>> --- a/include/linux/hwmon.h
>>>>>> +++ b/include/linux/hwmon.h
>>>>>> @@ -332,6 +332,7 @@ enum hwmon_pwm_attributes {
>>>>>>       hwmon_pwm_enable,
>>>>>>       hwmon_pwm_mode,
>>>>>>       hwmon_pwm_freq,
>>>>>> +    hwmon_pwm_fan_channel,
>>>>>>       hwmon_pwm_auto_channels_temp,
>>>>>>   };
>>>>>>
>>>>>> @@ -339,6 +340,7 @@ enum hwmon_pwm_attributes {
>>>>>>   #define HWMON_PWM_ENABLE        BIT(hwmon_pwm_enable)
>>>>>>   #define HWMON_PWM_MODE            BIT(hwmon_pwm_mode)
>>>>>>   #define HWMON_PWM_FREQ            BIT(hwmon_pwm_freq)
>>>>>> +#define HWMON_PWM_FAN_CHANNEL BIT(hwmon_pwm_fan_channel)
>>>>>>   #define HWMON_PWM_AUTO_CHANNELS_TEMP
>>>>>> BIT(hwmon_pwm_auto_channels_temp)
>>>>>>
>>>>>>   enum hwmon_intrusion_attributes {
>>>>>> --
>>>>>> 2.30.2
>>>>>>
>>>
>