| 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: <20251114073644.6813-1-wenliang202407@163.com>
Date: Fri, 14 Nov 2025 02:36:44 -0500
From: Wenliang Yan <wenliang202407@....com>
To: linux@...ck-us.net
Cc: christophe.jaillet@...adoo.fr,
conor+dt@...nel.org,
corbet@....net,
devicetree@...r.kernel.org,
jdelvare@...e.com,
krzk+dt@...nel.org,
linux-hwmon@...r.kernel.org,
linux-kernel@...r.kernel.org,
robh@...nel.org,
wenliang202407@....com
Subject: Re: [PATCH 1/8] dt-binding:ti,ina3221:Add SQ52210
At 2025-11-14 01:16:07, "Guenter Roeck" <linux@...ck-us.net> wrote:
>On Tue, Nov 11, 2025 at 03:05:44AM -0500, Wenliang Yan wrote:
>> enum ina3221_channels {
>> @@ -293,11 +303,39 @@ static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
>> * Shunt Voltage Sum register has 14-bit value with 1-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:
>> + 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:
>> + *val = regval >> 3;
>> + break;
>> + };
>
>This returns 0 if the register is not listed in the case statement while leaving
>val unset. It would probably be better to drop this function entirely and handle the
>conversions in the calling code.
>
This call is used quite frequently, and handling it directly in the calling code
would result in significant code redundancy. I believe adding default: *val = 0;
return -EOPNOTSUPP; should address your concerns.
>> return 0;
>> }
>>
>> +/*
>> + * Turns alert limit values into register values.
>> + * Opposite of the formula in ina3221_read_value().
>> + */
>> +static u16 sq52210_alert_to_reg(struct ina3221_data *ina, int reg, long val)
>> +{
>> + int regval;
>> + /*
>> + * Formula to convert voltage_uv to register value:
>> + * regval = (voltage_mv / scale) << shift
>> + * Results:
>> + * bus_voltage: (1 / 8mV) << 3 = 1 mV
>> + */
>> + switch (reg) {
>> + case INA3221_BUS1:
>> + case INA3221_BUS2:
>> + case INA3221_BUS3:
>> + /* clamp voltage */
>> + regval = clamp_val(val, -32760, 32760);
>> + return regval;
>> + case SQ52210_CURRENT1:
>> + case SQ52210_CURRENT2:
>> + case SQ52210_CURRENT3:
>> + /* signed register, result in mA */
>> + regval = DIV_ROUND_CLOSEST(val * 8000, ina->current_lsb_uA);
>> + return clamp_val(regval, -32760, 32760);
>> + case SQ52210_POWER1:
>> + case SQ52210_POWER2:
>> + case SQ52210_POWER3:
>> + regval = DIV_ROUND_CLOSEST(val * 8000, ina->power_lsb_uW);
>> + return clamp_val(regval, 0, 65528);
>> + default:
>> + /* programmer goofed */
>> + WARN_ON_ONCE(1);
>> + return 0;
>
>Same as above. I know other code uses the samew approach, but this kind of
>"validation" would be better to avoid. It would be much better to handle the
>conversions in the calling code.
>
As per your recommendation, I will remove this function and complete the
conversion directly within the sq52210_alert_limit_write function.
>> + }
>> +}
>> +
>> static int ina3221_read_chip(struct device *dev, u32 attr, long *val)
>> {
>> struct ina3221_data *ina = dev_get_drvdata(dev);
>> @@ -373,6 +461,25 @@ static int ina3221_read_in(struct device *dev, u32 attr, int channel, long *val)
>> case hwmon_in_enable:
>> *val = ina3221_is_enabled(ina, channel);
>> return 0;
>> + case hwmon_in_crit:
>> + case hwmon_in_lcrit:
>> + reg = alert_limit_reg[channel];
>> + ret = ina3221_read_value(ina, reg, ®val);
>> + if (ret)
>> + return ret;
>> + /*
>> + * Scale of bus voltage (mV): LSB is 8mV
>> + */
>> + *val = regval * 8;
>> + return 0;
>> + case hwmon_in_crit_alarm:
>> + case hwmon_in_lcrit_alarm:
>> + reg = alert_flag[channel];
>> + ret = regmap_field_read(ina->fields[reg], ®val);
>> + if (ret)
>> + return ret;
>> + *val = regval;
>> + return 0;
>> default:
>> return -EOPNOTSUPP;
>> }
>> @@ -450,6 +557,58 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
>> }
>> }
>>
>> +static const u32 sq52210_alert_mask[][INA3221_NUM_CHANNELS] = {
>> + [hwmon_curr_lcrit] = { BIT(15), BIT(14), BIT(13) },
>> + [hwmon_in_crit] = { BIT(12), BIT(11), BIT(10) },
>> + [hwmon_in_lcrit] = { BIT(9), BIT(8), BIT(7) },
>> + [hwmon_power_crit] = { BIT(6), BIT(5), BIT(4) },
>
>This does not work. hwmon_curr_xxx, hwmon_inxxx, and hwmon_power_xxx use
>different and overlapping number spaces. Even if that was not the case,
>the above would result in a serverely sparse array, and the callingo code
>would have to make sure that the row is actually populated before using it.
>
Using an array here isn't quite appropriate. I'll use a switch statement in
the next version to determine different alert flags to resolve this issue.
>> +};
>> +
>> +static int sq52210_alert_limit_write(struct ina3221_data *ina, u32 attr, int channel, long val)
>> +{
>> + struct regmap *regmap = ina->regmap;
>> + int ret, limit_reg, item;
>> + u32 alert_group;
>> +
>> + if (val < 0)
>> + return -EINVAL;
>> + item = channel % INA3221_NUM_CHANNELS;
>> + switch (item) {
>> + case 0:
>> + alert_group = SQ52210_MASK_ALERT_CHANNEL1;
>> + limit_reg = SQ52210_ALERT_LIMIT1;
>> + break;
>> + case 1:
>> + alert_group = SQ52210_MASK_ALERT_CHANNEL2;
>> + limit_reg = SQ52210_ALERT_LIMIT2;
>> + break;
>> + case 2:
>> + alert_group = SQ52210_MASK_ALERT_CHANNEL3;
>> + limit_reg = SQ52210_ALERT_LIMIT3;
>> + break;
>> + default:
>> + break;
>> + }
>> + /*
>> + * Clear all alerts first to avoid accidentally triggering ALERT pin
>> + * due to register write sequence. Then, only enable the alert
>> + * if the value is non-zero.
>> + */
>> + ret = regmap_update_bits(regmap, SQ52210_ALERT_CONFIG,
>> + alert_group, 0);
>> + if (ret < 0)
>> + return ret;
>> + ret = regmap_write(regmap, limit_reg,
>> + sq52210_alert_to_reg(ina, ina3221_curr_reg[attr][item], val));
>> + if (ret < 0)
>> + return ret;
>> +
>> + if (val)
>> + return regmap_update_bits(regmap, SQ52210_ALERT_CONFIG,
>> + alert_group, sq52210_alert_mask[attr][item]);
>> + return 0;
>> +}
>> +
Thanks,
Wenlaing Yan
Powered by blists - more mailing lists