[PATCH 0/1] clarify intent of fan min and max attributes

[PATCH 0/1] clarify intent of fan min and max attributes

[Ivor Wanders]

In [1] I got the feedback that fan min and max attributes are intended for
writing to the device and not merely providing constants to userspace.
This patch clarifies this intent in the documentation such that future
contributers don't make incorrect assumptions about them.

[1]: https://lore.kernel.org/linux-hwmon/ab8a1ff3-6d01-4331-ba5d-d677d1ad80b5@roeck-us.net/

Ivor Wanders (1):
  hwmon: clarify intent of fan min/max

 Documentation/hwmon/submitting-patches.rst |  4 +++-
 Documentation/hwmon/sysfs-interface.rst    | 12 +++++++++---
 2 files changed, 12 insertions(+), 4 deletions(-)

-- 
2.17.1

[PATCH 1/1] hwmon: clarify intent of fan min/max

[Ivor Wanders]

This adds a link to the hwmon sysfs attributes in the hwmon patch
submission bullet points. It also adds an explanation denoting the
intent of the fan min and max attributes.

Signed-off-by: Ivor Wanders <ivor@iwanders.net>
---
 Documentation/hwmon/submitting-patches.rst |  4 +++-
 Documentation/hwmon/sysfs-interface.rst    | 12 +++++++++---
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/Documentation/hwmon/submitting-patches.rst b/Documentation/hwmon/submitting-patches.rst
index 6482c4f13..d634c41d7 100644
--- a/Documentation/hwmon/submitting-patches.rst
+++ b/Documentation/hwmon/submitting-patches.rst
@@ -141,7 +141,9 @@ increase the chances of your change being accepted.
 
 * When deciding which sysfs attributes to support, look at the chip's
   capabilities. While we do not expect your driver to support everything the
-  chip may offer, it should at least support all limits and alarms.
+  chip may offer, it should at least support all limits and alarms. Only
+  add attributes that follow the intent of the attributes, as described in
+  Documentation/hwmon/sysfs-interface.rst.
 
 * Last but not least, please check if a driver for your chip already exists
   before starting to write a new driver. Especially for temperature sensors,
diff --git a/Documentation/hwmon/sysfs-interface.rst b/Documentation/hwmon/sysfs-interface.rst
index f76e9f8cc..72dd5e02d 100644
--- a/Documentation/hwmon/sysfs-interface.rst
+++ b/Documentation/hwmon/sysfs-interface.rst
@@ -167,13 +167,19 @@ Fans
 ****
 
 `fan[1-*]_min`
-		Fan minimum value
+		Fan minimum value, this is intended as a way to specify
+		the desired minimum speed to the device if the device
+		supports that. It is not intended for communicating
+		a constant that denotes the lowest possible fan speed.
 
 `fan[1-*]_max`
-		Fan maximum value
+		Fan maximum value, this is intended as a way to specify
+		the desired maximum speed to the device if the device
+		supports that. It is not intended for communicating
+		a constant that denotes the highest possible fan speed.
 
 `fan[1-*]_input`
-		Fan input value.
+		Fan input value, this is the fan's current speed.
 
 `fan[1-*]_div`
 		Fan divisor.
-- 
2.17.1

Re: [PATCH 1/1] hwmon: clarify intent of fan min/max

[Randy Dunlap]

Hi,

On 12/21/23 14:51, Ivor Wanders wrote:
> This adds a link to the hwmon sysfs attributes in the hwmon patch
> submission bullet points. It also adds an explanation denoting the
> intent of the fan min and max attributes.
> 
> Signed-off-by: Ivor Wanders <ivor@iwanders.net>
> ---
>  Documentation/hwmon/submitting-patches.rst |  4 +++-
>  Documentation/hwmon/sysfs-interface.rst    | 12 +++++++++---
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 


> diff --git a/Documentation/hwmon/sysfs-interface.rst b/Documentation/hwmon/sysfs-interface.rst
> index f76e9f8cc..72dd5e02d 100644
> --- a/Documentation/hwmon/sysfs-interface.rst
> +++ b/Documentation/hwmon/sysfs-interface.rst
> @@ -167,13 +167,19 @@ Fans
>  ****
>  
>  `fan[1-*]_min`
> -		Fan minimum value
> +		Fan minimum value, this is intended as a way to specify

Please change the comma here to either (a) semi-colon (';') or
(b) a period ('.') followed by a sentence beginning with a capital letter.

Same for the other 2 below also.

> +		the desired minimum speed to the device if the device
> +		supports that. It is not intended for communicating
> +		a constant that denotes the lowest possible fan speed.
>  
>  `fan[1-*]_max`
> -		Fan maximum value
> +		Fan maximum value, this is intended as a way to specify
> +		the desired maximum speed to the device if the device
> +		supports that. It is not intended for communicating
> +		a constant that denotes the highest possible fan speed.
>  
>  `fan[1-*]_input`
> -		Fan input value.
> +		Fan input value, this is the fan's current speed.
>  
>  `fan[1-*]_div`
>  		Fan divisor.

thanks.
-- 
#Randy
https://people.kernel.org/tglx/notes-about-netiquette
https://subspace.kernel.org/etiquette.html

[PATCH v2] hwmon: clarify intent of fan min/max

[Ivor Wanders]

This adds a link to the hwmon sysfs attributes in the hwmon patch
submission bullet points. It also adds an explanation denoting the
intent of the fan min and max attributes.

Signed-off-by: Ivor Wanders <ivor@iwanders.net>
---
 Documentation/hwmon/submitting-patches.rst |  4 +++-
 Documentation/hwmon/sysfs-interface.rst    | 12 +++++++++---
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/Documentation/hwmon/submitting-patches.rst b/Documentation/hwmon/submitting-patches.rst
index 6482c4f13..d634c41d7 100644
--- a/Documentation/hwmon/submitting-patches.rst
+++ b/Documentation/hwmon/submitting-patches.rst
@@ -141,7 +141,9 @@ increase the chances of your change being accepted.
 
 * When deciding which sysfs attributes to support, look at the chip's
   capabilities. While we do not expect your driver to support everything the
-  chip may offer, it should at least support all limits and alarms.
+  chip may offer, it should at least support all limits and alarms. Only
+  add attributes that follow the intent of the attributes, as described in
+  Documentation/hwmon/sysfs-interface.rst.
 
 * Last but not least, please check if a driver for your chip already exists
   before starting to write a new driver. Especially for temperature sensors,
diff --git a/Documentation/hwmon/sysfs-interface.rst b/Documentation/hwmon/sysfs-interface.rst
index f76e9f8cc..edfde3b13 100644
--- a/Documentation/hwmon/sysfs-interface.rst
+++ b/Documentation/hwmon/sysfs-interface.rst
@@ -167,13 +167,19 @@ Fans
 ****
 
 `fan[1-*]_min`
-		Fan minimum value
+		Fan minimum value. This is intended as a way to specify
+		the desired minimum speed to the device if the device
+		supports that. It is not intended for communicating
+		a constant that denotes the lowest possible fan speed.
 
 `fan[1-*]_max`
-		Fan maximum value
+		Fan maximum value. This is intended as a way to specify
+		the desired maximum speed to the device if the device
+		supports that. It is not intended for communicating
+		a constant that denotes the highest possible fan speed.
 
 `fan[1-*]_input`
-		Fan input value.
+		Fan input value. This is the fan's current speed.
 
 `fan[1-*]_div`
 		Fan divisor.
-- 
2.17.1

Re: [PATCH v2] hwmon: clarify intent of fan min/max

[Randy Dunlap]

On 12/21/23 15:31, Ivor Wanders wrote:
> This adds a link to the hwmon sysfs attributes in the hwmon patch
> submission bullet points. It also adds an explanation denoting the
> intent of the fan min and max attributes.
> 
> Signed-off-by: Ivor Wanders <ivor@iwanders.net>

LGTM. Thanks.
(other than telling us what changed from v1 to v2)

Acked-by: Randy Dunlap <rdunlap@infradead.org>

> ---
>  Documentation/hwmon/submitting-patches.rst |  4 +++-
>  Documentation/hwmon/sysfs-interface.rst    | 12 +++++++++---
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 


-- 
#Randy
https://people.kernel.org/tglx/notes-about-netiquette
https://subspace.kernel.org/etiquette.html

Re: [PATCH v2] hwmon: clarify intent of fan min/max

[Ivor Wanders]

> (other than telling us what changed from v1 to v2)

Appreciate the feedback! I'm new to this, so basically it means always
use --cover-letter, I also noticed that I should have probably added some
'to' entries, the command suggested by lore moved the original 'to' fields
to 'cc'. I'll be more diligent with the changelog in future contributions.

Change was incorporating the feedback from [1], changing the comma into a
period for all three changed sysfs entries.

~Ivor

[1]: https://lore.kernel.org/linux-hwmon/40285311-8adc-4ca9-86ce-27c8b723a102@infradead.org/

Re: [PATCH 0/1] clarify intent of fan min and max attributes

[Guenter Roeck]

On Thu, Dec 21, 2023 at 05:51:48PM -0500, Ivor Wanders wrote:
> In [1] I got the feedback that fan min and max attributes are intended for
> writing to the device and not merely providing constants to userspace.
> This patch clarifies this intent in the documentation such that future
> contributers don't make incorrect assumptions about them.
> 

Documentation/hwmon/sysfs-interface already states:

"
All entries (except name) are optional, and should only be created in a
given driver if the chip has the feature.
"

I do not see the point of clarifying this for individual attributes,
especially since that might create the impression that it would possibly
not apply to other attributes (for those not reading the above
documentation).

Guenter

Re: [PATCH v2] hwmon: clarify intent of fan min/max

[Guenter Roeck]

On Thu, Dec 21, 2023 at 06:31:33PM -0500, Ivor Wanders wrote:
> This adds a link to the hwmon sysfs attributes in the hwmon patch
> submission bullet points. It also adds an explanation denoting the
> intent of the fan min and max attributes.
> 

NACK, because that text would be required for _all_ hwmon sysfs
attributes which simply does not make sense to me.

Guenter

Re: [PATCH 0/1] clarify intent of fan min and max attributes

[Ivor Wanders]

> I do not see the point of clarifying this for individual attributes,
> especially since that might create the impression that it would possibly
> not apply to other attributes (for those not reading the above
> documentation).

Okay, fair point, I can see that lead to more confusion indeed.

~Ivor

Re: [PATCH v2] hwmon: clarify intent of fan min/max

[Ivor Wanders]

> NACK, because that text would be required for _all_ hwmon sysfs
> attributes which simply does not make sense to me.

Fair enough, thank you for your time and comments.

~Ivor