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

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

[Thomas Marangoni]

This series introduces a hwmon driver for the Würth Sensor tids-2521020222501.

I've tried to implement all possible hwmon features with this sensor.
The patch series is tested again main branch of linux-next and
hwmon-staging.

Note: This is my first patch series, I tried to follow the submitting
guide, as close as possible. If something is missing, please let me
know.

Thomas Marangoni (4):
  hwmon: Add driver for wsen tids-2521020222501
  hwmon: documentation: add tids
  dt-bindings: Add tids in bindings and wsen as vendor
  MAINTAINERS: Add tids driver as maintained

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


base-commit: 0c1c7a6a83feaf2cf182c52983ffe330ffb50280
-- 
2.51.1

[PATCH 1/4] hwmon: Add driver for wsen tids-2521020222501

[Thomas Marangoni]

This commit adds support for the wsen tids-2521020222501. 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_hyst
- hwmon_temp_max_hyst
- hwmon_chip_update_interval

Additional notes:
- The update interval only supports four fixed values.
- The alarm is reset on reading.
---
 drivers/hwmon/Kconfig  |  10 +
 drivers/hwmon/Makefile |   1 +
 drivers/hwmon/tids.c   | 560 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 571 insertions(+)
 create mode 100644 drivers/hwmon/tids.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 8c852bcac26f..5e578241001f 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2647,6 +2647,16 @@ config SENSORS_WM8350
 	  This driver can also be built as a module. If so, the module
 	  will be called wm8350-hwmon.
 
+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_ULTRA45
 	tristate "Sun Ultra45 PIC16F747"
 	depends on SPARC64
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index a8de5bc69f2a..def052b2bdfa 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -244,6 +244,7 @@ obj-$(CONFIG_SENSORS_W83L785TS)	+= w83l785ts.o
 obj-$(CONFIG_SENSORS_W83L786NG)	+= w83l786ng.o
 obj-$(CONFIG_SENSORS_WM831X)	+= wm831x-hwmon.o
 obj-$(CONFIG_SENSORS_WM8350)	+= wm8350-hwmon.o
+obj-$(CONFIG_SENSORS_TIDS) += tids.o
 obj-$(CONFIG_SENSORS_XGENE)	+= xgene-hwmon.o
 
 obj-$(CONFIG_SENSORS_OCC)	+= occ/
