Re: [PATCH v3 1/5] hwmon: it87: describe per-chip temperature resources

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

On Sat, Jan 10, 2026 at 02:26:09AM +0100, benoit.masson wrote:
> Add per-chip temp limit/offset/map counts and wire the driver
> to use them.
> 
> This keeps existing chips on the previous defaults while allowing newer
> chips to advertise larger resources.
> 
> Signed-off-by: benoit.masson <yahoo@perenite.com>
> ---
>  drivers/hwmon/it87.c | 51 +++++++++++++++++++++++++++++++++-----------
>  1 file changed, 38 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index e233aafa8856..ec5b1668dd7b 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -54,6 +54,7 @@
>  #include <linux/hwmon-vid.h>
>  #include <linux/err.h>
>  #include <linux/mutex.h>
> +#include <linux/minmax.h>
>  #include <linux/sysfs.h>
>  #include <linux/string.h>
>  #include <linux/dmi.h>
> @@ -279,8 +280,9 @@ static const u8 IT87_REG_AUTO_BASE[] = { 0x60, 0x68, 0x70, 0x78, 0xa0, 0xa8 };
>  #define NUM_VIN			ARRAY_SIZE(IT87_REG_VIN)
>  #define NUM_VIN_LIMIT		8
>  #define NUM_TEMP		6
> -#define NUM_TEMP_OFFSET		ARRAY_SIZE(IT87_REG_TEMP_OFFSET)
> -#define NUM_TEMP_LIMIT		3
> +#define IT87_TEMP_OFFSET_MAX	ARRAY_SIZE(IT87_REG_TEMP_OFFSET)
> +#define IT87_TEMP_LIMIT_DEFAULT	3
> +#define IT87_TEMP_MAP_DEFAULT	3

Adding IT87_ prefixes here is unnecessary and inconsistent.

_DEFAULT defines are unnecessary. num_temp_limit, num_temp_offset,
and num_temp_map should be added to all configuration entries.
With this, FEAT_TEMP_OFFSET and has_temp_offset() are unnecessary
and can be dropped since support is implicit with num_temp_offset > 0.

