Re: [PATCH v2] hwmon: (adm1177) fix sysfs ABI violation and current unit conversion

[Nuno Sá]

On Wed, 2026-03-25 at 05:13 +0000, Pradhan, Sanman wrote:
> From: Sanman Pradhan <psanman@xxxxxxxxxxx>
> 
> The adm1177 driver exposes the current alert threshold through
> hwmon_curr_max_alarm. This violates the hwmon sysfs ABI, where
> *_alarm attributes are read-only status flags and writable thresholds
> must use currN_max.
> 
> The driver also stores the threshold internally in microamps, while
> currN_max is defined in milliamps. Convert the threshold accordingly
> on both the read and write paths.
> 
> Widen the cached threshold and related calculations to 64 bits so
> that small shunt resistor values do not cause truncation or overflow.
> Also use 64-bit arithmetic for the mA/uA conversions, clamp writes
> to the range the hardware can represent, and propagate failures from
> adm1177_write_alert_thr() instead of silently ignoring them.
> 
> Update the hwmon documentation to reflect the attribute rename and
> the correct units returned by the driver.
> 
> Fixes: 09b08ac9e8d5 ("hwmon: (adm1177) Add ADM1177 Hot Swap Controller and Digital Power Monitor
> driver")
> Signed-off-by: Sanman Pradhan <psanman@xxxxxxxxxxx>
> ---

Arghh, just saw v2 now (and replied to v1). Seems AI still has some feedback [1] (even though
not strictly related to this patch). For reference, my comments [2]:

Anyways, as stated in my comment, after addressing the remaining "complain":

Acked-by: Nuno Sá <nuno.sa@xxxxxxxxxx>

[1]: https://sashiko.dev/#/patchset/20260325051246.28262-1-sanman.pradhan%40hpe.com
[2]: https://lore.kernel.org/linux-hwmon/f7069532401720fda1ca6e70b72742526fc79dec.camel@xxxxxxxxx/T/#t

- Nuno Sá

> v2:
> - Widen alert_threshold_ua to u64 throughout; use div_u64() and
>   (u64) casts to prevent overflow on read, write, and probe paths.
> - Update Documentation/hwmon/adm1177.rst for the attribute rename
>   and correct unit descriptions.
> 
> v1:
> - Rename hwmon_curr_max_alarm to hwmon_curr_max; add uA-to-mA and
>   mA-to-uA conversions with clamp_val on write path.
> - Propagate adm1177_write_alert_thr() return value on sysfs write;
>   add linux/math64.h and linux/minmax.h includes.
> ---
>  Documentation/hwmon/adm1177.rst |  8 ++---
>  drivers/hwmon/adm1177.c         | 54 +++++++++++++++++++--------------
>  2 files changed, 35 insertions(+), 27 deletions(-)
> 
> diff --git a/Documentation/hwmon/adm1177.rst b/Documentation/hwmon/adm1177.rst
> index 1c85a2af92bf7..375f6d6e03a7d 100644
> --- a/Documentation/hwmon/adm1177.rst
> +++ b/Documentation/hwmon/adm1177.rst
> @@ -27,10 +27,10 @@ for details.
>  Sysfs entries
>  -------------
>  
> -The following attributes are supported. Current maxim attribute
> +The following attributes are supported. Current maximum attribute
>  is read-write, all other attributes are read-only.
>  
> -in0_input		Measured voltage in microvolts.
> +in0_input		Measured voltage in millivolts.
>  
> -curr1_input		Measured current in microamperes.
> -curr1_max_alarm		Overcurrent alarm in microamperes.
> +curr1_input		Measured current in milliamperes.
> +curr1_max		Overcurrent shutdown threshold in milliamperes.
> diff --git a/drivers/hwmon/adm1177.c b/drivers/hwmon/adm1177.c
> index 8b2c965480e3f..7888afe8dafd6 100644
> --- a/drivers/hwmon/adm1177.c
> +++ b/drivers/hwmon/adm1177.c
> @@ -10,6 +10,8 @@
>  #include <linux/hwmon.h>
>  #include <linux/i2c.h>
>  #include <linux/init.h>
> +#include <linux/math64.h>
> +#include <linux/minmax.h>
>  #include <linux/module.h>
>  #include <linux/regulator/consumer.h>
>  
> @@ -33,7 +35,7 @@
>  struct adm1177_state {
>  	struct i2c_client	*client;
>  	u32			r_sense_uohm;
> -	u32			alert_threshold_ua;
> +	u64			alert_threshold_ua;
>  	bool			vrange_high;
>  };
>  
> @@ -48,7 +50,7 @@ static int adm1177_write_cmd(struct adm1177_state *st, u8 cmd)
>  }
>  
>  static int adm1177_write_alert_thr(struct adm1177_state *st,
> -				   u32 alert_threshold_ua)
> +				   u64 alert_threshold_ua)
>  {
>  	u64 val;
>  	int ret;
> @@ -91,8 +93,8 @@ static int adm1177_read(struct device *dev, enum hwmon_sensor_types type,
>  			*val = div_u64((105840000ull * dummy),
>  				       4096 * st->r_sense_uohm);
>  			return 0;
> -		case hwmon_curr_max_alarm:
> -			*val = st->alert_threshold_ua;
> +		case hwmon_curr_max:
> +			*val = div_u64(st->alert_threshold_ua, 1000);
>  			return 0;
>  		default:
>  			return -EOPNOTSUPP;
> @@ -126,9 +128,10 @@ static int adm1177_write(struct device *dev, enum hwmon_sensor_types type,
>  	switch (type) {
>  	case hwmon_curr:
>  		switch (attr) {
> -		case hwmon_curr_max_alarm:
> -			adm1177_write_alert_thr(st, val);
> -			return 0;
> +		case hwmon_curr_max:
> +			val = clamp_val(val, 0,
> +					div_u64(105840000ULL, st->r_sense_uohm));
> +			return adm1177_write_alert_thr(st, (u64)val * 1000);
>  		default:
>  			return -EOPNOTSUPP;
>  		}
> @@ -156,7 +159,7 @@ static umode_t adm1177_is_visible(const void *data,
>  			if (st->r_sense_uohm)
>  				return 0444;
>  			return 0;
> -		case hwmon_curr_max_alarm:
> +		case hwmon_curr_max:
>  			if (st->r_sense_uohm)
>  				return 0644;
>  			return 0;
> @@ -170,7 +173,7 @@ static umode_t adm1177_is_visible(const void *data,
>  
>  static const struct hwmon_channel_info * const adm1177_info[] = {
>  	HWMON_CHANNEL_INFO(curr,
> -			   HWMON_C_INPUT | HWMON_C_MAX_ALARM),
> +			   HWMON_C_INPUT | HWMON_C_MAX),
>  	HWMON_CHANNEL_INFO(in,
>  			   HWMON_I_INPUT),
>  	NULL
> @@ -192,7 +195,8 @@ static int adm1177_probe(struct i2c_client *client)
>  	struct device *dev = &client->dev;
>  	struct device *hwmon_dev;
>  	struct adm1177_state *st;
> -	u32 alert_threshold_ua;
> +	u64 alert_threshold_ua;
> +	u32 prop;
>  	int ret;
>  
>  	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> @@ -208,22 +212,26 @@ static int adm1177_probe(struct i2c_client *client)
>  	if (device_property_read_u32(dev, "shunt-resistor-micro-ohms",
>  				     &st->r_sense_uohm))
>  		st->r_sense_uohm = 0;
> -	if (device_property_read_u32(dev, "adi,shutdown-threshold-microamp",
> -				     &alert_threshold_ua)) {
> -		if (st->r_sense_uohm)
> -			/*
> -			 * set maximum default value from datasheet based on
> -			 * shunt-resistor
> -			 */
> -			alert_threshold_ua = div_u64(105840000000,
> -						     st->r_sense_uohm);
> -		else
> -			alert_threshold_ua = 0;
> +	if (!device_property_read_u32(dev, "adi,shutdown-threshold-microamp",
> +				      &prop)) {
> +		alert_threshold_ua = prop;
> +	} else if (st->r_sense_uohm) {
> +		/*
> +		 * set maximum default value from datasheet based on
> +		 * shunt-resistor
> +		 */
> +		alert_threshold_ua = div_u64(105840000000ULL,
> +					     st->r_sense_uohm);
> +	} else {
> +		alert_threshold_ua = 0;
>  	}
>  	st->vrange_high = device_property_read_bool(dev,
>  						    "adi,vrange-high-enable");
> -	if (alert_threshold_ua && st->r_sense_uohm)
> -		adm1177_write_alert_thr(st, alert_threshold_ua);
> +	if (alert_threshold_ua && st->r_sense_uohm) {
> +		ret = adm1177_write_alert_thr(st, alert_threshold_ua);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	ret = adm1177_write_cmd(st, ADM1177_CMD_V_CONT |
>  				    ADM1177_CMD_I_CONT |