Re: [lm-sensors] [PATCH] hwmon: (it87) Add support for IT8732F

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

Hi Justin,

On Tue, Aug 04, 2015 at 12:45:31PM -0700, Justin Maggard wrote:
> Add support for the IT8732F.  This chip is pretty similar to IT8721F,
> with the main difference being that the ADC LSB is 10.9 mV instead of
> 12 mV.
> 

I assume you have a datasheet. Since you say "main difference",
are you aware of any other differences ? Would it by any chance
be possible to share the datasheet with us ?

Further comments inline.

Thanks,
Guenter

> Signed-off-by: Justin Maggard <jmaggard@netgear.com>
> ---
>  Documentation/hwmon/it87 | 35 +++++++++++++++++++------------
>  drivers/hwmon/it87.c     | 54 +++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 66 insertions(+), 23 deletions(-)
> 
> diff --git a/Documentation/hwmon/it87 b/Documentation/hwmon/it87
> index e872948..733296d 100644
> --- a/Documentation/hwmon/it87
> +++ b/Documentation/hwmon/it87
> @@ -38,6 +38,10 @@ Supported chips:
>      Prefix: 'it8728'
>      Addresses scanned: from Super I/O config space (8 I/O ports)
>      Datasheet: Not publicly available
> +  * IT8732F
> +    Prefix: 'it8732'
> +    Addresses scanned: from Super I/O config space (8 I/O ports)
> +    Datasheet: Not publicly available
>    * IT8771E
>      Prefix: 'it8771'
>      Addresses scanned: from Super I/O config space (8 I/O ports)
> @@ -111,9 +115,9 @@ Description
>  -----------
>  
>  This driver implements support for the IT8603E, IT8620E, IT8623E, IT8705F,
> -IT8712F, IT8716F, IT8718F, IT8720F, IT8721F, IT8726F, IT8728F, IT8758E,
> -IT8771E, IT8772E, IT8781F, IT8782F, IT8783E/F, IT8786E, IT8790E, and SiS950
> -chips.
> +IT8712F, IT8716F, IT8718F, IT8720F, IT8721F, IT8726F, IT8728F, IT8732F,
> +IT8758E, IT8771E, IT8772E, IT8781F, IT8782F, IT8783E/F, IT8786E, IT8790E, and
> +SiS950 chips.
>  
>  These chips are 'Super I/O chips', supporting floppy disks, infrared ports,
>  joysticks and other miscellaneous stuff. For hardware monitoring, they
> @@ -137,10 +141,10 @@ The IT8716F, IT8718F, IT8720F, IT8721F/IT8758E and later IT8712F revisions
>  have support for 2 additional fans. The additional fans are supported by the
>  driver.
>  
> -The IT8716F, IT8718F, IT8720F, IT8721F/IT8758E, IT8781F, IT8782F, IT8783E/F,
> -and late IT8712F and IT8705F also have optional 16-bit tachometer counters
> -for fans 1 to 3. This is better (no more fan clock divider mess) but not
> -compatible with the older chips and revisions. The 16-bit tachometer mode
> +The IT8716F, IT8718F, IT8720F, IT8721F/IT8758E, IT8732F, IT8781F, IT8782F,
> +IT8783E/F, and late IT8712F and IT8705F also have optional 16-bit tachometer
> +counters for fans 1 to 3. This is better (no more fan clock divider mess) but
> +not compatible with the older chips and revisions. The 16-bit tachometer mode
>  is enabled by the driver when one of the above chips is detected.
>  
>  The IT8726F is just bit enhanced IT8716F with additional hardware
> @@ -159,6 +163,9 @@ IT8728F. It only supports 16-bit fan mode.
>  
>  The IT8790E supports up to 3 fans. 16-bit fan mode is always enabled.
>  
> +The IT8732F supports a closed-loop mode for fan control, but this is not
> +currently implemented by the driver.
> +
>  Temperatures are measured in degrees Celsius. An alarm is triggered once
>  when the Overtemperature Shutdown limit is crossed.
>  
> @@ -173,12 +180,14 @@ is done.
>  Voltage sensors (also known as IN sensors) report their values in volts. An
>  alarm is triggered if the voltage has crossed a programmable minimum or
>  maximum limit. Note that minimum in this case always means 'closest to
> -zero'; this is important for negative voltage measurements. All voltage
> -inputs can measure voltages between 0 and 4.08 volts, with a resolution of
> -0.016 volt (except IT8603E, IT8721F/IT8758E and IT8728F: 0.012 volt.) The
> -battery voltage in8 does not have limit registers.
> -
> -On the IT8603E, IT8721F/IT8758E, IT8781F, IT8782F, and IT8783E/F, some
> +zero'; this is important for negative voltage measurements. On most chips, all
> +voltage inputs can measure voltages between 0 and 4.08 volts, with a resolution
> +of 0.016 volt.  IT8603E, IT8721F/IT8758E and IT8728F can measure between 0 and
> +3.06 volts, with a resolution of 0.012 volt.  IT8732F can measure between 0 and
> +2.8 volts with a resolution of 0.0109 volt.  The battery voltage in8 does not
> +have limit registers.
> +
> +On the IT8603E, IT8721F/IT8758E, IT8732F, IT8781F, IT8782F, and IT8783E/F, some
>  voltage inputs are internal and scaled inside the chip:
>  * in3 (optional)
>  * in7 (optional for IT8781F, IT8782F, and IT8783E/F)
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index d0ee556..4f0efe7 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -21,6 +21,7 @@
>   *            IT8721F  Super I/O chip w/LPC interface
>   *            IT8726F  Super I/O chip w/LPC interface
>   *            IT8728F  Super I/O chip w/LPC interface
> + *            IT8732F  Super I/O chip w/LPC interface
>   *            IT8758E  Super I/O chip w/LPC interface
>   *            IT8771E  Super I/O chip w/LPC interface
>   *            IT8772E  Super I/O chip w/LPC interface
> @@ -69,8 +70,9 @@
>  
>  #define DRVNAME "it87"
>  
> -enum chips { it87, it8712, it8716, it8718, it8720, it8721, it8728, it8771,
> -	     it8772, it8781, it8782, it8783, it8786, it8790, it8603, it8620 };
> +enum chips { it87, it8712, it8716, it8718, it8720, it8721, it8728, it8732,
> +	     it8771, it8772, it8781, it8782, it8783, it8786, it8790, it8603,
> +	     it8620 };
>  
>  static unsigned short force_id;
>  module_param(force_id, ushort, 0);
> @@ -147,6 +149,7 @@ static inline void superio_exit(void)
>  #define IT8720F_DEVID 0x8720
>  #define IT8721F_DEVID 0x8721
>  #define IT8726F_DEVID 0x8726
> +#define IT8732F_DEVID 0x8732
>  #define IT8728F_DEVID 0x8728
>  #define IT8771E_DEVID 0x8771
>  #define IT8772E_DEVID 0x8772
> @@ -265,6 +268,7 @@ struct it87_devices {
>  #define FEAT_VID		(1 << 9)	/* Set if chip supports VID */
>  #define FEAT_IN7_INTERNAL	(1 << 10)	/* Set if in7 is internal */
>  #define FEAT_SIX_FANS		(1 << 11)	/* Supports six fans */
> +#define FEAT_10_9MV_ADC		(1 << 12)
>  
>  static const struct it87_devices it87_devices[] = {
>  	[it87] = {
> @@ -315,6 +319,15 @@ static const struct it87_devices it87_devices[] = {
>  		  | FEAT_IN7_INTERNAL,
>  		.peci_mask = 0x07,
>  	},
> +	[it8732] = {
> +		.name = "it8732",
> +		.suffix = "F",
> +		.features = FEAT_NEWER_AUTOPWM | FEAT_16BIT_FANS
> +		  | FEAT_TEMP_OFFSET | FEAT_TEMP_OLD_PECI | FEAT_TEMP_PECI
> +		  | FEAT_10_9MV_ADC | FEAT_IN7_INTERNAL,
> +		.peci_mask = 0x07,
> +		.old_peci_mask = 0x02,	/* Actually reports PCH */
> +	},
>  	[it8771] = {
>  		.name = "it8771",
>  		.suffix = "E",
> @@ -391,6 +404,7 @@ static const struct it87_devices it87_devices[] = {
>  
>  #define has_16bit_fans(data)	((data)->features & FEAT_16BIT_FANS)
>  #define has_12mv_adc(data)	((data)->features & FEAT_12MV_ADC)
> +#define has_10_9mv_adc(data)	((data)->features & FEAT_10_9MV_ADC)
>  #define has_newer_autopwm(data)	((data)->features & FEAT_NEWER_AUTOPWM)
>  #define has_old_autopwm(data)	((data)->features & FEAT_OLD_AUTOPWM)
>  #define has_temp_offset(data)	((data)->features & FEAT_TEMP_OFFSET)
> @@ -473,23 +487,35 @@ struct it87_data {
>  	s8 auto_temp[3][5];	/* [nr][0] is point1_temp_hyst */
>  };
>  
> -static int adc_lsb(const struct it87_data *data, int nr)
> +static void adc_lsb(const struct it87_data *data, int nr, int *num, int *denom)
>  {
> -	int lsb = has_12mv_adc(data) ? 12 : 16;
> +	if (has_12mv_adc(data)) {
> +		*num =  12;
> +		*denom =  1;
> +	} else if (has_10_9mv_adc(data)) {
> +		*num =  109;
> +		*denom =  10;
> +	} else {
> +		*num =  16;
> +		*denom =  1;
> +	}
>  	if (data->in_scaled & (1 << nr))
> -		lsb <<= 1;
> -	return lsb;
> +		*num <<= 1;
>  }

I would suggest to scale all return values up by a factor of 10,
and always divide by 10 in the calling code.

>  
>  static u8 in_to_reg(const struct it87_data *data, int nr, long val)
>  {
> -	val = DIV_ROUND_CLOSEST(val, adc_lsb(data, nr));
> +	int num, denom;
> +	adc_lsb(data, nr, &num, &denom);
> +	val = DIV_ROUND_CLOSEST(val * denom, num);

.. making this 

	val = DIV_ROUND_CLOSEST(val * 10, adc_lsb(data, nr));

>  	return clamp_val(val, 0, 255);
>  }
>  
>  static int in_from_reg(const struct it87_data *data, int nr, int val)
>  {
> -	return val * adc_lsb(data, nr);
> +	int num, denom;

[ side note: use checkpatch ]

> +	adc_lsb(data, nr, &num, &denom);
> +	return val * num / denom;

similar
	return (val * adc_lsb(data, nr)) / 10;
or better
	return DIV_ROUND_CLOSEST(val * adc_lsb(data, nr), 10);

>  }
>  
>  static inline u8 FAN_TO_REG(long rpm, int div)
> @@ -1515,9 +1541,14 @@ static ssize_t show_label(struct device *dev, struct device_attribute *attr,
>  	};
>  	struct it87_data *data = dev_get_drvdata(dev);
>  	int nr = to_sensor_dev_attr(attr)->index;
> +	const char *label;
>  
> -	return sprintf(buf, "%s\n", has_12mv_adc(data) ? labels_it8721[nr]
> -						       : labels[nr]);
> +	if (has_12mv_adc(data) || has_10_9mv_adc(data))
> +		label = labels_it8721[nr];
> +	else
> +		label = labels[nr];
> +
> +	return sprintf(buf, "%s\n", label);
>  }
>  static SENSOR_DEVICE_ATTR(in3_label, S_IRUGO, show_label, NULL, 0);
>  static SENSOR_DEVICE_ATTR(in7_label, S_IRUGO, show_label, NULL, 1);
> @@ -1853,6 +1884,9 @@ static int __init it87_find(unsigned short *address,
>  	case IT8728F_DEVID:
>  		sio_data->type = it8728;
>  		break;
> +	case IT8732F_DEVID:
> +		sio_data->type = it8732;
> +		break;
>  	case IT8771E_DEVID:
>  		sio_data->type = it8771;
>  		break;
> -- 
> 2.5.0
> 
> 
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors