To help summon the beautifully documented kernel of the future
promised by the move to ReST, this series revises the hwmon subsystem
documentation to make it more comprehensive and internally consistent.
The latter goal also requires a few API and ABI additions that should
have been in tree but weren't.
James Seo (11):
Documentation/hwmon: Move misplaced entry in hwmon docs index
hwmon: (core) Rename last parameter of devm_hwmon_register_with_info()
hwmon: (core) Revise kerneldoc comments
Documentation/hwmon: Revise hwmon kernel API reference
Documentation/hwmon: Revise PMBus core documentation
Documentation/hwmon: Revise patch submission checklist
Documentation/hwmon: Revise sysfs interface specification
Documentation/hwmon: Revise userspace tools documentation
ABI: sysfs-class-hwmon: Revise hwmon ABI documentation
hwmon: (core) Add missing beep-related standard attributes
ABI: sysfs-class-hwmon: Add missing hwmon standard attributes
Documentation/ABI/testing/sysfs-class-hwmon | 901 +++++++++++++-----
Documentation/hwmon/hwmon-kernel-api.rst | 740 ++++++++-------
Documentation/hwmon/index.rst | 2 +-
Documentation/hwmon/pmbus-core.rst | 348 ++++---
Documentation/hwmon/submitting-patches.rst | 110 +--
Documentation/hwmon/sysfs-interface.rst | 984 +++++++++-----------
Documentation/hwmon/userspace-tools.rst | 129 ++-
drivers/hwmon/hwmon.c | 169 ++--
include/linux/hwmon-sysfs.h | 19 +-
include/linux/hwmon.h | 90 +-
10 files changed, 2037 insertions(+), 1455 deletions(-)
base-commit: 1a5304fecee523060f26e2778d9d8e33c0562df3
--
2.34.1
Move the entry for the inspur-ipsps1 driver so that it no
longer appears in the hwmon docs TOC as a document relating
to the hwmon subsystem itself.
Signed-off-by: James Seo <[email protected]>
---
Documentation/hwmon/index.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index fa1208c62855..03b30a94a9e6 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -9,7 +9,6 @@ Hardware Monitoring
hwmon-kernel-api
pmbus-core
- inspur-ipsps1
submitting-patches
sysfs-interface
userspace-tools
@@ -85,6 +84,7 @@ Hardware Monitoring Kernel Drivers
ina2xx
ina238
ina3221
+ inspur-ipsps1
intel-m10-bmc-hwmon
ir35221
ir38064
--
2.34.1
Change the name of the groups parameter of
devm_hwmon_device_register_with_info() to extra_groups to
match the name given by the hwmon API reference and in
hwmon_device_register_with_info().
Signed-off-by: James Seo <[email protected]>
---
drivers/hwmon/hwmon.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 573b83b6c08c..5f205686065e 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -1029,7 +1029,7 @@ EXPORT_SYMBOL_GPL(devm_hwmon_device_register_with_groups);
* @name: hwmon name attribute
* @drvdata: driver data to attach to created device
* @chip: pointer to hwmon chip information
- * @groups: pointer to list of driver specific attribute groups
+ * @extra_groups: pointer to list of driver specific attribute groups
*
* Returns the pointer to the new device. The new device is automatically
* unregistered with the parent device.
@@ -1038,7 +1038,7 @@ struct device *
devm_hwmon_device_register_with_info(struct device *dev, const char *name,
void *drvdata,
const struct hwmon_chip_info *chip,
- const struct attribute_group **groups)
+ const struct attribute_group **extra_groups)
{
struct device **ptr, *hwdev;
@@ -1050,7 +1050,7 @@ devm_hwmon_device_register_with_info(struct device *dev, const char *name,
return ERR_PTR(-ENOMEM);
hwdev = hwmon_device_register_with_info(dev, name, drvdata, chip,
- groups);
+ extra_groups);
if (IS_ERR(hwdev))
goto error;
--
2.34.1
Add definitions for the following non-deprecated standard
attributes implemented in the API and mentioned in the sysfs
interface spec but not found in the ABI documentation.
Alarm attributes:
* inY_alarm, inY_min_alarm, inY_max_alarm,
inY_lcrit_alarm, inY_crit_alarm
* currY_alarm, currY_min_alarm, currY_max_alarm,
currY_lcrit_alarm, currY_crit_alarm
* powerY_alarm, powerY_cap_alarm,
powerY_max_alarm, powerY_crit_alarm
* fanY_alarm, fanY_min_alarm, fanY_max_alarm
* tempY_alarm, tempY_min_alarm, tempY_max_alarm,
tempY_lcrit_alarm, tempY_emergency_alarm
(tempY_crit_alarm already existed)
Beep attributes:
beep_enable, inY_beep, currY_beep, fanY_beep, tempY_beep
Sample average attributes:
samples, in_samples, power_samples, temp_samples, curr_samples
Fault attributes:
tempY_fault (fanY_fault already existed)
Signed-off-by: James Seo <[email protected]>
---
Documentation/ABI/testing/sysfs-class-hwmon | 358 +++++++++++++++++++-
1 file changed, 344 insertions(+), 14 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-class-hwmon b/Documentation/ABI/testing/sysfs-class-hwmon
index 7fc914bc70e2..2f6884874812 100644
--- a/Documentation/ABI/testing/sysfs-class-hwmon
+++ b/Documentation/ABI/testing/sysfs-class-hwmon
@@ -33,6 +33,23 @@ Description:
RW
+What: /sys/class/hwmon/hwmonX/beep_enable
+Description:
+ Enable or disable beeps.
+
+ - 0: No beeps
+ - 1: Beeps
+
+ RW
+
+What: /sys/class/hwmon/hwmonX/samples
+Description:
+ Samples in calculated average.
+
+ Applies to all types of channels.
+
+ RW
+
What: /sys/class/hwmon/hwmonX/inY_min
Description:
Voltage min value.
@@ -194,6 +211,76 @@ Description:
RO
+What: /sys/class/hwmon/hwmonX/inY_alarm
+Description:
+ Voltage channel alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/inY_min_alarm
+Description:
+ Voltage min alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/inY_max_alarm
+Description:
+ Voltage max alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/inY_lcrit_alarm
+Description:
+ Voltage critical min alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/inY_crit_alarm
+Description:
+ Voltage critical max alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/inY_beep
+Description:
+ Voltage channel beep.
+
+ - 0: Disable
+ - 1: Enable
+
+ RW
+
+What: /sys/class/hwmon/hwmonX/in_samples
+Description:
+ Voltage samples in calculated average.
+
+ RW
+
What: /sys/class/hwmon/hwmonX/fanY_min
Description:
Fan minimum value.
@@ -283,6 +370,48 @@ Description:
RW
+What: /sys/class/hwmon/hwmonX/fanY_alarm
+Description:
+ Fan channel alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/fanY_min_alarm
+Description:
+ Fan min alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/fanY_max_alarm
+Description:
+ Fan max alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/fanY_beep
+Description:
+ Fan channel beep.
+
+ - 0: Disable
+ - 1: Enable
+
+ RW
+
What: /sys/class/hwmon/hwmonX/fanY_fault
Description:
Fan channel fault indicator.
@@ -468,20 +597,6 @@ Description:
RW
-What: /sys/class/hwmon/hwmonX/tempY_crit_alarm
-Description:
- Temperature critical max alarm indicator.
-
- Contrary to regular alarm flags which clear themselves
- automatically when read, this one sticks until cleared by
- the user. This is done by writing 0 to the file. Writing
- other values is unsupported.
-
- - 0: No alarm
- - 1: Alarm
-
- RW
-
What: /sys/class/hwmon/hwmonX/tempY_crit_hyst
Description:
Temperature hysteresis value for critical limit.
@@ -645,6 +760,101 @@ Description:
RO
+What: /sys/class/hwmon/hwmonX/tempY_alarm
+Description:
+ Temperature channel alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/tempY_min_alarm
+Description:
+ Temperature min alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/tempY_max_alarm
+Description:
+ Temperature max alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/tempY_lcrit_alarm
+Description:
+ Temperature critical min alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/tempY_crit_alarm
+Description:
+ Temperature critical max alarm indicator.
+
+ Contrary to regular alarm flags which clear themselves
+ automatically when read, this one sticks until cleared by
+ the user. This is done by writing 0 to the file. Writing
+ other values is unsupported.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RW
+
+What: /sys/class/hwmon/hwmonX/tempY_emergency_alarm
+Description:
+ Temperature emergency max alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/tempY_beep
+Description:
+ Temperature channel beep.
+
+ - 0: Disable
+ - 1: Enable
+
+ RW
+
+What: /sys/class/hwmon/hwmonX/tempY_fault
+Description:
+ Temperature channel fault indicator.
+
+ Indicates whether a temperature sensor has reported failure.
+
+ - 0: OK
+ - 1: Failed
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/temp_samples
+Description:
+ Temperature samples in calculated average.
+
+ RW
+
What: /sys/class/hwmon/hwmonX/currY_max
Description:
Current max value.
@@ -748,6 +958,76 @@ Description:
RO
+What: /sys/class/hwmon/hwmonX/currY_alarm
+Description:
+ Current channel alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/currY_min_alarm
+Description:
+ Current min alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/currY_max_alarm
+Description:
+ Current max alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/currY_lcrit_alarm
+Description:
+ Current critical min alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/currY_crit_alarm
+Description:
+ Current critical max alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/currY_beep
+Description:
+ Current channel beep.
+
+ - 0: Disable
+ - 1: Enable
+
+ RW
+
+What: /sys/class/hwmon/hwmonX/curr_samples
+Description:
+ Current samples in calculated average.
+
+ RW
+
What: /sys/class/hwmon/hwmonX/powerY_average
Description:
Average power use.
@@ -950,6 +1230,56 @@ Description:
RO
+What: /sys/class/hwmon/hwmonX/powerY_alarm
+Description:
+ Power channel alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/powerY_cap_alarm
+Description:
+ Power reduction alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/powerY_max_alarm
+Description:
+ Power max alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/powerY_crit_alarm
+Description:
+ Power critical reduction alarm indicator.
+
+ Clears itself when read.
+
+ - 0: No alarm
+ - 1: Alarm
+
+ RO
+
+What: /sys/class/hwmon/hwmonX/power_samples
+Description:
+ Power samples in calculated average.
+
+ RW
+
What: /sys/class/hwmon/hwmonX/energyY_input
Description:
Cumulative energy use.
--
2.34.1
On 5/4/23 00:57, James Seo wrote:
> Change the name of the groups parameter of
> devm_hwmon_device_register_with_info() to extra_groups to
> match the name given by the hwmon API reference and in
> hwmon_device_register_with_info().
>
> Signed-off-by: James Seo <[email protected]>
> ---
> drivers/hwmon/hwmon.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 573b83b6c08c..5f205686065e 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -1029,7 +1029,7 @@ EXPORT_SYMBOL_GPL(devm_hwmon_device_register_with_groups);
> * @name: hwmon name attribute
> * @drvdata: driver data to attach to created device
> * @chip: pointer to hwmon chip information
> - * @groups: pointer to list of driver specific attribute groups
> + * @extra_groups: pointer to list of driver specific attribute groups
> *
> * Returns the pointer to the new device. The new device is automatically
> * unregistered with the parent device.
> @@ -1038,7 +1038,7 @@ struct device *
> devm_hwmon_device_register_with_info(struct device *dev, const char *name,
> void *drvdata,
> const struct hwmon_chip_info *chip,
> - const struct attribute_group **groups)
> + const struct attribute_group **extra_groups)
Please please please no such changes. I don't want to have to deal with
patch wars just because people believe variables should have other names.
Such changes add zero value unless one counts wasted review time as a "value".
Guenter
> {
> struct device **ptr, *hwdev;
>
> @@ -1050,7 +1050,7 @@ devm_hwmon_device_register_with_info(struct device *dev, const char *name,
> return ERR_PTR(-ENOMEM);
>
> hwdev = hwmon_device_register_with_info(dev, name, drvdata, chip,
> - groups);
> + extra_groups);
> if (IS_ERR(hwdev))
> goto error;
>
On Thu, May 04, 2023 at 12:57:42AM -0700, James Seo wrote:
> Move the entry for the inspur-ipsps1 driver so that it no
> longer appears in the hwmon docs TOC as a document relating
> to the hwmon subsystem itself.
LGTM, thanks!
Reviewed-by: Bagas Sanjaya <[email protected]>
--
An old man doll... just what I always wanted! - Clara
On Thu, May 04, 2023 at 08:29:57AM -0700, Guenter Roeck wrote:
> Please please please no such changes. I don't want to have to deal with
> patch wars just because people believe variables should have other names.
>
> Such changes add zero value unless one counts wasted review time as a "value".
>
> Guenter
>
Hello,
Of course arbitrarily changing variable names is pointless. But this
patch fulfills the intent of 848ba0a2f20dc121a3ef5272a24641d2bd963d8b,
which makes this change for devm_hwmon_device_register_with_info() in
hwmon-kernel-api.txt and in hwmon.h - but not in hwmon.c. The same
commit makes the same change for hwmon_device_register_with_info() in
all three files, so it obviously should have been in tree already.
The other reason for this patch is that for the purpose of generating
function documentation from kerneldocs, it is not feasible to call
this parameter "extra_groups" in the kerneldoc and still call it
"groups" in the function itself. Doing so results in the lines
"const struct attribute_group **groups / undescribed" and no mention
of "extra_groups" in the generated document. Leaving things as is, so
that [devm_]hwmon_device_register_with_info() have different names
for this parameter, is potentially confusing and more noticeable to
to the eye than I would like once rendered.
Is this good enough to proceed? And as a subsystem maintainer, is
there anything else, specifically or in general, that you would like
to see addressed?
James
On 5/5/23 06:15, James Seo wrote:
> On Thu, May 04, 2023 at 08:29:57AM -0700, Guenter Roeck wrote:
>> Please please please no such changes. I don't want to have to deal with
>> patch wars just because people believe variables should have other names.
>>
>> Such changes add zero value unless one counts wasted review time as a "value".
>>
>> Guenter
>>
>
> Hello,
>
> Of course arbitrarily changing variable names is pointless. But this
> patch fulfills the intent of 848ba0a2f20dc121a3ef5272a24641d2bd963d8b,
> which makes this change for devm_hwmon_device_register_with_info() in
> hwmon-kernel-api.txt and in hwmon.h - but not in hwmon.c. The same
> commit makes the same change for hwmon_device_register_with_info() in
> all three files, so it obviously should have been in tree already.
>
> The other reason for this patch is that for the purpose of generating
> function documentation from kerneldocs, it is not feasible to call
> this parameter "extra_groups" in the kerneldoc and still call it
> "groups" in the function itself. Doing so results in the lines
> "const struct attribute_group **groups / undescribed" and no mention
> of "extra_groups" in the generated document. Leaving things as is, so
> that [devm_]hwmon_device_register_with_info() have different names
> for this parameter, is potentially confusing and more noticeable to
> to the eye than I would like once rendered.
>
> Is this good enough to proceed? And as a subsystem maintainer, is
> there anything else, specifically or in general, that you would like
Marginally. That should have been explained in more detail in the
description.
> to see addressed?
>
I don't know. There are changes which seem to be more based on POV
than real improvement (such as the removal of the credit from the
PMBus document). I'll have to verify each and every reference to determine
if the change is appropriate. Also, the changes are substantial.
It will take a lot of time for me to review, and right now I do not have
that time. I have a hard time keeping up with code reviews.
Guenter
On Fri, May 05, 2023 at 06:30:53AM -0700, Guenter Roeck wrote:
> On 5/5/23 06:15, James Seo wrote:
>> On Thu, May 04, 2023 at 08:29:57AM -0700, Guenter Roeck wrote:
>>
>> Hello,
>>
>> Of course arbitrarily changing variable names is pointless. But this
>> patch fulfills the intent of 848ba0a2f20dc121a3ef5272a24641d2bd963d8b,
>> which makes this change for devm_hwmon_device_register_with_info() in
>> hwmon-kernel-api.txt and in hwmon.h - but not in hwmon.c. The same
>> commit makes the same change for hwmon_device_register_with_info() in
>> all three files, so it obviously should have been in tree already.
>>
>> The other reason for this patch is that for the purpose of generating
>> function documentation from kerneldocs, it is not feasible to call
>> this parameter "extra_groups" in the kerneldoc and still call it
>> "groups" in the function itself. Doing so results in the lines
>> "const struct attribute_group **groups / undescribed" and no mention
>> of "extra_groups" in the generated document. Leaving things as is, so
>> that [devm_]hwmon_device_register_with_info() have different names
>> for this parameter, is potentially confusing and more noticeable to
>> to the eye than I would like once rendered.
>>
>> Is this good enough to proceed? And as a subsystem maintainer, is
>> there anything else, specifically or in general, that you would like
>
> Marginally. That should have been explained in more detail in the
> description.
OK, I will add more detail.
>
>> to see addressed?
>>
>
> I don't know. There are changes which seem to be more based on POV
> than real improvement (such as the removal of the credit from the
> PMBus document). I'll have to verify each and every reference to determine
> if the change is appropriate. Also, the changes are substantial.
Yes, sorry. At some point comparing a local make htmldocs build to
docs.kernel.org became much easier to follow than slogging through
diffs, and some of the markup only makes sense once rendered next to
the automatic cross-references that Sphinx adds.
> It will take a lot of time for me to review, and right now I do not have
> that time. I have a hard time keeping up with code reviews.
>
> Guenter
>
No rush. Your time is always appreciated. As I have gathered some
feedback from Bagas, I will submit the smaller changes as a PATCH
series in a day or two and an updated RFC series that you can
tackle at your leisure.
James
On Thu, May 04, 2023 at 12:57:42AM -0700, James Seo wrote:
> Move the entry for the inspur-ipsps1 driver so that it no
> longer appears in the hwmon docs TOC as a document relating
> to the hwmon subsystem itself.
>
> Signed-off-by: James Seo <[email protected]>
> Reviewed-by: Bagas Sanjaya <[email protected]>
Applied to hwmon-next.
Guenter
> ---
> Documentation/hwmon/index.rst | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index fa1208c62855..03b30a94a9e6 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -9,7 +9,6 @@ Hardware Monitoring
>
> hwmon-kernel-api
> pmbus-core
> - inspur-ipsps1
> submitting-patches
> sysfs-interface
> userspace-tools
> @@ -85,6 +84,7 @@ Hardware Monitoring Kernel Drivers
> ina2xx
> ina238
> ina3221
> + inspur-ipsps1
> intel-m10-bmc-hwmon
> ir35221
> ir38064