[PATCH v1 0/7] hwmon: (it87) Complete handling multi-chip configuration

[PATCH v1 0/7] hwmon: (it87) Complete handling multi-chip configuration

[Frank Crawford]

Further support of multiple chips on a motherboard to disable
exiting configuration mode, including updating existing chips and adding
new chips that match.

Allow setting multiple chip IDs for testing, and correct listed chip
names andides, when required.

Update the system documentation.

---

Frank Crawford (7):
  Allow disabling exiting of configuration mode
  Disable configuration exit for certain chips
  List full chip model name
  Add chip_id in some info message
  Allow multiple chip IDs for force_id
  Add new chipset IT87952E
  Updated documentation for recent updates to it87

 Documentation/hwmon/it87.rst |  47 ++++++++++++--
 drivers/hwmon/it87.c         | 117 +++++++++++++++++++++++------------
 2 files changed, 120 insertions(+), 44 deletions(-)

-- 
2.39.1

[PATCH v1 1/7] hwmon: (it87) Allow disabling exiting of configuration mode

[Frank Crawford]

Disabling configuration mode on some chips can result in system
hang-ups and access failures to the Super-IO chip at the
second SIO address. Never exit configuration mode on these
chips to avoid the problem.

This patch should be applied in conjunction with a previous one to
initialise the second chip for certain mother boards.

Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
---
 drivers/hwmon/it87.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index a8a6a0ffee82..7049e81f5af1 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -128,10 +128,12 @@ static inline int superio_enter(int ioreg)
 	return 0;
 }
 
-static inline void superio_exit(int ioreg)
+static inline void superio_exit(int ioreg, bool doexit)
 {
-	outb(0x02, ioreg);
-	outb(0x02, ioreg + 1);
+	if (doexit) {
+		outb(0x02, ioreg);
+		outb(0x02, ioreg + 1);
+	}
 	release_region(ioreg, 2);
 }
 
