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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <6b5cd51f-be90-4646-9cfa-278e16c09dbe@roeck-us.net>
Date: Wed, 19 Nov 2025 14:23:17 -0800
From: Guenter Roeck <linux@...ck-us.net>
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, 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 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