[PATCH 0/5] hwmon: Add TSC1641 I2C power monitor driver

[PATCH 0/5] hwmon: Add TSC1641 I2C power monitor driver

[Igor Reznichenko]

This patch series adds support for the ST Microelectronics TSC1641
I2C power monitor. The TSC1641 provides bus voltage, current, power, 
and temperature measurements via the hwmon subsystem. The driver 
supports optional ALERT pin polarity configuration and exposes the
shunt resistor value and raw shunt voltage via sysfs.

Tested on Raspberry Pi 3B+ with a TSC1641 evaluation board.

Igor Reznichenko (5):
  drivers/hwmon: Add TSC1641 I2C power monitor driver
  drivers/hwmon: Add Kconfig entry for TSC1641
  drivers/hwmon: Add TSC1641 module to Makefile
  Documentation/hwmon: Add TSC1641 driver documentation
  Documentation/devicetree/bindings/hwmon: Add TSC1641 binding

 .../devicetree/bindings/hwmon/st,tsc1641.yaml |  54 ++
 Documentation/hwmon/index.rst                 |   1 +
 Documentation/hwmon/tsc1641.rst               |  73 ++
 drivers/hwmon/Kconfig                         |  12 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/tsc1641.c                       | 801 ++++++++++++++++++
 6 files changed, 942 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/st,tsc1641.yaml
 create mode 100644 Documentation/hwmon/tsc1641.rst
 create mode 100644 drivers/hwmon/tsc1641.c

-- 
2.43.0

[PATCH 1/5] drivers/hwmon: Add TSC1641 I2C power monitor driver

[Igor Reznichenko]

Add a new I2C hwmon driver for the ST Microelectronics TSC1641 16-bit
high-precision power monitor. The driver supports reading bus voltage,
current, power, and temperature. Sysfs attributes are exposed for
shunt resistor value, raw shunt voltage and update interval.
The driver integrates with the hwmon subsystem and supports optional
ALERT pin polarity configuration.

Signed-off-by: Igor Reznichenko <igor@reznichenko.net>
---
 drivers/hwmon/tsc1641.c | 801 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 801 insertions(+)
 create mode 100644 drivers/hwmon/tsc1641.c