@@ -497,6 +499,7 @@ static const struct it87_devices it87_devices[] = {
 struct it87_sio_data {
 	int sioaddr;
 	enum chips type;
+	u8 doexit;
 	/* Values read from Super-I/O config space */
 	u8 revision;
 	u8 vid_value;
@@ -523,6 +526,8 @@ struct it87_data {
 	u8 peci_mask;
 	u8 old_peci_mask;
 
+	bool doexit;		/* true if exit from sio config is ok */
+
 	unsigned short addr;
 	const char *name;
 	struct mutex update_lock;
@@ -2405,6 +2410,7 @@ static int __init it87_find(int sioaddr, unsigned short *address,
 	int err;
 	u16 chip_type;
 	const struct it87_devices *config;
+	bool doexit = true;
 
 	err = superio_enter(sioaddr);
 	if (err)
@@ -2501,6 +2507,8 @@ static int __init it87_find(int sioaddr, unsigned short *address,
 		goto exit;
 	}
 
+	sio_data->doexit = doexit;
+
 	err = 0;
 	sio_data->sioaddr = sioaddr;
 	sio_data->revision = superio_inb(sioaddr, DEVREV) & 0x0f;
@@ -2827,7 +2835,7 @@ static int __init it87_find(int sioaddr, unsigned short *address,
 		sio_data->skip_pwm |= dmi_data->skip_pwm;
 
 exit:
-	superio_exit(sioaddr);
+	superio_exit(sioaddr, doexit);
 	return err;
 }
 
@@ -3061,6 +3069,7 @@ static int it87_probe(struct platform_device *pdev)
 	data->addr = res->start;
 	data->sioaddr = sio_data->sioaddr;
 	data->type = sio_data->type;
+	data->doexit = sio_data->doexit;
 	data->features = it87_devices[sio_data->type].features;
 	data->peci_mask = it87_devices[sio_data->type].peci_mask;
 	data->old_peci_mask = it87_devices[sio_data->type].old_peci_mask;
@@ -3213,7 +3222,7 @@ static void it87_resume_sio(struct platform_device *pdev)
 			     reg2c);
 	}
 
-	superio_exit(data->sioaddr);
+	superio_exit(data->sioaddr, data->doexit);
 }
 
 static int it87_resume(struct device *dev)
-- 
2.39.1

[PATCH v1 2/7] hwmon: (it87) Disable configuration exit for certain chips

[Frank Crawford]

IT8790E and IT8792E/IT8795E have been identified as chips that can have
issues when disabling configuration mode.

Set to never exit configuration mode.

Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
---
 drivers/hwmon/it87.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index 7049e81f5af1..c5e46b94f0b8 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -2453,6 +2453,13 @@ static int __init it87_find(int sioaddr, unsigned short *address,
 		break;
 	case IT8792E_DEVID:
 		sio_data->type = it8792;
+		/*
+		 * Disabling configuration mode on IT8792E can result in system
+		 * hang-ups and access failures to the Super-IO chip at the
+		 * second SIO address. Never exit configuration mode on this
+		 * chip to avoid the problem.
+		 */
+		doexit = false;
 		break;
 	case IT8771E_DEVID:
 		sio_data->type = it8771;
@@ -2474,6 +2481,7 @@ static int __init it87_find(int sioaddr, unsigned short *address,
 		break;
 	case IT8790E_DEVID:
 		sio_data->type = it8790;
+		doexit = false;	/* See IT8792E comment above */
 		break;
 	case IT8603E_DEVID:
 	case IT8623E_DEVID:
-- 
2.39.1

Re: [PATCH v1 2/7] hwmon: (it87) Disable configuration exit for certain chips

[Guenter Roeck]

On Thu, Jan 26, 2023 at 03:02:18PM +1100, Frank Crawford wrote:
> IT8790E and IT8792E/IT8795E have been identified as chips that can have
> issues when disabling configuration mode.
> 
> Set to never exit configuration mode.
> 
> Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
> ---
>  drivers/hwmon/it87.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index 7049e81f5af1..c5e46b94f0b8 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -2453,6 +2453,13 @@ static int __init it87_find(int sioaddr, unsigned short *address,
>  		break;
>  	case IT8792E_DEVID:
>  		sio_data->type = it8792;
> +		/*
> +		 * Disabling configuration mode on IT8792E can result in system
> +		 * hang-ups and access failures to the Super-IO chip at the
> +		 * second SIO address. Never exit configuration mode on this
> +		 * chip to avoid the problem.
> +		 */
> +		doexit = false;

Why not just use a feature flag ?

Guenter

>  		break;
>  	case IT8771E_DEVID:
>  		sio_data->type = it8771;
> @@ -2474,6 +2481,7 @@ static int __init it87_find(int sioaddr, unsigned short *address,
>  		break;
>  	case IT8790E_DEVID:
>  		sio_data->type = it8790;
> +		doexit = false;	/* See IT8792E comment above */
>  		break;
>  	case IT8603E_DEVID:
>  	case IT8623E_DEVID:
> -- 
> 2.39.1
> 

Re: [PATCH v1 2/7] hwmon: (it87) Disable configuration exit for certain chips

[Frank Crawford]

On Thu, 2023-01-26 at 05:42 -0800, Guenter Roeck wrote:
> On Thu, Jan 26, 2023 at 03:02:18PM +1100, Frank Crawford wrote:
> > IT8790E and IT8792E/IT8795E have been identified as chips that can
> > have
> > issues when disabling configuration mode.
> > 
> > Set to never exit configuration mode.
> > 
> > Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
> > ---
> >  drivers/hwmon/it87.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> > index 7049e81f5af1..c5e46b94f0b8 100644
> > --- a/drivers/hwmon/it87.c
> > +++ b/drivers/hwmon/it87.c
> > @@ -2453,6 +2453,13 @@ static int __init it87_find(int sioaddr,
> > unsigned short *address,
> >                 break;
> >         case IT8792E_DEVID:
> >                 sio_data->type = it8792;
> > +               /*
> > +                * Disabling configuration mode on IT8792E can
> > result in system
> > +                * hang-ups and access failures to the Super-IO
> > chip at the
> > +                * second SIO address. Never exit configuration
> > mode on this
> > +                * chip to avoid the problem.
> > +                */
> > +               doexit = false;
> 
> Why not just use a feature flag ?

Good point.  I'll rework it to do that.

> 
> Guenter

Regards
Frank
> 
> >                 break;
> >         case IT8771E_DEVID:
> >                 sio_data->type = it8771;
> > @@ -2474,6 +2481,7 @@ static int __init it87_find(int sioaddr,
> > unsigned short *address,
> >                 break;
> >         case IT8790E_DEVID:
> >                 sio_data->type = it8790;
> > +               doexit = false; /* See IT8792E comment above */
> >                 break;
> >         case IT8603E_DEVID:
> >         case IT8623E_DEVID:
> > -- 
> > 2.39.1
> > 

[PATCH v1 3/7] hwmon: (it87) List full chip model name

[Frank Crawford]

ITE model names do not always match the chip ID.

Convert from just adding a suffix to specifying the full model name.

Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
---
 drivers/hwmon/it87.c | 46 ++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index c5e46b94f0b8..16394b38dce3 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -277,7 +277,7 @@ static const u8 IT87_REG_AUTO_BASE[] = { 0x60, 0x68, 0x70, 0x78, 0xa0, 0xa8 };
 
 struct it87_devices {
 	const char *name;
-	const char * const suffix;
+	const char * const model;
 	u32 features;
 	u8 peci_mask;
 	u8 old_peci_mask;
@@ -306,24 +306,24 @@ struct it87_devices {
 static const struct it87_devices it87_devices[] = {
 	[it87] = {
 		.name = "it87",
-		.suffix = "F",
+		.model = "IT87F",
 		.features = FEAT_OLD_AUTOPWM,	/* may need to overwrite */
 	},
 	[it8712] = {
 		.name = "it8712",
-		.suffix = "F",
+		.model = "IT8712F",
 		.features = FEAT_OLD_AUTOPWM | FEAT_VID,
 						/* may need to overwrite */
 	},
 	[it8716] = {
 		.name = "it8716",
-		.suffix = "F",
+		.model = "IT8716F",
 		.features = FEAT_16BIT_FANS | FEAT_TEMP_OFFSET | FEAT_VID
 		  | FEAT_FAN16_CONFIG | FEAT_FIVE_FANS | FEAT_PWM_FREQ2,
 	},
 	[it8718] = {
 		.name = "it8718",
-		.suffix = "F",
+		.model = "IT8718F",
 		.features = FEAT_16BIT_FANS | FEAT_TEMP_OFFSET | FEAT_VID
 		  | FEAT_TEMP_OLD_PECI | FEAT_FAN16_CONFIG | FEAT_FIVE_FANS
 		  | FEAT_PWM_FREQ2,
@@ -331,7 +331,7 @@ static const struct it87_devices it87_devices[] = {
 	},
 	[it8720] = {
 		.name = "it8720",
-		.suffix = "F",
+		.model = "IT8720F",
 		.features = FEAT_16BIT_FANS | FEAT_TEMP_OFFSET | FEAT_VID
 		  | FEAT_TEMP_OLD_PECI | FEAT_FAN16_CONFIG | FEAT_FIVE_FANS
 		  | FEAT_PWM_FREQ2,
@@ -339,7 +339,7 @@ static const struct it87_devices it87_devices[] = {
 	},
 	[it8721] = {
 		.name = "it8721",
-		.suffix = "F",
+		.model = "IT8721F",
 		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS
 		  | FEAT_TEMP_OFFSET | FEAT_TEMP_OLD_PECI | FEAT_TEMP_PECI
 		  | FEAT_FAN16_CONFIG | FEAT_FIVE_FANS | FEAT_IN7_INTERNAL
@@ -349,7 +349,7 @@ static const struct it87_devices it87_devices[] = {
 	},
 	[it8728] = {
 		.name = "it8728",
-		.suffix = "F",
+		.model = "IT8728F",
 		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS
 		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_FIVE_FANS
 		  | FEAT_IN7_INTERNAL | FEAT_PWM_FREQ2,
@@ -357,7 +357,7 @@ static const struct it87_devices it87_devices[] = {
 	},
 	[it8732] = {
 		.name = "it8732",
-		.suffix = "F",
+		.model = "IT8732F",
 		.features = FEAT_NEWER_AUTOPWM | FEAT_16BIT_FANS
 		  | FEAT_TEMP_OFFSET | FEAT_TEMP_OLD_PECI | FEAT_TEMP_PECI
 		  | FEAT_10_9MV_ADC | FEAT_IN7_INTERNAL,
@@ -366,7 +366,7 @@ static const struct it87_devices it87_devices[] = {
 	},
 	[it8771] = {
 		.name = "it8771",
-		.suffix = "E",
+		.model = "IT8771E",
 		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS
 		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_IN7_INTERNAL
 		  | FEAT_PWM_FREQ2,
@@ -378,7 +378,7 @@ static const struct it87_devices it87_devices[] = {
 	},
 	[it8772] = {
 		.name = "it8772",
-		.suffix = "E",
+		.model = "IT8772E",
 		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS
 		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_IN7_INTERNAL
 		  | FEAT_PWM_FREQ2,
@@ -390,28 +390,28 @@ static const struct it87_devices it87_devices[] = {
 	},
 	[it8781] = {
 		.name = "it8781",
-		.suffix = "F",
+		.model = "IT8781F",
 		.features = FEAT_16BIT_FANS | FEAT_TEMP_OFFSET
 		  | FEAT_TEMP_OLD_PECI | FEAT_FAN16_CONFIG | FEAT_PWM_FREQ2,
 		.old_peci_mask = 0x4,
 	},
 	[it8782] = {
 		.name = "it8782",
-		.suffix = "F",
+		.model = "IT8782F",
 		.features = FEAT_16BIT_FANS | FEAT_TEMP_OFFSET
 		  | FEAT_TEMP_OLD_PECI | FEAT_FAN16_CONFIG | FEAT_PWM_FREQ2,
 		.old_peci_mask = 0x4,
 	},
 	[it8783] = {
 		.name = "it8783",
-		.suffix = "E/F",
+		.model = "IT8783E/F",
 		.features = FEAT_16BIT_FANS | FEAT_TEMP_OFFSET
 		  | FEAT_TEMP_OLD_PECI | FEAT_FAN16_CONFIG | FEAT_PWM_FREQ2,
 		.old_peci_mask = 0x4,
 	},
 	[it8786] = {
 		.name = "it8786",
-		.suffix = "E",
+		.model = "IT8786E",
 		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS
 		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_IN7_INTERNAL
 		  | FEAT_PWM_FREQ2,
@@ -419,7 +419,7 @@ static const struct it87_devices it87_devices[] = {
 	},
 	[it8790] = {
 		.name = "it8790",
-		.suffix = "E",
+		.model = "IT8790E",
 		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS
 		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_IN7_INTERNAL
 		  | FEAT_PWM_FREQ2,
@@ -427,7 +427,7 @@ static const struct it87_devices it87_devices[] = {
 	},
 	[it8792] = {
 		.name = "it8792",
-		.suffix = "E",
+		.model = "IT8792E/IT8795E",
 		.features = FEAT_NEWER_AUTOPWM | FEAT_16BIT_FANS
 		  | FEAT_TEMP_OFFSET | FEAT_TEMP_OLD_PECI | FEAT_TEMP_PECI
 		  | FEAT_10_9MV_ADC | FEAT_IN7_INTERNAL,
@@ -436,7 +436,7 @@ static const struct it87_devices it87_devices[] = {
 	},
 	[it8603] = {
 		.name = "it8603",
-		.suffix = "E",
+		.model = "IT8603E",
 		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS
 		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_IN7_INTERNAL
 		  | FEAT_AVCC3 | FEAT_PWM_FREQ2,
@@ -444,7 +444,7 @@ static const struct it87_devices it87_devices[] = {
 	},
 	[it8620] = {
 		.name = "it8620",
-		.suffix = "E",
+		.model = "IT8620E",
 		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS
 		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_SIX_FANS
 		  | FEAT_IN7_INTERNAL | FEAT_SIX_PWM | FEAT_PWM_FREQ2
@@ -453,7 +453,7 @@ static const struct it87_devices it87_devices[] = {
 	},
 	[it8622] = {
 		.name = "it8622",
-		.suffix = "E",
+		.model = "IT8622E",
 		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS
 		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_FIVE_FANS
 		  | FEAT_FIVE_PWM | FEAT_IN7_INTERNAL | FEAT_PWM_FREQ2
@@ -462,7 +462,7 @@ static const struct it87_devices it87_devices[] = {
 	},
 	[it8628] = {
 		.name = "it8628",
-		.suffix = "E",
+		.model = "IT8628E",
 		.features = FEAT_NEWER_AUTOPWM | FEAT_12MV_ADC | FEAT_16BIT_FANS
 		  | FEAT_TEMP_OFFSET | FEAT_TEMP_PECI | FEAT_SIX_FANS
 		  | FEAT_IN7_INTERNAL | FEAT_SIX_PWM | FEAT_PWM_FREQ2
@@ -2520,8 +2520,8 @@ 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,
+	pr_info("Found %s chip at 0x%x, revision %d\n",
+		it87_devices[sio_data->type].model,
 		*address, sio_data->revision);
 
 	config = &it87_devices[sio_data->type];
-- 
2.39.1

[PATCH v1 4/7] hwmon: (it87) Add chip_id in some info message

[Frank Crawford]

In cases where there are multiple chips, print out which chip is
referred to, in the informational message.

Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
---
 drivers/hwmon/it87.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index 16394b38dce3..5ca8449887da 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -2505,13 +2505,15 @@ static int __init it87_find(int sioaddr, unsigned short *address,
 
 	superio_select(sioaddr, PME);
 	if (!(superio_inb(sioaddr, IT87_ACT_REG) & 0x01)) {
-		pr_info("Device not activated, skipping\n");
+		pr_info("Device (DEVID=0x%x) not activated, skipping\n",
+			chip_type);
 		goto exit;
 	}
 
 	*address = superio_inw(sioaddr, IT87_BASE_REG) & ~(IT87_EXTENT - 1);
 	if (*address == 0) {
-		pr_info("Base address not set, skipping\n");
+		pr_info("Base address not set (DEVID=0x%x), skipping\n",
+			chip_type);
 		goto exit;
 	}
 
-- 
2.39.1

Re: [PATCH v1 4/7] hwmon: (it87) Add chip_id in some info message

[Guenter Roeck]

On Thu, Jan 26, 2023 at 03:02:20PM +1100, Frank Crawford wrote:
> In cases where there are multiple chips, print out which chip is
> referred to, in the informational message.
> 
> Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
> ---
>  drivers/hwmon/it87.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index 16394b38dce3..5ca8449887da 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -2505,13 +2505,15 @@ static int __init it87_find(int sioaddr, unsigned short *address,
>  
>  	superio_select(sioaddr, PME);
>  	if (!(superio_inb(sioaddr, IT87_ACT_REG) & 0x01)) {
> -		pr_info("Device not activated, skipping\n");
> +		pr_info("Device (DEVID=0x%x) not activated, skipping\n",
> +			chip_type);

At this point the textual name (it87_devices[sio_data->type].model)
is already known. I would suggest to print that instead.

Thanks,
Guenter

>  		goto exit;
>  	}
>  
>  	*address = superio_inw(sioaddr, IT87_BASE_REG) & ~(IT87_EXTENT - 1);
>  	if (*address == 0) {
> -		pr_info("Base address not set, skipping\n");
> +		pr_info("Base address not set (DEVID=0x%x), skipping\n",
> +			chip_type);
>  		goto exit;
>  	}
>  
> -- 
> 2.39.1
> 

Re: [PATCH v1 4/7] hwmon: (it87) Add chip_id in some info message

[Frank Crawford]

On Thu, 2023-01-26 at 05:49 -0800, Guenter Roeck wrote:
> On Thu, Jan 26, 2023 at 03:02:20PM +1100, Frank Crawford wrote:
> > In cases where there are multiple chips, print out which chip is
> > referred to, in the informational message.
> > 
> > Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
> > ---
> >  drivers/hwmon/it87.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> > index 16394b38dce3..5ca8449887da 100644
> > --- a/drivers/hwmon/it87.c
> > +++ b/drivers/hwmon/it87.c
> > @@ -2505,13 +2505,15 @@ static int __init it87_find(int sioaddr,
> > unsigned short *address,
> >  
> >         superio_select(sioaddr, PME);
> >         if (!(superio_inb(sioaddr, IT87_ACT_REG) & 0x01)) {
> > -               pr_info("Device not activated, skipping\n");
> > +               pr_info("Device (DEVID=0x%x) not activated,
> > skipping\n",
> > +                       chip_type);
> 
> At this point the textual name (it87_devices[sio_data->type].model)
> is already known. I would suggest to print that instead.

You are right.  I'll update it to do so, although I also realised I
need to also print the sioaddr, so I can distinguish the case where
both chips are listed as the same.  This is most common when force_id
has been used.
> 
> Thanks,
> Guenter
> 
> >                 goto exit;
> >         }
> >  
> >         *address = superio_inw(sioaddr, IT87_BASE_REG) &
> > ~(IT87_EXTENT - 1);
> >         if (*address == 0) {
> > -               pr_info("Base address not set, skipping\n");
> > +               pr_info("Base address not set (DEVID=0x%x),
> > skipping\n",
> > +                       chip_type);
> >                 goto exit;
> >         }
> >  
> > -- 
> > 2.39.1
> > 

[PATCH v1 5/7] hwmon: (it87) Allow multiple chip IDs for force_id

[Frank Crawford]

Extend the force_id module parameter to allow specifying one or both
chip IDs separately.  If only a single parameter is given it defaults
to using that value for all chips, similar to previous usage.

Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
---
 drivers/hwmon/it87.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index 5ca8449887da..e8eeedd23bee 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -175,8 +175,9 @@ static inline void superio_exit(int ioreg, bool doexit)
 #define IT87_SIO_VID_REG	0xfc	/* VID value */
 #define IT87_SIO_BEEP_PIN_REG	0xf6	/* Beep pin mapping */
 
-/* Force chip ID to specified value. Should only be used for testing */
-static unsigned short force_id;
+/* Force chip IDs to specified values. Should only be used for testing */
+static unsigned short force_id[2];
+static unsigned int force_id_cnt;
 
 /* ACPI resource conflicts are ignored if this parameter is set to 1 */
 static bool ignore_resource_conflict;
@@ -2405,7 +2406,7 @@ static const struct attribute_group it87_group_auto_pwm = {
 
 /* SuperIO detection - will change isa_address if a chip is found */
 static int __init it87_find(int sioaddr, unsigned short *address,
-			    struct it87_sio_data *sio_data)
+			    struct it87_sio_data *sio_data, int chip_cnt)
 {
 	int err;
 	u16 chip_type;
@@ -2422,8 +2423,12 @@ static int __init it87_find(int sioaddr, unsigned short *address,
 	if (chip_type == 0xffff)
 		goto exit;
 
-	if (force_id)
-		chip_type = force_id;
+	if (force_id_cnt == 1) {
+		/* If only one value given use for all chips */
+		if (force_id[0])
+			chip_type = force_id[0];
+	} else if (force_id[chip_cnt])
+		chip_type = force_id[chip_cnt];
 
 	switch (chip_type) {
 	case IT8705F_DEVID:
@@ -3426,7 +3431,7 @@ static int __init sm_it87_init(void)
 	for (i = 0; i < ARRAY_SIZE(sioaddr); i++) {
 		memset(&sio_data, 0, sizeof(struct it87_sio_data));
 		isa_address[i] = 0;
-		err = it87_find(sioaddr[i], &isa_address[i], &sio_data);
+		err = it87_find(sioaddr[i], &isa_address[i], &sio_data, i);
 		if (err || isa_address[i] == 0)
 			continue;
 		/*
@@ -3475,8 +3480,8 @@ static void __exit sm_it87_exit(void)
 MODULE_AUTHOR("Chris Gauthron, Jean Delvare <jdelvare@suse.de>");
 MODULE_DESCRIPTION("IT8705F/IT871xF/IT872xF hardware monitoring driver");
 
-module_param(force_id, ushort, 0);
-MODULE_PARM_DESC(force_id, "Override the detected device ID");
+module_param_array(force_id, ushort, &force_id_cnt, 0);
+MODULE_PARM_DESC(force_id, "Override one or more detected device ID(s)");
 
 module_param(ignore_resource_conflict, bool, 0);
 MODULE_PARM_DESC(ignore_resource_conflict, "Ignore ACPI resource conflict");
-- 
2.39.1

[PATCH v1 6/7] hwmon: (it87) Add new chipset IT87952E

[Frank Crawford]

Add IT87952E, a new chip ID (DEV ID 0x8695), which appears to be an
updated version of the IT8792E and takes the same configuration.

The model name comes from the model name printed on the chip.

There is no datasheet publicly available.

Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
---
 drivers/hwmon/it87.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
index e8eeedd23bee..85ecc61dbf9e 100644
--- a/drivers/hwmon/it87.c
+++ b/drivers/hwmon/it87.c
@@ -34,6 +34,7 @@
  *            IT8786E  Super I/O chip w/LPC interface
  *            IT8790E  Super I/O chip w/LPC interface
  *            IT8792E  Super I/O chip w/LPC interface
+ *            IT87952E  Super I/O chip w/LPC interface
  *            Sis950   A clone of the IT8705F
  *
  *  Copyright (C) 2001 Chris Gauthron
@@ -63,7 +64,7 @@
 
 enum chips { it87, it8712, it8716, it8718, it8720, it8721, it8728, it8732,
 	     it8771, it8772, it8781, it8782, it8783, it8786, it8790,
-	     it8792, it8603, it8620, it8622, it8628 };
+	     it8792, it8603, it8620, it8622, it8628, it87952 };
 
 static struct platform_device *it87_pdev[2];
 
@@ -160,6 +161,7 @@ static inline void superio_exit(int ioreg, bool doexit)
 #define IT8622E_DEVID 0x8622
 #define IT8623E_DEVID 0x8623
 #define IT8628E_DEVID 0x8628
+#define IT87952E_DEVID 0x8695
 #define IT87_ACT_REG  0x30
 #define IT87_BASE_REG 0x60
 
@@ -470,6 +472,15 @@ static const struct it87_devices it87_devices[] = {
 		  | FEAT_SIX_TEMP | FEAT_VIN3_5V,
 		.peci_mask = 0x07,
 	},
+	[it87952] = {
+		.name = "it87952",
+		.model = "IT87952E",
+		.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 */
+	},
 };
 
 #define has_16bit_fans(data)	((data)->features & FEAT_16BIT_FANS)
@@ -2501,6 +2512,10 @@ static int __init it87_find(int sioaddr, unsigned short *address,
 	case IT8628E_DEVID:
 		sio_data->type = it8628;
 		break;
+	case IT87952E_DEVID:
+		sio_data->type = it87952;
+		doexit = false;	/* See IT8792E comment above */
+		break;
 	case 0xffff:	/* No device at all */
 		goto exit;
 	default:
-- 
2.39.1

Re: [PATCH v1 6/7] hwmon: (it87) Add new chipset IT87952E

[Guenter Roeck]

On Thu, Jan 26, 2023 at 03:02:22PM +1100, Frank Crawford wrote:
> Add IT87952E, a new chip ID (DEV ID 0x8695), which appears to be an
> updated version of the IT8792E and takes the same configuration.
> 
> The model name comes from the model name printed on the chip.
> 
> There is no datasheet publicly available.
> 
> Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
> ---
>  drivers/hwmon/it87.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index e8eeedd23bee..85ecc61dbf9e 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -34,6 +34,7 @@
>   *            IT8786E  Super I/O chip w/LPC interface
>   *            IT8790E  Super I/O chip w/LPC interface
>   *            IT8792E  Super I/O chip w/LPC interface
> + *            IT87952E  Super I/O chip w/LPC interface
>   *            Sis950   A clone of the IT8705F
>   *
>   *  Copyright (C) 2001 Chris Gauthron
> @@ -63,7 +64,7 @@
>  
>  enum chips { it87, it8712, it8716, it8718, it8720, it8721, it8728, it8732,
>  	     it8771, it8772, it8781, it8782, it8783, it8786, it8790,
> -	     it8792, it8603, it8620, it8622, it8628 };
> +	     it8792, it8603, it8620, it8622, it8628, it87952 };
>  
>  static struct platform_device *it87_pdev[2];
>  
> @@ -160,6 +161,7 @@ static inline void superio_exit(int ioreg, bool doexit)
>  #define IT8622E_DEVID 0x8622
>  #define IT8623E_DEVID 0x8623
>  #define IT8628E_DEVID 0x8628
> +#define IT87952E_DEVID 0x8695
>  #define IT87_ACT_REG  0x30
>  #define IT87_BASE_REG 0x60
>  
> @@ -470,6 +472,15 @@ static const struct it87_devices it87_devices[] = {
>  		  | FEAT_SIX_TEMP | FEAT_VIN3_5V,
>  		.peci_mask = 0x07,
>  	},
> +	[it87952] = {
> +		.name = "it87952",
> +		.model = "IT87952E",
> +		.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 */
> +	},
>  };
>  
>  #define has_16bit_fans(data)	((data)->features & FEAT_16BIT_FANS)
> @@ -2501,6 +2512,10 @@ static int __init it87_find(int sioaddr, unsigned short *address,
>  	case IT8628E_DEVID:
>  		sio_data->type = it8628;
>  		break;
> +	case IT87952E_DEVID:
> +		sio_data->type = it87952;
> +		doexit = false;	/* See IT8792E comment above */
> +		break;

Is that true for this chip as well ? I would have hoped they fix
the problem for an updated version of the same chip.

Thanks,
Guenter

>  	case 0xffff:	/* No device at all */
>  		goto exit;
>  	default:
> -- 
> 2.39.1
> 

Re: [PATCH v1 6/7] hwmon: (it87) Add new chipset IT87952E

[Frank Crawford]

On Thu, 2023-01-26 at 05:53 -0800, Guenter Roeck wrote:
> On Thu, Jan 26, 2023 at 03:02:22PM +1100, Frank Crawford wrote:
> > Add IT87952E, a new chip ID (DEV ID 0x8695), which appears to be an
> > updated version of the IT8792E and takes the same configuration.
> > 
> > The model name comes from the model name printed on the chip.
> > 
> > There is no datasheet publicly available.
> > 
> > Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
> > ---
> >  drivers/hwmon/it87.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> > index e8eeedd23bee..85ecc61dbf9e 100644
> > --- a/drivers/hwmon/it87.c
> > +++ b/drivers/hwmon/it87.c
> > @@ -34,6 +34,7 @@
> >   *            IT8786E  Super I/O chip w/LPC interface
> >   *            IT8790E  Super I/O chip w/LPC interface
> >   *            IT8792E  Super I/O chip w/LPC interface
> > + *            IT87952E  Super I/O chip w/LPC interface
> >   *            Sis950   A clone of the IT8705F
> >   *
> >   *  Copyright (C) 2001 Chris Gauthron
> > @@ -63,7 +64,7 @@
> >  
> >  enum chips { it87, it8712, it8716, it8718, it8720, it8721, it8728,
> > it8732,
> >              it8771, it8772, it8781, it8782, it8783, it8786,
> > it8790,
> > -            it8792, it8603, it8620, it8622, it8628 };
> > +            it8792, it8603, it8620, it8622, it8628, it87952 };
> >  
> >  static struct platform_device *it87_pdev[2];
> >  
> > @@ -160,6 +161,7 @@ static inline void superio_exit(int ioreg, bool
> > doexit)
> >  #define IT8622E_DEVID 0x8622
> >  #define IT8623E_DEVID 0x8623
> >  #define IT8628E_DEVID 0x8628
> > +#define IT87952E_DEVID 0x8695
> >  #define IT87_ACT_REG  0x30
> >  #define IT87_BASE_REG 0x60
> >  
> > @@ -470,6 +472,15 @@ static const struct it87_devices
> > it87_devices[] = {
> >                   | FEAT_SIX_TEMP | FEAT_VIN3_5V,
> >                 .peci_mask = 0x07,
> >         },
> > +       [it87952] = {
> > +               .name = "it87952",
> > +               .model = "IT87952E",
> > +               .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 */
> > +       },
> >  };
> >  
> >  #define has_16bit_fans(data)   ((data)->features &
> > FEAT_16BIT_FANS)
> > @@ -2501,6 +2512,10 @@ static int __init it87_find(int sioaddr,
> > unsigned short *address,
> >         case IT8628E_DEVID:
> >                 sio_data->type = it8628;
> >                 break;
> > +       case IT87952E_DEVID:
> > +               sio_data->type = it87952;
> > +               doexit = false; /* See IT8792E comment above */
> > +               break;
> 
> Is that true for this chip as well ? I would have hoped they fix
> the problem for an updated version of the same chip.

Unfortunately my testing has been through others for this chip, and the
effects have been similar to the IT8792E, i.e. sometimes found
sometimes not.  I can't be sure if the testers did something wrong, but
I felt it was better to assume that part of the firmware is unchanged
and still contains the same bug.
> 
> Thanks,
> Guenter

Regards
Frank
> 
> >         case 0xffff:    /* No device at all */
> >                 goto exit;
> >         default:
> > -- 
> > 2.39.1
> > 

[PATCH v1 7/7] hwmon: (it87) Updated documentation for recent updates to it87

[Frank Crawford]

Updated the driver documentation to list all current chips and
describe the module parameters as per descriptions in the code.

Signed-off-by: Frank Crawford <frank@crawford.emu.id.au>
---
 Documentation/hwmon/it87.rst | 47 ++++++++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 5 deletions(-)

diff --git a/Documentation/hwmon/it87.rst b/Documentation/hwmon/it87.rst
index 2d83f23bee93..5cef4f265000 100644
--- a/Documentation/hwmon/it87.rst
+++ b/Documentation/hwmon/it87.rst
@@ -145,6 +145,22 @@ Supported chips:
 
     Datasheet: Not publicly available
 
+  * IT8792E/IT8795E
+
+    Prefix: 'it8792'
+
+    Addresses scanned: from Super I/O config space (8 I/O ports)
+
+    Datasheet: Not publicly available
+
+  * IT87952E
+
+    Prefix: 'it87952'
+
+    Addresses scanned: from Super I/O config space (8 I/O ports)
+
+    Datasheet: Not publicly available
+
   * SiS950   [clone of IT8705F]
 
     Prefix: 'it87'
@@ -162,7 +178,7 @@ Authors:
 Module Parameters
 -----------------
 
-* update_vbat: int
+* update_vbat bool
     0 if vbat should report power on value, 1 if vbat should be updated after
     each read. Default is 0. On some boards the battery voltage is provided
     by either the battery or the onboard power supply. Only the first reading
@@ -171,11 +187,31 @@ Module Parameters
     the chip so can be read at any time. Excessive reading may decrease
     battery life but no information is given in the datasheet.
 
-* fix_pwm_polarity int
+* fix_pwm_polarity bool
     Force PWM polarity to active high (DANGEROUS). Some chips are
     misconfigured by BIOS - PWM values would be inverted. This option tries
     to fix this. Please contact your BIOS manufacturer and ask him for fix.
 
+* force_id short, short
+
+  Force multiple chip ID to specified value, separated by ','.
+  For example "force_id=0x8689,0x8633".  A value of 0 is ignored
+  for that chip.
+  Note: A single force_id value (e.g. "force_id=0x8689") is used for
+  all chips, to only set the first chip use "force_id=0x8689,0".
+  Should only be used for testing.
+
+* ignore_resource_conflict bool
+
+  Similar to acpi_enforce_resources=lax, but only affects this driver.
+  ACPI resource conflicts are ignored if this parameter is provided and
+  set to 1.
+  Provided since there are reports that system-wide acpi_enfore_resources=lax
+  can result in boot failures on some systems.
+  Note: This is inherently risky since it means that both ACPI and this driver
+  may access the chip at the same time. This can result in race conditions and,
+  worst case, result in unexpected system reboots.
+
 
 Hardware Interfaces
 -------------------
@@ -193,8 +229,8 @@ Description
 
 This driver implements support for the IT8603E, IT8620E, IT8623E, IT8628E,
 IT8705F, IT8712F, IT8716F, IT8718F, IT8720F, IT8721F, IT8726F, IT8728F, IT8732F,
-IT8758E, IT8771E, IT8772E, IT8781F, IT8782F, IT8783E/F, IT8786E, IT8790E, and
-SiS950 chips.
+IT8758E, IT8771E, IT8772E, IT8781F, IT8782F, IT8783E/F, IT8786E, IT8790E,
+IT8792E/IT8795E, IT87952E and SiS950 chips.
 
 These chips are 'Super I/O chips', supporting floppy disks, infrared ports,
 joysticks and other miscellaneous stuff. For hardware monitoring, they
@@ -238,7 +274,8 @@ of the fan is not supported (value 0 of pwmX_enable).
 The IT8620E and IT8628E are custom designs, hardware monitoring part is similar
 to IT8728F. It only supports 16-bit fan mode. Both chips support up to 6 fans.
 
-The IT8790E supports up to 3 fans. 16-bit fan mode is always enabled.
+The IT8790E, IT8792E/IT8795E and IT87952E support 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.
-- 
2.39.1