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

["Bence Csókás" <bence98@sch.bme.hu>]

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@gehealthcare.com>
> ---
>   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@gmail.com>
>   
>   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.

>   
>   ======================= ====================================================
> 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.

>   #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`?

>   #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?

>   
>   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?

[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.

>   				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