Re: [PATCH] hwmon:tmp102: add support for update interval

[Guenter Roeck <linux@roeck-us.net>]

Hi,

On 3/31/26 10:54, Flaviu Nistor wrote:
> Since the sensor supports different sampling intervals via
> bits CR0 and CR1 from the CONFIG register, add support in
> order for the conversion rate to be changed from user space.
> Default is 4 conv/sec.
> 
> Signed-off-by: Flaviu Nistor <flaviu.nistor@gmail.com>

The subject line should start with "hwmon: (tmp102) " to match the common
subject line format used in hwmon drivers.

Other than that, please check

https://sashiko.dev/#/patchset/20260331175418.16145-1-flaviu.nistor%40gmail.com

and fix the identified issues.

Additional comments inline.

Thanks,
Guenter

> ---
>   Documentation/hwmon/tmp102.rst |  19 +++-
>   drivers/hwmon/tmp102.c         | 165 ++++++++++++++++++++++++---------
>   2 files changed, 139 insertions(+), 45 deletions(-)
> 
> diff --git a/Documentation/hwmon/tmp102.rst b/Documentation/hwmon/tmp102.rst
> index 3c2cb5bab1e9..7ed649ac4cd0 100644
> --- a/Documentation/hwmon/tmp102.rst
> +++ b/Documentation/hwmon/tmp102.rst
> @@ -41,12 +41,25 @@ degree from -40 to +125 C. Resolution of the sensor is 0.0625 degree.  The
>   operating temperature has a minimum of -55 C and a maximum of +150 C.
>   
>   The TMP102 has a programmable update rate that can select between 8, 4, 1, and
> -0.5 Hz. (Currently the driver only supports the default of 4 Hz).
> +0.5 Hz.
>   
>   The TMP110 and TMP113 are software compatible with TMP102, but have different
>   accuracy (maximum error) specifications. The TMP110 has an accuracy (maximum error)
>   of 1.0 degree, TMP113 has an accuracy (maximum error) of 0.3 degree, while TMP102
>   has an accuracy (maximum error) of 2.0 degree.
>   
> -The driver provides the common sysfs-interface for temperatures (see
> -Documentation/hwmon/sysfs-interface.rst under Temperatures).
> +sysfs-Interface
> +---------------
> +
> +The following list includes the sysfs attributes that the driver provides, their
> +permissions and a short description:
> +
> +=============================== ======= ========================================
> +Name                            Perm    Description
> +=============================== ======= ========================================
> +temp1_input:                    RO      Temperature input
> +temp1_label:                    RO      Descriptive name for the sensor
> +temp1_max:                      RW      Maximum temperature
> +temp1_max_hyst:                 RW      Maximum hysteresis temperature
> +update_interval                 RW      Update conversions interval in milliseconds
> +=============================== ======= ========================================

This will likely trigger a documentation error since the "===" line is expected to
match the longest text in the table.


