[PATCH 0/8] (hwmon):(ina3221) Various improvement and add support for SQ52210

[PATCH 0/8] (hwmon):(ina3221) Various improvement and add support for SQ52210

[Wenliang Yan]

Add support for SQ52210 to the ina3221 driver. The datasheet depends on 
https://us1.silergy.com/cloud/index/uniqid/6912826d13b9c.html or
https://www.silergy.com/cloud/index/uniqid/6912826d13b9c.html
The password is 7IUCLe.

- SQ52210 is forward compatible with INA3221 and add alert register to
  implement four additional alert function.

- Add support for SQ52210, add current registers, power registers, and
  registers related to alerts.

- Add support for alert-type.

- The LSB for current and power can be pre-calculated for data read/write
  operations. The current LSB is determined by the calibration value and
  shunt resistor value, with the calibration value fixed within the driver.
  The power LSB can be derived from the current LSB.

- SQ52210 adds current, power, and alert-limit sensors, with read/write
  functions modified to accommodate these new changes.

- SQ52210 adds power attribute to report power data, and implements
  read/write functions for this purpose.

- Modify the read/write functions for current attributes.
  SQ52210 can directly use its internal current registers to compare
  with alert values for implementing curr_lcrit functionality.

Wenliang Yan (8):
  dt-binding:ti,ina3221:Add SQ52210
  hwmon:(ina3221)Add support for SQ52210
  hwmon:(ina3221)Support alert-type
  hwmon:(ina3221)Pre-calculate current and power LSB
  hwmon:(ina3221)Introduce power attribute and other characteristics of
    other attribute
  hwmon:(ina3221)Modify read/write functions for 'in' attribute
  hwmon:(ina3221)Support read/write functions for 'power' attribute
  hwmon:(ina3221)Support read/write functions for current_lcrict
    attribute

 .../devicetree/bindings/hwmon/ti,ina3221.yaml |  16 +-
 Documentation/hwmon/ina3221.rst               |  24 +
 drivers/hwmon/ina3221.c                       | 516 +++++++++++++++++-
 3 files changed, 541 insertions(+), 15 deletions(-)

-- 
2.17.1

[PATCH 1/8] dt-binding:ti,ina3221:Add SQ52210

[Wenliang Yan]

Add a compatible string for sq52210, sq52210 is forward compatible
with INA3221 and add alert register to implement four additional
alert function.

Signed-off-by: Wenliang Yan <wenliang202407@163.com>
---
 .../devicetree/bindings/hwmon/ti,ina3221.yaml    | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
index 5f10f1207d69..0fae82ca3ee1 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
@@ -12,7 +12,9 @@ maintainers:
 
 properties:
   compatible:
-    const: ti,ina3221
+    enum:
+      - silergy,sq52210
+      - ti,ina3221
 
   reg:
     maxItems: 1
@@ -77,6 +79,18 @@ patternProperties:
           exclude specific channels from the summation control function.
         type: boolean
 
+      alert-type:
+        description: |
+          The SQ52210 features a configurable alert function with four
+          types: SUL, BOL, BUL, and POL. Each channel can be configured to
+          select one of these types to enable the alert function. This alert
+          function can operate concurrently with both Critical and Warning
+          functions.
+
+          The configuration must use numerical values 0 through 3,
+          0 corresponds to SUL, 1 to BOL, 2 to BUL, and 3 to POL.
+        enum: [ 0, 1, 2, 3 ]
+
     required:
       - reg
 
-- 
2.17.1

[PATCH 2/8] hwmon:(ina3221)Add support for SQ52210

[Wenliang Yan]

SQ52210 is compatible with INA3221, but also includes current registers,
power registers, and registers related to alerts.

Signed-off-by: Wenliang Yan <wenliang202407@163.com>
---
 drivers/hwmon/ina3221.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 5ecc68dcf169..80c1bcc7edd7 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -34,6 +34,17 @@
 #define INA3221_SHUNT_SUM		0x0d
 #define INA3221_CRIT_SUM		0x0e
 #define INA3221_MASK_ENABLE		0x0f
+#define SQ52210_ALERT_CONFIG	0x12
+#define SQ52210_CALIBRATION		0x14
+#define SQ52210_CURRENT1		0x15
+#define SQ52210_CURRENT2		0x16
+#define SQ52210_CURRENT3		0x17
+#define SQ52210_POWER1			0x18
+#define SQ52210_POWER2			0x19
+#define SQ52210_POWER3			0x1A
+#define SQ52210_ALERT_LIMIT1	0x1B
+#define SQ52210_ALERT_LIMIT2	0x1C
+#define SQ52210_ALERT_LIMIT3	0x1D
 
 #define INA3221_CONFIG_MODE_MASK	GENMASK(2, 0)
 #define INA3221_CONFIG_MODE_POWERDOWN	0
@@ -108,8 +119,12 @@ struct ina3221_input {
 	bool summation_disable;
 };
 
