1. Patchew
  2. linux
  3. View series

[PATCH] (ina2xx) Drop bus_voltage_shift configuration

Jonas Rebmann posted 1 patch 1 month, 1 week ago 
drivers/hwmon/ina2xx.c | 15 ++++-----------
1 file changed, 4 insertions(+), 11 deletions(-)
[PATCH] (ina2xx) Drop bus_voltage_shift configuration
Posted by Jonas Rebmann 1 month, 1 week ago 
The INA219 has the lowest three bits of the bus voltage register
zero-reserved and the bus_voltage_shift ina2xx_config field was
introduced to accommodate for that.

The INA234 has four bits of the bus voltage, of the shunt voltage, and
of the current registers zero-reserved but the latter two were
implemented by choosing a 16x higher conversion constant instead of a
separate field specifying a bit shift.

For consistency and simplicity, drop bus_voltage_shift and adapt the
conversion constants for INA219 and INA234 accordingly, yielding the
same measurement values.

Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
---
 drivers/hwmon/ina2xx.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 836e15a5a780..d7c894d7353c 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -151,7 +151,6 @@ struct ina2xx_config {
 	bool has_update_interval;
 	int calibration_value;
 	int shunt_div;
-	int bus_voltage_shift;
 	int bus_voltage_lsb;	/* uV */
 	int power_lsb_factor;
 };
