Message-ID: <8372c65d-1202-4753-9002-3a0171943051@roeck-us.net>
Date: Wed, 19 Nov 2025 09:46:49 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Nuno Sá <noname.nuno@...il.com>,
 Thomas Marangoni <Thomas.Marangoni@...om-group.com>,
 linux-hwmon@...r.kernel.org
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, 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

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

Alphabetic order please.

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

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

Unused

>> +	int temperature;

Unused

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

Unnecessary initialization.

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

Also, the initialization is unnecessary.

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

Unnecessary initialization.

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

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

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

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

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

measurement

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

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

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

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

Powered by blists - more mailing lists