Re: [PATCH] (ina2xx) Drop bus_voltage_shift configuration

[Jonas Rebmann <jre@pengutronix.de>]

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    |