Re: [PATCH v5 0/8] hwmon: (ina3221) Various improvement and add support for SQ52210

[Guenter Roeck <linux@roeck-us.net>]

On Tue, Jan 27, 2026 at 10:07:50AM -0800, Guenter Roeck wrote:
> Hi,
> 
> On Mon, Jan 19, 2026 at 07:14:38AM -0500, Wenliang Yan wrote:
> > Make modifications according to the guidance provided in the reply.
> > 
> > 1.Modify the description for power[123]_input(PATCH 5).
> > 
> > 2.Re-annotate the significance of the limit value calculation
> > and the use of register masks in the sq52210_alert_limit_write.
> > Modify the calculation process to resolve arithmetic overflow issues.
> > The limit values SOL, BOL, and BUL are all stored using the upper
> > 13 bits of the register, so shifting is required. In contrast,
> > the POL value is configured by setting the lower three bits
> > to 0 directly(PATCH 6).
> > 
> > 3.Resolve arithmetic overflow issues in the ina3221_read_power(PATCH 7).
> > 
> > 4.Resolve arithmetic overflow issues in the ina3221_read_curr,
> > and validate channel indices in ina3221_write_in(PATCH 8).
> > 
> 
> AI review of the series provided the feedback below. Please fix or explain
> false positives.
> 

Another round of AI reviews below. Please address or explain.

Thanks,
Guenter

---
commit ef198f703062bc5ad58a148eb506821900c3522e
Author:     Wenliang Yan <wenliang202407@163.com>
hwmon: (ina3221) Support write/read functions for 'power' attribute

SQ52210 adds power attributes to report power data and implements
corresponding read/write functions for this purpose. This includes
reading power values, reading alert thresholds, reading alert
trigger status, and writing alert thresholds.

> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index cb292fee564b..2338b3288950 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -546,6 +553,12 @@ static int ina3221_read_power(struct device *dev, u32 attr, int channel, long *v
>  		if (ret)
>  			return ret;
>  		/* Return power in mW */
> -		*val = DIV_U64_ROUND_CLOSEST((u64)regval * (u64)ina->power_lsb_uW, 1000);
> +		*val = DIV_U64_ROUND_CLOSEST((u64)regval * (u64)ina->power_lsb_uW, 1000);
>  		return 0;

Does this return the power value in milliWatts? The hwmon ABI specifies
that power attributes should be returned in microWatts.

>  	case hwmon_power_crit_alarm:
>  		/* No actual register read if channel is disabled */
> @@ -736,6 +797,18 @@ static int ina3221_write_enable(struct device *dev, int channel, bool enable)
>  	return ret;
>  }
>  
> +static int ina3221_write_power(struct device *dev, u32 attr, int channel, long val)
> +{
> +	struct ina3221_data *ina = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_power_crit:
> +		return sq52210_alert_limit_write(ina, SQ52210_ALERT_POL, channel, val);

Does this pass the value in microWatts (as received from the core) to a
function that expects milliWatts? The sq52210_alert_limit_write()
implementation multiplies the input by 1000 to convert to microWatts,
which implies it expects the input in milliWatts.

> +	default:
> +		return 0;
> +	}
> +}
> +

commit 61e453283222e7fc9439a6d5c682ce1104dd24ef
Author:     Wenliang Yan <wenliang202407@163.com>
hwmon: (ina3221) Support for writing alert limit values and modify the 'ina3221_read_value' function

SQ52210 adds power, current, and limit registers. The ina3221_read_value
function has been refactored to adapt to the new register data reading
format.

> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index 4b2888be0ae2..cb292fee564b 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -345,6 +365,17 @@ static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
>  	case INA3221_CRIT3:
>  		*val = sign_extend32(regval >> 3, 12);
>  		break;
> +	case SQ52210_ALERT_LIMIT1:
> +	case SQ52210_ALERT_LIMIT2:
> +	case SQ52210_ALERT_LIMIT3:
> +		if (ina->alert_type_select & SQ52210_ALERT_ALL_SUL_MASK)
> +			*val = sign_extend32(regval, 15);

Does this return the unshifted register value? The standard Shunt Voltage
registers (INA3221_SHUNTx) return the value shifted right by 3.
Returning the unshifted value here causes a scaling inconsistency when
ina3221_read_curr() applies current_lsb_uA (which is based on the
shifted LSB), resulting in an 8x error in the reported current limit.

> +		else if (ina->alert_type_select & (SQ52210_ALERT_ALL_BOL_MASK
> +					| SQ52210_ALERT_ALL_BUL_MASK))
> +			*val = regval >> 3;
> +		else if (ina->alert_type_select & SQ52210_ALERT_ALL_POL_MASK)
> +			*val = regval;
> +		break;
>  	default:
>  		*val = 0;
>  		return -EOPNOTSUPP;
> @@ -443,6 +512,94 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
>  	}
>  }
>  
> +static int sq52210_alert_limit_write(struct ina3221_data *ina,
> +			enum sq52210_alert_types type, int channel, long val)
> +{
> +	struct regmap *regmap = ina->regmap;

[ ... ]

> +	return ret;
> +}
> +

Is this function unused in this commit? This might cause a build warning
or error depending on the compiler configuration.

commit a0f07f4a272baa0684c7d1ebd30e585dd23a1d77
Author:     Wenliang Yan <wenliang202407@163.com>
hwmon: (ina3221) Modify write/read functions for 'in' and 'curr' attribute

Modified the relevant read/write functions for 'in' and 'curr' attributes,
adding support for crit, lcrit, crit_alarm, and lcrit_alarm features.

> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index 2338b3288950..fdcd92082c02 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -823,6 +819,10 @@ static int ina3221_write_curr(struct device *dev, u32 attr,
>  	case hwmon_curr_crit:
>  	case hwmon_curr_max:
>  		return ina3221_write_curr_shunt(ina, attr, channel, val);
> +	case hwmon_curr_lcrit:
> +		return sq52210_alert_limit_write(ina, SQ52210_ALERT_SUL, channel, val);

Does this pass the current value (in milliAmps) directly to a function
that expects a Shunt Voltage value (in microVolts)? The
sq52210_alert_limit_write() function for SQ52210_ALERT_SUL divides by
5 (implying 5uV LSB), but does not account for the shunt resistor value.
This will set an incorrect limit unless the shunt resistor happens to
be 1 mOhm.

> +	default:
> +		return 0;
> +	}
> +}
> +