[PATCH 0/3] hwmon:(ina238)Add support for SQ52206

[PATCH 0/3] hwmon:(ina238)Add support for SQ52206

[Wenliang Yan]

From: Wenliang <wenliang202407@163.com>

Add support for Silergy i2c power monitor SQ52206 to the ina238
driver as those two are similar.

Signed-off-by: Wenliang <wenliang202407@163.com>
---

The manual content of the chip has not been fully disclosed yet. Can the manual be provided in the form of a cloud storage link for reviewing the driver code? If not, how can it be provided? It can be provided in V2.
This version patch is displayed through text, and both chips are power monitors with a 16 bit delta sigma ADC specifically designed for current sensing applications Operates from a 2.7-V to 5.5-V supply. Same pin definition.In addition, SQ52206 add detection for power peak, energy and charge.
The main differences between the two chips are as follows:
___________________________________
	    |  INA238  |  SQ52206  |
VBUS_LSB    |  3.125mV |   3.75mV  |
DIETEMP_LSB |  125 mC  | 7.8125 mC |
____________|__________|___________|
________________________________________________
reg address |     INA238    |     SQ52206       |
____________|_______________|___________________|
	0h  |     CONFIG    |     CONFIG        |
	1h  |   ADC_CONFIG  |    ADC_CONFIG     |
	2h  |   SHUNT_CAL   |    SHUNT_CAL      |
	3h  |       /       |SHUNT_TEMPCO(16bit)|
	4h  |     VSHUNT    |      VSHUNT       |
	5h  |      VBUS     |       VBUS        |
	6h  |     DIETEMP   |      DIETEMP      |
	7h  |     CURRENT   |      CURRENT      |
	8h  |      POWER    |       POWER       |
	9h  |       /       |    ENERGY(40bit)  |	
	Ah  |       /       |    CHARGE(40bit)  |
	Bh  |   DIAG_ALERT  |    DIAG_ALERT     |
	Ch  |      SOVL     |        SOVL       |
	Dh  |      SUVL     |        SUVL       |
	Eh  |      BOVL     |        BOVL       |
	Fh  |      BUVL     |        BUVL       |
	10h |  TEMP_LIMIT   |     TEMP_LIMIT    |
	11h |  PWR_LIMIT    |     PWR_LIMIT     |
	20h |               |   PWR_PEAK(24bit) |
	3Eh |MANUFACTURER_ID|        /          |
	3Fh |   DEVICE_ID   |        /          |
____________|_______________|___________________|
The ENERGY and CHARGE registers are 40 bit read-only registers used to display the values of energy and charge. PWR_PEAK register is 24 bit read-only registers.The CONFIG register has some differences:
__________________________________________________________________________
BIT |	    INA238               |                  SQ52206               |
____|____________________________|________________________________________|
 14 |  Reserved. Always reads 0  | reset ENERGY and CHARGE registers to 0 |
____|____________________________|________________________________________|
  4 |ADCRANGE 0h=163.84 1h=81.92 | (BIT4~3)ADCRANGE 2h/3h = 40.96,	  |
____|____________________________| 0h=163.84 1h=81.92                     |
  3 |  Reserved. Always reads 0  | 		  			  |
____|____________________________|________________________________________|


Wenliang (3):
  hwmon:(ina238)Add support for SQ52206
  hwmon:(ina238)Add new features for SQ52206
  dt-bindings:Add SQ52206 to ina2xx devicetree bindings

 .../devicetree/bindings/hwmon/ti,ina2xx.yaml  |   1 +
 Documentation/hwmon/ina238.rst                |  16 +
 drivers/hwmon/ina238.c                        | 286 ++++++++++++++----
 3 files changed, 240 insertions(+), 63 deletions(-)

-- 
2.43.0

[PATCH 1/3] hwmon:(ina238)Add support for SQ52206

[Wenliang Yan]

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, &regval);
 		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, &regval);
 		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, &regval);
 		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, &regval);
@@ -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.0

Re: [PATCH 1/3] hwmon:(ina238)Add support for SQ52206

[Guenter Roeck]

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 ?

>   
>   #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, &regval);
>   		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, &regval);
>   		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, &regval);
>   		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, &regval);
> @@ -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);

Re: [PATCH 1/3] hwmon:(ina238)Add support for SQ52206

[Wenliang Yan]

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, &regval);
>>   		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, &regval);
>>   		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, &regval);
>>   		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, &regval);
>> @@ -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

Re: [PATCH 1/3] hwmon:(ina238)Add support for SQ52206

[Guenter Roeck]

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

Re: [PATCH 1/3] hwmon:(ina238)Add support for SQ52206

[Wenliang Yan]

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