From mboxrd@z Thu Jan 1 00:00:00 1970 From: Guenter Roeck Date: Tue, 04 Aug 2015 20:08:22 +0000 Subject: Re: [lm-sensors] [PATCH] hwmon: (it87) Add support for IT8732F Message-Id: <20150804200822.GA8577@roeck-us.net> List-Id: References: <1331180739-19470-1-git-send-email-linux@roeck-us.net> In-Reply-To: <1331180739-19470-1-git-send-email-linux@roeck-us.net> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: lm-sensors@vger.kernel.org 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 > --- > 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