Message-ID: <5758dedd1a0d97ddc522448502ae07b0ede4ea67.camel@gmail.com>
Date: Wed, 19 Nov 2025 15:05:06 +0000
From: Nuno Sá <noname.nuno@...il.com>
To: Thomas Marangoni <Thomas.Marangoni@...om-group.com>, 
	linux-hwmon@...r.kernel.org
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 linux@...ck-us.net, 	corbet@....net, Jonathan.Cameron@...wei.com,
 michal.simek@....com, 	nuno.sa@...log.com, Frank.Li@....com,
 wenswang@...h.net, apokusinski01@...il.com, 	dixitparmar19@...il.com,
 vassilisamir@...il.com, paweldembicki@...il.com, 	heiko@...ech.de,
 neil.armstrong@...aro.org, kever.yang@...k-chips.com, 
	prabhakar.mahadev-lad.rj@...renesas.com, mani@...nel.org, dev@...l-k.io, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-doc@...r.kernel.org
Subject: Re: [PATCH v2 3/4] hwmon: Add driver for wsen tids

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@...om-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@...om-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á

Powered by blists - more mailing lists