@@ -172,8 +171,7 @@ static const struct ina2xx_config ina2xx_config[] = {
 		.config_default = INA219_CONFIG_DEFAULT,
 		.calibration_value = 4096,
 		.shunt_div = 100,
-		.bus_voltage_shift = 3,
-		.bus_voltage_lsb = 4000,
+		.bus_voltage_lsb = 500,
 		.power_lsb_factor = 20,
 		.has_alerts = false,
 		.has_ishunt = false,
@@ -184,7 +182,6 @@ static const struct ina2xx_config ina2xx_config[] = {
 		.config_default = INA226_CONFIG_DEFAULT,
 		.calibration_value = 2048,
 		.shunt_div = 400,
-		.bus_voltage_shift = 0,
 		.bus_voltage_lsb = 1250,
 		.power_lsb_factor = 25,
 		.has_alerts = true,
@@ -196,8 +193,7 @@ static const struct ina2xx_config ina2xx_config[] = {
 		.config_default = INA226_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,
+		.bus_voltage_lsb = 1600,
 		.power_lsb_factor = 32,
 		.has_alerts = true,
 		.has_ishunt = false,
@@ -207,7 +203,6 @@ static const struct ina2xx_config ina2xx_config[] = {
 	[ina260] = {
 		.config_default = INA260_CONFIG_DEFAULT,
 		.shunt_div = 400,
-		.bus_voltage_shift = 0,
 		.bus_voltage_lsb = 1250,
 		.power_lsb_factor = 8,
 		.has_alerts = true,
@@ -219,7 +214,6 @@ static const struct ina2xx_config ina2xx_config[] = {
 		.config_default = SY24655_CONFIG_DEFAULT,
 		.calibration_value = 4096,
 		.shunt_div = 400,
-		.bus_voltage_shift = 0,
 		.bus_voltage_lsb = 1250,
 		.power_lsb_factor = 25,
 		.has_alerts = true,
@@ -281,8 +275,7 @@ static int ina2xx_get_value(struct ina2xx_data *data, u8 reg,
 		val = DIV_ROUND_CLOSEST((s16)regval, data->config->shunt_div);
 		break;
 	case INA2XX_BUS_VOLTAGE:
-		val = (regval >> data->config->bus_voltage_shift) *
-		  data->config->bus_voltage_lsb;
+		val = regval * data->config->bus_voltage_lsb;
 		val = DIV_ROUND_CLOSEST(val, 1000);
 		break;
 	case INA2XX_POWER:
@@ -387,7 +380,7 @@ static u16 ina226_alert_to_reg(struct ina2xx_data *data, int reg, long val)
 		return clamp_val(val, 0, SHRT_MAX);
 	case INA2XX_BUS_VOLTAGE:
 		val = clamp_val(val, 0, 200000);
-		val = (val * 1000) << data->config->bus_voltage_shift;
+		val = val * 1000;
 		val = DIV_ROUND_CLOSEST(val, data->config->bus_voltage_lsb);
 		return clamp_val(val, 0, USHRT_MAX);
 	case INA2XX_POWER:

---
base-commit: f08c5de5f61a117ba5326d3d5b86e884077da2d0
change-id: 20260302-ina2xx-shift-89ed2c5d2e72

Best regards,
--  
Jonas Rebmann <jre@pengutronix.de>
Re: [PATCH] (ina2xx) Drop bus_voltage_shift configuration
Posted by Guenter Roeck 1 month, 1 week ago 
Hi,

On 3/2/26 07:26, Jonas Rebmann wrote:
> The INA219 has the lowest three bits of the bus voltage register
> zero-reserved and the bus_voltage_shift ina2xx_config field was
> introduced to accommodate for that.
> 
> The INA234 has four bits of the bus voltage, of the shunt voltage, and
> of the current registers zero-reserved but the latter two were
> implemented by choosing a 16x higher conversion constant instead of a
> separate field specifying a bit shift.
> 
> For consistency and simplicity, drop bus_voltage_shift and adapt the
> conversion constants for INA219 and INA234 accordingly, yielding the
> same measurement values.
> 

This isn't about simplicity, it is about correctness.

The datasheet for INA234 says for the lower 4 bits:

     Always returns 0. Remove these bits from the full result by doing a
     right arithmetic shift

which is what we should do for all chips with reserved bits instead of
assuming that the reserved bits always return 0.

Thanks,
Guenter

> Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
> ---
>   drivers/hwmon/ina2xx.c | 15 ++++-----------
>   1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 836e15a5a780..d7c894d7353c 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -151,7 +151,6 @@ struct ina2xx_config {
>   	bool has_update_interval;
>   	int calibration_value;
>   	int shunt_div;
> -	int bus_voltage_shift;
>   	int bus_voltage_lsb;	/* uV */
>   	int power_lsb_factor;
>   };
> @@ -172,8 +171,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>   		.config_default = INA219_CONFIG_DEFAULT,
>   		.calibration_value = 4096,
>   		.shunt_div = 100,
> -		.bus_voltage_shift = 3,
> -		.bus_voltage_lsb = 4000,
> +		.bus_voltage_lsb = 500,
>   		.power_lsb_factor = 20,
>   		.has_alerts = false,
>   		.has_ishunt = false,
> @@ -184,7 +182,6 @@ static const struct ina2xx_config ina2xx_config[] = {
>   		.config_default = INA226_CONFIG_DEFAULT,
>   		.calibration_value = 2048,
>   		.shunt_div = 400,
> -		.bus_voltage_shift = 0,
>   		.bus_voltage_lsb = 1250,
>   		.power_lsb_factor = 25,
>   		.has_alerts = true,
> @@ -196,8 +193,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>   		.config_default = INA226_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,
> +		.bus_voltage_lsb = 1600,
>   		.power_lsb_factor = 32,
>   		.has_alerts = true,
>   		.has_ishunt = false,
> @@ -207,7 +203,6 @@ static const struct ina2xx_config ina2xx_config[] = {
>   	[ina260] = {
>   		.config_default = INA260_CONFIG_DEFAULT,
>   		.shunt_div = 400,
> -		.bus_voltage_shift = 0,
>   		.bus_voltage_lsb = 1250,
>   		.power_lsb_factor = 8,
>   		.has_alerts = true,
> @@ -219,7 +214,6 @@ static const struct ina2xx_config ina2xx_config[] = {
>   		.config_default = SY24655_CONFIG_DEFAULT,
>   		.calibration_value = 4096,
>   		.shunt_div = 400,
> -		.bus_voltage_shift = 0,
>   		.bus_voltage_lsb = 1250,
>   		.power_lsb_factor = 25,
>   		.has_alerts = true,
> @@ -281,8 +275,7 @@ static int ina2xx_get_value(struct ina2xx_data *data, u8 reg,
>   		val = DIV_ROUND_CLOSEST((s16)regval, data->config->shunt_div);
>   		break;
>   	case INA2XX_BUS_VOLTAGE:
> -		val = (regval >> data->config->bus_voltage_shift) *
> -		  data->config->bus_voltage_lsb;
> +		val = regval * data->config->bus_voltage_lsb;
>   		val = DIV_ROUND_CLOSEST(val, 1000);
>   		break;
>   	case INA2XX_POWER:
> @@ -387,7 +380,7 @@ static u16 ina226_alert_to_reg(struct ina2xx_data *data, int reg, long val)
>   		return clamp_val(val, 0, SHRT_MAX);
>   	case INA2XX_BUS_VOLTAGE:
>   		val = clamp_val(val, 0, 200000);
> -		val = (val * 1000) << data->config->bus_voltage_shift;
> +		val = val * 1000;
>   		val = DIV_ROUND_CLOSEST(val, data->config->bus_voltage_lsb);
>   		return clamp_val(val, 0, USHRT_MAX);
>   	case INA2XX_POWER:
> 
> ---
> base-commit: f08c5de5f61a117ba5326d3d5b86e884077da2d0
> change-id: 20260302-ina2xx-shift-89ed2c5d2e72
> 
> Best regards,
> --
> Jonas Rebmann <jre@pengutronix.de>
>
Re: [PATCH] (ina2xx) Drop bus_voltage_shift configuration
Posted by Jonas Rebmann 1 month, 1 week ago 
Hi Guenter,

On 2026-03-02 16:50, Guenter Roeck wrote:
> Hi,
> 
> On 3/2/26 07:26, Jonas Rebmann wrote:
>> The INA219 has the lowest three bits of the bus voltage register
>> zero-reserved and the bus_voltage_shift ina2xx_config field was
>> introduced to accommodate for that.
>>
>> The INA234 has four bits of the bus voltage, of the shunt voltage, and
>> of the current registers zero-reserved but the latter two were
>> implemented by choosing a 16x higher conversion constant instead of a
>> separate field specifying a bit shift.
>>
>> For consistency and simplicity, drop bus_voltage_shift and adapt the
>> conversion constants for INA219 and INA234 accordingly, yielding the
>> same measurement values.
>>
> 
> This isn't about simplicity, it is about correctness.
> 
> The datasheet for INA234 says for the lower 4 bits:
> 
>       Always returns 0. Remove these bits from the full result by doing a
>       right arithmetic shift
> 
> which is what we should do for all chips with reserved bits instead of
> assuming that the reserved bits always return 0.

It says that for the reserved-zero bits at the lower end of the shunt
voltage and current registers of INA234 as well as for the bus voltage
register.

If bus_voltage_shift is kept, wouldn't a shunt_voltage_shift and
current_shift need to be introduced to support those registers of the
INA234 with the same notion of correctness?

Regards,
Jonas

> Thanks,
> Guenter
> 
>> Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
>> ---
>>    drivers/hwmon/ina2xx.c | 15 ++++-----------
>>    1 file changed, 4 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
>> index 836e15a5a780..d7c894d7353c 100644
>> --- a/drivers/hwmon/ina2xx.c
>> +++ b/drivers/hwmon/ina2xx.c
>> @@ -151,7 +151,6 @@ struct ina2xx_config {
>>    	bool has_update_interval;
>>    	int calibration_value;
>>    	int shunt_div;
>> -	int bus_voltage_shift;
>>    	int bus_voltage_lsb;	/* uV */
>>    	int power_lsb_factor;
>>    };
>> @@ -172,8 +171,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>>    		.config_default = INA219_CONFIG_DEFAULT,
>>    		.calibration_value = 4096,
>>    		.shunt_div = 100,
>> -		.bus_voltage_shift = 3,
>> -		.bus_voltage_lsb = 4000,
>> +		.bus_voltage_lsb = 500,
>>    		.power_lsb_factor = 20,
>>    		.has_alerts = false,
>>    		.has_ishunt = false,
>> @@ -184,7 +182,6 @@ static const struct ina2xx_config ina2xx_config[] = {
>>    		.config_default = INA226_CONFIG_DEFAULT,
>>    		.calibration_value = 2048,
>>    		.shunt_div = 400,
>> -		.bus_voltage_shift = 0,
>>    		.bus_voltage_lsb = 1250,
>>    		.power_lsb_factor = 25,
>>    		.has_alerts = true,
>> @@ -196,8 +193,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>>    		.config_default = INA226_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,
>> +		.bus_voltage_lsb = 1600,
>>    		.power_lsb_factor = 32,
>>    		.has_alerts = true,
>>    		.has_ishunt = false,
>> @@ -207,7 +203,6 @@ static const struct ina2xx_config ina2xx_config[] = {
>>    	[ina260] = {
>>    		.config_default = INA260_CONFIG_DEFAULT,
>>    		.shunt_div = 400,
>> -		.bus_voltage_shift = 0,
>>    		.bus_voltage_lsb = 1250,
>>    		.power_lsb_factor = 8,
>>    		.has_alerts = true,
>> @@ -219,7 +214,6 @@ static const struct ina2xx_config ina2xx_config[] = {
>>    		.config_default = SY24655_CONFIG_DEFAULT,
>>    		.calibration_value = 4096,
>>    		.shunt_div = 400,
>> -		.bus_voltage_shift = 0,
>>    		.bus_voltage_lsb = 1250,
>>    		.power_lsb_factor = 25,
>>    		.has_alerts = true,
>> @@ -281,8 +275,7 @@ static int ina2xx_get_value(struct ina2xx_data *data, u8 reg,
>>    		val = DIV_ROUND_CLOSEST((s16)regval, data->config->shunt_div);
>>    		break;
>>    	case INA2XX_BUS_VOLTAGE:
>> -		val = (regval >> data->config->bus_voltage_shift) *
>> -		  data->config->bus_voltage_lsb;
>> +		val = regval * data->config->bus_voltage_lsb;
>>    		val = DIV_ROUND_CLOSEST(val, 1000);
>>    		break;
>>    	case INA2XX_POWER:
>> @@ -387,7 +380,7 @@ static u16 ina226_alert_to_reg(struct ina2xx_data *data, int reg, long val)
>>    		return clamp_val(val, 0, SHRT_MAX);
>>    	case INA2XX_BUS_VOLTAGE:
>>    		val = clamp_val(val, 0, 200000);
>> -		val = (val * 1000) << data->config->bus_voltage_shift;
>> +		val = val * 1000;
>>    		val = DIV_ROUND_CLOSEST(val, data->config->bus_voltage_lsb);
>>    		return clamp_val(val, 0, USHRT_MAX);
>>    	case INA2XX_POWER:
>>


-- 
Pengutronix e.K.                           | Jonas Rebmann               |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-9    |
Re: [PATCH] (ina2xx) Drop bus_voltage_shift configuration
Posted by Guenter Roeck 1 month, 1 week ago 
On 3/2/26 08:02, Jonas Rebmann wrote:
> Hi Guenter,
> 
> On 2026-03-02 16:50, Guenter Roeck wrote:
>> Hi,
>>
>> On 3/2/26 07:26, Jonas Rebmann wrote:
>>> The INA219 has the lowest three bits of the bus voltage register
>>> zero-reserved and the bus_voltage_shift ina2xx_config field was
>>> introduced to accommodate for that.
>>>
>>> The INA234 has four bits of the bus voltage, of the shunt voltage, and
>>> of the current registers zero-reserved but the latter two were
>>> implemented by choosing a 16x higher conversion constant instead of a
>>> separate field specifying a bit shift.
>>>
>>> For consistency and simplicity, drop bus_voltage_shift and adapt the
>>> conversion constants for INA219 and INA234 accordingly, yielding the
>>> same measurement values.
>>>
>>
>> This isn't about simplicity, it is about correctness.
>>
>> The datasheet for INA234 says for the lower 4 bits:
>>
>>       Always returns 0. Remove these bits from the full result by doing a
>>       right arithmetic shift
>>
>> which is what we should do for all chips with reserved bits instead of
>> assuming that the reserved bits always return 0.
> 
> It says that for the reserved-zero bits at the lower end of the shunt
> voltage and current registers of INA234 as well as for the bus voltage
> register.
> 
> If bus_voltage_shift is kept, wouldn't a shunt_voltage_shift and
> current_shift need to be introduced to support those registers of the
> INA234 with the same notion of correctness?
> 

shunt_voltage_shift, yes, but the current register is not used,
so a current_shift would be unnecessary.

Thanks,
Guenter

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.