diff --git a/drivers/hwmon/tsc1641.c b/drivers/hwmon/tsc1641.c
new file mode 100644
index 000000000000..22b49a7918cf
--- /dev/null
+++ b/drivers/hwmon/tsc1641.c
@@ -0,0 +1,801 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for ST Microelectronics TSC1641 I2C power monitor
+ *
+ * 60 V, 16-bit high-precision power monitor with I2C and MIPI I3C interface
+ * Datasheet: https://www.st.com/resource/en/datasheet/tsc1641.pdf
+ *
+ * Copyright (C) 2025 Igor Reznichenko <igor@reznichenko.net>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/sysfs.h>
+
+/* I2C registers */
+#define TSC1641_CONFIG		0x00
+#define TSC1641_SHUNT_VOLTAGE	0x01
+#define TSC1641_LOAD_VOLTAGE	0x02
+#define TSC1641_POWER		0x03
+#define TSC1641_CURRENT		0x04
+#define TSC1641_TEMP		0x05
+#define TSC1641_MASK		0x06
+#define TSC1641_FLAG		0x07
+#define TSC1641_RSHUNT		0x08 /* Shunt resistance */
+#define TSC1641_SOL		0x09
+#define TSC1641_SUL		0x0A
+#define TSC1641_LOL		0x0B
+#define TSC1641_LUL		0x0C
+#define TSC1641_POL		0x0D
+#define TSC1641_TOL		0x0E
+#define TSC1641_MANUF_ID	0xFE /* 0x0006 */
+#define TSC1641_DIE_ID		0xFF /* 0x1000 */
+#define TSC1641_MAX_REG		0xFF
+
+#define TSC1641_RSHUNT_DEFAULT	0x0000
+#define TSC1641_CONFIG_DEFAULT	0x003F /* Enable temperature sensor */
+
+/* Bit mask for conversion time in the configuration register */
+#define TSC1641_CONV_TIME_MASK	GENMASK(7, 4)
+
+#define TSC1641_CONV_TIME_DEFAULT	1024
+#define TSC1641_MIN_UPDATE_INTERVAL	1024
+
+/* LSB value of different registers */
+#define TSC1641_VLOAD_LSB_MILLIVOLT	2
+#define TSC1641_POWER_LSB_MICROWATT	25000
+#define TSC1641_VSHUNT_LSB_NANOVOLT	2500	/* Use nanovolts to make it integer */
+#define TSC1641_RSHUNT_LSB_UOHM		10
+#define TSC1641_TEMP_LSB_MILLIDEGC	500
+
+/* Bit masks for enabling limit alerts in TSC1641_MASK*/
+#define TSC1641_SHUNT_OV_MASK		BIT(15)
+#define TSC1641_SHUNT_UV_MASK		BIT(14)
+#define TSC1641_LOAD_OV_MASK		BIT(13)
+#define TSC1641_LOAD_UV_MASK		BIT(12)
+#define TSC1641_POWER_OVER_MASK		BIT(11)
+#define TSC1641_TEMP_OVER_MASK		BIT(10)
+#define TSC1641_ALERT_POL_MASK		BIT(1)
+#define TSC1641_ALERT_LATCH_EN_MASK	BIT(0)
+
+/* Flags indicating alerts in TSC1641_FLAG register*/
+#define TSC1641_SHUNT_OV_FLAG		BIT(6)
+#define TSC1641_SHUNT_UV_FLAG		BIT(5)
+#define TSC1641_LOAD_OV_FLAG		BIT(4)
+#define TSC1641_LOAD_UV_FLAG		BIT(3)
+#define TSC1641_POWER_OVER_FLAG		BIT(2)
+#define TSC1641_TEMP_OVER_FLAG		BIT(1)
+
+static bool tsc1641_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case TSC1641_CONFIG:
+	case TSC1641_MASK:
+	case TSC1641_RSHUNT:
+	case TSC1641_SOL:
+	case TSC1641_SUL:
+	case TSC1641_LOL:
+	case TSC1641_LUL:
+	case TSC1641_POL:
+	case TSC1641_TOL:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool tsc1641_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case TSC1641_SHUNT_VOLTAGE:
+	case TSC1641_LOAD_VOLTAGE:
+	case TSC1641_POWER:
+	case TSC1641_CURRENT:
+	case TSC1641_TEMP:
+	case TSC1641_FLAG:
+	case TSC1641_MANUF_ID:
+	case TSC1641_DIE_ID:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config tsc1641_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.use_single_write = true,
+	.use_single_read = true,
+	.max_register = TSC1641_MAX_REG,
+	.cache_type = REGCACHE_MAPLE,
+	.volatile_reg = tsc1641_volatile_reg,
+	.writeable_reg = tsc1641_writeable_reg,
+};
+
+struct tsc1641_data {
+	long rshunt_uohm;
+	long current_lsb_uA;
+	/* protects register data during updates */
+	struct mutex update_lock;
+	struct regmap *regmap;
+	struct i2c_client *client;
+};
+
+static int tsc1641_set_shunt(struct tsc1641_data *data, u32 val)
+{
+	struct regmap *regmap = data->regmap;
+
+	if (!val)
+		return 0;
+
+	data->rshunt_uohm = val;
+	long mohm = DIV_ROUND_CLOSEST(data->rshunt_uohm, 1000);
+
+	data->current_lsb_uA = DIV_ROUND_CLOSEST(TSC1641_VSHUNT_LSB_NANOVOLT, mohm);
+	/* RSHUNT register LSB is 10uOhm so need to divide further*/
+	long rshunt_reg = DIV_ROUND_CLOSEST(data->rshunt_uohm, TSC1641_RSHUNT_LSB_UOHM);
+	int ret = regmap_write(regmap, TSC1641_RSHUNT, rshunt_reg);
+
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+/*
+ * Conversion times in uS, value in CONFIG[CT3:CT0] corresponds to index in this array
+ * See "Table 14. CT3 to CT0: conversion time" in:
+ * https://www.st.com/resource/en/datasheet/tsc1641.pdf
+ */
+static const int tsc1641_conv_times[] = { 128, 256, 512, 1024, 2048, 4096, 8192, 16384, 32768 };
+
+static int tsc1641_reg_to_upd_interval(u16 config)
+{
+	int idx = FIELD_GET(TSC1641_CONV_TIME_MASK, config);
+
+	idx = clamp_val(idx, 0, ARRAY_SIZE(tsc1641_conv_times) - 1);
+	int conv_time = tsc1641_conv_times[idx];
+
+	/* Don't support sub-millisecond update interval as it's not supported in hwmon */
+	conv_time = max(conv_time, TSC1641_MIN_UPDATE_INTERVAL);
+	/* Return nearest value in milliseconds */
+	return DIV_ROUND_CLOSEST(conv_time, 1000);
+}
+
+static u16 tsc1641_upd_interval_to_reg(long interval)
+{
+	/* Supported interval is 1ms - 33ms */
+	interval = clamp_val(interval, 1, 33);
+
+	int conv = interval * 1000;
+	int conv_bits = find_closest(conv, tsc1641_conv_times,
+				 ARRAY_SIZE(tsc1641_conv_times));
+
+	return FIELD_PREP(TSC1641_CONV_TIME_MASK, conv_bits);
+}
+
+static int tsc1641_chip_write(struct device *dev, u32 attr, long val)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_chip_update_interval:
+		return regmap_update_bits(data->regmap, TSC1641_CONFIG,
+					  TSC1641_CONV_TIME_MASK,
+					  tsc1641_upd_interval_to_reg(val));
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int tsc1641_chip_read(struct device *dev, u32 attr, long *val)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+	u32 regval;
+	int ret;
+
+	switch (attr) {
+	case hwmon_chip_update_interval:
+		ret = regmap_read(data->regmap, TSC1641_CONFIG, &regval);
+		if (ret)
+			return ret;
+
+		*val = tsc1641_reg_to_upd_interval(regval);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int tsc1641_reg_to_value(struct tsc1641_data *data, u8 reg, unsigned int regval)
+{
+	int val;
+
+	switch (reg) {
+	case TSC1641_SHUNT_VOLTAGE:
+		val = (s16)regval * TSC1641_VSHUNT_LSB_NANOVOLT;
+		/* Return microvolts */
+		return DIV_ROUND_CLOSEST(val, 1000);
+	case TSC1641_SOL:
+		fallthrough;
+	case TSC1641_SUL:
+		/* Used for current limits only, so return current in mA */
+		val = (s16)regval * data->current_lsb_uA;
+		return DIV_ROUND_CLOSEST(val, 1000);
+	case TSC1641_LOL:
+		fallthrough;
+	case TSC1641_LUL:
+		fallthrough;
+	case TSC1641_LOAD_VOLTAGE:
+		return (regval * TSC1641_VLOAD_LSB_MILLIVOLT);
+	case TSC1641_POWER:
+		fallthrough;
+	case TSC1641_POL:
+		return (regval * TSC1641_POWER_LSB_MICROWATT);
+	case TSC1641_CURRENT:
+		val = regval * data->current_lsb_uA;
+		/* Current in milliamps */
+		return DIV_ROUND_CLOSEST(val, 1000);
+	case TSC1641_TEMP:
+		fallthrough;
+	case TSC1641_TOL:
+		return (regval * TSC1641_TEMP_LSB_MILLIDEGC);
+	default:
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+}
+
+static int tsc1641_value_to_reg(struct tsc1641_data *data, u8 reg, unsigned int val)
+{
+	int regval;
+
+	switch (reg) {
+	case TSC1641_SOL:
+		fallthrough;
+	case TSC1641_SUL:
+		/* value is in milliamps, so convert to voltage first */
+		regval = (s16)val * data->rshunt_uohm;
+		regval = DIV_ROUND_CLOSEST(regval, TSC1641_VSHUNT_LSB_NANOVOLT);
+		return clamp_val(regval, SHRT_MIN, SHRT_MAX);
+	case TSC1641_LOL:
+		fallthrough;
+	case TSC1641_LUL:
+		regval = DIV_ROUND_CLOSEST(val, TSC1641_VLOAD_LSB_MILLIVOLT);
+		return clamp_val(regval, 0, USHRT_MAX);
+	case TSC1641_POL:
+		regval = DIV_ROUND_CLOSEST(val, TSC1641_POWER_LSB_MICROWATT);
+		return clamp_val(regval, 0, USHRT_MAX);
+	case TSC1641_TOL:
+		regval = DIV_ROUND_CLOSEST(val, TSC1641_TEMP_LSB_MILLIDEGC);
+		return clamp_val(regval, 0, USHRT_MAX);
+	default:
+		/* shouldn't be here */
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+}
+
+static int tsc1641_alert_limit_read(struct tsc1641_data *data, u32 mask, int reg, long *val)
+{
+	struct regmap *regmap = data->regmap;
+	int regval;
+	int ret;
+
+	mutex_lock(&data->update_lock);
+	ret = regmap_read(regmap, TSC1641_MASK, &regval);
+	if (ret)
+		goto abort;
+
+	if (regval & mask) {
+		ret = regmap_read(regmap, reg, &regval);
+		if (ret)
+			goto abort;
+		*val = tsc1641_reg_to_value(data, reg, regval);
+	} else {
+		*val = 0;
+	}
+abort:
+	mutex_unlock(&data->update_lock);
+	return ret;
+}
+
+static int tsc1641_alert_limit_write(struct tsc1641_data *data, u32 mask, int limit_reg,
+				     long val)
+{
+	struct regmap *regmap = data->regmap;
+	int ret;
+
+	if (val < 0)
+		return -EINVAL;
+
+	/*
+	 * Disable alert mask first, then write the value and enable alert mask
+	 */
+	mutex_lock(&data->update_lock);
+	ret = regmap_update_bits(regmap, TSC1641_MASK, mask, 0);
+	if (ret < 0)
+		goto abort;
+	ret = regmap_write(regmap, limit_reg, tsc1641_value_to_reg(data, limit_reg, val));
+	if (ret < 0)
+		goto abort;
+
+	if (val)
+		ret = regmap_update_bits(regmap, TSC1641_MASK, mask, mask);
+abort:
+	mutex_unlock(&data->update_lock);
+	return ret;
+}
+
+static int tsc1641_alert_read(struct regmap *regmap, u32 flag, long *val)
+{
+	unsigned int regval;
+	int ret;
+
+	ret = regmap_read_bypassed(regmap, TSC1641_FLAG, &regval);
+	if (ret)
+		return ret;
+
+	*val = !!(regval & flag);
+	return 0;
+}
+
+static int tsc1641_in_read(struct device *dev, u32 attr, long *val)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	unsigned int regval;
+	int ret;
+
+	switch (attr) {
+	case hwmon_in_input:
+		ret = regmap_read(regmap, TSC1641_LOAD_VOLTAGE, &regval);
+		if (ret)
+			return ret;
+		*val = tsc1641_reg_to_value(data, TSC1641_LOAD_VOLTAGE, regval);
+		break;
+	case hwmon_in_lcrit:
+		return tsc1641_alert_limit_read(data, TSC1641_LOAD_UV_MASK, TSC1641_LUL, val);
+	case hwmon_in_crit:
+		return tsc1641_alert_limit_read(data, TSC1641_LOAD_OV_MASK, TSC1641_LOL, val);
+	case hwmon_in_lcrit_alarm:
+		return tsc1641_alert_read(regmap, TSC1641_LOAD_UV_FLAG, val);
+	case hwmon_in_crit_alarm:
+		return tsc1641_alert_read(regmap, TSC1641_LOAD_OV_FLAG, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int tsc1641_curr_read(struct device *dev, u32 attr, long *val)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	unsigned int regval;
+	int ret;
+
+	/* Current limits are the shunt under/over voltage limits */
+	switch (attr) {
+	case hwmon_curr_input:
+		ret = regmap_read(regmap, TSC1641_CURRENT, &regval);
+		if (ret)
+			return ret;
+		*val = tsc1641_reg_to_value(data, TSC1641_CURRENT, regval);
+		break;
+	case hwmon_curr_lcrit:
+		return tsc1641_alert_limit_read(data, TSC1641_SHUNT_UV_MASK,
+						TSC1641_SUL, val);
+	case hwmon_curr_crit:
+		return tsc1641_alert_limit_read(data, TSC1641_SHUNT_OV_MASK,
+						TSC1641_SOL, val);
+	case hwmon_curr_lcrit_alarm:
+		return tsc1641_alert_read(regmap, TSC1641_SHUNT_UV_FLAG, val);
+	case hwmon_curr_crit_alarm:
+		return tsc1641_alert_read(regmap, TSC1641_SHUNT_OV_FLAG, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int tsc1641_power_read(struct device *dev, u32 attr, long *val)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	unsigned int regval;
+	int ret;
+
+	switch (attr) {
+	case hwmon_power_input:
+		ret = regmap_read(regmap, TSC1641_POWER, &regval);
+		if (ret)
+			return ret;
+		*val = tsc1641_reg_to_value(data, TSC1641_POWER, regval);
+		break;
+	case hwmon_power_crit:
+		return tsc1641_alert_limit_read(data, TSC1641_POWER_OVER_MASK,
+						TSC1641_POL, val);
+	case hwmon_power_crit_alarm:
+		return tsc1641_alert_read(regmap, TSC1641_POWER_OVER_FLAG, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int tsc1641_temp_read(struct device *dev, u32 attr, long *val)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	unsigned int regval;
+	int ret;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		ret = regmap_read(regmap, TSC1641_TEMP, &regval);
+		if (ret)
+			return ret;
+		*val = tsc1641_reg_to_value(data, TSC1641_TEMP, regval);
+		break;
+	case hwmon_temp_crit:
+		return tsc1641_alert_limit_read(data, TSC1641_TEMP_OVER_MASK,
+						TSC1641_TOL, val);
+	case hwmon_temp_crit_alarm:
+		return tsc1641_alert_read(regmap, TSC1641_TEMP_OVER_FLAG, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int tsc1641_in_write(struct device *dev, u32 attr, long val)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_in_lcrit:
+		return tsc1641_alert_limit_write(data, TSC1641_LOAD_UV_MASK, TSC1641_LUL, val);
+	case hwmon_in_crit:
+		return tsc1641_alert_limit_write(data, TSC1641_LOAD_OV_MASK, TSC1641_LOL, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int tsc1641_curr_write(struct device *dev, u32 attr, long val)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_curr_lcrit:
+		return tsc1641_alert_limit_write(data, TSC1641_SHUNT_UV_MASK,
+						TSC1641_SUL, val);
+	case hwmon_curr_crit:
+		return tsc1641_alert_limit_write(data, TSC1641_SHUNT_OV_MASK,
+						TSC1641_SOL, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int tsc1641_power_write(struct device *dev, u32 attr, long val)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_power_crit:
+		return tsc1641_alert_limit_write(data, TSC1641_POWER_OVER_MASK,
+						 TSC1641_POL, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static int tsc1641_temp_write(struct device *dev, u32 attr, long val)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_temp_crit:
+		return tsc1641_alert_limit_write(data, TSC1641_TEMP_OVER_MASK,
+						 TSC1641_TOL, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static umode_t tsc1641_is_visible(const void *data, enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_chip:
+		switch (attr) {
+		case hwmon_chip_update_interval:
+			return 0644;
+		default:
+			break;
+		}
+		break;
+	case hwmon_in:
+		switch (attr) {
+		case hwmon_in_input:
+			return 0444;
+		case hwmon_in_lcrit:
+		case hwmon_in_crit:
+			return 0644;
+		case hwmon_in_lcrit_alarm:
+		case hwmon_in_crit_alarm:
+			return 0444;
+		default:
+			break;
+		}
+		break;
+	case hwmon_curr:
+		switch (attr) {
+		case hwmon_curr_input:
+			return 0444;
+		case hwmon_curr_lcrit:
+		case hwmon_curr_crit:
+			return 0644;
+		case hwmon_curr_lcrit_alarm:
+		case hwmon_curr_crit_alarm:
+			return 0444;
+		default:
+			break;
+		}
+		break;
+	case hwmon_power:
+		switch (attr) {
+		case hwmon_power_input:
+			return 0444;
+		case hwmon_power_crit:
+			return 0644;
+		case hwmon_power_crit_alarm:
+			return 0444;
+		default:
+			break;
+		}
+		break;
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+			return 0444;
+		case hwmon_temp_crit:
+			return 0644;
+		case hwmon_temp_crit_alarm:
+			return 0444;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static int tsc1641_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	switch (type) {
+	case hwmon_chip:
+		return tsc1641_chip_read(dev, attr, val);
+	case hwmon_in:
+		return tsc1641_in_read(dev, attr, val);
+	case hwmon_curr:
+		return tsc1641_curr_read(dev, attr, val);
+	case hwmon_power:
+		return tsc1641_power_read(dev, attr, val);
+	case hwmon_temp:
+		return tsc1641_temp_read(dev, attr, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int tsc1641_write(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long val)
+{
+	switch (type) {
+	case hwmon_chip:
+		return tsc1641_chip_write(dev, attr, val);
+	case hwmon_in:
+		return tsc1641_in_write(dev, attr, val);
+	case hwmon_curr:
+		return tsc1641_curr_write(dev, attr, val);
+	case hwmon_power:
+		return tsc1641_power_write(dev, attr, val);
+	case hwmon_temp:
+		return tsc1641_temp_write(dev, attr, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static const struct hwmon_channel_info * const tsc1641_info[] = {
+	HWMON_CHANNEL_INFO(chip,
+			   HWMON_C_UPDATE_INTERVAL),
+	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_INPUT | HWMON_I_CRIT | HWMON_I_CRIT_ALARM |
+			   HWMON_I_LCRIT | HWMON_I_LCRIT_ALARM),
+	HWMON_CHANNEL_INFO(curr,
+			   HWMON_C_INPUT | HWMON_C_CRIT | HWMON_C_CRIT_ALARM |
+			   HWMON_C_LCRIT | HWMON_C_LCRIT_ALARM),
+	HWMON_CHANNEL_INFO(power,
+			   HWMON_P_INPUT | HWMON_P_CRIT | HWMON_P_CRIT_ALARM),
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_CRIT | HWMON_T_CRIT_ALARM),
+	NULL
+};
+
+static ssize_t shunt_resistor_show(struct device *dev,
+				   struct device_attribute *da, char *buf)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%li\n", data->rshunt_uohm);
+}
+
+static ssize_t shunt_voltage_uvolts_show(struct device *dev,
+					 struct device_attribute *da, char *buf)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	unsigned int regval;
+	int ret;
+
+	ret = regmap_read(regmap, TSC1641_SHUNT_VOLTAGE, &regval);
+	if (ret)
+		return ret;
+	int val = tsc1641_reg_to_value(data, TSC1641_SHUNT_VOLTAGE, regval);
+
+	return sysfs_emit(buf, "%d\n", val);
+}
+
+static ssize_t shunt_resistor_store(struct device *dev,
+				    struct device_attribute *da,
+				    const char *buf, size_t count)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul(buf, 10, &val);
+	if (ret < 0)
+		return ret;
+
+	mutex_lock(&data->update_lock);
+	ret = tsc1641_set_shunt(data, val);
+	mutex_unlock(&data->update_lock);
+	if (ret < 0)
+		return ret;
+	return count;
+}
+
+static const struct hwmon_ops tsc1641_hwmon_ops = {
+	.is_visible = tsc1641_is_visible,
+	.read = tsc1641_read,
+	.write = tsc1641_write,
+};
+
+static const struct hwmon_chip_info tsc1641_chip_info = {
+	.ops = &tsc1641_hwmon_ops,
+	.info = tsc1641_info,
+};
+
+static DEVICE_ATTR_RW(shunt_resistor);
+static DEVICE_ATTR_RO(shunt_voltage_uvolts);
+
+/* Rshunt and shunt voltage value is exposed via sysfs attributes */
+static struct attribute *tsc1641_attrs[] = {
+	&dev_attr_shunt_resistor.attr,
+	&dev_attr_shunt_voltage_uvolts.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(tsc1641);
+
+static int tsc1641_init(struct device *dev, struct tsc1641_data *data)
+{
+	struct regmap *regmap = data->regmap;
+	u32 shunt;
+	int ret;
+
+	if (device_property_read_u32(dev, "shunt-resistor", &shunt) < 0) {
+		shunt = TSC1641_RSHUNT_DEFAULT;
+		dev_info(dev, "using default shunt-resistor value =%u uOhm\n",
+			 shunt);
+	}
+
+	ret = tsc1641_set_shunt(data, shunt);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(regmap, TSC1641_CONFIG, TSC1641_CONFIG_DEFAULT);
+	if (ret < 0)
+		return ret;
+
+	bool active_high = device_property_read_bool(dev, "st,alert-polarity-active-high");
+
+	regmap_update_bits(regmap, TSC1641_MASK, TSC1641_ALERT_POL_MASK,
+			   FIELD_PREP(TSC1641_ALERT_POL_MASK, active_high));
+
+	return 0;
+}
+
+static int tsc1641_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct tsc1641_data *data;
+	struct device *hwmon_dev;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->client = client;
+	mutex_init(&data->update_lock);
+
+	data->regmap = devm_regmap_init_i2c(client, &tsc1641_regmap_config);
+	if (IS_ERR(data->regmap)) {
+		dev_err(dev, "failed to allocate register map\n");
+		return PTR_ERR(data->regmap);
+	}
+
+	ret = tsc1641_init(dev, data);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to configure device\n");
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
+							 data, &tsc1641_chip_info, tsc1641_groups);
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
+
+	dev_info(dev, "power monitor %s (Rshunt = %li uOhm)\n",
+		 client->name, data->rshunt_uohm);
+
+	return 0;
+}
+
+static const struct i2c_device_id tsc1641_id[] = {
+	{ "tsc1641", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, tsc1641_id);
+
+static const struct of_device_id __maybe_unused tsc1641_of_match[] = {
+	{ .compatible = "st,tsc1641" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, tsc1641_of_match);
+
+static struct i2c_driver tsc1641_driver = {
+	.driver = {
+		.name = "tsc1641",
+		.of_match_table = of_match_ptr(tsc1641_of_match),
+	},
+	.probe = tsc1641_probe,
+	.id_table = tsc1641_id,
+};
+
+module_i2c_driver(tsc1641_driver);
+
+MODULE_AUTHOR("Igor Reznichenko <igor@reznichenko.net>");
+MODULE_DESCRIPTION("tsc1641 driver");
+MODULE_LICENSE("GPL");
+
-- 
2.43.0

[PATCH 2/5] drivers/hwmon: Add Kconfig entry for TSC1641

[Igor Reznichenko]

Add a Kconfig entry for the TSC1641 driver under the HWMON_I2C menu.
The driver can be built as a module or built-in. Default is module.

Signed-off-by: Igor Reznichenko <igor@reznichenko.net>
---
 drivers/hwmon/Kconfig | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 2760feb9f83b..b9d7b02932a6 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2434,6 +2434,18 @@ config SENSORS_TMP513
 	  This driver can also be built as a module. If so, the module
 	  will be called tmp513.
 
+config SENSORS_TSC1641
+	tristate "ST Microelectronics TSC1641 Power Monitor"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  If you say yes here you get support for TSC1641 power  monitor chip.
+	  The TSC1641 driver is configured for the default configuration of
+	  the part except temperature is enabled by default.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called tsc1641.
+
 config SENSORS_VEXPRESS
 	tristate "Versatile Express"
 	depends on VEXPRESS_CONFIG
-- 
2.43.0

[PATCH 3/5] drivers/hwmon: Add TSC1641 module to Makefile

[Igor Reznichenko]

Add the TSC1641 driver to drivers/hwmon/Makefile so it can be
built as a module.

Signed-off-by: Igor Reznichenko <igor@reznichenko.net>
---
 drivers/hwmon/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 73b2abdcc6dd..a8de5bc69f2a 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -233,6 +233,7 @@ obj-$(CONFIG_SENSORS_TMP401)	+= tmp401.o
 obj-$(CONFIG_SENSORS_TMP421)	+= tmp421.o
 obj-$(CONFIG_SENSORS_TMP464)	+= tmp464.o
 obj-$(CONFIG_SENSORS_TMP513)	+= tmp513.o
+obj-$(CONFIG_SENSORS_TSC1641)	+= tsc1641.o
 obj-$(CONFIG_SENSORS_VEXPRESS)	+= vexpress-hwmon.o
 obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
 obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
-- 
2.43.0

[PATCH 4/5] Documentation/hwmon: Add TSC1641 driver documentation

[Igor Reznichenko]

Add hwmon documentation for the TSC1641 driver. This includes
description and the sysfs attributes.

Signed-off-by: Igor Reznichenko <igor@reznichenko.net>
---
 Documentation/hwmon/index.rst   |  1 +
 Documentation/hwmon/tsc1641.rst | 73 +++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)
 create mode 100644 Documentation/hwmon/tsc1641.rst

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 51a5bdf75b08..4fb9f91f83b3 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -253,6 +253,7 @@ Hardware Monitoring Kernel Drivers
    tps40422
    tps53679
    tps546d24
+   tsc1641
    twl4030-madc-hwmon
    ucd9000
    ucd9200
diff --git a/Documentation/hwmon/tsc1641.rst b/Documentation/hwmon/tsc1641.rst
new file mode 100644
index 000000000000..a93d1e72c70e
--- /dev/null
+++ b/Documentation/hwmon/tsc1641.rst
@@ -0,0 +1,73 @@
+Kernel driver tsc1641
+=====================
+
+Supported chips:
+
+  * ST TSC1641
+
+    Prefix: 'tsc1641'
+
+    Addresses scanned: -
+
+    Datasheet:
+	https://www.st.com/resource/en/datasheet/tsc1641.pdf
+
+Author:
+	- Igor Reznichenko <igor@reznichenko.net>
+
+
+Description
+-----------
+
+The TSC1641 is a high-precision current, voltage, power, and temperature
+monitoring analog front-end (AFE). It monitors current into a shunt resistor and load
+voltage up to 60 V in a synchronized way. Digital bus interface is I2C/SMbus.
+The TSC1641 allows the assertion of several alerts regarding the voltage, current,
+power and temperature.
+
+Sysfs entries
+-------------
+
+==================== =======================================================
+in0_input            bus voltage (mV)
+in0_crit             bus voltage crit alarm limit (mV)
+in0_crit_alarm       bus voltage crit alarm limit exceeded
+in0_lcrit            bus voltage low-crit alarm limit (mV)
+in0_lcrit_alarm      bus voltage low-crit alarm limit exceeded
+
+curr1_input          current measurement (mA)
+curr1_crit           current crit alarm limit (mA)
+curr1_crit_alarm     current crit alarm limit exceeded
+curr1_lcrit          current low-crit alarm limit (mA)
+curr1_lcrit_alarm    current low-crit alarm limit exceeded
+
+power1_input         power measurement (uW)
+power1_crit          power crit alarm limit (uW)
+power1_crit_alarm    power crit alarm limit exceeded
+
+shunt_resistor       shunt resistor value (uOhms)
+shunt_voltage_uvolts shunt voltage raw measurement (uV)
+
+temp1_input          temperature measurement (mdegC)
+temp1_crit           temperature crit alarm limit (mdegC)
+temp1_crit_alarm     temperature crit alarm limit exceeded
+
+update_interval      data conversion time (1 - 33ms), longer conversion time corresponds
+                     to higher effective resolution in bits
+==================== =======================================================
+
+General Remarks
+---------------
+
+The TSC1641 driver requires the value of the external shunt resistor to
+correctly compute current and power measurements. The resistor value, in
+micro-ohms, should be provided either through the device tree property
+"shunt-resistor" or via the writable sysfs attribute "shunt_resistor".
+Please refer to the Documentation/devicetree/bindings/hwmon/st,tsc1641.yaml
+for bindings if the device tree is used.
+
+If the shunt resistor value is not specified in the device tree, the driver
+initializes it to 0 uOhm by default. In this state, current and power
+measurements will read as zero and are considered invalid. To enable these
+measurements, users must configure the correct shunt resistor value at
+runtime by writing to the "shunt_resistor" sysfs attribute.
-- 
2.43.0

[PATCH 5/5] Documentation/devicetree/bindings/hwmon: Add TSC1641 binding

[Igor Reznichenko]

Add a devicetree binding for the TSC1641 I2C power monitor.

Signed-off-by: Igor Reznichenko <igor@reznichenko.net>
---
 .../devicetree/bindings/hwmon/st,tsc1641.yaml | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/st,tsc1641.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/st,tsc1641.yaml b/Documentation/devicetree/bindings/hwmon/st,tsc1641.yaml
new file mode 100644
index 000000000000..e79f6dab4a87
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/st,tsc1641.yaml
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/st,tsc1641.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ST Microelectronics TSC1641 I2C power monitor
+
+maintainers:
+  - Igor Reznichenko <igor@reznichenko.net>
+
+description: |
+  TSC1641 is a 60 V, 16-bit high-precision power monitor with I2C and MIPI I3C interface
+
+  Datasheets:
+    https://www.st.com/resource/en/datasheet/tsc1641.pdf
+
+properties:
+  compatible:
+    const: st,tsc1641
+
+  reg:
+    maxItems: 1
+
+  shunt-resistor:
+    description:
+      Shunt resistor value in micro-ohms.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  st,alert-polarity-active-high:
+    description: Default value is 0 which configures the normal polarity of the ALERT pin, being active low open-drain.
+      Setting this to 1 configures the polarity of the ALERT pin to be inverted and active high open-drain.
+      Specify this property to set the alert polarity to active-high.
+    $ref: /schemas/types.yaml#/definitions/flag
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        power-sensor@40 {
+            compatible = "st,tsc1641";
+            reg = <0x40>;
+            shunt-resistor = <5000>;
+            st,alert-polarity-active-high;
+        };
+    };
-- 
2.43.0

Re: [PATCH 5/5] Documentation/devicetree/bindings/hwmon: Add TSC1641 binding

[Rob Herring (Arm)]

On Tue, 21 Oct 2025 21:47:08 -0700, Igor Reznichenko wrote:
> Add a devicetree binding for the TSC1641 I2C power monitor.
> 
> Signed-off-by: Igor Reznichenko <igor@reznichenko.net>
> ---
>  .../devicetree/bindings/hwmon/st,tsc1641.yaml | 54 +++++++++++++++++++
>  1 file changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/st,tsc1641.yaml
> 

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

yamllint warnings/errors:
./Documentation/devicetree/bindings/hwmon/st,tsc1641.yaml:31:111: [warning] line too long (119 > 110 characters) (line-length)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20251022044708.314287-6-igor@reznichenko.net

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 5/5] Documentation/devicetree/bindings/hwmon: Add TSC1641 binding

[Krzysztof Kozlowski]

On 22/10/2025 06:47, Igor Reznichenko wrote:
> Add a devicetree binding for the TSC1641 I2C power monitor.

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

A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

Please organize the patch documenting compatible (DT bindings) before
their user.
See also:
https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bindings/submitting-patches.rst#L46


> 
> Signed-off-by: Igor Reznichenko <igor@reznichenko.net>
> ---
>  .../devicetree/bindings/hwmon/st,tsc1641.yaml | 54 +++++++++++++++++++
>  1 file changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/st,tsc1641.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/st,tsc1641.yaml b/Documentation/devicetree/bindings/hwmon/st,tsc1641.yaml
> new file mode 100644
> index 000000000000..e79f6dab4a87
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/st,tsc1641.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/st,tsc1641.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ST Microelectronics TSC1641 I2C power monitor
> +
> +maintainers:
> +  - Igor Reznichenko <igor@reznichenko.net>
> +
> +description: |
> +  TSC1641 is a 60 V, 16-bit high-precision power monitor with I2C and MIPI I3C interface

Please wrap code according to the preferred limit expressed in Kernel
coding style, so at 80.

> +
> +  Datasheets:
> +    https://www.st.com/resource/en/datasheet/tsc1641.pdf
> +
> +properties:
> +  compatible:
> +    const: st,tsc1641
> +
> +  reg:
> +    maxItems: 1
> +
> +  shunt-resistor:

Use existing property, git grep shunt.

And then test it...

Best regards,
Krzysztof

Re: [PATCH 2/5] drivers/hwmon: Add Kconfig entry for TSC1641

[Krzysztof Kozlowski]

On 22/10/2025 06:47, Igor Reznichenko wrote:
> Add a Kconfig entry for the TSC1641 driver under the HWMON_I2C menu.
> The driver can be built as a module or built-in. Default is module.
> 
> Signed-off-by: Igor Reznichenko <igor@reznichenko.net>
> ---
>  drivers/hwmon/Kconfig | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 2760feb9f83b..b9d7b02932a6 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2434,6 +2434,18 @@ config SENSORS_TMP513
>  	  This driver can also be built as a module. If so, the module
>  	  will be called tmp513.
>  
> +config SENSORS_TSC1641

This patch makes no sense on its own. Squash it.

> +	tristate "ST Microelectronics TSC1641 Power Moni


Best regards,
Krzysztof

Re: [PATCH 3/5] drivers/hwmon: Add TSC1641 module to Makefile

[Krzysztof Kozlowski]

On 22/10/2025 06:47, Igor Reznichenko wrote:
> Add the TSC1641 driver to drivers/hwmon/Makefile so it can be
> built as a module.
> 
This patch makes no sense on its own. Squash it.

Best regards,
Krzysztof

Re: [PATCH 0/5] hwmon: Add TSC1641 I2C power monitor driver

[Guenter Roeck]

On 10/21/25 21:47, Igor Reznichenko wrote:
> This patch series adds support for the ST Microelectronics TSC1641
> I2C power monitor. The TSC1641 provides bus voltage, current, power,
> and temperature measurements via the hwmon subsystem. The driver
> supports optional ALERT pin polarity configuration and exposes the
> shunt resistor value and raw shunt voltage via sysfs.
> 
> Tested on Raspberry Pi 3B+ with a TSC1641 evaluation board.
> 
> Igor Reznichenko (5):
>    drivers/hwmon: Add TSC1641 I2C power monitor driver
>    drivers/hwmon: Add Kconfig entry for TSC1641
>    drivers/hwmon: Add TSC1641 module to Makefile
>    Documentation/hwmon: Add TSC1641 driver documentation

Please squash all of the above into a single patch.

>    Documentation/devicetree/bindings/hwmon: Add TSC1641 binding

This patch should come first.

Thanks,
Guenter

> 
>   .../devicetree/bindings/hwmon/st,tsc1641.yaml |  54 ++
>   Documentation/hwmon/index.rst                 |   1 +
>   Documentation/hwmon/tsc1641.rst               |  73 ++
>   drivers/hwmon/Kconfig                         |  12 +
>   drivers/hwmon/Makefile                        |   1 +
>   drivers/hwmon/tsc1641.c                       | 801 ++++++++++++++++++
>   6 files changed, 942 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/hwmon/st,tsc1641.yaml
>   create mode 100644 Documentation/hwmon/tsc1641.rst
>   create mode 100644 drivers/hwmon/tsc1641.c
> 

Re: [PATCH 1/5] drivers/hwmon: Add TSC1641 I2C power monitor driver

[Guenter Roeck]

On Tue, Oct 21, 2025 at 09:47:04PM -0700, Igor Reznichenko wrote:
> Add a new I2C hwmon driver for the ST Microelectronics TSC1641 16-bit
> high-precision power monitor. The driver supports reading bus voltage,
> current, power, and temperature. Sysfs attributes are exposed for
> shunt resistor value, raw shunt voltage and update interval.
> The driver integrates with the hwmon subsystem and supports optional
> ALERT pin polarity configuration.
> 
> Signed-off-by: Igor Reznichenko <igor@reznichenko.net>

Please send a register dump.

> ---
>  drivers/hwmon/tsc1641.c | 801 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 801 insertions(+)
>  create mode 100644 drivers/hwmon/tsc1641.c
> 
> diff --git a/drivers/hwmon/tsc1641.c b/drivers/hwmon/tsc1641.c
> new file mode 100644
> index 000000000000..22b49a7918cf
> --- /dev/null
> +++ b/drivers/hwmon/tsc1641.c
> @@ -0,0 +1,801 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for ST Microelectronics TSC1641 I2C power monitor
> + *
> + * 60 V, 16-bit high-precision power monitor with I2C and MIPI I3C interface
> + * Datasheet: https://www.st.com/resource/en/datasheet/tsc1641.pdf
> + *
> + * Copyright (C) 2025 Igor Reznichenko <igor@reznichenko.net>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/sysfs.h>
> +
> +/* I2C registers */
> +#define TSC1641_CONFIG		0x00
> +#define TSC1641_SHUNT_VOLTAGE	0x01
> +#define TSC1641_LOAD_VOLTAGE	0x02
> +#define TSC1641_POWER		0x03
> +#define TSC1641_CURRENT		0x04
> +#define TSC1641_TEMP		0x05
> +#define TSC1641_MASK		0x06
> +#define TSC1641_FLAG		0x07
> +#define TSC1641_RSHUNT		0x08 /* Shunt resistance */
> +#define TSC1641_SOL		0x09
> +#define TSC1641_SUL		0x0A
> +#define TSC1641_LOL		0x0B
> +#define TSC1641_LUL		0x0C
> +#define TSC1641_POL		0x0D
> +#define TSC1641_TOL		0x0E
> +#define TSC1641_MANUF_ID	0xFE /* 0x0006 */
> +#define TSC1641_DIE_ID		0xFF /* 0x1000 */
> +#define TSC1641_MAX_REG		0xFF
> +
> +#define TSC1641_RSHUNT_DEFAULT	0x0000

This should be a reasonable value, such as 1mOhm, not 0.

> +#define TSC1641_CONFIG_DEFAULT	0x003F /* Enable temperature sensor */
> +
> +/* Bit mask for conversion time in the configuration register */
> +#define TSC1641_CONV_TIME_MASK	GENMASK(7, 4)
> +
> +#define TSC1641_CONV_TIME_DEFAULT	1024
> +#define TSC1641_MIN_UPDATE_INTERVAL	1024
> +
> +/* LSB value of different registers */
> +#define TSC1641_VLOAD_LSB_MILLIVOLT	2
> +#define TSC1641_POWER_LSB_MICROWATT	25000
> +#define TSC1641_VSHUNT_LSB_NANOVOLT	2500	/* Use nanovolts to make it integer */
> +#define TSC1641_RSHUNT_LSB_UOHM		10
> +#define TSC1641_TEMP_LSB_MILLIDEGC	500
> +
> +/* Bit masks for enabling limit alerts in TSC1641_MASK*/
> +#define TSC1641_SHUNT_OV_MASK		BIT(15)
> +#define TSC1641_SHUNT_UV_MASK		BIT(14)
> +#define TSC1641_LOAD_OV_MASK		BIT(13)
> +#define TSC1641_LOAD_UV_MASK		BIT(12)
> +#define TSC1641_POWER_OVER_MASK		BIT(11)
> +#define TSC1641_TEMP_OVER_MASK		BIT(10)
> +#define TSC1641_ALERT_POL_MASK		BIT(1)
> +#define TSC1641_ALERT_LATCH_EN_MASK	BIT(0)
> +
> +/* Flags indicating alerts in TSC1641_FLAG register*/
> +#define TSC1641_SHUNT_OV_FLAG		BIT(6)
> +#define TSC1641_SHUNT_UV_FLAG		BIT(5)
> +#define TSC1641_LOAD_OV_FLAG		BIT(4)
> +#define TSC1641_LOAD_UV_FLAG		BIT(3)
> +#define TSC1641_POWER_OVER_FLAG		BIT(2)
> +#define TSC1641_TEMP_OVER_FLAG		BIT(1)
> +
> +static bool tsc1641_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case TSC1641_CONFIG:
> +	case TSC1641_MASK:
> +	case TSC1641_RSHUNT:
> +	case TSC1641_SOL:
> +	case TSC1641_SUL:
> +	case TSC1641_LOL:
> +	case TSC1641_LUL:
> +	case TSC1641_POL:
> +	case TSC1641_TOL:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool tsc1641_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case TSC1641_SHUNT_VOLTAGE:
> +	case TSC1641_LOAD_VOLTAGE:
> +	case TSC1641_POWER:
> +	case TSC1641_CURRENT:
> +	case TSC1641_TEMP:
> +	case TSC1641_FLAG:
> +	case TSC1641_MANUF_ID:
> +	case TSC1641_DIE_ID:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct regmap_config tsc1641_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.use_single_write = true,
> +	.use_single_read = true,
> +	.max_register = TSC1641_MAX_REG,
> +	.cache_type = REGCACHE_MAPLE,
> +	.volatile_reg = tsc1641_volatile_reg,
> +	.writeable_reg = tsc1641_writeable_reg,
> +};
> +
> +struct tsc1641_data {
> +	long rshunt_uohm;
> +	long current_lsb_uA;

No CamelCase variables, please.

> +	/* protects register data during updates */
> +	struct mutex update_lock;

Locking is handled by the hwmon core and not needed here.

> +	struct regmap *regmap;
> +	struct i2c_client *client;
> +};
> +
> +static int tsc1641_set_shunt(struct tsc1641_data *data, u32 val)

The calling code 'val' variable is unsigned long, so this will cause overflows.

> +{
> +	struct regmap *regmap = data->regmap;
> +
> +	if (!val)
> +		return 0;

Either use claml() to limit the valid range, or return -EINVAL.

> +
> +	data->rshunt_uohm = val;
> +	long mohm = DIV_ROUND_CLOSEST(data->rshunt_uohm, 1000);

Please keep variable declarations at the beginning of functions.

if val < 500, mohm == 0, and

> +
> +	data->current_lsb_uA = DIV_ROUND_CLOSEST(TSC1641_VSHUNT_LSB_NANOVOLT, mohm);

there will be a nice divice-by-zero crash here. Also, shunt resistor values
of, for example, 1,500 uOhm will result in substantial measurement errors.
That seems unnecessary.

> +	/* RSHUNT register LSB is 10uOhm so need to divide further*/
> +	long rshunt_reg = DIV_ROUND_CLOSEST(data->rshunt_uohm, TSC1641_RSHUNT_LSB_UOHM);
> +	int ret = regmap_write(regmap, TSC1641_RSHUNT, rshunt_reg);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

regmap_write() returns a negative error code or 0, so all this complexity
is unnecessary. A simple
	return regmap_write(regmap, TSC1641_RSHUNT, rshunt_reg);
would do.

> +}
> +
> +/*
> + * Conversion times in uS, value in CONFIG[CT3:CT0] corresponds to index in this array
> + * See "Table 14. CT3 to CT0: conversion time" in:
> + * https://www.st.com/resource/en/datasheet/tsc1641.pdf
> + */
> +static const int tsc1641_conv_times[] = { 128, 256, 512, 1024, 2048, 4096, 8192, 16384, 32768 };
> +
> +static int tsc1641_reg_to_upd_interval(u16 config)
> +{
> +	int idx = FIELD_GET(TSC1641_CONV_TIME_MASK, config);
> +
> +	idx = clamp_val(idx, 0, ARRAY_SIZE(tsc1641_conv_times) - 1);
> +	int conv_time = tsc1641_conv_times[idx];
> +
> +	/* Don't support sub-millisecond update interval as it's not supported in hwmon */
> +	conv_time = max(conv_time, TSC1641_MIN_UPDATE_INTERVAL);
> +	/* Return nearest value in milliseconds */
> +	return DIV_ROUND_CLOSEST(conv_time, 1000);
> +}
> +
> +static u16 tsc1641_upd_interval_to_reg(long interval)
> +{
> +	/* Supported interval is 1ms - 33ms */
> +	interval = clamp_val(interval, 1, 33);
> +
> +	int conv = interval * 1000;

	interval = clamp_val(interval, 1, 33) * 1000;

would do just as well without extra variable.

> +	int conv_bits = find_closest(conv, tsc1641_conv_times,
> +				 ARRAY_SIZE(tsc1641_conv_times));
> +
> +	return FIELD_PREP(TSC1641_CONV_TIME_MASK, conv_bits);
> +}
> +
> +static int tsc1641_chip_write(struct device *dev, u32 attr, long val)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_chip_update_interval:
> +		return regmap_update_bits(data->regmap, TSC1641_CONFIG,
> +					  TSC1641_CONV_TIME_MASK,
> +					  tsc1641_upd_interval_to_reg(val));
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int tsc1641_chip_read(struct device *dev, u32 attr, long *val)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +	u32 regval;
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_chip_update_interval:
> +		ret = regmap_read(data->regmap, TSC1641_CONFIG, &regval);
> +		if (ret)
> +			return ret;
> +
> +		*val = tsc1641_reg_to_upd_interval(regval);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int tsc1641_reg_to_value(struct tsc1641_data *data, u8 reg, unsigned int regval)
> +{
> +	int val;
> +
> +	switch (reg) {
> +	case TSC1641_SHUNT_VOLTAGE:
> +		val = (s16)regval * TSC1641_VSHUNT_LSB_NANOVOLT;
> +		/* Return microvolts */
> +		return DIV_ROUND_CLOSEST(val, 1000);
> +	case TSC1641_SOL:
> +		fallthrough;

fallthrough is unnecessary here.

> +	case TSC1641_SUL:
> +		/* Used for current limits only, so return current in mA */
> +		val = (s16)regval * data->current_lsb_uA;
> +		return DIV_ROUND_CLOSEST(val, 1000);
> +	case TSC1641_LOL:
> +		fallthrough;

fallthrough is unnecessary here.

> +	case TSC1641_LUL:

fallthrough is unnecessary here.

> +		fallthrough;
> +	case TSC1641_LOAD_VOLTAGE:
> +		return (regval * TSC1641_VLOAD_LSB_MILLIVOLT);
> +	case TSC1641_POWER:
> +		fallthrough;

fallthrough is unnecessary here.

> +	case TSC1641_POL:
> +		return (regval * TSC1641_POWER_LSB_MICROWATT);
> +	case TSC1641_CURRENT:
> +		val = regval * data->current_lsb_uA;
> +		/* Current in milliamps */
> +		return DIV_ROUND_CLOSEST(val, 1000);
> +	case TSC1641_TEMP:
> +		fallthrough;

fallthrough is unnecessary here.

> +	case TSC1641_TOL:
> +		return (regval * TSC1641_TEMP_LSB_MILLIDEGC);
> +	default:
> +		WARN_ON_ONCE(1);

I would strongly suggest to drop tsc1641_reg_to_value() as well as
tsc1641_value_to_reg() and do the conversions where needed.

> +		return 0;
> +	}
> +}
> +
> +static int tsc1641_value_to_reg(struct tsc1641_data *data, u8 reg, unsigned int val)
> +{
> +	int regval;
> +
> +	switch (reg) {
> +	case TSC1641_SOL:
> +		fallthrough;

Not needed.

> +	case TSC1641_SUL:
> +		/* value is in milliamps, so convert to voltage first */
> +		regval = (s16)val * data->rshunt_uohm;
> +		regval = DIV_ROUND_CLOSEST(regval, TSC1641_VSHUNT_LSB_NANOVOLT);
> +		return clamp_val(regval, SHRT_MIN, SHRT_MAX);
> +	case TSC1641_LOL:
> +		fallthrough;

Not needed.

> +	case TSC1641_LUL:
> +		regval = DIV_ROUND_CLOSEST(val, TSC1641_VLOAD_LSB_MILLIVOLT);
> +		return clamp_val(regval, 0, USHRT_MAX);
> +	case TSC1641_POL:
> +		regval = DIV_ROUND_CLOSEST(val, TSC1641_POWER_LSB_MICROWATT);
> +		return clamp_val(regval, 0, USHRT_MAX);
> +	case TSC1641_TOL:
> +		regval = DIV_ROUND_CLOSEST(val, TSC1641_TEMP_LSB_MILLIDEGC);
> +		return clamp_val(regval, 0, USHRT_MAX);
> +	default:
> +		/* shouldn't be here */
> +		WARN_ON_ONCE(1);
> +		return 0;
> +	}
> +}
> +
> +static int tsc1641_alert_limit_read(struct tsc1641_data *data, u32 mask, int reg, long *val)
> +{
> +	struct regmap *regmap = data->regmap;
> +	int regval;
> +	int ret;
> +
> +	mutex_lock(&data->update_lock);
> +	ret = regmap_read(regmap, TSC1641_MASK, &regval);
> +	if (ret)
> +		goto abort;
> +
> +	if (regval & mask) {
> +		ret = regmap_read(regmap, reg, &regval);
> +		if (ret)
> +			goto abort;
> +		*val = tsc1641_reg_to_value(data, reg, regval);
> +	} else {
> +		*val = 0;

If limits are masked, and the situation is static, the attributes
should not be created in the first place. Returning 0 if a limit is
masked is wrong.

Actually, looking into the datasheets, the mask register only enables
the ALERT mask and reporting if limits are exceeeded. Setting and reading
the limits is always supported.

> +	}
> +abort:
> +	mutex_unlock(&data->update_lock);
> +	return ret;
> +}
> +
> +static int tsc1641_alert_limit_write(struct tsc1641_data *data, u32 mask, int limit_reg,
> +				     long val)
> +{
> +	struct regmap *regmap = data->regmap;
> +	int ret;
> +
> +	if (val < 0)
> +		return -EINVAL;

The alert limit range should be clamped.

> +
> +	/*
> +	 * Disable alert mask first, then write the value and enable alert mask

Why ? 

> +	 */
> +	mutex_lock(&data->update_lock);
> +	ret = regmap_update_bits(regmap, TSC1641_MASK, mask, 0);
> +	if (ret < 0)
> +		goto abort;
> +	ret = regmap_write(regmap, limit_reg, tsc1641_value_to_reg(data, limit_reg, val));
> +	if (ret < 0)
> +		goto abort;
> +
> +	if (val)
> +		ret = regmap_update_bits(regmap, TSC1641_MASK, mask, mask);

Disabling alerts if the limit is 0 is wrong: The limit can be set
to 0 on purpose. Only unmasking the limit if a limit is set is just as wrong.
Either limits are enabled and reported, or they are disabled and the attributes
must not be generated. Mis-using the ABI to declare "If the limit value is
0, mask the limit. Otherwise set the limit and unmask it" is unacceptable.

> +abort:
> +	mutex_unlock(&data->update_lock);
> +	return ret;
> +}
> +
> +static int tsc1641_alert_read(struct regmap *regmap, u32 flag, long *val)
> +{
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read_bypassed(regmap, TSC1641_FLAG, &regval);
> +	if (ret)
> +		return ret;
> +
> +	*val = !!(regval & flag);
> +	return 0;
> +}
> +
> +static int tsc1641_in_read(struct device *dev, u32 attr, long *val)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +	unsigned int regval;
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_in_input:
> +		ret = regmap_read(regmap, TSC1641_LOAD_VOLTAGE, &regval);
> +		if (ret)
> +			return ret;
> +		*val = tsc1641_reg_to_value(data, TSC1641_LOAD_VOLTAGE, regval);
> +		break;
> +	case hwmon_in_lcrit:
> +		return tsc1641_alert_limit_read(data, TSC1641_LOAD_UV_MASK, TSC1641_LUL, val);
> +	case hwmon_in_crit:
> +		return tsc1641_alert_limit_read(data, TSC1641_LOAD_OV_MASK, TSC1641_LOL, val);
> +	case hwmon_in_lcrit_alarm:
> +		return tsc1641_alert_read(regmap, TSC1641_LOAD_UV_FLAG, val);
> +	case hwmon_in_crit_alarm:
> +		return tsc1641_alert_read(regmap, TSC1641_LOAD_OV_FLAG, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int tsc1641_curr_read(struct device *dev, u32 attr, long *val)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +	unsigned int regval;
> +	int ret;
> +
> +	/* Current limits are the shunt under/over voltage limits */
> +	switch (attr) {
> +	case hwmon_curr_input:
> +		ret = regmap_read(regmap, TSC1641_CURRENT, &regval);
> +		if (ret)
> +			return ret;
> +		*val = tsc1641_reg_to_value(data, TSC1641_CURRENT, regval);
> +		break;
> +	case hwmon_curr_lcrit:
> +		return tsc1641_alert_limit_read(data, TSC1641_SHUNT_UV_MASK,
> +						TSC1641_SUL, val);
> +	case hwmon_curr_crit:
> +		return tsc1641_alert_limit_read(data, TSC1641_SHUNT_OV_MASK,
> +						TSC1641_SOL, val);
> +	case hwmon_curr_lcrit_alarm:
> +		return tsc1641_alert_read(regmap, TSC1641_SHUNT_UV_FLAG, val);
> +	case hwmon_curr_crit_alarm:
> +		return tsc1641_alert_read(regmap, TSC1641_SHUNT_OV_FLAG, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int tsc1641_power_read(struct device *dev, u32 attr, long *val)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +	unsigned int regval;
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_power_input:
> +		ret = regmap_read(regmap, TSC1641_POWER, &regval);
> +		if (ret)
> +			return ret;
> +		*val = tsc1641_reg_to_value(data, TSC1641_POWER, regval);
> +		break;
> +	case hwmon_power_crit:
> +		return tsc1641_alert_limit_read(data, TSC1641_POWER_OVER_MASK,
> +						TSC1641_POL, val);
> +	case hwmon_power_crit_alarm:
> +		return tsc1641_alert_read(regmap, TSC1641_POWER_OVER_FLAG, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int tsc1641_temp_read(struct device *dev, u32 attr, long *val)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +	unsigned int regval;
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		ret = regmap_read(regmap, TSC1641_TEMP, &regval);
> +		if (ret)
> +			return ret;
> +		*val = tsc1641_reg_to_value(data, TSC1641_TEMP, regval);
> +		break;
> +	case hwmon_temp_crit:
> +		return tsc1641_alert_limit_read(data, TSC1641_TEMP_OVER_MASK,
> +						TSC1641_TOL, val);
> +	case hwmon_temp_crit_alarm:
> +		return tsc1641_alert_read(regmap, TSC1641_TEMP_OVER_FLAG, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int tsc1641_in_write(struct device *dev, u32 attr, long val)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_in_lcrit:
> +		return tsc1641_alert_limit_write(data, TSC1641_LOAD_UV_MASK, TSC1641_LUL, val);
> +	case hwmon_in_crit:
> +		return tsc1641_alert_limit_write(data, TSC1641_LOAD_OV_MASK, TSC1641_LOL, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int tsc1641_curr_write(struct device *dev, u32 attr, long val)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_curr_lcrit:
> +		return tsc1641_alert_limit_write(data, TSC1641_SHUNT_UV_MASK,
> +						TSC1641_SUL, val);
> +	case hwmon_curr_crit:
> +		return tsc1641_alert_limit_write(data, TSC1641_SHUNT_OV_MASK,
> +						TSC1641_SOL, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int tsc1641_power_write(struct device *dev, u32 attr, long val)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_power_crit:
> +		return tsc1641_alert_limit_write(data, TSC1641_POWER_OVER_MASK,
> +						 TSC1641_POL, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static int tsc1641_temp_write(struct device *dev, u32 attr, long val)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_temp_crit:
> +		return tsc1641_alert_limit_write(data, TSC1641_TEMP_OVER_MASK,
> +						 TSC1641_TOL, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static umode_t tsc1641_is_visible(const void *data, enum hwmon_sensor_types type,
> +				  u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_chip:
> +		switch (attr) {
> +		case hwmon_chip_update_interval:
> +			return 0644;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +			return 0444;
> +		case hwmon_in_lcrit:
> +		case hwmon_in_crit:
> +			return 0644;
> +		case hwmon_in_lcrit_alarm:
> +		case hwmon_in_crit_alarm:
> +			return 0444;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_input:
> +			return 0444;
> +		case hwmon_curr_lcrit:
> +		case hwmon_curr_crit:
> +			return 0644;
> +		case hwmon_curr_lcrit_alarm:
> +		case hwmon_curr_crit_alarm:
> +			return 0444;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_power:
> +		switch (attr) {
> +		case hwmon_power_input:
> +			return 0444;
> +		case hwmon_power_crit:
> +			return 0644;
> +		case hwmon_power_crit_alarm:
> +			return 0444;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_input:
> +			return 0444;
> +		case hwmon_temp_crit:
> +			return 0644;
> +		case hwmon_temp_crit_alarm:
> +			return 0444;
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static int tsc1641_read(struct device *dev, enum hwmon_sensor_types type,
> +			u32 attr, int channel, long *val)
> +{
> +	switch (type) {
> +	case hwmon_chip:
> +		return tsc1641_chip_read(dev, attr, val);
> +	case hwmon_in:
> +		return tsc1641_in_read(dev, attr, val);
> +	case hwmon_curr:
> +		return tsc1641_curr_read(dev, attr, val);
> +	case hwmon_power:
> +		return tsc1641_power_read(dev, attr, val);
> +	case hwmon_temp:
> +		return tsc1641_temp_read(dev, attr, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int tsc1641_write(struct device *dev, enum hwmon_sensor_types type,
> +			 u32 attr, int channel, long val)
> +{
> +	switch (type) {
> +	case hwmon_chip:
> +		return tsc1641_chip_write(dev, attr, val);
> +	case hwmon_in:
> +		return tsc1641_in_write(dev, attr, val);
> +	case hwmon_curr:
> +		return tsc1641_curr_write(dev, attr, val);
> +	case hwmon_power:
> +		return tsc1641_power_write(dev, attr, val);
> +	case hwmon_temp:
> +		return tsc1641_temp_write(dev, attr, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static const struct hwmon_channel_info * const tsc1641_info[] = {
> +	HWMON_CHANNEL_INFO(chip,
> +			   HWMON_C_UPDATE_INTERVAL),
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT | HWMON_I_CRIT | HWMON_I_CRIT_ALARM |
> +			   HWMON_I_LCRIT | HWMON_I_LCRIT_ALARM),
> +	HWMON_CHANNEL_INFO(curr,
> +			   HWMON_C_INPUT | HWMON_C_CRIT | HWMON_C_CRIT_ALARM |
> +			   HWMON_C_LCRIT | HWMON_C_LCRIT_ALARM),
> +	HWMON_CHANNEL_INFO(power,
> +			   HWMON_P_INPUT | HWMON_P_CRIT | HWMON_P_CRIT_ALARM),
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT | HWMON_T_CRIT | HWMON_T_CRIT_ALARM),
> +	NULL
> +};
> +
> +static ssize_t shunt_resistor_show(struct device *dev,
> +				   struct device_attribute *da, char *buf)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%li\n", data->rshunt_uohm);
> +}
> +
> +static ssize_t shunt_voltage_uvolts_show(struct device *dev,
> +					 struct device_attribute *da, char *buf)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read(regmap, TSC1641_SHUNT_VOLTAGE, &regval);
> +	if (ret)
> +		return ret;
> +	int val = tsc1641_reg_to_value(data, TSC1641_SHUNT_VOLTAGE, regval);
> +
> +	return sysfs_emit(buf, "%d\n", val);
> +}
> +
> +static ssize_t shunt_resistor_store(struct device *dev,
> +				    struct device_attribute *da,
> +				    const char *buf, size_t count)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +	unsigned long val;
> +	int ret;
> +
> +	ret = kstrtoul(buf, 10, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	mutex_lock(&data->update_lock);
> +	ret = tsc1641_set_shunt(data, val);
> +	mutex_unlock(&data->update_lock);
> +	if (ret < 0)
> +		return ret;
> +	return count;
> +}
> +
> +static const struct hwmon_ops tsc1641_hwmon_ops = {
> +	.is_visible = tsc1641_is_visible,
> +	.read = tsc1641_read,
> +	.write = tsc1641_write,
> +};
> +
> +static const struct hwmon_chip_info tsc1641_chip_info = {
> +	.ops = &tsc1641_hwmon_ops,
> +	.info = tsc1641_info,
> +};
> +
> +static DEVICE_ATTR_RW(shunt_resistor);
> +static DEVICE_ATTR_RO(shunt_voltage_uvolts);
> +
> +/* Rshunt and shunt voltage value is exposed via sysfs attributes */
> +static struct attribute *tsc1641_attrs[] = {
> +	&dev_attr_shunt_resistor.attr,
> +	&dev_attr_shunt_voltage_uvolts.attr,

Either report as standard voltage (in0_input) or drop entirely.
The shunt voltage can be calculated from the shunt resisor value and
the current. A non-standard attribute to report it does not add value.

> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(tsc1641);
> +
> +static int tsc1641_init(struct device *dev, struct tsc1641_data *data)
> +{
> +	struct regmap *regmap = data->regmap;
> +	u32 shunt;
> +	int ret;
> +
> +	if (device_property_read_u32(dev, "shunt-resistor", &shunt) < 0) {
> +		shunt = TSC1641_RSHUNT_DEFAULT;
> +		dev_info(dev, "using default shunt-resistor value =%u uOhm\n",

The "=" does not add any value here.

> +			 shunt);
> +	}
> +
> +	ret = tsc1641_set_shunt(data, shunt);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(regmap, TSC1641_CONFIG, TSC1641_CONFIG_DEFAULT);
> +	if (ret < 0)
> +		return ret;
> +
> +	bool active_high = device_property_read_bool(dev, "st,alert-polarity-active-high");
> +
> +	regmap_update_bits(regmap, TSC1641_MASK, TSC1641_ALERT_POL_MASK,
> +			   FIELD_PREP(TSC1641_ALERT_POL_MASK, active_high));

Why ignore errors here ?

> +
> +	return 0;
> +}
> +
> +static int tsc1641_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct tsc1641_data *data;
> +	struct device *hwmon_dev;
> +	int ret;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +	mutex_init(&data->update_lock);
> +
> +	data->regmap = devm_regmap_init_i2c(client, &tsc1641_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		dev_err(dev, "failed to allocate register map\n");
> +		return PTR_ERR(data->regmap);
> +	}
> +
> +	ret = tsc1641_init(dev, data);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to configure device\n");
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
> +							 data, &tsc1641_chip_info, tsc1641_groups);
> +	if (IS_ERR(hwmon_dev))
> +		return PTR_ERR(hwmon_dev);
> +
> +	dev_info(dev, "power monitor %s (Rshunt = %li uOhm)\n",
> +		 client->name, data->rshunt_uohm);

Rshunt is displayed twice if the default is set. This is unnecessary noise.

> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id tsc1641_id[] = {
> +	{ "tsc1641", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, tsc1641_id);
> +
> +static const struct of_device_id __maybe_unused tsc1641_of_match[] = {
> +	{ .compatible = "st,tsc1641" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, tsc1641_of_match);
> +
> +static struct i2c_driver tsc1641_driver = {
> +	.driver = {
> +		.name = "tsc1641",
> +		.of_match_table = of_match_ptr(tsc1641_of_match),
> +	},
> +	.probe = tsc1641_probe,
> +	.id_table = tsc1641_id,
> +};
> +
> +module_i2c_driver(tsc1641_driver);
> +
> +MODULE_AUTHOR("Igor Reznichenko <igor@reznichenko.net>");
> +MODULE_DESCRIPTION("tsc1641 driver");
> +MODULE_LICENSE("GPL");
> +

Unnecessary empty line.

Re: [PATCH 1/5] drivers/hwmon: Add TSC1641 I2C power monitor driver

[Igor Reznichenko]

Guenter,
Thanks for the detailed feedback. I will address it.

> Please send a register dump.

Here's register dump after init during run: 

tsc1641 1-0040: 0x00: 0x003f
tsc1641 1-0040: 0x01: 0x0253
tsc1641 1-0040: 0x02: 0x0dc0
tsc1641 1-0040: 0x03: 0x0053
tsc1641 1-0040: 0x04: 0x0250
tsc1641 1-0040: 0x05: 0x0033
tsc1641 1-0040: 0x06: 0x0000
tsc1641 1-0040: 0x07: 0x0000
tsc1641 1-0040: 0x08: 0x01f4
tsc1641 1-0040: 0x09: 0x0000
tsc1641 1-0040: 0x0a: 0x0000
tsc1641 1-0040: 0x0b: 0x0000
tsc1641 1-0040: 0x0c: 0x0000
tsc1641 1-0040: 0x0d: 0x0000
tsc1641 1-0040: 0x0e: 0x0000
tsc1641 1-0040: 0xfe: 0x0006
tsc1641 1-0040: 0xff: 0x1000

> > +
> > +	/*
> > +	 * Disable alert mask first, then write the value and enable alert mask
> Why ? 

The idea was to prevent potential previous alert from propagating when changing 
the value, plus to only enable alert when crit/lcrit value is non-zero. 
But given your response below this is not the right thing to do.

> Disabling alerts if the limit is 0 is wrong: The limit can be set
> to 0 on purpose. Only unmasking the limit if a limit is set is just as wrong.
> Either limits are enabled and reported, or they are disabled and the attributes
> must not be generated. Mis-using the ABI to declare "If the limit value is
> 0, mask the limit. Otherwise set the limit and unmask it" is unacceptable.

Thanks for clarification. So would you recommend then that all alerts should 
be always on/unmasked for this chip or to add custom sysfs attributes to control 
them, since it has this capability?

> Either report as standard voltage (in0_input) or drop entirely.
> The shunt voltage can be calculated from the shunt resisor value and
> the current. A non-standard attribute to report it does not add value.

I'll drop it since the shunt voltage resolution is 2.5uV and it won't give 
accurate information to report it in mV.

Thanks, Igor

Re: [PATCH 1/5] drivers/hwmon: Add TSC1641 I2C power monitor driver

[Guenter Roeck]

On Thu, Oct 23, 2025 at 12:50:50AM -0700, Igor Reznichenko wrote:
> Guenter,
> Thanks for the detailed feedback. I will address it.
> 
> > Please send a register dump.
> 
> Here's register dump after init during run: 
> 
> tsc1641 1-0040: 0x00: 0x003f
> tsc1641 1-0040: 0x01: 0x0253
> tsc1641 1-0040: 0x02: 0x0dc0
> tsc1641 1-0040: 0x03: 0x0053
> tsc1641 1-0040: 0x04: 0x0250
> tsc1641 1-0040: 0x05: 0x0033
> tsc1641 1-0040: 0x06: 0x0000
> tsc1641 1-0040: 0x07: 0x0000
> tsc1641 1-0040: 0x08: 0x01f4
> tsc1641 1-0040: 0x09: 0x0000
> tsc1641 1-0040: 0x0a: 0x0000
> tsc1641 1-0040: 0x0b: 0x0000
> tsc1641 1-0040: 0x0c: 0x0000
> tsc1641 1-0040: 0x0d: 0x0000
> tsc1641 1-0040: 0x0e: 0x0000
> tsc1641 1-0040: 0xfe: 0x0006
> tsc1641 1-0040: 0xff: 0x1000
> 
Great, thanks a lot!

> > > +
> > > +	/*
> > > +	 * Disable alert mask first, then write the value and enable alert mask
> > Why ? 
> 
> The idea was to prevent potential previous alert from propagating when changing 
> the value, plus to only enable alert when crit/lcrit value is non-zero. 
> But given your response below this is not the right thing to do.
> 
> > Disabling alerts if the limit is 0 is wrong: The limit can be set
> > to 0 on purpose. Only unmasking the limit if a limit is set is just as wrong.
> > Either limits are enabled and reported, or they are disabled and the attributes
> > must not be generated. Mis-using the ABI to declare "If the limit value is
> > 0, mask the limit. Otherwise set the limit and unmask it" is unacceptable.
> 
> Thanks for clarification. So would you recommend then that all alerts should 
> be always on/unmasked for this chip or to add custom sysfs attributes to control 
> them, since it has this capability?
> 

Almost every chip has that capability. That does not warrant a custom sysfs
attribute. I'd suggest to just enable them all.

Guenter

[PATCH v2 0/2] hwmon: Add TSC1641 I2C power monitor driver

[Igor Reznichenko]

This patch series adds support for the ST Microelectronics TSC1641
I2C power monitor. The TSC1641 provides bus voltage, current, power,
and temperature measurements via the hwmon subsystem. The driver 
supports optional ALERT pin polarity configuration and exposes the
shunt resistor value and update interval via sysfs.

Tested on Raspberry Pi 3B+ with a TSC1641 evaluation board.

Changes in v2:
- Fixed devicetree binding name and formatting issues
- Alert limits are handled in a standard way
- Clamped alert limit values, constrained valid shunt values
- Cleaned up includes, fixed various style issues
- Expanded documentation

Igor Reznichenko (2):
  dt-bindings: hwmon: Add support for ST TSC1641 power monitor
  hwmon: Add TSC1641 I2C power monitor driver

 .../devicetree/bindings/hwmon/st,tsc1641.yaml |  59 ++
 Documentation/hwmon/index.rst                 |   1 +
 Documentation/hwmon/tsc1641.rst               |  84 +++
 drivers/hwmon/Kconfig                         |  12 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/tsc1641.c                       | 703 ++++++++++++++++++
 6 files changed, 860 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/st,tsc1641.yaml
 create mode 100644 Documentation/hwmon/tsc1641.rst
 create mode 100644 drivers/hwmon/tsc1641.c

-- 
2.43.0

[PATCH v2 1/2] dt-bindings: hwmon: Add support for ST TSC1641 power monitor

[Igor Reznichenko]

Add a devicetree binding for the TSC1641 I2C power monitor.

Signed-off-by: Igor Reznichenko <igor@reznichenko.net>
---
 .../devicetree/bindings/hwmon/st,tsc1641.yaml | 59 +++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/st,tsc1641.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/st,tsc1641.yaml b/Documentation/devicetree/bindings/hwmon/st,tsc1641.yaml
new file mode 100644
index 000000000000..cfa0e2cca869
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/st,tsc1641.yaml
@@ -0,0 +1,59 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/st,tsc1641.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ST Microelectronics TSC1641 I2C power monitor
+
+maintainers:
+  - Igor Reznichenko <igor@reznichenko.net>
+
+description: |
+  TSC1641 is a 60 V, 16-bit high-precision power monitor with I2C and
+  MIPI I3C interface
+
+  Datasheets:
+    https://www.st.com/resource/en/datasheet/tsc1641.pdf
+
+properties:
+  compatible:
+    const: st,tsc1641
+
+  reg:
+    maxItems: 1
+
+  shunt-resistor-micro-ohms:
+    description: Shunt resistor value in micro-ohms. Since device has internal
+      16-bit RSHUNT register with 10 uOhm LSB, the maximum value is capped at
+      655.35 mOhm.
+    minimum: 100
+    default: 1000
+    maximum: 655350
+
+  st,alert-polarity-active-high:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: Default value is 0 which configures the normal polarity of the
+      ALERT pin, being active low open-drain. Setting this to 1 configures the
+      polarity of the ALERT pin to be inverted and active high open-drain.
+      Specify this property to set the alert polarity to active-high.
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        power-sensor@40 {
+            compatible = "st,tsc1641";
+            reg = <0x40>;
+            shunt-resistor-micro-ohms = <1000>;
+            st,alert-polarity-active-high;
+        };
+    };
-- 
2.43.0

[PATCH v2 2/2] hwmon: Add TSC1641 I2C power monitor driver

[Igor Reznichenko]

Add a driver for the ST Microelectronics TSC1641 16-bit high-precision
power monitor. The driver supports reading bus voltage, current, power,
and temperature. Sysfs attributes are exposed for shunt resistor and
update interval. The driver integrates with the hwmon subsystem and
supports optional ALERT pin polarity configuration.

Signed-off-by: Igor Reznichenko <igor@reznichenko.net>
---
 Documentation/hwmon/index.rst   |   1 +
 Documentation/hwmon/tsc1641.rst |  84 ++++
 drivers/hwmon/Kconfig           |  12 +
 drivers/hwmon/Makefile          |   1 +
 drivers/hwmon/tsc1641.c         | 703 ++++++++++++++++++++++++++++++++
 5 files changed, 801 insertions(+)
 create mode 100644 Documentation/hwmon/tsc1641.rst
 create mode 100644 drivers/hwmon/tsc1641.c

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 51a5bdf75b08..4fb9f91f83b3 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -253,6 +253,7 @@ Hardware Monitoring Kernel Drivers
    tps40422
    tps53679
    tps546d24
+   tsc1641
    twl4030-madc-hwmon
    ucd9000
    ucd9200
diff --git a/Documentation/hwmon/tsc1641.rst b/Documentation/hwmon/tsc1641.rst
new file mode 100644
index 000000000000..f692a8ccbffc
--- /dev/null
+++ b/Documentation/hwmon/tsc1641.rst
@@ -0,0 +1,84 @@
+Kernel driver tsc1641
+=====================
+
+Supported chips:
+
+  * ST TSC1641
+
+    Prefix: 'tsc1641'
+
+    Addresses scanned: -
+
+    Datasheet:
+	https://www.st.com/resource/en/datasheet/tsc1641.pdf
+
+Author:
+	- Igor Reznichenko <igor@reznichenko.net>
+
+
+Description
+-----------
+
+The TSC1641 is a high-precision current, voltage, power, and temperature
+monitoring analog front-end (AFE). It monitors current into a shunt resistor and
+load voltage up to 60 V in a synchronized way. Digital bus interface is
+I2C/SMbus. The TSC1641 allows the assertion of several alerts regarding the
+voltage, current, power and temperature.
+
+Usage Notes
+-----------
+
+The TSC1641 driver requires the value of the external shunt resistor to
+correctly compute current and power measurements. The resistor value, in
+micro-ohms, should be provided either through the device tree property
+"shunt-resistor-micro-ohms" or via writable sysfs attribute "shunt_resistor".
+Please refer to the Documentation/devicetree/bindings/hwmon/st,tsc1641.yaml
+for bindings if the device tree is used.
+
+Supported range of shunt resistor values is from 100 uOhm to 655.35 mOhm.
+When selecting the value keep in mind device maximum DC power measurement is
+1600W. See datasheet p.22 for ST recommendations on selecting shunt value.
+
+If the shunt resistor value is not specified in the device tree, the driver
+initializes it to 1000 uOhm by default. Users may configure the correct shunt
+resistor value at runtime by writing to the "shunt_resistor" sysfs attribute.
+
+The driver only supports continuous operating mode.
+Measurement ranges:
+
+================ ===============================================================
+Current          Dependent on shunt
+Bus voltage      0-60V
+Maximum DC power 1600W
+Temperature      -40C to +125C
+================ ===============================================================
+
+Sysfs entries
+-------------
+
+==================== ===========================================================
+in0_input            bus voltage (mV)
+in0_crit             bus voltage crit alarm limit (mV)
+in0_crit_alarm       bus voltage crit alarm limit exceeded
+in0_lcrit            bus voltage low-crit alarm limit (mV)
+in0_lcrit_alarm      bus voltage low-crit alarm limit exceeded
+
+curr1_input          current measurement (mA)
+curr1_crit           current crit alarm limit (mA)
+curr1_crit_alarm     current crit alarm limit exceeded
+curr1_lcrit          current low-crit alarm limit (mA)
+curr1_lcrit_alarm    current low-crit alarm limit exceeded
+
+power1_input         power measurement (uW)
+power1_crit          power crit alarm limit (uW)
+power1_crit_alarm    power crit alarm limit exceeded
+
+shunt_resistor       shunt resistor value (uOhms)
+
+temp1_input          temperature measurement (mdegC)
+temp1_crit           temperature crit alarm limit (mdegC)
+temp1_crit_alarm     temperature crit alarm limit exceeded
+
+update_interval      data conversion time (1 - 33ms), longer conversion time
+                     corresponds to higher effective resolution in bits
+==================== ===========================================================
\ No newline at end of file
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 2760feb9f83b..b9d7b02932a6 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2434,6 +2434,18 @@ config SENSORS_TMP513
 	  This driver can also be built as a module. If so, the module
 	  will be called tmp513.
 
+config SENSORS_TSC1641
+	tristate "ST Microelectronics TSC1641 Power Monitor"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  If you say yes here you get support for TSC1641 power  monitor chip.
+	  The TSC1641 driver is configured for the default configuration of
+	  the part except temperature is enabled by default.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called tsc1641.
+
 config SENSORS_VEXPRESS
 	tristate "Versatile Express"
 	depends on VEXPRESS_CONFIG
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 73b2abdcc6dd..a8de5bc69f2a 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -233,6 +233,7 @@ obj-$(CONFIG_SENSORS_TMP401)	+= tmp401.o
 obj-$(CONFIG_SENSORS_TMP421)	+= tmp421.o
 obj-$(CONFIG_SENSORS_TMP464)	+= tmp464.o
 obj-$(CONFIG_SENSORS_TMP513)	+= tmp513.o
+obj-$(CONFIG_SENSORS_TSC1641)	+= tsc1641.o
 obj-$(CONFIG_SENSORS_VEXPRESS)	+= vexpress-hwmon.o
 obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
 obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
diff --git a/drivers/hwmon/tsc1641.c b/drivers/hwmon/tsc1641.c
new file mode 100644
index 000000000000..56f6d0ba2b49
--- /dev/null
+++ b/drivers/hwmon/tsc1641.c
@@ -0,0 +1,703 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for ST Microelectronics TSC1641 I2C power monitor
+ *
+ * 60 V, 16-bit high-precision power monitor with I2C and MIPI I3C interface
+ * Datasheet: https://www.st.com/resource/en/datasheet/tsc1641.pdf
+ *
+ * Copyright (C) 2025 Igor Reznichenko <igor@reznichenko.net>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/sysfs.h>
+
+/* I2C registers */
+#define TSC1641_CONFIG		0x00
+#define TSC1641_SHUNT_VOLTAGE	0x01
+#define TSC1641_LOAD_VOLTAGE	0x02
+#define TSC1641_POWER		0x03
+#define TSC1641_CURRENT		0x04
+#define TSC1641_TEMP		0x05
+#define TSC1641_MASK		0x06
+#define TSC1641_FLAG		0x07
+#define TSC1641_RSHUNT		0x08 /* Shunt resistance */
+#define TSC1641_SOL		0x09
+#define TSC1641_SUL		0x0A
+#define TSC1641_LOL		0x0B
+#define TSC1641_LUL		0x0C
+#define TSC1641_POL		0x0D
+#define TSC1641_TOL		0x0E
+#define TSC1641_MANUF_ID	0xFE /* 0x0006 */
+#define TSC1641_DIE_ID		0xFF /* 0x1000 */
+#define TSC1641_MAX_REG		0xFF
+
+#define TSC1641_RSHUNT_DEFAULT	1000   /* 1mOhm */
+#define TSC1641_CONFIG_DEFAULT	0x003F /* Enable temperature sensor */
+#define TSC1641_MASK_DEFAULT	0xFC00 /* Unmask all alerts */
+
+/* Bit mask for conversion time in the configuration register */
+#define TSC1641_CONV_TIME_MASK	GENMASK(7, 4)
+
+#define TSC1641_CONV_TIME_DEFAULT	1024
+#define TSC1641_MIN_UPDATE_INTERVAL	1024
+
+/* LSB value of different registers */
+#define TSC1641_VLOAD_LSB_MVOLT		2
+#define TSC1641_POWER_LSB_UWATT		25000
+#define TSC1641_VSHUNT_LSB_NVOLT	2500 /* Use nanovolts to make it integer */
+#define TSC1641_RSHUNT_LSB_UOHM		10
+#define TSC1641_TEMP_LSB_MDEGC		500
+
+/* Limits based on datasheet */
+#define TSC1641_RSHUNT_MIN_UOHM		100
+#define TSC1641_RSHUNT_MAX_UOHM		655350
+#define TSC1641_VLOAD_MAX_MVOLT		60000
+#define TSC1641_CURRENT_MIN_MAMP	(-819175)
+#define TSC1641_CURRENT_MAX_MAMP	819175
+#define TSC1641_TEMP_MIN_MDEGC		(-20000)
+#define TSC1641_TEMP_MAX_MDEGC		145000
+#define TSC1641_POWER_MAX_UWATT		1600000000
+
+#define TSC1641_ALERT_POL_MASK		BIT(1)
+#define TSC1641_ALERT_LATCH_EN_MASK	BIT(0)
+
+/* Flags indicating alerts in TSC1641_FLAG register*/
+#define TSC1641_SHUNT_OV_FLAG		BIT(6)
+#define TSC1641_SHUNT_UV_FLAG		BIT(5)
+#define TSC1641_LOAD_OV_FLAG		BIT(4)
+#define TSC1641_LOAD_UV_FLAG		BIT(3)
+#define TSC1641_POWER_OVER_FLAG		BIT(2)
+#define TSC1641_TEMP_OVER_FLAG		BIT(1)
+
+static bool tsc1641_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case TSC1641_CONFIG:
+	case TSC1641_MASK:
+	case TSC1641_RSHUNT:
+	case TSC1641_SOL:
+	case TSC1641_SUL:
+	case TSC1641_LOL:
+	case TSC1641_LUL:
+	case TSC1641_POL:
+	case TSC1641_TOL:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool tsc1641_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case TSC1641_SHUNT_VOLTAGE:
+	case TSC1641_LOAD_VOLTAGE:
+	case TSC1641_POWER:
+	case TSC1641_CURRENT:
+	case TSC1641_TEMP:
+	case TSC1641_FLAG:
+	case TSC1641_MANUF_ID:
+	case TSC1641_DIE_ID:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config tsc1641_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.use_single_write = true,
+	.use_single_read = true,
+	.max_register = TSC1641_MAX_REG,
+	.cache_type = REGCACHE_MAPLE,
+	.volatile_reg = tsc1641_volatile_reg,
+	.writeable_reg = tsc1641_writeable_reg,
+};
+
+struct tsc1641_data {
+	long rshunt_uohm;
+	long current_lsb_ua;
+	struct regmap *regmap;
+	struct i2c_client *client;
+};
+
+/*
+ * Upper limit due to chip 16-bit shunt register, lower limit to
+ * prevent current and power registers overflow
+ */
+static inline int tsc1641_validate_shunt(u32 val)
+{
+	if (val < TSC1641_RSHUNT_MIN_UOHM || val > TSC1641_RSHUNT_MAX_UOHM)
+		return -EINVAL;
+	return 0;
+}
+
+static int tsc1641_set_shunt(struct tsc1641_data *data, u32 val)
+{
+	struct regmap *regmap = data->regmap;
+	long rshunt_reg;
+
+	if (tsc1641_validate_shunt(val) < 0)
+		return -EINVAL;
+
+	data->rshunt_uohm = val;
+	data->current_lsb_ua = DIV_ROUND_CLOSEST(TSC1641_VSHUNT_LSB_NVOLT * 1000,
+						 data->rshunt_uohm);
+	/* RSHUNT register LSB is 10uOhm so need to divide further*/
+	rshunt_reg = DIV_ROUND_CLOSEST(data->rshunt_uohm, TSC1641_RSHUNT_LSB_UOHM);
+	return regmap_write(regmap, TSC1641_RSHUNT, clamp_val(rshunt_reg, 0, USHRT_MAX));
+}
+
+/*
+ * Conversion times in uS, value in CONFIG[CT3:CT0] corresponds to index in this array
+ * See "Table 14. CT3 to CT0: conversion time" in:
+ * https://www.st.com/resource/en/datasheet/tsc1641.pdf
+ */
+static const int tsc1641_conv_times[] = { 128, 256, 512, 1024, 2048, 4096, 8192, 16384, 32768 };
+
+static int tsc1641_reg_to_upd_interval(u16 config)
+{
+	int idx = FIELD_GET(TSC1641_CONV_TIME_MASK, config);
+
+	idx = clamp_val(idx, 0, ARRAY_SIZE(tsc1641_conv_times) - 1);
+	int conv_time = tsc1641_conv_times[idx];
+
+	/* Don't support sub-millisecond update interval as it's not supported in hwmon */
+	conv_time = max(conv_time, TSC1641_MIN_UPDATE_INTERVAL);
+	/* Return nearest value in milliseconds */
+	return DIV_ROUND_CLOSEST(conv_time, 1000);
+}
+
+static u16 tsc1641_upd_interval_to_reg(long interval)
+{
+	/* Supported interval is 1ms - 33ms */
+	interval = clamp_val(interval, 1, 33);
+
+	int conv = interval * 1000;
+	int conv_bits = find_closest(conv, tsc1641_conv_times,
+				     ARRAY_SIZE(tsc1641_conv_times));
+
+	return FIELD_PREP(TSC1641_CONV_TIME_MASK, conv_bits);
+}
+
+static int tsc1641_chip_write(struct device *dev, u32 attr, long val)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_chip_update_interval:
+		return regmap_update_bits(data->regmap, TSC1641_CONFIG,
+					  TSC1641_CONV_TIME_MASK,
+					  tsc1641_upd_interval_to_reg(val));
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int tsc1641_chip_read(struct device *dev, u32 attr, long *val)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+	u32 regval;
+	int ret;
+
+	switch (attr) {
+	case hwmon_chip_update_interval:
+		ret = regmap_read(data->regmap, TSC1641_CONFIG, &regval);
+		if (ret)
+			return ret;
+
+		*val = tsc1641_reg_to_upd_interval(regval);
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int tsc1641_alert_read(struct regmap *regmap, u32 flag, long *val)
+{
+	unsigned int regval;
+	int ret;
+
+	ret = regmap_read_bypassed(regmap, TSC1641_FLAG, &regval);
+	if (ret)
+		return ret;
+
+	*val = !!(regval & flag);
+	return 0;
+}
+
+static int tsc1641_in_read(struct device *dev, u32 attr, long *val)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	unsigned int regval;
+	int ret, reg;
+
+	switch (attr) {
+	case hwmon_in_input:
+		reg = TSC1641_LOAD_VOLTAGE;
+		break;
+	case hwmon_in_lcrit:
+		reg = TSC1641_LUL;
+		break;
+	case hwmon_in_crit:
+		reg = TSC1641_LOL;
+		break;
+	case hwmon_in_lcrit_alarm:
+		return tsc1641_alert_read(regmap, TSC1641_LOAD_UV_FLAG, val);
+	case hwmon_in_crit_alarm:
+		return tsc1641_alert_read(regmap, TSC1641_LOAD_OV_FLAG, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	ret = regmap_read(regmap, reg, &regval);
+	if (ret)
+		return ret;
+
+	*val = regval * TSC1641_VLOAD_LSB_MVOLT;
+	return 0;
+}
+
+static int tsc1641_curr_read(struct device *dev, u32 attr, long *val)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	int regval;
+	int ret, reg;
+
+	/* Current limits are the shunt under/over voltage limits */
+	switch (attr) {
+	case hwmon_curr_input:
+		reg = TSC1641_CURRENT;
+		break;
+	case hwmon_curr_lcrit:
+		reg = TSC1641_SUL;
+		break;
+	case hwmon_curr_crit:
+		reg = TSC1641_SOL;
+		break;
+	case hwmon_curr_lcrit_alarm:
+		return tsc1641_alert_read(regmap, TSC1641_SHUNT_UV_FLAG, val);
+	case hwmon_curr_crit_alarm:
+		return tsc1641_alert_read(regmap, TSC1641_SHUNT_OV_FLAG, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	ret = regmap_read(regmap, reg, &regval);
+	if (ret)
+		return ret;
+
+	/* Current in milliamps */
+	*val = DIV_ROUND_CLOSEST((s16)regval * data->current_lsb_ua, 1000);
+	return 0;
+}
+
+static int tsc1641_power_read(struct device *dev, u32 attr, long *val)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	unsigned int regval;
+	int ret, reg;
+
+	switch (attr) {
+	case hwmon_power_input:
+		reg = TSC1641_POWER;
+		break;
+	case hwmon_power_crit:
+		reg = TSC1641_POL;
+		break;
+	case hwmon_power_crit_alarm:
+		return tsc1641_alert_read(regmap, TSC1641_POWER_OVER_FLAG, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	ret = regmap_read(regmap, reg, &regval);
+	if (ret)
+		return ret;
+
+	*val = regval * TSC1641_POWER_LSB_UWATT;
+	return 0;
+}
+
+static int tsc1641_temp_read(struct device *dev, u32 attr, long *val)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	unsigned int regval;
+	int ret, reg;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		reg = TSC1641_TEMP;
+		break;
+	case hwmon_temp_crit:
+		reg = TSC1641_TOL;
+		break;
+	case hwmon_temp_crit_alarm:
+		return tsc1641_alert_read(regmap, TSC1641_TEMP_OVER_FLAG, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	ret = regmap_read(regmap, reg, &regval);
+	if (ret)
+		return ret;
+
+	*val = (s16)regval * TSC1641_TEMP_LSB_MDEGC;
+	return 0;
+}
+
+static int tsc1641_in_write(struct device *dev, u32 attr, long val)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	unsigned int regval;
+	int reg;
+
+	switch (attr) {
+	case hwmon_in_lcrit:
+		reg = TSC1641_LUL;
+		break;
+	case hwmon_in_crit:
+		reg = TSC1641_LOL;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	val = clamp_val(val, 0, TSC1641_VLOAD_MAX_MVOLT);
+	regval = DIV_ROUND_CLOSEST(val, TSC1641_VLOAD_LSB_MVOLT);
+
+	return regmap_write(regmap, reg, clamp_val(regval, 0, USHRT_MAX));
+}
+
+static int tsc1641_curr_write(struct device *dev, u32 attr, long val)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	int reg, regval;
+
+	switch (attr) {
+	case hwmon_curr_lcrit:
+		reg = TSC1641_SUL;
+		break;
+	case hwmon_curr_crit:
+		reg = TSC1641_SOL;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	/* Clamp to max 16-bit represantable current at min Rshunt */
+	val = clamp_val(val, TSC1641_CURRENT_MIN_MAMP, TSC1641_CURRENT_MAX_MAMP);
+	/* Convert val in milliamps to voltage */
+	regval = DIV_ROUND_CLOSEST(val * data->rshunt_uohm, TSC1641_VSHUNT_LSB_NVOLT);
+
+	return regmap_write(regmap, reg, clamp_val(regval, SHRT_MIN, SHRT_MAX));
+}
+
+static int tsc1641_power_write(struct device *dev, u32 attr, long val)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	unsigned int regval;
+
+	switch (attr) {
+	case hwmon_power_crit:
+		val = clamp_val(val, 0, TSC1641_POWER_MAX_UWATT);
+		regval = DIV_ROUND_CLOSEST(val, TSC1641_POWER_LSB_UWATT);
+		return regmap_write(regmap, TSC1641_POL, clamp_val(regval, 0, USHRT_MAX));
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int tsc1641_temp_write(struct device *dev, u32 attr, long val)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+	struct regmap *regmap = data->regmap;
+	int regval;
+
+	switch (attr) {
+	case hwmon_temp_crit:
+		val = clamp_val(val, TSC1641_TEMP_MIN_MDEGC, TSC1641_TEMP_MAX_MDEGC);
+		regval = DIV_ROUND_CLOSEST(val, TSC1641_TEMP_LSB_MDEGC);
+		return regmap_write(regmap, TSC1641_TOL, clamp_val(regval, SHRT_MIN, SHRT_MAX));
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static umode_t tsc1641_is_visible(const void *data, enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_chip:
+		switch (attr) {
+		case hwmon_chip_update_interval:
+			return 0644;
+		default:
+			break;
+		}
+		break;
+	case hwmon_in:
+		switch (attr) {
+		case hwmon_in_input:
+			return 0444;
+		case hwmon_in_lcrit:
+		case hwmon_in_crit:
+			return 0644;
+		case hwmon_in_lcrit_alarm:
+		case hwmon_in_crit_alarm:
+			return 0444;
+		default:
+			break;
+		}
+		break;
+	case hwmon_curr:
+		switch (attr) {
+		case hwmon_curr_input:
+			return 0444;
+		case hwmon_curr_lcrit:
+		case hwmon_curr_crit:
+			return 0644;
+		case hwmon_curr_lcrit_alarm:
+		case hwmon_curr_crit_alarm:
+			return 0444;
+		default:
+			break;
+		}
+		break;
+	case hwmon_power:
+		switch (attr) {
+		case hwmon_power_input:
+			return 0444;
+		case hwmon_power_crit:
+			return 0644;
+		case hwmon_power_crit_alarm:
+			return 0444;
+		default:
+			break;
+		}
+		break;
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+			return 0444;
+		case hwmon_temp_crit:
+			return 0644;
+		case hwmon_temp_crit_alarm:
+			return 0444;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static int tsc1641_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	switch (type) {
+	case hwmon_chip:
+		return tsc1641_chip_read(dev, attr, val);
+	case hwmon_in:
+		return tsc1641_in_read(dev, attr, val);
+	case hwmon_curr:
+		return tsc1641_curr_read(dev, attr, val);
+	case hwmon_power:
+		return tsc1641_power_read(dev, attr, val);
+	case hwmon_temp:
+		return tsc1641_temp_read(dev, attr, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int tsc1641_write(struct device *dev, enum hwmon_sensor_types type,
+			 u32 attr, int channel, long val)
+{
+	switch (type) {
+	case hwmon_chip:
+		return tsc1641_chip_write(dev, attr, val);
+	case hwmon_in:
+		return tsc1641_in_write(dev, attr, val);
+	case hwmon_curr:
+		return tsc1641_curr_write(dev, attr, val);
+	case hwmon_power:
+		return tsc1641_power_write(dev, attr, val);
+	case hwmon_temp:
+		return tsc1641_temp_write(dev, attr, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static const struct hwmon_channel_info * const tsc1641_info[] = {
+	HWMON_CHANNEL_INFO(chip,
+			   HWMON_C_UPDATE_INTERVAL),
+	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_INPUT | HWMON_I_CRIT | HWMON_I_CRIT_ALARM |
+			   HWMON_I_LCRIT | HWMON_I_LCRIT_ALARM),
+	HWMON_CHANNEL_INFO(curr,
+			   HWMON_C_INPUT | HWMON_C_CRIT | HWMON_C_CRIT_ALARM |
+			   HWMON_C_LCRIT | HWMON_C_LCRIT_ALARM),
+	HWMON_CHANNEL_INFO(power,
+			   HWMON_P_INPUT | HWMON_P_CRIT | HWMON_P_CRIT_ALARM),
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_CRIT | HWMON_T_CRIT_ALARM),
+	NULL
+};
+
+static ssize_t shunt_resistor_show(struct device *dev,
+				   struct device_attribute *da, char *buf)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, "%li\n", data->rshunt_uohm);
+}
+
+static ssize_t shunt_resistor_store(struct device *dev,
+				    struct device_attribute *da,
+				    const char *buf, size_t count)
+{
+	struct tsc1641_data *data = dev_get_drvdata(dev);
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul(buf, 10, &val);
+	if (ret < 0)
+		return ret;
+
+	if (val > U32_MAX)
+		return -EINVAL;
+
+	ret = tsc1641_set_shunt(data, (u32)val);
+	if (ret < 0)
+		return ret;
+	return count;
+}
+
+static const struct hwmon_ops tsc1641_hwmon_ops = {
+	.is_visible = tsc1641_is_visible,
+	.read = tsc1641_read,
+	.write = tsc1641_write,
+};
+
+static const struct hwmon_chip_info tsc1641_chip_info = {
+	.ops = &tsc1641_hwmon_ops,
+	.info = tsc1641_info,
+};
+
+static DEVICE_ATTR_RW(shunt_resistor);
+
+/* Shunt resistor value is exposed via sysfs attribute */
+static struct attribute *tsc1641_attrs[] = {
+	&dev_attr_shunt_resistor.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(tsc1641);
+
+static int tsc1641_init(struct device *dev, struct tsc1641_data *data)
+{
+	struct regmap *regmap = data->regmap;
+	bool active_high;
+	u32 shunt;
+	int ret;
+
+	if (device_property_read_u32(dev, "shunt-resistor-micro-ohms", &shunt) < 0)
+		shunt = TSC1641_RSHUNT_DEFAULT;
+
+	if (tsc1641_validate_shunt(shunt) < 0) {
+		dev_err(dev, "invalid shunt resistor value %u\n", shunt);
+		return -EINVAL;
+	}
+
+	ret = tsc1641_set_shunt(data, shunt);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(regmap, TSC1641_CONFIG, TSC1641_CONFIG_DEFAULT);
+	if (ret < 0)
+		return ret;
+
+	active_high = device_property_read_bool(dev, "st,alert-polarity-active-high");
+
+	return regmap_write(regmap, TSC1641_MASK, TSC1641_MASK_DEFAULT |
+			    FIELD_PREP(TSC1641_ALERT_POL_MASK, active_high));
+}
+
+static int tsc1641_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct tsc1641_data *data;
+	struct device *hwmon_dev;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->client = client;
+
+	data->regmap = devm_regmap_init_i2c(client, &tsc1641_regmap_config);
+	if (IS_ERR(data->regmap)) {
+		dev_err(dev, "failed to allocate register map\n");
+		return PTR_ERR(data->regmap);
+	}
+
+	ret = tsc1641_init(dev, data);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to configure device\n");
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
+							 data, &tsc1641_chip_info, tsc1641_groups);
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
+
+	dev_info(dev, "power monitor %s (Rshunt = %li uOhm)\n",
+		 client->name, data->rshunt_uohm);
+
+	return 0;
+}
+
+static const struct i2c_device_id tsc1641_id[] = {
+	{ "tsc1641", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, tsc1641_id);
+
+static const struct of_device_id __maybe_unused tsc1641_of_match[] = {
+	{ .compatible = "st,tsc1641" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, tsc1641_of_match);
+
+static struct i2c_driver tsc1641_driver = {
+	.driver = {
+		.name = "tsc1641",
+		.of_match_table = of_match_ptr(tsc1641_of_match),
+	},
+	.probe = tsc1641_probe,
+	.id_table = tsc1641_id,
+};
+
+module_i2c_driver(tsc1641_driver);
+
+MODULE_AUTHOR("Igor Reznichenko <igor@reznichenko.net>");
+MODULE_DESCRIPTION("tsc1641 driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH v2 0/2] hwmon: Add TSC1641 I2C power monitor driver

[Krzysztof Kozlowski]

On 26/10/2025 07:50, Igor Reznichenko wrote:
> This patch series adds support for the ST Microelectronics TSC1641
> I2C power monitor. The TSC1641 provides bus voltage, current, power,
> and temperature measurements via the hwmon subsystem. The driver 
> supports optional ALERT pin polarity configuration and exposes the
> shunt resistor value and update interval via sysfs.
> 
> Tested on Raspberry Pi 3B+ with a TSC1641 evaluation board.


Do not attach (thread) your patchsets to some other threads (unrelated
or older versions). This buries them deep in the mailbox and might
interfere with applying entire sets. See also:
https://elixir.bootlin.com/linux/v6.16-rc2/source/Documentation/process/submitting-patches.rst#L830

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add support for ST TSC1641 power monitor

[Krzysztof Kozlowski]

On 26/10/2025 07:50, Igor Reznichenko wrote:
> +properties:
> +  compatible:
> +    const: st,tsc1641

Subject: I asked to drop "binding" and not add "support for". "Support
for" makes little sense in terms of binding. How binding can support
anything? This is the "ST TSC1641 power monitor" not support.

> +
> +  reg:
> +    maxItems: 1
> +
> +  shunt-resistor-micro-ohms:
> +    description: Shunt resistor value in micro-ohms. Since device has internal
> +      16-bit RSHUNT register with 10 uOhm LSB, the maximum value is capped at
> +      655.35 mOhm.
> +    minimum: 100
> +    default: 1000
> +    maximum: 655350
> +
> +  st,alert-polarity-active-high:

Isn't this just interrupt? You need proper interrupts property and then
its flag define the type of interrupt.


Best regards,
Krzysztof

Re: [PATCH v2 2/2] hwmon: Add TSC1641 I2C power monitor driver

[Guenter Roeck]

On 10/25/25 23:50, Igor Reznichenko wrote:
> Add a driver for the ST Microelectronics TSC1641 16-bit high-precision
> power monitor. The driver supports reading bus voltage, current, power,
> and temperature. Sysfs attributes are exposed for shunt resistor and
> update interval. The driver integrates with the hwmon subsystem and
> supports optional ALERT pin polarity configuration.
> 
> Signed-off-by: Igor Reznichenko <igor@reznichenko.net>
> ---
>   Documentation/hwmon/index.rst   |   1 +
>   Documentation/hwmon/tsc1641.rst |  84 ++++
>   drivers/hwmon/Kconfig           |  12 +
>   drivers/hwmon/Makefile          |   1 +
>   drivers/hwmon/tsc1641.c         | 703 ++++++++++++++++++++++++++++++++
>   5 files changed, 801 insertions(+)
>   create mode 100644 Documentation/hwmon/tsc1641.rst
>   create mode 100644 drivers/hwmon/tsc1641.c
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index 51a5bdf75b08..4fb9f91f83b3 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -253,6 +253,7 @@ Hardware Monitoring Kernel Drivers
>      tps40422
>      tps53679
>      tps546d24
> +   tsc1641
>      twl4030-madc-hwmon
>      ucd9000
>      ucd9200
> diff --git a/Documentation/hwmon/tsc1641.rst b/Documentation/hwmon/tsc1641.rst
> new file mode 100644
> index 000000000000..f692a8ccbffc
> --- /dev/null
> +++ b/Documentation/hwmon/tsc1641.rst
> @@ -0,0 +1,84 @@
> +Kernel driver tsc1641

checkpatch:

WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#146: FILE: Documentation/hwmon/tsc1641.rst:1:
+Kernel driver tsc1641

> +=====================
> +
> +Supported chips:
> +
> +  * ST TSC1641
> +
> +    Prefix: 'tsc1641'
> +
> +    Addresses scanned: -
> +
> +    Datasheet:
> +	https://www.st.com/resource/en/datasheet/tsc1641.pdf
> +
> +Author:
> +	- Igor Reznichenko <igor@reznichenko.net>
> +
> +
> +Description
> +-----------
> +
> +The TSC1641 is a high-precision current, voltage, power, and temperature
> +monitoring analog front-end (AFE). It monitors current into a shunt resistor and
> +load voltage up to 60 V in a synchronized way. Digital bus interface is
> +I2C/SMbus. The TSC1641 allows the assertion of several alerts regarding the
> +voltage, current, power and temperature.
> +
> +Usage Notes
> +-----------
> +
> +The TSC1641 driver requires the value of the external shunt resistor to
> +correctly compute current and power measurements. The resistor value, in
> +micro-ohms, should be provided either through the device tree property
> +"shunt-resistor-micro-ohms" or via writable sysfs attribute "shunt_resistor".
> +Please refer to the Documentation/devicetree/bindings/hwmon/st,tsc1641.yaml
> +for bindings if the device tree is used.
> +
> +Supported range of shunt resistor values is from 100 uOhm to 655.35 mOhm.
> +When selecting the value keep in mind device maximum DC power measurement is
> +1600W. See datasheet p.22 for ST recommendations on selecting shunt value.
> +
> +If the shunt resistor value is not specified in the device tree, the driver
> +initializes it to 1000 uOhm by default. Users may configure the correct shunt
> +resistor value at runtime by writing to the "shunt_resistor" sysfs attribute.
> +
> +The driver only supports continuous operating mode.
> +Measurement ranges:
> +
> +================ ===============================================================
> +Current          Dependent on shunt
> +Bus voltage      0-60V
> +Maximum DC power 1600W
> +Temperature      -40C to +125C
> +================ ===============================================================
> +
> +Sysfs entries
> +-------------
> +
> +==================== ===========================================================
> +in0_input            bus voltage (mV)
> +in0_crit             bus voltage crit alarm limit (mV)
> +in0_crit_alarm       bus voltage crit alarm limit exceeded
> +in0_lcrit            bus voltage low-crit alarm limit (mV)
> +in0_lcrit_alarm      bus voltage low-crit alarm limit exceeded
> +
> +curr1_input          current measurement (mA)
> +curr1_crit           current crit alarm limit (mA)
> +curr1_crit_alarm     current crit alarm limit exceeded
> +curr1_lcrit          current low-crit alarm limit (mA)
> +curr1_lcrit_alarm    current low-crit alarm limit exceeded
> +
> +power1_input         power measurement (uW)
> +power1_crit          power crit alarm limit (uW)
> +power1_crit_alarm    power crit alarm limit exceeded
> +
> +shunt_resistor       shunt resistor value (uOhms)
> +
> +temp1_input          temperature measurement (mdegC)
> +temp1_crit           temperature crit alarm limit (mdegC)
> +temp1_crit_alarm     temperature crit alarm limit exceeded
> +
> +update_interval      data conversion time (1 - 33ms), longer conversion time
> +                     corresponds to higher effective resolution in bits
> +==================== ===========================================================
> \ No newline at end of file
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 2760feb9f83b..b9d7b02932a6 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2434,6 +2434,18 @@ config SENSORS_TMP513
>   	  This driver can also be built as a module. If so, the module
>   	  will be called tmp513.
>   
> +config SENSORS_TSC1641
> +	tristate "ST Microelectronics TSC1641 Power Monitor"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  If you say yes here you get support for TSC1641 power  monitor chip.
> +	  The TSC1641 driver is configured for the default configuration of
> +	  the part except temperature is enabled by default.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called tsc1641.
> +
>   config SENSORS_VEXPRESS
>   	tristate "Versatile Express"
>   	depends on VEXPRESS_CONFIG
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 73b2abdcc6dd..a8de5bc69f2a 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -233,6 +233,7 @@ obj-$(CONFIG_SENSORS_TMP401)	+= tmp401.o
>   obj-$(CONFIG_SENSORS_TMP421)	+= tmp421.o
>   obj-$(CONFIG_SENSORS_TMP464)	+= tmp464.o
>   obj-$(CONFIG_SENSORS_TMP513)	+= tmp513.o
> +obj-$(CONFIG_SENSORS_TSC1641)	+= tsc1641.o
>   obj-$(CONFIG_SENSORS_VEXPRESS)	+= vexpress-hwmon.o
>   obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o
>   obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
> diff --git a/drivers/hwmon/tsc1641.c b/drivers/hwmon/tsc1641.c
> new file mode 100644
> index 000000000000..56f6d0ba2b49
> --- /dev/null
> +++ b/drivers/hwmon/tsc1641.c
> @@ -0,0 +1,703 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for ST Microelectronics TSC1641 I2C power monitor
> + *
> + * 60 V, 16-bit high-precision power monitor with I2C and MIPI I3C interface
> + * Datasheet: https://www.st.com/resource/en/datasheet/tsc1641.pdf
> + *
> + * Copyright (C) 2025 Igor Reznichenko <igor@reznichenko.net>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/sysfs.h>

Using find_closest() requires including linux/util_macros.h.

> +
> +/* I2C registers */
> +#define TSC1641_CONFIG		0x00
> +#define TSC1641_SHUNT_VOLTAGE	0x01
> +#define TSC1641_LOAD_VOLTAGE	0x02
> +#define TSC1641_POWER		0x03
> +#define TSC1641_CURRENT		0x04
> +#define TSC1641_TEMP		0x05
> +#define TSC1641_MASK		0x06
> +#define TSC1641_FLAG		0x07
> +#define TSC1641_RSHUNT		0x08 /* Shunt resistance */
> +#define TSC1641_SOL		0x09
> +#define TSC1641_SUL		0x0A
> +#define TSC1641_LOL		0x0B
> +#define TSC1641_LUL		0x0C
> +#define TSC1641_POL		0x0D
> +#define TSC1641_TOL		0x0E
> +#define TSC1641_MANUF_ID	0xFE /* 0x0006 */
> +#define TSC1641_DIE_ID		0xFF /* 0x1000 */
> +#define TSC1641_MAX_REG		0xFF
> +
> +#define TSC1641_RSHUNT_DEFAULT	1000   /* 1mOhm */
> +#define TSC1641_CONFIG_DEFAULT	0x003F /* Enable temperature sensor */
> +#define TSC1641_MASK_DEFAULT	0xFC00 /* Unmask all alerts */
> +
> +/* Bit mask for conversion time in the configuration register */
> +#define TSC1641_CONV_TIME_MASK	GENMASK(7, 4)
> +
> +#define TSC1641_CONV_TIME_DEFAULT	1024
> +#define TSC1641_MIN_UPDATE_INTERVAL	1024
> +
> +/* LSB value of different registers */
> +#define TSC1641_VLOAD_LSB_MVOLT		2
> +#define TSC1641_POWER_LSB_UWATT		25000
> +#define TSC1641_VSHUNT_LSB_NVOLT	2500 /* Use nanovolts to make it integer */
> +#define TSC1641_RSHUNT_LSB_UOHM		10
> +#define TSC1641_TEMP_LSB_MDEGC		500
> +
> +/* Limits based on datasheet */
> +#define TSC1641_RSHUNT_MIN_UOHM		100
> +#define TSC1641_RSHUNT_MAX_UOHM		655350
> +#define TSC1641_VLOAD_MAX_MVOLT		60000
> +#define TSC1641_CURRENT_MIN_MAMP	(-819175)
> +#define TSC1641_CURRENT_MAX_MAMP	819175
> +#define TSC1641_TEMP_MIN_MDEGC		(-20000)

Why -20000 ? The chip limit is -40 degrees C.

> +#define TSC1641_TEMP_MAX_MDEGC		145000
> +#define TSC1641_POWER_MAX_UWATT		1600000000
> +
> +#define TSC1641_ALERT_POL_MASK		BIT(1)
> +#define TSC1641_ALERT_LATCH_EN_MASK	BIT(0)
> +
> +/* Flags indicating alerts in TSC1641_FLAG register*/
> +#define TSC1641_SHUNT_OV_FLAG		BIT(6)
> +#define TSC1641_SHUNT_UV_FLAG		BIT(5)
> +#define TSC1641_LOAD_OV_FLAG		BIT(4)
> +#define TSC1641_LOAD_UV_FLAG		BIT(3)
> +#define TSC1641_POWER_OVER_FLAG		BIT(2)
> +#define TSC1641_TEMP_OVER_FLAG		BIT(1)
> +
> +static bool tsc1641_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case TSC1641_CONFIG:
> +	case TSC1641_MASK:
> +	case TSC1641_RSHUNT:
> +	case TSC1641_SOL:
> +	case TSC1641_SUL:
> +	case TSC1641_LOL:
> +	case TSC1641_LUL:
> +	case TSC1641_POL:
> +	case TSC1641_TOL:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool tsc1641_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case TSC1641_SHUNT_VOLTAGE:
> +	case TSC1641_LOAD_VOLTAGE:
> +	case TSC1641_POWER:
> +	case TSC1641_CURRENT:
> +	case TSC1641_TEMP:
> +	case TSC1641_FLAG:
> +	case TSC1641_MANUF_ID:
> +	case TSC1641_DIE_ID:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct regmap_config tsc1641_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.use_single_write = true,
> +	.use_single_read = true,
> +	.max_register = TSC1641_MAX_REG,
> +	.cache_type = REGCACHE_MAPLE,
> +	.volatile_reg = tsc1641_volatile_reg,
> +	.writeable_reg = tsc1641_writeable_reg,
> +};
> +
> +struct tsc1641_data {
> +	long rshunt_uohm;
> +	long current_lsb_ua;
> +	struct regmap *regmap;
> +	struct i2c_client *client;

client is not used anywhere and can be dropped.

> +};
> +
> +/*
> + * Upper limit due to chip 16-bit shunt register, lower limit to
> + * prevent current and power registers overflow
> + */
> +static inline int tsc1641_validate_shunt(u32 val)
> +{
> +	if (val < TSC1641_RSHUNT_MIN_UOHM || val > TSC1641_RSHUNT_MAX_UOHM)
> +		return -EINVAL;

In some way this is inconsistent: It accepts a shunt resistor value of, say, 105
even though the chip can only accept multiples of 10 uOhm. In situations like this
I suggest to expect devicetree values to be accurate and to clamp values entered
through sysfs. More on that below.

> +	return 0;
> +}
> +
> +static int tsc1641_set_shunt(struct tsc1641_data *data, u32 val)
> +{
> +	struct regmap *regmap = data->regmap;
> +	long rshunt_reg;
> +
> +	if (tsc1641_validate_shunt(val) < 0)
> +		return -EINVAL;
> +
> +	data->rshunt_uohm = val;
> +	data->current_lsb_ua = DIV_ROUND_CLOSEST(TSC1641_VSHUNT_LSB_NVOLT * 1000,
> +						 data->rshunt_uohm);
> +	/* RSHUNT register LSB is 10uOhm so need to divide further*/
> +	rshunt_reg = DIV_ROUND_CLOSEST(data->rshunt_uohm, TSC1641_RSHUNT_LSB_UOHM);

This means that all calculations do not use the actual shunt resistor values used
by the chip, but an approximation. I would suggest to store and use the actual shunt
resistor value instead, not the one entered by the user.

> +	return regmap_write(regmap, TSC1641_RSHUNT, clamp_val(rshunt_reg, 0, USHRT_MAX));

The shunt resistor value is already validated, so the additional clamp here is
unnecessary.

> +}
> +
> +/*
> + * Conversion times in uS, value in CONFIG[CT3:CT0] corresponds to index in this array
> + * See "Table 14. CT3 to CT0: conversion time" in:
> + * https://www.st.com/resource/en/datasheet/tsc1641.pdf
> + */
> +static const int tsc1641_conv_times[] = { 128, 256, 512, 1024, 2048, 4096, 8192, 16384, 32768 };
> +
> +static int tsc1641_reg_to_upd_interval(u16 config)
> +{
> +	int idx = FIELD_GET(TSC1641_CONV_TIME_MASK, config);
> +
> +	idx = clamp_val(idx, 0, ARRAY_SIZE(tsc1641_conv_times) - 1);
> +	int conv_time = tsc1641_conv_times[idx];
> +
> +	/* Don't support sub-millisecond update interval as it's not supported in hwmon */
> +	conv_time = max(conv_time, TSC1641_MIN_UPDATE_INTERVAL);
> +	/* Return nearest value in milliseconds */
> +	return DIV_ROUND_CLOSEST(conv_time, 1000);
> +}
> +
> +static u16 tsc1641_upd_interval_to_reg(long interval)
> +{
> +	/* Supported interval is 1ms - 33ms */
> +	interval = clamp_val(interval, 1, 33);
> +
> +	int conv = interval * 1000;
> +	int conv_bits = find_closest(conv, tsc1641_conv_times,
> +				     ARRAY_SIZE(tsc1641_conv_times));
> +
> +	return FIELD_PREP(TSC1641_CONV_TIME_MASK, conv_bits);
> +}
> +
> +static int tsc1641_chip_write(struct device *dev, u32 attr, long val)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_chip_update_interval:
> +		return regmap_update_bits(data->regmap, TSC1641_CONFIG,
> +					  TSC1641_CONV_TIME_MASK,
> +					  tsc1641_upd_interval_to_reg(val));
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int tsc1641_chip_read(struct device *dev, u32 attr, long *val)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +	u32 regval;
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_chip_update_interval:
> +		ret = regmap_read(data->regmap, TSC1641_CONFIG, &regval);
> +		if (ret)
> +			return ret;
> +
> +		*val = tsc1641_reg_to_upd_interval(regval);
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int tsc1641_alert_read(struct regmap *regmap, u32 flag, long *val)
> +{
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read_bypassed(regmap, TSC1641_FLAG, &regval);
> +	if (ret)
> +		return ret;
> +
> +	*val = !!(regval & flag);
> +	return 0;
> +}
> +
> +static int tsc1641_in_read(struct device *dev, u32 attr, long *val)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +	unsigned int regval;
> +	int ret, reg;
> +
> +	switch (attr) {
> +	case hwmon_in_input:
> +		reg = TSC1641_LOAD_VOLTAGE;
> +		break;
> +	case hwmon_in_lcrit:
> +		reg = TSC1641_LUL;
> +		break;
> +	case hwmon_in_crit:
> +		reg = TSC1641_LOL;
> +		break;
> +	case hwmon_in_lcrit_alarm:
> +		return tsc1641_alert_read(regmap, TSC1641_LOAD_UV_FLAG, val);
> +	case hwmon_in_crit_alarm:
> +		return tsc1641_alert_read(regmap, TSC1641_LOAD_OV_FLAG, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ret = regmap_read(regmap, reg, &regval);
> +	if (ret)
> +		return ret;
> +
> +	*val = regval * TSC1641_VLOAD_LSB_MVOLT;

This applies to many of the registers:

if regval == 0x0000 or 0x7fff, and the SATF bit is set in the status register,
the voltage is out of range. This should be checked and -ENODATA should be
returned if it happens. Also, apparently, the register is only 15 bit wide and
should never have bit 15 set.

> +	return 0;
> +}
> +
> +static int tsc1641_curr_read(struct device *dev, u32 attr, long *val)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +	int regval;
> +	int ret, reg;
> +
> +	/* Current limits are the shunt under/over voltage limits */
> +	switch (attr) {
> +	case hwmon_curr_input:
> +		reg = TSC1641_CURRENT;
> +		break;
> +	case hwmon_curr_lcrit:
> +		reg = TSC1641_SUL;
> +		break;
> +	case hwmon_curr_crit:
> +		reg = TSC1641_SOL;
> +		break;
> +	case hwmon_curr_lcrit_alarm:
> +		return tsc1641_alert_read(regmap, TSC1641_SHUNT_UV_FLAG, val);
> +	case hwmon_curr_crit_alarm:
> +		return tsc1641_alert_read(regmap, TSC1641_SHUNT_OV_FLAG, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ret = regmap_read(regmap, reg, &regval);
> +	if (ret)
> +		return ret;
> +
> +	/* Current in milliamps */
> +	*val = DIV_ROUND_CLOSEST((s16)regval * data->current_lsb_ua, 1000);
> +	return 0;
> +}
> +
> +static int tsc1641_power_read(struct device *dev, u32 attr, long *val)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +	unsigned int regval;
> +	int ret, reg;
> +
> +	switch (attr) {
> +	case hwmon_power_input:
> +		reg = TSC1641_POWER;
> +		break;
> +	case hwmon_power_crit:
> +		reg = TSC1641_POL;
> +		break;
> +	case hwmon_power_crit_alarm:
> +		return tsc1641_alert_read(regmap, TSC1641_POWER_OVER_FLAG, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ret = regmap_read(regmap, reg, &regval);
> +	if (ret)
> +		return ret;
> +
> +	*val = regval * TSC1641_POWER_LSB_UWATT;
> +	return 0;
> +}
> +
> +static int tsc1641_temp_read(struct device *dev, u32 attr, long *val)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +	unsigned int regval;
> +	int ret, reg;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		reg = TSC1641_TEMP;
> +		break;
> +	case hwmon_temp_crit:
> +		reg = TSC1641_TOL;
> +		break;
> +	case hwmon_temp_crit_alarm:
> +		return tsc1641_alert_read(regmap, TSC1641_TEMP_OVER_FLAG, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	ret = regmap_read(regmap, reg, &regval);
> +	if (ret)
> +		return ret;
> +
> +	*val = (s16)regval * TSC1641_TEMP_LSB_MDEGC;
> +	return 0;
> +}
> +
> +static int tsc1641_in_write(struct device *dev, u32 attr, long val)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +	unsigned int regval;
> +	int reg;
> +
> +	switch (attr) {
> +	case hwmon_in_lcrit:
> +		reg = TSC1641_LUL;
> +		break;
> +	case hwmon_in_crit:
> +		reg = TSC1641_LOL;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	val = clamp_val(val, 0, TSC1641_VLOAD_MAX_MVOLT);
> +	regval = DIV_ROUND_CLOSEST(val, TSC1641_VLOAD_LSB_MVOLT);
> +
> +	return regmap_write(regmap, reg, clamp_val(regval, 0, USHRT_MAX));

Another unnecessary clamp. Please only clamp when necessary.

Also, I notice that the above limits the value range to [0, 60000],
and the register value to [0, 30000].

According to the datasheet, the chip should accept the complete register
range from 0 to 0xffff, or 131,070 mV, as limit values. That means it is
possible for someone to write 0xffff into a register which would then be
reported as limit when reading it, but writing that limit back would
actually change it. I recommend against doing that.

[ Yes, I know, a voltage above 60V might damage the chip, but that
   doesn't mean that accepting higher limit values should be rejected.
   Some BIOS / ROMMON vendor could decide to write a limit value of 0xffff
   to indicate no limit. It is not our business to reject that.
]

The same applies to all other limit registers as far as I can see.

> +}
> +
> +static int tsc1641_curr_write(struct device *dev, u32 attr, long val)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +	int reg, regval;
> +
> +	switch (attr) {
> +	case hwmon_curr_lcrit:
> +		reg = TSC1641_SUL;
> +		break;
> +	case hwmon_curr_crit:
> +		reg = TSC1641_SOL;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	/* Clamp to max 16-bit represantable current at min Rshunt */
> +	val = clamp_val(val, TSC1641_CURRENT_MIN_MAMP, TSC1641_CURRENT_MAX_MAMP);
> +	/* Convert val in milliamps to voltage */
> +	regval = DIV_ROUND_CLOSEST(val * data->rshunt_uohm, TSC1641_VSHUNT_LSB_NVOLT);
> +
> +	return regmap_write(regmap, reg, clamp_val(regval, SHRT_MIN, SHRT_MAX));

See below - clamping is insufficient for negative values, and it is not clear to me if
the limit register is signed or unsigned.

> +}
> +
> +static int tsc1641_power_write(struct device *dev, u32 attr, long val)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +	unsigned int regval;
> +
> +	switch (attr) {
> +	case hwmon_power_crit:
> +		val = clamp_val(val, 0, TSC1641_POWER_MAX_UWATT);
> +		regval = DIV_ROUND_CLOSEST(val, TSC1641_POWER_LSB_UWATT);
> +		return regmap_write(regmap, TSC1641_POL, clamp_val(regval, 0, USHRT_MAX));

regval is already guaranteed to be <= 64000, so the additional clamp here is unencessary.

> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int tsc1641_temp_write(struct device *dev, u32 attr, long val)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +	struct regmap *regmap = data->regmap;
> +	int regval;
> +
> +	switch (attr) {
> +	case hwmon_temp_crit:
> +		val = clamp_val(val, TSC1641_TEMP_MIN_MDEGC, TSC1641_TEMP_MAX_MDEGC);
> +		regval = DIV_ROUND_CLOSEST(val, TSC1641_TEMP_LSB_MDEGC);
> +		return regmap_write(regmap, TSC1641_TOL, clamp_val(regval, SHRT_MIN, SHRT_MAX));

This doesn't work as intended for negative values. regmap doesn't expect to see
negative register values and returns an error if trying to write one, so clamping
against SHRT_MIN and SHRT_MAX is insufficient. You also need to mask the result
against 0xffff.

Also, the datasheet doesn't say that the limit value would be signed. Did you verify
that negative temperature limit values are actually treated as negative values ?

> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static umode_t tsc1641_is_visible(const void *data, enum hwmon_sensor_types type,
> +				  u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_chip:
> +		switch (attr) {
> +		case hwmon_chip_update_interval:
> +			return 0644;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +			return 0444;
> +		case hwmon_in_lcrit:
> +		case hwmon_in_crit:
> +			return 0644;
> +		case hwmon_in_lcrit_alarm:
> +		case hwmon_in_crit_alarm:
> +			return 0444;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_input:
> +			return 0444;
> +		case hwmon_curr_lcrit:
> +		case hwmon_curr_crit:
> +			return 0644;
> +		case hwmon_curr_lcrit_alarm:
> +		case hwmon_curr_crit_alarm:
> +			return 0444;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_power:
> +		switch (attr) {
> +		case hwmon_power_input:
> +			return 0444;
> +		case hwmon_power_crit:
> +			return 0644;
> +		case hwmon_power_crit_alarm:
> +			return 0444;
> +		default:
> +			break;
> +		}
> +		break;
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_input:
> +			return 0444;
> +		case hwmon_temp_crit:
> +			return 0644;
> +		case hwmon_temp_crit_alarm:
> +			return 0444;
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static int tsc1641_read(struct device *dev, enum hwmon_sensor_types type,
> +			u32 attr, int channel, long *val)
> +{
> +	switch (type) {
> +	case hwmon_chip:
> +		return tsc1641_chip_read(dev, attr, val);
> +	case hwmon_in:
> +		return tsc1641_in_read(dev, attr, val);
> +	case hwmon_curr:
> +		return tsc1641_curr_read(dev, attr, val);
> +	case hwmon_power:
> +		return tsc1641_power_read(dev, attr, val);
> +	case hwmon_temp:
> +		return tsc1641_temp_read(dev, attr, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int tsc1641_write(struct device *dev, enum hwmon_sensor_types type,
> +			 u32 attr, int channel, long val)
> +{
> +	switch (type) {
> +	case hwmon_chip:
> +		return tsc1641_chip_write(dev, attr, val);
> +	case hwmon_in:
> +		return tsc1641_in_write(dev, attr, val);
> +	case hwmon_curr:
> +		return tsc1641_curr_write(dev, attr, val);
> +	case hwmon_power:
> +		return tsc1641_power_write(dev, attr, val);
> +	case hwmon_temp:
> +		return tsc1641_temp_write(dev, attr, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static const struct hwmon_channel_info * const tsc1641_info[] = {
> +	HWMON_CHANNEL_INFO(chip,
> +			   HWMON_C_UPDATE_INTERVAL),
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT | HWMON_I_CRIT | HWMON_I_CRIT_ALARM |
> +			   HWMON_I_LCRIT | HWMON_I_LCRIT_ALARM),

Why did you choose lcrit/crit attributes instead of min/max ? If there is only
one alert limit, that usually means the first level of alert, not a critical level.
Raising an alert does not mean it is a critical alert. Please reconsider.

> +	HWMON_CHANNEL_INFO(curr,
> +			   HWMON_C_INPUT | HWMON_C_CRIT | HWMON_C_CRIT_ALARM |
> +			   HWMON_C_LCRIT | HWMON_C_LCRIT_ALARM),
> +	HWMON_CHANNEL_INFO(power,
> +			   HWMON_P_INPUT | HWMON_P_CRIT | HWMON_P_CRIT_ALARM),
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT | HWMON_T_CRIT | HWMON_T_CRIT_ALARM),
> +	NULL
> +};
> +
> +static ssize_t shunt_resistor_show(struct device *dev,
> +				   struct device_attribute *da, char *buf)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%li\n", data->rshunt_uohm);
> +}
> +
> +static ssize_t shunt_resistor_store(struct device *dev,
> +				    struct device_attribute *da,
> +				    const char *buf, size_t count)
> +{
> +	struct tsc1641_data *data = dev_get_drvdata(dev);
> +	unsigned long val;
> +	int ret;
> +
> +	ret = kstrtoul(buf, 10, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (val > U32_MAX)
> +		return -EINVAL;
> +

Use kstrtouint() instead.

> +	ret = tsc1641_set_shunt(data, (u32)val);
> +	if (ret < 0)
> +		return ret;
> +	return count;
> +}
> +
> +static const struct hwmon_ops tsc1641_hwmon_ops = {
> +	.is_visible = tsc1641_is_visible,
> +	.read = tsc1641_read,
> +	.write = tsc1641_write,
> +};
> +
> +static const struct hwmon_chip_info tsc1641_chip_info = {
> +	.ops = &tsc1641_hwmon_ops,
> +	.info = tsc1641_info,
> +};
> +
> +static DEVICE_ATTR_RW(shunt_resistor);
> +
> +/* Shunt resistor value is exposed via sysfs attribute */
> +static struct attribute *tsc1641_attrs[] = {
> +	&dev_attr_shunt_resistor.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(tsc1641);
> +
> +static int tsc1641_init(struct device *dev, struct tsc1641_data *data)
> +{
> +	struct regmap *regmap = data->regmap;
> +	bool active_high;
> +	u32 shunt;
> +	int ret;
> +
> +	if (device_property_read_u32(dev, "shunt-resistor-micro-ohms", &shunt) < 0)
> +		shunt = TSC1641_RSHUNT_DEFAULT;
> +
> +	if (tsc1641_validate_shunt(shunt) < 0) {
> +		dev_err(dev, "invalid shunt resistor value %u\n", shunt);
> +		return -EINVAL;
> +	}
> +
> +	ret = tsc1641_set_shunt(data, shunt);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(regmap, TSC1641_CONFIG, TSC1641_CONFIG_DEFAULT);
> +	if (ret < 0)
> +		return ret;
> +
> +	active_high = device_property_read_bool(dev, "st,alert-polarity-active-high");
> +
> +	return regmap_write(regmap, TSC1641_MASK, TSC1641_MASK_DEFAULT |
> +			    FIELD_PREP(TSC1641_ALERT_POL_MASK, active_high));
> +}
> +
> +static int tsc1641_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct tsc1641_data *data;
> +	struct device *hwmon_dev;
> +	int ret;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +
> +	data->regmap = devm_regmap_init_i2c(client, &tsc1641_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		dev_err(dev, "failed to allocate register map\n");
> +		return PTR_ERR(data->regmap);

Use dev_err_probe() here as well.

> +	}
> +
> +	ret = tsc1641_init(dev, data);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to configure device\n");
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
> +							 data, &tsc1641_chip_info, tsc1641_groups);
> +	if (IS_ERR(hwmon_dev))
> +		return PTR_ERR(hwmon_dev);
> +
> +	dev_info(dev, "power monitor %s (Rshunt = %li uOhm)\n",
> +		 client->name, data->rshunt_uohm);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id tsc1641_id[] = {
> +	{ "tsc1641", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, tsc1641_id);
> +
> +static const struct of_device_id __maybe_unused tsc1641_of_match[] = {
> +	{ .compatible = "st,tsc1641" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, tsc1641_of_match);
> +
> +static struct i2c_driver tsc1641_driver = {
> +	.driver = {
> +		.name = "tsc1641",
> +		.of_match_table = of_match_ptr(tsc1641_of_match),
> +	},
> +	.probe = tsc1641_probe,
> +	.id_table = tsc1641_id,
> +};
> +
> +module_i2c_driver(tsc1641_driver);
> +
> +MODULE_AUTHOR("Igor Reznichenko <igor@reznichenko.net>");
> +MODULE_DESCRIPTION("tsc1641 driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add support for ST TSC1641 power monitor

[Guenter Roeck]

On 10/26/25 09:32, Krzysztof Kozlowski wrote:
> On 26/10/2025 07:50, Igor Reznichenko wrote:
>> +properties:
>> +  compatible:
>> +    const: st,tsc1641
> 
> Subject: I asked to drop "binding" and not add "support for". "Support
> for" makes little sense in terms of binding. How binding can support
> anything? This is the "ST TSC1641 power monitor" not support.
> 
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  shunt-resistor-micro-ohms:
>> +    description: Shunt resistor value in micro-ohms. Since device has internal
>> +      16-bit RSHUNT register with 10 uOhm LSB, the maximum value is capped at
>> +      655.35 mOhm.
>> +    minimum: 100
>> +    default: 1000
>> +    maximum: 655350
>> +
>> +  st,alert-polarity-active-high:
> 
> Isn't this just interrupt? You need proper interrupts property and then
> its flag define the type of interrupt.
> 

This is a value to write into the chip. It is orthogonal to how the interrupt
is reported to the interrupt controller. It may be active low by the chip and
inverted, or it may be active high by the chip and inverted. How does one express
an additional inverter in the interrupt signal path in a devicetree property ?
Can you give an example ?

Thanks,
Guenter

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add support for ST TSC1641 power monitor

[Igor Reznichenko]

> Subject: I asked to drop "binding" and not add "support for". "Support
> for" makes little sense in terms of binding. How binding can support
> anything? This is the "ST TSC1641 power monitor" not support.

Krzysztof,

Thanks for feedback, will fix this and will create following patch versions
in new threads.

>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  shunt-resistor-micro-ohms:
>> +    description: Shunt resistor value in micro-ohms. Since device has internal
>> +      16-bit RSHUNT register with 10 uOhm LSB, the maximum value is capped at
>> +      655.35 mOhm.
>> +    minimum: 100
>> +    default: 1000
>> +    maximum: 655350
>> +
>> +  st,alert-polarity-active-high:
>
>Isn't this just interrupt? You need proper interrupts property and then
>its flag define the type of interrupt.

This controls a bit written into device register.
I omitted interrupt property after looking at existing power monitor bindings,
especially hwmon/ti,ina2xx.yaml. INA226 has very similar bit controlling alert 
pin polarity and binding doesn't define alert pin as interrupt. Overall, I didn't
find many power monitor bindings defining alert pins as interrupts.

Thanks, Igor

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add support for ST TSC1641 power monitor

[Krzysztof Kozlowski]

On 26/10/2025 18:22, Guenter Roeck wrote:
> On 10/26/25 09:32, Krzysztof Kozlowski wrote:
>> On 26/10/2025 07:50, Igor Reznichenko wrote:
>>> +properties:
>>> +  compatible:
>>> +    const: st,tsc1641
>>
>> Subject: I asked to drop "binding" and not add "support for". "Support
>> for" makes little sense in terms of binding. How binding can support
>> anything? This is the "ST TSC1641 power monitor" not support.
>>
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  shunt-resistor-micro-ohms:
>>> +    description: Shunt resistor value in micro-ohms. Since device has internal
>>> +      16-bit RSHUNT register with 10 uOhm LSB, the maximum value is capped at
>>> +      655.35 mOhm.
>>> +    minimum: 100
>>> +    default: 1000
>>> +    maximum: 655350
>>> +
>>> +  st,alert-polarity-active-high:
>>
>> Isn't this just interrupt? You need proper interrupts property and then
>> its flag define the type of interrupt.
>>
> 
> This is a value to write into the chip. It is orthogonal to how the interrupt
> is reported to the interrupt controller. It may be active low by the chip and
> inverted, or it may be active high by the chip and inverted. How does one express
> an additional inverter in the interrupt signal path in a devicetree property ?
> Can you give an example ?


If that is the interrupt to the CPU, then it's just like I said - proper
flag to the interrupts property. There is no need to express inverter
separately from the interrupts, because that would mean you first
express interrupts incorrectly and then you add inverter to make it
correct. Just like people expressing RESET_N GPIO with ACTIVE_HIGH and
then making reversed set high/low in the driver :/

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add support for ST TSC1641 power monitor

[Krzysztof Kozlowski]

On 26/10/2025 19:46, Igor Reznichenko wrote:
>> Subject: I asked to drop "binding" and not add "support for". "Support
>> for" makes little sense in terms of binding. How binding can support
>> anything? This is the "ST TSC1641 power monitor" not support.
> 
> Krzysztof,
> 
> Thanks for feedback, will fix this and will create following patch versions
> in new threads.
> 
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  shunt-resistor-micro-ohms:
>>> +    description: Shunt resistor value in micro-ohms. Since device has internal
>>> +      16-bit RSHUNT register with 10 uOhm LSB, the maximum value is capped at
>>> +      655.35 mOhm.
>>> +    minimum: 100
>>> +    default: 1000
>>> +    maximum: 655350
>>> +
>>> +  st,alert-polarity-active-high:
>>
>> Isn't this just interrupt? You need proper interrupts property and then
>> its flag define the type of interrupt.
> 
> This controls a bit written into device register.
> I omitted interrupt property after looking at existing power monitor bindings,
> especially hwmon/ti,ina2xx.yaml. INA226 has very similar bit controlling alert 
> pin polarity and binding doesn't define alert pin as interrupt. Overall, I didn't
> find many power monitor bindings defining alert pins as interrupts.


On INA2xx that's SMBUS Alert. Is this the case here as well?

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add support for ST TSC1641 power monitor

[Guenter Roeck]

On 10/26/25 12:41, Krzysztof Kozlowski wrote:
> On 26/10/2025 19:46, Igor Reznichenko wrote:
>>> Subject: I asked to drop "binding" and not add "support for". "Support
>>> for" makes little sense in terms of binding. How binding can support
>>> anything? This is the "ST TSC1641 power monitor" not support.
>>
>> Krzysztof,
>>
>> Thanks for feedback, will fix this and will create following patch versions
>> in new threads.
>>
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  shunt-resistor-micro-ohms:
>>>> +    description: Shunt resistor value in micro-ohms. Since device has internal
>>>> +      16-bit RSHUNT register with 10 uOhm LSB, the maximum value is capped at
>>>> +      655.35 mOhm.
>>>> +    minimum: 100
>>>> +    default: 1000
>>>> +    maximum: 655350
>>>> +
>>>> +  st,alert-polarity-active-high:
>>>
>>> Isn't this just interrupt? You need proper interrupts property and then
>>> its flag define the type of interrupt.
>>
>> This controls a bit written into device register.
>> I omitted interrupt property after looking at existing power monitor bindings,
>> especially hwmon/ti,ina2xx.yaml. INA226 has very similar bit controlling alert
>> pin polarity and binding doesn't define alert pin as interrupt. Overall, I didn't
>> find many power monitor bindings defining alert pins as interrupts.
> 
> 
> On INA2xx that's SMBUS Alert. Is this the case here as well?
> 

It could be wired to SMBus alert, or it could be wired to a CPU interrupt pin.

Guenter

Re: [PATCH v2 2/2] hwmon: Add TSC1641 I2C power monitor driver

[Igor Reznichenko]

>In some way this is inconsistent: It accepts a shunt resistor value of, say, 105
>even though the chip can only accept multiples of 10 uOhm. In situations like this
>I suggest to expect devicetree values to be accurate and to clamp values entered
>through sysfs. More on that below.
>
>> +	return 0;
>> +}
>> +
>> +static int tsc1641_set_shunt(struct tsc1641_data *data, u32 val)
>> +{
>> +	struct regmap *regmap = data->regmap;
>> +	long rshunt_reg;
>> +
>> +	if (tsc1641_validate_shunt(val) < 0)
>> +		return -EINVAL;
>> +
>> +	data->rshunt_uohm = val;
>> +	data->current_lsb_ua = DIV_ROUND_CLOSEST(TSC1641_VSHUNT_LSB_NVOLT * 1000,
>> +						 data->rshunt_uohm);
>> +	/* RSHUNT register LSB is 10uOhm so need to divide further*/
>> +	rshunt_reg = DIV_ROUND_CLOSEST(data->rshunt_uohm, TSC1641_RSHUNT_LSB_UOHM);
>
>This means that all calculations do not use the actual shunt resistor values used
>by the chip, but an approximation. I would suggest to store and use the actual shunt
>resistor value instead, not the one entered by the user.

By "actual shunt" you mean defined in devicetree? Then does it mean disabling 
writing value by user via sysfs and making "shunt_resistor" read-only or leaving it
writable and clamping to devicetree value, thus discarding the user provided value?

>See below - clamping is insufficient for negative values, and it is not clear to me if
>the limit register is signed or unsigned.

>Also, the datasheet doesn't say that the limit value would be signed. Did you verify
>that negative temperature limit values are actually treated as negative values ?

SUL, SOL, TOL are signed, I verified. The negative limits for current and temperature
work well based on my testing.

>This doesn't work as intended for negative values. regmap doesn't expect to see
>negative register values and returns an error if trying to write one, so clamping
>against SHRT_MIN and SHRT_MAX is insufficient. You also need to mask the result
>against 0xffff.

I was under impression regmap would handle this masking correctly when defining
.val_bits = 16. E.g. in regmap.c:973 it selects formatting function for 16bit values.
I can mask explicitly if it's required.
It certainly doesn't throw error since negative alerts work as mentioned.

>Why did you choose lcrit/crit attributes instead of min/max ? If there is only
>one alert limit, that usually means the first level of alert, not a critical level.
>Raising an alert does not mean it is a critical alert. Please reconsider.

I used hwmon/ina2xx.c as a reference. It covers many similar power monitors which
have single threshold alerts and defines only lcrit/crit. If this is a wrong approach
I'll change to min/max.

The rest of the things are clear, I'll fix those.

Thanks, Igor

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add support for ST TSC1641 power monitor

[Krzysztof Kozlowski]

On 26/10/2025 20:58, Guenter Roeck wrote:
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  shunt-resistor-micro-ohms:
>>>>> +    description: Shunt resistor value in micro-ohms. Since device has internal
>>>>> +      16-bit RSHUNT register with 10 uOhm LSB, the maximum value is capped at
>>>>> +      655.35 mOhm.
>>>>> +    minimum: 100
>>>>> +    default: 1000
>>>>> +    maximum: 655350
>>>>> +
>>>>> +  st,alert-polarity-active-high:
>>>>
>>>> Isn't this just interrupt? You need proper interrupts property and then
>>>> its flag define the type of interrupt.
>>>
>>> This controls a bit written into device register.
>>> I omitted interrupt property after looking at existing power monitor bindings,
>>> especially hwmon/ti,ina2xx.yaml. INA226 has very similar bit controlling alert
>>> pin polarity and binding doesn't define alert pin as interrupt. Overall, I didn't
>>> find many power monitor bindings defining alert pins as interrupts.
>>
>>
>> On INA2xx that's SMBUS Alert. Is this the case here as well?
>>
> 
> It could be wired to SMBus alert, or it could be wired to a CPU interrupt pin.

So please explain me why CPU interrupt pin, which in every really every
device called "interrupts", would not be "interrupts" here? How CPU can
even guess the number of the interrupt in such case, without
"interrupts" property?

Best regards,
Krzysztof

Re: [PATCH v2 2/2] hwmon: Add TSC1641 I2C power monitor driver

[Guenter Roeck]

On 10/26/25 23:41, Igor Reznichenko wrote:
>> In some way this is inconsistent: It accepts a shunt resistor value of, say, 105
>> even though the chip can only accept multiples of 10 uOhm. In situations like this
>> I suggest to expect devicetree values to be accurate and to clamp values entered
>> through sysfs. More on that below.
>>
>>> +	return 0;
>>> +}
>>> +
>>> +static int tsc1641_set_shunt(struct tsc1641_data *data, u32 val)
>>> +{
>>> +	struct regmap *regmap = data->regmap;
>>> +	long rshunt_reg;
>>> +
>>> +	if (tsc1641_validate_shunt(val) < 0)
>>> +		return -EINVAL;
>>> +
>>> +	data->rshunt_uohm = val;
>>> +	data->current_lsb_ua = DIV_ROUND_CLOSEST(TSC1641_VSHUNT_LSB_NVOLT * 1000,
>>> +						 data->rshunt_uohm);
>>> +	/* RSHUNT register LSB is 10uOhm so need to divide further*/
>>> +	rshunt_reg = DIV_ROUND_CLOSEST(data->rshunt_uohm, TSC1641_RSHUNT_LSB_UOHM);
>>
>> This means that all calculations do not use the actual shunt resistor values used
>> by the chip, but an approximation. I would suggest to store and use the actual shunt
>> resistor value instead, not the one entered by the user.
> 
> By "actual shunt" you mean defined in devicetree? Then does it mean disabling
> writing value by user via sysfs and making "shunt_resistor" read-only or leaving it
> writable and clamping to devicetree value, thus discarding the user provided value?
> 

I said "used by the chip", and referred to the value written into TSC1641_RSHUNT_LSB_UOHM.

>> See below - clamping is insufficient for negative values, and it is not clear to me if
>> the limit register is signed or unsigned.
> 
>> Also, the datasheet doesn't say that the limit value would be signed. Did you verify
>> that negative temperature limit values are actually treated as negative values ?
> 
> SUL, SOL, TOL are signed, I verified. The negative limits for current and temperature
> work well based on my testing.
> 

Please add a respective comment into the code.

>> This doesn't work as intended for negative values. regmap doesn't expect to see
>> negative register values and returns an error if trying to write one, so clamping
>> against SHRT_MIN and SHRT_MAX is insufficient. You also need to mask the result
>> against 0xffff.
> 
> I was under impression regmap would handle this masking correctly when defining
> .val_bits = 16. E.g. in regmap.c:973 it selects formatting function for 16bit values.
> I can mask explicitly if it's required.
> It certainly doesn't throw error since negative alerts work as mentioned.
> 

My unit test code bails out on negative values, returning an error from regmap when
trying to write negative values. I had seen that before. Masking the value passed
to regmap with 0xffff solved the problem.

>> Why did you choose lcrit/crit attributes instead of min/max ? If there is only
>> one alert limit, that usually means the first level of alert, not a critical level.
>> Raising an alert does not mean it is a critical alert. Please reconsider.
> 
> I used hwmon/ina2xx.c as a reference. It covers many similar power monitors which
> have single threshold alerts and defines only lcrit/crit. If this is a wrong approach
> I'll change to min/max.

Isn't that great ? You can always find an example for everything in the Linux kernel
if you are looking for it.

Guenter

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add support for ST TSC1641 power monitor

[Guenter Roeck]

On 10/27/25 01:40, Krzysztof Kozlowski wrote:
> On 26/10/2025 20:58, Guenter Roeck wrote:
>>>>>> +  reg:
>>>>>> +    maxItems: 1
>>>>>> +
>>>>>> +  shunt-resistor-micro-ohms:
>>>>>> +    description: Shunt resistor value in micro-ohms. Since device has internal
>>>>>> +      16-bit RSHUNT register with 10 uOhm LSB, the maximum value is capped at
>>>>>> +      655.35 mOhm.
>>>>>> +    minimum: 100
>>>>>> +    default: 1000
>>>>>> +    maximum: 655350
>>>>>> +
>>>>>> +  st,alert-polarity-active-high:
>>>>>
>>>>> Isn't this just interrupt? You need proper interrupts property and then
>>>>> its flag define the type of interrupt.
>>>>
>>>> This controls a bit written into device register.
>>>> I omitted interrupt property after looking at existing power monitor bindings,
>>>> especially hwmon/ti,ina2xx.yaml. INA226 has very similar bit controlling alert
>>>> pin polarity and binding doesn't define alert pin as interrupt. Overall, I didn't
>>>> find many power monitor bindings defining alert pins as interrupts.
>>>
>>>
>>> On INA2xx that's SMBUS Alert. Is this the case here as well?
>>>
>>
>> It could be wired to SMBus alert, or it could be wired to a CPU interrupt pin.
> 
> So please explain me why CPU interrupt pin, which in every really every
> device called "interrupts", would not be "interrupts" here? How CPU can
> even guess the number of the interrupt in such case, without
> "interrupts" property?
> 

I thought we were discussing the need for the st,alert-polarity-active-high
property, sorry.

Guenter

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add support for ST TSC1641 power monitor

[Krzysztof Kozlowski]

On 27/10/2025 17:53, Guenter Roeck wrote:
> On 10/27/25 01:40, Krzysztof Kozlowski wrote:
>> On 26/10/2025 20:58, Guenter Roeck wrote:
>>>>>>> +  reg:
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  shunt-resistor-micro-ohms:
>>>>>>> +    description: Shunt resistor value in micro-ohms. Since device has internal
>>>>>>> +      16-bit RSHUNT register with 10 uOhm LSB, the maximum value is capped at
>>>>>>> +      655.35 mOhm.
>>>>>>> +    minimum: 100
>>>>>>> +    default: 1000
>>>>>>> +    maximum: 655350
>>>>>>> +
>>>>>>> +  st,alert-polarity-active-high:
>>>>>>
>>>>>> Isn't this just interrupt? You need proper interrupts property and then
>>>>>> its flag define the type of interrupt.
>>>>>
>>>>> This controls a bit written into device register.
>>>>> I omitted interrupt property after looking at existing power monitor bindings,
>>>>> especially hwmon/ti,ina2xx.yaml. INA226 has very similar bit controlling alert
>>>>> pin polarity and binding doesn't define alert pin as interrupt. Overall, I didn't
>>>>> find many power monitor bindings defining alert pins as interrupts.
>>>>
>>>>
>>>> On INA2xx that's SMBUS Alert. Is this the case here as well?
>>>>
>>>
>>> It could be wired to SMBus alert, or it could be wired to a CPU interrupt pin.
>>
>> So please explain me why CPU interrupt pin, which in every really every
>> device called "interrupts", would not be "interrupts" here? How CPU can
>> even guess the number of the interrupt in such case, without
>> "interrupts" property?
>>
> 
> I thought we were discussing the need for the st,alert-polarity-active-high
> property, sorry.


Yes, we kind of do, I am just trying to understand what is expressed
here. If this is a CPU interrupt, its flags should mark the proper
signal level, including inverter.

If this is something else (or both), then this property might make
sense, I just don't know what is this.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add support for ST TSC1641 power monitor

[Rob Herring]

On Mon, Oct 27, 2025 at 07:01:47PM +0100, Krzysztof Kozlowski wrote:
> On 27/10/2025 17:53, Guenter Roeck wrote:
> > On 10/27/25 01:40, Krzysztof Kozlowski wrote:
> >> On 26/10/2025 20:58, Guenter Roeck wrote:
> >>>>>>> +  reg:
> >>>>>>> +    maxItems: 1
> >>>>>>> +
> >>>>>>> +  shunt-resistor-micro-ohms:
> >>>>>>> +    description: Shunt resistor value in micro-ohms. Since device has internal
> >>>>>>> +      16-bit RSHUNT register with 10 uOhm LSB, the maximum value is capped at
> >>>>>>> +      655.35 mOhm.
> >>>>>>> +    minimum: 100
> >>>>>>> +    default: 1000
> >>>>>>> +    maximum: 655350
> >>>>>>> +
> >>>>>>> +  st,alert-polarity-active-high:
> >>>>>>
> >>>>>> Isn't this just interrupt? You need proper interrupts property and then
> >>>>>> its flag define the type of interrupt.
> >>>>>
> >>>>> This controls a bit written into device register.
> >>>>> I omitted interrupt property after looking at existing power monitor bindings,
> >>>>> especially hwmon/ti,ina2xx.yaml. INA226 has very similar bit controlling alert
> >>>>> pin polarity and binding doesn't define alert pin as interrupt. Overall, I didn't
> >>>>> find many power monitor bindings defining alert pins as interrupts.
> >>>>
> >>>>
> >>>> On INA2xx that's SMBUS Alert. Is this the case here as well?
> >>>>
> >>>
> >>> It could be wired to SMBus alert, or it could be wired to a CPU interrupt pin.
> >>
> >> So please explain me why CPU interrupt pin, which in every really every
> >> device called "interrupts", would not be "interrupts" here? How CPU can
> >> even guess the number of the interrupt in such case, without
> >> "interrupts" property?
> >>
> > 
> > I thought we were discussing the need for the st,alert-polarity-active-high
> > property, sorry.
> 
> 
> Yes, we kind of do, I am just trying to understand what is expressed
> here. If this is a CPU interrupt, its flags should mark the proper
> signal level, including inverter.
> 
> If this is something else (or both), then this property might make
> sense, I just don't know what is this.

If the device's polarity is programmable and there's possibly an 
inverter, you can't express both with just flags. In that case, flags 
should be the polarity after the inverter since flags are defined by the 
interrupt provider. So we'd need something to define the device side. 
Perhaps wait until someone actually has h/w with an inverter needing 
this.

Rob

[PATCH v2 1/2] dt-bindings: hwmon: Add support for ST TSC1641 power monitor

[Igor Reznichenko]

Understood. The bit in question controls the alert pin polarity on the device side,
independent of whether the pin is used as interrupt or not. I'll drop the property
for now and revisit if there's a board that actually uses an inverter or needs to
program the bit explicitly.

Thanks, Igor

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add support for ST TSC1641 power monitor

[Guenter Roeck]

On 10/28/25 08:17, Igor Reznichenko wrote:
> Understood. The bit in question controls the alert pin polarity on the device side,
> independent of whether the pin is used as interrupt or not. I'll drop the property
> for now and revisit if there's a board that actually uses an inverter or needs to
> program the bit explicitly.
> 

This is kind of unusual. The requirement used to be that devicetree properties
shall be complete. "Only if there is a known use case" is a significant policy
change. Has the policy changed recently ?

Thanks,
Guenter

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add support for ST TSC1641 power monitor

[Igor Reznichenko]

>On 10/28/25 08:17, Igor Reznichenko wrote:
>> Understood. The bit in question controls the alert pin polarity on the device side,
>> independent of whether the pin is used as interrupt or not. I'll drop the property
>> for now and revisit if there's a board that actually uses an inverter or needs to
>> program the bit explicitly.
>> 
>
>This is kind of unusual. The requirement used to be that devicetree properties
>shall be complete. "Only if there is a known use case" is a significant policy
>change. Has the policy changed recently ?
>
>Thanks,
>Guenter

Rob, following up on Guenter's question above.
I'm not sure whether it's better to drop the property as discussed earlier or keep
it for binding completeness. 
Could you clarify what approach is preferred?

Thanks, Igor

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add support for ST TSC1641 power monitor

[Krzysztof Kozlowski]

On 31/10/2025 05:40, Igor Reznichenko wrote:
>> On 10/28/25 08:17, Igor Reznichenko wrote:
>>> Understood. The bit in question controls the alert pin polarity on the device side,
>>> independent of whether the pin is used as interrupt or not. I'll drop the property
>>> for now and revisit if there's a board that actually uses an inverter or needs to
>>> program the bit explicitly.
>>>
>>
>> This is kind of unusual. The requirement used to be that devicetree properties
>> shall be complete. "Only if there is a known use case" is a significant policy
>> change. Has the policy changed recently ?
>>
>> Thanks,
>> Guenter
> 
> Rob, following up on Guenter's question above.
> I'm not sure whether it's better to drop the property as discussed earlier or keep
> it for binding completeness. 
> Could you clarify what approach is preferred?

Don't you have there possibility of interrupt (not only SMBus Alert)? At
least this is what I understood from previous talks.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add support for ST TSC1641 power monitor

[Igor Reznichenko]

>>> On 10/28/25 08:17, Igor Reznichenko wrote:
>>>> Understood. The bit in question controls the alert pin polarity on the device side,
>>>> independent of whether the pin is used as interrupt or not. I'll drop the property
>>>> for now and revisit if there's a board that actually uses an inverter or needs to
>>>> program the bit explicitly.
>>>>
>>>
>>> This is kind of unusual. The requirement used to be that devicetree properties
>>> shall be complete. "Only if there is a known use case" is a significant policy
>>> change. Has the policy changed recently ?
>>>
>>> Thanks,
>>> Guenter
>> 
>> Rob, following up on Guenter's question above.
>> I'm not sure whether it's better to drop the property as discussed earlier or keep
>> it for binding completeness. 
>> Could you clarify what approach is preferred?
>
>Don't you have there possibility of interrupt (not only SMBus Alert)? At
>least this is what I understood from previous talks.

Yes, the alert pin could be used as interrupt in principle.
Datasheet calls it "Multi-functional digital alert pin".

Thanks, Igor

Re: [PATCH v2 1/2] dt-bindings: hwmon: Add support for ST TSC1641 power monitor

[Guenter Roeck]

On 10/31/25 10:30, Igor Reznichenko wrote:
>>>> On 10/28/25 08:17, Igor Reznichenko wrote:
>>>>> Understood. The bit in question controls the alert pin polarity on the device side,
>>>>> independent of whether the pin is used as interrupt or not. I'll drop the property
>>>>> for now and revisit if there's a board that actually uses an inverter or needs to
>>>>> program the bit explicitly.
>>>>>
>>>>
>>>> This is kind of unusual. The requirement used to be that devicetree properties
>>>> shall be complete. "Only if there is a known use case" is a significant policy
>>>> change. Has the policy changed recently ?
>>>>
>>>> Thanks,
>>>> Guenter
>>>
>>> Rob, following up on Guenter's question above.
>>> I'm not sure whether it's better to drop the property as discussed earlier or keep
>>> it for binding completeness.
>>> Could you clarify what approach is preferred?
>>
>> Don't you have there possibility of interrupt (not only SMBus Alert)? At
>> least this is what I understood from previous talks.
> 
> Yes, the alert pin could be used as interrupt in principle.
> Datasheet calls it "Multi-functional digital alert pin".
> 

Maybe you could try adding an optional "interrupts" property.

Guenter