| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5b84260d-b8bc-48d7-8334-2ea662e82421@roeck-us.net>
Date: Tue, 18 Nov 2025 20:20:34 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Wenliang Yan <wenliang202407@....com>, Jean Delvare <jdelvare@...e.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>,
linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 6/8] hwmon: (ina3221) Support for writing alert limit
values and modify the 'ina3221_read_value' function.
On 11/18/25 04:51, Wenliang Yan wrote:
> SQ52210 adds power, current, and limit registers. The ina3221_read_value
> function has been refactored to adapt to the new register data reading
> format.
>
> Each channel supports four new alert trigger modes, but only one trigger
> mode can be active at any given time. Alert values are stored in the same
> register. The sq52210_alert_limit_write function has been added to write
> alert threshold values and configure alert source type.
>
> Signed-off-by: Wenliang Yan <wenliang202407@....com>
> ---
> drivers/hwmon/ina3221.c | 150 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 147 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index 1d589d402b52..9a25a1b40856 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -66,6 +66,14 @@
> #define INA3221_MASK_ENABLE_SCC_MASK GENMASK(14, 12)
>
> #define SQ52210_ALERT_CONFIG_MASK GENMASK(15, 4)
> +#define SQ52210_MASK_ALERT_CHANNEL1 (BIT(15) | BIT(12) | BIT(9) | BIT(6))
> +#define SQ52210_MASK_ALERT_CHANNEL2 (BIT(14) | BIT(11) | BIT(8) | BIT(5))
> +#define SQ52210_MASK_ALERT_CHANNEL3 (BIT(13) | BIT(10) | BIT(7) | BIT(4))
> +
> +#define SQ52210_ALERT_ALL_SUL_MASK (BIT(15) | BIT(14) | BIT(13))
> +#define SQ52210_ALERT_ALL_BOL_MASK (BIT(12) | BIT(11) | BIT(10))
> +#define SQ52210_ALERT_ALL_BUL_MASK (BIT(9) | BIT(8) | BIT(7))
> +#define SQ52210_ALERT_ALL_POL_MASK (BIT(6) | BIT(5) | BIT(4))
>
> #define INA3221_CONFIG_DEFAULT 0x7127
> #define INA3221_RSHUNT_DEFAULT 10000
> @@ -272,6 +280,18 @@ static inline int ina3221_wait_for_data(struct ina3221_data *ina)
> cvrf, cvrf, wait, wait * 2);
> }
>
> +static const u32 alert_groups[] = {
> + SQ52210_MASK_ALERT_CHANNEL1,
> + SQ52210_MASK_ALERT_CHANNEL2,
> + SQ52210_MASK_ALERT_CHANNEL3,
> +};
> +
> +static const u8 limit_regs[] = {
> + SQ52210_ALERT_LIMIT1,
> + SQ52210_ALERT_LIMIT2,
> + SQ52210_ALERT_LIMIT3,
> +};
> +
> static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
> int *val)
> {
> @@ -284,13 +304,55 @@ static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
>
> /*
> * Shunt Voltage Sum register has 14-bit value with 1-bit shift
> + * Current registers have 15-bit value
> + * Power registers have 16-bit value
> + * ALERT_LIMIT registers have 16-bit value with 3-bit shift
> * Other Shunt Voltage registers have 12 bits with 3-bit shift
> */
> - if (reg == INA3221_SHUNT_SUM || reg == INA3221_CRIT_SUM)
> + switch (reg) {
> + case INA3221_SHUNT_SUM:
> + case INA3221_CRIT_SUM:
> *val = sign_extend32(regval >> 1, 14);
> - else
> + break;
> + case SQ52210_CURRENT1:
> + case SQ52210_CURRENT2:
> + case SQ52210_CURRENT3:
> + *val = sign_extend32(regval, 15);
> + break;
> + case SQ52210_POWER1:
> + case SQ52210_POWER2:
> + case SQ52210_POWER3:
> + *val = regval;
> + break;
> + case INA3221_BUS1:
> + case INA3221_BUS2:
> + case INA3221_BUS3:
> + case INA3221_SHUNT1:
> + case INA3221_SHUNT2:
> + case INA3221_SHUNT3:
> + case INA3221_WARN1:
> + case INA3221_WARN2:
> + case INA3221_WARN3:
> + case INA3221_CRIT1:
> + case INA3221_CRIT2:
> + 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);
> + 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;
> + };
> return 0;
> }
>
> @@ -443,6 +505,88 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
> }
> }
>
> +static int sq52210_alert_limit_write(struct ina3221_data *ina, u32 attr, int channel, long val)
> +{
> + struct regmap *regmap = ina->regmap;
> + int item = channel % INA3221_NUM_CHANNELS;
> + u8 limit_reg;
> + u32 alert_group, alert_mask = 0;
> + int regval = 0;
> + int ret;
> +
> + if (item >= ARRAY_SIZE(alert_groups) || val < 0)
> + return -EINVAL;
> +
> + alert_group = alert_groups[item];
> + limit_reg = limit_regs[item];
> +
> + /* Clear alerts for this channel group first */
> + ret = regmap_update_bits(regmap, SQ52210_ALERT_CONFIG, alert_group, 0);
> + if (ret)
> + return ret;
> +
> + /* Determine alert type and calculate register value */
> + switch (attr) {
> + /*
> + * The alert warning logic is implemented by comparing the limit register values
> + * with the corresponding alert source register values. Since the current register
> + * is a 15-bit signed register and the power register is a 16-bit unsigned
> + * register, but the lower 3 bits of the limit register default to 0, the lower
> + * 3 bits will be forced to 0 when setting SUL and POL warning values.
> + * Formula to convert register value:
> + * bus_voltage: (regval / 8mV) << 3
> + * current: (regval / current_lsb) & 0xfff8
> + * power: (regval / current_lsb) & 0xfff8
> + */
> + case hwmon_curr_lcrit:
> + /* SUL: Shunt Under Limit - BIT(15), BIT(14), BIT(13) */
> + alert_mask = BIT(15 - item);
> + /* Current Register, signed register, result in mA */
> + regval = DIV_ROUND_CLOSEST(val * 1000, ina->current_lsb_uA) & 0xfff8;
> + regval = clamp_val(regval, -32760, 32760);
> + break;
> + case hwmon_in_crit:
> + /* BOL: Bus Over Limit - BIT(12), BIT(11), BIT(10) */
> + alert_mask = BIT(12 - item);
> + /* Bus Register, signed register, result in mV */
> + regval = clamp_val(val, -32760, 32760);
> + break;
> + case hwmon_in_lcrit:
> + /* BUL: Bus Under Limit - BIT(9), BIT(8), BIT(7) */
> + alert_mask = BIT(9 - item);
> + /* Bus Register, signed register, result in mV */
> + regval = clamp_val(val, -32760, 32760);
> + break;
> + case hwmon_power_crit:
Didn't I say this before ? The number space for each sensor type overlaps.
Sensor attributes for different types can not be used in the same case statement.
Did you even compile this ? I would be quite surprised if there is a compiler
that would accept this code.
Guenter
Powered by blists - more mailing lists