[PATCH v2 0/4] hwmon: Add driver for wsen-tids temperature driver

[PATCH v2 0/4] hwmon: Add driver for wsen-tids temperature driver

[Thomas Marangoni]

This patch series introduces a hwmon driver for wsen-tids temperature driver.

Following hwmon functions are supported:
- HWMON_T_INPUT
- HWMON_T_MIN
- HWMON_T_MAX
- HWMON_T_MIN_ALARM
- HWMON_T_MAX_ALARM
- HWMON_C_UPDATE_INTERVAL

A register dump from the device with i2cdump:
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00: 00 a0 00 00 4c 00 80 0c 17 e0 00 00 00 00 04 00

---
V1: https://lore.kernel.org/linux-hwmon/20251117123809.47488-1-Thomas.Marangoni@becom-group.com/

Changes since v1:
- Reworked regmap fields to regular regmap functions
- Reworded documentation
- Replaced temp_min_hyst with temp_min function
- Replaced temp_max_hyst with temp_max function
- Renamed tids-2521020222501 to tids

Thomas Marangoni (4):
  dt-bindings: Add trivial-devices for WSEN-tids temperature sensor and
    wsen as vendor-prefix
  MAINTAINERS: Add tids driver as maintained
  hwmon: Add driver for wsen tids
  hwmon: documentation: add tids

 .../devicetree/bindings/trivial-devices.yaml  |   2 +
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 Documentation/hwmon/tids.rst                  |  61 +++
 MAINTAINERS                                   |   7 +
 drivers/hwmon/Kconfig                         |  10 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/tids.c                          | 447 ++++++++++++++++++
 7 files changed, 530 insertions(+)
 create mode 100644 Documentation/hwmon/tids.rst
 create mode 100644 drivers/hwmon/tids.c


base-commit: fe4d0dea039f2befb93f27569593ec209843b0f5
-- 
2.51.1

[PATCH v2 1/4] dt-bindings: Add trivial-devices for WSEN-tids temperature sensor and wsen as vendor-prefix

[Thomas Marangoni]

WSEN-tids only has an i2c interface and an optional interrupt. This
should qualify the sensor as trivial device.

WSEN is a subcompany of Würth Electronics specialized on IC sensors.

Signed-off-by: Thomas Marangoni <Thomas.Marangoni@becom-group.com>
---
 Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 2eff6f274302..0a6b687552c8 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -488,6 +488,8 @@ properties:
           - ti,tsc2003
             # Winbond/Nuvoton H/W Monitor
           - winbond,w83793
+            # Würth temperature sensor with i2c interface
+          - wsen,tids
 
 required:
   - compatible
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 42d2bc0ce027..ef527e980137 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1787,6 +1787,8 @@ patternProperties:
     description: Würth Elektronik GmbH.
   "^welltech,.*":
     description: Welltech Computer Co., Limited.
+  "^wsen,.*":
+    description: Würth Elektronik eiSos GmbH & Co. KG
   "^wetek,.*":
     description: WeTek Electronics, limited.
   "^wexler,.*":
-- 
2.51.1

[PATCH v2 2/4] MAINTAINERS: Add tids driver as maintained

[Thomas Marangoni]

I've added myself as maintainer for the tids driver.

Signed-off-by: Thomas Marangoni <Thomas.Marangoni@becom-group.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 59b145dde215..7b74461b9fa8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -28042,6 +28042,13 @@ F:	include/linux/workqueue.h
 F:	kernel/workqueue.c
 F:	kernel/workqueue_internal.h
 
+WSEN TIDS DRIVER
+M:	Thomas Marangoni <Thomas.Marangoni@becom-group.com>
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	Documentation/hwmon/tids.rst
+F:	drivers/hwmon/tids.c
+
 WWAN DRIVERS
 M:	Loic Poulain <loic.poulain@oss.qualcomm.com>
 M:	Sergey Ryazanov <ryazanov.s.a@gmail.com>
-- 
2.51.1

[PATCH v2 3/4] hwmon: Add driver for wsen tids

[Thomas Marangoni]

Add support for the wsen tids. It is a low cost
and small-form-factor i2c temperature sensor.

It supports the following features:
- Continuous temperature reading in four intervals: 5 ms, 10 ms,
  20 ms and 40 ms.
- Low temperature alarm
- High temperature alarm

The driver supports following hwmon features:
- hwmon_temp_input
- hwmon_temp_min_alarm
- hwmon_temp_max_alarm
- hwmon_temp_min
- hwmon_temp_max
- hwmon_chip_update_interval

Additional notes:
- The update interval only supports four fixed values.
- The alarm is reset on reading.

Signed-off-by: Thomas Marangoni <Thomas.Marangoni@becom-group.com>
---
 drivers/hwmon/Kconfig  |  10 +
 drivers/hwmon/Makefile |   1 +
 drivers/hwmon/tids.c   | 447 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 458 insertions(+)
 create mode 100644 drivers/hwmon/tids.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 157678b821fc..2737350bb661 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2368,6 +2368,16 @@ config SENSORS_THMC50
 	  This driver can also be built as a module. If so, the module
 	  will be called thmc50.
 
+config SENSORS_TIDS
+	tristate "TIDS"
+	depends on I2C
+	help
+	  If you say yes here you get support for the temperature
+	  sensor WSEN TIDS from Würth Elektronik.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called tids.
+
 config SENSORS_TMP102
 	tristate "Texas Instruments TMP102"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index eade8e3b1bde..4eb77be3df67 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -227,6 +227,7 @@ obj-$(CONFIG_SENSORS_SY7636A)	+= sy7636a-hwmon.o
 obj-$(CONFIG_SENSORS_AMC6821)	+= amc6821.o
 obj-$(CONFIG_SENSORS_TC74)	+= tc74.o
 obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
+obj-$(CONFIG_SENSORS_TIDS)		+= tids.o
 obj-$(CONFIG_SENSORS_TMP102)	+= tmp102.o
 obj-$(CONFIG_SENSORS_TMP103)	+= tmp103.o
 obj-$(CONFIG_SENSORS_TMP108)	+= tmp108.o
