Re: [RFT PATCH] hwmon: Add support for currX_emergency and inX_emergency attributes

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing `_alarm` attributes for new emergency limits.
- [Low] Orphaned duplicate `inY_input` entry in sysfs ABI documentation.
--

--- Patch [1]: [RFT PATCH] hwmon: Add support for currX_emergency and inX_emergency attributes ---
commit d42d90a3ffe54304e60c7c4aa212e4dd46adefe6
Author: Guenter Roeck <[email protected]>

hwmon: Add support for currX_emergency and inX_emergency attributes

This commit introduces support for currX_emergency and inX_emergency attributes
in the hardware monitoring subsystem. It updates the sysfs ABI documentation
and adds the new attributes to the hwmon core templates and enums.

> diff --git a/Documentation/ABI/testing/sysfs-class-hwmon b/Documentation/ABI/testing/sysfs-class-hwmon
> index b185bdfc7186a..42fd89305101a 100644
> --- a/Documentation/ABI/testing/sysfs-class-hwmon
> +++ b/Documentation/ABI/testing/sysfs-class-hwmon

[ ... ]

> @@ -647,6 +659,20 @@ Description:
>  
>  		RW
>  
> +What:		/sys/class/hwmon/hwmonX/currY_emergency
> +Description:
> +		Current emergency high value.
> +
> +		Unit: milliampere
> +
> +		RW
> +
> +		If a current reaches or exceeds this limit, the system is
> +		expected to take drastic action such as immediate power down
> +		or reset. At the very least, it should report a fault.
> +
> +What:		/sys/class/hwmon/hwmonX/inY_input
> +
>  What:		/sys/class/hwmon/hwmonX/currY_input
>  Description:
>  		Current input value

[Severity: Low]
This isn't a bug, but should this What: /sys/class/hwmon/hwmonX/inY_input
line be here? 

It appears to be an orphaned duplicate entry.

> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 55a9a3ddd4aac..a07bfe7ac7dbf 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c

[ ... ]

> @@ -621,6 +621,7 @@ static const char * const hwmon_in_attr_templates[] = {
>  	[hwmon_in_max] = "in%d_max",
>  	[hwmon_in_lcrit] = "in%d_lcrit",
>  	[hwmon_in_crit] = "in%d_crit",
> +	[hwmon_in_emergency] = "in%d_emergency",
>  	[hwmon_in_average] = "in%d_average",
>  	[hwmon_in_lowest] = "in%d_lowest",
>  	[hwmon_in_highest] = "in%d_highest",

[Severity: Medium]
Are we missing the string definition for in%d_emergency_alarm here?

Without hwmon_in_emergency_alarm in the hwmon_in_attr_templates array,
drivers will not be able to use the hwmon_device_register_with_info API
to expose the voltage emergency alarm status.

[ ... ]

> @@ -644,6 +645,7 @@ static const char * const hwmon_curr_attr_templates[] = {
>  	[hwmon_curr_max] = "curr%d_max",
>  	[hwmon_curr_lcrit] = "curr%d_lcrit",
>  	[hwmon_curr_crit] = "curr%d_crit",
> +	[hwmon_curr_emergency] = "curr%d_emergency",
>  	[hwmon_curr_average] = "curr%d_average",
>  	[hwmon_curr_lowest] = "curr%d_lowest",
>  	[hwmon_curr_highest] = "curr%d_highest",

[Severity: Medium]
Similarly, does hwmon_curr_attr_templates also need a string definition
for curr%d_emergency_alarm?

> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
> index 77a6f2bffcba7..2cd433966723e 100644
> --- a/include/linux/hwmon.h
> +++ b/include/linux/hwmon.h
> @@ -134,6 +134,7 @@ enum hwmon_in_attributes {
>  	hwmon_in_max,
>  	hwmon_in_lcrit,
>  	hwmon_in_crit,
> +	hwmon_in_emergency,
>  	hwmon_in_average,
>  	hwmon_in_lowest,
>  	hwmon_in_highest,

[Severity: Medium]
Should hwmon_in_emergency_alarm be declared in enum hwmon_in_attributes
alongside the other voltage alarm attributes to complete the API feature?

[ ... ]

> @@ -178,6 +180,7 @@ enum hwmon_curr_attributes {
>  	hwmon_curr_max,
>  	hwmon_curr_lcrit,
>  	hwmon_curr_crit,
> +	hwmon_curr_emergency,
>  	hwmon_curr_average,
>  	hwmon_curr_lowest,
>  	hwmon_curr_highest,

[Severity: Medium]
Does enum hwmon_curr_attributes also need hwmon_curr_emergency_alarm declared
to allow exporting current emergency alarm statuses?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1