+enum ina3221_ids { ina3221, sq52210 };
+
+
 /**
  * struct ina3221_data - device specific information
+ * @chip: Chip type identifier
  * @pm_dev: Device pointer for pm runtime
  * @regmap: Register map of the device
  * @fields: Register fields of the device
@@ -120,6 +135,8 @@ struct ina3221_input {
  * @single_shot: running in single-shot operating mode
  */
 struct ina3221_data {
+	enum ina3221_ids chip;
+
 	struct device *pm_dev;
 	struct regmap *regmap;
 	struct regmap_field *fields[F_MAX_FIELDS];
@@ -734,6 +751,7 @@ static const struct regmap_range ina3221_yes_ranges[] = {
 	regmap_reg_range(INA3221_CONFIG, INA3221_BUS3),
 	regmap_reg_range(INA3221_SHUNT_SUM, INA3221_SHUNT_SUM),
 	regmap_reg_range(INA3221_MASK_ENABLE, INA3221_MASK_ENABLE),
+	regmap_reg_range(SQ52210_ALERT_CONFIG, SQ52210_POWER3),
 };
 
 static const struct regmap_access_table ina3221_volatile_table = {
@@ -818,13 +836,18 @@ static int ina3221_probe(struct i2c_client *client)
 	struct device *dev = &client->dev;
 	struct ina3221_data *ina;
 	struct device *hwmon_dev;
+	enum ina3221_ids chip;
 	char name[32];
 	int i, ret;
 
+	chip = (uintptr_t)i2c_get_match_data(client);
+
 	ina = devm_kzalloc(dev, sizeof(*ina), GFP_KERNEL);
 	if (!ina)
 		return -ENOMEM;
 
+	ina->chip = chip;
+
 	ina->regmap = devm_regmap_init_i2c(client, &ina3221_regmap_config);
 	if (IS_ERR(ina->regmap)) {
 		dev_err(dev, "Unable to allocate register map\n");
@@ -996,13 +1019,21 @@ static DEFINE_RUNTIME_DEV_PM_OPS(ina3221_pm, ina3221_suspend, ina3221_resume,
 				 NULL);
 
 static const struct of_device_id ina3221_of_match_table[] = {
-	{ .compatible = "ti,ina3221", },
+	{
+		.compatible = "silergy,sq52210",
+		.data = (void *)sq52210
+	},
+	{
+		.compatible = "ti,ina3221",
+		.data = (void *)ina3221
+	},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, ina3221_of_match_table);
 
 static const struct i2c_device_id ina3221_ids[] = {
-	{ "ina3221" },
+	{ "ina3221", ina3221 },
+	{ "sq52210", sq52210 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(i2c, ina3221_ids);
-- 
2.17.1

[PATCH 3/8] hwmon:(ina3221)Support alert-type

[Wenliang Yan]

SQ52210 supports setting alert-type, and this parameter has been
described in the devicetree. Add support for it to the driver.

Signed-off-by: Wenliang Yan <wenliang202407@163.com>
---
 drivers/hwmon/ina3221.c | 73 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 80c1bcc7edd7..ee9ad022e255 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -65,6 +65,8 @@
 
 #define INA3221_MASK_ENABLE_SCC_MASK	GENMASK(14, 12)
 
+#define SQ52210_ALERT_CONFIG_MASK	GENMASK(15, 4)
+
 #define INA3221_CONFIG_DEFAULT		0x7127
 #define INA3221_RSHUNT_DEFAULT		10000
 
@@ -105,6 +107,13 @@ enum ina3221_channels {
 	INA3221_NUM_CHANNELS
 };
 
+enum ina3221_alert_type {
+	SUL,
+	BOL,
+	BUL,
+	POL
+};
+
 /**
  * struct ina3221_input - channel input source specific information
  * @label: label of channel input source
@@ -121,9 +130,15 @@ struct ina3221_input {
 
 enum ina3221_ids { ina3221, sq52210 };
 
+struct ina3221_config {
+	bool has_alerts;	/* chip supports alerts and limits */
+	bool has_current;	/* chip has internal current reg */
+	bool has_power;		/* chip has internal power reg */
+};
 
 /**
  * struct ina3221_data - device specific information
+ * @config:	Used to store characteristics of different chips
  * @chip: Chip type identifier
  * @pm_dev: Device pointer for pm runtime
  * @regmap: Register map of the device
@@ -132,9 +147,11 @@ enum ina3221_ids { ina3221, sq52210 };
  * @reg_config: Register value of INA3221_CONFIG
  * @summation_shunt_resistor: equivalent shunt resistor value for summation
  * @summation_channel_control: Value written to SCC field in INA3221_MASK_ENABLE
+ * @alert_type_select: Used to store the alert trigger type
  * @single_shot: running in single-shot operating mode
  */
 struct ina3221_data {
+	const struct ina3221_config *config;
 	enum ina3221_ids chip;
 
 	struct device *pm_dev;
@@ -144,10 +161,24 @@ struct ina3221_data {
 	u32 reg_config;
 	int summation_shunt_resistor;
 	u32 summation_channel_control;
+	u32 alert_type_select;
 
 	bool single_shot;
 };
 
+static const struct ina3221_config ina3221_config[] = {
+	[ina3221] = {
+		.has_alerts = false,
+		.has_current = false,
+		.has_power = false,
+	},
+	[sq52210] = {
+		.has_alerts = true,
+		.has_current = true,
+		.has_power = true,
+	},
+};
+
 static inline bool ina3221_is_enabled(struct ina3221_data *ina, int channel)
 {
 	/* Summation channel checks shunt resistor values */
@@ -772,7 +803,7 @@ static int ina3221_probe_child_from_dt(struct device *dev,
 				       struct ina3221_data *ina)
 {
 	struct ina3221_input *input;
-	u32 val;
+	u32 val, alert_type;
 	int ret;
 
 	ret = of_property_read_u32(child, "reg", &val);
@@ -792,6 +823,34 @@ static int ina3221_probe_child_from_dt(struct device *dev,
 		return 0;
 	}
 
+	if (ina->config->has_alerts) {
+		ret = of_property_read_u32(child, "alert-type", &alert_type);
+		if (ret < 0) {
+			dev_err(dev, "missing alert-type property of %pOFn\n", child);
+			return ret;
+		} else if (alert_type > POL) {
+			dev_err(dev, "invalid alert-type of %pOFn\n", child);
+			return -EINVAL;
+		}
+		switch (alert_type) {
+		/* val is channel value*/
+		case SUL:
+			ina->alert_type_select |= BIT(15 - val);
+			break;
+		case BOL:
+			ina->alert_type_select |= BIT(12 - val);
+			break;
+		case BUL:
+			ina->alert_type_select |= BIT(9 - val);
+			break;
+		case POL:
+			ina->alert_type_select |= BIT(6 - val);
+			break;
+		default:
+			break;
+		}
+	}
+
 	/* Save the connected input label if available */
 	of_property_read_string(child, "label", &input->label);
 
@@ -847,6 +906,7 @@ static int ina3221_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	ina->chip = chip;
+	ina->config = &ina3221_config[chip];
 
 	ina->regmap = devm_regmap_init_i2c(client, &ina3221_regmap_config);
 	if (IS_ERR(ina->regmap)) {
@@ -1012,6 +1072,17 @@ static int ina3221_resume(struct device *dev)
 		}
 	}
 
+	/* Restore alert config register value to hardware */
+	if (ina->config->has_alerts) {
+		ret = regmap_update_bits(ina->regmap, SQ52210_ALERT_CONFIG,
+					 SQ52210_ALERT_CONFIG_MASK,
+					 ina->alert_type_select);
+		if (ret) {
+			dev_err(dev, "Unable to select alert type\n");
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
-- 
2.17.1

[PATCH 4/8] hwmon:(ina3221)Pre-calculate current and power LSB

[Wenliang Yan]

The LSB for current and power can be pre-calculated for data read/write
operations. The current LSB is determined by the calibration value and
shunt resistor value, with the calibration value fixed within the driver.
The power LSB can be derived from the current LSB.

Use DIV_ROUND_CLOSEST function to replace division operations and reduce
rouding errors.

Signed-off-by: Wenliang Yan <wenliang202407@163.com>
---
 drivers/hwmon/ina3221.c | 71 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 68 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index ee9ad022e255..e339860ed3a2 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -69,6 +69,7 @@
 
 #define INA3221_CONFIG_DEFAULT		0x7127
 #define INA3221_RSHUNT_DEFAULT		10000
+#define SQ52210_SHUNT_LSB			40000000	/* pV/LSB */
 
 enum ina3221_fields {
 	/* Configuration */
@@ -134,6 +135,8 @@ struct ina3221_config {
 	bool has_alerts;	/* chip supports alerts and limits */
 	bool has_current;	/* chip has internal current reg */
 	bool has_power;		/* chip has internal power reg */
+	int calibration_value;	/* calculate current_lsb */
+	int power_lsb_factor;
 };
 
 /**
@@ -148,6 +151,8 @@ struct ina3221_config {
  * @summation_shunt_resistor: equivalent shunt resistor value for summation
  * @summation_channel_control: Value written to SCC field in INA3221_MASK_ENABLE
  * @alert_type_select: Used to store the alert trigger type
+ * @current_lsb_uA: The value of one LSB corresponding to the current register
+ * @power_lsb_uW: The value of one LSB corresponding to the power register
  * @single_shot: running in single-shot operating mode
  */
 struct ina3221_data {
@@ -162,6 +167,8 @@ struct ina3221_data {
 	int summation_shunt_resistor;
 	u32 summation_channel_control;
 	u32 alert_type_select;
+	long current_lsb_uA;
+	long power_lsb_uW;
 
 	bool single_shot;
 };
@@ -176,6 +183,13 @@ static const struct ina3221_config ina3221_config[] = {
 		.has_alerts = true,
 		.has_current = true,
 		.has_power = true,
+		/*
+		 * With this default value configuration,
+		 * the following formula can be obtained:
+		 * Current_LSB = Shunt_LSB / Rshunt
+		 */
+		.calibration_value = 256,
+		.power_lsb_factor = 20,
 	},
 };
 
@@ -729,6 +743,25 @@ static const struct hwmon_chip_info ina3221_chip_info = {
 };
 
 /* Extra attribute groups */
+
+/*
+ * Calculate the value corresponding to one LSB of the current and
+ * power registers.
+ * formula : Current_LSB = Shunt_LSB / Rshunt
+ *			 Power_LSB = power_lsb_factor * Current_LSB
+ */
+static int ina3221_set_shunt(struct ina3221_data *ina, unsigned long val)
+{
+	if (!val || val > SQ52210_SHUNT_LSB)
+		return -EINVAL;
+
+	ina->current_lsb_uA = DIV_ROUND_CLOSEST(SQ52210_SHUNT_LSB, val);
+	ina->power_lsb_uW = ina->config->power_lsb_factor *
+			    ina->current_lsb_uA;
+
+	return 0;
+}
+
 static ssize_t ina3221_shunt_show(struct device *dev,
 				  struct device_attribute *attr, char *buf)
 {
@@ -761,7 +794,17 @@ static ssize_t ina3221_shunt_store(struct device *dev,
 
 	/* Update summation_shunt_resistor for summation channel */
 	ina->summation_shunt_resistor = ina3221_summation_shunt_resistor(ina);
-
+	/*
+	 * The current and power registers can only be used when
+	 * all enabled channels have identical shunt resistors
+	 */
+	if (ina->summation_shunt_resistor) {
+		if (ina->config->has_current) {
+			ret = ina3221_set_shunt(ina, val);
+			if (ret < 0)
+				return ret;
+		}
+	}
 	return count;
 }
 
@@ -953,6 +996,16 @@ static int ina3221_probe(struct i2c_client *client)
 			ina->summation_channel_control |= BIT(14 - i);
 	}
 
+	/*
+	 * The current and power registers can only be used when
+	 * all enabled channels have identical shunt resistors
+	 */
+	if (ina->summation_shunt_resistor) {
+		ret = ina3221_set_shunt(ina, ina->summation_shunt_resistor);
+		if (ret < 0)
+			return ret;
+	}
+
 	ina->pm_dev = dev;
 	dev_set_drvdata(dev, ina);
 
@@ -970,8 +1023,8 @@ static int ina3221_probe(struct i2c_client *client)
 	}
 
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, ina,
-							 &ina3221_chip_info,
-							 ina3221_groups);
+							&ina3221_chip_info,
+							ina3221_groups);
 	if (IS_ERR(hwmon_dev)) {
 		dev_err(dev, "Unable to register hwmon device\n");
 		ret = PTR_ERR(hwmon_dev);
@@ -1070,6 +1123,18 @@ static int ina3221_resume(struct device *dev)
 			dev_err(dev, "Unable to control summation channel\n");
 			return ret;
 		}
+		/*
+		 * The calibration register can only be enabled when all
+		 * shunt resistor values are identical.
+		 */
+		if (ina->config->has_current) {
+			ret = regmap_write(ina->regmap, SQ52210_CALIBRATION,
+						ina->config->calibration_value);
+			if (ret) {
+				dev_err(dev, "Unable to set calibration value\n");
+				return ret;
+			}
+		}
 	}
 
 	/* Restore alert config register value to hardware */
-- 
2.17.1

[PATCH 5/8] hwmon:(ina3221)Introduce power attribute and other characteristics of other attribute

[Wenliang Yan]

SQ52210 has built-in current and power sensors as well as multiple
alert functions. Add power attributes and different critical
characteristics in hwmon to report the corresponding data.

Signed-off-by: Wenliang Yan <wenliang202407@163.com>
---
 Documentation/hwmon/ina3221.rst | 24 +++++++++++++
 drivers/hwmon/ina3221.c         | 60 +++++++++++++++++++++++++++++----
 2 files changed, 77 insertions(+), 7 deletions(-)

diff --git a/Documentation/hwmon/ina3221.rst b/Documentation/hwmon/ina3221.rst
index 8c12c54d2c24..224c6cf735ed 100644
--- a/Documentation/hwmon/ina3221.rst
+++ b/Documentation/hwmon/ina3221.rst
@@ -13,6 +13,13 @@ Supported chips:
 
 	       https://www.ti.com/
 
+  * Silergy SQ52210
+
+    Prefix: 'SQ52210'
+
+    Addresses: I2C 0x40 - 0x43
+
+
 Author: Andrew F. Davis <afd@ti.com>
 
 Description
@@ -23,6 +30,9 @@ side of up to three D.C. power supplies. The INA3221 monitors both shunt drop
 and supply voltage, with programmable conversion times and averaging, current
 and power are calculated host-side from these.
 
+The SQ52210 is a mostly compatible chip from Silergy. It incorporates internal
+current and power registers, and provides an extra configurable alert function.
+
 Sysfs entries
 -------------
 
@@ -72,3 +82,17 @@ update_interval         Data conversion time in millisecond, following:
                         Note that setting update_interval to 0ms sets both BC
                         and SC to 140 us (minimum conversion time).
 ======================= =======================================================
+
+Additional sysfs entries for sq52210
+-------------------------------------
+
+======================= =======================================================
+in[123]_crit            Critical high bus voltage
+in[123]_crit_alarm      Bus voltage critical high alarm
+in[123]_lcrit           Critical low bus voltage
+in[123]_lcrit_alarm     Bus voltage critical low alarm
+curr[123]_lcrit         Critical low current
+curr[123]_lcrit_alarm   Current critical low alarm
+power[123]_input        Current for channels 1, 2, and 3 respectively
+power[123]_crit         Critical high power
+power[123]_crit_alarm   Power critical high alarm
diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index e339860ed3a2..77b2505a49a2 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -652,6 +652,8 @@ static umode_t ina3221_is_visible(const void *drvdata,
 {
 	const struct ina3221_data *ina = drvdata;
 	const struct ina3221_input *input = NULL;
+	bool has_alerts = ina->config->has_alerts;
+	bool has_power = ina->config->has_power;
 
 	switch (type) {
 	case hwmon_chip:
@@ -679,6 +681,16 @@ static umode_t ina3221_is_visible(const void *drvdata,
 			return 0444;
 		case hwmon_in_enable:
 			return 0644;
+		case hwmon_in_crit:
+		case hwmon_in_lcrit:
+			if (has_alerts)
+				return 0644;
+			return 0;
+		case hwmon_in_crit_alarm:
+		case hwmon_in_lcrit_alarm:
+			if (has_alerts)
+				return 0444;
+			return 0;
 		default:
 			return 0;
 		}
@@ -691,6 +703,28 @@ static umode_t ina3221_is_visible(const void *drvdata,
 		case hwmon_curr_crit:
 		case hwmon_curr_max:
 			return 0644;
+		case hwmon_curr_lcrit:
+			if (has_alerts)
+				return 0644;
+			return 0;
+		case hwmon_curr_lcrit_alarm:
+			if (has_alerts)
+				return 0444;
+			return 0;
+		default:
+			return 0;
+		}
+		case hwmon_power:
+		switch (attr) {
+		case hwmon_power_input:
+		case hwmon_power_crit_alarm:
+			if (has_power)
+				return 0444;
+			return 0;
+		case hwmon_power_crit:
+			if (has_alerts)
+				return 0644;
+			return 0;
 		default:
 			return 0;
 		}
@@ -701,7 +735,14 @@ static umode_t ina3221_is_visible(const void *drvdata,
 
 #define INA3221_HWMON_CURR_CONFIG (HWMON_C_INPUT | \
 				   HWMON_C_CRIT | HWMON_C_CRIT_ALARM | \
-				   HWMON_C_MAX | HWMON_C_MAX_ALARM)
+				   HWMON_C_MAX | HWMON_C_MAX_ALARM | \
+				   HWMON_C_LCRIT | HWMON_C_LCRIT_ALARM)
+#define SQ52210_HWMON_POWER_CONFIG (HWMON_P_INPUT | \
+				   HWMON_P_CRIT | HWMON_P_CRIT_ALARM)
+#define SQ52210_HWMON_BUS_CONFIG (HWMON_I_INPUT | \
+				   HWMON_I_ENABLE | HWMON_I_LABEL | \
+				   HWMON_I_LCRIT_ALARM | HWMON_I_LCRIT |\
+				   HWMON_I_CRIT_ALARM | HWMON_I_CRIT)
 
 static const struct hwmon_channel_info * const ina3221_info[] = {
 	HWMON_CHANNEL_INFO(chip,
@@ -711,9 +752,9 @@ static const struct hwmon_channel_info * const ina3221_info[] = {
 			   /* 0: dummy, skipped in is_visible */
 			   HWMON_I_INPUT,
 			   /* 1-3: input voltage Channels */
-			   HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL,
-			   HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL,
-			   HWMON_I_INPUT | HWMON_I_ENABLE | HWMON_I_LABEL,
+			   SQ52210_HWMON_BUS_CONFIG,
+			   SQ52210_HWMON_BUS_CONFIG,
+			   SQ52210_HWMON_BUS_CONFIG,
 			   /* 4-6: shunt voltage Channels */
 			   HWMON_I_INPUT,
 			   HWMON_I_INPUT,
@@ -727,6 +768,11 @@ static const struct hwmon_channel_info * const ina3221_info[] = {
 			   INA3221_HWMON_CURR_CONFIG,
 			   /* 4: summation of current channels */
 			   HWMON_C_INPUT | HWMON_C_CRIT | HWMON_C_CRIT_ALARM),
+	HWMON_CHANNEL_INFO(power,
+			   /* 1-3: power channels*/
+			   SQ52210_HWMON_POWER_CONFIG,
+			   SQ52210_HWMON_POWER_CONFIG,
+			   SQ52210_HWMON_POWER_CONFIG),
 	NULL
 };
 
@@ -748,7 +794,7 @@ static const struct hwmon_chip_info ina3221_chip_info = {
  * Calculate the value corresponding to one LSB of the current and
  * power registers.
  * formula : Current_LSB = Shunt_LSB / Rshunt
- *			 Power_LSB = power_lsb_factor * Current_LSB
+ *           Power_LSB = power_lsb_factor * Current_LSB
  */
 static int ina3221_set_shunt(struct ina3221_data *ina, unsigned long val)
 {
@@ -1023,8 +1069,8 @@ static int ina3221_probe(struct i2c_client *client)
 	}
 
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, ina,
-							&ina3221_chip_info,
-							ina3221_groups);
+							 &ina3221_chip_info,
+							 ina3221_groups);
 	if (IS_ERR(hwmon_dev)) {
 		dev_err(dev, "Unable to register hwmon device\n");
 		ret = PTR_ERR(hwmon_dev);
-- 
2.17.1

[PATCH 6/8] hwmon:(ina3221)Modify read/write functions for 'in' attribute

[Wenliang Yan]

SQ52210 adds current, power, and alert-limit sensors, with read/write
functions modified to accommodate these new changes.

The ina3221_read_value function has been rewritten to adapt to the
new register format for data reading.

The sq52210_alert_to_reg function has been added to handle reading
of different data types.

Each channel supports four new alert trigger modes, with only one
trigger active at any given time. Alert values are stored in the
same register. The sq52210_alert_limit_write function has been
implemented for writing alert threshold values.

The 'in' read/write functions have been modified to add crit,
lcrit, crit_alarm, and lcrit_alarm characteristics.

Signed-off-by: Wenliang Yan <wenliang202407@163.com>
---
 drivers/hwmon/ina3221.c | 182 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 178 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index 77b2505a49a2..abb6049c8eab 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -66,6 +66,9 @@
 #define INA3221_MASK_ENABLE_SCC_MASK	GENMASK(14, 12)
 
 #define SQ52210_ALERT_CONFIG_MASK	GENMASK(15, 4)
+#define SQ52210_MASK_ALERT_CHANNEL1 (BIT(15) | BIT(12) | BIT(9) | BIT(6))
+#define SQ52210_MASK_ALERT_CHANNEL2 (BIT(14) | BIT(11) | BIT(8) | BIT(5))
+#define SQ52210_MASK_ALERT_CHANNEL3 (BIT(13) | BIT(10) | BIT(7) | BIT(4))
 
 #define INA3221_CONFIG_DEFAULT		0x7127
 #define INA3221_RSHUNT_DEFAULT		10000
@@ -84,6 +87,9 @@ enum ina3221_fields {
 	/* Alert Flags: SF is the summation-alert flag */
 	F_SF, F_CF3, F_CF2, F_CF1,
 
+	/* Alert Flags: AFF is the alert function flag */
+	F_AFF3, F_AFF2, F_AFF1,
+
 	/* sentinel */
 	F_MAX_FIELDS
 };
@@ -99,6 +105,10 @@ static const struct reg_field ina3221_reg_fields[] = {
 	[F_CF3] = REG_FIELD(INA3221_MASK_ENABLE, 7, 7),
 	[F_CF2] = REG_FIELD(INA3221_MASK_ENABLE, 8, 8),
 	[F_CF1] = REG_FIELD(INA3221_MASK_ENABLE, 9, 9),
+
+	[F_AFF3] = REG_FIELD(SQ52210_ALERT_CONFIG, 1, 1),
+	[F_AFF2] = REG_FIELD(SQ52210_ALERT_CONFIG, 2, 2),
+	[F_AFF1] = REG_FIELD(SQ52210_ALERT_CONFIG, 3, 3),
 };
 
 enum ina3221_channels {
@@ -293,11 +303,39 @@ static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
 	 * Shunt Voltage Sum register has 14-bit value with 1-bit shift
 	 * Other Shunt Voltage registers have 12 bits with 3-bit shift
 	 */
-	if (reg == INA3221_SHUNT_SUM || reg == INA3221_CRIT_SUM)
+	switch (reg) {
+	case INA3221_SHUNT_SUM:
+	case INA3221_CRIT_SUM:
 		*val = sign_extend32(regval >> 1, 14);
-	else
+		break;
+	case SQ52210_CURRENT1:
+	case SQ52210_CURRENT2:
+	case SQ52210_CURRENT3:
+	case SQ52210_POWER1:
+	case SQ52210_POWER2:
+	case SQ52210_POWER3:
+		*val = regval;
+		break;
+	case INA3221_BUS1:
+	case INA3221_BUS2:
+	case INA3221_BUS3:
+	case INA3221_SHUNT1:
+	case INA3221_SHUNT2:
+	case INA3221_SHUNT3:
+	case INA3221_WARN1:
+	case INA3221_WARN2:
+	case INA3221_WARN3:
+	case INA3221_CRIT1:
+	case INA3221_CRIT2:
+	case INA3221_CRIT3:
 		*val = sign_extend32(regval >> 3, 12);
-
+		break;
+	case SQ52210_ALERT_LIMIT1:
+	case SQ52210_ALERT_LIMIT2:
+	case SQ52210_ALERT_LIMIT3:
+		*val = regval >> 3;
+		break;
+	};
 	return 0;
 }
 
@@ -311,6 +349,56 @@ static const u8 ina3221_in_reg[] = {
 	INA3221_SHUNT_SUM,
 };
 
+static const u8 alert_limit_reg[] = {
+	SQ52210_ALERT_LIMIT1,
+	SQ52210_ALERT_LIMIT2,
+	SQ52210_ALERT_LIMIT3,
+};
+
+static const u8 alert_flag[] = {
+	F_AFF1,
+	F_AFF2,
+	F_AFF3,
+};
+
+/*
+ * Turns alert limit values into register values.
+ * Opposite of the formula in ina3221_read_value().
+ */
+static u16 sq52210_alert_to_reg(struct ina3221_data *ina, int reg, long val)
+{
+	int regval;
+	/*
+	 * Formula to convert voltage_uv to register value:
+	 *     regval = (voltage_mv / scale) << shift
+	 * Results:
+	 *     bus_voltage: (1 / 8mV) << 3 = 1 mV
+	 */
+	switch (reg) {
+	case INA3221_BUS1:
+	case INA3221_BUS2:
+	case INA3221_BUS3:
+		/* clamp voltage */
+		regval = clamp_val(val, -32760, 32760);
+		return regval;
+	case SQ52210_CURRENT1:
+	case SQ52210_CURRENT2:
+	case SQ52210_CURRENT3:
+		/* signed register, result in mA */
+		regval = DIV_ROUND_CLOSEST(val * 8000, ina->current_lsb_uA);
+		return clamp_val(regval, -32760, 32760);
+	case SQ52210_POWER1:
+	case SQ52210_POWER2:
+	case SQ52210_POWER3:
+		regval = DIV_ROUND_CLOSEST(val * 8000, ina->power_lsb_uW);
+		return clamp_val(regval, 0, 65528);
+	default:
+		/* programmer goofed */
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+}
+
 static int ina3221_read_chip(struct device *dev, u32 attr, long *val)
 {
 	struct ina3221_data *ina = dev_get_drvdata(dev);
@@ -373,6 +461,25 @@ static int ina3221_read_in(struct device *dev, u32 attr, int channel, long *val)
 	case hwmon_in_enable:
 		*val = ina3221_is_enabled(ina, channel);
 		return 0;
+	case hwmon_in_crit:
+	case hwmon_in_lcrit:
+		reg = alert_limit_reg[channel];
+		ret = ina3221_read_value(ina, reg, &regval);
+		if (ret)
+			return ret;
+		/*
+		 * Scale of bus voltage (mV): LSB is 8mV
+		 */
+		*val = regval * 8;
+		return 0;
+	case hwmon_in_crit_alarm:
+	case hwmon_in_lcrit_alarm:
+		reg = alert_flag[channel];
+		ret = regmap_field_read(ina->fields[reg], &regval);
+		if (ret)
+			return ret;
+		*val = regval;
+		return 0;
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -450,6 +557,58 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
 	}
 }
 
+static const u32 sq52210_alert_mask[][INA3221_NUM_CHANNELS] = {
+	[hwmon_curr_lcrit] = { BIT(15), BIT(14), BIT(13) },
+	[hwmon_in_crit] = { BIT(12), BIT(11), BIT(10) },
+	[hwmon_in_lcrit] = { BIT(9), BIT(8), BIT(7) },
+	[hwmon_power_crit] = { BIT(6), BIT(5), BIT(4) },
+};
+
+static int sq52210_alert_limit_write(struct ina3221_data *ina, u32 attr, int channel, long val)
+{
+	struct regmap *regmap = ina->regmap;
+	int ret, limit_reg, item;
+	u32 alert_group;
+
+	if (val < 0)
+		return -EINVAL;
+	item = channel % INA3221_NUM_CHANNELS;
+	switch (item) {
+	case 0:
+		alert_group = SQ52210_MASK_ALERT_CHANNEL1;
+		limit_reg = SQ52210_ALERT_LIMIT1;
+		break;
+	case 1:
+		alert_group = SQ52210_MASK_ALERT_CHANNEL2;
+		limit_reg = SQ52210_ALERT_LIMIT2;
+		break;
+	case 2:
+		alert_group = SQ52210_MASK_ALERT_CHANNEL3;
+		limit_reg = SQ52210_ALERT_LIMIT3;
+		break;
+	default:
+		break;
+	}
+	/*
+	 * Clear all alerts first to avoid accidentally triggering ALERT pin
+	 * due to register write sequence. Then, only enable the alert
+	 * if the value is non-zero.
+	 */
+	ret = regmap_update_bits(regmap, SQ52210_ALERT_CONFIG,
+				alert_group, 0);
+	if (ret < 0)
+		return ret;
+	ret = regmap_write(regmap, limit_reg,
+			sq52210_alert_to_reg(ina, ina3221_curr_reg[attr][item], val));
+	if (ret < 0)
+		return ret;
+
+	if (val)
+		return regmap_update_bits(regmap, SQ52210_ALERT_CONFIG,
+					alert_group, sq52210_alert_mask[attr][item]);
+	return 0;
+}
+
 static int ina3221_write_chip(struct device *dev, u32 attr, long val)
 {
 	struct ina3221_data *ina = dev_get_drvdata(dev);
@@ -586,6 +745,21 @@ static int ina3221_write_enable(struct device *dev, int channel, bool enable)
 	return ret;
 }
 
+static int ina3221_write_in(struct device *dev, u32 attr, int channel, long val)
+{
+	struct ina3221_data *ina = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_in_lcrit:
+		return sq52210_alert_limit_write(ina, attr, channel, val);
+	case hwmon_in_crit:
+		return sq52210_alert_limit_write(ina, attr, channel, val);
+	case hwmon_in_enable:
+		return ina3221_write_enable(dev, channel, val);
+	default:
+		return 0;
+	}
+}
 static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,
 			u32 attr, int channel, long *val)
 {
@@ -620,7 +794,7 @@ static int ina3221_write(struct device *dev, enum hwmon_sensor_types type,
 		break;
 	case hwmon_in:
 		/* 0-align channel ID */
-		ret = ina3221_write_enable(dev, channel - 1, val);
+		ret = ina3221_write_in(dev, attr, channel - 1, val);
 		break;
 	case hwmon_curr:
 		ret = ina3221_write_curr(dev, attr, channel, val);
-- 
2.17.1

[PATCH 7/8] hwmon:(ina3221)Support read/write functions for 'power' attribute

[Wenliang Yan]

SQ52210 adds power attribute to report power data, and implements
read/write functions for this purpose.

Signed-off-by: Wenliang Yan <wenliang202407@163.com>
---
 drivers/hwmon/ina3221.c | 79 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 75 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index abb6049c8eab..ea01687ad1fa 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -348,19 +348,16 @@ static const u8 ina3221_in_reg[] = {
 	INA3221_SHUNT3,
 	INA3221_SHUNT_SUM,
 };
-
 static const u8 alert_limit_reg[] = {
 	SQ52210_ALERT_LIMIT1,
 	SQ52210_ALERT_LIMIT2,
 	SQ52210_ALERT_LIMIT3,
 };
-
 static const u8 alert_flag[] = {
 	F_AFF1,
 	F_AFF2,
 	F_AFF3,
 };
-
 /*
  * Turns alert limit values into register values.
  * Opposite of the formula in ina3221_read_value().
@@ -557,6 +554,61 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
 	}
 }
 
+static const u8 ina3221_power_reg[][INA3221_NUM_CHANNELS] = {
+	[hwmon_power_input] = { SQ52210_POWER1, SQ52210_POWER2, SQ52210_POWER3 },
+	[hwmon_power_crit] = { SQ52210_ALERT_LIMIT1, SQ52210_ALERT_LIMIT2,
+						SQ52210_ALERT_LIMIT3 },
+	[hwmon_power_crit_alarm] = { F_AFF1, F_AFF2, F_AFF3 },
+};
+
+static int ina3221_read_power(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct ina3221_data *ina = dev_get_drvdata(dev);
+	u8 reg = ina3221_power_reg[attr][channel];
+	int regval, ret;
+
+	switch (attr) {
+	case hwmon_power_input:
+		if (!ina3221_is_enabled(ina, channel))
+			return -ENODATA;
+
+		/* Write CONFIG register to trigger a single-shot measurement */
+		if (ina->single_shot) {
+			regmap_write(ina->regmap, INA3221_CONFIG,
+				     ina->reg_config);
+
+			ret = ina3221_wait_for_data(ina);
+			if (ret)
+				return ret;
+		}
+
+		ret = ina3221_read_value(ina, reg, &regval);
+		if (ret)
+			return ret;
+		/* Return power in mW */
+		*val = DIV_ROUND_CLOSEST(regval * ina->power_lsb_uW, 1000);
+		return 0;
+	case hwmon_power_crit:
+		reg = ina3221_power_reg[attr][channel];
+		ret = ina3221_read_value(ina, reg, &regval);
+		if (ret)
+			return ret;
+		/* Return power in mW */
+		*val = DIV_ROUND_CLOSEST(regval * ina->power_lsb_uW, 1000);
+		return 0;
+	case hwmon_power_crit_alarm:
+		reg = ina3221_power_reg[attr][channel];
+		ret = regmap_field_read(ina->fields[reg], &regval);
+		if (ret)
+			return ret;
+		*val = regval;
+		return 0;
+
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static const u32 sq52210_alert_mask[][INA3221_NUM_CHANNELS] = {
 	[hwmon_curr_lcrit] = { BIT(15), BIT(14), BIT(13) },
 	[hwmon_in_crit] = { BIT(12), BIT(11), BIT(10) },
@@ -760,6 +812,19 @@ static int ina3221_write_in(struct device *dev, u32 attr, int channel, long val)
 		return 0;
 	}
 }
+
+static int ina3221_write_power(struct device *dev, u32 attr, int channel, long val)
+{
+	struct ina3221_data *ina = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_power_crit:
+		return sq52210_alert_limit_write(ina, attr, channel, val);
+	default:
+		return 0;
+	}
+}
+
 static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,
 			u32 attr, int channel, long *val)
 {
@@ -776,6 +841,9 @@ static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,
 	case hwmon_curr:
 		ret = ina3221_read_curr(dev, attr, channel, val);
 		break;
+	case hwmon_power:
+		ret = ina3221_read_power(dev, attr, channel, val);
+		break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
@@ -799,6 +867,9 @@ static int ina3221_write(struct device *dev, enum hwmon_sensor_types type,
 	case hwmon_curr:
 		ret = ina3221_write_curr(dev, attr, channel, val);
 		break;
+	case hwmon_power:
+		ret = ina3221_write_power(dev, attr, channel, val);
+		break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
@@ -977,7 +1048,7 @@ static int ina3221_set_shunt(struct ina3221_data *ina, unsigned long val)
 
 	ina->current_lsb_uA = DIV_ROUND_CLOSEST(SQ52210_SHUNT_LSB, val);
 	ina->power_lsb_uW = ina->config->power_lsb_factor *
-			    ina->current_lsb_uA;
+			     ina->current_lsb_uA;
 
 	return 0;
 }
-- 
2.17.1

[PATCH 8/8] hwmon:(ina3221)Support read/write functions for current_lcrict attribute

[Wenliang Yan]

Modify the read/write functions for current attributes.
SQ52210 can directly use its internal current registers to compare
with alert values for implementing curr_lcrit functionality.

Signed-off-by: Wenliang Yan <wenliang202407@163.com>
---
 drivers/hwmon/ina3221.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
index ea01687ad1fa..50916ce26cb3 100644
--- a/drivers/hwmon/ina3221.c
+++ b/drivers/hwmon/ina3221.c
@@ -488,8 +488,11 @@ static const u8 ina3221_curr_reg[][INA3221_NUM_CHANNELS + 1] = {
 	[hwmon_curr_max] = { INA3221_WARN1, INA3221_WARN2, INA3221_WARN3, 0 },
 	[hwmon_curr_crit] = { INA3221_CRIT1, INA3221_CRIT2,
 			      INA3221_CRIT3, INA3221_CRIT_SUM },
+	[hwmon_curr_lcrit] = { SQ52210_ALERT_LIMIT1, SQ52210_ALERT_LIMIT2,
+						SQ52210_ALERT_LIMIT3, 0 },
 	[hwmon_curr_max_alarm] = { F_WF1, F_WF2, F_WF3, 0 },
 	[hwmon_curr_crit_alarm] = { F_CF1, F_CF2, F_CF3, F_SF },
+	[hwmon_curr_lcrit_alarm] = { F_AFF1, F_AFF2, F_AFF3, 0 },
 };
 
 static int ina3221_read_curr(struct device *dev, u32 attr,
@@ -536,8 +539,20 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
 		/* Return current in mA */
 		*val = DIV_ROUND_CLOSEST(voltage_nv, resistance_uo);
 		return 0;
+	case hwmon_curr_lcrit:
+		if (!resistance_uo)
+			return -ENODATA;
+
+		ret = ina3221_read_value(ina, reg, &regval);
+		if (ret)
+			return ret;
+
+		/* Return current in mA */
+		*val = DIV_ROUND_CLOSEST(regval * ina->current_lsb_uA, 1000);
+		return 0;
 	case hwmon_curr_crit_alarm:
 	case hwmon_curr_max_alarm:
+	case hwmon_curr_lcrit_alarm:
 		/* No actual register read if channel is disabled */
 		if (!ina3221_is_enabled(ina, channel)) {
 			/* Return 0 for alert flags */
@@ -703,10 +718,9 @@ static int ina3221_write_chip(struct device *dev, u32 attr, long val)
 	}
 }
 
-static int ina3221_write_curr(struct device *dev, u32 attr,
+static int ina3221_write_curr_shunt(struct ina3221_data *ina, u32 attr,
 			      int channel, long val)
 {
-	struct ina3221_data *ina = dev_get_drvdata(dev);
 	struct ina3221_input *input = ina->inputs;
 	u8 reg = ina3221_curr_reg[attr][channel];
 	int resistance_uo, current_ma, voltage_uv;
@@ -749,6 +763,22 @@ static int ina3221_write_curr(struct device *dev, u32 attr,
 	return regmap_write(ina->regmap, reg, regval);
 }
 
+static int ina3221_write_curr(struct device *dev, u32 attr,
+			      int channel, long val)
+{
+	struct ina3221_data *ina = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_curr_crit:
+	case hwmon_curr_max:
+		return ina3221_write_curr_shunt(ina, attr, channel, val);
+	case hwmon_curr_lcrit:
+		return sq52210_alert_limit_write(ina, attr, channel, val);
+	default:
+		return 0;
+	}
+}
+
 static int ina3221_write_enable(struct device *dev, int channel, bool enable)
 {
 	struct ina3221_data *ina = dev_get_drvdata(dev);
-- 
2.17.1

Re: [PATCH 1/8] dt-binding:ti,ina3221:Add SQ52210

[Krzysztof Kozlowski]

On 11/11/2025 09:05, Wenliang Yan wrote:
> Add a compatible string for sq52210, sq52210 is forward compatible
> with INA3221 and add alert register to implement four additional
> alert function.
> 
> Signed-off-by: Wenliang Yan <wenliang202407@163.com>


Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

It is never "dt-binding:". Also, all spaces are gone. Just look at `git
log`.


> ---
>  .../devicetree/bindings/hwmon/ti,ina3221.yaml    | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
> index 5f10f1207d69..0fae82ca3ee1 100644
> --- a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
> @@ -12,7 +12,9 @@ maintainers:
>  
>  properties:
>    compatible:
> -    const: ti,ina3221
> +    enum:
> +      - silergy,sq52210
> +      - ti,ina3221
>  
>    reg:
>      maxItems: 1
> @@ -77,6 +79,18 @@ patternProperties:
>            exclude specific channels from the summation control function.
>          type: boolean
>  
> +      alert-type:

Not a generic property, missing type and vendor prefix.

This clearly was not tested.


> +        description: |
> +          The SQ52210 features a configurable alert function with four
> +          types: SUL, BOL, BUL, and POL. Each channel can be configured to
> +          select one of these types to enable the alert function. This alert
> +          function can operate concurrently with both Critical and Warning
> +          functions.
> +
> +          The configuration must use numerical values 0 through 3,


Don't repeat constraints in free form text.

> +          0 corresponds to SUL, 1 to BOL, 2 to BUL, and 3 to POL.
> +        enum: [ 0, 1, 2, 3 ]

No, use string enum instead.

Anyway, does not look like DT property. Why would alert type be set per
board? Why I cannot change the alert during runtime?



Best regards,
Krzysztof

Re: [PATCH 1/8] dt-binding:ti,ina3221:Add SQ52210

[Rob Herring (Arm)]

On Tue, 11 Nov 2025 03:05:39 -0500, Wenliang Yan wrote:
> Add a compatible string for sq52210, sq52210 is forward compatible
> with INA3221 and add alert register to implement four additional
> alert function.
> 
> Signed-off-by: Wenliang Yan <wenliang202407@163.com>
> ---
>  .../devicetree/bindings/hwmon/ti,ina3221.yaml    | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml: alert-type: missing type definition

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20251111080546.32421-2-wenliang202407@163.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 1/8] dt-binding:ti,ina3221:Add SQ52210

[Wenliang Yan]

At 2025-11-11 17:32:18, "Rob Herring (Arm)" <robh@kernel.org> wrote:
>
>On Tue, 11 Nov 2025 03:05:39 -0500, Wenliang Yan wrote:
>> Add a compatible string for sq52210, sq52210 is forward compatible
>> with INA3221 and add alert register to implement four additional
>> alert function.
>> 
>> Signed-off-by: Wenliang Yan <wenliang202407@163.com>
>> ---
>>  .../devicetree/bindings/hwmon/ti,ina3221.yaml    | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>> 
>
>My bot found errors running 'make dt_binding_check' on your patch:
>
>yamllint warnings/errors:
>
>dtschema/dtc warnings/errors:
>/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml: alert-type: missing type definition
>
>doc reference errors (make refcheckdocs):
>
>See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20251111080546.32421-2-wenliang202407@163.com
>
>The base for the series is generally the latest rc1. A different dependency
>should be noted in *this* patch.
>
>If you already ran 'make dt_binding_check' and didn't see the above
>error(s), then make sure 'yamllint' is installed and dt-schema is up to
>date:
>
>pip3 install dtschema --upgrade
>
>Please check and re-submit after running the above command yourself. Note
>that DT_SCHEMA_FILES can be set to your schema file to speed up checking
>your schema. However, it must be unset to test all examples with your schema.

I apologize that my configuration error prevented these issues from being detected during runtime.
I will update the tools and retest before the next submission.

Thanks,
Wenlaing Yan

Re: [PATCH 1/8] dt-binding:ti,ina3221:Add SQ52210

[Wenliang Yan]

At 2025-11-11 16:17:26, "Krzysztof Kozlowski" <krzk@kernel.org> wrote:
>On 11/11/2025 09:05, Wenliang Yan wrote:
>> Add a compatible string for sq52210, sq52210 is forward compatible
>> with INA3221 and add alert register to implement four additional
>> alert function.
>> 
>> Signed-off-by: Wenliang Yan <wenliang202407@163.com>
>
>
>Please use subject prefixes matching the subsystem. You can get them for
>example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>your patch is touching. For bindings, the preferred subjects are
>explained here:
>https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
>
>It is never "dt-binding:". Also, all spaces are gone. Just look at `git
>log`.
>

I will change the subject to "dt-bindings: hwmon: ti,ina3221: Add SQ52210" before the next submission.

>
>> ---
>>  .../devicetree/bindings/hwmon/ti,ina3221.yaml    | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
>> index 5f10f1207d69..0fae82ca3ee1 100644
>> --- a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
>> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
>> @@ -12,7 +12,9 @@ maintainers:
>>  
>>  properties:
>>    compatible:
>> -    const: ti,ina3221
>> +    enum:
>> +      - silergy,sq52210
>> +      - ti,ina3221
>>  
>>    reg:
>>      maxItems: 1
>> @@ -77,6 +79,18 @@ patternProperties:
>>            exclude specific channels from the summation control function.
>>          type: boolean
>>  
>> +      alert-type:
>
>Not a generic property, missing type and vendor prefix.
>
>This clearly was not tested.
>
>
>> +        description: |
>> +          The SQ52210 features a configurable alert function with four
>> +          types: SUL, BOL, BUL, and POL. Each channel can be configured to
>> +          select one of these types to enable the alert function. This alert
>> +          function can operate concurrently with both Critical and Warning
>> +          functions.
>> +
>> +          The configuration must use numerical values 0 through 3,
>
>
>Don't repeat constraints in free form text.
>
>> +          0 corresponds to SUL, 1 to BOL, 2 to BUL, and 3 to POL.
>> +        enum: [ 0, 1, 2, 3 ]
>
>No, use string enum instead.
>
>Anyway, does not look like DT property. Why would alert type be set per
>board? Why I cannot change the alert during runtime?
>
>

You are right, my previous consideration was problematic. Indeed, there is
no need to set the alert type at the board level. I will modify this
content and test it before the next submission.


Thanks,
Wenlaing Yan

Re: [PATCH 6/8] hwmon:(ina3221)Modify read/write functions for 'in' attribute

[kernel test robot]

Hi Wenliang,

kernel test robot noticed the following build warnings:

[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on linus/master v6.18-rc5 next-20251111]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Wenliang-Yan/dt-binding-ti-ina3221-Add-SQ52210/20251111-161425
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20251111080546.32421-7-wenliang202407%40163.com
patch subject: [PATCH 6/8] hwmon:(ina3221)Modify read/write functions for 'in' attribute
config: i386-randconfig-013-20251112 (https://download.01.org/0day-ci/archive/20251112/202511121034.856ivnlW-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251112/202511121034.856ivnlW-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511121034.856ivnlW-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/hwmon/ina3221.c:563:21: warning: initializer overrides prior initialization of this subobject [-Winitializer-overrides]
     563 |         [hwmon_in_lcrit] = { BIT(9), BIT(8), BIT(7) },
         |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/hwmon/ina3221.c:561:23: note: previous initialization is here
     561 |         [hwmon_curr_lcrit] = { BIT(15), BIT(14), BIT(13) },
         |                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/hwmon/ina3221.c:589:2: warning: variable 'alert_group' is used uninitialized whenever switch default is taken [-Wsometimes-uninitialized]
     589 |         default:
         |         ^~~~~~~
   drivers/hwmon/ina3221.c:598:5: note: uninitialized use occurs here
     598 |                                 alert_group, 0);
         |                                 ^~~~~~~~~~~
   drivers/hwmon/ina3221.c:571:17: note: initialize the variable 'alert_group' to silence this warning
     571 |         u32 alert_group;
         |                        ^
         |                         = 0
   2 warnings generated.


vim +/alert_group +589 drivers/hwmon/ina3221.c

   559	
   560	static const u32 sq52210_alert_mask[][INA3221_NUM_CHANNELS] = {
   561		[hwmon_curr_lcrit] = { BIT(15), BIT(14), BIT(13) },
   562		[hwmon_in_crit] = { BIT(12), BIT(11), BIT(10) },
 > 563		[hwmon_in_lcrit] = { BIT(9), BIT(8), BIT(7) },
   564		[hwmon_power_crit] = { BIT(6), BIT(5), BIT(4) },
   565	};
   566	
   567	static int sq52210_alert_limit_write(struct ina3221_data *ina, u32 attr, int channel, long val)
   568	{
   569		struct regmap *regmap = ina->regmap;
   570		int ret, limit_reg, item;
   571		u32 alert_group;
   572	
   573		if (val < 0)
   574			return -EINVAL;
   575		item = channel % INA3221_NUM_CHANNELS;
   576		switch (item) {
   577		case 0:
   578			alert_group = SQ52210_MASK_ALERT_CHANNEL1;
   579			limit_reg = SQ52210_ALERT_LIMIT1;
   580			break;
   581		case 1:
   582			alert_group = SQ52210_MASK_ALERT_CHANNEL2;
   583			limit_reg = SQ52210_ALERT_LIMIT2;
   584			break;
   585		case 2:
   586			alert_group = SQ52210_MASK_ALERT_CHANNEL3;
   587			limit_reg = SQ52210_ALERT_LIMIT3;
   588			break;
 > 589		default:
   590			break;
   591		}
   592		/*
   593		 * Clear all alerts first to avoid accidentally triggering ALERT pin
   594		 * due to register write sequence. Then, only enable the alert
   595		 * if the value is non-zero.
   596		 */
   597		ret = regmap_update_bits(regmap, SQ52210_ALERT_CONFIG,
   598					alert_group, 0);
   599		if (ret < 0)
   600			return ret;
   601		ret = regmap_write(regmap, limit_reg,
   602				sq52210_alert_to_reg(ina, ina3221_curr_reg[attr][item], val));
   603		if (ret < 0)
   604			return ret;
   605	
   606		if (val)
   607			return regmap_update_bits(regmap, SQ52210_ALERT_CONFIG,
   608						alert_group, sq52210_alert_mask[attr][item]);
   609		return 0;
   610	}
   611	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/8] dt-binding:ti,ina3221:Add SQ52210

[Guenter Roeck]

On 11/11/25 00:05, Wenliang Yan wrote:
> Add a compatible string for sq52210, sq52210 is forward compatible
> with INA3221 and add alert register to implement four additional
> alert function.
> 
> Signed-off-by: Wenliang Yan <wenliang202407@163.com>
> ---
>   .../devicetree/bindings/hwmon/ti,ina3221.yaml    | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
> index 5f10f1207d69..0fae82ca3ee1 100644
> --- a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
> @@ -12,7 +12,9 @@ maintainers:
>   
>   properties:
>     compatible:
> -    const: ti,ina3221
> +    enum:
> +      - silergy,sq52210
> +      - ti,ina3221
>   
>     reg:
>       maxItems: 1
> @@ -77,6 +79,18 @@ patternProperties:
>             exclude specific channels from the summation control function.
>           type: boolean
>   
> +      alert-type:
> +        description: |
> +          The SQ52210 features a configurable alert function with four
> +          types: SUL, BOL, BUL, and POL. Each channel can be configured to
> +          select one of these types to enable the alert function. This alert
> +          function can operate concurrently with both Critical and Warning
> +          functions.
> +
> +          The configuration must use numerical values 0 through 3,
> +          0 corresponds to SUL, 1 to BOL, 2 to BUL, and 3 to POL.
> +        enum: [ 0, 1, 2, 3 ]
> +

Per datasheet, each of the alerts can be enabled independently. It is possible
to enable SUL, BOL, BUL, and POL on each channel at the same time. This is not
possible with the above property since it only permits enabling alerts for one
of the alert sources on each channel.

Also, I am not sure if it makes sense to have this as devicetree property.
It is not really a board property. It might make more sense to tie enabling
the alerts automatically if a channel is enabled and a limit is set for a
given channel.

Guenter

Re: [PATCH 6/8] hwmon:(ina3221)Modify read/write functions for 'in' attribute

[Guenter Roeck]

On Tue, Nov 11, 2025 at 03:05:44AM -0500, Wenliang Yan wrote:
> SQ52210 adds current, power, and alert-limit sensors, with read/write
> functions modified to accommodate these new changes.
> 
> The ina3221_read_value function has been rewritten to adapt to the
> new register format for data reading.
> 
> The sq52210_alert_to_reg function has been added to handle reading
> of different data types.
> 
> Each channel supports four new alert trigger modes, with only one
> trigger active at any given time. Alert values are stored in the
> same register. The sq52210_alert_limit_write function has been
> implemented for writing alert threshold values.
> 
> The 'in' read/write functions have been modified to add crit,
> lcrit, crit_alarm, and lcrit_alarm characteristics.
> 
> Signed-off-by: Wenliang Yan <wenliang202407@163.com>
> ---
>  drivers/hwmon/ina3221.c | 182 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 178 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index 77b2505a49a2..abb6049c8eab 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -66,6 +66,9 @@
>  #define INA3221_MASK_ENABLE_SCC_MASK	GENMASK(14, 12)
>  
>  #define SQ52210_ALERT_CONFIG_MASK	GENMASK(15, 4)
> +#define SQ52210_MASK_ALERT_CHANNEL1 (BIT(15) | BIT(12) | BIT(9) | BIT(6))
> +#define SQ52210_MASK_ALERT_CHANNEL2 (BIT(14) | BIT(11) | BIT(8) | BIT(5))
> +#define SQ52210_MASK_ALERT_CHANNEL3 (BIT(13) | BIT(10) | BIT(7) | BIT(4))
>  
>  #define INA3221_CONFIG_DEFAULT		0x7127
>  #define INA3221_RSHUNT_DEFAULT		10000
> @@ -84,6 +87,9 @@ enum ina3221_fields {
>  	/* Alert Flags: SF is the summation-alert flag */
>  	F_SF, F_CF3, F_CF2, F_CF1,
>  
> +	/* Alert Flags: AFF is the alert function flag */
> +	F_AFF3, F_AFF2, F_AFF1,
> +
>  	/* sentinel */
>  	F_MAX_FIELDS
>  };
> @@ -99,6 +105,10 @@ static const struct reg_field ina3221_reg_fields[] = {
>  	[F_CF3] = REG_FIELD(INA3221_MASK_ENABLE, 7, 7),
>  	[F_CF2] = REG_FIELD(INA3221_MASK_ENABLE, 8, 8),
>  	[F_CF1] = REG_FIELD(INA3221_MASK_ENABLE, 9, 9),
> +
> +	[F_AFF3] = REG_FIELD(SQ52210_ALERT_CONFIG, 1, 1),
> +	[F_AFF2] = REG_FIELD(SQ52210_ALERT_CONFIG, 2, 2),
> +	[F_AFF1] = REG_FIELD(SQ52210_ALERT_CONFIG, 3, 3),
>  };
>  
>  enum ina3221_channels {
> @@ -293,11 +303,39 @@ static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
>  	 * Shunt Voltage Sum register has 14-bit value with 1-bit shift
>  	 * Other Shunt Voltage registers have 12 bits with 3-bit shift
>  	 */
> -	if (reg == INA3221_SHUNT_SUM || reg == INA3221_CRIT_SUM)
> +	switch (reg) {
> +	case INA3221_SHUNT_SUM:
> +	case INA3221_CRIT_SUM:
>  		*val = sign_extend32(regval >> 1, 14);
> -	else
> +		break;
> +	case SQ52210_CURRENT1:
> +	case SQ52210_CURRENT2:
> +	case SQ52210_CURRENT3:
> +	case SQ52210_POWER1:
> +	case SQ52210_POWER2:
> +	case SQ52210_POWER3:
> +		*val = regval;
> +		break;
> +	case INA3221_BUS1:
> +	case INA3221_BUS2:
> +	case INA3221_BUS3:
> +	case INA3221_SHUNT1:
> +	case INA3221_SHUNT2:
> +	case INA3221_SHUNT3:
> +	case INA3221_WARN1:
> +	case INA3221_WARN2:
> +	case INA3221_WARN3:
> +	case INA3221_CRIT1:
> +	case INA3221_CRIT2:
> +	case INA3221_CRIT3:
>  		*val = sign_extend32(regval >> 3, 12);
> -
> +		break;
> +	case SQ52210_ALERT_LIMIT1:
> +	case SQ52210_ALERT_LIMIT2:
> +	case SQ52210_ALERT_LIMIT3:
> +		*val = regval >> 3;
> +		break;
> +	};

This returns 0 if the register is not listed in the case statement while leaving
val unset. It would probably be better to drop this function entirely and handle the
conversions in the calling code.

>  	return 0;
>  }
>  
> @@ -311,6 +349,56 @@ static const u8 ina3221_in_reg[] = {
>  	INA3221_SHUNT_SUM,
>  };
>  
> +static const u8 alert_limit_reg[] = {
> +	SQ52210_ALERT_LIMIT1,
> +	SQ52210_ALERT_LIMIT2,
> +	SQ52210_ALERT_LIMIT3,
> +};
> +
> +static const u8 alert_flag[] = {
> +	F_AFF1,
> +	F_AFF2,
> +	F_AFF3,
> +};
> +
> +/*
> + * Turns alert limit values into register values.
> + * Opposite of the formula in ina3221_read_value().
> + */
> +static u16 sq52210_alert_to_reg(struct ina3221_data *ina, int reg, long val)
> +{
> +	int regval;
> +	/*
> +	 * Formula to convert voltage_uv to register value:
> +	 *     regval = (voltage_mv / scale) << shift
> +	 * Results:
> +	 *     bus_voltage: (1 / 8mV) << 3 = 1 mV
> +	 */
> +	switch (reg) {
> +	case INA3221_BUS1:
> +	case INA3221_BUS2:
> +	case INA3221_BUS3:
> +		/* clamp voltage */
> +		regval = clamp_val(val, -32760, 32760);
> +		return regval;
> +	case SQ52210_CURRENT1:
> +	case SQ52210_CURRENT2:
> +	case SQ52210_CURRENT3:
> +		/* signed register, result in mA */
> +		regval = DIV_ROUND_CLOSEST(val * 8000, ina->current_lsb_uA);
> +		return clamp_val(regval, -32760, 32760);
> +	case SQ52210_POWER1:
> +	case SQ52210_POWER2:
> +	case SQ52210_POWER3:
> +		regval = DIV_ROUND_CLOSEST(val * 8000, ina->power_lsb_uW);
> +		return clamp_val(regval, 0, 65528);
> +	default:
> +		/* programmer goofed */
> +		WARN_ON_ONCE(1);
> +		return 0;

Same as above. I know other code uses the samew approach, but this kind of
"validation" would be better to avoid. It would be much better to handle the
conversions in the calling code.

> +	}
> +}
> +
>  static int ina3221_read_chip(struct device *dev, u32 attr, long *val)
>  {
>  	struct ina3221_data *ina = dev_get_drvdata(dev);
> @@ -373,6 +461,25 @@ static int ina3221_read_in(struct device *dev, u32 attr, int channel, long *val)
>  	case hwmon_in_enable:
>  		*val = ina3221_is_enabled(ina, channel);
>  		return 0;
> +	case hwmon_in_crit:
> +	case hwmon_in_lcrit:
> +		reg = alert_limit_reg[channel];
> +		ret = ina3221_read_value(ina, reg, &regval);
> +		if (ret)
> +			return ret;
> +		/*
> +		 * Scale of bus voltage (mV): LSB is 8mV
> +		 */
> +		*val = regval * 8;
> +		return 0;
> +	case hwmon_in_crit_alarm:
> +	case hwmon_in_lcrit_alarm:
> +		reg = alert_flag[channel];
> +		ret = regmap_field_read(ina->fields[reg], &regval);
> +		if (ret)
> +			return ret;
> +		*val = regval;
> +		return 0;
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> @@ -450,6 +557,58 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
>  	}
>  }
>  
> +static const u32 sq52210_alert_mask[][INA3221_NUM_CHANNELS] = {
> +	[hwmon_curr_lcrit] = { BIT(15), BIT(14), BIT(13) },
> +	[hwmon_in_crit] = { BIT(12), BIT(11), BIT(10) },
> +	[hwmon_in_lcrit] = { BIT(9), BIT(8), BIT(7) },
> +	[hwmon_power_crit] = { BIT(6), BIT(5), BIT(4) },

This does not work. hwmon_curr_xxx, hwmon_inxxx, and hwmon_power_xxx use
different and overlapping number spaces. Even if that was not the case,
the above would result in a serverely sparse array, and the callingo code
would have to make sure that the row is actually populated before using it.

> +};
> +
> +static int sq52210_alert_limit_write(struct ina3221_data *ina, u32 attr, int channel, long val)
> +{
> +	struct regmap *regmap = ina->regmap;
> +	int ret, limit_reg, item;
> +	u32 alert_group;
> +
> +	if (val < 0)
> +		return -EINVAL;
> +	item = channel % INA3221_NUM_CHANNELS;
> +	switch (item) {
> +	case 0:
> +		alert_group = SQ52210_MASK_ALERT_CHANNEL1;
> +		limit_reg = SQ52210_ALERT_LIMIT1;
> +		break;
> +	case 1:
> +		alert_group = SQ52210_MASK_ALERT_CHANNEL2;
> +		limit_reg = SQ52210_ALERT_LIMIT2;
> +		break;
> +	case 2:
> +		alert_group = SQ52210_MASK_ALERT_CHANNEL3;
> +		limit_reg = SQ52210_ALERT_LIMIT3;
> +		break;
> +	default:
> +		break;
> +	}
> +	/*
> +	 * Clear all alerts first to avoid accidentally triggering ALERT pin
> +	 * due to register write sequence. Then, only enable the alert
> +	 * if the value is non-zero.
> +	 */
> +	ret = regmap_update_bits(regmap, SQ52210_ALERT_CONFIG,
> +				alert_group, 0);
> +	if (ret < 0)
> +		return ret;
> +	ret = regmap_write(regmap, limit_reg,
> +			sq52210_alert_to_reg(ina, ina3221_curr_reg[attr][item], val));
> +	if (ret < 0)
> +		return ret;
> +
> +	if (val)
> +		return regmap_update_bits(regmap, SQ52210_ALERT_CONFIG,
> +					alert_group, sq52210_alert_mask[attr][item]);
> +	return 0;
> +}
> +
>  static int ina3221_write_chip(struct device *dev, u32 attr, long val)
>  {
>  	struct ina3221_data *ina = dev_get_drvdata(dev);
> @@ -586,6 +745,21 @@ static int ina3221_write_enable(struct device *dev, int channel, bool enable)
>  	return ret;
>  }
>  
> +static int ina3221_write_in(struct device *dev, u32 attr, int channel, long val)
> +{
> +	struct ina3221_data *ina = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_in_lcrit:
> +		return sq52210_alert_limit_write(ina, attr, channel, val);
> +	case hwmon_in_crit:
> +		return sq52210_alert_limit_write(ina, attr, channel, val);
> +	case hwmon_in_enable:
> +		return ina3221_write_enable(dev, channel, val);
> +	default:
> +		return 0;
> +	}
> +}
>  static int ina3221_read(struct device *dev, enum hwmon_sensor_types type,
>  			u32 attr, int channel, long *val)
>  {
> @@ -620,7 +794,7 @@ static int ina3221_write(struct device *dev, enum hwmon_sensor_types type,
>  		break;
>  	case hwmon_in:
>  		/* 0-align channel ID */
> -		ret = ina3221_write_enable(dev, channel - 1, val);
> +		ret = ina3221_write_in(dev, attr, channel - 1, val);
>  		break;
>  	case hwmon_curr:
>  		ret = ina3221_write_curr(dev, attr, channel, val);
> -- 
> 2.17.1
> 
> 

Re: [PATCH 7/8] hwmon:(ina3221)Support read/write functions for 'power' attribute

[Guenter Roeck]

On Tue, Nov 11, 2025 at 03:05:45AM -0500, Wenliang Yan wrote:
> SQ52210 adds power attribute to report power data, and implements
> read/write functions for this purpose.
> 
> Signed-off-by: Wenliang Yan <wenliang202407@163.com>
> ---
>  drivers/hwmon/ina3221.c | 79 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 75 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
> index abb6049c8eab..ea01687ad1fa 100644
> --- a/drivers/hwmon/ina3221.c
> +++ b/drivers/hwmon/ina3221.c
> @@ -348,19 +348,16 @@ static const u8 ina3221_in_reg[] = {
>  	INA3221_SHUNT3,
>  	INA3221_SHUNT_SUM,
>  };
> -
>  static const u8 alert_limit_reg[] = {
>  	SQ52210_ALERT_LIMIT1,
>  	SQ52210_ALERT_LIMIT2,
>  	SQ52210_ALERT_LIMIT3,
>  };
> -
>  static const u8 alert_flag[] = {
>  	F_AFF1,
>  	F_AFF2,
>  	F_AFF3,
>  };
> -

Please refrain from making such cosmetic changes.

Guenter

Re: [PATCH 1/8] dt-binding:ti,ina3221:Add SQ52210

[Wenliang Yan]

At 2025-11-13 10:03:01, "Guenter Roeck" <linux@roeck-us.net> wrote:
>On 11/11/25 00:05, Wenliang Yan wrote:
>> Add a compatible string for sq52210, sq52210 is forward compatible
>> with INA3221 and add alert register to implement four additional
>> alert function.
>> 
>> Signed-off-by: Wenliang Yan <wenliang202407@163.com>
>> ---
>>   .../devicetree/bindings/hwmon/ti,ina3221.yaml    | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
>> index 5f10f1207d69..0fae82ca3ee1 100644
>> --- a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
>> +++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
>> @@ -12,7 +12,9 @@ maintainers:
>>   
>>   properties:
>>     compatible:
>> -    const: ti,ina3221
>> +    enum:
>> +      - silergy,sq52210
>> +      - ti,ina3221
>>   
>>     reg:
>>       maxItems: 1
>> @@ -77,6 +79,18 @@ patternProperties:
>>             exclude specific channels from the summation control function.
>>           type: boolean
>>   
>> +      alert-type:
>> +        description: |
>> +          The SQ52210 features a configurable alert function with four
>> +          types: SUL, BOL, BUL, and POL. Each channel can be configured to
>> +          select one of these types to enable the alert function. This alert
>> +          function can operate concurrently with both Critical and Warning
>> +          functions.
>> +
>> +          The configuration must use numerical values 0 through 3,
>> +          0 corresponds to SUL, 1 to BOL, 2 to BUL, and 3 to POL.
>> +        enum: [ 0, 1, 2, 3 ]
>> +
>
>Per datasheet, each of the alerts can be enabled independently. It is possible
>to enable SUL, BOL, BUL, and POL on each channel at the same time. This is not
>possible with the above property since it only permits enabling alerts for one
>of the alert sources on each channel.
>
>Also, I am not sure if it makes sense to have this as devicetree property.
>It is not really a board property. It might make more sense to tie enabling
>the alerts automatically if a channel is enabled and a limit is set for a
>given channel.
>

The "If multiple function are enabled, the Alert Function with the highrst
signifivant bit position(D15-D4) takes priority and responds to the Alert
LImit Register" described on page 21 of the datasheet refers to the fact that
when different trigger sources are enabled simultaneously, only the highest
priority trigger source takes effect (SUL > BOL > BUL > POL).Therefore,
essentially only one type of alert can be active per channel.

Indeed, it is unnecessary to configure the alert-type at the board level.
I will modify this content and conduct testing before the next submission,
and also remove the alert-type support in Patch 3.

Thanks,
Wenlaing Yan

Re: [PATCH 1/8] dt-binding:ti,ina3221:Add SQ52210

[Wenliang Yan]

At 2025-11-14 01:16:07, "Guenter Roeck" <linux@roeck-us.net> wrote:
>On Tue, Nov 11, 2025 at 03:05:44AM -0500, Wenliang Yan wrote:
>>  enum ina3221_channels {
>> @@ -293,11 +303,39 @@ static int ina3221_read_value(struct ina3221_data *ina, unsigned int reg,
>>  	 * Shunt Voltage Sum register has 14-bit value with 1-bit shift
>>  	 * Other Shunt Voltage registers have 12 bits with 3-bit shift
>>  	 */
>> -	if (reg == INA3221_SHUNT_SUM || reg == INA3221_CRIT_SUM)
>> +	switch (reg) {
>> +	case INA3221_SHUNT_SUM:
>> +	case INA3221_CRIT_SUM:
>>  		*val = sign_extend32(regval >> 1, 14);
>> -	else
>> +		break;
>> +	case SQ52210_CURRENT1:
>> +	case SQ52210_CURRENT2:
>> +	case SQ52210_CURRENT3:
>> +	case SQ52210_POWER1:
>> +	case SQ52210_POWER2:
>> +	case SQ52210_POWER3:
>> +		*val = regval;
>> +		break;
>> +	case INA3221_BUS1:
>> +	case INA3221_BUS2:
>> +	case INA3221_BUS3:
>> +	case INA3221_SHUNT1:
>> +	case INA3221_SHUNT2:
>> +	case INA3221_SHUNT3:
>> +	case INA3221_WARN1:
>> +	case INA3221_WARN2:
>> +	case INA3221_WARN3:
>> +	case INA3221_CRIT1:
>> +	case INA3221_CRIT2:
>> +	case INA3221_CRIT3:
>>  		*val = sign_extend32(regval >> 3, 12);
>> -
>> +		break;
>> +	case SQ52210_ALERT_LIMIT1:
>> +	case SQ52210_ALERT_LIMIT2:
>> +	case SQ52210_ALERT_LIMIT3:
>> +		*val = regval >> 3;
>> +		break;
>> +	};
>
>This returns 0 if the register is not listed in the case statement while leaving
>val unset. It would probably be better to drop this function entirely and handle the
>conversions in the calling code.
>

This call is used quite frequently, and handling it directly in the calling code
would result in significant code redundancy. I believe adding default: *val = 0;
return -EOPNOTSUPP; should address your concerns.

>>  	return 0;
>>  }
>>  

>> +/*
>> + * Turns alert limit values into register values.
>> + * Opposite of the formula in ina3221_read_value().
>> + */
>> +static u16 sq52210_alert_to_reg(struct ina3221_data *ina, int reg, long val)
>> +{
>> +	int regval;
>> +	/*
>> +	 * Formula to convert voltage_uv to register value:
>> +	 *     regval = (voltage_mv / scale) << shift
>> +	 * Results:
>> +	 *     bus_voltage: (1 / 8mV) << 3 = 1 mV
>> +	 */
>> +	switch (reg) {
>> +	case INA3221_BUS1:
>> +	case INA3221_BUS2:
>> +	case INA3221_BUS3:
>> +		/* clamp voltage */
>> +		regval = clamp_val(val, -32760, 32760);
>> +		return regval;
>> +	case SQ52210_CURRENT1:
>> +	case SQ52210_CURRENT2:
>> +	case SQ52210_CURRENT3:
>> +		/* signed register, result in mA */
>> +		regval = DIV_ROUND_CLOSEST(val * 8000, ina->current_lsb_uA);
>> +		return clamp_val(regval, -32760, 32760);
>> +	case SQ52210_POWER1:
>> +	case SQ52210_POWER2:
>> +	case SQ52210_POWER3:
>> +		regval = DIV_ROUND_CLOSEST(val * 8000, ina->power_lsb_uW);
>> +		return clamp_val(regval, 0, 65528);
>> +	default:
>> +		/* programmer goofed */
>> +		WARN_ON_ONCE(1);
>> +		return 0;
>
>Same as above. I know other code uses the samew approach, but this kind of
>"validation" would be better to avoid. It would be much better to handle the
>conversions in the calling code.
>

As per your recommendation, I will remove this function and complete the
conversion directly within the sq52210_alert_limit_write function.

>> +	}
>> +}
>> +
>>  static int ina3221_read_chip(struct device *dev, u32 attr, long *val)
>>  {
>>  	struct ina3221_data *ina = dev_get_drvdata(dev);
>> @@ -373,6 +461,25 @@ static int ina3221_read_in(struct device *dev, u32 attr, int channel, long *val)
>>  	case hwmon_in_enable:
>>  		*val = ina3221_is_enabled(ina, channel);
>>  		return 0;
>> +	case hwmon_in_crit:
>> +	case hwmon_in_lcrit:
>> +		reg = alert_limit_reg[channel];
>> +		ret = ina3221_read_value(ina, reg, &regval);
>> +		if (ret)
>> +			return ret;
>> +		/*
>> +		 * Scale of bus voltage (mV): LSB is 8mV
>> +		 */
>> +		*val = regval * 8;
>> +		return 0;
>> +	case hwmon_in_crit_alarm:
>> +	case hwmon_in_lcrit_alarm:
>> +		reg = alert_flag[channel];
>> +		ret = regmap_field_read(ina->fields[reg], &regval);
>> +		if (ret)
>> +			return ret;
>> +		*val = regval;
>> +		return 0;
>>  	default:
>>  		return -EOPNOTSUPP;
>>  	}
>> @@ -450,6 +557,58 @@ static int ina3221_read_curr(struct device *dev, u32 attr,
>>  	}
>>  }
>>  
>> +static const u32 sq52210_alert_mask[][INA3221_NUM_CHANNELS] = {
>> +	[hwmon_curr_lcrit] = { BIT(15), BIT(14), BIT(13) },
>> +	[hwmon_in_crit] = { BIT(12), BIT(11), BIT(10) },
>> +	[hwmon_in_lcrit] = { BIT(9), BIT(8), BIT(7) },
>> +	[hwmon_power_crit] = { BIT(6), BIT(5), BIT(4) },
>
>This does not work. hwmon_curr_xxx, hwmon_inxxx, and hwmon_power_xxx use
>different and overlapping number spaces. Even if that was not the case,
>the above would result in a serverely sparse array, and the callingo code
>would have to make sure that the row is actually populated before using it.
>

Using an array here isn't quite appropriate. I'll use a switch statement in
the next version to determine different alert flags to resolve this issue.

>> +};
>> +
>> +static int sq52210_alert_limit_write(struct ina3221_data *ina, u32 attr, int channel, long val)
>> +{
>> +	struct regmap *regmap = ina->regmap;
>> +	int ret, limit_reg, item;
>> +	u32 alert_group;
>> +
>> +	if (val < 0)
>> +		return -EINVAL;
>> +	item = channel % INA3221_NUM_CHANNELS;
>> +	switch (item) {
>> +	case 0:
>> +		alert_group = SQ52210_MASK_ALERT_CHANNEL1;
>> +		limit_reg = SQ52210_ALERT_LIMIT1;
>> +		break;
>> +	case 1:
>> +		alert_group = SQ52210_MASK_ALERT_CHANNEL2;
>> +		limit_reg = SQ52210_ALERT_LIMIT2;
>> +		break;
>> +	case 2:
>> +		alert_group = SQ52210_MASK_ALERT_CHANNEL3;
>> +		limit_reg = SQ52210_ALERT_LIMIT3;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +	/*
>> +	 * Clear all alerts first to avoid accidentally triggering ALERT pin
>> +	 * due to register write sequence. Then, only enable the alert
>> +	 * if the value is non-zero.
>> +	 */
>> +	ret = regmap_update_bits(regmap, SQ52210_ALERT_CONFIG,
>> +				alert_group, 0);
>> +	if (ret < 0)
>> +		return ret;
>> +	ret = regmap_write(regmap, limit_reg,
>> +			sq52210_alert_to_reg(ina, ina3221_curr_reg[attr][item], val));
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (val)
>> +		return regmap_update_bits(regmap, SQ52210_ALERT_CONFIG,
>> +					alert_group, sq52210_alert_mask[attr][item]);
>> +	return 0;
>> +}
>> +
 


Thanks,
Wenlaing Yan

Re: [PATCH 1/8] dt-binding:ti,ina3221:Add SQ52210

[Wenliang Yan]

At 2025-11-14 01:18:30, "Guenter Roeck" <linux@roeck-us.net> wrote:
>On Tue, Nov 11, 2025 at 03:05:45AM -0500, Wenliang Yan wrote:
>> SQ52210 adds power attribute to report power data, and implements
>> read/write functions for this purpose.
>> 
>> Signed-off-by: Wenliang Yan <wenliang202407@163.com>
>> ---
>>  drivers/hwmon/ina3221.c | 79 ++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 75 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/hwmon/ina3221.c b/drivers/hwmon/ina3221.c
>> index abb6049c8eab..ea01687ad1fa 100644
>> --- a/drivers/hwmon/ina3221.c
>> +++ b/drivers/hwmon/ina3221.c
>> @@ -348,19 +348,16 @@ static const u8 ina3221_in_reg[] = {
>>  	INA3221_SHUNT3,
>>  	INA3221_SHUNT_SUM,
>>  };
>> -
>>  static const u8 alert_limit_reg[] = {
>>  	SQ52210_ALERT_LIMIT1,
>>  	SQ52210_ALERT_LIMIT2,
>>  	SQ52210_ALERT_LIMIT3,
>>  };
>> -
>>  static const u8 alert_flag[] = {
>>  	F_AFF1,
>>  	F_AFF2,
>>  	F_AFF3,
>>  };
>> -
>
>Please refrain from making such cosmetic changes.
>

Understood, I'll make the changes.

Thanks,
Wenlaing Yan