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

[Guenter Roeck]

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

Additional feedback:

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

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

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

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

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

(0 - 63) * 640 = -40320

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

Thanks,
Guenter