| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170809134252.GA18861@roeck-us.net>
Date: Wed, 9 Aug 2017 06:42:52 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
Cc: Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org,
linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [v5, 1/2] hwmon: (it87) Split out chip registers setting code on
probe path
On Wed, Aug 09, 2017 at 12:05:30PM +0200, Maciej S. Szmigiero wrote:
> This commit splits out chip registers setting code on probe path to
> separate functions so they can be reused for setting the device properly
> again when system resumes from suspend.
>
> While we are at it let's also make clear that on IT8720 and IT8782 it's
> the VCCH5V line that is (possibly) routed to in7.
> This will make it consistent with a similar message that it printed on
> IT8783.
>
> Signed-off-by: Maciej S. Szmigiero <mail@...iej.szmigiero.name>
Applied to hwmon-next.
Thanks,
Guenter
> ---
> Changes from v1: Move code of common probe / resume steps to new functions
> so we don't need to make large parts of probe function conditional on a
> newly added 'resume' parameter.
>
> Changes from v2: Code move of common probe / resume steps to new functions
> and actual resume functionality split into two, separate patches.
>
> Made a message about VCCH5V being routed to in7 consistent across all
> chips.
>
> Changes from v3, v4: Fix patch formatting.
>
> drivers/hwmon/it87.c | 138 ++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 88 insertions(+), 50 deletions(-)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index 4dfc7238313e..818f123ac475 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -2761,13 +2761,13 @@ static int __init it87_find(int sioaddr, unsigned short *address,
> uart6 = sio_data->type == it8782 && (reg & BIT(2));
>
> /*
> - * The IT8720F has no VIN7 pin, so VCCH should always be
> + * The IT8720F has no VIN7 pin, so VCCH5V should always be
> * routed internally to VIN7 with an internal divider.
> * Curiously, there still is a configuration bit to control
> * this, which means it can be set incorrectly. And even
> * more curiously, many boards out there are improperly
> * configured, even though the IT8720F datasheet claims
> - * that the internal routing of VCCH to VIN7 is the default
> + * that the internal routing of VCCH5V to VIN7 is the default
> * setting. So we force the internal routing in this case.
> *
> * On IT8782F, VIN7 is multiplexed with one of the UART6 pins.
> @@ -2777,7 +2777,7 @@ static int __init it87_find(int sioaddr, unsigned short *address,
> if ((sio_data->type == it8720 || uart6) && !(reg & BIT(1))) {
> reg |= BIT(1);
> superio_outb(sioaddr, IT87_SIO_PINX2_REG, reg);
> - pr_notice("Routing internal VCCH to in7\n");
> + pr_notice("Routing internal VCCH5V to in7\n");
> }
> if (reg & BIT(0))
> sio_data->internal |= BIT(0);
> @@ -2828,13 +2828,89 @@ static int __init it87_find(int sioaddr, unsigned short *address,
> return err;
> }
>
> +/*
> + * Some chips seem to have default value 0xff for all limit
> + * registers. For low voltage limits it makes no sense and triggers
> + * alarms, so change to 0 instead. For high temperature limits, it
> + * means -1 degree C, which surprisingly doesn't trigger an alarm,
> + * but is still confusing, so change to 127 degrees C.
> + */
> +static void it87_check_limit_regs(struct it87_data *data)
> +{
> + int i, reg;
> +
> + for (i = 0; i < NUM_VIN_LIMIT; i++) {
> + reg = it87_read_value(data, IT87_REG_VIN_MIN(i));
> + if (reg == 0xff)
> + it87_write_value(data, IT87_REG_VIN_MIN(i), 0);
> + }
> + for (i = 0; i < 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);
> + }
> +}
> +
> +/* Check if voltage monitors are reset manually or by some reason */
> +static void it87_check_voltage_monitors_reset(struct it87_data *data)
> +{
> + int reg;
> +
> + reg = it87_read_value(data, IT87_REG_VIN_ENABLE);
> + if ((reg & 0xff) == 0) {
> + /* Enable all voltage monitors */
> + it87_write_value(data, IT87_REG_VIN_ENABLE, 0xff);
> + }
> +}
> +
> +/* Check if tachometers are reset manually or by some reason */
> +static void it87_check_tachometers_reset(struct platform_device *pdev)
> +{
> + struct it87_sio_data *sio_data = dev_get_platdata(&pdev->dev);
> + struct it87_data *data = platform_get_drvdata(pdev);
> + u8 mask, fan_main_ctrl;
> +
> + mask = 0x70 & ~(sio_data->skip_fan << 4);
> + fan_main_ctrl = it87_read_value(data, IT87_REG_FAN_MAIN_CTRL);
> + if ((fan_main_ctrl & mask) == 0) {
> + /* Enable all fan tachometers */
> + fan_main_ctrl |= mask;
> + it87_write_value(data, IT87_REG_FAN_MAIN_CTRL,
> + fan_main_ctrl);
> + }
> +}
> +
> +/* Set tachometers to 16-bit mode if needed */
> +static void it87_check_tachometers_16bit_mode(struct platform_device *pdev)
> +{
> + struct it87_data *data = platform_get_drvdata(pdev);
> + int reg;
> +
> + if (!has_fan16_config(data))
> + return;
> +
> + reg = it87_read_value(data, IT87_REG_FAN_16BIT);
> + if (~reg & 0x07 & data->has_fan) {
> + dev_dbg(&pdev->dev,
> + "Setting fan1-3 to 16-bit mode\n");
> + it87_write_value(data, IT87_REG_FAN_16BIT,
> + reg | 0x07);
> + }
> +}
> +
> +static void it87_start_monitoring(struct it87_data *data)
> +{
> + it87_write_value(data, IT87_REG_CONFIG,
> + (it87_read_value(data, IT87_REG_CONFIG) & 0x3e)
> + | (update_vbat ? 0x41 : 0x01));
> +}
> +
> /* Called when we have found a new IT87. */
> static void it87_init_device(struct platform_device *pdev)
> {
> struct it87_sio_data *sio_data = dev_get_platdata(&pdev->dev);
> struct it87_data *data = platform_get_drvdata(pdev);
> int tmp, i;
> - u8 mask;
>
> /*
> * For each PWM channel:
> @@ -2855,23 +2931,7 @@ static void it87_init_device(struct platform_device *pdev)
> data->auto_pwm[i][3] = 0x7f; /* Full speed, hard-coded */
> }
>
> - /*
> - * Some chips seem to have default value 0xff for all limit
> - * registers. For low voltage limits it makes no sense and triggers
> - * alarms, so change to 0 instead. For high temperature limits, it
> - * means -1 degree C, which surprisingly doesn't trigger an alarm,
> - * but is still confusing, so change to 127 degrees C.
> - */
> - for (i = 0; i < NUM_VIN_LIMIT; i++) {
> - tmp = it87_read_value(data, IT87_REG_VIN_MIN(i));
> - if (tmp == 0xff)
> - it87_write_value(data, IT87_REG_VIN_MIN(i), 0);
> - }
> - for (i = 0; i < NUM_TEMP_LIMIT; i++) {
> - tmp = it87_read_value(data, IT87_REG_TEMP_HIGH(i));
> - if (tmp == 0xff)
> - it87_write_value(data, IT87_REG_TEMP_HIGH(i), 127);
> - }
> + it87_check_limit_regs(data);
>
> /*
> * Temperature channels are not forcibly enabled, as they can be
> @@ -2880,38 +2940,19 @@ static void it87_init_device(struct platform_device *pdev)
> * run-time through the temp{1-3}_type sysfs accessors if needed.
> */
>
> - /* Check if voltage monitors are reset manually or by some reason */
> - tmp = it87_read_value(data, IT87_REG_VIN_ENABLE);
> - if ((tmp & 0xff) == 0) {
> - /* Enable all voltage monitors */
> - it87_write_value(data, IT87_REG_VIN_ENABLE, 0xff);
> - }
> + it87_check_voltage_monitors_reset(data);
> +
> + it87_check_tachometers_reset(pdev);
>
> - /* Check if tachometers are reset manually or by some reason */
> - mask = 0x70 & ~(sio_data->skip_fan << 4);
> data->fan_main_ctrl = it87_read_value(data, IT87_REG_FAN_MAIN_CTRL);
> - if ((data->fan_main_ctrl & mask) == 0) {
> - /* Enable all fan tachometers */
> - data->fan_main_ctrl |= mask;
> - it87_write_value(data, IT87_REG_FAN_MAIN_CTRL,
> - data->fan_main_ctrl);
> - }
> data->has_fan = (data->fan_main_ctrl >> 4) & 0x07;
>
> - tmp = it87_read_value(data, IT87_REG_FAN_16BIT);
> -
> - /* Set tachometers to 16-bit mode if needed */
> - if (has_fan16_config(data)) {
> - if (~tmp & 0x07 & data->has_fan) {
> - dev_dbg(&pdev->dev,
> - "Setting fan1-3 to 16-bit mode\n");
> - it87_write_value(data, IT87_REG_FAN_16BIT,
> - tmp | 0x07);
> - }
> - }
> + it87_check_tachometers_16bit_mode(pdev);
>
> /* Check for additional fans */
> if (has_five_fans(data)) {
> + tmp = it87_read_value(data, IT87_REG_FAN_16BIT);
> +
> if (tmp & BIT(4))
> data->has_fan |= BIT(3); /* fan4 enabled */
> if (tmp & BIT(5))
> @@ -2933,10 +2974,7 @@ static void it87_init_device(struct platform_device *pdev)
> sio_data->skip_pwm |= BIT(5);
> }
>
> - /* Start monitoring */
> - it87_write_value(data, IT87_REG_CONFIG,
> - (it87_read_value(data, IT87_REG_CONFIG) & 0x3e)
> - | (update_vbat ? 0x41 : 0x01));
> + it87_start_monitoring(data);
> }
>
> /* Return 1 if and only if the PWM interface is safe to use */
Powered by blists - more mailing lists