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

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

On 4/1/26 09:47, 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>

Please address the feedback at:

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

sample_times should be static, and the concern about the mask seems real.

Additional feedback inline.

Thanks,
Guenter

> ---
> Changes in v2:
> - Implement all changes suggested inline by Guenter Roeck.
> - Fixed identified issues from link provided by Guenter Roeck:
> https://sashiko.dev/#/patchset/20260331175418.16145-1-flaviu.nistor%40gmail.com	
> - Link to v1: https://lore.kernel.org/all/20260331175418.16145-1-flaviu.nistor@gmail.com/
> 
>   Documentation/hwmon/tmp102.rst |  19 ++++-
>   drivers/hwmon/tmp102.c         | 137 ++++++++++++++++++++++++++-------
>   2 files changed, 125 insertions(+), 31 deletions(-)
> 
> diff --git a/Documentation/hwmon/tmp102.rst b/Documentation/hwmon/tmp102.rst
> index 3c2cb5bab1e9..425a09a3c9b3 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.25 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
> +=============================== ======= ===========================================
> diff --git a/drivers/hwmon/tmp102.c b/drivers/hwmon/tmp102.c
> index 5b10c395a84d..7d3ef64bd28c 100644
> --- a/drivers/hwmon/tmp102.c
> +++ b/drivers/hwmon/tmp102.c
> @@ -50,11 +50,16 @@
>   
>   #define CONVERSION_TIME_MS		35	/* in milli-seconds */
>   
> +#define NUM_SAMPLE_TIMES 4
> +#define DEFAULT_SAMPLE_TIME_MS 250

Please use

#define<space>NAME<tab>value

and align with the CONVERSION_TIME_MS definition.

> +const unsigned int *sample_times = (const unsigned int []){ 125, 250, 1000, 4000 };
> +
>   struct tmp102 {
>   	const char *label;
>   	struct regmap *regmap;
>   	u16 config_orig;
>   	unsigned long ready_time;
> +	u16 sample_time;
>   };
>   
>   /* convert left adjusted 13-bit TMP102 register value to milliCelsius */
> @@ -79,8 +84,20 @@ static int tmp102_read_string(struct device *dev, enum hwmon_sensor_types type,
>   	return 0;
>   }
>   
> -static int tmp102_read(struct device *dev, enum hwmon_sensor_types type,
> -		       u32 attr, int channel, long *temp)
> +static int tmp102_read_chip(struct device *dev, u32 attr, long *val)
> +{
> +	struct tmp102 *tmp102 = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_chip_update_interval:
> +		*val = tmp102->sample_time;
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int tmp102_read_temp(struct device *dev, u32 attr, long *val)
>   {
>   	struct tmp102 *tmp102 = dev_get_drvdata(dev);
>   	unsigned int regval;
> @@ -108,30 +125,80 @@ static int tmp102_read(struct device *dev, enum hwmon_sensor_types type,
>   	err = regmap_read(tmp102->regmap, reg, &regval);
>   	if (err < 0)
>   		return err;
> -	*temp = tmp102_reg_to_mC(regval);
> +
> +	*val = tmp102_reg_to_mC(regval);
>   
>   	return 0;
>   }
>   
> -static int tmp102_write(struct device *dev, enum hwmon_sensor_types type,
> -			u32 attr, int channel, long temp)
> +static int tmp102_read(struct device *dev, enum hwmon_sensor_types type,
> +		       u32 attr, int channel, long *val)
> +{
> +	switch (type) {
> +	case hwmon_chip:
> +		return tmp102_read_chip(dev, attr, val);
> +	case hwmon_temp:
> +		return tmp102_read_temp(dev, attr, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int tmp102_update_interval(struct device *dev, long val)
>   {
>   	struct tmp102 *tmp102 = dev_get_drvdata(dev);
> -	int reg;
> +	u8 index;
> +	s32 err;
> +
> +	index = find_closest(val, sample_times, NUM_SAMPLE_TIMES);
> +
> +	err = regmap_update_bits(tmp102->regmap, TMP102_CONF_REG,
> +				 0xc000, (3 - index) << 14);
> +	if (err < 0)
> +		return err;
> +	tmp102->sample_time = sample_times[index];
> +
> +	return 0;
> +}
>   
> +static int tmp102_write_chip(struct device *dev, u32 attr, long val)
> +{
>   	switch (attr) {
> -	case hwmon_temp_max_hyst:
> -		reg = TMP102_TLOW_REG;
> -		break;
> -	case hwmon_temp_max:
> -		reg = TMP102_THIGH_REG;
> -		break;
> +	case hwmon_chip_update_interval:
> +		return tmp102_update_interval(dev, val);
>   	default:
>   		return -EOPNOTSUPP;
>   	}
> +	return 0;
> +}
> +
> +static int tmp102_write(struct device *dev, enum hwmon_sensor_types type,
> +			u32 attr, int channel, long val)
> +{
> +	struct tmp102 *tmp102 = dev_get_drvdata(dev);
> +	int reg;
> +
> +	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;
> +		}
>   

Please add separate tmp102_write_temp() function.

> -	temp = clamp_val(temp, -256000, 255000);
> -	return regmap_write(tmp102->regmap, reg, tmp102_mC_to_reg(temp));
> +		val = clamp_val(val, -256000, 255000);
> +		return regmap_write(tmp102->regmap, reg, tmp102_mC_to_reg(val));
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
>   }
>   
>   static umode_t tmp102_is_visible(const void *data, enum hwmon_sensor_types type,
> @@ -139,27 +206,39 @@ 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;
> +		default:
> +			break;
> +		}
> +		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;
> +		default:
> +			break;
> +		}
> +		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 +316,8 @@ static int tmp102_probe(struct i2c_client *client)
>   	if (IS_ERR(tmp102->regmap))
>   		return PTR_ERR(tmp102->regmap);
>   
> +	tmp102->sample_time = DEFAULT_SAMPLE_TIME_MS;
> +
>   	err = regmap_read(tmp102->regmap, TMP102_CONF_REG, &regval);
>   	if (err < 0) {
>   		dev_err(dev, "error reading config register\n");