Re: [PATCH v2 3/4] hwmon: Add driver for wsen tids
From: Guenter Roeck
Date: Wed Nov 19 2025 - 12:46:52 EST
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@xxxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxx>
+ */
+
+#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.
Unused+
+/*
+ * 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;
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.
It is actually worse than that: The compiler will take the expression+ int ret;
+
+ if (attr == hwmon_temp_min)
+ ret = regmap_read(data->regmap, TIDS_REG_T_L_LIMIT, ®_data);
+ else if (attr == hwmon_temp_max)
+ ret = regmap_read(data->regmap, TIDS_REG_T_H_LIMIT, ®_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?
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.
...It doesn't say.
+
+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 */
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, ®map_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á