>  #define NUM_FAN			ARRAY_SIZE(IT87_REG_FAN)
>  #define NUM_FAN_DIV		3
>  #define NUM_PWM			ARRAY_SIZE(IT87_REG_PWM)
> @@ -290,6 +292,9 @@ struct it87_devices {
>  	const char *name;
>  	const char * const model;
>  	u32 features;
> +	u8 num_temp_limit;
> +	u8 num_temp_offset;
> +	u8 num_temp_map;
>  	u8 peci_mask;
>  	u8 old_peci_mask;
>  	u8 smbus_bitmap;	/* SMBus enable bits in extra config register */
> @@ -578,6 +583,9 @@ struct it87_data {
>  	int sioaddr;
>  	enum chips type;
>  	u32 features;
> +	u8 num_temp_limit;
> +	u8 num_temp_offset;
> +	u8 num_temp_map;
>  	u8 peci_mask;
>  	u8 old_peci_mask;
>  
> @@ -926,12 +934,13 @@ static struct it87_data *it87_update_device(struct device *dev)
>  			data->temp[i][0] =
>  				it87_read_value(data, IT87_REG_TEMP(i));
>  
> -			if (has_temp_offset(data) && i < NUM_TEMP_OFFSET)
> +			if (has_temp_offset(data) &&
> +			    i < data->num_temp_offset)
>  				data->temp[i][3] =
>  				  it87_read_value(data,
>  						  IT87_REG_TEMP_OFFSET[i]);
>  
> -			if (i >= NUM_TEMP_LIMIT)
> +			if (i >= data->num_temp_limit)
>  				continue;
>  
>  			data->temp[i][1] =
> @@ -1679,16 +1688,18 @@ static ssize_t show_pwm_temp_map(struct device *dev,
>  	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
>  	struct it87_data *data = it87_update_device(dev);
>  	int nr = sensor_attr->index;
> +	u8 num_map;
>  	int map;
>  
>  	if (IS_ERR(data))
>  		return PTR_ERR(data);
>  
> +	num_map = data->num_temp_map ?: IT87_TEMP_MAP_DEFAULT;
>  	map = data->pwm_temp_map[nr];
> -	if (map >= 3)
> +	if (map >= num_map)
>  		map = 0;	/* Should never happen */
> -	if (nr >= 3)		/* pwm channels 3..6 map to temp4..6 */
> -		map += 3;
> +	if (nr >= num_map)	/* pwm channels 3..6 map to temp4..6 */
> +		map += num_map;
>  
>  	return sprintf(buf, "%d\n", (int)BIT(map));
>  }
> @@ -1700,6 +1711,7 @@ static ssize_t set_pwm_temp_map(struct device *dev,
>  	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
>  	struct it87_data *data = dev_get_drvdata(dev);
>  	int nr = sensor_attr->index;
> +	u8 num_map = data->num_temp_map ?: IT87_TEMP_MAP_DEFAULT;
>  	long val;
>  	int err;
>  	u8 reg;
> @@ -1707,8 +1719,8 @@ static ssize_t set_pwm_temp_map(struct device *dev,
>  	if (kstrtol(buf, 10, &val) < 0)
>  		return -EINVAL;
>  
> -	if (nr >= 3)
> -		val -= 3;
> +	if (nr >= num_map)
> +		val -= num_map;
>  
>  	switch (val) {
>  	case BIT(0):
> @@ -3206,7 +3218,7 @@ static void it87_check_limit_regs(struct it87_data *data)
>  		if (reg == 0xff)
>  			it87_write_value(data, IT87_REG_VIN_MIN(i), 0);
>  	}
> -	for (i = 0; i < NUM_TEMP_LIMIT; i++) {
> +	for (i = 0; i < data->num_temp_limit; i++) {
>  		reg = it87_read_value(data, IT87_REG_TEMP_HIGH(i));
>  		if (reg == 0xff)
>  			it87_write_value(data, IT87_REG_TEMP_HIGH(i), 127);
> @@ -3399,6 +3411,7 @@ static int it87_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	struct device *dev = &pdev->dev;
>  	struct it87_sio_data *sio_data = dev_get_platdata(dev);
> +	const struct it87_devices *chip;
>  	int enable_pwm_interface;
>  	struct device *hwmon_dev;
>  	int err;
> @@ -3421,9 +3434,21 @@ static int it87_probe(struct platform_device *pdev)
>  	data->type = sio_data->type;
>  	data->smbus_bitmap = sio_data->smbus_bitmap;
>  	data->ec_special_config = sio_data->ec_special_config;
> -	data->features = it87_devices[sio_data->type].features;
> -	data->peci_mask = it87_devices[sio_data->type].peci_mask;
> -	data->old_peci_mask = it87_devices[sio_data->type].old_peci_mask;
> +	chip = &it87_devices[sio_data->type];
> +	data->features = chip->features;
> +	data->peci_mask = chip->peci_mask;
> +	data->old_peci_mask = chip->old_peci_mask;
> +	data->num_temp_limit = chip->num_temp_limit ?
> +			       chip->num_temp_limit : IT87_TEMP_LIMIT_DEFAULT;
> +	if (chip->num_temp_offset)
> +		data->num_temp_offset = min(chip->num_temp_offset,
> +					    (u8)IT87_TEMP_OFFSET_MAX);
> +	else if (has_temp_offset(data))
> +		data->num_temp_offset = IT87_TEMP_OFFSET_MAX;
> +	else
> +		data->num_temp_offset = 0;
> +	data->num_temp_map = chip->num_temp_map ?
> +			     chip->num_temp_map : IT87_TEMP_MAP_DEFAULT;

Runtime checks of build parameters are unacceptable and, again,
all fields should be initialized correctly in the configuration data.
That is what it is for. Runtime updates such as the above are both
unnecessary and unacceptable. num_temp_limit, num_temp_map and
num_temp_offset are available in the configuration data. Enter the data
there and use it.

Thanks,
Guenter