Re: [PATCH 2/2] hwmon: (ina2xx) Add support for INA234

[Ian Ray]

Note that an earlier version of the datasheet (rev C) said:
"Power [W] = 32 x CURRENT_LSB x POWER" (equation 4).

On Thu, Feb 19, 2026 at 11:15:14AM +0000, Tomaž Zaman wrote:
> 32 is the correct value, mine (25) is wrong. 
>

As Bence noted, in datasheet rev D this has now been changed (and IIUC,
the changed values are for example purposes).


> I’m curious about 25600, though, I believe it should be 1600 for bus_voltage_lsb (12 bit ADC on 16 bit regiters means 4 bit shift = 25600/16).
> 

I took this from table 7-1 of the INA234 datasheet (25.6 mV/LSB).

The voltage and current error between INA234 and the result from
multimeter is less than 3%.  So, the numbers seem to be empirically
correct (on my hardware).

> 
> 
> Tomaž Zaman, CEO 
> Mono Technologies Inc.
> 
> 
> 
> > On 18 Feb 2026 at 23:29 +0100, Bence Csókás <bence98@xxxxxxxxxx>, wrote:
> > Hi Ian,
> > 
> > thanks for this patch!
> > 
> > CC'ing Tomaz who has a downstream patch for this part as well.
> > 
> > On 2/17/26 10:23, Ian Ray wrote:
> > INA234 is register compatible to INA226 (excepting manufacturer and die
> > or device id registers) but has different scaling.
> > 
> > While the manufacturer and die/device id registers are different, these
> > are currently unused. Comment INA226_DIE_ID to aid future maintenance.
> > 
> > Signed-off-by: Ian Ray <ian.ray@xxxxxxxxxxxxxxxx>
> > ---
> > Documentation/hwmon/ina2xx.rst | 14 ++++++++++++--
> > drivers/hwmon/Kconfig | 2 +-
> > drivers/hwmon/ina2xx.c | 21 +++++++++++++++++++--
> > 3 files changed, 32 insertions(+), 5 deletions(-)
> > 
> > diff --git a/Documentation/hwmon/ina2xx.rst b/Documentation/hwmon/ina2xx.rst
> > index a3860aae444c..4c05bd5e24fb 100644
> > --- a/Documentation/hwmon/ina2xx.rst
> > +++ b/Documentation/hwmon/ina2xx.rst
> > @@ -74,6 +74,16 @@ Supported chips:
> > https://us1.silergy.com/
> > 
> > 
> > + * Texas Instruments INA234
> > +
> > + Prefix: 'ina234'
> > +
> > + Addresses: I2C 0x40 - 0x43
> > +
> > + Datasheet: Publicly available at the Texas Instruments website
> > +
> > + https://www.ti.com/
> > +
> > Author: Lothar Felten <lothar.felten@xxxxxxxxx>
> > 
> > Description
> > @@ -89,7 +99,7 @@ interface. The INA220 monitors both shunt drop and supply voltage.
> > The INA226 is a current shunt and power monitor with an I2C interface.
> > The INA226 monitors both a shunt voltage drop and bus supply voltage.
> > 
> > -INA230 and INA231 are high or low side current shunt and power monitors
> > +INA230, INA231, and INA234 are high or low side current shunt and power monitors
> > with an I2C interface. The chips monitor both a shunt voltage drop and
> > bus supply voltage.
> > 
> > @@ -124,7 +134,7 @@ power1_input Power(uW) measurement channel
> > shunt_resistor Shunt resistance(uOhm) channel (not for ina260)
> > ======================= ===============================================
> > 
> > -Additional sysfs entries for ina226, ina230, ina231, ina260, and sy24655
> > +Additional sysfs entries for ina226, ina230, ina231, ina234, ina260, and sy24655
> > ------------------------------------------------------------------------
> > 
> > As buildbot already complained: you need to match the dashes' length to
> > the text. Besides, I'm not sure that listing everything here is the best
> > approach. I would change it to something like
> > 
> > Additional sysfs entries for some supported parts
> > 
> > And then maybe list those parts in a bullet list or something. That way,
> > we only need to add lines going forward.
> > 

Yes, makes sense.

> > 
> > ======================= ====================================================
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 41c381764c2b..6aa8a89f4747 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -2284,7 +2284,7 @@ config SENSORS_INA2XX
> > select REGMAP_I2C
> > help
> > If you say yes here you get support for INA219, INA220, INA226,
> > - INA230, INA231, INA260, and SY24655 power monitor chips.
> > + INA230, INA231, INA234, INA260, and SY24655 power monitor chips.
> > 
> > The INA2xx driver is configured for the default configuration of
> > the part as described in the datasheet.
> > diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> > index 69ac0468dee4..923f8c953e8f 100644
> > --- a/drivers/hwmon/ina2xx.c
> > +++ b/drivers/hwmon/ina2xx.c
> > @@ -49,6 +49,8 @@
> > /* INA226 register definitions */
> > #define INA226_MASK_ENABLE 0x06
> > #define INA226_ALERT_LIMIT 0x07
> > +
> > +/* INA226-specific register definitions */
> > 
> > Isn't this comment redundant? Almost the same comment is already there 3
> > lines above.

