Hi Rudolf, hi Guenter,
Rudolf, thanks for the code. Guenter, thanks for the preliminary review.
On Tue, 19 Nov 2013 08:50:03 +0100, Rudolf Marek wrote:
> Add support for IT8603E.
>
> Signed-off-by: Rudolf Marek <r.marek@xxxxxxxxxxxx>
My own comments:
> Index: linux-3.12/Documentation/hwmon/it87
> ===================================================================
> --- linux-3.12.orig/Documentation/hwmon/it87 2013-11-18 23:56:09.701100351 +0100
> +++ linux-3.12/Documentation/hwmon/it87 2013-11-18 23:57:00.216465289 +0100
> @@ -2,6 +2,10 @@
> ==================
>
> Supported chips:
> + * IT8603E
> + Prefix: 'it8603'
> + Addresses scanned: from Super I/O config space (8 I/O ports)
> + Datasheet: Not publicly available
> * IT8705F
> Prefix: 'it87'
> Addresses scanned: from Super I/O config space (8 I/O ports)
> @@ -90,7 +94,7 @@
> Description
> -----------
>
> -This driver implements support for the IT8705F, IT8712F, IT8716F,
> +This driver implements support for the IT8603E, IT8705F, IT8712F, IT8716F,
> IT8718F, IT8720F, IT8721F, IT8726F, IT8728F, IT8758E, IT8771E, IT8772E,
> IT8782F, IT8783E/F, and SiS950 chips.
>
> @@ -129,6 +133,10 @@
> The IT8728F, IT8771E, and IT8772E are considered compatible with the IT8721F,
> until a datasheet becomes available (hopefully.)
>
> +The IT8603E is a custom design, hardware monitoring part is similar to
> +IT8728F. It only supports 16-bit fan mode, the full speed mode of the
> +fan is not supported (value 0 of pwmX_enable).
> +
> Temperatures are measured in degrees Celsius. An alarm is triggered once
> when the Overtemperature Shutdown limit is crossed.
>
> @@ -145,10 +153,10 @@
> 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 IT8721F/IT8758E and IT8728F: 0.012 volt.) The battery
> +0.016 volt (except IT8721F/IT8758E/IT8603E and IT8728F: 0.012 volt.) The battery
"IT8721F/IT8758E" was because both chips are undistinguishable. For all
other chips we use a comma (or "and") as a list separator.
> voltage in8 does not have limit registers.
>
> -On the IT8721F/IT8758E, IT8782F, and IT8783E/F, some voltage inputs are
> +On the IT8721F/IT8758E/IT8603E, IT8782F, and IT8783E/F, some voltage inputs are
Same here.
> internal and scaled inside the chip (in7 (optional for IT8782F and IT8783E/F),
> in8 and optionally in3). The driver handles this transparently so user-space
> doesn't have to care.
> Index: linux-3.12/drivers/hwmon/it87.c
> ===================================================================
> --- linux-3.12.orig/drivers/hwmon/it87.c 2013-11-18 23:56:09.633101207 +0100
> +++ linux-3.12/drivers/hwmon/it87.c 2013-11-18 23:59:25.998632558 +0100
> @@ -10,7 +10,8 @@
> * This driver supports only the Environment Controller in the IT8705F and
> * similar parts. The other devices are supported by different drivers.
> *
> - * Supports: IT8705F Super I/O chip w/LPC interface
> + * Supports: IT8603E Super I/O chip w/LPC interface
> + * IT8705F Super I/O chip w/LPC interface
> * IT8712F Super I/O chip w/LPC interface
> * IT8716F Super I/O chip w/LPC interface
> * IT8718F Super I/O chip w/LPC interface
> @@ -64,7 +65,7 @@
> #define DRVNAME "it87"
>
> enum chips { it87, it8712, it8716, it8718, it8720, it8721, it8728, it8771,
> - it8772, it8782, it8783 };
> + it8772, it8782, it8783, it8603 };
>
> static unsigned short force_id;
> module_param(force_id, ushort, 0);
> @@ -146,6 +147,7 @@
> #define IT8772E_DEVID 0x8772
> #define IT8782F_DEVID 0x8782
> #define IT8783E_DEVID 0x8783
> +#define IT8306E_DEVID 0x8603
> #define IT87_ACT_REG 0x30
> #define IT87_BASE_REG 0x60
>
> @@ -315,6 +317,12 @@
> | FEAT_TEMP_OLD_PECI,
> .old_peci_mask = 0x4,
> },
> + [it8603] = {
> + .name = "it8603",
> + .features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS
> + | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI,
> + .peci_mask = 0x07,
> + },
> };
>
> #define has_16bit_fans(data) ((data)->features & FEAT_16BIT_FANS)
> @@ -334,7 +342,7 @@
> u8 revision;
> u8 vid_value;
> u8 beep_pin;
> - u8 internal; /* Internal sensors can be labeled */
> + u16 internal; /* Internal sensors can be labeled */
I see no point in this change as you use bit 3 for in9.
> /* Features skipped based on config or DMI */
> u16 skip_in;
> u8 skip_vid;
> @@ -361,7 +369,7 @@
> unsigned long last_updated; /* In jiffies */
>
> u16 in_scaled; /* Internal voltage sensors are scaled */
> - u8 in[9][3]; /* [nr][0]=in, [1]=min, [2]=max */
> + u8 in[10][3]; /* [nr][0]=in, [1]=min, [2]=max */
> u8 has_fan; /* Bitfield, fans enabled */
> u16 fan[5][2]; /* Register values, [nr][0]=fan, [1]=min */
> u8 has_temp; /* Bitfield, temp sensors enabled */
> @@ -578,6 +586,8 @@
> 7, 2);
>
> static SENSOR_DEVICE_ATTR_2(in8_input, S_IRUGO, show_in, NULL, 8, 0);
> +static SENSOR_DEVICE_ATTR_2(in9_input, S_IRUGO, show_in, NULL, 9, 0);
> +
Unneeded blank line addition.
>
> /* 3 temperatures */
> static ssize_t show_temp(struct device *dev, struct device_attribute *attr,
> @@ -734,7 +744,7 @@
> {
> int ctrl = data->fan_main_ctrl & (1 << nr);
>
> - if (ctrl == 0) /* Full speed */
> + if (ctrl == 0 && data->type != it8603) /* Full speed */
Unneeded space between tabs and comment.
> return 0;
> if (data->pwm_ctrl[nr] & 0x80) /* Automatic mode */
> return 2;
> @@ -929,6 +939,10 @@
> return -EINVAL;
> }
>
> + /* IT8603E does not have on/off mode */
> + if (val == 0 && data->type == it8603)
> + return -EINVAL;
> +
> mutex_lock(&data->update_lock);
>
> if (val == 0) {
> @@ -948,10 +962,13 @@
> else /* Automatic mode */
> data->pwm_ctrl[nr] = 0x80 | data->pwm_temp_map[nr];
> it87_write_value(data, IT87_REG_PWM(nr), data->pwm_ctrl[nr]);
> - /* set SmartGuardian mode */
> - data->fan_main_ctrl |= (1 << nr);
> - it87_write_value(data, IT87_REG_FAN_MAIN_CTRL,
> - data->fan_main_ctrl);
> +
> + if (data->type != it8603) {
> + /* set SmartGuardian mode */
> + data->fan_main_ctrl |= (1 << nr);
> + it87_write_value(data, IT87_REG_FAN_MAIN_CTRL,
> + data->fan_main_ctrl);
> + }
> }
>
> mutex_unlock(&data->update_lock);
> @@ -1406,15 +1423,25 @@
> "3VSB",
> "Vbat",
> };
> + static const char * const labels_it8603[] = {
> + "AVCC3",
> + "3VSB",
> + "Vbat",
> + };
I see no good reason to add a separate list for the IT8603E, the list
is the same as labels_it8721 above except for "+3.3V" which has been
renamed to "AVCC3" but really that's exactly the same. Let's reuse
labels_it8721 to optimize the driver size.
> struct it87_data *data = dev_get_drvdata(dev);
> int nr = to_sensor_dev_attr(attr)->index;
>
> + if (data->type == it8603)
> + return sprintf(buf, "%s\n", labels_it8603[nr]);
> +
> return sprintf(buf, "%s\n", has_12mv_adc(data) ? labels_it8721[nr]
> : labels[nr]);
> }
> static SENSOR_DEVICE_ATTR(in3_label, S_IRUGO, show_label, NULL, 0);
> static SENSOR_DEVICE_ATTR(in7_label, S_IRUGO, show_label, NULL, 1);
> static SENSOR_DEVICE_ATTR(in8_label, S_IRUGO, show_label, NULL, 2);
> +/* special AVCC3 IT8306 in9 */
Missing trailing "E".
> +static SENSOR_DEVICE_ATTR(in9_label, S_IRUGO, show_label, NULL, 0);
>
> static ssize_t show_name(struct device *dev, struct device_attribute
> *devattr, char *buf)
> @@ -1424,7 +1451,7 @@
> }
> static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
>
> -static struct attribute *it87_attributes_in[9][5] = {
> +static struct attribute *it87_attributes_in[10][5] = {
> {
> &sensor_dev_attr_in0_input.dev_attr.attr,
> &sensor_dev_attr_in0_min.dev_attr.attr,
> @@ -1476,9 +1503,12 @@
> }, {
> &sensor_dev_attr_in8_input.dev_attr.attr,
> NULL
> +}, {
> + &sensor_dev_attr_in9_input.dev_attr.attr,
> + NULL
> } };
>
> -static const struct attribute_group it87_group_in[9] = {
> +static const struct attribute_group it87_group_in[10] = {
> { .attrs = it87_attributes_in[0] },
> { .attrs = it87_attributes_in[1] },
> { .attrs = it87_attributes_in[2] },
> @@ -1488,6 +1518,7 @@
> { .attrs = it87_attributes_in[6] },
> { .attrs = it87_attributes_in[7] },
> { .attrs = it87_attributes_in[8] },
> + { .attrs = it87_attributes_in[9] },
> };
>
> static struct attribute *it87_attributes_temp[3][6] = {
> @@ -1685,6 +1716,7 @@
> &sensor_dev_attr_in3_label.dev_attr.attr,
> &sensor_dev_attr_in7_label.dev_attr.attr,
> &sensor_dev_attr_in8_label.dev_attr.attr,
> + &sensor_dev_attr_in9_label.dev_attr.attr,
> NULL
> };
>
> @@ -1742,6 +1774,9 @@
> case IT8783E_DEVID:
> sio_data->type = it8783;
> break;
> + case IT8306E_DEVID:
> + sio_data->type = it8603;
> + break;
> case 0xffff: /* No device at all */
> goto exit;
> default:
> @@ -1763,11 +1798,13 @@
>
> err = 0;
> sio_data->revision = superio_inb(DEVREV) & 0x0f;
> - pr_info("Found IT%04xF chip at 0x%x, revision %d\n",
> - chip_type, *address, sio_data->revision);
> + pr_info("Found IT%04x%c chip at 0x%x, revision %d\n", chip_type,
> + chip_type == 0x8603 ? 'E' : 'F', *address,
> + sio_data->revision);
The IT877x chips are also 'E' not 'F', but this can be fixed in a
subsequent patch at it isn't related to IT8603E support.
>
> /* in8 (Vbat) is always internal */
> sio_data->internal = (1 << 2);
> + sio_data->skip_in |= (1 << 9); /* No VIN9 by default */
Please stick to the comment position already in place. Furthermore you
could do it only if sio_data->type != it8603, that way you don't have
to undo it later.
>
> /* Read GPIO config and VID value from LDN 7 (GPIO) */
> if (sio_data->type == it87) {
> @@ -1844,7 +1881,42 @@
> sio_data->internal |= (1 << 1);
>
> sio_data->beep_pin = superio_inb(IT87_SIO_BEEP_PIN_REG) & 0x3f;
> + } if (sio_data->type == it8603) {
This coding style is misleading. Either you have an "else", or you move
the "if" to the following line. I think you're missing the "else"
actually, and without it you introduce a big regression in the it87
driver.
Note that checkpatch DOES warn about this.
> + int reg27, reg29;
> +
> + sio_data->skip_vid = 1; /* No VID */
> + superio_select(GPIO);
>
> + reg27 = superio_inb(IT87_SIO_GPIO3_REG);
> +
> + /* Check if fan3 is there or not */
> + if (reg27 & (1 << 6))
> + sio_data->skip_pwm |= (1 << 2);
> + if (reg27 & (1 << 7))
> + sio_data->skip_fan |= (1 << 2);
> +
> + /* Check if fan2 is there or not */
> + reg29 = superio_inb(IT87_SIO_GPIO5_REG);
> + if (reg29 & (1 << 1))
> + sio_data->skip_pwm |= (1 << 1);
> + if (reg29 & (1 << 2))
> + sio_data->skip_fan |= (1 << 1);
> +
> + sio_data->skip_in |= (1 << 5); /* No VIN5 */
> + sio_data->skip_in |= (1 << 6); /* No VIN6 */
> +
> + /* no fan4 */
> + sio_data->skip_pwm |= (1 << 3);
> + sio_data->skip_fan |= (1 << 3);
> +
> + /* AVCC3 needs a special handling, map 0x2f to in9 */
> + sio_data->internal |= (1 << 3); /* in9 has label */
> + sio_data->skip_in &= ~(1 << 9); /* VIN9 mapped to 0x2f */
> +
> + sio_data->internal |= (1 << 1); /* in7 is VSB */
> + sio_data->internal |= (1 << 2); /* in8 is VBAT */
VBAT is already handled in the common code above.
> +
> + sio_data->beep_pin = superio_inb(IT87_SIO_BEEP_PIN_REG) & 0x3f;
> } else {
> int reg;
> bool uart6;
> @@ -2072,6 +2144,9 @@
> /* Check PWM configuration */
> enable_pwm_interface = it87_check_pwm(dev);
>
> + if (data->type == it8603)
> + data->in_scaled |= (1 << 9); /* in9 is AVCC3 */
I see no good reason for special-casing this, when it can be handled
genericly in the block below. Plus there was no point in setting
sio_data->internal |= (1 << 3) before if you were not going to check it
here.
> +
> /* Starting with IT8721F, we handle scaling of internal voltages */
> if (has_12mv_adc(data)) {
> if (sio_data->internal & (1 << 0))
> @@ -2102,7 +2177,7 @@
> if (err)
> return err;
>
> - for (i = 0; i < 9; i++) {
> + for (i = 0; i < 10; i++) {
Same change is needed at removal time.
> if (sio_data->skip_in & (1 << i))
> continue;
> err = sysfs_create_group(&dev->kobj, &it87_group_in[i]);
> @@ -2202,7 +2277,7 @@
> }
>
> /* Export labels for internal sensors */
> - for (i = 0; i < 3; i++) {
> + for (i = 0; i < 4; i++) {
> if (!(sio_data->internal & (1 << i)))
> continue;
> err = sysfs_create_file(&dev->kobj,
> @@ -2383,8 +2458,8 @@
> }
> data->has_fan = (data->fan_main_ctrl >> 4) & 0x07;
>
> - /* Set tachometers to 16-bit mode if needed */
> - if (has_16bit_fans(data)) {
> + /* Set tachometers to 16-bit mode if needed, IT8603E/IT8728? has it by default */
Missing "F" in comment, and line too long.
> + if (has_16bit_fans(data) && data->type != it8603) {
> tmp = it87_read_value(data, IT87_REG_FAN_16BIT);
> if (~tmp & 0x07 & data->has_fan) {
> dev_dbg(&pdev->dev,
> @@ -2462,6 +2537,10 @@
> data->in[i][2] =
> it87_read_value(data, IT87_REG_VIN_MAX(i));
> }
> +
> + if (data->type == it8603)
> + data->in[9][0] = it87_read_value(data, 0x2f);
> +
> /* in8 (battery) has no limit registers */
> data->in[8][0] = it87_read_value(data, IT87_REG_VIN(8));
>
>
>
I've make all the suggested changes myself to avoid another round trip.
Also I've updated Kconfig as suggested by Guenter. The resulting patch
can be seen at:
http://khali.linux-fr.org/devel/linux-3/jdelvare-hwmon/hwmon-it87-01-add-it8603e-support.patch
Please let me know if I got anything wrong. Retesting would be
appreciated, just in case.
I have also made the support available as a standalone driver which
builds at least down to kernel 3.0:
http://khali.linux-fr.org/devel/lm-sensors/drivers/it87/
This might make testing easier.
Thanks,
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
