| 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: <cceed588-9699-423e-8a33-33dcad471133@roeck-us.net>
Date: Wed, 16 Jul 2025 08:50:26 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Jonas Rebmann <jre@...gutronix.de>, Jean Delvare <jdelvare@...e.com>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>
Cc: linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
Krzysztof Kozlowski <krzk@...nel.org>, devicetree@...r.kernel.org,
kernel@...gutronix.de
Subject: Re: [PATCH 4/4] hwmon: ina238: Add support for INA228
On 7/15/25 13:49, Jonas Rebmann wrote:
> Add support for the Texas Instruments INA228 Ultra-Precise
> Power/Energy/Charge Monitor.
>
> The INA228 is very similar to the INA238 but offers four bits of extra
> precision in the temperature, voltage and current measurement fields.
> It also supports energy and charge monitoring, the latter of which is
> not supported through this patch.
>
> While it seems in the datasheet that some constants such as LSB values
> differ between the 228 and the 238, they differ only for those registers
> where four bits of precision have been added and they differ by a factor
> of 16 (VBUS, VSHUNT, DIETEMP, CURRENT).
>
> Therefore, the INA238 constants are still applicable with regard
> to the bit of the same significance.
>
> Signed-off-by: Jonas Rebmann <jre@...gutronix.de>
> ---
> drivers/hwmon/ina238.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 93 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hwmon/ina238.c b/drivers/hwmon/ina238.c
> index 44f7ce3c1d7b5a91f67d12c1d29e1e560024a04c..f8c74317344a5bbdf933a32b8c7e5aba13beda30 100644
> --- a/drivers/hwmon/ina238.c
> +++ b/drivers/hwmon/ina238.c
> @@ -107,6 +107,7 @@
> #define INA238_DIE_TEMP_LSB 1250000 /* 125.0000 mC/lsb */
> #define SQ52206_BUS_VOLTAGE_LSB 3750 /* 3.75 mV/lsb */
> #define SQ52206_DIE_TEMP_LSB 78125 /* 7.8125 mC/lsb */
> +#define INA228_DIE_TEMP_LSB 78125 /* 7.8125 mC/lsb */
>
> static const struct regmap_config ina238_regmap_config = {
> .max_register = INA238_REGISTERS,
> @@ -114,9 +115,10 @@ static const struct regmap_config ina238_regmap_config = {
> .val_bits = 16,
> };
>
> -enum ina238_ids { ina238, ina237, sq52206 };
> +enum ina238_ids { ina238, ina237, sq52206, ina228 };
>
> struct ina238_config {
> + bool has_20bit_voltage_current; /* vshunt, vbus and current are 20-bit fields */
> bool has_power_highest; /* chip detection power peak */
> bool has_energy; /* chip detection energy */
> u8 temp_shift; /* fixed parameters for temp calculate */
> @@ -137,6 +139,7 @@ struct ina238_data {
>
> static const struct ina238_config ina238_config[] = {
> [ina238] = {
> + .has_20bit_voltage_current = false,
> .has_energy = false,
> .has_power_highest = false,
> .temp_shift = 4,
> @@ -146,6 +149,7 @@ static const struct ina238_config ina238_config[] = {
> .temp_lsb = INA238_DIE_TEMP_LSB,
> },
> [ina237] = {
> + .has_20bit_voltage_current = false,
> .has_energy = false,
> .has_power_highest = false,
> .temp_shift = 4,
> @@ -155,6 +159,7 @@ static const struct ina238_config ina238_config[] = {
> .temp_lsb = INA238_DIE_TEMP_LSB,
> },
> [sq52206] = {
> + .has_20bit_voltage_current = false,
> .has_energy = true,
> .has_power_highest = true,
> .temp_shift = 0,
> @@ -163,6 +168,16 @@ static const struct ina238_config ina238_config[] = {
> .bus_voltage_lsb = SQ52206_BUS_VOLTAGE_LSB,
> .temp_lsb = SQ52206_DIE_TEMP_LSB,
> },
> + [ina228] = {
> + .has_20bit_voltage_current = true,
> + .has_energy = true,
> + .has_power_highest = false,
> + .temp_shift = 0,
> + .power_calculate_factor = 20,
> + .config_default = INA238_CONFIG_DEFAULT,
> + .bus_voltage_lsb = INA238_BUS_VOLTAGE_LSB,
> + .temp_lsb = INA228_DIE_TEMP_LSB,
> + },
> };
>
> static int ina238_read_reg24(const struct i2c_client *client, u8 reg, u32 *val)
> @@ -199,6 +214,56 @@ static int ina238_read_reg40(const struct i2c_client *client, u8 reg, u64 *val)
> return 0;
> }
>
> +static int ina228_read_shunt_voltage(struct device *dev, u32 attr, int channel,
> + long *val)
> +{
> + struct ina238_data *data = dev_get_drvdata(dev);
> + int regval;
> + int field;
> + int err;
> +
> + err = ina238_read_reg24(data->client, INA238_SHUNT_VOLTAGE, ®val);
> + if (err)
> + return err;
> +
> + /* bits 3-0 Reserved, always zero */
> + field = regval >> 4;
> +
> + /*
> + * gain of 1 -> LSB / 4
> + * This field has 16 bit on ina238. ina228 adds another 4 bits of
> + * precision. ina238 conversion factors can still be applied when
> + * dividing by 16.
> + */
> + *val = (field * INA238_SHUNT_VOLTAGE_LSB) * data->gain / (1000 * 4) / 16;
> + return 0;
> +}
> +
> +static int ina228_read_bus_voltage(struct device *dev, u32 attr, int channel,
> + long *val)
> +{
> + struct ina238_data *data = dev_get_drvdata(dev);
> + int regval;
> + int field;
> + int err;
> +
> + err = ina238_read_reg24(data->client, INA238_BUS_VOLTAGE, ®val);
> + if (err)
> + return err;
> +
> + /* bits 3-0 Reserved, always zero */
> + field = regval >> 4;
> +
> + /*
> + * gain of 1 -> LSB / 4
> + * This field has 16 bit on ina238. ina228 adds another 4 bits of
> + * precision. ina238 conversion factors can still be applied when
> + * dividing by 16.
> + */
> + *val = (field * data->config->bus_voltage_lsb) / 1000 / 16;
> + return 0;
> +}
Sign extension missing for both. Yes, I understand, the bus voltage is always positive,
but the shunt voltage isn't.
> +
> static int ina238_read_in(struct device *dev, u32 attr, int channel,
> long *val)
> {
> @@ -211,6 +276,8 @@ static int ina238_read_in(struct device *dev, u32 attr, int channel,
> case 0:
> switch (attr) {
> case hwmon_in_input:
> + if (data->config->has_20bit_voltage_current)
> + return ina228_read_shunt_voltage(dev, attr, channel, val);
> reg = INA238_SHUNT_VOLTAGE;
> break;
> case hwmon_in_max:
> @@ -234,6 +301,8 @@ static int ina238_read_in(struct device *dev, u32 attr, int channel,
> case 1:
> switch (attr) {
> case hwmon_in_input:
> + if (data->config->has_20bit_voltage_current)
> + return ina228_read_bus_voltage(dev, attr, channel, val);
> reg = INA238_BUS_VOLTAGE;
> break;
> case hwmon_in_max:
> @@ -341,13 +410,27 @@ static int ina238_read_current(struct device *dev, u32 attr, long *val)
>
> switch (attr) {
> case hwmon_curr_input:
> - err = regmap_read(data->regmap, INA238_CURRENT, ®val);
> - if (err < 0)
> - return err;
> + if (data->config->has_20bit_voltage_current) {
> + err = ina238_read_reg24(data->client, INA238_CURRENT, ®val);
> + if (err)
> + return err;
> + /* 4 Lowest 4 bits reserved zero */
> + regval >>= 4;
It is still a signed register and thus needs sign extension.
> + } else {
> + err = regmap_read(data->regmap, INA238_CURRENT, ®val);
> + if (err < 0)
> + return err;
> + /* sign-extend */
> + regval = (s16)regval;
> + }
>
> /* Signed register, fixed 1mA current lsb. result in mA */
> - *val = div_s64((s16)regval * INA238_FIXED_SHUNT * data->gain,
> + *val = div_s64(regval * INA238_FIXED_SHUNT * data->gain,
> data->rshunt * 4);
Reading this again, I suspect that "regval * INA238_FIXED_SHUNT * data->gain"
is a 32-bit value (it is never extended to 64 bit) which may overflow. That probably
never happened with the old chips, but regval is now a 20-bit value so overflows
are more likely than before. The code needs to make sure that the expression
has a 64-bit result.
> +
> + /* Account for 4 bit offset */
> + if (data->config->has_20bit_voltage_current)
> + *val /= 16;
> break;
> default:
> return -EOPNOTSUPP;
> @@ -773,6 +856,7 @@ static int ina238_probe(struct i2c_client *client)
> }
>
> static const struct i2c_device_id ina238_id[] = {
> + { "ina228", ina228 },
> { "ina237", ina237 },
> { "ina238", ina238 },
> { "sq52206", sq52206 },
> @@ -781,6 +865,10 @@ static const struct i2c_device_id ina238_id[] = {
> MODULE_DEVICE_TABLE(i2c, ina238_id);
>
> static const struct of_device_id __maybe_unused ina238_of_match[] = {
> + {
> + .compatible = "ti,ina228",
> + .data = (void *)ina228
> + },
> {
> .compatible = "ti,ina237",
> .data = (void *)ina237
>
Powered by blists - more mailing lists