INA226_DIE_ID is applicable for INA226, whereas the first 8 registers
are applicable to a wider set of chips (incuding INA234).  Perhaps I
should change the earlier comment to "INA2xx register definitions"?

> > 
> > #define INA226_DIE_ID 0xFF
> > 
> > /* SY24655 register definitions */
> > @@ -59,6 +61,7 @@
> > /* settings - depend on use case */
> > #define INA219_CONFIG_DEFAULT 0x399F /* PGA=8 */
> > #define INA226_CONFIG_DEFAULT 0x4527 /* averages=16 */
> > +#define INA234_CONFIG_DEFAULT 0x4527 /* averages=16 */
> > 
> > Do we need a new macro? Wouldn't it make sense to just use
> > `INA226_CONFIG_DEFAULT`?

Sure, we can re-use that.

> > 
> > #define INA260_CONFIG_DEFAULT 0x6527 /* averages=16 */
> > #define SY24655_CONFIG_DEFAULT 0x4527 /* averages=16 */
> > 
> > @@ -135,7 +138,7 @@ static const struct regmap_config ina2xx_regmap_config = {
> > .writeable_reg = ina2xx_writeable_reg,
> > };
> > 
> > -enum ina2xx_ids { ina219, ina226, ina260, sy24655 };
> > +enum ina2xx_ids { ina219, ina226, ina234, ina260, sy24655 };
> > 
> > Maybe it is time to break this into a multi-line enum?

Agreed!

> > 
> > 
> > struct ina2xx_config {
> > u16 config_default;
> > @@ -204,6 +207,15 @@ static const struct ina2xx_config ina2xx_config[] = {
> > .has_ishunt = false,
> > .has_power_average = true,
> > },
> > + [ina234] = {
> > + .config_default = INA234_CONFIG_DEFAULT,
> > + .calibration_value = 2048,
> > + .shunt_div = 400, /* 2.5 µV/LSB raw ADC reading from INA2XX_SHUNT_VOLTAGE */
> > + .bus_voltage_shift = 4,
> > + .bus_voltage_lsb = 25600,
> > + .power_lsb_factor = 32,
> > 
> > How did you derive this? According to "7.1.2 Current and Power
> > Calculations" [1] in the datasheet, `POWER_LSB = 2 * CURRENT_LSB`, so
> > I'd think this should be `= 2`, although I'll say I'm not familiar with
> > the IC itself. Tomaz, I do believe you had `25` here, was that just a
> > placeholder?

(See earlier discussion.)

> > 
> > [1]
> > https://www.ti.com/lit/ds/symlink/ina234.pdf#%5B%7B%22num%22%3A421%2C%22gen%22%3A0%7D%2C%7B%22name%22%3A%22XYZ%22%7D%2Cnull%2C316.653%2Cnull%5D
> > 
> > + .has_alerts = true,
> > + },
> > };
> > 
> > /*
> > @@ -768,7 +780,7 @@ static umode_t ina2xx_is_visible(const void *_data, enum hwmon_sensor_types type
> > case hwmon_chip:
> > switch (attr) {
> > case hwmon_chip_update_interval:
> > - if (chip == ina226 || chip == ina260)
> > + if (chip == ina226 || chip == ina234 || chip == ina260)
> > 
> > I'd say this deserves a new `has_*` member.

Agreed.

> > 
> > return 0644;
> > break;
> > default:
> > @@ -982,6 +994,7 @@ static const struct i2c_device_id ina2xx_id[] = {
> > { "ina226", ina226 },
> > { "ina230", ina226 },
> > { "ina231", ina226 },
> > + { "ina234", ina234 },
> > { "ina260", ina260 },
> > { "sy24655", sy24655 },
> > { }
> > @@ -1013,6 +1026,10 @@ static const struct of_device_id __maybe_unused ina2xx_of_match[] = {
> > .compatible = "ti,ina231",
> > .data = (void *)ina226
> > },
> > + {
> > + .compatible = "ti,ina234",
> > + .data = (void *)ina234
> > + },
> > {
> > .compatible = "ti,ina260",
> > .data = (void *)ina260
> > 
> > Bence

Many thanks for the reviews.

I will prepare a V2.

Regards,
Ian