.../devicetree/bindings/hwmon/ti,ina3221.yaml | 15 +- Documentation/hwmon/ina3221.rst | 24 + drivers/hwmon/ina3221.c | 545 +++++++++++++++++- 3 files changed, 570 insertions(+), 14 deletions(-)
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).
---
v4: https://lore.kernel.org/linux-hwmon/20260114081741.111340-1-wenliang202407@163.com/
v3: https://lore.kernel.org/linux-hwmon/20251120081921.39412-1-wenliang202407@163.com/
v2: https://lore.kernel.org/linux-hwmon/20251118125148.95603-1-wenliang202407@163.com/
v1: https://lore.kernel.org/linux-hwmon/20251111080546.32421-1-wenliang202407@163.com/
Wenliang Yan (8):
dt-bindings: hwmon: ti,ina3221: Add SQ52210
hwmon: (ina3221) Add support for SQ52210
hwmon: (ina3221) Pre-calculate current and power LSB
hwmon: (ina3221) Support alert configuration
hwmon: (ina3221) Introduce power attribute and alert characteristics
hwmon: (ina3221) Support for writing alert limit values and modify the
'ina3221_read_value' function
hwmon: (ina3221) Support write/read functions for 'power' attribute
hwmon: (ina3221) Modify write/read functions for 'in' and 'curr'
attribute
.../devicetree/bindings/hwmon/ti,ina3221.yaml | 15 +-
Documentation/hwmon/ina3221.rst | 24 +
drivers/hwmon/ina3221.c | 545 +++++++++++++++++-
3 files changed, 570 insertions(+), 14 deletions(-)
--
2.17.1
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.
Thanks,
Guenter
---
Analysis of Top 8 Commits in drivers/hwmon/ina3221.c
Commit: a0f07f4a272b hwmon: (ina3221) Modify write/read functions for 'in' and 'curr' attribute
--------------------------------------------------------------------------------------------
Change Categories:
- CHANGE-1: Added read/write support for inX_crit/lcrit/alarm
- CHANGE-2: Added read/write support for currX_lcrit/alarm
- CHANGE-3: Refactored ina3221_write_curr
Regressions/Bugs:
1. MAJOR BUG: In `ina3221_write_curr`, `hwmon_curr_lcrit` passes current value (in mA) directly to `sq52210_alert_limit_write`. `sq52210_alert_limit_write` treats the value as Shunt Voltage (in uV) for `SQ52210_ALERT_SUL` (divides by 5). This fails to account for the shunt resistor value. The alert limit will be incorrect for any shunt resistor other than 1 mOhm.
Evidence:
`ina3221_write_curr`: `return sq52210_alert_limit_write(ina, SQ52210_ALERT_SUL, channel, val);`
`sq52210_alert_limit_write`: `regval = DIV_ROUND_CLOSEST(val, 5) & 0xfff8;`
Commit: ef198f703062 hwmon: (ina3221) Support write/read functions for 'power' attribute
----------------------------------------------------------------------------------------
Change Categories:
- CHANGE-1: Added read/write support for powerX attributes
- CHANGE-2: Updated ina3221_is_visible
Regressions/Bugs:
1. BUG: `ina3221_is_visible` incorrectly enables `hwmon_power_crit` and `hwmon_power_crit_alarm` based solely on `has_alerts`. Standard INA3221 devices have `has_alerts` set to true but do not support power measurement (`has_power` is false). This exposes unsupported power attributes to userspace for INA3221.
Evidence:
```c
case hwmon_power_crit:
if (has_alerts)
return 0644;
return 0;
```
Commit: 61e453283222 hwmon: (ina3221) Support for writing alert limit values and modify the 'ina3221_read_value' function
-------------------------------------------------------------------------------------------------------------------------
Change Categories:
- CHANGE-1: Refactored `ina3221_read_value` to handle SQ52210 registers
- CHANGE-2: Added `sq52210_alert_limit_write`
Regressions/Bugs:
1. CRITICAL BUG: `ina3221_read_value` leaves `*val` uninitialized when reading `SQ52210_ALERT_LIMITx` registers if `ina->alert_type_select` is 0 (default). `ina3221_probe` initializes `alert_type_select` to 0 but does not sync it with the device's `SQ52210_ALERT_CONFIG` register. Reading these attributes before writing them results in returning garbage stack data to userspace.
Evidence:
```c
switch (reg) {
...
case SQ52210_ALERT_LIMIT3:
if (ina->alert_type_select & SQ52210_ALERT_ALL_SUL_MASK)
*val = ...
else if (...)
*val = ...
break;
}
return 0;
```
If no mask matches, `*val` is never assigned.
Summary:
Multiple significant regressions found in the series. The logic for SUL (Shunt Under Limit) handling for Current attributes is fundamentally broken regarding unit conversion (mA vs uV) and shunt resistance. Visibility checks for power attributes are too permissive for legacy devices. Uninitialized variable usage in read path poses a stability/correctness risk.
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;
> + }
> +}
> +
© 2016 - 2026 Red Hat, Inc.