Re: [PATCH v6] hwmon: (it87) Reapply probe path chip registers settings after resume

[Guenter Roeck]

On Wed, Aug 09, 2017 at 04:36:33PM +0200, Maciej S. Szmigiero wrote:
> After a suspend / resume cycle we possibly need to reapply chip registers
> settings that we had set or fixed in a probe path, since they might have
> been reset to default values or set incorrectly by a BIOS again.
> 
> Tested on a Gigabyte M720-US3 board, which requires routing internal VCCH5V
> to in7 (and had it wrong again on resume from S3).
> 
> Signed-off-by: Maciej S. Szmigiero 

Sorry, couple of additional comments.

> ---
> 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.
> 
> Changed message level about VCCH5V being routed to in7 again on resume to
> debug.
> 
> Added a "need_in7_reroute" flag and made VCCH5V to in7 rerouting on resume
> explicitly conditional on it.
> 
> Didn't finally add check for enable_pwm_interface being set before calling
> it87_check_pwm() in the resume function since this variable is it87_probe()
> function local and so adding this check would require adding it to device
> instance structure.
> 
> Changes from v3, v4: Fix patch formatting.
> 
> Changes from v5: Use 'bool' for on / off variables, don't change it87_find()
> function arguments to minimize the changes.
> 
>  drivers/hwmon/it87.c | 79 
> ++--
>  1 file changed, 77 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index 818f123ac475..f1000a8a2dd2 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -497,12 +497,14 @@ static const struct it87_devices it87_devices[] = {
>  #define has_vin3_5v(data)((data)->features & FEAT_VIN3_5V)
>  
>  struct it87_sio_data {
> + int sioaddr;
>   enum chips type;
>   /* Values read from Super-I/O config space */
>   u8 revision;
>   u8 vid_value;
>   u8 beep_pin;
>   u8 internal;/* Internal sensors can be labeled */
> + bool need_in7_reroute;
>   /* Features skipped based on config or DMI */
>   u16 skip_in;
>   u8 skip_vid;
> @@ -517,6 +519,7 @@ struct it87_sio_data {
>   */
>  struct it87_data {
>   const struct attribute_group *groups[7];
> + int sioaddr;
>   enum chips type;
>   u32 features;
>   u8 peci_mask;
> @@ -532,6 +535,7 @@ struct it87_data {
>   u16 in_internal;/* Bitfield, internal sensors (for labels) */
>   u16 has_in; /* Bitfield, voltage sensors enabled */
>   u8 in[NUM_VIN][3];  /* [nr][0]=in, [1]=min, [2]=max */
> + bool need_in7_reroute;
>   u8 has_fan; /* Bitfield, fans enabled */
>   u16 fan[NUM_FAN][2];/* Register values, [nr][0]=fan, [1]=min */
>   u8 has_temp;/* Bitfield, temp sensors enabled */
> @@ -2487,6 +2491,7 @@ static int __init it87_find(int sioaddr, unsigned short 
> *address,
>   }
>  
>   err = 0;
> + sio_data->sioaddr = sioaddr;
>   sio_data->revision = superio_inb(sioaddr, DEVREV) & 0x0f;
>   pr_info("Found IT%04x%s chip at 0x%x, revision %d\n", chip_type,
>   it87_devices[sio_data->type].suffix,
> @@ -2575,6 +2580,7 @@ static int __init it87_find(int sioaddr, unsigned short 
> *address,
>   reg2c |= BIT(1);
>   superio_outb(sioaddr, IT87_SIO_PINX2_REG,
>reg2c);
> + sio_data->need_in7_reroute = true;
>   pr_notice("Routing internal VCCH5V to in7.\n");
>   }
>   pr_notice("in7 routed to internal voltage divider, with 
> external pin disabled.\n");
> @@ -2777,6 +2783,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);
> + sio_data->need_in7_reroute = true;
>   pr_notice("Routing internal VCCH5V to in7\n");
>   }
>   if (reg & BIT(0))
> @@ -3024,8 +3031,6 @@ static int it87_check_pwm(struct device *dev)
>"PWM configuration is too broken to be 
> fixed\n");
>   }
>  
> - dev_info(dev,
> -  "Detected broken BIOS defaults, disabling PWM 
> interface\n");
>   return 0;
>   } else if (fix_pwm_polarity) {
>   dev_info(dev,
> @@ -3058,6 +3063,7 @@ static int it87_probe(struct platform_device *pdev)
>   return -ENOMEM;
>  
>   data->addr = res->start;
> + data->sioaddr = sio_data->sioaddr;
>

Re: [PATCH v6] hwmon: (it87) Reapply probe path chip registers settings after resume

[Guenter Roeck]

On Wed, Aug 09, 2017 at 04:36:33PM +0200, Maciej S. Szmigiero wrote:
> After a suspend / resume cycle we possibly need to reapply chip registers
> settings that we had set or fixed in a probe path, since they might have
> been reset to default values or set incorrectly by a BIOS again.
> 
> Tested on a Gigabyte M720-US3 board, which requires routing internal VCCH5V
> to in7 (and had it wrong again on resume from S3).
> 
> Signed-off-by: Maciej S. Szmigiero 

Sorry, couple of additional comments.

> ---
> 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.
> 
> Changed message level about VCCH5V being routed to in7 again on resume to
> debug.
> 
> Added a "need_in7_reroute" flag and made VCCH5V to in7 rerouting on resume
> explicitly conditional on it.
> 
> Didn't finally add check for enable_pwm_interface being set before calling
> it87_check_pwm() in the resume function since this variable is it87_probe()
> function local and so adding this check would require adding it to device
> instance structure.
> 
> Changes from v3, v4: Fix patch formatting.
> 
> Changes from v5: Use 'bool' for on / off variables, don't change it87_find()
> function arguments to minimize the changes.
> 
>  drivers/hwmon/it87.c | 79 
> ++--
>  1 file changed, 77 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index 818f123ac475..f1000a8a2dd2 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -497,12 +497,14 @@ static const struct it87_devices it87_devices[] = {
>  #define has_vin3_5v(data)((data)->features & FEAT_VIN3_5V)
>  
>  struct it87_sio_data {
> + int sioaddr;
>   enum chips type;
>   /* Values read from Super-I/O config space */
>   u8 revision;
>   u8 vid_value;
>   u8 beep_pin;
>   u8 internal;/* Internal sensors can be labeled */
> + bool need_in7_reroute;
>   /* Features skipped based on config or DMI */
>   u16 skip_in;
>   u8 skip_vid;
> @@ -517,6 +519,7 @@ struct it87_sio_data {
>   */
>  struct it87_data {
>   const struct attribute_group *groups[7];
> + int sioaddr;
>   enum chips type;
>   u32 features;
>   u8 peci_mask;
> @@ -532,6 +535,7 @@ struct it87_data {
>   u16 in_internal;/* Bitfield, internal sensors (for labels) */
>   u16 has_in; /* Bitfield, voltage sensors enabled */
>   u8 in[NUM_VIN][3];  /* [nr][0]=in, [1]=min, [2]=max */
> + bool need_in7_reroute;
>   u8 has_fan; /* Bitfield, fans enabled */
>   u16 fan[NUM_FAN][2];/* Register values, [nr][0]=fan, [1]=min */
>   u8 has_temp;/* Bitfield, temp sensors enabled */
> @@ -2487,6 +2491,7 @@ static int __init it87_find(int sioaddr, unsigned short 
> *address,
>   }
>  
>   err = 0;
> + sio_data->sioaddr = sioaddr;
>   sio_data->revision = superio_inb(sioaddr, DEVREV) & 0x0f;
>   pr_info("Found IT%04x%s chip at 0x%x, revision %d\n", chip_type,
>   it87_devices[sio_data->type].suffix,
> @@ -2575,6 +2580,7 @@ static int __init it87_find(int sioaddr, unsigned short 
> *address,
>   reg2c |= BIT(1);
>   superio_outb(sioaddr, IT87_SIO_PINX2_REG,
>reg2c);
> + sio_data->need_in7_reroute = true;
>   pr_notice("Routing internal VCCH5V to in7.\n");
>   }
>   pr_notice("in7 routed to internal voltage divider, with 
> external pin disabled.\n");
> @@ -2777,6 +2783,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);
> + sio_data->need_in7_reroute = true;
>   pr_notice("Routing internal VCCH5V to in7\n");
>   }
>   if (reg & BIT(0))
> @@ -3024,8 +3031,6 @@ static int it87_check_pwm(struct device *dev)
>"PWM configuration is too broken to be 
> fixed\n");
>   }
>  
> - dev_info(dev,
> -  "Detected broken BIOS defaults, disabling PWM 
> interface\n");
>   return 0;
>   } else if (fix_pwm_polarity) {
>   dev_info(dev,
> @@ -3058,6 +3063,7 @@ static int it87_probe(struct platform_device *pdev)
>   return -ENOMEM;
>  
>   data->addr = res->start;
> + data->sioaddr = sio_data->sioaddr;
>   data->type = 

[PATCH v6] hwmon: (it87) Reapply probe path chip registers settings after resume

[Maciej S. Szmigiero]

After a suspend / resume cycle we possibly need to reapply chip registers
settings that we had set or fixed in a probe path, since they might have
been reset to default values or set incorrectly by a BIOS again.

Tested on a Gigabyte M720-US3 board, which requires routing internal VCCH5V
to in7 (and had it wrong again on resume from S3).

Signed-off-by: Maciej S. Szmigiero 
---
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.

Changed message level about VCCH5V being routed to in7 again on resume to
debug.

Added a "need_in7_reroute" flag and made VCCH5V to in7 rerouting on resume
explicitly conditional on it.

Didn't finally add check for enable_pwm_interface being set before calling
it87_check_pwm() in the resume function since this variable is it87_probe()
function local and so adding this check would require adding it to device
instance structure.

Changes from v3, v4: Fix patch formatting.

Changes from v5: Use 'bool' for on / off variables, don't change it87_find()
function arguments to minimize the changes.

 drivers/hwmon/it87.c | 79 ++--
 1 file changed, 77 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index 818f123ac475..f1000a8a2dd2 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -497,12 +497,14 @@ static const struct it87_devices it87_devices[] = {
 #define has_vin3_5v(data)  ((data)->features & FEAT_VIN3_5V)
 
 struct it87_sio_data {
+   int sioaddr;
enum chips type;
/* Values read from Super-I/O config space */
u8 revision;
u8 vid_value;
u8 beep_pin;
u8 internal;/* Internal sensors can be labeled */
+   bool need_in7_reroute;
/* Features skipped based on config or DMI */
u16 skip_in;
u8 skip_vid;
@@ -517,6 +519,7 @@ struct it87_sio_data {
  */
 struct it87_data {
const struct attribute_group *groups[7];
+   int sioaddr;
enum chips type;
u32 features;
u8 peci_mask;
@@ -532,6 +535,7 @@ struct it87_data {
u16 in_internal;/* Bitfield, internal sensors (for labels) */
u16 has_in; /* Bitfield, voltage sensors enabled */
u8 in[NUM_VIN][3];  /* [nr][0]=in, [1]=min, [2]=max */
+   bool need_in7_reroute;
u8 has_fan; /* Bitfield, fans enabled */
u16 fan[NUM_FAN][2];/* Register values, [nr][0]=fan, [1]=min */
u8 has_temp;/* Bitfield, temp sensors enabled */
@@ -2487,6 +2491,7 @@ static int __init it87_find(int sioaddr, unsigned short 
*address,
}
 
err = 0;
+   sio_data->sioaddr = sioaddr;
sio_data->revision = superio_inb(sioaddr, DEVREV) & 0x0f;
pr_info("Found IT%04x%s chip at 0x%x, revision %d\n", chip_type,
it87_devices[sio_data->type].suffix,
@@ -2575,6 +2580,7 @@ static int __init it87_find(int sioaddr, unsigned short 
*address,
reg2c |= BIT(1);
superio_outb(sioaddr, IT87_SIO_PINX2_REG,
 reg2c);
+   sio_data->need_in7_reroute = true;
pr_notice("Routing internal VCCH5V to in7.\n");
}
pr_notice("in7 routed to internal voltage divider, with 
external pin disabled.\n");
@@ -2777,6 +2783,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);
+   sio_data->need_in7_reroute = true;
pr_notice("Routing internal VCCH5V to in7\n");
}
if (reg & BIT(0))
@@ -3024,8 +3031,6 @@ static int it87_check_pwm(struct device *dev)
 "PWM configuration is too broken to be 
fixed\n");
}
 
-   dev_info(dev,
-"Detected broken BIOS defaults, disabling PWM 
interface\n");
return 0;
} else if (fix_pwm_polarity) {
dev_info(dev,
@@ -3058,6 +3063,7 @@ static int it87_probe(struct platform_device *pdev)
return -ENOMEM;
 
data->addr = res->start;
+   data->sioaddr = sio_data->sioaddr;
data->type = sio_data->type;
data->features = it87_devices[sio_data->type].features;
data->peci_mask = it87_devices[sio_data->type].peci_mask;
@@ -3096,6 +3102,9 @@ static int it87_probe(struct platform_device 

[PATCH v6] hwmon: (it87) Reapply probe path chip registers settings after resume

[Maciej S. Szmigiero]

After a suspend / resume cycle we possibly need to reapply chip registers
settings that we had set or fixed in a probe path, since they might have
been reset to default values or set incorrectly by a BIOS again.

Tested on a Gigabyte M720-US3 board, which requires routing internal VCCH5V
to in7 (and had it wrong again on resume from S3).

Signed-off-by: Maciej S. Szmigiero 
---
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.

Changed message level about VCCH5V being routed to in7 again on resume to
debug.

Added a "need_in7_reroute" flag and made VCCH5V to in7 rerouting on resume
explicitly conditional on it.

Didn't finally add check for enable_pwm_interface being set before calling
it87_check_pwm() in the resume function since this variable is it87_probe()
function local and so adding this check would require adding it to device
instance structure.

Changes from v3, v4: Fix patch formatting.

Changes from v5: Use 'bool' for on / off variables, don't change it87_find()
function arguments to minimize the changes.

 drivers/hwmon/it87.c | 79 ++--
 1 file changed, 77 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index 818f123ac475..f1000a8a2dd2 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -497,12 +497,14 @@ static const struct it87_devices it87_devices[] = {
 #define has_vin3_5v(data)  ((data)->features & FEAT_VIN3_5V)
 
 struct it87_sio_data {
+   int sioaddr;
enum chips type;
/* Values read from Super-I/O config space */
u8 revision;
u8 vid_value;
u8 beep_pin;
u8 internal;/* Internal sensors can be labeled */
+   bool need_in7_reroute;
/* Features skipped based on config or DMI */
u16 skip_in;
u8 skip_vid;
@@ -517,6 +519,7 @@ struct it87_sio_data {
  */
 struct it87_data {
const struct attribute_group *groups[7];
+   int sioaddr;
enum chips type;
u32 features;
u8 peci_mask;
@@ -532,6 +535,7 @@ struct it87_data {
u16 in_internal;/* Bitfield, internal sensors (for labels) */
u16 has_in; /* Bitfield, voltage sensors enabled */
u8 in[NUM_VIN][3];  /* [nr][0]=in, [1]=min, [2]=max */
+   bool need_in7_reroute;
u8 has_fan; /* Bitfield, fans enabled */
u16 fan[NUM_FAN][2];/* Register values, [nr][0]=fan, [1]=min */
u8 has_temp;/* Bitfield, temp sensors enabled */
@@ -2487,6 +2491,7 @@ static int __init it87_find(int sioaddr, unsigned short 
*address,
}
 
err = 0;
+   sio_data->sioaddr = sioaddr;
sio_data->revision = superio_inb(sioaddr, DEVREV) & 0x0f;
pr_info("Found IT%04x%s chip at 0x%x, revision %d\n", chip_type,
it87_devices[sio_data->type].suffix,
@@ -2575,6 +2580,7 @@ static int __init it87_find(int sioaddr, unsigned short 
*address,
reg2c |= BIT(1);
superio_outb(sioaddr, IT87_SIO_PINX2_REG,
 reg2c);
+   sio_data->need_in7_reroute = true;
pr_notice("Routing internal VCCH5V to in7.\n");
}
pr_notice("in7 routed to internal voltage divider, with 
external pin disabled.\n");
@@ -2777,6 +2783,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);
+   sio_data->need_in7_reroute = true;
pr_notice("Routing internal VCCH5V to in7\n");
}
if (reg & BIT(0))
@@ -3024,8 +3031,6 @@ static int it87_check_pwm(struct device *dev)
 "PWM configuration is too broken to be 
fixed\n");
}
 
-   dev_info(dev,
-"Detected broken BIOS defaults, disabling PWM 
interface\n");
return 0;
} else if (fix_pwm_polarity) {
dev_info(dev,
@@ -3058,6 +3063,7 @@ static int it87_probe(struct platform_device *pdev)
return -ENOMEM;
 
data->addr = res->start;
+   data->sioaddr = sio_data->sioaddr;
data->type = sio_data->type;
data->features = it87_devices[sio_data->type].features;
data->peci_mask = it87_devices[sio_data->type].peci_mask;
@@ -3096,6 +3102,9 @@ static int it87_probe(struct platform_device *pdev)
 
/* Check PWM