diff --git a/drivers/hwmon/tids.c b/drivers/hwmon/tids.c
new file mode 100644
index 000000000000..62e778202a5f
--- /dev/null
+++ b/drivers/hwmon/tids.c
@@ -0,0 +1,447 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * Copyright (c) BECOM Electronics GmbH
+ *
+ * wsen_tids.c - Linux hwmon driver for WSEN-TIDS Temperature sensor
+ *
+ * Author: Thomas Marangoni <thomas.marangoni@becom-group.com>
+ */
+
+#include <linux/util_macros.h>
+#include <linux/regmap.h>
+#include <linux/minmax.h>
+#include <linux/hwmon.h>
+#include <linux/bits.h>
+#include <linux/math.h>
+#include <linux/i2c.h>
+
+/*
+ * TIDS registers
+ */
+#define TIDS_REG_DEVICE_ID	0x01
+#define TIDS_REG_T_H_LIMIT	0x02
+#define TIDS_REG_T_L_LIMIT	0x03
+#define TIDS_REG_CTRL		0x04
+#define TIDS_REG_STATUS		0x05
+#define TIDS_REG_DATA_T_L	0x06
+#define TIDS_REG_DATA_T_H	0x07
+#define TIDS_REG_SOFT_REST	0x0C
+
+#define TIDS_CTRL_ONE_SHOT_MASK		BIT(0)
+#define TIDS_CTRL_FREERUN_MASK		BIT(2)
+#define TIDS_CTRL_IF_ADD_INC_MASK	BIT(3)
+#define TIDS_CTRL_AVG_MASK		GENMASK(5, 4)
+#define TIDS_CTRL_AVG_SHIFT		4
+#define TIDS_CTRL_BDU_MASK		BIT(6)
+
+#define TIDS_STATUS_BUSY_MASK		BIT(0)
+#define TIDS_STATUS_OVER_THL_MASK	BIT(1)
+#define TIDS_STATUS_UNDER_TLL_MASK	BIT(2)
+
+#define TIDS_SOFT_REST_MASK		BIT(1)
+
+/*
+ * TIDS device IDs
+ */
+#define TIDS_ID 0xa0
+
+struct tids_data {
+	struct i2c_client *client;
+
+	struct regmap *regmap;
+
+	int irq;
+	int temperature;
+};
+
+static u8 update_intervals[] = { 40, 20, 10, 5 };
+
+static ssize_t tids_interval_read(struct device *dev, long *val)
+{
+	struct tids_data *data = dev_get_drvdata(dev);
+	unsigned int avg_value = 0;
+	int ret;
+
+	ret = regmap_read(data->regmap, TIDS_REG_CTRL, &avg_value);
+	if (ret < 0)
+		return ret;
+
+	avg_value = (avg_value & TIDS_CTRL_AVG_MASK) >> TIDS_CTRL_AVG_SHIFT;
+
+	*val = update_intervals[avg_value];
+
+	return 0;
+}
+
+static ssize_t tids_interval_write(struct device *dev, long val)
+{
+	struct tids_data *data = dev_get_drvdata(dev);
+	unsigned int avg_value;
+
+	avg_value = find_closest_descending(val, update_intervals,
+					    ARRAY_SIZE(update_intervals));
+
+	return regmap_write_bits(data->regmap, TIDS_REG_CTRL,
+				 TIDS_CTRL_AVG_MASK,
+				 avg_value << TIDS_CTRL_AVG_SHIFT);
+}
+
+static int tids_temperature1_read(struct device *dev, long *val)
+{
+	struct tids_data *data = dev_get_drvdata(dev);
+	u8 buf[2] = { 0 };
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, TIDS_REG_DATA_T_L, buf, 2);
+	if (ret < 0)
+		return ret;
+
+	/* temperature in °mC */
+	*val = (((s16)(buf[1] << 8) | buf[0])) * 10;
+
+	return 0;
+}
+
+static ssize_t tids_temperature_alarm_read(struct device *dev, u32 attr,
+					   long *val)
+{
+	struct tids_data *data = dev_get_drvdata(dev);
+	int ret;
+
+	if (attr == hwmon_temp_min_alarm)
+		ret = regmap_test_bits(data->regmap, TIDS_REG_STATUS,
+				       TIDS_STATUS_UNDER_TLL_MASK);
+	else if (attr == hwmon_temp_max_alarm)
+		ret = regmap_test_bits(data->regmap, TIDS_REG_STATUS,
+				       TIDS_STATUS_OVER_THL_MASK);
+	else
+		return -EOPNOTSUPP;
+
+	if (ret < 0)
+		return ret;
+
+	*val = ret;
+
+	return 0;
+}
+
+static int tids_temperature_minmax_read(struct device *dev, u32 attr, long *val)
+{
+	struct tids_data *data = dev_get_drvdata(dev);
+	unsigned int reg_data = 0;
+	int ret;
+
+	if (attr == hwmon_temp_min)
+		ret = regmap_read(data->regmap, TIDS_REG_T_L_LIMIT, &reg_data);
+	else if (attr == hwmon_temp_max)
+		ret = regmap_read(data->regmap, TIDS_REG_T_H_LIMIT, &reg_data);
+	else
+		return -EOPNOTSUPP;
+
+	if (ret < 0)
+		return ret;
+
+	/* temperature from register conversion in °mC */
+	*val = (((u8)reg_data - 63) * 640);
+
+	return 0;
+}
+
+static ssize_t tids_temperature_minmax_write(struct device *dev, u32 attr,
+					     long val)
+{
+	struct tids_data *data = dev_get_drvdata(dev);
+	u8 reg_data;
+
+	/* temperature in °mC */
+	val = clamp_val(val, -39680, 122880);
+	/* temperature to register conversion in °mC */
+	reg_data = (u8)(DIV_ROUND_CLOSEST(val, 640) + 63);
+
+	if (attr == hwmon_temp_min)
+		return regmap_write(data->regmap, TIDS_REG_T_L_LIMIT, reg_data);
+	else if (attr == hwmon_temp_max)
+		return regmap_write(data->regmap, TIDS_REG_T_H_LIMIT, reg_data);
+	else
+		return -EOPNOTSUPP;
+}
+
+static umode_t tids_hwmon_visible(const void *data,
+				  enum hwmon_sensor_types type, u32 attr,
+				  int channel)
+{
+	umode_t mode = 0;
+
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+		case hwmon_temp_min_alarm:
+		case hwmon_temp_max_alarm:
+			mode = 0444;
+			break;
+		case hwmon_temp_min:
+		case hwmon_temp_max:
+			mode = 0644;
+			break;
+		default:
+			break;
+		}
+		break;
+	case hwmon_chip:
+		switch (attr) {
+		case hwmon_chip_update_interval:
+			mode = 0644;
+			break;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return mode;
+}
+
+static int tids_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+			   u32 attr, int channel, long *val)
+{
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+			return tids_temperature1_read(dev, val);
+		case hwmon_temp_min_alarm:
+		case hwmon_temp_max_alarm:
+			return tids_temperature_alarm_read(dev, attr, val);
+		case hwmon_temp_min:
+		case hwmon_temp_max:
+			return tids_temperature_minmax_read(dev, attr, val);
+		default:
+			return -EOPNOTSUPP;
+		}
+	case hwmon_chip:
+		switch (attr) {
+		case hwmon_chip_update_interval:
+			return tids_interval_read(dev, val);
+		default:
+			return -EOPNOTSUPP;
+		}
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int tids_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
+			    u32 attr, int channel, long val)
+{
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_min:
+		case hwmon_temp_max:
+			return tids_temperature_minmax_write(dev, attr, val);
+		default:
+			return -EOPNOTSUPP;
+		}
+	case hwmon_chip:
+		switch (attr) {
+		case hwmon_chip_update_interval:
+			return tids_interval_write(dev, val);
+		default:
+			return -EOPNOTSUPP;
+		}
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static const struct hwmon_channel_info *const tids_info[] = {
+	HWMON_CHANNEL_INFO(chip, HWMON_C_UPDATE_INTERVAL),
+	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_MIN_ALARM |
+			  HWMON_T_MAX_ALARM | HWMON_T_MIN | HWMON_T_MAX),
+	NULL
+};
+
+static const struct hwmon_ops tids_ops = {
+	.is_visible	= tids_hwmon_visible,
+	.read		= tids_hwmon_read,
+	.write		= tids_hwmon_write,
+};
+
+static const struct hwmon_chip_info tids_chip_info = {
+	.ops	= &tids_ops,
+	.info	= tids_info,
+};
+
+static bool tids_regmap_writeable_reg(struct device *dev, unsigned int reg)
+{
+	if (reg >= TIDS_REG_T_H_LIMIT && reg <= TIDS_REG_CTRL)
+		return true;
+
+	if (reg == TIDS_REG_SOFT_REST)
+		return true;
+
+	return false;
+}
+
+static bool tids_regmap_readable_reg(struct device *dev, unsigned int reg)
+{
+	if (reg >= TIDS_REG_DEVICE_ID && reg <= TIDS_REG_DATA_T_H)
+		return true;
+
+	if (reg == TIDS_REG_SOFT_REST)
+		return true;
+
+	return false;
+}
+
+static bool tids_regmap_volatile_reg(struct device *dev, unsigned int reg)
+{
+	if (reg >= TIDS_REG_STATUS && reg <= TIDS_REG_DATA_T_H)
+		return true;
+
+	if (reg == TIDS_REG_SOFT_REST)
+		return true;
+
+	return false;
+}
+
+static const struct regmap_config regmap_config = {
+	.reg_bits		= 8,
+	.val_bits		= 8,
+	.max_register		= TIDS_REG_SOFT_REST,
+	.writeable_reg		= tids_regmap_writeable_reg,
+	.readable_reg		= tids_regmap_readable_reg,
+	.volatile_reg		= tids_regmap_volatile_reg,
+	.use_single_read	= 0,
+};
+
+static int tids_init(struct tids_data *data)
+{
+	int ret;
+
+	/* Triggering soft reset */
+	ret = regmap_write_bits(data->regmap, TIDS_REG_SOFT_REST,
+				TIDS_SOFT_REST_MASK, TIDS_SOFT_REST_MASK);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_clear_bits(data->regmap, TIDS_REG_SOFT_REST,
+				TIDS_SOFT_REST_MASK);
+	if (ret < 0)
+		return ret;
+
+	/* Allowing bulk read */
+	ret = regmap_write_bits(data->regmap, TIDS_REG_CTRL,
+				TIDS_CTRL_IF_ADD_INC_MASK,
+				TIDS_CTRL_IF_ADD_INC_MASK);
+	if (ret < 0)
+		return ret;
+
+	/* Set meassurement interval */
+	ret = regmap_clear_bits(data->regmap, TIDS_REG_CTRL,
+				TIDS_CTRL_AVG_MASK);
+	if (ret < 0)
+		return ret;
+
+	/* Set device to free run mode */
+	ret = regmap_write_bits(data->regmap, TIDS_REG_CTRL,
+				TIDS_CTRL_FREERUN_MASK, TIDS_CTRL_FREERUN_MASK);
+	if (ret < 0)
+		return ret;
+
+	/* Don't update temperature register until high and low value are read */
+	ret = regmap_write_bits(data->regmap, TIDS_REG_CTRL, TIDS_CTRL_BDU_MASK,
+				TIDS_CTRL_BDU_MASK);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int tids_probe(struct i2c_client *client)
+{
+	struct device *device = &client->dev;
+	struct device *hwmon_dev;
+	struct tids_data *data;
+	unsigned int value;
+	int ret;
+
+	data = devm_kzalloc(device, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->client = client;
+
+	/* Init regmap */
+	data->regmap = devm_regmap_init_i2c(data->client, &regmap_config);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(device, PTR_ERR(data->regmap),
+				     "regmap initialization failed\n");
+
+	/* Read device id, to check if i2c is working */
+	ret = regmap_read(data->regmap, TIDS_REG_DEVICE_ID, &value);
+	if (ret < 0)
+		return ret;
+
+	if (value != TIDS_ID)
+		return -ENODEV;
+
+	tids_init(data);
+
+	hwmon_dev = devm_hwmon_device_register_with_info(device, "tids", data,
+							 &tids_chip_info, NULL);
+
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static int tids_suspend(struct device *dev)
+{
+	struct tids_data *data = dev_get_drvdata(dev);
+
+	return regmap_clear_bits(data->regmap, TIDS_REG_CTRL,
+				 TIDS_CTRL_FREERUN_MASK);
+}
+
+static int tids_resume(struct device *dev)
+{
+	struct tids_data *data = dev_get_drvdata(dev);
+
+	return regmap_write_bits(data->regmap, TIDS_REG_CTRL,
+				 TIDS_CTRL_FREERUN_MASK,
+				 TIDS_CTRL_FREERUN_MASK);
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(tids_dev_pm_ops, tids_resume, tids_suspend);
+
+static const struct i2c_device_id tids_id[] = {
+	{ "tids", 0 },
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, tids_id);
+
+static const struct of_device_id tids_of_match[] = {
+	{ .compatible = "wsen,tids" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, tids_of_match);
+
+static struct i2c_driver tids_driver = {
+	.class	  = I2C_CLASS_HWMON,
+	.driver   = {
+		.name		= "tids",
+		.pm		= pm_sleep_ptr(&tids_dev_pm_ops),
+		.of_match_table	= tids_of_match,
+	},
+	.probe    = tids_probe,
+	.id_table = tids_id,
+};
+
+module_i2c_driver(tids_driver);
+
+MODULE_AUTHOR("Thomas Marangoni <Thomas.Marangoni@becom-group.com>");
+MODULE_DESCRIPTION("WSEN TIDS temperature sensor driver");
+MODULE_LICENSE("GPL");
-- 
2.51.1

[PATCH v2 4/4] hwmon: documentation: add tids

[Thomas Marangoni]

Add tids driver documentation

Signed-off-by: Thomas Marangoni <Thomas.Marangoni@becom-group.com>
---
 Documentation/hwmon/tids.rst | 61 ++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 Documentation/hwmon/tids.rst

diff --git a/Documentation/hwmon/tids.rst b/Documentation/hwmon/tids.rst
new file mode 100644
index 000000000000..254c4a90e6f8
--- /dev/null
+++ b/Documentation/hwmon/tids.rst
@@ -0,0 +1,61 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Kernel driver tids
+===================
+
+Supported Chips:
+
+  * WSEN TIDS
+
+    Prefix: 'tids'
+
+    Addresses scanned: None
+
+    Datasheet:
+
+      English: https://www.we-online.com/components/products/manual/Manual-um-wsen-tids-2521020222501%20(rev1.2).pdf
+
+Author: Thomas Marangoni <Thomas.Marangoni@becom-group.com>
+
+
+Description
+-----------
+
+This driver implements support for the WSEN TIDS chip, a temperature
+sensor. Temperature is measured in degree celsius. In sysfs interface,
+all values are scaled by 1000, i.e. the value for 31.5 degrees celsius is 31500.
+
+Usage Notes
+-----------
+
+The device communicates with the I2C protocol. Sensors can have the I2C
+address 0x38 or 0x3F. See Documentation/i2c/instantiating-devices.rst for methods
+to instantiate the device.
+
+Sysfs entries
+-------------
+
+=============== ============================================
+temp1_input     Measured temperature in millidegrees Celsius
+update_interval The interval for polling the sensor, in
+                milliseconds. Writable. Supported values are
+                5, 10, 20 or 40.
+temp1_max       The temperature in millidegrees Celsius, that
+                is triggering the temp1_max_alarm. Writable.
+                The lowest supported value is -39680 and the
+                highest supported value is 122880. Values are
+                saved in steps of 640.
+temp1_min       The temperature in millidegrees Celsius, that
+                is triggering the temp1_min_alarm. Writable.
+                The lowest supported value is -39680 and the
+                highest supported value is 122880. Values are
+                saved in steps of 640.
+temp1_max_alarm The alarm will be triggered when the level
+                reaches the value specified in
+                temp1_max. It will reset automatically
+                once it has been read.
+temp1_min_alarm The alarm will be triggered when the level
+                reaches the value specified in
+                temp1_min. It will reset automatically
+                once it has been read.
+=============== ============================================
-- 
2.51.1

Re: [PATCH v2 3/4] hwmon: Add driver for wsen tids

[Nuno Sá]

Hi Thomas,

I'm a bit surprised why you have me on Cc. Since I'm here, let me give some inputs...

On Wed, 2025-11-19 at 13:51 +0100, Thomas Marangoni wrote:
> Add support for the wsen tids. It is a low cost
> and small-form-factor i2c temperature sensor.
> 
> It supports the following features:
> - Continuous temperature reading in four intervals: 5 ms, 10 ms,
>   20 ms and 40 ms.
> - Low temperature alarm
> - High temperature alarm
> 
> The driver supports following hwmon features:
> - hwmon_temp_input
> - hwmon_temp_min_alarm
> - hwmon_temp_max_alarm
> - hwmon_temp_min
> - hwmon_temp_max
> - hwmon_chip_update_interval
> 
> Additional notes:
> - The update interval only supports four fixed values.
> - The alarm is reset on reading.
> 
> Signed-off-by: Thomas Marangoni <Thomas.Marangoni@becom-group.com>
> ---
>  drivers/hwmon/Kconfig  |  10 +
>  drivers/hwmon/Makefile |   1 +
>  drivers/hwmon/tids.c   | 447 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 458 insertions(+)
>  create mode 100644 drivers/hwmon/tids.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 157678b821fc..2737350bb661 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2368,6 +2368,16 @@ config SENSORS_THMC50
>  	  This driver can also be built as a module. If so, the module
>  	  will be called thmc50.
>  
> +config SENSORS_TIDS
> +	tristate "TIDS"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for the temperature
> +	  sensor WSEN TIDS from Würth Elektronik.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called tids.
> +
>  config SENSORS_TMP102
>  	tristate "Texas Instruments TMP102"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index eade8e3b1bde..4eb77be3df67 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -227,6 +227,7 @@ obj-$(CONFIG_SENSORS_SY7636A)	+= sy7636a-hwmon.o
>  obj-$(CONFIG_SENSORS_AMC6821)	+= amc6821.o
>  obj-$(CONFIG_SENSORS_TC74)	+= tc74.o
>  obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
> +obj-$(CONFIG_SENSORS_TIDS)		+= tids.o
>  obj-$(CONFIG_SENSORS_TMP102)	+= tmp102.o
>  obj-$(CONFIG_SENSORS_TMP103)	+= tmp103.o
>  obj-$(CONFIG_SENSORS_TMP108)	+= tmp108.o
> diff --git a/drivers/hwmon/tids.c b/drivers/hwmon/tids.c
> new file mode 100644
> index 000000000000..62e778202a5f
> --- /dev/null
> +++ b/drivers/hwmon/tids.c
> @@ -0,0 +1,447 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Copyright (c) BECOM Electronics GmbH
> + *
> + * wsen_tids.c - Linux hwmon driver for WSEN-TIDS Temperature sensor
> + *
> + * Author: Thomas Marangoni <thomas.marangoni@becom-group.com>
> + */
> +
> +#include <linux/util_macros.h>
> +#include <linux/regmap.h>
> +#include <linux/minmax.h>
> +#include <linux/hwmon.h>
> +#include <linux/bits.h>
> +#include <linux/math.h>
> +#include <linux/i2c.h>
> +
> +/*
> + * TIDS registers
> + */
> +#define TIDS_REG_DEVICE_ID	0x01
> +#define TIDS_REG_T_H_LIMIT	0x02
> +#define TIDS_REG_T_L_LIMIT	0x03
> +#define TIDS_REG_CTRL		0x04
> +#define TIDS_REG_STATUS		0x05
> +#define TIDS_REG_DATA_T_L	0x06
> +#define TIDS_REG_DATA_T_H	0x07
> +#define TIDS_REG_SOFT_REST	0x0C
> +
> +#define TIDS_CTRL_ONE_SHOT_MASK		BIT(0)
> +#define TIDS_CTRL_FREERUN_MASK		BIT(2)
> +#define TIDS_CTRL_IF_ADD_INC_MASK	BIT(3)
> +#define TIDS_CTRL_AVG_MASK		GENMASK(5, 4)
> +#define TIDS_CTRL_AVG_SHIFT		4
> +#define TIDS_CTRL_BDU_MASK		BIT(6)
> +
> +#define TIDS_STATUS_BUSY_MASK		BIT(0)
> +#define TIDS_STATUS_OVER_THL_MASK	BIT(1)
> +#define TIDS_STATUS_UNDER_TLL_MASK	BIT(2)
> +
> +#define TIDS_SOFT_REST_MASK		BIT(1)
> +
> +/*
> + * TIDS device IDs
> + */
> +#define TIDS_ID 0xa0
> +
> +struct tids_data {
> +	struct i2c_client *client;
> +
> +	struct regmap *regmap;
> +
> +	int irq;
> +	int temperature;
> +};
> +
> +static u8 update_intervals[] = { 40, 20, 10, 5 };

static const?

> +
> +static ssize_t tids_interval_read(struct device *dev, long *val)
> +{
> +	struct tids_data *data = dev_get_drvdata(dev);
> +	unsigned int avg_value = 0;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, TIDS_REG_CTRL, &avg_value);
> +	if (ret < 0)
> +		return ret;
> +
> +	avg_value = (avg_value & TIDS_CTRL_AVG_MASK) >> TIDS_CTRL_AVG_SHIFT;
> +
> +	*val = update_intervals[avg_value];
> +
> +	return 0;
> +}
> +
> +static ssize_t tids_interval_write(struct device *dev, long val)
> +{
> +	struct tids_data *data = dev_get_drvdata(dev);
> +	unsigned int avg_value;
> +
> +	avg_value = find_closest_descending(val, update_intervals,
> +					    ARRAY_SIZE(update_intervals));
> +
> +	return regmap_write_bits(data->regmap, TIDS_REG_CTRL,
> +				 TIDS_CTRL_AVG_MASK,
> +				 avg_value << TIDS_CTRL_AVG_SHIFT);
> +}
> +
> +static int tids_temperature1_read(struct device *dev, long *val)
> +{
> +	struct tids_data *data = dev_get_drvdata(dev);
> +	u8 buf[2] = { 0 };

Seems like __le16?
> +	int ret;
> +
> +	ret = regmap_bulk_read(data->regmap, TIDS_REG_DATA_T_L, buf, 2);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* temperature in °mC */
> +	*val = (((s16)(buf[1] << 8) | buf[0])) * 10;

Then __le16_to_cpu()?

> +
> +	return 0;
> +}
> +
> +static ssize_t tids_temperature_alarm_read(struct device *dev, u32 attr,
> +					   long *val)
> +{
> +	struct tids_data *data = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (attr == hwmon_temp_min_alarm)
> +		ret = regmap_test_bits(data->regmap, TIDS_REG_STATUS,
> +				       TIDS_STATUS_UNDER_TLL_MASK);
> +	else if (attr == hwmon_temp_max_alarm)
> +		ret = regmap_test_bits(data->regmap, TIDS_REG_STATUS,
> +				       TIDS_STATUS_OVER_THL_MASK);

Instead of passing attr and have this if() else why not passing the proper mask? Then
just regmap_read(regmag, reg, ...)?

> +	else
> +		return -EOPNOTSUPP;
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = ret;
> +
> +	return 0;
> +}
> +
> +static int tids_temperature_minmax_read(struct device *dev, u32 attr, long *val)
> +{
> +	struct tids_data *data = dev_get_drvdata(dev);
> +	unsigned int reg_data = 0;
> +	int ret;
> +
> +	if (attr == hwmon_temp_min)
> +		ret = regmap_read(data->regmap, TIDS_REG_T_L_LIMIT, &reg_data);
> +	else if (attr == hwmon_temp_max)
> +		ret = regmap_read(data->regmap, TIDS_REG_T_H_LIMIT, &reg_data);
> +	else
> +		return -EOPNOTSUPP;

Same as above but with the proper register

> +
> +	if (ret < 0)
> +		return ret;
> +
> +	/* temperature from register conversion in °mC */
> +	*val = (((u8)reg_data - 63) * 640);

Why the cast?

> +
> +	return 0;
> +}
> +
> +static ssize_t tids_temperature_minmax_write(struct device *dev, u32 attr,
> +					     long val)
> +{
> +	struct tids_data *data = dev_get_drvdata(dev);
> +	u8 reg_data;
> +
> +	/* temperature in °mC */
> +	val = clamp_val(val, -39680, 122880);
> +	/* temperature to register conversion in °mC */
> +	reg_data = (u8)(DIV_ROUND_CLOSEST(val, 640) + 63);
> +
> +	if (attr == hwmon_temp_min)
> +		return regmap_write(data->regmap, TIDS_REG_T_L_LIMIT, reg_data);
> +	else if (attr == hwmon_temp_max)
> +		return regmap_write(data->regmap, TIDS_REG_T_H_LIMIT, reg_data);
> +	else
> +		return -EOPNOTSUPP;

Redundant else if () and else

...

> 
> +
> +static int tids_init(struct tids_data *data)
> +{
> +	int ret;
> +
> +	/* Triggering soft reset */
> +	ret = regmap_write_bits(data->regmap, TIDS_REG_SOFT_REST,
> +				TIDS_SOFT_REST_MASK, TIDS_SOFT_REST_MASK);
> +	if (ret < 0)
> +		return ret;
> +

No need for sleep some time? Typically that's defined on the datasheet.

> +	ret = regmap_clear_bits(data->regmap, TIDS_REG_SOFT_REST,
> +				TIDS_SOFT_REST_MASK);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Allowing bulk read */
> +	ret = regmap_write_bits(data->regmap, TIDS_REG_CTRL,
> +				TIDS_CTRL_IF_ADD_INC_MASK,
> +				TIDS_CTRL_IF_ADD_INC_MASK);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Set meassurement interval */
> +	ret = regmap_clear_bits(data->regmap, TIDS_REG_CTRL,
> +				TIDS_CTRL_AVG_MASK);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Set device to free run mode */
> +	ret = regmap_write_bits(data->regmap, TIDS_REG_CTRL,
> +				TIDS_CTRL_FREERUN_MASK, TIDS_CTRL_FREERUN_MASK);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Don't update temperature register until high and low value are read */
> +	ret = regmap_write_bits(data->regmap, TIDS_REG_CTRL, TIDS_CTRL_BDU_MASK,
> +				TIDS_CTRL_BDU_MASK);

return regmap_write_bits();

> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int tids_probe(struct i2c_client *client)
> +{
> +	struct device *device = &client->dev;
> +	struct device *hwmon_dev;
> +	struct tids_data *data;
> +	unsigned int value;
> +	int ret;
> +
> +	data = devm_kzalloc(device, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +
> +	/* Init regmap */

The comment does not add any added value.

> +	data->regmap = devm_regmap_init_i2c(data->client, &regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(device, PTR_ERR(data->regmap),
> +				     "regmap initialization failed\n");
> +
> +	/* Read device id, to check if i2c is working */

Same

> +	ret = regmap_read(data->regmap, TIDS_REG_DEVICE_ID, &value);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (value != TIDS_ID)
> +		return -ENODEV;
> +
> +	tids_init(data);

Check for the return value.

> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(device, "tids", data,
> +							 &tids_chip_info, NULL);
> +
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static int tids_suspend(struct device *dev)
> +{
> +	struct tids_data *data = dev_get_drvdata(dev);
> +
> +	return regmap_clear_bits(data->regmap, TIDS_REG_CTRL,
> +				 TIDS_CTRL_FREERUN_MASK);
> +}
> +
> +static int tids_resume(struct device *dev)
> +{
> +	struct tids_data *data = dev_get_drvdata(dev);
> +
> +	return regmap_write_bits(data->regmap, TIDS_REG_CTRL,
> +				 TIDS_CTRL_FREERUN_MASK,
> +				 TIDS_CTRL_FREERUN_MASK);
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(tids_dev_pm_ops, tids_resume, tids_suspend);
> +
> +static const struct i2c_device_id tids_id[] = {
> +	{ "tids", 0 },

No need for 0

> +	{},

The above is already a terminator so you can drop the comma

- Nuno Sá

Re: [PATCH v2 2/4] MAINTAINERS: Add tids driver as maintained

[Krzysztof Kozlowski]

On 19/11/2025 13:51, Thomas Marangoni wrote:
> I've added myself as maintainer for the tids driver.

Please use imperative. See submitting patches.

> 
> Signed-off-by: Thomas Marangoni <Thomas.Marangoni@becom-group.com>
> ---
>  MAINTAINERS | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 59b145dde215..7b74461b9fa8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -28042,6 +28042,13 @@ F:	include/linux/workqueue.h
>  F:	kernel/workqueue.c
>  F:	kernel/workqueue_internal.h
>  
> +WSEN TIDS DRIVER
> +M:	Thomas Marangoni <Thomas.Marangoni@becom-group.com>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/hwmon/tids.rst
> +F:	drivers/hwmon/tids.c

There is no such file at this point, which leads to get_maintainers
warning. You need to properly order the pachset (see git log) to
maintain full bisectability.

Best regards,
Krzysztof

Re: [PATCH v2 4/4] hwmon: documentation: add tids

[Guenter Roeck]

On 11/19/25 04:51, Thomas Marangoni wrote:
> Add tids driver documentation
> 
> Signed-off-by: Thomas Marangoni <Thomas.Marangoni@becom-group.com>
> ---
>   Documentation/hwmon/tids.rst | 61 ++++++++++++++++++++++++++++++++++++

Needs to be added to Documentation/hwmon/index.rst.

I'd suggest to combine patches 2, 3, and 4 into a single patch.

>   1 file changed, 61 insertions(+)
>   create mode 100644 Documentation/hwmon/tids.rst
> 
> diff --git a/Documentation/hwmon/tids.rst b/Documentation/hwmon/tids.rst
> new file mode 100644
> index 000000000000..254c4a90e6f8
> --- /dev/null
> +++ b/Documentation/hwmon/tids.rst
> @@ -0,0 +1,61 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver tids
> +===================
> +
> +Supported Chips:
> +
> +  * WSEN TIDS
> +
> +    Prefix: 'tids'
> +
> +    Addresses scanned: None
> +
> +    Datasheet:
> +
> +      English: https://www.we-online.com/components/products/manual/Manual-um-wsen-tids-2521020222501%20(rev1.2).pdf
> +
> +Author: Thomas Marangoni <Thomas.Marangoni@becom-group.com>
> +
> +
> +Description
> +-----------
> +
> +This driver implements support for the WSEN TIDS chip, a temperature

This driver implements support -> This driver supports

> +sensor. Temperature is measured in degree celsius. In sysfs interface,
> +all values are scaled by 1000, i.e. the value for 31.5 degrees celsius is 31500.
> +

This is per ABI and does not need to be mentioned. If you want to mention any
details, the operating temperature and range and the current consumption (from the
introduction in the datasheet) would be acceptable.

> +Usage Notes
> +-----------
> +
> +The device communicates with the I2C protocol. Sensors can have the I2C
> +address 0x38 or 0x3F. See Documentation/i2c/instantiating-devices.rst for methods
> +to instantiate the device.
> +
> +Sysfs entries
> +-------------
> +
> +=============== ============================================
> +temp1_input     Measured temperature in millidegrees Celsius
> +update_interval The interval for polling the sensor, in
> +                milliseconds. Writable. Supported values are
> +                5, 10, 20 or 40.
> +temp1_max       The temperature in millidegrees Celsius, that
> +                is triggering the temp1_max_alarm. Writable.
> +                The lowest supported value is -39680 and the
> +                highest supported value is 122880. Values are
> +                saved in steps of 640.
> +temp1_min       The temperature in millidegrees Celsius, that
> +                is triggering the temp1_min_alarm. Writable.
> +                The lowest supported value is -39680 and the
> +                highest supported value is 122880. Values are
> +                saved in steps of 640.
> +temp1_max_alarm The alarm will be triggered when the level
> +                reaches the value specified in
> +                temp1_max. It will reset automatically
> +                once it has been read.

The datasheet suggests that it should be "exceeds", not "reaches".
I'd suggest to use "temperature", not "level".

> +temp1_min_alarm The alarm will be triggered when the level
> +                reaches the value specified in

Same here. Per datasheet: "Temperature is lower than low limit".
The datasheet says "exceeds the low limit", but that isn't technically
correct, because that means "higher than". "when the temperature is
lower than" should work.

> +                temp1_min. It will reset automatically
> +                once it has been read.
> +=============== ============================================

Re: [PATCH v2 3/4] hwmon: Add driver for wsen tids

[Guenter Roeck]

On 11/19/25 07:05, Nuno Sá wrote:
> Hi Thomas,
> 
> I'm a bit surprised why you have me on Cc. Since I'm here, let me give some inputs...
> 

Excellent feedback though. Some more comments inline.

> On Wed, 2025-11-19 at 13:51 +0100, Thomas Marangoni wrote:
>> Add support for the wsen tids. It is a low cost
>> and small-form-factor i2c temperature sensor.
>>
>> It supports the following features:
>> - Continuous temperature reading in four intervals: 5 ms, 10 ms,
>>    20 ms and 40 ms.
>> - Low temperature alarm
>> - High temperature alarm
>>
>> The driver supports following hwmon features:
>> - hwmon_temp_input
>> - hwmon_temp_min_alarm
>> - hwmon_temp_max_alarm
>> - hwmon_temp_min
>> - hwmon_temp_max
>> - hwmon_chip_update_interval
>>
>> Additional notes:
>> - The update interval only supports four fixed values.
>> - The alarm is reset on reading.
>>
>> Signed-off-by: Thomas Marangoni <Thomas.Marangoni@becom-group.com>
>> ---
>>   drivers/hwmon/Kconfig  |  10 +
>>   drivers/hwmon/Makefile |   1 +
>>   drivers/hwmon/tids.c   | 447 +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 458 insertions(+)
>>   create mode 100644 drivers/hwmon/tids.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 157678b821fc..2737350bb661 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -2368,6 +2368,16 @@ config SENSORS_THMC50
>>   	  This driver can also be built as a module. If so, the module
>>   	  will be called thmc50.
>>   
>> +config SENSORS_TIDS
>> +	tristate "TIDS"
>> +	depends on I2C
>> +	help
>> +	  If you say yes here you get support for the temperature
>> +	  sensor WSEN TIDS from Würth Elektronik.
>> +
>> +	  This driver can also be built as a module. If so, the module
>> +	  will be called tids.
>> +
>>   config SENSORS_TMP102
>>   	tristate "Texas Instruments TMP102"
>>   	depends on I2C
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index eade8e3b1bde..4eb77be3df67 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -227,6 +227,7 @@ obj-$(CONFIG_SENSORS_SY7636A)	+= sy7636a-hwmon.o
>>   obj-$(CONFIG_SENSORS_AMC6821)	+= amc6821.o
>>   obj-$(CONFIG_SENSORS_TC74)	+= tc74.o
>>   obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
>> +obj-$(CONFIG_SENSORS_TIDS)		+= tids.o
>>   obj-$(CONFIG_SENSORS_TMP102)	+= tmp102.o
>>   obj-$(CONFIG_SENSORS_TMP103)	+= tmp103.o
>>   obj-$(CONFIG_SENSORS_TMP108)	+= tmp108.o
>> diff --git a/drivers/hwmon/tids.c b/drivers/hwmon/tids.c
>> new file mode 100644
>> index 000000000000..62e778202a5f
>> --- /dev/null
>> +++ b/drivers/hwmon/tids.c
>> @@ -0,0 +1,447 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +/*
>> + * Copyright (c) BECOM Electronics GmbH
>> + *
>> + * wsen_tids.c - Linux hwmon driver for WSEN-TIDS Temperature sensor
>> + *
>> + * Author: Thomas Marangoni <thomas.marangoni@becom-group.com>
>> + */
>> +
>> +#include <linux/util_macros.h>
>> +#include <linux/regmap.h>
>> +#include <linux/minmax.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/bits.h>
>> +#include <linux/math.h>
>> +#include <linux/i2c.h>

Alphabetic order please.

>> +
>> +/*
>> + * TIDS registers
>> + */
>> +#define TIDS_REG_DEVICE_ID	0x01
>> +#define TIDS_REG_T_H_LIMIT	0x02
>> +#define TIDS_REG_T_L_LIMIT	0x03
>> +#define TIDS_REG_CTRL		0x04
>> +#define TIDS_REG_STATUS		0x05
>> +#define TIDS_REG_DATA_T_L	0x06
>> +#define TIDS_REG_DATA_T_H	0x07
>> +#define TIDS_REG_SOFT_REST	0x0C
>> +
>> +#define TIDS_CTRL_ONE_SHOT_MASK		BIT(0)
>> +#define TIDS_CTRL_FREERUN_MASK		BIT(2)
>> +#define TIDS_CTRL_IF_ADD_INC_MASK	BIT(3)
>> +#define TIDS_CTRL_AVG_MASK		GENMASK(5, 4)
>> +#define TIDS_CTRL_AVG_SHIFT		4
>> +#define TIDS_CTRL_BDU_MASK		BIT(6)
>> +
>> +#define TIDS_STATUS_BUSY_MASK		BIT(0)
>> +#define TIDS_STATUS_OVER_THL_MASK	BIT(1)
>> +#define TIDS_STATUS_UNDER_TLL_MASK	BIT(2)
>> +
>> +#define TIDS_SOFT_REST_MASK		BIT(1)
>> +
>> +/*
>> + * TIDS device IDs
>> + */
>> +#define TIDS_ID 0xa0
>> +
>> +struct tids_data {
>> +	struct i2c_client *client;
>> +
Unused

>> +	struct regmap *regmap;
>> +
>> +	int irq;

Unused

>> +	int temperature;

Unused

>> +};
>> +
>> +static u8 update_intervals[] = { 40, 20, 10, 5 };
> 
> static const?
> 
>> +
>> +static ssize_t tids_interval_read(struct device *dev, long *val)
>> +{
>> +	struct tids_data *data = dev_get_drvdata(dev);
>> +	unsigned int avg_value = 0;

Unnecessary initialization.

>> +	int ret;
>> +
>> +	ret = regmap_read(data->regmap, TIDS_REG_CTRL, &avg_value);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	avg_value = (avg_value & TIDS_CTRL_AVG_MASK) >> TIDS_CTRL_AVG_SHIFT;
>> +
>> +	*val = update_intervals[avg_value];
>> +
>> +	return 0;
>> +}
>> +
>> +static ssize_t tids_interval_write(struct device *dev, long val)
>> +{
>> +	struct tids_data *data = dev_get_drvdata(dev);
>> +	unsigned int avg_value;
>> +
>> +	avg_value = find_closest_descending(val, update_intervals,
>> +					    ARRAY_SIZE(update_intervals));
>> +
>> +	return regmap_write_bits(data->regmap, TIDS_REG_CTRL,
>> +				 TIDS_CTRL_AVG_MASK,
>> +				 avg_value << TIDS_CTRL_AVG_SHIFT);
>> +}
>> +
>> +static int tids_temperature1_read(struct device *dev, long *val)
>> +{
>> +	struct tids_data *data = dev_get_drvdata(dev);
>> +	u8 buf[2] = { 0 };
> 
> Seems like __le16?

Also, the initialization is unnecessary.

>> +	int ret;
>> +
>> +	ret = regmap_bulk_read(data->regmap, TIDS_REG_DATA_T_L, buf, 2);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* temperature in °mC */
>> +	*val = (((s16)(buf[1] << 8) | buf[0])) * 10;
> 
> Then __le16_to_cpu()?
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static ssize_t tids_temperature_alarm_read(struct device *dev, u32 attr,
>> +					   long *val)
>> +{
>> +	struct tids_data *data = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	if (attr == hwmon_temp_min_alarm)
>> +		ret = regmap_test_bits(data->regmap, TIDS_REG_STATUS,
>> +				       TIDS_STATUS_UNDER_TLL_MASK);
>> +	else if (attr == hwmon_temp_max_alarm)
>> +		ret = regmap_test_bits(data->regmap, TIDS_REG_STATUS,
>> +				       TIDS_STATUS_OVER_THL_MASK);
> 
> Instead of passing attr and have this if() else why not passing the proper mask? Then
> just regmap_read(regmag, reg, ...)?
> 
>> +	else
>> +		return -EOPNOTSUPP;
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	*val = ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int tids_temperature_minmax_read(struct device *dev, u32 attr, long *val)
>> +{
>> +	struct tids_data *data = dev_get_drvdata(dev);
>> +	unsigned int reg_data = 0;

Unnecessary initialization.

>> +	int ret;
>> +
>> +	if (attr == hwmon_temp_min)
>> +		ret = regmap_read(data->regmap, TIDS_REG_T_L_LIMIT, &reg_data);
>> +	else if (attr == hwmon_temp_max)
>> +		ret = regmap_read(data->regmap, TIDS_REG_T_H_LIMIT, &reg_data);
>> +	else
>> +		return -EOPNOTSUPP;
> 
> Same as above but with the proper register
> 
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* temperature from register conversion in °mC */
>> +	*val = (((u8)reg_data - 63) * 640);
> 
> Why the cast?
> 
It is actually worse than that: The compiler will take the expression
as unsigned and won't sign extend to negative values. So a register value of,
say, 62 will be reported as 255 degrees C. The cast needs to be to int for
this to work.

>> +
>> +	return 0;
>> +}
>> +
>> +static ssize_t tids_temperature_minmax_write(struct device *dev, u32 attr,
>> +					     long val)
>> +{
>> +	struct tids_data *data = dev_get_drvdata(dev);
>> +	u8 reg_data;
>> +
>> +	/* temperature in °mC */
>> +	val = clamp_val(val, -39680, 122880);
>> +	/* temperature to register conversion in °mC */
>> +	reg_data = (u8)(DIV_ROUND_CLOSEST(val, 640) + 63);
>> +
>> +	if (attr == hwmon_temp_min)
>> +		return regmap_write(data->regmap, TIDS_REG_T_L_LIMIT, reg_data);
>> +	else if (attr == hwmon_temp_max)
>> +		return regmap_write(data->regmap, TIDS_REG_T_H_LIMIT, reg_data);
>> +	else
>> +		return -EOPNOTSUPP;
> 
> Redundant else if () and else
> 

Also, as elsewhere, better to pass the register as argument.

> ...
> 
>>
>> +
>> +static int tids_init(struct tids_data *data)
>> +{
>> +	int ret;
>> +
>> +	/* Triggering soft reset */
>> +	ret = regmap_write_bits(data->regmap, TIDS_REG_SOFT_REST,
>> +				TIDS_SOFT_REST_MASK, TIDS_SOFT_REST_MASK);
>> +	if (ret < 0)
>> +		return ret;
>> +
> 
> No need for sleep some time? Typically that's defined on the datasheet.
> 
It doesn't say.

>> +	ret = regmap_clear_bits(data->regmap, TIDS_REG_SOFT_REST,
>> +				TIDS_SOFT_REST_MASK);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Allowing bulk read */
>> +	ret = regmap_write_bits(data->regmap, TIDS_REG_CTRL,
>> +				TIDS_CTRL_IF_ADD_INC_MASK,
>> +				TIDS_CTRL_IF_ADD_INC_MASK);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Set meassurement interval */

measurement

>> +	ret = regmap_clear_bits(data->regmap, TIDS_REG_CTRL,
>> +				TIDS_CTRL_AVG_MASK);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Set device to free run mode */
>> +	ret = regmap_write_bits(data->regmap, TIDS_REG_CTRL,
>> +				TIDS_CTRL_FREERUN_MASK, TIDS_CTRL_FREERUN_MASK);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Don't update temperature register until high and low value are read */
>> +	ret = regmap_write_bits(data->regmap, TIDS_REG_CTRL, TIDS_CTRL_BDU_MASK,
>> +				TIDS_CTRL_BDU_MASK);
> 
> return regmap_write_bits();
> 
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int tids_probe(struct i2c_client *client)
>> +{
>> +	struct device *device = &client->dev;
>> +	struct device *hwmon_dev;
>> +	struct tids_data *data;
>> +	unsigned int value;
>> +	int ret;
>> +
>> +	data = devm_kzalloc(device, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	data->client = client;
>> +
>> +	/* Init regmap */
> 
> The comment does not add any added value.
> 
>> +	data->regmap = devm_regmap_init_i2c(data->client, &regmap_config);
>> +	if (IS_ERR(data->regmap))
>> +		return dev_err_probe(device, PTR_ERR(data->regmap),
>> +				     "regmap initialization failed\n");
>> +
>> +	/* Read device id, to check if i2c is working */
> 
> Same
> 
>> +	ret = regmap_read(data->regmap, TIDS_REG_DEVICE_ID, &value);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (value != TIDS_ID)
>> +		return -ENODEV;
>> +
>> +	tids_init(data);
> 
> Check for the return value.
> 
>> +
>> +	hwmon_dev = devm_hwmon_device_register_with_info(device, "tids", data,
>> +							 &tids_chip_info, NULL);
>> +
>> +	return PTR_ERR_OR_ZERO(hwmon_dev);
>> +}
>> +
>> +static int tids_suspend(struct device *dev)
>> +{
>> +	struct tids_data *data = dev_get_drvdata(dev);
>> +
>> +	return regmap_clear_bits(data->regmap, TIDS_REG_CTRL,
>> +				 TIDS_CTRL_FREERUN_MASK);
>> +}
>> +
>> +static int tids_resume(struct device *dev)
>> +{
>> +	struct tids_data *data = dev_get_drvdata(dev);
>> +
>> +	return regmap_write_bits(data->regmap, TIDS_REG_CTRL,
>> +				 TIDS_CTRL_FREERUN_MASK,
>> +				 TIDS_CTRL_FREERUN_MASK);

Is this sufficient ? Does the chip possible require an init sequence
and updating the limit values ?

Check drivers/hwmon/jc42.c which saves and restores the configuration using
regmap API functions. Maybe something similar that would be useful here
as well.

>> +}
>> +
>> +static DEFINE_SIMPLE_DEV_PM_OPS(tids_dev_pm_ops, tids_resume, tids_suspend);
>> +
>> +static const struct i2c_device_id tids_id[] = {
>> +	{ "tids", 0 },
> 
> No need for 0
> 
>> +	{},
> 
> The above is already a terminator so you can drop the comma
> 
> - Nuno Sá
> 

Re: [PATCH v2 3/4] hwmon: Add driver for wsen tids

[Guenter Roeck]

On 11/19/25 04:51, Thomas Marangoni wrote:
> Add support for the wsen tids. It is a low cost
> and small-form-factor i2c temperature sensor.
> 

Additional feedback:

> +
> +static ssize_t tids_interval_write(struct device *dev, long val)
> +{
> +	struct tids_data *data = dev_get_drvdata(dev);
> +	unsigned int avg_value;
> +
> +	avg_value = find_closest_descending(val, update_intervals,
> +					    ARRAY_SIZE(update_intervals));
> +

Turns out find_closest_descending() can not handle large negative values
(close to the limit) correctly. val needs to be clamped to a reasonable range
(say, [0, 100]) before passing it to find_closest_descending().

> +	return regmap_write_bits(data->regmap, TIDS_REG_CTRL,
> +				 TIDS_CTRL_AVG_MASK,
> +				 avg_value << TIDS_CTRL_AVG_SHIFT);
> +}
> +
> +static int tids_temperature1_read(struct device *dev, long *val)

The "1" in the function name is really not needed here.

> +{
> +	struct tids_data *data = dev_get_drvdata(dev);
> +	u8 buf[2] = { 0 };
> +	int ret;
> +
> +	ret = regmap_bulk_read(data->regmap, TIDS_REG_DATA_T_L, buf, 2);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* temperature in °mC */
> +	*val = (((s16)(buf[1] << 8) | buf[0])) * 10;
> +
> +	return 0;
> +}
> +
> +static ssize_t tids_temperature_alarm_read(struct device *dev, u32 attr,
> +					   long *val)
> +{
> +	struct tids_data *data = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (attr == hwmon_temp_min_alarm)
> +		ret = regmap_test_bits(data->regmap, TIDS_REG_STATUS,
> +				       TIDS_STATUS_UNDER_TLL_MASK);
> +	else if (attr == hwmon_temp_max_alarm)
> +		ret = regmap_test_bits(data->regmap, TIDS_REG_STATUS,
> +				       TIDS_STATUS_OVER_THL_MASK);
> +	else
> +		return -EOPNOTSUPP;
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = ret;
> +
> +	return 0;
> +}
> +
> +static int tids_temperature_minmax_read(struct device *dev, u32 attr, long *val)
> +{
> +	struct tids_data *data = dev_get_drvdata(dev);
> +	unsigned int reg_data = 0;
> +	int ret;
> +
> +	if (attr == hwmon_temp_min)
> +		ret = regmap_read(data->regmap, TIDS_REG_T_L_LIMIT, &reg_data);
> +	else if (attr == hwmon_temp_max)
> +		ret = regmap_read(data->regmap, TIDS_REG_T_H_LIMIT, &reg_data);
> +	else
> +		return -EOPNOTSUPP;
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	/* temperature from register conversion in °mC */
> +	*val = (((u8)reg_data - 63) * 640);
> +
> +	return 0;
> +}
> +
> +static ssize_t tids_temperature_minmax_write(struct device *dev, u32 attr,
> +					     long val)
> +{
> +	struct tids_data *data = dev_get_drvdata(dev);
> +	u8 reg_data;
> +
> +	/* temperature in °mC */
> +	val = clamp_val(val, -39680, 122880);

(0 - 63) * 640 = -40320

While this is a bit below the "official" limit, it is the default value in
the chip register. Writing a limit that is read from the chip should be supported,
so the range should be clamped to [-40320, 122880].

Thanks,
Guenter

Re: [PATCH v2 1/4] dt-bindings: Add trivial-devices for WSEN-tids temperature sensor and wsen as vendor-prefix

[Krzysztof Kozlowski]

On Wed, Nov 19, 2025 at 01:51:38PM +0100, Thomas Marangoni wrote:
> WSEN-tids only has an i2c interface and an optional interrupt. This
> should qualify the sensor as trivial device.
> 
> WSEN is a subcompany of Würth Electronics specialized on IC sensors.
> 
> Signed-off-by: Thomas Marangoni <Thomas.Marangoni@becom-group.com>
> ---
>  Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>  2 files changed, 4 insertions(+)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

<form letter>
This is an automated instruction, just in case, because many review
tags are being ignored. If you know the process, just skip it entirely
(please do not feel offended by me posting it here - no bad intentions
intended, no patronizing, I just want to avoid wasted efforts). If you
do not know the process, here is a short explanation:

Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions of patchset, under or above your Signed-off-by tag, unless
patch changed significantly (e.g. new properties added to the DT
bindings). Tag is "received", when provided in a message replied to you
on the mailing list. Tools like b4 can help here ('b4 trailers -u ...').
However, there's no need to repost patches *only* to add the tags. The
upstream maintainer will do that for tags received on the version they
apply.

https://elixir.bootlin.com/linux/v6.15/source/Documentation/process/submitting-patches.rst#L591
</form letter>

Best regards,
Krzysztof