From: Wenliang <wenliang202407@163.com>
Add support for SQ52206 to the Ina238 driver. Add registers,
add calculation formulas, increase compatibility, adjust
struct and chip initialization.
Signed-off-by: Wenliang <wenliang202407@163.com>
---
Incorporate four additional registers to the original register
set of SQ52206 beyond INA238.
The ADC measurement range of SQ52206 is divided into 1/2/4,
so change the configuration of INA238_ADC_CONFIG.
SQ52206's calculation of power read values is different from
INA238.Add new value of BUS_VOLTAGE_LSB and DIE-TEMP_LSB for
SQ52206. As a result of these changes, modify both the power
and temperature read and write operations.
Add kind and struct ina238_config *config in struct ina238_data
to save the chip type and different configurations for each
chip type, promoting program reusability.
Due to the temperature reading of SQ52206 being a signed 16 bit
value, while INA238 is a 12 bit value. So we changed the
temperature reading function.
Extract the chip initialization process into a separate function
named ina238_init to facilitate adjustments for various chip types.
Add a corresponding compatible to the driver.
drivers/hwmon/ina238.c | 209 ++++++++++++++++++++++++++++-------------
1 file changed, 144 insertions(+), 65 deletions(-)
diff --git a/drivers/hwmon/ina238.c b/drivers/hwmon/ina238.c
index 2d9f12f68d50..131f5faefdb3 100644
--- a/drivers/hwmon/ina238.c
+++ b/drivers/hwmon/ina238.c
@@ -21,11 +21,14 @@
#define INA238_CONFIG 0x0
#define INA238_ADC_CONFIG 0x1
#define INA238_SHUNT_CALIBRATION 0x2
+#define SQ52206_SHUNT_TEMPCO 0x3
#define INA238_SHUNT_VOLTAGE 0x4
#define INA238_BUS_VOLTAGE 0x5
#define INA238_DIE_TEMP 0x6
#define INA238_CURRENT 0x7
#define INA238_POWER 0x8
+#define SQ52206_ENERGY 0x9
+#define SQ52206_CHARGE 0xa
#define INA238_DIAG_ALERT 0xb
#define INA238_SHUNT_OVER_VOLTAGE 0xc
#define INA238_SHUNT_UNDER_VOLTAGE 0xd
@@ -33,9 +36,12 @@
#define INA238_BUS_UNDER_VOLTAGE 0xf
#define INA238_TEMP_LIMIT 0x10
#define INA238_POWER_LIMIT 0x11
+#define SQ52206_POWER_PEAK 0x20
#define INA238_DEVICE_ID 0x3f /* not available on INA237 */
#define INA238_CONFIG_ADCRANGE BIT(4)
+#define SQ52206_CONFIG_ADCRANGE GENMASK(4, 3)
+#define SQ52206_READ_ADCRANGE(reg) FIELD_GET(SQ52206_CONFIG_ADCRANGE, reg)
#define INA238_DIAG_ALERT_TMPOL BIT(7)
#define INA238_DIAG_ALERT_SHNTOL BIT(6)
@@ -44,7 +50,7 @@
#define INA238_DIAG_ALERT_BUSUL BIT(3)
#define INA238_DIAG_ALERT_POL BIT(2)
-#define INA238_REGISTERS 0x11
+#define INA238_MAX_REGISTERS 0x20
#define INA238_RSHUNT_DEFAULT 10000 /* uOhm */
@@ -87,27 +93,63 @@
* shunt = 0x4000 / (819.2 * 10^6) / 0.001 = 20000 uOhms (with 1mA/lsb)
*
* Current (mA) = register value * 20000 / rshunt / 4 * gain
- * Power (W) = 0.2 * register value * 20000 / rshunt / 4 * gain
+ * Power (mW) = 0.2 * register value * 20000 / rshunt / 4 * gain
+ * (Specific for SQ52206)
+ * Power (mW) = 0.24 * register value * 20000 / rshunt / 4 * gain
*/
#define INA238_CALIBRATION_VALUE 16384
-#define INA238_FIXED_SHUNT 20000
+#define INA238_FIXED_SHUNT 20000
#define INA238_SHUNT_VOLTAGE_LSB 5 /* 5 uV/lsb */
#define INA238_BUS_VOLTAGE_LSB 3125 /* 3.125 mV/lsb */
-#define INA238_DIE_TEMP_LSB 125 /* 125 mC/lsb */
+#define INA238_DIE_TEMP_LSB 1250000 /* 125 mC/lsb */
+#define SQ52206_BUS_VOLTAGE_LSB 3750 /* 3.75 mV/lsb */
+#define SQ52206_DIE_TEMP_LSB 78125 /* 7.8125 mC/lsb */
static const struct regmap_config ina238_regmap_config = {
- .max_register = INA238_REGISTERS,
+ .max_register = INA238_MAX_REGISTERS,
.reg_bits = 8,
.val_bits = 16,
};
+enum ina238_ids { ina238, ina237, sq52206 };
+
+struct ina238_config {
+ u16 config_default;
+ int calibration_value;
+ int bus_voltage_lsb; /* uV */
+ int temp_lsb; /* mC */
+};
+
struct ina238_data {
+ const struct ina238_config *config;
struct i2c_client *client;
struct mutex config_lock;
struct regmap *regmap;
u32 rshunt;
int gain;
+ int kind;
+};
+
+static const struct ina238_config ina238_config[] = {
+ [ina238] = {
+ .config_default = INA238_CONFIG_DEFAULT,
+ .calibration_value = INA238_CALIBRATION_VALUE,
+ .bus_voltage_lsb = INA238_BUS_VOLTAGE_LSB,
+ .temp_lsb = INA238_DIE_TEMP_LSB,
+ },
+ [ina237] = {
+ .config_default = INA238_CONFIG_DEFAULT,
+ .calibration_value = INA238_CALIBRATION_VALUE,
+ .bus_voltage_lsb = INA238_BUS_VOLTAGE_LSB,
+ .temp_lsb = INA238_DIE_TEMP_LSB,
+ },
+ [sq52206] = {
+ .config_default = INA238_CONFIG_DEFAULT,
+ .calibration_value = INA238_CALIBRATION_VALUE,
+ .bus_voltage_lsb = SQ52206_BUS_VOLTAGE_LSB,
+ .temp_lsb = SQ52206_DIE_TEMP_LSB,
+ },
};
static int ina238_read_reg24(const struct i2c_client *client, u8 reg, u32 *val)
@@ -197,10 +239,10 @@ static int ina238_read_in(struct device *dev, u32 attr, int channel,
regval = (s16)regval;
if (channel == 0)
/* gain of 1 -> LSB / 4 */
- *val = (regval * INA238_SHUNT_VOLTAGE_LSB) /
- (1000 * (4 - data->gain + 1));
+ *val = (regval * INA238_SHUNT_VOLTAGE_LSB) *
+ data->gain / (1000 * 4);
else
- *val = (regval * INA238_BUS_VOLTAGE_LSB) / 1000;
+ *val = (regval * data->config->bus_voltage_lsb) / 1000;
break;
case hwmon_in_max_alarm:
case hwmon_in_min_alarm:
@@ -225,8 +267,8 @@ static int ina238_write_in(struct device *dev, u32 attr, int channel,
case 0:
/* signed value, clamp to max range +/-163 mV */
regval = clamp_val(val, -163, 163);
- regval = (regval * 1000 * (4 - data->gain + 1)) /
- INA238_SHUNT_VOLTAGE_LSB;
+ regval = (regval * 1000 * 4) /
+ INA238_SHUNT_VOLTAGE_LSB * data->gain;
regval = clamp_val(regval, S16_MIN, S16_MAX);
switch (attr) {
@@ -242,7 +284,7 @@ static int ina238_write_in(struct device *dev, u32 attr, int channel,
case 1:
/* signed value, positive values only. Clamp to max 102.396 V */
regval = clamp_val(val, 0, 102396);
- regval = (regval * 1000) / INA238_BUS_VOLTAGE_LSB;
+ regval = (regval * 1000) / data->config->bus_voltage_lsb;
regval = clamp_val(regval, 0, S16_MAX);
switch (attr) {
@@ -295,10 +337,14 @@ static int ina238_read_power(struct device *dev, u32 attr, long *val)
err = ina238_read_reg24(data->client, INA238_POWER, ®val);
if (err)
return err;
-
- /* Fixed 1mA lsb, scaled by 1000000 to have result in uW */
- power = div_u64(regval * 1000ULL * INA238_FIXED_SHUNT *
- data->gain, 20 * data->rshunt);
+ if (data->kind == sq52206)
+ /* Fixed 1mA lsb, scaled by 1000000 to have result in uW */
+ power = div_u64(regval * 1200ULL * INA238_FIXED_SHUNT *
+ data->gain, 20 * data->rshunt);
+ else
+ /* Fixed 1mA lsb, scaled by 1000000 to have result in uW */
+ power = div_u64(regval * 1000ULL * INA238_FIXED_SHUNT *
+ data->gain, 20 * data->rshunt);
/* Clamp value to maximum value of long */
*val = clamp_val(power, 0, LONG_MAX);
break;
@@ -311,8 +357,12 @@ static int ina238_read_power(struct device *dev, u32 attr, long *val)
* Truncated 24-bit compare register, lower 8-bits are
* truncated. Same conversion to/from uW as POWER register.
*/
- power = div_u64((regval << 8) * 1000ULL * INA238_FIXED_SHUNT *
- data->gain, 20 * data->rshunt);
+ if (data->kind == sq52206)
+ power = div_u64((regval << 8) * 1200ULL * INA238_FIXED_SHUNT *
+ data->gain, 20 * data->rshunt);
+ else
+ power = div_u64((regval << 8) * 1000ULL * INA238_FIXED_SHUNT *
+ data->gain, 20 * data->rshunt);
/* Clamp value to maximum value of long */
*val = clamp_val(power, 0, LONG_MAX);
break;
@@ -344,8 +394,12 @@ static int ina238_write_power(struct device *dev, u32 attr, long val)
* register.
*/
regval = clamp_val(val, 0, LONG_MAX);
- regval = div_u64(val * 20ULL * data->rshunt,
- 1000ULL * INA238_FIXED_SHUNT * data->gain);
+ if (data->kind == sq52206)
+ regval = div_u64(val * 20ULL * data->rshunt,
+ 1000ULL * INA238_FIXED_SHUNT * data->gain);
+ else
+ regval = div_u64(val * 24ULL * data->rshunt,
+ 1000ULL * INA238_FIXED_SHUNT * data->gain);
regval = clamp_val(regval >> 8, 0, U16_MAX);
return regmap_write(data->regmap, INA238_POWER_LIMIT, regval);
@@ -362,17 +416,29 @@ static int ina238_read_temp(struct device *dev, u32 attr, long *val)
err = regmap_read(data->regmap, INA238_DIE_TEMP, ®val);
if (err)
return err;
-
+ if (data->kind == sq52206)
+ /* Signed, bits 15-0 of register, result in mC */
+ regval = div_s64((s16)regval * data->config->temp_lsb,
+ 10000);
+ else
/* Signed, bits 15-4 of register, result in mC */
- *val = ((s16)regval >> 4) * INA238_DIE_TEMP_LSB;
+ *val = div_s64(((s16)regval >> 4) *
+ data->config->temp_lsb, 10000);
+ *val = clamp_val(regval, S16_MIN, S16_MAX);
break;
case hwmon_temp_max:
err = regmap_read(data->regmap, INA238_TEMP_LIMIT, ®val);
if (err)
return err;
-
- /* Signed, bits 15-4 of register, result in mC */
- *val = ((s16)regval >> 4) * INA238_DIE_TEMP_LSB;
+ if (data->kind == sq52206)
+ /* Signed, bits 15-0 of register, result in mC */
+ regval = div_s64((s16)regval * data->config->temp_lsb,
+ 10000);
+ else
+ /* Signed, bits 15-4 of register, result in mC */
+ *val = div_s64(((s16)regval >> 4) *
+ data->config->temp_lsb, 10000);
+ *val = clamp_val(regval, S16_MIN, S16_MAX);
break;
case hwmon_temp_max_alarm:
err = regmap_read(data->regmap, INA238_DIAG_ALERT, ®val);
@@ -396,9 +462,14 @@ static int ina238_write_temp(struct device *dev, u32 attr, long val)
if (attr != hwmon_temp_max)
return -EOPNOTSUPP;
- /* Signed, bits 15-4 of register */
- regval = (val / INA238_DIE_TEMP_LSB) << 4;
- regval = clamp_val(regval, S16_MIN, S16_MAX) & 0xfff0;
+ if (data->kind == sq52206) {
+ regval = div_u64(val*10000, data->config->temp_lsb);
+ regval = clamp_val(regval, S16_MIN, S16_MAX);
+ } else {
+ /* Signed, bits 15-4 of register */
+ regval = div_u64(val*10000, data->config->temp_lsb) << 4;
+ regval = clamp_val(regval, S16_MIN, S16_MAX) & 0xfff0;
+ }
return regmap_write(data->regmap, INA238_TEMP_LIMIT, regval);
}
@@ -530,20 +601,58 @@ static const struct hwmon_chip_info ina238_chip_info = {
.info = ina238_info,
};
+/*
+ * Initialize the configuration and calibration registers.
+ */
+static int ina238_init(struct ina238_data *data)
+{
+ int ret = 0;
+ int config;
+ /* Setup CONFIG register */
+ config = data->config->config_default;
+ if (data->gain == 1)
+ config |= INA238_CONFIG_ADCRANGE; /* ADCRANGE = 1 is /1 */
+ ret = regmap_write(data->regmap, INA238_CONFIG, config);
+ if (ret < 0)
+ return ret;
+ /* Setup ADC_CONFIG register */
+ ret = regmap_write(data->regmap, INA238_ADC_CONFIG,
+ INA238_ADC_CONFIG_DEFAULT);
+ if (ret < 0)
+ return ret;
+ /* Setup SHUNT_CALIBRATION register with fixed value */
+ ret = regmap_write(data->regmap, INA238_SHUNT_CALIBRATION,
+ data->config->calibration_value);
+ if (ret < 0)
+ return ret;
+ /* Setup alert/alarm configuration */
+ ret = regmap_write(data->regmap, INA238_DIAG_ALERT,
+ INA238_DIAG_ALERT_DEFAULT);
+ if (ret < 0)
+ return ret;
+ return 0;
+}
+static const struct i2c_device_id ina238_id[];
static int ina238_probe(struct i2c_client *client)
{
struct ina2xx_platform_data *pdata = dev_get_platdata(&client->dev);
struct device *dev = &client->dev;
struct device *hwmon_dev;
struct ina238_data *data;
- int config;
+ enum ina238_ids chip;
int ret;
+ chip = (uintptr_t)i2c_get_match_data(client);
+
data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
if (!data)
return -ENOMEM;
+ data->kind = chip;
data->client = client;
+ /* set the device type */
+ data->config = &ina238_config[data->kind];
+
mutex_init(&data->config_lock);
data->regmap = devm_regmap_init_i2c(client, &ina238_regmap_config);
@@ -564,48 +673,15 @@ static int ina238_probe(struct i2c_client *client)
/* load shunt gain value */
if (device_property_read_u32(dev, "ti,shunt-gain", &data->gain) < 0)
data->gain = 4; /* Default of ADCRANGE = 0 */
- if (data->gain != 1 && data->gain != 4) {
+ if (data->gain != 1 && data->gain != 2 && data->gain != 4) {
dev_err(dev, "invalid shunt gain value %u\n", data->gain);
return -EINVAL;
}
-
- /* Setup CONFIG register */
- config = INA238_CONFIG_DEFAULT;
- if (data->gain == 1)
- config |= INA238_CONFIG_ADCRANGE; /* ADCRANGE = 1 is /1 */
- ret = regmap_write(data->regmap, INA238_CONFIG, config);
- if (ret < 0) {
- dev_err(dev, "error configuring the device: %d\n", ret);
- return -ENODEV;
- }
-
- /* Setup ADC_CONFIG register */
- ret = regmap_write(data->regmap, INA238_ADC_CONFIG,
- INA238_ADC_CONFIG_DEFAULT);
- if (ret < 0) {
- dev_err(dev, "error configuring the device: %d\n", ret);
- return -ENODEV;
- }
-
- /* Setup SHUNT_CALIBRATION register with fixed value */
- ret = regmap_write(data->regmap, INA238_SHUNT_CALIBRATION,
- INA238_CALIBRATION_VALUE);
- if (ret < 0) {
- dev_err(dev, "error configuring the device: %d\n", ret);
- return -ENODEV;
- }
-
- /* Setup alert/alarm configuration */
- ret = regmap_write(data->regmap, INA238_DIAG_ALERT,
- INA238_DIAG_ALERT_DEFAULT);
- if (ret < 0) {
- dev_err(dev, "error configuring the device: %d\n", ret);
- return -ENODEV;
- }
+ ret = ina238_init(data);
hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data,
&ina238_chip_info,
- NULL);
+ ina238_groups);
if (IS_ERR(hwmon_dev))
return PTR_ERR(hwmon_dev);
@@ -616,7 +692,9 @@ static int ina238_probe(struct i2c_client *client)
}
static const struct i2c_device_id ina238_id[] = {
- { "ina238" },
+ { "ina238", ina238 },
+ { "ina237", ina237 },
+ { "sq52206", sq52206 },
{ }
};
MODULE_DEVICE_TABLE(i2c, ina238_id);
@@ -624,6 +702,7 @@ MODULE_DEVICE_TABLE(i2c, ina238_id);
static const struct of_device_id __maybe_unused ina238_of_match[] = {
{ .compatible = "ti,ina237" },
{ .compatible = "ti,ina238" },
+ { .compatible = "silergy,sq52206" },
{ },
};
MODULE_DEVICE_TABLE(of, ina238_of_match);
--
2.43.0On 12/23/24 22:35, Wenliang Yan wrote:
> From: Wenliang <wenliang202407@163.com>
>
> Add support for SQ52206 to the Ina238 driver. Add registers,
> add calculation formulas, increase compatibility, adjust
> struct and chip initialization.
>
> Signed-off-by: Wenliang <wenliang202407@163.com>
> ---
>
> Incorporate four additional registers to the original register
> set of SQ52206 beyond INA238.
> The ADC measurement range of SQ52206 is divided into 1/2/4,
> so change the configuration of INA238_ADC_CONFIG.
> SQ52206's calculation of power read values is different from
> INA238.Add new value of BUS_VOLTAGE_LSB and DIE-TEMP_LSB for
> SQ52206. As a result of these changes, modify both the power
> and temperature read and write operations.
> Add kind and struct ina238_config *config in struct ina238_data
> to save the chip type and different configurations for each
> chip type, promoting program reusability.
> Due to the temperature reading of SQ52206 being a signed 16 bit
> value, while INA238 is a 12 bit value. So we changed the
> temperature reading function.
> Extract the chip initialization process into a separate function
> named ina238_init to facilitate adjustments for various chip types.
> Add a corresponding compatible to the driver.
>
> drivers/hwmon/ina238.c | 209 ++++++++++++++++++++++++++++-------------
> 1 file changed, 144 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/hwmon/ina238.c b/drivers/hwmon/ina238.c
> index 2d9f12f68d50..131f5faefdb3 100644
> --- a/drivers/hwmon/ina238.c
> +++ b/drivers/hwmon/ina238.c
> @@ -21,11 +21,14 @@
> #define INA238_CONFIG 0x0
> #define INA238_ADC_CONFIG 0x1
> #define INA238_SHUNT_CALIBRATION 0x2
> +#define SQ52206_SHUNT_TEMPCO 0x3
> #define INA238_SHUNT_VOLTAGE 0x4
> #define INA238_BUS_VOLTAGE 0x5
> #define INA238_DIE_TEMP 0x6
> #define INA238_CURRENT 0x7
> #define INA238_POWER 0x8
> +#define SQ52206_ENERGY 0x9
> +#define SQ52206_CHARGE 0xa
> #define INA238_DIAG_ALERT 0xb
> #define INA238_SHUNT_OVER_VOLTAGE 0xc
> #define INA238_SHUNT_UNDER_VOLTAGE 0xd
> @@ -33,9 +36,12 @@
> #define INA238_BUS_UNDER_VOLTAGE 0xf
> #define INA238_TEMP_LIMIT 0x10
> #define INA238_POWER_LIMIT 0x11
> +#define SQ52206_POWER_PEAK 0x20
> #define INA238_DEVICE_ID 0x3f /* not available on INA237 */
>
> #define INA238_CONFIG_ADCRANGE BIT(4)
> +#define SQ52206_CONFIG_ADCRANGE GENMASK(4, 3)
> +#define SQ52206_READ_ADCRANGE(reg) FIELD_GET(SQ52206_CONFIG_ADCRANGE, reg)
>
> #define INA238_DIAG_ALERT_TMPOL BIT(7)
> #define INA238_DIAG_ALERT_SHNTOL BIT(6)
> @@ -44,7 +50,7 @@
> #define INA238_DIAG_ALERT_BUSUL BIT(3)
> #define INA238_DIAG_ALERT_POL BIT(2)
>
> -#define INA238_REGISTERS 0x11
> +#define INA238_MAX_REGISTERS 0x20
Why this change ?
>
> #define INA238_RSHUNT_DEFAULT 10000 /* uOhm */
>
> @@ -87,27 +93,63 @@
> * shunt = 0x4000 / (819.2 * 10^6) / 0.001 = 20000 uOhms (with 1mA/lsb)
> *
> * Current (mA) = register value * 20000 / rshunt / 4 * gain
> - * Power (W) = 0.2 * register value * 20000 / rshunt / 4 * gain
> + * Power (mW) = 0.2 * register value * 20000 / rshunt / 4 * gain
> + * (Specific for SQ52206)
> + * Power (mW) = 0.24 * register value * 20000 / rshunt / 4 * gain
> */
> #define INA238_CALIBRATION_VALUE 16384
> -#define INA238_FIXED_SHUNT 20000
> +#define INA238_FIXED_SHUNT 20000
Why this change ?
>
> #define INA238_SHUNT_VOLTAGE_LSB 5 /* 5 uV/lsb */
> #define INA238_BUS_VOLTAGE_LSB 3125 /* 3.125 mV/lsb */
> -#define INA238_DIE_TEMP_LSB 125 /* 125 mC/lsb */
> +#define INA238_DIE_TEMP_LSB 1250000 /* 125 mC/lsb */
> +#define SQ52206_BUS_VOLTAGE_LSB 3750 /* 3.75 mV/lsb */
> +#define SQ52206_DIE_TEMP_LSB 78125 /* 7.8125 mC/lsb */
>
> static const struct regmap_config ina238_regmap_config = {
> - .max_register = INA238_REGISTERS,
> + .max_register = INA238_MAX_REGISTERS,
> .reg_bits = 8,
> .val_bits = 16,
> };
>
> +enum ina238_ids { ina238, ina237, sq52206 };
> +
> +struct ina238_config {
> + u16 config_default;
> + int calibration_value;
.config_default and .calibration_value is the same for all chips.
I don't see the point of not just using the respective defines.
> + int bus_voltage_lsb; /* uV */
> + int temp_lsb; /* mC */
> +};
> +
> struct ina238_data {
> + const struct ina238_config *config;
> struct i2c_client *client;
> struct mutex config_lock;
> struct regmap *regmap;
> u32 rshunt;
> int gain;
> + int kind;
> +};
> +
> +static const struct ina238_config ina238_config[] = {
> + [ina238] = {
> + .config_default = INA238_CONFIG_DEFAULT,
> + .calibration_value = INA238_CALIBRATION_VALUE,
> + .bus_voltage_lsb = INA238_BUS_VOLTAGE_LSB,
> + .temp_lsb = INA238_DIE_TEMP_LSB,
> + },
> + [ina237] = {
> + .config_default = INA238_CONFIG_DEFAULT,
> + .calibration_value = INA238_CALIBRATION_VALUE,
> + .bus_voltage_lsb = INA238_BUS_VOLTAGE_LSB,
> + .temp_lsb = INA238_DIE_TEMP_LSB,
> + },
> + [sq52206] = {
> + .config_default = INA238_CONFIG_DEFAULT,
> + .calibration_value = INA238_CALIBRATION_VALUE,
> + .bus_voltage_lsb = SQ52206_BUS_VOLTAGE_LSB,
> + .temp_lsb = SQ52206_DIE_TEMP_LSB,
> + },
> };
>
> static int ina238_read_reg24(const struct i2c_client *client, u8 reg, u32 *val)
> @@ -197,10 +239,10 @@ static int ina238_read_in(struct device *dev, u32 attr, int channel,
> regval = (s16)regval;
> if (channel == 0)
> /* gain of 1 -> LSB / 4 */
> - *val = (regval * INA238_SHUNT_VOLTAGE_LSB) /
> - (1000 * (4 - data->gain + 1));
> + *val = (regval * INA238_SHUNT_VOLTAGE_LSB) *
> + data->gain / (1000 * 4);
Why this change ?
> else
> - *val = (regval * INA238_BUS_VOLTAGE_LSB) / 1000;
> + *val = (regval * data->config->bus_voltage_lsb) / 1000;
> break;
> case hwmon_in_max_alarm:
> case hwmon_in_min_alarm:
> @@ -225,8 +267,8 @@ static int ina238_write_in(struct device *dev, u32 attr, int channel,
> case 0:
> /* signed value, clamp to max range +/-163 mV */
> regval = clamp_val(val, -163, 163);
> - regval = (regval * 1000 * (4 - data->gain + 1)) /
> - INA238_SHUNT_VOLTAGE_LSB;
> + regval = (regval * 1000 * 4) /
> + INA238_SHUNT_VOLTAGE_LSB * data->gain;
Why this change ?
> regval = clamp_val(regval, S16_MIN, S16_MAX);
>
> switch (attr) {
> @@ -242,7 +284,7 @@ static int ina238_write_in(struct device *dev, u32 attr, int channel,
> case 1:
> /* signed value, positive values only. Clamp to max 102.396 V */
> regval = clamp_val(val, 0, 102396);
> - regval = (regval * 1000) / INA238_BUS_VOLTAGE_LSB;
> + regval = (regval * 1000) / data->config->bus_voltage_lsb;
> regval = clamp_val(regval, 0, S16_MAX);
>
> switch (attr) {
> @@ -295,10 +337,14 @@ static int ina238_read_power(struct device *dev, u32 attr, long *val)
> err = ina238_read_reg24(data->client, INA238_POWER, ®val);
> if (err)
> return err;
> -
> - /* Fixed 1mA lsb, scaled by 1000000 to have result in uW */
> - power = div_u64(regval * 1000ULL * INA238_FIXED_SHUNT *
> - data->gain, 20 * data->rshunt);
> + if (data->kind == sq52206)
> + /* Fixed 1mA lsb, scaled by 1000000 to have result in uW */
> + power = div_u64(regval * 1200ULL * INA238_FIXED_SHUNT *
> + data->gain, 20 * data->rshunt);
> + else
> + /* Fixed 1mA lsb, scaled by 1000000 to have result in uW */
> + power = div_u64(regval * 1000ULL * INA238_FIXED_SHUNT *
> + data->gain, 20 * data->rshunt);
> /* Clamp value to maximum value of long */
> *val = clamp_val(power, 0, LONG_MAX);
> break;
> @@ -311,8 +357,12 @@ static int ina238_read_power(struct device *dev, u32 attr, long *val)
> * Truncated 24-bit compare register, lower 8-bits are
> * truncated. Same conversion to/from uW as POWER register.
> */
> - power = div_u64((regval << 8) * 1000ULL * INA238_FIXED_SHUNT *
> - data->gain, 20 * data->rshunt);
> + if (data->kind == sq52206)
> + power = div_u64((regval << 8) * 1200ULL * INA238_FIXED_SHUNT *
> + data->gain, 20 * data->rshunt);
> + else
> + power = div_u64((regval << 8) * 1000ULL * INA238_FIXED_SHUNT *
> + data->gain, 20 * data->rshunt);
> /* Clamp value to maximum value of long */
> *val = clamp_val(power, 0, LONG_MAX);
> break;
> @@ -344,8 +394,12 @@ static int ina238_write_power(struct device *dev, u32 attr, long val)
> * register.
> */
> regval = clamp_val(val, 0, LONG_MAX);
> - regval = div_u64(val * 20ULL * data->rshunt,
> - 1000ULL * INA238_FIXED_SHUNT * data->gain);
> + if (data->kind == sq52206)
> + regval = div_u64(val * 20ULL * data->rshunt,
> + 1000ULL * INA238_FIXED_SHUNT * data->gain);
> + else
> + regval = div_u64(val * 24ULL * data->rshunt,
> + 1000ULL * INA238_FIXED_SHUNT * data->gain);
There is a constant factor against data->gain for the different chips.
Why not incorporate that into data->gain, or use a chip configuration
value for it ?
> regval = clamp_val(regval >> 8, 0, U16_MAX);
>
> return regmap_write(data->regmap, INA238_POWER_LIMIT, regval);
> @@ -362,17 +416,29 @@ static int ina238_read_temp(struct device *dev, u32 attr, long *val)
> err = regmap_read(data->regmap, INA238_DIE_TEMP, ®val);
> if (err)
> return err;
> -
> + if (data->kind == sq52206)
> + /* Signed, bits 15-0 of register, result in mC */
> + regval = div_s64((s16)regval * data->config->temp_lsb,
> + 10000);
> + else
> /* Signed, bits 15-4 of register, result in mC */
> - *val = ((s16)regval >> 4) * INA238_DIE_TEMP_LSB;
> + *val = div_s64(((s16)regval >> 4) *
> + data->config->temp_lsb, 10000);
> + *val = clamp_val(regval, S16_MIN, S16_MAX);
Why is this clamp_val() now necessary ?
It might make sense to add the shift count to struct ina238_config
instead of using if/else here.
In general, using if/else for some chip specifics and struct ina238_config
for others is confusing. I'd suggest to move all chip specific information
into struct ina238_config and avoid if/else in runtime code as much as possible.
> break;
> case hwmon_temp_max:
> err = regmap_read(data->regmap, INA238_TEMP_LIMIT, ®val);
> if (err)
> return err;
> -
> - /* Signed, bits 15-4 of register, result in mC */
> - *val = ((s16)regval >> 4) * INA238_DIE_TEMP_LSB;
> + if (data->kind == sq52206)
> + /* Signed, bits 15-0 of register, result in mC */
> + regval = div_s64((s16)regval * data->config->temp_lsb,
> + 10000);
> + else
> + /* Signed, bits 15-4 of register, result in mC */
> + *val = div_s64(((s16)regval >> 4) *
> + data->config->temp_lsb, 10000);
> + *val = clamp_val(regval, S16_MIN, S16_MAX);
> break;
> case hwmon_temp_max_alarm:
> err = regmap_read(data->regmap, INA238_DIAG_ALERT, ®val);
> @@ -396,9 +462,14 @@ static int ina238_write_temp(struct device *dev, u32 attr, long val)
> if (attr != hwmon_temp_max)
> return -EOPNOTSUPP;
>
> - /* Signed, bits 15-4 of register */
> - regval = (val / INA238_DIE_TEMP_LSB) << 4;
> - regval = clamp_val(regval, S16_MIN, S16_MAX) & 0xfff0;
> + if (data->kind == sq52206) {
> + regval = div_u64(val*10000, data->config->temp_lsb);
> + regval = clamp_val(regval, S16_MIN, S16_MAX);
> + } else {
> + /* Signed, bits 15-4 of register */
> + regval = div_u64(val*10000, data->config->temp_lsb) << 4;
> + regval = clamp_val(regval, S16_MIN, S16_MAX) & 0xfff0;
> + }
>
> return regmap_write(data->regmap, INA238_TEMP_LIMIT, regval);
> }
> @@ -530,20 +601,58 @@ static const struct hwmon_chip_info ina238_chip_info = {
> .info = ina238_info,
> };
>
> +/*
> + * Initialize the configuration and calibration registers.
> + */
> +static int ina238_init(struct ina238_data *data)
> +{
> + int ret = 0;
> + int config;
> + /* Setup CONFIG register */
> + config = data->config->config_default;
> + if (data->gain == 1)
> + config |= INA238_CONFIG_ADCRANGE; /* ADCRANGE = 1 is /1 */
> + ret = regmap_write(data->regmap, INA238_CONFIG, config);
> + if (ret < 0)
> + return ret;
> + /* Setup ADC_CONFIG register */
> + ret = regmap_write(data->regmap, INA238_ADC_CONFIG,
> + INA238_ADC_CONFIG_DEFAULT);
> + if (ret < 0)
> + return ret;
> + /* Setup SHUNT_CALIBRATION register with fixed value */
> + ret = regmap_write(data->regmap, INA238_SHUNT_CALIBRATION,
> + data->config->calibration_value);
> + if (ret < 0)
> + return ret;
> + /* Setup alert/alarm configuration */
> + ret = regmap_write(data->regmap, INA238_DIAG_ALERT,
> + INA238_DIAG_ALERT_DEFAULT);
> + if (ret < 0)
> + return ret;
> + return 0;
> +}
> +static const struct i2c_device_id ina238_id[];
Please avoid forward declarations.
> static int ina238_probe(struct i2c_client *client)
> {
> struct ina2xx_platform_data *pdata = dev_get_platdata(&client->dev);
> struct device *dev = &client->dev;
> struct device *hwmon_dev;
> struct ina238_data *data;
> - int config;
> + enum ina238_ids chip;
> int ret;
>
> + chip = (uintptr_t)i2c_get_match_data(client);
> +
> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> if (!data)
> return -ENOMEM;
>
> + data->kind = chip;
> data->client = client;
> + /* set the device type */
> + data->config = &ina238_config[data->kind];
> +
> mutex_init(&data->config_lock);
>
> data->regmap = devm_regmap_init_i2c(client, &ina238_regmap_config);
> @@ -564,48 +673,15 @@ static int ina238_probe(struct i2c_client *client)
> /* load shunt gain value */
> if (device_property_read_u32(dev, "ti,shunt-gain", &data->gain) < 0)
> data->gain = 4; /* Default of ADCRANGE = 0 */
> - if (data->gain != 1 && data->gain != 4) {
> + if (data->gain != 1 && data->gain != 2 && data->gain != 4) {
Chip independent changes should be in separate patch(es).
> dev_err(dev, "invalid shunt gain value %u\n", data->gain);
> return -EINVAL;
> }
> -
> - /* Setup CONFIG register */
> - config = INA238_CONFIG_DEFAULT;
> - if (data->gain == 1)
> - config |= INA238_CONFIG_ADCRANGE; /* ADCRANGE = 1 is /1 */
> - ret = regmap_write(data->regmap, INA238_CONFIG, config);
> - if (ret < 0) {
> - dev_err(dev, "error configuring the device: %d\n", ret);
> - return -ENODEV;
> - }
> -
> - /* Setup ADC_CONFIG register */
> - ret = regmap_write(data->regmap, INA238_ADC_CONFIG,
> - INA238_ADC_CONFIG_DEFAULT);
> - if (ret < 0) {
> - dev_err(dev, "error configuring the device: %d\n", ret);
> - return -ENODEV;
> - }
> -
> - /* Setup SHUNT_CALIBRATION register with fixed value */
> - ret = regmap_write(data->regmap, INA238_SHUNT_CALIBRATION,
> - INA238_CALIBRATION_VALUE);
> - if (ret < 0) {
> - dev_err(dev, "error configuring the device: %d\n", ret);
> - return -ENODEV;
> - }
> -
> - /* Setup alert/alarm configuration */
> - ret = regmap_write(data->regmap, INA238_DIAG_ALERT,
> - INA238_DIAG_ALERT_DEFAULT);
> - if (ret < 0) {
> - dev_err(dev, "error configuring the device: %d\n", ret);
> - return -ENODEV;
> - }
> + ret = ina238_init(data);
>
Errors must not be ignored.
> hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data,
> &ina238_chip_info,
> - NULL);
> + ina238_groups);
> if (IS_ERR(hwmon_dev))
> return PTR_ERR(hwmon_dev);
>
> @@ -616,7 +692,9 @@ static int ina238_probe(struct i2c_client *client)
> }
>
> static const struct i2c_device_id ina238_id[] = {
> - { "ina238" },
> + { "ina238", ina238 },
> + { "ina237", ina237 },
Unrelated change, and out of order (ina237 should come first).
> + { "sq52206", sq52206 },
> { }
> };
> MODULE_DEVICE_TABLE(i2c, ina238_id);
> @@ -624,6 +702,7 @@ MODULE_DEVICE_TABLE(i2c, ina238_id);
> static const struct of_device_id __maybe_unused ina238_of_match[] = {
> { .compatible = "ti,ina237" },
> { .compatible = "ti,ina238" },
> + { .compatible = "silergy,sq52206" },
.data is missing.
> { },
> };
> MODULE_DEVICE_TABLE(of, ina238_of_match);
Add support for SQ52206, the datasheet depends on
https://us1.silergy.com/cloud/index/uniqid/676b659b4a503
The password is fx6NEe.
At 2025-01-07 07:22:01, "Guenter Roeck" <linux@roeck-us.net> wrote:
>On 12/23/24 22:35, Wenliang Yan wrote:
>> From: Wenliang <wenliang202407@163.com>
>>
>> Add support for SQ52206 to the Ina238 driver. Add registers,
>> add calculation formulas, increase compatibility, adjust
>> struct and chip initialization.
>>
>> Signed-off-by: Wenliang <wenliang202407@163.com>
>> ---
>>
>> Incorporate four additional registers to the original register
>> set of SQ52206 beyond INA238.
>> The ADC measurement range of SQ52206 is divided into 1/2/4,
>> so change the configuration of INA238_ADC_CONFIG.
>> SQ52206's calculation of power read values is different from
>> INA238.Add new value of BUS_VOLTAGE_LSB and DIE-TEMP_LSB for
>> SQ52206. As a result of these changes, modify both the power
>> and temperature read and write operations.
>> Add kind and struct ina238_config *config in struct ina238_data
>> to save the chip type and different configurations for each
>> chip type, promoting program reusability.
>> Due to the temperature reading of SQ52206 being a signed 16 bit
>> value, while INA238 is a 12 bit value. So we changed the
>> temperature reading function.
>> Extract the chip initialization process into a separate function
>> named ina238_init to facilitate adjustments for various chip types.
>> Add a corresponding compatible to the driver.
>>
>> drivers/hwmon/ina238.c | 209 ++++++++++++++++++++++++++++-------------
>> 1 file changed, 144 insertions(+), 65 deletions(-)
>>
>> diff --git a/drivers/hwmon/ina238.c b/drivers/hwmon/ina238.c
>> index 2d9f12f68d50..131f5faefdb3 100644
>> --- a/drivers/hwmon/ina238.c
>> +++ b/drivers/hwmon/ina238.c
>> @@ -21,11 +21,14 @@
>> #define INA238_CONFIG 0x0
>> #define INA238_ADC_CONFIG 0x1
>> #define INA238_SHUNT_CALIBRATION 0x2
>> +#define SQ52206_SHUNT_TEMPCO 0x3
>> #define INA238_SHUNT_VOLTAGE 0x4
>> #define INA238_BUS_VOLTAGE 0x5
>> #define INA238_DIE_TEMP 0x6
>> #define INA238_CURRENT 0x7
>> #define INA238_POWER 0x8
>> +#define SQ52206_ENERGY 0x9
>> +#define SQ52206_CHARGE 0xa
>> #define INA238_DIAG_ALERT 0xb
>> #define INA238_SHUNT_OVER_VOLTAGE 0xc
>> #define INA238_SHUNT_UNDER_VOLTAGE 0xd
>> @@ -33,9 +36,12 @@
>> #define INA238_BUS_UNDER_VOLTAGE 0xf
>> #define INA238_TEMP_LIMIT 0x10
>> #define INA238_POWER_LIMIT 0x11
>> +#define SQ52206_POWER_PEAK 0x20
>> #define INA238_DEVICE_ID 0x3f /* not available on INA237 */
>>
>> #define INA238_CONFIG_ADCRANGE BIT(4)
>> +#define SQ52206_CONFIG_ADCRANGE GENMASK(4, 3)
>> +#define SQ52206_READ_ADCRANGE(reg) FIELD_GET(SQ52206_CONFIG_ADCRANGE, reg)
>>
>> #define INA238_DIAG_ALERT_TMPOL BIT(7)
>> #define INA238_DIAG_ALERT_SHNTOL BIT(6)
>> @@ -44,7 +50,7 @@
>> #define INA238_DIAG_ALERT_BUSUL BIT(3)
>> #define INA238_DIAG_ALERT_POL BIT(2)
>>
>> -#define INA238_REGISTERS 0x11
>> +#define INA238_MAX_REGISTERS 0x20
>
>Why this change ?
>
The maximum register address for SQ52206 is 0x20.
>>
>> #define INA238_RSHUNT_DEFAULT 10000 /* uOhm */
>>
>> @@ -87,27 +93,63 @@
>> * shunt = 0x4000 / (819.2 * 10^6) / 0.001 = 20000 uOhms (with 1mA/lsb)
>> *
>> * Current (mA) = register value * 20000 / rshunt / 4 * gain
>> - * Power (W) = 0.2 * register value * 20000 / rshunt / 4 * gain
>> + * Power (mW) = 0.2 * register value * 20000 / rshunt / 4 * gain
>> + * (Specific for SQ52206)
>> + * Power (mW) = 0.24 * register value * 20000 / rshunt / 4 * gain
>> */
>> #define INA238_CALIBRATION_VALUE 16384
>> -#define INA238_FIXED_SHUNT 20000
>> +#define INA238_FIXED_SHUNT 20000
>
>Why this change ?
>
Also refer to the chip manual provided in the website above.
>>
>> #define INA238_SHUNT_VOLTAGE_LSB 5 /* 5 uV/lsb */
>> #define INA238_BUS_VOLTAGE_LSB 3125 /* 3.125 mV/lsb */
>> -#define INA238_DIE_TEMP_LSB 125 /* 125 mC/lsb */
>> +#define INA238_DIE_TEMP_LSB 1250000 /* 125 mC/lsb */
>> +#define SQ52206_BUS_VOLTAGE_LSB 3750 /* 3.75 mV/lsb */
>> +#define SQ52206_DIE_TEMP_LSB 78125 /* 7.8125 mC/lsb */
>>
>> static const struct regmap_config ina238_regmap_config = {
>> - .max_register = INA238_REGISTERS,
>> + .max_register = INA238_MAX_REGISTERS,
>> .reg_bits = 8,
>> .val_bits = 16,
>> };
>>
>> +enum ina238_ids { ina238, ina237, sq52206 };
>> +
>> +struct ina238_config {
>> + u16 config_default;
>> + int calibration_value;
>
>.config_default and .calibration_value is the same for all chips.
>I don't see the point of not just using the respective defines.
>
Okay, I will change the definition here
>> + int bus_voltage_lsb; /* uV */
>> + int temp_lsb; /* mC */
>> +};
>> +
>> struct ina238_data {
>> + const struct ina238_config *config;
>> struct i2c_client *client;
>> struct mutex config_lock;
>> struct regmap *regmap;
>> u32 rshunt;
>> int gain;
>> + int kind;
>> +};
>> +
>> +static const struct ina238_config ina238_config[] = {
>> + [ina238] = {
>> + .config_default = INA238_CONFIG_DEFAULT,
>> + .calibration_value = INA238_CALIBRATION_VALUE,
>> + .bus_voltage_lsb = INA238_BUS_VOLTAGE_LSB,
>> + .temp_lsb = INA238_DIE_TEMP_LSB,
>> + },
>> + [ina237] = {
>> + .config_default = INA238_CONFIG_DEFAULT,
>> + .calibration_value = INA238_CALIBRATION_VALUE,
>> + .bus_voltage_lsb = INA238_BUS_VOLTAGE_LSB,
>> + .temp_lsb = INA238_DIE_TEMP_LSB,
>> + },
>> + [sq52206] = {
>> + .config_default = INA238_CONFIG_DEFAULT,
>> + .calibration_value = INA238_CALIBRATION_VALUE,
>> + .bus_voltage_lsb = SQ52206_BUS_VOLTAGE_LSB,
>> + .temp_lsb = SQ52206_DIE_TEMP_LSB,
>> + },
>> };
>>
>> static int ina238_read_reg24(const struct i2c_client *client, u8 reg, u32 *val)
>> @@ -197,10 +239,10 @@ static int ina238_read_in(struct device *dev, u32 attr, int channel,
>> regval = (s16)regval;
>> if (channel == 0)
>> /* gain of 1 -> LSB / 4 */
>> - *val = (regval * INA238_SHUNT_VOLTAGE_LSB) /
>> - (1000 * (4 - data->gain + 1));
>> + *val = (regval * INA238_SHUNT_VOLTAGE_LSB) *
>> + data->gain / (1000 * 4);
>
>Why this change ?
>
Because INA238 only has two gains of 1 and 4, the previous formula can
be used, but SQ52206 has a gain of 2, so I changed the formula
>> else
>> - *val = (regval * INA238_BUS_VOLTAGE_LSB) / 1000;
>> + *val = (regval * data->config->bus_voltage_lsb) / 1000;
>> break;
>> case hwmon_in_max_alarm:
>> case hwmon_in_min_alarm:
>> @@ -225,8 +267,8 @@ static int ina238_write_in(struct device *dev, u32 attr, int channel,
>> case 0:
>> /* signed value, clamp to max range +/-163 mV */
>> regval = clamp_val(val, -163, 163);
>> - regval = (regval * 1000 * (4 - data->gain + 1)) /
>> - INA238_SHUNT_VOLTAGE_LSB;
>> + regval = (regval * 1000 * 4) /
>> + INA238_SHUNT_VOLTAGE_LSB * data->gain;
>
>Why this change ?
>
Consistent with the reason described in the previous article.
>> regval = clamp_val(regval, S16_MIN, S16_MAX);
>>
>> switch (attr) {
>> @@ -242,7 +284,7 @@ static int ina238_write_in(struct device *dev, u32 attr, int channel,
>> case 1:
>> /* signed value, positive values only. Clamp to max 102.396 V */
>> regval = clamp_val(val, 0, 102396);
>> - regval = (regval * 1000) / INA238_BUS_VOLTAGE_LSB;
>> + regval = (regval * 1000) / data->config->bus_voltage_lsb;
>> regval = clamp_val(regval, 0, S16_MAX);
>>
>> switch (attr) {
>> @@ -295,10 +337,14 @@ static int ina238_read_power(struct device *dev, u32 attr, long *val)
>> err = ina238_read_reg24(data->client, INA238_POWER, ®val);
>> if (err)
>> return err;
>> -
>> - /* Fixed 1mA lsb, scaled by 1000000 to have result in uW */
>> - power = div_u64(regval * 1000ULL * INA238_FIXED_SHUNT *
>> - data->gain, 20 * data->rshunt);
>> + if (data->kind == sq52206)
>> + /* Fixed 1mA lsb, scaled by 1000000 to have result in uW */
>> + power = div_u64(regval * 1200ULL * INA238_FIXED_SHUNT *
>> + data->gain, 20 * data->rshunt);
>> + else
>> + /* Fixed 1mA lsb, scaled by 1000000 to have result in uW */
>> + power = div_u64(regval * 1000ULL * INA238_FIXED_SHUNT *
>> + data->gain, 20 * data->rshunt);
>
>
>> /* Clamp value to maximum value of long */
>> *val = clamp_val(power, 0, LONG_MAX);
>> break;
>> @@ -311,8 +357,12 @@ static int ina238_read_power(struct device *dev, u32 attr, long *val)
>> * Truncated 24-bit compare register, lower 8-bits are
>> * truncated. Same conversion to/from uW as POWER register.
>> */
>> - power = div_u64((regval << 8) * 1000ULL * INA238_FIXED_SHUNT *
>> - data->gain, 20 * data->rshunt);
>> + if (data->kind == sq52206)
>> + power = div_u64((regval << 8) * 1200ULL * INA238_FIXED_SHUNT *
>> + data->gain, 20 * data->rshunt);
>> + else
>> + power = div_u64((regval << 8) * 1000ULL * INA238_FIXED_SHUNT *
>> + data->gain, 20 * data->rshunt);
>> /* Clamp value to maximum value of long */
>> *val = clamp_val(power, 0, LONG_MAX);
>> break;
>> @@ -344,8 +394,12 @@ static int ina238_write_power(struct device *dev, u32 attr, long val)
>> * register.
>> */
>> regval = clamp_val(val, 0, LONG_MAX);
>> - regval = div_u64(val * 20ULL * data->rshunt,
>> - 1000ULL * INA238_FIXED_SHUNT * data->gain);
>> + if (data->kind == sq52206)
>> + regval = div_u64(val * 20ULL * data->rshunt,
>> + 1000ULL * INA238_FIXED_SHUNT * data->gain);
>> + else
>> + regval = div_u64(val * 24ULL * data->rshunt,
>> + 1000ULL * INA238_FIXED_SHUNT * data->gain);
>
>There is a constant factor against data->gain for the different chips.
>Why not incorporate that into data->gain, or use a chip configuration
>value for it ?
>
I will make adjustments in V2.
>> regval = clamp_val(regval >> 8, 0, U16_MAX);
>>
>> return regmap_write(data->regmap, INA238_POWER_LIMIT, regval);
>> @@ -362,17 +416,29 @@ static int ina238_read_temp(struct device *dev, u32 attr, long *val)
>> err = regmap_read(data->regmap, INA238_DIE_TEMP, ®val);
>> if (err)
>> return err;
>> -
>> + if (data->kind == sq52206)
>> + /* Signed, bits 15-0 of register, result in mC */
>> + regval = div_s64((s16)regval * data->config->temp_lsb,
>> + 10000);
>> + else
>> /* Signed, bits 15-4 of register, result in mC */
>> - *val = ((s16)regval >> 4) * INA238_DIE_TEMP_LSB;
>> + *val = div_s64(((s16)regval >> 4) *
>> + data->config->temp_lsb, 10000);
>> + *val = clamp_val(regval, S16_MIN, S16_MAX);
>
>Why is this clamp_val() now necessary ?
>
Yes, it's not necessary. I will make the necessary modifications.
>It might make sense to add the shift count to struct ina238_config
>instead of using if/else here.
>
>In general, using if/else for some chip specifics and struct ina238_config
>for others is confusing. I'd suggest to move all chip specific information
>into struct ina238_config and avoid if/else in runtime code as much as possible.
>
I will move all chip specific information into struct ina238_config.
>> break;
>> case hwmon_temp_max:
>> err = regmap_read(data->regmap, INA238_TEMP_LIMIT, ®val);
>> if (err)
>> return err;
>> -
>> - /* Signed, bits 15-4 of register, result in mC */
>> - *val = ((s16)regval >> 4) * INA238_DIE_TEMP_LSB;
>> + if (data->kind == sq52206)
>> + /* Signed, bits 15-0 of register, result in mC */
>> + regval = div_s64((s16)regval * data->config->temp_lsb,
>> + 10000);
>> + else
>> + /* Signed, bits 15-4 of register, result in mC */
>> + *val = div_s64(((s16)regval >> 4) *
>> + data->config->temp_lsb, 10000);
>> + *val = clamp_val(regval, S16_MIN, S16_MAX);
>> break;
>> case hwmon_temp_max_alarm:
>> err = regmap_read(data->regmap, INA238_DIAG_ALERT, ®val);
>> @@ -396,9 +462,14 @@ static int ina238_write_temp(struct device *dev, u32 attr, long val)
>> if (attr != hwmon_temp_max)
>> return -EOPNOTSUPP;
>>
>> - /* Signed, bits 15-4 of register */
>> - regval = (val / INA238_DIE_TEMP_LSB) << 4;
>> - regval = clamp_val(regval, S16_MIN, S16_MAX) & 0xfff0;
>> + if (data->kind == sq52206) {
>> + regval = div_u64(val*10000, data->config->temp_lsb);
>> + regval = clamp_val(regval, S16_MIN, S16_MAX);
>> + } else {
>> + /* Signed, bits 15-4 of register */
>> + regval = div_u64(val*10000, data->config->temp_lsb) << 4;
>> + regval = clamp_val(regval, S16_MIN, S16_MAX) & 0xfff0;
>> + }
>>
>> return regmap_write(data->regmap, INA238_TEMP_LIMIT, regval);
>> }
>> @@ -530,20 +601,58 @@ static const struct hwmon_chip_info ina238_chip_info = {
>> .info = ina238_info,
>> };
>>
>> +/*
>> + * Initialize the configuration and calibration registers.
>> + */
>> +static int ina238_init(struct ina238_data *data)
>> +{
>> + int ret = 0;
>> + int config;
>> + /* Setup CONFIG register */
>> + config = data->config->config_default;
>> + if (data->gain == 1)
>> + config |= INA238_CONFIG_ADCRANGE; /* ADCRANGE = 1 is /1 */
>> + ret = regmap_write(data->regmap, INA238_CONFIG, config);
>> + if (ret < 0)
>> + return ret;
>> + /* Setup ADC_CONFIG register */
>> + ret = regmap_write(data->regmap, INA238_ADC_CONFIG,
>> + INA238_ADC_CONFIG_DEFAULT);
>> + if (ret < 0)
>> + return ret;
>> + /* Setup SHUNT_CALIBRATION register with fixed value */
>> + ret = regmap_write(data->regmap, INA238_SHUNT_CALIBRATION,
>> + data->config->calibration_value);
>> + if (ret < 0)
>> + return ret;
>> + /* Setup alert/alarm configuration */
>> + ret = regmap_write(data->regmap, INA238_DIAG_ALERT,
>> + INA238_DIAG_ALERT_DEFAULT);
>> + if (ret < 0)
>> + return ret;
>> + return 0;
>> +}
>> +static const struct i2c_device_id ina238_id[];
>
>Please avoid forward declarations.
>
I will make adjustments in V2.
>> static int ina238_probe(struct i2c_client *client)
>> {
>> struct ina2xx_platform_data *pdata = dev_get_platdata(&client->dev);
>> struct device *dev = &client->dev;
>> struct device *hwmon_dev;
>> struct ina238_data *data;
>> - int config;
>> + enum ina238_ids chip;
>> int ret;
>>
>> + chip = (uintptr_t)i2c_get_match_data(client);
>> +
>> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> if (!data)
>> return -ENOMEM;
>>
>> + data->kind = chip;
>> data->client = client;
>> + /* set the device type */
>> + data->config = &ina238_config[data->kind];
>> +
>> mutex_init(&data->config_lock);
>>
>> data->regmap = devm_regmap_init_i2c(client, &ina238_regmap_config);
>> @@ -564,48 +673,15 @@ static int ina238_probe(struct i2c_client *client)
>> /* load shunt gain value */
>> if (device_property_read_u32(dev, "ti,shunt-gain", &data->gain) < 0)
>> data->gain = 4; /* Default of ADCRANGE = 0 */
>> - if (data->gain != 1 && data->gain != 4) {
>> + if (data->gain != 1 && data->gain != 2 && data->gain != 4) {
>
>Chip independent changes should be in separate patch(es).
>
SQ52206 has a gain of 2.
>> dev_err(dev, "invalid shunt gain value %u\n", data->gain);
>> return -EINVAL;
>> }
>> -
>> - /* Setup CONFIG register */
>> - config = INA238_CONFIG_DEFAULT;
>> - if (data->gain == 1)
>> - config |= INA238_CONFIG_ADCRANGE; /* ADCRANGE = 1 is /1 */
>> - ret = regmap_write(data->regmap, INA238_CONFIG, config);
>> - if (ret < 0) {
>> - dev_err(dev, "error configuring the device: %d\n", ret);
>> - return -ENODEV;
>> - }
>> -
>> - /* Setup ADC_CONFIG register */
>> - ret = regmap_write(data->regmap, INA238_ADC_CONFIG,
>> - INA238_ADC_CONFIG_DEFAULT);
>> - if (ret < 0) {
>> - dev_err(dev, "error configuring the device: %d\n", ret);
>> - return -ENODEV;
>> - }
>> -
>> - /* Setup SHUNT_CALIBRATION register with fixed value */
>> - ret = regmap_write(data->regmap, INA238_SHUNT_CALIBRATION,
>> - INA238_CALIBRATION_VALUE);
>> - if (ret < 0) {
>> - dev_err(dev, "error configuring the device: %d\n", ret);
>> - return -ENODEV;
>> - }
>> -
>> - /* Setup alert/alarm configuration */
>> - ret = regmap_write(data->regmap, INA238_DIAG_ALERT,
>> - INA238_DIAG_ALERT_DEFAULT);
>> - if (ret < 0) {
>> - dev_err(dev, "error configuring the device: %d\n", ret);
>> - return -ENODEV;
>> - }
>> + ret = ina238_init(data);
>>
>
>Errors must not be ignored.
>
I will make adjustments in V2.
>> hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data,
>> &ina238_chip_info,
>> - NULL);
>> + ina238_groups);
>> if (IS_ERR(hwmon_dev))
>> return PTR_ERR(hwmon_dev);
>>
>> @@ -616,7 +692,9 @@ static int ina238_probe(struct i2c_client *client)
>> }
>>
>> static const struct i2c_device_id ina238_id[] = {
>> - { "ina238" },
>> + { "ina238", ina238 },
>> + { "ina237", ina237 },
>
>Unrelated change, and out of order (ina237 should come first).
>
I will make adjustments in V2.
>> + { "sq52206", sq52206 },
>> { }
>> };
>> MODULE_DEVICE_TABLE(i2c, ina238_id);
>> @@ -624,6 +702,7 @@ MODULE_DEVICE_TABLE(i2c, ina238_id);
>> static const struct of_device_id __maybe_unused ina238_of_match[] = {
>> { .compatible = "ti,ina237" },
>> { .compatible = "ti,ina238" },
>> + { .compatible = "silergy,sq52206" },
>
>.data is missing.
>
I will make adjustments in V2.
>> { },
>> };
>> MODULE_DEVICE_TABLE(of, ina238_of_match);
Thanks,
Guenter
On 1/7/25 04:43, Wenliang Yan wrote:
...
>>> -#define INA238_REGISTERS 0x11
>>> +#define INA238_MAX_REGISTERS 0x20
>>
>> Why this change ?
>>
>
> The maximum register address for SQ52206 is 0x20.
>
That isn't what I referred to. Th value change is ok.
The question was why change INA238_REGISTERS to INA238_MAX_REGISTERS.
That is a personal preference change. I strongly discourage such
changes because if I accept them someone else may come tomorrow
and change the name again to match their preference.
>>> -#define INA238_FIXED_SHUNT 20000
>>> +#define INA238_FIXED_SHUNT 20000
>>
>> Why this change ?
>>
>
> Also refer to the chip manual provided in the website above.
>
The value didn't change. The indentation changed without reason.
>>>
>>> static int ina238_read_reg24(const struct i2c_client *client, u8 reg, u32 *val)
>>> @@ -197,10 +239,10 @@ static int ina238_read_in(struct device *dev, u32 attr, int channel,
>>> regval = (s16)regval;
>>> if (channel == 0)
>>> /* gain of 1 -> LSB / 4 */
>>> - *val = (regval * INA238_SHUNT_VOLTAGE_LSB) /
>>> - (1000 * (4 - data->gain + 1));
>>> + *val = (regval * INA238_SHUNT_VOLTAGE_LSB) *
>>> + data->gain / (1000 * 4);
>>
>> Why this change ?
>>
>
> Because INA238 only has two gains of 1 and 4, the previous formula can
> be used, but SQ52206 has a gain of 2, so I changed the formula
>
Ok.
>>> else
>>> - *val = (regval * INA238_BUS_VOLTAGE_LSB) / 1000;
>>> + *val = (regval * data->config->bus_voltage_lsb) / 1000;
>>> break;
>>> case hwmon_in_max_alarm:
>>> case hwmon_in_min_alarm:
>>> @@ -225,8 +267,8 @@ static int ina238_write_in(struct device *dev, u32 attr, int channel,
>>> case 0:
>>> /* signed value, clamp to max range +/-163 mV */
>>> regval = clamp_val(val, -163, 163);
>>> - regval = (regval * 1000 * (4 - data->gain + 1)) /
>>> - INA238_SHUNT_VOLTAGE_LSB;
>>> + regval = (regval * 1000 * 4) /
>>> + INA238_SHUNT_VOLTAGE_LSB * data->gain;
>>
>> Why this change ?
>>
>
> Consistent with the reason described in the previous article.
>
Ok.
>>> data->regmap = devm_regmap_init_i2c(client, &ina238_regmap_config);
>>> @@ -564,48 +673,15 @@ static int ina238_probe(struct i2c_client *client)
>>> /* load shunt gain value */
>>> if (device_property_read_u32(dev, "ti,shunt-gain", &data->gain) < 0)
>>> data->gain = 4; /* Default of ADCRANGE = 0 */
>>> - if (data->gain != 1 && data->gain != 4) {
>>> + if (data->gain != 1 && data->gain != 2 && data->gain != 4) {
>>
>> Chip independent changes should be in separate patch(es).
>>
>
> SQ52206 has a gain of 2.
>
Ok.
Thanks,
Guenter
At 2025-01-07 23:02:09, "Guenter Roeck" <linux@roeck-us.net> wrote:
>On 1/7/25 04:43, Wenliang Yan wrote:
>...
>>>> -#define INA238_REGISTERS 0x11
>>>> +#define INA238_MAX_REGISTERS 0x20
>>>
>>> Why this change ?
>>>
>>
>> The maximum register address for SQ52206 is 0x20.
>>
>
>That isn't what I referred to. Th value change is ok.
>The question was why change INA238_REGISTERS to INA238_MAX_REGISTERS.
>That is a personal preference change. I strongly discourage such
>changes because if I accept them someone else may come tomorrow
>and change the name again to match their preference.
>
I understand, I will modify.
>>>> -#define INA238_FIXED_SHUNT 20000
>>>> +#define INA238_FIXED_SHUNT 20000
>>>
>>> Why this change ?
>>>
>>
>> Also refer to the chip manual provided in the website above.
>>
>
>The value didn't change. The indentation changed without reason.
>
This is due to my format alignment, I will adjust it back.
>>>>
>>>> static int ina238_read_reg24(const struct i2c_client *client, u8 reg, u32 *val)
>>>> @@ -197,10 +239,10 @@ static int ina238_read_in(struct device *dev, u32 attr, int channel,
>>>> regval = (s16)regval;
>>>> if (channel == 0)
>>>> /* gain of 1 -> LSB / 4 */
>>>> - *val = (regval * INA238_SHUNT_VOLTAGE_LSB) /
>>>> - (1000 * (4 - data->gain + 1));
>>>> + *val = (regval * INA238_SHUNT_VOLTAGE_LSB) *
>>>> + data->gain / (1000 * 4);
>>>
>>> Why this change ?
>>>
>>
>> Because INA238 only has two gains of 1 and 4, the previous formula can
>> be used, but SQ52206 has a gain of 2, so I changed the formula
>>
>Ok.
>
>>>> else
>>>> - *val = (regval * INA238_BUS_VOLTAGE_LSB) / 1000;
>>>> + *val = (regval * data->config->bus_voltage_lsb) / 1000;
>>>> break;
>>>> case hwmon_in_max_alarm:
>>>> case hwmon_in_min_alarm:
>>>> @@ -225,8 +267,8 @@ static int ina238_write_in(struct device *dev, u32 attr, int channel,
>>>> case 0:
>>>> /* signed value, clamp to max range +/-163 mV */
>>>> regval = clamp_val(val, -163, 163);
>>>> - regval = (regval * 1000 * (4 - data->gain + 1)) /
>>>> - INA238_SHUNT_VOLTAGE_LSB;
>>>> + regval = (regval * 1000 * 4) /
>>>> + INA238_SHUNT_VOLTAGE_LSB * data->gain;
>>>
>>> Why this change ?
>>>
>>
>> Consistent with the reason described in the previous article.
>>
>Ok.
>
>>>> data->regmap = devm_regmap_init_i2c(client, &ina238_regmap_config);
>>>> @@ -564,48 +673,15 @@ static int ina238_probe(struct i2c_client *client)
>>>> /* load shunt gain value */
>>>> if (device_property_read_u32(dev, "ti,shunt-gain", &data->gain) < 0)
>>>> data->gain = 4; /* Default of ADCRANGE = 0 */
>>>> - if (data->gain != 1 && data->gain != 4) {
>>>> + if (data->gain != 1 && data->gain != 2 && data->gain != 4) {
>>>
>>> Chip independent changes should be in separate patch(es).
>>>
>>
>> SQ52206 has a gain of 2.
>>
>Ok.
>
>
>Thanks,
>Guenter
Thanks,
Wenliang Yan
© 2016 - 2026 Red Hat, Inc.