diff --git a/drivers/hwmon/tids.c b/drivers/hwmon/tids.c
new file mode 100644
index 000000000000..0176a5e8b574
--- /dev/null
+++ b/drivers/hwmon/tids.c
@@ -0,0 +1,560 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * Copyright (c) BECOM Electronics GmbH
+ *
+ * wsen_tids_2521020222501.c - Linux hwmon driver for WSEN-TIDS
+ *                             2521020222501 Temperature sensor
+ *
+ * Author: Thomas Marangoni <thomas.marangoni@becom-group.com>
+ */
+
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+/*
+ * TIDS registers
+ */
+#define TIDS_CMD_DEVICE_ID	0x01
+#define TIDS_CMD_T_H_LIMIT	0x02
+#define TIDS_CMD_T_L_LIMIT	0x03
+#define TIDS_CMD_CTRL		0x04
+#define TIDS_CMD_STATUS		0x05
+#define TIDS_CMD_DATA_T_L	0x06
+#define TIDS_CMD_DATA_T_H	0x07
+#define TIDS_CMD_SOFT_REST	0x0C
+
+/*
+ * TIDS device IDs
+ */
+#define TIDS_ID_2521020222501 0xa0
+
+enum tids_update_interval {
+	tids_update_interval_40ms = 0,
+	tids_update_interval_20ms = 1,
+	tids_update_interval_10ms = 2,
+	tids_update_interval_5ms = 3,
+};
+
+/* TIDS control register */
+static const struct reg_field tids_reg_field_ctrl_one_shot =
+	REG_FIELD(TIDS_CMD_CTRL, 0, 0);
+static const struct reg_field tids_reg_field_ctrl_freerun =
+	REG_FIELD(TIDS_CMD_CTRL, 2, 2);
+static const struct reg_field tids_reg_field_ctrl_if_add_inc =
+	REG_FIELD(TIDS_CMD_CTRL, 3, 3);
+static const struct reg_field tids_reg_field_ctrl_avg =
+	REG_FIELD(TIDS_CMD_CTRL, 4, 5);
+static const struct reg_field tids_reg_field_ctrl_bdu =
+	REG_FIELD(TIDS_CMD_CTRL, 6, 6);
+/* TIDS status register */
+static const struct reg_field tids_reg_field_status_busy =
+	REG_FIELD(TIDS_CMD_STATUS, 0, 0);
+static const struct reg_field tids_reg_field_status_over_thl =
+	REG_FIELD(TIDS_CMD_STATUS, 1, 1);
+static const struct reg_field tids_reg_field_status_under_tll =
+	REG_FIELD(TIDS_CMD_STATUS, 2, 2);
+/* TIDS reset register */
+static const struct reg_field tids_reg_field_soft_reset =
+	REG_FIELD(TIDS_CMD_SOFT_REST, 1, 1);
+
+struct tids_data {
+	struct i2c_client *client;
+
+	struct regmap *regmap;
+
+	/* regmap field for ctrl register */
+	struct regmap_field *reg_ctrl_one_shot;
+	struct regmap_field *reg_ctrl_freerun;
+	struct regmap_field *reg_ctrl_if_add_inc;
+	struct regmap_field *reg_ctrl_avg;
+	struct regmap_field *reg_ctrl_bdu;
+
+	/* regmap field for status register */
+	struct regmap_field *reg_status_busy;
+	struct regmap_field *reg_status_over_thl;
+	struct regmap_field *reg_status_under_tll;
+
+	/* regmap field for soft reset register*/
+	struct regmap_field *reg_soft_reset;
+
+	int irq;
+	int temperature;
+};
+
+static ssize_t tids_interval_read(struct device *dev, long *val)
+{
+	int ret = 0;
+	unsigned int avg_value = 0;
+	struct tids_data *data = dev_get_drvdata(dev);
+
+	ret = regmap_field_read(data->reg_ctrl_avg, &avg_value);
+	if (ret < 0)
+		return ret;
+
+	switch (avg_value) {
+	case tids_update_interval_40ms:
+		*val = 40;
+		break;
+	case tids_update_interval_20ms:
+		*val = 20;
+		break;
+	case tids_update_interval_10ms:
+		*val = 10;
+		break;
+	case tids_update_interval_5ms:
+		*val = 5;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static ssize_t tids_interval_write(struct device *dev, long val)
+{
+	unsigned int avg_value = 0;
+	struct tids_data *data = dev_get_drvdata(dev);
+
+	switch (val) {
+	case 40:
+		avg_value = tids_update_interval_40ms;
+		break;
+	case 20:
+		avg_value = tids_update_interval_20ms;
+		break;
+	case 10:
+		avg_value = tids_update_interval_10ms;
+		break;
+	case 5:
+		avg_value = tids_update_interval_5ms;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return regmap_field_write(data->reg_ctrl_avg, avg_value);
+}
+
+static int tids_temperature1_read(struct device *dev, long *val)
+{
+	int ret;
+	u8 buf[2] = { 0 };
+	struct tids_data *data = dev_get_drvdata(dev);
+
+	ret = regmap_bulk_read(data->regmap, TIDS_CMD_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)
+{
+	int ret = 0;
+	unsigned int reg_data = 0;
+	struct tids_data *data = dev_get_drvdata(dev);
+
+	if (attr == hwmon_temp_min_alarm)
+		ret = regmap_field_read(data->reg_status_under_tll, &reg_data);
+	else if (attr == hwmon_temp_max_alarm)
+		ret = regmap_field_read(data->reg_status_over_thl, &reg_data);
+	else
+		return -EOPNOTSUPP;
+
+	if (ret < 0)
+		return ret;
+
+	*val = reg_data;
+
+	return 0;
+}
+
+static int tids_temperature_hyst_read(struct device *dev, u32 attr, long *val)
+{
+	int ret;
+	struct tids_data *data = dev_get_drvdata(dev);
+	unsigned int reg_data = 0;
+
+	if (attr == hwmon_temp_min_hyst)
+		ret = regmap_read(data->regmap, TIDS_CMD_T_L_LIMIT, &reg_data);
+	else if (attr == hwmon_temp_max_hyst)
+		ret = regmap_read(data->regmap, TIDS_CMD_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_hyst_write(struct device *dev, u32 attr,
+					   long val)
+{
+	u8 reg_data = 0;
+	struct tids_data *data = dev_get_drvdata(dev);
+
+	// temperature in °mC
+	if (val < -39680 || val > 122880)
+		return -EINVAL;
+
+	// temperature to register conversion in °mC
+	reg_data = (u8)((val / 640) + 63);
+
+	if (attr == hwmon_temp_min_hyst)
+		return regmap_write(data->regmap, TIDS_CMD_T_L_LIMIT, reg_data);
+	else if (attr == hwmon_temp_max_hyst)
+		return regmap_write(data->regmap, TIDS_CMD_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_hyst:
+		case hwmon_temp_max_hyst:
+			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_hyst:
+		case hwmon_temp_max_hyst:
+			return tids_temperature_hyst_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_hyst:
+		case hwmon_temp_max_hyst:
+			return tids_temperature_hyst_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_HYST |
+					 HWMON_T_MAX_HYST),
+	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 >= 0x02 && reg <= 0x04)
+		return true;
+
+	if (reg == 0x0c)
+		return true;
+
+	return false;
+}
+
+static bool tids_regmap_readable_reg(struct device *dev, unsigned int reg)
+{
+	if (reg >= 0x01 && reg <= 0x07)
+		return true;
+
+	if (reg == 0x0c)
+		return true;
+
+	return false;
+}
+
+static bool tids_regmap_volatile_reg(struct device *dev, unsigned int reg)
+{
+	if (reg >= 0x05 && reg <= 0x07)
+		return true;
+
+	if (reg == 0x0c)
+		return true;
+
+	return false;
+}
+
+static const struct regmap_config regmap_config = {
+	.reg_bits			= 8,
+	.val_bits			= 8,
+	.max_register		= TIDS_CMD_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_probe_regmap(struct tids_data *data)
+{
+	struct device *dev = &data->client->dev;
+
+	/* Init regmap */
+	data->regmap = devm_regmap_init_i2c(data->client, &regmap_config);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(dev, PTR_ERR(data->regmap),
+				     "regmap initialization failed\n");
+
+	/* Allocate regmap_field for ctrl register */
+	data->reg_ctrl_one_shot = devm_regmap_field_alloc(
+		dev, data->regmap, tids_reg_field_ctrl_one_shot);
+	if (IS_ERR(data->reg_ctrl_one_shot))
+		return dev_err_probe(
+			dev, PTR_ERR(data->reg_ctrl_one_shot),
+			"regmap_field reg_ctrl_one_shot initialization failed\n");
+
+	data->reg_ctrl_freerun = devm_regmap_field_alloc(
+		dev, data->regmap, tids_reg_field_ctrl_freerun);
+	if (IS_ERR(data->reg_ctrl_freerun))
+		return dev_err_probe(
+			dev, PTR_ERR(data->reg_ctrl_freerun),
+			"regmap_field reg_ctrl_freerun initialization failed\n");
+
+	data->reg_ctrl_if_add_inc = devm_regmap_field_alloc(
+		dev, data->regmap, tids_reg_field_ctrl_if_add_inc);
+	if (IS_ERR(data->reg_ctrl_if_add_inc))
+		return dev_err_probe(
+			dev, PTR_ERR(data->reg_ctrl_if_add_inc),
+			"regmap_field reg_ctrl_if_add_inc initialization failed\n");
+
+	data->reg_ctrl_avg = devm_regmap_field_alloc(dev, data->regmap,
+						     tids_reg_field_ctrl_avg);
+	if (IS_ERR(data->reg_ctrl_avg))
+		return dev_err_probe(
+			dev, PTR_ERR(data->reg_ctrl_avg),
+			"regmap_field reg_ctrl_avg initialization failed\n");
+
+	data->reg_ctrl_bdu = devm_regmap_field_alloc(dev, data->regmap,
+						     tids_reg_field_ctrl_bdu);
+	if (IS_ERR(data->reg_ctrl_bdu))
+		return dev_err_probe(
+			dev, PTR_ERR(data->reg_ctrl_bdu),
+			"regmap_field reg_ctrl_bdu initialization failed\n");
+
+	/* Allocate regmap_field for status register */
+	data->reg_status_busy = devm_regmap_field_alloc(
+		dev, data->regmap, tids_reg_field_status_busy);
+	if (IS_ERR(data->reg_status_busy))
+		return dev_err_probe(
+			dev, PTR_ERR(data->reg_status_busy),
+			"regmap_field reg_status_busy initialization failed\n");
+
+	data->reg_status_over_thl = devm_regmap_field_alloc(
+		dev, data->regmap, tids_reg_field_status_over_thl);
+	if (IS_ERR(data->reg_status_over_thl))
+		return dev_err_probe(
+			dev, PTR_ERR(data->reg_status_over_thl),
+			"regmap_field reg_status_over_thl initialization failed\n");
+
+	data->reg_status_under_tll = devm_regmap_field_alloc(
+		dev, data->regmap, tids_reg_field_status_under_tll);
+	if (IS_ERR(data->reg_status_under_tll))
+		return dev_err_probe(
+			dev, PTR_ERR(data->reg_status_under_tll),
+			"regmap_field reg_status_under_tll initialization failed\n");
+
+	/* Allocate regmap_field for software_reset */
+	data->reg_soft_reset = devm_regmap_field_alloc(
+		dev, data->regmap, tids_reg_field_soft_reset);
+	if (IS_ERR(data->reg_soft_reset))
+		return dev_err_probe(
+			dev, PTR_ERR(data->reg_soft_reset),
+			"regmap_field reg_soft_reset initialization failed\n");
+
+	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 = 0;
+
+	data = devm_kzalloc(device, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->client = client;
+
+	ret = tids_probe_regmap(data);
+	if (ret != 0)
+		return ret;
+
+	/* Read device id, to check if i2c is working */
+	ret = regmap_read(data->regmap, TIDS_CMD_DEVICE_ID, &value);
+	if (ret < 0)
+		return ret;
+
+	/* Triggering soft reset */
+	ret = regmap_field_write(data->reg_soft_reset, 1);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_field_write(data->reg_soft_reset, 0);
+	if (ret < 0)
+		return ret;
+
+	/* Allowing bulk read */
+	ret = regmap_field_write(data->reg_ctrl_if_add_inc, 1);
+	if (ret < 0)
+		return ret;
+
+	/* Set meassurement interval */
+	ret = regmap_field_write(data->reg_ctrl_avg, tids_update_interval_40ms);
+	if (ret < 0)
+		return ret;
+
+	/* Set device to free run mode */
+	ret = regmap_field_write(data->reg_ctrl_freerun, 1);
+	if (ret < 0)
+		return ret;
+
+	/* Don't update temperature register until high and low value are read */
+	ret = regmap_field_write(data->reg_ctrl_bdu, 1);
+	if (ret < 0)
+		return ret;
+
+	hwmon_dev = devm_hwmon_device_register_with_info(
+		device, device->driver->name, 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_field_write(data->reg_ctrl_freerun, 0);
+}
+
+static int tids_resume(struct device *dev)
+{
+	struct tids_data *data = dev_get_drvdata(dev);
+
+	return regmap_field_write(data->reg_ctrl_freerun, 1);
+}
+
+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-2521020222501" },
+	{}
+};
+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

Re: [PATCH 1/4] hwmon: Add driver for wsen tids-2521020222501

[Guenter Roeck]

On 11/17/25 04:38, Thomas Marangoni wrote:
> This commit adds support for the wsen tids-2521020222501. It is a

What is the relevance of "-2521020222501" ? As far as I can see it is just
the order code, and the sensor is just "WSEN-TIDS", as suggested by the
documentation. I would sugest to drop the number unless it has some actual
relevance - and, if it does, provide a rationale for it that is better than
"this is the oder code". The order code can change anytime, after all.

> 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_hyst
> - hwmon_temp_max_hyst
> - hwmon_chip_update_interval
> 
> Additional notes:
> - The update interval only supports four fixed values.
> - The alarm is reset on reading.
> ---

If available, please send me a register dump so I can implement unit test code.


>   drivers/hwmon/Kconfig  |  10 +
>   drivers/hwmon/Makefile |   1 +
>   drivers/hwmon/tids.c   | 560 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 571 insertions(+)
>   create mode 100644 drivers/hwmon/tids.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 8c852bcac26f..5e578241001f 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -2647,6 +2647,16 @@ config SENSORS_WM8350
>   	  This driver can also be built as a module. If so, the module
>   	  will be called wm8350-hwmon.
>   
> +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_ULTRA45
>   	tristate "Sun Ultra45 PIC16F747"
>   	depends on SPARC64
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index a8de5bc69f2a..def052b2bdfa 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -244,6 +244,7 @@ obj-$(CONFIG_SENSORS_W83L785TS)	+= w83l785ts.o
>   obj-$(CONFIG_SENSORS_W83L786NG)	+= w83l786ng.o
>   obj-$(CONFIG_SENSORS_WM831X)	+= wm831x-hwmon.o
>   obj-$(CONFIG_SENSORS_WM8350)	+= wm8350-hwmon.o
> +obj-$(CONFIG_SENSORS_TIDS) += tids.o
>   obj-$(CONFIG_SENSORS_XGENE)	+= xgene-hwmon.o
>   
>   obj-$(CONFIG_SENSORS_OCC)	+= occ/
> diff --git a/drivers/hwmon/tids.c b/drivers/hwmon/tids.c
> new file mode 100644
> index 000000000000..0176a5e8b574
> --- /dev/null
> +++ b/drivers/hwmon/tids.c
> @@ -0,0 +1,560 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Copyright (c) BECOM Electronics GmbH
> + *
> + * wsen_tids_2521020222501.c - Linux hwmon driver for WSEN-TIDS
> + *                             2521020222501 Temperature sensor
> + *
> + * Author: Thomas Marangoni <thomas.marangoni@becom-group.com>
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>

Unnecessary include.

> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +/*
> + * TIDS registers
> + */
> +#define TIDS_CMD_DEVICE_ID	0x01
> +#define TIDS_CMD_T_H_LIMIT	0x02
> +#define TIDS_CMD_T_L_LIMIT	0x03
> +#define TIDS_CMD_CTRL		0x04
> +#define TIDS_CMD_STATUS		0x05
> +#define TIDS_CMD_DATA_T_L	0x06
> +#define TIDS_CMD_DATA_T_H	0x07
> +#define TIDS_CMD_SOFT_REST	0x0C
> +
> +/*
> + * TIDS device IDs
> + */
> +#define TIDS_ID_2521020222501 0xa0
> +
Not used.

> +enum tids_update_interval {
> +	tids_update_interval_40ms = 0,
> +	tids_update_interval_20ms = 1,
> +	tids_update_interval_10ms = 2,
> +	tids_update_interval_5ms = 3,
> +};
> +
> +/* TIDS control register */
> +static const struct reg_field tids_reg_field_ctrl_one_shot =
> +	REG_FIELD(TIDS_CMD_CTRL, 0, 0);
> +static const struct reg_field tids_reg_field_ctrl_freerun =
> +	REG_FIELD(TIDS_CMD_CTRL, 2, 2);
> +static const struct reg_field tids_reg_field_ctrl_if_add_inc =
> +	REG_FIELD(TIDS_CMD_CTRL, 3, 3);
> +static const struct reg_field tids_reg_field_ctrl_avg =
> +	REG_FIELD(TIDS_CMD_CTRL, 4, 5);
> +static const struct reg_field tids_reg_field_ctrl_bdu =
> +	REG_FIELD(TIDS_CMD_CTRL, 6, 6);
> +/* TIDS status register */
> +static const struct reg_field tids_reg_field_status_busy =
> +	REG_FIELD(TIDS_CMD_STATUS, 0, 0);
> +static const struct reg_field tids_reg_field_status_over_thl =
> +	REG_FIELD(TIDS_CMD_STATUS, 1, 1);
> +static const struct reg_field tids_reg_field_status_under_tll =
> +	REG_FIELD(TIDS_CMD_STATUS, 2, 2);
> +/* TIDS reset register */
> +static const struct reg_field tids_reg_field_soft_reset =
> +	REG_FIELD(TIDS_CMD_SOFT_REST, 1, 1);
> +

I seem to be missing something. Why would it make sense to allocate
all those regmap fields and not just use regmap_update_bits() ?
I don't see any benefit in that complexity, especially for single-bit
"fields".

> +struct tids_data {
> +	struct i2c_client *client;
> +
> +	struct regmap *regmap;
> +
> +	/* regmap field for ctrl register */
> +	struct regmap_field *reg_ctrl_one_shot;
> +	struct regmap_field *reg_ctrl_freerun;
> +	struct regmap_field *reg_ctrl_if_add_inc;
> +	struct regmap_field *reg_ctrl_avg;
> +	struct regmap_field *reg_ctrl_bdu;
> +
> +	/* regmap field for status register */
> +	struct regmap_field *reg_status_busy;
> +	struct regmap_field *reg_status_over_thl;
> +	struct regmap_field *reg_status_under_tll;
> +
> +	/* regmap field for soft reset register*/
> +	struct regmap_field *reg_soft_reset;
> +
> +	int irq;
> +	int temperature;
> +};
> +
> +static ssize_t tids_interval_read(struct device *dev, long *val)
> +{
> +	int ret = 0;
> +	unsigned int avg_value = 0;

Unnecessary initializations.

> +	struct tids_data *data = dev_get_drvdata(dev);
> +
> +	ret = regmap_field_read(data->reg_ctrl_avg, &avg_value);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (avg_value) {
> +	case tids_update_interval_40ms:
> +		*val = 40;
> +		break;
> +	case tids_update_interval_20ms:
> +		*val = 20;
> +		break;
> +	case tids_update_interval_10ms:
> +		*val = 10;
> +		break;
> +	case tids_update_interval_5ms:
> +		*val = 5;
> +		break;
> +	default:
> +		return -EINVAL;

Reading a value from a chip can not return -EINVAL.
EINVAL is "Invalid argument", or bad user input, not bad data from a chip.

On top of that, the regmap field is defined as two bits. The value returned
can not be out of range. A simple array read would do the trick.

static u8 update_intervals[] = { 40, 20, 10, 5 };

return update_intervals[avg_value];

> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t tids_interval_write(struct device *dev, long val)
> +{
> +	unsigned int avg_value = 0;
> +	struct tids_data *data = dev_get_drvdata(dev);

Again, please avoid unnecessary variable initializations. I am not going to
mention this again; please fix everywhere.

Also, please use "reverse christmas tree" (longest declaration first)
for variable declarations.

> +
> +	switch (val) {
> +	case 40:
> +		avg_value = tids_update_interval_40ms;
> +		break;
> +	case 20:
> +		avg_value = tids_update_interval_20ms;
> +		break;
> +	case 10:
> +		avg_value = tids_update_interval_10ms;
> +		break;
> +	case 5:
> +		avg_value = tids_update_interval_5ms;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +

This code should, similar to other drivers, approximate the provided value.
instead of letting the user figure out valid values. I would suggest to use
find_closest() or find_closest_descending() to determine valid values.

> +	return regmap_field_write(data->reg_ctrl_avg, avg_value);
> +}
> +
> +static int tids_temperature1_read(struct device *dev, long *val)
> +{
> +	int ret;
> +	u8 buf[2] = { 0 };
> +	struct tids_data *data = dev_get_drvdata(dev);
> +
> +	ret = regmap_bulk_read(data->regmap, TIDS_CMD_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)
> +{
> +	int ret = 0;
> +	unsigned int reg_data = 0;
> +	struct tids_data *data = dev_get_drvdata(dev);
> +
> +	if (attr == hwmon_temp_min_alarm)
> +		ret = regmap_field_read(data->reg_status_under_tll, &reg_data);
> +	else if (attr == hwmon_temp_max_alarm)
> +		ret = regmap_field_read(data->reg_status_over_thl, &reg_data);
> +	else
> +		return -EOPNOTSUPP;
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = reg_data;
> +
> +	return 0;
> +}
> +
> +static int tids_temperature_hyst_read(struct device *dev, u32 attr, long *val)
> +{
> +	int ret;
> +	struct tids_data *data = dev_get_drvdata(dev);
> +	unsigned int reg_data = 0;
> +
> +	if (attr == hwmon_temp_min_hyst)
> +		ret = regmap_read(data->regmap, TIDS_CMD_T_L_LIMIT, &reg_data);
> +	else if (attr == hwmon_temp_max_hyst)
> +		ret = regmap_read(data->regmap, TIDS_CMD_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_hyst_write(struct device *dev, u32 attr,
> +					   long val)
> +{
> +	u8 reg_data = 0;
> +	struct tids_data *data = dev_get_drvdata(dev);
> +
> +	// temperature in °mC
> +	if (val < -39680 || val > 122880)
> +		return -EINVAL;

Please use clamp_val().

> +
> +	// temperature to register conversion in °mC

You are using c++ comment style for single-line comments and C comment style for
multi-line comments. I am not a friend of C++-style comments, but I accept it.
However, I do ask for consistency. Either/or, please, but not both.

> +	reg_data = (u8)((val / 640) + 63);

Candidate for DIV_ROUND_CLOSEST() ?

> +
> +	if (attr == hwmon_temp_min_hyst)
> +		return regmap_write(data->regmap, TIDS_CMD_T_L_LIMIT, reg_data);
> +	else if (attr == hwmon_temp_max_hyst)
> +		return regmap_write(data->regmap, TIDS_CMD_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_hyst:
> +		case hwmon_temp_max_hyst:
> +			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_hyst:
> +		case hwmon_temp_max_hyst:
> +			return tids_temperature_hyst_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_hyst:
> +		case hwmon_temp_max_hyst:
> +			return tids_temperature_hyst_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_HYST |
> +					 HWMON_T_MAX_HYST),

Why do you use "hyst" for limit attributes ? A hysteresis without limit does
not make sense. The datasheet says that those are limits (thresholds),
not hysteresis values. The datasheet doesn't even mention the term
"hysteresis".

> +	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 >= 0x02 && reg <= 0x04)
> +		return true;
> +
> +	if (reg == 0x0c)
> +		return true;
> +

Registers are defined. Please use the definitions here and in the functions below.

> +	return false;
> +}
> +
> +static bool tids_regmap_readable_reg(struct device *dev, unsigned int reg)
> +{
> +	if (reg >= 0x01 && reg <= 0x07)
> +		return true;
> +
> +	if (reg == 0x0c)
> +		return true;
> +
> +	return false;
> +}
> +
> +static bool tids_regmap_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	if (reg >= 0x05 && reg <= 0x07)
> +		return true;
> +
> +	if (reg == 0x0c)
> +		return true;
> +
> +	return false;
> +}
> +
> +static const struct regmap_config regmap_config = {
> +	.reg_bits			= 8,
> +	.val_bits			= 8,
> +	.max_register		= TIDS_CMD_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_probe_regmap(struct tids_data *data)
> +{
> +	struct device *dev = &data->client->dev;
> +
> +	/* Init regmap */
> +	data->regmap = devm_regmap_init_i2c(data->client, &regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(dev, PTR_ERR(data->regmap),
> +				     "regmap initialization failed\n");
> +
> +	/* Allocate regmap_field for ctrl register */
> +	data->reg_ctrl_one_shot = devm_regmap_field_alloc(
> +		dev, data->regmap, tids_reg_field_ctrl_one_shot);
> +	if (IS_ERR(data->reg_ctrl_one_shot))
> +		return dev_err_probe(
> +			dev, PTR_ERR(data->reg_ctrl_one_shot),
> +			"regmap_field reg_ctrl_one_shot initialization failed\n");
> +
> +	data->reg_ctrl_freerun = devm_regmap_field_alloc(
> +		dev, data->regmap, tids_reg_field_ctrl_freerun);
> +	if (IS_ERR(data->reg_ctrl_freerun))
> +		return dev_err_probe(
> +			dev, PTR_ERR(data->reg_ctrl_freerun),
> +			"regmap_field reg_ctrl_freerun initialization failed\n");
> +
> +	data->reg_ctrl_if_add_inc = devm_regmap_field_alloc(
> +		dev, data->regmap, tids_reg_field_ctrl_if_add_inc);
> +	if (IS_ERR(data->reg_ctrl_if_add_inc))
> +		return dev_err_probe(
> +			dev, PTR_ERR(data->reg_ctrl_if_add_inc),
> +			"regmap_field reg_ctrl_if_add_inc initialization failed\n");
> +
> +	data->reg_ctrl_avg = devm_regmap_field_alloc(dev, data->regmap,
> +						     tids_reg_field_ctrl_avg);
> +	if (IS_ERR(data->reg_ctrl_avg))
> +		return dev_err_probe(
> +			dev, PTR_ERR(data->reg_ctrl_avg),
> +			"regmap_field reg_ctrl_avg initialization failed\n");
> +
> +	data->reg_ctrl_bdu = devm_regmap_field_alloc(dev, data->regmap,
> +						     tids_reg_field_ctrl_bdu);
> +	if (IS_ERR(data->reg_ctrl_bdu))
> +		return dev_err_probe(
> +			dev, PTR_ERR(data->reg_ctrl_bdu),
> +			"regmap_field reg_ctrl_bdu initialization failed\n");
> +
> +	/* Allocate regmap_field for status register */
> +	data->reg_status_busy = devm_regmap_field_alloc(
> +		dev, data->regmap, tids_reg_field_status_busy);
> +	if (IS_ERR(data->reg_status_busy))
> +		return dev_err_probe(
> +			dev, PTR_ERR(data->reg_status_busy),
> +			"regmap_field reg_status_busy initialization failed\n");
> +
> +	data->reg_status_over_thl = devm_regmap_field_alloc(
> +		dev, data->regmap, tids_reg_field_status_over_thl);
> +	if (IS_ERR(data->reg_status_over_thl))
> +		return dev_err_probe(
> +			dev, PTR_ERR(data->reg_status_over_thl),
> +			"regmap_field reg_status_over_thl initialization failed\n");
> +
> +	data->reg_status_under_tll = devm_regmap_field_alloc(
> +		dev, data->regmap, tids_reg_field_status_under_tll);
> +	if (IS_ERR(data->reg_status_under_tll))
> +		return dev_err_probe(
> +			dev, PTR_ERR(data->reg_status_under_tll),
> +			"regmap_field reg_status_under_tll initialization failed\n");
> +
> +	/* Allocate regmap_field for software_reset */
> +	data->reg_soft_reset = devm_regmap_field_alloc(
> +		dev, data->regmap, tids_reg_field_soft_reset);
> +	if (IS_ERR(data->reg_soft_reset))
> +		return dev_err_probe(
> +			dev, PTR_ERR(data->reg_soft_reset),
> +			"regmap_field reg_soft_reset initialization failed\n");

Following up on the above, that is a lot of code just to avoid using regmap_update_bits().
Again, what am I missing ?

> +
> +	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 = 0;
> +
> +	data = devm_kzalloc(device, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +
> +	ret = tids_probe_regmap(data);
> +	if (ret != 0)
> +		return ret;
> +
> +	/* Read device id, to check if i2c is working */
> +	ret = regmap_read(data->regmap, TIDS_CMD_DEVICE_ID, &value);
> +	if (ret < 0)
> +		return ret;

Why read this but not check it ?

> +
> +	/* Triggering soft reset */
> +	ret = regmap_field_write(data->reg_soft_reset, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_field_write(data->reg_soft_reset, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Allowing bulk read */
> +	ret = regmap_field_write(data->reg_ctrl_if_add_inc, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Set meassurement interval */
> +	ret = regmap_field_write(data->reg_ctrl_avg, tids_update_interval_40ms);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Set device to free run mode */
> +	ret = regmap_field_write(data->reg_ctrl_freerun, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Don't update temperature register until high and low value are read */
> +	ret = regmap_field_write(data->reg_ctrl_bdu, 1);
> +	if (ret < 0)
> +		return ret;
> +

Please move this code into a separate _init function. Also, is it really necessary
to write all those control register values bit by bit ? If so, that should be explained
since it adds a lot of non-obvious complexity to the code.

> +	hwmon_dev = devm_hwmon_device_register_with_info(
> +		device, device->driver->name, data, &tids_chip_info, NULL);

Just use "tids" instead of "device->driver->name".

> +
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static int tids_suspend(struct device *dev)
> +{
> +	struct tids_data *data = dev_get_drvdata(dev);
> +
> +	return regmap_field_write(data->reg_ctrl_freerun, 0);
> +}
> +
> +static int tids_resume(struct device *dev)
> +{
> +	struct tids_data *data = dev_get_drvdata(dev);
> +
> +	return regmap_field_write(data->reg_ctrl_freerun, 1);
> +}
> +
> +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-2521020222501" },
> +	{}
> +};
> +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");

Re: Re: [PATCH 1/4] hwmon: Add driver for wsen tids-2521020222501

[Thomas Marangoni]

On 11/17/25 17:24, Guenter Roeck wrote:
> On 11/17/25 04:38, Thomas Marangoni wrote:
>> This commit adds support for the wsen tids-2521020222501. It is a
>
> What is the relevance of "-2521020222501" ? As far as I can see it is 
> just
> the order code, and the sensor is just "WSEN-TIDS", as suggested by the
> documentation. I would sugest to drop the number unless it has some 
> actual
> relevance - and, if it does, provide a rationale for it that is better 
> than
> "this is the oder code". The order code can change anytime, after all.
>
Yes, you are right its just the order code. I've removed it from the 
next version
of the patch.

>> 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_hyst
>> - hwmon_temp_max_hyst
>> - hwmon_chip_update_interval
>>
>> Additional notes:
>> - The update interval only supports four fixed values.
>> - The alarm is reset on reading.
>> ---
>
> If available, please send me a register dump so I can implement unit 
> test code.

I don't know exactly what you mean with a register dump, I made one with
i2cdump. If that's not enough can you refer me to a documentation or give me
a command I can run?

This is the 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

>
>
>>   drivers/hwmon/Kconfig  |  10 +
>>   drivers/hwmon/Makefile |   1 +
>>   drivers/hwmon/tids.c   | 560 +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 571 insertions(+)
>>   create mode 100644 drivers/hwmon/tids.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 8c852bcac26f..5e578241001f 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -2647,6 +2647,16 @@ config SENSORS_WM8350
>>         This driver can also be built as a module. If so, the module
>>         will be called wm8350-hwmon.
>>
>> +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_ULTRA45
>>       tristate "Sun Ultra45 PIC16F747"
>>       depends on SPARC64
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index a8de5bc69f2a..def052b2bdfa 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -244,6 +244,7 @@ obj-$(CONFIG_SENSORS_W83L785TS)   += w83l785ts.o
>>   obj-$(CONFIG_SENSORS_W83L786NG)     += w83l786ng.o
>>   obj-$(CONFIG_SENSORS_WM831X)        += wm831x-hwmon.o
>>   obj-$(CONFIG_SENSORS_WM8350)        += wm8350-hwmon.o
>> +obj-$(CONFIG_SENSORS_TIDS) += tids.o
>>   obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o
>>
>>   obj-$(CONFIG_SENSORS_OCC)   += occ/
>> diff --git a/drivers/hwmon/tids.c b/drivers/hwmon/tids.c
>> new file mode 100644
>> index 000000000000..0176a5e8b574
>> --- /dev/null
>> +++ b/drivers/hwmon/tids.c
>> @@ -0,0 +1,560 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +/*
>> + * Copyright (c) BECOM Electronics GmbH
>> + *
>> + * wsen_tids_2521020222501.c - Linux hwmon driver for WSEN-TIDS
>> + *                             2521020222501 Temperature sensor
>> + *
>> + * Author: Thomas Marangoni <thomas.marangoni@becom-group.com>
>> + */
>> +
>> +#include <linux/bits.h>
>> +#include <linux/delay.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>
> Unnecessary include.
>
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +/*
>> + * TIDS registers
>> + */
>> +#define TIDS_CMD_DEVICE_ID   0x01
>> +#define TIDS_CMD_T_H_LIMIT   0x02
>> +#define TIDS_CMD_T_L_LIMIT   0x03
>> +#define TIDS_CMD_CTRL                0x04
>> +#define TIDS_CMD_STATUS              0x05
>> +#define TIDS_CMD_DATA_T_L    0x06
>> +#define TIDS_CMD_DATA_T_H    0x07
>> +#define TIDS_CMD_SOFT_REST   0x0C
>> +
>> +/*
>> + * TIDS device IDs
>> + */
>> +#define TIDS_ID_2521020222501 0xa0
>> +
> Not used.
>
>> +enum tids_update_interval {
>> +     tids_update_interval_40ms = 0,
>> +     tids_update_interval_20ms = 1,
>> +     tids_update_interval_10ms = 2,
>> +     tids_update_interval_5ms = 3,
>> +};
>> +
>> +/* TIDS control register */
>> +static const struct reg_field tids_reg_field_ctrl_one_shot =
>> +     REG_FIELD(TIDS_CMD_CTRL, 0, 0);
>> +static const struct reg_field tids_reg_field_ctrl_freerun =
>> +     REG_FIELD(TIDS_CMD_CTRL, 2, 2);
>> +static const struct reg_field tids_reg_field_ctrl_if_add_inc =
>> +     REG_FIELD(TIDS_CMD_CTRL, 3, 3);
>> +static const struct reg_field tids_reg_field_ctrl_avg =
>> +     REG_FIELD(TIDS_CMD_CTRL, 4, 5);
>> +static const struct reg_field tids_reg_field_ctrl_bdu =
>> +     REG_FIELD(TIDS_CMD_CTRL, 6, 6);
>> +/* TIDS status register */
>> +static const struct reg_field tids_reg_field_status_busy =
>> +     REG_FIELD(TIDS_CMD_STATUS, 0, 0);
>> +static const struct reg_field tids_reg_field_status_over_thl =
>> +     REG_FIELD(TIDS_CMD_STATUS, 1, 1);
>> +static const struct reg_field tids_reg_field_status_under_tll =
>> +     REG_FIELD(TIDS_CMD_STATUS, 2, 2);
>> +/* TIDS reset register */
>> +static const struct reg_field tids_reg_field_soft_reset =
>> +     REG_FIELD(TIDS_CMD_SOFT_REST, 1, 1);
>> +
>
> I seem to be missing something. Why would it make sense to allocate
> all those regmap fields and not just use regmap_update_bits() ?
> I don't see any benefit in that complexity, especially for single-bit
> "fields".

I was thinking it is a bit cleaner in code-style when using fields. In v2,
regfields have been removed and replaced with regmap functions.

>
>> +struct tids_data {
>> +     struct i2c_client *client;
>> +
>> +     struct regmap *regmap;
>> +
>> +     /* regmap field for ctrl register */
>> +     struct regmap_field *reg_ctrl_one_shot;
>> +     struct regmap_field *reg_ctrl_freerun;
>> +     struct regmap_field *reg_ctrl_if_add_inc;
>> +     struct regmap_field *reg_ctrl_avg;
>> +     struct regmap_field *reg_ctrl_bdu;
>> +
>> +     /* regmap field for status register */
>> +     struct regmap_field *reg_status_busy;
>> +     struct regmap_field *reg_status_over_thl;
>> +     struct regmap_field *reg_status_under_tll;
>> +
>> +     /* regmap field for soft reset register*/
>> +     struct regmap_field *reg_soft_reset;
>> +
>> +     int irq;
>> +     int temperature;
>> +};
>> +
>> +static ssize_t tids_interval_read(struct device *dev, long *val)
>> +{
>> +     int ret = 0;
>> +     unsigned int avg_value = 0;
>
> Unnecessary initializations.
>
>> +     struct tids_data *data = dev_get_drvdata(dev);
>> +
>> +     ret = regmap_field_read(data->reg_ctrl_avg, &avg_value);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     switch (avg_value) {
>> +     case tids_update_interval_40ms:
>> +             *val = 40;
>> +             break;
>> +     case tids_update_interval_20ms:
>> +             *val = 20;
>> +             break;
>> +     case tids_update_interval_10ms:
>> +             *val = 10;
>> +             break;
>> +     case tids_update_interval_5ms:
>> +             *val = 5;
>> +             break;
>> +     default:
>> +             return -EINVAL;
>
> Reading a value from a chip can not return -EINVAL.
> EINVAL is "Invalid argument", or bad user input, not bad data from a 
> chip.
>
> On top of that, the regmap field is defined as two bits. The value 
> returned
> can not be out of range. A simple array read would do the trick.
>
> static u8 update_intervals[] = { 40, 20, 10, 5 };
>
> return update_intervals[avg_value];
>
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static ssize_t tids_interval_write(struct device *dev, long val)
>> +{
>> +     unsigned int avg_value = 0;
>> +     struct tids_data *data = dev_get_drvdata(dev);
>
> Again, please avoid unnecessary variable initializations. I am not 
> going to
> mention this again; please fix everywhere.
>
> Also, please use "reverse christmas tree" (longest declaration first)
> for variable declarations.
>
>> +
>> +     switch (val) {
>> +     case 40:
>> +             avg_value = tids_update_interval_40ms;
>> +             break;
>> +     case 20:
>> +             avg_value = tids_update_interval_20ms;
>> +             break;
>> +     case 10:
>> +             avg_value = tids_update_interval_10ms;
>> +             break;
>> +     case 5:
>> +             avg_value = tids_update_interval_5ms;
>> +             break;
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +
>
> This code should, similar to other drivers, approximate the provided 
> value.
> instead of letting the user figure out valid values. I would suggest 
> to use
> find_closest() or find_closest_descending() to determine valid values.
>
>> +     return regmap_field_write(data->reg_ctrl_avg, avg_value);
>> +}
>> +
>> +static int tids_temperature1_read(struct device *dev, long *val)
>> +{
>> +     int ret;
>> +     u8 buf[2] = { 0 };
>> +     struct tids_data *data = dev_get_drvdata(dev);
>> +
>> +     ret = regmap_bulk_read(data->regmap, TIDS_CMD_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)
>> +{
>> +     int ret = 0;
>> +     unsigned int reg_data = 0;
>> +     struct tids_data *data = dev_get_drvdata(dev);
>> +
>> +     if (attr == hwmon_temp_min_alarm)
>> +             ret = regmap_field_read(data->reg_status_under_tll, 
>> &reg_data);
>> +     else if (attr == hwmon_temp_max_alarm)
>> +             ret = regmap_field_read(data->reg_status_over_thl, 
>> &reg_data);
>> +     else
>> +             return -EOPNOTSUPP;
>> +
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     *val = reg_data;
>> +
>> +     return 0;
>> +}
>> +
>> +static int tids_temperature_hyst_read(struct device *dev, u32 attr, 
>> long *val)
>> +{
>> +     int ret;
>> +     struct tids_data *data = dev_get_drvdata(dev);
>> +     unsigned int reg_data = 0;
>> +
>> +     if (attr == hwmon_temp_min_hyst)
>> +             ret = regmap_read(data->regmap, TIDS_CMD_T_L_LIMIT, 
>> &reg_data);
>> +     else if (attr == hwmon_temp_max_hyst)
>> +             ret = regmap_read(data->regmap, TIDS_CMD_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_hyst_write(struct device *dev, u32 
>> attr,
>> +                                        long val)
>> +{
>> +     u8 reg_data = 0;
>> +     struct tids_data *data = dev_get_drvdata(dev);
>> +
>> +     // temperature in °mC
>> +     if (val < -39680 || val > 122880)
>> +             return -EINVAL;
>
> Please use clamp_val().
>
>> +
>> +     // temperature to register conversion in °mC
>
> You are using c++ comment style for single-line comments and C comment 
> style for
> multi-line comments. I am not a friend of C++-style comments, but I 
> accept it.
> However, I do ask for consistency. Either/or, please, but not both.
>
>> +     reg_data = (u8)((val / 640) + 63);
>
> Candidate for DIV_ROUND_CLOSEST() ?
>
>> +
>> +     if (attr == hwmon_temp_min_hyst)
>> +             return regmap_write(data->regmap, TIDS_CMD_T_L_LIMIT, 
>> reg_data);
>> +     else if (attr == hwmon_temp_max_hyst)
>> +             return regmap_write(data->regmap, TIDS_CMD_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_hyst:
>> +             case hwmon_temp_max_hyst:
>> +                     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_hyst:
>> +             case hwmon_temp_max_hyst:
>> +                     return tids_temperature_hyst_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_hyst:
>> +             case hwmon_temp_max_hyst:
>> +                     return tids_temperature_hyst_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_HYST |
>> +                                      HWMON_T_MAX_HYST),
>
> Why do you use "hyst" for limit attributes ? A hysteresis without 
> limit does
> not make sense. The datasheet says that those are limits (thresholds),
> not hysteresis values. The datasheet doesn't even mention the term
> "hysteresis".

The documentation of the subsystem wasn't completely clear to me about which
one to pick. I changed it in v2 of this patch.

>
>> +     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 >= 0x02 && reg <= 0x04)
>> +             return true;
>> +
>> +     if (reg == 0x0c)
>> +             return true;
>> +
>
> Registers are defined. Please use the definitions here and in the 
> functions below.
>
>> +     return false;
>> +}
>> +
>> +static bool tids_regmap_readable_reg(struct device *dev, unsigned 
>> int reg)
>> +{
>> +     if (reg >= 0x01 && reg <= 0x07)
>> +             return true;
>> +
>> +     if (reg == 0x0c)
>> +             return true;
>> +
>> +     return false;
>> +}
>> +
>> +static bool tids_regmap_volatile_reg(struct device *dev, unsigned 
>> int reg)
>> +{
>> +     if (reg >= 0x05 && reg <= 0x07)
>> +             return true;
>> +
>> +     if (reg == 0x0c)
>> +             return true;
>> +
>> +     return false;
>> +}
>> +
>> +static const struct regmap_config regmap_config = {
>> +     .reg_bits                       = 8,
>> +     .val_bits                       = 8,
>> +     .max_register           = TIDS_CMD_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_probe_regmap(struct tids_data *data)
>> +{
>> +     struct device *dev = &data->client->dev;
>> +
>> +     /* Init regmap */
>> +     data->regmap = devm_regmap_init_i2c(data->client, &regmap_config);
>> +     if (IS_ERR(data->regmap))
>> +             return dev_err_probe(dev, PTR_ERR(data->regmap),
>> +                                  "regmap initialization failed\n");
>> +
>> +     /* Allocate regmap_field for ctrl register */
>> +     data->reg_ctrl_one_shot = devm_regmap_field_alloc(
>> +             dev, data->regmap, tids_reg_field_ctrl_one_shot);
>> +     if (IS_ERR(data->reg_ctrl_one_shot))
>> +             return dev_err_probe(
>> +                     dev, PTR_ERR(data->reg_ctrl_one_shot),
>> +                     "regmap_field reg_ctrl_one_shot initialization 
>> failed\n");
>> +
>> +     data->reg_ctrl_freerun = devm_regmap_field_alloc(
>> +             dev, data->regmap, tids_reg_field_ctrl_freerun);
>> +     if (IS_ERR(data->reg_ctrl_freerun))
>> +             return dev_err_probe(
>> +                     dev, PTR_ERR(data->reg_ctrl_freerun),
>> +                     "regmap_field reg_ctrl_freerun initialization 
>> failed\n");
>> +
>> +     data->reg_ctrl_if_add_inc = devm_regmap_field_alloc(
>> +             dev, data->regmap, tids_reg_field_ctrl_if_add_inc);
>> +     if (IS_ERR(data->reg_ctrl_if_add_inc))
>> +             return dev_err_probe(
>> +                     dev, PTR_ERR(data->reg_ctrl_if_add_inc),
>> +                     "regmap_field reg_ctrl_if_add_inc 
>> initialization failed\n");
>> +
>> +     data->reg_ctrl_avg = devm_regmap_field_alloc(dev, data->regmap,
>> + tids_reg_field_ctrl_avg);
>> +     if (IS_ERR(data->reg_ctrl_avg))
>> +             return dev_err_probe(
>> +                     dev, PTR_ERR(data->reg_ctrl_avg),
>> +                     "regmap_field reg_ctrl_avg initialization 
>> failed\n");
>> +
>> +     data->reg_ctrl_bdu = devm_regmap_field_alloc(dev, data->regmap,
>> + tids_reg_field_ctrl_bdu);
>> +     if (IS_ERR(data->reg_ctrl_bdu))
>> +             return dev_err_probe(
>> +                     dev, PTR_ERR(data->reg_ctrl_bdu),
>> +                     "regmap_field reg_ctrl_bdu initialization 
>> failed\n");
>> +
>> +     /* Allocate regmap_field for status register */
>> +     data->reg_status_busy = devm_regmap_field_alloc(
>> +             dev, data->regmap, tids_reg_field_status_busy);
>> +     if (IS_ERR(data->reg_status_busy))
>> +             return dev_err_probe(
>> +                     dev, PTR_ERR(data->reg_status_busy),
>> +                     "regmap_field reg_status_busy initialization 
>> failed\n");
>> +
>> +     data->reg_status_over_thl = devm_regmap_field_alloc(
>> +             dev, data->regmap, tids_reg_field_status_over_thl);
>> +     if (IS_ERR(data->reg_status_over_thl))
>> +             return dev_err_probe(
>> +                     dev, PTR_ERR(data->reg_status_over_thl),
>> +                     "regmap_field reg_status_over_thl 
>> initialization failed\n");
>> +
>> +     data->reg_status_under_tll = devm_regmap_field_alloc(
>> +             dev, data->regmap, tids_reg_field_status_under_tll);
>> +     if (IS_ERR(data->reg_status_under_tll))
>> +             return dev_err_probe(
>> +                     dev, PTR_ERR(data->reg_status_under_tll),
>> +                     "regmap_field reg_status_under_tll 
>> initialization failed\n");
>> +
>> +     /* Allocate regmap_field for software_reset */
>> +     data->reg_soft_reset = devm_regmap_field_alloc(
>> +             dev, data->regmap, tids_reg_field_soft_reset);
>> +     if (IS_ERR(data->reg_soft_reset))
>> +             return dev_err_probe(
>> +                     dev, PTR_ERR(data->reg_soft_reset),
>> +                     "regmap_field reg_soft_reset initialization 
>> failed\n");
>
> Following up on the above, that is a lot of code just to avoid using 
> regmap_update_bits().
> Again, what am I missing ?
>
>> +
>> +     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 = 0;
>> +
>> +     data = devm_kzalloc(device, sizeof(*data), GFP_KERNEL);
>> +     if (!data)
>> +             return -ENOMEM;
>> +
>> +     data->client = client;
>> +
>> +     ret = tids_probe_regmap(data);
>> +     if (ret != 0)
>> +             return ret;
>> +
>> +     /* Read device id, to check if i2c is working */
>> +     ret = regmap_read(data->regmap, TIDS_CMD_DEVICE_ID, &value);
>> +     if (ret < 0)
>> +             return ret;
>
> Why read this but not check it ?
>
>> +
>> +     /* Triggering soft reset */
>> +     ret = regmap_field_write(data->reg_soft_reset, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = regmap_field_write(data->reg_soft_reset, 0);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     /* Allowing bulk read */
>> +     ret = regmap_field_write(data->reg_ctrl_if_add_inc, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     /* Set meassurement interval */
>> +     ret = regmap_field_write(data->reg_ctrl_avg, 
>> tids_update_interval_40ms);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     /* Set device to free run mode */
>> +     ret = regmap_field_write(data->reg_ctrl_freerun, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     /* Don't update temperature register until high and low value 
>> are read */
>> +     ret = regmap_field_write(data->reg_ctrl_bdu, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +
>
> Please move this code into a separate _init function. Also, is it 
> really necessary
> to write all those control register values bit by bit ? If so, that 
> should be explained
> since it adds a lot of non-obvious complexity to the code.
>
>> +     hwmon_dev = devm_hwmon_device_register_with_info(
>> +             device, device->driver->name, data, &tids_chip_info, 
>> NULL);
>
> Just use "tids" instead of "device->driver->name".
>
>> +
>> +     return PTR_ERR_OR_ZERO(hwmon_dev);
>> +}
>> +
>> +static int tids_suspend(struct device *dev)
>> +{
>> +     struct tids_data *data = dev_get_drvdata(dev);
>> +
>> +     return regmap_field_write(data->reg_ctrl_freerun, 0);
>> +}
>> +
>> +static int tids_resume(struct device *dev)
>> +{
>> +     struct tids_data *data = dev_get_drvdata(dev);
>> +
>> +     return regmap_field_write(data->reg_ctrl_freerun, 1);
>> +}
>> +
>> +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-2521020222501" },
>> +     {}
>> +};
>> +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"); 

I've gone through all the other notes and included them in the next 
version of the
patch. I will send the next patch series within the next few hours.

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

[Thomas Marangoni]

Add tids driver documentation
---
 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..f3fea4e416ea
--- /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. Must be 5, 10, 20
+                or 40.
+temp1_max_hyst  The temperature in millidegrees Celsius, that
+                is triggering the temp1_max_alarm. Writable.
+                The lowest possible value is -39680 and the
+                highest possible value is 122880. Values are
+                saved in steps of 640.
+temp1_min_hyst  The temperature in millidegrees Celsius, that
+                is triggering the temp1_min_alarm. Writable.
+                The lowest possible value is -39680 and the
+                highest possible 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_hyst. 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_hyst. It will reset automatically
+                once it has been read.
+=============== ============================================
-- 
2.51.1

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

[Guenter Roeck]

On 11/17/25 04:38, Thomas Marangoni wrote:
> Add tids driver documentation
> ---
>   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..f3fea4e416ea
> --- /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. Must be 5, 10, 20
> +                or 40.

As mentioned in the driver feedback, this should be more generous and say
something like "Supported values are 5, 10, 20, or 40".

> +temp1_max_hyst  The temperature in millidegrees Celsius, that
> +                is triggering the temp1_max_alarm. Writable.
> +                The lowest possible value is -39680 and the

As above, s/possible/supported/. We don't usually expect users to know
supported value ranges for update intervals or temperature limits.

> +                highest possible value is 122880. Values are
> +                saved in steps of 640.
> +temp1_min_hyst  The temperature in millidegrees Celsius, that
> +                is triggering the temp1_min_alarm. Writable.
> +                The lowest possible value is -39680 and the
> +                highest possible value is 122880. Values are
> +                saved in steps of 640.

As mentioned in the driver feedback, I think those should be temp1_max
and temp1_min. I see no evidence that those are hysteresis values,
and the description here and in the datasheet confirms this.

> +temp1_max_alarm The alarm will be triggered when the level
> +                reaches the value specified in
> +                temp1_max_hyst. 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_hyst. It will reset automatically
> +                once it has been read.
> +=============== ============================================

[PATCH 3/4] dt-bindings: Add tids in bindings and wsen as vendor

[Thomas Marangoni]

This patch adds the tids driver to the bindings as trivial-devices
and adds the WSEN manufacturer to the vendor-prefixes.
---
 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..e120767ce119 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
+            # Temperature sensor with i2c interface
+          - wsen,tids-2521020222501
 
 required:
   - compatible
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 42d2bc0ce027..2cf753fdf5a7 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1823,6 +1823,8 @@ patternProperties:
     description: Wobo
   "^wolfvision,.*":
     description: WolfVision GmbH
+  "^wsen,.*":
+    description: Würth Elektronik eiSos GmbH & Co. KG
   "^x-powers,.*":
     description: X-Powers
   "^xen,.*":
-- 
2.51.1

Re: [PATCH 3/4] dt-bindings: Add tids in bindings and wsen as vendor

[Guenter Roeck]

On 11/17/25 04:38, Thomas Marangoni wrote:
> This patch adds the tids driver to the bindings as trivial-devices
> and adds the WSEN manufacturer to the vendor-prefixes.

This patch should be the first patch in the series.

> ---
>   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..e120767ce119 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
> +            # Temperature sensor with i2c interface
> +          - wsen,tids-2521020222501

I think this should just be "wsen,tids".

>   
>   required:
>     - compatible
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 42d2bc0ce027..2cf753fdf5a7 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -1823,6 +1823,8 @@ patternProperties:
>       description: Wobo
>     "^wolfvision,.*":
>       description: WolfVision GmbH
> +  "^wsen,.*":
> +    description: Würth Elektronik eiSos GmbH & Co. KG
>     "^x-powers,.*":
>       description: X-Powers
>     "^xen,.*":

Re: [PATCH 3/4] dt-bindings: Add tids in bindings and wsen as vendor

[Krzysztof Kozlowski]

On 17/11/2025 13:38, Thomas Marangoni wrote:
> This patch adds the tids driver to the bindings as trivial-devices
> and adds the WSEN manufacturer to the vendor-prefixes.
> ---

You need to run checkpatch...

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v6.16/source/Documentation/process/submitting-patches.rst#L94

Also, do not reference driver. Explain here hardware, why this is a
trivial device.

Best regards,
Krzysztof

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

[Thomas Marangoni]

I've added myself as maintainer for the tids driver.
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index ec635515c0c4..3f981252fa2a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -27965,6 +27965,12 @@ F:	Documentation/ABI/stable/sysfs-platform-wmi-bmof
 F:	Documentation/wmi/devices/wmi-bmof.rst
 F:	drivers/platform/x86/wmi-bmof.c
 
+WSEN TIDS DRIVER
+M:	Thomas Marangoni <Thomas.Marangoni@becom-group.com>
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	drivers/hwmon/tids.c
+
 WOLFSON MICROELECTRONICS DRIVERS
 L:	patches@opensource.cirrus.com
 S:	Supported
-- 
2.51.1

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

[Krzysztof Kozlowski]

On 17/11/2025 13:38, Thomas Marangoni wrote:
> I've added myself as maintainer for the tids driver.
> ---
>  MAINTAINERS | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ec635515c0c4..3f981252fa2a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -27965,6 +27965,12 @@ F:	Documentation/ABI/stable/sysfs-platform-wmi-bmof
>  F:	Documentation/wmi/devices/wmi-bmof.rst
>  F:	drivers/platform/x86/wmi-bmof.c
>  
> +WSEN TIDS DRIVER

S > O.

Best regards,
Krzysztof