> diff --git a/drivers/hwmon/tmp102.c b/drivers/hwmon/tmp102.c
> index 5b10c395a84d..c0805f6271b9 100644
> --- a/drivers/hwmon/tmp102.c
> +++ b/drivers/hwmon/tmp102.c
> @@ -50,11 +50,25 @@
>   
>   #define CONVERSION_TIME_MS		35	/* in milli-seconds */
>   
> +struct tmp102_params {
> +	u16 default_sample_time;
> +	u8 num_sample_times;
> +	const unsigned int *sample_times;
> +};
> +
>   struct tmp102 {
>   	const char *label;
>   	struct regmap *regmap;
>   	u16 config_orig;
>   	unsigned long ready_time;
> +	u16 sample_time;
> +	const struct tmp102_params *params;
> +};
> +
> +static const struct tmp102_params device_params = {
> +	.default_sample_time = 125,

Why change the default ?

> +	.num_sample_times = 4,
> +	.sample_times = (unsigned int []){ 125, 250, 1000, 4000 },
>   };
>   
>   /* convert left adjusted 13-bit TMP102 register value to milliCelsius */
> @@ -80,58 +94,113 @@ static int tmp102_read_string(struct device *dev, enum hwmon_sensor_types type,
>   }
>   
>   static int tmp102_read(struct device *dev, enum hwmon_sensor_types type,
> -		       u32 attr, int channel, long *temp)
> +		       u32 attr, int channel, long *val)
>   {
>   	struct tmp102 *tmp102 = dev_get_drvdata(dev);
>   	unsigned int regval;
>   	int err, reg;
>   
> -	switch (attr) {
> -	case hwmon_temp_input:
> -		/* Is it too early to return a conversion ? */
> -		if (time_before(jiffies, tmp102->ready_time)) {
> -			dev_dbg(dev, "%s: Conversion not ready yet..\n", __func__);
> -			return -EAGAIN;
> +	switch (type) {
> +	case hwmon_chip:

Please split into separate functions for handling chip and temperature data.

> +		switch (attr) {
> +		case hwmon_chip_update_interval:
> +			*val = tmp102->sample_time;
> +			break;
> +		default:
> +			return -EINVAL;

-EOPNOTSUPP

>   		}
> -		reg = TMP102_TEMP_REG;
>   		break;
> -	case hwmon_temp_max_hyst:
> -		reg = TMP102_TLOW_REG;
> -		break;
> -	case hwmon_temp_max:
> -		reg = TMP102_THIGH_REG;
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_input:
> +			/* Is it too early to return a conversion ? */
> +			if (time_before(jiffies, tmp102->ready_time)) {
> +				dev_dbg(dev, "%s: Conversion not ready yet..\n", __func__);
> +				return -EAGAIN;
> +			}
> +			reg = TMP102_TEMP_REG;
> +			break;
> +		case hwmon_temp_max_hyst:
> +			reg = TMP102_TLOW_REG;
> +			break;
> +		case hwmon_temp_max:
> +			reg = TMP102_THIGH_REG;
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +		err = regmap_read(tmp102->regmap, reg, &regval);
> +		if (err < 0)
> +			return err;
> +		*val = tmp102_reg_to_mC(regval);
>   		break;
>   	default:
> -		return -EOPNOTSUPP;
> +		return -EINVAL;

Do not change this.

>   	}
> +	return 0;
> +}
> +
> +static int tmp102_update_interval(struct device *dev, long val)
> +{
> +	struct tmp102 *tmp102 = dev_get_drvdata(dev);
> +	unsigned int reg;
> +	u8 index;
> +	s32 err;
> +
> +	index = find_closest(val, tmp102->params->sample_times,
> +			     (int)tmp102->params->num_sample_times);
>   
> -	err = regmap_read(tmp102->regmap, reg, &regval);
> +	err = regmap_read(tmp102->regmap, TMP102_CONF_REG, &reg);
> +	if (err < 0)
> +		return err;
> +	reg &= ~0x00c0;
> +	reg |= (3 - index) << 6;
> +	err = regmap_write(tmp102->regmap, TMP102_CONF_REG, reg);
>   	if (err < 0)
>   		return err;
> -	*temp = tmp102_reg_to_mC(regval);
> +	tmp102->sample_time = tmp102->params->sample_times[index];
>   
>   	return 0;
>   }
>   
> +static int tmp102_write_chip(struct device *dev, u32 attr, long val)
> +{
> +	switch (attr) {
> +	case hwmon_chip_update_interval:
> +		return tmp102_update_interval(dev, val);
> +	default:
> +		return -EINVAL;

-EOPNOTSUPP. This is not an "Invalid Argument" provided by the user.

> +	}
> +	return 0;
> +}
> +
>   static int tmp102_write(struct device *dev, enum hwmon_sensor_types type,
> -			u32 attr, int channel, long temp)
> +			u32 attr, int channel, long val)
>   {
>   	struct tmp102 *tmp102 = dev_get_drvdata(dev);
>   	int reg;
>   
> -	switch (attr) {
> -	case hwmon_temp_max_hyst:
> -		reg = TMP102_TLOW_REG;
> -		break;
> -	case hwmon_temp_max:
> -		reg = TMP102_THIGH_REG;
> -		break;
> +	switch (type) {
> +	case hwmon_chip:
> +		return tmp102_write_chip(dev, attr, val);
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_max_hyst:
> +			reg = TMP102_TLOW_REG;
> +			break;
> +		case hwmon_temp_max:
> +			reg = TMP102_THIGH_REG;
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +
> +		val = clamp_val(val, -256000, 255000);
> +		return regmap_write(tmp102->regmap, reg, tmp102_mC_to_reg(val));
>   	default:
> -		return -EOPNOTSUPP;
> +		return -EINVAL;

Sigh.

>   	}
> -
> -	temp = clamp_val(temp, -256000, 255000);
> -	return regmap_write(tmp102->regmap, reg, tmp102_mC_to_reg(temp));
> +	return 0;
>   }
>   
>   static umode_t tmp102_is_visible(const void *data, enum hwmon_sensor_types type,
> @@ -139,27 +208,35 @@ static umode_t tmp102_is_visible(const void *data, enum hwmon_sensor_types type,
>   {
>   	const struct tmp102 *tmp102 = data;
>   
> -	if (type != hwmon_temp)
> -		return 0;
> -
> -	switch (attr) {
> -	case hwmon_temp_input:
> -		return 0444;
> -	case hwmon_temp_label:
> -		if (tmp102->label)
> +	switch (type) {
> +	case hwmon_chip:
> +		switch (attr) {
> +		case hwmon_chip_update_interval:
> +			return 0644;
> +		}
> +		break;
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_input:
>   			return 0444;
> -		return 0;
> -	case hwmon_temp_max_hyst:
> -	case hwmon_temp_max:
> -		return 0644;
> +		case hwmon_temp_label:
> +			if (tmp102->label)
> +				return 0444;
> +			return 0;
> +		case hwmon_temp_max_hyst:
> +		case hwmon_temp_max:
> +			return 0644;

Please add "default:" to all those case statements.

> +		}
> +		break;
>   	default:
> -		return 0;
> +		break;
>   	}
> +	return 0;
>   }
>   
>   static const struct hwmon_channel_info * const tmp102_info[] = {
>   	HWMON_CHANNEL_INFO(chip,
> -			   HWMON_C_REGISTER_TZ),
> +			   HWMON_C_REGISTER_TZ | HWMON_C_UPDATE_INTERVAL),
>   	HWMON_CHANNEL_INFO(temp,
>   			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_MAX | HWMON_T_MAX_HYST),
>   	NULL
> @@ -237,6 +314,10 @@ static int tmp102_probe(struct i2c_client *client)
>   	if (IS_ERR(tmp102->regmap))
>   		return PTR_ERR(tmp102->regmap);
>   
> +	tmp102->params = &device_params;

Why have "params" in the first place ? I don't see the point unless
the driver supports more than one chip.

> +
> +	tmp102->sample_time = tmp102->params->default_sample_time;
> +
>   	err = regmap_read(tmp102->regmap, TMP102_CONF_REG, &regval);
>   	if (err < 0) {
>   		dev_err(dev, "error reading config register\n");